Repository navigation
fix: strip hop-by-hop headers on the default transport, not only http2 - #489
Open
pacocartones wants to merge 2 commits into
Open
pacocartones wants to merge 2 commits into
pacocartones wants to merge 2 commits into
Conversation
pacocartones
force-pushed
the
fix/strip-hop-by-hop-headers-default-transport
branch
from
August 23, 2026 01:15
b6c798f to
2202193
Compare
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.
pacocartones
force-pushed
the
fix/strip-hop-by-hop-headers-default-transport
branch
from
August 23, 2026 01:36
2202193 to
a0e8803
Compare
ilteoood
approved these changes
Aug 27, 2026
Tony133
approved these changes
Aug 27, 2026
Contributor
Author
|
Thanks for taking the time to review the hop-by-hop handling here. The branch is still even with |
pacocartones
added a commit
to pacocartones/fastify-reply-from
that referenced
this pull request
Sep 4, 2026
… path When the downstream client speaks HTTP/1, the response headers from the upstream were forwarded verbatim via copyHeaders(rewriteHeaders(res.headers, ...)). Any hop-by-hop header the upstream set - keep-alive, proxy-connection, transfer-encoding, or a header it named in its own Connection header - leaked to the client, which RFC 7230 Section 6.1 forbids a proxy from forwarding. The HTTP/2 downstream branch a few lines above already strips these with stripHttp1ConnectionHeaders(res.headers); this applies the same call to the HTTP/1 branch so both downstream transports behave identically. Distinct from fastify#489, which strips hop-by-hop headers on the REQUEST path (client -> upstream). This is the RESPONSE path (upstream -> client); the two are independent. Adds a regression test that reads the raw HTTP/1 response the proxy sends downstream (undici hides some headers) and asserts the upstream's Keep-Alive, proxy-connection and Connection-listed custom header are all stripped, while end-to-end headers (content-type) survive. The test fails on the current code (the upstream Keep-Alive leaks) and passes with the fix.
mcollina
pushed a commit
that referenced
this pull request
Nov 4, 2026
… path (#490) When the downstream client speaks HTTP/1, the response headers from the upstream were forwarded verbatim via copyHeaders(rewriteHeaders(res.headers, ...)). Any hop-by-hop header the upstream set - keep-alive, proxy-connection, transfer-encoding, or a header it named in its own Connection header - leaked to the client, which RFC 7230 Section 6.1 forbids a proxy from forwarding. The HTTP/2 downstream branch a few lines above already strips these with stripHttp1ConnectionHeaders(res.headers); this applies the same call to the HTTP/1 branch so both downstream transports behave identically. Distinct from #489, which strips hop-by-hop headers on the REQUEST path (client -> upstream). This is the RESPONSE path (upstream -> client); the two are independent. Adds a regression test that reads the raw HTTP/1 response the proxy sends downstream (undici hides some headers) and asserts the upstream's Keep-Alive, proxy-connection and Connection-listed custom header are all stripped, while end-to-end headers (content-type) survive. The test fails on the current code (the upstream Keep-Alive leaks) and passes with the fix.
This branch has not been deployed
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
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
te,proxy-connection,keep-aliveandupgradesupplied by the client are forwarded to the upstream on the default (undici) and core-http transports. RFC 7230 §6.1 says a proxy must consume these; they belong to a single hop.The HTTP/2 request path already drops them via
stripHttp1ConnectionHeaders. The HTTP/1 paths only removed the names listed in the client'sConnectionheader, so the fixed hop-by-hop set fell through. This routes both through the samestripHttp1ConnectionHeaders, giving one source of truth for what is hop-by-hop.TE: trailersis preserved, matching the existing HTTP/2 behaviour and Node.Rebased on top of #492
#492 landed while this sat, and it covers the
Connection-listed part on the undici path, so that portion of the original PR is gone. I have rebased onto currentmainand cut this down to only what #492 does not do. Confirmed against the post-#492 tree, sendingTE: gzip,Keep-Alive: timeout=5,Proxy-Connection: keep-alivethrough the proxy and reading what the upstream received:keep-aliveis already dropped by the client stack;teandproxy-connectionare the live leak.Test
test/strip-connection-headers.test.jsgains hop-by-hop cases for both transports plus aTE: trailerspreservation case. The two hop-by-hop tests fail on currentmainand pass here. All of #492's own tests in that file still pass unchanged.npx c8 --100 node --test→ 180 tests, 175 pass, 5 skipped for env, 0 fail, 100% coverage on the--100gate.npm run test:typescript→ 13/13.Scope
index.jsplus its test. Independent of #490, which covers the response path; either can merge first.