From 76ee061cac8b3d12157558f82076415abfe5a281 Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Wed, 27 May 2026 12:00:52 +0200 Subject: [PATCH] mr-668: drop actor_id from PolicyRequest; pass actor as separate arg MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The MR-731 "server-authoritative actor identity" invariant was enforced by an in-function chokepoint (`request.actor_id = actor.actor_id...` overwrite inside `authorize_request`). That worked but relied on every caller passing in a `PolicyRequest` and trusting the overwrite — a comment-enforced invariant. Move the invariant into the type system: * `PolicyRequest` no longer carries `actor_id`. The struct now models what a caller wants to do, not who they are. * `PolicyEngine::authorize(actor_id: &str, request: &PolicyRequest)` and `validate_request(actor_id, request)` take identity as a separate argument. The same shape `PolicyChecker::check` already had for the engine layer. * `authorize_request` in the HTTP layer extracts `actor_id` from the bearer-resolved `ResolvedActor` and passes it positionally — no overwrite step that could be skipped. * CLI `omnigraph policy explain` updated (the only other consumer that built a `PolicyRequest`). Public API break for the `omnigraph-policy` crate. Worth it: handlers can no longer accidentally populate `actor_id` from a request body field, and external consumers are forced by the compiler to source actor identity from a trusted path. The MR-731 chokepoint test `actor_id_resolves_from_bearer_token_ignoring_client_supplied_headers` still passes — the bearer-resolved actor is what reaches the engine. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/omnigraph-cli/src/main.rs | 9 +- crates/omnigraph-policy/src/lib.rs | 173 +++++++++++++++++------------ crates/omnigraph-server/src/lib.rs | 79 +++---------- 3 files changed, 121 insertions(+), 140 deletions(-) diff --git a/crates/omnigraph-cli/src/main.rs b/crates/omnigraph-cli/src/main.rs index d0324ed..29f6f97 100644 --- a/crates/omnigraph-cli/src/main.rs +++ b/crates/omnigraph-cli/src/main.rs @@ -1333,12 +1333,12 @@ fn print_commit_human(commit: &CommitOutput) { println!("created_at: {}", commit.created_at); } -fn print_policy_explain(decision: &PolicyDecision, request: &PolicyRequest) { +fn print_policy_explain(decision: &PolicyDecision, actor_id: &str, request: &PolicyRequest) { println!( "decision: {}", if decision.allowed { "allow" } else { "deny" } ); - println!("actor: {}", request.actor_id); + println!("actor: {}", actor_id); println!("action: {}", request.action); if let Some(branch) = &request.branch { println!("branch: {}", branch); @@ -2471,13 +2471,12 @@ async fn main() -> Result<()> { let config = load_cli_config(config.as_ref())?; let engine = resolve_policy_engine(&config)?; let request = PolicyRequest { - actor_id: actor, action, branch, target_branch, }; - let decision = engine.authorize(&request)?; - print_policy_explain(&decision, &request); + let decision = engine.authorize(&actor, &request)?; + print_policy_explain(&decision, &actor, &request); } }, Command::Optimize { diff --git a/crates/omnigraph-policy/src/lib.rs b/crates/omnigraph-policy/src/lib.rs index 79563c7..a98830a 100644 --- a/crates/omnigraph-policy/src/lib.rs +++ b/crates/omnigraph-policy/src/lib.rs @@ -202,9 +202,16 @@ pub enum PolicyExpectation { Deny, } +/// What a caller wants to do, sans identity. Actor identity flows +/// through a separate `actor_id: &str` parameter on +/// [`PolicyEngine::authorize`] / [`PolicyChecker::check`] — encoding +/// the architectural invariant that actor identity is server-authoritative +/// and must not be supplied by the same code path that supplies the +/// requested action. In the HTTP layer, the bearer-token middleware +/// resolves the actor and passes it independently; clients cannot +/// smuggle identity inside this struct. #[derive(Debug, Clone)] pub struct PolicyRequest { - pub actor_id: String, pub action: PolicyAction, pub branch: Option, pub target_branch: Option, @@ -413,22 +420,28 @@ impl PolicyEngine { PolicyCompiler::compile(&config, graph_id) } - pub fn authorize(&self, request: &PolicyRequest) -> Result { - if !self.known_actors.contains(request.actor_id.as_str()) { + /// Evaluate a request. `actor_id` is supplied as a separate + /// argument (not inside `PolicyRequest`) so the type system enforces + /// the "server-authoritative actor identity" invariant — clients + /// supplying a `PolicyRequest` cannot smuggle identity through the + /// same struct that carries the requested action. + pub fn authorize(&self, actor_id: &str, request: &PolicyRequest) -> Result { + if !self.known_actors.contains(actor_id) { return Ok(self.deny( + actor_id, request, None, format!( "policy denied action '{}' for unknown actor '{}'", - request.action, request.actor_id + request.action, actor_id ), )); } - let principal = entity_uid("Actor", &request.actor_id)?; + let principal = entity_uid("Actor", actor_id)?; let action = entity_uid("Action", request.action.as_str())?; - // MR-668 PR 6a: pick the resource entity based on the action's - // `resource_kind`. Server-scoped actions (`graph_list`) bind to + // Pick the resource entity based on the action's `resource_kind`. + // Server-scoped actions (`graph_list`) bind to // `Omnigraph::Server::"root"`; per-graph actions bind to // `Omnigraph::Graph::""`. let resource = match request.action.resource_kind() { @@ -471,7 +484,7 @@ impl PolicyEngine { matched_rule_id: matched_rule_id.clone(), message: format!( "policy allowed action '{}' for actor '{}'", - request.action, request.actor_id + request.action, actor_id ), }, Decision::Deny => { @@ -488,15 +501,15 @@ impl PolicyEngine { .as_deref() .map(|branch| format!(" targeting branch '{}'", branch)) .unwrap_or_default(), - request.actor_id + actor_id ); - self.deny(request, matched_rule_id, message) + self.deny(actor_id, request, matched_rule_id, message) } }) } - pub fn validate_request(&self, request: &PolicyRequest) -> Result<()> { - let _ = self.authorize(request)?; + pub fn validate_request(&self, actor_id: &str, request: &PolicyRequest) -> Result<()> { + let _ = self.authorize(actor_id, request)?; Ok(()) } @@ -506,12 +519,14 @@ impl PolicyEngine { } let mut failures = Vec::new(); for case in &tests.cases { - let decision = self.authorize(&PolicyRequest { - actor_id: case.actor.clone(), - action: case.action, - branch: case.branch.clone(), - target_branch: case.target_branch.clone(), - })?; + let decision = self.authorize( + &case.actor, + &PolicyRequest { + action: case.action, + branch: case.branch.clone(), + target_branch: case.target_branch.clone(), + }, + )?; let expected_allowed = matches!(case.expect, PolicyExpectation::Allow); if decision.allowed != expected_allowed { failures.push(format!( @@ -535,6 +550,7 @@ impl PolicyEngine { fn deny( &self, + _actor_id: &str, _request: &PolicyRequest, matched_rule_id: Option, message: String, @@ -750,10 +766,6 @@ fn cedar_literal(value: &str) -> String { } impl PolicyRequest { - pub fn actor_id(&self) -> &str { - &self.actor_id - } - pub fn action(&self) -> PolicyAction { self.action } @@ -892,13 +904,12 @@ impl PolicyChecker for PolicyEngine { ) -> Result<(), PolicyError> { let (branch, target_branch) = scope.to_branch_pair(); let request = PolicyRequest { - actor_id: actor.to_string(), action, branch: branch.map(|s| s.to_string()), target_branch: target_branch.map(|s| s.to_string()), }; let decision = self - .authorize(&request) + .authorize(actor, &request) .map_err(|e| PolicyError::Internal(e.to_string()))?; if decision.allowed { Ok(()) @@ -1014,33 +1025,39 @@ rules: let engine = PolicyCompiler::compile(&policy, "graph").unwrap(); let allow = engine - .authorize(&PolicyRequest { - actor_id: "act-bruno".to_string(), - action: PolicyAction::Change, - branch: Some("feature".to_string()), - target_branch: None, - }) + .authorize( + "act-bruno", + &PolicyRequest { + action: PolicyAction::Change, + branch: Some("feature".to_string()), + target_branch: None, + }, + ) .unwrap(); assert!(allow.allowed); assert_eq!(allow.matched_rule_id.as_deref(), Some("team-write")); let deny = engine - .authorize(&PolicyRequest { - actor_id: "act-bruno".to_string(), - action: PolicyAction::BranchDelete, - branch: None, - target_branch: Some("main".to_string()), - }) + .authorize( + "act-bruno", + &PolicyRequest { + action: PolicyAction::BranchDelete, + branch: None, + target_branch: Some("main".to_string()), + }, + ) .unwrap(); assert!(!deny.allowed); let admin = engine - .authorize(&PolicyRequest { - actor_id: "act-andrew".to_string(), - action: PolicyAction::BranchDelete, - branch: None, - target_branch: Some("main".to_string()), - }) + .authorize( + "act-andrew", + &PolicyRequest { + action: PolicyAction::BranchDelete, + branch: None, + target_branch: Some("main".to_string()), + }, + ) .unwrap(); assert!(admin.allowed); assert_eq!(admin.matched_rule_id.as_deref(), Some("admins-promote")); @@ -1109,27 +1126,31 @@ rules: let engine = PolicyCompiler::compile(&policy, "graph").unwrap(); let allow = engine - .authorize(&PolicyRequest { - actor_id: "act-ragnor".to_string(), - action: PolicyAction::SchemaApply, - branch: None, - target_branch: Some("main".to_string()), - }) + .authorize( + "act-ragnor", + &PolicyRequest { + action: PolicyAction::SchemaApply, + branch: None, + target_branch: Some("main".to_string()), + }, + ) .unwrap(); assert!(allow.allowed); let deny = engine - .authorize(&PolicyRequest { - actor_id: "act-ragnor".to_string(), - action: PolicyAction::SchemaApply, - branch: None, - target_branch: Some("feature".to_string()), - }) + .authorize( + "act-ragnor", + &PolicyRequest { + action: PolicyAction::SchemaApply, + branch: None, + target_branch: Some("feature".to_string()), + }, + ) .unwrap(); assert!(!deny.allowed); } - // ─── MR-668 PR 6a — server-scoped action (graph_list) ─ + // ─── MR-668 — server-scoped action (graph_list) ─ #[test] fn graph_list_action_authorizes_against_server_resource() { @@ -1155,24 +1176,28 @@ rules: let engine = PolicyCompiler::compile(&policy, "ignored").unwrap(); let allow = engine - .authorize(&PolicyRequest { - actor_id: "act-andrew".to_string(), - action: PolicyAction::GraphList, - branch: None, - target_branch: None, - }) + .authorize( + "act-andrew", + &PolicyRequest { + action: PolicyAction::GraphList, + branch: None, + target_branch: None, + }, + ) .unwrap(); assert!(allow.allowed); assert_eq!(allow.matched_rule_id.as_deref(), Some("admins-list-graphs")); // Different actor, same policy → deny. let deny = engine - .authorize(&PolicyRequest { - actor_id: "act-bruno".to_string(), - action: PolicyAction::GraphList, - branch: None, - target_branch: None, - }) + .authorize( + "act-bruno", + &PolicyRequest { + action: PolicyAction::GraphList, + branch: None, + target_branch: None, + }, + ) .unwrap(); assert!(!deny.allowed); } @@ -1251,12 +1276,14 @@ rules: .unwrap(); let engine = PolicyCompiler::compile(&policy, "graph").unwrap(); let allow = engine - .authorize(&PolicyRequest { - actor_id: "act-andrew".to_string(), - action: PolicyAction::Read, - branch: Some("main".to_string()), - target_branch: None, - }) + .authorize( + "act-andrew", + &PolicyRequest { + action: PolicyAction::Read, + branch: Some("main".to_string()), + target_branch: None, + }, + ) .unwrap(); assert!(allow.allowed); assert_eq!(allow.matched_rule_id.as_deref(), Some("team-read")); diff --git a/crates/omnigraph-server/src/lib.rs b/crates/omnigraph-server/src/lib.rs index e49555b..f8deb0c 100644 --- a/crates/omnigraph-server/src/lib.rs +++ b/crates/omnigraph-server/src/lib.rs @@ -1176,10 +1176,6 @@ async fn server_graphs_list( actor.as_ref().map(|Extension(actor)| actor), state.server_policy.as_deref(), PolicyRequest { - actor_id: actor - .as_ref() - .map(|Extension(actor)| actor.actor_id.as_ref().to_string()) - .unwrap_or_default(), action: PolicyAction::GraphList, branch: None, target_branch: None, @@ -1422,14 +1418,15 @@ fn log_policy_decision(actor_id: &str, request: &PolicyRequest, decision: &Polic /// are deferred until a managed cluster catalog lands). /// /// The MR-731 invariant lives inside this function: actor identity is -/// overwritten from the resolved bearer match, never trusted from the -/// caller-built `PolicyRequest.actor_id`. See -/// `actor_id_resolves_from_bearer_token_ignoring_client_supplied_headers` -/// at `tests/server.rs:1114-1216`. +/// supplied as a separate argument from the resolved bearer match. The +/// `PolicyRequest` struct itself does not carry identity (the field was +/// dropped from the type), so handlers cannot smuggle it through the +/// request. See `actor_id_resolves_from_bearer_token_ignoring_client_supplied_headers` +/// at `tests/server.rs`. fn authorize_request( actor: Option<&ResolvedActor>, policy: Option<&PolicyEngine>, - mut request: PolicyRequest, + request: PolicyRequest, ) -> std::result::Result<(), ApiError> { let Some(engine) = policy else { // MR-723 default-deny path. We're here when no PolicyEngine is @@ -1458,26 +1455,22 @@ fn authorize_request( let Some(actor) = actor else { return Err(ApiError::unauthorized("missing bearer token")); }; - // 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 + // SECURITY INVARIANT (MR-731): actor identity is supplied to the + // policy engine here as a separate argument, sourced from the + // bearer-token match resolved by `require_bearer_auth`. The + // `PolicyRequest` struct itself no longer carries `actor_id` (it + // was dropped from the type), so handlers cannot smuggle identity + // through the request body and there is no overwrite step that + // could be skipped. 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.actor_id.as_ref().to_string(); + let actor_id = actor.actor_id.as_ref(); let decision = engine - .authorize(&request) + .authorize(actor_id, &request) .map_err(|err| ApiError::internal(format!("policy: {err}")))?; - log_policy_decision(actor.actor_id.as_ref(), &request, &decision); + log_policy_decision(actor_id, &request, &decision); if decision.allowed { Ok(()) } else { @@ -1514,10 +1507,6 @@ async fn server_snapshot( actor.as_ref().map(|Extension(actor)| actor), handle.policy.as_deref(), PolicyRequest { - actor_id: actor - .as_ref() - .map(|Extension(actor)| actor.actor_id.as_ref().to_string()) - .unwrap_or_default(), action: PolicyAction::Read, branch: Some(branch.clone()), target_branch: None, @@ -1581,10 +1570,6 @@ async fn server_read( actor.as_ref().map(|Extension(actor)| actor), handle.policy.as_deref(), PolicyRequest { - actor_id: actor - .as_ref() - .map(|Extension(actor)| actor.actor_id.as_ref().to_string()) - .unwrap_or_default(), action: PolicyAction::Read, branch: policy_branch, target_branch: None, @@ -1641,10 +1626,6 @@ async fn server_export( actor.as_ref().map(|Extension(actor)| actor), handle.policy.as_deref(), PolicyRequest { - actor_id: actor - .as_ref() - .map(|Extension(actor)| actor.actor_id.as_ref().to_string()) - .unwrap_or_default(), action: PolicyAction::Export, branch: Some(branch.clone()), target_branch: None, @@ -1714,7 +1695,6 @@ async fn server_change( actor.as_ref().map(|Extension(actor)| actor), handle.policy.as_deref(), PolicyRequest { - actor_id: actor_id.map(str::to_string).unwrap_or_default(), action: PolicyAction::Change, branch: Some(branch.clone()), target_branch: None, @@ -1787,10 +1767,6 @@ async fn server_schema_get( actor.as_ref().map(|Extension(actor)| actor), handle.policy.as_deref(), PolicyRequest { - actor_id: actor - .as_ref() - .map(|Extension(actor)| actor.actor_id.as_ref().to_string()) - .unwrap_or_default(), action: PolicyAction::Read, branch: None, target_branch: None, @@ -1839,7 +1815,6 @@ async fn server_schema_apply( actor.as_ref().map(|Extension(actor)| actor), handle.policy.as_deref(), PolicyRequest { - actor_id: actor_id.map(str::to_string).unwrap_or_default(), action: PolicyAction::SchemaApply, branch: None, target_branch: Some("main".to_string()), @@ -1922,7 +1897,6 @@ async fn server_ingest( actor.as_ref().map(|Extension(actor)| actor), handle.policy.as_deref(), PolicyRequest { - actor_id: actor_id.map(str::to_string).unwrap_or_default(), action: PolicyAction::BranchCreate, branch: Some(from.clone()), target_branch: Some(branch.clone()), @@ -1933,7 +1907,6 @@ async fn server_ingest( actor.as_ref().map(|Extension(actor)| actor), handle.policy.as_deref(), PolicyRequest { - actor_id: actor_id.map(str::to_string).unwrap_or_default(), action: PolicyAction::Change, branch: Some(branch.clone()), target_branch: None, @@ -1983,10 +1956,6 @@ async fn server_branch_list( actor.as_ref().map(|Extension(actor)| actor), handle.policy.as_deref(), PolicyRequest { - actor_id: actor - .as_ref() - .map(|Extension(actor)| actor.actor_id.as_ref().to_string()) - .unwrap_or_default(), action: PolicyAction::Read, branch: None, target_branch: None, @@ -2036,10 +2005,6 @@ async fn server_branch_create( actor.as_ref().map(|Extension(actor)| actor), handle.policy.as_deref(), PolicyRequest { - actor_id: actor - .as_ref() - .map(|Extension(actor)| actor.actor_id.as_ref().to_string()) - .unwrap_or_default(), action: PolicyAction::BranchCreate, branch: Some(from.clone()), target_branch: Some(request.name.clone()), @@ -2107,7 +2072,6 @@ async fn server_branch_delete( actor.as_ref().map(|Extension(actor)| actor), handle.policy.as_deref(), PolicyRequest { - actor_id: actor_id.map(str::to_string).unwrap_or_default(), action: PolicyAction::BranchDelete, branch: None, target_branch: Some(branch.clone()), @@ -2169,7 +2133,6 @@ async fn server_branch_merge( actor.as_ref().map(|Extension(actor)| actor), handle.policy.as_deref(), PolicyRequest { - actor_id: actor_id.map(str::to_string).unwrap_or_default(), action: PolicyAction::BranchMerge, branch: Some(request.source.clone()), target_branch: Some(target.clone()), @@ -2223,10 +2186,6 @@ async fn server_commit_list( actor.as_ref().map(|Extension(actor)| actor), handle.policy.as_deref(), PolicyRequest { - actor_id: actor - .as_ref() - .map(|Extension(actor)| actor.actor_id.as_ref().to_string()) - .unwrap_or_default(), action: PolicyAction::Read, branch: query.branch.clone(), target_branch: None, @@ -2273,10 +2232,6 @@ async fn server_commit_show( actor.as_ref().map(|Extension(actor)| actor), handle.policy.as_deref(), PolicyRequest { - actor_id: actor - .as_ref() - .map(|Extension(actor)| actor.actor_id.as_ref().to_string()) - .unwrap_or_default(), action: PolicyAction::Read, branch: None, target_branch: None,