From ed4ca07ba68595b41acfc304ab987f348c1c4e98 Mon Sep 17 00:00:00 2001 From: Winston Howes Date: Wed, 9 Sep 2026 17:22:04 +0000 Subject: [PATCH] Handle credential provider source remapping across config layers (#44241) ## What changed - Let higher-priority providers in `features.network_proxy.credentials` displace lower-priority providers with overlapping `env` sources, including in profiles. Preserve inherited settings when providers swap sources, and compare environment names case-insensitively on Windows. - Preserve provider definitions across ordered batch remaps and persist displaced-provider removals without overwriting unrelated sibling updates. - Validate remapped providers using the credential broker's compilation rules before persisting writes, while allowing incomplete drafts and explicit deletions. - Omit displaced providers from config origins and report writes overridden by another provider's source ownership. - Build trusted credential-broker configuration before merging managed layers for project discovery, preserving remapped providers' environment protections. ## Testing Add unit and config RPC regression tests for source precedence, source swaps, ordered edits, concurrent sibling updates, atomic rejection of invalid remaps, persistence, and override reporting. Add a macOS managed-config regression test for remapped provider bindings during project discovery. GitOrigin-RevId: b7d402727acf4e5b0f25946db1cf18811f5e2679 --- codex-rs/Cargo.lock | 1 + codex-rs/app-server/Cargo.toml | 1 + .../app-server/src/config_manager_service.rs | 99 ++- ...config_manager_service_credential_edits.rs | 295 ++++++++ .../src/config_manager_service_tests.rs | 669 ++++++++++++++++++ .../app-server/tests/suite/v2/config_rpc.rs | 417 +++++++++++ codex-rs/config/src/loader/local.rs | 4 +- .../loader/managed_project_discovery_tests.rs | 51 ++ codex-rs/config/src/loader/mod.rs | 13 +- codex-rs/config/src/merge.rs | 32 + codex-rs/config/src/merge_tests.rs | 84 +++ codex-rs/config/src/state.rs | 23 + codex-rs/config/src/state_tests.rs | 38 + .../src/credential_broker/provider_config.rs | 8 + 14 files changed, 1690 insertions(+), 45 deletions(-) create mode 100644 codex-rs/app-server/src/config_manager_service_credential_edits.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index ae499d41a7..5424355fbe 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2212,6 +2212,7 @@ dependencies = [ "codex-model-provider", "codex-model-provider-info", "codex-models-manager", + "codex-network-proxy", "codex-otel", "codex-plugin", "codex-protocol", diff --git a/codex-rs/app-server/Cargo.toml b/codex-rs/app-server/Cargo.toml index 3b5060f097..6965b0c452 100644 --- a/codex-rs/app-server/Cargo.toml +++ b/codex-rs/app-server/Cargo.toml @@ -40,6 +40,7 @@ codex-aws-auth = { workspace = true } codex-cloud-config = { workspace = true } codex-code-mode = { workspace = true } codex-config = { workspace = true } +codex-network-proxy = { workspace = true } codex-connectors = { workspace = true } codex-core = { workspace = true } codex-core-plugins = { workspace = true } diff --git a/codex-rs/app-server/src/config_manager_service.rs b/codex-rs/app-server/src/config_manager_service.rs index 0c5556d89c..cc85d8cf1e 100644 --- a/codex-rs/app-server/src/config_manager_service.rs +++ b/codex-rs/app-server/src/config_manager_service.rs @@ -24,7 +24,6 @@ use codex_config::merge_toml_values; use codex_config::shell_environment_filter_entry; use codex_config::validate_shell_environment_policy_filter_config; use codex_core::config::deserialize_config_toml_with_base; -use codex_core::config::edit::ConfigEdit; use codex_core::config::edit::ConfigEditsBuilder; use codex_core::config::validate_feature_requirements_for_config_toml; use codex_core::path_utils; @@ -42,6 +41,11 @@ use tokio::task; use toml::Value as TomlValue; use toml_edit::Item as TomlItem; +#[path = "config_manager_service_credential_edits.rs"] +mod credential_edits; + +use credential_edits::CredentialProviderEdits; + #[derive(Debug, Error)] pub(crate) enum ConfigManagerError { #[error("{message}")] @@ -254,6 +258,7 @@ impl ConfigManager { } let mut user_config = user_layer.config.clone(); + let mut credential_provider_edits = CredentialProviderEdits::new(&user_config); let mut parsed_segments = Vec::new(); let mut config_edits = Vec::new(); @@ -331,15 +336,6 @@ impl ConfigManager { })?; } - let persist_segments = if matches!(strategy, MergeStrategy::Upsert) - && parsed_value.as_ref().is_some_and(|value| { - shell_environment_policy_representation_switch(&user_config, &segments, value) - }) { - vec!["shell_environment_policy".to_string()] - } else { - segments.clone() - }; - let original_value = value_at_path(&user_config, &persist_segments).cloned(); let structured_feature_toggle = preserves_network_proxy_settings || is_structured_feature_path(&segments) && parsed_value.as_ref().is_some_and(|value| { @@ -350,28 +346,13 @@ impl ConfigManager { }) }); - apply_merge(&mut user_config, &segments, parsed_value.as_ref(), strategy).map_err( - |err| match err { - MergeError::Validation(message) => ConfigManagerError::write( - ConfigWriteErrorCode::ConfigValidationError, - message, - ), - }, - )?; - - let updated_value = value_at_path(&user_config, &persist_segments).cloned(); - if original_value != updated_value { - config_edits.push(match updated_value { - Some(value) => ConfigEdit::SetPath { - segments: persist_segments, - value: toml_value_to_item(&value).map_err(|err| { - ConfigManagerError::anyhow("failed to build config edits", err) - })?, - }, - None => ConfigEdit::ClearPath { - segments: persist_segments, - }, - }); + if let Some(edit) = credential_provider_edits.apply( + &mut user_config, + &segments, + parsed_value.as_ref(), + strategy, + )? { + config_edits.push(edit); } if structured_feature_toggle { @@ -379,6 +360,7 @@ impl ConfigManager { } parsed_segments.push(segments); } + config_edits.extend(credential_provider_edits.remapping_edits(&user_config)?); validate_config(&user_config).map_err(|err| { ConfigManagerError::write( @@ -414,12 +396,20 @@ impl ConfigManager { ) })?; let effective = updated_layers.effective_config(); - validate_config(&effective).map_err(|err| { - ConfigManagerError::write( - ConfigWriteErrorCode::ConfigValidationError, - format!("Invalid configuration: {err}"), - ) - })?; + validate_config(&effective) + .and_then(|()| { + credential_provider_edits.validate_remapping( + &user_layer.config, + &user_config, + &updated_layers, + ) + }) + .map_err(|err| { + ConfigManagerError::write( + ConfigWriteErrorCode::ConfigValidationError, + format!("Invalid configuration: {err}"), + ) + })?; if !config_edits.is_empty() { ConfigEditsBuilder::for_config_path(provided_path.as_path()) @@ -577,6 +567,19 @@ fn structured_feature_depth(segments: &[String]) -> Option { is_structured_feature_path(&segments[..depth]).then_some(depth) } +fn credential_providers_path(segments: &[String]) -> Option<&[String]> { + match segments { + [features, proxy, credentials, ..] + if features == "features" + && proxy == "network_proxy" + && credentials == "credentials" => + { + Some(&segments[..3]) + } + _ => None, + } +} + fn apply_merge( root: &mut TomlValue, segments: &[String], @@ -608,7 +611,8 @@ fn apply_merge( if preserves_structured_feature_config || matches!(strategy, MergeStrategy::Upsert) - && (shell_environment_policy_representation_switch(root, segments, value) + && (credential_providers_path(segments).is_some() + || shell_environment_policy_representation_switch(root, segments, value) || (matches!(value_at_path(root, segments), Some(TomlValue::Table(_))) && matches!(value, TomlValue::Table(_)))) { @@ -866,6 +870,23 @@ fn find_effective_layer( layers: &ConfigLayerStack, segments: &[String], ) -> Option { + if credential_providers_path(segments).is_some() { + // A provider can be displaced by another provider's environment sources, + // even when that layer never writes the edited path itself. + let mut merged = TomlValue::Table(Default::default()); + let mut origin = None; + for layer in layers.layers_low_to_high() { + let previous = value_at_path(&merged, segments).cloned(); + merge_toml_values(&mut merged, &layer.config); + if value_at_path(&layer.config, segments).is_some() + || previous.as_ref() != value_at_path(&merged, segments) + { + origin = Some(layer.metadata()); + } + } + return origin; + } + for layer in layers.layers_high_to_low() { if value_at_semantic_path(&layer.config, segments).is_some() { return Some(layer.metadata()); diff --git a/codex-rs/app-server/src/config_manager_service_credential_edits.rs b/codex-rs/app-server/src/config_manager_service_credential_edits.rs new file mode 100644 index 0000000000..d902b134da --- /dev/null +++ b/codex-rs/app-server/src/config_manager_service_credential_edits.rs @@ -0,0 +1,295 @@ +//! Prepare config edits and preserve provider definitions across source remapping in a write batch. +//! Explicit replacements and deletions remain ordered; only automatic sibling eviction is +//! ignored when retaining definitions for a later source upsert. + +use super::ConfigManagerError; +use super::MergeError; +use super::apply_merge; +use super::shell_environment_policy_representation_switch; +use super::sparse_overlay; +use super::toml_value_to_item; +use super::value_at_path; +use codex_app_server_protocol::ConfigWriteErrorCode; +use codex_app_server_protocol::MergeStrategy; +use codex_config::ConfigLayerStack; +use codex_config::merge_toml_values; +use codex_core::config::edit::ConfigEdit; +use codex_network_proxy::CredentialProviderConfig; +use std::collections::BTreeMap; +use std::collections::BTreeSet; +use toml::Value as TomlValue; + +const PROVIDERS_PATH: [&str; 3] = ["features", "network_proxy", "credentials"]; + +pub(super) struct CredentialProviderEdits { + definitions: TomlValue, + displaced_provider_ids: BTreeSet, +} + +impl CredentialProviderEdits { + pub(super) fn new(config: &TomlValue) -> Self { + Self { + definitions: providers(config) + .cloned() + .unwrap_or_else(|| TomlValue::Table(Default::default())), + displaced_provider_ids: BTreeSet::new(), + } + } + + pub(super) fn apply( + &mut self, + config: &mut TomlValue, + segments: &[String], + value: Option<&TomlValue>, + strategy: MergeStrategy, + ) -> Result, ConfigManagerError> { + let persist_segments = if matches!(strategy, MergeStrategy::Upsert) + && value.is_some_and(|value| { + shell_environment_policy_representation_switch(config, segments, value) + }) { + vec!["shell_environment_policy".to_string()] + } else { + segments.to_vec() + }; + let original_value = value_at_path(config, &persist_segments).cloned(); + self.apply_provider_merge(config, segments, value, strategy) + .map_err(|err| match err { + MergeError::Validation(message) => { + ConfigManagerError::write(ConfigWriteErrorCode::ConfigValidationError, message) + } + })?; + if original_value.as_ref() == value_at_path(config, &persist_segments) { + Ok(None) + } else { + config_edit_at_path(config, persist_segments).map(Some) + } + } + + pub(super) fn remapping_edits( + &self, + config: &TomlValue, + ) -> Result, ConfigManagerError> { + // Use final definitions so later explicit edits and restored providers retain batch order. + self.displaced_provider_ids + .iter() + .map(|id| { + let mut segments = PROVIDERS_PATH.map(str::to_string).to_vec(); + segments.push(id.clone()); + config_edit_at_path(config, segments) + }) + .collect() + } + + fn apply_provider_merge( + &mut self, + config: &mut TomlValue, + segments: &[String], + value: Option<&TomlValue>, + strategy: MergeStrategy, + ) -> Result<(), MergeError> { + if !segments + .iter() + .zip(PROVIDERS_PATH) + .all(|(segment, expected)| segment == expected) + { + return apply_merge(config, segments, value, strategy).map(|_| ()); + } + + let previous_ids = providers(config) + .and_then(TomlValue::as_table) + .filter(|_| matches!(strategy, MergeStrategy::Upsert) && value.is_some()) + .into_iter() + .flat_map(|table| table.keys().cloned()) + .collect::>(); + let mut overlay = value.map(|value| sparse_overlay(segments, value)); + let restored_ids = overlay + .as_ref() + .and_then(providers) + .and_then(TomlValue::as_table) + .into_iter() + .flatten() + .filter(|(id, update)| { + matches!(strategy, MergeStrategy::Upsert) + && update.get("env").is_some() + && self.definitions.get(id.as_str()).is_some() + && self.definitions.get(id.as_str()) + != providers(config).and_then(|current| current.get(id.as_str())) + }) + .map(|(id, _)| id.clone()) + .collect::>(); + + if segments.len() > PROVIDERS_PATH.len() { + apply_merge( + &mut self.definitions, + &segments[PROVIDERS_PATH.len()..], + value, + strategy.clone(), + )?; + } else { + let update = value.and_then(|value| { + PROVIDERS_PATH[segments.len()..] + .iter() + .try_fold(value, |value, key| value.get(*key)) + }); + if let Some(update) = update { + if matches!(strategy, MergeStrategy::Upsert) { + merge_toml_values(&mut self.definitions, update); + } else { + self.definitions = update.clone(); + } + } else if value.is_none() + || matches!(strategy, MergeStrategy::Replace) + && !value.is_some_and(TomlValue::is_bool) + { + self.definitions = TomlValue::Table(Default::default()); + } + } + + if !restored_ids.is_empty() + && let Some(overlay) = overlay.as_mut() + && let Some(updates) = overlay + .get_mut("features") + .and_then(|features| features.get_mut("network_proxy")) + .and_then(|proxy| proxy.get_mut("credentials")) + .and_then(TomlValue::as_table_mut) + { + for id in restored_ids { + if let Some(definition) = self.definitions.get(&id) { + self.displaced_provider_ids.insert(id.clone()); + updates.insert(id, definition.clone()); + } + } + merge_toml_values(config, overlay); + } else { + apply_merge(config, segments, value, strategy)?; + } + self.displaced_provider_ids.extend( + previous_ids + .into_iter() + .filter(|id| providers(config).is_none_or(|table| table.get(id).is_none())), + ); + Ok(()) + } + + pub(super) fn validate_remapping( + &self, + original_user_config: &TomlValue, + user_config: &TomlValue, + layers: &ConfigLayerStack, + ) -> anyhow::Result<()> { + let mut previous = TomlValue::Table(Default::default()); + let mut effective = previous.clone(); + let mut user_reached = false; + let mut to_validate = BTreeMap::new(); + for layer in layers.layers_low_to_high() { + let is_user = layers + .get_active_user_layer() + .is_some_and(|user| std::ptr::eq(layer, user)); + merge_toml_values( + &mut previous, + if is_user { + original_user_config + } else { + &layer.config + }, + ); + merge_toml_values(&mut effective, &layer.config); + user_reached |= is_user; + if user_reached { + for (id, old) in providers(&previous) + .and_then(TomlValue::as_table) + .into_iter() + .flatten() + { + // Deleting an override and continuing an incomplete draft remain allowed. + if providers(user_config).is_none_or(|map| map.get(id).is_none()) { + continue; + } + let Some(current) = providers(&effective).and_then(|map| map.get(id)) else { + continue; + }; + if current == old { + continue; + } + let Ok(old): Result = old.clone().try_into() + else { + continue; + }; + if old.validate(id).is_ok() { + to_validate.insert(id.clone(), current.clone()); + } + } + // Compare final ownership, not temporary source moves within the write batch. + for (id, definition) in providers(&effective) + .and_then(TomlValue::as_table) + .into_iter() + .flatten() + { + let receives_source = providers(&previous) + .and_then(TomlValue::as_table) + .into_iter() + .flatten() + .any(|(old_id, old)| { + // Retained definitions exclude explicit deletes, not automatic eviction. + (self.definitions.get(old_id).is_some() + || self.definitions.get(id).is_some() + && providers(original_user_config).and_then(|map| map.get(id)) + != providers(user_config).and_then(|map| map.get(id))) + && sources(old).any(|old_source| { + !providers(&effective) + .and_then(|map| map.get(old_id)) + .is_some_and(|current| { + sources(current) + .any(|source| same_source(source, old_source)) + }) + && sources(definition) + .any(|source| same_source(source, old_source)) + }) + }); + if receives_source || to_validate.contains_key(id) { + // A higher same-ID layer may complete a fragment; a different ID may not. + to_validate.insert(id.clone(), definition.clone()); + } + } + } + } + for (id, definition) in to_validate { + let provider: CredentialProviderConfig = definition.try_into()?; + provider.validate(&id)?; + } + Ok(()) + } +} + +fn config_edit_at_path( + config: &TomlValue, + segments: Vec, +) -> Result { + match value_at_path(config, &segments) { + Some(value) => Ok(ConfigEdit::SetPath { + value: toml_value_to_item(value) + .map_err(|err| ConfigManagerError::anyhow("failed to build config edits", err))?, + segments, + }), + None => Ok(ConfigEdit::ClearPath { segments }), + } +} + +fn sources(provider: &TomlValue) -> impl Iterator { + provider + .get("env") + .and_then(TomlValue::as_array) + .into_iter() + .flatten() + .filter_map(TomlValue::as_str) +} + +fn same_source(left: &str, right: &str) -> bool { + left == right || cfg!(windows) && left.eq_ignore_ascii_case(right) +} + +fn providers(config: &TomlValue) -> Option<&TomlValue> { + PROVIDERS_PATH + .iter() + .try_fold(config, |value, key| value.get(*key)) +} diff --git a/codex-rs/app-server/src/config_manager_service_tests.rs b/codex-rs/app-server/src/config_manager_service_tests.rs index cf388933ff..6f8016476d 100644 --- a/codex-rs/app-server/src/config_manager_service_tests.rs +++ b/codex-rs/app-server/src/config_manager_service_tests.rs @@ -15,6 +15,605 @@ use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use tempfile::tempdir; +#[test] +fn provider_batch_remapping_preserves_ordered_edits() { + let initial = serde_json::json!({ + "features": { "network_proxy": { "credentials": { + "a": { "env": ["A_AUTH"], "patterns": ["alpha"], "url_prefixes": ["https://a.example"] }, + "b": { "env": ["B_AUTH"], "patterns": ["bravo"], "url_prefixes": ["https://b.example"], "auth": ["bearer"] }, + } } }, + }); + let retained = serde_json::json!({ + "env": ["A_AUTH"], "patterns": ["bravo"], "url_prefixes": ["https://b.example"], + }); + let mut changed_auth = retained.clone(); + changed_auth["auth"] = serde_json::json!(["token"]); + for (path, value, strategy, expected_b) in [ + ( + "b.auth", + serde_json::json!(["token"]), + MergeStrategy::Upsert, + changed_auth, + ), + ("b.auth", JsonValue::Null, MergeStrategy::Replace, retained), + ( + "b", + JsonValue::Null, + MergeStrategy::Replace, + serde_json::json!({ "env": ["A_AUTH"] }), + ), + ( + "b", + serde_json::json!({ "patterns": ["replacement"], "url_prefixes": ["https://new.example"] }), + MergeStrategy::Replace, + serde_json::json!({ "env": ["A_AUTH"], "patterns": ["replacement"], "url_prefixes": ["https://new.example"] }), + ), + ( + "", + JsonValue::Null, + MergeStrategy::Replace, + serde_json::json!({ "env": ["A_AUTH"] }), + ), + ] { + let mut config: TomlValue = serde_json::from_value(initial.clone()).unwrap(); + let tmp = tempdir().unwrap(); + let file = tmp.path().join(CONFIG_TOML_FILE); + std::fs::write(&file, toml::to_string(&config).unwrap()).unwrap(); + let mut providers = CredentialProviderEdits::new(&config); + let mut persistence = Vec::new(); + let path = if path.is_empty() { + "features.network_proxy.credentials".to_string() + } else { + format!("features.network_proxy.credentials.{path}") + }; + for (key_path, value, strategy) in [ + ( + "features.network_proxy.credentials.a.env", + serde_json::json!(["B_AUTH"]), + MergeStrategy::Upsert, + ), + (path.as_str(), value, strategy), + ( + "features.network_proxy.credentials.b.env", + serde_json::json!(["A_AUTH"]), + MergeStrategy::Upsert, + ), + ] { + persistence.extend( + providers + .apply( + &mut config, + &parse_key_path(key_path).unwrap(), + parse_value(value).unwrap().as_ref(), + strategy, + ) + .unwrap(), + ); + } + persistence.extend(providers.remapping_edits(&config).unwrap()); + ConfigEditsBuilder::for_config_path(&file) + .with_edits(persistence) + .apply_blocking() + .unwrap(); + assert_eq!( + toml::from_str::(&std::fs::read_to_string(&file).unwrap()).unwrap(), + config + ); + let mut expected = serde_json::json!({ "b": expected_b }); + if path != "features.network_proxy.credentials" { + expected["a"] = initial["features"]["network_proxy"]["credentials"]["a"].clone(); + expected["a"]["env"] = serde_json::json!(["B_AUTH"]); + } + assert_eq!( + config["features"]["network_proxy"]["credentials"], + serde_json::from_value::(expected).unwrap(), + "{path}" + ); + } +} + +#[tokio::test] +async fn provider_edits_preserve_another_writers_sibling_update() -> Result<()> { + let initial: TomlValue = toml::from_str( + r#" +[features.network_proxy.credentials.a] +env = ['A_AUTH'] +patterns = ['^alpha_[a-z]{8}$'] +url_prefixes = ['https://a.example'] +auth = ['bearer'] +[features.network_proxy.credentials.b] +env = ['B_AUTH'] +patterns = ['^bravo_[a-z]{8}$'] +url_prefixes = ['https://b.example'] +auth = ['bearer'] +[features.network_proxy.credentials.c] +env = ['C_AUTH'] +patterns = ['^charlie_[a-z]{8}$'] +url_prefixes = ['https://c.example'] +auth = ['bearer'] +"#, + )?; + for updates in [ + vec![("a.auth", serde_json::json!(["token"]))], + vec![("a.env", serde_json::json!(["C_AUTH"]))], + vec![ + ("a.env", serde_json::json!(["C_AUTH"])), + ("c.env", serde_json::json!(["A_AUTH"])), + ], + ] { + let tmp = tempdir()?; + let file = tmp.path().join(CONFIG_TOML_FILE); + std::fs::write(&file, toml::to_string(&initial)?)?; + let mut config = initial.clone(); + let mut providers = CredentialProviderEdits::new(&config); + let mut persistence = Vec::new(); + for (path, value) in updates { + persistence.extend(providers.apply( + &mut config, + &parse_key_path(&format!("features.network_proxy.credentials.{path}")).unwrap(), + parse_value(value).unwrap().as_ref(), + MergeStrategy::Upsert, + )?); + } + persistence.extend(providers.remapping_edits(&config)?); + + // W1 has prepared its edits; W2 updates B before W1's persistence reread. + ConfigManager::without_managed_config_for_tests(tmp.path().to_path_buf()) + .write_value(ConfigValueWriteParams { + file_path: None, + key_path: "features.network_proxy.credentials.b.auth".to_string(), + value: serde_json::json!(["token"]), + merge_strategy: MergeStrategy::Upsert, + expected_version: None, + }) + .await?; + ConfigEditsBuilder::for_config_path(&file) + .with_edits(persistence) + .apply() + .await?; + + config["features"]["network_proxy"]["credentials"]["b"]["auth"] = + TomlValue::Array(vec![TomlValue::String("token".to_string())]); + assert_eq!( + toml::from_str::(&std::fs::read_to_string(&file)?)?, + config + ); + } + Ok(()) +} + +#[test] +fn provider_remapping_respects_lower_layer_eviction() -> Result<()> { + let tmp = tempdir()?; + let file = AbsolutePathBuf::try_from(tmp.path().join(CONFIG_TOML_FILE))?; + let complete = |id: &str| -> TomlValue { + toml::from_str(&format!( + "[features.network_proxy.credentials.{id}]\nenv = ['VENDOR_PASSWORD']\npatterns = ['^pin_[a-z]{{8}}$']\nurl_prefixes = ['https://old.example']\n" + )).unwrap() + }; + // The middle layer must not let an evicted definition's fields reappear later. + for (lower_id, middle, user, valid, strategy, move_source) in [ + ("a_working", None, "", false, MergeStrategy::Upsert, false), + ("a_working", None, "", false, MergeStrategy::Replace, false), + ("z_new", None, "", true, MergeStrategy::Upsert, false), + ("z_new", None, "", true, MergeStrategy::Replace, false), + ( + "z_new", + Some("a_working"), + "", + false, + MergeStrategy::Upsert, + false, + ), + ( + "z_new", + Some("a_working"), + "a_working", + false, + MergeStrategy::Upsert, + false, + ), + ("a_working", None, "", false, MergeStrategy::Upsert, true), + ( + "a_working", + None, + "a_working", + false, + MergeStrategy::Upsert, + true, + ), + ] { + let mut user_config = if user.is_empty() { + TomlValue::Table(Default::default()) + } else { + complete(user) + }; + let original_user_config = user_config.clone(); + let mut edits = CredentialProviderEdits::new(&user_config); + if move_source { + edits + .apply( + &mut user_config, + &parse_key_path("features.network_proxy.credentials.a_working.env").unwrap(), + Some(&TomlValue::Array(vec![TomlValue::String( + "OTHER_PASSWORD".into(), + )])), + MergeStrategy::Upsert, + ) + .unwrap(); + } + edits + .apply( + &mut user_config, + &parse_key_path("features.network_proxy.credentials.z_new.env").unwrap(), + Some(&TomlValue::Array(vec![TomlValue::String( + "VENDOR_PASSWORD".into(), + )])), + strategy, + ) + .unwrap(); + let mut layers = vec![ConfigLayerEntry::new( + ConfigLayerSource::System { file: file.clone() }, + complete(lower_id), + )]; + if let Some(id) = middle { + layers.push(ConfigLayerEntry::new( + ConfigLayerSource::EnterpriseManaged { + id: "test".into(), + name: "test".into(), + }, + complete(id), + )); + } + layers.push(ConfigLayerEntry::new( + ConfigLayerSource::User { + file: file.clone(), + profile: None, + }, + user_config.clone(), + )); + let layers = ConfigLayerStack::new(layers, Default::default(), Default::default())?; + assert_eq!( + edits + .validate_remapping(&original_user_config, &user_config, &layers) + .is_ok(), + valid, + "{lower_id} / {middle:?} / {user}", + ); + } + for pattern in ["[", "^pin_[a-z]{8}$"] { + for reverse in [false, true] { + let original = complete("a_working"); + let mut config = original.clone(); + let mut edits = CredentialProviderEdits::new(&config); + let mut updates = [ + ("a_working.env", serde_json::json!(["OTHER_PASSWORD"])), + ("z_new.patterns", serde_json::json!([pattern])), + ]; + if reverse { + updates.reverse(); + } + for (path, value) in updates { + edits + .apply( + &mut config, + &parse_key_path(&format!("features.network_proxy.credentials.{path}")) + .unwrap(), + parse_value(value).unwrap().as_ref(), + MergeStrategy::Upsert, + ) + .unwrap(); + } + let layers = ConfigLayerStack::new( + vec![ + ConfigLayerEntry::new( + ConfigLayerSource::System { file: file.clone() }, + complete("z_new"), + ), + ConfigLayerEntry::new( + ConfigLayerSource::User { + file: file.clone(), + profile: None, + }, + config.clone(), + ), + ], + Default::default(), + Default::default(), + )?; + assert_eq!( + edits + .validate_remapping(&original, &config, &layers) + .is_ok(), + pattern != "[", + "{pattern}, reverse={reverse}" + ); + } + } + Ok(()) +} + +#[test] +fn provider_updates_preserve_working_definitions_and_drafts() -> Result<()> { + let tmp = tempdir()?; + let file = AbsolutePathBuf::try_from(tmp.path().join(CONFIG_TOML_FILE))?; + let pattern = "^pin_[a-z]{8}$"; + for (lower_pattern, initial, path, value, valid, managed_pattern) in [ + ( + pattern, + serde_json::json!({}), + "a.patterns", + serde_json::json!(["["]), + false, + None, + ), + ( + pattern, + serde_json::json!({}), + "a.auth", + serde_json::json!(["token"]), + true, + None, + ), + ( + "[", + serde_json::json!({"a": {"patterns": [pattern]}}), + "a", + JsonValue::Null, + true, + None, + ), + ( + pattern, + serde_json::json!({"a": {"patterns": ["["]}}), + "a.patterns", + serde_json::json!(["("]), + true, + None, + ), + ( + pattern, + serde_json::json!({"b": {"env": ["NEW_PASSWORD"]}}), + "b.patterns", + serde_json::json!(["["]), + true, + None, + ), + ( + pattern, + serde_json::json!({"b": {"env": ["NEW_PASSWORD"]}}), + "b", + serde_json::json!({"env": ["NEW_PASSWORD"], "patterns": [pattern]}), + true, + None, + ), + ( + pattern, + serde_json::json!({"b": {"env": ["NEW_PASSWORD"]}}), + "", + serde_json::json!({"b": {"env": ["NEW_PASSWORD"], "patterns": [pattern]}}), + true, + None, + ), + ( + "[", + serde_json::json!({"a": {"env": ["VENDOR_PASSWORD"]}}), + "a.env", + serde_json::json!([]), + false, + Some(pattern), + ), + ] { + let lower: TomlValue = serde_json::from_value(serde_json::json!({ + "features": {"network_proxy": {"credentials": {"a": { + "env": ["VENDOR_PASSWORD"], "patterns": [lower_pattern], + "url_prefixes": ["https://api.vendor.example"], + }}}} + }))?; + let original: TomlValue = serde_json::from_value(serde_json::json!({ + "features": {"network_proxy": {"credentials": initial}} + }))?; + let mut config = original.clone(); + let mut edits = CredentialProviderEdits::new(&config); + let key_path = if path.is_empty() { + "features.network_proxy.credentials".to_string() + } else { + format!("features.network_proxy.credentials.{path}") + }; + edits + .apply( + &mut config, + &parse_key_path(&key_path).unwrap(), + parse_value(value).unwrap().as_ref(), + MergeStrategy::Replace, + ) + .unwrap(); + let mut layers = vec![ + ConfigLayerEntry::new(ConfigLayerSource::System { file: file.clone() }, lower), + ConfigLayerEntry::new( + ConfigLayerSource::User { + file: file.clone(), + profile: None, + }, + config.clone(), + ), + ]; + if let Some(pattern) = managed_pattern { + layers.push(ConfigLayerEntry::new( + ConfigLayerSource::LegacyManagedConfigTomlFromFile { file: file.clone() }, + serde_json::from_value(serde_json::json!({"features": {"network_proxy": {"credentials": {"a": {"patterns": [pattern]}}}}}))?, + )); + } + let layers = ConfigLayerStack::new(layers, Default::default(), Default::default())?; + assert_eq!( + edits + .validate_remapping(&original, &config, &layers) + .is_ok(), + valid, + "{path}" + ); + } + Ok(()) +} + +#[test] +fn provider_draft_remapping_uses_final_source_ownership() -> Result<()> { + let tmp = tempdir()?; + let file = AbsolutePathBuf::try_from(tmp.path().join(CONFIG_TOML_FILE))?; + let config = |providers| -> TomlValue { + serde_json::from_value(serde_json::json!({ + "features": {"network_proxy": {"credentials": providers}} + })) + .unwrap() + }; + let complete = serde_json::json!({ + "env": ["VENDOR_PASSWORD"], "patterns": ["^pin_[a-z]{8}$"], + "url_prefixes": ["https://api.vendor.example"], + }); + let lower = config(serde_json::json!({"b": complete})); + let inherited_draft = config(serde_json::json!({ + "a": complete, "b": {"url_prefixes": []}, + })); + let inherited_only_draft = config(serde_json::json!({ + "b": {"env": ["VENDOR_PASSWORD"]}, + })); + let mut multi_source = serde_json::json!({"a": complete, "c": complete}); + multi_source["a"]["env"] = serde_json::json!(["VENDOR_PASSWORD", "VENDOR_ALIAS"]); + multi_source["c"]["env"] = serde_json::json!(["OTHER_PASSWORD"]); + let mut actual = Vec::new(); + let mut expected = Vec::new(); + for (name, lower, original, updates, strategy, valid) in [ + ( + "automatic eviction reveals an inherited draft", + inherited_only_draft.clone(), + config(multi_source.clone()), + vec![("c.env", serde_json::json!(["VENDOR_ALIAS"]))], + MergeStrategy::Upsert, + false, + ), + ( + "explicit deletion after remapping remains allowed", + inherited_only_draft.clone(), + config(multi_source), + vec![ + ("c.env", serde_json::json!(["VENDOR_ALIAS"])), + ("a", JsonValue::Null), + ], + MergeStrategy::Upsert, + true, + ), + ( + "inherited-only draft receives the source", + inherited_only_draft.clone(), + config(serde_json::json!({"a": complete})), + vec![("a.env", serde_json::json!(["OTHER_PASSWORD"]))], + MergeStrategy::Replace, + false, + ), + ( + "deletion may reveal an inherited-only draft", + inherited_only_draft.clone(), + config(serde_json::json!({"a": complete})), + vec![("a", JsonValue::Null)], + MergeStrategy::Replace, + true, + ), + ( + "deleting both providers may reveal an inherited draft", + inherited_only_draft, + config(serde_json::json!({"a": complete, "b": {}})), + vec![("a", JsonValue::Null), ("b", JsonValue::Null)], + MergeStrategy::Replace, + true, + ), + ( + "inherited draft receives the source", + lower.clone(), + inherited_draft.clone(), + vec![("a.env", serde_json::json!(["OTHER_PASSWORD"]))], + MergeStrategy::Replace, + false, + ), + ( + "whole provider deletion remains allowed", + lower.clone(), + inherited_draft.clone(), + vec![("a", JsonValue::Null)], + MergeStrategy::Replace, + true, + ), + ( + "deletion with an unchanged draft edit", + lower.clone(), + inherited_draft.clone(), + vec![ + ("a", JsonValue::Null), + ("b.url_prefixes", serde_json::json!([])), + ], + MergeStrategy::Replace, + true, + ), + ( + "batch completes the recipient", + lower, + inherited_draft, + vec![ + ("a.env", serde_json::json!(["OTHER_PASSWORD"])), + ("b.url_prefixes", serde_json::json!(["https://new.example"])), + ], + MergeStrategy::Replace, + true, + ), + ( + "batch restores the draft source", + config(serde_json::json!({})), + config(serde_json::json!({"b": {"env": ["DRAFT_PASSWORD"]}})), + vec![ + ("b", serde_json::json!({"patterns": ["^pin_[a-z]{8}$"]})), + ("b.env", serde_json::json!(["DRAFT_PASSWORD"])), + ], + MergeStrategy::Replace, + true, + ), + ] { + let mut config = original.clone(); + let mut edits = CredentialProviderEdits::new(&config); + for (path, value) in updates { + edits + .apply( + &mut config, + &parse_key_path(&format!("features.network_proxy.credentials.{path}")).unwrap(), + parse_value(value).unwrap().as_ref(), + strategy.clone(), + ) + .unwrap(); + } + let layers = ConfigLayerStack::new( + vec![ + ConfigLayerEntry::new(ConfigLayerSource::System { file: file.clone() }, lower), + ConfigLayerEntry::new( + ConfigLayerSource::User { + file: file.clone(), + profile: None, + }, + config.clone(), + ), + ], + Default::default(), + Default::default(), + )?; + actual.push(( + name, + edits + .validate_remapping(&original, &config, &layers) + .is_ok(), + )); + expected.push((name, valid)); + } + assert_eq!(actual, expected); + Ok(()) +} + #[test] fn toml_value_to_item_handles_nested_config_tables() { let config = r#" @@ -764,6 +1363,76 @@ async fn managed_auth_policy_survives_unusable_requirements_file_changes() -> Re Ok(()) } +#[tokio::test] +async fn provider_remapping_validates_fragments_even_when_source_is_overridden() -> Result<()> { + for (managed_provider, strategy) in [ + (None, MergeStrategy::Upsert), + (None, MergeStrategy::Replace), + (Some("z_new"), MergeStrategy::Upsert), + (Some("z_new"), MergeStrategy::Replace), + (Some("z_managed"), MergeStrategy::Upsert), + (Some("z_managed"), MergeStrategy::Replace), + ] { + let tmp = tempdir()?; + let initial = r#"[features.network_proxy.credentials.a_working] +env = ["VENDOR_PASSWORD"] +patterns = ["^pin_[a-z]{8}$"] +url_prefixes = ["https://old.example"] +"#; + std::fs::write(tmp.path().join(CONFIG_TOML_FILE), initial)?; + let managed_path = tmp.path().join("managed.toml"); + if let Some(managed_provider) = managed_provider { + std::fs::write( + &managed_path, + format!( + r#"[features.network_proxy.credentials.{managed_provider}] +env = ["VENDOR_PASSWORD"] +patterns = ["^pin_[a-z]{{8}}$"] +url_prefix_from_env = "VENDOR_ENDPOINT" +"# + ), + )?; + } + let service = ConfigManager::new_for_tests( + tmp.path().to_path_buf(), + vec![], + LoaderOverrides::with_managed_config_path_for_tests(managed_path), + CloudConfigBundleLoader::default(), + ); + let result = service + .write_value(ConfigValueWriteParams { + file_path: None, + key_path: match strategy { + MergeStrategy::Upsert => "features.network_proxy.credentials.z_new.env", + MergeStrategy::Replace => "features.network_proxy.credentials", + } + .to_string(), + value: match strategy { + MergeStrategy::Upsert => serde_json::json!(["VENDOR_PASSWORD"]), + MergeStrategy::Replace => { + serde_json::json!({"z_new": {"env": ["VENDOR_PASSWORD"]}}) + } + }, + merge_strategy: strategy, + expected_version: None, + }) + .await; + if managed_provider == Some("z_new") { + result?; + } else { + assert_eq!( + result.unwrap_err().write_error_code(), + Some(ConfigWriteErrorCode::ConfigValidationError) + ); + assert_eq!( + std::fs::read_to_string(tmp.path().join(CONFIG_TOML_FILE))?, + initial + ); + } + } + Ok(()) +} + #[tokio::test] async fn invalid_user_value_rejected_even_if_overridden_by_managed() { let tmp = tempdir().expect("tempdir"); diff --git a/codex-rs/app-server/tests/suite/v2/config_rpc.rs b/codex-rs/app-server/tests/suite/v2/config_rpc.rs index 18d480ff7d..cc0d48f643 100644 --- a/codex-rs/app-server/tests/suite/v2/config_rpc.rs +++ b/codex-rs/app-server/tests/suite/v2/config_rpc.rs @@ -1546,6 +1546,423 @@ model = "gpt-old" Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn config_provider_write_reports_source_displacement() -> Result<()> { + let codex_home = TempDir::new()?; + write_config(&codex_home, "")?; + let mut app_server = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .without_auto_env() + .with_args(&[ + "-c", + "features.network_proxy.credentials.enforced.env=['VENDOR_PASSWORD']", + "-c", + "features.network_proxy.credentials.enforced.patterns=['^pin_[a-z]{8}$']", + "-c", + "features.network_proxy.credentials.enforced.url_prefixes=['https://fixed.example']", + ]) + .build_initialized_with_timeout(DEFAULT_READ_TIMEOUT) + .await?; + + let mut provider = json!({ + "env": ["VENDOR_PASSWORD"], + "patterns": ["^pin_[a-z]{8}$"], + "url_prefixes": ["https://user.example"], + "auth": ["bearer"], + }); + for (suffix, value, overridden) in [ + ("", provider.clone(), true), + (".url_prefixes", json!(["https://updated.example"]), true), + (".env", json!(["OTHER_PASSWORD"]), false), + ] { + let write_id = app_server + .send_config_value_write_request(ConfigValueWriteParams { + file_path: None, + key_path: format!("features.network_proxy.credentials.user_provider{suffix}"), + value: value.clone(), + merge_strategy: MergeStrategy::Upsert, + expected_version: None, + }) + .await?; + let write: ConfigWriteResponse = + timeout(DEFAULT_READ_TIMEOUT, app_server.read_response(write_id)).await??; + let expected = overridden.then_some((ConfigLayerSource::SessionFlags, json!(null))); + assert_eq!( + ( + write.status, + write + .overridden_metadata + .map(|meta| { (meta.overriding_layer.name, meta.effective_value) }) + ), + ( + if overridden { + WriteStatus::OkOverridden + } else { + WriteStatus::Ok + }, + expected, + ), + ); + + if let Some(field) = suffix.strip_prefix('.') { + provider[field] = value; + } + let saved: toml::Value = toml::from_str(&std::fs::read_to_string( + codex_home.path().join("config.toml"), + )?)?; + assert_eq!( + serde_json::to_value( + &saved["features"]["network_proxy"]["credentials"]["user_provider"] + )?, + provider, + ); + let read_id = app_server + .send_config_read_request(ConfigReadParams { + include_layers: false, + cwd: None, + }) + .await?; + let read: ConfigReadResponse = + timeout(DEFAULT_READ_TIMEOUT, app_server.read_response(read_id)).await??; + assert_eq!( + read.config.additional["features"]["network_proxy"]["credentials"].get("user_provider"), + (!overridden).then_some(&provider), + ); + } + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn config_provider_field_removal_uses_higher_same_id_definition() -> Result<()> { + let codex_home = TempDir::new()?; + write_config( + &codex_home, + r#"[features.network_proxy.credentials.vendor] +env = ["VENDOR_PASSWORD"] +patterns = ["^pin_[a-z]{8}$"] +url_prefixes = ["https://fixed.example"] +"#, + )?; + let mut app_server = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .with_args(&[ + "-c", + "features.network_proxy.credentials.vendor.url_prefixes=['https://fixed.example']", + ]) + .build_initialized_with_timeout(DEFAULT_READ_TIMEOUT) + .await?; + let id = app_server + .send_config_value_write_request(ConfigValueWriteParams { + file_path: None, + key_path: "features.network_proxy.credentials.vendor.url_prefixes".to_string(), + value: json!(null), + merge_strategy: MergeStrategy::Replace, + expected_version: None, + }) + .await?; + let write: ConfigWriteResponse = + timeout(DEFAULT_READ_TIMEOUT, app_server.read_response(id)).await??; + assert_eq!(write.status, WriteStatus::OkOverridden); + + let saved: toml::Value = toml::from_str(&std::fs::read_to_string( + codex_home.path().join("config.toml"), + )?)?; + assert_eq!( + serde_json::to_value(&saved["features"]["network_proxy"]["credentials"]["vendor"])?, + json!({ "env": ["VENDOR_PASSWORD"], "patterns": ["^pin_[a-z]{8}$"] }), + ); + let id = app_server + .send_config_read_request(ConfigReadParams { + include_layers: false, + cwd: None, + }) + .await?; + let read: ConfigReadResponse = + timeout(DEFAULT_READ_TIMEOUT, app_server.read_response(id)).await??; + assert_eq!( + read.config.additional["features"]["network_proxy"]["credentials"]["vendor"], + json!({ + "env": ["VENDOR_PASSWORD"], "patterns": ["^pin_[a-z]{8}$"], + "url_prefixes": ["https://fixed.example"], "auth": [], + }), + ); + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn config_provider_remapping_rejects_invalid_replacements_atomically() -> Result<()> { + let codex_home = TempDir::new()?; + let initial = r#"[features.network_proxy] +enabled = true +credential_broker = true +[features.network_proxy.credentials.a_working] +env = ["VENDOR_PASSWORD"] +patterns = ["^pin_[a-z]{8}$"] +url_prefixes = ["https://api.vendor.example"] +"#; + write_config(&codex_home, initial)?; + let mut app_server = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .without_auto_env() + .build_initialized_with_timeout(DEFAULT_READ_TIMEOUT) + .await?; + for (key_path, merge_strategy, replacement) in [ + json!({ "env": ["VENDOR_PASSWORD"] }), + json!({ "env": ["VENDOR_PASSWORD"], "patterns": ["["], "url_prefixes": ["https://api.vendor.example"] }), + json!({ "env": ["VENDOR_PASSWORD"], "patterns": ["^pin_[a-z]{8}$"], "url_prefixes": ["http://api.vendor.example"] }), + json!({ "env": ["VENDOR_PASSWORD"], "patterns": ["^pin_[a-z]{8}$"], "url_prefixes": ["https://api.vendor.example"], "auth": ["header"] }), + ] + .into_iter() + .flat_map(|replacement| [ + ("features.network_proxy.credentials.z_new", MergeStrategy::Upsert, replacement.clone()), + ("features.network_proxy.credentials.a_working", MergeStrategy::Replace, replacement.clone()), + ("features.network_proxy.credentials", MergeStrategy::Replace, json!({ "a_working": replacement })), + ]) + .chain([ + ("features.network_proxy.credentials.a_working.patterns", MergeStrategy::Upsert, json!(["["])), + ("features.network_proxy.credentials.a_working.patterns", MergeStrategy::Replace, json!(["["])), + ("features.network_proxy.credentials.a_working.patterns", MergeStrategy::Replace, json!(null)), + ("features.network_proxy.credentials.a_working.url_prefixes", MergeStrategy::Upsert, json!(["http://api.vendor.example"])), + ]) { + let id = app_server + .send_config_value_write_request(ConfigValueWriteParams { + file_path: None, + key_path: key_path.to_string(), + value: replacement, + merge_strategy, + expected_version: None, + }) + .await?; + let error: JSONRPCError = timeout( + DEFAULT_READ_TIMEOUT, + app_server.read_stream_until_error_message(RequestId::Integer(id)), + ) + .await??; + assert_eq!( + error + .error + .data + .as_ref() + .and_then(|data| data.get("config_write_error_code")), + Some(&json!("configValidationError")) + ); + assert_eq!( + std::fs::read_to_string(codex_home.path().join("config.toml"))?, + initial + ); + } + let id = app_server + .send_config_batch_write_request(ConfigBatchWriteParams { + file_path: None, + edits: [ + ("a_working", "OTHER_PASSWORD"), + ("b_incomplete", "VENDOR_PASSWORD"), + ] + .into_iter() + .map(|(provider, source)| ConfigEdit { + key_path: format!("features.network_proxy.credentials.{provider}.env"), + value: json!([source]), + merge_strategy: MergeStrategy::Upsert, + }) + .collect(), + expected_version: None, + reload_user_config: false, + }) + .await?; + let error: JSONRPCError = timeout( + DEFAULT_READ_TIMEOUT, + app_server.read_stream_until_error_message(RequestId::Integer(id)), + ) + .await??; + assert_eq!( + error + .error + .data + .as_ref() + .and_then(|data| data.get("config_write_error_code")), + Some(&json!("configValidationError")) + ); + assert_eq!( + std::fs::read_to_string(codex_home.path().join("config.toml"))?, + initial + ); + let id = app_server + .send_config_batch_write_request(ConfigBatchWriteParams { + file_path: None, + edits: [ + ("env", json!(["VENDOR_PASSWORD"])), + ("patterns", json!(["^pin_[a-z]{8}$"])), + ("url_prefix_from_env", json!("VENDOR_ENDPOINT")), + ] + .into_iter() + .map(|(field, value)| ConfigEdit { + key_path: format!("features.network_proxy.credentials.z_new.{field}"), + value, + merge_strategy: MergeStrategy::Upsert, + }) + .collect(), + expected_version: None, + reload_user_config: false, + }) + .await?; + let written: ConfigWriteResponse = + timeout(DEFAULT_READ_TIMEOUT, app_server.read_response(id)).await??; + assert_eq!(written.status, WriteStatus::Ok); + let id = app_server + .send_config_read_request(ConfigReadParams { + include_layers: false, + cwd: None, + }) + .await?; + let read: ConfigReadResponse = + timeout(DEFAULT_READ_TIMEOUT, app_server.read_response(id)).await??; + assert_eq!( + read.config.additional["features"]["network_proxy"]["credentials"], + json!({ + "z_new": { "env": ["VENDOR_PASSWORD"], "patterns": ["^pin_[a-z]{8}$"], "url_prefix_from_env": "VENDOR_ENDPOINT", "url_prefixes": [], "auth": [] }, + }) + ); + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn config_provider_upsert_persists_displaced_siblings() -> Result<()> { + let upsert = |path: &str, value| ConfigEdit { + key_path: format!("features.network_proxy.credentials{path}"), + value, + merge_strategy: MergeStrategy::Upsert, + }; + for (mut edits, remapped_source) in [ + ( + vec![upsert(".z_new", json!({ "env": ["VENDOR_PASSWORD"] }))], + None, + ), + (vec![upsert(".z_new.env", json!(["VENDOR_PASSWORD"]))], None), + ( + vec![upsert( + "", + json!({ "z_new": { "env": ["VENDOR_PASSWORD"] } }), + )], + None, + ), + ( + vec![upsert( + "", + json!({ + "a_old": { "env": ["NEW_PASSWORD"] }, + "z_new": { "env": ["VENDOR_PASSWORD"] }, + }), + )], + Some("NEW_PASSWORD"), + ), + ( + vec![ + upsert(".z_new.env", json!(["VENDOR_PASSWORD"])), + upsert(".a_old.env", json!(["OTHER_PASSWORD"])), + ], + Some("OTHER_PASSWORD"), + ), + ( + vec![ + upsert(".z_new", json!({ "env": ["VENDOR_PASSWORD"] })), + ConfigEdit { + key_path: "model".to_string(), + value: json!("gpt-test"), + merge_strategy: MergeStrategy::Replace, + }, + upsert("", json!({ "a_old": { "env": ["OTHER_PASSWORD"] } })), + ], + Some("OTHER_PASSWORD"), + ), + ] { + let codex_home = TempDir::new()?; + write_config( + &codex_home, + r#"[features.network_proxy] +enabled = true +credential_broker = true + +[features.network_proxy.credentials.a_old] +env = ["VENDOR_PASSWORD"] +patterns = ["^old_[a-z]{8}$"] +url_prefixes = ["https://old.example"] +auth = ["token"] + +[features.network_proxy.credentials.z_new] +env = ["OTHER_PASSWORD"] +patterns = ["^new_[a-z]{8}$"] +url_prefixes = ["https://new.example"] +auth = ["bearer"] +"#, + )?; + let mut app_server = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .without_auto_env() + .build_initialized_with_timeout(DEFAULT_READ_TIMEOUT) + .await?; + + let mut expected = json!({ + "z_new": { + "auth": ["bearer"], + "env": ["VENDOR_PASSWORD"], + "patterns": ["^new_[a-z]{8}$"], + "url_prefixes": ["https://new.example"], + } + }); + if let Some(source) = remapped_source { + expected["a_old"] = json!({ + "auth": ["token"], + "env": [source], + "patterns": ["^old_[a-z]{8}$"], + "url_prefixes": ["https://old.example"], + }); + } + + let write_id = if edits.len() == 1 { + let edit = edits.pop().unwrap(); + app_server + .send_config_value_write_request(ConfigValueWriteParams { + file_path: None, + key_path: edit.key_path, + value: edit.value, + merge_strategy: edit.merge_strategy, + expected_version: None, + }) + .await? + } else { + app_server + .send_config_batch_write_request(ConfigBatchWriteParams { + file_path: None, + edits, + expected_version: None, + reload_user_config: false, + }) + .await? + }; + let write: ConfigWriteResponse = + timeout(DEFAULT_READ_TIMEOUT, app_server.read_response(write_id)).await??; + assert_eq!(write.status, WriteStatus::Ok); + + let read_id = app_server + .send_config_read_request(ConfigReadParams { + include_layers: false, + cwd: None, + }) + .await?; + let read: ConfigReadResponse = + timeout(DEFAULT_READ_TIMEOUT, app_server.read_response(read_id)).await??; + assert_eq!( + read.config.additional["features"]["network_proxy"]["credentials"], + expected + ); + assert_eq!( + read.origins["features.network_proxy.credentials.z_new.env.0"].version, + write.version + ); + } + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn config_value_write_updates_desktop_settings() -> Result<()> { let temp_dir = TempDir::new()?; diff --git a/codex-rs/config/src/loader/local.rs b/codex-rs/config/src/loader/local.rs index 05beb80205..c235166cc2 100644 --- a/codex-rs/config/src/loader/local.rs +++ b/codex-rs/config/src/loader/local.rs @@ -126,6 +126,8 @@ pub(super) async fn load_local_config_layers_with_overrides( let mut discovery_config = TomlValue::Table(toml::map::Map::new()); merge_toml_values(&mut discovery_config, &system.toml); merge_toml_values(&mut discovery_config, &user.toml); + let trusted_broker_config = + credential_broker_trusted_config(&discovery_config, &[], &loaded_managed); // Managed file and MDM values also govern the project boundary and trust. // Only this snapshot is resolved; the returned local layers stay raw. project_discovery::merge_managed_config_for_discovery( @@ -141,7 +143,7 @@ pub(super) async fn load_local_config_layers_with_overrides( let mut trust_context = project_trust_context( fs, &discovery_config, - &credential_broker_trusted_config(&discovery_config, &[], &loaded_managed), + &trusted_broker_config, cwd, &project_root_markers, codex_home.as_path(), diff --git a/codex-rs/config/src/loader/managed_project_discovery_tests.rs b/codex-rs/config/src/loader/managed_project_discovery_tests.rs index 4ce8b846e0..3974cdce6f 100644 --- a/codex-rs/config/src/loader/managed_project_discovery_tests.rs +++ b/codex-rs/config/src/loader/managed_project_discovery_tests.rs @@ -262,6 +262,57 @@ async fn managed_project_discovery_uses_managed_project_trust() -> anyhow::Resul Ok(()) } +#[cfg(target_os = "macos")] +#[tokio::test] +async fn managed_project_discovery_preserves_remapped_provider_bindings() -> anyhow::Result<()> { + use base64::Engine; + use base64::prelude::BASE64_STANDARD; + + let mut fixture = Fixture::new()?; + let providers = "[features.network_proxy.credentials]\n\ + a = { env = ['A_AUTH'], patterns = ['^pin_[a-z]{8}$'], url_prefixes = ['https://a.example'] }\n\ + b = { env = ['B_AUTH'], patterns = ['^pin_[a-z]{8}$'], url_prefix_from_env = 'B_ENDPOINT' }\n"; + std::fs::write( + fixture.home.join("config.toml"), + format!( + "{}\n[features.network_proxy]\nenabled = true\ncredential_broker = true\n\ + {providers}\n[shell_environment_policy.set]\nB_ENDPOINT = 'https://b.example'\n", + fixture.trust("trusted") + ), + )?; + std::fs::write( + &fixture.managed, + "[features.network_proxy.credentials.a]\nenv = ['A_AUTH']\n", + )?; + fixture.overrides.managed_preferences_base64 = Some(BASE64_STANDARD.encode( + "[features.network_proxy.credentials]\na = { env = ['B_AUTH'] }\nb = { env = ['A_AUTH'] }\n", + )); + std::fs::write( + fixture.cwd.join(".codex/config.toml"), + "[shell_environment_policy.set]\nB_ENDPOINT = 'https://attacker.example'\nTOOL_MODE = 'project'\n", + )?; + + let (canonical, local) = fixture.load().await?; + let mut local_effective = TomlValue::Table(toml::map::Map::new()); + for layer in local.config.layers { + merge_toml_values(&mut local_effective, &layer.toml); + } + let mut expected: TomlValue = toml::from_str(providers)?; + let credentials = &mut expected["features"]["network_proxy"]["credentials"]; + credentials["a"]["env"] = TomlValue::try_from(["B_AUTH"])?; + credentials["b"]["env"] = TomlValue::try_from(["A_AUTH"])?; + let expected_policy: TomlValue = + toml::from_str("[set]\nB_ENDPOINT = 'https://b.example'\nTOOL_MODE = 'project'\n")?; + for effective in [canonical.effective_config(), local_effective] { + assert_eq!( + &effective["features"]["network_proxy"]["credentials"], + &*credentials + ); + assert_eq!(effective["shell_environment_policy"], expected_policy); + } + Ok(()) +} + #[cfg(target_os = "macos")] #[tokio::test] async fn managed_project_discovery_mdm_overrides_file_markers() -> anyhow::Result<()> { diff --git a/codex-rs/config/src/loader/mod.rs b/codex-rs/config/src/loader/mod.rs index 962c8d0e8a..35f042a48a 100644 --- a/codex-rs/config/src/loader/mod.rs +++ b/codex-rs/config/src/loader/mod.rs @@ -366,6 +366,11 @@ pub async fn load_config_layers_state( if let Some(cli_overrides_layer) = cli_overrides_layer.as_ref() { merge_toml_values(&mut merged_so_far, cli_overrides_layer); } + let trusted_broker_config = credential_broker_trusted_config( + &merged_so_far, + &thread_config_layers, + &loaded_config_layers, + ); // Managed config wins over CLI config. Apply it before choosing the // project root and trust, but keep its final layers above project config. project_discovery::merge_managed_config_for_discovery( @@ -390,11 +395,7 @@ pub async fn load_config_layers_state( let mut project_trust_context = match project_trust_context( fs, &merged_so_far, - &credential_broker_trusted_config( - &merged_so_far, - &thread_config_layers, - &loaded_config_layers, - ), + &trusted_broker_config, &cwd, &project_root_markers, codex_home, @@ -1357,6 +1358,8 @@ async fn project_trust_context( }) } +// Start before managed layers are merged: replaying credential source remaps +// can discard provider fields needed to protect their environment bindings. fn credential_broker_trusted_config( merged_config: &TomlValue, thread_config_layers: &[ConfigLayerEntry], diff --git a/codex-rs/config/src/merge.rs b/codex-rs/config/src/merge.rs index 722e344f6f..15fde6a37c 100644 --- a/codex-rs/config/src/merge.rs +++ b/codex-rs/config/src/merge.rs @@ -105,6 +105,38 @@ fn merge_toml_values_at_path(base: &mut TomlValue, overlay: &TomlValue, path: &m normalize_case_insensitive_keys(base_table); normalize_case_insensitive_keys(&mut overlay_table); } + if path.split_last().is_some_and(|(field, feature_path)| { + field == "credentials" && is_structured_feature_path(feature_path) + }) { + for (provider_id, provider) in &overlay_table { + let Some(overlay_sources) = provider.get("env").and_then(TomlValue::as_array) + else { + continue; + }; + base_table.retain(|existing_id, existing| { + existing_id == provider_id + || !overlay_table + .get(existing_id) + .and_then(|provider| provider.get("env")) + .or_else(|| existing.get("env")) + .and_then(TomlValue::as_array) + .is_some_and(|existing_sources| { + existing_sources.iter().filter_map(TomlValue::as_str).any( + |existing_source| { + overlay_sources.iter().filter_map(TomlValue::as_str).any( + |overlay_source| { + existing_source == overlay_source + || cfg!(windows) + && existing_source + .eq_ignore_ascii_case(overlay_source) + }, + ) + }, + ) + }) + }); + } + } for (key, value) in overlay_table { path.push(key.clone()); if let Some(existing) = base_table.get_mut(&key) { diff --git a/codex-rs/config/src/merge_tests.rs b/codex-rs/config/src/merge_tests.rs index d0bf6a4d78..024d2639fd 100644 --- a/codex-rs/config/src/merge_tests.rs +++ b/codex-rs/config/src/merge_tests.rs @@ -312,6 +312,90 @@ fn network_proxy_feature_overrides_preserve_credential_broker_configuration() { assert_eq!(base, expected); } +#[test] +fn higher_priority_credential_provider_replaces_overlapping_sources() { + for (feature_path, base_source, overlay_source, keep_default) in [ + ("features", "VENDOR_TOKEN", "VENDOR_TOKEN", false), + ( + "profiles.work.features", + "VENDOR_TOKEN", + "VENDOR_TOKEN", + false, + ), + ("features", "vendor_token", "VENDOR_TOKEN", !cfg!(windows)), + ] { + let default_provider = format!( + "[{feature_path}.network_proxy.credentials.default]\n\ + env = ['{base_source}']\n\ + url_prefixes = ['https://default.example']\n" + ); + let shared_provider = format!( + "[{feature_path}.network_proxy.credentials.shared]\n\ + env = ['SHARED_TOKEN']\n\ + patterns = ['shared_[a-z]+']\n" + ); + let mut base = parse_toml(&format!("{default_provider}\n{shared_provider}")); + let overlay = parse_toml(&format!( + "[{feature_path}.network_proxy.credentials.override]\n\ + env = ['{overlay_source}']\n\ + url_prefixes = ['https://override.example']\n\ + [{feature_path}.network_proxy.credentials.shared]\n\ + url_prefixes = ['https://shared.example']\n" + )); + + merge_toml_values(&mut base, &overlay); + + let retained_default = if keep_default { &default_provider } else { "" }; + let expected = parse_toml(&format!( + "{retained_default}\n\ + [{feature_path}.network_proxy.credentials.override]\n\ + env = ['{overlay_source}']\n\ + url_prefixes = ['https://override.example']\n\ + [{feature_path}.network_proxy.credentials.shared]\n\ + env = ['SHARED_TOKEN']\n\ + patterns = ['shared_[a-z]+']\n\ + url_prefixes = ['https://shared.example']\n" + )); + assert_eq!(base, expected, "feature path: {feature_path}"); + } +} + +#[test] +fn credential_provider_remapping_preserves_inherited_settings() { + let base = parse_toml( + r#" +[features.network_proxy.credentials.a] +env = ["OLD_AUTH"] +patterns = ["acme_[a-z]{24}"] +url_prefixes = ["https://a.example"] +auth = ["token"] +[features.network_proxy.credentials.b] +env = ["OTHER_AUTH"] +patterns = ["other_[a-z]{24}"] +url_prefixes = ["https://b.example"] +auth = ["bearer"] +"#, + ); + + for new_a in ["NEW_AUTH", "OTHER_AUTH"] { + let mut merged = base.clone(); + let mut expected = base.clone(); + let providers = &mut expected["features"]["network_proxy"]["credentials"]; + providers["a"]["env"] = TomlValue::Array(vec![new_a.into()]); + providers["b"]["env"] = TomlValue::Array(vec!["OLD_AUTH".into()]); + let overlay = parse_toml(&format!( + "[features.network_proxy.credentials.a]\n\ + env = ['{new_a}']\n\ + [features.network_proxy.credentials.b]\n\ + env = ['OLD_AUTH']\n" + )); + + merge_toml_values(&mut merged, &overlay); + + assert_eq!(merged, expected); + } +} + /// Repeated opaque desktop overrides continue to replace their previous value. #[test] fn multi_agent_v2_cli_compatibility_excludes_opaque_desktop_paths() { diff --git a/codex-rs/config/src/state.rs b/codex-rs/config/src/state.rs index ea0283313b..2bab7280b5 100644 --- a/codex-rs/config/src/state.rs +++ b/codex-rs/config/src/state.rs @@ -466,12 +466,35 @@ impl ConfigLayerStack { pub fn origins(&self) -> HashMap { let mut origins = HashMap::new(); let mut path = Vec::new(); + let mut provider_paths = vec!["features.network_proxy.credentials.".to_string()]; for layer in self.layers_low_to_high() { let config = normalized_with_key_aliases(&layer.config, &[]); + if let Some(profiles) = config.get("profiles").and_then(TomlValue::as_table) { + provider_paths.extend( + profiles + .keys() + .map(|name| format!("profiles.{name}.features.network_proxy.credentials.")), + ); + } record_origins(&config, &layer.metadata(), &mut path, &mut origins); } + if let Some(layer) = self.layers_low_to_high().next_back() { + let effective = self.effective_config(); + let mut effective_origins = HashMap::new(); + record_origins( + &effective, + &layer.metadata(), + &mut path, + &mut effective_origins, + ); + origins.retain(|path, _| { + !provider_paths.iter().any(|prefix| path.starts_with(prefix)) + || effective_origins.contains_key(path) + }); + } + origins } diff --git a/codex-rs/config/src/state_tests.rs b/codex-rs/config/src/state_tests.rs index 45fa8b9518..8dad010f4a 100644 --- a/codex-rs/config/src/state_tests.rs +++ b/codex-rs/config/src/state_tests.rs @@ -78,6 +78,44 @@ fn origins_attribute_multi_agent_v2_enabled_to_overriding_boolean_layer() { ); } +#[test] +fn origins_omit_displaced_credential_providers() { + let temp_dir = TempDir::new().expect("tempdir"); + for scope in ["", "profiles.work."] { + let user = ConfigLayerEntry::new( + ConfigLayerSource::User { + file: test_user_config_path(&temp_dir, "config.toml"), + profile: None, + }, + toml::from_str(&format!( + "[{scope}features.network_proxy.credentials.user_provider]\nenv = ['VENDOR_TOKEN']\n", + )) + .expect("user config"), + ); + let session = ConfigLayerEntry::new( + ConfigLayerSource::SessionFlags, + toml::from_str(&format!( + "[{scope}features.network_proxy.credentials.session_provider]\nenv = ['VENDOR_TOKEN']\n", + )) + .expect("session config"), + ); + let metadata = session.metadata(); + let stack = ConfigLayerStack::new( + vec![user, session], + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + ) + .expect("valid layers"); + assert_eq!( + stack.origins(), + HashMap::from([( + format!("{scope}features.network_proxy.credentials.session_provider.env.0"), + metadata + )]) + ); + } +} + #[test] fn enabled_layers_validate_shell_environment_policy() { let layer = ConfigLayerEntry::new( diff --git a/codex-rs/network-proxy/src/credential_broker/provider_config.rs b/codex-rs/network-proxy/src/credential_broker/provider_config.rs index 7c9cb17bd7..566fa22780 100644 --- a/codex-rs/network-proxy/src/credential_broker/provider_config.rs +++ b/codex-rs/network-proxy/src/credential_broker/provider_config.rs @@ -21,6 +21,14 @@ pub struct CredentialProviderConfig { pub prefix: Option, } +impl CredentialProviderConfig { + /// Validate a complete provider definition using the broker's compilation rules. + pub fn validate(&self, id: &str) -> anyhow::Result<()> { + super::configured::ConfiguredCredentialProvider::compile(id, self)?; + Ok(()) + } +} + /// Authentication formats supported by declarative credential providers. #[derive(Clone, Copy, Debug, Deserialize, Eq, JsonSchema, PartialEq, Serialize)] #[serde(rename_all = "snake_case")]