From 65e53933fb91c1bad6c1ffb4e8a2c7cbe50a91d7 Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Thu, 28 May 2026 11:27:01 +0200 Subject: [PATCH] mr-668: server-scoped actions always require explicit policy (green) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `server_graphs_list` returned 200 in Open mode (`--unauthenticated`, no tokens, no policy) because `authorize_request`'s no-policy fallback only denied when `actor.is_some()` AND action != Read. In Open mode `actor: None`, so the denial branch never fired and the call returned `Ok(())` — leaking the registry (graph IDs + URIs that may contain S3 bucket paths or internal hostnames) to any unauthenticated caller. The docstring on `server_graphs_list` claimed it was "Cedar-gated" and that the server should "not leak the registry until the operator explicitly authorizes it" — docstring intent and code disagreed. Symptomatic fix: special-case GraphList. Breaks the moment another server-scoped action (`graph_create`, `graph_delete`) is added. Correct-by-design fix: tie authorization to the action's `resource_kind()`. Server-scoped actions (`PolicyResourceKind::Server`) always require explicit policy authorization — there is no runtime state where they're served by default. Per-graph actions keep the existing default-deny logic (DefaultDeny denies non-Read for authenticated actors; Open mode allows everything per the operator's `--unauthenticated` opt-in for graph DATA, but not for server topology). The fix uses the existing `PolicyResourceKind` enum that #119 already added — no new abstraction. Future server-scoped actions (runtime `graph_create`/`graph_delete` when the cluster catalog ships) automatically pick up the same enforcement without any per-action handler change. Changes: * `crates/omnigraph-server/src/lib.rs:51` — re-export `PolicyResourceKind` (the kind discriminator was already public on the omnigraph-policy crate; needed in scope here). * `crates/omnigraph-server/src/lib.rs:1457` — `authorize_request`'s no-policy fallback gains a server-scoped-action check that fires before the actor-based default-deny logic. Error message names the failure mode and points at `server.policy.file`. * `crates/omnigraph-server/tests/server.rs:5037` — `get_graphs_with_server_policy_authorizes_per_cedar` extended to register two graphs in non-alphabetical order and assert the admin-200 response is sorted alphabetically. Restores the sort-order coverage that lived in `get_graphs_lists_registered_graphs_in_multi_mode` before the red commit renamed it to assert denial. Also bundles a small adjacent cleanup that the bot-review flagged: * `crates/omnigraph-server/src/graph_id.rs:124` — drop the unreachable `"openapi.json"` entry from `is_reserved`. The regex `^[a-zA-Z0-9-]{1,64}$` rejects every dot-containing name before `is_reserved` can run, so dotted entries in this list were dead code that misled readers into thinking the list needed to cover them. Comment now names the structural exclusion. The `rejects_reserved_route_names` test loses its `openapi.json` row (covered by `rejects_dots` via the regex). Closes the "server-scoped management actions silently leak in Open mode" class. Red test from the previous commit (`get_graphs_denied_in_open_mode_without_server_policy`) turns green; all 78 server integration tests + 76 lib tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/omnigraph-server/src/graph_id.rs | 21 ++++++--- crates/omnigraph-server/src/lib.rs | 42 +++++++++++++----- crates/omnigraph-server/tests/server.rs | 58 +++++++++++++++++-------- 3 files changed, 83 insertions(+), 38 deletions(-) diff --git a/crates/omnigraph-server/src/graph_id.rs b/crates/omnigraph-server/src/graph_id.rs index ea0bcb6..d1a96c2 100644 --- a/crates/omnigraph-server/src/graph_id.rs +++ b/crates/omnigraph-server/src/graph_id.rs @@ -119,13 +119,16 @@ fn regex() -> &'static Regex { /// Reserved `graph_id` values that the regex alone wouldn't catch. /// The leading-underscore rule already excludes every engine-managed -/// filename pattern (`_schema.pg`, `__manifest`, etc.); this list covers -/// route-prefix collisions and standard endpoint names. +/// filename pattern (`_schema.pg`, `__manifest`, etc.); the regex +/// `^[a-zA-Z0-9-]{1,64}$` (see `regex()`) additionally rejects every +/// dot-containing name structurally — `openapi.json` and friends +/// never reach this check. +/// +/// This list only needs to cover route-prefix collisions and +/// top-level endpoint names whose spellings DO satisfy the regex +/// (no dots, no underscores). fn is_reserved(value: &str) -> bool { - matches!( - value, - "policies" | "healthz" | "openapi" | "openapi.json" | "graphs" - ) + matches!(value, "policies" | "healthz" | "openapi" | "graphs") } #[cfg(test)] @@ -205,7 +208,11 @@ mod tests { #[test] fn rejects_reserved_route_names() { - for bad in ["policies", "healthz", "openapi", "openapi.json", "graphs"] { + // Names that satisfy the regex but are still reserved because + // they'd collide with top-level route prefixes / endpoint names. + // Dot-containing names (e.g. `openapi.json`) are rejected by the + // regex, not this list — `rejects_dots` above covers them. + for bad in ["policies", "healthz", "openapi", "graphs"] { assert!( GraphId::try_from(bad).is_err(), "expected reject (reserved): {bad}" diff --git a/crates/omnigraph-server/src/lib.rs b/crates/omnigraph-server/src/lib.rs index 2509f82..cbf9e0c 100644 --- a/crates/omnigraph-server/src/lib.rs +++ b/crates/omnigraph-server/src/lib.rs @@ -50,7 +50,7 @@ use omnigraph_compiler::query::parser::parse_query; use omnigraph_compiler::{JsonParamMode, ParamMap}; pub use policy::{ PolicyAction, PolicyCompiler, PolicyConfig, PolicyDecision, PolicyEngine, PolicyExpectation, - PolicyRequest, PolicyTestConfig, + PolicyRequest, PolicyResourceKind, PolicyTestConfig, }; use serde::Deserialize; use serde_json::Value; @@ -1455,20 +1455,38 @@ fn authorize_request( request: PolicyRequest, ) -> std::result::Result<(), ApiError> { let Some(engine) = policy else { - // MR-723 default-deny path. We're here when no PolicyEngine is - // installed. Two startup-validated shapes can reach this: + // No PolicyEngine installed. Three runtime states can reach this: // // * **Open mode** (`--unauthenticated`): no tokens, no policy. - // `require_bearer_auth` short-circuits before this is called, - // but defense in depth — if a future change makes the - // middleware call here for an unauthenticated request, we - // want every action to remain Ok rather than 403. The - // operator opted in. + // Per-graph operations are open by operator opt-in (they + // accepted "trust the network" for graph data). // * **DefaultDeny mode**: tokens configured but no policy. The - // request went through bearer auth, so `actor` is Some and - // identifies a known actor. Only `Read` is permitted; every - // other action returns 403. This closes the "configured auth - // but forgot the policy file" trap from MR-723. + // request went through bearer auth, so `actor` is Some. Only + // per-graph `Read` is permitted; other per-graph actions + // return 403. Closes the "configured auth but forgot the + // policy file" trap from MR-723. + // * Either of the above with a **server-scoped** action + // (`graph_list`, future `graph_create`/`graph_delete`). + // + // Server-scoped actions are always denied here, regardless of + // mode or actor presence. The management surface leaks server + // topology (graph IDs + URIs that may contain S3 bucket paths + // or internal hostnames) — operators who opted into Open mode + // accepted exposure of graph DATA, not exposure of server + // topology. Closing the management surface by default in every + // runtime state means the docstring contract on + // `server_graphs_list` ("don't leak the registry until the + // operator explicitly authorizes it") holds uniformly; the + // operator's only path to enabling it is configuring an + // explicit `server.policy.file` in omnigraph.yaml. + if request.action.resource_kind() == PolicyResourceKind::Server { + return Err(ApiError::forbidden( + "server-scoped actions require an explicit `server.policy.file` \ + configured in omnigraph.yaml — the management surface is closed \ + by default in every runtime state, including --unauthenticated, \ + so that server topology is never exposed without operator opt-in.", + )); + } if actor.is_some() && request.action != PolicyAction::Read { return Err(ApiError::forbidden( "server runs in default-deny mode (bearer tokens configured but no \ diff --git a/crates/omnigraph-server/tests/server.rs b/crates/omnigraph-server/tests/server.rs index 73223b3..9548baa 100644 --- a/crates/omnigraph-server/tests/server.rs +++ b/crates/omnigraph-server/tests/server.rs @@ -5034,23 +5034,39 @@ graphs: assert_eq!(resp_authed.status(), StatusCode::FORBIDDEN); } - /// `GET /graphs` with a server policy that allows `graph_list` → 200. - /// `GET /graphs` with a server policy that does NOT allow `graph_list` → 403. + /// `GET /graphs` with a server policy that allows `graph_list` → 200 + /// and returns the registry sorted alphabetically by `graph_id`. + /// `GET /graphs` with a server policy that does NOT allow + /// `graph_list` (viewer group) → 403. + /// + /// This test owns the alphabetical-sort coverage that previously + /// lived in `get_graphs_lists_registered_graphs_in_multi_mode`. + /// That test now asserts denial in Open mode (server-scoped actions + /// require explicit policy in every runtime state), so the positive + /// body-shape assertions need a home where the response is + /// operator-authorized — here. #[tokio::test(flavor = "multi_thread")] async fn get_graphs_with_server_policy_authorizes_per_cedar() { use omnigraph_policy::PolicyEngine; use omnigraph_server::{GraphHandle, GraphId, GraphKey}; let dir = tempfile::tempdir().unwrap(); - let graph_uri = dir.path().join("alpha").to_str().unwrap().to_string(); + + // Two graphs deliberately registered in non-alphabetical order + // so the test would fail if the handler relied on insertion + // order instead of server-side sorting. let schema = fs::read_to_string(fixture("test.pg")).unwrap(); - let engine = Omnigraph::init(&graph_uri, &schema).await.unwrap(); - let handle = Arc::new(GraphHandle { - key: GraphKey::cluster(GraphId::try_from("alpha").unwrap()), - uri: graph_uri, - engine: Arc::new(engine), - policy: None, - }); + let mut handles = Vec::new(); + for id in ["beta", "alpha"] { + let graph_uri = dir.path().join(id).to_str().unwrap().to_string(); + let engine = Omnigraph::init(&graph_uri, &schema).await.unwrap(); + handles.push(Arc::new(GraphHandle { + key: GraphKey::cluster(GraphId::try_from(id).unwrap()), + uri: graph_uri, + engine: Arc::new(engine), + policy: None, + })); + } // Server policy: admins can graph_list, viewers cannot. let policy_path = dir.path().join("server-policy.yaml"); @@ -5076,17 +5092,11 @@ rules: ("act-bruno".to_string(), "bruno-token".to_string()), ]; let workload = omnigraph_server::workload::WorkloadController::from_env(); - let state = AppState::new_multi( - vec![handle], - tokens, - Some(server_policy), - workload, - None, - ) - .unwrap(); + let state = AppState::new_multi(handles, tokens, Some(server_policy), workload, None) + .unwrap(); let app = build_app(state); - // Admin → 200 + // Admin → 200, body returns both graphs alphabetically sorted. let resp_admin = app .clone() .oneshot( @@ -5104,6 +5114,16 @@ rules: StatusCode::OK, "admin must be allowed graph_list" ); + let body = to_bytes(resp_admin.into_body(), usize::MAX).await.unwrap(); + let json: Value = serde_json::from_slice(&body).unwrap(); + let graphs = json["graphs"].as_array().unwrap(); + assert_eq!(graphs.len(), 2, "response must list both registered graphs"); + assert_eq!( + graphs[0]["graph_id"].as_str().unwrap(), + "alpha", + "server must sort graphs alphabetically by graph_id (insertion order was 'beta', 'alpha')" + ); + assert_eq!(graphs[1]["graph_id"].as_str().unwrap(), "beta"); // Viewer → 403 let resp_viewer = app