Bug 209369 - Add the capability to change all of a website's cookies to SameSite=Strict
Summary: Add the capability to change all of a website's cookies to SameSite=Strict
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-20 17:05 PDT by John Wilander
Modified: 2020-03-24 15:35 PDT (History)
8 users (show)

See Also:


Attachments
Patch (36.21 KB, patch)
2020-03-20 17:22 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (42.37 KB, patch)
2020-03-23 12:33 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch for landing (42.50 KB, patch)
2020-03-23 15:18 PDT, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Wilander 2020-03-20 17:05:45 PDT
We should add the capability to change all cookies for a specific registrable domain to be SameSite=Strict.
Comment 1 Radar WebKit Bug Importer 2020-03-20 17:06:11 PDT
<rdar://problem/60710690>
Comment 2 John Wilander 2020-03-20 17:22:20 PDT
Created attachment 394153 [details]
Patch
Comment 3 David Kilzer (:ddkilzer) 2020-03-20 21:04:02 PDT
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.
Comment 4 John Wilander 2020-03-23 10:31:28 PDT
(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. :/
Comment 5 John Wilander 2020-03-23 12:33:29 PDT
Created attachment 394287 [details]
Patch
Comment 6 John Wilander 2020-03-23 13:29:56 PDT
win layout test failures are unrelated.
Comment 7 Alex Christensen 2020-03-23 14:07:49 PDT
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.
Comment 8 John Wilander 2020-03-23 14:19:56 PDT
(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!
Comment 9 John Wilander 2020-03-23 14:23:39 PDT
(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!
Comment 10 Alex Christensen 2020-03-23 14:31:53 PDT
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.
Comment 11 John Wilander 2020-03-23 14:35:11 PDT
(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!
Comment 12 John Wilander 2020-03-23 15:18:20 PDT
Created attachment 394310 [details]
Patch for landing
Comment 13 EWS 2020-03-23 16:00:56 PDT
Committed r258884: <https://trac.webkit.org/changeset/258884>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394310 [details].
Comment 14 David Kilzer (:ddkilzer) 2020-03-24 14:35:50 PDT
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.
Comment 15 Alex Christensen 2020-03-24 15:35:28 PDT
I don't think this is an issue in practice.  I also don't think the "slowness" of the safest option is too slow.