Bug 185624 – Session cookies aren't reliably set when using default WKWebSiteDataStore

• Component: WebKit API
• Status: RESOLVED FIXED

[Sihui Liu]

We could set session cookies through default WKWebSiteDataStore, but we may not get those cookies reliably after creating a WebView.

[Sihui Liu]

<rdar://problem/39111626>

[Sihui Liu]

Created attachment 340362 [details]
Patch

[Chris Dumez]

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?

[EWS Watchlist]

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

[EWS Watchlist]

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

[Geoffrey Garen]

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?

[Sihui Liu]

Created attachment 340403 [details]
Patch

[Sihui Liu]

(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.

[Sihui Liu]

(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.

[Geoffrey Garen]

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.

[Geoffrey Garen]

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.

[Geoffrey Garen]

> 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.

[Sihui Liu]

Created attachment 340441 [details]
Patch

[Sihui Liu]

(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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Geoffrey Garen]

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.

[WebKit Commit Bot]

Comment on attachment 340441 [details]
Patch

Clearing flags on attachment: 340441

Committed r231850: <https://trac.webkit.org/changeset/231850>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dawei Fenton (:realdawei)]

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

[Sihui Liu]

Reopened as API test is flaky.

[Sihui Liu]

Created attachment 340746 [details]
Patch

[Chris Dumez]

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.

[Sihui Liu]

Created attachment 340750 [details]
Patch

[Geoffrey Garen]

Comment on attachment 340750 [details]
Patch

cq+

[WebKit Commit Bot]

Comment on attachment 340750 [details]
Patch

Clearing flags on attachment: 340750

Committed r231999: <https://trac.webkit.org/changeset/231999>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.