From 5849d2b2e85e40e699a2621218045de1dd124737 Mon Sep 17 00:00:00 2001 From: Shiju Date: Wed, 1 Jul 2026 11:30:00 +0530 Subject: [PATCH] fix(mcp): reject JSON-RPC batches 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 --- .../src/l7/jsonrpc.rs | 80 ++++++++ .../src/l7/relay.rs | 177 +++++++++++++++++- .../openshell-supervisor-network/src/proxy.rs | 61 +++++- docs/reference/policy-schema.mdx | 4 +- docs/sandboxes/policies.mdx | 4 +- 5 files changed, 315 insertions(+), 11 deletions(-) diff --git a/crates/openshell-supervisor-network/src/l7/jsonrpc.rs b/crates/openshell-supervisor-network/src/l7/jsonrpc.rs index 7122edeee0..1657be9ea0 100644 --- a/crates/openshell-supervisor-network/src/l7/jsonrpc.rs +++ b/crates/openshell-supervisor-network/src/l7/jsonrpc.rs @@ -15,6 +15,17 @@ use crate::l7::provider::{L7Provider, L7Request}; pub const DEFAULT_MAX_BODY_BYTES: usize = 64 * 1024; +/// Stable classification reason for a top-level array on an MCP endpoint. +pub(crate) const UNSUPPORTED_MCP_BATCH_REASON: &str = "unsupported_mcp_batch"; + +/// 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 { + return error.to_string(); + } + format!("JSON-RPC request rejected: {error}") +} + /// Selects whether the parser should treat a JSON-RPC message as generic /// JSON-RPC 2.0 or as an MCP message with MCP method/params validation. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -177,6 +188,17 @@ pub fn parse_jsonrpc_body_with_options( }; if let serde_json::Value::Array(items) = value { + // Streamable HTTP carries one MCP message per POST. Reject the array + // itself before parsing members so every batch shape fails uniformly. + if inspection_options.mode == JsonRpcInspectionMode::Mcp { + return JsonRpcRequestInfo { + calls: Vec::new(), + is_batch: true, + receive_stream: false, + has_response: false, + error: Some(UNSUPPORTED_MCP_BATCH_REASON.to_string()), + }; + } if items.is_empty() { return JsonRpcRequestInfo { calls: Vec::new(), @@ -491,6 +513,64 @@ mod tests { assert_eq!(call.params.len(), 1); } + #[test] + fn mcp_batch_rejects_every_top_level_array_shape() { + let batches: &[&[u8]] = &[ + br"[]", + br#"[{"jsonrpc":"2.0","id":1,"method":"ping"}]"#, + br#"[{"jsonrpc":"2.0","method":"notifications/initialized"}]"#, + br#"[{"jsonrpc":"2.0","id":1,"result":{}}]"#, + br#"[{"jsonrpc":"2.0","id":1,"method":"ping"},{"jsonrpc":"2.0","id":1,"result":{}}]"#, + ]; + + for body in batches { + let info = parse_jsonrpc_body(body, JsonRpcInspectionMode::Mcp); + assert!(info.calls.is_empty(), "batch calls escaped: {info:?}"); + assert!( + info.is_batch, + "array was not classified as a batch: {info:?}" + ); + assert!(!info.has_response, "batch members were inspected: {info:?}"); + assert_eq!(info.error.as_deref(), Some(UNSUPPORTED_MCP_BATCH_REASON)); + } + } + + #[test] + fn mcp_batch_rejection_preserves_single_mcp_and_generic_batch_controls() { + let singles: &[(&[u8], Option<&str>, bool)] = &[ + ( + br#"{"jsonrpc":"2.0","id":1,"method":"ping"}"#, + Some("ping"), + false, + ), + ( + br#"{"jsonrpc":"2.0","method":"notifications/initialized"}"#, + Some("notifications/initialized"), + false, + ), + (br#"{"jsonrpc":"2.0","id":1,"result":{}}"#, None, true), + ]; + + for (body, expected_method, expected_response) in singles { + let mcp = parse_jsonrpc_body(body, JsonRpcInspectionMode::Mcp); + assert!(mcp.error.is_none(), "single MCP message failed: {mcp:?}"); + assert!(!mcp.is_batch); + assert_eq!( + mcp.calls.first().map(|call| call.method.as_str()), + *expected_method + ); + assert_eq!(mcp.has_response, *expected_response); + } + + let generic = parse_jsonrpc_body( + br#"[{"jsonrpc":"2.0","id":1,"method":"reports.list"},{"jsonrpc":"2.0","method":"reports.observed"}]"#, + JsonRpcInspectionMode::JsonRpc, + ); + assert!(generic.error.is_none(), "generic batch failed: {generic:?}"); + assert!(generic.is_batch); + assert_eq!(generic.calls.len(), 2); + } + #[test] fn mcp_mode_rejects_non_recommended_tool_names_by_default() { let body = br#"{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"read status","arguments":{}}}"#; diff --git a/crates/openshell-supervisor-network/src/l7/relay.rs b/crates/openshell-supervisor-network/src/l7/relay.rs index ed2bde1135..2fdad1fe86 100644 --- a/crates/openshell-supervisor-network/src/l7/relay.rs +++ b/crates/openshell-supervisor-network/src/l7/relay.rs @@ -409,7 +409,7 @@ where jsonrpc_info .as_ref() .and_then(|info| info.error.as_deref()) - .map(|error| format!("JSON-RPC request rejected: {error}")) + .map(crate::l7::jsonrpc::jsonrpc_parse_rejection_reason) }); let force_deny = parse_error_reason.is_some(); let (allowed, reason) = if let Some(reason) = parse_error_reason { @@ -1082,7 +1082,7 @@ where let parse_error_reason = jsonrpc_info .error .as_deref() - .map(|e| format!("JSON-RPC request rejected: {e}")); + .map(crate::l7::jsonrpc::jsonrpc_parse_rejection_reason); let response_frame_reason = jsonrpc_response_frame_hard_deny_reason(config.protocol, &jsonrpc_info); let force_deny = parse_error_reason.is_some() || response_frame_reason.is_some(); @@ -3307,6 +3307,121 @@ network_policies: ); } + #[derive(Debug, Clone, Copy)] + enum McpBatchEntryPath { + Direct, + RouteSelected, + } + + async fn assert_mcp_batch_denied( + body: &[u8], + entry_path: McpBatchEntryPath, + enforcement: EnforcementMode, + ) { + let (mut config, tunnel_engine, ctx) = mcp_test_relay_context(); + config.enforcement = enforcement; + let (mut app, mut relay_client) = tokio::io::duplex(8192); + let (mut relay_upstream, mut upstream) = tokio::io::duplex(8192); + let relay = tokio::spawn(async move { + match entry_path { + McpBatchEntryPath::Direct => { + relay_with_inspection( + &config, + tunnel_engine, + &mut relay_client, + &mut relay_upstream, + &ctx, + ) + .await + } + McpBatchEntryPath::RouteSelected => { + let configs = [config]; + relay_with_route_selection( + &configs, + tunnel_engine, + &mut relay_client, + &mut relay_upstream, + &ctx, + ) + .await + } + } + }); + + let request = format!( + "POST /mcp HTTP/1.1\r\nHost: mcp.example.test:8000\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n", + body.len() + ); + app.write_all(request.as_bytes()).await.unwrap(); + app.write_all(body).await.unwrap(); + + let mut response = Vec::new(); + tokio::time::timeout( + std::time::Duration::from_secs(1), + app.read_to_end(&mut response), + ) + .await + .expect("relay should close after denying an MCP batch") + .unwrap(); + let response = String::from_utf8(response).expect("denial response should be UTF-8"); + assert!( + response.starts_with("HTTP/1.1 403 Forbidden\r\n"), + "{entry_path:?}/{enforcement:?} returned an unexpected response: {response:?}" + ); + let (_, body) = response + .split_once("\r\n\r\n") + .expect("denial response should contain a body"); + let body: serde_json::Value = + serde_json::from_str(body).expect("denial body should be JSON"); + assert_eq!( + body.get("detail").and_then(serde_json::Value::as_str), + Some(crate::l7::jsonrpc::UNSUPPORTED_MCP_BATCH_REASON), + "{entry_path:?}/{enforcement:?} changed the stable denial detail" + ); + + tokio::time::timeout(std::time::Duration::from_secs(1), relay) + .await + .expect("relay should finish after denying an MCP batch") + .unwrap() + .unwrap(); + let mut byte = [0u8; 1]; + let read = + tokio::time::timeout(std::time::Duration::from_secs(1), upstream.read(&mut byte)) + .await + .expect("upstream side should close") + .unwrap(); + assert_eq!( + read, 0, + "{entry_path:?}/{enforcement:?} forwarded MCP batch bytes upstream" + ); + } + + #[tokio::test] + async fn mcp_rejects_jsonrpc_batches() { + let batches: &[&[u8]] = &[ + br"[]", + br#"[{"jsonrpc":"2.0","id":1,"method":"ping"}]"#, + br#"[{"jsonrpc":"2.0","method":"notifications/initialized"}]"#, + br#"[{"jsonrpc":"2.0","id":1,"result":{}}]"#, + br#"[{"jsonrpc":"2.0","id":1,"method":"ping"},{"jsonrpc":"2.0","id":1,"result":{}}]"#, + ]; + + for body in batches { + assert_mcp_batch_denied(body, McpBatchEntryPath::Direct, EnforcementMode::Enforce) + .await; + } + } + + #[tokio::test] + async fn mcp_batch_entry_path_matrix() { + let body = br#"[{"jsonrpc":"2.0","id":1,"method":"ping"}]"#; + for entry_path in [McpBatchEntryPath::Direct, McpBatchEntryPath::RouteSelected] { + for enforcement in [EnforcementMode::Audit, EnforcementMode::Enforce] { + assert_mcp_batch_denied(body, entry_path, enforcement).await; + } + } + } + #[tokio::test] async fn jsonrpc_relay_forwards_allowed_method() { let (config, tunnel_engine, ctx) = jsonrpc_test_relay_context(); @@ -3373,6 +3488,64 @@ network_policies: .unwrap(); } + #[tokio::test] + async fn jsonrpc_relay_forwards_allowed_batch_separately_from_mcp() { + let (config, tunnel_engine, ctx) = jsonrpc_test_relay_context(); + let (mut app, mut relay_client) = tokio::io::duplex(8192); + let (mut relay_upstream, mut upstream) = tokio::io::duplex(8192); + let relay = tokio::spawn(async move { + relay_with_inspection( + &config, + tunnel_engine, + &mut relay_client, + &mut relay_upstream, + &ctx, + ) + .await + }); + + let body = br#"[{"jsonrpc":"2.0","id":1,"method":"initialize"},{"jsonrpc":"2.0","method":"initialize"}]"#; + let request = format!( + "POST /rpc HTTP/1.1\r\nHost: jsonrpc.example.test:8000\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n", + body.len() + ); + app.write_all(request.as_bytes()).await.unwrap(); + app.write_all(body).await.unwrap(); + + let mut upstream_bytes = Vec::new(); + let mut upstream_buf = [0u8; 1024]; + while !upstream_bytes.ends_with(body) { + let read = tokio::time::timeout( + std::time::Duration::from_secs(1), + upstream.read(&mut upstream_buf), + ) + .await + .expect("allowed generic JSON-RPC batch should reach upstream") + .unwrap(); + assert_ne!(read, 0, "upstream closed before the full batch arrived"); + upstream_bytes.extend_from_slice(&upstream_buf[..read]); + } + + upstream + .write_all(b"HTTP/1.1 204 No Content\r\nContent-Length: 0\r\nConnection: close\r\n\r\n") + .await + .unwrap(); + + let mut response = [0u8; 512]; + let read = tokio::time::timeout(std::time::Duration::from_secs(1), app.read(&mut response)) + .await + .expect("generic JSON-RPC batch response should reach client") + .unwrap(); + assert!(String::from_utf8_lossy(&response[..read]).contains("204 No Content")); + + drop(app); + tokio::time::timeout(std::time::Duration::from_secs(1), relay) + .await + .expect("relay should complete") + .unwrap() + .unwrap(); + } + #[tokio::test] async fn mcp_relay_forwards_jsonrpc_response_frame() { let (config, tunnel_engine, ctx) = mcp_test_relay_context(); diff --git a/crates/openshell-supervisor-network/src/proxy.rs b/crates/openshell-supervisor-network/src/proxy.rs index 0d2c8c0258..447f84ce58 100644 --- a/crates/openshell-supervisor-network/src/proxy.rs +++ b/crates/openshell-supervisor-network/src/proxy.rs @@ -467,7 +467,7 @@ fn forward_l7_hard_deny_reason( request_info.jsonrpc.as_ref().and_then(|info| { info.error .as_deref() - .map(|error| format!("JSON-RPC request rejected: {error}")) + .map(crate::l7::jsonrpc::jsonrpc_parse_rejection_reason) .or_else(|| { crate::l7::relay::jsonrpc_response_frame_hard_deny_reason(protocol, info) }) @@ -475,6 +475,22 @@ fn forward_l7_hard_deny_reason( }) } +fn forward_l7_denial_detail( + force_deny: bool, + method: &str, + host: &str, + port: u16, + path: &str, + reason: &str, +) -> 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 { + return reason.to_string(); + } + format!("{method} {host}:{port}{path} denied by L7 policy: {reason}") +} + /// Emit a denial event to the aggregator channel (if configured). /// Used by `handle_tcp_connection` which owns `Option`. fn emit_denial( @@ -3791,14 +3807,11 @@ async fn handle_forward_proxy( &reason, "forward-l7-deny", ); + let detail = + forward_l7_denial_detail(force_deny, method, &host_lc, port, &path, &reason); respond( client, - &build_json_error_response( - 403, - "Forbidden", - "policy_denied", - &format!("{method} {host_lc}:{port}{path} denied by L7 policy: {reason}"), - ), + &build_json_error_response(403, "Forbidden", "policy_denied", &detail), ) .await?; return Ok(()); @@ -4577,6 +4590,40 @@ network_policies: ); } + #[test] + fn forward_mcp_batch_denial_preserves_stable_detail() { + let jsonrpc = crate::l7::jsonrpc::parse_jsonrpc_body( + br#"[{"jsonrpc":"2.0","id":1,"method":"ping"}]"#, + crate::l7::jsonrpc::JsonRpcInspectionMode::Mcp, + ); + let request_info = crate::l7::L7RequestInfo { + action: "POST".to_string(), + target: "/mcp".to_string(), + query_params: std::collections::HashMap::new(), + graphql: None, + jsonrpc: Some(jsonrpc), + }; + let reason = forward_l7_hard_deny_reason(crate::l7::L7Protocol::Mcp, &request_info) + .expect("MCP batch should be a forward-proxy hard denial"); + let detail = + forward_l7_denial_detail(true, "POST", "mcp.example.test", 8000, "/mcp", &reason); + assert_eq!(detail, crate::l7::jsonrpc::UNSUPPORTED_MCP_BATCH_REASON); + + let response = build_json_error_response(403, "Forbidden", "policy_denied", &detail); + let response = String::from_utf8(response).expect("denial response should be UTF-8"); + assert!(response.starts_with("HTTP/1.1 403 Forbidden\r\n")); + let (_, body) = response + .split_once("\r\n\r\n") + .expect("denial response should contain a body"); + let body: serde_json::Value = + serde_json::from_str(body).expect("denial body should be JSON"); + assert_eq!(body["error"], "policy_denied"); + assert_eq!( + body["detail"], + crate::l7::jsonrpc::UNSUPPORTED_MCP_BATCH_REASON + ); + } + #[test] fn forward_l7_hard_deny_reason_includes_jsonrpc_response_frames() { let request_info = crate::l7::L7RequestInfo { diff --git a/docs/reference/policy-schema.mdx b/docs/reference/policy-schema.mdx index 049ad47979..202ee62473 100644 --- a/docs/reference/policy-schema.mdx +++ b/docs/reference/policy-schema.mdx @@ -288,7 +288,9 @@ Do not combine `method`, `path`, or `query` with `operation_type`, `operation_na MCP rules match sandbox-to-server MCP Streamable HTTP request bodies by MCP method and optional tool selectors. OpenShell parses the underlying JSON-RPC 2.0 envelope, validates known MCP request and notification params, and preserves unknown extension methods as policy-addressable literal method strings. Until OpenShell exposes explicit MCP version profiles, `mcp.allow_all_known_mcp_methods` defaults to `false`, so endpoints require explicit MCP method rules. Set `mcp.allow_all_known_mcp_methods: true` to enable the endpoint method profile; in that mode, rules can omit `method`, and tool selectors are normalized to `tools/call` internally. By default, `tools/call` `params.name` must match the MCP-recommended tool-name pattern `^[A-Za-z0-9_.-]{1,128}$`; configure `mcp.strict_tool_names: false` on the endpoint only to allow a server that intentionally uses names outside that pattern. Wildcard `tool` matchers require `mcp.strict_tool_names` to remain enabled. JSON-RPC responses and server-to-client MCP messages on response bodies or SSE streams are relayed but are not currently parsed for policy enforcement. -Use `rules` for MCP allow rules and `deny_rules` for MCP deny rules. Deny rules take precedence over allow rules. If an MCP endpoint sets `mcp.allow_all_known_mcp_methods: true` and omits `rules`, OpenShell allows all MCP-family methods and all tools, then applies any `deny_rules`. Otherwise, the endpoint must define explicit rules. A broad allow or deny rule whose method matcher includes `tools/call` cannot be combined with tool-specific allow rules because it would bypass or erase the tool filter; add `tool` or `params.name` to scope `tools/call`, or remove the tool-specific rules. In a batch request, one denied call denies the full batch. +Use `rules` for MCP allow rules and `deny_rules` for MCP deny rules. Deny rules take precedence over allow rules. If an MCP endpoint sets `mcp.allow_all_known_mcp_methods: true` and omits `rules`, OpenShell allows all MCP-family methods and all tools, then applies any `deny_rules`. Otherwise, the endpoint must define explicit rules. A broad allow or deny rule whose method matcher includes `tools/call` cannot be combined with tool-specific allow rules because it would bypass or erase the tool filter; add `tool` or `params.name` to scope `tools/call`, or remove the tool-specific rules. + +Each MCP Streamable HTTP POST body must contain one JSON-RPC request, notification, or response. OpenShell rejects every top-level JSON array, including an empty array, before method policy evaluation. The proxy returns HTTP 403 with `detail` set to `unsupported_mcp_batch` and forwards no bytes upstream. | Field | Type | Required | Description | |---|---|---|---| diff --git a/docs/sandboxes/policies.mdx b/docs/sandboxes/policies.mdx index 212ba76fce..39bbf60bf2 100644 --- a/docs/sandboxes/policies.mdx +++ b/docs/sandboxes/policies.mdx @@ -628,7 +628,9 @@ Use `protocol: json-rpc` and `method` when you need generic JSON-RPC 2.0 matchin Generic JSON-RPC policy `params` matchers are not supported. Generic JSON-RPC policy rules match only the JSON-RPC method. For batch requests, OpenShell evaluates each JSON-RPC call independently and denies the whole batch if any call is denied. -For MCP, `tool` accepts a string glob or `{ any: [...] }` matcher for `tools/call` `params.name`. Rules that use `tool` or lower-level `params.name` must set `method: tools/call` unless `mcp.allow_all_known_mcp_methods: true` enables the endpoint method profile. MCP method globs are accepted only for the `tools/` method family, such as `tools/*`; omit `method` instead of writing `method: "*"` only when the endpoint method profile should allow all MCP methods. Omit `tool` to allow all tools for a `tools/call` method rule. OpenShell does not support MCP tool argument matching yet; allowed tools accept all argument payloads by default. Other MCP `params` keys are rejected. For batch requests, OpenShell evaluates each JSON-RPC call independently and denies the whole batch if any call is denied. +For MCP, `tool` accepts a string glob or `{ any: [...] }` matcher for `tools/call` `params.name`. Rules that use `tool` or lower-level `params.name` must set `method: tools/call` unless `mcp.allow_all_known_mcp_methods: true` enables the endpoint method profile. MCP method globs are accepted only for the `tools/` method family, such as `tools/*`; omit `method` instead of writing `method: "*"` only when the endpoint method profile should allow all MCP methods. Omit `tool` to allow all tools for a `tools/call` method rule. OpenShell does not support MCP tool argument matching yet; allowed tools accept all argument payloads by default. Other MCP `params` keys are rejected. + +Each MCP Streamable HTTP POST body must contain one JSON-RPC request, notification, or response. OpenShell rejects every top-level JSON array, including an empty array, before method policy evaluation. The proxy returns HTTP 403 with `detail` set to `unsupported_mcp_batch` and forwards no bytes upstream. ### GraphQL matching