diff --git a/codex-rs/core/src/sandboxing/mod.rs b/codex-rs/core/src/sandboxing/mod.rs index ecbdbed93..c43322ca7 100644 --- a/codex-rs/core/src/sandboxing/mod.rs +++ b/codex-rs/core/src/sandboxing/mod.rs @@ -673,6 +673,32 @@ mod tests { ); } + #[test] + fn root_write_policy_with_carveouts_still_uses_platform_sandbox() { + let blocked = AbsolutePathBuf::resolve_path_against_base( + "blocked", + std::env::current_dir().expect("current dir"), + ) + .expect("blocked path"); + let policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Write, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: blocked }, + access: FileSystemAccessMode::None, + }, + ]); + + assert_eq!( + should_require_platform_sandbox(&policy, NetworkSandboxPolicy::Enabled, false), + true + ); + } + #[test] fn full_access_restricted_policy_still_uses_platform_sandbox_for_restricted_network() { let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { diff --git a/codex-rs/protocol/src/permissions.rs b/codex-rs/protocol/src/permissions.rs index c5aabea6b..4e2f2fbad 100644 --- a/codex-rs/protocol/src/permissions.rs +++ b/codex-rs/protocol/src/permissions.rs @@ -123,6 +123,25 @@ impl Default for FileSystemSandboxPolicy { } impl FileSystemSandboxPolicy { + fn has_root_access(&self, predicate: impl Fn(FileSystemAccessMode) -> bool) -> bool { + matches!(self.kind, FileSystemSandboxKind::Restricted) + && self.entries.iter().any(|entry| { + matches!( + &entry.path, + FileSystemPath::Special { value } + if matches!(value, FileSystemSpecialPath::Root) && predicate(entry.access) + ) + }) + } + + fn has_explicit_deny_entries(&self) -> bool { + matches!(self.kind, FileSystemSandboxKind::Restricted) + && self + .entries + .iter() + .any(|entry| entry.access == FileSystemAccessMode::None) + } + pub fn unrestricted() -> Self { Self { kind: FileSystemSandboxKind::Unrestricted, @@ -148,13 +167,10 @@ impl FileSystemSandboxPolicy { pub fn has_full_disk_read_access(&self) -> bool { match self.kind { FileSystemSandboxKind::Unrestricted | FileSystemSandboxKind::ExternalSandbox => true, - FileSystemSandboxKind::Restricted => self.entries.iter().any(|entry| { - matches!( - &entry.path, - FileSystemPath::Special { value } - if matches!(value, FileSystemSpecialPath::Root) && entry.access.can_read() - ) - }), + FileSystemSandboxKind::Restricted => { + self.has_root_access(FileSystemAccessMode::can_read) + && !self.has_explicit_deny_entries() + } } } @@ -162,14 +178,10 @@ impl FileSystemSandboxPolicy { pub fn has_full_disk_write_access(&self) -> bool { match self.kind { FileSystemSandboxKind::Unrestricted | FileSystemSandboxKind::ExternalSandbox => true, - FileSystemSandboxKind::Restricted => self.entries.iter().any(|entry| { - matches!( - &entry.path, - FileSystemPath::Special { value } - if matches!(value, FileSystemSpecialPath::Root) - && entry.access.can_write() - ) - }), + FileSystemSandboxKind::Restricted => { + self.has_root_access(FileSystemAccessMode::can_write) + && !self.has_explicit_deny_entries() + } } } @@ -194,11 +206,24 @@ impl FileSystemSandboxPolicy { } let cwd_absolute = AbsolutePathBuf::from_absolute_path(cwd).ok(); + let mut readable_roots = Vec::new(); + if self.has_root_access(FileSystemAccessMode::can_read) + && let Some(cwd_absolute) = cwd_absolute.as_ref() + { + readable_roots.push(absolute_root_path_for_cwd(cwd_absolute)); + } + dedup_absolute_paths( - self.entries - .iter() - .filter(|entry| entry.access.can_read()) - .filter_map(|entry| resolve_file_system_path(&entry.path, cwd_absolute.as_ref())) + readable_roots + .into_iter() + .chain( + self.entries + .iter() + .filter(|entry| entry.access.can_read()) + .filter_map(|entry| { + resolve_file_system_path(&entry.path, cwd_absolute.as_ref()) + }), + ) .collect(), ) } @@ -212,11 +237,24 @@ impl FileSystemSandboxPolicy { let cwd_absolute = AbsolutePathBuf::from_absolute_path(cwd).ok(); let unreadable_roots = self.get_unreadable_roots_with_cwd(cwd); + let mut writable_roots = Vec::new(); + if self.has_root_access(FileSystemAccessMode::can_write) + && let Some(cwd_absolute) = cwd_absolute.as_ref() + { + writable_roots.push(absolute_root_path_for_cwd(cwd_absolute)); + } + dedup_absolute_paths( - self.entries - .iter() - .filter(|entry| entry.access.can_write()) - .filter_map(|entry| resolve_file_system_path(&entry.path, cwd_absolute.as_ref())) + writable_roots + .into_iter() + .chain( + self.entries + .iter() + .filter(|entry| entry.access.can_write()) + .filter_map(|entry| { + resolve_file_system_path(&entry.path, cwd_absolute.as_ref()) + }), + ) .collect(), ) .into_iter() @@ -543,6 +581,16 @@ fn resolve_file_system_path( } } +fn absolute_root_path_for_cwd(cwd: &AbsolutePathBuf) -> AbsolutePathBuf { + let root = cwd + .as_path() + .ancestors() + .last() + .unwrap_or_else(|| panic!("cwd must have a filesystem root")); + AbsolutePathBuf::from_absolute_path(root) + .unwrap_or_else(|err| panic!("cwd root must be an absolute path: {err}")) +} + fn resolve_file_system_special_path( value: &FileSystemSpecialPath, cwd: Option<&AbsolutePathBuf>, diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index de899ab65..f51e3e6b7 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -3352,6 +3352,56 @@ mod tests { assert!(writable.has_full_disk_write_access()); } + #[test] + fn restricted_file_system_policy_treats_root_with_carveouts_as_scoped_access() { + let cwd = TempDir::new().expect("tempdir"); + let cwd_absolute = + AbsolutePathBuf::from_absolute_path(cwd.path()).expect("absolute tempdir"); + let root = cwd_absolute + .as_path() + .ancestors() + .last() + .and_then(|path| AbsolutePathBuf::from_absolute_path(path).ok()) + .expect("filesystem root"); + let blocked = AbsolutePathBuf::resolve_path_against_base("blocked", cwd.path()) + .expect("resolve blocked"); + let policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Write, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: blocked.clone(), + }, + access: FileSystemAccessMode::None, + }, + ]); + + assert!(!policy.has_full_disk_read_access()); + assert!(!policy.has_full_disk_write_access()); + assert_eq!( + policy.get_readable_roots_with_cwd(cwd.path()), + vec![root.clone()] + ); + assert_eq!( + policy.get_unreadable_roots_with_cwd(cwd.path()), + vec![blocked.clone()] + ); + + let writable_roots = policy.get_writable_roots_with_cwd(cwd.path()); + assert_eq!(writable_roots.len(), 1); + assert_eq!(writable_roots[0].root, root); + assert!( + writable_roots[0] + .read_only_subpaths + .iter() + .any(|path| path.as_path() == blocked.as_path()) + ); + } + #[test] fn restricted_file_system_policy_derives_effective_paths() { let cwd = TempDir::new().expect("tempdir");