fix(provenance): tool_catalog_hash crashes on a malformed tools list - #229
Conversation
Signed-off-by: rajnisht7 <rajnishtiwari9787@gmail.com>
Qiang-Xu
left a comment
Qiang-Xu
left a comment
There was a problem hiding this comment.
Looks good to me, thanks!
Conflict was CHANGELOG.md only: agentrust-io#225, agentrust-io#227 and this branch each added a Fixed entry next to the TraceAGTAdapter one. Kept all four. Took main's copy of the shared TraceAGTAdapter line, since the dash sweep in agentrust-io#230 rewrote it there and CI now bans em dashes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014NL8o3PXq6kfs2SdmBv6ak
imran-siddique
left a comment
imran-siddique
left a comment
There was a problem hiding this comment.
Third in the set and the one closest to the attack, so worth taking.
check_tool_catalog(record, tools) exists because a verifier that never obtains what the server said to you has checked a document against itself. tools is that input: the untrusted party's own claim about itself, passed straight through to tool_catalog_hash(). It iterated and called .get() on each entry with no shape check, so a non-list crashed on iteration and a bad entry crashed on .get(). Both raised AttributeError or TypeError where the module promises ProvenanceError.
Two details done right:
The list itself and its entries are checked separately, and the entry error names the index. Debugging "one of your 40 tools is malformed" without knowing which is unpleasant.
The tests parametrize both shapes rather than picking one representative, including a nested list as an entry, which is the case a naive isinstance(t, (dict, list)) would have let through.
CI green. I merged main in to clear a CHANGELOG conflict from #225 and #227 landing first, and took main's copy of the shared line so the new no-dashes check passes. Nothing of yours changed.
Three solid fail-closed fixes in a row on verification paths. Appreciated.
Completes what #6 began and deferred while upstream PRs agentrust-io#225 and agentrust-io#227 were editing those functions. Those landed. agentrust-io#229 closed provenance.tool_catalog_hash; the other three were untouched, because _as_object guards fields inside a record and no guard on a field can reach the record's own type. provenance.verify_record and provenance.check_tool_catalog leaked 11 AttributeErrors of 12 non-object inputs apiece, which is not the ProvenanceError verify_record documents. content_marking.verify_assertion never checked that record_bytes were bytes, and bytes(5) is five zero bytes, so an int was hashed, failed to match, and the caller was told the record at the URL had changed: a specific and false accusation about somebody else's server. Every public entry point in the package now reports zero leaks under the sweep. Thirty-two tests, each shown load-bearing by removing the guard it covers (12, 11 and 10 failures). Also repairs the changelog sentence the agentrust-io#230 em dash sweep broke into "an object that is an array", which check_dashes.py cannot catch because the removal was clean and the sentence is what broke. 1031 passed, 1 skipped. Ruff clean.
3 participants
What this changes
check_tool_catalog(record, tools) is the function which this module's own docstring calls "the step that catches a live attack," because tools there is what the MCP server actually returned when asked and the untrusted party this check exists to catch. It passes tools straight through to tool_catalog_hash(), which iterated it and called .get(...) on each entry with no check that tools was a list or that its entries were objects.
but it crashes when tools itself being a non-list (a string, None, an int, a dict) raised AttributeError or TypeError depending on what iterating it did; a well-formed list with one malformed entry (a string, None, an int, a nested list) raised AttributeError on that entry's .get() call. Either way the caller got a crash instead of the documented ProvenanceError, on exactly the input a misbehaving maliciously or just buggily server controls.
Type of change
Spec section
None
Checklist
git commit -s)CHANGELOG.mdupdated (for any normative change)<!-- CHANGED: #NNN — description -->in spec text