From bed78fad06fb0436557e14755904668104e6dbfc Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Wed, 1 Jul 2026 15:58:52 -0700 Subject: [PATCH] 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!(