SameSite: Implement 'Strict'/'Lax' attribute parsing.

SameSite: Implement 'Strict'/'Lax' attribute parsing.

https://tools.ietf.org/html/draft-west-first-party-cookies-06 introduced
the notion of "Strict" or "Lax" enforcement of the "SameSite" attribute.
This patch implements the infrastructure changes necessary to support
that distinction, but does not yet implement the behavioral change
(that is, after this patch, `SameSite` will be rejected, while
`SameSite=Strict` and `SameSite=Lax` will have the same behavior that
`SameSite` alone has today).

Most of this patch is occupied with the fairly mechanical process of
swapping out a new 'CookieSameSite' enum for the existing boolean in
various constructors and setters. The most interesting piece is the
change to the storage backend, which now stores 0, 1, or 2 in the
database to represent the possible values, rather than 0 or 1 to
represent the boolean.

BUG=459154
Committed: https://crrev.com/e1a295845cbc338a564dc04e6e3e69b29ba7862f
Cr-Commit-Position: refs/heads/master@{#381201}

[Mike West, 2016-03-08 16:35:13]

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

[Mike West, 2016-03-08 16:35:14]

We have too many platforms. :(

Assuming I can make ChromeOS, Android, and iOS compile, WDYT of the patch? It's
hopefully not as large as it looks, mostly mechanical changes relating to the
replacement of a boolean with an enum.

(As an aside: we should really get rid of the other enums in the CanonicalCookie
constructors, as it's super confusing to reason about all the unittests that
just set long sets of `false, false, true, false`, etc.)

[Mike West, 2016-03-10 13:56:49]

mkwst@chromium.org changed reviewers:
+ jochen@chromium.org, marq@chromium.org, torne@chromium.org

[Mike West, 2016-03-10 13:56:49]

mkwst@chromium.org changed required reviewers:
+ mmenke@chromium.org

[Mike West, 2016-03-10 13:56:49]

marq@: Would you mind stamping the trivial changes to //ios?

torne@: Would you mind stamping the equally trivial changes to //android_webview
and chrome/browser/android?

jochen@: Would you mind stamping the //chrome and //components bits and pieces?

mmenke@: I think this will now actually compile everywhere. It's not high
priority, though, so no rush.

[Torne, 2016-03-10 14:09:28]

android_webview LGTM

[Mike West, 2016-03-10 15:30:34]

Patchset #3 (id:40001) has been deleted

[jochen (gone - plz use gerrit), 2016-03-10 15:53:24]

lgtm

[mmenke, 2016-03-11 18:08:31]

Just looked at net/

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/canonical_c...
File net/cookies/canonical_cookie.cc (right):

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/canonical_c...
net/cookies/canonical_cookie.cc:425: if (SameSite() &&
!options.include_same_site())
Should check the value of SameSite here explicitly, now that it's not a bool..

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

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/cookie_cons...
net/cookies/cookie_constants.cc:19: }  // namespace
nit:  Add a blank line after the start / before the end of the namespace/ 
(Without any line breaks in between, seems fine to leave those out, but now that
we have one, seems like the start/end of the namespace is as significant as any
other cluster of things inside it)

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/cookie_cons...
net/cookies/cookie_constants.cc:48: NET_EXPORT const std::string
CookieSameSiteToString(CookieSameSite samesite) {
Remove all NET_EXPORTs from this file...  They should be with the declaration,
not the definition.

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/cookie_cons...
net/cookies/cookie_constants.cc:48: NET_EXPORT const std::string
CookieSameSiteToString(CookieSameSite samesite) {
This method isn't used anywhere (Neither is CookiePriorityToString, looks like,
other than in tests, but that's another matter...).  Get rid of it?

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/cookie_cons...
net/cookies/cookie_constants.cc:63: std::string comp =
base::ToLowerASCII(samesite);
comp seems to violate the Google style guide.

I'd suggest just using base::EqualsCaseInsensitiveASCII.  Think it's clearer,
and we really don't get much from the difference.

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/cookie_cons...
File net/cookies/cookie_constants.h (right):

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/cookie_cons...
net/cookies/cookie_constants.h:21: enum CookieSameSite {
Can we make this an enum class instead?  Seems simpler to do it now rather than
later, and the type safety checks are nifty.

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/cookie_cons...
net/cookies/cookie_constants.h:22: COOKIE_SAME_SITE_NONE = 0,
SAME_SITE_NONE seems kinda weird as a property of a cookie.  Sounds almost like
we're not sending cookies to the "same site".  Could call it
COOKIE_RESTRICT_SAME_SITE_DISABLED or something...or could keep it as-is.  Just
thought I'd mention the name seems a little weird.

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/cookie_cons...
net/cookies/cookie_constants.h:36: NET_EXPORT const std::string
CookieSameSiteToString(CookieSameSite samesite);
This const seems weird (As does the one in CookiePriorityToString).  Doesn't
seem to get us anything.

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/cookie_cons...
net/cookies/cookie_constants.h:36: NET_EXPORT const std::string
CookieSameSiteToString(CookieSameSite samesite);
same_site is what we use everywhere else (x4, for this file and the CC file)

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/cookie_mons...
File net/cookies/cookie_monster_unittest.cc (right):

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/cookie_mons...
net/cookies/cookie_monster_unittest.cc:861: COOKIE_SAME_SITE_NONE,
COOKIE_PRIORITY_DEFAULT));
You're using NONE here, but DEFAULT above.  Is there a reason for this?

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/cookie_mons...
net/cookies/cookie_monster_unittest.cc:2343: COOKIE_SAME_SITE_NONE,
COOKIE_PRIORITY_DEFAULT);
And now you're back to none again.  Should be consistent, unless there's a
reason not to be.

https://codereview.chromium.org/1773133002/diff/80001/net/extras/sqlite/sqlit...
File net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc (right):

https://codereview.chromium.org/1773133002/diff/80001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc:610: static const
char kCookiePath[] = "/";
I think the preference is not to use "static" in this case?  There have been
discussions of that on chromium-dev.

https://codereview.chromium.org/1773133002/diff/80001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc:644: // Put the
cookies into a map, by name, so we can easily find them.
nit:  --"we"

https://codereview.chromium.org/1773133002/diff/80001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc:649: }
optional:  Suggest just using a range loop, and making the map store copies of
the cookies....  Think that makes the loop a little cleaner.

https://codereview.chromium.org/1773133002/diff/80001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc:651: // Validate
that each cookie has the correct samesite.
SameSite?

https://codereview.chromium.org/1773133002/diff/80001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc:654: ASSERT_TRUE(it
!= cookie_map.end());
Combining these two lines seems a little simpler, for all of these:

