341524 - Make webapps session storage follow the cookie prefs

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

As dbaron points out in bug 335540 comment 70. webapps session storage should be following the cookie prefs.  If cookies are disabled, session storage should be disabled too, for example.  Otherwise, there's really no point to having the cookie prefs -- sites can just use storage instead of cookies to circumvent the user's wishes.

Comment 1

[Boris Zbarsky [:bzbarsky]]

And I really wish I could target 1.8.1b1 or something...

Comment 2

[Neil Deakin]

shaver, or anyone: how do we want DOM storage to follow cookie prefs? DOM storage doesn't have any expiry handling or quite fit the cookie prefs available in the UI.

I could just use the cookie whitelist.

Comment 3

[Boris Zbarsky [:bzbarsky]]

We should certainly expire storage at shutdown if cookies are limited to session cookies, and we should follow the lists of sites that are not allowed to set cookies, etc.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The cookie prefs can essentially be expressed as a function mapping:
   domain -> one of { ALLOW, ALLOW_FOR_SESSION, BLOCK }
since there's both a default that is one of those three and then a list of per-domain exceptions that can be to any of those three.  Session storage should essentially call the function in the cookie code that does that mapping and behave appropriately.

Comment 5

[Neil Deakin]

Attachment: Check cookie prefs before using DOM storage

This patch does a check for cookie preferences before getting or modifying dom storage values.
- don't know what url should be used, the caller or the domain of the storage being accessed?
- added a method to nsICookieServiceInternal. Not sure if it should go in nsICookieService instead.
- one improvement would be to cache the settings.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So this looks like it's just making a two-way distinction rather than a three-way distinction.

As a user of the cookie prefs (I default to allow for session and mark sites that I log in to as allow), that doesn't really seem sufficient, since allow for session is actually critical for any user who doesn't want allow.  Many sites are unusable without cookies being at least session cookies; I fully expect sites that use DOM storage to have the same problems.

Comment 7

[Neil Deakin]

DOM Storage has both the capability of storing session only data and global data stored on disk. When cookies are enabled, both types are allowed. When the cookies are set to only store for that session, only session storage data may be used. When cookies are disabled, neither can be used.

I could change it such that global data is stored in the session store instead when cookies are set to expire at the end of the session.

Comment 8

[Christian Persch (GNOME) (away; not receiving bug mail)]

Cookies also have a lifetime, and embedders can implement nsICookiePermission and limit the lifetime from ::CanSetCookie. So webapps storage should also not only allow to distinguish session and permanent storage, but also allow lifetime limiting; and do it in a way that embedders can override.

Comment 9

[Johnny Stenback (:jst)]

I would also recommend (based on discussion in bug 339940) that we add the following code to the top of nsDOMStorage::MayUseStorage():

  if (!nsContentUtils::GetBoolPref("dom.storage.disabled")) {
    return PR_FALSE;
  }

Just to have a global way to disable just dom storage completely (w/o disabling cookies) in case there's ever a need to.

Comment 10

[Johnny Stenback (:jst)]

*** Bug 339940 has been marked as a duplicate of this bug. ***

Comment 11

[Johnny Stenback (:jst)]

w/o the '!' of course, duh.

Comment 12

[Neil Deakin]

shaver, could you comment on how we want to reuse the cookie preferences here?

Comment 13

[Johnny Stenback (:jst)]

I'm no cookie expert, but I've been doing some thinking (and discussing) about how we'd want the cookie prefs to affect DOM storage. I think we inevitably need to follow the cookie model pretty closely to avoid ad-sites from abusing DOM storage for user tracking all over the web, so for one thing we'd need to follow the cookie model where we require at least one dot in the domain name when storing data in global storage (the cookie code has code to deal with blocking .com, .co.uk etc, so we can most likely reuse that).

