Bug 212058 – Add SPI to unblock third party cookies from WKWebViews with ResourceLoadStatistics turned on

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Add SPI to unblock third party cookies from WKWebViews with ResourceLoadStatistics turned on

[Alex Christensen]

Created attachment 399692 [details]
Patch

[Alex Christensen]

<rdar://problem/60595539>

[Alex Christensen]

Created attachment 399705 [details]
Patch

[Alex Christensen]

Created attachment 399790 [details]
Patch

[Alex Christensen]

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.

[John Wilander]

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.

[Alex Christensen]

Created attachment 399850 [details]
Patch

[Alex Christensen]

Created attachment 399868 [details]
Patch

[Alex Christensen]

Created attachment 399873 [details]
Patch

[Alex Christensen]

Created attachment 399877 [details]
Patch

[Alex Christensen]

Created attachment 399901 [details]
Patch

[John Wilander]

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

[Chris Dumez]

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.

[John Wilander]

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.

[Alex Christensen]

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.

[John Wilander]

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

[Alex Christensen]

Created attachment 399983 [details]
Patch

[Alex Christensen]

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

[John Wilander]

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.

[Alex Christensen]

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

[Alex Christensen]

Created attachment 399989 [details]
Patch

[John Wilander]

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.

[Alex Christensen]

Created attachment 399994 [details]
Patch

[EWS]

Committed r262066: <https://trac.webkit.org/changeset/262066>

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