From 123e78b97b74d7288be553973361c34dac6830a6 Mon Sep 17 00:00:00 2001 From: iceweasel-oai Date: Wed, 6 May 2026 14:08:45 -0700 Subject: [PATCH] [codex] Fix Windows sandbox git safe.directory for worktrees (#21409) ## Why Windows sandboxed commands run as a sandbox user, while workspace repositories are usually owned by the real user. The sandbox compensates by injecting a temporary Git `safe.directory` entry into the child environment. That injection was still broken for linked worktrees because the helper followed the `.git` file's `gitdir:` pointer and injected the internal `.git/worktrees/...` location. Git's dubious-ownership check expects the worktree root instead, so sandboxed Git commands still failed in worktree-based Codex checkouts. ## What changed - Treat any `.git` marker, directory or file, as the worktree root for `safe.directory` injection. - Keep the safe-directory logic in `windows-sandbox-rs/src/sandbox_utils.rs` and have the one-shot elevated path reuse it. - Add regression coverage for both normal `.git` directories and gitfile-based worktrees. ## Validation - `cargo test -p codex-windows-sandbox sandbox_utils::tests` - `cargo test -p codex-windows-sandbox` built and ran; the new `sandbox_utils` tests passed, while two pre-existing legacy sandbox tests failed locally with `Access is denied`: `session::tests::legacy_non_tty_cmd_emits_output` and `spawn_prep::tests::legacy_spawn_env_applies_offline_network_rewrite`. --- .../windows-sandbox-rs/src/elevated_impl.rs | 67 +------------- .../windows-sandbox-rs/src/sandbox_utils.rs | 89 ++++++++++++++----- 2 files changed, 72 insertions(+), 84 deletions(-) diff --git a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs index 2c1c4f79ce..be9f1cfb98 100644 --- a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs +++ b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs @@ -37,72 +37,11 @@ mod windows_impl { use crate::policy::SandboxPolicy; use crate::policy::parse_policy; use crate::runner_client::spawn_runner_transport; + use crate::sandbox_utils::ensure_codex_home_exists; + use crate::sandbox_utils::inject_git_safe_directory; use crate::token::convert_string_sid_to_sid; use anyhow::Result; - use std::collections::HashMap; use std::path::Path; - use std::path::PathBuf; - - /// Ensures the parent directory of a path exists before writing to it. - /// Walks upward from `start` to locate the git worktree root, following gitfile redirects. - fn find_git_root(start: &Path) -> Option { - let mut cur = dunce::canonicalize(start).ok()?; - loop { - let marker = cur.join(".git"); - if marker.is_dir() { - return Some(cur); - } - if marker.is_file() { - if let Ok(txt) = std::fs::read_to_string(&marker) - && let Some(rest) = txt.trim().strip_prefix("gitdir:") - { - let gitdir = rest.trim(); - let resolved = if Path::new(gitdir).is_absolute() { - PathBuf::from(gitdir) - } else { - cur.join(gitdir) - }; - return resolved.parent().map(Path::to_path_buf).or(Some(cur)); - } - return Some(cur); - } - let parent = cur.parent()?; - if parent == cur { - return None; - } - cur = parent.to_path_buf(); - } - } - - /// Creates the sandbox user's Codex home directory if it does not already exist. - fn ensure_codex_home_exists(p: &Path) -> Result<()> { - std::fs::create_dir_all(p)?; - Ok(()) - } - - /// Adds a git safe.directory entry to the environment when running inside a repository. - /// git will not otherwise allow the Sandbox user to run git commands on the repo directory - /// which is owned by the primary user. - fn inject_git_safe_directory( - env_map: &mut HashMap, - cwd: &Path, - _logs_base_dir: Option<&Path>, - ) { - if let Some(git_root) = find_git_root(cwd) { - let mut cfg_count: usize = env_map - .get("GIT_CONFIG_COUNT") - .and_then(|v| v.parse::().ok()) - .unwrap_or(0); - let git_path = git_root.to_string_lossy().replace("\\\\", "/"); - env_map.insert( - format!("GIT_CONFIG_KEY_{cfg_count}"), - "safe.directory".to_string(), - ); - env_map.insert(format!("GIT_CONFIG_VALUE_{cfg_count}"), git_path); - cfg_count += 1; - env_map.insert("GIT_CONFIG_COUNT".to_string(), cfg_count.to_string()); - } - } pub use crate::windows_impl::CaptureResult; @@ -130,7 +69,7 @@ mod windows_impl { normalize_null_device_env(&mut env_map); ensure_non_interactive_pager(&mut env_map); inherit_path_env(&mut env_map); - inject_git_safe_directory(&mut env_map, cwd, None); + inject_git_safe_directory(&mut env_map, cwd); // Use a temp-based log dir that the sandbox user can write. let sandbox_base = codex_home.join(".sandbox"); ensure_codex_home_exists(&sandbox_base)?; diff --git a/codex-rs/windows-sandbox-rs/src/sandbox_utils.rs b/codex-rs/windows-sandbox-rs/src/sandbox_utils.rs index 5d64e5f844..fa08309550 100644 --- a/codex-rs/windows-sandbox-rs/src/sandbox_utils.rs +++ b/codex-rs/windows-sandbox-rs/src/sandbox_utils.rs @@ -8,28 +8,12 @@ use anyhow::Result; use std::collections::HashMap; use std::path::Path; -use std::path::PathBuf; -/// Walk upward from `start` to locate the git worktree root (supports gitfile redirects). -fn find_git_root(start: &Path) -> Option { +/// Walk upward from `start` to locate the git worktree root for `safe.directory`. +fn find_git_worktree_root_for_safe_directory(start: &Path) -> Option { let mut cur = dunce::canonicalize(start).ok()?; loop { - let marker = cur.join(".git"); - if marker.is_dir() { - return Some(cur); - } - if marker.is_file() { - if let Ok(txt) = std::fs::read_to_string(&marker) - && let Some(rest) = txt.trim().strip_prefix("gitdir:") - { - let gitdir = rest.trim(); - let resolved = if Path::new(gitdir).is_absolute() { - PathBuf::from(gitdir) - } else { - cur.join(gitdir) - }; - return resolved.parent().map(Path::to_path_buf).or(Some(cur)); - } + if cur.join(".git").exists() { return Some(cur); } let parent = cur.parent()?; @@ -50,7 +34,7 @@ pub fn ensure_codex_home_exists(p: &Path) -> Result<()> { /// git will not otherwise allow the Sandbox user to run git commands on the repo directory /// which is owned by the primary user. pub fn inject_git_safe_directory(env_map: &mut HashMap, cwd: &Path) { - if let Some(git_root) = find_git_root(cwd) { + if let Some(git_root) = find_git_worktree_root_for_safe_directory(cwd) { let mut cfg_count: usize = env_map .get("GIT_CONFIG_COUNT") .and_then(|v| v.parse::().ok()) @@ -65,3 +49,68 @@ pub fn inject_git_safe_directory(env_map: &mut HashMap, cwd: &Pa env_map.insert("GIT_CONFIG_COUNT".to_string(), cfg_count.to_string()); } } + +#[cfg(test)] +mod tests { + use super::inject_git_safe_directory; + use pretty_assertions::assert_eq; + use std::collections::HashMap; + use std::fs; + use std::path::Path; + use tempfile::TempDir; + + fn safe_directory_value(path: &Path) -> String { + dunce::canonicalize(path) + .expect("canonicalize path") + .to_string_lossy() + .replace("\\\\", "/") + } + + #[test] + fn injects_safe_directory_for_git_directory() { + let temp = TempDir::new().expect("tempdir"); + let repo = temp.path().join("repo"); + let nested = repo.join("nested"); + fs::create_dir_all(repo.join(".git")).expect("create .git"); + fs::create_dir_all(&nested).expect("create nested dir"); + + let mut env_map = HashMap::new(); + inject_git_safe_directory(&mut env_map, &nested); + + let expected = HashMap::from([ + ("GIT_CONFIG_COUNT".to_string(), "1".to_string()), + ("GIT_CONFIG_KEY_0".to_string(), "safe.directory".to_string()), + ( + "GIT_CONFIG_VALUE_0".to_string(), + safe_directory_value(&repo), + ), + ]); + assert_eq!(env_map, expected); + } + + #[test] + fn injects_worktree_root_for_gitfile() { + let temp = TempDir::new().expect("tempdir"); + let repo = temp.path().join("repo"); + let nested = repo.join("nested"); + fs::create_dir_all(&nested).expect("create nested dir"); + fs::write( + repo.join(".git"), + "gitdir: C:/Users/example/repo/.git/worktrees/codex3\n", + ) + .expect("write .git file"); + + let mut env_map = HashMap::new(); + inject_git_safe_directory(&mut env_map, &nested); + + let expected = HashMap::from([ + ("GIT_CONFIG_COUNT".to_string(), "1".to_string()), + ("GIT_CONFIG_KEY_0".to_string(), "safe.directory".to_string()), + ( + "GIT_CONFIG_VALUE_0".to_string(), + safe_directory_value(&repo), + ), + ]); + assert_eq!(env_map, expected); + } +}