From 921a68908bffab86963debd926f779af9ff23e3c Mon Sep 17 00:00:00 2001 From: Swarit Pandey Date: Sat, 15 Aug 2026 12:42:32 +0530 Subject: [PATCH] fix(mcp): redact secrets kept in command/args/url fields env and headers were dropped outright, but a credential embedded in the server URL or in an args flag passed through unredacted. Signed-off-by: Swarit Pandey --- internal/detector/mcp.go | 10 ++++++++-- internal/detector/mcp_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/internal/detector/mcp.go b/internal/detector/mcp.go index b4361f2..833c69c 100644 --- a/internal/detector/mcp.go +++ b/internal/detector/mcp.go @@ -9,6 +9,7 @@ import ( "github.com/tailscale/hujson" + "github.com/step-security/dev-machine-guard/internal/aiagents/redact" "github.com/step-security/dev-machine-guard/internal/executor" "github.com/step-security/dev-machine-guard/internal/model" "github.com/step-security/dev-machine-guard/internal/tcc" @@ -288,7 +289,12 @@ func filterProjectScopedMCPServers(projectsRaw json.RawMessage) map[string]any { return filtered } -// filterServerFields keeps only command, args, serverUrl, url from each server entry. +// filterServerFields keeps only command, args, serverUrl, url from each server +// entry. env and headers are dropped outright since those are the fields +// vendors document for credentials. But command/args/url/serverUrl are +// real-world credential locations too (a bearer token or API key in a query +// string, an --api-key flag baked into args), so the kept values still pass +// through redact.Value before being uploaded. func filterServerFields(serversRaw json.RawMessage) map[string]any { var servers map[string]map[string]any if err := json.Unmarshal(serversRaw, &servers); err != nil { @@ -302,7 +308,7 @@ func filterServerFields(serversRaw json.RawMessage) map[string]any { filtered := make(map[string]any) for k, v := range serverConfig { if allowedKeys[k] { - filtered[k] = v + filtered[k] = redact.Value(v) } } result[name] = filtered diff --git a/internal/detector/mcp_test.go b/internal/detector/mcp_test.go index 679f677..96860db 100644 --- a/internal/detector/mcp_test.go +++ b/internal/detector/mcp_test.go @@ -818,6 +818,32 @@ func TestFilterMCPContent_NonOpenCodeUnchanged(t *testing.T) { } } +// TestFilterServerFields_RedactsSecretsInKeptFields: command/args/url/serverUrl +// are the fields we keep (env/headers are dropped outright), but real MCP +// configs sometimes carry a bearer token or API key inside those kept fields +// too — e.g. a query-string token on the server URL, or an --api-key flag in +// args. Those values must still be redacted before upload, not passed through +// verbatim just because the field name isn't "env" or "headers". +func TestFilterServerFields_RedactsSecretsInKeptFields(t *testing.T) { + det := &MCPDetector{} + content := `{"mcpServers":{"pipeboard":{ + "url":"https://meta-ads.mcp.pipeboard.co/?token=abcdEFGH12345678opaqueTokenValue", + "command":"npx", + "args":["-y","server","--api-key=abcdEFGH12345678opaqueTokenValue"] + }}}` + + filtered, ok := det.filterMCPContent("cursor", "/Users/testuser/.cursor/mcp.json", []byte(content)) + if !ok { + t.Fatalf("expected filtering to succeed") + } + if strings.Contains(string(filtered), "abcdEFGH12345678opaqueTokenValue") { + t.Fatalf("secret leaked into filtered output: %s", filtered) + } + if !strings.Contains(string(filtered), "token=") || !strings.Contains(string(filtered), "REDACTED") { + t.Errorf("expected the url token to be replaced with a redaction placeholder, got: %s", filtered) + } +} + // TestMCPConfigDefinitions_OpenCodeIsPlatformAgnostic: the two OpenCode // definitions leave the Windows and Linux fields empty on purpose, so every // consumer that walks mcpConfigDefinitions — including the known-user-config