[release/0.144] fix(core) expand is_dangerous_command

codex-rs/core/src/exec_policy.rs

@@ -24,7 +24,8 @@ use codex_protocol::config_types::WindowsSandboxLevel;
 use codex_protocol::models::PermissionProfile;
 use codex_protocol::permissions::FileSystemSandboxKind;
 use codex_protocol::protocol::AskForApproval;
-use codex_shell_command::is_dangerous_command::command_might_be_dangerous;
+use codex_shell_command::is_dangerous_command::DangerousCommandMatch;
+use codex_shell_command::is_dangerous_command::dangerous_command_match;
 use codex_shell_command::is_safe_command::is_known_safe_command;
 use thiserror::Error;
 use tokio::fs;
@@ -325,16 +326,34 @@ impl ExecPolicyManager {
 
         match evaluation.decision {
             Decision::Forbidden => ExecApprovalRequirement::Forbidden {
-                reason: derive_forbidden_reason(command, &evaluation),
+                reason: derive_forbidden_reason(
+                    command,
+                    &evaluation,
+                    dangerous_command_match_for_heuristics(
+                        &evaluation,
+                        Decision::Forbidden,
+                        command_origin,
+                    ),
+                ),
             },
             Decision::Prompt => {
                 let prompt_is_rule = evaluation.matched_rules.iter().any(|rule_match| {
                     is_policy_match(rule_match) && rule_match.decision() == Decision::Prompt
                 });
                 match prompt_is_rejected_by_policy(approval_policy, prompt_is_rule) {
-                    Some(reason) => ExecApprovalRequirement::Forbidden {
+                    Some(reason) if prompt_is_rule => ExecApprovalRequirement::Forbidden {
                         reason: reason.to_string(),
                     },
+                    Some(reason) => ExecApprovalRequirement::Forbidden {
+                        reason: derive_rejected_prompt_reason(
+                            reason,
+                            dangerous_command_match_for_heuristics(
+                                &evaluation,
+                                Decision::Prompt,
+                                command_origin,
+                            ),
+                        ),
+                    },
                     None => ExecApprovalRequirement::NeedsApproval {
                         reason: derive_prompt_reason(command, &evaluation),
                         proposed_execpolicy_amendment: requested_amendment.or_else(|| {
@@ -630,11 +649,46 @@ pub async fn load_exec_policy(config_stack: &ConfigLayerStack) -> Result<Policy,
     Ok(policy.merge_overlay(requirements_policy.as_ref()))
 }
 
+fn dangerous_command_match_for_origin(
+    command: &[String],
+    command_origin: ExecPolicyCommandOrigin,
+) -> Option<DangerousCommandMatch> {
+    match command_origin {
+        ExecPolicyCommandOrigin::Generic => dangerous_command_match(command),
+        #[cfg(windows)]
+        ExecPolicyCommandOrigin::PowerShell => {
+            codex_shell_command::is_dangerous_command::dangerous_powershell_words_match(command)
+        }
+    }
+}
+
+/// Extract DangerousCommandMatch from an Evaluation
+fn dangerous_command_match_for_heuristics(
+    evaluation: &Evaluation,
+    decision: Decision,
+    command_origin: ExecPolicyCommandOrigin,
+) -> Option<DangerousCommandMatch> {
+    evaluation
+        .matched_rules
+        .iter()
+        .find_map(|rule_match| match rule_match {
+            RuleMatch::HeuristicsRuleMatch {
+                command,
+                decision: matched_decision,
+            } if *matched_decision == decision => {
+                dangerous_command_match_for_origin(command, command_origin)
+            }
+            _ => None,
+        })
+}
+
 /// If a command is not matched by any execpolicy rule, derive a [`Decision`].
 pub(crate) fn render_decision_for_unmatched_command(
     command: &[String],
     context: UnmatchedCommandContext<'_>,
 ) -> Decision {
+    let dangerous_command_match =
+        dangerous_command_match_for_origin(command, context.command_origin);
     let UnmatchedCommandContext {
         approval_policy,
         permission_profile,
@@ -674,27 +728,10 @@ pub(crate) fn render_decision_for_unmatched_command(
     // 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.
-    let command_is_dangerous = match command_origin {
-        ExecPolicyCommandOrigin::Generic => command_might_be_dangerous(command),
-        #[cfg(windows)]
-        ExecPolicyCommandOrigin::PowerShell => {
-            codex_shell_command::is_dangerous_command::is_dangerous_powershell_words(command)
-        }
-    };
-    if command_is_dangerous || windows_managed_fs_restrictions_without_sandbox_backend {
+    if dangerous_command_match.is_some() || windows_managed_fs_restrictions_without_sandbox_backend
+    {
         return match approval_policy {
-            AskForApproval::Never => {
-                let sandbox_is_explicitly_disabled = matches!(
-                    permission_profile,
-                    PermissionProfile::Disabled | PermissionProfile::External { .. }
-                );
-                if sandbox_is_explicitly_disabled {
-                    // If the sandbox is explicitly disabled, we should allow the command to run
-                    Decision::Allow
-                } else {
-                    Decision::Forbidden
-                }
-            }
+            AskForApproval::Never => Decision::Forbidden,
             AskForApproval::OnRequest
             | AskForApproval::UnlessTrusted
             | AskForApproval::Granular(_) => Decision::Prompt,
@@ -955,7 +992,11 @@ fn render_shlex_command(args: &[String]) -> String {
 /// Derive a string explaining why the command was forbidden. If `justification`
 /// is set by the user, this can contain instructions with recommended
 /// alternatives, for example.
-fn derive_forbidden_reason(command_args: &[String], evaluation: &Evaluation) -> String {
+fn derive_forbidden_reason(
+    command_args: &[String],
+    evaluation: &Evaluation,
+    dangerous_command_match: Option<DangerousCommandMatch>,
+) -> String {
     let command = render_shlex_command(command_args);
 
     let most_specific_forbidden = evaluation
@@ -980,7 +1021,37 @@ fn derive_forbidden_reason(command_args: &[String], evaluation: &Evaluation) ->
             let prefix = render_shlex_command(matched_prefix);
             format!("`{command}` rejected: policy forbids commands starting with `{prefix}`")
         }
-        None => format!("`{command}` rejected: blocked by policy"),
+        None => {
+            if let Some(dangerous_command_match) = dangerous_command_match {
+                let reason = dangerous_command_rejection_reason(dangerous_command_match);
+                format!("`{command}` rejected: {reason}")
+            } else {
+                format!("`{command}` rejected: blocked by policy")
+            }
+        }
+    }
+}
+
+fn derive_rejected_prompt_reason(
+    fallback_reason: &str,
+    dangerous_command_match: Option<DangerousCommandMatch>,
+) -> String {
+    match dangerous_command_match {
+        Some(dangerous_command_match @ DangerousCommandMatch::ForcedRm) => {
+            dangerous_command_rejection_reason(dangerous_command_match).to_string()
+        }
+        Some(DangerousCommandMatch::Other) | None => fallback_reason.to_string(),
+    }
+}
+
+fn dangerous_command_rejection_reason(
+    dangerous_command_match: DangerousCommandMatch,
+) -> &'static str {
+    match dangerous_command_match {
+        DangerousCommandMatch::ForcedRm => {
+            "rm -f style commands are not permitted. Use a safer approach"
+        }
+        DangerousCommandMatch::Other => "blocked by policy",
     }
 }
 

codex-rs/core/src/exec_policy_tests.rs

@@ -224,7 +224,7 @@ async fn returns_empty_policy_when_no_policy_files_exist() {
             decision: Decision::Allow,
             matched_rules: vec![RuleMatch::HeuristicsRuleMatch {
                 command: vec!["rm".to_string()],
-                decision: Decision::Allow
+                decision: Decision::Allow,
             }],
         },
         policy.check_multiple(commands.iter(), &|_| Decision::Allow)
@@ -481,7 +481,7 @@ async fn ignores_policies_outside_policy_dir() {
             decision: Decision::Allow,
             matched_rules: vec![RuleMatch::HeuristicsRuleMatch {
                 command: vec!["ls".to_string()],
-                decision: Decision::Allow
+                decision: Decision::Allow,
             }],
         },
         policy.check_multiple(command.iter(), &|_| Decision::Allow)
@@ -1346,6 +1346,28 @@ async fn exec_approval_requirement_rejects_unmatched_sandbox_escalation_when_gra
     .await;
 }
 
+#[test]
+fn other_danger_preserves_rejected_prompt_reason() {
+    assert_eq!(
+        derive_rejected_prompt_reason(
+            REJECT_SANDBOX_APPROVAL_REASON,
+            Some(DangerousCommandMatch::Other),
+        ),
+        REJECT_SANDBOX_APPROVAL_REASON
+    );
+}
+
+#[test]
+fn forced_rm_rejected_prompt_reason_does_not_repeat_command() {
+    assert_eq!(
+        derive_rejected_prompt_reason(
+            REJECT_SANDBOX_APPROVAL_REASON,
+            Some(DangerousCommandMatch::ForcedRm),
+        ),
+        "rm -f style commands are not permitted. Use a safer approach"
+    );
+}
+
 #[tokio::test]
 async fn mixed_rule_and_sandbox_prompt_prioritizes_rule_for_rejection_decision() {
     let policy_src = r#"prefix_rule(pattern=["git"], decision="prompt")"#;
@@ -1384,7 +1406,7 @@ async fn mixed_rule_and_sandbox_prompt_prioritizes_rule_for_rejection_decision()
 }
 
 #[tokio::test]
-async fn mixed_rule_and_sandbox_prompt_rejects_when_granular_rules_are_disabled() {
+async fn forced_rm_preserves_rule_rejection_when_granular_rules_are_disabled() {
     let policy_src = r#"prefix_rule(pattern=["git"], decision="prompt")"#;
     let mut parser = PolicyParser::new();
     parser
@@ -1394,7 +1416,7 @@ async fn mixed_rule_and_sandbox_prompt_rejects_when_granular_rules_are_disabled(
     let command = vec![
         "bash".to_string(),
         "-lc".to_string(),
-        "git status && madeup-cmd".to_string(),
+        "git status && rm -rf /tmp/example".to_string(),
     ];
 
     let requirement = manager
@@ -2015,10 +2037,85 @@ async fn dangerous_rm_rf_requires_approval_in_danger_full_access() {
     .await;
 }
 
+#[tokio::test]
+async fn dangerous_rm_rf_in_shell_loop_requires_approval_in_danger_full_access() {
+    let command = vec_str(&[
+        "bash",
+        "-lc",
+        "for target in /tmp/a /tmp/b; do rm -rf \"$target\"; done",
+    ]);
+
+    assert_exec_approval_requirement_for_command(
+        ExecApprovalRequirementScenario {
+            policy_src: None,
+            command: command.clone(),
+            approval_policy: AskForApproval::OnRequest,
+            permission_profile: PermissionProfile::Disabled,
+            sandbox_permissions: SandboxPermissions::UseDefault,
+            prefix_rule: None,
+        },
+        ExecApprovalRequirement::NeedsApproval {
+            reason: None,
+            proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(command)),
+        },
+    )
+    .await;
+}
+
 fn vec_str(items: &[&str]) -> Vec<String> {
     items.iter().map(std::string::ToString::to_string).collect()
 }
 
+#[tokio::test]
+async fn forced_rm_requires_approval_or_specific_rejection_on_all_platforms() {
+    let policy = ExecPolicyManager::new(Arc::new(Policy::empty()));
+    let permissions = SandboxPermissions::UseDefault;
+    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(ExecApprovalRequest {
+                command: &dangerous_command,
+                approval_policy: AskForApproval::OnRequest,
+                permission_profile: PermissionProfile::read_only(),
+                windows_sandbox_level: WindowsSandboxLevel::Disabled,
+                sandbox_permissions: permissions,
+                prefix_rule: None,
+            })
+            .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: rm -f style commands are not permitted. Use a safer approach"
+                .to_string(),
+        },
+        policy
+            .create_exec_approval_requirement_for_command(ExecApprovalRequest {
+                command: &dangerous_command,
+                approval_policy: AskForApproval::Never,
+                permission_profile: PermissionProfile::read_only(),
+                windows_sandbox_level: WindowsSandboxLevel::Disabled,
+                sandbox_permissions: permissions,
+                prefix_rule: None,
+            })
+            .await,
+        r#"On all platforms, a forbidden command should require approval
+            (unless AskForApproval::Never is specified)."#
+    );
+}
+
 /// Note this test behaves differently on Windows because it exercises an
 /// `if cfg!(windows)` code path in render_decision_for_unmatched_command().
 #[tokio::test]
@@ -2074,55 +2171,10 @@ async fn verify_approval_requirement_for_unsafe_powershell_command() {
             .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(ExecApprovalRequest {
-                command: &dangerous_command,
-                approval_policy: AskForApproval::OnRequest,
-                permission_profile: PermissionProfile::read_only(),
-                windows_sandbox_level: WindowsSandboxLevel::Disabled,
-                sandbox_permissions: permissions,
-                prefix_rule: None,
-            })
-            .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(ExecApprovalRequest {
-                command: &dangerous_command,
-                approval_policy: AskForApproval::Never,
-                permission_profile: PermissionProfile::read_only(),
-                windows_sandbox_level: WindowsSandboxLevel::Disabled,
-                sandbox_permissions: permissions,
-                prefix_rule: None,
-            })
-            .await,
-        r#"On all platforms, a forbidden command should require approval
-            (unless AskForApproval::Never is specified)."#
-    );
 }
 
 #[tokio::test]
-async fn dangerous_command_allowed_when_sandbox_is_explicitly_disabled() {
+async fn dangerous_command_forbidden_when_sandbox_is_explicitly_disabled() {
     let command = vec_str(&["rm", "-rf", "/tmp/nonexistent"]);
     assert_exec_approval_requirement_for_command(
         ExecApprovalRequirementScenario {
@@ -2135,11 +2187,9 @@ async fn dangerous_command_allowed_when_sandbox_is_explicitly_disabled() {
             sandbox_permissions: SandboxPermissions::UseDefault,
             prefix_rule: None,
         },
-        ExecApprovalRequirement::Skip {
-            bypass_sandbox: false,
-            proposed_execpolicy_amendment: Some(ExecPolicyAmendment {
-                command: vec_str(&["rm", "-rf", "/tmp/nonexistent"]),
-            }),
+        ExecApprovalRequirement::Forbidden {
+            reason: "`rm -rf /tmp/nonexistent` rejected: rm -f style commands are not permitted. Use a safer approach"
+                .to_string(),
         },
     )
     .await;