fix(security): mask sensitive values in MCP config API responses by sunshine-lang · Pull Request #2667 · bytedance/deer-flow

sunshine-lang · 2026-04-30T05:58:35Z

Summary

Mask env and headers dict values to "***" in GET/PUT /api/mcp/config responses
Remove oauth.client_secret and oauth.refresh_token from API responses (set to null)
Preserve existing secrets during PUT when frontend round-trips masked values back
15 new unit tests covering masking, merge, and full round-trip behavior

Security Motivation

GET /api/mcp/config returned plaintext secrets — the full env dict (with resolved API keys like GITHUB_TOKEN), headers (with auth tokens), and oauth.client_secret/refresh_token. Any authenticated user could read all MCP service credentials.

This is a data redaction fix: secrets remain in the config file and are used internally by MCP server processes, but are no longer exposed through the HTTP API.

Frontend Compatibility

The frontend only reads enabled and description from the config (MCPServerConfig extends Record<string, unknown>). The useEnableMCPServer hook GETs the full config, toggles enabled, and PUTs it back. The merge logic ensures masked values ("***") sent back via PUT are replaced with the real secrets from disk — no data loss on round-trip.

Zero breaking changes. The API shape is preserved; only sensitive values are redacted.

Changes

File	Change
`backend/app/gateway/routers/mcp.py`	Add `_mask_server_config()` and `_merge_preserving_secrets()` helpers; apply in GET and PUT handlers
`backend/tests/test_mcp_config_secrets.py`	15 unit tests: masking, merge, edge cases, round-trip

Relationship to PR #1646

PR #1646 takes a more aggressive approach (stripping all transport fields from the API, only exposing enabled/description). This PR takes a backward-compatible masking approach — the API structure is preserved, values are redacted. This is less invasive and can ship independently while #1646 is evaluated.

Test Plan

15 new unit tests pass (test_mcp_config_secrets.py)
Existing MCP tests pass (test_mcp_client_config.py, test_mcp_oauth.py, test_mcp_custom_interceptors.py)
Client conformance tests pass (test_client.py::TestMcpConfig)
ruff check + ruff format --check pass
GET returns "***" for env/header values, null for OAuth secrets
PUT preserves existing secrets when receiving masked values
PUT accepts new secret values (non-masked) correctly

🤖 Generated with Claude Code

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>

Copilot

Pull request overview

This PR reduces secret exposure from the gateway MCP configuration API by redacting sensitive fields in /api/mcp/config responses and attempting to preserve secrets when masked values are round-tripped back via PUT.

Changes:

Add masking helper to redact env/headers values and null out OAuth secrets in MCP config responses.
Add merge helper to preserve secrets when PUT receives masked placeholders/nulls.
Add unit tests covering masking, merge semantics, and round-trip behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
`backend/app/gateway/routers/mcp.py`	Implements masking/merge helpers and applies them to GET/PUT responses and updates.
`backend/tests/test_mcp_config_secrets.py`	Adds unit tests for masking, merge behavior, and a simulated frontend round-trip.

Copilot · 2026-04-30T07:16:18Z

+        # Convert merged servers to dict format for JSON serialization
        config_data = {
-            "mcpServers": {name: server.model_dump() for name, server in request.mcp_servers.items()},
+            "mcpServers": {name: server.model_dump() for name, server in merged_servers.items()},
            "skills": {name: {"enabled": skill.enabled} for name, skill in current_config.skills.items()},
        }


config_data is written with only mcpServers and skills. Any other top-level keys present in the existing extensions_config.json (e.g. mcpInterceptors in extensions_config.example.json) will be silently dropped on every PUT. Consider loading the existing JSON and updating only the mcpServers/skills keys (preserving the rest) to avoid destructive updates.

Good catch. Fixed in 0dc96c3 — the PUT handler now loads the raw JSON file and preserves all top-level keys (not just mcpServers/skills). Keys like mcpInterceptors are carried through on every write.

Copilot · 2026-04-30T07:16:19Z

+        # Load current config to preserve skills and secrets
        current_config = get_extensions_config()

