restrict globalStorage to same host

RESOLVED FIXED

Status

()

Core
DOM
P2
major
RESOLVED FIXED
10 years ago
4 years ago

People

(Reporter: James Justin Harrell, Assigned: dcamp)

Tracking

({addon-compat, dev-doc-complete})

unspecified
addon-compat, dev-doc-complete
Points:
---
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

10 years ago
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
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
 }
Status: UNCONFIRMED → NEW
Component: DOM → DOM: Mozilla Extensions
Ever confirmed: true
Flags: blocking1.9?
Blocking on figuring out what type of fix we want here (change spec, change our impl, do nothing)
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
Assignee: nobody → dcamp
(Assignee)

Comment 3

9 years ago
(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.
(Assignee)

Comment 4

9 years ago
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.

Updated

9 years ago
Flags: tracking1.9+ → blocking1.9+
Priority: P3 → P2
(Assignee)

Comment 5

9 years ago
Updating the title to reflect changes discussed in comment 4.
Summary: globalStorage property should refer directly to a Storage object → restrict globalStorage to same host
(Assignee)

Comment 6

9 years ago
Filed bug 422526 for implementing localStorage.
Created attachment 309113 [details] [diff] [review]
Fix
Attachment #309113 - Flags: review?(dcamp)

Comment 8

9 years ago
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...
(Assignee)

Comment 9

9 years ago
Created attachment 309158 [details] [diff] [review]
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.
Attachment #309113 - Attachment is obsolete: true
Attachment #309158 - Flags: review?(enndeakin)
Attachment #309113 - Flags: review?(dcamp)
(Assignee)

Updated

9 years ago
Attachment #309158 - Attachment is patch: true
Attachment #309158 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 10

9 years ago
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.
(Assignee)

Comment 11

9 years ago
Marking late-compat, as this would affect anyone trying to use globalStorage for a domain other than the current domain.
Keywords: late-compat
(Assignee)

Comment 12

9 years ago
Created attachment 309256 [details] [diff] [review]
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.
Attachment #309158 - Attachment is obsolete: true
Attachment #309256 - Flags: review?(enndeakin)
Attachment #309158 - Flags: review?(enndeakin)

Comment 13

9 years ago
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)?
(Assignee)

Comment 14

9 years ago
(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

9 years ago
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.
Attachment #309256 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 16

9 years ago
(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.

(Assignee)

Updated

9 years ago
Attachment #309256 - Flags: superreview?(jst)

Updated

9 years ago
Attachment #309256 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 17

9 years ago
Created attachment 310081 [details] [diff] [review]
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.
Attachment #310081 - Flags: approval1.9?
This'll need some doc love if it goes in.
Keywords: dev-doc-needed
Comment on attachment 310081 [details] [diff] [review]
fixed for review comments

It's a blocker.  Don't need approval.  Cleared flag.
Attachment #310081 - Flags: approval1.9?
(Assignee)

Comment 20

9 years ago
Created attachment 310269 [details] [diff] [review]
fixed for review comments (right patch this time)
Attachment #309256 - Attachment is obsolete: true
Attachment #310081 - Attachment is obsolete: true
(Assignee)

Comment 21

9 years ago
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
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
The documentation at

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

Has been updated.  Please let me know of any issues with it.
Keywords: dev-doc-needed → dev-doc-complete

Updated

9 years ago
Blocks: 426789
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.