fix: close the remaining #518 gaps in the mock upstream's JSON-RPC gate - #556
Conversation
… JSON-RPC gate Round two of agentrust-io#518, scoped by qubeena07's review on the issue: strict jsonrpc/id validation, argument depth and key-count caps, and rejection of NaN/Infinity/-Infinity. jsonrpc must equal "2.0" exactly, and id must be a string, number, or null when present. The byte cap from agentrust-io#544 bounds total size, not shape, so a payload well under 1MB can still push toward Python's recursion limit through deep nesting or cost real time to iterate through a flat object with thousands of keys. _MAX_ARG_DEPTH and _MAX_ARG_KEYS follow the _AIA_MAX_DEPTH pattern in tee/tpm.py: named constants with a comment stating this is a judgment call, not a value derived from an observed guarantee. NaN and Infinity are rejected via a custom parse_constant, since json.loads accepts them silently by default. _handle_mcp in mcp/server.py has the same gap and is left untouched; qubeena07 asked that the two validation paths not be unified in this PR.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
* fix: close #518's gateway-side gap in _handle_mcp qubeena07's review on #556 flagged that _handle_mcp in mcp/server.py has the same gap #556 closed in the mock upstream: no strict jsonrpc/id validation, no argument depth/key-count cap, and NaN/Infinity/-Infinity pass through json.loads silently. This applies the same three checks here, matching scripts/mock_upstream.py's behavior and _MAX_ARG_DEPTH / _MAX_ARG_KEYS values, kept in sync rather than unified per qubeena07's explicit request not to merge the two validation paths in one PR. Also extracts _parse_mcp_envelope (body size/parse) and _deny_response (policy-deny error mapping) out of _handle_mcp and _handle_tool_call, which were pre-existing at or over the repo's complexity threshold and would have gone further over it with the new checks inline. Both extractions are pure code motion, no behavior change. 14 new tests (6 jsonrpc/id, 4 depth/key-count, 3 NaN/Infinity, matching #556's coverage) plus all 64 existing tests across the affected test files pass. * test: cover the deny-response and list-recursion branches Codecov flagged on #561 Codecov's patch coverage check on PR #561 flagged 7 lines as untested in src/cmcp_runtime/mcp/server.py. All 7 are pre-existing behavior that got attributed to this PR's diff because the code moved during the _deny_response extraction, plus one branch in the new _arg_shape_violation that no existing test exercised. Adds tests for the upstream_error 502 branch, the attestation_stale and catalog_drift 503 branches, the advice-included branch in the deny response, and a depth-cap violation nested inside a list rather than a dict, since _arg_shape_violation recurses into lists too. No production code changed. All 7 previously-missing lines now show as covered; 49 tests pass, ruff clean.
imran-siddique
left a comment
imran-siddique
left a comment
There was a problem hiding this comment.
Merging this right after #561 so the pair lands together, which is what the comment-reference sync between the two constant sets depends on.
Most of what I would check here I already checked on #561: _MAX_ARG_DEPTH = 20 and _MAX_ARG_KEYS = 256 match exactly across both files, json.loads("NaN") returns nan on a default interpreter so the parse_constant gap is real, and @qubeena07's guidance on #518 asked for precisely this scoping, down to following the _AIA_MAX_DEPTH pattern in tee/tpm.py for how the constants are named and justified. You followed that pattern rather than approximating it.
The part I want to credit specifically is the comment on the caps saying this is a judgment call rather than a value derived from an observed guarantee. _AIA_MAX_DEPTH was sized from real observed data and these are not, and writing that down instead of borrowing the earlier constant's authority is the honest version. Someone tuning these later needs to know which kind of number they are looking at.
Flagging the uncapped list length rather than folding it in was also right. Scope was objects, a large flat list is still bounded by the 1MB cap, and widening quietly is how a reviewer's list stops being a list.
One note on what this is and is not. This is the demo mock, so its direct security value is lower than #561's. What it buys is that the mock now rejects what the real gateway rejects, which is the difference between a conformance reference and a lenient stand-in that lets a broken client look fine locally.
Approving and merging.
* fix: enforce a per-string length cap on tool arguments (#562) docs/spec/proxy-security.md's Fuzzing Definition of Done specs MAX_STRING_LENGTH at 1MB per string field. It was not implemented anywhere in src/ or scripts/. A single oversized string sits inside an otherwise shallow, low-key-count payload and passes the depth and key-count caps from #556/#561 unbounded, up to the whole-body byte cap. Extends the existing _arg_shape_violation walk in both files with a UTF-8 byte length check, covering string values and object keys. Keys are checked because the key-count cap bounds how many there are, not how large each one is, so one huge key would otherwise pass everything. The cap is not the spec's literal 1MB. That equals MAX_REQUEST_BYTES in both files today, so a 1MB string plus any surrounding JSON already exceeds the whole-body cap and the check could never fire before DOS-001's size rejection already had. It would have been dead code. Set to half the whole-body cap instead, derived from a named constant rather than a restated literal so raising the default carries this along with it. Verified reachable: an over-cap string is 500,101 bytes on the wire against a 1,000,000 byte body cap. The spec's 10MB MAX_REQUEST_BYTES versus the 1MB implemented in both files is left alone. Picking a number there is a maintainer call, not one to make inside this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KDXJ4ghkW6v56W8St2w5kg * test: cover the clean-list fall-through Codecov flagged The list branch of _arg_shape_violation returns None explicitly once the loop finds nothing, so that every branch terminates on its own rather than depending on an earlier branch's return for the string check below it to be reachable. That return had no test. The only list coverage was the rejection path, which would still pass if the walk wrongly rejected every list it saw. Adds the accepting case to both files so the two stay in step. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KDXJ4ghkW6v56W8St2w5kg --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
3 participants
Round two of #518, scoped by qubeena07's review comment on the issue.
What this closes
jsonrpc/idvalidation.jsonrpcmust equal"2.0"exactly.idmust be a string, number, or null when present (bool explicitly excluded, since it's anintsubclass in Python)._MAX_ARG_DEPTHand_MAX_ARG_KEYSfollow the_AIA_MAX_DEPTHpattern intee/tpm.py: named constants nearMAX_REQUEST_BYTES, with a comment stating this is a judgment call rather than a value derived from an observed guarantee. The 1MB byte cap from fix(mock-upstream): hardened JSON-RPC gate for the mock MCP upstream #544 bounds total size, not shape — a payload well under that limit can still push toward Python's recursion limit through deep nesting, or cost real time to iterate through a flat object with thousands of short keys.json.loadstakes these silently through itsparse_constanthook by default; a customparse_constantnow raises, surfacing as the existing -32700 parse error path.What this does not touch
_handle_mcpinmcp/server.py: has the same depth/key-count gap. qubeena07 flagged it but explicitly asked not to unify the two validation paths in this PR, so it's untouched here.Tests
19 existing tests + 13 new (6 jsonrpc/id, 4 depth/key-count, 3 NaN/Infinity). All 32 pass. No changes outside
scripts/mock_upstream.pyandtests/unit/test_mock_upstream_gate.py.