diff --git a/codex-rs/linux-sandbox/src/bwrap.rs b/codex-rs/linux-sandbox/src/bwrap.rs index 91d6621029..671e603f34 100644 --- a/codex-rs/linux-sandbox/src/bwrap.rs +++ b/codex-rs/linux-sandbox/src/bwrap.rs @@ -221,12 +221,14 @@ fn create_filesystem_args( .collect::>(); let unreadable_roots = file_system_sandbox_policy.get_unreadable_roots_with_cwd(cwd); + let mut logical_readable_roots = Vec::new(); let mut args = if file_system_sandbox_policy.has_full_disk_read_access() { // Read-only root, then mount a minimal device tree. // In bubblewrap (`bubblewrap.c`, `SETUP_MOUNT_DEV`), `--dev /dev` // creates the standard minimal nodes: null, zero, full, random, // urandom, and tty. `/dev` must be mounted before writable roots so // explicit `/dev/*` writable binds remain visible. + logical_readable_roots.push(PathBuf::from("/")); vec![ "--ro-bind".to_string(), "/".to_string(), @@ -262,6 +264,7 @@ fn create_filesystem_args( // the broad read baseline. Explicit unreadable carveouts are // re-applied later. if readable_roots.iter().any(|root| root == Path::new("/")) { + logical_readable_roots.push(PathBuf::from("/")); args = vec![ "--ro-bind".to_string(), "/".to_string(), @@ -278,10 +281,13 @@ fn create_filesystem_args( // for their restricted-read bootstrap mount. Plain read-only // roots must stay logical because callers may execute those // paths inside bwrap, such as Bazel runfiles helper binaries. - let mount_root = if writable_roots + let read_root_bootstraps_writable_root = writable_roots .iter() - .any(|writable_root| root.starts_with(writable_root.root.as_path())) - { + .any(|writable_root| root.starts_with(writable_root.root.as_path())); + if !read_root_bootstraps_writable_root { + logical_readable_roots.push(root.clone()); + } + let mount_root = if read_root_bootstraps_writable_root { canonical_target_if_symlinked_path(&root).unwrap_or(root) } else { root @@ -354,6 +360,7 @@ fn create_filesystem_args( args.push("--bind".to_string()); args.push(path_to_string(mount_root)); args.push(path_to_string(mount_root)); + append_logical_symlink_alias_args(&mut args, root, &logical_readable_roots); let mut read_only_subpaths: Vec = writable_root .read_only_subpaths @@ -423,6 +430,16 @@ fn path_depth(path: &Path) -> usize { } fn canonical_target_if_symlinked_path(path: &Path) -> Option { + let _ = first_symlink_alias_in_path(path)?; + let target = fs::canonicalize(path).ok()?; + if target.as_path() == path { + None + } else { + Some(target) + } +} + +fn first_symlink_alias_in_path(path: &Path) -> Option<(PathBuf, PathBuf)> { let mut current = PathBuf::new(); for component in path.components() { use std::path::Component; @@ -445,11 +462,11 @@ fn canonical_target_if_symlinked_path(path: &Path) -> Option { Err(_) => return None, }; if metadata.file_type().is_symlink() { - let target = fs::canonicalize(path).ok()?; - if target.as_path() == path { + let target = fs::canonicalize(¤t).ok()?; + if target.as_path() == current { return None; } - return Some(target); + return Some((current, target)); } } None @@ -474,6 +491,43 @@ fn normalize_command_cwd_for_bwrap(command_cwd: &Path) -> PathBuf { .unwrap_or_else(|_| command_cwd.to_path_buf()) } +fn append_logical_symlink_alias_args( + args: &mut Vec, + writable_root: &Path, + logical_readable_roots: &[PathBuf], +) { + let Some((alias_path, target)) = first_symlink_alias_in_path(writable_root) else { + return; + }; + if logical_readable_roots + .iter() + .any(|root| alias_path.starts_with(root)) + { + return; + } + + append_parent_dir_args_for_path(args, &alias_path, Path::new("/")); + args.push("--symlink".to_string()); + args.push(path_to_string(&target)); + args.push(path_to_string(&alias_path)); +} + +fn append_parent_dir_args_for_path(args: &mut Vec, path: &Path, anchor: &Path) { + let Some(parent) = path.parent() else { + return; + }; + let mut parent_dirs: Vec = parent + .ancestors() + .take_while(|path| *path != anchor) + .map(Path::to_path_buf) + .collect(); + parent_dirs.reverse(); + for parent_dir in parent_dirs { + args.push("--dir".to_string()); + args.push(path_to_string(&parent_dir)); + } +} + fn append_mount_target_parent_dir_args(args: &mut Vec, mount_target: &Path, anchor: &Path) { let mount_target_dir = if mount_target.is_dir() { mount_target @@ -922,6 +976,102 @@ mod tests { })); } + #[cfg(unix)] + #[test] + fn symlinked_writable_roots_recreate_logical_alias_when_parent_is_not_readable() { + let temp_dir = TempDir::new().expect("temp dir"); + let temp_root = temp_dir.path().canonicalize().expect("canonical temp dir"); + let real_root = temp_root.join("real"); + let link_root = temp_root.join("link"); + std::fs::create_dir_all(&real_root).expect("create real root"); + std::os::unix::fs::symlink(&real_root, &link_root).expect("create symlinked root"); + + let link_root = + AbsolutePathBuf::from_absolute_path(&link_root).expect("absolute symlinked root"); + let real_root_str = path_to_string(&real_root); + let link_root_str = path_to_string(link_root.as_path()); + let policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Minimal, + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: link_root }, + access: FileSystemAccessMode::Write, + }, + ]); + + let args = create_filesystem_args(&policy, temp_dir.path()).expect("filesystem args"); + let bind_index = args + .args + .windows(3) + .position(|window| window == ["--bind", real_root_str.as_str(), real_root_str.as_str()]) + .expect("writable root should bind real target"); + let symlink_index = args + .args + .windows(3) + .position(|window| { + window == ["--symlink", real_root_str.as_str(), link_root_str.as_str()] + }) + .expect("logical symlink alias should be recreated"); + + assert!( + bind_index < symlink_index, + "logical alias must point at the mounted writable target: {:#?}", + args.args + ); + } + + #[cfg(unix)] + #[test] + fn symlinked_writable_roots_skip_logical_alias_when_parent_is_readable() { + let temp_dir = TempDir::new().expect("temp dir"); + let temp_root = temp_dir.path().canonicalize().expect("canonical temp dir"); + let parent = temp_root.join("parent"); + let real_root = temp_root.join("real"); + let link_root = parent.join("link"); + std::fs::create_dir_all(&parent).expect("create parent"); + std::fs::create_dir_all(&real_root).expect("create real root"); + std::os::unix::fs::symlink(&real_root, &link_root).expect("create symlinked root"); + + let parent = AbsolutePathBuf::from_absolute_path(&parent).expect("absolute parent"); + let link_root = + AbsolutePathBuf::from_absolute_path(&link_root).expect("absolute symlinked root"); + let parent_str = path_to_string(parent.as_path()); + let real_root_str = path_to_string(&real_root); + let link_root_str = path_to_string(link_root.as_path()); + let policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: parent }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: link_root }, + access: FileSystemAccessMode::Write, + }, + ]); + + let args = create_filesystem_args(&policy, temp_dir.path()).expect("filesystem args"); + + assert!( + args.args.windows(3).any(|window| { + window == ["--ro-bind", parent_str.as_str(), parent_str.as_str()] + }) + ); + assert!(args.args.windows(3).any(|window| { + window == ["--bind", real_root_str.as_str(), real_root_str.as_str()] + })); + assert!( + !args.args.windows(3).any(|window| { + window == ["--symlink", real_root_str.as_str(), link_root_str.as_str()] + }), + "readable parent already exposes the logical symlink: {:#?}", + args.args + ); + } + #[cfg(unix)] #[test] fn protected_symlinked_directory_subpaths_bind_target_read_only() {