From 233e4c4e82aa03af86bd3050aad1d6807475e275 Mon Sep 17 00:00:00 2001 From: serrrfirat Date: Mon, 13 Apr 2026 15:48:44 +0300 Subject: [PATCH 1/5] fix(mcp): validate server names with strict allowlist (fixes #1882) MCP server names are interpolated into secret keys, tool name prefixes, and provider tags. Without validation, shell metacharacters (;|&`$), path separators (/\), dots, and other special characters in server names could enable injection attacks. This adds an allowlist-based check in McpServerConfig::validate() that only permits [a-zA-Z0-9_-]. Dots are excluded because LLM providers require tool names to match ^[a-zA-Z0-9_-]+$ and server names are used as tool name prefixes. To avoid breaking users with legacy names (e.g. "My Server"), load_mcp_servers_from() and load_mcp_servers_from_db() now skip invalid entries with a tracing::warn instead of failing the entire config load. Supersedes #1941 and incorporates its review feedback (removing dots from the allowlist, graceful degradation on invalid names). Co-Authored-By: Claude Opus 4.6 (1M context) --- src/tools/mcp/config.rs | 187 ++++++++++++++++++++++++++++++++++------ 1 file changed, 163 insertions(+), 24 deletions(-) diff --git a/src/tools/mcp/config.rs b/src/tools/mcp/config.rs index f913c62b03..a98cc1c186 100644 --- a/src/tools/mcp/config.rs +++ b/src/tools/mcp/config.rs @@ -165,6 +165,27 @@ impl McpServerConfig { }); } + // Allowlist: alphanumeric, dash, underscore. + // Rejects shell metacharacters (;|&`$), path separators (/\), + // dots (LLM providers require tool names match ^[a-zA-Z0-9_-]+$ + // and server names are used as tool name prefixes), null bytes, + // spaces, and other dangerous characters that could cause injection + // when names are interpolated into secret keys, tool name prefixes, + // or provider tags. + if !self + .name + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_') + { + return Err(ConfigError::InvalidConfig { + reason: format!( + "Server name '{}' contains invalid characters \ + (only alphanumeric, dash, underscore are allowed)", + self.name + ), + }); + } + match self.effective_transport() { EffectiveTransport::Http => { if self.url.is_empty() { @@ -541,14 +562,23 @@ pub async fn load_mcp_servers_from(path: impl AsRef) -> Result { let config: McpServersFile = serde_json::from_value(value)?; - // Validate every server on load so corrupted DB configs are caught early + // Validate every server on load. Invalid entries are skipped + // with a warning to avoid breaking all MCP integrations when + // legacy names don't pass tightened validation. + let mut valid = McpServersFile::default(); for server in &config.servers { - server.validate().map_err(|e| ConfigError::InvalidConfig { - reason: format!("Server '{}': {}", server.name, e), - })?; + if let Err(e) = server.validate() { + tracing::warn!( + server_name = %server.name, + "Skipping MCP server with invalid DB config: {e}" + ); + continue; + } + valid.servers.push(server.clone()); } - Ok(config) + Ok(valid) } Ok(None) => { // No entry in DB, fall back to disk @@ -887,31 +925,36 @@ mod tests { } #[tokio::test] - async fn test_load_rejects_corrupted_headers() { + async fn test_load_skips_corrupted_headers() { let dir = tempdir().unwrap(); let path = dir.path().join("mcp-servers.json"); // Write a config with an invalid header name directly to disk, // bypassing the add_mcp_server() validation path. let corrupted = serde_json::json!({ - "servers": [{ - "name": "bad-server", - "url": "https://mcp.example.com", - "enabled": true, - "headers": { "X Bad": "value" } - }] + "servers": [ + { + "name": "bad-server", + "url": "https://mcp.example.com", + "enabled": true, + "headers": { "X Bad": "value" } + }, + { + "name": "good-server", + "url": "https://mcp.good.com", + "enabled": true, + "headers": {} + } + ] }); tokio::fs::write(&path, corrupted.to_string()) .await .unwrap(); - let result = load_mcp_servers_from(&path).await; - assert!(result.is_err(), "Load should reject corrupted headers"); - let err = result.unwrap_err().to_string(); - assert!( - err.contains("bad-server"), - "Error should name the offending server, got: {err}" - ); + // Invalid entries are skipped, valid ones are kept + let result = load_mcp_servers_from(&path).await.unwrap(); + assert_eq!(result.servers.len(), 1); + assert_eq!(result.servers[0].name, "good-server"); } #[test] @@ -1445,4 +1488,100 @@ mod tests { std::env::remove_var("NEARAI_API_KEY"); } } + + #[test] + fn test_server_name_valid_characters_accepted() { + // Alphanumeric, dashes, and underscores are all valid + for name in ["notion", "my-server", "my_server", "MCP-1"] { + let config = McpServerConfig::new(name, "https://mcp.example.com"); + assert!( + config.validate().is_ok(), + "Name '{}' should be accepted", + name + ); + } + } + + #[test] + fn test_server_name_shell_metacharacters_rejected() { + let dangerous_names = [ + "server; rm -rf /", + "server$(whoami)", + "server`id`", + "server|cat /etc/passwd", + "server&bg", + "server>out", + "server Date: Wed, 15 Apr 2026 03:29:28 +0900 Subject: [PATCH 2/5] style: cargo fmt Co-Authored-By: Claude Opus 4.6 (1M context) --- src/tools/mcp/config.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/tools/mcp/config.rs b/src/tools/mcp/config.rs index a98cc1c186..3adfe85f89 100644 --- a/src/tools/mcp/config.rs +++ b/src/tools/mcp/config.rs @@ -1575,9 +1575,7 @@ mod tests { } ] }); - tokio::fs::write(&path, mixed.to_string()) - .await - .unwrap(); + tokio::fs::write(&path, mixed.to_string()).await.unwrap(); // Invalid entry is skipped, valid one is kept let result = load_mcp_servers_from(&path).await.unwrap(); From 5333cd96044f3ec5072d125b496cfa6240db3a32 Mon Sep 17 00:00:00 2001 From: serrrfirat Date: Tue, 14 Apr 2026 22:31:51 +0300 Subject: [PATCH 3/5] fix(mcp): preserve schema_version when filtering invalid servers McpServersFile::default() sets schema_version to 0 (u32::default), but configs loaded from JSON get schema_version 1 via serde default. The server-filtering logic in load_mcp_servers_from() and load_mcp_servers_from_db() was using McpServersFile::default(), silently downgrading schema_version from 1 to 0 on every load-filter-save cycle (e.g. via bootstrap_nearai_mcp_server). Co-Authored-By: Claude Opus 4.6 (1M context) --- src/tools/mcp/config.rs | 44 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 42 insertions(+), 2 deletions(-) diff --git a/src/tools/mcp/config.rs b/src/tools/mcp/config.rs index 3adfe85f89..3ee06ecfea 100644 --- a/src/tools/mcp/config.rs +++ b/src/tools/mcp/config.rs @@ -566,7 +566,10 @@ pub async fn load_mcp_servers_from(path: impl AsRef) -> Result Date: Wed, 15 Apr 2026 17:39:20 +0300 Subject: [PATCH 4/5] refactor(mcp): use Vec::retain for server filtering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace manual for-loop with `retain` as suggested in review — simpler and avoids constructing a new McpServersFile struct. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/tools/mcp/config.rs | 34 ++++++++++++++-------------------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/src/tools/mcp/config.rs b/src/tools/mcp/config.rs index 3ee06ecfea..ea57c9e159 100644 --- a/src/tools/mcp/config.rs +++ b/src/tools/mcp/config.rs @@ -560,28 +560,25 @@ pub async fn load_mcp_servers_from(path: impl AsRef) -> Result Result { match store.get_setting(user_id, "mcp_servers").await { Ok(Some(value)) => { - let config: McpServersFile = serde_json::from_value(value)?; + let mut config: McpServersFile = serde_json::from_value(value)?; // Validate every server on load. Invalid entries are skipped // with a warning to avoid breaking all MCP integrations when // legacy names don't pass tightened validation. - let mut valid = McpServersFile { - servers: Vec::new(), - schema_version: config.schema_version, - }; - for server in &config.servers { + config.servers.retain(|server| { if let Err(e) = server.validate() { tracing::warn!( server_name = %server.name, "Skipping MCP server with invalid DB config: {e}" ); - continue; + false + } else { + true } - valid.servers.push(server.clone()); - } - Ok(valid) + }); + Ok(config) } Ok(None) => { // No entry in DB, fall back to disk From 2ae541a6c5589e557bfc92c4a113598eecb6978c Mon Sep 17 00:00:00 2001 From: serrrfirat Date: Fri, 17 Apr 2026 08:53:42 +0300 Subject: [PATCH 5/5] =?UTF-8?q?fix:=20resolve=20CI=20failures=20=E2=80=94?= =?UTF-8?q?=20clippy=20too=5Fmany=5Farguments=20+=20restore=20merge-lost?= =?UTF-8?q?=20install-param=20extraction?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add #[allow(clippy::too_many_arguments)] to register_startup_channels - Restore tool_install/tool_activate/tool_auth parameter extraction block in pending_gate_extension_name that was lost during staging merge Co-Authored-By: Claude Opus 4.6 (1M context) --- src/channels/wasm/setup.rs | 1 + src/channels/web/server.rs | 14 ++++++++++++++ 2 files changed, 15 insertions(+) diff --git a/src/channels/wasm/setup.rs b/src/channels/wasm/setup.rs index 64e559f450..83cde31afa 100644 --- a/src/channels/wasm/setup.rs +++ b/src/channels/wasm/setup.rs @@ -141,6 +141,7 @@ pub async fn setup_wasm_channels( }) } +#[allow(clippy::too_many_arguments)] async fn register_startup_channels( loaded_channels: Vec, config: &Config, diff --git a/src/channels/web/server.rs b/src/channels/web/server.rs index 3c4f0ed19d..c57cf03c0c 100644 --- a/src/channels/web/server.rs +++ b/src/channels/web/server.rs @@ -2896,6 +2896,20 @@ async fn pending_gate_extension_name( ); } + if matches!( + tool_name, + "tool_install" + | "tool-install" + | "tool_activate" + | "tool-activate" + | "tool_auth" + | "tool-auth" + ) && let Some(name) = parsed_parameters.get("name").and_then(|v| v.as_str()) + && !name.trim().is_empty() + { + return Some(name.to_string()); + } + if let Some(tools) = state.tool_registry.as_ref() && let Some(name) = tools.provider_extension_for_tool(tool_name).await {