From 3e1a4906e47ba5514ceb1db024f63ee9294baec9 Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Mon, 22 Jun 2026 12:05:57 -0700 Subject: [PATCH] Deny transport for local-only Git operations --- codex-rs/git-utils/src/apply.rs | 12 +- codex-rs/git-utils/src/info.rs | 121 +++++++++++++++++++- codex-rs/git-utils/src/lib.rs | 2 + codex-rs/git-utils/src/local_only.rs | 8 ++ codex-rs/git-utils/src/operations.rs | 1 + codex-rs/tui/src/branch_summary.rs | 157 +++++++++++++++++++++++++- codex-rs/tui/src/get_git_diff.rs | 25 +++- codex-rs/tui/src/workspace_command.rs | 9 ++ 8 files changed, 326 insertions(+), 9 deletions(-) create mode 100644 codex-rs/git-utils/src/local_only.rs diff --git a/codex-rs/git-utils/src/apply.rs b/codex-rs/git-utils/src/apply.rs index b9adb82e6b3c..e9efed14b85a 100644 --- a/codex-rs/git-utils/src/apply.rs +++ b/codex-rs/git-utils/src/apply.rs @@ -124,7 +124,7 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { } fn resolve_git_root(cwd: &Path) -> io::Result { - let out = std::process::Command::new("git") + let out = local_git_command() .arg("rev-parse") .arg("--show-toplevel") .current_dir(cwd) @@ -149,7 +149,7 @@ fn write_temp_patch(diff: &str) -> io::Result<(tempfile::TempDir, PathBuf)> { } fn run_git(cwd: &Path, git_cfg: &[String], args: &[String]) -> io::Result<(i32, String, String)> { - let mut cmd = std::process::Command::new("git"); + let mut cmd = local_git_command(); for p in git_cfg { cmd.arg(p); } @@ -163,6 +163,12 @@ fn run_git(cwd: &Path, git_cfg: &[String], args: &[String]) -> io::Result<(i32, Ok((code, stdout, stderr)) } +fn local_git_command() -> std::process::Command { + let mut command = std::process::Command::new("git"); + command.envs(crate::local_only_git_env()); + command +} + fn quote_shell(s: &str) -> String { let simple = s .chars() @@ -329,7 +335,7 @@ pub fn stage_paths(git_root: &Path, diff: &str) -> io::Result<()> { if existing.is_empty() { return Ok(()); } - let mut cmd = std::process::Command::new("git"); + let mut cmd = local_git_command(); cmd.arg("add"); cmd.arg("--"); for p in &existing { diff --git a/codex-rs/git-utils/src/info.rs b/codex-rs/git-utils/src/info.rs index 48d229df8db5..f48169f31ed2 100644 --- a/codex-rs/git-utils/src/info.rs +++ b/codex-rs/git-utils/src/info.rs @@ -416,7 +416,11 @@ impl crate::FsmonitorProbeRunner for LocalFsmonitorProbeRunner<'_> { // Both probes are fast, bounded metadata queries that do not inspect the // worktree or index, so do not reduce the requested command's timeout. let mut command = Command::new(self.git); - command.args(args).current_dir(self.cwd).kill_on_drop(true); + command + .envs(crate::local_only_git_env()) + .args(args) + .current_dir(self.cwd) + .kill_on_drop(true); match timeout(GIT_COMMAND_TIMEOUT, command.output()).await { Ok(Ok(output)) if output.status.success() => Some(output.stdout), _ => None, @@ -437,6 +441,7 @@ async fn run_git_command_with_timeout_from( ) -> Option { let mut command = Command::new(git); command + .envs(crate::local_only_git_env()) .env("GIT_OPTIONAL_LOCKS", "0") // Keep internal Git commands independent of repository-selected hooks // and fsmonitor helpers while preserving built-in fsmonitor acceleration. @@ -918,6 +923,120 @@ mod tests { #[cfg(unix)] use std::os::unix::fs::PermissionsExt; + #[cfg(unix)] + fn run_git(cwd: &Path, args: &[&str]) -> std::process::Output { + let output = std::process::Command::new("git") + .args(args) + .current_dir(cwd) + .output() + .expect("run Git command"); + assert!( + output.status.success(), + "git {args:?} failed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + output + } + + #[cfg(unix)] + fn run_git_stdout(cwd: &Path, args: &[&str]) -> String { + String::from_utf8(run_git(cwd, args).stdout) + .expect("Git output should be UTF-8") + .trim() + .to_string() + } + + #[cfg(unix)] + fn commit_all(cwd: &Path, message: &str) { + run_git( + cwd, + &[ + "-c", + "user.name=Codex Test", + "-c", + "user.email=codex@example.com", + "commit", + "-qam", + message, + ], + ); + } + + #[cfg(unix)] + #[tokio::test] + async fn diff_against_sha_does_not_lazy_fetch_promisor_objects() { + let temp_dir = tempfile::tempdir().expect("create temp dir"); + let source = temp_dir.path().join("source"); + let clone = temp_dir.path().join("clone"); + std::fs::create_dir(&source).expect("create source repository"); + run_git(&source, &["init", "-q", "--initial-branch=main"]); + run_git(&source, &["config", "uploadpack.allowFilter", "true"]); + + std::fs::write(source.join("data.txt"), "before\n").expect("write initial blob"); + run_git(&source, &["add", "data.txt"]); + commit_all(&source, "initial"); + let base_sha = run_git_stdout(&source, &["rev-parse", "HEAD"]); + let base_blob = run_git_stdout(&source, &["rev-parse", "HEAD:data.txt"]); + + std::fs::write(source.join("data.txt"), "after\n").expect("write current blob"); + commit_all(&source, "current"); + + let complete_diff = diff_against_sha(&source, &GitSha::new(&base_sha)) + .await + .expect("diff complete repository"); + assert!( + complete_diff.contains("-before") && complete_diff.contains("+after"), + "complete-repository diff should remain available:\n{complete_diff}" + ); + + let source_url = format!("file://{}", source.display()); + run_git( + temp_dir.path(), + &[ + "-c", + "protocol.file.allow=always", + "clone", + "-q", + "--no-local", + "--filter=blob:none", + "--no-checkout", + &source_url, + clone.to_str().expect("clone path"), + ], + ); + run_git(&clone, &["checkout", "-q", "main"]); + + let missing = run_git_stdout( + &clone, + &["rev-list", "--objects", "--all", "--missing=print"], + ); + assert!( + missing.lines().any(|line| line == format!("?{base_blob}")), + "expected historical blob {base_blob} to remain missing:\n{missing}" + ); + + let helper = temp_dir.path().join("transport-helper.sh"); + std::fs::write(&helper, "#!/bin/sh\nprintf ran >\"$0.ran\"\nexit 1\n") + .expect("write transport helper"); + let mut permissions = std::fs::metadata(&helper) + .expect("read transport helper metadata") + .permissions(); + permissions.set_mode(/*mode*/ 0o755); + std::fs::set_permissions(&helper, permissions).expect("make transport helper executable"); + let helper_url = format!("ext::{}", helper.display()); + run_git(&clone, &["config", "remote.origin.url", &helper_url]); + run_git(&clone, &["config", "protocol.ext.allow", "always"]); + + let diff = diff_against_sha(&clone, &GitSha::new(&base_sha)).await; + + assert_eq!( + (diff, helper.with_extension("sh.ran").exists()), + (None, false), + "local-only diff must fail without invoking the promisor transport" + ); + } + #[test] fn canonicalize_git_remote_url_normalizes_github_variants() { for remote in [ diff --git a/codex-rs/git-utils/src/lib.rs b/codex-rs/git-utils/src/lib.rs index e04e6044109d..d4817d87c8a7 100644 --- a/codex-rs/git-utils/src/lib.rs +++ b/codex-rs/git-utils/src/lib.rs @@ -4,6 +4,7 @@ mod branch; mod errors; mod fsmonitor; mod info; +mod local_only; mod operations; mod platform; @@ -42,4 +43,5 @@ pub use info::git_diff_to_remote; pub use info::local_git_branches; pub use info::recent_commits; pub use info::resolve_root_git_project_for_trust; +pub use local_only::local_only_git_env; pub use platform::create_symlink; diff --git a/codex-rs/git-utils/src/local_only.rs b/codex-rs/git-utils/src/local_only.rs new file mode 100644 index 000000000000..042654324acc --- /dev/null +++ b/codex-rs/git-utils/src/local_only.rs @@ -0,0 +1,8 @@ +/// Environment overrides for Git commands that must only use local repository data. +/// +/// An empty `GIT_ALLOW_PROTOCOL` list denies every Git transport, including implicit fetches from +/// promisor remotes. `GIT_NO_LAZY_FETCH` provides defense in depth on Git versions that support it; +/// older versions safely ignore the unknown variable. +pub fn local_only_git_env() -> impl Iterator { + [("GIT_ALLOW_PROTOCOL", ""), ("GIT_NO_LAZY_FETCH", "1")].into_iter() +} diff --git a/codex-rs/git-utils/src/operations.rs b/codex-rs/git-utils/src/operations.rs index f17ce209b0a2..00f44a6233e4 100644 --- a/codex-rs/git-utils/src/operations.rs +++ b/codex-rs/git-utils/src/operations.rs @@ -117,6 +117,7 @@ where command.env(key, value); } } + command.envs(crate::local_only_git_env()); command.args(&args_vec); let output = command.output()?; if !output.status.success() { diff --git a/codex-rs/tui/src/branch_summary.rs b/codex-rs/tui/src/branch_summary.rs index 4698dc96e56e..8bbe885bd2f9 100644 --- a/codex-rs/tui/src/branch_summary.rs +++ b/codex-rs/tui/src/branch_summary.rs @@ -479,7 +479,7 @@ async fn run_git_command( argv.extend(args.iter().map(|arg| (*arg).to_string())); runner .run( - WorkspaceCommand::new(argv) + WorkspaceCommand::local_only_git(argv) .cwd(cwd.to_path_buf()) .env("GIT_OPTIONAL_LOCKS", "0"), ) @@ -513,10 +513,85 @@ mod tests { use super::*; use crate::workspace_command::WorkspaceCommand; use pretty_assertions::assert_eq; + #[cfg(unix)] + use std::fs; use std::future::Future; + #[cfg(unix)] + use std::os::unix::fs::PermissionsExt; use std::pin::Pin; + #[cfg(unix)] + use std::process::Command as ProcessCommand; use std::sync::Mutex; + #[cfg(unix)] + #[tokio::test] + async fn branch_diff_stats_do_not_lazy_fetch_promisor_objects() { + let temp_dir = tempfile::tempdir().expect("create temp dir"); + let source = temp_dir.path().join("source"); + let clone = temp_dir.path().join("clone"); + fs::create_dir(&source).expect("create source repository"); + run_git(&source, &["init", "-q", "--initial-branch=main"]); + run_git(&source, &["config", "uploadpack.allowFilter", "true"]); + + fs::write(source.join("data.txt"), "before\n").expect("write initial blob"); + run_git(&source, &["add", "data.txt"]); + commit_all(&source, "initial"); + let base_sha = run_git_stdout(&source, &["rev-parse", "HEAD"]); + let base_blob = run_git_stdout(&source, &["rev-parse", "HEAD:data.txt"]); + + fs::write(source.join("data.txt"), "after\n").expect("write current blob"); + commit_all(&source, "current"); + + let source_url = format!("file://{}", source.display()); + run_git( + temp_dir.path(), + &[ + "-c", + "protocol.file.allow=always", + "clone", + "-q", + "--no-local", + "--filter=blob:none", + "--no-checkout", + &source_url, + clone.to_str().expect("clone path"), + ], + ); + run_git(&clone, &["checkout", "-q", "main"]); + let missing = run_git_stdout( + &clone, + &["rev-list", "--objects", "--all", "--missing=print"], + ); + assert!( + missing.lines().any(|line| line == format!("?{base_blob}")), + "expected historical blob {base_blob} to remain missing:\n{missing}" + ); + run_git( + &clone, + &["update-ref", "refs/remotes/origin/main", &base_sha], + ); + + let helper = temp_dir.path().join("transport-helper.sh"); + fs::write(&helper, "#!/bin/sh\nprintf ran >\"$0.ran\"\nexit 1\n") + .expect("write transport helper"); + let mut permissions = fs::metadata(&helper) + .expect("read transport helper metadata") + .permissions(); + permissions.set_mode(/*mode*/ 0o755); + fs::set_permissions(&helper, permissions).expect("make transport helper executable"); + let helper_url = format!("ext::{}", helper.display()); + run_git(&clone, &["config", "remote.origin.url", &helper_url]); + run_git(&clone, &["config", "protocol.ext.allow", "always"]); + + let stats = branch_diff_stats_to_default_branch(&LocalRunner, &clone).await; + + assert_eq!( + (stats, helper.with_extension("sh.ran").exists()), + (None, false), + "local-only branch stats must fail without invoking the promisor transport" + ); + } + #[tokio::test] async fn branch_diff_stats_prefers_remote_default_ref_over_stale_local_branch() { let runner = FakeRunner::new(vec![ @@ -736,4 +811,84 @@ mod tests { }) } } + + #[cfg(unix)] + fn run_git(cwd: &Path, args: &[&str]) -> std::process::Output { + let output = ProcessCommand::new("git") + .args(args) + .current_dir(cwd) + .output() + .expect("run Git command"); + assert!( + output.status.success(), + "git {args:?} failed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + output + } + + #[cfg(unix)] + fn run_git_stdout(cwd: &Path, args: &[&str]) -> String { + String::from_utf8(run_git(cwd, args).stdout) + .expect("Git output should be UTF-8") + .trim() + .to_string() + } + + #[cfg(unix)] + fn commit_all(cwd: &Path, message: &str) { + run_git( + cwd, + &[ + "-c", + "user.name=Codex Test", + "-c", + "user.email=codex@example.com", + "commit", + "-qam", + message, + ], + ); + } + + #[cfg(unix)] + struct LocalRunner; + + #[cfg(unix)] + impl WorkspaceCommandExecutor for LocalRunner { + fn run( + &self, + command: WorkspaceCommand, + ) -> Pin< + Box< + dyn Future> + + Send + + '_, + >, + > { + Box::pin(async move { + let mut process = ProcessCommand::new(&command.argv[0]); + process + .args(&command.argv[1..]) + .current_dir(command.cwd.expect("test command cwd")); + for (key, value) in command.env { + match value { + Some(value) => { + process.env(key, value); + } + None => { + process.env_remove(key); + } + } + } + let output = process.output().expect("run test command"); + Ok(WorkspaceCommandOutput { + exit_code: output.status.code().expect("test command exit code"), + stdout: String::from_utf8(output.stdout).expect("utf8 stdout"), + stderr: String::from_utf8(output.stderr).expect("utf8 stderr"), + }) + }) + } + } } diff --git a/codex-rs/tui/src/get_git_diff.rs b/codex-rs/tui/src/get_git_diff.rs index ab7764b6b3f1..b463897c8c8e 100644 --- a/codex-rs/tui/src/get_git_diff.rs +++ b/codex-rs/tui/src/get_git_diff.rs @@ -34,7 +34,7 @@ struct WorkspaceFsmonitorProbeRunner<'a> { impl FsmonitorProbeRunner for WorkspaceFsmonitorProbeRunner<'_> { async fn run_probe(&mut self, args: &[&str]) -> Option> { let argv = ["git"].into_iter().chain(args.iter().copied()); - let command = WorkspaceCommand::new(argv).cwd(self.cwd.to_path_buf()); + let command = WorkspaceCommand::local_only_git(argv).cwd(self.cwd.to_path_buf()); match self.runner.run(command).await { Ok(output) if output.success() => Some(output.stdout.into_bytes()), _ => None, @@ -238,7 +238,7 @@ async fn run_git_command( ] .into_iter() .chain(args.iter().copied()); - let mut command = WorkspaceCommand::new(argv) + let mut command = WorkspaceCommand::local_only_git(argv) .cwd(cwd.to_path_buf()) .timeout(DIFF_COMMAND_TIMEOUT) .disable_output_cap(); @@ -764,7 +764,8 @@ mod tests { } fn filter_override_env(driver: &str) -> HashMap> { - HashMap::from([ + let mut env = expected_local_only_git_env(); + env.extend([ ("GIT_CONFIG_COUNT".to_string(), Some("3".to_string())), ( "GIT_CONFIG_KEY_0".to_string(), @@ -781,6 +782,14 @@ mod tests { Some(format!("{driver}.required")), ), ("GIT_CONFIG_VALUE_2".to_string(), Some("false".to_string())), + ]); + env + } + + fn expected_local_only_git_env() -> HashMap> { + HashMap::from([ + ("GIT_ALLOW_PROTOCOL".to_string(), Some(String::new())), + ("GIT_NO_LAZY_FETCH".to_string(), Some("1".to_string())), ]) } @@ -833,11 +842,19 @@ mod tests { command.argv.get(1).map(String::as_str), Some("config" | "version") ) { - assert_eq!(command.env, HashMap::new()); + assert_eq!(command.env, expected_local_only_git_env()); assert_eq!(command.timeout, Duration::from_secs(/*secs*/ 5)); assert_eq!(command.output_bytes_cap, 64 * 1024); assert_eq!(command.disable_output_cap, false); } else { + assert_eq!( + command.env.get("GIT_ALLOW_PROTOCOL"), + Some(&Some(String::new())) + ); + assert_eq!( + command.env.get("GIT_NO_LAZY_FETCH"), + Some(&Some("1".to_string())) + ); assert_eq!(command.timeout, DIFF_COMMAND_TIMEOUT); assert_eq!(command.disable_output_cap, true); } diff --git a/codex-rs/tui/src/workspace_command.rs b/codex-rs/tui/src/workspace_command.rs index 02495d66ddba..bd9a5dcacf08 100644 --- a/codex-rs/tui/src/workspace_command.rs +++ b/codex-rs/tui/src/workspace_command.rs @@ -62,6 +62,15 @@ impl WorkspaceCommand { } } + /// Creates a Git command that is restricted to already-local repository data. + pub(crate) fn local_only_git(argv: impl IntoIterator>) -> Self { + let mut command = Self::new(argv); + for (key, value) in codex_git_utils::local_only_git_env() { + command = command.env(key, value); + } + command + } + /// Sets the command working directory. pub(crate) fn cwd(mut self, cwd: impl Into) -> Self { self.cwd = Some(cwd.into());