Bug 212058 - Add SPI to unblock third party cookies from WKWebViews with ResourceLoadStatistics turned on
Summary: Add SPI to unblock third party cookies from WKWebViews with ResourceLoadStati...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-18 18:20 PDT by Alex Christensen
Modified: 2020-05-22 11:27 PDT (History)
13 users (show)

See Also:


Attachments
Patch (39.07 KB, patch)
2020-05-18 18:25 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (39.54 KB, patch)
2020-05-18 20:26 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (41.19 KB, patch)
2020-05-19 17:40 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (97.20 KB, patch)
2020-05-20 10:34 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (99.31 KB, patch)
2020-05-20 11:59 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (99.66 KB, patch)
2020-05-20 12:32 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (100.61 KB, patch)
2020-05-20 12:51 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (101.79 KB, patch)
2020-05-20 15:38 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (102.16 KB, patch)
2020-05-21 15:17 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (104.77 KB, patch)
2020-05-21 15:39 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (105.30 KB, patch)
2020-05-21 16:14 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2020-05-18 18:20:00 PDT
Add SPI to unblock third party cookies from WKWebViews with ResourceLoadStatistics turned on
Comment 1 Alex Christensen 2020-05-18 18:25:07 PDT
Created attachment 399692 [details]
Patch
Comment 2 Alex Christensen 2020-05-18 18:25:17 PDT
<rdar://problem/60595539>
Comment 3 Alex Christensen 2020-05-18 20:26:51 PDT
Created attachment 399705 [details]
Patch
Comment 4 Alex Christensen 2020-05-19 17:40:32 PDT
Created attachment 399790 [details]
Patch
Comment 5 Alex Christensen 2020-05-19 17:58:39 PDT
Comment on attachment 399790 [details]
Patch

Based on John's suggestion, I'm going to rename this and make it override the ThirdPartyCookieBlockingMode for a WKWebView.
Comment 6 John Wilander 2020-05-19 18:02:29 PDT
Comment on attachment 399790 [details]
Patch

This should make use of the existing enum WebCore::ThirdPartyCookieBlockingMode, probably with a new value None, and possibly also make use of the existing SPI infrastructure hooked up to setThirdPartyCookieBlockingMode() in various classes.
Comment 7 Alex Christensen 2020-05-20 10:34:20 PDT
Created attachment 399850 [details]
Patch
Comment 8 Alex Christensen 2020-05-20 11:59:34 PDT
Created attachment 399868 [details]
Patch
Comment 9 Alex Christensen 2020-05-20 12:32:20 PDT
Created attachment 399873 [details]
Patch
Comment 10 Alex Christensen 2020-05-20 12:51:47 PDT
Created attachment 399877 [details]
Patch
Comment 11 Alex Christensen 2020-05-20 15:38:13 PDT
Created attachment 399901 [details]
Patch
Comment 12 John Wilander 2020-05-21 13:34:06 PDT
Comment on attachment 399901 [details]
Patch

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

The logic of this looks sane. Please consider a boolean enum for self documentation purposes. I'd like Chris to have a look at the cookie cache part since that's code he wrote fairly recently.

> Source/WebCore/ChangeLog:11
> +        (WebCore::m_loadsFromNetwork): Deleted.

I think these are changes in the header file and not in the implementation file. Also, the deletion of m_loadsFromNetwork seems wrong.

> Source/WebCore/loader/CookieJar.cpp:45
> +static bool relaxThirdPartyCookieBlocking(const Document& document)

I think this function too should be called shouldRelaxThirdPartyCookieBlocking() since it returns a boolean.

> Source/WebCore/page/Page.h:1049
> +    bool m_relaxThirdPartyCookieBlocking { false };

I think this one should be called m_shouldRelaxThirdPartyCookieBlocking.

> Source/WebCore/page/PageConfiguration.h:134
> +    bool relaxThirdPartyCookieBlocking { false };

shouldRelaxThirdPartyCookieBlocking

> Source/WebCore/platform/network/CacheValidation.cpp:343
> +    return session.cookieRequestHeaderFieldValue(request.firstPartyForCookies(), SameSiteInfo::create(request), request.url(), WTF::nullopt, WTF::nullopt, request.url().protocolIs("https") ? IncludeSecureCookies::Yes : IncludeSecureCookies::No, ShouldAskITP::Yes, false).first;

It's unfortunate that we get a boolean parameter here. Did you consider a boolean enum to make it more self documenting?

> Source/WebCore/platform/network/NetworkStorageSession.h:153
> +    WEBCORE_EXPORT void setCookiesFromDOM(const URL& firstParty, const SameSiteInfo&, const URL&, Optional<FrameIdentifier>, Optional<PageIdentifier>, ShouldAskITP, const String&, bool relaxThirdPartyCookieBlocking) const;

