fix(proxy): Correct hop-by-hop header handling per RFC 9110#4459
fix(proxy): Correct hop-by-hop header handling per RFC 9110#4459yusukebe merged 8 commits intohonojs:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4459 +/- ##
==========================================
+ Coverage 91.32% 91.33% +0.01%
==========================================
Files 173 173
Lines 11084 11107 +23
Branches 3201 3209 +8
==========================================
+ Hits 10122 10145 +23
Misses 961 961
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
usualoma
left a comment
There was a problem hiding this comment.
Verified Connection header parsing
Thank you for pointing that out! I wasn't aware of that specification.
However, if headers like the following can be specified in the connection value and deleted, it seems this could lead to issues classified as "Hop-by-Hop Header Injection".
- X-Forwarded-For
- X-Real-IP
While it's desirable to have functionality that respects the "Connection" header, I feel it should only be enabled explicitly as an option.
test(proxy): fix test for hop-by-hop headers
a30e2a0 to
941d00b
Compare
Thank you for pointing this out! You're absolutely right about the Hop-by-Hop Header Injection risk. I've added a
|
941d00b to
c4a278c
Compare
There was a problem hiding this comment.
Hi @sugar-cat7, thanks for your help.
I believe strictConnectionProcessing should be fine!
Sorry, just one more thing when you have time.
When strictConnectionProcessing is enabled, the following request results in an "Internal Server Error". Since this option is mainly used in trusted environments, we generally don't need to worry about invalid values. However, configuration errors could still cause problems. In those cases, I think we should either return a 400 error or just ignore it.
$ curl -H 'connection: a a' http://localhost:3000/internal-proxy/x
@usualoma
|
b8eb419 to
ee4ed17
Compare
usualoma
left a comment
There was a problem hiding this comment.
I've left a comment, so please check it!
|
|
||
| const result = processConnectionHeader(raw, strictConnectionProcessing) | ||
| if (!result.isSuccess) { | ||
| return new Response(result.message, { status: 400 }) |
There was a problem hiding this comment.
Sorry for not explaining earlier, but I want to throw an HTTPException.
hono/src/validator/validator.ts
Line 81 in 4b796cf
There was a problem hiding this comment.
Changed to throw an error.
hono/src/helper/proxy/index.ts
Line 70 in ea37348
| ...requestInit | ||
| } = proxyInit instanceof Request ? { raw: proxyInit } : proxyInit ?? {} | ||
|
|
||
| const result = processConnectionHeader(raw, strictConnectionProcessing) |
There was a problem hiding this comment.
The variable name “result” seems to be a temporary variable used to hold the function's "result," so I believe it should be renamed.
There was a problem hiding this comment.
Reverted to initial implementation and consolidated logic into buildRequestInitFromRequest since errors are now thrown.
ea37348
usualoma
left a comment
There was a problem hiding this comment.
Thank you. I think it’s a great improvement!
|
Thanks! Merging and releasing a new version (it's a minor bump!). |
The code references RFC 2616, which has been obsoleted by RFC 7230 (2014) and then RFC 9110 (2022).
Fixed hop-by-hop headers list
'trailers'to'trailer'inhopByHopHeadersarrayTrailer(singular)'upgrade'headerVerified Connection header parsing
RFC 9110 Section 7.6.1:
Add tests
Run tests
bun run format:fix && bun run lint:fixto format the codeAdd TSDoc/JSDoc to document the code