mirror of
https://github.com/openai/codex.git
synced 2026-10-23 18:08:45 +00:00
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
This commit is contained in:
@@ -358,6 +358,7 @@ impl CatalogAction {
|
||||
pub struct McpCatalogBuilder {
|
||||
actions: Vec<CatalogAction>,
|
||||
disabled_server_names: BTreeSet<String>,
|
||||
requirements_disabled_server_names: BTreeMap<String, RequirementSource>,
|
||||
ema_idp: Option<McpServerIdpOAuthConfig>,
|
||||
}
|
||||
|
||||
@@ -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<CatalogAction>,
|
||||
disabled_server_names: BTreeSet<String>,
|
||||
requirements_disabled_server_names: BTreeMap<String, RequirementSource>,
|
||||
ema_idp: Option<McpServerIdpOAuthConfig>,
|
||||
servers: BTreeMap<String, ResolvedMcpServer>,
|
||||
conflicts: Vec<McpServerConflict>,
|
||||
@@ -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<String, McpServerConfig>) -> 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()
|
||||
};
|
||||
|
||||
@@ -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()
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -916,7 +916,10 @@ pub fn apply_http_headers_helper(
|
||||
) -> Result<Arc<dyn HttpClient>, 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());
|
||||
}
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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<String, (bool, Option<McpServerDisabledReason>)>>(),
|
||||
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(),
|
||||
})
|
||||
),
|
||||
),
|
||||
])
|
||||
);
|
||||
}
|
||||
|
||||
@@ -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());
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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 [
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user