fix(provenance): refuse conflicting tool schema aliases - #300
Conversation
Closes agentrust-io#297. Signed-off-by: Altru.dev <altrudevelop@gmail.com>
|
🔴 Contributor Check: HIGH
Automated check by AgenTrust Contributor Check. |
lywinged
left a comment
There was a problem hiding this comment.
Request changes, for one line. The direction is right and the reported vector is closed; the comparison that closes it sits one layer above the bytes the hash covers, and that layer lets a pair through.
For anyone reading this later: tool_catalog is a field of the MCP Server Provenance Record in spec/server-provenance-v1.md, not of the Trust Record, and validate_json refuses a Trust Record that carries it. Section 4 of that specification names the preimage as name, description and input_schema, and says nothing about inputSchema; accepting the MCP wire spelling is this implementation's convenience, and the ambiguity this PR closes is the one that convenience opened. It is reachable in practice: cmcp's proxy (mcp/proxy.py) sends tools/list to the upstream server and hands the returned objects to check_tool_catalog unchanged, so both spellings are the server's to set, while cmcp's catalog loader (catalog/loader.py) resolves inputSchema first and this hash resolves input_schema first.
What I ran on f5f184b. The three new tests copied into a worktree of main: two fail, one passes, which is the right shape. Full suite on this head 1188 passed and 1 skipped against 1185 on main, with ruff, tools/check_dashes.py and mypy clean. Two spellings alone hash identically, so a single-spelling producer sees no change. cmcp itself emits only inputSchema, never both, and no file in the organisation's repositories carries both spellings on one object, so nothing existing trips the new refusal.
The gap. The check is t["input_schema"] != t["inputSchema"], Python equality, and True == 1 and False == 0 in Python while anchor_bytes writes true and 1. On this head, a signed side of {"additionalProperties": 1} with a live side of {"additionalProperties": true} passes the new check and check_tool_catalog accepts it; 0 against false and a nested 1 against true behave the same. Whether that widens anything depends on the validator: Python's jsonschema treats 1 as true and 0 as false, so nothing changes there; a validator that discards an invalid keyword value rather than failing falls back to the default, and for additionalProperties the default is permissive. Narrow, and the function exists for exactly the cross-implementation case.
The fix is one token: compare anchor_bytes(t["input_schema"]) != anchor_bytes(t["inputSchema"]). On a copy of this branch that change leaves the three new tests passing, refuses the 1 and true pair and the 0 and false pair, still accepts a pair that differs only in key order, and the full suite stays at 1188. A fourth test for the 1 and true pair would pin it.
Not this PR, for the maintainers. Section 4 names one spelling and the two repositories resolve the pair in opposite orders; a stated precedence in the specification, or in cmcp, is what closes that for good, which is the second remedy #297 names. And a simpler design is available: refuse any tool object that carries both keys, equal or not, since neither this specification nor MCP's own wire format gives a server a reason to send both.
Tool-assisted: the runs and this write-up.
Signed-off-by: Imran Siddique <imran.siddique@opaque.co>
imran-siddique
left a comment
There was a problem hiding this comment.
Reviewed 427952e. The requested comparison now uses anchor_bytes, the same representation the catalog hashes. Three independent cases demonstrate that Python-equal boolean/integer values, including nested values, were accepted before the repair and are refused afterward. Equivalent object key order still produces the same hash. All 112 focused provenance tests pass; changed-file Ruff and hosted CI pass. No held workflows were found.
This addresses the comparison finding on the earlier head. I pushed the repair, so independent re-review is still required before merge.
lywinged
left a comment
There was a problem hiding this comment.
Approving at 427952e. The comparison now runs on the bytes the hash covers, and the new file pins it.
Measured on this head. The three pairs that passed at f5f184b are refused: 1 against true, 0 against false, and a nested 1 against true. A pair that differs only in object key order is still accepted, and a genuinely different pair is still refused, so the guard did not get coarser in the process. Full suite 1192 passed and 1 skipped, with ruff, tools/check_dashes.py and mypy clean.
Both test files are load-bearing, checked by putting the defect back. Restoring != in place of the byte comparison turns all three parametrized cases in test_alias_canonical_comparison.py red and leaves the earlier file green, which is the split you would want: the new file exists for exactly what the old one could not see. Removing the guard entirely turns five red across the two files.
One behaviour did change, and it is a diagnostic rather than a decision. A tool object that carries both aliases, differing values, and a number the anchor profile refuses used to fail with the conflicting-aliases error and now fails with UnanchorableValue naming the number, because the comparison reaches the serializer first. The other three combinations of those inputs that I tried raise what they raised before, and both outcomes are refusals, so nothing is accepted that was refused before.
Against current main, 52687e6, which now carries #239's change to the same module: merges with no conflict, and the merged tree runs 1217 passed and 1 skipped with the three checks clean.
The two notes from the earlier review are unchanged by this and still for the maintainers, not for this PR: a stated precedence for the two spellings, and the simpler design of refusing any object that carries both keys at all.
Tool-assisted: the runs and this write-up.
Closes #297.
What
tool_catalog_hash()accepts both the SDK spellinginput_schemaand the MCP wire spellinginputSchema. The compatibility fallback previously preferredinput_schemasilently when both were present, even when the two values disagreed.That left the live-catalog check vulnerable to an ambiguous tool object: a server could carry a snake-case schema matching the signed provenance record while also exposing a different camel-case schema.
This change preserves the existing compatibility rule for either spelling and for equal dual-key values, but refuses a tool object when both keys are present with different values.
Regression coverage
Focused tests hold three cases:
input_schema/inputSchemavalues hash identically to the existing single-key form;ProvenanceErrorbefore hashing;check_tool_catalog()refuses the decoy-schema construction from tool_catalog_hash silently prefers input_schema over inputSchema when both are present, letting a decoy schema pass check_tool_catalog #297 instead of reporting a clean match.Scope
No schema change, wire-format change, canonicalization change, signature change, or tool-count semantic change.
Credit: the issue and proof of concept were reported by @harshnair75567-cloud.
AI-assistance disclosure: ChatGPT assisted with source triage, duplicate/ownership review, implementation drafting, regression design, and exact-diff review.
altrudevreviewed the bounded claim and remains responsible for the contribution.