407839 - restrict globalStorage to same host

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[James Justin Harrell]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.11) Gecko/20071204 Ubuntu/7.10 (gutsy) Firefox/2.0.0.11
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b1) Gecko/2007110903 Firefox/3.0b1

The HTML5 globalStorage attribute now refers directly to a Storage object of the current origin, instead of a StorageList like it previously did. This is a very important change that really needs to get into Firefox 3.

The sooner this is changed, the lesser the impact on web developers.


Reproducible: Always

Steps to Reproduce:
Evaluate window.globalStorage

Actual Results:  
A StorageList object is returned.


Expected Results:  
A Storage object is returned.


http://www.whatwg.org/specs/web-apps/current-work/#the-globalstorage
http://www.whatwg.org/specs/web-apps/current-work/#the-default0

Comment 1

[Daniel Veditz [:dveditz]]

I know we've talked about this but I couldn't find an existing bug so we might as well confirm this one and use it.

Issues: 
1) how do we migrate data saved by web pages using FF2? I believe we're storing the actual source hostname for quota enforcement in addition to the named domain, although I don't know if that exists in FF2 or was added after. If it exists in FF2 then we can use that as the new key.

1a) if we do that it's possible a site used the same value name under two differnet namespaces, we'd have to decide which one wins.

2) do we continue to support the old syntax for those pages, or simply break and force them to adapt. I recommend simply breaking them otherwise we'll have to support the old syntax forever. web pages can do

 function getStorage(domain) {
    return (globalStorage instanceof Storage)
              ? globalStorage          // Firefox 3
              : globalStorage[domain]; // Firefox 2
 }

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Blocking on figuring out what type of fix we want here (change spec, change our impl, do nothing)

Comment 3

[Dave Camp (:dcamp)]

(In reply to comment #1)
> I know we've talked about this but I couldn't find an existing bug so we might
> as well confirm this one and use it.
> 
> Issues: 
> 1) how do we migrate data saved by web pages using FF2? I believe we're storing
> the actual source hostname for quota enforcement in addition to the named
> domain, although I don't know if that exists in FF2 or was added after. If it
> exists in FF2 then we can use that as the new key.

As of 2.0.0.2, the source hostname is stored in the db.  When upgrading from 2.0.0.1, the store hostname is used as the source hostname.

Comment 4

[Dave Camp (:dcamp)]

More recently, the spec has changed globalStorage -> localStorage, so we can leave globalStorage around for compatibility.

I think it would be reasonable to restrict globalStorage usage to same-domain, so that globalStorage[window.location.host] would be equivalent to localStorage, and globalStorage["not.window.location.host"] would throw a security error.

I think we should make that globalStorage change even if the localStorage change can't make it for ffx3, to prevent new developers from starting to rely on the different-domain semantics.

Comment 5

[Dave Camp (:dcamp)]

Updating the title to reflect changes discussed in comment 4.

Comment 6

[Dave Camp (:dcamp)]

Filed bug 422526 for implementing localStorage.

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: Fix

Comment 8

[[e-mail]]

semi-related: http://mxr.mozilla.org/mozilla/source/dom/src/storage/nsDOMStorage.cpp#1145

is there a bug on using eTLD there now? fixing that would be trivial...

Comment 9

[Dave Camp (:dcamp)]

Attachment: normalize requested hostname, some unit tests

Honza, sorry for the mid-air collision here, I was working on this patch last night.

This patch is the same as Honza's, but with a few additions:

* It normalizes the requested hostname before trying to string compare it with the current hostname.  This solves problems with mismatched case, and also IDN issues.

* Unit tests.

Comment 10

[Dave Camp (:dcamp)]

Oh, I did have one question;  I couldn't figure out why the "localdomain" was added to the requested hostname in CanAccessStorage().  I took it out because the previous check in GetStorageForDomain() should be rejecting anything without a period, so I don't think that case was ever hit.

Also, maybe we can just remove the check at the beginning of GetStorageForDomain() - you can no longer access a TLD's storage space anyway unless you are actually served from the TLD.

Comment 11

[Dave Camp (:dcamp)]

Marking late-compat, as this would affect anyone trying to use globalStorage for a domain other than the current domain.

Comment 12

[Dave Camp (:dcamp)]

Attachment: new version

This version gets rid of the tld check as mentioned in comment 10, and does the hostname normalization using the nsIIDNService instead of creating a bogus nsIURI object.

Comment 13

[[e-mail]]

Comment on attachment 309256 [details] [diff] [review]
new version

