From 4157b14280055a67c0374686229f49add9c24c2a Mon Sep 17 00:00:00 2001 From: jif-oai Date: Fri, 12 Jun 2026 14:38:18 +0200 Subject: [PATCH 1/2] Extract shared plugin MCP config parsing --- codex-rs/Cargo.lock | 1 + codex-rs/codex-mcp/src/lib.rs | 5 + codex-rs/codex-mcp/src/plugin_config.rs | 225 ++++++++++++++ codex-rs/codex-mcp/src/plugin_config_tests.rs | 278 ++++++++++++++++++ codex-rs/core-plugins/Cargo.toml | 1 + codex-rs/core-plugins/src/loader.rs | 129 ++------ codex-rs/core-plugins/src/loader_tests.rs | 76 ----- 7 files changed, 529 insertions(+), 186 deletions(-) create mode 100644 codex-rs/codex-mcp/src/plugin_config.rs create mode 100644 codex-rs/codex-mcp/src/plugin_config_tests.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index db8b8e396f14..4c22f4a66a36 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2739,6 +2739,7 @@ dependencies = [ "codex-git-utils", "codex-hooks", "codex-login", + "codex-mcp", "codex-model-provider", "codex-otel", "codex-plugin", diff --git a/codex-rs/codex-mcp/src/lib.rs b/codex-rs/codex-mcp/src/lib.rs index a270eb4e7ec5..e50c244129dc 100644 --- a/codex-rs/codex-mcp/src/lib.rs +++ b/codex-rs/codex-mcp/src/lib.rs @@ -42,6 +42,10 @@ pub use mcp::effective_mcp_servers_from_configured; pub use mcp::host_owned_codex_apps_enabled; pub use mcp::hosted_plugin_runtime_mcp_server_config; pub use mcp::tool_plugin_provenance; +pub use plugin_config::PluginMcpConfigParseOutcome; +pub use plugin_config::PluginMcpServerParseError; +pub use plugin_config::PluginMcpServerPlacement; +pub use plugin_config::parse_plugin_mcp_config; pub use mcp::McpServerStatusSnapshot; pub use mcp::McpSnapshotDetail; @@ -70,6 +74,7 @@ pub(crate) mod codex_apps; pub(crate) mod connection_manager; pub(crate) mod elicitation; pub(crate) mod mcp; +mod plugin_config; mod resource_client; pub(crate) mod rmcp_client; pub(crate) mod runtime; diff --git a/codex-rs/codex-mcp/src/plugin_config.rs b/codex-rs/codex-mcp/src/plugin_config.rs new file mode 100644 index 000000000000..7ddbfa91209c --- /dev/null +++ b/codex-rs/codex-mcp/src/plugin_config.rs @@ -0,0 +1,225 @@ +use codex_config::McpServerConfig; +use codex_config::McpServerEnvVar; +use codex_config::McpServerTransportConfig; +use serde::Deserialize; +use serde_json::Map as JsonMap; +use serde_json::Value as JsonValue; +use std::collections::BTreeMap; +use std::path::Component; +use std::path::Path; +use std::path::PathBuf; +use tracing::warn; + +/// Placement applied while normalizing MCP servers declared by a plugin. +#[derive(Clone, Copy, Debug)] +pub enum PluginMcpServerPlacement<'a> { + /// Preserve declared placement, resolving a relative working directory below the plugin root. + Declared, + /// Bind stdio servers to one environment and default their working directory to the plugin root. + Environment { environment_id: &'a str }, +} + +/// One plugin MCP server that could not be normalized into runtime configuration. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct PluginMcpServerParseError { + pub name: String, + pub message: String, +} + +/// Valid servers and per-server errors parsed from one plugin MCP file. +#[derive(Debug, Default, PartialEq)] +pub struct PluginMcpConfigParseOutcome { + pub servers: BTreeMap, + pub errors: Vec, +} + +#[derive(Debug, Default, Deserialize)] +#[serde(rename_all = "camelCase")] +struct PluginMcpServersFile { + mcp_servers: BTreeMap, +} + +#[derive(Debug, Deserialize)] +#[serde(untagged)] +enum PluginMcpFile { + McpServersObject(PluginMcpServersFile), + ServerMap(BTreeMap), +} + +impl PluginMcpFile { + fn into_mcp_servers(self) -> BTreeMap { + match self { + Self::McpServersObject(file) => file.mcp_servers, + Self::ServerMap(mcp_servers) => mcp_servers, + } + } +} + +/// Parses the two supported plugin MCP file shapes and normalizes each server. +/// +/// Invalid individual servers are returned as errors without discarding valid +/// siblings. A malformed top-level document fails the whole parse. +pub fn parse_plugin_mcp_config( + plugin_root: &Path, + contents: &str, + placement: PluginMcpServerPlacement<'_>, +) -> Result { + let parsed = serde_json::from_str::(contents)?; + let mut outcome = PluginMcpConfigParseOutcome::default(); + + for (name, config_value) in parsed.into_mcp_servers() { + match normalize_plugin_mcp_server(plugin_root, config_value, placement) { + Ok(config) => { + outcome.servers.insert(name, config); + } + Err(message) => outcome + .errors + .push(PluginMcpServerParseError { name, message }), + } + } + + Ok(outcome) +} + +fn normalize_plugin_mcp_server( + plugin_root: &Path, + value: JsonValue, + placement: PluginMcpServerPlacement<'_>, +) -> Result { + let mut object = normalize_plugin_mcp_server_value(plugin_root, value, placement); + if let PluginMcpServerPlacement::Environment { environment_id } = placement { + object.insert( + "environment_id".to_string(), + JsonValue::String(environment_id.to_string()), + ); + if object.contains_key("command") { + match object.remove("cwd") { + Some(JsonValue::String(cwd)) => object.insert( + "cwd".to_string(), + JsonValue::String( + executor_plugin_cwd(plugin_root, &cwd)? + .to_string_lossy() + .into_owned(), + ), + ), + Some(value) => object.insert("cwd".to_string(), value), + None => object.insert( + "cwd".to_string(), + JsonValue::String(plugin_root.to_string_lossy().into_owned()), + ), + }; + } + } + + let mut config = serde_json::from_value::(JsonValue::Object(object)) + .map_err(|err| err.to_string())?; + if matches!(placement, PluginMcpServerPlacement::Environment { .. }) + && !config.is_local_environment() + { + bind_remote_executor_env_vars(&mut config)?; + } + Ok(config) +} + +fn executor_plugin_cwd(plugin_root: &Path, configured_cwd: &str) -> Result { + let cwd = Path::new(configured_cwd); + if cwd.is_absolute() { + return Ok(cwd.to_path_buf()); + } + if cwd.components().any(|component| { + matches!( + component, + Component::ParentDir | Component::RootDir | Component::Prefix(_) + ) + }) { + return Err(format!( + "relative cwd `{configured_cwd}` must remain within plugin root `{}`", + plugin_root.display() + )); + } + Ok(plugin_root.join(cwd)) +} + +fn bind_remote_executor_env_vars(config: &mut McpServerConfig) -> Result<(), String> { + let McpServerTransportConfig::Stdio { env_vars, .. } = &mut config.transport else { + return Ok(()); + }; + for env_var in env_vars { + match env_var { + McpServerEnvVar::Name(name) => { + *env_var = McpServerEnvVar::Config { + name: std::mem::take(name), + source: Some("remote".to_string()), + }; + } + McpServerEnvVar::Config { name, source } => match source.as_deref() { + None => *source = Some("remote".to_string()), + Some("remote") => {} + Some("local") => { + return Err(format!( + "env_vars entry `{name}` cannot use source `local` in an executor-owned plugin" + )); + } + Some(source) => unreachable!("validated env_vars source `{source}`"), + }, + } + } + Ok(()) +} + +fn normalize_plugin_mcp_server_value( + plugin_root: &Path, + value: JsonValue, + placement: PluginMcpServerPlacement<'_>, +) -> JsonMap { + let mut object = match value { + JsonValue::Object(object) => object, + _ => return JsonMap::new(), + }; + + if let Some(JsonValue::String(transport_type)) = object.remove("type") { + match transport_type.as_str() { + "http" | "streamable_http" | "streamable-http" | "stdio" => {} + other => { + warn!( + plugin = %plugin_root.display(), + transport = other, + "plugin MCP server uses an unknown transport type" + ); + } + } + } + + if let Some(JsonValue::Object(mut oauth)) = object.remove("oauth") { + if oauth.remove("callbackPort").is_some() { + warn!( + plugin = %plugin_root.display(), + "plugin MCP server OAuth callbackPort is ignored; Codex uses global MCP OAuth callback settings" + ); + } + + if let Some(client_id) = oauth.remove("clientId") { + oauth.entry("client_id".to_string()).or_insert(client_id); + } + + if !oauth.is_empty() { + object.insert("oauth".to_string(), JsonValue::Object(oauth)); + } + } + + if matches!(placement, PluginMcpServerPlacement::Declared) + && let Some(JsonValue::String(cwd)) = object.get("cwd") + && !Path::new(cwd).is_absolute() + { + object.insert( + "cwd".to_string(), + JsonValue::String(plugin_root.join(cwd).display().to_string()), + ); + } + + object +} + +#[cfg(test)] +#[path = "plugin_config_tests.rs"] +mod tests; diff --git a/codex-rs/codex-mcp/src/plugin_config_tests.rs b/codex-rs/codex-mcp/src/plugin_config_tests.rs new file mode 100644 index 000000000000..8491923c9d07 --- /dev/null +++ b/codex-rs/codex-mcp/src/plugin_config_tests.rs @@ -0,0 +1,278 @@ +use super::PluginMcpConfigParseOutcome; +use super::PluginMcpServerParseError; +use super::PluginMcpServerPlacement; +use super::parse_plugin_mcp_config; +use codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID; +use codex_config::McpServerConfig; +use codex_config::McpServerEnvVar; +use codex_config::McpServerOAuthConfig; +use codex_config::McpServerTransportConfig; +use pretty_assertions::assert_eq; +use std::collections::BTreeMap; +use std::collections::HashMap; +use std::path::Path; +use std::path::PathBuf; + +fn plugin_root() -> PathBuf { + std::env::current_dir() + .expect("current directory") + .join("plugin-root") +} + +fn stdio_server( + command: &str, + environment_id: &str, + cwd: &Path, + env_vars: Vec, +) -> McpServerConfig { + McpServerConfig { + transport: McpServerTransportConfig::Stdio { + command: command.to_string(), + args: Vec::new(), + env: None, + env_vars, + cwd: Some(cwd.to_path_buf()), + }, + environment_id: environment_id.to_string(), + enabled: true, + required: false, + supports_parallel_tool_calls: false, + disabled_reason: None, + startup_timeout_sec: None, + tool_timeout_sec: None, + default_tools_approval_mode: None, + enabled_tools: None, + disabled_tools: None, + scopes: None, + oauth: None, + oauth_resource: None, + tools: HashMap::new(), + } +} + +#[test] +fn declared_placement_preserves_local_plugin_normalization() { + let plugin_root = plugin_root(); + let expected_stdio = stdio_server( + "demo-mcp", + "configured-environment", + &plugin_root.join("scripts"), + Vec::new(), + ); + let expected_http = McpServerConfig { + transport: McpServerTransportConfig::StreamableHttp { + url: "https://example.com/mcp".to_string(), + bearer_token_env_var: None, + http_headers: None, + env_http_headers: None, + }, + environment_id: DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), + enabled: true, + required: false, + supports_parallel_tool_calls: false, + disabled_reason: None, + startup_timeout_sec: None, + tool_timeout_sec: None, + default_tools_approval_mode: None, + enabled_tools: None, + disabled_tools: None, + scopes: None, + oauth: Some(McpServerOAuthConfig { + client_id: Some("client-id".to_string()), + }), + oauth_resource: None, + tools: HashMap::new(), + }; + + let outcome = parse_plugin_mcp_config( + &plugin_root, + r#"{ + "demo": { + "type": "stdio", + "command": "demo-mcp", + "environment_id": "configured-environment", + "cwd": "scripts" + }, + "hosted": { + "type": "http", + "url": "https://example.com/mcp", + "oauth": {"clientId": "client-id", "callbackPort": 9876} + } + }"#, + PluginMcpServerPlacement::Declared, + ) + .expect("parse plugin MCP config"); + + assert_eq!( + outcome, + PluginMcpConfigParseOutcome { + servers: BTreeMap::from([ + ("demo".to_string(), expected_stdio), + ("hosted".to_string(), expected_http), + ]), + errors: Vec::new(), + } + ); +} + +#[test] +fn environment_placement_forces_authority_and_defaults_cwd() { + let plugin_root = plugin_root(); + let outcome = parse_plugin_mcp_config( + &plugin_root, + r#"{ + "$schema":"https://example.com/plugin-mcp.schema.json", + "mcpServers":{"demo":{ + "command":"demo-mcp", + "environment_id":"local", + "env_vars":["EXECUTOR_TOKEN", {"name":"OTHER_TOKEN"}] + }} + }"#, + PluginMcpServerPlacement::Environment { + environment_id: "executor-1", + }, + ) + .expect("parse plugin MCP config"); + + assert_eq!( + outcome, + PluginMcpConfigParseOutcome { + servers: BTreeMap::from([( + "demo".to_string(), + stdio_server( + "demo-mcp", + "executor-1", + &plugin_root, + vec![ + McpServerEnvVar::Config { + name: "EXECUTOR_TOKEN".to_string(), + source: Some("remote".to_string()), + }, + McpServerEnvVar::Config { + name: "OTHER_TOKEN".to_string(), + source: Some("remote".to_string()), + }, + ], + ), + )]), + errors: Vec::new(), + } + ); +} + +#[test] +fn environment_placement_resolves_relative_cwd_beneath_plugin_root() { + let plugin_root = plugin_root(); + let outcome = parse_plugin_mcp_config( + &plugin_root, + r#"{"demo":{"command":"demo-mcp","cwd":"scripts"}}"#, + PluginMcpServerPlacement::Environment { + environment_id: "executor-1", + }, + ) + .expect("parse plugin MCP config"); + + assert_eq!( + outcome, + PluginMcpConfigParseOutcome { + servers: BTreeMap::from([( + "demo".to_string(), + stdio_server( + "demo-mcp", + "executor-1", + &plugin_root.join("scripts"), + Vec::new(), + ), + )]), + errors: Vec::new(), + } + ); +} + +#[test] +fn environment_placement_rejects_relative_cwd_that_escapes_package() { + let plugin_root = plugin_root(); + let outcome = parse_plugin_mcp_config( + &plugin_root, + r#"{"demo":{"command":"demo-mcp","cwd":"../outside"}}"#, + PluginMcpServerPlacement::Environment { + environment_id: "executor-1", + }, + ) + .expect("parse plugin MCP config"); + + assert_eq!( + outcome, + PluginMcpConfigParseOutcome { + servers: BTreeMap::new(), + errors: vec![PluginMcpServerParseError { + name: "demo".to_string(), + message: format!( + "relative cwd `../outside` must remain within plugin root `{}`", + plugin_root.display() + ), + }], + } + ); +} + +#[test] +fn environment_placement_rejects_orchestrator_env_vars() { + let plugin_root = plugin_root(); + let outcome = parse_plugin_mcp_config( + &plugin_root, + r#"{"demo":{"command":"demo-mcp","env_vars":[{"name":"TOKEN","source":"local"}]}}"#, + PluginMcpServerPlacement::Environment { + environment_id: "executor-1", + }, + ) + .expect("parse plugin MCP config"); + + assert_eq!( + outcome, + PluginMcpConfigParseOutcome { + servers: BTreeMap::new(), + errors: vec![PluginMcpServerParseError { + name: "demo".to_string(), + message: + "env_vars entry `TOKEN` cannot use source `local` in an executor-owned plugin" + .to_string(), + }], + } + ); +} + +#[test] +fn local_environment_placement_preserves_local_env_vars() { + let plugin_root = plugin_root(); + let outcome = parse_plugin_mcp_config( + &plugin_root, + r#"{"demo":{"command":"demo-mcp","env_vars":["TOKEN",{"name":"OTHER","source":"local"}]}}"#, + PluginMcpServerPlacement::Environment { + environment_id: DEFAULT_MCP_SERVER_ENVIRONMENT_ID, + }, + ) + .expect("parse plugin MCP config"); + + assert_eq!( + outcome, + PluginMcpConfigParseOutcome { + servers: BTreeMap::from([( + "demo".to_string(), + stdio_server( + "demo-mcp", + DEFAULT_MCP_SERVER_ENVIRONMENT_ID, + &plugin_root, + vec![ + McpServerEnvVar::Name("TOKEN".to_string()), + McpServerEnvVar::Config { + name: "OTHER".to_string(), + source: Some("local".to_string()), + }, + ], + ), + )]), + errors: Vec::new(), + } + ); +} diff --git a/codex-rs/core-plugins/Cargo.toml b/codex-rs/core-plugins/Cargo.toml index 7e0b1bf91e89..72064054ecd2 100644 --- a/codex-rs/core-plugins/Cargo.toml +++ b/codex-rs/core-plugins/Cargo.toml @@ -22,6 +22,7 @@ codex-exec-server = { workspace = true } codex-git-utils = { workspace = true } codex-hooks = { workspace = true } codex-login = { workspace = true } +codex-mcp = { workspace = true } codex-model-provider = { workspace = true } codex-otel = { workspace = true } codex-plugin = { workspace = true } diff --git a/codex-rs/core-plugins/src/loader.rs b/codex-rs/core-plugins/src/loader.rs index 1cc943017252..71f8c42f147f 100644 --- a/codex-rs/core-plugins/src/loader.rs +++ b/codex-rs/core-plugins/src/loader.rs @@ -22,6 +22,8 @@ use codex_core_skills::config_rules::skill_config_rules_from_stack; use codex_core_skills::loader::SkillRoot; use codex_core_skills::loader::load_skills_from_roots; use codex_exec_server::LOCAL_FS; +use codex_mcp::PluginMcpServerPlacement; +use codex_mcp::parse_plugin_mcp_config; use codex_plugin::AppConnectorId; use codex_plugin::LoadedPlugin; use codex_plugin::PluginCapabilitySummary; @@ -36,7 +38,6 @@ use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_plugins::find_plugin_manifest_path; use indexmap::IndexMap; use serde::Deserialize; -use serde_json::Map as JsonMap; use serde_json::Value as JsonValue; use std::collections::HashMap; use std::collections::HashSet; @@ -97,28 +98,6 @@ pub fn log_plugin_load_errors(outcome: &PluginLoadOutcome) { } } -#[derive(Debug, Default, Deserialize)] -#[serde(rename_all = "camelCase")] -struct PluginMcpServersFile { - mcp_servers: HashMap, -} - -#[derive(Debug, Deserialize)] -#[serde(untagged)] -enum PluginMcpFile { - McpServersObject(PluginMcpServersFile), - ServerMap(HashMap), -} - -impl PluginMcpFile { - fn into_mcp_servers(self) -> HashMap { - match self { - Self::McpServersObject(file) => file.mcp_servers, - Self::ServerMap(mcp_servers) => mcp_servers, - } - } -} - #[derive(Debug, Default, Deserialize)] #[serde(rename_all = "camelCase")] struct PluginAppFile { @@ -1156,99 +1135,29 @@ async fn load_mcp_servers_from_file( let Ok(contents) = tokio::fs::read_to_string(mcp_config_path.as_path()).await else { return PluginMcpDiscovery::default(); }; - let parsed = match serde_json::from_str::(&contents) { - Ok(parsed) => parsed, - Err(err) => { - warn!( - path = %mcp_config_path.display(), - "failed to parse plugin MCP config: {err}" - ); - return PluginMcpDiscovery::default(); - } - }; - normalize_plugin_mcp_servers( - plugin_root, - parsed.into_mcp_servers(), - mcp_config_path.to_string_lossy().as_ref(), - ) -} - -fn normalize_plugin_mcp_servers( - plugin_root: &Path, - plugin_mcp_servers: HashMap, - source: &str, -) -> PluginMcpDiscovery { - let mut mcp_servers = HashMap::new(); - - for (name, config_value) in plugin_mcp_servers { - let normalized = normalize_plugin_mcp_server_value(plugin_root, config_value); - match serde_json::from_value::(JsonValue::Object(normalized)) { - Ok(config) => { - mcp_servers.insert(name, config); - } + let parsed = + match parse_plugin_mcp_config(plugin_root, &contents, PluginMcpServerPlacement::Declared) { + Ok(parsed) => parsed, Err(err) => { warn!( - plugin = %plugin_root.display(), - server = name, - "failed to parse plugin MCP server from {source}: {err}" + path = %mcp_config_path.display(), + "failed to parse plugin MCP config: {err}" ); + return PluginMcpDiscovery::default(); } - } - } - - PluginMcpDiscovery { mcp_servers } -} - -fn normalize_plugin_mcp_server_value( - plugin_root: &Path, - value: JsonValue, -) -> JsonMap { - let mut object = match value { - JsonValue::Object(object) => object, - _ => return JsonMap::new(), - }; - - if let Some(JsonValue::String(transport_type)) = object.remove("type") { - match transport_type.as_str() { - "http" | "streamable_http" | "streamable-http" => {} - "stdio" => {} - other => { - warn!( - plugin = %plugin_root.display(), - transport = other, - "plugin MCP server uses an unknown transport type" - ); - } - } - } - - if let Some(JsonValue::Object(mut oauth)) = object.remove("oauth") { - if oauth.remove("callbackPort").is_some() { - warn!( - plugin = %plugin_root.display(), - "plugin MCP server OAuth callbackPort is ignored; Codex uses global MCP OAuth callback settings" - ); - } - - if let Some(client_id) = oauth.remove("clientId") { - oauth.entry("client_id".to_string()).or_insert(client_id); - } - - if !oauth.is_empty() { - object.insert("oauth".to_string(), JsonValue::Object(oauth)); - } - } - - if let Some(JsonValue::String(cwd)) = object.get("cwd") - && !Path::new(cwd).is_absolute() - { - object.insert( - "cwd".to_string(), - JsonValue::String(plugin_root.join(cwd).display().to_string()), + }; + for error in parsed.errors { + warn!( + plugin = %plugin_root.display(), + server = error.name, + path = %mcp_config_path.display(), + error = error.message, + "failed to parse plugin MCP server" ); } - - object + PluginMcpDiscovery { + mcp_servers: parsed.servers.into_iter().collect(), + } } #[derive(Debug, Default)] diff --git a/codex-rs/core-plugins/src/loader_tests.rs b/codex-rs/core-plugins/src/loader_tests.rs index 2e89776b4954..ddf1b66fecb4 100644 --- a/codex-rs/core-plugins/src/loader_tests.rs +++ b/codex-rs/core-plugins/src/loader_tests.rs @@ -218,82 +218,6 @@ enabled = true assert!(hooks_only_valid.apps.is_empty()); } -#[test] -fn plugin_mcp_file_supports_mcp_servers_object_format() { - let parsed = serde_json::from_str::( - r#"{ - "mcpServers": { - "sample": { - "command": "sample-mcp" - } - } -}"#, - ) - .expect("parse wrapped plugin mcp config") - .into_mcp_servers(); - - assert_eq!( - parsed, - HashMap::from([( - "sample".to_string(), - serde_json::json!({ - "command": "sample-mcp" - }), - )]) - ); -} - -#[test] -fn plugin_mcp_file_supports_mcp_servers_object_format_with_metadata() { - let parsed = serde_json::from_str::( - r#"{ - "$schema": "https://example.com/plugin-mcp.schema.json", - "mcpServers": { - "sample": { - "command": "sample-mcp" - } - } -}"#, - ) - .expect("parse plugin mcp config with metadata") - .into_mcp_servers(); - - assert_eq!( - parsed, - HashMap::from([( - "sample".to_string(), - serde_json::json!({ - "command": "sample-mcp" - }), - )]) - ); -} - -#[test] -fn plugin_mcp_file_supports_top_level_server_map_format() { - let parsed = serde_json::from_str::( - r#"{ - "linear": { - "type": "http", - "url": "https://mcp.linear.app/mcp" - } -}"#, - ) - .expect("parse flat plugin mcp config") - .into_mcp_servers(); - - assert_eq!( - parsed, - HashMap::from([( - "linear".to_string(), - serde_json::json!({ - "type": "http", - "url": "https://mcp.linear.app/mcp" - }), - )]) - ); -} - #[test] fn curated_plugin_cache_version_shortens_full_git_sha() { assert_eq!( From 2c491faf478bebfa1a7d9333dd8cc4cf66ec5591 Mon Sep 17 00:00:00 2001 From: jif-oai Date: Fri, 12 Jun 2026 14:55:07 +0200 Subject: [PATCH 2/2] Tighten executor plugin MCP normalization --- codex-rs/codex-mcp/src/plugin_config.rs | 41 +++++++++++-------- codex-rs/codex-mcp/src/plugin_config_tests.rs | 28 ++++++++++++- 2 files changed, 51 insertions(+), 18 deletions(-) diff --git a/codex-rs/codex-mcp/src/plugin_config.rs b/codex-rs/codex-mcp/src/plugin_config.rs index 7ddbfa91209c..7c53de3e4605 100644 --- a/codex-rs/codex-mcp/src/plugin_config.rs +++ b/codex-rs/codex-mcp/src/plugin_config.rs @@ -102,21 +102,19 @@ fn normalize_plugin_mcp_server( .into_owned(), ), ), - Some(value) => object.insert("cwd".to_string(), value), - None => object.insert( + Some(JsonValue::Null) | None => object.insert( "cwd".to_string(), JsonValue::String(plugin_root.to_string_lossy().into_owned()), ), + Some(value) => object.insert("cwd".to_string(), value), }; } } let mut config = serde_json::from_value::(JsonValue::Object(object)) .map_err(|err| err.to_string())?; - if matches!(placement, PluginMcpServerPlacement::Environment { .. }) - && !config.is_local_environment() - { - bind_remote_executor_env_vars(&mut config)?; + if matches!(placement, PluginMcpServerPlacement::Environment { .. }) { + bind_environment_env_vars(&mut config)?; } Ok(config) } @@ -140,28 +138,37 @@ fn executor_plugin_cwd(plugin_root: &Path, configured_cwd: &str) -> Result Result<(), String> { +fn bind_environment_env_vars(config: &mut McpServerConfig) -> Result<(), String> { + let is_local_environment = config.is_local_environment(); let McpServerTransportConfig::Stdio { env_vars, .. } = &mut config.transport else { return Ok(()); }; for env_var in env_vars { match env_var { - McpServerEnvVar::Name(name) => { + McpServerEnvVar::Name(name) if !is_local_environment => { *env_var = McpServerEnvVar::Config { name: std::mem::take(name), source: Some("remote".to_string()), }; } - McpServerEnvVar::Config { name, source } => match source.as_deref() { - None => *source = Some("remote".to_string()), - Some("remote") => {} - Some("local") => { - return Err(format!( - "env_vars entry `{name}` cannot use source `local` in an executor-owned plugin" - )); + McpServerEnvVar::Name(_) => {} + McpServerEnvVar::Config { name, source } => { + match (is_local_environment, source.as_deref()) { + (true, None | Some("local")) | (false, Some("remote")) => {} + (true, Some("remote")) => { + return Err(format!( + "env_vars entry `{name}` cannot use source `remote` in a local environment" + )); + } + (false, None) => *source = Some("remote".to_string()), + (false, Some("local")) => { + return Err(format!( + "env_vars entry `{name}` cannot use source `local` in an executor-owned plugin" + )); + } + (_, Some(source)) => unreachable!("validated env_vars source `{source}`"), } - Some(source) => unreachable!("validated env_vars source `{source}`"), - }, + } } } Ok(()) diff --git a/codex-rs/codex-mcp/src/plugin_config_tests.rs b/codex-rs/codex-mcp/src/plugin_config_tests.rs index 8491923c9d07..0ba8f42f3595 100644 --- a/codex-rs/codex-mcp/src/plugin_config_tests.rs +++ b/codex-rs/codex-mcp/src/plugin_config_tests.rs @@ -116,7 +116,7 @@ fn declared_placement_preserves_local_plugin_normalization() { } #[test] -fn environment_placement_forces_authority_and_defaults_cwd() { +fn environment_placement_forces_authority_and_defaults_null_cwd() { let plugin_root = plugin_root(); let outcome = parse_plugin_mcp_config( &plugin_root, @@ -125,6 +125,7 @@ fn environment_placement_forces_authority_and_defaults_cwd() { "mcpServers":{"demo":{ "command":"demo-mcp", "environment_id":"local", + "cwd":null, "env_vars":["EXECUTOR_TOKEN", {"name":"OTHER_TOKEN"}] }} }"#, @@ -276,3 +277,28 @@ fn local_environment_placement_preserves_local_env_vars() { } ); } + +#[test] +fn local_environment_placement_rejects_remote_env_vars() { + let plugin_root = plugin_root(); + let outcome = parse_plugin_mcp_config( + &plugin_root, + r#"{"demo":{"command":"demo-mcp","env_vars":[{"name":"TOKEN","source":"remote"}]}}"#, + PluginMcpServerPlacement::Environment { + environment_id: DEFAULT_MCP_SERVER_ENVIRONMENT_ID, + }, + ) + .expect("parse plugin MCP config"); + + assert_eq!( + outcome, + PluginMcpConfigParseOutcome { + servers: BTreeMap::new(), + errors: vec![PluginMcpServerParseError { + name: "demo".to_string(), + message: "env_vars entry `TOKEN` cannot use source `remote` in a local environment" + .to_string(), + }], + } + ); +}