NEW 129946
Regression: Session Cookies dropped from Application Cache (Appcache) Manifest Request 
https://bugs.webkit.org/show_bug.cgi?id=129946
Summary Regression: Session Cookies dropped from Application Cache (Appcache) Manifes...
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-
DetailsFormatted DiffDiff
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
(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. This *is* about HTTP authentication, because by not sending session cookies, you are completely breaking cookie auth to the manifest file.
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.