Invalid handling of cookies with invalid name-value-pair in set-cookie string

[eliassorensen]

Invalid handling of cookies with invalid name-value-pair in set-cookie string

[nfriedly]

hum, yeah, we should probably do one or the other. Or maybe both, with a strict mode that toggles between the two.

I'm curious how you ran into this (?)

[nfriedly]

Actually, there's already a silent flag; lets make it just follow the RFC and drop the cookie, but have it log about dropping an invalid cookie unless the silent flag is set. Can you put up a PR to do that?

[eliassorensen]

I'm a bit cautious changing the behavior to drop the cookie. I'm not sure what will break for other projects depending on this if we do it this way?

A strict mode could be an option for this, and making sure that it follows the spec completely.

Could we maybe have three modes? Sloppy / non-strict (as it is now), strict, and a mode that mimics the browser behavior (like what I saw in Chrome/Firefox). That would of course be a big change to the current simplistic API that you have created.

I saw it because I'm using it for a project listing the cookies on a website. Previously I just grabbed the cookies directly from the browser, but changing it to look at set-cookie headers instead, I noticed a mismatch on some sites between the new and old code. The way that browsers interpret the cookies was the culprit.

I've also opened a bug in the Chromium tracker for this. So interested in hearing their reasoning behind this behavior.

I'd be happy to do a PR for it when it's decided how the behavior should be 🙏🏼

[nfriedly]

I don't really like having 3 modes, but I could live with 2, and I suppose we could call it a semver major change on the off chance that someone is depending on the current behavior.

I'm curious to hear what Chromium folks say about this - could you link to the bug? My guess is that they're going to say that the current behavior is the de-facto standard and not change the implementation. (Or perhaps there's a newer RFC that covers it?)

[eliassorensen]

As far as I can see, there isn't an RFC that obsoletes 6265.

Investigating a bit more, you are right that it looks like they chose to implement it de-facto because of IE/Netscape, and therefore it doesn't match 6265 completely. Looking at https://bugzilla.mozilla.org/show_bug.cgi?id=270710 they write:

In general Mozilla wants to comply with standards. In the specific case of
cookies the historical implementations (Netscape and IE) have *never* strictly
matched the standards, and that even though it was Netscape who contributed the
standard in the first place.

Every time we've attempted to tighten up and match some aspect of the standard
that wasn't enforced before we've broken sites, and since they "work" in IE and
Netscape 4.x it's Mozilla that's considered at fault and we're forced to give in.

My Chromium issue is here: https://bugs.chromium.org/p/chromium/issues/detail?id=1346543

So I think it boils down to essentially: Do we want set-cookie-parser to parse it at browsers do? Or do we want it to strictly follow the RFC?

In my opinion, it could maybe be this option:

• Strict mode parses it exactly as the RFC (i.e. the cookie would be invalid/ignored)
• Non-strict would parse it as browsers (so changing the current behavior to have empty name, and set the value instead)

I can work on a PR on Monday if we agree on this 👍

It would be a major version because it essentially changes the behavior of the default non-strict mode.

[eliassorensen]

Okay, I got an update now from the Chromium team.

There is actually a new RFC that obsoletes 6265, but it's in draft, so it didn't come up in my searches.

It is called 6265bis, and have the following change:

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.

... so looks like they're actually changing the RFC to cover how it's de-facto implemented (which is nice..).

In this case, would it make sense to update the behavior in set-cookie-parser to this? I think it's mainly relevant for this library to parse like browsers do?

I'd be happy to do it.

[nfriedly]

Aah, yeah, let's do that. And add a reference to 6265bis in the readme. Thank you for doing the legwork!

[eliassorensen]

I'll do! No problem :-) Happy to help.

[eliassorensen]

@nfriedly I've created #54.

Feel free to comment on it.

I assume you decide on the versioning once merged.