mirror of
https://github.com/ModernRelay/omnigraph.git
synced 2026-06-09 01:35:18 +02:00
Scope the stored-query 404-hiding claim to non-invoke_query callers
Review found the deny==404 catalog-hiding was overstated as a contract: it holds only at the outer invoke_query gate. A caller that HOLDS invoke_query but lacks read/change gets the inner gate's 403 for an existing query vs 404 for an unknown one — so existence is visible to grant-holders by design (the intended double-gate). The handler docstring, OpenAPI 404 description, and server.md all claimed the 404 was airtight against any denied actor. Correct the wording in all three (no behavior change) and add the missing symmetric test (invoke_query but no read -> 403 for an existing query, 404 for unknown) so the actual contract is pinned. Also document that in default-deny mode (tokens, no policy) every invocation 404s until an invoke_query rule is configured. Nits: the from_specs collision comment said "first declared wins" but it is lexicographically-first by name (BTreeMap); the effective_tool_name docstring overclaimed the CLI display routes through it (it resolves the rule on its own output DTO).
This commit is contained in:
parent
566e9b7651
commit
f4c38bb75a
5 changed files with 58 additions and 15 deletions
|
|
@ -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<AppState>,
|
||||
Extension(handle): Extension<Arc<GraphHandle>>,
|
||||
|
|
@ -2195,8 +2198,10 @@ async fn server_invoke_query(
|
|||
Path(QueryNamePath { name }): Path<QueryNamePath>,
|
||||
Json(req): Json<InvokeStoredQueryRequest>,
|
||||
) -> std::result::Result<Json<InvokeStoredQueryResponse>, 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);
|
||||
|
||||
|
|
|
|||
|
|
@ -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) {
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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": {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue