SameSite: Strict/Lax behavior.

SameSite: Strict/Lax behavior.

This patch brings our "SameSite" implementation into line with
https://tools.ietf.org/html/draft-west-first-party-cookies-06 by teaching
CookieOptions about strict and lax request modes, and teaching URLRequestHttpJob
about the registrable-domain behaviors of both.

BUG=459154
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/f71d0bde417518f99f977a0ecbf480b375cf49ca
Cr-Commit-Position: refs/heads/master@{#382277}

[Mike West, 2016-03-10 15:27:01]

Description was changed from

==========
WIP: SameSite: Strict/Lax behavior.

Needs lots of tests to bring it into line with
https://tools.ietf.org/html/draft-west-first-party-cookies-06.

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

to

==========
WIP: SameSite: Strict/Lax behavior.

Needs lots of tests to bring it into line with
https://tools.ietf.org/html/draft-west-first-party-cookies-06.

BUG=459154
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Mike West, 2016-03-14 15:24:09]

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

[Mike West, 2016-03-14 15:24:10]

This is a WIP, building on the other patch you're reviewing. When you have time,
I'd appreciate you giving it a preliminary skim. I'll clean it up and add some
more tests once we've settled on the other patch. :)

Thanks! (And, if I should be distributing cookie reviews to other folks, do let
me know. I appreciate your feedback, but I know you're overloaded with reviews.)

https://codereview.chromium.org/1783813002/diff/20001/net/cookies/cookie_opti...
File net/cookies/cookie_options.h (right):

https://codereview.chromium.org/1783813002/diff/20001/net/cookies/cookie_opti...
net/cookies/cookie_options.h:40: CookieSameSite include_same_site() const {
return include_same_site_; }
TODO(me): Rename this to something like
`set_which_same_site_cookies_should_be_included` but shorter and sane.

https://codereview.chromium.org/1783813002/diff/20001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/1783813002/diff/20001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:748: }
This is the core of the change.

[Mike West, 2016-03-15 14:45:50]

Patchset #4 (id:60001) has been deleted

[Mike West, 2016-03-15 14:45:56]

Patchset #4 (id:60001) has been deleted

[Mike West, 2016-03-15 14:46:04]

Patchset #3 (id:40001) has been deleted

[Mike West, 2016-03-15 14:47:27]

Description was changed from

==========
WIP: SameSite: Strict/Lax behavior.

Needs lots of tests to bring it into line with
https://tools.ietf.org/html/draft-west-first-party-cookies-06.

BUG=459154
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
SameSite: Strict/Lax behavior.

This patch brings our "SameSite" implementation into line with
https://tools.ietf.org/html/draft-west-first-party-cookies-06 by teaching
CookieOptions about strict and lax request modes, and teaching URLRequestHttpJob
about the registrable-domain behaviors of both.

BUG=459154
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Mike West, 2016-03-15 14:48:31]

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

[Mike West, 2016-03-15 14:48:31]

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

[Mike West, 2016-03-15 14:48:32]

mmenke@: This is now less WIP, and more ready to review. If you have time, I'd
appreciate you giving it a look.

Jochen: Would you mind stamping the trivial change to RenderFrameMessageFilter?

[jochen (gone - plz use gerrit), 2016-03-15 20:32:06]

lgtm

[Mike West, 2016-03-17 05:29:37]

Friendly ping. :)

[mmenke, 2016-03-17 19:15:57]

https://codereview.chromium.org/1783813002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_message_filter.cc (right):

https://codereview.chromium.org/1783813002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_message_filter.cc:398:
net::CookieOptions::SameSiteMode::INCLUDE_STRICT_AND_LAX);
Is this right?  Seems like for an iframe with an origin that doesn't match that
of one of its parent frames, this should be
net::CookieOptions::SameSiteMode::INCLUDE_LAX.

https://codereview.chromium.org/1783813002/diff/80001/net/base/registry_contr...
File net/base/registry_controlled_domains/registry_controlled_domain.cc (right):

