From 68bee26a51620465b75c5020445549523c27d6d0 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 1 Aug 2025 11:31:24 -0700 Subject: [PATCH] feat: make .git read-only within a writable root when using Seatbelt --- codex-rs/core/src/seatbelt.rs | 54 +++++++-- codex-rs/core/src/spawn.rs | 2 + codex-rs/core/tests/sandbox.rs | 195 +++++++++++++++++++++++++++++++++ 3 files changed, 240 insertions(+), 11 deletions(-) create mode 100644 codex-rs/core/tests/sandbox.rs diff --git a/codex-rs/core/src/seatbelt.rs b/codex-rs/core/src/seatbelt.rs index be2acb1bdc..f0c041e5dd 100644 --- a/codex-rs/core/src/seatbelt.rs +++ b/codex-rs/core/src/seatbelt.rs @@ -4,6 +4,7 @@ use std::path::PathBuf; use tokio::process::Child; use crate::protocol::SandboxPolicy; +use crate::spawn::CODEX_SANDBOX_ENV_VAR; use crate::spawn::StdioPolicy; use crate::spawn::spawn_child_async; @@ -20,10 +21,11 @@ pub async fn spawn_command_under_seatbelt( sandbox_policy: &SandboxPolicy, cwd: PathBuf, stdio_policy: StdioPolicy, - env: HashMap, + mut env: HashMap, ) -> std::io::Result { let args = create_seatbelt_command_args(command, sandbox_policy, &cwd); let arg0 = None; + env.insert(CODEX_SANDBOX_ENV_VAR.to_string(), "seatbelt".to_string()); spawn_child_async( PathBuf::from(MACOS_PATH_TO_SEATBELT_EXECUTABLE), args, @@ -50,16 +52,45 @@ fn create_seatbelt_command_args( ) } else { let writable_roots = sandbox_policy.get_writable_roots_with_cwd(cwd); - let (writable_folder_policies, cli_args): (Vec, Vec) = writable_roots - .iter() - .enumerate() - .map(|(index, root)| { - let param_name = format!("WRITABLE_ROOT_{index}"); - let policy: String = format!("(subpath (param \"{param_name}\"))"); - let cli_arg = format!("-D{param_name}={}", root.to_string_lossy()); - (policy, cli_arg) - }) - .unzip(); + + let mut writable_folder_policies: Vec = Vec::new(); + let mut cli_args: Vec = Vec::new(); + + for (index, root) in writable_roots.iter().enumerate() { + // Canonicalize to avoid mismatches like /var vs /private/var on macOS. + let canonical_root = root.canonicalize().unwrap_or_else(|_| root.clone()); + let param_name = format!("WRITABLE_ROOT_{index}"); + cli_args.push(format!( + "-D{param_name}={}", + canonical_root.to_string_lossy() + )); + + // For WorkspaceWrite, if the writable root itself looks like a + // git repository (i.e., contains a top-level ".git" directory), + // then disallow writes specifically under that top-level ".git". + // Do NOT block ".git" directories under subdirectories: those + // are allowed when the parent of the repo is writable. + let policy_component = if let SandboxPolicy::WorkspaceWrite { .. } = sandbox_policy + { + let top_level_git = canonical_root.join(".git"); + if top_level_git.is_dir() { + let git_param_name = format!("WRITABLE_ROOT_{index}_GIT"); + cli_args.push(format!( + "-D{git_param_name}={}", + top_level_git.to_string_lossy() + )); + format!( + "(require-all (subpath (param \"{param_name}\")) (require-not (subpath (param \"{git_param_name}\"))))" + ) + } else { + format!("(subpath (param \"{param_name}\"))") + } + } else { + format!("(subpath (param \"{param_name}\"))") + }; + writable_folder_policies.push(policy_component); + } + if writable_folder_policies.is_empty() { ("".to_string(), Vec::::new()) } else { @@ -88,6 +119,7 @@ fn create_seatbelt_command_args( let full_policy = format!( "{MACOS_SEATBELT_BASE_POLICY}\n{file_read_policy}\n{file_write_policy}\n{network_policy}" ); + let mut seatbelt_args: Vec = vec!["-p".to_string(), full_policy]; seatbelt_args.extend(extra_cli_args); seatbelt_args.push("--".to_string()); diff --git a/codex-rs/core/src/spawn.rs b/codex-rs/core/src/spawn.rs index 9fde26539b..26a9e63494 100644 --- a/codex-rs/core/src/spawn.rs +++ b/codex-rs/core/src/spawn.rs @@ -17,6 +17,8 @@ use crate::protocol::SandboxPolicy; /// attributes, so this may change in the future. pub const CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR: &str = "CODEX_SANDBOX_NETWORK_DISABLED"; +pub const CODEX_SANDBOX_ENV_VAR: &str = "CODEX_SANDBOX"; + #[derive(Debug, Clone, Copy)] pub enum StdioPolicy { RedirectForShellTool, diff --git a/codex-rs/core/tests/sandbox.rs b/codex-rs/core/tests/sandbox.rs new file mode 100644 index 0000000000..b31ade863b --- /dev/null +++ b/codex-rs/core/tests/sandbox.rs @@ -0,0 +1,195 @@ +#![cfg(target_os = "macos")] +#![expect(clippy::expect_used)] + +use std::collections::HashMap; +use std::path::Path; +use std::path::PathBuf; + +use codex_core::protocol::SandboxPolicy; +use codex_core::seatbelt::spawn_command_under_seatbelt; +use codex_core::spawn::CODEX_SANDBOX_ENV_VAR; +use codex_core::spawn::StdioPolicy; +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.clone()], + network_access: false, + use_exact_writable_roots: 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.clone()], + network_access: false, + use_exact_writable_roots: 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::ReadOnly; + + 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; +} + +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"), + } +} + +/// Note that `path` must be absolute. +async fn touch(path: &Path, policy: &SandboxPolicy) -> bool { + assert!(path.is_absolute(), "Path must be absolute: {path:?}"); + let mut child = spawn_command_under_seatbelt( + vec![ + "/usr/bin/touch".to_string(), + path.to_string_lossy().to_string(), + ], + policy, + std::env::current_dir().expect("should be able to get current dir"), + StdioPolicy::RedirectForShellTool, + 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() +}