Add SPI to unblock third party cookies from WKWebViews with ResourceLoadStatistics turned on
Created attachment 399692 [details] Patch
<rdar://problem/60595539>
Created attachment 399705 [details] Patch
Created attachment 399790 [details] Patch
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 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.
Created attachment 399850 [details] Patch
Created attachment 399868 [details] Patch
Created attachment 399873 [details] Patch
Created attachment 399877 [details] Patch
Created attachment 399901 [details] Patch
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 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.
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.
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.
(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.
Created attachment 399983 [details] Patch
(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 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.
(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.
Created attachment 399989 [details] Patch
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.
Created attachment 399994 [details] Patch
Committed r262066: <https://trac.webkit.org/changeset/262066> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399994 [details].