Bug 198854 – -[WKWebsiteDataStore removeDataOfTypes:modifiedSince:completionHandler:] doesn't delete _WKWebsiteDataTypeCredentials

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Sihui Liu]

We added removal of persistent credentials for origin in https://bugs.webkit.org/show_bug.cgi?id=197510. We should also support clearing all credentials.

[Sihui Liu]

Created attachment 372113 [details]
Patch

[Sihui Liu]

Created attachment 372126 [details]
Patch

[Per Arne Vollan]

Comment on attachment 372126 [details]
Patch

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

I think this looks very good! Leaving the final review to Geoff or Alex, since they are experts in this area :)

> Source/WebCore/ChangeLog:5
> +        -[WKWebsiteDataStore removeDataOfTypes:modifiedSince:completionHandler:] doesn't delete _WKWebsiteDataTypeCredentials
> +        https://bugs.webkit.org/show_bug.cgi?id=198854
> +

Add radar?

[Geoffrey Garen]

Comment on attachment 372126 [details]
Patch

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

> Source/WebCore/platform/network/mac/CredentialStorageMac.mm:41
> +Vector<WebCore::SecurityOriginData> CredentialStorage::getOriginsWithPersistentCredentials()

We usually only use "get" in a function name to indicate that it returns its value through an out parameter. So, this should remain "originsWithPersistentCredentials()".

> Source/WebCore/platform/network/mac/CredentialStorageMac.mm:53
> +        auto allCredentials = [[NSURLCredentialStorage sharedCredentialStorage] allCredentials];

You should move this line out of the loop so that you don't have to call it for each origin.

Also, I think this is a memory leak. -allCredentials returns a dictionary by copy. If you don't release the dictionary, it will leak. You can avoid memory leaks by using smart pointers. The best way to use a smart pointer here is probably

    auto allCredentials = adoptNS([[NSURLCredentialStorage sharedCredentialStorage] allCredentials]);

You should also put sharedCredentialStorage in a local variable to avoid re-fetching it.

> Source/WebCore/platform/network/mac/CredentialStorageMac.mm:71
> +    removePersistentCredentialsWithOrigins(getOriginsWithPersistentCredentials());

Can you do something more efficient here, since we don't need to match against a set of origins, and we can just remove all credentials?

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1285
> +        if (!sessionID.isEphemeral()) {

Why do we avoid returning this data for ephemeral sessions?

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1369
> +        if (!sessionID.isEphemeral())
> +            WebCore::CredentialStorage::clearPersistentCredentials();

Why do we avoid deleting website data for ephemeral sessions?

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1508
> +        if (!sessionID.isEphemeral())
> +            WebCore::CredentialStorage::removePersistentCredentialsWithOrigins(originDatas);

Ditto.

[Sihui Liu]

<rdar://problem/51386058>

[Sihui Liu]

(In reply to Geoffrey Garen from comment #4)
> Comment on attachment 372126 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=372126&action=review
> 
> > Source/WebCore/platform/network/mac/CredentialStorageMac.mm:41
> > +Vector<WebCore::SecurityOriginData> CredentialStorage::getOriginsWithPersistentCredentials()
> 
> We usually only use "get" in a function name to indicate that it returns its
> value through an out parameter. So, this should remain
> "originsWithPersistentCredentials()".
> 

Okay.

> > Source/WebCore/platform/network/mac/CredentialStorageMac.mm:53
> > +        auto allCredentials = [[NSURLCredentialStorage sharedCredentialStorage] allCredentials];
> 
> You should move this line out of the loop so that you don't have to call it
> for each origin.
> 
> Also, I think this is a memory leak. -allCredentials returns a dictionary by
> copy. If you don't release the dictionary, it will leak. You can avoid
> memory leaks by using smart pointers. The best way to use a smart pointer
> here is probably
> 
>     auto allCredentials = adoptNS([[NSURLCredentialStorage
> sharedCredentialStorage] allCredentials]);
> 
> You should also put sharedCredentialStorage in a local variable to avoid
> re-fetching it.
> 

Okay.

> > Source/WebCore/platform/network/mac/CredentialStorageMac.mm:71
> > +    removePersistentCredentialsWithOrigins(getOriginsWithPersistentCredentials());
> 
> Can you do something more efficient here, since we don't need to match
> against a set of origins, and we can just remove all credentials?
> 

Since we don't have API for clear all credentials, we can do similar thing as removePersistentCredentialsWithOrigins.

> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1285
> > +        if (!sessionID.isEphemeral()) {
> 
> Why do we avoid returning this data for ephemeral sessions?
> 
> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1369
> > +        if (!sessionID.isEphemeral())
> > +            WebCore::CredentialStorage::clearPersistentCredentials();
> 
> Why do we avoid deleting website data for ephemeral sessions?
> 
> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1508
> > +        if (!sessionID.isEphemeral())
> > +            WebCore::CredentialStorage::removePersistentCredentialsWithOrigins(originDatas);
> 
> Ditto.

This is the old behavior and I didn't change. My understanding is [NSURLCredentialStorage sharedCredentialStorage] is used for credentials shared across different apps(?), and network process keeps a copy of credentials from ephemeral session in memory with maps in WebCore::CredentialStorage.

[Sihui Liu]

Created attachment 372142 [details]
Patch

[Geoffrey Garen]

Comment on attachment 372142 [details]
Patch

r=me

[Sihui Liu]

Created attachment 372306 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 372306 [details]
Patch for landing

Clearing flags on attachment: 372306

Committed r246530: <https://trac.webkit.org/changeset/246530>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.