536650 - Cookie values not saved for pages loaded from file://

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[William J. Edney]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2b5) Gecko/20091204 Firefox/3.6b5
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2b5) Gecko/20091204 Firefox/3.6b5

As the attached demonstration file shows, cookie values do not persist when a page is loaded using a file:// URL.

This behavior is a major regression from all previous versions of Mozilla/Firefox and has only started appearing in Firefox 3.6 beta 5.

Reproducible: Always

Steps to Reproduce:
1. Load the file from the filesystem (i.e. from a file:// URL). It will try to set a cookie value when it loads.
2. Click the button. It will alert the empty String.
3. Load the file from an http:// URL.
4. Click the button. It will alert a real value.
Actual Results:  
When loaded from a file:// URL, the cookie value is empty.

Expected Results:  
When loaded from a file:// URL, the cookie value has a real value.

Comment 1

[William J. Edney]

Attachment: A file demonstrating the problem

Comment 2

[Ria Klaassen (not reading all bugmail)]

Confirmed with latest trunk on Windows Vista.

Regression range is: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1d07eff12995&tochange=d6863ea5c26a

Comment 3

[[e-mail]]

Yes, this is a result of bug 526789. Disallowing empty hosts was deliberate in that bug. However, from abarth's testing: "IE8, Firefox 3.5, Safari 4, and Opera 10 support file:// URL cookies.  Chrome 3 does not.  (Of course, IE8 requires the user to click through an infobar to run javascript on a file URL)". Since most of the other browsers support it, we should allow them.

We'll need different patches for trunk and branch to do this, since stuff has landed on trunk since. I'll roll some patches.

Comment 4

[[e-mail]]

Why do you want cookies with file:// URI's, by the way? And why not set up a local webserver if you're using it to test things?

Comment 5

[William J. Edney]

Dan -

A couple of points:

Firstly, Chrome (all versions) support filesystem cookies when run with the --enable-file-cookies command line flag. The fact that the Chrome team has chosen this direction has caused *much* gnashing of teeth for many developers who have been developing filesystem-based web-technology apps for years and is not a happy situation at all. It is currently keeping me from recommending that browser to my customers.

Secondly, I (along with many others) have been developing 'desktop apps' (if you will) using "web technologies" with nary an 'http://' URL in sight for over 10 years. In a sense, using a Web browser as a replacement for VB or Delphi. Our customers love this and don't want to have to run a local webserver on each individual machine that they deploy to. The advent of technologies like Mozilla Prism means that running 'local file system based web apps' will only increase, not decrease.

Therefore, while I personally do run my development environment off of both a local webserver and the local file system, my customers will only be running off of the local file system and I tend to test heavily 'booting' my web app and using features of the browser when running off of 'file://' URLs. I have found issues with local storage, cross-origin URLs, and a few other 'new' technologies (which I have duly filed bugs for), but was a bit surprised when cookies stopped working on the latest FF 3.6 beta, since cookies have been around for so long.

In any case, thanks so much for working on this.

Cheers,

- Bill

Comment 6

[Adam Barth]

How do you handle the case of two of these local web application both using the cookie store?  It seems like they would cross-talk significantly.

Comment 7

[William J. Edney]

This is solved pretty easily. Our library (which does all of the 'low-level' cookie handling, if you will) requires that a unique 'appName' is defined for each individual application.

When it goes to store the cookie, it then uniques the "key" used by prepending it with our library's name, the specific app's name and then the key:

<libraryName> + '.' + <appName> + '.' + key

The library handles both cookie reading and writing, so the app developer using our library never sees this.

We feel that this provides enough uniquing so that the risk of collisions is quite low.

Comment 8

[[e-mail]]

Thanks for the detailed explanation. I generally dislike the idea of hostless cookies, because it's subverting the primary established mechanism for compartmentalizing them. But other cross-domain enforcement methods will fail in that case too; such attacks would be hard to execute; and we've never heard reports of it.

So, given that there's a good use case for them we should explicitly allow them and get test coverage so we don't break again.

Comment 9

[William J. Edney]

Dan -

That's wonderful - thanks!

I consider this a 'blocker' for the next release, but I'm not familiar enough with Mozilla procedures to be presumptuous and use any of the flag pulldowns above to mark it as such.

Do you concur and if so, would you feel comfortable marking it as a blocker?

Thanks again!

Cheers,

- Bill

Comment 10

[[e-mail]]

Attachment: branch patch v1

This reverts the overzealous checking for empty hosts, and explicitly allows them for file:// URI's. We still want to prevent hosts with a trailing '.' (including the '.' host itself) from getting into the DB - because then if we're asked to look up a host with a trailing '.', our last lookup will be for '.' itself, which could match something. (And it's more conservative, in terms of preserving behavior on branch, to prevent setting such cookies rather than looking them up. For trunk, we can be more strict.)

Getting this patch out first since 1.9.1 and 1.9.2 are more urgent.

Comment 11

[[e-mail]]

Comment on attachment 419177 [details] [diff] [review]
branch patch v1

Requesting approval for a regression fix. This partially reverts, and adds more tests for, a recent landing on 1.9.1 and 1.9.2 branches that disables cookies for file:// URI's. We should get this on both branches ASAP - if possible, for 1.9.2 itself.

Comment 12

[[e-mail]]

Passes tryserver tests. Bill, there are builds with this patch available at https://build.mozilla.org/tryserver-builds/[e-mail]-try-7c744b6b5a04/ ... if you're so inclined, could you please download and test? Having confirmation from you that this works as expected would help, since we're very close to release.

Comment 13

[William J. Edney]

Dan -

Downloaded the build and tested. My stuff is back to working again :-).

