sign.verify_record: harden max_age_seconds/max_future_skew_seconds validation to match provenance.py

CHANGELOG.md

@@ -13,6 +13,8 @@ Format: [Semantic Versioning](https://semver.org/). Spec versions follow `MAJOR.
 
 ### Fixed
 
+- **`sign.verify_record()` now validates `max_age_seconds` and `max_future_skew_seconds` the same way `provenance.verify_record()` does.** `provenance.verify_record()` rejects a malformed freshness bound via `_check_seconds()`, added per the review on #164. `sign.verify_record()`, the original Trust Record verifier, never received the same hardening: it checked only `max_future_skew_seconds < 0`, and `max_age_seconds` was compared against unvalidated. Passing `max_age_seconds=-1` — a value a caller might use meaning "no bound," since `None` is the documented way to disable the check — rejected every record, including one issued the same second, as `record is stale`, naming the record rather than the misconfigured argument. `_check_seconds()` is now defined once in `sign.py` and shared: `sign.verify_record()` calls it directly, and `provenance.verify_record()` imports it, passing its own `ProvenanceError` via a new `exc` parameter so each keeps its existing public error type.
+
 - **`provenance.tool_catalog_hash()` now refuses a malformed `tools` list instead of crashing.** Reached primarily through `check_tool_catalog(record, tools)`, the function the module's own docstring calls "the step that catches a live attack," because `tools` there is what the MCP server actually returned, i.e. the untrusted party this check exists to catch. The function iterated `tools` and called `.get(...)` on each entry with no check that `tools` was a list or that its entries were objects, so a malformed response (an entry that is a string, `None`, a number, or `tools` itself not being a list) raised an unhandled `AttributeError` or `TypeError` instead of the documented `ProvenanceError`. A server that is misbehaving maliciously or just buggily and is exactly the source `tools` has no reason to trust its shape. Fixed with an explicit `isinstance` check on `tools` and on each of its entries, naming the offending index.
 
 - **`content_marking.build_assertion()` and `content_marking.verify_assertion()` now refuse record bytes that aren't a JSON object, instead of crashing.** Both functions called `.get(...)` on the result of `json.loads(record_bytes)` without checking it was a dict first. Valid JSON is not always an object that is an array, a string, a number, `null`, and a bool are all valid top-level JSON  and `record_bytes` is exactly the kind of externally-sourced input this is likely to happen to: `build_assertion()` takes whatever bytes a caller hands it, and `verify_assertion()`'s docstring is explicit that its `record_bytes` are "the record bytes actually retrieved from its URL," i.e. a network response the caller does not control. Either function raised an unhandled `AttributeError` instead of the documented `ContentMarkingError`. `verify_assertion()` had a second, related gap: its second parse of `record_bytes` (after the hash check) was not wrapped in the try/except its first parse-adjacent check uses, so genuinely malformed (non-JSON) bytes that happened to hash-match raised `json.JSONDecodeError` instead of `ContentMarkingError` too. Both functions now check `isinstance(record, dict)` after parsing, and `verify_assertion()`'s second parse now catches `ValueError` the same way its hash-computation path already implicitly required valid bytes to reach.

src/agentrust_trace/sign.py

@@ -276,7 +276,28 @@ def _b64url_decode(value: str, *, field: str) -> bytes:
         return base64.urlsafe_b64decode(padded)
     except (binascii.Error, ValueError) as exc:
         raise ValueError(f"{field} is not valid base64url: {exc}") from exc
-
+def _check_seconds(
+    name: str, value: Any, *, optional: bool = False, exc: type[Exception] = ValueError
+) -> None:
+    """Reject a malformed freshness-policy input instead of silently acting on it.
+
+    A verifier's age/skew policy is configuration, and a wrong one fails in the
+    direction that matters: ``max_age_seconds=-1`` is not a stricter bound, it
+    classifies every record ever issued as stale, and a caller who meant to
+    disable the bound (``None``) would see a uniform refusal with no error
+    naming the cause. ``bool`` is excluded explicitly because it is a subclass
+    of ``int`` in Python, so ``True`` would otherwise pass as one second.
+
+    Shared by :func:`sign.verify_record` and :func:`provenance.verify_record`,
+    which pass their own exception type via *exc* so each keeps its existing
+    public error type (``ValueError`` and ``ProvenanceError`` respectively).
+    """
+    if optional and value is None:
+        return
+    if isinstance(value, bool) or not isinstance(value, int):
+        raise exc(f"{name} must be an integer, got {type(value).__name__}")
+    if value < 0:
+        raise exc(f"{name} must be non-negative, got {value}")
 
 def sign_record(record: dict[str, Any], key: Ed25519PrivateKey) -> dict[str, Any]:
     """Return a copy of *record* with ``cnf.jwk`` populated and a ``signature`` field added.
@@ -486,8 +507,10 @@ def verify_record(
         )
 
     # Freshness: bound the age of the record against its issued-at timestamp.
-    if max_future_skew_seconds < 0:
-        raise ValueError("max_future_skew_seconds must be non-negative")
+    # Both bounds are verifier configuration, not record data, and a malformed
+    # one is checked before it is used -- see `_check_seconds`.
+    _check_seconds("max_future_skew_seconds", max_future_skew_seconds)
+    _check_seconds("max_age_seconds", max_age_seconds, optional=True)
     iat = record.get("iat")
     if not isinstance(iat, int) or isinstance(iat, bool):
         raise ValueError("record has no valid integer 'iat' for freshness check")