fix: prompt for unsafe commands on Windows

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<String> {
         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,
-        ));
-    }
 }

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<Policy,
     Ok(policy)
 }
 
+/// If a command is not matched by any execpolicy rule, derive a [`Decision`].
+pub fn render_decision_for_unmatched_command(
+    approval_policy: AskForApproval,
+    sandbox_policy: &SandboxPolicy,
+    command: &[String],
+    sandbox_permissions: SandboxPermissions,
+) -> 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<String> {
+        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)."#
+        );
+    }
 }

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<Commands, F>(
         &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<RuleMatch> {
-        let mut matched_rules: Vec<RuleMatch> = match cmd.first() {
+        let matched_rules: Vec<RuleMatch> = 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<RuleMatch>) -> 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,