Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
7 changes: 6 additions & 1 deletion src/agentrust_trace/provenance.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
43 changes: 43 additions & 0 deletions tests/test_provenance.py
Original file line number Diff line number Diff line change
Expand Up @@ -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())
9 changes: 8 additions & 1 deletion tests/test_public_functions_raise_what_they_document.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
}

Expand Down Expand Up @@ -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"),
Expand Down