From f32c496144f3ae4617e401f8dbbbe337a363057c Mon Sep 17 00:00:00 2001 From: iceweasel-oai Date: Wed, 6 May 2026 11:53:26 -0700 Subject: [PATCH] [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` --- .../command_safety/is_dangerous_command.rs | 32 ----- .../src/command_safety/is_safe_command.rs | 129 ++++++++++++++---- .../command_safety/windows_safe_commands.rs | 2 - 3 files changed, 102 insertions(+), 61 deletions(-) diff --git a/codex-rs/shell-command/src/command_safety/is_dangerous_command.rs b/codex-rs/shell-command/src/command_safety/is_dangerous_command.rs index 19babd82e..fef98e836 100644 --- a/codex-rs/shell-command/src/command_safety/is_dangerous_command.rs +++ b/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 { #[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"]); diff --git a/codex-rs/shell-command/src/command_safety/is_safe_command.rs b/codex-rs/shell-command/src/command_safety/is_safe_command.rs index 7200672ec..b35144a12 100644 --- a/codex-rs/shell-command/src/command_safety/is_safe_command.rs +++ b/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(&[ diff --git a/codex-rs/shell-command/src/command_safety/windows_safe_commands.rs b/codex-rs/shell-command/src/command_safety/windows_safe_commands.rs index ebe0058f7..8ef3f8e8f 100644 --- a/codex-rs/shell-command/src/command_safety/windows_safe_commands.rs +++ b/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), ],