Ignore cookies with invalid 'SameSite' attribute values.

Ignore cookies with invalid 'SameSite' attribute values.

As per https://tools.ietf.org/html/draft-west-first-party-cookies-07#section-4.1

BUG=600983
Committed: https://crrev.com/1a9ce0d2b4854d406c62c23af936c633b2777feb
Cr-Commit-Position: refs/heads/master@{#385525}

[Mike West, 2016-04-06 14:38:07]

mkwst@chromium.org changed reviewers:
+ estark@chromium.org, mmenke@chromium.org

[Mike West, 2016-04-06 14:38:07]

estark@: Would you mind taking a look at this patch?

mmenke@: Would you mind stamping the //net/extras bit? If you have time, I'd
love for you to take a look at the rest as well. :)

[Mike West, 2016-04-06 14:39:08]

mkwst@chromium.org changed reviewers:
+ jochen@chromium.org

[Mike West, 2016-04-06 14:39:09]

jochen@: Would you mind stamping the changes in //chrome and //content. I'm just
adding an enum value to two switch statements which should never be triggered.

[mmenke, 2016-04-06 15:16:30]

https://codereview.chromium.org/1868493002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/cookies/cookies_helpers.cc (right):

https://codereview.chromium.org/1868493002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/cookies/cookies_helpers.cc:94: case
net::CookieSameSite::INVALID_MODE:
It seems unfortunate that we need to expose this new invalid SameSite state as
part of the API.  I don't suppose we could simple reflect this error as part of
the ParsedCookie.IsValid, and not add a value in the enum?

https://codereview.chromium.org/1868493002/diff/20001/net/cookies/cookie_cons...
File net/cookies/cookie_constants.cc (right):

https://codereview.chromium.org/1868493002/diff/20001/net/cookies/cookie_cons...
net/cookies/cookie_constants.cc:53: if
(base::EqualsCaseInsensitiveASCII(same_site, kSameSiteStrict))
From spec:

1. If "cookie-av"'s "attribute-value" is not a case-sensitive match for "Strict"
or "Lax", ignore the "cookie-av".

Either the spec is wrong, or this code is.  Given that the next line mentions a
case-insensitive comparison to "Lax", I suspect it's the spec.

[Mike West, 2016-04-06 15:22:55]

On 2016/04/06 at 15:16:30, mmenke wrote:
>
https://codereview.chromium.org/1868493002/diff/20001/chrome/browser/extensio...
> File chrome/browser/extensions/api/cookies/cookies_helpers.cc (right):
> 
>
https://codereview.chromium.org/1868493002/diff/20001/chrome/browser/extensio...
> chrome/browser/extensions/api/cookies/cookies_helpers.cc:94: case
net::CookieSameSite::INVALID_MODE:
> It seems unfortunate that we need to expose this new invalid SameSite state as
part of the API.  I don't suppose we could simple reflect this error as part of
the ParsedCookie.IsValid, and not add a value in the enum?

Hrm. Well, we could do something like `if (same_site_index_ != 0 && SameSite()
== CookieSameSite::DEFAULT_MODE)`? That seems less clear inside `ParsedCookie`,
but I agree that it would be clearer outside. If you'd prefer I do that (or have
a less confusing proposal?), I'm happy to.

>
https://codereview.chromium.org/1868493002/diff/20001/net/cookies/cookie_cons...
> File net/cookies/cookie_constants.cc (right):
> 
>
https://codereview.chromium.org/1868493002/diff/20001/net/cookies/cookie_cons...
> net/cookies/cookie_constants.cc:53: if
(base::EqualsCaseInsensitiveASCII(same_site, kSameSiteStrict))
> From spec:
> 
> 1. If "cookie-av"'s "attribute-value" is not a case-sensitive match for
"Strict" or "Lax", ignore the "cookie-av".
> 
> Either the spec is wrong, or this code is.  Given that the next line mentions
a case-insensitive comparison to "Lax", I suspect it's the spec.

*sigh*  I did a poor job proofreading. Time for an -08.

[mmenke, 2016-04-06 15:28:51]