-        # Convert request to dict format for JSON serialization
+        # Merge incoming server configs with existing secrets
+        merged_servers: dict[str, McpServerConfigResponse] = {}
+        for name, incoming in request.mcp_servers.items():
+            existing_server = current_config.mcp_servers.get(name)
+            if existing_server is not None:
+                merged_servers[name] = _merge_preserving_secrets(
+                    incoming,
+                    McpServerConfigResponse(**existing_server.model_dump()),
+                )


get_extensions_config() resolves $VAR placeholders to their environment values (see ExtensionsConfig.resolve_env_variables). Using those resolved values as the “existing secrets” source means a PUT round-trip can persist plaintext secrets into extensions_config.json and/or replace $VAR placeholders with the resolved secret. Consider preserving the raw on-disk values (load the JSON without env resolution, or keep both raw+resolved forms) and only merge masked fields from the raw config so placeholders aren’t lost and secrets aren’t written to disk unintentionally.

Fixed in 0dc96c3. The PUT handler now reads the raw extensions_config.json from disk (without $VAR resolution) and uses those un-resolved values as the merge source. This preserves $VAR placeholders on round-trip instead of persisting plaintext secrets.

Copilot · 2026-04-30T07:16:19Z

+    merged_env = {k: (existing.env.get(k, v) if v == _MASKED_VALUE else v) for k, v in incoming.env.items()}
+    merged_headers = {k: (existing.headers.get(k, v) if v == _MASKED_VALUE else v) for k, v in incoming.headers.items()}


_merge_preserving_secrets keeps the masked sentinel value ("***") when the key doesn’t exist in existing.env/existing.headers. If a client sends a new key with "***", that placeholder will be written back to the config file and likely break the server config. Suggest rejecting "***" for keys that don’t already exist (400), or requiring callers to provide a real value for new keys.

Fixed in 0dc96c3. _merge_preserving_secrets now raises HTTP 400 if a masked value *** is sent for a key that doesn't exist in the existing config. New keys must provide a real value.

Copilot · 2026-04-30T07:16:19Z

+    """
+    merged_env = {k: (existing.env.get(k, v) if v == _MASKED_VALUE else v) for k, v in incoming.env.items()}
+    merged_headers = {k: (existing.headers.get(k, v) if v == _MASKED_VALUE else v) for k, v in incoming.headers.items()}
+    merged_oauth = incoming.oauth
+    if incoming.oauth is not None and existing.oauth is not None:
+        merged_oauth = incoming.oauth.model_copy(
+            update={
+                "client_secret": (existing.oauth.client_secret if incoming.oauth.client_secret is None else incoming.oauth.client_secret),
+                "refresh_token": (existing.oauth.refresh_token if incoming.oauth.refresh_token is None else incoming.oauth.refresh_token),


OAuth secrets are preserved whenever the incoming client_secret/refresh_token are None. Since GET masks these fields by returning null, this also makes it impossible for API clients to intentionally clear an existing secret via PUT (sending null will always keep the old value). If clearing should be supported, add an explicit clear mechanism (e.g., allow empty string, or a separate clear_* flag) and document the behavior.

Suggested change

"""

merged_env = {k: (existing.env.get(k, v) if v == _MASKED_VALUE else v) for k, v in incoming.env.items()}

merged_headers = {k: (existing.headers.get(k, v) if v == _MASKED_VALUE else v) for k, v in incoming.headers.items()}

merged_oauth = incoming.oauth

if incoming.oauth is not None and existing.oauth is not None:

