mirror of
https://github.com/ModernRelay/omnigraph.git
synced 2026-06-21 02:28:07 +02:00
test(mcp): harden symptomatic MCP fixes to construction-enforced
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).
This commit is contained in:
parent
fbf455a250
commit
8dab7e2e61
4 changed files with 92 additions and 24 deletions
|
|
@ -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)]
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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<String>) -> 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())
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue