Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

http: join all duplicate headers #45982

Merged
merged 12 commits into from Jan 3, 2023
Merged

http: join all duplicate headers #45982

merged 12 commits into from Jan 3, 2023

Conversation

marco-ippolito
Copy link
Member

@marco-ippolito marco-ippolito commented Dec 26, 2022 •
edited

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
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot 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
addaleax previously requested changes Dec 26, 2022
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

marco-ippolito commented Dec 26, 2022 •
edited

@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
Copy link
Member

@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
Copy link
Member

This should be an option, not a CLI flag.

@marco-ippolito
Copy link
Member Author

marco-ippolito commented Dec 28, 2022 •
edited

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

@issuefiler
Copy link

issuefiler commented Dec 29, 2022 •
edited

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?

@marco-ippolito marco-ippolito changed the title http: duplicate authorization headers http: join duplicate headers Dec 29, 2022
@marco-ippolito marco-ippolito changed the title http: join duplicate headers http: join all duplicate headers Dec 29, 2022
addaleax
addaleax reviewed Dec 31, 2022
View reviewed changes
doc/api/http.md Outdated Show resolved Hide resolved
@addaleax addaleax dismissed their stale review December 31, 2022 15:54

has been addressed

mcollina
mcollina requested changes Jan 1, 2023
View reviewed changes
lib/_http_client.js Show resolved Hide resolved
mcollina
mcollina approved these changes Jan 1, 2023
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

anonrig
anonrig requested changes Jan 1, 2023
View reviewed changes
Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking and requesting changes because the documentation is wrong.

lib/_http_client.js Show resolved Hide resolved
lib/_http_server.js Show resolved Hide resolved
doc/api/http.md Show resolved Hide resolved
lib/_http_incoming.js Outdated Show resolved Hide resolved
lib/_http_incoming.js Outdated Show resolved Hide resolved
doc/api/http.md Outdated Show resolved Hide resolved
lib/_http_client.js Outdated Show resolved Hide resolved
lib/_http_common.js Outdated Show resolved Hide resolved
anonrig
anonrig requested changes Jan 1, 2023
View reviewed changes
lib/_http_incoming.js Outdated Show resolved Hide resolved
@RafaelGSS RafaelGSS mentioned this pull request Jan 17, 2023
Closed
@RafaelGSS
Copy link
Member

See: #46240.

RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
@marco-ippolito @RafaelGSS
http: join authorization headers
069a30b 
PR-URL: #45982
Backport-PR-URL: #46240
Fixes: #45699
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
RafaelGSS added a commit that referenced this pull request Jan 20, 2023
@RafaelGSS
2023-01-24, Version 19.5.0 (Current)
f34fd35 
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
@RafaelGSS RafaelGSS mentioned this pull request Jan 20, 2023
Merged
RafaelGSS added a commit that referenced this pull request Jan 20, 2023
@RafaelGSS
2023-01-24, Version 19.5.0 (Current)
ecb598d 
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
RafaelGSS added a commit that referenced this pull request Jan 20, 2023
@RafaelGSS
2023-01-24, Version 19.5.0 (Current)
17145d8 
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 <rafael.nunu@hotmail.com>
RafaelGSS added a commit that referenced this pull request Jan 20, 2023
@RafaelGSS
2023-01-24, Version 19.5.0 (Current)
07c9241 
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 <rafael.nunu@hotmail.com>
RafaelGSS added a commit that referenced this pull request Jan 24, 2023
@RafaelGSS
2023-01-24, Version 19.5.0 (Current)
40a206c 
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 <rafael.nunu@hotmail.com>
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
@marco-ippolito @juanarbol
http: join authorization headers
c3d6c1d 
PR-URL: #45982
Fixes: #45699
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
@juanarbol juanarbol mentioned this pull request Jan 28, 2023
Merged
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
@marco-ippolito @juanarbol
http: join authorization headers
4df1fcc 
PR-URL: #45982
Fixes: #45699
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
juanarbol added a commit that referenced this pull request Feb 1, 2023
@juanarbol
2023-02-01, Version 18.14.0 'Hydrogen' (LTS)
79eb3c6 
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
juanarbol added a commit that referenced this pull request Feb 1, 2023
@juanarbol
2023-02-02, Version 18.14.0 'Hydrogen' (LTS)
43bbe45 
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
juanarbol added a commit that referenced this pull request Feb 1, 2023
@juanarbol
2023-02-02, Version 18.14.0 'Hydrogen' (LTS)
220a5a1 
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
juanarbol added a commit that referenced this pull request Feb 2, 2023
@juanarbol
2023-02-02, Version 18.14.0 'Hydrogen' (LTS)
66ab03d 
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
@marco-ippolito marco-ippolito mentioned this pull request Apr 27, 2023
Merged
lpinca added a commit to lpinca/node that referenced this pull request Jun 30, 2023
@lpinca
http: null the joinDuplicateHeaders property on cleanup
03ad516 
Null the `joinDuplicateHeaders` property when the parser is freed.

Refs: nodejs#45982
@lpinca lpinca mentioned this pull request Jun 30, 2023
Merged
nodejs-github-bot pushed a commit that referenced this pull request Jul 3, 2023
@lpinca
http: null the joinDuplicateHeaders property on cleanup
651c02c 
Null the `joinDuplicateHeaders` property when the parser is freed.

Refs: #45982
PR-URL: #48608
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023
@lpinca @RafaelGSS
http: null the joinDuplicateHeaders property on cleanup
7d8a0b6 
Null the `joinDuplicateHeaders` property when the parser is freed.

Refs: #45982
PR-URL: #48608
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@lpinca @Ceres6
http: null the joinDuplicateHeaders property on cleanup
baf8d33 
Null the `joinDuplicateHeaders` property when the parser is freed.

Refs: nodejs#45982
PR-URL: nodejs#48608
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@lpinca @Ceres6
http: null the joinDuplicateHeaders property on cleanup
ba1cf4b 
Null the `joinDuplicateHeaders` property when the parser is freed.

Refs: nodejs#45982
PR-URL: nodejs#48608
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
ruyadorno pushed a commit that referenced this pull request Sep 11, 2023
@lpinca @ruyadorno
http: null the joinDuplicateHeaders property on cleanup
27d6833 
Null the `joinDuplicateHeaders` property when the parser is freed.

Refs: #45982
PR-URL: #48608
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
@lpinca @ruyadorno
http: null the joinDuplicateHeaders property on cleanup
eb4550a 
Null the `joinDuplicateHeaders` property when the parser is freed.

Refs: #45982
PR-URL: #48608
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
ruyadorno pushed a commit that referenced this pull request Sep 17, 2023
@lpinca @ruyadorno
http: null the joinDuplicateHeaders property on cleanup
01746c7 
Null the `joinDuplicateHeaders` property when the parser is freed.

Refs: #45982
PR-URL: #48608
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@panva panva panva left review comments

@addaleax addaleax addaleax left review comments

@mcollina mcollina mcollina approved these changes

@ShogunPanda ShogunPanda ShogunPanda approved these changes

@anonrig anonrig anonrig approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Duplicate Authorization headers should not be ignored.
10 participants
@marco-ippolito @nodejs-github-bot @addaleax @mcollina @issuefiler @RafaelGSS @ShogunPanda @panva @anonrig @targos