Bug 175232

Summary: Resource Load Statistics: Immediately deny third-party cookie creation for prevalent resources without interaction (rather than temporarily accepting the cookie)
Product: WebKit Reporter: tstapleton
Component: WebCore Misc.Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED CONFIGURATION CHANGED    
Severity: Normal CC: achristensen, aestes, bfulgham, buildbot, cdumez, dbates, mkwst, rniwa, w0nka, webkit-bug-importer, wilander
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=177394
Bug Depends on: 176386, 177394    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
none
Patch
none
Patch
none
Patch
none
Patch achristensen: review+

Description tstapleton 2017-08-04 17:23:59 PDT
In the previous default “one-in-all-in” third party cookie blocking behavior, a website could somewhat deterministically detect if the browser was rejecting third party cookies by attempting to set a test cookie. If the test cookie was not returned on subsequent requests to the server, the website would have a signal to not attempt the setting of additional unnecessary cookies in the browser.

Because the current implementation of ITP purges cookies following a 5 second delay, the test cookie will often be sent on successive requests to the server. This is taken as a signal that the browser will accept cookies resulting in the allocation of storage and processing resources toward the creation of a more substantial cookie that subsequently goes unused. The churn created by this interaction will be non-trivial in terms of both server-side processing and storage.

For a domain that has no previous cookie in its jar, it seems more appropriate to apply the previous behavior of blocking the cookie from being set.
Comment 1 Jack Wellborn 2017-08-11 15:04:03 PDT
Seconding this concern/recommendation. From the perspective of using a 3rd party cookie  for cross-site analytics, the previous behavior is predictable where as ITP is not. Worse it risks of data corruption. Because ITP allows a cookie to get set by a third party on iOS/Safari 11 and because there is way of knowing whether the cookie being set will truly be persistent or partitioned/shortened via ITP, many solutions will simply treat the ITP cookie as any other 3rd party cookie and start seeing inflation of uniques, etc... This leaves the only solution to do browser check for Safari and not drop a cookie, which is no better than if Safari still just blocked the third party cookie.
Comment 2 Brent Fulgham 2017-08-18 15:14:32 PDT
<rdar://problem/33709386>
Comment 3 Brent Fulgham 2017-08-31 13:51:18 PDT
Created attachment 319517 [details]
Patch
Comment 4 Build Bot 2017-08-31 13:54:22 PDT
Attachment 319517 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:133:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:148:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 John Wilander 2017-08-31 13:56:53 PDT
Patch looks as if it implements what we discussed. We need a dedicated test case though. For both cases; with and without user interaction.
Comment 6 Brent Fulgham 2017-09-01 14:55:15 PDT
Created attachment 319647 [details]
Patch
Comment 7 Build Bot 2017-09-01 14:56:54 PDT
Attachment 319647 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:135:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:148:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Build Bot 2017-09-01 15:52:45 PDT
Comment on attachment 319647 [details]
Patch

Attachment 319647 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4429066

