diff --git a/codex-rs/core/src/turn_metadata_tests.rs b/codex-rs/core/src/turn_metadata_tests.rs index c2bee7843d..37b601e0c6 100644 --- a/codex-rs/core/src/turn_metadata_tests.rs +++ b/codex-rs/core/src/turn_metadata_tests.rs @@ -77,6 +77,16 @@ fn workspace_metadata_serializes_structured_has_changes_refusal() { "operation": "status", }) ); + assert_eq!( + serde_json::to_value(codex_git_utils::GitReadError::InvalidConfigEnvironment { + reason: "malformed GIT_CONFIG_COUNT".to_string(), + }) + .expect("serialize invalid config environment"), + serde_json::json!({ + "reason": "invalidConfigEnvironment", + "details": "malformed GIT_CONFIG_COUNT", + }) + ); } #[cfg(any(unix, windows))] diff --git a/codex-rs/git-utils/src/apply.rs b/codex-rs/git-utils/src/apply.rs index 5aae7d7a55..e01903aa16 100644 --- a/codex-rs/git-utils/src/apply.rs +++ b/codex-rs/git-utils/src/apply.rs @@ -10,15 +10,15 @@ use std::io; use std::path::Path; use std::path::PathBuf; +#[cfg(test)] use crate::FsmonitorOverride; use crate::apply_output::parse_git_apply_output; use crate::git_command::GitRunner; -use crate::git_config_sources::ensure_no_worktree_config_sources; -use crate::merge_driver::ensure_no_selected_merge_drivers; -use crate::patch_paths::extract_patch_path_inventory; +use crate::guarded_config::GuardedGitConfig; +use crate::patch_paths::extract_patch_path_inventory_guarded; use crate::reverse_staging::stage_effective_paths_for_reverse; +#[cfg(test)] use crate::safe_git::DISABLED_HOOKS_PATH; -use crate::safe_git::ensure_no_selected_executable_git_filters; #[cfg(test)] use crate::safe_git::isolate_git_command_environment; @@ -48,26 +48,25 @@ pub struct ApplyGitResult { /// When [`ApplyGitRequest::preflight`] is `true`, this behaves like `git apply --check` and /// leaves the working tree untouched while still parsing the command output for diagnostics. pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { + let cfg_parts = configured_git_config_parts(); + // Construct from the caller's exact route before canonicalization so + // repository authority retains lexical enclosing worktrees. let git = GitRunner::for_cwd_io(&req.cwd)?; - let mut cfg_parts = configured_git_config_parts(); - ensure_no_worktree_config_sources(&git, &req.cwd, &cfg_parts)?; let requested_cwd = std::fs::canonicalize(&req.cwd)?; - let git_root = resolve_git_root(&git, &req.cwd, &cfg_parts)?; - if git_root != requested_cwd { - ensure_no_worktree_config_sources(&git, &git_root, &cfg_parts)?; - } + let expected_root = crate::get_git_repo_root(&requested_cwd) + .ok_or_else(|| io::Error::other("not a Git repository")) + .and_then(std::fs::canonicalize)?; + let mut config = GuardedGitConfig::authorize(&git, &expected_root, cfg_parts)?; + resolve_git_root(&config, &requested_cwd)?; // Write unified diff into a temporary file let (tmpdir, patch_path) = write_temp_patch(&req.diff)?; // Keep tmpdir alive until function end to ensure the file exists let _guard = tmpdir; let patch_path_inventory = - extract_patch_path_inventory(&git, &git_root, &patch_path, req.revert, &cfg_parts)?; + extract_patch_path_inventory_guarded(&config, &patch_path, req.revert)?; let patch_paths = &patch_path_inventory.effective_paths; - let filter_guard = - ensure_no_selected_executable_git_filters(&git, &git_root, patch_paths, &cfg_parts)?; - cfg_parts.extend(safe_git_config_parts()); - cfg_parts.extend_from_slice(filter_guard.git_config_args()); + config.authorize_filter_paths(patch_paths)?; let patch_arg = patch_path.to_string_lossy().to_string(); // Optional preflight: dry-run only; do not modify working tree @@ -77,8 +76,8 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { check_args.push("-R".to_string()); } check_args.push(patch_arg); - let rendered = render_command_for_log(&git_root, &cfg_parts, &check_args); - let (c_code, c_out, c_err) = run_git(&git, &git_root, &cfg_parts, &check_args)?; + let rendered = config.render_command_for_log(&check_args)?; + let (c_code, c_out, c_err) = run_guarded_apply(&config, &check_args)?; let (mut applied_paths, mut skipped_paths, mut conflicted_paths) = parse_git_apply_output(&c_out, &c_err); applied_paths.sort(); @@ -110,33 +109,27 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { plain_check_args.push("-R".to_string()); } plain_check_args.push(patch_arg.clone()); - let (plain_check_code, _, _) = run_git(&git, &git_root, &cfg_parts, &plain_check_args)?; + let (plain_check_code, _, _) = run_guarded_apply(&config, &plain_check_args)?; let mut args = vec!["apply".to_string()]; - let merge_guard = if plain_check_code != 0 { - let guard = ensure_no_selected_merge_drivers(&git, &git_root, patch_paths, &cfg_parts)?; - if let Some(guard) = &guard { - cfg_parts.extend_from_slice(guard.git_config_args()); - } + if plain_check_code != 0 { + config.install_three_way_merge_policy()?; args.push("--3way".to_string()); - guard } else { args.push("--index".to_string()); - None - }; + } if req.revert { // Stage only after the worktree-only applicability decision. The // guarded config is shared with the final Git command. - stage_effective_paths_for_reverse(&git, &git_root, patch_paths, &cfg_parts)?; + stage_effective_paths_for_reverse(&mut config, patch_paths)?; } if req.revert { args.push("-R".to_string()); } args.push(patch_arg); - let cmd_for_log = render_command_for_log(&git_root, &cfg_parts, &args); - let (code, stdout, stderr) = run_git(&git, &git_root, &cfg_parts, &args)?; - drop(merge_guard); + let cmd_for_log = config.render_command_for_log(&args)?; + let (code, stdout, stderr) = run_guarded_apply(&config, &args)?; let (mut applied_paths, mut skipped_paths, mut conflicted_paths) = if code == 0 { (patch_path_inventory.primary_paths, Vec::new(), Vec::new()) @@ -161,18 +154,10 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { }) } -fn resolve_git_root( - git: &GitRunner, - cwd: &Path, - git_config_args: &[String], -) -> io::Result { - let requested_cwd = std::fs::canonicalize(cwd)?; - let mut command = git.command_for_cwd(&requested_cwd)?; - command - .args(git_config_args) - .arg("rev-parse") - .arg("--show-toplevel"); - let out = git.output(command)?; +fn resolve_git_root(config: &GuardedGitConfig<'_>, requested_cwd: &Path) -> io::Result { + let mut command = config.rev_parse_command()?; + command.arg("--show-toplevel"); + let out = command.output()?; let code = out.status.code().unwrap_or(-1); if code != 0 { return Err(io::Error::other(format!( @@ -183,18 +168,7 @@ fn resolve_git_root( } let reported_root = PathBuf::from(String::from_utf8_lossy(&out.stdout).trim()); let root = std::fs::canonicalize(&reported_root)?; - let expected_root = crate::get_git_repo_root(&requested_cwd) - .ok_or_else(|| { - io::Error::new( - io::ErrorKind::PermissionDenied, - format!( - "refusing to apply a patch because Git resolved worktree {} without a .git marker above requested cwd {}", - root.display(), - requested_cwd.display() - ), - ) - }) - .and_then(std::fs::canonicalize)?; + let expected_root = config.canonical_root(); if root != expected_root { return Err(io::Error::new( io::ErrorKind::PermissionDenied, @@ -231,26 +205,33 @@ pub(crate) fn write_temp_patch(diff: &str) -> io::Result<(tempfile::TempDir, Pat Ok((dir, path)) } -pub(crate) fn run_git( - git: &GitRunner, - cwd: &Path, - git_cfg: &[String], +fn run_guarded_apply( + config: &GuardedGitConfig<'_>, args: &[String], ) -> io::Result<(i32, String, String)> { - let mut cmd = git.command_for_cwd(cwd)?; - for p in git_cfg { - cmd.arg(p); + let Some((subcommand, args)) = args.split_first() else { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "missing guarded Git subcommand", + )); + }; + if subcommand != "apply" { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "unexpected guarded Git subcommand", + )); } - for a in args { - cmd.arg(a); - } - let out = git.output(cmd)?; - let code = out.status.code().unwrap_or(-1); - let stdout = String::from_utf8_lossy(&out.stdout).into_owned(); - let stderr = String::from_utf8_lossy(&out.stderr).into_owned(); - Ok((code, stdout, stderr)) + let mut command = config.apply_command()?; + command.args(args); + let output = command.output()?; + Ok(( + output.status.code().unwrap_or(-1), + String::from_utf8_lossy(&output.stdout).into_owned(), + String::from_utf8_lossy(&output.stderr).into_owned(), + )) } +#[cfg(test)] pub(crate) fn safe_git_config_parts() -> Vec { vec![ "-c".to_string(), @@ -260,33 +241,6 @@ pub(crate) fn safe_git_config_parts() -> Vec { ] } -fn quote_shell(s: &str) -> String { - let simple = s - .chars() - .all(|c| c.is_ascii_alphanumeric() || "-_.:/@%+".contains(c)); - if simple { - s.to_string() - } else { - format!("'{}'", s.replace('\'', "'\\''")) - } -} - -fn render_command_for_log(cwd: &Path, git_cfg: &[String], args: &[String]) -> String { - let mut parts: Vec = Vec::new(); - parts.push("git".to_string()); - for a in git_cfg { - parts.push(quote_shell(a)); - } - for a in args { - parts.push(quote_shell(a)); - } - format!( - "(cd {} && {})", - quote_shell(&cwd.display().to_string()), - parts.join(" ") - ) -} - #[cfg(all(test, unix))] #[path = "apply_transport_tests.rs"] mod transport_tests; @@ -302,6 +256,14 @@ mod reverse_apply_tests; #[cfg(test)] mod tests { use super::*; + use crate::guarded_config::config_source_authorization_count; + use crate::guarded_config::merge_attribute_read_count; + use crate::guarded_config::merge_config_read_count; + use crate::guarded_config::reset_config_source_authorization_count; + use crate::guarded_config::reset_merge_policy_counts; + use crate::safe_git::filter_policy_overlay_count; + use crate::safe_git::filter_policy_read_count; + use crate::safe_git::reset_filter_policy_counts; use std::ffi::OsStr; use std::path::Path; use std::sync::Mutex; @@ -446,13 +408,38 @@ mod tests { revert: false, preflight: false, }; + reset_config_source_authorization_count(); let r = apply_git_patch(&req).expect("run apply"); + assert_eq!(config_source_authorization_count(), 1); assert_eq!(r.exit_code, 0, "exit code 0"); assert_eq!(r.applied_paths, vec!["hello.txt"]); // File exists now assert!(root.join("hello.txt").exists()); } + #[test] + fn preflight_uses_one_authorization_and_skips_merge_policy() { + let _g = env_lock().lock().unwrap(); + let repo = init_repo(); + let root = repo.path(); + let request = ApplyGitRequest { + cwd: root.to_path_buf(), + diff: "diff --git a/preflight.txt b/preflight.txt\nnew file mode 100644\n--- /dev/null\n+++ b/preflight.txt\n@@ -0,0 +1 @@\n+preflight\n".to_string(), + revert: false, + preflight: true, + }; + + reset_config_source_authorization_count(); + reset_merge_policy_counts(); + let result = apply_git_patch(&request).expect("preflight"); + + assert_eq!(result.exit_code, 0, "{}", result.stderr); + assert_eq!(config_source_authorization_count(), 1); + assert_eq!(merge_config_read_count(), 0); + assert_eq!(merge_attribute_read_count(), 0); + assert!(!root.join("preflight.txt").exists()); + } + #[test] fn apply_uses_cwd_repo_despite_inherited_repository_selectors() { let _g = env_lock().lock().unwrap(); @@ -576,6 +563,41 @@ mod tests { assert!(!marker.exists(), "enclosing Git shim must not run"); } + #[cfg(unix)] + #[test] + fn apply_uses_physical_repository_for_symlinked_nested_cwd() { + let _g = env_lock().lock().unwrap(); + let fixture = tempfile::tempdir().expect("fixture"); + let outer = fixture.path().join("outer"); + let target = fixture.path().join("target"); + let nested = target.join("nested"); + std::fs::create_dir_all(&outer).expect("outer"); + std::fs::create_dir_all(&nested).expect("nested"); + let (outer_init, _, outer_error) = run(&outer, &["git", "init", "-q"]); + assert_eq!(outer_init, 0, "outer init: {outer_error}"); + let (target_init, _, target_error) = run(&target, &["git", "init", "-q"]); + assert_eq!(target_init, 0, "target init: {target_error}"); + let lexical_cwd = outer.join("linked-nested"); + std::os::unix::fs::symlink(&nested, &lexical_cwd).expect("nested cwd symlink"); + + reset_config_source_authorization_count(); + let result = apply_git_patch(&ApplyGitRequest { + cwd: lexical_cwd, + diff: "diff --git a/physical.txt b/physical.txt\nnew file mode 100644\n--- /dev/null\n+++ b/physical.txt\n@@ -0,0 +1 @@\n+physical\n".to_string(), + revert: false, + preflight: false, + }) + .expect("apply through symlinked nested cwd"); + + assert_eq!(result.exit_code, 0); + assert_eq!(config_source_authorization_count(), 1); + assert_eq!( + read_file_normalized(&target.join("physical.txt")), + "physical\n" + ); + assert!(!outer.join("physical.txt").exists()); + } + #[test] fn apply_modify_conflict() { let _g = env_lock().lock().unwrap(); @@ -619,6 +641,22 @@ mod tests { #[test] fn apply_then_revert_success() { let _g = env_lock().lock().unwrap(); + if std::env::var_os("CODEX_GIT_UTILS_APPLY_ENV_CHILD").is_none() { + let config_dir = tempfile::tempdir().expect("config tempdir"); + let global_config = config_dir.path().join("global.gitconfig"); + let system_config = config_dir.path().join("system.gitconfig"); + std::fs::write(&global_config, "").expect("empty global config"); + std::fs::write(&system_config, "").expect("empty system config"); + run_isolated_test( + "apply::tests::apply_then_revert_success", + &[ + ("GIT_CONFIG_GLOBAL", global_config.as_os_str()), + ("GIT_CONFIG_SYSTEM", system_config.as_os_str()), + ], + ); + return; + } + let repo = init_repo(); let root = repo.path(); // Seed file and commit original content @@ -634,7 +672,9 @@ mod tests { revert: false, preflight: false, }; + reset_config_source_authorization_count(); let res_apply = apply_git_patch(&apply_req).expect("apply ok"); + assert_eq!(config_source_authorization_count(), 1); assert_eq!(res_apply.exit_code, 0, "forward apply succeeded"); let after_apply = read_file_normalized(&root.join("file.txt")); assert_eq!(after_apply, "ORIG\n"); @@ -646,7 +686,16 @@ mod tests { revert: true, preflight: false, }; + reset_config_source_authorization_count(); + reset_filter_policy_counts(); let res_revert = apply_git_patch(&revert_req).expect("revert ok"); + assert_eq!(config_source_authorization_count(), 1); + assert_eq!( + filter_policy_read_count(), + 1, + "reverse staging must skip a fresh Git-add policy when the index already matches" + ); + assert_eq!(filter_policy_overlay_count(), 0); assert_eq!(res_revert.exit_code, 0, "revert apply succeeded"); let after_revert = read_file_normalized(&root.join("file.txt")); assert_eq!(after_revert, "orig\n"); @@ -683,7 +732,9 @@ mod tests { revert: true, preflight: true, }; + reset_config_source_authorization_count(); let res_preflight = apply_git_patch(&preflight_req).expect("preflight ok"); + assert_eq!(config_source_authorization_count(), 1); assert_eq!(res_preflight.exit_code, 0, "revert preflight succeeded"); let (_code_after, staged_after, _stderr_after) = run(root, &["git", "diff", "--cached", "--name-only"]); @@ -713,7 +764,9 @@ diff --git a/ghost.txt b/ghost.txt\n--- a/ghost.txt\n+++ b/ghost.txt\n@@ -1,1 +1 revert: false, preflight: true, }; + reset_config_source_authorization_count(); let r1 = apply_git_patch(&req1).expect("preflight apply"); + assert_eq!(config_source_authorization_count(), 1); assert_ne!(r1.exit_code, 0, "preflight reports failure"); assert!( !root.join("ok.txt").exists(), @@ -731,7 +784,9 @@ diff --git a/ghost.txt b/ghost.txt\n--- a/ghost.txt\n+++ b/ghost.txt\n@@ -1,1 +1 revert: false, preflight: false, }; + reset_config_source_authorization_count(); let r2 = apply_git_patch(&req2).expect("direct apply"); + assert_eq!(config_source_authorization_count(), 1); assert_ne!(r2.exit_code, 0, "apply is expected to fail overall"); assert!( !r2.cmd_for_log.contains("--check"), @@ -759,7 +814,13 @@ diff --git a/ghost.txt b/ghost.txt\n--- a/ghost.txt\n+++ b/ghost.txt\n@@ -1,1 +1 revert, preflight, }; + reset_config_source_authorization_count(); let error = apply_git_patch(&request).expect_err("reject configured filter"); + assert_eq!( + config_source_authorization_count(), + 1, + "{revert} {preflight}" + ); assert_eq!(error.kind(), io::ErrorKind::Unsupported); assert!(!configured_filter_ran(root)); assert_eq!(read_file_normalized(&root.join("file.txt")), "orig\n"); @@ -847,8 +908,10 @@ diff --git a/ghost.txt b/ghost.txt\n--- a/ghost.txt\n+++ b/ghost.txt\n@@ -1,1 +1 assert_eq!(config_code, 0, "configure core.worktree: {config_err}"); let git = GitRunner::for_cwd_io(&attacker).expect("trusted Git"); + let guarded = GuardedGitConfig::authorize(&git, &attacker, Vec::new()) + .expect("authorize attacker repository config"); let error = - resolve_git_root(&git, &attacker, &[]).expect_err("reject redirected worktree"); + resolve_git_root(&guarded, &attacker).expect_err("reject redirected worktree"); assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); assert!(error.to_string().contains("instead of expected worktree")); } diff --git a/codex-rs/git-utils/src/apply_filter_tests.rs b/codex-rs/git-utils/src/apply_filter_tests.rs index a006385cee..95cd11f3be 100644 --- a/codex-rs/git-utils/src/apply_filter_tests.rs +++ b/codex-rs/git-utils/src/apply_filter_tests.rs @@ -536,6 +536,38 @@ fn command_scoped_offpath_filter_is_neutralized_after_local_config() { ); } +#[test] +fn filter_override_rejects_process_temp_directory_inside_worktree() { + let root_name = "CODEX_GIT_UTILS_WORKTREE_TMP_ROOT"; + if let Some(root) = std::env::var_os(root_name) { + let root = std::path::PathBuf::from(root); + let error = apply_git_patch(&request( + &root, /*revert*/ false, /*preflight*/ true, + )) + .expect_err("reject worktree-owned filter override"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + assert!(!configured_filter_ran(&root)); + return; + } + + let repo = init_repo(); + let root = repo.path(); + configure_filter(root, "demo"); + let worktree_temp = root.join("process-temp"); + std::fs::create_dir(&worktree_temp).expect("worktree temp directory"); + run_isolated_test( + "apply::filter_tests::filter_override_rejects_process_temp_directory_inside_worktree", + &[ + (root_name, root.as_os_str()), + ("TMPDIR", worktree_temp.as_os_str()), + #[cfg(windows)] + ("TEMP", worktree_temp.as_os_str()), + #[cfg(windows)] + ("TMP", worktree_temp.as_os_str()), + ], + ); +} + #[test] fn apply_neutralizes_filter_activated_for_racy_offpath_by_same_patch() { for (name, command) in [ diff --git a/codex-rs/git-utils/src/branch.rs b/codex-rs/git-utils/src/branch.rs index fd5d1d05a1..8d5c8f1e03 100644 --- a/codex-rs/git-utils/src/branch.rs +++ b/codex-rs/git-utils/src/branch.rs @@ -2,10 +2,11 @@ use std::ffi::OsString; use std::path::Path; use crate::GitToolingError; +use crate::git_command::GitRunner; use crate::operations::ensure_git_repository; use crate::operations::resolve_head; use crate::operations::resolve_repository_root; -use crate::operations::run_git_for_stdout; +use crate::operations::run_git_for_stdout_with_runner; /// Returns the merge-base commit between `HEAD` and the latest version between local /// and remote of the provided branch, if both exist. @@ -16,25 +17,28 @@ pub fn merge_base_with_head( repo_path: &Path, branch: &str, ) -> Result, GitToolingError> { - ensure_git_repository(repo_path)?; - let repo_root = resolve_repository_root(repo_path)?; - let head = match resolve_head(repo_root.as_path())? { + let git = GitRunner::for_cwd_io(repo_path)?; + ensure_git_repository(&git, repo_path)?; + let repo_root = resolve_repository_root(&git, repo_path)?; + let head = match resolve_head(&git, repo_root.as_path())? { Some(head) => head, None => return Ok(None), }; - let Some(branch_ref) = resolve_branch_ref(repo_root.as_path(), branch)? else { + let Some(branch_ref) = resolve_branch_ref(&git, repo_root.as_path(), branch)? else { return Ok(None); }; - let preferred_ref = - if let Some(upstream) = resolve_upstream_if_remote_ahead(repo_root.as_path(), branch)? { - resolve_branch_ref(repo_root.as_path(), &upstream)?.unwrap_or(branch_ref) - } else { - branch_ref - }; + let preferred_ref = if let Some(upstream) = + resolve_upstream_if_remote_ahead(&git, repo_root.as_path(), branch)? + { + resolve_branch_ref(&git, repo_root.as_path(), &upstream)?.unwrap_or(branch_ref) + } else { + branch_ref + }; - let merge_base = run_git_for_stdout( + let merge_base = run_git_for_stdout_with_runner( + &git, repo_root.as_path(), vec![ OsString::from("merge-base"), @@ -47,8 +51,13 @@ pub fn merge_base_with_head( Ok(Some(merge_base)) } -fn resolve_branch_ref(repo_root: &Path, branch: &str) -> Result, GitToolingError> { - let rev = run_git_for_stdout( +fn resolve_branch_ref( + git: &GitRunner, + repo_root: &Path, + branch: &str, +) -> Result, GitToolingError> { + let rev = run_git_for_stdout_with_runner( + git, repo_root, vec![ OsString::from("rev-parse"), @@ -66,10 +75,12 @@ fn resolve_branch_ref(repo_root: &Path, branch: &str) -> Result, } fn resolve_upstream_if_remote_ahead( + git: &GitRunner, repo_root: &Path, branch: &str, ) -> Result, GitToolingError> { - let upstream = match run_git_for_stdout( + let upstream = match run_git_for_stdout_with_runner( + git, repo_root, vec![ OsString::from("rev-parse"), @@ -90,7 +101,8 @@ fn resolve_upstream_if_remote_ahead( Err(other) => return Err(other), }; - let counts = match run_git_for_stdout( + let counts = match run_git_for_stdout_with_runner( + git, repo_root, vec![ OsString::from("rev-list"), @@ -120,7 +132,17 @@ fn resolve_upstream_if_remote_ahead( mod tests { use super::merge_base_with_head; use crate::GitToolingError; + #[cfg(unix)] + use crate::git_command::GitRunner; + use crate::git_command::git_runner_construction_count; + use crate::git_command::reset_git_runner_construction_count; + #[cfg(unix)] + use crate::operations::ensure_git_repository; + #[cfg(unix)] + use crate::operations::resolve_repository_root; use pretty_assertions::assert_eq; + #[cfg(unix)] + use std::io; use std::path::Path; use std::process::Command; use tempfile::tempdir; @@ -187,8 +209,14 @@ mod tests { run_git_in(repo, &["checkout", "feature"]); let expected = run_git_stdout(repo, &["merge-base", "HEAD", "main"]); + reset_git_runner_construction_count(); let merge_base = merge_base_with_head(repo, "main")?; assert_eq!(merge_base, Some(expected)); + assert_eq!( + git_runner_construction_count(), + 1, + "one high-level merge-base call must retain one Git runner" + ); Ok(()) } @@ -253,4 +281,67 @@ mod tests { Ok(()) } + + #[test] + fn merge_base_returns_none_when_head_is_unborn() -> Result<(), GitToolingError> { + let temp = tempdir()?; + let repo = temp.path(); + init_test_repo(repo); + + let merge_base = merge_base_with_head(repo, "main")?; + assert_eq!(merge_base, None); + + Ok(()) + } + + #[test] + fn merge_base_reports_non_repository() { + let temp = tempdir().expect("tempdir"); + let path = temp.path(); + + let error = merge_base_with_head(path, "main").expect_err("non-repository must fail"); + let GitToolingError::NotAGitRepository { path: error_path } = error else { + panic!("expected non-repository error, got {error:?}"); + }; + assert_eq!(error_path, path); + } + + #[cfg(unix)] + #[test] + fn retained_runner_refuses_gitdir_retarget_between_subcommands() { + use std::os::unix::fs::symlink; + + let fixture = tempdir().expect("fixture"); + let repo = fixture.path().join("repo"); + let admin = fixture.path().join("external-admin"); + std::fs::create_dir_all(&repo).expect("create repository"); + run_git_in( + fixture.path(), + &[ + "init", + "--separate-git-dir", + admin.to_str().expect("UTF-8 admin path"), + repo.to_str().expect("UTF-8 repository path"), + ], + ); + + reset_git_runner_construction_count(); + let git = GitRunner::for_cwd(&repo).expect("operation-scoped Git runner"); + ensure_git_repository(&git, &repo).expect("first merge-base subcommand"); + + symlink(&admin, repo.join("switch")).expect("Git-dir route switch"); + std::fs::write(repo.join(".git"), "gitdir: switch\n").expect("retarget Git-dir marker"); + + let error = resolve_repository_root(&git, &repo) + .expect_err("retained authority must refuse the second subcommand"); + let GitToolingError::Io(error) = error else { + panic!("expected metadata revalidation error, got {error:?}"); + }; + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied, "{error}"); + assert_eq!( + git_runner_construction_count(), + 1, + "subcommands must not rediscover authority after metadata retargeting" + ); + } } diff --git a/codex-rs/git-utils/src/errors.rs b/codex-rs/git-utils/src/errors.rs index 0c22fc424a..be2163faac 100644 --- a/codex-rs/git-utils/src/errors.rs +++ b/codex-rs/git-utils/src/errors.rs @@ -95,6 +95,11 @@ pub enum GitReadError { FilterSelectionProbeLimitExceeded { max_probes: usize }, #[error("executable filter {driver:?} is selected for {path:?}")] SelectedExecutableFilter { driver: String, path: String }, + #[error("invalid Git configuration environment: {reason}")] + InvalidConfigEnvironment { + #[serde(rename = "details")] + reason: String, + }, } /// Path fields in caller-visible diagnostic metadata use a deliberately lossy @@ -133,9 +138,9 @@ impl GitReadError { | Self::AuthorityRefused { .. } | Self::FilterSelectionProbeLimitExceeded { .. } | Self::SelectedExecutableFilter { .. } => std::io::ErrorKind::PermissionDenied, - Self::InvalidRepositoryMetadata { .. } | Self::InvalidOutput { .. } => { - std::io::ErrorKind::InvalidData - } + Self::InvalidRepositoryMetadata { .. } + | Self::InvalidOutput { .. } + | Self::InvalidConfigEnvironment { .. } => std::io::ErrorKind::InvalidData, Self::CommandTimedOut { .. } => std::io::ErrorKind::TimedOut, Self::CommandFailed { .. } => std::io::ErrorKind::Other, } diff --git a/codex-rs/git-utils/src/exact_index_policy.rs b/codex-rs/git-utils/src/exact_index_policy.rs index dd04aa455a..e7c2b6947f 100644 --- a/codex-rs/git-utils/src/exact_index_policy.rs +++ b/codex-rs/git-utils/src/exact_index_policy.rs @@ -6,7 +6,7 @@ use std::io; use std::path::Path; use crate::exact_filesystem_policy::filesystem_spelling_conflicts; -use crate::git_command::GitRunner; +use crate::guarded_config::GuardedGitConfig; #[derive(Clone, Debug, Eq, PartialEq)] pub(crate) enum ExactIndexPolicy { @@ -90,16 +90,14 @@ impl<'a> IndexEvidence<'a> { } pub(crate) fn resolve_exact_index_policy( - git: &GitRunner, - git_root: &Path, + config: &GuardedGitConfig<'_>, paths: &[String], content_filter_paths: &[String], - git_config_args: &[String], ) -> io::Result { - let output = read_index(git, git_root, git_config_args)?; + let output = read_index(config)?; let entries = parse_index_entries(&output)?; - let ignore_case = - effective_git_bool(git, git_root, git_config_args, "core.ignoreCase")?.unwrap_or(false); + let ignore_case = config.read_bool("core.ignoreCase")?.unwrap_or(false); + let git_root = config.canonical_root(); let (paths, content_filter_paths) = if ignore_case { let mapped = match map_ignore_case_paths(git_root, &entries, paths, content_filter_paths) { @@ -171,15 +169,14 @@ pub(crate) fn resolve_exact_index_policy( }) } -fn read_index(git: &GitRunner, git_root: &Path, git_config_args: &[String]) -> io::Result> { +fn read_index(config: &GuardedGitConfig<'_>) -> io::Result> { // One full, byte-oriented scan avoids invoking Git once per patch path and // lets one parse enforce aliases, directory prefixes, and flags together. - let mut command = git.command_for_cwd(git_root)?; + let mut command = config.ls_files_command()?; command - .env("GIT_OPTIONAL_LOCKS", "0") - .args(git_config_args) - .args(["ls-files", "--cached", "-v", "-z"]); - let output = git.output(command)?; + .disable_optional_locks() + .args(["--cached", "-v", "-z"]); + let output = command.output()?; if !output.status.success() { return Err(io::Error::other(format!( "git exact-index probe failed with status {}: {}", @@ -423,38 +420,6 @@ fn ascii_fold(value: &[u8]) -> Vec { value.iter().map(u8::to_ascii_lowercase).collect() } -pub(crate) fn effective_git_bool( - git: &GitRunner, - git_root: &Path, - git_config_args: &[String], - key: &str, -) -> io::Result> { - let mut command = git.command_for_cwd(git_root)?; - command - .env("GIT_OPTIONAL_LOCKS", "0") - .args(git_config_args) - .args(["config", "--type=bool", "--get", key]); - let output = git.output(command)?; - if output.status.code() == Some(1) && output.stdout.is_empty() && output.stderr.is_empty() { - return Ok(None); - } - if !output.status.success() { - return Err(io::Error::other(format!( - "git boolean config probe for {key} failed with status {}: {}", - output.status, - String::from_utf8_lossy(&output.stderr).trim() - ))); - } - match String::from_utf8_lossy(&output.stdout).trim() { - "true" => Ok(Some(true)), - "false" => Ok(Some(false)), - value => Err(io::Error::new( - io::ErrorKind::InvalidData, - format!("unexpected normalized Git boolean value for {key}: {value:?}"), - )), - } -} - fn quote_paths(paths: &BTreeSet) -> String { let shown = paths .iter() diff --git a/codex-rs/git-utils/src/exact_staging.rs b/codex-rs/git-utils/src/exact_staging.rs index 1ae755363a..8ff94ae796 100644 --- a/codex-rs/git-utils/src/exact_staging.rs +++ b/codex-rs/git-utils/src/exact_staging.rs @@ -4,17 +4,12 @@ use std::collections::BTreeSet; use std::io; use std::io::Seek; use std::io::Write; -use std::path::Path; use std::process::Stdio; -use crate::apply::run_git; use crate::exact_index_policy::ExactIndexPolicy; -use crate::exact_index_policy::effective_git_bool; use crate::exact_index_policy::resolve_exact_index_policy; -use crate::git_command::GitRunner; -use crate::git_config_sources::ensure_no_worktree_config_sources; +use crate::guarded_config::GuardedGitConfig; use crate::patch_paths::validate_patch_path; -use crate::safe_git::ensure_no_selected_git_add_filters; #[derive(Clone, Debug, Eq, PartialEq)] pub(crate) struct StagePathsResult { @@ -22,6 +17,12 @@ pub(crate) struct StagePathsResult { pub(crate) stderr: String, } +#[derive(Clone, Copy)] +enum StagingProvenance { + FromApply, + Standalone, +} + /// Stage only the literal paths supplied by a caller that has already applied /// the operation-specific containment and filesystem policy. /// @@ -35,19 +36,38 @@ pub(crate) struct StagePathsResult { /// bytes read for an already-confined index pathname. It cannot make this /// exact command recurse into, or create index entries for, descendant paths; /// the filter neutralizer also remains in force for the entire command. -pub(crate) fn update_index_exact_paths( - git: &GitRunner, - git_root: &Path, +pub(crate) fn update_index_exact_paths_from_apply( + config: &mut GuardedGitConfig<'_>, paths: &[String], content_filter_paths: &[String], - git_config_args: &[String], ) -> io::Result { - if paths.is_empty() { - return Ok(StagePathsResult { - exit_code: 0, - stderr: String::new(), - }); - } + update_index_exact_paths_common( + config, + paths, + content_filter_paths, + StagingProvenance::FromApply, + ) +} + +pub(crate) fn update_index_exact_paths_standalone( + config: &mut GuardedGitConfig<'_>, + paths: &[String], + content_filter_paths: &[String], +) -> io::Result { + update_index_exact_paths_common( + config, + paths, + content_filter_paths, + StagingProvenance::Standalone, + ) +} + +fn update_index_exact_paths_common( + config: &mut GuardedGitConfig<'_>, + paths: &[String], + content_filter_paths: &[String], + provenance: StagingProvenance, +) -> io::Result { for path in paths { if path.as_bytes().contains(&0) { return Err(io::Error::new( @@ -67,29 +87,34 @@ pub(crate) fn update_index_exact_paths( "content-filter staging path is not in the exact staging set", )); } + if matches!(provenance, StagingProvenance::FromApply) { + config.ensure_apply_filter_path_subset(content_filter_paths)?; + } + if paths.is_empty() { + return Ok(StagePathsResult { + exit_code: 0, + stderr: String::new(), + }); + } - ensure_no_worktree_config_sources(git, git_root, git_config_args)?; + let (paths, content_filter_paths) = + match resolve_exact_index_policy(config, paths, content_filter_paths)? { + ExactIndexPolicy::Proceed { + paths, + content_filter_paths, + } => (paths, content_filter_paths), + ExactIndexPolicy::Refuse { stderr } => { + return Ok(StagePathsResult { + exit_code: 1, + stderr, + }); + } + }; + if matches!(provenance, StagingProvenance::FromApply) { + config.ensure_apply_filter_path_subset(&content_filter_paths)?; + } - let (paths, content_filter_paths) = match resolve_exact_index_policy( - git, - git_root, - paths, - content_filter_paths, - git_config_args, - )? { - ExactIndexPolicy::Proceed { - paths, - content_filter_paths, - } => (paths, content_filter_paths), - ExactIndexPolicy::Refuse { stderr } => { - return Ok(StagePathsResult { - exit_code: 1, - stderr, - }); - } - }; - - let ignored = ignored_untracked_paths(git, git_root, &paths, git_config_args)?; + let ignored = ignored_untracked_paths(config, &paths)?; if !ignored.is_empty() { return Ok(StagePathsResult { exit_code: 1, @@ -99,32 +124,26 @@ pub(crate) fn update_index_exact_paths( ), }); } - if let Some(result) = sparse_checkout_policy(git, git_root, &paths, git_config_args)? { + if let Some(result) = sparse_checkout_policy(config, &paths)? { return Ok(result); } - let filter_guard = - ensure_no_selected_git_add_filters(git, git_root, &content_filter_paths, git_config_args)?; - let mut guarded_config = git_config_args.to_vec(); - guarded_config.extend_from_slice(filter_guard.git_config_args()); - - let mut update_args = vec![ - "--literal-pathspecs".to_string(), - "update-index".to_string(), - "--add".to_string(), - "--remove".to_string(), - "--".to_string(), - ]; - update_args.extend_from_slice(&paths); - let (exit_code, _stdout, stderr) = run_git(git, git_root, &guarded_config, &update_args)?; - Ok(StagePathsResult { exit_code, stderr }) + config.authorize_git_add_filter_paths(&content_filter_paths)?; + let mut command = config.update_index_literal_pathspecs_command()?; + command + .disable_optional_locks() + .args(["--add", "--remove", "--"]) + .args(&paths); + let output = command.output()?; + Ok(StagePathsResult { + exit_code: output.status.code().unwrap_or(-1), + stderr: String::from_utf8_lossy(&output.stderr).into_owned(), + }) } fn ignored_untracked_paths( - git: &GitRunner, - git_root: &Path, + config: &GuardedGitConfig<'_>, paths: &[String], - git_config_args: &[String], ) -> io::Result> { if paths.is_empty() { return Ok(BTreeSet::new()); @@ -138,15 +157,14 @@ fn ignored_untracked_paths( .map(|path| format!("./{path}")) .collect::>(); let input = nul_path_input(&probe_paths)?; - let mut command = git.command_for_cwd(git_root)?; + let mut command = config.check_ignore_command()?; command - .env("GIT_OPTIONAL_LOCKS", "0") - .args(git_config_args) + .disable_optional_locks() // `check-ignore --stdin` consumes pathnames rather than pathspecs and // rejects the global literal-pathspec mode as unsupported magic. - .args(["check-ignore", "--stdin", "-z"]) + .args(["--stdin", "-z"]) .stdin(Stdio::from(input)); - let output = git.output(command)?; + let output = command.output()?; match output.status.code() { Some(0) => { let ignored = parse_reported_paths(&output.stdout, &probe_paths, "ignored-path")? @@ -171,13 +189,10 @@ fn ignored_untracked_paths( } fn sparse_checkout_policy( - git: &GitRunner, - git_root: &Path, + config: &GuardedGitConfig<'_>, paths: &[String], - git_config_args: &[String], ) -> io::Result> { - if !effective_git_bool(git, git_root, git_config_args, "core.sparseCheckout")?.unwrap_or(false) - { + if !config.read_bool("core.sparseCheckout")?.unwrap_or(false) { return Ok(None); } @@ -185,12 +200,7 @@ fn sparse_checkout_policy( // `git-sparse-checkout` helper. Repository-controlled PATH entries are // outside the trusted executable boundary. `--list-cmds=builtins` is a // main-program capability query and cannot dispatch an external command. - let mut builtins_command = git.command_for_cwd(git_root)?; - builtins_command - .env("GIT_OPTIONAL_LOCKS", "0") - .args(git_config_args) - .arg("--list-cmds=builtins"); - let builtins = git.output(builtins_command)?; + let builtins = config.list_builtin_commands()?; let sparse_checkout_is_builtin = builtins.status.success() && std::str::from_utf8(&builtins.stdout) .is_ok_and(|output| output.lines().any(|command| command == "sparse-checkout")); @@ -203,13 +213,12 @@ fn sparse_checkout_policy( } let input = nul_path_input(paths)?; - let mut command = git.command_for_cwd(git_root)?; + let mut command = config.sparse_checkout_command()?; command - .env("GIT_OPTIONAL_LOCKS", "0") - .args(git_config_args) - .args(["sparse-checkout", "check-rules", "-z"]) + .disable_optional_locks() + .args(["check-rules", "-z"]) .stdin(Stdio::from(input)); - let output = git.output(command)?; + let output = command.output()?; if !output.status.success() { return Ok(Some(StagePathsResult { exit_code: output.status.code().unwrap_or(-1), diff --git a/codex-rs/git-utils/src/filter_sentinel.rs b/codex-rs/git-utils/src/filter_sentinel.rs index a38ebc6f0e..bb511d447d 100644 --- a/codex-rs/git-utils/src/filter_sentinel.rs +++ b/codex-rs/git-utils/src/filter_sentinel.rs @@ -51,6 +51,7 @@ pub(crate) fn classify_sentinel_filter_probes( } } +#[cfg(test)] pub(crate) fn sentinel_filter_probe_config_args( neutralization_args: &[String], driver: &str, diff --git a/codex-rs/git-utils/src/git_command.rs b/codex-rs/git-utils/src/git_command.rs index 2718c42e84..ca232e8704 100644 --- a/codex-rs/git-utils/src/git_command.rs +++ b/codex-rs/git-utils/src/git_command.rs @@ -9,6 +9,7 @@ use tokio::io::AsyncRead; use tokio::io::AsyncReadExt; use crate::errors::GitReadError; +use crate::git_config_environment::GitConfigEnvironmentSnapshot; #[cfg(test)] use crate::git_executable::git_executable_name; use crate::git_executable::harden_git_launch_environment; @@ -36,6 +37,7 @@ pub(crate) struct GitRunner { argv0: PathBuf, safe_path: std::ffi::OsString, authority: RepositoryAuthority, + config_environment: GitConfigEnvironmentSnapshot, } /// A Git command that can only be spawned through [`GitRunner::output`], @@ -117,6 +119,8 @@ impl GitAsyncCommand { impl GitRunner { pub(crate) fn for_cwd(cwd: &Path) -> Result { + #[cfg(test)] + GIT_RUNNER_CONSTRUCTION_COUNT.with(|count| count.set(count.get() + 1)); let authority = repository_authority_for_cwd(cwd)?; let search_path = std::env::var_os("PATH").ok_or(GitReadError::NoTrustedGit)?; Self::from_search_path(authority, &search_path) @@ -138,6 +142,7 @@ impl GitRunner { command.current_dir(parent); } harden_git_launch_environment(&mut command, &self.safe_path); + self.config_environment.apply_to(&mut command); GitCommand { inner: command } } @@ -180,6 +185,18 @@ impl GitRunner { self.authority.active_worktree_root() } + pub(crate) fn ensure_active_worktree_root(&self, root: &Path) -> io::Result<()> { + self.authority.ensure_active_worktree_root(root) + } + + pub(crate) fn ensure_repository_root_route(&self, root: &Path) -> io::Result<()> { + self.authority.ensure_repository_root_route(root) + } + + pub(crate) fn config_environment_value(&self, name: &str) -> Option<&OsStr> { + self.config_environment.value(name) + } + pub(crate) fn output(&self, mut command: GitCommand) -> io::Result { self.prepare_command_for_launch(&mut command.inner)?; command.inner.output() @@ -259,12 +276,18 @@ impl GitRunner { ) -> Result { authority.ensure_primary_authority()?; let selected = select_git_executable(&authority, search_path)?; + let config_environment = GitConfigEnvironmentSnapshot::capture().map_err(|error| { + GitReadError::InvalidConfigEnvironment { + reason: error.to_string(), + } + })?; Ok(Self { executable: selected.executable, #[cfg(any(unix, test))] argv0: selected.argv0, safe_path: selected.safe_path, authority, + config_environment, }) } @@ -275,11 +298,17 @@ impl GitRunner { ) -> Result { let authority = repository_authority_for_cwd(cwd)?; let safe_path = std::env::var_os("PATH").ok_or(GitReadError::NoTrustedGit)?; + let config_environment = GitConfigEnvironmentSnapshot::capture().map_err(|error| { + GitReadError::InvalidConfigEnvironment { + reason: error.to_string(), + } + })?; Ok(Self { argv0: executable.clone(), executable, safe_path, authority, + config_environment, }) } } @@ -305,6 +334,21 @@ async fn read_bounded_output( } } +#[cfg(test)] +thread_local! { + static GIT_RUNNER_CONSTRUCTION_COUNT: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +#[cfg(test)] +pub(crate) fn reset_git_runner_construction_count() { + GIT_RUNNER_CONSTRUCTION_COUNT.with(|count| count.set(0)); +} + +#[cfg(test)] +pub(crate) fn git_runner_construction_count() -> usize { + GIT_RUNNER_CONSTRUCTION_COUNT.with(std::cell::Cell::get) +} + pub(crate) fn repository_authority_for_cwd( cwd: &Path, ) -> Result { diff --git a/codex-rs/git-utils/src/git_command_tests.rs b/codex-rs/git-utils/src/git_command_tests.rs index 74fdba1814..b407d64e87 100644 --- a/codex-rs/git-utils/src/git_command_tests.rs +++ b/codex-rs/git-utils/src/git_command_tests.rs @@ -94,6 +94,110 @@ fn run_git_stdout(cwd: &Path, args: &[&str]) -> String { .to_string() } +#[tokio::test] +async fn runner_binds_config_environment_across_ambient_mutation() { + const CHILD: &str = "CODEX_GIT_CONFIG_BINDING_CHILD"; + if std::env::var_os(CHILD).is_none() { + let fixture = tempfile::tempdir().expect("fixture"); + let repo = fixture.path().join("repo"); + let safe_home = fixture.path().join("safe-home"); + let malicious_home = repo.join("malicious-home"); + std::fs::create_dir_all(&repo).expect("repo"); + std::fs::create_dir_all(&safe_home).expect("safe home"); + std::fs::create_dir_all(&malicious_home).expect("malicious home"); + run_git(&repo, &["init", "-q"]); + std::fs::write( + safe_home.join(".gitconfig"), + "[codex]\n\tbound = safe-home\n", + ) + .expect("safe home config"); + let malicious_global = repo.join("malicious.gitconfig"); + std::fs::write(&malicious_global, "[codex]\n\tbound = malicious\n") + .expect("malicious global config"); + std::fs::write( + malicious_home.join(".gitconfig"), + "[codex]\n\tbound = malicious-home\n", + ) + .expect("malicious home config"); + + let mut command = Command::new(std::env::current_exe().expect("test executable")); + isolate_git_command_environment(&mut command); + let output = command + .arg("git_command::tests::runner_binds_config_environment_across_ambient_mutation") + .arg("--exact") + .arg("--nocapture") + .env(CHILD, "1") + .env("RUST_TEST_THREADS", "1") + .env("CODEX_GIT_CONFIG_BINDING_ROOT", &repo) + .env("CODEX_GIT_CONFIG_BINDING_MALICIOUS", &malicious_global) + .env("CODEX_GIT_CONFIG_BINDING_MALICIOUS_HOME", &malicious_home) + .env("HOME", &safe_home) + .env("GIT_CONFIG_NOSYSTEM", "1") + .env("GIT_CONFIG_COUNT", "1") + .env("GIT_CONFIG_KEY_0", "codex.count") + .env("GIT_CONFIG_VALUE_0", "safe-count") + .env_remove("GIT_CONFIG_GLOBAL") + .env_remove("GIT_CONFIG_SYSTEM") + .env_remove("GIT_CONFIG_PARAMETERS") + .output() + .expect("isolated config binding test"); + assert!( + output.status.success(), + "isolated config binding test failed:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + return; + } + + let root = PathBuf::from(std::env::var_os("CODEX_GIT_CONFIG_BINDING_ROOT").expect("root")); + let malicious = + std::env::var_os("CODEX_GIT_CONFIG_BINDING_MALICIOUS").expect("malicious config"); + let malicious_home = + std::env::var_os("CODEX_GIT_CONFIG_BINDING_MALICIOUS_HOME").expect("malicious home"); + let runner = GitRunner::for_cwd_io(&root).expect("runner with captured config environment"); + + // SAFETY: the parent starts an isolated test process that runs only this + // exact test with one harness thread. No other application thread reads or + // writes these variables, and the process exits after the assertions. + unsafe { + std::env::set_var("GIT_CONFIG_GLOBAL", malicious); + std::env::set_var("HOME", malicious_home); + std::env::set_var("GIT_CONFIG_VALUE_0", "malicious-count"); + std::env::set_var( + "GIT_CONFIG_PARAMETERS", + "'codex.count'='malicious-parameter'", + ); + } + + for (key, expected) in [("codex.bound", "safe-home"), ("codex.count", "safe-count")] { + let mut command = runner.command_for_cwd(&root).expect("bound command"); + command.args(["config", "--get", key]); + let output = runner.output(command).expect("bound config read"); + assert!( + output.status.success(), + "{key}: {}", + String::from_utf8_lossy(&output.stderr) + ); + assert_eq!(String::from_utf8_lossy(&output.stdout).trim(), expected); + + let mut command = runner + .async_command_for_cwd(&root) + .expect("bound async command"); + command.args(["config", "--get", key]); + let output = runner + .output_async_bounded(command, MAX_INTERNAL_GIT_OUTPUT_BYTES) + .await + .expect("bound async config read"); + assert!( + output.status.success(), + "async {key}: {}", + String::from_utf8_lossy(&output.stderr) + ); + assert_eq!(String::from_utf8_lossy(&output.stdout).trim(), expected); + } +} + fn commit_all(cwd: &Path, message: &str) { run_git( cwd, @@ -547,6 +651,55 @@ fn git_read_error_io_kind_table_is_exhaustive() { }, io::ErrorKind::InvalidData, ), + ( + GitReadError::InvalidConfigEnvironment { + reason: "malformed GIT_CONFIG_COUNT".to_string(), + }, + io::ErrorKind::InvalidData, + ), + ( + GitReadError::RepositoryRootMismatch { + expected_root: PathBuf::from("expected"), + reported_root: PathBuf::from("reported"), + }, + io::ErrorKind::PermissionDenied, + ), + ( + GitReadError::CommandTimedOut { + operation: "status".to_string(), + }, + io::ErrorKind::TimedOut, + ), + ( + GitReadError::CommandFailed { + operation: "status".to_string(), + exit_code: Some(1), + }, + io::ErrorKind::Other, + ), + ( + GitReadError::InvalidOutput { + operation: "status".to_string(), + }, + io::ErrorKind::InvalidData, + ), + ( + GitReadError::AuthorityRefused { + operation: "status".to_string(), + }, + io::ErrorKind::PermissionDenied, + ), + ( + GitReadError::FilterSelectionProbeLimitExceeded { max_probes: 16 }, + io::ErrorKind::PermissionDenied, + ), + ( + GitReadError::SelectedExecutableFilter { + driver: "demo".to_string(), + path: "file.txt".to_string(), + }, + io::ErrorKind::PermissionDenied, + ), ] { assert_eq!(error.io_kind(), expected, "{error}"); assert_eq!(error.into_io_error().kind(), expected); diff --git a/codex-rs/git-utils/src/git_config_environment.rs b/codex-rs/git-utils/src/git_config_environment.rs new file mode 100644 index 0000000000..43d80a3afd --- /dev/null +++ b/codex-rs/git-utils/src/git_config_environment.rs @@ -0,0 +1,136 @@ +use std::collections::BTreeMap; +use std::ffi::OsStr; +use std::ffi::OsString; +use std::io; +use std::process::Command; + +const FIXED_CONFIG_ENVIRONMENT_KEYS: &[&str] = &[ + "GIT_CONFIG_GLOBAL", + "GIT_CONFIG_SYSTEM", + "GIT_CONFIG_NOSYSTEM", + "GIT_CONFIG_COUNT", + "GIT_CONFIG_PARAMETERS", + "HOME", + "XDG_CONFIG_HOME", + #[cfg(windows)] + "APPDATA", + #[cfg(windows)] + "PROGRAMDATA", + #[cfg(windows)] + "USERPROFILE", + #[cfg(windows)] + "HOMEDRIVE", + #[cfg(windows)] + "HOMEPATH", +]; + +// Git config command entries are an untrusted process input. Keep capture +// bounded rather than allocating attacker-selected amounts of memory before +// Git gets a chance to reject an unreasonable count. +const MAX_CONFIG_ENVIRONMENT_ENTRIES: usize = 1024; + +/// Exact config-relevant process environment bound to one +/// [`GitRunner`](crate::git_command::GitRunner). +/// +/// Every ordinary Git child receives these captured values (including explicit +/// removals for variables that were absent). Commands that intentionally +/// isolate a config source may override them after command construction. +pub(crate) struct GitConfigEnvironmentSnapshot { + entries: Box<[(OsString, Option)]>, +} + +impl std::fmt::Debug for GitConfigEnvironmentSnapshot { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter + .debug_struct("GitConfigEnvironmentSnapshot") + .field("captured_entries", &self.entries.len()) + .finish() + } +} + +impl GitConfigEnvironmentSnapshot { + pub(crate) fn capture() -> io::Result { + let environment = std::env::vars_os().collect::>(); + Self::capture_from(|name| captured_environment_value(&environment, name)) + } + + fn capture_from(mut value_for: impl FnMut(&OsStr) -> Option) -> io::Result { + let mut entries = FIXED_CONFIG_ENVIRONMENT_KEYS + .iter() + .map(|name| (OsString::from(name), value_for(OsStr::new(name)))) + .collect::>(); + + if let Some(raw_count) = value_for(OsStr::new("GIT_CONFIG_COUNT")) { + let count = raw_count + .to_str() + .ok_or_else(|| invalid_environment("non-UTF-8 GIT_CONFIG_COUNT"))? + .parse::() + .map_err(|_| invalid_environment("invalid GIT_CONFIG_COUNT"))?; + if count > MAX_CONFIG_ENVIRONMENT_ENTRIES { + return Err(invalid_environment( + "GIT_CONFIG_COUNT exceeds the supported safety bound", + )); + } + for index in 0..count { + for prefix in ["GIT_CONFIG_KEY_", "GIT_CONFIG_VALUE_"] { + let name = OsString::from(format!("{prefix}{index}")); + let value = value_for(&name); + entries.push((name, value)); + } + } + } + + Ok(Self { + entries: entries.into_boxed_slice(), + }) + } + + pub(crate) fn apply_to(&self, command: &mut Command) { + for (name, value) in &self.entries { + match value { + Some(value) => { + command.env(name, value); + } + None => { + command.env_remove(name); + } + } + } + } + + pub(crate) fn value(&self, name: &str) -> Option<&OsStr> { + self.entries + .iter() + .find_map(|(candidate, value)| (candidate == name).then_some(value.as_deref())) + .flatten() + } +} + +#[cfg(not(windows))] +fn captured_environment_value( + environment: &BTreeMap, + name: &OsStr, +) -> Option { + environment.get(name).cloned() +} + +#[cfg(windows)] +fn captured_environment_value( + environment: &BTreeMap, + name: &OsStr, +) -> Option { + environment.iter().find_map(|(candidate, value)| { + candidate + .to_string_lossy() + .eq_ignore_ascii_case(&name.to_string_lossy()) + .then(|| value.clone()) + }) +} + +fn invalid_environment(message: &str) -> io::Error { + io::Error::new(io::ErrorKind::InvalidData, message) +} + +#[cfg(test)] +#[path = "git_config_environment_tests.rs"] +mod tests; diff --git a/codex-rs/git-utils/src/git_config_environment_tests.rs b/codex-rs/git-utils/src/git_config_environment_tests.rs new file mode 100644 index 0000000000..3cdec9a790 --- /dev/null +++ b/codex-rs/git-utils/src/git_config_environment_tests.rs @@ -0,0 +1,126 @@ +use super::*; +use pretty_assertions::assert_eq; + +#[test] +fn snapshot_binds_present_empty_and_absent_fixed_and_indexed_values() { + let values = BTreeMap::from([ + (OsString::from("GIT_CONFIG_GLOBAL"), OsString::from("")), + ( + OsString::from("GIT_CONFIG_PARAMETERS"), + OsString::from("'safe.parameter'='present'"), + ), + (OsString::from("GIT_CONFIG_NOSYSTEM"), OsString::from("1")), + (OsString::from("HOME"), OsString::from("/safe/home")), + ( + OsString::from("XDG_CONFIG_HOME"), + OsString::from("/safe/xdg"), + ), + (OsString::from("GIT_CONFIG_COUNT"), OsString::from("2")), + ( + OsString::from("GIT_CONFIG_KEY_0"), + OsString::from("safe.one"), + ), + ( + OsString::from("GIT_CONFIG_VALUE_0"), + OsString::from("present"), + ), + ( + OsString::from("GIT_CONFIG_KEY_1"), + OsString::from("safe.two"), + ), + ]); + let snapshot = GitConfigEnvironmentSnapshot::capture_from(|name| values.get(name).cloned()) + .expect("capture environment"); + + assert_eq!(snapshot.value("GIT_CONFIG_GLOBAL"), Some(OsStr::new(""))); + assert_eq!(snapshot.value("HOME"), Some(OsStr::new("/safe/home"))); + assert_eq!(snapshot.value("GIT_CONFIG_SYSTEM"), None); + assert_eq!(snapshot.value("GIT_CONFIG_NOSYSTEM"), Some(OsStr::new("1"))); + assert_eq!( + snapshot.value("GIT_CONFIG_PARAMETERS"), + Some(OsStr::new("'safe.parameter'='present'")) + ); + assert_eq!( + snapshot.value("GIT_CONFIG_KEY_0"), + Some(OsStr::new("safe.one")) + ); + assert_eq!(snapshot.value("GIT_CONFIG_VALUE_1"), None); + + let mut command = Command::new("git"); + command + .env("GIT_CONFIG_SYSTEM", "/later/system") + .env("GIT_CONFIG_VALUE_1", "later"); + snapshot.apply_to(&mut command); + let child_environment = command + .get_envs() + .map(|(name, value)| (name.to_owned(), value.map(OsStr::to_owned))) + .collect::>(); + assert_eq!( + child_environment.get(OsStr::new("GIT_CONFIG_SYSTEM")), + Some(&None) + ); + assert_eq!( + child_environment.get(OsStr::new("GIT_CONFIG_VALUE_1")), + Some(&None) + ); + assert_eq!( + child_environment.get(OsStr::new("HOME")), + Some(&Some(OsString::from("/safe/home"))) + ); +} + +#[test] +fn snapshot_rejects_malformed_or_unbounded_command_config_count() { + for count in [OsString::from("invalid"), OsString::from("1025")] { + let error = GitConfigEnvironmentSnapshot::capture_from(|name| { + (name == "GIT_CONFIG_COUNT").then(|| count.clone()) + }) + .expect_err("reject count"); + assert_eq!(error.kind(), io::ErrorKind::InvalidData); + } +} + +#[cfg(unix)] +#[test] +fn snapshot_preserves_non_utf8_config_path_bytes() { + use std::os::unix::ffi::OsStringExt; + + let raw = OsString::from_vec(vec![b'/', b't', b'm', b'p', b'/', 0xff]); + let snapshot = GitConfigEnvironmentSnapshot::capture_from(|name| { + (name == "GIT_CONFIG_GLOBAL").then(|| raw.clone()) + }) + .expect("capture non-UTF-8 path"); + assert_eq!(snapshot.value("GIT_CONFIG_GLOBAL"), Some(raw.as_os_str())); +} + +#[cfg(windows)] +#[test] +fn snapshot_matches_mixed_case_windows_environment_names() { + let environment = BTreeMap::from([ + (OsString::from("git_config_count"), OsString::from("1")), + ( + OsString::from("Git_Config_Key_0"), + OsString::from("safe.key"), + ), + ( + OsString::from("gIt_CoNfIg_VaLuE_0"), + OsString::from("safe-value"), + ), + (OsString::from("Home"), OsString::from(r"C:\safe-home")), + ]); + let snapshot = GitConfigEnvironmentSnapshot::capture_from(|name| { + captured_environment_value(&environment, name) + }) + .expect("capture mixed-case Windows environment"); + + assert_eq!(snapshot.value("GIT_CONFIG_COUNT"), Some(OsStr::new("1"))); + assert_eq!( + snapshot.value("GIT_CONFIG_KEY_0"), + Some(OsStr::new("safe.key")) + ); + assert_eq!( + snapshot.value("GIT_CONFIG_VALUE_0"), + Some(OsStr::new("safe-value")) + ); + assert_eq!(snapshot.value("HOME"), Some(OsStr::new(r"C:\safe-home"))); +} diff --git a/codex-rs/git-utils/src/git_config_sources.rs b/codex-rs/git-utils/src/git_config_sources.rs index 121ffb7cf2..1eb3abcdec 100644 --- a/codex-rs/git-utils/src/git_config_sources.rs +++ b/codex-rs/git-utils/src/git_config_sources.rs @@ -237,7 +237,7 @@ async fn ensure_no_worktree_config_sources_impl( // Environment, HOME, and XDG paths are attacker-controlled byte strings. // Classify their exact OsString spelling before selected Git can open or // newline-delimit one of them. - for (description, candidate) in legacy_primary_config_source_candidates()? { + for (description, candidate) in legacy_primary_config_source_candidates(git)? { if !is_disabled_primary_config_path(&candidate) { reject_source(git, &git_root, &candidate, description)?; } diff --git a/codex-rs/git-utils/src/git_config_sources/primary_sources.rs b/codex-rs/git-utils/src/git_config_sources/primary_sources.rs index eba7ede571..4bccb0011a 100644 --- a/codex-rs/git-utils/src/git_config_sources/primary_sources.rs +++ b/codex-rs/git-utils/src/git_config_sources/primary_sources.rs @@ -1,3 +1,4 @@ +use std::ffi::OsStr; use std::io; use std::path::Path; use std::path::PathBuf; @@ -12,7 +13,9 @@ pub(super) fn default_system_config_source_candidates( git: &GitRunner, cwd: &Path, ) -> io::Result> { - if git_env_bool("GIT_CONFIG_NOSYSTEM")? || std::env::var_os("GIT_CONFIG_SYSTEM").is_some() { + if git_env_bool(git, "GIT_CONFIG_NOSYSTEM")? + || git.config_environment_value("GIT_CONFIG_SYSTEM").is_some() + { return Ok(Vec::new()); } // `GIT_CONFIG_SYSTEM` was added to `git var` in Git 2.42. The PSEC-4394 @@ -34,7 +37,9 @@ pub(super) async fn default_system_config_source_candidates_async( git: &GitRunner, cwd: &Path, ) -> io::Result> { - if git_env_bool("GIT_CONFIG_NOSYSTEM")? || std::env::var_os("GIT_CONFIG_SYSTEM").is_some() { + if git_env_bool(git, "GIT_CONFIG_NOSYSTEM")? + || git.config_environment_value("GIT_CONFIG_SYSTEM").is_some() + { return Ok(Vec::new()); } let Some(paths) = git_var_config_paths_async(git, cwd, "GIT_CONFIG_SYSTEM").await? else { @@ -50,16 +55,19 @@ pub(super) fn selected_git_home_config_candidates( git: &GitRunner, cwd: &Path, ) -> io::Result> { - if std::env::var_os("GIT_CONFIG_GLOBAL").is_some() { + if git.config_environment_value("GIT_CONFIG_GLOBAL").is_some() { return Ok(Vec::new()); } #[cfg(not(windows))] - if std::env::var_os("HOME").is_none() { + if git.config_environment_value("HOME").is_none() { return Ok(Vec::new()); } let dot_gitconfig = selected_git_path_candidate(git, cwd, "~/.gitconfig")?; let mut candidates = vec![dot_gitconfig.clone()]; - if std::env::var_os("XDG_CONFIG_HOME").is_none_or(|path| path.is_empty()) { + if git + .config_environment_value("XDG_CONFIG_HOME") + .is_none_or(OsStr::is_empty) + { let home = dot_gitconfig .parent() .ok_or_else(|| invalid_config_source("selected Git HOME has no parent"))?; @@ -72,16 +80,19 @@ pub(super) async fn selected_git_home_config_candidates_async( git: &GitRunner, cwd: &Path, ) -> io::Result> { - if std::env::var_os("GIT_CONFIG_GLOBAL").is_some() { + if git.config_environment_value("GIT_CONFIG_GLOBAL").is_some() { return Ok(Vec::new()); } #[cfg(not(windows))] - if std::env::var_os("HOME").is_none() { + if git.config_environment_value("HOME").is_none() { return Ok(Vec::new()); } let dot_gitconfig = selected_git_path_candidate_async(git, cwd, "~/.gitconfig").await?; let mut candidates = vec![dot_gitconfig.clone()]; - if std::env::var_os("XDG_CONFIG_HOME").is_none_or(|path| path.is_empty()) { + if git + .config_environment_value("XDG_CONFIG_HOME") + .is_none_or(OsStr::is_empty) + { let home = dot_gitconfig .parent() .ok_or_else(|| invalid_config_source("selected Git HOME has no parent"))?; @@ -94,7 +105,9 @@ pub(super) fn selected_git_prefix_system_candidate( git: &GitRunner, cwd: &Path, ) -> io::Result> { - if git_env_bool("GIT_CONFIG_NOSYSTEM")? || std::env::var_os("GIT_CONFIG_SYSTEM").is_some() { + if git_env_bool(git, "GIT_CONFIG_NOSYSTEM")? + || git.config_environment_value("GIT_CONFIG_SYSTEM").is_some() + { return Ok(None); } selected_git_path_candidate(git, cwd, "%(prefix)/etc/gitconfig").map(Some) @@ -104,7 +117,9 @@ pub(super) async fn selected_git_prefix_system_candidate_async( git: &GitRunner, cwd: &Path, ) -> io::Result> { - if git_env_bool("GIT_CONFIG_NOSYSTEM")? || std::env::var_os("GIT_CONFIG_SYSTEM").is_some() { + if git_env_bool(git, "GIT_CONFIG_NOSYSTEM")? + || git.config_environment_value("GIT_CONFIG_SYSTEM").is_some() + { return Ok(None); } selected_git_path_candidate_async(git, cwd, "%(prefix)/etc/gitconfig") @@ -262,24 +277,25 @@ fn parse_git_var_paths(output: &[u8]) -> io::Result> { .collect() } -pub(super) fn legacy_primary_config_source_candidates() -> io::Result> -{ +pub(super) fn legacy_primary_config_source_candidates( + git: &GitRunner, +) -> io::Result> { let mut candidates = Vec::new(); - match std::env::var_os("GIT_CONFIG_GLOBAL") { + match git.config_environment_value("GIT_CONFIG_GLOBAL") { Some(path) if !path.is_empty() => { candidates.push(("GIT_CONFIG_GLOBAL", PathBuf::from(path))); } Some(_) => {} None => { - let homes = git_home_directories(); - match std::env::var_os("XDG_CONFIG_HOME") { + let homes = git_home_directories(git); + match git.config_environment_value("XDG_CONFIG_HOME") { Some(xdg) if !xdg.is_empty() => candidates.push(( "XDG_CONFIG_HOME Git config", PathBuf::from(xdg).join("git/config"), )), _ => { #[cfg(windows)] - if let Some(app_data) = std::env::var_os("APPDATA") + if let Some(app_data) = git.config_environment_value("APPDATA") && !app_data.is_empty() { candidates.push(( @@ -300,15 +316,15 @@ pub(super) fn legacy_primary_config_source_candidates() -> io::Result { candidates.push(("GIT_CONFIG_SYSTEM", PathBuf::from(path))); } Some(_) => {} None => { #[cfg(windows)] - if let Some(program_data) = std::env::var_os("PROGRAMDATA") + if let Some(program_data) = git.config_environment_value("PROGRAMDATA") && !program_data.is_empty() { candidates.push(( @@ -322,24 +338,25 @@ pub(super) fn legacy_primary_config_source_candidates() -> io::Result Vec { - if let Some(home) = std::env::var_os("HOME") { - return vec![home]; +fn git_home_directories(git: &GitRunner) -> Vec { + if let Some(home) = git.config_environment_value("HOME") { + return vec![home.to_owned()]; } #[cfg(windows)] { let mut homes = Vec::new(); - if let (Some(drive), Some(path)) = - (std::env::var_os("HOMEDRIVE"), std::env::var_os("HOMEPATH")) - { - let mut home = drive; + if let (Some(drive), Some(path)) = ( + git.config_environment_value("HOMEDRIVE"), + git.config_environment_value("HOMEPATH"), + ) { + let mut home = drive.to_owned(); home.push(path); homes.push(home); } - if let Some(profile) = std::env::var_os("USERPROFILE") - && !homes.contains(&profile) + if let Some(profile) = git.config_environment_value("USERPROFILE") + && !homes.iter().any(|home| home == profile) { - homes.push(profile); + homes.push(profile.to_owned()); } homes } @@ -355,8 +372,8 @@ fn home_config_path(home: &std::ffi::OsStr, suffix: &str) -> PathBuf { } } -fn git_env_bool(name: &str) -> io::Result { - let Some(value) = std::env::var_os(name) else { +fn git_env_bool(git: &GitRunner, name: &str) -> io::Result { + let Some(value) = git.config_environment_value(name) else { return Ok(false); }; let value = value diff --git a/codex-rs/git-utils/src/git_config_sources_tests.rs b/codex-rs/git-utils/src/git_config_sources_tests.rs index 77b4c0fc9b..ed4727b422 100644 --- a/codex-rs/git-utils/src/git_config_sources_tests.rs +++ b/codex-rs/git-utils/src/git_config_sources_tests.rs @@ -811,6 +811,33 @@ fn command_scoped_include_paths_follow_the_same_boundary() { ); } +#[tokio::test] +async fn blocking_and_async_source_authorization_share_the_same_include_policy() { + let repo = init_repo(); + let root = repo.path(); + let git = GitRunner::for_cwd_io(root).expect("Git runner"); + + ensure_no_worktree_config_sources(&git, root, &[]).expect("blocking safe source graph"); + ensure_no_worktree_config_sources_async(&git, root, &[]) + .await + .expect("async safe source graph"); + + let worktree_include = root.join("attacker.gitconfig"); + std::fs::write(&worktree_include, "[codex]\n\tvalue = attacker\n").expect("worktree include"); + add_include( + root, + "include.path", + worktree_include.to_str().expect("UTF-8 fixture path"), + ); + let blocking = ensure_no_worktree_config_sources(&git, root, &[]) + .expect_err("blocking authorization must reject worktree include"); + let asynchronous = ensure_no_worktree_config_sources_async(&git, root, &[]) + .await + .expect_err("async authorization must reject worktree include"); + assert_eq!(blocking.kind(), io::ErrorKind::PermissionDenied); + assert_eq!(asynchronous.kind(), blocking.kind()); +} + #[test] fn rejects_every_duplicate_and_nested_include_target() { for unsafe_first in [true, false] { diff --git a/codex-rs/git-utils/src/guarded_config.rs b/codex-rs/git-utils/src/guarded_config.rs new file mode 100644 index 0000000000..6268668398 --- /dev/null +++ b/codex-rs/git-utils/src/guarded_config.rs @@ -0,0 +1,1370 @@ +use std::collections::BTreeMap; +use std::ffi::OsStr; +use std::io; +use std::io::Seek; +use std::path::Path; +use std::path::PathBuf; +use std::process::Stdio; +use std::sync::Arc; + +use crate::FsmonitorOverride; +use crate::FsmonitorProbeRunner; +use crate::git_command::GitAsyncCommand; +use crate::git_command::GitCommand; +use crate::git_command::GitRunner; +use crate::git_command::MAX_INTERNAL_GIT_OUTPUT_BYTES; +use crate::git_config::GitConfigEntry; +use crate::git_config::read_effective_config_with_fallback; +use crate::git_config::read_effective_config_with_fallback_async; +use crate::git_config_sources::ensure_no_worktree_config_sources; +use crate::git_config_sources::ensure_no_worktree_config_sources_async; +use crate::safe_git::DISABLED_HOOKS_PATH; +use crate::safe_git::ExecutableFilterDrivers; +use crate::safe_git::FilterAttributeValue; +use crate::safe_git::FilterExecution; +use crate::safe_git::FilterPolicyRole; +use crate::safe_git::FilterPolicySnapshot; +use crate::safe_git::MAX_EXECUTABLE_FILTER_DRIVERS; +use crate::safe_git::SelectedFilterPolicy; +use crate::safe_git::SentinelFilterProbeBudget; +use crate::safe_git::SentinelFilterProbeResolution; +use crate::safe_git::build_filter_policy_snapshot; +use crate::safe_git::classify_selected_filter; +use crate::safe_git::classify_sentinel_filter_probes; +use crate::safe_git::executable_filter_drivers; +use crate::safe_git::git_path_argument; +use crate::safe_git::parse_filter_attributes; +use crate::safe_git::parse_nul_paths; +use crate::safe_git::validate_executable_driver_count; +use crate::safe_git::write_nul_paths; + +const MAX_STATUS_TRACKED_PATHS: usize = 250_000; +const FILTER_NEUTRALIZATION_PLAN: [(&str, &str); 4] = [ + ("clean", ""), + ("smudge", ""), + ("process", ""), + ("required", "false"), +]; + +mod merge_overlay; +use merge_overlay::SealedMergeConfigOverride; +#[cfg(test)] +pub(crate) use merge_overlay::merge_attribute_read_count; +#[cfg(test)] +pub(crate) use merge_overlay::merge_config_read_count; +#[cfg(test)] +pub(crate) use merge_overlay::merge_overlay_count; +#[cfg(test)] +pub(crate) use merge_overlay::reset_merge_policy_counts; +mod reverse_probe; + +/// Proof that one exact Git config invocation has no worktree-controlled +/// source routes for one runner and canonical repository root. +/// +/// The capability deliberately owns the ordered base arguments and cannot be +/// cloned or rebound to another runner, root, or command environment. +pub(crate) struct ValidatedConfigSources<'git> { + git: &'git GitRunner, + canonical_root: PathBuf, + base_config_args: Box<[String]>, +} + +impl<'git> ValidatedConfigSources<'git> { + fn authorize( + git: &'git GitRunner, + canonical_root: &Path, + base_config_args: Vec, + ) -> io::Result { + #[cfg(test)] + CONFIG_SOURCE_AUTHORIZATION_COUNT.with(|count| count.set(count.get() + 1)); + + validate_base_config_args(&base_config_args)?; + let canonical_root = std::fs::canonicalize(canonical_root)?; + git.ensure_active_worktree_root(&canonical_root)?; + ensure_no_worktree_config_sources(git, &canonical_root, &base_config_args)?; + Ok(Self { + git, + canonical_root, + base_config_args: base_config_args.into_boxed_slice(), + }) + } + + async fn authorize_async( + git: &'git GitRunner, + canonical_root: &Path, + base_config_args: Vec, + ) -> io::Result { + #[cfg(test)] + CONFIG_SOURCE_AUTHORIZATION_COUNT.with(|count| count.set(count.get() + 1)); + + validate_base_config_args(&base_config_args)?; + let canonical_root = std::fs::canonicalize(canonical_root)?; + git.ensure_active_worktree_root(&canonical_root)?; + ensure_no_worktree_config_sources_async(git, &canonical_root, &base_config_args).await?; + Ok(Self { + git, + canonical_root, + base_config_args: base_config_args.into_boxed_slice(), + }) + } + + fn read_effective( + &self, + pattern: &str, + probe: &str, + ) -> io::Result> { + read_effective_config_with_fallback( + self.git, + &self.canonical_root, + &self.base_config_args, + pattern, + probe, + ) + } + + fn read_bool(&self, key: &str) -> io::Result> { + let mut command = self.git.command_for_cwd(&self.canonical_root)?; + command + .env("GIT_OPTIONAL_LOCKS", "0") + .args(&self.base_config_args) + .args(["config", "--type=bool", "--get", key]); + let output = self.git.output(command)?; + parse_bool_output(&output, key) + } + + async fn read_effective_async( + &self, + pattern: &str, + probe: &str, + ) -> io::Result> { + read_effective_config_with_fallback_async( + self.git, + &self.canonical_root, + &self.base_config_args, + pattern, + probe, + ) + .await + } + + async fn read_bool_async(&self, key: &str) -> io::Result> { + let mut command = self.git.async_command_for_cwd(&self.canonical_root)?; + command + .env("GIT_OPTIONAL_LOCKS", "0") + .args(&self.base_config_args) + .args(["config", "--type=bool", "--get", key]); + let output = self + .git + .output_async_bounded(command, MAX_INTERNAL_GIT_OUTPUT_BYTES) + .await?; + parse_bool_output(&output, key) + } +} + +fn parse_bool_output(output: &std::process::Output, key: &str) -> io::Result> { + if output.status.code() == Some(1) && output.stdout.is_empty() && output.stderr.is_empty() { + return Ok(None); + } + if !output.status.success() { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + format!( + "Git boolean config probe for {key:?} failed with status {}: {}", + output.status, + String::from_utf8_lossy(&output.stderr).trim() + ), + )); + } + match String::from_utf8_lossy(&output.stdout).trim() { + "true" => Ok(Some(true)), + "false" => Ok(Some(false)), + value => Err(io::Error::new( + io::ErrorKind::InvalidData, + format!("unexpected normalized Git boolean value {value:?} for {key:?}"), + )), + } +} + +fn validate_base_config_args(args: &[String]) -> io::Result<()> { + let mut pairs = args.chunks_exact(2); + for pair in &mut pairs { + let Some((key, _value)) = pair[1].split_once('=') else { + return Err(invalid_base_config_args()); + }; + if pair[0] != "-c" || key.is_empty() { + return Err(invalid_base_config_args()); + } + } + if !pairs.remainder().is_empty() { + return Err(invalid_base_config_args()); + } + Ok(()) +} + +fn invalid_base_config_args() -> io::Error { + io::Error::new( + io::ErrorKind::InvalidInput, + "guarded Git base config must contain only ordered -c key=value pairs", + ) +} + +/// Operation-owned Git configuration capability. +/// +/// All operation children are rooted at the authorized repository, inherit +/// the exact frozen base invocation, receive fixed library safety scalars, and +/// retain any sealed filter override for the capability lifetime. +pub(crate) struct GuardedGitConfig<'git> { + sources: ValidatedConfigSources<'git>, + identity: Arc, + // Ordered, typed snapshots keep each sealed filter overlay alive through + // every later child. Downstream staging may attach a fresh Git-add policy + // without rebuilding or weakening the source authorization. + filters: Vec, + // At most one merge-driver neutralizer may be attached. Command assembly + // places it between the apply and Git-add filter slots regardless of + // attachment timing. + merge: Option, + merge_policy_installed: bool, + // Status is a mutually exclusive operation state rather than another + // mutation overlay. `Some` with no neutralizer records the zero-driver + // case and is still required before the final status sink can run. + status: Option, +} + +struct StatusFilterPolicySnapshot { + neutralizer: Option, + fsmonitor: Option, +} + +#[derive(Debug, thiserror::Error)] +#[error("executable filter {driver:?} is selected for {path:?}")] +pub(crate) struct SelectedStatusFilterRefusal { + driver: String, + path: Vec, +} + +impl SelectedStatusFilterRefusal { + pub(crate) fn driver(&self) -> &str { + &self.driver + } + + pub(crate) fn path(&self) -> &[u8] { + &self.path + } +} + +#[derive(Debug, thiserror::Error)] +#[error("Git resolved {reported:?} instead of expected root {expected:?}")] +pub(crate) struct StatusRootMismatch { + expected: PathBuf, + reported: PathBuf, +} + +#[derive(Debug, thiserror::Error)] +#[error("selected Git authority has no active worktree root")] +pub(crate) struct NoActiveStatusWorktree; + +#[derive(Debug, thiserror::Error)] +#[error("Git filter attribute selection exceeded its {max_probes}-probe limit")] +pub(crate) struct StatusFilterProbeLimitExceeded { + max_probes: usize, +} + +#[derive(Debug, thiserror::Error)] +#[error("{description} failed with exit code {exit_code:?}")] +pub(crate) struct StatusPolicyCommandFailure { + description: String, + exit_code: Option, +} + +impl StatusPolicyCommandFailure { + pub(crate) fn exit_code(&self) -> Option { + self.exit_code + } +} + +impl StatusFilterProbeLimitExceeded { + pub(crate) fn max_probes(&self) -> usize { + self.max_probes + } +} + +impl StatusRootMismatch { + pub(crate) fn expected(&self) -> &Path { + &self.expected + } + + pub(crate) fn reported(&self) -> &Path { + &self.reported + } +} + +struct CapabilityIdentity; + +#[derive(Clone, Copy)] +enum BoundSubcommand { + Apply, + CheckAttr, + CheckIgnore, + HashObject, + ListBuiltinCommands, + LsFiles, + RevParse, + SparseCheckout, + UpdateIndexLiteralPathspecs, +} + +impl BoundSubcommand { + fn append_to(self, command: &mut GitCommand) { + match self { + Self::Apply => { + command.arg("apply"); + } + Self::CheckAttr => { + command.arg("check-attr"); + } + Self::CheckIgnore => { + command.arg("check-ignore"); + } + Self::HashObject => { + command.arg("hash-object"); + } + Self::ListBuiltinCommands => { + command.arg("--list-cmds=builtins"); + } + Self::LsFiles => { + command.arg("ls-files"); + } + Self::RevParse => { + command.arg("rev-parse"); + } + Self::SparseCheckout => { + command.arg("sparse-checkout"); + } + Self::UpdateIndexLiteralPathspecs => { + command.args(["--literal-pathspecs", "update-index"]); + } + } + } +} + +/// Async counterpart to [`GuardedGitCommand`]. The raw Tokio command never +/// leaves this module, so status callers cannot change its cwd, config +/// invocation, overlay order, or final metadata revalidation. +struct GuardedAsyncGitCommand<'operation, 'git> { + operation: &'operation GuardedGitConfig<'git>, + inner: GitAsyncCommand, +} + +impl GuardedAsyncGitCommand<'_, '_> { + fn arg(&mut self, arg: impl AsRef) -> &mut Self { + self.inner.arg(arg); + self + } + + fn args(&mut self, args: I) -> &mut Self + where + I: IntoIterator, + S: AsRef, + { + self.inner.args(args); + self + } + + fn disable_optional_locks(&mut self) -> &mut Self { + self.inner.env("GIT_OPTIONAL_LOCKS", "0"); + self + } + + fn stdin(&mut self, config: impl Into) -> &mut Self { + self.inner.stdin(config); + self + } + + async fn output(self) -> io::Result { + self.operation + .sources + .git + .output_async_bounded(self.inner, MAX_INTERNAL_GIT_OUTPUT_BYTES) + .await + } +} + +/// A sealed filter-only include owned for the complete capability lifetime. +/// Its fields and include argument are intentionally inaccessible outside this +/// module; callers can neither forge one nor append it to another command. +pub(crate) struct SealedFilterConfigOverride { + owner: Arc, + include_arg: String, + _config_dir: tempfile::TempDir, +} + +impl SealedFilterConfigOverride { + fn append_to( + &self, + owner: &Arc, + command: &mut GitCommand, + ) -> io::Result<()> { + self.ensure_owner(owner)?; + command.args(["-c", &self.include_arg]); + Ok(()) + } + + fn append_rendered_args( + &self, + owner: &Arc, + args: &mut Vec, + ) -> io::Result<()> { + self.ensure_owner(owner)?; + args.push("-c".to_string()); + args.push(self.include_arg.clone()); + Ok(()) + } + + fn ensure_owner(&self, owner: &Arc) -> io::Result<()> { + if !Arc::ptr_eq(&self.owner, owner) { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "sealed Git filter override belongs to another operation", + )); + } + Ok(()) + } +} + +/// A command whose runner, root, config invocation, overlay lifetime, and +/// fixed subcommand are inseparably bound to one operation capability. +pub(crate) struct GuardedGitCommand<'operation, 'git> { + operation: &'operation GuardedGitConfig<'git>, + inner: GitCommand, +} + +impl GuardedGitCommand<'_, '_> { + pub(crate) fn arg(&mut self, arg: impl AsRef) -> &mut Self { + self.inner.arg(arg); + self + } + + pub(crate) fn args(&mut self, args: I) -> &mut Self + where + I: IntoIterator, + S: AsRef, + { + self.inner.args(args); + self + } + + pub(crate) fn disable_optional_locks(&mut self) -> &mut Self { + self.inner.env("GIT_OPTIONAL_LOCKS", "0"); + self + } + + pub(crate) fn stdin(&mut self, config: impl Into) -> &mut Self { + self.inner.stdin(config); + self + } + + pub(crate) fn output(self) -> io::Result { + self.operation.sources.git.output(self.inner) + } +} + +impl<'git> GuardedGitConfig<'git> { + pub(crate) fn authorize( + git: &'git GitRunner, + canonical_root: &Path, + base_config_args: Vec, + ) -> io::Result { + Ok(Self { + sources: ValidatedConfigSources::authorize(git, canonical_root, base_config_args)?, + identity: Arc::new(CapabilityIdentity), + filters: Vec::new(), + merge: None, + merge_policy_installed: false, + status: None, + }) + } + + pub(crate) async fn authorize_status_async(git: &'git GitRunner) -> io::Result { + let root = git + .active_worktree_root() + .ok_or_else(|| io::Error::new(io::ErrorKind::NotFound, NoActiveStatusWorktree))?; + git.ensure_repository_root_route(root)?; + Ok(Self { + sources: ValidatedConfigSources::authorize_async(git, root, Vec::new()).await?, + identity: Arc::new(CapabilityIdentity), + filters: Vec::new(), + merge: None, + merge_policy_installed: false, + status: None, + }) + } + + pub(crate) fn canonical_root(&self) -> &Path { + &self.sources.canonical_root + } + + pub(crate) fn apply_command(&self) -> io::Result> { + self.guarded_command(BoundSubcommand::Apply) + } + + pub(crate) fn rev_parse_command(&self) -> io::Result> { + self.guarded_command(BoundSubcommand::RevParse) + } + + pub(crate) fn check_ignore_command(&self) -> io::Result> { + self.guarded_command(BoundSubcommand::CheckIgnore) + } + + pub(crate) fn list_builtin_commands(&self) -> io::Result { + let mut command = self.guarded_command(BoundSubcommand::ListBuiltinCommands)?; + command.disable_optional_locks(); + command.output() + } + + pub(crate) fn ls_files_command(&self) -> io::Result> { + self.guarded_command(BoundSubcommand::LsFiles) + } + + pub(crate) fn sparse_checkout_command(&self) -> io::Result> { + self.guarded_command(BoundSubcommand::SparseCheckout) + } + + pub(crate) fn update_index_literal_pathspecs_command( + &self, + ) -> io::Result> { + self.guarded_command(BoundSubcommand::UpdateIndexLiteralPathspecs) + } + + pub(crate) fn pending_filter_attribute_command<'operation>( + &'operation self, + neutralizer: &'operation SealedFilterConfigOverride, + ) -> io::Result> { + let mut command = self.command_with_attached_overlays()?; + neutralizer.append_to(&self.identity, &mut command)?; + BoundSubcommand::CheckAttr.append_to(&mut command); + Ok(GuardedGitCommand { + operation: self, + inner: command, + }) + } + + fn guarded_command( + &self, + subcommand: BoundSubcommand, + ) -> io::Result> { + let mut command = self.command_with_attached_overlays()?; + subcommand.append_to(&mut command); + Ok(GuardedGitCommand { + operation: self, + inner: command, + }) + } + + fn command_with_attached_overlays(&self) -> io::Result { + let mut command = self + .sources + .git + .command_for_cwd(&self.sources.canonical_root)?; + command.args(self.mutation_config_args()?); + Ok(command) + } + + fn mutation_config_args(&self) -> io::Result> { + if self.status.is_some() { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "status policy cannot be used by a mutation command", + )); + } + let mut args = self.sources.base_config_args.to_vec(); + args.extend(safe_scalar_override_args()); + let (apply, git_add) = self.ordered_filter_snapshots()?; + if let Some(neutralizer) = apply.and_then(FilterPolicySnapshot::neutralizer) { + neutralizer.append_rendered_args(&self.identity, &mut args)?; + } + if let Some(merge) = &self.merge { + merge.append_rendered_args(&self.identity, &mut args)?; + } + if let Some(neutralizer) = git_add.and_then(FilterPolicySnapshot::neutralizer) { + neutralizer.append_rendered_args(&self.identity, &mut args)?; + } + Ok(args) + } + + fn status_config_args( + &self, + fsmonitor: FsmonitorOverride, + neutralizer: Option<&SealedFilterConfigOverride>, + ) -> io::Result> { + self.ensure_status_exclusive_state()?; + let mut args = self.sources.base_config_args.to_vec(); + args.extend([ + "-c".to_string(), + format!("core.hooksPath={DISABLED_HOOKS_PATH}"), + "-c".to_string(), + fsmonitor.git_config_arg().to_string(), + ]); + if let Some(neutralizer) = neutralizer { + neutralizer.append_rendered_args(&self.identity, &mut args)?; + } + Ok(args) + } + + fn ensure_status_exclusive_state(&self) -> io::Result<()> { + if !self.filters.is_empty() || self.merge.is_some() || self.merge_policy_installed { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "status policy cannot coexist with mutation filter or merge policy", + )); + } + Ok(()) + } + + fn ordered_filter_snapshots( + &self, + ) -> io::Result<(Option<&FilterPolicySnapshot>, Option<&FilterPolicySnapshot>)> { + let ordered = match self.filters.as_slice() { + [] => (None, None), + [apply] if apply.role() == FilterPolicyRole::Apply => (Some(apply), None), + [git_add] if git_add.role() == FilterPolicyRole::GitAdd => (None, Some(git_add)), + [apply, git_add] + if apply.role() == FilterPolicyRole::Apply + && git_add.role() == FilterPolicyRole::GitAdd => + { + (Some(apply), Some(git_add)) + } + _ => { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "invalid ordered Git filter policy state", + )); + } + }; + if self.merge_policy_installed && ordered.0.is_none() { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "merge policy requires an apply filter snapshot", + )); + } + Ok(ordered) + } + + pub(crate) fn command_for_sentinel_filter_probe<'operation>( + &'operation self, + neutralizer: &'operation SealedFilterConfigOverride, + driver: &str, + required: bool, + ) -> io::Result> { + if !matches!(driver, "set" | "unset" | "unspecified") { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "Git filter sentinel probe requested for a non-sentinel driver", + )); + } + let mut command = self.command_with_attached_overlays()?; + neutralizer.append_to(&self.identity, &mut command)?; + command.args(["-c", &format!("filter.{driver}.required={required}")]); + BoundSubcommand::HashObject.append_to(&mut command); + Ok(GuardedGitCommand { + operation: self, + inner: command, + }) + } + + pub(crate) fn read_effective( + &self, + pattern: &str, + probe: &str, + ) -> io::Result> { + self.sources.read_effective(pattern, probe) + } + + pub(crate) fn read_bool(&self, key: &str) -> io::Result> { + self.sources.read_bool(key) + } + + /// Confirm selected Git agrees with the retained authority about the + /// repository root before any status worktree read. + pub(crate) async fn verify_status_root_async(&self, requested_cwd: &Path) -> io::Result<()> { + let requested_cwd = std::fs::canonicalize(requested_cwd)?; + let mut command = self.sources.git.async_command_for_cwd(&requested_cwd)?; + command + .env("GIT_OPTIONAL_LOCKS", "0") + .args(self.status_config_args(FsmonitorOverride::Disabled, None)?) + .args(["rev-parse", "--show-toplevel"]); + let output = self + .sources + .git + .output_async_bounded(command, MAX_INTERNAL_GIT_OUTPUT_BYTES) + .await?; + if !output.status.success() { + return Err(command_failure("status repository-root probe", &output)); + } + let reported = git_path_from_line_output(&output.stdout)?; + let reported = std::fs::canonicalize(reported).map_err(|_| { + io::Error::new( + io::ErrorKind::InvalidData, + "Git repository-root output did not resolve to an existing path", + ) + })?; + if reported != self.sources.canonical_root { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + StatusRootMismatch { + expected: self.sources.canonical_root.clone(), + reported, + }, + )); + } + Ok(()) + } + + /// Return the exact tracked-path byte strings for the Status policy. + async fn read_status_tracked_paths_async(&self) -> io::Result>> { + if self.status.is_some() { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "tracked paths must be read before status policy installation", + )); + } + let mut command = self.pending_status_command(FsmonitorOverride::Disabled, None)?; + command + .disable_optional_locks() + .args(["ls-files", "-z", "--cached"]); + let output = command.output().await?; + if !output.status.success() { + return Err(command_failure("tracked-path probe", &output)); + } + let paths = parse_nul_paths(&output.stdout)?; + if paths.len() > MAX_STATUS_TRACKED_PATHS { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + format!( + "tracked path count {} exceeds the status limit {MAX_STATUS_TRACKED_PATHS}", + paths.len() + ), + )); + } + Ok(paths) + } + + /// Install the mutually exclusive Status filter policy. The common + /// zero-driver path occupies the slot without scanning tracked paths. + pub(crate) async fn install_status_policy_async(&mut self) -> io::Result<()> { + self.ensure_status_exclusive_state()?; + if self.status.is_some() { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "status filter policy is already installed", + )); + } + #[cfg(test)] + FILTER_POLICY_READ_COUNT_FOR_STATUS.with(|count| count.set(count.get() + 1)); + let entries = self + .sources + .read_effective_async(crate::safe_git::EXECUTABLE_FILTER_CONFIG_PATTERN, "filter") + .await?; + let executable_drivers = executable_filter_drivers(&entries)?; + validate_executable_driver_count(&executable_drivers)?; + if executable_drivers.is_empty() { + self.status = Some(StatusFilterPolicySnapshot { + neutralizer: None, + fsmonitor: None, + }); + return Ok(()); + } + + let paths = self.read_status_tracked_paths_async().await?; + let neutralizer = self + .build_filter_override_async(&executable_drivers) + .await?; + let attributes = self + .read_status_filter_attributes_async(&paths, &executable_drivers, &neutralizer) + .await?; + let mut required_cache = BTreeMap::new(); + for (path, driver) in attributes { + if !executable_drivers.contains(&driver) { + continue; + } + let required = required_cache.get(&driver).copied(); + let mut policy = classify_selected_filter(&entries, &driver, required); + if policy == SelectedFilterPolicy::NeedsRequiredValue { + let required = self + .sources + .read_bool_async(&format!("filter.{driver}.required")) + .await + .map_err(|error| { + io::Error::new( + io::ErrorKind::Unsupported, + format!( + "refusing selected Git filter {driver:?} with malformed required value: {error}" + ), + ) + })? + .unwrap_or(false); + required_cache.insert(driver.clone(), required); + policy = classify_selected_filter(&entries, &driver, Some(required)); + } + if policy == SelectedFilterPolicy::Refused { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + SelectedStatusFilterRefusal { driver, path }, + )); + } + } + self.status = Some(StatusFilterPolicySnapshot { + neutralizer: Some(neutralizer), + fsmonitor: None, + }); + Ok(()) + } + + pub(crate) async fn detect_status_fsmonitor_async(&mut self) -> FsmonitorOverride { + if self.status.is_none() || self.ensure_status_exclusive_state().is_err() { + return FsmonitorOverride::Disabled; + } + if let Some(fsmonitor) = self.status.as_ref().and_then(|status| status.fsmonitor) { + return fsmonitor; + } + let fsmonitor = { + let mut runner = StatusFsmonitorProbeRunner { config: self }; + crate::detect_fsmonitor_override(&mut runner).await + }; + if let Some(status) = &mut self.status { + status.fsmonitor = Some(fsmonitor); + } + fsmonitor + } + + pub(crate) async fn status_output_async(&self) -> io::Result { + let status = self.status.as_ref().ok_or_else(|| { + io::Error::new( + io::ErrorKind::PermissionDenied, + "status output requires an installed status filter policy", + ) + })?; + let fsmonitor = status.fsmonitor.ok_or_else(|| { + io::Error::new( + io::ErrorKind::PermissionDenied, + "status output requires a retained fsmonitor decision", + ) + })?; + let mut command = self.pending_status_command(fsmonitor, status.neutralizer.as_ref())?; + command.disable_optional_locks().args([ + "status", + "--porcelain", + "--ignore-submodules=dirty", + ]); + command.output().await + } + + fn pending_status_command<'operation>( + &'operation self, + fsmonitor: FsmonitorOverride, + neutralizer: Option<&'operation SealedFilterConfigOverride>, + ) -> io::Result> { + let mut command = self + .sources + .git + .async_command_for_cwd(&self.sources.canonical_root)?; + command.args(self.status_config_args(fsmonitor, neutralizer)?); + Ok(GuardedAsyncGitCommand { + operation: self, + inner: command, + }) + } + + async fn read_status_filter_attributes_async( + &self, + paths: &[Vec], + executable_drivers: &ExecutableFilterDrivers, + neutralizer: &SealedFilterConfigOverride, + ) -> io::Result, String>> { + if paths.is_empty() { + return Ok(BTreeMap::new()); + } + let mut input = tempfile::tempfile()?; + write_nul_paths(&mut input, paths)?; + input.rewind()?; + let mut command = + self.pending_status_command(FsmonitorOverride::Disabled, Some(neutralizer))?; + command + .disable_optional_locks() + .args(["check-attr", "--stdin", "-z", "filter"]) + .stdin(Stdio::from(input)); + let output = command.output().await?; + if !output.status.success() { + return Err(command_failure("status filter attribute probe", &output)); + } + let attributes = parse_filter_attributes(&output.stdout, paths)?; + self.resolve_status_filter_sentinels_async(attributes, executable_drivers, neutralizer) + .await + } + + async fn resolve_status_filter_sentinels_async( + &self, + attributes: BTreeMap, FilterAttributeValue>, + executable_drivers: &ExecutableFilterDrivers, + neutralizer: &SealedFilterConfigOverride, + ) -> io::Result, String>> { + let mut resolved = BTreeMap::new(); + let mut probe_budget = SentinelFilterProbeBudget::default(); + for (path, attribute) in attributes { + match attribute { + FilterAttributeValue::Driver(driver) => { + resolved.insert(path, driver); + } + FilterAttributeValue::AmbiguousSentinel(driver) => { + if executable_drivers.contains(&driver) + && self + .status_sentinel_selects_driver_async( + &path, + &driver, + neutralizer, + &mut probe_budget, + ) + .await? + { + resolved.insert(path, driver); + } + } + } + } + Ok(resolved) + } + + async fn status_sentinel_selects_driver_async( + &self, + path: &[u8], + driver: &str, + neutralizer: &SealedFilterConfigOverride, + probe_budget: &mut SentinelFilterProbeBudget, + ) -> io::Result { + let required = self + .run_status_sentinel_probe_async( + path, + driver, + /*required*/ true, + neutralizer, + probe_budget, + ) + .await?; + if classify_sentinel_filter_probes( + required.status.success(), + /*optional_succeeded*/ None, + ) == SentinelFilterProbeResolution::SpecialAttributeState + { + return Ok(false); + } + let optional = self + .run_status_sentinel_probe_async( + path, + driver, + /*required*/ false, + neutralizer, + probe_budget, + ) + .await?; + if classify_sentinel_filter_probes( + required.status.success(), + Some(optional.status.success()), + ) == SentinelFilterProbeResolution::LiteralDriver + { + return Ok(true); + } + Err(command_failure("status filter sentinel probe", &optional)) + } + + async fn run_status_sentinel_probe_async( + &self, + path: &[u8], + driver: &str, + required: bool, + neutralizer: &SealedFilterConfigOverride, + probe_budget: &mut SentinelFilterProbeBudget, + ) -> io::Result { + if !matches!(driver, "set" | "unset" | "unspecified") { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "Git filter sentinel probe requested for a non-sentinel driver", + )); + } + probe_budget.ensure_probe_available().map_err(|_| { + io::Error::new( + io::ErrorKind::PermissionDenied, + StatusFilterProbeLimitExceeded { + max_probes: SentinelFilterProbeBudget::max_probes(), + }, + ) + })?; + let mut command = + self.pending_status_command(FsmonitorOverride::Disabled, Some(neutralizer))?; + command + .disable_optional_locks() + .args(["-c", &format!("filter.{driver}.required={required}")]) + .args(["hash-object", "--stdin"]) + .arg("--path") + .arg(git_path_argument(path)?) + .stdin(Stdio::null()); + let output = command.output().await?; + probe_budget.record_completed_probe(); + Ok(output) + } + + fn ensure_owned_config_path(&self, path: &Path, description: &str) -> io::Result<()> { + self.sources + .git + .ensure_config_source_is_not_worktree_controlled(path, description) + } + + pub(crate) fn build_filter_override( + &self, + executable_drivers: &ExecutableFilterDrivers, + ) -> io::Result { + let config_dir = tempfile::tempdir()?; + let config_path = config_dir.path().join("filter-neutralization.gitconfig"); + self.ensure_owned_config_path(&config_path, "owned Git filter neutralization")?; + std::fs::write(&config_path, [])?; + for driver in executable_drivers.iter() { + for (name, value) in FILTER_NEUTRALIZATION_PLAN { + self.write_filter_override_value(&config_path, driver, name, value)?; + } + } + let config_path = config_path.to_str().ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "non-UTF-8 filter guard path") + })?; + Ok(SealedFilterConfigOverride { + owner: Arc::clone(&self.identity), + include_arg: format!("include.path={config_path}"), + _config_dir: config_dir, + }) + } + + async fn build_filter_override_async( + &self, + executable_drivers: &ExecutableFilterDrivers, + ) -> io::Result { + if executable_drivers.len() > MAX_EXECUTABLE_FILTER_DRIVERS { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + "too many executable filter drivers for status neutralization", + )); + } + let config_dir = tempfile::tempdir()?; + let config_path = config_dir.path().join("filter-neutralization.gitconfig"); + self.ensure_owned_config_path(&config_path, "owned Git filter neutralization")?; + std::fs::write(&config_path, [])?; + for driver in executable_drivers.iter() { + for (name, value) in FILTER_NEUTRALIZATION_PLAN { + self.write_filter_override_value_async(&config_path, driver, name, value) + .await?; + } + } + let config_path = config_path.to_str().ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "non-UTF-8 filter guard path") + })?; + Ok(SealedFilterConfigOverride { + owner: Arc::clone(&self.identity), + include_arg: format!("include.path={config_path}"), + _config_dir: config_dir, + }) + } + + fn write_filter_override_value( + &self, + config_path: &Path, + driver: &str, + name: &str, + value: &str, + ) -> io::Result<()> { + debug_assert!(matches!(name, "clean" | "smudge" | "process" | "required")); + let mut command = self + .sources + .git + .command_for_cwd(&self.sources.canonical_root)?; + command + .args(&self.sources.base_config_args) + .args(["config", "--file"]) + .arg(config_path) + .args(["--add", &format!("filter.{driver}.{name}"), value]); + let output = self.sources.git.output(command)?; + if !output.status.success() { + return Err(io::Error::other(format!( + "failed to write Git filter neutralization for {driver:?} (status {}): {}", + output.status, + String::from_utf8_lossy(&output.stderr).trim() + ))); + } + Ok(()) + } + + async fn write_filter_override_value_async( + &self, + config_path: &Path, + driver: &str, + name: &str, + value: &str, + ) -> io::Result<()> { + debug_assert!(matches!(name, "clean" | "smudge" | "process" | "required")); + let mut command = self + .sources + .git + .async_command_for_cwd(&self.sources.canonical_root)?; + command + .args(&self.sources.base_config_args) + .args(["config", "--file"]) + .arg(config_path) + .args(["--add", &format!("filter.{driver}.{name}"), value]); + let output = self + .sources + .git + .output_async_bounded(command, MAX_INTERNAL_GIT_OUTPUT_BYTES) + .await?; + if !output.status.success() { + return Err(command_failure( + "status filter neutralization write", + &output, + )); + } + Ok(()) + } + + pub(crate) fn authorize_filter_paths(&mut self, paths: &[String]) -> io::Result<()> { + if self.status.is_some() { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "apply filter policy cannot coexist with status policy", + )); + } + if !self.filters.is_empty() { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "apply filter policy must be the first operation snapshot", + )); + } + let filter = + build_filter_policy_snapshot(self, paths, FilterExecution::AnyWorktreeOperation)?; + self.filters.push(filter); + Ok(()) + } + + pub(crate) fn ensure_apply_filter_path_subset(&self, paths: &[String]) -> io::Result<()> { + let [apply] = self.filters.as_slice() else { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "composed exact staging requires exactly one apply filter snapshot", + )); + }; + if apply.role() != FilterPolicyRole::Apply { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "composed exact staging requires an apply filter snapshot", + )); + } + if let Some(path) = paths + .iter() + .find(|path| !apply.contains_checked_path(path.as_str())) + { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + format!( + "content-filter staging path {path:?} was not authorized by the apply snapshot" + ), + )); + } + Ok(()) + } + + pub(crate) fn apply_filter_paths(&self) -> io::Result> { + let [apply] = self.filters.as_slice() else { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "merge policy requires exactly one apply filter snapshot", + )); + }; + if apply.role() != FilterPolicyRole::Apply { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "merge policy requires an apply filter snapshot", + )); + } + Ok(apply.checked_paths()) + } + + #[cfg(test)] + pub(crate) fn merge_include_arg(&self) -> Option<&str> { + self.merge + .as_ref() + .map(SealedMergeConfigOverride::include_arg) + } + + pub(crate) fn authorize_git_add_filter_paths(&mut self, paths: &[String]) -> io::Result<()> { + if self.status.is_some() { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "Git-add filter policy cannot coexist with status policy", + )); + } + match self.filters.as_slice() { + [] => {} + [apply] if apply.role() == FilterPolicyRole::Apply => {} + _ => { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "Git-add filter policy requires zero snapshots or exactly one apply snapshot", + )); + } + } + let filter = build_filter_policy_snapshot(self, paths, FilterExecution::GitAdd)?; + self.filters.push(filter); + Ok(()) + } + + pub(crate) fn render_command_for_log(&self, args: &[String]) -> io::Result { + let mut parts = vec!["git".to_string()]; + parts.extend(self.mutation_config_args()?); + parts.extend(args.iter().cloned()); + Ok(format!( + "(cd {} && {})", + quote_shell(&self.sources.canonical_root.display().to_string()), + parts + .into_iter() + .map(|part| quote_shell(&part)) + .collect::>() + .join(" ") + )) + } +} + +struct StatusFsmonitorProbeRunner<'a, 'git> { + config: &'a GuardedGitConfig<'git>, +} + +impl FsmonitorProbeRunner for StatusFsmonitorProbeRunner<'_, '_> { + async fn run_probe(&mut self, args: &[&str]) -> Option> { + let allowed = matches!( + args, + ["config", "--null", "--get", "core.fsmonitor"] + | [ + "config", + "--null", + "--type=bool", + "--fixed-value", + "--get", + "core.fsmonitor", + _, + ] + | ["version", "--build-options"] + ); + if !allowed { + return None; + } + let mut command = self + .config + .sources + .git + .async_command_for_cwd(&self.config.sources.canonical_root) + .ok()?; + command + .env("GIT_OPTIONAL_LOCKS", "0") + .args(&self.config.sources.base_config_args) + .args(args); + match self + .config + .sources + .git + .output_async_bounded(command, MAX_INTERNAL_GIT_OUTPUT_BYTES) + .await + { + Ok(output) if output.status.success() => Some(output.stdout), + Ok(_) | Err(_) => None, + } + } +} + +fn command_failure(description: &str, output: &std::process::Output) -> io::Error { + io::Error::new( + io::ErrorKind::Other, + StatusPolicyCommandFailure { + description: description.to_string(), + exit_code: output.status.code(), + }, + ) +} + +fn git_path_from_line_output(output: &[u8]) -> io::Result { + let output = output.strip_suffix(b"\n").unwrap_or(output); + #[cfg(windows)] + let output = output.strip_suffix(b"\r").unwrap_or(output); + if output.is_empty() { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + "empty Git repository-root output", + )); + } + #[cfg(unix)] + { + use std::ffi::OsString; + use std::os::unix::ffi::OsStringExt; + + Ok(PathBuf::from(OsString::from_vec(output.to_vec()))) + } + #[cfg(not(unix))] + { + String::from_utf8(output.to_vec()) + .map(PathBuf::from) + .map_err(|_| { + io::Error::new( + io::ErrorKind::InvalidData, + "non-UTF-8 Git repository-root output", + ) + }) + } +} + +fn safe_scalar_override_args() -> [String; 4] { + [ + "-c".to_string(), + format!("core.hooksPath={DISABLED_HOOKS_PATH}"), + "-c".to_string(), + FsmonitorOverride::Disabled.git_config_arg().to_string(), + ] +} + +fn quote_shell(value: &str) -> String { + let simple = value + .chars() + .all(|character| character.is_ascii_alphanumeric() || "-_.:/@%+".contains(character)); + if simple { + value.to_string() + } else { + format!("'{}'", value.replace('\'', "'\\''")) + } +} + +#[cfg(test)] +thread_local! { + static CONFIG_SOURCE_AUTHORIZATION_COUNT: std::cell::Cell = const { std::cell::Cell::new(0) }; + static FILTER_POLICY_READ_COUNT_FOR_STATUS: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +#[cfg(test)] +pub(crate) fn reset_config_source_authorization_count() { + CONFIG_SOURCE_AUTHORIZATION_COUNT.with(|count| count.set(0)); + FILTER_POLICY_READ_COUNT_FOR_STATUS.with(|count| count.set(0)); +} + +#[cfg(test)] +pub(crate) fn config_source_authorization_count() -> usize { + CONFIG_SOURCE_AUTHORIZATION_COUNT.with(std::cell::Cell::get) +} + +#[cfg(test)] +pub(crate) fn status_filter_policy_read_count() -> usize { + FILTER_POLICY_READ_COUNT_FOR_STATUS.with(std::cell::Cell::get) +} + +#[cfg(test)] +#[path = "guarded_config_tests.rs"] +mod tests; diff --git a/codex-rs/git-utils/src/guarded_config/merge_overlay.rs b/codex-rs/git-utils/src/guarded_config/merge_overlay.rs new file mode 100644 index 0000000000..ac9f6e5309 --- /dev/null +++ b/codex-rs/git-utils/src/guarded_config/merge_overlay.rs @@ -0,0 +1,322 @@ +use std::collections::BTreeMap; +use std::io; +use std::path::Path; +use std::process::Stdio; +use std::sync::Arc; + +use super::BoundSubcommand; +use super::CapabilityIdentity; +use super::GuardedGitConfig; +use crate::git_config::GitConfigEntry; + +const MERGE_CONFIG_PATTERN: &str = r"^(merge\.default|merge\..*\.driver)$"; + +/// A complete, fresh merge-config read bound to one authorized operation. +/// +/// The fields are private so callers cannot mint a partial driver inventory +/// and then ask the capability to treat its neutralizer as complete. +struct MergeConfigSnapshot { + owner: Arc, + entries: BTreeMap, +} + +impl MergeConfigSnapshot { + fn entries(&self) -> &BTreeMap { + &self.entries + } + + fn ensure_owner(&self, owner: &Arc) -> io::Result<()> { + if !Arc::ptr_eq(&self.owner, owner) { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "merge config snapshot belongs to another operation", + )); + } + Ok(()) + } + + fn nonempty_driver_keys(&self) -> io::Result> { + let mut keys = Vec::new(); + for entry in self.entries.values() { + if entry.key == "merge.default" { + continue; + } + let _driver = entry + .key + .strip_prefix("merge.") + .and_then(|key| key.strip_suffix(".driver")) + .ok_or_else(|| { + io::Error::new( + io::ErrorKind::InvalidData, + "malformed effective Git merge-driver key", + ) + })?; + if !entry.value.is_empty() { + keys.push(entry.key.as_str()); + } + } + Ok(keys) + } +} + +/// A sealed merge-driver-only include bound to one operation. +/// +/// Construction accepts only a complete `MergeConfigSnapshot`, and the type +/// exposes neither raw keys nor its include argument outside this module. +pub(super) struct SealedMergeConfigOverride { + owner: Arc, + include_arg: String, + _config_dir: tempfile::TempDir, +} + +impl SealedMergeConfigOverride { + pub(super) fn append_rendered_args( + &self, + owner: &Arc, + args: &mut Vec, + ) -> io::Result<()> { + self.ensure_owner(owner)?; + args.push("-c".to_string()); + args.push(self.include_arg.clone()); + Ok(()) + } + + fn ensure_owner(&self, owner: &Arc) -> io::Result<()> { + if !Arc::ptr_eq(&self.owner, owner) { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "sealed Git merge override belongs to another operation", + )); + } + Ok(()) + } + + #[cfg(test)] + pub(super) fn include_arg(&self) -> &str { + &self.include_arg + } +} + +impl<'git> GuardedGitConfig<'git> { + /// Install the complete fallback merge policy in one non-bypassable step. + /// This is the only crate-visible merge-overlay API: callers cannot mark + /// the policy complete without the fixed fresh config and attribute reads. + pub(crate) fn install_three_way_merge_policy(&mut self) -> io::Result<()> { + let paths = self.apply_filter_paths()?; + let snapshot = self.read_merge_config_snapshot()?; + let input = merge_attribute_input(&paths)?; + let output = self.query_merge_attributes(&snapshot, input)?; + if !output.status.success() { + return Err(io::Error::other(format!( + "git merge attribute probe failed with status {}: {}", + output.status, + String::from_utf8_lossy(&output.stderr).trim() + ))); + } + let attributes = crate::merge_driver::parse_merge_attributes(&output.stdout, &paths)?; + if let Some((driver, path)) = + crate::merge_driver::untrusted_driver_selection(snapshot.entries(), &attributes)? + { + return Err(io::Error::new( + io::ErrorKind::Unsupported, + format!( + "refusing to run an internal Git three-way apply with merge driver {driver:?} selected for {path:?}" + ), + )); + } + let neutralizer = self.build_merge_override(&snapshot)?; + self.attach_merge_override(neutralizer) + } + + /// Read merge-driver policy from the frozen, authorized base invocation. + /// Attached neutralizers are deliberately excluded so this is a fresh + /// view of the user's effective policy at fallback time. + fn read_merge_config_snapshot(&self) -> io::Result { + let _ = self.apply_filter_paths()?; + if self.merge_policy_installed { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "merge policy may be read only before its neutralizer is attached", + )); + } + #[cfg(test)] + MERGE_CONFIG_READ_COUNT.with(|count| count.set(count.get() + 1)); + Ok(MergeConfigSnapshot { + owner: Arc::clone(&self.identity), + entries: self.sources.read_effective(MERGE_CONFIG_PATTERN, "merge")?, + }) + } + + /// Run the one fixed fresh merge-attribute query while the merge + /// neutralizer is still absent. Existing apply-filter policy remains + /// attached, and callers cannot change the framing or attribute name. + fn query_merge_attributes( + &self, + snapshot: &MergeConfigSnapshot, + input: std::fs::File, + ) -> io::Result { + let _ = self.apply_filter_paths()?; + snapshot.ensure_owner(&self.identity)?; + if self.merge_policy_installed { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "merge attributes must be read before attaching the merge neutralizer", + )); + } + #[cfg(test)] + MERGE_ATTRIBUTE_READ_COUNT.with(|count| count.set(count.get() + 1)); + let mut command = self.command_with_attached_overlays()?; + BoundSubcommand::CheckAttr.append_to(&mut command); + command + .env("GIT_OPTIONAL_LOCKS", "0") + .args(["--stdin", "-z", "merge"]) + .stdin(Stdio::from(input)); + self.sources.git.output(command) + } + + fn build_merge_override( + &self, + snapshot: &MergeConfigSnapshot, + ) -> io::Result> { + snapshot.ensure_owner(&self.identity)?; + if self.merge_policy_installed { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "a merge neutralizer is already attached", + )); + } + let driver_keys = snapshot.nonempty_driver_keys()?; + if driver_keys.is_empty() { + return Ok(None); + } + + let config_dir = tempfile::tempdir()?; + let config_path = config_dir + .path() + .join("merge-driver-neutralization.gitconfig"); + self.ensure_owned_config_path(&config_path, "owned Git merge-driver neutralization")?; + std::fs::write(&config_path, [])?; + for key in driver_keys { + self.write_merge_override_value(&config_path, key)?; + } + let config_path = config_path.to_str().ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "non-UTF-8 merge guard path") + })?; + #[cfg(test)] + MERGE_OVERLAY_COUNT.with(|count| count.set(count.get() + 1)); + Ok(Some(SealedMergeConfigOverride { + owner: Arc::clone(&self.identity), + include_arg: format!("include.path={config_path}"), + _config_dir: config_dir, + })) + } + + fn attach_merge_override( + &mut self, + neutralizer: Option, + ) -> io::Result<()> { + if self.merge_policy_installed { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "a second merge neutralizer is not permitted", + )); + } + let [apply] = self.filters.as_slice() else { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "merge policy requires exactly one apply filter snapshot", + )); + }; + if apply.role() != crate::safe_git::FilterPolicyRole::Apply { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "merge policy requires an apply filter snapshot", + )); + } + if let Some(neutralizer) = &neutralizer { + neutralizer.ensure_owner(&self.identity)?; + } + self.merge = neutralizer; + self.merge_policy_installed = true; + Ok(()) + } + + fn write_merge_override_value(&self, config_path: &Path, key: &str) -> io::Result<()> { + let mut command = self + .sources + .git + .command_for_cwd(&self.sources.canonical_root)?; + command + .args(&self.sources.base_config_args) + .args(["config", "--file"]) + .arg(config_path) + .args(["--add", key, ""]); + let output = self.sources.git.output(command)?; + if !output.status.success() { + return Err(io::Error::other(format!( + "failed to write Git merge-driver neutralization for {key:?} (status {}): {}", + output.status, + String::from_utf8_lossy(&output.stderr).trim() + ))); + } + Ok(()) + } +} + +fn merge_attribute_input(paths: &[String]) -> io::Result { + use std::io::Seek; + use std::io::Write; + + if paths.is_empty() { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "refusing to inspect merge attributes for an empty patch path set", + )); + } + let mut input = tempfile::tempfile()?; + for path in paths { + if path.as_bytes().contains(&0) { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "merge attribute path contains NUL", + )); + } + input.write_all(path.as_bytes())?; + input.write_all(&[0])?; + } + input.rewind()?; + Ok(input) +} + +#[cfg(test)] +thread_local! { + static MERGE_CONFIG_READ_COUNT: std::cell::Cell = const { std::cell::Cell::new(0) }; + static MERGE_ATTRIBUTE_READ_COUNT: std::cell::Cell = const { std::cell::Cell::new(0) }; + static MERGE_OVERLAY_COUNT: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +#[cfg(test)] +pub(crate) fn reset_merge_policy_counts() { + MERGE_CONFIG_READ_COUNT.with(|count| count.set(0)); + MERGE_ATTRIBUTE_READ_COUNT.with(|count| count.set(0)); + MERGE_OVERLAY_COUNT.with(|count| count.set(0)); +} + +#[cfg(test)] +pub(crate) fn merge_config_read_count() -> usize { + MERGE_CONFIG_READ_COUNT.with(std::cell::Cell::get) +} + +#[cfg(test)] +pub(crate) fn merge_attribute_read_count() -> usize { + MERGE_ATTRIBUTE_READ_COUNT.with(std::cell::Cell::get) +} + +#[cfg(test)] +pub(crate) fn merge_overlay_count() -> usize { + MERGE_OVERLAY_COUNT.with(std::cell::Cell::get) +} + +#[cfg(test)] +#[path = "merge_overlay_tests.rs"] +mod tests; diff --git a/codex-rs/git-utils/src/guarded_config/merge_overlay_tests.rs b/codex-rs/git-utils/src/guarded_config/merge_overlay_tests.rs new file mode 100644 index 0000000000..069e4ff8c5 --- /dev/null +++ b/codex-rs/git-utils/src/guarded_config/merge_overlay_tests.rs @@ -0,0 +1,44 @@ +use super::*; +use crate::git_command::GitRunner; +use crate::safe_git::isolate_git_command_environment; +use pretty_assertions::assert_eq; + +fn run_git(cwd: &Path, args: &[&str]) { + let mut command = std::process::Command::new("git"); + isolate_git_command_environment(&mut command); + let output = command.current_dir(cwd).args(args).output().expect("Git"); + assert!( + output.status.success(), + "git {args:?}: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + +#[test] +fn sealed_merge_override_rejects_another_operation_identity() { + let repo = tempfile::tempdir().expect("repo"); + run_git(repo.path(), &["init", "-q"]); + run_git(repo.path(), &["config", "merge.unused.driver", "false"]); + let first_git = GitRunner::for_cwd_io(repo.path()).expect("first runner"); + let second_git = GitRunner::for_cwd_io(repo.path()).expect("second runner"); + let mut first = + GuardedGitConfig::authorize(&first_git, repo.path(), Vec::new()).expect("first config"); + let mut second = + GuardedGitConfig::authorize(&second_git, repo.path(), Vec::new()).expect("second config"); + first + .authorize_filter_paths(&["file.txt".to_string()]) + .expect("first apply snapshot"); + second + .authorize_filter_paths(&["file.txt".to_string()]) + .expect("second apply snapshot"); + + let snapshot = first.read_merge_config_snapshot().expect("merge snapshot"); + let neutralizer = first + .build_merge_override(&snapshot) + .expect("build merge override") + .expect("physical merge override"); + let error = second + .attach_merge_override(Some(neutralizer)) + .expect_err("cross-operation override must refuse"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); +} diff --git a/codex-rs/git-utils/src/guarded_config/reverse_probe.rs b/codex-rs/git-utils/src/guarded_config/reverse_probe.rs new file mode 100644 index 0000000000..fb9216e7a3 --- /dev/null +++ b/codex-rs/git-utils/src/guarded_config/reverse_probe.rs @@ -0,0 +1,91 @@ +use std::io; + +use super::GuardedGitConfig; + +#[derive(Clone, Copy)] +enum IntentToAddView { + Invisible, + Visible, +} + +impl<'git> GuardedGitConfig<'git> { + pub(crate) fn reverse_index_stage_output( + &self, + paths: &[String], + ) -> io::Result { + let mut command = self.command_with_attached_overlays()?; + command.args([ + "--literal-pathspecs", + "ls-files", + "-v", + "--stage", + "-z", + "--", + ]); + command.args(paths); + self.sources.git.output(command) + } + + pub(crate) fn reverse_cached_name_status_invisible_output( + &self, + paths: &[String], + ) -> io::Result { + self.reverse_cached_name_status_output(paths, IntentToAddView::Invisible) + } + + pub(crate) fn reverse_cached_name_status_visible_output( + &self, + paths: &[String], + ) -> io::Result { + self.reverse_cached_name_status_output(paths, IntentToAddView::Visible) + } + + fn reverse_cached_name_status_output( + &self, + paths: &[String], + view: IntentToAddView, + ) -> io::Result { + let intent_to_add = match view { + IntentToAddView::Invisible => "--ita-invisible-in-index", + IntentToAddView::Visible => "--ita-visible-in-index", + }; + let mut command = self.command_with_attached_overlays()?; + command.args([ + "--literal-pathspecs", + "diff", + "--cached", + "--name-status", + "-z", + "--no-renames", + intent_to_add, + "--no-ext-diff", + "--no-textconv", + "--ignore-submodules=none", + "--", + ]); + command.args(paths); + self.sources.git.output(command) + } + + pub(crate) fn reverse_worktree_changed_paths_output( + &self, + paths: &[String], + ) -> io::Result { + let mut command = self.command_with_attached_overlays()?; + #[cfg(unix)] + command.args(["-c", "core.filemode=true"]); + command.args([ + "--literal-pathspecs", + "diff-files", + "--name-only", + "-z", + "--no-renames", + "--no-ext-diff", + "--no-textconv", + "--ignore-submodules=none", + "--", + ]); + command.args(paths); + self.sources.git.output(command) + } +} diff --git a/codex-rs/git-utils/src/guarded_config_tests.rs b/codex-rs/git-utils/src/guarded_config_tests.rs new file mode 100644 index 0000000000..47806be71d --- /dev/null +++ b/codex-rs/git-utils/src/guarded_config_tests.rs @@ -0,0 +1,504 @@ +use super::*; +use crate::safe_git::isolate_git_command_environment; +use pretty_assertions::assert_eq; + +fn run_git(cwd: &Path, args: &[&str]) { + let mut command = std::process::Command::new("git"); + isolate_git_command_environment(&mut command); + let output = command.current_dir(cwd).args(args).output().expect("Git"); + assert!( + output.status.success(), + "git {args:?}: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + +fn run_isolated_config_test(test_name: &str) { + let environment = tempfile::tempdir().expect("isolated Git environment"); + let global_config = environment.path().join("global.gitconfig"); + let system_config = environment.path().join("system.gitconfig"); + std::fs::write(&global_config, "").expect("empty global config"); + std::fs::write(&system_config, "").expect("empty system config"); + + let mut command = std::process::Command::new(std::env::current_exe().expect("test binary")); + isolate_git_command_environment(&mut command); + let output = command + .arg(test_name) + .arg("--exact") + .arg("--nocapture") + .env("CODEX_GIT_UTILS_GUARDED_CONFIG_ENV_CHILD", "1") + .env("GIT_CONFIG_GLOBAL", &global_config) + .env("GIT_CONFIG_SYSTEM", &system_config) + .env("RUST_TEST_THREADS", "1") + .output() + .expect("run isolated test process"); + assert!( + output.status.success(), + "isolated test {test_name} failed:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); +} + +#[test] +fn attached_filter_snapshots_retain_multiple_owned_overrides() { + let repo = tempfile::tempdir().expect("repo"); + run_git(repo.path(), &["init", "-q"]); + std::fs::write(repo.path().join("second.txt"), "second\n").expect("write sink input"); + run_git( + repo.path(), + &["config", "filter.demo.clean", "git hash-object --stdin"], + ); + let git = GitRunner::for_cwd_io(repo.path()).expect("runner"); + let mut config = + GuardedGitConfig::authorize(&git, repo.path(), Vec::new()).expect("authorized config"); + + config + .authorize_filter_paths(&["first.txt".to_string()]) + .expect("first filter snapshot"); + config + .authorize_git_add_filter_paths(&["second.txt".to_string()]) + .expect("second filter snapshot"); + + assert_eq!(config.filters.len(), 2); + assert!( + config + .filters + .iter() + .all(|filter| filter.neutralizer().is_some()) + ); + + let apply_include = config.filters[0] + .neutralizer() + .expect("apply overlay") + .include_arg + .clone(); + let staging_include = config.filters[1] + .neutralizer() + .expect("staging overlay") + .include_arg + .clone(); + assert_ne!(apply_include, staging_include); + let apply_path = PathBuf::from( + apply_include + .strip_prefix("include.path=") + .expect("apply include path"), + ); + let staging_path = PathBuf::from( + staging_include + .strip_prefix("include.path=") + .expect("staging include path"), + ); + assert!(apply_path.is_file()); + assert!(staging_path.is_file()); + + let rendered = config + .render_command_for_log(&["update-index".to_string()]) + .expect("render ordered overlays"); + assert!( + rendered.find(&apply_include).expect("render apply overlay") + < rendered + .find(&staging_include) + .expect("render staging overlay") + ); + + let mut sink = config + .update_index_literal_pathspecs_command() + .expect("bound sink"); + sink.disable_optional_locks() + .args(["--add", "--remove", "--", "second.txt"]); + let output = sink.output().expect("run sink"); + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + assert!(apply_path.is_file(), "apply overlay must outlive the sink"); + assert!( + staging_path.is_file(), + "staging overlay must outlive the sink" + ); + + drop(config); + assert!(!apply_path.exists()); + assert!(!staging_path.exists()); +} + +#[test] +fn composed_staging_subset_proof_requires_exactly_one_apply_snapshot() { + let repo = tempfile::tempdir().expect("repo"); + run_git(repo.path(), &["init", "-q"]); + let git = GitRunner::for_cwd_io(repo.path()).expect("runner"); + let mut config = + GuardedGitConfig::authorize(&git, repo.path(), Vec::new()).expect("authorized config"); + + let error = config + .ensure_apply_filter_path_subset(&[]) + .expect_err("zero snapshots must refuse"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + + config + .authorize_filter_paths(&["allowed.txt".to_string()]) + .expect("apply snapshot"); + config + .ensure_apply_filter_path_subset(&["allowed.txt".to_string()]) + .expect("authorized subset"); + let error = config + .ensure_apply_filter_path_subset(&["outside.txt".to_string()]) + .expect_err("off-universe path must refuse"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + + config + .authorize_git_add_filter_paths(&[]) + .expect("Git-add snapshot"); + let error = config + .ensure_apply_filter_path_subset(&[]) + .expect_err("two snapshots must refuse"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); +} + +#[test] +fn zero_driver_merge_policy_still_occupies_the_single_install_slot() { + if std::env::var_os("CODEX_GIT_UTILS_GUARDED_CONFIG_ENV_CHILD").is_none() { + run_isolated_config_test( + "guarded_config::tests::zero_driver_merge_policy_still_occupies_the_single_install_slot", + ); + return; + } + let repo = tempfile::tempdir().expect("repo"); + run_git(repo.path(), &["init", "-q"]); + let git = GitRunner::for_cwd_io(repo.path()).expect("runner"); + let mut config = + GuardedGitConfig::authorize(&git, repo.path(), Vec::new()).expect("authorized config"); + + let error = config + .install_three_way_merge_policy() + .expect_err("merge policy without apply snapshot must refuse"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + + config + .authorize_filter_paths(&["file.txt".to_string()]) + .expect("apply snapshot"); + config + .install_three_way_merge_policy() + .expect("zero-driver merge policy"); + assert!(config.merge_policy_installed); + assert!(config.merge.is_none()); + + let error = config + .install_three_way_merge_policy() + .expect_err("second merge policy must refuse"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); +} + +#[test] +fn command_order_is_apply_filter_then_merge_then_git_add_filter() { + if std::env::var_os("CODEX_GIT_UTILS_GUARDED_CONFIG_ENV_CHILD").is_none() { + run_isolated_config_test( + "guarded_config::tests::command_order_is_apply_filter_then_merge_then_git_add_filter", + ); + return; + } + let repo = tempfile::tempdir().expect("repo"); + run_git(repo.path(), &["init", "-q"]); + std::fs::write(repo.path().join("file.txt"), "file\n").expect("write file"); + run_git( + repo.path(), + &["config", "filter.demo.clean", "git hash-object --stdin"], + ); + run_git(repo.path(), &["config", "merge.unused.driver", "false"]); + let git = GitRunner::for_cwd_io(repo.path()).expect("runner"); + let mut config = + GuardedGitConfig::authorize(&git, repo.path(), Vec::new()).expect("authorized config"); + + config + .authorize_filter_paths(&["file.txt".to_string()]) + .expect("apply filter policy"); + config + .install_three_way_merge_policy() + .expect("merge policy"); + config + .authorize_git_add_filter_paths(&["file.txt".to_string()]) + .expect("Git-add filter policy"); + + let apply_include = config.filters[0] + .neutralizer() + .expect("apply filter overlay") + .include_arg + .clone(); + let merge_include = config + .merge_include_arg() + .expect("merge overlay") + .to_string(); + let git_add_include = config.filters[1] + .neutralizer() + .expect("Git-add filter overlay") + .include_arg + .clone(); + let overlay_paths = [&apply_include, &merge_include, &git_add_include] + .map(|include| PathBuf::from(include.strip_prefix("include.path=").expect("include path"))); + + let rendered = config + .render_command_for_log(&["ls-files".to_string()]) + .expect("render command"); + let apply_offset = rendered.find(&apply_include).expect("apply include"); + let merge_offset = rendered.find(&merge_include).expect("merge include"); + let git_add_offset = rendered.find(&git_add_include).expect("Git-add include"); + assert!(apply_offset < merge_offset && merge_offset < git_add_offset); + assert_eq!(rendered.matches(&merge_include).count(), 1); + + let output = config + .ls_files_command() + .expect("bound command") + .output() + .expect("execute bound command"); + assert!(output.status.success()); + assert!(overlay_paths.iter().all(|path| path.is_file())); + + drop(config); + assert!(overlay_paths.iter().all(|path| !path.exists())); +} + +#[test] +fn global_builtin_query_is_a_fixed_no_argument_operation() { + let repo = tempfile::tempdir().expect("repo"); + run_git(repo.path(), &["init", "-q"]); + let git = GitRunner::for_cwd_io(repo.path()).expect("runner"); + let config = + GuardedGitConfig::authorize(&git, repo.path(), Vec::new()).expect("authorized config"); + + // The capability exposes only this no-argument execution method; callers + // never receive the global-only command and therefore cannot append a + // second global option or an external command name. + let output = config + .list_builtin_commands() + .expect("fixed builtin-command query"); + assert!(output.status.success()); + let output = std::str::from_utf8(&output.stdout).expect("UTF-8 builtin names"); + assert!(output.lines().any(|name| name == "add")); +} + +#[test] +fn authorization_rejects_untyped_global_config_arguments() { + let repo = tempfile::tempdir().expect("repo"); + run_git(repo.path(), &["init", "-q"]); + let git = GitRunner::for_cwd_io(repo.path()).expect("runner"); + + for args in [ + vec!["--config-env=include.path=UNSNAPSHOTTED".to_string()], + vec!["--git-dir=elsewhere".to_string()], + vec!["-c".to_string(), "missing-value".to_string()], + vec!["-c".to_string()], + ] { + let error = match GuardedGitConfig::authorize(&git, repo.path(), args) { + Ok(_) => panic!("accepted untyped base config arguments"), + Err(error) => error, + }; + assert_eq!(error.kind(), io::ErrorKind::InvalidInput); + } +} + +#[test] +fn authorization_rejects_another_runner_repository_and_nested_or_prefix_roots() { + let fixture = tempfile::tempdir().expect("fixture"); + let outer = fixture.path().join("repo"); + let nested = outer.join("nested"); + let prefix = fixture.path().join("repo-evil"); + std::fs::create_dir_all(&nested).expect("nested"); + std::fs::create_dir_all(&prefix).expect("prefix"); + run_git(&outer, &["init", "-q"]); + run_git(&nested, &["init", "-q"]); + run_git(&prefix, &["init", "-q"]); + let git = GitRunner::for_cwd_io(&outer).expect("outer runner"); + + for wrong_root in [&nested, &prefix] { + let error = match GuardedGitConfig::authorize(&git, wrong_root, Vec::new()) { + Ok(_) => panic!("accepted another repository root"), + Err(error) => error, + }; + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + } +} + +#[test] +fn sealed_filter_override_cannot_cross_operation_capabilities() { + let fixture = tempfile::tempdir().expect("fixture"); + let first_root = fixture.path().join("first"); + let second_root = fixture.path().join("second"); + std::fs::create_dir_all(&first_root).expect("first"); + std::fs::create_dir_all(&second_root).expect("second"); + run_git(&first_root, &["init", "-q"]); + run_git(&second_root, &["init", "-q"]); + run_git( + &first_root, + &["config", "filter.demo.clean", "git hash-object --stdin"], + ); + let first_git = GitRunner::for_cwd_io(&first_root).expect("first runner"); + let second_git = GitRunner::for_cwd_io(&second_root).expect("second runner"); + let mut first = + GuardedGitConfig::authorize(&first_git, &first_root, Vec::new()).expect("first capability"); + let second = GuardedGitConfig::authorize(&second_git, &second_root, Vec::new()) + .expect("second capability"); + first + .authorize_filter_paths(&["unselected.txt".to_string()]) + .expect("first filter policy"); + let sealed = first.filters[0].neutralizer().expect("first override"); + + let error = + match second.command_for_sentinel_filter_probe(sealed, "set", /*required*/ true) { + Ok(_) => panic!("accepted another operation's sealed override"), + Err(error) => error, + }; + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); +} + +#[tokio::test] +async fn zero_driver_status_occupies_its_slot_and_excludes_every_mutation_policy() { + if std::env::var_os("CODEX_GIT_UTILS_GUARDED_CONFIG_ENV_CHILD").is_none() { + run_isolated_config_test( + "guarded_config::tests::zero_driver_status_occupies_its_slot_and_excludes_every_mutation_policy", + ); + return; + } + let repo = tempfile::tempdir().expect("repo"); + run_git(repo.path(), &["init", "-q"]); + reset_config_source_authorization_count(); + crate::git_command::reset_git_runner_construction_count(); + let git = GitRunner::for_cwd_io(repo.path()).expect("runner"); + let mut config = GuardedGitConfig::authorize_status_async(&git) + .await + .expect("authorized status capability"); + assert_eq!(config_source_authorization_count(), 1); + assert_eq!(crate::git_command::git_runner_construction_count(), 1); + config + .verify_status_root_async(repo.path()) + .await + .expect("matching selected-Git root"); + let error = config + .status_output_async() + .await + .expect_err("status sink requires an installed policy"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + config + .install_status_policy_async() + .await + .expect("zero-driver status policy"); + assert_eq!(status_filter_policy_read_count(), 1); + let status = config.status.as_ref().expect("occupied status slot"); + assert!(status.neutralizer.is_none()); + assert_eq!(status.fsmonitor, None); + + let error = config + .install_status_policy_async() + .await + .expect_err("second status installation must fail"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + assert_eq!(status_filter_policy_read_count(), 1); + let error = config + .authorize_filter_paths(&[]) + .expect_err("status plus Apply must fail"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + let error = config + .authorize_git_add_filter_paths(&[]) + .expect_err("status plus GitAdd must fail"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + let error = config + .install_three_way_merge_policy() + .expect_err("status plus merge must fail"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + let error = config + .status_output_async() + .await + .expect_err("status sink requires a retained fsmonitor decision"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + + let first = config.detect_status_fsmonitor_async().await; + let second = config.detect_status_fsmonitor_async().await; + assert_eq!(first, second); + let output = config + .status_output_async() + .await + .expect("closed status sink"); + assert!(output.status.success()); + + let mut mutation = + GuardedGitConfig::authorize(&git, repo.path(), Vec::new()).expect("mutation capability"); + mutation + .authorize_filter_paths(&[]) + .expect("Apply filter policy"); + let reads_before = status_filter_policy_read_count(); + let error = mutation + .install_status_policy_async() + .await + .expect_err("Apply plus status must fail before a status read"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + assert_eq!(status_filter_policy_read_count(), reads_before); +} + +#[tokio::test] +async fn status_uses_the_shared_four_entry_overlay_and_retains_it_through_the_sink() { + if std::env::var_os("CODEX_GIT_UTILS_GUARDED_CONFIG_ENV_CHILD").is_none() { + run_isolated_config_test( + "guarded_config::tests::status_uses_the_shared_four_entry_overlay_and_retains_it_through_the_sink", + ); + return; + } + let repo = tempfile::tempdir().expect("repo"); + run_git(repo.path(), &["init", "-q"]); + std::fs::write(repo.path().join("file.txt"), "contents\n").expect("tracked file"); + std::fs::write(repo.path().join(".gitattributes"), "file.txt filter=demo\n") + .expect("attributes"); + run_git(repo.path(), &["add", "."]); + run_git(repo.path(), &["config", "filter.demo.smudge", "cat"]); + + let git = GitRunner::for_cwd_io(repo.path()).expect("runner"); + let mut config = GuardedGitConfig::authorize_status_async(&git) + .await + .expect("authorized status capability"); + config + .verify_status_root_async(repo.path()) + .await + .expect("matching root"); + config + .install_status_policy_async() + .await + .expect("optional smudge policy"); + let neutralizer = config + .status + .as_ref() + .and_then(|status| status.neutralizer.as_ref()) + .expect("status neutralizer"); + let overlay = PathBuf::from( + neutralizer + .include_arg + .strip_prefix("include.path=") + .expect("sealed include path"), + ); + assert!(overlay.is_file()); + let mut actual = Vec::new(); + for (name, expected) in FILTER_NEUTRALIZATION_PLAN { + let output = std::process::Command::new("git") + .args(["config", "--file"]) + .arg(&overlay) + .args(["--get", &format!("filter.demo.{name}")]) + .output() + .expect("read neutralization entry"); + assert!(output.status.success(), "read {name}"); + actual.push(( + name, + String::from_utf8_lossy(&output.stdout).trim().to_string(), + )); + assert_eq!(actual.last().map(|entry| entry.1.as_str()), Some(expected)); + } + assert_eq!(actual.len(), FILTER_NEUTRALIZATION_PLAN.len()); + + config.detect_status_fsmonitor_async().await; + let output = config.status_output_async().await.expect("guarded status"); + assert!(output.status.success()); + assert!(overlay.is_file(), "overlay must outlive the final child"); + drop(config); + assert!( + !overlay.exists(), + "overlay must be removed with the capability" + ); +} diff --git a/codex-rs/git-utils/src/info.rs b/codex-rs/git-utils/src/info.rs index ed01289d85..0e920ddb1e 100644 --- a/codex-rs/git-utils/src/info.rs +++ b/codex-rs/git-utils/src/info.rs @@ -15,17 +15,18 @@ use futures::future::join_all; use schemars::JsonSchema; use serde::Deserialize; use serde::Serialize; +use tokio::process::Command; use tokio::time::timeout; use ts_rs::TS; use crate::GitReadError; use crate::GitSha; use crate::git_command::GitRunner; -use crate::git_command::MAX_INTERNAL_GIT_OUTPUT_BYTES; -use crate::repository_authority::is_authority_refusal; use crate::safe_git::DISABLED_HOOKS_PATH; use crate::safe_git::GIT_COMMAND_TIMEOUT; -use crate::status_guard::prepare_status_filter_guard_within_deadline; +use crate::status_guard::detect_status_fsmonitor; +use crate::status_guard::prepare_status_config; +use crate::status_guard::read_status; /// Return `true` if the project folder specified by the `Config` is inside a /// Git repository. @@ -345,29 +346,15 @@ async fn try_get_has_changes_within_deadline( cwd: &Path, phase: &AtomicU8, ) -> Result { - let git = GitRunner::for_cwd(cwd)?; - let (git_root, filter_guard) = - Box::pin(prepare_status_filter_guard_within_deadline(&git, cwd)).await?; + let requested_cwd = std::fs::canonicalize(cwd).map_err(|_| GitReadError::NotRepository { + path: cwd.to_path_buf(), + })?; + let git = GitRunner::for_cwd(&requested_cwd)?; + let mut config = Box::pin(prepare_status_config(&git, &requested_cwd)).await?; phase.store(StatusReadPhase::Fsmonitor as u8, Ordering::Relaxed); - let fsmonitor = detect_local_fsmonitor_override(&git, &git_root).await; + detect_status_fsmonitor(&mut config).await; phase.store(StatusReadPhase::Status as u8, Ordering::Relaxed); - let output = try_run_git_command_with_timeout_from( - &git, - &["status", "--porcelain", "--ignore-submodules=dirty"], - &git_root, - fsmonitor, - filter_guard.git_config_args(), - "status", - ) - .await?; - if !output.status.success() { - return Err(GitReadError::CommandFailed { - operation: "status".to_string(), - exit_code: output.status.code(), - }); - } - - Ok(!output.stdout.is_empty()) + read_status(&config).await } fn parse_git_remote_urls(stdout: &str) -> Option> { @@ -475,12 +462,17 @@ pub async fn git_diff_to_remote(cwd: &Path) -> Option { async fn run_git_command_with_timeout(args: &[&str], cwd: &Path) -> Option { // These callers only inspect repository metadata. Worktree workflows probe // once and pass their override directly to the lower-level runner. - let git = GitRunner::for_cwd(cwd).ok()?; - run_git_command_with_timeout_from(&git, args, cwd, crate::FsmonitorOverride::Disabled).await + run_git_command_with_timeout_from( + Path::new("git"), + args, + cwd, + crate::FsmonitorOverride::Disabled, + ) + .await } struct LocalFsmonitorProbeRunner<'a> { - git: &'a GitRunner, + git: &'a Path, cwd: &'a Path, } @@ -488,94 +480,44 @@ impl crate::FsmonitorProbeRunner for LocalFsmonitorProbeRunner<'_> { async fn run_probe(&mut self, args: &[&str]) -> Option> { // Both probes are fast, bounded metadata queries that do not inspect the // worktree or index, so do not reduce the requested command's timeout. - let mut command = self.git.async_command_for_cwd(self.cwd).ok()?; - command.args(args); - match timeout( - GIT_COMMAND_TIMEOUT, - self.git - .output_async_bounded(command, MAX_INTERNAL_GIT_OUTPUT_BYTES), - ) - .await - { + let mut command = Command::new(self.git); + command + .envs(crate::local_only_git_env()) + .args(args) + .current_dir(self.cwd) + .kill_on_drop(true); + match timeout(GIT_COMMAND_TIMEOUT, command.output()).await { Ok(Ok(output)) if output.status.success() => Some(output.stdout), _ => None, } } } -async fn detect_local_fsmonitor_override(git: &GitRunner, cwd: &Path) -> crate::FsmonitorOverride { +async fn detect_local_fsmonitor_override(git: &Path, cwd: &Path) -> crate::FsmonitorOverride { let mut runner = LocalFsmonitorProbeRunner { git, cwd }; crate::detect_fsmonitor_override(&mut runner).await } async fn run_git_command_with_timeout_from( - git: &GitRunner, + git: &Path, args: &[&str], cwd: &Path, fsmonitor: crate::FsmonitorOverride, ) -> Option { - try_run_git_command_with_timeout_from(git, args, cwd, fsmonitor, &[], "gitMetadata") - .await - .ok() -} - -async fn try_run_git_command_with_timeout_from( - git: &GitRunner, - args: &[&str], - cwd: &Path, - fsmonitor: crate::FsmonitorOverride, - extra_git_config_args: &[String], - operation: &str, -) -> Result { - let disabled_hooks = format!("core.hooksPath={DISABLED_HOOKS_PATH}"); - let mut command = git.async_command_for_cwd(cwd).map_err(|error| { - if is_authority_refusal(&error) { - return GitReadError::AuthorityRefused { - operation: operation.to_string(), - }; - } - match error.kind() { - std::io::ErrorKind::InvalidData => GitReadError::InvalidOutput { - operation: operation.to_string(), - }, - _ => GitReadError::CommandFailed { - operation: operation.to_string(), - exit_code: None, - }, - } - })?; + let mut command = Command::new(git); command + .envs(crate::local_only_git_env()) .env("GIT_OPTIONAL_LOCKS", "0") // Keep internal Git commands independent of repository-selected hooks - // while preserving built-in fsmonitor acceleration. - .args(["-c", &disabled_hooks]) + // and fsmonitor helpers while preserving built-in fsmonitor acceleration. + .args(["-c", &format!("core.hooksPath={DISABLED_HOOKS_PATH}")]) .args(["-c", fsmonitor.git_config_arg()]) - // Keep the owning guard alive in the caller and append its private - // include last so it wins inherited and repository filter config. - .args(extra_git_config_args) - .args(args); - match timeout( - GIT_COMMAND_TIMEOUT, - git.output_async_bounded(command, MAX_INTERNAL_GIT_OUTPUT_BYTES), - ) - .await - { - Err(_) => Err(GitReadError::CommandTimedOut { - operation: operation.to_string(), - }), - Ok(Err(error)) if is_authority_refusal(&error) => Err(GitReadError::AuthorityRefused { - operation: operation.to_string(), - }), - Ok(Err(error)) if error.kind() == std::io::ErrorKind::InvalidData => { - Err(GitReadError::InvalidOutput { - operation: operation.to_string(), - }) - } - Ok(Err(_)) => Err(GitReadError::CommandFailed { - operation: operation.to_string(), - exit_code: None, - }), - Ok(Ok(output)) => Ok(output), + .args(args) + .current_dir(cwd) + .kill_on_drop(true); + match timeout(GIT_COMMAND_TIMEOUT, command.output()).await { + Ok(Ok(output)) => Some(output), + _ => None, } } @@ -856,10 +798,10 @@ async fn find_closest_sha(cwd: &Path, branches: &[String], remotes: &[String]) - } async fn diff_against_sha(cwd: &Path, sha: &GitSha) -> Option { - let git = GitRunner::for_cwd(cwd).ok()?; - let fsmonitor = detect_local_fsmonitor_override(&git, cwd).await; + let git = Path::new("git"); + let fsmonitor = detect_local_fsmonitor_override(git, cwd).await; let output = run_git_command_with_timeout_from( - &git, + git, &["diff", "--no-textconv", "--no-ext-diff", &sha.0], cwd, fsmonitor, @@ -874,7 +816,7 @@ async fn diff_against_sha(cwd: &Path, sha: &GitSha) -> Option { let mut diff = String::from_utf8(output.stdout).ok()?; if let Some(untracked_output) = run_git_command_with_timeout_from( - &git, + git, &["ls-files", "--others", "--exclude-standard"], cwd, fsmonitor, @@ -892,7 +834,6 @@ async fn diff_against_sha(cwd: &Path, sha: &GitSha) -> Option { if !untracked.is_empty() { // Use platform-appropriate null device and guard paths with `--`. let null_device: &str = if cfg!(windows) { "NUL" } else { "/dev/null" }; - let git = &git; let futures_iter = untracked.into_iter().map(|file| async move { let file_owned = file; let args_vec: Vec<&str> = vec![ @@ -1052,25 +993,6 @@ mod tests { .to_string() } - #[cfg(unix)] - fn resolved_git_executable() -> PathBuf { - let output = std::process::Command::new("/bin/sh") - .args(["-c", "command -v git"]) - .output() - .expect("resolve real Git executable"); - assert!(output.status.success(), "resolve real Git executable"); - PathBuf::from( - String::from_utf8(output.stdout) - .expect("real Git path should be UTF-8") - .trim(), - ) - } - - #[cfg(unix)] - fn shell_quote_path(path: &Path) -> String { - format!("'{}'", path.to_string_lossy().replace('\'', "'\\''")) - } - #[cfg(unix)] fn commit_all(cwd: &Path, message: &str) { run_git( @@ -1227,11 +1149,6 @@ mod tests { permissions.set_mode(0o755); std::fs::set_permissions(&git, permissions).expect("mark fake Git executable"); - // The config response mirrors: - // git -c core.fsmonitor=/tmp/fsmonitor-helper \ - // config --null --get core.fsmonitor - let git = GitRunner::from_executable_for_test(temp_dir.path(), git) - .expect("authority-bound fake Git"); let fsmonitor = detect_local_fsmonitor_override(&git, temp_dir.path()).await; let output = run_git_command_with_timeout_from( &git, @@ -1278,22 +1195,17 @@ mod tests { let git = temp_dir.path().join("git"); let global_config = temp_dir.path().join("git.global"); let log = temp_dir.path().join("git.log"); - let real_git = shell_quote_path(&resolved_git_executable()); std::fs::write( &git, - format!( - "#!/bin/sh\n\ - cwd=.\n\ - if [ \"$1\" = -C ]; then cwd=$2; shift 2; fi\n\ + "#!/bin/sh\n\ printf '%s\\n' \"$*\" >>\"$0.log\"\n\ case \"$1\" in\n\ config)\n\ - GIT_CONFIG_NOSYSTEM=1 GIT_CONFIG_GLOBAL=\"$0.global\" exec {real_git} -C \"$cwd\" \"$@\"\n\ + GIT_CONFIG_NOSYSTEM=1 GIT_CONFIG_GLOBAL=\"$0.global\" exec git \"$@\"\n\ ;;\n\ version) printf 'feature: fsmonitor--daemon\\n' ;;\n\ *) printf 'worktree output\\n' ;;\n\ - esac\n" - ), + esac\n", ) .expect("write layered-config Git"); let mut permissions = std::fs::metadata(&git) @@ -1328,8 +1240,6 @@ mod tests { "write local built-in fsmonitor config" ); - let git = - GitRunner::from_executable_for_test(&repo, git).expect("authority-bound layered Git"); let fsmonitor = detect_local_fsmonitor_override(&git, repo.as_path()).await; let output = run_git_command_with_timeout_from( &git, diff --git a/codex-rs/git-utils/src/lib.rs b/codex-rs/git-utils/src/lib.rs index 63d01dfe8d..8e73be1636 100644 --- a/codex-rs/git-utils/src/lib.rs +++ b/codex-rs/git-utils/src/lib.rs @@ -9,8 +9,10 @@ mod exact_staging; mod fsmonitor; mod git_command; mod git_config; +mod git_config_environment; mod git_config_sources; mod git_executable; +mod guarded_config; mod info; mod local_only; mod merge_driver; diff --git a/codex-rs/git-utils/src/merge_driver.rs b/codex-rs/git-utils/src/merge_driver.rs index 5120f321aa..9f6a1d61f7 100644 --- a/codex-rs/git-utils/src/merge_driver.rs +++ b/codex-rs/git-utils/src/merge_driver.rs @@ -1,104 +1,11 @@ +use crate::git_config::GitConfigEntry; +#[cfg(test)] +use crate::safe_git::isolate_git_command_environment; use std::collections::BTreeMap; use std::collections::BTreeSet; use std::io; -use std::io::Seek; -use std::io::Write; -use std::path::Path; -use std::process::Stdio; -use crate::git_command::GitRunner; -use crate::git_config::GitConfigEntry; -use crate::safe_git::GitConfigOverrideFile; -#[cfg(test)] -use crate::safe_git::isolate_git_command_environment; -use crate::safe_git::read_effective_config_with_fallback; - -const MERGE_CONFIG_PATTERN: &str = r"^(merge\.default|merge\..*\.driver)$"; - -pub(crate) fn ensure_no_selected_merge_drivers( - git: &GitRunner, - cwd: &Path, - paths: &[String], - git_config_args: &[String], -) -> io::Result> { - let entries = read_merge_config(git, cwd, git_config_args)?; - let attributes = read_merge_attributes(git, cwd, paths, git_config_args)?; - if let Some((driver, path)) = untrusted_driver_selection(&entries, &attributes)? { - return Err(io::Error::new( - io::ErrorKind::Unsupported, - format!( - "refusing to run an internal Git three-way apply with merge driver {driver:?} selected for {path:?}" - ), - )); - } - - let driver_keys = entries - .values() - .filter(|entry| entry.key != "merge.default" && !entry.value.is_empty()) - .map(|entry| entry.key.clone()) - .collect::>(); - if driver_keys.is_empty() { - return Ok(None); - } - - let guard = GitConfigOverrideFile::new("merge-driver-neutralization.gitconfig")?; - for key in driver_keys { - guard.add_value( - git, - cwd, - &key, - "", - &format!("Git merge-driver neutralization for {key:?}"), - )?; - } - Ok(Some(guard)) -} - -fn read_merge_config( - git: &GitRunner, - cwd: &Path, - git_config_args: &[String], -) -> io::Result> { - read_effective_config_with_fallback(git, cwd, git_config_args, MERGE_CONFIG_PATTERN, "merge") -} - -fn read_merge_attributes( - git: &GitRunner, - cwd: &Path, - paths: &[String], - git_config_args: &[String], -) -> io::Result> { - if paths.is_empty() { - return Err(io::Error::new( - io::ErrorKind::InvalidInput, - "refusing to inspect merge attributes for an empty patch path set", - )); - } - let mut input = tempfile::tempfile()?; - for path in paths { - input.write_all(path.as_bytes())?; - input.write_all(&[0])?; - } - input.rewind()?; - - let mut command = git.command_for_cwd(cwd)?; - command - .env("GIT_OPTIONAL_LOCKS", "0") - .args(git_config_args) - .args(["check-attr", "--stdin", "-z", "merge"]) - .stdin(Stdio::from(input)); - let output = git.output(command)?; - if !output.status.success() { - return Err(io::Error::other(format!( - "git merge attribute probe failed with status {}: {}", - output.status, - String::from_utf8_lossy(&output.stderr).trim() - ))); - } - parse_merge_attributes(&output.stdout, paths) -} - -fn parse_merge_attributes( +pub(crate) fn parse_merge_attributes( output: &[u8], expected_paths: &[String], ) -> io::Result> { @@ -145,7 +52,7 @@ fn parse_merge_attributes( Ok(attributes) } -fn untrusted_driver_selection( +pub(crate) fn untrusted_driver_selection( entries: &BTreeMap, attributes: &BTreeMap, ) -> io::Result> { diff --git a/codex-rs/git-utils/src/merge_driver_race_tests.rs b/codex-rs/git-utils/src/merge_driver_race_tests.rs index 4d43b9ed36..53d45f5b6a 100644 --- a/codex-rs/git-utils/src/merge_driver_race_tests.rs +++ b/codex-rs/git-utils/src/merge_driver_race_tests.rs @@ -3,6 +3,7 @@ use crate::apply::ApplyGitRequest; use crate::apply::apply_git_patch; use pretty_assertions::assert_eq; use std::fs::OpenOptions; +use std::io::Write; use std::path::Path; use std::path::PathBuf; use std::thread; diff --git a/codex-rs/git-utils/src/merge_driver_tests.rs b/codex-rs/git-utils/src/merge_driver_tests.rs index 5e67af5927..ba87579397 100644 --- a/codex-rs/git-utils/src/merge_driver_tests.rs +++ b/codex-rs/git-utils/src/merge_driver_tests.rs @@ -1,9 +1,19 @@ use super::*; use crate::apply::ApplyGitRequest; use crate::apply::apply_git_patch; +use crate::git_command::GitRunner; use crate::git_config::GitConfigOrigin; use crate::git_config::GitConfigScope; +use crate::guarded_config::GuardedGitConfig; +use crate::guarded_config::config_source_authorization_count; +use crate::guarded_config::merge_attribute_read_count; +use crate::guarded_config::merge_config_read_count; +use crate::guarded_config::merge_overlay_count; +use crate::guarded_config::reset_config_source_authorization_count; +use crate::guarded_config::reset_merge_policy_counts; +use pretty_assertions::assert_eq; use std::ffi::OsStr; +use std::path::Path; #[test] fn apply_allows_unused_global_merge_driver() { @@ -113,6 +123,8 @@ fn apply_rejects_global_merge_driver_before_three_way() { let (commit_code, _, commit_err) = run(root, &["git", "commit", "-am", "ours"]); assert_eq!(commit_code, 0, "commit ours: {commit_err}"); + reset_config_source_authorization_count(); + reset_merge_policy_counts(); let error = apply_git_patch(&ApplyGitRequest { cwd: root.to_path_buf(), diff, @@ -120,6 +132,10 @@ fn apply_rejects_global_merge_driver_before_three_way() { preflight: false, }) .expect_err("reject global merge driver"); + assert_eq!(config_source_authorization_count(), 1); + assert_eq!(merge_config_read_count(), 1); + assert_eq!(merge_attribute_read_count(), 1); + assert_eq!(merge_overlay_count(), 0); assert_eq!(error.kind(), io::ErrorKind::Unsupported); let (marker_code, _, _) = run(root, &["git", "config", "--get", "codex.mergeran"]); assert_ne!(marker_code, 0, "merge driver must not run"); @@ -165,6 +181,8 @@ fn three_way_apply_allows_unrelated_local_merge_driver() { "configure unused local driver: {config_err}" ); + reset_config_source_authorization_count(); + reset_merge_policy_counts(); let result = apply_git_patch(&ApplyGitRequest { cwd: root.to_path_buf(), diff, @@ -172,6 +190,10 @@ fn three_way_apply_allows_unrelated_local_merge_driver() { preflight: false, }) .expect("allow unrelated local merge driver during three-way fallback"); + assert_eq!(config_source_authorization_count(), 1); + assert_eq!(merge_config_read_count(), 1); + assert_eq!(merge_attribute_read_count(), 1); + assert_eq!(merge_overlay_count(), 1); assert_eq!(result.exit_code, 0, "three-way apply: {}", result.stderr); assert!(result.cmd_for_log.contains("--3way")); assert_eq!( @@ -206,6 +228,7 @@ fn apply_allows_clean_patch_with_selected_merge_driver() { ); assert_eq!(config_code, 0, "configure merge driver: {config_err}"); + reset_merge_policy_counts(); let result = apply_git_patch(&ApplyGitRequest { cwd: root.to_path_buf(), diff: "diff --git a/file.txt b/file.txt\n--- a/file.txt\n+++ b/file.txt\n@@ -1 +1 @@\n-old\n+new\n".to_string(), @@ -215,6 +238,9 @@ fn apply_allows_clean_patch_with_selected_merge_driver() { .expect("allow clean patch with selected merge driver"); assert_eq!(result.exit_code, 0); + assert_eq!(merge_config_read_count(), 0); + assert_eq!(merge_attribute_read_count(), 0); + assert_eq!(merge_overlay_count(), 0); assert!(!result.cmd_for_log.contains("--3way")); let contents = std::fs::read_to_string(root.join("file.txt")).expect("read file"); assert!( @@ -315,6 +341,7 @@ fn preflight_does_not_probe_or_run_selected_merge_driver() { ); assert_eq!(config_code, 0, "configure merge driver: {config_err}"); + reset_merge_policy_counts(); let result = apply_git_patch(&ApplyGitRequest { cwd: root.to_path_buf(), diff: "diff --git a/file.txt b/file.txt\n--- a/file.txt\n+++ b/file.txt\n@@ -1 +1 @@\n-old\n+new\n".to_string(), @@ -323,6 +350,9 @@ fn preflight_does_not_probe_or_run_selected_merge_driver() { }) .expect("preflight must not probe merge drivers"); assert_eq!(result.exit_code, 0); + assert_eq!(merge_config_read_count(), 0); + assert_eq!(merge_attribute_read_count(), 0); + assert_eq!(merge_overlay_count(), 0); assert_eq!( std::fs::read_to_string(root.join("file.txt")).expect("read file"), "old\n" @@ -331,6 +361,61 @@ fn preflight_does_not_probe_or_run_selected_merge_driver() { assert_ne!(marker_code, 0, "preflight must not run merge driver"); } +#[test] +fn merge_override_rejects_process_temp_directory_inside_worktree() { + let root_name = "CODEX_GIT_UTILS_MERGE_WORKTREE_TMP_ROOT"; + if let Some(root) = std::env::var_os(root_name) { + let root = std::path::PathBuf::from(root); + let patch = std::fs::read_to_string(root.join("fixture.patch")).expect("read patch"); + let before_tree = run(&root, &["git", "write-tree"]).1; + let before_contents = std::fs::read_to_string(root.join("file.txt")).expect("read ours"); + let error = apply_git_patch(&ApplyGitRequest { + cwd: root.clone(), + diff: patch, + revert: false, + preflight: false, + }) + .expect_err("reject worktree-owned merge override"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + assert_eq!(run(&root, &["git", "write-tree"]).1, before_tree); + assert_eq!( + std::fs::read_to_string(root.join("file.txt")).expect("read after"), + before_contents + ); + let (marker_code, _, _) = run(&root, &["git", "config", "--get", "codex.mergeran"]); + assert_ne!(marker_code, 0, "merge driver must not run"); + return; + } + + let repo = init_repo(); + let root = repo.path(); + let patch = build_three_way_fixture(root); + std::fs::write(root.join("fixture.patch"), patch).expect("write fixture patch"); + let (config_code, _, config_err) = run( + root, + &[ + "git", + "config", + "merge.unused.driver", + "git config codex.mergeran true && false", + ], + ); + assert_eq!(config_code, 0, "configure merge driver: {config_err}"); + let worktree_temp = root.join("process-temp"); + std::fs::create_dir(&worktree_temp).expect("worktree temp directory"); + run_isolated_merge_test( + "merge_driver::tests::merge_override_rejects_process_temp_directory_inside_worktree", + &[ + (root_name, root.as_os_str()), + ("TMPDIR", worktree_temp.as_os_str()), + #[cfg(windows)] + ("TEMP", worktree_temp.as_os_str()), + #[cfg(windows)] + ("TMP", worktree_temp.as_os_str()), + ], + ); +} + #[test] fn empty_name_driver_follows_effective_git_selection() { let repo = init_repo(); @@ -343,16 +428,14 @@ fn empty_name_driver_follows_effective_git_selection() { &["git", "config", "merge..driver", "empty-name helper"], ); assert_eq!(config_code, 0, "configure empty-name driver: {config_err}"); - let git = GitRunner::for_cwd_io(root).expect("trusted Git"); let paths = ["file.txt".to_string()]; - ensure_no_selected_merge_drivers(&git, root, &paths, &[]) - .expect("allow unused empty-name driver"); + install_merge_policy(root, &paths).expect("allow unused empty-name driver"); std::fs::write(root.join(".gitattributes"), "file.txt merge=\n") .expect("select empty-name driver"); - let selected_error = ensure_no_selected_merge_drivers(&git, root, &paths, &[]) - .expect_err("reject selected empty-name driver"); + let selected_error = + install_merge_policy(root, &paths).expect_err("reject selected empty-name driver"); assert_eq!(selected_error.kind(), io::ErrorKind::Unsupported); std::fs::write(root.join(".gitattributes"), "").expect("clear attributes"); @@ -361,18 +444,16 @@ fn empty_name_driver_follows_effective_git_selection() { default_code, 0, "configure empty merge.default: {default_err}" ); - let default_error = ensure_no_selected_merge_drivers(&git, root, &paths, &[]) + let default_error = install_merge_policy(root, &paths) .expect_err("reject empty-name driver selected by empty default"); assert_eq!(default_error.kind(), io::ErrorKind::Unsupported); let (empty_code, _, empty_err) = run(root, &["git", "config", "merge..driver", ""]); assert_eq!(empty_code, 0, "empty effective driver: {empty_err}"); - ensure_no_selected_merge_drivers(&git, root, &paths, &[]) - .expect("allow selected empty command through empty default"); + install_merge_policy(root, &paths).expect("allow selected empty command through empty default"); std::fs::write(root.join(".gitattributes"), "file.txt merge=\n") .expect("select empty-name driver"); - ensure_no_selected_merge_drivers(&git, root, &paths, &[]) - .expect("allow explicitly selected empty command"); + install_merge_policy(root, &paths).expect("allow explicitly selected empty command"); } #[test] @@ -524,6 +605,43 @@ fn config_entries(values: [(&str, &str); N]) -> BTreeMap io::Result<()> { + let git = GitRunner::for_cwd_io(root)?; + let mut config = GuardedGitConfig::authorize(&git, root, Vec::new())?; + config.authorize_filter_paths(paths)?; + config.install_three_way_merge_policy() +} + +fn build_three_way_fixture(root: &Path) -> String { + std::fs::write(root.join("file.txt"), "base\n").expect("write base"); + assert_eq!(run(root, &["git", "add", "file.txt"]).0, 0); + assert_eq!(run(root, &["git", "commit", "-m", "base"]).0, 0); + let base = run(root, &["git", "rev-parse", "HEAD"]).1; + std::fs::write(root.join("file.txt"), "theirs\n").expect("write theirs"); + assert_eq!(run(root, &["git", "add", "file.txt"]).0, 0); + assert_eq!(run(root, &["git", "commit", "-m", "theirs"]).0, 0); + let patch = run( + root, + &[ + "git", + "diff", + "--full-index", + base.trim(), + "HEAD", + "--", + "file.txt", + ], + ); + assert_eq!(patch.0, 0, "create patch: {}", patch.2); + assert_eq!( + run(root, &["git", "checkout", "-b", "ours", base.trim()]).0, + 0 + ); + std::fs::write(root.join("file.txt"), "ours\n").expect("write ours"); + assert_eq!(run(root, &["git", "commit", "-am", "ours"]).0, 0); + patch.1 +} + fn run(cwd: &Path, args: &[&str]) -> (i32, String, String) { let mut command = std::process::Command::new(args[0]); isolate_git_command_environment(&mut command); diff --git a/codex-rs/git-utils/src/operations.rs b/codex-rs/git-utils/src/operations.rs index 27c3e17ec4..24123d1ea4 100644 --- a/codex-rs/git-utils/src/operations.rs +++ b/codex-rs/git-utils/src/operations.rs @@ -7,8 +7,9 @@ use crate::GitToolingError; use crate::git_command::GitRunner; use crate::safe_git::DISABLED_HOOKS_PATH; -pub(crate) fn ensure_git_repository(path: &Path) -> Result<(), GitToolingError> { - match run_git_for_stdout( +pub(crate) fn ensure_git_repository(git: &GitRunner, path: &Path) -> Result<(), GitToolingError> { + match run_git_for_stdout_with_runner( + git, path, vec![ OsString::from("rev-parse"), @@ -29,8 +30,12 @@ pub(crate) fn ensure_git_repository(path: &Path) -> Result<(), GitToolingError> } } -pub(crate) fn resolve_head(path: &Path) -> Result, GitToolingError> { - match run_git_for_stdout( +pub(crate) fn resolve_head( + git: &GitRunner, + path: &Path, +) -> Result, GitToolingError> { + match run_git_for_stdout_with_runner( + git, path, vec![ OsString::from("rev-parse"), @@ -45,8 +50,12 @@ pub(crate) fn resolve_head(path: &Path) -> Result, GitToolingErro } } -pub(crate) fn resolve_repository_root(path: &Path) -> Result { - let root = run_git_for_stdout( +pub(crate) fn resolve_repository_root( + git: &GitRunner, + path: &Path, +) -> Result { + let root = run_git_for_stdout_with_runner( + git, path, vec![ OsString::from("rev-parse"), @@ -70,6 +79,7 @@ where Ok(()) } +#[cfg(test)] pub(crate) fn run_git_for_stdout( dir: &Path, args: I, @@ -79,7 +89,21 @@ where I: IntoIterator, S: AsRef, { - let run = run_git(dir, args, env)?; + let git = GitRunner::for_cwd_io(dir)?; + run_git_for_stdout_with_runner(&git, dir, args, env) +} + +pub(crate) fn run_git_for_stdout_with_runner( + git: &GitRunner, + dir: &Path, + args: I, + env: Option<&[(OsString, OsString)]>, +) -> Result +where + I: IntoIterator, + S: AsRef, +{ + let run = run_git_with_runner(git, dir, args, env)?; String::from_utf8(run.output.stdout) .map(|value| value.trim().to_string()) .map_err(|source| GitToolingError::GitOutputUtf8 { @@ -93,6 +117,20 @@ fn run_git( args: I, env: Option<&[(OsString, OsString)]>, ) -> Result +where + I: IntoIterator, + S: AsRef, +{ + let git = GitRunner::for_cwd_io(dir)?; + run_git_with_runner(&git, dir, args, env) +} + +fn run_git_with_runner( + git: &GitRunner, + dir: &Path, + args: I, + env: Option<&[(OsString, OsString)]>, +) -> Result where I: IntoIterator, S: AsRef, @@ -109,7 +147,6 @@ where args_vec.push(OsString::from(arg.as_ref())); } let command_string = build_command_string(&args_vec); - let git = GitRunner::for_cwd_io(dir)?; let mut command = git.command_for_cwd(dir)?; if let Some(envs) = env { for (key, value) in envs { diff --git a/codex-rs/git-utils/src/patch_paths.rs b/codex-rs/git-utils/src/patch_paths.rs index a77118c735..fceba6a5b3 100644 --- a/codex-rs/git-utils/src/patch_paths.rs +++ b/codex-rs/git-utils/src/patch_paths.rs @@ -5,13 +5,11 @@ use std::io; use std::path::Path; use std::path::PathBuf; -use crate::apply::run_git; -use crate::apply::safe_git_config_parts; use crate::apply::write_temp_patch; -use crate::exact_staging::update_index_exact_paths; +use crate::exact_staging::update_index_exact_paths_standalone; use crate::git_command::GitRunner; use crate::git_config::path_is_within; -use crate::git_config_sources::ensure_no_worktree_config_sources; +use crate::guarded_config::GuardedGitConfig; #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) struct PatchPathInventory { @@ -19,21 +17,17 @@ pub(crate) struct PatchPathInventory { pub(crate) effective_paths: Vec, } -/// Extract requested-orientation and effective paths with Git from a cwd whose -/// config sources have already been authorized for `git_config_args`. -pub(crate) fn extract_patch_path_inventory( - git: &GitRunner, - authorized_cwd: &Path, +/// Extract requested-orientation and effective paths through one bound +/// operation configuration. +pub(crate) fn extract_patch_path_inventory_guarded( + config: &GuardedGitConfig<'_>, patch_path: &Path, revert: bool, - git_config_args: &[String], ) -> io::Result { - let primary_paths = - git_apply_numstat_paths(git, authorized_cwd, patch_path, revert, git_config_args)?; + let primary_paths = git_apply_numstat_paths_guarded(config, patch_path, revert)?; // `git apply --numstat` reports only the destination of a rename. Parse the // opposite orientation too so both endpoints are included in the result. - let opposite_paths = - git_apply_numstat_paths(git, authorized_cwd, patch_path, !revert, git_config_args)?; + let opposite_paths = git_apply_numstat_paths_guarded(config, patch_path, !revert)?; if primary_paths.len() != opposite_paths.len() { return Err(io::Error::new( io::ErrorKind::InvalidData, @@ -65,17 +59,37 @@ pub(crate) fn extract_patch_path_inventory( }) } -pub(crate) fn extract_effective_paths_from_patch( - git: &GitRunner, - authorized_cwd: &Path, +/// Extract effective patch paths through a bound operation configuration. +pub(crate) fn extract_effective_paths_from_patch_guarded( + config: &GuardedGitConfig<'_>, patch_path: &Path, revert: bool, - git_config_args: &[String], ) -> io::Result> { - Ok( - extract_patch_path_inventory(git, authorized_cwd, patch_path, revert, git_config_args)? - .effective_paths, - ) + Ok(extract_patch_path_inventory_guarded(config, patch_path, revert)?.effective_paths) +} + +fn git_apply_numstat_paths_guarded( + config: &GuardedGitConfig<'_>, + patch_path: &Path, + revert: bool, +) -> io::Result> { + let mut command = config.apply_command()?; + command.args(["--numstat", "-z"]); + if revert { + command.arg("-R"); + } + command.arg("--").arg(patch_path); + let output = command.output()?; + if !output.status.success() { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + format!( + "failed to parse patch paths: {}", + String::from_utf8_lossy(&output.stderr).trim() + ), + )); + } + parse_numstat_paths(&output.stdout) } /// Best-effort extraction of the paths Git would apply. @@ -95,45 +109,18 @@ fn extract_paths_from_patch_from_cwd(diff_text: &str, cwd: &Path) -> Vec }; let paths = (|| -> io::Result> { let git = GitRunner::for_cwd_io(cwd)?; - let git_root = crate::get_git_repo_root(cwd) + let requested_cwd = std::fs::canonicalize(cwd)?; + let git_root = crate::get_git_repo_root(&requested_cwd) .ok_or_else(|| io::Error::other("not a Git repository"))?; let git_root = std::fs::canonicalize(git_root)?; - ensure_no_worktree_config_sources(&git, &git_root, &[])?; - extract_effective_paths_from_patch(&git, &git_root, &patch_path, /*revert*/ false, &[]) + let config = GuardedGitConfig::authorize(&git, &git_root, Vec::new())?; + extract_effective_paths_from_patch_guarded(&config, &patch_path, /*revert*/ false) })() .unwrap_or_default(); drop(tmpdir); paths } -fn git_apply_numstat_paths( - git: &GitRunner, - authorized_cwd: &Path, - patch_path: &Path, - revert: bool, - git_config_args: &[String], -) -> io::Result> { - let mut cmd = git.command_for_cwd(authorized_cwd)?; - cmd.args(git_config_args); - cmd.args(["apply", "--numstat", "-z"]); - if revert { - cmd.arg("-R"); - } - cmd.arg("--").arg(patch_path); - let out = git.output(cmd)?; - if !out.status.success() { - return Err(io::Error::new( - io::ErrorKind::InvalidInput, - format!( - "failed to parse patch paths: {}", - String::from_utf8_lossy(&out.stderr).trim() - ), - )); - } - - parse_numstat_paths(&out.stdout) -} - fn parse_numstat_paths(output: &[u8]) -> io::Result> { if !output.is_empty() && !output.ends_with(&[0]) { return Err(io::Error::new( @@ -275,31 +262,36 @@ fn invalid_windows_patch_component(component: &str) -> bool { /// Stage only the files that actually exist on disk for the given diff. pub fn stage_paths(git_root: &Path, diff: &str) -> io::Result<()> { let git = GitRunner::for_cwd_io(git_root)?; - let git_config_args = safe_git_config_parts(); - ensure_no_worktree_config_sources(&git, git_root, &git_config_args)?; + git.ensure_repository_root_route(git_root)?; + let canonical_root = std::fs::canonicalize(git_root)?; + let mut config = GuardedGitConfig::authorize(&git, &canonical_root, Vec::new())?; let (tmpdir, patch_path) = write_temp_patch(diff)?; - let paths = extract_effective_paths_from_patch( - &git, - git_root, - &patch_path, - /*revert*/ true, - &git_config_args, - )?; + let paths = + extract_effective_paths_from_patch_guarded(&config, &patch_path, /*revert*/ true)?; let _guard = tmpdir; - stage_effective_paths(&git, git_root, &paths, &git_config_args) + stage_effective_paths_standalone(&mut config, &paths) } -pub(crate) fn stage_effective_paths( - git: &GitRunner, - git_root: &Path, +fn stage_effective_paths_standalone( + config: &mut GuardedGitConfig<'_>, paths: &[String], - git_config_args: &[String], ) -> io::Result<()> { - let confined = confine_patch_paths(git, git_root, paths)?; + let (existing, content_filter_paths) = classify_exact_staging_leaves(config, paths)?; + let _result = update_index_exact_paths_standalone(config, &existing, &content_filter_paths)?; + // Preserve the public helper's historical best-effort treatment of a + // non-zero staging command. Security and probe failures still propagate. + Ok(()) +} + +fn classify_exact_staging_leaves( + config: &GuardedGitConfig<'_>, + paths: &[String], +) -> io::Result<(Vec, Vec)> { + let confined = confine_patch_paths_guarded(config, paths)?; let mut existing = Vec::new(); let mut content_filter_paths = Vec::new(); for path in confined.into_exact_leaves()? { - let joined = git_root.join(&path); + let joined = config.canonical_root().join(&path); if let Ok(metadata) = std::fs::symlink_metadata(&joined) { let file_type = metadata.file_type(); if leaf_is_traversable_directory(file_type) { @@ -313,19 +305,7 @@ pub(crate) fn stage_effective_paths( existing.push(path); } } - if existing.is_empty() { - return Ok(()); - } - let _result = update_index_exact_paths( - git, - git_root, - &existing, - &content_filter_paths, - git_config_args, - )?; - // Preserve the public helper's historical best-effort treatment of a - // non-zero staging command. Security and probe failures still propagate. - Ok(()) + Ok((existing, content_filter_paths)) } #[cfg(not(windows))] @@ -427,10 +407,33 @@ impl ConfinedPatchPaths { } } +#[cfg(test)] pub(crate) fn confine_patch_paths( git: &GitRunner, git_root: &Path, paths: &[String], +) -> io::Result { + let canonical_root = std::fs::canonicalize(git_root)?; + let config = GuardedGitConfig::authorize(git, &canonical_root, Vec::new())?; + confine_patch_paths_guarded(&config, paths) +} + +pub(crate) fn confine_patch_paths_guarded( + config: &GuardedGitConfig<'_>, + paths: &[String], +) -> io::Result { + let canonical_root = std::fs::canonicalize(config.canonical_root())?; + if paths.is_empty() || paths.iter().all(|path| !path.contains('/')) { + return confine_patch_paths_with_metadata(&canonical_root, paths, &[]); + } + let metadata_dirs = canonical_git_metadata_dirs_guarded(config)?; + confine_patch_paths_with_metadata(&canonical_root, paths, &metadata_dirs) +} + +fn confine_patch_paths_with_metadata( + canonical_root: &Path, + paths: &[String], + metadata_dirs: &[PathBuf], ) -> io::Result { if paths.is_empty() { return Ok(ConfinedPatchPaths { @@ -453,8 +456,6 @@ pub(crate) fn confine_patch_paths( }); } - let canonical_root = std::fs::canonicalize(git_root)?; - let metadata_dirs = canonical_git_metadata_dirs(git, &canonical_root)?; let mut entries = Vec::with_capacity(paths.len()); let mut prefix_cache = std::collections::BTreeMap::new(); @@ -468,9 +469,9 @@ pub(crate) fn confine_patch_paths( ); let (existing_len, mut projected) = longest_existing_strict_prefix( - &canonical_root, + canonical_root, &components, - &metadata_dirs, + metadata_dirs, &mut prefix_cache, )?; projected.extend( @@ -565,22 +566,27 @@ fn containment_error(message: &'static str) -> io::Error { io::Error::new(io::ErrorKind::PermissionDenied, message) } -fn canonical_git_metadata_dirs(git: &GitRunner, git_root: &Path) -> io::Result> { - let config_parts = safe_git_config_parts(); +fn canonical_git_metadata_dirs_guarded(config: &GuardedGitConfig<'_>) -> io::Result> { let queries = [ - vec!["rev-parse".to_string(), "--absolute-git-dir".to_string()], - vec!["rev-parse".to_string(), "--git-common-dir".to_string()], + ["rev-parse", "--absolute-git-dir"], + ["rev-parse", "--git-common-dir"], ]; let mut metadata_dirs = std::collections::BTreeSet::new(); for args in queries { - let (code, stdout, stderr) = run_git(git, git_root, &config_parts, &args)?; - if code != 0 { + #[cfg(test)] + CONTAINMENT_METADATA_QUERY_COUNT.with(|count| count.set(count.get() + 1)); + let mut command = config.rev_parse_command()?; + command.args(&args[1..]); + let output = command.output()?; + if !output.status.success() { return Err(io::Error::other(format!( - "failed to resolve Git repository metadata (exit {code}): {}", - stderr.trim() + "failed to resolve Git repository metadata (status {}): {}", + output.status, + String::from_utf8_lossy(&output.stderr).trim() ))); } - let path = stdout.trim_end_matches(['\r', '\n']); + let path = String::from_utf8_lossy(&output.stdout); + let path = path.trim_end_matches(['\r', '\n']); if path.is_empty() { return Err(io::Error::new( io::ErrorKind::InvalidData, @@ -591,13 +597,28 @@ fn canonical_git_metadata_dirs(git: &GitRunner, git_root: &Path) -> io::Result = const { std::cell::Cell::new(0) }; +} + +#[cfg(test)] +pub(crate) fn reset_containment_metadata_query_count() { + CONTAINMENT_METADATA_QUERY_COUNT.with(|count| count.set(0)); +} + +#[cfg(test)] +pub(crate) fn containment_metadata_query_count() -> usize { + CONTAINMENT_METADATA_QUERY_COUNT.with(std::cell::Cell::get) +} + fn insert_candidate_prefixes<'a>( components: impl IntoIterator, origin: ConfinedPathOrigin, diff --git a/codex-rs/git-utils/src/patch_paths_tests.rs b/codex-rs/git-utils/src/patch_paths_tests.rs index bcfea1e478..7f39edec53 100644 --- a/codex-rs/git-utils/src/patch_paths_tests.rs +++ b/codex-rs/git-utils/src/patch_paths_tests.rs @@ -1,8 +1,15 @@ use super::*; use crate::apply::ApplyGitRequest; use crate::apply::apply_git_patch; +use crate::apply::safe_git_config_parts; use crate::exact_staging::StagePathsResult; -use crate::exact_staging::update_index_exact_paths; +use crate::exact_staging::update_index_exact_paths_from_apply; +use crate::exact_staging::update_index_exact_paths_standalone; +use crate::guarded_config::config_source_authorization_count; +use crate::guarded_config::reset_config_source_authorization_count; +use crate::safe_git::filter_policy_overlay_count; +use crate::safe_git::filter_policy_read_count; +use crate::safe_git::reset_filter_policy_counts; use std::ffi::OsStr; use std::io; use std::path::Path; @@ -70,8 +77,8 @@ fn effective_paths(diff: &str, revert: bool) -> io::Result> { let git_root = crate::get_git_repo_root(cwd).ok_or_else(|| io::Error::other("not a Git repository"))?; let git_root = std::fs::canonicalize(git_root)?; - ensure_no_worktree_config_sources(&git, &git_root, &[])?; - let paths = extract_effective_paths_from_patch(&git, &git_root, &patch_path, revert, &[])?; + let config = GuardedGitConfig::authorize(&git, &git_root, Vec::new())?; + let paths = extract_effective_paths_from_patch_guarded(&config, &patch_path, revert)?; drop(tmpdir); Ok(paths) } @@ -81,6 +88,152 @@ fn best_effort_paths(diff: &str) -> Vec { extract_paths_from_patch_from_cwd(diff, repo.path()) } +#[cfg(unix)] +#[test] +fn best_effort_extraction_selects_the_physical_repo_from_a_symlinked_nested_cwd() { + let fixture = tempfile::tempdir().expect("fixture"); + let outer = fixture.path().join("outer"); + let target = fixture.path().join("target"); + let nested = target.join("nested"); + std::fs::create_dir_all(&outer).expect("outer"); + std::fs::create_dir_all(&nested).expect("nested"); + assert_eq!(run(&outer, &["git", "init", "-q"]).0, 0); + assert_eq!(run(&target, &["git", "init", "-q"]).0, 0); + let lexical_cwd = outer.join("linked-nested"); + std::os::unix::fs::symlink(&nested, &lexical_cwd).expect("symlink nested cwd"); + + assert_eq!( + extract_paths_from_patch_from_cwd(&new_file_diff("physical.txt"), &lexical_cwd), + vec!["physical.txt".to_string()] + ); +} + +#[cfg(unix)] +#[test] +fn standalone_stage_paths_preserves_a_symlinked_repository_route() { + let fixture = tempfile::tempdir().expect("fixture"); + let outer = fixture.path().join("outer"); + let target = fixture.path().join("target"); + std::fs::create_dir_all(&outer).expect("outer"); + std::fs::create_dir_all(&target).expect("target"); + assert_eq!(run(&outer, &["git", "init", "-q"]).0, 0); + assert_eq!(run(&target, &["git", "init", "-q"]).0, 0); + std::fs::write(target.join("routed.txt"), "routed\n").expect("write target file"); + let lexical_root = fixture.path().join("linked-root"); + std::os::unix::fs::symlink(&target, &lexical_root).expect("symlink repository root"); + + reset_config_source_authorization_count(); + stage_paths(&lexical_root, &new_file_diff("routed.txt")).expect("stage through symlink route"); + assert_eq!(config_source_authorization_count(), 1); + assert_eq!( + run(&target, &["git", "diff", "--cached", "--name-only"]).1, + "routed.txt\n" + ); + assert!( + run(&outer, &["git", "diff", "--cached", "--name-only"]) + .1 + .is_empty() + ); +} + +#[cfg(windows)] +#[test] +fn standalone_stage_paths_preserves_a_junction_repository_route() { + let fixture = tempfile::tempdir().expect("fixture"); + let outer = fixture.path().join("outer"); + let target = fixture.path().join("target"); + std::fs::create_dir_all(&outer).expect("outer"); + std::fs::create_dir_all(&target).expect("target"); + assert_eq!(run(&outer, &["git", "init", "-q"]).0, 0); + assert_eq!(run(&target, &["git", "init", "-q"]).0, 0); + std::fs::write(target.join("routed.txt"), "routed\n").expect("write target file"); + let lexical_root = fixture.path().join("linked-root"); + let _junction = create_dir_alias(&target, &lexical_root); + + reset_config_source_authorization_count(); + stage_paths(&lexical_root, &new_file_diff("routed.txt")).expect("stage through junction route"); + assert_eq!(config_source_authorization_count(), 1); + assert_eq!( + run(&target, &["git", "diff", "--cached", "--name-only"]).1, + "routed.txt\n" + ); + assert!( + run(&outer, &["git", "diff", "--cached", "--name-only"]) + .1 + .is_empty() + ); +} + +#[cfg(any(unix, windows))] +#[test] +fn standalone_stage_paths_preserves_an_independent_repository_alias() { + let fixture = tempfile::tempdir().expect("fixture"); + let independent = fixture.path().join("independent"); + let target = fixture.path().join("target"); + std::fs::create_dir_all(&independent).expect("independent"); + std::fs::create_dir_all(&target).expect("target"); + assert_eq!(run(&independent, &["git", "init", "-q"]).0, 0); + assert_eq!(run(&target, &["git", "init", "-q"]).0, 0); + std::fs::write(target.join("routed.txt"), "routed\n").expect("write target file"); + let lexical_root = independent.join("linked-root"); + let _route = create_dir_alias(&target, &lexical_root); + + stage_paths(&lexical_root, &new_file_diff("routed.txt")) + .expect("independent repository alias remains supported"); + assert_eq!( + run(&target, &["git", "diff", "--cached", "--name-only"]).1, + "routed.txt\n" + ); + assert!( + run(&independent, &["git", "diff", "--cached", "--name-only"]) + .1 + .is_empty() + ); +} + +#[cfg(any(unix, windows))] +#[test] +fn standalone_stage_paths_rejects_a_route_crossing_a_related_worktree() { + let fixture = tempfile::tempdir().expect("fixture"); + let primary = fixture.path().join("primary"); + let linked = fixture.path().join("linked"); + std::fs::create_dir_all(&primary).expect("primary"); + assert_eq!(run(&primary, &["git", "init", "-q"]).0, 0); + assert_eq!( + run( + &primary, + &["git", "config", "user.email", "codex@example.com"] + ) + .0, + 0 + ); + assert_eq!(run(&primary, &["git", "config", "user.name", "Codex"]).0, 0); + std::fs::write(primary.join("seed.txt"), "seed\n").expect("write seed"); + assert_eq!(run(&primary, &["git", "add", "seed.txt"]).0, 0); + assert_eq!(run(&primary, &["git", "commit", "-m", "seed"]).0, 0); + let linked_arg = linked.to_string_lossy().into_owned(); + assert_eq!( + run( + &primary, + &["git", "worktree", "add", "--detach", &linked_arg] + ) + .0, + 0 + ); + std::fs::write(linked.join("routed.txt"), "routed\n").expect("write linked file"); + let crossing_route = primary.join("linked-route"); + let _route = create_dir_alias(&linked, &crossing_route); + + let error = stage_paths(&crossing_route, &new_file_diff("routed.txt")) + .expect_err("related-worktree crossing must reject"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + assert!( + run(&linked, &["git", "diff", "--cached", "--name-only"]) + .1 + .is_empty() + ); +} + #[test] fn effective_paths_cover_supported_patch_headers() { let cases = [ @@ -530,9 +683,11 @@ fn index_entry(root: &Path, path: &str) -> String { fn exact_stage(root: &Path, paths: &[&str], content_filter_paths: &[&str]) -> StagePathsResult { let git = GitRunner::for_cwd_io(root).expect("trusted Git"); - update_index_exact_paths( - &git, - root, + let canonical_root = std::fs::canonicalize(root).expect("canonical root"); + let mut config = + GuardedGitConfig::authorize(&git, &canonical_root, Vec::new()).expect("authorized config"); + update_index_exact_paths_standalone( + &mut config, &paths .iter() .map(|path| (*path).to_string()) @@ -541,11 +696,251 @@ fn exact_stage(root: &Path, paths: &[&str], content_filter_paths: &[&str]) -> St .iter() .map(|path| (*path).to_string()) .collect::>(), - &safe_git_config_parts(), ) .expect("run exact staging primitive") } +fn stage_effective_paths( + git: &GitRunner, + root: &Path, + paths: &[String], + legacy_config_args: &[String], +) -> io::Result<()> { + let _ = legacy_config_args; + let canonical_root = std::fs::canonicalize(root)?; + let mut config = GuardedGitConfig::authorize(git, &canonical_root, Vec::new())?; + stage_effective_paths_standalone(&mut config, paths) +} + +#[test] +fn standalone_stage_paths_uses_one_source_authorization_and_one_filter_read() { + if std::env::var_os("CODEX_GIT_UTILS_PATH_ENV_CHILD").is_none() { + let config_dir = tempfile::tempdir().expect("config tempdir"); + let global_config = config_dir.path().join("global.gitconfig"); + let system_config = config_dir.path().join("system.gitconfig"); + std::fs::write(&global_config, "").expect("empty global config"); + std::fs::write(&system_config, "").expect("empty system config"); + run_isolated_test( + "patch_paths::tests::standalone_stage_paths_uses_one_source_authorization_and_one_filter_read", + &[ + ("GIT_CONFIG_GLOBAL", global_config.as_os_str()), + ("GIT_CONFIG_SYSTEM", system_config.as_os_str()), + ], + ); + return; + } + + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join("single.txt"), "single\n").expect("write staged file"); + + reset_config_source_authorization_count(); + reset_filter_policy_counts(); + stage_paths(root, &new_file_diff("single.txt")).expect("standalone exact staging"); + + assert_eq!(config_source_authorization_count(), 1); + assert_eq!(filter_policy_read_count(), 1); + assert_eq!(filter_policy_overlay_count(), 0); +} + +#[test] +fn composed_staging_freshly_observes_a_new_clean_driver_and_attribute() { + if std::env::var_os("CODEX_GIT_UTILS_PATH_ENV_CHILD").is_none() { + let config_dir = tempfile::tempdir().expect("config tempdir"); + let global_config = config_dir.path().join("global.gitconfig"); + let system_config = config_dir.path().join("system.gitconfig"); + std::fs::write(&global_config, "").expect("empty global config"); + std::fs::write(&system_config, "").expect("empty system config"); + run_isolated_test( + "patch_paths::tests::composed_staging_freshly_observes_a_new_clean_driver_and_attribute", + &[ + ("GIT_CONFIG_GLOBAL", global_config.as_os_str()), + ("GIT_CONFIG_SYSTEM", system_config.as_os_str()), + ], + ); + return; + } + + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join("file.txt"), "base\n").expect("write base file"); + assert_eq!(run(root, &["git", "add", "file.txt"]).0, 0); + assert_eq!(run(root, &["git", "commit", "-m", "base"]).0, 0); + std::fs::write(root.join("file.txt"), "changed\n").expect("modify file"); + + let git = GitRunner::for_cwd_io(root).expect("trusted Git"); + let canonical_root = std::fs::canonicalize(root).expect("canonical root"); + reset_config_source_authorization_count(); + reset_filter_policy_counts(); + let mut config = + GuardedGitConfig::authorize(&git, &canonical_root, Vec::new()).expect("authorization"); + config + .authorize_filter_paths(&["file.txt".to_string()]) + .expect("apply snapshot without filters"); + + assert_eq!( + run( + root, + &[ + "git", + "config", + "filter.fresh.clean", + "git config codex.stage-filter-ran true && git hash-object --stdin", + ], + ) + .0, + 0 + ); + std::fs::write(root.join(".gitattributes"), "file.txt filter=fresh\n") + .expect("select fresh filter"); + + let error = update_index_exact_paths_from_apply( + &mut config, + &["file.txt".to_string()], + &["file.txt".to_string()], + ) + .expect_err("fresh clean driver must refuse"); + assert_eq!(error.kind(), io::ErrorKind::Unsupported); + assert_eq!(config_source_authorization_count(), 1); + assert_eq!(filter_policy_read_count(), 2); + assert_eq!(filter_policy_overlay_count(), 1); + assert_ne!( + run(root, &["git", "config", "--get", "codex.stage-filter-ran"]).0, + 0, + "fresh policy must not execute the filter" + ); +} + +#[test] +fn composed_staging_required_read_ignores_the_apply_overlay_and_is_fresh() { + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join("file.txt"), "base\n").expect("write base file"); + assert_eq!(run(root, &["git", "add", "file.txt"]).0, 0); + assert_eq!(run(root, &["git", "commit", "-m", "base"]).0, 0); + std::fs::write(root.join("file.txt"), "changed\n").expect("modify file"); + assert_eq!( + run( + root, + &[ + "git", + "config", + "filter.demo.smudge", + "git config codex.stage-smudge-ran true && git hash-object --stdin", + ], + ) + .0, + 0 + ); + + let git = GitRunner::for_cwd_io(root).expect("trusted Git"); + let canonical_root = std::fs::canonicalize(root).expect("canonical root"); + reset_config_source_authorization_count(); + reset_filter_policy_counts(); + let mut config = + GuardedGitConfig::authorize(&git, &canonical_root, Vec::new()).expect("authorization"); + config + .authorize_filter_paths(&["file.txt".to_string()]) + .expect("unselected apply filter snapshot"); + + std::fs::write(root.join(".gitattributes"), "file.txt filter=demo\n") + .expect("select smudge-only filter"); + assert_eq!( + run(root, &["git", "config", "filter.demo.required", "true"]).0, + 0 + ); + + let error = update_index_exact_paths_from_apply( + &mut config, + &["file.txt".to_string()], + &["file.txt".to_string()], + ) + .expect_err("fresh required=true must refuse"); + assert_eq!(error.kind(), io::ErrorKind::Unsupported); + assert_eq!(config_source_authorization_count(), 1); + assert_eq!(filter_policy_read_count(), 2); + assert_eq!(filter_policy_overlay_count(), 2); + assert_ne!( + run(root, &["git", "config", "--get", "codex.stage-smudge-ran"]).0, + 0, + "smudge helper must remain neutralized" + ); +} + +#[test] +fn composed_exact_staging_refuses_requested_and_mapped_paths_outside_apply_universe() { + let requested_repo = init_repo(); + let requested_root = requested_repo.path(); + let requested_git = GitRunner::for_cwd_io(requested_root).expect("trusted Git"); + let mut requested_config = GuardedGitConfig::authorize( + &requested_git, + &std::fs::canonicalize(requested_root).expect("canonical root"), + Vec::new(), + ) + .expect("authorization"); + requested_config + .authorize_filter_paths(&["allowed.txt".to_string()]) + .expect("apply snapshot"); + let requested_error = update_index_exact_paths_from_apply( + &mut requested_config, + &["allowed.txt".to_string(), "outside.txt".to_string()], + &["outside.txt".to_string()], + ) + .expect_err("requested off-universe path must refuse"); + assert_eq!(requested_error.kind(), io::ErrorKind::PermissionDenied); + + let mapped_repo = init_repo(); + let mapped_root = mapped_repo.path(); + std::fs::write(mapped_root.join("File.txt"), "tracked\n").expect("write tracked spelling"); + assert_eq!(run(mapped_root, &["git", "add", "File.txt"]).0, 0); + assert_eq!(run(mapped_root, &["git", "commit", "-m", "base"]).0, 0); + std::fs::write(mapped_root.join("file.txt"), "alias\n").expect("write alias spelling"); + assert_eq!( + run(mapped_root, &["git", "config", "core.ignoreCase", "true"]).0, + 0 + ); + let mapped_git = GitRunner::for_cwd_io(mapped_root).expect("trusted Git"); + let mut mapped_config = GuardedGitConfig::authorize( + &mapped_git, + &std::fs::canonicalize(mapped_root).expect("canonical root"), + Vec::new(), + ) + .expect("authorization"); + mapped_config + .authorize_filter_paths(&["file.txt".to_string()]) + .expect("narrow apply snapshot"); + let mapped_error = update_index_exact_paths_from_apply( + &mut mapped_config, + &["File.txt".to_string(), "file.txt".to_string()], + &["file.txt".to_string()], + ) + .expect_err("mapped off-universe path must refuse"); + assert_eq!(mapped_error.kind(), io::ErrorKind::PermissionDenied); +} + +#[test] +fn composed_exact_staging_refuses_zero_or_two_prior_snapshots_even_when_empty() { + let repo = init_repo(); + let root = repo.path(); + let git = GitRunner::for_cwd_io(root).expect("trusted Git"); + let canonical_root = std::fs::canonicalize(root).expect("canonical root"); + + let mut zero = + GuardedGitConfig::authorize(&git, &canonical_root, Vec::new()).expect("authorization"); + let error = update_index_exact_paths_from_apply(&mut zero, &[], &[]) + .expect_err("zero snapshots must refuse"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + + let mut two = + GuardedGitConfig::authorize(&git, &canonical_root, Vec::new()).expect("authorization"); + two.authorize_filter_paths(&[]).expect("apply snapshot"); + two.authorize_git_add_filter_paths(&[]) + .expect("Git-add snapshot"); + let error = update_index_exact_paths_from_apply(&mut two, &[], &[]) + .expect_err("two snapshots must refuse"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); +} + fn assert_exact_refused_unchanged( root: &Path, paths: &[&str], @@ -598,6 +993,38 @@ impl Drop for DirectoryAlias { } } +#[test] +fn confinement_skips_metadata_queries_only_for_paths_without_ancestors() { + let _g = env_lock().lock().unwrap(); + let repo = init_repo(); + let root = repo.path(); + let git = GitRunner::for_cwd_io(root).expect("Git runner"); + let config = GuardedGitConfig::authorize(&git, root, Vec::new()).expect("authorization"); + + reset_containment_metadata_query_count(); + confine_patch_paths_guarded(&config, &["leaf".to_string()]).expect("single leaf"); + assert_eq!(containment_metadata_query_count(), 0); + + reset_containment_metadata_query_count(); + confine_patch_paths_guarded(&config, &["nested/leaf".to_string()]).expect("nested leaf"); + assert_eq!( + containment_metadata_query_count(), + 2, + "nested paths must still resolve private and common Git metadata" + ); +} + +#[test] +fn single_component_git_directory_is_refused_without_metadata_queries() { + let _g = env_lock().lock().unwrap(); + let repo = init_repo(); + let before = git_index_bytes(repo.path()); + + reset_containment_metadata_query_count(); + assert_stage_refused(repo.path(), ".git", &before); + assert_eq!(containment_metadata_query_count(), 0); +} + #[cfg(any(unix, windows))] fn assert_longest_existing_alias_prefix_carries_unresolved_suffix() { let repo = init_repo(); diff --git a/codex-rs/git-utils/src/repository_authority/authority.rs b/codex-rs/git-utils/src/repository_authority/authority.rs index 6c9b517e82..ff00ccafe9 100644 --- a/codex-rs/git-utils/src/repository_authority/authority.rs +++ b/codex-rs/git-utils/src/repository_authority/authority.rs @@ -146,6 +146,20 @@ impl RepositoryAuthority { }) } + pub(crate) fn ensure_active_worktree_root(&self, root: &Path) -> io::Result<()> { + let canonical_root = std::fs::canonicalize(root)?; + let same_root = canonical_root == self.active_worktree_root + || same_file::is_same_file(&canonical_root, &self.active_worktree_root)?; + if !same_root { + return Err(authority_refusal(format!( + "guarded Git repository root {} does not match runner repository {}", + canonical_root.display(), + self.active_worktree_root.display() + ))); + } + Ok(()) + } + #[cfg(test)] pub(crate) fn from_test_locations( roots: Vec, diff --git a/codex-rs/git-utils/src/repository_authority/authority/policy.rs b/codex-rs/git-utils/src/repository_authority/authority/policy.rs index 7539bc48a3..6995d06c93 100644 --- a/codex-rs/git-utils/src/repository_authority/authority/policy.rs +++ b/codex-rs/git-utils/src/repository_authority/authority/policy.rs @@ -17,6 +17,37 @@ use super::super::repository_common_dir_for_candidate_root; use super::RepositoryAuthority; impl RepositoryAuthority { + pub(crate) fn ensure_repository_root_route(&self, root: &Path) -> io::Result<()> { + self.ensure_active_worktree_root(root)?; + let crosses_worktree = self.route_boundaries.inspect_route(root)?.crosses_worktree; + let crosses_related_worktree = match root.parent() { + Some(parent) => self.has_active_common_dir_ancestor(parent)?, + None => false, + }; + if crosses_worktree && crosses_related_worktree { + return Err(authority_refusal(format!( + "refusing repository root route that crosses a worktree boundary: {}", + root.display() + ))); + } + Ok(()) + } + + fn has_active_common_dir_ancestor(&self, path: &Path) -> io::Result { + let Some(active) = &self.active_metadata else { + return Ok(false); + }; + for ancestor in path.ancestors() { + let Some(common_dir) = repository_common_dir_for_candidate_root(ancestor)? else { + continue; + }; + if directories_refer_to_same_location(&common_dir, &active.common_dir)? { + return Ok(true); + } + } + Ok(false) + } + pub(crate) fn canonical_command_cwd(&self, cwd: &Path) -> io::Result { let canonical = std::fs::canonicalize(cwd)?; let inspection = self.route_boundaries.inspect_route(cwd)?; diff --git a/codex-rs/git-utils/src/reverse_apply_tests.rs b/codex-rs/git-utils/src/reverse_apply_tests.rs index 37fd0dac57..c700a6b803 100644 --- a/codex-rs/git-utils/src/reverse_apply_tests.rs +++ b/codex-rs/git-utils/src/reverse_apply_tests.rs @@ -1,5 +1,12 @@ use super::*; +use crate::guarded_config::config_source_authorization_count; +use crate::guarded_config::merge_attribute_read_count; +use crate::guarded_config::merge_config_read_count; +use crate::guarded_config::reset_config_source_authorization_count; +use crate::guarded_config::reset_merge_policy_counts; +use crate::safe_git::filter_policy_read_count; use crate::safe_git::isolate_git_command_environment; +use crate::safe_git::reset_filter_policy_counts; use pretty_assertions::assert_eq; use std::ffi::OsStr; use std::path::Path; @@ -576,3 +583,77 @@ fn direct_success_paths_are_machine_readable_in_each_orientation() { assert_eq!(result.applied_paths, vec![expected]); } } + +#[test] +fn reverse_three_way_with_staging_candidate_uses_one_authorization() { + let child_marker = "CODEX_GIT_UTILS_REVERSE_THREE_WAY_TRACE_CHILD"; + if std::env::var_os(child_marker).is_none() { + let environment = tempfile::tempdir().expect("isolated Git environment"); + let global_config = environment.path().join("global.gitconfig"); + let system_config = environment.path().join("system.gitconfig"); + std::fs::write(&global_config, "").expect("empty global config"); + std::fs::write(&system_config, "").expect("empty system config"); + let mut command = std::process::Command::new(std::env::current_exe().expect("test binary")); + isolate_git_command_environment(&mut command); + let output = command + .arg("apply::reverse_apply_tests::reverse_three_way_with_staging_candidate_uses_one_authorization") + .arg("--exact") + .arg("--nocapture") + .env(child_marker, "1") + .env("GIT_CONFIG_GLOBAL", &global_config) + .env("GIT_CONFIG_SYSTEM", &system_config) + .env("RUST_TEST_THREADS", "1") + .output() + .expect("run isolated reverse three-way test"); + assert!( + output.status.success(), + "isolated reverse three-way test failed:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + return; + } + + let repo = init_repo(); + let root = repo.path(); + let base_contents = "01\n02\n03\n04\n05\n06\n07\n08\n09\n10\nbase\n12\n13\n14\n15\n"; + let theirs_contents = "01\n02\n03\n04\n05\n06\n07\n08\n09\n10\ntheirs\n12\n13\n14\n15\n"; + let independently_edited = + "01\n02\n03\n04\n05\n06\n07\nEIGHT\n09\n10\ntheirs\n12\n13\n14\n15\n"; + let expected = "01\n02\n03\n04\n05\n06\n07\nEIGHT\n09\n10\nbase\n12\n13\n14\n15\n"; + std::fs::write(root.join("file.txt"), base_contents).expect("write base"); + run_success(root, &["add", "file.txt"]); + run_success(root, &["commit", "-qm", "base"]); + let base = String::from_utf8(run_success(root, &["rev-parse", "HEAD"])).expect("base OID"); + std::fs::write(root.join("file.txt"), theirs_contents).expect("write theirs"); + run_success(root, &["add", "file.txt"]); + run_success(root, &["commit", "-qm", "theirs"]); + let patch = String::from_utf8(run_success( + root, + &[ + "diff", + "--full-index", + base.trim(), + "HEAD", + "--", + "file.txt", + ], + )) + .expect("patch"); + // Change a context line so a plain reverse check fails, while a three-way + // reverse can preserve the independent edit after exact staging. + std::fs::write(root.join("file.txt"), independently_edited).expect("write independent edit"); + + reset_config_source_authorization_count(); + reset_filter_policy_counts(); + reset_merge_policy_counts(); + let result = apply_git_patch(&request(root, &patch)).expect("reverse three-way apply"); + + assert_eq!(result.exit_code, 0, "{}", result.stderr); + assert!(result.cmd_for_log.contains("--3way")); + assert_eq!(config_source_authorization_count(), 1); + assert_eq!(filter_policy_read_count(), 2); + assert_eq!(merge_config_read_count(), 1); + assert_eq!(merge_attribute_read_count(), 1); + assert_eq!(read_file_normalized(&root.join("file.txt")), expected); +} diff --git a/codex-rs/git-utils/src/reverse_staging.rs b/codex-rs/git-utils/src/reverse_staging.rs index 0f7cd21ea1..c82d07eb87 100644 --- a/codex-rs/git-utils/src/reverse_staging.rs +++ b/codex-rs/git-utils/src/reverse_staging.rs @@ -3,29 +3,21 @@ use std::collections::BTreeMap; use std::collections::BTreeSet; use std::io; -use std::path::Path; -use crate::exact_staging::update_index_exact_paths; -use crate::git_command::GitRunner; -use crate::patch_paths::confine_patch_paths; +use crate::exact_staging::update_index_exact_paths_from_apply; +use crate::guarded_config::GuardedGitConfig; +use crate::patch_paths::confine_patch_paths_guarded; use crate::patch_paths::leaf_is_traversable_directory; use crate::patch_paths::leaf_may_run_git_content_filter; -#[cfg(unix)] -const WORKTREE_FILEMODE_CONFIG_ARGS: &[&str] = &["-c", "core.filemode=true"]; -#[cfg(not(unix))] -const WORKTREE_FILEMODE_CONFIG_ARGS: &[&str] = &[]; - pub(crate) fn stage_effective_paths_for_reverse( - git: &GitRunner, - git_root: &Path, + config: &mut GuardedGitConfig<'_>, paths: &[String], - git_config_args: &[String], ) -> io::Result<()> { - let confined = confine_patch_paths(git, git_root, paths)?; + let confined = confine_patch_paths_guarded(config, paths)?; let mut worktree_paths = BTreeMap::new(); for path in confined.into_exact_leaves()? { - let joined = git_root.join(&path); + let joined = config.canonical_root().join(&path); let state = match std::fs::symlink_metadata(&joined) { Ok(metadata) => { let file_type = metadata.file_type(); @@ -58,7 +50,7 @@ pub(crate) fn stage_effective_paths_for_reverse( } let exact_paths = worktree_paths.keys().cloned().collect::>(); - let index_entries = read_reverse_index_entries(git, git_root, &exact_paths, git_config_args)?; + let index_entries = read_reverse_index_entries(config, &exact_paths)?; for path in &exact_paths { let entries = index_entries.get(path).map(Vec::as_slice).unwrap_or(&[]); if !matches!( @@ -75,43 +67,12 @@ pub(crate) fn stage_effective_paths_for_reverse( } let cached_invisible_status = read_name_status( - git, - git_root, + config, &exact_paths, - git_config_args, - &[ - "--literal-pathspecs", - "diff", - "--cached", - "--name-status", - "-z", - "--no-renames", - "--ita-invisible-in-index", - "--no-ext-diff", - "--no-textconv", - "--ignore-submodules=none", - ], - "inspect reverse paths with intent-to-add entries hidden", - )?; - let cached_visible_status = read_name_status( - git, - git_root, - &exact_paths, - git_config_args, - &[ - "--literal-pathspecs", - "diff", - "--cached", - "--name-status", - "-z", - "--no-renames", - "--ita-visible-in-index", - "--no-ext-diff", - "--no-textconv", - "--ignore-submodules=none", - ], - "inspect reverse paths with intent-to-add entries visible", + ReverseCachedView::IntentToAddInvisible, )?; + let cached_visible_status = + read_name_status(config, &exact_paths, ReverseCachedView::IntentToAddVisible)?; if cached_invisible_status != cached_visible_status { let path = exact_paths .iter() @@ -121,24 +82,7 @@ pub(crate) fn stage_effective_paths_for_reverse( "refusing to stage reverse patch path {path:?} because it has intent-to-add index state" ))); } - let worktree_changed_paths = read_changed_paths( - git, - git_root, - &exact_paths, - git_config_args, - WORKTREE_FILEMODE_CONFIG_ARGS, - &[ - "--literal-pathspecs", - "diff-files", - "--name-only", - "-z", - "--no-renames", - "--no-ext-diff", - "--no-textconv", - "--ignore-submodules=none", - ], - "compare reverse path index entries with the worktree", - )?; + let worktree_changed_paths = read_changed_paths(config, &exact_paths)?; let mut staging_candidates = Vec::new(); for path in &exact_paths { @@ -178,13 +122,8 @@ pub(crate) fn stage_effective_paths_for_reverse( }) .cloned() .collect::>(); - let result = update_index_exact_paths( - git, - git_root, - &staging_candidates, - &content_filter_paths, - git_config_args, - )?; + let result = + update_index_exact_paths_from_apply(config, &staging_candidates, &content_filter_paths)?; if result.exit_code == 0 { Ok(()) } else { @@ -209,24 +148,10 @@ struct ReverseIndexEntry { } fn read_reverse_index_entries( - git: &GitRunner, - git_root: &Path, + config: &GuardedGitConfig<'_>, paths: &[String], - git_config_args: &[String], ) -> io::Result>> { - let mut command = git.command_for_cwd(git_root)?; - command - .args(git_config_args) - .args([ - "--literal-pathspecs", - "ls-files", - "-v", - "--stage", - "-z", - "--", - ]) - .args(paths); - let output = git.output(command)?; + let output = config.reverse_index_stage_output(paths)?; if !output.status.success() { return Err(io::Error::other(format!( "failed to inspect reverse patch index entries: {}", @@ -295,17 +220,35 @@ fn read_reverse_index_entries( Ok(entries) } +#[derive(Clone, Copy)] +enum ReverseCachedView { + IntentToAddInvisible, + IntentToAddVisible, +} + fn read_name_status( - git: &GitRunner, - git_root: &Path, + config: &GuardedGitConfig<'_>, paths: &[String], - git_config_args: &[String], - args: &[&str], - operation: &str, + view: ReverseCachedView, ) -> io::Result> { - let output = - run_path_list_command(git, git_root, paths, git_config_args, &[], args, operation)?; - let fields = nul_fields(&output, operation)?; + let (output, operation) = match view { + ReverseCachedView::IntentToAddInvisible => ( + config.reverse_cached_name_status_invisible_output(paths)?, + "inspect reverse paths with intent-to-add entries hidden", + ), + ReverseCachedView::IntentToAddVisible => ( + config.reverse_cached_name_status_visible_output(paths)?, + "inspect reverse paths with intent-to-add entries visible", + ), + }; + if !output.status.success() { + return Err(io::Error::other(format!( + "failed to {operation} (status {}): {}", + output.status, + String::from_utf8_lossy(&output.stderr).trim() + ))); + } + let fields = nul_fields(&output.stdout, operation)?; if fields.len() % 2 != 0 { return Err(invalid_reverse_index_output( "git diff returned an incomplete name-status record", @@ -331,26 +274,21 @@ fn read_name_status( } fn read_changed_paths( - git: &GitRunner, - git_root: &Path, + config: &GuardedGitConfig<'_>, paths: &[String], - git_config_args: &[String], - extra_git_args: &[&str], - args: &[&str], - operation: &str, ) -> io::Result> { - let output = run_path_list_command( - git, - git_root, - paths, - git_config_args, - extra_git_args, - args, - operation, - )?; + let operation = "compare reverse path index entries with the worktree"; + let output = config.reverse_worktree_changed_paths_output(paths)?; + if !output.status.success() { + return Err(io::Error::other(format!( + "failed to {operation} (status {}): {}", + output.status, + String::from_utf8_lossy(&output.stderr).trim() + ))); + } let expected = paths.iter().map(String::as_str).collect::>(); let mut changed = BTreeSet::new(); - for path in nul_fields(&output, operation)? { + for path in nul_fields(&output.stdout, operation)? { let path = parse_expected_path(path, &expected, operation)?; if !changed.insert(path) { return Err(invalid_reverse_index_output( @@ -361,33 +299,6 @@ fn read_changed_paths( Ok(changed) } -fn run_path_list_command( - git: &GitRunner, - git_root: &Path, - paths: &[String], - git_config_args: &[String], - extra_git_args: &[&str], - args: &[&str], - operation: &str, -) -> io::Result> { - let mut command = git.command_for_cwd(git_root)?; - command - .args(git_config_args) - .args(extra_git_args) - .args(args) - .arg("--") - .args(paths); - let output = git.output(command)?; - if !output.status.success() { - return Err(io::Error::other(format!( - "failed to {operation} (status {}): {}", - output.status, - String::from_utf8_lossy(&output.stderr).trim() - ))); - } - Ok(output.stdout) -} - fn reverse_staging_error(message: impl Into) -> io::Error { io::Error::new(io::ErrorKind::PermissionDenied, message.into()) } diff --git a/codex-rs/git-utils/src/safe_git.rs b/codex-rs/git-utils/src/safe_git.rs index 87ecde2f63..e999252790 100644 --- a/codex-rs/git-utils/src/safe_git.rs +++ b/codex-rs/git-utils/src/safe_git.rs @@ -1,28 +1,22 @@ use std::collections::BTreeMap; use std::collections::BTreeSet; -use std::ffi::OsString; use std::io; use std::io::Seek; use std::io::Write; -use std::path::Path; -use std::path::PathBuf; use std::process::Command; use std::process::Stdio; use tokio::time::Duration; -use crate::git_command::GitRunner; -use crate::git_command::MAX_INTERNAL_GIT_OUTPUT_BYTES; use crate::git_config::GitConfigEntry; -use crate::git_config::read_effective_config_with_fallback as read_effective_config_unchecked; -use crate::git_config::read_effective_config_with_fallback_async as read_effective_config_unchecked_async; -use crate::git_config_sources::ensure_no_worktree_config_sources; -use crate::git_config_sources::ensure_no_worktree_config_sources_async; +use crate::guarded_config::GuardedGitConfig; +use crate::guarded_config::SealedFilterConfigOverride; #[path = "filter_sentinel.rs"] mod filter_sentinel; pub(crate) use filter_sentinel::SentinelFilterProbeBudget; pub(crate) use filter_sentinel::SentinelFilterProbeResolution; pub(crate) use filter_sentinel::classify_sentinel_filter_probes; +#[cfg(test)] pub(crate) use filter_sentinel::sentinel_filter_probe_config_args; pub(crate) const DISABLED_HOOKS_PATH: &str = if cfg!(windows) { "NUL" } else { "/dev/null" }; pub(crate) const EXECUTABLE_FILTER_CONFIG_PATTERN: &str = @@ -30,119 +24,62 @@ pub(crate) const EXECUTABLE_FILTER_CONFIG_PATTERN: &str = pub(crate) const MAX_EXECUTABLE_FILTER_DRIVERS: usize = 256; /// Timeout for internal Git commands to prevent freezing on large repositories. pub(crate) const GIT_COMMAND_TIMEOUT: Duration = Duration::from_secs(5); - -#[derive(Debug)] -pub(crate) struct GitConfigOverrideFile { - git_config_args: [String; 2], - config_path: PathBuf, - _config_dir: tempfile::TempDir, -} - -impl GitConfigOverrideFile { - pub(crate) fn new(file_name: &str) -> io::Result { - let config_dir = tempfile::tempdir()?; - let config_path = config_dir.path().join(file_name); - std::fs::write(&config_path, [])?; - let config_path_arg = config_path.to_str().ok_or_else(|| { - io::Error::new( - io::ErrorKind::InvalidData, - "non-UTF-8 Git config override path", - ) - })?; - Ok(Self { - git_config_args: ["-c".to_string(), format!("include.path={config_path_arg}")], - config_path, - _config_dir: config_dir, - }) - } - - pub(crate) fn git_config_args(&self) -> &[String] { - &self.git_config_args - } - - pub(crate) fn add_value( - &self, - git: &GitRunner, - cwd: &Path, - key: &str, - value: &str, - description: &str, - ) -> io::Result<()> { - let mut command = git.command_for_cwd(cwd)?; - command.args(self.add_value_args(key, value)); - let output = git.output(command)?; - Self::check_add_value_output(output, description) - } - - pub(crate) async fn add_value_async( - &self, - git: &GitRunner, - cwd: &Path, - key: &str, - value: &str, - description: &str, - ) -> io::Result<()> { - let mut command = git.async_command_for_cwd(cwd)?; - command.args(self.add_value_args(key, value)); - let output = git - .output_async_bounded(command, MAX_INTERNAL_GIT_OUTPUT_BYTES) - .await?; - Self::check_add_value_output(output, description) - } - - fn add_value_args(&self, key: &str, value: &str) -> [OsString; 6] { - [ - OsString::from("config"), - OsString::from("--file"), - self.config_path.as_os_str().to_os_string(), - OsString::from("--add"), - OsString::from(key), - OsString::from(value), - ] - } - - fn check_add_value_output(output: std::process::Output, description: &str) -> io::Result<()> { - if !output.status.success() { - return Err(io::Error::other(format!( - "failed to write {description} (status {}): {}", - output.status, - String::from_utf8_lossy(&output.stderr).trim() - ))); - } - Ok(()) - } -} - #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) enum FilterAttributeValue { Driver(String), AmbiguousSentinel(String), } -pub(crate) struct GitFilterNeutralization { - config_override: Option, - filter_config: BTreeMap, +/// Complete executable-filter inventory derived from one authorized effective +/// config read. The private field prevents a caller from minting an incomplete +/// inventory and treating its neutralizer as complete. +pub(crate) struct ExecutableFilterDrivers(BTreeSet); + +impl ExecutableFilterDrivers { + pub(crate) fn iter(&self) -> impl Iterator { + self.0.iter() + } + + pub(crate) fn is_empty(&self) -> bool { + self.0.is_empty() + } + + pub(crate) fn contains(&self, driver: &str) -> bool { + self.0.contains(driver) + } + + pub(crate) fn len(&self) -> usize { + self.0.len() + } } -impl GitFilterNeutralization { - pub(crate) fn git_config_args(&self) -> &[String] { - self.config_override - .as_ref() - .map(GitConfigOverrideFile::git_config_args) - .unwrap_or_default() +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum FilterPolicyRole { + Apply, + GitAdd, +} + +pub(crate) struct FilterPolicySnapshot { + role: FilterPolicyRole, + checked_paths: BTreeSet, + neutralizer: Option, +} + +impl FilterPolicySnapshot { + pub(crate) fn neutralizer(&self) -> Option<&SealedFilterConfigOverride> { + self.neutralizer.as_ref() } - #[cfg(test)] - pub(crate) fn filter_value(&self, driver: &str, name: &str) -> Option<&str> { - effective_filter_value(&self.filter_config, driver, name) + pub(crate) fn role(&self) -> FilterPolicyRole { + self.role } - pub(crate) fn selected_filter_policy( - &self, - driver: &str, - required: Option, - ) -> SelectedFilterPolicy { - classify_selected_filter(&self.filter_config, driver, required) + pub(crate) fn contains_checked_path(&self, path: &str) -> bool { + self.checked_paths.contains(path) + } + + pub(crate) fn checked_paths(&self) -> Vec { + self.checked_paths.iter().cloned().collect() } } @@ -153,12 +90,21 @@ pub(crate) enum SelectedFilterPolicy { Allowed, } -const FILTER_NEUTRALIZATION_PLAN: [(&str, &str); 4] = [ - ("clean", ""), - ("smudge", ""), - ("process", ""), - ("required", "false"), -]; +pub(crate) fn validate_executable_driver_count( + executable_drivers: &ExecutableFilterDrivers, +) -> io::Result<()> { + if executable_drivers.len() > MAX_EXECUTABLE_FILTER_DRIVERS { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + format!( + "Git filter driver count {} exceeds the status guard limit {}", + executable_drivers.len(), + MAX_EXECUTABLE_FILTER_DRIVERS + ), + )); + } + Ok(()) +} const ISOLATED_GIT_ENVIRONMENT: [&str; 11] = [ "GIT_DIR", @@ -187,44 +133,12 @@ pub(crate) fn isolate_git_command_environment(command: &mut Command) { } } -pub(crate) fn ensure_no_selected_executable_git_filters( - git: &GitRunner, - cwd: &Path, +pub(crate) fn build_filter_policy_snapshot( + config: &GuardedGitConfig<'_>, paths: &[String], - git_config_args: &[String], -) -> io::Result { - ensure_no_selected_executable_git_filters_for( - git, - cwd, - paths, - git_config_args, - FilterExecution::AnyWorktreeOperation, - ) -} - -pub(crate) fn ensure_no_selected_git_add_filters( - git: &GitRunner, - cwd: &Path, - paths: &[String], - git_config_args: &[String], -) -> io::Result { - ensure_no_selected_executable_git_filters_for( - git, - cwd, - paths, - git_config_args, - FilterExecution::GitAdd, - ) -} - -fn ensure_no_selected_executable_git_filters_for( - git: &GitRunner, - cwd: &Path, - paths: &[String], - git_config_args: &[String], execution: FilterExecution, -) -> io::Result { - let entries = read_filter_config(git, cwd, git_config_args).map_err(|error| { +) -> io::Result { + let entries = read_filter_config(config).map_err(|error| { if matches!(execution, FilterExecution::GitAdd) && error.kind() == io::ErrorKind::InvalidData { @@ -237,25 +151,26 @@ fn ensure_no_selected_executable_git_filters_for( } })?; let executable_drivers = executable_filter_drivers(&entries)?; + let role = match execution { + FilterExecution::AnyWorktreeOperation => FilterPolicyRole::Apply, + FilterExecution::GitAdd => FilterPolicyRole::GitAdd, + }; + let checked_paths = paths.iter().cloned().collect(); if executable_drivers.is_empty() { - return Ok(GitFilterNeutralization { - config_override: None, - filter_config: entries, + return Ok(FilterPolicySnapshot { + role, + checked_paths, + neutralizer: None, }); } - let guard = executable_filter_guard(git, cwd, entries, &executable_drivers)?; + let neutralizer = config.build_filter_override(&executable_drivers)?; + #[cfg(test)] + FILTER_POLICY_OVERLAY_COUNT.with(|count| count.set(count.get() + 1)); let paths = paths .iter() .map(|path| path.as_bytes().to_vec()) .collect::>(); - let attributes = read_filter_attributes( - git, - cwd, - &paths, - git_config_args, - &executable_drivers, - &guard, - )?; + let attributes = read_filter_attributes(config, &paths, &executable_drivers, &neutralizer)?; let mut required_cache = BTreeMap::new(); for (path, driver) in &attributes { if !executable_drivers.contains(driver) { @@ -263,14 +178,9 @@ fn ensure_no_selected_executable_git_filters_for( } let refused = match execution { FilterExecution::AnyWorktreeOperation => true, - FilterExecution::GitAdd => git_add_filter_is_refused( - git, - cwd, - &guard.filter_config, - driver, - git_config_args, - &mut required_cache, - )?, + FilterExecution::GitAdd => { + git_add_filter_is_refused(config, &entries, driver, &mut required_cache)? + } }; if refused { return Err(io::Error::new( @@ -282,15 +192,17 @@ fn ensure_no_selected_executable_git_filters_for( )); } } - Ok(guard) + Ok(FilterPolicySnapshot { + role, + checked_paths, + neutralizer: Some(neutralizer), + }) } fn git_add_filter_is_refused( - git: &GitRunner, - cwd: &Path, + config: &GuardedGitConfig<'_>, entries: &BTreeMap, driver: &str, - git_config_args: &[String], required_cache: &mut BTreeMap, ) -> io::Result { let required = required_cache.get(driver).copied(); @@ -299,7 +211,17 @@ fn git_add_filter_is_refused( SelectedFilterPolicy::Allowed => return Ok(false), SelectedFilterPolicy::NeedsRequiredValue => {} } - let required = git_filter_required(git, cwd, driver, git_config_args)?; + let required = config + .read_bool(&format!("filter.{driver}.required")) + .map_err(|error| { + io::Error::new( + io::ErrorKind::Unsupported, + format!( + "refusing selected Git filter {driver:?} with malformed required value: {error}" + ), + ) + })? + .unwrap_or(false); required_cache.insert(driver.to_string(), required); Ok(matches!( classify_selected_filter(entries, driver, Some(required)), @@ -307,73 +229,8 @@ fn git_add_filter_is_refused( )) } -fn executable_filter_guard( - git: &GitRunner, - cwd: &Path, - filter_config: BTreeMap, - executable_drivers: &BTreeSet, -) -> io::Result { - let config_override = GitConfigOverrideFile::new("filter-neutralization.gitconfig")?; - let mut guard = GitFilterNeutralization { - config_override: None, - filter_config, - }; - for driver in executable_drivers { - debug_assert_executable_filter_driver(&guard.filter_config, driver); - let description = format!("Git filter neutralization for {driver:?}"); - for (key, value) in filter_neutralization_entries(driver) { - config_override.add_value(git, cwd, &key, value, &description)?; - } - } - guard.config_override = Some(config_override); - Ok(guard) -} - -pub(crate) async fn executable_filter_guard_async( - git: &GitRunner, - cwd: &Path, - filter_config: BTreeMap, - executable_drivers: &BTreeSet, -) -> io::Result { - validate_executable_driver_count(executable_drivers)?; - if executable_drivers.is_empty() { - return Ok(GitFilterNeutralization { - config_override: None, - filter_config, - }); - } - let config_override = GitConfigOverrideFile::new("filter-neutralization.gitconfig")?; - for driver in executable_drivers { - debug_assert_executable_filter_driver(&filter_config, driver); - let description = format!("Git filter neutralization for {driver:?}"); - for (key, value) in filter_neutralization_entries(driver) { - config_override - .add_value_async(git, cwd, &key, value, &description) - .await?; - } - } - Ok(GitFilterNeutralization { - config_override: Some(config_override), - filter_config, - }) -} - -fn validate_executable_driver_count(executable_drivers: &BTreeSet) -> io::Result<()> { - if executable_drivers.len() > MAX_EXECUTABLE_FILTER_DRIVERS { - return Err(io::Error::new( - io::ErrorKind::InvalidData, - format!( - "Git filter driver count {} exceeds the status guard limit {}", - executable_drivers.len(), - MAX_EXECUTABLE_FILTER_DRIVERS - ), - )); - } - Ok(()) -} - #[derive(Clone, Copy)] -enum FilterExecution { +pub(crate) enum FilterExecution { AnyWorktreeOperation, GitAdd, } @@ -386,53 +243,18 @@ enum FilterCommand { } fn read_filter_config( - git: &GitRunner, - cwd: &Path, - git_config_args: &[String], + config: &GuardedGitConfig<'_>, ) -> io::Result> { - read_effective_config_with_fallback( - git, - cwd, - git_config_args, - EXECUTABLE_FILTER_CONFIG_PATTERN, - "filter", - ) -} - -pub(crate) async fn read_filter_config_async( - git: &GitRunner, - cwd: &Path, - git_config_args: &[String], -) -> io::Result> { - ensure_no_worktree_config_sources_async(git, cwd, git_config_args).await?; - read_effective_config_unchecked_async( - git, - cwd, - git_config_args, - EXECUTABLE_FILTER_CONFIG_PATTERN, - "filter", - ) - .await -} - -pub(crate) fn read_effective_config_with_fallback( - git: &GitRunner, - cwd: &Path, - git_config_args: &[String], - pattern: &str, - probe: &str, -) -> io::Result> { - ensure_no_worktree_config_sources(git, cwd, git_config_args)?; - read_effective_config_unchecked(git, cwd, git_config_args, pattern, probe) + #[cfg(test)] + FILTER_POLICY_READ_COUNT.with(|count| count.set(count.get() + 1)); + config.read_effective(EXECUTABLE_FILTER_CONFIG_PATTERN, "filter") } fn read_filter_attributes( - git: &GitRunner, - cwd: &Path, + config: &GuardedGitConfig<'_>, paths: &[Vec], - git_config_args: &[String], - executable_drivers: &BTreeSet, - neutralization: &GitFilterNeutralization, + executable_drivers: &ExecutableFilterDrivers, + neutralization: &SealedFilterConfigOverride, ) -> io::Result, String>> { if paths.is_empty() { return Ok(BTreeMap::new()); @@ -441,22 +263,12 @@ fn read_filter_attributes( write_nul_paths(&mut input, paths)?; input.rewind()?; - let mut command = git.command_for_cwd(cwd)?; + let mut command = config.pending_filter_attribute_command(neutralization)?; command - .env("GIT_OPTIONAL_LOCKS", "0") - .args(git_config_args) - .args([ - "-c", - &format!("core.hooksPath={DISABLED_HOOKS_PATH}"), - "-c", - "core.fsmonitor=false", - "check-attr", - "--stdin", - "-z", - "filter", - ]) + .disable_optional_locks() + .args(["--stdin", "-z", "filter"]) .stdin(Stdio::from(input)); - let output = git.output(command)?; + let output = command.output()?; if !output.status.success() { return Err(io::Error::other(format!( "git filter attribute probe failed with status {}: {}", @@ -465,23 +277,14 @@ fn read_filter_attributes( ))); } let attributes = parse_filter_attributes(&output.stdout, paths)?; - resolve_filter_attribute_sentinels( - git, - cwd, - attributes, - git_config_args, - executable_drivers, - neutralization, - ) + resolve_filter_attribute_sentinels(config, attributes, executable_drivers, neutralization) } fn resolve_filter_attribute_sentinels( - git: &GitRunner, - cwd: &Path, + config: &GuardedGitConfig<'_>, attributes: BTreeMap, FilterAttributeValue>, - git_config_args: &[String], - executable_drivers: &BTreeSet, - neutralization: &GitFilterNeutralization, + executable_drivers: &ExecutableFilterDrivers, + neutralization: &SealedFilterConfigOverride, ) -> io::Result, String>> { let mut resolved = BTreeMap::new(); let mut probe_budget = SentinelFilterProbeBudget::default(); @@ -493,11 +296,9 @@ fn resolve_filter_attribute_sentinels( FilterAttributeValue::AmbiguousSentinel(driver) => { if executable_drivers.contains(&driver) && sentinel_spelling_selects_filter_driver( - git, - cwd, + config, &path, &driver, - git_config_args, neutralization, &mut probe_budget, )? @@ -513,20 +314,16 @@ fn resolve_filter_attribute_sentinels( /// Disambiguate Git's sentinel spellings with required/optional probes. The /// shared guard blanks every known executable driver before either probe. fn sentinel_spelling_selects_filter_driver( - git: &GitRunner, - cwd: &Path, + config: &GuardedGitConfig<'_>, path: &[u8], driver: &str, - git_config_args: &[String], - neutralization: &GitFilterNeutralization, + neutralization: &SealedFilterConfigOverride, probe_budget: &mut SentinelFilterProbeBudget, ) -> io::Result { let probe = SentinelSelectionProbe { - git, - cwd, + config, path, driver, - git_config_args, neutralization, }; let required = probe.run(/*required*/ true, probe_budget)?; @@ -552,12 +349,10 @@ fn sentinel_spelling_selects_filter_driver( } struct SentinelSelectionProbe<'a> { - git: &'a GitRunner, - cwd: &'a Path, + config: &'a GuardedGitConfig<'a>, path: &'a [u8], driver: &'a str, - git_config_args: &'a [String], - neutralization: &'a GitFilterNeutralization, + neutralization: &'a SealedFilterConfigOverride, } impl SentinelSelectionProbe<'_> { @@ -566,29 +361,26 @@ impl SentinelSelectionProbe<'_> { required: bool, probe_budget: &mut SentinelFilterProbeBudget, ) -> io::Result { - let probe_config_args = sentinel_filter_probe_config_args( - self.neutralization.git_config_args(), + if !matches!(self.driver, "set" | "unset" | "unspecified") { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "Git filter sentinel probe requested for a non-sentinel driver", + )); + } + let path = git_path_argument(self.path)?; + let mut command = self.config.command_for_sentinel_filter_probe( + self.neutralization, self.driver, required, )?; - let path = git_path_argument(self.path)?; - let mut command = self.git.command_for_cwd(self.cwd)?; command - .env("GIT_OPTIONAL_LOCKS", "0") - .args(self.git_config_args) - .args([ - "-c", - &format!("core.hooksPath={DISABLED_HOOKS_PATH}"), - "-c", - "core.fsmonitor=false", - ]) - .args(&probe_config_args) - .args(["hash-object", "--stdin"]) + .disable_optional_locks() + .arg("--stdin") .arg("--path") .arg(path) .stdin(Stdio::null()); probe_budget.ensure_probe_available()?; - let output = self.git.output(command)?; + let output = command.output()?; probe_budget.record_completed_probe(); Ok(output) } @@ -619,7 +411,29 @@ fn selected_executable_filter_for( } #[cfg(test)] -pub(crate) fn selected_filter( +fn executable_filter_drivers_for( + entries: &BTreeMap, + execution: FilterExecution, +) -> io::Result> { + let mut drivers = BTreeSet::new(); + for entry in entries.values() { + if entry.key.ends_with(".required") { + continue; + } + let (driver, command) = filter_driver_and_command(&entry.key)?; + let relevant = match execution { + FilterExecution::AnyWorktreeOperation => true, + FilterExecution::GitAdd => command != FilterCommand::Smudge, + }; + if relevant && !entry.value.is_empty() { + drivers.insert(driver); + } + } + Ok(drivers) +} + +#[cfg(test)] +fn selected_filter( drivers: &BTreeSet, attributes: &BTreeMap, String>, ) -> Option<(String, Vec)> { @@ -641,29 +455,18 @@ fn selected_executable_filter( pub(crate) fn executable_filter_drivers( entries: &BTreeMap, -) -> io::Result> { - executable_filter_drivers_for(entries, FilterExecution::AnyWorktreeOperation) -} - -fn executable_filter_drivers_for( - entries: &BTreeMap, - execution: FilterExecution, -) -> io::Result> { +) -> io::Result { let mut executable_drivers = BTreeSet::new(); for entry in entries.values() { if entry.key.ends_with(".required") { continue; } - let (driver, command) = filter_driver_and_command(&entry.key)?; - let relevant = match execution { - FilterExecution::AnyWorktreeOperation => true, - FilterExecution::GitAdd => command != FilterCommand::Smudge, - }; - if relevant && !entry.value.is_empty() { + let (driver, _command) = filter_driver_and_command(&entry.key)?; + if !entry.value.is_empty() { executable_drivers.insert(driver); } } - Ok(executable_drivers) + Ok(ExecutableFilterDrivers(executable_drivers)) } fn effective_filter_value<'a>( @@ -676,21 +479,7 @@ fn effective_filter_value<'a>( .map(|entry| entry.value.as_str()) } -fn filter_neutralization_entries( - driver: &str, -) -> impl Iterator + '_ { - FILTER_NEUTRALIZATION_PLAN - .into_iter() - .map(move |(name, value)| (format!("filter.{driver}.{name}"), value)) -} - -fn debug_assert_executable_filter_driver(entries: &BTreeMap, driver: &str) { - debug_assert!(["clean", "smudge", "process"].into_iter().any(|name| { - effective_filter_value(entries, driver, name).is_some_and(|value| !value.is_empty()) - })); -} - -fn classify_selected_filter( +pub(crate) fn classify_selected_filter( entries: &BTreeMap, driver: &str, required: Option, @@ -706,51 +495,6 @@ fn classify_selected_filter( Some(false) => SelectedFilterPolicy::Allowed, } } - -fn git_filter_required( - git: &GitRunner, - cwd: &Path, - driver: &str, - git_config_args: &[String], -) -> io::Result { - let mut command = git.command_for_cwd(cwd)?; - command - .env("GIT_OPTIONAL_LOCKS", "0") - .args(git_config_args) - .args(["config", "--type=bool", "--get"]) - .arg(format!("filter.{driver}.required")); - let output = git.output(command)?; - parse_git_filter_required_output(&output, driver) -} - -pub(crate) fn parse_git_filter_required_output( - output: &std::process::Output, - driver: &str, -) -> io::Result { - if output.status.code() == Some(1) && output.stdout.is_empty() && output.stderr.is_empty() { - return Ok(false); - } - if !output.status.success() { - return Err(io::Error::new( - io::ErrorKind::Unsupported, - format!( - "refusing selected Git filter {driver:?} with malformed required value: {}", - String::from_utf8_lossy(&output.stderr).trim() - ), - )); - } - match String::from_utf8_lossy(&output.stdout).trim() { - "true" => Ok(true), - "false" => Ok(false), - normalized => Err(io::Error::new( - io::ErrorKind::Unsupported, - format!( - "refusing selected Git filter {driver:?} with unexpected normalized required value {normalized:?}" - ), - )), - } -} - #[cfg(test)] fn filter_driver_name(key: &str) -> io::Result { filter_driver_and_command(key).map(|(driver, _command)| driver) @@ -859,6 +603,28 @@ fn invalid_filter_output(message: &str) -> io::Error { io::Error::new(io::ErrorKind::InvalidData, message) } +#[cfg(test)] +thread_local! { + static FILTER_POLICY_READ_COUNT: std::cell::Cell = const { std::cell::Cell::new(0) }; + static FILTER_POLICY_OVERLAY_COUNT: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +#[cfg(test)] +pub(crate) fn reset_filter_policy_counts() { + FILTER_POLICY_READ_COUNT.with(|count| count.set(0)); + FILTER_POLICY_OVERLAY_COUNT.with(|count| count.set(0)); +} + +#[cfg(test)] +pub(crate) fn filter_policy_read_count() -> usize { + FILTER_POLICY_READ_COUNT.with(std::cell::Cell::get) +} + +#[cfg(test)] +pub(crate) fn filter_policy_overlay_count() -> usize { + FILTER_POLICY_OVERLAY_COUNT.with(std::cell::Cell::get) +} + #[cfg(test)] #[path = "safe_git_tests.rs"] mod tests; diff --git a/codex-rs/git-utils/src/safe_git_tests.rs b/codex-rs/git-utils/src/safe_git_tests.rs index 73916e9f2e..653d315b4e 100644 --- a/codex-rs/git-utils/src/safe_git_tests.rs +++ b/codex-rs/git-utils/src/safe_git_tests.rs @@ -3,15 +3,13 @@ use crate::GitReadError; use crate::apply::ApplyGitRequest; use crate::apply::apply_git_patch; use crate::get_has_changes; +use crate::git_command::GitRunner; use crate::git_config::GitConfigScope; +use crate::guarded_config::GuardedGitConfig; #[cfg(unix)] use crate::patch_paths::stage_paths; #[cfg(unix)] -use crate::status_guard::git_root_from_stdout; -#[cfg(unix)] -use crate::status_guard::prepare_status_filter_guard; -#[cfg(unix)] -use crate::status_guard::resolve_git_root_async; +use crate::status_guard::prepare_status_config; use crate::try_get_has_changes; use pretty_assertions::assert_eq; use std::collections::BTreeMap; @@ -33,11 +31,11 @@ fn status_filter_driver_limit_is_exact() { let at_limit = (0..MAX_EXECUTABLE_FILTER_DRIVERS) .map(|index| format!("driver-{index}")) .collect::>(); - assert!(validate_executable_driver_count(&at_limit).is_ok()); + assert!(validate_executable_driver_count(&ExecutableFilterDrivers(at_limit.clone())).is_ok()); let mut over_limit = at_limit; over_limit.insert("one-too-many".to_string()); - let error = validate_executable_driver_count(&over_limit) + let error = validate_executable_driver_count(&ExecutableFilterDrivers(over_limit)) .expect_err("driver after exact limit must be refused"); assert_eq!(error.kind(), std::io::ErrorKind::InvalidData); } @@ -187,40 +185,6 @@ fn selected_filter_checkin_policy_table_is_canonical() { } } -#[tokio::test] -async fn sync_and_async_filter_guards_share_the_neutralization_plan() { - let repo = init_filter_repo(); - let root = repo.path(); - let git = GitRunner::for_cwd_io(root).expect("Git runner"); - let entries = filter_entries( - GitConfigScope::Local, - Path::new(".git/config"), - "filter.demo.smudge", - "helper-command", - ); - let drivers = BTreeSet::from(["demo".to_string()]); - - let sync_guard = executable_filter_guard(&git, root, entries.clone(), &drivers) - .expect("sync neutralization"); - let async_guard = executable_filter_guard_async(&git, root, entries, &drivers) - .await - .expect("async neutralization"); - let read_plan = |guard: &GitFilterNeutralization| { - read_filter_config(&git, root, guard.git_config_args()) - .expect("read effective neutralization") - .into_iter() - .filter(|(key, _entry)| key.starts_with("filter.demo.")) - .map(|(key, entry)| (key, entry.value)) - .collect::>() - }; - let expected = filter_neutralization_entries("demo") - .map(|(key, value)| (key, value.to_string())) - .collect::>(); - - assert_eq!(read_plan(&sync_guard), expected); - assert_eq!(read_plan(&async_guard), expected); -} - #[test] fn filter_snapshot_retains_required_without_treating_it_as_executable() { let mut entries = filter_entries( @@ -236,7 +200,9 @@ fn filter_snapshot_retains_required_without_treating_it_as_executable() { "true", )); assert_eq!( - executable_filter_drivers(&entries).expect("executable drivers"), + executable_filter_drivers(&entries) + .expect("executable drivers") + .0, BTreeSet::from(["demo".to_string()]) ); let selected = BTreeMap::from([(b"file.txt".to_vec(), "demo".to_string())]); @@ -245,13 +211,11 @@ fn filter_snapshot_retains_required_without_treating_it_as_executable() { .expect("Git add filter policy"), None ); - let neutralization = GitFilterNeutralization { - config_override: None, - filter_config: entries, - }; assert_eq!( - neutralization.filter_value("demo", "required"), - Some("true") + entries + .get("filter.demo.required") + .map(|entry| entry.value.as_str()), + Some("true"), ); let required_only = filter_entries( @@ -381,9 +345,12 @@ fn sentinel_special_states_and_literal_driver_names_remain_distinct() { let root = repo.path(); configure_marker_filter(root, driver); let git = GitRunner::for_cwd_io(root).expect("trusted Git"); + let mut config = + GuardedGitConfig::authorize(&git, root, Vec::new()).expect("authorized config sources"); std::fs::write(root.join(".gitattributes"), special_rule).expect("write special attribute"); - ensure_no_selected_executable_git_filters(&git, root, &["file.txt".to_string()], &[]) + config + .authorize_filter_paths(&["file.txt".to_string()]) .expect("allow special attribute state"); assert!(!marker_filter_ran(root), "special {driver}"); @@ -392,8 +359,9 @@ fn sentinel_special_states_and_literal_driver_names_remain_distinct() { format!("file.txt filter={driver}\n"), ) .expect("write literal attribute"); - let result = - ensure_no_selected_executable_git_filters(&git, root, &["file.txt".to_string()], &[]); + let mut literal_config = GuardedGitConfig::authorize(&git, root, Vec::new()) + .expect("authorize literal-attribute operation"); + let result = literal_config.authorize_filter_paths(&["file.txt".to_string()]); let error = match result { Ok(_) => panic!("accepted literal sentinel-named driver {driver}"), Err(error) => error, @@ -414,9 +382,12 @@ fn sentinel_probe_neutralizes_every_known_driver_after_attribute_swap() { .expect("write initial attribute"); let git = GitRunner::for_cwd_io(root).expect("trusted Git"); - let entries = read_filter_config(&git, root, &[]).expect("filter config"); + let config = + GuardedGitConfig::authorize(&git, root, Vec::new()).expect("authorized config sources"); + let entries = read_filter_config(&config).expect("filter config"); let executable_drivers = executable_filter_drivers(&entries).expect("executable drivers"); - let neutralization = executable_filter_guard(&git, root, entries, &executable_drivers) + let neutralization = config + .build_filter_override(&executable_drivers) .expect("filter neutralization"); let output = run_git_success(root, &["check-attr", "-z", "filter", "--", "file.txt"]); let attributes = parse_filter_attributes(&output.stdout, &[b"file.txt".to_vec()]) @@ -428,10 +399,8 @@ fn sentinel_probe_neutralizes_every_known_driver_after_attribute_swap() { ) .expect("swap attribute after snapshot"); let resolved = resolve_filter_attribute_sentinels( - &git, - root, + &config, attributes, - &[], &executable_drivers, &neutralization, ) @@ -447,9 +416,12 @@ fn high_cardinality_ordinary_sentinels_stop_at_hard_child_probe_budget() { let root = repo.path(); configure_marker_filter(root, "unspecified"); let git = GitRunner::for_cwd_io(root).expect("trusted Git"); - let entries = read_filter_config(&git, root, &[]).expect("filter config"); + let config = + GuardedGitConfig::authorize(&git, root, Vec::new()).expect("authorized config sources"); + let entries = read_filter_config(&config).expect("filter config"); let executable_drivers = executable_filter_drivers(&entries).expect("executable drivers"); - let neutralization = executable_filter_guard(&git, root, entries, &executable_drivers) + let neutralization = config + .build_filter_override(&executable_drivers) .expect("filter neutralization"); let attributes = (0..=SentinelFilterProbeBudget::max_probes()) .map(|index| { @@ -461,10 +433,8 @@ fn high_cardinality_ordinary_sentinels_stop_at_hard_child_probe_budget() { .collect(); let error = resolve_filter_attribute_sentinels( - &git, - root, + &config, attributes, - &[], &executable_drivers, &neutralization, ) @@ -477,36 +447,31 @@ fn high_cardinality_ordinary_sentinels_stop_at_hard_child_probe_budget() { } #[test] -fn unrelated_sentinel_probe_failures_remain_generic_and_fail_closed() { +fn sentinel_probe_rejects_non_sentinel_driver_before_launch() { let repo = init_filter_repo(); let root = repo.path(); configure_marker_filter(root, "set"); std::fs::write(root.join(".gitattributes"), "file.txt filter\n") .expect("write special attribute"); let git = GitRunner::for_cwd_io(root).expect("trusted Git"); - let entries = read_filter_config(&git, root, &[]).expect("filter config"); + let config = + GuardedGitConfig::authorize(&git, root, Vec::new()).expect("authorized config sources"); + let entries = read_filter_config(&config).expect("filter config"); let executable_drivers = executable_filter_drivers(&entries).expect("executable drivers"); - let neutralization = executable_filter_guard(&git, root, entries, &executable_drivers) + let neutralization = config + .build_filter_override(&executable_drivers) .expect("filter neutralization"); let mut budget = SentinelFilterProbeBudget::default(); - let malformed_config = ["-c".to_string(), "=".to_string()]; let error = sentinel_spelling_selects_filter_driver( - &git, - root, + &config, b"file.txt", - "set", - &malformed_config, + "ordinary", &neutralization, &mut budget, ) - .expect_err("malformed config must fail both probes"); - assert_eq!(error.kind(), io::ErrorKind::Other); - assert!( - error - .to_string() - .starts_with("git filter attribute selection probe failed with required status") - ); + .expect_err("reject non-sentinel probe"); + assert_eq!(error.kind(), io::ErrorKind::InvalidInput); assert!(!marker_filter_ran(root)); } @@ -781,15 +746,6 @@ async fn checked_has_changes_accepts_non_utf8_repository_root() { assert_eq!(try_get_has_changes(&repo_path).await, Ok(false)); } -#[cfg(unix)] -#[test] -fn git_root_output_preserves_non_utf8_bytes() { - use std::os::unix::ffi::OsStrExt; - - let path = git_root_from_stdout(b"/tmp/repo-\xff\n".to_vec()).expect("parse raw Git path"); - assert_eq!(path.as_os_str().as_bytes(), b"/tmp/repo-\xff"); -} - #[cfg(unix)] #[tokio::test] async fn root_probe_distinguishes_repository_command_failure() { @@ -798,7 +754,25 @@ async fn root_probe_distinguishes_repository_command_failure() { let temp_dir = tempfile::tempdir().expect("create temp dir"); let repo_path = create_test_git_repo(&temp_dir).await; let failing_git = temp_dir.path().join("failing-git"); - std::fs::write(&failing_git, "#!/bin/sh\nexit 42\n").expect("write failing Git"); + let resolved_git = std::process::Command::new("/bin/sh") + .args(["-c", "command -v git"]) + .output() + .expect("resolve real Git"); + assert!(resolved_git.status.success(), "resolve real Git"); + let real_git = format!( + "'{}'", + String::from_utf8(resolved_git.stdout) + .expect("Git path UTF-8") + .trim() + .replace('\'', "'\\''") + ); + std::fs::write( + &failing_git, + format!( + "#!/bin/sh\ncase \" $* \" in *\" rev-parse --show-toplevel \"*) exit 42 ;; esac\nexec {real_git} \"$@\"\n" + ), + ) + .expect("write failing Git"); let mut permissions = std::fs::metadata(&failing_git) .expect("read failing Git metadata") .permissions(); @@ -808,7 +782,7 @@ async fn root_probe_distinguishes_repository_command_failure() { let git = GitRunner::from_executable_for_test(&repo_path, failing_git) .expect("authority-bound failing Git"); assert_eq!( - resolve_git_root_async(&git, &repo_path).await, + prepare_status_config(&git, &repo_path).await.map(|_| ()), Err(GitReadError::CommandFailed { operation: "resolveGitRoot".to_string(), exit_code: Some(42), @@ -860,9 +834,7 @@ async fn legacy_git_without_show_scope_uses_origin_fallback() { let git = GitRunner::from_executable_for_test(&repo_path, wrapper) .expect("authority-bound legacy Git"); assert_eq!( - prepare_status_filter_guard(&git, &repo_path) - .await - .map(|_| ()), + prepare_status_config(&git, &repo_path).await.map(|_| ()), Err(GitReadError::SelectedExecutableFilter { driver: "legacy".to_string(), path: "test.txt".to_string(), @@ -898,6 +870,13 @@ async fn get_has_changes_rejects_core_worktree_redirection_before_running_filter .await; assert_eq!(get_has_changes(&repo_path).await, None); + assert_eq!( + try_get_has_changes(&repo_path).await, + Err(GitReadError::RepositoryRootMismatch { + expected_root: std::fs::canonicalize(&repo_path).expect("canonical repository"), + reported_root: std::fs::canonicalize(&redirected).expect("canonical redirection"), + }) + ); assert!(!configured_filter_ran(&repo_path).await); } diff --git a/codex-rs/git-utils/src/status_guard.rs b/codex-rs/git-utils/src/status_guard.rs index 1ed57d99f2..96e4cbec7a 100644 --- a/codex-rs/git-utils/src/status_guard.rs +++ b/codex-rs/git-utils/src/status_guard.rs @@ -1,384 +1,105 @@ -use std::collections::BTreeMap; -use std::collections::BTreeSet; use std::io; -use std::io::Seek; use std::path::Path; -use std::path::PathBuf; -use std::process::Stdio; - -#[cfg(all(test, unix))] -use tokio::time::timeout; +use crate::FsmonitorOverride; use crate::GitReadError; -use crate::git_command::GitAsyncCommand; use crate::git_command::GitRunner; -use crate::git_command::MAX_INTERNAL_GIT_OUTPUT_BYTES; +use crate::guarded_config::GuardedGitConfig; +use crate::guarded_config::NoActiveStatusWorktree; +use crate::guarded_config::SelectedStatusFilterRefusal; +use crate::guarded_config::StatusFilterProbeLimitExceeded; +use crate::guarded_config::StatusPolicyCommandFailure; +use crate::guarded_config::StatusRootMismatch; use crate::repository_authority::is_authority_refusal; -use crate::safe_git::DISABLED_HOOKS_PATH; -use crate::safe_git::FilterAttributeValue; -#[cfg(all(test, unix))] -use crate::safe_git::GIT_COMMAND_TIMEOUT; -use crate::safe_git::GitFilterNeutralization; -use crate::safe_git::SelectedFilterPolicy; -use crate::safe_git::SentinelFilterProbeBudget; -use crate::safe_git::SentinelFilterProbeResolution; -use crate::safe_git::classify_sentinel_filter_probes; -use crate::safe_git::executable_filter_drivers; -use crate::safe_git::executable_filter_guard_async; -use crate::safe_git::git_path_argument; -use crate::safe_git::parse_filter_attributes; -use crate::safe_git::parse_git_filter_required_output; -use crate::safe_git::parse_nul_paths; -use crate::safe_git::read_filter_config_async; -use crate::safe_git::sentinel_filter_probe_config_args; -use crate::safe_git::write_nul_paths; -const MAX_STATUS_TRACKED_PATHS: usize = 250_000; - -#[cfg(all(test, unix))] -pub(crate) async fn prepare_status_filter_guard( - git: &GitRunner, - cwd: &Path, -) -> Result<(PathBuf, GitFilterNeutralization), GitReadError> { - timeout( - GIT_COMMAND_TIMEOUT, - Box::pin(prepare_status_filter_guard_within_deadline(git, cwd)), - ) - .await - .map_err(|_| GitReadError::CommandTimedOut { - operation: "statusFilterPreparation".to_string(), - })? -} - -pub(crate) async fn prepare_status_filter_guard_within_deadline( - git: &GitRunner, - cwd: &Path, -) -> Result<(PathBuf, GitFilterNeutralization), GitReadError> { - let requested_cwd = std::fs::canonicalize(cwd).map_err(|_| GitReadError::NotRepository { - path: cwd.to_path_buf(), - })?; - let authorized_root = git - .active_worktree_root() - .ok_or_else(|| GitReadError::NotRepository { - path: requested_cwd.clone(), - })? - .to_path_buf(); - // Source authorization precedes the first config-consuming Git child and - // is performed once for the retained active repository authority. - let entries = read_filter_config_async(git, &authorized_root, &[]) - .await - .map_err(|error| map_io_error("filterConfig", error))?; - let git_root = resolve_git_root_async(git, &requested_cwd).await?; - let executable_drivers = - executable_filter_drivers(&entries).map_err(|_| invalid_output("filterConfig"))?; - let filter_guard = executable_filter_guard_async(git, &git_root, entries, &executable_drivers) - .await - .map_err(|error| map_io_error("filterNeutralization", error))?; - if executable_drivers.is_empty() { - return Ok((git_root, filter_guard)); - } - - let paths = read_tracked_paths_async(git, &git_root).await?; - let attributes = read_filter_attributes_async(git, &git_root, &paths).await?; - let attributes = resolve_filter_attribute_sentinels_async( - git, - &git_root, - attributes, - &executable_drivers, - &filter_guard, - ) - .await?; - let mut required_cache = BTreeMap::new(); - for (path, driver) in attributes { - if !executable_drivers.contains(&driver) { - continue; - } - let mut policy = - filter_guard.selected_filter_policy(&driver, required_cache.get(&driver).copied()); - if policy == SelectedFilterPolicy::NeedsRequiredValue { - let required = git_filter_required_async(git, &git_root, &driver).await?; - required_cache.insert(driver.clone(), required); - policy = filter_guard.selected_filter_policy(&driver, Some(required)); - } - if policy == SelectedFilterPolicy::Refused { - return Err(GitReadError::SelectedExecutableFilter { - driver, - path: String::from_utf8_lossy(&path).into_owned(), - }); - } - } - Ok((git_root, filter_guard)) -} - -pub(crate) async fn resolve_git_root_async( - git: &GitRunner, +/// One retained capability owns source authorization, filter policy, fsmonitor +/// selection, and the final bounded status child. The caller supplies the +/// phase transitions so the one outer deadline can report precise metadata. +pub(crate) async fn prepare_status_config<'git>( + git: &'git GitRunner, requested_cwd: &Path, -) -> Result { - let expected_root = git - .active_worktree_root() - .ok_or_else(|| GitReadError::NotRepository { - path: requested_cwd.to_path_buf(), - })? - .to_path_buf(); - let mut command = git - .async_command_for_cwd(requested_cwd) +) -> Result, GitReadError> { + let mut config = GuardedGitConfig::authorize_status_async(git) + .await + .map_err(|error| { + if error + .get_ref() + .is_some_and(|source| source.is::()) + { + GitReadError::NotRepository { + path: requested_cwd.to_path_buf(), + } + } else { + map_io_error("statusFilterPreparation", error) + } + })?; + config + .verify_status_root_async(requested_cwd) + .await .map_err(|error| map_io_error("resolveGitRoot", error))?; - command.env("GIT_OPTIONAL_LOCKS", "0").args([ - "-c", - &format!("core.hooksPath={DISABLED_HOOKS_PATH}"), - "-c", - "core.fsmonitor=false", - "rev-parse", - "--show-toplevel", - ]); - let output = command_output(git, command, "resolveGitRoot").await?; + config + .install_status_policy_async() + .await + .map_err(|error| map_io_error("statusFilterPreparation", error))?; + Ok(config) +} + +pub(crate) async fn detect_status_fsmonitor( + config: &mut GuardedGitConfig<'_>, +) -> FsmonitorOverride { + config.detect_status_fsmonitor_async().await +} + +pub(crate) async fn read_status(config: &GuardedGitConfig<'_>) -> Result { + let output = config + .status_output_async() + .await + .map_err(|error| map_io_error("status", error))?; if !output.status.success() { - return Err(command_failed("resolveGitRoot", output.status.code())); - } - let reported_root = git_root_from_stdout(output.stdout).and_then(|path| { - std::fs::canonicalize(path).map_err(|_| invalid_output("resolveGitRoot")) - })?; - if reported_root != expected_root { - return Err(GitReadError::RepositoryRootMismatch { - expected_root, - reported_root, + return Err(GitReadError::CommandFailed { + operation: "status".to_string(), + exit_code: output.status.code(), }); } - Ok(reported_root) -} - -pub(crate) fn git_root_from_stdout(output: Vec) -> Result { - let output = output.strip_suffix(b"\n").unwrap_or(&output); - #[cfg(windows)] - let output = output.strip_suffix(b"\r").unwrap_or(output); - if output.is_empty() { - return Err(invalid_output("resolveGitRoot")); - } - #[cfg(unix)] - { - use std::ffi::OsString; - use std::os::unix::ffi::OsStringExt; - - Ok(PathBuf::from(OsString::from_vec(output.to_vec()))) - } - #[cfg(not(unix))] - { - String::from_utf8(output.to_vec()) - .map(PathBuf::from) - .map_err(|_| invalid_output("resolveGitRoot")) - } -} - -async fn read_tracked_paths_async( - git: &GitRunner, - cwd: &Path, -) -> Result>, GitReadError> { - let mut command = git - .async_command_for_cwd(cwd) - .map_err(|error| map_io_error("trackedPaths", error))?; - command.env("GIT_OPTIONAL_LOCKS", "0").args([ - "-c", - &format!("core.hooksPath={DISABLED_HOOKS_PATH}"), - "-c", - "core.fsmonitor=false", - "ls-files", - "-z", - "--cached", - ]); - let output = command_output(git, command, "trackedPaths").await?; - if !output.status.success() { - return Err(command_failed("trackedPaths", output.status.code())); - } - let paths = parse_nul_paths(&output.stdout).map_err(|_| invalid_output("trackedPaths"))?; - if paths.len() > MAX_STATUS_TRACKED_PATHS { - return Err(invalid_output("trackedPaths")); - } - Ok(paths) -} - -async fn read_filter_attributes_async( - git: &GitRunner, - cwd: &Path, - paths: &[Vec], -) -> Result, FilterAttributeValue>, GitReadError> { - if paths.is_empty() { - return Ok(BTreeMap::new()); - } - let mut input = - tempfile::tempfile().map_err(|error| map_io_error("filterAttributes", error))?; - write_nul_paths(&mut input, paths).map_err(|_| invalid_output("filterAttributes"))?; - input - .rewind() - .map_err(|error| map_io_error("filterAttributes", error))?; - - let mut command = git - .async_command_for_cwd(cwd) - .map_err(|error| map_io_error("filterAttributes", error))?; - command - .env("GIT_OPTIONAL_LOCKS", "0") - .args([ - "-c", - &format!("core.hooksPath={DISABLED_HOOKS_PATH}"), - "-c", - "core.fsmonitor=false", - "check-attr", - "--stdin", - "-z", - "filter", - ]) - .stdin(Stdio::from(input)); - let output = command_output(git, command, "filterAttributes").await?; - if !output.status.success() { - return Err(command_failed("filterAttributes", output.status.code())); - } - parse_filter_attributes(&output.stdout, paths).map_err(|_| invalid_output("filterAttributes")) -} - -async fn resolve_filter_attribute_sentinels_async( - git: &GitRunner, - cwd: &Path, - attributes: BTreeMap, FilterAttributeValue>, - executable_drivers: &BTreeSet, - filter_guard: &GitFilterNeutralization, -) -> Result, String>, GitReadError> { - let mut resolved = BTreeMap::new(); - let mut probe_budget = SentinelFilterProbeBudget::default(); - for (path, attribute) in attributes { - match attribute { - FilterAttributeValue::Driver(driver) => { - resolved.insert(path, driver); - } - FilterAttributeValue::AmbiguousSentinel(driver) => { - if executable_drivers.contains(&driver) - && sentinel_spelling_selects_filter_driver_async( - git, - cwd, - &path, - &driver, - filter_guard, - &mut probe_budget, - ) - .await? - { - resolved.insert(path, driver); - } - } - } - } - Ok(resolved) -} - -async fn sentinel_spelling_selects_filter_driver_async( - git: &GitRunner, - cwd: &Path, - path: &[u8], - driver: &str, - filter_guard: &GitFilterNeutralization, - probe_budget: &mut SentinelFilterProbeBudget, -) -> Result { - let required = run_sentinel_selection_probe_async( - git, - cwd, - path, - driver, - /*required*/ true, - filter_guard, - probe_budget, - ) - .await?; - if classify_sentinel_filter_probes(required.status.success(), /*optional_succeeded*/ None) - == SentinelFilterProbeResolution::SpecialAttributeState - { - return Ok(false); - } - let optional = run_sentinel_selection_probe_async( - git, - cwd, - path, - driver, - /*required*/ false, - filter_guard, - probe_budget, - ) - .await?; - if classify_sentinel_filter_probes(required.status.success(), Some(optional.status.success())) - == SentinelFilterProbeResolution::LiteralDriver - { - return Ok(true); - } - Err(command_failed( - "filterAttributeSelection", - optional.status.code(), - )) -} - -async fn run_sentinel_selection_probe_async( - git: &GitRunner, - cwd: &Path, - path: &[u8], - driver: &str, - required: bool, - filter_guard: &GitFilterNeutralization, - probe_budget: &mut SentinelFilterProbeBudget, -) -> Result { - probe_budget.ensure_probe_available().map_err(|_| { - GitReadError::FilterSelectionProbeLimitExceeded { - max_probes: SentinelFilterProbeBudget::max_probes(), - } - })?; - let path = git_path_argument(path).map_err(|_| invalid_output("filterAttributeSelection"))?; - let probe_config_args = - sentinel_filter_probe_config_args(filter_guard.git_config_args(), driver, required) - .map_err(|_| invalid_output("filterAttributeSelection"))?; - let mut command = git - .async_command_for_cwd(cwd) - .map_err(|error| map_io_error("filterAttributeSelection", error))?; - command - .env("GIT_OPTIONAL_LOCKS", "0") - .args([ - "-c", - &format!("core.hooksPath={DISABLED_HOOKS_PATH}"), - "-c", - "core.fsmonitor=false", - ]) - .args(&probe_config_args) - .args(["hash-object", "--stdin"]) - .arg("--path") - .arg(path) - .stdin(Stdio::null()); - let output = command_output(git, command, "filterAttributeSelection").await?; - probe_budget.record_completed_probe(); - Ok(output) -} - -async fn git_filter_required_async( - git: &GitRunner, - cwd: &Path, - driver: &str, -) -> Result { - let mut command = git - .async_command_for_cwd(cwd) - .map_err(|error| map_io_error("filterRequired", error))?; - command - .env("GIT_OPTIONAL_LOCKS", "0") - .args(["config", "--type=bool", "--get"]) - .arg(format!("filter.{driver}.required")); - let output = command_output(git, command, "filterRequired").await?; - parse_git_filter_required_output(&output, driver) - .map_err(|error| map_io_error("filterRequired", error)) -} - -async fn command_output( - git: &GitRunner, - command: GitAsyncCommand, - operation: &str, -) -> Result { - git.output_async_bounded(command, MAX_INTERNAL_GIT_OUTPUT_BYTES) - .await - .map_err(|error| map_io_error(operation, error)) + Ok(!output.stdout.is_empty()) } pub(crate) fn map_io_error(operation: &str, error: io::Error) -> GitReadError { + if let Some(refusal) = error + .get_ref() + .and_then(|source| source.downcast_ref::()) + { + return GitReadError::SelectedExecutableFilter { + driver: refusal.driver().to_string(), + path: String::from_utf8_lossy(refusal.path()).into_owned(), + }; + } + if let Some(mismatch) = error + .get_ref() + .and_then(|source| source.downcast_ref::()) + { + return GitReadError::RepositoryRootMismatch { + expected_root: mismatch.expected().to_path_buf(), + reported_root: mismatch.reported().to_path_buf(), + }; + } + if let Some(limit) = error + .get_ref() + .and_then(|source| source.downcast_ref::()) + { + return GitReadError::FilterSelectionProbeLimitExceeded { + max_probes: limit.max_probes(), + }; + } + if let Some(failure) = error + .get_ref() + .and_then(|source| source.downcast_ref::()) + { + return GitReadError::CommandFailed { + operation: operation.to_string(), + exit_code: failure.exit_code(), + }; + } if is_authority_refusal(&error) { return GitReadError::AuthorityRefused { operation: operation.to_string(), @@ -388,21 +109,13 @@ pub(crate) fn map_io_error(operation: &str, error: io::Error) -> GitReadError { io::ErrorKind::TimedOut => GitReadError::CommandTimedOut { operation: operation.to_string(), }, - io::ErrorKind::InvalidData => invalid_output(operation), - _ => command_failed(operation, /*exit_code*/ None), - } -} - -fn command_failed(operation: &str, exit_code: Option) -> GitReadError { - GitReadError::CommandFailed { - operation: operation.to_string(), - exit_code, - } -} - -fn invalid_output(operation: &str) -> GitReadError { - GitReadError::InvalidOutput { - operation: operation.to_string(), + io::ErrorKind::InvalidData => GitReadError::InvalidOutput { + operation: operation.to_string(), + }, + _ => GitReadError::CommandFailed { + operation: operation.to_string(), + exit_code: None, + }, } } diff --git a/codex-rs/git-utils/src/status_guard_tests.rs b/codex-rs/git-utils/src/status_guard_tests.rs index 9bfd091aa4..06a1ac29b1 100644 --- a/codex-rs/git-utils/src/status_guard_tests.rs +++ b/codex-rs/git-utils/src/status_guard_tests.rs @@ -8,6 +8,7 @@ use pretty_assertions::assert_eq; use tokio::process::Command; use super::*; +use crate::safe_git::SentinelFilterProbeBudget; async fn run_git(cwd: &Path, args: &[&str]) -> std::process::Output { let output = Command::new("git") @@ -124,8 +125,11 @@ async fn dropping_status_preparation_kills_the_active_git_child() { ); let task_git = std::sync::Arc::clone(&git); let task_repo = repo.clone(); - let task = - tokio::spawn(async move { prepare_status_filter_guard(&task_git, &task_repo).await }); + let task = tokio::spawn(async move { + prepare_status_config(&task_git, &task_repo) + .await + .map(|_| ()) + }); tokio::time::timeout(std::time::Duration::from_secs(10), async { while !pid_file.exists() { @@ -193,23 +197,14 @@ async fn prepared_guard_neutralizes_filter_selected_after_the_probe() { .await; let git = GitRunner::for_cwd(&repo).expect("trusted Git"); - let (root, guard) = prepare_status_filter_guard(&git, &repo) + let mut guard = prepare_status_config(&git, &repo) .await .expect("prepare status guard"); std::fs::write(repo.join(".gitattributes"), "test.txt filter=race\n") .expect("select filter after probe"); - let mut command = git - .async_command_for_cwd(&root) - .expect("authorized status cwd"); - command - .args(["-c", &format!("core.hooksPath={DISABLED_HOOKS_PATH}")]) - .args(["-c", "core.fsmonitor=false"]) - .args(guard.git_config_args()) - .args(["status", "--porcelain"]); - let output = git.output_async(command).await.expect("run guarded status"); - - assert!(output.status.success(), "guarded status succeeds"); + detect_status_fsmonitor(&mut guard).await; + assert!(read_status(&guard).await.is_ok(), "guarded status succeeds"); assert!(!filter_marker_is_set(&repo, "codex.race-ran").await); } @@ -232,14 +227,14 @@ async fn optional_smudge_only_filter_is_allowed_but_required_is_refused() { .await; let git = GitRunner::for_cwd(&repo).expect("trusted Git"); - prepare_status_filter_guard(&git, &repo) + prepare_status_config(&git, &repo) .await .expect("allow optional smudge-only filter"); assert!(!filter_marker_is_set(&repo, "codex.smudge-ran").await); run_git(&repo, &["config", "filter.smudge-only.required", "true"]).await; assert_eq!( - prepare_status_filter_guard(&git, &repo).await.map(|_| ()), + prepare_status_config(&git, &repo).await.map(|_| ()), Err(GitReadError::SelectedExecutableFilter { driver: "smudge-only".to_string(), path: "test.txt".to_string(), @@ -281,7 +276,7 @@ async fn optional_smudge_required_value_is_queried_once_per_driver() { ); let git = GitRunner::from_executable_for_test(&repo, wrapper).expect("test Git runner"); - prepare_status_filter_guard(&git, &repo) + prepare_status_config(&git, &repo) .await .expect("allow optional smudge-only filter"); let required_queries = std::fs::read_to_string(log) @@ -324,7 +319,7 @@ async fn sentinel_probe_neutralizes_a_race_selected_known_driver() { ); let git = GitRunner::from_executable_for_test(&repo, wrapper).expect("test Git runner"); - prepare_status_filter_guard(&git, &repo) + prepare_status_config(&git, &repo) .await .expect("probe remains non-executing across attribute race"); assert!(!filter_marker_is_set(&repo, "codex.probe-filter-ran").await); @@ -359,7 +354,7 @@ async fn sentinel_probe_budget_fails_closed_at_the_exact_process_limit() { let result = tokio::time::timeout( std::time::Duration::from_secs(10), - Box::pin(prepare_status_filter_guard_within_deadline(&git, &repo)), + Box::pin(prepare_status_config(&git, &repo)), ) .await .expect("sentinel budget test should complete"); @@ -402,9 +397,11 @@ async fn ordinary_driver_uses_one_bulk_path_and_attribute_probe() { ); let git = GitRunner::from_executable_for_test(&repo, wrapper).expect("test Git runner"); - prepare_status_filter_guard(&git, &repo) + let mut config = prepare_status_config(&git, &repo) .await .expect("prepare ordinary-driver guard"); + detect_status_fsmonitor(&mut config).await; + read_status(&config).await.expect("final guarded status"); let lines = std::fs::read_to_string(log).expect("read wrapper log"); assert_eq!( lines @@ -427,5 +424,152 @@ async fn ordinary_driver_uses_one_bulk_path_and_attribute_probe() { .count(), 0 ); + let status = lines + .lines() + .find(|line| line.contains(" status --porcelain --ignore-submodules=dirty")) + .expect("final status command"); + let hooks = status + .find("core.hooksPath=") + .expect("hooks override before status"); + let fsmonitor = status + .find("core.fsmonitor=") + .expect("fsmonitor override before status"); + let filter = status + .find("include.path=") + .expect("sealed filter include before status"); + let status_command = status + .find(" status --porcelain") + .expect("status subcommand"); + assert!(hooks < fsmonitor && fsmonitor < filter && filter < status_command); + assert_eq!(status.matches("include.path=").count(), 1); assert!(!filter_marker_is_set(&repo, "codex.ordinary-filter-ran").await); } + +#[tokio::test] +async fn fsmonitor_probe_is_base_only_and_the_sealed_decision_controls_final_order() { + let temp_dir = tempfile::tempdir().expect("create temp dir"); + let repo = init_repo(&temp_dir, /*file_count*/ 0).await; + run_git(&repo, &["config", "core.fsmonitor", "true"]).await; + + let wrapper = temp_dir.path().join("git-wrapper"); + let log = temp_dir.path().join("git-wrapper.log"); + let real_git = shell_quote(&real_git()); + write_wrapper( + &wrapper, + &format!( + "#!/bin/sh\nprintf '%s\\n' \"$*\" >>{}\ncase \" $* \" in *\" version --build-options \"*) printf 'feature: fsmonitor--daemon\\n'; exit 0 ;; esac\nexec {real_git} \"$@\"\n", + shell_quote(&log) + ), + ); + let git = GitRunner::from_executable_for_test(&repo, wrapper).expect("test Git runner"); + let mut config = prepare_status_config(&git, &repo) + .await + .expect("prepare status capability"); + std::fs::write(&log, "").expect("clear preparation log"); + + assert_eq!( + detect_status_fsmonitor(&mut config).await, + FsmonitorOverride::BuiltIn + ); + assert_eq!( + detect_status_fsmonitor(&mut config).await, + FsmonitorOverride::BuiltIn, + "the retained decision must avoid a second probe" + ); + read_status(&config).await.expect("final status"); + + let lines = std::fs::read_to_string(&log) + .expect("read wrapper log") + .lines() + .map(str::to_string) + .collect::>(); + assert_eq!( + lines.len(), + 3, + "one config probe, one feature probe, one status" + ); + assert!(lines[0].ends_with("config --null --get core.fsmonitor")); + assert!(!lines[0].contains("core.fsmonitor=false")); + assert!(lines[1].ends_with("version --build-options")); + assert!(!lines[1].contains("core.fsmonitor=false")); + let hooks = format!("core.hooksPath={}", crate::safe_git::DISABLED_HOOKS_PATH); + let hooks_offset = lines[2].find(&hooks).expect("hooks override"); + let fsmonitor_offset = lines[2] + .find("core.fsmonitor=true") + .expect("typed fsmonitor override"); + let status_offset = lines[2].find("status --porcelain").expect("status command"); + assert!(hooks_offset < fsmonitor_offset && fsmonitor_offset < status_offset); + assert!(lines[2].ends_with("status --porcelain --ignore-submodules=dirty")); +} + +#[tokio::test] +async fn zero_driver_status_skips_tracked_paths_attributes_and_overlay_writes() { + let temp_dir = tempfile::tempdir().expect("create temp dir"); + let repo = init_repo(&temp_dir, /*file_count*/ 0).await; + let wrapper = temp_dir.path().join("git-wrapper"); + let log = temp_dir.path().join("git-wrapper.log"); + let real_git = shell_quote(&real_git()); + write_wrapper( + &wrapper, + &format!( + "#!/bin/sh\nprintf '%s\\n' \"$*\" >>{}\nexec {real_git} \"$@\"\n", + shell_quote(&log) + ), + ); + let git = GitRunner::from_executable_for_test(&repo, wrapper).expect("test Git runner"); + + prepare_status_config(&git, &repo) + .await + .expect("zero-driver status capability"); + let lines = std::fs::read_to_string(log).expect("read wrapper log"); + assert!(!lines.contains(" ls-files -z --cached"), "{lines}"); + assert!(!lines.contains(" check-attr --stdin -z filter"), "{lines}"); + assert!(!lines.contains(" config --file "), "{lines}"); +} + +#[tokio::test] +async fn status_driver_limit_fails_before_paths_or_overlay_writes() { + use std::io::Write; + + let temp_dir = tempfile::tempdir().expect("create temp dir"); + let repo = init_repo(&temp_dir, /*file_count*/ 0).await; + let mut local_config = std::fs::OpenOptions::new() + .append(true) + .open(repo.join(".git/config")) + .expect("open local config"); + for index in 0..=crate::safe_git::MAX_EXECUTABLE_FILTER_DRIVERS { + writeln!(local_config, "[filter \"driver-{index}\"]\n\tclean = cat") + .expect("append filter driver"); + } + drop(local_config); + + let wrapper = temp_dir.path().join("git-wrapper"); + let log = temp_dir.path().join("git-wrapper.log"); + let real_git = shell_quote(&real_git()); + write_wrapper( + &wrapper, + &format!( + "#!/bin/sh\nprintf '%s\\n' \"$*\" >>{}\nexec {real_git} \"$@\"\n", + shell_quote(&log) + ), + ); + let git = GitRunner::from_executable_for_test(&repo, wrapper).expect("test Git runner"); + let mut config = GuardedGitConfig::authorize_status_async(&git) + .await + .expect("authorized status capability"); + config + .verify_status_root_async(&repo) + .await + .expect("matching root"); + std::fs::write(&log, "").expect("clear authorization log"); + + let error = config + .install_status_policy_async() + .await + .expect_err("257th driver must fail closed"); + assert_eq!(error.kind(), io::ErrorKind::InvalidData); + let lines = std::fs::read_to_string(log).expect("read wrapper log"); + assert!(!lines.contains(" ls-files -z --cached"), "{lines}"); + assert!(!lines.contains(" check-attr --stdin -z filter"), "{lines}"); + assert!(!lines.contains(" config --file "), "{lines}"); +}