[webkit-reviews] review denied: [Bug 215201] Experimental: Cap the expiry of persistent cookies set in 3rd-party CNAME cloaked HTTP responses : [Attachment 406091] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied John Wilander <wilander at apple.com>'s
request for review:
Bug 215201: Experimental: Cap the expiry of persistent cookies set in 3rd-party
CNAME cloaked HTTP responses
https://bugs.webkit.org/show_bug.cgi?id=215201

Attachment 406091: Patch

https://bugs.webkit.org/attachment.cgi?id=406091&action=review




--- Comment #10 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 406091
  --> https://bugs.webkit.org/attachment.cgi?id=406091
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::CNAMECloakingMitigat
ionEnabled::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::CNAMECloakingMitigat
ionEnabled::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(cons
t 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.