From 8dab7e2e61191dd69e515da2278491f62c0e3ad5 Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Sat, 20 Jun 2026 17:09:27 +0200 Subject: [PATCH] test(mcp): harden symptomatic MCP fixes to construction-enforced MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four guards/refactors that convert previously convention-enforced MCP invariants into ones a future edit can't silently break: - H1 (loopback Host set): standalone.rs surface-guard asserts our loopback allow-list is a superset of rmcp's own default set, so an rmcp bump that adds a loopback form turns red instead of 403'ing that client. Pins the ::1 regression by construction rather than by a hand-kept literal list. - H2 (one stored-query gate): extract a single invoke_query_request() in handlers.rs used by GET /queries, POST /queries/{name}, and (imported) the MCP tools/list + tools/call stored paths. REST and MCP can no longer drift on which Cedar action governs the catalog. Deletes mcp.rs's local copy. - H3 (expose chokepoint): tests/mcp.rs source-walk guard bans .lookup( and registry.iter( in mcp.rs, so the agent surface can only reach stored queries through exposed()/exposed_by_name() — an @mcp(expose:false) query cannot leak back into tools/list via the expose-ignoring registry methods. - H4 (list-gate relaxation lower-bound): a permit-all actor must see every built-in tool, pinning the other end of the list-gate fix (no callable tool may be hidden from tools/list). cargo test -p omnigraph-mcp -p omnigraph-server green (the lone schema_routes flake was disk-pressure during the clean build; passes in isolation). --- crates/omnigraph-mcp/tests/standalone.rs | 21 ++++++++++ crates/omnigraph-server/src/handlers.rs | 35 ++++++++-------- crates/omnigraph-server/src/mcp.rs | 8 +--- crates/omnigraph-server/tests/mcp.rs | 52 ++++++++++++++++++++++++ 4 files changed, 92 insertions(+), 24 deletions(-) diff --git a/crates/omnigraph-mcp/tests/standalone.rs b/crates/omnigraph-mcp/tests/standalone.rs index af53449..b61bdfd 100644 --- a/crates/omnigraph-mcp/tests/standalone.rs +++ b/crates/omnigraph-mcp/tests/standalone.rs @@ -227,6 +227,27 @@ async fn unsupported_protocol_version_header_is_400_except_on_initialize() { assert_eq!(resp.status(), StatusCode::BAD_REQUEST, "non-init bogus version must be 400"); } +#[test] +fn loopback_host_set_covers_rmcp_default() { + // Construction guard for the `::1` regression: our loopback Host allow-list + // must stay a SUPERSET of rmcp's own default loopback set. We keep an + // explicit list (rather than deriving) for clarity, but pin it against the + // substrate default here — so an rmcp bump that adds a loopback form turns + // this red instead of silently 403'ing that client. + use rmcp::transport::streamable_http_server::StreamableHttpServerConfig; + let rmcp_default = StreamableHttpServerConfig::default().allowed_hosts; + assert!(!rmcp_default.is_empty(), "rmcp default should list loopback hosts"); + let ours = McpHostPolicy::from_bind(&"127.0.0.1:0".parse().unwrap(), &[], &[]) + .allowed_hosts + .expect("a loopback bind sets a Host allow-list"); + for host in &rmcp_default { + assert!( + ours.contains(host), + "loopback allow-list {ours:?} must cover rmcp default host {host:?}" + ); + } +} + /// rmcp surface guard — pins the API shapes the transport relies on. Turns red /// (compile error) on an rmcp bump that renames/moves any of these. Compile-only. #[allow(dead_code)] diff --git a/crates/omnigraph-server/src/handlers.rs b/crates/omnigraph-server/src/handlers.rs index 64f259a..8ea3515 100644 --- a/crates/omnigraph-server/src/handlers.rs +++ b/crates/omnigraph-server/src/handlers.rs @@ -457,6 +457,21 @@ pub(crate) fn authorize_any_branch( .map_err(|err| ApiError::internal(format!("policy: {err}"))) } +/// The single Cedar request that gates the **stored-query surface** — catalog +/// discovery *and* invocation, on REST *and* MCP. `invoke_query` is graph-scoped +/// (no branch dimension): the per-branch/snapshot access is enforced by the inner +/// `read`/`change` gate in the runner. Every gate site (`GET /queries`, +/// `POST /queries/{name}`, the MCP `tools/list` stored projection, and MCP +/// `tools/call` stored dispatch) calls this one function, so the REST and MCP +/// surfaces cannot drift on which action governs the catalog. +pub(crate) fn invoke_query_request() -> PolicyRequest { + PolicyRequest { + action: PolicyAction::InvokeQuery, + branch: None, + target_branch: None, + } +} + #[utoipa::path( get, path = "/snapshot", @@ -961,19 +976,7 @@ pub(crate) async fn server_invoke_query( // probed without the grant), but operational failures (401 missing bearer, // 500 policy-evaluation error) propagate with their true status via `?` // rather than being masked as a missing query. - match authorize( - actor_ref, - handle.policy.as_deref(), - PolicyRequest { - action: PolicyAction::InvokeQuery, - // Graph-scoped: no branch dimension. The per-branch/snapshot - // access is enforced by the inner read/change gate in the - // runner, so the outer gate must not resolve a branch (doing so - // was wrong for snapshot reads). - branch: None, - target_branch: None, - }, - )? { + match authorize(actor_ref, handle.policy.as_deref(), invoke_query_request())? { Authz::Allowed => {} Authz::Denied(_) => return Err(ApiError::not_found(NOT_FOUND)), } @@ -1080,11 +1083,7 @@ pub(crate) async fn server_list_queries( authorize_request( actor.as_ref().map(|Extension(actor)| actor), handle.policy.as_deref(), - PolicyRequest { - action: PolicyAction::InvokeQuery, - branch: None, - target_branch: None, - }, + invoke_query_request(), )?; let queries = match handle.queries.as_ref() { Some(registry) => registry diff --git a/crates/omnigraph-server/src/mcp.rs b/crates/omnigraph-server/src/mcp.rs index a35b762..936567e 100644 --- a/crates/omnigraph-server/src/mcp.rs +++ b/crates/omnigraph-server/src/mcp.rs @@ -21,7 +21,8 @@ use omnigraph_mcp::{ }; use crate::handlers::{ - Authz, authorize, authorize_any_branch, authorize_request, run_ingest, run_mutate, run_query, + Authz, authorize, authorize_any_branch, authorize_request, invoke_query_request, run_ingest, + run_mutate, run_query, }; use crate::queries::{QueryRegistry, StoredQuery}; use crate::{ @@ -423,11 +424,6 @@ fn read_request(branch: Option) -> PolicyRequest { PolicyRequest { action: PolicyAction::Read, branch, target_branch: None } } -fn invoke_query_request() -> PolicyRequest { - // Graph-scoped: no branch dimension. - PolicyRequest { action: PolicyAction::InvokeQuery, branch: None, target_branch: None } -} - fn actor_id<'a>(actor: Option<&'a ResolvedActor>) -> Option<&'a str> { actor.map(|a| a.actor_id.as_ref()) } diff --git a/crates/omnigraph-server/tests/mcp.rs b/crates/omnigraph-server/tests/mcp.rs index 6d4801e..4fd68c3 100644 --- a/crates/omnigraph-server/tests/mcp.rs +++ b/crates/omnigraph-server/tests/mcp.rs @@ -693,6 +693,58 @@ async fn stored_query_run_cannot_reach_unexposed_query() { assert_eq!(v["result"]["isError"], json!(true), "unexposed query must not run via stored_query_run: {v}"); } +#[test] +fn mcp_backend_resolves_only_through_the_exposed_chokepoint() { + // Construction guard for the stored-query expose class: the MCP backend must + // reach stored queries ONLY through exposed()/exposed_by_name(), never the + // expose-ignoring QueryRegistry::lookup/iter (those would leak an + // `@mcp(expose: false)` query to the agent surface). HTTP/service callers may + // use lookup/iter; the agent surface may not. A source-walk so a future edit + // that reintroduces the bug fails loudly here. + const SRC: &str = include_str!("../src/mcp.rs"); + assert!( + !SRC.contains(".lookup("), + "mcp.rs must not call registry.lookup — use exposed_by_name()" + ); + assert!( + !SRC.contains("registry.iter("), + "mcp.rs must not call registry.iter — use exposed()" + ); +} + +#[tokio::test] +async fn permit_all_actor_sees_every_builtin_tool() { + // Relaxation lower-bound for `list_gate`: when the policy permits every + // action, NO built-in may be hidden from tools/list (a hidden-but-callable + // tool is the class the list-gate fix closed). Pairs with + // `list_gate_matches_call_for_fixed_branchless_reads`, which pins the + // fixed-branchless reads' faithful gate at the other end. + let (_t, app) = app_for_loaded_graph_with_auth_tokens(&[("act", "tok")]).await; + let (_s, list) = + json_response(&app, mcp_request(Some("tok"), rpc(1, "tools/list", json!({})))).await; + let names = tool_names(&list); + for builtin in [ + "graph_health", + "graph_query", + "graph_snapshot", + "schema_get", + "branch_list", + "commit_list", + "commit_get", + "graph_mutate", + "graph_load", + "branch_create", + "branch_delete", + "branch_merge", + "schema_apply", + ] { + assert!( + names.contains(&builtin.to_string()), + "a permit-all actor must see built-in '{builtin}' (list_gate must not hide a callable tool): {names:?}" + ); + } +} + #[test] fn stored_query_shadowing_a_builtin_is_a_load_error() { // A stored query whose tool name collides with a built-in must fail loudly