diff --git a/codex-rs/git-utils/src/apply.rs b/codex-rs/git-utils/src/apply.rs index 20ebd63456..98a1f7156b 100644 --- a/codex-rs/git-utils/src/apply.rs +++ b/codex-rs/git-utils/src/apply.rs @@ -14,8 +14,9 @@ use std::path::PathBuf; use crate::FsmonitorOverride; use crate::safe_git::DISABLED_HOOKS_PATH; -use crate::safe_git::GitConfigOverride; -use crate::safe_git::configured_executable_git_config_overrides; +use crate::safe_git::EXECUTABLE_FILTER_CONFIG_PATTERN; +use crate::safe_git::EXECUTABLE_PATCH_CONFIG_PATTERN; +use crate::safe_git::ensure_no_executable_git_config; /// Parameters for invoking [`apply_git_patch`]. #[derive(Debug, Clone)] @@ -44,6 +45,7 @@ pub struct ApplyGitResult { /// leaves the working tree untouched while still parsing the command output for diagnostics. pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { let git_root = resolve_git_root(&req.cwd)?; + ensure_no_executable_git_config(&git_root, EXECUTABLE_PATCH_CONFIG_PATTERN)?; // Write unified diff into a temporary file let (tmpdir, patch_path) = write_temp_patch(&req.diff)?; @@ -54,8 +56,6 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { // Stage WT paths first to avoid index mismatch on revert. stage_paths(&git_root, &req.diff)?; } - let executable_git_config_overrides = configured_executable_git_config_overrides(&git_root)?; - // Build git args let mut args: Vec = vec!["apply".into(), "--3way".into()]; if req.revert { @@ -86,12 +86,7 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { } check_args.push(patch_path.to_string_lossy().to_string()); let rendered = render_command_for_log(&git_root, &cfg_parts, &check_args); - let (c_code, c_out, c_err) = run_git( - &git_root, - &cfg_parts, - &executable_git_config_overrides, - &check_args, - )?; + let (c_code, c_out, c_err) = run_git(&git_root, &cfg_parts, &check_args)?; let (mut applied_paths, mut skipped_paths, mut conflicted_paths) = parse_git_apply_output(&c_out, &c_err); applied_paths.sort(); @@ -112,12 +107,7 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { } let cmd_for_log = render_command_for_log(&git_root, &cfg_parts, &args); - let (code, stdout, stderr) = run_git( - &git_root, - &cfg_parts, - &executable_git_config_overrides, - &args, - )?; + let (code, stdout, stderr) = run_git(&git_root, &cfg_parts, &args)?; let (mut applied_paths, mut skipped_paths, mut conflicted_paths) = parse_git_apply_output(&stdout, &stderr); @@ -164,12 +154,7 @@ fn write_temp_patch(diff: &str) -> io::Result<(tempfile::TempDir, PathBuf)> { Ok((dir, path)) } -fn run_git( - cwd: &Path, - git_cfg: &[String], - executable_git_config_overrides: &[GitConfigOverride], - args: &[String], -) -> io::Result<(i32, String, String)> { +fn run_git(cwd: &Path, git_cfg: &[String], args: &[String]) -> io::Result<(i32, String, String)> { let mut cmd = std::process::Command::new("git"); for p in git_cfg { cmd.arg(p); @@ -177,7 +162,6 @@ fn run_git( for a in args { cmd.arg(a); } - add_executable_git_config_overrides(&mut cmd, executable_git_config_overrides); let out = cmd.current_dir(cwd).output()?; let code = out.status.code().unwrap_or(-1); let stdout = String::from_utf8_lossy(&out.stdout).into_owned(); @@ -194,21 +178,6 @@ fn safe_git_config_parts() -> Vec { ] } -fn add_executable_git_config_overrides( - command: &mut std::process::Command, - config_overrides: &[GitConfigOverride], -) { - if config_overrides.is_empty() { - return; - } - command.env("GIT_CONFIG_COUNT", config_overrides.len().to_string()); - for (index, (key, value)) in config_overrides.iter().enumerate() { - command - .env(format!("GIT_CONFIG_KEY_{index}"), key) - .env(format!("GIT_CONFIG_VALUE_{index}"), value); - } -} - fn quote_shell(s: &str) -> String { let simple = s .chars() @@ -364,6 +333,7 @@ fn unescape_c_string(input: &str) -> String { /// Stage only the files that actually exist on disk for the given diff. pub fn stage_paths(git_root: &Path, diff: &str) -> io::Result<()> { + ensure_no_executable_git_config(git_root, EXECUTABLE_FILTER_CONFIG_PATTERN)?; let paths = extract_paths_from_patch(diff); let mut existing: Vec = Vec::new(); for p in paths { @@ -378,13 +348,7 @@ pub fn stage_paths(git_root: &Path, diff: &str) -> io::Result<()> { let mut args = vec!["add".to_string(), "--".to_string()]; args.extend(existing); let config_parts = safe_git_config_parts(); - let executable_git_config_overrides = configured_executable_git_config_overrides(git_root)?; - let (_code, _, _) = run_git( - git_root, - &config_parts, - &executable_git_config_overrides, - &args, - )?; + let (_code, _, _) = run_git(git_root, &config_parts, &args)?; // We do not hard fail staging; best-effort is OK. Return Ok even on non-zero. Ok(()) } @@ -708,6 +672,28 @@ mod tests { code == 0 } + fn configure_merge_driver(root: &Path, tracked_path: &str) { + std::fs::write( + root.join(".gitattributes"), + format!("{tracked_path} merge=codex-test\n"), + ) + .expect("write attributes"); + let (add_code, _, add_err) = run(root, &["git", "add", ".gitattributes"]); + assert_eq!(add_code, 0, "add attributes: {add_err}"); + let (commit_code, _, commit_err) = run(root, &["git", "commit", "-m", "attributes"]); + assert_eq!(commit_code, 0, "commit attributes: {commit_err}"); + let (config_code, _, config_err) = run( + root, + &[ + "git", + "config", + "merge.codex-test.driver", + "git config codex.mergeran true && false", + ], + ); + assert_eq!(config_code, 0, "configure merge driver: {config_err}"); + } + #[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"; @@ -921,7 +907,7 @@ diff --git a/ghost.txt b/ghost.txt\n--- a/ghost.txt\n+++ b/ghost.txt\n@@ -1,1 +1 } #[test] - fn apply_ignores_configured_clean_filter() { + fn apply_rejects_configured_clean_filter_without_running_it() { let _g = env_lock().lock().unwrap(); let repo = init_repo(); let root = repo.path(); @@ -939,23 +925,39 @@ diff --git a/ghost.txt b/ghost.txt\n--- a/ghost.txt\n+++ b/ghost.txt\n@@ -1,1 +1 revert: false, preflight: true, }; - let preflight = apply_git_patch(&preflight_req).expect("preflight apply"); - assert_eq!(preflight.exit_code, 0, "preflight apply succeeded"); + let error = apply_git_patch(&preflight_req).expect_err("reject configured filter"); + assert_eq!(error.kind(), io::ErrorKind::Unsupported); assert!(!configured_filter_ran(root)); + assert_eq!(read_file_normalized(&root.join("file.txt")), "orig\n"); - let apply_req = ApplyGitRequest { + let stage_error = stage_paths(root, diff).expect_err("reject configured filter"); + assert_eq!(stage_error.kind(), io::ErrorKind::Unsupported); + assert!(!configured_filter_ran(root)); + } + + #[test] + fn apply_rejects_configured_merge_driver_without_running_it() { + let _g = env_lock().lock().unwrap(); + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join("file.txt"), "orig\n").expect("write file"); + let (add_code, _, add_err) = run(root, &["git", "add", "file.txt"]); + assert_eq!(add_code, 0, "add file: {add_err}"); + let (commit_code, _, commit_err) = run(root, &["git", "commit", "-m", "seed"]); + assert_eq!(commit_code, 0, "commit file: {commit_err}"); + configure_merge_driver(root, "file.txt"); + + let diff = "diff --git a/file.txt b/file.txt\n--- a/file.txt\n+++ b/file.txt\n@@ -1,1 +1,1 @@\n-orig\n+next\n"; + let request = ApplyGitRequest { cwd: root.to_path_buf(), diff: diff.to_string(), revert: false, preflight: false, }; - let applied = apply_git_patch(&apply_req).expect("apply"); - assert_eq!( - applied.exit_code, 0, - "apply succeeded\nstdout:\n{}\nstderr:\n{}", - applied.stdout, applied.stderr - ); - assert!(!configured_filter_ran(root)); - assert_eq!(read_file_normalized(&root.join("file.txt")), "next\n"); + let error = apply_git_patch(&request).expect_err("reject configured merge driver"); + assert_eq!(error.kind(), io::ErrorKind::Unsupported); + let (marker_code, _, _) = run(root, &["git", "config", "--get", "codex.mergeran"]); + assert_ne!(marker_code, 0, "merge driver must not run"); + assert_eq!(read_file_normalized(&root.join("file.txt")), "orig\n"); } } diff --git a/codex-rs/git-utils/src/info.rs b/codex-rs/git-utils/src/info.rs index 1b6fced0e6..9db3e8dc2e 100644 --- a/codex-rs/git-utils/src/info.rs +++ b/codex-rs/git-utils/src/info.rs @@ -18,9 +18,8 @@ use ts_rs::TS; use crate::GitSha; use crate::safe_git::DISABLED_HOOKS_PATH; -use crate::safe_git::EXECUTABLE_GIT_CONFIG_PATTERN; -use crate::safe_git::GitConfigOverride; -use crate::safe_git::executable_git_config_overrides_from_output; +use crate::safe_git::EXECUTABLE_FILTER_CONFIG_PATTERN; +use crate::safe_git::config_output_has_entries; /// Return `true` if the project folder specified by the `Config` is inside a /// Git repository. @@ -285,6 +284,9 @@ fn trim_git_suffix(value: &str) -> &str { pub async fn get_has_changes(cwd: &Path) -> Option { let git = Path::new("git"); + if has_configured_executable_filters_from(git, cwd).await? { + return None; + } let fsmonitor = detect_local_fsmonitor_override(git, cwd).await; let output = run_git_command_with_timeout_from(git, &["status", "--porcelain"], cwd, fsmonitor).await?; @@ -438,20 +440,17 @@ async fn run_git_command_with_timeout_from( cwd: &Path, fsmonitor: crate::FsmonitorOverride, ) -> Option { - let executable_git_config_overrides = - configured_executable_git_config_overrides_from(git, cwd).await?; let disabled_hooks = format!("core.hooksPath={DISABLED_HOOKS_PATH}"); let mut command = Command::new(git); command .env("GIT_OPTIONAL_LOCKS", "0") // Keep internal Git commands independent of repository-selected hooks - // and executable Git helpers while preserving built-in fsmonitor acceleration. + // while preserving built-in fsmonitor acceleration. .args(["-c", &disabled_hooks]) .args(["-c", fsmonitor.git_config_arg()]) + .args(args) .current_dir(cwd) .kill_on_drop(true); - add_executable_git_config_overrides(&mut command, &executable_git_config_overrides); - command.args(args); let result = timeout(GIT_COMMAND_TIMEOUT, command.output()).await; match result { @@ -460,10 +459,7 @@ async fn run_git_command_with_timeout_from( } } -async fn configured_executable_git_config_overrides_from( - git: &Path, - cwd: &Path, -) -> Option> { +async fn has_configured_executable_filters_from(git: &Path, cwd: &Path) -> Option { let mut command = Command::new(git); command .args([ @@ -471,7 +467,7 @@ async fn configured_executable_git_config_overrides_from( "--null", "--name-only", "--get-regexp", - EXECUTABLE_GIT_CONFIG_PATTERN, + EXECUTABLE_FILTER_CONFIG_PATTERN, ]) .current_dir(cwd) .kill_on_drop(true); @@ -487,22 +483,7 @@ async fn configured_executable_git_config_overrides_from( return None; } - Some(executable_git_config_overrides_from_output(&output.stdout)) -} - -fn add_executable_git_config_overrides( - command: &mut Command, - config_overrides: &[GitConfigOverride], -) { - if config_overrides.is_empty() { - return; - } - command.env("GIT_CONFIG_COUNT", config_overrides.len().to_string()); - for (index, (key, value)) in config_overrides.iter().enumerate() { - command - .env(format!("GIT_CONFIG_KEY_{index}"), key) - .env(format!("GIT_CONFIG_VALUE_{index}"), value); - } + Some(config_output_has_entries(&output.stdout)) } async fn get_git_remotes(cwd: &Path) -> Option> { @@ -783,6 +764,9 @@ async fn find_closest_sha(cwd: &Path, branches: &[String], remotes: &[String]) - async fn diff_against_sha(cwd: &Path, sha: &GitSha) -> Option { let git = Path::new("git"); + if has_configured_executable_filters_from(git, cwd).await? { + return None; + } let fsmonitor = detect_local_fsmonitor_override(git, cwd).await; let output = run_git_command_with_timeout_from( git, @@ -1070,17 +1054,17 @@ mod tests { } #[tokio::test] - async fn get_has_changes_ignores_configured_clean_filter() { + async fn get_has_changes_rejects_configured_clean_filter_without_running_it() { let temp_dir = tempfile::tempdir().expect("create temp dir"); let repo_path = create_test_git_repo(&temp_dir).await; configure_clean_filter(&repo_path, "test.txt").await; - assert_eq!(get_has_changes(&repo_path).await, Some(false)); + assert_eq!(get_has_changes(&repo_path).await, None); assert!(!configured_filter_ran(&repo_path).await); } #[tokio::test] - async fn git_diff_to_remote_ignores_configured_clean_filter() { + async fn git_diff_to_remote_rejects_configured_clean_filter_without_running_it() { let temp_dir = tempfile::tempdir().expect("create temp dir"); let repo_path = create_test_git_repo(&temp_dir).await; let remote_path = temp_dir.path().join("remote.git"); @@ -1110,10 +1094,7 @@ mod tests { configure_clean_filter(&repo_path, "test.txt").await; run_git(&repo_path, &["push", "origin", &branch]).await; - let state = git_diff_to_remote(&repo_path) - .await - .expect("collect working tree state"); - assert!(state.diff.is_empty()); + assert!(git_diff_to_remote(&repo_path).await.is_none()); assert!(!configured_filter_ran(&repo_path).await); } @@ -1167,9 +1148,6 @@ mod tests { "config --null --get core.fsmonitor".to_string(), "config --null --type=bool --fixed-value --get core.fsmonitor /tmp/fsmonitor-helper" .to_string(), - format!( - "config --null --name-only --get-regexp {EXECUTABLE_GIT_CONFIG_PATTERN}" - ), format!("-c {disabled_hooks} -c core.fsmonitor=false status --porcelain"), ] ); @@ -1257,7 +1235,6 @@ mod tests { vec![ "config --null --get core.fsmonitor".to_string(), "version --build-options".to_string(), - format!("config --null --name-only --get-regexp {EXECUTABLE_GIT_CONFIG_PATTERN}"), format!("-c {disabled_hooks} -c core.fsmonitor=true status --porcelain"), ] ); diff --git a/codex-rs/git-utils/src/operations.rs b/codex-rs/git-utils/src/operations.rs index d7aec12cf0..77e2d8f50d 100644 --- a/codex-rs/git-utils/src/operations.rs +++ b/codex-rs/git-utils/src/operations.rs @@ -6,8 +6,6 @@ use std::process::Command; use crate::GitToolingError; use crate::safe_git::DISABLED_HOOKS_PATH; -use crate::safe_git::GitConfigOverride; -use crate::safe_git::configured_executable_git_config_overrides; pub(crate) fn ensure_git_repository(path: &Path) -> Result<(), GitToolingError> { match run_git_for_stdout( @@ -110,7 +108,6 @@ where for arg in iterator { args_vec.push(OsString::from(arg.as_ref())); } - let executable_git_config_overrides = configured_executable_git_config_overrides(dir)?; let command_string = build_command_string(&args_vec); let mut command = Command::new("git"); command.current_dir(dir); @@ -119,7 +116,6 @@ where command.env(key, value); } } - add_executable_git_config_overrides(&mut command, &executable_git_config_overrides); command.args(&args_vec); let output = command.output()?; if !output.status.success() { @@ -136,21 +132,6 @@ where }) } -fn add_executable_git_config_overrides( - command: &mut Command, - config_overrides: &[GitConfigOverride], -) { - if config_overrides.is_empty() { - return; - } - command.env("GIT_CONFIG_COUNT", config_overrides.len().to_string()); - for (index, (key, value)) in config_overrides.iter().enumerate() { - command - .env(format!("GIT_CONFIG_KEY_{index}"), key) - .env(format!("GIT_CONFIG_VALUE_{index}"), value); - } -} - fn build_command_string(args: &[OsString]) -> String { if args.is_empty() { return "git".to_string(); diff --git a/codex-rs/git-utils/src/safe_git.rs b/codex-rs/git-utils/src/safe_git.rs index 6b8453fd94..a408659cd9 100644 --- a/codex-rs/git-utils/src/safe_git.rs +++ b/codex-rs/git-utils/src/safe_git.rs @@ -1,98 +1,50 @@ -use std::collections::BTreeSet; use std::io; use std::path::Path; use std::process::Command; pub(crate) const DISABLED_HOOKS_PATH: &str = if cfg!(windows) { "NUL" } else { "/dev/null" }; -pub(crate) const EXECUTABLE_GIT_CONFIG_PATTERN: &str = - r"^(filter\..*\.(clean|smudge|process|required)|merge\..*\.driver)$"; -pub(crate) type GitConfigOverride = (String, String); +pub(crate) const EXECUTABLE_FILTER_CONFIG_PATTERN: &str = r"^filter\..*\.(clean|smudge|process)$"; +pub(crate) const EXECUTABLE_PATCH_CONFIG_PATTERN: &str = + r"^(filter\..*\.(clean|smudge|process)|merge\..*\.driver)$"; -pub(crate) fn configured_executable_git_config_overrides( - cwd: &Path, -) -> io::Result> { +pub(crate) fn ensure_no_executable_git_config(cwd: &Path, pattern: &str) -> io::Result<()> { let output = Command::new("git") - .args([ - "config", - "--null", - "--name-only", - "--get-regexp", - EXECUTABLE_GIT_CONFIG_PATTERN, - ]) + .env("GIT_OPTIONAL_LOCKS", "0") + .args(["config", "--null", "--name-only", "--get-regexp", pattern]) .current_dir(cwd) .output()?; - if output + if !output .status .code() .is_some_and(|code| code == 0 || code == 1) { - return Ok(executable_git_config_overrides_from_output(&output.stdout)); + return Err(io::Error::other(format!( + "git config probe failed with status {}: {}", + output.status, + String::from_utf8_lossy(&output.stderr).trim() + ))); } - - Err(io::Error::other(format!( - "git config probe failed with status {}: {}", - output.status, - String::from_utf8_lossy(&output.stderr).trim() - ))) + if config_output_has_entries(&output.stdout) { + return Err(io::Error::new( + io::ErrorKind::Unsupported, + "refusing to run an internal Git worktree operation with executable Git helpers configured", + )); + } + Ok(()) } -pub(crate) fn executable_git_config_overrides_from_output(stdout: &[u8]) -> Vec { - let mut filter_drivers = BTreeSet::new(); - let mut merge_drivers = BTreeSet::new(); - - for key in stdout - .split(|byte| *byte == 0) - .filter(|key| !key.is_empty()) - .filter_map(|key| std::str::from_utf8(key).ok()) - { - if let Some(driver) = key - .strip_suffix(".clean") - .or_else(|| key.strip_suffix(".smudge")) - .or_else(|| key.strip_suffix(".process")) - .or_else(|| key.strip_suffix(".required")) - { - filter_drivers.insert(driver.to_string()); - } else if key.starts_with("merge.") && key.ends_with(".driver") { - merge_drivers.insert(key.to_string()); - } - } - - filter_drivers - .into_iter() - .flat_map(|driver| { - [ - (format!("{driver}.clean"), String::new()), - (format!("{driver}.smudge"), String::new()), - (format!("{driver}.process"), String::new()), - (format!("{driver}.required"), "false".to_string()), - ] - }) - .chain( - merge_drivers - .into_iter() - .map(|driver| (driver, String::new())), - ) - .collect() +pub(crate) fn config_output_has_entries(stdout: &[u8]) -> bool { + stdout.iter().any(|byte| *byte != 0) } #[cfg(test)] mod tests { use super::*; - use pretty_assertions::assert_eq; #[test] - fn executable_git_config_overrides_clear_filters_and_merge_drivers() { - let output = b"filter.x=y.clean\0filter.x=y.required\0merge.pwn.driver\0"; - - assert_eq!( - executable_git_config_overrides_from_output(output), - vec![ - ("filter.x=y.clean".to_string(), String::new()), - ("filter.x=y.smudge".to_string(), String::new()), - ("filter.x=y.process".to_string(), String::new()), - ("filter.x=y.required".to_string(), "false".to_string()), - ("merge.pwn.driver".to_string(), String::new()), - ] - ); + fn detects_nonempty_null_delimited_config_output() { + assert!(!config_output_has_entries(b"")); + assert!(!config_output_has_entries(b"\0")); + assert!(config_output_has_entries(b"filter.example.clean\0")); } }