A Christmas Day patch... I'm impressed :-).

Thanks so very much!

Let me know if there is anything else I can do on my end (more testing, if required, etc.) to get this approved for both branches and trunk.

Cheers,

- Bill

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 419177 [details] [diff] [review]
branch patch v1

a192=beltzner

Comment 15

[[e-mail]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/913a1606045b

Comment 16

[[e-mail]]

Attachment: trunk patch v1 - part 1

This makes the eTLD service more strict on what it'll take. It now rejects hosts with a leading dot, embedded '..' sequences, and the edge case '.' as invalid arguments; while accepting the empty string '' as a special case and returning NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS.

It's convenient to enforce this here, because this is where host string parsing takes place, and the host string has to make sense.

Comment 17

[[e-mail]]

Attachment: trunk patch v1 - part 2

This makes the cookieservice deal with non-domainable hosts (IP addresses, aliases such as 'localhost', eTLD's such as 'co.uk', and the empty host '') as follows:

a) Specify explicitly what nsICookieManager{2}::Remove, Add, CountCookiesFromHost, and GetCookiesFromHost will accept. Rather than expecting a string that abides by the cookieservice's internal standards, which is dangerous in the case of Add, it now performs normalization on the string. If the string has a leading '.' but is non-domainable, they throw.

b) For the methods that take an nsIURI ({Get,Set}CookieString*), it's expected that the URI is valid and thus a leading dot is not accepted. The empty host string '' is only accepted if the URI is a file:// scheme.

c) The 'domain=' attribute will accept the non-domainable cases, but downgrade the cookie to a non-domain one. This only really changes behavior for the empty host case, since the other cases would never get matched in domain cookie lookups anyway.

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 419177 [details] [diff] [review]
branch patch v1

Approved for 1.9.1.8, a=dveditz

Comment 19

