Bug 199385 - Only allow fetching and removing session credentials from WebsiteDataStore
Summary: Only allow fetching and removing session credentials from WebsiteDataStore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sihui Liu
URL:
Keywords: InRadar
: 199501 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-07-01 15:53 PDT by Sihui Liu
Modified: 2019-07-09 18:44 PDT (History)
9 users (show)

See Also:


Attachments
Patch (19.27 KB, patch)
2019-07-01 16:01 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews215 for win-future (13.69 MB, application/zip)
2019-07-01 17:06 PDT, Build Bot
no flags Details
Patch (24.75 KB, patch)
2019-07-02 00:04 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-highsierra (3.05 MB, application/zip)
2019-07-02 01:55 PDT, Build Bot
no flags Details
Patch (24.83 KB, patch)
2019-07-02 09:00 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (24.80 KB, patch)
2019-07-03 16:45 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (34.84 KB, patch)
2019-07-06 19:37 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (34.97 KB, patch)
2019-07-09 09:29 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sihui Liu 2019-07-01 15:53:22 PDT
...
Comment 1 Sihui Liu 2019-07-01 16:01:04 PDT
Created attachment 373275 [details]
Patch
Comment 2 Geoffrey Garen 2019-07-01 16:59:27 PDT
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?
Comment 3 Build Bot 2019-07-01 17:06:32 PDT
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
Comment 4 Build Bot 2019-07-01 17:06:36 PDT
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
Comment 5 Sihui Liu 2019-07-01 17:33:42 PDT
(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.
Comment 6 Sihui Liu 2019-07-02 00:04:52 PDT
Created attachment 373307 [details]
Patch
Comment 7 Build Bot 2019-07-02 01:55:20 PDT
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
Comment 8 Build Bot 2019-07-02 01:55:21 PDT
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
Comment 9 Sihui Liu 2019-07-02 09:00:40 PDT
Created attachment 373328 [details]
Patch
Comment 10 Sihui Liu 2019-07-02 09:54:39 PDT
> > > 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.
Comment 11 Alex Christensen 2019-07-03 13:03:11 PDT
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
Comment 12 Sihui Liu 2019-07-03 16:45:06 PDT
Created attachment 373436 [details]
Patch for landing
Comment 13 WebKit Commit Bot 2019-07-03 17:58:49 PDT
Comment on attachment 373436 [details]
Patch for landing

Clearing flags on attachment: 373436

Committed r247123: <https://trac.webkit.org/changeset/247123>
Comment 14 WebKit Commit Bot 2019-07-03 17:58:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2019-07-03 17:59:51 PDT
<rdar://problem/52622080>
Comment 16 Ryan Haddad 2019-07-05 09:59:48 PDT
Reverted r247123 for reason:

Caused TestWebKitAPI.Challenge.BasicProposedCredential to fail.

Committed r247162: <https://trac.webkit.org/changeset/247162>
Comment 17 Ryan Haddad 2019-07-05 10:03:21 PDT
(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.
Comment 18 Ryan Haddad 2019-07-05 10:04:03 PDT
*** Bug 199501 has been marked as a duplicate of this bug. ***
Comment 19 Ryan Haddad 2019-07-05 11:28:45 PDT
(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)'.
Comment 20 Sihui Liu 2019-07-05 17:52:06 PDT
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.
Comment 21 Sihui Liu 2019-07-06 19:37:12 PDT
Created attachment 373591 [details]
Patch
Comment 22 Aakash Jain 2019-07-07 09:26:19 PDT
> 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).
Comment 23 WebKit Commit Bot 2019-07-09 08:40:04 PDT
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
Comment 24 Sihui Liu 2019-07-09 09:29:01 PDT
Created attachment 373722 [details]
Patch for landing
Comment 25 Aakash Jain 2019-07-09 09:45:44 PDT
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.
Comment 26 WebKit Commit Bot 2019-07-09 12:26:04 PDT
Comment on attachment 373722 [details]
Patch for landing

Clearing flags on attachment: 373722

Committed r247270: <https://trac.webkit.org/changeset/247270>
Comment 27 WebKit Commit Bot 2019-07-09 12:26:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 Aakash Jain 2019-07-09 18:44:07 PDT
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