CookieMonster edge-case parsing improvements and tests.

CookieMonster quote parsing changes and tests.

This fixes one bug where a cookie like:
A="BBB"  ;
Would be "BBB"; in all other browser, but
"BBB"  ; in Chrome.  Additionally it fixes creating unintentional (but harmless) empty attributes.

We had previously tried to match Firefox, but after long discussions we decided it makes more sense to match Internet Explorer and Safari.  This means not explicitly handling quoted-string as proposed in the newer RFCs. 

Before:  A="B;C"; -> A="B;C";
After:   A="B;C"; -> A="B;

[Dean McNamee, 2009-01-02 16:01:22]

Oh man, what a mess.  I forgot how awful cookies can be.

I test on Safari Windows, I'm not sure if the network stack is identify, and I
don't have a Mac to test there.

This all started with Pawel trying to fix a TODO, after a lot of thinking and
testing, I think this is the best thing we can do, since the cookie spec wasn't
designed to handle quoting, and any rules to try to handle them are unclear.

[Dean McNamee, 2009-01-02 16:11:34]

The RFCs seem to suggest that value can be a quoted string, however, in
practice, this all seems like a mess and I'm not sure how to handle it.

lcamtuf is slowly convincing me that matching IE might be a bad idea.  Maybe I
can just fix the original issue and leave things as is for now?  :(

http://www.ietf.org/rfc/rfc2109.txt
http://www.ietf.org/rfc/rfc2965.txt

On 2009/01/02 16:01:22, Dean McNamee wrote:
> Oh man, what a mess.  I forgot how awful cookies can be.
> 
> I test on Safari Windows, I'm not sure if the network stack is identify, and I
> don't have a Mac to test there.
> 
> This all started with Pawel trying to fix a TODO, after a lot of thinking and
> testing, I think this is the best thing we can do, since the cookie spec
wasn't
> designed to handle quoting, and any rules to try to handle them are unclear.

[Dean McNamee, 2009-01-02 16:16:02]

Ok, one last note.  There are no real RFCs for cookies.  The above specify
cookies that look like nothing that is actually used in practice.  So we have to
decide, match an RFC that doesn't evenly remotely match reality, or match IE
which probably has a bunch of problems.  Or keep it the way we are, which is
sort of in the middle, and mostly matching Firefox.

On 2009/01/02 16:11:34, Dean McNamee wrote:
> The RFCs seem to suggest that value can be a quoted string, however, in
> practice, this all seems like a mess and I'm not sure how to handle it.
> 
> lcamtuf is slowly convincing me that matching IE might be a bad idea.  Maybe I
> can just fix the original issue and leave things as is for now?  :(
> 
> http://www.ietf.org/rfc/rfc2109.txt
> http://www.ietf.org/rfc/rfc2965.txt
> 
> On 2009/01/02 16:01:22, Dean McNamee wrote:
> > Oh man, what a mess.  I forgot how awful cookies can be.
> > 
> > I test on Safari Windows, I'm not sure if the network stack is identify, and
I
> > don't have a Mac to test there.
> > 
> > This all started with Pawel trying to fix a TODO, after a lot of thinking
and
> > testing, I think this is the best thing we can do, since the cookie spec
> wasn't
> > designed to handle quoting, and any rules to try to handle them are unclear.

[Peter Kasting, 2009-01-05 20:43:01]

On 2009/01/02 16:16:02, Dean McNamee wrote:
> So we have to
> decide, match an RFC that doesn't evenly remotely match reality, or match IE
> which probably has a bunch of problems.  Or keep it the way we are, which is
> sort of in the middle, and mostly matching Firefox.

Clearly we shouldn't match the RFC.  That leaves "match other browsers" and/or
"do something logical".  It seems like matching Safari or Firefox is preferable
to matching IE unless IE's cookie handling is obviously superior.  I don't
really know how the other browsers behave so I defer to your judgement.

As an author, I would expect "AAA"  ; to result in "AAA";, but I don't know what
I'd expect from cases with multiple quoted substrings.  Are quotes supposed to
be preserved straight across, or substrings not in quotes ignored, or ...?

[Dean McNamee, 2009-01-07 14:24:31]

On 2009/01/05 20:43:01, pkasting wrote:
> On 2009/01/02 16:16:02, Dean McNamee wrote:
> > So we have to
> > decide, match an RFC that doesn't evenly remotely match reality, or match IE
> > which probably has a bunch of problems.  Or keep it the way we are, which is
> > sort of in the middle, and mostly matching Firefox.
> 
> Clearly we shouldn't match the RFC.  That leaves "match other browsers" and/or
> "do something logical".  It seems like matching Safari or Firefox is
preferable
> to matching IE unless IE's cookie handling is obviously superior.  I don't
> really know how the other browsers behave so I defer to your judgement.
> 
> As an author, I would expect "AAA"  ; to result in "AAA";, but I don't know
what

Yeah, I agree with you here.  We currently have a bug, "AAA"  ; should be
"AAA";.  The question I guess is whether we support double quotes at all. 
(Well, cookies with double quotes will always work, it's just a question if they
work if they have a ; inside the quotes).  For what it's worth, I believe the
RFC says we should, because quoted-string should be allowed to have a ; in it. 
In one way or another, Firefox and Opera do.  However, Safari and IE don't.  So
it's not really practical to have a ; within double quotes, because it's not
going to work in IE.  But this also isn't a good argument.

I am going to try to keep our double quote handling, and just fix the trailing
whitespace issue.  I will update this CL.


> I'd expect from cases with multiple quoted substrings.  Are quotes supposed to
> be preserved straight across, or substrings not in quotes ignored, or ...?

[Dean McNamee, 2009-01-08 12:41:05]

I updated it to keep the double quote parsing.  I am still unsure though, from a
compat perspective, it seems smarter to follow IE than Firefox.  But from an RFC
and "logical" perspective, handling quoted-string also makes some sense :\

http://codereview.chromium.org/17045/diff/602/212
File net/base/cookie_monster_unittest.cc (right):

http://codereview.chromium.org/17045/diff/602/212#newcode52
Line 52: "\"zzz \"   \"ppp\" ",     "\"zzz \"",
This difference in particular scares me a bit.  I suppose we could run some
tests to try to tell if this ever happens in the wild.  I would figure that by
following what Firefox does, we're mostly OK, but I don't know if that's true in
regions of the world where Firefox isn't popular, etc.

[Peter Kasting, 2009-01-08 17:57:33]

Just commenting on desired behavior so we can nail that down before actually
reviewing code.

http://codereview.chromium.org/17045/diff/602/212
File net/base/cookie_monster_unittest.cc (right):

http://codereview.chromium.org/17045/diff/602/212#newcode47
Line 47: "\"zz;pp\" ;",             "\"zz;pp\"",
While in the abstract Firefox' behavior seems much better, once you look at the
last case here as well, IE/Safari's behavior kinda seems more consistent. 
Consider the following crazy string:

a="b;b=c";c=d"d=e;

Do we reject this?  Parse as a=["b;b=c"], c=[d"d=e]?  Parse as a=["b],
b=[c";c=d"d=e]?  Some other way?  I have no clue.  Whereas if we follow
IE/Safari, it seems like this ought to be a=["b], b=[c"], c=[d"d=e].

http://codereview.chromium.org/17045/diff/602/212#newcode52
Line 52: "\"zzz \"   \"ppp\" ",     "\"zzz \"",
On 2009/01/08 12:41:05, Dean McNamee wrote:
> This difference in particular scares me a bit.

Yeah, this is weird.  IE/Safari's handling makes a little more sense to me,
honestly.

http://codereview.chromium.org/17045/diff/602/212#newcode148
Line 148: // attributes with an empty string name and value.
That is weird.  Is that what everyone else does, too?

http://codereview.chromium.org/17045/diff/602/212#newcode178
Line 178: net::CookieMonster::ParsedCookie pc("ANCUUID=zohNumRKgI0oxyhSsV3Z7D  ;
"
Should this subtest check that we preserve whitespace when it _doesn't_ follow a
quoted string?  (Or should we not actually preserve that whitespace?)

[Dean McNamee, 2009-01-08 18:07:55]

Yeah man, I dunno, it's all kinda gnarly.  Half of me wants to go with IE, and
the other half thinks that this whole double quoted business had a point to
it...

http://codereview.chromium.org/17045/diff/602/212
File net/base/cookie_monster_unittest.cc (right):

http://codereview.chromium.org/17045/diff/602/212#newcode178
Line 178: net::CookieMonster::ParsedCookie pc("ANCUUID=zohNumRKgI0oxyhSsV3Z7D  ;
"
Many other tests check this (For example, the Whitespace one above).  We should
not preserve the trailing whitespace here.  I can add an EXPECT for that.

On 2009/01/08 17:57:33, pkasting wrote:
> Should this subtest check that we preserve whitespace when it _doesn't_ follow
a
> quoted string?  (Or should we not actually preserve that whitespace?)

[Peter Kasting, 2009-01-08 18:14:38]

I think my gut feeling is that since we don't truly treat quoted strings like
quoted strings (e.g. [a=b"c;] does not get rejected as invalid), we might as
well just treat quotes as normal characters the way IE/Safari seem to do.  That
seems better than treating them as quoted strings _sometimes_ (like Firefox
seems to do).

http://codereview.chromium.org/17045/diff/602/212
File net/base/cookie_monster_unittest.cc (right):

http://codereview.chromium.org/17045/diff/602/212#newcode178
Line 178: net::CookieMonster::ParsedCookie pc("ANCUUID=zohNumRKgI0oxyhSsV3Z7D  ;
"
On 2009/01/08 18:07:55, Dean McNamee wrote:
> Many other tests check this (For example, the Whitespace one above).  We
should
> not preserve the trailing whitespace here.  I can add an EXPECT for that.

I am a little confused because I'm not sure if you interpreted my question
correctly, but I think what you're saying is:
a="foo"  ;  --> a="foo";
a=foo    ;  --> a=foo;
...and that you're going to add an EXPECT to this test to ensure we do the
latter.

If so, we're on the same page.

[Dean McNamee, 2009-01-08 18:28:00]

On 2009/01/08 18:14:38, pkasting wrote:
> I think my gut feeling is that since we don't truly treat quoted strings like
> quoted strings (e.g. [a=b"c;] does not get rejected as invalid), we might as
> well just treat quotes as normal characters the way IE/Safari seem to do. 
That
> seems better than treating them as quoted strings _sometimes_ (like Firefox
> seems to do).
> 
> http://codereview.chromium.org/17045/diff/602/212
> File net/base/cookie_monster_unittest.cc (right):
> 
> http://codereview.chromium.org/17045/diff/602/212#newcode178
> Line 178: net::CookieMonster::ParsedCookie pc("ANCUUID=zohNumRKgI0oxyhSsV3Z7D 
;
> "
> On 2009/01/08 18:07:55, Dean McNamee wrote:
> > Many other tests check this (For example, the Whitespace one above).  We
> should
> > not preserve the trailing whitespace here.  I can add an EXPECT for that.
> 
> I am a little confused because I'm not sure if you interpreted my question
> correctly, but I think what you're saying is:
> a="foo"  ;  --> a="foo";
> a=foo    ;  --> a=foo;
> ...and that you're going to add an EXPECT to this test to ensure we do the
> latter.
> 
> If so, we're on the same page.

Added the test case to TrailingWhitespace.

[Peter Kasting, 2009-01-08 19:06:31]

Dean and I chatted some on IRC.  We didn't reach any consensus on what behavior
to use.  Dean feels like inventing a new, incompatible behavior would be bad, so
we should match some existing browser exactly.  I feel less strongly, since on
cases where everyone disagrees I'm not sure it can matter.

If we're going to pick an existing browser, IE has the most marketshare and the
simplest algorithm, and Dean's comments in the unittest suggest that Safari
(which is what sites who cared would think we are) matches it.  So I would
suggest copying IE.  Its algorithm is basically "scan for a semicolon,
completely ignoring quotes; then walk backwards to discard any trailing
whitespace".

If we're not picking an existing browser, we could probably do something like
"When encountering a quote, scan forward for another quote; if not found, treat
this as a normal character and move on, otherwise consume the whole string
including quotes and move on".  This would match IE for
multiple-quoted-substrings, and Firefox for
quoted-substrings-containing-semicolons, which seem like fairly logical,
compatible things to do.

At this point, I would like to hear Darin's opinion on our desired behavior.

[darin (slow to review), 2009-01-08 21:09:02]

On 2009/01/08 19:06:31, pkasting wrote:
> At this point, I would like to hear Darin's opinion on our desired behavior.

What algorithm does Safari implement?  I'm also curious about matching their
behavior for compatibility with sites that assume all WebKit based browsers are
the same.

Also, do we know of any big sites where it matters what we do?  Are there sites
that are broken in Firefox because it doesn't behave like IE?

[lcamtuf, 2009-01-09 12:52:23]

When it comes to the ";" affair, I am quite willing to make the claim that MSIE
does not parse:

Cookie: name="foo;bar"

...by breaking at ";" because of their careful reading of the RFC, but because
they *consistently* fail to properly tokenize all ;-delimited headers, including
situations that are in a *clear* violation of RFCs, and are not followed by any
other browser. For example, they also incorrectly parse this:

Content-Disposition: attachment; filename="foo.exe;bar.jpg"

...despite the RFC clearly permitting quoted-string, and including ; as a list
of characters permitted within quoted-string; *no* other browser is following
this logic.

Inventing new behaviors is bad, but that's exactly what we're doing here: we
want to follow MSIE habit on "Cookie", but non-MSIE habit on
"Content-Disposition" and other headers. This makes the implementation less
consistent and increases complexity.

There is also a security argument in support of recognizing quoted-string, if it
is mentioned in the RFC: both in Content-Disposition headers and in HTTP cookie
values, user-controlled values are routinely emitted by web servers these days.
In fact, I can assert that there are multiple Google applications that do so.
And so, if the set of permitted characters that web developer may infer by
carefully reading the RFC differs from what is actually safe to use with a web
browser, this is a bad design, and a completely unnecessary security risk.

MSIE currently creates this security risk with all ";"-delimited headers, by
permitting file name spoofing and cookie injection if ";" is not escaped (and
there are no consistently supported protocol-level ways to escape special
characters in HTTP headers, by the way). All the other browsers behave more
safely on "Content-Disposition"; and Firefox and Opera also behave more safely
on "Cookie" - only Safari and MSIE do not. 

So if both consistency and security is compromised, and nothing currently
depended upon is likely to break regardless of which option we pursue, why go
there?

[Dean McNamee, 2009-01-09 13:03:48]

On 2009/01/09 12:52:23, lcamtuf wrote:
> When it comes to the ";" affair, I am quite willing to make the claim that
MSIE
> does not parse:
> 
> Cookie: name="foo;bar"
> 
> ...by breaking at ";" because of their careful reading of the RFC, but because
> they *consistently* fail to properly tokenize all ;-delimited headers,
including
> situations that are in a *clear* violation of RFCs, and are not followed by
any
> other browser. For example, they also incorrectly parse this:
> 
> Content-Disposition: attachment; filename="foo.exe;bar.jpg"
> 
> ...despite the RFC clearly permitting quoted-string, and including ; as a list
> of characters permitted within quoted-string; *no* other browser is following
> this logic.
> 
> Inventing new behaviors is bad, but that's exactly what we're doing here: we
> want to follow MSIE habit on "Cookie", but non-MSIE habit on
> "Content-Disposition" and other headers. This makes the implementation less
> consistent and increases complexity.

How does this compare with Safari?  I think our final argument was to
follow Safari, not to follow IE.  Meaning we aren't creating a new
behavior if Safari doesn't have the content disposition problems but
still doesn't support quotes in cookies.  Our behavior is then just
the Safari behavior.

> 
> There is also a security argument in support of recognizing quoted-string, if
it
> is mentioned in the RFC: both in Content-Disposition headers and in HTTP
cookie
> values, user-controlled values are routinely emitted by web servers these
days.
> In fact, I can assert that there are multiple Google applications that do so.
> And so, if the set of permitted characters that web developer may infer by
> carefully reading the RFC differs from what is actually safe to use with a web
> browser, this is a bad design, and a completely unnecessary security risk.
> 
> MSIE currently creates this security risk with all ";"-delimited headers, by
> permitting file name spoofing and cookie injection if ";" is not escaped (and
> there are no consistently supported protocol-level ways to escape special
> characters in HTTP headers, by the way). All the other browsers behave more
> safely on "Content-Disposition"; and Firefox and Opera also behave more safely
> on "Cookie" - only Safari and MSIE do not. 

Since this is already a security risk you need to worry about, since
it's a problem that current exists in IE, all web developers already
have to be aware of it.  Making us support quoted strings doesn't
really help anyone since the application would still be insecure on
IE.  I had said originally that that isn't a good argument, so I'm not
saying it's a reason, I'm just saying.

I'm very unclear which side of this argument I want to be on.  I know
we don't want to follow Opera, because they reject lots of cookies and
I think this is something we want to try not to do.  Therefor if we
want to match anyone, we have to pick from: Firefox, IE, Safari.  From
those options, it seems the conclusion was matching Safari made the
most sense.

> 
> So if both consistency and security is compromised, and nothing currently
> depended upon is likely to break regardless of which option we pursue, why go
> there?

[Dean McNamee, 2009-01-09 13:13:19]

1 2 3 4 I declare a thumb war.

Ok, I just want to kill "the RFC" argument.  Because the RFC:

http://www.ietf.org/rfc/rfc2109.txt

Says that cookies look like this:

         Cookie: $Version="1";
                 Customer="WILE_E_COYOTE"; $Path="/acme";
                 Part_Number="Rocket_Launcher_0001"; $Path="/acme";
                 Shipping="FedEx"; $Path="/acme"

So the argument should not involve the RFC, and instead involve which browsers
we want to match and which behavior with think is the most logical and the best
for web developers and the future of the entire human universe and stuff.

On 2009/01/09 13:03:48, Dean McNamee wrote:
> On 2009/01/09 12:52:23, lcamtuf wrote:
> > When it comes to the ";" affair, I am quite willing to make the claim that
> MSIE
> > does not parse:
> > 
> > Cookie: name="foo;bar"
> > 
> > ...by breaking at ";" because of their careful reading of the RFC, but
because
> > they *consistently* fail to properly tokenize all ;-delimited headers,
> including
> > situations that are in a *clear* violation of RFCs, and are not followed by
> any
> > other browser. For example, they also incorrectly parse this:
> > 
> > Content-Disposition: attachment; filename="foo.exe;bar.jpg"
> > 
> > ...despite the RFC clearly permitting quoted-string, and including ; as a
list
> > of characters permitted within quoted-string; *no* other browser is
following
> > this logic.
> > 
> > Inventing new behaviors is bad, but that's exactly what we're doing here: we
> > want to follow MSIE habit on "Cookie", but non-MSIE habit on
> > "Content-Disposition" and other headers. This makes the implementation less
> > consistent and increases complexity.
> 
> How does this compare with Safari?  I think our final argument was to
> follow Safari, not to follow IE.  Meaning we aren't creating a new
> behavior if Safari doesn't have the content disposition problems but
> still doesn't support quotes in cookies.  Our behavior is then just
> the Safari behavior.
> 
> > 
> > There is also a security argument in support of recognizing quoted-string,
if
> it
> > is mentioned in the RFC: both in Content-Disposition headers and in HTTP
> cookie
> > values, user-controlled values are routinely emitted by web servers these
> days.
> > In fact, I can assert that there are multiple Google applications that do
so.
> > And so, if the set of permitted characters that web developer may infer by
> > carefully reading the RFC differs from what is actually safe to use with a
web
> > browser, this is a bad design, and a completely unnecessary security risk.
> > 
> > MSIE currently creates this security risk with all ";"-delimited headers, by
> > permitting file name spoofing and cookie injection if ";" is not escaped
(and
> > there are no consistently supported protocol-level ways to escape special
> > characters in HTTP headers, by the way). All the other browsers behave more
> > safely on "Content-Disposition"; and Firefox and Opera also behave more
safely
> > on "Cookie" - only Safari and MSIE do not. 
> 
> Since this is already a security risk you need to worry about, since
> it's a problem that current exists in IE, all web developers already
> have to be aware of it.  Making us support quoted strings doesn't
> really help anyone since the application would still be insecure on
> IE.  I had said originally that that isn't a good argument, so I'm not
> saying it's a reason, I'm just saying.
> 
> I'm very unclear which side of this argument I want to be on.  I know
> we don't want to follow Opera, because they reject lots of cookies and
> I think this is something we want to try not to do.  Therefor if we
> want to match anyone, we have to pick from: Firefox, IE, Safari.  From
> those options, it seems the conclusion was matching Safari made the
> most sense.
> 
> > 
> > So if both consistency and security is compromised, and nothing currently
> > depended upon is likely to break regardless of which option we pursue, why
go
> > there?

[Dean McNamee, 2009-01-09 13:22:32]

If we could go back in time and pick which behavior we thought was better, which
would it be?  Personally I think the non-quoted behavior is better, and it's
simpler for everyone to implement.  For example, lots of apps will be reading
cookies from javascript.  It would be complicated to create a real cookie parser
to get the split correctly.  It is likely that everyone now is just splitting on
;, for example:

http://www.quirksmode.org/js/cookies.html

Just sayin.

On 2009/01/09 13:13:19, Dean McNamee wrote:
> 1 2 3 4 I declare a thumb war.
> 
> Ok, I just want to kill "the RFC" argument.  Because the RFC:
> 
> http://www.ietf.org/rfc/rfc2109.txt
> 
> Says that cookies look like this:
> 
>          Cookie: $Version="1";
>                  Customer="WILE_E_COYOTE"; $Path="/acme";
>                  Part_Number="Rocket_Launcher_0001"; $Path="/acme";
>                  Shipping="FedEx"; $Path="/acme"
> 
> So the argument should not involve the RFC, and instead involve which browsers
> we want to match and which behavior with think is the most logical and the
best
> for web developers and the future of the entire human universe and stuff.
> 
> On 2009/01/09 13:03:48, Dean McNamee wrote:
> > On 2009/01/09 12:52:23, lcamtuf wrote:
> > > When it comes to the ";" affair, I am quite willing to make the claim that
> > MSIE
> > > does not parse:
> > > 
> > > Cookie: name="foo;bar"
> > > 
> > > ...by breaking at ";" because of their careful reading of the RFC, but
> because
> > > they *consistently* fail to properly tokenize all ;-delimited headers,
> > including
> > > situations that are in a *clear* violation of RFCs, and are not followed
by
> > any
> > > other browser. For example, they also incorrectly parse this:
> > > 
> > > Content-Disposition: attachment; filename="foo.exe;bar.jpg"
> > > 
> > > ...despite the RFC clearly permitting quoted-string, and including ; as a
> list
> > > of characters permitted within quoted-string; *no* other browser is
> following
> > > this logic.
> > > 
> > > Inventing new behaviors is bad, but that's exactly what we're doing here:
we
> > > want to follow MSIE habit on "Cookie", but non-MSIE habit on
> > > "Content-Disposition" and other headers. This makes the implementation
less
> > > consistent and increases complexity.
> > 
> > How does this compare with Safari?  I think our final argument was to
> > follow Safari, not to follow IE.  Meaning we aren't creating a new
> > behavior if Safari doesn't have the content disposition problems but
> > still doesn't support quotes in cookies.  Our behavior is then just
> > the Safari behavior.
> > 
> > > 
> > > There is also a security argument in support of recognizing quoted-string,
> if
> > it
> > > is mentioned in the RFC: both in Content-Disposition headers and in HTTP
> > cookie
> > > values, user-controlled values are routinely emitted by web servers these
> > days.
> > > In fact, I can assert that there are multiple Google applications that do
> so.
> > > And so, if the set of permitted characters that web developer may infer by
> > > carefully reading the RFC differs from what is actually safe to use with a
> web
> > > browser, this is a bad design, and a completely unnecessary security risk.
> > > 
> > > MSIE currently creates this security risk with all ";"-delimited headers,
by
> > > permitting file name spoofing and cookie injection if ";" is not escaped
> (and
> > > there are no consistently supported protocol-level ways to escape special
> > > characters in HTTP headers, by the way). All the other browsers behave
more
> > > safely on "Content-Disposition"; and Firefox and Opera also behave more
> safely
> > > on "Cookie" - only Safari and MSIE do not. 
> > 
> > Since this is already a security risk you need to worry about, since
> > it's a problem that current exists in IE, all web developers already
> > have to be aware of it.  Making us support quoted strings doesn't
> > really help anyone since the application would still be insecure on
> > IE.  I had said originally that that isn't a good argument, so I'm not
> > saying it's a reason, I'm just saying.
> > 
> > I'm very unclear which side of this argument I want to be on.  I know
> > we don't want to follow Opera, because they reject lots of cookies and
> > I think this is something we want to try not to do.  Therefor if we
> > want to match anyone, we have to pick from: Firefox, IE, Safari.  From
> > those options, it seems the conclusion was matching Safari made the
> > most sense.
> > 
> > > 
> > > So if both consistency and security is compromised, and nothing currently
> > > depended upon is likely to break regardless of which option we pursue, why
> go
> > > there?

[Dean McNamee, 2009-01-09 13:33:36]

So after some more digging, basically the answer I got was this.  The RFCs were
invented after all of the browsers actually implemented them, and the spec that
predated the RFCs was:

http://cgi.netscape.com/newsref/std/cookie_spec.html

Some people call this cookies v0, which the RFCs were for an attempted upgrade
to cookies v1, which seems to have mostly failed.  The above spec seems to be
the most realistic to what browser implement, and specifically says that cookie
values may not contain a ;.

I will implement the Safari / IE behavior and update this review.

The end is nigh.

On 2009/01/09 13:22:32, Dean McNamee wrote:
> If we could go back in time and pick which behavior we thought was better,
which
> would it be?  Personally I think the non-quoted behavior is better, and it's
> simpler for everyone to implement.  For example, lots of apps will be reading
> cookies from javascript.  It would be complicated to create a real cookie
parser
> to get the split correctly.  It is likely that everyone now is just splitting
on
> ;, for example:
> 
> http://www.quirksmode.org/js/cookies.html
> 
> Just sayin.
> 
> On 2009/01/09 13:13:19, Dean McNamee wrote:
> > 1 2 3 4 I declare a thumb war.
> > 
> > Ok, I just want to kill "the RFC" argument.  Because the RFC:
> > 
> > http://www.ietf.org/rfc/rfc2109.txt
> > 
> > Says that cookies look like this:
> > 
> >          Cookie: $Version="1";
> >                  Customer="WILE_E_COYOTE"; $Path="/acme";
> >                  Part_Number="Rocket_Launcher_0001"; $Path="/acme";
> >                  Shipping="FedEx"; $Path="/acme"
> > 
> > So the argument should not involve the RFC, and instead involve which
browsers
> > we want to match and which behavior with think is the most logical and the
> best
> > for web developers and the future of the entire human universe and stuff.
> > 
> > On 2009/01/09 13:03:48, Dean McNamee wrote:
> > > On 2009/01/09 12:52:23, lcamtuf wrote:
> > > > When it comes to the ";" affair, I am quite willing to make the claim
that
> > > MSIE
> > > > does not parse:
> > > > 
> > > > Cookie: name="foo;bar"
> > > > 
> > > > ...by breaking at ";" because of their careful reading of the RFC, but
> > because
> > > > they *consistently* fail to properly tokenize all ;-delimited headers,
> > > including
> > > > situations that are in a *clear* violation of RFCs, and are not followed
> by
> > > any
> > > > other browser. For example, they also incorrectly parse this:
> > > > 
> > > > Content-Disposition: attachment; filename="foo.exe;bar.jpg"
> > > > 
> > > > ...despite the RFC clearly permitting quoted-string, and including ; as
a
> > list
> > > > of characters permitted within quoted-string; *no* other browser is
> > following
> > > > this logic.
> > > > 
> > > > Inventing new behaviors is bad, but that's exactly what we're doing
here:
> we
> > > > want to follow MSIE habit on "Cookie", but non-MSIE habit on
> > > > "Content-Disposition" and other headers. This makes the implementation
> less
> > > > consistent and increases complexity.
> > > 
> > > How does this compare with Safari?  I think our final argument was to
> > > follow Safari, not to follow IE.  Meaning we aren't creating a new
> > > behavior if Safari doesn't have the content disposition problems but
> > > still doesn't support quotes in cookies.  Our behavior is then just
> > > the Safari behavior.
> > > 
> > > > 
> > > > There is also a security argument in support of recognizing
quoted-string,
> > if
> > > it
> > > > is mentioned in the RFC: both in Content-Disposition headers and in HTTP
> > > cookie
> > > > values, user-controlled values are routinely emitted by web servers
these
> > > days.
> > > > In fact, I can assert that there are multiple Google applications that
do
> > so.
> > > > And so, if the set of permitted characters that web developer may infer
by
> > > > carefully reading the RFC differs from what is actually safe to use with
a
> > web
> > > > browser, this is a bad design, and a completely unnecessary security
risk.
> > > > 
> > > > MSIE currently creates this security risk with all ";"-delimited
headers,
> by
> > > > permitting file name spoofing and cookie injection if ";" is not escaped
> > (and
> > > > there are no consistently supported protocol-level ways to escape
special
> > > > characters in HTTP headers, by the way). All the other browsers behave
> more
> > > > safely on "Content-Disposition"; and Firefox and Opera also behave more
> > safely
> > > > on "Cookie" - only Safari and MSIE do not. 
> > > 
> > > Since this is already a security risk you need to worry about, since
> > > it's a problem that current exists in IE, all web developers already
> > > have to be aware of it.  Making us support quoted strings doesn't
> > > really help anyone since the application would still be insecure on
> > > IE.  I had said originally that that isn't a good argument, so I'm not
> > > saying it's a reason, I'm just saying.
> > > 
> > > I'm very unclear which side of this argument I want to be on.  I know
> > > we don't want to follow Opera, because they reject lots of cookies and
> > > I think this is something we want to try not to do.  Therefor if we
> > > want to match anyone, we have to pick from: Firefox, IE, Safari.  From
> > > those options, it seems the conclusion was matching Safari made the
> > > most sense.
> > > 
> > > > 
> > > > So if both consistency and security is compromised, and nothing
currently
> > > > depended upon is likely to break regardless of which option we pursue,
why
> > go
> > > > there?

[Dean McNamee, 2009-01-09 14:11:14]

Updated, all tests passing with Safari / IE behavior.  It didn't really affect
tests, which is good, because really we are discussing what are hopefully edge
cases.

[Peter Kasting, 2009-01-09 18:02:53]

LGTM

http://codereview.chromium.org/17045/diff/619/620
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/17045/diff/619/620#newcode999
Line 999: // implement the Firefox behavior (A="B;C"; -> A="B;C;").  However,
since
Nit: Did you mean
A="B;C"; -> A="B;C";
?

[Dean McNamee, 2009-01-09 18:12:44]

Good catch, fixed.  Let's see what Darin thinks.

On 2009/01/09 18:02:53, pkasting wrote:
> LGTM
> 
> http://codereview.chromium.org/17045/diff/619/620
> File net/base/cookie_monster.cc (right):
> 
> http://codereview.chromium.org/17045/diff/619/620#newcode999
> Line 999: // implement the Firefox behavior (A="B;C"; -> A="B;C;").  However,
> since
> Nit: Did you mean
> A="B;C"; -> A="B;C";
> ?

[darin (slow to review), 2009-01-09 18:20:11]

LGTM