On 2016/04/06 15:22:55, Mike West wrote:
> On 2016/04/06 at 15:16:30, mmenke wrote:
> >
>
https://codereview.chromium.org/1868493002/diff/20001/chrome/browser/extensio...
> > File chrome/browser/extensions/api/cookies/cookies_helpers.cc (right):
> > 
> >
>
https://codereview.chromium.org/1868493002/diff/20001/chrome/browser/extensio...
> > chrome/browser/extensions/api/cookies/cookies_helpers.cc:94: case
> net::CookieSameSite::INVALID_MODE:
> > It seems unfortunate that we need to expose this new invalid SameSite state
as
> part of the API.  I don't suppose we could simple reflect this error as part
of
> the ParsedCookie.IsValid, and not add a value in the enum?
> 
> Hrm. Well, we could do something like `if (same_site_index_ != 0 && SameSite()
> == CookieSameSite::DEFAULT_MODE)`? That seems less clear inside
`ParsedCookie`,
> but I agree that it would be clearer outside. If you'd prefer I do that (or
have
> a less confusing proposal?), I'm happy to.

Ah, yea, I'd forgotten that we store values as indexes, instead of parsing them
when we create the parsed cookie.

Maybe "bool SameSiteInternal(SameSite *same_site)" and make it return false on
invalid values?

[Mike West, 2016-04-06 16:02:04]

Is this more or less what you're thinking, mmenke@? It's a big improvement
outside //net/cookies, and it's not _too_ ugly with the private function
wrapping the index check.

[mmenke, 2016-04-06 16:07:24]

On 2016/04/06 16:02:04, Mike West wrote:
> Is this more or less what you're thinking, mmenke@? It's a big improvement
> outside //net/cookies, and it's not _too_ ugly with the private function
> wrapping the index check.

Yea, that works for me.  Think the DEFAULT_MODE check is a little weird, but I
can certainly live with it, and tastes differ when it comes to code.  :)

https://codereview.chromium.org/1868493002/diff/40001/net/cookies/canonical_c...
File net/cookies/canonical_cookie_unittest.cc (right):

https://codereview.chromium.org/1868493002/diff/40001/net/cookies/canonical_c...
net/cookies/canonical_cookie_unittest.cc:126: TEST(CanonicalCookieTest,
CreateInvalid) {
CreateInvalid -> CreateInvalidSameSite?  There are other cases where
CanonicalCookie::Create fails, like trying to set cookies on other domains,
which this does not cover.

[Mike West, 2016-04-06 16:34:39]

https://codereview.chromium.org/1868493002/diff/40001/net/cookies/canonical_c...
File net/cookies/canonical_cookie_unittest.cc (right):

https://codereview.chromium.org/1868493002/diff/40001/net/cookies/canonical_c...
net/cookies/canonical_cookie_unittest.cc:126: TEST(CanonicalCookieTest,
CreateInvalid) {
On 2016/04/06 at 16:07:23, mmenke wrote:
> CreateInvalid -> CreateInvalidSameSite?  There are other cases where
CanonicalCookie::Create fails, like trying to set cookies on other domains,
which this does not cover.

Done.

[mmenke, 2016-04-06 16:38:19]

Oops, meant to LGTM it in that last message.

[Mike West, 2016-04-06 18:22:44]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2016-04-06 18:23:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1868493002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1868493002/60001

[commit-bot: I haz the power, 2016-04-06 19:44:29]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-06 19:46:11]

Description was changed from

==========
Ignore cookies with invalid 'SameSite' attribute values.

As per https://tools.ietf.org/html/draft-west-first-party-cookies-07#section-4.1

BUG=600983
==========

to

==========
Ignore cookies with invalid 'SameSite' attribute values.

As per https://tools.ietf.org/html/draft-west-first-party-cookies-07#section-4.1

BUG=600983

Committed: https://crrev.com/1a9ce0d2b4854d406c62c23af936c633b2777feb
Cr-Commit-Position: refs/heads/master@{#385525}
==========

[commit-bot: I haz the power, 2016-04-06 19:46:13]

Patchset 4 (id:??) landed as
https://crrev.com/1a9ce0d2b4854d406c62c23af936c633b2777feb
Cr-Commit-Position: refs/heads/master@{#385525}