diff --git a/codex-rs/sandboxing/src/seatbelt.rs b/codex-rs/sandboxing/src/seatbelt.rs index c8b9e9f04..9383c77b3 100644 --- a/codex-rs/sandboxing/src/seatbelt.rs +++ b/codex-rs/sandboxing/src/seatbelt.rs @@ -4,7 +4,9 @@ use codex_network_proxy::has_proxy_url_env_vars; use codex_network_proxy::proxy_url_env_value; use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::permissions::NetworkSandboxPolicy; +use codex_protocol::permissions::PROTECTED_METADATA_PATH_NAMES; use codex_protocol::protocol::SandboxPolicy; +use codex_protocol::protocol::WritableRoot; use codex_utils_absolute_path::AbsolutePathBuf; use std::collections::BTreeMap; use std::collections::BTreeSet; @@ -328,6 +330,7 @@ fn root_absolute_path() -> AbsolutePathBuf { struct SeatbeltAccessRoot { root: AbsolutePathBuf, excluded_subpaths: Vec, + protected_metadata_names: Vec, } fn build_seatbelt_access_policy( @@ -342,9 +345,11 @@ fn build_seatbelt_access_policy( let root = normalize_path_for_sandbox(access_root.root.as_path()).unwrap_or(access_root.root); let root_param = format!("{param_prefix}_{index}"); - params.push((root_param.clone(), root.into_path_buf())); + params.push((root_param.clone(), root.clone().into_path_buf())); - if access_root.excluded_subpaths.is_empty() { + if access_root.excluded_subpaths.is_empty() + && access_root.protected_metadata_names.is_empty() + { policy_components.push(format!("(subpath (param \"{root_param}\"))")); continue; } @@ -367,6 +372,11 @@ fn build_seatbelt_access_policy( "(require-not (subpath (param \"{excluded_param}\")))" )); } + for metadata_name in access_root.protected_metadata_names { + let regex = + seatbelt_protected_metadata_name_regex(&root, &metadata_name).replace('"', "\\\""); + require_parts.push(format!(r#"(require-not (regex #"{regex}"))"#)); + } policy_components.push(format!("(require-all {} )", require_parts.join(" "))); } @@ -380,6 +390,38 @@ fn build_seatbelt_access_policy( } } +fn seatbelt_protected_metadata_name_regex(root: &AbsolutePathBuf, name: &str) -> String { + let mut root = root.to_string_lossy().to_string(); + while root.len() > 1 && root.ends_with('/') { + root.pop(); + } + let root = regex_lite::escape(&root); + let name = regex_lite::escape(name); + if root == "/" { + format!(r#"^/{name}(/.*)?$"#) + } else { + format!(r#"^{root}/{name}(/.*)?$"#) + } +} + +fn protected_metadata_names_for_writable_root( + file_system_sandbox_policy: &FileSystemSandboxPolicy, + writable_root: &WritableRoot, + cwd: &Path, +) -> Vec { + let mut names = writable_root.protected_metadata_names.clone(); + for name in PROTECTED_METADATA_PATH_NAMES { + if names.iter().any(|existing| existing == name) { + continue; + } + let path = writable_root.root.join(*name); + if !file_system_sandbox_policy.can_write_path_with_cwd(path.as_path(), cwd) { + names.push((*name).to_string()); + } + } + names +} + fn build_seatbelt_unreadable_glob_policy( file_system_sandbox_policy: &FileSystemSandboxPolicy, cwd: &Path, @@ -586,6 +628,7 @@ pub fn create_seatbelt_command_args(args: CreateSeatbeltCommandArgsParams<'_>) - vec![SeatbeltAccessRoot { root: root_absolute_path(), excluded_subpaths: unreadable_roots.clone(), + protected_metadata_names: Vec::new(), }], ) } @@ -597,6 +640,11 @@ pub fn create_seatbelt_command_args(args: CreateSeatbeltCommandArgsParams<'_>) - .get_writable_roots_with_cwd(sandbox_policy_cwd) .into_iter() .map(|root| SeatbeltAccessRoot { + protected_metadata_names: protected_metadata_names_for_writable_root( + file_system_sandbox_policy, + &root, + sandbox_policy_cwd, + ), root: root.root, excluded_subpaths: root.read_only_subpaths, }) @@ -618,6 +666,7 @@ pub fn create_seatbelt_command_args(args: CreateSeatbeltCommandArgsParams<'_>) - vec![SeatbeltAccessRoot { root: root_absolute_path(), excluded_subpaths: unreadable_roots, + protected_metadata_names: Vec::new(), }], ); ( @@ -638,6 +687,7 @@ pub fn create_seatbelt_command_args(args: CreateSeatbeltCommandArgsParams<'_>) - .filter(|path| path.as_path().starts_with(root.as_path())) .cloned() .collect(), + protected_metadata_names: Vec::new(), root, }) .collect(), diff --git a/codex-rs/sandboxing/src/seatbelt_tests.rs b/codex-rs/sandboxing/src/seatbelt_tests.rs index 4962b1490..a8f14fc26 100644 --- a/codex-rs/sandboxing/src/seatbelt_tests.rs +++ b/codex-rs/sandboxing/src/seatbelt_tests.rs @@ -26,6 +26,7 @@ use codex_protocol::permissions::FileSystemSandboxEntry; use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::permissions::FileSystemSpecialPath; use codex_protocol::permissions::NetworkSandboxPolicy; +use codex_protocol::permissions::PROTECTED_METADATA_PATH_NAMES; use codex_protocol::protocol::SandboxPolicy; use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; @@ -59,6 +60,26 @@ fn seatbelt_policy_arg(args: &[String]) -> &str { .expect("seatbelt args should include policy text") } +fn seatbelt_protected_metadata_name_requirements(root: &Path) -> String { + let mut root = root.to_string_lossy().to_string(); + while root.len() > 1 && root.ends_with('/') { + root.pop(); + } + let root = regex_lite::escape(&root); + PROTECTED_METADATA_PATH_NAMES + .iter() + .map(|name| { + let name = regex_lite::escape(name); + if root == "/" { + format!(r#"(require-not (regex #"^/{name}(/.*)?$"))"#) + } else { + format!(r#"(require-not (regex #"^{root}/{name}(/.*)?$"))"#) + } + }) + .collect::>() + .join(" ") +} + struct TestConfigReloader; #[async_trait::async_trait] @@ -185,6 +206,12 @@ fn explicit_unreadable_paths_are_excluded_from_full_disk_read_and_write_access() policy.contains("(require-not (subpath (param \"WRITABLE_ROOT_0_EXCLUDED_0\")))"), "expected write carveout in policy:\n{policy}" ); + assert!( + policy.contains(&seatbelt_protected_metadata_name_requirements(Path::new( + "/" + ))), + "expected metadata protection regex deny requirements in policy:\n{policy}" + ); assert!( args.iter().any( |arg| arg == &format!("-DREADABLE_ROOT_0_EXCLUDED_0={}", unreadable_root.display()) @@ -773,12 +800,13 @@ fn create_seatbelt_args_full_network_with_proxy_is_still_proxy_only() { #[test] fn create_seatbelt_args_with_read_only_git_and_codex_subpaths() { // Create a temporary workspace with two writable roots: one containing - // top-level .git and .codex directories and one without them. + // top-level workspace metadata paths and one without them. let tmp = TempDir::new().expect("tempdir"); let PopulatedTmp { vulnerable_root, vulnerable_root_canonical, dot_git_canonical, + dot_agents_canonical: _, dot_codex_canonical, empty_root, empty_root_canonical, @@ -841,8 +869,22 @@ fn create_seatbelt_args_with_read_only_git_and_codex_subpaths() { "expected explicit writable root .git/.codex carveouts in policy:\n{policy_text}", ); assert!( - policy_text.contains("(subpath (param \"WRITABLE_ROOT_2\"))"), - "expected second explicit writable root grant in policy:\n{policy_text}", + policy_text.contains(&seatbelt_protected_metadata_name_requirements( + &cwd.canonicalize().expect("canonicalize cwd") + )), + "expected cwd metadata protection regex requirements in policy:\n{policy_text}", + ); + assert!( + policy_text.contains(&seatbelt_protected_metadata_name_requirements( + &vulnerable_root_canonical + )), + "expected populated root metadata protection regex requirements in policy:\n{policy_text}", + ); + assert!( + policy_text.contains(&seatbelt_protected_metadata_name_requirements( + &empty_root_canonical + )), + "expected empty root metadata protection regex requirements in policy:\n{policy_text}", ); let expected_definitions = [ @@ -1014,7 +1056,7 @@ fn create_seatbelt_args_with_read_only_git_and_codex_subpaths() { } #[test] -fn create_seatbelt_args_block_first_time_dot_codex_creation_with_exact_and_descendant_carveouts() { +fn create_seatbelt_args_block_first_time_dot_codex_creation_with_metadata_name_regex() { let tmp = TempDir::new().expect("tempdir"); let repo_root = tmp.path().join("repo"); fs::create_dir_all(&repo_root).expect("create repo root"); @@ -1056,12 +1098,10 @@ fn create_seatbelt_args_block_first_time_dot_codex_creation_with_exact_and_desce let policy_text = seatbelt_policy_arg(&args); assert!( - policy_text.contains("(require-not (literal (param \"WRITABLE_ROOT_0_EXCLUDED_1\")))"), - "expected exact .codex carveout in policy:\n{policy_text}" - ); - assert!( - policy_text.contains("(require-not (subpath (param \"WRITABLE_ROOT_0_EXCLUDED_1\")))"), - "expected descendant .codex carveout in policy:\n{policy_text}" + policy_text.contains(&seatbelt_protected_metadata_name_requirements( + &repo_root.canonicalize().expect("canonicalize repo root") + )), + "expected metadata protection regex requirements in policy:\n{policy_text}" ); } @@ -1165,19 +1205,20 @@ fn create_seatbelt_args_with_read_only_git_pointer_file() { #[test] fn create_seatbelt_args_for_cwd_as_git_repo() { // Create a temporary workspace with two writable roots: one containing - // top-level .git and .codex directories and one without them. + // top-level workspace metadata paths and one without them. let tmp = TempDir::new().expect("tempdir"); let PopulatedTmp { vulnerable_root, vulnerable_root_canonical, dot_git_canonical, + dot_agents_canonical, dot_codex_canonical, .. } = populate_tmpdir(tmp.path()); // Build a policy that does not specify any writable_roots, but does - // use the default ones (cwd and TMPDIR) and verifies the `.git` and - // `.codex` checks are done properly for cwd. + // use the default ones (cwd and TMPDIR) and verifies the protected + // metadata checks are done properly for cwd. let policy = SandboxPolicy::WorkspaceWrite { writable_roots: vec![], network_access: false, @@ -1206,92 +1247,87 @@ fn create_seatbelt_args_for_cwd_as_git_repo() { /*network*/ None, ); - let tmpdir_env_var = std::env::var("TMPDIR") + let slash_tmp = PathBuf::from("/tmp") + .canonicalize() + .expect("canonicalize /tmp"); + let policy_text = seatbelt_policy_arg(&args); + assert!( + policy_text.contains(&seatbelt_protected_metadata_name_requirements( + &vulnerable_root_canonical + )), + "expected cwd metadata protection regex requirements in policy:\n{policy_text}", + ); + assert!( + policy_text.contains(&seatbelt_protected_metadata_name_requirements(&slash_tmp)), + "expected /tmp metadata protection regex requirements in policy:\n{policy_text}", + ); + if let Some(tmpdir_env_var) = std::env::var("TMPDIR") .ok() .map(PathBuf::from) .and_then(|p| p.canonicalize().ok()) - .map(|p| p.to_string_lossy().to_string()); - - let tempdir_policy_entry = if tmpdir_env_var.is_some() { - r#" (require-all (subpath (param "WRITABLE_ROOT_2")) (require-not (literal (param "WRITABLE_ROOT_2_EXCLUDED_0"))) (require-not (subpath (param "WRITABLE_ROOT_2_EXCLUDED_0"))) (require-not (literal (param "WRITABLE_ROOT_2_EXCLUDED_1"))) (require-not (subpath (param "WRITABLE_ROOT_2_EXCLUDED_1"))) (require-not (literal (param "WRITABLE_ROOT_2_EXCLUDED_2"))) (require-not (subpath (param "WRITABLE_ROOT_2_EXCLUDED_2"))) )"# - } else { - "" - }; - - // Build the expected policy text using a raw string for readability. - // Note that the policy includes: - // - the base policy, - // - read-only access to the filesystem, - // - write access to WRITABLE_ROOT_0 (but not its metadata subpaths), WRITABLE_ROOT_1, and cwd as WRITABLE_ROOT_2. - let expected_policy = format!( - r#"{MACOS_SEATBELT_BASE_POLICY} -; allow read-only file operations -(allow file-read*) -(allow file-write* -(require-all (subpath (param "WRITABLE_ROOT_0")) (require-not (literal (param "WRITABLE_ROOT_0_EXCLUDED_0"))) (require-not (subpath (param "WRITABLE_ROOT_0_EXCLUDED_0"))) (require-not (literal (param "WRITABLE_ROOT_0_EXCLUDED_1"))) (require-not (subpath (param "WRITABLE_ROOT_0_EXCLUDED_1"))) (require-not (literal (param "WRITABLE_ROOT_0_EXCLUDED_2"))) (require-not (subpath (param "WRITABLE_ROOT_0_EXCLUDED_2"))) ) (subpath (param "WRITABLE_ROOT_1")){tempdir_policy_entry} -) - -"#, - ); - - let mut expected_args = vec![ - "-p".to_string(), - expected_policy, - format!( - "-DWRITABLE_ROOT_0={}", - vulnerable_root_canonical.to_string_lossy() - ), - format!( - "-DWRITABLE_ROOT_0_EXCLUDED_0={}", - dot_git_canonical.to_string_lossy() - ), - format!( - "-DWRITABLE_ROOT_0_EXCLUDED_1={}", - dot_codex_canonical.to_string_lossy() - ), - format!( - "-DWRITABLE_ROOT_0_EXCLUDED_2={}", - vulnerable_root_canonical.join(".agents").to_string_lossy() - ), - format!( - "-DWRITABLE_ROOT_1={}", - PathBuf::from("/tmp") - .canonicalize() - .expect("canonicalize /tmp") - .to_string_lossy() - ), - ]; - - if let Some(p) = tmpdir_env_var { - expected_args.push(format!("-DWRITABLE_ROOT_2={p}")); - expected_args.push(format!( - "-DWRITABLE_ROOT_2_EXCLUDED_0={}", - dot_git_canonical.to_string_lossy() - )); - expected_args.push(format!( - "-DWRITABLE_ROOT_2_EXCLUDED_1={}", - vulnerable_root_canonical.join(".agents").to_string_lossy() - )); - expected_args.push(format!( - "-DWRITABLE_ROOT_2_EXCLUDED_2={}", - dot_codex_canonical.to_string_lossy() - )); + { + assert!( + policy_text.contains(&seatbelt_protected_metadata_name_requirements( + &tmpdir_env_var + )), + "expected TMPDIR metadata protection regex requirements in policy:\n{policy_text}", + ); } - expected_args.extend( - macos_dir_params() - .into_iter() - .map(|(key, value)| format!("-D{key}={value}", value = value.to_string_lossy())), + let expected_root = format!( + "-DWRITABLE_ROOT_0={}", + vulnerable_root_canonical.to_string_lossy() ); + assert!( + args.contains(&expected_root), + "missing {expected_root}: {args:#?}" + ); + let expected_dot_git = format!( + "-DWRITABLE_ROOT_0_EXCLUDED_0={}", + dot_git_canonical.to_string_lossy() + ); + assert!( + args.contains(&expected_dot_git), + "missing {expected_dot_git}: {args:#?}" + ); + let expected_dot_codex = format!( + "-DWRITABLE_ROOT_0_EXCLUDED_1={}", + dot_codex_canonical.to_string_lossy() + ); + assert!( + args.contains(&expected_dot_codex), + "missing {expected_dot_codex}: {args:#?}" + ); + let unexpected_dot_agents = format!( + "-DWRITABLE_ROOT_0_EXCLUDED_1={}", + dot_agents_canonical.to_string_lossy() + ); + assert!( + !args.contains(&unexpected_dot_agents), + "missing .agents should be handled by regex rather than materialized as a path param: {args:#?}" + ); + let expected_slash_tmp = format!("-DWRITABLE_ROOT_1={}", slash_tmp.to_string_lossy()); + assert!( + args.contains(&expected_slash_tmp), + "missing {expected_slash_tmp}: {args:#?}" + ); + for (key, value) in macos_dir_params() { + let expected_definition = format!("-D{key}={}", value.to_string_lossy()); + assert!( + args.contains(&expected_definition), + "expected definition arg `{expected_definition}` in {args:#?}" + ); + } - expected_args.push("--".to_string()); - expected_args.extend(shell_command); - - assert_eq!(expected_args, args); + let command_index = args + .iter() + .position(|arg| arg == "--") + .expect("seatbelt args should include command separator"); + assert_eq!(args[command_index + 1..], shell_command); } struct PopulatedTmp { - /// Path containing a .git and .codex subfolder. + /// Path containing protected metadata subfolders. /// For the purposes of this test, we consider this a "vulnerable" root /// because a bad actor could write to .git/hooks/pre-commit so an /// unsuspecting user would run code as privileged the next time they @@ -1301,9 +1337,10 @@ struct PopulatedTmp { vulnerable_root: PathBuf, vulnerable_root_canonical: PathBuf, dot_git_canonical: PathBuf, + dot_agents_canonical: PathBuf, dot_codex_canonical: PathBuf, - /// Path without .git or .codex subfolders. + /// Path without protected metadata subfolders. empty_root: PathBuf, /// Canonicalized version of `empty_root`. empty_root_canonical: PathBuf, @@ -1337,12 +1374,14 @@ fn populate_tmpdir(tmp: &Path) -> PopulatedTmp { .canonicalize() .expect("canonicalize vulnerable_root"); let dot_git_canonical = vulnerable_root_canonical.join(".git"); + let dot_agents_canonical = vulnerable_root_canonical.join(".agents"); let dot_codex_canonical = vulnerable_root_canonical.join(".codex"); let empty_root_canonical = empty_root.canonicalize().expect("canonicalize empty_root"); PopulatedTmp { vulnerable_root, vulnerable_root_canonical, dot_git_canonical, + dot_agents_canonical, dot_codex_canonical, empty_root, empty_root_canonical,