From 9df485167121c50077cf84c7b0c81cf92b97b456 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 26 Mar 2026 15:21:46 -0700 Subject: [PATCH] permissions: start using PermissionProfile as the canonical runtime model --- .../app-server-protocol/src/protocol/v2.rs | 39 ++- .../app-server/src/bespoke_event_handling.rs | 24 +- codex-rs/core/src/codex.rs | 22 ++ codex-rs/core/src/codex_tests.rs | 49 ++++ codex-rs/core/src/config/config_tests.rs | 38 +++ codex-rs/core/src/config/mod.rs | 23 ++ .../core/src/tools/handlers/apply_patch.rs | 8 +- codex-rs/core/src/tools/handlers/mod.rs | 8 +- .../src/tools/handlers/multi_agents_common.rs | 2 + .../src/tools/handlers/multi_agents_tests.rs | 1 + .../src/tools/handlers/unified_exec_tests.rs | 8 +- .../runtimes/shell/unix_escalation_tests.rs | 16 +- .../core/tests/suite/request_permissions.rs | 144 +++++----- .../tests/suite/request_permissions_tool.rs | 16 +- codex-rs/protocol/src/models.rs | 265 +++++++++++++++++- codex-rs/protocol/src/permissions.rs | 7 +- codex-rs/sandboxing/src/manager_tests.rs | 16 +- codex-rs/sandboxing/src/policy_transforms.rs | 161 +++++------ .../sandboxing/src/policy_transforms_tests.rs | 88 +++--- .../tui/src/bottom_pane/approval_overlay.rs | 61 ++-- .../src/bottom_pane/approval_overlay.rs | 61 ++-- 21 files changed, 729 insertions(+), 328 deletions(-) diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 147c91db2c..4e389c3761 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -1070,19 +1070,30 @@ pub struct AdditionalFileSystemPermissions { impl From for AdditionalFileSystemPermissions { fn from(value: CoreFileSystemPermissions) -> Self { + let read = value + .explicit_path_entries() + .filter_map(|(path, access)| { + (access == codex_protocol::permissions::FileSystemAccessMode::Read) + .then_some(path.clone()) + }) + .collect::>(); + let write = value + .explicit_path_entries() + .filter_map(|(path, access)| { + (access == codex_protocol::permissions::FileSystemAccessMode::Write) + .then_some(path.clone()) + }) + .collect::>(); Self { - read: value.read, - write: value.write, + read: (!read.is_empty()).then_some(read), + write: (!write.is_empty()).then_some(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) } } @@ -6282,16 +6293,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"), ]), - }), + )), } ); } @@ -6375,16 +6386,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"), ]), - }), + )), macos: None, } ); diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index 15484d3d5f..ce833e686b 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -3088,10 +3088,10 @@ mod tests { network: Some(CoreNetworkPermissions { enabled: Some(true), }), - file_system: Some(CoreFileSystemPermissions { - read: Some(vec![absolute_path(input_path)]), - write: Some(vec![absolute_path(output_path)]), - }), + file_system: Some(CoreFileSystemPermissions::from_read_write_roots( + Some(vec![absolute_path(input_path)]), + Some(vec![absolute_path(output_path)]), + )), }; let cases = vec![ ( @@ -3118,10 +3118,10 @@ mod tests { }, }), CoreRequestPermissionProfile { - file_system: Some(CoreFileSystemPermissions { - read: None, - write: Some(vec![absolute_path(output_path)]), - }), + file_system: Some(CoreFileSystemPermissions::from_read_write_roots( + None, + Some(vec![absolute_path(output_path)]), + )), ..CoreRequestPermissionProfile::default() }, ), @@ -3136,10 +3136,10 @@ mod tests { }, }), CoreRequestPermissionProfile { - file_system: Some(CoreFileSystemPermissions { - read: Some(vec![absolute_path(input_path)]), - write: Some(vec![absolute_path(output_path)]), - }), + file_system: Some(CoreFileSystemPermissions::from_read_write_roots( + Some(vec![absolute_path(input_path)]), + Some(vec![absolute_path(output_path)]), + )), ..CoreRequestPermissionProfile::default() }, ), diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 3a27a8839b..0a9f08f346 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -618,6 +618,7 @@ impl Codex { compact_prompt: config.compact_prompt.clone(), approval_policy: config.permissions.approval_policy.clone(), approvals_reviewer: config.approvals_reviewer, + permission_profile: config.permissions.runtime_permission_profile(), sandbox_policy: config.permissions.sandbox_policy.clone(), file_system_sandbox_policy: config.permissions.file_system_sandbox_policy.clone(), network_sandbox_policy: config.permissions.network_sandbox_policy, @@ -857,6 +858,7 @@ pub(crate) struct TurnContext { pub(crate) collaboration_mode: CollaborationMode, pub(crate) personality: Option, pub(crate) approval_policy: Constrained, + pub(crate) permission_profile: PermissionProfile, pub(crate) sandbox_policy: Constrained, pub(crate) file_system_sandbox_policy: FileSystemSandboxPolicy, pub(crate) network_sandbox_policy: NetworkSandboxPolicy, @@ -965,6 +967,7 @@ impl TurnContext { collaboration_mode, personality: self.personality, approval_policy: self.approval_policy.clone(), + permission_profile: self.permission_profile.clone(), sandbox_policy: self.sandbox_policy.clone(), file_system_sandbox_policy: self.file_system_sandbox_policy.clone(), network_sandbox_policy: self.network_sandbox_policy, @@ -1073,6 +1076,7 @@ pub(crate) struct SessionConfiguration { /// When to escalate for approval for execution approval_policy: Constrained, approvals_reviewer: ApprovalsReviewer, + permission_profile: PermissionProfile, /// How to sandbox commands executed in the system sandbox_policy: Constrained, file_system_sandbox_policy: FileSystemSandboxPolicy, @@ -1184,6 +1188,11 @@ impl SessionConfiguration { &next_configuration.cwd, ); } + next_configuration.permission_profile = PermissionProfile::from_runtime_permissions( + &next_configuration.file_system_sandbox_policy, + next_configuration.network_sandbox_policy, + next_configuration.permission_profile.macos.as_ref(), + ); if let Some(app_server_client_name) = updates.app_server_client_name.clone() { next_configuration.app_server_client_name = Some(app_server_client_name); } @@ -1284,6 +1293,17 @@ impl Session { per_turn_config.service_tier = session_configuration.service_tier; per_turn_config.personality = session_configuration.personality; per_turn_config.approvals_reviewer = session_configuration.approvals_reviewer; + per_turn_config.permissions.approval_policy = session_configuration.approval_policy.clone(); + per_turn_config.permissions.sandbox_policy = session_configuration.sandbox_policy.clone(); + per_turn_config.permissions.file_system_sandbox_policy = + session_configuration.file_system_sandbox_policy.clone(); + per_turn_config.permissions.network_sandbox_policy = + session_configuration.network_sandbox_policy; + per_turn_config + .permissions + .macos_seatbelt_profile_extensions = + session_configuration.permission_profile.macos.clone(); + per_turn_config.permissions.sync_permission_profile(); let resolved_web_search_mode = resolve_web_search_mode_for_turn( &per_turn_config.web_search_mode, session_configuration.sandbox_policy.get(), @@ -1423,6 +1443,7 @@ impl Session { collaboration_mode: session_configuration.collaboration_mode.clone(), personality: session_configuration.personality, approval_policy: session_configuration.approval_policy.clone(), + permission_profile: session_configuration.permission_profile.clone(), sandbox_policy: session_configuration.sandbox_policy.clone(), file_system_sandbox_policy: session_configuration.file_system_sandbox_policy.clone(), network_sandbox_policy: session_configuration.network_sandbox_policy, @@ -5471,6 +5492,7 @@ async fn spawn_review_thread( collaboration_mode: parent_turn_context.collaboration_mode.clone(), personality: parent_turn_context.personality, approval_policy: parent_turn_context.approval_policy.clone(), + permission_profile: parent_turn_context.permission_profile.clone(), sandbox_policy: parent_turn_context.sandbox_policy.clone(), file_system_sandbox_policy: parent_turn_context.file_system_sandbox_policy.clone(), network_sandbox_policy: parent_turn_context.network_sandbox_policy, diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index 5a556f2071..c3d52517f5 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -72,6 +72,7 @@ use codex_protocol::config_types::Settings; use codex_protocol::models::BaseInstructions; use codex_protocol::models::ContentItem; use codex_protocol::models::DeveloperInstructions; +use codex_protocol::models::PermissionProfile; use codex_protocol::models::ResponseInputItem; use codex_protocol::models::ResponseItem; use codex_protocol::openai_models::ModelsResponse; @@ -1790,6 +1791,14 @@ async fn set_rate_limits_retains_previous_credits() { compact_prompt: config.compact_prompt.clone(), approval_policy: config.permissions.approval_policy.clone(), approvals_reviewer: config.approvals_reviewer, + permission_profile: PermissionProfile::from_runtime_permissions( + &config.permissions.file_system_sandbox_policy, + config.permissions.network_sandbox_policy, + config + .permissions + .macos_seatbelt_profile_extensions + .as_ref(), + ), sandbox_policy: config.permissions.sandbox_policy.clone(), file_system_sandbox_policy: config.permissions.file_system_sandbox_policy.clone(), network_sandbox_policy: config.permissions.network_sandbox_policy, @@ -1888,6 +1897,14 @@ async fn set_rate_limits_updates_plan_type_when_present() { compact_prompt: config.compact_prompt.clone(), approval_policy: config.permissions.approval_policy.clone(), approvals_reviewer: config.approvals_reviewer, + permission_profile: PermissionProfile::from_runtime_permissions( + &config.permissions.file_system_sandbox_policy, + config.permissions.network_sandbox_policy, + config + .permissions + .macos_seatbelt_profile_extensions + .as_ref(), + ), sandbox_policy: config.permissions.sandbox_policy.clone(), file_system_sandbox_policy: config.permissions.file_system_sandbox_policy.clone(), network_sandbox_policy: config.permissions.network_sandbox_policy, @@ -2232,6 +2249,14 @@ pub(crate) async fn make_session_configuration_for_tests() -> SessionConfigurati compact_prompt: config.compact_prompt.clone(), approval_policy: config.permissions.approval_policy.clone(), approvals_reviewer: config.approvals_reviewer, + permission_profile: PermissionProfile::from_runtime_permissions( + &config.permissions.file_system_sandbox_policy, + config.permissions.network_sandbox_policy, + config + .permissions + .macos_seatbelt_profile_extensions + .as_ref(), + ), sandbox_policy: config.permissions.sandbox_policy.clone(), file_system_sandbox_policy: config.permissions.file_system_sandbox_policy.clone(), network_sandbox_policy: config.permissions.network_sandbox_policy, @@ -2494,6 +2519,14 @@ async fn session_new_fails_when_zsh_fork_enabled_without_zsh_path() { compact_prompt: config.compact_prompt.clone(), approval_policy: config.permissions.approval_policy.clone(), approvals_reviewer: config.approvals_reviewer, + permission_profile: PermissionProfile::from_runtime_permissions( + &config.permissions.file_system_sandbox_policy, + config.permissions.network_sandbox_policy, + config + .permissions + .macos_seatbelt_profile_extensions + .as_ref(), + ), sandbox_policy: config.permissions.sandbox_policy.clone(), file_system_sandbox_policy: config.permissions.file_system_sandbox_policy.clone(), network_sandbox_policy: config.permissions.network_sandbox_policy, @@ -2588,6 +2621,14 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { compact_prompt: config.compact_prompt.clone(), approval_policy: config.permissions.approval_policy.clone(), approvals_reviewer: config.approvals_reviewer, + permission_profile: PermissionProfile::from_runtime_permissions( + &config.permissions.file_system_sandbox_policy, + config.permissions.network_sandbox_policy, + config + .permissions + .macos_seatbelt_profile_extensions + .as_ref(), + ), sandbox_policy: config.permissions.sandbox_policy.clone(), file_system_sandbox_policy: config.permissions.file_system_sandbox_policy.clone(), network_sandbox_policy: config.permissions.network_sandbox_policy, @@ -3425,6 +3466,14 @@ pub(crate) async fn make_session_and_context_with_dynamic_tools_and_rx( compact_prompt: config.compact_prompt.clone(), approval_policy: config.permissions.approval_policy.clone(), approvals_reviewer: config.approvals_reviewer, + permission_profile: PermissionProfile::from_runtime_permissions( + &config.permissions.file_system_sandbox_policy, + config.permissions.network_sandbox_policy, + config + .permissions + .macos_seatbelt_profile_extensions + .as_ref(), + ), sandbox_policy: config.permissions.sandbox_policy.clone(), file_system_sandbox_policy: config.permissions.file_system_sandbox_policy.clone(), network_sandbox_policy: config.permissions.network_sandbox_policy, diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 421bfa6bd6..0d61a1c606 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -19,6 +19,7 @@ use assert_matches::assert_matches; use codex_config::CONFIG_TOML_FILE; use codex_features::Feature; use codex_features::FeaturesToml; +use codex_protocol::models::PermissionProfile; use codex_protocol::permissions::FileSystemAccessMode; use codex_protocol::permissions::FileSystemPath; use codex_protocol::permissions::FileSystemSandboxEntry; @@ -514,6 +515,14 @@ fn default_permissions_profile_populates_runtime_sandbox_policy() -> std::io::Re }, ]), ); + assert_eq!( + config.permissions.permission_profile, + PermissionProfile::from_runtime_permissions( + &config.permissions.file_system_sandbox_policy, + config.permissions.network_sandbox_policy, + None, + ) + ); assert_eq!( config.permissions.sandbox_policy.get(), &SandboxPolicy::WorkspaceWrite { @@ -1130,6 +1139,15 @@ exclude_slash_tmp = true NetworkSandboxPolicy::from(sandbox_policy), "case `{name}` should preserve network semantics from legacy config" ); + assert_eq!( + config.permissions.permission_profile, + PermissionProfile::from_runtime_permissions( + &config.permissions.file_system_sandbox_policy, + config.permissions.network_sandbox_policy, + None, + ), + "case `{name}` should populate canonical permission profile from runtime policies" + ); assert_eq!( config .permissions @@ -4369,6 +4387,11 @@ fn test_precedence_fixture_with_o3_profile() -> std::io::Result<()> { model_provider: fixture.openai_provider.clone(), permissions: Permissions { approval_policy: Constrained::allow_any(AskForApproval::Never), + permission_profile: PermissionProfile::from_runtime_permissions( + &FileSystemSandboxPolicy::from(&SandboxPolicy::new_read_only_policy()), + NetworkSandboxPolicy::Restricted, + None, + ), sandbox_policy: Constrained::allow_any(SandboxPolicy::new_read_only_policy()), file_system_sandbox_policy: FileSystemSandboxPolicy::from( &SandboxPolicy::new_read_only_policy(), @@ -4512,6 +4535,11 @@ fn test_precedence_fixture_with_gpt3_profile() -> std::io::Result<()> { model_provider: fixture.openai_custom_provider.clone(), permissions: Permissions { approval_policy: Constrained::allow_any(AskForApproval::UnlessTrusted), + permission_profile: PermissionProfile::from_runtime_permissions( + &FileSystemSandboxPolicy::from(&SandboxPolicy::new_read_only_policy()), + NetworkSandboxPolicy::Restricted, + None, + ), sandbox_policy: Constrained::allow_any(SandboxPolicy::new_read_only_policy()), file_system_sandbox_policy: FileSystemSandboxPolicy::from( &SandboxPolicy::new_read_only_policy(), @@ -4653,6 +4681,11 @@ fn test_precedence_fixture_with_zdr_profile() -> std::io::Result<()> { model_provider: fixture.openai_provider.clone(), permissions: Permissions { approval_policy: Constrained::allow_any(AskForApproval::OnFailure), + permission_profile: PermissionProfile::from_runtime_permissions( + &FileSystemSandboxPolicy::from(&SandboxPolicy::new_read_only_policy()), + NetworkSandboxPolicy::Restricted, + None, + ), sandbox_policy: Constrained::allow_any(SandboxPolicy::new_read_only_policy()), file_system_sandbox_policy: FileSystemSandboxPolicy::from( &SandboxPolicy::new_read_only_policy(), @@ -4780,6 +4813,11 @@ fn test_precedence_fixture_with_gpt5_profile() -> std::io::Result<()> { model_provider: fixture.openai_provider.clone(), permissions: Permissions { approval_policy: Constrained::allow_any(AskForApproval::OnFailure), + permission_profile: PermissionProfile::from_runtime_permissions( + &FileSystemSandboxPolicy::from(&SandboxPolicy::new_read_only_policy()), + NetworkSandboxPolicy::Restricted, + None, + ), sandbox_policy: Constrained::allow_any(SandboxPolicy::new_read_only_policy()), file_system_sandbox_policy: FileSystemSandboxPolicy::from( &SandboxPolicy::new_read_only_policy(), diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 6db6f0e613..5c4ae4ef3f 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -79,6 +79,7 @@ use codex_protocol::config_types::WebSearchMode; use codex_protocol::config_types::WebSearchToolConfig; use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::models::MacOsSeatbeltProfileExtensions; +use codex_protocol::models::PermissionProfile; use codex_protocol::openai_models::ModelsResponse; use codex_protocol::openai_models::ReasoningEffort; use codex_protocol::permissions::FileSystemSandboxPolicy; @@ -228,6 +229,8 @@ pub(crate) fn test_config() -> Config { pub struct Permissions { /// Approval policy for executing commands. pub approval_policy: Constrained, + /// Canonical permission abstraction for the current thread/runtime. + pub permission_profile: PermissionProfile, /// Effective sandbox policy used for shell/unified exec. pub sandbox_policy: Constrained, /// Effective filesystem sandbox policy, including entries that cannot yet @@ -259,6 +262,20 @@ pub struct Permissions { pub macos_seatbelt_profile_extensions: Option, } +impl Permissions { + pub(crate) fn runtime_permission_profile(&self) -> PermissionProfile { + PermissionProfile::from_runtime_permissions( + &self.file_system_sandbox_policy, + self.network_sandbox_policy, + self.macos_seatbelt_profile_extensions.as_ref(), + ) + } + + pub(crate) fn sync_permission_profile(&mut self) { + self.permission_profile = self.runtime_permission_profile(); + } +} + /// Application configuration loaded from disk and merged with overrides. #[derive(Debug, Clone, PartialEq)] pub struct Config { @@ -2584,6 +2601,11 @@ impl Config { } else { NetworkSandboxPolicy::from(&effective_sandbox_policy) }; + let permission_profile = PermissionProfile::from_runtime_permissions( + &effective_file_system_sandbox_policy, + effective_network_sandbox_policy, + /*macos*/ None, + ); let config = Self { model, service_tier, @@ -2596,6 +2618,7 @@ impl Config { startup_warnings, permissions: Permissions { approval_policy: constrained_approval_policy.value, + permission_profile, sandbox_policy: constrained_sandbox_policy.value, file_system_sandbox_policy: effective_file_system_sandbox_policy, network_sandbox_policy: effective_network_sandbox_policy, diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index edb94fb4f0..75a09ef454 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -83,10 +83,10 @@ fn write_permissions_for_paths(file_paths: &[AbsolutePathBuf]) -> Option 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/core/src/tools/handlers/multi_agents_common.rs b/codex-rs/core/src/tools/handlers/multi_agents_common.rs index 452e05a2f4..4ce4baa607 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents_common.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents_common.rs @@ -277,6 +277,8 @@ pub(crate) fn apply_spawn_agent_runtime_overrides( })?; config.permissions.file_system_sandbox_policy = turn.file_system_sandbox_policy.clone(); config.permissions.network_sandbox_policy = turn.network_sandbox_policy; + config.permissions.macos_seatbelt_profile_extensions = turn.permission_profile.macos.clone(); + config.permissions.sync_permission_profile(); Ok(()) } diff --git a/codex-rs/core/src/tools/handlers/multi_agents_tests.rs b/codex-rs/core/src/tools/handlers/multi_agents_tests.rs index e57491762c..ed42b6b5dd 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents_tests.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents_tests.rs @@ -2479,6 +2479,7 @@ async fn build_agent_spawn_config_uses_turn_context_values() { .expect("sandbox policy set"); expected.permissions.file_system_sandbox_policy = file_system_sandbox_policy; expected.permissions.network_sandbox_policy = network_sandbox_policy; + expected.permissions.sync_permission_profile(); assert_eq!(config, expected); } diff --git a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs index 2390068cc5..9577c38517 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs @@ -181,10 +181,10 @@ fn exec_command_args_resolve_relative_additional_permissions_against_workdir() - assert_eq!( args.additional_permissions, Some(PermissionProfile { - file_system: Some(FileSystemPermissions { - read: None, - write: Some(vec![AbsolutePathBuf::try_from(expected_write)?]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + None, + Some(vec![AbsolutePathBuf::try_from(expected_write)?]), + )), ..Default::default() }) ); diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs index 46ba0cf908..a6978833f1 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs @@ -264,12 +264,12 @@ fn map_exec_result_preserves_stdout_and_stderr() { #[test] fn shell_request_escalation_execution_is_explicit() { let requested_permissions = 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("/tmp/output").unwrap(), ]), - }), + )), ..Default::default() }; let sandbox_policy = SandboxPolicy::WorkspaceWrite { @@ -632,6 +632,14 @@ async fn prepare_escalated_exec_permissions_preserve_macos_seatbelt_extensions() let permissions = Permissions { approval_policy: Constrained::allow_any(AskForApproval::Never), + permission_profile: PermissionProfile::from_runtime_permissions( + &read_only_file_system_sandbox_policy(), + codex_protocol::permissions::NetworkSandboxPolicy::Restricted, + Some(&MacOsSeatbeltProfileExtensions { + macos_preferences: MacOsPreferencesPermission::ReadWrite, + ..Default::default() + }), + ), sandbox_policy: Constrained::allow_any(SandboxPolicy::new_read_only_policy()), file_system_sandbox_policy: read_only_file_system_sandbox_policy(), network_sandbox_policy: codex_protocol::permissions::NetworkSandboxPolicy::Restricted, diff --git a/codex-rs/core/tests/suite/request_permissions.rs b/codex-rs/core/tests/suite/request_permissions.rs index 7c16599bf0..950f7fa75e 100644 --- a/codex-rs/core/tests/suite/request_permissions.rs +++ b/codex-rs/core/tests/suite/request_permissions.rs @@ -6,6 +6,7 @@ use codex_core::sandboxing::SandboxPermissions; use codex_features::Feature; use codex_protocol::models::FileSystemPermissions; use codex_protocol::models::PermissionProfile; +use codex_protocol::permissions::FileSystemAccessMode; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::ExecApprovalRequestEvent; @@ -292,20 +293,20 @@ fn workspace_write_excluding_tmp() -> SandboxPolicy { fn requested_directory_write_permissions(path: &Path) -> RequestPermissionProfile { RequestPermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(vec![]), - write: Some(vec![absolute_path(path)]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![]), + Some(vec![absolute_path(path)]), + )), ..RequestPermissionProfile::default() } } fn normalized_directory_write_permissions(path: &Path) -> Result { Ok(RequestPermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(vec![]), - write: Some(vec![AbsolutePathBuf::try_from(path.canonicalize()?)?]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![]), + Some(vec![AbsolutePathBuf::try_from(path.canonicalize()?)?]), + )), ..RequestPermissionProfile::default() }) } @@ -342,10 +343,10 @@ async fn with_additional_permissions_requires_approval_under_on_request() -> Res let call_id = "request_permissions_skip_approval"; let command = "touch requested-dir/requested-but-unused.txt"; let requested_permissions = PermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(vec![]), - write: Some(vec![absolute_path(&requested_dir_canonical)]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![]), + Some(vec![absolute_path(&requested_dir_canonical)]), + )), ..Default::default() }; let event = shell_event_with_request_permissions(call_id, command, &requested_permissions)?; @@ -520,10 +521,10 @@ async fn relative_additional_permissions_resolve_against_tool_workdir() -> Resul let call_id = "request_permissions_relative_workdir"; let command = "touch relative-write.txt"; let expected_permissions = PermissionProfile { - file_system: Some(FileSystemPermissions { - read: None, - write: Some(vec![absolute_path(&nested_dir_canonical)]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + None, + Some(vec![absolute_path(&nested_dir_canonical)]), + )), ..Default::default() }; let event = shell_event_with_raw_request_permissions( @@ -623,10 +624,10 @@ async fn read_only_with_additional_permissions_does_not_widen_to_unrequested_cwd "cwd-widened", unrequested_write, unrequested_write ); let requested_permissions = PermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(vec![]), - write: Some(vec![absolute_path(&requested_write)]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![]), + Some(vec![absolute_path(&requested_write)]), + )), ..Default::default() }; let event = shell_event_with_request_permissions(call_id, &command, &requested_permissions)?; @@ -724,10 +725,10 @@ async fn read_only_with_additional_permissions_does_not_widen_to_unrequested_tmp "tmp-widened", tmp_write, tmp_write ); let requested_permissions = PermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(vec![]), - write: Some(vec![absolute_path(&requested_write)]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![]), + Some(vec![absolute_path(&requested_write)]), + )), ..Default::default() }; let event = shell_event_with_request_permissions(call_id, &command, &requested_permissions)?; @@ -823,19 +824,19 @@ async fn workspace_write_with_additional_permissions_can_write_outside_cwd() -> "outside-cwd-ok", outside_write, outside_write ); let requested_permissions = RequestPermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(vec![]), - write: Some(vec![absolute_path(outside_dir.path())]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![]), + Some(vec![absolute_path(outside_dir.path())]), + )), ..RequestPermissionProfile::default() }; let normalized_requested_permissions = RequestPermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(vec![]), - write: Some(vec![AbsolutePathBuf::try_from( + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![]), + Some(vec![AbsolutePathBuf::try_from( outside_dir.path().canonicalize()?, )?]), - }), + )), ..RequestPermissionProfile::default() }; let event = shell_event_with_request_permissions(call_id, &command, &requested_permissions)?; @@ -925,19 +926,19 @@ async fn with_additional_permissions_denied_approval_blocks_execution() -> Resul "should-not-write", outside_write, outside_write ); let requested_permissions = PermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(vec![]), - write: Some(vec![absolute_path(outside_dir.path())]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![]), + Some(vec![absolute_path(outside_dir.path())]), + )), ..Default::default() }; let normalized_requested_permissions = PermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(vec![]), - write: Some(vec![AbsolutePathBuf::try_from( + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![]), + Some(vec![AbsolutePathBuf::try_from( outside_dir.path().canonicalize()?, )?]), - }), + )), ..Default::default() }; let event = shell_event_with_request_permissions(call_id, &command, &requested_permissions)?; @@ -1027,19 +1028,19 @@ async fn request_permissions_grants_apply_to_later_exec_command_calls() -> Resul "sticky-grant-ok", outside_write, outside_write ); let requested_permissions = RequestPermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(vec![]), - write: Some(vec![absolute_path(outside_dir.path())]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![]), + Some(vec![absolute_path(outside_dir.path())]), + )), ..Default::default() }; let normalized_requested_permissions = RequestPermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(vec![]), - write: Some(vec![AbsolutePathBuf::try_from( + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![]), + Some(vec![AbsolutePathBuf::try_from( outside_dir.path().canonicalize()?, )?]), - }), + )), ..Default::default() }; let responses = mount_sse_sequence( @@ -1491,35 +1492,35 @@ async fn partial_request_permissions_grants_do_not_preapprove_new_permissions() ); let requested_permissions = RequestPermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(vec![]), - write: Some(vec![ + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![]), + Some(vec![ absolute_path(first_dir.path()), absolute_path(second_dir.path()), ]), - }), + )), ..RequestPermissionProfile::default() }; let normalized_requested_permissions = RequestPermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(vec![]), - write: Some(vec![ + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![]), + Some(vec![ AbsolutePathBuf::try_from(first_dir.path().canonicalize()?)?, AbsolutePathBuf::try_from(second_dir.path().canonicalize()?)?, ]), - }), + )), ..RequestPermissionProfile::default() }; let granted_permissions = normalized_directory_write_permissions(first_dir.path())?; let second_dir_permissions = requested_directory_write_permissions(second_dir.path()); let merged_permissions = PermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(vec![]), - write: Some(vec![ + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![]), + Some(vec![ AbsolutePathBuf::try_from(first_dir.path().canonicalize()?)?, AbsolutePathBuf::try_from(second_dir.path().canonicalize()?)?, ]), - }), + )), ..Default::default() }; @@ -1584,16 +1585,31 @@ async fn partial_request_permissions_grants_do_not_preapprove_new_permissions() let approval_file_system = approval_permissions .file_system .unwrap_or_else(|| panic!("expected filesystem permissions")); - assert!(approval_file_system.read.as_ref().is_none_or(Vec::is_empty)); - let mut approval_writes = approval_file_system.write.unwrap_or_default(); + assert_eq!( + approval_file_system + .explicit_path_entries() + .filter(|(_, access)| *access == FileSystemAccessMode::Read) + .count(), + 0 + ); + + let mut approval_writes = approval_file_system + .explicit_path_entries() + .filter_map(|(path, access)| { + (access == FileSystemAccessMode::Write).then_some(path.clone()) + }) + .collect::>(); approval_writes.sort_by_key(|path| path.display().to_string()); let mut expected_writes = merged_permissions .file_system .unwrap_or_else(|| panic!("expected merged filesystem permissions")) - .write - .unwrap_or_default(); + .explicit_path_entries() + .filter_map(|(path, access)| { + (access == FileSystemAccessMode::Write).then_some(path.clone()) + }) + .collect::>(); expected_writes.sort_by_key(|path| path.display().to_string()); assert_eq!(approval_writes, expected_writes); diff --git a/codex-rs/core/tests/suite/request_permissions_tool.rs b/codex-rs/core/tests/suite/request_permissions_tool.rs index 14506f4a41..0578441e99 100644 --- a/codex-rs/core/tests/suite/request_permissions_tool.rs +++ b/codex-rs/core/tests/suite/request_permissions_tool.rs @@ -81,20 +81,20 @@ fn workspace_write_excluding_tmp() -> SandboxPolicy { fn requested_directory_write_permissions(path: &Path) -> RequestPermissionProfile { RequestPermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(vec![]), - write: Some(vec![absolute_path(path)]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![]), + Some(vec![absolute_path(path)]), + )), ..RequestPermissionProfile::default() } } fn normalized_directory_write_permissions(path: &Path) -> Result { Ok(RequestPermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(vec![]), - write: Some(vec![AbsolutePathBuf::try_from(path.canonicalize()?)?]), - }), + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(vec![]), + Some(vec![AbsolutePathBuf::try_from(path.canonicalize()?)?]), + )), ..RequestPermissionProfile::default() }) } diff --git a/codex-rs/protocol/src/models.rs b/codex-rs/protocol/src/models.rs index 6fa8162bf7..f0d3231466 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 codex_utils_image::PromptImageMode; @@ -12,6 +13,12 @@ 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::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; @@ -65,15 +72,119 @@ 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, } 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::Special { .. } => None, + }) + } + + 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)) + } + } } } @@ -221,6 +332,87 @@ impl PermissionProfile { pub fn is_empty(&self) -> bool { self.network.is_none() && self.file_system.is_none() && self.macos.is_none() } + + pub fn from_runtime_permissions( + file_system_sandbox_policy: &FileSystemSandboxPolicy, + network_sandbox_policy: NetworkSandboxPolicy, + macos: Option<&MacOsSeatbeltProfileExtensions>, + ) -> Self { + Self { + network: Some(network_sandbox_policy.into()), + file_system: Some(file_system_sandbox_policy.into()), + macos: macos.cloned(), + } + } + + pub fn from_legacy_sandbox_policy( + sandbox_policy: &SandboxPolicy, + cwd: &Path, + macos: Option<&MacOsSeatbeltProfileExtensions>, + ) -> Self { + Self::from_runtime_permissions( + &FileSystemSandboxPolicy::from_legacy_sandbox_policy(sandbox_policy, cwd), + NetworkSandboxPolicy::from(sandbox_policy), + macos, + ) + } + + 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 { + crate::permissions::FileSystemSandboxKind::Restricted => value.entries.clone(), + crate::permissions::FileSystemSandboxKind::Unrestricted + | crate::permissions::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)] @@ -1652,6 +1844,71 @@ mod tests { assert_eq!(permission_profile.is_empty(), false); } + #[test] + fn file_system_permissions_deserialize_legacy_read_write_shape() { + let file_system = serde_json::from_value::(serde_json::json!({ + "read": ["/tmp/read-only"], + "write": ["/tmp/read-write"], + })) + .expect("deserialize legacy filesystem permissions"); + + assert_eq!( + file_system, + FileSystemPermissions::from_read_write_roots( + Some(vec![ + AbsolutePathBuf::from_absolute_path("/tmp/read-only") + .expect("path must be absolute"), + ]), + Some(vec![ + AbsolutePathBuf::from_absolute_path("/tmp/read-write") + .expect("path must be absolute"), + ]), + ) + ); + } + + #[test] + fn file_system_permissions_serialize_explicit_paths_as_legacy_read_write_shape() { + let file_system = FileSystemPermissions::from_read_write_roots( + Some(vec![ + AbsolutePathBuf::from_absolute_path("/tmp/read-only") + .expect("path must be absolute"), + ]), + Some(vec![ + AbsolutePathBuf::from_absolute_path("/tmp/read-write") + .expect("path must be absolute"), + ]), + ); + + let value = serde_json::to_value(file_system).expect("serialize filesystem permissions"); + + assert_eq!( + value, + serde_json::json!({ + "read": ["/tmp/read-only"], + "write": ["/tmp/read-write"], + }) + ); + } + + #[test] + fn file_system_permissions_round_trip_special_entries_through_canonical_shape() { + let file_system = FileSystemPermissions { + entries: vec![FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::CurrentWorkingDirectory, + }, + access: FileSystemAccessMode::Write, + }], + }; + + let value = serde_json::to_value(&file_system).expect("serialize filesystem permissions"); + let reparsed = serde_json::from_value::(value) + .expect("deserialize filesystem permissions"); + + assert_eq!(reparsed, file_system); + } + #[test] fn macos_preferences_permission_deserializes_read_write() { let permission = serde_json::from_str::("\"read_write\"") diff --git a/codex-rs/protocol/src/permissions.rs b/codex-rs/protocol/src/permissions.rs index 978334f696..f634189bf3 100644 --- a/codex-rs/protocol/src/permissions.rs +++ b/codex-rs/protocol/src/permissions.rs @@ -45,6 +45,7 @@ impl NetworkSandboxPolicy { Copy, PartialEq, Eq, + Hash, PartialOrd, Ord, Serialize, @@ -71,7 +72,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 { @@ -114,7 +115,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, @@ -155,7 +156,7 @@ struct FileSystemSemanticSignature { unreadable_roots: Vec, } -#[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 { diff --git a/codex-rs/sandboxing/src/manager_tests.rs b/codex-rs/sandboxing/src/manager_tests.rs index 67911bff6a..d95193a4ea 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()), + )), ..Default::default() }), }, @@ -186,10 +186,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 045c7e3f90..35f505e5ff 100644 --- a/codex-rs/sandboxing/src/policy_transforms.rs +++ b/codex-rs/sandboxing/src/policy_transforms.rs @@ -4,7 +4,6 @@ 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; @@ -55,13 +54,26 @@ 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(path.as_path()) + .ok() + .and_then(|path| AbsolutePathBuf::from_absolute_path(path).ok()) + .unwrap_or(path), + }, + 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()); let macos = additional_permissions.macos; @@ -102,8 +114,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()), @@ -133,28 +144,13 @@ pub fn intersect_permission_profiles( 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 } + let granted_entries = granted.file_system.unwrap_or_default().entries; + let entries = requested_file_system + .entries + .into_iter() + .filter(|entry| granted_entries.contains(entry)) + .collect(); + FileSystemPermissions { entries } }) .filter(|file_system| !file_system.is_empty()); let network = match (requested.network, granted.network) { @@ -180,47 +176,17 @@ pub fn intersect_permission_profiles( } } -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); +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 { @@ -234,7 +200,7 @@ fn dedup_absolute_paths(paths: Vec) -> Vec { out } -fn additional_permission_roots( +fn additional_permission_explicit_path_roots( additional_permissions: &PermissionProfile, ) -> (Vec, Vec) { ( @@ -242,14 +208,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(), ), ) @@ -257,28 +233,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 @@ -297,14 +259,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, ) } } @@ -364,7 +327,11 @@ fn sandbox_policy_with_additional_permissions( return sandbox_policy.clone(); } - let (extra_reads, extra_writes) = additional_permission_roots(additional_permissions); + // Legacy SandboxPolicy remains a best-effort projection during the + // migration to PermissionProfile-backed thread permissions. The direct + // filesystem sandbox policy carries the full permission shape. + let (extra_reads, extra_writes) = + additional_permission_explicit_path_roots(additional_permissions); match sandbox_policy { SandboxPolicy::DangerFullAccess => SandboxPolicy::DangerFullAccess, diff --git a/codex-rs/sandboxing/src/policy_transforms_tests.rs b/codex-rs/sandboxing/src/policy_transforms_tests.rs index a9c759a205..0b10a13083 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()]), + )), ..Default::default() }) .expect("permissions"); @@ -122,10 +122,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]), + )) ); } @@ -147,20 +147,20 @@ fn normalize_additional_permissions_canonicalizes_symlinked_write_paths() { .expect("absolute canonical 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![expected_write_dir]), - }) + Some(FileSystemPermissions::from_read_write_roots( + Some(vec![]), + Some(vec![expected_write_dir]), + )) ); } @@ -168,10 +168,7 @@ fn normalize_additional_permissions_canonicalizes_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()), macos: None, }) .expect("permissions"); @@ -201,12 +198,12 @@ fn normalize_additional_permissions_preserves_default_macos_preferences_permissi #[test] fn intersect_permission_profiles_preserves_default_macos_grants() { let requested = PermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(Vec::from(["/tmp/requested" + file_system: Some(FileSystemPermissions::from_read_write_roots( + Some(Vec::from(["/tmp/requested" .try_into() .expect("absolute path")])), - write: None, - }), + None, + )), macos: Some(MacOsSeatbeltProfileExtensions { macos_preferences: MacOsPreferencesPermission::ReadWrite, macos_automation: MacOsAutomationPermission::BundleIds(vec![ @@ -221,10 +218,7 @@ fn intersect_permission_profiles_preserves_default_macos_grants() { ..Default::default() }; let granted = PermissionProfile { - file_system: Some(FileSystemPermissions { - read: Some(Vec::new()), - write: None, - }), + file_system: Some(FileSystemPermissions::default()), macos: Some(MacOsSeatbeltProfileExtensions::default()), ..Default::default() }; @@ -292,10 +286,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()), + )), ..Default::default() }, ); @@ -340,10 +334,10 @@ fn effective_permissions_merge_macos_extensions_with_additional_permissions() { macos_contacts: MacOsContactsPermission::None, }), Some(&PermissionProfile { - 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()), + )), macos: Some(MacOsSeatbeltProfileExtensions { macos_preferences: MacOsPreferencesPermission::ReadWrite, macos_automation: MacOsAutomationPermission::BundleIds(vec![ @@ -391,10 +385,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()), + )), ..Default::default() }, ); @@ -431,8 +425,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!( @@ -501,10 +497,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/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index 1b403c251f..5baecbaba4 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -23,6 +23,7 @@ use codex_protocol::models::MacOsAutomationPermission; use codex_protocol::models::MacOsContactsPermission; use codex_protocol::models::MacOsPreferencesPermission; 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; @@ -760,20 +761,24 @@ 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)| { + (access == FileSystemAccessMode::Read).then_some(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)| { + (access == FileSystemAccessMode::Write).then_some(format!("`{}`", path.display())) + }) + .collect::>() + .join(", "); + if !writes.is_empty() { parts.push(format!("write {writes}")); } } @@ -972,10 +977,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")]), + )), }, } } @@ -1249,10 +1254,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( @@ -1330,10 +1335,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")]), + )), ..Default::default() }), }; @@ -1378,10 +1383,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")]), + )), ..Default::default() }), }; diff --git a/codex-rs/tui_app_server/src/bottom_pane/approval_overlay.rs b/codex-rs/tui_app_server/src/bottom_pane/approval_overlay.rs index f5d1cee621..9e179eb4ff 100644 --- a/codex-rs/tui_app_server/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui_app_server/src/bottom_pane/approval_overlay.rs @@ -23,6 +23,7 @@ use codex_protocol::models::MacOsAutomationPermission; use codex_protocol::models::MacOsContactsPermission; use codex_protocol::models::MacOsPreferencesPermission; 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; @@ -746,20 +747,24 @@ 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)| { + (access == FileSystemAccessMode::Read).then_some(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)| { + (access == FileSystemAccessMode::Write).then_some(format!("`{}`", path.display())) + }) + .collect::>() + .join(", "); + if !writes.is_empty() { parts.push(format!("write {writes}")); } } @@ -958,10 +963,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")]), + )), }, } } @@ -1235,10 +1240,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( @@ -1316,10 +1321,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")]), + )), ..Default::default() }), }; @@ -1364,10 +1369,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")]), + )), ..Default::default() }), };