From 48e229c763c679763011bb71b3f5aff09ea9b60f Mon Sep 17 00:00:00 2001 From: Felix Xia Date: Tue, 23 Jun 2026 16:00:40 +0100 Subject: [PATCH] Apply MCP matchers to plugin servers --- codex-rs/core/src/config/config_tests.rs | 118 ++++++++++++++++++++++- codex-rs/core/src/config/mod.rs | 56 +++++++---- 2 files changed, 151 insertions(+), 23 deletions(-) diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 6aceb7349b..9f7358cefe 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -4466,15 +4466,20 @@ fn filter_mcp_servers_by_allowlist_blocks_all_when_empty() { } #[test] -fn filter_plugin_mcp_servers_by_allowlist_enforces_plugin_and_identity_rules() { +fn filter_plugin_mcp_servers_by_allowlist_enforces_plugin_and_matcher_rules() { const MATCHED_SERVER: &str = "matched-should-allow"; const MISMATCHED_SERVER: &str = "mismatched-should-disable"; + const MATCHER_MISMATCHED_SERVER: &str = "matcher-mismatched-should-disable"; const UNLISTED_SERVER: &str = "unlisted-should-disable"; const GOOD_CMD: &str = "good-cmd"; let mut servers = HashMap::from([ (MATCHED_SERVER.to_string(), stdio_mcp(GOOD_CMD)), (MISMATCHED_SERVER.to_string(), stdio_mcp("bad-cmd")), + ( + MATCHER_MISMATCHED_SERVER.to_string(), + stdio_mcp_with_args(GOOD_CMD, &["unexpected"]), + ), ( UNLISTED_SERVER.to_string(), http_mcp("https://example.com/mcp"), @@ -4502,13 +4507,52 @@ fn filter_plugin_mcp_servers_by_allowlist_enforces_plugin_and_identity_rules() { }, }, ), + ( + MATCHER_MISMATCHED_SERVER.to_string(), + McpServerRequirement { + identity: McpServerIdentity::Command { + command: GOOD_CMD.to_string(), + }, + }, + ), ])), }, )]), source.clone(), ); + let matchers = Sourced::new( + BTreeMap::from([ + ( + MATCHED_SERVER.to_string(), + McpServerMatcher::Command(McpServerCommandMatcher { + command: GOOD_CMD.to_string(), + args: Vec::new(), + }), + ), + ( + MISMATCHED_SERVER.to_string(), + McpServerMatcher::Command(McpServerCommandMatcher { + command: "bad-cmd".to_string(), + args: Vec::new(), + }), + ), + ( + MATCHER_MISMATCHED_SERVER.to_string(), + McpServerMatcher::Command(McpServerCommandMatcher { + command: GOOD_CMD.to_string(), + args: Vec::new(), + }), + ), + ]), + source.clone(), + ); - filter_plugin_mcp_servers_by_requirements("sample@test", &mut servers, Some(&requirements)); + filter_plugin_mcp_servers_by_requirements( + "sample@test", + &mut servers, + Some(&requirements), + Some(&matchers), + ); let reason = Some(McpServerDisabledReason::Requirements { source }); assert_eq!( @@ -4522,6 +4566,10 @@ fn filter_plugin_mcp_servers_by_allowlist_enforces_plugin_and_identity_rules() { HashMap::from([ (MATCHED_SERVER.to_string(), (true, None)), (MISMATCHED_SERVER.to_string(), (false, reason.clone())), + ( + MATCHER_MISMATCHED_SERVER.to_string(), + (false, reason.clone()), + ), (UNLISTED_SERVER.to_string(), (false, reason)), ]) ); @@ -4548,7 +4596,12 @@ fn filter_plugin_mcp_servers_by_allowlist_blocks_unlisted_plugin() { source.clone(), ); - filter_plugin_mcp_servers_by_requirements("sample@test", &mut servers, Some(&requirements)); + filter_plugin_mcp_servers_by_requirements( + "sample@test", + &mut servers, + Some(&requirements), + /*mcp_matchers*/ None, + ); assert_eq!( servers @@ -4568,6 +4621,65 @@ fn filter_plugin_mcp_servers_by_allowlist_blocks_unlisted_plugin() { ); } +#[test] +fn filter_plugin_mcp_servers_by_matchers_enforces_name_and_invocation() { + const MATCHED_SERVER: &str = "matched"; + const MISMATCHED_SERVER: &str = "mismatched"; + const UNLISTED_SERVER: &str = "unlisted"; + + let mut servers = HashMap::from([ + ( + MATCHED_SERVER.to_string(), + stdio_mcp_with_args("forge", &["approved"]), + ), + ( + MISMATCHED_SERVER.to_string(), + stdio_mcp_with_args("forge", &["rejected"]), + ), + ( + UNLISTED_SERVER.to_string(), + stdio_mcp_with_args("forge", &["approved"]), + ), + ]); + let source = RequirementSource::LegacyManagedConfigTomlFromMdm; + let matcher = McpServerMatcher::Command(McpServerCommandMatcher { + command: "forge".to_string(), + args: vec![McpServerValueMatcher::Exact { + value: "approved".to_string(), + }], + }); + let matchers = Sourced::new( + BTreeMap::from([ + (MATCHED_SERVER.to_string(), matcher.clone()), + (MISMATCHED_SERVER.to_string(), matcher), + ]), + source.clone(), + ); + + filter_plugin_mcp_servers_by_requirements( + "sample@test", + &mut servers, + /*plugin_requirements*/ None, + Some(&matchers), + ); + + let reason = Some(McpServerDisabledReason::Requirements { source }); + assert_eq!( + servers + .iter() + .map(|(name, server)| ( + name.clone(), + (server.enabled, server.disabled_reason.clone()) + )) + .collect::)>>(), + HashMap::from([ + (MATCHED_SERVER.to_string(), (true, None)), + (MISMATCHED_SERVER.to_string(), (false, reason.clone())), + (UNLISTED_SERVER.to_string(), (false, reason)), + ]) + ); +} + #[tokio::test] async fn rebuild_preserving_session_layers_refreshes_requirements() -> std::io::Result<()> { let codex_home = TempDir::new()?; diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 5ed06d6fef..a2041bbb66 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -1489,24 +1489,22 @@ impl Config { plugin_id: &str, mcp_servers: &mut HashMap, ) { + let requirements = self.config_layer_stack.requirements(); filter_plugin_mcp_servers_by_requirements( plugin_id, mcp_servers, - self.config_layer_stack.requirements().plugins.as_ref(), + requirements.plugins.as_ref(), + requirements.mcp_server_matchers.as_ref(), ); - let empty_mcp_allowlist = self - .config_layer_stack - .requirements() + let empty_mcp_allowlist = requirements .mcp_servers .as_ref() .filter(|requirements| requirements.value.is_empty()); - let empty_mcp_matchers = self - .config_layer_stack - .requirements() - .mcp_server_matchers - .as_ref() - .filter(|requirements| requirements.value.is_empty()); - filter_mcp_servers_by_requirements(mcp_servers, empty_mcp_allowlist, empty_mcp_matchers); + filter_mcp_servers_by_requirements( + mcp_servers, + empty_mcp_allowlist, + /*mcp_matchers*/ None, + ); } pub async fn to_mcp_config( @@ -1915,20 +1913,38 @@ fn filter_plugin_mcp_servers_by_requirements( plugin_config_name: &str, mcp_servers: &mut HashMap, plugin_requirements: Option<&Sourced>>, + mcp_matchers: Option<&Sourced>>, ) { - let Some(requirements) = plugin_requirements else { + if plugin_requirements.is_none() && mcp_matchers.is_none() { return; - }; - let source = requirements.source.clone(); - let plugin_mcp_requirements = requirements - .value - .get(plugin_config_name) + } + + let source = RequirementSource::composite( + plugin_requirements + .iter() + .map(|requirements| requirements.source.clone()) + .chain( + mcp_matchers + .iter() + .map(|requirements| requirements.source.clone()), + ), + ); + let plugin_mcp_requirements = plugin_requirements + .and_then(|requirements| requirements.value.get(plugin_config_name)) .and_then(|plugin| plugin.mcp_servers.as_ref()); for (name, server) in mcp_servers.iter_mut() { - let allowed = plugin_mcp_requirements - .and_then(|mcp_requirements| mcp_requirements.get(name)) - .is_some_and(|requirement| mcp_server_matches_requirement(requirement, server)); + let allowed_by_plugin_requirement = plugin_requirements.is_none() + || plugin_mcp_requirements + .and_then(|mcp_requirements| mcp_requirements.get(name)) + .is_some_and(|requirement| mcp_server_matches_requirement(requirement, server)); + let allowed_by_matcher = mcp_matchers.is_none_or(|matchers| { + matchers + .value + .get(name) + .is_some_and(|matcher| matcher.matches(server)) + }); + let allowed = allowed_by_plugin_requirement && allowed_by_matcher; if allowed { server.disabled_reason = None; } else {