Bug 175232 – Resource Load Statistics: Immediately deny third-party cookie creation for prevalent resources without interaction (rather than temporarily accepting the cookie)

• Component: WebCore Misc.
• Status: RESOLVED CONFIGURATION CHANGED

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.

[Jack Wellborn]

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.

[Brent Fulgham]

<rdar://problem/33709386>

[Brent Fulgham]

Created attachment 319517 [details]
Patch

[Build Bot]

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.

[John Wilander]

Patch looks as if it implements what we discussed. We need a dedicated test case though. For both cases; with and without user interaction.

[Brent Fulgham]

Created attachment 319647 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Brent Fulgham]

Created attachment 319668 [details]
Patch

[Build Bot]

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.

[Brent Fulgham]

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

[Alex Christensen]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 319920 [details]
Patch

[Build Bot]

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.

[Alex Christensen]

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.

[Brent Fulgham]

Rebaselining the patch to build on top of Bug 176386.

[Brent Fulgham]

Created attachment 319939 [details]
Patch

[Build Bot]

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.

[Alex Christensen]

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?

[Brent Fulgham]

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

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 320027 [details]
Patch

[Build Bot]

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.

[Alex Christensen]

Comment on attachment 320027 [details]
Patch

Much more elegant.  R=me

[Brent Fulgham]

Committed r221683: <http://trac.webkit.org/changeset/221683>

[Brent Fulgham]

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.

[John Wilander]

This was eventually fixed inhttps://bugs.webkit.org/show_bug.cgi?id=177394.

[Chris Dumez]

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.