diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index b595f77f68..6876405c7d 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -379,10 +379,11 @@ alongside `[permissions.filesystem]`: enabled = true ``` -`[network_proxy]` and its compatibility alias `[experimental_network]` still work. -Do not mix either with `[permissions.network]`, even across requirements layers. A single -layer cannot use both top-level names; across layers the top-level names merge with normal -precedence. `configRequirements/read` returns the result in `requirements.network`. +Codex reads `[network_proxy]` and its alias `[experimental_network]` as +`[permissions.network]` within each source before merging managed requirements. +Across sources, use the same surface (legacy or nested). Use at most one top-level spelling per layer. +If a layer also sets `[permissions.network]`, both tables must describe the same policy. +`configRequirements/read` returns the result in `requirements.network`. In `config.toml`, `[features.network_proxy]` configures the proxy, while `[permissions..network]` configures a profile's network settings. Managed requirements diff --git a/codex-rs/cloud-config/src/validation.rs b/codex-rs/cloud-config/src/validation.rs index 4718658ac4..e2ee6019a3 100644 --- a/codex-rs/cloud-config/src/validation.rs +++ b/codex-rs/cloud-config/src/validation.rs @@ -23,13 +23,7 @@ pub(crate) fn validate_bundle( enterprise_managed_requirements, } = bundle_layers; - let requirements = compose_requirements(enterprise_managed_requirements).map_err(|_| { - CloudConfigBundleLoadError::new( - CloudConfigBundleLoadErrorCode::InvalidBundle, - /*status_code*/ None, - "invalid cloud config bundle: invalid requirements", - ) - })?; + let requirements = compose_requirements(enterprise_managed_requirements)?; if let Some(providers) = requirements.and_then(|requirements| requirements.model_providers) { validate_model_providers(&providers).map_err(|_| { CloudConfigBundleLoadError::new( diff --git a/codex-rs/cloud-config/src/validation_tests.rs b/codex-rs/cloud-config/src/validation_tests.rs index ad5503f9ea..565f74a087 100644 --- a/codex-rs/cloud-config/src/validation_tests.rs +++ b/codex-rs/cloud-config/src/validation_tests.rs @@ -4,28 +4,56 @@ use pretty_assertions::assert_eq; use tempfile::tempdir; #[test] -fn cloud_validation_rejects_mixed_network_requirements() { +fn cloud_validation_checks_network_spellings_per_fragment() { let home = tempdir().expect("tempdir"); let base_dir = AbsolutePathBuf::from_absolute_path(home.path()).expect("absolute path"); let mut bundle = CloudConfigBundle::default(); for alias in ["experimental_network", "network_proxy"] { let nested = "[permissions.network]\nenabled = false"; let top_level = format!("[{alias}]\nenabled = false"); - for contents in [ - vec![format!("{nested}\n{top_level}")], - vec![nested.to_owned(), top_level], + let conflicting = format!("[{alias}]\nenabled = true"); + for (contents, expected) in [ + (vec![format!("{nested}\n{top_level}")], Ok(())), + ( + vec![nested.to_owned(), conflicting.clone()], + Err(CloudConfigBundleLoadErrorCode::InvalidBundle), + ), + ( + vec![conflicting.clone(), nested.to_owned()], + Err(CloudConfigBundleLoadErrorCode::InvalidBundle), + ), + ( + vec![ + "[permissions.network]\ndenied_domains = ['shared.test']".to_string(), + format!("[{alias}]\nallowed_domains = ['shared.test']"), + ], + Err(CloudConfigBundleLoadErrorCode::InvalidBundle), + ), + ( + vec![format!( + "{nested}\n{conflicting}\nreview_domains = ['private-policy.example']" + )], + Err(CloudConfigBundleLoadErrorCode::InvalidBundle), + ), ] { bundle.requirements_toml.enterprise_managed = contents .into_iter() .enumerate() .map(|(index, contents)| CloudRequirementsFragment { - id: index.to_string(), + id: format!("fragment-{index}"), name: index.to_string(), contents, }) .collect(); - let err = validate_bundle(&bundle, &base_dir).expect_err("mixed policy spellings"); - assert_eq!(err.code(), CloudConfigBundleLoadErrorCode::InvalidBundle); + let result = validate_bundle(&bundle, &base_dir); + if let Err(error) = &result { + let message = error.to_string(); + for fragment in &bundle.requirements_toml.enterprise_managed { + assert!(message.contains(&fragment.id), "{message}"); + } + assert!(!message.contains("private-policy.example"), "{message}"); + } + assert_eq!(result.map_err(|error| error.code()), expected); } } } diff --git a/codex-rs/config/src/config_requirements.rs b/codex-rs/config/src/config_requirements.rs index ff8f5bd638..a5819448bd 100644 --- a/codex-rs/config/src/config_requirements.rs +++ b/codex-rs/config/src/config_requirements.rs @@ -390,6 +390,18 @@ pub enum NetworkDomainPermissionToml { Deny, } +impl NetworkDomainPermissionToml { + /// Unsupported patterns retain their exact spelling. They can never compare equal + /// to a valid selector and disappear when two published declarations are combined. + pub(crate) fn pattern_identity(self, pattern: &str) -> Result { + match self { + Self::Allow => codex_network_proxy::canonical_allow_pattern(pattern), + Self::Deny => codex_network_proxy::canonical_deny_pattern(pattern), + } + .map_err(|_| pattern.to_owned()) + } +} + impl std::fmt::Display for NetworkDomainPermissionToml { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { let permission = match self { @@ -456,6 +468,45 @@ pub struct NetworkRequirementsToml { pub header_injections: Option>, } +impl NetworkRequirementsToml { + /// Compare the selector identities used by the proxy without modifying the published + /// representation. Keep allow and deny contributions distinct, including shadowed grants. + pub(crate) fn describes_same_policy(&self, other: &Self) -> bool { + if self == other { + return true; + } + let domain_rules = |policy: &Self| { + policy.domains.as_ref().map(|domains| { + domains + .entries + .iter() + .map(|(pattern, &rule)| (rule, rule.pattern_identity(pattern))) + .collect::>() + }) + }; + let review_patterns = |policy: &Self| { + policy.review_domains.as_ref().map(|patterns| { + patterns + .iter() + .map(|pattern| { + codex_network_proxy::canonical_review_pattern(pattern) + .map_err(|_| pattern.to_owned()) + }) + .collect::>() + }) + }; + if domain_rules(self) != domain_rules(other) + || review_patterns(self) != review_patterns(other) + { + return false; + } + let mut comparable = other.clone(); + comparable.domains.clone_from(&self.domains); + comparable.review_domains.clone_from(&self.review_domains); + self == &comparable + } +} + #[derive(Serialize, Deserialize, Clone, PartialEq, Eq)] pub struct NetworkHeaderInjectionToml { pub host: String, @@ -1344,15 +1395,16 @@ pub struct ConfigRequirementsWithSources { impl ConfigRequirementsWithSources { pub(crate) fn validate_network_spelling(&self) -> Result<(), ConstraintError> { - if let (Some(nested), Some(top_level)) = (&self.permissions_network, &self.network) { + if let (Some(nested), Some(top_level)) = (&self.permissions_network, &self.network) + && !nested.value.describes_same_policy(&top_level.value) + { return Err(ConstraintError::InvalidValue { field_name: "permissions.network", candidate: format!( "configured together with top-level network requirements from {}", top_level.source ), - allowed: "specify managed network requirements through exactly one surface" - .to_string(), + allowed: "legacy and permissions.network requirements must describe the same network policy".to_string(), requirement_source: nested.source.clone(), }); } @@ -1542,6 +1594,8 @@ impl ConfigRequirementsWithSources { } } + /// Public loader view. Older runtime clients still read the top-level + /// network field when selecting an executor policy, including on resume. pub fn into_toml(self) -> ConfigRequirementsToml { let ConfigRequirementsWithSources { config_server, @@ -1596,6 +1650,12 @@ impl ConfigRequirementsWithSources { permissions.get_or_insert_with(Default::default).network = Some(Box::new(network.value)); } + let network = network.map(|sourced| sourced.value).or_else(|| { + permissions + .as_ref() + .and_then(|permissions| permissions.network.as_deref()) + .cloned() + }); ConfigRequirementsToml { config_server: config_server.map(|sourced| sourced.value), allowed_login_methods: allowed_login_methods.map(|sourced| sourced.value), @@ -1637,7 +1697,7 @@ impl ConfigRequirementsWithSources { apps: apps.map(|sourced| sourced.value), rules: rules.map(|sourced| sourced.value), enforce_residency: enforce_residency.map(|sourced| sourced.value), - network: network.map(|sourced| sourced.value), + network, application: application.map(|sourced| sourced.value), permissions, auto_review: auto_review.map(|sourced| sourced.value), @@ -2522,10 +2582,13 @@ impl TryFrom for ConfigRequirements { /*source*/ None, ), }; - let network = permissions_network - .map(|network| *network) - .or(network) - .map(|Sourced { value, source }| Sourced::new(NetworkConstraints::from(value), source)); + let network = match (permissions_network, network) { + // A dual-published source keeps its original selector text on older executors. + (Some(nested), Some(legacy)) if nested.source == legacy.source => Some(legacy), + (Some(nested), _) => Some(*nested), + (None, legacy) => legacy, + } + .map(|Sourced { value, source }| Sourced::new(NetworkConstraints::from(value), source)); let filesystem = permissions .filter(|permissions| permissions.value.filesystem.is_some()) .map(|Sourced { value, source }| { @@ -4995,6 +5058,15 @@ command = "python3 /enterprise/hooks/pre.py" )) }; assert_eq!(parse("network_proxy")?, parse("experimental_network")?); + for table in ["network_proxy", "experimental_network"] { + let dual: ConfigRequirementsToml = from_str(&format!( + "[{table}]\ndenied_domains = ['2001:db8::1']\n\ + [permissions.network]\ndenied_domains = ['2001:0db8:0:0:0:0:0:1']" + ))?; + let expected = dual.network.clone().map(NetworkConstraints::from); + let runtime = ConfigRequirements::try_from(with_unknown_source(dual))?; + assert_eq!(runtime.network.map(|network| network.value), expected); + } for values in ["", "enabled = true"] { let both = format!("[network_proxy]\n{values}\n[experimental_network]\n{values}"); assert!(from_str::(&both).is_err()); diff --git a/codex-rs/config/src/loader/managed_project_discovery_tests.rs b/codex-rs/config/src/loader/managed_project_discovery_tests.rs index 1148a048b5..45b7506e13 100644 --- a/codex-rs/config/src/loader/managed_project_discovery_tests.rs +++ b/codex-rs/config/src/loader/managed_project_discovery_tests.rs @@ -172,6 +172,13 @@ async fn managed_network_requirements_control_broker_project_discovery() -> anyh format!("[{table}]\nenabled = {enabled}\n"), )?; let (canonical, local) = fixture.load().await?; + let policy_only = crate::loader::load_managed_requirements_state( + &TestFileSystem, + &fixture.home, + fixture.overrides.clone(), + ) + .await?; + assert_eq!(&policy_only, canonical.requirements_toml()); let mut local_effective = TomlValue::Table(toml::map::Map::new()); for layer in local.config.layers { merge_toml_values(&mut local_effective, &layer.toml); @@ -203,8 +210,9 @@ async fn project_discovery_rejects_invalid_network_requirements() -> anyhow::Res fixture.overrides.system_requirements_path = Some(path.clone()); for table in ["network_proxy", "experimental_network"] { for (nested, message) in [ - ("http_port = 8090", "exactly one surface"), - ("enabled = false", "exactly one surface"), + ("http_port = 8090", "same network policy"), + ("enabled = false", "same network policy"), + ("review_domains = []", "same network policy"), ("", "rename the managed profile"), ( "extends = ':workspace'\nenabled = true", diff --git a/codex-rs/config/src/requirements_layers/layer.rs b/codex-rs/config/src/requirements_layers/layer.rs index 9fca525118..2f24e53c2c 100644 --- a/codex-rs/config/src/requirements_layers/layer.rs +++ b/codex-rs/config/src/requirements_layers/layer.rs @@ -154,10 +154,17 @@ impl ComposableRequirementsLayer { { table.insert("features".to_string(), features); } - if let Some(table) = regular_toml.as_table_mut() - && let Some(network) = table.remove("experimental_network") + if let Some(legacy) = &requirements.network + && let Some(nested) = requirements + .permissions + .as_ref() + .and_then(|p| p.network.as_deref()) + && !legacy.describes_same_policy(nested) { - table.insert("network_proxy".to_string(), network); + return Err(RequirementsCompositionError::Parse { + layer_source: source, + message: "legacy and permissions.network requirements must describe the same network policy".into(), + }); } // Hostname lookup is configuration-driven and may block on DNS, so only @@ -301,11 +308,14 @@ pub(crate) fn strip_cloud_auth_requirements(source: &RequirementSource, value: & } } -fn remove_top_level_field(value: &mut TomlValue, key: &str) -> Option { +pub(super) fn remove_top_level_field(value: &mut TomlValue, key: &str) -> Option { value.as_table_mut()?.remove(key) } -fn remove_nested_field_and_prune_empty(value: &mut TomlValue, path: &[&str]) -> Option { +pub(super) fn remove_nested_field_and_prune_empty( + value: &mut TomlValue, + path: &[&str], +) -> Option { let (key, remaining) = path.split_first()?; let table = value.as_table_mut()?; if remaining.is_empty() { diff --git a/codex-rs/config/src/requirements_layers/mod.rs b/codex-rs/config/src/requirements_layers/mod.rs index 569b1d2c12..ec5f9fac12 100644 --- a/codex-rs/config/src/requirements_layers/mod.rs +++ b/codex-rs/config/src/requirements_layers/mod.rs @@ -1,6 +1,7 @@ mod hooks; mod layer; mod models; +mod network; mod permissions; mod rules; mod stack; diff --git a/codex-rs/config/src/requirements_layers/network.rs b/codex-rs/config/src/requirements_layers/network.rs new file mode 100644 index 0000000000..34b1bc9480 --- /dev/null +++ b/codex-rs/config/src/requirements_layers/network.rs @@ -0,0 +1,78 @@ +//! Read compatible spellings of the shared network policy and preserve source attribution. + +use crate::NetworkRequirementsToml; +use crate::RequirementSource; +use crate::Sourced; +use crate::merge::merge_toml_values; +use toml::Value; + +use super::layer::ComposableRequirementsLayer; +use super::layer::remove_nested_field_and_prune_empty; +use super::layer::remove_top_level_field; +use super::stack::RequirementsCompositionError; + +/// Fold each validated source's network table before the regular merge. The +/// submitted TOML distinguishes empty replacement lists from omitted lists. +pub(super) fn compose( + layers: &mut [ComposableRequirementsLayer], +) -> Result>>, RequirementsCompositionError> { + let mut merged = Value::Table(toml::Table::new()); + let mut sources = Vec::new(); + let mut has_legacy = false; + let mut has_nested = false; + for layer in layers { + let regular_toml = &mut layer.regular_toml; + let legacy = remove_top_level_field(regular_toml, "network_proxy") + .or_else(|| remove_top_level_field(regular_toml, "experimental_network")); + let nested = remove_nested_field_and_prune_empty(regular_toml, &["permissions", "network"]); + has_legacy |= legacy.is_some(); + has_nested |= nested.is_some(); + let value = match (legacy, nested) { + (Some(Value::Table(legacy)), Some(Value::Table(nested))) => { + // The effective rules already agree. A deny can shadow an allow, + // but an explicit empty allowlist clears inherited allows. + if let (Some(left), Some(right)) = ( + legacy.get("allowed_domains").and_then(Value::as_array), + nested.get("allowed_domains").and_then(Value::as_array), + ) && left.is_empty() != right.is_empty() + { + return Err(RequirementsCompositionError::Parse { + layer_source: layer.source.clone(), + message: "legacy and permissions.network requirements specify different allowed_domains replacements".into(), + }); + } + // Keep the legacy spelling for the executor. + Value::Table(legacy) + } + (Some(value), None) | (None, Some(value)) => value, + (None, None) => continue, + (Some(_), Some(_)) => { + return Err(RequirementsCompositionError::Parse { + layer_source: layer.source.clone(), + message: "shared network requirements must be tables".into(), + }); + } + }; + sources.push(layer.source.clone()); + merge_toml_values(&mut merged, &value); + } + if sources.is_empty() { + return Ok(None); + } + // Until list composition can preserve competing replacements, require + // multiple contributing sources to use the same surface. + let mixed_sources = sources.len() > 1 && has_legacy && has_nested; + let source = RequirementSource::composite(sources.into_iter().rev()); + if mixed_sources { + return Err(RequirementsCompositionError::Parse { + layer_source: source, + message: "mixed legacy and permissions.network requirements".to_string(), + }); + } + let value = merged.try_into().map_err(|err: toml::de::Error| { + RequirementsCompositionError::ComposedParse { + message: err.to_string(), + } + })?; + Ok(Some(Box::new(Sourced::new(value, source)))) +} diff --git a/codex-rs/config/src/requirements_layers/stack.rs b/codex-rs/config/src/requirements_layers/stack.rs index 1323bb8cd2..300adbbd3a 100644 --- a/codex-rs/config/src/requirements_layers/stack.rs +++ b/codex-rs/config/src/requirements_layers/stack.rs @@ -20,7 +20,6 @@ use crate::ConfigRequirementsToml; use crate::ConfigRequirementsWithSources; use crate::RequirementSource; use crate::Sourced; -use crate::config_requirements::PermissionsRequirementsToml; use crate::config_requirements::merge_otel_requirements; use crate::merge::merge_toml_values; use std::cell::OnceCell; @@ -55,6 +54,30 @@ pub enum RequirementsCompositionError { }, } +impl From for crate::CloudConfigBundleLoadError { + fn from(error: RequirementsCompositionError) -> Self { + // Parser messages can contain the submitted TOML, including credentials. + let reason = match error { + RequirementsCompositionError::Parse { layer_source, .. } => { + format!("invalid requirements in {layer_source}") + } + RequirementsCompositionError::Conflict { + existing_source, + incoming_source, + .. + } => { + format!("conflicting requirements from {existing_source} and {incoming_source}") + } + RequirementsCompositionError::ComposedParse { .. } => "invalid requirements".to_owned(), + }; + Self::new( + crate::CloudConfigBundleLoadErrorCode::InvalidBundle, + /*status_code*/ None, + format!("invalid cloud config bundle: {reason}"), + ) + } +} + impl From for io::Error { fn from(error: RequirementsCompositionError) -> Self { io::Error::new(io::ErrorKind::InvalidData, error) @@ -150,10 +173,11 @@ impl RequirementsLayerStack { self, ) -> Result, RequirementsCompositionError> { let Self { - layers, + mut layers, hook_directory_field, } = self; + let network = super::network::compose(&mut layers)?; let mut merged_toml = TomlValue::Table(toml::map::Map::new()); for layer in &layers { // An endpoint and its credentials must come from the same requirements layer. @@ -173,11 +197,8 @@ impl RequirementsLayerStack { })?; let mut output = ConfigRequirementsWithSources::default(); populate_merged_regular_fields_with_sources(&mut output, requirements, &layers); - output.validate_network_spelling().map_err(|err| { - RequirementsCompositionError::ComposedParse { - message: err.to_string(), - } - })?; + output.permissions_network = network; + // Merge OTEL requirements so higher-priority exact values replace // lower-priority values while exporters stay atomic. let mut otel = None; @@ -281,9 +302,9 @@ fn populate_merged_regular_fields_with_sources( apps, rules: _, enforce_residency, - network, + network: _, application, - mut permissions, + permissions, auto_review, models, additional_developer_instructions, @@ -341,7 +362,6 @@ fn populate_merged_regular_fields_with_sources( set_sourced!(marketplaces, &["marketplaces"]); set_sourced!(apps, &["apps"]); set_sourced!(enforce_residency, &["enforce_residency"]); - set_sourced!(network, &["network_proxy"]); set_sourced!(application, &["application"]); let permission_sources = |contributes: fn(&toml::Table) -> bool| { RequirementSource::composite( @@ -358,12 +378,6 @@ fn populate_merged_regular_fields_with_sources( .map(|layer| layer.source.clone()), ) }; - if let Some(network) = PermissionsRequirementsToml::take_network(&mut permissions) { - output.permissions_network = Some(Box::new(Sourced::new( - *network, - permission_sources(|table| table.contains_key("network")), - ))); - } output.permissions = permissions.map(|value| { let source = if value.filesystem.is_some() { permission_sources(|table| table.contains_key("filesystem")) diff --git a/codex-rs/config/src/requirements_layers/stack_tests.rs b/codex-rs/config/src/requirements_layers/stack_tests.rs index 85b28885e4..ee42c6a531 100644 --- a/codex-rs/config/src/requirements_layers/stack_tests.rs +++ b/codex-rs/config/src/requirements_layers/stack_tests.rs @@ -48,7 +48,16 @@ fn compose_with_hook_directory_field( } fn expected_requirements(contents: impl AsRef) -> ConfigRequirementsToml { - toml::from_str(contents.as_ref()).expect("parse expected requirements TOML") + let mut expected: ConfigRequirementsToml = + toml::from_str(contents.as_ref()).expect("parse expected requirements TOML"); + if let Some(shared) = expected + .permissions + .as_ref() + .and_then(|p| p.network.as_deref()) + { + expected.network = Some(shared.clone()); + } + expected } #[test] @@ -813,13 +822,13 @@ fn network_maps_use_regular_toml_merge() { composed, expected_requirements( r#" -[experimental_network.domains] +[permissions.network.domains] "example.com" = "allow" "high.example.com" = "allow" "internal.example.com" = "deny" "low.example.com" = "deny" -[experimental_network.unix_sockets] +[permissions.network.unix_sockets] "/tmp/admin.sock" = "deny" "/tmp/high.sock" = "allow" "/tmp/low.sock" = "allow" @@ -856,17 +865,26 @@ fn network_aliases_merge_with_layer_precedence_and_sources() { let composed = compose_requirements_for_hostname(vec![low, high], /*hostname*/ None) .expect("compose network aliases") .expect("requirements present"); + let expected: crate::NetworkRequirementsToml = toml::from_str( + "enabled = false\nallow_local_binding = false\nallow_upstream_proxy = true\n[domains]\n'low.example.com' = 'allow'\n'high.example.com' = 'allow'\n'shared.example.com' = 'deny'", + ).expect("expected network"); assert_eq!( - composed.network, - Some(Sourced::new( - expected_requirements( - "[network_proxy]\nenabled = false\nallow_local_binding = false\nallow_upstream_proxy = true\n\ - [network_proxy.domains]\n'low.example.com' = 'allow'\n'high.example.com' = 'allow'\n'shared.example.com' = 'deny'" - ) - .network - .expect("network requirements"), - expected_source, - )) + composed, + ConfigRequirementsWithSources { + permissions_network: Some(Box::new(Sourced::new( + expected.clone(), + expected_source + ))), + ..Default::default() + } + ); + let public_view = composed.into_toml(); + assert_eq!(public_view.network.as_ref(), Some(&expected)); + assert_eq!( + public_view + .permissions + .and_then(|permissions| permissions.network), + Some(Box::new(expected)) ); } } @@ -897,6 +915,151 @@ fn permissions_network_conflicts_with_top_level_network_across_layers() { } } +#[test] +fn shadowed_allows_agree_across_network_spellings() { + let legacy = "[experimental_network]\nallowed_domains = ['a.test', 'b.test']\ndenied_domains = ['a.test', 'b.test']"; + let expected = expected_requirements( + "[permissions.network.domains]\n'a.test' = 'deny'\n'b.test' = 'deny'", + ); + for nested in [ + "[permissions.network]\nallowed_domains = ['a.test']\ndenied_domains = ['a.test', 'b.test']", + "[permissions.network.domains]\n'a.test' = 'deny'\n'b.test' = 'deny'", + ] { + let dual = format!("{legacy}\n{nested}"); + assert_eq!( + compose(vec![layer("same", "Same", &dual)]).expect("both hosts remain denied"), + Some(expected.clone()), + ); + } +} + +#[test] +fn empty_network_allowlist_clears_inherited_allows() { + for table in ["experimental_network", "permissions.network"] { + let low = format!( + "[{table}]\nallowed_domains = ['inherited.test']\ndenied_domains = ['blocked.test']" + ); + let clear = format!("[{table}]\nallowed_domains = []"); + assert_eq!( + compose(vec![ + layer("low", "Low", &low), + layer("high", "High", &clear) + ]) + .expect("clear inherited allowlist"), + Some(expected_requirements( + "[permissions.network.domains]\n'blocked.test' = 'deny'" + )) + ); + } +} + +#[test] +fn network_surfaces_in_one_layer_must_agree() { + for table in ["network_proxy", "experimental_network"] { + for fields in ["", "allowed_domains = []", "description = 'device policy'"] { + let entry = layer("device", "Device", &format!("[{table}]\n{fields}")); + let expected = Sourced::new( + crate::NetworkRequirementsToml::default(), + entry.source.clone(), + ); + assert_eq!( + compose_requirements_for_hostname([entry], /*hostname*/ None) + .expect("valid legacy input"), + Some(ConfigRequirementsWithSources { + permissions_network: Some(Box::new(expected)), + ..Default::default() + }), + ); + } + let legacy_lists = "allowed_domains = ['MANAGED.TEST.']\ndenied_domains = ['2001:db8::1']"; + let legacy_map = "domains = { 'MANAGED.TEST.' = 'allow', '2001:db8::1' = 'deny' }"; + let nested_lists = + "allowed_domains = ['managed.test']\ndenied_domains = ['2001:0db8:0:0:0:0:0:1']"; + let nested_map = "domains = { 'managed.test' = 'allow', '2001:0db8:0:0:0:0:0:1' = 'deny' }"; + let expected = expected_requirements(format!( + "[permissions.network]\n{legacy_lists}\nreview_domains = ['*.First.Test.', 'second.test', '2001:db8::2']" + )); + for legacy in [legacy_lists, legacy_map] { + for nested in [nested_lists, nested_map] { + let dual = format!( + "[{table}]\n{legacy}\nreview_domains = ['*.First.Test.', 'second.test', '2001:db8::2']\n\ + [permissions.network]\n{nested}\nreview_domains = ['SECOND.TEST.', '?*.first.test', '*.first.test', '2001:0db8:0:0:0:0:0:2']" + ); + assert_eq!( + compose(vec![layer("same", "Same", &dual)]).expect("equivalent copies"), + Some(expected.clone()) + ); + } + } + // An exact allow/deny pair stays a single denial even when only the nested + // copy supplies the shadowed allow. The published denial keeps the legacy text. + for (left, right, published) in [ + ( + "allowed_domains = ['ONE.TEST.']\ndenied_domains = ['ONE.TEST.']", + "denied_domains = ['one.test']", + "ONE.TEST.", + ), + ( + "denied_domains = ['one.test']", + "allowed_domains = ['ONE.TEST.']\ndenied_domains = ['ONE.TEST.']", + "one.test", + ), + ] { + let dual = format!("[{table}]\n{left}\n[permissions.network]\n{right}"); + assert_eq!( + compose(vec![layer("same", "Same", &dual)]).expect("equivalent list operations"), + Some(expected_requirements(format!( + "[permissions.network]\ndenied_domains = ['{published}']" + ))), + ); + } + for (legacy, nested) in [ + ( + "allowed_domains = ['127.0.0.1:*']", + "allowed_domains = ['127.0.0.1']", + ), + ( + "allowed_domains = ['one.test']", + "allowed_domains = ['one.test:443/path']", + ), + ( + "review_domains = ['*.one.test']", + "review_domains = ['**.ONE.TEST.']", + ), + ( + "allowed_domains = ['ONE.TEST.']\ndenied_domains = ['one.test']", + "denied_domains = ['one.test']", + ), + ] { + let conflicting = format!("[{table}]\n{legacy}\n[permissions.network]\n{nested}"); + assert!(matches!( + compose(vec![layer("conflict", "Conflict", &conflicting)]), + Err(RequirementsCompositionError::Parse { .. }) + )); + } + let shadowed = format!( + "[{table}]\nallowed_domains = ['host.test']\ndenied_domains = ['host.test']\n[permissions.network]\nallowed_domains = []\ndenied_domains = ['host.test']" + ); + assert!(matches!( + compose(vec![layer("dual", "Dual", &shadowed)]), + Err(RequirementsCompositionError::Parse { message, .. }) if message.contains("different allowed_domains replacements") + )); + let entry = layer( + "invalid", + "Invalid", + &format!("[{table}]\n[permissions.network]\nenabled = false"), + ); + let source = entry.source.clone(); + let err = compose_requirements_for_hostname(vec![entry], /*hostname*/ None) + .expect_err("conflicting network requirement surfaces must fail within one layer"); + assert!( + matches!(&err, RequirementsCompositionError::Parse { layer_source, .. } if *layer_source == source), + "{err}" + ); + assert!(err.to_string().contains("same network policy"), "{err}"); + } +} + #[test] fn browser_and_computer_use_requirements_use_regular_toml_merge() { let composed = compose(vec![ @@ -1603,7 +1766,7 @@ deny_read = [{denied_path:?}] layer( "profile", "Profile", - "[permissions.other]\nextends = ':workspace'", + "[permissions.other]\nextends = ':workspace'\n[permissions.other.network]\nenabled = false", ), ], /*hostname*/ None, @@ -1630,6 +1793,8 @@ enabled = true [permissions.other] extends = ":workspace" +[permissions.other.network] +enabled = false [permissions.managed] extends = ":workspace" diff --git a/codex-rs/core/src/config/network_config_tests.rs b/codex-rs/core/src/config/network_config_tests.rs index b9517ec6fc..b2ea969ece 100644 --- a/codex-rs/core/src/config/network_config_tests.rs +++ b/codex-rs/core/src/config/network_config_tests.rs @@ -146,7 +146,7 @@ action = ['redact'] "[unix_sockets]\n'relative.sock' = 'allow'", "[unix_sockets]\n'~/allowed.sock' = 'allow'", "[unix_sockets]\n\"/tmp/\\u0000allowed.sock\" = 'allow'", - "[domains]\n'[' = 'allow'", + "[domains]\n'api[12].example' = 'allow'", ] { let network: NetworkToml = toml::from_str(raw).unwrap(); let projected = project_environment_profile_network(Some(network.clone())) @@ -154,6 +154,14 @@ action = ['redact'] .unwrap(); assert_eq!(projected, network); let configured_proxy = projected.to_network_proxy_config(); + assert!( + PreparedNetworkConfig { + configured_proxy: configured_proxy.clone() + } + .build(/*requirements*/ None, &PermissionProfile::read_only()) + .expect("a disabled unmanaged profile can load unused selectors") + .is_none() + ); let policy = EnvironmentNetworkPolicy::from_config( &configured_proxy, /*managed_allowed_domains_only*/ false, diff --git a/codex-rs/core/src/config/network_proxy_spec_tests.rs b/codex-rs/core/src/config/network_proxy_spec_tests.rs index fa179e0a89..2a3d60fc48 100644 --- a/codex-rs/core/src/config/network_proxy_spec_tests.rs +++ b/codex-rs/core/src/config/network_proxy_spec_tests.rs @@ -27,6 +27,72 @@ fn domain_permissions( } } +#[tokio::test] +async fn saved_grant_cannot_open_loopback_under_fixed_allowlist() -> anyhow::Result<()> { + let profile = PermissionProfile::workspace_write(); + let spec = NetworkProxySpec::from_config_and_constraints( + NetworkProxyConfig { + proxy_url: "http://127.0.0.1:0".into(), + enable_socks5: false, + allow_upstream_proxy: false, + ..Default::default() + }, + Some(NetworkConstraints { + enabled: Some(true), + managed_allowed_domains_only: Some(true), + allow_local_binding: Some(false), + domains: Some(domain_permissions([( + "127.0.0.1:*", + NetworkDomainPermissionToml::Allow, + )])), + ..Default::default() + }), + &profile, + )?; + let mut saved = Policy::empty(); + saved.add_network_rule( + "127.0.0.1", + codex_execpolicy::NetworkRuleProtocol::Http, + Allow, + /*justification*/ None, + )?; + let spec = spec.with_exec_policy_network_rules(&saved)?; + assert_eq!( + spec.config.allowed_domains(), + Some(vec!["127.0.0.1:*".to_owned()]) + ); + + core_test_support::skip_if_no_network!(Ok(())); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await?; + let proxy = spec + .start_proxy( + &profile, + ManagedProxyRouting::DedicatedListeners, + DefaultFalse, + /*policy_decider*/ None, + /*blocked_request_observer*/ None, + /*enable_network_approval_flow*/ false, + NetworkProxyAuditMetadata::default(), + ) + .await?; + let request = codex_http_client::HttpClientBuilder::new() + .build_direct()? + .get(format!("http://{}/", proxy.proxy().http_addr())) + .header(http::header::HOST, listener.local_addr()?.to_string()) + .timeout(std::time::Duration::from_secs(5)) + .send(); + tokio::select! { + biased; + connection = listener.accept() => anyhow::bail!("loopback destination reached: {:?}", connection?.1), + response = request => { + let response = response?; + assert_eq!(response.status(), http::StatusCode::FORBIDDEN); + assert_eq!(response.json::().await?["reason"], "not_allowed_local"); + } + } + Ok(()) +} + #[tokio::test] async fn attachment_socket_grants_respect_configured_restrictions_at_remote_launch() -> anyhow::Result<()> { diff --git a/codex-rs/tui/src/debug_config.rs b/codex-rs/tui/src/debug_config.rs index 12ec79a8cb..353c4b1766 100644 --- a/codex-rs/tui/src/debug_config.rs +++ b/codex-rs/tui/src/debug_config.rs @@ -668,7 +668,6 @@ mod tests { use codex_config::ConfigLayerStack; use codex_config::ConfigRequirements; use codex_config::ConfigRequirementsToml; - use codex_config::ConfigRequirementsWithSources; use codex_config::Constrained; use codex_config::ConstrainedWithSource; use codex_config::ConstraintError; @@ -686,11 +685,13 @@ mod tests { use codex_config::NetworkUnixSocketPermissionToml; use codex_config::NetworkUnixSocketPermissionsToml; use codex_config::RequirementSource; + use codex_config::RequirementsLayerEntry; use codex_config::ResidencyRequirement; use codex_config::SandboxModeRequirement; use codex_config::Sourced; use codex_config::WebSearchModeRequirement; use codex_config::WindowsRequirementsToml; + use codex_config::compose_requirements; use codex_config::sandbox_mode_requirement_for_permission_profile; use codex_config::types::FeedbackConfigToml; use codex_protocol::config_types::ApprovalsReviewer; @@ -1220,13 +1221,20 @@ interrupt_message = false "experimental_network", "permissions.network", ] { - let toml: ConfigRequirementsToml = - toml::from_str(&format!("[{table}]\nenabled = true")).expect("requirements"); - let mut sourced = ConfigRequirementsWithSources::default(); - sourced.merge_unset_fields(RequirementSource::Unknown, toml.clone()); - let requirements = ConfigRequirements::try_from(sourced).expect("valid requirements"); - let stack = - ConfigLayerStack::new(Vec::new(), requirements, toml).expect("config layer stack"); + let layer = RequirementsLayerEntry::from_toml( + RequirementSource::EnterpriseManaged { + id: "network".to_string(), + name: "Network policy".to_string(), + }, + format!("[{table}]\nenabled = true"), + ); + let sourced = compose_requirements([layer]) + .expect("compose requirements") + .expect("managed network requirements"); + let requirements = + ConfigRequirements::try_from(sourced.clone()).expect("valid requirements"); + let stack = ConfigLayerStack::new(Vec::new(), requirements, sourced.into_toml()) + .expect("config layer stack"); outputs.push(format!("{table}:\n{}", render_stack_to_text(&stack))); } diff --git a/codex-rs/tui/src/snapshots/codex_tui__debug_config__tests__debug_config_network_requirement_surfaces.snap b/codex-rs/tui/src/snapshots/codex_tui__debug_config__tests__debug_config_network_requirement_surfaces.snap index 4dc4214def..a9fe60b909 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__debug_config__tests__debug_config_network_requirement_surfaces.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__debug_config__tests__debug_config_network_requirement_surfaces.snap @@ -9,7 +9,7 @@ Config layer stack (lowest precedence first): Requirements: - - network_proxy: enabled=true (source: ) + - permissions.network: enabled=true (source: enterprise-managed requirements Network policy (network)) experimental_network: /debug-config @@ -18,7 +18,7 @@ Config layer stack (lowest precedence first): Requirements: - - network_proxy: enabled=true (source: ) + - permissions.network: enabled=true (source: enterprise-managed requirements Network policy (network)) permissions.network: /debug-config @@ -27,4 +27,4 @@ Config layer stack (lowest precedence first): Requirements: - - permissions.network: enabled=true (source: ) + - permissions.network: enabled=true (source: enterprise-managed requirements Network policy (network)) diff --git a/codex-rs/windows-sandbox-service/src/machine_policy_tests.rs b/codex-rs/windows-sandbox-service/src/machine_policy_tests.rs index db194e757f..29de0f9ab4 100644 --- a/codex-rs/windows-sandbox-service/src/machine_policy_tests.rs +++ b/codex-rs/windows-sandbox-service/src/machine_policy_tests.rs @@ -318,7 +318,7 @@ fn malformed_machine_policy_fails_closed() { validate_requirements( &WindowsSandboxProvisioningSettings::default(), WindowsSandboxProxyListeners::default(), - &format!("[{table}]\nenabled = true\n[permissions.network]\nenabled = true") + &format!("[{table}]\nenabled = true\n[permissions.network]\nenabled = false") ) .is_err() );