diff --git a/crates/tui/src/child_env.rs b/crates/tui/src/child_env.rs new file mode 100644 index 00000000..2cb4e032 --- /dev/null +++ b/crates/tui/src/child_env.rs @@ -0,0 +1,181 @@ +//! Sanitized environment handling for child processes. + +use std::collections::HashMap; +use std::ffi::{OsStr, OsString}; + +/// Convert a string env map into owned OS strings for child env helpers. +pub fn string_map_env( + env: &HashMap, +) -> impl Iterator + '_ { + env.iter() + .map(|(key, value)| (OsString::from(key), OsString::from(value))) +} + +/// Return the environment for a child process after dropping parent secrets. +/// +/// `overrides` are trusted call-site values, such as sandbox markers, hook +/// variables, MCP server config, or RLM context path. They are applied after the +/// parent allowlist so explicit values win. +pub fn sanitized_child_env(overrides: I) -> Vec<(OsString, OsString)> +where + I: IntoIterator, + K: AsRef, + V: AsRef, +{ + let mut env = Vec::new(); + for (key, value) in std::env::vars_os() { + if is_allowed_parent_env_key(&key) { + upsert_env(&mut env, key, value); + } + } + for (key, value) in overrides { + upsert_env( + &mut env, + key.as_ref().to_os_string(), + value.as_ref().to_os_string(), + ); + } + env +} + +pub fn apply_to_command(cmd: &mut std::process::Command, overrides: I) +where + I: IntoIterator, + K: AsRef, + V: AsRef, +{ + cmd.env_clear(); + for (key, value) in sanitized_child_env(overrides) { + cmd.env(key, value); + } +} + +pub fn apply_to_tokio_command(cmd: &mut tokio::process::Command, overrides: I) +where + I: IntoIterator, + K: AsRef, + V: AsRef, +{ + cmd.env_clear(); + for (key, value) in sanitized_child_env(overrides) { + cmd.env(key, value); + } +} + +pub fn apply_to_pty_command(cmd: &mut portable_pty::CommandBuilder, overrides: I) +where + I: IntoIterator, + K: AsRef, + V: AsRef, +{ + cmd.env_clear(); + for (key, value) in sanitized_child_env(overrides) { + cmd.env(key, value); + } +} + +fn is_allowed_parent_env_key(key: &OsStr) -> bool { + let key = key.to_string_lossy(); + let normalized = key.to_ascii_uppercase(); + matches!( + normalized.as_str(), + "PATH" + | "HOME" + | "USER" + | "USERNAME" + | "LOGNAME" + | "LANG" + | "LANGUAGE" + | "LC_ALL" + | "LC_CTYPE" + | "LC_MESSAGES" + | "TERM" + | "COLORTERM" + | "NO_COLOR" + | "FORCE_COLOR" + | "SHELL" + | "TMPDIR" + | "TMP" + | "TEMP" + | "__CF_USER_TEXT_ENCODING" + | "SYSTEMROOT" + | "WINDIR" + | "COMSPEC" + | "PATHEXT" + | "USERPROFILE" + | "HOMEDRIVE" + | "HOMEPATH" + ) || normalized.starts_with("LC_") +} + +fn upsert_env(env: &mut Vec<(OsString, OsString)>, key: OsString, value: OsString) { + let normalized = normalize_key(&key); + env.retain(|(existing, _)| normalize_key(existing) != normalized); + env.push((key, value)); +} + +fn normalize_key(key: &OsStr) -> String { + key.to_string_lossy().to_ascii_uppercase() +} + +#[cfg(test)] +mod tests { + use super::*; + use std::sync::{Mutex, OnceLock}; + + fn env_lock() -> &'static Mutex<()> { + static LOCK: OnceLock> = OnceLock::new(); + LOCK.get_or_init(|| Mutex::new(())) + } + + #[test] + fn sanitized_child_env_drops_parent_secret_like_values() { + let _guard = env_lock().lock().expect("env lock"); + let previous = std::env::var_os("DEEPSEEK_CHILD_ENV_TEST_SECRET"); + unsafe { + std::env::set_var("DEEPSEEK_CHILD_ENV_TEST_SECRET", "parent-secret"); + } + + let env = sanitized_child_env(std::iter::empty::<(OsString, OsString)>()); + + match previous { + Some(value) => unsafe { + std::env::set_var("DEEPSEEK_CHILD_ENV_TEST_SECRET", value); + }, + None => unsafe { + std::env::remove_var("DEEPSEEK_CHILD_ENV_TEST_SECRET"); + }, + } + + assert!( + env.iter() + .all(|(key, _)| key != "DEEPSEEK_CHILD_ENV_TEST_SECRET") + ); + } + + #[test] + fn explicit_child_env_values_win_over_parent_allowlist() { + let _guard = env_lock().lock().expect("env lock"); + let previous = std::env::var_os("PATH"); + unsafe { + std::env::set_var("PATH", "/parent/bin"); + } + + let env = sanitized_child_env([(OsString::from("PATH"), OsString::from("/explicit/bin"))]); + + match previous { + Some(value) => unsafe { + std::env::set_var("PATH", value); + }, + None => unsafe { + std::env::remove_var("PATH"); + }, + } + + let path = env + .iter() + .find(|(key, _)| normalize_key(key) == "PATH") + .map(|(_, value)| value); + assert_eq!(path, Some(&OsString::from("/explicit/bin"))); + } +} diff --git a/crates/tui/src/main.rs b/crates/tui/src/main.rs index a9a32814..2cb7270d 100644 --- a/crates/tui/src/main.rs +++ b/crates/tui/src/main.rs @@ -16,6 +16,7 @@ mod acp_server; mod audit; mod auto_reasoning; mod automation_manager; +mod child_env; mod client; mod command_safety; mod commands; @@ -3548,9 +3549,7 @@ fn run_sandbox_command(args: SandboxArgs) -> Result<()> { .current_dir(&exec_env.cwd) .stdout(Stdio::piped()) .stderr(Stdio::piped()); - for (key, value) in &exec_env.env { - cmd.env(key, value); - } + child_env::apply_to_command(&mut cmd, child_env::string_map_env(&exec_env.env)); let mut child = cmd .spawn() diff --git a/crates/tui/src/mcp.rs b/crates/tui/src/mcp.rs index b59553d6..5e5075c3 100644 --- a/crates/tui/src/mcp.rs +++ b/crates/tui/src/mcp.rs @@ -16,6 +16,7 @@ use serde::{Deserialize, Serialize}; use tokio::io::{AsyncBufReadExt, AsyncWriteExt}; use tokio::process::{Child, ChildStdin, ChildStdout}; +use crate::child_env; use crate::network_policy::{Decision, NetworkPolicyDecider, host_from_url}; use crate::utils::write_atomic; @@ -666,9 +667,7 @@ impl McpConnection { .stderr(std::process::Stdio::null()) .kill_on_drop(true); - for (key, value) in &config.env { - cmd.env(key, value); - } + child_env::apply_to_tokio_command(&mut cmd, child_env::string_map_env(&config.env)); let mut child = cmd.spawn().with_context(|| { let env_keys: Vec<&str> = config.env.keys().map(String::as_str).collect(); diff --git a/crates/tui/src/repl/runtime.rs b/crates/tui/src/repl/runtime.rs index 449cdf98..73f64bd9 100644 --- a/crates/tui/src/repl/runtime.rs +++ b/crates/tui/src/repl/runtime.rs @@ -16,6 +16,7 @@ //! that happens to contain "REQ" or "FINAL" can't be confused with control //! messages. +use std::ffi::OsString; use std::path::{Path, PathBuf}; use std::process::Stdio; use std::time::{Duration, Instant}; @@ -25,6 +26,8 @@ use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; use tokio::process::{Child, ChildStdin, ChildStdout, Command}; use uuid::Uuid; +use crate::child_env; + // --------------------------------------------------------------------------- // Public types // --------------------------------------------------------------------------- @@ -192,9 +195,15 @@ impl PythonRuntime { .stderr(Stdio::piped()) .kill_on_drop(true); - if let Some(path) = context_path { - cmd.env("RLM_CONTEXT_FILE", path); - } + let context_env = context_path + .map(|path| { + vec![( + OsString::from("RLM_CONTEXT_FILE"), + path.as_os_str().to_os_string(), + )] + }) + .unwrap_or_default(); + child_env::apply_to_tokio_command(&mut cmd, context_env); let mut child = cmd .spawn() diff --git a/crates/tui/src/tools/shell.rs b/crates/tui/src/tools/shell.rs index e9f0cc3f..c92a9cda 100644 --- a/crates/tui/src/tools/shell.rs +++ b/crates/tui/src/tools/shell.rs @@ -25,6 +25,7 @@ use std::os::unix::process::CommandExt; use portable_pty::{CommandBuilder, PtySize, native_pty_system}; use super::shell_output::{summarize_output, truncate_with_meta}; +use crate::child_env; use crate::sandbox::{ CommandSpec, ExecEnv, @@ -765,10 +766,7 @@ impl ShellManager { cmd.stdin(Stdio::piped()); } - // Set environment variables from exec_env - for (key, value) in &exec_env.env { - cmd.env(key, value); - } + child_env::apply_to_command(&mut cmd, child_env::string_map_env(&exec_env.env)); let mut child = cmd .spawn() @@ -904,9 +902,7 @@ impl ShellManager { } install_parent_death_signal(&mut cmd); - for (key, value) in &exec_env.env { - cmd.env(key, value); - } + child_env::apply_to_command(&mut cmd, child_env::string_map_env(&exec_env.env)); let mut child = cmd .spawn() @@ -1010,9 +1006,7 @@ impl ShellManager { cmd.arg(arg); } cmd.cwd(working_dir); - for (key, value) in &exec_env.env { - cmd.env(key, value); - } + child_env::apply_to_pty_command(&mut cmd, child_env::string_map_env(&exec_env.env)); let child = pair .slave @@ -1048,9 +1042,7 @@ impl ShellManager { cmd.process_group(0); } - for (key, value) in &exec_env.env { - cmd.env(key, value); - } + child_env::apply_to_command(&mut cmd, child_env::string_map_env(&exec_env.env)); let mut child = cmd .spawn() diff --git a/crates/tui/src/tools/shell/tests.rs b/crates/tui/src/tools/shell/tests.rs index 028e0055..c443039b 100644 --- a/crates/tui/src/tools/shell/tests.rs +++ b/crates/tui/src/tools/shell/tests.rs @@ -2,8 +2,14 @@ use super::*; use crate::tools::spec::ToolContext; use serde_json::{Value, json}; +use std::sync::{Mutex, OnceLock}; use tempfile::tempdir; +fn env_lock() -> &'static Mutex<()> { + static LOCK: OnceLock> = OnceLock::new(); + LOCK.get_or_init(|| Mutex::new(())) +} + fn echo_command(message: &str) -> String { format!("echo {message}") } @@ -82,6 +88,49 @@ fn failed_network_shell_result(stdout: &str, stderr: &str) -> ShellResult { } } +#[test] +#[cfg(unix)] +fn shell_execution_scrubs_parent_env_and_keeps_explicit_env() { + let _guard = env_lock().lock().expect("env lock"); + let previous = std::env::var_os("DEEPSEEK_CHILD_ENV_SHELL_SECRET"); + unsafe { + std::env::set_var("DEEPSEEK_CHILD_ENV_SHELL_SECRET", "parent-secret"); + } + + let tmp = tempdir().expect("tempdir"); + let mut manager = ShellManager::new(tmp.path().to_path_buf()); + let mut extra = std::collections::HashMap::new(); + extra.insert( + "DEEPSEEK_CHILD_ENV_EXPLICIT".to_string(), + "explicit-value".to_string(), + ); + + let result = manager + .execute_with_options_env( + "printf '%s\\n%s\\n' \"${DEEPSEEK_CHILD_ENV_SHELL_SECRET-unset}\" \"${DEEPSEEK_CHILD_ENV_EXPLICIT-unset}\"", + None, + 5000, + false, + None, + false, + None, + extra, + ) + .expect("execute"); + + match previous { + Some(value) => unsafe { + std::env::set_var("DEEPSEEK_CHILD_ENV_SHELL_SECRET", value); + }, + None => unsafe { + std::env::remove_var("DEEPSEEK_CHILD_ENV_SHELL_SECRET"); + }, + } + + assert_eq!(result.status, ShellStatus::Completed); + assert_eq!(result.stdout, "unset\nexplicit-value\n"); +} + #[test] fn test_sync_execution() { let tmp = tempdir().expect("tempdir");