diff --git a/codex-rs/linux-sandbox/src/bwrap.rs b/codex-rs/linux-sandbox/src/bwrap.rs index 4e49e4e9dc..e1a680bcf4 100644 --- a/codex-rs/linux-sandbox/src/bwrap.rs +++ b/codex-rs/linux-sandbox/src/bwrap.rs @@ -26,6 +26,7 @@ use std::process::Command; use codex_protocol::error::CodexErr; use codex_protocol::error::Result; +use codex_protocol::permissions::is_preserved_directory_path_name; use codex_protocol::permissions::is_preserved_path_name; use codex_protocol::protocol::FileSystemSandboxPolicy; use codex_protocol::protocol::WritableRoot; @@ -122,44 +123,82 @@ impl FileIdentity { } } +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum SyntheticMountTargetKind { + EmptyFile, + EmptyDirectory, +} + #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) struct SyntheticMountTarget { path: PathBuf, + kind: SyntheticMountTargetKind, // 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, + // cleanup does not delete a real pre-existing file or directory. + pre_existing_path: Option, } impl SyntheticMountTarget { pub(crate) fn missing(path: &Path) -> Self { Self { path: path.to_path_buf(), - pre_existing_file: None, + kind: SyntheticMountTargetKind::EmptyFile, + pre_existing_path: None, + } + } + + pub(crate) fn missing_empty_directory(path: &Path) -> Self { + Self { + path: path.to_path_buf(), + kind: SyntheticMountTargetKind::EmptyDirectory, + pre_existing_path: None, } } pub(crate) fn existing_empty_file(path: &Path, metadata: &Metadata) -> Self { Self { path: path.to_path_buf(), - pre_existing_file: Some(FileIdentity::from_metadata(metadata)), + kind: SyntheticMountTargetKind::EmptyFile, + pre_existing_path: Some(FileIdentity::from_metadata(metadata)), } } - pub(crate) fn preserves_pre_existing_file(&self) -> bool { - self.pre_existing_file.is_some() + fn existing_empty_directory(path: &Path, metadata: &Metadata) -> Self { + Self { + path: path.to_path_buf(), + kind: SyntheticMountTargetKind::EmptyDirectory, + pre_existing_path: Some(FileIdentity::from_metadata(metadata)), + } + } + + pub(crate) fn preserves_pre_existing_path(&self) -> bool { + self.pre_existing_path.is_some() } pub(crate) fn path(&self) -> &Path { &self.path } + pub(crate) fn kind(&self) -> SyntheticMountTargetKind { + self.kind + } + pub(crate) fn should_remove_after_bwrap(&self, metadata: &Metadata) -> bool { - if !metadata.file_type().is_file() || metadata.len() != 0 { - return false; + match self.kind { + SyntheticMountTargetKind::EmptyFile => { + if !metadata.file_type().is_file() || metadata.len() != 0 { + return false; + } + } + SyntheticMountTargetKind::EmptyDirectory => { + if !metadata.file_type().is_dir() { + return false; + } + } } - match self.pre_existing_file { - Some(pre_existing_file) => pre_existing_file != FileIdentity::from_metadata(metadata), + match self.pre_existing_path { + Some(pre_existing_path) => pre_existing_path != FileIdentity::from_metadata(metadata), None => true, } } @@ -856,13 +895,20 @@ fn append_read_only_subpath_args( ))); } - if let Some(metadata) = transient_empty_preserved_file_metadata(subpath) + if let Some(metadata) = transient_empty_preserved_path_metadata(subpath) && is_within_allowed_write_paths(subpath, allowed_write_paths) { - // Another concurrent bwrap setup can leave a zero-byte mount target at + // Another concurrent bwrap setup can leave an empty 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(bwrap_args, subpath, &metadata)?; + match metadata { + EmptyPreservedPathMetadata::File(metadata) => { + append_existing_empty_file_bind_data_args(bwrap_args, subpath, &metadata)?; + } + EmptyPreservedPathMetadata::Directory(metadata) => { + append_existing_empty_directory_args(bwrap_args, subpath, &metadata); + } + } return Ok(()); } @@ -870,7 +916,7 @@ fn append_read_only_subpath_args( if let Some(first_missing_component) = find_first_non_existent_component(subpath) && is_within_allowed_write_paths(&first_missing_component, allowed_write_paths) { - append_missing_empty_file_bind_data_args(bwrap_args, &first_missing_component)?; + append_missing_read_only_subpath_args(bwrap_args, &first_missing_component)?; } return Ok(()); } @@ -894,6 +940,30 @@ fn append_empty_file_bind_data_args(bwrap_args: &mut BwrapArgs, path: &Path) -> Ok(()) } +fn append_empty_directory_args(bwrap_args: &mut BwrapArgs, path: &Path) { + bwrap_args.args.push("--perms".to_string()); + bwrap_args.args.push("555".to_string()); + bwrap_args.args.push("--tmpfs".to_string()); + bwrap_args.args.push(path_to_string(path)); + bwrap_args.args.push("--remount-ro".to_string()); + bwrap_args.args.push(path_to_string(path)); +} + +fn append_missing_read_only_subpath_args(bwrap_args: &mut BwrapArgs, path: &Path) -> Result<()> { + if path + .file_name() + .is_some_and(is_preserved_directory_path_name) + { + append_empty_directory_args(bwrap_args, path); + bwrap_args + .synthetic_mount_targets + .push(SyntheticMountTarget::missing_empty_directory(path)); + return Ok(()); + } + + append_missing_empty_file_bind_data_args(bwrap_args, path) +} + fn append_missing_empty_file_bind_data_args(bwrap_args: &mut BwrapArgs, path: &Path) -> Result<()> { append_empty_file_bind_data_args(bwrap_args, path)?; bwrap_args @@ -914,6 +984,19 @@ fn append_existing_empty_file_bind_data_args( Ok(()) } +fn append_existing_empty_directory_args( + bwrap_args: &mut BwrapArgs, + path: &Path, + metadata: &Metadata, +) { + append_empty_directory_args(bwrap_args, path); + bwrap_args + .synthetic_mount_targets + .push(SyntheticMountTarget::existing_empty_directory( + path, metadata, + )); +} + fn append_unreadable_root_args( bwrap_args: &mut BwrapArgs, unreadable_root: &Path, @@ -999,17 +1082,33 @@ 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 { +enum EmptyPreservedPathMetadata { + File(Metadata), + Directory(Metadata), +} + +fn transient_empty_preserved_path_metadata(path: &Path) -> Option { if !path.file_name().is_some_and(is_preserved_path_name) { return None; } let metadata = fs::symlink_metadata(path).ok()?; if metadata.file_type().is_file() && metadata.len() == 0 { - Some(metadata) - } else { - None + return Some(EmptyPreservedPathMetadata::File(metadata)); } + + if metadata.file_type().is_dir() && directory_is_empty(path) { + return Some(EmptyPreservedPathMetadata::Directory(metadata)); + } + + None +} + +fn directory_is_empty(path: &Path) -> bool { + let Ok(mut entries) = fs::read_dir(path) else { + return false; + }; + entries.next().is_none() } fn first_writable_symlink_component_in_path( @@ -1479,6 +1578,8 @@ mod tests { .expect("filesystem args"); assert_empty_file_bound_without_perms(&args.args, &blocked); + assert_empty_directory_mounted_read_only(&args.args, &workspace.join(".agents")); + assert_empty_directory_mounted_read_only(&args.args, &workspace.join(".codex")); assert_eq!(args.preserved_files.len(), 1); assert_eq!( synthetic_mount_target_paths(&args), @@ -1518,6 +1619,8 @@ mod tests { let dot_git_str = path_to_string(&dot_git); assert_empty_file_bound_without_perms(&args.args, &dot_git); + assert_empty_directory_mounted_read_only(&args.args, &workspace.join(".agents")); + assert_empty_directory_mounted_read_only(&args.args, &workspace.join(".codex")); assert_eq!( synthetic_mount_target_paths(&args), vec![ @@ -1656,11 +1759,17 @@ mod tests { "--ro-bind-data".to_string(), null_fd.clone(), "/.git".to_string(), - "--ro-bind-data".to_string(), - null_fd.clone(), + "--perms".to_string(), + "555".to_string(), + "--tmpfs".to_string(), "/.agents".to_string(), - "--ro-bind-data".to_string(), - null_fd.clone(), + "--remount-ro".to_string(), + "/.agents".to_string(), + "--perms".to_string(), + "555".to_string(), + "--tmpfs".to_string(), + "/.codex".to_string(), + "--remount-ro".to_string(), "/.codex".to_string(), // Rebind /dev after the root bind so device nodes remain // writable/usable inside the writable root. @@ -1668,13 +1777,19 @@ mod tests { "/dev".to_string(), "/dev".to_string(), "--ro-bind-data".to_string(), - null_fd.clone(), - "/dev/.git".to_string(), - "--ro-bind-data".to_string(), - null_fd.clone(), - "/dev/.agents".to_string(), - "--ro-bind-data".to_string(), null_fd, + "/dev/.git".to_string(), + "--perms".to_string(), + "555".to_string(), + "--tmpfs".to_string(), + "/dev/.agents".to_string(), + "--remount-ro".to_string(), + "/dev/.agents".to_string(), + "--perms".to_string(), + "555".to_string(), + "--tmpfs".to_string(), + "/dev/.codex".to_string(), + "--remount-ro".to_string(), "/dev/.codex".to_string(), ] ); @@ -2264,6 +2379,20 @@ mod tests { ); } + fn assert_empty_directory_mounted_read_only(args: &[String], path: &Path) { + let path = path_to_string(path); + assert!( + args.windows(4) + .any(|window| window == ["--perms", "555", "--tmpfs", path.as_str()]), + "expected empty directory mount for {path}: {args:#?}" + ); + assert!( + args.windows(2) + .any(|window| window == ["--remount-ro", path.as_str()]), + "expected read-only remount for {path}: {args:#?}" + ); + } + fn synthetic_mount_target_paths(args: &BwrapArgs) -> Vec { args.synthetic_mount_targets .iter() diff --git a/codex-rs/linux-sandbox/src/linux_run_main.rs b/codex-rs/linux-sandbox/src/linux_run_main.rs index 9c4826b818..23245db44f 100644 --- a/codex-rs/linux-sandbox/src/linux_run_main.rs +++ b/codex-rs/linux-sandbox/src/linux_run_main.rs @@ -694,10 +694,19 @@ fn register_synthetic_mount_targets( marker_dir.display() ) }); - let target = if target.preserves_pre_existing_file() + let target = if target.preserves_pre_existing_path() && synthetic_mount_marker_dir_has_active_synthetic_owner(&marker_dir) { - crate::bwrap::SyntheticMountTarget::missing(target.path()) + match target.kind() { + crate::bwrap::SyntheticMountTargetKind::EmptyFile => { + crate::bwrap::SyntheticMountTarget::missing(target.path()) + } + crate::bwrap::SyntheticMountTargetKind::EmptyDirectory => { + crate::bwrap::SyntheticMountTarget::missing_empty_directory( + target.path(), + ) + } + } } else { target.clone() }; @@ -721,7 +730,7 @@ fn register_synthetic_mount_targets( } fn synthetic_mount_marker_contents(target: &crate::bwrap::SyntheticMountTarget) -> &'static [u8] { - if target.preserves_pre_existing_file() { + if target.preserves_pre_existing_path() { SYNTHETIC_MOUNT_MARKER_EXISTING } else { SYNTHETIC_MOUNT_MARKER_SYNTHETIC @@ -835,13 +844,24 @@ fn remove_synthetic_mount_target(target: &crate::bwrap::SyntheticMountTarget) { if !target.should_remove_after_bwrap(&metadata) { return; } - 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() - ), + match target.kind() { + crate::bwrap::SyntheticMountTargetKind::EmptyFile => 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() + ), + }, + crate::bwrap::SyntheticMountTargetKind::EmptyDirectory => match fs::remove_dir(path) { + 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 synthetic bubblewrap mount target {}: {err}", + path.display() + ), + }, } } 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 a436983d5a..dbeb3d0dce 100644 --- a/codex-rs/linux-sandbox/src/linux_run_main_tests.rs +++ b/codex-rs/linux-sandbox/src/linux_run_main_tests.rs @@ -260,22 +260,26 @@ fn managed_proxy_preflight_argv_is_wrapped_for_full_access_policy() { } #[test] -fn cleanup_synthetic_mount_targets_removes_only_empty_files() { +fn cleanup_synthetic_mount_targets_removes_only_empty_mount_targets() { 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 empty_file = temp_dir.path().join(".git"); + let empty_dir = temp_dir.path().join(".agents"); + let non_empty_file = temp_dir.path().join("non-empty"); let missing_file = temp_dir.path().join(".missing"); std::fs::write(&empty_file, "").expect("write empty file"); + std::fs::create_dir(&empty_dir).expect("create empty dir"); std::fs::write(&non_empty_file, "keep").expect("write nonempty file"); let registrations = register_synthetic_mount_targets(&[ crate::bwrap::SyntheticMountTarget::missing(&empty_file), + crate::bwrap::SyntheticMountTarget::missing_empty_directory(&empty_dir), crate::bwrap::SyntheticMountTarget::missing(&non_empty_file), crate::bwrap::SyntheticMountTarget::missing(&missing_file), ]); cleanup_synthetic_mount_targets(®istrations); assert!(!empty_file.exists()); + assert!(!empty_dir.exists()); assert_eq!( std::fs::read_to_string(&non_empty_file).expect("read nonempty file"), "keep" diff --git a/codex-rs/protocol/src/permissions.rs b/codex-rs/protocol/src/permissions.rs index 295f801728..607c89c728 100644 --- a/codex-rs/protocol/src/permissions.rs +++ b/codex-rs/protocol/src/permissions.rs @@ -37,6 +37,10 @@ pub fn is_preserved_path_name(name: &OsStr) -> bool { .any(|preserved| name == OsStr::new(preserved)) } +pub fn is_preserved_directory_path_name(name: &OsStr) -> bool { + name == OsStr::new(PRESERVED_AGENTS_PATH_NAME) || name == OsStr::new(PRESERVED_CODEX_PATH_NAME) +} + /// Returns the preserved workspace metadata name when an agent write to `path` /// should be blocked before execution. pub fn forbidden_agent_preserved_path_write(