| 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: | P2 | Keywords: | InRadar | ||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Mac | ||||||||||
| OS: | OS X 10.9 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
ray
2014-03-07 17:36:42 PST
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. (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. 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 I don't see anything in the spec that would require dropping cookies or HTTP auth info, FWIW. 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? > 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.
(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. (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. 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. Is there any update on the status of this issue? I would greatly appreciate knowing if/when/how this issue will be resolved. Thanks! Created attachment 232753 [details]
Patch
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? 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 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
(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. I wonder how this happens. Perhaps CookieStorageShim works on Yosemite for this? (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. 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. 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). (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. |