diff --git a/codex-rs/config/src/schema.rs b/codex-rs/config/src/schema.rs index c641f1703d..fec33cbeb3 100644 --- a/codex-rs/config/src/schema.rs +++ b/codex-rs/config/src/schema.rs @@ -117,12 +117,43 @@ pub fn mcp_servers_schema(schema_gen: &mut SchemaGenerator) -> Schema { /// Build the config schema for `config.toml`. pub fn config_schema() -> RootSchema { - SchemaSettings::draft07() + let mut schema = SchemaSettings::draft07() .with(|settings| { settings.option_add_null_type = false; }) .into_generator() - .into_root_schema_for::() + .into_root_schema_for::(); + add_shell_environment_policy_constraints(&mut schema); + schema +} + +// Mirror the custom deserializer's mixed-form rejection in the generated schema. +fn add_shell_environment_policy_constraints(schema: &mut RootSchema) { + let Some(Schema::Object(policy)) = schema.definitions.get_mut("ShellEnvironmentPolicyToml") + else { + return; + }; + let all_of = policy + .subschemas + .get_or_insert_default() + .all_of + .get_or_insert_default(); + for fields in [["exclude", "filters"], ["filters", "include_only"]] { + let forbidden_fields = Schema::Object(SchemaObject { + object: Some(Box::new(ObjectValidation { + required: fields.into_iter().map(str::to_string).collect(), + ..Default::default() + })), + ..Default::default() + }); + all_of.push(Schema::Object(SchemaObject { + subschemas: Some(Box::new(SubschemaValidation { + not: Some(Box::new(forbidden_fields)), + ..Default::default() + })), + ..Default::default() + })); + } } /// Canonicalize a JSON value by sorting its keys. diff --git a/codex-rs/config/src/state.rs b/codex-rs/config/src/state.rs index 463ab23e60..f79b0f0072 100644 --- a/codex-rs/config/src/state.rs +++ b/codex-rs/config/src/state.rs @@ -9,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; @@ -277,23 +278,20 @@ impl ConfigLayerStack { requirements: ConfigRequirements, requirements_toml: ConfigRequirementsToml, ) -> std::io::Result { - if let Some(layer) = layers.iter().find(|layer| { - layer - .config - .get("shell_environment_policy") - .and_then(TomlValue::as_table) - .is_some_and(|policy| { - policy.contains_key("filters") - && (policy.contains_key("exclude") || policy.contains_key("include_only")) - }) - }) { - return Err(std::io::Error::new( - std::io::ErrorKind::InvalidData, - format!( - "cannot mix `filters` with legacy `exclude` or `include_only` in {}", - format_config_layer_source(&layer.name, CONFIG_TOML_FILE) - ), - )); + // 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 { diff --git a/codex-rs/config/src/state_tests.rs b/codex-rs/config/src/state_tests.rs index fb6e26968b..bebe68e34f 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 69fd80816e..5f32670ab8 100644 --- a/codex-rs/config/src/types.rs +++ b/codex-rs/config/src/types.rs @@ -955,7 +955,6 @@ pub struct ShellEnvironmentPolicyToml { } #[derive(Deserialize)] -#[serde(deny_unknown_fields)] struct ShellEnvironmentPolicyTomlRaw { inherit: Option, ignore_default_excludes: Option, @@ -977,6 +976,16 @@ impl<'de> Deserialize<'de> for ShellEnvironmentPolicyToml { "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, diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index 0bbcbea131..cdd6eb2271 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -2630,6 +2630,24 @@ }, "ShellEnvironmentPolicyToml": { "additionalProperties": false, + "allOf": [ + { + "not": { + "required": [ + "exclude", + "filters" + ] + } + }, + { + "not": { + "required": [ + "filters", + "include_only" + ] + } + } + ], "description": "Policy for building the `env` when spawning a process via shell-like tools.", "properties": { "exclude": { diff --git a/codex-rs/core/src/config/config_loader_tests.rs b/codex-rs/core/src/config/config_loader_tests.rs index 0096919eaf..cdef5c42ee 100644 --- a/codex-rs/core/src/config/config_loader_tests.rs +++ b/codex-rs/core/src/config/config_loader_tests.rs @@ -410,6 +410,110 @@ exclude = ["LEGACY_*"] ); } +#[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_duplicate_shell_environment_policy_filters_before_merging() { + 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"); + + 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.PATH".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"); + + 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_*" = "exlude" +"#, + 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/core/src/config/schema_tests.rs b/codex-rs/core/src/config/schema_tests.rs index dd67ead898..479f7f318c 100644 --- a/codex-rs/core/src/config/schema_tests.rs +++ b/codex-rs/core/src/config/schema_tests.rs @@ -73,3 +73,32 @@ fn config_schema_hides_unsupported_inline_mcp_bearer_token() { (false, true), ); } + +#[test] +fn shell_environment_policy_schema_rejects_mixed_filter_representations() { + let schema_json = config_schema_json().expect("serialize config schema"); + let schema_value: serde_json::Value = + serde_json::from_slice(&schema_json).expect("decode schema json"); + let constraints = schema_value + .pointer("/definitions/ShellEnvironmentPolicyToml/allOf") + .expect("shell environment policy constraints should exist") + .as_array() + .expect("shell environment policy constraints should be an array"); + let required_pairs = constraints + .iter() + .map(|constraint| { + constraint + .pointer("/not/required") + .expect("constraint should prohibit a required-field pair") + .clone() + }) + .collect::>(); + + assert_eq!( + required_pairs, + vec![ + serde_json::json!(["exclude", "filters"]), + serde_json::json!(["filters", "include_only"]), + ] + ); +}