diff --git a/codex-rs/cli/src/cloud_config.rs b/codex-rs/cli/src/cloud_config.rs index 5e87d12b0d..9dd74aae8d 100644 --- a/codex-rs/cli/src/cloud_config.rs +++ b/codex-rs/cli/src/cloud_config.rs @@ -1,5 +1,6 @@ use anyhow::Context; use anyhow::Result; +use codex_app_server::in_process::EmbeddedNetworkPolicy; use codex_cloud_config::cloud_config_bundle_loader_for_storage; use codex_config::CloudConfigBundleLoader; use codex_config::ConfigLoadOptions; @@ -17,7 +18,7 @@ pub(crate) async fn load_config( config_overrides: &CliConfigOverrides, loader_overrides: LoaderOverrides, ) -> Result { - config_builder( + let mut config = config_builder( config_overrides, loader_overrides, ConfigOverrides::default(), @@ -25,7 +26,9 @@ pub(crate) async fn load_config( .await? .build() .await - .context("failed to load configuration") + .context("failed to load configuration")?; + EmbeddedNetworkPolicy::default().activate(&mut config); + Ok(config) } pub(crate) async fn config_builder( @@ -33,6 +36,7 @@ pub(crate) async fn config_builder( loader_overrides: LoaderOverrides, harness_overrides: ConfigOverrides, ) -> Result { + let network_policy = EmbeddedNetworkPolicy::load(&loader_overrides).await; let cli_overrides = config_overrides .parse_overrides() .map_err(anyhow::Error::msg)?; @@ -55,8 +59,10 @@ pub(crate) async fn config_builder( .await .context("failed to load bootstrap configuration")?; let cloud_config_bundle = cloud_config_bundle_loader_for_storage( - bootstrap_auth_config(codex_home.as_path(), &bootstrap_config) - .context("failed to resolve cloud configuration authentication")?, + network_policy.bind_bootstrap_auth( + bootstrap_auth_config(codex_home.as_path(), &bootstrap_config) + .context("failed to resolve cloud configuration authentication")?, + ), /*enable_codex_api_key_env*/ false, ) .await diff --git a/codex-rs/cli/src/mcp_cmd.rs b/codex-rs/cli/src/mcp_cmd.rs index ac4b0154b3..02eb45c3e4 100644 --- a/codex-rs/cli/src/mcp_cmd.rs +++ b/codex-rs/cli/src/mcp_cmd.rs @@ -258,7 +258,8 @@ impl McpCli { run_get(&config, args).await?; } McpSubcommand::Add(args) => { - run_add(&config_overrides, args).await?; + let config = cloud_config::load_config(&config_overrides, loader_overrides).await?; + run_add(&config, args).await?; } McpSubcommand::Remove(args) => { run_remove(&config_overrides, args).await?; @@ -293,15 +294,7 @@ async fn validate_profile_v2_migration( Ok(()) } -async fn run_add(config_overrides: &CliConfigOverrides, add_args: AddArgs) -> Result<()> { - // Validate any provided overrides even though they are not currently applied. - let overrides = config_overrides - .parse_overrides() - .map_err(anyhow::Error::msg)?; - let config = Config::load_with_cli_overrides(overrides) - .await - .context("failed to load configuration")?; - +async fn run_add(config: &Config, add_args: AddArgs) -> Result<()> { let AddArgs { name, transport_args, @@ -309,7 +302,7 @@ async fn run_add(config_overrides: &CliConfigOverrides, add_args: AddArgs) -> Re validate_server_name(&name)?; - let codex_home = find_codex_home().context("failed to resolve CODEX_HOME")?; + let codex_home = &config.codex_home; let (transport, oauth_client_id, oauth_client_secret, client_registration, oauth_resource) = match transport_args { @@ -451,13 +444,13 @@ async fn run_add(config_overrides: &CliConfigOverrides, add_args: AddArgs) -> Re tools: HashMap::new(), }; - let mut servers = load_global_mcp_servers(&codex_home) + let mut servers = load_global_mcp_servers(codex_home) .await .with_context(|| format!("failed to load MCP servers from {}", codex_home.display()))?; let credential_name = new_entry.oauth_credential_name(&name); servers.insert(name.clone(), new_entry); - ConfigEditsBuilder::new(&codex_home) + ConfigEditsBuilder::new(codex_home) .replace_mcp_servers(&servers) .apply() .await diff --git a/codex-rs/cli/src/mcp_cmd_tests.rs b/codex-rs/cli/src/mcp_cmd_tests.rs index 98ebe6a709..c968695f39 100644 --- a/codex-rs/cli/src/mcp_cmd_tests.rs +++ b/codex-rs/cli/src/mcp_cmd_tests.rs @@ -1,7 +1,14 @@ -//! Verifies MCP client-secret argument requirements and redacted CLI diagnostics. +//! Verifies MCP argument handling and managed networking in standalone commands. use clap::Parser; +use codex_http_client::ClientRouteClass; +use codex_http_client::HttpError; +use codex_http_client::NetworkPolicyDenied; use pretty_assertions::assert_eq; +use std::path::PathBuf; +use std::time::Duration; +use tokio::process::Command; +use tokio::time::timeout; use super::McpCli; use super::McpSubcommand; @@ -60,3 +67,280 @@ fn oauth_client_secret_requires_url_and_client_id_without_disclosure() { assert!(!error.to_string().contains("cli-secret-marker")); } } + +async fn isolated_test_home(test_name: &str) -> anyhow::Result> { + const CHILD: &str = "CODEX_MCP_NETWORK_TEST_CHILD"; + let test_name = format!("mcp_cmd::tests::{test_name}"); + if std::env::var(CHILD).as_deref() == Ok(test_name.as_str()) { + return Ok(Some(codex_core::config::find_codex_home()?.to_path_buf())); + } + + // Only the child receives the temporary home; the production loader resolves + // CODEX_HOME normally and the parent test process keeps its environment. + let home = tempfile::tempdir()?; + let output = timeout( + Duration::from_secs(45), + Command::new(std::env::current_exe()?) + .args(["--exact", &test_name, "--nocapture"]) + .kill_on_drop(true) + .env(CHILD, &test_name) + .env("CODEX_HOME", home.path()) + .env("NO_PROXY", "127.0.0.1,localhost") + .env("no_proxy", "127.0.0.1,localhost") + .env_remove("OPENAI_API_KEY") + .env_remove("CODEX_API_KEY") + .env_remove("CODEX_ACCESS_TOKEN") + .current_dir(home.path()) + .output(), + ) + .await??; + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + output.status.success() && stdout.contains("1 passed; 0 failed"), + "{test_name}: {stdout}\n{}", + String::from_utf8_lossy(&output.stderr) + ); + Ok(None) +} + +async fn config_with_application_network( + home: &std::path::Path, + requirements: &str, + extra_config: &str, +) -> anyhow::Result { + std::fs::write( + home.join("config.toml"), + format!( + "forced_login_method = 'api'\n\ + cli_auth_credentials_store = 'file'\n\ + mcp_oauth_credentials_store = 'file'\n\ + [features]\nplugins = false\nrecommended_plugins = false\n\ + {extra_config}" + ), + )?; + let requirements_path = home.join("requirements.toml"); + std::fs::write(&requirements_path, requirements)?; + crate::cloud_config::load_config( + &codex_utils_cli::CliConfigOverrides::default(), + codex_core::config::LoaderOverrides { + system_config_path: Some(home.join("absent-system-config.toml")), + system_requirements_path: Some(requirements_path), + ignore_project_config: true, + ..codex_core::config::LoaderOverrides::without_managed_config_for_tests() + }, + ) + .await +} + +fn http_add_args(url: &str) -> super::AddArgs { + let cli = McpCli::try_parse_from(["mcp", "add", "policy-probe", "--url", url]) + .expect("parse HTTP add arguments"); + let McpSubcommand::Add(args) = cli.subcommand else { + panic!("expected MCP add"); + }; + args +} + +#[tokio::test] +async fn application_network_mcp_add_denies_initial_oauth_discovery_before_connect() +-> anyhow::Result<()> { + let Some(home) = isolated_test_home( + "application_network_mcp_add_denies_initial_oauth_discovery_before_connect", + ) + .await? + else { + return Ok(()); + }; + let listener = std::net::TcpListener::bind("127.0.0.1:0")?; + listener.set_nonblocking(true)?; + let url = format!("https://{}/mcp", listener.local_addr()?); + let config = config_with_application_network(&home, "[application.network]\n", "").await?; + + // Add deliberately saves the server when discovery is unknown. A successful + // command is therefore not evidence that its network request was permitted. + tokio::time::timeout( + std::time::Duration::from_secs(5), + super::run_add(&config, http_add_args(&url)), + ) + .await??; + let saved: toml::Value = toml::from_str(&std::fs::read_to_string(home.join("config.toml"))?)?; + assert_eq!( + saved["mcp_servers"]["policy-probe"]["url"].as_str(), + Some(url.as_str()) + ); + // No accepted TCP connection also proves no HTTP discovery request arrived. + assert_eq!( + listener.accept().unwrap_err().kind(), + std::io::ErrorKind::WouldBlock + ); + Ok(()) +} + +#[tokio::test] +async fn application_network_mcp_add_discovers_when_policy_is_disabled() -> anyhow::Result<()> { + let Some(home) = + isolated_test_home("application_network_mcp_add_discovers_when_policy_is_disabled").await? + else { + return Ok(()); + }; + let server = wiremock::MockServer::start().await; + wiremock::Mock::given(wiremock::matchers::any()) + .respond_with(wiremock::ResponseTemplate::new(404)) + .mount(&server) + .await; + let url = format!("{}/mcp", server.uri()); + let config = + config_with_application_network(&home, "[application.network]\nenabled = false\n", "") + .await?; + tokio::time::timeout( + std::time::Duration::from_secs(5), + super::run_add(&config, http_add_args(&url)), + ) + .await??; + let requests = server.received_requests().await.expect("recorded requests"); + assert!( + requests.iter().any(|request| request.url.path() == "/mcp"), + "the positive control must perform real OAuth discovery" + ); + // The 404 response has no OAuth support, so this cannot open a browser. + Ok(()) +} + +#[tokio::test] +async fn application_network_mcp_login_denies_even_an_allowed_mcp_server() -> anyhow::Result<()> { + let Some(home) = + isolated_test_home("application_network_mcp_login_denies_even_an_allowed_mcp_server") + .await? + else { + return Ok(()); + }; + let listener = std::net::TcpListener::bind("127.0.0.1:0")?; + listener.set_nonblocking(true)?; + let url = format!("https://{}/mcp", listener.local_addr()?); + let requirements = + format!("[application.network]\n[mcp_servers.policy-probe.identity]\nurl = {url:?}\n"); + let extra_config = format!("[mcp_servers.policy-probe]\nurl = {url:?}\n"); + let config = config_with_application_network(&home, &requirements, &extra_config).await?; + let server = &config.mcp_servers.get()["policy-probe"]; + assert!( + server.enabled, + "MCP identity allowlist must permit this server" + ); + assert!(server.disabled_reason.is_none()); + let error = tokio::time::timeout( + std::time::Duration::from_secs(5), + super::run_login( + &config, + super::LoginArgs { + name: "policy-probe".to_string(), + no_browser: true, + scopes: Vec::new(), + oauth_client_registration: None, + }, + ), + ) + .await? + .expect_err("MCP permission must not override application network restrictions"); + let message = format!("{error:#}"); + assert!( + message.contains("destination denied by application network policy"), + "{message}" + ); + assert_eq!( + listener.accept().unwrap_err().kind(), + std::io::ErrorKind::WouldBlock + ); + Ok(()) +} + +#[tokio::test] +async fn application_network_loader_denies_unlisted_destination_before_connect() +-> anyhow::Result<()> { + let Some(home) = + isolated_test_home("application_network_loader_denies_unlisted_destination_before_connect") + .await? + else { + return Ok(()); + }; + let listener = std::net::TcpListener::bind("127.0.0.1:0")?; + listener.set_nonblocking(true)?; + let url = format!("https://{}/must-not-connect", listener.local_addr()?); + let config = config_with_application_network( + &home, + "[application.network.domains]\n'allowed.example' = 'allow'\n", + "", + ) + .await?; + let factory = config.http_client_factory(); + factory + .network_policy() + .acquire(&"https://allowed.example/v1".parse()?)?; + let client = factory.build_client(&url, ClientRouteClass::Other)?; + assert!(matches!( + timeout(Duration::from_secs(2), client.get(&url).send()).await?, + Err(HttpError::Policy(NetworkPolicyDenied::Destination)) + )); + assert_eq!( + listener.accept().unwrap_err().kind(), + std::io::ErrorKind::WouldBlock + ); + Ok(()) +} + +#[tokio::test] +async fn application_network_loader_allows_listed_https_destination_to_connect() +-> anyhow::Result<()> { + let Some(home) = + isolated_test_home("application_network_loader_allows_listed_https_destination_to_connect") + .await? + else { + return Ok(()); + }; + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await?; + let url = format!("https://{}/allowed", listener.local_addr()?); + let config = config_with_application_network( + &home, + "[application.network.domains]\n'127.0.0.1' = 'allow'\n", + "", + ) + .await?; + let client = config + .http_client_factory() + .build_client(&url, ClientRouteClass::Other)?; + let (response, connection) = timeout(Duration::from_secs(2), async { + tokio::join!(client.get(&url).send(), async { + let (stream, _) = listener.accept().await?; + // This listener proves TCP permission; it deliberately does not provide TLS. + drop(stream); + Ok::<_, std::io::Error>(()) + }) + }) + .await?; + connection?; + assert!(response.is_err(), "the raw listener cannot complete TLS"); + assert!(!matches!(response, Err(HttpError::Policy(_)))); + Ok(()) +} + +#[tokio::test] +async fn application_network_loader_rejects_malformed_managed_policy() -> anyhow::Result<()> { + let Some(home) = + isolated_test_home("application_network_loader_rejects_malformed_managed_policy").await? + else { + return Ok(()); + }; + let error = config_with_application_network( + &home, + "[application.network.domains]\n'*.example.com' = 'allow'\n", + "", + ) + .await + .expect_err("invalid application policy must not load as unrestricted"); + let message = format!("{error:#}"); + assert!(message.contains("requirements.toml"), "{message}"); + assert!( + message.contains("application.network.domains requires exact ASCII domain names"), + "{message}" + ); + Ok(()) +}