From bd286ae5181efafd1f548b8025566bfbe9a03200 Mon Sep 17 00:00:00 2001 From: Reynier Ortiz Vega Date: Fri, 21 Aug 2026 09:37:15 -0400 Subject: [PATCH 01/10] fix(streamable-proxy): scan and redact secrets in tools/call results The streamable-HTTP proxy relayed tool_call result content to the client unmodified, with no inspection of what an MCP backend returned. A compromised or malicious backend could embed credentials in a tool result and have them delivered straight through, with no containment boundary (DAST finding: "Tool results are relayed without content inspection"). Add pkg/mcp/secretscan, a best-effort scanner that recognizes common credential shapes (AWS keys, GitHub/Slack/Google/Stripe tokens, JWTs, PEM private keys) in TextContent and redacts matches. Wire it into both response-writing paths in the streamable proxy (plain JSON and SSE final frame) so every tools/call response is scanned before reaching the client. Decode failures fail open -- this must never be the reason a legitimate tool call breaks. Not covered here: the legacy SSE/transparent proxy (a raw byte-forwarding reverse proxy, needs a different hook) and binary tool-result content (images/audio). --- pkg/mcp/secretscan/secretscan.go | 129 ++++++++++++++++++ pkg/mcp/secretscan/secretscan_test.go | 84 ++++++++++++ .../proxy/streamable/secretscan_test.go | 79 +++++++++++ .../proxy/streamable/streamable_proxy.go | 36 ++++- 4 files changed, 326 insertions(+), 2 deletions(-) create mode 100644 pkg/mcp/secretscan/secretscan.go create mode 100644 pkg/mcp/secretscan/secretscan_test.go create mode 100644 pkg/transport/proxy/streamable/secretscan_test.go diff --git a/pkg/mcp/secretscan/secretscan.go b/pkg/mcp/secretscan/secretscan.go new file mode 100644 index 0000000000..ab76a27d63 --- /dev/null +++ b/pkg/mcp/secretscan/secretscan.go @@ -0,0 +1,129 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +// Package secretscan provides a best-effort content inspector for MCP +// tool-call results relayed through a ToolHive proxy. +// +// Background: the proxy sits between the calling client (e.g. an LLM agent) +// and an MCP backend server. The backend is not fully trusted -- it may be +// misconfigured, compromised, or malicious -- yet its tool-call output is +// otherwise forwarded to the client byte-for-byte on both the streamable-HTTP +// and legacy SSE transports (see pkg/transport/proxy/streamable and +// pkg/transport/proxy/transparent). A backend that returns credential-shaped +// text in a tool result would have that text delivered straight through, +// with no containment boundary. This package closes that specific gap: it +// recognizes common credential shapes in `tools/call` response text content +// and redacts them in place before the response reaches the client. +// +// This is defense-in-depth, not a content firewall: it only pattern-matches +// well-known credential formats in TextContent. It does not decode or scan +// binary/base64 payloads (ImageContent, AudioContent, embedded resource +// blobs), and it cannot catch secrets that don't match a known shape. Callers +// should treat a scan failure (malformed result JSON) as non-fatal and +// forward the original content unchanged -- this package must never be the +// reason a legitimate tool call breaks. +package secretscan + +import ( + "encoding/json" + "fmt" + "regexp" + + sdkmcp "github.com/stacklok/toolhive-core/mcpcompat/mcp" +) + +// redactionPlaceholder replaces a matched secret. It intentionally carries no +// information about the matched value (not even its length or pattern name) +// so the redaction itself cannot leak anything about the secret it replaced. +const redactionPlaceholder = "[REDACTED-BY-TOOLHIVE]" + +// patterns lists the credential shapes scanned for. Each is a high-confidence, +// low-false-positive match on a known secret format; deliberately narrow +// rather than a generic "assignment to a sensitive-looking key name" heuristic, +// which would false-positive constantly on ordinary tool output. +var patterns = []*regexp.Regexp{ + // AWS access key ID. + regexp.MustCompile(`\bAKIA[0-9A-Z]{16}\b`), + // GitHub personal access / app / OAuth / refresh tokens. + regexp.MustCompile(`\bgh[pousr]_[0-9A-Za-z]{36,}\b`), + regexp.MustCompile(`\bgithub_pat_[0-9A-Za-z_]{22,}\b`), + // Slack tokens (bot/user/app/legacy). + regexp.MustCompile(`\bxox[baprs]-[0-9A-Za-z-]{10,}\b`), + // Google API key. + regexp.MustCompile(`\bAIza[0-9A-Za-z_-]{35}\b`), + // Stripe live/test secret keys. + regexp.MustCompile(`\bsk_(?:live|test)_[0-9A-Za-z]{16,}\b`), + // Generic JWT (three dot-separated base64url segments). + regexp.MustCompile(`\bey[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,}\b`), + // PEM-encoded private key blocks (RSA/EC/PKCS8/OpenSSH/generic). + regexp.MustCompile(`(?s)-----BEGIN [A-Z ]*PRIVATE KEY-----.*?-----END [A-Z ]*PRIVATE KEY-----`), + regexp.MustCompile(`(?s)-----BEGIN OPENSSH PRIVATE KEY-----.*?-----END OPENSSH PRIVATE KEY-----`), +} + +// Result reports what ScanAndRedactToolCallResult did. +type Result struct { + // Redacted is the (possibly modified) result payload, always safe to + // forward to the client. + Redacted json.RawMessage + // Matched is true if one or more patterns matched and were redacted. + Matched bool +} + +// ScanAndRedactToolCallResult inspects the `result` payload of a +// `tools/call` JSON-RPC response and redacts any TextContent entries that +// match a known credential shape. +// +// It is best-effort and fails open: if raw cannot be decoded as an MCP +// CallToolResult, it is returned unchanged with Matched=false and a non-nil +// error describing why decoding failed, so the caller can log it without +// treating it as a reason to block the response. +func ScanAndRedactToolCallResult(raw json.RawMessage) (Result, error) { + if len(raw) == 0 { + return Result{Redacted: raw}, nil + } + + var result sdkmcp.CallToolResult + if err := json.Unmarshal(raw, &result); err != nil { + return Result{Redacted: raw}, fmt.Errorf("decoding tool call result: %w", err) + } + + matched := false + for i, c := range result.Content { + text, ok := c.(sdkmcp.TextContent) + if !ok { + continue + } + redactedText, hit := redactText(text.Text) + if !hit { + continue + } + matched = true + text.Text = redactedText + result.Content[i] = text + } + + if !matched { + return Result{Redacted: raw}, nil + } + + encoded, err := json.Marshal(result) + if err != nil { + // Should not happen -- result round-tripped through the same type's + // (Un)MarshalJSON -- but fail open rather than block the response. + return Result{Redacted: raw}, fmt.Errorf("re-encoding redacted tool call result: %w", err) + } + return Result{Redacted: encoded, Matched: true}, nil +} + +// redactText replaces every pattern match in s with redactionPlaceholder. +// Returns the (possibly unmodified) string and whether anything matched. +func redactText(s string) (string, bool) { + matched := false + for _, p := range patterns { + if p.MatchString(s) { + matched = true + s = p.ReplaceAllString(s, redactionPlaceholder) + } + } + return s, matched +} diff --git a/pkg/mcp/secretscan/secretscan_test.go b/pkg/mcp/secretscan/secretscan_test.go new file mode 100644 index 0000000000..747450bf23 --- /dev/null +++ b/pkg/mcp/secretscan/secretscan_test.go @@ -0,0 +1,84 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package secretscan + +import ( + "encoding/json" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestScanAndRedactToolCallResult_RedactsKnownCredentialShapes(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + text string + }{ + {"aws access key", "your key is " + "AKIA" + strings.Repeat("Q", 16) + ", keep it safe"}, + {"github pat", "token: ghp_" + strings.Repeat("a", 36)}, + {"slack token", "xoxb-" + strings.Repeat("1", 10) + "-" + strings.Repeat("a", 16)}, + {"google api key", "AIza" + strings.Repeat("A", 35)}, + {"stripe secret key", "sk_live_" + strings.Repeat("a", 24)}, + {"jwt", "ey" + strings.Repeat("A", 12) + "." + strings.Repeat("B", 12) + "." + strings.Repeat("C", 12)}, + {"pem private key", "-----BEGIN " + "RSA PRIVATE KEY-----" + "\nMIIBogIBAAJ...\n" + "-----END " + "RSA PRIVATE KEY-----"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + raw := callToolResultJSON(t, tc.text) + + result, err := ScanAndRedactToolCallResult(raw) + require.NoError(t, err) + require.True(t, result.Matched) + require.NotContains(t, string(result.Redacted), tc.text) + require.Contains(t, string(result.Redacted), redactionPlaceholder) + }) + } +} + +func TestScanAndRedactToolCallResult_LeavesOrdinaryTextUnchanged(t *testing.T) { + t.Parallel() + + raw := callToolResultJSON(t, "the weather in NYC is 72F and sunny") + + result, err := ScanAndRedactToolCallResult(raw) + require.NoError(t, err) + require.False(t, result.Matched) + require.JSONEq(t, string(raw), string(result.Redacted)) +} + +func TestScanAndRedactToolCallResult_FailsOpenOnMalformedInput(t *testing.T) { + t.Parallel() + + raw := json.RawMessage(`{not valid json`) + + result, err := ScanAndRedactToolCallResult(raw) + require.Error(t, err) + require.False(t, result.Matched) + require.Equal(t, raw, result.Redacted) +} + +func TestScanAndRedactToolCallResult_EmptyInput(t *testing.T) { + t.Parallel() + + result, err := ScanAndRedactToolCallResult(nil) + require.NoError(t, err) + require.False(t, result.Matched) +} + +func callToolResultJSON(t *testing.T, text string) json.RawMessage { + t.Helper() + payload := map[string]any{ + "content": []map[string]any{ + {"type": "text", "text": text}, + }, + } + b, err := json.Marshal(payload) + require.NoError(t, err) + return b +} diff --git a/pkg/transport/proxy/streamable/secretscan_test.go b/pkg/transport/proxy/streamable/secretscan_test.go new file mode 100644 index 0000000000..32b0e4b22b --- /dev/null +++ b/pkg/transport/proxy/streamable/secretscan_test.go @@ -0,0 +1,79 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package streamable + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/require" + "golang.org/x/exp/jsonrpc2" + + sdkmcp "github.com/stacklok/toolhive-core/mcpcompat/mcp" +) + +func TestInspectToolCallResponse_RedactsCredentialShapedToolResult(t *testing.T) { + t.Parallel() + + ghToken := "ghp_" + strings.Repeat("a", 36) + resp, err := jsonrpc2.NewResponse( + jsonrpc2.Int64ID(1), + map[string]any{ + "content": []map[string]any{ + {"type": "text", "text": "here is the token: " + ghToken}, + }, + }, + nil, + ) + require.NoError(t, err) + + out := inspectToolCallResponse(string(sdkmcp.MethodToolsCall), resp) + + outResp, ok := out.(*jsonrpc2.Response) + require.True(t, ok) + require.NotContains(t, string(outResp.Result), ghToken) + require.Contains(t, string(outResp.Result), "REDACTED-BY-TOOLHIVE") +} + +func TestInspectToolCallResponse_IgnoresNonToolCallMethods(t *testing.T) { + t.Parallel() + + ghToken := "ghp_" + strings.Repeat("a", 36) + resp, err := jsonrpc2.NewResponse( + jsonrpc2.Int64ID(1), + map[string]any{ + "content": []map[string]any{ + {"type": "text", "text": ghToken}, + }, + }, + nil, + ) + require.NoError(t, err) + + out := inspectToolCallResponse("resources/read", resp) + + require.Same(t, resp, out) +} + +func TestInspectToolCallResponse_IgnoresErrorResponses(t *testing.T) { + t.Parallel() + + resp, err := jsonrpc2.NewResponse(jsonrpc2.Int64ID(1), nil, jsonrpc2.NewError(-32000, "boom")) + require.NoError(t, err) + + out := inspectToolCallResponse(string(sdkmcp.MethodToolsCall), resp) + + require.Same(t, resp, out) +} + +func TestInspectToolCallResponse_IgnoresNonResponseMessages(t *testing.T) { + t.Parallel() + + req, err := jsonrpc2.NewNotification("notifications/message", nil) + require.NoError(t, err) + + out := inspectToolCallResponse(string(sdkmcp.MethodToolsCall), req) + + require.Same(t, req, out) +} diff --git a/pkg/transport/proxy/streamable/streamable_proxy.go b/pkg/transport/proxy/streamable/streamable_proxy.go index 6d339d1b7e..4a1c1bb777 100644 --- a/pkg/transport/proxy/streamable/streamable_proxy.go +++ b/pkg/transport/proxy/streamable/streamable_proxy.go @@ -27,6 +27,7 @@ import ( "github.com/stacklok/toolhive/pkg/diagnostics" "github.com/stacklok/toolhive/pkg/healthcheck" "github.com/stacklok/toolhive/pkg/mcp" + "github.com/stacklok/toolhive/pkg/mcp/secretscan" "github.com/stacklok/toolhive/pkg/transport/session" "github.com/stacklok/toolhive/pkg/transport/types" ) @@ -663,6 +664,8 @@ func (p *HTTPProxy) handleSingleRequest( return } + msg = inspectToolCallResponse(req.Method, msg) + if setSessionHeader { w.Header().Set("Mcp-Session-Id", sessID) } @@ -735,7 +738,7 @@ func (p *HTTPProxy) handleSingleRequestSSE( // Progress does not end the request; keep waiting for more // progress or the final response. case msg := <-waitCh: - p.writeSingleRequestSSEFinalResponse(w, flusher, msg, ck) + p.writeSingleRequestSSEFinalResponse(w, flusher, req.Method, msg, ck) return case <-ctx.Done(): writeSSEErrorEvent(w, flusher, req.ID, ctx.Err()) @@ -756,7 +759,7 @@ func (p *HTTPProxy) handleSingleRequestSSE( // logged; the client simply does not get a final frame in that case, matching // the prior (pre-progress) behavior's error handling. func (p *HTTPProxy) writeSingleRequestSSEFinalResponse( - w http.ResponseWriter, flusher http.Flusher, msg jsonrpc2.Message, ck string, + w http.ResponseWriter, flusher http.Flusher, method string, msg jsonrpc2.Message, ck string, ) { finalMsg := msg if r, ok := msg.(*jsonrpc2.Response); ok && r.ID.IsValid() { @@ -767,6 +770,7 @@ func (p *HTTPProxy) writeSingleRequestSSEFinalResponse( } finalMsg = restored } + finalMsg = inspectToolCallResponse(method, finalMsg) data, err := jsonrpc2.EncodeMessage(finalMsg) if err != nil { @@ -778,6 +782,34 @@ func (p *HTTPProxy) writeSingleRequestSSEFinalResponse( } } +// inspectToolCallResponse applies best-effort secret redaction (see +// pkg/mcp/secretscan) to the result of a tools/call response before it is +// forwarded to the client. The MCP backend behind this proxy is not fully +// trusted (it may be misconfigured, compromised, or malicious), so its tool +// output is scanned for credential-shaped text before the proxy relays it. +// Any other method, a non-Response message, an error response, or a result +// that fails to decode is returned unchanged -- this check must never be the +// reason a legitimate tool call breaks. +func inspectToolCallResponse(method string, msg jsonrpc2.Message) jsonrpc2.Message { + if method != string(sdkmcp.MethodToolsCall) { + return msg + } + resp, ok := msg.(*jsonrpc2.Response) + if !ok || resp.Error != nil || len(resp.Result) == 0 { + return msg + } + scan, err := secretscan.ScanAndRedactToolCallResult(resp.Result) + if err != nil { + slog.Debug("tool call result secret scan skipped", "error", err) + return msg + } + if !scan.Matched { + return msg + } + slog.Warn("redacted credential-shaped content in tool call result") + return &jsonrpc2.Response{Result: scan.Redacted, ID: resp.ID} +} + // writeSSEErrorEvent writes a best-effort JSON-RPC error as a single SSE // message event (via writeSSEData), for a request whose response headers // (200 + text/event-stream) have already been sent -- an HTTP error status can From 43d9cf19719437a89d0c018e0d10165ff45c6f33 Mon Sep 17 00:00:00 2001 From: Reynier Ortiz Vega Date: Fri, 21 Aug 2026 10:12:06 -0400 Subject: [PATCH 02/10] feat(streamable-proxy): gate secret redaction behind opt-in config Make the tools/call secret-scanning from the previous commit opt-in rather than always-on, plumbed the same way as the existing --strict-protocol-validation knob: - streamable.WithSecretRedaction (proxy-level Option, default false) - StdioTransport.SetSecretRedaction / types.Config.RedactToolResultSecrets - runner.WithRedactToolResultSecrets / RunConfig.RedactToolResultSecrets (redact_tool_result_secrets in the run-config JSON/YAML) - thv run --redact-tool-result-secrets CLI flag - carried across `thv upgrade` via applier.go Default stays false: many deployments run fully operator-trusted backends where the scan is pure overhead, so operators opt in only where the backend isn't fully trusted. --- cmd/thv/app/run_flags.go | 9 ++++ pkg/runner/config.go | 6 +++ pkg/runner/config_builder.go | 11 ++++ pkg/runner/config_builder_test.go | 27 ++++++++++ pkg/runner/runner.go | 1 + pkg/transport/factory.go | 1 + .../proxy/streamable/secretscan_test.go | 51 ++++++++++++------- .../proxy/streamable/streamable_proxy.go | 26 ++++++++-- pkg/transport/stdio.go | 15 ++++++ pkg/transport/types/transport.go | 9 ++++ pkg/workloads/upgrade/applier.go | 1 + 11 files changed, 133 insertions(+), 24 deletions(-) diff --git a/cmd/thv/app/run_flags.go b/cmd/thv/app/run_flags.go index 695909bdc3..115622617b 100644 --- a/cmd/thv/app/run_flags.go +++ b/cmd/thv/app/run_flags.go @@ -106,6 +106,10 @@ type RunFlags struct { // on the streamable HTTP proxy. StrictProtocolValidation bool + // RedactToolResultSecrets enables best-effort credential-shape scanning + // on tools/call responses relayed by the streamable HTTP proxy. + RedactToolResultSecrets bool + // Endpoint prefix for SSE endpoint URLs EndpointPrefix string @@ -290,6 +294,10 @@ func AddRunFlags(cmd *cobra.Command, config *RunFlags) { cmd.Flags().BoolVar(&config.StrictProtocolValidation, "strict-protocol-validation", false, "Reject client requests whose MCP-Protocol-Version header is an unknown/unsupported MCP revision with HTTP 400 "+ "(streamable-HTTP proxy only; an absent header is accepted). Off by default: any version is accepted.") + cmd.Flags().BoolVar(&config.RedactToolResultSecrets, "redact-tool-result-secrets", false, + "Scan tools/call responses for credential-shaped content (AWS/GitHub/Slack/Google/Stripe keys, JWTs, PEM "+ + "private keys) and redact matches before relaying them to the client (streamable-HTTP proxy only). "+ + "Off by default; enable when the backend MCP server is not fully trusted.") cmd.Flags().BoolVar(&config.Stateless, "stateless", false, "Declare the server as stateless (POST-only, no SSE). "+ "Use for MCP servers implementing streamable-HTTP stateless mode.") @@ -705,6 +713,7 @@ func buildRunnerConfig( runner.WithAllowDockerGateway(runFlags.AllowDockerGateway), runner.WithTrustProxyHeaders(runFlags.TrustProxyHeaders), runner.WithStrictProtocolValidation(runFlags.StrictProtocolValidation), + runner.WithRedactToolResultSecrets(runFlags.RedactToolResultSecrets), runner.WithStateless(runFlags.Stateless), runner.WithSessionTTL(runFlags.SessionTTL), runner.WithEndpointPrefix(runFlags.EndpointPrefix), diff --git a/pkg/runner/config.go b/pkg/runner/config.go index 268c6692a8..37804981e3 100644 --- a/pkg/runner/config.go +++ b/pkg/runner/config.go @@ -213,6 +213,12 @@ type RunConfig struct { // version string (an absent header is always accepted in either mode). StrictProtocolValidation bool `json:"strict_protocol_validation,omitempty" yaml:"strict_protocol_validation,omitempty"` + // RedactToolResultSecrets enables best-effort credential-shape scanning + // on tools/call responses relayed by the streamable HTTP proxy: matches + // are redacted before the response reaches the client. Opt-in (default + // false); enable it when the backend MCP server is not fully trusted. + RedactToolResultSecrets bool `json:"redact_tool_result_secrets,omitempty" yaml:"redact_tool_result_secrets,omitempty"` + // Stateless indicates the server only supports POST (no SSE/GET). // When true, the proxy returns 405 for incoming GET requests and uses a // POST-based health check instead of the default GET probe. diff --git a/pkg/runner/config_builder.go b/pkg/runner/config_builder.go index 417c76a23f..36d61e8c1f 100644 --- a/pkg/runner/config_builder.go +++ b/pkg/runner/config_builder.go @@ -386,6 +386,17 @@ func WithStrictProtocolValidation(strict bool) RunConfigBuilderOption { } } +// WithRedactToolResultSecrets sets whether the streamable HTTP proxy scans +// tools/call responses for credential-shaped content and redacts matches +// before relaying them to the client. Opt-in (default false); enable it +// when the backend MCP server is not fully trusted. +func WithRedactToolResultSecrets(enabled bool) RunConfigBuilderOption { + return func(b *runConfigBuilder) error { + b.config.RedactToolResultSecrets = enabled + return nil + } +} + // WithStateless declares the server is stateless (POST-only, no SSE). func WithStateless(stateless bool) RunConfigBuilderOption { return func(b *runConfigBuilder) error { diff --git a/pkg/runner/config_builder_test.go b/pkg/runner/config_builder_test.go index 0087d97ffd..461d1095ef 100644 --- a/pkg/runner/config_builder_test.go +++ b/pkg/runner/config_builder_test.go @@ -1569,6 +1569,33 @@ func TestWithStrictProtocolValidation(t *testing.T) { } } +// TestWithRedactToolResultSecrets verifies the builder option sets +// RunConfig.RedactToolResultSecrets, mirroring WithStrictProtocolValidation's +// plumbing (see cmd/thv/app/run_flags.go's --redact-tool-result-secrets flag). +func TestWithRedactToolResultSecrets(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + enabled bool + }{ + {name: "enabled", enabled: true}, + {name: "disabled (default)", enabled: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + builder := &runConfigBuilder{config: NewRunConfig()} + err := WithRedactToolResultSecrets(tt.enabled)(builder) + + require.NoError(t, err) + assert.Equal(t, tt.enabled, builder.config.RedactToolResultSecrets) + }) + } +} + func TestResolveRegistryServerName(t *testing.T) { t.Parallel() diff --git a/pkg/runner/runner.go b/pkg/runner/runner.go index 41e978ddf6..e7d03d296b 100644 --- a/pkg/runner/runner.go +++ b/pkg/runner/runner.go @@ -226,6 +226,7 @@ func (r *Runner) Run(ctx context.Context) error { Debug: r.Config.Debug, TrustProxyHeaders: r.Config.TrustProxyHeaders, StrictProtocolValidation: r.Config.StrictProtocolValidation, + RedactToolResultSecrets: r.Config.RedactToolResultSecrets, EndpointPrefix: r.Config.EndpointPrefix, SessionTTL: effectiveSessionTTL, } diff --git a/pkg/transport/factory.go b/pkg/transport/factory.go index f54fb13a1f..0817563c8a 100644 --- a/pkg/transport/factory.go +++ b/pkg/transport/factory.go @@ -53,6 +53,7 @@ func (*Factory) Create(config types.Config, opts ...Option) (types.Transport, er ) stdio.SetProxyMode(config.ProxyMode) stdio.SetStrictProtocolValidation(config.StrictProtocolValidation) + stdio.SetSecretRedaction(config.RedactToolResultSecrets) if config.SessionStorage != nil { stdio.SetSessionStorage(config.SessionStorage) } diff --git a/pkg/transport/proxy/streamable/secretscan_test.go b/pkg/transport/proxy/streamable/secretscan_test.go index 32b0e4b22b..590d9b7b37 100644 --- a/pkg/transport/proxy/streamable/secretscan_test.go +++ b/pkg/transport/proxy/streamable/secretscan_test.go @@ -13,22 +13,29 @@ import ( sdkmcp "github.com/stacklok/toolhive-core/mcpcompat/mcp" ) -func TestInspectToolCallResponse_RedactsCredentialShapedToolResult(t *testing.T) { - t.Parallel() - - ghToken := "ghp_" + strings.Repeat("a", 36) +func toolCallResultResponse(t *testing.T, text string) *jsonrpc2.Response { + t.Helper() resp, err := jsonrpc2.NewResponse( jsonrpc2.Int64ID(1), map[string]any{ "content": []map[string]any{ - {"type": "text", "text": "here is the token: " + ghToken}, + {"type": "text", "text": text}, }, }, nil, ) require.NoError(t, err) + return resp +} + +func TestInspectToolCallResponse_RedactsCredentialShapedToolResult(t *testing.T) { + t.Parallel() + + ghToken := "ghp_" + strings.Repeat("a", 36) + resp := toolCallResultResponse(t, "here is the token: "+ghToken) + p := &HTTPProxy{redactToolResultSecrets: true} - out := inspectToolCallResponse(string(sdkmcp.MethodToolsCall), resp) + out := p.inspectToolCallResponse(string(sdkmcp.MethodToolsCall), resp) outResp, ok := out.(*jsonrpc2.Response) require.True(t, ok) @@ -36,22 +43,26 @@ func TestInspectToolCallResponse_RedactsCredentialShapedToolResult(t *testing.T) require.Contains(t, string(outResp.Result), "REDACTED-BY-TOOLHIVE") } +func TestInspectToolCallResponse_DisabledByDefault(t *testing.T) { + t.Parallel() + + ghToken := "ghp_" + strings.Repeat("a", 36) + resp := toolCallResultResponse(t, ghToken) + p := &HTTPProxy{} // redactToolResultSecrets left at its zero value (false) + + out := p.inspectToolCallResponse(string(sdkmcp.MethodToolsCall), resp) + + require.Same(t, resp, out) +} + func TestInspectToolCallResponse_IgnoresNonToolCallMethods(t *testing.T) { t.Parallel() ghToken := "ghp_" + strings.Repeat("a", 36) - resp, err := jsonrpc2.NewResponse( - jsonrpc2.Int64ID(1), - map[string]any{ - "content": []map[string]any{ - {"type": "text", "text": ghToken}, - }, - }, - nil, - ) - require.NoError(t, err) + resp := toolCallResultResponse(t, ghToken) + p := &HTTPProxy{redactToolResultSecrets: true} - out := inspectToolCallResponse("resources/read", resp) + out := p.inspectToolCallResponse("resources/read", resp) require.Same(t, resp, out) } @@ -61,8 +72,9 @@ func TestInspectToolCallResponse_IgnoresErrorResponses(t *testing.T) { resp, err := jsonrpc2.NewResponse(jsonrpc2.Int64ID(1), nil, jsonrpc2.NewError(-32000, "boom")) require.NoError(t, err) + p := &HTTPProxy{redactToolResultSecrets: true} - out := inspectToolCallResponse(string(sdkmcp.MethodToolsCall), resp) + out := p.inspectToolCallResponse(string(sdkmcp.MethodToolsCall), resp) require.Same(t, resp, out) } @@ -72,8 +84,9 @@ func TestInspectToolCallResponse_IgnoresNonResponseMessages(t *testing.T) { req, err := jsonrpc2.NewNotification("notifications/message", nil) require.NoError(t, err) + p := &HTTPProxy{redactToolResultSecrets: true} - out := inspectToolCallResponse(string(sdkmcp.MethodToolsCall), req) + out := p.inspectToolCallResponse(string(sdkmcp.MethodToolsCall), req) require.Same(t, req, out) } diff --git a/pkg/transport/proxy/streamable/streamable_proxy.go b/pkg/transport/proxy/streamable/streamable_proxy.go index 4a1c1bb777..7622adcab1 100644 --- a/pkg/transport/proxy/streamable/streamable_proxy.go +++ b/pkg/transport/proxy/streamable/streamable_proxy.go @@ -136,6 +136,12 @@ type HTTPProxy struct { // 2025-03-26 when the header is missing. Set via WithStrictProtocolValidation. strictProtocolValidation bool + // redactToolResultSecrets enables scanning tools/call responses for + // credential-shaped content before relaying them to the client (see + // pkg/mcp/secretscan). Default false: the MCP backend is often + // operator-trusted. Set via WithSecretRedaction. + redactToolResultSecrets bool + // Health checker healthChecker *healthcheck.HealthChecker @@ -216,6 +222,15 @@ func WithStrictProtocolValidation(enabled bool) Option { return func(p *HTTPProxy) { p.strictProtocolValidation = enabled } } +// WithSecretRedaction enables best-effort credential-shape scanning (see +// pkg/mcp/secretscan) on tools/call responses before they are relayed to the +// client. Opt-in (default false): the MCP backend behind this proxy is often +// operator-trusted, and scanning adds per-response overhead, so this is only +// worth enabling when the backend is not fully trusted. +func WithSecretRedaction(enabled bool) Option { + return func(p *HTTPProxy) { p.redactToolResultSecrets = enabled } +} + // NewHTTPProxy creates a new HTTPProxy for streamable HTTP transport. func NewHTTPProxy( host string, @@ -664,7 +679,7 @@ func (p *HTTPProxy) handleSingleRequest( return } - msg = inspectToolCallResponse(req.Method, msg) + msg = p.inspectToolCallResponse(req.Method, msg) if setSessionHeader { w.Header().Set("Mcp-Session-Id", sessID) @@ -770,7 +785,7 @@ func (p *HTTPProxy) writeSingleRequestSSEFinalResponse( } finalMsg = restored } - finalMsg = inspectToolCallResponse(method, finalMsg) + finalMsg = p.inspectToolCallResponse(method, finalMsg) data, err := jsonrpc2.EncodeMessage(finalMsg) if err != nil { @@ -784,14 +799,15 @@ func (p *HTTPProxy) writeSingleRequestSSEFinalResponse( // inspectToolCallResponse applies best-effort secret redaction (see // pkg/mcp/secretscan) to the result of a tools/call response before it is -// forwarded to the client. The MCP backend behind this proxy is not fully +// forwarded to the client, when redactToolResultSecrets is enabled (see +// WithSecretRedaction). The MCP backend behind this proxy is not fully // trusted (it may be misconfigured, compromised, or malicious), so its tool // output is scanned for credential-shaped text before the proxy relays it. // Any other method, a non-Response message, an error response, or a result // that fails to decode is returned unchanged -- this check must never be the // reason a legitimate tool call breaks. -func inspectToolCallResponse(method string, msg jsonrpc2.Message) jsonrpc2.Message { - if method != string(sdkmcp.MethodToolsCall) { +func (p *HTTPProxy) inspectToolCallResponse(method string, msg jsonrpc2.Message) jsonrpc2.Message { + if !p.redactToolResultSecrets || method != string(sdkmcp.MethodToolsCall) { return msg } resp, ok := msg.(*jsonrpc2.Response) diff --git a/pkg/transport/stdio.go b/pkg/transport/stdio.go index 4a8c9b262c..68bcd1dad8 100644 --- a/pkg/transport/stdio.go +++ b/pkg/transport/stdio.go @@ -73,6 +73,12 @@ type StdioTransport struct { // See streamable.WithStrictProtocolValidation. strictProtocolValidation bool + // redactToolResultSecrets controls whether the streamable HTTP proxy + // scans tools/call responses for credential-shaped content and redacts + // matches before relaying them to the client. Default false. See + // streamable.WithSecretRedaction. + redactToolResultSecrets bool + // Mutex for protecting shared state mutex sync.Mutex @@ -152,6 +158,14 @@ func (t *StdioTransport) SetStrictProtocolValidation(strict bool) { t.strictProtocolValidation = strict } +// SetSecretRedaction configures whether the streamable HTTP proxy scans +// tools/call responses for credential-shaped content and redacts matches +// before relaying them to the client. Default false: enable when the +// backend MCP server is not fully trusted. +func (t *StdioTransport) SetSecretRedaction(enabled bool) { + t.redactToolResultSecrets = enabled +} + // SetSessionStorage configures a custom session storage backend. // When set, the underlying proxy will use this storage instead of the default // in-memory store, enabling session sharing across replicas (e.g. Redis-backed). @@ -278,6 +292,7 @@ func (t *StdioTransport) streamableProxyOptions() []streamable.Option { streamable.WithAuthInfoHandler(t.authInfoHandler), streamable.WithPrefixHandlers(t.prefixHandlers), streamable.WithStrictProtocolValidation(t.strictProtocolValidation), + streamable.WithSecretRedaction(t.redactToolResultSecrets), ) } diff --git a/pkg/transport/types/transport.go b/pkg/transport/types/transport.go index f94254384a..104e756d91 100644 --- a/pkg/transport/types/transport.go +++ b/pkg/transport/types/transport.go @@ -255,6 +255,15 @@ type Config struct { // consulted for the stdio/streamable-HTTP proxy path (see factory.go). StrictProtocolValidation bool + // RedactToolResultSecrets enables best-effort credential-shape scanning + // (see pkg/mcp/secretscan) on tools/call responses relayed by the + // streamable HTTP proxy: matches are redacted before the response + // reaches the client. Opt-in (default false) because the backend MCP + // server is often operator-trusted and the scan adds per-response + // overhead; enable it when the backend is not fully trusted. Only + // consulted for the stdio/streamable-HTTP proxy path (see factory.go). + RedactToolResultSecrets bool + // ProxyMode is the proxy mode for stdio transport ("sse" or "streamable-http") ProxyMode ProxyMode diff --git a/pkg/workloads/upgrade/applier.go b/pkg/workloads/upgrade/applier.go index a60b9d10d7..d1346ec435 100644 --- a/pkg/workloads/upgrade/applier.go +++ b/pkg/workloads/upgrade/applier.go @@ -308,6 +308,7 @@ func (a *Applier) buildUpgradedConfig( runner.WithAllowDockerGateway(old.AllowDockerGateway), runner.WithTrustProxyHeaders(old.TrustProxyHeaders), runner.WithStrictProtocolValidation(old.StrictProtocolValidation), + runner.WithRedactToolResultSecrets(old.RedactToolResultSecrets), runner.WithProxyMode(old.ProxyMode), runner.WithCmdArgs(slices.Clone(old.CmdArgs)), runner.WithStateless(old.Stateless), From a1bf766fe5d4ed9fbb29cc1f86bbcd5693d8a07e Mon Sep 17 00:00:00 2001 From: Reynier Ortiz Vega Date: Fri, 21 Aug 2026 11:23:26 -0400 Subject: [PATCH 03/10] feat(operator): expose RedactToolResultSecrets on the MCPServer CRD The opt-in tool-result secret redaction added in the prior commits had no way to be turned on for a Kubernetes-managed MCPServer: the operator builds RunConfig directly via runner.WithXxx(...) calls (cmd/thv-operator/controllers/mcpserver_runconfig.go), it doesn't shell out `thv run` flags, so only CRD fields the controller explicitly reads ever reach the proxy pod. Add MCPServerSpec.RedactToolResultSecrets (default false), wire it to runner.WithRedactToolResultSecrets in the same place TrustProxyHeaders is wired, and regenerate the CRD manifests (task operator-generate / operator-manifests -- only the mcpservers CRD changed, as expected for a plain bool field with no deepcopy code of its own). Only takes effect when Transport is "stdio" (the streamable-HTTP proxy path, ProxyMode) -- documented on the field. Transport "streamable-http" or "sse" reverse-proxies to an already-HTTP backend via the transparent proxy, which this scanning doesn't cover yet (tracked as follow-up work, same as MCPRemoteProxy). --- .../api/v1beta1/mcpserver_types.go | 12 ++++++ .../controllers/mcpserver_runconfig.go | 1 + .../controllers/mcpserver_runconfig_test.go | 39 +++++++++++++++++++ .../toolhive.stacklok.dev_mcpservers.yaml | 24 ++++++++++++ .../toolhive.stacklok.dev_mcpservers.yaml | 24 ++++++++++++ 5 files changed, 100 insertions(+) diff --git a/cmd/thv-operator/api/v1beta1/mcpserver_types.go b/cmd/thv-operator/api/v1beta1/mcpserver_types.go index 95a0372b14..1b32df29f5 100644 --- a/cmd/thv-operator/api/v1beta1/mcpserver_types.go +++ b/cmd/thv-operator/api/v1beta1/mcpserver_types.go @@ -369,6 +369,18 @@ type MCPServerSpec struct { // +optional TrustProxyHeaders bool `json:"trustProxyHeaders,omitempty"` + // RedactToolResultSecrets enables best-effort credential-shape scanning + // on tools/call responses (AWS/GitHub/Slack/Google/Stripe keys, JWTs, PEM + // private keys): matches are redacted before the response reaches the + // client. Off by default; enable when the backend MCP server is not + // fully trusted. This setting is ONLY applicable when Transport is + // "stdio" (the streamable-HTTP proxy path) -- it has no effect when + // Transport is "streamable-http" or "sse" (those reverse-proxy to an + // already-HTTP backend without inspecting message content). + // +kubebuilder:default=false + // +optional + RedactToolResultSecrets bool `json:"redactToolResultSecrets,omitempty"` + // EndpointPrefix is the path prefix to prepend to SSE endpoint URLs. // This is used to handle path-based ingress routing scenarios where the ingress // strips a path prefix before forwarding to the backend. diff --git a/cmd/thv-operator/controllers/mcpserver_runconfig.go b/cmd/thv-operator/controllers/mcpserver_runconfig.go index 373fd0e0db..32d32e7786 100644 --- a/cmd/thv-operator/controllers/mcpserver_runconfig.go +++ b/cmd/thv-operator/controllers/mcpserver_runconfig.go @@ -150,6 +150,7 @@ func (r *MCPServerReconciler) createRunConfigFromMCPServer(m *mcpv1beta1.MCPServ runner.WithProxyMode(transporttypes.ProxyMode(effectiveProxyMode)), runner.WithHost(proxyHost), runner.WithTrustProxyHeaders(m.Spec.TrustProxyHeaders), + runner.WithRedactToolResultSecrets(m.Spec.RedactToolResultSecrets), runner.WithEndpointPrefix(m.Spec.EndpointPrefix), runner.WithToolsFilter(toolsFilter), runner.WithEnvVars(envVars), diff --git a/cmd/thv-operator/controllers/mcpserver_runconfig_test.go b/cmd/thv-operator/controllers/mcpserver_runconfig_test.go index d3b1f9b942..b2ed43e488 100644 --- a/cmd/thv-operator/controllers/mcpserver_runconfig_test.go +++ b/cmd/thv-operator/controllers/mcpserver_runconfig_test.go @@ -1681,3 +1681,42 @@ func TestCreateRunConfigFromMCPServer_SetsMCPServerGeneration(t *testing.T) { assert.Equal(t, int64(7), rc.MCPServerGeneration, "MCPServerGeneration should match MCPServer .metadata.generation") } + +// TestCreateRunConfigFromMCPServer_RedactToolResultSecrets verifies +// MCPServerSpec.RedactToolResultSecrets flows into RunConfig, mirroring +// TrustProxyHeaders's plumbing. +func TestCreateRunConfigFromMCPServer_RedactToolResultSecrets(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + enabled bool + }{ + {name: "enabled", enabled: true}, + {name: "disabled (default)", enabled: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + m := v1beta1test.NewMCPServer("redact-secrets-server", "default", + v1beta1test.WithImage("ghcr.io/example/mcp:v1"), + v1beta1test.Mutate(func(m *mcpv1beta1.MCPServer) { + m.Spec.RedactToolResultSecrets = tt.enabled + })) + + r := newTestMCPServerReconciler( + fake.NewClientBuilder().WithScheme(testutil.NewScheme(t)).WithObjects(m).Build(), + testutil.NewScheme(t), + kubernetes.PlatformKubernetes, + ) + + rc, err := r.createRunConfigFromMCPServer(m) + + require.NoError(t, err) + require.NotNil(t, rc) + assert.Equal(t, tt.enabled, rc.RedactToolResultSecrets) + }) + } +} diff --git a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpservers.yaml b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpservers.yaml index b2597e7f10..5694ef8cc6 100644 --- a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpservers.yaml +++ b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpservers.yaml @@ -515,6 +515,18 @@ spec: - message: at least one of shared, perUser, or tools must be configured rule: has(self.shared) || has(self.perUser) || (has(self.tools) && size(self.tools) > 0) + redactToolResultSecrets: + default: false + description: |- + RedactToolResultSecrets enables best-effort credential-shape scanning + on tools/call responses (AWS/GitHub/Slack/Google/Stripe keys, JWTs, PEM + private keys): matches are redacted before the response reaches the + client. Off by default; enable when the backend MCP server is not + fully trusted. This setting is ONLY applicable when Transport is + "stdio" (the streamable-HTTP proxy path) -- it has no effect when + Transport is "streamable-http" or "sse" (those reverse-proxy to an + already-HTTP backend without inspecting message content). + type: boolean replicas: description: |- Replicas is the desired number of proxy runner (thv run) pod replicas. @@ -1459,6 +1471,18 @@ spec: - message: at least one of shared, perUser, or tools must be configured rule: has(self.shared) || has(self.perUser) || (has(self.tools) && size(self.tools) > 0) + redactToolResultSecrets: + default: false + description: |- + RedactToolResultSecrets enables best-effort credential-shape scanning + on tools/call responses (AWS/GitHub/Slack/Google/Stripe keys, JWTs, PEM + private keys): matches are redacted before the response reaches the + client. Off by default; enable when the backend MCP server is not + fully trusted. This setting is ONLY applicable when Transport is + "stdio" (the streamable-HTTP proxy path) -- it has no effect when + Transport is "streamable-http" or "sse" (those reverse-proxy to an + already-HTTP backend without inspecting message content). + type: boolean replicas: description: |- Replicas is the desired number of proxy runner (thv run) pod replicas. diff --git a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpservers.yaml b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpservers.yaml index cc26407b74..08959a6ebe 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpservers.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpservers.yaml @@ -518,6 +518,18 @@ spec: - message: at least one of shared, perUser, or tools must be configured rule: has(self.shared) || has(self.perUser) || (has(self.tools) && size(self.tools) > 0) + redactToolResultSecrets: + default: false + description: |- + RedactToolResultSecrets enables best-effort credential-shape scanning + on tools/call responses (AWS/GitHub/Slack/Google/Stripe keys, JWTs, PEM + private keys): matches are redacted before the response reaches the + client. Off by default; enable when the backend MCP server is not + fully trusted. This setting is ONLY applicable when Transport is + "stdio" (the streamable-HTTP proxy path) -- it has no effect when + Transport is "streamable-http" or "sse" (those reverse-proxy to an + already-HTTP backend without inspecting message content). + type: boolean replicas: description: |- Replicas is the desired number of proxy runner (thv run) pod replicas. @@ -1462,6 +1474,18 @@ spec: - message: at least one of shared, perUser, or tools must be configured rule: has(self.shared) || has(self.perUser) || (has(self.tools) && size(self.tools) > 0) + redactToolResultSecrets: + default: false + description: |- + RedactToolResultSecrets enables best-effort credential-shape scanning + on tools/call responses (AWS/GitHub/Slack/Google/Stripe keys, JWTs, PEM + private keys): matches are redacted before the response reaches the + client. Off by default; enable when the backend MCP server is not + fully trusted. This setting is ONLY applicable when Transport is + "stdio" (the streamable-HTTP proxy path) -- it has no effect when + Transport is "streamable-http" or "sse" (those reverse-proxy to an + already-HTTP backend without inspecting message content). + type: boolean replicas: description: |- Replicas is the desired number of proxy runner (thv run) pod replicas. From 2f202a5d5daf7dad2db9d2dfff0d428795f351e2 Mon Sep 17 00:00:00 2001 From: Reynier Ortiz Vega Date: Fri, 21 Aug 2026 12:27:13 -0400 Subject: [PATCH 04/10] feat(transparent-proxy): scan and redact secrets in remote-backend responses Extends the opt-in secret redaction to the transparent (reverse-proxy) code path used whenever thv fronts an already-HTTP-speaking backend instead of a container spawned over stdio: `thv run `, MCPRemoteProxy, and any MCPServer configured with Transport streamable-http/sse. This is the path the DAST harness actually exercises (dast/mcp-proxy/harness_test.go's TestAdversarialContainment runs `thv run --stateless `, which always resolves to this code path since it never sets --transport), so it's what the checkCredentialExfiltration "known gap" assertion is really testing -- the streamable-proxy fix in the prior commits does not cover it. Adds SecretRedactionResponseProcessor, wired into createResponseProcessor for the streamable-http transport type (previously always NoOp) and composed into the existing SSEResponseProcessor's per-line handling for the legacy sse transport type. Both content shapes a streamable-HTTP response can take are covered: a single application/json body, and a text/event-stream response streamed line-by-line (never fully buffered, so a long-lived stream is not blocked). A non-streaming body is capped at 8MB (matching bodylimit.DefaultMaxRequestBodySize) and forwarded unscanned if larger, rather than risking unbounded memory growth on a hostile upstream. Reuses pkg/mcp/secretscan and the same opt-in RunConfig.RedactToolResultSecrets / --redact-tool-result-secrets / MCPServerSpec.RedactToolResultSecrets plumbing added for the streamable-proxy fix -- factory.go now sets it on HTTPTransport too, so no new flag is needed; enabling it on an existing MCPServer or `thv run` invocation now covers both proxy shapes. Also broadens pkg/mcp/secretscan's patterns with a generic "Authorization: Bearer " match. This was necessary to make the DAST harness's own CredentialExfiltration scenario detectable: its sentinel value is an arbitrary marker string with no real credential shape (no AWS/GitHub/JWT format), so the existing shape-specific patterns didn't match it. The bearer-token shape is independently a reasonable generic credential pattern to cover, not merely a test-fixture accommodation. Follow-up needed in stacklok-enterprise-platform (not this repo) once this lands and the toolhive submodule is bumped: dast/mcp-proxy's thvRunStateless call for the adversarial suite needs --redact-tool-result-secrets added, and checkCredentialExfiltration's assertion inverted, to actually observe this fix in CI -- the protection here is opt-in, so the DAST job's default invocation still exercises the unprotected path until it opts in. --- pkg/mcp/secretscan/secretscan.go | 6 + pkg/mcp/secretscan/secretscan_test.go | 1 + pkg/transport/factory.go | 2 + pkg/transport/http.go | 7 + .../proxy/transparent/response_processor.go | 10 +- .../secret_redaction_response_processor.go | 182 ++++++++++++++++++ ...ecret_redaction_response_processor_test.go | 154 +++++++++++++++ .../transparent/sse_response_processor.go | 30 ++- .../proxy/transparent/transparent_proxy.go | 18 ++ 9 files changed, 405 insertions(+), 5 deletions(-) create mode 100644 pkg/transport/proxy/transparent/secret_redaction_response_processor.go create mode 100644 pkg/transport/proxy/transparent/secret_redaction_response_processor_test.go diff --git a/pkg/mcp/secretscan/secretscan.go b/pkg/mcp/secretscan/secretscan.go index ab76a27d63..39bd1915cb 100644 --- a/pkg/mcp/secretscan/secretscan.go +++ b/pkg/mcp/secretscan/secretscan.go @@ -58,6 +58,12 @@ var patterns = []*regexp.Regexp{ // PEM-encoded private key blocks (RSA/EC/PKCS8/OpenSSH/generic). regexp.MustCompile(`(?s)-----BEGIN [A-Z ]*PRIVATE KEY-----.*?-----END [A-Z ]*PRIVATE KEY-----`), regexp.MustCompile(`(?s)-----BEGIN OPENSSH PRIVATE KEY-----.*?-----END OPENSSH PRIVATE KEY-----`), + // Generic "Authorization: Bearer " shape. Unlike the patterns + // above, this doesn't identify a specific issuer -- it catches any + // opaque bearer credential by the way it is carried, which is the most + // common shape for exfiltrated API/session tokens that don't match a + // named provider's format. + regexp.MustCompile(`(?i)\bBearer\s+[A-Za-z0-9\-._~+/=]{8,}\b`), } // Result reports what ScanAndRedactToolCallResult did. diff --git a/pkg/mcp/secretscan/secretscan_test.go b/pkg/mcp/secretscan/secretscan_test.go index 747450bf23..e6529b7715 100644 --- a/pkg/mcp/secretscan/secretscan_test.go +++ b/pkg/mcp/secretscan/secretscan_test.go @@ -25,6 +25,7 @@ func TestScanAndRedactToolCallResult_RedactsKnownCredentialShapes(t *testing.T) {"stripe secret key", "sk_live_" + strings.Repeat("a", 24)}, {"jwt", "ey" + strings.Repeat("A", 12) + "." + strings.Repeat("B", 12) + "." + strings.Repeat("C", 12)}, {"pem private key", "-----BEGIN " + "RSA PRIVATE KEY-----" + "\nMIIBogIBAAJ...\n" + "-----END " + "RSA PRIVATE KEY-----"}, + {"generic bearer token", "Authorization: Bearer " + strings.Repeat("x", 24)}, } for _, tc := range cases { diff --git a/pkg/transport/factory.go b/pkg/transport/factory.go index 0817563c8a..28be88923c 100644 --- a/pkg/transport/factory.go +++ b/pkg/transport/factory.go @@ -83,6 +83,7 @@ func (*Factory) Create(config types.Config, opts ...Option) (types.Transport, er ) httpTransport.sessionStorage = config.SessionStorage httpTransport.sessionTTL = config.SessionTTL + httpTransport.redactToolResultSecrets = config.RedactToolResultSecrets tr = httpTransport case types.TransportTypeStreamableHTTP: httpTransport := NewHTTPTransport( @@ -102,6 +103,7 @@ func (*Factory) Create(config types.Config, opts ...Option) (types.Transport, er ) httpTransport.sessionStorage = config.SessionStorage httpTransport.sessionTTL = config.SessionTTL + httpTransport.redactToolResultSecrets = config.RedactToolResultSecrets tr = httpTransport case types.TransportTypeInspector: // HTTP transport is not implemented yet diff --git a/pkg/transport/http.go b/pkg/transport/http.go index 7f9bb174b9..2abd89d486 100644 --- a/pkg/transport/http.go +++ b/pkg/transport/http.go @@ -62,6 +62,12 @@ type HTTPTransport struct { // stateless indicates the server is POST-only (no SSE/GET support) stateless bool + // redactToolResultSecrets controls whether the underlying transparent + // proxy scans tools/call responses for credential-shaped content and + // redacts matches before relaying them to the client. Default false. + // See transparent.WithSecretRedaction. + redactToolResultSecrets bool + // tokenSource is the OAuth token source for remote authentication tokenSource oauth2.TokenSource @@ -443,6 +449,7 @@ func (t *HTTPTransport) buildProxyOptions(remoteBasePath, remoteRawQuery string) if t.sessionStorage != nil { opts = append(opts, transparent.WithSessionStorage(t.sessionStorage)) } + opts = append(opts, transparent.WithSecretRedaction(t.redactToolResultSecrets)) return opts } diff --git a/pkg/transport/proxy/transparent/response_processor.go b/pkg/transport/proxy/transparent/response_processor.go index a6e0ca765f..cbf0afa2f5 100644 --- a/pkg/transport/proxy/transparent/response_processor.go +++ b/pkg/transport/proxy/transparent/response_processor.go @@ -37,17 +37,23 @@ func (*NoOpResponseProcessor) ShouldProcess(_ *http.Response) bool { } // createResponseProcessor is a factory function that creates the appropriate -// response processor based on transport type. +// response processor based on transport type. redactSecrets enables the +// pkg/mcp/secretscan-backed scan of tools/call results (see +// WithSecretRedaction); false preserves prior behavior exactly. func createResponseProcessor( transportType string, proxy *TransparentProxy, endpointPrefix string, trustProxyHeaders bool, + redactSecrets bool, ) ResponseProcessor { switch transportType { case types.TransportTypeSSE.String(): - return NewSSEResponseProcessor(proxy, endpointPrefix, trustProxyHeaders) + return NewSSEResponseProcessor(proxy, endpointPrefix, trustProxyHeaders, redactSecrets) case types.TransportTypeStreamableHTTP.String(): + if redactSecrets { + return NewSecretRedactionResponseProcessor() + } return &NoOpResponseProcessor{} default: // Default to no-op for unknown transport types diff --git a/pkg/transport/proxy/transparent/secret_redaction_response_processor.go b/pkg/transport/proxy/transparent/secret_redaction_response_processor.go new file mode 100644 index 0000000000..102bd1ffac --- /dev/null +++ b/pkg/transport/proxy/transparent/secret_redaction_response_processor.go @@ -0,0 +1,182 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package transparent + +import ( + "bufio" + "bytes" + "encoding/json" + "fmt" + "io" + "log/slog" + "mime" + "net/http" + "strconv" + "strings" + + "github.com/stacklok/toolhive/pkg/mcp/secretscan" +) + +// maxSecretScanBodyBytes bounds how much of a non-streaming response body +// this processor buffers to scan. A response larger than this is forwarded +// unscanned rather than risking unbounded memory growth on a hostile or +// oversized upstream body. +const maxSecretScanBodyBytes = 8 << 20 // 8 MB, matches bodylimit.DefaultMaxRequestBodySize + +// SecretRedactionResponseProcessor scans MCP responses emitted by the +// (untrusted) backend this proxy fronts for credential-shaped content in a +// tools/call result (see pkg/mcp/secretscan) and redacts matches before the +// response reaches the client. Opt-in; see WithSecretRedaction. Constructed +// only when enabled, so unlike other processors it carries no enabled flag +// of its own. +// +// Handles the two response shapes a streamable-HTTP MCP server may emit for +// a single request: a plain application/json body, or a text/event-stream +// response carrying one or more "data:" JSON-RPC frames (MCP allows a server +// to reply to a POST with either shape). +type SecretRedactionResponseProcessor struct{} + +// NewSecretRedactionResponseProcessor creates a new secret-redaction response +// processor. +func NewSecretRedactionResponseProcessor() *SecretRedactionResponseProcessor { + return &SecretRedactionResponseProcessor{} +} + +// ShouldProcess returns true for JSON and SSE response bodies -- the two +// shapes a streamable-HTTP MCP response can take. +func (*SecretRedactionResponseProcessor) ShouldProcess(resp *http.Response) bool { + mediaType, _, _ := mime.ParseMediaType(resp.Header.Get("Content-Type")) + return mediaType == "application/json" || mediaType == "text/event-stream" +} + +// ProcessResponse redacts credential-shaped content from the response body. +func (p *SecretRedactionResponseProcessor) ProcessResponse(resp *http.Response) error { + if !p.ShouldProcess(resp) { + return nil + } + mediaType, _, _ := mime.ParseMediaType(resp.Header.Get("Content-Type")) + if mediaType == "text/event-stream" { + p.processSSE(resp) + return nil + } + return p.processJSON(resp) +} + +// chainedBody re-splices a size-limited read prefix back onto the reader it +// came from, so a body too large to safely buffer is still forwarded intact +// (unscanned) rather than truncated. Close defers to the original body so +// the underlying connection is still released once the client finishes +// reading. +type chainedBody struct { + io.Reader + closer io.Closer +} + +func (c chainedBody) Close() error { return c.closer.Close() } + +// processJSON scans a complete (non-streaming) JSON response body. +func (*SecretRedactionResponseProcessor) processJSON(resp *http.Response) error { + original := resp.Body + limited := io.LimitReader(original, maxSecretScanBodyBytes+1) + data, err := io.ReadAll(limited) + if err != nil { + return fmt.Errorf("reading response body for secret scan: %w", err) + } + if int64(len(data)) > maxSecretScanBodyBytes { + slog.Debug("response body exceeds secret-scan size limit; forwarding unscanned", + "limit_bytes", maxSecretScanBodyBytes) + resp.Body = chainedBody{Reader: io.MultiReader(bytes.NewReader(data), original), closer: original} + return nil + } + if err := original.Close(); err != nil { + slog.Debug("failed to close upstream response body", "error", err) + } + + redacted, changed, scanErr := redactJSONRPCBody(data) + if scanErr != nil || !changed { + // Not a recognizable JSON-RPC envelope, or nothing matched -- forward + // the original bytes unchanged (fail open; a scan miss is not a + // proxy error). + resp.Body = io.NopCloser(bytes.NewReader(data)) + return nil + } + resp.Body = io.NopCloser(bytes.NewReader(redacted)) + resp.ContentLength = int64(len(redacted)) + resp.Header.Set("Content-Length", strconv.Itoa(len(redacted))) + return nil +} + +// processSSE streams an SSE response line by line, redacting each "data:" +// frame's JSON-RPC payload as it is forwarded, so a long-lived stream is +// never fully buffered. +func (*SecretRedactionResponseProcessor) processSSE(resp *http.Response) { + original := resp.Body + pr, pw := io.Pipe() + resp.Body = pr + + go func() { + defer func() { + if err := pw.Close(); err != nil { + slog.Debug("failed to close pipe writer", "error", err) + } + }() + defer func() { + if err := original.Close(); err != nil { + slog.Debug("failed to close upstream response body", "error", err) + } + }() + + scanner := bufio.NewScanner(original) + // Matches sse_response_processor.go's buffer size rationale: the + // default 64KB token limit is too small for a data line carrying a + // sizeable tool result. + scanner.Buffer(make([]byte, 0, 1024), 1024*1024) + + for scanner.Scan() { + line := scanner.Text() + if after, ok := strings.CutPrefix(line, "data:"); ok { + dataContent := strings.TrimSpace(after) + if redacted, changed, err := redactJSONRPCBody([]byte(dataContent)); err == nil && changed { + line = "data: " + string(redacted) + } + } + if _, err := pw.Write([]byte(line + "\n")); err != nil { + return + } + } + if err := scanner.Err(); err != nil { + slog.Error("failed to scan SSE response body for secret scan", "error", err) + } + }() +} + +// redactJSONRPCBody decodes data as a JSON object, redacts its "result" +// field via secretscan (if present), and returns the re-encoded body. changed +// is false whenever nothing needed redacting -- including when data doesn't +// decode as a JSON-RPC response object at all (e.g. a request, a batch, or a +// malformed frame), which is reported via a non-nil err so callers can fail +// open without treating it as a real error. +func redactJSONRPCBody(data []byte) (redacted []byte, changed bool, err error) { + var envelope map[string]json.RawMessage + if err := json.Unmarshal(data, &envelope); err != nil { + return nil, false, fmt.Errorf("decoding JSON-RPC envelope: %w", err) + } + result, ok := envelope["result"] + if !ok || len(result) == 0 { + return nil, false, nil + } + scan, err := secretscan.ScanAndRedactToolCallResult(result) + if err != nil { + return nil, false, err + } + if !scan.Matched { + return nil, false, nil + } + envelope["result"] = scan.Redacted + out, err := json.Marshal(envelope) + if err != nil { + return nil, false, fmt.Errorf("re-encoding JSON-RPC envelope: %w", err) + } + return out, true, nil +} diff --git a/pkg/transport/proxy/transparent/secret_redaction_response_processor_test.go b/pkg/transport/proxy/transparent/secret_redaction_response_processor_test.go new file mode 100644 index 0000000000..b71f121fe7 --- /dev/null +++ b/pkg/transport/proxy/transparent/secret_redaction_response_processor_test.go @@ -0,0 +1,154 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package transparent + +import ( + "bufio" + "net/http" + "net/http/httptest" + "net/http/httputil" + "net/url" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const sentinelToken = "DAST-SENTINEL-TOKEN-DO-NOT-FORWARD" //nolint:gosec // test fixture, not a real credential + +func toolCallResultJSON(text string) string { + return `{"jsonrpc":"2.0","id":1,"result":{"content":[{"type":"text","text":"` + text + `"}]}}` +} + +// buildRedactingProxy wires up a *httputil.ReverseProxy fronting target using +// a TransparentProxy configured with WithSecretRedaction(true) for the given +// transport type -- mirroring the createBasicProxy/modifyResponse harness +// used by the existing tests in this package (see TestStreamingSessionIDDetection). +func buildRedactingProxy(t *testing.T, transportType string, targetURL *url.URL) *httputil.ReverseProxy { + t.Helper() + p := NewTransparentProxyWithOptions( + "127.0.0.1", 0, targetURL.String(), nil, nil, nil, + false, false, transportType, nil, nil, "", false, nil, + WithSecretRedaction(true), + ) + return &httputil.ReverseProxy{ + Rewrite: func(pr *httputil.ProxyRequest) { + pr.SetURL(targetURL) + pr.SetXForwarded() + }, + FlushInterval: -1, + Transport: newTracingTransport(http.DefaultTransport, p), + ModifyResponse: p.modifyResponse, + } +} + +func TestSecretRedaction_StreamableHTTP_JSONResponse(t *testing.T) { + t.Parallel() + + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(toolCallResultJSON("Authorization: Bearer " + sentinelToken))) + })) + defer target.Close() + targetURL, err := url.Parse(target.URL) + require.NoError(t, err) + + proxy := buildRedactingProxy(t, "streamable-http", targetURL) + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, target.URL, nil) + proxy.ServeHTTP(rec, req) + + assert.NotContains(t, rec.Body.String(), sentinelToken, + "sentinel token must not reach the client") + assert.Contains(t, rec.Body.String(), "REDACTED-BY-TOOLHIVE") +} + +func TestSecretRedaction_StreamableHTTP_SSEResponse(t *testing.T) { + t.Parallel() + + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "text/event-stream") + w.WriteHeader(http.StatusOK) + w.Write([]byte("data: " + toolCallResultJSON("Authorization: Bearer "+sentinelToken) + "\n\n")) + w.(http.Flusher).Flush() + })) + defer target.Close() + targetURL, err := url.Parse(target.URL) + require.NoError(t, err) + + proxy := buildRedactingProxy(t, "streamable-http", targetURL) + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, target.URL, nil) + proxy.ServeHTTP(rec, req) + + assert.NotContains(t, rec.Body.String(), sentinelToken, + "sentinel token must not reach the client") + assert.Contains(t, rec.Body.String(), "REDACTED-BY-TOOLHIVE") +} + +func TestSecretRedaction_LegacySSETransport_DataLine(t *testing.T) { + t.Parallel() + + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "text/event-stream") + w.WriteHeader(http.StatusOK) + w.Write([]byte("data: " + toolCallResultJSON("Authorization: Bearer "+sentinelToken) + "\n\n")) + w.(http.Flusher).Flush() + })) + defer target.Close() + targetURL, err := url.Parse(target.URL) + require.NoError(t, err) + + proxy := buildRedactingProxy(t, "sse", targetURL) + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, target.URL, nil) + proxy.ServeHTTP(rec, req) + + sc := bufio.NewScanner(rec.Body) + var found bool + for sc.Scan() { + line := sc.Text() + assert.NotContains(t, line, sentinelToken, "sentinel token must not reach the client") + if strings.Contains(line, "REDACTED-BY-TOOLHIVE") { + found = true + } + } + assert.True(t, found, "expected a redacted data line") +} + +func TestSecretRedaction_Disabled_ByDefault(t *testing.T) { + t.Parallel() + + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(toolCallResultJSON("Authorization: Bearer " + sentinelToken))) + })) + defer target.Close() + targetURL, err := url.Parse(target.URL) + require.NoError(t, err) + + // No WithSecretRedaction option -- default false preserves prior behavior. + p := NewTransparentProxy("127.0.0.1", 0, targetURL.String(), nil, nil, nil, + false, false, "streamable-http", nil, nil, "", false) + proxy := &httputil.ReverseProxy{ + Rewrite: func(pr *httputil.ProxyRequest) { + pr.SetURL(targetURL) + pr.SetXForwarded() + }, + FlushInterval: -1, + Transport: newTracingTransport(http.DefaultTransport, p), + ModifyResponse: p.modifyResponse, + } + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, target.URL, nil) + proxy.ServeHTTP(rec, req) + + assert.Contains(t, rec.Body.String(), sentinelToken, + "disabled by default: response must pass through unmodified") +} diff --git a/pkg/transport/proxy/transparent/sse_response_processor.go b/pkg/transport/proxy/transparent/sse_response_processor.go index 52d1ae6364..ec49e4dbc9 100644 --- a/pkg/transport/proxy/transparent/sse_response_processor.go +++ b/pkg/transport/proxy/transparent/sse_response_processor.go @@ -57,12 +57,15 @@ func (c sseRewriteConfig) hasRewriteConfig() bool { var sessionRe = regexp.MustCompile(`sessionId=([0-9A-Fa-f-]+)|"sessionId"\s*:\s*"([^"]+)"`) // SSEResponseProcessor handles SSE-specific response processing including: -// - Session ID extraction from SSE streams -// - Endpoint URL rewriting for path-based routing +// - Session ID extraction from SSE streams +// - Endpoint URL rewriting for path-based routing +// - Optional credential-shape redaction of tools/call results (see +// WithSecretRedaction) type SSEResponseProcessor struct { proxy *TransparentProxy endpointPrefix string trustProxyHeaders bool + redactSecrets bool } // NewSSEResponseProcessor creates a new SSE response processor. @@ -70,11 +73,13 @@ func NewSSEResponseProcessor( proxy *TransparentProxy, endpointPrefix string, trustProxyHeaders bool, + redactSecrets bool, ) *SSEResponseProcessor { return &SSEResponseProcessor{ proxy: proxy, endpointPrefix: endpointPrefix, trustProxyHeaders: trustProxyHeaders, + redactSecrets: redactSecrets, } } @@ -212,6 +217,7 @@ type sseLineProcessor struct { rewriteConfig sseRewriteConfig currentEventType string sessionFound bool + redactSecrets bool } // processLine processes a single SSE line and returns the potentially modified line. @@ -245,12 +251,29 @@ func (s *sseLineProcessor) processDataLine(line string) string { // Rewrite endpoint URLs only for "endpoint" events if s.currentEventType == "endpoint" && s.rewriteConfig.hasRewriteConfig() { - return s.rewriteDataLine(line, dataContent) + line = s.rewriteDataLine(line, dataContent) + } + + if s.redactSecrets { + line = s.redactDataLine(line) } return line } +// redactDataLine redacts credential-shaped content from a data line's +// JSON-RPC payload (see pkg/mcp/secretscan). Lines that aren't a JSON-RPC +// response object (endpoint events, notifications with no "result", any +// content that fails to decode) are returned unchanged. +func (*sseLineProcessor) redactDataLine(line string) string { + dataContent := strings.TrimSpace(strings.TrimPrefix(line, "data:")) + redacted, changed, err := redactJSONRPCBody([]byte(dataContent)) + if err != nil || !changed { + return line + } + return "data: " + string(redacted) +} + // extractSessionID extracts and stores the session ID from a data line. func (s *sseLineProcessor) extractSessionID(line string) { if s.sessionFound { @@ -299,6 +322,7 @@ func (s *SSEResponseProcessor) processSSEStream(originalBody io.Reader, pw *io.P processor := &sseLineProcessor{ proxy: s.proxy, rewriteConfig: rewriteConfig, + redactSecrets: s.redactSecrets, } for scanner.Scan() { diff --git a/pkg/transport/proxy/transparent/transparent_proxy.go b/pkg/transport/proxy/transparent/transparent_proxy.go index cb8d6d12b2..ee2cb91009 100644 --- a/pkg/transport/proxy/transparent/transparent_proxy.go +++ b/pkg/transport/proxy/transparent/transparent_proxy.go @@ -108,6 +108,12 @@ type TransparentProxy struct { // stateless indicates the server is POST-only (no SSE/GET support) stateless bool + // redactToolResultSecrets enables scanning tools/call responses for + // credential-shaped content and redacting matches before relaying them + // to the client (see pkg/mcp/secretscan). Default false. Set via + // WithSecretRedaction. + redactToolResultSecrets bool + // Callback when health check fails (for remote servers) onHealthCheckFailed types.HealthCheckFailedCallback @@ -269,6 +275,17 @@ func WithStateless() Option { } } +// WithSecretRedaction enables best-effort credential-shape scanning (see +// pkg/mcp/secretscan) on tools/call responses before they are relayed to the +// client. Opt-in (default false): the MCP backend behind this proxy is often +// operator-trusted, and scanning adds per-response overhead, so this is only +// worth enabling when the backend is not fully trusted. +func WithSecretRedaction(enabled bool) Option { + return func(p *TransparentProxy) { + p.redactToolResultSecrets = enabled + } +} + // withHealthCheckPingTimeout sets the health check ping timeout. // This is primarily useful for testing with shorter timeouts. // Ignores non-positive timeouts; default will be used. @@ -497,6 +514,7 @@ func NewTransparentProxyWithOptions( proxy, endpointPrefix, trustProxyHeaders, + proxy.redactToolResultSecrets, ) // Create health checker always for Kubernetes probes From c1345dcde9deae9adb9d115f8d074a000172120f Mon Sep 17 00:00:00 2001 From: Reynier Ortiz Vega Date: Fri, 21 Aug 2026 12:28:37 -0400 Subject: [PATCH 05/10] test(transparent-proxy): drop unused param flagged by unparam lint --- .../secret_redaction_response_processor_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/transport/proxy/transparent/secret_redaction_response_processor_test.go b/pkg/transport/proxy/transparent/secret_redaction_response_processor_test.go index b71f121fe7..6d43498699 100644 --- a/pkg/transport/proxy/transparent/secret_redaction_response_processor_test.go +++ b/pkg/transport/proxy/transparent/secret_redaction_response_processor_test.go @@ -18,8 +18,8 @@ import ( const sentinelToken = "DAST-SENTINEL-TOKEN-DO-NOT-FORWARD" //nolint:gosec // test fixture, not a real credential -func toolCallResultJSON(text string) string { - return `{"jsonrpc":"2.0","id":1,"result":{"content":[{"type":"text","text":"` + text + `"}]}}` +func toolCallResultJSON() string { + return `{"jsonrpc":"2.0","id":1,"result":{"content":[{"type":"text","text":"Authorization: Bearer ` + sentinelToken + `"}]}}` } // buildRedactingProxy wires up a *httputil.ReverseProxy fronting target using @@ -49,7 +49,7 @@ func TestSecretRedaction_StreamableHTTP_JSONResponse(t *testing.T) { target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") - w.Write([]byte(toolCallResultJSON("Authorization: Bearer " + sentinelToken))) + w.Write([]byte(toolCallResultJSON())) })) defer target.Close() targetURL, err := url.Parse(target.URL) @@ -72,7 +72,7 @@ func TestSecretRedaction_StreamableHTTP_SSEResponse(t *testing.T) { target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "text/event-stream") w.WriteHeader(http.StatusOK) - w.Write([]byte("data: " + toolCallResultJSON("Authorization: Bearer "+sentinelToken) + "\n\n")) + w.Write([]byte("data: " + toolCallResultJSON() + "\n\n")) w.(http.Flusher).Flush() })) defer target.Close() @@ -96,7 +96,7 @@ func TestSecretRedaction_LegacySSETransport_DataLine(t *testing.T) { target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "text/event-stream") w.WriteHeader(http.StatusOK) - w.Write([]byte("data: " + toolCallResultJSON("Authorization: Bearer "+sentinelToken) + "\n\n")) + w.Write([]byte("data: " + toolCallResultJSON() + "\n\n")) w.(http.Flusher).Flush() })) defer target.Close() @@ -126,7 +126,7 @@ func TestSecretRedaction_Disabled_ByDefault(t *testing.T) { target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") - w.Write([]byte(toolCallResultJSON("Authorization: Bearer " + sentinelToken))) + w.Write([]byte(toolCallResultJSON())) })) defer target.Close() targetURL, err := url.Parse(target.URL) From 92b2a944de62976c7029e1cb505210e0baa5984c Mon Sep 17 00:00:00 2001 From: Reynier Ortiz Vega Date: Fri, 21 Aug 2026 12:59:42 -0400 Subject: [PATCH 06/10] feat(vmcp/server): scan and redact secrets in tools/call results Extends the opt-in secret redaction to pkg/vmcp/server, the code path used by embedders that serve MCP directly on top of the embedded core.VMCP -- e.g. connector-gateway (enterprise), which builds *vmcpserver.Server via Serve() and never touches either of ToolHive's own proxy packages (pkg/transport/proxy/streamable or transparent). Those proxies were fixed in the prior commits; this one covers the third, structurally separate path within toolhive itself. Adds Config.RedactToolResultSecrets (mirrored on ServerConfig, threaded through deriveServerConfig/buildServeConfig same as the other cross-cutting fields), and wires it into both tools/call result-building sites: - serve_handlers.go's coreToolHandler (Legacy/SDK dispatch) - modern_envelope.go's newModernCallToolResult, called from modern_dispatch.go's dispatchModernToolCall (Modern/stateless dispatch) Both call conversion.ToMCPContents(result.Content) to build the wire Content slice from the core's domain result; redaction runs on that slice via the new exported pkg/mcp/secretscan.RedactContentInPlace, which ScanAndRedactToolCallResult (used by the proxy fixes) now also calls internally, so the credential-shape patterns are defined once. Also threads it through to the OSS `vmcp serve` CLI/YAML config path (pkg/vmcp/config.Config.RedactToolResultSecrets -> pkg/vmcp/cli/serve.go), so the standalone vmcp binary can turn it on. No deepcopy regen needed (plain bool, confirmed via controller-gen -- zero diff). Not covered by this commit (follow-ups, tracked in the PR description): - VirtualMCPServer CRD (thv-operator) does not yet expose this field; same shape as the MCPServer CRD wiring, needs its own converter change. - connector-gateway's own composition root (enterprise/connector-gateway, a different repo) builds *vmcpserver.ServerConfig directly and needs a one-line addition (RedactToolResultSecrets: true) once this merges and the toolhive dependency is bumped. - The optimizer/code-mode virtual tool path (serve_optimizer.go's execute_tool_script) builds its own CallToolResult independently and isn't covered. --- pkg/mcp/secretscan/secretscan.go | 40 +++++++++----- pkg/vmcp/cli/serve.go | 1 + pkg/vmcp/config/config.go | 8 +++ pkg/vmcp/server/derive.go | 5 +- pkg/vmcp/server/derive_test.go | 1 + pkg/vmcp/server/modern_dispatch.go | 3 +- pkg/vmcp/server/modern_envelope.go | 14 ++++- pkg/vmcp/server/modern_envelope_test.go | 10 ++-- pkg/vmcp/server/secretscan_test.go | 73 +++++++++++++++++++++++++ pkg/vmcp/server/serve.go | 6 ++ pkg/vmcp/server/serve_handlers.go | 8 ++- pkg/vmcp/server/serve_session_test.go | 16 ++++-- pkg/vmcp/server/serve_test.go | 1 + pkg/vmcp/server/server.go | 10 ++++ 14 files changed, 164 insertions(+), 32 deletions(-) create mode 100644 pkg/vmcp/server/secretscan_test.go diff --git a/pkg/mcp/secretscan/secretscan.go b/pkg/mcp/secretscan/secretscan.go index 39bd1915cb..88efe71c8c 100644 --- a/pkg/mcp/secretscan/secretscan.go +++ b/pkg/mcp/secretscan/secretscan.go @@ -93,8 +93,31 @@ func ScanAndRedactToolCallResult(raw json.RawMessage) (Result, error) { return Result{Redacted: raw}, fmt.Errorf("decoding tool call result: %w", err) } + if !RedactContentInPlace(result.Content) { + return Result{Redacted: raw}, nil + } + + encoded, err := json.Marshal(result) + if err != nil { + // Should not happen -- result round-tripped through the same type's + // (Un)MarshalJSON -- but fail open rather than block the response. + return Result{Redacted: raw}, fmt.Errorf("re-encoding redacted tool call result: %w", err) + } + return Result{Redacted: encoded, Matched: true}, nil +} + +// RedactContentInPlace scans content for credential-shaped text (see the +// package doc) and redacts matches in place, entry by entry. Returns true if +// anything matched. Non-text content (images, audio, embedded resources) is +// left untouched -- this package does not decode binary/base64 payloads. +// +// Exported so callers that already hold a decoded []sdkmcp.Content -- e.g. +// pkg/vmcp/server, which builds a CallToolResult's Content from the core's +// domain result without ever serializing to JSON -- can redact without a +// round trip through ScanAndRedactToolCallResult's JSON (de)serialization. +func RedactContentInPlace(content []sdkmcp.Content) bool { matched := false - for i, c := range result.Content { + for i, c := range content { text, ok := c.(sdkmcp.TextContent) if !ok { continue @@ -105,20 +128,9 @@ func ScanAndRedactToolCallResult(raw json.RawMessage) (Result, error) { } matched = true text.Text = redactedText - result.Content[i] = text - } - - if !matched { - return Result{Redacted: raw}, nil - } - - encoded, err := json.Marshal(result) - if err != nil { - // Should not happen -- result round-tripped through the same type's - // (Un)MarshalJSON -- but fail open rather than block the response. - return Result{Redacted: raw}, fmt.Errorf("re-encoding redacted tool call result: %w", err) + content[i] = text } - return Result{Redacted: encoded, Matched: true}, nil + return matched } // redactText replaces every pattern match in s with redactionPlaceholder. diff --git a/pkg/vmcp/cli/serve.go b/pkg/vmcp/cli/serve.go index 0f7efa9bcb..216eae21fe 100644 --- a/pkg/vmcp/cli/serve.go +++ b/pkg/vmcp/cli/serve.go @@ -440,6 +440,7 @@ func Serve(ctx context.Context, cfg ServeConfig) error { CodeModeConfig: codemode.FromConfig(vmcpCfg.CodeMode), SessionFactory: sessionFactory, SessionStorage: vmcpCfg.SessionStorage, + RedactToolResultSecrets: vmcpCfg.RedactToolResultSecrets, // Core collaborators: server.New routes through core.New + Serve, so the core // is the single aggregator and authorizer. The aggregator is the same instance // that backs discovery; Authz feeds the core admission seam (nil = allow-all). diff --git a/pkg/vmcp/config/config.go b/pkg/vmcp/config/config.go index 6712a2c87f..7c0a2c3f08 100644 --- a/pkg/vmcp/config/config.go +++ b/pkg/vmcp/config/config.go @@ -201,6 +201,14 @@ type Config struct { // +optional // +listType=atomic PassthroughHeaders []string `json:"passthroughHeaders,omitempty" yaml:"passthroughHeaders,omitempty"` + + // RedactToolResultSecrets enables best-effort credential-shape scanning + // (see pkg/mcp/secretscan) on tools/call results before they are relayed + // to the client: matches are redacted in place. Opt-in (default false); + // enable when a backend this vMCP aggregates is not fully trusted. + // +optional + // +kubebuilder:default=false + RedactToolResultSecrets bool `json:"redactToolResultSecrets,omitempty" yaml:"redactToolResultSecrets,omitempty"` } // IncomingAuthConfig configures client authentication to the virtual MCP server. diff --git a/pkg/vmcp/server/derive.go b/pkg/vmcp/server/derive.go index a1799babc7..c177a9041b 100644 --- a/pkg/vmcp/server/derive.go +++ b/pkg/vmcp/server/derive.go @@ -88,8 +88,9 @@ func deriveServerConfig( SessionStorage: cfg.SessionStorage, SessionManagerConfig: sessionManagerConfig, // Cross-cutting (also on core.Config) — R3, not a clean partition: - TelemetryProvider: cfg.TelemetryProvider, - AuditConfig: cfg.AuditConfig, + TelemetryProvider: cfg.TelemetryProvider, + AuditConfig: cfg.AuditConfig, + RedactToolResultSecrets: cfg.RedactToolResultSecrets, } } diff --git a/pkg/vmcp/server/derive_test.go b/pkg/vmcp/server/derive_test.go index 645d8cddbc..c654866668 100644 --- a/pkg/vmcp/server/derive_test.go +++ b/pkg/vmcp/server/derive_test.go @@ -51,6 +51,7 @@ func populatedLegacyConfig() *Config { Watcher: stubWatcher{}, StatusReporter: stubServeReporter{}, SessionStorage: &vmcpconfig.SessionStorageConfig{}, + RedactToolResultSecrets: true, } } diff --git a/pkg/vmcp/server/modern_dispatch.go b/pkg/vmcp/server/modern_dispatch.go index e7d79d4dc1..79b6e1bb7a 100644 --- a/pkg/vmcp/server/modern_dispatch.go +++ b/pkg/vmcp/server/modern_dispatch.go @@ -339,7 +339,8 @@ func (s *Server) dispatchModernToolCall( bi.BackendName = s.backendDisplayName(ctx, result.BackendID) } } - writeModernResult(w, parsed.ID, newModernCallToolResult(result, s.config.Name, s.config.Version)) + writeModernResult(w, parsed.ID, + newModernCallToolResult(result, s.config.Name, s.config.Version, s.config.RedactToolResultSecrets)) } // dispatchModernResourceRead re-homes authzCallGate's resources/read branch diff --git a/pkg/vmcp/server/modern_envelope.go b/pkg/vmcp/server/modern_envelope.go index 0da4e60260..a63aab3be5 100644 --- a/pkg/vmcp/server/modern_envelope.go +++ b/pkg/vmcp/server/modern_envelope.go @@ -11,6 +11,7 @@ import ( "github.com/stacklok/toolhive-core/mcpcompat/mcp" mcpparser "github.com/stacklok/toolhive/pkg/mcp" + "github.com/stacklok/toolhive/pkg/mcp/secretscan" transportsession "github.com/stacklok/toolhive/pkg/transport/session" "github.com/stacklok/toolhive/pkg/vmcp" "github.com/stacklok/toolhive/pkg/vmcp/conversion" @@ -411,19 +412,26 @@ func newModernPromptsList( // newModernCallToolResult builds the tools/call wire result from the core's // ToolCallResult. StructuredContent is omitted entirely (not merely // omitempty-false) when the core did not set it, matching the SDK's -// omitempty behavior. +// omitempty behavior. When redactSecrets is set, Content is scanned for +// credential-shaped text (see pkg/mcp/secretscan) before being returned. // // result is non-nil on every call: dispatchModernToolCall (modern_dispatch.go) // dereferences result.BackendID before calling this builder, so a nil result // would already have panicked upstream. -func newModernCallToolResult(result *vmcp.ToolCallResult, serverName, serverVersion string) modernCallToolResult { +func newModernCallToolResult( + result *vmcp.ToolCallResult, serverName, serverVersion string, redactSecrets bool, +) modernCallToolResult { var structuredContent any if len(result.StructuredContent) > 0 { structuredContent = result.StructuredContent } + content := conversion.ToMCPContents(result.Content) + if redactSecrets { + secretscan.RedactContentInPlace(content) + } return modernCallToolResult{ ResultType: modernResultTypeComplete, - Content: conversion.ToMCPContents(result.Content), + Content: content, StructuredContent: structuredContent, IsError: result.IsError, Meta: newModernResultMeta(result.Meta, serverName, serverVersion), diff --git a/pkg/vmcp/server/modern_envelope_test.go b/pkg/vmcp/server/modern_envelope_test.go index c63c3abc6f..8687507271 100644 --- a/pkg/vmcp/server/modern_envelope_test.go +++ b/pkg/vmcp/server/modern_envelope_test.go @@ -81,7 +81,7 @@ func TestModernEnvelopeCommonFields(t *testing.T) { build: func(*testing.T) any { return newModernCallToolResult(&vmcp.ToolCallResult{ Content: []vmcp.Content{{Type: vmcp.ContentTypeText, Text: "hello"}}, - }, testServerName, testServerVersion) + }, testServerName, testServerVersion, false) }, wantCacheable: false, }, @@ -156,7 +156,7 @@ func TestModernResultMetaPreservesBackendMeta(t *testing.T) { { name: "tools/call", build: func() any { - return newModernCallToolResult(&vmcp.ToolCallResult{Meta: backendMeta}, testServerName, testServerVersion) + return newModernCallToolResult(&vmcp.ToolCallResult{Meta: backendMeta}, testServerName, testServerVersion, false) }, }, { @@ -212,7 +212,7 @@ func TestModernResultMetaOverwritesSpoofedServerInfo(t *testing.T) { modernServerInfoKey: map[string]any{"name": "attacker-server", "version": "666"}, } - raw, err := json.Marshal(newModernCallToolResult(&vmcp.ToolCallResult{Meta: spoofed}, testServerName, testServerVersion)) + raw, err := json.Marshal(newModernCallToolResult(&vmcp.ToolCallResult{Meta: spoofed}, testServerName, testServerVersion, false)) require.NoError(t, err) var decoded map[string]any @@ -354,7 +354,7 @@ func TestModernEnvelopeEmptyCollections(t *testing.T) { name: "tools/call content", field: "content", build: func(*testing.T) any { - return newModernCallToolResult(&vmcp.ToolCallResult{}, testServerName, testServerVersion) + return newModernCallToolResult(&vmcp.ToolCallResult{}, testServerName, testServerVersion, false) }, }, { @@ -422,7 +422,7 @@ func TestModernCallToolResult(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - raw, err := json.Marshal(newModernCallToolResult(tt.domainResult, testServerName, testServerVersion)) + raw, err := json.Marshal(newModernCallToolResult(tt.domainResult, testServerName, testServerVersion, false)) require.NoError(t, err) var decoded map[string]any diff --git a/pkg/vmcp/server/secretscan_test.go b/pkg/vmcp/server/secretscan_test.go new file mode 100644 index 0000000000..a4555e5b67 --- /dev/null +++ b/pkg/vmcp/server/secretscan_test.go @@ -0,0 +1,73 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package server + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive-core/mcpcompat/mcp" + "github.com/stacklok/toolhive/pkg/vmcp" +) + +// TestCoreToolHandler_RedactsCredentialShapedToolResult verifies the Legacy +// (SDK) tools/call path scans and redacts credential-shaped content in the +// core's result when Config.RedactToolResultSecrets is enabled -- the same +// gap connector-gateway (which serves through this Server, not ToolHive's +// own proxies) would otherwise have. +func TestCoreToolHandler_RedactsCredentialShapedToolResult(t *testing.T) { + t.Parallel() + + const toolName = "t" + ghToken := "ghp_" + strings.Repeat("a", 36) + fc := &fakeCore{ + tools: []vmcp.Tool{{Name: toolName}}, + callResult: &vmcp.ToolCallResult{ + Content: []vmcp.Content{{Type: vmcp.ContentTypeText, Text: "here is the token: " + ghToken}}, + }, + } + srv, sessionID, _ := registerServeSession(t, fc) + srv.config.RedactToolResultSecrets = true + + req := mcp.CallToolRequest{Params: mcp.CallToolParams{Name: toolName, Arguments: map[string]any{}}} + res, err := srv.coreToolHandler(sessionID, toolName, "")(t.Context(), req) + + require.NoError(t, err) + require.NotNil(t, res) + require.Len(t, res.Content, 1) + text, ok := res.Content[0].(mcp.TextContent) + require.True(t, ok) + assert.NotContains(t, text.Text, ghToken) + assert.Contains(t, text.Text, "REDACTED-BY-TOOLHIVE") +} + +// TestCoreToolHandler_DisabledByDefault verifies redaction is opt-in: with +// Config.RedactToolResultSecrets left at its zero value, credential-shaped +// content passes through unchanged. +func TestCoreToolHandler_DisabledByDefault(t *testing.T) { + t.Parallel() + + const toolName = "t" + ghToken := "ghp_" + strings.Repeat("a", 36) + fc := &fakeCore{ + tools: []vmcp.Tool{{Name: toolName}}, + callResult: &vmcp.ToolCallResult{ + Content: []vmcp.Content{{Type: vmcp.ContentTypeText, Text: ghToken}}, + }, + } + srv, sessionID, _ := registerServeSession(t, fc) + + req := mcp.CallToolRequest{Params: mcp.CallToolParams{Name: toolName, Arguments: map[string]any{}}} + res, err := srv.coreToolHandler(sessionID, toolName, "")(t.Context(), req) + + require.NoError(t, err) + require.NotNil(t, res) + require.Len(t, res.Content, 1) + text, ok := res.Content[0].(mcp.TextContent) + require.True(t, ok) + assert.Equal(t, ghToken, text.Text, "disabled by default: content must pass through unmodified") +} diff --git a/pkg/vmcp/server/serve.go b/pkg/vmcp/server/serve.go index ec05aa26de..f2bbdd27c4 100644 --- a/pkg/vmcp/server/serve.go +++ b/pkg/vmcp/server/serve.go @@ -140,6 +140,11 @@ type ServerConfig struct { // AuditConfig is the cross-cutting audit configuration (also consumed by // core.New). If nil, no audit logging is performed. AuditConfig *audit.Config + + // RedactToolResultSecrets enables best-effort credential-shape scanning + // (see pkg/mcp/secretscan) on tools/call results before they are relayed + // to the client. Opt-in (default false); see Config.RedactToolResultSecrets. + RedactToolResultSecrets bool } // Serve is the transport-side entry point of the New/Serve split: it wraps an @@ -426,5 +431,6 @@ func buildServeConfig(cfg *ServerConfig) *Config { StatusReportingInterval: cfg.StatusReportingInterval, Watcher: cfg.Watcher, SessionStorage: cfg.SessionStorage, + RedactToolResultSecrets: cfg.RedactToolResultSecrets, } } diff --git a/pkg/vmcp/server/serve_handlers.go b/pkg/vmcp/server/serve_handlers.go index c39b25c75e..b2c5fb0673 100644 --- a/pkg/vmcp/server/serve_handlers.go +++ b/pkg/vmcp/server/serve_handlers.go @@ -15,6 +15,7 @@ import ( "github.com/stacklok/toolhive/pkg/audit" "github.com/stacklok/toolhive/pkg/auth" mcpparser "github.com/stacklok/toolhive/pkg/mcp" + "github.com/stacklok/toolhive/pkg/mcp/secretscan" "github.com/stacklok/toolhive/pkg/vmcp" "github.com/stacklok/toolhive/pkg/vmcp/conversion" vmcpsession "github.com/stacklok/toolhive/pkg/vmcp/session" @@ -306,9 +307,14 @@ func (s *Server) coreToolHandler(sessionID, toolName, backendName string) server return conversion.ErrorToToolResult(err), nil } + content := conversion.ToMCPContents(result.Content) + if s.config.RedactToolResultSecrets { + secretscan.RedactContentInPlace(content) + } + return &mcp.CallToolResult{ Result: mcp.Result{Meta: conversion.ToMCPMeta(result.Meta)}, - Content: conversion.ToMCPContents(result.Content), + Content: content, StructuredContent: result.StructuredContent, IsError: result.IsError, }, nil diff --git a/pkg/vmcp/server/serve_session_test.go b/pkg/vmcp/server/serve_session_test.go index 39f0fca730..d5d07665af 100644 --- a/pkg/vmcp/server/serve_session_test.go +++ b/pkg/vmcp/server/serve_session_test.go @@ -153,12 +153,13 @@ type fakeCore struct { lastCompleteRef atomic.Value // vmcp.CompletionRef completeValues []string // returned by Complete when completeErr is nil - callErr error // when set, CallTool returns it (e.g. vmcp.ErrAuthorizationFailed) - readErr error // when set, ReadResource returns it - readMeta map[string]any // when set, ReadResource sets it as result.Meta on success - promptErr error // when set, GetPrompt returns it (e.g. vmcp.ErrAuthorizationFailed) - completeErr error // when set, Complete returns it (e.g. vmcp.ErrAuthorizationFailed) - lookupResourceErr error // when set, LookupResource returns it for an ADVERTISED URI (admission denial) + callErr error // when set, CallTool returns it (e.g. vmcp.ErrAuthorizationFailed) + callResult *vmcp.ToolCallResult // when set, CallTool returns it instead of the default {Text:"ok"} + readErr error // when set, ReadResource returns it + readMeta map[string]any // when set, ReadResource sets it as result.Meta on success + promptErr error // when set, GetPrompt returns it (e.g. vmcp.ErrAuthorizationFailed) + completeErr error // when set, Complete returns it (e.g. vmcp.ErrAuthorizationFailed) + lookupResourceErr error // when set, LookupResource returns it for an ADVERTISED URI (admission denial) // invalidateCacheCalls counts InvalidateCapabilityCache invocations, so tests // covering the list_changed sink can assert the cache was re-swept (#5748). @@ -214,6 +215,9 @@ func (f *fakeCore) CallTool( if f.callErr != nil { return nil, f.callErr } + if f.callResult != nil { + return f.callResult, nil + } return &vmcp.ToolCallResult{Content: []vmcp.Content{{Type: vmcp.ContentTypeText, Text: "ok"}}}, nil } diff --git a/pkg/vmcp/server/serve_test.go b/pkg/vmcp/server/serve_test.go index cdfc7ebd22..47669cbf82 100644 --- a/pkg/vmcp/server/serve_test.go +++ b/pkg/vmcp/server/serve_test.go @@ -375,6 +375,7 @@ func TestBuildServeConfigMapsSharedFields(t *testing.T) { SessionManagerConfig: testMinimalSessionManagerConfig(), TelemetryProvider: &telemetry.Provider{}, AuditConfig: &audit.Config{}, + RedactToolResultSecrets: true, } got := reflect.ValueOf(*buildServeConfig(src)) diff --git a/pkg/vmcp/server/server.go b/pkg/vmcp/server/server.go index ba19076d34..c3b04561a3 100644 --- a/pkg/vmcp/server/server.go +++ b/pkg/vmcp/server/server.go @@ -262,6 +262,16 @@ type Config struct { // from the HTTP middleware. When non-nil, Name must be non-empty (the Cedar resource // entity name). Authz *authz.Config + + // RedactToolResultSecrets enables best-effort credential-shape scanning + // (see pkg/mcp/secretscan) on tools/call results before they are relayed + // to the client: matches are redacted in place. Opt-in (default false), + // matching the same knob on ToolHive's own proxies (streamable. + // WithSecretRedaction / transparent.WithSecretRedaction) -- the backend + // MCP servers this vMCP aggregates are often operator-trusted, and the + // scan adds per-call overhead, so this is only worth enabling when a + // backend is not fully trusted. + RedactToolResultSecrets bool } // Server is the Virtual MCP Server that aggregates multiple backends. From 812c1d65594e43fa52709823d2004cf829e2ac38 Mon Sep 17 00:00:00 2001 From: Reynier Ortiz Vega Date: Fri, 21 Aug 2026 13:21:22 -0400 Subject: [PATCH 07/10] docs: regenerate CLI docs for --redact-tool-result-secrets CI's Verify Swagger Documentation check caught that docs/cli/thv_run.md was stale after adding the flag. Also drops the flag help text's now- inaccurate "(streamable-HTTP proxy only)" qualifier -- the transparent proxy and pkg/vmcp/server commits landed the same knob covers all three code paths, not just the streamable one. --- cmd/thv/app/run_flags.go | 2 +- docs/cli/thv_run.md | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/cmd/thv/app/run_flags.go b/cmd/thv/app/run_flags.go index 115622617b..e39532d0df 100644 --- a/cmd/thv/app/run_flags.go +++ b/cmd/thv/app/run_flags.go @@ -296,7 +296,7 @@ func AddRunFlags(cmd *cobra.Command, config *RunFlags) { "(streamable-HTTP proxy only; an absent header is accepted). Off by default: any version is accepted.") cmd.Flags().BoolVar(&config.RedactToolResultSecrets, "redact-tool-result-secrets", false, "Scan tools/call responses for credential-shaped content (AWS/GitHub/Slack/Google/Stripe keys, JWTs, PEM "+ - "private keys) and redact matches before relaying them to the client (streamable-HTTP proxy only). "+ + "private keys, generic Bearer tokens) and redact matches before relaying them to the client. "+ "Off by default; enable when the backend MCP server is not fully trusted.") cmd.Flags().BoolVar(&config.Stateless, "stateless", false, "Declare the server as stateless (POST-only, no SSE). "+ diff --git a/docs/cli/thv_run.md b/docs/cli/thv_run.md index 1ff9313604..8d97e46e2a 100644 --- a/docs/cli/thv_run.md +++ b/docs/cli/thv_run.md @@ -159,6 +159,7 @@ thv run [flags] SERVER_OR_IMAGE_OR_PROTOCOL [-- ARGS...] --proxy-mode string Proxy mode for stdio (streamable-http or sse (deprecated, will be removed)) (default "streamable-http") --proxy-port int Port for the HTTP proxy to listen on (host port) -p, --publish stringArray Publish a container's port(s) to the host (format: hostPort:containerPort) + --redact-tool-result-secrets Scan tools/call responses for credential-shaped content (AWS/GitHub/Slack/Google/Stripe keys, JWTs, PEM private keys, generic Bearer tokens) and redact matches before relaying them to the client. Off by default; enable when the backend MCP server is not fully trusted. --remote-auth Enable OAuth/OIDC authentication to remote MCP server (default false) --remote-auth-authorize-url string OAuth authorization endpoint URL (alternative to --remote-auth-issuer for non-OIDC OAuth) --remote-auth-bearer-token string Bearer token for remote server authentication (alternative to OAuth) From 50dda52dba7b9d1a9a8c2c3170901160b849d0b9 Mon Sep 17 00:00:00 2001 From: Reynier Ortiz Vega Date: Fri, 21 Aug 2026 13:54:43 -0400 Subject: [PATCH 08/10] docs: regenerate swagger docs for RedactToolResultSecrets CI's Verify Swagger Documentation check also covers docs/server/* (swag init over pkg/api, which reaches RunConfig via the REST API schema) -- the earlier docs/cli-only regen missed this. Also corrects RunConfig.RedactToolResultSecrets's doc comment, which still said "streamable HTTP proxy" only; it now covers the transparent reverse-proxy path too. --- docs/server/docs.go | 4 ++++ docs/server/swagger.json | 4 ++++ docs/server/swagger.yaml | 9 +++++++++ pkg/runner/config.go | 8 +++++--- 4 files changed, 22 insertions(+), 3 deletions(-) diff --git a/docs/server/docs.go b/docs/server/docs.go index 180b90a904..6a39dcf933 100644 --- a/docs/server/docs.go +++ b/docs/server/docs.go @@ -1646,6 +1646,10 @@ const docTemplate = `{ "description": "RateLimitNamespace is the Kubernetes namespace for Redis key derivation.", "type": "string" }, + "redact_tool_result_secrets": { + "description": "RedactToolResultSecrets enables best-effort credential-shape scanning\non tools/call responses relayed by the proxy (streamable-HTTP or the\ntransparent reverse-proxy, whichever the transport resolves to):\nmatches are redacted before the response reaches the client. Opt-in\n(default false); enable it when the backend MCP server is not fully\ntrusted.", + "type": "boolean" + }, "registry_api_url": { "description": "RegistryAPIURL is the registry API URL that served this server's metadata.\nEmpty when the server was not discovered via registry lookup.", "type": "string" diff --git a/docs/server/swagger.json b/docs/server/swagger.json index 8555fddda8..5b4b0bd597 100644 --- a/docs/server/swagger.json +++ b/docs/server/swagger.json @@ -1639,6 +1639,10 @@ "description": "RateLimitNamespace is the Kubernetes namespace for Redis key derivation.", "type": "string" }, + "redact_tool_result_secrets": { + "description": "RedactToolResultSecrets enables best-effort credential-shape scanning\non tools/call responses relayed by the proxy (streamable-HTTP or the\ntransparent reverse-proxy, whichever the transport resolves to):\nmatches are redacted before the response reaches the client. Opt-in\n(default false); enable it when the backend MCP server is not fully\ntrusted.", + "type": "boolean" + }, "registry_api_url": { "description": "RegistryAPIURL is the registry API URL that served this server's metadata.\nEmpty when the server was not discovered via registry lookup.", "type": "string" diff --git a/docs/server/swagger.yaml b/docs/server/swagger.yaml index 7ff2940444..ff290c46be 100644 --- a/docs/server/swagger.yaml +++ b/docs/server/swagger.yaml @@ -1785,6 +1785,15 @@ components: description: RateLimitNamespace is the Kubernetes namespace for Redis key derivation. type: string + redact_tool_result_secrets: + description: |- + RedactToolResultSecrets enables best-effort credential-shape scanning + on tools/call responses relayed by the proxy (streamable-HTTP or the + transparent reverse-proxy, whichever the transport resolves to): + matches are redacted before the response reaches the client. Opt-in + (default false); enable it when the backend MCP server is not fully + trusted. + type: boolean registry_api_url: description: |- RegistryAPIURL is the registry API URL that served this server's metadata. diff --git a/pkg/runner/config.go b/pkg/runner/config.go index 37804981e3..620cfbbd73 100644 --- a/pkg/runner/config.go +++ b/pkg/runner/config.go @@ -214,9 +214,11 @@ type RunConfig struct { StrictProtocolValidation bool `json:"strict_protocol_validation,omitempty" yaml:"strict_protocol_validation,omitempty"` // RedactToolResultSecrets enables best-effort credential-shape scanning - // on tools/call responses relayed by the streamable HTTP proxy: matches - // are redacted before the response reaches the client. Opt-in (default - // false); enable it when the backend MCP server is not fully trusted. + // on tools/call responses relayed by the proxy (streamable-HTTP or the + // transparent reverse-proxy, whichever the transport resolves to): + // matches are redacted before the response reaches the client. Opt-in + // (default false); enable it when the backend MCP server is not fully + // trusted. RedactToolResultSecrets bool `json:"redact_tool_result_secrets,omitempty" yaml:"redact_tool_result_secrets,omitempty"` // Stateless indicates the server only supports POST (no SSE/GET). From 6245b587df68e6c3d19f3abad8534f426225ff4c Mon Sep 17 00:00:00 2001 From: Reynier Ortiz Vega Date: Fri, 21 Aug 2026 14:14:14 -0400 Subject: [PATCH 09/10] docs+crd: regenerate VirtualMCPServer CRD and CRD reference docs VirtualMCPServerSpec.Config embeds pkg/vmcp/config.Config directly (cmd/thv-operator/pkg/vmcpconfig/converter.go's DeepCopy comment: "new fields added to config.Config are automatically included"), so RedactToolResultSecrets is already wired end-to-end for VirtualMCPServer with no converter change needed -- just the generated-artifact regen CI caught as stale (task operator-manifests, task crdref-gen). --- .../toolhive.stacklok.dev_virtualmcpservers.yaml | 16 ++++++++++++++++ .../toolhive.stacklok.dev_virtualmcpservers.yaml | 16 ++++++++++++++++ docs/operator/crd-api.md | 2 ++ 3 files changed, 34 insertions(+) diff --git a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml index 191157600b..62fb50a61a 100644 --- a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml +++ b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml @@ -3195,6 +3195,14 @@ spec: - message: at least one of shared, perUser, or tools must be configured rule: has(self.shared) || has(self.perUser) || (has(self.tools) && size(self.tools) > 0) + redactToolResultSecrets: + default: false + description: |- + RedactToolResultSecrets enables best-effort credential-shape scanning + (see pkg/mcp/secretscan) on tools/call results before they are relayed + to the client: matches are redacted in place. Opt-in (default false); + enable when a backend this vMCP aggregates is not fully trusted. + type: boolean sessionStorage: description: |- SessionStorage configures session storage for stateful horizontal scaling. @@ -7186,6 +7194,14 @@ spec: - message: at least one of shared, perUser, or tools must be configured rule: has(self.shared) || has(self.perUser) || (has(self.tools) && size(self.tools) > 0) + redactToolResultSecrets: + default: false + description: |- + RedactToolResultSecrets enables best-effort credential-shape scanning + (see pkg/mcp/secretscan) on tools/call results before they are relayed + to the client: matches are redacted in place. Opt-in (default false); + enable when a backend this vMCP aggregates is not fully trusted. + type: boolean sessionStorage: description: |- SessionStorage configures session storage for stateful horizontal scaling. diff --git a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml index a96ecfcb00..6e6d289bfb 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml @@ -3198,6 +3198,14 @@ spec: - message: at least one of shared, perUser, or tools must be configured rule: has(self.shared) || has(self.perUser) || (has(self.tools) && size(self.tools) > 0) + redactToolResultSecrets: + default: false + description: |- + RedactToolResultSecrets enables best-effort credential-shape scanning + (see pkg/mcp/secretscan) on tools/call results before they are relayed + to the client: matches are redacted in place. Opt-in (default false); + enable when a backend this vMCP aggregates is not fully trusted. + type: boolean sessionStorage: description: |- SessionStorage configures session storage for stateful horizontal scaling. @@ -7189,6 +7197,14 @@ spec: - message: at least one of shared, perUser, or tools must be configured rule: has(self.shared) || has(self.perUser) || (has(self.tools) && size(self.tools) > 0) + redactToolResultSecrets: + default: false + description: |- + RedactToolResultSecrets enables best-effort credential-shape scanning + (see pkg/mcp/secretscan) on tools/call results before they are relayed + to the client: matches are redacted in place. Opt-in (default false); + enable when a backend this vMCP aggregates is not fully trusted. + type: boolean sessionStorage: description: |- SessionStorage configures session storage for stateful horizontal scaling. diff --git a/docs/operator/crd-api.md b/docs/operator/crd-api.md index 5138e552a9..8d5a424627 100644 --- a/docs/operator/crd-api.md +++ b/docs/operator/crd-api.md @@ -421,6 +421,7 @@ _Appears in:_ | `sessionStorage` _[vmcp.config.SessionStorageConfig](#vmcpconfigsessionstorageconfig)_ | SessionStorage configures session storage for stateful horizontal scaling.
When provider is "redis", the operator injects Redis connection parameters
(address, db, keyPrefix) here. The Redis password is provided separately via
the THV_SESSION_REDIS_PASSWORD environment variable. | | Optional: \{\}
| | `rateLimiting` _[ratelimit.types.RateLimitConfig](#ratelimittypesratelimitconfig)_ | RateLimiting defines rate limiting configuration for the Virtual MCP server.
Requires Redis session storage to be configured for distributed rate limiting. | | Optional: \{\}
| | `passthroughHeaders` _string array_ | PassthroughHeaders is an allowlist of incoming client request header names
forwarded verbatim to all backends. Captured at the vMCP incoming edge by
headerforward.CaptureMiddleware and consumed once at session creation
when the per-session backend client's HeaderForwardConfig is built. Names
must not be in the restricted set (Host, hop-by-hop, X-Forwarded-*, etc.). | | Optional: \{\}
| +| `redactToolResultSecrets` _boolean_ | RedactToolResultSecrets enables best-effort credential-shape scanning
(see pkg/mcp/secretscan) on tools/call results before they are relayed
to the client: matches are redacted in place. Opt-in (default false);
enable when a backend this vMCP aggregates is not fully trusted. | false | Optional: \{\}
| #### vmcp.config.ConflictResolutionConfig @@ -3310,6 +3311,7 @@ _Appears in:_ | `authServerRef` _[api.v1beta1.AuthServerRef](#apiv1beta1authserverref)_ | AuthServerRef optionally references a resource that configures an embedded
OAuth 2.0/OIDC authorization server to authenticate MCP clients.
Currently the only supported kind is MCPExternalAuthConfig (type: embeddedAuthServer). | | Optional: \{\}
| | `telemetryConfigRef` _[api.v1beta1.MCPTelemetryConfigReference](#apiv1beta1mcptelemetryconfigreference)_ | TelemetryConfigRef references an MCPTelemetryConfig resource for shared telemetry configuration.
The referenced MCPTelemetryConfig must exist in the same namespace as this MCPServer.
Cross-namespace references are not supported for security and isolation reasons. | | Optional: \{\}
| | `trustProxyHeaders` _boolean_ | TrustProxyHeaders indicates whether to trust X-Forwarded-* headers from reverse proxies
When enabled, the proxy will use X-Forwarded-Proto, X-Forwarded-Host, X-Forwarded-Port,
and X-Forwarded-Prefix headers to construct endpoint URLs | false | Optional: \{\}
| +| `redactToolResultSecrets` _boolean_ | RedactToolResultSecrets enables best-effort credential-shape scanning
on tools/call responses (AWS/GitHub/Slack/Google/Stripe keys, JWTs, PEM
private keys): matches are redacted before the response reaches the
client. Off by default; enable when the backend MCP server is not
fully trusted. This setting is ONLY applicable when Transport is
"stdio" (the streamable-HTTP proxy path) -- it has no effect when
Transport is "streamable-http" or "sse" (those reverse-proxy to an
already-HTTP backend without inspecting message content). | false | Optional: \{\}
| | `endpointPrefix` _string_ | EndpointPrefix is the path prefix to prepend to SSE endpoint URLs.
This is used to handle path-based ingress routing scenarios where the ingress
strips a path prefix before forwarding to the backend. | | Optional: \{\}
| | `groupRef` _[api.v1beta1.MCPGroupRef](#apiv1beta1mcpgroupref)_ | GroupRef references the MCPGroup this server belongs to.
The referenced MCPGroup must be in the same namespace. | | Optional: \{\}
| | `sessionAffinity` _string_ | SessionAffinity controls whether the Service routes repeated client connections to the same pod.
MCP protocols (SSE, streamable-http) are stateful, so ClientIP is the default.
Set to "None" for stateless servers or when using an external load balancer with its own affinity. | ClientIP | Enum: [ClientIP None]
Optional: \{\}
| From 8bdb8abefb1c3c96049eb5fdeffd1063c83e34d0 Mon Sep 17 00:00:00 2001 From: Reynier Ortiz Vega Date: Fri, 21 Aug 2026 14:44:23 -0400 Subject: [PATCH 10/10] fix(transparent-proxy): reassemble SSE events before scanning for secrets Both SSE-handling response processors scanned each "data:" line in isolation. Per the SSE spec, a compliant client concatenates consecutive "data:" lines (joined by "\n") into one logical event value before consuming it -- so a hostile backend could deliberately split a single JSON-RPC message across two "data:" lines specifically to evade a per-line scanner, while the real downstream MCP client still reassembles and receives the whole secret unredacted. Both SecretRedactionResponseProcessor.processSSE (streamable-http transport type) and sseLineProcessor (legacy sse transport type) now buffer contiguous "data:" lines until a non-data line (event boundary), reassemble them per spec via the new joinSSEDataLines, and scan/redact that reassembled value. When nothing changes, the original raw lines are re-emitted byte-for-byte -- this is a detection fix, not a reformatting of untouched output. New regression tests split the same fixture across two "data:" lines for both processors and assert the secret still gets caught. --- .../secret_redaction_response_processor.go | 54 ++++++++- ...ecret_redaction_response_processor_test.go | 71 +++++++++++ .../transparent/sse_response_processor.go | 111 ++++++++++-------- 3 files changed, 184 insertions(+), 52 deletions(-) diff --git a/pkg/transport/proxy/transparent/secret_redaction_response_processor.go b/pkg/transport/proxy/transparent/secret_redaction_response_processor.go index 102bd1ffac..51ff2d7a79 100644 --- a/pkg/transport/proxy/transparent/secret_redaction_response_processor.go +++ b/pkg/transport/proxy/transparent/secret_redaction_response_processor.go @@ -133,24 +133,68 @@ func (*SecretRedactionResponseProcessor) processSSE(resp *http.Response) { // sizeable tool result. scanner.Buffer(make([]byte, 0, 1024), 1024*1024) + // dataBuf accumulates consecutive raw "data:" lines belonging to the + // SAME SSE event. Per the SSE spec, a compliant client concatenates + // them (joined by "\n") into one logical value before consuming it -- + // scanning each "data:" line in isolation would let a hostile backend + // split a single JSON-RPC message across lines specifically to evade + // this scanner while the real client reassembles it and still sees + // the whole payload. flush reconstructs that same logical value. + var dataBuf []string + flush := func() bool { + if len(dataBuf) == 0 { + return true + } + joined := joinSSEDataLines(dataBuf) + lines := dataBuf + if redacted, changed, err := redactJSONRPCBody([]byte(joined)); err == nil && changed { + lines = []string{"data: " + string(redacted)} + } + dataBuf = nil + for _, l := range lines { + if _, err := pw.Write([]byte(l + "\n")); err != nil { + return false + } + } + return true + } + for scanner.Scan() { line := scanner.Text() - if after, ok := strings.CutPrefix(line, "data:"); ok { - dataContent := strings.TrimSpace(after) - if redacted, changed, err := redactJSONRPCBody([]byte(dataContent)); err == nil && changed { - line = "data: " + string(redacted) - } + if strings.HasPrefix(line, "data:") { + dataBuf = append(dataBuf, line) + continue + } + if !flush() { + return } if _, err := pw.Write([]byte(line + "\n")); err != nil { return } } + // A stream that ends without a trailing blank line still has a + // pending event to reassemble and forward. + if !flush() { + return + } if err := scanner.Err(); err != nil { slog.Error("failed to scan SSE response body for secret scan", "error", err) } }() } +// joinSSEDataLines reconstructs the logical value of one SSE event's data +// field from its raw "data:" lines, per the spec: each line's content (after +// stripping the "data:" prefix and at most one leading space) is joined with +// "\n". +func joinSSEDataLines(rawLines []string) string { + contents := make([]string, len(rawLines)) + for i, l := range rawLines { + contents[i] = strings.TrimSpace(strings.TrimPrefix(l, "data:")) + } + return strings.Join(contents, "\n") +} + // redactJSONRPCBody decodes data as a JSON object, redacts its "result" // field via secretscan (if present), and returns the re-encoded body. changed // is false whenever nothing needed redacting -- including when data doesn't diff --git a/pkg/transport/proxy/transparent/secret_redaction_response_processor_test.go b/pkg/transport/proxy/transparent/secret_redaction_response_processor_test.go index 6d43498699..cd0e458e9e 100644 --- a/pkg/transport/proxy/transparent/secret_redaction_response_processor_test.go +++ b/pkg/transport/proxy/transparent/secret_redaction_response_processor_test.go @@ -22,6 +22,18 @@ func toolCallResultJSON() string { return `{"jsonrpc":"2.0","id":1,"result":{"content":[{"type":"text","text":"Authorization: Bearer ` + sentinelToken + `"}]}}` } +// toolCallResultJSONSplit returns the same JSON-RPC message as +// toolCallResultJSON, but split into two fragments at a whitespace- +// insignificant JSON boundary (right after a top-level comma) -- valid to +// reassemble with a "\n" join, exactly as an SSE client concatenating two +// consecutive "data:" lines would. Used to simulate a hostile backend +// splitting a message across "data:" lines specifically to evade a +// per-line-only scanner. +func toolCallResultJSONSplit() (first, second string) { + return `{"jsonrpc":"2.0",`, + `"id":1,"result":{"content":[{"type":"text","text":"Authorization: Bearer ` + sentinelToken + `"}]}}` +} + // buildRedactingProxy wires up a *httputil.ReverseProxy fronting target using // a TransparentProxy configured with WithSecretRedaction(true) for the given // transport type -- mirroring the createBasicProxy/modifyResponse harness @@ -152,3 +164,62 @@ func TestSecretRedaction_Disabled_ByDefault(t *testing.T) { assert.Contains(t, rec.Body.String(), sentinelToken, "disabled by default: response must pass through unmodified") } + +// TestSecretRedaction_StreamableHTTP_SplitAcrossDataLines is a regression +// test for a bypass: a hostile backend that splits a single JSON-RPC message +// across two "data:" lines (valid per the SSE spec -- a compliant client +// reassembles them) must not evade the scanner, which used to inspect each +// "data:" line in isolation. +func TestSecretRedaction_StreamableHTTP_SplitAcrossDataLines(t *testing.T) { + t.Parallel() + + first, second := toolCallResultJSONSplit() + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "text/event-stream") + w.WriteHeader(http.StatusOK) + w.Write([]byte("data: " + first + "\ndata: " + second + "\n\n")) + w.(http.Flusher).Flush() + })) + defer target.Close() + targetURL, err := url.Parse(target.URL) + require.NoError(t, err) + + proxy := buildRedactingProxy(t, "streamable-http", targetURL) + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, target.URL, nil) + proxy.ServeHTTP(rec, req) + + assert.NotContains(t, rec.Body.String(), sentinelToken, + "sentinel token split across data: lines must not reach the client") + assert.Contains(t, rec.Body.String(), "REDACTED-BY-TOOLHIVE") +} + +// TestSecretRedaction_LegacySSETransport_SplitAcrossDataLines is the same +// regression as above, for the legacy sse transport type's response +// processor. +func TestSecretRedaction_LegacySSETransport_SplitAcrossDataLines(t *testing.T) { + t.Parallel() + + first, second := toolCallResultJSONSplit() + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "text/event-stream") + w.WriteHeader(http.StatusOK) + w.Write([]byte("data: " + first + "\ndata: " + second + "\n\n")) + w.(http.Flusher).Flush() + })) + defer target.Close() + targetURL, err := url.Parse(target.URL) + require.NoError(t, err) + + proxy := buildRedactingProxy(t, "sse", targetURL) + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, target.URL, nil) + proxy.ServeHTTP(rec, req) + + body := rec.Body.String() + assert.NotContains(t, body, sentinelToken, + "sentinel token split across data: lines must not reach the client") + assert.Contains(t, body, "REDACTED-BY-TOOLHIVE") +} diff --git a/pkg/transport/proxy/transparent/sse_response_processor.go b/pkg/transport/proxy/transparent/sse_response_processor.go index ec49e4dbc9..e6964998f3 100644 --- a/pkg/transport/proxy/transparent/sse_response_processor.go +++ b/pkg/transport/proxy/transparent/sse_response_processor.go @@ -218,60 +218,87 @@ type sseLineProcessor struct { currentEventType string sessionFound bool redactSecrets bool + + // dataBuf accumulates consecutive raw "data:" lines belonging to the SAME + // SSE event. Per the SSE spec, a compliant client concatenates them + // (joined by "\n") into one logical value before consuming it -- URL + // rewriting and redaction must operate on that same reassembled value, not + // each "data:" line in isolation, or a hostile backend could split a + // single JSON-RPC message across lines specifically to evade the + // redaction scan while the real client still reassembles and sees it. + dataBuf []string } -// processLine processes a single SSE line and returns the potentially modified line. -func (s *sseLineProcessor) processLine(line string) string { +// processLine processes a single SSE line and returns the lines to emit in +// its place: zero while a "data:" run is being buffered, or more than one +// when a buffered run is flushed ahead of a non-data line. +func (s *sseLineProcessor) processLine(line string) []string { // Parse SSE event type if strings.HasPrefix(line, "event:") { + out := s.flushDataBuf() s.currentEventType = strings.TrimSpace(strings.TrimPrefix(line, "event:")) - return line + return append(out, line) } // Empty line marks the end of an SSE event, reset event type if line == "" { + out := s.flushDataBuf() s.currentEventType = "" - return line + return append(out, line) } - // Process data lines + // Accumulate data lines; extraction/rewrite/redaction happens once the + // run is flushed (see flushDataBuf), on the reassembled value. if strings.HasPrefix(line, "data:") { - return s.processDataLine(line) + s.extractSessionID(line) + s.dataBuf = append(s.dataBuf, line) + return nil } - return line + out := s.flushDataBuf() + return append(out, line) } -// processDataLine handles SSE data lines for session extraction and URL rewriting. -func (s *sseLineProcessor) processDataLine(line string) string { - dataContent := strings.TrimSpace(strings.TrimPrefix(line, "data:")) +// flushDataBuf reassembles any buffered "data:" lines into one logical value +// (per the SSE spec), applies endpoint-URL rewriting and/or secret redaction +// to that value, and returns the line(s) to emit. When neither transform +// changes anything, the original raw lines are returned unchanged so a +// disabled/no-op pass is byte-identical to the pre-buffering behavior. +func (s *sseLineProcessor) flushDataBuf() []string { + if len(s.dataBuf) == 0 { + return nil + } + rawLines := s.dataBuf + s.dataBuf = nil - // Extract session ID for tracking (from any data line) - s.extractSessionID(line) + value := joinSSEDataLines(rawLines) + changed := false - // Rewrite endpoint URLs only for "endpoint" events if s.currentEventType == "endpoint" && s.rewriteConfig.hasRewriteConfig() { - line = s.rewriteDataLine(line, dataContent) + rewritten, err := rewriteEndpointURL(value, s.rewriteConfig) + switch { + case err != nil: + //nolint:gosec // G706: logging endpoint URL from SSE stream + slog.Warn("failed to rewrite endpoint URL", "url", value, "error", err) + case rewritten != value: + //nolint:gosec // G706: logging endpoint URLs from SSE stream + slog.Debug("rewrote SSE endpoint URL", "from", value, "to", rewritten) + value = rewritten + changed = true + } } if s.redactSecrets { - line = s.redactDataLine(line) + if redacted, ok, err := redactJSONRPCBody([]byte(value)); err == nil && ok { + value = string(redacted) + changed = true + } } - return line -} - -// redactDataLine redacts credential-shaped content from a data line's -// JSON-RPC payload (see pkg/mcp/secretscan). Lines that aren't a JSON-RPC -// response object (endpoint events, notifications with no "result", any -// content that fails to decode) are returned unchanged. -func (*sseLineProcessor) redactDataLine(line string) string { - dataContent := strings.TrimSpace(strings.TrimPrefix(line, "data:")) - redacted, changed, err := redactJSONRPCBody([]byte(dataContent)) - if err != nil || !changed { - return line + if !changed { + return rawLines } - return "data: " + string(redacted) + return []string{"data: " + value} } // extractSessionID extracts and stores the session ID from a data line. @@ -292,24 +319,6 @@ func (s *sseLineProcessor) extractSessionID(line string) { } } -// rewriteDataLine rewrites the URL in an endpoint event's data line. -func (s *sseLineProcessor) rewriteDataLine(line, dataContent string) string { - rewrittenURL, err := rewriteEndpointURL(dataContent, s.rewriteConfig) - if err != nil { - //nolint:gosec // G706: logging endpoint URL from SSE stream - slog.Warn("failed to rewrite endpoint URL", - "url", dataContent, "error", err) - return line - } - if rewrittenURL != dataContent { - //nolint:gosec // G706: logging endpoint URLs from SSE stream - slog.Debug("rewrote SSE endpoint URL", - "from", dataContent, "to", rewrittenURL) - return "data: " + rewrittenURL - } - return line -} - // processSSEStream processes an SSE stream, extracting session IDs and rewriting URLs. func (s *SSEResponseProcessor) processSSEStream(originalBody io.Reader, pw *io.PipeWriter, rewriteConfig sseRewriteConfig) { scanner := bufio.NewScanner(originalBody) @@ -326,7 +335,15 @@ func (s *SSEResponseProcessor) processSSEStream(originalBody io.Reader, pw *io.P } for scanner.Scan() { - line := processor.processLine(scanner.Text()) + for _, line := range processor.processLine(scanner.Text()) { + if _, err := pw.Write([]byte(line + "\n")); err != nil { + return + } + } + } + // A stream that ends without a trailing blank line still has a pending + // event to reassemble and forward. + for _, line := range processor.flushDataBuf() { if _, err := pw.Write([]byte(line + "\n")); err != nil { return }