Bug 129946 - Regression: Session Cookies dropped from Application Cache (Appcache) Manifest Request
Summary: Regression: Session Cookies dropped from Application Cache (Appcache) Manifes...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.9
: P2 Normal
Assignee: Vicki Pfau
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-03-07 17:36 PST by ray
Modified: 2014-06-12 12:59 PDT (History)
11 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description ray 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.
Comment 1 Alexey Proskuryakov 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.
Comment 2 ray 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.
Comment 3 gbaker 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
Comment 4 Ian 'Hixie' Hickson 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.
Comment 5 Alexey Proskuryakov 2014-04-14 16:28:41 PDT
<rdar://problem/16570516>
Comment 6 Dev 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?
Comment 7 Alexey Proskuryakov 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.
Comment 8 Dev 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.
Comment 9 schmidty 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.
Comment 10 Alexey Proskuryakov 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.
Comment 11 Dev 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!
Comment 12 Vicki Pfau 2014-06-09 17:57:24 PDT
Created attachment 232753 [details]
Patch
Comment 13 Alexey Proskuryakov 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?
Comment 14 Build Bot 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
Comment 15 Build Bot 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
Comment 16 Vicki Pfau 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.
Comment 17 Alexey Proskuryakov 2014-06-09 23:18:49 PDT
I wonder how this happens. Perhaps CookieStorageShim works on Yosemite for this?
Comment 18 Vicki Pfau 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.
Comment 19 Alexey Proskuryakov 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.
Comment 20 Alexey Proskuryakov 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).
Comment 21 Vicki Pfau 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.