Bug 159464 - Implement Same-Site cookies
Summary: Implement Same-Site cookies
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on: 184903
Blocks: 184894 184897 187197
  Show dependency treegraph
 
Reported: 2016-07-06 05:29 PDT by Mike West
Modified: 2019-12-02 03:19 PST (History)
26 users (show)

See Also:


Attachments
Patch and layout tests (227.53 KB, patch)
2018-04-17 17:11 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout tests (226.74 KB, patch)
2018-04-17 17:17 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout tests (231.29 KB, patch)
2018-04-17 18:18 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout tests (231.40 KB, patch)
2018-04-17 18:31 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout tests (241.73 KB, patch)
2018-04-17 20:08 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout tests (241.72 KB, patch)
2018-04-17 22:55 PDT, Daniel Bates
bfulgham: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.58 MB, application/zip)
2018-04-18 04:42 PDT, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mike West 2016-07-06 05:29:41 PDT
https://tools.ietf.org/html/draft-ietf-httpbis-cookie-same-site defines a `SameSite` cookie attribute which allows servers to mitigate the risk of cross-site request forgery attacks, as well as some kinds of cross-origin information leakage.

Chrome shipped this feature in 51 (https://bugs.chromium.org/p/chromium/issues/detail?id=459154#c32), Firefox is working on an implementation (https://bugzilla.mozilla.org/show_bug.cgi?id=795346). It would be lovely if WebKit could do the same.
Comment 1 Mike West 2016-07-06 05:31:21 PDT
John, Brent: I know cookies are outside your purview, but perhaps you could triage this to the right folks?
Comment 2 John Wilander 2016-07-06 09:01:33 PDT
rdar://problem/27196358
Comment 3 Mike West 2016-07-07 09:01:40 PDT
I've started putting together a test suite at http://rfc6265.biz/tests/, which might be helpful if y'all start taking a closer look at this.
Comment 4 Daniel Bates 2018-04-17 15:04:24 PDT
(In reply to Mike West from comment #3)
> I've started putting together a test suite at http://rfc6265.biz/tests/,
> which might be helpful if y'all start taking a closer look at this.

The tests seem to be broken. The first four tests either all fail or timeout when run in Chrome Canary 68.0.3398.0. I did not continue to test the other tests linked off of this site given the success of the first four.
Comment 5 Daniel Bates 2018-04-17 15:05:03 PDT
(In reply to John Wilander from comment #2)
> rdar://problem/27196358

This was duped to...
Comment 6 Daniel Bates 2018-04-17 15:05:13 PDT
<rdar://problem/27196358>
Comment 7 Daniel Bates 2018-04-17 17:11:54 PDT
Created attachment 338163 [details]
Patch and layout tests
Comment 8 Daniel Bates 2018-04-17 17:14:24 PDT
Comment on attachment 338163 [details]
Patch and layout tests

View in context: https://bugs.webkit.org/attachment.cgi?id=338163&action=review

> Source/WebCore/loader/CookieJar.cpp:65
> +    const auto& request = document.loader()->request();
> +    return SameSiteInfo::create(request);

Will inline line 64 into line 65 such that this reads:

return SameSiteInfo::create(document.loader()->request());
Comment 9 EWS Watchlist 2018-04-17 17:14:40 PDT Comment hidden (obsolete)
Comment 10 Daniel Bates 2018-04-17 17:17:25 PDT
Created attachment 338166 [details]
Patch and layout tests
Comment 11 EWS Watchlist 2018-04-17 17:21:22 PDT Comment hidden (obsolete)
Comment 12 Daniel Bates 2018-04-17 18:18:40 PDT
Created attachment 338174 [details]
Patch and layout tests
Comment 13 EWS Watchlist 2018-04-17 18:21:58 PDT Comment hidden (obsolete)
Comment 14 Daniel Bates 2018-04-17 18:31:09 PDT
Created attachment 338175 [details]
Patch and layout tests
Comment 15 EWS Watchlist 2018-04-17 18:34:16 PDT Comment hidden (obsolete)
Comment 16 Daniel Bates 2018-04-17 20:08:42 PDT
Created attachment 338191 [details]
Patch and layout tests
Comment 17 EWS Watchlist 2018-04-17 20:11:46 PDT Comment hidden (obsolete)
Comment 18 Daniel Bates 2018-04-17 22:55:32 PDT
Created attachment 338197 [details]
Patch and layout tests
Comment 19 EWS Watchlist 2018-04-17 22:58:13 PDT
Attachment 338197 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/mac/CookieJarMac.mm:73:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 111 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 EWS Watchlist 2018-04-18 04:42:16 PDT
Comment on attachment 338197 [details]
Patch and layout tests

Attachment 338197 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7355865

New failing tests:
http/tests/preload/onload_event.html
Comment 21 EWS Watchlist 2018-04-18 04:42:27 PDT
Created attachment 338209 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 22 Brent Fulgham 2018-04-18 20:41:39 PDT
Comment on attachment 338197 [details]
Patch and layout tests

View in context: https://bugs.webkit.org/attachment.cgi?id=338197&action=review

This looks reasonable. Is it safe to land for internal machines?

> Source/WebCore/loader/CookieJar.cpp:64
> +    return SameSiteInfo::create(document.loader()->request());

If we are inlining, might this be better?

if (auto loader = document.loader())
    return SameSiteInfo::create(loader->request());

return { }]

> Source/WebCore/platform/network/mac/CookieJarMac.mm:-151
> -    }];

