diff --git a/codex-rs/core/src/config/mcp_network_policy_tests.rs b/codex-rs/core/src/config/mcp_network_policy_tests.rs new file mode 100644 index 0000000000..60f1fb0904 --- /dev/null +++ b/codex-rs/core/src/config/mcp_network_policy_tests.rs @@ -0,0 +1,326 @@ +#![cfg(test)] + +//! Exercises the policy carried by MCP independently of tool-call publication. + +use super::*; +use crate::config::ConfigBuilder; +use anyhow::Context; +use codex_execpolicy::Decision; +use codex_execpolicy::NetworkRuleProtocol::Https; +use codex_http_client::HttpClientBuilder; +use codex_network_proxy::NetworkMode; +use codex_protocol::models::PermissionProfile as Profile; +use codex_protocol::models::PermissionProfile::Disabled; +use codex_protocol::models::PermissionProfile::External; +use codex_protocol::permissions::NetworkSandboxPolicy::Enabled; +use codex_protocol::permissions::NetworkSandboxPolicy::Restricted; +use codex_protocol::protocol::AskForApproval::Never; +use codex_protocol::protocol::AskForApproval::OnRequest; +use http::StatusCode; +use pretty_assertions::assert_eq; +use serde_json::Value; +use serde_json::json; +use std::time::Duration; +use test_case::test_case; + +fn saved_rules(rules: &[(&str, Decision)]) -> anyhow::Result { + let mut policy = Policy::empty(); + for &(host, decision) in rules { + policy.add_network_rule(host, Https, decision, /*justification*/ None)?; + } + Ok(policy) +} + +fn primary_controller_policy( + controller: &NetworkProxySpec, + profile: &Profile, + rules: &Policy, +) -> std::io::Result { + NetworkProxySpec::mcp_network_policy( + Some(controller), + profile, + McpNetworkRoute { + primary_allows_proxy: !matches!(profile, Disabled), + local_binding: LocalBindingPolicy::DefaultFalse, + owner: None, + }, + profile, + OnRequest, + rules, + ) +} + +#[test_case(Profile::read_only(), false, None, false; "restricted offline")] +#[test_case(Profile::read_only(), true, None, true; "controller proxy")] +#[test_case(Profile::read_only(), false, Some(true), true; "managed proxy")] +#[test_case(Profile::read_only(), true, Some(false), false; "managed disable")] +#[test_case(External { network: Enabled }, false, None, true; "external network")] +#[test_case(Disabled, false, None, true; "disabled sandbox")] +#[test_case(Disabled, true, Some(false), false; "managed veto in disabled sandbox")] +fn controller_policy_keeps_managed_restrictions_after_stale_grants( + profile: Profile, + proxy_enabled: bool, + managed_enabled: Option, + expected_enabled: bool, +) -> anyhow::Result<()> { + let requirements = serde_json::from_value(json!({ + "enabled": managed_enabled, "managed_allowed_domains_only": true, "allow_local_binding": true, + "review_domains": ["allowed2.example.com"], + "domains": {"allowed?.example.com": "allow", "blocked.example.com": "deny"}, + }))?; + let config = NetworkProxyConfig { + enabled: proxy_enabled, + mode: NetworkMode::Limited, + ..Default::default() + }; + let controller = + NetworkProxySpec::from_config_and_constraints(config, Some(requirements), &profile)?; + let saved = saved_rules(&[ + ("previously-allowed.example.com", Decision::Allow), + ("allowed1.example.com", Decision::Forbidden), + ])?; + let result = primary_controller_policy(&controller, &profile, &saved)?; + let routed = managed_enabled.unwrap_or(proxy_enabled) && !matches!(profile, Disabled); + let mut denied = vec!["blocked.example.com"]; + if routed && matches!(profile, Profile::Managed { .. }) { + denied.push("allowed1.example.com"); + } + denied.push("allowed2.example.com"); + assert_eq!( + serde_json::to_value(result)?, + json!({ + "enabled": expected_enabled, "mode": if routed { "limited" } else { "full" }, "allowedDomains": ["allowed?.example.com"], + "deniedDomains": denied, "hardDenyAllowlistMisses": true, + }) + ); + Ok(()) +} + +const MANAGED_DOMAINS: &str = + "enabled = true\ndomains = { '*.example' = 'allow', 'admin.test' = 'deny' }"; + +#[test_case(None, false, Profile::read_only(), Some(MANAGED_DOMAINS), OnRequest, false, false; "no proxy restricted")] +#[test_case(None, false, Disabled, Some(MANAGED_DOMAINS), Never, true, false; "no proxy unrestricted")] +#[test_case(Some(Profile::workspace_write()), true, External { network: Enabled }, Some(MANAGED_DOMAINS), OnRequest, true, true; "external secondary retains thread denial under wildcard")] +#[test_case(Some(External { network: Enabled }), true, Profile::workspace_write(), Some(MANAGED_DOMAINS), Never, true, true; "managed secondary cannot restore rejected grants")] +#[test_case(Some(Disabled), false, Disabled, Some(MANAGED_DOMAINS), Never, true, false; "full access primary uses direct networking")] +#[test_case(Some(Disabled), false, Profile::workspace_write(), Some(MANAGED_DOMAINS), Never, false, false; "restricted secondary stays offline with full access primary")] +#[test_case(Some(Profile::workspace_write()), true, Disabled, Some(MANAGED_DOMAINS), OnRequest, true, true; "full access secondary uses primary proxy")] +#[test_case(Some(Profile::workspace_write()), true, Profile::workspace_write(), Some(MANAGED_DOMAINS), OnRequest, true, false; "managed call may approve misses")] +#[test_case(Some(Profile::workspace_write()), true, Profile::workspace_write(), Some(MANAGED_DOMAINS), Never, true, true; "never denies routed misses")] +#[test_case(Some(Profile::workspace_write()), true, Profile::workspace_write(), None, OnRequest, true, true; "profile-only proxy denies misses")] +#[test_case(Some(Disabled), false, Disabled, None, OnRequest, true, false; "direct call bypasses profile and saved rules")] +#[test_case(Some(Disabled), false, Disabled, Some("http_port = 3128"), OnRequest, true, false; "managed port leaves direct calls unrestricted")] +fn controller_allowlist_misses_follow_call_profile( + thread: Option, + primary_allows_proxy: bool, + profile: Profile, + requirements: Option<&str>, + approval: AskForApproval, + enabled: bool, + hard_deny: bool, +) -> anyhow::Result<()> { + let mut config: NetworkProxyConfig = toml::from_str( + r#" + enabled = true + mode = 'limited' + allow_local_binding = true + domains = { '**.profile.test' = 'allow', 'blocked.example' = 'deny' } + mitm = true + mitm_hooks = [{ host = 'Hook.Profile.Test.:443', match = { methods = ['GET'], path_prefixes = ['/allowed/'] } }] + "#, + )?; + config.review_domains.push("review.profile.test".into()); + let spec = NetworkProxySpec::from_config_and_constraints( + config, + requirements.map(toml::from_str).transpose()?, + &profile, + )?; + let saved = saved_rules(&[ + ("review.profile.test", Decision::Allow), + ("saved-deny.test", Decision::Forbidden), + ])?; + let result = NetworkProxySpec::mcp_network_policy( + thread.as_ref().map(|_| &spec), + thread.as_ref().unwrap_or(&profile), + McpNetworkRoute { + primary_allows_proxy, + local_binding: LocalBindingPolicy::DefaultFalse, + owner: None, + }, + &profile, + approval, + &saved, + )?; + let mut allowed = Vec::new(); + let mut denied = Vec::new(); + if thread.is_some() && requirements == Some(MANAGED_DOMAINS) { + allowed.push("*.example"); + denied.push("admin.test"); + } + let uses_profile_rules = matches!(thread, Some(Profile::Managed { .. })); + if uses_profile_rules { + allowed.extend(["**.profile.test", "review.profile.test"]); + denied.extend(["blocked.example", "saved-deny.test"]); + } + if primary_allows_proxy { + denied.extend(["review.profile.test", "hook.profile.test"]); + } + assert_eq!( + serde_json::to_value(result)?, + json!({ + "enabled": enabled, "mode": if primary_allows_proxy { "limited" } else { "full" }, + "allowedDomains": allowed, "deniedDomains": denied, "hardDenyAllowlistMisses": hard_deny, + }) + ); + Ok(()) +} + +const FIXED_CONTROLLER_RULES: &str = "managed_allowed_domains_only = true\ndomains = { 'managed.example' = 'allow', 'managed-denied.example' = 'deny' }"; +const ACTIVE_CONTROLLER: &str = + "enabled = true\nmode = 'limited'\ndomains = { 'controller-denied.example' = 'deny' }"; + +#[test_case(None, None, None, [true, false, true]; "absent controller")] +#[test_case(Some("enabled = false"), Some(FIXED_CONTROLLER_RULES), Some("managed-denied.example"), [false; 3]; "disabled controller with a disjoint managed allowlist")] +#[test_case(Some(ACTIVE_CONTROLLER), None, Some("controller-denied.example"), [true; 3]; "profile-only proxy with a reviewable owner")] +fn owner_activation_and_traffic_policy( + configured: Option<&str>, + managed: Option<&str>, + controller_denial: Option<&str>, + [required, offline, direct]: [bool; 3], +) -> anyhow::Result<()> { + let fixed_owner = managed.is_some() || configured.is_none(); + let spec = NetworkProxySpec::from_config_and_constraints( + toml::from_str(configured.unwrap_or_default())?, + managed.map(toml::from_str).transpose()?, + &Profile::read_only(), + )?; + let controller = configured.or(managed).map(|_| &spec); + let mut owner: EnvironmentNetworkPolicy = serde_json::from_value(json!({ + "domains": {"owner.example": "allow", "owner-denied.example": "deny"}, + "reviewDomains": ["owner-review.example"], "allowUpstreamProxy": true, + "allowLocalBinding": true, "dangerouslyAllowAllUnixSockets": false, "managedAllowedDomainsOnly": fixed_owner, + }))?; + let saved = saved_rules(&[("owner-denied.example", Decision::Allow)])?; + let mut denied = vec!["owner-denied.example"]; + denied.extend(controller_denial); + denied.push("owner-review.example"); + for (requires_proxy, network, expected) in [ + (true, Restricted, required), + (false, Restricted, offline), + (false, Enabled, direct), + ] { + owner.requires_proxy = requires_proxy; + let profile = Profile::from_runtime_permissions( + &Profile::read_only().file_system_sandbox_policy(), + network, + ); + for approval in [OnRequest, Never] { + let result = NetworkProxySpec::mcp_network_policy( + controller, + &Disabled, + McpNetworkRoute { + primary_allows_proxy: false, + local_binding: LocalBindingPolicy::DefaultFalse, + owner: Some(&owner), + }, + &profile, + approval, + &saved, + )?; + assert_eq!( + json!(result), + json!({ + "enabled": expected, "mode": if fixed_owner { "full" } else { "limited" }, "allowedDomains": ["owner.example"], + "deniedDomains": denied, "hardDenyAllowlistMisses": fixed_owner || approval == Never, + }) + ); + } + } + Ok(()) +} + +const MIXED_DOMAINS: &str = r#" +[permissions.profile.network.domains] +"*" = "allow" +"*.example" = "allow" +"named.example" = "allow" +"localhost" = "allow" +"127.0.0.1:*" = "allow" +"192.168.0.2" = "allow" +"[::1]" = "allow" +"#; + +#[test_case("domains = { 'allowed.example' = 'allow' }", "outside.invalid", true, &["allowed.example"]; "allowlist miss")] +#[test_case(MIXED_DOMAINS, "127.0.0.1", false, &["192.168.0.2", "[::1]"]; "wildcard cannot grant loopback")] +#[test_case(MIXED_DOMAINS, "192.168.0.1", false, &["192.168.0.2", "[::1]"]; "wildcard cannot grant private address")] +#[tokio::test] +async fn profile_only_allowlist_projection_matches_native_proxy_denial( + domains: &str, + blocked_host: &str, + allow_local_binding: bool, + expected_grants: &[&str], +) -> anyhow::Result<()> { + let home = tempfile::tempdir()?; + let source = format!( + r#" +default_permissions = "profile" +features.network_proxy = true +permissions.profile.filesystem.":root" = "read" +[permissions.profile.network] +enabled = true +enable_socks5 = false +allow_local_binding = {allow_local_binding} +{domains} +"# + ); + std::fs::write(home.path().join("config.toml"), source)?; + let config = ConfigBuilder::without_managed_config_for_tests() + .codex_home(home.path().to_path_buf()) + .without_project_context() + .build() + .await?; + let spec = config + .permissions + .network + .as_ref() + .context("loaded profile proxy")?; + let profile = config.permissions.permission_profile(); + let requirements = config.config_layer_stack.requirements_toml(); + assert!(requirements.network.is_none()); + let result = primary_controller_policy(spec, profile, &Policy::empty())?; + assert!(result.enabled && result.hard_deny_allowlist_misses); + assert_eq!(result.allowed_domains, expected_grants); + core_test_support::skip_if_no_network!(Ok(())); + let proxy = spec + .start_proxy( + profile, + ManagedProxyRouting::DedicatedListeners, + LocalBindingPolicy::DefaultFalse, + /*policy_decider*/ None, + /*blocked_request_observer*/ None, + /*enable_network_approval_flow*/ false, + NetworkProxyAuditMetadata::default(), + ) + .await?; + let response = HttpClientBuilder::new() + .build_direct()? + .get(format!("http://{}/", proxy.proxy().http_addr())) + .header(http::header::HOST, blocked_host) + .timeout(Duration::from_secs(5)) + .send() + .await?; + assert_eq!(response.status(), StatusCode::FORBIDDEN); + assert_eq!(response.headers()["x-proxy-error"], "blocked-by-allowlist"); + let denial = response.json::().await?; + assert_eq!(denial["host"], blocked_host); + assert_eq!( + denial["reason"], + if allow_local_binding { + "not_allowed" + } else { + "not_allowed_local" + } + ); + Ok(()) +} diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index dc50426f76..04bb9ee1f2 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -210,6 +210,8 @@ pub use network_config::NetworkConfigInputs; pub use network_config::PreparedNetworkConfig; pub use network_config::project_environment_profile_network; pub use network_config::validate_environment_network_policy; +#[cfg(test)] +pub(crate) use network_proxy_spec::McpNetworkRoute; pub use network_proxy_spec::NetworkProxySpec; pub use network_proxy_spec::StartedNetworkProxy; pub use permission_profile_catalog::PermissionProfileCatalogEntry; diff --git a/codex-rs/core/src/config/network_proxy_spec.rs b/codex-rs/core/src/config/network_proxy_spec.rs index 3d4591c1e4..f7d2c2b621 100644 --- a/codex-rs/core/src/config/network_proxy_spec.rs +++ b/codex-rs/core/src/config/network_proxy_spec.rs @@ -1,5 +1,6 @@ use codex_config::NetworkConstraints; use codex_execpolicy::Policy; +use codex_mcp::NetworkPolicyMetadata; use codex_network_proxy::BlockedRequestObserver; use codex_network_proxy::ConfigReloader; use codex_network_proxy::ConfigReloaderFuture; @@ -8,6 +9,7 @@ use codex_network_proxy::EnvironmentNetworkPolicy; use codex_network_proxy::LocalBindingPolicy; use codex_network_proxy::ManagedProxyRouting; use codex_network_proxy::NetworkDecision; +use codex_network_proxy::NetworkDomainPermission; use codex_network_proxy::NetworkPolicyDecider; use codex_network_proxy::NetworkProxy; use codex_network_proxy::NetworkProxyAuditMetadata; @@ -22,8 +24,9 @@ use codex_network_proxy::managed_proxy_ports; use codex_network_proxy::normalize_host; use codex_network_proxy::validate_policy_against_constraints; use codex_protocol::models::PermissionProfile; +use codex_protocol::protocol::AskForApproval; use codex_utils_path_uri::Platform; -use std::collections::HashSet; +use std::net::IpAddr; use std::sync::Arc; #[derive(Debug, Clone, PartialEq, Eq)] @@ -35,6 +38,14 @@ pub struct NetworkProxySpec { hard_deny_allowlist_misses: bool, } +/// The primary attachment controls the shared proxy. The executor handling the call +/// supplies local-binding semantics even when it inherits the controller policy. +pub(crate) struct McpNetworkRoute<'a> { + pub(crate) primary_allows_proxy: bool, + pub(crate) local_binding: LocalBindingPolicy, + pub(crate) owner: Option<&'a EnvironmentNetworkPolicy>, +} + pub struct StartedNetworkProxy { proxy: NetworkProxy, network_policy_decider: Option>, @@ -270,13 +281,29 @@ impl NetworkProxySpec { exec_policy: &Policy, local_binding_policy: LocalBindingPolicy, ) -> std::io::Result { - if matches!(permission_profile, PermissionProfile::Disabled) { + Self::compose_environment_policy( + controller, + policy, + permission_profile, + exec_policy, + Some(local_binding_policy), + ) + } + + fn compose_environment_policy( + controller: Option<&Self>, + policy: &EnvironmentNetworkPolicy, + permission_profile: &PermissionProfile, + exec_policy: &Policy, + routed_binding: Option, + ) -> std::io::Result { + if routed_binding.is_some() && matches!(permission_profile, PermissionProfile::Disabled) { return Err(std::io::Error::new( std::io::ErrorKind::InvalidInput, "environment network policy requires managed network enforcement", )); } - if controller.is_some_and(|controller| !controller.enabled()) { + if routed_binding.is_some() && controller.is_some_and(|controller| !controller.enabled()) { return Err(std::io::Error::new( std::io::ErrorKind::InvalidInput, "environment network policy cannot override a disabled controller proxy", @@ -297,7 +324,8 @@ impl NetworkProxySpec { )?, }; policy.apply_to(&mut spec.config); - spec.config.allow_local_binding = Some(local_binding_policy.resolve(&spec.config)); + spec.config.allow_local_binding = + Some(routed_binding.unwrap_or_default().resolve(&spec.config)); let protected_denials = spec.config.denied_domains().unwrap_or_default(); // A fixed controller allowlist remains a ceiling; an expandable one is only a baseline. @@ -318,19 +346,34 @@ impl NetworkProxySpec { // Saved grants can extend a reviewable owner policy; owner denials are restored last. let (allowed_domains, denied_domains) = exec_policy.compiled_network_domains(); - if allow_owner_grants { - upsert_network_domains(&mut spec.config, allowed_domains, /*allow*/ true); + spec.config.apply_saved_domain_rules( + if allow_owner_grants { + allowed_domains + } else { + Vec::new() + }, + &denied_domains, + ); + for host in protected_denials { + spec.config.upsert_domain_permission( + host, + NetworkDomainPermission::Deny, + normalize_host, + ); } - upsert_network_domains(&mut spec.config, denied_domains, /*allow*/ false); - upsert_network_domains(&mut spec.config, protected_denials, /*allow*/ false); spec.constraints.denied_domains = spec.config.denied_domains(); - - validate_policy_against_constraints(&spec.config, &spec.constraints).map_err(|error| { - std::io::Error::new( - std::io::ErrorKind::InvalidInput, - format!("environment network policy violates managed requirements: {error}"), - ) - })?; + if routed_binding.is_some() { + validate_policy_against_constraints(&spec.config, &spec.constraints).map_err( + |error| { + std::io::Error::new( + std::io::ErrorKind::InvalidInput, + format!( + "environment network policy violates managed requirements: {error}" + ), + ) + }, + )?; + } tracing::debug!( controller_policy = ?controller.map(Self::environment_policy), controller_allow_all_unix_sockets = ?controller.and_then(|spec| spec.config.dangerously_allow_all_unix_sockets), @@ -341,12 +384,101 @@ impl NetworkProxySpec { Ok(spec) } + /// Derives the network restrictions a trusted MCP server may apply to a call. + #[cfg_attr(not(test), expect(dead_code, reason = "used by the next PR"))] + pub(crate) fn mcp_network_policy( + controller: Option<&Self>, + controller_profile: &PermissionProfile, + route: McpNetworkRoute<'_>, + permission_profile: &PermissionProfile, + approval_policy: AskForApproval, + exec_policy: &Policy, + ) -> std::io::Result { + let direct_network = permission_profile.network_sandbox_policy().is_enabled(); + // An owner can supply its own proxy when the controller is absent. A disabled + // controller vetoes that route, so file tools receive an offline policy. + let proxy_active = match route.owner { + Some(owner) => controller.map_or(owner.requires_proxy || direct_network, Self::enabled), + None => route.primary_allows_proxy && controller.is_some_and(Self::enabled), + }; + let network_enabled = proxy_active || (route.owner.is_none() && direct_network); + let spec = if let Some(owner) = route.owner { + // Offline file tools retain the owner's traffic rules without needing a startable proxy. + Self::compose_environment_policy( + controller, + owner, + permission_profile, + exec_policy, + proxy_active.then_some(route.local_binding), + )? + } else if let Some(controller) = controller + && proxy_active + { + // The routed controller uses the thread's profile even for calls from another executor. + let spec = controller.recompute_for_permission_profile(controller_profile)?; + spec.with_exec_policy_network_rules(exec_policy) + .unwrap_or(spec) + } else { + // Direct calls retain administrative policy, without importing unused proxy settings. + Self::from_config_and_constraints( + Default::default(), + controller.and_then(|controller| controller.requirements.clone()), + controller_profile, + )? + }; + + let config = spec.config; + // The native decider reviews fresh misses only for managed calls with approvals enabled. + let can_review = (route.owner.is_some() || spec.requirements.is_some()) + && Self::managed_sandbox_active(permission_profile) + && approval_policy != AskForApproval::Never; + // The domain-only MCP contract cannot check DNS answers at connect time. + // With local access restricted, only explicit IP grants are safe to forward. + let require_ip_literal = proxy_active && !route.local_binding.resolve(&config); + // An MXC executor cannot launch an active proxy with an explicit local-binding denial. + let can_route = + !(require_ip_literal && route.local_binding == LocalBindingPolicy::RequireTrue); + let mut allowed_domains = config.allowed_domains().unwrap_or_default(); + if require_ip_literal { + allowed_domains.retain(|rule| { + !rule.contains(['*', '?']) && normalize_host(rule).parse::().is_ok() + }); + } + let denied_domains = config.denied_domains().unwrap_or_default(); + // The receiver cannot prompt for review or enforce a proxy hook's per-request restrictions. + let mut additional_denials = config.review_domains; + if proxy_active { + additional_denials.extend( + config + .mitm_hooks + .iter() + .map(|hook| normalize_host(&hook.host)), + ); + } + Ok(NetworkPolicyMetadata { + enabled: network_enabled && can_route && spec.constraints.enabled != Some(false), + mode: config.mode, + allowed_domains, + denied_domains: Self::merge_domain_lists(denied_domains, &additional_denials), + hard_deny_allowlist_misses: require_ip_literal + || spec.hard_deny_allowlist_misses + || (proxy_active && !can_review), + }) + } + pub(crate) fn with_exec_policy_network_rules( &self, exec_policy: &Policy, ) -> std::io::Result { let mut spec = self.clone(); - apply_exec_policy_network_rules(&mut spec.config, exec_policy); + let (allowed, denied) = exec_policy.compiled_network_domains(); + spec.config.apply_saved_domain_rules(allowed, &denied); + if validate_policy_against_constraints(&spec.config, &spec.constraints).is_ok() { + return Ok(spec); + } + // A stale grant cannot invalidate saved denials. Fixed deny lists retain their configured set. + spec.config.domains.clone_from(&self.config.domains); + spec.config.apply_saved_domain_rules(Vec::new(), &denied); validate_policy_against_constraints(&spec.config, &spec.constraints).map_err(|err| { std::io::Error::new( std::io::ErrorKind::InvalidInput, @@ -540,28 +672,9 @@ impl NetworkProxySpec { } } -fn apply_exec_policy_network_rules(config: &mut NetworkProxyConfig, exec_policy: &Policy) { - let (allowed_domains, denied_domains) = exec_policy.compiled_network_domains(); - upsert_network_domains(config, allowed_domains, /*allow*/ true); - upsert_network_domains(config, denied_domains, /*allow*/ false); -} - -fn upsert_network_domains(config: &mut NetworkProxyConfig, hosts: Vec, allow: bool) { - let mut incoming = HashSet::new(); - for host in hosts { - if incoming.insert(host.clone()) { - config.upsert_domain_permission( - host, - if allow { - codex_network_proxy::NetworkDomainPermission::Allow - } else { - codex_network_proxy::NetworkDomainPermission::Deny - }, - normalize_host, - ); - } - } -} +#[cfg(test)] +#[path = "mcp_network_policy_tests.rs"] +mod mcp_network_policy_tests; #[cfg(test)] #[path = "network_proxy_spec_tests.rs"] 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 d015fb6c40..fa179e0a89 100644 --- a/codex-rs/core/src/config/network_proxy_spec_tests.rs +++ b/codex-rs/core/src/config/network_proxy_spec_tests.rs @@ -778,6 +778,7 @@ async fn environment_local_binding_preserves_explicit_denials_and_inherits_omitt allowed_domains = ["example.com"] "#, )?; + let empty = Policy::empty(); for (name, configured, required, expected_bindings) in [ ( "absent", @@ -883,21 +884,60 @@ async fn environment_local_binding_preserves_explicit_denials_and_inherits_omitt "{name}: {local_binding_policy:?}, owner={owner_binding:?}" ); let carrier = controller.as_ref().unwrap_or(&composed); + // Exercise both directions of a mixed-executor route when the controller + // leaves binding unspecified. An explicit denial cannot start an MXC proxy. + let (primary_os, primary_binding) = if local_binding_policy == DefaultFalse + && carrier.config.allow_local_binding.is_none() + { + (Platform::Windows, RequireTrue) + } else { + (Platform::Linux, DefaultFalse) + }; let proxy = NetworkProxy::builder() .state(Arc::new(carrier.build_state_with_audit_metadata( Default::default(), - Platform::Linux, - DefaultFalse, + primary_os, + primary_binding, )?)) .managed_by_codex(/*managed_by_codex*/ false) .build() .await?; - for policy in [owner, composed.environment_policy()] { + for policy in [Some(owner), Some(composed.environment_policy()), None] { + if policy.is_none() && (controller.is_none() || owner_binding.is_some()) { + continue; + } + if let Some(controller) = &controller { + let mcp = NetworkProxySpec::mcp_network_policy( + Some(controller), + &profile, + McpNetworkRoute { + primary_allows_proxy: true, + local_binding: local_binding_policy, + owner: policy.as_ref(), + }, + &profile, + AskForApproval::OnRequest, + &empty, + )?; + assert_eq!( + mcp.enabled, + expected || local_binding_policy == DefaultFalse + ); + if policy.is_some() || name == "managed_omitted" { + assert_eq!( + mcp.allowed_domains.iter().any(|host| host == "example.com"), + expected + ); + } + if !expected { + assert!(mcp.hard_deny_allowlist_misses); + } + } let scoped = proxy.for_execution( "remote", "binding-test", "binding-token".to_string(), - Some(policy), + policy, Some(Arc::new(|_request| async { NetworkDecision::Allow })), )?; let launch = scoped diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index 02b51f259c..fce3479f90 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -12,14 +12,17 @@ use codex_execpolicy::Decision; use codex_execpolicy::Error as ExecPolicyRuleError; use codex_execpolicy::Evaluation; use codex_execpolicy::MatchOptions; -use codex_execpolicy::NetworkRuleProtocol; use codex_execpolicy::Policy; use codex_execpolicy::PolicyParser; use codex_execpolicy::RequirementsExecPolicy; use codex_execpolicy::RuleMatch; use codex_execpolicy::blocking_append_allow_prefix_rule; use codex_execpolicy::blocking_append_network_rule; +use codex_network_proxy::NetworkProxy; use codex_protocol::approvals::ExecPolicyAmendment; +use codex_protocol::approvals::NetworkApprovalContext; +use codex_protocol::approvals::NetworkPolicyAmendment; +use codex_protocol::approvals::NetworkPolicyRuleAction; use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::models::PermissionProfile; use codex_protocol::permissions::FileSystemSandboxKind; @@ -34,6 +37,8 @@ use tokio::task::spawn_blocking; use tracing::instrument; use crate::config::Config; +use crate::network_policy_decision::ExecPolicyNetworkRuleAmendment; +use crate::network_policy_decision::execpolicy_network_rule_amendment; use crate::sandboxing::SandboxPermissions; use crate::tools::sandboxing::ExecApprovalRequirement; use codex_shell_command::bash::parse_shell_lc_plain_commands; @@ -558,14 +563,16 @@ impl ExecPolicyManager { Ok(()) } - pub(crate) async fn append_network_rule_and_update( + /// Keeps the policy used by subsequent calls in step with native enforcement and persistence. + pub(crate) async fn apply_network_rule_and_persist( &self, codex_home: &Path, host: &str, - protocol: NetworkRuleProtocol, - decision: Decision, - justification: Option, - ) -> Result<(), ExecPolicyUpdateError> { + amendment: &NetworkPolicyAmendment, + approval_context: &NetworkApprovalContext, + proxy: Option<&NetworkProxy>, + on_policy_applied: impl FnOnce(), + ) -> anyhow::Result<()> { let _update_guard = self.update_lock .acquire() @@ -575,19 +582,48 @@ impl ExecPolicyManager { "exec policy update semaphore closed".to_string(), ), })?; + let ExecPolicyNetworkRuleAmendment { + protocol, + decision, + justification, + } = execpolicy_network_rule_amendment(amendment, approval_context, host); + let mut updated_policy = self.current().as_ref().clone(); + updated_policy.add_network_rule(host, protocol, decision, Some(justification.clone()))?; + let updated_policy = Arc::new(updated_policy); + if let Some(proxy) = proxy { + match amendment.action { + NetworkPolicyRuleAction::Allow => proxy + .add_allowed_domain(host) + .await + .map_err(|err| anyhow::anyhow!("failed to update runtime allowlist: {err}"))?, + NetworkPolicyRuleAction::Deny => proxy + .add_denied_domain(host) + .await + .map_err(|err| anyhow::anyhow!("failed to update runtime denylist: {err}"))?, + } + } + // Explicit denials also apply to owner-routed calls without a shared proxy. Publish + // before the cancellable write. Grants become available to other calls only after a save. + if decision == Decision::Forbidden { + self.policy.store(Arc::clone(&updated_policy)); + } + let notify_after_save = if proxy.is_some() { + on_policy_applied(); + None + } else { + Some(on_policy_applied) + }; let policy_path = default_policy_path(codex_home); - let host = host.to_string(); spawn_blocking({ let policy_path = policy_path.clone(); - let host = host.clone(); - let justification = justification.clone(); + let host = host.to_owned(); move || { blocking_append_network_rule( &policy_path, &host, protocol, decision, - justification.as_deref(), + Some(&justification), ) } }) @@ -598,9 +634,10 @@ impl ExecPolicyManager { source, })?; - let mut updated_policy = self.current().as_ref().clone(); - updated_policy.add_network_rule(&host, protocol, decision, justification)?; - self.policy.store(Arc::new(updated_policy)); + self.policy.store(updated_policy); + if let Some(notify) = notify_after_save { + notify(); + } Ok(()) } } diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 829f313e96..570fda86dc 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -324,7 +324,6 @@ use crate::guardian::GuardianReviewSessionManager; use crate::mcp::McpEnvironmentScope; use crate::mcp::McpManager; use crate::mcp::McpThreadIdentity; -use crate::network_policy_decision::execpolicy_network_rule_amendment; use crate::rollout::map_session_init_error; use crate::session::startup_prewarm::SessionStartupPrewarmHandle; use crate::shell; @@ -3184,49 +3183,19 @@ impl Session { .session_configuration .codex_home() .clone(); - let execpolicy_amendment = - execpolicy_network_rule_amendment(amendment, network_approval_context, &host); - let mut on_policy_applied = Some(on_policy_applied); - - if let Some(started_network_proxy) = self.services.network_proxy.load_full() { - let proxy = started_network_proxy.proxy(); - match amendment.action { - NetworkPolicyRuleAction::Allow => proxy - .add_allowed_domain(&host) - .await - .map_err(|err| anyhow::anyhow!("failed to update runtime allowlist: {err}"))?, - NetworkPolicyRuleAction::Deny => proxy - .add_denied_domain(&host) - .await - .map_err(|err| anyhow::anyhow!("failed to update runtime denylist: {err}"))?, - } - // Active enforcement changed successfully. Notify the owner before - // the next fallible await so cancellation cannot contradict it. - if let Some(on_policy_applied) = on_policy_applied.take() { - on_policy_applied(); - } - } - + let started_proxy = self.services.network_proxy.load_full(); + let proxy = started_proxy.as_deref().map(StartedNetworkProxy::proxy); self.services .exec_policy - .append_network_rule_and_update( + .apply_network_rule_and_persist( &codex_home, &host, - execpolicy_amendment.protocol, - execpolicy_amendment.decision, - Some(execpolicy_amendment.justification), + amendment, + network_approval_context, + proxy.as_ref(), + on_policy_applied, ) .await - .map_err(|err| { - anyhow::anyhow!("failed to persist network policy amendment to execpolicy: {err}") - })?; - - // Without a running proxy, persistence is the first effective policy change. - if let Some(on_policy_applied) = on_policy_applied { - on_policy_applied(); - } - - Ok(()) } fn validated_network_policy_amendment_host( diff --git a/codex-rs/core/src/session/network_policy_tests.rs b/codex-rs/core/src/session/network_policy_tests.rs new file mode 100644 index 0000000000..1cd31e405e --- /dev/null +++ b/codex-rs/core/src/session/network_policy_tests.rs @@ -0,0 +1,188 @@ +#![cfg(test)] + +//! Compares session network enforcement with the policy sent to MCP servers. + +use super::*; +use pretty_assertions::assert_eq; +use test_case::test_case; + +#[test_case("1.1.1.1", "8.8.8.8", Decision::Allow; "stale grant")] +#[test_case("1.1.1.1.", "8.8.8.8", Decision::Allow; "equivalent managed spelling")] +#[test_case("1.1.1.1", "bad{host}", Decision::Forbidden; "malformed denial")] +#[tokio::test] +async fn stale_saved_grant_keeps_valid_denial_on_native_proxy_and_mcp( + configured: &str, + host: &str, + decision: Decision, +) -> anyhow::Result<()> { + let permission_profile = PermissionProfile::workspace_write(); + let spec = crate::config::NetworkProxySpec::from_config_and_constraints( + NetworkProxyConfig::default(), + Some(NetworkConstraints { + enabled: Some(true), + domains: Some(NetworkDomainPermissionsToml { + entries: std::collections::BTreeMap::from([( + configured.to_string(), + NetworkDomainPermissionToml::Allow, + )]), + }), + managed_allowed_domains_only: Some(true), + ..Default::default() + }), + &permission_profile, + )?; + let mut exec_policy = Policy::empty(); + exec_policy.add_network_rule( + host, + NetworkRuleProtocol::Https, + decision, + /*justification*/ None, + )?; + exec_policy.add_network_rule( + "1.1.1.1", + NetworkRuleProtocol::Https, + Decision::Forbidden, + /*justification*/ None, + )?; + + let (started_proxy, _) = Session::start_managed_network_proxy( + &spec, + &exec_policy, + &permission_profile, + SandboxType::None, + /*network_policy_decider*/ None, + /*blocked_request_observer*/ None, + /*managed_network_requirements_enabled*/ false, + crate::config::NetworkProxyAuditMetadata::default(), + ) + .await?; + + let native = started_proxy.proxy().current_cfg().await?; + let mcp = crate::config::NetworkProxySpec::mcp_network_policy( + Some(&spec), + &permission_profile, + crate::config::McpNetworkRoute { + primary_allows_proxy: true, + local_binding: codex_network_proxy::LocalBindingPolicy::DefaultFalse, + owner: None, + }, + &permission_profile, + AskForApproval::OnRequest, + &exec_policy, + )?; + let native_domains = ( + native.allowed_domains().unwrap_or_default(), + native.denied_domains().unwrap_or_default(), + ); + assert_eq!(native_domains.1, ["1.1.1.1"]); + assert!(native_domains.0.iter().all(|host| host == configured)); + assert!(mcp.enabled && mcp.hard_deny_allowlist_misses); + assert_eq!((mcp.allowed_domains, mcp.denied_domains), native_domains); + Ok(()) +} + +#[test_case(true; "shared proxy")] +#[test_case(false; "owner proxy without controller")] +#[tokio::test] +async fn failed_network_rule_write_keeps_native_denial_in_mcp_policy( + shared_proxy: bool, +) -> anyhow::Result<()> { + const DENIED: &str = "denied.example"; + const GRANTED: &str = "granted.example"; + let home = tempfile::tempdir()?; + let profile = PermissionProfile::workspace_write(); + let mut network = NetworkProxyConfig { + enabled: true, + allow_local_binding: Some(true), + ..Default::default() + }; + network.set_allowed_domains(vec![DENIED.to_owned()]); + let spec = crate::config::NetworkProxySpec::from_config_and_constraints( + network, /*requirements*/ None, &profile, + )?; + let controller = shared_proxy.then_some(&spec); + let owner = (!shared_proxy).then(|| spec.environment_policy()); + let binding = codex_network_proxy::LocalBindingPolicy::DefaultFalse; + let session = make_session_with_config(|config| { + config.codex_home = home.path().abs(); + config + .permissions + .set_permission_profile(profile.clone()) + .expect("set profile"); + config.permissions.network = controller.cloned(); + }) + .await?; + let policy_file = crate::exec_policy::default_policy_path(home.path()); + std::fs::create_dir_all(&policy_file)?; + for (host, action) in [ + (GRANTED, NetworkPolicyRuleAction::Allow), + (DENIED, NetworkPolicyRuleAction::Deny), + ] { + let mut at_apply = None; + let error = session + .persist_network_policy_amendment( + &NetworkPolicyAmendment { + host: host.to_owned(), + action, + }, + &NetworkApprovalContext { + host: host.to_owned(), + protocol: NetworkApprovalProtocol::Https, + }, + || at_apply = Some(session.services.exec_policy.current()), + ) + .await + .expect_err("a directory cannot be opened for appending a rule"); + assert!( + matches!(error.downcast_ref::(), + Some(ExecPolicyUpdateError::AppendRule { path, .. }) if path == &policy_file + ), + "{error}" + ); + // A shared proxy notifies at installation. With no shared proxy a failed save never notifies. + let denied = match action { + NetworkPolicyRuleAction::Allow => vec![], + NetworkPolicyRuleAction::Deny => vec![host.to_owned()], + }; + assert_eq!( + at_apply.map(|policy| policy.compiled_network_domains()), + shared_proxy.then_some((Vec::new(), denied)) + ); + } + let policy = session.services.exec_policy.current(); + let native_domains = if let Some(proxy) = session.services.network_proxy.load_full() { + proxy.proxy().current_cfg().await?.domains + } else { + crate::config::NetworkProxySpec::for_environment( + controller, + owner.as_ref().expect("owner policy"), + &profile, + &policy, + binding, + )? + .environment_policy() + .domains + }; + let mcp = crate::config::NetworkProxySpec::mcp_network_policy( + controller, + &profile, + crate::config::McpNetworkRoute { + primary_allows_proxy: shared_proxy, + local_binding: binding, + owner: owner.as_ref(), + }, + &profile, + AskForApproval::OnRequest, + &policy, + )?; + let mut expected = json!({ (DENIED): "deny" }); + if shared_proxy { + expected[GRANTED] = json!("allow"); + } + assert_eq!(serde_json::to_value(native_domains)?, expected); + assert_eq!( + (mcp.allowed_domains, mcp.denied_domains), + (vec![], vec![DENIED.to_owned()]) + ); + Ok(()) +} diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index e067b0375f..e1e60bdfd7 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -3,6 +3,9 @@ use codex_protocol::ContextWindowId; #[path = "compaction_replay_tests.rs"] mod compaction_replay_tests; +#[path = "network_policy_tests.rs"] +mod network_policy_tests; + #[path = "notification_tests.rs"] mod notification_tests; @@ -1258,52 +1261,6 @@ async fn start_managed_network_proxy_applies_execpolicy_network_rules() -> anyho Ok(()) } -#[tokio::test] -async fn start_managed_network_proxy_ignores_invalid_execpolicy_network_rules() -> anyhow::Result<()> -{ - let permission_profile = PermissionProfile::workspace_write(); - let spec = crate::config::NetworkProxySpec::from_config_and_constraints( - NetworkProxyConfig::default(), - Some(NetworkConstraints { - domains: Some(NetworkDomainPermissionsToml { - entries: std::collections::BTreeMap::from([( - "managed.example.com".to_string(), - NetworkDomainPermissionToml::Allow, - )]), - }), - managed_allowed_domains_only: Some(true), - ..Default::default() - }), - &permission_profile, - )?; - let mut exec_policy = Policy::empty(); - exec_policy.add_network_rule( - "example.com", - NetworkRuleProtocol::Https, - Decision::Allow, - /*justification*/ None, - )?; - - let (started_proxy, _) = Session::start_managed_network_proxy( - &spec, - &exec_policy, - &permission_profile, - SandboxType::None, - /*network_policy_decider*/ None, - /*blocked_request_observer*/ None, - /*managed_network_requirements_enabled*/ false, - crate::config::NetworkProxyAuditMetadata::default(), - ) - .await?; - - let current_cfg = started_proxy.proxy().current_cfg().await?; - assert_eq!( - current_cfg.allowed_domains(), - Some(vec!["managed.example.com".to_string()]) - ); - Ok(()) -} - #[tokio::test] async fn managed_network_proxy_decider_survives_full_access_start() -> anyhow::Result<()> { let full_access_permission_profile = PermissionProfile::Disabled; diff --git a/codex-rs/core/tests/common/lib.rs b/codex-rs/core/tests/common/lib.rs index babc3ecf42..7aaf3d3f45 100644 --- a/codex-rs/core/tests/common/lib.rs +++ b/codex-rs/core/tests/common/lib.rs @@ -30,6 +30,7 @@ pub mod code_mode_host; pub mod context_snapshot; pub mod exec_server; pub mod hooks; +pub mod network_policy_mcp; pub mod oauth; pub mod process; pub mod responses; diff --git a/codex-rs/core/tests/common/network_policy_mcp.rs b/codex-rs/core/tests/common/network_policy_mcp.rs new file mode 100644 index 0000000000..641300e97f --- /dev/null +++ b/codex-rs/core/tests/common/network_policy_mcp.rs @@ -0,0 +1,183 @@ +//! Mocks one trusted MCP HTTP endpoint while forwarding real executor RPCs unchanged. + +use crate::exec_server::ExecServerProcess; +use crate::is_remote_test_environment; +use anyhow::Context; +use codex_config::types::McpServerConfig; +use codex_exec_server::HttpHeader; +use codex_exec_server::HttpRequestParams; +use codex_exec_server::HttpRequestResponse; +use codex_exec_server::REMOTE_ENVIRONMENT_ID; +use futures::SinkExt; +use futures::StreamExt; +use serde_json::Value; +use serde_json::json; +use std::sync::Arc; +use tokio::net::TcpListener; +use tokio::task::JoinHandle; +use tokio::task::JoinSet; +use tokio_tungstenite::accept_async; +use tokio_tungstenite::connect_async; +use tokio_tungstenite::tungstenite::Message; +use tokio_tungstenite::tungstenite::http::Uri; + +pub const TRUSTED_MCP_URL: &str = "https://chatgpt.com/backend-api/ps/mcp"; + +pub struct NetworkPolicyMcp { + pub executor_url: String, + task: JoinHandle>, + _executor: Option, +} + +impl NetworkPolicyMcp { + pub fn server_config() -> McpServerConfig { + serde_json::from_value(json!({ + "url": TRUSTED_MCP_URL, + "environment_id": REMOTE_ENVIRONMENT_ID, + })) + .expect("trusted network policy MCP configuration") + } + + pub async fn start_with_capabilities(experimental: Value) -> anyhow::Result { + let experimental = Arc::new(experimental); + let executor = if is_remote_test_environment() { + None + } else { + Some(ExecServerProcess::start().await?) + }; + let upstream_url = match &executor { + Some(executor) => executor.websocket_url.clone(), + None => std::env::var("CODEX_TEST_REMOTE_EXEC_SERVER_URL")?, + }; + let listener = TcpListener::bind("127.0.0.1:0").await?; + let executor_url = format!("ws://{}", listener.local_addr()?); + let task = tokio::spawn(async move { + let mut sessions = JoinSet::new(); + loop { + tokio::select! { + accepted = listener.accept() => { + let (stream, _) = accepted?; + let upstream_url = upstream_url.clone(); + let experimental = Arc::clone(&experimental); + sessions.spawn(async move { + let mut downstream = accept_async(stream).await?; + let (mut upstream, _) = connect_async(upstream_url).await?; + let mut responses = JoinSet::new(); + loop { + tokio::select! { + frame = downstream.next() => { + let Some(frame) = frame else { break; }; + let frame = frame?; + if frame.is_close() { break; } + let request = match &frame { + Message::Text(text) => serde_json::from_str::(text).ok(), + Message::Binary(bytes) => serde_json::from_slice(bytes).ok(), + _ => None, + }; + if let Some(request) = request + && request["method"] == "http/request" + { + let url: Uri = request["params"]["url"].as_str().context("HTTP request URL")?.parse()?; + anyhow::ensure!( + url.host().is_none_or(|host| !host.eq_ignore_ascii_case("chatgpt.com")) + || request["params"]["url"] == TRUSTED_MCP_URL, + "unsupported request to ChatGPT fixture origin: {url}" + ); + if request["params"]["url"] == TRUSTED_MCP_URL { + responses.spawn(respond(request, Arc::clone(&experimental))); + continue; + } + } + upstream.send(frame).await?; + } + frame = upstream.next() => { + let Some(frame) = frame else { break; }; + let frame = frame?; + if frame.is_close() { break; } + downstream.send(frame).await?; + } + Some(response) = responses.join_next(), if !responses.is_empty() => { + for message in response?? { + downstream.send(Message::Text(message.to_string().into())).await?; + } + } + else => break, + } + } + anyhow::Ok(()) + }); + } + Some(session) = sessions.join_next(), if !sessions.is_empty() => session??, + } + } + }); + Ok(Self { + executor_url, + task, + _executor: executor, + }) + } +} + +impl Drop for NetworkPolicyMcp { + fn drop(&mut self) { + self.task.abort(); + } +} + +async fn respond(request: Value, experimental: Arc) -> anyhow::Result> { + let params: HttpRequestParams = serde_json::from_value(request["params"].clone())?; + anyhow::ensure!(params.url == TRUSTED_MCP_URL, "unexpected MCP endpoint"); + let mut response = HttpRequestResponse { + status: 405, + headers: Vec::new(), + body: Vec::new().into(), + }; + if params.method == "POST" { + let body: Value = + serde_json::from_slice(params.body.as_ref().context("MCP body")?.as_ref())?; + let result = match body["method"].as_str() { + Some("initialize") => json!({ + "protocolVersion": body["params"]["protocolVersion"], + "capabilities": {"tools": {}, "experimental": *experimental}, + "serverInfo": {"name": "network-policy", "version": "1"} + }), + Some("tools/list") => json!({"tools": [{ + "name": "network_policy_meta", "inputSchema": {"type": "object"}, + "annotations": {"readOnlyHint": true} + }]}), + Some("tools/call") => { + anyhow::ensure!( + body["params"]["name"] == "network_policy_meta", + "unknown MCP tool" + ); + let metadata = body["params"]["_meta"].clone(); + json!({"content": [{"type": "text", "text": metadata.to_string()}], + "structuredContent": metadata, "isError": false}) + } + Some("notifications/initialized") => Value::Null, + method => anyhow::bail!("unexpected MCP method {method:?}"), + }; + response.status = if body.get("id").is_some() { 200 } else { 202 }; + response.headers = vec![HttpHeader { + name: "content-type".to_string(), + value: "application/json".to_string(), + value_env_var: None, + }]; + if response.status == 200 { + response.body = + serde_json::to_vec(&json!({"jsonrpc": "2.0", "id": body["id"], "result": result}))? + .into(); + } + } + let body = params + .stream_response + .then(|| std::mem::replace(&mut response.body, Vec::new().into())); + let mut messages = vec![json!({"id": request["id"], "result": response})]; + if let Some(body) = body { + messages.push(json!({"method": "http/request/bodyDelta", "params": { + "requestId": params.request_id, "seq": 1, "deltaBase64": body, "done": true + }})); + } + Ok(messages) +} diff --git a/codex-rs/core/tests/suite/mcp_network_policy_compat_tests.rs b/codex-rs/core/tests/suite/mcp_network_policy_compat_tests.rs new file mode 100644 index 0000000000..2a54e9cdca --- /dev/null +++ b/codex-rs/core/tests/suite/mcp_network_policy_compat_tests.rs @@ -0,0 +1,46 @@ +//! Compatibility for trusted MCP servers that predate network policy negotiation. + +use anyhow::Context; +use codex_config::test_support::CloudConfigBundleFixture; +use codex_mcp::MCP_NETWORK_POLICY_META_CAPABILITY; +use core_test_support::network_policy_mcp::NetworkPolicyMcp; +use core_test_support::responses::start_mock_server; +use core_test_support::skip_if_no_network; +use core_test_support::test_codex::test_codex; +use pretty_assertions::assert_eq; +use serde_json::json; + +#[tokio::test] +async fn trusted_server_without_policy_support_keeps_caller_metadata() -> anyhow::Result<()> { + skip_if_no_network!(Ok(())); + let server = start_mock_server().await; + let mcp = NetworkPolicyMcp::start_with_capabilities(json!({})).await?; + let requirements = CloudConfigBundleFixture::loader_with_enterprise_requirement( + "[experimental_network]\nenabled = false\ndomains = { 'blocked.example' = 'deny' }", + ); + let fixture = test_codex() + .with_exec_server_url(mcp.executor_url.clone()) + .with_cloud_config_bundle(requirements) + .with_config(|config| { + let servers = [("legacy".into(), NetworkPolicyMcp::server_config())]; + config + .mcp_servers + .set(servers.into()) + .expect("legacy MCP configuration"); + }) + .build_with_remote_and_local_env(&server) + .await?; + let result = fixture + .codex + .call_mcp_tool( + "legacy", + "network_policy_meta", + /*arguments*/ None, + Some(json!({"caller": "keep", "codex/network-policy-meta": {"enabled": true}})), + ) + .await?; + let meta = result.structured_content.context("echoed call metadata")?; + assert_eq!(meta["caller"], "keep"); + assert!(meta.get(MCP_NETWORK_POLICY_META_CAPABILITY).is_none()); + Ok(()) +} diff --git a/codex-rs/core/tests/suite/rmcp_client.rs b/codex-rs/core/tests/suite/rmcp_client.rs index dec59fddbd..45c13ab02b 100644 --- a/codex-rs/core/tests/suite/rmcp_client.rs +++ b/codex-rs/core/tests/suite/rmcp_client.rs @@ -137,6 +137,8 @@ use tokio::time::Instant; use tokio::time::sleep; use wiremock::MockServer; +#[path = "mcp_network_policy_compat_tests.rs"] +mod network_policy_compat_tests; #[path = "mcp_oauth_refresh_tests.rs"] mod oauth_refresh_tests; #[path = "mcp_sandbox_tests.rs"] diff --git a/codex-rs/network-proxy/src/config.rs b/codex-rs/network-proxy/src/config.rs index 2db5eb7358..2922fde98b 100644 --- a/codex-rs/network-proxy/src/config.rs +++ b/codex-rs/network-proxy/src/config.rs @@ -323,6 +323,30 @@ impl NetworkProxyConfig { self.set_domain_entries(denied_domains, NetworkDomainPermission::Deny); } + /// Applies valid saved denials even when a malformed batch invalidates its grants. + pub fn apply_saved_domain_rules(&mut self, allowed: Vec, denied: &[String]) { + let valid_denials = crate::policy::compile_denylist(denied).is_ok(); + if valid_denials && crate::policy::compile_allowlist(&allowed).is_ok() { + for host in allowed { + self.upsert_domain_permission(host, NetworkDomainPermission::Allow, normalize_host); + } + } + for host in denied { + if !valid_denials + && crate::policy::compile_denylist(std::slice::from_ref(host)).is_err() + { + continue; + } + // Managed constraints compare raw selectors case-insensitively. Preserve other aliases; + // the native and MCP domain matchers give this denial precedence over all of them. + self.upsert_domain_permission( + host.clone(), + NetworkDomainPermission::Deny, + str::to_ascii_lowercase, + ); + } + } + pub fn upsert_domain_permission( &mut self, host: String, diff --git a/codex-rs/network-proxy/src/proxy.rs b/codex-rs/network-proxy/src/proxy.rs index 5b1240ae10..6c7f1c5d36 100644 --- a/codex-rs/network-proxy/src/proxy.rs +++ b/codex-rs/network-proxy/src/proxy.rs @@ -1595,28 +1595,37 @@ impl NetworkProxy { Ok(addrs) } - pub async fn replace_config_state(&self, new_state: ConfigState) -> Result<()> { - let current_cfg = self.state.current_cfg().await?; + /// Verifies that an in-place reload preserves the running proxy's listeners. + pub fn validate_config_update( + current_cfg: &config::NetworkProxyConfig, + next: &config::NetworkProxyConfig, + ) -> Result<()> { anyhow::ensure!( - new_state.config.enabled == current_cfg.enabled, + next.enabled == current_cfg.enabled, "cannot update network.enabled on a running proxy" ); anyhow::ensure!( - new_state.config.proxy_url == current_cfg.proxy_url, + next.proxy_url == current_cfg.proxy_url, "cannot update network.proxy_url on a running proxy" ); anyhow::ensure!( - new_state.config.socks_url == current_cfg.socks_url, + next.socks_url == current_cfg.socks_url, "cannot update network.socks_url on a running proxy" ); anyhow::ensure!( - new_state.config.enable_socks5 == current_cfg.enable_socks5, + next.enable_socks5 == current_cfg.enable_socks5, "cannot update network.enable_socks5 on a running proxy" ); anyhow::ensure!( - new_state.config.enable_socks5_udp == current_cfg.enable_socks5_udp, + next.enable_socks5_udp == current_cfg.enable_socks5_udp, "cannot update network.enable_socks5_udp on a running proxy" ); + Ok(()) + } + + pub async fn replace_config_state(&self, new_state: ConfigState) -> Result<()> { + let current_cfg = self.state.current_cfg().await?; + Self::validate_config_update(¤t_cfg, &new_state.config)?; let config = self .state .local_binding_policy