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] If 'set-cookie' header is an array of a single string, it's split into individual characters #16452

Closed
jinwoo opened this issue Oct 24, 2017 · 8 comments
Closed

[http2] If 'set-cookie' header is an array of a single string, it's split into individual characters #16452

jinwoo opened this issue Oct 24, 2017 · 8 comments
Labels
http2 Issues or PRs related to the http2 subsystem.

Comments

@jinwoo
Copy link
Contributor

jinwoo commented Oct 24, 2017

  • Version: 8.7.0
  • Platform: Darwin Kernel Version 16.7.0: Thu Jun 15 17:36:27 PDT 2017; root:xnu-3789.70.16~2/RELEASE_X86_64 x86_64
  • Subsystem:

The 'set-cookie' header field can be an array of multiple values. But when it is an array of only one string, the string is split into an array of its individual characters.

E.g.

headers['set-cookie'] = ['mycookie=foo'];

will end up with 12 'set-cookie' fields each of which has 'm', 'y', 'c', 'o',... etc as its value.

I think the bug is in mapToHeaders() function in util.js:

node/lib/internal/http2/util.js

Line 391 in f16b9c1

function mapToHeaders(map,

  1. At line 404, isArray is set to true in this case:

    node/lib/internal/http2/util.js

    Line 404 in f16b9c1

    const isArray = Array.isArray(value);
  2. At line 410, value is converted to a single string:

    node/lib/internal/http2/util.js

    Line 410 in f16b9c1

    value = String(value[0]);
  3. But then at line 433, it's again split into individual characters even though it's been already converted to a single string in step 2 above:

    node/lib/internal/http2/util.js

    Line 433 in f16b9c1

    for (var k = 0; k < value.length; k++) {
@jasnell
Copy link
Member

jasnell commented Oct 24, 2017

heh.. that's certainly not good! Will investigate! :-)

/cc @mcollina @apapirovski

@jinwoo
Copy link
Contributor Author

jinwoo commented Oct 24, 2017

I think the fix can be just a one-line change: setting isArray to true after line 410.

@jinwoo
Copy link
Contributor Author

jinwoo commented Oct 24, 2017

Sorry I meant 'setting isArray to false

@apapirovski
Copy link
Member

Hi @jinwoo, thanks for the report! I can indeed confirm that this bug still exists in master. I'm going to open a PR, unless you would like to fix this in which case just let me know and I would be happy to help answer any questions about the process.

I think we would like to get this into the next 8.x & 9.x releases.

@jinwoo
Copy link
Contributor Author

jinwoo commented Oct 24, 2017

Hi @apapirovski , I can whip up a quick fix if that's simpler.

@apapirovski
Copy link
Member

apapirovski commented Oct 24, 2017 •
edited
Loading

@jinwoo either way works for me :) If you would like to contribute to Node, please go ahead. You might want to also edit the test at test/parallel/test-http2-util-headers-list.js to test for this particular case.

(I just didn't want to come across as pressuring you into fixing this yourself. 😊)

@jinwoo
Copy link
Contributor Author

jinwoo commented Oct 24, 2017

All right. Will send a PR soon. Thanks!

jinwoo added a commit to jinwoo/node that referenced this issue Oct 24, 2017
@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
@jinwoo jinwoo mentioned this issue Oct 24, 2017
Closed
4 tasks
@mscdex mscdex added the http2 Issues or PRs related to the http2 subsystem. label Oct 24, 2017
@fcicq
Copy link

fcicq commented Oct 26, 2017 •
edited
Loading

Ah. it affects me as I use the headers from http.fetch result.
one way to avoid from the client side is,

if (headers['set-cookie'] && headers['set-cookie'].length == 1) {
  headers['set-cookie'] = headers['set-cookie'][0]
}

addaleax pushed a commit to ayojs/ayo that referenced this issue 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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
gibfahn pushed a commit that referenced this issue 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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
gibfahn pushed a commit that referenced this issue 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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
gibfahn pushed a commit that referenced this issue 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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this issue 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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

http2: fix mapToHeaders() with single string value jinwoo/node
5 participants
@apapirovski @mscdex @jasnell @fcicq @jinwoo