[codex] Handle git pagination flags by position (#21381)

## Why

This is a follow-up to the Windows Git safe-command bypass fix for
BUGB-15601. Git's global `--paginate` / `-p` flags can route output
through a configured pager, so they should not be auto-approved as safe
before the subcommand. At the same time, `-p` after read-only
subcommands like `log`, `diff`, and `show` is the common patch-output
flag, so treating every `-p` as unsafe would make ordinary read-only
inspection commands prompt unnecessarily.

## What Changed

- Split Git option safety matching into explicit global-option and
subcommand-option lists.
- Treat global `git --paginate ...` and `git -p ...` as unsafe.
- Keep post-subcommand patch usage such as `git log -p`, `git diff -p`,
and `git show -p HEAD` safe.
- Keep the pagination coverage with the shared Git safe-command
implementation rather than the Windows wrapper tests.
- Remove the stale `git_global_option_requires_prompt` helper now that
safe-command Git option matching owns the prompt-required lists.

## Testing

- `cargo test -p codex-shell-command`

codex-rs/shell-command/src/command_safety/is_dangerous_command.rs

@@ -68,32 +68,6 @@ fn is_git_global_option_with_inline_value(arg: &str) -> bool {
     ) || ((arg.starts_with("-C") || arg.starts_with("-c")) && arg.len() > 2)
 }
 
