diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index c9ac2f5fb2..330b6bea37 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -1090,7 +1090,7 @@ Notes: - Empty `command` arrays are rejected. - Prefer `permissionProfile` for command permission overrides. It selects an active profile by id (for example `:read-only`, `:workspace`, or a user-defined `[permissions.]` profile) rather than accepting low-level filesystem/network permissions. The legacy `sandboxPolicy` field accepts the same shape used by `turn/start` (e.g., `dangerFullAccess`, `readOnly`, `workspaceWrite` with flags, `externalSandbox` with `networkAccess` `restricted|enabled`), but cannot be combined with `permissionProfile`. -- `env` merges into the environment produced by the server's shell environment policy. Matching names are overridden; unspecified variables are left intact. +- `env` merges into the environment produced by the server's shell environment policy. Matching names are overridden; unspecified variables are left intact. On Windows, ASCII names are matched case-insensitively, and requests containing multiple ASCII-case-equivalent names are rejected. - When omitted, `timeoutMs` falls back to the server default. - When omitted, `outputBytesCap` falls back to the server default of 1 MiB per stream. - `disableOutputCap: true` disables stdout/stderr capture truncation for that `command/exec` request. It cannot be combined with `outputBytesCap`. @@ -1225,6 +1225,7 @@ For interactive or streaming processes, set `tty: true` or `streamStdoutStderr: - Empty `command` arrays and empty `processHandle` strings are rejected. - `cwd` is required and must be absolute. - `process/spawn` is intentionally unsandboxed and does not define sandbox-selection fields such as `sandboxPolicy` or `permissionProfile`. +- `env` overrides and unsets inherited variables by exact name on Unix. On Windows, ASCII names are matched case-insensitively, and requests containing multiple ASCII-case-equivalent names are rejected. - Duplicate active `processHandle` values are rejected on the same connection; the same handle can be reused after the prior process exits. - `tty: true` implies PTY mode plus `streamStdin: true` and `streamStdoutStderr: true`. - `process/writeStdin` accepts either `deltaBase64`, `closeStdin`, or both. diff --git a/codex-rs/app-server/src/request_processors.rs b/codex-rs/app-server/src/request_processors.rs index 587325c246..78ad1228ab 100644 --- a/codex-rs/app-server/src/request_processors.rs +++ b/codex-rs/app-server/src/request_processors.rs @@ -493,6 +493,7 @@ mod apps_processor; mod catalog_processor; mod command_exec_processor; mod config_processor; +mod environment_overrides; mod environment_processor; mod external_agent_config_processor; mod external_agent_session_import; diff --git a/codex-rs/app-server/src/request_processors/command_exec_processor.rs b/codex-rs/app-server/src/request_processors/command_exec_processor.rs index dfe7555d58..cf9e9b932a 100644 --- a/codex-rs/app-server/src/request_processors/command_exec_processor.rs +++ b/codex-rs/app-server/src/request_processors/command_exec_processor.rs @@ -1,3 +1,4 @@ +use super::environment_overrides::apply_environment_overrides; use super::*; #[derive(Clone)] @@ -152,16 +153,8 @@ impl CommandExecRequestProcessor { /*thread_id*/ None, ); if let Some(env_overrides) = env_overrides { - for (key, value) in env_overrides { - match value { - Some(value) => { - env.insert(key, value); - } - None => { - env.remove(&key); - } - } - } + apply_environment_overrides(&mut env, env_overrides) + .map_err(|error| invalid_params(format!("command/exec env: {error}")))?; } let timeout_ms = match timeout_ms { Some(timeout_ms) => match u64::try_from(timeout_ms) { diff --git a/codex-rs/app-server/src/request_processors/environment_overrides.rs b/codex-rs/app-server/src/request_processors/environment_overrides.rs new file mode 100644 index 0000000000..d9c7fb7476 --- /dev/null +++ b/codex-rs/app-server/src/request_processors/environment_overrides.rs @@ -0,0 +1,98 @@ +use std::collections::HashMap; +use std::fmt; + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum EnvironmentNameSemantics { + CaseSensitive, + WindowsAsciiCaseInsensitive, +} + +impl EnvironmentNameSemantics { + const fn for_target() -> Self { + if cfg!(windows) { + Self::WindowsAsciiCaseInsensitive + } else { + Self::CaseSensitive + } + } +} + +#[derive(Clone, Debug, Eq, PartialEq)] +pub(super) struct EnvironmentOverrideError { + first_key: String, + second_key: String, +} + +impl fmt::Display for EnvironmentOverrideError { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + formatter, + "environment override keys '{}' and '{}' are equivalent on Windows", + self.first_key, self.second_key + ) + } +} + +pub(super) fn apply_environment_overrides( + env: &mut HashMap, + overrides: HashMap>, +) -> Result<(), EnvironmentOverrideError> { + apply_environment_overrides_with_semantics( + env, + overrides, + EnvironmentNameSemantics::for_target(), + ) +} + +fn apply_environment_overrides_with_semantics( + env: &mut HashMap, + overrides: HashMap>, + semantics: EnvironmentNameSemantics, +) -> Result<(), EnvironmentOverrideError> { + match semantics { + EnvironmentNameSemantics::CaseSensitive => { + for (key, value) in overrides { + match value { + Some(value) => { + env.insert(key, value); + } + None => { + env.remove(&key); + } + } + } + } + EnvironmentNameSemantics::WindowsAsciiCaseInsensitive => { + validate_windows_override_keys(&overrides)?; + for (key, value) in overrides { + env.retain(|existing_key, _| !existing_key.eq_ignore_ascii_case(&key)); + if let Some(value) = value { + env.insert(key, value); + } + } + } + } + Ok(()) +} + +fn validate_windows_override_keys( + overrides: &HashMap>, +) -> Result<(), EnvironmentOverrideError> { + let mut keys = overrides.keys().collect::>(); + keys.sort_unstable(); + for (index, first_key) in keys.iter().enumerate() { + for second_key in &keys[index + 1..] { + if first_key.eq_ignore_ascii_case(second_key) { + return Err(EnvironmentOverrideError { + first_key: (*first_key).clone(), + second_key: (*second_key).clone(), + }); + } + } + } + Ok(()) +} + +#[cfg(test)] +#[path = "environment_overrides_tests.rs"] +mod tests; diff --git a/codex-rs/app-server/src/request_processors/environment_overrides_tests.rs b/codex-rs/app-server/src/request_processors/environment_overrides_tests.rs new file mode 100644 index 0000000000..e6e8e64018 --- /dev/null +++ b/codex-rs/app-server/src/request_processors/environment_overrides_tests.rs @@ -0,0 +1,180 @@ +use super::*; +use pretty_assertions::assert_eq; + +fn string_map(entries: [(&str, &str); N]) -> HashMap { + entries + .into_iter() + .map(|(key, value)| (key.to_string(), value.to_string())) + .collect() +} + +fn override_map( + entries: [(&str, Option<&str>); N], +) -> HashMap> { + entries + .into_iter() + .map(|(key, value)| (key.to_string(), value.map(std::string::ToString::to_string))) + .collect() +} + +fn apply_legacy_case_sensitive_overrides( + env: &mut HashMap, + overrides: HashMap>, +) { + for (key, value) in overrides { + match value { + Some(value) => { + env.insert(key, value); + } + None => { + env.remove(&key); + } + } + } +} + +#[test] +fn windows_present_override_replaces_every_case_variant() { + let mut env = string_map([ + ("git_allow_protocol", "ext"), + ("Git_Allow_Protocol", "file"), + ("OTHER", "keep"), + ]); + + apply_environment_overrides_with_semantics( + &mut env, + override_map([("GIT_ALLOW_PROTOCOL", Some(""))]), + EnvironmentNameSemantics::WindowsAsciiCaseInsensitive, + ) + .expect("Windows override should be unambiguous"); + + assert_eq!( + env, + string_map([("GIT_ALLOW_PROTOCOL", ""), ("OTHER", "keep")]) + ); +} + +#[test] +fn windows_unset_removes_every_case_variant() { + let mut env = string_map([("Path", "first"), ("PATH", "second"), ("OTHER", "keep")]); + + apply_environment_overrides_with_semantics( + &mut env, + override_map([("path", None)]), + EnvironmentNameSemantics::WindowsAsciiCaseInsensitive, + ) + .expect("Windows unset should be unambiguous"); + + assert_eq!(env, string_map([("OTHER", "keep")])); +} + +#[test] +fn windows_local_only_overrides_replace_both_permissive_controls() { + let mut env = string_map([ + ("git_allow_protocol", "ext"), + ("git_no_lazy_fetch", "0"), + ("OTHER", "keep"), + ]); + + apply_environment_overrides_with_semantics( + &mut env, + override_map([ + ("GIT_ALLOW_PROTOCOL", Some("")), + ("GIT_NO_LAZY_FETCH", Some("1")), + ]), + EnvironmentNameSemantics::WindowsAsciiCaseInsensitive, + ) + .expect("local-only overrides should be unambiguous"); + + assert_eq!( + env, + string_map([ + ("GIT_ALLOW_PROTOCOL", ""), + ("GIT_NO_LAZY_FETCH", "1"), + ("OTHER", "keep"), + ]) + ); +} + +#[test] +fn windows_rejects_equivalent_request_keys_before_mutation() { + let initial = string_map([("Path", "old"), ("OTHER", "keep")]); + let mut env = initial.clone(); + + let error = apply_environment_overrides_with_semantics( + &mut env, + override_map([("PATH", Some("first")), ("Path", Some("second"))]), + EnvironmentNameSemantics::WindowsAsciiCaseInsensitive, + ) + .expect_err("equivalent Windows request keys should be rejected"); + + assert_eq!( + error, + EnvironmentOverrideError { + first_key: "PATH".to_string(), + second_key: "Path".to_string(), + } + ); + assert_eq!(env, initial); +} + +#[test] +fn case_sensitive_mode_preserves_differently_cased_names() { + let mut env = string_map([("Path", "old"), ("PATH", "replace"), ("OTHER", "keep")]); + + apply_environment_overrides_with_semantics( + &mut env, + override_map([("PATH", None), ("path", Some("new"))]), + EnvironmentNameSemantics::CaseSensitive, + ) + .expect("case-sensitive overrides should not conflict"); + + assert_eq!( + env, + string_map([("Path", "old"), ("path", "new"), ("OTHER", "keep")]) + ); +} + +#[test] +fn case_sensitive_mode_matches_legacy_merge_behavior() { + let fixtures = [ + ( + string_map([("Path", "old"), ("PATH", "replace"), ("OTHER", "keep")]), + override_map([("PATH", None), ("path", Some("new"))]), + ), + ( + string_map([("FOO", "old"), ("BAR", "keep"), ("EMPTY", "old")]), + override_map([ + ("FOO", Some("new")), + ("ADDED", Some("value")), + ("EMPTY", None), + ]), + ), + ]; + + for (initial, overrides) in fixtures { + let mut expected = initial.clone(); + apply_legacy_case_sensitive_overrides(&mut expected, overrides.clone()); + let mut actual = initial; + + apply_environment_overrides_with_semantics( + &mut actual, + overrides, + EnvironmentNameSemantics::CaseSensitive, + ) + .expect("case-sensitive overrides should not conflict"); + + assert_eq!(actual, expected); + } +} + +#[test] +fn target_semantics_matches_platform() { + let expected = if cfg!(windows) { + EnvironmentNameSemantics::WindowsAsciiCaseInsensitive + } else { + EnvironmentNameSemantics::CaseSensitive + }; + + assert_eq!(EnvironmentNameSemantics::for_target(), expected); +} diff --git a/codex-rs/app-server/src/request_processors/process_exec_processor.rs b/codex-rs/app-server/src/request_processors/process_exec_processor.rs index 0b84c7f7b9..aa7a8ec574 100644 --- a/codex-rs/app-server/src/request_processors/process_exec_processor.rs +++ b/codex-rs/app-server/src/request_processors/process_exec_processor.rs @@ -42,6 +42,7 @@ use crate::error_code::invalid_request; use crate::outgoing_message::ConnectionId; use crate::outgoing_message::ConnectionRequestId; use crate::outgoing_message::OutgoingMessageSender; +use crate::request_processors::environment_overrides::apply_environment_overrides; const EXEC_TIMEOUT_EXIT_CODE: i32 = 124; const OUTPUT_CHUNK_SIZE_HINT: usize = 64 * 1024; @@ -96,16 +97,8 @@ impl ProcessExecRequestProcessor { } let mut env = std::env::vars().collect::>(); if let Some(env_overrides) = env_overrides { - for (key, value) in env_overrides { - match value { - Some(value) => { - env.insert(key, value); - } - None => { - env.remove(&key); - } - } - } + apply_environment_overrides(&mut env, env_overrides) + .map_err(|error| invalid_params(format!("{method_name} env: {error}")))?; } let expiration = match timeout_ms { Some(Some(timeout_ms)) => match u64::try_from(timeout_ms) { diff --git a/codex-rs/app-server/tests/suite/v2/command_exec.rs b/codex-rs/app-server/tests/suite/v2/command_exec.rs index 7cad6c0388..3f146c69aa 100644 --- a/codex-rs/app-server/tests/suite/v2/command_exec.rs +++ b/codex-rs/app-server/tests/suite/v2/command_exec.rs @@ -37,6 +37,17 @@ use super::connection_handling_websocket::send_initialize_request; use super::connection_handling_websocket::send_request; use super::connection_handling_websocket::spawn_websocket_server; +#[cfg(windows)] +fn require_absent_windows_environment(names: &[&str]) -> Result<()> { + for name in names { + anyhow::ensure!( + std::env::var_os(name).is_none(), + "cannot establish lowercase Windows environment fixture because {name} is already set" + ); + } + Ok(()) +} + #[tokio::test] async fn command_exec_without_streams_can_be_terminated() -> Result<()> { let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await; @@ -193,6 +204,268 @@ async fn command_exec_env_overrides_merge_with_server_environment_and_support_un Ok(()) } +#[cfg(windows)] +#[tokio::test] +async fn command_exec_env_overrides_use_windows_case_folding() -> Result<()> { + let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await; + let codex_home = TempDir::new()?; + create_config_toml(codex_home.path(), &server.uri(), "never")?; + require_absent_windows_environment(&[ + "GIT_ALLOW_PROTOCOL", + "GIT_NO_LAZY_FETCH", + "CASEFOLD_REMOVE_ME", + ])?; + let mut mcp = TestAppServer::new_with_env( + codex_home.path(), + &[ + ("git_allow_protocol", Some("ext")), + ("git_no_lazy_fetch", Some("0")), + ("casefold_remove_me", Some("inherited")), + ], + ) + .await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let system_root = std::env::var_os("SystemRoot").context("SystemRoot should be set")?; + let powershell = Path::new(&system_root) + .join("System32") + .join("WindowsPowerShell") + .join("v1.0") + .join("powershell.exe"); + let inherited_script = r#" +$entries = [System.Environment]::GetEnvironmentVariables('Process') +function Exact-Keys([string] $name) { + @($entries.Keys | Where-Object { + [string]::Equals( + [string] $_, + $name, + [System.StringComparison]::Ordinal + ) + }) +} +$allowLower = @(Exact-Keys 'git_allow_protocol') +$allowUpper = @(Exact-Keys 'GIT_ALLOW_PROTOCOL') +$noLazyLower = @(Exact-Keys 'git_no_lazy_fetch') +$noLazyUpper = @(Exact-Keys 'GIT_NO_LAZY_FETCH') +$removedLower = @(Exact-Keys 'casefold_remove_me') +$removedUpper = @(Exact-Keys 'CASEFOLD_REMOVE_ME') +if ($allowLower.Count -ne 1 -or [string] $entries[$allowLower[0]] -ne 'ext') { exit 21 } +if ($allowUpper.Count -ne 0) { exit 22 } +if ($noLazyLower.Count -ne 1 -or [string] $entries[$noLazyLower[0]] -ne '0') { exit 23 } +if ($noLazyUpper.Count -ne 0) { exit 24 } +if ($removedLower.Count -ne 1 -or [string] $entries[$removedLower[0]] -ne 'inherited') { exit 25 } +if ($removedUpper.Count -ne 0) { exit 26 } +[Console]::Out.Write('lowercase-inherited') +"#; + let inherited_request_id = mcp + .send_command_exec_request(CommandExecParams { + command: vec![ + powershell.display().to_string(), + "-NoProfile".to_string(), + "-NonInteractive".to_string(), + "-Command".to_string(), + inherited_script.to_string(), + ], + process_id: None, + tty: false, + stream_stdin: false, + stream_stdout_stderr: false, + output_bytes_cap: None, + disable_output_cap: false, + disable_timeout: false, + timeout_ms: None, + cwd: None, + env: None, + size: None, + sandbox_policy: None, + permission_profile: None, + }) + .await?; + + let inherited_response = mcp + .read_stream_until_response_message(RequestId::Integer(inherited_request_id)) + .await?; + let inherited_response: CommandExecResponse = to_response(inherited_response)?; + assert_eq!( + inherited_response, + CommandExecResponse { + exit_code: 0, + stdout: "lowercase-inherited".to_string(), + stderr: String::new(), + } + ); + + let overridden_script = r#" +$entries = [System.Environment]::GetEnvironmentVariables('Process') +function Matching-Keys([string] $name) { + @($entries.Keys | Where-Object { + [string]::Equals( + [string] $_, + $name, + [System.StringComparison]::OrdinalIgnoreCase + ) + }) +} +$allow = @(Matching-Keys 'GIT_ALLOW_PROTOCOL') +$noLazy = @(Matching-Keys 'GIT_NO_LAZY_FETCH') +$removed = @(Matching-Keys 'CASEFOLD_REMOVE_ME') +if ($allow.Count -ne 1 -or [string] $entries[$allow[0]] -ne '') { exit 11 } +if ($noLazy.Count -ne 1 -or [string] $entries[$noLazy[0]] -ne '1') { exit 12 } +if ($removed.Count -ne 0) { exit 13 } +[Console]::Out.Write('casefold-ok') +"#; + let command_request_id = mcp + .send_command_exec_request(CommandExecParams { + command: vec![ + powershell.display().to_string(), + "-NoProfile".to_string(), + "-NonInteractive".to_string(), + "-Command".to_string(), + overridden_script.to_string(), + ], + process_id: None, + tty: false, + stream_stdin: false, + stream_stdout_stderr: false, + output_bytes_cap: None, + disable_output_cap: false, + disable_timeout: false, + timeout_ms: None, + cwd: None, + env: Some(HashMap::from([ + ("GIT_ALLOW_PROTOCOL".to_string(), Some(String::new())), + ("GIT_NO_LAZY_FETCH".to_string(), Some("1".to_string())), + ("CASEFOLD_REMOVE_ME".to_string(), None), + ])), + size: None, + sandbox_policy: None, + permission_profile: None, + }) + .await?; + + let response = mcp + .read_stream_until_response_message(RequestId::Integer(command_request_id)) + .await?; + let response: CommandExecResponse = to_response(response)?; + assert_eq!( + response, + CommandExecResponse { + exit_code: 0, + stdout: "casefold-ok".to_string(), + stderr: String::new(), + } + ); + + Ok(()) +} + +#[cfg(windows)] +#[tokio::test] +async fn command_exec_empty_git_allowlist_blocks_local_file_transport_on_windows() -> Result<()> { + let repository_root = TempDir::new()?; + let repository = repository_root.path().join("remote.git"); + let init = std::process::Command::new("git") + .args(["init", "-q", "--bare"]) + .arg(&repository) + .output() + .context("initialize bare Git repository")?; + anyhow::ensure!( + init.status.success(), + "git init failed: {}", + String::from_utf8_lossy(&init.stderr) + ); + let repository_url = url::Url::from_file_path(&repository) + .map_err(|()| anyhow::anyhow!("convert repository path to file URL"))? + .to_string(); + + let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await; + let codex_home = TempDir::new()?; + create_config_toml(codex_home.path(), &server.uri(), "never")?; + require_absent_windows_environment(&["GIT_ALLOW_PROTOCOL", "GIT_NO_LAZY_FETCH"])?; + let mut mcp = TestAppServer::new_with_env( + codex_home.path(), + &[ + ("git_allow_protocol", Some("file")), + ("git_no_lazy_fetch", Some("0")), + ], + ) + .await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let control_request_id = mcp + .send_command_exec_request(CommandExecParams { + command: vec![ + "git".to_string(), + "ls-remote".to_string(), + repository_url.clone(), + ], + process_id: None, + tty: false, + stream_stdin: false, + stream_stdout_stderr: false, + output_bytes_cap: None, + disable_output_cap: false, + disable_timeout: false, + timeout_ms: None, + cwd: None, + env: None, + size: None, + sandbox_policy: None, + permission_profile: None, + }) + .await?; + + let control_response = mcp + .read_stream_until_response_message(RequestId::Integer(control_request_id)) + .await?; + let control_response: CommandExecResponse = to_response(control_response)?; + assert_eq!( + control_response, + CommandExecResponse { + exit_code: 0, + stdout: String::new(), + stderr: String::new(), + }, + "inherited lowercase allowlist should permit file:// through command/exec" + ); + + let command_request_id = mcp + .send_command_exec_request(CommandExecParams { + command: vec!["git".to_string(), "ls-remote".to_string(), repository_url], + process_id: None, + tty: false, + stream_stdin: false, + stream_stdout_stderr: false, + output_bytes_cap: None, + disable_output_cap: false, + disable_timeout: false, + timeout_ms: None, + cwd: None, + env: Some(HashMap::from([ + ("GIT_ALLOW_PROTOCOL".to_string(), Some(String::new())), + ("GIT_NO_LAZY_FETCH".to_string(), Some("1".to_string())), + ])), + size: None, + sandbox_policy: None, + permission_profile: None, + }) + .await?; + + let response = mcp + .read_stream_until_response_message(RequestId::Integer(command_request_id)) + .await?; + let response: CommandExecResponse = to_response(response)?; + assert_ne!(response.exit_code, 0, "empty allowlist should deny file://"); + assert_eq!(response.stdout, ""); + assert!( + response.stderr.contains("transport 'file' not allowed"), + "expected Git transport denial, got: {}", + response.stderr + ); + + Ok(()) +} + #[tokio::test] async fn command_exec_accepts_permission_profile() -> Result<()> { let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await; diff --git a/codex-rs/git-utils/src/apply.rs b/codex-rs/git-utils/src/apply.rs index b9adb82e6b..92237ad797 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 { @@ -592,6 +598,10 @@ fn regex_ci(pat: &str) -> Regex { Regex::new(&format!("(?i){pat}")).unwrap_or_else(|e| panic!("invalid regex: {e}")) } +#[cfg(all(test, unix))] +#[path = "apply_transport_tests.rs"] +mod transport_tests; + #[cfg(test)] mod tests { use super::*; diff --git a/codex-rs/git-utils/src/apply_transport_tests.rs b/codex-rs/git-utils/src/apply_transport_tests.rs new file mode 100644 index 0000000000..809804a24f --- /dev/null +++ b/codex-rs/git-utils/src/apply_transport_tests.rs @@ -0,0 +1,154 @@ +use super::ApplyGitRequest; +use super::apply_git_patch; +use pretty_assertions::assert_eq; +use std::fs; +use std::os::unix::fs::PermissionsExt; +use std::path::Path; +use std::process::Command; +use std::process::Output; + +#[test] +fn three_way_apply_does_not_lazy_fetch_promisor_objects() { + let temp_dir = tempfile::tempdir().expect("create temp dir"); + let source = temp_dir.path().join("source"); + let partial = temp_dir.path().join("partial"); + 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"), "one\nbase\nthree\n").expect("write base blob"); + run_git(&source, &["add", "data.txt"]); + commit_all(&source, "base"); + let base_blob = run_git_stdout(&source, &["rev-parse", "HEAD:data.txt"]); + + fs::write(source.join("data.txt"), "one\nbase\npatched\n").expect("write patch blob"); + let patch = String::from_utf8(run_git(&source, &["diff", "--full-index", "--binary"]).stdout) + .expect("patch should be UTF-8"); + run_git(&source, &["checkout", "-q", "--", "data.txt"]); + + fs::write(source.join("data.txt"), "current\nbase\nthree\n").expect("write head blob"); + commit_all(&source, "head"); + + let complete_result = apply_git_patch(&ApplyGitRequest { + cwd: source.clone(), + diff: patch.clone(), + revert: false, + preflight: false, + }) + .expect("apply patch in complete clone"); + assert_eq!( + ( + complete_result.exit_code, + fs::read_to_string(source.join("data.txt")).expect("read complete result"), + ), + (0, "current\nbase\npatched\n".to_string()), + "three-way apply should retain normal behavior when every object is local; stderr: {}", + complete_result.stderr, + ); + run_git(&source, &["reset", "-q", "--hard", "HEAD"]); + + 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, + partial.to_str().expect("partial path"), + ], + ); + run_git(&partial, &["checkout", "-q", "main"]); + let missing = run_git_stdout( + &partial, + &["rev-list", "--objects", "--all", "--missing=print"], + ); + assert!( + missing.lines().any(|line| line == format!("?{base_blob}")), + "expected patch base blob {base_blob} to remain missing:\n{missing}", + ); + + 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(&partial, &["config", "remote.origin.url", &helper_url]); + run_git(&partial, &["config", "protocol.ext.allow", "always"]); + + let patch_path = temp_dir.path().join("change.patch"); + fs::write(&patch_path, &patch).expect("write attack reproduction patch"); + let unprotected = Command::new("git") + .args(["apply", "--3way", patch_path.to_str().expect("patch path")]) + .env_remove("GIT_ALLOW_PROTOCOL") + .env_remove("GIT_NO_LAZY_FETCH") + .current_dir(&partial) + .output() + .expect("run unprotected Git apply"); + let helper_marker = helper.with_extension("sh.ran"); + assert_eq!( + (unprotected.status.success(), helper_marker.exists()), + (false, true), + "unprotected three-way apply should reach the selected promisor transport", + ); + fs::remove_file(&helper_marker).expect("remove reproduction marker"); + run_git(&partial, &["reset", "-q", "--hard", "HEAD"]); + + let partial_result = apply_git_patch(&ApplyGitRequest { + cwd: partial, + diff: patch, + revert: false, + preflight: false, + }) + .expect("run fail-closed apply"); + + assert_eq!( + (partial_result.exit_code == 0, helper_marker.exists(),), + (false, false), + "local-only apply must fail without invoking the promisor transport", + ); +} + +fn run_git(cwd: &Path, args: &[&str]) -> Output { + let output = Command::new("git") + .args(args) + .current_dir(cwd) + .output() + .unwrap_or_else(|error| panic!("run git {args:?}: {error}")); + assert!( + output.status.success(), + "git {args:?} failed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); + output +} + +fn run_git_stdout(cwd: &Path, args: &[&str]) -> String { + String::from_utf8_lossy(&run_git(cwd, args).stdout) + .trim() + .to_string() +} + +fn commit_all(cwd: &Path, message: &str) { + run_git( + cwd, + &[ + "-c", + "user.name=Codex Test", + "-c", + "user.email=codex@example.com", + "commit", + "-qam", + message, + ], + ); +} diff --git a/codex-rs/git-utils/src/info.rs b/codex-rs/git-utils/src/info.rs index f6db5db539..8601ab26b5 100644 --- a/codex-rs/git-utils/src/info.rs +++ b/codex-rs/git-utils/src/info.rs @@ -424,7 +424,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, @@ -445,6 +449,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. @@ -908,6 +913,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 e04e604410..d4817d87c8 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 0000000000..042654324a --- /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 f17ce209b0..00f44a6233 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 555b717930..cb4e17298b 100644 --- a/codex-rs/tui/src/branch_summary.rs +++ b/codex-rs/tui/src/branch_summary.rs @@ -440,7 +440,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"), ) @@ -474,10 +474,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![ @@ -880,4 +955,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 ab7764b6b3..b463897c8c 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 02495d66dd..bd9a5dcacf 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());