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

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted 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 406121: Patch

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




--- Comment #23 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 406121
  --> https://bugs.webkit.org/attachment.cgi?id=406121
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.