From fe8e540a80f17716bf2a3007f06e8a602192c704 Mon Sep 17 00:00:00 2001 From: ioanbudea-OAI Date: Wed, 11 Nov 2026 01:47:54 +0000 Subject: [PATCH] Centralize permission config selection and legacy translation (#60702) ## What changed Move permission selection into `codex-config` with a shared `PermissionConfig` representation for legacy sandbox settings and named profiles. Resolve selectors using config layer precedence while preserving explicit session profile selection and sandbox mode overrides. Share legacy permission translation between config derivation and core loading, preserving merged workspace roots, network access, temporary-directory exclusions, and platform sandbox availability without modifying source layers or field origins. ## Testing Add coverage for mixed selector precedence, merged legacy settings, unchanged source metadata, and the Windows read-only fallback. Extend the managed preferences loader test to verify settings merged across layers and the absence of an active named profile for legacy permissions. GitOrigin-RevId: a435be7af38424ac33711e6a7557f52b9b672577 --- codex-rs/config/src/config_toml.rs | 30 +--- codex-rs/config/src/lib.rs | 4 + codex-rs/config/src/permission_config.rs | 107 +++++++++++ .../config/src/permission_config_tests.rs | 167 ++++++++++++++++++ .../core/src/config/config_loader_tests.rs | 19 +- codex-rs/core/src/config/mod.rs | 106 +++-------- .../src/app/tests/startup_defaults_tests.rs | 4 +- 7 files changed, 327 insertions(+), 110 deletions(-) create mode 100644 codex-rs/config/src/permission_config.rs create mode 100644 codex-rs/config/src/permission_config_tests.rs diff --git a/codex-rs/config/src/config_toml.rs b/codex-rs/config/src/config_toml.rs index 97d880947e..340861ba53 100644 --- a/codex-rs/config/src/config_toml.rs +++ b/codex-rs/config/src/config_toml.rs @@ -54,7 +54,6 @@ use codex_protocol::config_types::WebSearchToolConfig; use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::models::PermissionProfile; use codex_protocol::openai_models::ReasoningEffort; -use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::AskForApproval; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::Platform; @@ -877,37 +876,12 @@ impl ConfigToml { .map(|_| SandboxMode::WorkspaceWrite) }) .unwrap_or_default(); - let effective_sandbox_mode = effective_sandbox_mode( + let permission_profile = crate::translate_legacy_permissions( resolved_sandbox_mode, + self.sandbox_workspace_write.as_ref(), Platform::native(), windows_sandbox_level, ); - - let permission_profile = match effective_sandbox_mode { - SandboxMode::ReadOnly => PermissionProfile::read_only(), - SandboxMode::WorkspaceWrite => match self.sandbox_workspace_write.as_ref() { - Some(SandboxWorkspaceWrite { - writable_roots, - network_access, - exclude_tmpdir_env_var, - exclude_slash_tmp, - }) => { - let network_policy = if *network_access { - NetworkSandboxPolicy::Enabled - } else { - NetworkSandboxPolicy::Restricted - }; - PermissionProfile::workspace_write_with( - writable_roots, - network_policy, - *exclude_tmpdir_env_var, - *exclude_slash_tmp, - ) - } - None => PermissionProfile::workspace_write(), - }, - SandboxMode::DangerFullAccess => PermissionProfile::Disabled, - }; if configured_sandbox_mode.is_none() && let Some(constraint) = permission_profile_constraint && let Err(err) = constraint.can_set(&permission_profile) diff --git a/codex-rs/config/src/lib.rs b/codex-rs/config/src/lib.rs index 6bff2b4f88..4a10ce6872 100644 --- a/codex-rs/config/src/lib.rs +++ b/codex-rs/config/src/lib.rs @@ -34,6 +34,7 @@ mod model_provider_defaults; mod model_provider_requirements; mod overrides; mod path_context; +mod permission_config; pub mod permissions_toml; mod plugin_edit; pub mod profile_toml; @@ -207,6 +208,9 @@ pub use merge::shell_environment_filter_entry; pub use model_provider_defaults::apply_default_gcp_auth; pub use overrides::build_cli_overrides_layer; pub use path_context::ConfigPathContext; +pub use permission_config::PermissionConfig; +pub use permission_config::resolve_permission_config; +pub use permission_config::translate_legacy_permissions; pub use plugin_edit::PluginConfigEdit; pub use plugin_edit::apply_user_plugin_config_edits; pub use plugin_edit::clear_user_plugin; diff --git a/codex-rs/config/src/permission_config.rs b/codex-rs/config/src/permission_config.rs new file mode 100644 index 0000000000..04baea0484 --- /dev/null +++ b/codex-rs/config/src/permission_config.rs @@ -0,0 +1,107 @@ +//! Selects legacy or named permissions without changing the source config layers. +//! Legacy settings are merged before translation; managed constraints apply afterward. + +use crate::ConfigLayerSource; +use crate::ConfigLayerStack; +use crate::config_toml::ConfigToml; +use crate::config_toml::effective_sandbox_mode; +use crate::types::SandboxWorkspaceWrite; +use codex_protocol::config_types::SandboxMode; +use codex_protocol::config_types::WindowsSandboxLevel; +use codex_protocol::models::PermissionProfile; +use codex_protocol::permissions::NetworkSandboxPolicy; +use codex_utils_path_uri::Platform; + +#[derive(serde::Deserialize)] +struct PermissionSelectors { + sandbox_mode: Option, + default_permissions: Option, +} + +/// A config-selected permission request, before managed selection and constraints. +/// `None` from the resolver means trust/platform-dependent defaults still apply. +#[derive(Debug, Clone, Copy, PartialEq)] +pub enum PermissionConfig<'a> { + Legacy { + mode: SandboxMode, + workspace_write: Option<&'a SandboxWorkspaceWrite>, + }, + Named { + id: &'a str, + }, +} + +/// Resolves competing selectors at their original layer precedence. Field origins +/// remain available from `layers.origins()`; no synthetic config is inserted. +/// `config` must be the effective merged view of `layers`. +pub fn resolve_permission_config<'a>( + layers: &ConfigLayerStack, + config: &'a ConfigToml, + sandbox_mode_override: Option, +) -> Option> { + let legacy = |mode| PermissionConfig::Legacy { + mode, + workspace_write: config.sandbox_workspace_write.as_ref(), + }; + if let Some(mode) = sandbox_mode_override { + return Some(legacy(mode)); + } + let named = config + .default_permissions + .as_deref() + .map(|id| PermissionConfig::Named { id }); + // Explicit session profile selection retains the existing override exception. + if layers + .layers_high_to_low() + .find(|layer| matches!(layer.name, ConfigLayerSource::SessionFlags)) + .and_then(|layer| layer.config.clone().try_into::().ok()) + .is_some_and(|selectors| selectors.default_permissions.is_some()) + { + return named; + } + let mut selection = None; + for layer in layers.layers_low_to_high() { + let Ok(selectors) = layer.config.clone().try_into::() else { + continue; + }; + if selectors.sandbox_mode.is_some() { + selection = config.sandbox_mode.map(legacy); + } + if selectors.default_permissions.is_some() { + selection = named; + } + } + selection.or_else(|| named.or_else(|| config.sandbox_mode.map(legacy))) +} + +/// Translates a merged legacy request using the executor's sandbox availability. +/// This preserves custom roots, networking and temporary-directory exclusions; +/// it does not assign a named-profile identity or apply managed constraints. +pub fn translate_legacy_permissions( + mode: SandboxMode, + workspace_write: Option<&SandboxWorkspaceWrite>, + platform: Platform, + windows_sandbox_level: WindowsSandboxLevel, +) -> PermissionProfile { + match effective_sandbox_mode(mode, platform, windows_sandbox_level) { + SandboxMode::ReadOnly => PermissionProfile::read_only(), + SandboxMode::WorkspaceWrite => match workspace_write { + Some(settings) => PermissionProfile::workspace_write_with( + &settings.writable_roots, + if settings.network_access { + NetworkSandboxPolicy::Enabled + } else { + NetworkSandboxPolicy::Restricted + }, + settings.exclude_tmpdir_env_var, + settings.exclude_slash_tmp, + ), + None => PermissionProfile::workspace_write(), + }, + SandboxMode::DangerFullAccess => PermissionProfile::Disabled, + } +} + +#[cfg(test)] +#[path = "permission_config_tests.rs"] +mod tests; diff --git a/codex-rs/config/src/permission_config_tests.rs b/codex-rs/config/src/permission_config_tests.rs new file mode 100644 index 0000000000..99c2e9e71e --- /dev/null +++ b/codex-rs/config/src/permission_config_tests.rs @@ -0,0 +1,167 @@ +#![cfg(test)] + +use super::*; +use crate::ConfigLayerEntry; +use crate::ConfigRequirements; +use crate::ConfigRequirementsToml; +use pretty_assertions::assert_eq; +use tempfile::TempDir; + +#[test] +fn permission_config_merges_legacy_fields_without_changing_sources() -> anyhow::Result<()> { + let home = TempDir::new()?; + let root = crate::AbsolutePathBuf::from_absolute_path(home.path())?; + let cloud = ConfigLayerEntry::new( + ConfigLayerSource::EnterpriseManaged { + id: "policy".into(), + name: "policy".into(), + }, + toml::from_str(&format!( + r#" +default_permissions = ":read-only" +[sandbox_workspace_write] +writable_roots = [{}] +exclude_tmpdir_env_var = true +"#, + serde_json::to_string(&root)? + ))?, + ); + let user = ConfigLayerEntry::new( + ConfigLayerSource::User { + file: root.join("config.toml"), + profile: None, + }, + toml::from_str("[sandbox_workspace_write]\nnetwork_access = true")?, + ); + let managed = ConfigLayerEntry::new( + ConfigLayerSource::LegacyManagedConfigTomlFromMdm, + toml::from_str("sandbox_mode = 'workspace-write'")?, + ); + let cloud_metadata = cloud.metadata(); + let user_metadata = user.metadata(); + let managed_metadata = managed.metadata(); + let stack = ConfigLayerStack::new( + vec![cloud, user, managed], + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + )?; + let original = stack.effective_config(); + let config = ConfigToml::try_from(original.clone())?; + let Some(PermissionConfig::Legacy { + mode, + workspace_write, + }) = resolve_permission_config(&stack, &config, /*sandbox_mode_override*/ None) + else { + anyhow::bail!("MDM legacy selector should override the cloud profile"); + }; + assert_eq!( + translate_legacy_permissions( + mode, + workspace_write, + Platform::Linux, + WindowsSandboxLevel::Disabled + ), + PermissionProfile::workspace_write_with( + &[root], + NetworkSandboxPolicy::Enabled, + /*exclude_tmpdir_env_var*/ true, + /*exclude_slash_tmp*/ false + ), + ); + let origins = stack.origins(); + assert_eq!( + ( + origins.get("sandbox_mode"), + origins.get("sandbox_workspace_write.writable_roots.0"), + origins.get("sandbox_workspace_write.network_access") + ), + ( + Some(&managed_metadata), + Some(&cloud_metadata), + Some(&user_metadata) + ), + ); + assert_eq!(stack.effective_config(), original); + Ok(()) +} + +#[test] +fn permission_config_preserves_mixed_selector_precedence() -> anyhow::Result<()> { + let home = TempDir::new()?; + for (user, session, managed, expected) in [ + ( + "default_permissions = ':workspace'", + "", + "sandbox_mode = 'read-only'", + PermissionConfig::Legacy { + mode: SandboxMode::ReadOnly, + workspace_write: None, + }, + ), + ( + "sandbox_mode = 'read-only'", + "", + "default_permissions = ':workspace'", + PermissionConfig::Named { id: ":workspace" }, + ), + ( + "", + "default_permissions = ':workspace'", + "sandbox_mode = 'read-only'", + PermissionConfig::Named { id: ":workspace" }, + ), + ( + "sandbox_mode = 'read-only'\ndefault_permissions = ':workspace'", + "", + "", + PermissionConfig::Named { id: ":workspace" }, + ), + ] { + let stack = ConfigLayerStack::new( + vec![ + ConfigLayerEntry::new( + ConfigLayerSource::User { + file: crate::AbsolutePathBuf::from_absolute_path( + home.path().join("config.toml"), + )?, + profile: None, + }, + toml::from_str(user)?, + ), + ConfigLayerEntry::new(ConfigLayerSource::SessionFlags, toml::from_str(session)?), + ConfigLayerEntry::new( + ConfigLayerSource::LegacyManagedConfigTomlFromMdm, + toml::from_str(managed)?, + ), + ], + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + )?; + let config = ConfigToml::try_from(stack.effective_config())?; + assert_eq!( + resolve_permission_config(&stack, &config, /*sandbox_mode_override*/ None), + Some(expected) + ); + assert_eq!( + resolve_permission_config(&stack, &config, Some(SandboxMode::ReadOnly)), + Some(PermissionConfig::Legacy { + mode: SandboxMode::ReadOnly, + workspace_write: None + }), + ); + } + Ok(()) +} + +#[test] +fn permission_config_translation_uses_executor_sandbox_availability() { + assert_eq!( + translate_legacy_permissions( + SandboxMode::WorkspaceWrite, + /*workspace_write*/ None, + Platform::Windows, + WindowsSandboxLevel::Disabled + ), + PermissionProfile::read_only(), + ); +} diff --git a/codex-rs/core/src/config/config_loader_tests.rs b/codex-rs/core/src/config/config_loader_tests.rs index 23612f7dc9..1ef73a3708 100644 --- a/codex-rs/core/src/config/config_loader_tests.rs +++ b/codex-rs/core/src/config/config_loader_tests.rs @@ -1198,6 +1198,14 @@ async fn managed_preferences_expand_home_directory_in_workspace_write_roots() -> }; let tmp = tempdir()?; + std::fs::write( + tmp.path().join(CONFIG_TOML_FILE), + r#" +default_permissions = ":read-only" +[sandbox_workspace_write] +network_access = true +"#, + )?; let mut loader_overrides = LoaderOverrides::with_managed_config_path_for_tests(tmp.path().join("managed_config.toml")); loader_overrides.managed_preferences_base64 = Some( @@ -1215,12 +1223,20 @@ writable_roots = ["~/code"] .codex_home(tmp.path().to_path_buf()) .fallback_cwd(Some(tmp.path().to_path_buf())) .loader_overrides(loader_overrides) + .cloud_config_bundle(CloudConfigBundleFixture::loader_with_enterprise_config( + "[sandbox_workspace_write]\nexclude_tmpdir_env_var = true", + )) .build() .await?; let expected_root = AbsolutePathBuf::from_absolute_path(home.join("code"))?; match &config.legacy_sandbox_policy() { - SandboxPolicy::WorkspaceWrite { writable_roots, .. } => { + SandboxPolicy::WorkspaceWrite { + writable_roots, + network_access: true, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: false, + } => { assert_eq!( writable_roots .iter() @@ -1232,6 +1248,7 @@ writable_roots = ["~/code"] other => panic!("expected workspace-write policy, got {other:?}"), } + assert_eq!(config.permissions.active_permission_profile(), None); Ok(()) } diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index efcddd3811..9625db49e9 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -19,6 +19,7 @@ use codex_config::FeatureRequirementsToml; use codex_config::ManagedAuthPolicy; use codex_config::McpEnterpriseManagedAuthConfig; use codex_config::McpServerRequirement; +use codex_config::PermissionConfig; use codex_config::PluginRequirementsToml; use codex_config::ProfileV2Name; use codex_config::ResidencyRequirement; @@ -38,7 +39,9 @@ use codex_config::loader::load_config_layers_state; use codex_config::loader::project_trust_key; use codex_config::permissions_toml::PermissionProfileToml; use codex_config::permissions_toml::PermissionsToml; +use codex_config::resolve_permission_config; use codex_config::sandbox_mode_requirement_for_permission_profile; +use codex_config::translate_legacy_permissions; use codex_config::types::ApprovalsReviewer; use codex_config::types::AuthCredentialsStoreMode; use codex_config::types::AuthKeyringBackendKind; @@ -142,6 +145,7 @@ use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::AbsolutePathBufGuard; use codex_utils_path_uri::PathConvention; use codex_utils_path_uri::PathUri; +use codex_utils_path_uri::Platform; use http::HeaderValue; use rmcp::model::ElicitationCapability; use rmcp::model::FormElicitationCapability; @@ -2696,18 +2700,6 @@ fn is_session_layer(source: &ConfigLayerSource) -> bool { matches!(source, ConfigLayerSource::SessionFlags) } -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -enum PermissionConfigSyntax { - Legacy, - Profiles, -} - -#[derive(Debug, Deserialize, Default)] -struct PermissionSelectionToml { - default_permissions: Option, - sandbox_mode: Option, -} - // Resolve the named-profile catalog and selected profile id together. Runtime // profile constraints are applied later after this selection compiles into a // concrete `PermissionProfile`. @@ -2729,68 +2721,16 @@ impl EffectivePermissionSelection<'_> { fn profiles_are_active( &self, default_permissions_override: Option<&str>, - permission_config_syntax: Option, + permission_config: Option>, ) -> bool { self.persisted_profile_id_was_provided || self.requirements_force_profile_selection || default_permissions_override.is_some() - || matches!( - permission_config_syntax, - Some(PermissionConfigSyntax::Profiles) - ) - || permission_config_syntax.is_none() + || matches!(permission_config, Some(PermissionConfig::Named { .. })) + || permission_config.is_none() } } -fn resolve_permission_config_syntax( - config_layer_stack: &ConfigLayerStack, - cfg: &ConfigToml, - sandbox_mode_override: Option, -) -> Option { - if sandbox_mode_override.is_some() { - return Some(PermissionConfigSyntax::Legacy); - } - - let session_flags_select_profiles = config_layer_stack - .layers_high_to_low() - .find(|layer| matches!(layer.name, ConfigLayerSource::SessionFlags)) - .and_then(|layer| { - layer - .config - .clone() - .try_into::() - .ok() - }) - .is_some_and(|selection| selection.default_permissions.is_some()); - if session_flags_select_profiles { - return Some(PermissionConfigSyntax::Profiles); - } - - let mut selection = None; - for layer in config_layer_stack.layers_low_to_high() { - let Ok(layer_selection) = layer.config.clone().try_into::() else { - continue; - }; - - if layer_selection.sandbox_mode.is_some() { - selection = Some(PermissionConfigSyntax::Legacy); - } - if layer_selection.default_permissions.is_some() { - selection = Some(PermissionConfigSyntax::Profiles); - } - } - - selection.or_else(|| { - if cfg.default_permissions.is_some() { - Some(PermissionConfigSyntax::Profiles) - } else if cfg.sandbox_mode.is_some() { - Some(PermissionConfigSyntax::Legacy) - } else { - None - } - }) -} - /// Optional overrides for user configuration (e.g., from CLI flags). #[derive(Default, Debug, Clone)] pub struct ConfigOverrides { @@ -3710,7 +3650,7 @@ impl Config { repo_root.as_ref().map(AbsolutePathBuf::as_path), ) .unwrap_or(ProjectConfig { trust_level: None }); - let permission_config_syntax = resolve_permission_config_syntax( + let permission_config = resolve_permission_config( &config_layer_stack, &cfg, sandbox_mode, @@ -3758,8 +3698,8 @@ impl Config { )?; if effective_permission_selection.has_profiles() && !matches!( - permission_config_syntax, - Some(PermissionConfigSyntax::Legacy) + permission_config, + Some(PermissionConfig::Legacy { .. }) ) && effective_permission_selection.selected_profile_id.is_none() && !effective_permission_selection.requirements_force_profile_selection @@ -3788,7 +3728,7 @@ impl Config { let profiles_are_active = effective_permission_selection.profiles_are_active( default_permissions_override.as_deref(), - permission_config_syntax, + permission_config, ); let prefer_mxc = features.enabled(Feature::PreferMxc) && config_allows_mxc( @@ -3811,8 +3751,8 @@ impl Config { .persisted_profile_id_was_provided || default_permissions_override.is_some() || matches!( - permission_config_syntax, - Some(PermissionConfigSyntax::Profiles) + permission_config, + Some(PermissionConfig::Named { .. }) ); let custom_permission_profiles = permission_profile_catalog_from_permissions( &config_layer_stack, @@ -3824,14 +3764,14 @@ impl Config { .collect(); let using_implicit_builtin_profile = !effective_permission_selection .persisted_profile_id_was_provided - && permission_config_syntax.is_none() + && permission_config.is_none() && effective_permission_selection.selected_profile_id.is_none(); let should_seed_legacy_workspace_roots = effective_permission_selection .selected_profile_id .is_none() && matches!( - permission_config_syntax, - None | Some(PermissionConfigSyntax::Legacy) + permission_config, + None | Some(PermissionConfig::Legacy { .. }) ); let legacy_workspace_roots_explicit = should_seed_legacy_workspace_roots && cfg @@ -3964,14 +3904,22 @@ impl Config { // Derive the old `sandbox_mode` defaults as a profile first, then // keep a legacy-compatible projection only for the remaining code // paths that still speak `SandboxPolicy`. - let mut permission_profile = cfg - .derive_permission_profile( + let mut permission_profile = if let Some(PermissionConfig::Legacy { mode, workspace_write }) = permission_config { + translate_legacy_permissions( + mode, + workspace_write, + Platform::native(), + legacy_windows_sandbox_level, + ) + } else { + cfg.derive_permission_profile( sandbox_mode, legacy_windows_sandbox_level, Some(&active_project), Some(&constrained_permission_profile), ) - .await; + .await + }; // The legacy-derived profiles above are expected to be // representable as `SandboxPolicy`. This guard keeps the old safe // fallback behavior if future changes make this branch derive a diff --git a/codex-rs/tui/src/app/tests/startup_defaults_tests.rs b/codex-rs/tui/src/app/tests/startup_defaults_tests.rs index 349a732708..4511c32c2e 100644 --- a/codex-rs/tui/src/app/tests/startup_defaults_tests.rs +++ b/codex-rs/tui/src/app/tests/startup_defaults_tests.rs @@ -24,7 +24,7 @@ pub(super) async fn run_startup_for_test( selection: SessionSelection, app_server_target: AppServerTarget, ) -> Result { - App::run( + crate::boxed_future!(App::run( tui, initial_backends, config.clone(), @@ -58,7 +58,7 @@ pub(super) async fn run_startup_for_test( crate::startup_draft::tests::quiet_startup_test_pump(), /*managed_worktree*/ None, /*daemon_cli_executable*/ None, - ) + )) .await }