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

http: join all duplicate headers #45982

Merged
merged 12 commits into from
Jan 3, 2023

Conversation

marco-ippolito
Copy link
Member

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

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
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

@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

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

@issuefiler
Copy link

issuefiler commented Dec 29, 2022

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
doc/api/http.md Outdated Show resolved Hide resolved
@addaleax addaleax dismissed their stale review December 31, 2022 15:54

has been addressed

lib/_http_client.js Show resolved Hide resolved
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

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
lib/_http_incoming.js Outdated Show resolved Hide resolved
@RafaelGSS
Copy link
Member

See: #46240.

RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
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
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
RafaelGSS added a commit that referenced this pull request Jan 20, 2023
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
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
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
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
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
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
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
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
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
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
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
lpinca added a commit to lpinca/node that referenced this pull request Jun 30, 2023
Null the `joinDuplicateHeaders` property when the parser is freed.

Refs: nodejs#45982
nodejs-github-bot pushed a commit that referenced this pull request Jul 3, 2023
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
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
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
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
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
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
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
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
Development

Successfully merging this pull request may close these issues.

Duplicate Authorization headers should not be ignored.
10 participants