openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-09-13 11:47:17 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: require approval for git commands

Browse Source
This commit is contained in:
Eva Wong
2026-06-08 10:06:02 -07:00
parent b128da272e
commit f020858eea
3 changed files with 18 additions and 431 deletions
Download Patch File Download Diff File Expand all files Collapse all files

73
codex-rs/shell-command/src/command_safety/is_dangerous_command.rs
View File

@@ -43,31 +43,6 @@ pub fn is_dangerous_powershell_words(command: &[String]) -> bool {
}
}
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)
}
pub(crate) fn executable_name_lookup_key(raw: &str) -> Option<String> {
#[cfg(windows)]
{
@@ -94,54 +69,6 @@ pub(crate) fn executable_name_lookup_key(raw: &str) -> Option<String> {
}
}
/// 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 executable_name_lookup_key(cmd0).as_deref() != Some("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);

284
codex-rs/shell-command/src/command_safety/is_safe_command.rs
View File

@@ -1,9 +1,5 @@
use crate::bash::parse_shell_lc_plain_commands;
use crate::command_safety::is_dangerous_command::executable_name_lookup_key;
// 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;
#[cfg(windows)]
use crate::command_safety::windows_safe_commands::is_safe_command_windows;
#[cfg(windows)]
@@ -153,9 +149,6 @@ fn is_safe_to_call_with_exec(command: &[String]) -> bool {
})
}
// Git
Some("git") => is_safe_git_command(command),
// Special-case `sed -n {N|M,N}p`
Some("sed")
if {
@@ -172,128 +165,6 @@ fn is_safe_to_call_with_exec(command: &[String]) -> bool {
}
}
pub(crate) fn is_safe_git_command(command: &[String]) -> bool {
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 {
"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
}
}
}
// 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
}
#[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()
.map(String::as_str)
.any(|arg| git_matches_option_pattern(arg, UNSAFE_GIT_GLOBAL_OPTIONS))
}
fn git_subcommand_args_are_read_only(args: &[String]) -> bool {
!args
.iter()
.map(String::as_str)
.any(|arg| git_matches_option_pattern(arg, UNSAFE_GIT_SUBCOMMAND_OPTIONS))
}
// (bash parsing helpers implemented in crate::bash)
/* ----------------------------------------------------------
@@ -345,13 +216,6 @@ mod tests {
#[test]
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"
@@ -377,20 +241,16 @@ mod tests {
}
#[test]
fn git_branch_mutating_flags_are_not_safe() {
fn git_commands_require_approval() {
assert!(!is_known_safe_command(&vec_str(&["git", "status"])));
assert!(!is_known_safe_command(&vec_str(&["git", "diff", "-p"])));
assert!(!is_known_safe_command(&vec_str(&[
"git", "branch", "-d", "feature"
"git", "log", "-p", "-1"
])));
assert!(!is_known_safe_command(&vec_str(&[
"git",
"branch",
"new-branch"
"git", "show", "-p", "HEAD",
])));
}
#[test]
fn git_branch_global_options_respect_safety_rules() {
assert!(is_known_safe_command(&vec_str(&[
assert!(!is_known_safe_command(&vec_str(&[
"git",
"branch",
"--show-current",
@@ -399,131 +259,13 @@ mod tests {
"git", "branch", "-d", "feature",
])));
assert!(!is_known_safe_command(&vec_str(&[
"bash",
"-lc",
"git branch -d feature",
])));
}
#[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_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",
])));
}
#[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(&[
"git", "-C", ".", "status",
])));
assert!(!is_known_safe_command(&vec_str(&["git", "-C.", "status",])));
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",
])));
for args in [
vec_str(&["git", "--config-env", "core.pager=PAGER", "show", "HEAD"]),
vec_str(&["git", "--config-env=core.pager=PAGER", "show", "HEAD"]),
vec_str(&["git", "--git-dir", ".evil-git", "diff", "HEAD~1..HEAD"]),
vec_str(&["git", "--git-dir=.evil-git", "diff", "HEAD~1..HEAD"]),
vec_str(&["git", "--work-tree", ".", "status"]),
vec_str(&["git", "--work-tree=.", "status"]),
vec_str(&["git", "--exec-path", ".git/helpers", "show", "HEAD"]),
vec_str(&["git", "--exec-path=.git/helpers", "show", "HEAD"]),
vec_str(&["git", "--namespace", "attacker", "show", "HEAD"]),
vec_str(&["git", "--namespace=attacker", "show", "HEAD"]),
vec_str(&["git", "--super-prefix", "attacker/", "show", "HEAD"]),
vec_str(&["git", "--super-prefix=attacker/", "show", "HEAD"]),
] {
assert!(
!is_known_safe_command(&args),
"expected {args:?} to require approval due to unsafe git global option",
);
}
assert!(!is_known_safe_command(&vec_str(&[
"bash",
"-lc",
"git -C .project-deps/test-fixtures status",
])));
assert!(!is_known_safe_command(&vec_str(&[
"bash",
"-lc",
"git --git-dir=.evil-git diff HEAD~1..HEAD",
])));
}
#[test]
@@ -636,12 +378,12 @@ mod tests {
}
#[test]
fn windows_git_full_path_is_safe() {
fn windows_git_full_path_status_requires_approval() {
if !cfg!(windows) {
return;
}
assert!(is_known_safe_command(&vec_str(&[
assert!(!is_known_safe_command(&vec_str(&[
r"C:\Program Files\Git\cmd\git.exe",
"status",
])));
@@ -651,11 +393,6 @@ mod tests {
fn bash_lc_safe_examples() {
assert!(is_known_safe_command(&vec_str(&["bash", "-lc", "ls"])));
assert!(is_known_safe_command(&vec_str(&["bash", "-lc", "ls -1"])));
assert!(is_known_safe_command(&vec_str(&[
"bash",
"-lc",
"git status"
])));
assert!(is_known_safe_command(&vec_str(&[
"bash",
"-lc",
@@ -705,6 +442,11 @@ mod tests {
#[test]
fn bash_lc_unsafe_examples() {
assert!(!is_known_safe_command(&vec_str(&[
"bash",
"-lc",
"git status"
])));
assert!(
!is_known_safe_command(&vec_str(&["bash", "-lc", "git", "status"])),
"Four arg version is not known to be safe."

92
codex-rs/shell-command/src/command_safety/windows_safe_commands.rs
View File

@@ -1,4 +1,3 @@
use crate::command_safety::is_safe_command::is_safe_git_command;
use crate::command_safety::powershell_parser::PowershellParseOutcome;
use crate::command_safety::powershell_parser::parse_with_powershell_ast;
use std::path::Path;
@@ -188,7 +187,7 @@ pub(crate) fn is_safe_powershell_words(words: &[String]) -> bool {
"select-object" | "select" => true,
"get-item" => true,
"git" => is_safe_git_command(words),
"git" => false,
"rg" => is_safe_ripgrep(words),
@@ -225,7 +224,6 @@ fn is_safe_ripgrep(words: &[String]) -> bool {
mod tests {
use super::*;
use crate::powershell::try_find_pwsh_executable_blocking;
use pretty_assertions::assert_eq;
use std::string::ToString;
/// Converts a slice of string literals into owned `String`s for the tests.
@@ -234,7 +232,7 @@ mod tests {
}
#[test]
fn recognizes_safe_powershell_wrappers() {
fn classifies_powershell_wrappers() {
assert!(is_safe_command_windows(&vec_str(&[
"powershell.exe",
"-NoLogo",
@@ -242,7 +240,7 @@ mod tests {
"Get-ChildItem -Path .",
])));
assert!(is_safe_command_windows(&vec_str(&[
assert!(!is_safe_command_windows(&vec_str(&[
"powershell.exe",
"-NoProfile",
"-Command",
@@ -290,7 +288,7 @@ mod tests {
}
#[test]
fn allows_read_only_pipelines_and_git_usage() {
fn allows_read_only_pipelines() {
let Some(pwsh) = try_find_pwsh_executable_blocking() else {
return;
};
@@ -313,7 +311,7 @@ mod tests {
"Get-Content foo.rs | Select-Object -Skip 200".to_string()
]));
assert!(is_safe_command_windows(&[
assert!(!is_safe_command_windows(&[
pwsh.clone(),
"-Command".to_string(),
"git show HEAD:foo.rs".to_string()
@@ -332,86 +330,6 @@ mod tests {
]));
}
#[test]
fn rejects_git_global_override_options() {
let Some(pwsh) = try_find_pwsh_executable_blocking() else {
return;
};
let pwsh: String = pwsh.as_path().to_str().unwrap().into();
for script in [
"git -c core.pager=cat show HEAD:foo.rs",
"git --config-env core.pager=PAGER show HEAD:foo.rs",
"git --config-env=core.pager=PAGER show HEAD:foo.rs",
"git --git-dir .evil-git diff HEAD~1..HEAD",
"git --git-dir=.evil-git diff HEAD~1..HEAD",
"git --work-tree . status",
"git --work-tree=. status",
"git --exec-path .git/helpers show HEAD:foo.rs",
"git --exec-path=.git/helpers show HEAD:foo.rs",
"git --namespace attacker show HEAD:foo.rs",
"git --namespace=attacker show HEAD:foo.rs",
"git --super-prefix attacker/ show HEAD:foo.rs",
"git --super-prefix=attacker/ show HEAD:foo.rs",
] {
assert!(
!is_safe_command_windows(&[
pwsh.clone(),
"-NoLogo".to_string(),
"-NoProfile".to_string(),
"-Command".to_string(),
script.to_string(),
]),
"expected {script:?} to require approval due to unsafe git global option",
);
}
}
#[test]
fn rejects_git_subcommand_options_with_side_effects() {
let results: Vec<(&str, bool)> = [
"git diff --output codex_poc.txt",
"git diff --ext-diff HEAD",
"git log --textconv -1",
"git show --output=codex_poc.txt HEAD",
"git cat-file --filters HEAD:a.txt",
]
.into_iter()
.map(|script| {
(
script,
is_safe_command_windows(&[
"powershell.exe".to_string(),
"-NoProfile".to_string(),
"-Command".to_string(),
script.to_string(),
]),
)
})
.collect();
assert_eq!(
vec![
("git diff --output codex_poc.txt", false),
("git diff --ext-diff HEAD", false),
("git log --textconv -1", false),
("git show --output=codex_poc.txt HEAD", false),
("git cat-file --filters HEAD:a.txt", false),
],
results
);
}
#[test]
fn rejects_stop_parsing_git_forms() {
assert!(!is_safe_command_windows(&vec_str(&[
"powershell.exe",
"-NoProfile",
"-Command",
"git log --% HEAD --output=codex_poc.txt",
])));
}
#[test]
fn rejects_powershell_commands_with_side_effects() {
assert!(!is_safe_command_windows(&vec_str(&[