-/// Git global options that can redirect config, repository, or helper lookup
-/// and therefore must never be auto-approved as "safe".
-pub(crate) fn git_global_option_requires_prompt(arg: &str) -> bool {
-    matches!(
-        arg,
-        // `-C` can redirect Git into a repo whose config runs helpers such as
-        // `core.fsmonitor` during read-only commands like `status`.
-        "-C" | "-c"
-            | "--config-env"
-            | "--exec-path"
-            | "--git-dir"
-            | "--namespace"
-            | "--super-prefix"
-            | "--work-tree"
-    ) || matches!(
-        arg,
-        s if ((s.starts_with("-C") || s.starts_with("-c")) && s.len() > 2)
-            || 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=")
-    )
-}
-
 pub(crate) fn executable_name_lookup_key(raw: &str) -> Option<String> {
     #[cfg(windows)]
     {
@@ -200,12 +174,6 @@ mod tests {
         assert!(command_might_be_dangerous(&vec_str(&["rm", "-f", "/"])));
     }
 
-    #[test]
-    fn git_dash_c_requires_prompt() {
-        assert!(git_global_option_requires_prompt("-C"));
-        assert!(git_global_option_requires_prompt("-C/path/to/repo"));
-    }
-
     #[test]
     fn direct_powershell_words_reuse_windows_dangerous_detection() {
         let command = vec_str(&["Remove-Item", "test", "-Force"]);

codex-rs/shell-command/src/command_safety/is_safe_command.rs

@@ -4,7 +4,6 @@ use crate::command_safety::is_dangerous_command::executable_name_lookup_key;
 // 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::is_dangerous_command::git_global_option_requires_prompt;
 use crate::command_safety::windows_safe_commands::is_safe_command_windows;
 #[cfg(windows)]
 use crate::command_safety::windows_safe_commands::is_safe_powershell_words as is_safe_powershell_words_windows;
@@ -170,19 +169,17 @@ fn is_safe_to_call_with_exec(command: &[String]) -> bool {
 }
 
 pub(crate) fn is_safe_git_command(command: &[String]) -> bool {
-    // Global options that redirect config, repository, or helper lookup can make
-    // otherwise read-only git commands execute attacker-controlled code, so they
-    // must never be auto-approved.
-    if git_has_unsafe_global_option(command) {
-        return false;
-    }
-
     let Some((subcommand_idx, subcommand)) =
         find_git_subcommand(command, &["status", "log", "diff", "show", "branch"])
     else {
         return false;
     };
 
+    let global_args = &command[1..subcommand_idx];
+    if git_has_unsafe_global_option(global_args) {
+        return false;
+    }
+
     let subcommand_args = &command[subcommand_idx + 1..];
 
     match subcommand {
@@ -226,30 +223,71 @@ fn git_branch_is_read_only(branch_args: &[String]) -> bool {
     saw_read_only_flag
 }
 
-fn git_has_unsafe_global_option(command: &[String]) -> bool {
-    command
+#[derive(Clone, Copy)]
+enum GitOptionPattern {
+    Exact(&'static str),
+    ShortWithInlineValue(&'static str),
+    Prefix(&'static str),
+}
+
+const UNSAFE_GIT_GLOBAL_OPTIONS: &[GitOptionPattern] = &[
+    GitOptionPattern::Exact("-C"),
+    GitOptionPattern::ShortWithInlineValue("-C"),
+    GitOptionPattern::Exact("-c"),
+    GitOptionPattern::ShortWithInlineValue("-c"),
+    GitOptionPattern::Exact("-p"),
+    GitOptionPattern::Exact("--config-env"),
+    GitOptionPattern::Prefix("--config-env="),
+    GitOptionPattern::Exact("--exec-path"),
+    GitOptionPattern::Prefix("--exec-path="),
+    GitOptionPattern::Exact("--git-dir"),
+    GitOptionPattern::Prefix("--git-dir="),
+    GitOptionPattern::Exact("--namespace"),
+    GitOptionPattern::Prefix("--namespace="),
+    GitOptionPattern::Exact("--paginate"),
+    GitOptionPattern::Exact("--super-prefix"),
+    GitOptionPattern::Prefix("--super-prefix="),
+    GitOptionPattern::Exact("--work-tree"),
+    GitOptionPattern::Prefix("--work-tree="),
+];
+
+const UNSAFE_GIT_SUBCOMMAND_OPTIONS: &[GitOptionPattern] = &[
+    GitOptionPattern::Exact("--output"),
+    GitOptionPattern::Prefix("--output="),
+    GitOptionPattern::Exact("--ext-diff"),
+    GitOptionPattern::Exact("--textconv"),
+    GitOptionPattern::Exact("--exec"),
+    GitOptionPattern::Prefix("--exec="),
+];
+
+impl GitOptionPattern {
+    fn matches(self, arg: &str) -> bool {
+        match self {
+            GitOptionPattern::Exact(option) => arg == option,
+            GitOptionPattern::ShortWithInlineValue(option) => {
+                arg.starts_with(option) && arg.len() > option.len()
+            }
+            GitOptionPattern::Prefix(prefix) => arg.starts_with(prefix),
+        }
+    }
+}
+
+fn git_matches_option_pattern(arg: &str, patterns: &[GitOptionPattern]) -> bool {
+    patterns.iter().any(|pattern| pattern.matches(arg))
+}
+
+fn git_has_unsafe_global_option(global_args: &[String]) -> bool {
+    global_args
         .iter()
-        .skip(1)
         .map(String::as_str)
-        .any(git_global_option_requires_prompt)
+        .any(|arg| git_matches_option_pattern(arg, UNSAFE_GIT_GLOBAL_OPTIONS))
 }
 
 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=")
-    })
+    !args
+        .iter()
+        .map(String::as_str)
+        .any(|arg| git_matches_option_pattern(arg, UNSAFE_GIT_SUBCOMMAND_OPTIONS))
 }
 
 // (bash parsing helpers implemented in crate::bash)
@@ -395,6 +433,43 @@ mod tests {
         ])));
     }
 
+    #[test]
+    fn git_global_pagination_flags_are_not_safe() {
+        assert!(!is_known_safe_command(&vec_str(&[
+            "git",
+            "--paginate",
+            "log",
+            "-1",
+        ])));
+        assert!(!is_known_safe_command(&vec_str(&[
+            "git", "-p", "log", "-1",
+        ])));
+        assert!(!is_known_safe_command(&vec_str(&[
+            "bash",
+            "-lc",
+            "git --paginate log -1",
+        ])));
+        assert!(!is_known_safe_command(&vec_str(&[
+            "bash",
+            "-lc",
+            "git -p log -1",
+        ])));
+    }
+
+    #[test]
+    fn git_subcommand_patch_flags_remain_safe() {
+        assert!(is_known_safe_command(&vec_str(&["git", "log", "-p", "-1"])));
+        assert!(is_known_safe_command(&vec_str(&["git", "diff", "-p"])));
+        assert!(is_known_safe_command(&vec_str(&[
+            "git", "show", "-p", "HEAD",
+        ])));
+        assert!(is_known_safe_command(&vec_str(&[
+            "bash",
+            "-lc",
+            "git log -p -1",
+        ])));
+    }
+
     #[test]
     fn git_global_override_flags_are_not_safe() {
         assert!(!is_known_safe_command(&vec_str(&[

codex-rs/shell-command/src/command_safety/windows_safe_commands.rs

@@ -373,7 +373,6 @@ mod tests {
             "git diff --output codex_poc.txt",
             "git diff --ext-diff HEAD",
             "git log --textconv -1",
-            "git log --paginate -1",
             "git show --output=codex_poc.txt HEAD",
             "git cat-file --filters HEAD:a.txt",
         ]
@@ -396,7 +395,6 @@ mod tests {
                 ("git diff --output codex_poc.txt", false),
                 ("git diff --ext-diff HEAD", false),
                 ("git log --textconv -1", false),
-                ("git log --paginate -1", false),
                 ("git show --output=codex_poc.txt HEAD", false),
                 ("git cat-file --filters HEAD:a.txt", false),
             ],