mirror of
https://github.com/openai/codex.git
synced 2026-11-03 16:23:51 +00:00
[mcp] Fix plugin MCP approval policy. (#19537)
Plugin MCP servers are loaded from plugin manifests rather than top-level `[mcp_servers]`, so their tool approval preferences need to be stored and applied through the owning plugin config. Without this, choosing "Always allow" for a plugin MCP tool could write a preference that was not reliably used on later tool calls. ## Summary - Add plugin-scoped MCP policy config under `plugins.<plugin>.mcp_servers`, including server enablement, tool allow/deny lists, server defaults, and per-tool approval modes. - Overlay plugin MCP policy onto manifest-provided server configs when plugins are loaded. - Route persistent "Always allow" writes for plugin MCP tools back to the owning `plugins.<plugin>.mcp_servers.<server>.tools.<tool>` config entry. - Reload user config after persisting an approval and make the plugin load cache config-aware so stale plugin MCP policy is not reused after `config.toml` changes. - Regenerate the config schema and add coverage for plugin MCP policy loading, approval lookup, persistence, and stale-cache prevention. ## Testing - `cargo test -p codex-config` - `cargo test -p codex-core-plugins` - `cargo test -p codex-core --lib plugin_mcp`
This commit is contained in:
1 parent
4241df4d79
commit
e20391e567
15 files changed
+919
-28
No files matched your search
@@ -780,6 +780,7 @@ allowed_approval_policies = ["on-request"]
|
||||
feature_requirements: None,
|
||||
hooks: None,
|
||||
mcp_servers: None,
|
||||
plugins: None,
|
||||
apps: None,
|
||||
rules: None,
|
||||
enforce_residency: None,
|
||||
@@ -836,6 +837,7 @@ allowed_approval_policies = ["on-request"]
|
||||
feature_requirements: None,
|
||||
hooks: None,
|
||||
mcp_servers: None,
|
||||
plugins: None,
|
||||
apps: None,
|
||||
rules: None,
|
||||
enforce_residency: None,
|
||||
@@ -1043,6 +1045,7 @@ async fn load_config_layers_includes_cloud_requirements() -> anyhow::Result<()>
|
||||
feature_requirements: None,
|
||||
hooks: None,
|
||||
mcp_servers: None,
|
||||
plugins: None,
|
||||
apps: None,
|
||||
rules: None,
|
||||
enforce_residency: None,
|
||||
|
||||
@@ -2404,6 +2404,258 @@ fn filter_mcp_servers_by_allowlist_blocks_all_when_empty() {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn filter_plugin_mcp_servers_by_allowlist_enforces_plugin_and_identity_rules() {
|
||||
const MATCHED_SERVER: &str = "matched-should-allow";
|
||||
const MISMATCHED_SERVER: &str = "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")),
|
||||
(
|
||||
UNLISTED_SERVER.to_string(),
|
||||
http_mcp("https://example.com/mcp"),
|
||||
),
|
||||
]);
|
||||
let source = RequirementSource::CloudRequirements;
|
||||
let requirements = Sourced::new(
|
||||
BTreeMap::from([(
|
||||
"sample@test".to_string(),
|
||||
codex_config::PluginRequirementsToml {
|
||||
mcp_servers: Some(BTreeMap::from([
|
||||
(
|
||||
MATCHED_SERVER.to_string(),
|
||||
McpServerRequirement {
|
||||
identity: McpServerIdentity::Command {
|
||||
command: GOOD_CMD.to_string(),
|
||||
},
|
||||
},
|
||||
),
|
||||
(
|
||||
MISMATCHED_SERVER.to_string(),
|
||||
McpServerRequirement {
|
||||
identity: McpServerIdentity::Command {
|
||||
command: GOOD_CMD.to_string(),
|
||||
},
|
||||
},
|
||||
),
|
||||
])),
|
||||
},
|
||||
)]),
|
||||
source.clone(),
|
||||
);
|
||||
|
||||
filter_plugin_mcp_servers_by_requirements("sample@test", &mut servers, Some(&requirements));
|
||||
|
||||
let reason = Some(McpServerDisabledReason::Requirements { source });
|
||||
assert_eq!(
|
||||
servers
|
||||
.iter()
|
||||
.map(|(name, server)| (
|
||||
name.clone(),
|
||||
(server.enabled, server.disabled_reason.clone())
|
||||
))
|
||||
.collect::<HashMap<String, (bool, Option<McpServerDisabledReason>)>>(),
|
||||
HashMap::from([
|
||||
(MATCHED_SERVER.to_string(), (true, None)),
|
||||
(MISMATCHED_SERVER.to_string(), (false, reason.clone())),
|
||||
(UNLISTED_SERVER.to_string(), (false, reason)),
|
||||
])
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn filter_plugin_mcp_servers_by_allowlist_blocks_unlisted_plugin() {
|
||||
let mut servers = HashMap::from([("server-a".to_string(), stdio_mcp("cmd-a"))]);
|
||||
let source = RequirementSource::CloudRequirements;
|
||||
let requirements = Sourced::new(
|
||||
BTreeMap::from([(
|
||||
"other@test".to_string(),
|
||||
codex_config::PluginRequirementsToml {
|
||||
mcp_servers: Some(BTreeMap::from([(
|
||||
"server-a".to_string(),
|
||||
McpServerRequirement {
|
||||
identity: McpServerIdentity::Command {
|
||||
command: "cmd-a".to_string(),
|
||||
},
|
||||
},
|
||||
)])),
|
||||
},
|
||||
)]),
|
||||
source.clone(),
|
||||
);
|
||||
|
||||
filter_plugin_mcp_servers_by_requirements("sample@test", &mut servers, Some(&requirements));
|
||||
|
||||
assert_eq!(
|
||||
servers
|
||||
.iter()
|
||||
.map(|(name, server)| (
|
||||
name.clone(),
|
||||
(server.enabled, server.disabled_reason.clone())
|
||||
))
|
||||
.collect::<HashMap<String, (bool, Option<McpServerDisabledReason>)>>(),
|
||||
HashMap::from([(
|
||||
"server-a".to_string(),
|
||||
(
|
||||
false,
|
||||
Some(McpServerDisabledReason::Requirements { source })
|
||||
)
|
||||
)])
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn to_mcp_config_applies_plugin_mcp_cloud_requirements() -> anyhow::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
let plugin_root = codex_home
|
||||
.path()
|
||||
.join("plugins/cache")
|
||||
.join("test/sample/local");
|
||||
std::fs::create_dir_all(plugin_root.join(".codex-plugin"))?;
|
||||
std::fs::write(
|
||||
plugin_root.join(".codex-plugin/plugin.json"),
|
||||
r#"{"name":"sample"}"#,
|
||||
)?;
|
||||
std::fs::write(
|
||||
plugin_root.join(".mcp.json"),
|
||||
r#"{
|
||||
"mcpServers": {
|
||||
"sample": {
|
||||
"type": "http",
|
||||
"url": "https://sample.example/mcp"
|
||||
},
|
||||
"unlisted": {
|
||||
"type": "http",
|
||||
"url": "https://unlisted.example/mcp"
|
||||
}
|
||||
}
|
||||
}"#,
|
||||
)?;
|
||||
std::fs::write(
|
||||
codex_home.path().join(CONFIG_TOML_FILE),
|
||||
r#"
|
||||
[features]
|
||||
plugins = true
|
||||
|
||||
[plugins."sample@test"]
|
||||
enabled = true
|
||||
"#,
|
||||
)?;
|
||||
|
||||
let requirements = codex_config::ConfigRequirementsToml {
|
||||
plugins: Some(BTreeMap::from([(
|
||||
"sample@test".to_string(),
|
||||
codex_config::PluginRequirementsToml {
|
||||
mcp_servers: Some(BTreeMap::from([(
|
||||
"sample".to_string(),
|
||||
McpServerRequirement {
|
||||
identity: McpServerIdentity::Url {
|
||||
url: "https://sample.example/mcp".to_string(),
|
||||
},
|
||||
},
|
||||
)])),
|
||||
},
|
||||
)])),
|
||||
..Default::default()
|
||||
};
|
||||
let config = ConfigBuilder::default()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.cloud_requirements(CloudRequirementsLoader::new(async move {
|
||||
Ok(Some(requirements))
|
||||
}))
|
||||
.build()
|
||||
.await?;
|
||||
let plugins_manager = PluginsManager::new(codex_home.path().to_path_buf());
|
||||
let mcp_config = config.to_mcp_config(&plugins_manager).await;
|
||||
|
||||
assert_eq!(
|
||||
mcp_config
|
||||
.configured_mcp_servers
|
||||
.get("sample")
|
||||
.map(|server| (server.enabled, server.disabled_reason.clone())),
|
||||
Some((true, None))
|
||||
);
|
||||
assert_eq!(
|
||||
mcp_config
|
||||
.configured_mcp_servers
|
||||
.get("unlisted")
|
||||
.map(|server| (server.enabled, server.disabled_reason.clone())),
|
||||
Some((
|
||||
false,
|
||||
Some(McpServerDisabledReason::Requirements {
|
||||
source: RequirementSource::CloudRequirements,
|
||||
})
|
||||
))
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn to_mcp_config_empty_mcp_requirements_disable_plugin_mcps() -> anyhow::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
let plugin_root = codex_home
|
||||
.path()
|
||||
.join("plugins/cache")
|
||||
.join("test/sample/local");
|
||||
std::fs::create_dir_all(plugin_root.join(".codex-plugin"))?;
|
||||
std::fs::write(
|
||||
plugin_root.join(".codex-plugin/plugin.json"),
|
||||
r#"{"name":"sample"}"#,
|
||||
)?;
|
||||
std::fs::write(
|
||||
plugin_root.join(".mcp.json"),
|
||||
r#"{
|
||||
"mcpServers": {
|
||||
"sample": {
|
||||
"type": "http",
|
||||
"url": "https://sample.example/mcp"
|
||||
}
|
||||
}
|
||||
}"#,
|
||||
)?;
|
||||
std::fs::write(
|
||||
codex_home.path().join(CONFIG_TOML_FILE),
|
||||
r#"
|
||||
[features]
|
||||
plugins = true
|
||||
|
||||
[plugins."sample@test"]
|
||||
enabled = true
|
||||
"#,
|
||||
)?;
|
||||
|
||||
let requirements = codex_config::ConfigRequirementsToml {
|
||||
mcp_servers: Some(BTreeMap::new()),
|
||||
..Default::default()
|
||||
};
|
||||
let config = ConfigBuilder::default()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.cloud_requirements(CloudRequirementsLoader::new(async move {
|
||||
Ok(Some(requirements))
|
||||
}))
|
||||
.build()
|
||||
.await?;
|
||||
let plugins_manager = PluginsManager::new(codex_home.path().to_path_buf());
|
||||
let mcp_config = config.to_mcp_config(&plugins_manager).await;
|
||||
|
||||
assert_eq!(
|
||||
mcp_config
|
||||
.configured_mcp_servers
|
||||
.get("sample")
|
||||
.map(|server| (server.enabled, server.disabled_reason.clone())),
|
||||
Some((
|
||||
false,
|
||||
Some(McpServerDisabledReason::Requirements {
|
||||
source: RequirementSource::CloudRequirements,
|
||||
})
|
||||
))
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn add_dir_override_extends_workspace_writable_roots() -> std::io::Result<()> {
|
||||
let temp_dir = TempDir::new()?;
|
||||
@@ -6499,6 +6751,7 @@ async fn test_requirements_web_search_mode_allowlist_does_not_warn_when_unset()
|
||||
feature_requirements: None,
|
||||
hooks: None,
|
||||
mcp_servers: None,
|
||||
plugins: None,
|
||||
apps: None,
|
||||
rules: None,
|
||||
enforce_residency: None,
|
||||
@@ -7210,6 +7463,7 @@ async fn explicit_sandbox_mode_falls_back_when_disallowed_by_requirements() -> s
|
||||
feature_requirements: None,
|
||||
hooks: None,
|
||||
mcp_servers: None,
|
||||
plugins: None,
|
||||
apps: None,
|
||||
rules: None,
|
||||
enforce_residency: None,
|
||||
|
||||
@@ -18,6 +18,7 @@ use codex_config::FeatureRequirementsToml;
|
||||
use codex_config::LoaderOverrides;
|
||||
use codex_config::McpServerIdentity;
|
||||
use codex_config::McpServerRequirement;
|
||||
use codex_config::PluginRequirementsToml;
|
||||
use codex_config::ResidencyRequirement;
|
||||
use codex_config::SandboxModeRequirement;
|
||||
use codex_config::Sourced;
|
||||
@@ -975,8 +976,26 @@ impl Config {
|
||||
) -> McpConfig {
|
||||
let loaded_plugins = plugins_manager.plugins_for_config(self).await;
|
||||
let mut configured_mcp_servers = self.mcp_servers.get().clone();
|
||||
for (name, plugin_server) in loaded_plugins.effective_mcp_servers() {
|
||||
configured_mcp_servers.entry(name).or_insert(plugin_server);
|
||||
for plugin in loaded_plugins
|
||||
.plugins()
|
||||
.iter()
|
||||
.filter(|plugin| plugin.is_active())
|
||||
{
|
||||
let mut plugin_mcp_servers = plugin.mcp_servers.clone();
|
||||
filter_plugin_mcp_servers_by_requirements(
|
||||
&plugin.config_name,
|
||||
&mut plugin_mcp_servers,
|
||||
self.config_layer_stack.requirements().plugins.as_ref(),
|
||||
);
|
||||
for (name, plugin_server) in plugin_mcp_servers {
|
||||
configured_mcp_servers.entry(name).or_insert(plugin_server);
|
||||
}
|
||||
}
|
||||
if let Some(mcp_requirements) = self.config_layer_stack.requirements().mcp_servers.as_ref()
|
||||
&& mcp_requirements.value.is_empty()
|
||||
{
|
||||
// A present empty allowlist bans all MCPs, including plugin MCPs merged above.
|
||||
filter_mcp_servers_by_requirements(&mut configured_mcp_servers, Some(mcp_requirements));
|
||||
}
|
||||
|
||||
McpConfig {
|
||||
@@ -1186,6 +1205,35 @@ fn filter_mcp_servers_by_requirements(
|
||||
}
|
||||
}
|
||||
|
||||
fn filter_plugin_mcp_servers_by_requirements(
|
||||
plugin_config_name: &str,
|
||||
mcp_servers: &mut HashMap<String, McpServerConfig>,
|
||||
plugin_requirements: Option<&Sourced<BTreeMap<String, PluginRequirementsToml>>>,
|
||||
) {
|
||||
let Some(requirements) = plugin_requirements else {
|
||||
return;
|
||||
};
|
||||
let source = requirements.source.clone();
|
||||
let plugin_mcp_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));
|
||||
if allowed {
|
||||
server.disabled_reason = None;
|
||||
} else {
|
||||
server.enabled = false;
|
||||
server.disabled_reason = Some(McpServerDisabledReason::Requirements {
|
||||
source: source.clone(),
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn constrain_mcp_servers(
|
||||
mcp_servers: HashMap<String, McpServerConfig>,
|
||||
mcp_requirements: Option<&Sourced<BTreeMap<String, McpServerRequirement>>>,
|
||||
@@ -1876,6 +1924,7 @@ impl Config {
|
||||
feature_requirements,
|
||||
managed_hooks: _,
|
||||
mcp_servers,
|
||||
plugins: _,
|
||||
exec_policy: _,
|
||||
enforce_residency,
|
||||
network: network_requirements,
|
||||
|
||||
Reference in new issue
Block a user