From c3d5e4418f05908ffeb68f6257fbacbb1e65e607 Mon Sep 17 00:00:00 2001 From: Winston Howes Date: Mon, 24 Aug 2026 19:44:19 +0000 Subject: [PATCH] Add credential brokering to network proxy feature config (#40466) ## What changed - Add `features.network_proxy.credential_broker` and preserve structured network proxy settings when toggling the feature through config APIs. - Protect broker enablement and provider context variables from project config, while carrying trusted context into filtered shell environments. - Propagate brokered credentials through shell snapshots and restore real credentials for unsandboxed escalated commands. - Normalize broker context environment keys case-insensitively on Windows and avoid conflicting remote managed-network enforcement when no proxy launch configuration is available. ## Testing - Cover config layering, editing, project sanitization, Windows environment handling, shell snapshots, escalated execution, and end-to-end GitHub credential brokering in the zsh-fork runtime. GitOrigin-RevId: 500e51e804e1988022052ccfe00b8fac5e495f4f --- .../app-server/src/config_manager_service.rs | 101 +++++++-- .../src/config_manager_service_tests.rs | 195 +++++++++++++++-- codex-rs/config/src/fingerprint.rs | 10 +- codex-rs/config/src/lib.rs | 1 + codex-rs/config/src/loader/local.rs | 2 + codex-rs/config/src/loader/mod.rs | 177 ++++++++++++++- codex-rs/config/src/loader/tests.rs | 204 +++++++++++++++++- codex-rs/config/src/merge.rs | 36 ++-- codex-rs/config/src/merge_tests.rs | 25 +++ codex-rs/config/src/overrides.rs | 6 +- codex-rs/core/config.schema.json | 3 + .../core/src/config/config_loader_tests.rs | 22 +- codex-rs/core/src/config/edit.rs | 36 ++-- codex-rs/core/src/config/edit_tests.rs | 31 +++ codex-rs/core/src/config/mod.rs | 4 + codex-rs/core/src/config/permissions.rs | 3 + codex-rs/core/src/tools/runtimes/mod.rs | 7 +- codex-rs/core/src/tools/runtimes/mod_tests.rs | 66 +++++- .../runtimes/zsh_fork/unix_escalation.rs | 22 +- .../zsh_fork/unix_escalation_tests.rs | 28 ++- .../core/src/unified_exec/process_manager.rs | 3 + codex-rs/core/tests/suite/approvals.rs | 83 +++++++ codex-rs/features/src/feature_configs.rs | 2 + .../network-proxy/src/credential_broker.rs | 5 + .../src/credential_broker/providers.rs | 2 +- codex-rs/network-proxy/src/lib.rs | 1 + codex-rs/protocol/src/shell_environment.rs | 17 ++ 27 files changed, 1000 insertions(+), 92 deletions(-) diff --git a/codex-rs/app-server/src/config_manager_service.rs b/codex-rs/app-server/src/config_manager_service.rs index 63b6ed7775..05a5f0b7fa 100644 --- a/codex-rs/app-server/src/config_manager_service.rs +++ b/codex-rs/app-server/src/config_manager_service.rs @@ -19,6 +19,7 @@ use codex_config::ConfigLayerStack; use codex_config::ConfigRequirementsToml; use codex_config::ShellEnvironmentPolicyFilterRepresentation; use codex_config::config_toml::ConfigToml; +use codex_config::is_structured_feature_path; use codex_config::merge_toml_values; use codex_config::shell_environment_filter_entry; use codex_config::validate_shell_environment_policy_filter_config; @@ -312,6 +313,22 @@ impl ConfigManager { { segments[2] = pattern; } + if cfg!(windows) + && let [policy, field, key] = segments.as_slice() + && policy == "shell_environment_policy" + && field == "set" + && let Some(existing) = user_config + .get(policy) + .and_then(|policy| policy.get(field)) + .and_then(TomlValue::as_table) + .and_then(|overrides| { + overrides + .keys() + .find(|candidate| candidate.eq_ignore_ascii_case(key)) + }) + { + segments[2].clone_from(existing); + } if !value.is_null() { match segments.as_slice() { [segment] if segment == "profile" => { @@ -329,9 +346,28 @@ impl ConfigManager { _ => {} } } - let parsed_value = parse_value(value).map_err(|message| { - ConfigManagerError::write(ConfigWriteErrorCode::ConfigValidationError, message) - })?; + let preserves_network_proxy_settings = value.is_null() + && is_structured_feature_path(&segments) + && segments + .last() + .is_some_and(|feature| feature == "network_proxy") + && { + let has_broker_settings = |config: &TomlValue| { + value_at_path(config, &segments) + .and_then(TomlValue::as_table) + .is_some_and(|feature| feature.contains_key("credential_broker")) + }; + has_broker_settings(&user_config) + || !has_broker_settings(&user_layer.config) + && has_broker_settings(&layers.effective_config()) + }; + let parsed_value = if preserves_network_proxy_settings { + Some(TomlValue::Boolean(false)) + } else { + parse_value(value).map_err(|message| { + ConfigManagerError::write(ConfigWriteErrorCode::ConfigValidationError, message) + })? + }; if matches!(strategy, MergeStrategy::Upsert) && let Some(value) = parsed_value.as_ref() && matches!(segments.as_slice(), [policy, ..] if policy == "shell_environment_policy") @@ -354,6 +390,15 @@ impl ConfigManager { 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| { + value.is_bool() + || matches!(strategy, MergeStrategy::Upsert) + && value.as_table().is_some_and(|table| { + table.len() == 1 && table.contains_key("enabled") + }) + }); apply_merge(&mut user_config, &segments, parsed_value.as_ref(), strategy).map_err( |err| match err { @@ -379,6 +424,9 @@ impl ConfigManager { }); } + if structured_feature_toggle { + segments.push("enabled".to_string()); + } parsed_segments.push(segments); } @@ -569,6 +617,16 @@ enum MergeError { Validation(String), } +fn structured_feature_depth(segments: &[String]) -> Option { + let depth = match segments { + [profiles, _, _, _, ..] if profiles == "profiles" => 4, + [_, _, ..] => 2, + _ => return None, + }; + + is_structured_feature_path(&segments[..depth]).then_some(depth) +} + fn apply_merge( root: &mut TomlValue, segments: &[String], @@ -585,17 +643,8 @@ fn apply_merge( )); }; - let multi_agent_v2_feature_depth = match segments { - [features, feature, ..] if features == "features" && feature == "multi_agent_v2" => Some(2), - [profiles, _, features, feature, ..] - if profiles == "profiles" && features == "features" && feature == "multi_agent_v2" => - { - Some(4) - } - _ => None, - }; - let preserves_multi_agent_v2_feature_config = - multi_agent_v2_feature_depth.is_some_and(|feature_depth| { + let preserves_structured_feature_config = + structured_feature_depth(segments).is_some_and(|feature_depth| { match value_at_path(root, &segments[..feature_depth]) { Some(TomlValue::Boolean(_)) => { segments.len() > feature_depth || matches!(value, TomlValue::Table(_)) @@ -607,7 +656,7 @@ fn apply_merge( } }); - if preserves_multi_agent_v2_feature_config + if preserves_structured_feature_config || matches!(strategy, MergeStrategy::Upsert) && (shell_environment_policy_representation_switch(root, segments, value) || (matches!(value_at_path(root, segments), Some(TomlValue::Table(_))) @@ -770,20 +819,26 @@ fn value_at_path<'a>(root: &'a TomlValue, segments: &[String]) -> Option<&'a Tom fn value_at_semantic_path<'a>(root: &'a TomlValue, segments: &[String]) -> Option<&'a TomlValue> { shell_environment_filter_entry(root, segments) .map(|(_, value)| value) + .or_else(|| match segments { + [policy, field, key] + if cfg!(windows) && policy == "shell_environment_policy" && field == "set" => + { + root.get(policy)? + .get(field)? + .as_table()? + .iter() + .find(|(candidate, _)| candidate.eq_ignore_ascii_case(key)) + .map(|(_, value)| value) + } + _ => None, + }) .or_else(|| value_at_path(root, segments)) .or_else(|| { let (field, parents) = segments.split_last()?; if field != "enabled" { return None; } - let is_multi_agent_v2_feature = match parents { - [features, feature] => features == "features" && feature == "multi_agent_v2", - [profiles, _, features, feature] => { - profiles == "profiles" && features == "features" && feature == "multi_agent_v2" - } - _ => false, - }; - if !is_multi_agent_v2_feature { + if structured_feature_depth(parents) != Some(parents.len()) { return None; } let feature = value_at_path(root, parents)?; 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 1631e71468..580ecd417b 100644 --- a/codex-rs/app-server/src/config_manager_service_tests.rs +++ b/codex-rs/app-server/src/config_manager_service_tests.rs @@ -1198,12 +1198,16 @@ async fn multi_agent_v2_boolean_layer_owns_enabled_origin_and_overrides() { let user_path = tmp.path().join(CONFIG_TOML_FILE); std::fs::write( &user_path, - "[features.multi_agent_v2]\nenabled = true\nsubagent_usage_hint_text = \"keep\"\n", + "[features.multi_agent_v2]\nenabled = true\nsubagent_usage_hint_text = \"keep\"\n\n[features.network_proxy]\nenabled = true\ncredential_broker = true\n", ) .expect("user config"); let managed_path = tmp.path().join("managed_config.toml"); - std::fs::write(&managed_path, "[features]\nmulti_agent_v2 = false\n").expect("managed config"); + std::fs::write( + &managed_path, + "[features]\nmulti_agent_v2 = false\nnetwork_proxy = false\n", + ) + .expect("managed config"); let managed_file = AbsolutePathBuf::try_from(managed_path.clone()).expect("managed file"); let service = ConfigManager::new_for_tests( tmp.path().to_path_buf(), @@ -1219,15 +1223,18 @@ async fn multi_agent_v2_boolean_layer_owns_enabled_origin_and_overrides() { }) .await .expect("read config"); - assert_eq!( - read.origins - .get("features.multi_agent_v2.enabled") - .expect("enabled origin") - .name, - ApiConfigLayerSource::LegacyManagedConfigTomlFromFile { - file: managed_file.clone(), - }, - ); + for path in [ + "features.multi_agent_v2.enabled", + "features.network_proxy", + "features.network_proxy.enabled", + ] { + assert_eq!( + read.origins.get(path).expect("managed feature origin").name, + ApiConfigLayerSource::LegacyManagedConfigTomlFromFile { + file: managed_file.clone(), + }, + ); + } let result = service .write_value(ConfigValueWriteParams { @@ -1248,6 +1255,109 @@ async fn multi_agent_v2_boolean_layer_owns_enabled_origin_and_overrides() { assert_eq!(overridden.effective_value, serde_json::json!(false)); } +#[tokio::test] +async fn structured_feature_toggle_ignores_unrelated_managed_settings() -> Result<()> { + let tmp = tempdir()?; + let user_path = tmp.path().join(CONFIG_TOML_FILE); + std::fs::write( + &user_path, + "[features.network_proxy]\nenabled = false\ncredential_broker = true\n", + )?; + + let managed_path = tmp.path().join("managed_config.toml"); + std::fs::write( + &managed_path, + "[features.network_proxy]\nproxy_url = 'http://127.0.0.1:4321'\n", + )?; + let service = ConfigManager::new_for_tests( + tmp.path().to_path_buf(), + vec![], + LoaderOverrides::with_managed_config_path_for_tests(managed_path.clone()), + CloudConfigBundleLoader::default(), + ); + + service + .write_value(ConfigValueWriteParams { + file_path: Some(user_path.display().to_string()), + key_path: "features.network_proxy".to_string(), + value: serde_json::Value::Null, + merge_strategy: MergeStrategy::Replace, + expected_version: None, + }) + .await?; + assert_eq!( + toml::from_str::(&std::fs::read_to_string(&user_path)?)?, + toml::from_str::( + "[features.network_proxy]\nenabled = false\ncredential_broker = true\n" + )? + ); + + let result = service + .write_value(ConfigValueWriteParams { + file_path: Some(user_path.display().to_string()), + key_path: "features.network_proxy".to_string(), + value: serde_json::json!(true), + merge_strategy: MergeStrategy::Replace, + expected_version: None, + }) + .await?; + + assert_eq!(result.status, WriteStatus::Ok); + assert_eq!(result.overridden_metadata, None); + + let selected_path = tmp.path().join("work.config.toml"); + std::fs::write(&selected_path, "")?; + let mut loader_overrides = LoaderOverrides::with_managed_config_path_for_tests(managed_path); + loader_overrides.user_config_path = Some(AbsolutePathBuf::from_absolute_path(&selected_path)?); + loader_overrides.user_config_profile = Some("work".parse()?); + let profile_service = ConfigManager::new_for_tests( + tmp.path().to_path_buf(), + vec![], + loader_overrides, + CloudConfigBundleLoader::default(), + ); + profile_service + .write_value(ConfigValueWriteParams { + file_path: Some(selected_path.display().to_string()), + key_path: "features.network_proxy".to_string(), + value: serde_json::Value::Null, + merge_strategy: MergeStrategy::Replace, + expected_version: None, + }) + .await?; + assert_eq!( + toml::from_str::(&std::fs::read_to_string(&selected_path)?)?, + toml::from_str::("[features]\nnetwork_proxy = false\n")? + ); + + service + .batch_write(ConfigBatchWriteParams { + edits: vec![ + codex_app_server_protocol::ConfigEdit { + key_path: "features.network_proxy.credential_broker".to_string(), + value: serde_json::Value::Null, + merge_strategy: MergeStrategy::Replace, + }, + codex_app_server_protocol::ConfigEdit { + key_path: "features.network_proxy".to_string(), + value: serde_json::Value::Null, + merge_strategy: MergeStrategy::Replace, + }, + ], + file_path: Some(user_path.display().to_string()), + expected_version: None, + reload_user_config: false, + }) + .await?; + assert!( + toml::from_str::(&std::fs::read_to_string(&user_path)?)? + .get("features") + .and_then(|features| features.get("network_proxy")) + .is_none() + ); + Ok(()) +} + #[tokio::test] async fn upsert_merges_tables_replace_overwrites() -> Result<()> { let tmp = tempdir().expect("tempdir"); @@ -1485,6 +1595,29 @@ subagent_usage_hint_text = "Delegate carefully." serde_json::Value::Null, "", ), + ( + r#"[features] +network_proxy = true +"#, + "features.network_proxy.credential_broker", + serde_json::json!(true), + r#"[features.network_proxy] +enabled = true +credential_broker = true +"#, + ), + ( + r#"[features.network_proxy] +enabled = true +credential_broker = true +"#, + "features.network_proxy", + serde_json::json!(false), + r#"[features.network_proxy] +enabled = false +credential_broker = true +"#, + ), ( r#"[desktop.features.multi_agent_v2] custom = true @@ -1590,7 +1723,7 @@ inherit = "all" exclude = [ "AWS_*", # keep this comment ] -set = { KEEP = "1", OTHER = "2" } # keep this inline table +set = { KEEP = "1", OTHER = "2", GH_HOST = "github.stale.example" } # keep this inline table "#, )?; @@ -1612,9 +1745,32 @@ inherit = "core" exclude = [ "AWS_*", # keep this comment ] -set = { KEEP = "1", OTHER = "2" } # keep this inline table +set = { KEEP = "1", OTHER = "2", GH_HOST = "github.stale.example" } # keep this inline table "# ); + #[cfg(windows)] + { + service + .write_value(ConfigValueWriteParams { + file_path: Some(path.display().to_string()), + key_path: "shell_environment_policy.set.gh_host".to_string(), + value: serde_json::json!("github.trusted.example"), + merge_strategy: MergeStrategy::Upsert, + expected_version: None, + }) + .await?; + let config: TomlValue = toml::from_str(&std::fs::read_to_string(&path)?)?; + let overrides = config + .get("shell_environment_policy") + .and_then(|policy| policy.get("set")) + .and_then(TomlValue::as_table) + .expect("environment overrides"); + assert_eq!( + overrides.get("GH_HOST").and_then(TomlValue::as_str), + Some("github.trusted.example") + ); + assert!(!overrides.contains_key("gh_host")); + } service .read(ConfigReadParams { include_layers: false, @@ -1723,6 +1879,7 @@ exclude = ["AWS_*"] "#, "shell_environment_policy.filters.AWS_*", serde_json::json!("include"), + serde_json::Value::Null, ), ( r#"[shell_environment_policy.filters] @@ -1730,10 +1887,18 @@ exclude = ["AWS_*"] "#, "shell_environment_policy.exclude", serde_json::json!(["AWS_*"]), + serde_json::Value::Null, + ), + #[cfg(windows)] + ( + "[shell_environment_policy.set]\nGH_HOST = 'github.managed.example'\n", + "shell_environment_policy.set.gh_host", + serde_json::json!("github.user.example"), + serde_json::json!("github.managed.example"), ), ]; - for (managed, key_path, value) in cases { + for (managed, key_path, value, expected_effective_value) in cases { let tmp = tempdir()?; let path = tmp.path().join(CONFIG_TOML_FILE); std::fs::write(&path, "")?; @@ -1765,7 +1930,7 @@ exclude = ["AWS_*"] overridden.overriding_layer.name, ApiConfigLayerSource::LegacyManagedConfigTomlFromFile { file: managed_file } ); - assert_eq!(overridden.effective_value, serde_json::Value::Null); + assert_eq!(overridden.effective_value, expected_effective_value); service .read(ConfigReadParams { include_layers: false, diff --git a/codex-rs/config/src/fingerprint.rs b/codex-rs/config/src/fingerprint.rs index 001f2b1bef..75cfd3eafd 100644 --- a/codex-rs/config/src/fingerprint.rs +++ b/codex-rs/config/src/fingerprint.rs @@ -1,5 +1,5 @@ use crate::ConfigLayerMetadata; -use crate::merge::is_multi_agent_v2_feature_path; +use crate::merge::is_structured_feature_path; use serde_json::Value as JsonValue; use sha2::Digest; use sha2::Sha256; @@ -29,7 +29,13 @@ pub(super) fn record_origins( } _ => { if !path.is_empty() { - if matches!(value, TomlValue::Boolean(_)) && is_multi_agent_v2_feature_path(path) { + if matches!(value, TomlValue::Boolean(_)) && is_structured_feature_path(path) { + if path + .last() + .is_some_and(|feature| feature == "network_proxy") + { + origins.insert(path.join("."), meta.clone()); + } path.push("enabled".to_string()); origins.insert(path.join("."), meta.clone()); path.pop(); diff --git a/codex-rs/config/src/lib.rs b/codex-rs/config/src/lib.rs index e441ea4578..fc5e05a687 100644 --- a/codex-rs/config/src/lib.rs +++ b/codex-rs/config/src/lib.rs @@ -154,6 +154,7 @@ pub use mcp_types::McpServerToolConfig; pub use mcp_types::McpServerTransportConfig; pub use mcp_types::RawMcpServerConfig; pub use merge::ShellEnvironmentPolicyFilterRepresentation; +pub use merge::is_structured_feature_path; pub use merge::merge_toml_values; pub use merge::shell_environment_filter_entry; pub use overrides::build_cli_overrides_layer; diff --git a/codex-rs/config/src/loader/local.rs b/codex-rs/config/src/loader/local.rs index 2f57f552bf..b341734188 100644 --- a/codex-rs/config/src/loader/local.rs +++ b/codex-rs/config/src/loader/local.rs @@ -1,3 +1,4 @@ +use super::credential_broker_trusted_config; use super::discover_project_layers; use super::layer_io; use super::load_config_toml_for_required_layer_raw; @@ -134,6 +135,7 @@ pub(super) async fn load_local_config_layers_with_overrides( let trust_context = project_trust_context( fs, &discovery_config, + &credential_broker_trusted_config(&discovery_config, &[], &loaded_managed), cwd, &project_root_markers, codex_home.as_path(), diff --git a/codex-rs/config/src/loader/mod.rs b/codex-rs/config/src/loader/mod.rs index 7e82fc9e34..ec6ea9e419 100644 --- a/codex-rs/config/src/loader/mod.rs +++ b/codex-rs/config/src/loader/mod.rs @@ -38,7 +38,9 @@ use crate::thread_config::ThreadConfigContext; 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_protocol::config_types::ApprovalsReviewer; +use codex_protocol::config_types::EnvironmentVariablePattern; use codex_protocol::config_types::SandboxMode; use codex_protocol::config_types::TrustLevel; use codex_protocol::protocol::AskForApproval; @@ -47,6 +49,7 @@ use codex_utils_absolute_path::AbsolutePathBufGuard; use codex_utils_path_uri::PathUri; use dunce::canonicalize as normalize_path; use serde::Deserialize; +use std::collections::HashMap; use std::io; use std::path::Path; #[cfg(windows)] @@ -389,6 +392,11 @@ pub async fn load_config_layers_state( let project_trust_context = match project_trust_context( fs, &merged_so_far, + &credential_broker_trusted_config( + &merged_so_far, + &thread_config_layers, + &loaded_config_layers, + ), &cwd, &project_root_markers, codex_home, @@ -996,6 +1004,8 @@ struct ProjectTrustContext { repo_root_lookup_keys: Option>, projects_trust: std::collections::HashMap, user_config_file: AbsolutePathBuf, + credential_broker_configured: bool, + credential_broker_binding_env: HashMap, } #[derive(Deserialize)] @@ -1111,7 +1121,11 @@ fn project_layer_entry( entry.with_hooks_config_folder_override(hooks_config_folder_override) } -fn sanitize_project_config(config: &mut TomlValue) -> Vec { +fn sanitize_project_config( + config: &mut TomlValue, + credential_broker_configured: bool, + trusted_binding_env: &HashMap, +) -> Vec { let Some(table) = config.as_table_mut() else { return Vec::new(); }; @@ -1122,10 +1136,99 @@ fn sanitize_project_config(config: &mut TomlValue) -> Vec { ignored_keys.push((*key).to_string()); } } - if let Some(features) = table.get_mut("features").and_then(TomlValue::as_table_mut) - && features.remove("respect_system_proxy").is_some() + if let Some(features) = table.get_mut("features").and_then(TomlValue::as_table_mut) { + if features.remove("respect_system_proxy").is_some() { + ignored_keys.push("features.respect_system_proxy".to_string()); + } + if credential_broker_configured + && features + .get("network_proxy") + .is_some_and(|network_proxy| network_proxy.as_bool().is_some()) + { + features.remove("network_proxy"); + ignored_keys.push("features.network_proxy".to_string()); + } else if let Some(network_proxy) = features + .get_mut("network_proxy") + .and_then(TomlValue::as_table_mut) + { + if network_proxy.remove("credential_broker").is_some() { + ignored_keys.push("features.network_proxy.credential_broker".to_string()); + } + if credential_broker_configured && network_proxy.remove("enabled").is_some() { + ignored_keys.push("features.network_proxy.enabled".to_string()); + } + } + } + if credential_broker_configured + && let Some(policy) = table + .get_mut("shell_environment_policy") + .and_then(TomlValue::as_table_mut) { - ignored_keys.push("features.respect_system_proxy".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)) + { + ignored_keys.push(format!("shell_environment_policy.set.{key}")); + false + } else { + true + } + }); + } + + if let Some(include_only) = policy + .get_mut("include_only") + .and_then(TomlValue::as_array_mut) + && !include_only.is_empty() + { + for binding_key in &binding_keys { + if !include_only.iter().any(|pattern| { + pattern.as_str().is_some_and(|pattern| { + EnvironmentVariablePattern::new_case_insensitive(pattern) + .matches(binding_key) + }) + }) { + include_only.push(TomlValue::String(binding_key.to_string())); + } + } + } + + if let Some(filters) = policy.get_mut("filters").and_then(TomlValue::as_table_mut) + && filters + .values() + .any(|action| action.as_str() == Some("include")) + { + for binding_key in &binding_keys { + if !filters.iter().any(|(pattern, action)| { + action.as_str() == Some("include") + && EnvironmentVariablePattern::new_case_insensitive(pattern) + .matches(binding_key) + }) { + filters.insert( + binding_key.to_string(), + TomlValue::String("include".to_string()), + ); + } + } + } + + if !trusted_binding_env.is_empty() + && ["inherit", "exclude", "include_only", "filters"] + .iter() + .any(|key| policy.contains_key(*key)) + { + let overrides = policy + .entry("set".to_string()) + .or_insert_with(|| TomlValue::Table(toml::map::Map::new())); + if let Some(overrides) = overrides.as_table_mut() { + for (key, value) in trusted_binding_env { + overrides.insert(key.clone(), TomlValue::String(value.clone())); + } + } + } } // Repository contents must not turn an ordinary key into a permission increase. if let Some(chat) = table @@ -1164,6 +1267,7 @@ fn project_ignored_config_keys_warning( async fn project_trust_context( fs: &dyn ExecutorFileSystem, merged_config: &TomlValue, + trusted_broker_config: &TomlValue, cwd: &AbsolutePathBuf, project_root_markers: &[String], config_base_dir: &Path, @@ -1198,6 +1302,45 @@ 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 + .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)) + }); + let credential_broker_binding_env = if credential_broker_configured { + let trusted_policy_overrides = trusted_broker_config + .get("shell_environment_policy") + .and_then(|policy| policy.get("set")) + .and_then(TomlValue::as_table); + credential_broker_provider_context_env_keys() + .filter_map(|key| { + let trusted_override = trusted_policy_overrides + .and_then(|overrides| { + overrides.iter().find(|(candidate, _)| { + if cfg!(windows) { + candidate.eq_ignore_ascii_case(key) + } else { + candidate.as_str() == key + } + }) + }) + .and_then(|(name, value)| { + value + .as_str() + .map(|value| (name.clone(), value.to_string())) + }); + trusted_override.or_else(|| { + std::env::var(key) + .ok() + .map(|value| (key.to_string(), value)) + }) + }) + .collect() + } else { + HashMap::new() + }; Ok(ProjectTrustContext { project_root, @@ -1209,9 +1352,29 @@ async fn project_trust_context( repo_root_lookup_keys, projects_trust, user_config_file: user_config_file.clone(), + credential_broker_configured, + credential_broker_binding_env, }) } +fn credential_broker_trusted_config( + merged_config: &TomlValue, + thread_config_layers: &[ConfigLayerEntry], + loaded_config_layers: &LoadedConfigLayers, +) -> TomlValue { + let mut trusted_config = merged_config.clone(); + for layer in thread_config_layers { + merge_toml_values(&mut trusted_config, &layer.config); + } + if let Some(config) = loaded_config_layers.managed_config.as_ref() { + merge_toml_values(&mut trusted_config, &config.managed_config); + } + if let Some(config) = loaded_config_layers.managed_config_from_mdm.as_ref() { + merge_toml_values(&mut trusted_config, &config.managed_config); + } + trusted_config +} + /// Canonicalize the path and convert it to a string to be used as a key in the /// projects trust map. On Windows, strips UNC, when possible, to try to ensure /// that different paths that point to the same location have the same key. @@ -1556,7 +1719,11 @@ async fn discover_project_layers( dot_codex_abs.as_path(), )?; } - let ignored_project_config_keys = sanitize_project_config(&mut config); + let ignored_project_config_keys = sanitize_project_config( + &mut config, + trust_context.credential_broker_configured, + &trust_context.credential_broker_binding_env, + ); if disabled_reason.is_none() && !ignored_project_config_keys.is_empty() { startup_warnings.push(project_ignored_config_keys_warning( &dot_codex_abs, diff --git a/codex-rs/config/src/loader/tests.rs b/codex-rs/config/src/loader/tests.rs index 92c3102a77..a944bc9d8b 100644 --- a/codex-rs/config/src/loader/tests.rs +++ b/codex-rs/config/src/loader/tests.rs @@ -19,13 +19,172 @@ use tempfile::tempdir; pub(super) struct TestFileSystem; +#[test] +fn project_config_cannot_override_configured_credential_broker_hosts() { + let mut config: TomlValue = toml::from_str( + "[shell_environment_policy.set]\n\ + GH_HOST = 'attacker.example'\n\ + OPENAI_BASE_URL = 'https://attacker.example/v1'", + ) + .expect("valid project config"); + + let ignored = sanitize_project_config( + &mut config, + /*credential_broker_configured*/ true, + &HashMap::new(), + ); + + assert_eq!( + ignored, + [ + "shell_environment_policy.set.GH_HOST", + "shell_environment_policy.set.OPENAI_BASE_URL", + ] + ); + assert_eq!( + config, + toml::from_str::("[shell_environment_policy.set]") + .expect("valid expected config") + ); +} + +#[test] +fn project_config_cannot_change_configured_credential_broker_state() { + for project_config in [ + "[features]\nnetwork_proxy = true", + "[features]\nnetwork_proxy = false", + "[features.network_proxy]\nenabled = true", + "[features.network_proxy]\nenabled = false", + ] { + let mut config: TomlValue = toml::from_str(project_config).expect("valid project config"); + + let ignored = sanitize_project_config( + &mut config, + /*credential_broker_configured*/ true, + &HashMap::new(), + ); + + assert_eq!(ignored.len(), 1); + assert!( + config + .get("features") + .and_then(|features| features.get("network_proxy")) + .is_none_or(|network_proxy| { + network_proxy + .as_table() + .is_some_and(|network_proxy| !network_proxy.contains_key("enabled")) + }) + ); + } +} + +#[test] +fn project_environment_filters_preserve_credential_host_bindings() { + for (project_config, expected_policy) in [ + ( + "[shell_environment_policy]\ninclude_only = ['GH_ENTERPRISE_TOKEN']", + "[shell_environment_policy]\ninclude_only = ['GH_ENTERPRISE_TOKEN', 'GH_HOST', 'OPENAI_BASE_URL']", + ), + ( + "[shell_environment_policy]\nexclude = ['*HOST*', '*BASE_URL*', 'OTHER']", + "[shell_environment_policy]\nexclude = ['*HOST*', '*BASE_URL*', 'OTHER']", + ), + ( + "[shell_environment_policy.filters]\nGH_ENTERPRISE_TOKEN = 'include'\n'*HOST*' = 'exclude'", + "[shell_environment_policy.filters]\nGH_ENTERPRISE_TOKEN = 'include'\n'*HOST*' = 'exclude'\nGH_HOST = 'include'\nOPENAI_BASE_URL = 'include'", + ), + ( + "[shell_environment_policy.filters]\n'*HOST*' = 'exclude'\nOTHER = 'exclude'", + "[shell_environment_policy.filters]\n'*HOST*' = 'exclude'\nOTHER = 'exclude'", + ), + ( + "[shell_environment_policy]\nexclude = ['*']", + "[shell_environment_policy]\nexclude = ['*']", + ), + ] { + let mut config: TomlValue = toml::from_str(project_config).expect("valid project config"); + + assert!( + sanitize_project_config( + &mut config, + /*credential_broker_configured*/ true, + &HashMap::new(), + ) + .is_empty() + ); + assert_eq!( + config, + toml::from_str::(expected_policy).expect("valid expected policy") + ); + } +} + +#[test] +fn project_environment_filters_preserve_only_trusted_credential_host_bindings() { + let host = "github.enterprise.example"; + let token = "ghp_enterprise_secret"; + let trusted_binding_env = HashMap::from([("GH_HOST".to_string(), host.to_string())]); + + for project_policy in [ + "inherit = 'none'", + "inherit = 'core'", + "exclude = ['*']", + "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"); + sanitize_project_config( + &mut project, + /*credential_broker_configured*/ true, + &trusted_binding_env, + ); + + let mut merged: TomlValue = toml::from_str(&format!( + "[shell_environment_policy.set]\nGH_ENTERPRISE_TOKEN = '{token}'" + )) + .expect("valid user config"); + merge_toml_values(&mut merged, &project); + let policy: crate::shell_environment_policy::ShellEnvironmentPolicyToml = merged + .get("shell_environment_policy") + .expect("shell environment policy") + .clone() + .try_into() + .expect("valid shell environment policy"); + let actual = codex_protocol::shell_environment::populate_env( + [ + ("GH_HOST", host), + ("AWS_SECRET_ACCESS_KEY", "unrelated_secret"), + ] + .into_iter() + .map(|(key, value)| (key.to_string(), value.to_string())), + &policy.into(), + /*thread_id*/ None, + ); + + assert_eq!( + actual, + [("GH_HOST", host), ("GH_ENTERPRISE_TOKEN", token)] + .into_iter() + .map(|(key, value)| (key.to_string(), value.to_string())) + .collect::>(), + "project policy: {project_policy}" + ); + } +} + #[test] fn project_config_cannot_bind_permission_shortcuts() { let safe = "[tui.keymap.chat]\nincrease_reasoning_effort = 'f9'\n"; for key in ["previous_permission_mode", "next_permission_mode"] { let mut config = toml::from_str(&format!("{safe}{key} = 'page-down'")).unwrap(); assert_eq!( - sanitize_project_config(&mut config), + sanitize_project_config( + &mut config, + /*credential_broker_configured*/ false, + &HashMap::new(), + ), [format!("tui.keymap.chat.{key}")] ); assert_eq!(config, toml::from_str::(safe).unwrap()); @@ -746,6 +905,49 @@ async fn local_layers_keep_raw_paths_order_and_legacy_requirements() { ) ); + #[cfg(windows)] + { + std::fs::write( + &system_file, + "model_instructions_file='./system.md'\n\ + [shell_environment_policy.set]\nGH_HOST='github.stale.example'\n", + ) + .expect("write stale system GitHub host"); + std::fs::write( + &user_file, + format!( + "{}\n[features.network_proxy]\nenabled=true\ncredential_broker=true\n\ + [shell_environment_policy.set]\ngh_host='github.trusted.example'\n", + user_config("trusted") + ), + ) + .expect("write lowercase trusted GitHub host"); + std::fs::write( + dot_codex.join(CONFIG_TOML_FILE), + "[shell_environment_policy]\ninherit='none'\n", + ) + .expect("write project environment policy"); + let layers = local::load_local_config_layers_with_overrides( + &TestFileSystem, + &codex_home, + &cwd, + &overrides, + ) + .await + .expect("load lowercase trusted GitHub host"); + let binding = layers.config.layers[2] + .toml + .get("shell_environment_policy") + .and_then(|policy| policy.get("set")) + .and_then(TomlValue::as_table) + .expect("trusted binding preserved"); + assert_eq!( + binding.get("gh_host").and_then(TomlValue::as_str), + Some("github.trusted.example") + ); + assert!(!binding.contains_key("GH_HOST")); + } + std::fs::write(&user_file, user_config("untrusted")).expect("write user config"); let layers = local::load_local_config_layers_with_overrides( &TestFileSystem, diff --git a/codex-rs/config/src/merge.rs b/codex-rs/config/src/merge.rs index be853b9932..187d8bcdd0 100644 --- a/codex-rs/config/src/merge.rs +++ b/codex-rs/config/src/merge.rs @@ -1,5 +1,6 @@ use crate::key_aliases::normalize_key_aliases; use crate::key_aliases::normalized_with_key_aliases; +use codex_network_proxy::credential_broker_provider_context_env_keys; use codex_network_proxy::normalize_host; use toml::Value as TomlValue; @@ -58,24 +59,21 @@ pub fn merge_toml_values(base: &mut TomlValue, overlay: &TomlValue) { merge_toml_values_at_path(base, overlay, &mut Vec::new()); } -pub(crate) fn is_multi_agent_v2_feature_path>(path: &[S]) -> bool { - match path { - [features, feature] => { - features.as_ref() == "features" && feature.as_ref() == "multi_agent_v2" - } - [profiles, _, features, feature] => { - profiles.as_ref() == "profiles" - && features.as_ref() == "features" - && feature.as_ref() == "multi_agent_v2" - } - _ => false, - } +pub fn is_structured_feature_path>(path: &[S]) -> bool { + let (features, feature) = match path { + [profiles, _, features, feature] if profiles.as_ref() == "profiles" => (features, feature), + [features, feature] => (features, feature), + _ => return false, + }; + + features.as_ref() == "features" + && matches!(feature.as_ref(), "multi_agent_v2" | "network_proxy") } fn merge_toml_values_at_path(base: &mut TomlValue, overlay: &TomlValue, path: &mut Vec) { replace_shell_environment_policy_filter_representation(base, overlay, path); - if is_multi_agent_v2_feature_path(path) { + if is_structured_feature_path(path) { if let TomlValue::Boolean(enabled) = base && overlay.is_table() { @@ -105,6 +103,18 @@ 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 cfg!(windows) + && matches!(path.as_slice(), [policy, field] if policy == "shell_environment_policy" && field == "set") + { + for key in overlay_table.keys().filter(|key| { + credential_broker_provider_context_env_keys() + .any(|binding_key| key.eq_ignore_ascii_case(binding_key)) + }) { + base_table.retain(|candidate, _| { + candidate == key || !candidate.eq_ignore_ascii_case(key) + }); + } + } for (key, value) in overlay_table { path.push(key.clone()); diff --git a/codex-rs/config/src/merge_tests.rs b/codex-rs/config/src/merge_tests.rs index be8c8fd047..d1986e4db5 100644 --- a/codex-rs/config/src/merge_tests.rs +++ b/codex-rs/config/src/merge_tests.rs @@ -255,6 +255,31 @@ fn multi_agent_v2_cli_overrides_preserve_boolean_and_nested_configuration() { } } +#[test] +fn network_proxy_feature_overrides_preserve_credential_broker_configuration() { + let enabled = ( + "features.network_proxy".to_string(), + TomlValue::Boolean(true), + ); + let broker = ( + "features.network_proxy.credential_broker".to_string(), + TomlValue::Boolean(true), + ); + let expected = + parse_toml("[features.network_proxy]\nenabled = true\ncredential_broker = true\n"); + + for overrides in [vec![enabled.clone(), broker.clone()], vec![broker, enabled]] { + assert_eq!(crate::build_cli_overrides_layer(&overrides), expected); + } + + let mut base = parse_toml("[features]\nnetwork_proxy = true\n"); + merge_toml_values( + &mut base, + &parse_toml("[features.network_proxy]\ncredential_broker = true\n"), + ); + assert_eq!(base, 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/overrides.rs b/codex-rs/config/src/overrides.rs index 5edc3b6810..70d5fcd2bc 100644 --- a/codex-rs/config/src/overrides.rs +++ b/codex-rs/config/src/overrides.rs @@ -1,4 +1,4 @@ -use crate::merge::is_multi_agent_v2_feature_path; +use crate::merge::is_structured_feature_path; use crate::merge::merge_toml_values; use toml::Value as TomlValue; @@ -29,7 +29,7 @@ fn apply_toml_override(root: &mut TomlValue, path: &str, value: TomlValue) { if is_last { match current { TomlValue::Table(table) => { - if is_multi_agent_v2_feature_path(&traversed_segments) + if is_structured_feature_path(&traversed_segments) && let Some(existing) = table.get_mut(segment) { match (&mut *existing, &value) { @@ -77,7 +77,7 @@ fn apply_toml_override(root: &mut TomlValue, path: &str, value: TomlValue) { current = table .entry(segment.to_string()) .or_insert_with(|| TomlValue::Table(Table::new())); - if is_multi_agent_v2_feature_path(&traversed_segments) + if is_structured_feature_path(&traversed_segments) && let TomlValue::Boolean(enabled) = current { *current = TomlValue::Table(Table::from_iter([( diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index e492e0dcab..139d73aa27 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -2381,6 +2381,9 @@ "allow_upstream_proxy": { "type": "boolean" }, + "credential_broker": { + "type": "boolean" + }, "dangerously_allow_all_unix_sockets": { "type": "boolean" }, diff --git a/codex-rs/core/src/config/config_loader_tests.rs b/codex-rs/core/src/config/config_loader_tests.rs index 49ca2c415b..52f5f54fb1 100644 --- a/codex-rs/core/src/config/config_loader_tests.rs +++ b/codex-rs/core/src/config/config_loader_tests.rs @@ -3713,6 +3713,10 @@ experimental_realtime_ws_base_url = "wss://attacker.example/realtime" [features] respect_system_proxy = true +[features.network_proxy] +enabled = true +credential_broker = true + [otel] environment = "attacker" @@ -3737,6 +3741,12 @@ wire_api = "responses" /*project_root_markers*/ None, ) .await?; + let managed_config_path = tmp.path().join("managed_config.toml"); + tokio::fs::write( + &managed_config_path, + "[features.network_proxy]\nenabled = false\ncredential_broker = true\n", + ) + .await?; let cwd = AbsolutePathBuf::from_absolute_path(&project_root)?; let layers = load_config_layers_state( @@ -3744,7 +3754,7 @@ wire_api = "responses" &codex_home, Some(cwd), &[] as &[(String, TomlValue)], - LoaderOverrides::default(), + LoaderOverrides::with_managed_config_path_for_tests(managed_config_path), &codex_config::NoopThreadConfigLoader, ) .await?; @@ -3767,6 +3777,8 @@ wire_api = "responses" "experimental_realtime_ws_base_url", "otel", "features.respect_system_proxy", + "features.network_proxy.credential_broker", + "features.network_proxy.enabled", ]; let expected_startup_warnings = vec![format!( concat!( @@ -3783,6 +3795,14 @@ wire_api = "responses" ); let effective_config = layers.effective_config(); + assert_eq!( + effective_config + .get("features") + .and_then(|features| features.get("network_proxy")) + .and_then(|network_proxy| network_proxy.get("enabled")) + .and_then(TomlValue::as_bool), + Some(false) + ); assert_eq!( effective_config.get("model"), Some(&TomlValue::String("project-model".to_string())) diff --git a/codex-rs/core/src/config/edit.rs b/codex-rs/core/src/config/edit.rs index 35de852cf1..be4237429b 100644 --- a/codex-rs/core/src/config/edit.rs +++ b/codex-rs/core/src/config/edit.rs @@ -2,6 +2,7 @@ use crate::path_utils::resolve_symlink_write_paths; use crate::path_utils::write_atomically; use anyhow::Context; use codex_config::CONFIG_TOML_FILE; +use codex_config::is_structured_feature_path; use codex_config::types::McpServerConfig; use codex_config::types::ResumeCwdMode; use codex_config::types::SessionPickerViewMode; @@ -323,7 +324,7 @@ impl ConfigDocument { Ok(self.set_skill_config(SkillConfigSelector::Name(name.clone()), *enabled)) } ConfigEdit::SetPath { segments, value } => { - if is_multi_agent_v2_feature_path(segments) && value.as_bool().is_some() { + if is_structured_feature_path(segments) && value.as_bool().is_some() { let mut existing = Some(self.doc.as_item()); for segment in segments { existing = existing.and_then(|item| item.as_table_like()?.get(segment)); @@ -336,7 +337,26 @@ impl ConfigDocument { } Ok(self.insert(segments, value.clone())) } - ConfigEdit::ClearPath { segments } => Ok(self.clear_owned(segments)), + ConfigEdit::ClearPath { segments } => { + let preserves_broker_settings = is_structured_feature_path(segments) + && segments + .last() + .is_some_and(|feature| feature == "network_proxy") + && segments + .iter() + .try_fold(self.doc.as_item(), |item, segment| { + item.as_table_like()?.get(segment) + }) + .and_then(TomlItem::as_table_like) + .is_some_and(|feature| feature.contains_key("credential_broker")); + if preserves_broker_settings { + let mut enabled_segments = segments.clone(); + enabled_segments.push("enabled".to_string()); + Ok(self.insert(&enabled_segments, value(false))) + } else { + Ok(self.clear_owned(segments)) + } + } ConfigEdit::SetProjectTrustLevel { path, level } => { // Delegate to the existing, tested logic in config.rs to // ensure tables are explicit and migration is preserved. @@ -618,7 +638,7 @@ impl ConfigDocument { } let item = current.get_mut(segment.as_str())?; - if is_multi_agent_v2_feature_path(&segments[..=index]) + if is_structured_feature_path(&segments[..=index]) && let Some(enabled) = item.as_bool() { let mut feature = document_helpers::new_implicit_table(); @@ -669,16 +689,6 @@ impl ConfigDocument { } } -fn is_multi_agent_v2_feature_path(segments: &[String]) -> bool { - match segments { - [features, feature] => features == "features" && feature == "multi_agent_v2", - [profiles, _, features, feature] => { - profiles == "profiles" && features == "features" && feature == "multi_agent_v2" - } - _ => false, - } -} - fn normalize_skill_config_path(path: &Path) -> String { dunce::canonicalize(path) .unwrap_or_else(|_| path.to_path_buf()) diff --git a/codex-rs/core/src/config/edit_tests.rs b/codex-rs/core/src/config/edit_tests.rs index 605a6a9f2a..b2ace568ef 100644 --- a/codex-rs/core/src/config/edit_tests.rs +++ b/codex-rs/core/src/config/edit_tests.rs @@ -136,6 +136,37 @@ fn multi_agent_v2_feature_toggle_preserves_nested_configuration() { ); } +#[test] +fn network_proxy_feature_toggle_preserves_credential_broker_configuration() { + let tmp = tempdir().expect("tmpdir"); + let codex_home = tmp.path(); + let config_path = codex_home.join(CONFIG_TOML_FILE); + for (initial, disabled, enabled_again) in [ + ( + "[features]\nnetwork_proxy = true\n", + "[features]\n", + "[features]\nnetwork_proxy = true\n", + ), + ( + "[features.network_proxy]\nenabled = true\ncredential_broker = true\ndomains = { \"github.com\" = \"allow\" }\n", + "[features.network_proxy]\nenabled = false\ncredential_broker = true\ndomains = { \"github.com\" = \"allow\" }\n", + "[features.network_proxy]\nenabled = true\ncredential_broker = true\ndomains = { \"github.com\" = \"allow\" }\n", + ), + ] { + std::fs::write(&config_path, initial).expect("write config"); + for (enabled, expected) in [(false, disabled), (true, enabled_again)] { + ConfigEditsBuilder::new(codex_home) + .set_feature_enabled("network_proxy", enabled) + .apply_blocking() + .expect("toggle feature"); + let updated: TomlValue = + toml::from_str(&std::fs::read_to_string(&config_path).expect("read config")) + .expect("parse config"); + assert_eq!(updated, toml::from_str::(expected).unwrap()); + } + } +} + /// Adding nested multi-agent settings must retain an existing legacy boolean toggle. #[test] fn multi_agent_v2_nested_edit_preserves_legacy_boolean_toggle() { diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 0d0ac94a9e..57a071fd89 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -3602,6 +3602,8 @@ impl Config { network_proxy, ); } + configured_network_proxy_config + .set_credential_broker_openai_base_url(cfg.openai_base_url.as_deref()); configured_network_proxy_config.enabled = true; } if cfg.approval_policy == Some(AskForApproval::UnlessTrusted) { @@ -4475,6 +4477,8 @@ impl Config { network_proxy, ); } + configured_network_proxy_config + .set_credential_broker_openai_base_url(cfg.openai_base_url.as_deref()); configured_network_proxy_config.enabled = true; } configured_network_proxy_config diff --git a/codex-rs/core/src/config/permissions.rs b/codex-rs/core/src/config/permissions.rs index 092bb33a56..a3b6dc5963 100644 --- a/codex-rs/core/src/config/permissions.rs +++ b/codex-rs/core/src/config/permissions.rs @@ -189,6 +189,9 @@ pub(crate) fn apply_network_proxy_feature_config( mitm: None, } .apply_to_network_proxy_config(config); + if let Some(credential_broker) = feature_config.credential_broker { + config.set_credential_broker_enabled(credential_broker); + } } pub(crate) fn resolve_permission_profile( diff --git a/codex-rs/core/src/tools/runtimes/mod.rs b/codex-rs/core/src/tools/runtimes/mod.rs index 929ef6c563..f8c90db3fa 100644 --- a/codex-rs/core/src/tools/runtimes/mod.rs +++ b/codex-rs/core/src/tools/runtimes/mod.rs @@ -39,6 +39,8 @@ pub(crate) fn exec_env_for_sandbox_permissions( sandbox_permissions: SandboxPermissions, ) -> HashMap { let mut env = env.clone(); + // Escalated commands intentionally use the original, unbrokered environment. This branch is + // defensive cleanup for a caller that passes an environment already prepared by Codex. if sandbox_permissions.requires_escalated_permissions() && env.contains_key(PROXY_ACTIVE_ENV_KEY) { @@ -253,7 +255,7 @@ pub(crate) fn maybe_wrap_shell_lc_with_snapshot( PLUGIN_METRICS_OUTPUT_ENV_VAR, ], ); - let (proxy_captures, proxy_exports) = build_proxy_env_exports(); + let (proxy_captures, proxy_exports) = build_proxy_env_exports(env); let runtime_path_prepend_exports = runtime_path_prepends.shell_exports_after_snapshot(explicit_env_overrides); let override_captures = join_shell_blocks([override_captures, proxy_captures]); @@ -292,10 +294,11 @@ fn build_override_exports( build_override_exports_for_keys("__CODEX_SNAPSHOT_OVERRIDE", &keys) } -fn build_proxy_env_exports() -> (String, String) { +fn build_proxy_env_exports(env: &HashMap) -> (String, String) { let mut keys = PROXY_ENV_KEYS .iter() .copied() + .chain(codex_network_proxy::brokered_credential_env_keys(env)) .chain(CUSTOM_CA_ENV_KEYS) .filter(|key| is_valid_shell_variable_name(key)) .collect::>(); diff --git a/codex-rs/core/src/tools/runtimes/mod_tests.rs b/codex-rs/core/src/tools/runtimes/mod_tests.rs index b99cd4bedc..d978777f40 100644 --- a/codex-rs/core/src/tools/runtimes/mod_tests.rs +++ b/codex-rs/core/src/tools/runtimes/mod_tests.rs @@ -63,10 +63,20 @@ fn shell_with_snapshot( } async fn test_network_proxy() -> anyhow::Result { - let state = codex_network_proxy::build_config_state( - NetworkProxyConfig::default(), - NetworkProxyConstraints::default(), - )?; + test_network_proxy_with_config(NetworkProxyConfig::default()).await +} + +pub(super) async fn test_credential_broker_network_proxy() -> anyhow::Result { + let mut config = NetworkProxyConfig::default(); + config.set_credential_broker_enabled(/*enabled*/ true); + test_network_proxy_with_config(config).await +} + +async fn test_network_proxy_with_config( + config: NetworkProxyConfig, +) -> anyhow::Result { + let state = + codex_network_proxy::build_config_state(config, NetworkProxyConstraints::default())?; NetworkProxy::builder() .state(Arc::new(NetworkProxyState::with_reloader( state, @@ -737,6 +747,54 @@ fn maybe_wrap_shell_lc_with_snapshot_restores_proxy_env_from_process_env() { ); } +#[tokio::test] +async fn snapshot_wrapper_replays_dummy_and_preserves_unbrokered_credentials() -> anyhow::Result<()> +{ + let proxy = test_credential_broker_network_proxy().await?; + let dir = tempdir()?; + let snapshot = dir.path().join("snapshot.sh"); + std::fs::write( + &snapshot, + "# Snapshot file\nexport OPENAI_API_KEY='stale'\nexport GITHUB_TOKEN='ghp_snapshot_only'\nexport GH_HOST='github.example.com'\n", + )?; + let (shell, snapshot) = shell_with_snapshot(ShellType::Bash, "/bin/bash", snapshot.abs()); + let mut env = HashMap::from([ + ( + "OPENAI_API_KEY".to_string(), + "sk-proj-abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_".to_string(), + ), + ("GITHUB_TOKEN".to_string(), String::new()), + ]); + proxy.apply_to_env(&mut env); + let dummy = env["OPENAI_API_KEY"].clone(); + let command = vec![ + "/bin/bash".to_string(), + "-lc".to_string(), + "printf '%s\\n%s\\n%s' \"$OPENAI_API_KEY\" \"${GITHUB_TOKEN-unset}\" \"$GH_HOST\"" + .to_string(), + ]; + let rewritten = maybe_wrap_shell_lc_with_snapshot( + &command, + &shell, + Some(&snapshot), + &HashMap::new(), + &env, + &RuntimePathPrepends::default(), + ); + let output = Command::new(&rewritten[0]) + .args(&rewritten[1..]) + .env_clear() + .envs(env) + .output()?; + + assert!(output.status.success(), "command failed: {output:?}"); + assert_eq!( + String::from_utf8_lossy(&output.stdout), + format!("{dummy}\nghp_snapshot_only\ngithub.example.com") + ); + Ok(()) +} + #[cfg(target_os = "macos")] #[test] fn maybe_wrap_shell_lc_with_snapshot_refreshes_codex_proxy_git_ssh_command() { diff --git a/codex-rs/core/src/tools/runtimes/zsh_fork/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/zsh_fork/unix_escalation.rs index b842f43afe..631bf4c6db 100644 --- a/codex-rs/core/src/tools/runtimes/zsh_fork/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/zsh_fork/unix_escalation.rs @@ -672,7 +672,7 @@ impl CoreShellCommandExecutor { program: &AbsolutePathBuf, argv: &[String], workdir: &AbsolutePathBuf, - env: HashMap, + mut env: HashMap, execution: EscalationExecution, ) -> anyhow::Result { let command = join_program_and_argv(program, argv); @@ -683,12 +683,20 @@ impl CoreShellCommandExecutor { }; let prepared = match execution { - EscalationExecution::Unsandboxed => PreparedExec { - command, - cwd: workdir.to_path_buf(), - env: exec_env_for_sandbox_permissions(&env, SandboxPermissions::RequireEscalated), - arg0: Some(first_arg.clone()), - }, + EscalationExecution::Unsandboxed => { + if let Some(network) = self.network.as_ref() { + network.restore_brokered_credentials(&mut env, &mut []); + } + PreparedExec { + command, + cwd: workdir.to_path_buf(), + env: exec_env_for_sandbox_permissions( + &env, + SandboxPermissions::RequireEscalated, + ), + arg0: Some(first_arg.clone()), + } + } EscalationExecution::TurnDefault => { self.prepare_sandboxed_exec(PrepareSandboxedExecParams { command, diff --git a/codex-rs/core/src/tools/runtimes/zsh_fork/unix_escalation_tests.rs b/codex-rs/core/src/tools/runtimes/zsh_fork/unix_escalation_tests.rs index 00f5f806ab..84343cd3b3 100644 --- a/codex-rs/core/src/tools/runtimes/zsh_fork/unix_escalation_tests.rs +++ b/codex-rs/core/src/tools/runtimes/zsh_fork/unix_escalation_tests.rs @@ -10,6 +10,7 @@ use crate::config::Constrained; use crate::guardian::GuardianReviewContext; use crate::sandboxing::SandboxPermissions; use crate::session::tests::make_session_and_context; +use crate::tools::runtimes::tests::test_credential_broker_network_proxy; use anyhow::Context; use codex_execpolicy::Decision; use codex_execpolicy::Evaluation; @@ -355,13 +356,14 @@ fn shell_request_escalation_execution_is_explicit() { #[tokio::test] async fn unsandboxed_intercepted_exec_strips_managed_network_env() -> anyhow::Result<()> { let workdir = test_sandbox_cwd(); + let network = test_credential_broker_network_proxy().await?; let executor = CoreShellCommandExecutor { command: Vec::new(), cwd: workdir.clone(), permission_profile: PermissionProfile::workspace_write(), sandbox: SandboxType::None, env: HashMap::new(), - network: None, + network: Some(network.clone()), network_environment_id: None, windows_sandbox_level: WindowsSandboxLevel::Disabled, arg0: None, @@ -371,15 +373,24 @@ async fn unsandboxed_intercepted_exec_strips_managed_network_env() -> anyhow::Re use_legacy_landlock: false, }; let mut env = HashMap::new(); + env.insert("GH_TOKEN".to_string(), "ghp-real".to_string()); env.insert(PROXY_ACTIVE_ENV_KEY.to_string(), "1".to_string()); for key in PROXY_ENV_KEYS { env.insert((*key).to_string(), format!("proxy-{key}")); } + network.apply_to_env(&mut env); + assert_ne!(env.get("GH_TOKEN").map(String::as_str), Some("ghp-real")); + let dummy = env.get("GH_TOKEN").expect("brokered token").clone(); let prepared = executor .prepare_escalated_exec( &AbsolutePathBuf::from_absolute_path("/usr/bin/curl")?, - &["curl".to_string(), "example.com".to_string()], + &[ + "curl".to_string(), + "-H".to_string(), + format!("Authorization: Bearer {dummy}"), + "example.com".to_string(), + ], &workdir, env, EscalationExecution::Unsandboxed, @@ -396,6 +407,19 @@ async fn unsandboxed_intercepted_exec_strips_managed_network_env() -> anyhow::Re "unsandboxed intercepted exec should strip managed-network proxy env var {key}" ); } + assert_eq!( + prepared.env.get("GH_TOKEN").map(String::as_str), + Some("ghp-real") + ); + assert_eq!( + prepared.command, + vec![ + "/usr/bin/curl".to_string(), + "-H".to_string(), + format!("Authorization: Bearer {dummy}"), + "example.com".to_string(), + ] + ); Ok(()) } diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 504853d8cf..181551409d 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -1084,6 +1084,9 @@ impl UnifiedExecProcessManager { .as_ref() .filter(|launch| launch.policy_decision_timeout_ms.is_some()) .and_then(|_| network.and_then(NetworkProxy::remote_policy_decider)); + if environment.is_remote() && network.is_some() && network_proxy_launch.is_none() { + request.exec_server_enforce_managed_network = false; + } request.exec_server_network_proxy = network_proxy_launch; request.exec_server_env_config = exec_server_env_config; request.exec_server_shell_snapshot = shell_snapshot; diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index ace9b004ac..f1ab8e8595 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -43,6 +43,7 @@ use core_test_support::responses::ev_function_call_with_namespace; use core_test_support::responses::ev_response_created; use core_test_support::responses::mount_sse_once; use core_test_support::responses::mount_sse_once_match; +use core_test_support::responses::mount_sse_sequence; use core_test_support::responses::sse; use core_test_support::responses::start_mock_server; use core_test_support::skip_if_no_network; @@ -2797,6 +2798,88 @@ async fn spawned_subagent_execpolicy_amendment_propagates_to_parent_session() -> Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[cfg(unix)] +async fn zsh_fork_receives_brokered_github_credential_and_provider_context() -> Result<()> { + skip_if_no_network!(Ok(())); + + const GH_HOST: &str = "github.example.com"; + const REAL_GITHUB_TOKEN: &str = + "ghp_abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; + + let Some(runtime) = zsh_fork_runtime("zsh-fork credential broker environment test")? else { + return Ok(()); + }; + + let home = Arc::new(TempDir::new()?); + fs::write( + home.path().join("config.toml"), + r#"default_permissions = "brokered" +features = { network_proxy = { enabled = true, credential_broker = true } } +permissions = { brokered = { extends = ":workspace", network = { enabled = true, allow_local_binding = true } } } +"#, + )?; + let mut builder = zsh_fork_test_builder(runtime, AskForApproval::Never) + .with_home(home) + .with_cloud_config_bundle(managed_network_requirements_loader()) + .with_config(|config| { + config.permissions.shell_environment_policy.r#set.extend([ + ("GH_HOST".to_string(), GH_HOST.to_string()), + ( + "GH_ENTERPRISE_TOKEN".to_string(), + REAL_GITHUB_TOKEN.to_string(), + ), + ]); + }); + let server = start_mock_server().await; + let test = builder.build(&server).await?; + + let call_id = "zsh-fork-brokered-github-credential"; + let command = r#"printf '%s\n%s\n%s\n' "$GH_HOST" "$GH_ENTERPRISE_TOKEN" "$CODEX_NETWORK_PROXY_CREDENTIAL_BROKER_ACTIVE""#; + let event = shell_event( + call_id, + command, + /*timeout_ms*/ 10_000, + SandboxPermissions::UseDefault, + )?; + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-zsh-fork-broker-1"), + event, + ev_completed("resp-zsh-fork-broker-1"), + ]), + sse(vec![ + ev_assistant_message("msg-zsh-fork-broker", "done"), + ev_completed("resp-zsh-fork-broker-2"), + ]), + ], + ) + .await; + + submit_turn_preserving_active_permission_profile( + &test, + "show the brokered GitHub environment", + AskForApproval::Never, + ) + .await?; + wait_for_completion_without_approval(&test).await; + + let result = parse_result(&responses.requests()[1].function_call_output(call_id)); + assert_eq!(result.exit_code, Some(0), "command failed: {result:?}"); + assert!(!result.stdout.contains(REAL_GITHUB_TOKEN)); + let values = result.stdout.lines().collect::>(); + assert_eq!(values.len(), 3, "unexpected command output: {result:?}"); + assert_eq!(values[0], GH_HOST); + assert_ne!(values[1], REAL_GITHUB_TOKEN); + assert!(values[1].starts_with("ghp_")); + assert_eq!(values[1].len(), REAL_GITHUB_TOKEN.len()); + assert_eq!(values[2], "1"); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[cfg(unix)] async fn env_zsh_script_spawned_by_python_can_request_escalation_under_zsh_fork() -> Result<()> { diff --git a/codex-rs/features/src/feature_configs.rs b/codex-rs/features/src/feature_configs.rs index 988dd8a865..b9e6ae4fec 100644 --- a/codex-rs/features/src/feature_configs.rs +++ b/codex-rs/features/src/feature_configs.rs @@ -423,6 +423,8 @@ pub struct NetworkProxyConfigToml { pub unix_sockets: Option>, #[serde(skip_serializing_if = "Option::is_none")] pub allow_local_binding: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub credential_broker: Option, } impl FeatureConfig for NetworkProxyConfigToml { diff --git a/codex-rs/network-proxy/src/credential_broker.rs b/codex-rs/network-proxy/src/credential_broker.rs index 0d5c4d3ae3..b58dc8b3d9 100644 --- a/codex-rs/network-proxy/src/credential_broker.rs +++ b/codex-rs/network-proxy/src/credential_broker.rs @@ -321,6 +321,11 @@ pub fn brokered_credential_dummy_env_keys(env: &HashMap) -> Vec< keys } +/// Returns environment keys used to bind registered credential providers to their destinations. +pub fn credential_broker_provider_context_env_keys() -> impl Iterator { + providers::credential_providers().flat_map(|provider| provider.context_env_vars.iter().copied()) +} + /// Returns credential keys plus provider context keys already present in an environment with an /// active broker. pub fn brokered_credential_env_keys( diff --git a/codex-rs/network-proxy/src/credential_broker/providers.rs b/codex-rs/network-proxy/src/credential_broker/providers.rs index 760a9aa5ad..4bc56952c1 100644 --- a/codex-rs/network-proxy/src/credential_broker/providers.rs +++ b/codex-rs/network-proxy/src/credential_broker/providers.rs @@ -15,7 +15,7 @@ type TranslateRequestHeader = fn(&HeaderMap, &str, &str) -> Option; /// Providers must be declared as `static` values because the broker uses their addresses as stable /// identities when deduplicating credential records. pub(super) struct CredentialProvider { - context_env_vars: &'static [&'static str], + pub(super) context_env_vars: &'static [&'static str], sources: &'static [CredentialSource], pub(super) reset_on_configuration_change: bool, dummy_value: fn(&str) -> String, diff --git a/codex-rs/network-proxy/src/lib.rs b/codex-rs/network-proxy/src/lib.rs index 6a2a3ed494..aaed6a2865 100644 --- a/codex-rs/network-proxy/src/lib.rs +++ b/codex-rs/network-proxy/src/lib.rs @@ -43,6 +43,7 @@ pub use config::managed_proxy_ports; pub use credential_broker::CREDENTIAL_BROKER_ACTIVE_ENV_KEY; pub use credential_broker::brokered_credential_dummy_env_keys; pub use credential_broker::brokered_credential_env_keys; +pub use credential_broker::credential_broker_provider_context_env_keys; pub use environment_policy::EnvironmentNetworkPolicy; pub use mitm_hook::InjectedHeaderConfig; pub use mitm_hook::MitmHookActionsConfig; diff --git a/codex-rs/protocol/src/shell_environment.rs b/codex-rs/protocol/src/shell_environment.rs index 73d14a79fd..e8bdaa40ca 100644 --- a/codex-rs/protocol/src/shell_environment.rs +++ b/codex-rs/protocol/src/shell_environment.rs @@ -137,6 +137,8 @@ where // Step 4 - Apply user-provided overrides. for (key, val) in &policy.r#set { + #[cfg(windows)] + env_map.retain(|existing, _| !existing.eq_ignore_ascii_case(key)); env_map.insert(key.clone(), val.clone()); } @@ -248,6 +250,21 @@ mod windows_tests { ]); assert_eq!(result, expected); + + let policy = ShellEnvironmentPolicy { + inherit: ShellEnvironmentPolicyInherit::All, + ignore_default_excludes: true, + r#set: HashMap::from([("gh_host".to_string(), "github.trusted.example".to_string())]), + ..Default::default() + }; + assert_eq!( + populate_env( + make_vars(&[("GH_HOST", "github.stale.example")]), + &policy, + /*thread_id*/ None, + ), + HashMap::from([("gh_host".to_string(), "github.trusted.example".to_string(),)]), + ); } #[test]