From 0c066570d1769ff67274daeb14048c8d9da0696c Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Mon, 22 Jun 2026 10:59:47 -0700 Subject: [PATCH] Close internal Git worktree helper variants --- codex-rs/git-utils/src/apply.rs | 146 +++++++++++++++++++++++++++++++- codex-rs/git-utils/src/info.rs | 92 +++++++++++++++++++- 2 files changed, 232 insertions(+), 6 deletions(-) diff --git a/codex-rs/git-utils/src/apply.rs b/codex-rs/git-utils/src/apply.rs index 98a1f7156b..08710d4b22 100644 --- a/codex-rs/git-utils/src/apply.rs +++ b/codex-rs/git-utils/src/apply.rs @@ -130,10 +130,11 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { } fn resolve_git_root(cwd: &Path) -> io::Result { + let requested_cwd = std::fs::canonicalize(cwd)?; let out = std::process::Command::new("git") .arg("rev-parse") .arg("--show-toplevel") - .current_dir(cwd) + .current_dir(&requested_cwd) .output()?; let code = out.status.code().unwrap_or(-1); if code != 0 { @@ -143,8 +144,19 @@ 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)?; + if !requested_cwd.starts_with(&root) { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + format!( + "refusing to apply a patch because Git resolved worktree {} outside requested cwd {}", + root.display(), + requested_cwd.display() + ), + )); + } + Ok(root) } fn write_temp_patch(diff: &str) -> io::Result<(tempfile::TempDir, PathBuf)> { @@ -345,6 +357,7 @@ pub fn stage_paths(git_root: &Path, diff: &str) -> io::Result<()> { if existing.is_empty() { return Ok(()); } + ensure_paths_do_not_enter_submodules(git_root, &existing)?; let mut args = vec!["add".to_string(), "--".to_string()]; args.extend(existing); let config_parts = safe_git_config_parts(); @@ -353,6 +366,50 @@ pub fn stage_paths(git_root: &Path, diff: &str) -> io::Result<()> { Ok(()) } +fn ensure_paths_do_not_enter_submodules(git_root: &Path, paths: &[String]) -> io::Result<()> { + let mut candidates = std::collections::BTreeSet::new(); + for path in paths { + let mut components = path.split('/').filter(|component| !component.is_empty()); + let Some(first) = components.next() else { + continue; + }; + let mut candidate = first.to_string(); + candidates.insert(candidate.clone()); + for component in components { + candidate.push('/'); + candidate.push_str(component); + candidates.insert(candidate.clone()); + } + } + + let mut args = vec![ + "ls-files".to_string(), + "--stage".to_string(), + "-z".to_string(), + "--".to_string(), + ]; + args.extend(candidates); + let config_parts = safe_git_config_parts(); + let (code, stdout, stderr) = run_git(git_root, &config_parts, &args)?; + if code != 0 { + return Err(io::Error::other(format!( + "failed to inspect patch paths for submodules (exit {code}): {}", + stderr.trim() + ))); + } + if stdout + .split('\0') + .filter(|record| !record.is_empty()) + .any(|record| record.starts_with("160000 ")) + { + return Err(io::Error::new( + io::ErrorKind::Unsupported, + "refusing to stage a patch path that is a submodule or enters a submodule", + )); + } + Ok(()) +} + // ============ Parser ported from VS Code (TS) ============ /// Parse `git apply` output into applied/skipped/conflicted path groupings. @@ -694,6 +751,54 @@ mod tests { assert_eq!(config_code, 0, "configure merge driver: {config_err}"); } + fn init_submodule_with_clean_filter(parent: &Path) { + let source = tempfile::tempdir().expect("submodule source"); + let source_root = source.path(); + let _ = run(source_root, &["git", "init"]); + let _ = run( + source_root, + &["git", "config", "user.email", "codex@example.com"], + ); + let _ = run(source_root, &["git", "config", "user.name", "Codex"]); + std::fs::write(source_root.join("file.txt"), "original\n").expect("write submodule file"); + std::fs::write( + source_root.join(".gitattributes"), + "file.txt filter=codex-test\n", + ) + .expect("write submodule attributes"); + let _ = run(source_root, &["git", "add", "."]); + let _ = run(source_root, &["git", "commit", "-m", "seed"]); + + let source_path = source_root.to_string_lossy().into_owned(); + let (add_code, _, add_err) = run( + parent, + &[ + "git", + "-c", + "protocol.file.allow=always", + "submodule", + "add", + &source_path, + "nested", + ], + ); + assert_eq!(add_code, 0, "add submodule: {add_err}"); + let _ = run(parent, &["git", "commit", "-m", "add submodule"]); + + let nested = parent.join("nested"); + let (config_code, _, config_err) = run( + &nested, + &[ + "git", + "config", + "filter.codex-test.clean", + "git config codex.filterran true && git hash-object --stdin", + ], + ); + assert_eq!(config_code, 0, "configure submodule filter: {config_err}"); + std::fs::write(nested.join("file.txt"), "modified\n").expect("dirty submodule file"); + } + #[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"; @@ -960,4 +1065,39 @@ diff --git a/ghost.txt b/ghost.txt\n--- a/ghost.txt\n+++ b/ghost.txt\n@@ -1,1 +1 assert_ne!(marker_code, 0, "merge driver must not run"); assert_eq!(read_file_normalized(&root.join("file.txt")), "orig\n"); } + + #[test] + fn resolve_git_root_rejects_core_worktree_outside_requested_cwd() { + let temp = tempfile::tempdir().expect("tempdir"); + let buried_repo = temp.path().join("attacker"); + let victim = temp.path().join("victim"); + std::fs::create_dir_all(buried_repo.join("objects")).expect("objects"); + std::fs::create_dir_all(buried_repo.join("refs")).expect("refs"); + std::fs::create_dir_all(&victim).expect("victim"); + std::fs::write(buried_repo.join("HEAD"), "ref: refs/heads/main\n").expect("HEAD"); + std::fs::write( + buried_repo.join("config"), + format!( + "[core]\n\trepositoryformatversion = 0\n\tbare = false\n\tworktree = {}\n", + victim.display() + ), + ) + .expect("config"); + + let error = resolve_git_root(&buried_repo).expect_err("reject redirected worktree"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + } + + #[test] + fn stage_paths_rejects_gitlink_before_entering_submodule() { + let _g = env_lock().lock().unwrap(); + let repo = init_repo(); + let root = repo.path(); + init_submodule_with_clean_filter(root); + + let diff = "diff --git a/nested b/nested\nindex 1111111..2222222 160000\n--- a/nested\n+++ b/nested\n@@ -1 +1 @@\n-Subproject commit 1111111111111111111111111111111111111111\n+Subproject commit 2222222222222222222222222222222222222222\n"; + let error = stage_paths(root, diff).expect_err("reject gitlink staging"); + assert_eq!(error.kind(), io::ErrorKind::Unsupported); + assert!(!configured_filter_ran(&root.join("nested"))); + } } diff --git a/codex-rs/git-utils/src/info.rs b/codex-rs/git-utils/src/info.rs index 5f277a5c0f..b2bb9b561d 100644 --- a/codex-rs/git-utils/src/info.rs +++ b/codex-rs/git-utils/src/info.rs @@ -288,8 +288,13 @@ pub async fn get_has_changes(cwd: &Path) -> Option { return None; } let fsmonitor = detect_local_fsmonitor_override(git, cwd).await; - let output = - run_git_command_with_timeout_from(git, &["status", "--porcelain"], cwd, fsmonitor).await?; + let output = run_git_command_with_timeout_from( + git, + &["status", "--porcelain", "--ignore-submodules=dirty"], + cwd, + fsmonitor, + ) + .await?; if !output.status.success() { return None; } @@ -773,7 +778,14 @@ async fn diff_against_sha(cwd: &Path, sha: &GitSha) -> Option { let fsmonitor = detect_local_fsmonitor_override(git, cwd).await; let output = run_git_command_with_timeout_from( git, - &["diff", "--no-textconv", "--no-ext-diff", &sha.0], + &[ + "diff", + "--no-textconv", + "--no-ext-diff", + "--submodule=short", + "--ignore-submodules=dirty", + &sha.0, + ], cwd, fsmonitor, ) @@ -1019,6 +1031,47 @@ mod tests { output.status.success() } + async fn add_submodule_with_clean_filter(parent: &Path) { + let source = tempfile::tempdir().expect("submodule source"); + let source_path = source.path(); + run_git(source_path, &["init"]).await; + run_git(source_path, &["config", "user.name", "Test User"]).await; + run_git(source_path, &["config", "user.email", "test@example.com"]).await; + std::fs::write(source_path.join("nested.txt"), "original\n").expect("nested file"); + std::fs::write( + source_path.join(".gitattributes"), + "nested.txt filter=codex-test\n", + ) + .expect("nested attributes"); + run_git(source_path, &["add", "."]).await; + run_git(source_path, &["commit", "-m", "seed"]).await; + + run_git( + parent, + &[ + "-c", + "protocol.file.allow=always", + "submodule", + "add", + source_path.to_str().expect("source path"), + "nested", + ], + ) + .await; + run_git(parent, &["commit", "-m", "add submodule"]).await; + let nested = parent.join("nested"); + run_git( + &nested, + &[ + "config", + "filter.codex-test.clean", + "git config codex.filterran true && git hash-object --stdin", + ], + ) + .await; + std::fs::write(nested.join("nested.txt"), "modified\n").expect("dirty nested file"); + } + #[test] fn canonicalize_git_remote_url_normalizes_github_variants() { for remote in [ @@ -1066,6 +1119,39 @@ mod tests { assert!(!configured_filter_ran(&repo_path).await); } + #[tokio::test] + async fn get_has_changes_does_not_enter_dirty_submodules() { + let temp_dir = tempfile::tempdir().expect("create temp dir"); + let repo_path = create_test_git_repo(&temp_dir).await; + add_submodule_with_clean_filter(&repo_path).await; + + assert_eq!(get_has_changes(&repo_path).await, Some(false)); + assert!(!configured_filter_ran(&repo_path.join("nested")).await); + } + + #[tokio::test] + async fn diff_against_sha_does_not_enter_dirty_submodules() { + let temp_dir = tempfile::tempdir().expect("create temp dir"); + let repo_path = create_test_git_repo(&temp_dir).await; + add_submodule_with_clean_filter(&repo_path).await; + run_git(&repo_path, &["config", "diff.submodule", "diff"]).await; + let head = Command::new("git") + .args(["rev-parse", "HEAD"]) + .current_dir(&repo_path) + .output() + .await + .expect("read HEAD"); + assert!(head.status.success()); + let head = String::from_utf8(head.stdout).expect("utf8 HEAD"); + + assert!( + diff_against_sha(&repo_path, &GitSha::new(head.trim())) + .await + .is_some() + ); + assert!(!configured_filter_ran(&repo_path.join("nested")).await); + } + #[tokio::test] async fn git_diff_to_remote_rejects_configured_clean_filter_without_running_it() { let temp_dir = tempfile::tempdir().expect("create temp dir");