GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — http: join all duplicate headers by marco-ippolito · Pull Request #45982 · nodejs/node

Conversation

@marco-ippolito

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
marco-ippolito added 2 commits December 26, 2022 15:46
@marco-ippolito
http: join authorization headers
17677bf
@marco-ippolito
http: added more doc
de0749b
@nodejs-github-bot

nodejs-github-bot commented Dec 26, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http Issues and 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

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnoordhuis and bricss reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@marco-ippolito

marco-ippolito commented Dec 26, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@addaleax

addaleax commented Dec 26, 2022

Copy link
Copy Markdown
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.

climba03003 and bricss reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mcollina

mcollina commented Dec 28, 2022

Copy link
Copy Markdown
Member

This should be an option, not a CLI flag.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@marco-ippolito

marco-ippolito commented Dec 28, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

marco-ippolito added 4 commits December 28, 2022 20:36
@marco-ippolito
Revert "http: added more doc"
5510be6
This reverts commit de0749b.
@marco-ippolito
Revert "http: join authorization headers"
3e1b292
This reverts commit 17677bf.
@marco-ippolito
http: joinAuthorizationHeaders
fa8aa5c
@marco-ippolito
http: more doc
2386a24
@marco-ippolito
marco-ippolito requested a review from addaleax December 28, 2022 23:06
@issuefiler

issuefiler commented Dec 29, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

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?

addaleax reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@marco-ippolito marco-ippolito changed the title http: duplicate authorization headers http: join duplicate headers Dec 29, 2022
@marco-ippolito
http: generic join header
fdc2c00
@marco-ippolito marco-ippolito changed the title http: join duplicate headers http: join all duplicate headers Dec 29, 2022
@marco-ippolito
http: default value
2d52bd8
mcollina
mcollina reviewed Dec 29, 2022
View reviewed changes
Comment thread test/parallel/test-http-request-join-authorization-headers.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

@marco-ippolito
http: more tests
8e54765
addaleax
addaleax reviewed Dec 31, 2022
View reviewed changes
Comment thread doc/api/http.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

@addaleax
addaleax dismissed their stale review December 31, 2022 15:54

has been addressed

mcollina
mcollina requested changes Jan 1, 2023
View reviewed changes
Comment thread lib/_http_client.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

mcollina
mcollina approved these changes Jan 1, 2023
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@marco-ippolito
http: rfc compliace
e948db0
anonrig
anonrig requested changes Jan 1, 2023
View reviewed changes

@anonrig anonrig left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread lib/_http_client.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/_http_server.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread doc/api/http.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/_http_incoming.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/_http_incoming.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread doc/api/http.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/_http_client.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/_http_common.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

anonrig
anonrig requested changes Jan 1, 2023
View reviewed changes
Comment thread lib/_http_incoming.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

@RafaelGSS

RafaelGSS commented Jan 17, 2023

Copy link
Copy Markdown
Member

See: #46240.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@RafaelGSS RafaelGSS added backport-open-v19.x and removed backport-requested-v19.x labels Jan 17, 2023
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 <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Jan 20, 2023
v19.5.0 proposal #46286
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)
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
@juanarbol 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
@marco-ippolito @juanarbol
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]>
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
@targos targos added backported-to-v19.x and removed backport-open-v19.x labels Mar 13, 2023
@marco-ippolito 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
@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
http: null the joinDuplicateHeaders property on cleanup #48608
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matthew Aitken <[email protected]>
@coderabbitai coderabbitai Bot mentioned this pull request Jul 24, 2026
fix: system-wide audit — harden auth, validation, idempotency, outbox and queue paths chuanghiduoc/nestjs-fastify-nx#127
Merged
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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash PRs the Commit Queue should land as one squashed commit. http Issues and 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.

Uh oh!

There was an error while loading. Please reload this page.

10 participants

@marco-ippolito @nodejs-github-bot @addaleax @mcollina @issuefiler @RafaelGSS @ShogunPanda @panva @anonrig @targos
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.