diff --git a/codex-rs/config/src/merge.rs b/codex-rs/config/src/merge.rs index 94d670154663..a3152f1f8880 100644 --- a/codex-rs/config/src/merge.rs +++ b/codex-rs/config/src/merge.rs @@ -9,6 +9,8 @@ pub fn merge_toml_values(base: &mut TomlValue, overlay: &TomlValue) { } fn merge_toml_values_at_path(base: &mut TomlValue, overlay: &TomlValue, path: &mut Vec) { + replace_shell_environment_policy_filter_representation(base, overlay, path); + if let TomlValue::Table(overlay_table) = overlay && let TomlValue::Table(base_table) = base { @@ -34,6 +36,31 @@ fn merge_toml_values_at_path(base: &mut TomlValue, overlay: &TomlValue, path: &m } } +/// Switching between legacy arrays and keyed filters replaces lower filter +/// fields instead of attempting to reconcile the two representations. +fn replace_shell_environment_policy_filter_representation( + base: &mut TomlValue, + overlay: &TomlValue, + path: &[String], +) { + if !matches!(path, [policy] if policy == "shell_environment_policy") { + return; + } + let TomlValue::Table(base) = base else { + return; + }; + let TomlValue::Table(overlay) = overlay else { + return; + }; + + if overlay.contains_key("filters") { + base.remove("exclude"); + base.remove("include_only"); + } else if overlay.contains_key("exclude") || overlay.contains_key("include_only") { + base.remove("filters"); + } +} + fn is_permission_network_domains_path(path: &[String]) -> bool { matches!( path, diff --git a/codex-rs/config/src/merge_tests.rs b/codex-rs/config/src/merge_tests.rs index f9da6e7ebc14..500222d879a9 100644 --- a/codex-rs/config/src/merge_tests.rs +++ b/codex-rs/config/src/merge_tests.rs @@ -124,3 +124,127 @@ fn merge_toml_values_normalizes_permission_network_domains_before_overlaying() { ); assert_eq!(base, expected); } + +#[test] +fn shell_environment_policy_legacy_array_overlay_replaces_legacy_array() { + let mut base = parse_toml( + r#" +[shell_environment_policy] +exclude = ["LOW_*", "SHARED_*"] +"#, + ); + let overlay = parse_toml( + r#" +[shell_environment_policy] +exclude = ["HIGH_*"] +"#, + ); + + merge_toml_values(&mut base, &overlay); + + assert_eq!(base, overlay); +} + +#[test] +fn shell_environment_policy_filters_overlay_merges_by_exact_key() { + let mut base = parse_toml( + r#" +[shell_environment_policy.filters] +"FLIP_*" = "exclude" +"KEEP_*" = "include" +"#, + ); + let overlay = parse_toml( + r#" +[shell_environment_policy.filters] +"ADD_*" = "exclude" +"flip_*" = "include" +"#, + ); + + merge_toml_values(&mut base, &overlay); + + assert_eq!( + base, + parse_toml( + r#" +[shell_environment_policy.filters] +"ADD_*" = "exclude" +"FLIP_*" = "exclude" +"KEEP_*" = "include" +"flip_*" = "include" +"#, + ) + ); +} + +#[test] +fn shell_environment_policy_filters_replace_lower_legacy_filter_fields() { + let mut base = parse_toml( + r#" +[shell_environment_policy] +inherit = "core" +exclude = ["FLIP_TO_INCLUDE", "KEEP_EXCLUDED"] +include_only = ["FLIP_TO_EXCLUDE", "KEEP_INCLUDED"] +"#, + ); + let overlay = parse_toml( + r#" +[shell_environment_policy.filters] +"ADD_INCLUDED" = "include" +"FLIP_TO_EXCLUDE" = "exclude" +"FLIP_TO_INCLUDE" = "include" +"#, + ); + + merge_toml_values(&mut base, &overlay); + + assert_eq!( + base, + parse_toml( + r#" +[shell_environment_policy] +inherit = "core" + +[shell_environment_policy.filters] +"ADD_INCLUDED" = "include" +"FLIP_TO_EXCLUDE" = "exclude" +"FLIP_TO_INCLUDE" = "include" +"#, + ) + ); +} + +#[test] +fn shell_environment_policy_legacy_arrays_replace_lower_filters() { + let mut base = parse_toml( + r#" +[shell_environment_policy] +inherit = "core" + +[shell_environment_policy.filters] +"FLIP_TO_EXCLUDE" = "include" +"LOW_EXCLUDED" = "exclude" +"KEEP_INCLUDED" = "include" +"#, + ); + let overlay = parse_toml( + r#" +[shell_environment_policy] +exclude = ["FLIP_TO_EXCLUDE", "HIGH_EXCLUDED"] +"#, + ); + + merge_toml_values(&mut base, &overlay); + + assert_eq!( + base, + parse_toml( + r#" +[shell_environment_policy] +inherit = "core" +exclude = ["FLIP_TO_EXCLUDE", "HIGH_EXCLUDED"] +"#, + ) + ); +} diff --git a/codex-rs/config/src/state.rs b/codex-rs/config/src/state.rs index 9d8de1c8852c..f79b0f00726e 100644 --- a/codex-rs/config/src/state.rs +++ b/codex-rs/config/src/state.rs @@ -1,5 +1,7 @@ +use crate::CONFIG_TOML_FILE; use crate::config_requirements::ConfigRequirements; use crate::config_requirements::ConfigRequirementsToml; +use crate::format_config_layer_source; use super::fingerprint::record_origins; use super::fingerprint::version_for_toml; @@ -7,6 +9,7 @@ use super::key_aliases::normalized_with_key_aliases; use super::merge::merge_toml_values; use crate::CloudConfigBundleLoader; use crate::ProfileV2Name; +use crate::types::ShellEnvironmentPolicyToml; use codex_app_server_protocol::ConfigLayer; use codex_app_server_protocol::ConfigLayerMetadata; use codex_app_server_protocol::ConfigLayerSource; @@ -275,6 +278,21 @@ impl ConfigLayerStack { requirements: ConfigRequirements, requirements_toml: ConfigRequirementsToml, ) -> std::io::Result { + // Validate enabled layers before merging to reject mixed forms and malformed entries + for layer in layers.iter().filter(|layer| !layer.is_disabled()) { + let Some(policy) = layer.config.get("shell_environment_policy") else { + continue; + }; + let _: ShellEnvironmentPolicyToml = policy.clone().try_into().map_err(|error| { + std::io::Error::new( + std::io::ErrorKind::InvalidData, + format!( + "invalid shell environment policy in {}: {error}", + format_config_layer_source(&layer.name, CONFIG_TOML_FILE) + ), + ) + })?; + } let user_layer_index = verify_layer_ordering(&layers)?; Ok(Self { layers, diff --git a/codex-rs/config/src/state_tests.rs b/codex-rs/config/src/state_tests.rs index fb6e26968b1a..bebe68e34fdd 100644 --- a/codex-rs/config/src/state_tests.rs +++ b/codex-rs/config/src/state_tests.rs @@ -39,6 +39,34 @@ no_memories_if_mcp_or_web_search = true ); } +#[test] +fn disabled_layers_do_not_validate_shell_environment_policy() { + let layer = ConfigLayerEntry::new_disabled( + ConfigLayerSource::Project { + dot_codex_folder: AbsolutePathBuf::from_absolute_path("/untrusted/.codex") + .expect("project path should be absolute"), + }, + toml::from_str( + r#" +[shell_environment_policy] +exclude = ["LEGACY_*"] + +[shell_environment_policy.filters] +"CANONICAL_*" = "include" +"#, + ) + .expect("project config"), + "project is untrusted", + ); + + ConfigLayerStack::new( + vec![layer], + ConfigRequirements::default(), + ConfigRequirementsToml::default(), + ) + .expect("disabled layers should not be validated"); +} + #[test] fn active_user_layer_is_highest_precedence_user_layer() { let temp_dir = TempDir::new().expect("tempdir"); diff --git a/codex-rs/config/src/types.rs b/codex-rs/config/src/types.rs index e459f9585b54..51ca74faf0cf 100644 --- a/codex-rs/config/src/types.rs +++ b/codex-rs/config/src/types.rs @@ -18,6 +18,7 @@ pub use codex_protocol::config_types::ModeKind; pub use codex_protocol::config_types::Personality; pub use codex_protocol::config_types::ServiceTier; use codex_protocol::config_types::ShellEnvironmentPolicy; +use codex_protocol::config_types::ShellEnvironmentPolicyFilter; use codex_protocol::config_types::ShellEnvironmentPolicyInherit; pub use codex_protocol::config_types::WebSearchMode; use codex_utils_absolute_path::AbsolutePathBuf; @@ -932,39 +933,98 @@ impl From for codex_app_server_protocol::SandboxSettings } /// Policy for building the `env` when spawning a process via shell-like tools. -#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Default, JsonSchema)] +#[derive(Serialize, Debug, Clone, PartialEq, Default, JsonSchema)] #[schemars(deny_unknown_fields)] pub struct ShellEnvironmentPolicyToml { pub inherit: Option, pub ignore_default_excludes: Option, - /// List of regular expressions. + /// Legacy list of regular expressions to exclude. pub exclude: Option>, pub r#set: Option>, - /// List of regular expressions. + /// Legacy list of regular expressions to include. pub include_only: Option>, + /// Pattern actions used by the canonical table representation. + /// + /// 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, } +#[derive(Deserialize)] +struct ShellEnvironmentPolicyTomlRaw { + inherit: Option, + ignore_default_excludes: Option, + exclude: Option>, + r#set: Option>, + include_only: Option>, + filters: Option>, + experimental_use_profile: Option, +} + +impl<'de> Deserialize<'de> for ShellEnvironmentPolicyToml { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + let raw = ShellEnvironmentPolicyTomlRaw::deserialize(deserializer)?; + if raw.filters.is_some() && (raw.exclude.is_some() || raw.include_only.is_some()) { + return Err(serde::de::Error::custom( + "cannot mix `filters` with legacy `exclude` or `include_only`", + )); + } + if let Some(filters) = raw.filters.as_ref() { + let mut patterns = std::collections::HashSet::new(); + for pattern in filters.keys() { + if !patterns.insert(pattern.to_ascii_lowercase()) { + return Err(serde::de::Error::custom(format!( + "duplicate shell environment filter `{pattern}` ignoring ASCII case" + ))); + } + } + } + Ok(Self { + inherit: raw.inherit, + ignore_default_excludes: raw.ignore_default_excludes, + exclude: raw.exclude, + r#set: raw.r#set, + include_only: raw.include_only, + filters: raw.filters, + experimental_use_profile: raw.experimental_use_profile, + }) + } +} + impl From for ShellEnvironmentPolicy { fn from(toml: ShellEnvironmentPolicyToml) -> Self { // Default to inheriting the full environment when not specified. let inherit = toml.inherit.unwrap_or(ShellEnvironmentPolicyInherit::All); let ignore_default_excludes = toml.ignore_default_excludes.unwrap_or(true); - let exclude = toml - .exclude - .unwrap_or_default() + let mut exclude = toml.exclude.unwrap_or_default(); + let mut include_only = toml.include_only.unwrap_or_default(); + for (pattern, filter) in toml.filters.unwrap_or_default() { + exclude.retain(|candidate| candidate != &pattern); + include_only.retain(|candidate| candidate != &pattern); + match filter { + ShellEnvironmentPolicyFilter::Include => include_only.push(pattern), + ShellEnvironmentPolicyFilter::Exclude => exclude.push(pattern), + } + } + let exclude = exclude .into_iter() .map(|s| EnvironmentVariablePattern::new_case_insensitive(&s)) .collect(); let r#set = toml.r#set.unwrap_or_default(); - let include_only = toml - .include_only - .unwrap_or_default() + let include_only = include_only .into_iter() .map(|s| EnvironmentVariablePattern::new_case_insensitive(&s)) .collect(); diff --git a/codex-rs/config/src/types_tests.rs b/codex-rs/config/src/types_tests.rs index 2c3f69d9867e..2bffcdc4427c 100644 --- a/codex-rs/config/src/types_tests.rs +++ b/codex-rs/config/src/types_tests.rs @@ -1,6 +1,77 @@ use super::*; use pretty_assertions::assert_eq; +#[test] +fn shell_environment_policy_accepts_legacy_lists_or_filters() { + let legacy: ShellEnvironmentPolicyToml = toml::from_str( + r#" +exclude = ["LEGACY_*", "SHARED_*"] +include_only = ["PATH", "HOME"] +"#, + ) + .expect("legacy arrays should remain valid in config.toml"); + assert_eq!( + legacy, + ShellEnvironmentPolicyToml { + exclude: Some(vec!["LEGACY_*".to_string(), "SHARED_*".to_string()]), + include_only: Some(vec!["PATH".to_string(), "HOME".to_string()]), + ..Default::default() + } + ); + + let filtered: ShellEnvironmentPolicyToml = toml::from_str( + r#" +[filters] +"FLIP_TO_EXCLUDE" = "exclude" +"FLIP_TO_INCLUDE" = "include" +"#, + ) + .expect("filters should be valid in config.toml"); + assert_eq!( + filtered, + ShellEnvironmentPolicyToml { + filters: Some(BTreeMap::from([ + ( + "FLIP_TO_EXCLUDE".to_string(), + ShellEnvironmentPolicyFilter::Exclude, + ), + ( + "FLIP_TO_INCLUDE".to_string(), + ShellEnvironmentPolicyFilter::Include, + ), + ])), + ..Default::default() + } + ); + assert_eq!( + ShellEnvironmentPolicy::from(filtered), + ShellEnvironmentPolicy::from(ShellEnvironmentPolicyToml { + exclude: Some(vec!["FLIP_TO_EXCLUDE".to_string()]), + include_only: Some(vec!["FLIP_TO_INCLUDE".to_string()]), + ..Default::default() + }) + ); +} + +#[test] +fn shell_environment_policy_rejects_mixed_legacy_lists_and_filters() { + let error = toml::from_str::( + r#" +exclude = ["LEGACY_*"] + +[filters] +"CANONICAL_*" = "include" +"#, + ) + .expect_err("one config layer must not mix legacy lists and filters"); + + assert!( + error + .to_string() + .contains("cannot mix `filters` with legacy `exclude` or `include_only`") + ); +} + #[test] fn deserialize_skill_config_with_name_selector() { let cfg: SkillConfig = toml::from_str( diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index 9944c2994cd2..38baf8e36d94 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -2606,6 +2606,14 @@ ], "type": "string" }, + "ShellEnvironmentPolicyFilter": { + "description": "Assigns a shell environment variable pattern to the include-only or exclude set. Includes do not re-add variables removed by another exclude pattern.", + "enum": [ + "include", + "exclude" + ], + "type": "string" + }, "ShellEnvironmentPolicyInherit": { "oneOf": [ { @@ -2636,7 +2644,7 @@ "description": "Policy for building the `env` when spawning a process via shell-like tools.", "properties": { "exclude": { - "description": "List of regular expressions.", + "description": "Legacy list of regular expressions to exclude.", "items": { "type": "string" }, @@ -2645,11 +2653,18 @@ "experimental_use_profile": { "type": "boolean" }, + "filters": { + "additionalProperties": { + "$ref": "#/definitions/ShellEnvironmentPolicyFilter" + }, + "description": "Pattern actions used by the canonical table representation.\n\nOrdinary 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.", + "type": "object" + }, "ignore_default_excludes": { "type": "boolean" }, "include_only": { - "description": "List of regular expressions.", + "description": "Legacy list of regular expressions to include.", "items": { "type": "string" }, @@ -5214,6 +5229,7 @@ "default": { "exclude": null, "experimental_use_profile": null, + "filters": null, "ignore_default_excludes": null, "include_only": null, "inherit": null, diff --git a/codex-rs/core/src/config/config_loader_tests.rs b/codex-rs/core/src/config/config_loader_tests.rs index c01c555519ad..1033b6e31e5a 100644 --- a/codex-rs/core/src/config/config_loader_tests.rs +++ b/codex-rs/core/src/config/config_loader_tests.rs @@ -379,6 +379,140 @@ unknown_key = true"#; assert_eq!(config_error, &expected_config_error); } +#[tokio::test] +async fn non_strict_config_rejects_mixed_shell_environment_policy_before_higher_layer_merge() { + let tmp = tempdir().expect("tempdir"); + let contents = r#" +[shell_environment_policy] +exclude = ["LEGACY_*"] + +[shell_environment_policy.filters] +"CANONICAL_*" = "include" +"#; + std::fs::write(tmp.path().join(CONFIG_TOML_FILE), contents).expect("write config"); + + let err = ConfigBuilder::default() + .codex_home(tmp.path().to_path_buf()) + .fallback_cwd(Some(tmp.path().to_path_buf())) + .loader_overrides(LoaderOverrides::without_managed_config_for_tests()) + .cli_overrides(vec![( + "shell_environment_policy.filters.HIGHER_*".to_string(), + TomlValue::String("exclude".to_string()), + )]) + .strict_config(/*strict_config*/ false) + .build() + .await + .expect_err("one config layer must not mix legacy lists and filters"); + + assert!( + err.to_string() + .contains("cannot mix `filters` with legacy `exclude` or `include_only`") + ); +} + +#[tokio::test] +async fn shell_environment_policy_unknown_fields_follow_strict_config() { + let tmp = tempdir().expect("tempdir"); + let contents = r#" +[shell_environment_policy] +future_field = true +"#; + let config_path = tmp.path().join(CONFIG_TOML_FILE); + std::fs::write(&config_path, contents).expect("write config"); + + ConfigBuilder::default() + .codex_home(tmp.path().to_path_buf()) + .fallback_cwd(Some(tmp.path().to_path_buf())) + .loader_overrides(LoaderOverrides::without_managed_config_for_tests()) + .strict_config(/*strict_config*/ false) + .build() + .await + .expect("non-strict config should ignore unknown fields"); + + let err = ConfigBuilder::default() + .codex_home(tmp.path().to_path_buf()) + .fallback_cwd(Some(tmp.path().to_path_buf())) + .loader_overrides(LoaderOverrides::without_managed_config_for_tests()) + .strict_config(/*strict_config*/ true) + .build() + .await + .expect_err("strict config should reject unknown fields"); + + assert_eq!( + config_error_from_io(&err).message, + "unknown configuration field `shell_environment_policy.future_field`" + ); +} + +#[tokio::test] +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" +"#; + std::fs::write(tmp.path().join(CONFIG_TOML_FILE), contents).expect("write config"); + + let err = ConfigBuilder::default() + .codex_home(tmp.path().to_path_buf()) + .fallback_cwd(Some(tmp.path().to_path_buf())) + .loader_overrides(LoaderOverrides::without_managed_config_for_tests()) + .cli_overrides(vec![( + "shell_environment_policy.filters.secret_token".to_string(), + TomlValue::String("include".to_string()), + )]) + .strict_config(/*strict_config*/ false) + .build() + .await + .expect_err("case variants across layers should be rejected after merging"); + + assert!( + err.to_string() + .contains("duplicate shell environment filter") + ); +} + +#[tokio::test] +async fn non_strict_config_rejects_malformed_shell_policy_before_representation_conversion() { + let cases = [ + ( + r#" +[shell_environment_policy] +exclude = ["SECRET_*", 17] +"#, + vec![( + "shell_environment_policy.filters.PATH".to_string(), + TomlValue::String("include".to_string()), + )], + ), + ( + r#" +[shell_environment_policy.filters] +"SECRET_*" = "keep" +"#, + vec![( + "shell_environment_policy.exclude".to_string(), + TomlValue::Array(vec![TomlValue::String("PATH".to_string())]), + )], + ), + ]; + + for (contents, cli_overrides) in cases { + let tmp = tempdir().expect("tempdir"); + std::fs::write(tmp.path().join(CONFIG_TOML_FILE), contents).expect("write config"); + + ConfigBuilder::default() + .codex_home(tmp.path().to_path_buf()) + .fallback_cwd(Some(tmp.path().to_path_buf())) + .loader_overrides(LoaderOverrides::without_managed_config_for_tests()) + .cli_overrides(cli_overrides) + .strict_config(/*strict_config*/ false) + .build() + .await + .expect_err("malformed shell policy should be rejected before conversion"); + } +} + #[tokio::test] async fn strict_config_rejects_unknown_cli_override_key() { let tmp = tempdir().expect("tempdir"); diff --git a/codex-rs/protocol/src/config_types.rs b/codex-rs/protocol/src/config_types.rs index 713945dbde74..4e54301d91ca 100644 --- a/codex-rs/protocol/src/config_types.rs +++ b/codex-rs/protocol/src/config_types.rs @@ -199,6 +199,16 @@ pub enum ShellEnvironmentPolicyInherit { None, } +/// Assigns a shell environment variable pattern to the include-only or exclude +/// set. Includes do not re-add variables removed by another exclude pattern. +#[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "lowercase")] +#[ts(export_to = "v2/")] +pub enum ShellEnvironmentPolicyFilter { + Include, + Exclude, +} + pub type EnvironmentVariablePattern = WildMatchPattern<'*', '?'>; /// Deriving the `env` based on this policy works as follows: