diff --git a/backend/app/gateway/routers/mcp.py b/backend/app/gateway/routers/mcp.py index 386fc13c66..d384062669 100644 --- a/backend/app/gateway/routers/mcp.py +++ b/backend/app/gateway/routers/mcp.py @@ -63,6 +63,99 @@ class McpConfigUpdateRequest(BaseModel): ) +_MASKED_VALUE = "***" + + +def _mask_server_config(server: McpServerConfigResponse) -> McpServerConfigResponse: + """Return a copy of server config with sensitive fields masked. + + Masks env values, header values, and removes OAuth secrets so they + are not exposed through the GET API endpoint. + """ + masked_env = {k: _MASKED_VALUE for k in server.env} + masked_headers = {k: _MASKED_VALUE for k in server.headers} + masked_oauth = None + if server.oauth is not None: + masked_oauth = server.oauth.model_copy( + update={ + "client_secret": None, + "refresh_token": None, + } + ) + return server.model_copy( + update={ + "env": masked_env, + "headers": masked_headers, + "oauth": masked_oauth, + } + ) + + +def _merge_preserving_secrets( + incoming: McpServerConfigResponse, + existing: McpServerConfigResponse, +) -> McpServerConfigResponse: + """Merge incoming config with existing, preserving secrets masked by GET. + + When the frontend toggles ``enabled`` it round-trips the full config: + GET (masked) → modify enabled → PUT (masked values sent back). + This function ensures masked values (``***``) are replaced with the + real secrets from the current on-disk config. + + ``***`` is only accepted for keys that already exist in *existing*. + New keys must provide a real value. + + For OAuth secrets, ``None`` means "preserve the existing stored value" + so masked GET responses can be safely round-tripped. To explicitly clear + a stored secret, clients may send an empty string, which is converted + to ``None`` before persisting. + """ + merged_env = {} + for k, v in incoming.env.items(): + if v == _MASKED_VALUE: + if k in existing.env: + merged_env[k] = existing.env[k] + else: + raise HTTPException( + status_code=400, + detail=f"Cannot set env key '{k}' to masked value '***'; provide a real value.", + ) + else: + merged_env[k] = v + + merged_headers = {} + for k, v in incoming.headers.items(): + if v == _MASKED_VALUE: + if k in existing.headers: + merged_headers[k] = existing.headers[k] + else: + raise HTTPException( + status_code=400, + detail=f"Cannot set header '{k}' to masked value '***'; provide a real value.", + ) + else: + merged_headers[k] = v + + merged_oauth = incoming.oauth + if incoming.oauth is not None and existing.oauth is not None: + # None = preserve (masked round-trip), "" = explicitly clear, else = new value + merged_client_secret = existing.oauth.client_secret if incoming.oauth.client_secret is None else (None if incoming.oauth.client_secret == "" else incoming.oauth.client_secret) + merged_refresh_token = existing.oauth.refresh_token if incoming.oauth.refresh_token is None else (None if incoming.oauth.refresh_token == "" else incoming.oauth.refresh_token) + merged_oauth = incoming.oauth.model_copy( + update={ + "client_secret": merged_client_secret, + "refresh_token": merged_refresh_token, + } + ) + return incoming.model_copy( + update={ + "env": merged_env, + "headers": merged_headers, + "oauth": merged_oauth, + } + ) + + @router.get( "/mcp/config", response_model=McpConfigResponse, @@ -83,7 +176,7 @@ async def get_mcp_configuration() -> McpConfigResponse: "enabled": true, "command": "npx", "args": ["-y", "@modelcontextprotocol/server-github"], - "env": {"GITHUB_TOKEN": "ghp_xxx"}, + "env": {"GITHUB_TOKEN": "***"}, "description": "GitHub MCP server for repository operations" } } @@ -92,7 +185,8 @@ async def get_mcp_configuration() -> McpConfigResponse: """ config = get_extensions_config() - return McpConfigResponse(mcp_servers={name: McpServerConfigResponse(**server.model_dump()) for name, server in config.mcp_servers.items()}) + servers = {name: _mask_server_config(McpServerConfigResponse(**server.model_dump())) for name, server in config.mcp_servers.items()} + return McpConfigResponse(mcp_servers=servers) @router.put( @@ -142,14 +236,39 @@ async def update_mcp_configuration(request: McpConfigUpdateRequest) -> McpConfig config_path = Path.cwd().parent / "extensions_config.json" logger.info(f"No existing extensions config found. Creating new config at: {config_path}") - # Load current config to preserve skills configuration + # Load current config to preserve skills current_config = get_extensions_config() - # Convert request to dict format for JSON serialization - config_data = { - "mcpServers": {name: server.model_dump() for name, server in request.mcp_servers.items()}, - "skills": {name: {"enabled": skill.enabled} for name, skill in current_config.skills.items()}, - } + # Load raw (un-resolved) JSON from disk to use as the merge source. + # This preserves $VAR placeholders in env values and top-level keys + # like mcpInterceptors that would otherwise be lost. + raw_servers: dict[str, dict] = {} + raw_other_keys: dict = {} + if config_path is not None and config_path.exists(): + with open(config_path, encoding="utf-8") as f: + raw_data = json.load(f) + raw_servers = raw_data.get("mcpServers", {}) + # Preserve any top-level keys beyond mcpServers/skills + for key, value in raw_data.items(): + if key not in ("mcpServers", "skills"): + raw_other_keys[key] = value + + # Merge incoming server configs with raw on-disk secrets + merged_servers: dict[str, McpServerConfigResponse] = {} + for name, incoming in request.mcp_servers.items(): + raw_server = raw_servers.get(name) + if raw_server is not None: + merged_servers[name] = _merge_preserving_secrets( + incoming, + McpServerConfigResponse(**raw_server), + ) + else: + merged_servers[name] = incoming + + # Build config data preserving all top-level keys from the original file + config_data = dict(raw_other_keys) + config_data["mcpServers"] = {name: server.model_dump() for name, server in merged_servers.items()} + config_data["skills"] = {name: {"enabled": skill.enabled} for name, skill in current_config.skills.items()} # Write the configuration to file with open(config_path, "w", encoding="utf-8") as f: @@ -162,7 +281,8 @@ async def update_mcp_configuration(request: McpConfigUpdateRequest) -> McpConfig # Reload the configuration and update the global cache reloaded_config = reload_extensions_config() - return McpConfigResponse(mcp_servers={name: McpServerConfigResponse(**server.model_dump()) for name, server in reloaded_config.mcp_servers.items()}) + servers = {name: _mask_server_config(McpServerConfigResponse(**server.model_dump())) for name, server in reloaded_config.mcp_servers.items()} + return McpConfigResponse(mcp_servers=servers) except Exception as e: logger.error(f"Failed to update MCP configuration: {e}", exc_info=True) diff --git a/backend/tests/test_mcp_config_secrets.py b/backend/tests/test_mcp_config_secrets.py new file mode 100644 index 0000000000..831b8611b5 --- /dev/null +++ b/backend/tests/test_mcp_config_secrets.py @@ -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"