Bug 206108

Summary: document.cookie should not do a sync IPC to the network process for iframes that do not have storage access
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, berto, cgarcia, commit-queue, dbates, ews-watchlist, galpeter, ggaren, gns, japhet, webkit-bug-importer, wilander, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 206442    
Attachments:
Description Flags
WIP Patch
none
WIP Patch
none
WIP Patch
none
WIP Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2020-01-10 16:26:55 PST
document.cookie should not do a sync IPC to the network process for iframes that do not have storage access.
Comment 1 Chris Dumez 2020-01-10 16:55:01 PST
Created attachment 387397 [details]
WIP Patch
Comment 2 Chris Dumez 2020-01-13 08:56:13 PST
Created attachment 387535 [details]
WIP Patch
Comment 3 Chris Dumez 2020-01-13 14:34:01 PST
Created attachment 387567 [details]
WIP Patch
Comment 4 Chris Dumez 2020-01-13 16:57:23 PST
Created attachment 387593 [details]
WIP Patch

Almost ready.
Comment 5 Chris Dumez 2020-01-14 11:50:59 PST
Created attachment 387681 [details]
Patch
Comment 6 Chris Dumez 2020-01-14 12:03:19 PST
Created attachment 387684 [details]
Patch
Comment 7 Geoffrey Garen 2020-01-14 13:55:48 PST
Comment on attachment 387684 [details]
Patch

r=me
Comment 8 John Wilander 2020-01-14 15:34:56 PST
Comment on attachment 387684 [details]
Patch

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

Thanks for fixing the preferences read and the improvements to when blocking is done and to test async behavior. See inline comments.

> Source/WebKit/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

I thought we always described the change in both the WebCore and the WebKit change logs, at least when there are significant changes in both.

> Source/WebKit/WebProcess/WebPage/WebCookieJar.cpp:65
> +        return false;

Does this match some existing behavior?

> Source/WebKit/WebProcess/WebPage/WebCookieJar.cpp:69
> +        return false;

What does this imply? Can there be cookies for empty resource domains?

> Source/WebKit/WebProcess/WebPage/WebCookieJar.cpp:108
> +    if (!WebProcess::singleton().ensureNetworkProcessConnection().connection().sendSync(Messages::NetworkConnectionToWebProcess::CookiesForDOM(document.firstPartyForCookies(), sameSiteInfo(document), url, frameID, pageID, shouldIncludeSecureCookies(document, url), delegatesCookieBlockingToNetworkProcess ? ShouldApplyITPCookieBlockingPolicy::Yes : ShouldApplyITPCookieBlockingPolicy::No), Messages::NetworkConnectionToWebProcess::CookiesForDOM::Reply(cookieString, secureCookiesAccessed), 0))

Now that I see the use of ShouldApplyITPCookieBlockingPolicy, I think a more detailed enum is called for, such as { IsFirstParty, IsThirdPartyWithStorageAccess, IsThirdPartyWithoutStorageAccess, IsMissingPartyContext }, the last value being my attempt to encode the two cases with empty domains as commented on above. Then ITP in the network process can make an informed decision instead of being served a decision Apply policy Yes or No.

Such an enum would also make sense in both places instead of this delegatesCookieBlockingToNetworkProcess boolean plus the Yes/No enum.
Comment 9 Chris Dumez 2020-01-14 15:38:46 PST
Comment on attachment 387684 [details]
Patch

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

>> Source/WebKit/ChangeLog:6
>> +        Reviewed by NOBODY (OOPS!).
> 
> I thought we always described the change in both the WebCore and the WebKit change logs, at least when there are significant changes in both.

Not sure what the policy is. I can add the changelog to both WebKit and WebCore.

>> Source/WebKit/WebProcess/WebPage/WebCookieJar.cpp:65
>> +        return false;
> 
> Does this match some existing behavior?

This logic is identical to what is used in NetworkStorageSession::shouldBlockCookies() on network process side.

>> Source/WebKit/WebProcess/WebPage/WebCookieJar.cpp:69
>> +        return false;
> 
> What does this imply? Can there be cookies for empty resource domains?

This logic is identical to what is used in NetworkStorageSession::shouldBlockCookies() on network process side.

>> Source/WebKit/WebProcess/WebPage/WebCookieJar.cpp:108
>> +    if (!WebProcess::singleton().ensureNetworkProcessConnection().connection().sendSync(Messages::NetworkConnectionToWebProcess::CookiesForDOM(document.firstPartyForCookies(), sameSiteInfo(document), url, frameID, pageID, shouldIncludeSecureCookies(document, url), delegatesCookieBlockingToNetworkProcess ? ShouldApplyITPCookieBlockingPolicy::Yes : ShouldApplyITPCookieBlockingPolicy::No), Messages::NetworkConnectionToWebProcess::CookiesForDOM::Reply(cookieString, secureCookiesAccessed), 0))
> 
> Now that I see the use of ShouldApplyITPCookieBlockingPolicy, I think a more detailed enum is called for, such as { IsFirstParty, IsThirdPartyWithStorageAccess, IsThirdPartyWithoutStorageAccess, IsMissingPartyContext }, the last value being my attempt to encode the two cases with empty domains as commented on above. Then ITP in the network process can make an informed decision instead of being served a decision Apply policy Yes or No.
> 
> Such an enum would also make sense in both places instead of this delegatesCookieBlockingToNetworkProcess boolean plus the Yes/No enum.

The logic for checking is the same on the network process and WebProcess side. A yes/no bit is thus enough to communicate to the network process that we already did the check.
Comment 10 Chris Dumez 2020-01-14 16:42:47 PST
Created attachment 387730 [details]
Patch
Comment 11 John Wilander 2020-01-14 17:22:15 PST
Comment on attachment 387730 [details]
Patch

The shouldAskITPInNetworkProcess boolean and the ShouldAskITP enum are both good improvements. I assume you are waiting for EWS before you add cq+. Regardless, I'm OK for it to go on the queue.
Comment 12 WebKit Commit Bot 2020-01-14 19:10:21 PST
The commit-queue encountered the following flaky tests while processing attachment 387730 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org)
The commit-queue is continuing to process your patch.
Comment 13 WebKit Commit Bot 2020-01-14 19:11:01 PST
Comment on attachment 387730 [details]
Patch

Clearing flags on attachment: 387730

Committed r254556: <https://trac.webkit.org/changeset/254556>
Comment 14 WebKit Commit Bot 2020-01-14 19:11:03 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2020-01-14 19:12:15 PST
<rdar://problem/58592851>