diff --git a/codex-rs/codex-mcp/src/mcp/codex_apps_endpoint.rs b/codex-rs/codex-mcp/src/mcp/codex_apps_endpoint.rs new file mode 100644 index 0000000000..7d7d3588fe --- /dev/null +++ b/codex-rs/codex-mcp/src/mcp/codex_apps_endpoint.rs @@ -0,0 +1,149 @@ +use codex_client::is_allowed_chatgpt_host; +use codex_config::McpServerProvenance; +use url::Host; +use url::Url; + +use super::McpConfig; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(super) struct HostOwnedCodexAppsMcpEndpoint { + url: trusted_codex_apps_mcp_url::TrustedCodexAppsMcpUrl, + provenance: McpServerProvenance, +} + +impl HostOwnedCodexAppsMcpEndpoint { + fn new(url: trusted_codex_apps_mcp_url::TrustedCodexAppsMcpUrl) -> Self { + Self { + url, + provenance: McpServerProvenance::HostOwnedCodexApps, + } + } + + pub(super) fn into_parts(self) -> (String, McpServerProvenance) { + (self.url.into_string(), self.provenance) + } + + #[cfg(test)] + pub(super) fn url(&self) -> &str { + self.url.as_str() + } + + #[cfg(test)] + pub(super) fn provenance(&self) -> McpServerProvenance { + self.provenance + } +} + +/// Builds the only endpoint allowed to receive host-owned Codex Apps provenance. +pub(super) 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 = trusted_codex_apps_mcp_url::from_base_url( + &config.chatgpt_base_url, + config.apps_mcp_path_override.as_deref(), + )?; + Ok(HostOwnedCodexAppsMcpEndpoint::new(url)) +} + +mod trusted_codex_apps_mcp_url { + use super::*; + + #[derive(Debug, Clone, PartialEq, Eq)] + pub(super) struct TrustedCodexAppsMcpUrl(String); + + impl TrustedCodexAppsMcpUrl { + fn new(url: String) -> Result { + let parsed_url = Url::parse(&url) + .map_err(|err| format!("invalid Codex Apps MCP URL `{url}`: {err}"))?; + validate_url(&parsed_url, &url, "URL")?; + Ok(Self(url)) + } + + pub(super) fn into_string(self) -> String { + self.0 + } + + #[cfg(test)] + pub(super) fn as_str(&self) -> &str { + &self.0 + } + } + + pub(super) fn from_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_url(&parsed_base_url, base_url, "base URL")?; + + let mut base_url = base_url.to_string(); + if is_allowed_chatgpt_host_url(&parsed_base_url.host()) + && !parsed_base_url.path().contains("/backend-api") + { + base_url = format!("{base_url}/backend-api"); + } + let (base_url, default_path) = if base_url.contains("/backend-api") { + (base_url, "wham/apps") + } else if base_url.contains("/api/codex") { + (base_url, "apps") + } else { + (format!("{base_url}/api/codex"), "apps") + }; + let path = apps_mcp_path_override + .unwrap_or(default_path) + .trim_start_matches('/'); + TrustedCodexAppsMcpUrl::new(format!("{base_url}/{path}")) + } + + fn validate_url(url: &Url, original_url: &str, label: &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 {label} `{original_url}`; expected a URL without credentials, query, or fragment" + )); + } + + let scheme = url.scheme(); + let host = url.host(); + let valid_first_party_url = + scheme == "https" && url.port().is_none() && is_allowed_chatgpt_host_url(&host); + let valid_local_url = + cfg!(debug_assertions) && matches!(scheme, "http" | "https") && is_localhost(&host); + if valid_first_party_url || valid_local_url { + Ok(()) + } else { + Err(format!( + "invalid Codex Apps MCP {label} `{original_url}`; expected an HTTPS URL for chatgpt.com, chat.openai.com, or chatgpt-staging.com" + )) + } + } + + fn is_allowed_chatgpt_host_url(host: &Option>) -> bool { + let Some(Host::Domain(host)) = host else { + return false; + }; + is_allowed_chatgpt_host(host) + } + + fn is_localhost(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, + } + } +} + +#[cfg(test)] +#[path = "codex_apps_endpoint_tests.rs"] +mod tests; diff --git a/codex-rs/codex-mcp/src/mcp/codex_apps_endpoint_tests.rs b/codex-rs/codex-mcp/src/mcp/codex_apps_endpoint_tests.rs new file mode 100644 index 0000000000..ad5946dd07 --- /dev/null +++ b/codex-rs/codex-mcp/src/mcp/codex_apps_endpoint_tests.rs @@ -0,0 +1,96 @@ +use super::*; +use pretty_assertions::assert_eq; + +#[test] +fn trusted_url_keeps_existing_paths() { + assert_eq!( + trusted_codex_apps_mcp_url::from_base_url( + "https://chatgpt.com/backend-api", + /*apps_mcp_path_override*/ None, + ) + .expect("trusted ChatGPT URL should build") + .as_str(), + "https://chatgpt.com/backend-api/wham/apps" + ); + assert_eq!( + trusted_codex_apps_mcp_url::from_base_url( + "https://chat.openai.com", + /*apps_mcp_path_override*/ None, + ) + .expect("trusted legacy ChatGPT URL should build") + .as_str(), + "https://chat.openai.com/backend-api/wham/apps" + ); + assert_eq!( + trusted_codex_apps_mcp_url::from_base_url( + "http://localhost:8080/api/codex", + /*apps_mcp_path_override*/ None, + ) + .expect("local debug URL should build") + .as_str(), + "http://localhost:8080/api/codex/apps" + ); + assert_eq!( + trusted_codex_apps_mcp_url::from_base_url( + "http://localhost:8080", + /*apps_mcp_path_override*/ None, + ) + .expect("local debug URL should build") + .as_str(), + "http://localhost:8080/api/codex/apps" + ); +} + +#[test] +fn trusted_url_rejects_untrusted_base_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 = trusted_codex_apps_mcp_url::from_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 trusted_url_rejects_override_that_adds_query_or_fragment() { + for path_override in ["custom/mcp?token=secret", "custom/mcp#fragment"] { + let err = + trusted_codex_apps_mcp_url::from_base_url("https://chatgpt.com", Some(path_override)) + .expect_err("untrusted final URL should be rejected"); + + assert!( + err.starts_with("invalid Codex Apps MCP URL"), + "unexpected error: {err}" + ); + } +} + +#[test] +fn endpoint_pairs_trusted_url_with_provenance() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let config = crate::mcp::tests::test_mcp_config(codex_home.path().to_path_buf()); + let endpoint = + host_owned_codex_apps_mcp_endpoint(&config).expect("trusted ChatGPT URL should build"); + + assert_eq!( + (endpoint.url(), endpoint.provenance()), + ( + "https://chatgpt.com/backend-api/wham/apps", + McpServerProvenance::HostOwnedCodexApps, + ) + ); +} diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index cb81b54eed..46e6453b4f 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -10,6 +10,7 @@ pub use auth::resolve_oauth_scopes; pub use auth::should_retry_without_scopes; pub(crate) mod auth; +mod codex_apps_endpoint; use std::collections::HashMap; use std::env; @@ -17,10 +18,8 @@ use std::path::PathBuf; use std::time::Duration; use async_channel::unbounded; -use codex_client::is_allowed_chatgpt_host; use codex_config::Constrained; use codex_config::McpServerConfig; -use codex_config::McpServerProvenance; use codex_config::McpServerTransportConfig; use codex_config::types::AppToolApproval; use codex_config::types::ApprovalsReviewer; @@ -37,12 +36,11 @@ 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; use crate::runtime::McpRuntimeEnvironment; +use codex_apps_endpoint::host_owned_codex_apps_mcp_endpoint; pub const CODEX_APPS_MCP_SERVER_NAME: &str = "codex_apps"; const MCP_TOOL_NAME_PREFIX: &str = "mcp"; @@ -202,6 +200,7 @@ pub fn with_codex_apps_mcp( auth: Option<&CodexAuth>, config: &McpConfig, ) -> HashMap { + remove_reserved_codex_apps_server(&mut servers); if config.apps_enabled && auth.is_some_and(CodexAuth::uses_codex_backend) { match codex_apps_mcp_server_config(config) { Ok(server_config) => { @@ -209,17 +208,24 @@ pub fn with_codex_apps_mcp( } 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); } servers } pub fn configured_mcp_servers(config: &McpConfig) -> HashMap { - config.configured_mcp_servers.clone() + let mut servers = config.configured_mcp_servers.clone(); + remove_reserved_codex_apps_server(&mut servers); + servers +} + +fn remove_reserved_codex_apps_server(servers: &mut HashMap) { + if servers.remove(CODEX_APPS_MCP_SERVER_NAME).is_some() { + tracing::warn!( + "Ignoring configured MCP server with reserved name `{CODEX_APPS_MCP_SERVER_NAME}`" + ); + } } pub fn effective_mcp_servers( @@ -356,11 +362,6 @@ pub async fn collect_mcp_snapshot_from_manager( .await } -#[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_-]+$`. /// MCP server/tool names are user-controlled, so sanitize the fully-qualified /// name we expose to the model by replacing any disallowed character with `_`. @@ -390,106 +391,9 @@ fn codex_apps_mcp_bearer_token_env_var() -> Option { } } -#[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"); - } - let (base_url, default_path) = if base_url.contains("/backend-api") { - (base_url, "wham/apps") - } else if base_url.contains("/api/codex") { - (base_url, "apps") - } else { - (format!("{base_url}/api/codex"), "apps") - }; - let path = apps_mcp_path_override - .unwrap_or(default_path) - .trim_start_matches('/'); - Ok(TrustedCodexAppsMcpUrl(format!("{base_url}/{path}"))) -} - -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" - )); - } - - 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; - }; - is_allowed_chatgpt_host(host) -} - -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; + let (url, provenance) = endpoint.into_parts(); Ok(McpServerConfig { transport: McpServerTransportConfig::StreamableHttp { @@ -503,7 +407,7 @@ fn codex_apps_mcp_server_config(config: &McpConfig) -> Result McpConfig { +pub(super) fn test_mcp_config(codex_home: PathBuf) -> McpConfig { McpConfig { chatgpt_base_url: "https://chatgpt.com".to_string(), apps_mcp_path_override: None, @@ -161,73 +162,15 @@ fn tool_plugin_provenance_collects_app_and_mcp_sources() { ); } -#[test] -fn codex_apps_mcp_url_for_base_url_keeps_existing_paths() { - assert_eq!( - codex_apps_mcp_url_for_base_url( - "https://chatgpt.com/backend-api", - /*apps_mcp_path_override*/ None, - ) - .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, - ) - .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, - ) - .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, - ) - .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 codex_home = tempfile::tempdir().expect("tempdir"); let config = test_mcp_config(codex_home.path().to_path_buf()); - assert_eq!( - codex_apps_mcp_url(&config).expect("trusted ChatGPT URL should build"), - "https://chatgpt.com/backend-api/wham/apps" - ); + let endpoint = + host_owned_codex_apps_mcp_endpoint(&config).expect("trusted ChatGPT URL should build"); + + assert_eq!(endpoint.url(), "https://chatgpt.com/backend-api/wham/apps"); } #[test] @@ -266,20 +209,6 @@ fn codex_apps_server_config_is_marked_host_owned() { ); } -#[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 codex_home = tempfile::tempdir().expect("tempdir"); @@ -313,6 +242,39 @@ fn with_codex_apps_mcp_does_not_trust_untrusted_base_url() { assert!(!servers.contains_key(CODEX_APPS_MCP_SERVER_NAME)); } +#[test] +fn configured_mcp_servers_ignore_reserved_codex_apps_name() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let mut config = test_mcp_config(codex_home.path().to_path_buf()); + config.configured_mcp_servers.insert( + CODEX_APPS_MCP_SERVER_NAME.to_string(), + McpServerConfig { + transport: McpServerTransportConfig::StreamableHttp { + url: "https://user.example/mcp".to_string(), + bearer_token_env_var: None, + http_headers: None, + env_http_headers: None, + }, + experimental_environment: None, + enabled: true, + required: false, + supports_parallel_tool_calls: false, + disabled_reason: None, + provenance: Default::default(), + startup_timeout_sec: None, + tool_timeout_sec: None, + default_tools_approval_mode: None, + enabled_tools: None, + disabled_tools: None, + scopes: None, + oauth_resource: None, + tools: HashMap::new(), + }, + ); + + assert!(!configured_mcp_servers(&config).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");