I think it should be called shouldRelaxThirdPartyCookieBlocking.

If we made this boolean (ideally boolean enum) parameter have a default false value, we wouldn't have to send in 'false' in all call sites. However, I do see you had to make ShouldAskITP mandatory which is an example of why such params never last.

> Source/WebKit/NetworkProcess/NetworkLoadParameters.h:62
> +    bool relaxThirdPartyCookieBlocking { false };

shouldRelaxThirdPartyCookieBlocking

> Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.h:107
> +    bool m_relaxThirdPartyCookieBlocking { false };

m_shouldRelaxThirdPartyCookieBlocking

> Source/WebKit/Shared/WebPageCreationParameters.h:223
> +    bool relaxThirdPartyCookieBlocking { false };

shouldRelaxThirdPartyCookieBlocking

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:1257
> +- (BOOL)_relaxThirdPartyCookieBlocking

_shouldRelaxThirdPartyCookieBlocking

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfigurationPrivate.h:129
> +@property (nonatomic, setter=_setRelaxThirdPartyCookieBlocking:) BOOL _relaxThirdPartyCookieBlocking WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

I worry that this SPI will be abused. :(
Comment 13 Chris Dumez 2020-05-21 13:38:22 PDT
Comment on attachment 399901 [details]
Patch

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

>> Source/WebCore/loader/CookieJar.cpp:45
>> +static bool relaxThirdPartyCookieBlocking(const Document& document)
> 
> I think this function too should be called shouldRelaxThirdPartyCookieBlocking() since it returns a boolean.

+1

> Source/WebCore/loader/CookieJar.cpp:142
> +        std::pair<String, bool> result = session->cookieRequestHeaderFieldValue(firstParty, sameSiteInfo, url, frameID, pageID, includeSecureCookies, ShouldAskITP::Yes, false);

That's why we use enum classes instead of boolean for parameters. This looks very obscure passing false here.

>> Source/WebCore/page/Page.h:1049
>> +    bool m_relaxThirdPartyCookieBlocking { false };
> 
> I think this one should be called m_shouldRelaxThirdPartyCookieBlocking.

+1

> Source/WebCore/platform/network/NetworkStorageSession.cpp:114
> +    if (relaxThirdPartyCookieBlocking)

Seems weird to pass in a new relaxThirdPartyCookieBlocking just to return early if it is true. This makes me think that we should not add a parameter and that call sites should check if they should relax policy.
Comment 14 John Wilander 2020-05-21 14:00:55 PDT
Going back to my first comments, did you try using the existing infrastructure for cookie blocking with WebCore::ThirdPartyCookieBlockingMode? It seems just setting the blocking mode would be simpler, introduce fewer function signature changes, and be less prone to breakage down the road.
Comment 15 Alex Christensen 2020-05-21 14:13:01 PDT
ThirdPartyCookieBlockingMode cannot be used for this because it is per-session.  We need this to be only for one WKWebView's loads, with other WKWebViews in the same session having a different value.  I could've done it with an overrideThirdPartyCookieBlockingMode, but that is basically what I did with a bool.  I'm going to make the bool a 2-state enum.
Comment 16 John Wilander 2020-05-21 14:14:57 PDT
(In reply to Alex Christensen from comment #15)
> ThirdPartyCookieBlockingMode cannot be used for this because it is
> per-session.  We need this to be only for one WKWebView's loads, with other
> WKWebViews in the same session having a different value.  I could've done it
> with an overrideThirdPartyCookieBlockingMode, but that is basically what I
> did with a bool.  I'm going to make the bool a 2-state enum.

Thanks. I think you already told me about the per-view vs per-session issue elsewhere.
Comment 17 Alex Christensen 2020-05-21 15:17:52 PDT
Created attachment 399983 [details]
Patch
Comment 18 Alex Christensen 2020-05-21 15:19:42 PDT
(In reply to Chris Dumez from comment #13)
> Seems weird to pass in a new relaxThirdPartyCookieBlocking just to return
> early if it is true. This makes me think that we should not add a parameter
> and that call sites should check if they should relax policy.
We could indeed do this at all callsites, but making it a parameter makes us consider if we can find a Page to ask for this for future call sites.  Same technique we use when we add an unused LockHolder& parameter.
Comment 19 John Wilander 2020-05-21 15:28:45 PDT
Comment on attachment 399983 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        (WebCore::m_loadsFromNetwork): Deleted.

I still think this is wrong.

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNetWin.cpp:200
> +void NetworkStorageSession::setCookiesFromDOM(const URL& firstParty, const SameSiteInfo&, const URL& url, Optional<FrameIdentifier> frameID, Optional<PageIdentifier> pageID, ShouldAskITP, const String& value, bool) const

Shouldn't this also use the new enum?

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNetWin.cpp:236
> +std::pair<String, bool> NetworkStorageSession::cookiesForDOM(const URL& firstParty, const SameSiteInfo&, const URL& url, Optional<FrameIdentifier> frameID, Optional<PageIdentifier> pageID, IncludeSecureCookies includeSecureCookies, ShouldAskITP, bool) const

Ditto.

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNetWin.cpp:251
> +std::pair<String, bool> NetworkStorageSession::cookieRequestHeaderFieldValue(const URL& firstParty, const SameSiteInfo&, const URL& url, Optional<FrameIdentifier> frameID, Optional<PageIdentifier> pageID, IncludeSecureCookies includeSecureCookies, ShouldAskITP, bool) const

Ditto.

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNetWin.cpp:283
> +bool NetworkStorageSession::getRawCookies(const URL& firstParty, const SameSiteInfo&, const URL& url, Optional<FrameIdentifier> frameID, Optional<PageIdentifier> pageID, ShouldAskITP, bool, Vector<Cookie>& rawCookies) const

Ditto.

> Source/WebCore/platform/network/curl/NetworkStorageSessionCurl.cpp:98
> +void NetworkStorageSession::setCookiesFromDOM(const URL& firstParty, const SameSiteInfo&, const URL& url, Optional<FrameIdentifier>, Optional<PageIdentifier>, ShouldAskITP, const String& value, bool) const

Ditto.

> Source/WebCore/platform/network/curl/NetworkStorageSessionCurl.cpp:130
> +std::pair<String, bool> NetworkStorageSession::cookiesForDOM(const URL& firstParty, const SameSiteInfo&, const URL& url, Optional<FrameIdentifier>, Optional<PageIdentifier>, IncludeSecureCookies, ShouldAskITP, bool) const

Ditto.

> Source/WebCore/platform/network/curl/NetworkStorageSessionCurl.cpp:199
> +bool NetworkStorageSession::getRawCookies(const URL& firstParty, const SameSiteInfo&, const URL& url, Optional<FrameIdentifier>, Optional<PageIdentifier>, ShouldAskITP, bool, Vector<Cookie>& rawCookies) const

Ditto.

> Source/WebCore/platform/network/curl/NetworkStorageSessionCurl.cpp:214
> +std::pair<String, bool> NetworkStorageSession::cookieRequestHeaderFieldValue(const URL& firstParty, const SameSiteInfo&, const URL& url, Optional<FrameIdentifier>, Optional<PageIdentifier>, IncludeSecureCookies, ShouldAskITP, bool) const

Ditto.

> Source/WebKit/NetworkProcess/NetworkLoadParameters.h:63
> +    WebCore::ShouldRelaxThirdPartyCookieBlocking shouldRelaxThirdPartyCookieBlocking { false };

Does this work? Seems fragile in what 'false' maps to. Isn't that dependent on the order of the enum values now?

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:1264
> +    bool allowed = WebCore::applicationBundleIdentifier() == "com.apple.WebKit.TestWebKitAPI";

Should this use _s? I don't know if we do that in ObjC files.
Comment 20 Alex Christensen 2020-05-21 15:37:21 PDT
(In reply to John Wilander from comment #19)

> Does this work? Seems fragile in what 'false' maps to. Isn't that dependent
> on the order of the enum values now?
I'm surprised this compiled.  Fixing.

> > Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:1264
> > +    bool allowed = WebCore::applicationBundleIdentifier() == "com.apple.WebKit.TestWebKitAPI";
> 
> Should this use _s? I don't know if we do that in ObjC files.
May as well.  I don't think it matters in this case because there's a operator==(const String& a, const char* b) but it's still good practice.  Will do.

The rest were programmatic find/replaces that failed, and EWS was about to tell me the same fixes you just did.
Comment 21 Alex Christensen 2020-05-21 15:39:37 PDT
Created attachment 399989 [details]
Patch
Comment 22 John Wilander 2020-05-21 15:49:03 PDT
Comment on attachment 399989 [details]
Patch

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

If EWS is happy and you fix the one stray change log thing, I'm happy.

> Source/WebCore/ChangeLog:18
> +        (WebCore::m_loadsFromNetwork): Deleted.

It's probably the change log generator messing this up because the diff shows this member as untouched. Please remove.
Comment 23 Alex Christensen 2020-05-21 16:14:14 PDT
Created attachment 399994 [details]
Patch
Comment 24 EWS 2020-05-22 11:27:18 PDT
Committed r262066: <https://trac.webkit.org/changeset/262066>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399994 [details].