Bug 159464 – Implement Same-Site cookies

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Mike West]

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.

[Mike West]

John, Brent: I know cookies are outside your purview, but perhaps you could triage this to the right folks?

[John Wilander]

rdar://problem/27196358

[Mike West]

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.

[Daniel Bates]

(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.

[Daniel Bates]

(In reply to John Wilander from comment #2)
> rdar://problem/27196358

This was duped to...

[Daniel Bates]

<rdar://problem/27196358>

[Daniel Bates]

Created attachment 338163 [details]
Patch and layout tests

[Daniel Bates]

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());

[EWS Watchlist]

Attachment 338163 [details] did not pass style-queue:


ERROR: LayoutTests/TestExpectations:776:  Path does not exist.  [test/expectations] [5]
ERROR: Source/WebCore/platform/network/mac/CookieJarMac.mm:73:  More than one command on the same line  [whitespace/newline] [4]
ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/TestExpectations:776:  Path does not exist.  [test/expectations] [5]
Total errors found: 3 in 105 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Created attachment 338166 [details]
Patch and layout tests

[EWS Watchlist]

Attachment 338166 [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 105 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Created attachment 338174 [details]
Patch and layout tests

[EWS Watchlist]

Attachment 338174 [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 107 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Created attachment 338175 [details]
Patch and layout tests

[EWS Watchlist]

Attachment 338175 [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 107 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Created attachment 338191 [details]
Patch and layout tests

[EWS Watchlist]

Attachment 338191 [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.

[Daniel Bates]

Created attachment 338197 [details]
Patch and layout tests

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Brent Fulgham]

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?

[Daniel Bates]

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().

[Daniel Bates]

(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 :)

[Daniel Bates]

Committed r230921: <https://trac.webkit.org/changeset/230921>

[Ryan Haddad]

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

[Ryan Haddad]

(In reply to Ryan Haddad from comment #26)
> This change broke 32-bit macOS builds:

Correction: It broke all the builds.

[Daniel Bates]

(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>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 184903

[Ryan Haddad]

Here were the remaining failures after the last build fix attempt:

https://build.webkit.org/builders/Apple%20Sierra%20Release%20%2832-bit%20Build%29/builds/10535

https://build.webkit.org/builders/Apple%20Win%20Release%20(Build)/builds/8989
https://build.webkit.org/builders/Apple%20Win%20Debug%20(Build)/builds/8584

[Daniel Bates]

Committed r230944: <https://trac.webkit.org/changeset/230944>

[Daniel Bates]

Committed ChangeLog fix in <https://trac.webkit.org/changeset/230945>. Committed build fix in <https://trac.webkit.org/changeset/230946>

[Daniel Bates]

Another attempt to fix the build committed in <https://trac.webkit.org/changeset/230947>.

[Daniel Bates]

Another attempt to fix the build committed in <https://trac.webkit.org/changeset/230948>.

[Fujii Hironori]

Committed r230981: <https://trac.webkit.org/changeset/230981>