From 7a86f654d490de703f496e5a5d8a8948fe50da7b Mon Sep 17 00:00:00 2001 From: Andrew Altshuler Date: Sun, 17 May 2026 02:51:34 +0300 Subject: [PATCH] policy: codify signed-token-claim-only actor identity (MR-731) (#101) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Warm-up commit for the policy chassis epic (MR-722). PR #1 of the chassis series — same role as schema-lint v1's commit #1 baseline. Zero behavioral change; establishes the regression test, the load-bearing doc comment, and the user-doc paragraph for an invariant already true in code. Server auth already resolves `actor_id` from the matched bearer token at `omnigraph-server/src/lib.rs:692-694`, overwriting whatever the handler put in the PolicyRequest. The principle is named in docs/dev/invariants.md Hard Invariant 11 ("clients cannot set actor identity directly"). What was missing: a regression test, a load-bearing doc comment at the resolution site, and a user-facing documentation paragraph. This commit adds all three. Why first. The actor-identity invariant is the foundation every other policy decision stands on. If `actor_id` can be spoofed, every chassis primitive (per-row scope, audit log, two-person rule) becomes ungated. Pinning the invariant first means PR #2 (the chassis core) doesn't have to re-prove this assertion. Changes: * crates/omnigraph-server/tests/server.rs — new regression test actor_id_resolves_from_bearer_token_ignoring_client_supplied_headers with three sub-assertions: - spoof-up: bearer for denied actor + X-Actor-Id naming allowed actor → 403 (header doesn't promote) - spoof-down: bearer for allowed actor + X-Actor-Id naming denied actor → 200 (header doesn't demote) - empty-string spoof: empty X-Actor-Id doesn't clear resolved actor Cross-link to MR-777 (auth boundary cases — actor-id collision + malformed bearer) noted in the test docstring. * crates/omnigraph-server/src/lib.rs — expanded doc comment at the actor-resolution site explaining the SECURITY INVARIANT, citing Hard Invariant 11, the Supabase RLS history footgun, and the regression test that pins the contract. Reader thinking "I should let clients override actor_id for impersonation" hits this comment first. * docs/user/policy.md — new "Actor identity (signed-claim-only)" section near the existing Server enforcement section. Closes the user-facing doc gap MR-731's "Done when" requires. Architectural decisions for PR #2+ pinned this session (not implemented here, recorded so future implementers don't re-litigate): - PolicyEngine moves to new `omnigraph-policy` workspace crate so both engine and server can depend on it (Q2). - `enforce(action, scope, actor)` will take a new `ResourceScope` enum, leaving room for MR-725's per-type and per-row variants (Q3). - `PolicyAction::Admin` is kept and wired (Option A) — meta-action for policy-management surfaces (hot reload, audit log query, approvals list) as those consumer features land (Q4). Test results: - cargo test -p omnigraph-server --test server: 45 pass (44 existing + 1 new); no regressions - scripts/check-agents-md.sh: passes (34 links / 33 docs OK) Out of scope (PR #2+): - Omnigraph::with_policy() + enforce() method - omnigraph-policy crate creation - ResourceScope enum - CLI policy injection into Omnigraph - HTTP-layer redundant-check removal - MR-724 Admin action wiring (PR #2) - MR-723 default-deny 3-state (PR #4) - MR-736 severity warn/deny (PR #5) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 --- crates/omnigraph-server/src/lib.rs | 16 ++- crates/omnigraph-server/tests/server.rs | 126 ++++++++++++++++++++++++ docs/user/policy.md | 8 ++ 3 files changed, 148 insertions(+), 2 deletions(-) diff --git a/crates/omnigraph-server/src/lib.rs b/crates/omnigraph-server/src/lib.rs index 5b63eb0..7e911b6 100644 --- a/crates/omnigraph-server/src/lib.rs +++ b/crates/omnigraph-server/src/lib.rs @@ -689,8 +689,20 @@ fn authorize_request( let Some(actor) = actor else { return Err(ApiError::unauthorized("missing bearer token")); }; - // Authoritative actor_id is the authenticated session, not whatever the - // handler put in the request. Prevents an empty-string default at any + // SECURITY INVARIANT (MR-731): actor identity comes from the matched + // bearer token, never from a client-supplied request header, query + // parameter, or body field. This line is the single chokepoint where + // the authoritative actor (resolved from the bearer match by + // `require_bearer_auth`) overwrites whatever the handler put in the + // PolicyRequest. Removing or weakening it lets clients spoof identity — + // exactly the Supabase RLS footgun ("trusting raw_user_meta_data is + // asking the attacker if they're an admin"). The principle is codified + // in `docs/dev/invariants.md` Hard Invariant 11 ("clients cannot set + // actor identity directly") and pinned by the regression test + // `actor_id_resolves_from_bearer_token_ignoring_client_supplied_headers` + // in `crates/omnigraph-server/tests/server.rs`. + // + // Side effect: also prevents an empty-string default at any handler // call site from ever reaching the engine as a policy subject. request.actor_id = actor.as_str().to_string(); let decision = engine diff --git a/crates/omnigraph-server/tests/server.rs b/crates/omnigraph-server/tests/server.rs index 03f4aa7..998da23 100644 --- a/crates/omnigraph-server/tests/server.rs +++ b/crates/omnigraph-server/tests/server.rs @@ -975,6 +975,132 @@ rules: assert_eq!(bad_status, StatusCode::UNAUTHORIZED); } +/// Regression test for MR-731: actor identity comes from the matched +/// bearer token, never from a client-supplied request header. A future +/// "convenience" PR that lets clients override `actor_id` to spoof +/// another identity must break this test. The principle is named in +/// `docs/dev/invariants.md` Hard Invariant 11 and at the actor-resolution +/// site in `omnigraph-server/src/lib.rs::authorize_request`. +/// +/// Two assertions in one fixture: +/// 1. Spoof-up: bearer for a *denied* actor + X-Actor-Id naming an +/// *allowed* actor — policy still denies (proves the spoof header +/// doesn't promote the request). +/// 2. Spoof-down: bearer for an *allowed* actor + X-Actor-Id naming a +/// *denied* actor — policy still allows (proves the server-resolved +/// identity wins; the spoof can't trick the request into a denial +/// either, which would otherwise be a confusing UX trap). +/// +/// Cross-reference: MR-777 covers boundary cases like actor-id +/// *collision* (two distinct tokens minting the same actor_id) and +/// malformed bearer header parsing. See `auth_boundary_case_coverage` +/// suite when it lands; the two tests together pin the full bearer-token +/// → actor identity contract. +#[tokio::test(flavor = "multi_thread")] +async fn actor_id_resolves_from_bearer_token_ignoring_client_supplied_headers() { + let temp = init_loaded_repo().await; + let repo = repo_path(temp.path()); + let policy_path = temp.path().join("policy.yaml"); + // Same readers/writers split as + // `bearer_token_resolves_to_correct_actor_for_policy_decisions` — + // `act-a` can read main, `act-b` cannot. The asymmetry is what + // makes the spoof-up/spoof-down distinction observable. + fs::write( + &policy_path, + r#" +version: 1 +groups: + readers: [act-a] + writers: [act-b] +protected_branches: [main] +rules: + - id: readers-only + allow: + actors: { group: readers } + actions: [read] + branch_scope: any +"#, + ) + .unwrap(); + let state = AppState::open_with_bearer_tokens_and_policy( + repo.to_string_lossy().to_string(), + vec![ + ("act-a".to_string(), "token-a".to_string()), + ("act-b".to_string(), "token-b".to_string()), + ], + Some(&policy_path), + ) + .await + .unwrap(); + let app = build_app(state); + + // (1) Spoof-up: bearer for act-b (denied) + X-Actor-Id: act-a (allowed). + // If the server were trusting the header, this would succeed as + // act-a. The contract is: the bearer wins. Expect 403 because + // act-b can't read. + let (spoof_up_status, spoof_up_body) = json_response( + &app, + Request::builder() + .uri("/snapshot?branch=main") + .method(Method::GET) + .header("authorization", "Bearer token-b") + .header("x-actor-id", "act-a") + .body(Body::empty()) + .unwrap(), + ) + .await; + let spoof_up_error: ErrorOutput = serde_json::from_value(spoof_up_body).unwrap(); + assert_eq!( + spoof_up_status, + StatusCode::FORBIDDEN, + "X-Actor-Id must not promote a denied bearer to an allowed actor", + ); + assert_eq!( + spoof_up_error.code, + Some(omnigraph_server::api::ErrorCode::Forbidden), + ); + + // (2) Spoof-down: bearer for act-a (allowed) + X-Actor-Id: act-b (denied). + // If the server were trusting the header, this would fail as act-b. + // The contract is: the bearer wins. Expect 200 because act-a can read. + let (spoof_down_status, _) = json_response( + &app, + Request::builder() + .uri("/snapshot?branch=main") + .method(Method::GET) + .header("authorization", "Bearer token-a") + .header("x-actor-id", "act-b") + .body(Body::empty()) + .unwrap(), + ) + .await; + assert_eq!( + spoof_down_status, + StatusCode::OK, + "X-Actor-Id must not demote an allowed bearer to a denied actor", + ); + + // (3) Empty-string spoof attempt: an X-Actor-Id of "" must not + // leak through as the policy subject. Same expectation as (1): + // bearer for act-b is denied regardless of what the header tries. + let (empty_spoof_status, _) = json_response( + &app, + Request::builder() + .uri("/snapshot?branch=main") + .method(Method::GET) + .header("authorization", "Bearer token-b") + .header("x-actor-id", "") + .body(Body::empty()) + .unwrap(), + ) + .await; + assert_eq!( + empty_spoof_status, + StatusCode::FORBIDDEN, + "empty X-Actor-Id must not clear the resolved actor", + ); +} + #[tokio::test(flavor = "multi_thread")] async fn policy_allows_read_but_distinguishes_401_from_403() { let (_temp, app) = app_for_loaded_repo_with_auth_tokens_and_policy( diff --git a/docs/user/policy.md b/docs/user/policy.md index 746e35f..e0f1fda 100644 --- a/docs/user/policy.md +++ b/docs/user/policy.md @@ -42,3 +42,11 @@ Each rule must use exactly one of `branch_scope` or `target_branch_scope`. ## Server enforcement Every mutating endpoint calls `authorize_request()` *before* the handler runs; decisions are logged with actor / action / branch / outcome / matched rule. + +## Actor identity (signed-claim-only) + +The actor identity used for every policy decision comes from the matched bearer token — never from a client-supplied request header, query parameter, or body field. The server resolves the token at the auth middleware boundary, looks up the actor it was minted for, and overwrites whatever the handler may have placed in the policy request. Clients cannot set `actor_id` directly. + +This is intentional. Trusting client-supplied identity for authorization is "asking the attacker if they're an admin" — Supabase's RLS history names the same footgun. The chokepoint lives in `authorize_request` in `crates/omnigraph-server/src/lib.rs` and is named in `docs/dev/invariants.md` Hard Invariant 11. A regression test asserts the contract: a request with `Authorization: Bearer ` plus `X-Actor-Id: actor-B` always evaluates as actor A, never as actor B. + +If you find yourself wanting to let clients override `actor_id` for impersonation, delegation, or service-account flows — that's a feature, but it needs explicit design (e.g., signed delegation claims, an `On-Behalf-Of` audit trail). It is not a convenience knob.