From ef3855933a8b4f44912a3957a8694745fe385d83 Mon Sep 17 00:00:00 2001 From: Shiju Date: Mon, 13 Jul 2026 14:40:49 +0530 Subject: [PATCH] refactor(jsonrpc): carry typed inspection errors Signed-off-by: Shiju --- .../src/l7/jsonrpc.rs | 204 ++++++++++++++---- .../src/l7/relay.rs | 67 ++++-- .../openshell-supervisor-network/src/proxy.rs | 46 ++-- 3 files changed, 243 insertions(+), 74 deletions(-) diff --git a/crates/openshell-supervisor-network/src/l7/jsonrpc.rs b/crates/openshell-supervisor-network/src/l7/jsonrpc.rs index 7122edeee0..8469906a07 100644 --- a/crates/openshell-supervisor-network/src/l7/jsonrpc.rs +++ b/crates/openshell-supervisor-network/src/l7/jsonrpc.rs @@ -94,9 +94,70 @@ pub struct JsonRpcRequestInfo { pub is_batch: bool, pub receive_stream: bool, pub has_response: bool, - pub error: Option, + /// Typed inspection failure discovered before policy evaluation. + pub error: Option, } +/// Stable kind of failure found while inspecting a JSON-RPC-family message. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[non_exhaustive] +pub enum JsonRpcInspectionErrorKind { + /// The request body is not valid JSON. + InvalidJson, + /// The decoded JSON fails JSON-RPC or protocol-specific message validation. + InvalidMessage, +} + +/// Typed JSON-RPC inspection failure with an inseparable kind and diagnostic. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct JsonRpcInspectionError { + kind: JsonRpcInspectionErrorKind, + detail: String, +} + +impl JsonRpcInspectionError { + fn invalid_json() -> Self { + Self { + kind: JsonRpcInspectionErrorKind::InvalidJson, + detail: "invalid JSON".to_string(), + } + } + + fn invalid_message(detail: impl Into) -> Self { + Self { + kind: JsonRpcInspectionErrorKind::InvalidMessage, + detail: detail.into(), + } + } + + /// Return the stable failure kind used by typed control flow. + #[must_use] + pub const fn kind(&self) -> JsonRpcInspectionErrorKind { + self.kind + } + + /// Return the existing policy-visible and human-readable diagnostic. + /// + /// Callers must use [`Self::kind`] instead of parsing this text for control flow. + #[must_use] + pub fn detail(&self) -> &str { + &self.detail + } + + /// Render the existing proxy denial reason for an inspection failure. + pub(crate) fn rejection_reason(&self) -> String { + format!("JSON-RPC request rejected: {self}") + } +} + +impl std::fmt::Display for JsonRpcInspectionError { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter.write_str(&self.detail) + } +} + +impl std::error::Error for JsonRpcInspectionError {} + #[derive(Debug, Clone, PartialEq, Eq)] pub struct JsonRpcCallInfo { /// JSON-RPC method, or the MCP method name after typed MCP parsing. @@ -111,6 +172,16 @@ pub struct JsonRpcCallInfo { } impl JsonRpcRequestInfo { + fn rejected(is_batch: bool, error: JsonRpcInspectionError) -> Self { + Self { + calls: Vec::new(), + is_batch, + receive_stream: false, + has_response: false, + error: Some(error), + } + } + /// MCP streamable HTTP uses an empty GET to receive server messages. It has /// no request body to inspect, but it must still pass through MCP endpoints. pub(crate) fn receive_stream() -> Self { @@ -167,24 +238,15 @@ pub fn parse_jsonrpc_body_with_options( inspection_options: JsonRpcInspectionOptions, ) -> JsonRpcRequestInfo { let Ok(value) = serde_json::from_slice::(body) else { - return JsonRpcRequestInfo { - calls: Vec::new(), - is_batch: false, - receive_stream: false, - has_response: false, - error: Some("invalid JSON".to_string()), - }; + return JsonRpcRequestInfo::rejected(false, JsonRpcInspectionError::invalid_json()); }; if let serde_json::Value::Array(items) = value { if items.is_empty() { - return JsonRpcRequestInfo { - calls: Vec::new(), - is_batch: true, - receive_stream: false, - has_response: false, - error: Some("empty batch".to_string()), - }; + return JsonRpcRequestInfo::rejected( + true, + JsonRpcInspectionError::invalid_message("empty batch"), + ); } let mut calls = Vec::new(); let mut has_response = false; @@ -193,13 +255,12 @@ pub fn parse_jsonrpc_body_with_options( Ok(JsonRpcMessageInfo::Call(call)) => calls.push(call), Ok(JsonRpcMessageInfo::Response) => has_response = true, Err(error) => { - return JsonRpcRequestInfo { - calls: Vec::new(), - is_batch: true, - receive_stream: false, - has_response: false, - error: Some(format!("batch item invalid: {error}")), - }; + return JsonRpcRequestInfo::rejected( + true, + JsonRpcInspectionError::invalid_message(format!( + "batch item invalid: {error}" + )), + ); } } } @@ -227,13 +288,9 @@ pub fn parse_jsonrpc_body_with_options( has_response: true, error: None, }, - Err(error) => JsonRpcRequestInfo { - calls: Vec::new(), - is_batch: false, - receive_stream: false, - has_response: false, - error: Some(error), - }, + Err(error) => { + JsonRpcRequestInfo::rejected(false, JsonRpcInspectionError::invalid_message(error)) + } } } @@ -444,6 +501,47 @@ mod tests { assert!(info.error.is_none()); } + #[test] + fn inspection_error_kinds_distinguish_invalid_json_and_messages() { + fn assert_inspection_error( + body: &[u8], + mode: JsonRpcInspectionMode, + expected_kind: JsonRpcInspectionErrorKind, + expected_detail: &str, + ) { + let info = parse_jsonrpc_body(body, mode); + let error = info.error.as_ref().expect("inspection failure"); + + assert_eq!(error.kind(), expected_kind); + assert_eq!(error.detail(), expected_detail); + } + + assert_inspection_error( + b"{", + JsonRpcInspectionMode::JsonRpc, + JsonRpcInspectionErrorKind::InvalidJson, + "invalid JSON", + ); + assert_inspection_error( + br"[]", + JsonRpcInspectionMode::JsonRpc, + JsonRpcInspectionErrorKind::InvalidMessage, + "empty batch", + ); + assert_inspection_error( + br"null", + JsonRpcInspectionMode::JsonRpc, + JsonRpcInspectionErrorKind::InvalidMessage, + "missing or non-string 'jsonrpc' field", + ); + assert_inspection_error( + br#"[{"jsonrpc":"2.0","id":1,"method":"reports.list"},{"id":2,"method":"reports.search"}]"#, + JsonRpcInspectionMode::JsonRpc, + JsonRpcInspectionErrorKind::InvalidMessage, + "batch item invalid: missing or non-string 'jsonrpc' field", + ); + } + #[test] fn ignores_params_when_extracting_method() { let body = br#"{"jsonrpc":"2.0","id":1,"method":"reports.search","params":{"query":"quarterly","filters":{"scope":"workspace/main"}}}"#; @@ -499,7 +597,8 @@ mod tests { assert!(info.calls.is_empty()); assert!( info.error - .as_deref() + .as_ref() + .map(JsonRpcInspectionError::detail) .is_some_and(|error| error.contains("strict_tool_names")), "expected strict tool-name error, got {info:?}" ); @@ -534,9 +633,14 @@ mod tests { let info = parse_jsonrpc_body(body, JsonRpcInspectionMode::Mcp); assert!(info.calls.is_empty()); + assert_eq!( + info.error.as_ref().map(JsonRpcInspectionError::kind), + Some(JsonRpcInspectionErrorKind::InvalidMessage) + ); assert!( info.error - .as_deref() + .as_ref() + .map(JsonRpcInspectionError::detail) .is_some_and(|error| error.contains("invalid MCP request params")), "expected MCP params validation error, got {info:?}" ); @@ -636,7 +740,7 @@ mod tests { assert!(info.calls.is_empty()); assert_eq!( - info.error.as_deref(), + info.error.as_ref().map(JsonRpcInspectionError::detail), Some("missing or non-string 'jsonrpc' field") ); } @@ -652,7 +756,7 @@ mod tests { assert!(info.calls.is_empty()); assert!(info.is_batch); assert_eq!( - info.error.as_deref(), + info.error.as_ref().map(JsonRpcInspectionError::detail), Some("batch item invalid: missing or non-string 'jsonrpc' field") ); } @@ -664,7 +768,7 @@ mod tests { assert!(info.calls.is_empty()); assert_eq!( - info.error.as_deref(), + info.error.as_ref().map(JsonRpcInspectionError::detail), Some("unsupported JSON-RPC version '1.0'") ); } @@ -684,6 +788,24 @@ mod tests { assert_eq!(info.calls[1].method, "reports.search"); } + #[test] + fn preserves_current_mcp_batch_acceptance() { + let body = br#"[ + {"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"read_status","arguments":{}}}, + {"jsonrpc":"2.0","id":2,"method":"tools/call","params":{"name":"search_web","arguments":{"query":"openshell"}}} + ]"#; + let info = parse_jsonrpc_body(body, JsonRpcInspectionMode::Mcp); + + assert!( + info.error.is_none(), + "MCP batch should remain valid: {info:?}" + ); + assert!(info.is_batch); + assert_eq!(info.calls.len(), 2); + assert_eq!(info.calls[0].tool.as_deref(), Some("read_status")); + assert_eq!(info.calls[1].tool.as_deref(), Some("search_web")); + } + #[test] fn parses_batch_with_calls_and_responses() { let body = br#"[ @@ -708,7 +830,7 @@ mod tests { assert!(info.calls.is_empty()); assert!(!info.has_response); assert_eq!( - info.error.as_deref(), + info.error.as_ref().map(JsonRpcInspectionError::detail), Some("JSON-RPC response includes both result and error") ); } @@ -719,7 +841,10 @@ mod tests { let result_info = parse_jsonrpc_body(result_body, JsonRpcInspectionMode::JsonRpc); assert!(result_info.calls.is_empty()); assert_eq!( - result_info.error.as_deref(), + result_info + .error + .as_ref() + .map(JsonRpcInspectionError::detail), Some("JSON-RPC message includes both method and result/error") ); @@ -727,7 +852,10 @@ mod tests { let error_info = parse_jsonrpc_body(error_body, JsonRpcInspectionMode::JsonRpc); assert!(error_info.calls.is_empty()); assert_eq!( - error_info.error.as_deref(), + error_info + .error + .as_ref() + .map(JsonRpcInspectionError::detail), Some("JSON-RPC message includes both method and result/error") ); } @@ -743,7 +871,7 @@ mod tests { assert!(info.calls.is_empty()); assert!(info.is_batch); assert_eq!( - info.error.as_deref(), + info.error.as_ref().map(JsonRpcInspectionError::detail), Some("batch item invalid: JSON-RPC message includes both method and result/error") ); } diff --git a/crates/openshell-supervisor-network/src/l7/relay.rs b/crates/openshell-supervisor-network/src/l7/relay.rs index c43fb35f9d..3efdc80057 100644 --- a/crates/openshell-supervisor-network/src/l7/relay.rs +++ b/crates/openshell-supervisor-network/src/l7/relay.rs @@ -408,8 +408,8 @@ where .or_else(|| { jsonrpc_info .as_ref() - .and_then(|info| info.error.as_deref()) - .map(|error| format!("JSON-RPC request rejected: {error}")) + .and_then(|info| info.error.as_ref()) + .map(crate::l7::jsonrpc::JsonRpcInspectionError::rejection_reason) }); let force_deny = parse_error_reason.is_some(); let (allowed, reason) = if let Some(reason) = parse_error_reason { @@ -1085,8 +1085,8 @@ where let parse_error_reason = jsonrpc_info .error - .as_deref() - .map(|e| format!("JSON-RPC request rejected: {e}")); + .as_ref() + .map(crate::l7::jsonrpc::JsonRpcInspectionError::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(); @@ -1611,6 +1611,27 @@ fn jsonrpc_request_for_call( item_request } +fn jsonrpc_policy_input(info: &crate::l7::jsonrpc::JsonRpcRequestInfo) -> serde_json::Value { + let call = if info.is_batch { + None + } else { + info.calls.first() + }; + serde_json::json!({ + "method": call.map(|call| call.method.as_str()), + "params": call.map(|call| &call.params), + "tool": call.and_then(|call| call.tool.as_deref()), + "receive_stream": info.receive_stream, + "has_response": info.has_response, + // Rust keeps the inspection failure kind typed. Rego's stable boundary is + // still the original diagnostic string or null. + "error": info + .error + .as_ref() + .map(crate::l7::jsonrpc::JsonRpcInspectionError::detail), + }) +} + fn evaluate_l7_request_once( engine: &TunnelPolicyEngine, ctx: &L7EvalContext, @@ -1639,17 +1660,7 @@ fn evaluate_l7_request_once( "path": request.target, "query_params": request.query_params.clone(), "graphql": request.graphql.clone(), - "jsonrpc": request.jsonrpc.as_ref().map(|j| { - let call = if j.is_batch { None } else { j.calls.first() }; - serde_json::json!({ - "method": call.map(|call| call.method.as_str()), - "params": call.map(|call| &call.params), - "tool": call.and_then(|call| call.tool.as_deref()), - "receive_stream": j.receive_stream, - "has_response": j.has_response, - "error": j.error, - }) - }), + "jsonrpc": request.jsonrpc.as_ref().map(jsonrpc_policy_input), } }); @@ -2384,6 +2395,32 @@ network_policies: assert!(reason.contains("WEBSOCKET_TEXT /ws not permitted")); } + #[test] + fn jsonrpc_inspection_error_opa_projection_remains_string_or_null() { + let invalid_json = crate::l7::jsonrpc::parse_jsonrpc_body( + b"{", + crate::l7::jsonrpc::JsonRpcInspectionMode::JsonRpc, + ); + let invalid_message = crate::l7::jsonrpc::parse_jsonrpc_body( + br#"{"id":1,"method":"reports.list"}"#, + crate::l7::jsonrpc::JsonRpcInspectionMode::JsonRpc, + ); + let accepted = crate::l7::jsonrpc::parse_jsonrpc_body( + br#"{"jsonrpc":"2.0","id":1,"method":"reports.list"}"#, + crate::l7::jsonrpc::JsonRpcInspectionMode::JsonRpc, + ); + + assert_eq!( + jsonrpc_policy_input(&invalid_json)["error"], + serde_json::json!("invalid JSON") + ); + assert_eq!( + jsonrpc_policy_input(&invalid_message)["error"], + serde_json::json!("missing or non-string 'jsonrpc' field") + ); + assert!(jsonrpc_policy_input(&accepted)["error"].is_null()); + } + #[test] fn jsonrpc_batch_evaluates_each_call() { let data = r#" diff --git a/crates/openshell-supervisor-network/src/proxy.rs b/crates/openshell-supervisor-network/src/proxy.rs index 38cab79c79..db6315dd86 100644 --- a/crates/openshell-supervisor-network/src/proxy.rs +++ b/crates/openshell-supervisor-network/src/proxy.rs @@ -468,8 +468,8 @@ fn forward_l7_hard_deny_reason( .or_else(|| { request_info.jsonrpc.as_ref().and_then(|info| { info.error - .as_deref() - .map(|error| format!("JSON-RPC request rejected: {error}")) + .as_ref() + .map(crate::l7::jsonrpc::JsonRpcInspectionError::rejection_reason) .or_else(|| { crate::l7::relay::jsonrpc_response_frame_hard_deny_reason(protocol, info) }) @@ -4862,27 +4862,31 @@ network_policies: #[test] fn forward_l7_hard_deny_reason_includes_jsonrpc_errors() { - let request_info = crate::l7::L7RequestInfo { - action: "POST".to_string(), - target: "/rpc".to_string(), - query_params: std::collections::HashMap::new(), - graphql: None, - jsonrpc: Some(crate::l7::jsonrpc::JsonRpcRequestInfo { - calls: Vec::new(), - is_batch: false, - receive_stream: false, - has_response: false, - error: Some("missing or non-string 'jsonrpc' field".to_string()), - }), - }; + let cases: &[(&[u8], &str)] = &[ + (b"{", "JSON-RPC request rejected: invalid JSON"), + ( + br#"{"id":1,"method":"reports.list"}"#, + "JSON-RPC request rejected: missing or non-string 'jsonrpc' field", + ), + ]; - let reason = forward_l7_hard_deny_reason(crate::l7::L7Protocol::JsonRpc, &request_info) - .expect("JSON-RPC parse error"); + for &(body, expected_reason) in cases { + let request_info = crate::l7::L7RequestInfo { + action: "POST".to_string(), + target: "/rpc".to_string(), + query_params: std::collections::HashMap::new(), + graphql: None, + jsonrpc: Some(crate::l7::jsonrpc::parse_jsonrpc_body( + body, + crate::l7::jsonrpc::JsonRpcInspectionMode::JsonRpc, + )), + }; - assert_eq!( - reason, - "JSON-RPC request rejected: missing or non-string 'jsonrpc' field" - ); + let reason = forward_l7_hard_deny_reason(crate::l7::L7Protocol::JsonRpc, &request_info) + .expect("JSON-RPC parse error"); + + assert_eq!(reason, expected_reason); + } } #[test]