openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-21 17:48:44 +00:00
Code Issues Packages Projects Releases Wiki Activity

Require approval for generic Git commands

Browse Source
This commit is contained in:
Chris Bookholt
2026-06-29 15:23:34 -07:00
parent 5019886cb2
commit 87d89bec88
4 changed files with 80 additions and 237 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/shell-command/src/command_safety/is_dangerous_command.rs
-73
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);
codex-rs/shell-command/src/command_safety/is_safe_command.rs
+14 -141
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)]
@@ -172,131 +168,11 @@ 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" => {
// These subcommands can consult repository-controlled configuration
// for external helpers, so argv-only classification is not enough to
// auto-approve them.
false
}
"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))
pub(crate) fn is_safe_git_command(_command: &[String]) -> bool {
// Git behavior depends on repository config, attributes, the discovered
// repository, environment, and TTY state. This argv-only classifier cannot
// prove that any Git command avoids repository-selected executables.
false
}
// (bash parsing helpers implemented in crate::bash)
@@ -350,12 +226,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", "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"
@@ -381,20 +251,23 @@ mod tests {
}
#[test]
fn git_config_sensitive_subcommands_require_approval() {
fn git_commands_require_approval() {
for args in [
vec_str(&["git", "status"]),
vec_str(&["git", "log", "-1"]),
vec_str(&["git", "diff"]),
vec_str(&["git", "show", "HEAD"]),
vec_str(&["git", "branch"]),
vec_str(&["git", "branch", "--show-current"]),
vec_str(&["bash", "-lc", "git status"]),
vec_str(&["bash", "-lc", "git log -1"]),
vec_str(&["bash", "-lc", "git diff"]),
vec_str(&["bash", "-lc", "git show HEAD"]),
vec_str(&["bash", "-lc", "git branch"]),
] {
assert!(
!is_known_safe_command(&args),
"expected {args:?} to require approval because git may invoke repository-configured helpers",
"expected {args:?} to require approval because Git may invoke repository-configured helpers",
);
}
}
@@ -412,8 +285,8 @@ mod tests {
}
#[test]
fn git_branch_global_options_respect_safety_rules() {
assert!(is_known_safe_command(&vec_str(&[
fn git_branch_read_only_flags_still_require_approval() {
assert!(!is_known_safe_command(&vec_str(&[
"git",
"branch",
"--show-current",
@@ -661,12 +534,12 @@ mod tests {
}
#[test]
fn windows_git_full_path_is_safe() {
fn windows_git_full_path_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",
])));
codex-rs/shell-command/src/command_safety/windows_safe_commands.rs
+24 -21
View File
@@ -283,7 +283,7 @@ mod tests {
}
#[test]
fn allows_read_only_pipelines_and_git_branch_usage() {
fn allows_read_only_pipelines() {
let Some(pwsh) = try_find_pwsh_executable_blocking() else {
return;
};
@@ -306,12 +306,6 @@ mod tests {
"Get-Content foo.rs | Select-Object -Skip 200".to_string()
]));
assert!(is_safe_command_windows(&[
pwsh.clone(),
"-Command".to_string(),
"git branch --show-current".to_string()
]));
assert!(is_safe_command_windows(&[
pwsh.clone(),
"-Command".to_string(),
@@ -327,20 +321,27 @@ mod tests {
#[test]
fn rejects_git_config_sensitive_subcommands() {
let results: Vec<(&str, bool)> = ["git status", "git log -1", "git diff", "git show HEAD"]
.into_iter()
.map(|script| {
(
script,
is_safe_command_windows(&[
"powershell.exe".to_string(),
"-NoProfile".to_string(),
"-Command".to_string(),
script.to_string(),
]),
)
})
.collect();
let results: Vec<(&str, bool)> = [
"git status",
"git log -1",
"git diff",
"git show HEAD",
"git branch",
"git branch --show-current",
]
.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![
@@ -348,6 +349,8 @@ mod tests {
("git log -1", false),
("git diff", false),
("git show HEAD", false),
("git branch", false),
("git branch --show-current", false),
],
results
);