diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index cbffcd7e43..ad4c8b8f39 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -337,15 +337,17 @@ impl ExecPolicyManager { }, None => ExecApprovalRequirement::NeedsApproval { reason: derive_prompt_reason(command, &evaluation), - proposed_execpolicy_amendment: requested_amendment.or_else(|| { - if auto_amendment_allowed { - try_derive_execpolicy_amendment_for_prompt_rules( - &evaluation.matched_rules, - ) - } else { - None - } - }), + proposed_execpolicy_amendment: suppress_repo_sensitive_git_amendment( + requested_amendment.or_else(|| { + if auto_amendment_allowed { + try_derive_execpolicy_amendment_for_prompt_rules( + &evaluation.matched_rules, + ) + } else { + None + } + }), + ), }, } } @@ -364,11 +366,13 @@ impl ExecPolicyManager { is_policy_match(rule_match) && rule_match.decision() == Decision::Allow }) }), - proposed_execpolicy_amendment: if auto_amendment_allowed { - try_derive_execpolicy_amendment_for_allow_rules(&evaluation.matched_rules) - } else { - None - }, + proposed_execpolicy_amendment: suppress_repo_sensitive_git_amendment( + if auto_amendment_allowed { + try_derive_execpolicy_amendment_for_allow_rules(&evaluation.matched_rules) + } else { + None + }, + ), }, } } @@ -848,6 +852,33 @@ 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( + amendment: Option, +) -> Option { + amendment.filter(|amendment| !starts_with_git_executable(&amendment.command)) +} + +fn starts_with_git_executable(command: &[String]) -> bool { + let Some(executable_name) = command + .first() + .and_then(|executable| Path::new(executable).file_name()) + .and_then(|name| name.to_str()) + else { + return false; + }; + let executable_name = executable_name.to_ascii_lowercase(); + let executable_name = [".exe", ".cmd", ".bat", ".com"] + .into_iter() + .find_map(|suffix| executable_name.strip_suffix(suffix)) + .unwrap_or(&executable_name); + executable_name == "git" +} + fn derive_requested_execpolicy_amendment_from_prefix_rule( prefix_rule: Option<&Vec>, matched_rules: &[RuleMatch], diff --git a/codex-rs/core/src/exec_policy_tests.rs b/codex-rs/core/src/exec_policy_tests.rs index cab401e0ca..1becceda6f 100644 --- a/codex-rs/core/src/exec_policy_tests.rs +++ b/codex-rs/core/src/exec_policy_tests.rs @@ -1019,10 +1019,7 @@ prefix_rule(pattern=["git"], decision="prompt") }, ExecApprovalRequirement::NeedsApproval { reason: None, - proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec![ - disallowed_git_path, - "status".to_string(), - ])), + proposed_execpolicy_amendment: None, }, ) .await; @@ -1199,6 +1196,180 @@ fn path_qualified_safe_basename_requires_approval_for_untrusted_projects() { ); } +#[tokio::test] +async fn generic_git_policy_matrix_preserves_sandbox_and_escalation_semantics() { + let command = vec!["git".to_string(), "status".to_string()]; + + for approval_policy in [ + AskForApproval::OnRequest, + AskForApproval::Granular(GranularApprovalConfig { + sandbox_approval: true, + rules: true, + skill_approval: true, + request_permissions: true, + mcp_elicitations: true, + }), + ] { + assert_exec_approval_requirement_for_command( + ExecApprovalRequirementScenario { + policy_src: None, + command: command.clone(), + approval_policy, + permission_profile: PermissionProfile::workspace_write(), + sandbox_permissions: SandboxPermissions::UseDefault, + prefix_rule: None, + }, + ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: None, + }, + ) + .await; + + assert_exec_approval_requirement_for_command( + ExecApprovalRequirementScenario { + policy_src: None, + command: command.clone(), + approval_policy, + permission_profile: PermissionProfile::workspace_write(), + sandbox_permissions: SandboxPermissions::RequireEscalated, + prefix_rule: None, + }, + ExecApprovalRequirement::NeedsApproval { + reason: None, + proposed_execpolicy_amendment: None, + }, + ) + .await; + } + + assert_exec_approval_requirement_for_command( + ExecApprovalRequirementScenario { + policy_src: None, + command, + approval_policy: AskForApproval::UnlessTrusted, + permission_profile: PermissionProfile::workspace_write(), + sandbox_permissions: SandboxPermissions::UseDefault, + prefix_rule: None, + }, + ExecApprovalRequirement::NeedsApproval { + reason: None, + proposed_execpolicy_amendment: None, + }, + ) + .await; +} + +#[tokio::test] +async fn generic_git_one_shot_approval_does_not_create_cross_repo_cache() { + let manager = ExecPolicyManager::default(); + let command = vec!["git".to_string(), "status".to_string()]; + + // A one-shot approval does not mutate the policy manager. Because Codex no + // longer offers a durable amendment for repository-sensitive Git, the same + // argv in a second checkout must prompt again. + for checkout in ["first checkout", "second checkout"] { + let requirement = manager + .create_exec_approval_requirement_for_command(ExecApprovalRequest { + command: &command, + approval_policy: AskForApproval::UnlessTrusted, + permission_profile: PermissionProfile::workspace_write(), + windows_sandbox_level: WindowsSandboxLevel::RestrictedToken, + sandbox_permissions: SandboxPermissions::UseDefault, + prefix_rule: None, + }) + .await; + + assert_eq!( + requirement, + ExecApprovalRequirement::NeedsApproval { + reason: None, + proposed_execpolicy_amendment: None, + }, + "{checkout} must require its own approval", + ); + } +} + +#[tokio::test] +async fn generic_git_never_offers_requested_or_shell_lowered_durable_amendments() { + for (command, prefix_rule) in [ + ( + vec!["git".to_string(), "status".to_string()], + Some(vec!["git".to_string(), "status".to_string()]), + ), + ( + vec![ + "bash".to_string(), + "-lc".to_string(), + "git status".to_string(), + ], + None, + ), + ( + vec![ + if cfg!(windows) { + r"C:\Program Files\Git\cmd\git.exe".to_string() + } else { + "/usr/bin/git".to_string() + }, + "status".to_string(), + ], + None, + ), + ] { + assert_exec_approval_requirement_for_command( + ExecApprovalRequirementScenario { + policy_src: None, + command, + approval_policy: AskForApproval::UnlessTrusted, + permission_profile: PermissionProfile::workspace_write(), + sandbox_permissions: SandboxPermissions::UseDefault, + prefix_rule, + }, + ExecApprovalRequirement::NeedsApproval { + reason: None, + proposed_execpolicy_amendment: None, + }, + ) + .await; + } +} + +#[tokio::test] +async fn explicit_user_authored_git_policy_rule_still_allows_matching_commands() { + for command in [ + vec![ + "git".to_string(), + "status".to_string(), + "--short".to_string(), + ], + vec![ + "bash".to_string(), + "-lc".to_string(), + "git status --short".to_string(), + ], + ] { + assert_exec_approval_requirement_for_command( + ExecApprovalRequirementScenario { + policy_src: Some( + r#"prefix_rule(pattern=["git", "status"], decision="allow")"#.to_string(), + ), + command, + 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; + } +} + #[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_safe_command.rs b/codex-rs/shell-command/src/command_safety/is_safe_command.rs index 6e11d8150a..b941091956 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 @@ -307,6 +307,60 @@ mod tests { } } + #[test] + fn git_repo_selected_helper_and_transport_variants_require_approval() { + for args in [ + // `status` can invoke a repository-selected fsmonitor. + vec_str(&["git", "status", "--short"]), + // Diff-producing commands can invoke external diff and textconv helpers. + vec_str(&["git", "diff", "--ext-diff", "HEAD"]), + vec_str(&["git", "log", "--textconv", "-1"]), + vec_str(&["git", "show", "--textconv", "HEAD"]), + // Filters and transport were not in the old safelist, but keep them + // in the regression matrix so future safelist expansion fails closed. + vec_str(&["git", "cat-file", "--filters", "HEAD:file.txt"]), + vec_str(&["git", "remote", "show", "origin"]), + vec_str(&["git", "fetch", "origin"]), + // Shell lowering must preserve the same decision. + vec_str(&["bash", "-lc", "git status --short"]), + vec_str(&["bash", "-lc", "git diff --ext-diff HEAD"]), + vec_str(&["bash", "-lc", "git log --textconv -1"]), + vec_str(&["bash", "-lc", "git show --textconv HEAD"]), + vec_str(&["bash", "-lc", "git branch --show-current"]), + ] { + assert!( + !is_known_safe_command(&args), + "expected repository-sensitive Git variant {args:?} to require approval", + ); + } + } + + #[test] + fn path_qualified_git_variants_require_approval() { + let absolute_git = if cfg!(windows) { + r"C:\Program Files\Git\cmd\git.exe" + } else { + "/usr/bin/git" + }; + let current_relative_git = if cfg!(windows) { r".\git.exe" } else { "./git" }; + let parent_relative_git = if cfg!(windows) { + r"..\git.exe" + } else { + "../git" + }; + + for args in [ + vec_str(&[absolute_git, "status"]), + vec_str(&[current_relative_git, "status"]), + vec_str(&[parent_relative_git, "status"]), + ] { + assert!( + !is_known_safe_command(&args), + "expected path-qualified Git executable {args:?} to require approval", + ); + } + } + #[test] fn git_branch_mutating_flags_are_not_safe() { assert!(!is_known_safe_command(&vec_str(&[