From 0bc7ee91937d5cb9ff0e6a77e35cc89a91a81892 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Preet=20=F0=9F=9A=80?= <114741744+preetDev004@users.noreply.github.com> Date: Wed, 16 Jul 2025 19:00:39 -0400 Subject: [PATCH] Added mcp-server name validation (#1591) This PR implements server name validation for MCP (Model Context Protocol) servers to ensure they conform to the required pattern ^[a-zA-Z0-9_-]+$. This addresses the TODO comment in mcp_connection_manager.rs:82. + Added validation before spawning MCP client tasks + Invalid server names are added to errors map with descriptive messages I have read the CLA Document and I hereby sign the CLA --------- Co-authored-by: Michael Bolin --- codex-rs/core/src/mcp_connection_manager.rs | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/codex-rs/core/src/mcp_connection_manager.rs b/codex-rs/core/src/mcp_connection_manager.rs index 6ae1865f..7cf67627 100644 --- a/codex-rs/core/src/mcp_connection_manager.rs +++ b/codex-rs/core/src/mcp_connection_manager.rs @@ -79,9 +79,19 @@ impl McpConnectionManager { // Launch all configured servers concurrently. let mut join_set = JoinSet::new(); + let mut errors = ClientStartErrors::new(); for (server_name, cfg) in mcp_servers { - // TODO: Verify server name: require `^[a-zA-Z0-9_-]+$`? + // Validate server name before spawning + if !is_valid_mcp_server_name(&server_name) { + let error = anyhow::anyhow!( + "invalid server name '{}': must match pattern ^[a-zA-Z0-9_-]+$", + server_name + ); + errors.insert(server_name, error); + continue; + } + join_set.spawn(async move { let McpServerConfig { command, args, env } = cfg; let client_res = McpClient::new_stdio_client(command, args, env).await; @@ -117,7 +127,6 @@ impl McpConnectionManager { let mut clients: HashMap> = HashMap::with_capacity(join_set.len()); - let mut errors = ClientStartErrors::new(); while let Some(res) = join_set.join_next().await { let (server_name, client_res) = res?; // JoinError propagation @@ -208,3 +217,10 @@ pub async fn list_all_tools( Ok(aggregated) } + +fn is_valid_mcp_server_name(server_name: &str) -> bool { + !server_name.is_empty() + && server_name + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-') +}