mirror of
https://github.com/openai/codex.git
synced 2026-05-01 03:42:05 +03:00
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 <viyatb@openai.com>
This commit is contained in:
@@ -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>,
|
||||
|
||||
Reference in New Issue
Block a user