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.

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

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.

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.

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.

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.

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.

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.

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.

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.

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

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

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

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.

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

I don't think the patch here is sufficient.

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.

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 on attachment 233242 [details] [diff] [review] Better patch I'd rather delegate the sr= to dveditz, for his security oversight.

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!

(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']

> > 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.

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 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 on attachment 234405 [details] [diff] [review] address review comments Just what I wanted, thanks! sr=dveditz

Comment on attachment 234405 [details] [diff] [review] address review comments r=jst

Comment on attachment 234405 [details] [diff] [review] address review comments Drivers: baaaaaaam!

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

(Beltzner claims not, that it should trunk-land first or something.) I can't explain the "baaaaaaaaaaam" thing, but I bet the barmaid can.

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

Trunk version is checked in

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

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