mirror of
https://github.com/openai/codex.git
synced 2026-09-28 16:53:06 +08:00
Prevent launch context from reaching child processes (#37607)
## Why Model-reachable child processes should not inherit Codex launch context. ## What changed - Treat `OPENAI_FEDERATION_RULE_ID` and `OPENAI_IDENTITY_TOKEN_FILE` as non-inheritable environment variables, with case-insensitive matching. - Remove them after shell environment policy overrides and before spawning commands across execution, MCP, hooks, Git helpers, and remote helper processes. ## Testing - Cover inherited and explicitly configured variants, including mixed-case names. - Verify the variables are absent from real child environments and app-server command and process execution. GitOrigin-RevId: 2535527893985fef0995617f4c5b2462bea7c136
This commit is contained in:
@@ -1,4 +1,5 @@
|
||||
use super::*;
|
||||
use codex_protocol::shell_environment::is_non_inheritable_env_var;
|
||||
|
||||
#[derive(Clone)]
|
||||
pub(crate) struct CommandExecRequestProcessor {
|
||||
@@ -163,6 +164,7 @@ impl CommandExecRequestProcessor {
|
||||
}
|
||||
}
|
||||
}
|
||||
env.retain(|name, _| !is_non_inheritable_env_var(name));
|
||||
let timeout_ms = match timeout_ms {
|
||||
Some(timeout_ms) => match u64::try_from(timeout_ms) {
|
||||
Ok(timeout_ms) => Some(timeout_ms),
|
||||
|
||||
@@ -25,6 +25,7 @@ use codex_core::exec::ExecExpirationOutcome;
|
||||
use codex_core::exec::IO_DRAIN_TIMEOUT_MS;
|
||||
use codex_exec_server::EnvironmentManager;
|
||||
use codex_protocol::exec_output::bytes_to_string_smart;
|
||||
use codex_protocol::shell_environment::is_non_inheritable_env_var;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_pty::DEFAULT_OUTPUT_BYTES_CAP;
|
||||
use codex_utils_pty::ProcessHandle;
|
||||
@@ -107,6 +108,7 @@ impl ProcessExecRequestProcessor {
|
||||
}
|
||||
}
|
||||
}
|
||||
env.retain(|name, _| !is_non_inheritable_env_var(name));
|
||||
let expiration = match timeout_ms {
|
||||
Some(Some(timeout_ms)) => match u64::try_from(timeout_ms) {
|
||||
Ok(timeout_ms) => timeout_ms.into(),
|
||||
|
||||
@@ -18,6 +18,7 @@ use codex_app_server_protocol::RequestId;
|
||||
use codex_app_server_protocol::SandboxPolicy;
|
||||
use codex_exec_server::CODEX_EXEC_SERVER_URL_ENV_VAR;
|
||||
use codex_protocol::models::BUILT_IN_PERMISSION_PROFILE_READ_ONLY;
|
||||
use codex_protocol::shell_environment::OPENAI_FEDERATION_RULE_ID_ENV_VAR;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::collections::HashMap;
|
||||
use std::path::Path;
|
||||
@@ -151,7 +152,7 @@ async fn command_exec_env_overrides_merge_with_server_environment_and_support_un
|
||||
command: vec![
|
||||
"/bin/sh".to_string(),
|
||||
"-lc".to_string(),
|
||||
"printf '%s|%s|%s|%s' \"$COMMAND_EXEC_BASELINE\" \"$COMMAND_EXEC_EXTRA\" \"${RUST_LOG-unset}\" \"$CODEX_HOME\"".to_string(),
|
||||
"printf '%s|%s|%s|%s|%s|%s' \"$COMMAND_EXEC_BASELINE\" \"$COMMAND_EXEC_EXTRA\" \"${RUST_LOG-unset}\" \"$CODEX_HOME\" \"$OPENAI_FEDERATION_RULE_ID\" \"$openai_identity_token_file\"".to_string(),
|
||||
],
|
||||
process_id: None,
|
||||
tty: false,
|
||||
@@ -169,6 +170,14 @@ async fn command_exec_env_overrides_merge_with_server_environment_and_support_un
|
||||
),
|
||||
("COMMAND_EXEC_EXTRA".to_string(), Some("added".to_string())),
|
||||
("RUST_LOG".to_string(), None),
|
||||
(
|
||||
OPENAI_FEDERATION_RULE_ID_ENV_VAR.to_string(),
|
||||
Some("rule".to_string()),
|
||||
),
|
||||
(
|
||||
"openai_identity_token_file".to_string(),
|
||||
Some("/run/identity-token".to_string()),
|
||||
),
|
||||
])),
|
||||
size: None,
|
||||
sandbox_policy: None,
|
||||
@@ -181,7 +190,7 @@ async fn command_exec_env_overrides_merge_with_server_environment_and_support_un
|
||||
response,
|
||||
CommandExecResponse {
|
||||
exit_code: 0,
|
||||
stdout: format!("request|added|unset|{}", codex_home.path().display()),
|
||||
stdout: format!("request|added|unset|{}||", codex_home.path().display()),
|
||||
stderr: String::new(),
|
||||
}
|
||||
);
|
||||
|
||||
@@ -42,7 +42,7 @@ async fn process_spawn_returns_before_exit_and_emits_exit_notification() -> Resu
|
||||
"while (!(Test-Path -LiteralPath $env:CODEX_PROCESS_EXEC_RELEASE_FILE)) { ",
|
||||
"Start-Sleep -Milliseconds 20 ",
|
||||
"}; ",
|
||||
"[Console]::Out.Write('process-out'); ",
|
||||
"[Console]::Out.Write(('process-out|{0}|{1}' -f $env:OpenAI_Federation_Rule_Id, $env:OPENAI_IDENTITY_TOKEN_FILE)); ",
|
||||
"[Console]::Error.Write('process-err')",
|
||||
)
|
||||
.to_string(),
|
||||
@@ -54,7 +54,7 @@ async fn process_spawn_returns_before_exit_and_emits_exit_notification() -> Resu
|
||||
concat!(
|
||||
"printf process > \"$CODEX_PROCESS_EXEC_PROBE_FILE\"; ",
|
||||
"while [ ! -e \"$CODEX_PROCESS_EXEC_RELEASE_FILE\" ]; do sleep 0.05; done; ",
|
||||
"printf process-out; ",
|
||||
"printf 'process-out|%s|%s' \"$OpenAI_Federation_Rule_Id\" \"$OPENAI_IDENTITY_TOKEN_FILE\"; ",
|
||||
"printf process-err >&2",
|
||||
)
|
||||
.to_string(),
|
||||
@@ -69,6 +69,14 @@ async fn process_spawn_returns_before_exit_and_emits_exit_notification() -> Resu
|
||||
"CODEX_PROCESS_EXEC_RELEASE_FILE".to_string(),
|
||||
Some(release_file.display().to_string()),
|
||||
),
|
||||
(
|
||||
"OpenAI_Federation_Rule_Id".to_string(),
|
||||
Some("rule".to_string()),
|
||||
),
|
||||
(
|
||||
"OPENAI_IDENTITY_TOKEN_FILE".to_string(),
|
||||
Some("/run/identity-token".to_string()),
|
||||
),
|
||||
]);
|
||||
let spawn_request_id = mcp
|
||||
.send_process_spawn_request(ProcessSpawnParams {
|
||||
@@ -94,7 +102,7 @@ async fn process_spawn_returns_before_exit_and_emits_exit_notification() -> Resu
|
||||
ProcessExitedNotification {
|
||||
process_handle,
|
||||
exit_code: 0,
|
||||
stdout: "process-out".to_string(),
|
||||
stdout: "process-out||".to_string(),
|
||||
stdout_cap_reached: false,
|
||||
stderr: "process-err".to_string(),
|
||||
stderr_cap_reached: false,
|
||||
|
||||
@@ -16,6 +16,7 @@ workspace = true
|
||||
codex-code-mode-protocol = { workspace = true }
|
||||
codex-http-client = { workspace = true }
|
||||
codex-install-context = { workspace = true }
|
||||
codex-protocol = { workspace = true }
|
||||
codex-websocket-client = { workspace = true }
|
||||
futures = { workspace = true }
|
||||
tokio = { workspace = true, features = ["io-util", "macros", "net", "process", "rt", "sync", "time"] }
|
||||
@@ -24,6 +25,5 @@ tokio-util = { workspace = true, features = ["rt"] }
|
||||
tracing = { workspace = true }
|
||||
|
||||
[dev-dependencies]
|
||||
codex-protocol = { workspace = true }
|
||||
pretty_assertions = { workspace = true }
|
||||
tokio = { workspace = true, features = ["test-util"] }
|
||||
|
||||
@@ -33,6 +33,7 @@ use codex_code_mode_protocol::host::SESSION_RESOURCE_LIMITS_CAPABILITY;
|
||||
use codex_code_mode_protocol::host::SupportedProtocolVersions;
|
||||
use codex_code_mode_protocol::host::TransportLane;
|
||||
use codex_http_client::HttpClientFactory;
|
||||
use codex_protocol::shell_environment::scrub_non_inheritable_env_vars;
|
||||
use codex_websocket_client::WebSocketConnector;
|
||||
use futures::StreamExt;
|
||||
use tokio::io::AsyncBufReadExt;
|
||||
@@ -215,16 +216,16 @@ impl Connection {
|
||||
let mut command = Command::new(host_program);
|
||||
#[cfg(unix)]
|
||||
command.process_group(0);
|
||||
let mut child = command
|
||||
command
|
||||
.stdin(Stdio::piped())
|
||||
.stdout(Stdio::piped())
|
||||
.stderr(Stdio::piped())
|
||||
.kill_on_drop(true)
|
||||
.spawn()
|
||||
.map_err(|error| ConnectionError::Spawn {
|
||||
host_program: host_program.to_path_buf(),
|
||||
error,
|
||||
})?;
|
||||
.kill_on_drop(true);
|
||||
scrub_non_inheritable_env_vars(command.as_std_mut());
|
||||
let mut child = command.spawn().map_err(|error| ConnectionError::Spawn {
|
||||
host_program: host_program.to_path_buf(),
|
||||
error,
|
||||
})?;
|
||||
|
||||
if let Some(stderr) = child.stderr.take() {
|
||||
tokio::spawn(async move {
|
||||
|
||||
@@ -8,6 +8,7 @@ use tokio::process::Command;
|
||||
use tracing::trace;
|
||||
|
||||
use codex_protocol::permissions::NetworkSandboxPolicy;
|
||||
use codex_protocol::shell_environment::is_non_inheritable_env_var;
|
||||
|
||||
/// Experimental environment variable that will be set to some non-empty value
|
||||
/// if both of the following are true:
|
||||
@@ -60,6 +61,8 @@ pub(crate) async fn spawn_child_async(request: SpawnChildRequest<'_>) -> std::io
|
||||
mut env,
|
||||
} = request;
|
||||
|
||||
env.retain(|name, _| !is_non_inheritable_env_var(name));
|
||||
|
||||
trace!(
|
||||
"spawn_child_async: {program:?} {args:?} {arg0:?} {cwd:?} {network_sandbox_policy:?} {stdio_policy:?} {env:?}"
|
||||
);
|
||||
|
||||
@@ -23,6 +23,7 @@ pub(crate) use codex_network_proxy::is_managed_proxy_env_var;
|
||||
pub(crate) use codex_network_proxy::strip_managed_proxy_env;
|
||||
use codex_protocol::config_types::WindowsSandboxLevel;
|
||||
use codex_protocol::models::AdditionalPermissionProfile;
|
||||
use codex_protocol::shell_environment::is_non_inheritable_env_var;
|
||||
use codex_sandboxing::SandboxCommand;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
@@ -304,6 +305,7 @@ fn build_override_exports(
|
||||
.keys()
|
||||
.map(String::as_str)
|
||||
.chain(restore_even_when_absent.iter().copied())
|
||||
.filter(|key| !is_non_inheritable_env_var(key))
|
||||
.filter(|key| is_valid_shell_variable_name(key))
|
||||
.collect::<Vec<_>>();
|
||||
keys.sort_unstable();
|
||||
|
||||
@@ -1082,10 +1082,16 @@ fn maybe_wrap_shell_lc_with_snapshot_does_not_embed_override_values_in_argv() {
|
||||
"-lc".to_string(),
|
||||
"printf '%s' \"$OPENAI_API_KEY\"".to_string(),
|
||||
];
|
||||
let explicit_env_overrides = HashMap::from([(
|
||||
"OPENAI_API_KEY".to_string(),
|
||||
"super-secret-value".to_string(),
|
||||
)]);
|
||||
let explicit_env_overrides = HashMap::from([
|
||||
(
|
||||
"OPENAI_API_KEY".to_string(),
|
||||
"super-secret-value".to_string(),
|
||||
),
|
||||
(
|
||||
"openai_identity_token_file".to_string(),
|
||||
"/run/identity-token".to_string(),
|
||||
),
|
||||
]);
|
||||
let rewritten = maybe_wrap_shell_lc_with_snapshot(
|
||||
&command,
|
||||
&session_shell,
|
||||
@@ -1099,6 +1105,7 @@ fn maybe_wrap_shell_lc_with_snapshot_does_not_embed_override_values_in_argv() {
|
||||
);
|
||||
|
||||
assert!(!rewritten[2].contains("super-secret-value"));
|
||||
assert!(!rewritten[2].contains("openai_identity_token_file"));
|
||||
let output = Command::new(&rewritten[0])
|
||||
.args(&rewritten[1..])
|
||||
.env("OPENAI_API_KEY", "super-secret-value")
|
||||
|
||||
@@ -66,6 +66,7 @@ use codex_protocol::error::SandboxErr;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::ExecCommandSource;
|
||||
use codex_protocol::protocol::TerminalInteractionEvent;
|
||||
use codex_protocol::shell_environment::is_non_inheritable_env_var;
|
||||
use codex_sandboxing::SandboxCommand;
|
||||
use codex_tools::ToolName;
|
||||
use codex_utils_output_truncation::approx_tokens_from_byte_count;
|
||||
@@ -123,7 +124,10 @@ fn exec_env_policy_from_shell_policy(
|
||||
.collect::<Vec<_>>();
|
||||
exclude.push(CODEX_PERMISSION_PROFILE_ENV_VAR.to_string());
|
||||
let mut r#set = policy.r#set.clone();
|
||||
r#set.retain(|key, _| !key.eq_ignore_ascii_case(CODEX_PERMISSION_PROFILE_ENV_VAR));
|
||||
r#set.retain(|key, _| {
|
||||
!key.eq_ignore_ascii_case(CODEX_PERMISSION_PROFILE_ENV_VAR)
|
||||
&& !is_non_inheritable_env_var(key)
|
||||
});
|
||||
codex_exec_server::ExecEnvPolicy {
|
||||
inherit: policy.inherit.clone(),
|
||||
ignore_default_excludes: policy.ignore_default_excludes,
|
||||
@@ -144,8 +148,9 @@ fn env_overlay_for_exec_server(
|
||||
request_env
|
||||
.iter()
|
||||
.filter(|(key, value)| {
|
||||
key.as_str() == CODEX_PERMISSION_PROFILE_ENV_VAR
|
||||
|| local_policy_env.get(*key) != Some(*value)
|
||||
!is_non_inheritable_env_var(key)
|
||||
&& (key.as_str() == CODEX_PERMISSION_PROFILE_ENV_VAR
|
||||
|| local_policy_env.get(*key) != Some(*value))
|
||||
})
|
||||
.map(|(key, value)| (key.clone(), value.clone()))
|
||||
.collect()
|
||||
|
||||
@@ -51,6 +51,7 @@ fn env_overlay_for_exec_server_keeps_runtime_changes_only() {
|
||||
let request_env = HashMap::from([
|
||||
("HOME".to_string(), "/client-home".to_string()),
|
||||
("PATH".to_string(), "/sandbox-path".to_string()),
|
||||
("OpenAI_Federation_Rule_Id".to_string(), "rule".to_string()),
|
||||
("SHELL_SET".to_string(), "policy".to_string()),
|
||||
("CODEX_THREAD_ID".to_string(), "thread-1".to_string()),
|
||||
(
|
||||
@@ -81,13 +82,17 @@ fn env_overlay_for_exec_server_keeps_runtime_changes_only() {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn exec_env_policy_excludes_runtime_permission_profile() {
|
||||
fn exec_env_policy_excludes_non_inheritable_and_runtime_variables() {
|
||||
let policy = ShellEnvironmentPolicy {
|
||||
r#set: HashMap::from([
|
||||
(
|
||||
"codex_permission_profile".to_string(),
|
||||
"stale-profile".to_string(),
|
||||
),
|
||||
(
|
||||
"openai_identity_token_file".to_string(),
|
||||
"/run/identity-token".to_string(),
|
||||
),
|
||||
("KEEP".to_string(), "value".to_string()),
|
||||
]),
|
||||
..Default::default()
|
||||
|
||||
@@ -11,6 +11,7 @@ use tracing::debug;
|
||||
use tracing::warn;
|
||||
|
||||
use codex_http_client::HttpClientFactory;
|
||||
use codex_protocol::shell_environment::scrub_non_inheritable_env_vars;
|
||||
use codex_utils_rustls_provider::ensure_rustls_crypto_provider;
|
||||
use codex_websocket_client::WebSocketConnector;
|
||||
use codex_websocket_client::WebSocketTlsMode;
|
||||
@@ -441,6 +442,7 @@ fn stdio_command_process(stdio_command: &StdioExecServerCommand) -> Command {
|
||||
let mut command = Command::new(&stdio_command.program);
|
||||
command.args(&stdio_command.args);
|
||||
command.envs(&stdio_command.env);
|
||||
scrub_non_inheritable_env_vars(command.as_std_mut());
|
||||
if let Some(cwd) = &stdio_command.cwd {
|
||||
command.current_dir(cwd);
|
||||
}
|
||||
|
||||
@@ -327,7 +327,7 @@ fn spawn_command(
|
||||
SandboxExecRequest {
|
||||
command: argv,
|
||||
cwd,
|
||||
env,
|
||||
mut env,
|
||||
arg0,
|
||||
..
|
||||
}: SandboxExecRequest,
|
||||
@@ -346,6 +346,7 @@ fn spawn_command(
|
||||
// TODO(anp): Keep PathUri through the filesystem helper launch boundary.
|
||||
let cwd = cwd.to_abs_path().map_err(io_error)?;
|
||||
command.current_dir(cwd.as_path());
|
||||
env.retain(|name, _| !codex_protocol::shell_environment::is_non_inheritable_env_var(name));
|
||||
command.env_clear();
|
||||
command.envs(env);
|
||||
command.stdin(std::process::Stdio::piped());
|
||||
|
||||
@@ -649,6 +649,7 @@ fn child_env(params: &ExecParams) -> HashMap<String, String> {
|
||||
None => params.env.clone(),
|
||||
};
|
||||
env.remove(crate::CODEX_EXEC_SERVER_EXIT_ON_STDIN_CLOSE_ENV_VAR);
|
||||
env.retain(|name, _| !shell_environment::is_non_inheritable_env_var(name));
|
||||
env
|
||||
}
|
||||
|
||||
@@ -1268,12 +1269,19 @@ mod tests {
|
||||
let mut params = test_exec_params(HashMap::from([
|
||||
("OVERLAY".to_string(), "overlay".to_string()),
|
||||
("POLICY_SET".to_string(), "overlay-wins".to_string()),
|
||||
(
|
||||
"openai_identity_token_file".to_string(),
|
||||
"/run/identity-token".to_string(),
|
||||
),
|
||||
]));
|
||||
params.env_policy = Some(ExecEnvPolicy {
|
||||
inherit: ShellEnvironmentPolicyInherit::None,
|
||||
ignore_default_excludes: true,
|
||||
exclude: Vec::new(),
|
||||
r#set: HashMap::from([("POLICY_SET".to_string(), "policy".to_string())]),
|
||||
r#set: HashMap::from([
|
||||
("POLICY_SET".to_string(), "policy".to_string()),
|
||||
("OpenAI_Federation_Rule_Id".to_string(), "rule".to_string()),
|
||||
]),
|
||||
include_only: Vec::new(),
|
||||
});
|
||||
|
||||
|
||||
@@ -124,12 +124,14 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result<ApplyGitResult> {
|
||||
}
|
||||
|
||||
fn resolve_git_root(cwd: &Path) -> io::Result<PathBuf> {
|
||||
let out = std::process::Command::new("git")
|
||||
let mut command = std::process::Command::new("git");
|
||||
command
|
||||
.args(["-c", crate::SAFE_BARE_REPOSITORY_CONFIG])
|
||||
.arg("rev-parse")
|
||||
.arg("--show-toplevel")
|
||||
.current_dir(cwd)
|
||||
.output()?;
|
||||
.current_dir(cwd);
|
||||
crate::scrub_non_inheritable_environment(&mut command);
|
||||
let out = command.output()?;
|
||||
let code = out.status.code().unwrap_or(-1);
|
||||
if code != 0 {
|
||||
return Err(io::Error::other(format!(
|
||||
@@ -158,6 +160,7 @@ fn run_git(cwd: &Path, git_cfg: &[String], args: &[String]) -> io::Result<(i32,
|
||||
for a in args {
|
||||
cmd.arg(a);
|
||||
}
|
||||
crate::scrub_non_inheritable_environment(&mut cmd);
|
||||
let out = cmd.current_dir(cwd).output()?;
|
||||
let code = out.status.code().unwrap_or(-1);
|
||||
let stdout = String::from_utf8_lossy(&out.stdout).into_owned();
|
||||
@@ -338,6 +341,7 @@ pub fn stage_paths(git_root: &Path, diff: &str) -> io::Result<()> {
|
||||
for p in &existing {
|
||||
cmd.arg(OsStr::new(p));
|
||||
}
|
||||
crate::scrub_non_inheritable_environment(&mut cmd);
|
||||
let out = cmd.current_dir(git_root).output()?;
|
||||
let _code = out.status.code().unwrap_or(-1);
|
||||
// We do not hard fail staging; best-effort is OK. Return Ok even on non-zero.
|
||||
|
||||
@@ -29,6 +29,7 @@ impl Drop for KillGitProcessTreeOnDrop {
|
||||
}
|
||||
|
||||
fn spawn_git_command(command: &mut Command) -> Option<(Child, KillGitProcessTreeOnDrop)> {
|
||||
crate::scrub_non_inheritable_environment(command.as_std_mut());
|
||||
#[cfg(unix)]
|
||||
command.process_group(0);
|
||||
command.kill_on_drop(true);
|
||||
|
||||
@@ -48,3 +48,7 @@ pub use info::recent_commits;
|
||||
pub use info::resolve_root_git_project_for_trust;
|
||||
pub use platform::create_symlink;
|
||||
pub use status::get_has_changes_in_repo;
|
||||
|
||||
pub(crate) fn scrub_non_inheritable_environment(command: &mut std::process::Command) {
|
||||
codex_protocol::shell_environment::scrub_non_inheritable_env_vars(command);
|
||||
}
|
||||
|
||||
@@ -120,6 +120,7 @@ where
|
||||
}
|
||||
}
|
||||
command.args(&args_vec);
|
||||
crate::scrub_non_inheritable_environment(&mut command);
|
||||
let output = command.output()?;
|
||||
if !output.status.success() {
|
||||
let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string();
|
||||
|
||||
@@ -8,6 +8,7 @@ use std::time::Duration;
|
||||
use std::time::Instant;
|
||||
|
||||
use async_channel::Sender;
|
||||
use codex_protocol::shell_environment::scrub_non_inheritable_env_vars;
|
||||
#[cfg(windows)]
|
||||
use codex_utils_pty::JobObject;
|
||||
use tokio::io::AsyncWriteExt;
|
||||
@@ -395,6 +396,7 @@ fn build_command(shell: &CommandShell, handler: &ConfiguredHandler) -> Command {
|
||||
command.arg(&handler.command);
|
||||
}
|
||||
command.envs(&handler.env);
|
||||
scrub_non_inheritable_env_vars(command.as_std_mut());
|
||||
command
|
||||
}
|
||||
|
||||
|
||||
@@ -28,6 +28,7 @@ use async_channel::Receiver;
|
||||
use codex_config::ConfigLayerStack;
|
||||
use codex_plugin::PluginHookSource;
|
||||
use codex_protocol::ThreadId;
|
||||
use codex_protocol::shell_environment::scrub_non_inheritable_env_vars;
|
||||
use std::time::Duration;
|
||||
use tokio::process::Command;
|
||||
|
||||
@@ -271,5 +272,6 @@ pub fn command_from_argv(argv: &[String]) -> Option<Command> {
|
||||
}
|
||||
let mut command = Command::new(program);
|
||||
command.args(args);
|
||||
scrub_non_inheritable_env_vars(command.as_std_mut());
|
||||
Some(command)
|
||||
}
|
||||
|
||||
@@ -4,6 +4,44 @@ use crate::config_types::ShellEnvironmentPolicyInherit;
|
||||
use std::collections::HashMap;
|
||||
|
||||
pub const CODEX_THREAD_ID_ENV_VAR: &str = "CODEX_THREAD_ID";
|
||||
pub const OPENAI_FEDERATION_RULE_ID_ENV_VAR: &str = "OPENAI_FEDERATION_RULE_ID";
|
||||
pub const OPENAI_IDENTITY_TOKEN_FILE_ENV_VAR: &str = "OPENAI_IDENTITY_TOKEN_FILE";
|
||||
|
||||
/// Environment variables that model-reachable child processes must not inherit.
|
||||
pub const NON_INHERITABLE_ENV_VARS: &[&str] = &[
|
||||
OPENAI_FEDERATION_RULE_ID_ENV_VAR,
|
||||
OPENAI_IDENTITY_TOKEN_FILE_ENV_VAR,
|
||||
];
|
||||
|
||||
pub fn is_non_inheritable_env_var(name: &str) -> bool {
|
||||
NON_INHERITABLE_ENV_VARS
|
||||
.iter()
|
||||
.any(|restricted| restricted.eq_ignore_ascii_case(name))
|
||||
}
|
||||
|
||||
/// Configures a child command to omit non-inheritable variables from the
|
||||
/// process environment and explicit command overrides.
|
||||
///
|
||||
/// This prevents accidental propagation of Codex launch context; it is not a
|
||||
/// filesystem security boundary for the referenced identity-token file.
|
||||
pub fn scrub_non_inheritable_env_vars(command: &mut std::process::Command) {
|
||||
let configured_names = command
|
||||
.get_envs()
|
||||
.map(|(name, _)| name.to_os_string())
|
||||
.collect::<Vec<_>>();
|
||||
|
||||
for name in NON_INHERITABLE_ENV_VARS {
|
||||
command.env_remove(name);
|
||||
}
|
||||
for name in std::env::vars_os()
|
||||
.map(|(name, _)| name)
|
||||
.chain(configured_names)
|
||||
{
|
||||
if name.to_str().is_some_and(is_non_inheritable_env_var) {
|
||||
command.env_remove(name);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Construct a shell environment from the supplied process environment and
|
||||
/// shell-environment policy.
|
||||
@@ -106,6 +144,10 @@ where
|
||||
env_map.insert(CODEX_THREAD_ID_ENV_VAR.to_string(), thread_id.to_string());
|
||||
}
|
||||
|
||||
// Restricted launch context cannot be restored through user-provided shell
|
||||
// environment overrides.
|
||||
env_map.retain(|name, _| !is_non_inheritable_env_var(name));
|
||||
|
||||
env_map
|
||||
}
|
||||
|
||||
@@ -148,6 +190,10 @@ pub const WINDOWS_CORE_ENV_VARS: &[&str] = &[
|
||||
"PWSH",
|
||||
];
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "shell_environment_tests.rs"]
|
||||
mod tests;
|
||||
|
||||
#[cfg(all(test, target_os = "windows"))]
|
||||
mod windows_tests {
|
||||
use super::*;
|
||||
|
||||
@@ -0,0 +1,84 @@
|
||||
use std::collections::HashMap;
|
||||
use std::process::Command;
|
||||
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
use super::*;
|
||||
|
||||
const CHILD_MODE_ENV_VAR: &str = "CODEX_SHELL_ENVIRONMENT_SCRUBBER_TEST_MODE";
|
||||
const TEST_NAME: &str =
|
||||
"shell_environment::tests::command_scrubber_removes_names_from_real_child_environment";
|
||||
|
||||
#[test]
|
||||
fn non_inheritable_environment_is_removed_after_policy_overrides() {
|
||||
let vars = [
|
||||
("SAFE".to_string(), "inherited".to_string()),
|
||||
(
|
||||
"openai_federation_rule_id".to_string(),
|
||||
"inherited-rule".to_string(),
|
||||
),
|
||||
];
|
||||
let policy = ShellEnvironmentPolicy {
|
||||
inherit: ShellEnvironmentPolicyInherit::All,
|
||||
ignore_default_excludes: true,
|
||||
r#set: HashMap::from([
|
||||
("SAFE".to_string(), "override".to_string()),
|
||||
(
|
||||
"OpenAI_Identity_Token_File".to_string(),
|
||||
"/run/identity-token".to_string(),
|
||||
),
|
||||
]),
|
||||
..Default::default()
|
||||
};
|
||||
|
||||
assert_eq!(
|
||||
populate_env(vars, &policy, /*thread_id*/ None),
|
||||
HashMap::from([("SAFE".to_string(), "override".to_string())])
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn command_scrubber_removes_names_from_real_child_environment() {
|
||||
if std::env::var_os(CHILD_MODE_ENV_VAR).is_none() {
|
||||
let output = Command::new(std::env::current_exe().expect("locate current test binary"))
|
||||
.args([TEST_NAME, "--exact", "--nocapture"])
|
||||
.env(CHILD_MODE_ENV_VAR, "1")
|
||||
.env("OpenAI_Federation_Rule_Id", "inherited-rule")
|
||||
.output()
|
||||
.expect("run inherited-environment test process");
|
||||
assert!(
|
||||
output.status.success(),
|
||||
"child failed\nstdout:\n{}\nstderr:\n{}",
|
||||
String::from_utf8_lossy(&output.stdout),
|
||||
String::from_utf8_lossy(&output.stderr),
|
||||
);
|
||||
return;
|
||||
}
|
||||
|
||||
let mut command = environment_command();
|
||||
command
|
||||
.env("openai_identity_token_file", "/run/identity-token")
|
||||
.env("SAFE", "value");
|
||||
scrub_non_inheritable_env_vars(&mut command);
|
||||
let output = command.output().expect("read child environment");
|
||||
assert!(output.status.success());
|
||||
let stdout = String::from_utf8_lossy(&output.stdout);
|
||||
let restricted_names = stdout
|
||||
.lines()
|
||||
.filter_map(|line| line.split_once('=').map(|(name, _)| name))
|
||||
.filter(|name| is_non_inheritable_env_var(name))
|
||||
.collect::<Vec<_>>();
|
||||
assert_eq!(restricted_names, Vec::<&str>::new());
|
||||
}
|
||||
|
||||
#[cfg(windows)]
|
||||
fn environment_command() -> Command {
|
||||
let mut command = Command::new("cmd.exe");
|
||||
command.args(["/D", "/C", "set"]);
|
||||
command
|
||||
}
|
||||
|
||||
#[cfg(not(windows))]
|
||||
fn environment_command() -> Command {
|
||||
Command::new("env")
|
||||
}
|
||||
@@ -1,6 +1,7 @@
|
||||
use anyhow::Result;
|
||||
use anyhow::anyhow;
|
||||
use codex_config::types::McpServerEnvVar;
|
||||
use codex_protocol::shell_environment::is_non_inheritable_env_var;
|
||||
use http::HeaderMap;
|
||||
use http::HeaderName;
|
||||
use http::HeaderValue;
|
||||
@@ -16,13 +17,17 @@ pub(crate) fn create_env_for_mcp_server(
|
||||
env_vars: &[McpServerEnvVar],
|
||||
) -> Result<HashMap<OsString, OsString>> {
|
||||
let additional_env_vars = local_stdio_env_var_names(env_vars)?;
|
||||
let env = DEFAULT_ENV_VARS
|
||||
let mut env: HashMap<OsString, OsString> = DEFAULT_ENV_VARS
|
||||
.iter()
|
||||
.copied()
|
||||
.chain(additional_env_vars)
|
||||
.filter_map(|var| env::var_os(var).map(|value| (OsString::from(var), value)))
|
||||
.chain(extra_env.unwrap_or_default())
|
||||
.collect();
|
||||
env.retain(|name, _| {
|
||||
name.to_str()
|
||||
.is_none_or(|name| !is_non_inheritable_env_var(name))
|
||||
});
|
||||
Ok(env)
|
||||
}
|
||||
|
||||
@@ -33,18 +38,24 @@ pub(crate) fn create_env_overlay_for_remote_mcp_server(
|
||||
// Remote stdio should inherit PATH/HOME/etc. from the executor side, not
|
||||
// from the orchestrator process. Only forward variables explicitly named
|
||||
// by the MCP config plus literal env overrides from that config.
|
||||
env_vars
|
||||
let mut env: HashMap<OsString, OsString> = env_vars
|
||||
.iter()
|
||||
.filter(|var| !var.is_remote_source())
|
||||
.filter_map(|var| env::var_os(var.name()).map(|value| (OsString::from(var.name()), value)))
|
||||
.chain(extra_env.unwrap_or_default())
|
||||
.collect()
|
||||
.collect();
|
||||
env.retain(|name, _| {
|
||||
name.to_str()
|
||||
.is_none_or(|name| !is_non_inheritable_env_var(name))
|
||||
});
|
||||
env
|
||||
}
|
||||
|
||||
pub(crate) fn remote_mcp_env_var_names(env_vars: &[McpServerEnvVar]) -> Vec<String> {
|
||||
env_vars
|
||||
.iter()
|
||||
.filter(|var| var.is_remote_source())
|
||||
.filter(|var| !is_non_inheritable_env_var(var.name()))
|
||||
.map(|var| var.name().to_string())
|
||||
.collect()
|
||||
}
|
||||
@@ -56,7 +67,10 @@ fn local_stdio_env_var_names(env_vars: &[McpServerEnvVar]) -> Result<impl Iterat
|
||||
remote_var.name()
|
||||
));
|
||||
}
|
||||
Ok(env_vars.iter().map(McpServerEnvVar::name))
|
||||
Ok(env_vars
|
||||
.iter()
|
||||
.map(McpServerEnvVar::name)
|
||||
.filter(|name| !is_non_inheritable_env_var(name)))
|
||||
}
|
||||
|
||||
pub(crate) fn build_default_headers(
|
||||
@@ -182,11 +196,18 @@ mod tests {
|
||||
let value = "custom".to_string();
|
||||
let expected = OsString::from(&value);
|
||||
let env = create_env_for_mcp_server(
|
||||
Some(HashMap::from([(OsString::from("TZ"), expected.clone())])),
|
||||
Some(HashMap::from([
|
||||
(OsString::from("TZ"), expected.clone()),
|
||||
(
|
||||
OsString::from("openai_identity_token_file"),
|
||||
OsString::from("/run/identity-token"),
|
||||
),
|
||||
])),
|
||||
&[],
|
||||
)
|
||||
.expect("local MCP env should build");
|
||||
assert_eq!(env.get(OsStr::new("TZ")), Some(&expected));
|
||||
assert!(!env.contains_key(OsStr::new("openai_identity_token_file")));
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -210,8 +231,13 @@ mod tests {
|
||||
let _default_guard = EnvVarGuard::set(default_var, "from-default");
|
||||
let _custom_guard = EnvVarGuard::set(custom_var, &custom_value);
|
||||
|
||||
let env =
|
||||
create_env_overlay_for_remote_mcp_server(/*extra_env*/ None, &[custom_var.into()]);
|
||||
let env = create_env_overlay_for_remote_mcp_server(
|
||||
Some(HashMap::from([(
|
||||
OsString::from("OpenAI_Federation_Rule_Id"),
|
||||
OsString::from("rule"),
|
||||
)])),
|
||||
&[custom_var.into()],
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
env,
|
||||
@@ -260,6 +286,10 @@ mod tests {
|
||||
name: "REMOTE".to_string(),
|
||||
source: Some("remote".to_string()),
|
||||
},
|
||||
McpServerEnvVar::Config {
|
||||
name: "openai_identity_token_file".to_string(),
|
||||
source: Some("remote".to_string()),
|
||||
},
|
||||
]);
|
||||
|
||||
assert_eq!(names, vec!["REMOTE".to_string()]);
|
||||
|
||||
@@ -113,7 +113,8 @@ struct PowershellParserProcess {
|
||||
|
||||
impl PowershellParserProcess {
|
||||
fn spawn(executable: &str) -> std::io::Result<Self> {
|
||||
let mut child = Command::new(executable)
|
||||
let mut command = Command::new(executable);
|
||||
command
|
||||
.args([
|
||||
"-NoLogo",
|
||||
"-NoProfile",
|
||||
@@ -123,8 +124,9 @@ impl PowershellParserProcess {
|
||||
])
|
||||
.stdin(Stdio::piped())
|
||||
.stdout(Stdio::piped())
|
||||
.stderr(Stdio::null())
|
||||
.spawn()?;
|
||||
.stderr(Stdio::null());
|
||||
codex_protocol::shell_environment::scrub_non_inheritable_env_vars(&mut command);
|
||||
let mut child = command.spawn()?;
|
||||
let stdin = match take_child_stdin(&mut child) {
|
||||
Ok(stdin) => stdin,
|
||||
Err(error) => {
|
||||
|
||||
@@ -9,6 +9,7 @@ use std::sync::Mutex;
|
||||
use std::time::Duration;
|
||||
|
||||
use anyhow::Context as _;
|
||||
use codex_protocol::shell_environment::scrub_non_inheritable_env_vars;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use socket2::Socket;
|
||||
use tokio::process::Command;
|
||||
@@ -341,6 +342,7 @@ async fn handle_escalate_session_with_policy(
|
||||
.stdout(Stdio::null())
|
||||
.stderr(Stdio::null())
|
||||
.kill_on_drop(true);
|
||||
scrub_non_inheritable_env_vars(command.as_std_mut());
|
||||
unsafe {
|
||||
command.pre_exec(move || {
|
||||
for (dst_fd, src_fd) in msg.fds.iter().zip(&fds) {
|
||||
|
||||
Reference in New Issue
Block a user