From cb2581dae8de14ca5282332ec329c335bf04195f Mon Sep 17 00:00:00 2001 From: Matthew Zeng Date: Thu, 22 Oct 2026 21:02:23 +0000 Subject: [PATCH] Preserve MCP disable reasons across runtime overlays (#56266) ## Why A requirements denial on a custom `codex_apps` registration could block the trusted hosted Apps replacement. Requirements filtering also lost whether a server had been explicitly disabled, which must remain effective across runtime overlays. ## What changed - Track explicit disables separately from requirements denials, retaining the requirement source across catalog rebuilds and server materialization. - Allow trusted host-owned Apps to bypass a previous registration's requirements denial while preserving explicit disables and blocking later custom replacements. - Preserve combined explicit and requirements disables during repeated filtering, and reject those servers when preparing standalone MCP login HTTP clients. ## Testing Extend regression coverage for hosted Apps replacements, later custom replacements, cloud catalog refreshes, repeated requirements filtering, and standalone login rejection. GitOrigin-RevId: 736a13c40fbf7436b4d0682f70c335f33f45962f --- codex-rs/codex-mcp/src/catalog.rs | 42 +++++++++++++++++++--- codex-rs/codex-mcp/src/catalog_tests.rs | 21 +++++++++++ codex-rs/codex-mcp/src/runtime.rs | 5 ++- codex-rs/config/src/mcp_types.rs | 20 ++++++++++- codex-rs/core/src/config/config_tests.rs | 15 ++++++-- codex-rs/core/src/config/mod.rs | 11 ++---- codex-rs/ext/mcp/src/cloud_plugin_tests.rs | 10 ++++++ codex-rs/ext/mcp/tests/hosted_apps_mcp.rs | 18 ++++++++++ 8 files changed, 124 insertions(+), 18 deletions(-) diff --git a/codex-rs/codex-mcp/src/catalog.rs b/codex-rs/codex-mcp/src/catalog.rs index 332516533c..e2119308c3 100644 --- a/codex-rs/codex-mcp/src/catalog.rs +++ b/codex-rs/codex-mcp/src/catalog.rs @@ -358,6 +358,7 @@ impl CatalogAction { pub struct McpCatalogBuilder { actions: Vec, disabled_server_names: BTreeSet, + requirements_disabled_server_names: BTreeMap, ema_idp: Option, } @@ -514,6 +515,7 @@ impl McpCatalogBuilder { } let mut disabled_server_names = self.disabled_server_names; + let mut requirements_disabled_server_names = self.requirements_disabled_server_names; let ema_idp = self.ema_idp; let servers = winners .into_iter() @@ -529,13 +531,38 @@ impl McpCatalogBuilder { registration.config.auth, codex_config::McpServerAuth::EmaAuth )); - if !registration.config.enabled || disabled_server_names.contains(&name) { - registration.config.enabled = false; - if persist_disabled_name { - // Preserve legacy disabled winners across later runtime overlays. - disabled_server_names.insert(name.clone()); + if !registration.config.enabled && persist_disabled_name { + match ®istration.config.disabled_reason { + Some( + McpServerDisabledReason::Requirements { source } + | McpServerDisabledReason::ExplicitAndRequirements { source }, + ) => { + requirements_disabled_server_names + .insert(name.clone(), source.clone()); + if matches!( + registration.config.disabled_reason, + Some(McpServerDisabledReason::ExplicitAndRequirements { .. }) + ) { + disabled_server_names.insert(name.clone()); + } + } + Some(McpServerDisabledReason::Unknown) | None => { + disabled_server_names.insert(name.clone()); + } } } + if disabled_server_names.contains(&name) { + registration.config.enabled = false; + } + // Only trusted Apps may ignore a previous registration's requirements denial. + // Retain it so a later custom replacement is still blocked. + if !registration + .source + .is_host_owned_apps(&name, ®istration.config) + && let Some(source) = requirements_disabled_server_names.get(&name) + { + registration.config.disable_by_requirements(source.clone()); + } if matches!( registration.config.auth, codex_config::McpServerAuth::EmaAuth @@ -564,6 +591,7 @@ impl McpCatalogBuilder { ResolvedMcpCatalog { actions: self.actions, disabled_server_names, + requirements_disabled_server_names, ema_idp, servers, conflicts, @@ -603,6 +631,7 @@ impl ResolvedMcpServer { pub struct ResolvedMcpCatalog { actions: Vec, disabled_server_names: BTreeSet, + requirements_disabled_server_names: BTreeMap, ema_idp: Option, servers: BTreeMap, conflicts: Vec, @@ -617,6 +646,7 @@ impl ResolvedMcpCatalog { McpCatalogBuilder { actions: self.actions.clone(), disabled_server_names: self.disabled_server_names.clone(), + requirements_disabled_server_names: self.requirements_disabled_server_names.clone(), ema_idp: self.ema_idp.clone(), } } @@ -644,6 +674,8 @@ impl ResolvedMcpCatalog { /// Panics if a materialized server is missing from the catalog. pub fn with_materialized_servers(&self, servers: HashMap) -> Self { let mut builder = McpCatalogBuilder { + disabled_server_names: self.disabled_server_names.clone(), + requirements_disabled_server_names: self.requirements_disabled_server_names.clone(), ema_idp: self.ema_idp.clone(), ..Default::default() }; diff --git a/codex-rs/codex-mcp/src/catalog_tests.rs b/codex-rs/codex-mcp/src/catalog_tests.rs index 14dcedc750..e079c81564 100644 --- a/codex-rs/codex-mcp/src/catalog_tests.rs +++ b/codex-rs/codex-mcp/src/catalog_tests.rs @@ -749,7 +749,15 @@ fn environment_policy_exempts_only_explicitly_host_owned_apps() { true, ), ] { + let custom = server("https://custom.example/mcp"); let mut builder = ResolvedMcpCatalog::builder(); + builder.register(McpServerRegistration::from_config( + CODEX_APPS_MCP_SERVER_NAME.to_string(), + custom.clone(), + )); + let denied = builder + .build_with_environment_authority(|_| McpEnvironmentAuthority::Restricted(&policy)); + let mut builder = denied.to_builder(); builder.register(registration); let catalog = builder .build_with_environment_authority(|_| McpEnvironmentAuthority::Restricted(&policy)); @@ -761,6 +769,19 @@ fn environment_policy_exempts_only_explicitly_host_owned_apps() { .enabled, expected ); + let mut builder = catalog + .with_materialized_servers(catalog.configured_servers()) + .to_builder(); + builder.register(McpServerRegistration::from_extension( + CODEX_APPS_MCP_SERVER_NAME.to_string(), + "custom", + /*contribution_order*/ 1, + custom, + )); + assert_eq!( + builder.build().configured_servers(), + denied.configured_servers() + ); } } diff --git a/codex-rs/codex-mcp/src/runtime.rs b/codex-rs/codex-mcp/src/runtime.rs index 4d46c67696..2cb29fb685 100644 --- a/codex-rs/codex-mcp/src/runtime.rs +++ b/codex-rs/codex-mcp/src/runtime.rs @@ -916,7 +916,10 @@ pub fn apply_http_headers_helper( ) -> Result, String> { if matches!( config.disabled_reason, - Some(McpServerDisabledReason::Requirements { .. }) + Some( + McpServerDisabledReason::Requirements { .. } + | McpServerDisabledReason::ExplicitAndRequirements { .. } + ) ) { return Err("the MCP server is disabled by managed requirements".to_string()); } diff --git a/codex-rs/config/src/mcp_types.rs b/codex-rs/config/src/mcp_types.rs index b6243ab801..d80899e210 100644 --- a/codex-rs/config/src/mcp_types.rs +++ b/codex-rs/config/src/mcp_types.rs @@ -62,13 +62,16 @@ pub enum McpServerDisabledReason { Unknown, /// The server was disabled by config requirements from the given source. Requirements { source: RequirementSource }, + /// An explicit disable is also denied by requirements. + ExplicitAndRequirements { source: RequirementSource }, } impl fmt::Display for McpServerDisabledReason { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { McpServerDisabledReason::Unknown => write!(f, "unknown"), - McpServerDisabledReason::Requirements { source } => { + McpServerDisabledReason::Requirements { source } + | McpServerDisabledReason::ExplicitAndRequirements { source } => { write!(f, "requirements ({source})") } } @@ -311,6 +314,21 @@ pub struct McpServerConfig { } impl McpServerConfig { + /// Disables this registration without losing an existing explicit disable. + pub fn disable_by_requirements(&mut self, source: RequirementSource) { + let explicitly_disabled = match self.disabled_reason { + Some(McpServerDisabledReason::Requirements { .. }) => false, + Some(McpServerDisabledReason::ExplicitAndRequirements { .. }) => true, + Some(McpServerDisabledReason::Unknown) | None => !self.enabled, + }; + self.enabled = false; + self.disabled_reason = Some(if explicitly_disabled { + McpServerDisabledReason::ExplicitAndRequirements { source } + } else { + McpServerDisabledReason::Requirements { source } + }); + } + pub fn is_local_environment(&self) -> bool { self.environment_id == DEFAULT_MCP_SERVER_ENVIRONMENT_ID } diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 40b809731b..751dd48ad1 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -10,6 +10,7 @@ use codex_config::ConfigLayerEntry; use codex_config::ConfigLayerSource; use codex_config::ConfigLayerStack; use codex_config::McpServerCommandMatcher; +use codex_config::McpServerDisabledReason; use codex_config::McpServerIdentity; use codex_config::McpServerRequirement; use codex_config::McpServerValueMatcher; @@ -5430,9 +5431,11 @@ fn filter_mcp_servers_by_allowlist_blocks_all_when_empty() { ("server-b".to_string(), http_mcp("https://example.com/b")), ]); + servers.get_mut("server-b").expect("server-b").enabled = false; let source = RequirementSource::LegacyManagedConfigTomlFromMdm; let requirements = Sourced::new(BTreeMap::new(), source.clone()); filter_mcp_servers_by_requirements(&mut servers, Some(&requirements)); + filter_mcp_servers_by_requirements(&mut servers, Some(&requirements)); let reason = Some(McpServerDisabledReason::Requirements { source }); assert_eq!( @@ -5444,8 +5447,16 @@ fn filter_mcp_servers_by_allowlist_blocks_all_when_empty() { )) .collect::)>>(), HashMap::from([ - ("server-a".to_string(), (false, reason.clone())), - ("server-b".to_string(), (false, reason)), + ("server-a".to_string(), (false, reason)), + ( + "server-b".to_string(), + ( + false, + Some(McpServerDisabledReason::ExplicitAndRequirements { + source: requirements.source.clone(), + }) + ), + ), ]) ); } diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 1ce4a28839..076983dfc5 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -43,7 +43,6 @@ use codex_config::types::AuthCredentialsStoreMode; use codex_config::types::AuthKeyringBackendKind; use codex_config::types::History; use codex_config::types::McpServerConfig; -use codex_config::types::McpServerDisabledReason; use codex_config::types::MemoriesConfig; use codex_config::types::ModelAvailabilityNuxConfig; use codex_config::types::Notice; @@ -2266,10 +2265,7 @@ fn filter_mcp_servers_by_requirements( if allowed { server.disabled_reason = None; } else { - server.enabled = false; - server.disabled_reason = Some(McpServerDisabledReason::Requirements { - source: source.clone(), - }); + server.disable_by_requirements(source.clone()); } } } @@ -2302,10 +2298,7 @@ fn filter_plugin_mcp_servers_by_requirements( if allowed { server.disabled_reason = None; } else { - server.enabled = false; - server.disabled_reason = Some(McpServerDisabledReason::Requirements { - source: source.clone(), - }); + server.disable_by_requirements(source.clone()); } } } diff --git a/codex-rs/ext/mcp/src/cloud_plugin_tests.rs b/codex-rs/ext/mcp/src/cloud_plugin_tests.rs index 23a6809315..af96289ca3 100644 --- a/codex-rs/ext/mcp/src/cloud_plugin_tests.rs +++ b/codex-rs/ext/mcp/src/cloud_plugin_tests.rs @@ -8,6 +8,7 @@ use crate::PluginProviders; use crate::PluginsThreadState; use crate::install_plugin_providers; use anyhow::Context; +use codex_config::test_support::CloudConfigBundleFixture; use codex_core_plugins::ExecutorPluginProvider; use codex_core_plugins::PluginCatalog; use codex_core_plugins::PluginCatalogEntry; @@ -506,6 +507,7 @@ async fn run_cloud_catalog_refresh_lifecycle() -> anyhow::Result<()> { let provider = Arc::new(CloudProvider::default()); let step_packages = Arc::new(StepPackages::default()); let mut extensions = ExtensionRegistryBuilder::new(); + crate::install(&mut extensions); extensions.prompt_contributor(step_packages.clone()); install_plugin_providers( &mut extensions, @@ -515,6 +517,13 @@ async fn run_cloud_catalog_refresh_lifecycle() -> anyhow::Result<()> { .with_cloud_provider(provider.clone()), ); let mut builder = test_codex() + .with_cloud_config_bundle( + CloudConfigBundleFixture::enterprise_config( + "[mcp_servers.codex_apps]\nurl = 'https://example.com/custom'", + ) + .add_enterprise_requirement("[mcp_servers]") + .into_loader(), + ) .with_exec_server_url("none") .with_extensions(Arc::new(extensions.build())) .with_auth(codex_login::CodexAuth::from_external_chatgpt_tokens( @@ -537,6 +546,7 @@ async fn run_cloud_catalog_refresh_lifecycle() -> anyhow::Result<()> { config.chatgpt_base_url = apps_server.chatgpt_base_url; }); let test = builder.build_with_auto_env(&server).await?; + assert!(!test.config.mcp_servers.get()[codex_mcp::CODEX_APPS_MCP_SERVER_NAME].enabled); let calendar = ("calendar", "list_events", "Cloud Demo"); let gmail = ("gmail", "search_email", "Gmail Demo"); for (turn, reply, expected_plugin, new_account, tool_call) in [ diff --git a/codex-rs/ext/mcp/tests/hosted_apps_mcp.rs b/codex-rs/ext/mcp/tests/hosted_apps_mcp.rs index 4d0e252494..5de9c29c5c 100644 --- a/codex-rs/ext/mcp/tests/hosted_apps_mcp.rs +++ b/codex-rs/ext/mcp/tests/hosted_apps_mcp.rs @@ -1,6 +1,7 @@ use std::sync::Arc; use codex_config::McpServerTransportConfig; +use codex_config::test_support::CloudConfigBundleFixture; use codex_core::McpManager; use codex_core::config::Config; use codex_core::config::ConfigBuilder; @@ -59,11 +60,28 @@ async fn runtime_overlay_preserves_disabled_server() -> TestResult { ), ("mcp_servers.codex_apps.enabled".to_string(), false.into()), ]) + .cloud_config_bundle( + CloudConfigBundleFixture::loader_with_enterprise_requirement("[mcp_servers]"), + ) .build() .await?; let auth = CodexAuth::create_dummy_chatgpt_auth_for_testing(); let manager = installed_manager(&config, Some(auth.clone())); + let configured = manager.configured_servers(&config).await; + // Standalone login must still reject requirements-denied, explicitly disabled entries. + let login_client = Arc::new(codex_exec_server::RouteAwareHttpClient::new( + config.http_client_factory(), + )); + assert_eq!( + codex_mcp::apply_http_headers_helper( + login_client, + &configured[CODEX_APPS_MCP_SERVER_NAME], + codex_home.path().to_path_buf(), + ) + .err(), + Some("the MCP server is disabled by managed requirements".to_string()) + ); let servers = manager.effective_servers(&config, Some(&auth)).await; let server = servers .get(CODEX_APPS_MCP_SERVER_NAME)