http: join all duplicate headers by marco-ippolito · Pull Request #45982 · nodejs/node

marco-ippolito · 2022-12-26T15:11:21Z

resolves: #45699

I have introduced a new option named joinDuplicateHeaders in http.request and http.createServer to allow joining multiple headers instead of discarding the ones after the first as described by rfc 9110

nodejs-github-bot · 2022-12-26T15:11:25Z

Review requested:

@nodejs/http
@nodejs/net

addaleax

I'm -1 on adding a CLI flag. This changes behavior of existing code in a potentially sensitive way, and users do not necessarily know which parts of their application require current behavior or not.

I think the suggestion from the ticket is actually way better (and as I'm spelling this out, I realize it's your own suggestion 🙂), which is to add a flag to the HTTP server/client options instead.

marco-ippolito · 2022-12-26T16:01:11Z

@addaleax yes it was my suggestion to use the option in server/client 😄. While working on it I realized it was very messy and produced unexpected behavior (the option missing from server but present in the client) so I switched to a flag which seemed to me a cleaner and "universal" solution.

addaleax · 2022-12-26T18:20:35Z

@marco-ippolito How does using a CLI flag solve that? Typically, client and server won't be running in the same process or be even developed/deployed by the same person, so you'll end up with a good chance of mismatches for this option either way.

mcollina · 2022-12-28T18:34:37Z

This should be an option, not a CLI flag.

marco-ippolito · 2022-12-28T19:36:07Z

ok changing it to option. Should I mark it as experimental in the docs?

This reverts commit de0749b.

This reverts commit 17677bf.

issuefiler · 2022-12-29T06:51:47Z

Actually, RFC 9110 requires multiple occurrences of all comma-separated headers to be joinable. Authorization is just one of them I came up with for example. How about having something that is more general and conforming to the new RFC?

test/parallel/test-http-request-join-authorization-headers.js

doc/api/http.md

has been addressed

lib/_http_client.js

mcollina

lgtm

anonrig

Blocking and requesting changes because the documentation is wrong.

lib/_http_client.js

lib/_http_server.js

doc/api/http.md

lib/_http_incoming.js

doc/api/http.md

lib/_http_client.js

lib/_http_common.js

lib/_http_incoming.js

RafaelGSS · 2023-01-17T19:40:51Z

See: #46240.

PR-URL: #45982 Backport-PR-URL: #46240 Fixes: #45699 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Paolo Insogna <[email protected]>

