diff --git a/CHANGELOG.md b/CHANGELOG.md index 301ffbe..0e2ea7d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,8 @@ Format: [Semantic Versioning](https://semver.org/). Spec versions follow `MAJOR. ### Fixed +- **`provenance.build_record()` coerced an explicitly supplied `issued_at` before the validator that exists to inspect it ever ran.** `stamped_at = int(issued_at if issued_at is not None else time.time())` handed the converted value to `_check_structure()`, whose guard carries the comment "bool is an int subclass, and True would otherwise pass as a timestamp". The order defeated that guard: `True` arrived as `1`, `False` as `0`, `1.9` as `1`, `"123"` as `123`, and `-0.5` as `0`, so every one of them satisfied the non-negative-integer test and was written into the record. The last case is the diagnostic one, since a negative non-integer became an accepted non-negative timestamp. The same line leaked two exception classes the module does not document: `issued_at=[1]` left `build_record` as a `TypeError` and `issued_at="abc"` as a `ValueError`, where every other public function in the module is held to `ProvenanceError`. An explicitly supplied value now reaches `_check_structure()` untouched and only an omitted one is stamped with `int(time.time())`, which puts `isinstance` in front of the conversion and closes both at once. This is distinct from #142 and #146: those moved the structural rules into the shared helper, and the helper was always strict. The caller path defeated it by normalizing first. Nothing that used to produce a valid record stops doing so; a valid integer is carried through unchanged and an omitted value is still stamped. `tests/test_public_functions_raise_what_they_document.py` listed `provenance.build_record` under `NO_ARGUMENT_TO_SWEEP` because it has no positional argument, which is why the junk matrix never reached it, so the function is now wired into that sweep with `issued_at` as the varied argument and a witness that pins the sweep actually arrives. Reported by @altrudev in #320, with the two undocumented exception classes and the reason the sweep never saw them found by @lywinged. + - **`intent_bridge.verify_bridge()` compared the required transcript's call to the executed call with host-language equality, which is not the identity relation the bridge digests under.** Every other comparison in that function is over RFC 8785 canonical bytes, but the transcript binding used `before.get("tool_call") != tool_call`. Python holds `True == 1` and `False == 0`, nested objects included, so a transcript whose `before.tool_call` substituted a boolean for the corresponding integer (or the reverse) had different JCS bytes from the call the authorization digested and was still reported as bound to the execution. The signed `tool_call_digest` was never affected: it is checked against the actual `tool_call`, so the executed call could not differ from the authorized one. What could differ was the separately supplied transcript, in the one place whose purpose is to show that the two agree. The comparison is now `compare_digest` over `digest_jcs`, reusing the digest already computed for the `tool_call_digest` check. The isinstance guard stays in front of it, and a `transcript.before.tool_call` that JCS has no form for raises `AuthorizationMismatch` rather than `IntentBridgeError`, so the documented result class for a malformed transcript is unchanged. Regression coverage pins all four substitutions with an assertion that each one is Python-equal, so a test that stopped exercising the defect would fail rather than pass quietly, plus an unchanged-call control and a parametrized check that a non-object transcript call keeps its original exception class. No wire format, schema, scope, digest definition, or normative bridge semantics change. Reported by @altrudev in #317. - **`intent_bridge.verify_bridge()` now refuses malformed signed decision values instead of classifying every non-`allow` value as a policy denial.** The bridge schema permits only the literal strings `"allow"` and `"deny"`, while the runtime previously used `decision != "allow"` as its branch, so re-signed values such as `true`, `1`, `null`, `""`, and `"reject"` all surfaced as `AuthorizationDenied`. That conflated malformed producer output with a legitimate signed denial. The verifier now establishes the enum explicitly before allow/deny semantics; only the literal valid `"deny"` reaches `AuthorizationDenied`, while malformed values raise `IntentBridgeError`. The signature, trust-key, scope, digest, transcript, and wire-format rules are unchanged. diff --git a/src/agentrust_trace/provenance.py b/src/agentrust_trace/provenance.py index 08fe9a3..fdaff01 100644 --- a/src/agentrust_trace/provenance.py +++ b/src/agentrust_trace/provenance.py @@ -235,7 +235,12 @@ def build_record( "a record needs artifact identity, endpoint identity, or both. One with " "neither identifies nothing." ) - stamped_at = int(issued_at if issued_at is not None else time.time()) + # An explicitly supplied value reaches _check_structure untouched: coercing first + # defeats the guard there, whose whole subject is what the caller actually passed + # (#320). int() on a bool, a float, or a numeric string yields something the + # isinstance test then accepts, and int() on anything else raises a class this + # module does not document. + stamped_at = issued_at if issued_at is not None else int(time.time()) _check_structure( kind=kind, artifact=artifact, diff --git a/tests/test_provenance.py b/tests/test_provenance.py index 95f9320..991fe4f 100644 --- a/tests/test_provenance.py +++ b/tests/test_provenance.py @@ -763,3 +763,46 @@ def test_the_record_type_is_checked_before_the_record_is_read() -> None: with pytest.raises(ProvenanceError) as excinfo: verify_record([], key_to_jwk(key)) assert "unknown format" not in str(excinfo.value) + + +# #320: `build_record` coerced an explicitly supplied `issued_at` with `int()` before +# handing it to `_check_structure`, so the guard there never saw what the caller passed. +# Its own comment says what it is for: "bool is an int subclass, and True would otherwise +# pass as a timestamp". The order defeated it. +@pytest.mark.parametrize( + ("supplied", "was_coerced_to"), + [ + (True, 1), + (False, 0), + (1.9, 1), + ("123", 123), + (-0.5, 0), + ], +) +def test_an_explicitly_supplied_issued_at_is_validated_before_it_is_coerced( + supplied: object, was_coerced_to: int +) -> None: + assert int(supplied) == was_coerced_to, "the coercion this pins must still be the one" + with pytest.raises(ProvenanceError, match="issued_at"): + _record(issued_at=supplied) + + +@pytest.mark.parametrize("supplied", [[1], "abc", b"7", {"t": 1}, float("nan")]) +def test_an_unconvertible_issued_at_raises_what_the_module_documents( + supplied: object, +) -> None: + """`int()` raised `TypeError` or `ValueError` here, which no caller written against + this module's contract catches.""" + with pytest.raises(ProvenanceError, match="issued_at"): + _record(issued_at=supplied) + + +def test_a_valid_issued_at_is_still_carried_through_unchanged() -> None: + assert _record(issued_at=1_754_000_000)["issued_at"] == 1_754_000_000 + + +def test_an_omitted_issued_at_is_still_stamped_with_the_current_time() -> None: + before = int(time.time()) + stamped = _record()["issued_at"] + assert isinstance(stamped, int) and not isinstance(stamped, bool) + assert before <= stamped <= int(time.time()) diff --git a/tests/test_public_functions_raise_what_they_document.py b/tests/test_public_functions_raise_what_they_document.py index 2a4fb30..f09a41f 100644 --- a/tests/test_public_functions_raise_what_they_document.py +++ b/tests/test_public_functions_raise_what_they_document.py @@ -74,6 +74,13 @@ lambda v: content_marking.verify_assertion(v, _RECORD_BYTES), "intent_bridge.digest_jcs": intent_bridge.digest_jcs, "intent_bridge.sign_bridge": lambda v: intent_bridge.sign_bridge(v, _KEY), + # #320: every argument is keyword-only, which is why this was listed as + # unsweepable and why a coercion in front of the validator went unnoticed. + # Only issued_at varies; the rest are valid so a refusal can only come from it. + "provenance.build_record": lambda v: provenance.build_record( + kind="publisher-asserted", publisher="did:web:example.com", tools=[], + artifact={"package": "x", "digest": "sha256:" + "a" * 64}, issued_at=v, + ), "provenance.check_tool_catalog": lambda v: provenance.check_tool_catalog(v, []), "provenance.sign_record": lambda v: provenance.sign_record(v, _KEY), "provenance.tool_catalog_hash": provenance.tool_catalog_hash, @@ -98,7 +105,6 @@ #: somebody remembered. NO_ARGUMENT_TO_SWEEP = { "sign.generate_key", "sign.load_signing_key", - "provenance.build_record", "intent_bridge.verify_bridge", # every argument is keyword-only and required } @@ -172,6 +178,7 @@ def test_no_public_function_raises_an_undocumented_exception(name: str) -> None: "content_marking.verify_assertion": (None, "ContentMarkingError"), "intent_bridge.digest_jcs": ("a-string", "IntentBridgeError"), "intent_bridge.sign_bridge": ({"k": float("nan")}, "IntentBridgeError"), + "provenance.build_record": (True, "ProvenanceError"), "provenance.check_tool_catalog": (None, "ProvenanceError"), "provenance.sign_record": (None, "ProvenanceError"), "provenance.tool_catalog_hash": (None, "ProvenanceError"),