diff --git a/codex-rs/core/src/exec_policy_tests.rs b/codex-rs/core/src/exec_policy_tests.rs index cf17387ef5..b6b6bcd23d 100644 --- a/codex-rs/core/src/exec_policy_tests.rs +++ b/codex-rs/core/src/exec_policy_tests.rs @@ -1017,8 +1017,8 @@ prefix_rule(pattern=["git"], decision="prompt") sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }, - ExecApprovalRequirement::Skip { - bypass_sandbox: false, + ExecApprovalRequirement::NeedsApproval { + reason: None, proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec![ disallowed_git_path, "status".to_string(), @@ -1138,6 +1138,46 @@ fn known_safe_on_request_still_prompts_for_restricted_sandbox_escalation() { ); } +#[test] +fn git_status_requires_approval_for_untrusted_projects() { + let command = vec!["git".to_string(), "status".to_string()]; + + assert_eq!( + Decision::Prompt, + render_decision_for_unmatched_command( + &command, + UnmatchedCommandContext { + approval_policy: AskForApproval::UnlessTrusted, + permission_profile: &PermissionProfile::workspace_write(), + windows_sandbox_level: WindowsSandboxLevel::Disabled, + sandbox_permissions: SandboxPermissions::UseDefault, + used_complex_parsing: false, + command_origin: ExecPolicyCommandOrigin::Generic, + }, + ) + ); +} + +#[test] +fn git_branch_requires_approval_for_untrusted_projects() { + let command = vec!["git".to_string(), "branch".to_string()]; + + assert_eq!( + Decision::Prompt, + render_decision_for_unmatched_command( + &command, + UnmatchedCommandContext { + approval_policy: AskForApproval::UnlessTrusted, + permission_profile: &PermissionProfile::workspace_write(), + windows_sandbox_level: WindowsSandboxLevel::Disabled, + sandbox_permissions: SandboxPermissions::UseDefault, + used_complex_parsing: false, + command_origin: ExecPolicyCommandOrigin::Generic, + }, + ) + ); +} + #[test] fn managed_cwd_write_profile_has_filesystem_restrictions() { let file_system_sandbox_policy = FileSystemSandboxPolicy::restricted(vec![ 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 fef98e8362..7a8960612f 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 @@ -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 { #[cfg(windows)] { @@ -94,54 +69,6 @@ pub(crate) fn executable_name_lookup_key(raw: &str) -> Option { } } -/// 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); 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 13b249e8b3..d4431e9a47 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 @@ -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", ]))); 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 134bdae609..ec22adf7cb 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 @@ -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 );