851310 - The wrong local storage area is shown to chrome code when a session-only exception applies

Status: NEW

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

Description

[Ian Nartowicz]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/20100101 Firefox/17.0
Build ID: 20130107124423

Steps to reproduce:

1) Block cookies globally (which blocks DOM storage).
2) Create an exception for one site to allow cookies until the end of the session.
3) Create local storage items from that site.
4) Examine window.localStorage from chrome, for example an addon



Actual results:

No local storage items are visible.


Expected results:

The local storage items should be visible to the chrome code.

This can only really be tested after bug 600307 lands since window.localStorage didn't work in chrome before that.  See bug 600307 comment 84, raising here to keep track.  Although there is no native Firefox UI to examine DOM storage from chrome, you can use existing addons such as Cookie Controller.

The bug is in canUseStorage(), which checks for a chrome caller before setting isSession based on an exception.  So chrome code gets the standard storage area, while content gets the session-only storage area.  Note that in a private browsing window, everything works since there is only one (non-persistent) storage area.

Comment 1

[Honza Bambas (:mayhemer)]

It seems like we should enhance the nsIDOMStorageManager API to tell explicitly what storage should be picked or how it should behave.  There is an arg for specifying PB mode, but no arg for specifying session-only state.  New method is needed like createSpecificStorage(principal, pb, so);

Comment 2

[Ian Nartowicz]

Does it really need that?  A document can only be using one storage at a time, session-only or persistent.  In fact they are really just a technical convenience for managing whether the one actual storage area is persisted or not, so it doesn't make sense to be able to pick a different one from the one "in use".  For example, if the permissions for a site indicate session-only then the other storage can't (shouldn't!) have anything sensible in it so there is no reason to look at it.

I assume this all works if the permissions are changed while a document has storage items, so that the document can still see its own storage items?

So don't we just need canUseStorage() to pick the correct area when called from chrome, just as it does when called from content?  Even with a new API, you would still need to determine whether to set an so flag, and currently calls from chrome get that wrong because they are just granted permission without ever checking for a session-only site exception.

Comment 3

[Honza Bambas (:mayhemer)]

No.  There is one cache per-origin that keeps 3 data sets: default, session only, private browsing.  A storage object (there can be more then one, each for different window with different principal object with the same code base origin) using the cache knows what set to work with and instructs the cache.  The storage object determines whether session only is enabled by inspecting permissions of the *caller's* principal.  In your case that is the chrome principal and for it the session only state cannot be determined in all cases.


As I'm looking at the CanUseStorage method, it is clear we are not even trying to determine session only state when caller is chrome (it's dropped to false).  Seems like when the caller is chrome, we may want to inspect the storage bound principal rather then subject principal.

I'll think about this after 600307 lands.

Comment 4

[Ian Nartowicz]

Well I won't argue about the implementation details, just so long as chrome can see whatever a web page sees (and sets).  Is that actually correct in all situations?  If chrome automatically has the right to see storage items regardless of permissions, will there be cases where it can see items that a page no longer has the right to see?

I ran some tests on whether content sees the same storage items when permissions change.  Not chrome this time, just content.  From a web page that has full cookie permissions I call setItem("test", "test3"), then getItem("test").  It comes back as "test3", as expected.  Then I set the permissions to session-only and call getItem("test").  In the current version, it comes back as null.  The origin is exactly the same, so there is no reason why the result should be different.  It works the other way too, a storage item written when the cookie permissions were session-only cannot be seen if the cookie permissions change to allow persistent cookies.  If the permissions are toggle back, the key reappears.

With your changes, this appears to work correctly.  When the permissions change, the same storage keys are visible to the page.

Another test: set an item.  Switch permissions to block all cookies.  Switch back.  In current versions, all local storage has gone.  With your patch, it cannot be seen when cookies are blocked, but is still present when cookies are allowed again.  I think this is correct.  Amazing how thoroughly flaky the previous implementation was!

I'll do more extensive tests once your final patch lands and appears in nightly builds.

Comment 5

[Ian Nartowicz]

I'm testing with what I assume includes the patch from bug 600307 (build ID 20130416030901) and things seem to have got slightly worse.  Chrome still can't see the same localStorage as content, but that was expected.  Now, chrome also can't see sessionStorage items under the same conditions.  Surely there aren't separate areas for sessionStorage depending on whether permissions allow only session cookies or not?

