From 2ba751ecad4957bab29dda4ec9a94c2bd4fc6294 Mon Sep 17 00:00:00 2001 From: willamhou Date: Fri, 3 Apr 2026 10:48:39 +0800 Subject: [PATCH 1/6] fix(mcp): validate server names with allowlist to prevent injection (fixes #1882) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Server name validation only checked for empty names. Names are interpolated into secret storage keys, tool name prefixes, and provider tags — a name containing shell metacharacters (;|&`$), path separators, or null bytes could cause injection. Switch from blocklist to allowlist: only alphanumeric, dash, underscore, and dot are allowed. Adds 5 regression tests covering shell metacharacters, path separators, null bytes, valid characters, and corrupted config load. Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude Co-Authored-By: Happy --- src/tools/mcp/config.rs | 86 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 82 insertions(+), 4 deletions(-) diff --git a/src/tools/mcp/config.rs b/src/tools/mcp/config.rs index 94b08a71ef8..e55e645941a 100644 --- a/src/tools/mcp/config.rs +++ b/src/tools/mcp/config.rs @@ -165,22 +165,23 @@ impl McpServerConfig { }); } - // Allowlist: alphanumeric, dash, underscore. + // Allowlist: lowercase 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, + // and server names are used as tool name prefixes), uppercase + // (canonicalize_extension_name() only accepts lowercase), 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 == '_') + .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || c == '-' || c == '_') { return Err(ConfigError::InvalidConfig { reason: format!( "Server name '{}' contains invalid characters \ - (only alphanumeric, dash, underscore are allowed)", + (only lowercase alphanumeric, dash, underscore are allowed)", self.name ), }); @@ -1134,6 +1135,83 @@ mod tests { assert!(!config.requires_auth()); } + #[test] + fn test_server_name_valid_characters_accepted() { + // Alphanumeric, dashes, underscores, dots are all valid + for name in ["notion", "my-server", "my_server", "server.local", "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: Fri, 3 Apr 2026 19:19:09 +0800 Subject: [PATCH 2/6] =?UTF-8?q?fix(mcp):=20address=20review=20=E2=80=94=20?= =?UTF-8?q?drop=20dot=20from=20allowlist,=20graceful=20load=20on=20invalid?= =?UTF-8?q?=20names?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove dot from server name allowlist: LLM providers require tool names match ^[a-zA-Z0-9_-]+$ and server names are used as tool name prefixes - Change load_mcp_servers_from() and load_mcp_servers_from_db() to skip invalid entries with a warning instead of failing the entire config, preventing legacy names from disabling all MCP integrations on upgrade - Add test_server_name_dot_rejected regression test - Update load tests to verify skip-invalid behavior with mixed configs Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude Co-Authored-By: Happy --- src/tools/mcp/config.rs | 49 ++++++++++++++++++++++++++--------------- 1 file changed, 31 insertions(+), 18 deletions(-) diff --git a/src/tools/mcp/config.rs b/src/tools/mcp/config.rs index e55e645941a..c5015d8445e 100644 --- a/src/tools/mcp/config.rs +++ b/src/tools/mcp/config.rs @@ -1137,8 +1137,8 @@ mod tests { #[test] fn test_server_name_valid_characters_accepted() { - // Alphanumeric, dashes, underscores, dots are all valid - for name in ["notion", "my-server", "my_server", "server.local", "MCP-1"] { + // Lowercase alphanumeric, dashes, underscores are 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(), @@ -1188,28 +1188,41 @@ mod tests { assert!(config.validate().is_err()); } + #[test] + fn test_server_name_dot_rejected() { + // Dots are rejected because server names are used as tool name + // prefixes and LLM providers require ^[a-zA-Z0-9_-]+$ + let config = McpServerConfig::new("server.local", "https://mcp.example.com"); + assert!(config.validate().is_err()); + } + #[tokio::test] - async fn test_load_rejects_corrupted_server_name() { + async fn test_load_skips_invalid_server_name() { let dir = tempdir().unwrap(); let path = dir.path().join("mcp-servers.json"); - let corrupted = serde_json::json!({ - "servers": [{ - "name": "bad;rm -rf /", - "url": "https://mcp.example.com", - "enabled": true, - "headers": {} - }] + // Mix of one invalid and one valid server + let mixed = serde_json::json!({ + "servers": [ + { + "name": "bad;rm -rf /", + "url": "https://mcp.example.com", + "enabled": true, + "headers": {} + }, + { + "name": "good-server", + "url": "https://mcp.good.com", + "enabled": true, + "headers": {} + } + ] }); - tokio::fs::write(&path, corrupted.to_string()) - .await - .unwrap(); + tokio::fs::write(&path, mixed.to_string()).await.unwrap(); - let result = load_mcp_servers_from(&path).await; - assert!( - result.is_err(), - "Load should reject server with dangerous name" - ); + let result = load_mcp_servers_from(&path).await.unwrap(); + assert_eq!(result.servers.len(), 1, "Should skip invalid, keep valid"); + assert_eq!(result.servers[0].name, "good-server"); } #[test] From 8c64726fe7bd19b50b628d3c29dff46d4374d2f7 Mon Sep 17 00:00:00 2001 From: willamhou Date: Wed, 15 Apr 2026 14:49:27 +0800 Subject: [PATCH 3/6] fix: restrict MCP server name allowlist to lowercase + preserve schema_version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address henrypark133 review feedback: 1. Restrict allowlist to lowercase — uppercase names pass validation but are silently dropped by canonicalize_extension_name() at runtime. 2. Preserve schema_version when filtering invalid servers on load, instead of resetting to default. [skip-regression-check] Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude Co-Authored-By: Happy --- src/tools/mcp/config.rs | 121 +++++++--------------------------------- 1 file changed, 20 insertions(+), 101 deletions(-) diff --git a/src/tools/mcp/config.rs b/src/tools/mcp/config.rs index c5015d8445e..ac8d13eb83a 100644 --- a/src/tools/mcp/config.rs +++ b/src/tools/mcp/config.rs @@ -166,13 +166,11 @@ impl McpServerConfig { } // Allowlist: lowercase 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), uppercase - // (canonicalize_extension_name() only accepts lowercase), null bytes, - // spaces, and other dangerous characters that could cause injection - // when names are interpolated into secret keys, tool name prefixes, - // or provider tags. + // Uppercase is rejected because canonicalize_extension_name() + // (extensions/naming.rs) only accepts lowercase — uppercase names + // pass validation but are silently dropped at runtime. + // Also rejects shell metacharacters (;|&`$), path separators (/\), + // dots, null bytes, spaces, and other dangerous characters. if !self .name .chars() @@ -1196,6 +1194,21 @@ mod tests { assert!(config.validate().is_err()); } + #[test] + fn test_server_name_uppercase_rejected() { + // Uppercase is rejected because canonicalize_extension_name() only + // accepts lowercase — uppercase names pass here but are silently + // dropped at runtime by the extension manager. + for name in ["MCP-1", "MyServer", "Notion"] { + let config = McpServerConfig::new(name, "https://mcp.example.com"); + assert!( + config.validate().is_err(), + "Uppercase name '{}' should be rejected", + name + ); + } + } + #[tokio::test] async fn test_load_skips_invalid_server_name() { let dir = tempdir().unwrap(); @@ -1586,100 +1599,6 @@ mod tests { } } - #[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: Sun, 19 Apr 2026 19:04:37 +0800 Subject: [PATCH 4/6] fix(mcp): use ExtensionName as single source of truth for server name validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the manual character allowlist in validate() with ExtensionName::new() (the canonical identity validator). This fixes three issues from review: 1. Catches edge cases the manual check missed: leading/trailing separators (-server, server_), consecutive separators (my--server, my__server), and bare separators (-, _). 2. Enforces 64-char length limit (MAX_NAME_LEN from ironclaw_common). 3. Single source of truth — validation rules are no longer duplicated between MCP config and the extension naming system. Add 6 regression tests covering leading/trailing separators, consecutive separators, bare separators, 64-char acceptance, and 65-char rejection. Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude Co-Authored-By: Happy --- src/tools/mcp/config.rs | 96 +++++++++++++++++++++++++++++------------ 1 file changed, 69 insertions(+), 27 deletions(-) diff --git a/src/tools/mcp/config.rs b/src/tools/mcp/config.rs index ac8d13eb83a..fe34184d1aa 100644 --- a/src/tools/mcp/config.rs +++ b/src/tools/mcp/config.rs @@ -9,6 +9,8 @@ use std::path::{Path, PathBuf}; use serde::{Deserialize, Serialize}; use tokio::fs; +use ironclaw_common::ExtensionName; + use crate::bootstrap::ironclaw_base_dir; use crate::tools::mcp::McpTool; use crate::tools::tool::ToolError; @@ -159,31 +161,14 @@ impl McpServerConfig { /// Validate the server configuration. pub fn validate(&self) -> Result<(), ConfigError> { - if self.name.is_empty() { - return Err(ConfigError::InvalidConfig { - reason: "Server name cannot be empty".to_string(), - }); - } - - // Allowlist: lowercase alphanumeric, dash, underscore. - // Uppercase is rejected because canonicalize_extension_name() - // (extensions/naming.rs) only accepts lowercase — uppercase names - // pass validation but are silently dropped at runtime. - // Also rejects shell metacharacters (;|&`$), path separators (/\), - // dots, null bytes, spaces, and other dangerous characters. - if !self - .name - .chars() - .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || c == '-' || c == '_') - { - return Err(ConfigError::InvalidConfig { - reason: format!( - "Server name '{}' contains invalid characters \ - (only lowercase alphanumeric, dash, underscore are allowed)", - self.name - ), - }); - } + // Delegate name validation to the canonical identity validator. + // This is the single source of truth for extension/server names and + // catches: empty, too-long (>64), uppercase, path traversal, + // leading/trailing separator, consecutive underscores/dashes, + // shell metacharacters, dots, null bytes, spaces, etc. + ExtensionName::new(&self.name).map_err(|e| ConfigError::InvalidConfig { + reason: format!("Invalid server name '{}': {e}", self.name), + })?; match self.effective_transport() { EffectiveTransport::Http => { @@ -1196,8 +1181,8 @@ mod tests { #[test] fn test_server_name_uppercase_rejected() { - // Uppercase is rejected because canonicalize_extension_name() only - // accepts lowercase — uppercase names pass here but are silently + // Uppercase is rejected because the canonical name validator + // only accepts lowercase — uppercase names would be silently // dropped at runtime by the extension manager. for name in ["MCP-1", "MyServer", "Notion"] { let config = McpServerConfig::new(name, "https://mcp.example.com"); @@ -1209,6 +1194,63 @@ mod tests { } } + #[test] + fn test_server_name_leading_trailing_separator_rejected() { + // Catches edge cases the old manual allowlist missed + for name in ["-server", "server-", "_server", "server_"] { + let config = McpServerConfig::new(name, "https://mcp.example.com"); + assert!( + config.validate().is_err(), + "Name '{}' with leading/trailing separator should be rejected", + name + ); + } + } + + #[test] + fn test_server_name_consecutive_separators_rejected() { + for name in ["my--server", "my__server"] { + let config = McpServerConfig::new(name, "https://mcp.example.com"); + assert!( + config.validate().is_err(), + "Name '{}' with consecutive separators should be rejected", + name + ); + } + } + + #[test] + fn test_server_name_bare_separator_rejected() { + for name in ["-", "_"] { + let config = McpServerConfig::new(name, "https://mcp.example.com"); + assert!( + config.validate().is_err(), + "Bare separator '{}' should be rejected", + name + ); + } + } + + #[test] + fn test_server_name_64_chars_accepted() { + let name = "a".repeat(64); + let config = McpServerConfig::new(&name, "https://mcp.example.com"); + assert!( + config.validate().is_ok(), + "64-char name should be accepted" + ); + } + + #[test] + fn test_server_name_65_chars_rejected() { + let name = "a".repeat(65); + let config = McpServerConfig::new(&name, "https://mcp.example.com"); + assert!( + config.validate().is_err(), + "65-char name should be rejected (max is 64)" + ); + } + #[tokio::test] async fn test_load_skips_invalid_server_name() { let dir = tempdir().unwrap(); From ee4f9952600256a910ca6c78112513940574c27a Mon Sep 17 00:00:00 2001 From: willamhou Date: Sun, 19 Apr 2026 21:16:48 +0800 Subject: [PATCH 5/6] style: cargo fmt Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude Co-Authored-By: Happy --- src/tools/mcp/config.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/tools/mcp/config.rs b/src/tools/mcp/config.rs index fe34184d1aa..efa182c3de4 100644 --- a/src/tools/mcp/config.rs +++ b/src/tools/mcp/config.rs @@ -1235,10 +1235,7 @@ mod tests { fn test_server_name_64_chars_accepted() { let name = "a".repeat(64); let config = McpServerConfig::new(&name, "https://mcp.example.com"); - assert!( - config.validate().is_ok(), - "64-char name should be accepted" - ); + assert!(config.validate().is_ok(), "64-char name should be accepted"); } #[test] From 346b811fe8d8fa3f4456fde541618b54b8e2162c Mon Sep 17 00:00:00 2001 From: Firat Sertgoz Date: Sun, 19 Apr 2026 17:18:46 +0300 Subject: [PATCH 6/6] =?UTF-8?q?test(mcp):=20address=20henrypark133=20revie?= =?UTF-8?q?w=20=E2=80=94=20add=20DB=20load=20regression=20(#1941)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/tools/mcp/config.rs | 43 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/src/tools/mcp/config.rs b/src/tools/mcp/config.rs index efa182c3de4..1f47dfa8f39 100644 --- a/src/tools/mcp/config.rs +++ b/src/tools/mcp/config.rs @@ -1277,6 +1277,49 @@ mod tests { assert_eq!(result.servers[0].name, "good-server"); } + #[cfg(feature = "libsql")] + #[tokio::test] + async fn test_load_from_db_skips_invalid_server_name() { + let (db, _tmp) = crate::testing::test_db().await; + let user_id = "mcp-user"; + + // Mirror the legacy-upgrade case: one invalid persisted entry should + // not prevent the loader from returning the remaining valid servers. + db.set_setting( + user_id, + "mcp_servers", + &serde_json::json!({ + "schema_version": 7, + "servers": [ + { + "name": "bad;rm -rf /", + "url": "https://mcp.example.com", + "enabled": true, + "headers": {} + }, + { + "name": "good-server", + "url": "https://mcp.good.com", + "enabled": true, + "headers": {} + } + ] + }), + ) + .await + .unwrap(); + + let result = load_mcp_servers_from_db(db.as_ref(), user_id) + .await + .unwrap(); + assert_eq!( + result.schema_version, 7, + "DB load should preserve schema version" + ); + assert_eq!(result.servers.len(), 1, "Should skip invalid, keep valid"); + assert_eq!(result.servers[0].name, "good-server"); + } + #[test] fn test_header_crlf_injection_rejected() { let mut headers = HashMap::new();