Validate upstream JSON-RPC responses in transparent proxy#5288
Validate upstream JSON-RPC responses in transparent proxy#5288bishnubista wants to merge 8 commits into
Conversation
The transparent proxy forwarded malformed upstream MCP frames to clients with HTTP 200 even when the response violated JSON-RPC 2.0 structure. This adds a boundary check in NoOpResponseProcessor that rejects structurally invalid upstream frames and returns a synthetic 502 carrying a JSON-RPC error to the client, so the proxy stops being a silent amplifier for malformed (or adversarial) upstream servers. Validation runs only for streamable-http POST/200 responses that carry an MCP request signal (MCP-Protocol-Version or Mcp-Session-Id) and an application/json content type, with non-identity Content-Encoding traffic passed through untouched. Body reads are bounded to 100 MiB to match existing streamable-HTTP limits in pkg/vmcp. Rewritten error responses replace headers wholesale so upstream session/cookie/cache metadata is not smuggled into the proxy-generated error. SSE traffic is unaffected. Closes stacklok#5247 Signed-off-by: bishnubista <bista.developer@gmail.com>
4fcdb4b to
a7badd9
Compare
…-jsonrpc-responses
Remove the top-level json.Decoder.More check from upstream JSON-RPC response validation. More is intended for array and object iteration, so using it as a top-level single-value guard can miss malformed trailing delimiter bytes. Keep the second Decode call as the exact-single-value check and compare its result with io.EOF via errors.Is. This rejects trailing JSON values and trailing syntax junk while accepting clean EOF after the first decoded response. Add a regression case for a valid JSON-RPC response followed by a stray closing delimiter so malformed trailing bytes cannot pass validation.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5288 +/- ##
==========================================
+ Coverage 70.78% 70.82% +0.03%
==========================================
Files 685 685
Lines 69435 69543 +108
==========================================
+ Hits 49150 49251 +101
+ Misses 16692 16687 -5
- Partials 3593 3605 +12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
We have middleware that does the validation and parsing. This happens outside of the transparent proxy, but still in the path. Why not use the existing middleware instead? |
Move strict JSON-RPC response body validation into pkg/mcp so the transparent proxy reuses shared MCP parsing code instead of carrying a private copy. Use parsed MCP request context from the parser middleware as the primary signal for response validation, while preserving protocol/session header fallback for batch and compatibility cases that the request parser does not currently cover. Keep the transparent proxy's ModifyResponse path responsible for rejecting malformed upstream responses with the existing bounded read and 502 rewrite.
…-jsonrpc-responses
Good point. I agree the JSON-RPC validation logic should not live as bespoke protocol code in I traced the path again: the existing MCP parser middleware runs before the transparent proxy and parses inbound requests into context, but I didn’t find an existing generic response validator that can inspect upstream response bodies and rewrite them before they reach the client. The response-side parsing I found is authz-specific or audit-only/lenient. I’ll adjust this by moving the response validation helper into |
github-actions
Bot
added
size/L
aponcedeleonch
left a comment
aponcedeleonch
left a comment
There was a problem hiding this comment.
Thanks, this is a well-scoped security fix. Process note for next time: please ask to have the issue assigned to you before opening a PR, per our contributor guidelines. #5247 is the linked issue.
On the change: I confirmed both halves. On main the transparent proxy's response hook is a literal no-op invoked unconditionally, so malformed upstream frames reach clients as 200s. This PR adds strict JSON-RPC 2.0 validation at the streamable-http boundary and rewrites invalid frames into a bounded, header-scrubbed 502, gated conservatively so it won't false-reject SSE, compressed bodies, or non-MCP JSON. Bodies are forwarded byte-for-byte, so no int-precision or re-serialization risk. Builds, passes the touched packages plus full-repo lint, and the test matrix is thorough.
Reconciling with the earlier review: JAORMX's point about reusing existing validation instead of bespoke protocol code is addressed. The helper now lives in pkg/mcp (ValidateJSONRPCResponseBody) and reuses the parsed-request signal. amirejaz's dead-branch nit is also gone: the old dec.More() construct was replaced with a reachable single-value guard that's now tested.
Approving. Left a couple of non-blocking nits inline. One follow-up ask that isn't code: amirejaz suggested opening an issue to track the deferred SSE-response validation gap (an SSE stream can't be rewritten to 502 mid-response), worth opening so the gap stays explicit.
| // Used for transports that don't require response processing (e.g., streamable-http). | ||
| // NoOpResponseProcessor is the default processor for non-SSE transports. | ||
| // It validates JSON-RPC responses for streamable HTTP and otherwise leaves responses unchanged. | ||
| type NoOpResponseProcessor struct{} |
There was a problem hiding this comment.
Non-blocking nit: this type is no longer a no-op (the doc comment even says it validates now), and ShouldProcess still returns false while ProcessResponse does real work. That's safe today only because modifyResponse calls ProcessResponse directly, but a future caller that gates on ShouldProcess would silently skip validation. Worth renaming (e.g. StreamableHTTPResponseProcessor) and making ShouldProcess reflect reality.
| } | ||
|
|
||
| // Read one byte past the cap so we can detect oversize without allocating beyond it. | ||
| body, err := io.ReadAll(io.LimitReader(resp.Body, maxJSONRPCResponseBytes+1)) |
There was a problem hiding this comment.
Non-blocking: this buffers the entire response body (up to 100 MiB) and parses it on every streamable-http response, replacing the previously streamed path. Fine for typical MCP payloads and the cap bounds memory, but large tool outputs now incur a full buffer plus parse. Worth a comment here or a follow-up if latency on big results ever matters.
aponcedeleonch
left a comment
aponcedeleonch
left a comment
There was a problem hiding this comment.
Upon further review the CI is failing, we should fix it before merging
4 participants
Summary
The transparent proxy (
thv run) currently forwards upstream MCP server responses to clients with HTTP 200 even when the JSON-RPC frame is structurally invalid — missingjsonrpc:"2.0", invalididtype, non-object body, etc. The proxy sits between potentially untrusted upstream MCP servers and trusted clients, so a compromised or misconfigured upstream can push malformed frames straight through, where they may crash strict JSON-RPC parsers, be misinterpreted as legitimate responses, or trigger undefined behaviour in client-side parsers.This PR validates upstream JSON-RPC frames at the proxy boundary and rewrites invalid responses into a structured JSON-RPC error so the proxy stops being a silent amplifier for malformed (or adversarial) upstream servers.
NoOpResponseProcessor.ProcessResponse(streamable-http/ default transport). SSE is unaffected.jsonrpc=="2.0",id∈ {string, number, null}, exactly one ofresult/error;error.codemust be an integer,error.messagemust be a string.POST+200,application/json(-rpc) media type, non-identityContent-Encodingpassed through untouched, request must carry an MCP streamable-HTTP signal (MCP-Protocol-VersionorMcp-Session-Id) so non-MCP JSON traffic flowing through the catch-all is not rewritten.{"jsonrpc":"2.0","id":null,"error":{"code":-32000,"message":"Invalid upstream JSON-RPC response","data":"<detail>"}}.-32000is the JSON-RPC implementation-defined server-error code;-32603is reserved for internal JSON-RPC errors. Response headers are replaced wholesale so upstreamMcp-Session-Id,Set-Cookie,ETag,Cache-Control, etc. are not smuggled into the proxy-generated error.io.LimitReaderatmaxJSONRPCResponseBytes = 100 << 20(100 MiB), matching streamable-HTTP precedent inpkg/vmcp/clientandpkg/vmcp/session/internal/backend. Oversized responses are rejected with the same 502 shape so the proxy cannot be amplified into a memory DoS.Closes #5247
Type of change
Test plan
task test)task test-e2e)task lint-fix)Local verification:
go test ./pkg/transport/proxy/transparent/... -count=1— pass.golangci-lint run --timeout=5m ./pkg/transport/proxy/transparent/...— 0 issues.New test cases in
pkg/transport/proxy/transparent/response_processor_test.go:result: null,application/json; charset=utf-8.jsonrpc, invalididtype, non-object body,result+errorboth present, trailing JSON, fractionalerror.code.text/event-stream,application/jsonsomethingelse.Content-Encoding: gzip) passed through unchanged for both valid and malformed payloads; explicitContent-Encoding: identitystill validates.MCP-Protocol-VersionorMcp-Session-Id→ validate.Mcp-Session-Id,Set-Cookie,Etag,Cache-Controlfrom the response.maxJSONRPCResponseBytes) is rejected with a size-limit error.API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.No CRD or operator API surface is touched.
Does this introduce a user-facing change?
Yes. Clients of the transparent proxy that previously received malformed upstream responses verbatim with HTTP 200 will now receive HTTP 502 with a synthetic JSON-RPC error body. Conformant upstream MCP servers are unaffected — only structurally invalid responses are rewritten.
Special notes for reviewers
Two scope decisions called out explicitly so reviewers can push back if either is wrong for the project:
Validation runs in
ModifyResponse, aftertracingTransport.RoundTripmay have observed an upstreamMcp-Session-Idand registered proxy-side session state. This PR strips upstream session headers from the proxy-generated 502 so clients never receive a session id derived from a malformed initialize response, but it does not roll back server-side proxy session state created before validation. Moving validation earlier (or adding a rollback path for invalid initialize responses) touchestracingTransportand felt outside the scope of "structurally validate the upstream frame." Happy to open a follow-up — or fold it into this PR if maintainers prefer.Backward-compat clients that POST without
MCP-Protocol-Versionon the very first initialize will not trigger validation — same as today's behaviour, so no regression. If broader coverage is preferred,tracingTransport.RoundTripalready buffers and parses the request body to detectinitialize; a small context-propagated marker would letProcessResponsevalidate those frames too. Easy follow-up.Open to feedback on:
pkg/vmcpprecedent; can tighten to 10 MiB if maintainers prefer an explicit security default).