diff --git a/codex-rs/app-server/src/config_manager_service.rs b/codex-rs/app-server/src/config_manager_service.rs index 809314388f..4b42c28aa9 100644 --- a/codex-rs/app-server/src/config_manager_service.rs +++ b/codex-rs/app-server/src/config_manager_service.rs @@ -616,26 +616,6 @@ fn value_at_path<'a>(root: &'a TomlValue, segments: &[String]) -> Option<&'a Tom Some(current) } -/// Looks up a path according to its config semantics. -/// -/// Shell filter patterns compare case-insensitively and are normalized in the -/// merged effective config, while raw layers preserve their original spelling. -/// Match that path segment case-insensitively so equivalent raw and effective -/// paths compare without changing ordinary TOML path semantics. -fn value_at_semantic_path<'a>(root: &'a TomlValue, segments: &[String]) -> Option<&'a TomlValue> { - let [policy, filters, pattern] = segments else { - return value_at_path(root, segments); - }; - if policy != "shell_environment_policy" || filters != "filters" { - return value_at_path(root, segments); - } - - let filters = value_at_path(root, &segments[..2])?.as_table()?; - filters - .iter() - .find_map(|(candidate, value)| candidate.eq_ignore_ascii_case(pattern).then_some(value)) -} - fn override_message(layer: &ConfigLayerSource) -> String { match layer { ConfigLayerSource::Mdm { domain, key: _ } => { @@ -673,10 +653,10 @@ fn compute_override_metadata( segments: &[String], ) -> Option { let user_value = match layers.get_active_user_layer() { - Some(user_layer) => value_at_semantic_path(&user_layer.config, segments), + Some(user_layer) => value_at_path(&user_layer.config, segments), None => return None, }; - let effective_value = value_at_semantic_path(effective, segments); + let effective_value = value_at_path(effective, segments); if user_value.is_some() && user_value == effective_value { return None; @@ -716,9 +696,7 @@ fn find_effective_layer( segments: &[String], ) -> Option { for layer in layers.layers_high_to_low() { - if let Some(meta) = - value_at_semantic_path(&layer.config, segments).map(|_| layer.metadata()) - { + if let Some(meta) = value_at_path(&layer.config, segments).map(|_| layer.metadata()) { return Some(meta); } } 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 f09da45832..8deae8b065 100644 --- a/codex-rs/app-server/src/config_manager_service_tests.rs +++ b/codex-rs/app-server/src/config_manager_service_tests.rs @@ -841,68 +841,6 @@ async fn write_value_reports_managed_override() { assert_eq!(overridden.effective_value, serde_json::json!("never")); } -#[tokio::test] -async fn write_value_matches_shell_filter_paths_case_insensitively() -> Result<()> { - let tmp = tempdir()?; - let config_path = tmp.path().join(CONFIG_TOML_FILE); - std::fs::write(&config_path, "")?; - - let service = ConfigManager::without_managed_config_for_tests(tmp.path().to_path_buf()); - let result = service - .write_value(ConfigValueWriteParams { - file_path: Some(config_path.display().to_string()), - key_path: "shell_environment_policy.filters.PATH".to_string(), - value: serde_json::json!("include"), - merge_strategy: MergeStrategy::Replace, - expected_version: None, - }) - .await?; - - assert_eq!(result.status, WriteStatus::Ok); - assert_eq!(result.overridden_metadata, None); - assert!(std::fs::read_to_string(config_path)?.contains("PATH = \"include\"")); - Ok(()) -} - -#[tokio::test] -async fn write_value_reports_case_insensitive_shell_filter_override() -> Result<()> { - let tmp = tempdir()?; - let config_path = tmp.path().join(CONFIG_TOML_FILE); - std::fs::write(&config_path, "")?; - - let managed_path = tmp.path().join("managed_config.toml"); - std::fs::write( - &managed_path, - "[shell_environment_policy.filters]\npath = \"exclude\"\n", - )?; - let managed_file = AbsolutePathBuf::try_from(managed_path.clone())?; - let service = ConfigManager::new_for_tests( - tmp.path().to_path_buf(), - vec![], - LoaderOverrides::with_managed_config_path_for_tests(managed_path), - CloudConfigBundleLoader::default(), - ); - - let result = service - .write_value(ConfigValueWriteParams { - file_path: Some(config_path.display().to_string()), - key_path: "shell_environment_policy.filters.PATH".to_string(), - value: serde_json::json!("include"), - merge_strategy: MergeStrategy::Replace, - expected_version: None, - }) - .await?; - - assert_eq!(result.status, WriteStatus::OkOverridden); - let overridden = result.overridden_metadata.expect("overridden metadata"); - assert_eq!( - overridden.overriding_layer.name, - ConfigLayerSource::LegacyManagedConfigTomlFromFile { file: managed_file } - ); - assert_eq!(overridden.effective_value, serde_json::json!("exclude")); - Ok(()) -} - #[tokio::test] async fn upsert_merges_tables_replace_overwrites() -> Result<()> { let tmp = tempdir().expect("tempdir"); diff --git a/codex-rs/config/src/merge.rs b/codex-rs/config/src/merge.rs index 71208d06de..4fdb81c2f1 100644 --- a/codex-rs/config/src/merge.rs +++ b/codex-rs/config/src/merge.rs @@ -17,8 +17,10 @@ fn merge_toml_values_at_path(base: &mut TomlValue, overlay: &TomlValue, path: &m normalize_key_aliases(path, base_table); let mut overlay_table = overlay_table.clone(); normalize_key_aliases(path, &mut overlay_table); - normalize_merge_table_keys(path, base_table); - normalize_merge_table_keys(path, &mut overlay_table); + if is_permission_network_domains_path(path) { + normalize_network_domain_keys(base_table); + normalize_network_domain_keys(&mut overlay_table); + } for (key, value) in overlay_table { path.push(key.clone()); @@ -87,7 +89,6 @@ fn convert_legacy_to_filters(table: &mut toml::map::Map) { None => toml::map::Map::new(), }; table.remove("filters"); - normalize_table_keys(&mut filters, str::to_ascii_lowercase); for (field, action) in [("exclude", "exclude"), ("include_only", "include")] { let Some(TomlValue::Array(patterns)) = table.get(field).cloned() else { continue; @@ -96,7 +97,7 @@ fn convert_legacy_to_filters(table: &mut toml::map::Map) { for pattern in patterns { if let TomlValue::String(pattern) = pattern { filters - .entry(pattern.to_ascii_lowercase()) + .entry(pattern) .or_insert_with(|| TomlValue::String(action.to_string())); } } @@ -107,11 +108,10 @@ fn convert_legacy_to_filters(table: &mut toml::map::Map) { } fn convert_filters_to_legacy(table: &mut toml::map::Map) { - let Some(TomlValue::Table(mut filters)) = table.get("filters").cloned() else { + let Some(TomlValue::Table(filters)) = table.get("filters").cloned() else { return; }; table.remove("filters"); - normalize_table_keys(&mut filters, str::to_ascii_lowercase); for (pattern, action) in filters { match action.as_str() { Some("exclude") => push_legacy_pattern(table, "exclude", "include_only", pattern), @@ -134,10 +134,10 @@ fn push_legacy_pattern( let Some(items) = items.as_array_mut() else { return; }; - if !items.iter().any(|item| { - item.as_str() - .is_some_and(|candidate| candidate.eq_ignore_ascii_case(&pattern)) - }) { + if !items + .iter() + .any(|item| item.as_str().is_some_and(|candidate| candidate == pattern)) + { items.push(TomlValue::String(pattern)); } } @@ -154,38 +154,23 @@ fn remove_patterns_from_legacy_array( return; } items.retain(|item| { - item.as_str().is_none_or(|candidate| { - !patterns - .iter() - .any(|pattern| candidate.eq_ignore_ascii_case(pattern)) - }) + item.as_str() + .is_none_or(|candidate| !patterns.iter().any(|pattern| candidate == *pattern)) }); } -/// Canonicalizes keys for maps whose keys compare independently of TOML's -/// case-sensitive key semantics, so equivalent entries collide before merging. -fn normalize_merge_table_keys(path: &[String], table: &mut toml::map::Map) { - match path { +fn is_permission_network_domains_path(path: &[String]) -> bool { + matches!( + path, [permissions, _, network, domains] - if permissions == "permissions" && network == "network" && domains == "domains" => - { - normalize_table_keys(table, normalize_host); - } - [policy, filters] if policy == "shell_environment_policy" && filters == "filters" => { - // Environment-variable patterns compare case-insensitively. - normalize_table_keys(table, str::to_ascii_lowercase); - } - _ => {} - } + if permissions == "permissions" && network == "network" && domains == "domains" + ) } -fn normalize_table_keys( - table: &mut toml::map::Map, - normalize_key: impl Fn(&str) -> String, -) { +fn normalize_network_domain_keys(table: &mut toml::map::Map) { let entries = std::mem::take(table); - for (key, value) in entries { - table.insert(normalize_key(&key), value); + for (pattern, value) in entries { + table.insert(normalize_host(&pattern), value); } } diff --git a/codex-rs/config/src/merge_tests.rs b/codex-rs/config/src/merge_tests.rs index 1e41512d16..7ccb40c1bc 100644 --- a/codex-rs/config/src/merge_tests.rs +++ b/codex-rs/config/src/merge_tests.rs @@ -146,7 +146,7 @@ exclude = ["HIGH_*"] } #[test] -fn shell_environment_policy_filters_overlay_merges_by_key_case_insensitively() { +fn shell_environment_policy_filters_overlay_merges_by_exact_key() { let mut base = parse_toml( r#" [shell_environment_policy.filters] @@ -169,9 +169,10 @@ fn shell_environment_policy_filters_overlay_merges_by_key_case_insensitively() { parse_toml( r#" [shell_environment_policy.filters] -"add_*" = "exclude" +"ADD_*" = "exclude" +"FLIP_*" = "exclude" +"KEEP_*" = "include" "flip_*" = "include" -"keep_*" = "include" "#, ) ); @@ -202,11 +203,11 @@ include_only = ["FLIP_TO_EXCLUDE", "KEEP_INCLUDED"] parse_toml( r#" [shell_environment_policy.filters] -"add_included" = "include" -"flip_to_exclude" = "exclude" -"flip_to_include" = "include" -"keep_excluded" = "exclude" -"keep_included" = "include" +"ADD_INCLUDED" = "include" +"FLIP_TO_EXCLUDE" = "exclude" +"FLIP_TO_INCLUDE" = "include" +"KEEP_EXCLUDED" = "exclude" +"KEEP_INCLUDED" = "include" "#, ) ); @@ -237,7 +238,7 @@ exclude = ["FLIP_TO_EXCLUDE", "HIGH_EXCLUDED"] r#" [shell_environment_policy] exclude = ["FLIP_TO_EXCLUDE", "HIGH_EXCLUDED"] -include_only = ["keep_included"] +include_only = ["KEEP_INCLUDED"] "#, ) ); diff --git a/codex-rs/config/src/types.rs b/codex-rs/config/src/types.rs index 5f32670ab8..81908de6f2 100644 --- a/codex-rs/config/src/types.rs +++ b/codex-rs/config/src/types.rs @@ -949,6 +949,8 @@ pub struct ShellEnvironmentPolicyToml { /// Ordinary config keeps accepting the legacy arrays above during the /// migration. Requirements will accept only this keyed form, keeping array /// compatibility isolated so the legacy fields can be deprecated later. + /// Pattern keys merge case-sensitively across config layers even though the + /// resulting patterns match environment variable names case-insensitively. pub filters: Option>, pub experimental_use_profile: Option, diff --git a/codex-rs/core/src/config/config_loader_tests.rs b/codex-rs/core/src/config/config_loader_tests.rs index 9b514f8d9e..8fe1275221 100644 --- a/codex-rs/core/src/config/config_loader_tests.rs +++ b/codex-rs/core/src/config/config_loader_tests.rs @@ -445,12 +445,11 @@ future_field = true } #[tokio::test] -async fn non_strict_config_rejects_duplicate_shell_environment_policy_filters_before_merging() { +async fn non_strict_config_rejects_shell_filter_case_variants_across_layers() { let tmp = tempdir().expect("tempdir"); let contents = r#" [shell_environment_policy.filters] "SECRET_TOKEN" = "exclude" -"secret_token" = "include" "#; std::fs::write(tmp.path().join(CONFIG_TOML_FILE), contents).expect("write config"); @@ -459,13 +458,13 @@ async fn non_strict_config_rejects_duplicate_shell_environment_policy_filters_be .fallback_cwd(Some(tmp.path().to_path_buf())) .loader_overrides(LoaderOverrides::without_managed_config_for_tests()) .cli_overrides(vec![( - "shell_environment_policy.filters.PATH".to_string(), + "shell_environment_policy.filters.secret_token".to_string(), TomlValue::String("include".to_string()), )]) .strict_config(/*strict_config*/ false) .build() .await - .expect_err("case-insensitive duplicate filters should be rejected per layer"); + .expect_err("case variants across layers should be rejected after merging"); assert!( err.to_string()