Bug 129946

Summary:

Regression: Session Cookies dropped from Application Cache (Appcache) Manifest Request

Product:

WebKit

Reporter:

ray <mihir.ray>

Component:

WebCore Misc.

Assignee:

Vicki Pfau <jeffrey+webkit>

Status:

NEW

Severity:

Normal

CC:

ap, buildbot, commit-queue, dbates, devdoshi, foobar22, ian, japhet, jeffrey+webkit, rniwa, schmidtyfi

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

OS X 10.9

Attachments:

Description	Flags
Contains master page appcache.html. This page will load an iframe which installs application cache.	none
Patch	ap: review-, buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2	none

ray

Reported 2014-03-07 17:36:42 PST

Created attachment 226191 [details] Contains master page appcache.html. This page will load an iframe which installs application cache. Starting Safari 7.0.2 the Session Cookies are not included in the appcache manifest request. Only cookies with expiry date or cookies which are marked as HttpOnly are added to manifest request. Steps to reproduce: 1. Create manifest file eg. test.manifest CACHE MANIFEST # v. 1 CACHE: appcache.html NETWORK: * 2. Create a manifest loader file eg. manifestloader.html points to manifest. <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd"> <html manifest="test.manifest"> <head></head><body></body> </html> 3. Inside appcache.html add code to do below steps: • On page load add two cookies • First cookie for root path (session cookie) • Second cookie for root path with expiry date set to today + 4 days • Add code to load an iframe whose src is set to manifestloader.html. • When iframe loads it will request test.manifest. Expected Results: Check the test.manifest request in n/w trace in safari or fiddler. It should have two cookies on the request. Actual Results: The manifest request (test.manifest) only has one cookie present on request. This is the second cookie with expiry date set. The session cookie is missing.

Attachments
Contains master page appcache.html. This page will load an iframe which installs application cache. (73.94 KB, application/x-zip-compressed) 2014-03-07 17:36 PST, ray	no flags	Details
Patch (9.02 KB, patch) 2014-06-09 17:57 PDT, Vicki Pfau	ap: review- buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (742.62 KB, application/zip) 2014-06-09 19:42 PDT, Build Bot	no flags	Details
View All Add attachment proposed patch, testcase, etc.

Alexey Proskuryakov

Comment 1 2014-03-12 10:00:35 PDT

This sounds like correct behavior to me - we shouldn't be storing content that's specific to a particular login session persistently. The user could log in to a different account next time. Furthermore, we shouldn't be sending persistent cookies or credentials either, it's a bug if we do.

ray

Comment 2 2014-03-12 15:49:49 PDT

