From eb2b739d6a0da69c6f5bdc1885db4d2cc9e2bd36 Mon Sep 17 00:00:00 2001
From: iceweasel-oai <iceweasel@openai.com>
Date: Thu, 25 Sep 2025 19:46:20 -0700
Subject: [PATCH] core: add potentially dangerous command check (#4211)

Certain shell commands are potentially dangerous, and we want to check
for them.
Unless the user has explicitly approved a command, we will *always* ask
them for approval
when one of these commands is encountered, regardless of whether they
are in a sandbox, or what their approval policy is.

The first (of probably many) such examples is `git reset --hard`. We
will be conservative and check for any `git reset`
---
 codex-rs/core/src/bash.rs                     | 15 +++
 .../command_safety/is_dangerous_command.rs    | 99 +++++++++++++++++++
 .../src/command_safety/is_safe_command.rs     |  9 +-
 codex-rs/core/src/command_safety/mod.rs       |  1 +
 codex-rs/core/src/safety.rs                   | 56 ++++++++++-
 5 files changed, 172 insertions(+), 8 deletions(-)
 create mode 100644 codex-rs/core/src/command_safety/is_dangerous_command.rs

diff --git a/codex-rs/core/src/bash.rs b/codex-rs/core/src/bash.rs
index f25b4f7f..c87f2764 100644
--- a/codex-rs/core/src/bash.rs
+++ b/codex-rs/core/src/bash.rs
@@ -88,6 +88,21 @@ pub fn try_parse_word_only_commands_sequence(tree: &Tree, src: &str) -> Option<V
     Some(commands)
 }
 
