Parse SSE responses per event, not per line

[jhrozek]

Summary

processSSEResponse processed a buffered text/event-stream body line by line. SSE semantics are per event: within one event all data: field values concatenate, and a blank line is structural. Processing lines meant we parsed the body differently than the client does — and every such disagreement is a way for an upstream MCP server to smuggle an unfiltered tool list past the filter (#5257). Five were reachable. Four were reproduced leaking before the fix and verified filtered after; the second below did not leak beforehand — the per-line loop filtered that line independently, so it is a hazard this rewrite creates and then closes:

• A payload split across two data: lines. Each half failed to decode on its own, so both passed through and the client reassembled the full list.
• Two JSON-RPC messages as two data: fields of one event. Assembling produced two concatenated values, which json.Unmarshal rejects as trailing data, so the result-detector missed it. This one did not leak on main — it is created and closed within this PR.
• Mixed line terminators. SSE permits CR, LF and CRLF interchangeably within one stream; sniffing one convention for the whole body left entire events buried inside what we treated as a single line.
• A leading UTF-8 BOM. Clients strip it per the WHATWG decode algorithm before parsing lines, so our data: prefix match failed where theirs succeeded.
• A Response carrying both error and result. filterListResponse returned early on any error and the result went out untouched. The reference client's schema strips the stray error and accepts the result. Transport-independent — this one affected the JSON path too.

The fix scans (line, terminator) pairs and groups them into events, assembles each event's data payload, then decodes and filters once. Every line is re-emitted with the terminator it was scanned with, so the body is reproduced structurally and the old separator reconciliation — which wrote back at most one separator however many it dropped, collapsing multi-event bodies — is deleted rather than patched.

A frame that fails closed now carries a correlatable JSON-RPC error instead of an empty data buffer, which per WHATWG is never dispatched and left the client hanging on its own timeout.

Refs #6037

Type of change

• Bug fix
• New feature
• Refactoring (no behavior change)
• Dependency update
• Documentation
• Other (describe):

Test plan

• Unit tests (task test)
• E2E tests (task test-e2e)
• Linting (task lint-fix)
• Manual testing (describe below)

go test -count=1 ./pkg/authz/... green. task test panics locally on a gotestfmt bug that reproduces on a clean tree, so the underlying go test was used, always with -count=1 after the cache masked a result once.

Each of the five bypasses has a dedicated regression test asserting on real wire bytes, and each was proven to fail against the pre-fix code. Beyond that:

• A structural round-trip test pins byte-identity across separator and trailing shapes, including a lone trailing CR that guards a bounds check.
• Independently verified: all six attack shapes filtered, the authorized tool still present, and the round trip byte-exact across 36 permutations of {CR, LF, CRLF}² × {none, LF, CRLF, CR}.

Linting not run locally, per a standing preference; leaving it to CI.

Does this introduce a user-facing change?

Yes, though it is a security fix rather than a feature. Tool, prompt and resource lists that an upstream MCP server could previously smuggle past authorization filtering — by framing the response in ways a client parses differently than the proxy did — are now filtered correctly.

Special notes for reviewers

The security invariant, worded carefully. At the SSE framing layer, every payload a strict client can parse and correlate as this request's Response is now assembled and decoded before filtering. The residual pass-through paths carry only payloads no strict parser accepts: a split inside a string literal leaves a raw control character in a JSON string, which the client's own parser rejects. So the gap is closed by construction rather than by luck — measured both ways.

This deliberately does not claim more. It is not true of the per-method filters underneath, which still fall back to unfiltered pass-through on a result shape they cannot unmarshal. That is pre-existing and out of scope here.

Two deliberate non-recursions / non-fixes, both load-bearing:

• carriesResult walks top-level JSON values but bails at the first undecodable one, and the batch probe scans one level without recursing. Recursing was implemented and then backed out: it is O(d²) against encoding/json's own 10000 nesting cap, an easy amplification handle from the same adversarial upstream, bought to protect against nested-batch flattening no client performs. Tests pin both false results so neither gets "fixed" into a regression.
• A trailing event with no closing blank line gets no fabricated terminator. The consequence is recorded in a comment and pinned by a test.

Two disclosed behaviour changes. An upstream legitimately packing two JSON-RPC messages into one SSE event now receives a fail-closed error envelope rather than a filtered list; that shape is off-spec for MCP, so failing closed is right, but it is a change. And a Response carrying error together with an explicit "result":null is now treated as the both-fields protocol violation, so its code and message are replaced by our envelope — the fail-closed direction, and objectCarriesResult treats a null result as carrying one deliberately, with a test row pinning it.

Rebase note. #6066 landed on main mid-review and scrubs the client-visible error text, since it can name tools the caller is not authorized to see. The conflict resolution keeps that scrub and moves it into the one place both transports build their error body, so neither can leak it — a structural guarantee rather than a convention.

Review round applied. Two confirmed regressions this rewrite introduced are fixed in the final commit: leading non-JSON Unicode whitespace (U+000B, U+000C, U+0085, U+00A0, U+1680, U+3000) bypassing the filter because Go's JSON scanner and unicode.IsSpace disagree about what whitespace is, and the loss of the per-line safety net when one malformed data: value made a whole event skip filtering. Both reproduced before fixing and revert-proven after. Also added, from the reviewer: a strict WHATWG-grammar SSE parser so error-path assertions test delivery rather than bytes, and the fail-closed invariant for the remainder of the stream after a mid-stream failure.

Not folded in: #6092's sseCarriesResult remains line-based inside this now-per-event file. It only decides whether to filter and errs toward filtering, so it is a readability inconsistency rather than a correctness one, and its author offered to leave it.

Stacked on #6087; review that first.

Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 96.26168% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.43%. Comparing base (f805322) to head (fefd7b4).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/authz/response_filter.go	96.26%	1 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6088      +/-   ##
==========================================
+ Coverage   72.34%   72.43%   +0.08%     
==========================================
  Files         731      733       +2     
  Lines       75748    75853     +105     
==========================================
+ Hits        54799    54943     +144     
+ Misses      17047    17018      -29     
+ Partials     3902     3892      -10     

☔ 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.

[JAORMX]

Reviewed with the security posture first, since this is the authorization filter and the threat actor is the MCP server behind it.

Short version: this is a clear improvement on what's in main, and it fixes more than the description claims — see §4. It also introduces one bypass that I think needs closing before it lands, which is why I'm leading with that rather than because the PR is weak.

Everything marked CONFIRMED I reproduced by running code against this branch, fix-sse-filter-error-framing and origin/main. PLAUSIBLE is reasoning only.

---

1. Leading non-JSON Unicode whitespace bypasses the filter — CONFIRMED, must-fix

pkg/authz/response_filter.go:294 (assembly) with :399 (carriesResult).

The carriesResult rewrite moved bytes.TrimSpace inside valueCarriesResult, where it only runs on values json.Decoder has already accepted. encoding/json's scanner treats only SP, TAB, CR and LF as whitespace; bytes.TrimSpace / unicode.IsSpace also strip \v, \f, U+0085, U+00A0, U+1680 and U+3000. So for an event whose assembled payload begins with one of those:

• jsonrpc2.DecodeMessage fails on the first byte → not the isResponse branch
• carriesResult fails on the same byte → not the fail-closed branch
• control reaches :357 case decodeErr != nil → the event's lines are forwarded verbatim, unfiltered

Meanwhile the go-sdk client parses that frame fine, because its own SSE scanner does exactly

case bytes.Equal(before, dataKey):
    data := bytes.TrimSpace(after)

— modelcontextprotocol/go-sdk mcp/event.go:139-146.

Scenario. Upstream answers tools/list with data: \v{"jsonrpc":"2.0","id":1,"result":{…full unfiltered list…}}\n\n. The proxy forwards it; a go-sdk client strips the \v and receives every tool, including the ones Cedar denies.

leading byte	main	#6087	this PR	go-sdk client accepts
\v \f U+0085 U+00A0 U+1680 U+3000	filtered	filtered	leaks	yes

This is a regression introduced here rather than a pre-existing gap: main and #6087 catch all six, because the old carriesResult ran bytes.TrimSpace(data) before json.Unmarshal. It's the same disagreement-with-the-client class the PR exists to close, just relocated.

Fix, verified — one line at :294:

assembled := bytes.TrimSpace(bytes.Join(dataValues, []byte("\n")))

This aligns assembly with the client's own normalisation. All six then come out properly filtered rather than merely dropped — weather survives, admin_tool gone — and the whole pkg/authz suite stays green. The mirror case (trailing \v/\f/NBSP) is already caught, because the decoder consumes the value before hitting it, so the gap is specifically leading bytes.

A strict WHATWG client is unaffected: JSON.parse rejects U+000B et al., since the JSON grammar's whitespace is only TAB/LF/CR/SP. That part is PLAUSIBLE — reasoned from the grammar, not executed — so the exposure I can demonstrate is Go clients.

A regression test is offered at the bottom of this review.

2. Per-event assembly gives up the per-line safety net — CONFIRMED, must-fix or disclose

response_filter.go:294-303 with :357, and carriesResult's stop-at-first-bad-value at :399.

One malformed data: value anywhere in an event now makes the whole event skip filtering.

Scenario. data: garbage followed by data: {"jsonrpc":"2.0","id":1,"result":{…list with admin_tool…}} inside one event (no blank line between). Assembled: garbage\n{…}. The decoder stops at garbage, carriesResult returns false, and both lines are forwarded unfiltered. On main and #6087 the second line was filtered independently. 9 of the 10 trivial prefixes I tried work (garbage, //comment, {, }, [, NaN, +1, 'quoted', an embedded NUL); only 01 fails.

To be fair to the code as written: TestCarriesResult pins this with a "don't fix this into scanning past bad input" comment, and its reasoning — no strict client can read past the bad value — is correct. My concern is narrower than the reasoning: it justifies not catching the result, whereas the consequence is forwarding the unfiltered list verbatim. That's a strictly weaker posture than the per-line loop it replaces, and it puts denied tool names in plaintext on the wire. The "Special notes" invariant is true as worded, but it doesn't surface that the rewrite trades away coverage main has.

Fix, verified: when the assembled payload as a whole is undecodable, probe each LF-separated data value on its own before passing through — linear, no recursion, no new amplification handle, and it leaves carriesResult itself (and its test) untouched. All ten prefixes then fail closed with the suite green.

Equally acceptable, and entirely your call: leave the behaviour and say so explicitly in the body and in the comment at :357. Right now the comment reads as "this is fine" rather than "we accept losing coverage here", which is the bit a future reader needs.

3. Vector 2 didn't leak before the fix — body only

Minor and worth one line: the second bullet's shape (two JSON-RPC messages as two data: fields of one event) measures FILTERED on both main and #6087 — the per-line loop filtered the second line independently. It's a hazard the event-based rewrite creates and then closes, which is exactly what TestResponseFilteringWriter_SSE_ConcatenatedEventBypass's doc comment says. Only the summary's "Each one was reproduced leaking before the fix" overclaims. The other four genuinely leaked.

4. Checked and found sound

The five vectors, each reproduced against main, #6087 and this branch on real wire bytes:

#	vector	leaked pre-fix	closed here
1	payload split across two data: lines	yes	yes, and correctly filtered
2	two messages as two data: fields	no (see §3)	yes, fails closed with a correlatable envelope
3	mixed CR / LF / CRLF terminators	yes	yes, filtered
4	leading UTF-8 BOM	yes	yes, filtered, and the BOM is dropped rather than re-emitted
5	Response carrying both error and result	yes, on SSE and JSON	yes on both paths

Fail-closed posture preserved. With a body of failing-event, good-event, failing-event: neither failing event's payload reaches the wire, the good event is still filtered and delivered, two correlatable error envelopes go out, and all three event boundaries survive. Worth noting that the new error+result check makes the filter-failure path reachable for the first time, so SSE_ErrorAndResultBypass genuinely exercises the #5257 drop rather than testing cosmetics — that property had no coverage anywhere before this PR.

Every input path reaches a decision. I walked and tested events with no data: field, event: / id: / retry: / comment (:) lines, data: with no space, tab after colon, bare data, single-space "blank" lines, consecutive blank lines, blank-lines-only bodies, lone trailing CR, CR-only bodies, and an unterminated final event (filtered, no fabricated terminator). All correct. The idx+1 < len(rawResponse) bounds check at :221 holds for a lone trailing CR, and bytes.Join(dataValues, "\n") matches WHATWG's "append value then LF, strip the final LF" exactly.

Residual pass-throughs that are not new and that I could not turn into a leak: mid-payload BOM, data: data: {…}, DATA:, data :, and nested batch [[{…result…}]]. All byte-identical to main, and in each case a conformant client either ignores the field or can't parse the buffer. The data : comment is precisely right, by the way — I checked it against the go-sdk's bytes.Cut(line, ':') + bytes.Equal(before, dataKey), and data really does miss.

Things this PR fixes without claiming them, which is the part I'd most want a second reader to notice:

• main corrupts every multi-event SSE body. Two events in, one event with two concatenated data: fields out — the blank line between them is eaten by the old separator reconciliation. The client assembles {notif}\n{response}, can't parse it, and loses the tool list entirely. Any server that emits a progress notification before its tools/list response hits this today. Fixed here and pinned by SSE_MultiEventBody.
• main writes nothing at all for a body with no line separator — unsupported SSE line separator in N-byte response, client gets an empty 200. This branch reproduces such a body byte-exactly.
• The reachable half of #6037 is fixed here, not in #6087: on main the carriesResult drop path emits a bare separator and no data, which I measured as a full response of literally "\n\n".

Test quality. Assertions are on real wire bytes and additionally decode the fail-closed envelope to check the id correlates — that's the difference between "didn't leak" and "didn't leak and didn't hang", and it's the right assertion. SSE_FilteredEventTerminators exists specifically to cover a hole SSE_StructuralRoundTrip leaves open (a hardcoded "\n" in the replacement branch would otherwise pass the entire suite); that's careful self-review. Deliberate behaviours carry "don't fix this into X" comments that will actually stop a future regression. errorResponseBody being the single place both transports build their body is a structural guarantee rather than a convention, as the rebase note says. Production diff is 340 lines in one file, inside the 400-line guidance despite the +905 headline.

5. Nits

• :213 — outputLines buffers the body a second time as slice headers: 48 bytes per line, against 24 for the old bytes.Split. Measured 200k blank lines → ~9.6 MB of headers for 200 KB of body, and the line count is upstream-controlled. Pre-existing in kind, doubled here; only worth a bound if you care. My taste, not a documented convention.
• :246/:249 — two Write calls per line, including Write(nil) for every blank line's text. Harmless behind bufio; the term write could be skipped when nil. Taste.
• filterListResponse:465 — error present together with "result":null is now a violation, so a legitimate upstream error carrying "result":null has its code and message replaced by our internal-error envelope. Fail-closed direction and defensible; a !bytes.Equal(result, []byte("null")) refinement or a comment would avoid surprising that upstream.
• Undisclosed behaviour change: an upstream legitimately packing two messages into one event now gets a hard error envelope instead of a filtered list. Off-spec for MCP, so failing closed is right — just worth a line in the body.

6. Verdict

needs-work → merge once §1 is fixed (one line, verified) and §2 is either fixed or explicitly disclosed. §1 is the only thing I'd actually block on; everything else is sound, and the rewrite is better reasoned than the code it replaces.

On sequencing: I'd land this together with #6087 rather than separately. I byte-compared #6087 against origin/main across a 25-shape matrix and the multi-event framing cases and it is identical on every reachable body — so it's safe alone, but it delivers no reachable behaviour change either, and the mis-framing its comment flags is already on main rather than introduced by it. All the value is in this PR: four real leaks and two functional bugs. (#6087 also currently doesn't compile — details in a comment over there.)

7. What I did not verify

• TS-SDK client exploitability for §1 and for the residual pass-throughs is reasoned from the WHATWG event-stream grammar and the JSON whitespace production, not executed. The go-sdk side is executed.
• I tested at the ResponseFilteringWriter seam with httptest, not end-to-end through a real proxy and MCP server.
• pkg/mcp/tool_filter.go — which the comment at :202 says this routine was adapted from — almost certainly carries the same class of bug and I did not review it. Out of scope here, but probably worth its own pass.
• I did not run task lint / task test; I used go test ./pkg/authz/ directly. CI is green on this branch, so lint is covered.

Offered regression test for §1

Adapted to this branch's helpers and style. I ran it here: it fails on 6 of 7 cases against this branch as-is (the no-prefix baseline passes) and passes with the one-line :294 fix, with the rest of pkg/authz green. Take it, rewrite it, or drop it — a permanent guard seemed worth more than the prose above.

Each case asserts the authorized tool survives, deliberately: a NotContains check alone would also be satisfied by failing closed, which would silently degrade every legitimate response that happens to carry leading whitespace.

pkg/authz/response_filter_test.go

// TestResponseFilteringWriter_SSE_LeadingUnicodeSpaceBypass is a regression
// test for a #5257-class leak: the filter and the client must agree on where
// an event's data payload starts.
//
// bytes.TrimSpace (and therefore unicode.IsSpace) treats \v, \f, U+0085,
// U+00A0, U+1680 and U+3000 as whitespace; encoding/json's scanner accepts
// only SP, TAB, CR and LF. So a payload whose first byte is one of those is
// undecodable to us -- filterSSEEventData's decodeErr branch passes the event
// through unfiltered -- while the go-sdk client strips it and parses the frame
// fine, because its own SSE scanner does exactly
//
//	data := bytes.TrimSpace(after)
//
// (modelcontextprotocol/go-sdk mcp/event.go:139-146). carriesResult doesn't
// catch it either: json.Decoder fails on the same first byte, and the
// bytes.TrimSpace inside valueCarriesResult only runs on values the decoder
// already accepted.
//
// The fix is to normalise the assembled payload the same way the client does,
// so these are filtered rather than forwarded -- which is why each case
// asserts the authorized tool SURVIVES: merely failing closed would also
// satisfy a NotContains check, and would silently degrade every legitimate
// response that happens to carry leading whitespace.
func TestResponseFilteringWriter_SSE_LeadingUnicodeSpaceBypass(t *testing.T) {
	t.Parallel()

	authorizer := newWeatherOnlyAuthorizer(t)

	resultJSON, err := json.Marshal(mcp.ListToolsResult{
		Tools: []mcp.Tool{
			{Name: "weather", Description: "Get weather information"},
			{Name: "admin_tool", Description: "Sensitive admin operations"},
		},
	})
	require.NoError(t, err)
	encoded, err := jsonrpc2.EncodeMessage(&jsonrpc2.Response{
		ID:     jsonrpc2.Int64ID(1),
		Result: json.RawMessage(resultJSON),
	})
	require.NoError(t, err)

	testCases := []struct {
		name   string
		prefix string
	}{
		{name: "no prefix (baseline)", prefix: ""},
		{name: "vertical tab U+000B", prefix: "\v"},
		{name: "form feed U+000C", prefix: "\f"},
		{name: "next line U+0085", prefix: "�"},
		{name: "no-break space U+00A0", prefix: " "},
		{name: "ogham space mark U+1680", prefix: " "},
		{name: "ideographic space U+3000", prefix: "　"},
	}

	for _, tc := range testCases {
		t.Run(tc.name, func(t *testing.T) {
			t.Parallel()

			req := newUser1Request(t)
			rr := httptest.NewRecorder()
			rfw := NewResponseFilteringWriter(rr, authorizer, req, string(mcp.MethodToolsList), nil, nil)
			rfw.ResponseWriter.Header().Set("Content-Type", "text/event-stream")

			_, err := rfw.Write([]byte("data: " + tc.prefix + string(encoded) + "\n\n"))
			require.NoError(t, err)
			require.NoError(t, rfw.FlushAndFilter())

			out := rr.Body.String()
			assert.NotContains(t, out, "admin_tool",
				"a leading byte that bytes.TrimSpace strips but encoding/json rejects must not bypass the filter")
			assert.Contains(t, out, "weather",
				"the event must be filtered, not merely failed closed: the authorized tool must survive")

			// The replacement payload must be a frame the client can decode.
			// Asserting on the wire bytes rather than on substrings pins that
			// the prefix was normalised away, not merely tolerated.
			firstLine := strings.TrimSuffix(strings.SplitN(out, "\n", 2)[0], "\r")
			msg, err := jsonrpc2.DecodeMessage([]byte(strings.TrimPrefix(firstLine, "data: ")))
			require.NoError(t, err, "the emitted frame must itself be valid JSON-RPC")
			resp, ok := msg.(*jsonrpc2.Response)
			require.True(t, ok, "the emitted frame must be a clean Response")
			assert.Equal(t, jsonrpc2.Int64ID(1), resp.ID,
				"the upstream response id must be preserved")
		})
	}
}

[jhrozek]

Both must-fixes are in, each reproduced before fixing and revert-proven after. Pushed as a separate commit rather than amended, so the delta against what you reviewed is visible.

§1 — leading non-JSON whitespace: CONFIRMED, fixed

Reproduced exactly as you described. All six prefixes leaked admin_tool under a weather-only policy; the no-prefix baseline filtered correctly:

none (baseline)      leaks_admin=false
VT U+000B            leaks_admin=true
FF U+000C            leaks_admin=true
NEL U+0085           leaks_admin=true
NBSP U+00A0          leaks_admin=true
OGHAM U+1680         leaks_admin=true
IDEO U+3000          leaks_admin=true

Your one-line fix at the assembly point, with the reasoning in a comment. After it, all six come out filtered — weather survives, admin_tool gone — not merely dropped. And your point about the test shape is the one I'd have got wrong: each case asserts the authorized tool survives, because NotContains alone would also be satisfied by failing closed, silently degrading every legitimate response carrying leading whitespace.

You're right that this is a regression this PR introduced rather than a pre-existing gap, and right about why: moving TrimSpace inside valueCarriesResult meant it only ran on values the decoder had already accepted. Same disagreement-with-the-client class the PR exists to close, relocated one layer down.

§2 — per-event assembly giving up the per-line net: fixed, not just disclosed

Fixed. When the assembled payload as a whole is undecodable, each data: value is now probed on its own before the event is conceded as needing no filtering. It only ever turns a pass-through into a fail-closed envelope, never the reverse, so it can't reintroduce the split-payload bypass — that argument is in the comment so it doesn't get "simplified" later.

Your distinction is what made me fix rather than disclose: TestCarriesResult's reasoning justifies not catching the result, but the consequence is forwarding the unfiltered list verbatim, and that's strictly weaker than the loop it replaces. Three of your prefixes are pinned as a regression test; I skipped 01 since you found it doesn't reproduce.

§3 — body corrected

Corrected. Vector 2 (two messages as two data: fields of one event) did not leak pre-fix — the per-line loop filtered the second line independently. It's a hazard this rewrite creates and then closes, which is what the test's own doc comment says; only the summary overclaimed. The other four genuinely leaked, which I'd verified.

Your three offered test assets — all taken

• WHATWG parser + delivery guard. Adapted to our two-arg method. The justification is what sold it: after Use standard JSON-RPC error codes and scrub leaked error text #6066 the payload looks conformant while being undeliverable, so bytes aren't evidence — only a dispatched event is.
• Zero-events companion. In, guarding the guard.
• Asset 3, the fail-closed invariant for the rest of the stream. This was the real gap, and it caught a defect in my own first attempt: I'd asserted only NotContains(admin_tool), which passes if the loop truncates the stream — precisely the regression the test exists to catch. It now also asserts an authorized tool survives the second event and that the failing event still emits its envelope. Verified against a truncate-after-failure mutation, which produces two error envelopes and no weather and is now caught.

Nits

• filterListResponse and "result":null — taking the point; a legitimate upstream error carrying "result":null now gets its code and message replaced. Fail-closed direction, and I've left the behaviour, but it's worth knowing that objectCarriesResult treats an explicit null as carrying a result deliberately (RawMessage's UnmarshalJSON runs for JSON null, so the field is non-nil whenever the key exists) — there's a test row pinning it so nobody "fixes" it into skipping nulls.
• Two-messages-in-one-event now getting a hard envelope — disclosed in the body.
• outputLines headers and the double Write — noted, left. Taste, and I agree it's yours to call rather than a convention.

One interaction from the rebase worth flagging

#6092's fail-closed default: branch routes a 2xx body with an unrecognized Content-Type into processSSEResponse when it sniffs a result. So the per-event assembler has a caller it didn't have when you measured it — bodies arriving with a wrong or missing Content-Type. Not a defect in either change, but it means §1 and §2 were reachable through one more path than either of us tested.

On sseCarriesResult staying line-based inside a now-per-event file: leaving it, per your offer. It only decides whether to filter and errs toward filtering, so it's a readability inconsistency rather than a correctness one. Happy to take it if you'd prefer it folded in.

Finally — on pkg/mcp/tool_filter.go, which you flagged as out of scope and probably carrying the same class of bug: it does, and it's live on main. I confirmed mixed terminators, a leading BOM, and a batch array all leak there, and there's no carriesResult equivalent in that file at all. Separate PR; I have the repros.

[jhrozek]

Correction to my previous comment: the §2 fix was not actually in the commit I pushed, despite the commit message claiming it. Fixed and force-pushed as f2eb6c11f.

What happened: I ran mutation experiments on response_filter.go to revert-prove the tests while a parallel agent was editing the same file, and one restore clobbered the other's write. The commit captured a state where the per-value probe's comment was present but the loop body was not — return event unconditionally.

My process error is the more useful part: I committed on the strength of a full-suite run that preceded the final file state, rather than verifying the committed content. So the green result I reported was real but stale. The commit shipped with its own guard test failing, which I have now confirmed directly against f6cd378ac in an isolated worktree:

--- FAIL: ..._SSE_PerValueProbeCatchesConcatenatedGarbage/plain_garbage_prefix
--- FAIL: ..._SSE_PerValueProbeCatchesConcatenatedGarbage/bare_NaN_prefix
--- FAIL: ..._SSE_PerValueProbeCatchesConcatenatedGarbage/comment-like_prefix

Two things worth noting, since they bound the damage:

• Only that one hunk was lost. §1's bytes.TrimSpace on assembly and all four new tests were present and correct in the bad commit. Scoped by diffing the working tree against the commit rather than assuming.
• The test caught it, which is the point of having written it. A commit whose message claims a fix it doesn't contain is exactly what a revert-proven guard is for.

I have changed how I verify: every commit in the stack is now checked out into its own worktree and tested there, not at the tip. All four pass:

081a7681c  PASS  Frame SSE filter-failure errors as SSE events
49f3c344f  PASS  Parse SSE responses per event, not per line
51b253700  PASS  Tighten SSE filter comments and close test gaps
f2eb6c11f  PASS  Close two SSE filter bypasses found in review

And I verified the pushed object contains the loop, not just my local one.

One correction to my own claim in the previous comment, from the same pass: I said test 4 (the fail-closed-rest-of-stream invariant) also guards the §2 fix. It does not. Its failing frame carries error and result on one clean Response, so it trips filterListResponse's existing both-fields check — a different fail-closed path from the undecodable/per-value-probe one. ..._PerValueProbeCatchesConcatenatedGarbage is what pins §2; test 4 pins the continue-after-failure property on a pre-existing trigger. Both are worth having, but they guard different things and I described them as overlapping.

[JAORMX]

Panel review — recommendation: REQUEST-CHANGES

The per-event rewrite is the right design and closes real #5257 bypasses (concatenated events, split payloads within one event, error+result smuggling, silent-drop → fail-closed envelopes). But it introduces one new bypass (a regression vs main) and leaves one incompletely fixed — both in the unsafe direction, where the proxy parses less than the client that actually consumes these streams.

Root cause: the rewrite adopts strict WHATWG EventSource line semantics, but the reference client it cites — go-sdk mcp/event.go scanEvents, used by ToolHive's own vMCP/mcpcompat client stack — splits on LF only and trims each data: value individually. The invariant that actually holds is "the filter must never parse less than any real client." Every finding below was reproduced end-to-end against branch head 3fed5609.

Gating

[1] CRITICAL — lone CR is a line terminator for the proxy but JSON whitespace for the go-sdk client. Full authz bypass, regression vs main. (response_filter.go:251, :258-259; reported independently by 4 of 5 lenses)

Upstream returns text/event-stream/200 for tools/list with a bare \r mid-payload:

data: {"jsonrpc":"2.0","id":1,\r"result":{"tools":[{"name":"admin_tool"}]}}\n\n

bytes.IndexAny(raw, "\r\n") splits at the lone CR; line 2 loses its data: prefix and is dropped, so the assembled payload is a truncated object that fails to decode, carriesResult is false, the per-value probe is false, and the body is re-emitted verbatim. go-sdk reads it as one line (ReadBytes('\n') + TrimRight), keeps the interior CR as insignificant JSON whitespace, and delivers admin_tool. On main the single-separator sniff picked \n and filtered correctly — so this PR regresses main.
Fix (pick one): split on \n only (matching the client); or fail closed on any \r not part of a CRLF; or dual-assemble and fail closed if either reading carries a result.

[2] HIGH — data: values joined raw and trimmed once; go-sdk trims each value first. Interior exotic whitespace defeats the filter. (:323, :336; misleading comment at :329-335)

A unicode.IsSpace byte that Go's JSON scanner rejects (U+000B, U+000C, U+0085, U+00A0, U+1680, U+3000) at an interior data:-value boundary survives in the proxy's joined buffer but is stripped per-value by the client. 6/6 variants leak. main was equally exposed → this is an incomplete fix, not a regression.

data: {"jsonrpc":"2.0","id":1,<U+00A0>\ndata: "result":{"tools":[{"name":"admin_tool"}]}}\n\n

Fix: bytes.TrimSpace each dataValues element before the join (and in the :354-362 probe). Also correct the comment at :329-335 — its claim that go-sdk "trims its data buffer with exactly this function" is what made the granularity error look safe. TestResponseFilteringWriter_SSE_LeadingNonJSONWhitespaceStillFiltered uses these exact code points but only as a prefix on a single-line payload, where whole-buffer trim coincides.

[3] MEDIUM (gating subset) — the test oracle shares the implementation's bug. parseSSEStream (test ~:2240) reimplements the same WHATWG lone-CR splitting as the code under test, so the suite agrees with the implementation by construction and structurally cannot detect [1]. Re-point the oracle at the client's line model (LF-only, TrimRight "\r\n", per-value TrimSpace) and add regressions for [1] (lone CR inside a data payload) and [2] (exotic whitespace at an interior value boundary).

Non-gating follow-ups (fine as a second PR)

• [4] LOW — the fail-closed envelope inherits the offending event's event: name; go-sdk skips non-message events (streamable.go evt.Name != "message"), so the Filter-failure errors on the SSE path are silently discarded #6037 hang persists for named events. Drop/rewrite the event: field when substituting.
• [5] LOW — an upstream error alongside explicit "result":null now loses its real code (e.g. -32601 → generic -32603); DecodeMessage sets Result to non-nil "null", tripping the both-set check at :544. A null result can't carry a list, so failing closed buys nothing. Exempt a literal null (len(trimmed) > 0 && !bytes.Equal(trimmed, []byte("null"))); leave objectCarriesResult's null-is-a-result rule alone.
• [6] LOW (plausible) — sseCarriesResult (:516) is still per-line, so the unrecognized-media-type gate is weaker than the per-event processor it guards. No client found parses text/plain as SSE, hence plausible not confirmed; have it reuse the event-based assembly so the sniffer is never weaker than the processor. Its doc comment ("applies its own per-line filtering") is now stale.
• [7] LOW — comment at :270-277 wrongly states an unterminated trailing event "is never dispatched by a spec-compliant client"; go-sdk dispatches it at EOF (mcp/event.go:149). Runtime is correct and secure; the comment would mislead a future editor into "fixing" framing here.
• Remaining test gaps: the per-value-probe test asserts only NotContains(admin_tool), so a silent drop (the Filter-failure errors on the SSE path are silently discarded #6037 hang) would pass, and it uses an unparsed request so the error ID is uncorrelatable; the application/json error+result fail-closed path is provably 0-hit; probe ordering at :134/:140 is unpinned.

What the PR gets right — please don't churn these

The core per-event thesis and data:-value grouping; byte-preserving pass-through of original terminators; fail-closed-with-JSON-RPC-envelope over silent drops (#6037); the error+result fail-closed rule (a genuine, non-obvious TS-SDK-zod bypass caught); the BOM strip; and the single-space CutPrefix with its mid-string-literal rationale. The line-for-line mcp/event.go citations are the only reason this review could be adjudicated at all — two of them are factually wrong ([2], [7]), so fix the text but keep the practice.

All three gating fixes are small and can only convert pass-throughs into filtered or fail-closed frames, so none can create new exposure — this should be one revision cycle, not a redesign.

---

_{🤖 Multi-lens panel review (SSE framing · security-bypass · JSON-RPC/protocol · Go correctness · test coverage), each finding adversarially verified. Generated with Claude Code.}

[jhrozek]

All three gating findings fixed in c0e60922a. I verified each claim independently before changing anything, and every one held — including the regression.

The invariant is the finding

"The filter must never parse less than any real client" is the right frame and I had the wrong one. I built the scanner to the WHATWG grammar; the reading that decides whether a payload reaches a caller is go-sdk's scanEvents. Confirmed against go-sdk@v1.7.0-pre.3/mcp/event.go: ReadBytes('\n') (LF only), TrimRight(line, "\r\n") so an interior CR survives, and data := bytes.TrimSpace(after) per value. Both bypasses fall straight out of those three lines.

[1] CRITICAL — confirmed, including the regression

Reproduced end-to-end, with a client model built from scanEvents rather than string-matching:

body	this branch (before)	origin/main
lone \r mid-payload	leaks admin_tool	filters correctly
U+00A0 at interior value boundary	leaks	leaks

So [1] regresses main exactly as you said, and [2] is the incomplete fix you called it. In both cases my client model reassembles the denied tool, so these are disclosures rather than divergence-in-principle.

Fix: LF-only scan, per-value trim

Went with matching the client rather than failing closed on stray CR, because it can only ever decode and filter more than before, never less. Reasoning now in the code: a browser EventSource does treat a lone CR as a terminator and will mis-frame a pass-through body containing one — but identically to how it would mis-frame the backend's own bytes, since output stays byte-preserving. Worst case is an unparseable payload for that client, i.e. denial, not disclosure.

One subtlety the LF-only port introduces that wasn't in your writeup: a CRLF line's \r now lives in the line text, not the terminator, so replacing a data line would have silently downgraded it to bare LF while untouched lines stayed CRLF. The \r is re-attached on replacement. _SSE_FilteredEventTerminators's CRLF byte-scan would have caught it, and I re-verified independently: zero bare LFs on a filtered CRLF stream, and pass-through byte-identical.

[3] — the sharpest finding, and it indicts a test I accepted

You're right that the oracle shares the implementation's bug and so structurally cannot detect [1]. Worth recording how it got there: parseSSEStream came to this PR from an earlier review on #6087, on the argument that conformant-looking bytes aren't evidence of delivery — which was correct. But it was written to the WHATWG grammar, the same wrong model as the code it was auditing, so it laundered the bug into the test suite. Re-pointed at the go-sdk client model, with a doc comment saying why that's the security-relevant oracle rather than the spec.

Both regressions now have tests, each asserting the authorized tool survives as well as the denied one being absent — a NotContains alone also passes on a silent drop, which is the #6037 hang.

Non-gating: [4], [5], [7] taken now; [6] left

• [4] taken — and it mattered more than "LOW" suggests: the envelope inherited the offending event's name, and go-sdk skips non-message events, so on a named event the Filter-failure errors on the SSE path are silently discarded #6037 hang survived the fix meant to end it. Envelope substitution now drops the event: line, proven via the corrected oracle.
• [5] taken — literal "result":null exempted from the both-set rule, so a legitimate -32601 no longer becomes our generic -32603. objectCarriesResult's null-is-a-result rule left alone, as you asked.
• [7] taken — comment corrected; go-sdk dispatches at EOF via yieldEvent().
• [6] left, per JAORMX's earlier offer on this PR to leave sseCarriesResult to your side. Its stale "per-line filtering" doc is fixed.

On the two wrong citations

Both were mine and both were load-bearing: I wrote that go-sdk "trims its data buffer with exactly this function" (it trims per value — that is [2]) and that a spec-compliant client never dispatches an unterminated trailing event. Corrected, and I've kept the practice of citing mcp/event.go line-by-line, since your point about it being what made this adjudicable is the reason the errors were catchable at all.

Remaining test gaps you listed are closed except the application/json error+result path being 0-hit and the probe ordering at the media-type gate, which I've left — both are in #6092's dispatch code rather than the per-event processor.

Four commits, each verified in its own worktree so the stack stays bisectable.