From cb544cfc9448f0fad150363c2f840f556561a563 Mon Sep 17 00:00:00 2001 From: reidliu41 Date: Mon, 9 Mar 2026 15:59:21 +0800 Subject: [PATCH 1/3] fix(mcp): header safety validation and Authorization conflict bug from #704 --- src/tools/mcp/client.rs | 12 +++- src/tools/mcp/config.rs | 90 +++++++++++++++++++++++++ src/tools/mcp/http_transport.rs | 116 ++++++++++++++++++++++++++++++++ 3 files changed, 217 insertions(+), 1 deletion(-) diff --git a/src/tools/mcp/client.rs b/src/tools/mcp/client.rs index cd74d572bb8..f4204d684f0 100644 --- a/src/tools/mcp/client.rs +++ b/src/tools/mcp/client.rs @@ -237,9 +237,19 @@ impl McpClient { } /// Build the headers map for a request (auth, session-id, custom headers). + /// + /// Custom headers are applied first. OAuth token injection is skipped if the + /// user has explicitly configured an Authorization header, so user-provided + /// credentials are never silently overwritten. async fn build_request_headers(&self) -> Result, ToolError> { let mut headers = self.custom_headers.clone(); - if let Some(token) = self.get_access_token().await? { + + // Only inject OAuth token if the user hasn't set a custom Authorization header. + let has_custom_auth = self + .server_config + .as_ref() + .is_some_and(|c| c.has_custom_auth_header()); + if !has_custom_auth && let Some(token) = self.get_access_token().await? { headers.insert("Authorization".to_string(), format!("Bearer {}", token)); } if let Some(ref session_manager) = self.session_manager diff --git a/src/tools/mcp/config.rs b/src/tools/mcp/config.rs index 7dd4be57dab..cbb292a464b 100644 --- a/src/tools/mcp/config.rs +++ b/src/tools/mcp/config.rs @@ -188,9 +188,41 @@ impl McpServerConfig { } } + // Validate custom header names and values (prevent CRLF injection) + for (name, value) in &self.headers { + if name.is_empty() { + return Err(ConfigError::InvalidConfig { + reason: "Header name cannot be empty".to_string(), + }); + } + if name.bytes().any(|b| b == b'\r' || b == b'\n') { + return Err(ConfigError::InvalidConfig { + reason: format!("Header name '{}' contains invalid CRLF characters", name), + }); + } + if value.bytes().any(|b| b == b'\r' || b == b'\n') { + return Err(ConfigError::InvalidConfig { + reason: format!( + "Header value for '{}' contains invalid CRLF characters", + name + ), + }); + } + } + Ok(()) } + /// Check if any custom header sets an Authorization value. + /// + /// Used to skip OAuth token injection when the user has explicitly + /// configured an Authorization header (e.g. for API-key-based servers). + pub fn has_custom_auth_header(&self) -> bool { + self.headers + .keys() + .any(|k| k.eq_ignore_ascii_case("authorization")) + } + /// Check if this server requires authentication. /// /// Returns true if OAuth is pre-configured OR if this is a remote HTTPS server @@ -830,6 +862,64 @@ mod tests { assert!(!config.requires_auth()); } + #[test] + fn test_header_crlf_injection_rejected() { + let mut headers = HashMap::new(); + headers.insert("X-Good".to_string(), "safe".to_string()); + headers.insert("X-Bad\r\nInjected: true".to_string(), "value".to_string()); + + let config = + McpServerConfig::new("server", "https://mcp.example.com").with_headers(headers); + let err = config.validate().unwrap_err().to_string(); + assert!(err.contains("CRLF"), "Expected CRLF error, got: {err}"); + } + + #[test] + fn test_header_value_crlf_injection_rejected() { + let mut headers = HashMap::new(); + headers.insert( + "X-Header".to_string(), + "value\r\nInjected: true".to_string(), + ); + + let config = + McpServerConfig::new("server", "https://mcp.example.com").with_headers(headers); + let err = config.validate().unwrap_err().to_string(); + assert!(err.contains("CRLF"), "Expected CRLF error, got: {err}"); + } + + #[test] + fn test_header_empty_name_rejected() { + let mut headers = HashMap::new(); + headers.insert(String::new(), "value".to_string()); + + let config = + McpServerConfig::new("server", "https://mcp.example.com").with_headers(headers); + let err = config.validate().unwrap_err().to_string(); + assert!( + err.contains("empty"), + "Expected empty name error, got: {err}" + ); + } + + #[test] + fn test_has_custom_auth_header_case_insensitive() { + let headers = HashMap::from([("authorization".to_string(), "Bearer token".to_string())]); + let config = + McpServerConfig::new("server", "https://mcp.example.com").with_headers(headers); + assert!(config.has_custom_auth_header()); + + let headers = HashMap::from([("AUTHORIZATION".to_string(), "Bearer token".to_string())]); + let config = + McpServerConfig::new("server", "https://mcp.example.com").with_headers(headers); + assert!(config.has_custom_auth_header()); + + let headers = HashMap::from([("X-Api-Key".to_string(), "key".to_string())]); + let config = + McpServerConfig::new("server", "https://mcp.example.com").with_headers(headers); + assert!(!config.has_custom_auth_header()); + } + #[test] fn test_custom_headers() { let headers = HashMap::from([ diff --git a/src/tools/mcp/http_transport.rs b/src/tools/mcp/http_transport.rs index 2a51ae635d1..82d119348fd 100644 --- a/src/tools/mcp/http_transport.rs +++ b/src/tools/mcp/http_transport.rs @@ -383,4 +383,120 @@ mod tests { HttpMcpTransport::new("http://localhost:8080", "test").with_custom_headers(headers); assert_eq!(transport.custom_headers.get("X-Custom").unwrap(), "value"); } + + // -- Wire-level echo server tests ----------------------------------------- + // + // These tests spin up a real HTTP server that echoes received headers back + // as a JSON-RPC result, verifying that custom headers and Authorization + // handling work end-to-end through the actual HTTP transport. + + /// Spawn a lightweight echo server that returns received headers as a + /// JSON-RPC response. Returns `(url, join_handle)`. + async fn spawn_echo_server() -> (String, tokio::task::JoinHandle<()>) { + use axum::{Router, extract::Request, routing::post}; + use tokio::net::TcpListener; + + async fn echo_headers(req: Request) -> axum::response::Json { + let mut map = serde_json::Map::new(); + for (name, value) in req.headers() { + if let Ok(v) = value.to_str() { + map.insert(name.to_string(), serde_json::Value::String(v.to_string())); + } + } + axum::response::Json(serde_json::json!({ + "jsonrpc": "2.0", + "id": 1, + "result": map, + })) + } + + let app = Router::new().route("/", post(echo_headers)); + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let url = format!("http://127.0.0.1:{}", addr.port()); + + let handle = tokio::spawn(async move { + axum::serve(listener, app).await.unwrap(); + }); + + (url, handle) + } + + #[tokio::test] + async fn test_wire_custom_headers_sent() { + let (url, _handle) = spawn_echo_server().await; + + let custom = HashMap::from([ + ("X-Api-Key".to_string(), "secret-key".to_string()), + ("X-Org-Id".to_string(), "org-123".to_string()), + ]); + let transport = HttpMcpTransport::new(&url, "echo-test").with_custom_headers(custom); + + let request = McpRequest { + jsonrpc: "2.0".to_string(), + id: 1, + method: "initialize".to_string(), + params: Some(serde_json::json!({})), + }; + let per_request_headers = HashMap::new(); + let response = transport + .send(&request, &per_request_headers) + .await + .unwrap(); + + let echoed = response.result.unwrap(); + assert_eq!(echoed["x-api-key"], "secret-key"); + assert_eq!(echoed["x-org-id"], "org-123"); + } + + #[tokio::test] + async fn test_wire_per_request_headers_override_custom() { + let (url, _handle) = spawn_echo_server().await; + + let custom = HashMap::from([( + "authorization".to_string(), + "Bearer custom-token".to_string(), + )]); + let transport = HttpMcpTransport::new(&url, "echo-test").with_custom_headers(custom); + + // Per-request header should override the custom header + let per_request = HashMap::from([( + "authorization".to_string(), + "Bearer oauth-token".to_string(), + )]); + let request = McpRequest { + jsonrpc: "2.0".to_string(), + id: 1, + method: "initialize".to_string(), + params: Some(serde_json::json!({})), + }; + let response = transport.send(&request, &per_request).await.unwrap(); + + let echoed = response.result.unwrap(); + // reqwest merges headers; later one wins + assert_eq!(echoed["authorization"], "Bearer oauth-token"); + } + + #[tokio::test] + async fn test_wire_custom_auth_preserved_when_no_per_request_auth() { + let (url, _handle) = spawn_echo_server().await; + + let custom = HashMap::from([( + "authorization".to_string(), + "Bearer custom-token".to_string(), + )]); + let transport = HttpMcpTransport::new(&url, "echo-test").with_custom_headers(custom); + + let per_request = HashMap::new(); // no per-request auth + let request = McpRequest { + jsonrpc: "2.0".to_string(), + id: 1, + method: "initialize".to_string(), + params: Some(serde_json::json!({})), + }; + let response = transport.send(&request, &per_request).await.unwrap(); + + let echoed = response.result.unwrap(); + assert_eq!(echoed["authorization"], "Bearer custom-token"); + } } From 3cdaa9a605a3225651f99cdb5fba87858e6d6453 Mon Sep 17 00:00:00 2001 From: reidliu41 Date: Tue, 10 Mar 2026 07:35:40 +0800 Subject: [PATCH 2/3] fix(mcp): enforce RFC 9110 header validation on all config load paths Replace hand-written CRLF checks with reqwest::header::HeaderName::from_bytes() and HeaderValue::from_str(), catching spaces, colons, null bytes, and all non-token characters that the previous validation missed. Add validation to load_mcp_servers_from() and load_mcp_servers_from_db() so corrupted configs from disk or DB are rejected at load time instead of silently flowing through to McpClient. Improve app.rs error handling to distinguish "no config" from "corrupted config" (including malformed JSON). Also fix build_request_headers() to check self.custom_headers directly instead of indirectly via server_config, and clarify the wire test comment about HeaderMap::insert replacement semantics. --- src/app.rs | 14 ++++- src/tools/mcp/client.rs | 6 +-- src/tools/mcp/config.rs | 92 +++++++++++++++++++++++++++++---- src/tools/mcp/http_transport.rs | 3 +- 4 files changed, 100 insertions(+), 15 deletions(-) diff --git a/src/app.rs b/src/app.rs index 9fcb19f3f1e..b898a23e8fe 100644 --- a/src/app.rs +++ b/src/app.rs @@ -722,7 +722,19 @@ impl AppBuilder { } } Err(e) => { - tracing::debug!("No MCP servers configured ({})", e); + if matches!( + e, + crate::tools::mcp::config::ConfigError::InvalidConfig { .. } + | crate::tools::mcp::config::ConfigError::Json(_) + ) { + tracing::warn!( + "MCP server configuration is invalid: {}. \ + Fix or remove the corrupted config.", + e + ); + } else { + tracing::debug!("No MCP servers configured ({})", e); + } } } } diff --git a/src/tools/mcp/client.rs b/src/tools/mcp/client.rs index f4204d684f0..c0858c89feb 100644 --- a/src/tools/mcp/client.rs +++ b/src/tools/mcp/client.rs @@ -246,9 +246,9 @@ impl McpClient { // Only inject OAuth token if the user hasn't set a custom Authorization header. let has_custom_auth = self - .server_config - .as_ref() - .is_some_and(|c| c.has_custom_auth_header()); + .custom_headers + .keys() + .any(|k| k.eq_ignore_ascii_case("authorization")); if !has_custom_auth && let Some(token) = self.get_access_token().await? { headers.insert("Authorization".to_string(), format!("Bearer {}", token)); } diff --git a/src/tools/mcp/config.rs b/src/tools/mcp/config.rs index cbb292a464b..6a1ce8b37bf 100644 --- a/src/tools/mcp/config.rs +++ b/src/tools/mcp/config.rs @@ -188,26 +188,27 @@ impl McpServerConfig { } } - // Validate custom header names and values (prevent CRLF injection) + // Validate custom header names and values using the http crate's RFC 9110 + // token validation (catches CRLF, spaces, colons, null bytes, etc.) for (name, value) in &self.headers { if name.is_empty() { return Err(ConfigError::InvalidConfig { reason: "Header name cannot be empty".to_string(), }); } - if name.bytes().any(|b| b == b'\r' || b == b'\n') { - return Err(ConfigError::InvalidConfig { - reason: format!("Header name '{}' contains invalid CRLF characters", name), - }); - } - if value.bytes().any(|b| b == b'\r' || b == b'\n') { + if reqwest::header::HeaderName::from_bytes(name.as_bytes()).is_err() { return Err(ConfigError::InvalidConfig { reason: format!( - "Header value for '{}' contains invalid CRLF characters", + "Header name '{}' is not a valid HTTP header name (RFC 9110)", name ), }); } + if reqwest::header::HeaderValue::from_str(value).is_err() { + return Err(ConfigError::InvalidConfig { + reason: format!("Header value for '{}' contains invalid characters", name), + }); + } } Ok(()) @@ -413,6 +414,13 @@ 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 + for server in &config.servers { + server.validate().map_err(|e| ConfigError::InvalidConfig { + reason: format!("Server '{}': {}", server.name, e), + })?; + } Ok(config) } Ok(None) => { @@ -701,6 +715,34 @@ mod tests { assert!(config.servers.is_empty()); } + #[tokio::test] + async fn test_load_rejects_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" } + }] + }); + 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}" + ); + } + #[test] fn test_token_secret_names() { let config = McpServerConfig::new("notion", "https://mcp.notion.com"); @@ -871,7 +913,10 @@ mod tests { let config = McpServerConfig::new("server", "https://mcp.example.com").with_headers(headers); let err = config.validate().unwrap_err().to_string(); - assert!(err.contains("CRLF"), "Expected CRLF error, got: {err}"); + assert!( + err.contains("not a valid HTTP header name"), + "Expected RFC 9110 error, got: {err}" + ); } #[test] @@ -885,7 +930,34 @@ mod tests { let config = McpServerConfig::new("server", "https://mcp.example.com").with_headers(headers); let err = config.validate().unwrap_err().to_string(); - assert!(err.contains("CRLF"), "Expected CRLF error, got: {err}"); + assert!( + err.contains("invalid characters"), + "Expected invalid characters error, got: {err}" + ); + } + + #[test] + fn test_header_name_with_space_rejected() { + let headers = HashMap::from([("X Bad".to_string(), "value".to_string())]); + let config = + McpServerConfig::new("server", "https://mcp.example.com").with_headers(headers); + assert!(config.validate().is_err()); + } + + #[test] + fn test_header_name_with_colon_rejected() { + let headers = HashMap::from([("X:Bad".to_string(), "value".to_string())]); + let config = + McpServerConfig::new("server", "https://mcp.example.com").with_headers(headers); + assert!(config.validate().is_err()); + } + + #[test] + fn test_header_name_with_null_byte_rejected() { + let headers = HashMap::from([("X-Bad\0".to_string(), "value".to_string())]); + let config = + McpServerConfig::new("server", "https://mcp.example.com").with_headers(headers); + assert!(config.validate().is_err()); } #[test] diff --git a/src/tools/mcp/http_transport.rs b/src/tools/mcp/http_transport.rs index 82d119348fd..63066a7f8c3 100644 --- a/src/tools/mcp/http_transport.rs +++ b/src/tools/mcp/http_transport.rs @@ -473,7 +473,8 @@ mod tests { let response = transport.send(&request, &per_request).await.unwrap(); let echoed = response.result.unwrap(); - // reqwest merges headers; later one wins + // Per-request headers are inserted after custom headers via HeaderMap::insert, + // which replaces any existing entry for the same key. assert_eq!(echoed["authorization"], "Bearer oauth-token"); } From 93ea7f0ce12cfd9f617e98f14740684dc753c505 Mon Sep 17 00:00:00 2001 From: reidliu41 Date: Tue, 10 Mar 2026 07:48:07 +0800 Subject: [PATCH 3/3] fix ci issue --- src/tools/mcp/http_transport.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/tools/mcp/http_transport.rs b/src/tools/mcp/http_transport.rs index 63066a7f8c3..d50d54d604c 100644 --- a/src/tools/mcp/http_transport.rs +++ b/src/tools/mcp/http_transport.rs @@ -434,7 +434,7 @@ mod tests { let request = McpRequest { jsonrpc: "2.0".to_string(), - id: 1, + id: Some(1), method: "initialize".to_string(), params: Some(serde_json::json!({})), }; @@ -466,7 +466,7 @@ mod tests { )]); let request = McpRequest { jsonrpc: "2.0".to_string(), - id: 1, + id: Some(1), method: "initialize".to_string(), params: Some(serde_json::json!({})), }; @@ -491,7 +491,7 @@ mod tests { let per_request = HashMap::new(); // no per-request auth let request = McpRequest { jsonrpc: "2.0".to_string(), - id: 1, + id: Some(1), method: "initialize".to_string(), params: Some(serde_json::json!({})), };