diff --git a/codex-rs/linux-sandbox/src/bwrap.rs b/codex-rs/linux-sandbox/src/bwrap.rs index 08ecda6c51..6cbe76cff5 100644 --- a/codex-rs/linux-sandbox/src/bwrap.rs +++ b/codex-rs/linux-sandbox/src/bwrap.rs @@ -15,9 +15,11 @@ use std::collections::HashSet; use std::ffi::OsString; use std::fs; use std::fs::File; +use std::fs::Metadata; use std::io; use std::os::fd::AsRawFd; use std::os::unix::ffi::OsStringExt; +use std::os::unix::fs::MetadataExt; use std::path::Path; use std::path::PathBuf; use std::process::Command; @@ -101,6 +103,61 @@ impl BwrapNetworkMode { pub(crate) struct BwrapArgs { pub args: Vec, pub preserved_files: Vec, + pub synthetic_mount_targets: Vec, +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +struct FileIdentity { + dev: u64, + ino: u64, +} + +impl FileIdentity { + fn from_metadata(metadata: &Metadata) -> Self { + Self { + dev: metadata.dev(), + ino: metadata.ino(), + } + } +} + +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) struct SyntheticMountTarget { + path: PathBuf, + // If an empty preserved path was already present, remember its inode so + // cleanup does not delete a real pre-existing file. + pre_existing_file: Option, +} + +impl SyntheticMountTarget { + pub(crate) fn missing(path: &Path) -> Self { + Self { + path: path.to_path_buf(), + pre_existing_file: None, + } + } + + fn existing_empty_file(path: &Path, metadata: &Metadata) -> Self { + Self { + path: path.to_path_buf(), + pre_existing_file: Some(FileIdentity::from_metadata(metadata)), + } + } + + pub(crate) fn path(&self) -> &Path { + &self.path + } + + pub(crate) fn should_remove_after_bwrap(&self, metadata: &Metadata) -> bool { + if !metadata.file_type().is_file() || metadata.len() != 0 { + return false; + } + + match self.pre_existing_file { + Some(pre_existing_file) => pre_existing_file != FileIdentity::from_metadata(metadata), + None => true, + } + } } /// Wrap a command with bubblewrap so the filesystem is read-only by default, @@ -126,6 +183,7 @@ pub(crate) fn create_bwrap_command_args( Ok(BwrapArgs { args: command, preserved_files: Vec::new(), + synthetic_mount_targets: Vec::new(), }) } else { Ok(create_bwrap_flags_full_filesystem(command, options)) @@ -165,6 +223,7 @@ fn create_bwrap_flags_full_filesystem(command: Vec, options: BwrapOption BwrapArgs { args, preserved_files: Vec::new(), + synthetic_mount_targets: Vec::new(), } } @@ -179,6 +238,7 @@ fn create_bwrap_flags( let BwrapArgs { args: filesystem_args, preserved_files, + synthetic_mount_targets, } = create_filesystem_args( file_system_sandbox_policy, sandbox_policy_cwd, @@ -216,6 +276,7 @@ fn create_bwrap_flags( Ok(BwrapArgs { args, preserved_files, + synthetic_mount_targets, }) } @@ -348,6 +409,7 @@ fn create_filesystem_args( args }; let mut preserved_files = Vec::new(); + let mut synthetic_mount_targets = Vec::new(); let mut allowed_write_paths = Vec::with_capacity(writable_roots.len()); for writable_root in &writable_roots { let root = writable_root.root.as_path(); @@ -381,6 +443,7 @@ fn create_filesystem_args( append_unreadable_root_args( &mut args, &mut preserved_files, + &mut synthetic_mount_targets, unreadable_root, &allowed_write_paths, )?; @@ -416,7 +479,13 @@ fn create_filesystem_args( } read_only_subpaths.sort_by_key(|path| path_depth(path)); for subpath in read_only_subpaths { - append_read_only_subpath_args(&mut args, &subpath, &allowed_write_paths)?; + append_read_only_subpath_args( + &mut args, + &mut preserved_files, + &mut synthetic_mount_targets, + &subpath, + &allowed_write_paths, + )?; } let mut nested_unreadable_roots: Vec = unreadable_roots .iter() @@ -432,6 +501,7 @@ fn create_filesystem_args( append_unreadable_root_args( &mut args, &mut preserved_files, + &mut synthetic_mount_targets, &unreadable_root, &allowed_write_paths, )?; @@ -453,6 +523,7 @@ fn create_filesystem_args( append_unreadable_root_args( &mut args, &mut preserved_files, + &mut synthetic_mount_targets, &unreadable_root, &allowed_write_paths, )?; @@ -461,6 +532,7 @@ fn create_filesystem_args( Ok(BwrapArgs { args, preserved_files, + synthetic_mount_targets, }) } @@ -787,6 +859,8 @@ fn append_mount_target_parent_dir_args(args: &mut Vec, mount_target: &Pa fn append_read_only_subpath_args( args: &mut Vec, + preserved_files: &mut Vec, + synthetic_mount_targets: &mut Vec, subpath: &Path, allowed_write_paths: &[PathBuf], ) -> Result<()> { @@ -804,13 +878,32 @@ fn append_read_only_subpath_args( ))); } + if let Some(metadata) = transient_empty_preserved_file_metadata(subpath) + && is_within_allowed_write_paths(subpath, allowed_write_paths) + { + // Another concurrent bwrap setup can leave a zero-byte mount target at + // a missing preserved path. Treat it like the missing case instead of + // binding that transient host path as the stable source. + append_existing_empty_file_bind_data_args( + args, + preserved_files, + synthetic_mount_targets, + subpath, + &metadata, + )?; + return Ok(()); + } + if !subpath.exists() { if let Some(first_missing_component) = find_first_non_existent_component(subpath) && is_within_allowed_write_paths(&first_missing_component, allowed_write_paths) { - args.push("--ro-bind".to_string()); - args.push("/dev/null".to_string()); - args.push(path_to_string(&first_missing_component)); + append_missing_empty_file_bind_data_args( + args, + preserved_files, + synthetic_mount_targets, + &first_missing_component, + )?; } return Ok(()); } @@ -823,9 +916,48 @@ fn append_read_only_subpath_args( Ok(()) } +fn append_empty_file_bind_data_args( + args: &mut Vec, + preserved_files: &mut Vec, + path: &Path, +) -> Result<()> { + if preserved_files.is_empty() { + preserved_files.push(File::open("/dev/null")?); + } + let null_fd = preserved_files[0].as_raw_fd().to_string(); + args.push("--ro-bind-data".to_string()); + args.push(null_fd); + args.push(path_to_string(path)); + Ok(()) +} + +fn append_missing_empty_file_bind_data_args( + args: &mut Vec, + preserved_files: &mut Vec, + synthetic_mount_targets: &mut Vec, + path: &Path, +) -> Result<()> { + append_empty_file_bind_data_args(args, preserved_files, path)?; + synthetic_mount_targets.push(SyntheticMountTarget::missing(path)); + Ok(()) +} + +fn append_existing_empty_file_bind_data_args( + args: &mut Vec, + preserved_files: &mut Vec, + synthetic_mount_targets: &mut Vec, + path: &Path, + metadata: &Metadata, +) -> Result<()> { + append_empty_file_bind_data_args(args, preserved_files, path)?; + synthetic_mount_targets.push(SyntheticMountTarget::existing_empty_file(path, metadata)); + Ok(()) +} + fn append_unreadable_root_args( args: &mut Vec, preserved_files: &mut Vec, + synthetic_mount_targets: &mut Vec, unreadable_root: &Path, allowed_write_paths: &[PathBuf], ) -> Result<()> { @@ -850,9 +982,12 @@ fn append_unreadable_root_args( if let Some(first_missing_component) = find_first_non_existent_component(unreadable_root) && is_within_allowed_write_paths(&first_missing_component, allowed_write_paths) { - args.push("--ro-bind".to_string()); - args.push("/dev/null".to_string()); - args.push(path_to_string(&first_missing_component)); + append_missing_empty_file_bind_data_args( + args, + preserved_files, + synthetic_mount_targets, + &first_missing_component, + )?; } return Ok(()); } @@ -901,16 +1036,9 @@ fn append_existing_unreadable_path_args( return Ok(()); } - if preserved_files.is_empty() { - preserved_files.push(File::open("/dev/null")?); - } - let null_fd = preserved_files[0].as_raw_fd().to_string(); args.push("--perms".to_string()); args.push("000".to_string()); - args.push("--ro-bind-data".to_string()); - args.push(null_fd); - args.push(path_to_string(unreadable_root)); - Ok(()) + append_empty_file_bind_data_args(args, preserved_files, unreadable_root) } /// Returns true when `path` is under any allowed writable root. @@ -920,6 +1048,24 @@ fn is_within_allowed_write_paths(path: &Path, allowed_write_paths: &[PathBuf]) - .any(|root| path.starts_with(root)) } +fn transient_empty_preserved_file_metadata(path: &Path) -> Option { + if !has_preserved_path_name(path) { + return None; + } + + let metadata = fs::symlink_metadata(path).ok()?; + if metadata.file_type().is_file() && metadata.len() == 0 { + Some(metadata) + } else { + None + } +} + +fn has_preserved_path_name(path: &Path) -> bool { + path.file_name() + .is_some_and(|name| name == ".git" || name == ".agents" || name == ".codex") +} + fn first_writable_symlink_component_in_path( target_path: &Path, allowed_write_paths: &[PathBuf], @@ -1358,6 +1504,80 @@ mod tests { assert!(message.contains(&real_linked_private_str), "{message}"); } + #[test] + fn missing_read_only_subpath_uses_empty_file_bind_data() { + let temp_dir = TempDir::new().expect("temp dir"); + let workspace = temp_dir.path().join("workspace"); + let blocked = workspace.join("blocked"); + std::fs::create_dir_all(&workspace).expect("create workspace"); + + let workspace_root = + AbsolutePathBuf::from_absolute_path(&workspace).expect("absolute workspace"); + let blocked_root = AbsolutePathBuf::from_absolute_path(&blocked).expect("absolute blocked"); + let policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: workspace_root, + }, + access: FileSystemAccessMode::Write, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: blocked_root }, + access: FileSystemAccessMode::Read, + }, + ]); + + let args = + create_filesystem_args(&policy, temp_dir.path(), NO_UNREADABLE_GLOB_SCAN_MAX_DEPTH) + .expect("filesystem args"); + + assert_empty_file_bound_without_perms(&args.args, &blocked); + assert_eq!(args.preserved_files.len(), 1); + assert_eq!(synthetic_mount_target_paths(&args), vec![blocked.clone()]); + assert!( + !blocked.exists(), + "missing path mask should not materialize host-side preserved paths at arg construction time", + ); + } + + #[test] + fn transient_empty_preserved_file_uses_empty_file_bind_data() { + let temp_dir = TempDir::new().expect("temp dir"); + let workspace = temp_dir.path().join("workspace"); + let dot_git = workspace.join(".git"); + std::fs::create_dir_all(&workspace).expect("create workspace"); + File::create(&dot_git).expect("create empty .git file"); + + let workspace_root = + AbsolutePathBuf::from_absolute_path(&workspace).expect("absolute workspace"); + let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: workspace_root, + }, + access: FileSystemAccessMode::Write, + }]); + + let args = + create_filesystem_args(&policy, temp_dir.path(), NO_UNREADABLE_GLOB_SCAN_MAX_DEPTH) + .expect("filesystem args"); + let dot_git_str = path_to_string(&dot_git); + + assert_empty_file_bound_without_perms(&args.args, &dot_git); + assert_eq!(synthetic_mount_target_paths(&args), vec![dot_git.clone()]); + assert!( + !args + .args + .windows(3) + .any(|window| window == ["--ro-bind", dot_git_str.as_str(), dot_git_str.as_str()]), + "transient empty preserved file should not be treated as a stable bind source", + ); + let metadata = std::fs::symlink_metadata(&dot_git).expect("stat .git"); + assert!( + !args.synthetic_mount_targets[0].should_remove_after_bwrap(&metadata), + "pre-existing empty preserved files must not be cleaned up as synthetic targets", + ); + } + #[test] fn ignores_missing_writable_roots() { let temp_dir = TempDir::new().expect("temp dir"); @@ -1411,6 +1631,16 @@ mod tests { NO_UNREADABLE_GLOB_SCAN_MAX_DEPTH, ) .expect("bwrap fs args"); + assert_eq!(args.preserved_files.len(), 1); + assert_eq!( + synthetic_mount_target_paths(&args), + vec![ + PathBuf::from("/.git"), + PathBuf::from("/.agents"), + PathBuf::from("/.codex"), + ] + ); + let null_fd = args.preserved_files[0].as_raw_fd().to_string(); assert_eq!( args.args, vec![ @@ -1425,11 +1655,17 @@ mod tests { "--bind".to_string(), "/".to_string(), "/".to_string(), - // Mask the default protected .codex subpath under that writable + // Mask the default preserved .codex subpath under that writable // root. Because the root is `/` in this test, the carveout path - // appears as `/.codex`. - "--ro-bind".to_string(), - "/dev/null".to_string(), + // appears as `/.codex`, `/.agents`, and `/.git`. + "--ro-bind-data".to_string(), + null_fd.clone(), + "/.git".to_string(), + "--ro-bind-data".to_string(), + null_fd.clone(), + "/.agents".to_string(), + "--ro-bind-data".to_string(), + null_fd, "/.codex".to_string(), // Rebind /dev after the root bind so device nodes remain // writable/usable inside the writable root. @@ -1899,6 +2135,7 @@ mod tests { let blocked_file_str = path_to_string(blocked_file.as_path()); assert_eq!(args.preserved_files.len(), 1); + assert!(args.synthetic_mount_targets.is_empty()); assert!(args.args.windows(5).any(|window| { window[0] == "--perms" && window[1] == "000" @@ -2002,4 +2239,31 @@ mod tests { "expected file mask for {path}: {args:#?}" ); } + + /// Assert that `path` is backed by an fd-supplied empty file without + /// changing the next mount operation's permissions. + fn assert_empty_file_bound_without_perms(args: &[String], path: &Path) { + let path = path_to_string(path); + assert!( + args.windows(3) + .any(|window| { window[0] == "--ro-bind-data" && window[2] == path }), + "expected empty file bind for {path}: {args:#?}" + ); + assert!( + !args.windows(5).any(|window| { + window[0] == "--perms" + && window[1] == "000" + && window[2] == "--ro-bind-data" + && window[4] == path + }), + "missing path bind should not set explicit file perms for {path}: {args:#?}" + ); + } + + fn synthetic_mount_target_paths(args: &BwrapArgs) -> Vec { + args.synthetic_mount_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 0eede8bb81..c3d833875d 100644 --- a/codex-rs/linux-sandbox/src/linux_run_main.rs +++ b/codex-rs/linux-sandbox/src/linux_run_main.rs @@ -1,6 +1,7 @@ use clap::Parser; use std::ffi::CString; use std::fmt; +use std::fs; use std::fs::File; use std::io::Read; use std::os::fd::FromRawFd; @@ -443,7 +444,7 @@ fn run_bwrap_with_proc_fallback( options, ); apply_inner_command_argv0(&mut bwrap_args.args); - exec_bwrap(bwrap_args.args, bwrap_args.preserved_files); + run_or_exec_bwrap(bwrap_args); } fn bwrap_network_mode( @@ -480,6 +481,7 @@ fn build_bwrap_argv( crate::bwrap::BwrapArgs { args: argv, preserved_files: bwrap_args.preserved_files, + synthetic_mount_targets: bwrap_args.synthetic_mount_targets, } } @@ -568,6 +570,78 @@ fn resolve_true_command() -> String { "true".to_string() } +fn run_or_exec_bwrap(bwrap_args: crate::bwrap::BwrapArgs) -> ! { + if bwrap_args.synthetic_mount_targets.is_empty() { + exec_bwrap(bwrap_args.args, bwrap_args.preserved_files); + } + run_bwrap_in_child_with_synthetic_mount_cleanup(bwrap_args); +} + +fn run_bwrap_in_child_with_synthetic_mount_cleanup(bwrap_args: crate::bwrap::BwrapArgs) -> ! { + let synthetic_mount_targets = bwrap_args.synthetic_mount_targets.clone(); + let pid = unsafe { libc::fork() }; + if pid < 0 { + let err = std::io::Error::last_os_error(); + panic!("failed to fork for bubblewrap: {err}"); + } + + if pid == 0 { + exec_bwrap(bwrap_args.args, bwrap_args.preserved_files); + } + + let mut status: libc::c_int = 0; + let wait_res = unsafe { libc::waitpid(pid, &mut status as *mut libc::c_int, 0) }; + if wait_res < 0 { + let err = std::io::Error::last_os_error(); + panic!("waitpid failed for bubblewrap child: {err}"); + } + + cleanup_synthetic_mount_targets(&synthetic_mount_targets); + exit_with_wait_status(status); +} + +fn cleanup_synthetic_mount_targets(targets: &[crate::bwrap::SyntheticMountTarget]) { + for target in targets.iter().rev() { + let path = target.path(); + let metadata = match fs::symlink_metadata(path) { + Ok(metadata) => metadata, + Err(err) if err.kind() == std::io::ErrorKind::NotFound => continue, + Err(err) => panic!( + "failed to inspect synthetic bubblewrap mount target {}: {err}", + path.display() + ), + }; + if !target.should_remove_after_bwrap(&metadata) { + continue; + } + match fs::remove_file(path) { + Ok(()) => {} + Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} + Err(err) => panic!( + "failed to remove synthetic bubblewrap mount target {}: {err}", + path.display() + ), + } + } +} + +fn exit_with_wait_status(status: libc::c_int) -> ! { + if libc::WIFEXITED(status) { + std::process::exit(libc::WEXITSTATUS(status)); + } + + if libc::WIFSIGNALED(status) { + let signal = libc::WTERMSIG(status); + unsafe { + libc::signal(signal, libc::SIG_DFL); + libc::kill(libc::getpid(), signal); + } + std::process::exit(128 + signal); + } + + std::process::exit(1); +} + /// Run a short-lived bubblewrap preflight in a child process and capture stderr. /// /// Strategy: @@ -581,6 +655,7 @@ fn resolve_true_command() -> String { /// command, and reads are bounded to a fixed max size. fn run_bwrap_in_child_capture_stderr(bwrap_args: crate::bwrap::BwrapArgs) -> String { const MAX_PREFLIGHT_STDERR_BYTES: u64 = 64 * 1024; + let synthetic_mount_targets = bwrap_args.synthetic_mount_targets.clone(); let mut pipe_fds = [0; 2]; let pipe_res = unsafe { libc::pipe2(pipe_fds.as_mut_ptr(), libc::O_CLOEXEC) }; @@ -628,6 +703,7 @@ fn run_bwrap_in_child_capture_stderr(bwrap_args: crate::bwrap::BwrapArgs) -> Str let err = std::io::Error::last_os_error(); panic!("waitpid failed for bubblewrap child: {err}"); } + cleanup_synthetic_mount_targets(&synthetic_mount_targets); 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 0eef358424..4d54b4988f 100644 --- a/codex-rs/linux-sandbox/src/linux_run_main_tests.rs +++ b/codex-rs/linux-sandbox/src/linux_run_main_tests.rs @@ -255,6 +255,29 @@ fn managed_proxy_preflight_argv_is_wrapped_for_full_access_policy() { assert!(argv.iter().any(|arg| arg == "--")); } +#[test] +fn cleanup_synthetic_mount_targets_removes_only_empty_files() { + let temp_dir = tempfile::TempDir::new().expect("tempdir"); + let empty_file = temp_dir.path().join(".agents"); + let non_empty_file = temp_dir.path().join(".codex"); + let missing_file = temp_dir.path().join(".missing"); + std::fs::write(&empty_file, "").expect("write empty file"); + std::fs::write(&non_empty_file, "keep").expect("write nonempty file"); + + cleanup_synthetic_mount_targets(&[ + crate::bwrap::SyntheticMountTarget::missing(&empty_file), + crate::bwrap::SyntheticMountTarget::missing(&non_empty_file), + crate::bwrap::SyntheticMountTarget::missing(&missing_file), + ]); + + assert!(!empty_file.exists()); + assert_eq!( + std::fs::read_to_string(&non_empty_file).expect("read nonempty file"), + "keep" + ); + assert!(!missing_file.exists()); +} + #[test] fn managed_proxy_inner_command_includes_route_spec() { let sandbox_policy = SandboxPolicy::new_read_only_policy(); diff --git a/codex-rs/protocol/src/permissions.rs b/codex-rs/protocol/src/permissions.rs index 63c2e3c4f7..9040649c47 100644 --- a/codex-rs/protocol/src/permissions.rs +++ b/codex-rs/protocol/src/permissions.rs @@ -483,7 +483,7 @@ impl FileSystemSandboxPolicy { if let Ok(cwd_root) = AbsolutePathBuf::from_absolute_path(cwd) { for protected_path in default_read_only_subpaths_for_writable_root( - &cwd_root, /*protect_missing_dot_codex*/ true, + &cwd_root, /*protect_missing_preserved_paths*/ true, ) { append_default_read_only_path_if_no_explicit_rule( &mut file_system_policy.entries, @@ -494,7 +494,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, @@ -687,14 +687,17 @@ impl FileSystemSandboxPolicy { .iter() .filter(|path| normalize_effective_absolute_path((*path).clone()) == root) .collect(); - let protect_missing_dot_codex = AbsolutePathBuf::from_absolute_path(cwd) + let protect_missing_preserved_paths = AbsolutePathBuf::from_absolute_path(cwd) .ok() .is_some_and(|cwd| normalize_effective_absolute_path(cwd) == root); let mut read_only_subpaths: Vec = - default_read_only_subpaths_for_writable_root(&root, protect_missing_dot_codex) - .into_iter() - .filter(|path| !has_explicit_resolved_path_entry(&resolved_entries, path)) - .collect(); + default_read_only_subpaths_for_writable_root( + &root, + protect_missing_preserved_paths, + ) + .into_iter() + .filter(|path| !has_explicit_resolved_path_entry(&resolved_entries, path)) + .collect(); // Narrower explicit non-write entries carve out broader writable roots. // More specific write entries still remain writable because they appear // as separate WritableRoot values and are checked independently. @@ -1300,7 +1303,7 @@ fn normalize_effective_absolute_path(path: AbsolutePathBuf) -> AbsolutePathBuf { fn default_read_only_subpaths_for_writable_root( writable_root: &AbsolutePathBuf, - protect_missing_dot_codex: bool, + protect_missing_preserved_paths: bool, ) -> Vec { let mut subpaths: Vec = Vec::new(); let top_level_git = writable_root.join(".git"); @@ -1309,7 +1312,7 @@ fn default_read_only_subpaths_for_writable_root( // writable root itself. let top_level_git_is_file = top_level_git.as_path().is_file(); let top_level_git_is_dir = top_level_git.as_path().is_dir(); - if top_level_git_is_dir || top_level_git_is_file { + if top_level_git_is_dir || top_level_git_is_file || protect_missing_preserved_paths { if top_level_git_is_file && is_git_pointer_file(&top_level_git) && let Some(gitdir) = resolve_gitdir_from_file(&top_level_git) @@ -1320,16 +1323,16 @@ fn default_read_only_subpaths_for_writable_root( } let top_level_agents = writable_root.join(".agents"); - if top_level_agents.as_path().is_dir() { + if protect_missing_preserved_paths || top_level_agents.as_path().is_dir() { subpaths.push(top_level_agents); } - // Keep top-level project metadata under .codex read-only to the agent by + // Keep top-level preserved paths under .codex read-only to the agent by // default. For the workspace root itself, protect it even before the // directory exists so first-time creation still goes through the - // protected-path approval flow. + // preserved path approval flow. let top_level_codex = writable_root.join(".codex"); - if protect_missing_dot_codex || top_level_codex.as_path().is_dir() { + if protect_missing_preserved_paths || top_level_codex.as_path().is_dir() { subpaths.push(top_level_codex); } @@ -1540,12 +1543,14 @@ mod tests { #[cfg(unix)] #[test] - fn writable_roots_proactively_protect_missing_dot_codex() { + fn writable_roots_proactively_protect_missing_preserved_paths() { let cwd = TempDir::new().expect("tempdir"); let expected_root = AbsolutePathBuf::from_absolute_path( cwd.path().canonicalize().expect("canonicalize cwd"), ) .expect("absolute canonical root"); + let expected_dot_git = expected_root.join(".git"); + let expected_dot_agents = expected_root.join(".agents"); let expected_dot_codex = expected_root.join(".codex"); let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { @@ -1558,6 +1563,16 @@ mod tests { let writable_roots = policy.get_writable_roots_with_cwd(cwd.path()); assert_eq!(writable_roots.len(), 1); assert_eq!(writable_roots[0].root, expected_root); + assert!( + writable_roots[0] + .read_only_subpaths + .contains(&expected_dot_git) + ); + assert!( + writable_roots[0] + .read_only_subpaths + .contains(&expected_dot_agents) + ); assert!( writable_roots[0] .read_only_subpaths @@ -1613,12 +1628,14 @@ mod tests { #[cfg(unix)] #[test] - fn writable_roots_skip_default_dot_codex_when_explicit_user_rule_exists() { + fn writable_roots_skip_default_preserved_paths_when_explicit_user_rule_exists() { let cwd = TempDir::new().expect("tempdir"); let expected_root = AbsolutePathBuf::from_absolute_path( cwd.path().canonicalize().expect("canonicalize cwd"), ) .expect("absolute canonical root"); + let explicit_dot_git = expected_root.join(".git"); + let explicit_dot_agents = expected_root.join(".agents"); let explicit_dot_codex = expected_root.join(".codex"); let policy = FileSystemSandboxPolicy::restricted(vec![ @@ -1628,6 +1645,18 @@ mod tests { }, access: FileSystemAccessMode::Write, }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: explicit_dot_git.clone(), + }, + access: FileSystemAccessMode::Write, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: explicit_dot_agents.clone(), + }, + access: FileSystemAccessMode::Write, + }, FileSystemSandboxEntry { path: FileSystemPath::Path { path: explicit_dot_codex.clone(), @@ -1641,6 +1670,18 @@ mod tests { .iter() .find(|root| root.root == expected_root) .expect("workspace writable root"); + assert!( + !workspace_root + .read_only_subpaths + .contains(&explicit_dot_git), + "explicit .git rule should win over the default preserved path carveout" + ); + assert!( + !workspace_root + .read_only_subpaths + .contains(&explicit_dot_agents), + "explicit .agents rule should win over the default preserved path carveout" + ); assert!( !workspace_root .read_only_subpaths @@ -1656,8 +1697,9 @@ mod tests { } #[test] - fn legacy_workspace_write_projection_blocks_missing_dot_codex_writes() { + fn legacy_workspace_write_projection_blocks_missing_preserved_path_writes() { let cwd = TempDir::new().expect("tempdir"); + let dot_git_config = cwd.path().join(".git").join("config"); let dot_codex_config = cwd.path().join(".codex").join("config.toml"); let policy = SandboxPolicy::WorkspaceWrite { writable_roots: vec![], @@ -1669,12 +1711,27 @@ mod tests { let file_system_policy = FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd(&policy, cwd.path()); + assert!(!file_system_policy.can_write_path_with_cwd(&dot_git_config, cwd.path())); assert!(!file_system_policy.can_write_path_with_cwd(&dot_codex_config, cwd.path())); } #[test] fn legacy_workspace_write_projection_accepts_relative_cwd() { let relative_cwd = Path::new("workspace"); + let expected_dot_git = AbsolutePathBuf::from_absolute_path( + std::env::current_dir() + .expect("current dir") + .join(relative_cwd) + .join(".git"), + ) + .expect("absolute dot git"); + let expected_dot_agents = AbsolutePathBuf::from_absolute_path( + std::env::current_dir() + .expect("current dir") + .join(relative_cwd) + .join(".agents"), + ) + .expect("absolute dot agents"); let expected_dot_codex = AbsolutePathBuf::from_absolute_path( std::env::current_dir() .expect("current dir") @@ -1707,6 +1764,18 @@ mod tests { }, access: FileSystemAccessMode::Write, }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: expected_dot_git, + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: expected_dot_agents, + }, + access: FileSystemAccessMode::Read, + }, FileSystemSandboxEntry { path: FileSystemPath::Path { path: expected_dot_codex, @@ -1715,10 +1784,19 @@ mod tests { }, ]) ); + assert!( + !file_system_policy.can_write_path_with_cwd(Path::new(".git/config"), relative_cwd,) + ); assert!( !file_system_policy .can_write_path_with_cwd(Path::new(".codex/config.toml"), relative_cwd,) ); + assert!( + !file_system_policy.can_write_path_with_cwd( + Path::new(".agents/skills/example/SKILL.md"), + relative_cwd, + ) + ); } #[cfg(unix)] diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 1e84e1806c..1328e6cc1b 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -1253,13 +1253,13 @@ impl SandboxPolicy { roots .into_iter() .map(|writable_root| { - let protect_missing_dot_codex = cwd_root + let protect_missing_preserved_paths = cwd_root .as_ref() .is_some_and(|cwd_root| cwd_root == &writable_root); WritableRoot { read_only_subpaths: default_read_only_subpaths_for_writable_root( &writable_root, - protect_missing_dot_codex, + protect_missing_preserved_paths, ), root: writable_root, } @@ -1272,7 +1272,7 @@ impl SandboxPolicy { fn default_read_only_subpaths_for_writable_root( writable_root: &AbsolutePathBuf, - protect_missing_dot_codex: bool, + protect_missing_preserved_paths: bool, ) -> Vec { let mut subpaths: Vec = Vec::new(); let top_level_git = writable_root.join(".git"); @@ -1281,7 +1281,7 @@ fn default_read_only_subpaths_for_writable_root( // writable root itself. let top_level_git_is_file = top_level_git.as_path().is_file(); let top_level_git_is_dir = top_level_git.as_path().is_dir(); - if top_level_git_is_dir || top_level_git_is_file { + if top_level_git_is_dir || top_level_git_is_file || protect_missing_preserved_paths { if top_level_git_is_file && is_git_pointer_file(&top_level_git) && let Some(gitdir) = resolve_gitdir_from_file(&top_level_git) @@ -1292,16 +1292,16 @@ fn default_read_only_subpaths_for_writable_root( } let top_level_agents = writable_root.join(".agents"); - if top_level_agents.as_path().is_dir() { + if protect_missing_preserved_paths || top_level_agents.as_path().is_dir() { subpaths.push(top_level_agents); } - // Keep top-level project metadata under .codex read-only to the agent by + // Keep top-level preserved paths under .codex read-only to the agent by // default. For the workspace root itself, protect it even before the // directory exists so first-time creation still goes through the - // protected-path approval flow. + // preserved path approval flow. let top_level_codex = writable_root.join(".codex"); - if protect_missing_dot_codex || top_level_codex.as_path().is_dir() { + if protect_missing_preserved_paths || top_level_codex.as_path().is_dir() { subpaths.push(top_level_codex); } @@ -4375,7 +4375,6 @@ mod tests { #[test] fn restricted_file_system_policy_derives_effective_paths() { let cwd = TempDir::new().expect("tempdir"); - std::fs::create_dir_all(cwd.path().join(".agents")).expect("create .agents"); std::fs::create_dir_all(cwd.path().join(".codex")).expect("create .codex"); let canonical_cwd = codex_utils_absolute_path::canonicalize_preserving_symlinks(cwd.path()) .expect("canonicalize cwd"); @@ -4456,6 +4455,11 @@ mod tests { .expect("canonical docs/public"); let expected_dot_codex = AbsolutePathBuf::from_absolute_path(canonical_cwd.join(".codex")) .expect("canonical .codex"); + let expected_dot_git = AbsolutePathBuf::from_absolute_path(canonical_cwd.join(".git")) + .expect("canonical .git"); + let expected_dot_agents = + AbsolutePathBuf::from_absolute_path(canonical_cwd.join(".agents")) + .expect("canonical .agents"); let policy = FileSystemSandboxPolicy::restricted(vec![ FileSystemSandboxEntry { path: FileSystemPath::Special { @@ -4480,7 +4484,9 @@ mod tests { ( canonical_cwd, vec![ + expected_dot_agents.to_path_buf(), expected_dot_codex.to_path_buf(), + expected_dot_git.to_path_buf(), expected_docs.to_path_buf() ], ), @@ -4489,6 +4495,46 @@ mod tests { ); } + #[test] + fn legacy_workspace_write_nested_readable_root_stays_writable() { + let cwd = TempDir::new().expect("tempdir"); + let docs = AbsolutePathBuf::resolve_path_against_base("docs", cwd.path()); + let canonical_cwd = codex_utils_absolute_path::canonicalize_preserving_symlinks(cwd.path()) + .expect("canonicalize cwd"); + let expected_dot_codex = AbsolutePathBuf::from_absolute_path(canonical_cwd.join(".codex")) + .expect("canonical .codex"); + let expected_dot_git = AbsolutePathBuf::from_absolute_path(canonical_cwd.join(".git")) + .expect("canonical .git"); + let expected_dot_agents = + AbsolutePathBuf::from_absolute_path(canonical_cwd.join(".agents")) + .expect("canonical .agents"); + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![], + read_only_access: ReadOnlyAccess::Restricted { + include_platform_defaults: true, + readable_roots: vec![docs], + }, + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + + assert_eq!( + sorted_writable_roots( + FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd(&policy, cwd.path()) + .get_writable_roots_with_cwd(cwd.path()) + ), + vec![( + canonical_cwd, + vec![ + expected_dot_agents.to_path_buf(), + expected_dot_codex.to_path_buf(), + expected_dot_git.to_path_buf() + ] + )] + ); + } + #[test] fn file_system_policy_rejects_legacy_bridge_for_non_workspace_writes() { let cwd = if cfg!(windows) { diff --git a/codex-rs/sandboxing/src/seatbelt_tests.rs b/codex-rs/sandboxing/src/seatbelt_tests.rs index b691485746..b51cbcf81c 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::protocol::ReadOnlyAccess; use codex_protocol::protocol::SandboxPolicy; use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; @@ -200,8 +201,10 @@ fn explicit_unreadable_paths_are_excluded_from_full_disk_read_and_write_access() writable_definitions, vec![ "-DWRITABLE_ROOT_0=/".to_string(), - "-DWRITABLE_ROOT_0_EXCLUDED_0=/.codex".to_string(), - format!("-DWRITABLE_ROOT_0_EXCLUDED_1={}", unreadable_root.display()), + "-DWRITABLE_ROOT_0_EXCLUDED_0=/.git".to_string(), + "-DWRITABLE_ROOT_0_EXCLUDED_1=/.agents".to_string(), + "-DWRITABLE_ROOT_0_EXCLUDED_2=/.codex".to_string(), + format!("-DWRITABLE_ROOT_0_EXCLUDED_3={}", unreadable_root.display()), ], "unexpected write carveout parameters in args: {args:#?}" ); @@ -349,6 +352,49 @@ fn seatbelt_args_without_extension_profile_keep_legacy_preferences_read_access() assert!(!policy.contains("(allow user-preference-write)")); } +#[test] +fn seatbelt_legacy_workspace_write_nested_readable_root_stays_writable() { + let tmp = TempDir::new().expect("tempdir"); + let cwd = tmp.path().join("workspace"); + fs::create_dir_all(cwd.join("docs")).expect("create docs"); + let docs = AbsolutePathBuf::from_absolute_path(cwd.join("docs")).expect("absolute docs"); + let args = create_seatbelt_command_args_for_legacy_policy( + vec!["/bin/true".to_string()], + &SandboxPolicy::WorkspaceWrite { + writable_roots: vec![], + read_only_access: ReadOnlyAccess::Restricted { + include_platform_defaults: true, + readable_roots: vec![docs.clone()], + }, + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }, + cwd.as_path(), + /*enforce_managed_network*/ false, + /*network*/ None, + ); + + assert!( + !args + .iter() + .any(|arg| arg.ends_with(&format!("={}", docs.as_path().display()))), + "legacy workspace-write readable roots under cwd should not become seatbelt carveouts:\n{args:#?}", + ); + assert!( + args.iter() + .any(|arg| arg.starts_with("-DWRITABLE_ROOT_0_EXCLUDED_") + && arg.ends_with("/workspace/.agents")), + "expected proactive .agents carveout for cwd root: {args:#?}", + ); + assert!( + args.iter() + .any(|arg| arg.starts_with("-DWRITABLE_ROOT_0_EXCLUDED_") + && arg.ends_with("/workspace/.codex")), + "expected proactive .codex carveout for cwd root: {args:#?}", + ); +} + #[test] fn create_seatbelt_args_allows_local_binding_when_explicitly_enabled() { let policy = dynamic_network_policy( @@ -773,12 +819,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 preserved 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, @@ -828,12 +875,20 @@ fn create_seatbelt_args_with_read_only_git_and_codex_subpaths() { ); assert!( policy_text.contains("WRITABLE_ROOT_0_EXCLUDED_0"), + "expected cwd .git carveout in policy:\n{policy_text}", + ); + assert!( + policy_text.contains("WRITABLE_ROOT_0_EXCLUDED_1"), + "expected cwd .agents carveout in policy:\n{policy_text}", + ); + assert!( + policy_text.contains("WRITABLE_ROOT_0_EXCLUDED_2"), "expected cwd .codex carveout in policy:\n{policy_text}", ); assert!( policy_text.contains("WRITABLE_ROOT_1_EXCLUDED_0") && policy_text.contains("WRITABLE_ROOT_1_EXCLUDED_1"), - "expected explicit writable root .git/.codex carveouts in policy:\n{policy_text}", + "expected explicit writable root preserved path carveouts in policy:\n{policy_text}", ); assert!( policy_text.contains("(subpath (param \"WRITABLE_ROOT_2\"))"), @@ -849,6 +904,20 @@ fn create_seatbelt_args_with_read_only_git_and_codex_subpaths() { ), format!( "-DWRITABLE_ROOT_0_EXCLUDED_0={}", + cwd.canonicalize() + .expect("canonicalize cwd") + .join(".git") + .display() + ), + format!( + "-DWRITABLE_ROOT_0_EXCLUDED_1={}", + cwd.canonicalize() + .expect("canonicalize cwd") + .join(".agents") + .display() + ), + format!( + "-DWRITABLE_ROOT_0_EXCLUDED_2={}", cwd.canonicalize() .expect("canonicalize cwd") .join(".codex") @@ -1037,11 +1106,11 @@ 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\")))"), + policy_text.contains("(require-not (literal (param \"WRITABLE_ROOT_0_EXCLUDED_2\")))"), "expected exact .codex carveout in policy:\n{policy_text}" ); assert!( - policy_text.contains("(require-not (subpath (param \"WRITABLE_ROOT_0_EXCLUDED_1\")))"), + policy_text.contains("(require-not (subpath (param \"WRITABLE_ROOT_0_EXCLUDED_2\")))"), "expected descendant .codex carveout in policy:\n{policy_text}" ); } @@ -1146,19 +1215,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 preserved 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 preserved + // path checks are done properly for cwd. let policy = SandboxPolicy::WorkspaceWrite { writable_roots: vec![], network_access: false, @@ -1194,7 +1264,7 @@ fn create_seatbelt_args_for_cwd_as_git_repo() { .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"))) )"# + 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 { "" }; @@ -1203,13 +1273,13 @@ fn create_seatbelt_args_for_cwd_as_git_repo() { // Note that the policy includes: // - the base policy, // - read-only access to the filesystem, - // - write access to WRITABLE_ROOT_0 (but not its .git or .codex), WRITABLE_ROOT_1, and cwd as WRITABLE_ROOT_2. + // - write access to WRITABLE_ROOT_0 (but not its preserved paths), 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"))) ) (subpath (param "WRITABLE_ROOT_1")){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"))) ) (subpath (param "WRITABLE_ROOT_1")){tempdir_policy_entry} ) "#, @@ -1228,6 +1298,10 @@ fn create_seatbelt_args_for_cwd_as_git_repo() { ), format!( "-DWRITABLE_ROOT_0_EXCLUDED_1={}", + dot_agents_canonical.to_string_lossy() + ), + format!( + "-DWRITABLE_ROOT_0_EXCLUDED_2={}", dot_codex_canonical.to_string_lossy() ), format!( @@ -1247,6 +1321,10 @@ fn create_seatbelt_args_for_cwd_as_git_repo() { )); expected_args.push(format!( "-DWRITABLE_ROOT_2_EXCLUDED_1={}", + dot_agents_canonical.to_string_lossy() + )); + expected_args.push(format!( + "-DWRITABLE_ROOT_2_EXCLUDED_2={}", dot_codex_canonical.to_string_lossy() )); } @@ -1264,7 +1342,7 @@ fn create_seatbelt_args_for_cwd_as_git_repo() { } struct PopulatedTmp { - /// Path containing a .git and .codex subfolder. + /// Path containing preserved 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 @@ -1274,9 +1352,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 preserved subfolders. empty_root: PathBuf, /// Canonicalized version of `empty_root`. empty_root_canonical: PathBuf, @@ -1310,12 +1389,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,