Deliver SSE filter failures as parseable events - #6085
Conversation
When response filtering failed on the SSE path, writeErrorResponse wrote a bare JSON object into the text/event-stream body with no 'data: ' prefix and no blank-line terminator. Under the WHATWG event-stream grammar that parses as an unrecognized field and is silently discarded, so the client never saw the error at all — it saw a stream that simply ended with no response and hung until its own timeout. Two compounding problems lived in the same function: the WriteHeader(500) was usually a no-op because earlier stream writes had already committed the headers, and the stop=true early return skipped the line-separator reconciliation that exists to keep SSE parsing intact. Of the options #6037 poses (emit a framed error event, or abandon the stream), emit the event: the Streamable HTTP transport expects the SSE stream to eventually carry the JSON-RPC response for the POSTed request, an error response is a response, and the reference TypeScript SDK's SSE loop parses error responses for completion detection — so a framed, correlatable error is strictly better than a close the client cannot tell from success. The filter has already failed closed by this point; what the client deserves is the ability to stop waiting and to know why. writeSSEError frames the error as a self-terminated event: a leading separator first terminates any partially written preceding event (fixing the truncation interaction — a partial 'data:' line can no longer strand the error in another event's buffer), then 'data: <error>' and a blank line to dispatch it. No status rewrite is attempted mid-stream. The JSON path keeps writeErrorResponse unchanged. The delivery guard runs the written bytes through a WHATWG-grammar SSE parser and requires exactly one dispatched, default-typed event whose data is the conformant error — because after #5950 the payload alone LOOKS correct while remaining invisible to any SSE client, conformant bytes are deliberately not accepted as evidence here. A companion test pins that the pre-fix shape yields zero events under the same parser. Fixes #6037 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n Rebased onto post-#6066 main: filterErrorBody adopts #6066's scrubbed generic message and server-side logging (the error parameter now feeds the log only), and writeSSEError logs at the same Error level. The delivery fix itself is unchanged — #6066 made the SSE-path payload conformant and scrubbed but left it framed as a bare JSON object no event-stream parser dispatches.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6085 +/- ##
==========================================
- Coverage 72.28% 72.26% -0.03%
==========================================
Files 728 728
Lines 75514 75530 +16
==========================================
- Hits 54588 54579 -9
- Misses 17002 17045 +43
+ Partials 3924 3906 -18 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Closing as superseded by #6087, which fixes the same bug (#6037) and was opened independently in parallel. Having compared the two: #6087's approach is the better fix. It emits the error as a substitute The test assets from this PR that are independent of the fix's shape — the strict WHATWG-grammar SSE parser, the delivery guard built on it, and the zero-events companion proving the pre-fix shape is never dispatched — are offered on #6087 for the author to take or leave. No action needed from reviewers here. The stacked #6086 has been rebased off this branch and retargeted to |
2 participants
Summary
Why. When response filtering fails on the SSE path,
ResponseFilteringWriter.writeErrorResponsewrites a bare JSON object into thetext/event-streambody — nodata:prefix, no blank-line terminator. Under the WHATWG event-stream grammar that line parses as an unrecognized field (named{"jsonrpc"up to the first colon) and is silently discarded: the client never sees the error at all, only a stream that ends with no response, and hangs until its own timeout. Two compounding problems live in the same function: theWriteHeader(500)is usually a no-op (earlier stream writes and the proxy'sFlushhave already committed the 200/text/event-streamheaders, so Go logssuperfluous response.WriteHeader call), and thestop=trueearly return skips the line-separator reconciliation whose own comment says it exists to keep SSE parsing intact — so truncation can strand a partial event.#6066 made this MORE deceptive, not fixed. It rewrote
writeErrorResponseso the payload on this path is now conformant JSON-RPC (-32603, scrubbed message) — while remaining framed as a bare JSON object no event-stream parser will ever dispatch. Conformant bytes are not delivery. A reviewer glancing at that code post-#6066 will conclude it is fine; #6055's reviewer notes predicted exactly this trap ("please don't take the green bytes there as evidence the path works"). The delivery, not the payload, is what this PR fixes.The decision (#6037 poses it explicitly: emit a framed error event, or abandon the stream?): emit the event, then end the stream. The Streamable HTTP transport expects the SSE stream to eventually carry the JSON-RPC response for the POSTed request; an error response is a response; and the reference TypeScript SDK's SSE loop parses error responses for completion detection — so a framed, correlatable error is strictly better than a close the client cannot distinguish from success. The filter has already failed closed by this point; what the client deserves is the ability to stop waiting and know why.
How: new
writeSSEErrorframes the error as a fully self-terminated event —data:line can no longer strand the error inside another event's buffer); at an event boundary the blank line is a spec-level no-op;data:field line, followed by the blank line that dispatches it;WriteHeader(500)is gone from the SSE path (the JSON path keeps it, unchanged);stop=trueearly return is now safe by construction, because the frame does not depend on the loop's later reconciliation.The scrub semantics from #6066 are preserved: the shared
filterErrorBodyemits the generic-32603 "internal error"body, and both writers log the underlying error server-side only.Closes #6037 (all three of the issue's components: delivery framing, the mid-stream
WriteHeader, the truncation/linesep interaction).Type of change
Test plan
go test ./pkg/authzgreen (plus./pkg/mcp ./pkg/webhook/...for the neighborhood).task testnot used locally (known gotestfmt v2.5.0 panic); CI runs the real thing.task lint-fix) — 0 issues.The guard deliberately refuses to accept conformant bytes as evidence.
TestWriteSSEErrorIsParseableByAnSSEClientruns the written bytes through a strict WHATWG-grammar SSE parser (parseSSEStream, implemented per the spec's "interpreting an event stream" rules: unrecognized fields ignored, dispatch only on a blank line with a non-empty data buffer, undispatched data discarded at EOF) and requires exactly one dispatched, default-typed event whose data is the conformant JSON-RPC error — across LF and CRLF streams, with int64/string ids echoed and an unrecoverable id omitted (never"id":null).TestWriteSSEErrorAfterPartialEventpins the truncation fix (the error parses standalone after an undispatcheddata:orevent:line). The companionTestBareJSONInEventStreamYieldsNoSSEEventfeeds the parser exactly what the pre-fix code writes and asserts zero events — documenting the failure mode and guarding the guard's strictness.Proven fail-before/pass-after: with
writeSSEError's body reverted to the pre-fix behavior (WriteHeader(500)+ bare write), both delivery tests fail; restored, the package is green. The recorder also pins that no status rewrite is attempted mid-stream.API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.Changes
pkg/authz/response_filter.gofilterErrorBody(shared body builder, #6066 scrub preserved);writeErrorResponsescoped to the JSON path with a do-not-use-on-SSE warning; newwriteSSEError;writeSSEDataLinetakes the stream's separator and routes error paths to itpkg/authz/response_filter_test.goparseSSEStreamhelper + the three delivery/truncation/zero-event guardsDoes this introduce a user-facing change?
Yes. When response filtering fails mid-SSE-stream (e.g. a backend frame smuggling a result outside a clean Response), clients now receive a parseable JSON-RPC error event carrying the request id, instead of a silently truncated stream that forces them to hang until their own timeout.
Special notes for reviewers
writeSSEErrorreturns nil once the event is written: the failure has been delivered in-band and logged server-side; only a write failure propagates.pkg/mcp/tool_filter.go(whichprocessSSEResponsewas originally adapted from) does not share this bug's trigger surface the same way and is deliberately not touched here — Stop splicing whole SSE buffers into error text #6079 recently swept the error-text side of that file; if its delivery paths need the same treatment, that is its own change.🤖 Generated with Claude Code
https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n