From 7cdf6b09bcfac388a231ddd7e04989cfa009e9fb Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Wed, 1 Jul 2026 09:34:06 -0700 Subject: [PATCH] git-utils: confine staged paths to the worktree --- codex-rs/git-utils/src/patch_paths.rs | 279 +++++++++++++++- codex-rs/git-utils/src/patch_paths_tests.rs | 332 ++++++++++++++++++++ 2 files changed, 606 insertions(+), 5 deletions(-) diff --git a/codex-rs/git-utils/src/patch_paths.rs b/codex-rs/git-utils/src/patch_paths.rs index ad1878e619..0c98df22de 100644 --- a/codex-rs/git-utils/src/patch_paths.rs +++ b/codex-rs/git-utils/src/patch_paths.rs @@ -2,11 +2,13 @@ use std::io; use std::path::Path; +use std::path::PathBuf; use crate::apply::run_git; use crate::apply::safe_git_config_parts; use crate::apply::write_temp_patch; use crate::git_command::GitRunner; +use crate::git_config::path_is_within; pub(crate) fn extract_effective_paths_from_patch( git: &GitRunner, @@ -236,11 +238,17 @@ pub(crate) fn stage_effective_paths( git_root: &Path, paths: &[String], ) -> io::Result<()> { - let mut existing: Vec = Vec::new(); - for p in paths { - let joined = git_root.join(p); - if std::fs::symlink_metadata(&joined).is_ok() { - existing.push(p.clone()); + let confined = confine_patch_paths(git, git_root, paths)?; + let mut existing = Vec::new(); + for path in confined.into_exact_leaves() { + let joined = git_root.join(&path); + if let Ok(metadata) = std::fs::symlink_metadata(&joined) { + if leaf_is_traversable_directory(metadata.file_type()) { + return Err(containment_error( + "refusing to recursively stage a directory patch path", + )); + } + existing.push(path); } } if existing.is_empty() { @@ -258,6 +266,267 @@ pub(crate) fn stage_effective_paths( Ok(()) } +#[cfg(not(windows))] +fn leaf_is_traversable_directory(file_type: std::fs::FileType) -> bool { + file_type.is_dir() +} + +#[cfg(windows)] +fn leaf_is_traversable_directory(file_type: std::fs::FileType) -> bool { + use std::os::windows::fs::FileTypeExt; + + // Git traverses junctions and container-mapped directory symlinks. Refuse + // all directory-valued reparse leaves; true file symlinks remain allowed. + file_type.is_dir() || file_type.is_symlink_dir() +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum ConfinedPathRole { + StrictAncestor, + Leaf, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum ConfinedPathOrigin { + Raw, + Canonical, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct ConfinedPathCandidate { + pub(crate) path: String, + pub(crate) origin: ConfinedPathOrigin, + pub(crate) role: ConfinedPathRole, + pub(crate) depth: usize, +} + +impl ConfinedPathCandidate { + fn new(path: String, origin: ConfinedPathOrigin, role: ConfinedPathRole) -> Self { + let depth = path.split('/').count(); + Self { + path, + origin, + role, + depth, + } + } +} + +#[derive(Debug)] +pub(crate) struct ConfinedPatchPath { + pub(crate) exact_leaf: String, + pub(crate) candidates: Vec, +} + +#[derive(Debug)] +pub(crate) struct ConfinedPatchPaths { + pub(crate) entries: Vec, +} + +impl ConfinedPatchPaths { + fn into_exact_leaves(self) -> Vec { + self.entries + .into_iter() + .map(|entry| entry.exact_leaf) + .collect() + } +} + +pub(crate) fn confine_patch_paths( + git: &GitRunner, + git_root: &Path, + paths: &[String], +) -> io::Result { + if paths.is_empty() { + return Ok(ConfinedPatchPaths { + entries: Vec::new(), + }); + } + if paths.iter().all(|path| !path.contains('/')) { + return Ok(ConfinedPatchPaths { + entries: paths + .iter() + .map(|leaf| ConfinedPatchPath { + exact_leaf: leaf.clone(), + candidates: [ConfinedPathOrigin::Raw, ConfinedPathOrigin::Canonical] + .map(|origin| { + ConfinedPathCandidate::new(leaf.clone(), origin, ConfinedPathRole::Leaf) + }) + .into(), + }) + .collect(), + }); + } + + let canonical_root = std::fs::canonicalize(git_root)?; + let metadata_dirs = canonical_git_metadata_dirs(git, &canonical_root)?; + let mut entries = Vec::with_capacity(paths.len()); + let mut prefix_cache = std::collections::BTreeMap::new(); + + for leaf in paths { + let components = leaf.split('/').collect::>(); + let mut candidates = Vec::new(); + insert_candidate_prefixes( + components.iter().copied(), + ConfinedPathOrigin::Raw, + &mut candidates, + ); + + let (existing_len, mut projected) = longest_existing_strict_prefix( + &canonical_root, + &components, + &metadata_dirs, + &mut prefix_cache, + )?; + projected.extend( + components[existing_len..] + .iter() + .map(|component| (*component).to_string()), + ); + insert_candidate_prefixes( + projected.iter().map(String::as_str), + ConfinedPathOrigin::Canonical, + &mut candidates, + ); + entries.push(ConfinedPatchPath { + exact_leaf: leaf.clone(), + candidates, + }); + } + + Ok(ConfinedPatchPaths { entries }) +} + +fn longest_existing_strict_prefix( + canonical_root: &Path, + components: &[&str], + metadata_dirs: &[PathBuf], + prefix_cache: &mut std::collections::BTreeMap>>, +) -> io::Result<(usize, Vec)> { + let mut longest = (0, Vec::new()); + for existing_len in 1..components.len() { + let prefix = components[..existing_len].join("/"); + if let Some(cached) = prefix_cache.get(&prefix) { + if let Some(relative) = cached { + longest = (existing_len, relative.clone()); + } + continue; + } + match std::fs::canonicalize(canonical_root.join(&prefix)) { + Ok(resolved) => { + let relative = + confined_relative_components(&resolved, canonical_root, metadata_dirs)?; + prefix_cache.insert(prefix, Some(relative.clone())); + longest = (existing_len, relative); + } + Err(error) if error.kind() == io::ErrorKind::NotFound => { + prefix_cache.insert(prefix, None); + } + Err(error) => return Err(error), + } + } + Ok(longest) +} + +fn confined_relative_components( + resolved: &Path, + canonical_root: &Path, + metadata_dirs: &[PathBuf], +) -> io::Result> { + if metadata_dirs + .iter() + .any(|metadata_dir| path_is_within(resolved, metadata_dir)) + { + return Err(containment_error( + "patch path alias resolves into Git repository metadata", + )); + } + let relative = resolved + .strip_prefix(canonical_root) + .map_err(|_| containment_error("patch path alias resolves outside the Git worktree"))?; + if relative.as_os_str().is_empty() { + return Err(containment_error( + "patch path alias resolves to the Git worktree root", + )); + } + relative + .components() + .map(|component| { + component + .as_os_str() + .to_str() + .map(str::to_string) + .ok_or_else(|| { + io::Error::new( + io::ErrorKind::InvalidData, + "patch path alias is not valid UTF-8", + ) + }) + }) + .collect() +} + +fn containment_error(message: &'static str) -> io::Error { + io::Error::new(io::ErrorKind::PermissionDenied, message) +} + +fn canonical_git_metadata_dirs(git: &GitRunner, git_root: &Path) -> io::Result> { + let config_parts = safe_git_config_parts(); + let queries = [ + vec!["rev-parse".to_string(), "--absolute-git-dir".to_string()], + vec!["rev-parse".to_string(), "--git-common-dir".to_string()], + ]; + let mut metadata_dirs = std::collections::BTreeSet::new(); + for args in queries { + let (code, stdout, stderr) = run_git(git, git_root, &config_parts, &args)?; + if code != 0 { + return Err(io::Error::other(format!( + "failed to resolve Git repository metadata (exit {code}): {}", + stderr.trim() + ))); + } + let path = stdout.trim_end_matches(['\r', '\n']); + if path.is_empty() { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + "Git returned an empty repository metadata path", + )); + } + let path = PathBuf::from(path); + let absolute = if path.is_absolute() { + path + } else { + git_root.join(path) + }; + metadata_dirs.insert(std::fs::canonicalize(absolute)?); + } + Ok(metadata_dirs.into_iter().collect()) +} + +fn insert_candidate_prefixes<'a>( + components: impl IntoIterator, + origin: ConfinedPathOrigin, + candidates: &mut Vec, +) { + let components = components.into_iter().collect::>(); + let mut path = String::new(); + for (index, component) in components.iter().enumerate() { + if !path.is_empty() { + path.push('/'); + } + path.push_str(component); + candidates.push(ConfinedPathCandidate::new( + path.clone(), + origin, + if index + 1 == components.len() { + ConfinedPathRole::Leaf + } else { + ConfinedPathRole::StrictAncestor + }, + )); + } +} + #[cfg(test)] #[path = "patch_paths_tests.rs"] mod tests; diff --git a/codex-rs/git-utils/src/patch_paths_tests.rs b/codex-rs/git-utils/src/patch_paths_tests.rs index 7af43300e7..0616c460cc 100644 --- a/codex-rs/git-utils/src/patch_paths_tests.rs +++ b/codex-rs/git-utils/src/patch_paths_tests.rs @@ -1,8 +1,10 @@ use super::*; use crate::apply::ApplyGitRequest; use crate::apply::apply_git_patch; +use std::ffi::OsStr; use std::io; use std::path::Path; +use std::path::PathBuf; use std::sync::Mutex; use std::sync::OnceLock; @@ -26,6 +28,22 @@ fn run(cwd: &Path, args: &[&str]) -> (i32, String, String) { ) } +fn run_isolated_test(test_name: &str, env: &[(&str, &OsStr)]) { + let mut command = std::process::Command::new(std::env::current_exe().expect("test binary")); + crate::safe_git::isolate_git_command_environment(&mut command); + let output = command + .args([test_name, "--exact", "--nocapture"]) + .env("CODEX_GIT_UTILS_PATH_ENV_CHILD", "1") + .env("RUST_TEST_THREADS", "1") + .envs(env.iter().copied()) + .output() + .expect("run isolated test process"); + assert!( + output.status.success(), + "isolated test {test_name} failed: {output:?}" + ); +} + fn init_repo() -> tempfile::TempDir { let dir = tempfile::tempdir().expect("tempdir"); let root = dir.path(); @@ -378,3 +396,317 @@ fn patch_applies_valid_unix_colon_and_backslash_paths() { "old slash\n" ); } + +fn new_file_diff(path: &str) -> String { + format!( + "diff --git a/{path} b/{path}\nnew file mode 100644\n--- /dev/null\n+++ b/{path}\n@@ -0,0 +1 @@\n+new\n" + ) +} + +fn git_index_bytes(root: &Path) -> Vec { + let mut command = std::process::Command::new("git"); + crate::safe_git::isolate_git_command_environment(&mut command); + let output = command + .args(["ls-files", "--stage", "-z"]) + .current_dir(root) + .output() + .expect("inspect index"); + assert!(output.status.success(), "{:?}", output.status); + output.stdout +} + +fn assert_stage_refused(root: &Path, path: &str, before: &[u8]) { + let error = stage_paths(root, &new_file_diff(path)).expect_err("reject path alias"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + assert_eq!(git_index_bytes(root), before); +} + +fn commit_seed(root: &Path) { + std::fs::write(root.join("seed.txt"), "seed\n").expect("write seed"); + assert_eq!(run(root, &["git", "add", "seed.txt"]).0, 0); + assert_eq!(run(root, &["git", "commit", "-m", "seed"]).0, 0); +} + +#[cfg(any(unix, windows))] +fn create_dir_alias(target: &Path, alias: &Path) -> DirectoryAlias { + #[cfg(unix)] + std::os::unix::fs::symlink(target, alias).expect("create directory symlink"); + #[cfg(windows)] + { + let output = std::process::Command::new("cmd") + .args(["/C", "mklink", "/J"]) + .arg(alias) + .arg(target) + .output() + .expect("spawn mklink"); + assert!(output.status.success(), "mklink /J failed: {output:?}"); + } + DirectoryAlias(alias.to_path_buf()) +} + +#[cfg(any(unix, windows))] +struct DirectoryAlias(PathBuf); + +#[cfg(any(unix, windows))] +impl Drop for DirectoryAlias { + fn drop(&mut self) { + #[cfg(unix)] + let _ = std::fs::remove_file(&self.0); + #[cfg(windows)] + let _ = std::fs::remove_dir(&self.0); + } +} + +#[cfg(any(unix, windows))] +fn assert_longest_existing_alias_prefix_carries_unresolved_suffix() { + let repo = init_repo(); + let root = repo.path(); + std::fs::create_dir_all(root.join("container/realdir")).expect("create alias target"); + let alias = root.join("alias"); + let _alias = create_dir_alias(&root.join("container/realdir"), &alias); + + let git = GitRunner::for_cwd_io(root).expect("git runner"); + let paths = [ + "alias/nested/file.txt".to_string(), + "missing/path.txt".to_string(), + "alias/nested/file.txt".to_string(), + ]; + let confined = confine_patch_paths(&git, root, &paths).expect("confine"); + assert_eq!( + confine_patch_paths(&git, root, &paths) + .expect("confine exact leaves") + .into_exact_leaves(), + paths + ); + assert!( + confine_patch_paths(&git, root, &[]) + .unwrap() + .into_exact_leaves() + .is_empty() + ); + use ConfinedPathOrigin::*; + use ConfinedPathRole::*; + let candidate = |path: &str, origin: ConfinedPathOrigin, role: ConfinedPathRole| { + ConfinedPathCandidate::new(path.to_string(), origin, role) + }; + assert_eq!( + confined.entries[0].candidates, + vec![ + candidate("alias", Raw, StrictAncestor), + candidate("alias/nested", Raw, StrictAncestor), + candidate("alias/nested/file.txt", Raw, Leaf), + candidate("container", Canonical, StrictAncestor), + candidate("container/realdir", Canonical, StrictAncestor), + candidate("container/realdir/nested", Canonical, StrictAncestor), + candidate("container/realdir/nested/file.txt", Canonical, Leaf), + ] + ); + assert_eq!( + confined.entries[0] + .candidates + .iter() + .map(|candidate| candidate.depth) + .collect::>(), + [1, 2, 3, 1, 2, 3, 4] + ); + let missing = &confined.entries[1].candidates; + assert_eq!( + ( + &missing[0].path, + missing[0].origin, + &missing[2].path, + missing[2].origin + ), + (&missing[2].path, Raw, &missing[0].path, Canonical) + ); +} + +#[cfg(unix)] +#[test] +fn longest_existing_symlink_prefix_carries_the_unresolved_suffix() { + assert_longest_existing_alias_prefix_carries_unresolved_suffix(); +} + +#[cfg(windows)] +#[test] +fn longest_existing_junction_prefix_carries_the_unresolved_suffix() { + assert_longest_existing_alias_prefix_carries_unresolved_suffix(); +} + +#[cfg(any(unix, windows))] +#[test] +fn stage_rejects_strict_ancestor_alias_outside_or_to_worktree_root_without_mutation() { + let _g = env_lock().lock().unwrap(); + let repo = init_repo(); + let root = repo.path(); + let outside = tempfile::tempdir().expect("outside directory"); + std::fs::write(outside.path().join("marker.txt"), "outside\n").expect("outside marker"); + let before = git_index_bytes(root); + + for (name, target) in [("outside-alias", outside.path()), ("root-alias", root)] { + let alias = root.join(name); + let _alias = create_dir_alias(target, &alias); + assert_stage_refused(root, &format!("{name}/marker.txt"), &before); + #[cfg(windows)] + assert_stage_refused(root, name, &before); + } + assert_eq!( + std::fs::read(outside.path().join("marker.txt")).unwrap(), + b"outside\n" + ); + + std::fs::create_dir(root.join("inside")).expect("create in-tree return target"); + let escape = root.join("escape"); + let _escape = create_dir_alias(outside.path(), &escape); + let _return = create_dir_alias(&root.join("inside"), &outside.path().join("return")); + assert_stage_refused(root, "escape/return/file.txt", &before); +} + +#[cfg(any(unix, windows))] +#[test] +fn stage_rejects_aliases_into_private_and_common_git_metadata() { + let _g = env_lock().lock().unwrap(); + let repo = init_repo(); + commit_seed(repo.path()); + let linked_holder = tempfile::tempdir().expect("linked holder"); + let linked = linked_holder.path().join("linked"); + let linked_arg = linked.to_string_lossy().into_owned(); + assert_eq!( + run( + repo.path(), + &["git", "worktree", "add", "-b", "s3a-linked", &linked_arg] + ) + .0, + 0 + ); + + let resolve = |arg| { + let output = run(&linked, &["git", "rev-parse", arg]); + assert_eq!(output.0, 0, "resolve {arg}: {}", output.2); + let path = PathBuf::from(output.1.trim()); + std::fs::canonicalize(if path.is_absolute() { + path + } else { + linked.join(path) + }) + .unwrap() + }; + let targets = [resolve("--absolute-git-dir"), resolve("--git-common-dir")]; + assert_ne!(targets[0], targets[1]); + let before = git_index_bytes(&linked); + + for (index, target) in targets.into_iter().enumerate() { + let name = format!("metadata-alias-{index}"); + let alias = linked.join(&name); + let _alias = create_dir_alias(&target, &alias); + assert_stage_refused(&linked, &format!("{name}/probe"), &before); + #[cfg(windows)] + assert_stage_refused(&linked, &name, &before); + } +} + +#[cfg(unix)] +#[test] +fn stage_allows_outside_pointing_leaf_symlink_without_touching_target() { + let _g = env_lock().lock().unwrap(); + let repo = init_repo(); + let outside = tempfile::tempdir().expect("outside directory"); + let target = outside.path().join("target.txt"); + std::fs::write(&target, "outside\n").expect("write target"); + std::os::unix::fs::symlink(&target, repo.path().join("leaf")).expect("create leaf symlink"); + + stage_paths(repo.path(), &new_file_diff("leaf")).expect("stage leaf symlink"); + assert_eq!( + std::fs::read_to_string(target).expect("read target"), + "outside\n" + ); + assert!(String::from_utf8_lossy(&git_index_bytes(repo.path())).contains("\tleaf\0")); +} + +#[test] +fn stage_allows_ordinary_missing_and_unrelated_unicode_paths() { + let _g = env_lock().lock().unwrap(); + let repo = init_repo(); + let root = repo.path(); + std::fs::create_dir(root.join("résumé")).expect("create Unicode directory"); + std::fs::write(root.join("résumé/file.txt"), "new\n").expect("write Unicode file"); + + stage_paths(root, &new_file_diff("résumé/file.txt")).expect("stage Unicode path"); + assert!(String::from_utf8_lossy(&git_index_bytes(root)).contains("résumé/file.txt")); + + let before = git_index_bytes(root); + stage_paths(root, &new_file_diff("absent/nested.txt")).expect("allow missing path"); + assert_eq!(git_index_bytes(root), before); + assert!( + !root.join("absent").exists(), + "containment must be observational" + ); + + std::fs::create_dir(root.join("directory")).unwrap(); + std::fs::write(root.join("directory/unrelated.txt"), "unrelated\n").unwrap(); + assert_stage_refused(root, "directory", &before); +} + +#[test] +fn containment_does_not_claim_uninitialized_gitlinks() { + let _g = env_lock().lock().unwrap(); + let repo = init_repo(); + let root = repo.path(); + commit_seed(root); + let oid = run(root, &["git", "rev-parse", "HEAD"]); + assert_eq!(oid.0, 0, "{}", oid.2); + let cacheinfo = format!("160000,{},nested", oid.1.trim()); + assert_eq!( + run( + root, + &["git", "update-index", "--add", "--cacheinfo", &cacheinfo] + ) + .0, + 0 + ); + assert!(!root.join("nested").exists()); + + let git = GitRunner::for_cwd_io(root).expect("git runner"); + let paths = ["nested/file.txt".to_string()]; + let confined = confine_patch_paths(&git, root, &paths).expect("record candidates only"); + let candidate = &confined.entries[0].candidates[0]; + assert_eq!( + (candidate.path.as_str(), candidate.origin), + ("nested", ConfinedPathOrigin::Raw) + ); +} + +#[cfg(any(unix, windows))] +#[test] +fn containment_metadata_queries_ignore_inherited_git_selection_environment() { + let _g = env_lock().lock().unwrap(); + if std::env::var_os("CODEX_GIT_UTILS_PATH_ENV_CHILD").is_some() { + let root = PathBuf::from( + std::env::var_os("CODEX_GIT_UTILS_TARGET_REPO").expect("target repository"), + ); + let git = GitRunner::for_cwd_io(&root).expect("target git runner"); + let paths = ["metadata-alias/probe".to_string()]; + let error = + confine_patch_paths(&git, &root, &paths).expect_err("use target repository metadata"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + return; + } + + let target = init_repo(); + let metadata_alias = target.path().join("metadata-alias"); + let _metadata_alias = create_dir_alias(&target.path().join(".git"), &metadata_alias); + let alternate = init_repo(); + let alternate_git_dir = alternate.path().join(".git"); + let target_env = ("CODEX_GIT_UTILS_TARGET_REPO", target.path().as_os_str()); + for (name, value) in [ + ("GIT_DIR", alternate_git_dir.as_os_str()), + ("GIT_WORK_TREE", alternate.path().as_os_str()), + ("GIT_COMMON_DIR", alternate_git_dir.as_os_str()), + ("GIT_PREFIX", OsStr::new("elsewhere/")), + ] { + run_isolated_test( + "patch_paths::tests::containment_metadata_queries_ignore_inherited_git_selection_environment", + &[target_env, (name, value)], + ); + } +}