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

Note that cookie serialisation algorithm may produce output that doesn't match cookie-string #1210

Closed
gsnedders opened this issue May 30, 2020 · 13 comments · Fixed by #2220
Closed
Assignees
Labels

Comments

@gsnedders
Copy link

Ideally we'd define server-side parsing of cookie-string (or at least recommend behaviour here for things that don't match cookie-string!), given this does cause problems for some servers (e.g. aio-libs/aiohttp#2683).

Until then we should at least make it clear that the "the algorithm to compute the cookie-string from a cookie store and a request-uri" can create things that don't match cookie-string (e.g., nameless cookies after #1144). There's a number of WPT expected results that don't match cookie-string.

@mnot mnot added the 6265bis label Jun 1, 2020
@miketaylr
Copy link
Collaborator

miketaylr commented Feb 23, 2021

Until then we should at least make it clear that the "the algorithm to compute the cookie-string from a cookie store and a request-uri" can create things that don't match cookie-string (e.g., nameless cookies after #1144).

Can you clarify what a fix would look like? Bonus points for a PR. 😅

(I mean, I can also attempt a PR, just not clear on what the recommended fix would be)

@gsnedders
Copy link
Author

Can you clarify what a fix would look like? Bonus points for a PR. 😅

(I mean, I can also attempt a PR, just not clear on what the recommended fix would be)

You attempting would probably be as good as me attempting!

@miketaylr
Copy link
Collaborator

I'm skeptical, however willing to try!

And now I think I actually understand the bug report (and didn't when I commented). @gsnedders is saying that the ABNF in https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-07#section-4.2.1 has a bug, since cookie-pair is defined as:

cookie-pair = cookie-name BWS "=" BWS cookie-value

But nameless and valueless cookies exist.

cf.
#1119
#1074

@miketaylr
Copy link
Collaborator

I think maybe this can be solved by making the following changes:

This is pretty simple to grok:
cookie-pair = cookie-name cookie-value

This is supposed to convey "a nameless cookie, OR a valueless cookie (but not both)"
cookie-name = 1*cookie-octet / (*cookie-octet BWS "=" BWS)

That should work for the following valid cookie-pairs:

f; (nameless)
=f; (nameless2)
f=; (named cookie with empty cookie-value, i.e., valueless)
f=1 (name + value)

If that's the route we go, then in https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-07#section-4.2.1

cookie-string = cookie-pair *( ";" SP cookie-pair )

That should work.

However... unless we tweak cookie-value, I think you can end up with the following invalid cookie: =. Will need to formalize "if no name, there must be a value".

Maybe we can just add an empty-cookie-value and then do something like:

cookie-pair = cookie-name (empty-cookie-value / cookie-value) 🤔

@miketaylr
Copy link
Collaborator

miketaylr commented Feb 25, 2021

cookie-pair = cookie-name (empty-cookie-value / cookie-value) 🤔

Actually that won't fix the bug I described. 🙈

Maybe this super elegant solution (but the above ABNF would have to be tweaked):

cookie-pair = (empty-cookie-name cookie-value) / (cookie-name empty-cookie-value) / (cookie-name cookie-value)

@DCtheTall
Copy link
Contributor

But nameless and valueless cookies exist.

FWIW nameless and valueless cookies are no longer RFC compliant after #1236. Up until then only Set-Cookie strings with empty names and empty values weren't accepted, but there were ways around this to get empty-name-empty-value cookies into user agents' cookie jar using the CookieStore API.

@chlily1
Copy link
Contributor

chlily1 commented Feb 26, 2021

There are nameless cookies and there are valueless cookies (and, as you said, there are no nameless-and-valueless cookies).

FWIW, the syntax in section 4.1.1 where cookie-pair is defined is a SHOULD, not a MUST, on the server's part. I think one issue is that reusing it to define cookie-string makes that also implicitly a SHOULD, if I'm reading this correctly:

   If the server conforms to the requirements in
   Section 4.1 (and the user agent conforms to the requirements in
   Section 5), the user agent will send a Cookie header that conforms to
   the following grammar:

Edit: To clarify, I'm thinking there's no accuracy issue with the spec as written, since if the server does things as recommended (and sends a cookie with name=value), then the user agent will respond as given by cookie-string. But that is still totally confusing, at least to me, so I think we should still define this better.

@miketaylr
Copy link
Collaborator

But nameless and valueless cookies exist.

(love the ambiguities of english 😄 -- thanks for the explicit callout and pointer to #1236 @DCtheTall)

@chlily1 chlily1 self-assigned this Mar 9, 2021
@recvfrom
Copy link
Contributor

recvfrom commented Aug 5, 2021

In the nameless cookie case, it seems that an unhandled edge case arises because of the following, from the Retrieval Algorithm section:

4. Serialize the cookie-list into a cookie-string by processing each cookie
   in the cookie-list in order:

   1.  If the cookies' name is not empty, output the cookie's name followed by
       the %x3D ("=") character.

   2.  If the cookies' value is not empty, output the cookie's value.

To illustrate the problem with some examples:

  • if name is empty and value is test, the algorithm above works - the resulting cookie line would be test
  • if name is empty and value is ambiguous=value, though, the resulting cookie line would be ambiguous=value, which would be interpreted as a cookie with name ambiguous and value value.

One solution for this, which is currently specified by the Cookie Store API, is to forbid cookies with an empty name and a value containing '=' from being set. Limiting value from containing a '=' is not mentioned in the The Set-Cookie Header Field section or the Storage Model section of RFC6265bis, though.

It seems like using the following in the Retrieval Algorithm section would be a better approach:

4. Serialize the cookie-list into a cookie-string by processing each cookie
   in the cookie-list in order:

   1.  If the cookies' name is not empty, output the cookie's name.

   2.  Output the %x3D ("=") character.

   3.  If the cookies' value is not empty, output the cookie's value.

This would fix the issue in the example above (the resulting cookie lines would be =test and =ambiguous=value respectively, both of which would be interpreted correctly). If we want to allow test to be produced in the first case, we could replace step 2 above with something like:

   2.  If the cookie's name is empty and the cookie's value does not contain
       the %x3D ("=") character, either output the %x3D ("=") character or
       skip this step.  Otherwise, output the %x3D ("=") character.

I'll submit a PR for this change.

@chlily1
Copy link
Contributor

chlily1 commented Aug 5, 2021

The empty-name-but-nonempty-value case is serialized as test and not =test for historical/compat reasons. There's some earlier discussion in #159. The case where the value contains an '=' is interesting though, and not tested by WPTs I believe.

The Set-Cookie header can't produce a nameless cookie with value ambiguous=value so this is only a concern for non-HTTP APIs that don't parse a set-cookie-string. Could we instead just add to the Storage Model section the restriction that the cookie-name and cookie-value cannot contain '=' or ';' characters?

(Side note, more generally, this is getting out of hand and we should try to factor out common logic in the Storage Model and set-cookie-string parsing sections.)

@recvfrom
Copy link
Contributor

recvfrom commented Aug 6, 2021

hmm, looks like there is a WPT for this case, but the expected output for it would be interpreted incorrectly :(

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/cookies/name/name.html;l=22-26

Wouldn't =ambiguous=value be allowed in a Set-Cookie header?

Also, '=' is a valid base64 character, so not allowing it in cookie-value might cause a lot of headaches

@chlily1
Copy link
Contributor

chlily1 commented Aug 6, 2021

Oh, my mistake, you're right that Set-Cookie: =ambiguous=value creates a nameless cookie with value ambiguous=value. Looks like Chrome/Edge/Firefox agree on how to serialize these, but Safari ignores nameless cookies altogether.

I would be a bit concerned about breaking stuff if this serialization changed. :-/

Theoretically there's a use case for this behavior... since Set-Cookie: =foo=1 and Set-Cookie: =bar=2 are the same cookie (the nameless cookie) but with different values, and they overwrite each other, a site could be relying on getting the output Cookie: foo=1 or Cookie: bar=2 (but never both) as a way of (sort of) setting two mutually exclusive cookies named foo and bar.

@miketaylr
Copy link
Collaborator

Maybe we just fix this with a note cautioning against using nameless cookies, and point to this issue as a warning.

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

Successfully merging a pull request may close this issue.

6 participants