From fd1bf50410623cb25dec8e172ba8ae3ec679397a Mon Sep 17 00:00:00 2001 From: Winston Howes Date: Mon, 24 Aug 2026 22:37:05 +0000 Subject: [PATCH] Harden project config when credential brokering is active (#40490) ## Why Project configuration must not influence credential-provider environment variables or shell startup behavior while credentials are being brokered. ## What changed - Track credential brokering as unconfigured, disabled, or enabled, and apply effective network proxy requirements before sanitizing project layers. - When brokering is enabled, ignore project settings for shell snapshots, profile loading, `ZDOTDIR`, `BASH_ENV`, and credential-provider environment variables. - Preserve those project shell settings when requirements disable credential brokering. ## Testing Added coverage for active and disabled broker states, protected credential environment variables, and shell startup settings. GitOrigin-RevId: 6967cbffdfbb60424a2d2324bad5b91bd2ccfcf6 --- codex-rs/config/src/loader/local.rs | 28 +++++++-- codex-rs/config/src/loader/mod.rs | 88 ++++++++++++++++++++++++----- codex-rs/config/src/loader/tests.rs | 81 ++++++++++++++++++++++---- 3 files changed, 169 insertions(+), 28 deletions(-) diff --git a/codex-rs/config/src/loader/local.rs b/codex-rs/config/src/loader/local.rs index b341734188..05beb80205 100644 --- a/codex-rs/config/src/loader/local.rs +++ b/codex-rs/config/src/loader/local.rs @@ -1,3 +1,5 @@ +use super::CredentialBrokerProjectState; +use super::apply_credential_broker_requirements; use super::credential_broker_trusted_config; use super::discover_project_layers; use super::layer_io; @@ -15,6 +17,7 @@ use crate::ConfigLayerSource; use crate::LoaderOverrides; use crate::RequirementSource; use crate::RequirementsLayerEntry; +use crate::compose_requirements; use crate::default_project_root_markers; use crate::merge_toml_values; use codex_file_system::ExecutorFileSystem; @@ -132,7 +135,10 @@ pub(super) async fn load_local_config_layers_with_overrides( )?; let project_root_markers = project_root_markers_from_config(&discovery_config)? .unwrap_or_else(default_project_root_markers); - let trust_context = project_trust_context( + let requirements = + local_requirements_layers(fs, codex_home.as_path(), overrides, loaded_managed.clone()) + .await?; + let mut trust_context = project_trust_context( fs, &discovery_config, &credential_broker_trusted_config(&discovery_config, &[], &loaded_managed), @@ -142,6 +148,23 @@ pub(super) async fn load_local_config_layers_with_overrides( &user_file, ) .await?; + if trust_context.credential_broker != CredentialBrokerProjectState::Unconfigured { + let broker_requirements = requirements.clone().project(&[ + vec!["features".to_string(), "network_proxy".to_string()], + vec![ + "feature_requirements".to_string(), + "network_proxy".to_string(), + ], + vec!["experimental_network".to_string(), "enabled".to_string()], + ]); + let effective_requirements = + compose_requirements(broker_requirements.layers.into_iter().map(|layer| { + RequirementsLayerEntry::from_toml_value(layer.source, layer.toml) + .with_base_dir(layer.base_dir) + }))? + .unwrap_or_default(); + apply_credential_broker_requirements(&mut trust_context, &effective_requirements); + } let project_layers = discover_project_layers( fs, cwd, @@ -169,9 +192,6 @@ pub(super) async fn load_local_config_layers_with_overrides( ]; append_project_layers(fs, &mut config_layers, project_layers.layers).await?; - let requirements = - local_requirements_layers(fs, codex_home.as_path(), overrides, loaded_managed.clone()) - .await?; append_legacy_config_layers(&mut config_layers, loaded_managed, &codex_home)?; Ok(LocalConfigLayers { diff --git a/codex-rs/config/src/loader/mod.rs b/codex-rs/config/src/loader/mod.rs index ec6ea9e419..48ea0d3bc5 100644 --- a/codex-rs/config/src/loader/mod.rs +++ b/codex-rs/config/src/loader/mod.rs @@ -39,6 +39,7 @@ use crate::thread_config::ThreadConfigLoader; use codex_file_system::ExecutorFileSystem; use codex_git_utils::resolve_root_git_project_for_trust; use codex_network_proxy::credential_broker_provider_context_env_keys; +use codex_network_proxy::is_credential_broker_provider_env_key; use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::config_types::EnvironmentVariablePattern; use codex_protocol::config_types::SandboxMode; @@ -389,7 +390,7 @@ pub async fn load_config_layers_state( return Err(err); } }; - let project_trust_context = match project_trust_context( + let mut project_trust_context = match project_trust_context( fs, &merged_so_far, &credential_broker_trusted_config( @@ -420,6 +421,7 @@ pub async fn load_config_layers_state( return Err(err); } }; + apply_credential_broker_requirements(&mut project_trust_context, &config_requirements_toml); let project_layers = load_project_layers( fs, &cwd, @@ -994,6 +996,38 @@ fn toml_value_from_serializable(value: T) -> io::Result>, projects_trust: std::collections::HashMap, user_config_file: AbsolutePathBuf, - credential_broker_configured: bool, + credential_broker: CredentialBrokerProjectState, credential_broker_binding_env: HashMap, } @@ -1123,13 +1157,15 @@ fn project_layer_entry( fn sanitize_project_config( config: &mut TomlValue, - credential_broker_configured: bool, + credential_broker: CredentialBrokerProjectState, trusted_binding_env: &HashMap, ) -> Vec { let Some(table) = config.as_table_mut() else { return Vec::new(); }; + let credential_broker_configured = + credential_broker != CredentialBrokerProjectState::Unconfigured; let mut ignored_keys = Vec::new(); for key in PROJECT_LOCAL_CONFIG_DENYLIST { if table.remove(*key).is_some() { @@ -1137,6 +1173,11 @@ fn sanitize_project_config( } } if let Some(features) = table.get_mut("features").and_then(TomlValue::as_table_mut) { + if credential_broker == CredentialBrokerProjectState::Enabled + && features.remove("shell_snapshot").is_some() + { + ignored_keys.push("features.shell_snapshot".to_string()); + } if features.remove("respect_system_proxy").is_some() { ignored_keys.push("features.respect_system_proxy".to_string()); } @@ -1159,17 +1200,23 @@ fn sanitize_project_config( } } } - if credential_broker_configured + if credential_broker == CredentialBrokerProjectState::Enabled && let Some(policy) = table .get_mut("shell_environment_policy") .and_then(TomlValue::as_table_mut) { + if policy.remove("experimental_use_profile").is_some() { + ignored_keys.push("shell_environment_policy.experimental_use_profile".to_string()); + } let binding_keys = credential_broker_provider_context_env_keys().collect::>(); if let Some(overrides) = policy.get_mut("set").and_then(TomlValue::as_table_mut) { overrides.retain(|key, _| { - if binding_keys - .iter() - .any(|binding_key| key.eq_ignore_ascii_case(binding_key)) + if key.eq_ignore_ascii_case("ZDOTDIR") + || key.eq_ignore_ascii_case("BASH_ENV") + || is_credential_broker_provider_env_key(key) + || binding_keys + .iter() + .any(|binding_key| key.eq_ignore_ascii_case(binding_key)) { ignored_keys.push(format!("shell_environment_policy.set.{key}")); false @@ -1302,13 +1349,26 @@ async fn project_trust_context( .into_iter() .filter_map(|(key, project)| project.trust_level.map(|trust_level| (key, trust_level))) .collect(); - let credential_broker_configured = trusted_broker_config + let network_proxy = trusted_broker_config .get("features") .and_then(|features| features.get("network_proxy")) - .and_then(TomlValue::as_table) - .is_some_and(|network_proxy| { - network_proxy.get("credential_broker") == Some(&TomlValue::Boolean(true)) - }); + .and_then(TomlValue::as_table); + let credential_broker_configured = network_proxy.is_some_and(|network_proxy| { + network_proxy.get("credential_broker") == Some(&TomlValue::Boolean(true)) + }); + let credential_broker = if credential_broker_configured { + if network_proxy + .and_then(|network_proxy| network_proxy.get("enabled")) + .and_then(TomlValue::as_bool) + .unwrap_or(false) + { + CredentialBrokerProjectState::Enabled + } else { + CredentialBrokerProjectState::Disabled + } + } else { + CredentialBrokerProjectState::Unconfigured + }; let credential_broker_binding_env = if credential_broker_configured { let trusted_policy_overrides = trusted_broker_config .get("shell_environment_policy") @@ -1352,7 +1412,7 @@ async fn project_trust_context( repo_root_lookup_keys, projects_trust, user_config_file: user_config_file.clone(), - credential_broker_configured, + credential_broker, credential_broker_binding_env, }) } @@ -1721,7 +1781,7 @@ async fn discover_project_layers( } let ignored_project_config_keys = sanitize_project_config( &mut config, - trust_context.credential_broker_configured, + trust_context.credential_broker, &trust_context.credential_broker_binding_env, ); if disabled_reason.is_none() && !ignored_project_config_keys.is_empty() { diff --git a/codex-rs/config/src/loader/tests.rs b/codex-rs/config/src/loader/tests.rs index a944bc9d8b..412167fd68 100644 --- a/codex-rs/config/src/loader/tests.rs +++ b/codex-rs/config/src/loader/tests.rs @@ -30,7 +30,7 @@ fn project_config_cannot_override_configured_credential_broker_hosts() { let ignored = sanitize_project_config( &mut config, - /*credential_broker_configured*/ true, + CredentialBrokerProjectState::Enabled, &HashMap::new(), ); @@ -55,12 +55,17 @@ fn project_config_cannot_change_configured_credential_broker_state() { "[features]\nnetwork_proxy = false", "[features.network_proxy]\nenabled = true", "[features.network_proxy]\nenabled = false", + "[features]\nshell_snapshot = true", + "[features]\nshell_snapshot = false", + "[shell_environment_policy]\nexperimental_use_profile = true", + "[shell_environment_policy.set]\nGH_TOKEN = ''", + "[shell_environment_policy.set]\nOPENAI_API_KEY = ''", ] { let mut config: TomlValue = toml::from_str(project_config).expect("valid project config"); let ignored = sanitize_project_config( &mut config, - /*credential_broker_configured*/ true, + CredentialBrokerProjectState::Enabled, &HashMap::new(), ); @@ -75,9 +80,54 @@ fn project_config_cannot_change_configured_credential_broker_state() { .is_some_and(|network_proxy| !network_proxy.contains_key("enabled")) }) ); + assert!( + config + .get("features") + .and_then(|features| features.get("shell_snapshot")) + .is_none() + ); + assert!( + config + .get("shell_environment_policy") + .and_then(|policy| policy.get("experimental_use_profile")) + .is_none() + ); } } +#[test] +fn disabled_credential_broker_preserves_project_shell_settings() { + let mut config: TomlValue = toml::from_str( + "[features]\nnetwork_proxy = true\nshell_snapshot = false\n\ + [shell_environment_policy]\nexperimental_use_profile = true\n\ + [shell_environment_policy.set]\n\ + GH_HOST = 'attacker.example'\n\ + OPENAI_BASE_URL = 'https://project.example/v1'\n\ + ZDOTDIR = '/project-startup'\nBASH_ENV = '/project-startup'", + ) + .expect("valid project config"); + + let ignored = sanitize_project_config( + &mut config, + CredentialBrokerProjectState::Disabled, + &HashMap::new(), + ); + + assert_eq!(ignored, vec!["features.network_proxy".to_string()]); + assert_eq!( + config, + toml::from_str::( + "[features]\nshell_snapshot = false\n\ + [shell_environment_policy]\nexperimental_use_profile = true\n\ + [shell_environment_policy.set]\n\ + GH_HOST = 'attacker.example'\n\ + OPENAI_BASE_URL = 'https://project.example/v1'\n\ + ZDOTDIR = '/project-startup'\nBASH_ENV = '/project-startup'" + ) + .expect("valid expected config") + ); +} + #[test] fn project_environment_filters_preserve_credential_host_bindings() { for (project_config, expected_policy) in [ @@ -107,7 +157,7 @@ fn project_environment_filters_preserve_credential_host_bindings() { assert!( sanitize_project_config( &mut config, - /*credential_broker_configured*/ true, + CredentialBrokerProjectState::Enabled, &HashMap::new(), ) .is_empty() @@ -132,12 +182,16 @@ fn project_environment_filters_preserve_only_trusted_credential_host_bindings() "filters = { '*' = 'exclude' }", "include_only = ['GH_ENTERPRISE_TOKEN']", ] { - let mut project: TomlValue = - toml::from_str(&format!("[shell_environment_policy]\n{project_policy}")) - .expect("valid project config"); + let mut project: TomlValue = toml::from_str(&format!( + "[shell_environment_policy]\n{project_policy}\n\ + [shell_environment_policy.set]\n\ + ZDOTDIR = '/untrusted-project-startup'\n\ + BASH_ENV = '/untrusted-project-startup'" + )) + .expect("valid project config"); sanitize_project_config( &mut project, - /*credential_broker_configured*/ true, + CredentialBrokerProjectState::Enabled, &trusted_binding_env, ); @@ -182,7 +236,7 @@ fn project_config_cannot_bind_permission_shortcuts() { assert_eq!( sanitize_project_config( &mut config, - /*credential_broker_configured*/ false, + CredentialBrokerProjectState::Unconfigured, &HashMap::new(), ), [format!("tui.keymap.chat.{key}")] @@ -829,7 +883,14 @@ async fn local_layers_keep_raw_paths_order_and_legacy_requirements() { ) }; let user_file = codex_home.join(CONFIG_TOML_FILE); - std::fs::write(&user_file, user_config("trusted")).expect("write user config"); + std::fs::write( + &user_file, + format!( + "{}\n[features.network_proxy]\nenabled=true\ncredential_broker=true\n", + user_config("trusted") + ), + ) + .expect("write user config"); let system_file = system_dir.join(CONFIG_TOML_FILE); std::fs::write(&system_file, "model_instructions_file = \"./system.md\"") .expect("write system config"); @@ -847,7 +908,7 @@ async fn local_layers_keep_raw_paths_order_and_legacy_requirements() { let requirements_file = managed_dir.join("requirements.toml"); std::fs::write( &requirements_file, - "allowed_sandbox_modes = [\"read-only\"]\nlog_dir = \"./logs\"", + "allowed_sandbox_modes = [\"future-mode\"]\nlog_dir = \"./logs\"", ) .expect("write system requirements");