From b733879c4b4555963ba4cd97dec1b8b2185ca810 Mon Sep 17 00:00:00 2001 From: Eva Wong Date: Fri, 24 Apr 2026 15:13:06 -0700 Subject: [PATCH] Add preserved path name policy primitive --- codex-rs/linux-sandbox/src/bwrap.rs | 68 +++++++- codex-rs/linux-sandbox/src/linux_run_main.rs | 135 ++++++++++++++- .../linux-sandbox/src/linux_run_main_tests.rs | 37 ++++ codex-rs/protocol/src/permissions.rs | 118 ++++++++++--- codex-rs/protocol/src/protocol.rs | 29 ++++ codex-rs/sandboxing/src/seatbelt.rs | 28 ++- codex-rs/sandboxing/src/seatbelt_tests.rs | 64 ++++--- .../shell-command/src/preserved_path_write.rs | 161 +----------------- codex-rs/tui/src/app/thread_routing.rs | 16 +- codex-rs/tui/src/app_server_session.rs | 8 +- 10 files changed, 436 insertions(+), 228 deletions(-) diff --git a/codex-rs/linux-sandbox/src/bwrap.rs b/codex-rs/linux-sandbox/src/bwrap.rs index e1a680bcf4..34fc574bf1 100644 --- a/codex-rs/linux-sandbox/src/bwrap.rs +++ b/codex-rs/linux-sandbox/src/bwrap.rs @@ -106,6 +106,7 @@ pub(crate) struct BwrapArgs { pub args: Vec, pub preserved_files: Vec, pub synthetic_mount_targets: Vec, + pub protected_create_targets: Vec, } #[derive(Clone, Copy, Debug, PartialEq, Eq)] @@ -138,6 +139,23 @@ pub(crate) struct SyntheticMountTarget { pre_existing_path: Option, } +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) struct ProtectedCreateTarget { + path: PathBuf, +} + +impl ProtectedCreateTarget { + pub(crate) fn missing(path: &Path) -> Self { + Self { + path: path.to_path_buf(), + } + } + + pub(crate) fn path(&self) -> &Path { + &self.path + } +} + impl SyntheticMountTarget { pub(crate) fn missing(path: &Path) -> Self { Self { @@ -228,6 +246,7 @@ pub(crate) fn create_bwrap_command_args( args: command, preserved_files: Vec::new(), synthetic_mount_targets: Vec::new(), + protected_create_targets: Vec::new(), }) } else { Ok(create_bwrap_flags_full_filesystem(command, options)) @@ -268,6 +287,7 @@ fn create_bwrap_flags_full_filesystem(command: Vec, options: BwrapOption args, preserved_files: Vec::new(), synthetic_mount_targets: Vec::new(), + protected_create_targets: Vec::new(), } } @@ -283,6 +303,7 @@ fn create_bwrap_flags( args: filesystem_args, preserved_files, synthetic_mount_targets, + protected_create_targets, } = create_filesystem_args( file_system_sandbox_policy, sandbox_policy_cwd, @@ -321,6 +342,7 @@ fn create_bwrap_flags( args, preserved_files, synthetic_mount_targets, + protected_create_targets, }) } @@ -361,6 +383,7 @@ fn create_filesystem_args( writable_roots.push(WritableRoot { root: AbsolutePathBuf::from_absolute_path("/")?, read_only_subpaths: Vec::new(), + preserved_path_names: Vec::new(), }); } let mut unreadable_roots = file_system_sandbox_policy @@ -456,6 +479,7 @@ fn create_filesystem_args( args, preserved_files: Vec::new(), synthetic_mount_targets: Vec::new(), + protected_create_targets: Vec::new(), }; let mut allowed_write_paths = Vec::with_capacity(writable_roots.len()); for writable_root in &writable_roots { @@ -518,6 +542,13 @@ fn create_filesystem_args( if let Some(target) = &symlink_target { read_only_subpaths = remap_paths_for_symlink_target(read_only_subpaths, root, target); } + append_protected_create_targets_for_writable_root( + &mut bwrap_args, + writable_root, + root, + symlink_target.as_deref(), + &read_only_subpaths, + ); read_only_subpaths.sort_by_key(|path| path_depth(path)); for subpath in read_only_subpaths { append_read_only_subpath_args(&mut bwrap_args, &subpath, &allowed_write_paths)?; @@ -555,6 +586,29 @@ fn create_filesystem_args( Ok(bwrap_args) } +fn append_protected_create_targets_for_writable_root( + bwrap_args: &mut BwrapArgs, + writable_root: &WritableRoot, + root: &Path, + symlink_target: Option<&Path>, + read_only_subpaths: &[PathBuf], +) { + for name in &writable_root.preserved_path_names { + let mut path = root.join(name); + if let Some(target) = symlink_target + && let Ok(relative_path) = path.strip_prefix(root) + { + path = target.join(relative_path); + } + if read_only_subpaths.iter().any(|subpath| subpath == &path) || path.exists() { + continue; + } + bwrap_args + .protected_create_targets + .push(ProtectedCreateTarget::missing(&path)); + } +} + fn expand_unreadable_globs_with_ripgrep( patterns: &[String], cwd: &Path, @@ -1669,7 +1723,12 @@ mod tests { ); assert!( !synthetic_mount_target_paths(&args).contains(&dot_git), - "missing child .git should not be tracked for post-bwrap cleanup", + "missing child .git should not be materialized as a synthetic mount target", + ); + assert_eq!( + protected_create_target_paths(&args), + vec![dot_git], + "missing child .git is enforced by the preserved-name sandbox policy", ); } @@ -2399,4 +2458,11 @@ mod tests { .map(|target| target.path().to_path_buf()) .collect() } + + fn protected_create_target_paths(args: &BwrapArgs) -> Vec { + args.protected_create_targets + .iter() + .map(|target| target.path().to_path_buf()) + .collect() + } } diff --git a/codex-rs/linux-sandbox/src/linux_run_main.rs b/codex-rs/linux-sandbox/src/linux_run_main.rs index 23245db44f..d6a9c18af3 100644 --- a/codex-rs/linux-sandbox/src/linux_run_main.rs +++ b/codex-rs/linux-sandbox/src/linux_run_main.rs @@ -32,6 +32,7 @@ const FORWARDED_SIGNALS: &[libc::c_int] = &[libc::SIGHUP, libc::SIGINT, libc::SIGQUIT, libc::SIGTERM]; const SYNTHETIC_MOUNT_MARKER_SYNTHETIC: &[u8] = b"synthetic\n"; const SYNTHETIC_MOUNT_MARKER_EXISTING: &[u8] = b"existing\n"; +const PROTECTED_CREATE_MARKER: &[u8] = b"protected-create\n"; #[derive(Debug)] struct SyntheticMountTargetRegistration { @@ -40,6 +41,13 @@ struct SyntheticMountTargetRegistration { marker_dir: PathBuf, } +#[derive(Debug)] +struct ProtectedCreateTargetRegistration { + target: crate::bwrap::ProtectedCreateTarget, + marker_file: PathBuf, + marker_dir: PathBuf, +} + #[derive(Debug, Parser)] /// CLI surface for the Linux sandbox helper. /// @@ -501,6 +509,7 @@ fn build_bwrap_argv( args: argv, preserved_files: bwrap_args.preserved_files, synthetic_mount_targets: bwrap_args.synthetic_mount_targets, + protected_create_targets: bwrap_args.protected_create_targets, } } @@ -590,7 +599,9 @@ fn resolve_true_command() -> String { } fn run_or_exec_bwrap(bwrap_args: crate::bwrap::BwrapArgs) -> ! { - if bwrap_args.synthetic_mount_targets.is_empty() { + if bwrap_args.synthetic_mount_targets.is_empty() + && bwrap_args.protected_create_targets.is_empty() + { exec_bwrap(bwrap_args.args, bwrap_args.preserved_files); } run_bwrap_in_child_with_synthetic_mount_cleanup(bwrap_args); @@ -601,8 +612,11 @@ fn run_bwrap_in_child_with_synthetic_mount_cleanup(bwrap_args: crate::bwrap::Bwr args, preserved_files, synthetic_mount_targets, + protected_create_targets, } = bwrap_args; let synthetic_mount_registrations = register_synthetic_mount_targets(&synthetic_mount_targets); + let protected_create_registrations = + register_protected_create_targets(&protected_create_targets); let parent_pid = unsafe { libc::getpid() }; let pid = unsafe { libc::fork() }; if pid < 0 { @@ -624,7 +638,9 @@ fn run_bwrap_in_child_with_synthetic_mount_cleanup(bwrap_args: crate::bwrap::Bwr let status = wait_for_bwrap_child(pid); BWRAP_CHILD_PID.store(0, Ordering::SeqCst); cleanup_synthetic_mount_targets(&synthetic_mount_registrations); - exit_with_wait_status(status); + let protected_create_violation = + cleanup_protected_create_targets(&protected_create_registrations); + exit_with_wait_status_or_policy_violation(status, protected_create_violation); } fn terminate_with_parent(parent_pid: libc::pid_t) { @@ -729,6 +745,37 @@ fn register_synthetic_mount_targets( }) } +fn register_protected_create_targets( + targets: &[crate::bwrap::ProtectedCreateTarget], +) -> Vec { + with_synthetic_mount_registry_lock(|| { + targets + .iter() + .map(|target| { + let marker_dir = synthetic_mount_marker_dir(target.path()); + fs::create_dir_all(&marker_dir).unwrap_or_else(|err| { + panic!( + "failed to create protected create marker directory {}: {err}", + marker_dir.display() + ) + }); + let marker_file = marker_dir.join(std::process::id().to_string()); + fs::write(&marker_file, PROTECTED_CREATE_MARKER).unwrap_or_else(|err| { + panic!( + "failed to register protected create target {}: {err}", + target.path().display() + ) + }); + ProtectedCreateTargetRegistration { + target: target.clone(), + marker_file, + marker_dir, + } + }) + .collect() + }) +} + fn synthetic_mount_marker_contents(target: &crate::bwrap::SyntheticMountTarget) -> &'static [u8] { if target.preserves_pre_existing_path() { SYNTHETIC_MOUNT_MARKER_EXISTING @@ -831,6 +878,75 @@ fn cleanup_synthetic_mount_targets(targets: &[SyntheticMountTargetRegistration]) }); } +fn cleanup_protected_create_targets(targets: &[ProtectedCreateTargetRegistration]) -> bool { + with_synthetic_mount_registry_lock(|| { + for target in targets.iter().rev() { + match fs::remove_file(&target.marker_file) { + Ok(()) => {} + Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} + Err(err) => panic!( + "failed to unregister protected create target {}: {err}", + target.target.path().display() + ), + } + } + + let mut violation = false; + for target in targets.iter().rev() { + if synthetic_mount_marker_dir_has_active_process(&target.marker_dir) { + if target.target.path().exists() { + violation = true; + } + continue; + } + violation |= remove_protected_create_target(&target.target); + match fs::remove_dir(&target.marker_dir) { + Ok(()) => {} + Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} + Err(err) if err.kind() == std::io::ErrorKind::DirectoryNotEmpty => {} + Err(err) => panic!( + "failed to remove protected create marker directory {}: {err}", + target.marker_dir.display() + ), + } + } + violation + }) +} + +fn remove_protected_create_target(target: &crate::bwrap::ProtectedCreateTarget) -> bool { + let path = target.path(); + let metadata = match fs::symlink_metadata(path) { + Ok(metadata) => metadata, + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return false, + Err(err) => panic!( + "failed to inspect protected create target {}: {err}", + path.display() + ), + }; + + if metadata.is_dir() { + fs::remove_dir_all(path).unwrap_or_else(|err| { + panic!( + "failed to remove protected create target directory {}: {err}", + path.display() + ) + }); + } else { + fs::remove_file(path).unwrap_or_else(|err| { + panic!( + "failed to remove protected create target file {}: {err}", + path.display() + ) + }); + } + eprintln!( + "sandbox blocked creation of preserved workspace metadata path {}", + path.display() + ); + true +} + fn remove_synthetic_mount_target(target: &crate::bwrap::SyntheticMountTarget) { let path = target.path(); let metadata = match fs::symlink_metadata(path) { @@ -947,6 +1063,17 @@ fn exit_with_wait_status(status: libc::c_int) -> ! { std::process::exit(1); } +fn exit_with_wait_status_or_policy_violation( + status: libc::c_int, + protected_create_violation: bool, +) -> ! { + if protected_create_violation && libc::WIFEXITED(status) && libc::WEXITSTATUS(status) == 0 { + std::process::exit(1); + } + + exit_with_wait_status(status); +} + /// Run a short-lived bubblewrap preflight in a child process and capture stderr. /// /// Strategy: @@ -964,8 +1091,11 @@ fn run_bwrap_in_child_capture_stderr(bwrap_args: crate::bwrap::BwrapArgs) -> Str args, preserved_files, synthetic_mount_targets, + protected_create_targets, } = bwrap_args; let synthetic_mount_registrations = register_synthetic_mount_targets(&synthetic_mount_targets); + let protected_create_registrations = + register_protected_create_targets(&protected_create_targets); let mut pipe_fds = [0; 2]; let pipe_res = unsafe { libc::pipe2(pipe_fds.as_mut_ptr(), libc::O_CLOEXEC) }; @@ -1009,6 +1139,7 @@ fn run_bwrap_in_child_capture_stderr(bwrap_args: crate::bwrap::BwrapArgs) -> Str wait_for_bwrap_child(pid); cleanup_synthetic_mount_targets(&synthetic_mount_registrations); + cleanup_protected_create_targets(&protected_create_registrations); String::from_utf8_lossy(&stderr_bytes).into_owned() } diff --git a/codex-rs/linux-sandbox/src/linux_run_main_tests.rs b/codex-rs/linux-sandbox/src/linux_run_main_tests.rs index dbeb3d0dce..4098e7ae64 100644 --- a/codex-rs/linux-sandbox/src/linux_run_main_tests.rs +++ b/codex-rs/linux-sandbox/src/linux_run_main_tests.rs @@ -352,6 +352,43 @@ fn cleanup_synthetic_mount_targets_preserves_real_pre_existing_empty_file() { assert!(empty_file.exists()); } +#[test] +fn cleanup_protected_create_targets_removes_created_path_and_reports_violation() { + let temp_dir = tempfile::TempDir::new().expect("tempdir"); + let dot_git = temp_dir.path().join(".git"); + let target = crate::bwrap::ProtectedCreateTarget::missing(&dot_git); + + let registrations = register_protected_create_targets(&[target]); + std::fs::create_dir(&dot_git).expect("create protected path"); + let violation = cleanup_protected_create_targets(®istrations); + + assert!(violation); + assert!(!dot_git.exists()); +} + +#[test] +fn cleanup_protected_create_targets_waits_for_other_active_registrations() { + let temp_dir = tempfile::TempDir::new().expect("tempdir"); + let dot_git = temp_dir.path().join(".git"); + let target = crate::bwrap::ProtectedCreateTarget::missing(&dot_git); + + let registrations = register_protected_create_targets(std::slice::from_ref(&target)); + let active_marker = registrations[0].marker_dir.join("1"); + std::fs::write(&active_marker, PROTECTED_CREATE_MARKER).expect("write active marker"); + std::fs::write(&dot_git, "").expect("create protected path"); + + let violation = cleanup_protected_create_targets(®istrations); + assert!(violation); + assert!(dot_git.exists()); + + std::fs::remove_file(active_marker).expect("remove active marker"); + let registrations = register_protected_create_targets(std::slice::from_ref(&target)); + let violation = cleanup_protected_create_targets(®istrations); + + assert!(violation); + assert!(!dot_git.exists()); +} + #[test] fn bwrap_signal_forwarder_terminates_child_and_keeps_parent_alive() { let supervisor_pid = unsafe { libc::fork() }; diff --git a/codex-rs/protocol/src/permissions.rs b/codex-rs/protocol/src/permissions.rs index 607c89c728..d450c2bc88 100644 --- a/codex-rs/protocol/src/permissions.rs +++ b/codex-rs/protocol/src/permissions.rs @@ -57,21 +57,16 @@ pub fn forbidden_agent_preserved_path_write( let target = resolve_candidate_path(path, cwd)?; let (preserved_path, preserved_name) = first_preserved_component(target.as_path())?; - if has_explicit_write_entry_for_path(file_system_sandbox_policy, &preserved_path, cwd) { + if has_explicit_write_entry_for_preserved_path( + file_system_sandbox_policy, + &preserved_path, + target.as_path(), + cwd, + ) { return None; } - if !file_system_sandbox_policy.can_write_path_with_cwd(preserved_path.as_path(), cwd) { - return Some(preserved_name); - } - - // A child directory of an existing Git repository must keep normal Git - // discovery working, so the sandbox layer intentionally does not - // materialize a missing child `.git`. Block that creation at command time. - if preserved_name == PRESERVED_GIT_PATH_NAME - && !preserved_path.as_path().exists() - && has_ancestor_git_metadata(preserved_path.as_path()) - { + if !file_system_sandbox_policy.can_write_path_with_cwd(target.as_path(), cwd) { return Some(preserved_name); } @@ -512,7 +507,7 @@ impl FileSystemSandboxPolicy { for writable_root in writable_roots { for protected_path in default_read_only_subpaths_for_writable_root( writable_root, - /*protect_missing_dot_codex*/ false, + /*protect_missing_preserved_paths*/ false, ) { append_default_read_only_path_if_no_explicit_rule( &mut file_system_policy.entries, @@ -617,7 +612,28 @@ impl FileSystemSandboxPolicy { } pub fn can_write_path_with_cwd(&self, path: &Path, cwd: &Path) -> bool { - self.resolve_access_with_cwd(path, cwd).can_write() + if !self.resolve_access_with_cwd(path, cwd).can_write() { + return false; + } + if self.has_full_disk_write_access() { + return true; + } + !self.is_preserved_path_write_denied(path, cwd) + } + + fn is_preserved_path_write_denied(&self, path: &Path, cwd: &Path) -> bool { + if !matches!(self.kind, FileSystemSandboxKind::Restricted) { + return false; + } + + let Some(target) = resolve_candidate_path(path, cwd) else { + return true; + }; + let Some((preserved_path, _)) = first_preserved_component(target.as_path()) else { + return false; + }; + + !has_explicit_write_entry_for_preserved_path(self, &preserved_path, target.as_path(), cwd) } pub fn with_additional_readable_roots( @@ -802,6 +818,10 @@ impl FileSystemSandboxPolicy { }), ); WritableRoot { + preserved_path_names: default_preserved_path_names_for_writable_root( + &root, + &resolved_entries, + ), root, // Preserve literal in-root protected paths like `.git` and // `.codex` so downstream sandboxes can still detect and mask @@ -1387,6 +1407,20 @@ pub(crate) fn default_read_only_subpaths_for_writable_root( dedup_absolute_paths(subpaths, /*normalize_effective_paths*/ false) } +fn default_preserved_path_names_for_writable_root( + writable_root: &AbsolutePathBuf, + entries: &[ResolvedFileSystemEntry], +) -> Vec { + PRESERVED_PATH_NAMES + .iter() + .filter(|name| { + let path = writable_root.join(**name); + !has_explicit_resolved_write_entry(entries, &path) + }) + .map(|name| (*name).to_string()) + .collect() +} + fn append_default_read_only_project_root_subpath_if_no_explicit_rule( entries: &mut Vec, subpath: impl Into, @@ -1467,8 +1501,9 @@ fn legacy_non_cwd_writable_roots( dedup_absolute_paths(roots, /*normalize_effective_paths*/ true) .into_iter() .map(|root| WritableRoot { + preserved_path_names: default_preserved_path_names_for_writable_root(&root, &[]), read_only_subpaths: default_read_only_subpaths_for_writable_root( - &root, /*protect_missing_dot_codex*/ false, + &root, /*protect_missing_preserved_paths*/ false, ), root, }) @@ -1482,6 +1517,15 @@ fn has_explicit_resolved_path_entry( entries.iter().any(|entry| &entry.path == path) } +fn has_explicit_resolved_write_entry( + entries: &[ResolvedFileSystemEntry], + path: &AbsolutePathBuf, +) -> bool { + entries + .iter() + .any(|entry| entry.access.can_write() && &entry.path == path) +} + fn first_preserved_component(path: &Path) -> Option<(AbsolutePathBuf, &'static str)> { let mut candidate = PathBuf::new(); for component in path.components() { @@ -1501,15 +1545,17 @@ fn preserved_path_name(name: &OsStr) -> Option<&'static str> { .find(|preserved| name == OsStr::new(preserved)) } -fn has_explicit_write_entry_for_path( +fn has_explicit_write_entry_for_preserved_path( policy: &FileSystemSandboxPolicy, - path: &AbsolutePathBuf, + preserved_path: &AbsolutePathBuf, + target: &Path, cwd: &Path, ) -> bool { - policy - .resolved_entries_with_cwd(cwd) - .iter() - .any(|entry| entry.access.can_write() && &entry.path == path) + policy.resolved_entries_with_cwd(cwd).iter().any(|entry| { + entry.access.can_write() + && target.starts_with(entry.path.as_path()) + && entry.path.as_path().starts_with(preserved_path.as_path()) + }) } fn has_ancestor_git_metadata(path: &Path) -> bool { @@ -1768,8 +1814,15 @@ mod tests { !writable_roots[0] .read_only_subpaths .contains(&expected_dot_git), - "missing child .git under an existing parent repo is enforced by command policy so Git discovery still works" + "missing child .git under an existing parent repo stays absent so Git discovery still works" ); + assert!( + writable_roots[0] + .preserved_path_names + .contains(&".git".to_string()), + "missing child .git creation is denied by the preserved-name policy primitive" + ); + assert!(!policy.can_write_path_with_cwd(expected_dot_git.join("config").as_path(), &cwd)); assert!( writable_roots[0] .read_only_subpaths @@ -1855,18 +1908,36 @@ mod tests { .contains(&explicit_dot_git), "explicit .git rule should win over the default preserved path carveout" ); + assert!( + !workspace_root + .preserved_path_names + .contains(&".git".to_string()), + "explicit .git rule should win over the preserved-name policy" + ); assert!( !workspace_root .read_only_subpaths .contains(&explicit_dot_agents), "explicit .agents rule should win over the default preserved path carveout" ); + assert!( + !workspace_root + .preserved_path_names + .contains(&".agents".to_string()), + "explicit .agents rule should win over the preserved-name policy" + ); assert!( !workspace_root .read_only_subpaths .contains(&explicit_dot_codex), "explicit .codex rule should win over the default protected carveout" ); + assert!( + !workspace_root + .preserved_path_names + .contains(&".codex".to_string()), + "explicit .codex rule should win over the preserved-name policy" + ); assert!( policy.can_write_path_with_cwd( explicit_dot_codex.join("config.toml").as_path(), @@ -1913,8 +1984,7 @@ mod tests { exclude_slash_tmp: true, }; - let file_system_policy = - FileSystemSandboxPolicy::from_legacy_sandbox_policy(&policy, cwd.path()); + let file_system_policy = FileSystemSandboxPolicy::from_legacy_sandbox_policy(&policy); assert!( !file_system_policy diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 67bad250b9..02c5e1e81d 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -83,6 +83,7 @@ pub use crate::permissions::FileSystemSandboxKind; pub use crate::permissions::FileSystemSandboxPolicy; pub use crate::permissions::FileSystemSpecialPath; pub use crate::permissions::NetworkSandboxPolicy; +use crate::permissions::PRESERVED_PATH_NAMES; use crate::permissions::default_read_only_subpaths_for_writable_root; pub use crate::request_permissions::RequestPermissionsArgs; pub use crate::request_user_input::RequestUserInputEvent; @@ -1091,6 +1092,10 @@ pub struct WritableRoot { /// By construction, these subpaths are all under `root`. pub read_only_subpaths: Vec, + + /// Path component names that must not be created or replaced under `root` + /// unless the policy grants an explicit write rule for that preserved path. + pub preserved_path_names: Vec, } impl WritableRoot { @@ -1107,8 +1112,31 @@ impl WritableRoot { } } + if self.path_contains_preserved_name(path) { + return false; + } + true } + + fn path_contains_preserved_name(&self, path: &Path) -> bool { + let Ok(relative_path) = path.strip_prefix(&self.root) else { + return false; + }; + + relative_path.components().any(|component| { + self.preserved_path_names + .iter() + .any(|name| component.as_os_str() == std::ffi::OsStr::new(name)) + }) + } +} + +fn default_preserved_path_names() -> Vec { + PRESERVED_PATH_NAMES + .iter() + .map(|name| (*name).to_string()) + .collect() } impl FromStr for SandboxPolicy { @@ -1257,6 +1285,7 @@ impl SandboxPolicy { &writable_root, /*protect_missing_preserved_paths*/ true, ), + preserved_path_names: default_preserved_path_names(), root: writable_root, } }) diff --git a/codex-rs/sandboxing/src/seatbelt.rs b/codex-rs/sandboxing/src/seatbelt.rs index c8b9e9f04b..60ba3f7d19 100644 --- a/codex-rs/sandboxing/src/seatbelt.rs +++ b/codex-rs/sandboxing/src/seatbelt.rs @@ -328,6 +328,7 @@ fn root_absolute_path() -> AbsolutePathBuf { struct SeatbeltAccessRoot { root: AbsolutePathBuf, excluded_subpaths: Vec, + preserved_path_names: Vec, } fn build_seatbelt_access_policy( @@ -342,9 +343,9 @@ 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.preserved_path_names.is_empty() { policy_components.push(format!("(subpath (param \"{root_param}\"))")); continue; } @@ -367,6 +368,11 @@ fn build_seatbelt_access_policy( "(require-not (subpath (param \"{excluded_param}\")))" )); } + for preserved_name in access_root.preserved_path_names { + let regex = + seatbelt_preserved_path_name_regex(&root, &preserved_name).replace('"', "\\\""); + require_parts.push(format!(r#"(require-not (regex #"{regex}"))"#)); + } policy_components.push(format!("(require-all {} )", require_parts.join(" "))); } @@ -380,6 +386,20 @@ fn build_seatbelt_access_policy( } } +fn seatbelt_preserved_path_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 build_seatbelt_unreadable_glob_policy( file_system_sandbox_policy: &FileSystemSandboxPolicy, cwd: &Path, @@ -586,6 +606,7 @@ pub fn create_seatbelt_command_args(args: CreateSeatbeltCommandArgsParams<'_>) - vec![SeatbeltAccessRoot { root: root_absolute_path(), excluded_subpaths: unreadable_roots.clone(), + preserved_path_names: Vec::new(), }], ) } @@ -599,6 +620,7 @@ pub fn create_seatbelt_command_args(args: CreateSeatbeltCommandArgsParams<'_>) - .map(|root| SeatbeltAccessRoot { root: root.root, excluded_subpaths: root.read_only_subpaths, + preserved_path_names: root.preserved_path_names, }) .collect(), ) @@ -618,6 +640,7 @@ pub fn create_seatbelt_command_args(args: CreateSeatbeltCommandArgsParams<'_>) - vec![SeatbeltAccessRoot { root: root_absolute_path(), excluded_subpaths: unreadable_roots, + preserved_path_names: Vec::new(), }], ); ( @@ -638,6 +661,7 @@ pub fn create_seatbelt_command_args(args: CreateSeatbeltCommandArgsParams<'_>) - .filter(|path| path.as_path().starts_with(root.as_path())) .cloned() .collect(), + preserved_path_names: Vec::new(), root, }) .collect(), diff --git a/codex-rs/sandboxing/src/seatbelt_tests.rs b/codex-rs/sandboxing/src/seatbelt_tests.rs index cf50baa2bd..95bb132109 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::PRESERVED_PATH_NAMES; use codex_protocol::protocol::ReadOnlyAccess; use codex_protocol::protocol::SandboxPolicy; use codex_utils_absolute_path::AbsolutePathBuf; @@ -60,6 +61,26 @@ fn seatbelt_policy_arg(args: &[String]) -> &str { .expect("seatbelt args should include policy text") } +fn seatbelt_preserved_path_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); + PRESERVED_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] @@ -1279,11 +1300,20 @@ fn create_seatbelt_args_for_cwd_as_git_repo() { .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"))) (require-not (literal (param "WRITABLE_ROOT_2_EXCLUDED_3"))) (require-not (subpath (param "WRITABLE_ROOT_2_EXCLUDED_3"))) (require-not (literal (param "WRITABLE_ROOT_2_EXCLUDED_4"))) (require-not (subpath (param "WRITABLE_ROOT_2_EXCLUDED_4"))) (require-not (literal (param "WRITABLE_ROOT_2_EXCLUDED_5"))) (require-not (subpath (param "WRITABLE_ROOT_2_EXCLUDED_5"))) )"# + let slash_tmp = PathBuf::from("/tmp") + .canonicalize() + .expect("canonicalize /tmp"); + let tempdir_policy_entry = if let Some(p) = &tmpdir_env_var { + let preserved_requirements = seatbelt_preserved_path_name_requirements(Path::new(p)); + format!( + 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"))) (require-not (literal (param "WRITABLE_ROOT_2_EXCLUDED_3"))) (require-not (subpath (param "WRITABLE_ROOT_2_EXCLUDED_3"))) (require-not (literal (param "WRITABLE_ROOT_2_EXCLUDED_4"))) (require-not (subpath (param "WRITABLE_ROOT_2_EXCLUDED_4"))) (require-not (literal (param "WRITABLE_ROOT_2_EXCLUDED_5"))) (require-not (subpath (param "WRITABLE_ROOT_2_EXCLUDED_5"))) {preserved_requirements} )"# + ) } else { - "" + String::new() }; + let root_0_preserved_requirements = + seatbelt_preserved_path_name_requirements(&vulnerable_root_canonical); + let root_1_preserved_requirements = seatbelt_preserved_path_name_requirements(&slash_tmp); // Build the expected policy text using a raw string for readability. // Note that the policy includes: @@ -1296,7 +1326,7 @@ fn create_seatbelt_args_for_cwd_as_git_repo() { ; 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"))) ) (require-all (subpath (param "WRITABLE_ROOT_1")) (require-not (literal (param "WRITABLE_ROOT_1_EXCLUDED_0"))) (require-not (subpath (param "WRITABLE_ROOT_1_EXCLUDED_0"))) (require-not (literal (param "WRITABLE_ROOT_1_EXCLUDED_1"))) (require-not (subpath (param "WRITABLE_ROOT_1_EXCLUDED_1"))) (require-not (literal (param "WRITABLE_ROOT_1_EXCLUDED_2"))) (require-not (subpath (param "WRITABLE_ROOT_1_EXCLUDED_2"))) ){tempdir_policy_entry} +(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"))) {root_0_preserved_requirements} ) (require-all (subpath (param "WRITABLE_ROOT_1")) (require-not (literal (param "WRITABLE_ROOT_1_EXCLUDED_0"))) (require-not (subpath (param "WRITABLE_ROOT_1_EXCLUDED_0"))) (require-not (literal (param "WRITABLE_ROOT_1_EXCLUDED_1"))) (require-not (subpath (param "WRITABLE_ROOT_1_EXCLUDED_1"))) (require-not (literal (param "WRITABLE_ROOT_1_EXCLUDED_2"))) (require-not (subpath (param "WRITABLE_ROOT_1_EXCLUDED_2"))) {root_1_preserved_requirements} ){tempdir_policy_entry} ) "#, @@ -1321,36 +1351,18 @@ fn create_seatbelt_args_for_cwd_as_git_repo() { "-DWRITABLE_ROOT_0_EXCLUDED_2={}", dot_codex_canonical.to_string_lossy() ), - format!( - "-DWRITABLE_ROOT_1={}", - PathBuf::from("/tmp") - .canonicalize() - .expect("canonicalize /tmp") - .to_string_lossy() - ), + format!("-DWRITABLE_ROOT_1={}", slash_tmp.to_string_lossy()), format!( "-DWRITABLE_ROOT_1_EXCLUDED_0={}", - PathBuf::from("/tmp") - .canonicalize() - .expect("canonicalize /tmp") - .join(".git") - .to_string_lossy() + slash_tmp.join(".git").to_string_lossy() ), format!( "-DWRITABLE_ROOT_1_EXCLUDED_1={}", - PathBuf::from("/tmp") - .canonicalize() - .expect("canonicalize /tmp") - .join(".agents") - .to_string_lossy() + slash_tmp.join(".agents").to_string_lossy() ), format!( "-DWRITABLE_ROOT_1_EXCLUDED_2={}", - PathBuf::from("/tmp") - .canonicalize() - .expect("canonicalize /tmp") - .join(".codex") - .to_string_lossy() + slash_tmp.join(".codex").to_string_lossy() ), ]; diff --git a/codex-rs/shell-command/src/preserved_path_write.rs b/codex-rs/shell-command/src/preserved_path_write.rs index ab966000dc..49be17468b 100644 --- a/codex-rs/shell-command/src/preserved_path_write.rs +++ b/codex-rs/shell-command/src/preserved_path_write.rs @@ -1,5 +1,4 @@ use std::path::Path; -use std::path::PathBuf; use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::permissions::forbidden_agent_preserved_path_write; @@ -9,18 +8,6 @@ pub fn preserved_path_write_forbidden_reason( cwd: &Path, file_system_sandbox_policy: &FileSystemSandboxPolicy, ) -> Option { - let commands = crate::bash::parse_shell_lc_plain_commands(command) - .or_else(|| crate::bash::parse_shell_lc_command_word_prefixes(command)) - .unwrap_or_else(|| vec![command.to_vec()]); - - for simple_command in commands { - if let Some(name) = - simple_command_preserved_path_write(&simple_command, cwd, file_system_sandbox_policy) - { - return Some(preserved_path_write_reason(name)); - } - } - if let Some(targets) = crate::bash::parse_shell_lc_write_redirection_targets(command) { for target in targets { if let Some(name) = forbidden_agent_preserved_path_write( @@ -39,89 +26,6 @@ fn preserved_path_write_reason(name: &str) -> String { format!("command targets preserved workspace metadata path `{name}`") } -fn simple_command_preserved_path_write( - command: &[String], - cwd: &Path, - file_system_sandbox_policy: &FileSystemSandboxPolicy, -) -> Option<&'static str> { - let program = command.first().map(|program| { - Path::new(program) - .file_name() - .and_then(|name| name.to_str()) - .unwrap_or(program) - })?; - - match program { - "git" => git_init_preserved_path_write(command, cwd, file_system_sandbox_policy), - "touch" | "mkdir" | "rm" | "rmdir" | "ln" | "mv" | "cp" | "install" => command - .iter() - .skip(1) - .filter(|arg| !arg.starts_with('-')) - .find_map(|arg| { - forbidden_agent_preserved_path_write( - Path::new(arg), - cwd, - file_system_sandbox_policy, - ) - }), - _ => None, - } -} - -fn git_init_preserved_path_write( - command: &[String], - cwd: &Path, - file_system_sandbox_policy: &FileSystemSandboxPolicy, -) -> Option<&'static str> { - let mut git_cwd = PathBuf::from(cwd); - let mut index = 1; - - while index < command.len() { - match command[index].as_str() { - "-C" => { - let next = command.get(index + 1)?; - git_cwd = resolve_shell_operand(Path::new(next), &git_cwd); - index += 2; - } - "--" => { - index += 1; - break; - } - arg if arg.starts_with('-') => { - index += 1; - } - _ => break, - } - } - - if command.get(index).map(String::as_str) != Some("init") { - return None; - } - - let init_target = command - .iter() - .skip(index + 1) - .find(|arg| !arg.starts_with('-')) - .map_or_else( - || git_cwd.clone(), - |arg| resolve_shell_operand(Path::new(arg), &git_cwd), - ); - - forbidden_agent_preserved_path_write( - init_target.join(".git").as_path(), - &git_cwd, - file_system_sandbox_policy, - ) -} - -fn resolve_shell_operand(path: &Path, cwd: &Path) -> PathBuf { - if path.is_absolute() { - path.to_path_buf() - } else { - cwd.join(path) - } -} - #[cfg(test)] mod tests { use std::path::Path; @@ -160,7 +64,7 @@ mod tests { } } - fn legacy_workspace_write_policy(cwd: &Path) -> FileSystemSandboxPolicy { + fn legacy_workspace_write_policy() -> FileSystemSandboxPolicy { let policy = SandboxPolicy::WorkspaceWrite { writable_roots: vec![], read_only_access: ReadOnlyAccess::Restricted { @@ -171,31 +75,7 @@ mod tests { exclude_tmpdir_env_var: true, exclude_slash_tmp: true, }; - FileSystemSandboxPolicy::from_legacy_sandbox_policy(&policy, cwd) - } - - #[test] - fn preserved_path_detector_blocks_git_init_under_parent_repo() { - let repo = TestDir::new("git-init-under-parent-repo"); - std::fs::create_dir(repo.path().join(".git")).expect("create parent .git"); - let cwd = repo.path().join("sub"); - std::fs::create_dir(&cwd).expect("create cwd"); - let policy = legacy_workspace_write_policy(&cwd); - - let reason = preserved_path_write_forbidden_reason( - &[ - "/bin/bash".to_string(), - "-lc".to_string(), - "git init".to_string(), - ], - &cwd, - &policy, - ); - - assert_eq!( - reason, - Some("command targets preserved workspace metadata path `.git`".to_string()) - ); + FileSystemSandboxPolicy::from_legacy_sandbox_policy(&policy) } #[test] @@ -204,7 +84,7 @@ mod tests { std::fs::create_dir(repo.path().join(".git")).expect("create parent .git"); let cwd = repo.path().join("sub"); std::fs::create_dir(&cwd).expect("create cwd"); - let policy = legacy_workspace_write_policy(&cwd); + let policy = legacy_workspace_write_policy(); let reason = preserved_path_write_forbidden_reason( &[ @@ -220,9 +100,9 @@ mod tests { } #[test] - fn preserved_path_detector_blocks_direct_preserved_path_writes() { + fn preserved_path_detector_leaves_direct_writes_to_sandbox_policy() { let cwd = TestDir::new("direct-preserved-path-writes"); - let policy = legacy_workspace_write_policy(cwd.path()); + let policy = legacy_workspace_write_policy(); let reason = preserved_path_write_forbidden_reason( &[ @@ -234,10 +114,7 @@ mod tests { &policy, ); - assert_eq!( - reason, - Some("command targets preserved workspace metadata path `.git`".to_string()) - ); + assert_eq!(reason, None); } #[test] @@ -246,7 +123,7 @@ mod tests { std::fs::create_dir(repo.path().join(".git")).expect("create parent .git"); let cwd = repo.path().join("sub"); std::fs::create_dir(&cwd).expect("create cwd"); - let policy = legacy_workspace_write_policy(&cwd); + let policy = legacy_workspace_write_policy(); let reason = preserved_path_write_forbidden_reason( &[ @@ -263,28 +140,4 @@ mod tests { Some("command targets preserved workspace metadata path `.git`".to_string()) ); } - - #[test] - fn preserved_path_detector_blocks_git_init_inside_complex_script() { - let repo = TestDir::new("git-init-inside-complex-script"); - std::fs::create_dir(repo.path().join(".git")).expect("create parent .git"); - let cwd = repo.path().join("sub"); - std::fs::create_dir(&cwd).expect("create cwd"); - let policy = legacy_workspace_write_policy(&cwd); - - let reason = preserved_path_write_forbidden_reason( - &[ - "/bin/bash".to_string(), - "-lc".to_string(), - "set -e\nif git init -q; then\n exit 22\nfi".to_string(), - ], - &cwd, - &policy, - ); - - assert_eq!( - reason, - Some("command targets preserved workspace metadata path `.git`".to_string()) - ); - } } diff --git a/codex-rs/tui/src/app/thread_routing.rs b/codex-rs/tui/src/app/thread_routing.rs index 3d7c6edbb5..ae27144bbf 100644 --- a/codex-rs/tui/src/app/thread_routing.rs +++ b/codex-rs/tui/src/app/thread_routing.rs @@ -617,7 +617,6 @@ impl App { runtime_permission_profile_for_turn_start( self.runtime_sandbox_policy_override.as_ref(), sandbox_policy, - cwd.as_path(), ), model.to_string(), effort, @@ -1490,7 +1489,6 @@ impl App { fn runtime_permission_profile_for_turn_start( runtime_sandbox_policy_override: Option<&SandboxPolicy>, sandbox_policy: &SandboxPolicy, - cwd: &std::path::Path, ) -> Option { runtime_sandbox_policy_override?; match sandbox_policy { @@ -1498,10 +1496,7 @@ fn runtime_permission_profile_for_turn_start( SandboxPolicy::ReadOnly { .. } | SandboxPolicy::WorkspaceWrite { .. } | SandboxPolicy::DangerFullAccess => Some( - codex_protocol::models::PermissionProfile::from_legacy_sandbox_policy( - sandbox_policy, - cwd, - ), + codex_protocol::models::PermissionProfile::from_legacy_sandbox_policy(sandbox_policy), ), } } @@ -1514,28 +1509,23 @@ mod tests { #[test] fn runtime_permission_profile_for_turn_start_only_when_sandbox_was_overridden() { - let cwd = std::path::Path::new("/tmp/project"); let sandbox_policy = SandboxPolicy::DangerFullAccess; assert_eq!( runtime_permission_profile_for_turn_start( /*runtime_sandbox_policy_override*/ None, &sandbox_policy, - cwd, ), None ); let profile = - runtime_permission_profile_for_turn_start(Some(&sandbox_policy), &sandbox_policy, cwd) + runtime_permission_profile_for_turn_start(Some(&sandbox_policy), &sandbox_policy) .expect("runtime sandbox override should send active permissions"); assert_eq!( profile, - codex_protocol::models::PermissionProfile::from_legacy_sandbox_policy( - &sandbox_policy, - cwd, - ) + codex_protocol::models::PermissionProfile::from_legacy_sandbox_policy(&sandbox_policy) ); } } diff --git a/codex-rs/tui/src/app_server_session.rs b/codex-rs/tui/src/app_server_session.rs index 6bf6be4308..57afc558e0 100644 --- a/codex-rs/tui/src/app_server_session.rs +++ b/codex-rs/tui/src/app_server_session.rs @@ -1529,10 +1529,8 @@ mod tests { #[test] fn embedded_turn_start_permission_overrides_send_runtime_profile_only_when_provided() { - let cwd = std::path::Path::new("/tmp/project"); let sandbox_policy = SandboxPolicy::DangerFullAccess; - let permission_profile = - PermissionProfile::from_legacy_sandbox_policy(&sandbox_policy, cwd); + let permission_profile = PermissionProfile::from_legacy_sandbox_policy(&sandbox_policy); assert_eq!( turn_start_permission_overrides( @@ -1555,10 +1553,8 @@ mod tests { #[test] fn remote_turn_start_permission_overrides_keep_legacy_sandbox_policy() { - let cwd = std::path::Path::new("/tmp/project"); let sandbox_policy = SandboxPolicy::DangerFullAccess; - let permission_profile = - PermissionProfile::from_legacy_sandbox_policy(&sandbox_policy, cwd); + let permission_profile = PermissionProfile::from_legacy_sandbox_policy(&sandbox_policy); assert_eq!( turn_start_permission_overrides(