ASSERT_TRUE(cookie_map.find(kNoneName) != cookie_map.end()).

Or even:  ASSERT_EQ(1u, cookie_map.count(kNoneName));

[mmenke, 2016-03-11 18:09:07]

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/parsed_cook...
File net/cookies/parsed_cookie.h (right):

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/parsed_cook...
net/cookies/parsed_cookie.h:53: bool HasSameSite() const { return
same_site_index_ != 0; }
Also, is this needed?

[marq (ping after 24h), 2016-03-11 21:38:25]

ios/net LGTM

[Mike West, 2016-03-14 10:18:54]

Thanks, all!

mmenke@: I believe that latest patchset addresses your feedback. Mind taking
another look?

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/canonical_c...
File net/cookies/canonical_cookie.cc (right):

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/canonical_c...
net/cookies/canonical_cookie.cc:425: if (SameSite() &&
!options.include_same_site())
On 2016/03/11 at 18:08:30, mmenke wrote:
> Should check the value of SameSite here explicitly, now that it's not a bool..

Yup. Adding a TODO as well to point to the fact that this doesn't yet
distinguish behavior between "strict" and "lax".

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

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/cookie_cons...
net/cookies/cookie_constants.cc:19: }  // namespace
On 2016/03/11 at 18:08:30, mmenke wrote:
> nit:  Add a blank line after the start / before the end of the namespace/ 
(Without any line breaks in between, seems fine to leave those out, but now that
we have one, seems like the start/end of the namespace is as significant as any
other cluster of things inside it)

