Skip to content

fix(provenance): validate an explicit issued_at before coercing it - #334

Merged
imran-siddique merged 1 commit into
mainfrom
fix/provenance-issued-at-validate-before-coerce
Sep 12, 2026
Merged

fix(provenance): validate an explicit issued_at before coercing it#334
imran-siddique merged 1 commit into
mainfrom
fix/provenance-issued-at-validate-before-coerce

Conversation

@imran-siddique

Copy link
Copy Markdown
Member

Closes #320.

build_record() ran int(issued_at if issued_at is not None else time.time()) at provenance.py:238 and handed the converted value to _check_structure(), whose guard at line 202 carries the comment "bool is an int subclass, and True would otherwise pass as a timestamp". The order defeated the guard, which can only speak about what the caller passed.

Reproduced at 57d5165, with the two controls that have to keep working:

ACCEPTED  issued_at=True     -> record issued_at=1
ACCEPTED  issued_at=False    -> record issued_at=0
ACCEPTED  issued_at=1.9      -> record issued_at=1
ACCEPTED  issued_at='123'    -> record issued_at=123
ACCEPTED  issued_at=-0.5     -> record issued_at=0
TypeError  issued_at=[1]       (undocumented class)
ValueError  issued_at='abc'    (undocumented class)
ACCEPTED  issued_at=None     -> record issued_at=1789187091
ACCEPTED  issued_at=123      -> record issued_at=123

-0.5 becoming an accepted 0 is the diagnostic case: a negative non-integer turned into a valid-looking timestamp. The two undocumented exception classes come from the same line, and no caller written against this module's contract catches them.

Change

One line. An explicitly supplied value reaches _check_structure() untouched; only an omitted one is stamped with int(time.time()). That puts isinstance in front of the conversion, so the five coercions and the two leaked classes close together.

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.

Why nothing caught it

tests/test_public_functions_raise_what_they_document.py listed provenance.build_record under NO_ARGUMENT_TO_SWEEP because the function has no positional argument, so the junk matrix never reached it. It is now in CALLS with issued_at as the varied argument and every other argument valid, per that file's own warning about a second-argument TypeError reading like a leak in the first, plus a REACHES witness so a sweep that never arrives cannot report clean.

Verified in both directions

  • With the fix: tests/test_provenance.py 127 passed, the sweep file 58 passed, repository 1429 passed.
  • Against origin/main's provenance.py: 10 of the new provenance cases fail, and the sweep reports both test_no_public_function_raises_an_undocumented_exception[provenance.build_record] and test_the_sweep_actually_reaches_each_function[provenance.build_record].
  • The parametrized coercion test asserts int(supplied) == was_coerced_to before asserting the refusal, so if the coercion this pins ever stops being the one, the test fails rather than passing quietly.
  • The four repository-wide failures in test_generators_reproduce_fixtures.py and test_safe_integer_range.py are pre-existing Windows path-separator artifacts, confirmed on a clean origin/main worktree.

Reported by @altrudev. The two undocumented exception classes and the reason the sweep never saw them were found by @lywinged.

🤖 Generated with Claude Code

https://claude.ai/code/session_01X27MBo6tGVw1UmmchuGFnW

`build_record()` ran `int(issued_at ...)` and handed the result to
`_check_structure()`, whose guard carries the comment "bool is an int
subclass, and True would otherwise pass as a timestamp". The order defeated
it: True arrived as 1, False as 0, 1.9 as 1, "123" as 123, and -0.5 as 0, so
all five satisfied the non-negative-integer test and were written into the
record. A negative non-integer becoming an accepted timestamp is the
diagnostic case.

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 here 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.

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. It is now wired into
that sweep with issued_at as the varied argument, with a witness pinning that
the sweep arrives.

Reported by @altrudev in #320. The two undocumented exception classes and the
reason the sweep never saw them were found by @lywinged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X27MBo6tGVw1UmmchuGFnW
@imran-siddique
imran-siddique requested review from a team and lywinged as code owners September 12, 2026 04:30
@imran-siddique
imran-siddique merged commit 0aefa1f into main Sep 12, 2026
9 checks passed
@imran-siddique
imran-siddique deleted the fix/provenance-issued-at-validate-before-coerce branch September 12, 2026 04:31
lywinged added a commit to lywinged/trace-spec that referenced this pull request Sep 12, 2026
…ep was still reporting

Rebased onto `c04d938`. agentrust-io#334 merged the reordering half of agentrust-io#320 and closed the
issue, and the strict marker here did exactly what it is for: on the rebase,
`test_no_keyword_argument_leaks_an_undocumented_exception[provenance.build_record.issued_at]`
went `XPASS(strict)` and the file failed rather than going quietly green.

