diff --git a/codex-rs/git-utils/src/apply.rs b/codex-rs/git-utils/src/apply.rs index 4dd850b435..d6118c27c0 100644 --- a/codex-rs/git-utils/src/apply.rs +++ b/codex-rs/git-utils/src/apply.rs @@ -55,11 +55,14 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { // Keep tmpdir alive until function end to ensure the file exists let _guard = tmpdir; let patch_paths = extract_effective_paths_from_patch(&git, &patch_path, req.revert)?; - ensure_no_selected_executable_git_filters(&git, &git_root, &patch_paths, &cfg_parts)?; + 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()); if req.revert && !req.preflight { // Stage WT paths first to avoid index mismatch on revert. - stage_effective_paths(&git, &git_root, &patch_paths)?; + stage_effective_paths(&git, &git_root, &patch_paths, &cfg_parts)?; } // Build git args @@ -68,8 +71,6 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { args.push("-R".into()); } - cfg_parts.extend(safe_git_config_parts()); - args.push(patch_path.to_string_lossy().to_string()); // Optional preflight: dry-run only; do not modify working tree @@ -255,6 +256,10 @@ fn render_command_for_log(cwd: &Path, git_cfg: &[String], args: &[String]) -> St #[path = "apply_transport_tests.rs"] mod transport_tests; +#[cfg(test)] +#[path = "apply_filter_tests.rs"] +mod filter_tests; + #[cfg(test)] mod tests { use super::*; @@ -371,6 +376,13 @@ mod tests { code == 0 } + #[cfg(unix)] + fn trusted_git_directory() -> PathBuf { + std::env::split_paths(&std::env::var_os("PATH").expect("PATH")) + .find(|directory| directory.is_absolute() && directory.join("git").is_file()) + .expect("trusted Git directory") + } + #[test] fn parse_output_unescapes_quoted_paths() { let stderr = "error: patch failed: \"hello\\tworld.txt\":1\n"; @@ -453,6 +465,77 @@ mod tests { assert_eq!(read_file_normalized(&root.join("file.txt")), "new\n"); } + #[cfg(unix)] + #[test] + fn apply_uses_logical_process_cwd_to_reject_enclosing_git() { + use std::os::unix::fs::PermissionsExt; + + let _g = env_lock().lock().unwrap(); + if std::env::var_os("CODEX_GIT_UTILS_APPLY_LOGICAL_CWD_CHILD").is_none() { + let fixture = tempfile::tempdir().expect("fixture"); + let outer = fixture.path().join("outer"); + let physical_nested = fixture.path().join("physical-nested"); + let lexical_nested = outer.join("nested"); + let outer_bin = outer.join("bin"); + let outer_git = outer_bin.join("git"); + let marker = outer_bin.join("git.ran"); + std::fs::create_dir_all(&outer_bin).expect("outer Git directory"); + std::fs::create_dir_all(&physical_nested).expect("physical nested repository"); + let (outer_init, _, outer_err) = run(&outer, &["git", "init", "-q"]); + assert_eq!(outer_init, 0, "init outer repository: {outer_err}"); + let (nested_init, _, nested_err) = run(&physical_nested, &["git", "init", "-q"]); + assert_eq!(nested_init, 0, "init nested repository: {nested_err}"); + std::os::unix::fs::symlink(&physical_nested, &lexical_nested) + .expect("symlink nested repository"); + std::fs::write(&outer_git, "#!/bin/sh\nprintf ran >\"$0.ran\"\nexit 1\n") + .expect("outer Git shim"); + let mut permissions = std::fs::metadata(&outer_git) + .expect("outer Git metadata") + .permissions(); + permissions.set_mode(0o755); + std::fs::set_permissions(&outer_git, permissions).expect("executable outer Git"); + let path = std::env::join_paths([outer_bin, trusted_git_directory()]).expect("PATH"); + + 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::tests::apply_uses_logical_process_cwd_to_reject_enclosing_git") + .arg("--exact") + .arg("--nocapture") + .current_dir(&lexical_nested) + .env("CODEX_GIT_UTILS_APPLY_LOGICAL_CWD_CHILD", "1") + .env("CODEX_GIT_UTILS_APPLY_LOGICAL_CWD_MARKER", &marker) + .env("PWD", &lexical_nested) + .env("PATH", path) + .env("RUST_TEST_THREADS", "1") + .output() + .expect("run isolated logical-cwd test"); + assert!( + output.status.success(), + "isolated logical-cwd test failed:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert!(!marker.exists(), "enclosing Git shim must not run"); + return; + } + + let cwd = std::env::current_dir().expect("physical process cwd"); + let marker = PathBuf::from( + std::env::var_os("CODEX_GIT_UTILS_APPLY_LOGICAL_CWD_MARKER").expect("marker path"), + ); + let result = apply_git_patch(&ApplyGitRequest { + cwd, + diff: "diff --git a/hello.txt b/hello.txt\nnew file mode 100644\n--- /dev/null\n+++ b/hello.txt\n@@ -0,0 +1 @@\n+hello\n".to_string(), + revert: false, + preflight: true, + }) + .expect("preflight through trusted Git"); + assert_eq!(result.exit_code, 0, "preflight should succeed"); + assert!(!marker.exists(), "enclosing Git shim must not run"); + } + #[test] fn apply_modify_conflict() { let _g = env_lock().lock().unwrap(); diff --git a/codex-rs/git-utils/src/apply_filter_tests.rs b/codex-rs/git-utils/src/apply_filter_tests.rs new file mode 100644 index 0000000000..8536f9617c --- /dev/null +++ b/codex-rs/git-utils/src/apply_filter_tests.rs @@ -0,0 +1,479 @@ +use super::*; +use pretty_assertions::assert_eq; +use std::ffi::OsStr; +use std::fs::File; +use std::fs::FileTimes; +use std::path::Path; +use std::time::UNIX_EPOCH; + +const PATCH: &str = + "diff --git a/file.txt b/file.txt\n--- a/file.txt\n+++ b/file.txt\n@@ -1 +1 @@\n-orig\n+next\n"; +const FILTER_COMMAND: &str = "git config codex.filterran true && git hash-object --stdin"; +const PROCESS_FILTER_COMMAND: &str = + "git config codex.filterran true && git rev-parse --verify refs/codex-filter-must-not-run"; +const COMMAND_SCOPED_FILTER_CONFIG: &str = + "filter.demo.clean=git config codex.filterran true && git hash-object --stdin"; + +#[derive(Clone, Copy, Debug)] +enum PatchKind { + Direct, + ThreeWay, +} + +fn run(cwd: &Path, args: &[&str]) -> (i32, String, String) { + let mut command = std::process::Command::new("git"); + isolate_git_command_environment(&mut command); + let output = command + .args(args) + .current_dir(cwd) + .output() + .expect("run Git"); + ( + output.status.code().unwrap_or(-1), + String::from_utf8_lossy(&output.stdout).into_owned(), + String::from_utf8_lossy(&output.stderr).into_owned(), + ) +} + +fn run_success(cwd: &Path, args: &[&str]) { + let (code, _, stderr) = run(cwd, args); + assert_eq!(code, 0, "git {args:?}: {stderr}"); +} + +fn init_repo() -> tempfile::TempDir { + let repo = tempfile::tempdir().expect("tempdir"); + let root = repo.path(); + run_success(root, &["init"]); + run_success(root, &["config", "user.email", "codex@example.com"]); + run_success(root, &["config", "user.name", "Codex"]); + std::fs::write(root.join("file.txt"), "orig\n").expect("write file"); + run_success(root, &["add", "file.txt"]); + run_success(root, &["commit", "-m", "seed"]); + repo +} + +fn configure_filter(root: &Path, driver: &str) { + configure_filter_command(root, driver, "clean", FILTER_COMMAND); +} + +fn configure_filter_command(root: &Path, driver: &str, name: &str, command: &str) { + run_success( + root, + &["config", &format!("filter.{driver}.{name}"), command], + ); +} + +fn make_index_racy(root: &Path) { + let index = File::options() + .write(true) + .open(root.join(".git/index")) + .expect("open index"); + index + .set_times(FileTimes::new().set_modified(UNIX_EPOCH)) + .expect("make index entries racy"); +} + +fn build_patch(root: &Path, kind: PatchKind, paths: &[&str]) -> String { + if matches!(kind, PatchKind::Direct) && paths == ["file.txt"] { + return PATCH.to_string(); + } + std::fs::write(root.join("file.txt"), "next\n").expect("write target postimage"); + let mut args = vec!["diff"]; + if matches!(kind, PatchKind::ThreeWay) { + args.extend(["--full-index", "--binary"]); + } + args.push("--"); + args.extend_from_slice(paths); + let (code, stdout, stderr) = run(root, &args); + assert_eq!(code, 0, "generate patch: {stderr}"); + let mut checkout = vec!["checkout", "-q", "--"]; + checkout.extend_from_slice(paths); + run_success(root, &checkout); + stdout +} + +fn index_contents(root: &Path, path: &str) -> String { + let (code, stdout, stderr) = run(root, &["show", &format!(":{path}")]); + assert_eq!(code, 0, "read index {path}: {stderr}"); + stdout +} + +fn setup_racy_offpath_filter( + driver: &str, + name: &str, + command: &str, + kind: PatchKind, +) -> (tempfile::TempDir, String, String) { + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join("outside.txt"), "outside\n").expect("write off-path file"); + std::fs::write( + root.join(".gitattributes"), + format!("outside.txt filter={driver}\n"), + ) + .expect("write off-path attributes"); + run_success(root, &["add", ".gitattributes", "outside.txt"]); + run_success(root, &["commit", "-m", "off-path filter target"]); + let patch = build_patch(root, kind, &["file.txt"]); + configure_filter_command(root, driver, name, command); + run_success( + root, + &["config", &format!("filter.{driver}.required"), "true"], + ); + let outside_index = index_contents(root, "outside.txt"); + make_index_racy(root); + (repo, patch, outside_index) +} + +fn assert_guarded_round_trip( + repo: &tempfile::TempDir, + patch: String, + outside_index: &str, + context: &str, +) { + let root = repo.path(); + let forward = apply_git_patch(&ApplyGitRequest { + cwd: root.to_path_buf(), + diff: patch.clone(), + revert: false, + preflight: false, + }) + .expect("guard forward apply"); + assert_eq!(forward.exit_code, 0, "{context}: {}", forward.stderr); + assert!(!configured_filter_ran(root), "{context}: forward helper"); + assert_eq!(index_contents(root, "file.txt"), "next\n", "{context}"); + assert_eq!( + index_contents(root, "outside.txt"), + outside_index, + "{context}" + ); + + make_index_racy(root); + let reverse = apply_git_patch(&ApplyGitRequest { + cwd: root.to_path_buf(), + diff: patch, + revert: true, + preflight: false, + }) + .expect("guard reverse apply"); + assert_eq!(reverse.exit_code, 0, "{context}: {}", reverse.stderr); + assert!(!configured_filter_ran(root), "{context}: reverse helper"); + assert_eq!(index_contents(root, "file.txt"), "orig\n", "{context}"); + assert_eq!( + index_contents(root, "outside.txt"), + outside_index, + "{context}" + ); +} + +fn configured_filter_ran(root: &Path) -> bool { + run(root, &["config", "--get", "codex.filterran"]).0 == 0 +} + +fn request(root: &Path, revert: bool, preflight: bool) -> ApplyGitRequest { + ApplyGitRequest { + cwd: root.to_path_buf(), + diff: PATCH.to_string(), + revert, + preflight, + } +} + +fn run_isolated_test(test_name: &str, env: &[(&str, &OsStr)]) { + let mut command = std::process::Command::new(std::env::current_exe().expect("test binary")); + isolate_git_command_environment(&mut command); + command + .arg(test_name) + .arg("--exact") + .arg("--nocapture") + .env("CODEX_GIT_UTILS_APPLY_FILTER_ENV_CHILD", "1") + .env("RUST_TEST_THREADS", "1"); + for (name, value) in env { + command.env(name, value); + } + let output = command.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 reverse_staging_uses_command_scoped_filter_override() { + if std::env::var_os("CODEX_GIT_UTILS_APPLY_FILTER_ENV_CHILD").is_none() { + run_isolated_test( + "apply::filter_tests::reverse_staging_uses_command_scoped_filter_override", + &[("CODEX_APPLY_GIT_CFG", OsStr::new("filter.demo.clean="))], + ); + return; + } + + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join(".gitattributes"), "file.txt filter=demo\n") + .expect("write attributes"); + run_success(root, &["add", ".gitattributes"]); + run_success(root, &["commit", "-m", "attributes"]); + configure_filter(root, "demo"); + + let forward = apply_git_patch(&request( + root, /*revert*/ false, /*preflight*/ false, + )) + .expect("apply with disabled command-scoped filter"); + assert_eq!(forward.exit_code, 0); + let reverse = apply_git_patch(&request( + root, /*revert*/ true, /*preflight*/ false, + )) + .expect("reverse with disabled command-scoped filter"); + assert_eq!(reverse.exit_code, 0); + assert!(!configured_filter_ran(root)); + assert_eq!( + std::fs::read_to_string(root.join("file.txt")).expect("read file"), + "orig\n" + ); +} + +#[test] +fn apply_treats_empty_name_filter_as_selected_only() { + for command in ["clean", "smudge", "process"] { + let repo = init_repo(); + let root = repo.path(); + run_success( + root, + &["config", &format!("filter..{command}"), FILTER_COMMAND], + ); + + std::fs::write(root.join(".gitattributes"), "file.txt filter=other\n") + .expect("write unrelated attributes"); + let unused = apply_git_patch(&request( + root, /*revert*/ false, /*preflight*/ true, + )) + .expect("allow unused empty-name filter"); + assert_eq!(unused.exit_code, 0, "{command}"); + assert!(!configured_filter_ran(root), "{command}"); + + std::fs::write(root.join(".gitattributes"), "file.txt filter=\n") + .expect("select empty-name filter"); + let selected = apply_git_patch(&request( + root, /*revert*/ false, /*preflight*/ true, + )) + .expect_err("reject selected empty-name filter"); + assert_eq!(selected.kind(), io::ErrorKind::Unsupported, "{command}"); + assert!(!configured_filter_ran(root), "{command}"); + } +} + +#[test] +fn apply_distinguishes_filter_sentinels_from_literal_driver_names() { + for (driver, sentinel_attribute) in + [("set", "filter"), ("unset", "-filter"), ("unspecified", "")] + { + let repo = init_repo(); + let root = repo.path(); + configure_filter(root, driver); + run_success( + root, + &["config", &format!("filter.{driver}.smudge"), FILTER_COMMAND], + ); + run_success( + root, + &[ + "config", + &format!("filter.{driver}.process"), + FILTER_COMMAND, + ], + ); + + let attributes = if sentinel_attribute.is_empty() { + String::new() + } else { + format!("file.txt {sentinel_attribute}\n") + }; + std::fs::write(root.join(".gitattributes"), attributes).expect("write sentinel attribute"); + let sentinel = apply_git_patch(&request( + root, /*revert*/ false, /*preflight*/ true, + )) + .expect("allow attribute sentinel"); + assert_eq!(sentinel.exit_code, 0, "{driver}"); + assert!(!configured_filter_ran(root), "{driver}"); + + std::fs::write( + root.join(".gitattributes"), + format!("file.txt filter={driver}\n"), + ) + .expect("write literal driver attribute"); + let selected = apply_git_patch(&request( + root, /*revert*/ false, /*preflight*/ true, + )) + .expect_err("reject literal sentinel-named driver"); + assert_eq!(selected.kind(), io::ErrorKind::Unsupported, "{driver}"); + assert!(!configured_filter_ran(root), "{driver}"); + } +} + +#[test] +fn apply_allows_effectively_empty_empty_name_filter() { + if std::env::var_os("CODEX_GIT_UTILS_APPLY_FILTER_ENV_CHILD").is_none() { + run_isolated_test( + "apply::filter_tests::apply_allows_effectively_empty_empty_name_filter", + &[( + "CODEX_APPLY_GIT_CFG", + OsStr::new("filter..clean=,filter..smudge=,filter..process="), + )], + ); + return; + } + + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join(".gitattributes"), "file.txt filter=\n") + .expect("select empty-name filter"); + for command in ["clean", "smudge", "process"] { + run_success( + root, + &["config", &format!("filter..{command}"), FILTER_COMMAND], + ); + } + run_success(root, &["config", "filter.unused.clean", "false"]); + + let result = apply_git_patch(&request( + root, /*revert*/ false, /*preflight*/ true, + )) + .expect("allow effectively empty empty-name filter"); + assert_eq!(result.exit_code, 0); + assert!(!configured_filter_ran(root)); +} + +#[test] +fn apply_neutralizes_racy_offpath_clean_and_process_filters() { + for (name, command) in [ + ("clean", FILTER_COMMAND), + ("process", PROCESS_FILTER_COMMAND), + ] { + for kind in [PatchKind::Direct, PatchKind::ThreeWay] { + let (repo, patch, outside_index) = + setup_racy_offpath_filter("demo", name, command, kind); + assert_guarded_round_trip(&repo, patch, &outside_index, &format!("{name} {kind:?}")); + } + } +} + +#[test] +fn apply_neutralizes_empty_and_equals_filter_driver_names() { + for (driver, name, command, kind) in [ + ("", "clean", FILTER_COMMAND, PatchKind::Direct), + ( + "x=y", + "process", + PROCESS_FILTER_COMMAND, + PatchKind::ThreeWay, + ), + ] { + let (repo, patch, outside_index) = setup_racy_offpath_filter(driver, name, command, kind); + assert_guarded_round_trip( + &repo, + patch, + &outside_index, + &format!("driver {driver:?} {name}"), + ); + } +} + +#[test] +fn command_scoped_offpath_filter_is_neutralized_after_local_config() { + if std::env::var_os("CODEX_GIT_UTILS_APPLY_FILTER_ENV_CHILD").is_none() { + run_isolated_test( + "apply::filter_tests::command_scoped_offpath_filter_is_neutralized_after_local_config", + &[( + "CODEX_APPLY_GIT_CFG", + OsStr::new(COMMAND_SCOPED_FILTER_CONFIG), + )], + ); + return; + } + + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join("outside.txt"), "outside\n").expect("write off-path file"); + std::fs::write(root.join(".gitattributes"), "outside.txt filter=demo\n") + .expect("write off-path attributes"); + run_success(root, &["add", ".gitattributes", "outside.txt"]); + run_success(root, &["commit", "-m", "off-path filter target"]); + run_success(root, &["config", "filter.demo.clean", ""]); + run_success(root, &["config", "filter.demo.required", "true"]); + let outside_index = index_contents(root, "outside.txt"); + make_index_racy(root); + assert_guarded_round_trip( + &repo, + PATCH.to_string(), + &outside_index, + "command-scoped clean", + ); +} + +#[test] +fn apply_neutralizes_filter_activated_for_racy_offpath_by_same_patch() { + for (name, command) in [ + ("clean", FILTER_COMMAND), + ("process", PROCESS_FILTER_COMMAND), + ] { + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join("outside.txt"), "outside\n").expect("write off-path file"); + std::fs::write(root.join(".gitattributes"), "outside.txt -filter\n") + .expect("write preimage attributes"); + run_success(root, &["add", ".gitattributes", "outside.txt"]); + run_success(root, &["commit", "-m", "off-path filter preimage"]); + + std::fs::write(root.join(".gitattributes"), "outside.txt filter=demo\n") + .expect("write postimage attributes"); + let patch = build_patch(root, PatchKind::ThreeWay, &[".gitattributes", "file.txt"]); + configure_filter_command(root, "demo", name, command); + run_success(root, &["config", "filter.demo.required", "true"]); + let outside_index = index_contents(root, "outside.txt"); + make_index_racy(root); + + assert_guarded_round_trip( + &repo, + patch, + &outside_index, + &format!("dynamic off-path {name}"), + ); + assert_eq!( + std::fs::read_to_string(root.join(".gitattributes")).expect("read restored attributes"), + "outside.txt -filter\n" + ); + } +} + +#[test] +fn apply_still_refuses_clean_and_process_filters_selected_for_target() { + for (name, command) in [ + ("clean", FILTER_COMMAND), + ("process", PROCESS_FILTER_COMMAND), + ] { + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join(".gitattributes"), "file.txt filter=demo\n") + .expect("write target attributes"); + run_success(root, &["add", ".gitattributes"]); + run_success(root, &["commit", "-m", "target filter"]); + configure_filter_command(root, "demo", name, command); + run_success(root, &["config", "filter.demo.required", "true"]); + + let error = apply_git_patch(&request( + root, /*revert*/ false, /*preflight*/ false, + )) + .expect_err("refuse selected target filter"); + assert_eq!(error.kind(), io::ErrorKind::Unsupported, "{name}"); + assert!(!configured_filter_ran(root), "{name}"); + assert_eq!(index_contents(root, "file.txt"), "orig\n", "{name}"); + assert_eq!( + std::fs::read_to_string(root.join("file.txt")).expect("read target"), + "orig\n", + "{name}" + ); + } +} diff --git a/codex-rs/git-utils/src/git_command.rs b/codex-rs/git-utils/src/git_command.rs index 997578b839..5edb011f3c 100644 --- a/codex-rs/git-utils/src/git_command.rs +++ b/codex-rs/git-utils/src/git_command.rs @@ -16,7 +16,7 @@ pub(crate) struct GitRunner { struct UntrustedGitLocations { roots: Vec, - common_dir: Option, + common_dirs: Vec, } impl GitRunner { @@ -76,25 +76,110 @@ impl GitRunner { } fn untrusted_git_locations_for_cwd(cwd: &Path) -> Result { + let lexical_cwd = if cwd.is_absolute() { + cwd.to_path_buf() + } else { + std::env::current_dir() + .map_err(|_| GitReadError::NotRepository { + path: cwd.to_path_buf(), + })? + .join(cwd) + }; let canonical_cwd = std::fs::canonicalize(cwd).map_err(|_| GitReadError::NotRepository { path: cwd.to_path_buf(), })?; let worktree_root = crate::get_git_repo_root(&canonical_cwd) .and_then(|root| std::fs::canonicalize(root).ok()) .unwrap_or_else(|| canonical_cwd.clone()); - let mut roots = vec![worktree_root.clone()]; + let mut locations = UntrustedGitLocations { + roots: Vec::new(), + common_dirs: Vec::new(), + }; + record_repository_ancestry(&worktree_root, &mut locations)?; + + // Canonicalization can erase a repository-controlled symlink prefix. Walk + // the requested spelling too, deliberately retaining symlink and `..` + // components so every lexical enclosing checkout remains untrusted. + let lexical_base = if lexical_cwd.is_dir() { + lexical_cwd + } else { + lexical_cwd + .parent() + .ok_or_else(|| GitReadError::NotRepository { + path: cwd.to_path_buf(), + })? + .to_path_buf() + }; + record_repository_ancestry(&lexical_base, &mut locations)?; + + // Callers commonly obtain their default cwd from `current_dir()`, which + // can already have erased a symlink spelling. Recover the standard logical + // process cwd only when it is absolute and resolves to both the requested + // cwd and the process cwd. Treating extra roots as untrusted cannot widen + // executable selection. + if let Some(logical_cwd) = validated_logical_process_cwd(&canonical_cwd) { + record_repository_ancestry(&logical_cwd, &mut locations)?; + } + Ok(locations) +} + +fn validated_logical_process_cwd(canonical_cwd: &Path) -> Option { + let process_cwd = std::fs::canonicalize(std::env::current_dir().ok()?).ok()?; + if !paths_equal(&process_cwd, canonical_cwd) { + return None; + } + let logical_cwd = PathBuf::from(std::env::var_os("PWD")?); + if !logical_cwd.is_absolute() { + return None; + } + let canonical_logical_cwd = std::fs::canonicalize(&logical_cwd).ok()?; + paths_equal(&canonical_logical_cwd, canonical_cwd).then_some(logical_cwd) +} + +fn record_repository_ancestry( + start: &Path, + locations: &mut UntrustedGitLocations, +) -> Result<(), GitReadError> { + push_unique(&mut locations.roots, start.to_path_buf()); + record_repository_marker(start, locations)?; + for ancestor in start.parent().into_iter().flat_map(Path::ancestors) { + let dot_git = ancestor.join(".git"); + match std::fs::symlink_metadata(&dot_git) { + Ok(_) => { + push_unique(&mut locations.roots, ancestor.to_path_buf()); + let canonical_root = + std::fs::canonicalize(ancestor).map_err(|_| GitReadError::NoTrustedGit)?; + push_unique(&mut locations.roots, canonical_root.clone()); + record_repository_marker(ancestor, locations)?; + } + Err(error) if error.kind() == io::ErrorKind::NotFound => {} + Err(_) => return Err(GitReadError::NoTrustedGit), + } + } + Ok(()) +} + +fn record_repository_marker( + worktree_root: &Path, + locations: &mut UntrustedGitLocations, +) -> Result<(), GitReadError> { let dot_git = worktree_root.join(".git"); let common_dir = match std::fs::symlink_metadata(&dot_git) { - Ok(_) => Some(resolve_common_git_dir(&dot_git).map_err(|()| GitReadError::NoTrustedGit)?), - Err(error) if error.kind() == io::ErrorKind::NotFound => None, + Ok(_) => resolve_common_git_dir(&dot_git).map_err(|()| GitReadError::NoTrustedGit)?, + Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(()), Err(_) => return Err(GitReadError::NoTrustedGit), }; - if let Some(common_dir) = &common_dir - && !path_is_within(common_dir, &worktree_root) - { - roots.push(common_dir.clone()); + if !path_is_within(&common_dir, worktree_root) { + push_unique(&mut locations.roots, common_dir.clone()); + } + push_unique(&mut locations.common_dirs, common_dir); + Ok(()) +} + +fn push_unique(paths: &mut Vec, path: PathBuf) { + if !paths.iter().any(|existing| paths_equal(existing, &path)) { + paths.push(path); } - Ok(UntrustedGitLocations { roots, common_dir }) } fn path_is_untrusted(path: &Path, locations: &UntrustedGitLocations) -> bool { @@ -106,9 +191,9 @@ fn path_is_untrusted(path: &Path, locations: &UntrustedGitLocations) -> bool { return true; } locations - .common_dir - .as_deref() - .is_some_and(|common_dir| path_is_in_worktree_for_common_dir(path, common_dir)) + .common_dirs + .iter() + .any(|common_dir| path_is_in_worktree_for_common_dir(path, common_dir)) } fn path_is_in_worktree_for_common_dir(path: &Path, expected_common_dir: &Path) -> bool { diff --git a/codex-rs/git-utils/src/git_command_tests.rs b/codex-rs/git-utils/src/git_command_tests.rs index 29debcd749..77807ba8d4 100644 --- a/codex-rs/git-utils/src/git_command_tests.rs +++ b/codex-rs/git-utils/src/git_command_tests.rs @@ -2,6 +2,8 @@ use super::*; use pretty_assertions::assert_eq; use std::process::Command; +use crate::safe_git::DISABLED_HOOKS_PATH; + #[cfg(unix)] fn write_executable(path: &Path, body: &str) { use std::os::unix::fs::PermissionsExt; @@ -18,6 +20,12 @@ fn run_git(cwd: &Path, args: &[&str]) { let mut command = Command::new("git"); isolate_git_command_environment(&mut command); let status = command + .args([ + "-c", + &format!("core.hooksPath={DISABLED_HOOKS_PATH}"), + "-c", + "core.fsmonitor=false", + ]) .args(args) .current_dir(cwd) .status() @@ -25,6 +33,23 @@ fn run_git(cwd: &Path, args: &[&str]) { assert!(status.success(), "git {args:?} failed"); } +fn commit_all(cwd: &Path, message: &str) { + run_git( + cwd, + &[ + "-c", + "user.name=Codex Test", + "-c", + "user.email=codex@example.com", + "-c", + "commit.gpgSign=false", + "commit", + "-qam", + message, + ], + ); +} + fn write_git_candidate(directory: &Path) { std::fs::create_dir_all(directory).expect("create candidate directory"); let candidate = directory.join(git_executable_name()); @@ -37,9 +62,14 @@ fn write_git_candidate(directory: &Path) { } fn locations_for_root(root: &Path) -> UntrustedGitLocations { + let mut roots = vec![root.to_path_buf()]; + push_unique( + &mut roots, + std::fs::canonicalize(root).expect("canonical root"), + ); UntrustedGitLocations { - roots: vec![std::fs::canonicalize(root).expect("canonical root")], - common_dir: None, + roots, + common_dirs: Vec::new(), } } @@ -107,6 +137,130 @@ fn linked_worktree_rejects_git_from_main_and_linked_worktrees() { )); } +#[test] +fn nested_repository_rejects_git_from_enclosing_repository() { + let fixture = tempfile::tempdir().expect("fixture"); + let outer = fixture.path().join("outer"); + let nested = outer.join("nested"); + let outer_bin = outer.join("bin"); + let trusted_bin = fixture.path().join("trusted-bin"); + std::fs::create_dir_all(&nested).expect("create nested repository"); + run_git(&outer, &["init", "-q"]); + run_git(&nested, &["init", "-q"]); + write_git_candidate(&outer_bin); + write_git_candidate(&trusted_bin); + + let locations = untrusted_git_locations_for_cwd(&nested).expect("untrusted locations"); + assert!( + path_is_untrusted(&outer_bin.join(git_executable_name()), &locations), + "Git from an enclosing repository must remain repository-controlled" + ); + assert_eq!( + selected_git(&locations, &[&outer_bin, &trusted_bin]), + trusted_bin.join(git_executable_name()) + ); +} + +#[cfg(unix)] +#[test] +fn symlinked_nested_repository_rejects_git_from_lexical_enclosing_repository() { + let fixture = tempfile::tempdir().expect("fixture"); + let outer = fixture.path().join("outer"); + let physical_nested = fixture.path().join("physical-nested"); + let lexical_nested = outer.join("nested"); + let outer_bin = outer.join("bin"); + let trusted_bin = fixture.path().join("trusted-bin"); + std::fs::create_dir_all(&outer).expect("create outer repository"); + std::fs::create_dir_all(&physical_nested).expect("create physical nested repository"); + run_git(&outer, &["init", "-q"]); + run_git(&physical_nested, &["init", "-q"]); + std::os::unix::fs::symlink(&physical_nested, &lexical_nested) + .expect("symlink nested repository"); + write_git_candidate(&outer_bin); + write_git_candidate(&trusted_bin); + + let locations = untrusted_git_locations_for_cwd(&lexical_nested).expect("untrusted locations"); + assert!( + path_is_untrusted(&outer_bin.join(git_executable_name()), &locations), + "Git from the lexical enclosing repository must remain repository-controlled" + ); + assert_eq!( + selected_git(&locations, &[&outer_bin, &trusted_bin]), + trusted_bin.join(git_executable_name()) + ); +} + +#[test] +fn nested_repository_rejects_git_from_enclosing_repository_main_worktree() { + let fixture = tempfile::tempdir().expect("fixture"); + let main = fixture.path().join("main"); + let linked = fixture.path().join("linked"); + let nested = linked.join("nested"); + let main_bin = main.join("bin"); + let trusted_bin = fixture.path().join("trusted-bin"); + std::fs::create_dir_all(&main).expect("create main worktree"); + run_git(&main, &["init", "-q"]); + run_git(&main, &["worktree", "add", "--orphan", path_text(&linked)]); + std::fs::create_dir_all(&nested).expect("create nested repository"); + run_git(&nested, &["init", "-q"]); + write_git_candidate(&main_bin); + write_git_candidate(&trusted_bin); + + let locations = untrusted_git_locations_for_cwd(&nested).expect("untrusted locations"); + assert!( + path_is_untrusted(&main_bin.join(git_executable_name()), &locations), + "all worktrees of an enclosing repository must remain repository-controlled" + ); + assert_eq!( + selected_git(&locations, &[&main_bin, &trusted_bin]), + trusted_bin.join(git_executable_name()) + ); +} + +#[test] +fn submodule_rejects_git_from_enclosing_superproject() { + let fixture = tempfile::tempdir().expect("fixture"); + let source = fixture.path().join("source"); + let outer = fixture.path().join("outer"); + let submodule = outer.join("nested"); + let outer_bin = outer.join("bin"); + let trusted_bin = fixture.path().join("trusted-bin"); + std::fs::create_dir_all(&source).expect("create source repository"); + std::fs::create_dir_all(&outer).expect("create superproject"); + run_git(&source, &["init", "-q"]); + std::fs::write(source.join("source.txt"), "source\n").expect("write source file"); + run_git(&source, &["add", "source.txt"]); + commit_all(&source, "source"); + run_git(&outer, &["init", "-q"]); + std::fs::write(outer.join("outer.txt"), "outer\n").expect("write outer file"); + run_git(&outer, &["add", "outer.txt"]); + commit_all(&outer, "outer"); + run_git( + &outer, + &[ + "-c", + "protocol.file.allow=always", + "submodule", + "add", + "-q", + path_text(&source), + "nested", + ], + ); + write_git_candidate(&outer_bin); + write_git_candidate(&trusted_bin); + + let locations = untrusted_git_locations_for_cwd(&submodule).expect("untrusted locations"); + assert!( + path_is_untrusted(&outer_bin.join(git_executable_name()), &locations), + "Git from a superproject must remain repository-controlled" + ); + assert_eq!( + selected_git(&locations, &[&outer_bin, &trusted_bin]), + trusted_bin.join(git_executable_name()) + ); +} + #[test] fn bare_backed_linked_worktree_allows_external_git_in_sibling_directory() { let fixture = tempfile::tempdir().expect("fixture"); @@ -171,6 +325,28 @@ fn separate_dot_git_dir_rejects_main_candidate_and_allows_unrelated_repo_candida ); } +#[test] +fn resolver_rejects_parent_traversal_spelled_through_repository() { + let fixture = tempfile::tempdir().expect("fixture"); + let repo = fixture.path().join("repo"); + let trusted_bin = fixture.path().join("trusted-bin"); + std::fs::create_dir_all(&repo).expect("create repository"); + write_git_candidate(&trusted_bin); + + let locations = locations_for_root(&repo); + for root in &locations.roots { + let traversing_path = root.join("../trusted-bin"); + let search_path = std::env::join_paths([traversing_path]).expect("PATH"); + assert!( + matches!( + GitRunner::from_search_path(&locations, &search_path), + Err(GitReadError::NoTrustedGit) + ), + "resolver accepted parent traversal from {root:?}" + ); + } +} + #[cfg(windows)] #[test] fn resolver_selects_native_git_exe_only() { diff --git a/codex-rs/git-utils/src/operations.rs b/codex-rs/git-utils/src/operations.rs index d58ff0beed..1564b81a19 100644 --- a/codex-rs/git-utils/src/operations.rs +++ b/codex-rs/git-utils/src/operations.rs @@ -2,14 +2,10 @@ use std::ffi::OsStr; use std::ffi::OsString; use std::path::Path; use std::path::PathBuf; -#[cfg(test)] -use std::process::Command; use crate::GitToolingError; use crate::git_command::GitRunner; use crate::safe_git::DISABLED_HOOKS_PATH; -#[cfg(test)] -use crate::safe_git::isolate_git_command_environment; pub(crate) fn ensure_git_repository(path: &Path) -> Result<(), GitToolingError> { match run_git_for_stdout( @@ -155,70 +151,5 @@ struct GitRun { } #[cfg(test)] -mod tests { - use super::*; - - fn init_repo() -> tempfile::TempDir { - let repo = tempfile::tempdir().expect("tempdir"); - let mut command = Command::new("git"); - isolate_git_command_environment(&mut command); - let status = command - .args(["init", "-q"]) - .current_dir(repo.path()) - .status() - .expect("initialize repository"); - assert!(status.success()); - repo - } - - #[test] - fn caller_env_cannot_restore_repository_or_pathspec_selectors() { - let target = init_repo(); - let alternate = init_repo(); - std::fs::write(target.path().join("target.txt"), "target\n").expect("target file"); - std::fs::write(alternate.path().join("alternate.txt"), "alternate\n") - .expect("alternate file"); - for (repo, path) in [(&target, "target.txt"), (&alternate, "alternate.txt")] { - let mut command = Command::new("git"); - isolate_git_command_environment(&mut command); - let status = command - .args(["add", path]) - .current_dir(repo.path()) - .status() - .expect("add file"); - assert!(status.success()); - } - - let alternate_git_dir = alternate.path().join(".git"); - let env = [ - ( - OsString::from("GIT_DIR"), - alternate_git_dir.as_os_str().into(), - ), - ( - OsString::from("GIT_WORK_TREE"), - alternate.path().as_os_str().into(), - ), - ( - OsString::from("GIT_COMMON_DIR"), - alternate_git_dir.as_os_str().into(), - ), - ( - OsString::from("GIT_INDEX_FILE"), - alternate_git_dir.join("index").into_os_string(), - ), - (OsString::from("GIT_PREFIX"), OsString::from("elsewhere/")), - (OsString::from("GIT_LITERAL_PATHSPECS"), OsString::from("1")), - (OsString::from("GIT_GLOB_PATHSPECS"), OsString::from("1")), - (OsString::from("GIT_NOGLOB_PATHSPECS"), OsString::from("1")), - (OsString::from("GIT_ICASE_PATHSPECS"), OsString::from("1")), - ( - OsString::from("GIT_CONFIG"), - alternate_git_dir.join("config").into_os_string(), - ), - ]; - let output = run_git_for_stdout(target.path(), ["ls-files"], Some(&env)) - .expect("query cwd-selected index"); - assert_eq!(output, "target.txt"); - } -} +#[path = "operations_tests.rs"] +mod tests; diff --git a/codex-rs/git-utils/src/operations_tests.rs b/codex-rs/git-utils/src/operations_tests.rs new file mode 100644 index 0000000000..e775212e26 --- /dev/null +++ b/codex-rs/git-utils/src/operations_tests.rs @@ -0,0 +1,66 @@ +use super::*; +use crate::safe_git::isolate_git_command_environment; +use std::process::Command; + +fn init_repo() -> tempfile::TempDir { + let repo = tempfile::tempdir().expect("tempdir"); + let mut command = Command::new("git"); + isolate_git_command_environment(&mut command); + let status = command + .args(["init", "-q"]) + .current_dir(repo.path()) + .status() + .expect("initialize repository"); + assert!(status.success()); + repo +} + +#[test] +fn caller_env_cannot_restore_repository_or_pathspec_selectors() { + let target = init_repo(); + let alternate = init_repo(); + std::fs::write(target.path().join("target.txt"), "target\n").expect("target file"); + std::fs::write(alternate.path().join("alternate.txt"), "alternate\n").expect("alternate file"); + for (repo, path) in [(&target, "target.txt"), (&alternate, "alternate.txt")] { + let mut command = Command::new("git"); + isolate_git_command_environment(&mut command); + let status = command + .args(["add", path]) + .current_dir(repo.path()) + .status() + .expect("add file"); + assert!(status.success()); + } + + let alternate_git_dir = alternate.path().join(".git"); + let env = [ + ( + OsString::from("GIT_DIR"), + alternate_git_dir.as_os_str().into(), + ), + ( + OsString::from("GIT_WORK_TREE"), + alternate.path().as_os_str().into(), + ), + ( + OsString::from("GIT_COMMON_DIR"), + alternate_git_dir.as_os_str().into(), + ), + ( + OsString::from("GIT_INDEX_FILE"), + alternate_git_dir.join("index").into_os_string(), + ), + (OsString::from("GIT_PREFIX"), OsString::from("elsewhere/")), + (OsString::from("GIT_LITERAL_PATHSPECS"), OsString::from("1")), + (OsString::from("GIT_GLOB_PATHSPECS"), OsString::from("1")), + (OsString::from("GIT_NOGLOB_PATHSPECS"), OsString::from("1")), + (OsString::from("GIT_ICASE_PATHSPECS"), OsString::from("1")), + ( + OsString::from("GIT_CONFIG"), + alternate_git_dir.join("config").into_os_string(), + ), + ]; + let output = run_git_for_stdout(target.path(), ["ls-files"], Some(&env)) + .expect("query cwd-selected index"); + assert_eq!(output, "target.txt"); +} diff --git a/codex-rs/git-utils/src/patch_paths.rs b/codex-rs/git-utils/src/patch_paths.rs index 99ff58a28e..d8823227b9 100644 --- a/codex-rs/git-utils/src/patch_paths.rs +++ b/codex-rs/git-utils/src/patch_paths.rs @@ -231,39 +231,47 @@ pub fn stage_paths(git_root: &Path, diff: &str) -> io::Result<()> { let (tmpdir, patch_path) = write_temp_patch(diff)?; let paths = extract_effective_paths_from_patch(&git, &patch_path, /*revert*/ true)?; let _guard = tmpdir; - stage_effective_paths(&git, git_root, &paths) + stage_effective_paths(&git, git_root, &paths, &safe_git_config_parts()) } pub(crate) fn stage_effective_paths( git: &GitRunner, git_root: &Path, paths: &[String], + git_config_args: &[String], ) -> io::Result<()> { let confined = confine_patch_paths(git, git_root, 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); if let Ok(metadata) = std::fs::symlink_metadata(&joined) { - if leaf_is_traversable_directory(metadata.file_type()) { + let file_type = metadata.file_type(); + if leaf_is_traversable_directory(file_type) { return Err(containment_error( "refusing to recursively stage a directory patch path", )); } + if leaf_may_run_git_content_filter(file_type) { + content_filter_paths.push(path.clone()); + } existing.push(path); } } if existing.is_empty() { return Ok(()); } - ensure_no_selected_git_add_filters(git, git_root, &existing, &[])?; + let filter_guard = + ensure_no_selected_git_add_filters(git, git_root, &content_filter_paths, git_config_args)?; let mut args = vec![ "--literal-pathspecs".to_string(), "add".to_string(), "--".to_string(), ]; args.extend(existing); - let config_parts = safe_git_config_parts(); - let (_code, _, _) = run_git(git, git_root, &config_parts, &args)?; + let mut guarded_config = git_config_args.to_vec(); + guarded_config.extend_from_slice(filter_guard.git_config_args()); + let (_code, _, _) = run_git(git, git_root, &guarded_config, &args)?; // We do not hard fail staging; best-effort is OK. Return Ok even on non-zero. Ok(()) } @@ -273,6 +281,21 @@ fn leaf_is_traversable_directory(file_type: std::fs::FileType) -> bool { file_type.is_dir() } +#[cfg(unix)] +fn leaf_may_run_git_content_filter(file_type: std::fs::FileType) -> bool { + // Git stages an exact Unix symlink as a mode-120000 blob containing the + // link target. The whole-command neutralizer covers unrelated racy index + // entries while this target is omitted from the selected-filter refusal. + !file_type.is_symlink() +} + +#[cfg(not(unix))] +fn leaf_may_run_git_content_filter(_file_type: std::fs::FileType) -> bool { + // Keep the conservative policy on platforms whose symlink staging + // behavior can depend on repository and host configuration. + true +} + #[cfg(windows)] fn leaf_is_traversable_directory(file_type: std::fs::FileType) -> bool { use std::os::windows::fs::FileTypeExt; diff --git a/codex-rs/git-utils/src/patch_paths_tests.rs b/codex-rs/git-utils/src/patch_paths_tests.rs index f3aed3b4eb..52000767e6 100644 --- a/codex-rs/git-utils/src/patch_paths_tests.rs +++ b/codex-rs/git-utils/src/patch_paths_tests.rs @@ -452,6 +452,74 @@ fn init_repo_with_selected_filter(config_key: &str) -> tempfile::TempDir { repo } +#[cfg(unix)] +struct RacyFilterFixture { + repo: tempfile::TempDir, + marker: PathBuf, +} + +#[cfg(unix)] +fn init_racy_filter_fixture(driver: &str, command: &str) -> RacyFilterFixture { + use std::os::unix::fs::PermissionsExt; + use std::os::unix::fs::symlink; + use std::time::Duration; + use std::time::SystemTime; + + let repo = init_repo(); + let root = repo.path(); + std::fs::write( + root.join(".gitattributes"), + format!( + "outside.txt filter={driver}\nlink filter={driver}\nselected.txt filter={driver}\n" + ), + ) + .expect("write attributes"); + std::fs::write(root.join("outside.txt"), "outside-racy\n").expect("write racy file"); + std::fs::write(root.join("target.txt"), "old target\n").expect("write target"); + std::fs::write(root.join("selected.txt"), "old selected\n").expect("write selected target"); + symlink("old-target", root.join("link")).expect("create initial symlink"); + + let future = SystemTime::UNIX_EPOCH + Duration::from_secs(1_893_484_800); + let outside = std::fs::File::options() + .read(true) + .write(true) + .open(root.join("outside.txt")) + .expect("open racy file"); + outside + .set_times(std::fs::FileTimes::new().set_modified(future)) + .expect("set future racy mtime"); + assert_eq!(run(root, &["git", "add", "."]).0, 0); + assert_eq!(run(root, &["git", "commit", "-m", "base"]).0, 0); + + let marker = root.join("filter-ran"); + let helper = root.join("filter-helper.sh"); + let helper_body = if command == "clean" { + format!("#!/bin/sh\n: > '{}'\ncat\n", marker.display()) + } else { + format!("#!/bin/sh\n: > '{}'\nexit 1\n", marker.display()) + }; + std::fs::write(&helper, helper_body).expect("write filter helper"); + let mut permissions = std::fs::metadata(&helper) + .expect("filter helper metadata") + .permissions(); + permissions.set_mode(0o755); + std::fs::set_permissions(&helper, permissions).expect("make filter helper executable"); + let key = format!("filter.{driver}.{command}"); + let helper_arg = helper.to_string_lossy().into_owned(); + assert_eq!(run(root, &["git", "config", &key, &helper_arg]).0, 0); + let required_key = format!("filter.{driver}.required"); + assert_eq!(run(root, &["git", "config", &required_key, "true"]).0, 0); + assert!(!marker.exists(), "fixture setup must not run the filter"); + RacyFilterFixture { repo, marker } +} + +#[cfg(unix)] +fn index_entry(root: &Path, path: &str) -> String { + let (code, stdout, stderr) = run(root, &["git", "ls-files", "--stage", "--", path]); + assert_eq!(code, 0, "read index entry: {stderr}"); + stdout +} + #[cfg(any(unix, windows))] fn create_dir_alias(target: &Path, alias: &Path) -> DirectoryAlias { #[cfg(unix)] @@ -752,8 +820,13 @@ fn staging_rejects_global_lfs_filter_without_running_it() { std::env::var_os("CODEX_GIT_UTILS_TARGET_REPO").expect("target repository"), ); let git = GitRunner::for_cwd_io(&root).expect("trusted Git"); - let error = stage_effective_paths(&git, &root, &["file.txt".to_string()]) - .expect_err("reject global Git LFS filter"); + let error = stage_effective_paths( + &git, + &root, + &["file.txt".to_string()], + &safe_git_config_parts(), + ) + .expect_err("reject global Git LFS filter"); assert_eq!(error.kind(), io::ErrorKind::Unsupported); return; } @@ -818,20 +891,226 @@ fn staging_rejects_selected_process_but_allows_smudge_only_filter() { let process_repo = init_repo_with_selected_filter("filter.selected.process"); let process_root = process_repo.path(); let process_git = GitRunner::for_cwd_io(process_root).expect("trusted Git"); - let error = stage_effective_paths(&process_git, process_root, &["file.txt".to_string()]) - .expect_err("reject selected process filter"); + let error = stage_effective_paths( + &process_git, + process_root, + &["file.txt".to_string()], + &safe_git_config_parts(), + ) + .expect_err("reject selected process filter"); assert_eq!(error.kind(), io::ErrorKind::Unsupported); let smudge_repo = init_repo_with_selected_filter("filter.selected.smudge"); let smudge_root = smudge_repo.path(); let smudge_git = GitRunner::for_cwd_io(smudge_root).expect("trusted Git"); - stage_effective_paths(&smudge_git, smudge_root, &["file.txt".to_string()]) - .expect("smudge-only filters do not run during staging"); + stage_effective_paths( + &smudge_git, + smudge_root, + &["file.txt".to_string()], + &safe_git_config_parts(), + ) + .expect("smudge-only filters do not run during staging"); let (code, staged, stderr) = run(smudge_root, &["git", "diff", "--cached", "--name-only"]); assert_eq!(code, 0, "read staged paths: {stderr}"); assert_eq!(staged, "file.txt\n"); } +#[cfg(unix)] +#[test] +fn staging_neutralizes_off_path_racy_filters_without_changing_their_index_entry() { + for (driver, command) in [ + ("selected", "clean"), + ("selected", "process"), + ("", "clean"), + ("x=y", "clean"), + ] { + let control = init_racy_filter_fixture(driver, command); + let control_root = control.repo.path(); + std::fs::write(control_root.join("target.txt"), "new target\n") + .expect("modify control target"); + let _ = run(control_root, &["git", "add", "--", "target.txt"]); + assert!( + control.marker.exists(), + "raw git add should execute the off-path racy {driver:?} {command} filter" + ); + + let guarded = init_racy_filter_fixture(driver, command); + let guarded_root = guarded.repo.path(); + let outside_before = index_entry(guarded_root, "outside.txt"); + std::fs::write(guarded_root.join("target.txt"), "new target\n") + .expect("modify guarded target"); + let git = GitRunner::for_cwd_io(guarded_root).expect("trusted Git"); + stage_effective_paths( + &git, + guarded_root, + &["target.txt".to_string()], + &safe_git_config_parts(), + ) + .expect("stage with off-path filters neutralized"); + + assert!( + !guarded.marker.exists(), + "guarded staging must not execute the off-path racy {driver:?} {command} filter" + ); + assert_eq!( + index_entry(guarded_root, "outside.txt"), + outside_before, + "staging must preserve the unrelated index entry for {driver:?} {command}" + ); + let (code, staged, stderr) = run(guarded_root, &["git", "diff", "--cached", "--name-only"]); + assert_eq!(code, 0, "read staged paths: {stderr}"); + assert_eq!( + staged, "target.txt\n", + "driver={driver:?} command={command}" + ); + } +} + +#[cfg(unix)] +#[test] +fn staging_allows_selected_symlink_while_neutralizing_off_path_filters() { + use std::os::unix::fs::symlink; + + for (command, core_symlinks) in [("clean", true), ("process", false)] { + let fixture = init_racy_filter_fixture("selected", command); + let root = fixture.repo.path(); + assert_eq!( + run( + root, + &[ + "git", + "config", + "core.symlinks", + if core_symlinks { "true" } else { "false" }, + ], + ) + .0, + 0 + ); + std::fs::remove_file(root.join("link")).expect("remove old symlink"); + symlink("new-target", root.join("link")).expect("create updated symlink"); + let outside_before = index_entry(root, "outside.txt"); + + let git = GitRunner::for_cwd_io(root).expect("trusted Git"); + stage_effective_paths(&git, root, &["link".to_string()], &safe_git_config_parts()) + .expect("stage selected symlink"); + + assert!( + !fixture.marker.exists(), + "staging a symlink must not run {command} filters" + ); + assert_eq!(index_entry(root, "outside.txt"), outside_before); + let link_entry = index_entry(root, "link"); + assert!( + link_entry.starts_with("120000 "), + "symlink index mode with core.symlinks={core_symlinks}: {link_entry:?}" + ); + let (code, blob, stderr) = run(root, &["git", "show", ":link"]); + assert_eq!(code, 0, "read staged link: {stderr}"); + assert_eq!(blob, "new-target"); + let (code, staged, stderr) = run(root, &["git", "diff", "--cached", "--name-only"]); + assert_eq!(code, 0, "read staged paths: {stderr}"); + assert_eq!(staged, "link\n"); + } +} + +#[cfg(unix)] +#[test] +fn staging_allows_optional_smudge_only_but_refuses_required_or_malformed_smudge() { + for (required, accepted) in [("false", true), ("yes", false), ("not-a-bool", false)] { + let fixture = init_racy_filter_fixture("selected", "smudge"); + let root = fixture.repo.path(); + assert_eq!( + run( + root, + &["git", "config", "filter.selected.required", required], + ) + .0, + 0 + ); + std::fs::write(root.join("selected.txt"), "new selected\n") + .expect("modify selected target"); + let before = git_index_bytes(root); + let git = GitRunner::for_cwd_io(root).expect("trusted Git"); + let result = stage_effective_paths( + &git, + root, + &["selected.txt".to_string()], + &safe_git_config_parts(), + ); + + assert!( + !fixture.marker.exists(), + "smudge helper must not run for required={required}" + ); + if accepted { + result.expect("stage optional smudge-only target"); + let (code, staged, stderr) = run(root, &["git", "diff", "--cached", "--name-only"]); + assert_eq!(code, 0, "read staged paths: {stderr}"); + assert_eq!(staged, "selected.txt\n"); + } else { + let error = result.expect_err("refuse required or malformed smudge-only target"); + assert_eq!(error.kind(), io::ErrorKind::Unsupported); + assert_eq!(git_index_bytes(root), before); + } + } +} + +#[test] +fn optional_smudge_target_does_not_mask_a_later_clean_target() { + let repo = init_repo(); + let root = repo.path(); + std::fs::write( + root.join(".gitattributes"), + "a.txt filter=smudger\nz.txt filter=cleaner\n", + ) + .expect("write attributes"); + std::fs::write(root.join("a.txt"), "old a\n").expect("write smudge target"); + std::fs::write(root.join("z.txt"), "old z\n").expect("write clean target"); + assert_eq!(run(root, &["git", "add", "."]).0, 0); + assert_eq!(run(root, &["git", "commit", "-m", "base"]).0, 0); + assert_eq!( + run( + root, + &[ + "git", + "config", + "filter.smudger.smudge", + "codex-definitely-missing-smudge-command", + ], + ) + .0, + 0 + ); + assert_eq!( + run( + root, + &[ + "git", + "config", + "filter.cleaner.clean", + "codex-definitely-missing-clean-command", + ], + ) + .0, + 0 + ); + std::fs::write(root.join("a.txt"), "new a\n").expect("modify smudge target"); + std::fs::write(root.join("z.txt"), "new z\n").expect("modify clean target"); + let before = git_index_bytes(root); + + let git = GitRunner::for_cwd_io(root).expect("trusted Git"); + let error = stage_effective_paths( + &git, + root, + &["a.txt".to_string(), "z.txt".to_string()], + &safe_git_config_parts(), + ) + .expect_err("reject the selected clean target after optional smudge"); + assert_eq!(error.kind(), io::ErrorKind::Unsupported); + assert_eq!(git_index_bytes(root), before); +} + #[test] fn staging_filter_probe_uses_only_existing_non_directory_leaves() { let repo = init_repo(); @@ -870,6 +1149,7 @@ fn staging_filter_probe_uses_only_existing_non_directory_leaves() { "deleted.txt".to_string(), "kept.txt".to_string(), ], + &safe_git_config_parts(), ) .expect("probe only the surviving staging leaf"); @@ -913,6 +1193,7 @@ fn staging_skips_filter_probe_when_no_leaf_will_be_staged() { &git, root, &["missing.txt".to_string(), "deleted.txt".to_string()], + &safe_git_config_parts(), ) .expect("skip staging when every effective leaf is absent"); diff --git a/codex-rs/git-utils/src/safe_git.rs b/codex-rs/git-utils/src/safe_git.rs index c83ad7ad24..5a2561811d 100644 --- a/codex-rs/git-utils/src/safe_git.rs +++ b/codex-rs/git-utils/src/safe_git.rs @@ -13,7 +13,30 @@ use crate::git_config::parse_effective_config; use crate::git_config::parse_effective_config_with_origins; pub(crate) const DISABLED_HOOKS_PATH: &str = if cfg!(windows) { "NUL" } else { "/dev/null" }; -pub(crate) const EXECUTABLE_FILTER_CONFIG_PATTERN: &str = r"^filter\..*\.(clean|smudge|process)$"; +pub(crate) const EXECUTABLE_FILTER_CONFIG_PATTERN: &str = + r"^filter\..*\.(clean|smudge|process|required)$"; + +#[derive(Debug, Clone, PartialEq, Eq)] +enum FilterAttributeValue { + Driver(String), + AmbiguousSentinel(String), +} + +pub(crate) struct GitFilterNeutralization { + git_config_args: Vec, + _config_dir: Option, + filter_config: BTreeMap, +} + +impl GitFilterNeutralization { + pub(crate) fn git_config_args(&self) -> &[String] { + &self.git_config_args + } + + pub(crate) fn filter_value(&self, driver: &str, name: &str) -> Option<&str> { + effective_filter_value(&self.filter_config, driver, name) + } +} const ISOLATED_GIT_ENVIRONMENT: [&str; 11] = [ "GIT_DIR", @@ -47,7 +70,7 @@ pub(crate) fn ensure_no_selected_executable_git_filters( cwd: &Path, paths: &[String], git_config_args: &[String], -) -> io::Result<()> { +) -> io::Result { ensure_no_selected_executable_git_filters_for( git, cwd, @@ -62,7 +85,7 @@ pub(crate) fn ensure_no_selected_git_add_filters( cwd: &Path, paths: &[String], git_config_args: &[String], -) -> io::Result<()> { +) -> io::Result { ensure_no_selected_executable_git_filters_for( git, cwd, @@ -78,27 +101,114 @@ fn ensure_no_selected_executable_git_filters_for( paths: &[String], git_config_args: &[String], execution: FilterExecution, -) -> io::Result<()> { +) -> io::Result { let entries = read_filter_config(git, cwd, git_config_args)?; - if !entries.values().any(|entry| !entry.value.is_empty()) { - return Ok(()); + let neutralized_drivers = executable_filter_drivers_for(&entries, execution)?; + let target_drivers = match execution { + FilterExecution::AnyWorktreeOperation => neutralized_drivers.clone(), + FilterExecution::GitAdd => executable_filter_drivers(&entries)?, + }; + if target_drivers.is_empty() { + return Ok(GitFilterNeutralization { + git_config_args: Vec::new(), + _config_dir: None, + filter_config: entries, + }); } let paths = paths .iter() .map(|path| path.as_bytes().to_vec()) .collect::>(); - let attributes = read_filter_attributes(git, cwd, &paths, git_config_args)?; - if let Some((driver, path)) = selected_executable_filter_for(&entries, &attributes, execution)? - { - return Err(io::Error::new( - io::ErrorKind::Unsupported, - format!( - "refusing to run an internal Git worktree operation with executable filter {driver:?} selected for {}", - String::from_utf8_lossy(&path) - ), - )); + let attributes = read_filter_attributes(git, cwd, &paths, git_config_args, &target_drivers)?; + for (path, driver) in &attributes { + if !target_drivers.contains(driver) { + continue; + } + let refused = neutralized_drivers.contains(driver) + || matches!(execution, FilterExecution::GitAdd) + && git_filter_required(git, cwd, &entries, driver)?; + if refused { + return Err(io::Error::new( + io::ErrorKind::Unsupported, + format!( + "refusing to run an internal Git worktree operation with executable filter {driver:?} selected for {}", + String::from_utf8_lossy(path) + ), + )); + } + } + if neutralized_drivers.is_empty() { + return Ok(GitFilterNeutralization { + git_config_args: Vec::new(), + _config_dir: None, + filter_config: entries, + }); + } + executable_filter_guard(git, cwd, entries, &neutralized_drivers) +} + +fn executable_filter_guard( + git: &GitRunner, + cwd: &Path, + filter_config: BTreeMap, + executable_drivers: &BTreeSet, +) -> io::Result { + let config_dir = tempfile::tempdir()?; + let config_path = config_dir.path().join("filter-neutralization.gitconfig"); + std::fs::write(&config_path, [])?; + let guard = GitFilterNeutralization { + git_config_args: vec![ + "-c".to_string(), + format!( + "include.path={}", + config_path + .to_str() + .ok_or_else(|| invalid_filter_output("non-UTF-8 filter guard path"))? + ), + ], + _config_dir: Some(config_dir), + filter_config, + }; + for driver in executable_drivers { + debug_assert!(["clean", "smudge", "process"].into_iter().any(|name| { + guard + .filter_value(driver, name) + .is_some_and(|value| !value.is_empty()) + })); + for command in ["clean", "smudge", "process"] { + guard.write_config_value(git, cwd, &config_path, driver, command, "")?; + } + guard.write_config_value(git, cwd, &config_path, driver, "required", "false")?; + } + Ok(guard) +} + +impl GitFilterNeutralization { + fn write_config_value( + &self, + git: &GitRunner, + cwd: &Path, + config_path: &Path, + driver: &str, + name: &str, + value: &str, + ) -> io::Result<()> { + let mut command = git.command(); + command + .args(["config", "--file"]) + .arg(config_path) + .args(["--add", &format!("filter.{driver}.{name}"), value]) + .current_dir(cwd); + let output = 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(()) } - Ok(()) } #[derive(Clone, Copy)] @@ -192,6 +302,7 @@ fn read_filter_attributes( cwd: &Path, paths: &[Vec], git_config_args: &[String], + executable_drivers: &BTreeSet, ) -> io::Result, String>> { if paths.is_empty() { return Ok(BTreeMap::new()); @@ -224,7 +335,131 @@ fn read_filter_attributes( String::from_utf8_lossy(&output.stderr).trim() ))); } - parse_filter_attributes(&output.stdout, paths) + let attributes = parse_filter_attributes(&output.stdout, paths)?; + resolve_filter_attribute_sentinels(git, cwd, attributes, git_config_args, executable_drivers) +} + +fn resolve_filter_attribute_sentinels( + git: &GitRunner, + cwd: &Path, + attributes: BTreeMap, FilterAttributeValue>, + git_config_args: &[String], + executable_drivers: &BTreeSet, +) -> io::Result, String>> { + let mut resolved = BTreeMap::new(); + 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( + git, + cwd, + &path, + &driver, + git_config_args, + )? + { + resolved.insert(path, driver); + } + } + } + } + Ok(resolved) +} + +/// `git check-attr` serializes both its three special states and literal +/// driver names with the same `set`, `unset`, and `unspecified` strings. Ask +/// Git to resolve the ambiguity with every command for that driver overridden +/// to empty. A required literal driver fails while a special state succeeds. +/// Retrying with the driver optional distinguishes that expected failure from +/// an unrelated probe error. No filter process or shell is started. +fn sentinel_spelling_selects_filter_driver( + git: &GitRunner, + cwd: &Path, + path: &[u8], + driver: &str, + git_config_args: &[String], +) -> io::Result { + let required = run_sentinel_selection_probe( + git, + cwd, + path, + driver, + git_config_args, + /*required*/ true, + )?; + if required.status.success() { + return Ok(false); + } + let optional = run_sentinel_selection_probe( + git, + cwd, + path, + driver, + git_config_args, + /*required*/ false, + )?; + if optional.status.success() { + return Ok(true); + } + Err(io::Error::other(format!( + "git filter attribute selection probe failed with required status {} and optional status {}: {}", + required.status, + optional.status, + String::from_utf8_lossy(&optional.stderr).trim() + ))) +} + +fn run_sentinel_selection_probe( + git: &GitRunner, + cwd: &Path, + path: &[u8], + driver: &str, + git_config_args: &[String], + required: bool, +) -> io::Result { + let mut command = git.command(); + command + .env("GIT_OPTIONAL_LOCKS", "0") + .args(git_config_args) + .args([ + "-c", + &format!("core.hooksPath={DISABLED_HOOKS_PATH}"), + "-c", + "core.fsmonitor=false", + "-c", + &format!("filter.{driver}.required={required}"), + "-c", + &format!("filter.{driver}.clean="), + "-c", + &format!("filter.{driver}.smudge="), + "-c", + &format!("filter.{driver}.process="), + "hash-object", + "--stdin", + ]) + .arg("--path") + .arg(git_path_argument(path)?) + .current_dir(cwd) + .stdin(Stdio::null()); + git.output(command) +} + +#[cfg(unix)] +fn git_path_argument(path: &[u8]) -> io::Result { + use std::os::unix::ffi::OsStringExt; + + Ok(std::ffi::OsString::from_vec(path.to_vec())) +} + +#[cfg(not(unix))] +fn git_path_argument(path: &[u8]) -> io::Result { + let path = std::str::from_utf8(path) + .map_err(|_| invalid_filter_output("non-UTF-8 Git filter attribute path"))?; + Ok(path.into()) } fn selected_executable_filter_for( @@ -232,23 +467,20 @@ fn selected_executable_filter_for( attributes: &BTreeMap, String>, execution: FilterExecution, ) -> io::Result)>> { - let mut executable_drivers = BTreeSet::new(); - for entry in entries.values() { - 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() { - executable_drivers.insert(driver); - } - } + let executable_drivers = executable_filter_drivers_for(entries, execution)?; + Ok(selected_filter(&executable_drivers, attributes)) +} + +fn selected_filter( + drivers: &BTreeSet, + attributes: &BTreeMap, String>, +) -> Option<(String, Vec)> { for (path, driver) in attributes { - if executable_drivers.contains(driver) { - return Ok(Some((driver.clone(), path.clone()))); + if drivers.contains(driver) { + return Some((driver.clone(), path.clone())); } } - Ok(None) + None } #[cfg(test)] @@ -259,7 +491,87 @@ fn selected_executable_filter( selected_executable_filter_for(entries, attributes, FilterExecution::AnyWorktreeOperation) } -#[cfg(test)] +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> { + 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() { + executable_drivers.insert(driver); + } + } + Ok(executable_drivers) +} + +fn effective_filter_value<'a>( + entries: &'a BTreeMap, + driver: &str, + name: &str, +) -> Option<&'a str> { + entries + .get(&format!("filter.{driver}.{name}")) + .map(|entry| entry.value.as_str()) +} + +fn git_filter_required( + git: &GitRunner, + cwd: &Path, + entries: &BTreeMap, + driver: &str, +) -> io::Result { + let Some(value) = effective_filter_value(entries, driver, "required") else { + return Ok(false); + }; + let config = format!("codex.required={value}"); + let mut command = git.command(); + command + .env("GIT_OPTIONAL_LOCKS", "0") + .args([ + "-c", + &config, + "config", + "--type=bool", + "--get", + "codex.required", + ]) + .current_dir(cwd); + let output = git.output(command)?; + if !output.status.success() { + return Err(io::Error::new( + io::ErrorKind::Unsupported, + format!( + "refusing selected Git filter {driver:?} with malformed required value {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:?}" + ), + )), + } +} + fn filter_driver_name(key: &str) -> io::Result { filter_driver_and_command(key).map(|(driver, _command)| driver) } @@ -279,7 +591,6 @@ fn filter_driver_and_command(key: &str) -> io::Result<(String, FilterCommand)> { .strip_suffix(suffix) .map(|driver| (driver, command)) }) - .filter(|(driver, _command)| !driver.is_empty()) .ok_or_else(|| invalid_filter_output("malformed filter config key"))?; Ok((driver.to_string(), command)) } @@ -301,7 +612,7 @@ fn write_nul_paths(input: &mut std::fs::File, paths: &[Vec]) -> io::Result<( fn parse_filter_attributes( output: &[u8], expected_paths: &[Vec], -) -> io::Result, String>> { +) -> io::Result, FilterAttributeValue>> { let expected = expected_paths .iter() .map(Vec::as_slice) @@ -329,10 +640,13 @@ fn parse_filter_attributes( } let driver = std::str::from_utf8(record[2]) .map_err(|_| invalid_filter_output("non-UTF-8 Git filter attribute value"))?; - if attributes - .insert(record[0].to_vec(), driver.to_string()) - .is_some() - { + let value = match driver { + "set" | "unset" | "unspecified" => { + FilterAttributeValue::AmbiguousSentinel(driver.to_string()) + } + _ => FilterAttributeValue::Driver(driver.to_string()), + }; + if attributes.insert(record[0].to_vec(), value).is_some() { return Err(invalid_filter_output( "duplicate Git filter attribute record", )); diff --git a/codex-rs/git-utils/src/safe_git_tests.rs b/codex-rs/git-utils/src/safe_git_tests.rs index 4a10d44f97..39ffd242bf 100644 --- a/codex-rs/git-utils/src/safe_git_tests.rs +++ b/codex-rs/git-utils/src/safe_git_tests.rs @@ -6,6 +6,7 @@ use crate::git_config::GitConfigScope; use crate::patch_paths::stage_paths; use pretty_assertions::assert_eq; use std::collections::BTreeMap; +use std::collections::BTreeSet; #[cfg(unix)] use std::ffi::OsStr; use std::path::Path; @@ -90,6 +91,55 @@ fn git_add_filter_policy_rejects_clean_and_process_but_allows_smudge_only() { } } +#[test] +fn filter_snapshot_retains_required_without_treating_it_as_executable() { + let mut entries = filter_entries( + GitConfigScope::Local, + Path::new("config"), + "filter.demo.smudge", + "smudge-command", + ); + entries.extend(filter_entries( + GitConfigScope::Command, + Path::new("command line:"), + "filter.demo.required", + "true", + )); + assert_eq!( + executable_filter_drivers(&entries).expect("executable drivers"), + BTreeSet::from(["demo".to_string()]) + ); + let neutralization = GitFilterNeutralization { + git_config_args: Vec::new(), + _config_dir: None, + filter_config: entries, + }; + assert_eq!( + neutralization.filter_value("demo", "required"), + Some("true") + ); + + let required_only = filter_entries( + GitConfigScope::Local, + Path::new("config"), + "filter.demo.required", + "true", + ); + assert!( + executable_filter_drivers(&required_only) + .expect("required-only config") + .is_empty() + ); +} + +#[test] +fn filter_driver_parser_accepts_empty_name() { + assert_eq!( + filter_driver_name("filter..clean").expect("empty filter name"), + "" + ); +} + #[test] fn filter_attribute_parser_rejects_malformed_or_unexpected_records() { let paths = vec![b"a.txt".to_vec(), b"b.txt".to_vec()]; @@ -97,12 +147,14 @@ fn filter_attribute_parser_rejects_malformed_or_unexpected_records() { parse_filter_attributes(b"a.txt\0filter\0unspecified\0b.txt\0filter\0lfs\0", &paths) .expect("parse attributes"); assert_eq!( - parsed.get(b"a.txt".as_slice()).map(String::as_str), - Some("unspecified") + parsed.get(b"a.txt".as_slice()), + Some(&FilterAttributeValue::AmbiguousSentinel( + "unspecified".to_string() + )) ); assert_eq!( - parsed.get(b"b.txt".as_slice()).map(String::as_str), - Some("lfs") + parsed.get(b"b.txt".as_slice()), + Some(&FilterAttributeValue::Driver("lfs".to_string())) ); for output in [