Done.

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/cookie_cons...
net/cookies/cookie_constants.cc:48: NET_EXPORT const std::string
CookieSameSiteToString(CookieSameSite samesite) {
On 2016/03/11 at 18:08:30, mmenke wrote:
> This method isn't used anywhere (Neither is CookiePriorityToString, looks
like, other than in tests, but that's another matter...).  Get rid of it?

Hrm. You're right. I thought we needed both for serialization somewhere, but
dropping that seems fine as it only ends up happening in tests. I'll give you a
separate CL for the existing method.

> Remove all NET_EXPORTs from this file...  They should be with the declaration,
not the definition.

Done.

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/cookie_cons...
net/cookies/cookie_constants.cc:63: std::string comp =
base::ToLowerASCII(samesite);
On 2016/03/11 at 18:08:30, mmenke wrote:
> comp seems to violate the Google style guide.
> 
> I'd suggest just using base::EqualsCaseInsensitiveASCII.  Think it's clearer,
and we really don't get much from the difference.

Sure.

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/cookie_cons...
File net/cookies/cookie_constants.h (right):

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/cookie_cons...
net/cookies/cookie_constants.h:21: enum CookieSameSite {
On 2016/03/11 at 18:08:30, mmenke wrote:
> Can we make this an enum class instead?  Seems simpler to do it now rather
than later, and the type safety checks are nifty.

Sure.

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/cookie_cons...
net/cookies/cookie_constants.h:22: COOKIE_SAME_SITE_NONE = 0,
On 2016/03/11 at 18:08:30, mmenke wrote:
> SAME_SITE_NONE seems kinda weird as a property of a cookie.  Sounds almost
like we're not sending cookies to the "same site".  Could call it
COOKIE_RESTRICT_SAME_SITE_DISABLED or something...or could keep it as-is.  Just
thought I'd mention the name seems a little weird.

`CookieSameSite::NO_RESTRICTION`?

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/cookie_cons...
net/cookies/cookie_constants.h:36: NET_EXPORT const std::string
CookieSameSiteToString(CookieSameSite samesite);
On 2016/03/11 at 18:08:30, mmenke wrote:
> This const seems weird (As does the one in CookiePriorityToString).  Doesn't
seem to get us anything.

Removed.

> same_site is what we use everywhere else (x4, for this file and the CC file)

Changed.

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/cookie_mons...
File net/cookies/cookie_monster_unittest.cc (right):

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/cookie_mons...
net/cookies/cookie_monster_unittest.cc:861: COOKIE_SAME_SITE_NONE,
COOKIE_PRIORITY_DEFAULT));
On 2016/03/11 at 18:08:31, mmenke wrote:
> You're using NONE here, but DEFAULT above.  Is there a reason for this?

No. No good reason, here or elsewhere.

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/parsed_cook...
File net/cookies/parsed_cookie.h (right):

https://codereview.chromium.org/1773133002/diff/80001/net/cookies/parsed_cook...
net/cookies/parsed_cookie.h:53: bool HasSameSite() const { return
same_site_index_ != 0; }
On 2016/03/11 at 18:09:07, mmenke wrote:
> Also, is this needed?

Not anymore. Removed.

https://codereview.chromium.org/1773133002/diff/80001/net/extras/sqlite/sqlit...
File net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc (right):

https://codereview.chromium.org/1773133002/diff/80001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc:644: // Put the
cookies into a map, by name, so we can easily find them.
On 2016/03/11 at 18:08:31, mmenke wrote:
> nit:  --"we"

1. Ugh. I continue to not like this rule. :/
2. This was copy/pasted from elsewhere. Would you like me to change it in
existing methods as well?

https://codereview.chromium.org/1773133002/diff/80001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc:649: }
On 2016/03/11 at 18:08:31, mmenke wrote:
> optional:  Suggest just using a range loop, and making the map store copies of
the cookies....  Think that makes the loop a little cleaner.

Done.

