diff --git a/crates/ironclaw_gateway/static/app.js b/crates/ironclaw_gateway/static/app.js index bbaf6ff57c..6c0374201e 100644 --- a/crates/ironclaw_gateway/static/app.js +++ b/crates/ironclaw_gateway/static/app.js @@ -6520,12 +6520,24 @@ function installWasmExtension() { }); } +function normalizeMcpServerName(raw) { + return raw.toLowerCase() + .replace(/[^a-z0-9_-]/g, '_') + .replace(/_+/g, '_') + .replace(/^_|_$/g, ''); +} + function addMcpServer() { - var name = document.getElementById('mcp-install-name').value.trim(); - if (!name) { + var rawName = document.getElementById('mcp-install-name').value.trim(); + if (!rawName) { showToast(I18n.t('mcp.serverNameRequired'), 'error'); return; } + var name = normalizeMcpServerName(rawName); + if (!name) { + showToast('Server name contains no valid characters', 'error'); + return; + } var url = document.getElementById('mcp-install-url').value.trim(); if (!url) { showToast(I18n.t('mcp.urlRequired'), 'error'); diff --git a/crates/ironclaw_tui/src/widgets/conversation.rs b/crates/ironclaw_tui/src/widgets/conversation.rs index d2715a99f6..19bda4433b 100644 --- a/crates/ironclaw_tui/src/widgets/conversation.rs +++ b/crates/ironclaw_tui/src/widgets/conversation.rs @@ -859,7 +859,14 @@ impl ConversationWidget { let mut items_str = items.join(", "); // Truncate if too long if items_str.len() > 60 { - items_str.truncate(57); + // Use char_indices to find a safe truncation point (UTF-8 safe) + let truncate_at = items_str + .char_indices() + .map(|(i, _)| i) + .take_while(|&i| i <= 57) + .last() + .unwrap_or(0); + items_str.truncate(truncate_at); items_str.push_str("..."); } diff --git a/src/agent/thread_ops.rs b/src/agent/thread_ops.rs index 721f4dd268..75d0529028 100644 --- a/src/agent/thread_ops.rs +++ b/src/agent/thread_ops.rs @@ -405,7 +405,9 @@ impl Agent { .iter() .any(|rule| rule.action == ironclaw_safety::PolicyAction::Block) { - return Ok(SubmissionResult::error("Input rejected by safety policy.")); + return Ok(SubmissionResult::error( + "Input rejected by safety policy.", + )); } if let Some(warning) = self.safety().scan_inbound_for_secrets(content) { tracing::warn!( @@ -426,7 +428,8 @@ impl Agent { // acknowledgment, not a completed LLM turn. return Ok(SubmissionResult::Ok { message: Some( - "Message queued — will be processed after the current turn.".into(), + "Message queued — will be processed after the current turn." + .into(), ), }); } diff --git a/src/channels/web/handlers/extensions.rs b/src/channels/web/handlers/extensions.rs index a73822e5bf..e7f43998fe 100644 --- a/src/channels/web/handlers/extensions.rs +++ b/src/channels/web/handlers/extensions.rs @@ -139,8 +139,19 @@ pub async fn extensions_install_handler( _ => None, }); + let normalized_name = crate::tools::mcp::config::normalize_server_name(&req.name); + if normalized_name.is_empty() { + return Ok(Json(ActionResponse::fail( + "Server name contains no valid characters".to_string(), + ))); + } match ext_mgr - .install(&req.name, req.url.as_deref(), kind_hint, &user.user_id) + .install( + &normalized_name, + req.url.as_deref(), + kind_hint, + &user.user_id, + ) .await { Ok(result) => Ok(Json(ActionResponse::ok(result.message))), diff --git a/src/cli/mcp.rs b/src/cli/mcp.rs index 9139d86d6a..7ac8640346 100644 --- a/src/cli/mcp.rs +++ b/src/cli/mcp.rs @@ -14,7 +14,7 @@ use crate::secrets::SecretsStore; use crate::tools::mcp::{ McpClient, McpProcessManager, McpServerConfig, McpSessionManager, OAuthConfig, auth::{authorize_mcp_server, is_authenticated}, - config::{self, EffectiveTransport, McpServersFile}, + config::{self, EffectiveTransport, McpServersFile, normalize_server_name}, factory::create_client_from_config, }; @@ -166,7 +166,7 @@ pub async fn run_mcp_command(cmd: McpCommand) -> anyhow::Result<()> { /// Add a new MCP server. async fn add_server(args: McpAddArgs) -> anyhow::Result<()> { let McpAddArgs { - name, + name: raw_name, url, transport, command, @@ -181,6 +181,16 @@ async fn add_server(args: McpAddArgs) -> anyhow::Result<()> { description, } = args; + // Normalize the server name: lowercase, replace special chars with + // underscores so descriptive names like "My Twitter Server" work (#2236). + let name = normalize_server_name(&raw_name); + if name.is_empty() { + anyhow::bail!("Server name '{}' contains no valid characters", raw_name); + } + if name != raw_name { + println!(" (normalized name: '{}' -> '{}')", raw_name, name); + } + let transport_lower = transport.to_lowercase(); let mut config = match transport_lower.as_str() { @@ -290,6 +300,7 @@ async fn add_server(args: McpAddArgs) -> anyhow::Result<()> { /// Remove an MCP server. async fn remove_server(name: String) -> anyhow::Result<()> { + let name = normalize_server_name(&name); let (db, owner_id) = connect_db().await; let mut servers = load_servers(db.as_deref(), &owner_id).await?; if !servers.remove(&name) { @@ -411,6 +422,7 @@ async fn list_servers(verbose: bool) -> anyhow::Result<()> { /// Authenticate with an MCP server. async fn auth_server(name: String, user_id: String) -> anyhow::Result<()> { + let name = normalize_server_name(&name); // Get server config let (db, owner_id) = connect_db().await; let servers = load_servers(db.as_deref(), &owner_id).await?; @@ -495,6 +507,7 @@ async fn auth_server(name: String, user_id: String) -> anyhow::Result<()> { /// Test connection to an MCP server. async fn test_server(name: String, user_id: String) -> anyhow::Result<()> { + let name = normalize_server_name(&name); // Get server config let (db, owner_id) = connect_db().await; let servers = load_servers(db.as_deref(), &owner_id).await?; @@ -568,9 +581,11 @@ async fn test_server(name: String, user_id: String) -> anyhow::Result<()> { }; println!(" • {}{}", tool.name, approval); if !tool.description.is_empty() { - // Truncate long descriptions - let desc = if tool.description.len() > 60 { - format!("{}...", &tool.description[..57]) + // Truncate long descriptions (char-safe to avoid + // panicking on multi-byte UTF-8 boundaries, #1947) + let desc = if tool.description.chars().count() > 60 { + let truncated: String = tool.description.chars().take(57).collect(); + format!("{truncated}...") } else { tool.description.clone() }; @@ -618,6 +633,7 @@ async fn test_server(name: String, user_id: String) -> anyhow::Result<()> { /// Toggle server enabled/disabled state. async fn toggle_server(name: String, enable: bool, disable: bool) -> anyhow::Result<()> { + let name = normalize_server_name(&name); let (db, owner_id) = connect_db().await; let mut servers = load_servers(db.as_deref(), &owner_id).await?; @@ -741,4 +757,54 @@ mod tests { assert!(result.is_err()); assert!(result.unwrap_err().contains("invalid env var format")); } + + /// Regression test for #1947: byte-index slicing panics on multi-byte UTF-8. + /// + /// Reproduces the original bug: a description with multi-byte chars where + /// byte index 57 falls inside a character boundary would panic with + /// `&tool.description[..57]`. + #[test] + fn test_tool_description_truncation_multibyte_utf8() { + // Each CJK character is 3 bytes. 20 chars = 60 bytes. + // Byte index 57 falls inside the 20th character (bytes 57..59). + let description = "\u{4e00}".repeat(20); // 20 CJK chars, 60 bytes + assert_eq!(description.len(), 60); + assert_eq!(description.chars().count(), 20); + + // Simulate the fixed truncation logic + let desc = if description.chars().count() > 60 { + let truncated: String = description.chars().take(57).collect(); + format!("{truncated}...") + } else { + description.clone() + }; + // 20 chars <= 60, so no truncation needed + assert_eq!(desc, description); + + // Now test with a string that actually exceeds 60 chars + let long_desc = "\u{4e00}".repeat(61); // 61 CJK chars + assert_eq!(long_desc.chars().count(), 61); + let desc = if long_desc.chars().count() > 60 { + let truncated: String = long_desc.chars().take(57).collect(); + format!("{truncated}...") + } else { + long_desc.clone() + }; + assert_eq!(desc.chars().count(), 60); // 57 chars + "..." + assert!(desc.ends_with("...")); + } + + /// Verify ASCII descriptions still truncate correctly. + #[test] + fn test_tool_description_truncation_ascii() { + let description = "a".repeat(80); + let desc = if description.chars().count() > 60 { + let truncated: String = description.chars().take(57).collect(); + format!("{truncated}...") + } else { + description.clone() + }; + assert_eq!(desc.len(), 60); + assert!(desc.ends_with("...")); + } } diff --git a/src/tools/mcp/config.rs b/src/tools/mcp/config.rs index 011a12842b..ae95b7c047 100644 --- a/src/tools/mcp/config.rs +++ b/src/tools/mcp/config.rs @@ -76,7 +76,7 @@ impl McpServerConfig { /// Create a new MCP server configuration. pub fn new(name: impl Into, url: impl Into) -> Self { Self { - name: name.into(), + name: normalize_server_name(&name.into()), url: url.into(), transport: None, headers: HashMap::new(), @@ -95,7 +95,7 @@ impl McpServerConfig { env: HashMap, ) -> Self { Self { - name: name.into(), + name: normalize_server_name(&name.into()), url: String::new(), transport: Some(McpTransportConfig::Stdio { command: command.into(), @@ -113,7 +113,7 @@ impl McpServerConfig { /// Create a new Unix socket transport MCP server configuration. pub fn new_unix(name: impl Into, socket_path: impl Into) -> Self { Self { - name: name.into(), + name: normalize_server_name(&name.into()), url: String::new(), transport: Some(McpTransportConfig::Unix { socket_path: socket_path.into(), @@ -239,6 +239,7 @@ impl McpServerConfig { /// (which likely supports Dynamic Client Registration even without pre-configured OAuth). /// /// Non-HTTP transports (stdio, unix) never require auth. + /// Returns false if a custom Authorization header is already set (#1948). pub fn requires_auth(&self) -> bool { // Non-HTTP transports don't use HTTP auth if !matches!(self.effective_transport(), EffectiveTransport::Http) { @@ -398,7 +399,8 @@ impl McpServersFile { } /// Add or update a server configuration. - pub fn upsert(&mut self, config: McpServerConfig) { + pub fn upsert(&mut self, mut config: McpServerConfig) { + config.name = normalize_server_name(&config.name); if let Some(existing) = self.get_mut(&config.name) { *existing = config; } else { @@ -741,6 +743,48 @@ pub async fn remove_mcp_server_db( Ok(()) } +/// Normalize a server name for internal use. +/// +/// Converts to lowercase, replaces spaces and non-alphanumeric characters +/// (except hyphens and underscores) with underscores, collapses consecutive +/// underscores, and trims leading/trailing underscores. +/// +/// This allows users to supply descriptive names like "My Twitter Server" +/// which are stored as `my_twitter_server` (#2236). +pub fn normalize_server_name(name: &str) -> String { + let lowered = name.to_lowercase(); + let normalized: String = lowered + .chars() + .map(|c| { + if c.is_ascii_alphanumeric() || c == '-' || c == '_' { + c + } else { + '_' + } + }) + .collect(); + + // Collapse consecutive underscores and trim leading/trailing underscores + let mut result = String::with_capacity(normalized.len()); + let mut prev_underscore = true; // treat start as underscore to trim leading + for c in normalized.chars() { + if c == '_' { + if !prev_underscore { + result.push('_'); + } + prev_underscore = true; + } else { + result.push(c); + prev_underscore = false; + } + } + // Trim trailing underscore + if result.ends_with('_') { + result.pop(); + } + result +} + /// Check if a URL points to a loopback address (localhost, 127.0.0.1, [::1]). /// /// Uses `url::Url` for proper parsing so edge cases (IPv6, userinfo, ports) @@ -1372,6 +1416,106 @@ mod tests { ); } + // --- Issue #2236 regression: normalize_server_name --- + + #[test] + fn test_normalize_server_name_basic() { + assert_eq!(normalize_server_name("notion"), "notion"); + assert_eq!(normalize_server_name("my-server"), "my-server"); + assert_eq!(normalize_server_name("my_server"), "my_server"); + } + + #[test] + fn test_normalize_server_name_uppercase() { + assert_eq!(normalize_server_name("MyServer"), "myserver"); + assert_eq!(normalize_server_name("NOTION"), "notion"); + } + + #[test] + fn test_normalize_server_name_spaces_and_special_chars() { + assert_eq!( + normalize_server_name("My Twitter Server"), + "my_twitter_server" + ); + assert_eq!(normalize_server_name("server@v2!"), "server_v2"); + assert_eq!(normalize_server_name("a b"), "a_b"); + } + + #[test] + fn test_normalize_server_name_empty_and_all_special() { + assert_eq!(normalize_server_name(""), ""); + assert_eq!(normalize_server_name("@#$"), ""); + assert_eq!(normalize_server_name("___"), ""); + } + + #[test] + fn test_normalize_server_name_leading_trailing() { + assert_eq!(normalize_server_name(" hello "), "hello"); + assert_eq!(normalize_server_name("__hello__"), "hello"); + } + + #[test] + fn test_constructor_normalizes_name() { + let config = McpServerConfig::new("My-Server", "https://example.com"); + assert_eq!(config.name, "my-server"); + + let config = McpServerConfig::new("UPPER CASE", "https://example.com"); + assert_eq!(config.name, "upper_case"); + } + + #[test] + fn test_upsert_normalizes_name() { + let mut file = McpServersFile::default(); + + let raw_config = McpServerConfig { + name: "My-Server".to_string(), + url: "https://example.com".to_string(), + transport: None, + headers: HashMap::new(), + oauth: None, + enabled: true, + description: None, + cached_tools: Vec::new(), + }; + file.upsert(raw_config); + + assert!(file.get("my-server").is_some()); + assert!(file.get("My-Server").is_none()); + } + + // --- Issue #1948 regression: requires_auth with custom Authorization header --- + + #[test] + fn test_requires_auth_skipped_with_custom_auth_header() { + let headers = + HashMap::from([("Authorization".to_string(), "Bearer my-api-key".to_string())]); + let config = + McpServerConfig::new("api-server", "https://mcp.example.com").with_headers(headers); + assert!( + !config.requires_auth(), + "requires_auth() should return false when Authorization header is set" + ); + } + + #[test] + fn test_requires_auth_skipped_with_lowercase_auth_header() { + let headers = HashMap::from([("authorization".to_string(), "Bearer token".to_string())]); + let config = + McpServerConfig::new("api-server", "https://mcp.example.com").with_headers(headers); + assert!(!config.requires_auth()); + } + + #[test] + fn test_requires_auth_still_true_with_non_auth_headers() { + let headers = HashMap::from([("X-Api-Key".to_string(), "key123".to_string())]); + let config = + McpServerConfig::new("api-server", "https://mcp.example.com").with_headers(headers); + assert!( + config.requires_auth(), + "requires_auth() should still return true with non-Authorization headers" + ); + } + // --- Issue 3 regression: is_localhost_url rejects attacker subdomains --- #[test]