diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index 28974f43de..f0c734150f 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -337,7 +337,7 @@ impl ExecPolicyManager { }, None => ExecApprovalRequirement::NeedsApproval { reason: derive_prompt_reason(command, &evaluation), - proposed_execpolicy_amendment: suppress_repo_sensitive_git_amendment( + proposed_execpolicy_amendment: suppress_non_durable_execpolicy_amendment( requested_amendment.or_else(|| { if auto_amendment_allowed { try_derive_execpolicy_amendment_for_prompt_rules( @@ -366,7 +366,7 @@ impl ExecPolicyManager { is_policy_match(rule_match) && rule_match.decision() == Decision::Allow }) }), - proposed_execpolicy_amendment: suppress_repo_sensitive_git_amendment( + proposed_execpolicy_amendment: suppress_non_durable_execpolicy_amendment( if auto_amendment_allowed { try_derive_execpolicy_amendment_for_allow_rules(&evaluation.matched_rules) } else { @@ -800,6 +800,25 @@ fn commands_for_exec_policy(command: &[String]) -> ExecPolicyCommands { } } +/// Return one plain generic Git command when the request can be lowered +/// without ambiguity. +/// +/// Zsh-fork uses this to scope a parent approval to the exact intercepted Git +/// child. Multiple commands, complex parsing, wrappers such as `env`, and +/// PowerShell-flavored commands deliberately do not qualify. +pub(crate) fn single_plain_git_command(command: &[String]) -> Option> { + let parsed = commands_for_exec_policy(command); + if parsed.used_complex_parsing + || parsed.command_origin != ExecPolicyCommandOrigin::Generic + || parsed.commands.len() != 1 + { + return None; + } + + let command = parsed.commands.into_iter().next()?; + starts_with_git_executable(&command).then_some(command) +} + /// Derive a proposed execpolicy amendment when a command requires user approval /// - If any execpolicy rule prompts, return None, because an amendment would not skip that policy requirement. /// - Otherwise return the first heuristics Prompt. @@ -854,22 +873,32 @@ fn try_derive_execpolicy_amendment_for_allow_rules( /// Generic Git safety depends on the repository discovered at execution time, /// so an approval for one checkout must not become a durable allow rule that -/// silently applies to another checkout. Explicit user-authored policy rules -/// still take precedence during evaluation; this only removes amendments that -/// Codex would otherwise offer to persist from a runtime approval. -fn suppress_repo_sensitive_git_amendment( +/// silently applies to another checkout. Generic command delegators are also +/// context-sensitive and can hide Git or another executable behind the same +/// persisted prefix. Explicit user-authored policy rules still take precedence +/// during evaluation; this only removes amendments that Codex would otherwise +/// offer to persist from a runtime approval. +fn suppress_non_durable_execpolicy_amendment( amendment: Option, ) -> Option { - amendment.filter(|amendment| !starts_with_git_executable(&amendment.command)) + amendment.filter(|amendment| !starts_with_non_durable_executable(&amendment.command)) } fn starts_with_git_executable(command: &[String]) -> bool { - let Some(executable_name) = command - .first() - .and_then(|executable| executable.rsplit(['/', '\\']).next()) - else { - return false; - }; + starts_with_executable_named(command, "git") +} + +fn starts_with_non_durable_executable(command: &[String]) -> bool { + normalized_executable_name(command) + .is_some_and(|name| matches!(name.as_str(), "git" | "env" | "sudo" | "command" | "nice")) +} + +fn starts_with_executable_named(command: &[String], expected: &str) -> bool { + normalized_executable_name(command).is_some_and(|name| name == expected) +} + +fn normalized_executable_name(command: &[String]) -> Option { + let executable_name = command.first()?.rsplit(['/', '\\']).next()?; let executable_name = executable_name .as_bytes() .get(..2) @@ -880,7 +909,7 @@ fn starts_with_git_executable(command: &[String]) -> bool { .into_iter() .find_map(|suffix| executable_name.strip_suffix(suffix)) .unwrap_or(&executable_name); - executable_name == "git" + Some(executable_name.to_string()) } fn derive_requested_execpolicy_amendment_from_prefix_rule( diff --git a/codex-rs/core/src/exec_policy_tests.rs b/codex-rs/core/src/exec_policy_tests.rs index 6b289c97ff..ed76083beb 100644 --- a/codex-rs/core/src/exec_policy_tests.rs +++ b/codex-rs/core/src/exec_policy_tests.rs @@ -1231,6 +1231,54 @@ fn git_executable_detection_is_platform_independent() { } } +#[test] +fn non_durable_wrapper_detection_is_platform_independent() { + for executable in [ + "env", + "/usr/bin/env", + r"C:\Windows\System32\ENV.EXE", + r"C:env.cmd", + "/usr/bin/sudo", + r"C:\tools\COMMAND.BAT", + "/usr/bin/nice", + ] { + assert!( + starts_with_non_durable_executable(&[executable.to_string()]), + "expected {executable:?} to be non-durable" + ); + } + + for executable in [ + "envy", + "/tmp/env.exe.old", + "/usr/bin/env/", + r"C:notenv.exe", + "commander", + "nicety", + ] { + assert!( + !starts_with_non_durable_executable(&[executable.to_string()]), + "expected {executable:?} to remain eligible for normal amendment handling" + ); + } +} + +#[test] +fn single_plain_git_command_rejects_wrappers_and_multiple_commands() { + assert_eq!( + single_plain_git_command(&vec_str(&["/bin/zsh", "-lc", "git status --short"])), + Some(vec_str(&["git", "status", "--short"])) + ); + assert_eq!( + single_plain_git_command(&vec_str(&["/bin/zsh", "-lc", "env git status"])), + None + ); + assert_eq!( + single_plain_git_command(&vec_str(&["/bin/zsh", "-lc", "git status && git diff",])), + None + ); +} + #[tokio::test] async fn generic_git_policy_matrix_preserves_sandbox_and_escalation_semantics() { let command = vec!["git".to_string(), "status".to_string()]; @@ -1327,28 +1375,42 @@ async fn generic_git_one_shot_approval_does_not_create_cross_repo_cache() { } #[tokio::test] -async fn generic_git_never_offers_requested_or_shell_lowered_durable_amendments() { +async fn non_durable_commands_never_offer_generated_durable_amendments() { for (command, prefix_rule) in [ ( - vec!["git".to_string(), "status".to_string()], - Some(vec!["git".to_string(), "status".to_string()]), + vec_str(&["git", "status"]), + Some(vec_str(&["git", "status"])), ), + (vec_str(&["bash", "-lc", "git status"]), None), + (vec_str(&["/usr/bin/git", "status"]), None), ( - vec![ - "bash".to_string(), - "-lc".to_string(), - "git status".to_string(), - ], + vec_str(&[r"C:\Program Files\Git\cmd\GIT.EXE", "status"]), None, ), - (vec!["/usr/bin/git".to_string(), "status".to_string()], None), + (vec_str(&["env", "git", "status"]), None), + (vec_str(&["/usr/bin/env", "-i", "git", "status"]), None), + (vec_str(&["env", "--", "git", "status"]), None), ( - vec![ - r"C:\Program Files\Git\cmd\GIT.EXE".to_string(), - "status".to_string(), - ], + vec_str(&["env", "-u", "GIT_CONFIG_SYSTEM", "git", "status"]), None, ), + ( + vec_str(&["env", "GIT_OPTIONAL_LOCKS=0", "git", "status"]), + Some(vec_str(&["env", "GIT_OPTIONAL_LOCKS=0"])), + ), + (vec_str(&["env", "-S", "git status"]), None), + (vec_str(&["env", "FOO=1", "cargo", "check"]), None), + ( + vec_str(&[ + r"C:\Windows\System32\ENV.EXE", + r"C:\Program Files\Git\cmd\git.exe", + "status", + ]), + None, + ), + (vec_str(&["sudo", "git", "status"]), None), + (vec_str(&["command", "git", "status"]), None), + (vec_str(&["nice", "git", "status"]), None), ] { assert_exec_approval_requirement_for_command( ExecApprovalRequirementScenario { @@ -1368,6 +1430,60 @@ async fn generic_git_never_offers_requested_or_shell_lowered_durable_amendments( } } +#[tokio::test] +async fn env_wrappers_suppress_sandbox_fallback_amendments() { + for (sandbox_permissions, expected) in [ + ( + SandboxPermissions::UseDefault, + ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: None, + }, + ), + ( + SandboxPermissions::RequireEscalated, + ExecApprovalRequirement::NeedsApproval { + reason: None, + proposed_execpolicy_amendment: None, + }, + ), + ] { + assert_exec_approval_requirement_for_command( + ExecApprovalRequirementScenario { + policy_src: None, + command: vec_str(&["/usr/bin/env", "GIT_OPTIONAL_LOCKS=0", "git", "status"]), + approval_policy: AskForApproval::OnRequest, + permission_profile: PermissionProfile::workspace_write(), + sandbox_permissions, + prefix_rule: None, + }, + expected, + ) + .await; + } +} + +#[tokio::test] +async fn explicit_user_authored_env_git_rule_still_allows_matching_commands() { + assert_exec_approval_requirement_for_command( + ExecApprovalRequirementScenario { + policy_src: Some( + r#"prefix_rule(pattern=["env", "git", "status"], decision="allow")"#.to_string(), + ), + command: vec_str(&["env", "git", "status"]), + approval_policy: AskForApproval::UnlessTrusted, + permission_profile: PermissionProfile::workspace_write(), + sandbox_permissions: SandboxPermissions::UseDefault, + prefix_rule: None, + }, + ExecApprovalRequirement::Skip { + bypass_sandbox: true, + proposed_execpolicy_amendment: None, + }, + ) + .await; +} + #[tokio::test] async fn explicit_user_authored_git_policy_rule_still_allows_matching_commands() { for command in [ diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 0d386103db..bf8eecc23d 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -5,6 +5,8 @@ Executes shell requests under the orchestrator: asks for approval when needed, builds sandbox transform inputs, and runs them under the current SandboxAttempt. */ #[cfg(unix)] +mod trusted_executable; +#[cfg(unix)] pub(crate) mod unix_escalation; pub(crate) mod zsh_fork_backend; diff --git a/codex-rs/core/src/tools/runtimes/shell/trusted_executable.rs b/codex-rs/core/src/tools/runtimes/shell/trusted_executable.rs new file mode 100644 index 0000000000..5c62e580ff --- /dev/null +++ b/codex-rs/core/src/tools/runtimes/shell/trusted_executable.rs @@ -0,0 +1,198 @@ +//! Trust checks for executable paths reported by the zsh execve interceptor. +//! +//! Policy matching, reporting, and execution keep the resolved absolute path. +//! These helpers only recover a bare executable name for unmatched-command +//! classification after proving that the original request used that bare name +//! and the resolved host path cannot be replaced by the agent. + +use crate::sandboxing::SandboxPermissions; +use crate::tools::sandboxing::ExecApprovalRequirement; +use codex_protocol::models::AdditionalPermissionProfile; +use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::protocol::AskForApproval; +use codex_utils_absolute_path::AbsolutePathBuf; +use std::collections::HashMap; +use std::ffi::CString; +use std::os::unix::ffi::OsStrExt; +use std::path::Path; +use std::path::PathBuf; +use std::sync::Mutex; + +#[derive(Clone, Debug, Eq, PartialEq)] +pub(super) struct TrustedExecutableDir { + path: PathBuf, + canonical_path: PathBuf, +} + +/// One parent-approved command that may suppress one redundant intercepted +/// child prompt in the same tool invocation. +/// +/// Matching re-runs the trusted-path proof and compares the complete normalized +/// argv. The value is consumed on success and is never persisted or reused. +#[derive(Debug)] +pub(super) struct ParentApprovedIntercept { + command: Mutex>>, +} + +impl ParentApprovedIntercept { + fn new(command: Vec) -> Self { + Self { + command: Mutex::new(Some(command)), + } + } + + pub(super) fn for_parent_git_approval( + command: &[String], + exec_approval_requirement: &ExecApprovalRequirement, + approval_policy: AskForApproval, + sandbox_permissions: SandboxPermissions, + additional_permissions: Option<&AdditionalPermissionProfile>, + ) -> Option { + if approval_policy != AskForApproval::UnlessTrusted + || sandbox_permissions != SandboxPermissions::UseDefault + || additional_permissions.is_some() + || !matches!( + exec_approval_requirement, + ExecApprovalRequirement::NeedsApproval { reason: None, .. } + ) + { + return None; + } + + crate::exec_policy::single_plain_git_command(command).map(Self::new) + } + + pub(super) fn consume_if_matches( + &self, + program: &AbsolutePathBuf, + argv: &[String], + trusted_executable_dirs: &[TrustedExecutableDir], + file_system_sandbox_policy: &FileSystemSandboxPolicy, + cwd: &AbsolutePathBuf, + ) -> bool { + let Some(intercepted_command) = trusted_intercepted_command( + program, + argv, + trusted_executable_dirs, + file_system_sandbox_policy, + cwd, + ) else { + return false; + }; + let Ok(mut approved_command) = self.command.lock() else { + return false; + }; + if approved_command.as_deref() != Some(intercepted_command.as_slice()) { + return false; + } + approved_command.take(); + true + } +} + +pub(super) fn trusted_executable_dirs( + env: &HashMap, + file_system_sandbox_policy: &FileSystemSandboxPolicy, + cwd: &AbsolutePathBuf, +) -> Vec { + env.get("PATH") + .into_iter() + .flat_map(std::env::split_paths) + .filter(|path| path.is_absolute()) + .filter_map(|path| { + if agent_can_write_path(file_system_sandbox_policy, cwd, &path) { + return None; + } + let canonical_path = std::fs::canonicalize(&path).ok()?; + if agent_can_write_path(file_system_sandbox_policy, cwd, &canonical_path) { + return None; + } + Some(TrustedExecutableDir { + path, + canonical_path, + }) + }) + .collect() +} + +pub(super) fn trusted_intercepted_executable_name( + program: &AbsolutePathBuf, + argv: &[String], + trusted_executable_dirs: &[TrustedExecutableDir], + file_system_sandbox_policy: &FileSystemSandboxPolicy, + cwd: &AbsolutePathBuf, +) -> Option { + let argv_zero = argv.first()?; + let argv_zero_path = Path::new(argv_zero); + let is_bare_name = argv_zero_path.components().count() == 1; + let resolved_name_matches = program.as_path().file_name() == Some(argv_zero_path.as_os_str()); + let resolved_from_trusted_path = program.as_path().parent().is_some_and(|parent| { + trusted_executable_dirs.iter().any(|directory| { + directory.path == parent + && std::fs::canonicalize(&directory.path) + .is_ok_and(|canonical_path| canonical_path == directory.canonical_path) + }) + }); + let resolved_target_is_read_only = + std::fs::canonicalize(program.as_path()) + .ok() + .is_some_and(|canonical_program| { + !agent_can_write_path(file_system_sandbox_policy, cwd, &canonical_program) + }); + + if is_bare_name + && resolved_name_matches + && resolved_from_trusted_path + && resolved_target_is_read_only + { + Some(argv_zero.clone()) + } else { + None + } +} + +fn trusted_intercepted_command( + program: &AbsolutePathBuf, + argv: &[String], + trusted_executable_dirs: &[TrustedExecutableDir], + file_system_sandbox_policy: &FileSystemSandboxPolicy, + cwd: &AbsolutePathBuf, +) -> Option> { + let executable_name = trusted_intercepted_executable_name( + program, + argv, + trusted_executable_dirs, + file_system_sandbox_policy, + cwd, + )?; + Some( + std::iter::once(executable_name) + .chain(argv.iter().skip(1).cloned()) + .collect(), + ) +} + +fn agent_can_write_path( + file_system_sandbox_policy: &FileSystemSandboxPolicy, + cwd: &AbsolutePathBuf, + path: &Path, +) -> bool { + if !file_system_sandbox_policy.has_full_disk_write_access() { + return path.ancestors().any(|ancestor| { + file_system_sandbox_policy.can_write_path_with_cwd(ancestor, cwd.as_path()) + }); + } + + path.ancestors().any(|ancestor| { + let Ok(ancestor) = CString::new(ancestor.as_os_str().as_bytes()) else { + return true; + }; + // SAFETY: `ancestor` is a NUL-terminated C string that remains alive + // for the duration of this read-only access check. + unsafe { libc::access(ancestor.as_ptr(), libc::W_OK) == 0 } + }) +} + +#[cfg(test)] +#[path = "trusted_executable_tests.rs"] +mod tests; diff --git a/codex-rs/core/src/tools/runtimes/shell/trusted_executable_tests.rs b/codex-rs/core/src/tools/runtimes/shell/trusted_executable_tests.rs new file mode 100644 index 0000000000..1089a81cf6 --- /dev/null +++ b/codex-rs/core/src/tools/runtimes/shell/trusted_executable_tests.rs @@ -0,0 +1,193 @@ +use super::ParentApprovedIntercept; +use super::trusted_executable_dirs; +use crate::sandboxing::SandboxPermissions; +use crate::tools::sandboxing::ExecApprovalRequirement; +use codex_protocol::models::AdditionalPermissionProfile; +use codex_protocol::models::PermissionProfile; +use codex_protocol::protocol::AskForApproval; +use codex_utils_absolute_path::AbsolutePathBuf; +use std::collections::HashMap; +use std::path::Path; + +fn host_git() -> &'static Path { + ["/usr/bin/git", "/bin/git", "/opt/homebrew/bin/git"] + .into_iter() + .map(Path::new) + .find(|path| path.is_file()) + .expect("test host should provide git") +} + +fn trusted_git_fixture() -> ( + AbsolutePathBuf, + AbsolutePathBuf, + PermissionProfile, + Vec, +) { + let program = AbsolutePathBuf::from_absolute_path(host_git()).unwrap(); + let cwd = AbsolutePathBuf::try_from(std::env::current_dir().unwrap()).unwrap(); + let permission_profile = PermissionProfile::workspace_write(); + let mut env = HashMap::new(); + env.insert( + "PATH".to_string(), + program + .as_path() + .parent() + .expect("git should have a parent") + .display() + .to_string(), + ); + let trusted_dirs = + trusted_executable_dirs(&env, &permission_profile.file_system_sandbox_policy(), &cwd); + assert!(!trusted_dirs.is_empty()); + (program, cwd, permission_profile, trusted_dirs) +} + +#[test] +fn parent_approved_git_intercept_is_narrowly_scoped() { + let command = vec![ + "/bin/zsh".to_string(), + "-lc".to_string(), + "git status --short".to_string(), + ]; + let heuristic_prompt = ExecApprovalRequirement::NeedsApproval { + reason: None, + proposed_execpolicy_amendment: None, + }; + assert!( + ParentApprovedIntercept::for_parent_git_approval( + &command, + &heuristic_prompt, + AskForApproval::UnlessTrusted, + SandboxPermissions::UseDefault, + /*additional_permissions*/ None, + ) + .is_some() + ); + + let policy_prompt = ExecApprovalRequirement::NeedsApproval { + reason: Some("required by policy".to_string()), + proposed_execpolicy_amendment: None, + }; + assert!( + ParentApprovedIntercept::for_parent_git_approval( + &command, + &policy_prompt, + AskForApproval::UnlessTrusted, + SandboxPermissions::UseDefault, + /*additional_permissions*/ None, + ) + .is_none() + ); + assert!( + ParentApprovedIntercept::for_parent_git_approval( + &command, + &heuristic_prompt, + AskForApproval::UnlessTrusted, + SandboxPermissions::RequireEscalated, + /*additional_permissions*/ None, + ) + .is_none() + ); + assert!( + ParentApprovedIntercept::for_parent_git_approval( + &command, + &heuristic_prompt, + AskForApproval::UnlessTrusted, + SandboxPermissions::UseDefault, + Some(&AdditionalPermissionProfile::default()), + ) + .is_none() + ); + assert!( + ParentApprovedIntercept::for_parent_git_approval( + &["/bin/zsh".into(), "-lc".into(), "env git status".into()], + &heuristic_prompt, + AskForApproval::UnlessTrusted, + SandboxPermissions::UseDefault, + /*additional_permissions*/ None, + ) + .is_none() + ); +} + +#[test] +fn parent_approval_matches_one_exact_trusted_intercept() { + let (program, cwd, permission_profile, trusted_dirs) = trusted_git_fixture(); + let approved = ParentApprovedIntercept::new(vec![ + "git".to_string(), + "status".to_string(), + "--short".to_string(), + ]); + let argv = [ + "git".to_string(), + "status".to_string(), + "--short".to_string(), + ]; + + assert!(approved.consume_if_matches( + &program, + &argv, + &trusted_dirs, + &permission_profile.file_system_sandbox_policy(), + &cwd, + )); + assert!(!approved.consume_if_matches( + &program, + &argv, + &trusted_dirs, + &permission_profile.file_system_sandbox_policy(), + &cwd, + )); +} + +#[test] +fn parent_approval_rejects_changed_or_path_qualified_argv() { + let (program, cwd, permission_profile, trusted_dirs) = trusted_git_fixture(); + for argv in [ + vec!["git".to_string(), "diff".to_string()], + vec![ + program.to_string_lossy().to_string(), + "status".to_string(), + "--short".to_string(), + ], + ] { + let approved = ParentApprovedIntercept::new(vec![ + "git".to_string(), + "status".to_string(), + "--short".to_string(), + ]); + assert!(!approved.consume_if_matches( + &program, + &argv, + &trusted_dirs, + &permission_profile.file_system_sandbox_policy(), + &cwd, + )); + } +} + +#[test] +fn parent_approval_rejects_writable_path_shadow() { + let writable_dir = tempfile::tempdir().unwrap(); + let shadow_git = writable_dir.path().join("git"); + std::fs::write(&shadow_git, "not a trusted host executable").unwrap(); + let cwd = AbsolutePathBuf::from_absolute_path(writable_dir.path()).unwrap(); + let permission_profile = PermissionProfile::workspace_write(); + let mut env = HashMap::new(); + env.insert( + "PATH".to_string(), + writable_dir.path().display().to_string(), + ); + let trusted_dirs = + trusted_executable_dirs(&env, &permission_profile.file_system_sandbox_policy(), &cwd); + let approved = ParentApprovedIntercept::new(vec!["git".into(), "status".into()]); + + assert!(trusted_dirs.is_empty()); + assert!(!approved.consume_if_matches( + &AbsolutePathBuf::from_absolute_path(&shadow_git).unwrap(), + &["git".into(), "status".into()], + &trusted_dirs, + &permission_profile.file_system_sandbox_policy(), + &cwd, + )); +} diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index 34f040d13d..d7aac1a232 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -1,4 +1,8 @@ use super::ShellRequest; +use super::trusted_executable::ParentApprovedIntercept; +use super::trusted_executable::TrustedExecutableDir; +use super::trusted_executable::trusted_executable_dirs; +use super::trusted_executable::trusted_intercepted_executable_name; use crate::exec::ExecCapturePolicy; use crate::exec::ExecExpiration; use crate::exec::cancel_when_either; @@ -68,10 +72,7 @@ use codex_shell_escalation::Stopwatch; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; use std::collections::HashMap; -use std::ffi::CString; use std::io; -use std::os::unix::ffi::OsStrExt; -use std::path::Path; use std::path::PathBuf; use std::sync::Arc; use std::time::Duration; @@ -253,6 +254,13 @@ pub(super) async fn try_run_zsh_fork( &command_executor.file_system_sandbox_policy, &command_executor.cwd, ), + parent_approved_intercept: ParentApprovedIntercept::for_parent_git_approval( + &req.command, + &req.exec_approval_requirement, + ctx.turn.approval_policy.value(), + req.sandbox_permissions, + req.additional_permissions.as_ref(), + ), stopwatch: stopwatch.clone(), }; @@ -345,6 +353,13 @@ pub(crate) async fn prepare_unified_exec_zsh_fork( &command_executor.file_system_sandbox_policy, &command_executor.cwd, ), + parent_approved_intercept: ParentApprovedIntercept::for_parent_git_approval( + &req.command, + &req.exec_approval_requirement, + ctx.turn.approval_policy.value(), + req.sandbox_permissions, + req.additional_permissions.as_ref(), + ), stopwatch: Stopwatch::unlimited(), }; @@ -378,6 +393,7 @@ struct CoreShellActionProvider { approval_sandbox_permissions: SandboxPermissions, prompt_permissions: Option, trusted_executable_dirs: Vec, + parent_approved_intercept: Option, stopwatch: Stopwatch, } @@ -684,6 +700,28 @@ impl CoreShellActionProvider { SandboxPermissions::RequireEscalated => unsandboxed_allowed, SandboxPermissions::WithAdditionalPermissions => true, }; + let decision = if evaluation.decision == Decision::Prompt + && !decision_driven_by_policy + && !needs_escalation + && self + .parent_approved_intercept + .as_ref() + .is_some_and(|approved| { + approved.consume_if_matches( + program, + argv, + &self.trusted_executable_dirs, + &self.file_system_sandbox_policy, + workdir, + ) + }) { + tracing::debug!( + "reusing exact parent approval for trusted intercepted command {program:?}" + ); + Decision::Allow + } else { + evaluation.decision + }; let decision_source = if decision_driven_by_policy { DecisionSource::PrefixRule @@ -701,7 +739,7 @@ impl CoreShellActionProvider { ), }; self.process_decision( - evaluation.decision, + decision, needs_escalation, program, argv, @@ -821,12 +859,6 @@ struct CandidateCommands { trusted_executable_name: Option, } -#[derive(Clone, Debug, Eq, PartialEq)] -struct TrustedExecutableDir { - path: PathBuf, - canonical_path: PathBuf, -} - fn commands_for_intercepted_exec_policy( program: &AbsolutePathBuf, argv: &[String], @@ -869,88 +901,6 @@ fn commands_for_intercepted_exec_policy( } } -fn trusted_executable_dirs( - env: &HashMap, - file_system_sandbox_policy: &FileSystemSandboxPolicy, - cwd: &AbsolutePathBuf, -) -> Vec { - env.get("PATH") - .into_iter() - .flat_map(std::env::split_paths) - .filter(|path| path.is_absolute()) - .filter_map(|path| { - if agent_can_write_path(file_system_sandbox_policy, cwd, &path) { - return None; - } - let canonical_path = std::fs::canonicalize(&path).ok()?; - if agent_can_write_path(file_system_sandbox_policy, cwd, &canonical_path) { - return None; - } - Some(TrustedExecutableDir { - path, - canonical_path, - }) - }) - .collect() -} - -fn trusted_intercepted_executable_name( - program: &AbsolutePathBuf, - argv: &[String], - trusted_executable_dirs: &[TrustedExecutableDir], - file_system_sandbox_policy: &FileSystemSandboxPolicy, - cwd: &AbsolutePathBuf, -) -> Option { - let argv_zero = argv.first()?; - let argv_zero_path = Path::new(argv_zero); - let is_bare_name = argv_zero_path.components().count() == 1; - let resolved_name_matches = program.as_path().file_name() == Some(argv_zero_path.as_os_str()); - let resolved_from_trusted_path = program.as_path().parent().is_some_and(|parent| { - trusted_executable_dirs.iter().any(|directory| { - directory.path == parent - && std::fs::canonicalize(&directory.path) - .is_ok_and(|canonical_path| canonical_path == directory.canonical_path) - }) - }); - let resolved_target_is_read_only = - std::fs::canonicalize(program.as_path()) - .ok() - .is_some_and(|canonical_program| { - !agent_can_write_path(file_system_sandbox_policy, cwd, &canonical_program) - }); - - if is_bare_name - && resolved_name_matches - && resolved_from_trusted_path - && resolved_target_is_read_only - { - Some(argv_zero.clone()) - } else { - None - } -} - -fn agent_can_write_path( - file_system_sandbox_policy: &FileSystemSandboxPolicy, - cwd: &AbsolutePathBuf, - path: &Path, -) -> bool { - if !file_system_sandbox_policy.has_full_disk_write_access() { - return path.ancestors().any(|ancestor| { - file_system_sandbox_policy.can_write_path_with_cwd(ancestor, cwd.as_path()) - }); - } - - path.ancestors().any(|ancestor| { - let Ok(ancestor) = CString::new(ancestor.as_os_str().as_bytes()) else { - return true; - }; - // SAFETY: `ancestor` is a NUL-terminated C string that remains alive - // for the duration of this read-only access check. - unsafe { libc::access(ancestor.as_ptr(), libc::W_OK) == 0 } - }) -} - struct CoreShellCommandExecutor { command: Vec, cwd: AbsolutePathBuf, diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs index 505824018e..3c8b7c9cef 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs @@ -607,6 +607,7 @@ async fn preapproved_additional_permissions_escalate_intercepted_exec() -> anyho approval_sandbox_permissions: SandboxPermissions::UseDefault, prompt_permissions: Some(requested_permissions), trusted_executable_dirs: Vec::new(), + parent_approved_intercept: None, stopwatch: codex_shell_escalation::Stopwatch::new(Duration::from_secs(1)), }; @@ -744,6 +745,7 @@ async fn execve_permission_request_hook_short_circuits_prompt() -> anyhow::Resul approval_sandbox_permissions: SandboxPermissions::RequireEscalated, prompt_permissions: None, trusted_executable_dirs: Vec::new(), + parent_approved_intercept: None, stopwatch: codex_shell_escalation::Stopwatch::new(Duration::from_secs(1)), }; @@ -962,6 +964,7 @@ prefix_rule(pattern = ["{cat_path_literal}"], decision = "allow") approval_sandbox_permissions: SandboxPermissions::UseDefault, prompt_permissions: None, trusted_executable_dirs: Vec::new(), + parent_approved_intercept: None, stopwatch: codex_shell_escalation::Stopwatch::new(Duration::from_secs(1)), }; @@ -1006,6 +1009,7 @@ async fn denied_reads_keep_granular_sandbox_rejection_for_escalation() -> anyhow approval_sandbox_permissions: SandboxPermissions::RequireEscalated, prompt_permissions: None, trusted_executable_dirs: Vec::new(), + parent_approved_intercept: None, stopwatch: codex_shell_escalation::Stopwatch::new(Duration::from_secs(1)), }; diff --git a/codex-rs/core/tests/suite/exec_policy.rs b/codex-rs/core/tests/suite/exec_policy.rs index b213b79c2c..619bb18051 100644 --- a/codex-rs/core/tests/suite/exec_policy.rs +++ b/codex-rs/core/tests/suite/exec_policy.rs @@ -96,7 +96,10 @@ enum AgentExecTool { UnifiedExec, } -async fn assert_generic_git_prompts_without_amendment(tool: AgentExecTool) -> Result<()> { +async fn assert_generic_git_prompts_without_amendment( + tool: AgentExecTool, + command: &str, +) -> Result<()> { let server = start_mock_server().await; let mut builder = test_codex().with_config(move |config| { if matches!(tool, AgentExecTool::UnifiedExec) { @@ -112,7 +115,7 @@ async fn assert_generic_git_prompts_without_amendment(tool: AgentExecTool) -> Re "git-approval-shell", "shell_command", json!({ - "command": "git status --short", + "command": command, "timeout_ms": 1_000, }), ), @@ -120,7 +123,7 @@ async fn assert_generic_git_prompts_without_amendment(tool: AgentExecTool) -> Re "git-approval-unified-exec", "exec_command", json!({ - "cmd": "git status --short", + "cmd": command, "yield_time_ms": 1_000, }), ), @@ -168,7 +171,7 @@ async fn assert_generic_git_prompts_without_amendment(tool: AgentExecTool) -> Re approval .command .iter() - .any(|argument| argument.contains("git status --short")), + .any(|argument| argument.contains(command)), "unexpected {tool:?} approval command: {:?}", approval.command ); @@ -200,12 +203,31 @@ async fn assert_generic_git_prompts_without_amendment(tool: AgentExecTool) -> Re #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn generic_git_shell_command_prompts_without_amendment() -> Result<()> { - assert_generic_git_prompts_without_amendment(AgentExecTool::Shell).await + assert_generic_git_prompts_without_amendment(AgentExecTool::Shell, "git status --short").await } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn generic_git_unified_exec_prompts_without_amendment() -> Result<()> { - assert_generic_git_prompts_without_amendment(AgentExecTool::UnifiedExec).await + assert_generic_git_prompts_without_amendment(AgentExecTool::UnifiedExec, "git status --short") + .await +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn env_wrapped_git_shell_command_prompts_without_amendment() -> Result<()> { + assert_generic_git_prompts_without_amendment( + AgentExecTool::Shell, + "env GIT_OPTIONAL_LOCKS=0 git status --short", + ) + .await +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn env_wrapped_git_unified_exec_prompts_without_amendment() -> Result<()> { + assert_generic_git_prompts_without_amendment( + AgentExecTool::UnifiedExec, + "env GIT_OPTIONAL_LOCKS=0 git status --short", + ) + .await } #[cfg(windows)] diff --git a/codex-rs/core/tests/suite/generic_git_zsh_fork.rs b/codex-rs/core/tests/suite/generic_git_zsh_fork.rs new file mode 100644 index 0000000000..0ad06a5efa --- /dev/null +++ b/codex-rs/core/tests/suite/generic_git_zsh_fork.rs @@ -0,0 +1,197 @@ +use anyhow::Result; +use codex_config::types::ApprovalsReviewer; +use codex_protocol::config_types::CollaborationMode; +use codex_protocol::config_types::ModeKind; +use codex_protocol::config_types::Settings; +use codex_protocol::models::PermissionProfile; +use codex_protocol::protocol::AskForApproval; +use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::Op; +use codex_protocol::protocol::ReviewDecision; +use codex_protocol::protocol::ThreadSettingsOverrides; +use codex_protocol::user_input::UserInput; +use core_test_support::responses::ev_assistant_message; +use core_test_support::responses::ev_completed; +use core_test_support::responses::ev_function_call; +use core_test_support::responses::ev_response_created; +use core_test_support::responses::mount_sse_once; +use core_test_support::responses::sse; +use core_test_support::responses::start_mock_server; +use core_test_support::skip_if_no_network; +use core_test_support::test_codex::TestCodex; +use core_test_support::test_codex::local_selections; +use core_test_support::test_codex::turn_permission_fields; +use core_test_support::wait_for_event; +use core_test_support::zsh_fork::build_unified_exec_zsh_fork_test; +use core_test_support::zsh_fork::build_zsh_fork_test; +use core_test_support::zsh_fork::zsh_fork_runtime; +use serde_json::json; + +#[derive(Clone, Copy, Debug)] +enum AgentExecTool { + Shell, + UnifiedExec, +} + +async fn assert_generic_git_uses_one_parent_approval(tool: AgentExecTool) -> Result<()> { + skip_if_no_network!(Ok(())); + + let Some(runtime) = zsh_fork_runtime("generic Git single approval test")? else { + return Ok(()); + }; + let server = start_mock_server().await; + let approval_policy = AskForApproval::UnlessTrusted; + let permission_profile = PermissionProfile::workspace_write(); + let test = match tool { + AgentExecTool::Shell => { + build_zsh_fork_test( + &server, + runtime, + approval_policy, + permission_profile, + |_home| {}, + ) + .await? + } + AgentExecTool::UnifiedExec => { + build_unified_exec_zsh_fork_test( + &server, + runtime, + approval_policy, + permission_profile, + |_home| {}, + ) + .await? + } + }; + let call_id = match tool { + AgentExecTool::Shell => "generic-git-zsh-fork-shell", + AgentExecTool::UnifiedExec => "generic-git-zsh-fork-unified-exec", + }; + let (tool_name, args) = match tool { + AgentExecTool::Shell => ( + "shell_command", + json!({ + "command": "git status --short", + "timeout_ms": 30_000, + }), + ), + AgentExecTool::UnifiedExec => ( + "exec_command", + json!({ + "cmd": "git status --short", + "yield_time_ms": 30_000, + }), + ), + }; + let _first_response = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-generic-git-zsh-fork-1"), + ev_function_call(call_id, tool_name, &serde_json::to_string(&args)?), + ev_completed("resp-generic-git-zsh-fork-1"), + ]), + ) + .await; + let _results = mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("msg-generic-git-zsh-fork-1", "done"), + ev_completed("resp-generic-git-zsh-fork-2"), + ]), + ) + .await; + + submit_turn(&test, approval_policy).await?; + let approval = expect_approval_or_completion(&test).await; + let EventMsg::ExecApprovalRequest(approval) = approval else { + panic!("{tool:?} generic Git completed without parent approval"); + }; + assert_eq!( + approval.approval_id, None, + "{tool:?} first approval must be the parent tool command" + ); + assert_eq!(approval.proposed_execpolicy_amendment, None); + assert!( + approval + .command + .iter() + .any(|argument| argument.contains("git status --short")), + "unexpected {tool:?} parent approval command: {:?}", + approval.command + ); + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::Approved, + }) + .await?; + + match expect_approval_or_completion(&test).await { + EventMsg::TurnComplete(_) => {} + EventMsg::ExecApprovalRequest(approval) => panic!( + "{tool:?} emitted a redundant child approval after parent approval: {:?}", + approval.command + ), + event => panic!("unexpected {tool:?} event: {event:?}"), + } + + Ok(()) +} + +async fn submit_turn(test: &TestCodex, approval_policy: AskForApproval) -> Result<()> { + let session_model = test.session_configured.model.clone(); + let (sandbox_policy, permission_profile) = turn_permission_fields( + test.session_configured.permission_profile.clone(), + test.cwd.path(), + ); + test.codex + .submit(Op::UserInput { + items: vec![UserInput::Text { + text: "run repository-sensitive Git through zsh fork".into(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + responsesapi_client_metadata: None, + additional_context: Default::default(), + thread_settings: ThreadSettingsOverrides { + environments: Some(local_selections(test.config.cwd.clone())), + approval_policy: Some(approval_policy), + approvals_reviewer: Some(ApprovalsReviewer::User), + sandbox_policy: Some(sandbox_policy), + permission_profile, + collaboration_mode: Some(CollaborationMode { + mode: ModeKind::Default, + settings: Settings { + model: session_model, + reasoning_effort: None, + developer_instructions: None, + }, + }), + ..Default::default() + }, + }) + .await?; + Ok(()) +} + +async fn expect_approval_or_completion(test: &TestCodex) -> EventMsg { + wait_for_event(&test.codex, |event| { + matches!( + event, + EventMsg::ExecApprovalRequest(_) | EventMsg::TurnComplete(_) + ) + }) + .await +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn shell_zsh_fork_generic_git_uses_one_parent_approval() -> Result<()> { + assert_generic_git_uses_one_parent_approval(AgentExecTool::Shell).await +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn unified_exec_zsh_fork_generic_git_uses_one_parent_approval() -> Result<()> { + assert_generic_git_uses_one_parent_approval(AgentExecTool::UnifiedExec).await +} diff --git a/codex-rs/core/tests/suite/mod.rs b/codex-rs/core/tests/suite/mod.rs index 3e27463370..44615d8e9c 100644 --- a/codex-rs/core/tests/suite/mod.rs +++ b/codex-rs/core/tests/suite/mod.rs @@ -55,6 +55,8 @@ mod exec_policy; #[cfg(not(target_os = "windows"))] mod extension_sandbox; mod fork_thread; +#[cfg(unix)] +mod generic_git_zsh_fork; #[cfg(not(target_os = "windows"))] mod guardian_review; #[cfg(not(target_os = "windows"))]