Repository navigation
Fix: strip hop-by-hop response headers, share SSE normalization, and dedupe the listener helpers - #1605
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 8 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…dedupe the listener helpers Three drift defects from the listener/parser audit (rossoctl#1567), where one copy got a fix its twins did not: 1. Hop-by-hop response headers reached the client. The forward proxy's four response-header copy loops relayed resp.Header verbatim — requests were stripped (stripForUpstream), responses were not, and a comment on one loop even claimed the stripping happened. Through the TLS bridge an HTTP/1.1 origin's Keep-Alive lands inside an HTTP/2 HEADERS frame, which Node's http2 rejects with ERR_HTTP2_STREAM_ERROR. All four sites now share copyResponseHeaders, which skips the hop-by-hop set plus anything the response's own Connection header names. 2. Only the inference parsers handled a BOM or CR-only line endings. A BOM-prefixed or CR-only MCP/A2A stream matched no data: line, the extension stayed empty, and IBAC treats an unclassified exchange as one that passes by default. normalizeSSE moved to parsercommon.NormalizeSSE and is now used by all six buffered scanners. 3. Request helpers copied between listeners. isEventStream was defined three times and setBody/syncHeaders were inlined in the reverse proxy; all now live in core/listener/httpx (IsEventStream, SetRequestBody, SyncRequestHeaders) with one definition and six call sites. Every fix is pinned by a test that was red on main: hop-by-hop headers absent end-to-end on both the buffered and streaming paths, and BOM/CR-only MCP and A2A streams classifying. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
c83faa1 to
d2b6d63
Compare
clawgenti
left a comment
clawgenti
left a comment
There was a problem hiding this comment.
Summary
Clean, well-scoped fix for the three drift defects in #1567. copyResponseHeaders now strips the RFC 9110 hop-by-hop set (plus Connection-named headers) on all four non-101 response paths, while the 101 splice still copies headers raw, which it has to. NormalizeSSE was moved, not changed, and every buffered data: line scanner in the parsers now goes through it (I grepped: no LF-only split is left over a raw body). The helpers moved into httpx are the same code, so the dedupe doesn't change behaviour.
Verified locally on d2b6d63: go vet and gofmt -l are clean on the touched packages, and go test ./listener/... ./plugins/{a2a,mcp,inference}parser/ ./plugins/internal/... is green. I also copied the six new tests onto origin/main and all six fail there, so they really are regression pins.
Two small comments inline. Neither blocks the merge.
Author: huang195 (MEMBER, maintainer)
Areas reviewed: Go (listener forward/reverse/extproc, httpx, a2a/mcp/inference parsers), tests
Agent/IDE config (.claude/.vscode): none
Commits: 1 commit, signed off: yes (nit: the subject is 96 chars, over the advisory 72)
CI status: DCO, pre-commit, lint, action-pinning and dependency review pass; core Go CI and CodeQL were still pending when this was posted (local run green)
| // The streaming path has its own copy loop (streamPassthrough); the same rule must | ||
| // hold there. No StreamingResponder is configured, so the passthrough loop is the | ||
| // one that runs. | ||
| func TestForwardProxy_HopByHopResponseHeadersStrippedOnStream(t *testing.T) { |
There was a problem hiding this comment.
suggestion: these two tests pin the buffered path (serveOutbound) and streamPassthrough. The other two call sites, handleStreamingResponse (when a StreamingResponder is configured) and streamFallbackBuffered (an SSE stream with a WritesResponseBody plugin), aren't covered. The helper is shared, so the logic is covered once. But the bug in #1567 was literally a copy loop at one site, so if someone inlines a loop back into one of those two paths, nothing would catch it. A small table that adds a no-op StreamingResponder plugin and a no-op response mutator would cover all four sites.
| @@ -1893,31 +1889,50 @@ func (s *Server) attributeForRecording(pctx *pipeline.Context, clientHeaders htt | |||
| // recordOutboundRequestEvent records pctx's request row under sid. Its pair is | |||
There was a problem hiding this comment.
nit: this doc comment belongs to recordOutboundRequestEvent (around line 2054). On main it was already stranded above syncHeaders. Now that syncHeaders is gone, it sits directly above hopByHopHeaders, separated only by a blank line. go doc won't attach it to either one, and a reader will think it describes the map. Since you're in this block anyway, it's worth moving it down to the function.
github-project-automation
Bot
moved this from New/ToDo
to Done
in Rossoctl Issue Prioritization
Summary
Closes #1567 — three places where copied listener/parser code drifted apart, one copy getting a fix its twins did not.
1. Hop-by-hop response headers reached the client
The forward proxy's four response-header copy loops relayed
resp.Headerverbatim. Requests were stripped (stripForUpstream), responses were not — and the comment on one of the loops claimed the stripping happened. Through the TLS bridge, an HTTP/1.1 origin'sKeep-Alivelands inside an HTTP/2 HEADERS frame, which Node'shttp2rejects withERR_HTTP2_STREAM_ERROR(reproduced in the issue).Fix: one
copyResponseHeaders(w, resp)used at all four sites. It skips the hop-by-hop set — the response-side twin ofstripForUpstream's list — plus anything the response's ownConnectionheader names, which RFC 9110 §7.6.3 also makes per-connection.Content-Lengthis left to the sites that manage it.2. Only the inference parsers survived a BOM or CR-only SSE body
The buffered
data:scanners in a2a-parser and mcp-parser split on LF over the raw body. A BOM-prefixed body defeats the prefix check on the first line, and a CR-only body is one line to an LF split — so the extension stayed empty and IBAC treated the exchange as unclassified, which passes by default. inference-parser already hadnormalizeSSEfor exactly this.Fix:
normalizeSSEmoved toparsercommon.NormalizeSSE, its doc comment carried over, and all six buffered scanners now use it (inference ×3, a2a ×2, mcp ×1).3. Request helpers copied between listeners
isEventStreamwas defined three times; the reverse proxy inlined copies ofsetBodyandsyncHeaders. All three now live incore/listener/httpx(IsEventStream,SetRequestBody,SyncRequestHeaders) — byte-identical logic moved, six call sites updated, net −100 lines.Testing
Every fix is pinned by a test that was red on
mainand green after:TestForwardProxy_HopByHopResponseHeadersStripped/...StrippedOnStream—Keep-Aliveand aConnection-named header absent end-to-end, end-to-end header still forwarded, on both the buffered and streaming paths.TestA2AParser_OnResponse_BufferedSSELeadingBOM/...CROnlyLineEndings,TestMCPParser_OnResponse_SSELeadingBOM/...CROnlyLineEndings— a wire-legal-shaped stream now classifies. (The MCP BOM fixture deliberately makes the first data frame the only parseable one; a second result frame would let both shapes pass for the wrong reason.)Full
core/listener/...andcore/plugins/...suites green;go vetandgofmtclean.Assisted-By: Claude (Anthropic AI) noreply@anthropic.com