diff --git a/crates/forge_domain/src/policies/engine.rs b/crates/forge_domain/src/policies/engine.rs index b89747a906..c4ca9d204e 100644 --- a/crates/forge_domain/src/policies/engine.rs +++ b/crates/forge_domain/src/policies/engine.rs @@ -25,64 +25,30 @@ impl<'a> PolicyEngine<'a> { self.evaluate_policies(operation) } - /// Internal helper function to evaluate policies for a given operation - /// Returns permission result, defaults to Confirm if no policies match + /// Internal helper function to evaluate policies for a given operation. + /// Returns the permission of the first policy whose rule matches the + /// operation, or [`Permission::Confirm`] if no policy matches. fn evaluate_policies(&self, operation: &PermissionOperation) -> Permission { - let has_policies = !self.policies.policies.is_empty(); - - if !has_policies { - return Permission::Confirm; - } - - let mut last_allow: Option = None; - - // Evaluate all policies in order: workflow policies first, then extended - // policies - - if let Some(permission) = self.evaluate_policy_set(self.policies.policies.iter(), operation) - { - match permission { - Permission::Deny | Permission::Confirm => { - // Return immediately for denials or confirmations - return permission; - } - Permission::Allow => { - // Keep track of the last allow - last_allow = Some(permission); - } - } - } - - // Return last allow if found, otherwise default to Confirm - last_allow.unwrap_or(Permission::Confirm) - } - - /// Helper function to evaluate a set of policies - /// Returns the first non-Allow result, or the last Allow result if all are - /// Allow + self.evaluate_policy_set(self.policies.policies.iter(), operation) + .unwrap_or(Permission::Confirm) + } + + /// Finds the most-specific matching policy. Ties broken by restrictiveness + /// (Deny > Confirm > Allow). fn evaluate_policy_set<'p, I: IntoIterator>( &self, policies: I, operation: &PermissionOperation, ) -> Option { - let mut last_allow: Option = None; - - for policy in policies { - if let Some(permission) = policy.eval(operation) { - match permission { - Permission::Deny | Permission::Confirm => { - // Return immediately for denials or confirmations - return Some(permission); - } - Permission::Allow => { - // Keep track of the last allow - last_allow = Some(permission); - } - } - } - } - - last_allow + policies + .into_iter() + .filter_map(|policy| { + policy + .eval(operation) + .map(|permission| (permission, policy.specificity())) + }) + .max_by_key(|(permission, specificity)| (*specificity, permission.restrictiveness())) + .map(|(permission, _)| permission) } } @@ -187,6 +153,148 @@ mod tests { assert_eq!(actual, Permission::Allow); } + /// Regression: specific allow should win over broad deny (issue #3085). + #[test] + fn test_policy_engine_specific_allow_should_win_over_broad_deny() { + let fixture_workflow = PolicyConfig::new() + .add_policy(Policy::Simple { + permission: Permission::Allow, + rule: Rule::Read(ReadRule { read: "**/*".to_string(), dir: None }), + }) + .add_policy(Policy::Simple { + permission: Permission::Allow, + rule: Rule::Write(WriteRule { write: "**/*.rs".to_string(), dir: None }), + }) + .add_policy(Policy::Simple { + permission: Permission::Deny, + rule: Rule::Write(WriteRule { write: "**/*".to_string(), dir: None }), + }); + let fixture = PolicyEngine::new(&fixture_workflow); + let operation = PermissionOperation::Write { + path: std::path::PathBuf::from("test.rs"), + cwd: std::path::PathBuf::from("/test/cwd"), + message: "Create/overwrite file: test.rs".to_string(), + }; + + let actual = fixture.can_perform(&operation); + let expected = Permission::Allow; + + assert_eq!(actual, expected); + } + + /// Verifies order independence: deny first, allow second should yield same + /// result. + #[test] + fn test_policy_engine_specific_allow_wins_regardless_of_order() { + let fixture_workflow = PolicyConfig::new() + .add_policy(Policy::Simple { + permission: Permission::Deny, + rule: Rule::Write(WriteRule { write: "**/*".to_string(), dir: None }), + }) + .add_policy(Policy::Simple { + permission: Permission::Allow, + rule: Rule::Write(WriteRule { write: "**/*.rs".to_string(), dir: None }), + }); + let fixture = PolicyEngine::new(&fixture_workflow); + let rs_op = PermissionOperation::Write { + path: std::path::PathBuf::from("test.rs"), + cwd: std::path::PathBuf::from("/test/cwd"), + message: "Create/overwrite file: test.rs".to_string(), + }; + let py_op = PermissionOperation::Write { + path: std::path::PathBuf::from("test.py"), + cwd: std::path::PathBuf::from("/test/cwd"), + message: "Create/overwrite file: test.py".to_string(), + }; + + // .rs matches both rules; the more-specific allow wins. + assert_eq!(fixture.can_perform(&rs_op), Permission::Allow); + // .py only matches the broad deny, so it stays denied. + assert_eq!(fixture.can_perform(&py_op), Permission::Deny); + } + + /// Verifies carve-out exception: specific deny beats broad allow. + #[test] + fn test_policy_engine_specific_deny_carves_out_exception_in_broad_allow() { + let fixture_workflow = PolicyConfig::new() + .add_policy(Policy::Simple { + permission: Permission::Allow, + rule: Rule::Write(WriteRule { write: "**/*.rs".to_string(), dir: None }), + }) + .add_policy(Policy::Simple { + permission: Permission::Deny, + rule: Rule::Write(WriteRule { write: "secret.rs".to_string(), dir: None }), + }); + let fixture = PolicyEngine::new(&fixture_workflow); + let secret_op = PermissionOperation::Write { + path: std::path::PathBuf::from("secret.rs"), + cwd: std::path::PathBuf::from("/test/cwd"), + message: "Create/overwrite file: secret.rs".to_string(), + }; + let other_op = PermissionOperation::Write { + path: std::path::PathBuf::from("main.rs"), + cwd: std::path::PathBuf::from("/test/cwd"), + message: "Create/overwrite file: main.rs".to_string(), + }; + + assert_eq!(fixture.can_perform(&secret_op), Permission::Deny); + assert_eq!(fixture.can_perform(&other_op), Permission::Allow); + } + + /// Tie-breaker: Deny wins when specificity is equal. + #[test] + fn test_policy_engine_equal_specificity_prefers_deny() { + let fixture_workflow = PolicyConfig::new() + .add_policy(Policy::Simple { + permission: Permission::Allow, + rule: Rule::Write(WriteRule { write: "secret.rs".to_string(), dir: None }), + }) + .add_policy(Policy::Simple { + permission: Permission::Deny, + rule: Rule::Write(WriteRule { write: "secret.rs".to_string(), dir: None }), + }); + let fixture = PolicyEngine::new(&fixture_workflow); + let operation = PermissionOperation::Write { + path: std::path::PathBuf::from("secret.rs"), + cwd: std::path::PathBuf::from("/test/cwd"), + message: "Create/overwrite file: secret.rs".to_string(), + }; + + let actual = fixture.can_perform(&operation); + let expected = Permission::Deny; + + assert_eq!(actual, expected); + } + + #[test] + fn test_policy_engine_path_prefix_specificity() { + let fixture_workflow = PolicyConfig::new() + .add_policy(Policy::Simple { + permission: Permission::Allow, + rule: Rule::Write(WriteRule { write: "src/**/*.rs".to_string(), dir: None }), + }) + .add_policy(Policy::Simple { + permission: Permission::Deny, + rule: Rule::Write(WriteRule { write: "*.rs".to_string(), dir: None }), + }); + let fixture = PolicyEngine::new(&fixture_workflow); + let inside_src = PermissionOperation::Write { + path: std::path::PathBuf::from("src/utils/helper.rs"), + cwd: std::path::PathBuf::from("/test/cwd"), + message: "Create/overwrite file: src/utils/helper.rs".to_string(), + }; + let outside_src = PermissionOperation::Write { + path: std::path::PathBuf::from("test.rs"), + cwd: std::path::PathBuf::from("/test/cwd"), + message: "Create/overwrite file: test.rs".to_string(), + }; + + // Path "narrowness" (src/ prefix) wins because it has more literals. + assert_eq!(fixture.can_perform(&inside_src), Permission::Allow); + // File outside src/ only matches the broad deny. + assert_eq!(fixture.can_perform(&outside_src), Permission::Deny); + } + #[test] fn test_policy_engine_can_perform_net_fetch() { let fixture_workflow = fixture_workflow_with_net_fetch_policy(); diff --git a/crates/forge_domain/src/policies/policy.rs b/crates/forge_domain/src/policies/policy.rs index 36ac5a344f..0516213cde 100644 --- a/crates/forge_domain/src/policies/policy.rs +++ b/crates/forge_domain/src/policies/policy.rs @@ -111,6 +111,16 @@ impl Policy { _ => None, } } + + /// Returns the max specificity across child rules. + pub fn specificity(&self) -> usize { + match self { + Policy::Simple { permission: _, rule } => rule.specificity(), + Policy::All { all } => all.iter().map(Policy::specificity).max().unwrap_or(0), + Policy::Any { any } => any.iter().map(Policy::specificity).max().unwrap_or(0), + Policy::Not { not } => not.specificity(), + } + } } impl Display for Policy { diff --git a/crates/forge_domain/src/policies/rule.rs b/crates/forge_domain/src/policies/rule.rs index 652dbab8a9..3550aa501f 100644 --- a/crates/forge_domain/src/policies/rule.rs +++ b/crates/forge_domain/src/policies/rule.rs @@ -97,6 +97,22 @@ impl Rule { _ => false, } } + + /// Returns a specificity score (count of literal chars) for tie-breaking. + pub fn specificity(&self) -> usize { + fn count_literals(pattern: &str) -> usize { + pattern.chars().filter(|c| !matches!(c, '*' | '?')).count() + } + + let (primary, dir) = match self { + Rule::Write(r) => (r.write.as_str(), r.dir.as_deref()), + Rule::Read(r) => (r.read.as_str(), r.dir.as_deref()), + Rule::Execute(r) => (r.command.as_str(), r.dir.as_deref()), + Rule::Fetch(r) => (r.url.as_str(), r.dir.as_deref()), + }; + + count_literals(primary) + dir.map_or(0, count_literals) + } } /// Helper function to match a glob pattern against a path or string diff --git a/crates/forge_domain/src/policies/types.rs b/crates/forge_domain/src/policies/types.rs index 00433f5f7f..748f01599d 100644 --- a/crates/forge_domain/src/policies/types.rs +++ b/crates/forge_domain/src/policies/types.rs @@ -15,6 +15,17 @@ pub enum Permission { Confirm, } +impl Permission { + /// Tie-breaker score: Deny > Confirm > Allow. + pub(crate) fn restrictiveness(&self) -> u8 { + match self { + Permission::Deny => 2, + Permission::Confirm => 1, + Permission::Allow => 0, + } + } +} + impl Display for Permission { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { match self {