merged_oauth = incoming.oauth.model_copy(

update={

"client_secret": (existing.oauth.client_secret if incoming.oauth.client_secret is None else incoming.oauth.client_secret),

"refresh_token": (existing.oauth.refresh_token if incoming.oauth.refresh_token is None else incoming.oauth.refresh_token),

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 = {k: (existing.env.get(k, v) if v == _MASKED_VALUE else v) for k, v in incoming.env.items()}

merged_headers = {k: (existing.headers.get(k, v) if v == _MASKED_VALUE else v) for k, v in incoming.headers.items()}

merged_oauth = incoming.oauth

if incoming.oauth is not None and existing.oauth is not None:

merged_client_secret = existing.oauth.client_secret

if incoming.oauth.client_secret == "":

merged_client_secret = None

elif incoming.oauth.client_secret is not None:

merged_client_secret = incoming.oauth.client_secret

merged_refresh_token = existing.oauth.refresh_token

if incoming.oauth.refresh_token == "":

merged_refresh_token = None

elif incoming.oauth.refresh_token is not None:

merged_refresh_token = incoming.oauth.refresh_token

merged_oauth = incoming.oauth.model_copy(

update={

"client_secret": merged_client_secret,

"refresh_token": merged_refresh_token,

Fixed in 0dc96c3. Adopted the suggested approach: None preserves existing (for masked round-trip), empty string "" explicitly clears the secret, any other value replaces it. Added tests for both clearing and preserving behavior.

- 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>

WillemJiang requested a review from Copilot April 30, 2026 07:09

Copilot started reviewing on behalf of WillemJiang April 30, 2026 07:10 View session

Copilot AI reviewed Apr 30, 2026

View reviewed changes

WillemJiang added the reviewing The PR is in reviewing status label Apr 30, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

fix(security): mask sensitive values in MCP config API responses#2667
sunshine-lang wants to merge 2 commits intobytedance:mainfrom
sunshine-lang:fix/security-mcp-config-mask-secrets

sunshine-lang commented Apr 30, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 30, 2026

Uh oh!

sunshine-lang Apr 30, 2026

Uh oh!

Copilot AI Apr 30, 2026

Uh oh!

sunshine-lang Apr 30, 2026

Uh oh!

Copilot AI Apr 30, 2026

Uh oh!

sunshine-lang Apr 30, 2026

Uh oh!

Copilot AI Apr 30, 2026

Uh oh!

sunshine-lang Apr 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		merged_env = {k: (existing.env.get(k, v) if v == _MASKED_VALUE else v) for k, v in incoming.env.items()}
		merged_headers = {k: (existing.headers.get(k, v) if v == _MASKED_VALUE else v) for k, v in incoming.headers.items()}

-    """
-    merged_env = {k: (existing.env.get(k, v) if v == _MASKED_VALUE else v) for k, v in incoming.env.items()}
-    merged_headers = {k: (existing.headers.get(k, v) if v == _MASKED_VALUE else v) for k, v in incoming.headers.items()}
-    merged_oauth = incoming.oauth
-    if incoming.oauth is not None and existing.oauth is not None:
-        merged_oauth = incoming.oauth.model_copy(
-            update={
-                "client_secret": (existing.oauth.client_secret if incoming.oauth.client_secret is None else incoming.oauth.client_secret),
-                "refresh_token": (existing.oauth.refresh_token if incoming.oauth.refresh_token is None else incoming.oauth.refresh_token),
+    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 = {k: (existing.env.get(k, v) if v == _MASKED_VALUE else v) for k, v in incoming.env.items()}
+    merged_headers = {k: (existing.headers.get(k, v) if v == _MASKED_VALUE else v) for k, v in incoming.headers.items()}
+    merged_oauth = incoming.oauth
+    if incoming.oauth is not None and existing.oauth is not None:
+        merged_client_secret = existing.oauth.client_secret
+        if incoming.oauth.client_secret == "":
+            merged_client_secret = None
+        elif incoming.oauth.client_secret is not None:
+            merged_client_secret = incoming.oauth.client_secret
+        merged_refresh_token = existing.oauth.refresh_token
+        if incoming.oauth.refresh_token == "":
+            merged_refresh_token = None
+        elif incoming.oauth.refresh_token is not None:
+            merged_refresh_token = incoming.oauth.refresh_token
+        merged_oauth = incoming.oauth.model_copy(
+            update={
+                "client_secret": merged_client_secret,
+                "refresh_token": merged_refresh_token,

Conversation

sunshine-lang commented Apr 30, 2026

Summary

Security Motivation

Frontend Compatibility

Changes

Relationship to PR #1646

Test Plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

sunshine-lang Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

sunshine-lang Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

sunshine-lang Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

sunshine-lang Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants