feat(streamable-proxy,transparent-proxy,vmcp-server,operator): opt-in secret redaction for tool-call results

[reyortiz3]

Summary

A DAST assessment flagged: "Tool results are relayed without content inspection" (Medium) -- a compromised or malicious MCP backend can embed credentials in a tools/call result and have them delivered straight through to the client, with no containment boundary.

Adds pkg/mcp/secretscan, a best-effort scanner recognizing common credential shapes (AWS access keys, GitHub/Slack/Google/Stripe tokens, JWTs, PEM private-key blocks, generic Authorization: Bearer <token>) and redacting matches with a content-free placeholder. secretscan.RedactContentInPlace is the shared primitive (operates on a []mcp.Content slice in place); ScanAndRedactToolCallResult (JSON in/out, for the two byte-stream proxies) is built on top of it.

Wired into three structurally separate code paths within ToolHive that can front an untrusted MCP backend:

1. pkg/transport/proxy/streamable -- the stdio-managed-container path (thv run <image>, MCPServer with Transport: stdio). Scans both response-writing paths (plain JSON, SSE final frame).
2. pkg/transport/proxy/transparent -- the reverse-proxy path used whenever the backend already speaks HTTP: thv run <remote-url>, MCPRemoteProxy, and MCPServer with Transport: streamable-http/sse. New SecretRedactionResponseProcessor handles both response shapes a streamable-HTTP server can emit (a plain application/json body, and a text/event-stream response streamed line-by-line so a long-lived stream is never fully buffered); the legacy sse-transport-type path gets the same redaction composed into the existing SSEResponseProcessor. This is the path the repo's own DAST harness actually exercises -- dast/mcp-proxy/harness_test.go's TestAdversarialContainment runs thv run --stateless <url>, which never sets --transport and so always resolves to streamable-http against a bare URL, i.e. this branch, not the stdio/streamable-proxy branch from (1) alone.
3. pkg/vmcp/server -- the embedded-vMCP-as-a-library path used by anything that builds *vmcpserver.Server directly on top of core.VMCP (e.g. the enterprise connector-gateway), bypassing both proxy packages above entirely. Wired into both the Legacy/SDK dispatch (serve_handlers.go's coreToolHandler) and the Modern/stateless dispatch (modern_envelope.go's newModernCallToolResult), the two places that build the wire Content from the core's domain result.

Fails open throughout: a result payload that doesn't decode as a recognizable JSON-RPC/CallToolResult shape is forwarded unchanged (logged at Debug) -- this must never be the reason a legitimate tool call breaks. A non-streaming transparent-proxy response body is capped at 8MB (matching bodylimit.DefaultMaxRequestBodySize) and forwarded unscanned if larger, rather than risking unbounded memory growth on a hostile upstream.

SSE event reassembly (bypass fix). Both SSE-handling response processors in path (2) buffer contiguous data: lines until the event boundary and reassemble them per the SSE spec (join with \n) before scanning -- not line-by-line. A hostile backend could otherwise split a single JSON-RPC message across two data: lines specifically to evade a per-line scanner, while a real downstream MCP client still reassembles and receives the whole secret. Regression tests (TestSecretRedaction_StreamableHTTP_SplitAcrossDataLines, TestSecretRedaction_LegacySSETransport_SplitAcrossDataLines) cover this directly. When nothing needs redacting, the original raw lines are re-emitted byte-for-byte, so disabled/no-op behavior is unchanged.

Opt-in, default off

Many deployments run fully operator-trusted MCP backends where this scan is pure overhead with no security benefit, so it's plumbed as a config knob rather than always-on, the same way --strict-protocol-validation is -- and the same knob name covers all three paths:

• streamable.WithSecretRedaction / transparent.WithSecretRedaction -- proxy-level options (paths 1-2)
• StdioTransport.SetSecretRedaction / HTTPTransport.redactToolResultSecrets / types.Config.RedactToolResultSecrets (paths 1-2)
• runner.WithRedactToolResultSecrets / RunConfig.RedactToolResultSecrets (redact_tool_result_secrets in the run-config JSON/YAML) (paths 1-2)
• thv run --redact-tool-result-secrets CLI flag, carried across thv upgrade via applier.go (paths 1-2)
• vmcpserver.Config.RedactToolResultSecrets / ServerConfig.RedactToolResultSecrets (path 3, the embedded-vMCP-as-a-library path)
• vmcpconfig.Config.RedactToolResultSecrets (redactToolResultSecrets in vmcp YAML config) -> vmcp serve CLI (path 3, standalone OSS binary)

Kubernetes / operator:

