Repository navigation
fix(provenance,validate): remove str() coercion on digest/hash fields in provenance and enforce strict JSON in schema loaders (#627) - #632
Conversation
|
🟡 Contributor Check: MEDIUM
Automated check by AgenTrust Contributor Check. |
rajnisht7
left a comment
There was a problem hiding this comment.
the overall changes looks good, there is one point needs to be addressed:
check_tool_catalog() still missing the strict tool_catalog.hash validation. issue #627 explicitly called this out, the proposed fix said the isinstance(..., str) + digest check should apply in both verify_record and check_tool_catalog, whereas this PR only did the first one
Current path still does:
expected = catalog.get("hash")
if actual != expected:
raise ToolCatalogMismatch(...)so a malformed/non-string hash ends up as ToolCatalogMismatch instead of ProvenanceError. which breaks the intended distinction (document malformed vs live catalog mismatch), and check_tool_catalog is documented as independently callable before verify_record, kindly add the same guard before the comparison (and ideally before computing actual), plus a regression test that hits check_tool_catalog with a non-string / malformed hash and asserts ProvenanceError.
lywinged
left a comment
There was a problem hiding this comment.
Request changes. Reviewed at 75e2f87 on da69b31.
One thing is missing that CONTRIBUTING's "Before a pull request is reviewed" asks for: a test for each rejection the change adds, failing when that rejection is switched off. The schema must be a JSON object check in _loads_schema has none: deleting it leaves all 4788 tests passing. I will review the rest once it is in.
CI at 75e2f87 failed in the portability step for a reason this pull request did not cause: main failed the same step at da69b31, before #624, and passes at dcca4e0.
Generated by Claude Code
|
Thank you @rajnisht7 and @lywinged! Addressed both points in
|
… and harden _loads_schema (agentrust-io#627) Require isinstance(..., str) and _DIGEST_RE.match(...) on artifact.digest, endpoint.spki_sha256, and tool_catalog.hash in verify_record and check_tool_catalog in src/agentrust_trace/provenance.py, and reject duplicate JSON keys, non-finite constants, and non-object roots in _loads_schema in src/agentrust_trace/validate.py. Closes agentrust-io#627 Signed-off-by: Raghuvar Karthik Durga <177662415+drk-ram@users.noreply.github.com>
11f2320 to
64df3cd
Compare
lywinged
left a comment
There was a problem hiding this comment.
Approve. Reviewed at 64df3cd on e9ab302.
Both points from the first round are in. The schema must be a JSON object check now has the "[1, 2, 3]" case, and deleting the check fails the test. check_tool_catalog answers a tool_catalog.hash of "not-a-digest", 5, uppercase hex or null with ProvenanceError, and an absent hash still reads as ToolCatalogMismatch; the new test checks the class and the message for its own three inputs. With that, Closes #627 closes the issue with its check_tool_catalog point met.
What I checked rather than read:
- CI's commands, run here on 3.11 and 3.12: ruff, the dash check and mypy clean; the suite at 5222 passed and 98 skipped on 3.11 and 5221 and 99 on 3.12; the replay experiment, the two
LC_ALL=C PYTHONUTF8=0runs, thePYTHONOPTIMIZE=2command and the two--helpcalls pass. The head is one commit one9ab302, which ismainas I write, so that is also the merged result. - The description's two validation lines reproduce: 263 passed for the three test files it lists, and
tools/review_bar.pyreports 2 fail on behaviour, 0 on a missing name or file and 17 pass or skip. - Removed alone, each of the seven rejections the change adds fails a test: the object check fails 375, the duplicate-key hook 131 and the
parse_constanthook 304, the schema test plus the tests its leaked cache takes with it, as below; thecheck_tool_catalogguard, and each of the threeisinstanceclauses reverted tostr(), fail 1. The guard narrowed toisinstancealone, without the digest match, also fails 1. - On
main,verify_recordaccepts three signed records whoseartifact.digest,endpoint.spki_sha256andtool_catalog.hashare each anintsubclass whose__str__is a digest. This head refuses all three.verify_recordrefuses the same JSON values on both commits: null, true, false, 5, 5.5, an array, an object, an uppercase digest and an empty string in each of the three fields raiseProvenanceError, and none of the 691 JSON files in the tree carriesspki_sha256,tool_catalogormcp-server-provenance. - Python callers see more than the
intsubclass.build_recordrefuses acollections.UserStringdigest thatmainaccepted at build time and then could not sign. A(str, Enum)digest thatmainrefused now verifies.maincompared a(str, Enum)kindby itsstr(), which is the class and member name rather than the value, so atee-attestedrecord withattestation: nullverified and one with attestation present was refused; here the member reads as its value and both go the other way. - The four packaged schemas parse to equal values under
json.loadsand_loads_schema, which refuses a repeated key at the top level, three objects deep and inside an array,NaN,Infinity,-Infinity, and a top-level array, string or null.
Things I would not hold the merge for:
- When a case of the schema test stops raising, it takes other tests with it: 374 in 16 files for the object check, 130 in 9 for the duplicate-key hook and 303 in 16 for the
parse_constanthook, because the closingcache_clear()calls run only when every case raises, so_schema()keeps the temporary schema. Atry/finallyaround the loop would keep a future failure to one test. kind=record["kind"]reverted tostr(record.get("kind"))leaves every test passing.- The
verify_recordcases fortool_catalog.hashare_StrDigest,123and"not-a-digest", whichmainrefuses too, the first on canonicalization and the other two on the digest pattern, so the test pins the clause through its message. The inputmainaccepts, theintsubclass above, is not among them. - The guard sits after
tool_catalog_hashhas been computed, not before it as today's comment says; nothing turns on the order. - The description's changelog bullet does not say that
check_tool_catalognow raisesProvenanceErrorfor a malformed or null hash wheremainraisedToolCatalogMismatch; the merging Maintainer writes the CHANGELOG entry from that bullet.
Generated by Claude Code
What this changes
Closes #627.
src/agentrust_trace/provenance.py(_check_structure,verify_record):str(...)coercion before_DIGEST_RE.match(...)onartifact.digest,endpoint.spki_sha256, andtool_catalog.hashwithisinstance(value, str) and _DIGEST_RE.match(value).src/agentrust_trace/validate.py(_schema,profiles_with_schema):_loads_schemawithobject_pairs_hook(rejecting duplicate keys) andparse_constant(rejectingNaN/Infinity).Type of change
Spec section
spec/server-provenance-v1.mdsection 3 andspec/trace-v0.2.mdsection 3.Tests for a fix
tests.test_provenance_locator_types::test_provenance_rejects_str_coercible_non_string_digests_and_hashestests.test_provenance_locator_types::test_validate_loads_schema_rejects_duplicate_keys_and_nonfinite_constantspython -m pytest tests/test_provenance_locator_types.py tests/test_provenance.py tests/test_validate.py -q->263 passedpython tools/review_bar.py --base origin/main --head HEAD->2 fail on behaviour, 0 fail on a missing name or fileChangelog entry
agentrust_trace.provenance,agentrust_trace.validate): requireisinstance(..., str)before matchingartifact.digest,endpoint.spki_sha256, andtool_catalog.hashinprovenance, and reject duplicate JSON keys and non-finite constants when loading packaged schemas invalidate(fix(provenance,validate): remove str() coercion on digest/hash fields in provenance and enforce strict JSON in schema loaders #627, @drk-ram).Checklist
git commit -s)CHANGELOG.mdupdated (for any normative change)<!-- CHANGED: #NNN: description -->in spec text