From f0ccf9fcb4b7a043ad069209f42c12d90d5190e6 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 5 Jan 2026 10:44:29 -0800 Subject: [PATCH] feat: add forbidden_reason arg to prefix_rule() in *.rules --- codex-rs/cli/tests/execpolicy.rs | 58 ++++++++++ codex-rs/core/src/exec_policy.rs | 106 ++++++++++++++++-- codex-rs/core/tests/suite/exec_policy.rs | 2 +- codex-rs/execpolicy/README.md | 7 +- .../execpolicy/examples/example.codexpolicy | 1 + codex-rs/execpolicy/src/error.rs | 2 + codex-rs/execpolicy/src/parser.rs | 19 ++++ codex-rs/execpolicy/src/policy.rs | 1 + codex-rs/execpolicy/src/rule.rs | 7 ++ codex-rs/execpolicy/tests/basic.rs | 73 +++++++++++- 10 files changed, 265 insertions(+), 11 deletions(-) diff --git a/codex-rs/cli/tests/execpolicy.rs b/codex-rs/cli/tests/execpolicy.rs index 7d8a2b1c45..13c614be1b 100644 --- a/codex-rs/cli/tests/execpolicy.rs +++ b/codex-rs/cli/tests/execpolicy.rs @@ -59,3 +59,61 @@ prefix_rule( Ok(()) } + +#[test] +fn execpolicy_check_includes_forbidden_reason_when_present() +-> Result<(), Box> { + let codex_home = TempDir::new()?; + let policy_path = codex_home.path().join("rules").join("policy.rules"); + fs::create_dir_all( + policy_path + .parent() + .expect("policy path should have a parent"), + )?; + fs::write( + &policy_path, + r#" +prefix_rule( + pattern = ["git", "push"], + decision = "forbidden", + forbidden_reason = "pushing is blocked in this repo", +) +"#, + )?; + + let output = Command::new(codex_utils_cargo_bin::cargo_bin("codex")?) + .env("CODEX_HOME", codex_home.path()) + .args([ + "execpolicy", + "check", + "--rules", + policy_path + .to_str() + .expect("policy path should be valid UTF-8"), + "git", + "push", + "origin", + "main", + ]) + .output()?; + + assert!(output.status.success()); + let result: serde_json::Value = serde_json::from_slice(&output.stdout)?; + assert_eq!( + result, + json!({ + "decision": "forbidden", + "matchedRules": [ + { + "prefixRuleMatch": { + "matchedPrefix": ["git", "push"], + "decision": "forbidden", + "forbiddenReason": "pushing is blocked in this repo" + } + } + ] + }) + ); + + Ok(()) +} diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index 5caf7d9b82..234d89fef8 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -28,8 +28,8 @@ use crate::features::Feature; use crate::features::Features; use crate::sandboxing::SandboxPermissions; use crate::tools::sandboxing::ExecApprovalRequirement; +use shlex::try_join as shlex_try_join; -const FORBIDDEN_REASON: &str = "execpolicy forbids this command"; const PROMPT_CONFLICT_REASON: &str = "execpolicy requires approval for this command, but AskForApproval is set to Never"; const PROMPT_REASON: &str = "execpolicy requires approval for this command"; @@ -128,7 +128,7 @@ impl ExecPolicyManager { match evaluation.decision { Decision::Forbidden => ExecApprovalRequirement::Forbidden { - reason: FORBIDDEN_REASON.to_string(), + reason: derive_forbidden_reason(command, &evaluation), }, Decision::Prompt => { if matches!(approval_policy, AskForApproval::Never) { @@ -310,6 +310,53 @@ fn derive_prompt_reason(evaluation: &Evaluation) -> Option { }) } +fn render_shlex_command(args: &[String]) -> String { + shlex_try_join(args.iter().map(String::as_str)).unwrap_or_else(|_| args.join(" ")) +} + +fn derive_forbidden_reason(command_args: &[String], evaluation: &Evaluation) -> String { + let command = render_shlex_command(command_args); + + let forbidden_reason = evaluation + .matched_rules + .iter() + .filter_map(|rule_match| match rule_match { + RuleMatch::PrefixRuleMatch { + matched_prefix, + decision: Decision::Forbidden, + forbidden_reason: Some(reason), + } => Some((matched_prefix.len(), reason.as_str())), + _ => None, + }) + .max_by_key(|(matched_prefix_len, _)| *matched_prefix_len) + .map(|(_, reason)| reason); + + if let Some(forbidden_reason) = forbidden_reason { + return format!("{command} rejected: {forbidden_reason}"); + } + + let matched_prefix = evaluation + .matched_rules + .iter() + .filter_map(|rule_match| match rule_match { + RuleMatch::PrefixRuleMatch { + matched_prefix, + decision: Decision::Forbidden, + .. + } => Some(matched_prefix), + _ => None, + }) + .max_by_key(|matched_prefix| matched_prefix.len()); + + match matched_prefix { + Some(prefix) => { + let prefix = render_shlex_command(prefix); + format!("{command} rejected: policy forbids commands starting with `{prefix}`") + } + None => format!("{command} rejected: blocked by policy"), + } +} + async fn collect_policy_files(dir: impl AsRef) -> Result, ExecPolicyError> { let dir = dir.as_ref(); let mut read_dir = match fs::read_dir(dir).await { @@ -450,7 +497,8 @@ mod tests { decision: Decision::Forbidden, matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: vec!["rm".to_string()], - decision: Decision::Forbidden + decision: Decision::Forbidden, + forbidden_reason: None, }], }, policy.check_multiple(command.iter(), &|_| Decision::Allow) @@ -528,7 +576,8 @@ mod tests { decision: Decision::Forbidden, matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: vec!["rm".to_string()], - decision: Decision::Forbidden + decision: Decision::Forbidden, + forbidden_reason: None, }], }, policy.check_multiple([vec!["rm".to_string()]].iter(), &|_| Decision::Allow) @@ -538,7 +587,8 @@ mod tests { decision: Decision::Prompt, matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: vec!["ls".to_string()], - decision: Decision::Prompt + decision: Decision::Prompt, + forbidden_reason: None, }], }, policy.check_multiple([vec!["ls".to_string()]].iter(), &|_| Decision::Allow) @@ -560,7 +610,7 @@ prefix_rule(pattern=["rm"], decision="forbidden") let forbidden_script = vec![ "bash".to_string(), "-lc".to_string(), - "rm -rf /tmp".to_string(), + "rm -rf /some/important/folder".to_string(), ]; let manager = ExecPolicyManager::new(policy); @@ -574,10 +624,52 @@ prefix_rule(pattern=["rm"], decision="forbidden") ) .await; + let forbidden_script_rendered = + shlex::try_join(forbidden_script.iter().map(String::as_str)).expect("shlex join"); assert_eq!( requirement, ExecApprovalRequirement::Forbidden { - reason: FORBIDDEN_REASON.to_string() + reason: format!( + "{forbidden_script_rendered} rejected: policy forbids commands starting with `rm`" + ) + } + ); + } + + #[tokio::test] + async fn forbidden_reason_is_included_in_forbidden_exec_approval_requirement() { + let policy_src = r#" +prefix_rule( + pattern=["rm"], + decision="forbidden", + forbidden_reason="destructive command", +) +"#; + let mut parser = PolicyParser::new(); + parser + .parse("test.rules", policy_src) + .expect("parse policy"); + let policy = Arc::new(parser.build()); + + let manager = ExecPolicyManager::new(policy); + let requirement = manager + .create_exec_approval_requirement_for_command( + &Features::with_defaults(), + &[ + "rm".to_string(), + "-rf".to_string(), + "/some/important/folder".to_string(), + ], + AskForApproval::OnRequest, + &SandboxPolicy::DangerFullAccess, + SandboxPermissions::UseDefault, + ) + .await; + + assert_eq!( + requirement, + ExecApprovalRequirement::Forbidden { + reason: "rm -rf /some/important/folder rejected: destructive command".to_string() } ); } diff --git a/codex-rs/core/tests/suite/exec_policy.rs b/codex-rs/core/tests/suite/exec_policy.rs index 470478ad75..bde1e9ca4e 100644 --- a/codex-rs/core/tests/suite/exec_policy.rs +++ b/codex-rs/core/tests/suite/exec_policy.rs @@ -97,7 +97,7 @@ async fn execpolicy_blocks_shell_invocation() -> Result<()> { assert!( end.aggregated_output - .contains("execpolicy forbids this command"), + .contains("rejected: policy forbids commands starting with `echo`"), "unexpected output: {}", end.aggregated_output ); diff --git a/codex-rs/execpolicy/README.md b/codex-rs/execpolicy/README.md index 288a46dcbc..3e527e08b8 100644 --- a/codex-rs/execpolicy/README.md +++ b/codex-rs/execpolicy/README.md @@ -1,9 +1,10 @@ # codex-execpolicy ## Overview -- Policy engine and CLI built around `prefix_rule(pattern=[...], decision?, match?, not_match?)`. +- Policy engine and CLI built around `prefix_rule(pattern=[...], decision?, forbidden_reason?, match?, not_match?)`. - This release covers the prefix-rule subset of the execpolicy language; a richer language will follow. - Tokens are matched in order; any `pattern` element may be a list to denote alternatives. `decision` defaults to `allow`; valid values: `allow`, `prompt`, `forbidden`. +- When `decision = "forbidden"`, an optional `forbidden_reason` can be provided to explain why the command is blocked. - `match` / `not_match` supply example invocations that are validated at load time (think of them as unit tests); examples can be token arrays or strings (strings are tokenized with `shlex`). - The CLI always prints the JSON serialization of the evaluation result. - The legacy rule matcher lives in `codex-execpolicy-legacy`. @@ -14,6 +15,7 @@ prefix_rule( pattern = ["cmd", ["alt1", "alt2"]], # ordered tokens; list entries denote alternatives decision = "prompt", # allow | prompt | forbidden; defaults to allow + forbidden_reason = "explain why this is forbidden", # only valid when decision="forbidden" match = [["cmd", "alt1"], "cmd alt2"], # examples that must match this rule not_match = [["cmd", "oops"], "cmd alt3"], # examples that must not match this rule ) @@ -40,7 +42,8 @@ cargo run -p codex-execpolicy -- check --rules path/to/policy.rules git status { "prefixRuleMatch": { "matchedPrefix": ["", "..."], - "decision": "allow|prompt|forbidden" + "decision": "allow|prompt|forbidden", + "forbiddenReason": "..." } } ], diff --git a/codex-rs/execpolicy/examples/example.codexpolicy b/codex-rs/execpolicy/examples/example.codexpolicy index 5bb691b6f7..1469beacae 100644 --- a/codex-rs/execpolicy/examples/example.codexpolicy +++ b/codex-rs/execpolicy/examples/example.codexpolicy @@ -4,6 +4,7 @@ prefix_rule( pattern = ["git", "reset", "--hard"], decision = "forbidden", + forbidden_reason = "destructive operation", match = [ ["git", "reset", "--hard"], ], diff --git a/codex-rs/execpolicy/src/error.rs b/codex-rs/execpolicy/src/error.rs index 2f168a027e..9664e71a5c 100644 --- a/codex-rs/execpolicy/src/error.rs +++ b/codex-rs/execpolicy/src/error.rs @@ -11,6 +11,8 @@ pub enum Error { InvalidPattern(String), #[error("invalid example: {0}")] InvalidExample(String), + #[error("invalid rule: {0}")] + InvalidRule(String), #[error( "expected every example to match at least one rule. rules: {rules:?}; unmatched examples: \ {examples:?}" diff --git a/codex-rs/execpolicy/src/parser.rs b/codex-rs/execpolicy/src/parser.rs index d505490554..92b3a16a99 100644 --- a/codex-rs/execpolicy/src/parser.rs +++ b/codex-rs/execpolicy/src/parser.rs @@ -212,6 +212,7 @@ fn policy_builtins(builder: &mut GlobalsBuilder) { decision: Option<&'v str>, r#match: Option>>, not_match: Option>>, + forbidden_reason: Option<&'v str>, eval: &mut Evaluator<'v, '_, '_>, ) -> anyhow::Result { let decision = match decision { @@ -219,6 +220,23 @@ fn policy_builtins(builder: &mut GlobalsBuilder) { None => Decision::Allow, }; + let forbidden_reason = match forbidden_reason { + Some(raw) if raw.trim().is_empty() => { + return Err( + Error::InvalidRule("forbidden_reason cannot be empty".to_string()).into(), + ); + } + Some(raw) => Some(raw.to_string()), + None => None, + }; + + if forbidden_reason.is_some() && decision != Decision::Forbidden { + return Err(Error::InvalidRule(format!( + "forbidden_reason requires decision=\"forbidden\" (got {decision:?})" + )) + .into()); + } + let pattern_tokens = parse_pattern(pattern)?; let matches: Vec> = @@ -246,6 +264,7 @@ fn policy_builtins(builder: &mut GlobalsBuilder) { rest: rest.clone(), }, decision, + forbidden_reason: forbidden_reason.clone(), }) as RuleRef }) .collect(); diff --git a/codex-rs/execpolicy/src/policy.rs b/codex-rs/execpolicy/src/policy.rs index 991e904ae9..fa438f0815 100644 --- a/codex-rs/execpolicy/src/policy.rs +++ b/codex-rs/execpolicy/src/policy.rs @@ -46,6 +46,7 @@ impl Policy { .into(), }, decision, + forbidden_reason: None, }); self.rules_by_program.insert(first_token.clone(), rule); diff --git a/codex-rs/execpolicy/src/rule.rs b/codex-rs/execpolicy/src/rule.rs index cd0756bbb3..59edb23cbd 100644 --- a/codex-rs/execpolicy/src/rule.rs +++ b/codex-rs/execpolicy/src/rule.rs @@ -63,6 +63,11 @@ pub enum RuleMatch { #[serde(rename = "matchedPrefix")] matched_prefix: Vec, decision: Decision, + /// Optional explanation for why a matching rule forbids this command. + /// + /// Only present when provided in the policy and `decision == forbidden`. + #[serde(rename = "forbiddenReason", skip_serializing_if = "Option::is_none")] + forbidden_reason: Option, }, HeuristicsRuleMatch { command: Vec, @@ -83,6 +88,7 @@ impl RuleMatch { pub struct PrefixRule { pub pattern: PrefixPattern, pub decision: Decision, + pub forbidden_reason: Option, } pub trait Rule: Any + Debug + Send + Sync { @@ -104,6 +110,7 @@ impl Rule for PrefixRule { .map(|matched_prefix| RuleMatch::PrefixRuleMatch { matched_prefix, decision: self.decision, + forbidden_reason: self.forbidden_reason.clone(), }) } } diff --git a/codex-rs/execpolicy/tests/basic.rs b/codex-rs/execpolicy/tests/basic.rs index 7ae5e6e213..6f732751c9 100644 --- a/codex-rs/execpolicy/tests/basic.rs +++ b/codex-rs/execpolicy/tests/basic.rs @@ -64,6 +64,7 @@ prefix_rule( matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["git", "status"]), decision: Decision::Allow, + forbidden_reason: None, }], }, evaluation @@ -71,6 +72,56 @@ prefix_rule( Ok(()) } +#[test] +fn forbidden_reason_is_attached_to_forbidden_matches() -> Result<()> { + let policy_src = r#" +prefix_rule( + pattern = ["rm"], + decision = "forbidden", + forbidden_reason = "destructive command", +) + "#; + let mut parser = PolicyParser::new(); + parser.parse("test.rules", policy_src)?; + let policy = parser.build(); + + let evaluation = policy.check( + &tokens(&["rm", "-rf", "/some/important/folder"]), + &allow_all, + ); + assert_eq!( + Evaluation { + decision: Decision::Forbidden, + matched_rules: vec![RuleMatch::PrefixRuleMatch { + matched_prefix: tokens(&["rm"]), + decision: Decision::Forbidden, + forbidden_reason: Some("destructive command".to_string()), + }], + }, + evaluation + ); + Ok(()) +} + +#[test] +fn forbidden_reason_requires_forbidden_decision() { + let policy_src = r#" +prefix_rule( + pattern = ["ls"], + decision = "allow", + forbidden_reason = "not allowed here", +) + "#; + let mut parser = PolicyParser::new(); + let err = parser + .parse("test.rules", policy_src) + .expect_err("expected parse error"); + assert!( + err.to_string() + .contains("invalid rule: forbidden_reason requires decision=\"forbidden\"") + ); +} + #[test] fn add_prefix_rule_extends_policy() -> Result<()> { let mut policy = Policy::empty(); @@ -84,17 +135,19 @@ fn add_prefix_rule_extends_policy() -> Result<()> { rest: vec![PatternToken::Single(String::from("-l"))].into(), }, decision: Decision::Prompt, + forbidden_reason: None, })], rules ); - let evaluation = policy.check(&tokens(&["ls", "-l", "/tmp"]), &allow_all); + let evaluation = policy.check(&tokens(&["ls", "-l", "/some/important/folder"]), &allow_all); assert_eq!( Evaluation { decision: Decision::Prompt, matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["ls", "-l"]), decision: Decision::Prompt, + forbidden_reason: None, }], }, evaluation @@ -142,6 +195,7 @@ prefix_rule( rest: Vec::::new().into(), }, decision: Decision::Prompt, + forbidden_reason: None, }), RuleSnapshot::Prefix(PrefixRule { pattern: PrefixPattern { @@ -149,6 +203,7 @@ prefix_rule( rest: vec![PatternToken::Single("commit".to_string())].into(), }, decision: Decision::Forbidden, + forbidden_reason: None, }), ], git_rules @@ -161,6 +216,7 @@ prefix_rule( matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["git"]), decision: Decision::Prompt, + forbidden_reason: None, }], }, status_eval @@ -174,10 +230,12 @@ prefix_rule( RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["git"]), decision: Decision::Prompt, + forbidden_reason: None, }, RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["git", "commit"]), decision: Decision::Forbidden, + forbidden_reason: None, }, ], }, @@ -211,6 +269,7 @@ prefix_rule( rest: vec![PatternToken::Alts(vec!["-c".to_string(), "-l".to_string()])].into(), }, decision: Decision::Allow, + forbidden_reason: None, })], bash_rules ); @@ -221,6 +280,7 @@ prefix_rule( rest: vec![PatternToken::Alts(vec!["-c".to_string(), "-l".to_string()])].into(), }, decision: Decision::Allow, + forbidden_reason: None, })], sh_rules ); @@ -232,6 +292,7 @@ prefix_rule( matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["bash", "-c"]), decision: Decision::Allow, + forbidden_reason: None, }], }, bash_eval @@ -244,6 +305,7 @@ prefix_rule( matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["sh", "-l"]), decision: Decision::Allow, + forbidden_reason: None, }], }, sh_eval @@ -277,6 +339,7 @@ prefix_rule( .into(), }, decision: Decision::Allow, + forbidden_reason: None, })], rules ); @@ -288,6 +351,7 @@ prefix_rule( matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["npm", "i", "--legacy-peer-deps"]), decision: Decision::Allow, + forbidden_reason: None, }], }, npm_i @@ -303,6 +367,7 @@ prefix_rule( matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["npm", "install", "--no-save"]), decision: Decision::Allow, + forbidden_reason: None, }], }, npm_install @@ -332,6 +397,7 @@ prefix_rule( matched_rules: vec![RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["git", "status"]), decision: Decision::Allow, + forbidden_reason: None, }], }, match_eval @@ -378,10 +444,12 @@ prefix_rule( RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["git"]), decision: Decision::Prompt, + forbidden_reason: None, }, RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["git", "commit"]), decision: Decision::Forbidden, + forbidden_reason: None, }, ], }, @@ -419,14 +487,17 @@ prefix_rule( RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["git"]), decision: Decision::Prompt, + forbidden_reason: None, }, RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["git"]), decision: Decision::Prompt, + forbidden_reason: None, }, RuleMatch::PrefixRuleMatch { matched_prefix: tokens(&["git", "commit"]), decision: Decision::Forbidden, + forbidden_reason: None, }, ], },