From 5b1fe8c0f2077b7933e75f79204e50fde885c672 Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Wed, 1 Jul 2026 15:17:42 -0700 Subject: [PATCH 1/4] git-utils: fix selected filter edge cases --- codex-rs/git-utils/src/apply.rs | 6 +- codex-rs/git-utils/src/apply_filter_tests.rs | 227 +++++++++++++++++++ codex-rs/git-utils/src/patch_paths.rs | 6 +- codex-rs/git-utils/src/safe_git.rs | 172 ++++++++++++-- codex-rs/git-utils/src/safe_git_tests.rs | 18 +- 5 files changed, 407 insertions(+), 22 deletions(-) create mode 100644 codex-rs/git-utils/src/apply_filter_tests.rs diff --git a/codex-rs/git-utils/src/apply.rs b/codex-rs/git-utils/src/apply.rs index 4dd850b435..effc46d217 100644 --- a/codex-rs/git-utils/src/apply.rs +++ b/codex-rs/git-utils/src/apply.rs @@ -59,7 +59,7 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { 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 @@ -255,6 +255,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::*; 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..f41f58e3eb --- /dev/null +++ b/codex-rs/git-utils/src/apply_filter_tests.rs @@ -0,0 +1,227 @@ +use super::*; +use pretty_assertions::assert_eq; +use std::ffi::OsStr; +use std::path::Path; + +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"; + +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) { + run_success( + root, + &["config", &format!("filter.{driver}.clean"), FILTER_COMMAND], + ); +} + +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)); +} diff --git a/codex-rs/git-utils/src/patch_paths.rs b/codex-rs/git-utils/src/patch_paths.rs index d6889fce89..7d606b94a5 100644 --- a/codex-rs/git-utils/src/patch_paths.rs +++ b/codex-rs/git-utils/src/patch_paths.rs @@ -230,13 +230,14 @@ 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, &[]) } 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(); @@ -260,7 +261,8 @@ pub(crate) fn stage_effective_paths( "--".to_string(), ]; args.extend(existing); - let config_parts = safe_git_config_parts(); + let mut config_parts = git_config_args.to_vec(); + config_parts.extend(safe_git_config_parts()); let (_code, _, _) = run_git(git, git_root, &config_parts, &args)?; // We do not hard fail staging; best-effort is OK. Return Ok even on non-zero. Ok(()) diff --git a/codex-rs/git-utils/src/safe_git.rs b/codex-rs/git-utils/src/safe_git.rs index 4d28c631c3..84172da83d 100644 --- a/codex-rs/git-utils/src/safe_git.rs +++ b/codex-rs/git-utils/src/safe_git.rs @@ -15,6 +15,12 @@ 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)$"; +#[derive(Debug, Clone, PartialEq, Eq)] +enum FilterAttributeValue { + Driver(String), + AmbiguousSentinel(String), +} + const ISOLATED_GIT_ENVIRONMENT: [&str; 11] = [ "GIT_DIR", "GIT_WORK_TREE", @@ -49,14 +55,16 @@ pub(crate) fn ensure_no_selected_executable_git_filters( git_config_args: &[String], ) -> io::Result<()> { let entries = read_filter_config(git, cwd, git_config_args)?; - if !entries.values().any(|entry| !entry.value.is_empty()) { + let executable_drivers = executable_filter_drivers(&entries)?; + if executable_drivers.is_empty() { return Ok(()); } let paths = paths .iter() .map(|path| path.as_bytes().to_vec()) .collect::>(); - let attributes = read_filter_attributes(git, cwd, &paths, git_config_args)?; + let attributes = + read_filter_attributes(git, cwd, &paths, git_config_args, &executable_drivers)?; if let Some((driver, path)) = selected_executable_filter(&entries, &attributes)? { return Err(io::Error::new( io::ErrorKind::Unsupported, @@ -147,6 +155,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()); @@ -179,13 +188,149 @@ 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( entries: &BTreeMap, attributes: &BTreeMap, String>, ) -> io::Result)>> { + let executable_drivers = executable_filter_drivers(entries)?; + for (path, driver) in attributes { + if executable_drivers.contains(driver) { + return Ok(Some((driver.clone(), path.clone()))); + } + } + Ok(None) +} + +fn executable_filter_drivers( + entries: &BTreeMap, +) -> io::Result> { let mut executable_drivers = BTreeSet::new(); for entry in entries.values() { let driver = filter_driver_name(&entry.key)?; @@ -193,12 +338,7 @@ fn selected_executable_filter( executable_drivers.insert(driver); } } - for (path, driver) in attributes { - if executable_drivers.contains(driver) { - return Ok(Some((driver.clone(), path.clone()))); - } - } - Ok(None) + Ok(executable_drivers) } fn filter_driver_name(key: &str) -> io::Result { @@ -208,7 +348,6 @@ fn filter_driver_name(key: &str) -> io::Result { let driver = [".clean", ".smudge", ".process"] .into_iter() .find_map(|suffix| remainder.strip_suffix(suffix)) - .filter(|driver| !driver.is_empty()) .ok_or_else(|| invalid_filter_output("malformed filter config key"))?; Ok(driver.to_string()) } @@ -230,7 +369,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) @@ -258,10 +397,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 7f5dca8a83..3fa123644d 100644 --- a/codex-rs/git-utils/src/safe_git_tests.rs +++ b/codex-rs/git-utils/src/safe_git_tests.rs @@ -57,6 +57,14 @@ fn selected_filter_policy_allows_effective_empty_value() { ); } +#[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()]; @@ -64,12 +72,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 [ From 9ec1720f7c1703c429c6590949396504964994e7 Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Wed, 1 Jul 2026 15:22:59 -0700 Subject: [PATCH 2/4] git-utils: reject Git from enclosing repositories --- codex-rs/git-utils/src/apply.rs | 78 +++++++++ codex-rs/git-utils/src/git_command.rs | 109 +++++++++++-- codex-rs/git-utils/src/git_command_tests.rs | 168 +++++++++++++++++++- codex-rs/git-utils/src/operations.rs | 73 +-------- codex-rs/git-utils/src/operations_tests.rs | 66 ++++++++ 5 files changed, 410 insertions(+), 84 deletions(-) create mode 100644 codex-rs/git-utils/src/operations_tests.rs diff --git a/codex-rs/git-utils/src/apply.rs b/codex-rs/git-utils/src/apply.rs index 12c999e9c8..2e7120105e 100644 --- a/codex-rs/git-utils/src/apply.rs +++ b/codex-rs/git-utils/src/apply.rs @@ -722,6 +722,13 @@ mod tests { .replace("\r\n", "\n") } + #[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 extract_paths_handles_quoted_headers() { let diff = "diff --git \"a/hello world.txt\" \"b/hello world.txt\"\nnew file mode 100644\n--- /dev/null\n+++ b/hello world.txt\n@@ -0,0 +1 @@\n+hi\n"; @@ -825,6 +832,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/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..15667106f9 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()); @@ -39,7 +64,7 @@ fn write_git_candidate(directory: &Path) { fn locations_for_root(root: &Path) -> UntrustedGitLocations { UntrustedGitLocations { roots: vec![std::fs::canonicalize(root).expect("canonical root")], - common_dir: None, + common_dirs: Vec::new(), } } @@ -107,6 +132,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 +320,23 @@ 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); + let traversing_path = locations.roots[0].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) + )); +} + #[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"); +} From b262c52803d45ab7c927f5020b3f924b593b476f Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Wed, 1 Jul 2026 15:50:37 -0700 Subject: [PATCH 3/4] git-utils: mirror resolver roots in traversal test --- codex-rs/git-utils/src/git_command_tests.rs | 24 +++++++++++++++------ 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/codex-rs/git-utils/src/git_command_tests.rs b/codex-rs/git-utils/src/git_command_tests.rs index 15667106f9..77807ba8d4 100644 --- a/codex-rs/git-utils/src/git_command_tests.rs +++ b/codex-rs/git-utils/src/git_command_tests.rs @@ -62,8 +62,13 @@ 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")], + roots, common_dirs: Vec::new(), } } @@ -329,12 +334,17 @@ fn resolver_rejects_parent_traversal_spelled_through_repository() { write_git_candidate(&trusted_bin); let locations = locations_for_root(&repo); - let traversing_path = locations.roots[0].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) - )); + 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)] From bed78fad06fb0436557e14755904668104e6dbfc Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Wed, 1 Jul 2026 15:58:52 -0700 Subject: [PATCH 4/4] git-utils: neutralize off-path filter helpers --- codex-rs/git-utils/src/apply.rs | 7 +- codex-rs/git-utils/src/apply_filter_tests.rs | 254 ++++++++++++++++++- codex-rs/git-utils/src/patch_paths.rs | 6 +- codex-rs/git-utils/src/safe_git.rs | 98 ++++++- codex-rs/git-utils/src/safe_git_tests.rs | 42 +++ 5 files changed, 395 insertions(+), 12 deletions(-) diff --git a/codex-rs/git-utils/src/apply.rs b/codex-rs/git-utils/src/apply.rs index 49909adf47..d6118c27c0 100644 --- a/codex-rs/git-utils/src/apply.rs +++ b/codex-rs/git-utils/src/apply.rs @@ -55,7 +55,10 @@ 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. @@ -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 diff --git a/codex-rs/git-utils/src/apply_filter_tests.rs b/codex-rs/git-utils/src/apply_filter_tests.rs index f41f58e3eb..8536f9617c 100644 --- a/codex-rs/git-utils/src/apply_filter_tests.rs +++ b/codex-rs/git-utils/src/apply_filter_tests.rs @@ -1,11 +1,24 @@ 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"); @@ -40,9 +53,116 @@ fn init_repo() -> tempfile::TempDir { } 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}.clean"), FILTER_COMMAND], + &["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}" ); } @@ -225,3 +345,135 @@ fn apply_allows_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/patch_paths.rs b/codex-rs/git-utils/src/patch_paths.rs index 7d606b94a5..f894f68e8d 100644 --- a/codex-rs/git-utils/src/patch_paths.rs +++ b/codex-rs/git-utils/src/patch_paths.rs @@ -230,7 +230,7 @@ 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( @@ -261,9 +261,7 @@ pub(crate) fn stage_effective_paths( "--".to_string(), ]; args.extend(existing); - let mut config_parts = git_config_args.to_vec(); - config_parts.extend(safe_git_config_parts()); - let (_code, _, _) = run_git(git, git_root, &config_parts, &args)?; + let (_code, _, _) = run_git(git, git_root, git_config_args, &args)?; // We do not hard fail staging; best-effort is OK. Return Ok even on non-zero. Ok(()) } diff --git a/codex-rs/git-utils/src/safe_git.rs b/codex-rs/git-utils/src/safe_git.rs index 84172da83d..bb362f9e34 100644 --- a/codex-rs/git-utils/src/safe_git.rs +++ b/codex-rs/git-utils/src/safe_git.rs @@ -13,7 +13,8 @@ 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 { @@ -21,6 +22,24 @@ enum FilterAttributeValue { 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> { + self.filter_config + .get(&format!("filter.{driver}.{name}")) + .map(|entry| entry.value.as_str()) + } +} + const ISOLATED_GIT_ENVIRONMENT: [&str; 11] = [ "GIT_DIR", "GIT_WORK_TREE", @@ -53,11 +72,15 @@ pub(crate) fn ensure_no_selected_executable_git_filters( cwd: &Path, paths: &[String], git_config_args: &[String], -) -> io::Result<()> { +) -> io::Result { let entries = read_filter_config(git, cwd, git_config_args)?; let executable_drivers = executable_filter_drivers(&entries)?; if executable_drivers.is_empty() { - return Ok(()); + return Ok(GitFilterNeutralization { + git_config_args: Vec::new(), + _config_dir: None, + filter_config: entries, + }); } let paths = paths .iter() @@ -74,7 +97,71 @@ pub(crate) fn ensure_no_selected_executable_git_filters( ), )); } - Ok(()) + executable_filter_guard(git, cwd, entries, &executable_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(()) + } } fn read_filter_config( @@ -333,6 +420,9 @@ fn executable_filter_drivers( ) -> io::Result> { let mut executable_drivers = BTreeSet::new(); for entry in entries.values() { + if entry.key.ends_with(".required") { + continue; + } let driver = filter_driver_name(&entry.key)?; if !entry.value.is_empty() { executable_drivers.insert(driver); diff --git a/codex-rs/git-utils/src/safe_git_tests.rs b/codex-rs/git-utils/src/safe_git_tests.rs index 3fa123644d..12fcf008f1 100644 --- a/codex-rs/git-utils/src/safe_git_tests.rs +++ b/codex-rs/git-utils/src/safe_git_tests.rs @@ -2,6 +2,7 @@ use super::*; use crate::git_config::GitConfigScope; use pretty_assertions::assert_eq; use std::collections::BTreeMap; +use std::collections::BTreeSet; use std::path::Path; #[test] @@ -57,6 +58,47 @@ fn selected_filter_policy_allows_effective_empty_value() { ); } +#[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!(