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

URL Decoding of Raw Cookie Values #112

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

barakshechter
Copy link

URL decoding of raw cookie header key-value pairs as recommended by h…ttps://tools.ietf.org/html/rfc6265#section-4.1.1

Using cookie-session, signed cookies are not recognized when the serialized session cookie contains special characters such as =, since they end up being url encoded. This PR will URL Decode the raw values parsed from the cookie header.

@dougwilson
Copy link
Contributor

Thanks! There are two issues with this, if you can help:

(1) This is not backwards-compatible, as it will now throw for cookies like foo=% when they would not throw before (and would get the value of % the user likely expected)

(2) Can you quote the part in the RFC that specifies that the decoding is URL decoding (i.e. percent decoding)? I tried looking in the link you provided, but didn't see it anywhere. Can you inline quote the exact thing the RFC says regarding that?

@barakshechter
Copy link
Author

@dougwilson Good point, I went back and looked at the RFC and expressjs documentation. The RFC doesn't specify the encoding to use, but rather recommends an encoding be used. Express by default will encode cookies using encodeUriComponent http://expressjs.com/en/api.html#res.cookie

Would it perhaps be worth passing a decode field in the options, and if present, apply it to the value parsed out from the regex? This would be backwards compatible since it would be undefined in existing usage; could be swapped out for a minor version tick.

@dougwilson
Copy link
Contributor

How would that be different from just like writing foo = decodeURIComponent(cookies.get(name)), besides harder to tell what type of encoding the cookie value is in when reading the code? This module just leaves it up to the user to choose the method to encode and decode the cookie since there is no specification for it currently.

@barakshechter
Copy link
Author

barakshechter commented Jul 15, 2019

The issue arises when you're using signed cookies. Signing of the cookie happens on the cookie value directly, but then the set-cookie might encode them both. Essentially, when trying to verify the signature, you need to potentially decode the raw cookie value before trying the various signing keys to find a signature match.

What you end up having is:

data = 'some-random-data=='
sig = sign(data)
res.cookie(name, data)
res.cookie(`${name}.sig`, data)

and effectively, the following ends up being true:

res.headers['cookie'] === `${name}=${encodeUriComponent(data)}; ${name}.sig=${encodeUriComponent(sig)}`

@barakshechter
Copy link
Author

Sorry @dougwilson , needed to translate my train of thought into english.

@dougwilson
Copy link
Contributor

So why not just set the cookie with this module if you're trying to get the cookie back again with this module? Then it will always do what you want.

Otherwise you can just have res.cookie in express not encode the value in it's options. You're going to end up with this option between one of these two if you don't just use this module to set the cookie value too, and you can already set the encoding in express (for example, to just not encode), which seems like the simpler solution for your specific problem, right?

@barakshechter
Copy link
Author

Agreed that I could theoretically have express not encode the cookie, in this particular case, the service using cookie-session is behind a proxy which is managing the user's cookies. I figured that giving the ability to specify an encoding/decoding here would provide greater flexibility.

@dougwilson
Copy link
Contributor

The reason there is no option to encode / decode in this module was a design choice, not an accidental omission.

The express API has issue and should not be copied here because it provides no way for the user to handle decoding errors, for example. The API choice of this module does, because you always get back the raw value and can then encode / decode as you like, including however you want to handle errors that happen during that process.

@barakshechter
Copy link
Author

barakshechter commented Jul 16, 2019

The API choice of this module does, because you always get back the raw value

With the exception of signatures. The opinion of this module is that the cookie's signature is based on the raw value. If the cookie value and its corresponding signature were encoded as part of the set-cookie, their contents will not match

@dougwilson
Copy link
Contributor

What does that have to do with handling decoding errors? I'm not sure I understand how your response fits in to what was quoted?

@dougwilson
Copy link
Contributor

The opinion of this module is that the cookie's signature is based on the raw value. If the cookie value and its corresponding signature were encoded as part of the set-cookie, their contents will not match

That's right. So if you want to not use this module to generate the signature, you still have to do it in a compatible way... just like the name of the signature cookie has to end with ".sig" and not like ".foobar"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants