From 9deb8ce3fcc48b43216c8391a00572cbefcb6ae5 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Mon, 23 Mar 2026 22:22:44 -0700 Subject: [PATCH] Move sandbox policy transforms into codex-sandboxing (#15599) ## Summary - move the pure sandbox policy transform helpers from `codex-core` into `codex-sandboxing` - move the corresponding unit tests with the extracted implementation - update `core` and `app-server` callers to import the moved APIs directly, without re-exports or proxy methods ## Testing - cargo test -p codex-sandboxing - cargo test -p codex-core sandboxing - cargo test -p codex-app-server --lib - just fix -p codex-sandboxing - just fix -p codex-core - just fix -p codex-app-server - just fmt - just argument-comment-lint --- codex-rs/Cargo.lock | 2 + codex-rs/app-server/Cargo.toml | 1 + .../app-server/src/bespoke_event_handling.rs | 2 +- codex-rs/core/src/sandboxing/mod.rs | 467 +--------------- codex-rs/core/src/sandboxing/mod_tests.rs | 511 ----------------- codex-rs/core/src/state/session.rs | 2 +- codex-rs/core/src/state/turn.rs | 2 +- .../core/src/tools/handlers/apply_patch.rs | 7 +- codex-rs/core/src/tools/handlers/mod.rs | 11 +- .../src/tools/handlers/request_permissions.rs | 2 +- codex-rs/sandboxing/Cargo.toml | 1 + codex-rs/sandboxing/src/lib.rs | 1 + codex-rs/sandboxing/src/policy_transforms.rs | 463 +++++++++++++++ .../sandboxing/src/policy_transforms_tests.rs | 528 ++++++++++++++++++ 14 files changed, 1020 insertions(+), 980 deletions(-) create mode 100644 codex-rs/sandboxing/src/policy_transforms.rs create mode 100644 codex-rs/sandboxing/src/policy_transforms_tests.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index c14c633c4..d0c6c390d 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1448,6 +1448,7 @@ dependencies = [ "codex-otel", "codex-protocol", "codex-rmcp-client", + "codex-sandboxing", "codex-shell-command", "codex-state", "codex-utils-absolute-path", @@ -2472,6 +2473,7 @@ dependencies = [ "codex-protocol", "codex-utils-absolute-path", "dirs", + "dunce", "libc", "pretty_assertions", "serde_json", diff --git a/codex-rs/app-server/Cargo.toml b/codex-rs/app-server/Cargo.toml index c6be85984..f9e069703 100644 --- a/codex-rs/app-server/Cargo.toml +++ b/codex-rs/app-server/Cargo.toml @@ -46,6 +46,7 @@ codex-protocol = { workspace = true } codex-app-server-protocol = { workspace = true } codex-feedback = { workspace = true } codex-rmcp-client = { workspace = true } +codex-sandboxing = { workspace = true } codex-state = { workspace = true } codex-utils-absolute-path = { workspace = true } codex-utils-json-to-toml = { workspace = true } diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index 34640a50c..15484d3d5 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -110,7 +110,6 @@ use codex_core::ThreadManager; use codex_core::find_thread_name_by_id; use codex_core::review_format::format_review_findings_block; use codex_core::review_prompts; -use codex_core::sandboxing::intersect_permission_profiles; use codex_protocol::ThreadId; use codex_protocol::dynamic_tools::DynamicToolCallOutputContentItem as CoreDynamicToolCallOutputContentItem; use codex_protocol::dynamic_tools::DynamicToolResponse as CoreDynamicToolResponse; @@ -136,6 +135,7 @@ use codex_protocol::request_permissions::RequestPermissionProfile as CoreRequest use codex_protocol::request_permissions::RequestPermissionsResponse as CoreRequestPermissionsResponse; use codex_protocol::request_user_input::RequestUserInputAnswer as CoreRequestUserInputAnswer; use codex_protocol::request_user_input::RequestUserInputResponse as CoreRequestUserInputResponse; +use codex_sandboxing::policy_transforms::intersect_permission_profiles; use codex_shell_command::parse_command::shlex_join; use std::collections::HashMap; use std::convert::TryFrom; diff --git a/codex-rs/core/src/sandboxing/mod.rs b/codex-rs/core/src/sandboxing/mod.rs index e9cf082ca..fdce99d3e 100644 --- a/codex-rs/core/src/sandboxing/mod.rs +++ b/codex-rs/core/src/sandboxing/mod.rs @@ -19,31 +19,23 @@ use crate::spawn::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR; use crate::tools::sandboxing::SandboxablePreference; use codex_network_proxy::NetworkProxy; use codex_protocol::config_types::WindowsSandboxLevel; -use codex_protocol::models::FileSystemPermissions; +#[cfg(target_os = "macos")] use codex_protocol::models::MacOsSeatbeltProfileExtensions; -use codex_protocol::models::NetworkPermissions; use codex_protocol::models::PermissionProfile; pub use codex_protocol::models::SandboxPermissions; -use codex_protocol::permissions::FileSystemAccessMode; -use codex_protocol::permissions::FileSystemPath; -use codex_protocol::permissions::FileSystemSandboxEntry; -use codex_protocol::permissions::FileSystemSandboxKind; use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::permissions::NetworkSandboxPolicy; -use codex_protocol::protocol::NetworkAccess; -use codex_protocol::protocol::ReadOnlyAccess; use codex_sandboxing::landlock::allow_network_for_proxy; use codex_sandboxing::landlock::create_linux_sandbox_command_args_for_policies; -use codex_sandboxing::macos_permissions::intersect_macos_seatbelt_profile_extensions; -use codex_sandboxing::macos_permissions::merge_macos_seatbelt_profile_extensions; +use codex_sandboxing::policy_transforms::EffectiveSandboxPermissions; +use codex_sandboxing::policy_transforms::effective_file_system_sandbox_policy; +use codex_sandboxing::policy_transforms::effective_network_sandbox_policy; +use codex_sandboxing::policy_transforms::should_require_platform_sandbox; #[cfg(target_os = "macos")] use codex_sandboxing::seatbelt::MACOS_PATH_TO_SEATBELT_EXECUTABLE; #[cfg(target_os = "macos")] use codex_sandboxing::seatbelt::create_seatbelt_command_args_for_policies_with_extensions; -use codex_utils_absolute_path::AbsolutePathBuf; -use dunce::canonicalize; use std::collections::HashMap; -use std::collections::HashSet; use std::path::Path; use std::path::PathBuf; @@ -116,425 +108,6 @@ pub(crate) enum SandboxTransformError { SeatbeltUnavailable, } -#[derive(Debug, Clone, PartialEq, Eq)] -pub(crate) struct EffectiveSandboxPermissions { - pub(crate) sandbox_policy: SandboxPolicy, - pub(crate) macos_seatbelt_profile_extensions: Option, -} - -impl EffectiveSandboxPermissions { - pub(crate) fn new( - sandbox_policy: &SandboxPolicy, - macos_seatbelt_profile_extensions: Option<&MacOsSeatbeltProfileExtensions>, - additional_permissions: Option<&PermissionProfile>, - ) -> Self { - let Some(additional_permissions) = additional_permissions else { - return Self { - sandbox_policy: sandbox_policy.clone(), - macos_seatbelt_profile_extensions: macos_seatbelt_profile_extensions.cloned(), - }; - }; - - Self { - sandbox_policy: sandbox_policy_with_additional_permissions( - sandbox_policy, - additional_permissions, - ), - macos_seatbelt_profile_extensions: merge_macos_seatbelt_profile_extensions( - macos_seatbelt_profile_extensions, - additional_permissions.macos.as_ref(), - ), - } - } -} - -pub(crate) fn normalize_additional_permissions( - additional_permissions: PermissionProfile, -) -> Result { - let network = additional_permissions - .network - .filter(|network| !network.is_empty()); - 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 } - }) - .filter(|file_system| !file_system.is_empty()); - let macos = additional_permissions.macos; - - Ok(PermissionProfile { - network, - file_system, - macos, - }) -} - -pub(crate) fn merge_permission_profiles( - base: Option<&PermissionProfile>, - permissions: Option<&PermissionProfile>, -) -> Option { - let Some(permissions) = permissions else { - return base.cloned(); - }; - - match base { - Some(base) => { - let network = match (base.network.as_ref(), permissions.network.as_ref()) { - ( - Some(NetworkPermissions { - enabled: Some(true), - }), - _, - ) - | ( - _, - Some(NetworkPermissions { - enabled: Some(true), - }), - ) => Some(NetworkPermissions { - enabled: Some(true), - }), - _ => None, - }; - 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()), - }) - .filter(|file_system| !file_system.is_empty()), - (Some(base), None) => Some(base.clone()), - (None, Some(permissions)) => Some(permissions.clone()), - (None, None) => None, - }; - let macos = merge_macos_seatbelt_profile_extensions( - base.macos.as_ref(), - permissions.macos.as_ref(), - ); - - Some(PermissionProfile { - network, - file_system, - macos, - }) - .filter(|permissions| !permissions.is_empty()) - } - None => Some(permissions.clone()).filter(|permissions| !permissions.is_empty()), - } -} - -pub fn intersect_permission_profiles( - requested: PermissionProfile, - granted: PermissionProfile, -) -> PermissionProfile { - let file_system = requested - .file_system - .map(|requested_file_system| { - let granted_file_system = granted.file_system.unwrap_or_default(); - let read = requested_file_system - .read - .map(|requested_read| { - let granted_read = granted_file_system.read.unwrap_or_default(); - requested_read - .into_iter() - .filter(|path| granted_read.contains(path)) - .collect() - }) - .filter(|paths: &Vec<_>| !paths.is_empty()); - let write = requested_file_system - .write - .map(|requested_write| { - let granted_write = granted_file_system.write.unwrap_or_default(); - requested_write - .into_iter() - .filter(|path| granted_write.contains(path)) - .collect() - }) - .filter(|paths: &Vec<_>| !paths.is_empty()); - FileSystemPermissions { read, write } - }) - .filter(|file_system| !file_system.is_empty()); - let network = match (requested.network, granted.network) { - ( - Some(NetworkPermissions { - enabled: Some(true), - }), - Some(NetworkPermissions { - enabled: Some(true), - }), - ) => Some(NetworkPermissions { - enabled: Some(true), - }), - _ => None, - }; - - let macos = intersect_macos_seatbelt_profile_extensions(requested.macos, granted.macos); - - PermissionProfile { - network, - file_system, - macos, - } -} - -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(path.as_path()) - .ok() - .and_then(|path| AbsolutePathBuf::from_absolute_path(path).ok()) - .unwrap_or(path); - if seen.insert(canonicalized.clone()) { - out.push(canonicalized); - } - } - - 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, - } -} - -fn dedup_absolute_paths(paths: Vec) -> Vec { - let mut out = Vec::with_capacity(paths.len()); - let mut seen = HashSet::new(); - for path in paths { - if seen.insert(path.to_path_buf()) { - out.push(path); - } - } - out -} - -fn additional_permission_roots( - additional_permissions: &PermissionProfile, -) -> (Vec, Vec) { - ( - dedup_absolute_paths( - additional_permissions - .file_system - .as_ref() - .and_then(|file_system| file_system.read.clone()) - .unwrap_or_default(), - ), - dedup_absolute_paths( - additional_permissions - .file_system - .as_ref() - .and_then(|file_system| file_system.write.clone()) - .unwrap_or_default(), - ), - ) -} - -fn merge_file_system_policy_with_additional_permissions( - file_system_policy: &FileSystemSandboxPolicy, - extra_reads: Vec, - extra_writes: Vec, -) -> 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); - } - } - merged_policy - } - FileSystemSandboxKind::Unrestricted | FileSystemSandboxKind::ExternalSandbox => { - file_system_policy.clone() - } - } -} - -pub(crate) fn effective_file_system_sandbox_policy( - file_system_policy: &FileSystemSandboxPolicy, - additional_permissions: Option<&PermissionProfile>, -) -> FileSystemSandboxPolicy { - let Some(additional_permissions) = additional_permissions else { - return file_system_policy.clone(); - }; - - let (extra_reads, extra_writes) = additional_permission_roots(additional_permissions); - if extra_reads.is_empty() && extra_writes.is_empty() { - file_system_policy.clone() - } else { - merge_file_system_policy_with_additional_permissions( - file_system_policy, - extra_reads, - extra_writes, - ) - } -} - -fn merge_read_only_access_with_additional_reads( - read_only_access: &ReadOnlyAccess, - extra_reads: Vec, -) -> ReadOnlyAccess { - match read_only_access { - ReadOnlyAccess::FullAccess => ReadOnlyAccess::FullAccess, - ReadOnlyAccess::Restricted { - include_platform_defaults, - readable_roots, - } => { - let mut merged = readable_roots.clone(); - merged.extend(extra_reads); - ReadOnlyAccess::Restricted { - include_platform_defaults: *include_platform_defaults, - readable_roots: dedup_absolute_paths(merged), - } - } - } -} - -fn merge_network_access( - base_network_access: bool, - additional_permissions: &PermissionProfile, -) -> bool { - base_network_access - || additional_permissions - .network - .as_ref() - .and_then(|network| network.enabled) - .unwrap_or(false) -} - -fn sandbox_policy_with_additional_permissions( - sandbox_policy: &SandboxPolicy, - additional_permissions: &PermissionProfile, -) -> SandboxPolicy { - if additional_permissions.is_empty() { - return sandbox_policy.clone(); - } - - let (extra_reads, extra_writes) = additional_permission_roots(additional_permissions); - - match sandbox_policy { - SandboxPolicy::DangerFullAccess => SandboxPolicy::DangerFullAccess, - SandboxPolicy::ExternalSandbox { network_access } => SandboxPolicy::ExternalSandbox { - network_access: if merge_network_access( - network_access.is_enabled(), - additional_permissions, - ) { - NetworkAccess::Enabled - } else { - NetworkAccess::Restricted - }, - }, - SandboxPolicy::WorkspaceWrite { - writable_roots, - read_only_access, - network_access, - exclude_tmpdir_env_var, - exclude_slash_tmp, - } => { - let mut merged_writes = writable_roots.clone(); - merged_writes.extend(extra_writes); - SandboxPolicy::WorkspaceWrite { - writable_roots: dedup_absolute_paths(merged_writes), - read_only_access: merge_read_only_access_with_additional_reads( - read_only_access, - extra_reads, - ), - network_access: merge_network_access(*network_access, additional_permissions), - exclude_tmpdir_env_var: *exclude_tmpdir_env_var, - exclude_slash_tmp: *exclude_slash_tmp, - } - } - SandboxPolicy::ReadOnly { - access, - network_access, - } => { - if extra_writes.is_empty() { - SandboxPolicy::ReadOnly { - access: merge_read_only_access_with_additional_reads(access, extra_reads), - network_access: merge_network_access(*network_access, additional_permissions), - } - } else { - // todo(dylan) - for now, this grants more access than the request. We should restrict this, - // but we should add a new SandboxPolicy variant to handle this. While the feature is still - // UnderDevelopment, it's a useful approximation of the desired behavior. - SandboxPolicy::WorkspaceWrite { - writable_roots: dedup_absolute_paths(extra_writes), - read_only_access: merge_read_only_access_with_additional_reads( - access, - extra_reads, - ), - network_access: merge_network_access(*network_access, additional_permissions), - exclude_tmpdir_env_var: false, - exclude_slash_tmp: false, - } - } - } - } -} - -pub(crate) fn should_require_platform_sandbox( - file_system_policy: &FileSystemSandboxPolicy, - network_policy: NetworkSandboxPolicy, - has_managed_network_requirements: bool, -) -> bool { - if has_managed_network_requirements { - return true; - } - - if !network_policy.is_enabled() { - return !matches!( - file_system_policy.kind, - FileSystemSandboxKind::ExternalSandbox - ); - } - - match file_system_policy.kind { - FileSystemSandboxKind::Restricted => !file_system_policy.has_full_disk_write_access(), - FileSystemSandboxKind::Unrestricted | FileSystemSandboxKind::ExternalSandbox => false, - } -} - #[derive(Default)] pub struct SandboxManager; @@ -609,30 +182,12 @@ impl SandboxManager { macos_seatbelt_profile_extensions, additional_permissions.as_ref(), ); - let (effective_file_system_policy, effective_network_policy) = - if let Some(additional_permissions) = additional_permissions { - let (extra_reads, extra_writes) = - additional_permission_roots(&additional_permissions); - let file_system_sandbox_policy = - if extra_reads.is_empty() && extra_writes.is_empty() { - file_system_policy.clone() - } else { - merge_file_system_policy_with_additional_permissions( - file_system_policy, - extra_reads, - extra_writes, - ) - }; - let network_sandbox_policy = - if merge_network_access(network_policy.is_enabled(), &additional_permissions) { - NetworkSandboxPolicy::Enabled - } else { - NetworkSandboxPolicy::Restricted - }; - (file_system_sandbox_policy, network_sandbox_policy) - } else { - (file_system_policy.clone(), network_policy) - }; + let effective_file_system_policy = effective_file_system_sandbox_policy( + file_system_policy, + additional_permissions.as_ref(), + ); + let effective_network_policy = + effective_network_sandbox_policy(network_policy, additional_permissions.as_ref()); let mut env = spec.env; if !effective_network_policy.is_enabled() { env.insert( diff --git a/codex-rs/core/src/sandboxing/mod_tests.rs b/codex-rs/core/src/sandboxing/mod_tests.rs index 9a7a34e49..62fa4e5ba 100644 --- a/codex-rs/core/src/sandboxing/mod_tests.rs +++ b/codex-rs/core/src/sandboxing/mod_tests.rs @@ -1,13 +1,4 @@ -#[cfg(target_os = "macos")] -use super::EffectiveSandboxPermissions; use super::SandboxManager; -use super::effective_file_system_sandbox_policy; -#[cfg(target_os = "macos")] -use super::intersect_permission_profiles; -use super::merge_file_system_policy_with_additional_permissions; -use super::normalize_additional_permissions; -use super::sandbox_policy_with_additional_permissions; -use super::should_require_platform_sandbox; use crate::exec::SandboxType; use crate::protocol::NetworkAccess; use crate::protocol::ReadOnlyAccess; @@ -15,14 +6,6 @@ use crate::protocol::SandboxPolicy; use crate::tools::sandboxing::SandboxablePreference; use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::models::FileSystemPermissions; -#[cfg(target_os = "macos")] -use codex_protocol::models::MacOsAutomationPermission; -#[cfg(target_os = "macos")] -use codex_protocol::models::MacOsContactsPermission; -#[cfg(target_os = "macos")] -use codex_protocol::models::MacOsPreferencesPermission; -#[cfg(target_os = "macos")] -use codex_protocol::models::MacOsSeatbeltProfileExtensions; use codex_protocol::models::NetworkPermissions; use codex_protocol::models::PermissionProfile; use codex_protocol::permissions::FileSystemAccessMode; @@ -35,15 +18,8 @@ use codex_utils_absolute_path::AbsolutePathBuf; use dunce::canonicalize; use pretty_assertions::assert_eq; use std::collections::HashMap; -#[cfg(unix)] -use std::path::Path; use tempfile::TempDir; -#[cfg(unix)] -fn symlink_dir(original: &Path, link: &Path) -> std::io::Result<()> { - std::os::unix::fs::symlink(original, link) -} - #[test] fn danger_full_access_defaults_to_no_sandbox_without_network_requirements() { let manager = SandboxManager::new(); @@ -90,62 +66,6 @@ fn restricted_file_system_uses_platform_sandbox_without_managed_network() { assert_eq!(sandbox, expected); } -#[test] -fn full_access_restricted_policy_skips_platform_sandbox_when_network_is_enabled() { - let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { - path: FileSystemPath::Special { - value: FileSystemSpecialPath::Root, - }, - access: FileSystemAccessMode::Write, - }]); - - assert_eq!( - should_require_platform_sandbox(&policy, NetworkSandboxPolicy::Enabled, false), - false - ); -} - -#[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 { - path: FileSystemPath::Special { - value: FileSystemSpecialPath::Root, - }, - access: FileSystemAccessMode::Write, - }]); - - assert_eq!( - should_require_platform_sandbox(&policy, NetworkSandboxPolicy::Restricted, false), - true - ); -} - #[test] fn transform_preserves_unrestricted_file_system_policy_for_restricted_network() { let manager = SandboxManager::new(); @@ -191,317 +111,6 @@ fn transform_preserves_unrestricted_file_system_policy_for_restricted_network() ); } -#[test] -fn normalize_additional_permissions_preserves_network() { - let temp_dir = TempDir::new().expect("create temp dir"); - let path = AbsolutePathBuf::from_absolute_path( - canonicalize(temp_dir.path()).expect("canonicalize temp dir"), - ) - .expect("absolute temp dir"); - let permissions = normalize_additional_permissions(PermissionProfile { - network: Some(NetworkPermissions { - enabled: Some(true), - }), - file_system: Some(FileSystemPermissions { - read: Some(vec![path.clone()]), - write: Some(vec![path.clone()]), - }), - ..Default::default() - }) - .expect("permissions"); - - assert_eq!( - permissions.network, - Some(NetworkPermissions { - enabled: Some(true), - }) - ); - assert_eq!( - permissions.file_system, - Some(FileSystemPermissions { - read: Some(vec![path.clone()]), - write: Some(vec![path]), - }) - ); -} - -#[cfg(unix)] -#[test] -fn normalize_additional_permissions_canonicalizes_symlinked_write_paths() { - let temp_dir = TempDir::new().expect("create temp dir"); - let real_root = temp_dir.path().join("real"); - let link_root = temp_dir.path().join("link"); - let write_dir = real_root.join("write"); - std::fs::create_dir_all(&write_dir).expect("create write dir"); - symlink_dir(&real_root, &link_root).expect("create symlinked root"); - - let link_write_dir = - AbsolutePathBuf::from_absolute_path(link_root.join("write")).expect("link write dir"); - let expected_write_dir = AbsolutePathBuf::from_absolute_path( - write_dir.canonicalize().expect("canonicalize write dir"), - ) - .expect("absolute canonical write dir"); - - let permissions = normalize_additional_permissions(PermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(vec![]), - write: Some(vec![link_write_dir]), - }), - ..Default::default() - }) - .expect("permissions"); - - assert_eq!( - permissions.file_system, - Some(FileSystemPermissions { - read: Some(vec![]), - write: Some(vec![expected_write_dir]), - }) - ); -} - -#[test] -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, - }), - macos: None, - }) - .expect("permissions"); - - assert_eq!(permissions, PermissionProfile::default()); -} - -#[cfg(target_os = "macos")] -#[test] -fn normalize_additional_permissions_preserves_default_macos_preferences_permission() { - let permissions = normalize_additional_permissions(PermissionProfile { - macos: Some(MacOsSeatbeltProfileExtensions::default()), - ..Default::default() - }) - .expect("permissions"); - - assert_eq!( - permissions, - PermissionProfile { - macos: Some(MacOsSeatbeltProfileExtensions::default()), - ..Default::default() - } - ); -} - -#[cfg(target_os = "macos")] -#[test] -fn intersect_permission_profiles_preserves_default_macos_grants() { - let requested = PermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(Vec::from(["/tmp/requested" - .try_into() - .expect("absolute path")])), - write: None, - }), - macos: Some(MacOsSeatbeltProfileExtensions { - macos_preferences: MacOsPreferencesPermission::ReadWrite, - macos_automation: MacOsAutomationPermission::BundleIds(vec![ - "com.apple.Notes".to_string(), - ]), - macos_launch_services: false, - macos_accessibility: true, - macos_calendar: true, - macos_reminders: false, - macos_contacts: MacOsContactsPermission::None, - }), - ..Default::default() - }; - let granted = PermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(Vec::new()), - write: None, - }), - macos: Some(MacOsSeatbeltProfileExtensions::default()), - ..Default::default() - }; - - assert_eq!( - intersect_permission_profiles(requested, granted), - PermissionProfile { - macos: Some(MacOsSeatbeltProfileExtensions::default()), - ..Default::default() - } - ); -} - -#[cfg(target_os = "macos")] -#[test] -fn normalize_additional_permissions_preserves_macos_permissions() { - let permissions = normalize_additional_permissions(PermissionProfile { - macos: Some(MacOsSeatbeltProfileExtensions { - macos_preferences: MacOsPreferencesPermission::ReadWrite, - macos_automation: MacOsAutomationPermission::BundleIds(vec![ - "com.apple.Notes".to_string(), - ]), - macos_launch_services: true, - macos_accessibility: true, - macos_calendar: true, - macos_reminders: false, - macos_contacts: MacOsContactsPermission::None, - }), - ..Default::default() - }) - .expect("permissions"); - - assert_eq!( - permissions.macos, - Some(MacOsSeatbeltProfileExtensions { - macos_preferences: MacOsPreferencesPermission::ReadWrite, - macos_automation: MacOsAutomationPermission::BundleIds(vec![ - "com.apple.Notes".to_string(), - ]), - macos_launch_services: true, - macos_accessibility: true, - macos_calendar: true, - macos_reminders: false, - macos_contacts: MacOsContactsPermission::None, - }) - ); -} - -#[test] -fn read_only_additional_permissions_can_enable_network_without_writes() { - let temp_dir = TempDir::new().expect("create temp dir"); - let path = AbsolutePathBuf::from_absolute_path( - canonicalize(temp_dir.path()).expect("canonicalize temp dir"), - ) - .expect("absolute temp dir"); - let policy = sandbox_policy_with_additional_permissions( - &SandboxPolicy::ReadOnly { - access: ReadOnlyAccess::Restricted { - include_platform_defaults: true, - readable_roots: vec![path.clone()], - }, - network_access: false, - }, - &PermissionProfile { - network: Some(NetworkPermissions { - enabled: Some(true), - }), - file_system: Some(FileSystemPermissions { - read: Some(vec![path.clone()]), - write: Some(Vec::new()), - }), - ..Default::default() - }, - ); - - assert_eq!( - policy, - SandboxPolicy::ReadOnly { - access: ReadOnlyAccess::Restricted { - include_platform_defaults: true, - readable_roots: vec![path], - }, - network_access: true, - } - ); -} -#[cfg(target_os = "macos")] -#[test] -fn effective_permissions_merge_macos_extensions_with_additional_permissions() { - let temp_dir = TempDir::new().expect("create temp dir"); - let path = AbsolutePathBuf::from_absolute_path( - canonicalize(temp_dir.path()).expect("canonicalize temp dir"), - ) - .expect("absolute temp dir"); - let effective_permissions = EffectiveSandboxPermissions::new( - &SandboxPolicy::ReadOnly { - access: ReadOnlyAccess::Restricted { - include_platform_defaults: true, - readable_roots: vec![path.clone()], - }, - network_access: false, - }, - Some(&MacOsSeatbeltProfileExtensions { - macos_preferences: MacOsPreferencesPermission::ReadOnly, - macos_automation: MacOsAutomationPermission::BundleIds(vec![ - "com.apple.Calendar".to_string(), - ]), - macos_launch_services: false, - macos_accessibility: false, - macos_calendar: false, - macos_reminders: false, - macos_contacts: MacOsContactsPermission::None, - }), - Some(&PermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(vec![path]), - write: Some(Vec::new()), - }), - macos: Some(MacOsSeatbeltProfileExtensions { - macos_preferences: MacOsPreferencesPermission::ReadWrite, - macos_automation: MacOsAutomationPermission::BundleIds(vec![ - "com.apple.Notes".to_string(), - ]), - macos_launch_services: true, - macos_accessibility: true, - macos_calendar: true, - macos_reminders: false, - macos_contacts: MacOsContactsPermission::None, - }), - ..Default::default() - }), - ); - - assert_eq!( - effective_permissions.macos_seatbelt_profile_extensions, - Some(MacOsSeatbeltProfileExtensions { - macos_preferences: MacOsPreferencesPermission::ReadWrite, - macos_automation: MacOsAutomationPermission::BundleIds(vec![ - "com.apple.Calendar".to_string(), - "com.apple.Notes".to_string(), - ]), - macos_launch_services: true, - macos_accessibility: true, - macos_calendar: true, - macos_reminders: false, - macos_contacts: MacOsContactsPermission::None, - }) - ); -} - -#[test] -fn external_sandbox_additional_permissions_can_enable_network() { - let temp_dir = TempDir::new().expect("create temp dir"); - let path = AbsolutePathBuf::from_absolute_path( - canonicalize(temp_dir.path()).expect("canonicalize temp dir"), - ) - .expect("absolute temp dir"); - let policy = sandbox_policy_with_additional_permissions( - &SandboxPolicy::ExternalSandbox { - network_access: NetworkAccess::Restricted, - }, - &PermissionProfile { - network: Some(NetworkPermissions { - enabled: Some(true), - }), - file_system: Some(FileSystemPermissions { - read: Some(vec![path]), - write: Some(Vec::new()), - }), - ..Default::default() - }, - ); - - assert_eq!( - policy, - SandboxPolicy::ExternalSandbox { - network_access: NetworkAccess::Enabled, - } - ); -} - #[test] fn transform_additional_permissions_enable_network_for_external_sandbox() { let manager = SandboxManager::new(); @@ -649,123 +258,3 @@ fn transform_additional_permissions_preserves_denied_entries() { NetworkSandboxPolicy::Restricted ); } - -#[test] -fn merge_file_system_policy_with_additional_permissions_preserves_unreadable_roots() { - let temp_dir = TempDir::new().expect("create temp dir"); - let cwd = AbsolutePathBuf::from_absolute_path( - canonicalize(temp_dir.path()).expect("canonicalize temp dir"), - ) - .expect("absolute temp dir"); - let allowed_path = cwd.join("allowed").expect("allowed path"); - let denied_path = cwd.join("denied").expect("denied path"); - let merged_policy = merge_file_system_policy_with_additional_permissions( - &FileSystemSandboxPolicy::restricted(vec![ - FileSystemSandboxEntry { - path: FileSystemPath::Special { - value: FileSystemSpecialPath::Root, - }, - access: FileSystemAccessMode::Read, - }, - FileSystemSandboxEntry { - path: FileSystemPath::Path { - path: denied_path.clone(), - }, - access: FileSystemAccessMode::None, - }, - ]), - vec![allowed_path.clone()], - Vec::new(), - ); - - assert_eq!( - merged_policy.entries.contains(&FileSystemSandboxEntry { - path: FileSystemPath::Path { path: denied_path }, - access: FileSystemAccessMode::None, - }), - true - ); - assert_eq!( - merged_policy.entries.contains(&FileSystemSandboxEntry { - path: FileSystemPath::Path { path: allowed_path }, - access: FileSystemAccessMode::Read, - }), - true - ); -} - -#[test] -fn effective_file_system_sandbox_policy_returns_base_policy_without_additional_permissions() { - let temp_dir = TempDir::new().expect("create temp dir"); - let cwd = AbsolutePathBuf::from_absolute_path( - canonicalize(temp_dir.path()).expect("canonicalize temp dir"), - ) - .expect("absolute temp dir"); - let denied_path = cwd.join("denied").expect("denied path"); - let base_policy = FileSystemSandboxPolicy::restricted(vec![ - FileSystemSandboxEntry { - path: FileSystemPath::Special { - value: FileSystemSpecialPath::Root, - }, - access: FileSystemAccessMode::Read, - }, - FileSystemSandboxEntry { - path: FileSystemPath::Path { path: denied_path }, - access: FileSystemAccessMode::None, - }, - ]); - - let effective_policy = effective_file_system_sandbox_policy(&base_policy, None); - - assert_eq!(effective_policy, base_policy); -} - -#[test] -fn effective_file_system_sandbox_policy_merges_additional_write_roots() { - let temp_dir = TempDir::new().expect("create temp dir"); - let cwd = AbsolutePathBuf::from_absolute_path( - canonicalize(temp_dir.path()).expect("canonicalize temp dir"), - ) - .expect("absolute temp dir"); - let allowed_path = cwd.join("allowed").expect("allowed path"); - let denied_path = cwd.join("denied").expect("denied path"); - let base_policy = FileSystemSandboxPolicy::restricted(vec![ - FileSystemSandboxEntry { - path: FileSystemPath::Special { - value: FileSystemSpecialPath::Root, - }, - access: FileSystemAccessMode::Read, - }, - FileSystemSandboxEntry { - path: FileSystemPath::Path { - path: denied_path.clone(), - }, - access: FileSystemAccessMode::None, - }, - ]); - let additional_permissions = PermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(vec![]), - write: Some(vec![allowed_path.clone()]), - }), - ..Default::default() - }; - - let effective_policy = - effective_file_system_sandbox_policy(&base_policy, Some(&additional_permissions)); - - assert_eq!( - effective_policy.entries.contains(&FileSystemSandboxEntry { - path: FileSystemPath::Path { path: denied_path }, - access: FileSystemAccessMode::None, - }), - true - ); - assert_eq!( - effective_policy.entries.contains(&FileSystemSandboxEntry { - path: FileSystemPath::Path { path: allowed_path }, - access: FileSystemAccessMode::Write, - }), - true - ); -} diff --git a/codex-rs/core/src/state/session.rs b/codex-rs/core/src/state/session.rs index 563e8b340..81978160a 100644 --- a/codex-rs/core/src/state/session.rs +++ b/codex-rs/core/src/state/session.rs @@ -2,6 +2,7 @@ use codex_protocol::models::PermissionProfile; use codex_protocol::models::ResponseItem; +use codex_sandboxing::policy_transforms::merge_permission_profiles; use std::collections::HashMap; use std::collections::HashSet; @@ -11,7 +12,6 @@ use crate::context_manager::ContextManager; use crate::protocol::RateLimitSnapshot; use crate::protocol::TokenUsage; use crate::protocol::TokenUsageInfo; -use crate::sandboxing::merge_permission_profiles; use crate::session_startup_prewarm::SessionStartupPrewarmHandle; use crate::truncate::TruncationPolicy; use codex_protocol::protocol::TurnContextItem; diff --git a/codex-rs/core/src/state/turn.rs b/codex-rs/core/src/state/turn.rs index a6ae1ba8c..07d040ca2 100644 --- a/codex-rs/core/src/state/turn.rs +++ b/codex-rs/core/src/state/turn.rs @@ -1,5 +1,6 @@ //! Turn-scoped state and active turn metadata scaffolding. +use codex_sandboxing::policy_transforms::merge_permission_profiles; use indexmap::IndexMap; use std::collections::HashMap; use std::sync::Arc; @@ -19,7 +20,6 @@ use tokio::sync::oneshot; use crate::codex::TurnContext; use crate::protocol::ReviewDecision; use crate::protocol::TokenUsage; -use crate::sandboxing::merge_permission_profiles; use crate::tasks::SessionTask; use codex_protocol::models::PermissionProfile; diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 1d799da7e..bbe3572c2 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -11,8 +11,6 @@ use crate::client_common::tools::ToolSpec; use crate::codex::Session; use crate::codex::TurnContext; use crate::function_tool::FunctionCallError; -use crate::sandboxing::effective_file_system_sandbox_policy; -use crate::sandboxing::merge_permission_profiles; use crate::tools::context::ApplyPatchToolOutput; use crate::tools::context::FunctionToolOutput; use crate::tools::context::SharedTurnDiffTracker; @@ -35,6 +33,9 @@ use codex_apply_patch::ApplyPatchAction; use codex_apply_patch::ApplyPatchFileChange; use codex_protocol::models::FileSystemPermissions; use codex_protocol::models::PermissionProfile; +use codex_sandboxing::policy_transforms::effective_file_system_sandbox_policy; +use codex_sandboxing::policy_transforms::merge_permission_profiles; +use codex_sandboxing::policy_transforms::normalize_additional_permissions; use codex_utils_absolute_path::AbsolutePathBuf; use std::collections::BTreeSet; use std::sync::Arc; @@ -89,7 +90,7 @@ fn write_permissions_for_paths(file_paths: &[AbsolutePathBuf]) -> Option { - crate::sandboxing::intersect_permission_profiles( - effective_permissions.clone(), - granted_permissions, - ) == *effective_permissions + intersect_permission_profiles(effective_permissions.clone(), granted_permissions) + == *effective_permissions } _ => false, }; diff --git a/codex-rs/core/src/tools/handlers/request_permissions.rs b/codex-rs/core/src/tools/handlers/request_permissions.rs index 2ba41ed9a..4df8328e7 100644 --- a/codex-rs/core/src/tools/handlers/request_permissions.rs +++ b/codex-rs/core/src/tools/handlers/request_permissions.rs @@ -1,8 +1,8 @@ use async_trait::async_trait; use codex_protocol::request_permissions::RequestPermissionsArgs; +use codex_sandboxing::policy_transforms::normalize_additional_permissions; use crate::function_tool::FunctionCallError; -use crate::sandboxing::normalize_additional_permissions; use crate::tools::context::FunctionToolOutput; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; diff --git a/codex-rs/sandboxing/Cargo.toml b/codex-rs/sandboxing/Cargo.toml index 0e2ab443a..fc340d650 100644 --- a/codex-rs/sandboxing/Cargo.toml +++ b/codex-rs/sandboxing/Cargo.toml @@ -16,6 +16,7 @@ codex-network-proxy = { workspace = true } codex-protocol = { workspace = true } codex-utils-absolute-path = { workspace = true } dirs = { workspace = true } +dunce = { workspace = true } libc = { workspace = true } serde_json = { workspace = true } tracing = { workspace = true, features = ["log"] } diff --git a/codex-rs/sandboxing/src/lib.rs b/codex-rs/sandboxing/src/lib.rs index 17291ec2a..edd817e65 100644 --- a/codex-rs/sandboxing/src/lib.rs +++ b/codex-rs/sandboxing/src/lib.rs @@ -1,5 +1,6 @@ pub mod landlock; pub mod macos_permissions; +pub mod policy_transforms; #[cfg(target_os = "macos")] pub mod seatbelt; #[cfg(target_os = "macos")] diff --git a/codex-rs/sandboxing/src/policy_transforms.rs b/codex-rs/sandboxing/src/policy_transforms.rs new file mode 100644 index 000000000..045c7e3f9 --- /dev/null +++ b/codex-rs/sandboxing/src/policy_transforms.rs @@ -0,0 +1,463 @@ +use crate::macos_permissions::intersect_macos_seatbelt_profile_extensions; +use crate::macos_permissions::merge_macos_seatbelt_profile_extensions; +use codex_protocol::models::FileSystemPermissions; +use codex_protocol::models::MacOsSeatbeltProfileExtensions; +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; +use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::NetworkSandboxPolicy; +use codex_protocol::protocol::NetworkAccess; +use codex_protocol::protocol::ReadOnlyAccess; +use codex_protocol::protocol::SandboxPolicy; +use codex_utils_absolute_path::AbsolutePathBuf; +use dunce::canonicalize; +use std::collections::HashSet; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct EffectiveSandboxPermissions { + pub sandbox_policy: SandboxPolicy, + pub macos_seatbelt_profile_extensions: Option, +} + +impl EffectiveSandboxPermissions { + pub fn new( + sandbox_policy: &SandboxPolicy, + macos_seatbelt_profile_extensions: Option<&MacOsSeatbeltProfileExtensions>, + additional_permissions: Option<&PermissionProfile>, + ) -> Self { + let Some(additional_permissions) = additional_permissions else { + return Self { + sandbox_policy: sandbox_policy.clone(), + macos_seatbelt_profile_extensions: macos_seatbelt_profile_extensions.cloned(), + }; + }; + + Self { + sandbox_policy: effective_sandbox_policy(sandbox_policy, Some(additional_permissions)), + macos_seatbelt_profile_extensions: merge_macos_seatbelt_profile_extensions( + macos_seatbelt_profile_extensions, + additional_permissions.macos.as_ref(), + ), + } + } +} + +pub fn normalize_additional_permissions( + additional_permissions: PermissionProfile, +) -> Result { + let network = additional_permissions + .network + .filter(|network| !network.is_empty()); + 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 } + }) + .filter(|file_system| !file_system.is_empty()); + let macos = additional_permissions.macos; + + Ok(PermissionProfile { + network, + file_system, + macos, + }) +} + +pub fn merge_permission_profiles( + base: Option<&PermissionProfile>, + permissions: Option<&PermissionProfile>, +) -> Option { + let Some(permissions) = permissions else { + return base.cloned(); + }; + + match base { + Some(base) => { + let network = match (base.network.as_ref(), permissions.network.as_ref()) { + ( + Some(NetworkPermissions { + enabled: Some(true), + }), + _, + ) + | ( + _, + Some(NetworkPermissions { + enabled: Some(true), + }), + ) => Some(NetworkPermissions { + enabled: Some(true), + }), + _ => None, + }; + 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()), + }) + .filter(|file_system| !file_system.is_empty()), + (Some(base), None) => Some(base.clone()), + (None, Some(permissions)) => Some(permissions.clone()), + (None, None) => None, + }; + let macos = merge_macos_seatbelt_profile_extensions( + base.macos.as_ref(), + permissions.macos.as_ref(), + ); + + Some(PermissionProfile { + network, + file_system, + macos, + }) + .filter(|permissions| !permissions.is_empty()) + } + None => Some(permissions.clone()).filter(|permissions| !permissions.is_empty()), + } +} + +pub fn intersect_permission_profiles( + requested: PermissionProfile, + granted: PermissionProfile, +) -> PermissionProfile { + let file_system = requested + .file_system + .map(|requested_file_system| { + let granted_file_system = granted.file_system.unwrap_or_default(); + let read = requested_file_system + .read + .map(|requested_read| { + let granted_read = granted_file_system.read.unwrap_or_default(); + requested_read + .into_iter() + .filter(|path| granted_read.contains(path)) + .collect() + }) + .filter(|paths: &Vec<_>| !paths.is_empty()); + let write = requested_file_system + .write + .map(|requested_write| { + let granted_write = granted_file_system.write.unwrap_or_default(); + requested_write + .into_iter() + .filter(|path| granted_write.contains(path)) + .collect() + }) + .filter(|paths: &Vec<_>| !paths.is_empty()); + FileSystemPermissions { read, write } + }) + .filter(|file_system| !file_system.is_empty()); + let network = match (requested.network, granted.network) { + ( + Some(NetworkPermissions { + enabled: Some(true), + }), + Some(NetworkPermissions { + enabled: Some(true), + }), + ) => Some(NetworkPermissions { + enabled: Some(true), + }), + _ => None, + }; + + let macos = intersect_macos_seatbelt_profile_extensions(requested.macos, granted.macos); + + PermissionProfile { + network, + file_system, + macos, + } +} + +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(path.as_path()) + .ok() + .and_then(|path| AbsolutePathBuf::from_absolute_path(path).ok()) + .unwrap_or(path); + if seen.insert(canonicalized.clone()) { + out.push(canonicalized); + } + } + + 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, + } +} + +fn dedup_absolute_paths(paths: Vec) -> Vec { + let mut out = Vec::with_capacity(paths.len()); + let mut seen = HashSet::new(); + for path in paths { + if seen.insert(path.to_path_buf()) { + out.push(path); + } + } + out +} + +fn additional_permission_roots( + additional_permissions: &PermissionProfile, +) -> (Vec, Vec) { + ( + dedup_absolute_paths( + additional_permissions + .file_system + .as_ref() + .and_then(|file_system| file_system.read.clone()) + .unwrap_or_default(), + ), + dedup_absolute_paths( + additional_permissions + .file_system + .as_ref() + .and_then(|file_system| file_system.write.clone()) + .unwrap_or_default(), + ), + ) +} + +fn merge_file_system_policy_with_additional_permissions( + file_system_policy: &FileSystemSandboxPolicy, + extra_reads: Vec, + extra_writes: Vec, +) -> 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); + } + } + merged_policy + } + FileSystemSandboxKind::Unrestricted | FileSystemSandboxKind::ExternalSandbox => { + file_system_policy.clone() + } + } +} + +pub fn effective_file_system_sandbox_policy( + file_system_policy: &FileSystemSandboxPolicy, + additional_permissions: Option<&PermissionProfile>, +) -> FileSystemSandboxPolicy { + let Some(additional_permissions) = additional_permissions else { + return file_system_policy.clone(); + }; + + let (extra_reads, extra_writes) = additional_permission_roots(additional_permissions); + if extra_reads.is_empty() && extra_writes.is_empty() { + file_system_policy.clone() + } else { + merge_file_system_policy_with_additional_permissions( + file_system_policy, + extra_reads, + extra_writes, + ) + } +} + +fn merge_read_only_access_with_additional_reads( + read_only_access: &ReadOnlyAccess, + extra_reads: Vec, +) -> ReadOnlyAccess { + match read_only_access { + ReadOnlyAccess::FullAccess => ReadOnlyAccess::FullAccess, + ReadOnlyAccess::Restricted { + include_platform_defaults, + readable_roots, + } => { + let mut merged = readable_roots.clone(); + merged.extend(extra_reads); + ReadOnlyAccess::Restricted { + include_platform_defaults: *include_platform_defaults, + readable_roots: dedup_absolute_paths(merged), + } + } + } +} + +fn merge_network_access( + base_network_access: bool, + additional_permissions: &PermissionProfile, +) -> bool { + base_network_access + || additional_permissions + .network + .as_ref() + .and_then(|network| network.enabled) + .unwrap_or(false) +} + +pub fn effective_network_sandbox_policy( + network_policy: NetworkSandboxPolicy, + additional_permissions: Option<&PermissionProfile>, +) -> NetworkSandboxPolicy { + if additional_permissions + .is_some_and(|permissions| merge_network_access(network_policy.is_enabled(), permissions)) + { + NetworkSandboxPolicy::Enabled + } else if additional_permissions.is_some() { + NetworkSandboxPolicy::Restricted + } else { + network_policy + } +} + +fn sandbox_policy_with_additional_permissions( + sandbox_policy: &SandboxPolicy, + additional_permissions: &PermissionProfile, +) -> SandboxPolicy { + if additional_permissions.is_empty() { + return sandbox_policy.clone(); + } + + let (extra_reads, extra_writes) = additional_permission_roots(additional_permissions); + + match sandbox_policy { + SandboxPolicy::DangerFullAccess => SandboxPolicy::DangerFullAccess, + SandboxPolicy::ExternalSandbox { network_access } => SandboxPolicy::ExternalSandbox { + network_access: if merge_network_access( + network_access.is_enabled(), + additional_permissions, + ) { + NetworkAccess::Enabled + } else { + NetworkAccess::Restricted + }, + }, + SandboxPolicy::WorkspaceWrite { + writable_roots, + read_only_access, + network_access, + exclude_tmpdir_env_var, + exclude_slash_tmp, + } => { + let mut merged_writes = writable_roots.clone(); + merged_writes.extend(extra_writes); + SandboxPolicy::WorkspaceWrite { + writable_roots: dedup_absolute_paths(merged_writes), + read_only_access: merge_read_only_access_with_additional_reads( + read_only_access, + extra_reads, + ), + network_access: merge_network_access(*network_access, additional_permissions), + exclude_tmpdir_env_var: *exclude_tmpdir_env_var, + exclude_slash_tmp: *exclude_slash_tmp, + } + } + SandboxPolicy::ReadOnly { + access, + network_access, + } => { + if extra_writes.is_empty() { + SandboxPolicy::ReadOnly { + access: merge_read_only_access_with_additional_reads(access, extra_reads), + network_access: merge_network_access(*network_access, additional_permissions), + } + } else { + // todo(dylan) - for now, this grants more access than the request. We should restrict this, + // but we should add a new SandboxPolicy variant to handle this. While the feature is still + // UnderDevelopment, it's a useful approximation of the desired behavior. + SandboxPolicy::WorkspaceWrite { + writable_roots: dedup_absolute_paths(extra_writes), + read_only_access: merge_read_only_access_with_additional_reads( + access, + extra_reads, + ), + network_access: merge_network_access(*network_access, additional_permissions), + exclude_tmpdir_env_var: false, + exclude_slash_tmp: false, + } + } + } + } +} + +fn effective_sandbox_policy( + sandbox_policy: &SandboxPolicy, + additional_permissions: Option<&PermissionProfile>, +) -> SandboxPolicy { + additional_permissions.map_or_else( + || sandbox_policy.clone(), + |permissions| sandbox_policy_with_additional_permissions(sandbox_policy, permissions), + ) +} + +pub fn should_require_platform_sandbox( + file_system_policy: &FileSystemSandboxPolicy, + network_policy: NetworkSandboxPolicy, + has_managed_network_requirements: bool, +) -> bool { + if has_managed_network_requirements { + return true; + } + + if !network_policy.is_enabled() { + return !matches!( + file_system_policy.kind, + FileSystemSandboxKind::ExternalSandbox + ); + } + + match file_system_policy.kind { + FileSystemSandboxKind::Restricted => !file_system_policy.has_full_disk_write_access(), + FileSystemSandboxKind::Unrestricted | FileSystemSandboxKind::ExternalSandbox => false, + } +} + +#[cfg(test)] +#[path = "policy_transforms_tests.rs"] +mod tests; diff --git a/codex-rs/sandboxing/src/policy_transforms_tests.rs b/codex-rs/sandboxing/src/policy_transforms_tests.rs new file mode 100644 index 000000000..a9c759a20 --- /dev/null +++ b/codex-rs/sandboxing/src/policy_transforms_tests.rs @@ -0,0 +1,528 @@ +#[cfg(target_os = "macos")] +use super::EffectiveSandboxPermissions; +use super::effective_file_system_sandbox_policy; +#[cfg(target_os = "macos")] +use super::intersect_permission_profiles; +use super::merge_file_system_policy_with_additional_permissions; +use super::normalize_additional_permissions; +use super::sandbox_policy_with_additional_permissions; +use super::should_require_platform_sandbox; +use codex_protocol::models::FileSystemPermissions; +#[cfg(target_os = "macos")] +use codex_protocol::models::MacOsAutomationPermission; +#[cfg(target_os = "macos")] +use codex_protocol::models::MacOsContactsPermission; +#[cfg(target_os = "macos")] +use codex_protocol::models::MacOsPreferencesPermission; +#[cfg(target_os = "macos")] +use codex_protocol::models::MacOsSeatbeltProfileExtensions; +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::FileSystemSandboxPolicy; +use codex_protocol::permissions::FileSystemSpecialPath; +use codex_protocol::permissions::NetworkSandboxPolicy; +use codex_protocol::protocol::NetworkAccess; +use codex_protocol::protocol::ReadOnlyAccess; +use codex_protocol::protocol::SandboxPolicy; +use codex_utils_absolute_path::AbsolutePathBuf; +use dunce::canonicalize; +use pretty_assertions::assert_eq; +#[cfg(unix)] +use std::path::Path; +use tempfile::TempDir; + +#[cfg(unix)] +fn symlink_dir(original: &Path, link: &Path) -> std::io::Result<()> { + std::os::unix::fs::symlink(original, link) +} + +#[test] +fn full_access_restricted_policy_skips_platform_sandbox_when_network_is_enabled() { + let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Write, + }]); + + assert_eq!( + should_require_platform_sandbox(&policy, NetworkSandboxPolicy::Enabled, false), + false + ); +} + +#[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 { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Write, + }]); + + assert_eq!( + should_require_platform_sandbox(&policy, NetworkSandboxPolicy::Restricted, false), + true + ); +} + +#[test] +fn normalize_additional_permissions_preserves_network() { + let temp_dir = TempDir::new().expect("create temp dir"); + let path = AbsolutePathBuf::from_absolute_path( + canonicalize(temp_dir.path()).expect("canonicalize temp dir"), + ) + .expect("absolute temp dir"); + let permissions = normalize_additional_permissions(PermissionProfile { + network: Some(NetworkPermissions { + enabled: Some(true), + }), + file_system: Some(FileSystemPermissions { + read: Some(vec![path.clone()]), + write: Some(vec![path.clone()]), + }), + ..Default::default() + }) + .expect("permissions"); + + assert_eq!( + permissions.network, + Some(NetworkPermissions { + enabled: Some(true), + }) + ); + assert_eq!( + permissions.file_system, + Some(FileSystemPermissions { + read: Some(vec![path.clone()]), + write: Some(vec![path]), + }) + ); +} + +#[cfg(unix)] +#[test] +fn normalize_additional_permissions_canonicalizes_symlinked_write_paths() { + let temp_dir = TempDir::new().expect("create temp dir"); + let real_root = temp_dir.path().join("real"); + let link_root = temp_dir.path().join("link"); + let write_dir = real_root.join("write"); + std::fs::create_dir_all(&write_dir).expect("create write dir"); + symlink_dir(&real_root, &link_root).expect("create symlinked root"); + + let link_write_dir = + AbsolutePathBuf::from_absolute_path(link_root.join("write")).expect("link write dir"); + let expected_write_dir = AbsolutePathBuf::from_absolute_path( + write_dir.canonicalize().expect("canonicalize write dir"), + ) + .expect("absolute canonical write dir"); + + let permissions = normalize_additional_permissions(PermissionProfile { + file_system: Some(FileSystemPermissions { + read: Some(vec![]), + write: Some(vec![link_write_dir]), + }), + ..Default::default() + }) + .expect("permissions"); + + assert_eq!( + permissions.file_system, + Some(FileSystemPermissions { + read: Some(vec![]), + write: Some(vec![expected_write_dir]), + }) + ); +} + +#[test] +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, + }), + macos: None, + }) + .expect("permissions"); + + assert_eq!(permissions, PermissionProfile::default()); +} + +#[cfg(target_os = "macos")] +#[test] +fn normalize_additional_permissions_preserves_default_macos_preferences_permission() { + let permissions = normalize_additional_permissions(PermissionProfile { + macos: Some(MacOsSeatbeltProfileExtensions::default()), + ..Default::default() + }) + .expect("permissions"); + + assert_eq!( + permissions, + PermissionProfile { + macos: Some(MacOsSeatbeltProfileExtensions::default()), + ..Default::default() + } + ); +} + +#[cfg(target_os = "macos")] +#[test] +fn intersect_permission_profiles_preserves_default_macos_grants() { + let requested = PermissionProfile { + file_system: Some(FileSystemPermissions { + read: Some(Vec::from(["/tmp/requested" + .try_into() + .expect("absolute path")])), + write: None, + }), + macos: Some(MacOsSeatbeltProfileExtensions { + macos_preferences: MacOsPreferencesPermission::ReadWrite, + macos_automation: MacOsAutomationPermission::BundleIds(vec![ + "com.apple.Notes".to_string(), + ]), + macos_launch_services: false, + macos_accessibility: true, + macos_calendar: true, + macos_reminders: false, + macos_contacts: MacOsContactsPermission::None, + }), + ..Default::default() + }; + let granted = PermissionProfile { + file_system: Some(FileSystemPermissions { + read: Some(Vec::new()), + write: None, + }), + macos: Some(MacOsSeatbeltProfileExtensions::default()), + ..Default::default() + }; + + assert_eq!( + intersect_permission_profiles(requested, granted), + PermissionProfile { + macos: Some(MacOsSeatbeltProfileExtensions::default()), + ..Default::default() + } + ); +} + +#[cfg(target_os = "macos")] +#[test] +fn normalize_additional_permissions_preserves_macos_permissions() { + let permissions = normalize_additional_permissions(PermissionProfile { + macos: Some(MacOsSeatbeltProfileExtensions { + macos_preferences: MacOsPreferencesPermission::ReadWrite, + macos_automation: MacOsAutomationPermission::BundleIds(vec![ + "com.apple.Notes".to_string(), + ]), + macos_launch_services: true, + macos_accessibility: true, + macos_calendar: true, + macos_reminders: false, + macos_contacts: MacOsContactsPermission::None, + }), + ..Default::default() + }) + .expect("permissions"); + + assert_eq!( + permissions.macos, + Some(MacOsSeatbeltProfileExtensions { + macos_preferences: MacOsPreferencesPermission::ReadWrite, + macos_automation: MacOsAutomationPermission::BundleIds(vec![ + "com.apple.Notes".to_string(), + ]), + macos_launch_services: true, + macos_accessibility: true, + macos_calendar: true, + macos_reminders: false, + macos_contacts: MacOsContactsPermission::None, + }) + ); +} + +#[test] +fn read_only_additional_permissions_can_enable_network_without_writes() { + let temp_dir = TempDir::new().expect("create temp dir"); + let path = AbsolutePathBuf::from_absolute_path( + canonicalize(temp_dir.path()).expect("canonicalize temp dir"), + ) + .expect("absolute temp dir"); + let policy = sandbox_policy_with_additional_permissions( + &SandboxPolicy::ReadOnly { + access: ReadOnlyAccess::Restricted { + include_platform_defaults: true, + readable_roots: vec![path.clone()], + }, + network_access: false, + }, + &PermissionProfile { + network: Some(NetworkPermissions { + enabled: Some(true), + }), + file_system: Some(FileSystemPermissions { + read: Some(vec![path.clone()]), + write: Some(Vec::new()), + }), + ..Default::default() + }, + ); + + assert_eq!( + policy, + SandboxPolicy::ReadOnly { + access: ReadOnlyAccess::Restricted { + include_platform_defaults: true, + readable_roots: vec![path], + }, + network_access: true, + } + ); +} + +#[cfg(target_os = "macos")] +#[test] +fn effective_permissions_merge_macos_extensions_with_additional_permissions() { + let temp_dir = TempDir::new().expect("create temp dir"); + let path = AbsolutePathBuf::from_absolute_path( + canonicalize(temp_dir.path()).expect("canonicalize temp dir"), + ) + .expect("absolute temp dir"); + let effective_permissions = EffectiveSandboxPermissions::new( + &SandboxPolicy::ReadOnly { + access: ReadOnlyAccess::Restricted { + include_platform_defaults: true, + readable_roots: vec![path.clone()], + }, + network_access: false, + }, + Some(&MacOsSeatbeltProfileExtensions { + macos_preferences: MacOsPreferencesPermission::ReadOnly, + macos_automation: MacOsAutomationPermission::BundleIds(vec![ + "com.apple.Calendar".to_string(), + ]), + macos_launch_services: false, + macos_accessibility: false, + macos_calendar: false, + macos_reminders: false, + macos_contacts: MacOsContactsPermission::None, + }), + Some(&PermissionProfile { + file_system: Some(FileSystemPermissions { + read: Some(vec![path]), + write: Some(Vec::new()), + }), + macos: Some(MacOsSeatbeltProfileExtensions { + macos_preferences: MacOsPreferencesPermission::ReadWrite, + macos_automation: MacOsAutomationPermission::BundleIds(vec![ + "com.apple.Notes".to_string(), + ]), + macos_launch_services: true, + macos_accessibility: true, + macos_calendar: true, + macos_reminders: false, + macos_contacts: MacOsContactsPermission::None, + }), + ..Default::default() + }), + ); + + assert_eq!( + effective_permissions.macos_seatbelt_profile_extensions, + Some(MacOsSeatbeltProfileExtensions { + macos_preferences: MacOsPreferencesPermission::ReadWrite, + macos_automation: MacOsAutomationPermission::BundleIds(vec![ + "com.apple.Calendar".to_string(), + "com.apple.Notes".to_string(), + ]), + macos_launch_services: true, + macos_accessibility: true, + macos_calendar: true, + macos_reminders: false, + macos_contacts: MacOsContactsPermission::None, + }) + ); +} + +#[test] +fn external_sandbox_additional_permissions_can_enable_network() { + let temp_dir = TempDir::new().expect("create temp dir"); + let path = AbsolutePathBuf::from_absolute_path( + canonicalize(temp_dir.path()).expect("canonicalize temp dir"), + ) + .expect("absolute temp dir"); + let policy = sandbox_policy_with_additional_permissions( + &SandboxPolicy::ExternalSandbox { + network_access: NetworkAccess::Restricted, + }, + &PermissionProfile { + network: Some(NetworkPermissions { + enabled: Some(true), + }), + file_system: Some(FileSystemPermissions { + read: Some(vec![path]), + write: Some(Vec::new()), + }), + ..Default::default() + }, + ); + + assert_eq!( + policy, + SandboxPolicy::ExternalSandbox { + network_access: NetworkAccess::Enabled, + } + ); +} + +#[test] +fn merge_file_system_policy_with_additional_permissions_preserves_unreadable_roots() { + let temp_dir = TempDir::new().expect("create temp dir"); + let cwd = AbsolutePathBuf::from_absolute_path( + canonicalize(temp_dir.path()).expect("canonicalize temp dir"), + ) + .expect("absolute temp dir"); + let allowed_path = cwd.join("allowed").expect("allowed path"); + let denied_path = cwd.join("denied").expect("denied path"); + let merged_policy = merge_file_system_policy_with_additional_permissions( + &FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: denied_path.clone(), + }, + access: FileSystemAccessMode::None, + }, + ]), + vec![allowed_path.clone()], + Vec::new(), + ); + + assert_eq!( + merged_policy.entries.contains(&FileSystemSandboxEntry { + path: FileSystemPath::Path { path: denied_path }, + access: FileSystemAccessMode::None, + }), + true + ); + assert_eq!( + merged_policy.entries.contains(&FileSystemSandboxEntry { + path: FileSystemPath::Path { path: allowed_path }, + access: FileSystemAccessMode::Read, + }), + true + ); +} + +#[test] +fn effective_file_system_sandbox_policy_returns_base_policy_without_additional_permissions() { + let temp_dir = TempDir::new().expect("create temp dir"); + let cwd = AbsolutePathBuf::from_absolute_path( + canonicalize(temp_dir.path()).expect("canonicalize temp dir"), + ) + .expect("absolute temp dir"); + let denied_path = cwd.join("denied").expect("denied path"); + let base_policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: denied_path }, + access: FileSystemAccessMode::None, + }, + ]); + + let effective_policy = effective_file_system_sandbox_policy(&base_policy, None); + + assert_eq!(effective_policy, base_policy); +} + +#[test] +fn effective_file_system_sandbox_policy_merges_additional_write_roots() { + let temp_dir = TempDir::new().expect("create temp dir"); + let cwd = AbsolutePathBuf::from_absolute_path( + canonicalize(temp_dir.path()).expect("canonicalize temp dir"), + ) + .expect("absolute temp dir"); + let allowed_path = cwd.join("allowed").expect("allowed path"); + let denied_path = cwd.join("denied").expect("denied path"); + let base_policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: denied_path.clone(), + }, + access: FileSystemAccessMode::None, + }, + ]); + let additional_permissions = PermissionProfile { + file_system: Some(FileSystemPermissions { + read: Some(vec![]), + write: Some(vec![allowed_path.clone()]), + }), + ..Default::default() + }; + + let effective_policy = + effective_file_system_sandbox_policy(&base_policy, Some(&additional_permissions)); + + assert_eq!( + effective_policy.entries.contains(&FileSystemSandboxEntry { + path: FileSystemPath::Path { path: denied_path }, + access: FileSystemAccessMode::None, + }), + true + ); + assert_eq!( + effective_policy.entries.contains(&FileSystemSandboxEntry { + path: FileSystemPath::Path { path: allowed_path }, + access: FileSystemAccessMode::Write, + }), + true + ); +}