From 2ca4f55c0d8080aedb02aed0ffba9455f4251c4d Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Wed, 1 Jul 2026 07:45:11 -0700 Subject: [PATCH 1/6] Bind Git worktree helpers to a trusted executable --- codex-rs/git-utils/src/apply.rs | 223 +++++++++++++++++--- codex-rs/git-utils/src/errors.rs | 8 + codex-rs/git-utils/src/git_command.rs | 207 ++++++++++++++++++ codex-rs/git-utils/src/git_command_tests.rs | 194 +++++++++++++++++ codex-rs/git-utils/src/git_config.rs | 31 +++ codex-rs/git-utils/src/git_config_tests.rs | 10 + codex-rs/git-utils/src/lib.rs | 3 + codex-rs/git-utils/src/operations.rs | 82 ++++++- codex-rs/git-utils/src/safe_git.rs | 30 +++ 9 files changed, 751 insertions(+), 37 deletions(-) create mode 100644 codex-rs/git-utils/src/git_command.rs create mode 100644 codex-rs/git-utils/src/git_command_tests.rs create mode 100644 codex-rs/git-utils/src/git_config.rs create mode 100644 codex-rs/git-utils/src/git_config_tests.rs create mode 100644 codex-rs/git-utils/src/safe_git.rs diff --git a/codex-rs/git-utils/src/apply.rs b/codex-rs/git-utils/src/apply.rs index 92237ad797b3..12c999e9c861 100644 --- a/codex-rs/git-utils/src/apply.rs +++ b/codex-rs/git-utils/src/apply.rs @@ -13,6 +13,12 @@ use std::io; use std::path::Path; use std::path::PathBuf; +use crate::FsmonitorOverride; +use crate::git_command::GitRunner; +use crate::safe_git::DISABLED_HOOKS_PATH; +#[cfg(test)] +use crate::safe_git::isolate_git_command_environment; + /// Parameters for invoking [`apply_git_patch`]. #[derive(Debug, Clone)] pub struct ApplyGitRequest { @@ -39,7 +45,9 @@ pub struct ApplyGitResult { /// When [`ApplyGitRequest::preflight`] is `true`, this behaves like `git apply --check` and /// leaves the working tree untouched while still parsing the command output for diagnostics. pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { - let git_root = resolve_git_root(&req.cwd)?; + let git = GitRunner::for_cwd_io(&req.cwd)?; + let mut cfg_parts = configured_git_config_parts(); + let git_root = resolve_git_root(&git, &req.cwd, &cfg_parts)?; // Write unified diff into a temporary file let (tmpdir, patch_path) = write_temp_patch(&req.diff)?; @@ -57,18 +65,7 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { args.push("-R".into()); } - // Optional: additional git config via env knob (defaults OFF) - let mut cfg_parts: Vec = Vec::new(); - if let Ok(cfg) = std::env::var("CODEX_APPLY_GIT_CFG") { - for pair in cfg.split(',') { - let p = pair.trim(); - if p.is_empty() || !p.contains('=') { - continue; - } - cfg_parts.push("-c".into()); - cfg_parts.push(p.to_string()); - } - } + cfg_parts.extend(safe_git_config_parts()); args.push(patch_path.to_string_lossy().to_string()); @@ -80,7 +77,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, &check_args)?; + let (c_code, c_out, c_err) = run_git(&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(); @@ -101,7 +98,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, &args)?; + let (code, stdout, stderr) = run_git(&git, &git_root, &cfg_parts, &args)?; let (mut applied_paths, mut skipped_paths, mut conflicted_paths) = parse_git_apply_output(&stdout, &stderr); @@ -123,12 +120,19 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { }) } -fn resolve_git_root(cwd: &Path) -> io::Result { - let out = local_git_command() +fn resolve_git_root( + git: &GitRunner, + cwd: &Path, + git_config_args: &[String], +) -> io::Result { + let requested_cwd = std::fs::canonicalize(cwd)?; + let mut command = git.command(); + command + .args(git_config_args) .arg("rev-parse") .arg("--show-toplevel") - .current_dir(cwd) - .output()?; + .current_dir(&requested_cwd); + let out = git.output(command)?; let code = out.status.code().unwrap_or(-1); if code != 0 { return Err(io::Error::other(format!( @@ -137,8 +141,47 @@ fn resolve_git_root(cwd: &Path) -> io::Result { String::from_utf8_lossy(&out.stderr) ))); } - let root = String::from_utf8_lossy(&out.stdout).trim().to_string(); - Ok(PathBuf::from(root)) + let reported_root = PathBuf::from(String::from_utf8_lossy(&out.stdout).trim()); + let root = std::fs::canonicalize(&reported_root)?; + let expected_root = crate::get_git_repo_root(&requested_cwd) + .ok_or_else(|| { + io::Error::new( + io::ErrorKind::PermissionDenied, + format!( + "refusing to apply a patch because Git resolved worktree {} without a .git marker above requested cwd {}", + root.display(), + requested_cwd.display() + ), + ) + }) + .and_then(std::fs::canonicalize)?; + if root != expected_root { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + format!( + "refusing to apply a patch because Git resolved worktree {} instead of expected worktree {} for requested cwd {}", + root.display(), + expected_root.display(), + requested_cwd.display() + ), + )); + } + Ok(root) +} + +fn configured_git_config_parts() -> Vec { + let mut cfg_parts = Vec::new(); + if let Ok(cfg) = std::env::var("CODEX_APPLY_GIT_CFG") { + for pair in cfg.split(',') { + let pair = pair.trim(); + if pair.is_empty() || !pair.contains('=') { + continue; + } + cfg_parts.push("-c".to_string()); + cfg_parts.push(pair.to_string()); + } + } + cfg_parts } fn write_temp_patch(diff: &str) -> io::Result<(tempfile::TempDir, PathBuf)> { @@ -148,25 +191,34 @@ fn write_temp_patch(diff: &str) -> io::Result<(tempfile::TempDir, PathBuf)> { Ok((dir, path)) } -fn run_git(cwd: &Path, git_cfg: &[String], args: &[String]) -> io::Result<(i32, String, String)> { - let mut cmd = local_git_command(); +fn run_git( + git: &GitRunner, + cwd: &Path, + git_cfg: &[String], + args: &[String], +) -> io::Result<(i32, String, String)> { + let mut cmd = git.command(); for p in git_cfg { cmd.arg(p); } for a in args { cmd.arg(a); } - let out = cmd.current_dir(cwd).output()?; + cmd.current_dir(cwd); + let out = git.output(cmd)?; let code = out.status.code().unwrap_or(-1); let stdout = String::from_utf8_lossy(&out.stdout).into_owned(); let stderr = String::from_utf8_lossy(&out.stderr).into_owned(); Ok((code, stdout, stderr)) } -fn local_git_command() -> std::process::Command { - let mut command = std::process::Command::new("git"); - command.envs(crate::local_only_git_env()); - command +fn safe_git_config_parts() -> Vec { + vec![ + "-c".to_string(), + format!("core.hooksPath={DISABLED_HOOKS_PATH}"), + "-c".to_string(), + FsmonitorOverride::Disabled.git_config_arg().to_string(), + ] } fn quote_shell(s: &str) -> String { @@ -324,6 +376,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<()> { + let git = GitRunner::for_cwd_io(git_root)?; let paths = extract_paths_from_patch(diff); let mut existing: Vec = Vec::new(); for p in paths { @@ -335,13 +388,15 @@ pub fn stage_paths(git_root: &Path, diff: &str) -> io::Result<()> { if existing.is_empty() { return Ok(()); } - let mut cmd = local_git_command(); + let mut cmd = git.command(); + cmd.args(safe_git_config_parts()); cmd.arg("add"); cmd.arg("--"); for p in &existing { cmd.arg(OsStr::new(p)); } - let out = cmd.current_dir(git_root).output()?; + cmd.current_dir(git_root); + let out = git.output(cmd)?; let _code = out.status.code().unwrap_or(-1); // We do not hard fail staging; best-effort is OK. Return Ok even on non-zero. Ok(()) @@ -605,6 +660,7 @@ mod transport_tests; #[cfg(test)] mod tests { use super::*; + use std::ffi::OsStr; use std::path::Path; use std::sync::Mutex; use std::sync::OnceLock; @@ -615,7 +671,9 @@ mod tests { } fn run(cwd: &Path, args: &[&str]) -> (i32, String, String) { - let out = std::process::Command::new(args[0]) + let mut command = std::process::Command::new(args[0]); + isolate_git_command_environment(&mut command); + let out = command .args(&args[1..]) .current_dir(cwd) .output() @@ -627,6 +685,27 @@ mod tests { ) } + 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_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) + ); + } + fn init_repo() -> tempfile::TempDir { let dir = tempfile::tempdir().expect("tempdir"); let root = dir.path(); @@ -678,10 +757,12 @@ mod tests { let _g = env_lock().lock().unwrap(); let repo = init_repo(); let root = repo.path(); + let nested_cwd = root.join("nested"); + std::fs::create_dir(&nested_cwd).expect("nested cwd"); let diff = "diff --git a/hello.txt b/hello.txt\nnew file mode 100644\n--- /dev/null\n+++ b/hello.txt\n@@ -0,0 +1,2 @@\n+hello\n+world\n"; let req = ApplyGitRequest { - cwd: root.to_path_buf(), + cwd: nested_cwd, diff: diff.to_string(), revert: false, preflight: false, @@ -692,6 +773,58 @@ mod tests { assert!(root.join("hello.txt").exists()); } + #[test] + fn apply_uses_cwd_repo_despite_inherited_repository_selectors() { + let _g = env_lock().lock().unwrap(); + if std::env::var_os("CODEX_GIT_UTILS_APPLY_ENV_CHILD").is_none() { + let alternate = init_repo(); + let alternate_root = alternate.path(); + std::fs::write(alternate_root.join("sentinel.txt"), "alternate\n") + .expect("write alternate sentinel"); + let (add_code, _, add_err) = run(alternate_root, &["git", "add", "sentinel.txt"]); + assert_eq!(add_code, 0, "add alternate sentinel: {add_err}"); + let (commit_code, _, commit_err) = + run(alternate_root, &["git", "commit", "-m", "alternate"]); + assert_eq!(commit_code, 0, "commit alternate sentinel: {commit_err}"); + + let alternate_git_dir = alternate_root.join(".git"); + let alternate_index = alternate_git_dir.join("index"); + run_isolated_test( + "apply::tests::apply_uses_cwd_repo_despite_inherited_repository_selectors", + &[ + ("GIT_DIR", alternate_git_dir.as_os_str()), + ("GIT_WORK_TREE", alternate_root.as_os_str()), + ("GIT_COMMON_DIR", alternate_git_dir.as_os_str()), + ("GIT_INDEX_FILE", alternate_index.as_os_str()), + ("GIT_PREFIX", OsStr::new("elsewhere/")), + ], + ); + assert_eq!( + read_file_normalized(&alternate_root.join("sentinel.txt")), + "alternate\n" + ); + return; + } + + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join("file.txt"), "old\n").expect("write target file"); + let (add_code, _, add_err) = run(root, &["git", "add", "file.txt"]); + assert_eq!(add_code, 0, "add target file: {add_err}"); + let (commit_code, _, commit_err) = run(root, &["git", "commit", "-m", "target"]); + assert_eq!(commit_code, 0, "commit target file: {commit_err}"); + + let result = apply_git_patch(&ApplyGitRequest { + cwd: root.to_path_buf(), + diff: "diff --git a/file.txt b/file.txt\n--- a/file.txt\n+++ b/file.txt\n@@ -1 +1 @@\n-old\n+new\n".to_string(), + revert: false, + preflight: false, + }) + .expect("apply in cwd-selected repository"); + assert_eq!(result.exit_code, 0); + assert_eq!(read_file_normalized(&root.join("file.txt")), "new\n"); + } + #[test] fn apply_modify_conflict() { let _g = env_lock().lock().unwrap(); @@ -854,4 +987,30 @@ diff --git a/ghost.txt b/ghost.txt\n--- a/ghost.txt\n+++ b/ghost.txt\n@@ -1,1 +1 "non-preflight path should not use --check" ); } + + #[test] + fn resolve_git_root_rejects_core_worktree_redirection() { + let temp = tempfile::tempdir().expect("tempdir"); + let attacker = temp.path().join("attacker"); + let victim = temp.path().join("victim"); + std::fs::create_dir_all(&attacker).expect("attacker"); + std::fs::create_dir_all(&victim).expect("victim"); + let (init_code, _, init_err) = run(&attacker, &["git", "init"]); + assert_eq!(init_code, 0, "init attacker repo: {init_err}"); + + for redirected_worktree in [&victim, temp.path()] { + let redirected_worktree = redirected_worktree.to_string_lossy(); + let (config_code, _, config_err) = run( + &attacker, + &["git", "config", "core.worktree", &redirected_worktree], + ); + assert_eq!(config_code, 0, "configure core.worktree: {config_err}"); + + let git = GitRunner::for_cwd_io(&attacker).expect("trusted Git"); + let error = + resolve_git_root(&git, &attacker, &[]).expect_err("reject redirected worktree"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + assert!(error.to_string().contains("instead of expected worktree")); + } + } } diff --git a/codex-rs/git-utils/src/errors.rs b/codex-rs/git-utils/src/errors.rs index 41f7b6df495c..0615a394216c 100644 --- a/codex-rs/git-utils/src/errors.rs +++ b/codex-rs/git-utils/src/errors.rs @@ -33,3 +33,11 @@ pub enum GitToolingError { #[error(transparent)] Io(#[from] std::io::Error), } + +#[derive(Clone, Debug, Error, PartialEq, Eq)] +pub(crate) enum GitReadError { + #[error("no trusted Git executable is available")] + NoTrustedGit, + #[error("{path:?} is not a Git repository")] + NotRepository { path: PathBuf }, +} diff --git a/codex-rs/git-utils/src/git_command.rs b/codex-rs/git-utils/src/git_command.rs new file mode 100644 index 000000000000..997578b839c3 --- /dev/null +++ b/codex-rs/git-utils/src/git_command.rs @@ -0,0 +1,207 @@ +use std::ffi::OsStr; +use std::io; +use std::path::Path; +use std::path::PathBuf; +use std::process::Command; + +use crate::errors::GitReadError; +use crate::git_config::path_is_within; +use crate::safe_git::isolate_git_command_environment; + +/// A Git executable outside the repository-controlled roots for one operation. +#[derive(Clone, Debug)] +pub(crate) struct GitRunner { + executable: PathBuf, +} + +struct UntrustedGitLocations { + roots: Vec, + common_dir: Option, +} + +impl GitRunner { + pub(crate) fn for_cwd(cwd: &Path) -> Result { + let locations = untrusted_git_locations_for_cwd(cwd)?; + let search_path = std::env::var_os("PATH").ok_or(GitReadError::NoTrustedGit)?; + Self::from_search_path(&locations, &search_path) + } + + pub(crate) fn for_cwd_io(cwd: &Path) -> io::Result { + Self::for_cwd(cwd).map_err(|error| io::Error::new(io::ErrorKind::NotFound, error)) + } + + pub(crate) fn command(&self) -> Command { + Command::new(&self.executable) + } + + pub(crate) fn output(&self, mut command: Command) -> io::Result { + isolate_git_command_environment(&mut command); + command.envs(crate::local_only_git_env()); + command.output() + } + + fn from_search_path( + untrusted: &UntrustedGitLocations, + search_path: &OsStr, + ) -> Result { + for directory in std::env::split_paths(search_path) { + if !directory.is_absolute() { + continue; + } + let candidate = directory.join(git_executable_name()); + if path_is_untrusted(&candidate, untrusted) { + continue; + } + let Ok(canonical_parent) = std::fs::canonicalize(&directory) else { + continue; + }; + if path_is_untrusted(&canonical_parent, untrusted) { + continue; + } + let Ok(canonical_candidate) = std::fs::canonicalize(&candidate) else { + continue; + }; + if path_is_untrusted(&canonical_candidate, untrusted) + || !is_native_executable_file(&canonical_candidate) + { + continue; + } + return Ok(Self { + // Preserve multicall spelling because argv[0] may select mode. + executable: candidate, + }); + } + Err(GitReadError::NoTrustedGit) + } +} + +fn untrusted_git_locations_for_cwd(cwd: &Path) -> Result { + 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 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, + Err(_) => return Err(GitReadError::NoTrustedGit), + }; + if let Some(common_dir) = &common_dir + && !path_is_within(common_dir, &worktree_root) + { + roots.push(common_dir.clone()); + } + Ok(UntrustedGitLocations { roots, common_dir }) +} + +fn path_is_untrusted(path: &Path, locations: &UntrustedGitLocations) -> bool { + if locations + .roots + .iter() + .any(|root| path_is_within(path, root)) + { + return true; + } + locations + .common_dir + .as_deref() + .is_some_and(|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 { + let path = if path.is_dir() { + path + } else { + path.parent().unwrap_or(path) + }; + for ancestor in path.ancestors() { + let dot_git = ancestor.join(".git"); + match std::fs::symlink_metadata(&dot_git) { + Ok(_) => match resolve_common_git_dir(&dot_git) { + Ok(common_dir) if paths_equal(&common_dir, expected_common_dir) => return true, + Ok(_) => {} + Err(()) => return true, + }, + Err(error) if error.kind() == io::ErrorKind::NotFound => {} + Err(_) => return true, + } + } + false +} + +fn paths_equal(left: &Path, right: &Path) -> bool { + path_is_within(left, right) && path_is_within(right, left) +} + +fn resolve_common_git_dir(dot_git: &Path) -> Result { + if dot_git.is_dir() { + return std::fs::canonicalize(dot_git).map_err(|_| ()); + } + let contents = std::fs::read_to_string(dot_git).map_err(|_| ())?; + let git_dir = contents + .trim() + .strip_prefix("gitdir:") + .map(str::trim) + .filter(|path| !path.is_empty()) + .ok_or(())?; + let git_dir = canonicalize_from(dot_git.parent().ok_or(())?, git_dir)?; + let commondir = git_dir.join("commondir"); + if commondir.is_file() { + let common_dir = std::fs::read_to_string(commondir).map_err(|_| ())?; + let common_dir = common_dir.trim(); + if common_dir.is_empty() { + return Err(()); + } + return canonicalize_from(&git_dir, common_dir); + } + if git_dir + .parent() + .is_some_and(|parent| parent.file_name() == Some(OsStr::new("worktrees"))) + { + return std::fs::canonicalize(git_dir.parent().and_then(Path::parent).ok_or(())?) + .map_err(|_| ()); + } + Ok(git_dir) +} + +fn canonicalize_from(base: &Path, path: &str) -> Result { + std::fs::canonicalize(base.join(path)).map_err(|_| ()) +} + +#[cfg(windows)] +fn git_executable_name() -> &'static str { + "git.exe" +} + +#[cfg(not(windows))] +fn git_executable_name() -> &'static str { + "git" +} + +#[cfg(unix)] +fn is_native_executable_file(path: &Path) -> bool { + use std::os::unix::fs::PermissionsExt; + + std::fs::metadata(path) + .is_ok_and(|metadata| metadata.is_file() && metadata.permissions().mode() & 0o111 != 0) +} + +#[cfg(windows)] +fn is_native_executable_file(path: &Path) -> bool { + path.extension() + .and_then(OsStr::to_str) + .is_some_and(|extension| extension.eq_ignore_ascii_case("exe")) + && std::fs::metadata(path).is_ok_and(|metadata| metadata.is_file()) +} + +#[cfg(not(any(unix, windows)))] +fn is_native_executable_file(path: &Path) -> bool { + std::fs::metadata(path).is_ok_and(|metadata| metadata.is_file()) +} + +#[cfg(test)] +#[path = "git_command_tests.rs"] +mod tests; diff --git a/codex-rs/git-utils/src/git_command_tests.rs b/codex-rs/git-utils/src/git_command_tests.rs new file mode 100644 index 000000000000..29debcd7496e --- /dev/null +++ b/codex-rs/git-utils/src/git_command_tests.rs @@ -0,0 +1,194 @@ +use super::*; +use pretty_assertions::assert_eq; +use std::process::Command; + +#[cfg(unix)] +fn write_executable(path: &Path, body: &str) { + use std::os::unix::fs::PermissionsExt; + + std::fs::write(path, body).expect("write executable"); + let mut permissions = std::fs::metadata(path) + .expect("executable metadata") + .permissions(); + permissions.set_mode(0o755); + std::fs::set_permissions(path, permissions).expect("set executable permissions"); +} + +fn run_git(cwd: &Path, args: &[&str]) { + let mut command = Command::new("git"); + isolate_git_command_environment(&mut command); + let status = command + .args(args) + .current_dir(cwd) + .status() + .expect("run real Git"); + assert!(status.success(), "git {args:?} failed"); +} + +fn write_git_candidate(directory: &Path) { + std::fs::create_dir_all(directory).expect("create candidate directory"); + let candidate = directory.join(git_executable_name()); + #[cfg(unix)] + write_executable(&candidate, "#!/bin/sh\nexit 0\n"); + #[cfg(windows)] + std::fs::write(candidate, b"MZ").expect("write native executable fixture"); + #[cfg(not(any(unix, windows)))] + std::fs::write(candidate, b"git fixture").expect("write executable fixture"); +} + +fn locations_for_root(root: &Path) -> UntrustedGitLocations { + UntrustedGitLocations { + roots: vec![std::fs::canonicalize(root).expect("canonical root")], + common_dir: None, + } +} + +fn path_text(path: &Path) -> &str { + path.to_str().expect("UTF-8 fixture path") +} + +fn selected_git(locations: &UntrustedGitLocations, directories: &[&Path]) -> PathBuf { + let search_path = std::env::join_paths(directories).expect("PATH"); + GitRunner::from_search_path(locations, &search_path) + .expect("trusted Git") + .executable +} + +#[cfg(unix)] +#[test] +fn resolver_skips_untrusted_path_entries_and_runs_external_candidate() { + let fixture = tempfile::tempdir().expect("fixture"); + let repo = fixture.path().join("repo"); + let repo_bin = repo.join("bin"); + let outside = fixture.path().join("outside"); + let trusted_bin = fixture.path().join("trusted-bin"); + std::fs::create_dir_all(&repo_bin).expect("repo bin"); + std::fs::create_dir_all(&outside).expect("outside bin"); + std::fs::create_dir_all(&trusted_bin).expect("trusted bin"); + write_executable(&repo_bin.join("git"), "#!/bin/sh\nexit 1\n"); + std::os::unix::fs::symlink(repo_bin.join("git"), outside.join("git")) + .expect("outside symlink into repository"); + write_executable(&trusted_bin.join("git"), "#!/bin/sh\nprintf 'trusted\\n'\n"); + + let path = std::env::join_paths([ + PathBuf::from("relative"), + repo_bin, + outside, + trusted_bin.clone(), + ]) + .expect("PATH"); + let locations = locations_for_root(&repo); + let runner = GitRunner::from_search_path(&locations, &path).expect("trusted Git"); + assert_eq!(runner.executable, trusted_bin.join("git")); + let output = runner.output(runner.command()).expect("run trusted Git"); + assert_eq!(output.stdout, b"trusted\n"); +} + +#[test] +fn linked_worktree_rejects_git_from_main_and_linked_worktrees() { + let fixture = tempfile::tempdir().expect("fixture"); + let main = fixture.path().join("main"); + let linked = fixture.path().join("linked"); + let git_dir = main.join(".git/worktrees/linked"); + let main_bin = main.join("bin"); + std::fs::create_dir_all(&git_dir).expect("linked Git directory"); + std::fs::create_dir_all(&linked).expect("linked worktree"); + std::fs::write( + linked.join(".git"), + format!("gitdir: {}\n", git_dir.display()), + ) + .expect("linked .git file"); + write_git_candidate(&main_bin); + + let locations = untrusted_git_locations_for_cwd(&linked).expect("untrusted locations"); + assert!(path_is_untrusted( + &main_bin.join(git_executable_name()), + &locations + )); +} + +#[test] +fn bare_backed_linked_worktree_allows_external_git_in_sibling_directory() { + let fixture = tempfile::tempdir().expect("fixture"); + let bare = fixture.path().join("repository.git"); + let linked = fixture.path().join("linked"); + let trusted_bin = fixture.path().join("trusted-bin"); + run_git(fixture.path(), &["init", "--bare", path_text(&bare)]); + run_git( + fixture.path(), + &[ + "--git-dir", + path_text(&bare), + "worktree", + "add", + "--orphan", + path_text(&linked), + ], + ); + write_git_candidate(&trusted_bin); + + let locations = untrusted_git_locations_for_cwd(&linked).expect("untrusted locations"); + assert_eq!( + selected_git(&locations, &[&trusted_bin]), + trusted_bin.join(git_executable_name()) + ); +} + +#[test] +fn separate_dot_git_dir_rejects_main_candidate_and_allows_unrelated_repo_candidate() { + let fixture = tempfile::tempdir().expect("fixture"); + let main = fixture.path().join("main"); + let common_dir = fixture.path().join("git-storage/.git"); + let linked = fixture.path().join("linked"); + let main_bin = main.join("bin"); + let unrelated = fixture.path().join("unrelated"); + let unrelated_bin = unrelated.join("bin"); + let malformed = fixture.path().join("malformed"); + let malformed_bin = malformed.join("bin"); + std::fs::create_dir_all(&main).expect("create main worktree"); + std::fs::create_dir_all(common_dir.parent().expect("common-dir parent")) + .expect("create common-dir parent"); + run_git( + fixture.path(), + &[ + "init", + "--separate-git-dir", + path_text(&common_dir), + path_text(&main), + ], + ); + run_git(&main, &["worktree", "add", "--orphan", path_text(&linked)]); + run_git(fixture.path(), &["init", path_text(&unrelated)]); + write_git_candidate(&main_bin); + write_git_candidate(&unrelated_bin); + write_git_candidate(&malformed_bin); + std::fs::write(malformed.join(".git"), "not a gitdir").expect("malformed marker"); + + let locations = untrusted_git_locations_for_cwd(&linked).expect("untrusted locations"); + assert_eq!( + selected_git(&locations, &[&main_bin, &malformed_bin, &unrelated_bin]), + unrelated_bin.join(git_executable_name()) + ); +} + +#[cfg(windows)] +#[test] +fn resolver_selects_native_git_exe_only() { + assert!(paths_equal( + Path::new(r"C:\Repo\.git"), + Path::new(r"c:\repo\.GIT") + )); + let fixture = tempfile::tempdir().expect("fixture"); + let repo = fixture.path().join("repo"); + let scripts = fixture.path().join("scripts"); + let native = fixture.path().join("native"); + std::fs::create_dir_all(&repo).expect("repo"); + std::fs::create_dir_all(&scripts).expect("scripts"); + std::fs::create_dir_all(&native).expect("native"); + std::fs::write(scripts.join("git.cmd"), "@exit /b 0\r\n").expect("script"); + std::fs::write(native.join("git.exe"), b"MZ").expect("native executable fixture"); + let locations = locations_for_root(&repo); + let path = std::env::join_paths([scripts, native.clone()]).expect("PATH"); + let runner = GitRunner::from_search_path(&locations, &path).expect("native Git"); + assert_eq!(runner.executable, native.join("git.exe")); +} diff --git a/codex-rs/git-utils/src/git_config.rs b/codex-rs/git-utils/src/git_config.rs new file mode 100644 index 000000000000..236ab89ef76d --- /dev/null +++ b/codex-rs/git-utils/src/git_config.rs @@ -0,0 +1,31 @@ +use std::path::Component; +use std::path::Path; + +pub(crate) fn path_is_within(path: &Path, root: &Path) -> bool { + let mut path_components = path.components(); + for root_component in root.components() { + let Some(path_component) = path_components.next() else { + return false; + }; + if !components_equal(path_component, root_component) { + return false; + } + } + true +} + +#[cfg(windows)] +fn components_equal(left: Component<'_>, right: Component<'_>) -> bool { + left.as_os_str() + .to_string_lossy() + .eq_ignore_ascii_case(&right.as_os_str().to_string_lossy()) +} + +#[cfg(not(windows))] +fn components_equal(left: Component<'_>, right: Component<'_>) -> bool { + left == right +} + +#[cfg(test)] +#[path = "git_config_tests.rs"] +mod tests; diff --git a/codex-rs/git-utils/src/git_config_tests.rs b/codex-rs/git-utils/src/git_config_tests.rs new file mode 100644 index 000000000000..3795caf2f875 --- /dev/null +++ b/codex-rs/git-utils/src/git_config_tests.rs @@ -0,0 +1,10 @@ +use super::*; + +#[test] +fn path_containment_uses_component_boundaries() { + let root = Path::new("/repo/root"); + assert!(path_is_within(Path::new("/repo/root"), root)); + assert!(path_is_within(Path::new("/repo/root/config"), root)); + assert!(!path_is_within(Path::new("/repo/rooted/config"), root)); + assert!(!path_is_within(Path::new("/repo"), root)); +} diff --git a/codex-rs/git-utils/src/lib.rs b/codex-rs/git-utils/src/lib.rs index d4817d87c8a7..550461074a69 100644 --- a/codex-rs/git-utils/src/lib.rs +++ b/codex-rs/git-utils/src/lib.rs @@ -3,10 +3,13 @@ mod baseline; mod branch; mod errors; mod fsmonitor; +mod git_command; +mod git_config; mod info; mod local_only; mod operations; mod platform; +mod safe_git; pub use apply::ApplyGitRequest; pub use apply::ApplyGitResult; diff --git a/codex-rs/git-utils/src/operations.rs b/codex-rs/git-utils/src/operations.rs index 00f44a6233e4..d58ff0beed0c 100644 --- a/codex-rs/git-utils/src/operations.rs +++ b/codex-rs/git-utils/src/operations.rs @@ -2,11 +2,14 @@ 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; - -const DISABLED_HOOKS_PATH: &str = if cfg!(windows) { "NUL" } else { "/dev/null" }; +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( @@ -110,16 +113,16 @@ where args_vec.push(OsString::from(arg.as_ref())); } let command_string = build_command_string(&args_vec); - let mut command = Command::new("git"); + let git = GitRunner::for_cwd_io(dir)?; + let mut command = git.command(); command.current_dir(dir); if let Some(envs) = env { for (key, value) in envs { command.env(key, value); } } - command.envs(crate::local_only_git_env()); command.args(&args_vec); - let output = command.output()?; + let output = git.output(command)?; if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); return Err(GitToolingError::GitCommand { @@ -150,3 +153,72 @@ struct GitRun { command: String, output: std::process::Output, } + +#[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"); + } +} diff --git a/codex-rs/git-utils/src/safe_git.rs b/codex-rs/git-utils/src/safe_git.rs new file mode 100644 index 000000000000..8d45a3355200 --- /dev/null +++ b/codex-rs/git-utils/src/safe_git.rs @@ -0,0 +1,30 @@ +use std::process::Command; + +pub(crate) const DISABLED_HOOKS_PATH: &str = if cfg!(windows) { "NUL" } else { "/dev/null" }; + +const ISOLATED_GIT_ENVIRONMENT: [&str; 11] = [ + "GIT_DIR", + "GIT_WORK_TREE", + "GIT_COMMON_DIR", + "GIT_INDEX_FILE", + "GIT_PREFIX", + "GIT_LITERAL_PATHSPECS", + "GIT_GLOB_PATHSPECS", + "GIT_NOGLOB_PATHSPECS", + "GIT_ICASE_PATHSPECS", + "GIT_EXEC_PATH", + // Legacy `GIT_CONFIG` affects `git config` but not ordinary worktree + // commands, so inheriting it can make a safety probe inspect different + // configuration than the command it guards. + "GIT_CONFIG", +]; + +/// Keep internal worktree operations bound to their explicit cwd and pathspec +/// semantics instead of inheriting repository, index, or pathspec selectors. +/// Deliberately leave Git config channels intact: callers may rely on normal +/// system/global configuration, and executable helpers are probed separately. +pub(crate) fn isolate_git_command_environment(command: &mut Command) { + for name in ISOLATED_GIT_ENVIRONMENT { + command.env_remove(name); + } +} From 9ec1720f7c1703c429c6590949396504964994e7 Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Wed, 1 Jul 2026 15:22:59 -0700 Subject: [PATCH 2/6] 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 12c999e9c861..2e7120105e84 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 997578b839c3..5edb011f3c18 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 29debcd7496e..15667106f931 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 d58ff0beed0c..1564b81a1945 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 000000000000..e775212e2675 --- /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/6] 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 15667106f931..77807ba8d4c5 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 f062f6916ee2118fe060cf376b98f6225b875163 Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Wed, 1 Jul 2026 16:20:33 -0700 Subject: [PATCH 4/6] git-utils: reject raw Windows PATH traversal --- codex-rs/git-utils/src/git_command.rs | 78 +++++++++++ codex-rs/git-utils/src/git_command_tests.rs | 142 +++++++++++++++++++- 2 files changed, 218 insertions(+), 2 deletions(-) diff --git a/codex-rs/git-utils/src/git_command.rs b/codex-rs/git-utils/src/git_command.rs index 5edb011f3c18..4491959515a8 100644 --- a/codex-rs/git-utils/src/git_command.rs +++ b/codex-rs/git-utils/src/git_command.rs @@ -1,7 +1,11 @@ use std::ffi::OsStr; use std::io; +#[cfg(windows)] +use std::path::Component; use std::path::Path; use std::path::PathBuf; +#[cfg(windows)] +use std::path::Prefix; use std::process::Command; use crate::errors::GitReadError; @@ -48,6 +52,13 @@ impl GitRunner { if !directory.is_absolute() { continue; } + // Check the PATH spelling before appending `git`. On Windows, + // PathBuf::push resolves `..` when its base uses a verbatim prefix, + // which would otherwise erase repository traversal before the + // first containment check. + if search_directory_is_untrusted(&directory, untrusted) { + continue; + } let candidate = directory.join(git_executable_name()); if path_is_untrusted(&candidate, untrusted) { continue; @@ -196,6 +207,73 @@ fn path_is_untrusted(path: &Path, locations: &UntrustedGitLocations) -> bool { .any(|common_dir| path_is_in_worktree_for_common_dir(path, common_dir)) } +fn search_directory_is_untrusted(directory: &Path, locations: &UntrustedGitLocations) -> bool { + #[cfg(windows)] + if windows_path_requires_fail_closed(directory) + || windows_path_has_untrusted_canonical_ancestor(directory, locations) + { + return true; + } + path_is_untrusted(directory, locations) +} + +#[cfg(windows)] +fn windows_path_requires_fail_closed(path: &Path) -> bool { + let mut components = path.components(); + let supported_namespace = match components.next() { + Some(Component::Prefix(prefix)) => match prefix.kind() { + Prefix::Disk(_) + | Prefix::VerbatimDisk(_) + | Prefix::UNC(_, _) + | Prefix::VerbatimUNC(_, _) => true, + Prefix::DeviceNS(device) => windows_device_namespace_is_filesystem(device), + Prefix::Verbatim(namespace) => namespace + .to_str() + .is_some_and(|namespace| namespace.eq_ignore_ascii_case("UNC")), + }, + _ => false, + }; + !supported_namespace || components.any(|component| matches!(component, Component::ParentDir)) +} + +#[cfg(windows)] +fn windows_device_namespace_is_filesystem(device: &OsStr) -> bool { + let bytes = device.as_encoded_bytes(); + bytes.eq_ignore_ascii_case(b"UNC") + || matches!(bytes, [drive, b':'] if drive.is_ascii_alphabetic()) +} + +#[cfg(windows)] +fn windows_path_has_untrusted_canonical_ancestor( + path: &Path, + locations: &UntrustedGitLocations, +) -> bool { + let Ok(canonical_path) = std::fs::canonicalize(path) else { + return true; + }; + if path_is_untrusted(&canonical_path, locations) { + return true; + } + for ancestor in path.ancestors().skip(1) { + let canonical_ancestor = match std::fs::canonicalize(ancestor) { + Ok(canonical_ancestor) => canonical_ancestor, + Err(error) + if matches!( + error.kind(), + io::ErrorKind::NotFound | io::ErrorKind::InvalidInput + ) => + { + continue; + } + Err(_) => return true, + }; + if path_is_untrusted(&canonical_ancestor, locations) { + return true; + } + } + false +} + fn path_is_in_worktree_for_common_dir(path: &Path, expected_common_dir: &Path) -> bool { let path = if path.is_dir() { path diff --git a/codex-rs/git-utils/src/git_command_tests.rs b/codex-rs/git-utils/src/git_command_tests.rs index 77807ba8d4c5..5ea99191d6aa 100644 --- a/codex-rs/git-utils/src/git_command_tests.rs +++ b/codex-rs/git-utils/src/git_command_tests.rs @@ -61,6 +61,22 @@ fn write_git_candidate(directory: &Path) { std::fs::write(candidate, b"git fixture").expect("write executable fixture"); } +#[cfg(windows)] +fn create_junction(path: &Path, target: &Path) { + let output = Command::new("cmd.exe") + .args(["/D", "/C", "mklink", "/J"]) + .arg(path) + .arg(target) + .output() + .expect("create junction"); + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + output.status.success(), + "mklink failed: stdout={stdout} stderr={stderr}" + ); +} + fn locations_for_root(root: &Path) -> UntrustedGitLocations { let mut roots = vec![root.to_path_buf()]; push_unique( @@ -73,6 +89,16 @@ fn locations_for_root(root: &Path) -> UntrustedGitLocations { } } +fn raw_parent_traversal(root: &Path, sibling: &str) -> PathBuf { + let separator = std::path::MAIN_SEPARATOR.to_string(); + let mut path = root.as_os_str().to_os_string(); + path.push(&separator); + path.push(".."); + path.push(&separator); + path.push(sibling); + path.into() +} + fn path_text(path: &Path) -> &str { path.to_str().expect("UTF-8 fixture path") } @@ -335,8 +361,31 @@ fn resolver_rejects_parent_traversal_spelled_through_repository() { let locations = locations_for_root(&repo); for root in &locations.roots { - let traversing_path = root.join("../trusted-bin"); - let search_path = std::env::join_paths([traversing_path]).expect("PATH"); + // Append without PathBuf::push: it resolves `..` when `root` has a + // verbatim Windows prefix, before the resolver can inspect the PATH + // spelling. + let traversing_path = raw_parent_traversal(root, "trusted-bin"); + let search_path = std::env::join_paths([&traversing_path]).expect("PATH"); + let split_paths = std::env::split_paths(&search_path).collect::>(); + assert_eq!(split_paths, vec![traversing_path.clone()]); + assert!( + search_directory_is_untrusted(&split_paths[0], &locations), + "raw PATH traversal was not rejected from {root:?}" + ); + + #[cfg(windows)] + if matches!( + root.components().next(), + Some(Component::Prefix(prefix)) if prefix.kind().is_verbatim() + ) { + assert_eq!( + split_paths[0].join(git_executable_name()), + std::fs::canonicalize(&trusted_bin) + .expect("canonical trusted bin") + .join(git_executable_name()) + ); + } + assert!( matches!( GitRunner::from_search_path(&locations, &search_path), @@ -347,6 +396,95 @@ fn resolver_rejects_parent_traversal_spelled_through_repository() { } } +#[cfg(windows)] +#[test] +fn resolver_rejects_parent_traversal_across_windows_namespaces() { + let traversing = [ + r"C:\Repo\..\outside", + r"\\?\C:\Repo\..\outside", + r"\\Server\Share\Repo\..\outside", + r"\\?\UNC\Server\Share\Repo\..\outside", + r"\\?\unc\Server\Share\Repo\..\outside", + r"\\.\C:\Repo\..\outside", + r"\\.\UNC\Server\Share\Repo\..\outside", + r"\\?\C:\RÉPO\..\outside", + ]; + for path in traversing { + assert!( + windows_path_requires_fail_closed(Path::new(path)), + "parent traversal was accepted: {path:?}" + ); + } + + let normalized_external = [ + r"C:\outside", + r"\\?\C:\outside", + r"\\Server\Share\outside", + r"\\?\UNC\Server\Share\outside", + r"\\?\unc\Server\Share\outside", + r"\\.\C:\outside", + r"\\.\UNC\Server\Share\outside", + ]; + for path in normalized_external { + assert!( + !windows_path_requires_fail_closed(Path::new(path)), + "normalized filesystem path was rejected: {path:?}" + ); + } +} + +#[cfg(windows)] +#[test] +fn resolver_rejects_unicode_case_alias_through_repository_junction() { + let fixture = tempfile::tempdir().expect("fixture"); + let repo = fixture.path().join("Répo"); + let outside = fixture.path().join("outside"); + let junction = repo.join("git-bin"); + std::fs::create_dir_all(&repo).expect("create repository"); + write_git_candidate(&outside); + create_junction(&junction, &outside); + + let case_alias = fixture.path().join("RÉPO").join("git-bin"); + let verbatim_case_alias = PathBuf::from(format!(r"\\?\{}", case_alias.display())); + assert_eq!( + std::fs::canonicalize(&verbatim_case_alias).expect("canonical alias"), + std::fs::canonicalize(&outside).expect("canonical outside") + ); + + let locations = locations_for_root(&repo); + assert!( + !path_is_untrusted(&verbatim_case_alias, &locations), + "fixture must exercise the Unicode alias before canonical ancestry" + ); + assert!(search_directory_is_untrusted( + &verbatim_case_alias, + &locations + )); + let search_path = std::env::join_paths([verbatim_case_alias]).expect("PATH"); + assert!(matches!( + GitRunner::from_search_path(&locations, &search_path), + Err(GitReadError::NoTrustedGit) + )); +} + +#[cfg(windows)] +#[test] +fn resolver_fails_closed_for_unsupported_windows_device_namespaces() { + let unsupported = [ + r"\\?\GLOBALROOT\Device\HarddiskVolumeShadowCopy1\git.exe", + r"\\?\Volume{11111111-1111-1111-1111-111111111111}\git.exe", + r"\\.\PhysicalDrive0", + r"\\.\pipe\codex-git", + ]; + + for path in unsupported { + assert!( + windows_path_requires_fail_closed(Path::new(path)), + "unsupported namespace was trusted: {path:?}" + ); + } +} + #[cfg(windows)] #[test] fn resolver_selects_native_git_exe_only() { From edda59a703ba52a7ccb4834e95eae3f9cc0792a6 Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Wed, 1 Jul 2026 16:49:19 -0700 Subject: [PATCH 5/6] git-utils: fix Windows traversal regression test --- codex-rs/git-utils/src/git_command_tests.rs | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/codex-rs/git-utils/src/git_command_tests.rs b/codex-rs/git-utils/src/git_command_tests.rs index 5ea99191d6aa..4347e71d50c4 100644 --- a/codex-rs/git-utils/src/git_command_tests.rs +++ b/codex-rs/git-utils/src/git_command_tests.rs @@ -361,9 +361,8 @@ fn resolver_rejects_parent_traversal_spelled_through_repository() { let locations = locations_for_root(&repo); for root in &locations.roots { - // Append without PathBuf::push: it resolves `..` when `root` has a - // verbatim Windows prefix, before the resolver can inspect the PATH - // spelling. + // Construct the PATH entry as raw text so its `..` component survives + // long enough for the resolver to inspect the original spelling. let traversing_path = raw_parent_traversal(root, "trusted-bin"); let search_path = std::env::join_paths([&traversing_path]).expect("PATH"); let split_paths = std::env::split_paths(&search_path).collect::>(); @@ -373,19 +372,6 @@ fn resolver_rejects_parent_traversal_spelled_through_repository() { "raw PATH traversal was not rejected from {root:?}" ); - #[cfg(windows)] - if matches!( - root.components().next(), - Some(Component::Prefix(prefix)) if prefix.kind().is_verbatim() - ) { - assert_eq!( - split_paths[0].join(git_executable_name()), - std::fs::canonicalize(&trusted_bin) - .expect("canonical trusted bin") - .join(git_executable_name()) - ); - } - assert!( matches!( GitRunner::from_search_path(&locations, &search_path), From 300bbcbf5c58c6256c8f6e76a98e93e5893dcf22 Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Wed, 1 Jul 2026 16:50:40 -0700 Subject: [PATCH 6/6] git-utils: clarify raw PATH rejection --- codex-rs/git-utils/src/git_command.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/codex-rs/git-utils/src/git_command.rs b/codex-rs/git-utils/src/git_command.rs index 4491959515a8..7e68c547c4f9 100644 --- a/codex-rs/git-utils/src/git_command.rs +++ b/codex-rs/git-utils/src/git_command.rs @@ -52,10 +52,10 @@ impl GitRunner { if !directory.is_absolute() { continue; } - // Check the PATH spelling before appending `git`. On Windows, - // PathBuf::push resolves `..` when its base uses a verbatim prefix, - // which would otherwise erase repository traversal before the - // first containment check. + // Check the raw PATH spelling before canonicalization. A search + // directory can traverse through an untrusted repository and + // resolve outside it; checking only the resolved parent or + // candidate would lose that provenance. if search_directory_is_untrusted(&directory, untrusted) { continue; }