fix: unsafe auto-approval of git commands (#10258)

fixes https://github.com/openai/codex/issues/10160 and some more.

## Description

Hardens Git command safety to prevent approval bypasses for destructive
or write-capable invocations (branch delete, risky push forms,
output/config-override flags), so these commands no longer auto-run as
“safe.”

- `git branch -d` variants (especially in worktrees / with global
options like -C / -c)
- `git show|diff|log --output` ... style file-write flags
- risky Git config override flags (-c, --config-env) that can trigger
external execution
- dangerous push forms that weren’t fully caught (`--force*`,
`--delete`, `+refspec`, `:refspec`)
- grouped short-flag delete forms (e.g. stacked branch flags containing
`d/D`)

will fast follow with a common git policy to bring windows to parity.

---------

Co-authored-by: Eric Traut <etraut@openai.com>

codex-rs/core/src/command_safety/is_dangerous_command.rs

@@ -27,12 +27,100 @@ pub fn command_might_be_dangerous(command: &[String]) -> bool {
     false
 }
 
+fn is_git_global_option_with_value(arg: &str) -> bool {
+    matches!(
+        arg,
+        "-C" | "-c"
+            | "--config-env"
+            | "--exec-path"
+            | "--git-dir"
+            | "--namespace"
+            | "--super-prefix"
+            | "--work-tree"
+    )
+}
+
+fn is_git_global_option_with_inline_value(arg: &str) -> bool {
+    matches!(
+        arg,
+        s if s.starts_with("--config-env=")
+            || s.starts_with("--exec-path=")
+            || s.starts_with("--git-dir=")
+            || s.starts_with("--namespace=")
+            || s.starts_with("--super-prefix=")
+            || s.starts_with("--work-tree=")
+    ) || ((arg.starts_with("-C") || arg.starts_with("-c")) && arg.len() > 2)
+}
+
+/// Find the first matching git subcommand, skipping known global options that
+/// may appear before it (e.g., `-C`, `-c`, `--git-dir`).
+///
+/// Shared with `is_safe_command` to avoid git-global-option bypasses.
+pub(crate) fn find_git_subcommand<'a>(
+    command: &'a [String],
+    subcommands: &[&str],
+) -> Option<(usize, &'a str)> {
+    let cmd0 = command.first().map(String::as_str)?;
+    if !cmd0.ends_with("git") {
+        return None;
+    }
+
+    let mut skip_next = false;
+    for (idx, arg) in command.iter().enumerate().skip(1) {
+        if skip_next {
+            skip_next = false;
+            continue;
+        }
+
+        let arg = arg.as_str();
+
+        if is_git_global_option_with_inline_value(arg) {
+            continue;
+        }
+
+        if is_git_global_option_with_value(arg) {
+            skip_next = true;
+            continue;
+        }
+
+        if arg == "--" || arg.starts_with('-') {
+            continue;
+        }
+
+        if subcommands.contains(&arg) {
+            return Some((idx, arg));
+        }
+
+        // In git, the first non-option token is the subcommand. If it isn't
+        // one of the subcommands we're looking for, we must stop scanning to
+        // avoid misclassifying later positional args (e.g., branch names).
+        return None;
+    }
+
+    None
+}
+
 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") => {
+        Some(cmd) if cmd.ends_with("git") => {
-            matches!(command.get(1).map(String::as_str), Some("reset" | "rm"))
+            let Some((subcommand_idx, subcommand)) =
+                find_git_subcommand(command, &["reset", "rm", "branch", "push", "clean"])
+            else {
+                return false;
+            };
+
+            match subcommand {
+                "reset" | "rm" => true,
+                "branch" => git_branch_is_delete(&command[subcommand_idx + 1..]),
+                "push" => git_push_is_dangerous(&command[subcommand_idx + 1..]),
+                "clean" => git_clean_is_force(&command[subcommand_idx + 1..]),
+                other => {
+                    debug_assert!(false, "unexpected git subcommand from matcher: {other}");
+                    false
+                }
+            }
         }
 
         Some("rm") => matches!(command.get(1).map(String::as_str), Some("-f" | "-rf")),
