Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(8)

Issue 1783813002: SameSite: Strict/Lax behavior. (Closed)

Created:
3 years, 1 month ago by Mike West
Modified:
3 years, 1 month ago
CC:
cbentzel+watch_chromium.org, chromium-reviews, clamy, creis+watch_chromium.org, darin-cc_chromium.org, jam, nasko+codewatch_chromium.org, nasko
Base URL:
https://chromium.googlesource.com/chromium/src.git@strict-lax
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Patch Set 2 : WIP. #

Total comments: 2

Patch Set 3 : Moar. #

Total comments: 18

Patch Set 4 : mmenke #

Total comments: 7

Patch Set 5 : OOPIF. #

Total comments: 10

Patch Set 6 : Comment. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+508 lines, -128 lines) Patch
M content/browser/frame_host/render_frame_message_filter.cc View 1 2 3 4 2 chunks +13 lines, -1 line 0 comments Download
M content/browser/frame_host/render_frame_message_filter_browsertest.cc View 1 2 3 4 3 chunks +44 lines, -13 lines 0 comments Download
M net/base/registry_controlled_domains/registry_controlled_domain.h View 3 chunks +9 lines, -2 lines 0 comments Download
M net/base/registry_controlled_domains/registry_controlled_domain.cc View 1 2 3 2 chunks +8 lines, -0 lines 0 comments Download
M net/base/registry_controlled_domains/registry_controlled_domain_unittest.cc View 1 2 3 3 chunks +12 lines, -6 lines 0 comments Download
M net/cookies/canonical_cookie.cc View 1 2 3 1 chunk +15 lines, -7 lines 0 comments Download
M net/cookies/canonical_cookie_unittest.cc View 1 2 3 2 chunks +30 lines, -34 lines 0 comments Download
M net/cookies/cookie_monster.cc View 1 2 3 3 chunks +6 lines, -3 lines 0 comments Download
M net/cookies/cookie_options.h View 1 2 3 4 chunks +16 lines, -4 lines 0 comments Download
M net/cookies/cookie_options.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M net/cookies/cookie_store.cc View 1 2 3 1 chunk +2 lines, -1 line 0 comments Download
M net/cookies/cookie_store_unittest.h View 1 2 3 1 chunk +2 lines, -1 line 0 comments Download
M net/url_request/url_request_http_job.cc View 1 2 3 4 5 2 chunks +35 lines, -10 lines 0 comments Download
M net/url_request/url_request_unittest.cc View 1 2 4 chunks +64 lines, -32 lines 0 comments Download
A third_party/WebKit/LayoutTests/http/tests/cookies/resources/echo-json.php View 1 2 3 4 5 1 chunk +7 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/http/tests/cookies/resources/post-cookies-onmessage.php View 1 2 3 4 5 1 chunk +14 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/http/tests/cookies/resources/post-cookies-to-opener.php View 1 2 3 4 5 1 chunk +11 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/http/tests/cookies/resources/testharness-helpers.js View 1 2 3 1 chunk +19 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/http/tests/cookies/same-site/basics.html View 1 2 3 4 5 1 chunk +75 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/http/tests/cookies/same-site/popup-cross-site.html View 1 2 3 1 chunk +26 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/http/tests/cookies/same-site/popup-cross-site-post.html View 1 2 3 1 chunk +32 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/http/tests/cookies/same-site/popup-same-site.html View 1 2 3 1 chunk +21 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/http/tests/cookies/same-site/popup-same-site-post.html View 1 2 3 1 chunk +26 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Document.h View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/dom/Document.cpp View 1 2 3 4 1 chunk +19 lines, -12 lines 0 comments Download

Messages

