diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index 9d9f262ab5..aa84dc6152 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -36,6 +36,8 @@ use codex_protocol::protocol::McpListToolsResponseEvent; use rmcp::model::ReadResourceRequestParams; use rmcp::model::ReadResourceResult; use serde_json::Value; +use url::Host; +use url::Url; use crate::codex_apps::codex_apps_tools_cache_key; use crate::connection_manager::McpConnectionManager; @@ -205,10 +207,15 @@ pub fn with_codex_apps_mcp( config: &McpConfig, ) -> HashMap { if config.apps_enabled && auth.is_some_and(CodexAuth::uses_codex_backend) { - servers.insert( - CODEX_APPS_MCP_SERVER_NAME.to_string(), - codex_apps_mcp_server_config(config), - ); + match codex_apps_mcp_server_config(config) { + Ok(server_config) => { + servers.insert(CODEX_APPS_MCP_SERVER_NAME.to_string(), server_config); + } + Err(err) => { + tracing::warn!("Skipping host-owned Codex Apps MCP server: {err}"); + servers.remove(CODEX_APPS_MCP_SERVER_NAME); + } + } } else { servers.remove(CODEX_APPS_MCP_SERVER_NAME); } @@ -353,11 +360,9 @@ pub async fn collect_mcp_snapshot_from_manager( .await } -pub(crate) fn codex_apps_mcp_url(config: &McpConfig) -> String { - codex_apps_mcp_url_for_base_url( - &config.chatgpt_base_url, - config.apps_mcp_path_override.as_deref(), - ) +#[cfg(test)] +pub(crate) fn codex_apps_mcp_url(config: &McpConfig) -> Result { + host_owned_codex_apps_mcp_endpoint(config).map(|endpoint| endpoint.url.0) } /// The Responses API requires tool names to match `^[a-zA-Z0-9_-]+$`. @@ -389,19 +394,47 @@ fn codex_apps_mcp_bearer_token_env_var() -> Option { } } -fn normalize_codex_apps_base_url(base_url: &str) -> String { - let mut base_url = base_url.trim_end_matches('/').to_string(); - if (base_url.starts_with("https://chatgpt.com") - || base_url.starts_with("https://chat.openai.com")) - && !base_url.contains("/backend-api") +#[derive(Debug, Clone, PartialEq, Eq)] +struct HostOwnedCodexAppsMcpEndpoint { + url: TrustedCodexAppsMcpUrl, + provenance: McpServerProvenance, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +struct TrustedCodexAppsMcpUrl(String); + +fn host_owned_codex_apps_mcp_endpoint( + config: &McpConfig, +) -> Result { + // HostOwnedCodexApps gates first-party connector behavior, including + // privileged file upload handling. Keep the trusted URL check and the + // provenance grant together so a config-derived URL cannot receive the + // host-owned marker without passing this audit point. + let url = codex_apps_mcp_url_for_base_url( + &config.chatgpt_base_url, + config.apps_mcp_path_override.as_deref(), + )?; + Ok(HostOwnedCodexAppsMcpEndpoint { + url, + provenance: McpServerProvenance::HostOwnedCodexApps, + }) +} + +fn codex_apps_mcp_url_for_base_url( + base_url: &str, + apps_mcp_path_override: Option<&str>, +) -> Result { + let base_url = base_url.trim_end_matches('/'); + let parsed_base_url = Url::parse(base_url) + .map_err(|err| format!("invalid Codex Apps MCP base URL `{base_url}`: {err}"))?; + validate_codex_apps_mcp_base_url(&parsed_base_url, base_url)?; + + let mut base_url = base_url.to_string(); + if is_allowed_codex_apps_chatgpt_host(&parsed_base_url.host()) + && !parsed_base_url.path().contains("/backend-api") { base_url = format!("{base_url}/backend-api"); } - base_url -} - -fn codex_apps_mcp_url_for_base_url(base_url: &str, apps_mcp_path_override: Option<&str>) -> String { - let base_url = normalize_codex_apps_base_url(base_url); let (base_url, default_path) = if base_url.contains("/backend-api") { (base_url, "wham/apps") } else if base_url.contains("/api/codex") { @@ -412,13 +445,63 @@ fn codex_apps_mcp_url_for_base_url(base_url: &str, apps_mcp_path_override: Optio let path = apps_mcp_path_override .unwrap_or(default_path) .trim_start_matches('/'); - format!("{base_url}/{path}") + Ok(TrustedCodexAppsMcpUrl(format!("{base_url}/{path}"))) } -fn codex_apps_mcp_server_config(config: &McpConfig) -> McpServerConfig { - let url = codex_apps_mcp_url(config); +fn validate_codex_apps_mcp_base_url(url: &Url, original_base_url: &str) -> Result<(), String> { + if !url.username().is_empty() + || url.password().is_some() + || url.query().is_some() + || url.fragment().is_some() + { + return Err(format!( + "invalid Codex Apps MCP base URL `{original_base_url}`; expected a URL without credentials, query, or fragment" + )); + } - McpServerConfig { + let scheme = url.scheme(); + let host = url.host(); + let valid_first_party_url = + scheme == "https" && url.port().is_none() && is_allowed_codex_apps_chatgpt_host(&host); + let valid_local_url = cfg!(debug_assertions) + && matches!(scheme, "http" | "https") + && is_localhost_for_codex_apps(&host); + if valid_first_party_url || valid_local_url { + Ok(()) + } else { + Err(format!( + "invalid Codex Apps MCP base URL `{original_base_url}`; expected an HTTPS URL for chatgpt.com, chat.openai.com, or chatgpt-staging.com" + )) + } +} + +fn is_allowed_codex_apps_chatgpt_host(host: &Option>) -> bool { + let Some(Host::Domain(host)) = host else { + return false; + }; + let host = *host; + + host == "chatgpt.com" + || host == "chat.openai.com" + || host == "chatgpt-staging.com" + || host.ends_with(".chatgpt.com") + || host.ends_with(".chatgpt-staging.com") +} + +fn is_localhost_for_codex_apps(host: &Option>) -> bool { + match host { + Some(Host::Domain(host)) => *host == "localhost", + Some(Host::Ipv4(ip)) => ip.is_loopback(), + Some(Host::Ipv6(ip)) => ip.is_loopback(), + _ => false, + } +} + +fn codex_apps_mcp_server_config(config: &McpConfig) -> Result { + let endpoint = host_owned_codex_apps_mcp_endpoint(config)?; + let TrustedCodexAppsMcpUrl(url) = endpoint.url; + + Ok(McpServerConfig { transport: McpServerTransportConfig::StreamableHttp { url, bearer_token_env_var: codex_apps_mcp_bearer_token_env_var(), @@ -430,7 +513,7 @@ fn codex_apps_mcp_server_config(config: &McpConfig) -> McpServerConfig { required: false, supports_parallel_tool_calls: false, disabled_reason: None, - provenance: McpServerProvenance::HostOwnedCodexApps, + provenance: endpoint.provenance, startup_timeout_sec: Some(Duration::from_secs(30)), tool_timeout_sec: None, default_tools_approval_mode: None, @@ -439,7 +522,7 @@ fn codex_apps_mcp_server_config(config: &McpConfig) -> McpServerConfig { scopes: None, oauth_resource: None, tools: HashMap::new(), - } + }) } fn protocol_tool_from_rmcp_tool(name: &str, tool: &rmcp::model::Tool) -> Option { diff --git a/codex-rs/codex-mcp/src/mcp/mod_tests.rs b/codex-rs/codex-mcp/src/mcp/mod_tests.rs index f080d10532..d601d10a87 100644 --- a/codex-rs/codex-mcp/src/mcp/mod_tests.rs +++ b/codex-rs/codex-mcp/src/mcp/mod_tests.rs @@ -170,45 +170,73 @@ fn codex_apps_mcp_url_for_base_url_keeps_existing_paths() { codex_apps_mcp_url_for_base_url( "https://chatgpt.com/backend-api", /*apps_mcp_path_override*/ None, - ), - "https://chatgpt.com/backend-api/wham/apps" + ) + .expect("trusted ChatGPT URL should build"), + TrustedCodexAppsMcpUrl("https://chatgpt.com/backend-api/wham/apps".to_string()) ); assert_eq!( codex_apps_mcp_url_for_base_url( "https://chat.openai.com", /*apps_mcp_path_override*/ None, - ), - "https://chat.openai.com/backend-api/wham/apps" + ) + .expect("trusted legacy ChatGPT URL should build"), + TrustedCodexAppsMcpUrl("https://chat.openai.com/backend-api/wham/apps".to_string()) ); assert_eq!( codex_apps_mcp_url_for_base_url( "http://localhost:8080/api/codex", /*apps_mcp_path_override*/ None, - ), - "http://localhost:8080/api/codex/apps" + ) + .expect("local debug URL should build"), + TrustedCodexAppsMcpUrl("http://localhost:8080/api/codex/apps".to_string()) ); assert_eq!( codex_apps_mcp_url_for_base_url( "http://localhost:8080", /*apps_mcp_path_override*/ None, - ), - "http://localhost:8080/api/codex/apps" + ) + .expect("local debug URL should build"), + TrustedCodexAppsMcpUrl("http://localhost:8080/api/codex/apps".to_string()) ); } +#[test] +fn codex_apps_mcp_url_for_base_url_rejects_untrusted_urls() { + for base_url in [ + "http://chatgpt.com/backend-api", + "https://example.com/backend-api", + "https://chatgpt.com.evil.example/backend-api", + "https://evilchatgpt.com/backend-api", + "https://foo.chat.openai.com/backend-api", + "https://chatgpt.com:4443/backend-api", + "https://user:pass@chatgpt.com/backend-api", + "https://chatgpt.com/backend-api?token=secret", + ] { + let err = codex_apps_mcp_url_for_base_url(base_url, /*apps_mcp_path_override*/ None) + .expect_err("untrusted URL should be rejected"); + + assert!( + err.starts_with("invalid Codex Apps MCP base URL"), + "unexpected error: {err}" + ); + } +} + #[test] fn codex_apps_mcp_url_uses_legacy_codex_apps_path() { - let config = test_mcp_config(PathBuf::from("/tmp")); + let codex_home = tempfile::tempdir().expect("tempdir"); + let config = test_mcp_config(codex_home.path().to_path_buf()); assert_eq!( - codex_apps_mcp_url(&config), + codex_apps_mcp_url(&config).expect("trusted ChatGPT URL should build"), "https://chatgpt.com/backend-api/wham/apps" ); } #[test] fn codex_apps_server_config_uses_legacy_codex_apps_path() { - let mut config = test_mcp_config(PathBuf::from("/tmp")); + let codex_home = tempfile::tempdir().expect("tempdir"); + let mut config = test_mcp_config(codex_home.path().to_path_buf()); let auth = CodexAuth::create_dummy_chatgpt_auth_for_testing(); let mut servers = with_codex_apps_mcp(HashMap::new(), /*auth*/ None, &config); @@ -230,17 +258,35 @@ fn codex_apps_server_config_uses_legacy_codex_apps_path() { #[test] fn codex_apps_server_config_is_marked_host_owned() { - let config = test_mcp_config(PathBuf::from("/tmp")); + let codex_home = tempfile::tempdir().expect("tempdir"); + let config = test_mcp_config(codex_home.path().to_path_buf()); assert_eq!( - codex_apps_mcp_server_config(&config).provenance, + codex_apps_mcp_server_config(&config) + .expect("trusted ChatGPT URL should build") + .provenance, McpServerProvenance::HostOwnedCodexApps ); } +#[test] +fn host_owned_codex_apps_endpoint_pairs_trusted_url_with_provenance() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let config = test_mcp_config(codex_home.path().to_path_buf()); + + assert_eq!( + host_owned_codex_apps_mcp_endpoint(&config).expect("trusted ChatGPT URL should build"), + HostOwnedCodexAppsMcpEndpoint { + url: TrustedCodexAppsMcpUrl("https://chatgpt.com/backend-api/wham/apps".to_string(),), + provenance: McpServerProvenance::HostOwnedCodexApps, + } + ); +} + #[test] fn codex_apps_server_config_uses_configured_apps_mcp_path_override() { - let mut config = test_mcp_config(PathBuf::from("/tmp")); + let codex_home = tempfile::tempdir().expect("tempdir"); + let mut config = test_mcp_config(codex_home.path().to_path_buf()); config.apps_mcp_path_override = Some("/custom/mcp".to_string()); config.apps_enabled = true; let auth = CodexAuth::create_dummy_chatgpt_auth_for_testing(); @@ -257,6 +303,19 @@ fn codex_apps_server_config_uses_configured_apps_mcp_path_override() { assert_eq!(url, "https://chatgpt.com/backend-api/custom/mcp"); } +#[test] +fn with_codex_apps_mcp_does_not_trust_untrusted_base_url() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let mut config = test_mcp_config(codex_home.path().to_path_buf()); + config.apps_enabled = true; + config.chatgpt_base_url = "https://chatgpt.com.evil.example/backend-api".to_string(); + let auth = CodexAuth::create_dummy_chatgpt_auth_for_testing(); + + let servers = with_codex_apps_mcp(HashMap::new(), Some(&auth), &config); + + assert!(!servers.contains_key(CODEX_APPS_MCP_SERVER_NAME)); +} + #[tokio::test] async fn effective_mcp_servers_preserve_user_servers_and_add_codex_apps() { let codex_home = tempfile::tempdir().expect("tempdir");