fix: strip hop-by-hop headers on the default transport, not only http2 - #489
Open
pacocartones wants to merge 5 commits into
Open
fix: strip hop-by-hop headers on the default transport, not only http2#489pacocartones wants to merge 5 commits into
pacocartones wants to merge 5 commits into
Conversation
A media type is case-insensitive and may be followed by parameters (RFC 9110 section 8.3.1), but the check that disables the upstream request timeout for server-sent events compared the raw `content-type` header with strict equality against `text/event-stream`. Upstreams that answer `text/event-stream; charset=utf-8` therefore fall straight through it. Starlette's `EventSourceResponse` and Spring's `text/event-stream;charset=UTF-8` are two common ones. The timeout is never cleared, it fires while the stream is idle between events, and `req.abort()` then runs after the response headers have already been forwarded downstream: the client gets a truncated stream and the proxy throws an uncaught `ERR_HTTP_HEADERS_SENT`. Parse the header instead of comparing it. `fast-content-type-parse` is already a direct dependency and `index.js` uses it the same way, so this adds no new dependency. `safeParse` normalises the case and drops the parameters, and never throws on a malformed or absent header. Signed-off-by: Manuel Sánchez <mpktmpktmpktmpkt@gmail.com>
The two tests added by this PR were too tight for CI. The http2 one hung and took the whole file past the 30s limit in 10 of 11 jobs. The session timer is armed when the http2 client connects, so `sessionTimeout: 100` had to cover connecting plus the target's first response. On a loaded runner that does not fit: in the same file `http2 request timeout` took 6167ms where it takes 325ms locally. When the timer fires before the response arrives, `done` is called a second time after the downstream headers are already sent, the proxied response is never terminated, and the test waits forever rather than failing. Both SSE tests now use a 1000ms timeout with a 2000ms quiet period, keeping the quiet period comfortably longer than the timeout, which is the property under test. The http1 sibling passed everywhere but has the same shape, so it gets the same headroom rather than waiting for it to flake later. Still red without the fix, and for the right reason: http1 ERR_HTTP_HEADERS_SENT, uncaught, headers already sent http2 received 'data: first\n\n', expected 'data: first\n\ndata: last\n\n' Verified locally: lint clean, 166 unit tests pass over three consecutive runs, tstyche 13/13. Signed-off-by: Manuel Sánchez <mpktmpktmpktmpkt@gmail.com>
Addresses the review: the two integration tests I added armed a proxy timeout and then a longer setTimeout on the target, racing two clocks. On a loaded runner the timeout could fire before the first response and the test hung — exactly the flakiness flagged. The regression is purely about parsing the response content-type (a media type is case-insensitive and may carry parameters, RFC 9110 §8.3.1). That is a pure function, so it is now tested as one: `isServerSentEvents` is exported and test/sse-content-type.test.js covers bare, parametrised, uppercase, negative and empty inputs with no server, socket or timer. Reverting the function to the old strict `=== 'text/event-stream'` turns those cases red, which is the bug. The two integration tests keep their end-to-end coverage but drop the timers: the target now answers the SSE response immediately and the test asserts 200 plus the body, the same shape as the sibling `* sse removes timeout test` already in each file. No setTimeout, no race.
The http2 request path removed hop-by-hop headers (RFC 7230 §6.1) via stripHttp1ConnectionHeaders, but the default (undici) and core-http paths only dropped the names listed in the client's Connection header. As a result `te`, `proxy-connection`, `keep-alive` and `upgrade` supplied by the client were forwarded to the upstream over the default transport, which is not what a proxy should do with headers meant to be consumed hop-by-hop. Reproduced with controls (proxy -> raw upstream capturing the exact request line): `te` and `keep-alive` reached the upstream; a Connection-listed header was correctly removed (control). This closes the HTTP/1 vs HTTP/2 gap by building the forwarded headers through the same stripHttp1ConnectionHeaders the http2 path already uses, so there is a single source of truth for what is hop-by-hop. `TE: trailers` is preserved, matching the existing http2 behaviour and Node. Tests added in strip-connection-headers.test.js for both transports; they fail on the previous code and pass here. Full suite green.
The new SSE http2 test registered t.after(target.close) before t.after(instance.close), and t.after hooks run FIFO. The SSE response disarms the plugin's http2 session timeout, so that session only dies when instance.close() destroys it. Before Node 24 an http2 server's close() waits for open sessions, so on the Node 20/22 CI runners target.close() waited on a session nothing else could close and the file hit the 30s test timeout with every subtest green. Registering instance.close() first makes the teardown order match the dependency.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The http2 request path strips hop-by-hop headers (RFC 7230 §6.1) via
stripHttp1ConnectionHeaders, but the default (undici) and core-http paths only removed the names listed in the client'sConnectionheader. Sote,proxy-connection,keep-aliveandupgradesent by a client were forwarded to the upstream over the default transport — headers a proxy is supposed to consume, not pass on.This is an HTTP/1 vs HTTP/2 parity gap, not a smuggling issue (I checked:
Content-Length+Transfer-Encodingtogether already yield a 400 and nothing reaches the upstream).Reproduction, with controls
Proxy
@fastify/reply-fromon its default transport → a rawnetupstream that captures the exact request line:Measuring what the http2 path already removes vs what the default path forwarded:
teproxy-connectionkeep-aliveupgradeThe fix
Build the forwarded request headers through the same
stripHttp1ConnectionHeadersthe http2 path already uses, so there is one source of truth for what is hop-by-hop. This also keeps the existing behaviour of dropping Connection-listed names beforerewriteRequestHeadersruns (that function is a superset of the old manual logic).TE: trailersis preserved, matching the http2 path and Node.Tests
Added to
test/strip-connection-headers.test.jsfor both the undici and core-http transports: the new tests fail on the previous code and pass here. I also kept aTE: trailerscase to lock in that it is not stripped. Fullnpm testis green (unit + tstyche).Scope note
I kept this focused on the parity gap.
proxy-authorizationandtrailerare hop-by-hop too (RFC 7235 / 7230) but were forwarded on both transports —stripHttp1ConnectionHeadersdoesn't list them. Happy to extend the list in a follow-up if you'd like; I left it out to keep this change to the asymmetry.