Skip to content

Commit 8f378eb

Browse files
committed
fix(l7): ignore unsupported JSON-RPC params
Signed-off-by: Kris Hicks <khicks@nvidia.com>
1 parent 7b7e297 commit 8f378eb

5 files changed

Lines changed: 116 additions & 269 deletions

File tree

crates/openshell-supervisor-network/data/sandbox-policy.rego

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -742,8 +742,8 @@ query_value_matches(value, matcher) if {
742742
glob.match(any_patterns[i], [], value)
743743
}
744744

745-
# JSON-RPC-family method matching. Generic JSON-RPC policies match only method;
746-
# MCP policies also match scalar params, including params.name from tool aliases.
745+
# JSON-RPC-family method matching. Generic JSON-RPC policies match only method.
746+
# MCP policies may also match params.name from tool aliases.
747747
jsonrpc_rule_matches(request, endpoint, rule) if {
748748
jsonrpc := object.get(request, "jsonrpc", null)
749749
is_object(jsonrpc)

crates/openshell-supervisor-network/src/l7/jsonrpc.rs

Lines changed: 76 additions & 137 deletions
Original file line numberDiff line numberDiff line change
@@ -101,12 +101,12 @@ pub struct JsonRpcRequestInfo {
101101
pub struct JsonRpcCallInfo {
102102
/// JSON-RPC method, or the MCP method name after typed MCP parsing.
103103
pub method: String,
104-
/// Flattened scalar params used by the current Rego matcher path. Strings,
105-
/// numbers, and booleans are represented as strings for compatibility with
106-
/// the existing query matcher implementation.
104+
/// Policy-visible params for JSON-RPC-family matching. Generic JSON-RPC
105+
/// leaves this empty because params matching is not supported. MCP exposes
106+
/// only `params.name` for tools/call tool selection.
107107
pub params: HashMap<String, String>,
108-
/// MCP `tools/call` tool name when known. Generic JSON-RPC leaves this as
109-
/// a best-effort projection of `params.name`.
108+
/// MCP `tools/call` tool name when known. Generic JSON-RPC leaves this
109+
/// unset because params are not inspected.
110110
pub tool: Option<String>,
111111
}
112112

@@ -288,12 +288,10 @@ fn parse_jsonrpc_call(
288288
.get("method")
289289
.and_then(|m| m.as_str())
290290
.ok_or_else(|| "missing or non-string 'method' field".to_string())?;
291-
let params = flatten_jsonrpc_params_opt(value.get("params"))?;
292-
let tool = params.get("name").cloned();
293291
Ok(JsonRpcCallInfo {
294292
method: method.to_string(),
295-
params,
296-
tool,
293+
params: HashMap::new(),
294+
tool: None,
297295
})
298296
}
299297

@@ -345,9 +343,10 @@ fn parse_mcp_call(
345343
validate_mcp_tool_name(tool_name)?;
346344
}
347345

346+
let params = mcp_policy_params(tool.as_deref());
348347
return Ok(JsonRpcCallInfo {
349348
method: mcp_request.method_name().to_string(),
350-
params: flatten_jsonrpc_params_opt(request.params.as_ref())?,
349+
params,
351350
tool,
352351
});
353352
}
@@ -367,28 +366,11 @@ fn parse_mcp_call(
367366

368367
Ok(JsonRpcCallInfo {
369368
method: notification.method,
370-
params: flatten_jsonrpc_params_opt(notification.params.as_ref())?,
369+
params: HashMap::new(),
371370
tool: None,
372371
})
373372
}
374373

375-
fn flatten_jsonrpc_params(
376-
value: &serde_json::Value,
377-
) -> std::result::Result<HashMap<String, String>, String> {
378-
let mut params = HashMap::<String, FlattenedParam>::new();
379-
flatten_json_value("", 0, value, &mut params)?;
380-
Ok(params
381-
.into_iter()
382-
.map(|(key, param)| (key, param.value))
383-
.collect())
384-
}
385-
386-
fn flatten_jsonrpc_params_opt(
387-
value: Option<&serde_json::Value>,
388-
) -> std::result::Result<HashMap<String, String>, String> {
389-
value.map_or_else(|| Ok(HashMap::new()), flatten_jsonrpc_params)
390-
}
391-
392374
fn mcp_tool_name(request: &McpRequest) -> Option<String> {
393375
if let McpRequest::CallTool(params) = request {
394376
Some(params.name.clone())
@@ -397,6 +379,14 @@ fn mcp_tool_name(request: &McpRequest) -> Option<String> {
397379
}
398380
}
399381

382+
fn mcp_policy_params(tool: Option<&str>) -> HashMap<String, String> {
383+
let mut params = HashMap::new();
384+
if let Some(tool) = tool {
385+
params.insert("name".to_string(), tool.to_string());
386+
}
387+
params
388+
}
389+
400390
// OpenShell's default MCP hardening enforces the spec-recommended tool-name
401391
// boundary for tools/call. See McpOptions in proto/sandbox.proto for sources.
402392
fn validate_mcp_tool_name(name: &str) -> std::result::Result<(), String> {
@@ -413,69 +403,6 @@ fn validate_mcp_tool_name(name: &str) -> std::result::Result<(), String> {
413403
}
414404
Ok(())
415405
}
416-
417-
fn flatten_json_value(
418-
prefix: &str,
419-
path_segments: usize,
420-
value: &serde_json::Value,
421-
out: &mut HashMap<String, FlattenedParam>,
422-
) -> std::result::Result<(), String> {
423-
// Keep the runtime input flat for the existing OPA matcher. Literal dotted
424-
// keys are accepted; if they collide with a flattened nested path, the
425-
// literal key wins because it has fewer JSON path segments.
426-
match value {
427-
serde_json::Value::Object(map) => {
428-
for (key, child) in map {
429-
let next_path_segments = path_segments + 1;
430-
let next = if prefix.is_empty() {
431-
key.clone()
432-
} else {
433-
format!("{prefix}.{key}")
434-
};
435-
flatten_json_value(&next, next_path_segments, child, out)?;
436-
}
437-
}
438-
serde_json::Value::String(s) if !prefix.is_empty() => {
439-
insert_flattened_param(out, prefix, s.clone(), path_segments)?;
440-
}
441-
serde_json::Value::Number(n) if !prefix.is_empty() => {
442-
insert_flattened_param(out, prefix, n.to_string(), path_segments)?;
443-
}
444-
serde_json::Value::Bool(b) if !prefix.is_empty() => {
445-
insert_flattened_param(out, prefix, b.to_string(), path_segments)?;
446-
}
447-
_ => {}
448-
}
449-
Ok(())
450-
}
451-
452-
#[derive(Debug, Clone)]
453-
struct FlattenedParam {
454-
value: String,
455-
path_segments: usize,
456-
}
457-
458-
fn insert_flattened_param(
459-
out: &mut HashMap<String, FlattenedParam>,
460-
key: &str,
461-
value: String,
462-
path_segments: usize,
463-
) -> std::result::Result<(), String> {
464-
let param = FlattenedParam {
465-
value,
466-
path_segments,
467-
};
468-
if let Some(existing) = out.get_mut(key) {
469-
if param.path_segments < existing.path_segments {
470-
*existing = param;
471-
} else if param.path_segments == existing.path_segments {
472-
return Err(format!("ambiguous params key collision at '{key}'"));
473-
}
474-
return Ok(());
475-
}
476-
out.insert(key.to_string(), param);
477-
Ok(())
478-
}
479406
#[cfg(test)]
480407
mod tests {
481408
use super::*;
@@ -519,21 +446,32 @@ mod tests {
519446

520447
#[test]
521448
fn ignores_params_when_extracting_method() {
522-
let body = br#"{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"submit_report","arguments":{"scope":"workspace/main"}}}"#;
449+
let body = br#"{"jsonrpc":"2.0","id":1,"method":"reports.search","params":{"query":"quarterly","filters":{"scope":"workspace/main"}}}"#;
523450
let info = parse_jsonrpc_body(body, JsonRpcInspectionMode::JsonRpc);
524451
assert!(info.error.is_none());
525452
assert_eq!(
526453
info.calls.first().map(|call| call.method.as_str()),
527-
Some("tools/call")
454+
Some("reports.search")
528455
);
529-
let params = &info.calls.first().expect("single request call").params;
456+
let call = info.calls.first().expect("single request call");
457+
assert!(call.params.is_empty());
458+
assert!(call.tool.is_none());
459+
}
460+
461+
#[test]
462+
fn ignores_dotted_param_collisions_for_generic_jsonrpc() {
463+
let body = br#"{"jsonrpc":"2.0","id":1,"method":"reports.search","params":{"filters.scope":{"value":"literal"},"filters":{"scope.value":"nested"}}}"#;
464+
let info = parse_jsonrpc_body(body, JsonRpcInspectionMode::JsonRpc);
465+
466+
assert!(info.error.is_none(), "params should be ignored: {info:?}");
530467
assert_eq!(
531-
params.get("name").map(String::as_str),
532-
Some("submit_report")
468+
info.calls.first().map(|call| call.method.as_str()),
469+
Some("reports.search")
533470
);
534-
assert_eq!(
535-
params.get("arguments.scope").map(String::as_str),
536-
Some("workspace/main")
471+
assert!(
472+
info.calls
473+
.first()
474+
.is_some_and(|call| call.params.is_empty() && call.tool.is_none())
537475
);
538476
}
539477

@@ -547,9 +485,10 @@ mod tests {
547485
assert_eq!(call.method, "tools/call");
548486
assert_eq!(call.tool.as_deref(), Some("search_web"));
549487
assert_eq!(
550-
call.params.get("arguments.query").map(String::as_str),
551-
Some("openshell")
488+
call.params.get("name").map(String::as_str),
489+
Some("search_web")
552490
);
491+
assert_eq!(call.params.len(), 1);
553492
}
554493

555494
#[test]
@@ -583,6 +522,10 @@ mod tests {
583522
.expect("permissive MCP call should parse");
584523
assert!(info.error.is_none(), "permissive MCP call failed: {info:?}");
585524
assert_eq!(call.tool.as_deref(), Some("read status"));
525+
assert_eq!(
526+
call.params.get("name").map(String::as_str),
527+
Some("read status")
528+
);
586529
}
587530

588531
#[test]
@@ -613,31 +556,43 @@ mod tests {
613556
info.calls.first().map(|call| call.method.as_str()),
614557
Some("vendor/extension")
615558
);
559+
assert!(
560+
info.calls
561+
.first()
562+
.is_some_and(|call| call.params.is_empty() && call.tool.is_none())
563+
);
616564
}
617565

618566
#[test]
619-
fn allows_literal_dotted_param_keys() {
620-
let body = br#"{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"arguments.scope":"workspace/other","arguments":{"scope":"workspace/main"}}}"#;
621-
let info = parse_jsonrpc_body(body, JsonRpcInspectionMode::JsonRpc);
622-
let params = &info.calls.first().expect("single request call").params;
567+
fn mcp_mode_ignores_tool_arguments_when_extracting_policy_params() {
568+
let body = br#"{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"read_status","arguments":{"scope.key":"literal","scope":{"key":"nested"}}}}"#;
569+
let info = parse_jsonrpc_body(body, JsonRpcInspectionMode::Mcp);
570+
let call = info.calls.first().expect("single MCP call");
623571

624-
assert!(info.error.is_none());
572+
assert!(info.error.is_none(), "expected valid MCP call: {info:?}");
573+
assert_eq!(call.tool.as_deref(), Some("read_status"));
625574
assert_eq!(
626-
params.get("arguments.scope").map(String::as_str),
627-
Some("workspace/other")
575+
call.params.get("name").map(String::as_str),
576+
Some("read_status")
628577
);
578+
assert_eq!(call.params.len(), 1);
629579
}
630580

631581
#[test]
632582
fn accepts_any_valid_jsonrpc_params_shape() {
633583
let body =
634-
br#"{"jsonrpc":"2.0","id":1,"method":"tools/call","params":["ignored",{"nested":true}]}"#;
584+
br#"{"jsonrpc":"2.0","id":1,"method":"reports.search","params":["ignored",{"nested":true}]}"#;
635585
let info = parse_jsonrpc_body(body, JsonRpcInspectionMode::JsonRpc);
636586

637587
assert!(info.error.is_none());
638588
assert_eq!(
639589
info.calls.first().map(|call| call.method.as_str()),
640-
Some("tools/call")
590+
Some("reports.search")
591+
);
592+
assert!(
593+
info.calls
594+
.first()
595+
.is_some_and(|call| call.params.is_empty() && call.tool.is_none())
641596
);
642597
}
643598

@@ -676,7 +631,7 @@ mod tests {
676631

677632
#[test]
678633
fn rejects_requests_missing_jsonrpc_version() {
679-
let body = br#"{"id":1,"method":"tools/list"}"#;
634+
let body = br#"{"id":1,"method":"reports.list"}"#;
680635
let info = parse_jsonrpc_body(body, JsonRpcInspectionMode::JsonRpc);
681636

682637
assert!(info.calls.is_empty());
@@ -689,8 +644,8 @@ mod tests {
689644
#[test]
690645
fn rejects_batch_items_missing_jsonrpc_version() {
691646
let body = br#"[
692-
{"jsonrpc":"2.0","id":1,"method":"tools/list"},
693-
{"id":2,"method":"tools/call","params":{"name":"read_status"}}
647+
{"jsonrpc":"2.0","id":1,"method":"reports.list"},
648+
{"id":2,"method":"reports.search","params":{"query":"quarterly"}}
694649
]"#;
695650
let info = parse_jsonrpc_body(body, JsonRpcInspectionMode::JsonRpc);
696651

@@ -704,7 +659,7 @@ mod tests {
704659

705660
#[test]
706661
fn rejects_unsupported_jsonrpc_version() {
707-
let body = br#"{"jsonrpc":"1.0","id":1,"method":"tools/list"}"#;
662+
let body = br#"{"jsonrpc":"1.0","id":1,"method":"reports.list"}"#;
708663
let info = parse_jsonrpc_body(body, JsonRpcInspectionMode::JsonRpc);
709664

710665
assert!(info.calls.is_empty());
@@ -714,41 +669,25 @@ mod tests {
714669
);
715670
}
716671

717-
#[test]
718-
fn detects_flattened_param_collisions() {
719-
let mut params = HashMap::from([(
720-
"arguments.scope".to_string(),
721-
FlattenedParam {
722-
value: "first".to_string(),
723-
path_segments: 2,
724-
},
725-
)]);
726-
727-
let error = insert_flattened_param(&mut params, "arguments.scope", "second".to_string(), 2)
728-
.expect_err("duplicate flattened key should be ambiguous");
729-
730-
assert!(error.contains("ambiguous params key collision"));
731-
}
732-
733672
#[test]
734673
fn parses_valid_batch_without_error() {
735674
let body = br#"[
736-
{"jsonrpc":"2.0","id":1,"method":"tools/list"},
737-
{"jsonrpc":"2.0","id":2,"method":"tools/call","params":{"name":"read_status"}}
675+
{"jsonrpc":"2.0","id":1,"method":"reports.list"},
676+
{"jsonrpc":"2.0","id":2,"method":"reports.search","params":{"query":"quarterly"}}
738677
]"#;
739678
let info = parse_jsonrpc_body(body, JsonRpcInspectionMode::JsonRpc);
740679
assert!(info.error.is_none());
741680
assert!(info.is_batch);
742681
assert!(!info.has_response);
743682
assert_eq!(info.calls.len(), 2);
744-
assert_eq!(info.calls[0].method, "tools/list");
745-
assert_eq!(info.calls[1].method, "tools/call");
683+
assert_eq!(info.calls[0].method, "reports.list");
684+
assert_eq!(info.calls[1].method, "reports.search");
746685
}
747686

748687
#[test]
749688
fn parses_batch_with_calls_and_responses() {
750689
let body = br#"[
751-
{"jsonrpc":"2.0","id":1,"method":"tools/list"},
690+
{"jsonrpc":"2.0","id":1,"method":"reports.list"},
752691
{"jsonrpc":"2.0","id":2,"result":{"ok":true}}
753692
]"#;
754693
let info = parse_jsonrpc_body(body, JsonRpcInspectionMode::JsonRpc);
@@ -757,7 +696,7 @@ mod tests {
757696
assert!(info.is_batch);
758697
assert!(info.has_response);
759698
assert_eq!(info.calls.len(), 1);
760-
assert_eq!(info.calls[0].method, "tools/list");
699+
assert_eq!(info.calls[0].method, "reports.list");
761700
}
762701

763702
#[test]
@@ -796,7 +735,7 @@ mod tests {
796735
#[test]
797736
fn rejects_batch_item_with_method_and_result() {
798737
let body = br#"[
799-
{"jsonrpc":"2.0","id":1,"method":"tools/list"},
738+
{"jsonrpc":"2.0","id":1,"method":"reports.list"},
800739
{"jsonrpc":"2.0","id":2,"method":"initialize","result":{}}
801740
]"#;
802741
let info = parse_jsonrpc_body(body, JsonRpcInspectionMode::JsonRpc);

0 commit comments

Comments
 (0)