diff --git a/codex-rs/cli/src/landlock.rs b/codex-rs/cli/src/landlock.rs index bc43eb57cd..8d0d901b96 100644 --- a/codex-rs/cli/src/landlock.rs +++ b/codex-rs/cli/src/landlock.rs @@ -3,6 +3,8 @@ //! 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::exec::spawn_child; use codex_core::protocol::SandboxPolicy; use std::os::unix::process::ExitStatusExt; use std::process; @@ -19,8 +21,9 @@ 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()?; + codex_core::linux::apply_sandbox_policy_to_current_thread(&sandbox_policy, &cwd)?; + let child = spawn_child(command, cwd, sandbox_policy, StdioPolicy::Inherit)?; + let status = child.status()?; Ok(status) }); let status = handle diff --git a/codex-rs/cli/src/seatbelt.rs b/codex-rs/cli/src/seatbelt.rs index 3c7ec2ba93..b668253b81 100644 --- a/codex-rs/cli/src/seatbelt.rs +++ b/codex-rs/cli/src/seatbelt.rs @@ -1,4 +1,5 @@ -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; pub async fn run_seatbelt( @@ -6,10 +7,9 @@ pub async fn run_seatbelt( 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() + let child = + spawn_command_under_seatbelt(command, &sandbox_policy, cwd, StdioPolicy::Inherit).await; + let status = child .map_err(|e| anyhow::anyhow!("Failed to spawn command: {}", e))? .wait() .await diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index aa761d2e7d..0af563f659 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,39 @@ 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 } +pub enum StdioPolicy { + RedirectForShellTool, + Inherit, +} + /// Spawns the appropriate child process for the ExecParams. -async fn spawn_child(command: Vec, cwd: PathBuf) -> std::io::Result { +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 exec() 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 +295,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..55652edde5 100644 --- a/codex-rs/core/src/linux.rs +++ b/codex-rs/core/src/linux.rs @@ -49,8 +49,8 @@ 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 + apply_sandbox_policy_to_current_thread(&sandbox_policy, ¶ms.cwd)?; + exec(params, &sandbox_policy, ctrl_c_copy).await }) }) .join(); @@ -68,7 +68,7 @@ pub async fn exec_linux( /// 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, + 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())