fix: close #518's gateway-side gap in _handle_mcp - #561
Conversation
qubeena07's review on agentrust-io#556 flagged that _handle_mcp in mcp/server.py has the same gap agentrust-io#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 agentrust-io#556's coverage) plus all 64 existing tests across the affected test files pass.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…gged on agentrust-io#561 Codecov's patch coverage check on PR agentrust-io#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.
I read @qubeena07's guidance on #518 directly rather than taking your summary of it, and this matches it closely, including the parts it declines.
Their words: "I am not asking you to unify the two in this PR, but whatever caps you settle on here should eventually apply there too, otherwise the real gateway is left with the same depth and key count gap the mock just closed." That is this PR. Doing the follow-through in a separate change, on the file that actually matters, is the right shape.
Verified rather than taken:
json.loads("NaN")returnsnanon a default interpreter, so theparse_constantgap is real. Routing the rejection through the existingValueErrorbranch so it surfaces as-32700/MCP_PARSE_FAILUREis better than adding a new error path for it, because a caller sees one parse-failure contract rather than two.- Constant parity is exact.
_MAX_ARG_DEPTH = 20and_MAX_ARG_KEYS = 256on both sides. - The
_deny_responseextraction you describe as pure code motion is. Theupstream_error:502 branch, the{attestation_stale, catalog_drift}503 branch and the-32000codes are identical betweenorigin/mainand this head, relocated and not rewritten. That was the part I was going to look hardest at, since a refactor landing in the same commit as new checks is where behaviour usually goes missing quietly.
Two judgement calls I want to endorse explicitly, because both could have gone the lazy way.
Excluding bool from the valid id types. It is an int subclass in Python, so the obvious isinstance(id, (str, int, type(None))) accepts true as a request id and nothing downstream would notice. Naming it in the description rather than leaving it as a silent detail is what let me check it quickly.
Leaving the arguments must-be-a-dict gap alone and saying so. _arg_shape_violation tolerates a non-dict gracefully, so you are right that not touching it regresses nothing, and flagging it beats folding an unflagged change into a PR scoped to a reviewer's list.
One note for the record rather than for you: the caps are kept in sync by comment reference rather than by shared code, so that reference depends on #556 landing. Taking that next.
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
Closes the gateway-side gap qubeena07 flagged in review on #556 (round two of #518).
What this closes
_handle_mcpinsrc/cmcp_runtime/mcp/server.py- the real inbound gateway path, not the demo mock - had the same gap #556 closed inscripts/mock_upstream.py: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_KEYSare the same values asscripts/mock_upstream.py, with the same rationale in the comment: DOS-001's byte cap bounds total size, not shape, and 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 keys.json.loadstakes these silently through itsparse_constanthook by default; a customparse_constantnow raises, surfacing as the existing -32700 parse error path (MCP_PARSE_FAILURE).What this does not touch
argumentsmust-be-a-dict type check. The mock has this; the gateway doesn't, and it's a separate, unflagged gap -_arg_shape_violationtolerates non-dictargumentsgracefully (walks it, no violation raised), so this PR doesn't regress anything by leaving it alone. Flagging in case it's worth its own pass.CMCPProxy.call_tool.Refactor note
_handle_mcpand_handle_tool_callwere both at or over this repo's cyclomatic complexity threshold before this change and would have gone further over it with the new checks inline. Extracted_parse_mcp_envelope(body size/parse, now returns the parsed dict or an errorResponse) and_deny_response(policy-deny error mapping) out of them - both are pure code motion, no behavior change, covered by the existing test suite passing unchanged.Tests
14 new tests (6 jsonrpc/id, 4 depth/key-count, 3 NaN/Infinity - matching #556's coverage shape) plus all 64 existing tests across
test_mcp_server_auth.pyand the other test files that instantiateMCPServer(test_workflow_scope.py,test_break_glass.py,test_low_batch_186_187_191_194.py,test_session_reset.py,test_initialize_protocol_version.py) pass. Ruff clean on both touched files.