>diff -r 67e943d1fe15 dom/src/storage/nsDOMStorage.cpp
>+  nsCOMPtr<nsIIDNService> idn = do_GetService(NS_IDNSERVICE_CONTRACTID);
>+  if (idn) {
...
>+  } else {
>+    // Don't have the IDN service, best we can do is URL escape.

shouldn't we always have an idnservice? can we just fail if (!idn)?

Comment 14

[Dave Camp (:dcamp)]

(In reply to comment #13)
> (From update of attachment 309256 [details] [diff] [review])
> >diff -r 67e943d1fe15 dom/src/storage/nsDOMStorage.cpp
> >+  nsCOMPtr<nsIIDNService> idn = do_GetService(NS_IDNSERVICE_CONTRACTID);
> >+  if (idn) {
> ...
> >+  } else {
> >+    // Don't have the IDN service, best we can do is URL escape.
> 
> shouldn't we always have an idnservice? can we just fail if (!idn)?

nsStandardURL deals with a missing IDN service, but maybe things have changed since it was written?

Comment 15

[Neil Deakin]

Comment on attachment 309256 [details] [diff] [review]
new version

+function run()
+{
+  // This script expects to be loaded as test1.example.org

Does the test framework ensure this somehow? Same with the child frame.

+  try {
+    storage = globalStorage["test1.EXAMPLE.ORG"];
+  }
+  catch (ex) {
+    message += "\n failed globalStorage[\"test1.example.org\"]";

Should use the same case here so its easier to debug failures.

Comment 16

[Dave Camp (:dcamp)]

(In reply to comment #15)
> (From update of attachment 309256 [details] [diff] [review])
> +function run()
> +{
> +  // This script expects to be loaded as test1.example.org
> 
> Does the test framework ensure this somehow? Same with the child frame.

The main test loads the two iframes with the hostnames they expect, but I'll add a test to make sure they are.

> 
> +  try {
> +    storage = globalStorage["test1.EXAMPLE.ORG"];
> +  }
> +  catch (ex) {
> +    message += "\n failed globalStorage[\"test1.example.org\"]";
> 
> Should use the same case here so its easier to debug failures.

Indeed, thanks.

Comment 17

[Dave Camp (:dcamp)]

Attachment: fixed for review comments

I'm asking for approval since this is late-compat:

This changes the implementation of globalStorage to only allow access to same-host storage.  The HTML5 spec has removed globalStorage in favor of localStorage, which doesn't allow access to another hostname's storage area.  While it's too late to implement this spec change for 1.9, restricting globalStorage will help ease the transition to localStorage and prevent any unintended inter-site communication (a motivating factor in the spec change).

This will break any existing website that is currently trying to write to or read from a parent/subdomain's storage area.  Before making the spec, Hixie wasn't able to find any use of this capability in the wild.

We're the only shipping browser with globalStorage support, although the IE8 preview supports it.  I don't know offhand if IE8 is planning to track the spec and replace globalStorage with localStorage for their final release.

Comment 18

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

This'll need some doc love if it goes in.

Comment 19

[Damon Sicore (:damons)]

Comment on attachment 310081 [details] [diff] [review]
fixed for review comments

It's a blocker.  Don't need approval.  Cleared flag.

Comment 20

[Dave Camp (:dcamp)]

Attachment: fixed for review comments (right patch this time)

Comment 21

[Dave Camp (:dcamp)]

Checking in src/storage/nsDOMStorage.cpp;
/cvsroot/mozilla/dom/src/storage/nsDOMStorage.cpp,v  <--  nsDOMStorage.cpp
new revision: 1.25; previous revision: 1.24
done
Checking in tests/mochitest/bugs/Makefile.in;
/cvsroot/mozilla/dom/tests/mochitest/bugs/Makefile.in,v  <--  Makefile.in
new revision: 1.24; previous revision: 1.23
done
RCS file: /cvsroot/mozilla/dom/tests/mochitest/bugs/iframe_bug407839-1.html,v
done
Checking in tests/mochitest/bugs/iframe_bug407839-1.html;
/cvsroot/mozilla/dom/tests/mochitest/bugs/iframe_bug407839-1.html,v  <--  iframe_bug407839-1.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/dom/tests/mochitest/bugs/iframe_bug407839-2.html,v
done
Checking in tests/mochitest/bugs/iframe_bug407839-2.html;
/cvsroot/mozilla/dom/tests/mochitest/bugs/iframe_bug407839-2.html,v  <--  iframe_bug407839-2.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/dom/tests/mochitest/bugs/test_bug407839.html,v
done
Checking in tests/mochitest/bugs/test_bug407839.html;
/cvsroot/mozilla/dom/tests/mochitest/bugs/test_bug407839.html,v  <--  test_bug407839.html
initial revision: 1.1
done

Comment 22

[Eric Shepherd [:sheppy]]

The documentation at

http://developer.mozilla.org/en/docs/DOM:Storage#globalStorage

Has been updated.  Please let me know of any issues with it.