WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
,
EWS Watchlist
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
,
EWS Watchlist
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
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Sihui Liu
Comment 1
2019-07-01 16:01:04 PDT
Created
attachment 373275
[details]
Patch
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
Created
attachment 373307
[details]
Patch
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
Created
attachment 373328
[details]
Patch
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
<
rdar://problem/52622080
>
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
Created
attachment 373591
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug