diff --git a/codex-rs/git-utils/src/apply.rs b/codex-rs/git-utils/src/apply.rs index effc46d217..49909adf47 100644 --- a/codex-rs/git-utils/src/apply.rs +++ b/codex-rs/git-utils/src/apply.rs @@ -375,6 +375,13 @@ mod tests { code == 0 } + #[cfg(unix)] + fn trusted_git_directory() -> PathBuf { + std::env::split_paths(&std::env::var_os("PATH").expect("PATH")) + .find(|directory| directory.is_absolute() && directory.join("git").is_file()) + .expect("trusted Git directory") + } + #[test] fn parse_output_unescapes_quoted_paths() { let stderr = "error: patch failed: \"hello\\tworld.txt\":1\n"; @@ -457,6 +464,77 @@ mod tests { assert_eq!(read_file_normalized(&root.join("file.txt")), "new\n"); } + #[cfg(unix)] + #[test] + fn apply_uses_logical_process_cwd_to_reject_enclosing_git() { + use std::os::unix::fs::PermissionsExt; + + let _g = env_lock().lock().unwrap(); + if std::env::var_os("CODEX_GIT_UTILS_APPLY_LOGICAL_CWD_CHILD").is_none() { + let fixture = tempfile::tempdir().expect("fixture"); + let outer = fixture.path().join("outer"); + let physical_nested = fixture.path().join("physical-nested"); + let lexical_nested = outer.join("nested"); + let outer_bin = outer.join("bin"); + let outer_git = outer_bin.join("git"); + let marker = outer_bin.join("git.ran"); + std::fs::create_dir_all(&outer_bin).expect("outer Git directory"); + std::fs::create_dir_all(&physical_nested).expect("physical nested repository"); + let (outer_init, _, outer_err) = run(&outer, &["git", "init", "-q"]); + assert_eq!(outer_init, 0, "init outer repository: {outer_err}"); + let (nested_init, _, nested_err) = run(&physical_nested, &["git", "init", "-q"]); + assert_eq!(nested_init, 0, "init nested repository: {nested_err}"); + std::os::unix::fs::symlink(&physical_nested, &lexical_nested) + .expect("symlink nested repository"); + std::fs::write(&outer_git, "#!/bin/sh\nprintf ran >\"$0.ran\"\nexit 1\n") + .expect("outer Git shim"); + let mut permissions = std::fs::metadata(&outer_git) + .expect("outer Git metadata") + .permissions(); + permissions.set_mode(0o755); + std::fs::set_permissions(&outer_git, permissions).expect("executable outer Git"); + let path = std::env::join_paths([outer_bin, trusted_git_directory()]).expect("PATH"); + + let mut command = + std::process::Command::new(std::env::current_exe().expect("test binary")); + isolate_git_command_environment(&mut command); + let output = command + .arg("apply::tests::apply_uses_logical_process_cwd_to_reject_enclosing_git") + .arg("--exact") + .arg("--nocapture") + .current_dir(&lexical_nested) + .env("CODEX_GIT_UTILS_APPLY_LOGICAL_CWD_CHILD", "1") + .env("CODEX_GIT_UTILS_APPLY_LOGICAL_CWD_MARKER", &marker) + .env("PWD", &lexical_nested) + .env("PATH", path) + .env("RUST_TEST_THREADS", "1") + .output() + .expect("run isolated logical-cwd test"); + assert!( + output.status.success(), + "isolated logical-cwd test failed:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert!(!marker.exists(), "enclosing Git shim must not run"); + return; + } + + let cwd = std::env::current_dir().expect("physical process cwd"); + let marker = PathBuf::from( + std::env::var_os("CODEX_GIT_UTILS_APPLY_LOGICAL_CWD_MARKER").expect("marker path"), + ); + let result = apply_git_patch(&ApplyGitRequest { + cwd, + diff: "diff --git a/hello.txt b/hello.txt\nnew file mode 100644\n--- /dev/null\n+++ b/hello.txt\n@@ -0,0 +1 @@\n+hello\n".to_string(), + revert: false, + preflight: true, + }) + .expect("preflight through trusted Git"); + assert_eq!(result.exit_code, 0, "preflight should succeed"); + assert!(!marker.exists(), "enclosing Git shim must not run"); + } + #[test] fn apply_modify_conflict() { let _g = env_lock().lock().unwrap(); diff --git a/codex-rs/git-utils/src/git_command.rs b/codex-rs/git-utils/src/git_command.rs index 997578b839..5edb011f3c 100644 --- a/codex-rs/git-utils/src/git_command.rs +++ b/codex-rs/git-utils/src/git_command.rs @@ -16,7 +16,7 @@ pub(crate) struct GitRunner { struct UntrustedGitLocations { roots: Vec, - common_dir: Option, + common_dirs: Vec, } impl GitRunner { @@ -76,25 +76,110 @@ impl GitRunner { } fn untrusted_git_locations_for_cwd(cwd: &Path) -> Result { + let lexical_cwd = if cwd.is_absolute() { + cwd.to_path_buf() + } else { + std::env::current_dir() + .map_err(|_| GitReadError::NotRepository { + path: cwd.to_path_buf(), + })? + .join(cwd) + }; let canonical_cwd = std::fs::canonicalize(cwd).map_err(|_| GitReadError::NotRepository { path: cwd.to_path_buf(), })?; let worktree_root = crate::get_git_repo_root(&canonical_cwd) .and_then(|root| std::fs::canonicalize(root).ok()) .unwrap_or_else(|| canonical_cwd.clone()); - let mut roots = vec![worktree_root.clone()]; + let mut locations = UntrustedGitLocations { + roots: Vec::new(), + common_dirs: Vec::new(), + }; + record_repository_ancestry(&worktree_root, &mut locations)?; + + // Canonicalization can erase a repository-controlled symlink prefix. Walk + // the requested spelling too, deliberately retaining symlink and `..` + // components so every lexical enclosing checkout remains untrusted. + let lexical_base = if lexical_cwd.is_dir() { + lexical_cwd + } else { + lexical_cwd + .parent() + .ok_or_else(|| GitReadError::NotRepository { + path: cwd.to_path_buf(), + })? + .to_path_buf() + }; + record_repository_ancestry(&lexical_base, &mut locations)?; + + // Callers commonly obtain their default cwd from `current_dir()`, which + // can already have erased a symlink spelling. Recover the standard logical + // process cwd only when it is absolute and resolves to both the requested + // cwd and the process cwd. Treating extra roots as untrusted cannot widen + // executable selection. + if let Some(logical_cwd) = validated_logical_process_cwd(&canonical_cwd) { + record_repository_ancestry(&logical_cwd, &mut locations)?; + } + Ok(locations) +} + +fn validated_logical_process_cwd(canonical_cwd: &Path) -> Option { + let process_cwd = std::fs::canonicalize(std::env::current_dir().ok()?).ok()?; + if !paths_equal(&process_cwd, canonical_cwd) { + return None; + } + let logical_cwd = PathBuf::from(std::env::var_os("PWD")?); + if !logical_cwd.is_absolute() { + return None; + } + let canonical_logical_cwd = std::fs::canonicalize(&logical_cwd).ok()?; + paths_equal(&canonical_logical_cwd, canonical_cwd).then_some(logical_cwd) +} + +fn record_repository_ancestry( + start: &Path, + locations: &mut UntrustedGitLocations, +) -> Result<(), GitReadError> { + push_unique(&mut locations.roots, start.to_path_buf()); + record_repository_marker(start, locations)?; + for ancestor in start.parent().into_iter().flat_map(Path::ancestors) { + let dot_git = ancestor.join(".git"); + match std::fs::symlink_metadata(&dot_git) { + Ok(_) => { + push_unique(&mut locations.roots, ancestor.to_path_buf()); + let canonical_root = + std::fs::canonicalize(ancestor).map_err(|_| GitReadError::NoTrustedGit)?; + push_unique(&mut locations.roots, canonical_root.clone()); + record_repository_marker(ancestor, locations)?; + } + Err(error) if error.kind() == io::ErrorKind::NotFound => {} + Err(_) => return Err(GitReadError::NoTrustedGit), + } + } + Ok(()) +} + +fn record_repository_marker( + worktree_root: &Path, + locations: &mut UntrustedGitLocations, +) -> Result<(), GitReadError> { let dot_git = worktree_root.join(".git"); let common_dir = match std::fs::symlink_metadata(&dot_git) { - Ok(_) => Some(resolve_common_git_dir(&dot_git).map_err(|()| GitReadError::NoTrustedGit)?), - Err(error) if error.kind() == io::ErrorKind::NotFound => None, + Ok(_) => resolve_common_git_dir(&dot_git).map_err(|()| GitReadError::NoTrustedGit)?, + Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(()), Err(_) => return Err(GitReadError::NoTrustedGit), }; - if let Some(common_dir) = &common_dir - && !path_is_within(common_dir, &worktree_root) - { - roots.push(common_dir.clone()); + if !path_is_within(&common_dir, worktree_root) { + push_unique(&mut locations.roots, common_dir.clone()); + } + push_unique(&mut locations.common_dirs, common_dir); + Ok(()) +} + +fn push_unique(paths: &mut Vec, path: PathBuf) { + if !paths.iter().any(|existing| paths_equal(existing, &path)) { + paths.push(path); } - Ok(UntrustedGitLocations { roots, common_dir }) } fn path_is_untrusted(path: &Path, locations: &UntrustedGitLocations) -> bool { @@ -106,9 +191,9 @@ fn path_is_untrusted(path: &Path, locations: &UntrustedGitLocations) -> bool { return true; } locations - .common_dir - .as_deref() - .is_some_and(|common_dir| path_is_in_worktree_for_common_dir(path, common_dir)) + .common_dirs + .iter() + .any(|common_dir| path_is_in_worktree_for_common_dir(path, common_dir)) } fn path_is_in_worktree_for_common_dir(path: &Path, expected_common_dir: &Path) -> bool { diff --git a/codex-rs/git-utils/src/git_command_tests.rs b/codex-rs/git-utils/src/git_command_tests.rs index 29debcd749..15667106f9 100644 --- a/codex-rs/git-utils/src/git_command_tests.rs +++ b/codex-rs/git-utils/src/git_command_tests.rs @@ -2,6 +2,8 @@ use super::*; use pretty_assertions::assert_eq; use std::process::Command; +use crate::safe_git::DISABLED_HOOKS_PATH; + #[cfg(unix)] fn write_executable(path: &Path, body: &str) { use std::os::unix::fs::PermissionsExt; @@ -18,6 +20,12 @@ fn run_git(cwd: &Path, args: &[&str]) { let mut command = Command::new("git"); isolate_git_command_environment(&mut command); let status = command + .args([ + "-c", + &format!("core.hooksPath={DISABLED_HOOKS_PATH}"), + "-c", + "core.fsmonitor=false", + ]) .args(args) .current_dir(cwd) .status() @@ -25,6 +33,23 @@ fn run_git(cwd: &Path, args: &[&str]) { assert!(status.success(), "git {args:?} failed"); } +fn commit_all(cwd: &Path, message: &str) { + run_git( + cwd, + &[ + "-c", + "user.name=Codex Test", + "-c", + "user.email=codex@example.com", + "-c", + "commit.gpgSign=false", + "commit", + "-qam", + message, + ], + ); +} + fn write_git_candidate(directory: &Path) { std::fs::create_dir_all(directory).expect("create candidate directory"); let candidate = directory.join(git_executable_name()); @@ -39,7 +64,7 @@ fn write_git_candidate(directory: &Path) { fn locations_for_root(root: &Path) -> UntrustedGitLocations { UntrustedGitLocations { roots: vec![std::fs::canonicalize(root).expect("canonical root")], - common_dir: None, + common_dirs: Vec::new(), } } @@ -107,6 +132,130 @@ fn linked_worktree_rejects_git_from_main_and_linked_worktrees() { )); } +#[test] +fn nested_repository_rejects_git_from_enclosing_repository() { + let fixture = tempfile::tempdir().expect("fixture"); + let outer = fixture.path().join("outer"); + let nested = outer.join("nested"); + let outer_bin = outer.join("bin"); + let trusted_bin = fixture.path().join("trusted-bin"); + std::fs::create_dir_all(&nested).expect("create nested repository"); + run_git(&outer, &["init", "-q"]); + run_git(&nested, &["init", "-q"]); + write_git_candidate(&outer_bin); + write_git_candidate(&trusted_bin); + + let locations = untrusted_git_locations_for_cwd(&nested).expect("untrusted locations"); + assert!( + path_is_untrusted(&outer_bin.join(git_executable_name()), &locations), + "Git from an enclosing repository must remain repository-controlled" + ); + assert_eq!( + selected_git(&locations, &[&outer_bin, &trusted_bin]), + trusted_bin.join(git_executable_name()) + ); +} + +#[cfg(unix)] +#[test] +fn symlinked_nested_repository_rejects_git_from_lexical_enclosing_repository() { + let fixture = tempfile::tempdir().expect("fixture"); + let outer = fixture.path().join("outer"); + let physical_nested = fixture.path().join("physical-nested"); + let lexical_nested = outer.join("nested"); + let outer_bin = outer.join("bin"); + let trusted_bin = fixture.path().join("trusted-bin"); + std::fs::create_dir_all(&outer).expect("create outer repository"); + std::fs::create_dir_all(&physical_nested).expect("create physical nested repository"); + run_git(&outer, &["init", "-q"]); + run_git(&physical_nested, &["init", "-q"]); + std::os::unix::fs::symlink(&physical_nested, &lexical_nested) + .expect("symlink nested repository"); + write_git_candidate(&outer_bin); + write_git_candidate(&trusted_bin); + + let locations = untrusted_git_locations_for_cwd(&lexical_nested).expect("untrusted locations"); + assert!( + path_is_untrusted(&outer_bin.join(git_executable_name()), &locations), + "Git from the lexical enclosing repository must remain repository-controlled" + ); + assert_eq!( + selected_git(&locations, &[&outer_bin, &trusted_bin]), + trusted_bin.join(git_executable_name()) + ); +} + +#[test] +fn nested_repository_rejects_git_from_enclosing_repository_main_worktree() { + let fixture = tempfile::tempdir().expect("fixture"); + let main = fixture.path().join("main"); + let linked = fixture.path().join("linked"); + let nested = linked.join("nested"); + let main_bin = main.join("bin"); + let trusted_bin = fixture.path().join("trusted-bin"); + std::fs::create_dir_all(&main).expect("create main worktree"); + run_git(&main, &["init", "-q"]); + run_git(&main, &["worktree", "add", "--orphan", path_text(&linked)]); + std::fs::create_dir_all(&nested).expect("create nested repository"); + run_git(&nested, &["init", "-q"]); + write_git_candidate(&main_bin); + write_git_candidate(&trusted_bin); + + let locations = untrusted_git_locations_for_cwd(&nested).expect("untrusted locations"); + assert!( + path_is_untrusted(&main_bin.join(git_executable_name()), &locations), + "all worktrees of an enclosing repository must remain repository-controlled" + ); + assert_eq!( + selected_git(&locations, &[&main_bin, &trusted_bin]), + trusted_bin.join(git_executable_name()) + ); +} + +#[test] +fn submodule_rejects_git_from_enclosing_superproject() { + let fixture = tempfile::tempdir().expect("fixture"); + let source = fixture.path().join("source"); + let outer = fixture.path().join("outer"); + let submodule = outer.join("nested"); + let outer_bin = outer.join("bin"); + let trusted_bin = fixture.path().join("trusted-bin"); + std::fs::create_dir_all(&source).expect("create source repository"); + std::fs::create_dir_all(&outer).expect("create superproject"); + run_git(&source, &["init", "-q"]); + std::fs::write(source.join("source.txt"), "source\n").expect("write source file"); + run_git(&source, &["add", "source.txt"]); + commit_all(&source, "source"); + run_git(&outer, &["init", "-q"]); + std::fs::write(outer.join("outer.txt"), "outer\n").expect("write outer file"); + run_git(&outer, &["add", "outer.txt"]); + commit_all(&outer, "outer"); + run_git( + &outer, + &[ + "-c", + "protocol.file.allow=always", + "submodule", + "add", + "-q", + path_text(&source), + "nested", + ], + ); + write_git_candidate(&outer_bin); + write_git_candidate(&trusted_bin); + + let locations = untrusted_git_locations_for_cwd(&submodule).expect("untrusted locations"); + assert!( + path_is_untrusted(&outer_bin.join(git_executable_name()), &locations), + "Git from a superproject must remain repository-controlled" + ); + assert_eq!( + selected_git(&locations, &[&outer_bin, &trusted_bin]), + trusted_bin.join(git_executable_name()) + ); +} + #[test] fn bare_backed_linked_worktree_allows_external_git_in_sibling_directory() { let fixture = tempfile::tempdir().expect("fixture"); @@ -171,6 +320,23 @@ fn separate_dot_git_dir_rejects_main_candidate_and_allows_unrelated_repo_candida ); } +#[test] +fn resolver_rejects_parent_traversal_spelled_through_repository() { + let fixture = tempfile::tempdir().expect("fixture"); + let repo = fixture.path().join("repo"); + let trusted_bin = fixture.path().join("trusted-bin"); + std::fs::create_dir_all(&repo).expect("create repository"); + write_git_candidate(&trusted_bin); + + let locations = locations_for_root(&repo); + let traversing_path = locations.roots[0].join("../trusted-bin"); + let search_path = std::env::join_paths([traversing_path]).expect("PATH"); + assert!(matches!( + GitRunner::from_search_path(&locations, &search_path), + Err(GitReadError::NoTrustedGit) + )); +} + #[cfg(windows)] #[test] fn resolver_selects_native_git_exe_only() { diff --git a/codex-rs/git-utils/src/operations.rs b/codex-rs/git-utils/src/operations.rs index d58ff0beed..1564b81a19 100644 --- a/codex-rs/git-utils/src/operations.rs +++ b/codex-rs/git-utils/src/operations.rs @@ -2,14 +2,10 @@ use std::ffi::OsStr; use std::ffi::OsString; use std::path::Path; use std::path::PathBuf; -#[cfg(test)] -use std::process::Command; use crate::GitToolingError; use crate::git_command::GitRunner; use crate::safe_git::DISABLED_HOOKS_PATH; -#[cfg(test)] -use crate::safe_git::isolate_git_command_environment; pub(crate) fn ensure_git_repository(path: &Path) -> Result<(), GitToolingError> { match run_git_for_stdout( @@ -155,70 +151,5 @@ struct GitRun { } #[cfg(test)] -mod tests { - use super::*; - - fn init_repo() -> tempfile::TempDir { - let repo = tempfile::tempdir().expect("tempdir"); - let mut command = Command::new("git"); - isolate_git_command_environment(&mut command); - let status = command - .args(["init", "-q"]) - .current_dir(repo.path()) - .status() - .expect("initialize repository"); - assert!(status.success()); - repo - } - - #[test] - fn caller_env_cannot_restore_repository_or_pathspec_selectors() { - let target = init_repo(); - let alternate = init_repo(); - std::fs::write(target.path().join("target.txt"), "target\n").expect("target file"); - std::fs::write(alternate.path().join("alternate.txt"), "alternate\n") - .expect("alternate file"); - for (repo, path) in [(&target, "target.txt"), (&alternate, "alternate.txt")] { - let mut command = Command::new("git"); - isolate_git_command_environment(&mut command); - let status = command - .args(["add", path]) - .current_dir(repo.path()) - .status() - .expect("add file"); - assert!(status.success()); - } - - let alternate_git_dir = alternate.path().join(".git"); - let env = [ - ( - OsString::from("GIT_DIR"), - alternate_git_dir.as_os_str().into(), - ), - ( - OsString::from("GIT_WORK_TREE"), - alternate.path().as_os_str().into(), - ), - ( - OsString::from("GIT_COMMON_DIR"), - alternate_git_dir.as_os_str().into(), - ), - ( - OsString::from("GIT_INDEX_FILE"), - alternate_git_dir.join("index").into_os_string(), - ), - (OsString::from("GIT_PREFIX"), OsString::from("elsewhere/")), - (OsString::from("GIT_LITERAL_PATHSPECS"), OsString::from("1")), - (OsString::from("GIT_GLOB_PATHSPECS"), OsString::from("1")), - (OsString::from("GIT_NOGLOB_PATHSPECS"), OsString::from("1")), - (OsString::from("GIT_ICASE_PATHSPECS"), OsString::from("1")), - ( - OsString::from("GIT_CONFIG"), - alternate_git_dir.join("config").into_os_string(), - ), - ]; - let output = run_git_for_stdout(target.path(), ["ls-files"], Some(&env)) - .expect("query cwd-selected index"); - assert_eq!(output, "target.txt"); - } -} +#[path = "operations_tests.rs"] +mod tests; diff --git a/codex-rs/git-utils/src/operations_tests.rs b/codex-rs/git-utils/src/operations_tests.rs new file mode 100644 index 0000000000..e775212e26 --- /dev/null +++ b/codex-rs/git-utils/src/operations_tests.rs @@ -0,0 +1,66 @@ +use super::*; +use crate::safe_git::isolate_git_command_environment; +use std::process::Command; + +fn init_repo() -> tempfile::TempDir { + let repo = tempfile::tempdir().expect("tempdir"); + let mut command = Command::new("git"); + isolate_git_command_environment(&mut command); + let status = command + .args(["init", "-q"]) + .current_dir(repo.path()) + .status() + .expect("initialize repository"); + assert!(status.success()); + repo +} + +#[test] +fn caller_env_cannot_restore_repository_or_pathspec_selectors() { + let target = init_repo(); + let alternate = init_repo(); + std::fs::write(target.path().join("target.txt"), "target\n").expect("target file"); + std::fs::write(alternate.path().join("alternate.txt"), "alternate\n").expect("alternate file"); + for (repo, path) in [(&target, "target.txt"), (&alternate, "alternate.txt")] { + let mut command = Command::new("git"); + isolate_git_command_environment(&mut command); + let status = command + .args(["add", path]) + .current_dir(repo.path()) + .status() + .expect("add file"); + assert!(status.success()); + } + + let alternate_git_dir = alternate.path().join(".git"); + let env = [ + ( + OsString::from("GIT_DIR"), + alternate_git_dir.as_os_str().into(), + ), + ( + OsString::from("GIT_WORK_TREE"), + alternate.path().as_os_str().into(), + ), + ( + OsString::from("GIT_COMMON_DIR"), + alternate_git_dir.as_os_str().into(), + ), + ( + OsString::from("GIT_INDEX_FILE"), + alternate_git_dir.join("index").into_os_string(), + ), + (OsString::from("GIT_PREFIX"), OsString::from("elsewhere/")), + (OsString::from("GIT_LITERAL_PATHSPECS"), OsString::from("1")), + (OsString::from("GIT_GLOB_PATHSPECS"), OsString::from("1")), + (OsString::from("GIT_NOGLOB_PATHSPECS"), OsString::from("1")), + (OsString::from("GIT_ICASE_PATHSPECS"), OsString::from("1")), + ( + OsString::from("GIT_CONFIG"), + alternate_git_dir.join("config").into_os_string(), + ), + ]; + let output = run_git_for_stdout(target.path(), ["ls-files"], Some(&env)) + .expect("query cwd-selected index"); + assert_eq!(output, "target.txt"); +}