https://codereview.chromium.org/1783813002/diff/80001/net/base/registry_contr...
net/base/registry_controlled_domains/registry_controlled_domain.cc:222: if
(origin1.unique() || origin2.unique())
Why doesn't SameDomainOrHost need this check?  Also, is this check tested?

https://codereview.chromium.org/1783813002/diff/80001/net/base/registry_contr...
net/base/registry_controlled_domains/registry_controlled_domain.cc:226: filter);
This seems backwards.  Really seems like SameDomainOrHost(gurl1, gurl2) should
just be calling SameDomainOrHost(gurl1.GetOrigin(), gurl2.GetOrigin());

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

https://codereview.chromium.org/1783813002/diff/80001/net/cookies/canonical_c...
net/cookies/canonical_cookie.cc:425: if (SameSite() ==
CookieSameSite::STRICT_MODE &&
Suggest a switch statement, just to make clear how these are related to each
other.

https://codereview.chromium.org/1783813002/diff/80001/net/cookies/cookie_opti...
File net/cookies/cookie_options.h (right):

https://codereview.chromium.org/1783813002/diff/80001/net/cookies/cookie_opti...
net/cookies/cookie_options.h:22: DO_NOT_INCLUDE
SameSiteMode::DO_NOT_INCLUDE seems weird...Maybe SameSiteCookieMode?  The
"cookie" may seem redundant, but it's not a mode for what we do if the site is
the same, but what we do for same site cookies, so think it's clearer, if rather
verbose.

https://codereview.chromium.org/1783813002/diff/80001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/1783813002/diff/80001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:742:
registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES)) {
Add draft RFC link here?  (Could add it near either enum instead, but think it's
easiest to follow the enums here, then from one enum to here to the other one)

https://codereview.chromium.org/1783813002/diff/80001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/1783813002/diff/80001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2664: const std::string kCrossHost =
"cross-origin.test";
I'm fine with these tests, but seems like we should have unit tests in
canonical_cookie_unittest.cc.  Also, should we have integration tests at the
browser layer (Or, ick, layout tests)?  Do we have tests that depend on the
initiator being set correctly by the RDH?

[mmenke, 2016-03-17 19:16:38]

And sorry for the delay....Doesn't feel like I have that many reviews this week,
but sometime, not managing to keep up with them, and not getting any coding
done, either...Hrm...Not sure how that works.

[mmenke, 2016-03-17 19:20:29]

On 2016/03/17 19:16:38, mmenke wrote:
> And sorry for the delay....Doesn't feel like I have that many reviews this
week,
> but sometime, not managing to keep up with them, and not getting any coding
> done, either...Hrm...Not sure how that works.

Erm..sometime == somehow.

[Mike West, 2016-03-17 19:57:12]

A few brief comments; I'll send you code changes tomorrow. Thank you!

https://codereview.chromium.org/1783813002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_message_filter.cc (right):

https://codereview.chromium.org/1783813002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_message_filter.cc:398:
net::CookieOptions::SameSiteMode::INCLUDE_STRICT_AND_LAX);
On 2016/03/17 at 19:15:56, mmenke wrote:
> Is this right?  Seems like for an iframe with an origin that doesn't match
that of one of its parent frames, this should be
net::CookieOptions::SameSiteMode::INCLUDE_LAX.

Hrm. I think you're right, which is unfortunate, as it's going to make this more
complicated. Not much more complicated, though; we just need to compare the
registrable domains of |url| and |first_party_for_cookies|, as the latter takes
the kinds of things into account that you're mentioning.

https://codereview.chromium.org/1783813002/diff/80001/net/base/registry_contr...
File net/base/registry_controlled_domains/registry_controlled_domain.cc (right):

