diff --git a/codex-rs/core-plugins/src/git_transport.rs b/codex-rs/core-plugins/src/git_transport.rs index 9c8fbcb107..45d2177b7e 100644 --- a/codex-rs/core-plugins/src/git_transport.rs +++ b/codex-rs/core-plugins/src/git_transport.rs @@ -53,7 +53,12 @@ impl NeutralGitCwd { Ok(Self { directory }) } - pub(crate) fn configure(&self, command: &mut Command) { + #[cfg(test)] + fn from_prebuilt_directory(directory: TempDir) -> Self { + Self { directory } + } + + pub(crate) fn configure_transport_command(&self, command: &mut Command) { sanitize_repository_environment(command); command .current_dir(self.directory.path()) @@ -62,6 +67,9 @@ impl NeutralGitCwd { } fn initialize_empty_repository(root: &std::path::Path) -> std::io::Result<()> { + // Build the discovery boundary without invoking Git. Running `git init` + // before this repository exists could rediscover and honor a hostile + // parent repository, which is exactly the behavior this type prevents. let git_dir = root.join(".git"); std::fs::create_dir(&git_dir)?; std::fs::create_dir(git_dir.join("objects"))?; @@ -98,7 +106,7 @@ mod tests { for name in REPOSITORY_LOCAL_GIT_ENVIRONMENT_VARIABLES { command.env(name, "hostile"); } - neutral_cwd.configure(&mut command); + neutral_cwd.configure_transport_command(&mut command); assert_eq!( command.get_current_dir(), @@ -130,17 +138,20 @@ mod tests { fn preserves_trusted_global_system_and_auth_environment() { let neutral_cwd = NeutralGitCwd::new().expect("create neutral Git working directory"); let trusted = [ + ("GIT_ASKPASS", "/trusted/askpass"), ("GIT_CONFIG_GLOBAL", "/trusted/global.gitconfig"), ("GIT_CONFIG_SYSTEM", "/trusted/system.gitconfig"), + ("GIT_SSH", "/trusted/ssh"), ("GIT_SSH_COMMAND", "/trusted/ssh-wrapper"), ("HOME", "/trusted/home"), + ("SSH_AUTH_SOCK", "/trusted/ssh-agent.sock"), ("XDG_CONFIG_HOME", "/trusted/config"), ]; let mut command = Command::new("git"); for (name, value) in trusted { command.env(name, value); } - neutral_cwd.configure(&mut command); + neutral_cwd.configure_transport_command(&mut command); for (name, value) in trusted { assert_eq!( @@ -156,7 +167,7 @@ mod tests { #[cfg(unix)] #[test] - fn nested_neutral_directory_does_not_run_parent_repository_transport_helper() { + fn nested_neutral_directory_ignores_parent_repository_transport_config() { let root = tempfile::tempdir().expect("create test root"); let fixture = create_transport_fixture(root.path()); @@ -165,9 +176,31 @@ mod tests { .tempdir_in(&fixture.hostile_repo) .expect("create nested neutral directory"); initialize_empty_repository(directory.path()).expect("initialize neutral repository"); - let neutral_cwd = NeutralGitCwd { directory }; + let neutral_cwd = NeutralGitCwd::from_prebuilt_directory(directory); + let empty_config = root.path().join("empty.gitconfig"); + fs::write(&empty_config, "").expect("write empty Git config"); + + let mut config_command = Command::new("git"); + config_command + .env("GIT_CONFIG_GLOBAL", &empty_config) + .env("GIT_CONFIG_SYSTEM", &empty_config); + neutral_cwd.configure_transport_command(&mut config_command); + let config_output = config_command + .args([ + "config", + "--get-regexp", + "^(core\\.sshCommand|credential\\.helper)$", + ]) + .output() + .expect("inspect effective transport configuration"); + assert_eq!(config_output.status.code(), Some(1)); + assert!(config_output.stdout.is_empty()); + let mut command = Command::new("git"); - neutral_cwd.configure(&mut command); + command + .env("GIT_CONFIG_GLOBAL", &empty_config) + .env("GIT_CONFIG_SYSTEM", &empty_config); + neutral_cwd.configure_transport_command(&mut command); let output = command .args([ "ls-remote", @@ -189,6 +222,75 @@ mod tests { ); } + #[cfg(unix)] + #[test] + fn neutral_directory_honors_trusted_global_and_system_transport_config() { + let root = tempfile::tempdir().expect("create test root"); + let fixture = create_transport_fixture(root.path()); + let global_config = root.path().join("global.gitconfig"); + let system_config = root.path().join("system.gitconfig"); + fs::write( + &global_config, + format!( + "[url \"file://{}\"]\n\tinsteadOf = https://trusted.example/repository\n[core]\n\tsshCommand = trusted-ssh-command\n", + fixture.source.display() + ), + ) + .expect("write trusted global Git config"); + fs::write( + &system_config, + "[credential]\n\thelper = trusted-credential-helper\n", + ) + .expect("write trusted system Git config"); + let neutral_cwd = NeutralGitCwd::new().expect("create neutral Git working directory"); + + let mut ssh_config = Command::new("git"); + ssh_config + .env("GIT_CONFIG_GLOBAL", &global_config) + .env("GIT_CONFIG_SYSTEM", &system_config); + neutral_cwd.configure_transport_command(&mut ssh_config); + let ssh_output = ssh_config + .args(["config", "--get", "core.sshCommand"]) + .output() + .expect("read trusted SSH configuration"); + assert!(ssh_output.status.success()); + assert_eq!( + String::from_utf8_lossy(&ssh_output.stdout).trim(), + "trusted-ssh-command" + ); + + let mut credential_config = Command::new("git"); + credential_config + .env("GIT_CONFIG_GLOBAL", &global_config) + .env("GIT_CONFIG_SYSTEM", &system_config); + neutral_cwd.configure_transport_command(&mut credential_config); + let credential_output = credential_config + .args(["config", "--get", "credential.helper"]) + .output() + .expect("read trusted credential configuration"); + assert!(credential_output.status.success()); + assert_eq!( + String::from_utf8_lossy(&credential_output.stdout).trim(), + "trusted-credential-helper" + ); + + let mut ls_remote = Command::new("git"); + ls_remote + .env("GIT_CONFIG_GLOBAL", &global_config) + .env("GIT_CONFIG_SYSTEM", &system_config); + neutral_cwd.configure_transport_command(&mut ls_remote); + let output = ls_remote + .args(["ls-remote", "https://trusted.example/repository", "HEAD"]) + .output() + .expect("run git ls-remote through trusted global rewrite"); + assert!( + output.status.success(), + "trusted global URL rewrite should remain available: {}", + String::from_utf8_lossy(&output.stderr) + ); + assert!(!output.stdout.is_empty()); + } + #[cfg(unix)] #[test] fn neutral_directory_ignores_inherited_git_common_dir() { @@ -197,7 +299,7 @@ mod tests { let neutral_cwd = NeutralGitCwd::new().expect("create neutral Git working directory"); let mut command = Command::new("git"); command.env("GIT_COMMON_DIR", fixture.hostile_repo.join(".git")); - neutral_cwd.configure(&mut command); + neutral_cwd.configure_transport_command(&mut command); let output = command .args([ @@ -237,7 +339,7 @@ mod tests { "GIT_CONFIG_VALUE_1", fixture.source.to_string_lossy().as_ref(), ); - neutral_cwd.configure(&mut command); + neutral_cwd.configure_transport_command(&mut command); let output = command .args([ @@ -295,6 +397,14 @@ mod tests { permissions.set_mode(0o755); fs::set_permissions(&helper, permissions).expect("mark transport helper executable"); run_git(&hostile_repo, &["config", "protocol.ext.allow", "always"]); + run_git( + &hostile_repo, + &["config", "core.sshCommand", "hostile-ssh-command"], + ); + run_git( + &hostile_repo, + &["config", "credential.helper", "!hostile-credential-helper"], + ); let rewrite_key = format!("url.ext::{}.insteadOf", helper.display()); run_git( &hostile_repo, diff --git a/codex-rs/core-plugins/src/loader.rs b/codex-rs/core-plugins/src/loader.rs index 43e27a1bc9..9942378cef 100644 --- a/codex-rs/core-plugins/src/loader.rs +++ b/codex-rs/core-plugins/src/loader.rs @@ -1457,8 +1457,11 @@ fn clone_git_plugin_source( sparse_checkout_path: Option<&str>, destination: &Path, ) -> Result<(), String> { + let neutral_cwd = NeutralGitCwd::new() + .map_err(|err| format!("failed to create neutral Git working directory: {err}"))?; if let Some(sparse_checkout_path) = sparse_checkout_path { run_git( + &neutral_cwd, &[ "clone", "--filter=blob:none", @@ -1470,6 +1473,7 @@ fn clone_git_plugin_source( /*cwd*/ None, )?; run_git( + &neutral_cwd, &[ "sparse-checkout", "set", @@ -1481,25 +1485,24 @@ fn clone_git_plugin_source( )?; } else { run_git( + &neutral_cwd, &["clone", url, destination.to_string_lossy().as_ref()], /*cwd*/ None, )?; } if let Some(target) = sha.or(ref_name) { - run_git(&["checkout", target], Some(destination))?; + run_git(&neutral_cwd, &["checkout", target], Some(destination))?; } else if sparse_checkout_path.is_some() { - run_git(&["checkout"], Some(destination))?; + run_git(&neutral_cwd, &["checkout"], Some(destination))?; } Ok(()) } -fn run_git(args: &[&str], cwd: Option<&Path>) -> Result<(), String> { - let neutral_cwd = NeutralGitCwd::new() - .map_err(|err| format!("failed to create neutral Git working directory: {err}"))?; +fn run_git(neutral_cwd: &NeutralGitCwd, args: &[&str], cwd: Option<&Path>) -> Result<(), String> { let mut command = Command::new("git"); command.args(args); command.env("GIT_TERMINAL_PROMPT", "0"); - neutral_cwd.configure(&mut command); + neutral_cwd.configure_transport_command(&mut command); if let Some(cwd) = cwd { command.current_dir(cwd); } diff --git a/codex-rs/core-plugins/src/loader_tests.rs b/codex-rs/core-plugins/src/loader_tests.rs index 8e5a76b4bf..12939fd897 100644 --- a/codex-rs/core-plugins/src/loader_tests.rs +++ b/codex-rs/core-plugins/src/loader_tests.rs @@ -463,15 +463,22 @@ fn materialize_git_subdir_uses_sparse_checkout() { fs::write(repo.path().join("plugins/other/marker.txt"), "other").expect("write other marker"); fs::write(repo.path().join("root.txt"), "root").expect("write root marker"); - run_git(&["init"], Some(repo.path())).expect("init git repo"); + let neutral_cwd = NeutralGitCwd::new().expect("create neutral Git working directory"); + run_git(&neutral_cwd, &["init"], Some(repo.path())).expect("init git repo"); run_git( + &neutral_cwd, &["config", "user.email", "test@example.com"], Some(repo.path()), ) .expect("configure git email"); - run_git(&["config", "user.name", "Test User"], Some(repo.path())).expect("configure git name"); - run_git(&["add", "."], Some(repo.path())).expect("stage git repo"); - run_git(&["commit", "-m", "init"], Some(repo.path())).expect("commit git repo"); + run_git( + &neutral_cwd, + &["config", "user.name", "Test User"], + Some(repo.path()), + ) + .expect("configure git name"); + run_git(&neutral_cwd, &["add", "."], Some(repo.path())).expect("stage git repo"); + run_git(&neutral_cwd, &["commit", "-m", "init"], Some(repo.path())).expect("commit git repo"); let materialized = materialize_marketplace_plugin_source( codex_home.path(), diff --git a/codex-rs/core-plugins/src/marketplace_add/install.rs b/codex-rs/core-plugins/src/marketplace_add/install.rs index ccbac0aeb9..8413fa30b2 100644 --- a/codex-rs/core-plugins/src/marketplace_add/install.rs +++ b/codex-rs/core-plugins/src/marketplace_add/install.rs @@ -11,14 +11,21 @@ pub(super) fn clone_git_source( sparse_paths: &[String], destination: &Path, ) -> Result<(), MarketplaceAddError> { + let neutral_cwd = NeutralGitCwd::new().map_err(|err| { + MarketplaceAddError::Internal(format!( + "failed to create neutral Git working directory: {err}" + )) + })?; let destination_string = destination.to_string_lossy().to_string(); if sparse_paths.is_empty() { run_git( + &neutral_cwd, &["clone", url, destination_string.as_str()], /*cwd*/ None, )?; if let Some(ref_name) = ref_name { run_git( + &neutral_cwd, &["checkout", ref_name], Some(Path::new(&destination_string)), )?; @@ -27,6 +34,7 @@ pub(super) fn clone_git_source( } run_git( + &neutral_cwd, &[ "clone", "--filter=blob:none", @@ -38,8 +46,12 @@ pub(super) fn clone_git_source( )?; let mut sparse_args = vec!["sparse-checkout", "set"]; sparse_args.extend(sparse_paths.iter().map(String::as_str)); - run_git(&sparse_args, Some(destination))?; - run_git(&["checkout", ref_name.unwrap_or("HEAD")], Some(destination))?; + run_git(&neutral_cwd, &sparse_args, Some(destination))?; + run_git( + &neutral_cwd, + &["checkout", ref_name.unwrap_or("HEAD")], + Some(destination), + )?; Ok(()) } @@ -111,16 +123,15 @@ pub(super) fn marketplace_staging_root(install_root: &Path) -> PathBuf { install_root.join(".staging") } -fn run_git(args: &[&str], cwd: Option<&Path>) -> Result<(), MarketplaceAddError> { - let neutral_cwd = NeutralGitCwd::new().map_err(|err| { - MarketplaceAddError::Internal(format!( - "failed to create neutral Git working directory: {err}" - )) - })?; +fn run_git( + neutral_cwd: &NeutralGitCwd, + args: &[&str], + cwd: Option<&Path>, +) -> Result<(), MarketplaceAddError> { let mut command = Command::new("git"); command.args(args); command.env("GIT_TERMINAL_PROMPT", "0"); - neutral_cwd.configure(&mut command); + neutral_cwd.configure_transport_command(&mut command); if let Some(cwd) = cwd { command.current_dir(cwd); } diff --git a/codex-rs/core-plugins/src/marketplace_upgrade/git.rs b/codex-rs/core-plugins/src/marketplace_upgrade/git.rs index ffa54966ad..a5cccf1f11 100644 --- a/codex-rs/core-plugins/src/marketplace_upgrade/git.rs +++ b/codex-rs/core-plugins/src/marketplace_upgrade/git.rs @@ -153,7 +153,7 @@ fn git_command(neutral_cwd: &NeutralGitCwd) -> Command { command .env("GIT_OPTIONAL_LOCKS", "0") .env("GIT_TERMINAL_PROMPT", "0"); - neutral_cwd.configure(&mut command); + neutral_cwd.configure_transport_command(&mut command); command } diff --git a/codex-rs/core-plugins/src/startup_sync.rs b/codex-rs/core-plugins/src/startup_sync.rs index af3b0a3192..b74f781c7d 100644 --- a/codex-rs/core-plugins/src/startup_sync.rs +++ b/codex-rs/core-plugins/src/startup_sync.rs @@ -583,7 +583,7 @@ fn git_ls_remote_head_sha(git_binary: &str) -> Result { .arg("ls-remote") .arg("https://github.com/openai/plugins.git") .arg("HEAD"); - neutral_cwd.configure(&mut command); + neutral_cwd.configure_transport_command(&mut command); let output = run_git_command_with_timeout( &mut command, "git ls-remote curated plugins repo",