New failing tests:
http/tests/loading/resourceLoadStatistics/third-party-cookie-with-and-without-user-interaction.html
Comment 9 Build Bot 2017-09-01 15:52:47 PDT
Created attachment 319658 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 10 Brent Fulgham 2017-09-01 16:41:17 PDT
Created attachment 319668 [details]
Patch
Comment 11 Build Bot 2017-09-01 16:44:07 PDT
Attachment 319668 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:135:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:148:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Brent Fulgham 2017-09-01 16:53:26 PDT
(In reply to Build Bot from comment #11)
> Attachment 319668 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:63:  Extra
> space before ( in function call  [whitespace/parens] [4]
> ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:135:  Extra
> space before ( in function call  [whitespace/parens] [4]
> ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:148: 
> Extra space before ( in function call  [whitespace/parens] [4]
> Total errors found: 3 in 19 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

Whoops. Test isn't compatible with stuff prior to High Sierra.
Comment 13 Alex Christensen 2017-09-05 10:54:41 PDT
Comment on attachment 319668 [details]
Patch

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

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:248
> +    if (!domainsWithInteraction.isEmpty()) {

These checks don't do anything.

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:250
> +            m_prevalentTopPrivatelyControlledDomainsWithoutInteraction.remove(domain);

If we clear first, there's a lot of unnecessary removing from empty sets.

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.h:82
>      RetainPtr<NSURLSession> m_sessionWithoutCredentialStorage;
>      RetainPtr<WKNetworkSessionDelegate> m_sessionWithoutCredentialStorageDelegate;
> +    RetainPtr<NSURLSession> m_sessionWithoutCredentialOrCookieStorage;
> +    RetainPtr<WKNetworkSessionDelegate> m_sessionWithoutCredentialOrCookieStorageDelegate;

Instead of making a third session, we should probably just make the m_sessionWithoutCredentialStorage have a nil HTTPCookieStorage.  The intent of that session is that it's stateless.  Maybe that should be a separate patch because it will affect more than just this.
Comment 14 Brent Fulgham 2017-09-05 11:24:23 PDT
Comment on attachment 319668 [details]
Patch

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

Thank you for reviewing this. Updated patch coming soon.

>> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:248
>> +    if (!domainsWithInteraction.isEmpty()) {
> 
> These checks don't do anything.

I'll get rid of them.

>> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:250
>> +            m_prevalentTopPrivatelyControlledDomainsWithoutInteraction.remove(domain);
> 
> If we clear first, there's a lot of unnecessary removing from empty sets.

Good point. I'll check before removing.

>> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.h:82
>> +    RetainPtr<WKNetworkSessionDelegate> m_sessionWithoutCredentialOrCookieStorageDelegate;
> 
> Instead of making a third session, we should probably just make the m_sessionWithoutCredentialStorage have a nil HTTPCookieStorage.  The intent of that session is that it's stateless.  Maybe that should be a separate patch because it will affect more than just this.

Ok. I filed Bug 176386 to do this in a follow-up.
Comment 15 Brent Fulgham 2017-09-05 11:59:27 PDT
Created attachment 319920 [details]
Patch
Comment 16 Build Bot 2017-09-05 12:02:04 PDT
Attachment 319920 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:135:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:148:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Alex Christensen 2017-09-05 12:04:17 PDT
Comment on attachment 319920 [details]
Patch

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

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.h:82
> +    RetainPtr<NSURLSession> m_sessionWithoutCredentialOrCookieStorage;
> +    RetainPtr<WKNetworkSessionDelegate> m_sessionWithoutCredentialOrCookieStorageDelegate;

To be more clear, I think we should not add a new NSURLSession per NetworkSession just for this.  I think we should make the m_sessionWithoutCredentialStorage not store cookies either instead.  I think we should do that in a separate patch, and I think this patch depends on that patch.
Comment 18 Brent Fulgham 2017-09-05 13:46:54 PDT
Rebaselining the patch to build on top of Bug 176386.
Comment 19 Brent Fulgham 2017-09-05 14:56:12 PDT
Created attachment 319939 [details]
Patch
Comment 20 Build Bot 2017-09-05 14:58:56 PDT
Attachment 319939 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:135:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:148:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 Alex Christensen 2017-09-05 15:49:13 PDT
This patch really blurs the concepts of blocking cookies and a stateless request with no credentials also.  If we are going to block cookies, do we want to also block credentials such as basic authentication credentials?
Comment 22 Brent Fulgham 2017-09-05 16:09:33 PDT
(In reply to Alex Christensen from comment #21)
> This patch really blurs the concepts of blocking cookies and a stateless
> request with no credentials also.  If we are going to block cookies, do we
> want to also block credentials such as basic authentication credentials?

I think so. We hit this case when we've decided that the request does not meet or criteria for an origin that we trust to store cookies or other data. I think local certificates fall under this category, too.
Comment 23 Brent Fulgham 2017-09-05 17:14:52 PDT
I talked to Alex in person, and he suggested we get rid of the boolean and piggy-back off of the "no credentials" storage case.
Comment 24 Brent Fulgham 2017-09-06 09:09:58 PDT
Created attachment 320027 [details]
Patch
Comment 25 Build Bot 2017-09-06 09:11:49 PDT
Attachment 320027 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:135:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:148:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 26 Alex Christensen 2017-09-06 10:22:50 PDT
Comment on attachment 320027 [details]
Patch

Much more elegant.  R=me
Comment 27 Brent Fulgham 2017-09-06 10:30:52 PDT
Committed r221683: <http://trac.webkit.org/changeset/221683>
Comment 28 Brent Fulgham 2017-10-17 17:42:04 PDT
Reopening. We discovered some compatibility problems with this change, and rolled out the immediate cookie blocking here:

https://trac.webkit.org/changeset/222414/webkit

We need to address this in a different way to support the ability to block initially, then unblock when entering a domain that should have cookie access.
Comment 29 John Wilander 2018-02-05 09:47:15 PST
This was eventually fixed inhttps://bugs.webkit.org/show_bug.cgi?id=177394.
Comment 30 Chris Dumez 2018-08-08 13:53:50 PDT
Comment on attachment 320027 [details]
Patch

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

> Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:123
> +        LOG(NetworkSession, "%llu Creating stateless NetworkDataTask with URL %s", [m_task taskIdentifier], nsRequest.URL.absoluteString.UTF8String);

This logging is wrong.

> Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:128
> +        LOG(NetworkSession, "%llu Creating NetworkDataTask with URL %s", [m_task taskIdentifier], nsRequest.URL.absoluteString.UTF8String);

Seems reversed with this one.