https://codereview.chromium.org/1783813002/diff/80001/net/base/registry_contr...
net/base/registry_controlled_domains/registry_controlled_domain.cc:222: if
(origin1.unique() || origin2.unique())
On 2016/03/17 at 19:15:56, mmenke wrote:
> Why doesn't SameDomainOrHost need this check?  Also, is this check tested?

This is leftover from a previous version; we don't need this check (as unique
origins serialize to "null", which is an invalid GURL, which means that the GURL
version will return `false`).

https://codereview.chromium.org/1783813002/diff/80001/net/base/registry_contr...
net/base/registry_controlled_domains/registry_controlled_domain.cc:226: filter);
On 2016/03/17 at 19:15:56, mmenke wrote:
> This seems backwards.  Really seems like SameDomainOrHost(gurl1, gurl2) should
just be calling SameDomainOrHost(gurl1.GetOrigin(), gurl2.GetOrigin());

I thought about that, but we'd need to add some infrastructure to `url::Origin`.
In particular, `SameDomainOrHost` relies on `GetDomainOrRegistry`, which calls
out to `GURL::HostIsIPAddress`. That relies on some URL parsing infrastructure
that assumes it has a whole URL, and relies on canonicalizing the host
`url::Component`. It turned into a whole lot of code for something that's a lot
simpler to handle by going from an `url::Origin` to `GURL`.

I can go that route if you think it's worthwhile, but it will take a CL or three
to get through.

https://codereview.chromium.org/1783813002/diff/80001/net/cookies/cookie_opti...
File net/cookies/cookie_options.h (right):

https://codereview.chromium.org/1783813002/diff/80001/net/cookies/cookie_opti...
net/cookies/cookie_options.h:22: DO_NOT_INCLUDE
On 2016/03/17 at 19:15:56, mmenke wrote:
> SameSiteMode::DO_NOT_INCLUDE seems weird...Maybe SameSiteCookieMode?  The
"cookie" may seem redundant, but it's not a mode for what we do if the site is
the same, but what we do for same site cookies, so think it's clearer, if rather
verbose.

I come from Blink. I was born in verbosity. :)

https://codereview.chromium.org/1783813002/diff/80001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/1783813002/diff/80001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:2664: const std::string kCrossHost =
"cross-origin.test";
On 2016/03/17 at 19:15:57, mmenke wrote:
> I'm fine with these tests, but seems like we should have unit tests in
canonical_cookie_unittest.cc.

You're right. These are more or less integration tests... I'm happy to add some
more unit tests tomorrow.

> Also, should we have integration tests at the browser layer (Or, ick, layout
tests)?

I plan to write layout tests to help kickstart Mozilla's progress; I'm working
with folks there to try to teach their test engine about
cross-registrable-domain origins
(https://github.com/w3c/web-platform-tests/issues/2669). It's going to take some
time... :(

> Do we have tests that depend on the initiator being set correctly by the RDH?

That behavior shouldn't have changed with this CL, so I hope the existing tests
do the job. I assume I wrote tests... :) I can look them up in the morning.

[Mike West, 2016-03-18 14:27:17]

mmenke: Thanks! I've addressed your comments in the latest patchset.

jochen: Would you mind taking a look at the layout tests I've added to this CL?
I can't put them up on WPT, as that server doesn't support multiple registrable
domains. Working on it, but it's worth putting them into our LayoutTests in the
meantime. WDYT?

https://codereview.chromium.org/1783813002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_message_filter.cc (right):

https://codereview.chromium.org/1783813002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_message_filter.cc:398:
net::CookieOptions::SameSiteMode::INCLUDE_STRICT_AND_LAX);
On 2016/03/17 at 19:57:11, Mike West wrote:
> On 2016/03/17 at 19:15:56, mmenke wrote:
> > Is this right?  Seems like for an iframe with an origin that doesn't match
that of one of its parent frames, this should be
net::CookieOptions::SameSiteMode::INCLUDE_LAX.

Done, and added a browser test to cover.

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

