fix(mcp): reject JSON-RPC batches#2083
Conversation
Reject top-level JSON arrays on MCP Streamable HTTP endpoints before policy evaluation or upstream forwarding. Preserve single-message MCP handling and generic JSON-RPC batch behavior. Return a stable denial detail across direct, route-selected, and forward-proxy paths. Signed-off-by: Shiju <shiju@nvidia.com>
shiju-nv
requested review from
a team,
derekwaynecarr,
maxamillion and
mrunalp
as code owners
krishicks
left a comment
krishicks
left a comment
There was a problem hiding this comment.
Could we avoid passing unsupported_mcp_batch through the generic human-readable reason string and then recovering its semantics through string equality? A structured hard-denial/error classification would let the response boundary choose the stable API code without jsonrpc_parse_rejection_reason and forward_l7_denial_detail both special-casing this value.
Also, the parser test already exercises every top-level array shape. Could we keep one representative batch for the direct/route-selected × audit/enforce relay matrix and remove mcp_rejects_jsonrpc_batches plus mcp_batch_rejection_preserves_single_mcp_and_generic_batch_controls? The latter’s assertions all describe behavior that predates this change.
| } | ||
|
|
||
| #[test] | ||
| fn mcp_batch_rejection_preserves_single_mcp_and_generic_batch_controls() { |
There was a problem hiding this comment.
I don't think this test would have failed before this change, and it seems to duplicate parses_valid_batch_without_error. I would remove it.
|
|
||
| /// Preserve stable classifier reasons while contextualizing ordinary JSON-RPC parse failures. | ||
| pub(crate) fn jsonrpc_parse_rejection_reason(error: &str) -> String { | ||
| if error == UNSUPPORTED_MCP_BATCH_REASON { |
There was a problem hiding this comment.
This seems to special-case UNSUPPORTED_MCP_BATCH_REASON, but I don't think this is a good pattern. I would prefer it if the jsonrpc_info was the only source of this info.
| ) -> String { | ||
| // Stable classifier reasons are machine-readable API details. Policy and | ||
| // ordinary parse denials retain request context for human diagnostics. | ||
| if force_deny && reason == crate::l7::jsonrpc::UNSUPPORTED_MCP_BATCH_REASON { |
There was a problem hiding this comment.
This is another special-casing of UNSUPPORTED_MCP_BATCH_REASON which I think could be resolved through better design.
|
Important I feel like things like this would be best aligned with an
I don't expect to see it's return, but we should do our best to not exclude folks from being able to use older MCP servers at the gateway/proxy level. I would suggest, that until a Note If we intend to expose a key of |
|
I think we could also choose just not to reject JSON-RPC batches for MCP in any event, given some versions of the protocol require and some remove batching. We're not in the business of validating the MCP payload against any specific protocol version. We only need to know about the batching at all so we can validate the entire batch against the allowed calls. We know there will be structural differences in the payloads across different versions that require special handling, like the upcoming stateless changes, and we'll need to support that. |
An MCP policy engine needs an explicit set of supported wire profiles and fail-closed behavior for structures it cannot safely interpret. We can offer that as individual options within the I want to be clear: for a policy engine, treating every version’s wire forms as universally acceptable imposes cumulative compatibility and validation costs on every MCP policy. The "batch on"/"batch off" argument can be most cleanly seen in the reaction to the PR that removed batching from the spec, after it was merged. It's drama that we can avoid by providing this as a single Specifically,
I think that is a strong argument in favor of per-version behavior, especially given the changes in the current draft. The policy-relevant features of messages across protocol versions need to be enunciated for a policy engine to enforce them. The draft says that selected body fields are mirrored into HTTP headers so intermediaries can "route and inspect requests without parsing the body." However, it also says that any server processing the body MUST validate that the header values match their corresponding body values. It further requires For OpenShell to use those headers safely at the Agent Security Boundary, it would have two representations to reconcile and, for parameter headers, tool-schema state to maintain or consult. I do not want endpoints that are not using that MCP version profile to incur that additional validation complexity. |
3 participants
Summary
This PR rejects every top-level JSON array in MCP mode before member parsing, method policy evaluation, or upstream delivery. It leaves existing generic JSON-RPC batch behavior unchanged.
Related Issue
Closes #2082
Changes
unsupported_mcp_batchclassification as soon as the MCP parser sees a top-level array, preserve that exact detail in dedicated, route-selected, and explicit forward-proxy denial paths, and leave ordinary JSON-RPC parse diagnostics unchanged.Testing
mise run pre-commitpassesChecklist