• MCPServer (paths 1-2): the operator builds RunConfig directly via runner.WithXxx(...) calls in mcpserver_runconfig.go -- it never shells out thv run flags -- so only CRD fields the controller explicitly reads reach the proxy pod. Added MCPServerSpec.RedactToolResultSecrets (redactToolResultSecrets in the CRD, default false) next to the existing trustProxyHeaders field, wired the same way, and regenerated the CRD manifests.
• VirtualMCPServer (path 3): VirtualMCPServerSpec.Config embeds pkg/vmcp/config.Config directly, and the converter (cmd/thv-operator/pkg/vmcpconfig/converter.go) DeepCopy()s it wholesale ("new fields added to config.Config are automatically included") -- so this one is wired end-to-end for free, no converter change needed. Just required regenerating the CRD manifests and CRD reference docs, which CI's generated-artifact checks caught as stale.

apiVersion: toolhive.stacklok.dev/v1beta1
kind: MCPServer
metadata:
  name: my-server
spec:
  transport: stdio   # or streamable-http / sse -- both proxy shapes are now covered
  redactToolResultSecrets: true

apiVersion: toolhive.stacklok.dev/v1beta1
kind: VirtualMCPServer
metadata:
  name: my-vmcp
spec:
  config:
    redactToolResultSecrets: true

Not covered by this PR (flagging for follow-up)

• MCPRemoteProxy CRD -- the transparent-proxy fix (path 2) applies to it at the code level (same HTTPTransport), but its CRD (mcpremoteproxy_types.go) doesn't yet expose redactToolResultSecrets. Same small change as the MCPServer wiring.
• connector-gateway's own composition root (enterprise/connector-gateway, a different repo -- stacklok-enterprise-platform) builds *vmcpserver.ServerConfig directly in assembly.go; it needs a one-line RedactToolResultSecrets: true addition once this PR merges and the toolhive dependency is bumped, to actually turn this on for connector-gateway. Not done here since it can't build against an unmerged toolhive change.
• dast/mcp-proxy harness update (also in stacklok-enterprise-platform) -- needs --redact-tool-result-secrets added to the adversarial suite's thvRunStateless call and checkCredentialExfiltration's assertion inverted, to actually observe path (2)'s fix in CI once the submodule 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.
• Binary tool-result content (ImageContent/AudioContent base64 data) is not scanned -- out of scope for a text-pattern matcher.
• CallToolResult.StructuredContent (the parallel machine-readable payload some tools return alongside Content) is not scanned anywhere in this PR -- only Content is. A backend that puts a credential in StructuredContent only (or duplicates it there) leaks it unredacted. Scanning it means walking an arbitrary JSON tree for string leaves rather than the fixed []Content shape; not done here.

Test plan

• go build ./... and go vet ./... clean across the whole repo
• gofmt -l clean on all touched files
• golangci-lint run -- 0 new issues on all touched packages (6 pre-existing unrelated staticcheck findings elsewhere in the operator package, untouched by this PR)
• pkg/mcp/secretscan/secretscan_test.go -- each credential pattern (including the new generic Bearer-token pattern) redacted, ordinary text untouched, malformed input fails open, empty input handled
• pkg/transport/proxy/streamable/secretscan_test.go -- redacts only when enabled, disabled-by-default verified, ignores other methods/error responses/non-Response messages
• pkg/transport/proxy/transparent/secret_redaction_response_processor_test.go -- redacts a Bearer <token>-carrying tools/call result for both the streamable-http (JSON and SSE-shaped response) and legacy sse transport types; disabled-by-default verified; split-across-data:-lines bypass covered for both transport types
• pkg/vmcp/server/secretscan_test.go -- redacts the Legacy/SDK dispatch path's result when enabled; disabled-by-default verified
• TestWithRedactToolResultSecrets in pkg/runner/config_builder_test.go, mirroring TestWithStrictProtocolValidation
• TestCreateRunConfigFromMCPServer_RedactToolResultSecrets in cmd/thv-operator/controllers/mcpserver_runconfig_test.go, verifying the CRD field flows into RunConfig
• pkg/vmcp/server's existing reflection-based "every Config/ServerConfig field must be populated by the derivation helpers" guard tests (TestBuildServeConfigMapsSharedFields, TestDeriveServerConfigMapsAllFields) updated and passing with the new field
• task operator-manifests / task crdref-gen regenerated and committed (MCPServer and VirtualMCPServer CRDs, docs/operator/crd-api.md) -- CI's generated-artifact checks are green
• task docs' CLI + swagger regen committed (docs/cli/thv_run.md, docs/server/*) -- CI's Verify Swagger Documentation check is green
• Full existing pkg/mcp/..., pkg/transport/..., pkg/runner/..., pkg/vmcp/..., pkg/workloads/upgrade/..., cmd/thv/app/..., cmd/thv-operator/api/..., cmd/thv-operator/controllers/... suites pass unmodified (pre-existing cmd/thv-operator/test-integration envtest failures are unrelated -- missing local kubebuilder/etcd binary, not caused by this change)
• All CI checks green on the PR (gh pr checks)

