Add the capability to change all of a website's cookies to SameSite=Strict

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[John Wilander]

We should add the capability to change all cookies for a specific registrable domain to be SameSite=Strict.

[Radar WebKit Bug Importer]

<rdar://problem/60710690>

[John Wilander]

Created attachment 394153 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 394153 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394153&action=review

> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:129
> +            NSMutableDictionary<NSHTTPCookiePropertyKey, id> *mutableProperties = [[[nsCookie properties] mutableCopy] autorelease];

While this is correct, it's better to use a RetainPtr<> here (and add .get() where needed), although I'm not certain the RetainPtr<NSMutableDictionary<...>> syntax will work, in which case you'd have to fall back to the less-satisfying RetainPtr<NSMutableDictionary>> declaration:

            RetainPtr<NSMutableDictionary<NSHTTPCookiePropertyKey, id>> *mutableProperties = adoptNS([[nsCookie properties] mutableCopy]);
            mutableProperties[NSHTTPCookieSameSitePolicy] = NSHTTPCookieSameSiteStrict;
            NSHTTPCookie *strictCookie = [NSHTTPCookie cookieWithProperties:mutableProperties.get()];
            [newCookiesToAdd addObject:strictCookie];

Or to use explicit mutableCopy/release (which is nicer because once we switch to ARC, the fix is just to remove the -release call instead of removing RetainPtr<> and the .get() calls):

            NSMutableDictionary<NSHTTPCookiePropertyKey, id> *mutableProperties = [[nsCookie properties] mutableCopy];
            mutableProperties[NSHTTPCookieSameSitePolicy] = NSHTTPCookieSameSiteStrict;
            NSHTTPCookie *strictCookie = [NSHTTPCookie cookieWithProperties:mutableProperties];
            [mutableProperties release];
            [newCookiesToAdd addObject:strictCookie];

Adding objects to the autoreleasePool in a loop is a good way to cause spikes in memory use (by filling up the pool itself and/or by causing the objects themselves not to be released immediately), and creates more work later when draining the pool.

> Tools/WebKitTestRunner/TestInvocation.cpp:958
> +    if (WKStringIsEqualToUTF8CString(messageName, "StatisticsSetToSameSiteStrictCookies")) {
> +        ASSERT(WKGetTypeID(messageBody) == WKStringGetTypeID());
> +        WKStringRef hostName = static_cast<WKStringRef>(messageBody);
> +        TestController::singleton().setStatisticsToSameSiteStrictCookies(hostName);
> +        return;
> +    }

This method is nearly 700 lines long!  Surely there's a better way to structure this using some kind of hash table and switch statement?

Not necessary to land this patch, but it shouldn't be left like this.

[John Wilander]

(In reply to David Kilzer (:ddkilzer) from comment #3)
> Comment on attachment 394153 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394153&action=review
> 
> > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:129
> > +            NSMutableDictionary<NSHTTPCookiePropertyKey, id> *mutableProperties = [[[nsCookie properties] mutableCopy] autorelease];
> 
> While this is correct, it's better to use a RetainPtr<> here (and add .get()
> where needed), although I'm not certain the
> RetainPtr<NSMutableDictionary<...>> syntax will work, in which case you'd
> have to fall back to the less-satisfying RetainPtr<NSMutableDictionary>>
> declaration:
> 
>             RetainPtr<NSMutableDictionary<NSHTTPCookiePropertyKey, id>>
> *mutableProperties = adoptNS([[nsCookie properties] mutableCopy]);
>             mutableProperties[NSHTTPCookieSameSitePolicy] =
> NSHTTPCookieSameSiteStrict;
>             NSHTTPCookie *strictCookie = [NSHTTPCookie
> cookieWithProperties:mutableProperties.get()];
>             [newCookiesToAdd addObject:strictCookie];
> 
> Or to use explicit mutableCopy/release (which is nicer because once we
> switch to ARC, the fix is just to remove the -release call instead of
> removing RetainPtr<> and the .get() calls):
> 
>             NSMutableDictionary<NSHTTPCookiePropertyKey, id>
> *mutableProperties = [[nsCookie properties] mutableCopy];
>             mutableProperties[NSHTTPCookieSameSitePolicy] =
> NSHTTPCookieSameSiteStrict;
>             NSHTTPCookie *strictCookie = [NSHTTPCookie
> cookieWithProperties:mutableProperties];
>             [mutableProperties release];
>             [newCookiesToAdd addObject:strictCookie];

Thank you, Dave! Exactly the kind of feedback and suggestions I was looking for. The manual release version works so I'll pick that.

> Adding objects to the autoreleasePool in a loop is a good way to cause
> spikes in memory use (by filling up the pool itself and/or by causing the
> objects themselves not to be released immediately), and creates more work
> later when draining the pool.