@@ -45,6 +133,48 @@ fn is_dangerous_to_call_with_exec(command: &[String]) -> bool {
     }
 }
 
+fn git_branch_is_delete(branch_args: &[String]) -> bool {
+    // Git allows stacking short flags (for example, `-dv` or `-vd`). Treat any
+    // short-flag group containing `d`/`D` as a delete flag.
+    branch_args.iter().map(String::as_str).any(|arg| {
+        matches!(arg, "-d" | "-D" | "--delete")
+            || arg.starts_with("--delete=")
+            || short_flag_group_contains(arg, 'd')
+            || short_flag_group_contains(arg, 'D')
+    })
+}
+
+fn short_flag_group_contains(arg: &str, target: char) -> bool {
+    arg.starts_with('-') && !arg.starts_with("--") && arg.chars().skip(1).any(|c| c == target)
+}
+
+fn git_push_is_dangerous(push_args: &[String]) -> bool {
+    push_args.iter().map(String::as_str).any(|arg| {
+        matches!(
+            arg,
+            "--force" | "--force-with-lease" | "--force-if-includes" | "--delete" | "-f" | "-d"
+        ) || arg.starts_with("--force-with-lease=")
+            || arg.starts_with("--force-if-includes=")
+            || arg.starts_with("--delete=")
+            || short_flag_group_contains(arg, 'f')
+            || short_flag_group_contains(arg, 'd')
+            || git_push_refspec_is_dangerous(arg)
+    })
+}
+
+fn git_push_refspec_is_dangerous(arg: &str) -> bool {
+    // `+<refspec>` forces updates and `:<dst>` deletes remote refs.
+    (arg.starts_with('+') || arg.starts_with(':')) && arg.len() > 1
+}
+
+fn git_clean_is_force(clean_args: &[String]) -> bool {
+    clean_args.iter().map(String::as_str).any(|arg| {
+        matches!(arg, "--force" | "-f")
+            || arg.starts_with("--force=")
+            || short_flag_group_contains(arg, 'f')
+    })
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -63,7 +193,7 @@ mod tests {
         assert!(command_might_be_dangerous(&vec_str(&[
             "bash",
             "-lc",
-            "git reset --hard"
+            "git reset --hard",
         ])));
     }
 
@@ -72,7 +202,7 @@ mod tests {
         assert!(command_might_be_dangerous(&vec_str(&[
             "zsh",
             "-lc",
-            "git reset --hard"
+            "git reset --hard",
         ])));
     }
 
@@ -86,14 +216,14 @@ mod tests {
         assert!(!command_might_be_dangerous(&vec_str(&[
             "bash",
             "-lc",
-            "git status"
+            "git status",
         ])));
     }
 
     #[test]
     fn sudo_git_reset_is_dangerous() {
         assert!(command_might_be_dangerous(&vec_str(&[
-            "sudo", "git", "reset", "--hard"
+            "sudo", "git", "reset", "--hard",
         ])));
     }
 
@@ -102,7 +232,141 @@ mod tests {
         assert!(command_might_be_dangerous(&vec_str(&[
             "/usr/bin/git",
             "reset",
-            "--hard"
+            "--hard",
+        ])));
+    }
+
+    #[test]
+    fn git_branch_delete_is_dangerous() {
+        assert!(command_might_be_dangerous(&vec_str(&[
+            "git", "branch", "-d", "feature",
+        ])));
+        assert!(command_might_be_dangerous(&vec_str(&[
+            "git", "branch", "-D", "feature",
+        ])));
+        assert!(command_might_be_dangerous(&vec_str(&[
+            "bash",
+            "-lc",
+            "git branch --delete feature",
+        ])));
+    }
+
+    #[test]
+    fn git_branch_delete_with_stacked_short_flags_is_dangerous() {
+        assert!(command_might_be_dangerous(&vec_str(&[
+            "git", "branch", "-dv", "feature",
+        ])));
+        assert!(command_might_be_dangerous(&vec_str(&[
+            "git", "branch", "-vd", "feature",
+        ])));
+        assert!(command_might_be_dangerous(&vec_str(&[
+            "git", "branch", "-vD", "feature",
+        ])));
+        assert!(command_might_be_dangerous(&vec_str(&[
+            "git", "branch", "-Dvv", "feature",
+        ])));
+    }
+
+    #[test]
+    fn git_branch_delete_with_global_options_is_dangerous() {
+        assert!(command_might_be_dangerous(&vec_str(&[
+            "git", "-C", ".", "branch", "-d", "feature",
+        ])));
+        assert!(command_might_be_dangerous(&vec_str(&[
+            "git",
+            "-c",
+            "color.ui=false",
+            "branch",
+            "-D",
+            "feature",
+        ])));
+        assert!(command_might_be_dangerous(&vec_str(&[
+            "bash",
+            "-lc",
+            "git -C . branch -d feature",
+        ])));
+    }
+
+    #[test]
+    fn git_checkout_reset_is_not_dangerous() {
+        // The first non-option token is "checkout", so later positional args
+        // like branch names must not be treated as subcommands.
+        assert!(!command_might_be_dangerous(&vec_str(&[
+            "git", "checkout", "reset",
+        ])));
+    }
+
+    #[test]
+    fn git_push_force_is_dangerous() {
+        assert!(command_might_be_dangerous(&vec_str(&[
+            "git", "push", "--force", "origin", "main",
+        ])));
+        assert!(command_might_be_dangerous(&vec_str(&[
+            "git", "push", "-f", "origin", "main",
+        ])));
+        assert!(command_might_be_dangerous(&vec_str(&[
+            "git",
+            "-C",
+            ".",
+            "push",
+            "--force-with-lease",
+            "origin",
+            "main",
+        ])));
+    }
+
+    #[test]
+    fn git_push_plus_refspec_is_dangerous() {
+        assert!(command_might_be_dangerous(&vec_str(&[
+            "git", "push", "origin", "+main",
+        ])));
+        assert!(command_might_be_dangerous(&vec_str(&[
+            "git",
+            "push",
+            "origin",
+            "+refs/heads/main:refs/heads/main",
+        ])));
+    }
+
+    #[test]
+    fn git_push_delete_flag_is_dangerous() {
+        assert!(command_might_be_dangerous(&vec_str(&[
+            "git", "push", "--delete", "origin", "feature",
+        ])));
+        assert!(command_might_be_dangerous(&vec_str(&[
+            "git", "push", "-d", "origin", "feature",
+        ])));
+    }
+
+    #[test]
+    fn git_push_delete_refspec_is_dangerous() {
+        assert!(command_might_be_dangerous(&vec_str(&[
+            "git", "push", "origin", ":feature",
+        ])));
+        assert!(command_might_be_dangerous(&vec_str(&[
+            "bash",
+            "-lc",
+            "git push origin :feature",
+        ])));
+    }
+
+    #[test]
+    fn git_push_without_force_is_not_dangerous() {
+        assert!(!command_might_be_dangerous(&vec_str(&[
+            "git", "push", "origin", "main",
+        ])));
+    }
+
+    #[test]
+    fn git_clean_force_is_dangerous_even_when_f_is_not_first_flag() {
+        assert!(command_might_be_dangerous(&vec_str(&[
+            "git", "clean", "-fdx",
+        ])));
+        assert!(command_might_be_dangerous(&vec_str(&[
+            "git", "clean", "-xdf",
+        ])));
+        assert!(command_might_be_dangerous(&vec_str(&[
+            "git", "clean", "--force",
         ])));
     }
 

codex-rs/core/src/command_safety/is_safe_command.rs

@@ -1,4 +1,8 @@
 use crate::bash::parse_shell_lc_plain_commands;
+// Find the first matching git subcommand, skipping known global options that
+// may appear before it (e.g., `-C`, `-c`, `--git-dir`).
+// Implemented in `is_dangerous_command` and shared here.
+use crate::command_safety::is_dangerous_command::find_git_subcommand;
 use crate::command_safety::windows_safe_commands::is_safe_command_windows;
 
 pub fn is_known_safe_command(command: &[String]) -> bool {
@@ -131,13 +135,36 @@ fn is_safe_to_call_with_exec(command: &[String]) -> bool {
         }
 
         // Git
-        Some("git") => matches!(
+        Some("git") => {
-            command.get(1).map(String::as_str),
+            // Global config overrides like `-c core.pager=...` can force git
-            Some("branch" | "status" | "log" | "diff" | "show")
+            // to execute arbitrary external commands. With no sandboxing, we
-        ),
+            // should always prompt in those cases.
+            if git_has_config_override_global_option(command) {
+                return false;
+            }
 
-        // Rust
+            let Some((subcommand_idx, subcommand)) =
-        Some("cargo") if command.get(1).map(String::as_str) == Some("check") => true,
+                find_git_subcommand(command, &["status", "log", "diff", "show", "branch"])
+            else {
+                return false;
+            };
+
+            let subcommand_args = &command[subcommand_idx + 1..];
+
+            match subcommand {
+                "status" | "log" | "diff" | "show" => {
+                    git_subcommand_args_are_read_only(subcommand_args)
+                }
+                "branch" => {
+                    git_subcommand_args_are_read_only(subcommand_args)
+                        && git_branch_is_read_only(subcommand_args)
+                }
+                other => {
+                    debug_assert!(false, "unexpected git subcommand from matcher: {other}");
+                    false
+                }
+            }
+        }
 
         // Special-case `sed -n {N|M,N}p`
         Some("sed")
@@ -155,6 +182,60 @@ fn is_safe_to_call_with_exec(command: &[String]) -> bool {
     }
 }
 
+// Treat `git branch` as safe only when the arguments clearly indicate
+// a read-only query, not a branch mutation (create/rename/delete).
+fn git_branch_is_read_only(branch_args: &[String]) -> bool {
+    if branch_args.is_empty() {
+        // `git branch` with no additional args lists branches.
+        return true;
+    }
+
+    let mut saw_read_only_flag = false;
+    for arg in branch_args.iter().map(String::as_str) {
+        match arg {
+            "--list" | "-l" | "--show-current" | "-a" | "--all" | "-r" | "--remotes" | "-v"
+            | "-vv" | "--verbose" => {
+                saw_read_only_flag = true;
+            }
+            _ if arg.starts_with("--format=") => {
+                saw_read_only_flag = true;
+            }
+            _ => {
+                // Any other flag or positional argument may create, rename, or delete branches.
+                return false;
+            }
+        }
+    }
+
+    saw_read_only_flag
+}
+
+fn git_has_config_override_global_option(command: &[String]) -> bool {
+    command.iter().map(String::as_str).any(|arg| {
+        matches!(arg, "-c" | "--config-env")
+            || (arg.starts_with("-c") && arg.len() > 2)
+            || arg.starts_with("--config-env=")
+    })
+}
+
+fn git_subcommand_args_are_read_only(args: &[String]) -> bool {
+    // Flags that can write to disk or execute external tools should never be
+    // auto-approved on an unsandboxed machine.
+    const UNSAFE_GIT_FLAGS: &[&str] = &[
+        "--output",
+        "--ext-diff",
+        "--textconv",
+        "--exec",
+        "--paginate",
+    ];
+
+    !args.iter().map(String::as_str).any(|arg| {
+        UNSAFE_GIT_FLAGS.contains(&arg)
+            || arg.starts_with("--output=")
+            || arg.starts_with("--exec=")
+    })
+}
+
 // (bash parsing helpers implemented in crate::bash)
 
 /* ----------------------------------------------------------
@@ -207,6 +288,12 @@ mod tests {
     fn known_safe_examples() {
         assert!(is_safe_to_call_with_exec(&vec_str(&["ls"])));
         assert!(is_safe_to_call_with_exec(&vec_str(&["git", "status"])));
+        assert!(is_safe_to_call_with_exec(&vec_str(&["git", "branch"])));
+        assert!(is_safe_to_call_with_exec(&vec_str(&[
+            "git",
+            "branch",
+            "--show-current"
+        ])));
         assert!(is_safe_to_call_with_exec(&vec_str(&["base64"])));
         assert!(is_safe_to_call_with_exec(&vec_str(&[
             "sed", "-n", "1,5p", "file.txt"
@@ -231,6 +318,86 @@ mod tests {
         }
     }
 
+    #[test]
+    fn git_branch_mutating_flags_are_not_safe() {
+        assert!(!is_known_safe_command(&vec_str(&[
+            "git", "branch", "-d", "feature"
+        ])));
+        assert!(!is_known_safe_command(&vec_str(&[
+            "git",
+            "branch",
+            "new-branch"
+        ])));
+    }
+
+    #[test]
+    fn git_branch_global_options_respect_safety_rules() {
+        use pretty_assertions::assert_eq;
+
+        assert_eq!(
+            is_known_safe_command(&vec_str(&["git", "-C", ".", "branch", "--show-current"])),
+            true
+        );
+        assert_eq!(
+            is_known_safe_command(&vec_str(&["git", "-C", ".", "branch", "-d", "feature"])),
+            false
+        );
+        assert_eq!(
+            is_known_safe_command(&vec_str(&["bash", "-lc", "git -C . branch -d feature",])),
+            false
+        );
+    }
+
+    #[test]
+    fn git_first_positional_is_the_subcommand() {
+        // In git, the first non-option token is the subcommand. Later positional
+        // args (like branch names) must not be treated as subcommands.
+        assert!(!is_known_safe_command(&vec_str(&[
+            "git", "checkout", "status",
+        ])));
+    }
+
+    #[test]
+    fn git_output_and_config_override_flags_are_not_safe() {
+        assert!(!is_known_safe_command(&vec_str(&[
+            "git",
+            "log",
+            "--output=/tmp/git-log-out-test",
+            "-n",
+            "1",
+        ])));
+        assert!(!is_known_safe_command(&vec_str(&[
+            "git",
+            "diff",
+            "--output",
+            "/tmp/git-diff-out-test",
+        ])));
+        assert!(!is_known_safe_command(&vec_str(&[
+            "git",
+            "show",
+            "--output=/tmp/git-show-out-test",
+            "HEAD",
+        ])));
+        assert!(!is_known_safe_command(&vec_str(&[
+            "git",
+            "-c",
+            "core.pager=cat",
+            "log",
+            "-n",
+            "1",
+        ])));
+        assert!(!is_known_safe_command(&vec_str(&[
+            "git",
+            "-ccore.pager=cat",
+            "status",
+        ])));
+    }
+
+    #[test]
+    fn cargo_check_is_not_safe() {
+        assert!(!is_known_safe_command(&vec_str(&["cargo", "check"])));
+    }
+
     #[test]
     fn zsh_lc_safe_command_sequence() {
         assert!(is_known_safe_command(&vec_str(&["zsh", "-lc", "ls"])));

codex-rs/core/src/exec_policy.rs

@@ -1280,6 +1280,30 @@ prefix_rule(
         );
     }
 
+    #[tokio::test]
+    async fn dangerous_git_push_requires_approval_in_danger_full_access() {
+        let command = vec_str(&["git", "push", "origin", "+main"]);
+        let manager = ExecPolicyManager::default();
+        let requirement = manager
+            .create_exec_approval_requirement_for_command(ExecApprovalRequest {
+                features: &Features::with_defaults(),
+                command: &command,
+                approval_policy: AskForApproval::OnRequest,
+                sandbox_policy: &SandboxPolicy::DangerFullAccess,
+                sandbox_permissions: SandboxPermissions::UseDefault,
+                prefix_rule: None,
+            })
+            .await;
+
+        assert_eq!(
+            requirement,
+            ExecApprovalRequirement::NeedsApproval {
+                reason: None,
+                proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(command)),
+            }
+        );
+    }
+
     fn vec_str(items: &[&str]) -> Vec<String> {
         items.iter().map(std::string::ToString::to_string).collect()
     }