[codecov]

Codecov Report

❌ Patch coverage is 82.67327% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.86%. Comparing base (7f15a63) to head (5321a3b).

Files with missing lines	Patch %	Lines
...transparent/secret_redaction_response_processor.go	75.28%	22 Missing ⚠️
pkg/transport/proxy/streamable/streamable_proxy.go	76.47%	4 Missing ⚠️
...nsport/proxy/transparent/sse_response_processor.go	88.23%	4 Missing ⚠️
pkg/mcp/secretscan/secretscan.go	93.10%	2 Missing ⚠️
pkg/transport/stdio.go	50.00%	1 Missing ⚠️
pkg/vmcp/cli/serve.go	0.00%	1 Missing ⚠️
pkg/vmcp/server/modern_envelope.go	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6409      +/-   ##
==========================================
+ Coverage   77.85%   77.86%   +0.01%     
==========================================
  Files         760      762       +2     
  Lines       73043    73219     +176     
==========================================
+ Hits        56865    57014     +149     
- Misses      16173    16200      +27     
  Partials        5        5              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[aponcedeleonch]

Nice piece of work, and the plumbing is the part I'd have expected to go wrong and didn't. The activation path mirrors --strict-protocol-validation at every hop (RunFlags -> cobra BoolVar -> runner.WithX -> RunConfig -> types.Config -> Set/With option -> the carry line in applier.go), which is the repo's existing idiom for this class of feature. Reassembling SSE events before scanning is the right instinct too, and I'm glad it came with regression tests.

Two things I'd like to see before this merges, then smaller notes inline.

The scanner is bypassable on the legacy SSE path

processSSEStream still ends with the pre-existing io.Copy(pw, readCloser) tail-drain (sse_response_processor.go:352-358). The scanner caps a token at 1MB, so a backend that emits one data: line over that trips ErrTooLong, both loops exit, and every remaining byte gets copied to the client unscanned. Nothing reaches the client except a successful-looking stream.

That's a cheaper evasion than splitting a payload across data: lines, which this PR specifically defends against. The new SecretRedactionResponseProcessor.processSSE hits the same condition and fails the other way: it closes the pipe with a nil error, so the client sees a clean EOF on a truncated stream.

Related, and I think it's the root cause: there are now two independent implementations of buffer-data-lines-then-reassemble-then-redact, one per transport, each with its own test. The divergence above is what you'd predict from that. I'd rather see the reassembly live in one place with redaction as a hook.

Please make the scanner an interface

secretscan is currently a concrete package: exported package-level functions over a package-level var patterns, called directly from the four fire points. That makes redactToolResultSecrets mean "the regex list is on" rather than "redaction is on", and it means nobody can substitute a different detector without editing this package.

What I'd like is a Redactor interface with the pattern matcher registered as the default, so OSS behavior is byte-identical:

type Redactor interface {
    RedactToolResult(ctx context.Context, r *sdkmcp.CallToolResult) (bool, error)
}

var active Redactor = patternRedactor{}

func Register(r Redactor)  // for init() from a build-tagged overlay
func Active() Redactor

Three shape details that are cheap now and breaking later:

• Take the whole *CallToolResult, not []Content. You already flag StructuredContent as an uncovered gap, and closing it later would be an interface change.
• Return an error. The regex matcher can't fail; anything remote times out or trips a breaker. bool alone bakes fail-open in permanently, and some deployments will want fail-closed.
• Thread a ctx. Unused by the regex path, and it's the one thing you can't add later without touching every signature.

Plus a Redactor field on vmcpserver.ServerConfig and Config, so a consumer building its own composition root can inject one instead of reaching for a process-wide registry.

On scope: I'm not asking for a second implementation here, and the OSS default should stay the working regex scanner rather than an ErrEnterpriseRequired stub. This is a control OSS users should keep getting by default. It's roughly a hundred lines of new and moved code in one package, and it doesn't touch the CRD, chart, flag, RunConfig, or any of the regen, so most of this PR stands as written.

[aponcedeleonch]

Anchoring here because the lines I mean are just below and outside the hunk.

scanner.Err() at 352 logs and moves on, then io.Copy(pw, readCloser) at 357 forwards the rest of the body raw. With redaction enabled that's an unscanned passthrough.

