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: split set-cookie when using setHeaders #51649

Merged

Conversation

marco-ippolito
Copy link
Member

@marco-ippolito marco-ippolito commented Feb 2, 2024

Fixes: #51599
This allows set-cookie header to work correctly with setHeaders

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Feb 2, 2024
Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

lib/_http_outgoing.js Outdated Show resolved Hide resolved
lib/_http_outgoing.js Outdated Show resolved Hide resolved
lib/_http_outgoing.js Outdated Show resolved Hide resolved
lib/_http_outgoing.js Outdated Show resolved Hide resolved
@marco-ippolito marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 3, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 3, 2024
@nodejs-github-bot
Copy link
Collaborator

lib/_http_outgoing.js Outdated Show resolved Hide resolved
Co-authored-by: akhil marsonya <16393876+marsonya@users.noreply.github.com>
@marco-ippolito marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 3, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 3, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@marco-ippolito marco-ippolito added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Feb 4, 2024
@marco-ippolito marco-ippolito added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 4, 2024
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Feb 4, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/51649
✔  Done loading data for nodejs/node/pull/51649
----------------------------------- PR info ------------------------------------
Title      http: split set-cookie when using setHeaders (#51649)
Author     Marco Ippolito  (@marco-ippolito)
Branch     marco-ippolito:feat/set-headers-cookies -> nodejs:main
Labels     http, author ready
Commits    2
 - http: split set-cookie when using setHeaders
 - Update lib/_http_outgoing.js
Committers 2
 - marco-ippolito 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/51649
Fixes: https://github.com/nodejs/node/issues/51599
Reviewed-By: Paolo Insogna 
Reviewed-By: Luigi Pinca 
Reviewed-By: Akhil Marsonya 
Reviewed-By: Minwoo Jung 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/51649
Fixes: https://github.com/nodejs/node/issues/51599
Reviewed-By: Paolo Insogna 
Reviewed-By: Luigi Pinca 
Reviewed-By: Akhil Marsonya 
Reviewed-By: Minwoo Jung 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 02 Feb 2024 11:18:39 GMT
   ✔  Approvals: 4
   ✔  - Paolo Insogna (@ShogunPanda): https://github.com/nodejs/node/pull/51649#pullrequestreview-1860949554
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/51649#pullrequestreview-1860948676
   ✔  - Akhil Marsonya (@marsonya): https://github.com/nodejs/node/pull/51649#pullrequestreview-1860999209
   ✔  - Minwoo Jung (@JungMinu): https://github.com/nodejs/node/pull/51649#pullrequestreview-1861448465
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-02-04T09:38:30Z: https://ci.nodejs.org/job/node-test-pull-request/57047/
- Querying data for job/node-test-pull-request/57047/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 51649
From https://github.com/nodejs/node
 * branch                  refs/pull/51649/merge -> FETCH_HEAD
✔  Fetched commits as c975384264dc..d546c502a56e
--------------------------------------------------------------------------------
[main 4c1cc83b8f] http: split set-cookie when using setHeaders
 Author: marco-ippolito 
 Date: Fri Feb 2 11:55:37 2024 +0100
 2 files changed, 64 insertions(+), 2 deletions(-)
[main 918a9ad1d1] Update lib/_http_outgoing.js
 Author: Marco Ippolito 
 Date: Sat Feb 3 12:23:09 2024 +0100
 1 file changed, 1 insertion(+), 1 deletion(-)
   ✔  Patches applied
There are 2 commits in the PR. Attempting autorebase.
Rebasing (2/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
http: split set-cookie when using setHeaders

PR-URL: #51649
Fixes: #51599
Reviewed-By: Paolo Insogna paolo@cowtech.it
Reviewed-By: Luigi Pinca luigipinca@gmail.com
Reviewed-By: Akhil Marsonya akhil.marsonya27@gmail.com
Reviewed-By: Minwoo Jung nodecorelab@gmail.com

[detached HEAD 23373b6075] http: split set-cookie when using setHeaders
Author: marco-ippolito marcoippolito54@gmail.com
Date: Fri Feb 2 11:55:37 2024 +0100
2 files changed, 64 insertions(+), 2 deletions(-)
Rebasing (3/4)
Rebasing (4/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
Update lib/_http_outgoing.js

Co-authored-by: akhil marsonya 16393876+marsonya@users.noreply.github.com
PR-URL: #51649
Fixes: #51599
Reviewed-By: Paolo Insogna paolo@cowtech.it
Reviewed-By: Luigi Pinca luigipinca@gmail.com
Reviewed-By: Akhil Marsonya akhil.marsonya27@gmail.com
Reviewed-By: Minwoo Jung nodecorelab@gmail.com

[detached HEAD 33837b68ff] Update lib/_http_outgoing.js
Author: Marco Ippolito marcoippolito54@gmail.com
Date: Sat Feb 3 12:23:09 2024 +0100
1 file changed, 1 insertion(+), 1 deletion(-)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/7775666607

@marco-ippolito marco-ippolito added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Feb 4, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 4, 2024
@nodejs-github-bot nodejs-github-bot merged commit 9448c61 into nodejs:main Feb 4, 2024
70 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 9448c61

rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Feb 9, 2024
PR-URL: nodejs#51649
Fixes: nodejs#51599
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
targos pushed a commit that referenced this pull request Feb 15, 2024
PR-URL: #51649
Fixes: #51599
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
marco-ippolito added a commit to marco-ippolito/node that referenced this pull request Feb 19, 2024
PR-URL: nodejs#51649
Fixes: nodejs#51599
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@marco-ippolito marco-ippolito mentioned this pull request Mar 1, 2024
richardlau pushed a commit that referenced this pull request Mar 25, 2024
PR-URL: #51649
Fixes: #51599
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
PR-URL: #51649
Fixes: #51599
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@richardlau richardlau mentioned this pull request Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OutgoingMessage.setHeaders(Headers) doesn't handle Set-Cookie headers properly.
10 participants