From dc46620ef4da513081c312bc7bc019196cfbc104 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Mon, 9 Mar 2026 21:27:49 -0700 Subject: [PATCH] refactor: centralize filesystem permissions precedence --- codex-rs/protocol/src/permissions.rs | 304 +++++++++++++++++++++++---- 1 file changed, 265 insertions(+), 39 deletions(-) diff --git a/codex-rs/protocol/src/permissions.rs b/codex-rs/protocol/src/permissions.rs index 9624f36cd6..66059e675f 100644 --- a/codex-rs/protocol/src/permissions.rs +++ b/codex-rs/protocol/src/permissions.rs @@ -121,6 +121,22 @@ pub struct FileSystemSandboxPolicy { pub entries: Vec, } +#[derive(Debug, Clone, PartialEq, Eq)] +struct ResolvedFileSystemEntry { + path: AbsolutePathBuf, + access: FileSystemAccessMode, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +struct FileSystemSemanticSignature { + has_full_disk_read_access: bool, + has_full_disk_write_access: bool, + include_platform_defaults: bool, + readable_roots: Vec, + writable_roots: Vec, + unreadable_roots: Vec, +} + #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema, TS)] #[serde(tag = "type", rename_all = "snake_case")] #[ts(tag = "type")] @@ -163,6 +179,25 @@ impl FileSystemSandboxPolicy { .any(|entry| entry.access == FileSystemAccessMode::None) } + fn has_write_narrowing_entries(&self) -> bool { + matches!(self.kind, FileSystemSandboxKind::Restricted) + && self.entries.iter().any(|entry| { + if entry.access.can_write() { + return false; + } + + match &entry.path { + FileSystemPath::Path { .. } => true, + FileSystemPath::Special { value } => !matches!( + value, + FileSystemSpecialPath::Root + | FileSystemSpecialPath::Minimal + | FileSystemSpecialPath::Unknown { .. } + ), + } + }) + } + pub fn unrestricted() -> Self { Self { kind: FileSystemSandboxKind::Unrestricted, @@ -229,7 +264,7 @@ impl FileSystemSandboxPolicy { FileSystemSandboxKind::Unrestricted | FileSystemSandboxKind::ExternalSandbox => true, FileSystemSandboxKind::Restricted => { self.has_root_access(FileSystemAccessMode::can_write) - && !self.has_explicit_deny_entries() + && !self.has_write_narrowing_entries() } } } @@ -248,31 +283,64 @@ impl FileSystemSandboxPolicy { }) } + pub fn resolve_access_with_cwd(&self, path: &Path, cwd: &Path) -> FileSystemAccessMode { + match self.kind { + FileSystemSandboxKind::Unrestricted | FileSystemSandboxKind::ExternalSandbox => { + return FileSystemAccessMode::Write; + } + FileSystemSandboxKind::Restricted => {} + } + + let Some(path) = resolve_candidate_path(path, cwd) else { + return FileSystemAccessMode::None; + }; + + self.resolved_entries_with_cwd(cwd) + .into_iter() + .filter(|entry| path.as_path().starts_with(entry.path.as_path())) + .max_by_key(resolved_entry_precedence) + .map(|entry| entry.access) + .unwrap_or(FileSystemAccessMode::None) + } + + pub fn can_read_path_with_cwd(&self, path: &Path, cwd: &Path) -> bool { + self.resolve_access_with_cwd(path, cwd).can_read() + } + + pub fn can_write_path_with_cwd(&self, path: &Path, cwd: &Path) -> bool { + self.resolve_access_with_cwd(path, cwd).can_write() + } + + pub fn needs_direct_runtime_enforcement( + &self, + network_policy: NetworkSandboxPolicy, + cwd: &Path, + ) -> bool { + if !matches!(self.kind, FileSystemSandboxKind::Restricted) { + return false; + } + + let Ok(legacy_policy) = self.to_legacy_sandbox_policy(network_policy, cwd) else { + return true; + }; + + self.semantic_signature(cwd) + != FileSystemSandboxPolicy::from_legacy_sandbox_policy(&legacy_policy, cwd) + .semantic_signature(cwd) + } + /// Returns the explicit readable roots resolved against the provided cwd. pub fn get_readable_roots_with_cwd(&self, cwd: &Path) -> Vec { if self.has_full_disk_read_access() { return Vec::new(); } - 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( - readable_roots + self.resolved_entries_with_cwd(cwd) .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()) - }), - ) + .filter(|entry| entry.access.can_read()) + .filter(|entry| self.can_read_path_with_cwd(entry.path.as_path(), cwd)) + .map(|entry| entry.path) .collect(), ) } @@ -284,32 +352,22 @@ impl FileSystemSandboxPolicy { return Vec::new(); } - let cwd_absolute = AbsolutePathBuf::from_absolute_path(cwd).ok(); + let resolved_entries = self.resolved_entries_with_cwd(cwd); let read_only_roots = dedup_absolute_paths( - self.entries + resolved_entries .iter() .filter(|entry| !entry.access.can_write()) - .filter_map(|entry| resolve_file_system_path(&entry.path, cwd_absolute.as_ref())) + .filter(|entry| !self.can_write_path_with_cwd(entry.path.as_path(), cwd)) + .map(|entry| entry.path.clone()) .collect(), ); - 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( - writable_roots + resolved_entries .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()) - }), - ) + .filter(|entry| entry.access.can_write()) + .filter(|entry| self.can_write_path_with_cwd(entry.path.as_path(), cwd)) + .map(|entry| entry.path) .collect(), ) .into_iter() @@ -339,12 +397,12 @@ impl FileSystemSandboxPolicy { return Vec::new(); } - let cwd_absolute = AbsolutePathBuf::from_absolute_path(cwd).ok(); dedup_absolute_paths( - self.entries + self.resolved_entries_with_cwd(cwd) .iter() .filter(|entry| entry.access == FileSystemAccessMode::None) - .filter_map(|entry| resolve_file_system_path(&entry.path, cwd_absolute.as_ref())) + .filter(|entry| !self.can_read_path_with_cwd(entry.path.as_path(), cwd)) + .map(|entry| entry.path.clone()) .collect(), ) } @@ -504,6 +562,32 @@ impl FileSystemSandboxPolicy { } }) } + + fn resolved_entries_with_cwd(&self, cwd: &Path) -> Vec { + let cwd_absolute = AbsolutePathBuf::from_absolute_path(cwd).ok(); + self.entries + .iter() + .filter_map(|entry| { + resolve_entry_path(&entry.path, cwd_absolute.as_ref()).map(|path| { + ResolvedFileSystemEntry { + path, + access: entry.access, + } + }) + }) + .collect() + } + + fn semantic_signature(&self, cwd: &Path) -> FileSystemSemanticSignature { + FileSystemSemanticSignature { + has_full_disk_read_access: self.has_full_disk_read_access(), + has_full_disk_write_access: self.has_full_disk_write_access(), + include_platform_defaults: self.include_platform_defaults(), + readable_roots: self.get_readable_roots_with_cwd(cwd), + writable_roots: self.get_writable_roots_with_cwd(cwd), + unreadable_roots: self.get_unreadable_roots_with_cwd(cwd), + } + } } impl From<&SandboxPolicy> for NetworkSandboxPolicy { @@ -641,6 +725,36 @@ fn resolve_file_system_path( } } +fn resolve_entry_path( + path: &FileSystemPath, + cwd: Option<&AbsolutePathBuf>, +) -> Option { + match path { + FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + } => cwd.map(absolute_root_path_for_cwd), + _ => resolve_file_system_path(path, cwd), + } +} + +fn resolve_candidate_path(path: &Path, cwd: &Path) -> Option { + if path.is_absolute() { + AbsolutePathBuf::from_absolute_path(path).ok() + } else { + AbsolutePathBuf::resolve_path_against_base(path, cwd).ok() + } +} + +fn resolved_entry_precedence(entry: &ResolvedFileSystemEntry) -> (usize, u8) { + let specificity = entry.path.as_path().components().count(); + let access_priority = match entry.access { + FileSystemAccessMode::Read => 0, + FileSystemAccessMode::Write => 1, + FileSystemAccessMode::None => 2, + }; + (specificity, access_priority) +} + fn absolute_root_path_for_cwd(cwd: &AbsolutePathBuf) -> AbsolutePathBuf { let root = cwd .as_path() @@ -808,6 +922,7 @@ fn resolve_gitdir_from_file(dot_git: &AbsolutePathBuf) -> Option std::io::Result<()> { @@ -835,4 +950,115 @@ mod tests { ); Ok(()) } + + #[test] + fn resolve_access_with_cwd_uses_most_specific_entry() { + let cwd = TempDir::new().expect("tempdir"); + let docs = + AbsolutePathBuf::resolve_path_against_base("docs", cwd.path()).expect("resolve docs"); + let docs_private = AbsolutePathBuf::resolve_path_against_base("docs/private", cwd.path()) + .expect("resolve docs/private"); + let docs_private_public = + AbsolutePathBuf::resolve_path_against_base("docs/private/public", cwd.path()) + .expect("resolve docs/private/public"); + let policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::CurrentWorkingDirectory, + }, + access: FileSystemAccessMode::Write, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: docs.clone() }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: docs_private.clone(), + }, + access: FileSystemAccessMode::None, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: docs_private_public.clone(), + }, + access: FileSystemAccessMode::Write, + }, + ]); + + assert_eq!( + policy.resolve_access_with_cwd(cwd.path(), cwd.path()), + FileSystemAccessMode::Write + ); + assert_eq!( + policy.resolve_access_with_cwd(docs.as_path(), cwd.path()), + FileSystemAccessMode::Read + ); + assert_eq!( + policy.resolve_access_with_cwd(docs_private.as_path(), cwd.path()), + FileSystemAccessMode::None + ); + assert_eq!( + policy.resolve_access_with_cwd(docs_private_public.as_path(), cwd.path()), + FileSystemAccessMode::Write + ); + } + + #[test] + fn split_only_nested_carveouts_need_direct_runtime_enforcement() { + let cwd = TempDir::new().expect("tempdir"); + let docs = + AbsolutePathBuf::resolve_path_against_base("docs", cwd.path()).expect("resolve docs"); + let policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::CurrentWorkingDirectory, + }, + access: FileSystemAccessMode::Write, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: docs }, + access: FileSystemAccessMode::Read, + }, + ]); + + assert!( + policy.needs_direct_runtime_enforcement(NetworkSandboxPolicy::Restricted, cwd.path(),) + ); + + let legacy_workspace_write = + FileSystemSandboxPolicy::from(&SandboxPolicy::new_workspace_write_policy()); + assert!( + !legacy_workspace_write + .needs_direct_runtime_enforcement(NetworkSandboxPolicy::Restricted, cwd.path(),) + ); + } + + #[test] + fn root_write_with_read_only_child_is_not_full_disk_write() { + let cwd = TempDir::new().expect("tempdir"); + let docs = + AbsolutePathBuf::resolve_path_against_base("docs", cwd.path()).expect("resolve docs"); + let policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Write, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: docs.clone() }, + access: FileSystemAccessMode::Read, + }, + ]); + + assert!(!policy.has_full_disk_write_access()); + assert_eq!( + policy.resolve_access_with_cwd(docs.as_path(), cwd.path()), + FileSystemAccessMode::Read + ); + assert!( + policy.needs_direct_runtime_enforcement(NetworkSandboxPolicy::Restricted, cwd.path(),) + ); + } }