diff --git a/crates/omnigraph-server/src/lib.rs b/crates/omnigraph-server/src/lib.rs index 10d2b2d..6f32e15 100644 --- a/crates/omnigraph-server/src/lib.rs +++ b/crates/omnigraph-server/src/lib.rs @@ -2174,7 +2174,7 @@ struct QueryNamePath { (status = 400, description = "Bad request (param type error; snapshot on a stored mutation)", body = ErrorOutput), (status = 401, description = "Unauthorized", body = ErrorOutput), (status = 403, description = "Forbidden (the inner `change` gate for a stored mutation)", body = ErrorOutput), - (status = 404, description = "Unknown stored query, or `invoke_query` denied (indistinguishable)", body = ErrorOutput), + (status = 404, description = "Unknown stored query, or `invoke_query` denied — indistinguishable to a caller without the grant", body = ErrorOutput), (status = 409, description = "Merge conflict", body = ErrorOutput), (status = 429, description = "Per-actor admission cap exceeded; honor `Retry-After` header", body = ErrorOutput), ), @@ -2186,8 +2186,11 @@ struct QueryNamePath { /// request body — callers send only runtime inputs (`params`, `branch`, /// `snapshot`). Gated by the `invoke_query` Cedar action at the boundary; /// a stored *mutation* additionally passes the engine's `change` gate -/// (double-gated). A denied actor and an unknown query both return the -/// same 404, so the catalog can't be probed. +/// (double-gated). An actor **without** `invoke_query` cannot tell a denied +/// query from a missing one — both return the same 404, so the catalog +/// can't be probed without the grant. Once `invoke_query` is held, the +/// inner `read`/`change` gate may surface a 403 for an existing query the +/// actor can't run (the intended double-gate signal). async fn server_invoke_query( State(state): State, Extension(handle): Extension>, @@ -2195,8 +2198,10 @@ async fn server_invoke_query( Path(QueryNamePath { name }): Path, Json(req): Json, ) -> std::result::Result, ApiError> { - // Deny is indistinguishable from a missing query: both 404 with this - // exact message, so an unauthorized caller can't probe the catalog. + // A caller without `invoke_query` can't tell a denial from a missing + // query: both 404 with this exact message, so the catalog can't be + // probed without the grant. (A caller that holds invoke_query may still + // see the inner gate's 403 for an existing query it can't run — intended.) const NOT_FOUND: &str = "stored query not found"; let actor_ref = actor.as_ref().map(|Extension(actor)| actor); diff --git a/crates/omnigraph-server/src/queries.rs b/crates/omnigraph-server/src/queries.rs index 868866b..07a4883 100644 --- a/crates/omnigraph-server/src/queries.rs +++ b/crates/omnigraph-server/src/queries.rs @@ -52,10 +52,10 @@ impl StoredQuery { } /// The MCP tool name this query is catalogued under: the explicit - /// `tool_name` override, else the query `name`. This is the catalog - /// key — enforced unique across exposed queries at load — and the one - /// definition every consumer (uniqueness check, catalog projection, - /// CLI display) reads, so the resolution can't drift between them. + /// `tool_name` override, else the query `name`. The catalog key — + /// enforced unique across exposed queries at load. Server-side + /// consumers (the uniqueness check, the future catalog projection) read + /// this; the CLI `queries list` resolves the same rule on its own DTO. pub fn effective_tool_name(&self) -> &str { self.tool_name.as_deref().unwrap_or(&self.name) } @@ -141,9 +141,11 @@ impl QueryRegistry { // Exposed queries are catalogued under their effective tool name; // two claiming one name is an MCP-namespace collision. Refuse it at // load (collected, not fail-fast), naming the loser and the winner. - // Iterating the `BTreeMap` keeps "first declared wins" and the error - // order deterministic. Scoped to a block so these borrows of - // `by_name` end before it is moved into `Self`. + // Iterating the `BTreeMap` makes the winner deterministic (the + // lexicographically-first query name; config is a map, so YAML + // declaration order isn't preserved anyway) and the error order + // stable. Scoped to a block so these borrows of `by_name` end + // before it is moved into `Self`. { let mut claimed: BTreeMap<&str, &str> = BTreeMap::new(); for query in by_name.values().filter(|q| q.expose) { diff --git a/crates/omnigraph-server/tests/server.rs b/crates/omnigraph-server/tests/server.rs index ca5fa58..77f1664 100644 --- a/crates/omnigraph-server/tests/server.rs +++ b/crates/omnigraph-server/tests/server.rs @@ -239,12 +239,14 @@ async fn app_with_stored_queries( /// - `act-invoke`: invoke_query + read (stored reads, not mutations) /// - `act-full`: invoke_query + read + change (stored mutations) /// - `act-noinvoke`: read only, no invoke_query (boundary-denied) +/// - `act-invokeonly`: invoke_query only, no read (clears the boundary, inner read denies) const INVOKE_POLICY_YAML: &str = r#" version: 1 groups: invokers: ["act-invoke"] full: ["act-full"] readers: ["act-noinvoke"] + invoke_only: ["act-invokeonly"] protected_branches: [main] rules: - id: invokers-invoke-and-read @@ -262,6 +264,11 @@ rules: actors: { group: readers } actions: [read] branch_scope: any + - id: invoke-only-no-read + allow: + actors: { group: invoke_only } + actions: [invoke_query] + branch_scope: any "#; const FIND_PERSON_GQ: &str = @@ -380,6 +387,34 @@ async fn invoke_unknown_query_and_denied_actor_return_identical_404() { ); } +#[tokio::test(flavor = "multi_thread")] +async fn invoke_query_holder_without_read_sees_403_not_404() { + // The 404-hiding is for callers WITHOUT invoke_query. An actor that + // HOLDS invoke_query but lacks `read` clears the boundary gate, then the + // inner read gate denies → 403 for an EXISTING read query, vs 404 for an + // unknown one. Existence is visible to grant-holders by design (the + // documented double-gate); this pins that actual contract. + let (_temp, app) = app_with_stored_queries( + &[("find_person", FIND_PERSON_GQ, false)], + &[("act-invokeonly", "t-invokeonly")], + INVOKE_POLICY_YAML, + ) + .await; + let (exists_status, _) = json_response( + &app, + invoke_request("find_person", "t-invokeonly", json!({ "params": { "name": "Alice" } })), + ) + .await; + let (absent_status, _) = + json_response(&app, invoke_request("does_not_exist", "t-invokeonly", json!({}))).await; + assert_eq!( + exists_status, + StatusCode::FORBIDDEN, + "an existing read query the holder can't read → inner-gate 403" + ); + assert_eq!(absent_status, StatusCode::NOT_FOUND, "unknown query still 404s"); +} + fn drifted_test_schema() -> String { fs::read_to_string(fixture("test.pg")) .unwrap() diff --git a/docs/user/server.md b/docs/user/server.md index b6dcaea..6b20926 100644 --- a/docs/user/server.md +++ b/docs/user/server.md @@ -60,7 +60,8 @@ Server-level management endpoints (v0.6.0+): Invoke a curated, server-side stored query by **name** — the source comes from the graph's `queries:` registry, so the client never sends `.gq`. Body (all fields optional): `{ "params": { … }, "branch": "main", "snapshot": null }`, where `params` keys match the query's declared parameters. The response is the **read envelope** (`ReadOutput`) for a stored read or the **mutation envelope** (`ChangeOutput`) for a stored mutation — serialized untagged, so the wire shape is identical to `/query` / `/mutate`. - **Gate:** `invoke_query` (per-graph, branch-scoped) at the boundary. A stored *mutation* is **double-gated** — it also passes the engine's `change` gate, so an actor with `invoke_query` but not `change` gets `403`. -- **Deny == unknown:** an `invoke_query` denial and an unknown query name return the **same `404`** (identical body), so an unauthorized caller cannot probe the catalog. +- **Deny == unknown, for callers without `invoke_query`:** for a caller lacking the grant, an `invoke_query` denial and an unknown query name return the **same `404`** (identical body), so the catalog can't be probed. A caller that *holds* `invoke_query` may still get the inner gate's `403` for an existing query it can't `read`/`change` (the double-gate, above) — so existence is visible to grant-holders by design. +- **Requires an explicit policy grant when auth is on.** In default-deny mode (bearer tokens but no `policy.file`), only `read` is permitted, so *every* `/queries/{name}` call returns `404` until an `invoke_query` rule is configured. - A stored mutation cannot target a `snapshot` (`400`); a parameter type error is a structured `400` naming the parameter. ## Adding and removing graphs (multi mode) diff --git a/openapi.json b/openapi.json index f0108c3..e8f8dd6 100644 --- a/openapi.json +++ b/openapi.json @@ -835,7 +835,7 @@ "queries" ], "summary": "Invoke a curated, server-side stored query by name.", - "description": "The query source comes from the graph's `queries:` registry, not the\nrequest body — callers send only runtime inputs (`params`, `branch`,\n`snapshot`). Gated by the `invoke_query` Cedar action at the boundary;\na stored *mutation* additionally passes the engine's `change` gate\n(double-gated). A denied actor and an unknown query both return the\nsame 404, so the catalog can't be probed.", + "description": "The query source comes from the graph's `queries:` registry, not the\nrequest body — callers send only runtime inputs (`params`, `branch`,\n`snapshot`). Gated by the `invoke_query` Cedar action at the boundary;\na stored *mutation* additionally passes the engine's `change` gate\n(double-gated). An actor **without** `invoke_query` cannot tell a denied\nquery from a missing one — both return the same 404, so the catalog\ncan't be probed without the grant. Once `invoke_query` is held, the\ninner `read`/`change` gate may surface a 403 for an existing query the\nactor can't run (the intended double-gate signal).", "operationId": "invoke_query", "parameters": [ { @@ -900,7 +900,7 @@ } }, "404": { - "description": "Unknown stored query, or `invoke_query` denied (indistinguishable)", + "description": "Unknown stored query, or `invoke_query` denied — indistinguishable to a caller without the grant", "content": { "application/json": { "schema": {