Frame SSE filter-failure errors as SSE events

[jhrozek]

Summary

On the text/event-stream path, a filtering failure wrote a bare JSON object straight into the stream body with no data: prefix. Per the WHATWG event-stream grammar that line parses as a field named {"jsonrpc", and an unrecognized field is ignored — so the client never saw the error at all. It saw a stream that stopped with no response to its request, and hung until its own timeout. That is the worst available failure mode, because the caller cannot distinguish it from a dead server.

Three problems compounded in the same spot, all fixed here:

• No data: prefix, so the error was silently discarded by every SSE client.
• A mid-stream WriteHeader(500) that was usually a no-op: Flush() has already committed the headers by then, so Go logs superfluous response.WriteHeader call and drops it. Useless even when it lands, since Content-Type is still text/event-stream.
• An early return that skipped the trailing line-separator reconciliation — the very code whose comment says it exists so as not to break SSE parsing.

The fix emits the error as a data: line in place of the frame that failed and lets the loop continue, so the separator and the reconciliation frame it exactly as they frame a successfully filtered frame. Remaining frames are unrelated to the failed one, so continuing also removes the truncation the old early return caused.

writeSSEDataLine's stop return value collapses into err != nil, which the diff shows is now exactly equivalent.

Fixes #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/... is green at this commit, and each commit in the stack now builds and tests individually — verified with a per-commit go vet loop. That was not true when this PR was first opened: errorResponseBody became a method here while the matching test fix landed only in a later commit on #6088, so this commit did not compile. Corrected, and the earlier claim in this section was wrong.

task test is not usable locally — it panics 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.

New tests were revert-proven individually: the production change reverted, the test confirmed failing, then restored.

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

Does this introduce a user-facing change?

Yes. When response filtering fails on an SSE stream, the client now receives a parseable JSON-RPC error event correlated to its request id, instead of a stream that ends with no response. Clients that previously hung to their own timeout now get an error they can surface.

Special notes for reviewers

This buys framing parity with the success path, not general SSE correctness. A body carrying more than one event is still mis-framed by the separator reconciliation, and a body with no trailing blank line still gets no terminator. Both predate this change, apply equally when filtering succeeds, and are left alone here — the follow-up PR rewrites that routine and fixes both.

The failure branch is unreachable from any real backend response. jsonrpc2.EncodeMessage performs no validation and only fails on JSON that DecodeMessage already accepted; every list filter's json.Marshal operates on unmarshal-derived values with swallowed errors; mcp.Tool.MarshalJSON's one error needs RawInputSchema, which is json:"-". So the tests cover it at the seam rather than end-to-end, and no test-only injection point was added to production code — .claude/rules/testing.md forbids those. Please don't read the encode-failure fallback as dead code.

Stacked on this: #6087 rewrites processSSEResponse to parse events rather than lines, which closes five filter bypasses. Review order matters — that one supersedes some of this mechanism.

Generated with Claude Code

[JAORMX]

Offering three test assets from the parallel #6085 (now closed in favour of this PR) — take or leave any of them. They are independent of the fix's exact shape, with one caveat at the end.

1. A strict WHATWG-grammar SSE parser + a delivery guard built on it. The point of asserting through a real grammar implementation rather than string-matching: after #6066 the error payload on this path looks conformant while being undeliverable, so a guard must refuse to accept conformant bytes as evidence — only a dispatched event counts. The parser is deliberately strict (unrecognized fields ignored, dispatch only on a blank line with a non-empty data buffer, undispatched data discarded at EOF), which an independent review panel verified against the spec grammar rather than a permissive approximation:

parseSSEStream (~70 lines, click to expand)

// sseEvent is one event dispatched by parseSSEStream.
type sseEvent struct {
	eventType string
	data      string
}

// parseSSEStream interprets body per the WHATWG HTML "server-sent events"
// event-stream grammar (§"Interpreting an event stream"): lines end at CRLF,
// LF, or CR; a line starting with ':' is a comment; a line containing ':'
// splits into a field name and value (one leading space of the value is
// stripped); a line without ':' is a field with an empty value. Only
// recognized fields have any effect — "data" appends value+"\n" to the data
// buffer, "event" sets the event type — and an UNRECOGNIZED field is ignored
// outright. A blank line dispatches an event only when the data buffer is
// non-empty, and pending data left at end-of-stream is discarded.
//
// The strictness is the point of these tests: a bare JSON object written
// into a stream is one giant unrecognized field ('{"jsonrpc"' up to the
// first colon) and yields no event at all — the silent client hang of #6037
// — so a conformant-looking payload is not evidence of delivery.
func parseSSEStream(t *testing.T, body []byte) []sseEvent {
	t.Helper()

	normalized := strings.ReplaceAll(string(body), "\r\n", "\n")
	normalized = strings.ReplaceAll(normalized, "\r", "\n")
	lines := strings.Split(normalized, "\n")
	// A trailing line terminator makes Split produce a final "" that is not a
	// blank LINE (a blank line is two consecutive terminators); drop it so an
	// unterminated final event is correctly left undispatched.
	if len(lines) > 0 && lines[len(lines)-1] == "" {
		lines = lines[:len(lines)-1]
	}

	var events []sseEvent
	var dataBuf strings.Builder
	var eventType string
	for _, line := range lines {
		switch {
		case line == "":
			if dataBuf.Len() > 0 {
				events = append(events, sseEvent{
					eventType: eventType,
					data:      strings.TrimSuffix(dataBuf.String(), "\n"),
				})
			}
			dataBuf.Reset()
			eventType = ""
		case strings.HasPrefix(line, ":"):
			// Comment; ignored.
		default:
			name, value, hasColon := strings.Cut(line, ":")
			if hasColon {
				value = strings.TrimPrefix(value, " ")
			} else {
				value = ""
			}
			switch name {
			case "data":
				dataBuf.WriteString(value)
				dataBuf.WriteString("\n")
			case "event":
				eventType = value
			default:
				// Unrecognized field: ignored per the grammar. A bare JSON
				// line lands here.
			}
		}
	}
	return events
}

A delivery guard on it then looks like (adapt names to this PR's writeSSEErrorLine):

// The error must arrive as exactly one dispatched, default-typed SSE event
// whose data is the conformant JSON-RPC error — run through a WHATWG-grammar
// parser, because conformant bytes that no SSE client dispatches (the
// pre-fix behavior, and the post-#6066 shape) must fail this test.
events := parseSSEStream(t, rr.Body.Bytes())
require.Len(t, events, 1, "body: %q", rr.Body.String())
assert.Empty(t, events[0].eventType, "MCP clients only process default/'message' events")
assert.JSONEq(t, wantErrorJSON, events[0].data)

2. The zero-events companion — feeds the parser exactly what the pre-fix code wrote and asserts nothing dispatches. It documents the failure mode and, more importantly, guards the guard: if parseSSEStream ever loosens enough to accept a bare JSON object, this fails first:

func TestBareJSONInEventStreamYieldsNoSSEEvent(t *testing.T) {
	t.Parallel()
	body := errorResponseBody(jsonrpc2.Int64ID(1)) // this PR's body builder
	assert.Empty(t, parseSSEStream(t, body),
		"a bare JSON object must not parse as an SSE event")
	assert.Empty(t, parseSSEStream(t, append(body, '
')),
		"a line terminator alone must not turn a bare JSON object into an event")
}

3. The one neither PR has, and the one I would most want added: the fail-closed invariant for the rest of the stream. Both PRs (and #6085 before them) pin the error frame's cosmetics thoroughly, but nothing pins what happens to the remaining lines after a mid-stream filter failure — and that is the actual security property on this path (#5257: a frame carrying a result outside a clean Response must never pass through unfiltered). Since this PR's loop continues after the substitute error line, the invariant to pin is that a smuggled-result line after the failure point still never reaches the client raw:

// After a mid-stream filter failure, subsequent lines must still be
// classified — a smuggled result following the failed frame must never be
// written through raw (#5257 fail-closed). Sketch; adapt the trigger to
// however this PR's tests drive the error path.
stream := "data: " + failingFrame + "
" +
	"data: " + `{"jsonrpc":"2.0","id":9,"result":{"tools":[{"name":"admin_tool"}]}}` + "

"
_, _ = rfw.Write([]byte(stream))
require.NoError(t, rfw.FlushAndFilter())
assert.NotContains(t, rr.Body.String(), "admin_tool",
	"a smuggled result after a filter failure must not pass through unfiltered")

Caveat: items 1 and 2 were written and revert-proven against #6085's implementation on top of main; I have not run them against this branch, and #6088's per-event rewrite may require adapting both (the parser itself is implementation-independent, the drivers aren't). Happy for any or all of this to be taken verbatim, reshaped, or dropped.

[JAORMX]

Both red checks trace to one typo, and there are two more failures hiding behind it. All CONFIRMED — reproduced locally at 587fd357.

The two CI failures are the same thing

pkg/authz/response_filter_test.go:1284 and :1326 call errorResponseBody(...) as a free function, but this PR makes it a method on *ResponseFilteringWriter (response_filter.go:545):

pkg/authz/response_filter_test.go:1284:12: undefined: errorResponseBody
pkg/authz/response_filter_test.go:1326:28: undefined: errorResponseBody (typecheck)

That is the entire content of Linting / Lint Go Code. Tests / Test Go Code shows panic: BUG: Empty package name encountered. at gotestfmt parser/parse.go:220 — the same upstream gotestfmt bug you hit locally, triggered here by the build failure rather than being a second problem. Fix is rfw.errorResponseBody(...) in both places; both checks should then go green together.

Two assertions fail once it compiles

I patched the receiver in a scratch copy and ran it:

• :1293 asserts float64(500); the actual code is mcpparser.CodeInternalError = −32603 (pkg/mcp/revision.go:238). Observed expected: 500 / actual: -32603. The doc comment above it says "a code-500 error" too.
• :1296 asserts assert.Contains(t, message, wrapped.Error()) — it requires the wrapped error's text to reach the client. The code correctly sends the fixed generic "internal error", so this fails with "internal error" does not contain "boom".

The second one is worth flagging beyond "wrong expectation": it asserts exactly what #6066 and security.md forbid. The hazard isn't the red check, it's that someone clearing red CI could satisfy it by making the envelope echo err — reinstating the leak of text that can name tools the caller isn't authorized to see. Your own doc comment on errorResponseBody says why that must not happen.

The helpful part

#6088 already contains the corrected version of this exact test — float64(mcpparser.CodeInternalError), plus assert.NotContains(string(body), wrapped.Error()) with a distinctive sentinel error, which pins the scrub properly rather than by coincidence. So this is a cherry-pick down, not new work.

One note on the body

The test plan says go test -count=1 ./pkg/authz/... is green and that every new test was proven to be a real regression guard by reverting the production change. Neither can hold at this commit, since the package doesn't build. The shape of the failures — free function, and a message expected to contain the wrapped text — reads like tests written against a local iteration before errorResponseBody became a method and before the #6066 scrub was folded in, then not re-run after the final refactor. Mechanical slip rather than anything about the judgement in the change; just worth correcting in the body so the next reader isn't misled about what was verified.

On the change itself

The approach looks right to me, and I checked a few things worth reporting:

• stop really does collapse into err != nil. Once both writeErrorResponse returns move out of writeSSEDataLine, the only surviving stop=true path returns a non-nil errBug wrap, so the claim is exact.
• No WriteHeader on the SSE path is right, and so is the reasoning — Flush() has committed the headers, and text/event-stream with a bare JSON body would be unreadable anyway.
• Zero regression risk. I byte-compared this branch against origin/main across a 25-shape probe matrix plus multi-event framing cases: output is identical on every reachable body. Consistent with your note that the failure branch isn't reachable from a real backend.
• The mis-framing you flag is pre-existing on main, not introduced here — I verified the multi-event output is byte-identical. So this PR documents it rather than shipping it.

Two consequences of that last point, offered as sequencing input rather than as change requests:

• It is safe to land alone — it can't regress anything — but it also delivers no reachable behaviour change, since the branch it fixes needs a filter/encode failure that no real backend response can produce.
• The reachable half of Filter-failure errors on the SSE path are silently discarded #6037 is untouched: the carriesResult fail-closed drop path emits a separator and no data, which I measured as a full response of literally "\n\n" on both main and this branch. That's the same hang, reachable from any malicious upstream, and Parse SSE responses per event, not per line #6088 is what fixes it.

So my suggestion would be to fix the three test defects and land this together with #6088 rather than ahead of it. Review of #6088 is on that PR — it's careful work and fixes more than it claims, including two user-visible bugs on main; it does also need one bypass closed, which I've written up there.

[JAORMX]

Heads-up: we put this into conflict. #6092 ("Close authz response filter bypasses via media type and status") merged to main at 10:58Z as 92c70db2 — our PR, touching the same two files, hours after I reviewed yours. Sorry about the ordering. This PR is now mergeable: false / mergeable_state: dirty.

I ran the merge to see what you're actually facing rather than guess:

pkg/authz/response_filter.go auto-merges cleanly — no conflict. #6092's production changes sit in FlushAndFilter (media-type normalization, full-2xx status handling, and a fail-closed default: branch) plus one new helper next to carriesResult; yours are lower down in processSSEResponse / writeSSEDataLine / writeSSEErrorLine / errorResponseBody. They don't touch the same lines.

All the conflict is in pkg/authz/response_filter_test.go — 3 hunks, and all three have the same cause: both PRs append a new table-driven test at the end of the file, after TestResponseFilteringWriter_JSON_DisguisedResponseFrame. No function names collide (TestErrorResponseBody / TestWriteSSEErrorLine vs TestResponseFilteringWriter_FilterBypassAttempts).

One wrinkle worth knowing before you start: git interleaved the two appends on their shared table-driven skeleton (testCases := []struct{, for _, tc := range, t.Run, closing braces), so resolving hunk-by-hunk and keeping both sides produces invalid Go — the fragments don't reassemble into two whole functions. Resolve at function granularity instead. Concretely, this worked for me:

1. take main's version of the test file
2. re-add the "errors" import your tests need
3. append your two new tests whole

That produces a gofmt-clean file, and the only remaining compile error is the pre-existing errorResponseBody receiver one from my earlier comment — i.e. the conflict resolution itself adds no work beyond what you already had.

#6088 reports clean, but only because its base is fix-sse-filter-error-framing. It inherits this the moment you rebase here, so don't read its green state as being unaffected.

One new interaction to be aware of, not a defect in either PR: #6092's fail-closed default: branch routes a 2xx body with an unrecognized Content-Type into processJSONResponse or processSSEResponse when it sniffs a JSON-RPC result. So those two functions — the ones you rewrite here and in #6088 — now have two callers they didn't have when you wrote them, and your per-event assembler will see bodies that arrived with a wrong or missing Content-Type. Related: the sniffer #6092 added (sseCarriesResult) is line-based and mirrors sniffSSEToolsList in pkg/mcp/tool_filter.go, so once #6088 lands it'll be splitting per line in a file that otherwise parses per event. It only decides whether to filter and errs toward filtering, so it's not a correctness problem — but it'll read as inconsistent, and since it's our code landing in your rewrite, entirely your call whether to fold it in or leave it for us.

Happy to do the rebase for you if you'd rather not deal with it — just say so and I'll push it. Not touching your branches otherwise.

[jhrozek]

Thanks — all three test defects confirmed and fixed, and you were right about the cause being worse than a typo.

The root cause was mine and structural. I made errorResponseBody a method during a conflict resolution in this commit, but only fixed the tests in a later commit on #6088. So this commit never compiled in isolation — I had verified the stack tip, not each commit. All four commits now build and test individually, so the stack is bisectable, which it wasn't. I verified that with a per-commit go vet loop rather than trusting the tip.

On the assert.Contains(message, wrapped.Error()) one specifically — your framing is the important part, and I've made the test assert the opposite. It now checks the wrapped error is absent from the encoded body, using a distinctive sentinel error so it can't pass by coincidence. Someone clearing red CI can no longer satisfy it by reinstating the leak.

Test plan claim corrected. You're right that neither the green-suite nor the revert-proven claim could hold at a commit that doesn't build. Body updated to say what was actually verified and at which commit.

Rebased onto main past #6092. Your resolution recipe was exactly right, including the warning — git interleaved both sides' appended tests on the shared table skeleton, so hunk-by-hunk produced invalid Go. I resolved at function granularity: each commit's own test file, plus TestResponseFilteringWriter_FilterBypassAttempts lifted whole, plus your modified TestResponseFilteringWriter_ErrorResponse taken from main (I initially patched that one line-by-line and it silently kept json.Marshal — taking the whole function was the fix). No apology needed on the ordering.

Sequencing: agreed, land them together. Your measurement that this commit is byte-identical to main on every reachable body matches my own conclusion that the branch it fixes is unreachable from a real backend, and it's the reason I flagged the failure branch as defensive rather than live.

On your three offered test assets — taken, two verbatim in spirit and one adapted. They're on #6088 since that's where the mechanism lives:

• The WHATWG parser and the zero-events companion, adapted to our two-arg method (rfw.errorResponseBody(id, err)). The reasoning 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 byte assertions aren't evidence.
• Asset 3, the fail-closed invariant for the rest of the stream. This was the real gap and it caught something: my first version asserted only NotContains(admin_tool), which would also pass if the loop truncated the stream — the exact regression it exists to catch. It now asserts an authorized tool survives the second event, and I verified that specifically against a truncate-after-failure mutation.

Not folding in, deliberately: #6092's sseCarriesResult staying line-based inside a now-per-event file. You called it — it only decides whether to filter and errs toward filtering, so it's a readability inconsistency rather than a correctness one, and it's yours. Happy to take it if you'd rather.

[codecov]

Codecov Report

❌ Patch coverage is 62.06897% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.31%. Comparing base (92c70db) to head (081a768).
⚠️ Report is 1 commits behind head on main.

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

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6087   +/-   ##
=======================================
  Coverage   72.31%   72.31%           
=======================================
  Files         730      730           
  Lines       75630    75639    +9     
=======================================
+ Hits        54689    54699   +10     
+ Misses      17035    17034    -1     
  Partials     3906     3906           

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