As for how long to keep data in storage goes I think we should change the storage code to use an in-memory storage object even for global storage (unless we can make the DB code clear the DB reliably even in case of a browser crash etc) for domains for which the cookie settings are set to store for the length of the session only (same applies if that's set globally).

When checking whether to grant access to DOM storage for a certain domain I think the domain to use in that check is the domain of the loading page, not the domain for which global storage is requested/accessed.

Cc:ing dveditz as he should be in on any decisions made about how we limit usage of DOM storage, and he's got a much better understanding of the cookie prefs than I do.

Comment 14

[Mike Schroepfer]

Beltzner/DVeditz any other comments here?   If not I recommend we land what is here as it is better than what we have.  

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't think the patch here is sufficient.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 227337 [details] [diff] [review]
Check cookie prefs before using DOM storage

We want a pref separate from cookies to disable session and global storage entirely (comment 9). Probably a single pref is OK, maybe one each though since sessionStorage is much less troubling. This can be a UI-less internal pref for embeddors and in case something goes horribly wrong.

If you're using the cookie management features (and it makes sense -- conceptually simpler for the user) then we need to use nsICookiePermission to get back whether each host is session-only rather than relying solely on the global cookie setting (comment 4). Unfortunately the current API doesn't provide the right information -- canSetCookie might pop some UI we don't want and requires extra arguments we don't really have, and canAccess doesn't distinguish between ACCESS_ALLOW and ACCESS_SESSION (since it expects cookies to get downgraded appropriately in canSetCookie).

nsICookiePermission is probably more frozen than nsICookieServiceInternal, but both probably need to get the _MOZILLA_1_8_BRANCH uglification treatment on the branch if you change them. (but read on, I'm not sure you should change them at all.)

nsCookieService::CheckPrefs() doesn't get you the answers we need (for one thing it uses nsICookiePermission::canAccess which drops session-only permissions as noted above).

CheckPrefs() disallows FTP urls, the storage spec says it should be allowed (Section 5.9.8.4). Personally I'm dubious about the spec and am just fine blocking FTP, but did you intend to? If you did I'd be much more comfortable with an http(s) whitelist.

CheckPrefs() might pop a dialog in a p3p-enabled build.

Ultimately (though not immediately) we will have to implement per-site storage quotas. For that you could use the lower-level nsIPermissionManager directly (instead of an enum, the integer values could be the quota in Kb, 0 meaning "session-only").

>+nsDOMStorage::MayUseStorage()
>+{
>+  nsAutoString url(NS_LITERAL_STRING("http://"));
>+  url.Append(mDomain);

Re-constituting URIs out of strings has caused several security issues in the past, could you keep the document's URI around and use that instead? (or the codebase from the principal)

> NS_IMETHODIMP
> nsDOMStorage::Key(PRUint32 aIndex, nsAString& aKey)
> {
> 
>+  if (!MayUseStorage())
>+    return NS_OK;

NS_OK instead of an error (ditto below)? is aKey empty at this point?

> NS_IMETHODIMP
> nsDOMStorage::GetItem(const nsAString& aKey, nsIDOMStorageItem **aItem)
> {
>+  if (!MayUseStorage())
>+    return NS_OK;
>+
>   *aItem = nsnull;

Move the check after initializing aItem? Since this is a public API should we at least assert aItem isn't null? (not entirely up on DOM code habits in this regard).

Seems like there ought to be a check before even returning a globalStorage object, not just on its method calls.

Comment 17

[Neil Deakin]

Attachment: Better patch

This version does something closer to cookies:

- disabling cookies disables storage, unless the site is on the whitelist.
- enabling all cookies enables storage, except if the site is on the blocked list.
- if a site is set to session only in the block list, only session storage may be used. Global storages are treated like session storages.
- similarly, if the cookie preferences are set to session only, all storage usage is session only
- if the cookie preferences are set to prompt, this is treated the same as reject cookies. Not sure whether we want prompting to occur here. Do cookies prompt every time a cookie is set, or just once per session?
- the hidden preference dom.storage.disabled can be used to disable DOM storage.

Comment 18

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

Comment on attachment 233242 [details] [diff] [review]
Better patch

I'd rather delegate the sr= to dveditz, for his security oversight.

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 233242 [details] [diff] [review]
Better patch

>+static const char kStorageDisabled[] = "dom.storage.disabled";
...
>+  if (nsContentUtils::GetBoolPref(kStorageDisabled))
>+    return PR_FALSE;

Please change the pref name to .enabled and add ! to the test -- we've got about a dozen .enabled prefs and no .disabled (there's a single blah.blah_disabled one though) so it'd match prevailing style. [On the other hand we do have a bunch of "dom.disable_blah" prefs, but I think that's pretty awful since it makes it harder to sort related prefs together. A large part depends on whether we think there will be any other storage prefs in the future, and I strongly bet there will be (size limits?) unless finer grained controls get stored elsewhere]

Whatever the name, the default pref needs to be added to all.js

>+  else if (perm == nsIPermissionManager::UNKNOWN_ACTION) {

This must be "perm != nsIPermissionManager::ALLOW_ACTION" instead -- there may be future values that wouldn't be "unknown", but we don't know now if they're variants on OK or variants on not OK.

>   if (subjectPrincipal == systemPrincipal || !currentDomain.IsEmpty()) {
>-    return GetStorageForDomain(aDomain, NS_ConvertUTF8toUTF16(currentDomain),
>+    return GetStorageForDomain(uri, aDomain, NS_ConvertUTF8toUTF16(currentDomain),
>                                subjectPrincipal == systemPrincipal, aStorage);

If this is all you're using the systemPrincipal for you could use ssm->SubjectPrincipalIsSystem() call instead (or nsContentUtils::IsCallerChrome(), but that just adds overhead if you've already got ssm).

>+  // fail if the domain contains no periods

When the "effective TLD" stuff gets checked in storage should use that instead, but yay for blocking .com etc at least.

But wait, is the ".localdomain" stuff from the spec implemented? what if the user is developing a page on http://localhost/ ?

Nearly there, but I'd like to see a new patch. Please use the same diff settings to make it easier to diff the diffs so I don't have to re-review the whole patch.

Thanks!

Comment 20

[Neil Deakin]

(In reply to comment #19)
> Please change the pref name to .enabled and add ! to the test -- we've got
> about a dozen .enabled prefs and no .disabled

Would that mean that people upgrading would still get DOM storage enabled?
Do we want to explicitly disable DOM storage for mail?

> 
> When the "effective TLD" stuff gets checked in storage should use that instead,
> but yay for blocking .com etc at least.

Will add a comment about this.

> 
> But wait, is the ".localdomain" stuff from the spec implemented? what if the
> user is developing a page on http://localhost/ ?
> 

Yes, that is implemented. It should work if you ask for globalStorage['localhost.localdomain']

Comment 21

[Daniel Veditz [:dveditz]]

> > Please change the pref name to .enabled and add ! to the test
> 
> Would that mean that people upgrading would still get DOM storage enabled?

Why wouldn't they? The new default pref should be .enabled set to true, and you bail out if the pref is set to false (which it won't be unless people set it manually). There's no existing pref value to migrate to the new name so we're fine on that end, too.

> Do we want to explicitly disable DOM storage for mail?

Good point. It is effectively disabled because JavaScript is disabled by default, but we really do want to block it because there are idio^H^H^H^Hpeople who turn it on for various reasons. That's a separate issue that should be tracked in a new bug, though, not preventing this patch from going in.

Comment 22

[Daniel Veditz [:dveditz]]

Although it's easy enough to get the root docshell and check for APP_TYPE_MAIL and block it. So if it's easier for you getting it done at the same time rather than splitting it out that's fine with me.  No pref, just always blocked.

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 233242 [details] [diff] [review]
Better patch

r- based on dveditz' comments, but aside from that this looks good to me. One additional minor comment below.

- In nsDOMStorage.h:

   // true if the storage database should be used for values
   PRBool mUseDB;
 
+  // true if the preferences indicates that this storage should be session only
+  PRBool mSessionOnly;
+
   // true if items from the database are cached
   PRBool mItemsCached;

Those PRBools should be PRPackedBool.

Comment 24

[Neil Deakin]

Attachment: address review comments

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 234405 [details] [diff] [review]
address review comments

Just what I wanted, thanks!
sr=dveditz

Comment 26

[Johnny Stenback (:jst)]

Comment on attachment 234405 [details] [diff] [review]
address review comments

r=jst

Comment 27

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

Comment on attachment 234405 [details] [diff] [review]
address review comments

Drivers: baaaaaaam!

Comment 28

[Mike Schroepfer]

Did you mean to approve :-)?

(In reply to comment #27)
> (From update of attachment 234405 [details] [diff] [review] [edit])
> Drivers: baaaaaaam!
> 

Comment 29

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

(Beltzner claims not, that it should trunk-land first or something.)

I can't explain the "baaaaaaaaaaam" thing, but I bet the barmaid can.

Comment 30

[Mike Schroepfer]

Neil - can we get this on trunk and we can do an approval over the weekend?

Comment 31

[Neil Deakin]

Attachment: Branch version of patch

Comment 32

[Neil Deakin]

Trunk version is checked in

Comment 33

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

Comment on attachment 234539 [details] [diff] [review]
Branch version of patch

a=beltzner on behalf of drivers, please land on MOZILLA_1_8_BRANCH and mark fixed1.8.1

Comment 34

[Mike Connor [:mconnor]]

Checked this in on branch for Neil, since it was one of the last blockers holding back freeze.