The second case filed under the same entry did not flip.
`test_what_a_producer_accepts_its_own_verifier_accepts` asks whether a producer
emits anything its own verifier refuses, and for `issued_at` it still reported one
value, `10000000000000000000`. That is the half agentrust-io#334 did not carry: `_check_structure`
had no upper bound, so `build_record` accepted a timestamp `sign_record` cannot
canonicalise and the caller met `rfc8785`'s `IntegerDomainError` rather than the
`ProvenanceError` this module documents. Proposed on agentrust-io#320 on 09-10 and carried on
agentrust-io#323, which is now superseded on everything else.

`_check_structure` gains the bound, reading `JCS_SAFE_INTEGER` from `sign.py`
rather than writing the number a third time, and `LEAKS_FILED` is empty. The
comment above it records what the entry held and why it went, so the marker's two
firings are not lost with it.

`_check_structure` is shared, so this reaches `verify_record` too. Under the
default freshness policy a record carrying such a timestamp was already refused as
dated in the future, so no record that verified before is refused now. What changes
is the caller who widens `max_future_skew_seconds` past the gap: that returned
`rfc8785.IntegerDomainError` out of a function documented to raise
`ProvenanceError`, and now returns `ProvenanceError`. A test pins it.

The boundary is asserted against the canonicalizer rather than against a literal:
each of the four cases checks `build_record` and `rfc8785.dumps` agree about
whether the value exists, so the guard cannot drift away from the thing it is
guarding.

Verified in both directions. With the clause: 1547 passed, 1 skipped, and the
sweep at 152 passed with nothing xfailed. With only the clause reverted: 4 fail,
being the two out-of-range boundary cases, the verifier case, and the sweep's own
producer-and-verifier case, which is the instrument that reported the gap now
confirming it is closed. `ruff`, `mypy` and `tools/check_dashes.py` pass. No
`CHANGELOG.md` hunk, per the instruction on 09-11.

Signed-off-by: LouieLuNZ <48041247+lywinged@users.noreply.github.com>
lywinged added a commit to lywinged/trace-spec that referenced this pull request Sep 12, 2026
…ep was still reporting

Rebased onto `c04d938`. agentrust-io#334 merged the reordering half of agentrust-io#320 and closed the
issue, and the strict marker here did exactly what it is for: on the rebase,
`test_no_keyword_argument_leaks_an_undocumented_exception[provenance.build_record.issued_at]`
went `XPASS(strict)` and the file failed rather than going quietly green.

The second case filed under the same entry did not flip.
`test_what_a_producer_accepts_its_own_verifier_accepts` asks whether a producer
emits anything its own verifier refuses, and for `issued_at` it still reported one
value, `10000000000000000000`. That is the half agentrust-io#334 did not carry: `_check_structure`
had no upper bound, so `build_record` accepted a timestamp `sign_record` cannot
canonicalise and the caller met `rfc8785`'s `IntegerDomainError` rather than the
`ProvenanceError` this module documents. Proposed on agentrust-io#320 on 09-10 and carried on
agentrust-io#323, which is now superseded on everything else.

`_check_structure` gains the bound, reading `JCS_SAFE_INTEGER` from `sign.py`
rather than writing the number a third time, and `LEAKS_FILED` is empty. The
comment above it records what the entry held and why it went, so the marker's two
firings are not lost with it.

`_check_structure` is shared, so this reaches `verify_record` too. Under the
default freshness policy a record carrying such a timestamp was already refused as
dated in the future, so no record that verified before is refused now. What changes
is the caller who widens `max_future_skew_seconds` past the gap: that returned
`rfc8785.IntegerDomainError` out of a function documented to raise
`ProvenanceError`, and now returns `ProvenanceError`. A test pins it.

The boundary is asserted against the canonicalizer rather than against a literal:
each of the four cases checks `build_record` and `rfc8785.dumps` agree about
whether the value exists, so the guard cannot drift away from the thing it is
guarding.

Verified in both directions. With the clause: 1547 passed, 1 skipped, and the
sweep at 152 passed with nothing xfailed. With only the clause reverted: 4 fail,
being the two out-of-range boundary cases, the verifier case, and the sweep's own
producer-and-verifier case, which is the instrument that reported the gap now
confirming it is closed. `ruff`, `mypy` and `tools/check_dashes.py` pass. No
`CHANGELOG.md` hunk, per the instruction on 09-11.

Signed-off-by: LouieLuNZ <48041247+lywinged@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

provenance.build_record coerces malformed issued_at values before validation

1 participant