(In reply to comment #1) > This sounds like correct behavior to me - we shouldn't be storing content that's specific to a particular > login session persistently. The user could log in to a different account next time. > > Furthermore, we shouldn't be sending persistent cookies or credentials either, it's a bug if we do. Are you saying manifests cannot be dependent on session cookies (or any cookies)? There may be merit to this argument, but I do not see anything in the standard to suggest it (?). In fact, I think there are very good reasons other browsers do not do things that way (including prior versions of Safari!). Consider forms-based authentication mechanisms, which generate an authentication token in the form of a cookie. By not sending this token with the manifest request, you are effectively demanding anonymous access to the manifest resource, are you not? As a second example, imagine a web site where 99% of traffic represents repeat visits from the same user from the same user agent. It seems entirely inappropriate for the browser to prevent the site's developers from optimizing this case and gracefully degrading the experience (out of the app-cache) for the 10% case. There are many other scenarios. This appears to be a serious regression in Safari 7.

gbaker

Comment 3 2014-03-28 19:54:17 PDT

If SessionCookie is not sent to the server then how is anyone supposed to use AppCache for authenticated users? The assumption that you are making is that the page in AppCache is landing page, sadly for my case that's not the case. Here is our login flow looks like. 1. If User is not authenticated: Navigate to http://FooBar.com -> 302 to auth page -> after auth 302 to http://FooBar.com/userName (and this page is in AppCache) -> Use window.history.replaceState({}, "Title", "/") 2. If User is already authenticated Navigate to http://FooBar.com -> 302 to http://FooBar.com/userName (and this page is in AppCache) -> Use window.history.replaceState({}, "Title", "/") so that to user URL does not contain userName. BTW, even if user loads http://FooBar.com, we do verify that he/she is logged in so there is no real issue of un-intended access (after all any data access will require authentication). But now this flow has broken for us. It's only Safari that's broken, all other browsers work just fine. If you think sending SessionCookie is wrong, then please work with Standards committee to change this behavior, but plesae-plesae do not come up with interpretation of the standard that's different from other browsers. Thanks

Ian 'Hixie' Hickson

Comment 4 2014-04-07 09:40:36 PDT

I don't see anything in the spec that would require dropping cookies or HTTP auth info, FWIW.

Alexey Proskuryakov

Comment 5 2014-04-14 16:28:41 PDT

<rdar://problem/16570516>

Dev

Comment 6 2014-05-16 14:22:14 PDT

This issue has been affecting my commercial application for a few months now. Desktop Safari is the only browser that does not work as expected; iOS Safari(!) and Chrome/Chrome for Android/IE/Firefox all behave as expected. I have filed a bug report a while ago through the Apple Developer site, but I did not receive any response. My manifests include resources that require authentication (e.g. API requests), so the appcache will get 401s from my API for the requests it makes without the expected session information. My understanding of the spec and its intended uses makes me think this behavior is incorrect. Is there any update on the status of this issue?

Alexey Proskuryakov

Comment 7 2014-05-16 15:52:04 PDT

> appcache will get 401s from my API for the requests it makes without the expected session information That sounds like a different issue. This bug as reported is about HTTP cookies, not about HTTP authentication.

Dev

Comment 8 2014-05-17 15:59:51 PDT

(In reply to comment #7) > > appcache will get 401s from my API for the requests it makes without the expected session information > > That sounds like a different issue. This bug as reported is about HTTP cookies, not about HTTP authentication. I'm using a signed HTTP cookie for authentication, so I believe it is the same issue. My API returns a 401 status code for requests that require authentication but do not provide any valid authentication cookie. I've set up logging and verified that the appcache requests use a different session and don't have the required cookie. I've also replicated this with vanilla PHP sessions, which use a cookie with a session ID.

schmidty

Comment 9 2014-05-30 10:36:31 PDT

Alexey Proskuryakov

Comment 10 2014-05-30 10:44:19 PDT

The comment about HTTP auth was specifically in response to comment 6, which mentioned 401 errors (401 is an HTTP error code that's returned when HTTP authentication is required). Dev has since clarified that they use this error code for a different purpose in their application. This bug tracks cookies and not HTTP authentication, no need to discuss that any further. Please see <http://tools.ietf.org/html/rfc2617> for further details.

Dev

Comment 11 2014-05-30 12:35:13 PDT

Is there any update on the status of this issue? I would greatly appreciate knowing if/when/how this issue will be resolved. Thanks!

Vicki Pfau

Comment 12 2014-06-09 17:57:24 PDT

Created attachment 232753 [details] Patch

Alexey Proskuryakov

Comment 13 2014-06-09 18:58:09 PDT

Comment on attachment 232753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232753&action=review > Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1609 > +#if ENABLE(NETWORK_PROCESS) && PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED <= 1090 Quick question first: what about Yosemite, does it somehow already work?

Build Bot

Comment 14 2014-06-09 19:42:50 PDT

Comment on attachment 232753 [details] Patch Attachment 232753 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5772407440670720 New failing tests: media/W3C/video/preload/preload_property_exists.html

Build Bot

Comment 15 2014-06-09 19:42:54 PDT

Created attachment 232762 [details] Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Vicki Pfau

Comment 16 2014-06-09 19:45:07 PDT

(In reply to comment #13) > (From update of attachment 232753 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=232753&action=review > > > Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1609 > > +#if ENABLE(NETWORK_PROCESS) && PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED <= 1090 > > Quick question first: what about Yosemite, does it somehow already work? Yes, on Yosemite, we seem to share the cookie session between the network process and the web process already. I wasn't able to find where this happened, but I was able to verify that the bug was not present before this patch on Yosemite, regardless.

Alexey Proskuryakov

Comment 17 2014-06-09 23:18:49 PDT

I wonder how this happens. Perhaps CookieStorageShim works on Yosemite for this?

Vicki Pfau

Comment 18 2014-06-10 17:33:20 PDT

(In reply to comment #17) > I wonder how this happens. Perhaps CookieStorageShim works on Yosemite for this? After prodding at this quite a bit, it looks like we go through the WKNSURLSessionLocal cookie shim on Yosemite. I'm not quite sure why we aren't on Mavericks, since the shim is still installed. It might be worth looking at that a bit deeper.

Alexey Proskuryakov

Comment 19 2014-06-10 23:17:52 PDT

I suspect that internals of CFNetwork have changed, so the same functions are not being called. If it's not too hard, I would prefer a solution that works roughly the same on all platforms - it's not cool to inject cookies inside CFNetwork on some platforms, and via setHTTPHeaderField on others. This could have unexpected compatibility or security consequences. I'd start with asking Jer, who might have insight into the possible differences.

Alexey Proskuryakov

Comment 20 2014-06-12 09:11:06 PDT

Comment on attachment 232753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232753&action=review r=me on the test. Can you land it and only enable on Yosemite and later? > Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:496 > + m_frame->loader().client().cookieStorageCopyRequestHeaderFields(request); As discussed in person, this is not reliable - CFNetwork would overwrite the header if it has any cookies (such as from a preceding appcache update, or from a ping request).

Vicki Pfau

Comment 21 2014-06-12 12:59:20 PDT

(In reply to comment #20) > (From update of attachment 232753 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=232753&action=review > > r=me on the test. Can you land it and only enable on Yosemite and later? > > > Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:496 > > + m_frame->loader().client().cookieStorageCopyRequestHeaderFields(request); > > As discussed in person, this is not reliable - CFNetwork would overwrite the header if it has any cookies (such as from a preceding appcache update, or from a ping request). There must have been a misunderstanding, then. CFNetwork does not override the header if it's already present. However, this does mean that cookies that are set during the app cache load would be thrown by the wayside.

Note You need to log in before you can comment on or make changes to this bug.