diff --git a/src/app.rs b/src/app.rs index 6394625b..c53bebab 100644 --- a/src/app.rs +++ b/src/app.rs @@ -563,7 +563,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 377d0488..d3bf17b8 100644 --- a/src/tools/mcp/client.rs +++ b/src/tools/mcp/client.rs @@ -246,9 +246,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 + .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)); } if let Some(ref session_manager) = self.session_manager diff --git a/src/tools/mcp/config.rs b/src/tools/mcp/config.rs index 7dd4be57..6a1ce8b3 100644 --- a/src/tools/mcp/config.rs +++ b/src/tools/mcp/config.rs @@ -188,9 +188,42 @@ impl McpServerConfig { } } + // 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 reqwest::header::HeaderName::from_bytes(name.as_bytes()).is_err() { + return Err(ConfigError::InvalidConfig { + reason: format!( + "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(()) } + /// 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 @@ -381,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) => { @@ -669,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"); @@ -830,6 +904,94 @@ 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("not a valid HTTP header name"), + "Expected RFC 9110 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("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] + 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 2a51ae63..d50d54d6 100644 --- a/src/tools/mcp/http_transport.rs +++ b/src/tools/mcp/http_transport.rs @@ -383,4 +383,121 @@ 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: Some(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: Some(1), + method: "initialize".to_string(), + params: Some(serde_json::json!({})), + }; + let response = transport.send(&request, &per_request).await.unwrap(); + + let echoed = response.result.unwrap(); + // 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"); + } + + #[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: Some(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"); + } }