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.

Sign up for GitHub

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

http2: fix mapToHeaders() with single string value #16458

Closed
wants to merge 2 commits into from
Closed

http2: fix mapToHeaders() with single string value #16458

wants to merge 2 commits into from

Conversation

jinwoo
Copy link
Contributor

@jinwoo jinwoo commented Oct 24, 2017

This is for issue 16452. When 'set-cookie' header is set with an array
that has only one string value, it's split into its individual
characters.

Fix by resetting isArray to false when the value is converted from an
array to a string.

Fixes: #16452

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

http2

@jinwoo
http2: fix mapToHeaders() with single string value
eb2af57 
This is for issue 16452. When 'set-cookie' header is set with an array
that has only one string value, it's split into its individual
characters.

Fix by resetting `isArray` to false when the value is converted from an
array to a string.

Fixes: nodejs#16452
@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x http2 Issues or PRs related to the http2 subsystem. labels Oct 24, 2017
apapirovski
apapirovski approved these changes Oct 24, 2017
View reviewed changes
Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

Thanks @jinwoo! Looks great :)

@jinwoo
Copy link
Contributor Author

jinwoo commented Oct 24, 2017

Thanks, @apapirovski , for the quick approval!

@apapirovski
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/10936/

@jasnell
Copy link
Member

jasnell commented Oct 25, 2017

CI looks good.

lpinca
lpinca approved these changes Oct 25, 2017
View reviewed changes
test/parallel/test-http2-util-headers-list.js Outdated
@@ -154,6 +154,17 @@ const {
);
}

{
// Arrays containing a single set-cookie value are handled correctly
// (issue #16452)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: can you please use the full URL: https://github.com/nodejs/node/issues/16452?

test/parallel/test-http2-util-headers-list.js Outdated
{
// Arrays containing a single set-cookie value are handled correctly
// (issue #16452)
const headers = Object.create({});
Copy link
Member

Choose a reason for hiding this comment

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

Nit: just {} should be fine here.

@jinwoo
address review comments.
4f6b1bc 
- full url for the github issue
- simplify the header creation in test
@jinwoo
Copy link
Contributor Author

jinwoo commented Oct 25, 2017

Addressed the review comments.

mcollina
mcollina approved these changes Oct 26, 2017
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

@mcollina
Copy link
Member

Fresh CI: https://ci.nodejs.org/job/node-test-pull-request/10996/

@mcollina
Copy link
Member

CI (again): https://ci.nodejs.org/job/node-test-pull-request/11000/

@mcollina mcollina self-assigned this Oct 26, 2017
@mcollina
Copy link
Member

Landed as b61a08c.

@mcollina mcollina closed this Oct 26, 2017
@mcollina
Copy link
Member

Thanks so much for your first contribution to Node!

mcollina pushed a commit that referenced this pull request Oct 26, 2017
@jinwoo @mcollina
http2: fix mapToHeaders() with single string value
b61a08c 
This is for issue 16452. When 'set-cookie' header is set with an array
that has only one string value, it's split into its individual
characters.

Fix by resetting `isArray` to false when the value is converted from an
array to a string.

Fixes: #16452
PR-URL: #16458
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@jinwoo @addaleax
http2: fix mapToHeaders() with single string value
971c4f7 
This is for issue 16452. When 'set-cookie' header is set with an array
that has only one string value, it's split into its individual
characters.

Fix by resetting `isArray` to false when the value is converted from an
array to a string.

Fixes: nodejs/node#16452
PR-URL: nodejs/node#16458
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
@jinwoo @gibfahn
http2: fix mapToHeaders() with single string value
8587c2e 
This is for issue 16452. When 'set-cookie' header is set with an array
that has only one string value, it's split into its individual
characters.

Fix by resetting `isArray` to false when the value is converted from an
array to a string.

Fixes: #16452
PR-URL: #16458
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
@jinwoo @gibfahn
http2: fix mapToHeaders() with single string value
b4bed98 
This is for issue 16452. When 'set-cookie' header is set with an array
that has only one string value, it's split into its individual
characters.

Fix by resetting `isArray` to false when the value is converted from an
array to a string.

Fixes: #16452
PR-URL: #16458
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
gibfahn pushed a commit that referenced this pull request Oct 31, 2017
@jinwoo @gibfahn
http2: fix mapToHeaders() with single string value
2c2b658 
This is for issue 16452. When 'set-cookie' header is set with an array
that has only one string value, it's split into its individual
characters.

Fix by resetting `isArray` to false when the value is converted from an
array to a string.

Fixes: #16452
PR-URL: #16458
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@gibfahn gibfahn mentioned this pull request Oct 31, 2017
Merged
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@jinwoo @addaleax
http2: fix mapToHeaders() with single string value
1067602 
This is for issue 16452. When 'set-cookie' header is set with an array
that has only one string value, it's split into its individual
characters.

Fix by resetting `isArray` to false when the value is converted from an
array to a string.

Fixes: nodejs/node#16452
PR-URL: nodejs/node#16458
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

@apapirovski apapirovski apapirovski approved these changes

Assignees

@mcollina mcollina

Labels
http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[http2] If 'set-cookie' header is an array of a single string, it's split into individual characters
7 participants
@jinwoo @apapirovski @jasnell @mcollina @lpinca @cjihrig @nodejs-github-bot