From cb379d779744aada3e44e861bb9a653e22e5917a Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 22 May 2025 09:51:19 -0700 Subject: [PATCH] feat: introduce support for shell_environment_policy in config.toml (#1061) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit To date, when handling `shell` and `local_shell` tool calls, we were spawning new processes using the environment inherited from the Codex process itself. This means that the sensitive `OPENAI_API_KEY` that Codex needs to talk to OpenAI models was made available to everything run by `shell` and `local_shell`. While there are cases where that might be useful, it does not seem like a good default. This PR introduces a complex `shell_environment_policy` config option to control the `env` used with these tool calls. It is inevitably a bit complex so that it is possible to override individual components of the policy so without having to restate the entire thing. Details are in the updated `README.md` in this PR, but here is the relevant bit that explains the individual fields of `shell_environment_policy`: | Field | Type | Default | Description | | ------------------------- | -------------------------- | ------- | ----------------------------------------------------------------------------------------------------------------------------------------------- | | `inherit` | string | `core` | Starting template for the environment:
`core` (`HOME`, `PATH`, `USER`, …), `all` (clone full parent env), or `none` (start empty). | | `ignore_default_excludes` | boolean | `false` | When `false`, Codex removes any var whose **name** contains `KEY`, `SECRET`, or `TOKEN` (case-insensitive) before other rules run. | | `exclude` | array<string> | `[]` | Case-insensitive glob patterns to drop after the default filter.
Examples: `"AWS_*"`, `"AZURE_*"`. | | `set` | table<string,string> | `{}` | Explicit key/value overrides or additions – always win over inherited values. | | `include_only` | array<string> | `[]` | If non-empty, a whitelist of patterns; only variables that match _one_ pattern survive the final step. (Generally used with `inherit = "all"`.) | In particular, note that the default is `inherit = "core"`, so: * if you have extra env variables that you want to inherit from the parent process, use `inherit = "all"` and then specify `include_only` * if you have extra env variables where you want to hardcode the values, the default `inherit = "core"` will work fine, but then you need to specify `set` This configuration is not battle-tested, so we will probably still have to play with it a bit. `core/src/exec_env.rs` has the critical business logic as well as unit tests. Though if nothing else, previous to this change: ``` $ cargo run --bin codex -- debug seatbelt -- printenv OPENAI_API_KEY # ...prints OPENAI_API_KEY... ``` But after this change it does not print anything (as desired). One final thing to call out about this PR is that the `configure_command!` macro we use in `core/src/exec.rs` has to do some complex logic with respect to how it builds up the `env` for the process being spawned under Landlock/seccomp. Specifically, doing `cmd.env_clear()` followed by `cmd.envs(&$env_map)` (which is arguably the most intuitive way to do it) caused the Landlock unit tests to fail because the processes spawned by the unit tests started failing in unexpected ways! If we forgo `env_clear()` in favor of updating env vars one at a time, the tests still pass. The comment in the code talks about this a bit, and while I would like to investigate this more, I need to move on for the moment, but I do plan to come back to it to fully understand what is going on. For example, this suggests that we might not be able to spawn a C program that calls `env_clear()`, which would be...weird. We may still have to fiddle with our Landlock config if that is the case. --- .github/workflows/rust-ci.yml | 2 + codex-rs/Cargo.lock | 8 + codex-rs/README.md | 43 ++++++ codex-rs/cli/src/landlock.rs | 8 +- codex-rs/cli/src/linux-sandbox/main.rs | 8 +- codex-rs/cli/src/main.rs | 14 +- codex-rs/cli/src/seatbelt.rs | 19 ++- codex-rs/core/Cargo.toml | 2 + codex-rs/core/src/codex.rs | 5 + codex-rs/core/src/config.rs | 13 ++ codex-rs/core/src/config_types.rs | 89 +++++++++++ codex-rs/core/src/exec.rs | 51 ++++++- codex-rs/core/src/exec_env.rs | 196 +++++++++++++++++++++++++ codex-rs/core/src/exec_linux.rs | 2 + codex-rs/core/src/landlock.rs | 12 +- codex-rs/core/src/lib.rs | 1 + 16 files changed, 455 insertions(+), 18 deletions(-) create mode 100644 codex-rs/core/src/exec_env.rs diff --git a/.github/workflows/rust-ci.yml b/.github/workflows/rust-ci.yml index c1d231f9..f0eadaf2 100644 --- a/.github/workflows/rust-ci.yml +++ b/.github/workflows/rust-ci.yml @@ -100,6 +100,8 @@ jobs: id: test continue-on-error: true run: cargo test --all-features --target ${{ matrix.target }} + env: + RUST_BACKTRACE: 1 # Fail the job if any of the previous steps failed. - name: verify all steps passed diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 5358065c..6408e8de 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -526,6 +526,7 @@ dependencies = [ "futures", "landlock", "libc", + "maplit", "mcp-types", "mime_guess", "openssl-sys", @@ -548,6 +549,7 @@ dependencies = [ "tree-sitter", "tree-sitter-bash", "uuid", + "wildmatch", "wiremock", ] @@ -4309,6 +4311,12 @@ dependencies = [ "wasm-bindgen", ] +[[package]] +name = "wildmatch" +version = "2.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "68ce1ab1f8c62655ebe1350f589c61e505cf94d385bc6a12899442d9081e71fd" + [[package]] name = "winapi" version = "0.3.9" diff --git a/codex-rs/README.md b/codex-rs/README.md index bedce9f2..705d3130 100644 --- a/codex-rs/README.md +++ b/codex-rs/README.md @@ -222,6 +222,49 @@ Currently, customers whose accounts are set to use Zero Data Retention (ZDR) mus disable_response_storage = true ``` +### shell_environment_policy + +Codex spawns subprocesses (e.g. when executing a `local_shell` tool-call suggested by the assistant). By default it passes **only a minimal core subset** of your environment to those subprocesses to avoid leaking credentials. You can tune this behavior via the **`shell_environment_policy`** block in +`config.toml`: + +```toml +[shell_environment_policy] +# inherit can be "core" (default), "all", or "none" +inherit = "core" +# set to true to *skip* the filter for `"*KEY*"` and `"*TOKEN*"` +ignore_default_excludes = false +# exclude patterns (case-insensitive globs) +exclude = ["AWS_*", "AZURE_*"] +# force-set / override values +set = { CI = "1" } +# if provided, *only* vars matching these patterns are kept +include_only = ["PATH", "HOME"] +``` + +| Field | Type | Default | Description | +| ------------------------- | -------------------------- | ------- | ----------------------------------------------------------------------------------------------------------------------------------------------- | +| `inherit` | string | `core` | Starting template for the environment:
`core` (`HOME`, `PATH`, `USER`, …), `all` (clone full parent env), or `none` (start empty). | +| `ignore_default_excludes` | boolean | `false` | When `false`, Codex removes any var whose **name** contains `KEY`, `SECRET`, or `TOKEN` (case-insensitive) before other rules run. | +| `exclude` | array<string> | `[]` | Case-insensitive glob patterns to drop after the default filter.
Examples: `"AWS_*"`, `"AZURE_*"`. | +| `set` | table<string,string> | `{}` | Explicit key/value overrides or additions – always win over inherited values. | +| `include_only` | array<string> | `[]` | If non-empty, a whitelist of patterns; only variables that match _one_ pattern survive the final step. (Generally used with `inherit = "all"`.) | + +The patterns are **glob style**, not full regular expressions: `*` matches any +number of characters, `?` matches exactly one, and character classes like +`[A-Z]`/`[^0-9]` are supported. Matching is always **case-insensitive**. This +syntax is documented in code as `EnvironmentVariablePattern` (see +`core/src/config_types.rs`). + +If you just need a clean slate with a few custom entries you can write: + +```toml +[shell_environment_policy] +inherit = "none" +set = { PATH = "/usr/bin", MY_FLAG = "1" } +``` + +Currently, `CODEX_SANDBOX_NETWORK_DISABLED=1` is also added to the environment, assuming network is disabled. This is not configurable. + ### notify Specify a program that will be executed to get notified about events generated by Codex. Note that the program will receive the notification argument as a string of JSON, e.g.: diff --git a/codex-rs/cli/src/landlock.rs b/codex-rs/cli/src/landlock.rs index 998072c5..5a65fcbc 100644 --- a/codex-rs/cli/src/landlock.rs +++ b/codex-rs/cli/src/landlock.rs @@ -3,27 +3,29 @@ //! On Linux the command is executed inside a Landlock + seccomp sandbox by //! calling the low-level `exec_linux` helper from `codex_core::linux`. +use codex_core::config::Config; use codex_core::exec::StdioPolicy; use codex_core::exec::spawn_child_sync; use codex_core::exec_linux::apply_sandbox_policy_to_current_thread; -use codex_core::protocol::SandboxPolicy; use std::process::ExitStatus; use crate::exit_status::handle_exit_status; /// Execute `command` in a Linux sandbox (Landlock + seccomp) the way Codex /// would. -pub fn run_landlock(command: Vec, sandbox_policy: SandboxPolicy) -> anyhow::Result<()> { +pub fn run_landlock(command: Vec, config: &Config) -> anyhow::Result<()> { if command.is_empty() { anyhow::bail!("command args are empty"); } // Spawn a new thread and apply the sandbox policies there. + let env = codex_core::exec_env::create_env(&config.shell_environment_policy); + let sandbox_policy = config.sandbox_policy.clone(); let handle = std::thread::spawn(move || -> anyhow::Result { let cwd = std::env::current_dir()?; apply_sandbox_policy_to_current_thread(&sandbox_policy, &cwd)?; - let mut child = spawn_child_sync(command, cwd, &sandbox_policy, StdioPolicy::Inherit)?; + let mut child = spawn_child_sync(command, cwd, &sandbox_policy, StdioPolicy::Inherit, env)?; let status = child.wait()?; Ok(status) }); diff --git a/codex-rs/cli/src/linux-sandbox/main.rs b/codex-rs/cli/src/linux-sandbox/main.rs index f71f9b86..31416565 100644 --- a/codex-rs/cli/src/linux-sandbox/main.rs +++ b/codex-rs/cli/src/linux-sandbox/main.rs @@ -10,6 +10,8 @@ fn main() -> anyhow::Result<()> { use codex_cli::LandlockCommand; use codex_cli::create_sandbox_policy; use codex_cli::landlock; + use codex_core::config::Config; + use codex_core::config::ConfigOverrides; let LandlockCommand { full_auto, @@ -17,6 +19,10 @@ fn main() -> anyhow::Result<()> { command, } = LandlockCommand::parse(); let sandbox_policy = create_sandbox_policy(full_auto, sandbox); - landlock::run_landlock(command, sandbox_policy)?; + let config = Config::load_with_overrides(ConfigOverrides { + sandbox_policy: Some(sandbox_policy), + ..Default::default() + })?; + landlock::run_landlock(command, &config)?; Ok(()) } diff --git a/codex-rs/cli/src/main.rs b/codex-rs/cli/src/main.rs index aa0691d8..b2b1b8cf 100644 --- a/codex-rs/cli/src/main.rs +++ b/codex-rs/cli/src/main.rs @@ -4,6 +4,8 @@ use codex_cli::SeatbeltCommand; use codex_cli::create_sandbox_policy; use codex_cli::proto; use codex_cli::seatbelt; +use codex_core::config::Config; +use codex_core::config::ConfigOverrides; use codex_exec::Cli as ExecCli; use codex_tui::Cli as TuiCli; @@ -86,7 +88,11 @@ async fn main() -> anyhow::Result<()> { full_auto, }) => { let sandbox_policy = create_sandbox_policy(full_auto, sandbox); - seatbelt::run_seatbelt(command, sandbox_policy).await?; + let config = Config::load_with_overrides(ConfigOverrides { + sandbox_policy: Some(sandbox_policy), + ..Default::default() + })?; + seatbelt::run_seatbelt(command, &config).await?; } #[cfg(unix)] DebugCommand::Landlock(LandlockCommand { @@ -95,7 +101,11 @@ async fn main() -> anyhow::Result<()> { full_auto, }) => { let sandbox_policy = create_sandbox_policy(full_auto, sandbox); - codex_cli::landlock::run_landlock(command, sandbox_policy)?; + let config = Config::load_with_overrides(ConfigOverrides { + sandbox_policy: Some(sandbox_policy), + ..Default::default() + })?; + codex_cli::landlock::run_landlock(command, &config)?; } #[cfg(not(unix))] DebugCommand::Landlock(_) => { diff --git a/codex-rs/cli/src/seatbelt.rs b/codex-rs/cli/src/seatbelt.rs index e40848ca..d4a78404 100644 --- a/codex-rs/cli/src/seatbelt.rs +++ b/codex-rs/cli/src/seatbelt.rs @@ -1,16 +1,21 @@ +use codex_core::config::Config; use codex_core::exec::StdioPolicy; use codex_core::exec::spawn_command_under_seatbelt; -use codex_core::protocol::SandboxPolicy; +use codex_core::exec_env::create_env; use crate::exit_status::handle_exit_status; -pub async fn run_seatbelt( - command: Vec, - sandbox_policy: SandboxPolicy, -) -> anyhow::Result<()> { +pub async fn run_seatbelt(command: Vec, config: &Config) -> anyhow::Result<()> { let cwd = std::env::current_dir()?; - let mut child = - spawn_command_under_seatbelt(command, &sandbox_policy, cwd, StdioPolicy::Inherit).await?; + let env = create_env(&config.shell_environment_policy); + let mut child = spawn_command_under_seatbelt( + command, + &config.sandbox_policy, + cwd, + StdioPolicy::Inherit, + env, + ) + .await?; let status = child.wait().await?; handle_exit_status(status); } diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index e2979497..2d4ed8f3 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -46,6 +46,7 @@ tracing = { version = "0.1.41", features = ["log"] } tree-sitter = "0.25.3" tree-sitter-bash = "0.23.3" uuid = { version = "1", features = ["serde", "v4"] } +wildmatch = "2.4.0" [target.'cfg(target_os = "linux")'.dependencies] libc = "0.2.172" @@ -58,6 +59,7 @@ openssl-sys = { version = "*", features = ["vendored"] } [dev-dependencies] assert_cmd = "2" +maplit = "1.0.2" predicates = "3" pretty_assertions = "1.4.1" tempfile = "3" diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 0f914727..69e50478 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -37,6 +37,7 @@ use crate::client::ModelClient; use crate::client_common::Prompt; use crate::client_common::ResponseEvent; use crate::config::Config; +use crate::config_types::ShellEnvironmentPolicy; use crate::conversation_history::ConversationHistory; use crate::error::CodexErr; use crate::error::Result as CodexResult; @@ -45,6 +46,7 @@ use crate::exec::ExecParams; use crate::exec::ExecToolCallOutput; use crate::exec::SandboxType; use crate::exec::process_exec_tool_call; +use crate::exec_env::create_env; use crate::flags::OPENAI_STREAM_MAX_RETRIES; use crate::mcp_connection_manager::McpConnectionManager; use crate::mcp_connection_manager::try_parse_fully_qualified_tool_name; @@ -171,6 +173,7 @@ pub(crate) struct Session { instructions: Option, approval_policy: AskForApproval, sandbox_policy: SandboxPolicy, + shell_environment_policy: ShellEnvironmentPolicy, writable_roots: Mutex>, /// Manager for external MCP servers/tools. @@ -634,6 +637,7 @@ async fn submission_loop( instructions, approval_policy, sandbox_policy, + shell_environment_policy: config.shell_environment_policy.clone(), cwd, writable_roots, mcp_connection_manager, @@ -1124,6 +1128,7 @@ fn to_exec_params(params: ShellToolCallParams, sess: &Session) -> ExecParams { command: params.command, cwd: sess.resolve_path(params.workdir.clone()), timeout_ms: params.timeout_ms, + env: create_env(&sess.shell_environment_policy), } } diff --git a/codex-rs/core/src/config.rs b/codex-rs/core/src/config.rs index de97b36e..2a3f4543 100644 --- a/codex-rs/core/src/config.rs +++ b/codex-rs/core/src/config.rs @@ -1,6 +1,8 @@ use crate::config_profile::ConfigProfile; use crate::config_types::History; use crate::config_types::McpServerConfig; +use crate::config_types::ShellEnvironmentPolicy; +use crate::config_types::ShellEnvironmentPolicyToml; use crate::config_types::Tui; use crate::config_types::UriBasedFileOpener; use crate::flags::OPENAI_DEFAULT_MODEL; @@ -37,6 +39,8 @@ pub struct Config { pub sandbox_policy: SandboxPolicy, + pub shell_environment_policy: ShellEnvironmentPolicy, + /// Disable server-side response storage (sends the full conversation /// context with every request). Currently necessary for OpenAI customers /// who have opted into Zero Data Retention (ZDR). @@ -108,6 +112,9 @@ pub struct ConfigToml { /// Default approval policy for executing commands. pub approval_policy: Option, + #[serde(default)] + pub shell_environment_policy: ShellEnvironmentPolicyToml, + // The `default` attribute ensures that the field is treated as `None` when // the key is omitted from the TOML. Without it, Serde treats the field as // required because we supply a custom deserializer. @@ -302,6 +309,8 @@ impl Config { })? .clone(); + let shell_environment_policy = cfg.shell_environment_policy.into(); + let resolved_cwd = { use std::env; @@ -336,6 +345,7 @@ impl Config { .or(cfg.approval_policy) .unwrap_or_else(AskForApproval::default), sandbox_policy, + shell_environment_policy, disable_response_storage: disable_response_storage .or(config_profile.disable_response_storage) .or(cfg.disable_response_storage) @@ -677,6 +687,7 @@ disable_response_storage = true model_provider: fixture.openai_provider.clone(), approval_policy: AskForApproval::Never, sandbox_policy: SandboxPolicy::new_read_only_policy(), + shell_environment_policy: ShellEnvironmentPolicy::default(), disable_response_storage: false, instructions: None, notify: None, @@ -714,6 +725,7 @@ disable_response_storage = true model_provider: fixture.openai_chat_completions_provider.clone(), approval_policy: AskForApproval::UnlessAllowListed, sandbox_policy: SandboxPolicy::new_read_only_policy(), + shell_environment_policy: ShellEnvironmentPolicy::default(), disable_response_storage: false, instructions: None, notify: None, @@ -766,6 +778,7 @@ disable_response_storage = true model_provider: fixture.openai_provider.clone(), approval_policy: AskForApproval::OnFailure, sandbox_policy: SandboxPolicy::new_read_only_policy(), + shell_environment_policy: ShellEnvironmentPolicy::default(), disable_response_storage: true, instructions: None, notify: None, diff --git a/codex-rs/core/src/config_types.rs b/codex-rs/core/src/config_types.rs index 22c3e856..6696f76f 100644 --- a/codex-rs/core/src/config_types.rs +++ b/codex-rs/core/src/config_types.rs @@ -4,6 +4,7 @@ // definitions that do not contain business logic. use std::collections::HashMap; +use wildmatch::WildMatchPattern; use serde::Deserialize; @@ -86,3 +87,91 @@ pub struct Tui { /// using the mouse without needing to hold down a modifier key. pub disable_mouse_capture: bool, } + +#[derive(Deserialize, Debug, Clone, PartialEq, Default)] + +pub enum ShellEnvironmentPolicyInherit { + /// "Core" environment variables for the platform. On UNIX, this would + /// include HOME, LOGNAME, PATH, SHELL, and USER, among others. + #[default] + Core, + + /// Inherits the full environment from the parent process. + All, + + /// Do not inherit any environment variables from the parent process. + None, +} + +/// Policy for building the `env` when spawning a process via either the +/// `shell` or `local_shell` tool. +#[derive(Deserialize, Debug, Clone, PartialEq, Default)] +pub struct ShellEnvironmentPolicyToml { + pub inherit: Option, + + pub ignore_default_excludes: Option, + + /// List of regular expressions. + pub exclude: Option>, + + pub r#set: Option>, + + /// List of regular expressions. + pub include_only: Option>, +} + +pub type EnvironmentVariablePattern = WildMatchPattern<'*', '?'>; + +/// Deriving the `env` based on this policy works as follows: +/// 1. Create an initial map based on the `inherit` policy. +/// 2. If `ignore_default_excludes` is false, filter the map using the default +/// exclude pattern(s), which are: `"*KEY*"` and `"*TOKEN*"`. +/// 3. If `exclude` is not empty, filter the map using the provided patterns. +/// 4. Insert any entries from `r#set` into the map. +/// 5. If non-empty, filter the map using the `include_only` patterns. +#[derive(Debug, Clone, PartialEq, Default)] +pub struct ShellEnvironmentPolicy { + /// Starting point when building the environment. + pub inherit: ShellEnvironmentPolicyInherit, + + /// True to skip the check to exclude default environment variables that + /// contain "KEY" or "TOKEN" in their name. + pub ignore_default_excludes: bool, + + /// Environment variable names to exclude from the environment. + pub exclude: Vec, + + /// (key, value) pairs to insert in the environment. + pub r#set: HashMap, + + /// Environment variable names to retain in the environment. + pub include_only: Vec, +} + +impl From for ShellEnvironmentPolicy { + fn from(toml: ShellEnvironmentPolicyToml) -> Self { + let inherit = toml.inherit.unwrap_or(ShellEnvironmentPolicyInherit::Core); + let ignore_default_excludes = toml.ignore_default_excludes.unwrap_or(false); + let exclude = toml + .exclude + .unwrap_or_default() + .into_iter() + .map(|s| EnvironmentVariablePattern::new_case_insensitive(&s)) + .collect(); + let r#set = toml.r#set.unwrap_or_default(); + let include_only = toml + .include_only + .unwrap_or_default() + .into_iter() + .map(|s| EnvironmentVariablePattern::new_case_insensitive(&s)) + .collect(); + + Self { + inherit, + ignore_default_excludes, + exclude, + r#set, + include_only, + } + } +} diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index 158a0da9..96b601b6 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -1,6 +1,7 @@ #[cfg(unix)] use std::os::unix::process::ExitStatusExt; +use std::collections::HashMap; use std::io; use std::path::Path; use std::path::PathBuf; @@ -59,6 +60,7 @@ pub struct ExecParams { pub command: Vec, pub cwd: PathBuf, pub timeout_ms: Option, + pub env: HashMap, } #[derive(Clone, Copy, Debug, PartialEq)] @@ -87,12 +89,14 @@ pub async fn process_exec_tool_call( command, cwd, timeout_ms, + env, } = params; let child = spawn_command_under_seatbelt( command, sandbox_policy, cwd, StdioPolicy::RedirectForShellTool, + env, ) .await?; consume_truncated_output(child, ctrl_c, timeout_ms).await @@ -145,9 +149,10 @@ pub async fn spawn_command_under_seatbelt( sandbox_policy: &SandboxPolicy, cwd: PathBuf, stdio_policy: StdioPolicy, + env: HashMap, ) -> std::io::Result { let seatbelt_command = create_seatbelt_command(command, sandbox_policy, &cwd); - spawn_child_async(seatbelt_command, cwd, sandbox_policy, stdio_policy).await + spawn_child_async(seatbelt_command, cwd, sandbox_policy, stdio_policy, env).await } fn create_seatbelt_command( @@ -233,6 +238,7 @@ async fn exec( command, cwd, timeout_ms, + env, }: ExecParams, sandbox_policy: &SandboxPolicy, ctrl_c: Arc, @@ -242,6 +248,7 @@ async fn exec( cwd, sandbox_policy, StdioPolicy::RedirectForShellTool, + env, ) .await?; consume_truncated_output(child, ctrl_c, timeout_ms).await @@ -259,7 +266,8 @@ macro_rules! configure_command { $command: expr, $cwd: expr, $sandbox_policy: expr, - $stdio_policy: expr + $stdio_policy: expr, + $env_map: expr ) => {{ // For now, we take `SandboxPolicy` as a parameter to spawn_child() because // we need to determine whether to set the @@ -279,6 +287,38 @@ macro_rules! configure_command { cmd.args(&$command[1..]); cmd.current_dir($cwd); + // Previously, to update the env for `cmd`, we did the straightforward + // thing of calling `env_clear()` followed by `envs(&env_map)` so + // that the spawned process inherited *only* the variables explicitly + // provided by the caller. On Linux, the combination of `env_clear()` + // and Landlock/seccomp caused a permission error whereas this more + // "surgical" approach of setting variables individually appears to + // work fine. More time with `strace` and friends is merited to fully + // debug thus, though we will soon use a helper binary like we do for + // Seatbelt, which will simplify this logic. + + // Iterate through the current process environment first so we can + // decide, for every variable that already exists, whether we need to + // override its value. + let mut remaining_overrides = $env_map.clone(); + for (key, current_val) in std::env::vars() { + if let Some(desired_val) = remaining_overrides.remove(&key) { + // The caller provided a value for this variable. Override it + // only if the value differs from what is currently set. + if desired_val != current_val { + cmd.env(&key, desired_val); + } + } + // If the variable was not in `env_map`, we leave it unchanged. + } + + // Any entries still left in `remaining_overrides` were not present in + // the parent environment. Add them now so that the child process sees + // the complete set requested by the caller. + for (key, val) in remaining_overrides { + cmd.env(key, val); + } + if !$sandbox_policy.has_full_network_access() { cmd.env(CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR, "1"); } @@ -313,8 +353,9 @@ pub(crate) async fn spawn_child_async( cwd: PathBuf, sandbox_policy: &SandboxPolicy, stdio_policy: StdioPolicy, + env: HashMap, ) -> std::io::Result { - let mut cmd = configure_command!(Command, command, cwd, sandbox_policy, stdio_policy)?; + let mut cmd = configure_command!(Command, command, cwd, sandbox_policy, stdio_policy, env)?; cmd.kill_on_drop(true).spawn() } @@ -326,13 +367,15 @@ pub fn spawn_child_sync( cwd: PathBuf, sandbox_policy: &SandboxPolicy, stdio_policy: StdioPolicy, + env: HashMap, ) -> std::io::Result { let mut cmd = configure_command!( std::process::Command, command, cwd, sandbox_policy, - stdio_policy + stdio_policy, + env )?; cmd.spawn() } diff --git a/codex-rs/core/src/exec_env.rs b/codex-rs/core/src/exec_env.rs new file mode 100644 index 00000000..2957f3da --- /dev/null +++ b/codex-rs/core/src/exec_env.rs @@ -0,0 +1,196 @@ +use crate::config_types::EnvironmentVariablePattern; +use crate::config_types::ShellEnvironmentPolicy; +use crate::config_types::ShellEnvironmentPolicyInherit; +use std::collections::HashMap; +use std::collections::HashSet; + +/// Construct an environment map based on the rules in the specified policy. The +/// resulting map can be passed directly to `Command::envs()` after calling +/// `env_clear()` to ensure no unintended variables are leaked to the spawned +/// process. +/// +/// The derivation follows the algorithm documented in the struct-level comment +/// for [`ShellEnvironmentPolicy`]. +pub fn create_env(policy: &ShellEnvironmentPolicy) -> HashMap { + populate_env(std::env::vars(), policy) +} + +fn populate_env(vars: I, policy: &ShellEnvironmentPolicy) -> HashMap +where + I: IntoIterator, +{ + // Step 1 – determine the starting set of variables based on the + // `inherit` strategy. + let mut env_map: HashMap = match policy.inherit { + ShellEnvironmentPolicyInherit::All => vars.into_iter().collect(), + ShellEnvironmentPolicyInherit::None => HashMap::new(), + ShellEnvironmentPolicyInherit::Core => { + const CORE_VARS: &[&str] = &[ + "HOME", "LOGNAME", "PATH", "SHELL", "USER", "USERNAME", "TMPDIR", "TEMP", "TMP", + ]; + let allow: HashSet<&str> = CORE_VARS.iter().copied().collect(); + vars.into_iter() + .filter(|(k, _)| allow.contains(k.as_str())) + .collect() + } + }; + + // Internal helper – does `name` match **any** pattern in `patterns`? + let matches_any = |name: &str, patterns: &[EnvironmentVariablePattern]| -> bool { + patterns.iter().any(|pattern| pattern.matches(name)) + }; + + // Step 2 – Apply the default exclude if not disabled. + if !policy.ignore_default_excludes { + let default_excludes = vec![ + EnvironmentVariablePattern::new_case_insensitive("*KEY*"), + EnvironmentVariablePattern::new_case_insensitive("*SECRET*"), + EnvironmentVariablePattern::new_case_insensitive("*TOKEN*"), + ]; + env_map.retain(|k, _| !matches_any(k, &default_excludes)); + } + + // Step 3 – Apply custom excludes. + if !policy.exclude.is_empty() { + env_map.retain(|k, _| !matches_any(k, &policy.exclude)); + } + + // Step 4 – Apply user-provided overrides. + for (key, val) in &policy.r#set { + env_map.insert(key.clone(), val.clone()); + } + + // Step 5 – If include_only is non-empty, keep *only* the matching vars. + if !policy.include_only.is_empty() { + env_map.retain(|k, _| matches_any(k, &policy.include_only)); + } + + env_map +} + +#[cfg(test)] +mod tests { + #![allow(clippy::unwrap_used, clippy::expect_used)] + + use super::*; + use crate::config_types::ShellEnvironmentPolicyInherit; + use maplit::hashmap; + + fn make_vars(pairs: &[(&str, &str)]) -> Vec<(String, String)> { + pairs + .iter() + .map(|(k, v)| (k.to_string(), v.to_string())) + .collect() + } + + #[test] + fn test_core_inherit_and_default_excludes() { + let vars = make_vars(&[ + ("PATH", "/usr/bin"), + ("HOME", "/home/user"), + ("API_KEY", "secret"), + ("SECRET_TOKEN", "t"), + ]); + + let policy = ShellEnvironmentPolicy::default(); // inherit Core, default excludes on + let result = populate_env(vars, &policy); + + let expected: HashMap = hashmap! { + "PATH".to_string() => "/usr/bin".to_string(), + "HOME".to_string() => "/home/user".to_string(), + }; + + assert_eq!(result, expected); + } + + #[test] + fn test_include_only() { + let vars = make_vars(&[("PATH", "/usr/bin"), ("FOO", "bar")]); + + let policy = ShellEnvironmentPolicy { + // skip default excludes so nothing is removed prematurely + ignore_default_excludes: true, + include_only: vec![EnvironmentVariablePattern::new_case_insensitive("*PATH")], + ..Default::default() + }; + + let result = populate_env(vars, &policy); + + let expected: HashMap = hashmap! { + "PATH".to_string() => "/usr/bin".to_string(), + }; + + assert_eq!(result, expected); + } + + #[test] + fn test_set_overrides() { + let vars = make_vars(&[("PATH", "/usr/bin")]); + + let mut policy = ShellEnvironmentPolicy { + ignore_default_excludes: true, + ..Default::default() + }; + policy.r#set.insert("NEW_VAR".to_string(), "42".to_string()); + + let result = populate_env(vars, &policy); + + let expected: HashMap = hashmap! { + "PATH".to_string() => "/usr/bin".to_string(), + "NEW_VAR".to_string() => "42".to_string(), + }; + + assert_eq!(result, expected); + } + + #[test] + fn test_inherit_all() { + let vars = make_vars(&[("PATH", "/usr/bin"), ("FOO", "bar")]); + + let policy = ShellEnvironmentPolicy { + inherit: ShellEnvironmentPolicyInherit::All, + ignore_default_excludes: true, // keep everything + ..Default::default() + }; + + let result = populate_env(vars.clone(), &policy); + let expected: HashMap = vars.into_iter().collect(); + assert_eq!(result, expected); + } + + #[test] + fn test_inherit_all_with_default_excludes() { + let vars = make_vars(&[("PATH", "/usr/bin"), ("API_KEY", "secret")]); + + let policy = ShellEnvironmentPolicy { + inherit: ShellEnvironmentPolicyInherit::All, + ..Default::default() + }; + + let result = populate_env(vars, &policy); + let expected: HashMap = hashmap! { + "PATH".to_string() => "/usr/bin".to_string(), + }; + assert_eq!(result, expected); + } + + #[test] + fn test_inherit_none() { + let vars = make_vars(&[("PATH", "/usr/bin"), ("HOME", "/home")]); + + let mut policy = ShellEnvironmentPolicy { + inherit: ShellEnvironmentPolicyInherit::None, + ignore_default_excludes: true, + ..Default::default() + }; + policy + .r#set + .insert("ONLY_VAR".to_string(), "yes".to_string()); + + let result = populate_env(vars, &policy); + let expected: HashMap = hashmap! { + "ONLY_VAR".to_string() => "yes".to_string(), + }; + assert_eq!(result, expected); + } +} diff --git a/codex-rs/core/src/exec_linux.rs b/codex-rs/core/src/exec_linux.rs index e74c5621..76bd428a 100644 --- a/codex-rs/core/src/exec_linux.rs +++ b/codex-rs/core/src/exec_linux.rs @@ -34,6 +34,7 @@ pub fn exec_linux( command, cwd, timeout_ms, + env, } = params; apply_sandbox_policy_to_current_thread(&sandbox_policy, &cwd)?; let child = spawn_child_async( @@ -41,6 +42,7 @@ pub fn exec_linux( cwd, &sandbox_policy, StdioPolicy::RedirectForShellTool, + env, ) .await?; consume_truncated_output(child, ctrl_c_copy, timeout_ms).await diff --git a/codex-rs/core/src/landlock.rs b/codex-rs/core/src/landlock.rs index 6e9b8de7..07c56815 100644 --- a/codex-rs/core/src/landlock.rs +++ b/codex-rs/core/src/landlock.rs @@ -143,20 +143,29 @@ mod tests { #![expect(clippy::unwrap_used, clippy::expect_used)] use super::*; + use crate::config_types::ShellEnvironmentPolicy; use crate::exec::ExecParams; use crate::exec::SandboxType; use crate::exec::process_exec_tool_call; + use crate::exec_env::create_env; use crate::protocol::SandboxPolicy; + use std::collections::HashMap; use std::sync::Arc; use tempfile::NamedTempFile; use tokio::sync::Notify; + fn create_env_from_core_vars() -> HashMap { + let policy = ShellEnvironmentPolicy::default(); + create_env(&policy) + } + #[allow(clippy::print_stdout)] async fn run_cmd(cmd: &[&str], writable_roots: &[PathBuf], timeout_ms: u64) { let params = ExecParams { command: cmd.iter().map(|elm| elm.to_string()).collect(), cwd: std::env::current_dir().expect("cwd should exist"), timeout_ms: Some(timeout_ms), + env: create_env_from_core_vars(), }; let sandbox_policy = @@ -236,9 +245,10 @@ mod tests { let params = ExecParams { command: cmd.iter().map(|s| s.to_string()).collect(), cwd: std::env::current_dir().expect("cwd should exist"), - // Give the tool a generous 2‑second timeout so even slow DNS timeouts + // Give the tool a generous 2-second timeout so even slow DNS timeouts // do not stall the suite. timeout_ms: Some(2_000), + env: create_env_from_core_vars(), }; let sandbox_policy = SandboxPolicy::new_read_only_policy(); diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 759f1029..261ae0a0 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -17,6 +17,7 @@ pub mod config_types; mod conversation_history; pub mod error; pub mod exec; +pub mod exec_env; pub mod exec_linux; mod flags; mod is_safe_command;