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

* fix(mcp): header safety validation and Authorization conflict bug from #704 * 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. * fix ci issue
2026-08-25 14:53:34 +00:00 · 2026-03-11 16:53:02 -07:00
parent 2094d6e30d
commit a1b3911b27
4 changed files with 303 additions and 2 deletions
@@ -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);
+                        }
                    }
                }
            }
@@ -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<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
@@ -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([
@@ -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");
+    }
 }