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

CookieStore allows you to set a cookie with no name and value #149

Closed
DCtheTall opened this issue Jul 9, 2020 · 5 comments · Fixed by #150
Closed

CookieStore allows you to set a cookie with no name and value #149

DCtheTall opened this issue Jul 9, 2020 · 5 comments · Fixed by #150

Comments

@DCtheTall
Copy link
Member

DCtheTall commented Jul 9, 2020

It has recently come to my attention that it is possible to set a cookie with the cookieStore API using empty name and value strings. This is not spec compliant with how most browsers parse Set-Cookie headers.

In Chrome, it is currently it is possible to set a cookie using the following:

cookieStore.set('', '');  // Promise resolves.
cookieStore.get('');  // Promise resolves cookie with empty name and value.

I have not tested in other browsers but can get back to you on that if others are interested.

As discussed here, Safari, Firefox, and now Chrome ignore cookies set with "" or "=". I think the CookieStore API should mirror this behavior as well. The Promise returned by set('', '') should either be rejected or resolve and the cookie is ignored (imho I think the former is more appropriate).

There are currently web platform tests that exercise this behavior which I believe need to be changed as well.

@ayuishii
Copy link
Collaborator

ayuishii commented Jul 9, 2020

Thanks for pointing out this issue and providing context @DCtheTall!

@mikewest as the cookies RFC author, what would your recommendation be?

@inexorabletash
Copy link
Member

Yes, thanks @DCtheTall !

It sounds like the rest of the world got a little bit less broken while we had this in progress, and can catch up. I agree that we should make set('','') throw now.

My reading of httpwg/http-extensions#159 is that to align we would still allow set('x', '') and set('', 'x')

A new step in https://wicg.github.io/cookie-store/#set-a-cookie should take care of it, e.g.:If name’s length is 0 and value's length is 0, then return failure.

https://wicg.github.io/cookie-store/#intro-opinions needs a rewrite/update (but that was already the case)

@DCtheTall
Copy link
Member Author

DCtheTall commented Jul 14, 2020

@inexorabletash thanks for the response.

I wonder if https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-06#section-5.4 also needs to be updated with a step to ignore cookies with no name and value, but I think there is probably a better place to discuss that 😅

@mikewest
Copy link
Member

  1. It seems quite reasonable to me to reject cookies lacking both names and values (and we do that in step 4 of https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-06#section-5.3). If I could go back in time, I would also reject cookies without names. Cookies without values make some conceptual sense, but nameless cookies were a mistake IMO. Given that this API needs to interop with the string-based API, I can understand inheriting the same weirdness.

    So, throwing on set('', ''), while allowing set('name', '') and set('', 'value') sounds reasonable to me.

  2. If https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-06#section-5.4 needs to change, I'd appreciate y'all filing a bug at https://github.com/httpwg/http-extensions/issues/new. I'm not sure it does, since it shouldn't be possible to produce a cookie string that parses without a name and without a value, given step 4 of https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-06#section-5.3. I guess since we're adding new ways of "receiving a cookie" that don't run through that algorithm, we should be a little more defensive? PRs welcome. :)

@DCtheTall
Copy link
Member Author

@mikewest thanks, I'll have a PR put together for this repository shortly and I'll open a bug for 5.4 of rfc6265bis. I agree that I think the assumption that a restriction in 5.3 will automatically apply to any cookie running through the algorithm in 5.4, which APIs like this one show is not always the case 😄

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 a pull request may close this issue.

4 participants