From 82d192fec334d147501f071d447afde6471cbd9a Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 12 Jan 2026 14:12:56 -0800 Subject: [PATCH] fix requires_initial_appoval --- .../command_safety/is_dangerous_command.rs | 58 --------- codex-rs/core/src/exec_policy.rs | 112 ++++++++++++++++-- codex-rs/execpolicy/src/policy.rs | 31 +++-- 3 files changed, 124 insertions(+), 77 deletions(-) diff --git a/codex-rs/core/src/command_safety/is_dangerous_command.rs b/codex-rs/core/src/command_safety/is_dangerous_command.rs index 014cd7c0fa..256f36c600 100644 --- a/codex-rs/core/src/command_safety/is_dangerous_command.rs +++ b/codex-rs/core/src/command_safety/is_dangerous_command.rs @@ -1,46 +1,8 @@ -use codex_protocol::protocol::AskForApproval; -use codex_protocol::protocol::SandboxPolicy; - -use crate::sandboxing::SandboxPermissions; - use crate::bash::parse_shell_lc_plain_commands; -use crate::is_safe_command::is_known_safe_command; #[cfg(windows)] #[path = "windows_dangerous_commands.rs"] mod windows_dangerous_commands; -pub fn requires_initial_appoval( - policy: AskForApproval, - sandbox_policy: &SandboxPolicy, - command: &[String], - sandbox_permissions: SandboxPermissions, -) -> bool { - if is_known_safe_command(command) { - return false; - } - match policy { - AskForApproval::Never | AskForApproval::OnFailure => false, - AskForApproval::OnRequest => { - // In DangerFullAccess or ExternalSandbox, only prompt if the command looks dangerous. - if matches!( - sandbox_policy, - SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } - ) { - return command_might_be_dangerous(command); - } - - // In restricted sandboxes (ReadOnly/WorkspaceWrite), do not prompt for - // non‑escalated, non‑dangerous commands — let the sandbox enforce - // restrictions (e.g., block network/write) without a user prompt. - if sandbox_permissions.requires_escalated_permissions() { - return true; - } - command_might_be_dangerous(command) - } - AskForApproval::UnlessTrusted => !is_known_safe_command(command), - } -} - pub fn command_might_be_dangerous(command: &[String]) -> bool { #[cfg(windows)] { @@ -86,7 +48,6 @@ fn is_dangerous_to_call_with_exec(command: &[String]) -> bool { #[cfg(test)] mod tests { use super::*; - use codex_protocol::protocol::NetworkAccess; fn vec_str(items: &[&str]) -> Vec { items.iter().map(std::string::ToString::to_string).collect() @@ -154,23 +115,4 @@ mod tests { fn rm_f_is_dangerous() { assert!(command_might_be_dangerous(&vec_str(&["rm", "-f", "/"]))); } - - #[test] - fn external_sandbox_only_prompts_for_dangerous_commands() { - let external_policy = SandboxPolicy::ExternalSandbox { - network_access: NetworkAccess::Restricted, - }; - assert!(!requires_initial_appoval( - AskForApproval::OnRequest, - &external_policy, - &vec_str(&["ls"]), - SandboxPermissions::UseDefault, - )); - assert!(requires_initial_appoval( - AskForApproval::OnRequest, - &external_policy, - &vec_str(&["rm", "-rf", "/"]), - SandboxPermissions::UseDefault, - )); - } } diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index d8880a9b3c..9d85bf5f22 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -5,9 +5,10 @@ use std::sync::Arc; use arc_swap::ArcSwap; -use crate::command_safety::is_dangerous_command::requires_initial_appoval; use crate::config_loader::ConfigLayerStack; use crate::config_loader::ConfigLayerStackOrdering; +use crate::is_dangerous_command::command_might_be_dangerous; +use crate::is_safe_command::is_known_safe_command; use codex_execpolicy::AmendError; use codex_execpolicy::Decision; use codex_execpolicy::Error as ExecPolicyRuleError; @@ -116,14 +117,15 @@ impl ExecPolicyManager { let exec_policy = self.current(); let commands = parse_shell_lc_plain_commands(command).unwrap_or_else(|| vec![command.to_vec()]); - let heuristics_fallback = |cmd: &[String]| { - if requires_initial_appoval(approval_policy, sandbox_policy, cmd, sandbox_permissions) { - Decision::Prompt - } else { - Decision::Allow - } + let exec_policy_fallback = |cmd: &[String]| { + render_decision_for_unmatched_command( + approval_policy, + sandbox_policy, + cmd, + sandbox_permissions, + ) }; - let evaluation = exec_policy.check_multiple(commands.iter(), &heuristics_fallback); + let evaluation = exec_policy.check_multiple(commands.iter(), &exec_policy_fallback); match evaluation.decision { Decision::Forbidden => ExecApprovalRequirement::Forbidden { @@ -242,6 +244,70 @@ pub async fn load_exec_policy(config_stack: &ConfigLayerStack) -> Result Decision { + if is_known_safe_command(command) { + return Decision::Allow; + } + + // On Windows, ReadOnly sandbox is not a real sandbox, so special-case it + // here. + let runtime_sandbox_provides_safety = + cfg!(windows) && matches!(sandbox_policy, SandboxPolicy::ReadOnly); + + // If the command is flagged as dangerous or we have no sandbox protection, + // we should never allow it to run without user approval. + // + // We prefer to prompt the user rather than outright forbid the command, + // but if the user has explicitly disabled prompts, we must + // forbid the command. + if command_might_be_dangerous(command) || runtime_sandbox_provides_safety { + return if matches!(approval_policy, AskForApproval::Never) { + Decision::Forbidden + } else { + Decision::Prompt + }; + } + + match approval_policy { + AskForApproval::Never | AskForApproval::OnFailure => { + // We allow the command to run, relying on the sandbox for + // protection. + Decision::Allow + } + AskForApproval::UnlessTrusted => { + // We already checked `is_known_safe_command(command)` and it + // returned false, so we must prompt. + Decision::Prompt + } + AskForApproval::OnRequest => { + match sandbox_policy { + SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => { + // The user has indicated we should "just run" commands + // in their unrestricted environment, so we do so since the + // command has not been flagged as dangerous. + Decision::Allow + } + SandboxPolicy::ReadOnly | SandboxPolicy::WorkspaceWrite { .. } => { + // In restricted sandboxes (ReadOnly/WorkspaceWrite), do not prompt for + // non‑escalated, non‑dangerous commands — let the sandbox enforce + // restrictions (e.g., block network/write) without a user prompt. + if sandbox_permissions.requires_escalated_permissions() { + Decision::Prompt + } else { + Decision::Allow + } + } + } + } + } +} + fn default_policy_path(codex_home: &Path) -> PathBuf { codex_home.join(RULES_DIR_NAME).join(DEFAULT_POLICY_FILE) } @@ -426,6 +492,7 @@ mod tests { use crate::features::Features; use codex_app_server_protocol::ConfigLayerSource; use codex_protocol::protocol::AskForApproval; + use codex_protocol::protocol::NetworkAccess; use codex_protocol::protocol::SandboxPolicy; use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; @@ -1051,4 +1118,33 @@ prefix_rule( } ); } + + #[test] + fn decide_how_to_deal_with_for_unmatched_command() { + fn vec_str(items: &[&str]) -> Vec { + items.iter().map(std::string::ToString::to_string).collect() + } + + let external_policy = SandboxPolicy::ExternalSandbox { + network_access: NetworkAccess::Restricted, + }; + assert_eq!( + Decision::Allow, + render_decision_for_unmatched_command( + AskForApproval::OnRequest, + &external_policy, + &vec_str(&["ls"]), + SandboxPermissions::UseDefault, + ) + ); + assert_eq!( + Decision::Prompt, + render_decision_for_unmatched_command( + AskForApproval::OnRequest, + &external_policy, + &vec_str(&["rm", "-rf", "/"]), + SandboxPermissions::UseDefault, + ) + ); + } } diff --git a/codex-rs/execpolicy/src/policy.rs b/codex-rs/execpolicy/src/policy.rs index 0c06d572e4..1e758277b8 100644 --- a/codex-rs/execpolicy/src/policy.rs +++ b/codex-rs/execpolicy/src/policy.rs @@ -61,6 +61,7 @@ impl Policy { Evaluation::from_matches(matched_rules) } + /// Checks multiple commands and aggregates the results. pub fn check_multiple( &self, commands: Commands, @@ -81,12 +82,19 @@ impl Policy { Evaluation::from_matches(matched_rules) } + /// Returns matching rules for the given command. If no rules match and + /// `heuristics_fallback` is provided, returns a single + /// `HeuristicsRuleMatch` with the decision rendered by + /// `heuristics_fallback`. + /// + /// If `heuristics_fallback.is_some()`, then the returned vector is + /// guaranteed to be non-empty. pub fn matches_for_command( &self, cmd: &[String], heuristics_fallback: HeuristicsFallback<'_>, ) -> Vec { - let mut matched_rules: Vec = match cmd.first() { + let matched_rules: Vec = match cmd.first() { Some(first) => self .rules_by_program .get_vec(first) @@ -95,14 +103,16 @@ impl Policy { None => Vec::new(), }; - if let (true, Some(heuristics_fallback)) = (matched_rules.is_empty(), heuristics_fallback) { - matched_rules.push(RuleMatch::HeuristicsRuleMatch { + if matched_rules.is_empty() + && let Some(heuristics_fallback) = heuristics_fallback + { + vec![RuleMatch::HeuristicsRuleMatch { command: cmd.to_vec(), decision: heuristics_fallback(cmd), - }); + }] + } else { + matched_rules } - - matched_rules } } @@ -121,12 +131,11 @@ impl Evaluation { .any(|rule_match| !matches!(rule_match, RuleMatch::HeuristicsRuleMatch { .. })) } + /// Caller is responsible for ensuring that `matched_rules` is non-empty. fn from_matches(matched_rules: Vec) -> Self { - let decision = matched_rules - .iter() - .map(RuleMatch::decision) - .max() - .unwrap_or(Decision::Allow); + let decision = matched_rules.iter().map(RuleMatch::decision).max(); + #[expect(clippy::expect_used)] + let decision = decision.expect("invariant failed: matched_rules must be non-empty"); Self { decision,