https://codereview.chromium.org/1783813002/diff/80001/net/cookies/canonical_c...
net/cookies/canonical_cookie.cc:425: if (SameSite() ==
CookieSameSite::STRICT_MODE &&
On 2016/03/17 at 19:15:56, mmenke wrote:
> Suggest a switch statement, just to make clear how these are related to each
other.

I've turned this into a switch, but I'm not actually sure it's any clearer...
WDYT?

https://codereview.chromium.org/1783813002/diff/80001/net/cookies/cookie_opti...
File net/cookies/cookie_options.h (right):

https://codereview.chromium.org/1783813002/diff/80001/net/cookies/cookie_opti...
net/cookies/cookie_options.h:22: DO_NOT_INCLUDE
On 2016/03/17 at 19:57:12, Mike West wrote:
> On 2016/03/17 at 19:15:56, mmenke wrote:
> > SameSiteMode::DO_NOT_INCLUDE seems weird...Maybe SameSiteCookieMode?  The
"cookie" may seem redundant, but it's not a mode for what we do if the site is
the same, but what we do for same site cookies, so think it's clearer, if rather
verbose.
> 
> I come from Blink. I was born in verbosity. :)

Done.

https://codereview.chromium.org/1783813002/diff/80001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/1783813002/diff/80001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:742:
registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES)) {
On 2016/03/17 at 19:15:57, mmenke wrote:
> Add draft RFC link here?  (Could add it near either enum instead, but think
it's easiest to follow the enums here, then from one enum to here to the other
one)

I added a comment block at the top of this section which I hope helps.

It might make sense to move all of this to `URLRequest`, actually, just leaving
something like
`options.set_same_site_cookie_mode(request_.same_site_cookie_mode())`. I didn't
do that here because I recall you wanting to slim down `URLRequest`'s surface
area, and this piece is really only used once.

https://codereview.chromium.org/1783813002/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_message_filter.cc (right):

https://codereview.chromium.org/1783813002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_message_filter.cc:402: // initiated in a
strict or lax same-site context.,[
Honestly, I need to think about whether or not this is a bug. I think it is, but
maybe the behavior makes sense? Idunno. It's not clear that we need to reason
about the initiator after we've already opened and rendered a document.

[Mike West, 2016-03-18 14:27:30]

Patchset #4 (id:100001) has been deleted

[mmenke, 2016-03-18 15:58:13]

content/browser/ and net/ LGTM, but you should have someone who knows layout
tests review the layout test code, as I can't make anything out of them.

https://codereview.chromium.org/1783813002/diff/80001/net/base/registry_contr...
File net/base/registry_controlled_domains/registry_controlled_domain.cc (right):

https://codereview.chromium.org/1783813002/diff/80001/net/base/registry_contr...
net/base/registry_controlled_domains/registry_controlled_domain.cc:226: filter);
On 2016/03/17 19:57:12, Mike West wrote:
> On 2016/03/17 at 19:15:56, mmenke wrote:
> > This seems backwards.  Really seems like SameDomainOrHost(gurl1, gurl2)
should
> just be calling SameDomainOrHost(gurl1.GetOrigin(), gurl2.GetOrigin());
> 
> I thought about that, but we'd need to add some infrastructure to
`url::Origin`.
> In particular, `SameDomainOrHost` relies on `GetDomainOrRegistry`, which calls
> out to `GURL::HostIsIPAddress`. That relies on some URL parsing infrastructure
> that assumes it has a whole URL, and relies on canonicalizing the host
> `url::Component`. It turned into a whole lot of code for something that's a
lot
> simpler to handle by going from an `url::Origin` to `GURL`.
> 
> I can go that route if you think it's worthwhile, but it will take a CL or
three
> to get through.

Didn't respond to this, but I think it's fine for now.  Longer term, may be
worth doing, though.

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

https://codereview.chromium.org/1783813002/diff/80001/net/cookies/canonical_c...
net/cookies/canonical_cookie.cc:425: if (SameSite() ==
CookieSameSite::STRICT_MODE &&
On 2016/03/18 14:27:17, Mike West wrote:
> On 2016/03/17 at 19:15:56, mmenke wrote:
> > Suggest a switch statement, just to make clear how these are related to each
> other.
> 
> I've turned this into a switch, but I'm not actually sure it's any clearer...
> WDYT?

I think it's marginally better, but not really a huge improvement.  IF you
disagree, feel free to revert.

https://codereview.chromium.org/1783813002/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_message_filter.cc (right):

https://codereview.chromium.org/1783813002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_message_filter.cc:378: const GURL&
first_party_for_cookies,
first_party_for_cookies is the main frame URL here?

https://codereview.chromium.org/1783813002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_message_filter.cc:402: // initiated in a
strict or lax same-site context.,[
On 2016/03/18 14:27:17, Mike West wrote:
> Honestly, I need to think about whether or not this is a bug. I think it is,
but
> maybe the behavior makes sense? Idunno. It's not clear that we need to reason
> about the initiator after we've already opened and rendered a document.

My feeling is that the cookies we send while requesting the HTML file should
exactly match the cookies that the HTML file can access, modulo new cookies and
expiring cookies.  Anything else seems weird to me.

If the thinking is that the iframe knows its an iframe, and can behavior
correspondingly, seems like we could just send that information as a request
header instead of a new SameSite value - that makes the cookie security model
consistent.  If the idea is we should hide cookies based on the "initiator",
seems like should be defined the same way for HTTP and HTML.

https://codereview.chromium.org/1783813002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_message_filter.cc:402: // initiated in a
strict or lax same-site context.,[
nit:  Remove the ",["

https://codereview.chromium.org/1783813002/diff/120001/net/url_request/url_re...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/1783813002/diff/120001/net/url_request/url_re...
net/url_request/url_request_http_job.cc:729: // Set |options|'
SameSiteCookieMode according to the rules laid out in
As-is, this comment really confused me as I tried to parse it.  |options|'s? 
|options| is a variable name, so it's singular.  Or maybe just remove
|options|'?

https://codereview.chromium.org/1783813002/diff/120001/net/url_request/url_re...
net/url_request/url_request_http_job.cc:744: // once the feature is no longer
behind a flag: https://crbug.com/459154.
Just a comment on the design of this stuff:  Letting an iframe access a lax
cookie, but then hiding it from POSTs generated by that iframe seems like a
behavior that's going to cause developer confusion.

[Mike West, 2016-03-19 11:23:56]

mkwst@chromium.org changed reviewers:
+ clamy@chromium.org, nasko@chromium.org

[Mike West, 2016-03-19 11:23:56]

clamy@/nasko@: It looks like the newly-added tests are failing under site
isolation; any ideas where I might start looking?

[Mike West, 2016-03-21 08:08:34]

On 2016/03/19 at 11:23:56, Mike West wrote:
> clamy@/nasko@: It looks like the newly-added tests are failing under site
isolation; any ideas where I might start looking?

`Document::firstPartyForCookies` was relying on `Document::topDocument` which
doesn't work at all under site isolation. This CL now changes the former
method's implementation to work with the SecurityContext stored on the frame
tree. It's a bit broken, as it doesn't properly account for sandboxed iframes
(where we want the URL rather than the origin), but since sandboxed frames are
blocked from accessing `document.cookie` anyway, it should have minimal direct
impact.

[Mike West, 2016-03-21 08:38:55]

mkwst@chromium.org changed reviewers:
+ philipj@opera.com
- clamy@chromium.org, nasko@chromium.org

[Mike West, 2016-03-21 08:38:56]

philipj@: Would you mind taking a look at just the layout tests? You know things
about `testharness.js`, you see... :)

[Mike West, 2016-03-21 08:58:57]

Patchset #5 (id:140001) has been deleted

[Mike West, 2016-03-21 08:59:06]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-21 08:59:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1783813002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1783813002/160001

[philipj_slow, 2016-03-21 09:18:23]

Didn't dig too deeply into what this new stuff is, but the tests LGTM with a few
comments/questions.

https://codereview.chromium.org/1783813002/diff/160001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/cookies/same-site/basics.html
(right):

https://codereview.chromium.org/1783813002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/cookies/same-site/basics.html:13:
console.log(document.cookie);
Leftover from debugging, or nice to have if the test starts failing?

https://codereview.chromium.org/1783813002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/cookies/same-site/basics.html:16:
test(t => {
s/t/_/ since it's not used

https://codereview.chromium.org/1783813002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/cookies/same-site/basics.html:20:
document.cookie = STRICT_DOM + "=1; SameSite=Strict; domain=" + TEST_HOST + ";
path=/";
Why is the cookie set both on ORIGINAL_HOST and now again on TEST_HOST? It makes
it not so obvious which of the two have an effect below. If the answer is mixed,
maybe set =2 here or something?

https://codereview.chromium.org/1783813002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/cookies/same-site/basics.html:30:
assert_equals(j[STRICT_DOM], "1", "strict");
Nice how t.step_func wrapping is no longer needed with promise_test :)

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

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-21 10:15:39]

Dry run: Try jobs failed on following builders:
  linux_blink_oilpan_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_blink_oil...)

[SGx420, 2016-03-21 10:34:36]

sgx420@gmail.com changed reviewers:
+ SGx420@gmail.com

[SGx420, 2016-03-21 10:34:37]

lgtm

[Mike West, 2016-03-21 10:58:03]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-21 10:58:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1783813002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1783813002/200001

[Mike West, 2016-03-21 10:58:10]

Patchset #6 (id:180001) has been deleted

[Mike West, 2016-03-21 11:03:12]

mkwst@chromium.org changed reviewers:
+ sgx420@gmail.com
- SGx420@gmail.com

[Mike West, 2016-03-21 11:03:13]

https://codereview.chromium.org/1783813002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/cookies/resources/post-cookies-onmessage.php
(right):

https://codereview.chromium.org/1783813002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/cookies/resources/post-cookies-onmessage.php:5:
echo json_encode($_COOKIE, JSON_PRETTY_PRINT);
I think our linux trybots don't support JSON_PRETTY_PRINT yet. Which is
strange... filed https://bugs.chromium.org/p/chromium/issues/detail?id=596425

https://codereview.chromium.org/1783813002/diff/160001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/cookies/same-site/basics.html
(right):

https://codereview.chromium.org/1783813002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/cookies/same-site/basics.html:13:
console.log(document.cookie);
On 2016/03/21 at 09:18:23, philipj_UTC7 wrote:
> Leftover from debugging, or nice to have if the test starts failing?

Leftover, dropped, thanks.

https://codereview.chromium.org/1783813002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/cookies/same-site/basics.html:16:
test(t => {
On 2016/03/21 at 09:18:23, philipj_UTC7 wrote:
> s/t/_/ since it's not used

Done.

https://codereview.chromium.org/1783813002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/cookies/same-site/basics.html:20:
document.cookie = STRICT_DOM + "=1; SameSite=Strict; domain=" + TEST_HOST + ";
path=/";
On 2016/03/21 at 09:18:23, philipj_UTC7 wrote:
> Why is the cookie set both on ORIGINAL_HOST and now again on TEST_HOST? It
makes it not so obvious which of the two have an effect below. If the answer is
mixed, maybe set =2 here or something?

Because we need to do cross-registrable domain requests in order to verify
behavior (see the last promise_test, which grabs data from ORIGINAL_HOST and not
TEST_HOST. I can flip it to "2" if that helps disambiguate.

https://codereview.chromium.org/1783813002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/cookies/same-site/basics.html:30:
assert_equals(j[STRICT_DOM], "1", "strict");
On 2016/03/21 at 09:18:23, philipj_UTC7 wrote:
> Nice how t.step_func wrapping is no longer needed with promise_test :)

Indeed!

[philipj_slow, 2016-03-21 11:17:47]

https://codereview.chromium.org/1783813002/diff/160001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/cookies/same-site/basics.html
(right):

https://codereview.chromium.org/1783813002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/cookies/same-site/basics.html:20:
document.cookie = STRICT_DOM + "=1; SameSite=Strict; domain=" + TEST_HOST + ";
path=/";
On 2016/03/21 11:03:13, Mike West wrote:
> On 2016/03/21 at 09:18:23, philipj_UTC7 wrote:
> > Why is the cookie set both on ORIGINAL_HOST and now again on TEST_HOST? It
> makes it not so obvious which of the two have an effect below. If the answer
is
> mixed, maybe set =2 here or something?
> 
> Because we need to do cross-registrable domain requests in order to verify
> behavior (see the last promise_test, which grabs data from ORIGINAL_HOST and
not
> TEST_HOST. I can flip it to "2" if that helps disambiguate.

Oh, OK. Using "2" would make it obvious which value is used where and that
there's no mistake, so that'd be nice.

[philipj_slow, 2016-03-21 11:17:47]

https://codereview.chromium.org/1783813002/diff/160001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/cookies/same-site/basics.html
(right):

https://codereview.chromium.org/1783813002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/cookies/same-site/basics.html:20:
document.cookie = STRICT_DOM + "=1; SameSite=Strict; domain=" + TEST_HOST + ";
path=/";
On 2016/03/21 11:03:13, Mike West wrote:
> On 2016/03/21 at 09:18:23, philipj_UTC7 wrote:
> > Why is the cookie set both on ORIGINAL_HOST and now again on TEST_HOST? It
> makes it not so obvious which of the two have an effect below. If the answer
is
> mixed, maybe set =2 here or something?
> 
> Because we need to do cross-registrable domain requests in order to verify
> behavior (see the last promise_test, which grabs data from ORIGINAL_HOST and
not
> TEST_HOST. I can flip it to "2" if that helps disambiguate.

Oh, OK. Using "2" would make it obvious which value is used where and that
there's no mistake, so that'd be nice.

[Mike West, 2016-03-21 11:18:05]

Patchset #6 (id:200001) has been deleted

[Mike West, 2016-03-21 11:18:12]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-21 11:18:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1783813002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1783813002/220001

[Mike West, 2016-03-21 11:19:06]

On 2016/03/21 at 11:17:47, philipj wrote:
>
https://codereview.chromium.org/1783813002/diff/160001/third_party/WebKit/Lay...
> File third_party/WebKit/LayoutTests/http/tests/cookies/same-site/basics.html
(right):
> 
>
https://codereview.chromium.org/1783813002/diff/160001/third_party/WebKit/Lay...
> third_party/WebKit/LayoutTests/http/tests/cookies/same-site/basics.html:20:
document.cookie = STRICT_DOM + "=1; SameSite=Strict; domain=" + TEST_HOST + ";
path=/";
> On 2016/03/21 11:03:13, Mike West wrote:
> > On 2016/03/21 at 09:18:23, philipj_UTC7 wrote:
> > > Why is the cookie set both on ORIGINAL_HOST and now again on TEST_HOST? It
> > makes it not so obvious which of the two have an effect below. If the answer
is
> > mixed, maybe set =2 here or something?
> > 
> > Because we need to do cross-registrable domain requests in order to verify
> > behavior (see the last promise_test, which grabs data from ORIGINAL_HOST and
not
> > TEST_HOST. I can flip it to "2" if that helps disambiguate.
> 
> Oh, OK. Using "2" would make it obvious which value is used where and that
there's no mistake, so that'd be nice.

Done, thanks!

[commit-bot: I haz the power, 2016-03-21 13:10:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-21 13:10:58]

Dry run: This issue passed the CQ dry run.

[Mike West, 2016-03-21 14:06:09]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2016-03-21 14:06:09]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
mmenke@chromium.org, philipj@opera.com, SGx420@gmail.com
Link to the patchset: https://codereview.chromium.org/1783813002/#ps220001
(title: "Comment.")

[commit-bot: I haz the power, 2016-03-21 14:06:30]

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

[commit-bot: I haz the power, 2016-03-21 14:15:34]

Committed patchset #6 (id:220001)

[commit-bot: I haz the power, 2016-03-21 14:16:59]

Description was changed from

==========
SameSite: Strict/Lax behavior.

This patch brings our "SameSite" implementation into line with
https://tools.ietf.org/html/draft-west-first-party-cookies-06 by teaching
CookieOptions about strict and lax request modes, and teaching URLRequestHttpJob
about the registrable-domain behaviors of both.

BUG=459154
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
SameSite: Strict/Lax behavior.

This patch brings our "SameSite" implementation into line with
https://tools.ietf.org/html/draft-west-first-party-cookies-06 by teaching
CookieOptions about strict and lax request modes, and teaching URLRequestHttpJob
about the registrable-domain behaviors of both.

BUG=459154
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

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

[commit-bot: I haz the power, 2016-03-21 14:17:01]

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

[Mike West, 2016-03-21 14:41:01]

https://codereview.chromium.org/1783813002/diff/120001/net/url_request/url_re...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/1783813002/diff/120001/net/url_request/url_re...
net/url_request/url_request_http_job.cc:744: // once the feature is no longer
behind a flag: https://crbug.com/459154.
On 2016/03/18 at 15:58:13, mmenke wrote:
> Just a comment on the design of this stuff:  Letting an iframe access a lax
cookie, but then hiding it from POSTs generated by that iframe seems like a
behavior that's going to cause developer confusion.

I don't think there's a scenario in which an request in an iframe would be
same-site enough to get a lax cookie but not a strict cookie. The document is
either same-site with its embedder, in which case it gets both, or it's not
same-site with its embedder, in which case it gets neither.

Strict/lax only has effect on things like redirects and popups, which create a
top-level browsing context whose "first-party for cookies" matches the URL
that's being loaded (modulo the `URLRequest::FirstPartyURLPolicy`). I agree that
this might cause confusion, but my hope is that the majority of the confusion
will actually be on the part of attackers trying to execute CSRF attacks. :)

I've updated the comment in an effort to make that more clear.

[mmenke, 2016-03-22 15:42:30]

On 2016/03/21 14:41:01, Mike West wrote:
>
https://codereview.chromium.org/1783813002/diff/120001/net/url_request/url_re...
> File net/url_request/url_request_http_job.cc (right):
> 
>
https://codereview.chromium.org/1783813002/diff/120001/net/url_request/url_re...
> net/url_request/url_request_http_job.cc:744: // once the feature is no longer
> behind a flag: https://crbug.com/459154.
> On 2016/03/18 at 15:58:13, mmenke wrote:
> > Just a comment on the design of this stuff:  Letting an iframe access a lax
> cookie, but then hiding it from POSTs generated by that iframe seems like a
> behavior that's going to cause developer confusion.
> 
> I don't think there's a scenario in which an request in an iframe would be
> same-site enough to get a lax cookie but not a strict cookie. The document is
> either same-site with its embedder, in which case it gets both, or it's not
> same-site with its embedder, in which case it gets neither.
> 
> Strict/lax only has effect on things like redirects and popups, which create a
> top-level browsing context whose "first-party for cookies" matches the URL
> that's being loaded (modulo the `URLRequest::FirstPartyURLPolicy`). I agree
that
> this might cause confusion, but my hope is that the majority of the confusion
> will actually be on the part of attackers trying to execute CSRF attacks. :)
> 
> I've updated the comment in an effort to make that more clear.

Ahh...I missed that Lax was for main frame only (And double-checking, that
squares with the url_request_http_job code.  The code is sufficiently different
from the RFC that this is non-obvious).