[[e-mail]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/74a9c37bc887

Comment 20

[Shawn Wilsher :sdwilsh]

Comment on attachment 419711 [details] [diff] [review]
trunk patch v1 - part 1

r=sdwilsh

Technically, this is an API change, and therefore needs sr.  Asking bz to take a look, but maybe someone else would be better.

Comment 21

[Shawn Wilsher :sdwilsh]

Comment on attachment 419716 [details] [diff] [review]
trunk patch v1 - part 2

For review comments with full context, please see http://reviews.visophyte.org/r/419716/.

on file: extensions/cookie/test/unit/test_bug526789.js line 133
>   emptyuri = ios.newURI("file:///", null, null);
>   do_check_eq(emptyuri.asciiHost, "");
>   do_check_eq(ios.newURI("file://./", null, null).asciiHost, "");
>   do_check_eq(ios.newURI("file://foo.bar/", null, null).asciiHost, "");

nit: NetUtil.newURI.  You won't have to pass the nulls in then too.


on file: extensions/cookie/test/unit/test_bug526789.js line 215
>   var uri = ios.newURI(uriString, null, null);

nit: NetUtil.newURI


This also requires sr due to API change.

r=sdwilsh

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 419711 [details] [diff] [review]
trunk patch v1 - part 1

This looks fine.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 419716 [details] [diff] [review]
trunk patch v1 - part 2

> +    aHostURI->SchemeIs("file", &isFileURI);

Why is this the right check?  What about other non-authority schemes?  I almost with nsIStandardURL exposed the authority flag; maybe we should do that instead?  Or do we really really want to limit it to file:// and not allow protocol handlers to control this?

>+    nsresult rv = mIDNService->ConvertUTF8toACE(aHost, aHost);

This sort of thing always worries me.  What if the callee writes to the out param before being done with the in param?  Please make a copy for the in param here.

Other than those issues, looks ok.

Comment 24

[Daniel Veditz [:dveditz]]

(In reply to comment #23)
> Why is this the right check?  What about other non-authority schemes?

If we're contemplating this at all we need to save something other than an empty host. It's bad enough all file: cookies are shared, we don't need file: and some new thing to share data.

Comment 25

[[e-mail]]

(In reply to comment #24)
> If we're contemplating this at all we need to save something other than an
> empty host. It's bad enough all file: cookies are shared, we don't need file:
> and some new thing to share data.

Agreed. If we must have cookies with an empty host, we really don't want to encourage their use, so having them work with just file:// is fine by me.

(In reply to comment #23)
> >+    nsresult rv = mIDNService->ConvertUTF8toACE(aHost, aHost);
> 
> This sort of thing always worries me.  What if the callee writes to the out
> param before being done with the in param?  Please make a copy for the in param
> here.

Sure, will fix. (In this case it's fine, the callee makes a copy itself, but agree that it's fragile.)

> Other than those issues, looks ok.

Great, thanks. Did you mean to mark sr+ or you'd like to see another patch?

Comment 26

[[e-mail]]

Attachment: trunk patch v1.1 - part 2

Updated per sdwilsh's and bz's comments.

Comment 27

[Boris Zbarsky [:bzbarsky]]

I'd still like an answer to my questions from comment 23.

Comment 28

[Boris Zbarsky [:bzbarsky]]

> It's bad enough all file: cookies are shared, we don't need file:
> and some new thing to share data.

OK.  That's a good reason... now can you please document that in the code?  ;)

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 421079 [details] [diff] [review]
trunk patch v1.1 - part 2

sr=me with those comments added.

Comment 30

[[e-mail]]

http://hg.mozilla.org/mozilla-central/rev/e63c419af667
http://hg.mozilla.org/mozilla-central/rev/21362ffb3b04

Comment 31

[[e-mail]]

Comment on attachment 421079 [details] [diff] [review]
trunk patch v1.1 - part 2

>+nsresult
> nsCookieService::GetBaseDomainFromHost(const nsACString &aHost,
>                                        nsCString        &aBaseDomain)
> {
>+  // aHost must not contain a trailing dot, or be the string '.'.
>+  if (!aHost.IsEmpty() && aHost.Last() == '.')
>     return NS_ERROR_INVALID_ARG;
> 
>+  // aHost may contain a leading dot; if so, strip it now.
>+  nsDependentCString host(aHost);
Why do we even allow constructing an nsDependentCString from an nsACString?