RESOLVED FIXED 199385
Only allow fetching and removing session credentials from WebsiteDataStore
https://bugs.webkit.org/show_bug.cgi?id=199385
Summary Only allow fetching and removing session credentials from WebsiteDataStore
Sihui Liu
Reported 2019-07-01 15:53:22 PDT
...
Attachments
Patch (19.27 KB, patch)
2019-07-01 16:01 PDT, Sihui Liu
no flags
Archive of layout-test-results from ews215 for win-future (13.69 MB, application/zip)
2019-07-01 17:06 PDT, EWS Watchlist
no flags
Patch (24.75 KB, patch)
2019-07-02 00:04 PDT, Sihui Liu
no flags
Archive of layout-test-results from ews116 for mac-highsierra (3.05 MB, application/zip)
2019-07-02 01:55 PDT, EWS Watchlist
no flags
Patch (24.83 KB, patch)
2019-07-02 09:00 PDT, Sihui Liu
no flags
Patch for landing (24.80 KB, patch)
2019-07-03 16:45 PDT, Sihui Liu
no flags
Patch (34.84 KB, patch)
2019-07-06 19:37 PDT, Sihui Liu
no flags
Patch for landing (34.97 KB, patch)
2019-07-09 09:29 PDT, Sihui Liu
no flags
Sihui Liu
Comment 1 2019-07-01 16:01:04 PDT
Geoffrey Garen
Comment 2 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?
EWS Watchlist
Comment 3 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
EWS Watchlist
Comment 4 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
Sihui Liu
Comment 5 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.
Sihui Liu
Comment 6 2019-07-02 00:04:52 PDT
EWS Watchlist
Comment 7 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
EWS Watchlist
Comment 8 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
Sihui Liu
Comment 9 2019-07-02 09:00:40 PDT
Sihui Liu
Comment 10 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.
Alex Christensen
Comment 11 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
Sihui Liu
Comment 12 2019-07-03 16:45:06 PDT
Created attachment 373436 [details] Patch for landing
WebKit Commit Bot
Comment 13 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>
WebKit Commit Bot
Comment 14 2019-07-03 17:58:51 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 15 2019-07-03 17:59:51 PDT
Ryan Haddad
Comment 16 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>
Ryan Haddad
Comment 17 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.
Ryan Haddad
Comment 18 2019-07-05 10:04:03 PDT
*** Bug 199501 has been marked as a duplicate of this bug. ***
Ryan Haddad
Comment 19 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)'.
Sihui Liu
Comment 20 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.
Sihui Liu
Comment 21 2019-07-06 19:37:12 PDT
Aakash Jain
Comment 22 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).
WebKit Commit Bot
Comment 23 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
Sihui Liu
Comment 24 2019-07-09 09:29:01 PDT
Created attachment 373722 [details] Patch for landing
Aakash Jain
Comment 25 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.
WebKit Commit Bot
Comment 26 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>
WebKit Commit Bot
Comment 27 2019-07-09 12:26:06 PDT
All reviewed patches have been landed. Closing bug.
Aakash Jain
Comment 28 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
Note You need to log in before you can comment on or make changes to this bug.