Total messages: 53 (24 generated)
Mike West
This is a WIP, building on the other patch you're reviewing. When you have time, ...
3 years, 1 month ago (2016-03-14 15:24:10 UTC) #3
Mike West
mmenke@: This is now less WIP, and more ready to review. If you have time, ...
3 years, 1 month ago (2016-03-15 14:48:32 UTC) #10
jochen (gone - plz use gerrit)
lgtm
3 years, 1 month ago (2016-03-15 20:32:06 UTC) #11
Mike West
Friendly ping. :)
3 years, 1 month ago (2016-03-17 05:29:37 UTC) #12
mmenke
https://codereview.chromium.org/1783813002/diff/80001/content/browser/frame_host/render_frame_message_filter.cc File content/browser/frame_host/render_frame_message_filter.cc (right): https://codereview.chromium.org/1783813002/diff/80001/content/browser/frame_host/render_frame_message_filter.cc#newcode398 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 ...
3 years, 1 month ago (2016-03-17 19:15:57 UTC) #13
mmenke
And sorry for the delay....Doesn't feel like I have that many reviews this week, but ...
3 years, 1 month ago (2016-03-17 19:16:38 UTC) #14
mmenke
On 2016/03/17 19:16:38, mmenke wrote: > And sorry for the delay....Doesn't feel like I have ...
3 years, 1 month ago (2016-03-17 19:20:29 UTC) #15
Mike West
A few brief comments; I'll send you code changes tomorrow. Thank you! https://codereview.chromium.org/1783813002/diff/80001/content/browser/frame_host/render_frame_message_filter.cc File content/browser/frame_host/render_frame_message_filter.cc ...
3 years, 1 month ago (2016-03-17 19:57:12 UTC) #16
Mike West
mmenke: Thanks! I've addressed your comments in the latest patchset. jochen: Would you mind taking ...
3 years, 1 month ago (2016-03-18 14:27:17 UTC) #17
mmenke
content/browser/ and net/ LGTM, but you should have someone who knows layout tests review the ...
3 years, 1 month ago (2016-03-18 15:58:13 UTC) #19
Mike West
clamy@/nasko@: It looks like the newly-added tests are failing under site isolation; any ideas where ...
3 years, 1 month ago (2016-03-19 11:23:56 UTC) #21
Mike West
On 2016/03/19 at 11:23:56, Mike West wrote: > clamy@/nasko@: It looks like the newly-added tests ...
3 years, 1 month ago (2016-03-21 08:08:34 UTC) #22
Mike West
philipj@: Would you mind taking a look at just the layout tests? You know things ...
3 years, 1 month ago (2016-03-21 08:38:56 UTC) #24
commit-bot: I haz the power
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
3 years, 1 month ago (2016-03-21 08:59:20 UTC) #27
philipj_slow
Didn't dig too deeply into what this new stuff is, but the tests LGTM with ...
3 years, 1 month ago (2016-03-21 09:18:23 UTC) #28
commit-bot: I haz the power
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_oilpan_rel/builds/19446)
3 years, 1 month ago (2016-03-21 10:15:39 UTC) #30
SGx420
lgtm
3 years, 1 month ago (2016-03-21 10:34:37 UTC) #32
commit-bot: I haz the power
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
3 years, 1 month ago (2016-03-21 10:58:07 UTC) #34
Mike West
https://codereview.chromium.org/1783813002/diff/160001/third_party/WebKit/LayoutTests/http/tests/cookies/resources/post-cookies-onmessage.php File third_party/WebKit/LayoutTests/http/tests/cookies/resources/post-cookies-onmessage.php (right): https://codereview.chromium.org/1783813002/diff/160001/third_party/WebKit/LayoutTests/http/tests/cookies/resources/post-cookies-onmessage.php#newcode5 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 ...
3 years, 1 month ago (2016-03-21 11:03:13 UTC) #37
philipj_slow
https://codereview.chromium.org/1783813002/diff/160001/third_party/WebKit/LayoutTests/http/tests/cookies/same-site/basics.html File third_party/WebKit/LayoutTests/http/tests/cookies/same-site/basics.html (right): https://codereview.chromium.org/1783813002/diff/160001/third_party/WebKit/LayoutTests/http/tests/cookies/same-site/basics.html#newcode20 third_party/WebKit/LayoutTests/http/tests/cookies/same-site/basics.html:20: document.cookie = STRICT_DOM + "=1; SameSite=Strict; domain=" + TEST_HOST ...
3 years, 1 month ago (2016-03-21 11:17:47 UTC) #38
philipj_slow
https://codereview.chromium.org/1783813002/diff/160001/third_party/WebKit/LayoutTests/http/tests/cookies/same-site/basics.html File third_party/WebKit/LayoutTests/http/tests/cookies/same-site/basics.html (right): https://codereview.chromium.org/1783813002/diff/160001/third_party/WebKit/LayoutTests/http/tests/cookies/same-site/basics.html#newcode20 third_party/WebKit/LayoutTests/http/tests/cookies/same-site/basics.html:20: document.cookie = STRICT_DOM + "=1; SameSite=Strict; domain=" + TEST_HOST ...
3 years, 1 month ago (2016-03-21 11:17:47 UTC) #39
commit-bot: I haz the power
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
3 years, 1 month ago (2016-03-21 11:18:25 UTC) #42
Mike West
On 2016/03/21 at 11:17:47, philipj wrote: > https://codereview.chromium.org/1783813002/diff/160001/third_party/WebKit/LayoutTests/http/tests/cookies/same-site/basics.html > File third_party/WebKit/LayoutTests/http/tests/cookies/same-site/basics.html (right): > > https://codereview.chromium.org/1783813002/diff/160001/third_party/WebKit/LayoutTests/http/tests/cookies/same-site/basics.html#newcode20 ...
3 years, 1 month ago (2016-03-21 11:19:06 UTC) #43
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
3 years, 1 month ago (2016-03-21 13:10:58 UTC) #45
commit-bot: I haz the power
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
3 years, 1 month ago (2016-03-21 14:06:30 UTC) #48
commit-bot: I haz the power
Committed patchset #6 (id:220001)
3 years, 1 month ago (2016-03-21 14:15:34 UTC) #49
commit-bot: I haz the power
Patchset 6 (id:??) landed as https://crrev.com/f71d0bde417518f99f977a0ecbf480b375cf49ca Cr-Commit-Position: refs/heads/master@{#382277}
3 years, 1 month ago (2016-03-21 14:17:01 UTC) #51
Mike West
https://codereview.chromium.org/1783813002/diff/120001/net/url_request/url_request_http_job.cc File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/1783813002/diff/120001/net/url_request/url_request_http_job.cc#newcode744 net/url_request/url_request_http_job.cc:744: // once the feature is no longer behind a ...
3 years, 1 month ago (2016-03-21 14:41:01 UTC) #52
mmenke
3 years, 1 month ago (2016-03-22 15:42:30 UTC) #53
Message was sent while issue was closed.
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).

Powered by Google App Engine
This is Rietveld 408576698