From d1b30474a92d08ad0e7c65bd77e79d74b98fa7f4 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 16 Apr 2026 17:02:23 -0700 Subject: [PATCH] protocol: canonicalize file system permissions --- .../app-server-protocol/src/protocol/v2.rs | 40 ++-- .../core/src/tools/handlers/apply_patch.rs | 8 +- codex-rs/core/src/tools/handlers/mod.rs | 8 +- codex-rs/exec-server/src/fs_sandbox.rs | 59 +++--- codex-rs/exec-server/tests/file_system.rs | 8 +- codex-rs/protocol/src/models.rs | 200 +++++++++++++++++- codex-rs/protocol/src/permissions.rs | 34 ++- codex-rs/sandboxing/src/manager_tests.rs | 16 +- codex-rs/sandboxing/src/policy_transforms.rs | 158 ++++++-------- .../sandboxing/src/policy_transforms_tests.rs | 93 ++++---- codex-rs/tui/src/app.rs | 16 +- codex-rs/tui/src/app/app_server_requests.rs | 8 +- .../src/app_server_approval_conversions.rs | 20 +- .../tui/src/bottom_pane/approval_overlay.rs | 63 +++--- .../src/chatwidget/tests/approval_requests.rs | 16 +- 15 files changed, 467 insertions(+), 280 deletions(-) diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 5173549326..da9c897a6a 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -1154,19 +1154,27 @@ pub struct AdditionalFileSystemPermissions { impl From for AdditionalFileSystemPermissions { fn from(value: CoreFileSystemPermissions) -> Self { - Self { - read: value.read, - write: value.write, - } + let (read, write) = value.legacy_read_write_roots().unwrap_or_else(|| { + let read = value + .explicit_path_entries() + .filter_map(|(path, access)| access.can_read().then_some(path.clone())) + .collect::>(); + let write = value + .explicit_path_entries() + .filter_map(|(path, access)| access.can_write().then_some(path.clone())) + .collect::>(); + ( + (!read.is_empty()).then_some(read), + (!write.is_empty()).then_some(write), + ) + }); + Self { read, write } } } impl From for CoreFileSystemPermissions { fn from(value: AdditionalFileSystemPermissions) -> Self { - Self { - read: value.read, - write: value.write, - } + CoreFileSystemPermissions::from_read_write_roots(value.read, value.write) } } @@ -6761,16 +6769,16 @@ mod tests { network: Some(CoreNetworkPermissions { enabled: Some(true), }), - file_system: Some(CoreFileSystemPermissions { - read: Some(vec![ + file_system: Some(CoreFileSystemPermissions::from_read_write_roots( + Some(vec![ AbsolutePathBuf::try_from(PathBuf::from(read_only_path)) .expect("path must be absolute"), ]), - write: Some(vec![ + Some(vec![ AbsolutePathBuf::try_from(PathBuf::from(read_write_path)) .expect("path must be absolute"), ]), - }), + )), } ); } @@ -6854,16 +6862,16 @@ mod tests { network: Some(CoreNetworkPermissions { enabled: Some(true), }), - file_system: Some(CoreFileSystemPermissions { - read: Some(vec![ + file_system: Some(CoreFileSystemPermissions::from_read_write_roots( + Some(vec![ AbsolutePathBuf::try_from(PathBuf::from(read_only_path)) .expect("path must be absolute"), ]), - write: Some(vec![ + Some(vec![ AbsolutePathBuf::try_from(PathBuf::from(read_write_path)) .expect("path must be absolute"), ]), - }), + )), } ); } diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 8e932c3ad7..59b464ea2f 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -82,10 +82,10 @@ fn write_permissions_for_paths( .ok()?; let permissions = (!write_paths.is_empty()).then_some(PermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(vec![]), - write: Some(write_paths), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![]), + Some(write_paths), + )), ..Default::default() })?; diff --git a/codex-rs/core/src/tools/handlers/mod.rs b/codex-rs/core/src/tools/handlers/mod.rs index 6b48680b8a..f436921e13 100644 --- a/codex-rs/core/src/tools/handlers/mod.rs +++ b/codex-rs/core/src/tools/handlers/mod.rs @@ -238,12 +238,12 @@ mod tests { fn file_system_permissions(path: &std::path::Path) -> PermissionProfile { PermissionProfile { - file_system: Some(FileSystemPermissions { - read: None, - write: Some(vec![ + file_system: Some(FileSystemPermissions::from_read_write_roots( + None, + Some(vec![ AbsolutePathBuf::from_absolute_path(path).expect("absolute path"), ]), - }), + )), ..Default::default() } } diff --git a/codex-rs/exec-server/src/fs_sandbox.rs b/codex-rs/exec-server/src/fs_sandbox.rs index 7b0ac256d4..87ca4be18a 100644 --- a/codex-rs/exec-server/src/fs_sandbox.rs +++ b/codex-rs/exec-server/src/fs_sandbox.rs @@ -3,6 +3,9 @@ use std::collections::HashMap; use codex_app_server_protocol::JSONRPCErrorError; use codex_protocol::models::FileSystemPermissions; use codex_protocol::models::PermissionProfile; +use codex_protocol::permissions::FileSystemAccessMode; +use codex_protocol::permissions::FileSystemPath; +use codex_protocol::permissions::FileSystemSandboxEntry; use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::ReadOnlyAccess; @@ -121,16 +124,23 @@ impl FileSystemSandboxRunner { match additional_permissions.and_then(|permissions| permissions.file_system.clone()) { Some(mut file_system) => { if let Some(helper_read_root) = &helper_read_root { - let read_paths = file_system.read.get_or_insert_with(Vec::new); - if !read_paths.contains(helper_read_root) { - read_paths.push(helper_read_root.clone()); + let helper_read_entry = FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: helper_read_root.clone(), + }, + access: FileSystemAccessMode::Read, + }; + if !file_system.entries.contains(&helper_read_entry) { + file_system.entries.push(helper_read_entry); } } Some(file_system) } - None => helper_read_root.map(|helper_read_root| FileSystemPermissions { - read: Some(vec![helper_read_root]), - write: None, + None => helper_read_root.map(|helper_read_root| { + FileSystemPermissions::from_read_write_roots( + Some(vec![helper_read_root]), + /*write*/ None, + ) }), }; @@ -373,27 +383,20 @@ mod tests { network: Some(NetworkPermissions { enabled: Some(true), }), - file_system: Some(FileSystemPermissions { - read: Some(vec![]), - write: Some(vec![writable.clone()]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![]), + Some(vec![writable.clone()]), + )), })); + let (read, write) = permissions + .file_system + .as_ref() + .and_then(FileSystemPermissions::legacy_read_write_roots) + .expect("helper permissions should stay lossless as legacy read/write roots"); assert_eq!(permissions.network, None); - assert_eq!( - permissions - .file_system - .as_ref() - .and_then(|fs| fs.write.clone()), - Some(vec![writable]) - ); - assert_eq!( - permissions - .file_system - .as_ref() - .and_then(|fs| fs.read.clone()), - Some(vec![readable]) - ); + assert_eq!(write, Some(vec![writable])); + assert_eq!(read, Some(vec![readable])); } #[test] @@ -415,10 +418,10 @@ mod tests { assert_eq!(permissions.network, None); assert_eq!( permissions.file_system, - Some(FileSystemPermissions { - read: Some(vec![readable]), - write: None, - }) + Some(FileSystemPermissions::from_read_write_roots( + Some(vec![readable]), + None, + )) ); } } diff --git a/codex-rs/exec-server/tests/file_system.rs b/codex-rs/exec-server/tests/file_system.rs index 8b9fb90ecc..49c5cf4f05 100644 --- a/codex-rs/exec-server/tests/file_system.rs +++ b/codex-rs/exec-server/tests/file_system.rs @@ -475,10 +475,10 @@ async fn file_system_sandboxed_write_allows_additional_write_root(use_remote: bo let mut sandbox = read_only_sandbox(readable_dir); sandbox.additional_permissions = Some(PermissionProfile { network: None, - file_system: Some(FileSystemPermissions { - read: None, - write: Some(vec![absolute_path(writable_dir)]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + None, + Some(vec![absolute_path(writable_dir)]), + )), }); file_system diff --git a/codex-rs/protocol/src/models.rs b/codex-rs/protocol/src/models.rs index 2200456527..bef9e1eb60 100644 --- a/codex-rs/protocol/src/models.rs +++ b/codex-rs/protocol/src/models.rs @@ -1,4 +1,5 @@ use std::collections::HashMap; +use std::io; use std::path::Path; use std::sync::LazyLock; @@ -14,6 +15,13 @@ use ts_rs::TS; use crate::config_types::ApprovalsReviewer; use crate::config_types::CollaborationMode; use crate::config_types::SandboxMode; +use crate::permissions::FileSystemAccessMode; +use crate::permissions::FileSystemPath; +use crate::permissions::FileSystemSandboxEntry; +use crate::permissions::FileSystemSandboxKind; +use crate::permissions::FileSystemSandboxPolicy; +use crate::permissions::FileSystemSpecialPath; +use crate::permissions::NetworkSandboxPolicy; use crate::protocol::AskForApproval; use crate::protocol::COLLABORATION_MODE_CLOSE_TAG; use crate::protocol::COLLABORATION_MODE_OPEN_TAG; @@ -80,15 +88,126 @@ impl SandboxPermissions { } } -#[derive(Debug, Clone, Default, Eq, Hash, PartialEq, Serialize, Deserialize, JsonSchema, TS)] +#[derive(Debug, Clone, Default, Eq, Hash, PartialEq, JsonSchema, TS)] pub struct FileSystemPermissions { - pub read: Option>, - pub write: Option>, + pub entries: Vec, } +pub type LegacyReadWriteRoots = (Option>, Option>); + impl FileSystemPermissions { pub fn is_empty(&self) -> bool { - self.read.is_none() && self.write.is_none() + self.entries.is_empty() + } + + pub fn from_read_write_roots( + read: Option>, + write: Option>, + ) -> Self { + let mut entries = Vec::new(); + if let Some(read) = read { + entries.extend(read.into_iter().map(|path| FileSystemSandboxEntry { + path: FileSystemPath::Path { path }, + access: FileSystemAccessMode::Read, + })); + } + if let Some(write) = write { + entries.extend(write.into_iter().map(|path| FileSystemSandboxEntry { + path: FileSystemPath::Path { path }, + access: FileSystemAccessMode::Write, + })); + } + Self { entries } + } + + pub fn explicit_path_entries( + &self, + ) -> impl Iterator { + self.entries.iter().filter_map(|entry| match &entry.path { + FileSystemPath::Path { path } => Some((path, entry.access)), + FileSystemPath::GlobPattern { .. } | FileSystemPath::Special { .. } => None, + }) + } + + pub fn legacy_read_write_roots(&self) -> Option { + self.as_legacy_permissions() + .map(|legacy| (legacy.read, legacy.write)) + } + + fn as_legacy_permissions(&self) -> Option { + let mut read = Vec::new(); + let mut write = Vec::new(); + + for entry in &self.entries { + let FileSystemPath::Path { path } = &entry.path else { + return None; + }; + match entry.access { + FileSystemAccessMode::Read => read.push(path.clone()), + FileSystemAccessMode::Write => write.push(path.clone()), + FileSystemAccessMode::None => return None, + } + } + + Some(LegacyFileSystemPermissions { + read: (!read.is_empty()).then_some(read), + write: (!write.is_empty()).then_some(write), + }) + } +} + +#[derive(Debug, Clone, Default, Eq, Hash, PartialEq, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +struct LegacyFileSystemPermissions { + #[serde(default, skip_serializing_if = "Option::is_none")] + read: Option>, + #[serde(default, skip_serializing_if = "Option::is_none")] + write: Option>, +} + +#[derive(Debug, Clone, Default, Eq, Hash, PartialEq, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +struct CanonicalFileSystemPermissions { + #[serde(default, skip_serializing_if = "Vec::is_empty")] + entries: Vec, +} + +#[derive(Debug, Clone, Deserialize)] +#[serde(untagged)] +enum FileSystemPermissionsDe { + Canonical(CanonicalFileSystemPermissions), + Legacy(LegacyFileSystemPermissions), +} + +impl Serialize for FileSystemPermissions { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + if let Some(legacy) = self.as_legacy_permissions() { + legacy.serialize(serializer) + } else { + CanonicalFileSystemPermissions { + entries: self.entries.clone(), + } + .serialize(serializer) + } + } +} + +impl<'de> Deserialize<'de> for FileSystemPermissions { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + match FileSystemPermissionsDe::deserialize(deserializer)? { + FileSystemPermissionsDe::Canonical(CanonicalFileSystemPermissions { entries }) => { + Ok(Self { entries }) + } + FileSystemPermissionsDe::Legacy(LegacyFileSystemPermissions { read, write }) => { + Ok(Self::from_read_write_roots(read, write)) + } + } } } @@ -113,6 +232,79 @@ impl PermissionProfile { pub fn is_empty(&self) -> bool { self.network.is_none() && self.file_system.is_none() } + + pub fn from_runtime_permissions( + file_system_sandbox_policy: &FileSystemSandboxPolicy, + network_sandbox_policy: NetworkSandboxPolicy, + ) -> Self { + Self { + network: Some(network_sandbox_policy.into()), + file_system: Some(file_system_sandbox_policy.into()), + } + } + + pub fn from_legacy_sandbox_policy(sandbox_policy: &SandboxPolicy, cwd: &Path) -> Self { + Self::from_runtime_permissions( + &FileSystemSandboxPolicy::from_legacy_sandbox_policy(sandbox_policy, cwd), + NetworkSandboxPolicy::from(sandbox_policy), + ) + } + + pub fn file_system_sandbox_policy(&self) -> FileSystemSandboxPolicy { + self.file_system.as_ref().map_or_else( + || FileSystemSandboxPolicy::restricted(Vec::new()), + FileSystemSandboxPolicy::from, + ) + } + + pub fn network_sandbox_policy(&self) -> NetworkSandboxPolicy { + if self + .network + .as_ref() + .and_then(|network| network.enabled) + .unwrap_or(false) + { + NetworkSandboxPolicy::Enabled + } else { + NetworkSandboxPolicy::Restricted + } + } + + pub fn to_legacy_sandbox_policy(&self, cwd: &Path) -> io::Result { + self.file_system_sandbox_policy() + .to_legacy_sandbox_policy(self.network_sandbox_policy(), cwd) + } +} + +impl From for NetworkPermissions { + fn from(value: NetworkSandboxPolicy) -> Self { + Self { + enabled: Some(value.is_enabled()), + } + } +} + +impl From<&FileSystemSandboxPolicy> for FileSystemPermissions { + fn from(value: &FileSystemSandboxPolicy) -> Self { + let entries = match value.kind { + FileSystemSandboxKind::Restricted => value.entries.clone(), + FileSystemSandboxKind::Unrestricted | FileSystemSandboxKind::ExternalSandbox => { + vec![FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Write, + }] + } + }; + Self { entries } + } +} + +impl From<&FileSystemPermissions> for FileSystemSandboxPolicy { + fn from(value: &FileSystemPermissions) -> Self { + FileSystemSandboxPolicy::restricted(value.entries.clone()) + } } #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, JsonSchema, TS)] diff --git a/codex-rs/protocol/src/permissions.rs b/codex-rs/protocol/src/permissions.rs index d774f6dc0d..a180880f02 100644 --- a/codex-rs/protocol/src/permissions.rs +++ b/codex-rs/protocol/src/permissions.rs @@ -46,6 +46,7 @@ impl NetworkSandboxPolicy { Debug, Clone, Copy, + Hash, PartialEq, Eq, PartialOrd, @@ -74,7 +75,7 @@ impl FileSystemAccessMode { } } -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema, TS)] +#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize, JsonSchema, TS)] #[serde(tag = "kind", rename_all = "snake_case")] #[ts(tag = "kind")] pub enum FileSystemSpecialPath { @@ -117,7 +118,7 @@ impl FileSystemSpecialPath { } } -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema, TS)] +#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize, JsonSchema, TS)] pub struct FileSystemSandboxEntry { pub path: FileSystemPath, pub access: FileSystemAccessMode, @@ -236,7 +237,7 @@ impl ReadDenyMatcher { } } -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema, TS)] +#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize, JsonSchema, TS)] #[serde(tag = "type", rename_all = "snake_case")] #[ts(tag = "type")] pub enum FileSystemPath { @@ -747,13 +748,14 @@ impl FileSystemSandboxPolicy { FileSystemSandboxKind::Restricted => { let cwd_absolute = AbsolutePathBuf::from_absolute_path(cwd).ok(); let mut include_platform_defaults = false; - let mut has_full_disk_read_access = false; - let mut has_full_disk_write_access = false; + let has_full_disk_read_access = self.has_full_disk_read_access(); + let has_full_disk_write_access = self.has_full_disk_write_access(); let mut workspace_root_writable = false; let mut writable_roots = Vec::new(); let mut readable_roots = Vec::new(); let mut tmpdir_writable = false; let mut slash_tmp_writable = false; + let mut unbridgeable_root_write = false; for entry in &self.entries { match &entry.path { @@ -772,10 +774,15 @@ impl FileSystemSandboxPolicy { FileSystemPath::Special { value } => match value { FileSystemSpecialPath::Root => match entry.access { FileSystemAccessMode::None => {} - FileSystemAccessMode::Read => has_full_disk_read_access = true, + FileSystemAccessMode::Read => { + if !has_full_disk_read_access + && let Some(cwd) = cwd_absolute.as_ref() + { + readable_roots.push(absolute_root_path_for_cwd(cwd)); + } + } FileSystemAccessMode::Write => { - has_full_disk_read_access = true; - has_full_disk_write_access = true; + unbridgeable_root_write = true; } }, FileSystemSpecialPath::Minimal => { @@ -870,7 +877,11 @@ impl FileSystemSandboxPolicy { exclude_tmpdir_env_var: !tmpdir_writable, exclude_slash_tmp: !slash_tmp_writable, } - } else if !writable_roots.is_empty() || tmpdir_writable || slash_tmp_writable { + } else if unbridgeable_root_write + || !writable_roots.is_empty() + || tmpdir_writable + || slash_tmp_writable + { return Err(io::Error::new( io::ErrorKind::InvalidInput, "permissions profile requests filesystem writes outside the workspace root, which is not supported until the runtime enforces FileSystemSandboxPolicy directly", @@ -2079,6 +2090,11 @@ mod tests { assert!( policy.needs_direct_runtime_enforcement(NetworkSandboxPolicy::Restricted, cwd.path(),) ); + assert!( + policy + .to_legacy_sandbox_policy(NetworkSandboxPolicy::Restricted, cwd.path()) + .is_err() + ); } #[test] diff --git a/codex-rs/sandboxing/src/manager_tests.rs b/codex-rs/sandboxing/src/manager_tests.rs index f4e019ee28..c860947b03 100644 --- a/codex-rs/sandboxing/src/manager_tests.rs +++ b/codex-rs/sandboxing/src/manager_tests.rs @@ -130,10 +130,10 @@ fn transform_additional_permissions_enable_network_for_external_sandbox() { network: Some(NetworkPermissions { enabled: Some(true), }), - file_system: Some(FileSystemPermissions { - read: Some(vec![path]), - write: Some(Vec::new()), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![path]), + Some(Vec::new()), + )), }), }, policy: &SandboxPolicy::ExternalSandbox { @@ -183,10 +183,10 @@ fn transform_additional_permissions_preserves_denied_entries() { cwd: cwd.clone(), env: HashMap::new(), additional_permissions: Some(PermissionProfile { - file_system: Some(FileSystemPermissions { - read: None, - write: Some(vec![allowed_path.clone()]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + None, + Some(vec![allowed_path.clone()]), + )), ..Default::default() }), }, diff --git a/codex-rs/sandboxing/src/policy_transforms.rs b/codex-rs/sandboxing/src/policy_transforms.rs index 73ecf81c9b..bcd02acd55 100644 --- a/codex-rs/sandboxing/src/policy_transforms.rs +++ b/codex-rs/sandboxing/src/policy_transforms.rs @@ -1,7 +1,6 @@ use codex_protocol::models::FileSystemPermissions; use codex_protocol::models::NetworkPermissions; use codex_protocol::models::PermissionProfile; -use codex_protocol::permissions::FileSystemAccessMode; use codex_protocol::permissions::FileSystemPath; use codex_protocol::permissions::FileSystemSandboxEntry; use codex_protocol::permissions::FileSystemSandboxKind; @@ -45,13 +44,29 @@ pub fn normalize_additional_permissions( let file_system = additional_permissions .file_system .map(|file_system| { - let read = file_system - .read - .map(|paths| normalize_permission_paths(paths, "file_system.read")); - let write = file_system - .write - .map(|paths| normalize_permission_paths(paths, "file_system.write")); - FileSystemPermissions { read, write } + let mut entries = Vec::with_capacity(file_system.entries.len()); + for entry in file_system.entries { + let path = match entry.path { + FileSystemPath::Path { path } => FileSystemPath::Path { + path: canonicalize_preserving_symlinks(path.as_path()) + .ok() + .and_then(|path| AbsolutePathBuf::from_absolute_path(path).ok()) + .unwrap_or(path), + }, + FileSystemPath::GlobPattern { pattern } => { + FileSystemPath::GlobPattern { pattern } + } + FileSystemPath::Special { value } => FileSystemPath::Special { value }, + }; + let normalized_entry = FileSystemSandboxEntry { + path, + access: entry.access, + }; + if !entries.contains(&normalized_entry) { + entries.push(normalized_entry); + } + } + FileSystemPermissions { entries } }) .filter(|file_system| !file_system.is_empty()); Ok(PermissionProfile { @@ -89,8 +104,7 @@ pub fn merge_permission_profiles( }; let file_system = match (base.file_system.as_ref(), permissions.file_system.as_ref()) { (Some(base), Some(permissions)) => Some(FileSystemPermissions { - read: merge_permission_paths(base.read.as_ref(), permissions.read.as_ref()), - write: merge_permission_paths(base.write.as_ref(), permissions.write.as_ref()), + entries: merge_permission_entries(&base.entries, &permissions.entries), }) .filter(|file_system| !file_system.is_empty()), (Some(base), None) => Some(base.clone()), @@ -116,11 +130,12 @@ pub fn intersect_permission_profiles( .file_system .map(|requested_file_system| { let granted_file_system = granted.file_system.unwrap_or_default(); - let read = - intersect_permission_paths(requested_file_system.read, granted_file_system.read); - let write = - intersect_permission_paths(requested_file_system.write, granted_file_system.write); - FileSystemPermissions { read, write } + let entries = requested_file_system + .entries + .into_iter() + .filter(|entry| granted_file_system.entries.contains(entry)) + .collect(); + FileSystemPermissions { entries } }) .filter(|file_system| !file_system.is_empty()); let network = match (requested.network, granted.network) { @@ -143,67 +158,17 @@ pub fn intersect_permission_profiles( } } -fn intersect_permission_paths( - requested: Option>, - granted: Option>, -) -> Option> { - requested.and_then(|requested_paths| { - if requested_paths.is_empty() { - return granted.map(|_| Vec::new()); - } - - let granted_paths = granted.unwrap_or_default(); - Some( - requested_paths - .into_iter() - .filter(|path| granted_paths.contains(path)) - .collect::>(), - ) - .filter(|paths| !paths.is_empty()) - }) -} - -fn normalize_permission_paths( - paths: Vec, - _permission_kind: &str, -) -> Vec { - let mut out = Vec::with_capacity(paths.len()); - let mut seen = HashSet::new(); - - for path in paths { - let canonicalized = canonicalize_preserving_symlinks(path.as_path()) - .ok() - .and_then(|path| AbsolutePathBuf::from_absolute_path(path).ok()) - .unwrap_or(path); - if seen.insert(canonicalized.clone()) { - out.push(canonicalized); +fn merge_permission_entries( + base: &[FileSystemSandboxEntry], + permissions: &[FileSystemSandboxEntry], +) -> Vec { + let mut merged = Vec::with_capacity(base.len() + permissions.len()); + for entry in base.iter().chain(permissions.iter()) { + if !merged.contains(entry) { + merged.push(entry.clone()); } } - - out -} - -fn merge_permission_paths( - base: Option<&Vec>, - permissions: Option<&Vec>, -) -> Option> { - match (base, permissions) { - (Some(base), Some(permissions)) => { - let mut merged = Vec::with_capacity(base.len() + permissions.len()); - let mut seen = HashSet::with_capacity(base.len() + permissions.len()); - - for path in base.iter().chain(permissions.iter()) { - if seen.insert(path.clone()) { - merged.push(path.clone()); - } - } - - Some(merged).filter(|paths| !paths.is_empty()) - } - (Some(base), None) => Some(base.clone()), - (None, Some(permissions)) => Some(permissions.clone()), - (None, None) => None, - } + merged } fn dedup_absolute_paths(paths: Vec) -> Vec { @@ -225,14 +190,24 @@ fn additional_permission_roots( additional_permissions .file_system .as_ref() - .and_then(|file_system| file_system.read.clone()) + .map(|file_system| { + file_system + .explicit_path_entries() + .filter_map(|(path, access)| access.can_read().then_some(path.clone())) + .collect() + }) .unwrap_or_default(), ), dedup_absolute_paths( additional_permissions .file_system .as_ref() - .and_then(|file_system| file_system.write.clone()) + .map(|file_system| { + file_system + .explicit_path_entries() + .filter_map(|(path, access)| access.can_write().then_some(path.clone())) + .collect() + }) .unwrap_or_default(), ), ) @@ -240,28 +215,14 @@ fn additional_permission_roots( fn merge_file_system_policy_with_additional_permissions( file_system_policy: &FileSystemSandboxPolicy, - extra_reads: Vec, - extra_writes: Vec, + additional_permissions: &FileSystemPermissions, ) -> FileSystemSandboxPolicy { match file_system_policy.kind { FileSystemSandboxKind::Restricted => { let mut merged_policy = file_system_policy.clone(); - for path in extra_reads { - let entry = FileSystemSandboxEntry { - path: FileSystemPath::Path { path }, - access: FileSystemAccessMode::Read, - }; - if !merged_policy.entries.contains(&entry) { - merged_policy.entries.push(entry); - } - } - for path in extra_writes { - let entry = FileSystemSandboxEntry { - path: FileSystemPath::Path { path }, - access: FileSystemAccessMode::Write, - }; - if !merged_policy.entries.contains(&entry) { - merged_policy.entries.push(entry); + for entry in &additional_permissions.entries { + if !merged_policy.entries.contains(entry) { + merged_policy.entries.push(entry.clone()); } } merged_policy @@ -280,14 +241,15 @@ pub fn effective_file_system_sandbox_policy( return file_system_policy.clone(); }; - let (extra_reads, extra_writes) = additional_permission_roots(additional_permissions); - if extra_reads.is_empty() && extra_writes.is_empty() { + let Some(file_system_permissions) = additional_permissions.file_system.as_ref() else { + return file_system_policy.clone(); + }; + if file_system_permissions.is_empty() { file_system_policy.clone() } else { merge_file_system_policy_with_additional_permissions( file_system_policy, - extra_reads, - extra_writes, + file_system_permissions, ) } } diff --git a/codex-rs/sandboxing/src/policy_transforms_tests.rs b/codex-rs/sandboxing/src/policy_transforms_tests.rs index f33d312f15..3e16462ec8 100644 --- a/codex-rs/sandboxing/src/policy_transforms_tests.rs +++ b/codex-rs/sandboxing/src/policy_transforms_tests.rs @@ -106,10 +106,10 @@ fn normalize_additional_permissions_preserves_network() { network: Some(NetworkPermissions { enabled: Some(true), }), - file_system: Some(FileSystemPermissions { - read: Some(vec![path.clone()]), - write: Some(vec![path.clone()]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![path.clone()]), + Some(vec![path.clone()]), + )), }) .expect("permissions"); @@ -121,10 +121,10 @@ fn normalize_additional_permissions_preserves_network() { ); assert_eq!( permissions.file_system, - Some(FileSystemPermissions { - read: Some(vec![path.clone()]), - write: Some(vec![path]), - }) + Some(FileSystemPermissions::from_read_write_roots( + Some(vec![path.clone()]), + Some(vec![path]), + )) ); } @@ -141,23 +141,23 @@ fn normalize_additional_permissions_preserves_symlinked_write_paths() { let link_write_dir = AbsolutePathBuf::from_absolute_path(link_root.join("write")).expect("link write dir"); let permissions = normalize_additional_permissions(PermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(vec![]), - write: Some(vec![link_write_dir]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![]), + Some(vec![link_write_dir]), + )), ..Default::default() }) .expect("permissions"); assert_eq!( permissions.file_system, - Some(FileSystemPermissions { - read: Some(vec![]), - write: Some(vec![ + Some(FileSystemPermissions::from_read_write_roots( + Some(vec![]), + Some(vec![ AbsolutePathBuf::from_absolute_path(link_root.join("write")) - .expect("link write dir") + .expect("link write dir"), ]), - }) + )) ); } @@ -165,10 +165,7 @@ fn normalize_additional_permissions_preserves_symlinked_write_paths() { fn normalize_additional_permissions_drops_empty_nested_profiles() { let permissions = normalize_additional_permissions(PermissionProfile { network: Some(NetworkPermissions { enabled: None }), - file_system: Some(FileSystemPermissions { - read: None, - write: None, - }), + file_system: Some(FileSystemPermissions::default()), }) .expect("permissions"); @@ -183,10 +180,10 @@ fn intersect_permission_profiles_preserves_explicit_empty_requested_reads() { ) .expect("absolute temp dir"); let requested = PermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(vec![]), - write: Some(vec![path]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![]), + Some(vec![path]), + )), ..Default::default() }; let granted = requested.clone(); @@ -205,10 +202,10 @@ fn intersect_permission_profiles_drops_ungranted_nonempty_path_requests() { ) .expect("absolute temp dir"); let requested = PermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(vec![path]), - write: None, - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![path]), + None, + )), ..Default::default() }; @@ -226,10 +223,10 @@ fn intersect_permission_profiles_drops_explicit_empty_reads_without_grant() { ) .expect("absolute temp dir"); let requested = PermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(vec![]), - write: Some(vec![path]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![]), + Some(vec![path]), + )), ..Default::default() }; @@ -258,10 +255,10 @@ fn read_only_additional_permissions_can_enable_network_without_writes() { network: Some(NetworkPermissions { enabled: Some(true), }), - file_system: Some(FileSystemPermissions { - read: Some(vec![path.clone()]), - write: Some(Vec::new()), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![path.clone()]), + Some(Vec::new()), + )), }, ); @@ -292,10 +289,10 @@ fn external_sandbox_additional_permissions_can_enable_network() { network: Some(NetworkPermissions { enabled: Some(true), }), - file_system: Some(FileSystemPermissions { - read: Some(vec![path]), - write: Some(Vec::new()), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![path]), + Some(Vec::new()), + )), }, ); @@ -331,8 +328,10 @@ fn merge_file_system_policy_with_additional_permissions_preserves_unreadable_roo access: FileSystemAccessMode::None, }, ]), - vec![allowed_path.clone()], - Vec::new(), + &FileSystemPermissions::from_read_write_roots( + Some(vec![allowed_path.clone()]), + Some(Vec::new()), + ), ); assert_eq!( @@ -402,10 +401,10 @@ fn effective_file_system_sandbox_policy_merges_additional_write_roots() { }, ]); let additional_permissions = PermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(vec![]), - write: Some(vec![allowed_path.clone()]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![]), + Some(vec![allowed_path.clone()]), + )), ..Default::default() }; diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index ab20b2214c..4fb194dfec 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -9037,10 +9037,10 @@ guardian_approval = true network: Some(NetworkPermissions { enabled: Some(true), }), - file_system: Some(FileSystemPermissions { - read: Some(vec![test_absolute_path("/tmp/read-only")]), - write: Some(vec![test_absolute_path("/tmp/write")]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![test_absolute_path("/tmp/read-only")]), + Some(vec![test_absolute_path("/tmp/write")]), + )), }) ); assert_eq!( @@ -9133,10 +9133,10 @@ guardian_approval = true network: Some(NetworkPermissions { enabled: Some(true), }), - file_system: Some(FileSystemPermissions { - read: Some(vec![test_absolute_path("/tmp/read-only")]), - write: Some(vec![test_absolute_path("/tmp/write")]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![test_absolute_path("/tmp/read-only")]), + Some(vec![test_absolute_path("/tmp/write")]), + )), } ); } diff --git a/codex-rs/tui/src/app/app_server_requests.rs b/codex-rs/tui/src/app/app_server_requests.rs index e2483eb25d..4a97e26d00 100644 --- a/codex-rs/tui/src/app/app_server_requests.rs +++ b/codex-rs/tui/src/app/app_server_requests.rs @@ -536,10 +536,10 @@ mod tests { network: Some(NetworkPermissions { enabled: Some(true), }), - file_system: Some(FileSystemPermissions { - read: Some(vec![absolute_path(read_path)]), - write: Some(vec![absolute_path(write_path)]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![absolute_path(read_path)]), + Some(vec![absolute_path(write_path)]), + )), }, scope: codex_protocol::request_permissions::PermissionGrantScope::Session, }, diff --git a/codex-rs/tui/src/app_server_approval_conversions.rs b/codex-rs/tui/src/app_server_approval_conversions.rs index 8d2335eb7d..2db10aeeba 100644 --- a/codex-rs/tui/src/app_server_approval_conversions.rs +++ b/codex-rs/tui/src/app_server_approval_conversions.rs @@ -35,12 +35,12 @@ pub(crate) fn granted_permission_profile_from_request( network: value.network.map(|network| AdditionalNetworkPermissions { enabled: network.enabled, }), - file_system: value - .file_system - .map(|file_system| AdditionalFileSystemPermissions { - read: file_system.read, - write: file_system.write, - }), + file_system: value.file_system.map(|file_system| { + let (read, write) = file_system + .legacy_read_write_roots() + .unwrap_or((None, None)); + AdditionalFileSystemPermissions { read, write } + }), } } @@ -82,10 +82,10 @@ mod tests { network: Some(NetworkPermissions { enabled: Some(true), }), - file_system: Some(FileSystemPermissions { - read: Some(vec![absolute_path("/tmp/read-only")]), - write: Some(vec![absolute_path("/tmp/write")]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![absolute_path("/tmp/read-only")]), + Some(vec![absolute_path("/tmp/write")]), + )), }), codex_app_server_protocol::GrantedPermissionProfile { network: Some(codex_app_server_protocol::AdditionalNetworkPermissions { diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index 0e2bb2b2e3..5d5ba9b1ff 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -21,6 +21,7 @@ use codex_features::Features; use codex_protocol::ThreadId; use codex_protocol::mcp::RequestId; use codex_protocol::models::PermissionProfile; +use codex_protocol::permissions::FileSystemAccessMode; use codex_protocol::protocol::ElicitationAction; use codex_protocol::protocol::FileChange; use codex_protocol::protocol::NetworkApprovalContext; @@ -798,20 +799,26 @@ pub(crate) fn format_additional_permissions_rule( parts.push("network".to_string()); } if let Some(file_system) = additional_permissions.file_system.as_ref() { - if let Some(read) = file_system.read.as_ref() { - let reads = read - .iter() - .map(|path| format!("`{}`", path.display())) - .collect::>() - .join(", "); + let reads = file_system + .explicit_path_entries() + .filter_map(|(path, access)| { + matches!(access, FileSystemAccessMode::Read).then_some(path) + }) + .map(|path| format!("`{}`", path.display())) + .collect::>() + .join(", "); + if !reads.is_empty() { parts.push(format!("read {reads}")); } - if let Some(write) = file_system.write.as_ref() { - let writes = write - .iter() - .map(|path| format!("`{}`", path.display())) - .collect::>() - .join(", "); + let writes = file_system + .explicit_path_entries() + .filter_map(|(path, access)| { + matches!(access, FileSystemAccessMode::Write).then_some(path) + }) + .map(|path| format!("`{}`", path.display())) + .collect::>() + .join(", "); + if !writes.is_empty() { parts.push(format!("write {writes}")); } } @@ -965,10 +972,10 @@ mod tests { network: Some(NetworkPermissions { enabled: Some(true), }), - file_system: Some(FileSystemPermissions { - read: Some(vec![absolute_path("/tmp/readme.txt")]), - write: Some(vec![absolute_path("/tmp/out.txt")]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![absolute_path("/tmp/readme.txt")]), + Some(vec![absolute_path("/tmp/out.txt")]), + )), }, } } @@ -1266,10 +1273,10 @@ mod tests { #[test] fn additional_permissions_exec_options_hide_execpolicy_amendment() { let additional_permissions = PermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(vec![absolute_path("/tmp/readme.txt")]), - write: Some(vec![absolute_path("/tmp/out.txt")]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![absolute_path("/tmp/readme.txt")]), + Some(vec![absolute_path("/tmp/out.txt")]), + )), ..Default::default() }; let options = exec_options( @@ -1347,10 +1354,10 @@ mod tests { network: Some(NetworkPermissions { enabled: Some(true), }), - file_system: Some(FileSystemPermissions { - read: Some(vec![absolute_path("/tmp/readme.txt")]), - write: Some(vec![absolute_path("/tmp/out.txt")]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![absolute_path("/tmp/readme.txt")]), + Some(vec![absolute_path("/tmp/out.txt")]), + )), }), }; @@ -1397,10 +1404,10 @@ mod tests { network: Some(NetworkPermissions { enabled: Some(true), }), - file_system: Some(FileSystemPermissions { - read: Some(vec![absolute_path("/tmp/readme.txt")]), - write: Some(vec![absolute_path("/tmp/out.txt")]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![absolute_path("/tmp/readme.txt")]), + Some(vec![absolute_path("/tmp/out.txt")]), + )), }), }; diff --git a/codex-rs/tui/src/chatwidget/tests/approval_requests.rs b/codex-rs/tui/src/chatwidget/tests/approval_requests.rs index 512b415926..7c207297d8 100644 --- a/codex-rs/tui/src/chatwidget/tests/approval_requests.rs +++ b/codex-rs/tui/src/chatwidget/tests/approval_requests.rs @@ -135,10 +135,10 @@ fn app_server_exec_approval_request_preserves_permissions_context() { network: Some(NetworkPermissions { enabled: Some(true), }), - file_system: Some(FileSystemPermissions { - read: Some(vec![read_path]), - write: Some(vec![write_path]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![read_path]), + Some(vec![write_path]), + )), }) ); } @@ -172,10 +172,10 @@ fn app_server_request_permissions_preserves_file_system_permissions() { network: Some(NetworkPermissions { enabled: Some(true), }), - file_system: Some(FileSystemPermissions { - read: Some(vec![read_path]), - write: Some(vec![write_path]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![read_path]), + Some(vec![write_path]), + )), } ); }