However, now even content loses track of its storage items when cookie permissions change.  Set cookie permissions to session-only.  Store a sessionStorage item and a localStorage item  Change permissions to allow cookies forever and those two items no longer exist.  They don't come back if the permissions are changed back.  I don't think it is correct that changing permissions to session-only and back again should delete anything.  It works when changing from allow cookies forever to session-only (storage areas inherited in this case?).

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to Ian Nartowicz from comment #5)
> I'm testing with what I assume includes the patch from bug 600307 (build ID
> 20130416030901) and things seem to have got slightly worse.  Chrome still
> can't see the same localStorage as content, but that was expected.  Now,
> chrome also can't see sessionStorage items under the same conditions.

You mean same condition are the session-only setting?
 
> Surely there aren't separate areas for sessionStorage depending on whether
> permissions allow only session cookies or not?

There of course are.  When the browser switches to session-only mode, the normal data set is copied to the session-only set, and the session-only set is then used instead of the normal one.

When the browser switches back from session-only mode, all the session-only data are dropped from memory and the normal data set is used again, containing the values as before switching to session-only mode.

> 
> However, now even content loses track of its storage items when cookie
> permissions change.  Set cookie permissions to session-only.  Store a
> sessionStorage item and a localStorage item  Change permissions to allow
> cookies forever and those two items no longer exist.  They don't come back
> if the permissions are changed back.  I don't think it is correct that
> changing permissions to session-only and back again should delete anything. 

Hmm.. this is interesting.  However, I am not sure how to recognize between "I left session only and want to get rid of all session only data" and "I left session only and since now I want to persist the data that I've collected during session only mode".

I might misunderstood the spec, but the "throw away" behavior seems to me expected and is there since ever.

Bug 600307 just made this behavior consistent and correct (according how we understand the behavior should be) for both local/ and sessionStorage.

> It works when changing from allow cookies forever to session-only (storage
> areas inherited in this case?).

Yes, as I said above.  It's copied and then threw away.

Comment 7

[Honza Bambas (:mayhemer)]

Btw, this behavior has been decided a long ago at bug 487695.

Comment 8

[Ian Nartowicz]

(In reply to Honza Bambas (:mayhemer) from comment #7)
> Btw, this behavior has been decided a long ago at bug 487695.

Interesting, but bizarre.  Also not the way cookies work.  Doing what it says on the tin and discarding the localStorage items at the end of the session would be nice, but I guess there is no concept of expiration for them to follow.  I don't think we should persist data that was set when the permission was to not persist data, even if that permission has now changed?

Why does sessionStorage also disappear?  Surely changing the session-only permission should have no effect on items which are by definition session-only.

Comment 9

[Ian Nartowicz]

Are the session-only areas really discarded?  I am seeing them persist.  Once a session-only area has been created as a clone of the normal areas then it lasts until Firefox is closed.  Leaving session-only mode and returning to it the same items are still there.  When entering session-only permission mode for the second time, the existing storage areas are not cloned again, but instead the content script just sees whatever was there when it was previously in session-only mode.

Comment 10

[Ian Nartowicz]

(In reply to Honza Bambas (:mayhemer) from comment #6)
> (In reply to Ian Nartowicz from comment #5)
  
> > Surely there aren't separate areas for sessionStorage depending on whether
> > permissions allow only session cookies or not?
> 
> There of course are.  When the browser switches to session-only mode, the
> normal data set is copied to the session-only set, and the session-only set
> is then used instead of the normal one.
> 
> When the browser switches back from session-only mode, all the session-only
> data are dropped from memory and the normal data set is used again,
> containing the values as before switching to session-only mode.
> 
Well that explains the behaviour with sessionStorage.  Not nice, but what is the answer?  Should chrome scripts be shown the same thing content scripts see, even though it is sandboxed and will disappear at some point?  Or should it be able to see the, in the case of localStorage at least, data which is written to disk and will still be there tomorrow?  Or both?

There shouldn't be a problem for sessionStorage except that it is currently also being sandboxed.  Still, it wouldn't be the end of the world if a chrome script couldn't see data that can't currently be accessed by a content script and isn't stored on disk.  If those items change suddenly when permissions are changed, so be it, so long as they can be seen from chrome.