From 3b5fe5ca35d914645a818d454a3931f6748b7e77 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Sat, 7 Mar 2026 21:15:47 -0800 Subject: [PATCH] protocol: keep root carveouts sandboxed (#13452) ## Why A restricted filesystem policy that grants `:root` read or write access but also carries explicit deny entries should still behave like scoped access with carveouts, not like unrestricted disk access. Without that distinction, later platform backends cannot preserve blocked subpaths under root-level permissions because the protocol layer reports the policy as fully unrestricted. ## What changed - taught `FileSystemSandboxPolicy` to treat root access plus explicit deny entries as scoped access rather than full-disk access - derived readable and writable roots from the filesystem root when root access is combined with carveouts, while preserving the denied paths as read-only subpaths - added protocol coverage for root-write policies with carveouts and a core sandboxing regression so those policies still require platform sandboxing ## Verification - added protocol coverage in `protocol/src/permissions.rs` and `protocol/src/protocol.rs` for root access with explicit carveouts - added platform-sandbox regression coverage in `core/src/sandboxing/mod.rs` - verified the current PR state with `just clippy` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/13452). * #13453 * __->__ #13452 * #13451 * #13449 * #13448 * #13445 * #13440 * #13439 --------- Co-authored-by: viyatb-oai --- codex-rs/core/src/sandboxing/mod.rs | 26 ++++++++ codex-rs/protocol/src/permissions.rs | 94 +++++++++++++++++++++------- codex-rs/protocol/src/protocol.rs | 50 +++++++++++++++ 3 files changed, 147 insertions(+), 23 deletions(-) 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");