From 63e4a900c95eb4610ce089d57396f19df854b4f1 Mon Sep 17 00:00:00 2001 From: starr-openai Date: Fri, 17 Apr 2026 13:44:01 -0700 Subject: [PATCH] exec-server: preserve fs helper runtime env (#18380) ## Summary - preserve a small fs-helper runtime env allowlist (`PATH`, temp vars) instead of launching the sandboxed helper with an empty env - add unit coverage for the allowlist and transformed sandbox request env - add a Linux smoke test that starts the test exec-server with a fake `bwrap` on `PATH`, runs a sandboxed fs write through the remote fs helper path, and asserts that bwrap path was exercised ## Validation - `cd /tmp/codex-worktrees/fs-helper-env-defaults/codex-rs && export PATH=$HOME/code/openai/project/dotslash-gen/bin:$HOME/.local/bin:$PATH && bazel test --bes_backend= --bes_results_url= //codex-rs/exec-server:exec-server-file_system-test --test_filter=sandboxed_file_system_helper_finds_bwrap_on_preserved_path` - `cd /tmp/codex-worktrees/fs-helper-env-defaults/codex-rs && export PATH=$HOME/code/openai/project/dotslash-gen/bin:$HOME/.local/bin:$PATH && bazel test --bes_backend= --bes_results_url= //codex-rs/exec-server:exec-server-unit-tests --test_filter="helper_env|sandbox_exec_request_carries_helper_env"` - earlier on this branch before the smoke-test harness adjustment: `cd /tmp/codex-worktrees/fs-helper-env-defaults/codex-rs && export PATH=$HOME/code/openai/project/dotslash-gen/bin:$HOME/.local/bin:$PATH && bazel test --bes_backend= --bes_results_url= //codex-rs/exec-server:all` Co-authored-by: Codex --- codex-rs/exec-server/src/fs_sandbox.rs | 131 +++++++++++++++++- .../exec-server/tests/common/exec_server.rs | 10 ++ codex-rs/exec-server/tests/file_system.rs | 94 +++++++++++++ 3 files changed, 233 insertions(+), 2 deletions(-) diff --git a/codex-rs/exec-server/src/fs_sandbox.rs b/codex-rs/exec-server/src/fs_sandbox.rs index 7b0ac256d..04cded30e 100644 --- a/codex-rs/exec-server/src/fs_sandbox.rs +++ b/codex-rs/exec-server/src/fs_sandbox.rs @@ -27,14 +27,20 @@ use crate::local_file_system::current_sandbox_cwd; use crate::rpc::internal_error; use crate::rpc::invalid_request; +const FS_HELPER_ENV_ALLOWLIST: &[&str] = &["PATH", "TMPDIR", "TMP", "TEMP"]; + #[derive(Clone, Debug)] pub(crate) struct FileSystemSandboxRunner { runtime_paths: ExecServerRuntimePaths, + helper_env: HashMap, } impl FileSystemSandboxRunner { pub(crate) fn new(runtime_paths: ExecServerRuntimePaths) -> Self { - Self { runtime_paths } + Self { + runtime_paths, + helper_env: helper_env(), + } } pub(crate) async fn run( @@ -85,7 +91,7 @@ impl FileSystemSandboxRunner { program: helper.as_path().as_os_str().to_owned(), args: vec![CODEX_FS_HELPER_ARG1.to_string()], cwd: cwd.clone(), - env: HashMap::new(), + env: self.helper_env.clone(), additional_permissions: Some( self.helper_permissions(sandbox_context.additional_permissions.as_ref()), ), @@ -195,6 +201,26 @@ fn normalize_top_level_alias(path: AbsolutePathBuf) -> AbsolutePathBuf { path } +fn helper_env() -> HashMap { + helper_env_from_vars(std::env::vars_os()) +} + +fn helper_env_from_vars( + vars: impl IntoIterator, +) -> HashMap { + vars.into_iter() + .filter_map(|(key, value)| { + let key = key.to_string_lossy(); + helper_env_key_is_allowed(&key) + .then(|| (key.into_owned(), value.to_string_lossy().into_owned())) + }) + .collect() +} + +fn helper_env_key_is_allowed(key: &str) -> bool { + FS_HELPER_ENV_ALLOWLIST.contains(&key) || (cfg!(windows) && key.eq_ignore_ascii_case("PATH")) +} + async fn run_command( command: SandboxExecRequest, request_json: Vec, @@ -286,9 +312,14 @@ fn json_error(err: serde_json::Error) -> JSONRPCErrorError { #[cfg(test)] mod tests { + use std::collections::HashMap; + use std::ffi::OsString; + use codex_protocol::models::FileSystemPermissions; use codex_protocol::models::NetworkPermissions; use codex_protocol::models::PermissionProfile; + use codex_protocol::permissions::FileSystemSandboxPolicy; + use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::ReadOnlyAccess; use codex_protocol::protocol::SandboxPolicy; use codex_utils_absolute_path::AbsolutePathBuf; @@ -297,6 +328,9 @@ mod tests { use crate::ExecServerRuntimePaths; use super::FileSystemSandboxRunner; + use super::helper_env; + use super::helper_env_from_vars; + use super::helper_env_key_is_allowed; use super::sandbox_policy_with_helper_runtime_defaults; #[test] @@ -396,6 +430,99 @@ mod tests { ); } + #[test] + fn helper_env_carries_only_allowlisted_runtime_vars() { + let env = helper_env(); + + let expected = std::env::vars_os() + .filter_map(|(key, value)| { + let key = key.to_string_lossy(); + helper_env_key_is_allowed(&key) + .then(|| (key.into_owned(), value.to_string_lossy().into_owned())) + }) + .collect::>(); + + assert_eq!(env, expected); + } + + #[test] + fn helper_env_preserves_path_for_system_bwrap_discovery_without_leaking_secrets() { + let env = helper_env_from_vars( + [ + ("PATH", "/usr/bin:/bin"), + ("TMPDIR", "/tmp/codex"), + ("TMP", "/tmp"), + ("TEMP", "/tmp"), + ("HOME", "/home/user"), + ("OPENAI_API_KEY", "secret"), + ("HTTPS_PROXY", "http://proxy.example"), + ] + .map(|(key, value)| (OsString::from(key), OsString::from(value))), + ); + + assert_eq!( + env, + HashMap::from([ + ("PATH".to_string(), "/usr/bin:/bin".to_string()), + ("TMPDIR".to_string(), "/tmp/codex".to_string()), + ("TMP".to_string(), "/tmp".to_string()), + ("TEMP".to_string(), "/tmp".to_string()), + ]) + ); + } + + #[cfg(windows)] + #[test] + fn helper_env_preserves_windows_path_key_for_system_bwrap_discovery() { + let env = helper_env_from_vars( + [ + ("Path", r"C:\Windows\System32"), + ("PATH_INJECTION", "bad"), + ("OPENAI_API_KEY", "secret"), + ] + .map(|(key, value)| (OsString::from(key), OsString::from(value))), + ); + + assert_eq!( + env, + HashMap::from([("Path".to_string(), r"C:\Windows\System32".to_string())]) + ); + } + + #[test] + fn sandbox_exec_request_carries_helper_env() { + let Some((path_key, path)) = std::env::vars_os().find(|(key, _)| { + let key = key.to_string_lossy(); + key == "PATH" || (cfg!(windows) && key.eq_ignore_ascii_case("PATH")) + }) else { + return; + }; + let path_key = path_key.to_string_lossy().into_owned(); + let path = path.to_string_lossy().into_owned(); + let codex_self_exe = std::env::current_exe().expect("current exe"); + let runtime_paths = + ExecServerRuntimePaths::new(codex_self_exe.clone(), Some(codex_self_exe)) + .expect("runtime paths"); + let runner = FileSystemSandboxRunner::new(runtime_paths); + let cwd = AbsolutePathBuf::current_dir().expect("cwd"); + let sandbox_policy = SandboxPolicy::new_workspace_write_policy(); + let file_system_policy = + FileSystemSandboxPolicy::from_legacy_sandbox_policy(&sandbox_policy, cwd.as_path()); + let sandbox_context = crate::FileSystemSandboxContext::new(sandbox_policy.clone()); + + let request = runner + .sandbox_exec_request( + &sandbox_policy, + &file_system_policy, + NetworkSandboxPolicy::Restricted, + &cwd, + &sandbox_context, + ) + .expect("sandbox exec request"); + + assert_eq!(request.env.get(&path_key), Some(&path)); + } + #[test] fn helper_permissions_include_helper_read_root_without_additional_permissions() { let codex_self_exe = std::env::current_exe().expect("current exe"); diff --git a/codex-rs/exec-server/tests/common/exec_server.rs b/codex-rs/exec-server/tests/common/exec_server.rs index 2a134546a..4ff740871 100644 --- a/codex-rs/exec-server/tests/common/exec_server.rs +++ b/codex-rs/exec-server/tests/common/exec_server.rs @@ -57,6 +57,15 @@ pub(crate) fn test_codex_helper_paths() -> anyhow::Result } pub(crate) async fn exec_server() -> anyhow::Result { + exec_server_with_env(std::iter::empty::<(&str, &str)>()).await +} + +pub(crate) async fn exec_server_with_env(env: I) -> anyhow::Result +where + I: IntoIterator, + K: AsRef, + V: AsRef, +{ let helper_paths = test_codex_helper_paths()?; let codex_home = TempDir::new()?; let mut child = Command::new(&helper_paths.codex_exe); @@ -66,6 +75,7 @@ pub(crate) async fn exec_server() -> anyhow::Result { child.stderr(Stdio::inherit()); child.kill_on_drop(true); child.env("CODEX_HOME", codex_home.path()); + child.envs(env); let mut child = child.spawn()?; let websocket_url = read_listen_url_from_stdout(&mut child).await?; diff --git a/codex-rs/exec-server/tests/file_system.rs b/codex-rs/exec-server/tests/file_system.rs index 8b9fb90ec..275152ade 100644 --- a/codex-rs/exec-server/tests/file_system.rs +++ b/codex-rs/exec-server/tests/file_system.rs @@ -2,6 +2,8 @@ mod common; +#[cfg(target_os = "linux")] +use std::os::unix::fs::PermissionsExt; use std::os::unix::fs::symlink; use std::path::Path; use std::path::PathBuf; @@ -31,6 +33,8 @@ use test_case::test_case; use common::exec_server::ExecServerHarness; use common::exec_server::TestCodexHelperPaths; use common::exec_server::exec_server; +#[cfg(target_os = "linux")] +use common::exec_server::exec_server_with_env; use common::exec_server::test_codex_helper_paths; struct FileSystemContext { @@ -148,6 +152,96 @@ fn alias_root_candidate() -> Result> { Ok(None) } +#[cfg(target_os = "linux")] +fn write_fake_bwrap(bin_dir: &Path) -> Result { + std::fs::create_dir_all(bin_dir)?; + let fake_bwrap = bin_dir.join("bwrap"); + std::fs::write( + &fake_bwrap, + r#"#!/bin/bash +set -euo pipefail + +for arg in "$@"; do + if [[ "${arg}" == "--help" ]]; then + echo "Usage: bwrap --argv0" + exit 0 + fi +done + +printf '%s\n' "$*" >> "${0}.log" + +args=("$@") +argv0="" +command_start=-1 +for i in "${!args[@]}"; do + if [[ "${args[$i]}" == "--argv0" && $((i + 1)) -lt ${#args[@]} ]]; then + argv0="${args[$((i + 1))]}" + fi + if [[ "${args[$i]}" == "--" ]]; then + command_start=$((i + 1)) + break + fi +done + +if [[ "${command_start}" -lt 0 || "${command_start}" -ge "${#args[@]}" ]]; then + echo "fake bwrap did not find an inner command" >&2 + exit 125 +fi + +cmd=("${args[@]:$command_start}") +if [[ -n "${argv0}" ]]; then + exec -a "${argv0}" "${cmd[@]}" +fi +exec "${cmd[@]}" +"#, + )?; + let mut permissions = std::fs::metadata(&fake_bwrap)?.permissions(); + permissions.set_mode(0o755); + std::fs::set_permissions(&fake_bwrap, permissions)?; + Ok(fake_bwrap) +} + +#[cfg(target_os = "linux")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn sandboxed_file_system_helper_finds_bwrap_on_preserved_path() -> Result<()> { + let tmp = TempDir::new()?; + let fake_bin_dir = tmp.path().join("bin"); + let fake_bwrap = write_fake_bwrap(&fake_bin_dir)?; + let mut path_entries = vec![fake_bin_dir]; + if let Some(path) = std::env::var_os("PATH") { + path_entries.extend(std::env::split_paths(&path)); + } + let helper_path = std::env::join_paths(path_entries)?; + + let server = exec_server_with_env([("PATH", helper_path.as_os_str())]).await?; + let environment = Environment::create(Some(server.websocket_url().to_string())).await?; + let file_system = environment.get_filesystem(); + let workspace = tmp.path().join("workspace"); + std::fs::create_dir_all(&workspace)?; + let file_path = workspace.join("created.txt"); + let sandbox = workspace_write_sandbox(workspace); + + file_system + .write_file( + &absolute_path(file_path.clone()), + b"written through fs helper".to_vec(), + Some(&sandbox), + ) + .await?; + + assert_eq!(std::fs::read(&file_path)?, b"written through fs helper"); + + let bwrap_log = fake_bwrap.with_file_name("bwrap.log"); + let log = std::fs::read_to_string(&bwrap_log) + .with_context(|| format!("expected fake bwrap log at {}", bwrap_log.display()))?; + assert!( + log.contains("--argv0"), + "expected fs helper sandbox path to invoke PATH bwrap with --argv0, got: {log}" + ); + + Ok(()) +} + #[test_case(false ; "local")] #[test_case(true ; "remote")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)]