+/// Returns the sequence of plain commands within a `bash -lc "..."` invocation
+/// when the script only contains word-only commands joined by safe operators.
+pub fn parse_bash_lc_plain_commands(command: &[String]) -> Option<Vec<Vec<String>>> {
+    let [bash, flag, script] = command else {
+        return None;
+    };
+
+    if bash != "bash" || flag != "-lc" {
+        return None;
+    }
+
+    let tree = try_parse_bash(script)?;
+    try_parse_word_only_commands_sequence(&tree, script)
+}
+
 fn parse_plain_command_from_node(cmd: tree_sitter::Node, src: &str) -> Option<Vec<String>> {
     if cmd.kind() != "command" {
         return None;
diff --git a/codex-rs/core/src/command_safety/is_dangerous_command.rs b/codex-rs/core/src/command_safety/is_dangerous_command.rs
new file mode 100644
index 00000000..852af93e
--- /dev/null
+++ b/codex-rs/core/src/command_safety/is_dangerous_command.rs
@@ -0,0 +1,99 @@
+use crate::bash::parse_bash_lc_plain_commands;
+
+pub fn command_might_be_dangerous(command: &[String]) -> bool {
+    if is_dangerous_to_call_with_exec(command) {
+        return true;
+    }
+
+    // Support `bash -lc "<script>"` where the any part of the script might contain a dangerous command.
+    if let Some(all_commands) = parse_bash_lc_plain_commands(command)
+        && all_commands
+            .iter()
+            .any(|cmd| is_dangerous_to_call_with_exec(cmd))
+    {
+        return true;
+    }
+
+    false
+}
+
+fn is_dangerous_to_call_with_exec(command: &[String]) -> bool {
+    let cmd0 = command.first().map(String::as_str);
+
+    match cmd0 {
+        Some(cmd) if cmd.ends_with("git") || cmd.ends_with("/git") => {
+            matches!(command.get(1).map(String::as_str), Some("reset" | "rm"))
+        }
+
+        Some("rm") => matches!(command.get(1).map(String::as_str), Some("-f" | "-rf")),
+
+        // for sudo <cmd> simply do the check for <cmd>
+        Some("sudo") => is_dangerous_to_call_with_exec(&command[1..]),
+
+        // ── anything else ─────────────────────────────────────────────────
+        _ => false,
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    fn vec_str(items: &[&str]) -> Vec<String> {
+        items.iter().map(std::string::ToString::to_string).collect()
+    }
+
+    #[test]
+    fn git_reset_is_dangerous() {
+        assert!(command_might_be_dangerous(&vec_str(&["git", "reset"])));
+    }
+
+    #[test]
+    fn bash_git_reset_is_dangerous() {
+        assert!(command_might_be_dangerous(&vec_str(&[
+            "bash",
+            "-lc",
+            "git reset --hard"
+        ])));
+    }
+
+    #[test]
+    fn git_status_is_not_dangerous() {
+        assert!(!command_might_be_dangerous(&vec_str(&["git", "status"])));
+    }
+
+    #[test]
+    fn bash_git_status_is_not_dangerous() {
+        assert!(!command_might_be_dangerous(&vec_str(&[
+            "bash",
+            "-lc",
+            "git status"
+        ])));
+    }
+
+    #[test]
+    fn sudo_git_reset_is_dangerous() {
+        assert!(command_might_be_dangerous(&vec_str(&[
+            "sudo", "git", "reset", "--hard"
+        ])));
+    }
+
+    #[test]
+    fn usr_bin_git_is_dangerous() {
+        assert!(command_might_be_dangerous(&vec_str(&[
+            "/usr/bin/git",
+            "reset",
+            "--hard"
+        ])));
+    }
+
+    #[test]
+    fn rm_rf_is_dangerous() {
+        assert!(command_might_be_dangerous(&vec_str(&["rm", "-rf", "/"])));
+    }
+
+    #[test]
+    fn rm_f_is_dangerous() {
+        assert!(command_might_be_dangerous(&vec_str(&["rm", "-f", "/"])));
+    }
+}
diff --git a/codex-rs/core/src/command_safety/is_safe_command.rs b/codex-rs/core/src/command_safety/is_safe_command.rs
index c3321540..c1273caa 100644
--- a/codex-rs/core/src/command_safety/is_safe_command.rs
+++ b/codex-rs/core/src/command_safety/is_safe_command.rs
@@ -1,5 +1,4 @@
-use crate::bash::try_parse_bash;
-use crate::bash::try_parse_word_only_commands_sequence;
+use crate::bash::parse_bash_lc_plain_commands;
 
 pub fn is_known_safe_command(command: &[String]) -> bool {
     #[cfg(target_os = "windows")]
@@ -20,11 +19,7 @@ pub fn is_known_safe_command(command: &[String]) -> bool {
     // introduce side effects ( "&&", "||", ";", and "|" ). If every
     // individual command in the script is itself a known‑safe command, then
     // the composite expression is considered safe.
-    if let [bash, flag, script] = command
-        && bash == "bash"
-        && flag == "-lc"
-        && let Some(tree) = try_parse_bash(script)
-        && let Some(all_commands) = try_parse_word_only_commands_sequence(&tree, script)
+    if let Some(all_commands) = parse_bash_lc_plain_commands(command)
         && !all_commands.is_empty()
         && all_commands
             .iter()
diff --git a/codex-rs/core/src/command_safety/mod.rs b/codex-rs/core/src/command_safety/mod.rs
index b3095c86..8279d679 100644
--- a/codex-rs/core/src/command_safety/mod.rs
+++ b/codex-rs/core/src/command_safety/mod.rs
@@ -1,3 +1,4 @@
+pub mod is_dangerous_command;
 pub mod is_safe_command;
 #[cfg(target_os = "windows")]
 pub mod windows_safe_commands;
diff --git a/codex-rs/core/src/safety.rs b/codex-rs/core/src/safety.rs
index 42b1ffdf..a3462c34 100644
--- a/codex-rs/core/src/safety.rs
+++ b/codex-rs/core/src/safety.rs
@@ -7,7 +7,9 @@ use codex_apply_patch::ApplyPatchAction;
 use codex_apply_patch::ApplyPatchFileChange;
 
 use crate::exec::SandboxType;
-use crate::is_safe_command::is_known_safe_command;
+
+use crate::command_safety::is_dangerous_command::command_might_be_dangerous;
+use crate::command_safety::is_safe_command::is_known_safe_command;
 use crate::protocol::AskForApproval;
 use crate::protocol::SandboxPolicy;
 
@@ -85,6 +87,13 @@ pub fn assess_command_safety(
     approved: &HashSet<Vec<String>>,
     with_escalated_permissions: bool,
 ) -> SafetyCheck {
+    // Some commands look dangerous. Even if they are run inside a sandbox,
+    // unless the user has explicitly approved them, we should ask,
+    // regardless of the approval policy and sandbox policy.
+    if command_might_be_dangerous(command) && !approved.contains(command) {
+        return SafetyCheck::AskUser;
+    }
+
     // A command is "trusted" because either:
     // - it belongs to a set of commands we consider "safe" by default, or
     // - the user has explicitly approved the command for this session
@@ -98,6 +107,7 @@ pub fn assess_command_safety(
     // would probably be fine to run the command in a sandbox, but when
     // `approved.contains(command)` is `true`, the user may have approved it for
     // the session _because_ they know it needs to run outside a sandbox.
+
     if is_known_safe_command(command) || approved.contains(command) {
         return SafetyCheck::AutoApprove {
             sandbox_type: SandboxType::None,
@@ -325,6 +335,50 @@ mod tests {
         assert_eq!(safety_check, SafetyCheck::AskUser);
     }
 
+    #[test]
+    fn dangerous_command_allowed_if_explicitly_approved() {
+        let command = vec!["git".to_string(), "reset".to_string(), "--hard".to_string()];
+        let approval_policy = AskForApproval::OnRequest;
+        let sandbox_policy = SandboxPolicy::ReadOnly;
+        let mut approved: HashSet<Vec<String>> = HashSet::new();
+        approved.insert(command.clone());
+        let request_escalated_privileges = false;
+
+        let safety_check = assess_command_safety(
+            &command,
+            approval_policy,
+            &sandbox_policy,
+            &approved,
+            request_escalated_privileges,
+        );
+
+        assert_eq!(
+            safety_check,
+            SafetyCheck::AutoApprove {
+                sandbox_type: SandboxType::None
+            }
+        );
+    }
+
+    #[test]
+    fn dangerous_command_not_allowed_if_not_explicitly_approved() {
+        let command = vec!["git".to_string(), "reset".to_string(), "--hard".to_string()];
+        let approval_policy = AskForApproval::Never;
+        let sandbox_policy = SandboxPolicy::ReadOnly;
+        let approved: HashSet<Vec<String>> = HashSet::new();
+        let request_escalated_privileges = false;
+
+        let safety_check = assess_command_safety(
+            &command,
+            approval_policy,
+            &sandbox_policy,
+            &approved,
+            request_escalated_privileges,
+        );
+
+        assert_eq!(safety_check, SafetyCheck::AskUser);
+    }
+
     #[test]
     fn test_request_escalated_privileges_no_sandbox_fallback() {
         let command = vec!["git".to_string(), "commit".to_string()];