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 014cd7c0fae..256f36c6005 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 d8880a9b3c4..b057035bc9d 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) } @@ -1051,4 +1117,108 @@ prefix_rule( } ); } + + fn vec_str(items: &[&str]) -> Vec { + items.iter().map(std::string::ToString::to_string).collect() + } + + /// Note this test behaves differently on Windows because it exercises an + /// `if cfg!(windows)` code path in render_decision_for_unmatched_command(). + #[tokio::test] + async fn verify_approval_requirement_for_unsafe_powershell_command() { + // `brew install powershell` to run this test on a Mac! + // Note `pwsh` is required to parse a PowerShell command to see if it + // is safe. + if which::which("pwsh").is_err() { + return; + } + + let policy = ExecPolicyManager::new(Arc::new(Policy::empty())); + let features = Features::with_defaults(); + let permissions = SandboxPermissions::UseDefault; + + // This command should not be run without user approval unless there is + // a proper sandbox in place to ensure safety. + let sneaky_command = vec_str(&["pwsh", "-Command", "echo hi @(calc)"]); + let expected_amendment = Some(ExecPolicyAmendment::new(vec_str(&[ + "pwsh", + "-Command", + "echo hi @(calc)", + ]))); + let (pwsh_approval_reason, expected_req) = if cfg!(windows) { + ( + r#"On Windows, SandboxPolicy::ReadOnly should be assumed to mean + that no sandbox is present, so anything that is not "provably + safe" should require approval."#, + ExecApprovalRequirement::NeedsApproval { + reason: None, + proposed_execpolicy_amendment: expected_amendment.clone(), + }, + ) + } else { + ( + "On non-Windows, rely on the read-only sandbox to prevent harm.", + ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: expected_amendment.clone(), + }, + ) + }; + assert_eq!( + expected_req, + policy + .create_exec_approval_requirement_for_command( + &features, + &sneaky_command, + AskForApproval::OnRequest, + &SandboxPolicy::ReadOnly, + permissions, + ) + .await, + "{pwsh_approval_reason}" + ); + + // This is flagged as a dangerous command on all platforms. + let dangerous_command = vec_str(&["rm", "-rf", "/important/data"]); + assert_eq!( + ExecApprovalRequirement::NeedsApproval { + reason: None, + proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec_str(&[ + "rm", + "-rf", + "/important/data", + ]))), + }, + policy + .create_exec_approval_requirement_for_command( + &features, + &dangerous_command, + AskForApproval::OnRequest, + &SandboxPolicy::ReadOnly, + permissions, + ) + .await, + r#"On all platforms, a forbidden command should require approval + (unless AskForApproval::Never is specified)."# + ); + + // A dangerous command should be forbidden if the user has specified + // AskForApproval::Never. + assert_eq!( + ExecApprovalRequirement::Forbidden { + reason: "`rm -rf /important/data` rejected: blocked by policy".to_string(), + }, + policy + .create_exec_approval_requirement_for_command( + &features, + &dangerous_command, + AskForApproval::Never, + &SandboxPolicy::ReadOnly, + permissions, + ) + .await, + r#"On all platforms, a forbidden command should require approval + (unless AskForApproval::Never is specified)."# + ); + } } diff --git a/codex-rs/execpolicy/src/policy.rs b/codex-rs/execpolicy/src/policy.rs index 0c06d572e4b..1e758277b84 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,