diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 8976b17ad9..30ccb0edf6 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3404,6 +3404,7 @@ dependencies = [ name = "codex-features" version = "0.0.0" dependencies = [ + "codex-network-proxy", "codex-otel", "codex-protocol", "pretty_assertions", diff --git a/codex-rs/app-server/src/config_manager_service.rs b/codex-rs/app-server/src/config_manager_service.rs index b7016229e8..0c5556d89c 100644 --- a/codex-rs/app-server/src/config_manager_service.rs +++ b/codex-rs/app-server/src/config_manager_service.rs @@ -276,22 +276,6 @@ 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" => { @@ -318,7 +302,10 @@ impl ConfigManager { 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")) + .is_some_and(|feature| { + feature.contains_key("credential_broker") + || feature.contains_key("credentials") + }) }; has_broker_settings(&user_config) || !has_broker_settings(&user_layer.config) @@ -782,19 +769,6 @@ 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()?; 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 c7d88939df..cf388933ff 100644 --- a/codex-rs/app-server/src/config_manager_service_tests.rs +++ b/codex-rs/app-server/src/config_manager_service_tests.rs @@ -1322,6 +1322,29 @@ async fn structured_feature_toggle_ignores_unrelated_managed_settings() -> Resul .and_then(|features| features.get("network_proxy")) .is_none() ); + + let configured_provider = "[features.network_proxy]\nenabled = true\n\n\ + [features.network_proxy.credentials.vendor]\n\ + env = ['VENDOR_TOKEN']\n\ + patterns = ['token_[a-z]{24}']\n\ + url_prefixes = ['https://api.vendor.example']\n"; + std::fs::write(&user_path, configured_provider)?; + 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?; + let updated: TomlValue = toml::from_str(&std::fs::read_to_string(&user_path)?)?; + let feature = &updated["features"]["network_proxy"]; + assert_eq!( + feature.get("enabled").and_then(TomlValue::as_bool), + Some(false) + ); + assert!(feature.get("credentials").is_some()); Ok(()) } @@ -1715,28 +1738,32 @@ exclude = [ set = { KEEP = "1", OTHER = "2", GH_HOST = "github.stale.example" } # keep this inline table "# ); - #[cfg(windows)] - { - service + for (value, expected_set) in [ + ( + serde_json::json!("github.trusted.example"), + "KEEP='1'\nOTHER='2'\nGH_HOST='github.stale.example'\ngh_host='github.trusted.example'", + ), + ( + serde_json::Value::Null, + "KEEP='1'\nOTHER='2'\nGH_HOST='github.stale.example'", + ), + ] { + let response = 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"), + value, merge_strategy: MergeStrategy::Upsert, expected_version: None, }) .await?; + assert_eq!(response.status, WriteStatus::Ok); + assert_eq!(response.overridden_metadata, None); 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") + config["shell_environment_policy"]["set"], + toml::from_str::(expected_set)? ); - assert!(!overrides.contains_key("gh_host")); } service .read(ConfigReadParams { @@ -1846,7 +1873,7 @@ exclude = ["AWS_*"] "#, "shell_environment_policy.filters.AWS_*", serde_json::json!("include"), - serde_json::Value::Null, + Some(serde_json::Value::Null), ), ( r#"[shell_environment_policy.filters] @@ -1854,21 +1881,42 @@ exclude = ["AWS_*"] "#, "shell_environment_policy.exclude", serde_json::json!(["AWS_*"]), - serde_json::Value::Null, + Some(serde_json::Value::Null), + ), + ( + "[shell_environment_policy.set]\nGH_HOST = 'github.managed.example'\n", + "shell_environment_policy.set.GH_HOST", + serde_json::json!("github.user.example"), + Some(serde_json::json!("github.managed.example")), ), - #[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"), + None, + ), + ( + "[shell_environment_policy.set]\ngh_host = 'github.managed.example'\n", + "shell_environment_policy.set.gh_host", + serde_json::json!("github.user.example"), + Some(serde_json::json!("github.managed.example")), + ), + ( + "[shell_environment_policy.set]\ngh_host = 'github.managed.example'\n", + "shell_environment_policy.set.GH_HOST", + serde_json::json!("github.user.example"), + None, ), ]; 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, "")?; + std::fs::write( + &path, + "[shell_environment_policy.set]\n\ + GH_HOST = 'github.original.example'\ngh_host = 'github.original-lower.example'\n", + )?; let managed_path = tmp.path().join("managed_config.toml"); std::fs::write(&managed_path, managed)?; let managed_file = AbsolutePathBuf::try_from(managed_path.clone())?; @@ -1889,15 +1937,20 @@ exclude = ["AWS_*"] }) .await?; - assert_eq!(response.status, WriteStatus::OkOverridden); - let overridden = response - .overridden_metadata - .expect("managed representation should override the user edit"); - assert_eq!( - overridden.overriding_layer.name, - ApiConfigLayerSource::LegacyManagedConfigTomlFromFile { file: managed_file } - ); - assert_eq!(overridden.effective_value, expected_effective_value); + if let Some(expected_effective_value) = expected_effective_value { + assert_eq!(response.status, WriteStatus::OkOverridden); + let overridden = response + .overridden_metadata + .expect("managed setting should override the user edit"); + assert_eq!( + overridden.overriding_layer.name, + ApiConfigLayerSource::LegacyManagedConfigTomlFromFile { file: managed_file } + ); + assert_eq!(overridden.effective_value, expected_effective_value); + } else { + assert_eq!(response.status, WriteStatus::Ok); + assert_eq!(response.overridden_metadata, None); + } service .read(ConfigReadParams { include_layers: false, diff --git a/codex-rs/config/src/loader/mod.rs b/codex-rs/config/src/loader/mod.rs index 48ea0d3bc5..962c8d0e8a 100644 --- a/codex-rs/config/src/loader/mod.rs +++ b/codex-rs/config/src/loader/mod.rs @@ -38,10 +38,8 @@ 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_network_proxy::is_credential_broker_provider_env_key; use codex_protocol::config_types::ApprovalsReviewer; -use codex_protocol::config_types::EnvironmentVariablePattern; use codex_protocol::config_types::SandboxMode; use codex_protocol::config_types::TrustLevel; use codex_protocol::protocol::AskForApproval; @@ -50,7 +48,6 @@ 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)] @@ -1039,7 +1036,7 @@ struct ProjectTrustContext { projects_trust: std::collections::HashMap, user_config_file: AbsolutePathBuf, credential_broker: CredentialBrokerProjectState, - credential_broker_binding_env: HashMap, + credential_broker_provider_env_keys: Vec, } #[derive(Deserialize)] @@ -1158,7 +1155,7 @@ fn project_layer_entry( fn sanitize_project_config( config: &mut TomlValue, credential_broker: CredentialBrokerProjectState, - trusted_binding_env: &HashMap, + trusted_provider_env_keys: &[String], ) -> Vec { let Some(table) = config.as_table_mut() else { return Vec::new(); @@ -1195,6 +1192,9 @@ fn sanitize_project_config( if network_proxy.remove("credential_broker").is_some() { ignored_keys.push("features.network_proxy.credential_broker".to_string()); } + if network_proxy.remove("credentials").is_some() { + ignored_keys.push("features.network_proxy.credentials".to_string()); + } if credential_broker_configured && network_proxy.remove("enabled").is_some() { ignored_keys.push("features.network_proxy.enabled".to_string()); } @@ -1208,15 +1208,17 @@ fn sanitize_project_config( if policy.remove("experimental_use_profile").is_some() { ignored_keys.push("shell_environment_policy.experimental_use_profile".to_string()); } - let binding_keys = credential_broker_provider_context_env_keys().collect::>(); + let environment_keys_match = |left: &str, right: &str| { + left == right || cfg!(windows) && left.eq_ignore_ascii_case(right) + }; if let Some(overrides) = policy.get_mut("set").and_then(TomlValue::as_table_mut) { overrides.retain(|key, _| { if key.eq_ignore_ascii_case("ZDOTDIR") || key.eq_ignore_ascii_case("BASH_ENV") || is_credential_broker_provider_env_key(key) - || binding_keys + || trusted_provider_env_keys .iter() - .any(|binding_key| key.eq_ignore_ascii_case(binding_key)) + .any(|provider_key| environment_keys_match(key, provider_key)) { ignored_keys.push(format!("shell_environment_policy.set.{key}")); false @@ -1225,57 +1227,6 @@ fn sanitize_project_config( } }); } - - 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 @@ -1369,38 +1320,27 @@ async fn project_trust_context( } else { CredentialBrokerProjectState::Unconfigured }; - 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() - }; + let configured_providers = network_proxy + .and_then(|network_proxy| network_proxy.get("credentials")) + .and_then(TomlValue::as_table); + let credential_broker_provider_env_keys = configured_providers + .into_iter() + .flat_map(|providers| providers.values()) + .flat_map(|provider| { + provider + .get("env") + .and_then(TomlValue::as_array) + .into_iter() + .flatten() + .filter_map(TomlValue::as_str) + .chain( + provider + .get("url_prefix_from_env") + .and_then(TomlValue::as_str), + ) + .map(str::to_string) + }) + .collect::>(); Ok(ProjectTrustContext { project_root, @@ -1413,7 +1353,7 @@ async fn project_trust_context( projects_trust, user_config_file: user_config_file.clone(), credential_broker, - credential_broker_binding_env, + credential_broker_provider_env_keys, }) } @@ -1782,7 +1722,7 @@ async fn discover_project_layers( let ignored_project_config_keys = sanitize_project_config( &mut config, trust_context.credential_broker, - &trust_context.credential_broker_binding_env, + &trust_context.credential_broker_provider_env_keys, ); if disabled_reason.is_none() && !ignored_project_config_keys.is_empty() { startup_warnings.push(project_ignored_config_keys_warning( diff --git a/codex-rs/config/src/loader/tests.rs b/codex-rs/config/src/loader/tests.rs index 9be08e94f1..77c32f3351 100644 --- a/codex-rs/config/src/loader/tests.rs +++ b/codex-rs/config/src/loader/tests.rs @@ -15,6 +15,7 @@ use codex_file_system::WalkOutcome; use codex_file_system::WriteFileOptions; use codex_utils_path_uri::PathUri; use pretty_assertions::assert_eq; +use std::collections::HashMap; use tempfile::tempdir; pub(super) struct TestFileSystem; @@ -28,11 +29,7 @@ fn project_config_cannot_override_configured_credential_broker_hosts() { ) .expect("valid project config"); - let ignored = sanitize_project_config( - &mut config, - CredentialBrokerProjectState::Enabled, - &HashMap::new(), - ); + let ignored = sanitize_project_config(&mut config, CredentialBrokerProjectState::Enabled, &[]); assert_eq!( ignored, @@ -48,6 +45,73 @@ fn project_config_cannot_override_configured_credential_broker_hosts() { ); } +#[test] +fn project_config_cannot_override_custom_credential_provider_or_binding() { + let mut config: TomlValue = toml::from_str( + "[features.network_proxy.credentials.attacker]\n\ + env = ['STRIPE_API_KEY']\npatterns = ['.*']\nurl_prefixes = ['attacker.example']\n\ + [shell_environment_policy.set]\n\ + STRIPE_API_KEY = 'attacker-token'\nSTRIPE_HOST = 'attacker.example'", + ) + .expect("valid project config"); + let ignored = sanitize_project_config( + &mut config, + CredentialBrokerProjectState::Enabled, + &["STRIPE_API_KEY".to_string(), "STRIPE_HOST".to_string()], + ); + + assert_eq!( + ignored, + [ + "features.network_proxy.credentials", + "shell_environment_policy.set.STRIPE_API_KEY", + "shell_environment_policy.set.STRIPE_HOST", + ] + ); +} + +#[test] +fn project_config_uses_platform_case_for_custom_credential_environment_keys() { + let mut config: TomlValue = toml::from_str( + "[shell_environment_policy.set]\n\ + stripe_api_key = 'application-value'\n\ + stripe_host = 'application.example'", + ) + .expect("valid project config"); + + let ignored = sanitize_project_config( + &mut config, + CredentialBrokerProjectState::Enabled, + &["STRIPE_API_KEY".to_string(), "STRIPE_HOST".to_string()], + ); + + if cfg!(windows) { + assert_eq!( + ignored, + [ + "shell_environment_policy.set.stripe_api_key", + "shell_environment_policy.set.stripe_host", + ] + ); + assert_eq!( + config, + toml::from_str::("[shell_environment_policy.set]") + .expect("valid expected config") + ); + } else { + assert!(ignored.is_empty()); + assert_eq!( + config, + toml::from_str::( + "[shell_environment_policy.set]\n\ + stripe_api_key = 'application-value'\n\ + stripe_host = 'application.example'" + ) + .expect("valid expected config") + ); + } +} + #[test] fn project_config_cannot_change_configured_credential_broker_state() { for project_config in [ @@ -63,11 +127,8 @@ fn project_config_cannot_change_configured_credential_broker_state() { ] { let mut config: TomlValue = toml::from_str(project_config).expect("valid project config"); - let ignored = sanitize_project_config( - &mut config, - CredentialBrokerProjectState::Enabled, - &HashMap::new(), - ); + let ignored = + sanitize_project_config(&mut config, CredentialBrokerProjectState::Enabled, &[]); assert_eq!(ignored.len(), 1); assert!( @@ -107,11 +168,7 @@ fn disabled_credential_broker_preserves_project_shell_settings() { ) .expect("valid project config"); - let ignored = sanitize_project_config( - &mut config, - CredentialBrokerProjectState::Disabled, - &HashMap::new(), - ); + let ignored = sanitize_project_config(&mut config, CredentialBrokerProjectState::Disabled, &[]); assert_eq!(ignored, vec!["features.network_proxy".to_string()]); assert_eq!( @@ -129,51 +186,35 @@ fn disabled_credential_broker_preserves_project_shell_settings() { } #[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 = ['*']", - ), +fn project_environment_filters_preserve_child_policy() { + for project_config in [ + "[shell_environment_policy]\ninclude_only = ['GH_ENTERPRISE_TOKEN']", + "[shell_environment_policy]\nexclude = ['*HOST*', '*BASE_URL*', 'OTHER']", + "[shell_environment_policy.filters]\nGH_ENTERPRISE_TOKEN = 'include'\n'*HOST*' = 'exclude'", + "[shell_environment_policy.filters]\n'*HOST*' = 'exclude'\nOTHER = 'exclude'", + "[shell_environment_policy]\nexclude = ['*']", + "[shell_environment_policy]\ninclude_only = ['STRIPE_API_KEY']", + "[shell_environment_policy.filters]\nSTRIPE_API_KEY = 'include'\n'*HOST*' = 'exclude'", ] { - let mut config: TomlValue = toml::from_str(project_config).expect("valid project config"); + let expected: TomlValue = toml::from_str(project_config).expect("valid project config"); + let mut config = expected.clone(); assert!( sanitize_project_config( &mut config, CredentialBrokerProjectState::Enabled, - &HashMap::new(), + &["STRIPE_API_KEY".to_string(), "STRIPE_HOST".to_string()], ) .is_empty() ); - assert_eq!( - config, - toml::from_str::(expected_policy).expect("valid expected policy") - ); + assert_eq!(config, expected); } } #[test] -fn project_environment_filters_preserve_only_trusted_credential_host_bindings() { +fn project_environment_filters_keep_excluded_hosts_out_of_children() { 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'", @@ -189,11 +230,7 @@ fn project_environment_filters_preserve_only_trusted_credential_host_bindings() BASH_ENV = '/untrusted-project-startup'" )) .expect("valid project config"); - sanitize_project_config( - &mut project, - CredentialBrokerProjectState::Enabled, - &trusted_binding_env, - ); + sanitize_project_config(&mut project, CredentialBrokerProjectState::Enabled, &[]); let mut merged: TomlValue = toml::from_str(&format!( "[shell_environment_policy.set]\nGH_ENTERPRISE_TOKEN = '{token}'" @@ -219,7 +256,7 @@ fn project_environment_filters_preserve_only_trusted_credential_host_bindings() assert_eq!( actual, - [("GH_HOST", host), ("GH_ENTERPRISE_TOKEN", token)] + [("GH_ENTERPRISE_TOKEN", token)] .into_iter() .map(|(key, value)| (key.to_string(), value.to_string())) .collect::>(), @@ -234,11 +271,7 @@ fn project_config_cannot_bind_permission_shortcuts() { 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, - CredentialBrokerProjectState::Unconfigured, - &HashMap::new(), - ), + sanitize_project_config(&mut config, CredentialBrokerProjectState::Unconfigured, &[],), [format!("tui.keymap.chat.{key}")] ); assert_eq!(config, toml::from_str::(safe).unwrap()); @@ -966,7 +999,6 @@ async fn local_layers_keep_raw_paths_order_and_legacy_requirements() { ) ); - #[cfg(windows)] { std::fs::write( &system_file, @@ -980,7 +1012,7 @@ async fn local_layers_keep_raw_paths_order_and_legacy_requirements() { format!( "{}\n[features.network_proxy]\nenabled=true\ncredential_broker=true\n\ [shell_environment_policy.set]\ngh_host='github.trusted.example'\n\ - gh_enterprise_token='ghp_trusted'\n", + gh_enterprise_token='ghp_trusted'\nOPENAI_BASE_URL=''\n", user_config("trusted") ), ) @@ -998,19 +1030,11 @@ async fn local_layers_keep_raw_paths_order_and_legacy_requirements() { ) .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") + layers.config.layers[2].toml, + toml::from_str::("[shell_environment_policy]\ninherit='none'") + .expect("project policy") ); - assert!(!binding.contains_key("GH_HOST")); - assert!(!binding.contains_key("gh_enterprise_token")); - assert!(!binding.contains_key("GH_ENTERPRISE_TOKEN")); } std::fs::write(&user_file, user_config("untrusted")).expect("write user config"); diff --git a/codex-rs/config/src/merge.rs b/codex-rs/config/src/merge.rs index 177436cf60..722e344f6f 100644 --- a/codex-rs/config/src/merge.rs +++ b/codex-rs/config/src/merge.rs @@ -1,6 +1,5 @@ use crate::key_aliases::normalize_key_aliases; use crate::key_aliases::normalized_with_key_aliases; -use codex_network_proxy::is_credential_broker_provider_env_key; use codex_network_proxy::normalize_host; use toml::Value as TomlValue; @@ -106,19 +105,6 @@ 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| is_credential_broker_provider_env_key(key)) - { - base_table.retain(|candidate, _| { - candidate == key || !candidate.eq_ignore_ascii_case(key) - }); - } - } - 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 270436927b..d0bf6a4d78 100644 --- a/codex-rs/config/src/merge_tests.rs +++ b/codex-rs/config/src/merge_tests.rs @@ -375,6 +375,84 @@ exclude = ["HIGH_*"] assert_eq!(base, overlay); } +#[test] +fn shell_environment_policy_set_overlay_preserves_case_distinct_keys() { + let mut base = parse_toml( + r#" +[features.network_proxy.credentials.stripe] +env = ["STRIPE_API_KEY"] +patterns = ["stripe_[a-z]+"] +url_prefix_from_env = "STRIPE_HOST" + +[shell_environment_policy.set] +stripe_api_key = "stale" +stripe_host = "stale.example" +foo = "lowercase" +"#, + ); + let overlay = parse_toml( + r#" +[shell_environment_policy.set] +STRIPE_API_KEY = "trusted" +STRIPE_HOST = "trusted.example" +FOO = "uppercase" +"#, + ); + + merge_toml_values(&mut base, &overlay); + + let values = base["shell_environment_policy"]["set"] + .as_table() + .expect("shell environment overrides"); + assert_eq!( + values.get("foo").and_then(TomlValue::as_str), + Some("lowercase") + ); + assert_eq!( + values.get("FOO").and_then(TomlValue::as_str), + Some("uppercase") + ); + assert_eq!(values.len(), 6); +} + +#[test] +fn later_credential_provider_preserves_case_distinct_environment_overrides() { + let mut base = parse_toml( + "[shell_environment_policy.set]\n\ + stripe_api_key = 'stale'\n\ + stripe_host = 'stale.example'", + ); + let override_layer = parse_toml( + "[shell_environment_policy.set]\n\ + STRIPE_API_KEY = 'previous'\n\ + STRIPE_HOST = 'previous.example'", + ); + let final_override_layer = parse_toml( + "[shell_environment_policy.set]\n\ + stripe_api_key = 'trusted'\n\ + stripe_host = 'current.example'", + ); + let provider_layer = parse_toml( + "[features.network_proxy.credentials.stripe]\n\ + env = ['STRIPE_API_KEY']\n\ + url_prefix_from_env = 'STRIPE_HOST'", + ); + + merge_toml_values(&mut base, &override_layer); + merge_toml_values(&mut base, &final_override_layer); + merge_toml_values(&mut base, &provider_layer); + + assert_eq!( + base["shell_environment_policy"]["set"], + parse_toml( + "stripe_api_key = 'trusted'\n\ + stripe_host = 'current.example'\n\ + STRIPE_API_KEY = 'previous'\n\ + STRIPE_HOST = 'previous.example'", + ) + ); +} + #[test] fn shell_environment_policy_filters_overlay_merges_by_key_case_insensitively() { let mut base = parse_toml( diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index c3b46a6b7d..b0c2f9c9c2 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -1193,6 +1193,62 @@ }, "type": "object" }, + "CredentialAuthMethod": { + "description": "Authentication formats supported by declarative credential providers.", + "enum": [ + "bearer", + "token", + "basic", + "header" + ], + "type": "string" + }, + "CredentialProviderConfig": { + "additionalProperties": false, + "description": "Declarative description of an environment-backed credential family.", + "properties": { + "auth": { + "default": [], + "items": { + "$ref": "#/definitions/CredentialAuthMethod" + }, + "type": "array" + }, + "env": { + "default": [], + "items": { + "type": "string" + }, + "type": "array" + }, + "header": { + "type": "string" + }, + "patterns": { + "default": [], + "items": { + "type": "string" + }, + "type": "array" + }, + "prefix": { + "type": "string" + }, + "url_prefix_from_env": { + "description": "Environment variable containing an additional URL prefix or hostname.", + "type": "string" + }, + "url_prefixes": { + "default": [], + "description": "URL prefixes authorized for injection. Bare loopback hosts imply HTTP; other bare hosts imply HTTPS.", + "items": { + "type": "string" + }, + "type": "array" + } + }, + "type": "object" + }, "CurrentTimeReminderConfigToml": { "additionalProperties": false, "properties": { @@ -2589,6 +2645,12 @@ "credential_broker": { "type": "boolean" }, + "credentials": { + "additionalProperties": { + "$ref": "#/definitions/CredentialProviderConfig" + }, + "type": "object" + }, "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 bb25c2c543..0c11f7ff27 100644 --- a/codex-rs/core/src/config/config_loader_tests.rs +++ b/codex-rs/core/src/config/config_loader_tests.rs @@ -1926,6 +1926,85 @@ enable_socks5 = false Ok(()) } +#[tokio::test] +async fn credential_broker_ambiguity_guard_survives_permission_profile_selection() +-> anyhow::Result<()> { + for (key, other_key, other_value, ambiguous) in [ + ("GH_HOST", "gh_host", "second.example", true), + ("VENDOR_HOST", "vendor_host", "second.example", true), + ("VENDOR_PASSWORD", "vendor_password", "second.example", true), + ("VENDOR_HOST", "vendor_host", "first.example", false), + ("FOO", "foo", "second.example", false), + ] { + let home = tempdir()?; + tokio::fs::write( + home.path().join(CONFIG_TOML_FILE), + format!( + r#" +default_permissions = "first" +[features.network_proxy] +enabled = true +credential_broker = true +[features.network_proxy.credentials.vendor] +env = ["VENDOR_PASSWORD"] +patterns = ["pin_[a-z]{{8}}"] +url_prefixes = ["https://api.vendor.example"] +url_prefix_from_env = "VENDOR_HOST" +auth = ["bearer"] +[permissions.first] +extends = ":workspace" +[permissions.first.network] +enabled = true +[permissions.second] +extends = "first" +[shell_environment_policy.set] +{key} = "first.example" +{other_key} = "{other_value}" +"# + ), + ) + .await?; + let config = ConfigBuilder::default() + .codex_home(home.path().to_path_buf()) + .fallback_cwd(Some(home.path().to_path_buf())) + .loader_overrides(LoaderOverrides::without_managed_config_for_tests()) + .build() + .await?; + let expected_enabled = !(cfg!(windows) && ambiguous); + assert_eq!( + config.permissions.shell_environment_policy.r#set, + HashMap::from([ + (key.to_string(), "first.example".to_string()), + (other_key.to_string(), other_value.to_string()), + ]) + ); + assert_eq!( + config + .permissions + .network + .as_ref() + .unwrap() + .credential_broker_enabled(), + expected_enabled, + "{key}" + ); + let mut selected = config.permissions.active_permission_profile().unwrap(); + selected.id = "second".to_string(); + let rebuilt = config + .network_proxy_spec_for_active_permission_profile( + &selected, + config.permissions.permission_profile(), + )? + .unwrap(); + assert_eq!( + rebuilt.credential_broker_enabled(), + expected_enabled, + "{key}" + ); + } + Ok(()) +} + #[tokio::test] async fn resolve_permission_profile_inherits_across_configured_and_managed_profiles() -> anyhow::Result<()> { diff --git a/codex-rs/core/src/config/edit.rs b/codex-rs/core/src/config/edit.rs index 52447014fe..4fc7ff06bf 100644 --- a/codex-rs/core/src/config/edit.rs +++ b/codex-rs/core/src/config/edit.rs @@ -349,7 +349,10 @@ impl ConfigDocument { item.as_table_like()?.get(segment) }) .and_then(TomlItem::as_table_like) - .is_some_and(|feature| feature.contains_key("credential_broker")); + .is_some_and(|feature| { + feature.contains_key("credential_broker") + || feature.contains_key("credentials") + }); if preserves_broker_settings { let mut enabled_segments = segments.clone(); enabled_segments.push("enabled".to_string()); diff --git a/codex-rs/core/src/config/edit_tests.rs b/codex-rs/core/src/config/edit_tests.rs index 19434c39f2..bc89f6ea97 100644 --- a/codex-rs/core/src/config/edit_tests.rs +++ b/codex-rs/core/src/config/edit_tests.rs @@ -179,6 +179,11 @@ fn network_proxy_feature_toggle_preserves_credential_broker_configuration() { "[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", ), + ( + "[features.network_proxy]\nenabled = true\ncredentials = { vendor = { env = [\"VENDOR_TOKEN\"] } }\n", + "[features.network_proxy]\nenabled = false\ncredentials = { vendor = { env = [\"VENDOR_TOKEN\"] } }\n", + "[features.network_proxy]\nenabled = true\ncredentials = { vendor = { env = [\"VENDOR_TOKEN\"] } }\n", + ), ] { std::fs::write(&config_path, initial).expect("write config"); for (enabled, expected) in [(false, disabled), (true, enabled_again)] { diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 9af0ec6d0d..c4053ef20e 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -563,10 +563,12 @@ fn profile_allows_configured_network_proxy(permission_profile: &PermissionProfil } fn build_network_proxy_spec( - configured_network_proxy_config: NetworkProxyConfig, + mut configured_network_proxy_config: NetworkProxyConfig, network_requirements: Option>, permission_profile: &PermissionProfile, + environment_overrides: &HashMap, ) -> std::io::Result> { + configured_network_proxy_config.configure_credential_broker_environment(environment_overrides); let (network_requirements, network_requirements_source) = match network_requirements { Some(Sourced { value, source }) => (Some(value), Some(source)), None => (None, None), @@ -3750,7 +3752,7 @@ impl Config { })? .clone(); - let shell_environment_policy = cfg.shell_environment_policy.into(); + let shell_environment_policy = ShellEnvironmentPolicy::from(cfg.shell_environment_policy); let allow_login_shell = cfg.allow_login_shell.unwrap_or(true); let history = cfg.history.unwrap_or_default(); @@ -4058,6 +4060,7 @@ impl Config { configured_network_proxy_config, network_requirements, &network_permission_profile, + &shell_environment_policy.r#set, )?; let mut helper_readable_roots = get_readable_roots_required_for_codex_runtime( &codex_home, @@ -4581,6 +4584,7 @@ impl Config { configured_network_proxy_config, self.config_layer_stack.requirements().network.clone(), permission_profile, + &self.permissions.shell_environment_policy.r#set, ) } diff --git a/codex-rs/core/src/config/permissions.rs b/codex-rs/core/src/config/permissions.rs index 894d9c60ba..61a4d4d060 100644 --- a/codex-rs/core/src/config/permissions.rs +++ b/codex-rs/core/src/config/permissions.rs @@ -192,6 +192,9 @@ pub(crate) fn apply_network_proxy_feature_config( if let Some(credential_broker) = feature_config.credential_broker { config.set_credential_broker_enabled(credential_broker); } + if let Some(credentials) = feature_config.credentials.as_ref() { + config.credential_providers.clone_from(credentials); + } } /// Resolves a named permission profile and its inherited configuration. diff --git a/codex-rs/core/src/shell_snapshot.rs b/codex-rs/core/src/shell_snapshot.rs index c8b689dbf2..4aecafa091 100644 --- a/codex-rs/core/src/shell_snapshot.rs +++ b/codex-rs/core/src/shell_snapshot.rs @@ -16,15 +16,10 @@ use anyhow::anyhow; use anyhow::bail; use codex_exec_server::Environment; use codex_network_proxy::CREDENTIAL_BROKER_ACTIVE_ENV_KEY; +use codex_network_proxy::CredentialBrokerContext; use codex_network_proxy::NetworkProxy; -use codex_network_proxy::brokered_credential_binding_env_keys; -use codex_network_proxy::brokered_credential_dummy_env_keys; -use codex_network_proxy::brokered_credential_env_keys; use codex_network_proxy::brokered_credential_marker_env_keys; use codex_network_proxy::brokered_credential_value_env_keys; -use codex_network_proxy::credential_broker_provider_context_env_keys; -use codex_network_proxy::credential_broker_provider_sources_allowed; -use codex_network_proxy::is_credential_broker_provider_env_key; use codex_otel::SessionTelemetry; use codex_protocol::ThreadId; use codex_protocol::config_types::ShellEnvironmentPolicy; @@ -358,14 +353,16 @@ impl ShellSnapshotFile { }) } + #[must_use = "pass private context to the broker when preparing a managed child"] pub(crate) fn restore_credentials( &self, env: &mut HashMap, shell_environment_policy: &ShellEnvironmentPolicy, - ) { + ) -> CredentialBrokerContext { let Some(credentials) = self.credentials.as_ref() else { - return; + return CredentialBrokerContext::default(); }; + let mut credential_context = HashMap::new(); let mut allowed_snapshot_env = create_env_from_vars( credentials @@ -407,52 +404,77 @@ impl ShellSnapshotFile { let Some(credential_keys) = credentials.context_credential_keys.get(key) else { continue; }; - if contains_env_key(explicit_env_overrides, key) - || !contains_env_key(&allowed_snapshot_env, key) + let binding_keys = credentials.binding_context_credential_keys.get(key); + let restores_binding_context = binding_keys.is_some_and(|keys| { + keys.iter() + .any(|key| restored_credential_keys.contains(&key)) + }); + let needs_binding_context = restores_binding_context + || binding_keys.is_some_and(|keys| { + keys.iter().any(|key| { + env_value(env, key) + .is_some_and(|value| snapshot_real_credentials.get(key) == Some(value)) + }) + }); + let allowed_context = contains_env_key(&allowed_snapshot_env, key); + let explicit_context = env_value(explicit_env_overrides, key); + if explicit_context.is_some() && allowed_context + || !allowed_context && !needs_binding_context || !credential_keys .iter() .any(|credential_key| contains_env_key(&allowed_snapshot_env, credential_key)) { continue; } - let restores_binding_context = credentials - .binding_context_credential_keys - .get(key) - .is_some_and(|binding_keys| { - binding_keys - .iter() - .any(|credential_key| restored_credential_keys.contains(&credential_key)) - }); if restores_binding_context || !contains_env_key(env, key) { - insert_env_value(env, key, value); + let target = if allowed_context { + &mut *env + } else { + &mut credential_context + }; + insert_env_value(target, key, explicit_context.unwrap_or(value)); } } - let previous = env.insert( - CREDENTIAL_BROKER_ACTIVE_ENV_KEY.to_string(), - "1".to_string(), - ); - credentials - .network_proxy - .restore_brokered_credentials(env, &mut []); - if let Some(previous) = previous { - env.insert(CREDENTIAL_BROKER_ACTIVE_ENV_KEY.to_string(), previous); - } else { - env.remove(CREDENTIAL_BROKER_ACTIVE_ENV_KEY); + for env in [env, &mut credential_context] { + let previous = env.insert( + CREDENTIAL_BROKER_ACTIVE_ENV_KEY.to_string(), + "1".to_string(), + ); + credentials + .network_proxy + .restore_brokered_credentials(env, &mut []); + if let Some(previous) = previous { + env.insert(CREDENTIAL_BROKER_ACTIVE_ENV_KEY.to_string(), previous); + } else { + env.remove(CREDENTIAL_BROKER_ACTIVE_ENV_KEY); + } } + credential_context.into() } - pub(crate) fn restore_fail_open_aliases(&self, env: &mut HashMap) { + pub(crate) fn restore_fail_open_aliases( + &self, + env: &mut HashMap, + environment_id: Option<&str>, + ) { let Some(credentials) = self.credentials.as_ref() else { return; }; if env_value(env, CREDENTIAL_BROKER_ACTIVE_ENV_KEY).is_some_and(|active| active == "1") { for (key, alias) in &credentials.fail_open_aliases { - let has_snapshot_dummy = alias - .dummy_value - .as_ref() - .is_some_and(|dummy| env_value(env, key) == Some(dummy)); - if has_snapshot_dummy { + let has_snapshot_value = alias.dummy_value.as_ref().is_some_and(|dummy| { + env_value(env, key).is_some_and(|value| { + value == dummy + || credentials.network_proxy.child_credential_alias_matches( + key, + value, + dummy, + environment_id, + ) + }) + }); + if has_snapshot_value { if !alias.authorized { remove_env_value(env, key); } @@ -589,20 +611,31 @@ async fn capture_snapshot( .filter(|key| !contains_env_key(&original_env, key)) .collect::>(); let policy = &credential_broker.shell_environment_policy; - let inherited_env = create_env_from_vars(std::env::vars(), policy, /*thread_id*/ None); + // Child visibility must not change the trusted destination of an inherited credential. + let inherited_env = std::env::vars().collect(); + let network_config = credential_broker.network_proxy.current_cfg().await?; + let inherited_env = network_config + .credential_broker_context + .with_fallbacks(&inherited_env); let mut restored_env = original_env.clone(); credential_broker .network_proxy .restore_brokered_credentials(&mut restored_env, &mut []); let mut discovery_env = restored_env.clone(); - replace_provider_context_with_inherited( + let provider_environment = credential_broker + .network_proxy + .credential_broker_environment(&discovery_env); + let provider_context_keys = provider_environment.provider_context_keys; + replace_provider_context_with_trusted( &mut discovery_env, &inherited_env, - credential_broker_provider_context_env_keys(), + provider_context_keys.clone(), + &provider_context_keys, ); for (key, value) in &policy.r#set { if !contains_env_key(&discovery_env, key) - || credential_broker_provider_context_env_keys() + || provider_context_keys + .iter() .any(|context_key| context_key.eq_ignore_ascii_case(key)) { insert_env_value(&mut discovery_env, key, value); @@ -610,8 +643,11 @@ async fn capture_snapshot( } credential_broker .network_proxy - .apply_to_env(&mut discovery_env); - let brokered_keys = brokered_credential_dummy_env_keys(&discovery_env); + .apply_to_env_for_snapshot(&mut discovery_env); + let discovery_metadata = credential_broker + .network_proxy + .credential_broker_environment(&discovery_env); + let brokered_keys = discovery_metadata.credential_keys; let mut env = create_env_from_vars( restored_env .iter() @@ -619,20 +655,47 @@ async fn capture_snapshot( policy, /*thread_id*/ None, ); - replace_provider_context_with_inherited( - &mut env, - &inherited_env, - credential_broker_provider_context_env_keys(), + let allowed_context_env = create_env_from_vars( + provider_context_keys + .iter() + .map(|key| (key.clone(), String::new())), + policy, + /*thread_id*/ None, ); - credential_broker.network_proxy.apply_to_env(&mut env); - let allowed_brokered_keys = brokered_credential_dummy_env_keys(&env); + replace_provider_context_with_trusted( + &mut env, + &discovery_env, + provider_context_keys.clone(), + &[], + ); + credential_broker + .network_proxy + .apply_to_env_for_snapshot(&mut env); + let mut snapshot_allowed_env = env.clone(); + // The live broker regenerates this marker; captured dummies can belong to another scope. + snapshot_allowed_env.remove("CODEX_NETWORK_PROXY_BROKERED_CREDENTIALS"); + for key in &provider_context_keys { + if env_value(&original_env, key) != env_value(&env, key) { + remove_env_value(&mut snapshot_allowed_env, key); + } + } + for key in &provider_context_keys { + if !contains_env_key(&allowed_context_env, key) { + remove_env_value(&mut snapshot_allowed_env, key); + } + } + let allowed_metadata = credential_broker + .network_proxy + .credential_broker_environment(&env); + let allowed_brokered_keys = allowed_metadata.credential_keys; let unbrokered_credentials: HashMap = env .iter() .filter(|(key, _)| { - is_credential_broker_provider_env_key(key) - && !credential_broker_provider_context_env_keys() - .any(|context| context.eq_ignore_ascii_case(key)) - && !allowed_brokered_keys.contains(key) + allowed_metadata.provider_keys.iter().any(|provider_key| { + provider_key == *key || cfg!(windows) && provider_key.eq_ignore_ascii_case(key) + }) && !provider_context_keys.iter().any(|context| { + context == *key || cfg!(windows) && context.eq_ignore_ascii_case(key) + }) && !allowed_brokered_keys.contains(key) }) .map(|(key, value)| (key.clone(), value.clone())) .collect(); @@ -658,7 +721,11 @@ async fn capture_snapshot( let mut brokered_alias_keys = Vec::new(); let fail_open_aliases: HashMap = env .iter() - .filter(|(key, _)| !is_credential_broker_provider_env_key(key)) + .filter(|(key, _)| { + !allowed_metadata.provider_keys.iter().any(|provider_key| { + provider_key == *key || cfg!(windows) && provider_key.eq_ignore_ascii_case(key) + }) + }) .filter_map(|(key, value)| { let mut real_value = env_value(&restored_env, key) .or_else(|| env_value(&policy.r#set, key))? @@ -674,8 +741,13 @@ async fn capture_snapshot( env_value(&restored_env, source) .or_else(|| env_value(&policy.r#set, source)) .is_some_and(|source_value| { - real_value == *source_value - || source_value.len() >= 16 && real_value.contains(source_value) + credential_broker + .network_proxy + .credential_broker_source_matches_text( + source, + source_value, + &real_value, + ) }) }); let brokered_alias = !has_brokered_source @@ -688,23 +760,25 @@ async fn capture_snapshot( brokered_alias_keys.push(key.clone()); } - let authorized = credential_broker_provider_sources_allowed( - &real_value, - if brokered_alias { value } else { &virtualized }, - &restored_env, - |source| { - let allowed_sources = create_env_from_vars( - std::iter::once((source.to_string(), "1".to_string())), - policy, - /*thread_id*/ None, - ); - env_value(&allowed_sources, source) - .is_some_and(|source_value| !source_value.is_empty()) - && env_value(&policy.r#set, source).is_none_or(|source_value| { - env_value(&restored_env, source) == Some(source_value) - }) - }, - ); + let authorized = credential_broker + .network_proxy + .credential_broker_sources_allowed( + &real_value, + if brokered_alias { value } else { &virtualized }, + &restored_env, + |source| { + let allowed_sources = create_env_from_vars( + std::iter::once((source.to_string(), "1".to_string())), + policy, + /*thread_id*/ None, + ); + env_value(&allowed_sources, source) + .is_some_and(|source_value| !source_value.is_empty()) + && env_value(&policy.r#set, source).is_none_or(|source_value| { + env_value(&restored_env, source) == Some(source_value) + }) + }, + ); Some(( key.clone(), FailOpenAlias { @@ -721,10 +795,11 @@ async fn capture_snapshot( restored_env .iter() .filter(|&(key, value)| { - is_credential_broker_provider_env_key(key) - && !credential_broker_provider_context_env_keys() - .any(|context| context.eq_ignore_ascii_case(key)) - && !value.is_empty() + discovery_metadata.provider_keys.iter().any(|source| { + source == key || cfg!(windows) && source.eq_ignore_ascii_case(key) + }) && !provider_context_keys.iter().any(|context| { + context == key || cfg!(windows) && context.eq_ignore_ascii_case(key) + }) && !value.is_empty() }) .map(|(key, _)| key.clone()), ); @@ -737,12 +812,6 @@ async fn capture_snapshot( .filter(|(_, value)| !value.is_empty()) .map(|(key, _)| key.clone()), ); - let mut snapshot_env = env.clone(); - for key in credential_broker_provider_context_env_keys() { - if env_value(&original_env, key) != env_value(&env, key) { - remove_env_value(&mut snapshot_env, key); - } - } let Some(PreparedSnapshot { script: snapshot, aliases: mut credential_env, @@ -754,7 +823,7 @@ async fn capture_snapshot( restored: &restored_env, configured: &policy.r#set, discovered: &discovery_env, - allowed: &snapshot_env, + allowed: &snapshot_allowed_env, is_allowed_unset: &|key| { create_env_from_vars( std::iter::once((key.to_string(), String::new())), @@ -820,21 +889,19 @@ async fn capture_snapshot( startup_credential_keys.dedup(); let mut context_credential_keys = HashMap::>::new(); let mut binding_context_credential_keys = HashMap::>::new(); - for key in &allowed_brokered_keys { - let mut provider_env = env.clone(); - provider_env - .retain(|candidate, _| candidate == key || !allowed_brokered_keys.contains(candidate)); - for context_key in brokered_credential_binding_env_keys(&provider_env) { + for (key, value) in &credential_env { + let provider_metadata = credential_broker + .network_proxy + .credential_broker_environment_for_text(value, &env); + for context_key in provider_metadata.binding_keys { binding_context_credential_keys - .entry(context_key.to_string()) + .entry(context_key) .or_default() .push(key.clone()); } - for context_key in brokered_credential_env_keys(&provider_env) - .filter(|context_key| *context_key != key.as_str()) - { + for context_key in provider_metadata.context_keys { context_credential_keys - .entry(context_key.to_string()) + .entry(context_key) .or_default() .push(key.clone()); } @@ -861,14 +928,19 @@ async fn capture_snapshot( )) } -fn replace_provider_context_with_inherited( +fn replace_provider_context_with_trusted( env: &mut HashMap, - inherited_env: &HashMap, - context_keys: impl Iterator, + trusted_env: &HashMap, + context_keys: impl IntoIterator, + preserve_untrusted_keys: &[String], ) { for key in context_keys { - if let Some(value) = env_value(inherited_env, key) { - insert_env_value(env, key, value); + if let Some(value) = env_value(trusted_env, &key) { + insert_env_value(env, &key, value); + } else if !preserve_untrusted_keys.iter().any(|candidate| { + candidate == &key || cfg!(windows) && candidate.eq_ignore_ascii_case(&key) + }) { + remove_env_value(env, &key); } } } diff --git a/codex-rs/core/src/shell_snapshot_tests.rs b/codex-rs/core/src/shell_snapshot_tests.rs index 9222989e8f..4193e35221 100644 --- a/codex-rs/core/src/shell_snapshot_tests.rs +++ b/codex-rs/core/src/shell_snapshot_tests.rs @@ -8,6 +8,8 @@ use crate::tools::runtimes::maybe_wrap_shell_lc_with_snapshot; #[cfg(unix)] use crate::tools::runtimes::prepare_brokered_shell_snapshot_env; #[cfg(unix)] +use codex_network_proxy::CredentialProviderConfig; +#[cfg(unix)] use codex_network_proxy::NetworkProxyConfig; #[cfg(unix)] use codex_protocol::config_types::EnvironmentVariablePattern; @@ -94,13 +96,39 @@ fn assert_posix_snapshot_sections(snapshot: &str) { assert!(snapshot.contains("setopts ")); } +#[cfg(not(windows))] +#[test] +fn preserved_provider_context_respects_unix_environment_casing() { + let mut env = HashMap::from([ + ("GH_HOST".to_string(), "startup.example.com".to_string()), + ("gh_host".to_string(), "custom.example.com".to_string()), + ]); + + replace_provider_context_with_trusted( + &mut env, + &HashMap::new(), + ["GH_HOST".to_string(), "gh_host".to_string()], + &["gh_host".to_string()], + ); + + assert_eq!( + env, + HashMap::from([("gh_host".to_string(), "custom.example.com".to_string())]) + ); +} + #[cfg(windows)] #[test] fn inherited_provider_context_matches_windows_environment_casing() { let mut env = HashMap::new(); let inherited = HashMap::from([("gh_host".to_string(), "github.example.com".to_string())]); - replace_provider_context_with_inherited(&mut env, &inherited, std::iter::once("GH_HOST")); + replace_provider_context_with_trusted( + &mut env, + &inherited, + std::iter::once("GH_HOST".to_string()), + &[], + ); assert_eq!( env, @@ -473,8 +501,10 @@ async fn inactive_profiles_keep_snapshots_but_active_brokers_require_sandbox() - assert!(rebuilt.path().as_path().is_file()); let proxy = started_proxy.proxy(); let mut old_env = HashMap::new(); - rebuilt.restore_credentials(&mut old_env, environment.shell_environment_policy()); - proxy.apply_to_env(&mut old_env); + let context = rebuilt.restore_credentials(&mut old_env, environment.shell_environment_policy()); + old_env = context + .prepare_child_environment(&proxy, old_env, /*environment_id*/ None)? + .env; let original_config = proxy.current_cfg().await?; let mut changed_config = original_config.clone(); @@ -507,8 +537,11 @@ async fn inactive_profiles_keep_snapshots_but_active_brokers_require_sandbox() - .context("a changed broker configuration must invalidate cached snapshots")?; assert!(!Arc::ptr_eq(&rebuilt, &refreshed)); let mut new_env = HashMap::new(); - refreshed.restore_credentials(&mut new_env, environment.shell_environment_policy()); - proxy.apply_to_env(&mut new_env); + let context = + refreshed.restore_credentials(&mut new_env, environment.shell_environment_policy()); + new_env = context + .prepare_child_environment(&proxy, new_env, /*environment_id*/ None)? + .env; assert_ne!(new_env["OPENAI_API_KEY"], old_env["OPENAI_API_KEY"]); assert_ne!(new_env["OPENAI_API_KEY"], "sk-snapshot-cache-test"); @@ -567,6 +600,16 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul export GH_ENTERPRISE_TOKEN='ghp_enterprise_secret'\n\ export GH_HOST='attacker.example'\n\ export OPENAI_API_KEY='sk-proj-snapshot-secret'\n\ + export STRIPE_API_KEY='stripe_live_abcdefghijklmnopqrstuvwx'\n\ + export STRIPE_HOST='https://startup.stripe.example/v1'\n\ + export STRIPE_AUTH_HEADER=\"Bearer $STRIPE_API_KEY\"\n\ + export VENDOR_PASSWORD='pin_abcdefgh'\n\ + export VENDOR_AUTH_HEADER=\"Bearer $VENDOR_PASSWORD\"\n\ + export VENDOR_HOST='https://attacker.vendor.example/v2'\n\ + export LOCAL_TOKEN='local_abcdefghijklmnopqrstuvwx'\n\ + export LOCAL_URL='http://127.0.0.1:1234/v1'\n\ + export LOCAL_AUTH_HEADER=\"Bearer $LOCAL_TOKEN\"\n\ + unset LOCAL_TOKEN\n\ export AUTH_BUNDLE=\"GitHub $GH_TOKEN\n\ OpenAI $OPENAI_API_KEY\"\n\ export OPENAI_BASE_URL='https://api.snapshot.example/v1'\n\ @@ -579,6 +622,39 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul let mut network_config = NetworkProxyConfig::default(); network_config.set_credential_broker_enabled(/*enabled*/ true); + network_config.allow_local_binding = true; + network_config.credential_providers.insert( + "local".to_string(), + CredentialProviderConfig { + env: vec!["LOCAL_TOKEN".to_string()], + patterns: vec!["local_[a-z]{24}".to_string()], + url_prefixes: vec!["https://public.local-provider.example/v1".to_string()], + url_prefix_from_env: Some("LOCAL_URL".to_string()), + ..CredentialProviderConfig::default() + }, + ); + network_config.credential_providers.insert( + "stripe".to_string(), + CredentialProviderConfig { + env: vec!["STRIPE_API_KEY".to_string()], + patterns: vec!["stripe_live_[a-z]{24}".to_string()], + url_prefix_from_env: Some("STRIPE_HOST".to_string()), + ..CredentialProviderConfig::default() + }, + ); + network_config.credential_providers.insert( + "vendor".to_string(), + CredentialProviderConfig { + env: vec!["VENDOR_PASSWORD".to_string()], + patterns: vec!["pin_[a-z]{8}".to_string()], + url_prefix_from_env: Some("VENDOR_HOST".to_string()), + ..CredentialProviderConfig::default() + }, + ); + network_config.configure_credential_broker_environment(&HashMap::from([( + "STRIPE_HOST".to_string(), + "api.stripe.example".to_string(), + )])); let permission_profile = PermissionProfile::workspace_write(); let network_spec = NetworkProxySpec::from_config_and_constraints( network_config, @@ -602,7 +678,11 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul }; let trusted_startup = ("BASH_ENV".to_string(), startup.display().to_string()); let shell_environment_policy = ShellEnvironmentPolicy { - exclude: vec![EnvironmentVariablePattern::new_case_insensitive("HOME")], + exclude: vec![ + EnvironmentVariablePattern::new_case_insensitive("HOME"), + EnvironmentVariablePattern::new_case_insensitive("VENDOR_HOST"), + EnvironmentVariablePattern::new_case_insensitive("STRIPE_HOST"), + ], r#set: HashMap::from([ trusted_startup.clone(), ("GH_HOST".to_string(), "github.example.com".to_string()), @@ -621,6 +701,10 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul ("PATH".to_string(), "/enterprise/bin".to_string()), ("CORP_REGION".to_string(), "production".to_string()), ("NPM_TOKEN".to_string(), "npm_enterprise_token".to_string()), + ( + "VENDOR_HOST".to_string(), + "https://api.vendor.example/v2".to_string(), + ), ]), ..ShellEnvironmentPolicy::default() }; @@ -638,6 +722,29 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul /*sandbox*/ None, ) .await?; + let captured_credentials = credentials.as_ref().expect("brokered snapshot credentials"); + assert_eq!( + captured_credentials + .context_env + .get("STRIPE_HOST") + .map(String::as_str), + Some("api.stripe.example"), + "excluding a trusted destination must not let startup replace it" + ); + assert_eq!( + captured_credentials + .context_env + .get("VENDOR_HOST") + .map(String::as_str), + Some("https://api.vendor.example/v2") + ); + assert_eq!( + captured_credentials + .context_env + .get("OPENAI_BASE_URL") + .map(String::as_str), + Some("https://api.snapshot.example/v1") + ); let snapshot = fs::read_to_string(&path).await?; for secret in [ @@ -645,12 +752,17 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul "ghp_readonly_secret", "ghp_enterprise_secret", "sk-proj-snapshot-secret", + "stripe_live_abcdefghijklmnopqrstuvwx", + "pin_abcdefgh", + "local_abcdefghijklmnopqrstuvwx", "identity-token-secret", ] { assert!(!snapshot.contains(secret), "snapshot exposed {secret}"); } assert!(!snapshot.contains("attacker.example")); + assert!(!snapshot.contains("CODEX_NETWORK_PROXY_BROKERED_CREDENTIALS")); assert!(snapshot.contains("api.snapshot.example")); + assert!(!snapshot.contains("attacker.vendor.example")); assert!(snapshot.contains("IDENTITY_SEEN=\"missing\"")); assert!(snapshot.contains("EXCLUDED_PARENT_HOME=\"missing\"")); assert!(snapshot.contains("STARTUP_PATH_OVERRIDE_SEEN=\"/enterprise/bin\"")); @@ -658,6 +770,8 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul assert!(snapshot.contains("STARTUP_CORP_REGION_SEEN=\"production\"")); assert!(snapshot.contains("STARTUP_NPM_TOKEN_SEEN=\"npm_enterprise_token\"")); assert!(snapshot.contains("declare -rx HOMEBREW_GITHUB_API_TOKEN=\"${GITHUB_TOKEN-}\"")); + assert!(snapshot.contains("${VENDOR_PASSWORD-}")); + assert!(!snapshot.contains("api.stripe.example")); validate_snapshot( &shell, @@ -750,10 +864,13 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul ), ("GH_HOST".to_string(), "attacker.example".to_string()), ]); - replace_provider_context_with_inherited( + replace_provider_context_with_trusted( &mut inherited_discovery_env, &inherited_context, - credential_broker_provider_context_env_keys(), + network_proxy + .credential_broker_environment(&HashMap::new()) + .provider_context_keys, + &[], ); assert_eq!( inherited_discovery_env.get("GH_HOST").map(String::as_str), @@ -764,10 +881,13 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul "GH_ENTERPRISE_TOKEN".to_string(), inherited_secret.to_string(), )]); - replace_provider_context_with_inherited( + replace_provider_context_with_trusted( &mut inherited_allowed_env, &inherited_context, - brokered_credential_binding_env_keys(&inherited_discovery_env), + network_proxy + .credential_broker_environment(&inherited_discovery_env) + .binding_keys, + &[], ); network_proxy.apply_to_env(&mut inherited_allowed_env); assert_eq!( @@ -802,15 +922,22 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul for key in unset_credential_keys { unset_env.insert(key.clone(), unset_policy.r#set.remove(key).unwrap()); } - snapshot_file.restore_credentials(&mut unset_env, &unset_policy); - snapshot_file.restore_fail_open_aliases(&mut unset_env); + let _ = snapshot_file.restore_credentials(&mut unset_env, &unset_policy); + snapshot_file.restore_fail_open_aliases(&mut unset_env, /*environment_id*/ None); assert!( unset_credential_keys .iter() .all(|key| !unset_env.contains_key(key)) ); - let mut env = HashMap::from([("GH_HOST".to_string(), "github.example.com".to_string())]); + let mut env = HashMap::from([ + ("GH_HOST".to_string(), "github.example.com".to_string()), + ("STRIPE_HOST".to_string(), "api.stripe.example".to_string()), + ( + "VENDOR_HOST".to_string(), + "https://api.vendor.example/v2".to_string(), + ), + ]); env.extend( shell_environment_policy .r#set @@ -818,7 +945,7 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul .filter(|(key, _)| unset_credential_keys.contains(key)) .map(|(key, value)| (key.clone(), value.clone())), ); - snapshot_file.restore_credentials(&mut env, &shell_environment_policy); + let credential_context = snapshot_file.restore_credentials(&mut env, &shell_environment_policy); for (key, value) in [ ("GH_TOKEN", "ghp_shell_only_secret"), ("GITHUB_TOKEN", "ghp_readonly_secret"), @@ -826,6 +953,10 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul ("GITHUB_ENTERPRISE_TOKEN", "ghp_removed_enterprise_secret"), ("UNSET_AUTH_HEADER", "Bearer ghp_removed_enterprise_secret"), ("OPENAI_API_KEY", "sk-proj-snapshot-secret"), + ("STRIPE_API_KEY", "stripe_live_abcdefghijklmnopqrstuvwx"), + ("STRIPE_HOST", "api.stripe.example"), + ("VENDOR_PASSWORD", "pin_abcdefgh"), + ("VENDOR_HOST", "https://api.vendor.example/v2"), ] { assert_eq!(env.get(key).map(String::as_str), Some(value), "{key}"); } @@ -833,6 +964,120 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul env.get("OPENAI_BASE_URL").map(String::as_str), Some("https://api.snapshot.example/v1") ); + let explicit_vendor_policy = ShellEnvironmentPolicy { + inherit: codex_protocol::config_types::ShellEnvironmentPolicyInherit::None, + r#set: HashMap::from([("VENDOR_PASSWORD".to_string(), "pin_abcdefgh".to_string())]), + ..ShellEnvironmentPolicy::default() + }; + let mut explicit_vendor_env = explicit_vendor_policy.r#set.clone(); + let explicit_vendor_context = + snapshot_file.restore_credentials(&mut explicit_vendor_env, &explicit_vendor_policy); + assert!(!explicit_vendor_env.contains_key("VENDOR_HOST")); + assert_eq!( + explicit_vendor_context.with_fallbacks(&explicit_vendor_env)["VENDOR_HOST"], + "https://api.vendor.example/v2" + ); + explicit_vendor_env = explicit_vendor_context + .prepare_child_environment( + &network_proxy, + explicit_vendor_env, + /*environment_id*/ None, + )? + .env; + assert!(!explicit_vendor_env.contains_key("VENDOR_HOST")); + assert_ne!(explicit_vendor_env["VENDOR_PASSWORD"], "pin_abcdefgh"); + let prepared_vendor_env = explicit_vendor_env.clone(); + network_proxy.apply_to_env(&mut explicit_vendor_env); + assert_eq!(explicit_vendor_env, prepared_vendor_env); + + let current_vendor_host = "https://api.current-vendor.example/v2"; + let mut current_vendor_env = explicit_vendor_policy.r#set.clone(); + current_vendor_env.insert("VENDOR_HOST".to_string(), current_vendor_host.to_string()); + let current_vendor_context = + snapshot_file.restore_credentials(&mut current_vendor_env, &explicit_vendor_policy); + current_vendor_env = current_vendor_context + .prepare_child_environment( + &network_proxy, + current_vendor_env, + /*environment_id*/ None, + )? + .env; + assert_eq!( + current_vendor_env.get("VENDOR_HOST").map(String::as_str), + Some(current_vendor_host) + ); + assert_ne!(current_vendor_env["VENDOR_PASSWORD"], "pin_abcdefgh"); + + for excluded_context in [false, true] { + let mut policy = shell_environment_policy.clone(); + if excluded_context { + policy + .exclude + .push(EnvironmentVariablePattern::new_case_insensitive( + "OPENAI_BASE_URL", + )); + } + let ambient_host = "https://api.ambient.example/v1"; + let mut restored_env = + HashMap::from([("OPENAI_BASE_URL".to_string(), ambient_host.to_string())]); + let context = snapshot_file.restore_credentials(&mut restored_env, &policy); + assert_eq!( + restored_env["OPENAI_BASE_URL"], + if excluded_context { + ambient_host + } else { + "https://api.snapshot.example/v1" + } + ); + restored_env = context + .prepare_child_environment(&network_proxy, restored_env, /*environment_id*/ None)? + .env; + assert_ne!(restored_env["OPENAI_API_KEY"], "sk-proj-snapshot-secret"); + assert_eq!( + restored_env["OPENAI_BASE_URL"], + if excluded_context { + ambient_host + } else { + "https://api.snapshot.example/v1" + } + ); + } + + for (credential_key, context_key, real, current_host) in [ + ( + "GH_ENTERPRISE_TOKEN", + "GH_HOST", + "ghp_enterprise_secret", + "github.override.example", + ), + ( + "VENDOR_PASSWORD", + "VENDOR_HOST", + "pin_abcdefgh", + "https://api.current-vendor.example/v2", + ), + ] { + let policy = ShellEnvironmentPolicy { + ignore_default_excludes: true, + include_only: vec![EnvironmentVariablePattern::new_case_insensitive( + credential_key, + )], + r#set: HashMap::from([(context_key.to_string(), current_host.to_string())]), + ..ShellEnvironmentPolicy::default() + }; + let mut restored_env = create_env_from_vars([], &policy, /*thread_id*/ None); + let context = snapshot_file.restore_credentials(&mut restored_env, &policy); + assert!(!restored_env.contains_key(context_key)); + assert_eq!( + context.with_fallbacks(&restored_env)[context_key], + current_host + ); + restored_env = context + .prepare_child_environment(&network_proxy, restored_env, /*environment_id*/ None)? + .env; + assert_ne!(restored_env[credential_key], real, "{credential_key}"); + assert!(!restored_env.contains_key(context_key)); + } let unbrokered_replay = Command::new("/bin/bash") .arg("-c") @@ -848,28 +1093,54 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul "Bearer ghp_shell_only_secret\nGitHub ghp_shell_only_secret\nOpenAI sk-proj-snapshot-secret" ); - network_proxy.apply_to_env(&mut env); + let mut cleared_local_context = HashMap::from([ + ( + "LOCAL_AUTH_HEADER".to_string(), + snapshot_file + .credentials + .as_ref() + .unwrap() + .fail_open_aliases["LOCAL_AUTH_HEADER"] + .dummy_value + .clone() + .unwrap(), + ), + ("LOCAL_URL".to_string(), String::new()), + ]); + network_proxy.apply_to_env_for_snapshot(&mut cleared_local_context); + env = credential_context + .prepare_child_environment(&network_proxy, env, Some("snapshot-child"))? + .env; + snapshot_file.restore_fail_open_aliases(&mut env, Some("snapshot-child")); assert_ne!(env["GH_TOKEN"], "ghp_shell_only_secret"); assert_ne!(env["GITHUB_TOKEN"], "ghp_readonly_secret"); assert_ne!(env["GH_ENTERPRISE_TOKEN"], "ghp_enterprise_secret"); assert_ne!(env["OPENAI_API_KEY"], "sk-proj-snapshot-secret"); + assert_ne!( + env["STRIPE_API_KEY"], + "stripe_live_abcdefghijklmnopqrstuvwx" + ); + assert_ne!(env["VENDOR_PASSWORD"], "pin_abcdefgh"); + assert!(!env.contains_key("LOCAL_TOKEN")); + let mut replay_env = env.clone(); + replay_env.remove("STRIPE_HOST"); let snapshot_path = snapshot_file.path(); - prepare_brokered_shell_snapshot_env(&mut env, Some(&snapshot_path), &shell); + prepare_brokered_shell_snapshot_env(&mut replay_env, Some(&snapshot_path), &shell); let replay_command = maybe_wrap_shell_lc_with_snapshot( &shell.derive_exec_args( - "printf '%s\\n%s\\n%s' \"$HOMEBREW_GITHUB_API_TOKEN\" \"$AUTH_HEADER\" \"$AUTH_BUNDLE\"", + "printf '%s\\n%s\\n%s\\n%s\\n%s\\n%s\\n%s' \"$HOMEBREW_GITHUB_API_TOKEN\" \"$AUTH_HEADER\" \"$AUTH_BUNDLE\" \"$VENDOR_PASSWORD\" \"${VENDOR_HOST-missing}\" \"${STRIPE_HOST-missing}\" \"$LOCAL_AUTH_HEADER\"", /*use_login_shell*/ false, ), &shell, Some(&snapshot_path), &shell_environment_policy.r#set, - &env, + &replay_env, &RuntimePathPrepends::default(), ); let replay = Command::new(&replay_command[0]) .args(&replay_command[1..]) .env_clear() - .envs(&env) + .envs(&replay_env) .output()?; assert!( replay.status.success(), @@ -878,8 +1149,13 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul assert_eq!( String::from_utf8(replay.stdout)?, format!( - "{}\nBearer {}\nGitHub {}\nOpenAI {}", - env["GITHUB_TOKEN"], env["GH_TOKEN"], env["GH_TOKEN"], env["OPENAI_API_KEY"] + "{}\nBearer {}\nGitHub {}\nOpenAI {}\n{}\n{}\nmissing\nBearer local_abcdefghijklmnopqrstuvwx", + env["GITHUB_TOKEN"], + env["GH_TOKEN"], + env["GH_TOKEN"], + env["OPENAI_API_KEY"], + env["VENDOR_PASSWORD"], + env["VENDOR_HOST"] ) ); @@ -888,7 +1164,7 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul ..ShellEnvironmentPolicy::default() }; let mut filtered_env = HashMap::new(); - snapshot_file.restore_credentials(&mut filtered_env, &filtered_policy); + let _ = snapshot_file.restore_credentials(&mut filtered_env, &filtered_policy); assert!(filtered_env.is_empty()); let partially_filtered_credential_broker = SnapshotCredentialBroker { @@ -948,7 +1224,8 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul credentials: filtered_credentials, }; let mut fail_open_env = HashMap::new(); - filtered_snapshot.restore_fail_open_aliases(&mut fail_open_env); + filtered_snapshot + .restore_fail_open_aliases(&mut fail_open_env, /*environment_id*/ None); assert!(!fail_open_env.contains_key("AUTH_BUNDLE")); } @@ -1010,7 +1287,10 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul export HIDDEN_HEADER=\"prefix\\\"$GH_TOKEN\\\"suffix\"\n\ export GITHUB_TOKEN='ghp_0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefgh'\n\ export UNKNOWN_HEADER=\"Bearer $GITHUB_TOKEN\"\n\ - unset GH_TOKEN GITHUB_TOKEN GITHUB_ENTERPRISE_TOKEN\n", + export STRIPE_API_KEY='stripe_live_zyxwvutsrqponmlkjihgfedc'\n\ + export STRIPE_HIDDEN_HEADER=\"Bearer $STRIPE_API_KEY\"\n\ + export STRIPE_HOST='api.stripe.example'\n\ + unset GH_TOKEN GITHUB_TOKEN GITHUB_ENTERPRISE_TOKEN STRIPE_API_KEY\n", )?; let mut previously_brokered_env = HashMap::from([( "GH_TOKEN".to_string(), @@ -1037,15 +1317,19 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul let inherited_snapshot = fs::read_to_string(&inherited_path).await?; assert!(!inherited_snapshot.contains("ghp_hidden_alias_secret")); assert!(!inherited_snapshot.contains("ghp_0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefgh")); + assert!(!inherited_snapshot.contains("stripe_live_zyxwvutsrqponmlkjihgfedc")); let inherited_replay = Command::new("/bin/bash") .arg("-c") - .arg(". \"$1\" && printf '%s\\n%s' \"${HIDDEN_HEADER-unset}\" \"${UNKNOWN_HEADER-unset}\"") + .arg(". \"$1\" && printf '%s\\n%s\\n%s' \"${HIDDEN_HEADER-unset}\" \"${UNKNOWN_HEADER-unset}\" \"${STRIPE_HIDDEN_HEADER-unset}\"") .arg("snapshot") .arg(inherited_path.as_path()) .env_clear() .output()?; assert!(inherited_replay.status.success()); - assert_eq!(String::from_utf8(inherited_replay.stdout)?, "unset\nunset"); + assert_eq!( + String::from_utf8(inherited_replay.stdout)?, + "unset\nunset\nunset" + ); let snapshot_file = ShellSnapshotFile { path: inherited_path, @@ -1053,19 +1337,26 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul }; let real_hidden_header = "prefix\"ghp_hidden_alias_secret\"suffix"; let real_header = "Bearer ghp_0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefgh"; + let real_stripe_header = "Bearer stripe_live_zyxwvutsrqponmlkjihgfedc"; let mut brokered_env = HashMap::new(); - snapshot_file.restore_credentials( + let credential_context = snapshot_file.restore_credentials( &mut brokered_env, &inherited_credential_broker.shell_environment_policy, ); - inherited_credential_broker - .network_proxy - .apply_to_env(&mut brokered_env); + brokered_env = credential_context + .prepare_child_environment( + &inherited_credential_broker.network_proxy, + brokered_env, + /*environment_id*/ None, + )? + .env; let dummy_header = brokered_env["UNKNOWN_HEADER"].clone(); let dummy_hidden_header = brokered_env["HIDDEN_HEADER"].clone(); + let dummy_stripe_header = brokered_env["STRIPE_HIDDEN_HEADER"].clone(); assert_ne!(dummy_header, real_header); assert_ne!(dummy_hidden_header, real_hidden_header); - snapshot_file.restore_fail_open_aliases(&mut brokered_env); + assert_ne!(dummy_stripe_header, real_stripe_header); + snapshot_file.restore_fail_open_aliases(&mut brokered_env, /*environment_id*/ None); assert_eq!( brokered_env.get("UNKNOWN_HEADER").map(String::as_str), Some(dummy_header.as_str()) @@ -1074,13 +1365,17 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul brokered_env.get("HIDDEN_HEADER").map(String::as_str), Some(dummy_hidden_header.as_str()) ); + assert_eq!( + brokered_env.get("STRIPE_HIDDEN_HEADER").map(String::as_str), + Some(dummy_stripe_header.as_str()) + ); let mut fail_open_env = HashMap::new(); - snapshot_file.restore_credentials( + let _ = snapshot_file.restore_credentials( &mut fail_open_env, &inherited_credential_broker.shell_environment_policy, ); - snapshot_file.restore_fail_open_aliases(&mut fail_open_env); + snapshot_file.restore_fail_open_aliases(&mut fail_open_env, /*environment_id*/ None); assert_eq!( fail_open_env.get("UNKNOWN_HEADER").map(String::as_str), Some(real_header) @@ -1089,6 +1384,12 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul fail_open_env.get("HIDDEN_HEADER").map(String::as_str), Some(real_hidden_header) ); + assert_eq!( + fail_open_env + .get("STRIPE_HIDDEN_HEADER") + .map(String::as_str), + Some(real_stripe_header) + ); std::fs::write( &startup, @@ -1147,9 +1448,11 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul }; let policy = &inherited_credential_broker.shell_environment_policy; let mut restored = HashMap::new(); - snapshot_file.restore_credentials(&mut restored, policy); - network_proxy.apply_to_env(&mut restored); - snapshot_file.restore_fail_open_aliases(&mut restored); + let context = snapshot_file.restore_credentials(&mut restored, policy); + restored = context + .prepare_child_environment(&network_proxy, restored, /*environment_id*/ None)? + .env; + snapshot_file.restore_fail_open_aliases(&mut restored, /*environment_id*/ None); assert_eq!( restored.get("GH_ENTERPRISE_TOKEN").map(String::as_str), Some(real) @@ -1162,7 +1465,7 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul "GH_ENTERPRISE_TOKEN", )); let mut filtered = HashMap::new(); - snapshot_file.restore_credentials(&mut filtered, &excluded); + let _ = snapshot_file.restore_credentials(&mut filtered, &excluded); assert!(!filtered.contains_key("GH_ENTERPRISE_TOKEN")); let mut overridden = policy.clone(); @@ -1170,7 +1473,7 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul .r#set .insert("GH_ENTERPRISE_TOKEN".to_string(), String::new()); let mut empty = overridden.r#set.clone(); - snapshot_file.restore_credentials(&mut empty, &overridden); + let _ = snapshot_file.restore_credentials(&mut empty, &overridden); assert_eq!( empty.get("GH_ENTERPRISE_TOKEN").map(String::as_str), Some("") @@ -1201,9 +1504,11 @@ async fn snapshot_discovers_and_redacts_shell_initialized_credentials() -> Resul }; std::fs::write(&snapshot_file.path, snapshot)?; let mut env = HashMap::from([("AUTH_HEADER".to_string(), format!("Bearer {real}"))]); - snapshot_file.restore_credentials(&mut env, &policy); - network_proxy.apply_to_env(&mut env); - snapshot_file.restore_fail_open_aliases(&mut env); + let context = snapshot_file.restore_credentials(&mut env, &policy); + env = context + .prepare_child_environment(&network_proxy, env, /*environment_id*/ None)? + .env; + snapshot_file.restore_fail_open_aliases(&mut env, /*environment_id*/ None); let replayed = Command::new("/bin/bash") .args([ "-c", diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index d6a04c4e7f..05251b03b5 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -339,11 +339,16 @@ impl<'a> ToolRuntime for UnifiedExecRunt }; let shell_snapshot_location = shell_snapshot.as_ref().map(|snapshot| snapshot.path()); let mut env = exec_env_for_sandbox_permissions(&req.env, launch_sandbox_permissions); - if let Some(snapshot) = shell_snapshot.as_ref() + let snapshot_credential_context = if let Some(snapshot) = shell_snapshot.as_ref() && (managed_network.is_some() || base_command.get(1).is_some_and(|flag| flag == "-lc")) { - snapshot.restore_credentials(&mut env, req.turn_environment.shell_environment_policy()); - } + Some( + snapshot + .restore_credentials(&mut env, req.turn_environment.shell_environment_policy()), + ) + } else { + None + }; let (mut env, managed_network_context, network_proxy_launch) = match managed_network { Some(network) if environment_is_remote => { let mut launch = network.remote_launch_config().await.map_err(|err| { @@ -391,8 +396,10 @@ impl<'a> ToolRuntime for UnifiedExecRunt } } Some(network) => { - let prepared = network - .prepare_for_optional_environment( + let prepared = snapshot_credential_context + .unwrap_or_default() + .prepare_child_environment( + network, env, Some(&req.turn_environment.selection.environment_id), ) @@ -407,7 +414,10 @@ impl<'a> ToolRuntime for UnifiedExecRunt None => (env, None, None), }; if let Some(snapshot) = shell_snapshot.as_ref() { - snapshot.restore_fail_open_aliases(&mut env); + snapshot.restore_fail_open_aliases( + &mut env, + Some(&req.turn_environment.selection.environment_id), + ); } if managed_network.is_some() && !environment_is_remote @@ -445,7 +455,23 @@ impl<'a> ToolRuntime for UnifiedExecRunt env.insert(format!("{prefix}{key}"), value); } } - let explicit_env_overrides = req.explicit_env_overrides.clone(); + let mut explicit_env_overrides = req.explicit_env_overrides.clone(); + if let Some(network) = managed_network + && !environment_is_remote + { + let broker = network.credential_broker_environment(&env); + for key in broker + .credential_keys + .into_iter() + .chain(broker.context_keys) + { + if !codex_network_proxy::is_credential_broker_provider_env_key(&key) + && let Some(value) = env.get(&key) + { + explicit_env_overrides.insert(key, value.clone()); + } + } + } let metrics_sidecar = sidecar_for_command( ctx, &req.command, diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index efa1b98a88..9f68417829 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -2807,6 +2807,7 @@ async fn spawned_subagent_execpolicy_amendment_propagates_to_parent_session() -> #[test_case("direct", true, true; "login_startup")] #[test_case("filtered_startup", false, false; "filtered_global_startup")] #[test_case("startup_override", false, false; "global_startup_overrides")] +#[test_case("additional_permissions", false, false; "filtered_host_additional_permissions")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn shell_startup_credentials_are_brokered( shell_mode: &'static str, @@ -2818,8 +2819,16 @@ async fn shell_startup_credentials_are_brokered( const GH_HOST: &str = "github.example.com"; const REAL_GITHUB_TOKEN: &str = "ghp_abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; + const CUSTOM_HOST: &str = "api.custom.example"; + const REAL_CUSTOM_TOKEN: &str = "custom_abcdefghijklmnopqrstuvwx"; + let request_extra_permissions = shell_mode == "additional_permissions"; + let approval_policy = if request_extra_permissions { + AskForApproval::OnRequest + } else { + AskForApproval::Never + }; let global_startup = matches!(shell_mode, "filtered_startup" | "startup_override"); - let direct = shell_mode == "direct" || global_startup; + let direct = shell_mode == "direct" || global_startup || request_extra_permissions; let builder = if shell_mode == "zsh_fork" { let Some(runtime) = zsh_fork_runtime("zsh-fork credential broker environment test")? else { @@ -2830,10 +2839,12 @@ async fn shell_startup_credentials_are_brokered( let Some(zsh) = codex_core::shell::get_shell(codex_core::shell::ShellType::Zsh) else { return Ok(()); }; - test_codex().with_user_shell(zsh).with_config(|config| { - config.permissions.approval_policy = Constrained::allow_any(AskForApproval::Never); - config.features.enable(Feature::ShellTool).unwrap(); - }) + test_codex() + .with_user_shell(zsh) + .with_config(move |config| { + config.permissions.approval_policy = Constrained::allow_any(approval_policy); + config.features.enable(Feature::ShellTool).unwrap(); + }) }; let outside_dir = tempfile::tempdir_in(std::env::current_dir()?)?; @@ -2848,8 +2859,10 @@ async fn shell_startup_credentials_are_brokered( format!( "export GH_HOST='{GH_HOST}'\n\ if [[ ! -o login ]]; then export GH_ENTERPRISE_TOKEN='{REAL_GITHUB_TOKEN}'; fi\n\ - export AUTH_HEADER=\"Bearer $GH_ENTERPRISE_TOKEN\"\n\ + export CUSTOM_API_KEY='{REAL_CUSTOM_TOKEN}'\n\ + export CUSTOM_HOST='{CUSTOM_HOST}'\n\ export APP_SETTING EXCLUDED_SETTING\n\ + export AUTH_HEADER=\"Bearer $GH_ENTERPRISE_TOKEN\"\n\ export BROKERED_STARTUP_CWD=\"$PWD\"\n\ if {untrusted_snapshot_condition}; then\n\ print -r -- escaped > {outside_path_arg} 2>/dev/null || true\n\ @@ -2882,7 +2895,7 @@ async fn shell_startup_credentials_are_brokered( fs::write( &path, format!( - "#!/bin/sh\nexport GH_ENTERPRISE_TOKEN='{REAL_GITHUB_TOKEN}'\nexport AUTH_HEADER=\"Bearer $GH_ENTERPRISE_TOKEN\"\nexec {} \"$@\"\n", + "#!/bin/sh\nexport GH_ENTERPRISE_TOKEN='{REAL_GITHUB_TOKEN}'\nexport CUSTOM_API_KEY='{REAL_CUSTOM_TOKEN}'\nexport AUTH_HEADER=\"Bearer $GH_ENTERPRISE_TOKEN\"\nexec {} \"$@\"\n", shlex::try_quote(&zsh_path)? ), )?; @@ -2895,9 +2908,17 @@ async fn shell_startup_credentials_are_brokered( home.path().join("config.toml"), format!( r#"default_permissions = "brokered" -features = {{ network_proxy = {{ enabled = true, credential_broker = true }} }} permissions = {{ brokered = {{ extends = ":workspace", network = {{ enabled = true, allow_local_binding = true }} }} }} +[features.network_proxy] +enabled = true +credential_broker = true + +[features.network_proxy.credentials.custom] +env = ["CUSTOM_API_KEY"] +patterns = ["custom_[a-z]{{24}}"] +url_prefix_from_env = "CUSTOM_HOST" + [shell_environment_policy.set] ZDOTDIR = "{}" "#, @@ -2918,13 +2939,27 @@ ZDOTDIR = "{}" config.permissions.shell_environment_policy.exclude.push( EnvironmentVariablePattern::new_case_insensitive("EXCLUDED_SETTING"), ); - if shell_mode == "filtered_startup" { + if request_extra_permissions { + config + .features + .enable(Feature::ExecPermissionApprovals) + .unwrap(); + config + .features + .enable(Feature::RequestPermissionsTool) + .unwrap(); config.permissions.shell_environment_policy.exclude.push( - EnvironmentVariablePattern::new_case_insensitive("GH_ENTERPRISE_TOKEN"), + EnvironmentVariablePattern::new_case_insensitive("CUSTOM_HOST"), ); + } else if shell_mode == "filtered_startup" { + config.permissions.shell_environment_policy.exclude.extend([ + EnvironmentVariablePattern::new_case_insensitive("GH_ENTERPRISE_TOKEN"), + EnvironmentVariablePattern::new_case_insensitive("CUSTOM_API_KEY"), + ]); } else if shell_mode == "startup_override" { config.permissions.shell_environment_policy.r#set.extend([ ("GH_ENTERPRISE_TOKEN".to_string(), String::new()), + ("CUSTOM_API_KEY".to_string(), String::new()), ("AUTH_HEADER".to_string(), "enterprise-header".to_string()), ]); } @@ -2950,13 +2985,19 @@ ZDOTDIR = "{}" workdir }; let call_id = "zsh-fork-brokered-github-credential"; - let command = r#"printf '%s\n%s\n%s\n%s\n%s\n%s\n%s\n' "$GH_HOST" "$GH_ENTERPRISE_TOKEN" "$CODEX_NETWORK_PROXY_CREDENTIAL_BROKER_ACTIVE" "$PWD" "$BROKERED_STARTUP_CWD" "${LOGIN_SNAPSHOT_READY-unset}" "$AUTH_HEADER""#; + let command = r#"printf '%s\n%s\n%s\n%s\n%s\n%s\n%s\n%s\n%s\n' "$GH_HOST" "$GH_ENTERPRISE_TOKEN" "$CUSTOM_HOST" "$CUSTOM_API_KEY" "$CODEX_NETWORK_PROXY_CREDENTIAL_BROKER_ACTIVE" "$PWD" "$BROKERED_STARTUP_CWD" "${LOGIN_SNAPSHOT_READY-unset}" "$AUTH_HEADER""#; let command = format!( "APP_SETTING=production; EXCLUDED_SETTING=denied; [ \"$(/usr/bin/printenv APP_SETTING)\" = production ] || exit 1; if /usr/bin/printenv EXCLUDED_SETTING >/dev/null; then exit 1; fi; {command}" ); + let command = if request_extra_permissions { + let script = format!("{command}\nprintf escaped 2>/dev/null > {outside_path_arg} || true"); + format!("/bin/sh -c {}", shlex::try_quote(&script)?) + } else { + command.to_string() + }; let snapshot_dir_arg = shlex::try_join([snapshot_dir.to_string_lossy().as_ref()])?; let command = format!("{command}; /bin/cat {snapshot_dir_arg}/*.sh > captured-snapshot"); - let arguments = if direct { + let mut arguments = if direct { let Some(zsh) = codex_core::shell::get_shell(codex_core::shell::ShellType::Zsh) else { return Ok(()); }; @@ -2978,6 +3019,12 @@ ZDOTDIR = "{}" "yield_time_ms": 10_000, }) }; + if request_extra_permissions { + let approved_dir = test.home.path().join("approved-command"); + fs::create_dir(&approved_dir)?; + arguments["sandbox_permissions"] = json!(SandboxPermissions::WithAdditionalPermissions); + arguments["additional_permissions"] = json!({"file_system": {"write": [approved_dir]}}); + } let event = ev_function_call(call_id, "exec_command", &serde_json::to_string(&arguments)?); let responses = mount_sse_sequence( &server, @@ -2998,10 +3045,35 @@ ZDOTDIR = "{}" submit_turn_preserving_active_permission_profile( &test, "show the brokered GitHub environment", - AskForApproval::Never, + approval_policy, ) .await?; - wait_for_completion_without_approval(&test).await; + if request_extra_permissions { + let mut approvals = 0; + loop { + let event = wait_for_event(&test.codex, |event| { + matches!( + event, + EventMsg::ExecApprovalRequest(_) | EventMsg::TurnComplete(_) + ) + }) + .await; + let EventMsg::ExecApprovalRequest(approval) = event else { + break; + }; + approvals += 1; + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::Approved, + }) + .await?; + } + assert!(approvals > 0, "additional permissions must be approved"); + } else { + wait_for_completion_without_approval(&test).await; + } let output = responses.requests()[1].function_call_output(call_id); if shell_mode == "zsh_fork" { @@ -3014,14 +3086,16 @@ ZDOTDIR = "{}" let result = parse_result(&output); assert_eq!(result.exit_code, Some(0), "command failed: {result:?}"); assert!(!result.stdout.contains(REAL_GITHUB_TOKEN)); + assert!(!result.stdout.contains(REAL_CUSTOM_TOKEN)); let values = result.stdout.lines().collect::>(); - assert_eq!(values.len(), 7, "unexpected command output: {result:?}"); + assert_eq!(values.len(), 9, "unexpected command output: {result:?}"); assert_eq!(values[0], GH_HOST); assert_ne!(values[1], REAL_GITHUB_TOKEN); if global_startup { assert_eq!(values[1], ""); + assert_eq!(values[3], ""); assert_eq!( - values[6], + values[8], if shell_mode == "startup_override" { "enterprise-header" } else { @@ -3031,12 +3105,23 @@ ZDOTDIR = "{}" } else { assert!(values[1].starts_with("ghp_")); assert_eq!(values[1].len(), REAL_GITHUB_TOKEN.len()); - assert_eq!(values[6], format!("Bearer {}", values[1])); + assert!(values[3].starts_with("custom_")); + assert_eq!(values[3].len(), REAL_CUSTOM_TOKEN.len()); + assert_eq!(values[8], format!("Bearer {}", values[1])); } - assert_eq!(values[2], "1"); - assert_eq!(PathBuf::from(values[3]), fs::canonicalize(&workdir)?); - assert_eq!(PathBuf::from(values[4]), fs::canonicalize(&workdir)?); - assert_eq!(values[5], if login { "ready" } else { "unset" }); + assert_eq!( + values[2], + if request_extra_permissions { + "" + } else { + CUSTOM_HOST + } + ); + assert_ne!(values[3], REAL_CUSTOM_TOKEN); + assert_eq!(values[4], "1"); + assert_eq!(PathBuf::from(values[5]), fs::canonicalize(&workdir)?); + assert_eq!(PathBuf::from(values[6]), fs::canonicalize(&workdir)?); + assert_eq!(values[7], if login { "ready" } else { "unset" }); assert!( !outside_path.exists(), "shell snapshot startup wrote outside the command sandbox" @@ -3048,6 +3133,10 @@ ZDOTDIR = "{}" !snapshot.contains(REAL_GITHUB_TOKEN), "shell snapshot persisted a real GitHub credential" ); + assert!( + !snapshot.contains(REAL_CUSTOM_TOKEN), + "shell snapshot persisted a real configured credential" + ); Ok(()) } diff --git a/codex-rs/features/Cargo.toml b/codex-rs/features/Cargo.toml index 95be6c400d..03721949f2 100644 --- a/codex-rs/features/Cargo.toml +++ b/codex-rs/features/Cargo.toml @@ -13,6 +13,7 @@ path = "src/lib.rs" workspace = true [dependencies] +codex-network-proxy = { workspace = true } codex-otel = { workspace = true } codex-protocol = { workspace = true } schemars = { workspace = true } diff --git a/codex-rs/features/src/feature_configs.rs b/codex-rs/features/src/feature_configs.rs index b933b03615..1482439206 100644 --- a/codex-rs/features/src/feature_configs.rs +++ b/codex-rs/features/src/feature_configs.rs @@ -1,5 +1,6 @@ use crate::FeatureConfig; use crate::FeatureToml; +use codex_network_proxy::CredentialProviderConfig; use codex_protocol::openai_models::ReasoningEffort; use schemars::JsonSchema; use serde::Deserialize; @@ -478,6 +479,8 @@ pub struct NetworkProxyConfigToml { pub allow_local_binding: Option, #[serde(skip_serializing_if = "Option::is_none")] pub credential_broker: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub credentials: Option>, } impl FeatureConfig for NetworkProxyConfigToml { diff --git a/codex-rs/network-proxy/src/config.rs b/codex-rs/network-proxy/src/config.rs index 4c203eb826..0a880282ed 100644 --- a/codex-rs/network-proxy/src/config.rs +++ b/codex-rs/network-proxy/src/config.rs @@ -139,6 +139,9 @@ pub struct NetworkProxyConfig { pub mitm: bool, #[serde(default)] pub credential_broker: bool, + /// Whether brokerage enabled MITM rather than inheriting an explicit setting. + #[serde(skip)] + pub credential_broker_enabled_mitm: bool, #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] pub credential_providers: BTreeMap, /// Trusted OpenAI endpoint derived from local configuration, never sent to remote executors. @@ -170,6 +173,7 @@ impl Default for NetworkProxyConfig { allow_local_binding: false, mitm: false, credential_broker: false, + credential_broker_enabled_mitm: false, credential_providers: BTreeMap::new(), credential_broker_openai_host: None, credential_broker_context: crate::CredentialBrokerContext::default(), @@ -182,7 +186,13 @@ impl Default for NetworkProxyConfig { impl NetworkProxyConfig { pub fn set_credential_broker_enabled(&mut self, enabled: bool) { self.credential_broker = enabled; - self.mitm |= enabled; + if enabled { + self.credential_broker_enabled_mitm |= !self.mitm; + self.mitm = true; + } else if self.credential_broker_enabled_mitm { + self.mitm = !self.mitm_hooks.is_empty(); + self.credential_broker_enabled_mitm = false; + } } pub fn set_credential_broker_openai_base_url(&mut self, base_url: Option<&str>) { @@ -691,6 +701,7 @@ mod tests { allow_local_binding: false, mitm: false, credential_broker: false, + credential_broker_enabled_mitm: false, credential_providers: BTreeMap::new(), credential_broker_openai_host: None, credential_broker_context: crate::CredentialBrokerContext::default(), @@ -700,6 +711,56 @@ mod tests { ); } + #[test] + fn disabling_credential_broker_restores_independent_mitm_setting() { + for (mitm, add_hook) in [(false, false), (true, false), (false, true)] { + let mut original = NetworkProxyConfig { + enabled: true, + mitm, + ..Default::default() + }; + let mut config = original.clone(); + for _ in 0..2 { + config.set_credential_broker_enabled(/*enabled*/ true); + } + if add_hook { + config.mitm_hooks.push(MitmHookConfig { + host: "api.example".to_string(), + ..Default::default() + }); + original.mitm_hooks.clone_from(&config.mitm_hooks); + original.mitm = true; + } + for _ in 0..2 { + config.set_credential_broker_enabled(/*enabled*/ false); + } + assert_eq!(config, original); + assert_eq!( + crate::RemoteNetworkProxyConfig::from_effective_config(&config).is_err(), + original.mitm + ); + } + } + + #[test] + #[cfg(windows)] + fn ambiguous_credential_environment_preserves_remote_proxy_support() { + let mut config = NetworkProxyConfig { + enabled: true, + ..Default::default() + }; + let expected = crate::RemoteNetworkProxyConfig::from_effective_config(&config).unwrap(); + config.set_credential_broker_enabled(/*enabled*/ true); + config.configure_credential_broker_environment(&HashMap::from([ + ("GH_HOST".to_string(), "first.example".to_string()), + ("gh_host".to_string(), "second.example".to_string()), + ])); + assert_eq!( + crate::RemoteNetworkProxyConfig::from_effective_config(&config).unwrap(), + expected + ); + } + #[test] #[cfg(unix)] fn credential_broker_context_accepts_non_unicode_environment() { diff --git a/codex-rs/network-proxy/src/credential_broker/environment.rs b/codex-rs/network-proxy/src/credential_broker/environment.rs index 55314fb839..4455e217b0 100644 --- a/codex-rs/network-proxy/src/credential_broker/environment.rs +++ b/codex-rs/network-proxy/src/credential_broker/environment.rs @@ -12,7 +12,9 @@ use super::providers; use super::registry::BrokeredCredentialProvider; use super::remove_env_value; use super::set_env_value; +use crate::NetworkProxy; use crate::NetworkProxyConfig; +use crate::PreparedManagedNetwork; use std::borrow::Cow; use std::collections::HashMap; @@ -20,6 +22,12 @@ use std::collections::HashMap; #[derive(Clone, Default, Eq, PartialEq)] pub struct CredentialBrokerContext(HashMap); +impl From> for CredentialBrokerContext { + fn from(env: HashMap) -> Self { + Self(env) + } +} + impl std::fmt::Debug for CredentialBrokerContext { fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { formatter.write_str("CredentialBrokerContext()") @@ -27,6 +35,34 @@ impl std::fmt::Debug for CredentialBrokerContext { } impl CredentialBrokerContext { + /// Uses private destination hints during brokerage and returns an environment ready to spawn. + /// Context values override routing inputs, but never change the child's visible values. + pub fn prepare_child_environment( + self, + proxy: &NetworkProxy, + mut env: HashMap, + environment_id: Option<&str>, + ) -> anyhow::Result { + let previous = self + .0 + .into_iter() + .map(|(key, value)| { + let previous = + env_entry(&env, &key).map(|(key, value)| (key.to_string(), value.to_string())); + set_env_value(&mut env, &key, value); + (key, previous) + }) + .collect::>(); + let mut prepared = proxy.prepare_for_optional_environment(env, environment_id)?; + for (key, previous) in previous { + remove_env_value(&mut prepared.env, &key); + if let Some((key, value)) = previous { + prepared.env.insert(key, value); + } + } + Ok(prepared) + } + pub(crate) fn capture( config: &NetworkProxyConfig, overrides: &HashMap, @@ -50,7 +86,8 @@ impl CredentialBrokerContext { ) } - pub(super) fn with_fallbacks<'a>( + /// Resolves destination hints without applying or changing child environment policy. + pub fn with_fallbacks<'a>( &self, env: &'a HashMap, ) -> Cow<'a, HashMap> { diff --git a/codex-rs/network-proxy/src/proxy.rs b/codex-rs/network-proxy/src/proxy.rs index eb5ec10410..14ca1a8819 100644 --- a/codex-rs/network-proxy/src/proxy.rs +++ b/codex-rs/network-proxy/src/proxy.rs @@ -2157,6 +2157,58 @@ mod tests { Ok(()) } + #[tokio::test] + async fn private_credential_context_prepares_spawn_ready_env() -> Result<()> { + #[cfg(target_os = "windows")] + let _permit = WINDOWS_INGRESS_TEST_LOCK.acquire().await.unwrap(); + let mut config = NetworkProxyConfig { + enabled: true, + ..NetworkProxyConfig::default() + }; + config.set_credential_broker_enabled(/*enabled*/ true); + let state = Arc::new(network_proxy_state_for_policy(config)); + let proxy = NetworkProxy::builder() + .state(Arc::clone(&state)) + .build() + .await?; + let handle = proxy.run().await?; + let real = "ghp-private-context-test"; + for previous in [None, Some(""), Some("visible.example")] { + let mut env = HashMap::from([("GH_ENTERPRISE_TOKEN".to_string(), real.to_string())]); + if let Some(previous) = previous { + env.insert("GH_HOST".to_string(), previous.to_string()); + } + let context = crate::CredentialBrokerContext::from(HashMap::from([( + "GH_HOST".to_string(), + "private.enterprise.example".to_string(), + )])); + let prepared = + context.prepare_child_environment(&proxy, env, Some("snapshot-context"))?; + assert_eq!(prepared.env.get("GH_HOST").map(String::as_str), previous); + let dummy = &prepared.env["GH_ENTERPRISE_TOKEN"]; + assert_ne!(dummy, real); + for (host, expected) in [ + ("private.enterprise.example", real), + ("visible.example", dummy.as_str()), + ] { + let mut headers = rama_http::HeaderMap::new(); + headers.insert( + rama_http::header::AUTHORIZATION, + format!("Bearer {dummy}").parse()?, + ); + state + .for_environment_id(Some("snapshot-context")) + .inject_request_credentials(host, &mut headers); + assert_eq!( + headers[rama_http::header::AUTHORIZATION], + format!("Bearer {expected}") + ); + } + } + handle.shutdown().await?; + Ok(()) + } + #[tokio::test] async fn remote_launch_config_carries_execution_scope() -> Result<()> { #[cfg(target_os = "windows")] diff --git a/codex-rs/shell-command/src/shell_snapshot_credentials.rs b/codex-rs/shell-command/src/shell_snapshot_credentials.rs index 66a96f7b65..8a74c3cc89 100644 --- a/codex-rs/shell-command/src/shell_snapshot_credentials.rs +++ b/codex-rs/shell-command/src/shell_snapshot_credentials.rs @@ -60,9 +60,32 @@ pub fn prepare_snapshot_credentials( .iter() .filter(|credential_key| { real_credential_value(credential_key).is_some_and(|real| { - value == real || real.len() >= 16 && value.contains(real) - }) || discovered.get(*credential_key).is_some_and(|dummy| { - value == dummy || dummy.len() >= 16 && value.contains(dummy) + value == real + || value.contains(real) + && (real.len() >= 16 + || discovered.get(*credential_key).is_some_and(|dummy| { + discovered.get(key).is_some_and(|virtualized| { + virtualized != value && virtualized.contains(dummy) + }) + })) + }) || [ + original.get(*credential_key), + discovered.get(*credential_key), + ] + .into_iter() + .flatten() + .filter(|dummy| !dummy.is_empty()) + .any(|dummy| { + value == dummy + || value.contains(dummy) + && (dummy.len() >= 16 + || real_credential_value(credential_key).is_some_and(|real| { + // Confirm short spans with the broker's path and pattern rules. + let mut restored = value.replace(dummy, real); + restored != *value + && virtualize_text(&mut restored) + && discovered.get(key) == Some(&restored) + })) }) }) .map(String::as_str) diff --git a/codex-rs/shell-command/src/shell_snapshot_tests.rs b/codex-rs/shell-command/src/shell_snapshot_tests.rs index c1d39964ba..c8b877d40d 100644 --- a/codex-rs/shell-command/src/shell_snapshot_tests.rs +++ b/codex-rs/shell-command/src/shell_snapshot_tests.rs @@ -528,7 +528,12 @@ fn brokered_zsh_snapshot_preserves_tied_credential_aliases() -> Result<()> { let brokered_keys = vec!["VENDOR_PASSWORD".to_string()]; for separator in [":", "|", "'", "\t", "$", "_", "x"] { let appended = format!("ordinary{separator}other"); - for rcquotes in [false, true] { + for (rcquotes, captured_credential) in [ + (false, real), + (true, real), + (false, "captured$0123456789abcdef"), + (true, "pin_ab$cd"), + ] { let script = snapshot_capture_script(ShellType::Zsh, CAPTURE_NON_INTERACTIVE).unwrap(); let setup = format!( "{}\nexport -UT SERVICE_URLS service_urls=( \"https://user:$VENDOR_PASSWORD@example.invalid/path\" '' {} ) {}\nservice_urls+=({})\n{script}", @@ -542,11 +547,14 @@ fn brokered_zsh_snapshot_preserves_tied_credential_aliases() -> Result<()> { shlex::try_quote(&appended)?, ); let original = HashMap::from([ - ("VENDOR_PASSWORD".to_string(), real.to_string()), + ( + "VENDOR_PASSWORD".to_string(), + captured_credential.to_string(), + ), ( "SERVICE_URLS".to_string(), [ - format!("https://user:{real}@example.invalid/path"), + format!("https://user:{captured_credential}@example.invalid/path"), String::new(), last.to_string(), appended.clone(), @@ -557,21 +565,25 @@ fn brokered_zsh_snapshot_preserves_tied_credential_aliases() -> Result<()> { let captured = Command::new("/bin/zsh") .args(["-fc", &setup]) .env_clear() - .env("VENDOR_PASSWORD", real) + .env("VENDOR_PASSWORD", captured_credential) .env("PATH", "/usr/bin:/bin") .output()?; assert!(captured.status.success(), "{captured:?}"); let mut snapshot = String::from_utf8(captured.stdout)?; let allowed = original .iter() - .map(|(key, value)| (key.clone(), value.replace(real, dummy))) + .map(|(key, value)| (key.clone(), value.replace(captured_credential, dummy))) + .collect::>(); + let restored = original + .iter() + .map(|(key, value)| (key.clone(), value.replace(captured_credential, real))) .collect::>(); let rewritten = rewrite_snapshot_credentials( ShellType::Zsh, &mut snapshot, SnapshotCredentialEnvironment { original: &original, - restored: &original, + restored: &restored, configured: &HashMap::new(), discovered: &allowed, allowed: &allowed, @@ -581,7 +593,9 @@ fn brokered_zsh_snapshot_preserves_tied_credential_aliases() -> Result<()> { allowed_brokered_keys: &brokered_keys, }, |text| { - *text = text.replace(real, dummy); + *text = text + .replace(real, dummy) + .replace(captured_credential, dummy); true }, ) @@ -601,7 +615,7 @@ fn brokered_zsh_snapshot_preserves_tied_credential_aliases() -> Result<()> { .env("VENDOR_PASSWORD", live) .output()?; assert!(replayed.status.success(), "{replayed:?}"); - let scalar = original["SERVICE_URLS"].replace(real, live); + let scalar = original["SERVICE_URLS"].replace(captured_credential, live); let mut array = vec![ format!("https://user:{live}@example.invalid/path"), String::new(), @@ -1895,6 +1909,178 @@ fn brokered_snapshot_alias_values_follow_allowed_credential_overrides() { } } +#[test] +fn brokered_short_alias_respects_policy_override() -> Result<()> { + let real = "pin_abcdefgh"; + let dummy = "pin_qrstuvwx"; + let keys = vec!["VENDOR_PASSWORD".to_string()]; + for captured in [real, "pin_ijklmnop"] { + let original = HashMap::from([ + ("VENDOR_PASSWORD".to_string(), captured.to_string()), + ("AUTH_HEADER".to_string(), format!("Bearer {captured}")), + ]); + let restored = HashMap::from([ + ("VENDOR_PASSWORD".to_string(), real.to_string()), + ("AUTH_HEADER".to_string(), format!("Bearer {real}")), + ]); + let discovered = HashMap::from([ + ("VENDOR_PASSWORD".to_string(), dummy.to_string()), + ("AUTH_HEADER".to_string(), format!("Bearer {dummy}")), + ]); + for replacement in ["", "replacement"] { + let configured = HashMap::from([("AUTH_HEADER".to_string(), replacement.to_string())]); + let mut allowed = discovered.clone(); + allowed.extend(configured.clone()); + let mut snapshot = format!( + "# Snapshot file\n\0\0AUTH_HEADER\0export AUTH_HEADER='Bearer {captured}'\n\0\0" + ); + let prepared = rewrite_snapshot_credentials( + ShellType::Bash, + &mut snapshot, + SnapshotCredentialEnvironment { + original: &original, + restored: &restored, + configured: &configured, + discovered: &discovered, + allowed: &allowed, + is_allowed_unset: &|_| false, + brokered_keys: &keys, + brokered_alias_keys: &[], + allowed_brokered_keys: &keys, + }, + |value| { + *value = value.replace(real, dummy).replace(captured, dummy); + true + }, + ) + .expect("policy override keeps short aliases usable"); + assert_eq!(prepared.aliases, configured); + assert!(!snapshot.contains(real)); + assert!(!snapshot.contains(captured)); + let output = Command::new("/bin/bash") + .args(["-c", &format!("{snapshot}\nprintf '%s' \"$AUTH_HEADER\"")]) + .env_clear() + .output()?; + assert!(output.status.success(), "{output:?}"); + assert_eq!(output.stdout, replacement.as_bytes()); + } + } + Ok(()) +} + +#[test] +fn brokered_snapshot_aliases_follow_captured_dummies() -> Result<()> { + for (real, captured, dummy) in [ + ( + "ghp_real_abcdefghijklmnopqrstuvwxyz012345", + "ghp_old_abcdefghijklmnopqrstuvwxyz0123456", + "ghp_new_abcdefghijklmnopqrstuvwxyz0123456", + ), + ("pin_abcdefgh", "pin_ijklmnop", "pin_ijklmnop"), + ("pin_abcdefgh", "pin_ijklmnop", "pin_qrstuvwx"), + ] { + let original = HashMap::from([ + ("VENDOR_PASSWORD".to_string(), captured.to_string()), + ("AUTH_HEADER".to_string(), format!("Bearer {captured}")), + ("READONLY_HEADER".to_string(), format!("Bearer {captured}")), + ]); + let mut restored = original.clone(); + restored.insert("VENDOR_PASSWORD".to_string(), real.to_string()); + let allowed = HashMap::from([ + ("VENDOR_PASSWORD".to_string(), dummy.to_string()), + ("AUTH_HEADER".to_string(), format!("Bearer {dummy}")), + ("READONLY_HEADER".to_string(), format!("Bearer {dummy}")), + ]); + let keys = vec!["VENDOR_PASSWORD".to_string()]; + let mut snapshot = format!( + "# Snapshot file\n\0\0AUTH_HEADER\0export AUTH_HEADER='Bearer {captured}'\n\0READONLY_HEADER\0declare -rx READONLY_HEADER='Bearer {captured}'\n\0\0" + ); + let rewrite = rewrite_snapshot_credentials( + ShellType::Bash, + &mut snapshot, + SnapshotCredentialEnvironment { + original: &original, + restored: &restored, + configured: &HashMap::new(), + discovered: &allowed, + allowed: &allowed, + is_allowed_unset: &|_| false, + brokered_keys: &keys, + brokered_alias_keys: &[], + allowed_brokered_keys: &keys, + }, + |text| { + *text = text + .replace(&format!("Bearer {real}"), &format!("Bearer {dummy}")) + .replace(&format!("Bearer {captured}"), &format!("Bearer {dummy}")); + true + }, + ) + .expect("captured dummy aliases should be rewritten"); + assert_eq!( + rewrite.aliases, + HashMap::from([ + ("AUTH_HEADER".to_string(), format!("Bearer {dummy}")), + ("READONLY_HEADER".to_string(), format!("Bearer {dummy}")), + ]) + ); + let replay = Command::new("/bin/bash") + .args([ + "-c", + &format!("{snapshot}\nprintf '%s\\n%s' \"$AUTH_HEADER\" \"$READONLY_HEADER\""), + ]) + .env_clear() + .env("VENDOR_PASSWORD", real) + .output()?; + assert!(replay.status.success(), "{replay:?}"); + assert_eq!( + replay.stdout, + format!("Bearer {real}\nBearer {real}").as_bytes() + ); + if std::path::Path::new("/bin/zsh").exists() { + let script = snapshot_capture_script(ShellType::Zsh, CAPTURE_NON_INTERACTIVE).unwrap(); + let capture_output = Command::new("/bin/zsh") + .args(["-fc", &format!("readonly READONLY_HEADER\n{script}")]) + .env_clear() + .envs(&original) + .env("PATH", "/usr/bin:/bin") + .output()?; + assert!(capture_output.status.success(), "{capture_output:?}"); + let mut snapshot = String::from_utf8(capture_output.stdout)?; + let rewritten = rewrite_snapshot_credentials( + ShellType::Zsh, + &mut snapshot, + SnapshotCredentialEnvironment { + original: &original, + restored: &restored, + configured: &HashMap::new(), + discovered: &allowed, + allowed: &allowed, + is_allowed_unset: &|_| false, + brokered_keys: &keys, + brokered_alias_keys: &[], + allowed_brokered_keys: &keys, + }, + |text| { + *text = text.replace(real, dummy).replace(captured, dummy); + true + }, + ) + .expect("omitted readonly exports retain their alias metadata"); + assert_eq!(rewritten.aliases, rewrite.aliases); + let replay = Command::new("/bin/zsh") + .args(["-fc", &format!("readonly READONLY_HEADER\n{snapshot}\n[[ $parameters[READONLY_HEADER] == *readonly* ]] || exit 1\nprintf '%s' \"$READONLY_HEADER\"")]) + .env_clear() + .env("VENDOR_PASSWORD", real) + .env("READONLY_HEADER", format!("Bearer {real}")) + .output()?; + assert!(replay.status.success(), "{replay:?}"); + assert_eq!(replay.stdout, format!("Bearer {real}").as_bytes()); + } + } + Ok(()) +} + #[test] fn brokered_snapshot_rejects_shell_escaped_credentials() { let real = "token_ab'cdefgh";