From ec69e784c20c9376b1480dc0f670319f58e1292a Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Tue, 30 Jun 2026 11:54:44 -0700 Subject: [PATCH] Handle Windows environment overrides safely --- codex-rs/app-server/README.md | 3 +- codex-rs/app-server/src/request_processors.rs | 1 + .../command_exec_processor.rs | 13 +- .../environment_overrides.rs | 98 +++++++ .../environment_overrides_tests.rs | 180 ++++++++++++ .../process_exec_processor.rs | 13 +- .../app-server/tests/suite/v2/command_exec.rs | 273 ++++++++++++++++++ 7 files changed, 560 insertions(+), 21 deletions(-) create mode 100644 codex-rs/app-server/src/request_processors/environment_overrides.rs create mode 100644 codex-rs/app-server/src/request_processors/environment_overrides_tests.rs 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;