From d34bc66466670630ec9ca080c775e279c66d536a Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 14 Apr 2026 20:48:01 -0700 Subject: [PATCH] sandbox: remove dead seatbelt helper and update tests (#17859) ## Why `spawn_command_under_seatbelt()` in `codex-rs/core/src/seatbelt.rs` had fallen out of production use and was only referenced by test-only wrappers. That left us with sandbox tests that could stay green even if the actual seatbelt exec path regressed, because production shell execution now flows through `SandboxManager::transform()` and `ExecRequest::from_sandbox_exec_request()` instead of that helper. Removing the dead helper also exposed one downstream `codex-exec` integration test that still imported it, which broke `just clippy`. ## What Changed - Removed `codex-rs/core/src/seatbelt.rs` and stopped exporting `codex_core::seatbelt`. - Removed the redundant `codex-rs/core/tests/suite/seatbelt.rs` coverage that only exercised the dead helper. - Kept the `openpty` regression check, but moved it into `codex-rs/core/tests/suite/exec.rs` so it now runs through `process_exec_tool_call()`. - Fixed the seatbelt denial test in `codex-rs/core/tests/suite/exec.rs` to use `/usr/bin/touch`, so it actually exercises the sandbox instead of a nonexistent path. - Updated `codex-rs/exec/tests/suite/sandbox.rs` on macOS to build the sandboxed command through `build_exec_request()` and spawn the transformed command, instead of importing the removed helper. - Left the lower-level seatbelt policy coverage in `codex-rs/sandboxing/src/seatbelt_tests.rs`, where the policy generator is still covered directly. ## Verification - `cargo test -p codex-core suite::exec::` - `cargo test -p codex-exec` - `cargo clippy -p codex-exec --tests -- -D warnings` --- codex-rs/core/src/lib.rs | 1 - codex-rs/core/src/seatbelt.rs | 48 ---- codex-rs/core/tests/suite/exec.rs | 45 +++- codex-rs/core/tests/suite/mod.rs | 1 - codex-rs/core/tests/suite/seatbelt.rs | 316 -------------------------- codex-rs/exec/tests/suite/sandbox.rs | 66 +++++- 6 files changed, 97 insertions(+), 380 deletions(-) delete mode 100644 codex-rs/core/src/seatbelt.rs delete mode 100644 codex-rs/core/tests/suite/seatbelt.rs diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 5e62199d2..93d62faca 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -139,7 +139,6 @@ pub use project_doc::discover_project_doc_paths; pub use project_doc::read_project_docs; mod rollout; pub(crate) mod safety; -pub mod seatbelt; mod session_rollout_init_error; pub mod shell; pub(crate) mod shell_snapshot; diff --git a/codex-rs/core/src/seatbelt.rs b/codex-rs/core/src/seatbelt.rs deleted file mode 100644 index c54ff3fd5..000000000 --- a/codex-rs/core/src/seatbelt.rs +++ /dev/null @@ -1,48 +0,0 @@ -#![cfg(target_os = "macos")] - -use crate::spawn::CODEX_SANDBOX_ENV_VAR; -use crate::spawn::SpawnChildRequest; -use crate::spawn::StdioPolicy; -use crate::spawn::spawn_child_async; -use codex_network_proxy::NetworkProxy; -use codex_protocol::permissions::FileSystemSandboxPolicy; -use codex_protocol::permissions::NetworkSandboxPolicy; -use codex_protocol::protocol::SandboxPolicy; -use codex_sandboxing::seatbelt::MACOS_PATH_TO_SEATBELT_EXECUTABLE; -use codex_sandboxing::seatbelt::create_seatbelt_command_args_for_policies; -use codex_utils_absolute_path::AbsolutePathBuf; -use std::collections::HashMap; -use std::path::PathBuf; -use tokio::process::Child; - -pub async fn spawn_command_under_seatbelt( - command: Vec, - command_cwd: AbsolutePathBuf, - sandbox_policy: &SandboxPolicy, - sandbox_policy_cwd: &AbsolutePathBuf, - stdio_policy: StdioPolicy, - network: Option<&NetworkProxy>, - mut env: HashMap, -) -> std::io::Result { - let args = create_seatbelt_command_args_for_policies( - command, - &FileSystemSandboxPolicy::from_legacy_sandbox_policy(sandbox_policy, sandbox_policy_cwd), - NetworkSandboxPolicy::from(sandbox_policy), - sandbox_policy_cwd, - /*enforce_managed_network*/ false, - network, - ); - let arg0 = None; - env.insert(CODEX_SANDBOX_ENV_VAR.to_string(), "seatbelt".to_string()); - spawn_child_async(SpawnChildRequest { - program: PathBuf::from(MACOS_PATH_TO_SEATBELT_EXECUTABLE), - args, - arg0, - cwd: command_cwd, - network_sandbox_policy: NetworkSandboxPolicy::from(sandbox_policy), - network, - stdio_policy, - env, - }) - .await -} diff --git a/codex-rs/core/tests/suite/exec.rs b/codex-rs/core/tests/suite/exec.rs index 19869ebb2..c80c8a334 100644 --- a/codex-rs/core/tests/suite/exec.rs +++ b/codex-rs/core/tests/suite/exec.rs @@ -1,8 +1,5 @@ #![cfg(target_os = "macos")] -use std::collections::HashMap; -use std::string::ToString; - use codex_core::exec::ExecCapturePolicy; use codex_core::exec::ExecParams; use codex_core::exec::process_exec_tool_call; @@ -17,6 +14,7 @@ use codex_protocol::protocol::SandboxPolicy; use codex_sandboxing::SandboxType; use codex_sandboxing::get_platform_sandbox; use core_test_support::PathExt; +use std::collections::HashMap; use tempfile::TempDir; fn skip_test() -> bool { @@ -29,14 +27,18 @@ fn skip_test() -> bool { } #[expect(clippy::expect_used)] -async fn run_test_cmd(tmp: TempDir, cmd: Vec<&str>) -> Result { +async fn run_test_cmd(tmp: TempDir, command: I) -> Result +where + I: IntoIterator, + S: Into, +{ let sandbox_type = get_platform_sandbox(/*windows_sandbox_enabled*/ false) .expect("should be able to get sandbox type"); assert_eq!(sandbox_type, SandboxType::MacosSeatbelt); let cwd = tmp.path().abs(); let params = ExecParams { - command: cmd.iter().map(ToString::to_string).collect(), + command: command.into_iter().map(Into::into).collect(), cwd: cwd.clone(), expiration: 1000.into(), capture_policy: ExecCapturePolicy::ShellTool, @@ -129,6 +131,37 @@ async fn exit_command_not_found_is_ok() { run_test_cmd(tmp, cmd).await.unwrap(); } +#[tokio::test] +async fn openpty_works_under_real_exec_seatbelt_path() { + if skip_test() { + return; + } + + let python = match which::which("python3") { + Ok(path) => path, + Err(_) => { + eprintln!("python3 not found in PATH, skipping test."); + return; + } + }; + + let tmp = TempDir::new().expect("should be able to create temp dir"); + let cmd = vec![ + python.to_string_lossy().into_owned(), + "-c".to_string(), + r#"import os + +master, slave = os.openpty() +os.write(slave, b"ping") +assert os.read(master, 4) == b"ping""# + .to_string(), + ]; + + let output = run_test_cmd(tmp, cmd).await.unwrap(); + assert_eq!(output.stdout.text, ""); + assert_eq!(output.stderr.text, ""); +} + /// Writing a file fails and should be considered a sandbox error #[tokio::test] async fn write_file_fails_as_sandbox_error() { @@ -139,7 +172,7 @@ async fn write_file_fails_as_sandbox_error() { let tmp = TempDir::new().expect("should be able to create temp dir"); let path = tmp.path().join("test.txt"); let cmd = vec![ - "/user/bin/touch", + "/usr/bin/touch", path.to_str().expect("should be able to get path"), ]; diff --git a/codex-rs/core/tests/suite/mod.rs b/codex-rs/core/tests/suite/mod.rs index 8f3d9e15d..9f8389c45 100644 --- a/codex-rs/core/tests/suite/mod.rs +++ b/codex-rs/core/tests/suite/mod.rs @@ -85,7 +85,6 @@ mod rmcp_client; mod rollout_list_find; mod safety_check_downgrade; mod search_tool; -mod seatbelt; mod shell_command; mod shell_serialization; mod shell_snapshot; diff --git a/codex-rs/core/tests/suite/seatbelt.rs b/codex-rs/core/tests/suite/seatbelt.rs deleted file mode 100644 index 808a8a022..000000000 --- a/codex-rs/core/tests/suite/seatbelt.rs +++ /dev/null @@ -1,316 +0,0 @@ -#![cfg(target_os = "macos")] - -//! Tests for the macOS sandboxing that are specific to Seatbelt. -//! Tests that apply to both Mac and Linux sandboxing should go in sandbox.rs. - -use std::collections::HashMap; -use std::path::Path; -use std::path::PathBuf; - -use codex_core::seatbelt::spawn_command_under_seatbelt; -use codex_core::spawn::CODEX_SANDBOX_ENV_VAR; -use codex_core::spawn::StdioPolicy; -use codex_protocol::protocol::SandboxPolicy; -use codex_utils_absolute_path::AbsolutePathBuf; -use tempfile::TempDir; - -struct TestScenario { - repo_parent: PathBuf, - file_outside_repo: PathBuf, - repo_root: PathBuf, - file_in_repo_root: PathBuf, - file_in_dot_git_dir: PathBuf, -} - -struct TestExpectations { - file_outside_repo_is_writable: bool, - file_in_repo_root_is_writable: bool, - file_in_dot_git_dir_is_writable: bool, -} - -impl TestScenario { - async fn run_test(&self, policy: &SandboxPolicy, expectations: TestExpectations) { - if std::env::var(CODEX_SANDBOX_ENV_VAR) == Ok("seatbelt".to_string()) { - eprintln!("{CODEX_SANDBOX_ENV_VAR} is set to 'seatbelt', skipping test."); - return; - } - - assert_eq!( - touch(&self.file_outside_repo, policy).await, - expectations.file_outside_repo_is_writable - ); - assert_eq!( - self.file_outside_repo.exists(), - expectations.file_outside_repo_is_writable - ); - - assert_eq!( - touch(&self.file_in_repo_root, policy).await, - expectations.file_in_repo_root_is_writable - ); - assert_eq!( - self.file_in_repo_root.exists(), - expectations.file_in_repo_root_is_writable - ); - - assert_eq!( - touch(&self.file_in_dot_git_dir, policy).await, - expectations.file_in_dot_git_dir_is_writable - ); - assert_eq!( - self.file_in_dot_git_dir.exists(), - expectations.file_in_dot_git_dir_is_writable - ); - } -} - -/// If the user has added a workspace root that is not a Git repo root, then -/// the user has to specify `--skip-git-repo-check` or go through some -/// interstitial that indicates they are taking on some risk because Git -/// cannot be used to backup their work before the agent begins. -/// -/// Because the user has agreed to this risk, we do not try find all .git -/// folders in the workspace and block them (though we could change our -/// position on this in the future). -#[tokio::test] -async fn if_parent_of_repo_is_writable_then_dot_git_folder_is_writable() { - let tmp = TempDir::new().expect("should be able to create temp dir"); - let test_scenario = create_test_scenario(&tmp); - let policy = SandboxPolicy::WorkspaceWrite { - writable_roots: vec![test_scenario.repo_parent.as_path().try_into().unwrap()], - read_only_access: Default::default(), - network_access: false, - exclude_tmpdir_env_var: true, - exclude_slash_tmp: true, - }; - - test_scenario - .run_test( - &policy, - TestExpectations { - file_outside_repo_is_writable: true, - file_in_repo_root_is_writable: true, - file_in_dot_git_dir_is_writable: true, - }, - ) - .await; -} - -/// When the writable root is the root of a Git repository (as evidenced by the -/// presence of a .git folder), then the .git folder should be read-only if -/// the policy is `WorkspaceWrite`. -#[tokio::test] -async fn if_git_repo_is_writable_root_then_dot_git_folder_is_read_only() { - let tmp = TempDir::new().expect("should be able to create temp dir"); - let test_scenario = create_test_scenario(&tmp); - let policy = SandboxPolicy::WorkspaceWrite { - writable_roots: vec![test_scenario.repo_root.as_path().try_into().unwrap()], - read_only_access: Default::default(), - network_access: false, - exclude_tmpdir_env_var: true, - exclude_slash_tmp: true, - }; - - test_scenario - .run_test( - &policy, - TestExpectations { - file_outside_repo_is_writable: false, - file_in_repo_root_is_writable: true, - file_in_dot_git_dir_is_writable: false, - }, - ) - .await; -} - -/// Under DangerFullAccess, all writes should be permitted anywhere on disk, -/// including inside the .git folder. -#[tokio::test] -async fn danger_full_access_allows_all_writes() { - let tmp = TempDir::new().expect("should be able to create temp dir"); - let test_scenario = create_test_scenario(&tmp); - let policy = SandboxPolicy::DangerFullAccess; - - test_scenario - .run_test( - &policy, - TestExpectations { - file_outside_repo_is_writable: true, - file_in_repo_root_is_writable: true, - file_in_dot_git_dir_is_writable: true, - }, - ) - .await; -} - -/// Under ReadOnly, writes should not be permitted anywhere on disk. -#[tokio::test] -async fn read_only_forbids_all_writes() { - let tmp = TempDir::new().expect("should be able to create temp dir"); - let test_scenario = create_test_scenario(&tmp); - let policy = SandboxPolicy::new_read_only_policy(); - - test_scenario - .run_test( - &policy, - TestExpectations { - file_outside_repo_is_writable: false, - file_in_repo_root_is_writable: false, - file_in_dot_git_dir_is_writable: false, - }, - ) - .await; -} - -#[tokio::test] -async fn openpty_works_under_seatbelt() { - if std::env::var(CODEX_SANDBOX_ENV_VAR) == Ok("seatbelt".to_string()) { - eprintln!("{CODEX_SANDBOX_ENV_VAR} is set to 'seatbelt', skipping test."); - return; - } - - if which::which("python3").is_err() { - eprintln!("python3 not found in PATH, skipping test."); - return; - } - - let policy = SandboxPolicy::new_read_only_policy(); - let command_cwd = AbsolutePathBuf::current_dir().expect("getcwd"); - let sandbox_cwd = command_cwd.clone(); - - let mut child = spawn_command_under_seatbelt( - vec![ - "python3".to_string(), - "-c".to_string(), - r#"import os - -master, slave = os.openpty() -os.write(slave, b"ping") -assert os.read(master, 4) == b"ping""# - .to_string(), - ], - command_cwd, - &policy, - &sandbox_cwd, - StdioPolicy::RedirectForShellTool, - /*network*/ None, - HashMap::new(), - ) - .await - .expect("should be able to spawn python under seatbelt"); - - let status = child - .wait() - .await - .expect("should be able to wait for child process"); - assert!(status.success(), "python exited with {status:?}"); -} - -#[tokio::test] -async fn java_home_finds_runtime_under_seatbelt() { - if std::env::var(CODEX_SANDBOX_ENV_VAR) == Ok("seatbelt".to_string()) { - eprintln!("{CODEX_SANDBOX_ENV_VAR} is set to 'seatbelt', skipping test."); - return; - } - - let java_home_path = Path::new("/usr/libexec/java_home"); - if !java_home_path.exists() { - eprintln!("/usr/libexec/java_home is not present, skipping test."); - return; - } - - let baseline_output = tokio::process::Command::new(java_home_path) - .env_remove("JAVA_HOME") - .output() - .await - .expect("should be able to invoke java_home outside seatbelt"); - if !baseline_output.status.success() { - eprintln!( - "java_home exited with {:?} outside seatbelt, skipping test", - baseline_output.status - ); - return; - } - - let policy = SandboxPolicy::new_read_only_policy(); - let command_cwd = AbsolutePathBuf::current_dir().expect("getcwd"); - let sandbox_cwd = command_cwd.clone(); - - let mut env: HashMap = std::env::vars().collect(); - env.remove("JAVA_HOME"); - env.remove(CODEX_SANDBOX_ENV_VAR); - - let child = spawn_command_under_seatbelt( - vec![java_home_path.to_string_lossy().to_string()], - command_cwd, - &policy, - &sandbox_cwd, - StdioPolicy::RedirectForShellTool, - /*network*/ None, - env, - ) - .await - .expect("should be able to spawn java_home under seatbelt"); - - let output = child - .wait_with_output() - .await - .expect("should be able to wait for java_home child"); - assert!( - output.status.success(), - "java_home under seatbelt exited with {:?}, stderr: {}", - output.status, - String::from_utf8_lossy(&output.stderr) - ); - - let stdout = String::from_utf8_lossy(&output.stdout); - assert!( - !stdout.trim().is_empty(), - "java_home stdout unexpectedly empty under seatbelt" - ); -} - -#[expect(clippy::expect_used)] -fn create_test_scenario(tmp: &TempDir) -> TestScenario { - let repo_parent = tmp.path().to_path_buf(); - let repo_root = repo_parent.join("repo"); - let dot_git_dir = repo_root.join(".git"); - - std::fs::create_dir(&repo_root).expect("should be able to create repo root"); - std::fs::create_dir(&dot_git_dir).expect("should be able to create .git dir"); - - TestScenario { - file_outside_repo: repo_parent.join("outside.txt"), - repo_parent, - file_in_repo_root: repo_root.join("repo_file.txt"), - repo_root, - file_in_dot_git_dir: dot_git_dir.join("dot_git_file.txt"), - } -} - -#[expect(clippy::expect_used)] -/// Note that `path` must be absolute. -async fn touch(path: &Path, policy: &SandboxPolicy) -> bool { - assert!(path.is_absolute(), "Path must be absolute: {path:?}"); - let command_cwd = AbsolutePathBuf::current_dir().expect("getcwd"); - let sandbox_cwd = command_cwd.clone(); - let mut child = spawn_command_under_seatbelt( - vec![ - "/usr/bin/touch".to_string(), - path.to_string_lossy().to_string(), - ], - command_cwd, - policy, - &sandbox_cwd, - StdioPolicy::RedirectForShellTool, - /*network*/ None, - HashMap::new(), - ) - .await - .expect("should be able to spawn command under seatbelt"); - child - .wait() - .await - .expect("should be able to wait for child process") - .success() -} diff --git a/codex-rs/exec/tests/suite/sandbox.rs b/codex-rs/exec/tests/suite/sandbox.rs index 7a8c3238e..691b590f4 100644 --- a/codex-rs/exec/tests/suite/sandbox.rs +++ b/codex-rs/exec/tests/suite/sandbox.rs @@ -19,17 +19,67 @@ async fn spawn_command_under_sandbox( stdio_policy: StdioPolicy, env: HashMap, ) -> std::io::Result { - use codex_core::seatbelt::spawn_command_under_seatbelt; - spawn_command_under_seatbelt( - command, - command_cwd, + use codex_core::exec::ExecCapturePolicy; + use codex_core::exec::ExecParams; + use codex_core::exec::build_exec_request; + use codex_core::sandboxing::SandboxPermissions; + use codex_protocol::config_types::WindowsSandboxLevel; + use codex_protocol::permissions::FileSystemSandboxPolicy; + use codex_protocol::permissions::NetworkSandboxPolicy; + use std::process::Stdio; + + let codex_linux_sandbox_exe = None; + let exec_request = build_exec_request( + ExecParams { + command, + cwd: command_cwd, + expiration: 1000.into(), + capture_policy: ExecCapturePolicy::ShellTool, + env, + network: None, + sandbox_permissions: SandboxPermissions::UseDefault, + windows_sandbox_level: WindowsSandboxLevel::Disabled, + windows_sandbox_private_desktop: false, + justification: None, + arg0: None, + }, sandbox_policy, + &FileSystemSandboxPolicy::from_legacy_sandbox_policy(sandbox_policy, sandbox_cwd), + NetworkSandboxPolicy::from(sandbox_policy), sandbox_cwd, - stdio_policy, - /*network*/ None, - env, + &codex_linux_sandbox_exe, + /*use_legacy_landlock*/ false, ) - .await + .map_err(|err| io::Error::other(err.to_string()))?; + + let (program, args) = exec_request + .command + .split_first() + .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidInput, "command args are empty"))?; + + let mut child = tokio::process::Command::new(program); + if let Some(arg0) = exec_request.arg0.as_deref() { + child.arg0(arg0); + } + child.args(args); + child.current_dir(exec_request.cwd); + child.env_clear(); + child.envs(exec_request.env); + + match stdio_policy { + StdioPolicy::RedirectForShellTool => { + child.stdin(Stdio::null()); + child.stdout(Stdio::piped()).stderr(Stdio::piped()); + } + StdioPolicy::Inherit => { + child + .stdin(Stdio::inherit()) + .stdout(Stdio::inherit()) + .stderr(Stdio::inherit()); + } + } + + child.kill_on_drop(true).spawn() } #[cfg(target_os = "linux")]