I assume the manual release resolves this concern.

> > Tools/WebKitTestRunner/TestInvocation.cpp:958
> > +    if (WKStringIsEqualToUTF8CString(messageName, "StatisticsSetToSameSiteStrictCookies")) {
> > +        ASSERT(WKGetTypeID(messageBody) == WKStringGetTypeID());
> > +        WKStringRef hostName = static_cast<WKStringRef>(messageBody);
> > +        TestController::singleton().setStatisticsToSameSiteStrictCookies(hostName);
> > +        return;
> > +    }
> 
> This method is nearly 700 lines long!  Surely there's a better way to
> structure this using some kind of hash table and switch statement?
> 
> Not necessary to land this patch, but it shouldn't be left like this.

Agreed. :/

[John Wilander]

Created attachment 394287 [details]
Patch

[John Wilander]

win layout test failures are unrelated.

[Alex Christensen]

Comment on attachment 394287 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394287&action=review

> Source/WTF/wtf/PlatformHave.h:447
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000)

Unfortunately, many existing checks in this file are incorrect.  You have to explicitly state that this is also available on watchOS and tvOS.

> Source/WebCore/platform/network/NetworkStorageSession.cpp:95
> +    UNUSED_PARAM(domain);

Just leave out the parameter name.
(const RegistrableDomain&,

> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:124
> +    RetainPtr<NSMutableArray> oldCookiesToDelete = adoptNS([[NSMutableArray alloc] init]);

These can be RetainPtr<NSMutableArray<NSHTTPCookie>>

> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:130
> +            NSMutableDictionary<NSHTTPCookiePropertyKey, id> *mutableProperties = [[nsCookie properties] mutableCopy];

RetainPtr<NSMutableDictionary<NSHTTPCookiePropertyKey, id>> mutableProperties = adoptNS([[nsCookie properties] mutableCopy]);
Then we don't need to manually release below.

> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:139
> +    if (!newCookiesToAdd.get().count)
> +        return completionHandler();

I don't think this early return actually does anything.

[John Wilander]

(In reply to Alex Christensen from comment #7)
> Comment on attachment 394287 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394287&action=review
> 
> > Source/WTF/wtf/PlatformHave.h:447
> > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000)
> 
> Unfortunately, many existing checks in this file are incorrect.  You have to
> explicitly state that this is also available on watchOS and tvOS.

Oh. I'll look for an example.

> > Source/WebCore/platform/network/NetworkStorageSession.cpp:95
> > +    UNUSED_PARAM(domain);
> 
> Just leave out the parameter name.
> (const RegistrableDomain&,

Will fix.

> > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:124
> > +    RetainPtr<NSMutableArray> oldCookiesToDelete = adoptNS([[NSMutableArray alloc] init]);
> 
> These can be RetainPtr<NSMutableArray<NSHTTPCookie>>
> 
> > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:130
> > +            NSMutableDictionary<NSHTTPCookiePropertyKey, id> *mutableProperties = [[nsCookie properties] mutableCopy];
> 
> RetainPtr<NSMutableDictionary<NSHTTPCookiePropertyKey, id>>
> mutableProperties = adoptNS([[nsCookie properties] mutableCopy]);
> Then we don't need to manually release below.

See Dave Kilzer's comments above. Do you agree manual release is better given his input?

> > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:139
> > +    if (!newCookiesToAdd.get().count)
> > +        return completionHandler();
> 
> I don't think this early return actually does anything.

Will remove.

Thanks, Alex!

[John Wilander]

(In reply to John Wilander from comment #8)
> (In reply to Alex Christensen from comment #7)
> > Comment on attachment 394287 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=394287&action=review
> > 
> > > Source/WTF/wtf/PlatformHave.h:447
> > > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000)
> > 
> > Unfortunately, many existing checks in this file are incorrect.  You have to
> > explicitly state that this is also available on watchOS and tvOS.
> 
> Oh. I'll look for an example.

What about Mac Catalyst?

Would this work?

#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000) || PLATFORM(MACCATALYST) || PLATFORM(WATCHOS) || PLATFORM(APPLETV)
> > > Source/WebCore/platform/network/NetworkStorageSession.cpp:95
> > > +    UNUSED_PARAM(domain);
> > 
> > Just leave out the parameter name.
> > (const RegistrableDomain&,
> 
> Will fix.
> 
> > > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:124
> > > +    RetainPtr<NSMutableArray> oldCookiesToDelete = adoptNS([[NSMutableArray alloc] init]);
> > 
> > These can be RetainPtr<NSMutableArray<NSHTTPCookie>>
> > 
> > > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:130
> > > +            NSMutableDictionary<NSHTTPCookiePropertyKey, id> *mutableProperties = [[nsCookie properties] mutableCopy];
> > 
> > RetainPtr<NSMutableDictionary<NSHTTPCookiePropertyKey, id>>
> > mutableProperties = adoptNS([[nsCookie properties] mutableCopy]);
> > Then we don't need to manually release below.
> 
> See Dave Kilzer's comments above. Do you agree manual release is better
> given his input?
> 
> > > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:139
> > > +    if (!newCookiesToAdd.get().count)
> > > +        return completionHandler();
> > 
> > I don't think this early return actually does anything.
> 
> Will remove.
> 
> Thanks, Alex!

[Alex Christensen]

Dave's concern was about the use of autorelease, which will not free the object until the next time the autorelease pool is drained, which in this case is after an unbounded number of loop iterations.  We should not use autorelease, I agree. 
I'm just saying that instead of manually calling release, use RetainPtr's destructor which does the same thing.  If the RetainPtr is in the scope of the loop, it will call release (not autorelease) each time the loop iterates.

(In reply to John Wilander from comment #9)
> Would this work?
> 
> #if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) ||
> (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000) ||
> PLATFORM(MACCATALYST) || PLATFORM(WATCHOS) || PLATFORM(APPLETV)
Looks good.

[John Wilander]

(In reply to Alex Christensen from comment #10)
> Dave's concern was about the use of autorelease, which will not free the
> object until the next time the autorelease pool is drained, which in this
> case is after an unbounded number of loop iterations.  We should not use
> autorelease, I agree. 
> I'm just saying that instead of manually calling release, use RetainPtr's
> destructor which does the same thing.  If the RetainPtr is in the scope of
> the loop, it will call release (not autorelease) each time the loop iterates.

I was referring to this comment:
"Or to use explicit mutableCopy/release (which is nicer because once we switch to ARC, the fix is just to remove the -release call instead of removing RetainPtr<> and the .get() calls)"

I can do either.

> (In reply to John Wilander from comment #9)
> > Would this work?
> > 
> > #if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) ||
> > (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000) ||
> > PLATFORM(MACCATALYST) || PLATFORM(WATCHOS) || PLATFORM(APPLETV)
> Looks good.

Thanks!

[John Wilander]

Created attachment 394310 [details]
Patch for landing

[EWS]

Committed r258884: <https://trac.webkit.org/changeset/258884>

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

[David Kilzer (:ddkilzer)]

Comment on attachment 394310 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=394310&action=review

> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:143
> +    BEGIN_BLOCK_OBJC_EXCEPTIONS;
> +    for (NSHTTPCookie *oldCookie in oldCookiesToDelete.get())
> +        deleteHTTPCookie(cookieStorage().get(), oldCookie);
> +
> +    for (NSHTTPCookie *newCookie in newCookiesToAdd.get())
> +        [nsCookieStorage() setCookie:newCookie];
> +    END_BLOCK_OBJC_EXCEPTIONS;

Had a thought here after this landed:  What happens when an exception is thrown on the first -setCookie: call?  That would mean every cookie is deleted and the new cookies are never saved.

Exceptions in other places could lead to some old cookies not being deleted or some new cookies not being added.

Not sure what the best fix for this is, but the middle ground would be:

    BEGIN_BLOCK_OBJC_EXCEPTIONS;
    for (NSHTTPCookie *oldCookie in oldCookiesToDelete.get())
        deleteHTTPCookie(cookieStorage().get(), oldCookie);
    END_BLOCK_OBJC_EXCEPTIONS;

    BEGIN_BLOCK_OBJC_EXCEPTIONS;
    for (NSHTTPCookie *newCookie in newCookiesToAdd.get())
        [nsCookieStorage() setCookie:newCookie];
    END_BLOCK_OBJC_EXCEPTIONS;

The "safest" strategy (meaning preserves the most cookies) but slowest (because the try/catch block is inside the for loop) might be:

    for (NSHTTPCookie *oldCookie in oldCookiesToDelete.get()) {
        BEGIN_BLOCK_OBJC_EXCEPTIONS;
        deleteHTTPCookie(cookieStorage().get(), oldCookie);
        END_BLOCK_OBJC_EXCEPTIONS;
    }

    for (NSHTTPCookie *newCookie in newCookiesToAdd.get()) {
        BEGIN_BLOCK_OBJC_EXCEPTIONS;
        [nsCookieStorage() setCookie:newCookie];
        END_BLOCK_OBJC_EXCEPTIONS;
    }

This may also not be an issue in practice, but I wanted to mention it after I noticed it.

[Alex Christensen]

I don't think this is an issue in practice.  I also don't think the "slowness" of the safest option is too slow.