Bug 199385 – Only allow fetching and removing session credentials from WebsiteDataStore

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sihui Liu]

...

[Sihui Liu]

Created attachment 373275 [details]
Patch

[Geoffrey Garen]

Comment on attachment 373275 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1660
> +                session->credentialStorage().removeCredentialsWithOrigin(origin);

My understanding is that using an HTTP Auth credential will establish both a WebCore::CredentialStorage entry and a non-persistent NSURLCredentialStorage entry. Is that right? If so, do we need to do additional work to remove the non-persistent NSURLCredentialStorage entry? You might need an HTTP test to test this, to see whether a credential is sent over the HTTP connection.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:-536
> -#if PLATFORM(COCOA)
> -    if (dataTypes.contains(WebsiteDataType::Credentials) && isPersistent()) {
> -        for (auto& processPool : processPools()) {
> -            if (!processPool->networkProcess())
> -                continue;
> -            
> -            callbackAggregator->addPendingCallback();
> -            WTF::CompletionHandler<void(Vector<WebCore::SecurityOriginData>&&)> completionHandler = [callbackAggregator](Vector<WebCore::SecurityOriginData>&& origins) mutable {
> -                WebsiteData websiteData;
> -                for (auto& origin : origins)
> -                    websiteData.entries.append(WebsiteData::Entry { origin, WebsiteDataType::Credentials, 0 });
> -                callbackAggregator->removePendingCallback(WTFMove(websiteData));
> -            };
> -            processPool->networkProcess()->sendWithAsyncReply(Messages::NetworkProcess::OriginsWithPersistentCredentials(), WTFMove(completionHandler));
> -        }
> -    }
> -#endif
> -

Don't we still need to list non-persistent credentials in this function?

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:-1104
> -    if (dataTypes.contains(WebsiteDataType::Credentials) && isPersistent()) {
> -        for (auto& processPool : processPools()) {
> -            if (!processPool->networkProcess())
> -                continue;
> -            
> -            callbackAggregator->addPendingCallback();
> -            WTF::CompletionHandler<void()> completionHandler = [callbackAggregator]() mutable {
> -                callbackAggregator->removePendingCallback();
> -            };
> -            processPool->networkProcess()->sendWithAsyncReply(Messages::NetworkProcess::RemoveCredentialsWithOrigins(origins), WTFMove(completionHandler));
> -        }
> -    }
> -

