From aa845b89906a774fe8dd09f255558eafcb4e4f61 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 9 May 2025 11:38:39 -0700 Subject: [PATCH] feat: experimental env var: CODEX_SANDBOX_NETWORK_DISABLED Previous to this change: ``` $ cargo run --bin codex -- debug seatbelt --full-auto -- cargo test ---- keeps_previous_response_id_between_tasks stdout ---- thread 'keeps_previous_response_id_between_tasks' panicked at /Users/mbolin/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/wiremock-0.6.3/src/mock_server/builder.rs:107:46: Failed to bind an OS port for a mock server.: Os { code: 1, kind: PermissionDenied, message: "Operation not permitted" } note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace failures: keeps_previous_response_id_between_tasks test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s error: test failed, to rerun pass `-p codex-core --test previous_response_id` ``` --- codex-rs/cli/src/landlock.rs | 9 +- codex-rs/cli/src/seatbelt.rs | 26 +++-- codex-rs/core/src/exec.rs | 102 +++++++++++++++----- codex-rs/core/src/linux.rs | 27 +++++- codex-rs/core/tests/previous_response_id.rs | 8 ++ codex-rs/core/tests/stream_no_completed.rs | 8 ++ codex-rs/mcp-client/src/mcp_client.rs | 1 + 7 files changed, 140 insertions(+), 41 deletions(-) diff --git a/codex-rs/cli/src/landlock.rs b/codex-rs/cli/src/landlock.rs index bc43eb57cd..892d238e1b 100644 --- a/codex-rs/cli/src/landlock.rs +++ b/codex-rs/cli/src/landlock.rs @@ -3,10 +3,11 @@ //! 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::exec::StdioPolicy; +use codex_core::linux::spawn_command_under_landlock; use codex_core::protocol::SandboxPolicy; use std::os::unix::process::ExitStatusExt; use std::process; -use std::process::Command; use std::process::ExitStatus; /// Execute `command` in a Linux sandbox (Landlock + seccomp) the way Codex @@ -19,8 +20,10 @@ pub fn run_landlock(command: Vec, sandbox_policy: SandboxPolicy) -> anyh // Spawn a new thread and apply the sandbox policies there. let handle = std::thread::spawn(move || -> anyhow::Result { let cwd = std::env::current_dir()?; - codex_core::linux::apply_sandbox_policy_to_current_thread(sandbox_policy, &cwd)?; - let status = Command::new(&command[0]).args(&command[1..]).status()?; + let mut child = + spawn_command_under_landlock(command, &sandbox_policy, cwd, StdioPolicy::Inherit) + .await?; + let status = child.wait().await?; Ok(status) }); let status = handle diff --git a/codex-rs/cli/src/seatbelt.rs b/codex-rs/cli/src/seatbelt.rs index 3c7ec2ba93..00a41fb739 100644 --- a/codex-rs/cli/src/seatbelt.rs +++ b/codex-rs/cli/src/seatbelt.rs @@ -1,18 +1,24 @@ -use codex_core::exec::create_seatbelt_command; +use codex_core::exec::StdioPolicy; +use codex_core::exec::spawn_command_under_seatbelt; use codex_core::protocol::SandboxPolicy; +use std::os::unix::process::ExitStatusExt; +use std::process; pub async fn run_seatbelt( command: Vec, sandbox_policy: SandboxPolicy, ) -> anyhow::Result<()> { let cwd = std::env::current_dir().expect("failed to get cwd"); - let seatbelt_command = create_seatbelt_command(command, &sandbox_policy, &cwd); - let status = tokio::process::Command::new(seatbelt_command[0].clone()) - .args(&seatbelt_command[1..]) - .spawn() - .map_err(|e| anyhow::anyhow!("Failed to spawn command: {}", e))? - .wait() - .await - .map_err(|e| anyhow::anyhow!("Failed to wait for command: {}", e))?; - std::process::exit(status.code().unwrap_or(1)); + let mut child = + spawn_command_under_seatbelt(command, &sandbox_policy, cwd, StdioPolicy::Inherit).await?; + let status = child.wait().await?; + + // Use ExitStatus to derive the exit code. + if let Some(code) = status.code() { + process::exit(code); + } else if let Some(signal) = status.signal() { + process::exit(128 + signal); + } else { + process::exit(1); + } } diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index aa761d2e7d..2d248740d9 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -42,6 +42,16 @@ const MACOS_SEATBELT_BASE_POLICY: &str = include_str!("seatbelt_base_policy.sbpl /// already has root access. const MACOS_PATH_TO_SEATBELT_EXECUTABLE: &str = "/usr/bin/sandbox-exec"; +/// Experimental environment variable that will be set to some non-empty value +/// if both of the following are true: +/// +/// 1. The process was spawned by Codex as part of a shell tool call. +/// 2. SandboxPolicy.has_full_network_access() was false for the tool call. +/// +/// We may try to have just one environment variable for all sandboxing +/// attributes, so this may change in the future. +pub const CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR: &str = "CODEX_SANDBOX_NETWORK_DISABLED"; + #[derive(Debug, Clone)] pub struct ExecParams { pub command: Vec, @@ -90,23 +100,21 @@ pub async fn process_exec_tool_call( let start = Instant::now(); let raw_output_result = match sandbox_type { - SandboxType::None => exec(params, ctrl_c).await, + SandboxType::None => exec(params, sandbox_policy, ctrl_c).await, SandboxType::MacosSeatbelt => { let ExecParams { command, cwd, timeout_ms, } = params; - let seatbelt_command = create_seatbelt_command(command, sandbox_policy, &cwd); - exec( - ExecParams { - command: seatbelt_command, - cwd, - timeout_ms, - }, - ctrl_c, + let child = spawn_command_under_seatbelt( + command, + sandbox_policy, + cwd, + StdioPolicy::RedirectForShellTool, ) - .await + .await?; + consume_truncated_output(child, ctrl_c, timeout_ms).await } SandboxType::LinuxSeccomp => exec_linux(params, ctrl_c, sandbox_policy).await, }; @@ -151,7 +159,17 @@ pub async fn process_exec_tool_call( } } -pub fn create_seatbelt_command( +pub async fn spawn_command_under_seatbelt( + command: Vec, + sandbox_policy: &SandboxPolicy, + cwd: PathBuf, + stdio_policy: StdioPolicy, +) -> std::io::Result { + let seatbelt_command = create_seatbelt_command(command, sandbox_policy, &cwd); + spawn_child(seatbelt_command, cwd, sandbox_policy, stdio_policy).await +} + +fn create_seatbelt_command( command: Vec, sandbox_policy: &SandboxPolicy, cwd: &Path, @@ -235,16 +253,41 @@ pub async fn exec( cwd, timeout_ms, }: ExecParams, + sandbox_policy: &SandboxPolicy, ctrl_c: Arc, ) -> Result { - let child = spawn_child(command, cwd).await?; + let child = spawn_child( + command, + cwd, + sandbox_policy, + StdioPolicy::RedirectForShellTool, + ) + .await?; consume_truncated_output(child, ctrl_c, timeout_ms).await } +#[derive(Debug, Clone, Copy)] +pub enum StdioPolicy { + RedirectForShellTool, + Inherit, +} + /// Spawns the appropriate child process for the ExecParams. -async fn spawn_child(command: Vec, cwd: PathBuf) -> std::io::Result { +pub(crate) async fn spawn_child( + command: Vec, + cwd: PathBuf, + sandbox_policy: &SandboxPolicy, + stdio_policy: StdioPolicy, +) -> std::io::Result { + // For now, we take `SandboxPolicy` as a parameter to spawn_child() because + // we need to determine whether to set the + // `CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR` environment variable. + // Ultimately, we should be stricter about the environment variables that + // are set for the command (as we are when spawning an MCP server), so + // instead of SandboxPolicy, we should take the exact env to use for the + // Command (i.e., `env_clear().envs(env)`). if command.is_empty() { - return Err(std::io::Error::new( + return Err(io::Error::new( io::ErrorKind::InvalidInput, "command args are empty", )); @@ -254,16 +297,29 @@ async fn spawn_child(command: Vec, cwd: PathBuf) -> std::io::Result { + // Do not create a file descriptor for stdin because otherwise some + // commands may hang forever waiting for input. For example, ripgrep has + // a heuristic where it may try to read from stdin as explained here: + // https://github.com/BurntSushi/ripgrep/blob/e2362d4d5185d02fa857bf381e7bd52e66fafc73/crates/core/flags/hiargs.rs#L1101-L1103 + cmd.stdin(Stdio::null()); + + cmd.stdout(Stdio::piped()).stderr(Stdio::piped()); + } + StdioPolicy::Inherit => { + // Inherit stdin, stdout, and stderr from the parent process. + cmd.stdin(Stdio::inherit()) + .stdout(Stdio::inherit()) + .stderr(Stdio::inherit()); + } + } + + cmd.kill_on_drop(true).spawn() } /// Consumes the output of a child process, truncating it so it is suitable for diff --git a/codex-rs/core/src/linux.rs b/codex-rs/core/src/linux.rs index 9928cfee4e..c001591b6d 100644 --- a/codex-rs/core/src/linux.rs +++ b/codex-rs/core/src/linux.rs @@ -9,7 +9,8 @@ use crate::error::Result; use crate::error::SandboxErr; use crate::exec::ExecParams; use crate::exec::RawExecToolCallOutput; -use crate::exec::exec; +use crate::exec::StdioPolicy; +use crate::exec::spawn_child; use crate::protocol::SandboxPolicy; use landlock::ABI; @@ -49,8 +50,14 @@ pub async fn exec_linux( .expect("Failed to create runtime"); rt.block_on(async { - apply_sandbox_policy_to_current_thread(sandbox_policy, ¶ms.cwd)?; - exec(params, ctrl_c_copy).await + ExecParams { + command, + cwd, + timeout_ms, + } = params; + let child = + spawn_command_under_landlock(command, &sandbox_policy, cwd, StdioPolicy::Inherit)?; + consume_truncated_output(child, ctrl_c, timeout_ms).await }) }) .join(); @@ -65,10 +72,20 @@ pub async fn exec_linux( } } +pub async fn spawn_command_under_landlock( + command: Vec, + sandbox_policy: &SandboxPolicy, + cwd: PathBuf, + stdio_policy: StdioPolicy, +) -> std::io::Result { + apply_sandbox_policy_to_current_thread(&sandbox_policy, ¶ms.cwd)?; + spawn_child(command, cwd, &sandbox_policy, stdio_policy).await +} + /// Apply sandbox policies inside this thread so only the child inherits /// them, not the entire CLI process. -pub fn apply_sandbox_policy_to_current_thread( - sandbox_policy: SandboxPolicy, +fn apply_sandbox_policy_to_current_thread( + sandbox_policy: &SandboxPolicy, cwd: &Path, ) -> Result<()> { if !sandbox_policy.has_full_network_access() { diff --git a/codex-rs/core/tests/previous_response_id.rs b/codex-rs/core/tests/previous_response_id.rs index c318f38ba5..2c899df0e9 100644 --- a/codex-rs/core/tests/previous_response_id.rs +++ b/codex-rs/core/tests/previous_response_id.rs @@ -3,6 +3,7 @@ use std::time::Duration; use codex_core::Codex; use codex_core::ModelProviderInfo; use codex_core::config::Config; +use codex_core::exec::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR; use codex_core::protocol::InputItem; use codex_core::protocol::Op; use serde_json::Value; @@ -50,6 +51,13 @@ data: {{\"type\":\"response.completed\",\"response\":{{\"id\":\"{}\",\"output\": async fn keeps_previous_response_id_between_tasks() { #![allow(clippy::unwrap_used)] + if std::env::var(CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR).is_ok() { + println!( + "Skipping test because it cannot execute when network is disabled in a Codex sandbox." + ); + return; + } + // Mock server let server = MockServer::start().await; diff --git a/codex-rs/core/tests/stream_no_completed.rs b/codex-rs/core/tests/stream_no_completed.rs index cfb7d44b2c..5b50d7ac26 100644 --- a/codex-rs/core/tests/stream_no_completed.rs +++ b/codex-rs/core/tests/stream_no_completed.rs @@ -6,6 +6,7 @@ use std::time::Duration; use codex_core::Codex; use codex_core::ModelProviderInfo; use codex_core::config::Config; +use codex_core::exec::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR; use codex_core::protocol::InputItem; use codex_core::protocol::Op; use tokio::time::timeout; @@ -34,6 +35,13 @@ data: {{\"type\":\"response.completed\",\"response\":{{\"id\":\"{}\",\"output\": async fn retries_on_early_close() { #![allow(clippy::unwrap_used)] + if std::env::var(CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR).is_ok() { + println!( + "Skipping test because it cannot execute when network is disabled in a Codex sandbox." + ); + return; + } + let server = MockServer::start().await; struct SeqResponder; diff --git a/codex-rs/mcp-client/src/mcp_client.rs b/codex-rs/mcp-client/src/mcp_client.rs index 1c6a765c57..641de0e89a 100644 --- a/codex-rs/mcp-client/src/mcp_client.rs +++ b/codex-rs/mcp-client/src/mcp_client.rs @@ -81,6 +81,7 @@ impl McpClient { ) -> std::io::Result { let mut child = Command::new(program) .args(args) + .env_clear() .envs(create_env_for_mcp_server(env)) .stdin(std::process::Stdio::piped()) .stdout(std::process::Stdio::piped())