From ad2fc278497c80a1a3973f602d258c7d6fddeee5 Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Sun, 31 May 2026 15:45:19 +0200 Subject: [PATCH] Make invoke_query graph-scoped (one branch authority) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit invoke_query gates reaching the curated stored-query surface — a graph-level capability. Per-branch/snapshot access is already enforced by the inner read/change gate in run_query/run_mutate (authorized against the resolved branch), so branch-scoping the outer gate was redundant AND wrong for snapshot reads (it defaulted to main). Drop the branch dimension: remove InvokeQuery from uses_branch_scope (it joins admin as graph-scoped) and authorize the boundary gate with branch: None. Lossless: an actor confined to branch X by their read/change rules can still only invoke a stored query that touches X. A rule that sets branch_scope on invoke_query is now rejected by validate() — write invoke_query in its own rule. Ripple (atomic): restructure the server invoke fixture so invoke_query sits in its own branch_scope-free rule; invert invoke_query_is_branch_scoped -> invoke_query_rejects_branch_scope; the per-graph authorize test uses branch: None; docs (policy.md, server.md, the InvokeQuery doc). No wire/OpenAPI change. --- crates/omnigraph-policy/src/lib.rs | 56 +++++++++++-------------- crates/omnigraph-server/src/lib.rs | 6 ++- crates/omnigraph-server/tests/server.rs | 33 +++++++++------ docs/user/policy.md | 4 +- docs/user/server.md | 2 +- 5 files changed, 53 insertions(+), 48 deletions(-) diff --git a/crates/omnigraph-policy/src/lib.rs b/crates/omnigraph-policy/src/lib.rs index 2df616d..cb59796 100644 --- a/crates/omnigraph-policy/src/lib.rs +++ b/crates/omnigraph-policy/src/lib.rs @@ -57,16 +57,19 @@ pub enum PolicyAction { /// `omnigraph.yaml` and restarting. GraphList, /// Gates invoking a server-side stored query by name. Per-graph and - /// branch-scoped, like `Read`/`Change`. In this release it is - /// **coarse**: an `invoke_query` allow rule permits *any* stored - /// query on the graph (there is no per-query dimension yet). A - /// future, additive refinement adds an optional query-name scope to - /// rules without changing rules written against the coarse action. + /// **graph-scoped** (no branch dimension, like `Admin`): the per-branch + /// access of the query body is enforced by the inner `Read`/`Change` + /// gate, so branch-scoping this outer gate would be redundant (and was + /// wrong for snapshot reads). A rule that sets `branch_scope` on + /// `invoke_query` is rejected by `validate()`. In this release it is + /// **coarse**: an `invoke_query` allow rule permits *any* stored query + /// on the graph (no per-query dimension yet); a future, additive + /// refinement adds an optional query-name scope. /// - /// This gate sits at the HTTP boundary. The underlying engine `_as` - /// writers still enforce `Read`/`Change` per the stored query's body, - /// so a stored *mutation* is double-gated: `invoke_query` to reach - /// the tool, plus `change` for the write itself. + /// This gate sits at the HTTP boundary. The engine `_as` writers still + /// enforce `Read`/`Change` per the query body, so a stored *mutation* + /// is double-gated: `invoke_query` to reach the tool, plus `change` for + /// the write itself. InvokeQuery, } @@ -87,10 +90,7 @@ impl PolicyAction { } fn uses_branch_scope(self) -> bool { - matches!( - self, - Self::Read | Self::Export | Self::Change | Self::InvokeQuery - ) + matches!(self, Self::Read | Self::Export | Self::Change) } fn uses_target_branch_scope(self) -> bool { @@ -1306,7 +1306,7 @@ rules: "act-alice", &PolicyRequest { action: PolicyAction::InvokeQuery, - branch: Some("main".to_string()), + branch: None, target_branch: None, }, ) @@ -1323,7 +1323,7 @@ rules: "act-bruno", &PolicyRequest { action: PolicyAction::InvokeQuery, - branch: Some("main".to_string()), + branch: None, target_branch: None, }, ) @@ -1332,10 +1332,10 @@ rules: } #[test] - fn invoke_query_is_branch_scoped() { - // Unlike server-scoped actions, invoke_query accepts a - // `branch_scope` qualifier — it runs against a branch like - // read/change — so validation passes and the rule authorizes. + fn invoke_query_rejects_branch_scope() { + // invoke_query is graph-scoped (like admin) — per-branch access is + // enforced by the inner read/change gate — so a rule that puts a + // `branch_scope` qualifier on it is rejected at validate(). let policy: PolicyConfig = serde_yaml::from_str( r#" version: 1 @@ -1350,19 +1350,11 @@ rules: "#, ) .unwrap(); - policy.validate().unwrap(); - let engine = PolicyCompiler::compile(&policy, "graph").unwrap(); - let allow = engine - .authorize( - "act-alice", - &PolicyRequest { - action: PolicyAction::InvokeQuery, - branch: Some("review".to_string()), - target_branch: None, - }, - ) - .unwrap(); - assert!(allow.allowed); + let err = policy.validate().unwrap_err().to_string(); + assert!( + err.contains("branch_scope") && err.contains("invoke_query"), + "branch_scope on invoke_query must be rejected: {err}" + ); } #[test] diff --git a/crates/omnigraph-server/src/lib.rs b/crates/omnigraph-server/src/lib.rs index cbac2fa..168c9fb 100644 --- a/crates/omnigraph-server/src/lib.rs +++ b/crates/omnigraph-server/src/lib.rs @@ -2213,7 +2213,11 @@ async fn server_invoke_query( handle.policy.as_deref(), PolicyRequest { action: PolicyAction::InvokeQuery, - branch: req.branch.clone().or_else(|| Some("main".to_string())), + // 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, }, ) diff --git a/crates/omnigraph-server/tests/server.rs b/crates/omnigraph-server/tests/server.rs index 4c07210..7770b18 100644 --- a/crates/omnigraph-server/tests/server.rs +++ b/crates/omnigraph-server/tests/server.rs @@ -249,25 +249,34 @@ groups: invoke_only: ["act-invokeonly"] protected_branches: [main] rules: - - id: invokers-invoke-and-read + # invoke_query is graph-scoped — its own rules, no branch_scope. + - id: invokers-can-invoke allow: actors: { group: invokers } - actions: [invoke_query, read] - branch_scope: any - - id: full-invoke-read-change + actions: [invoke_query] + - id: full-can-invoke allow: actors: { group: full } - actions: [invoke_query, read, change] - branch_scope: any - - id: readers-read-only - allow: - actors: { group: readers } - actions: [read] - branch_scope: any - - id: invoke-only-no-read + actions: [invoke_query] + - id: invoke-only-can-invoke allow: actors: { group: invoke_only } actions: [invoke_query] + # read / change are branch-scoped. + - id: invokers-can-read + allow: + actors: { group: invokers } + actions: [read] + branch_scope: any + - id: full-can-read-change + allow: + actors: { group: full } + actions: [read, change] + branch_scope: any + - id: readers-can-read + allow: + actors: { group: readers } + actions: [read] branch_scope: any "#; diff --git a/docs/user/policy.md b/docs/user/policy.md index 5e96684..21acda8 100644 --- a/docs/user/policy.md +++ b/docs/user/policy.md @@ -14,7 +14,7 @@ Per-graph actions (bind to `Omnigraph::Graph::""`): 6. `branch_delete` 7. `branch_merge` 8. `admin` — reserved for policy-management surfaces (hot reload, audit log, approvals). No call site today; see MR-724 for the reservation rationale. -9. `invoke_query` — gates invoking a server-side stored query (the `queries:` registry). Branch-scoped, like `read` / `change`. Coarse in this release: an `invoke_query` allow rule permits any stored query on the graph; a future, additive refinement adds an optional per-query-name scope without changing rules written against the coarse action. Enforced at `POST /queries/{name}` (see [server](server.md)). A stored *mutation* is double-gated: `invoke_query` to reach the tool, plus `change` for the write itself (the engine `_as` writers still enforce per the query body). +9. `invoke_query` — gates invoking a server-side stored query (the `queries:` registry). Graph-scoped (like `admin`) — per-branch access is enforced by the inner `read` / `change` gate, so a rule that sets `branch_scope` on `invoke_query` is rejected. Coarse in this release: an `invoke_query` allow rule permits any stored query on the graph; a future, additive refinement adds an optional per-query-name scope without changing rules written against the coarse action. Enforced at `POST /queries/{name}` (see [server](server.md)). A stored *mutation* is double-gated: `invoke_query` to reach the tool, plus `change` for the write itself (the engine `_as` writers still enforce per the query body). Server-scoped action (v0.6.0+; binds to `Omnigraph::Server::"root"`): @@ -24,7 +24,7 @@ Server-scoped actions cannot use `branch_scope` or `target_branch_scope` — the ## Scope kinds -- `branch_scope` — applied to source branch (`read`, `export`, `change`, `invoke_query`) +- `branch_scope` — applied to source branch (`read`, `export`, `change`) - `target_branch_scope` — applied to destination (`schema_apply`, branch ops, run ops) - `protected_branches` — named list with special rules; rule scopes are `any | protected | unprotected` diff --git a/docs/user/server.md b/docs/user/server.md index 48f0a3f..d67002e 100644 --- a/docs/user/server.md +++ b/docs/user/server.md @@ -68,7 +68,7 @@ List the graph's **`mcp.expose`** stored queries as a typed tool catalog — eno 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`. +- **Gate:** `invoke_query` (per-graph, graph-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, 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.