fix(mcp): header safety validation and Authorization conflict bug from #704

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);
+                        }
                     }
                 }
             }

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<HashMap<String, String>, 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

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<Path>) -> Result<McpServersF
     let content = fs::read_to_string(path).await?;
     let config: McpServersFile = serde_json::from_str(&content)?;
 
+    // Validate every server on load so corrupted configs are caught early
+    for server in &config.servers {
+        server.validate().map_err(|e| ConfigError::InvalidConfig {
+            reason: format!("Server '{}': {}", server.name, e),
+        })?;
+    }
+
     Ok(config)
 }
 
@@ -457,6 +497,12 @@ pub async fn load_mcp_servers_from_db(
     match store.get_setting(user_id, "mcp_servers").await {
         Ok(Some(value)) => {
             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([

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<serde_json::Value> {
+            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");
+    }
 }