https://codereview.chromium.org/1773133002/diff/80001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc:651: // Validate
that each cookie has the correct samesite.
On 2016/03/11 at 18:08:31, mmenke wrote:
> SameSite?

Indeed.

https://codereview.chromium.org/1773133002/diff/80001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc:654: ASSERT_TRUE(it
!= cookie_map.end());
On 2016/03/11 at 18:08:31, mmenke wrote:
> Combining these two lines seems a little simpler, for all of these:
> 
> ASSERT_TRUE(cookie_map.find(kNoneName) != cookie_map.end()).
> 
> Or even:  ASSERT_EQ(1u, cookie_map.count(kNoneName));

Yup. Makes sense.

[Mike West, 2016-03-14 13:34:35]

Patchset #7 (id:140001) has been deleted

[Mike West, 2016-03-14 13:34:47]

Patchset #6 (id:120001) has been deleted

[Mike West, 2016-03-14 15:15:08]

https://codereview.chromium.org/1773133002/diff/160001/net/cookies/cookie_con...
File net/cookies/cookie_constants.h (right):

https://codereview.chromium.org/1773133002/diff/160001/net/cookies/cookie_con...
net/cookies/cookie_constants.h:24: STRICT_MODE = 2,
Windows has a "STRICT" macro. So.. "STRICT_MODE"? I added mode to everything,
but I'd appreciate other suggestions...

[mmenke, 2016-03-14 16:25:05]

On 2016/03/14 15:15:08, Mike West wrote:
>
https://codereview.chromium.org/1773133002/diff/160001/net/cookies/cookie_con...
> File net/cookies/cookie_constants.h (right):
> 
>
https://codereview.chromium.org/1773133002/diff/160001/net/cookies/cookie_con...
> net/cookies/cookie_constants.h:24: STRICT_MODE = 2,
> Windows has a "STRICT" macro. So.. "STRICT_MODE"? I added mode to everything,
> but I'd appreciate other suggestions...

There's a good chance I won't get back to this today - if not, will definitely
get to it tomorrow.

[mmenke, 2016-03-14 20:14:50]

LGTM

https://codereview.chromium.org/1773133002/diff/80001/net/extras/sqlite/sqlit...
File net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc (right):

https://codereview.chromium.org/1773133002/diff/80001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc:644: // Put the
cookies into a map, by name, so we can easily find them.
On 2016/03/14 10:18:54, Mike West wrote:
> On 2016/03/11 at 18:08:31, mmenke wrote:
> > nit:  --"we"
> 
> 1. Ugh. I continue to not like this rule. :/
> 2. This was copy/pasted from elsewhere. Would you like me to change it in
> existing methods as well?

Up to you.

https://codereview.chromium.org/1773133002/diff/160001/net/cookies/canonical_...
File net/cookies/canonical_cookie_unittest.cc (right):

https://codereview.chromium.org/1773133002/diff/160001/net/cookies/canonical_...
net/cookies/canonical_cookie_unittest.cc:89: cookie =
CanonicalCookie::Create(url, "A=2; SameSite=Strict", creation_time,
Should we have a lax test somewhere in this file, or are you relying on the
parsed_cookie_unittest.cc for that?  Seems like we should have comparable
strict/lax tests somewhere.  Also, should we have a test with just "; SameSite"
somewhere, to make sure things don't fall over for people who were fooling with
the old behavior?

https://codereview.chromium.org/1773133002/diff/160001/net/cookies/cookie_con...
File net/cookies/cookie_constants.h (right):

https://codereview.chromium.org/1773133002/diff/160001/net/cookies/cookie_con...
net/cookies/cookie_constants.h:24: STRICT_MODE = 2,
On 2016/03/14 15:15:08, Mike West wrote:
> Windows has a "STRICT" macro. So.. "STRICT_MODE"? I added mode to everything,
> but I'd appreciate other suggestions...

Clearly we should rework the draft spec to avoid conflicting with Microsoft's
macros (And maybe LAX, too, to avoid conflicting with airport names?).  :)

