Closed
Bug 407839
Opened 17 years ago
Closed 16 years ago
restrict globalStorage to same host
Categories
(Core :: DOM: Core & HTML, defect, P2)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
People
(Reporter: HeroreV, Assigned: dcamp)
References
Details
(Keywords: addon-compat, dev-doc-complete)
Attachments
(1 file, 4 obsolete files)
9.96 KB,
patch
|
Details | Diff | Splinter Review |
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•17 years ago
|
||
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•17 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•16 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•16 years ago
|
Flags: tracking1.9+ → blocking1.9+
Priority: P3 → P2
Assignee | ||
Comment 5•16 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•16 years ago
|
||
Filed bug 422526 for implementing localStorage.
Comment 7•16 years ago
|
||
Attachment #309113 -
Flags: review?(dcamp)
Comment 8•16 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•16 years ago
|
||
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•16 years ago
|
Attachment #309158 -
Attachment is patch: true
Attachment #309158 -
Attachment mime type: application/octet-stream → text/plain
Assignee | ||
Comment 10•16 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•16 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•16 years ago
|
||
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•16 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•16 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•16 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•16 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•16 years ago
|
Attachment #309256 -
Flags: superreview?(jst)
Updated•16 years ago
|
Attachment #309256 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Comment 17•16 years ago
|
||
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 19•16 years ago
|
||
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•16 years ago
|
||
Attachment #309256 -
Attachment is obsolete: true
Attachment #310081 -
Attachment is obsolete: true
Assignee | ||
Comment 21•16 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
Closed: 16 years ago
Resolution: --- → FIXED
Comment 22•16 years ago
|
||
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•11 years ago
|
Component: DOM: Mozilla Extensions → DOM
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•