Don't we still need to remove non-persistent credentials in this function?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebsiteDatastore.mm:-207
> -    [websiteDataStore removeDataOfTypes:[NSSet setWithObject:_WKWebsiteDataTypeCredentials] forDataRecords:[NSArray arrayWithObject:expectedRecord.get()] completionHandler:^(void) {

Can we include a test that adds a persistent credential to the shared credential store, and then verifies that removing website data does not remove that credential?

[EWS Watchlist]

Comment on attachment 373275 [details]
Patch

Attachment 373275 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12632551

New failing tests:
fast/block/float/float-with-anonymous-previous-sibling.html

[EWS Watchlist]

Created attachment 373281 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Sihui Liu]

(In reply to Geoffrey Garen from comment #2)
> Comment on attachment 373275 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373275&action=review
> 
> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1660
> > +                session->credentialStorage().removeCredentialsWithOrigin(origin);
> 
> My understanding is that using an HTTP Auth credential will establish both a
> WebCore::CredentialStorage entry and a non-persistent NSURLCredentialStorage
> entry. Is that right? If so, do we need to do additional work to remove the
> non-persistent NSURLCredentialStorage entry? You might need an HTTP test to
> test this, to see whether a credential is sent over the HTTP connection.
> 

I plan to fix the API first and treat this as a different bug. Our current problem is fetched credentials cannot be deleted. After this patch the API only has access to credentials stored in maps of CredentialStorage, so if client asks to delete credentials and then fetch, we will return empty set.

Next step is to include session credentials in NSURLCredentialStorage in fetch and removal, which needs some testing to see networking behavior.

> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:-536
> > -#if PLATFORM(COCOA)
> > -    if (dataTypes.contains(WebsiteDataType::Credentials) && isPersistent()) {
> > -        for (auto& processPool : processPools()) {
> > -            if (!processPool->networkProcess())
> > -                continue;
> > -            
> > -            callbackAggregator->addPendingCallback();
> > -            WTF::CompletionHandler<void(Vector<WebCore::SecurityOriginData>&&)> completionHandler = [callbackAggregator](Vector<WebCore::SecurityOriginData>&& origins) mutable {
> > -                WebsiteData websiteData;
> > -                for (auto& origin : origins)
> > -                    websiteData.entries.append(WebsiteData::Entry { origin, WebsiteDataType::Credentials, 0 });
> > -                callbackAggregator->removePendingCallback(WTFMove(websiteData));
> > -            };
> > -            processPool->networkProcess()->sendWithAsyncReply(Messages::NetworkProcess::OriginsWithPersistentCredentials(), WTFMove(completionHandler));
> > -        }
> > -    }
> > -#endif
> > -
> 
> Don't we still need to list non-persistent credentials in this function?
> 

It's already handled by processPool->networkProcess()->fetchWebsiteData.

> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:-1104
> > -    if (dataTypes.contains(WebsiteDataType::Credentials) && isPersistent()) {
> > -        for (auto& processPool : processPools()) {
> > -            if (!processPool->networkProcess())
> > -                continue;
> > -            
> > -            callbackAggregator->addPendingCallback();
> > -            WTF::CompletionHandler<void()> completionHandler = [callbackAggregator]() mutable {
> > -                callbackAggregator->removePendingCallback();
> > -            };
> > -            processPool->networkProcess()->sendWithAsyncReply(Messages::NetworkProcess::RemoveCredentialsWithOrigins(origins), WTFMove(completionHandler));
> > -        }
> > -    }
> > -
> 
> Don't we still need to remove non-persistent credentials in this function?
> 

It's already handled by processPool->networkProcess()->deleteWebsiteDataForOrigins.

> > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebsiteDatastore.mm:-207
> > -    [websiteDataStore removeDataOfTypes:[NSSet setWithObject:_WKWebsiteDataTypeCredentials] forDataRecords:[NSArray arrayWithObject:expectedRecord.get()] completionHandler:^(void) {
> 
> Can we include a test that adds a persistent credential to the shared
> credential store, and then verifies that removing website data does not
> remove that credential?

Yes, we should be able to do this with NSURLCredentialStorage API.

[Sihui Liu]

Created attachment 373307 [details]
Patch

[EWS Watchlist]

Comment on attachment 373307 [details]
Patch

Attachment 373307 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12637230

New failing tests:
jquery/offset.html

[EWS Watchlist]

Created attachment 373310 [details]
Archive of layout-test-results from ews116 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Sihui Liu]

Created attachment 373328 [details]
Patch

[Sihui Liu]

> > > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebsiteDatastore.mm:-207
> > > -    [websiteDataStore removeDataOfTypes:[NSSet setWithObject:_WKWebsiteDataTypeCredentials] forDataRecords:[NSArray arrayWithObject:expectedRecord.get()] completionHandler:^(void) {
> > 
> > Can we include a test that adds a persistent credential to the shared
> > credential store, and then verifies that removing website data does not
> > remove that credential?
> 
> Yes, we should be able to do this with NSURLCredentialStorage API.

Inspecting the API again, t's probably not easy to test this. For API clients, the normal flow is they fetch the records from the API first and then pass some records to API to delete. If they cannot fetch the persistent entries as WKWebsiteDataRecord, they cannot delete it.

[Alex Christensen]

Comment on attachment 373328 [details]
Patch

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

r=me

> Source/WebCore/platform/network/CredentialStorage.cpp:198
> +    return;

unnecessary

> Source/WebCore/platform/network/CredentialStorage.cpp:203
> +    return;

ditto

[Sihui Liu]

Created attachment 373436 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 373436 [details]
Patch for landing

Clearing flags on attachment: 373436

Committed r247123: <https://trac.webkit.org/changeset/247123>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/52622080>

[Ryan Haddad]

Reverted r247123 for reason:

Caused TestWebKitAPI.Challenge.BasicProposedCredential to fail.

Committed r247162: <https://trac.webkit.org/changeset/247162>

[Ryan Haddad]

(In reply to Ryan Haddad from comment #16)
> Reverted r247123 for reason:
> 
> Caused TestWebKitAPI.Challenge.BasicProposedCredential to fail.
> 
> Committed r247162: <https://trac.webkit.org/changeset/247162>

    TestWebKitAPI.Challenge.BasicProposedCredential
        _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL.
        
        /Volumes/Data/slave/mojave-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/Challenge.mm:200
        Value of: receivedSecondChallenge
          Actual: false
        Expected: true

https://build.webkit.org/builders/Apple%20Mojave%20Release%20WK2%20(Tests)/builds/5133/steps/run-api-tests/logs/stdio

I was able to reproduce the failure locally with a build of r247123 with the following:
run-api-tests Challenge --iterations 10

I could not reproduce the failure with r247121.

[Ryan Haddad]

*** Bug 199501 has been marked as a duplicate of this bug. ***

[Ryan Haddad]

(In reply to Ryan Haddad from comment #17)
> (In reply to Ryan Haddad from comment #16)
> 
> I was able to reproduce the failure locally with a build of r247123 with the
> following:
> run-api-tests Challenge --iterations 10
> 
> I could not reproduce the failure with r247121.

It looks like the bots are seeing these tests time out, too:
    TestWebKitAPI.WKWebsiteDataStore.RemovePersistentCredentials

    TestWebKitAPI.Challenge.BasicProposedCredential

    TestWebKitAPI.WKWebsiteDataStore.FetchPersistentCredentials

    TestWebKitAPI.Challenge.SecIdentity

There are keychain prompts on screen asking for access to '127.0.0.1 (testuser) and (username)'.

[Sihui Liu]

The patch introduces a behavior change that removeDataOfTypes would not remove persistent credentials. Therefore, the persistent credentials created in one test can be used by the later test, and didReceiveAuthenticationChallenge is not invoked (previous credentials are automatically applied).

To solve this issue, we need to provide a method to clear the persistent credentials in the test.

The original patch itself should be safe. We just need to make some changes on the tests.

[Sihui Liu]

Created attachment 373591 [details]
Patch

[Aakash Jain]

> There are keychain prompts on screen asking for access to '127.0.0.1 (testuser) and (username)'.
Still noticing this on Mac api-tests ews bots. See <rdar://problem/52736905>.

iOS API tests bots are happy (https://ews-build.webkit.org/#/builders/9), but mac API tests bots (https://ews-build.webkit.org/#/builders/3) are still consistently failing 4 API tests which Ryan mentioned above (and getting stuck occasionally probably because of keychain prompts).

[WebKit Commit Bot]

Comment on attachment 373591 [details]
Patch

Rejecting attachment 373591 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 373591, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebKit/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: https://webkit-queues.webkit.org/results/12698296

[Sihui Liu]

Created attachment 373722 [details]
Patch for landing

[Aakash Jain]

Comment on attachment 373722 [details]
Patch for landing

Let's have EWS run through this patch before landing. I have pressed the 'Submit to EWS' buttons.

[WebKit Commit Bot]

Comment on attachment 373722 [details]
Patch for landing

Clearing flags on attachment: 373722

Committed r247270: <https://trac.webkit.org/changeset/247270>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Aakash Jain]

Same tests have started to fail again on EWS mac api tests queue.

e.g.: https://ews-build.webkit.org/#/builders/3/builds/4439

TestWebKitAPI.Challenge.BasicProposedCredential,
TestWebKitAPI.WKWebsiteDataStore.FetchPersistentCredentials,
TestWebKitAPI.Challenge.SecIdentity