I'm fine with MODE - agree it's not great, but don't have a better suggestion. 
:(

https://codereview.chromium.org/1773133002/diff/160001/net/cookies/parsed_coo...
File net/cookies/parsed_cookie_unittest.cc (right):

https://codereview.chromium.org/1773133002/diff/160001/net/cookies/parsed_coo...
net/cookies/parsed_cookie_unittest.cc:174: EXPECT_EQ(COOKIE_PRIORITY_DEFAULT,
pc.Priority());
optional:  Should we keep the SameSite::DEFAULT_MODE here?  Doesn't exactly seem
vital.

[Mike West, 2016-03-15 08:35:17]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2016-03-15 08:35:18]

The patchset sent to the CQ was uploaded after l-g-t-m from torne@chromium.org,
marq@chromium.org, jochen@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1773133002/#ps180001
(title: "mmenke@")

[commit-bot: I haz the power, 2016-03-15 08:35:28]

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

[Mike West, 2016-03-15 09:41:11]

Thanks!

https://codereview.chromium.org/1773133002/diff/160001/net/cookies/canonical_...
File net/cookies/canonical_cookie_unittest.cc (right):

https://codereview.chromium.org/1773133002/diff/160001/net/cookies/canonical_...
net/cookies/canonical_cookie_unittest.cc:89: cookie =
CanonicalCookie::Create(url, "A=2; SameSite=Strict", creation_time,
On 2016/03/14 at 20:14:50, mmenke wrote:
> Should we have a lax test somewhere in this file, or are you relying on the
parsed_cookie_unittest.cc for that?  Seems like we should have comparable
strict/lax tests somewhere.  Also, should we have a test with just "; SameSite"
somewhere, to make sure things don't fall over for people who were fooling with
the old behavior?

I plan to add more detailed tests for the distinction between the modes as part
of the next CL, but you're right that it would be a good idea to verify the
parsing behavior here.

[commit-bot: I haz the power, 2016-03-15 10:08:02]

Committed patchset #7 (id:180001)

[commit-bot: I haz the power, 2016-03-15 10:09:37]

Description was changed from

==========
SameSite: Implement 'Strict'/'Lax' attribute parsing.

https://tools.ietf.org/html/draft-west-first-party-cookies-06 introduced
the notion of "Strict" or "Lax" enforcement of the "SameSite" attribute.
This patch implements the infrastructure changes necessary to support
that distinction, but does not yet implement the behavioral change
(that is, after this patch, `SameSite` will be rejected, while
`SameSite=Strict` and `SameSite=Lax` will have the same behavior that
`SameSite` alone has today).

Most of this patch is occupied with the fairly mechanical process of
swapping out a new 'CookieSameSite' enum for the existing boolean in
various constructors and setters. The most interesting piece is the
change to the storage backend, which now stores 0, 1, or 2 in the
database to represent the possible values, rather than 0 or 1 to
represent the boolean.

BUG=459154
==========

to

==========
SameSite: Implement 'Strict'/'Lax' attribute parsing.

https://tools.ietf.org/html/draft-west-first-party-cookies-06 introduced
the notion of "Strict" or "Lax" enforcement of the "SameSite" attribute.
This patch implements the infrastructure changes necessary to support
that distinction, but does not yet implement the behavioral change
(that is, after this patch, `SameSite` will be rejected, while
`SameSite=Strict` and `SameSite=Lax` will have the same behavior that
`SameSite` alone has today).

Most of this patch is occupied with the fairly mechanical process of
swapping out a new 'CookieSameSite' enum for the existing boolean in
various constructors and setters. The most interesting piece is the
change to the storage backend, which now stores 0, 1, or 2 in the
database to represent the possible values, rather than 0 or 1 to
represent the boolean.

BUG=459154

Committed: https://crrev.com/e1a295845cbc338a564dc04e6e3e69b29ba7862f
Cr-Commit-Position: refs/heads/master@{#381201}
==========

[commit-bot: I haz the power, 2016-03-15 10:09:38]

Patchset 7 (id:??) landed as
https://crrev.com/e1a295845cbc338a564dc04e6e3e69b29ba7862f
Cr-Commit-Position: refs/heads/master@{#381201}