Notable changes: * crypto: * (SEMVER-MINOR) add CryptoKey Symbol.toStringTag (Filip Skokan) [#46042](#46042) * (SEMVER-MINOR) add KeyObject Symbol.toStringTag (Filip Skokan) [#46043](#46043) * http: * (SEMVER-MINOR) join authorization headers (Marco Ippolito) [#45982](#45982) * lib: * add webstreams to Duplex.from() (Debadree Chatterjee) [#46190](#46190) * stream: * implement finished() for ReadableStream and WritableStream (Debadree Chatterjee) [#46205](#46205) PR-URL: TBD

Notable changes: * crypto: * (SEMVER-MINOR) add CryptoKey Symbol.toStringTag (Filip Skokan) [#46042](#46042) * (SEMVER-MINOR) add KeyObject Symbol.toStringTag (Filip Skokan) [#46043](#46043) * http: * (SEMVER-MINOR) join authorization headers (Marco Ippolito) [#45982](#45982) * lib: * add webstreams to Duplex.from() (Debadree Chatterjee) [#46190](#46190) * stream: * implement finished() for ReadableStream and WritableStream (Debadree Chatterjee) [#46205](#46205) PR-URL: #46286

Notable changes: * http: * (SEMVER-MINOR) join authorization headers (Marco Ippolito) [#45982](#45982) * lib: * add webstreams to Duplex.from() (Debadree Chatterjee) [#46190](#46190) * stream: * implement finished() for ReadableStream and WritableStream (Debadree Chatterjee) [#46205](#46205) PR-URL: #46286 Signed-off-by: RafaelGSS <[email protected]>

PR-URL: #45982 Fixes: #45699 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Paolo Insogna <[email protected]>

Notable changes: * deps: * upgrade npm to 9.3.1 (npm team) #46242 * doc: * add parallelism note to os.cpus() (Colin Ihrig) #45895 * http: * join authorization headers (Marco Ippolito) #45982 * improved timeout defaults handling (Paolo Insogna) #45778 * stream: * implement finished() for ReadableStream and WritableStream (Debadree Chatterjee) #46205 PR-URL: #46396

Null the `joinDuplicateHeaders` property when the parser is freed. Refs: nodejs#45982

Null the `joinDuplicateHeaders` property when the parser is freed. Refs: #45982 PR-URL: #48608 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matthew Aitken <[email protected]>

Null the `joinDuplicateHeaders` property when the parser is freed. Refs: nodejs#45982 PR-URL: nodejs#48608 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matthew Aitken <[email protected]>

Null the `joinDuplicateHeaders` property when the parser is freed. Refs: #45982 PR-URL: #48608 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matthew Aitken <[email protected]>

marco-ippolito added 2 commits December 26, 2022 15:46

http: join authorization headers

17677bf

http: added more doc

de0749b

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Dec 26, 2022

addaleax previously requested changes Dec 26, 2022

View reviewed changes

marco-ippolito added 4 commits December 28, 2022 20:36

Revert "http: added more doc"

5510be6

This reverts commit de0749b.

Revert "http: join authorization headers"

3e1b292

This reverts commit 17677bf.

http: joinAuthorizationHeaders

fa8aa5c

http: more doc

2386a24

marco-ippolito requested a review from addaleax December 28, 2022 23:06

marco-ippolito changed the title ~~http: duplicate authorization headers~~ http: join duplicate headers Dec 29, 2022

http: generic join header

fdc2c00

marco-ippolito changed the title ~~http: join duplicate headers~~ http: join all duplicate headers Dec 29, 2022

http: default value

2d52bd8

mcollina reviewed Dec 29, 2022

View reviewed changes

test/parallel/test-http-request-join-authorization-headers.js Show resolved Hide resolved

http: more tests

8e54765

addaleax reviewed Dec 31, 2022

View reviewed changes

doc/api/http.md Outdated Show resolved Hide resolved

mcollina requested changes Jan 1, 2023

View reviewed changes

lib/_http_client.js Show resolved Hide resolved

mcollina approved these changes Jan 1, 2023

View reviewed changes

http: rfc compliace

e948db0

anonrig requested changes Jan 1, 2023

View reviewed changes

lib/_http_incoming.js Outdated Show resolved Hide resolved

RafaelGSS mentioned this pull request Jan 17, 2023

[v19.x backport] http: join authorization headers #46240

Closed

RafaelGSS added backport-open-v19.x and removed backport-requested-v19.x labels Jan 17, 2023

RafaelGSS mentioned this pull request Jan 20, 2023

v19.5.0 proposal #46286

Merged

juanarbol pushed a commit that referenced this pull request Jan 26, 2023

http: join authorization headers

c3d6c1d

PR-URL: #45982 Fixes: #45699 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Paolo Insogna <[email protected]>

juanarbol mentioned this pull request Jan 28, 2023

V18.14.0 proposal #46396

Merged

juanarbol pushed a commit that referenced this pull request Jan 31, 2023

http: join authorization headers

4df1fcc

PR-URL: #45982 Fixes: #45699 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Paolo Insogna <[email protected]>

targos added backported-to-v19.x and removed backport-open-v19.x labels Mar 13, 2023

marco-ippolito mentioned this pull request Apr 27, 2023

http: prevent writing to the body when not allowed by HTTP spec #47732

Merged

lpinca added a commit to lpinca/node that referenced this pull request Jun 30, 2023

http: null the joinDuplicateHeaders property on cleanup

03ad516

Null the `joinDuplicateHeaders` property when the parser is freed. Refs: nodejs#45982

lpinca mentioned this pull request Jun 30, 2023

http: null the joinDuplicateHeaders property on cleanup #48608

Merged

Uh oh!

Conversation

marco-ippolito commented Dec 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Dec 26, 2022

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

marco-ippolito commented Dec 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Dec 26, 2022

Uh oh!

mcollina commented Dec 28, 2022

Uh oh!

marco-ippolito commented Dec 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

issuefiler commented Dec 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

anonrig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RafaelGSS commented Jan 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

marco-ippolito commented Dec 26, 2022 •

edited

Loading

marco-ippolito commented Dec 26, 2022 •

edited

Loading

marco-ippolito commented Dec 28, 2022 •

edited

Loading

issuefiler commented Dec 29, 2022 •

edited

Loading