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

fix: server side Set-Cookie always set an array. #367

Merged
merged 2 commits into from Jun 3, 2019

Conversation

sullivanpt
Copy link
Contributor

@sullivanpt sullivanpt commented Jun 2, 2019

Explanation. nodejs expects setHeader('Set-Cookie', value) to be
an array. If given a string it will accept and properly send it.
However, any subsequent attempts to replace the string with an array
to send multiple Set-Cookie headers will fail; it will instead send a
single Set-Cookie header with a concatenated array. The client will only
parse the first cookie, and then you'll spend hours pulling your
hair out.

Example:

res.SetHeader('Set-Cookie', 'serialized cookie')
res.SetHeader('Set-Cookie', ['serialized cookie','another cookie'])

generates

Set-Cookie: serialized cookie,another cookie

when what you want is

Set-Cookie: serialized cookie
Set-Cookie: another cookie

Explanation. nodejs expects setHeader('Set-Cookie', value) to be
an array. If given a string it will accept and properly send it.
However, any subsequent attempts to replace the string with an array
to send multiple Set-Cookie headers will fail; it will instead send a
single Set-Cookie header with a concatenated array. The client will only
parse the first cookie, and then you'll spend hours pulling your
hair out.

Example:

res.SetHeader('Set-Cookie', 'serialized cookie')
res.SetHeader('Set-Cookie', ['serialized cookie','another cookie'])

generates

Set-Cookie: serialized cookie,another cookie

when what you want is

Set-Cookie: serialized cookie
Set-Cookie: another cookie
@pi0
Copy link
Member

pi0 commented Jun 3, 2019

Hi @sullivanpt & thanks for digging in. About adding headers, actually, this is the same implementation for Express [append] (which is also used for res.cookie) so I'm wondering if this problem applies for all express users as well!

But at least rfc6265 confirms this:

Origin servers SHOULD NOT fold multiple Set-Cookie header fields into
a single header field. The usual mechanism for folding HTTP headers
fields (i.e., as defined in [RFC2616]) might change the semantics of
the Set-Cookie header field because the %x2C (",") character is used
by Set-Cookie in a way that conflicts with such folding.

@pi0 pi0 merged commit 4d3feff into nuxt-community:dev Jun 3, 2019
@sullivanpt
Copy link
Contributor Author

sullivanpt commented Jun 3, 2019

Thanks for the quick response and merge.

Regarding the express implementation, yes, I was sure I'd seen this implementation before which is why it took me so long to suspect that block of code. Then I stumbled across this person complaining of a similar cookie problem just using express and I was like "oh! I get it now...". http://www.connecto.io/blog/nodejs-express-how-to-set-multiple-cookies-in-the-same-response-object/

There's one more bug in this module that I'm struggling to find the correct fix for: when switching providers it fails to switch properly if the new provider has an expired token and the userinfo reload on provider switch fails. I'll make a PR if I can figure something intelligent out.

But overall I love this module. it makes OAuth trivial. Love it! Thanks a million

@sullivanpt sullivanpt deleted the fix-server-set-cookie-array branch Jun 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants