486654 - implement localStorage for 1.9.1

Status: RESOLVED FIXED

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

Description

[Honza Bambas (:mayhemer)]

+++ This bug was initially created as a clone of Bug #422526 +++

I'm preparing a patch to address the security review comments, https://wiki.mozilla.org/Firefox3.1/localStorage_Security_Review

(In reply to comment #93)
> From the security review ..:
> 
> > *  currently if cookie lifetime is session-only we turn 
> > globalStorage/localStorage into a lame current-page-only memory store. It would 
> > be better to throw an error (QUOTA_EXCEEDED_ERR is mentioned in the spec for 
> > this case) or return null for locationStorage itself. Something that lets 
> > developers switch to using real sessionStorage instead, or telling users to 
> > enable the feature.
> 
> Not sure why that's a security comment, but it seems sensible for 1.9.1

As it's not clear what to do, I won't fix this at this time. Unfortunatelly I
could not find the localStorage spec anymore.

> 
> > * test that blocking cookies for a host does block localStorage for that host
> 
> Yes, we need this for 1.9.1

Done

> 
> > * test that localStorage is truly origin-based -- i.e. windows on different 
> > ports or schemes should not be able to access each other's localStorage even 
> > if the same host.
> 
> Absolutely needed for 1.9.1, yes. That's the definition of localStorage and
> keeps us from leaking data.

It's already in the test suit.

> 
> > * use of globalStorage[] should put a deprecation warning on the error 
> > console. Probably just the first use per window to avoid perf problems 
> > (object creation).
> 
> I don't think we need this for branch, as it would require that we break string
> freeze - we should file a follow-up and get it fixed for trunk.
> 
> > * add default prefs to all.js for missing values (e.g. quota).
> 
> Needed for 1.9.1.

I found missing only "dom.storage.default_quota".

> 
> > * Should we put a length limit on keys? It's counted in the quota so we think we're OK.
> 
> Not needed for 1.9.1
> 
> > * test both keys and values with embedded nulls
> 
> Needed for 1.9.1.

Done.

> 
> > * test both keys and values with invalid UTF-8 sequences and invalid UTF-16, including ending on partial sequences.
> 
> Absolutely needed for 1.9.1

The interface is fully UTF-16, so I don't think there is need to test UTF-8.

> 
> > * we feed UTF-16 to sqlite, make sure it handles this OK and converts to utf-8 OK. 
> 
> Absolutely needed for 1.9.1

It's actually the same test as for the previous point. I am trying to store
"\xD800" string value using setItem, but getItem returns a different string.
Looks like there is wrong conversion somewhere inside the sqlite code: call to
sqlite3VdbeMemSetStr, it passes w/o error report that the input is
incorrect/broken. Does this mean we should sanitize the input on level of the storage? This already affects globalStorage, so we could defer fix for this issue.

Comment 1

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

(In reply to comment #0)
> As it's not clear what to do, I won't fix this at this time. Unfortunatelly I
> could not find the localStorage spec anymore.

Yeah, it moved, it's now here:

http://dev.w3.org/html5/webstorage/#the-localstorage-attribute

Comment 2

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

Honza: did you mean to post the patch that addresses all the points you say are "done" in comment 0?

Comment 3

[Honza Bambas (:mayhemer)]

Attachment: wip 1

This is it. Just 3 tests, one of them failing, and added default preference.

Comment 4

[William J. Edney]

Honza -

Noticed that in the patch that you're allocating 5MB / domain, as per the spec.

IE8, it turns out, allocates 10MB / domain for their 'localStorage' implementation.

Here's a bug I filed a while back on this:

https://bugzilla.mozilla.org/show_bug.cgi?id=461684

Unless there's major objections, could we set:

pref("dom.storage.default_quota",      10240);

I mean, we're starting to get 500GB drives in laptops now (for under $300.00) :-)

Cheers,

- Bill

Comment 5

[Boris Zbarsky [:bzbarsky]]

Bill, not everyone is using a computer they bought yesterday, nor is everyone using a computer with their profile on a filesystem that they fully own (i.e. no quota).  In any case, that has nothing to do with this bug.  As you note, there's a separate bug on the quota size issue, and that's where it should be discussed.  Or better yet, in the standardization process.

Comment 6

[William J. Edney]

Boris -

Suitably chastised... and rightly so.

- Bill (slinking off to corner).

Comment 7

[Honza Bambas (:mayhemer)]

> *  currently if cookie lifetime is session-only we turn 
> globalStorage/localStorage into a lame current-page-only memory store. It would 
> be better to throw an error (QUOTA_EXCEEDED_ERR is mentioned in the spec for 
> this case) or return null for locationStorage itself. Something that lets 
> developers switch to using real sessionStorage instead, or telling users to 
> enable the feature.

From the spec:
"Treating persistent storage as cookies: user agents should present the persistent storage and database features to the user in a way that does not distinguish them from HTTP session cookies. [RFC2109] [RFC2965]"

Based on this I'd say that what we do now is correct. If user decides to store data only per-session then it should apply to localStorage as well. BTW, if we throw an exception or something then site can recognize that UA is storing just per-session, that could be a privacy issue.

Comment 8

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

ETA on a reviewable patch here? If this doesn't make Beta 4 (code freeze Wed Apr 15) then it's not making it at all.

Comment 9

[Honza Bambas (:mayhemer)]

I just need to figure out what to with the broken UTF-16 that is already in and affects globalStorage. I'll discuss this quickly with someone on IRC. I think the patch (wip 1) w/o the failing test is what we need.

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: v1

(In reply to comment #8)
> ETA on a reviewable patch here? If this doesn't make Beta 4 (code freeze Wed
> Apr 15) then it's not making it at all.

Status:

- there is a broken UTF-16 problem: storing a broken value results to getting back a different string, screwed by sqlite; after chat with dveditz we agree to land it with this issue. Also, I checked there is the same problem with globalStorage on both 1.9.1 and trunk, so it's already existing problem, should be fixed in a followup; tests are in but just as todos.

- sorry for my comment 7, it's not accurate, I was looking just to the dom storage code; window.localStorage implementation blocks getting localStorage object when cookies are session-only for an origin (a host) (security error); I tried to remove that condition and tests show values are stored only per-session, as I would expect and localStorage works normally in that mode (checked by a modified localstorage base test)

This patch is built on top of the 1.9.1 version of the localStorage patch.

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to comment #10)
> - sorry for my comment 7, it's not accurate, I was looking just to the dom
> storage code; window.localStorage implementation blocks getting localStorage
> object when cookies are session-only for an origin (a host) (security error); I
> tried to remove that condition and tests show values are stored only
> per-session, as I would expect and localStorage works normally in that mode
> (checked by a modified localstorage base test)

After another chat with dveditz we decided to revert to the original suggestion, to return localStorage in case cookies are only per-session but throw quota error when storing to it. This allows sites identify an error and prevent potential data loss but not to expose state of cookie behavior. What I suggest (or as I understand the spec) allows sites to identify it: storing to localStorage, loading the same-origin page in e.g an iframe and accessing the storage may reveal that cookies are in just per-session mode.

This change should probably be landed on trunk first? I will create a bug for it.

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 371653 [details] [diff] [review]
v1

- In nsGlobalWindow::GetLocalStorage():

-    if (sessionOnly)
-      return NS_ERROR_DOM_SECURITY_ERR;
-

Per discussion with Honza, we'll leave this change out of the patch, and leave localStorage a bit crippled in this case for now. We'll fix that in a followup bug for another release.

r+sr=jst with that (and I was told dveditz agreed with the above conclusion as well).

Comment 13

[Honza Bambas (:mayhemer)]

To be clear, Daniel Veditz didn't agree, he suggested to throw DOM_QUOTA_ERROR when storing. But, it's not clear how localStorage.clear() and localStorage.removeItem() should behave. It's also a modification of the data but DOM_QUOTA_ERROR is not applicable for them. Leave them silently pass? It's not clear at the moment and I think there are already some discussions about this:

http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2009-April/019238.html

So, it seems better to leave it as is and fix in a follow up.

Comment 14

[Daniel Veditz [:dveditz]]

I'm confused about what you think I agree with, and I'm not entirely sure what returning an error at that point in C++ means to a caller from web content (is it an exception? or turned into something else along the way?).

Our overriding goal should be to have a compatible implementation--if we didn't care we could stick with globalStorage! That means we're going to have to hash out the spec with WHATWG and that's not going to make any immanent code-freeze. But I sure hope the "another release" we'll fix this for is before 3.5 final!

What I don't like about our current scheme is that it is _not_ just "localStorage but not written to disk". If you had a multi-page web-app its interaction with "localStorage" would be complete different if cookies were set to session-only, and the app would be better off knowing that's the case so they could fall back to sessionStorage (or server-side storage).

I don't mind Brady's option 1 or 2 so much, unlike him. At least not in the "cookies are blocked" case, maybe his concern is that it could be a hint that the user is in private-browsing mode. I might slightly prefer removing the localStorage object entirely because it reinforces object detection which sites ought to be doing to handle browsers that don't support it. But that means we can't throw an exception when the user tries to get the object, it's just not there.

But that means the thing that adds localStorage to the DOM has to know that logic, it can't be something inside the localStorage code :-(. So second best an effective quota of 0 seems to fit the bill in my mind. I'd have clear() and removeItem() silently fail as Brady suggested.

Brady prefers his option 3, but that's not what we were doing. If we did that and had a fully workable in-memory localStorage object I could live with it, but ours was crippled. Kind of the worst of both worlds IMHO.

His option 4 sounds hard/slow and requires us to get option 3 working correctly first. I like his option 5 for private browsing and for the session-only cookies state it collapses to option 2 since nothing ever got stored there.

... we really ought to be exposing stored data through the (cookie?) UI, at least the bulk amount per host (like flash) if not per-item details.

Comment 15

[Honza Bambas (:mayhemer)]

Daniel, what I want to do as part of this bug is to find a quick compatible solution to let this be landed on 1.9.1 ASAP. 

What I suggest now after I completely went through the Brady's thread is to throw a DOM_SECURITY_ERROR when accessing window.localStorage object while in either private-browsing-mode or session-only-cookies-mode. The letter is already implemented. It means:

- very simple fix (1 minute to implement)
- we may expect that even not well designed web apps will do error handling of access to localstorage object (I mean the object, not values); so, sites should not break that massively than if we play with dom quota; sites may fallback to server storage soon, there is no data loss; also, you suggest to hide localStorage presence completely from DOM to prevent object detection, except a potential privacy issue this is the same as what I suggest
- we don't expose any existing data (same as a new profile); no privacy issues
- we won't give web devs any crippled/unpredictable/unspecified object they must rely on

I would expect bug 487695 will be fixed before FF 3.5 final release, anyway, if there is a solution found (that is unclear at this time, there are many arguments, many approaches and with it many quit different implementation designs).

I'll answer to your comment also there.

Comment 16

[Honza Bambas (:mayhemer)]

Attachment: v2

This is patch according to my last comment - disallowing localStorage object access in private browsing mode for now.

If you all agree, please r+ and we can land. If not please suggest something we can have reviewed and landed before next Wednesday (3 business days from now).

Comment 17

[Honza Bambas (:mayhemer)]

(Bugzilla interdiff of v1 and v2 is correct)

Comment 18

[(no longer active)]

(In reply to comment #15)
> What I suggest now after I completely went through the Brady's thread is to
> throw a DOM_SECURITY_ERROR when accessing window.localStorage object while in
> either private-browsing-mode or session-only-cookies-mode. The letter is
> already implemented. It means:

A brain-dead way to break this would be:

var storage = window.localStorage;
// use |storage| from this point forward

Any web page which does that will not be affected by the user changing their cookie settings, unless it's reloaded.

Comment 19

[Honza Bambas (:mayhemer)]

(In reply to comment #18)
> A brain-dead way to break this would be:
> 
> var storage = window.localStorage;
> // use |storage| from this point forward
> 
> Any web page which does that will not be affected by the user changing their
> cookie settings, unless it's reloaded.

Yes, you are right. But it's not a problem as I'm not trying to do any security restriction. Intention of this patch is to give some simple compatible 20/80 temporary solution to get localStorage on 1.9.1. Please read comment 15. What to exactly do for the final release is discussed in bug 487695.

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 372070 [details] [diff] [review]
v2

+  if (nsDOMStorageManager::gStorageManager->InPrivateBrowsingMode())

To be on the safe side here, I'd like to see us null check gStorageManager before accssing it. Most other code that uses it checks as well.

r=jst with that. Dveditz should look at this as well to be sure he's ok with this for the beta.

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 372070 [details] [diff] [review]
v2

sr=dveditz

I'll file a bug on Ehsan's objection -- it will need to be fixed before 1.9.1 final. Bug 487695 is too high-level to assume that will cause it to get fixed.

Comment 22

[Honza Bambas (:mayhemer)]

Attachment: v12.1 for 1.9.1 + crash fix + security review updates [Checkin comment 25]

This is the complete localStorage patch for Firefox 3.5 (Gecko 1.9.1). It's 1.9.1 merge of already r'd adn sr'd patch baked for a long time on mozilla-central + one crash fix that is also already landed on trunk + r'd and sr'd v2 security review updates patch from this bug.

I'll land the v2 update patch soon on moz-central.

Comment 23

[Honza Bambas (:mayhemer)]

Attachment: v2 for trunk [Checkin comment 23]

http://hg.mozilla.org/mozilla-central/rev/6eddf5c0fe3e

Comment 24

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

Comment on attachment 372579 [details] [diff] [review]
v12.1 for 1.9.1 + crash fix + security review updates [Checkin comment 25]

a191=beltzner

Comment 25

[Honza Bambas (:mayhemer)]

Comment on attachment 372579 [details] [diff] [review]
v12.1 for 1.9.1 + crash fix + security review updates [Checkin comment 25]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/dc7929b978bb

Comment 26

[Honza Bambas (:mayhemer)]

So, it's there and looks green! Dave Camp, Johnny Stenback, Boris Zbarsky, Daniel Veditz and all others that reviewed, superreviewed or helped in any other way to get this done, one big thank you!

Comment 27

[Honza Bambas (:mayhemer)]

(In reply to comment #21)
> I'll file a bug on Ehsan's objection -- it will need to be fixed before 1.9.1
> final. Bug 487695 is too high-level to assume that will cause it to get fixed.

Submitted bug 488446 for that.

Comment 28

[Eric Shepherd [:sheppy]]

I believe this is adequately documented now.  We already had:

https://developer.mozilla.org/En/DOM/Storage

I've updated the references that said it was in trunk but not included in Firefox 3.5, and made a few other minor corrections.

In addition, I've documented the interfaces behind DOM storage:

https://developer.mozilla.org/En/NsIDOMStorageWindow
https://developer.mozilla.org/En/NsIDOMStorage2
https://developer.mozilla.org/En/NsIDOMStorageEvent
https://developer.mozilla.org/En/NsIDOMStorageItem
https://developer.mozilla.org/En/NsIDOMStorage
https://developer.mozilla.org/En/NsIDOMStorageList

To top it all off, I've added the localStorage attribute to the DOM window documentation (both sessionStorage and globalStorage were already listed), and I've marked globalStorage as non-standard there:

https://developer.mozilla.org/En/DOM/Window

Please review these articles and either correct any errors or let me know about them so I can.

Comment 29

[Honza Bambas (:mayhemer)]

Few notes:

https://developer.mozilla.org/En/NsIDOMStorage2:
getItem method returns DOMString (a value directly), not nsIDOMStorageItem

https://developer.mozilla.org/En/NsIDOMStorageItem:
this interface is not used for localStorage, it's used for globalStorage and sessionStorage (will be used only for globalStorage after bug 455070 is fixed)

secure attribute means the item was stored for https page; all items (stored for either http or https) are visible for https page, only http stored items are visible for http page - therefor this attribute has no meaning for localStorage that is bound to an origin

Comment 30

[Eric Shepherd [:sheppy]]

I've addressed the issues in comment #29.  Thanks!  Not sure how I messed up the return from getItem(). :)

Comment 31

[Eric Shepherd [:sheppy]]

Oh yes I do... copy & paste error from nsIDOMStorage.  OK.  Anyway, it's fixed.