fix(security): mask sensitive values in MCP config API responses (#2667)

* fix(security): mask sensitive values in MCP config API responses GET /api/mcp/config previously returned plaintext secrets including env dict values (API keys), headers (auth tokens), and OAuth client_secret/refresh_token. Any authenticated user could read all MCP service credentials. This commit masks sensitive fields in GET/PUT responses while preserving the key structure so the frontend round-trip (GET masked → toggle enabled → PUT) correctly preserves existing secrets. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): address Copilot review on MCP config masking - Load raw JSON (un-resolved $VAR placeholders) as merge source instead of resolved config, preventing plaintext secrets from replacing $VAR placeholders on disk (Comment 2) - Preserve all top-level keys (e.g. mcpInterceptors) in PUT, not just mcpServers/skills (Comment 1) - Reject masked value '***' for new keys that don't exist in existing config, returning 400 with actionable error (Comment 3) - Allow empty string '' to explicitly clear OAuth secrets, while None means 'preserve existing' for safe round-trip (Comment 4) - Add 3 new tests for rejection, clearing, and edge cases (18 total) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
2026-05-21 15:36:48 +00:00 · 2026-05-21 10:28:57 +08:00
parent e19bec1422
commit 7ec8d3a6e7
2 changed files with 434 additions and 9 deletions
@@ -0,0 +1,305 @@
+"""Tests for MCP config secret masking and preservation.
+
+Verifies that GET /api/mcp/config masks sensitive fields (env values,
+header values, OAuth secrets) and that PUT /api/mcp/config correctly
+preserves existing secrets when the frontend round-trips masked values.
+"""
+
+from __future__ import annotations
+
+import pytest
+
+from app.gateway.routers.mcp import (
+    McpOAuthConfigResponse,
+    McpServerConfigResponse,
+    _mask_server_config,
+    _merge_preserving_secrets,
+)
+
+# ---------------------------------------------------------------------------
+# _mask_server_config
+# ---------------------------------------------------------------------------
+
+
+def test_mask_replaces_env_values_with_asterisks():
+    """Env dict values should be replaced with '***'."""
+    server = McpServerConfigResponse(
+        env={"GITHUB_TOKEN": "ghp_real_secret_123", "API_KEY": "sk-abc"},
+    )
+    masked = _mask_server_config(server)
+    assert masked.env == {"GITHUB_TOKEN": "***", "API_KEY": "***"}
+
+
+def test_mask_replaces_header_values_with_asterisks():
+    """Header dict values should be replaced with '***'."""
+    server = McpServerConfigResponse(
+        headers={"Authorization": "Bearer tok_123", "X-API-Key": "key_456"},
+    )
+    masked = _mask_server_config(server)
+    assert masked.headers == {"Authorization": "***", "X-API-Key": "***"}
+
+
+def test_mask_removes_oauth_secrets():
+    """OAuth client_secret and refresh_token should be set to None."""
+    server = McpServerConfigResponse(
+        oauth=McpOAuthConfigResponse(
+            client_id="my-client",
+            client_secret="super-secret",
+            refresh_token="refresh-token-abc",
+            token_url="https://auth.example.com/token",
+        ),
+    )
+    masked = _mask_server_config(server)
+    assert masked.oauth is not None
+    assert masked.oauth.client_secret is None
+    assert masked.oauth.refresh_token is None
+    # Non-secret fields preserved
+    assert masked.oauth.client_id == "my-client"
+    assert masked.oauth.token_url == "https://auth.example.com/token"
+
+
+def test_mask_preserves_non_secret_fields():
+    """Non-sensitive fields should pass through unchanged."""
+    server = McpServerConfigResponse(
+        enabled=True,
+        type="stdio",
+        command="npx",
+        args=["-y", "@modelcontextprotocol/server-github"],
+        env={"KEY": "val"},
+        description="GitHub MCP server",
+    )
+    masked = _mask_server_config(server)
+    assert masked.enabled is True
+    assert masked.type == "stdio"
+    assert masked.command == "npx"
+    assert masked.args == ["-y", "@modelcontextprotocol/server-github"]
+    assert masked.description == "GitHub MCP server"
+
+
+def test_mask_handles_empty_env_and_headers():
+    """Empty env/headers dicts should remain empty."""
+    server = McpServerConfigResponse()
+    masked = _mask_server_config(server)
+    assert masked.env == {}
+    assert masked.headers == {}
+
+
+def test_mask_handles_no_oauth():
+    """Server without OAuth should remain None."""
+    server = McpServerConfigResponse(oauth=None)
+    masked = _mask_server_config(server)
+    assert masked.oauth is None
+
+
+def test_mask_does_not_mutate_original():
+    """Masking should return a new object, not modify the original."""
+    server = McpServerConfigResponse(env={"KEY": "secret"})
+    masked = _mask_server_config(server)
+    assert server.env["KEY"] == "secret"
+    assert masked.env["KEY"] == "***"
+
+
+# ---------------------------------------------------------------------------
+# _merge_preserving_secrets
+# ---------------------------------------------------------------------------
+
+
+def test_merge_preserves_masked_env_values():
+    """Incoming '***' env values should be replaced with existing secrets."""
+    incoming = McpServerConfigResponse(env={"KEY": "***"})
+    existing = McpServerConfigResponse(env={"KEY": "real_secret"})
+    merged = _merge_preserving_secrets(incoming, existing)
+    assert merged.env["KEY"] == "real_secret"
+
+
+def test_merge_preserves_masked_header_values():
+    """Incoming '***' header values should be replaced with existing secrets."""
+    incoming = McpServerConfigResponse(headers={"Authorization": "***"})
+    existing = McpServerConfigResponse(headers={"Authorization": "Bearer real"})
+    merged = _merge_preserving_secrets(incoming, existing)
+    assert merged.headers["Authorization"] == "Bearer real"
+
+
+def test_merge_preserves_oauth_secrets_when_none():
+    """Incoming None oauth secrets should preserve existing values."""
+    incoming = McpServerConfigResponse(
+        oauth=McpOAuthConfigResponse(
+            client_secret=None,
+            refresh_token=None,
+            token_url="https://auth.example.com/token",
+        ),
+    )
+    existing = McpServerConfigResponse(
+        oauth=McpOAuthConfigResponse(
+            client_secret="existing-secret",
+            refresh_token="existing-refresh",
+            token_url="https://auth.example.com/token",
+        ),
+    )
+    merged = _merge_preserving_secrets(incoming, existing)
+    assert merged.oauth is not None
+    assert merged.oauth.client_secret == "existing-secret"
+    assert merged.oauth.refresh_token == "existing-refresh"
+
+
+def test_merge_accepts_new_secret_values():
+    """Incoming real secret values should replace existing ones."""
+    incoming = McpServerConfigResponse(
+        env={"KEY": "new_secret"},
+        oauth=McpOAuthConfigResponse(
+            client_secret="new-client-secret",
+            refresh_token="new-refresh-token",
+            token_url="https://auth.example.com/token",
+        ),
+    )
+    existing = McpServerConfigResponse(
+        env={"KEY": "old_secret"},
+        oauth=McpOAuthConfigResponse(
+            client_secret="old-secret",
+            refresh_token="old-refresh",
+            token_url="https://auth.example.com/token",
+        ),
+    )
+    merged = _merge_preserving_secrets(incoming, existing)
+    assert merged.env["KEY"] == "new_secret"
+    assert merged.oauth.client_secret == "new-client-secret"
+    assert merged.oauth.refresh_token == "new-refresh-token"
+
+
+def test_merge_handles_no_existing_oauth():
+    """When existing has no oauth but incoming does, keep incoming."""
+    incoming = McpServerConfigResponse(
+        oauth=McpOAuthConfigResponse(
+            client_secret="new-secret",
+            token_url="https://auth.example.com/token",
+        ),
+    )
+    existing = McpServerConfigResponse(oauth=None)
+    merged = _merge_preserving_secrets(incoming, existing)
+    assert merged.oauth is not None
+    assert merged.oauth.client_secret == "new-secret"
+
+
+def test_merge_does_not_mutate_original():
+    """Merge should return a new object, not modify the original."""
+    incoming = McpServerConfigResponse(env={"KEY": "***"})
+    existing = McpServerConfigResponse(env={"KEY": "secret"})
+    merged = _merge_preserving_secrets(incoming, existing)
+    assert incoming.env["KEY"] == "***"
+    assert existing.env["KEY"] == "secret"
+    assert merged.env["KEY"] == "secret"
+
+
+# ---------------------------------------------------------------------------
+# Comment 2 fix: masked value for new key is rejected
+# ---------------------------------------------------------------------------
+
+
+def test_merge_rejects_masked_value_for_new_env_key():
+    """Sending '***' for a key that doesn't exist in existing should raise 400."""
+    from fastapi import HTTPException
+
+    incoming = McpServerConfigResponse(env={"NEW_KEY": "***"})
+    existing = McpServerConfigResponse(env={})
+    with pytest.raises(HTTPException) as exc_info:
+        _merge_preserving_secrets(incoming, existing)
+    assert exc_info.value.status_code == 400
+    assert "NEW_KEY" in exc_info.value.detail
+
+
+def test_merge_rejects_masked_value_for_new_header_key():
+    """Sending '***' for a header key that doesn't exist should raise 400."""
+    from fastapi import HTTPException
+
+    incoming = McpServerConfigResponse(headers={"X-New-Auth": "***"})
+    existing = McpServerConfigResponse(headers={})
+    with pytest.raises(HTTPException) as exc_info:
+        _merge_preserving_secrets(incoming, existing)
+    assert exc_info.value.status_code == 400
+    assert "X-New-Auth" in exc_info.value.detail
+
+
+# ---------------------------------------------------------------------------
+# Comment 4 fix: empty string clears OAuth secrets
+# ---------------------------------------------------------------------------
+
+
+def test_merge_empty_string_clears_oauth_client_secret():
+    """Sending '' for client_secret should clear the stored value."""
+    incoming = McpServerConfigResponse(
+        oauth=McpOAuthConfigResponse(
+            client_secret="",
+            refresh_token=None,
+            token_url="https://auth.example.com/token",
+        ),
+    )
+    existing = McpServerConfigResponse(
+        oauth=McpOAuthConfigResponse(
+            client_secret="existing-secret",
+            refresh_token="existing-refresh",
+            token_url="https://auth.example.com/token",
+        ),
+    )
+    merged = _merge_preserving_secrets(incoming, existing)
+    assert merged.oauth.client_secret is None
+    assert merged.oauth.refresh_token == "existing-refresh"
+
+
+def test_merge_empty_string_clears_oauth_refresh_token():
+    """Sending '' for refresh_token should clear the stored value."""
+    incoming = McpServerConfigResponse(
+        oauth=McpOAuthConfigResponse(
+            client_secret=None,
+            refresh_token="",
+            token_url="https://auth.example.com/token",
+        ),
+    )
+    existing = McpServerConfigResponse(
+        oauth=McpOAuthConfigResponse(
+            client_secret="existing-secret",
+            refresh_token="existing-refresh",
+            token_url="https://auth.example.com/token",
+        ),
+    )
+    merged = _merge_preserving_secrets(incoming, existing)
+    assert merged.oauth.client_secret == "existing-secret"
+    assert merged.oauth.refresh_token is None
+
+
+# ---------------------------------------------------------------------------
+# Round-trip integration: mask → merge should preserve original secrets
+# ---------------------------------------------------------------------------
+
+
+def test_roundtrip_mask_then_merge_preserves_original_secrets():
+    """Simulates the full frontend round-trip: GET (masked) → toggle → PUT."""
+    original = McpServerConfigResponse(
+        enabled=True,
+        env={"GITHUB_TOKEN": "ghp_real_secret"},
+        headers={"Authorization": "Bearer real_token"},
+        oauth=McpOAuthConfigResponse(
+            client_id="client-123",
+            client_secret="oauth-secret",
+            refresh_token="refresh-abc",
+            token_url="https://auth.example.com/token",
+        ),
+        description="GitHub MCP server",
+    )
+
+    # Step 1: Server returns masked config (simulates GET response)
+    masked = _mask_server_config(original)
+    assert masked.env["GITHUB_TOKEN"] == "***"
+    assert masked.oauth.client_secret is None
+
+    # Step 2: Frontend toggles enabled and sends back (simulates PUT request)
+    from_frontend = masked.model_copy(update={"enabled": False})
+
+    # Step 3: Server merges with existing secrets (simulates PUT handler)
+    restored = _merge_preserving_secrets(from_frontend, original)
+    assert restored.enabled is False
+    assert restored.env["GITHUB_TOKEN"] == "ghp_real_secret"
+    assert restored.headers["Authorization"] == "Bearer real_token"
+    assert restored.oauth.client_secret == "oauth-secret"
+    assert restored.oauth.refresh_token == "refresh-abc"
+    # Non-secret fields from the update are preserved
+    assert restored.description == "GitHub MCP server"