Was this synchronous behavior important to the logic? Is changing to 'cookiesForURL' an issue?
Comment 23 Daniel Bates 2018-04-18 20:56:03 PDT
Comment on attachment 338197 [details]
Patch and layout tests

View in context: https://bugs.webkit.org/attachment.cgi?id=338197&action=review

>> Source/WebCore/loader/CookieJar.cpp:64
>> +    return SameSiteInfo::create(document.loader()->request());
> 
> If we are inlining, might this be better?
> 
> if (auto loader = document.loader())
>     return SameSiteInfo::create(loader->request());
> 
> return { }]

Will change code before landing to match your suggestion excluding the empty line above "return { }" as I do not feel this improves the readability of the code, adding a * after auto for clarity, and substituting a ';' for ']':

if (auto* loader = document.loader())
    return SameSiteInfo::create(loader->request());
return { };

>> Source/WebCore/platform/network/mac/CookieJarMac.mm:-151
>> -    }];
> 
> Was this synchronous behavior important to the logic? Is changing to 'cookiesForURL' an issue?

There is no behavior change. Notice that cookiesForURL is overloaded and I moved this code to cookiesForURL(NSHTTPCookieStorage *, NSURL *, NSURL *, const std::optional<SameSiteInfo>&, NSString *partition). Maybe your remark indicates confusion with having two cookiesForURL()s? I am open to naming suggestions (or other ideas) to improve the clarity of the code and remove the need to overload cookiesForURL().
Comment 24 Daniel Bates 2018-04-18 20:57:15 PDT
(In reply to Brent Fulgham from comment #22)
> Comment on attachment 338197 [details]
> Patch and layout tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338197&action=review
> 
> [...] Is it safe to land for internal machines?
> 

Builds and runs on my Apple Internal machine :)
Comment 25 Daniel Bates 2018-04-23 13:58:47 PDT
Committed r230921: <https://trac.webkit.org/changeset/230921>
Comment 26 Ryan Haddad 2018-04-23 14:05:01 PDT
This change broke 32-bit macOS builds:

./platform/network/mac/CookieJarMac.mm:101:21: error: use of undeclared identifier '_kCFHTTPCookiePolicyPropertySiteForCookies'
./platform/network/mac/CookieJarMac.mm:102:21: error: use of undeclared identifier '_kCFHTTPCookiePolicyPropertyIsTopLevelNavigation'

https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20%2832-bit%20Build%29/builds/5399
Comment 27 Ryan Haddad 2018-04-23 14:11:10 PDT
(In reply to Ryan Haddad from comment #26)
> This change broke 32-bit macOS builds:

Correction: It broke all the builds.
Comment 28 Daniel Bates 2018-04-23 14:22:29 PDT
(In reply to Ryan Haddad from comment #27)
> (In reply to Ryan Haddad from comment #26)
> > This change broke 32-bit macOS builds:
> 
> Correction: It broke all the builds.

Committed build fixes in <https://trac.webkit.org/changeset/230923> and <https://trac.webkit.org/changeset/230924>
Comment 29 WebKit Commit Bot 2018-04-23 17:33:17 PDT
Re-opened since this is blocked by bug 184903
Comment 31 Daniel Bates 2018-04-24 00:37:15 PDT
Committed r230944: <https://trac.webkit.org/changeset/230944>
Comment 32 Daniel Bates 2018-04-24 00:48:15 PDT
Committed ChangeLog fix in <https://trac.webkit.org/changeset/230945>. Committed build fix in <https://trac.webkit.org/changeset/230946>
Comment 33 Daniel Bates 2018-04-24 00:52:08 PDT
Another attempt to fix the build committed in <https://trac.webkit.org/changeset/230947>.
Comment 34 Daniel Bates 2018-04-24 01:03:06 PDT
Another attempt to fix the build committed in <https://trac.webkit.org/changeset/230948>.
Comment 35 Fujii Hironori 2018-04-24 20:28:53 PDT
Committed r230981: <https://trac.webkit.org/changeset/230981>