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

Multiple cookie headers need special-case concatenation using ; instead of , #11256

Closed
isaacs opened this issue Feb 9, 2017 · 0 comments
Closed
Labels
http Issues or PRs related to the http subsystem.

Comments

@isaacs
Copy link
Contributor

isaacs commented Feb 9, 2017

Version: v8.0.0-pre
Platform: Darwin geegaw.local 16.4.0 Darwin Kernel Version 16.4.0: Thu Dec 22 22:53:21 PST 2016; root:xnu-3789.41.3~3/RELEASE_X86_64 x86_64
Subsystem: http

Example program:

require('http').createServer(function (q, s) {
  console.error(q.headers.cookie)
  s.end(q.headers.cookie + '\n')
}).listen(8080))
$ curl localhost:8080 -H cookie:foo=bar -H cookie:baz=boo
foo=bar, baz=boo

Expected output:

$ curl localhost:8080 -H cookie:foo=bar -H cookie:baz=boo
foo=bar; baz=boo

Explanation

Unlike most headers, multiple cookie headers should be concatenated using a ; rather than a ,. Most web browsers handle this already, by only ever sending a single cookie header which is already joined with semicolons.

However, clients MAY send multiple cookie headers, which Node then automatically concatenates using commas. (The specified mechanism for joining multiple headers in most cases.)

This throws off userland cookie-parsing logic, especially because , is a valid character in the cookie value, and in fact must be used to specify a cookie expiration date.

Suggestion

Server: Special-case concatenating multiple cookie request headers using a ; instead of a ,.

Client: When sending multiple cookie headers with headers: { cookie: ['x=y', 'a=b' ] }, concatenate them in the request using ; rather than ,.

I suspect that this didn't come up before because web browsers and userland client cookiejars already do this properly.

@targos targos added the http Issues or PRs related to the http subsystem. label Feb 9, 2017
mscdex added a commit to mscdex/io.js that referenced this issue Mar 9, 2017
Previously, separate incoming Cookie headers would be concatenated
with a comma, which can cause confusion in userland code when it
comes to parsing the final Cookie header value. This commit
concatenates using a semicolon instead.

Fixes: nodejs#11256
PR-URL: nodejs#11259
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
mscdex added a commit to mscdex/io.js that referenced this issue Mar 9, 2017
This commit enables automatic concatenation of multiple Cookie header
values with a semicolon, except when 2D header arrays are used.

Fixes: nodejs#11256
PR-URL: nodejs#11259
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
jungx098 pushed a commit to jungx098/node that referenced this issue Mar 21, 2017
Previously, separate incoming Cookie headers would be concatenated
with a comma, which can cause confusion in userland code when it
comes to parsing the final Cookie header value. This commit
concatenates using a semicolon instead.

Fixes: nodejs#11256
PR-URL: nodejs#11259
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
jungx098 pushed a commit to jungx098/node that referenced this issue Mar 21, 2017
This commit enables automatic concatenation of multiple Cookie header
values with a semicolon, except when 2D header arrays are used.

Fixes: nodejs#11256
PR-URL: nodejs#11259
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

No branches or pull requests

2 participants