Summary: | Session cookies aren't reliably set when using default WKWebSiteDataStore | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sihui Liu <sihui_liu> | ||||||||||||||||
Component: | WebKit API | Assignee: | Sihui Liu <sihui_liu> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | achristensen, beidson, cdumez, commit-queue, ews-watchlist, ggaren, herr.ernst, realdawei, sihui_liu, webkit-bug-importer | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Sihui Liu
2018-05-14 14:23:16 PDT
Created attachment 340362 [details]
Patch
Comment on attachment 340362 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340362&action=review I cannot comment about the overall approach but I have a few comments: > Source/WebKit/NetworkProcess/mac/RemoteNetworkingContext.mm:51 > + auto* session = NetworkStorageSession::storageSession(sessionID); Double looking seems unfortunate here. Why not update the if above to look like: if (auto* session = NetworkStorageSession::storageSession(sessionID)) { > Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:-61 > - allCookies = WebCore::NetworkStorageSession::defaultStorageSession().getAllCookies(); Doesn't this mean we're only returning the cookies added by the API for the default session now? Previously, we'd ask the default storage session which presumably could return cookies saved on disk, no? > Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:-103 > - WebCore::NetworkStorageSession::defaultStorageSession().deleteCookie(cookie); Ditto. Doesn't this mean that in the common case (default session), the API can only delete cookies that were added via the API? Comment on attachment 340362 [details] Patch Attachment 340362 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/7682434 New failing tests: transitions/interrupted-transition-hardware.html Created attachment 340373 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Comment on attachment 340362 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340362&action=review > Source/WebKit/NetworkProcess/mac/RemoteNetworkingContext.mm:50 > + if (sessionID == PAL::SessionID::defaultSessionID()) { Why do we check for the default session here? Created attachment 340403 [details]
Patch
(In reply to Geoffrey Garen from comment #6) > Comment on attachment 340362 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=340362&action=review > > > Source/WebKit/NetworkProcess/mac/RemoteNetworkingContext.mm:50 > > + if (sessionID == PAL::SessionID::defaultSessionID()) { > > Why do we check for the default session here? It seems only the default session could be created without considering parameter pendingcookies, as it used uiProcessCookieStorageIdentifier to specify cookie storage to sync. Also in APIHTTPCookieStore, we only used pendingcookies param for sessions other than default session. (In reply to Chris Dumez from comment #3) > Comment on attachment 340362 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=340362&action=review > > I cannot comment about the overall approach but I have a few comments: > > > Source/WebKit/NetworkProcess/mac/RemoteNetworkingContext.mm:51 > > + auto* session = NetworkStorageSession::storageSession(sessionID); > > Double looking seems unfortunate here. Why not update the if above to look > like: > if (auto* session = NetworkStorageSession::storageSession(sessionID)) { > > > Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:-61 > > - allCookies = WebCore::NetworkStorageSession::defaultStorageSession().getAllCookies(); > > Doesn't this mean we're only returning the cookies added by the API for the > default session now? Previously, we'd ask the default storage session which > presumably could return cookies saved on disk, no? > Yes! I've updated the patch to still use default storage session to read cookies from persistent storage. Comment on attachment 340403 [details]
Patch
I don't understand the pending cookie design in general. It seems like pending cookies + processPoolForCookieStorageOperations() are an attempt to avoid launching the network process? But in what world do we expect our client to add cookies and then not do any networking?
I guess this is a workaround for the fact that the API client can create a WKWebsiteDatastore and a WKProcessPool separately, and pair arbitrary combinations of data store and process pool?
In addition to not understanding the design in general, I don't understand in particular why pending cookies used to apply only to the default session, or in particular why we now want pending cookies to apply only to the default session only if they are not session cookies.
I guess we need to discuss in person with Brady and/or Alex.
Comment on attachment 340403 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340403&action=review > Source/WebKit/NetworkProcess/mac/RemoteNetworkingContext.mm:49 > + if (auto* session = NetworkStorageSession::storageSession(sessionID)) { I think the UI process needs to clear pending cookies after it first sends them to the network process. Otherwise, if the cookies get deleted or overwritten, then a crash that reinitializes the network process will restore the pending cookies, which is not correct. Also, I think that clearing pending cookies is a better design because continuing to send them when they should not be set is very surprising. Once we make that change, we should also be able to change this code. We don't need any check, and instead we can unconditionally set pending cookies. If you like, you can also ASSERT (parameters.pendingCookies().isEmpty() || sessionID == PAL::SessionID::defaultSessionID()), which will verify this check I'm asking you to remove. > Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:80 > + if (m_owningDataStore->sessionID() == PAL::SessionID::defaultSessionID() && !cookie.session) It would be nice to file a Radar about the session cookie syncing issue in CFNetwork, and then comment that this session cookie check is an exploit workaround for that bug. Otherwise, the check seems out of the blue and unnecessary. I don't understand why non-default data store sessions skip using NetworkStorageSession's setCookie API. I'd like to understand that. > Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:101 > + if (m_owningDataStore->sessionID() == PAL::SessionID::defaultSessionID() && !cookie.session) Ditto. > Source/WebKit/UIProcess/WebProcessPool.cpp:520 > + if (m_websiteDataStore) > + m_networkProcess->send(Messages::NetworkProcess::AddWebsiteDataStore(m_websiteDataStore->websiteDataStore().parameters()), 0); Let's move this code down a little bit so it can be next to the other call to AddWebsiteDataStore. I don't understand why clients that specify 'withWebsiteDataStore' also get a separate m_websiteDataStore. I'd like to know why. > I don't understand why non-default data store sessions skip using
> NetworkStorageSession's setCookie API. I'd like to understand that.
I believe you should be able to write a test where you make a non-default data store, set a persistent cookie in it, release the data store, then make another non-default data store for the same path, and you'll find that your cookie did not get saved. That seems like a bug.
Created attachment 340441 [details]
Patch
(In reply to Geoffrey Garen from comment #11) > Comment on attachment 340403 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=340403&action=review > > > Source/WebKit/NetworkProcess/mac/RemoteNetworkingContext.mm:49 > > + if (auto* session = NetworkStorageSession::storageSession(sessionID)) { > > I think the UI process needs to clear pending cookies after it first sends > them to the network process. Otherwise, if the cookies get deleted or > overwritten, then a crash that reinitializes the network process will > restore the pending cookies, which is not correct. > > Also, I think that clearing pending cookies is a better design because > continuing to send them when they should not be set is very surprising. > > Once we make that change, we should also be able to change this code. We > don't need any check, and instead we can unconditionally set pending > cookies. If you like, you can also ASSERT > (parameters.pendingCookies().isEmpty() || sessionID == > PAL::SessionID::defaultSessionID()), which will verify this check I'm asking > you to remove. > Great advice. Adopted in the new patch. > > Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:80 > > + if (m_owningDataStore->sessionID() == PAL::SessionID::defaultSessionID() && !cookie.session) > > It would be nice to file a Radar about the session cookie syncing issue in > CFNetwork, and then comment that this session cookie check is an exploit > workaround for that bug. Otherwise, the check seems out of the blue and > unnecessary. > > I don't understand why non-default data store sessions skip using > NetworkStorageSession's setCookie API. I'd like to understand that. > Seems like a bug. Tracking in https://bugs.webkit.org/show_bug.cgi?id=185666. > > Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:101 > > + if (m_owningDataStore->sessionID() == PAL::SessionID::defaultSessionID() && !cookie.session) > > Ditto. > > > Source/WebKit/UIProcess/WebProcessPool.cpp:520 > > + if (m_websiteDataStore) > > + m_networkProcess->send(Messages::NetworkProcess::AddWebsiteDataStore(m_websiteDataStore->websiteDataStore().parameters()), 0); > > Let's move this code down a little bit so it can be next to the other call > to AddWebsiteDataStore. > > I don't understand why clients that specify 'withWebsiteDataStore' also get > a separate m_websiteDataStore. I'd like to know why. It seems 'withWebsiteDataStore' in ensureNetworkProcess is only used in WebsiteDataStore::fetchDataAndApply and WebsiteDataStore::removeData, where we want to add another webSiteDataStore to Network Process before fetching/removing data from it. And m_websiteDataStore is defaultDataStore. Comment on attachment 340441 [details] Patch Attachment 340441 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7697206 New failing tests: http/tests/preload/onload_event.html Created attachment 340484 [details]
Archive of layout-test-results from ews204 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 340441 [details]
Patch
This seems like an improvement, so I'll say r=me.
I think we need to follow up on the potential design problems that we discovered while working on this.
Comment on attachment 340441 [details] Patch Clearing flags on attachment: 340441 Committed r231850: <https://trac.webkit.org/changeset/231850> All reviewed patches have been landed. Closing bug. API test TestWebKitAPI.WebKit.WKHTTPCookieStoreWithoutProcessPool is flaky on the bots after this change. Here is the failure output: /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:379 Value of: message.UTF8String Actual: "cookie:SessionCookieName=CookieValue; PersistentCookieName=CookieValue" Expected: "cookie:PersistentCookieName=CookieValue; SessionCookieName=CookieValue" https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20(Tests)/builds/4580 Reopened as API test is flaky. Created attachment 340746 [details]
Patch
Comment on attachment 340746 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340746&action=review > Tools/ChangeLog:3 > + Session cookies aren't reliably set when using default WKWebSiteDataStore Title seems wrong (unrelated to this change). The cookies are set, merely returned in a different order, right? > Tools/ChangeLog:9 > + Modified test to set correct expection. typo: expection Also, I would clarify that you're making the API test robust to cookies being returned in a different order. Created attachment 340750 [details]
Patch
Comment on attachment 340750 [details]
Patch
cq+
Comment on attachment 340750 [details] Patch Clearing flags on attachment: 340750 Committed r231999: <https://trac.webkit.org/changeset/231999> All reviewed patches have been landed. Closing bug. |