Skip to content

fix: handle unframed JSON body with text/event-stream content type in MCP proxy#2229

Open
RegondaChandan wants to merge 7 commits into
envoyproxy:mainfrom
RegondaChandan:fix/mcpproxy-unframed-json-sse
Open

fix: handle unframed JSON body with text/event-stream content type in MCP proxy#2229
RegondaChandan wants to merge 7 commits into
envoyproxy:mainfrom
RegondaChandan:fix/mcpproxy-unframed-json-sse

Conversation

@RegondaChandan

Copy link
Copy Markdown

Description

The MCP proxy parses upstream response bodies strictly according to the advertised
Content-Type. When a backend responds with Content-Type: text/event-stream but a
plain, unframed JSON body (no event:/data: lines), the SSE parser finds no events
and the JSON-RPC payload is silently dropped: tools/list returns {"tools":[]},
the session initializes fine, and no error is surfaced. Such backends exist in the
wild (e.g. api.refero.design's hosted MCP server).

This is the mirror image of the existing fallback for backends that send SSE data
despite Content-Type: application/json (e.g. Slack MCP). For responses advertising
SSE, the proxy now peeks at the first significant byte of the stream: a valid SSE
stream never starts with {, so in that case the body is read in full and decoded
as a single JSON-RPC message, the response Content-Type is corrected to
application/json, and the message goes through the same modification/write path
as the JSON branch (which is extracted into a shared helper). If the body turns out
not to be valid JSON-RPC, it still falls through to the SSE parser over the buffered
bytes.

Genuine SSE streaming is unaffected: detection only consumes leading whitespace via
a buffered peek, and the buffered reader is handed to the SSE parser as before.

Related Issues/PRs (if applicable)

Fixes #2218

Special notes for reviewers (if applicable)

The three new regression tests fail on main (the unframed JSON payload is dropped
and the recorded body is empty) and pass with this change.

… MCP proxy

Signed-off-by: RegondaChandan <sairegonda9@gmail.com>
@RegondaChandan
RegondaChandan requested a review from a team as a code owner June 12, 2026 18:17
@dosubot dosubot Bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jun 12, 2026

@Hritik003 Hritik003 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this #1997 missed the case you are addressing in the PR, thanks for fixing it

Comment on lines +827 to +834

// startsWithJSONObject reports whether the stream begins with '{', consuming only
// leading whitespace, which is insignificant to both SSE and JSON parsing.
func startsWithJSONObject(br *bufio.Reader) bool {
for {
b, err := br.ReadByte()
if err != nil {
return false

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use Peek here? Peek(n) returns a view of the upcoming bytes without advancing the stream.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, switched the BOM check over to Peek. i left the whitespace skip
as a byte scan on purpose though, since this branch also handles real SSE
streaming. Peek blocks until it has the full count buffered (or hits EOF), so
peeking a big window could hold back the first event from flushing. just
peeking the 3 BOM bytes keeps that path cheap. can make the whole thing a
single Peek if you'd rather.

Comment on lines -791 to +852
var sseReader io.Reader = resp.Body
var sseReader io.Reader

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you explain this change?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure. the = resp.Body was just a default for sseReader. once i moved the
json write path out into writeSingleJSONRPCMessage, both branches set sseReader
themselves now (json branch sets bytes.NewReader(body) or returns early, the
event-stream branch sets the bufio reader or returns early), so resp.Body never
gets read unwrapped anymore. that left the initial value dead so i dropped it.

@Hritik003

Hritik003 commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

just thinking out loud,

Intext/event-stream path, it first calls startsWithJSONObject, which only recognizes ASCII whitespace and {. But when the first byte is 0xEF (the BOM), which is neither—it returns false and then the message is treated as SSE, handed to the SSE parser, and effectively dropped. It's a minor edge case (only matters for backends that both mislabel the content type and emit a BOM). Can add a one liner which trims the BOM prefix before reading

func startsWithJSONObject(br *bufio.Reader) bool {
	buf, _ := br.Peek(br.Size())
	buf = bytes.TrimPrefix(buf, utf8BOM) < ---add this snippet?
	for _, b := range buf {
	.......
	}
	return false
}

RegondaChandan and others added 2 commits June 16, 2026 18:02
startsWithJSONObject treated a leading UTF-8 BOM as a non-JSON byte, so a
BOM-prefixed unframed JSON body advertised as text/event-stream was handed
to the SSE parser and silently dropped. Skip the BOM before scanning; the
decode path (tryDecodeJSONRPCMessage) already strips it.

Signed-off-by: RegondaChandan <sairegonda9@gmail.com>
@RegondaChandan

Copy link
Copy Markdown
Author

@Hritik003 nice catch, fixed in d56acce. startsWithJSONObject skips a leading BOM now before scanning, so a BOM-prefixed body doesn't get handed to the SSE parser and dropped. tryDecodeJSONRPCMessage already strips the BOM on the decode side so it was really just the detection missing it. added a regression test too.

@Hritik003

Copy link
Copy Markdown
Contributor

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the MCP proxy response handling to gracefully handle backends that advertise a text/event-stream Content-Type but send an unframed, plain JSON-RPC message instead. It introduces a helper startsWithJSONObject to peek at the stream (ignoring UTF-8 BOM and leading whitespace) and handles valid JSON-RPC messages as JSON. Additionally, comprehensive unit tests have been added to verify this behavior under various conditions, including leading whitespace, BOM, and invalid JSON fallbacks. There are no review comments, so I have no feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@Hritik003

Copy link
Copy Markdown
Contributor

/retest

@Hritik003 Hritik003 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@missBerg missBerg added the area/mcp MCP proxy, MCPRoute, and MCP spec conformance label Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/mcp MCP proxy, MCPRoute, and MCP spec conformance size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MCP proxy drops tools/list payload when backend sends Content-Type: text/event-stream with an unframed JSON body

3 participants