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

rfc6265bis - Cookie Parser - cookie-name includes too many characters now #2848

Closed
joakime opened this issue Jul 30, 2024 · 11 comments
Closed
Labels

Comments

@joakime
Copy link

joakime commented Jul 30, 2024

The cookie-name changed from the definition of ...

to

cookie-name = 1*cookie-octet

As a result of commit 5469d51 from PR #1145

This changed the list of available characters on cookie-name in the following way.

Hex character Token cookie-octet changed
x20 false false
x21 ! true true
x22 " false false
x23 # true true
x24 $ true true
x25 % true true
x26 & true true
x27 ' true true
x28 ( false true changed
x29 ) false true changed
x2A * true true
x2B + true true
x2C , false false
x2D - true true
x2E . true true
x2F / false true changed
x30 0 true true
x31 1 true true
x32 2 true true
x33 3 true true
x34 4 true true
x35 5 true true
x36 6 true true
x37 7 true true
x38 8 true true
x39 9 true true
x3A : false true changed
x3B ; false false
x3C < false true changed
x3D = false true changed
x3E > false true changed
x3F ? false true changed
x40 @ false true changed
x41 A true true
x42 B true true
x43 C true true
x44 D true true
x45 E true true
x46 F true true
x47 G true true
x48 H true true
x49 I true true
x4A J true true
x4B K true true
x4C L true true
x4D M true true
x4E N true true
x4F O true true
x50 P true true
x51 Q true true
x52 R true true
x53 S true true
x54 T true true
x55 U true true
x56 V true true
x57 W true true
x58 X true true
x59 Y true true
x5A Z true true
x5B [ false true changed
x5C (backslash) false false
x5D ] false true changed
x5E ^ true true
x5F _ true true
x60 (backtick) true true
x61 a true true
x62 b true true
x63 c true true
x64 d true true
x65 e true true
x66 f true true
x67 g true true
x68 h true true
x69 i true true
x6A j true true
x6B k true true
x6C l true true
x6D m true true
x6E n true true
x6F o true true
x70 p true true
x71 q true true
x72 r true true
x73 s true true
x74 t true true
x75 u true true
x76 v true true
x77 w true true
x78 x true true
x79 y true true
x7A z true true
x7B { false true changed
x7C | true true
x7D } false true changed
x7E ~ true true

It is particularly an issue with the x3D = character.
But what about the rest of the changes?

Do we really want to allow cookies with names like a(foo)=z and a[{b|c}]=z now?

@miketaylr
Copy link
Collaborator

Do we know if the major browsers are interoperable here?

@markt-asf
Copy link
Contributor

My reading of the issues (#1074, #1119) was that the primary driver was to allow white space both around and in cookie names and values.

The part of the proposal to allow white space in cookie-name was to change it from token to 1*cookie-octet and to add %20 to cookie-octet. However, %20 was not added to cookie-octet. I can't tell if this was an oversight or was deliberate. It does seem odd to add a whole bunch of extra characters to the allowed set without adding the one that was requested so it looks more like oversight to me.

Some very basic testing indicates that the latest Chrome, Firefox and Safari all handle a cookie name of ()/:<>? @[]{} (includes %20) without complaint although I am not sure how much weight that observation should carry.

If we want to allow spaces in cookie values then we need to add %20 to cookie-octet.

If we want to allow spaces in cookie names then we can either add %20 to cookie-octet or create a new definition for cookie-name that is "anything that is allowed in token plus %20".

We also need to resolve the issue created by allowing = in a cookie name. That leads, with the current text, to the server setting a cookie name a=b with value c being interpreted by the user agent as a cookie with name a and value b=c. Not allowing = in cookie-name seems to be the simplest solution that problem.

Given the above and combined with my (narrow) experience with Tomcat that any change to the rules around cookies causes breakage for someone, my preferred way forward would be the minimal change that achieves support for %20 in cookie-name and cookie-value:

  • add %20 to cookie-octet (for cookie-value)
  • create a new definition for cookie-name that is essentially anything allowed in token + %20

@bagder
Copy link

bagder commented Aug 1, 2024

I am strongly against allowing spaces (%20) in cookie names since it will break cookie parsers all over (for example curl). Similar to my arguments around not accepting TABs (%09). See #2262

@markt-asf
Copy link
Contributor

What about the switch to use 1*cookie-octet rather than 1*token for cookie-name? Do you see a similar issue there?

@bagder
Copy link

bagder commented Aug 1, 2024

I completely agree with this issue that at least the new treatment of = is going to break parsers widely. I do not see the point in this expanded syntax. We have decades of existing cookie parsers to keep in mind. Cookies are not reserved to be used by a small subset of browsers.

@reschke
Copy link
Contributor

reschke commented Aug 1, 2024

FWIW, I just noticed that I approved that PR back then, and I shouldn't have. In my defense, I was focusing on a different part of the PR. That'll teach me to check more before approving.

#1145 (review)

@joakime
Copy link
Author

joakime commented Aug 1, 2024

I completely agree with this issue that at least the new treatment of = is going to break parsers widely. I do not see the point in this expanded syntax. We have decades of existing cookie parsers to keep in mind. Cookies are not reserved to be used by a small subset of browsers.

Not to mention that allowing = also means the other parts of the spec related to generation and parsing are now in conflict with that decision.

Currently they have parts like ...

3. If the name-value-pair string lacks a %x3D ("=") character, then the name
string is empty, and the value string is the value of name-value-pair.
Otherwise, the name string consists of the characters up to, but not
including, the first %x3D ("=") character, and the (possibly empty) value
string consists of the characters after the first %x3D ("=") character.

4. If the cookie-av string contains a %x3D ("=") character:
1. The (possibly empty) attribute-name string consists of the characters
up to, but not including, the first %x3D ("=") character, and the
(possibly empty) attribute-value string consists of the characters
after the first %x3D ("=") character.

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

@markt-asf
Copy link
Contributor

I think there are two issues here:

  1. The change to the definition of cookie-name
  2. Whether space should be supported in cookie-value

The consensus - at least in the comments here - for the first issue seems to be to revert to using token for cookie-name. I'll prepare a PR for that.

There hasn't been much discussion about adding %20 to cookie-octet. Is there a desire to do that? I'm currently -0 on that but open to having my mind changed.

@bagder
Copy link

bagder commented Aug 6, 2024

Allowing space (or TAB) in the value is a breaking change that most certainly will not be working in many existing cookie parsers. The curl cookie parser has been in use since October 1998, it still is, and it does not support spaces in either name nor value. Because cookies were always said not to have those.

Correction: curl's parser does handle embedded spaces (not tabs) fine, and strips off leading and trailing ones.

@bagder
Copy link

bagder commented Aug 6, 2024

I suppose my argument is still: it is super fragile to change the set of accepted characters so it should be avoided in the name of compatibility with code that already shipped and will not update again in a long time.

@sbingler
Copy link
Collaborator

Closed by #2857

The original syntax change was indeed a mistake which has been reverted.

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

No branches or pull requests

6 participants