Bug 215201

<rdar://problem/57454633>

Created attachment 406067 [details] Patch

Created attachment 406069 [details] Patch

Attempt to fix the non-Cocoa builds.

Created attachment 406091 [details] Patch

Skipping the tests for iOS 13 after consulting Jonathan Bedard. That should fix the iOS bot issue.

Comment on attachment 406091 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406091&action=review Haven't had time to do a full review yet. Will resume later (after lunch) unless someone beats me to it. > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:314 > + RetainPtr<NSMutableDictionary<NSHTTPCookiePropertyKey, id>> properties = adoptNS([[cookie properties] mutableCopy]); auto > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:315 > + RetainPtr<NSDate> date = adoptNS([[NSDate alloc] initWithTimeIntervalSinceNow:cap.seconds()]); auto > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:140 > +static WebCore::RegistrableDomain lastCNAMEDomain(NSArray<NSString*>* cnames) Missing spaces before the stars. > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:149 > + return WebCore::RegistrableDomain::uncheckedCreateFromHost(emptyString()); Why cannot we return { }; ? > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:157 > + auto&& cnameDomain = lastCNAMEDomain([m_task _resolvedCNAMEChain]); why auto&& and not auto?

(In reply to Chris Dumez from comment #7) > Comment on attachment 406091 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=406091&action=review > > Haven't had time to do a full review yet. Will resume later (after lunch) > unless someone beats me to it. > > > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:314 > > + RetainPtr<NSMutableDictionary<NSHTTPCookiePropertyKey, id>> properties = adoptNS([[cookie properties] mutableCopy]); > > auto Will fix. > > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:315 > > + RetainPtr<NSDate> date = adoptNS([[NSDate alloc] initWithTimeIntervalSinceNow:cap.seconds()]); > > auto Will fix. > > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:140 > > +static WebCore::RegistrableDomain lastCNAMEDomain(NSArray<NSString*>* cnames) > > Missing spaces before the stars. Will fix. > > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:149 > > + return WebCore::RegistrableDomain::uncheckedCreateFromHost(emptyString()); > > Why cannot we return { }; ? Will fix. > > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:157 > > + auto&& cnameDomain = lastCNAMEDomain([m_task _resolvedCNAMEChain]); > > why auto&& and not auto? Will fix. Thanks so far, Chris!

`return cappedCookies` in `applyCookiePolicyForThirdPartyCNAMECloaking` should be autoreleased or we will end up with a +1 retain count. Alternatively you could use the convenience to create the NSMutableArray by calling [NSMutableArray arrayWithCapacity:count]. In that case you won't need to autorelease before returning from the block

Comment on attachment 406091 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406091&action=review > Source/WebKit/ChangeLog:43 > + * NetworkProcess/NetworkProcess.cpp: > + (WebKit::NetworkProcess::resetParametersToDefaultValues): > + (WebKit::NetworkProcess::setIsRunningResourceLoadStatisticsTest): > + (WebKit::NetworkProcess::setFirstPartyHostCNAMEDomainForTesting): > + (WebKit::NetworkProcess::setThirdPartyCNAMEDomainForTesting): Please add more per-function change log information. > Source/WebCore/platform/network/NetworkStorageSession.h:196 > +#if PLATFORM(COCOA) || USE(CFURLCONNECTION) > + WEBCORE_EXPORT static NSHTTPCookie *capExpiryOfPersistentCookie(NSHTTPCookie *, Seconds cap); > +#endif It seems unfortunate to have Cocoa specific types referenced in a platform agnostic file. I think this likely needs more thought on how this should abstracted to support all loader backends, if and when they want to support exposing the cname chain and support mutation of cookies. > Source/WebKit/NetworkProcess/NetworkProcess.cpp:819 > +#if HAVE(CFNETWORK_CNAME_AND_COOKIE_TRANSFORM_SPI) > + networkSession->resetCNAMEDomainData(); > + networkSession->setCNAMECloakingMitigationEnabled(WebCore::CNAMECloakingMitigationEnabled::No); It doesn't look like the implementations of resetCNAMEDomainData() and setCNAMECloakingMitigationEnabled are guarded by the same conditional being used here. > Source/WebKit/NetworkProcess/NetworkProcess.cpp:875 > +#if HAVE(CFNETWORK_CNAME_AND_COOKIE_TRANSFORM_SPI) > + networkSession->setCNAMECloakingMitigationEnabled(WebCore::CNAMECloakingMitigationEnabled::Yes); > +#endif This is seems unexpected. There is nothing ResourceLoadStatistics related about cname cloaking it seems. Why would enabling ResourceLoadStatistics testing enable this. Seems like this should be treated like all other features and enable as needed, not grouped in with another feature. > Source/WebKit/Shared/WebPreferences.yaml:2063 > + humanReadableDescription: "Enable third-party CNAME cloaking mitigation when Intelligent Tracking Prevention is enabled" I don't think including marketing names like Intelligent Tracking Prevention is warranted here. There really doesn't seem to be any correlation. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1968 > +void WebsiteDataStore::setResourceLoadStatisticsThirdPartyCNAMEDomainForTesting(const URL& cnameURL, CompletionHandler<void()>&& completionHandler) > +{ > + if (cnameURL.host() != "testwebkit.org"_s && cnameURL.host() != "3rdpartytestwebkit.org"_s) { > + completionHandler(); > + return; > + } > + > + auto callbackAggregator = CallbackAggregator::create(WTFMove(completionHandler)); > + > + for (auto& processPool : processPools()) { > + if (auto* networkProcess = processPool->networkProcess()) > + networkProcess->setThirdPartyCNAMEDomainForTesting(m_sessionID, WebCore::RegistrableDomain { cnameURL }, [callbackAggregator] { }); > + } > +} This (and a lot of the rest of the patch) seem to be adding a lot of binary code bloat that is only used for testing. Can you explain why this can't be tested without adding all this infrastructure? Is the issue no way to mock out the networking bits needed to return a CNAME chain? > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2317 > #if ENABLE(RESOURCE_LOAD_STATISTICS) > thirdPartyCookieBlockingMode(), > WebCore::SameSiteStrictEnforcementEnabled::No, > + WebCore::CNAMECloakingMitigationEnabled::No, > #endif This doesn't seem like the appropriate conditional. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:207 > + void setResourceLoadStatisticsFirstPartyHostCNAMEDomainForTesting(const URL& firstPartyURL, const URL& cnameURL, CompletionHandler<void()>&&); > + void setResourceLoadStatisticsThirdPartyCNAMEDomainForTesting(const URL&, CompletionHandler<void()>&&); These function names don't make sense to me. There doesn't seem to be anything ResourceLoadStatistics about these.

Comment on attachment 406091 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406091&action=review > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:205 > + return cappedCookies; return [cappedCookies autorelease]; Alternatively use [NSMutableArray arrayWithCapacity:count] in which case autorelease won't be needed

(In reply to jmehta from comment #11) > Comment on attachment 406091 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=406091&action=review > > > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:205 > > + return cappedCookies; > > return [cappedCookies autorelease]; > > Alternatively use [NSMutableArray arrayWithCapacity:count] in which case > autorelease won't be needed Thanks! Are you saying the existing: auto* cappedCookies = [[NSMutableArray alloc] initWithCapacity:count] … should be: auto* cappedCookies = [NSMutableArray arrayWithCapacity:count]; ?

Comment on attachment 406091 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406091&action=review > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1200 > + if (!canSendMessage()) { This check is not needed. > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1210 > + if (!canSendMessage()) { Ditto. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1942 > + if (cnameURL.host() != "testwebkit.org"_s && cnameURL.host() != "3rdpartytestwebkit.org"_s) { No _s for string comparisons. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1950 > + if (auto* networkProcess = processPool->networkProcess()) Shouldn't this ensureNetworkProcess() to make sure the test domain actually does get set? > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1957 > + if (cnameURL.host() != "testwebkit.org"_s && cnameURL.host() != "3rdpartytestwebkit.org"_s) { No _s for string comparisons. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1965 > + if (auto* networkProcess = processPool->networkProcess()) ensureNetworkProcess()

Comment on attachment 406091 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406091&action=review > Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:300 > +@property (nonatomic, copy, nullable) NSArray<NSHTTPCookie*>* (^_cookieTransformCallback)(NSArray<NSHTTPCookie*>* cookies); These should be surrounded by HAVE(CFNETWORK_CNAME_AND_COOKIE_TRANSFORM) > Source/WebCore/platform/network/NetworkStorageSession.h:194 > +#if PLATFORM(COCOA) || USE(CFURLCONNECTION) USE(CFURLCONNECTION) is only true on AppleWin, which doesn't have NSHTTPCookie. > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:136 > + return [task respondsToSelector:@selector(_cookieTransformCallback)] Please add a FIXME saying when we can remove these selector checks.

Hi Sam! (In reply to Sam Weinig from comment #10) > Comment on attachment 406091 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=406091&action=review > > > Source/WebKit/ChangeLog:43 > > + * NetworkProcess/NetworkProcess.cpp: > > + (WebKit::NetworkProcess::resetParametersToDefaultValues): > > + (WebKit::NetworkProcess::setIsRunningResourceLoadStatisticsTest): > > + (WebKit::NetworkProcess::setFirstPartyHostCNAMEDomainForTesting): > > + (WebKit::NetworkProcess::setThirdPartyCNAMEDomainForTesting): > > Please add more per-function change log information. Will do. > > Source/WebCore/platform/network/NetworkStorageSession.h:196 > > +#if PLATFORM(COCOA) || USE(CFURLCONNECTION) > > + WEBCORE_EXPORT static NSHTTPCookie *capExpiryOfPersistentCookie(NSHTTPCookie *, Seconds cap); > > +#endif > > It seems unfortunate to have Cocoa specific types referenced in a platform > agnostic file. I think this likely needs more thought on how this should > abstracted to support all loader backends, if and when they want to support > exposing the cname chain and support mutation of cookies. I agree and felt the same way when I added the compile time check to fix the non-Cocoa builds. There is no NetworkStorageSessionCocoa.h and I wanted to reuse the existing code for capping cookies' expiry which is why I broke it out of parseDOMCookie() in NetworkStorageSessionCocoa.mm. I did however check to see if there were other such instances in NetworkStorageSession.h and there is a block guarded by #if PLATFORM(COCOA) || USE(CFURLCONNECTION). Would moving my declaration to that block resolve the issue for you? > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:819 > > +#if HAVE(CFNETWORK_CNAME_AND_COOKIE_TRANSFORM_SPI) > > + networkSession->resetCNAMEDomainData(); > > + networkSession->setCNAMECloakingMitigationEnabled(WebCore::CNAMECloakingMitigationEnabled::No); > > It doesn't look like the implementations of resetCNAMEDomainData() and > setCNAMECloakingMitigationEnabled are guarded by the same conditional being > used here. I'll add the conditional. > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:875 > > +#if HAVE(CFNETWORK_CNAME_AND_COOKIE_TRANSFORM_SPI) > > + networkSession->setCNAMECloakingMitigationEnabled(WebCore::CNAMECloakingMitigationEnabled::Yes); > > +#endif > > This is seems unexpected. There is nothing ResourceLoadStatistics related > about cname cloaking it seems. Why would enabling ResourceLoadStatistics > testing enable this. Seems like this should be treated like all other > features and enable as needed, not grouped in with another feature. In fact, this patch is very much part of ITP. You can see it in the compile-time flag it's under and that the two functions that implement the functionality: · NetworkDataTaskCocoa::updateFirstPartyInfoForTaskAndSession() · NetworkDataTaskCocoa::applyCookiePolicyForThirdPartyCNAMECloaking() … are both controlled by the newly exposed NetworkStorageSession::resourceLoadStatisticsEnabled(). > > Source/WebKit/Shared/WebPreferences.yaml:2063 > > + humanReadableDescription: "Enable third-party CNAME cloaking mitigation when Intelligent Tracking Prevention is enabled" > > I don't think including marketing names like Intelligent Tracking Prevention > is warranted here. There really doesn't seem to be any correlation. As explained above, this feature is turned on and off with the ITP setting. As you can see in related experimental settings, we do refer to ITP explicitly. This is to help developers understand what the setting is for since the commonly known name is ITP. > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1968 > > +void WebsiteDataStore::setResourceLoadStatisticsThirdPartyCNAMEDomainForTesting(const URL& cnameURL, CompletionHandler<void()>&& completionHandler) > > +{ > > + if (cnameURL.host() != "testwebkit.org"_s && cnameURL.host() != "3rdpartytestwebkit.org"_s) { > > + completionHandler(); > > + return; > > + } > > + > > + auto callbackAggregator = CallbackAggregator::create(WTFMove(completionHandler)); > > + > > + for (auto& processPool : processPools()) { > > + if (auto* networkProcess = processPool->networkProcess()) > > + networkProcess->setThirdPartyCNAMEDomainForTesting(m_sessionID, WebCore::RegistrableDomain { cnameURL }, [callbackAggregator] { }); > > + } > > +} > > This (and a lot of the rest of the patch) seem to be adding a lot of binary > code bloat that is only used for testing. Can you explain why this can't be > tested without adding all this infrastructure? Is the issue no way to mock > out the networking bits needed to return a CNAME chain? Correct. The code you refer to is mocking the DNS resolution data that I have no way of controlling in layout tests. > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2317 > > #if ENABLE(RESOURCE_LOAD_STATISTICS) > > thirdPartyCookieBlockingMode(), > > WebCore::SameSiteStrictEnforcementEnabled::No, > > + WebCore::CNAMECloakingMitigationEnabled::No, > > #endif > > This doesn't seem like the appropriate conditional. As explained above, this code is part of ITP, or RESOURCE_LOAD_STATISTICS. > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:207 > > + void setResourceLoadStatisticsFirstPartyHostCNAMEDomainForTesting(const URL& firstPartyURL, const URL& cnameURL, CompletionHandler<void()>&&); > > + void setResourceLoadStatisticsThirdPartyCNAMEDomainForTesting(const URL&, CompletionHandler<void()>&&); > > These function names don't make sense to me. There doesn't seem to be > anything ResourceLoadStatistics about these. Ditto. Thanks, Sam! Please advise.

(In reply to Chris Dumez from comment #13) > Comment on attachment 406091 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=406091&action=review > > > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1200 > > + if (!canSendMessage()) { > > This check is not needed. Will fix. > > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1210 > > + if (!canSendMessage()) { > > Ditto. Will fix. > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1942 > > + if (cnameURL.host() != "testwebkit.org"_s && cnameURL.host() != "3rdpartytestwebkit.org"_s) { > > No _s for string comparisons. Will fix. > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1950 > > + if (auto* networkProcess = processPool->networkProcess()) > > Shouldn't this ensureNetworkProcess() to make sure the test domain actually > does get set? Didn't think about that. Will fix. > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1957 > > + if (cnameURL.host() != "testwebkit.org"_s && cnameURL.host() != "3rdpartytestwebkit.org"_s) { > > No _s for string comparisons. Will fix. > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1965 > > + if (auto* networkProcess = processPool->networkProcess()) > > ensureNetworkProcess() Will fix. Thanks!

(In reply to Alex Christensen from comment #14) > Comment on attachment 406091 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=406091&action=review > > > Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:300 > > +@property (nonatomic, copy, nullable) NSArray<NSHTTPCookie*>* (^_cookieTransformCallback)(NSArray<NSHTTPCookie*>* cookies); > > These should be surrounded by HAVE(CFNETWORK_CNAME_AND_COOKIE_TRANSFORM) Will fix. > > Source/WebCore/platform/network/NetworkStorageSession.h:194 > > +#if PLATFORM(COCOA) || USE(CFURLCONNECTION) > > USE(CFURLCONNECTION) is only true on AppleWin, which doesn't have > NSHTTPCookie. Aha! That means I cannot move it to the block I mentioned in my reply to Sam. > > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:136 > > + return [task respondsToSelector:@selector(_cookieTransformCallback)] > > Please add a FIXME saying when we can remove these selector checks. Will fix. Thanks!

Created attachment 406113 [details] Patch

Created attachment 406121 [details] Patch

(In reply to John Wilander from comment #15) > > > Source/WebCore/platform/network/NetworkStorageSession.h:196 > > > +#if PLATFORM(COCOA) || USE(CFURLCONNECTION) > > > + WEBCORE_EXPORT static NSHTTPCookie *capExpiryOfPersistentCookie(NSHTTPCookie *, Seconds cap); > > > +#endif > > > > It seems unfortunate to have Cocoa specific types referenced in a platform > > agnostic file. I think this likely needs more thought on how this should > > abstracted to support all loader backends, if and when they want to support > > exposing the cname chain and support mutation of cookies. > > I agree and felt the same way when I added the compile time check to fix the > non-Cocoa builds. There is no NetworkStorageSessionCocoa.h and I wanted to > reuse the existing code for capping cookies' expiry which is why I broke it > out of parseDOMCookie() in NetworkStorageSessionCocoa.mm. > > I did however check to see if there were other such instances in > NetworkStorageSession.h and there is a block guarded by #if PLATFORM(COCOA) > || USE(CFURLCONNECTION). Would moving my declaration to that block resolve > the issue for you? Not really :(. The file is a huge soup of chaos, and really needs to be managed better, but adding more platform specific stuff to it is going in the wrong direction. We have an existing attraction for Cookies, WebCore::Cookie, and if that can't be used for the job, we should figure out why and fix it. > > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:875 > > > +#if HAVE(CFNETWORK_CNAME_AND_COOKIE_TRANSFORM_SPI) > > > + networkSession->setCNAMECloakingMitigationEnabled(WebCore::CNAMECloakingMitigationEnabled::Yes); > > > +#endif > > > > This is seems unexpected. There is nothing ResourceLoadStatistics related > > about cname cloaking it seems. Why would enabling ResourceLoadStatistics > > testing enable this. Seems like this should be treated like all other > > features and enable as needed, not grouped in with another feature. > > In fact, this patch is very much part of ITP. You can see it in the > compile-time flag it's under and that the two functions that implement the > functionality: > · NetworkDataTaskCocoa::updateFirstPartyInfoForTaskAndSession() > · NetworkDataTaskCocoa::applyCookiePolicyForThirdPartyCNAMECloaking() > … are both controlled by the newly exposed > NetworkStorageSession::resourceLoadStatisticsEnabled(). I don't follow. Just because you put it behind the same preference name doesn't mean a lot (unless you are saying something else by this). I think the less we use vague terms like ITP in WebCore and WebKit the better. This seems to be a distinct feature, "CNAME Cloaking Mitigation", that stands on its own. Perhaps I am missing something. What aspects of the other features that fall under the ITP umbrella does this depend on? > > > > Source/WebKit/Shared/WebPreferences.yaml:2063 > > > + humanReadableDescription: "Enable third-party CNAME cloaking mitigation when Intelligent Tracking Prevention is enabled" > > > > I don't think including marketing names like Intelligent Tracking Prevention > > is warranted here. There really doesn't seem to be any correlation. > > As explained above, this feature is turned on and off with the ITP setting. > As you can see in related experimental settings, we do refer to ITP > explicitly. This is to help developers understand what the setting is for > since the commonly known name is ITP. I don't think this helpful. "CNAME cloaking mitigation" on its own describes the feature. There is no need to pad it out. > > > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1968 > > > +void WebsiteDataStore::setResourceLoadStatisticsThirdPartyCNAMEDomainForTesting(const URL& cnameURL, CompletionHandler<void()>&& completionHandler) > > > +{ > > > + if (cnameURL.host() != "testwebkit.org"_s && cnameURL.host() != "3rdpartytestwebkit.org"_s) { > > > + completionHandler(); > > > + return; > > > + } > > > + > > > + auto callbackAggregator = CallbackAggregator::create(WTFMove(completionHandler)); > > > + > > > + for (auto& processPool : processPools()) { > > > + if (auto* networkProcess = processPool->networkProcess()) > > > + networkProcess->setThirdPartyCNAMEDomainForTesting(m_sessionID, WebCore::RegistrableDomain { cnameURL }, [callbackAggregator] { }); > > > + } > > > +} > > > > This (and a lot of the rest of the patch) seem to be adding a lot of binary > > code bloat that is only used for testing. Can you explain why this can't be > > tested without adding all this infrastructure? Is the issue no way to mock > > out the networking bits needed to return a CNAME chain? > > Correct. The code you refer to is mocking the DNS resolution data that I > have no way of controlling in layout tests. I think you should look into how to we can mock DNS resolution below WebKit, otherwise we aren't really exercising the stack very well, and we end up shipping a lot of unnecessary code to customers.

(In reply to Sam Weinig from comment #20) > (In reply to John Wilander from comment #15) > > > > Source/WebCore/platform/network/NetworkStorageSession.h:196 > > > > +#if PLATFORM(COCOA) || USE(CFURLCONNECTION) > > > > + WEBCORE_EXPORT static NSHTTPCookie *capExpiryOfPersistentCookie(NSHTTPCookie *, Seconds cap); > > > > +#endif > > > > > > It seems unfortunate to have Cocoa specific types referenced in a platform > > > agnostic file. I think this likely needs more thought on how this should > > > abstracted to support all loader backends, if and when they want to support > > > exposing the cname chain and support mutation of cookies. > > > > I agree and felt the same way when I added the compile time check to fix the > > non-Cocoa builds. There is no NetworkStorageSessionCocoa.h and I wanted to > > reuse the existing code for capping cookies' expiry which is why I broke it > > out of parseDOMCookie() in NetworkStorageSessionCocoa.mm. > > > > I did however check to see if there were other such instances in > > NetworkStorageSession.h and there is a block guarded by #if PLATFORM(COCOA) > > || USE(CFURLCONNECTION). Would moving my declaration to that block resolve > > the issue for you? > > Not really :(. The file is a huge soup of chaos, and really needs to be > managed better, but adding more platform specific stuff to it is going in > the wrong direction. We have an existing attraction for Cookies, > WebCore::Cookie, and if that can't be used for the job, we should figure out > why and fix it. My alternative for this patch seems to be to copy the code to the Cocoa-specific task and leave the cookie capping code in NetworkStorageSessionCocoa.mm and NetworkStorageSession.h alone. > > > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:875 > > > > +#if HAVE(CFNETWORK_CNAME_AND_COOKIE_TRANSFORM_SPI) > > > > + networkSession->setCNAMECloakingMitigationEnabled(WebCore::CNAMECloakingMitigationEnabled::Yes); > > > > +#endif > > > > > > This is seems unexpected. There is nothing ResourceLoadStatistics related > > > about cname cloaking it seems. Why would enabling ResourceLoadStatistics > > > testing enable this. Seems like this should be treated like all other > > > features and enable as needed, not grouped in with another feature. > > > > In fact, this patch is very much part of ITP. You can see it in the > > compile-time flag it's under and that the two functions that implement the > > functionality: > > · NetworkDataTaskCocoa::updateFirstPartyInfoForTaskAndSession() > > · NetworkDataTaskCocoa::applyCookiePolicyForThirdPartyCNAMECloaking() > > … are both controlled by the newly exposed > > NetworkStorageSession::resourceLoadStatisticsEnabled(). > > I don't follow. Just because you put it behind the same preference name > doesn't mean a lot (unless you are saying something else by this). I think > the less we use vague terms like ITP in WebCore and WebKit the better. This > seems to be a distinct feature, "CNAME Cloaking Mitigation", that stands on > its own. Perhaps I am missing something. What aspects of the other features > that fall under the ITP umbrella does this depend on? ITP is a set of smaller features, all intended to prevent cross-site tracking. This particular one is about addressing something that trackers have deployed with the intent to work around the existing cap on client-side cookies. That's why this patch uses the same cookie expiry capping code. The 7-day expiry of website data is an ITP thing. You could argue that downgrades of referrers is its own feature, HSTS super cookie prevention is its own feature, third-party cookie blocking is its own feature, website data deletion is its own feature, verified partitioned cache is its own feature etc. But they are all part of ITP. > > > > Source/WebKit/Shared/WebPreferences.yaml:2063 > > > > + humanReadableDescription: "Enable third-party CNAME cloaking mitigation when Intelligent Tracking Prevention is enabled" > > > > > > I don't think including marketing names like Intelligent Tracking Prevention > > > is warranted here. There really doesn't seem to be any correlation. > > > > As explained above, this feature is turned on and off with the ITP setting. > > As you can see in related experimental settings, we do refer to ITP > > explicitly. This is to help developers understand what the setting is for > > since the commonly known name is ITP. > > I don't think this helpful. "CNAME cloaking mitigation" on its own describes > the feature. There is no need to pad it out. I can change the description. No worries. > > > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1968 > > > > +void WebsiteDataStore::setResourceLoadStatisticsThirdPartyCNAMEDomainForTesting(const URL& cnameURL, CompletionHandler<void()>&& completionHandler) > > > > +{ > > > > + if (cnameURL.host() != "testwebkit.org"_s && cnameURL.host() != "3rdpartytestwebkit.org"_s) { > > > > + completionHandler(); > > > > + return; > > > > + } > > > > + > > > > + auto callbackAggregator = CallbackAggregator::create(WTFMove(completionHandler)); > > > > + > > > > + for (auto& processPool : processPools()) { > > > > + if (auto* networkProcess = processPool->networkProcess()) > > > > + networkProcess->setThirdPartyCNAMEDomainForTesting(m_sessionID, WebCore::RegistrableDomain { cnameURL }, [callbackAggregator] { }); > > > > + } > > > > +} > > > > > > This (and a lot of the rest of the patch) seem to be adding a lot of binary > > > code bloat that is only used for testing. Can you explain why this can't be > > > tested without adding all this infrastructure? Is the issue no way to mock > > > out the networking bits needed to return a CNAME chain? > > > > Correct. The code you refer to is mocking the DNS resolution data that I > > have no way of controlling in layout tests. > > I think you should look into how to we can mock DNS resolution below WebKit, > otherwise we aren't really exercising the stack very well, and we end up > shipping a lot of unnecessary code to customers.

(In reply to John Wilander from comment #21) > (In reply to Sam Weinig from comment #20) > > (In reply to John Wilander from comment #15) > > > > > Source/WebCore/platform/network/NetworkStorageSession.h:196 > > > > > +#if PLATFORM(COCOA) || USE(CFURLCONNECTION) > > > > > + WEBCORE_EXPORT static NSHTTPCookie *capExpiryOfPersistentCookie(NSHTTPCookie *, Seconds cap); > > > > > +#endif > > > > > > > > It seems unfortunate to have Cocoa specific types referenced in a platform > > > > agnostic file. I think this likely needs more thought on how this should > > > > abstracted to support all loader backends, if and when they want to support > > > > exposing the cname chain and support mutation of cookies. > > > > > > I agree and felt the same way when I added the compile time check to fix the > > > non-Cocoa builds. There is no NetworkStorageSessionCocoa.h and I wanted to > > > reuse the existing code for capping cookies' expiry which is why I broke it > > > out of parseDOMCookie() in NetworkStorageSessionCocoa.mm. > > > > > > I did however check to see if there were other such instances in > > > NetworkStorageSession.h and there is a block guarded by #if PLATFORM(COCOA) > > > || USE(CFURLCONNECTION). Would moving my declaration to that block resolve > > > the issue for you? > > > > Not really :(. The file is a huge soup of chaos, and really needs to be > > managed better, but adding more platform specific stuff to it is going in > > the wrong direction. We have an existing attraction for Cookies, > > WebCore::Cookie, and if that can't be used for the job, we should figure out > > why and fix it. > > My alternative for this patch seems to be to copy the code to the > Cocoa-specific task and leave the cookie capping code in > NetworkStorageSessionCocoa.mm and NetworkStorageSession.h alone. Why not try to make it work with WebCore::Cookie? > > > > > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:875 > > > > > +#if HAVE(CFNETWORK_CNAME_AND_COOKIE_TRANSFORM_SPI) > > > > > + networkSession->setCNAMECloakingMitigationEnabled(WebCore::CNAMECloakingMitigationEnabled::Yes); > > > > > +#endif > > > > > > > > This is seems unexpected. There is nothing ResourceLoadStatistics related > > > > about cname cloaking it seems. Why would enabling ResourceLoadStatistics > > > > testing enable this. Seems like this should be treated like all other > > > > features and enable as needed, not grouped in with another feature. > > > > > > In fact, this patch is very much part of ITP. You can see it in the > > > compile-time flag it's under and that the two functions that implement the > > > functionality: > > > · NetworkDataTaskCocoa::updateFirstPartyInfoForTaskAndSession() > > > · NetworkDataTaskCocoa::applyCookiePolicyForThirdPartyCNAMECloaking() > > > … are both controlled by the newly exposed > > > NetworkStorageSession::resourceLoadStatisticsEnabled(). > > > > I don't follow. Just because you put it behind the same preference name > > doesn't mean a lot (unless you are saying something else by this). I think > > the less we use vague terms like ITP in WebCore and WebKit the better. This > > seems to be a distinct feature, "CNAME Cloaking Mitigation", that stands on > > its own. Perhaps I am missing something. What aspects of the other features > > that fall under the ITP umbrella does this depend on? > > ITP is a set of smaller features, all intended to prevent cross-site > tracking. This particular one is about addressing something that trackers > have deployed with the intent to work around the existing cap on client-side > cookies. That's why this patch uses the same cookie expiry capping code. The > 7-day expiry of website data is an ITP thing. > > You could argue that downgrades of referrers is its own feature, HSTS super > cookie prevention is its own feature, third-party cookie blocking is its own > feature, website data deletion is its own feature, verified partitioned > cache is its own feature etc. But they are all part of ITP. I would argue those are all independent features, and grouping them under a single banner only confuses things, at least at the engine level. We should name functions and classes and feature flags based on what they do, not under some vague umbrella.

Comment on attachment 406121 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406121&action=review r=me, but please look at the Mac-AS bot failure. I can't tell if that is due to missing SPI on that bot, or a true AS-specific build bug in the code. > Source/WebCore/PAL/ChangeLog:11 > + - _cookieTransformCallback Nit: Extra space before _cookieTransformCallback > Source/WebKit/ChangeLog:36 > + - _cookieTransformCallback Ditto > Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:299 > +#if HAVE(CFNETWORK_CNAME_AND_COOKIE_TRANSFORM) Should this be HAVE(CFNETWORK_CNAME_AND_COOKIE_TRANSFORM_SPI)? > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:313 > + if (![cookie isSessionOnly] && (!cookie.expiresDate || cookie.expiresDate.timeIntervalSinceNow > cap.seconds())) { Nit: This would have been easier to understand with an early return for session-only cookies. > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:134 > +// FIXME: Remove these selector checks when macOS Big Sur has shipped. Nit: Please file a Bugzilla+Radar to make sure we do this! > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:189 > + Nit: Extra line > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1942 > + if (cnameURL.host() != "testwebkit.org" && cnameURL.host() != "3rdpartytestwebkit.org") { Nit: It would be smart to have a more central place to maintain our test server names, possibly in a WebCoreTestSupport location or something.

(In reply to Brent Fulgham from comment #23) > Comment on attachment 406121 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=406121&action=review > > r=me, but please look at the Mac-AS bot failure. I can't tell if that is due > to missing SPI on that bot, or a true AS-specific build bug in the code. > > > Source/WebCore/PAL/ChangeLog:11 > > + - _cookieTransformCallback > > Nit: Extra space before _cookieTransformCallback Will fix. > > Source/WebKit/ChangeLog:36 > > + - _cookieTransformCallback > > Ditto Will fix. > > Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:299 > > +#if HAVE(CFNETWORK_CNAME_AND_COOKIE_TRANSFORM) > > Should this be HAVE(CFNETWORK_CNAME_AND_COOKIE_TRANSFORM_SPI)? You're right! This is very likely the reason for the AS bot failure. Will make sure it's fixed before landing. > > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:313 > > + if (![cookie isSessionOnly] && (!cookie.expiresDate || cookie.expiresDate.timeIntervalSinceNow > cap.seconds())) { > > Nit: This would have been easier to understand with an early return for > session-only cookies. Will fix. > > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:134 > > +// FIXME: Remove these selector checks when macOS Big Sur has shipped. > > Nit: Please file a Bugzilla+Radar to make sure we do this! Will do. > > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:189 > > + > > Nit: Extra line Will fix. > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1942 > > + if (cnameURL.host() != "testwebkit.org" && cnameURL.host() != "3rdpartytestwebkit.org") { > > Nit: It would be smart to have a more central place to maintain our test > server names, possibly in a WebCoreTestSupport location or something. True. I'll file a Bugzilla about that. Thanks, Brent!

Created attachment 406188 [details] Patch

Comment on attachment 406188 [details] Patch Just waiting to see that the bots are happy before landing.

Created attachment 406190 [details] Patch

Comment on attachment 406190 [details] Patch Added the Buzilla link to the comment, as requested by Brent.

Um, my feedback has not been addressed. Please don't land this.

(In reply to Sam Weinig from comment #29) > Um, my feedback has not been addressed. Please don't land this. Thanks for taking the time to review this work. I didn't intend to make it feel like I was dismissing any of your comments without thought. And while I agree with the thrust of you design recommendations, I do not agree that this patch is the place to make large architecture changes or create new testing infrastructure. I think John did address the most actionable items you brought up. What seems to remain are: (1) You wish that the various ITP concepts were decoupled and expressed in smaller, specific code concepts. I agree, but that is way out of scope for this patch. That is a large refactoring project that should be done independently. (2) You seem to feel that using the public name for ITP in the flag that controls an ITP behavior is confusing. I simply disagree with that concern. (3) You dislike Cocoa-specific types being used in a file (which already does this). I agree this would be a good thing to fix, but blocking this patch to fix this existing issue isn't appropriate. (4) Designing a system to mock DNS lookup in the WebKit engine before landing this patch is a nice idea, but wholly inappropriate for this patch. I will admit that we should file radars to act on these items you suggest, but they are not reasons to block this work. I will get to work on filing those bugs and scheduling time to address them properly. Thanks!

Comment on attachment 406190 [details] Patch Patch now passes on all bots.

(In reply to Brent Fulgham from comment #30) > (In reply to Sam Weinig from comment #29) > > Um, my feedback has not been addressed. Please don't land this. > > Thanks for taking the time to review this work. I didn't intend to make it > feel like I was dismissing any of your comments without thought. And while I > agree with the thrust of you design recommendations, I do not agree that > this patch is the place to make large architecture changes or create new > testing infrastructure. I don't believe I recommended any large architectural design changes. I do think that having the right testing infrastructure in place for new functionality is appropriate. As this is one of the first times (maybe the first time) we have had DNS related functionality being used in WebCore, ensuring we have a way to test that is absolutely appropriate. If you don't want to block on this, you could file additional bugs and note that you will be following up, but simply dismissing the feedback doesn't feel like the right response. > > I think John did address the most actionable items you brought up. What > seems to remain are: > > (1) You wish that the various ITP concepts were decoupled and expressed in > smaller, specific code concepts. I agree, but that is way out of scope for > this patch. That is a large refactoring project that should be done > independently. This is new functionality. There is no reason to have this coupled to the ITP concept or name. There is no large refactoring needed. Just use better names in the new code. > > (2) You seem to feel that using the public name for ITP in the flag that > controls an ITP behavior is confusing. I simply disagree with that concern. I do feel that. If you disagree, then it would be nice if you could state disagreement so it can be discussed. Disagreeing by bypassing my review does not seem to a be an appropriate way to convey that to me. > > (3) You dislike Cocoa-specific types being used in a file (which already > does this). I agree this would be a good thing to fix, but blocking this > patch to fix this existing issue isn't appropriate. I did not ask John to remove the existing issues, just not add more to it. Again, this is how we improve the code. We don't propagate bad design choices. > > (4) Designing a system to mock DNS lookup in the WebKit engine before > landing this patch is a nice idea, but wholly inappropriate for this patch. I completely disagree. In many ways, ensuring we have appropriate ways to test things before we make changes is absolutely an appropriate topic to discuss in patch review. Again, if you disagree, arguing that point would be appropriate, disregarding the feedback is not. Please don't land this change while this discussion is on going.

Committed r265389: <https://trac.webkit.org/changeset/265389> All reviewed patches have been landed. Closing bug and clearing flags on attachment 406190 [details].

(In reply to Sam Weinig from comment #32) > I do think that having the right testing infrastructure in place for new > functionality is appropriate. As this is one of the first times (maybe the > first time) we have had DNS related functionality being used in WebCore, > ensuring we have a way to test that is absolutely appropriate. If you don't > want to block on this, you could file additional bugs and note that you will > be following up, but simply dismissing the feedback doesn't feel like the > right response. Yes -- I did intend to follow up with bugs, but was pressed for time. I'm sorry that made you feel like I was planning to ignore the idea. I've now filed Bug 215294 to implement this concept. (More to come).

(In reply to Brent Fulgham from comment #30) > (1) You wish that the various ITP concepts were decoupled and expressed in > smaller, specific code concepts. I agree, but that is way out of scope for > this patch. That is a large refactoring project that should be done > independently. I filed Bug 215295 to properly decouple and name the various bits making up ITP features.

(In reply to Sam Weinig from comment #20) > Not really :(. The file is a huge soup of chaos, and really needs to be > managed better, but adding more platform specific stuff to it is going in > the wrong direction. We have an existing attraction for Cookies, > WebCore::Cookie, and if that can't be used for the job, we should figure out > why and fix it. I filed Bug 215297 to address this feedback.

I don't want to step into an argument here, but I agree with the position that this particular setting, and all the others that John mentioned, should not be bundled under the umbrella name "ITP" at the WebCore level. ITP is the marketing term we expose from Safari and WebKit. It's an intentionally vague term that covers a lot of smaller features, that the users don't really care about. But those smaller features should be described properly in the code, and controlled independently. This will help us as developers, and help Web developers who happen to see these error messages. e.g. If I was told that ITP wasn't working, where would I look? If I were told that there was a problem with CNAME cloaking I'd at least have something to start from. The code base seems to use the term "resourceLoadStatistics" to cover a lot of things, and doesn't imply tracking prevention. Could we clean this up?

I also think it is time that all the ITP related toggles that we expose to users be grouped together, probably by putting them in a submenu where the existing ITP debug mode toggle is. (We could probably provide categories for all experimental features and build submenus that way) Also, was this intended to be user facing? Why not an internal feature? And if it is user facing, why isn't it part of the mysterious "debug mode" (which I have understood to mean "more tracking prevention features that are coming soon" as well as "more debug output")?