From b7d2321ef7b2405b72a4186447c810756866896b Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Sat, 7 Nov 2026 02:52:09 +0000 Subject: [PATCH] Derive MCP network policy and preserve saved denials (#60046) ## Why Stale saved grants or malformed rules can cause valid network denials to be discarded. A failed rule write can also leave the in-memory policy inconsistent with native enforcement. ## What changed - Add a helper to derive MCP network policy from controller and environment policies, permission profiles, approval settings, and saved rules. Preserve managed restrictions and conservatively deny traffic that requires review, request hooks, or DNS checks the metadata cannot express. - Retain valid saved denials when grants violate managed constraints or a rule batch is malformed. - Serialize runtime updates and persistence, publishing denials before the write so they remain effective if saving fails. Publish saved grants after a successful write. - Extract validation of listener settings for running proxy updates. ## Testing Add coverage for policy composition, mixed-executor local binding, native proxy denial parity, stale grants, malformed denials, failed persistence, and compatibility with trusted MCP servers without policy negotiation support. GitOrigin-RevId: ef00632acb48a51fc582f18d20a3e713f953a191 --- .../src/config/mcp_network_policy_tests.rs | 326 ++++++++++++++++++ codex-rs/core/src/config/mod.rs | 2 + .../core/src/config/network_proxy_spec.rs | 189 ++++++++-- .../src/config/network_proxy_spec_tests.rs | 48 ++- codex-rs/core/src/exec_policy.rs | 63 +++- codex-rs/core/src/session/mod.rs | 45 +-- .../core/src/session/network_policy_tests.rs | 188 ++++++++++ codex-rs/core/src/session/tests.rs | 49 +-- codex-rs/core/tests/common/lib.rs | 1 + .../core/tests/common/network_policy_mcp.rs | 183 ++++++++++ .../suite/mcp_network_policy_compat_tests.rs | 46 +++ codex-rs/core/tests/suite/rmcp_client.rs | 2 + codex-rs/network-proxy/src/config.rs | 24 ++ codex-rs/network-proxy/src/proxy.rs | 23 +- 14 files changed, 1043 insertions(+), 146 deletions(-) create mode 100644 codex-rs/core/src/config/mcp_network_policy_tests.rs create mode 100644 codex-rs/core/src/session/network_policy_tests.rs create mode 100644 codex-rs/core/tests/common/network_policy_mcp.rs create mode 100644 codex-rs/core/tests/suite/mcp_network_policy_compat_tests.rs 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