The trigger is cheap. scanner.Buffer(make([]byte, 0, 1024), 1024*1024*1) sets maxTokenSize to 1MB, and bufio/scan.go does if len(s.buf) >= s.maxTokenSize { s.setErr(ErrTooLong) }. Per Scan's doc it "returns false when there are no more tokens, either by reaching the end of the input or an error", so one oversized data: line exits both loops and drops straight into the tail-drain. Everything after that point reaches the client untouched.

That pre-dates this PR and was fine when the processor only rewrote endpoint URLs. It isn't fine once the same function is the enforcement point for a security control. I think this needs pw.CloseWithError(scanner.Err()) so a hostile or just oversized stream fails visibly instead of degrading into passthrough.

[aponcedeleonch]

Same oversized-line condition as the legacy SSE path, opposite failure. scanner.Err() gets logged at 180 and then this pw.Close() closes the pipe with a nil error, so the client sees a clean EOF on a truncated stream and can't tell it apart from a normal end.

pw.CloseWithError(err) would surface it.

[aponcedeleonch]

This says the field is ONLY applicable when Transport is stdio and has no effect for streamable-http or sse, but the PR wires both of those through the transparent proxy, and factory.go sets httpTransport.redactToolResultSecrets for TransportTypeSSE and TransportTypeStreamableHTTP. Reads like text from an earlier iteration that didn't get updated when paths 2 and 3 landed.

Worth fixing because it's baked into both generated CRD files, both chart templates (twice each, two versions), and docs/operator/crd-api.md. So it's a customer-facing doc telling operators the field is inert exactly where it now works. The swagger description got it right, so it's just this comment plus regen.

[aponcedeleonch]

The streamable proxy gates on method != string(sdkmcp.MethodToolsCall) before scanning; this path scans any envelope carrying a result, whatever the method.

It works out today because only a tools/call result has a content array, so everything else decodes to empty Content and no-ops. But it means one knob has different scan scope depending on transport, and the safety rests on an SDK decode detail rather than on the check being there. I'd add the method filter for symmetry.

[aponcedeleonch]

Worth noting for the DAST harness follow-up: this requires a result key, so server-initiated notifications/message are never scanned. The adversarial fixture emits the sentinel in that shape as well as in the tools/call result, so it's worth checking per-transport before inverting checkCredentialExfiltration. The streamable-http variant should flip cleanly; I'm less sure about sse.

[aponcedeleonch]

This round-trips the result through CallToolResult, and on the version this PR pins (toolhive-core v0.0.41) that's lossy.

UnmarshalJSON reads _meta, content, structuredContent, isError, and:

// Capture resultType (SEP-2322 multi round-trip classification), if present.
if rt, ok := raw["resultType"].(string); ok {
    r.resultType = rt
}

resultType lands in an unexported field, and MarshalJSON builds its output map from only _meta, content, structuredContent, and isError. So resultType is dropped, along with anything else not in that set.

The part that makes it awkward to debug is that it only happens when a pattern matched. An unmatched result is forwarded as the original bytes, so the two paths aren't equivalent. Splicing the redacted content back into the original json.RawMessage would avoid it, though I realize that's more work than it sounds.

[aponcedeleonch]

The return value gets dropped here, and modern_envelope.go:430 does the same. The streamable proxy logs a Warn when it redacts; these two paths and the transparent processor are silent.

So an operator who turns this on has no way to tell whether it's ever fired on two of the three paths. Given this exists to answer a pen-test finding, "did it fire" seems like the first question anyone will ask. A counter would be better than a log line, but a log line would already be an improvement.

[aponcedeleonch]

This one's looser than the framing at the top of the list. I ran it, and Authorization: Bearer YOUR_TOKEN_HERE matches, so any tool that returns HTTP API documentation gets its examples rewritten. (Bearer <your-token-here> does not match, for what it's worth, so it depends on the placeholder style.) The package doc says the patterns are deliberately narrow rather than a generic heuristic, and this pattern's own comment concedes it's different in kind.

I'm not against keeping it, the DAST fixture is shaped exactly like this. But I'd either call it out as a known false-positive class in the flag help, or put it behind its own opt-in so the named-issuer patterns can stay high-confidence.

[aponcedeleonch]

Nit: the comment says the prefix and "at most one leading space" are stripped, but strings.TrimSpace removes "all leading and trailing white space, as defined by Unicode". No practical difference for JSON payloads, so it's the comment that's off rather than the code.

[aponcedeleonch]

Nit: extractSessionID still runs per raw line while redaction moved to the reassembled value. If a sessionId ever straddled two data: lines this would miss it, which is the same evasion flushDataBuf exists to close. It only feeds the proxy's own tracking rather than anything security-relevant, so low stakes, but the asymmetry stood out.