1244505 - Firefox 44 no longer allows spaces in cookie names, breaking some apps

Status: VERIFIED FIXED

(Core :: Networking: Cookies, defect)

Description

[Kohei Yoshino]

I know whitespaces are now allowed in cookie names, guess the behaviour has been changed in Bug 1233784. (I don't have access to the bug)

https://www.mozilla.org/en-US/security/advisories/mfsa2016-04/

But I have seen at least 2 reports regarding whitespaces in cookie names:

https://support.mozilla.org/en-US/questions/1107073
https://input.mozilla.org/en-US/dashboard/response/5795493

Is this change intended? If so, I'll write a site compatibility doc for Web developers.

Comment 1

[Kohei Yoshino]

I meant: whitespaces are *not* allowed in cookie names

Comment 2

[Kohei Yoshino]

Another report:
https://input.mozilla.org/en-US/dashboard/response/5795475

Perhaps related:
https://input.mozilla.org/en-US/dashboard/response/5792410
https://input.mozilla.org/en-US/dashboard/response/5793857

Comment 3

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/whitespaces-are-no-longer-allowed-in-cookie-names/

Comment 4

[Kohei Yoshino]

There aren't so many reports from developers so far, but it's difficult to measure the impact of the change. For end users, they are just no longer able to sign into a site or use specific site features, and they don't know what should they do.

Should we allow spaces, not vertical tabs or whatever, for interoperability, maybe? My guess is all other browsers allow spaces for historical reasons, and that's why we are seeing reports from developers this time.

Comment 5

[Kohei Yoshino]

[Tracking Requested - why for this release]: A new site compatibility issue. Some apps are broken due to a side effect from a security fix in Firefox 44.

Comment 6

[u408661]

Let me dig up my notes from this - it may be worth backing out all or part of this change (this is the second bug I've seen today).

Comment 7

[u408661]

Quoting https://bugzilla.mozilla.org/show_bug.cgi?id=1191423#c49 (since it's a security bug, and not generally open):

"I had a look at other browsers, and the status of compat there is much more consistent than with odd characters in the values. All browsers I tested (with the exception of Firefox) disallow any character 0x1e and below, as well as space (0x20). IE/Edge is the only browser other than Firefox that allows 0x1f in a name. Beyond that, it gets a bit messier, but not too crazy (safari is slightly stricter than everyone else). I think we're safe disallowing characters <= 0x20 to occur in the name."

Though now it's seeming that's not 100% true (maybe my test was faulty in some way). I propose changing the list of disallowed name characters (and *only* name characters) to *not* include 0x20 (space), as that seems the most likely culprit. Unfortunately, I won't be able to test the fix, as I don't have accounts on any of the affected sites.

Jason, Patrick, what do you think? (The patch is simple enough, will make it while I await response.)

Comment 9

[u408661]

Attachment: Allow spaces in cookie names

Here's the patch to allow spaces in cookie names. Everything else that I originally disallowed still seems reasonable to disallow.

Comment 10

[Patrick McManus [:mcmanus]]

(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #7)
> 
> Though now it's seeming that's not 100% true (maybe my test was faulty in

I think the only right answer here is maximum compat - let's do what chrome does. Can we figure out why your test doesn't match the reports?

Comment 11

[u408661]

Yeah, space is the only one I got wrong - my test did single-character names, not names containing certain characters (mixed with other, known-legal characters). When I changed, both safari and chrome accept 0x20 (safari also accepts 0x09 (\t), while chrome doesn't - we always disallowed \t). So, the patch I've put up seems like the right course.

Jason, no need from info from you, just an r+ would be good so we can get this fixed (or Patrick can feel free to steal the r+ if he wants).

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2351a9ec305b

Comment 13

[James]

Could you confirm a timeframe for when the fix will be released / what version number

The internet has been broken for those users using firefox where sites have cookies with spaces and have had to use other browsers such as chrome / internet explorer

thanks

james

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2351a9ec305b

Comment 15

[Patrick McManus [:mcmanus]]

plz nom the backports

Comment 16

[u408661]

Comment on attachment 8714365 [details] [diff] [review]
Allow spaces in cookie names

Approval Request Comment
[Feature/regressing bug #]: 1233784
[User impact if declined]: Some sites unable to set cookies
[Describe test coverage new/current, TreeHerder]: On m-c, manual testing
[Risks and why]: Very low - this is a return to previous behavior, and matches Safari and Chrome
[String/UUID change made/needed]: None

Comment 17

[chris hofmann]

stack overflow article that lots of developers probably reference and appears at the top of google search results on related topics.  the article traces the ugly and twisted history of cookie specification.

http://stackoverflow.com/questions/1969232/allowed-characters-in-cookies

might be worth pitching in a few comments on that thread if cross browser testing reviles different results than posted there.

probably would be good to have tests against the specs and defacto standards implemented in the other browsers to avoid the unlikely chance that changes happen again in this area in the future.

Comment 18

[Doug Turner (:dougt)]

Tests please?

Comment 19

[chris hofmann]

This article also has some interesting observations on the universe of cookie implementations

https://github.com/golang/go/issues/7243

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8714365 [details] [diff] [review]
Allow spaces in cookie names

Oh, this makes me sad...

Can I ask why we don't have tests for this? We should be checking for invalid cookies names and with all the codes in illegalNameCharacters
Until now, taking on the behalf of Ritu as she mentioned taking it in 44.0.1
Should be also in 45 beta 3.

Comment 21

[James]

We refer to the issue as cookiegate

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9962682e5e75

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d6f5a4905c66

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-release/rev/c51ca5032cf0

Comment 25

[Kohei Yoshino]

Updated the site compat doc: https://www.fxsitecompat.com/en-CA/docs/2016/whitespaces-are-no-longer-allowed-in-cookie-names/

Comment 26

[u408661]

(In reply to Sylvestre Ledru [:sylvestre] from comment #20)
> Can I ask why we don't have tests for this? We should be checking for
> invalid cookies names and with all the codes in illegalNameCharacters

There is a test, but it's not exhaustive; in other words, the test doesn't specifically test every single value in illegalNameCharacters - there's no point, as if it blocks one character in the array, it blocks them all (as we've seen in practice).

I will add a new test to ensure we continue allowing spaces in both names and values, though.

Comment 27

[u408661]

Attachment: test patch

Here's a test to ensure spaces stay allowed.

Comment 28

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Kohei, could you please verify that the fix works as expected on the latest Nightly build? Thanks!

Comment 29

[Kohei Yoshino]

Attachment: testcase

Comment 30

[Kohei Yoshino]

LGTM on Nightly.

Comment 31

[chris hofmann]

yeah, the problem here is not that we needed an automated test to tell us if the characters we intended to block were actually blocked (although that kind of testing has some value).  

The problem was in assessing how wide the use of the space character was in cookie names and values across the corpus of the web, or how ambitious we needed to be in blocking that specific character.  

The best ways to figure that out would be to 1) find articles like the ones mentioned in comment 17 and 19 or 2) use things like bclary's web crawler to scan top sites *and* the dark corners of the web to determine actual use/depenency and frequency on the web.

The article in comment 19 represented a few months of kicking around how best to classify use of cookies on the web.  It stated:

Comment 13:

Alternative proposal. Split bytes into three sets:

white: [0x21, 0x7e] minus double-quote, comma, semi-colon and backslash.
gray: 0x20 space and 0x2c comma.
black: everything else.

Drop any black bytes. If there are any gray bytes, quote the entire cookie value.
Otherwise (white bytes only), do not quote the value.

Under this classification we make some changes definitely in the "safe" blocking zone, but we made one change in the "grey" zone and that came back to bite us.

The one area of analysis that could still be done is to think about and understand how quoting works v. unquoted (double quoted) passing of characters, and how that works in all the browsers, and potentially on some websites.  Its not clear if we have tested or thought about that much in bug comments, but maybe someone has.

one other lesson re-learned here.  don't mess with lou montulli's cookie code without lots of time and effort and testing. ;-)   That might be some of the oldest code remaining in the code base.

Comment 32

[chris hofmann]

> don't mess with lou montulli's cookie code without lots of time and effort and testing. ;-) 

that should have been time and testing on the web, with analysis of user comments like appeared in this bug.

this also represented another case of a change that jumped trains and could have used longer bake time on nightly, beta, and aurora and time for random reports that said "this site doesn't work correctly for me anymore", and then investigations like what :kohei undertook.   

GREAT JOB :kohei!  Thanks for saving us before this change went to all users.

Comment 33

[Petruta Horea [:phorea], Desktop Test Engineering (back on Sep 5th)]

Verified as fixed using the attached testcase with Firefox 44.0.1, Firefox 45 beta 3, latest Aurora 46.0a2 and latest Nightly 47.0a1 (2016-02-05) under Win 7 x64, Ubuntu 14.04 x64 and Mac OS X 10.9.5.

Comment 34

[Kohei Yoshino]

(In reply to chris hofmann from comment #32)
> GREAT JOB :kohei!  Thanks for saving us before this change went to all users.

Yay. The finding is part of my Firefox Site Compatibility initiative :)
https://www.fxsitecompat.com/about/

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/140603de6df9

Comment 36

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/140603de6df9

Comment 37

[Sylvestre Ledru [:Sylvestre]]

Added in the release notes with "Allows spaces in cookie names (1244505)" as wording

Comment 38

[Anthony]

Have you considered that perhaps the applications that depend on SP in cookie names are themselves broken?

According to RFC 6265 HTTP State Management Mechanism, s4.1.1 Syntax:
cookie-name    = token
token          = <token, defined in [RFC2616], Section 2.2>

Consulting RFC 2616, s2.2 Basic Rules:
token          = 1*<any CHAR except CTLs or separators>
separators     = "(" | ")" | "<" | ">" | "@"
               | "," | ";" | ":" | "\" | <">
               | "/" | "[" | "]" | "?" | "="
               | "{" | "}" | SP | HT

So the requirement is that cookie names do not allow SP characters.

Comment 39

[u408661]

(In reply to Anthony from comment #38)
> Have you considered that perhaps the applications that depend on SP in
> cookie names are themselves broken?

Yes, according to RFC 6265, they are, in fact broken. That was why the change to disallow spaces was made in the first place. Unfortunately, however, the world we live in is not as cleanly defined as the RFC would hope - spaces are (as this bug indicates) rather common in the real world, and are allowed by other browsers. So, in the interest of operating on the internet we have (and not the internet we may wish we had), we'll continue allowing spaces in cookie names.

Comment 40

[Anthony]

(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #39)
> So, in the interest of operating on the
> internet we have (and not the internet we may wish we had), we'll continue
> allowing spaces in cookie names.

This is disappointing and shows why the internet is such a broken place. Nobody is prepared to stick to the RFCs and Standards to improve interoperability. Instead we're reduced to the same level as that mother of the little misbehaving brat who keeps saying "aww, come on, let him do it" whenever he does something wrong.