362446 - webapps storage should have size limits on the storage allowed per site

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

See bug 335540 comment 72.  As Hixie points out, allowing some site to store 20GB of data (say) on the user's hard drive is suboptimal.

We may want to make this subject to a check for expanded privileges like UniversalBrowserWrite or something.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Needless to say, the problem is that this allows websites to fill up the user's hard drive if they choose to be annoying.

Comment 2

[Jay Patel [:jay]]

Too late for 1.8.0.9/1.8.1.1, let's try to get a handle on this for the next one.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Actually, not an issue for 1.8.0.

Comment 4

[Neil Deakin]

Attachment: simple implementation

This is a simple implementation which just prevents storing of more that 5mb per requested domain.

I'm not sure what the desired implementation is here. The spec seems to imply that 5mb for each domain that sets value, but it isn't clear if that includes subdomains. Also, this differs because it is the calling domain, not necessarily the domain being requested as implemented in this patch.

Comment 5

[Dave Camp (:dcamp)]

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

Comment 6

[Dave Camp (:dcamp)]

Attachment: limit storage based on the site setting the key

This patch limits storage based on the domain making the request, but this required updating the table.

It also defines an mozIStorageFunction to count the size of rows;  LENGTH() as provided by sqlite counts the number of utf8 characters in the value, this function just returns the bytes used.

(moved over from 363517)

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 248418 [details] [diff] [review]
limit storage based on the site setting the key

Neil should review this, once he's done I'm happy to sr...

Comment 8

[Neil Deakin]

Comment on attachment 248418 [details] [diff] [review]
limit storage based on the site setting the key

This seems fairly complex for what it needs to do. Can't you just use ALTER TABLE to add a column without requiring an entirely new table?

I'm also not sure if the characters vs. bytes distinction is worth the extra code for calculating lengths. Does the use of the custom function have a performance penalty?

To improve performance when there a lot of keys, quota checking only needs to be done, say, every x amount of data written.
For instance, if there is only 200 characters currently in the table, then adding 50 more isn't going to overflow.

Comment 9

[Dave Camp (:dcamp)]

(In reply to comment #8)
> (From update of attachment 248418 [details] [diff] [review] [edit])
> This seems fairly complex for what it needs to do. Can't you just use ALTER
> TABLE to add a column without requiring an entirely new table?

Maybe, depending what sort of compatibility you want.  The old code uses INSERT INTO moz_webappsstore VALUES (?, ? ..) - if an old version runs against a new store, it'll crap out completely.

If that's acceptable, a table alteration makes much more sense.

> I'm also not sure if the characters vs. bytes distinction is worth the extra
> code for calculating lengths. Does the use of the custom function have a
> performance penalty?

There's the cost of constructing the mozStorageValueArray.  It'd be worth measuring against the cost of counting over larger values.

> 
> To improve performance when there a lot of keys, quota checking only needs to
> be done, say, every x amount of data written.
> For instance, if there is only 200 characters currently in the table, then
> adding 50 more isn't going to overflow.


That makes sense;  also a cache similar to the one in the earlier patch could be done reasonably easily.

> 

Comment 10

[Neil Deakin]

Dave, are you working on a newer patch for this bug?

Comment 11

[Dave Camp (:dcamp)]

Attachment: another cut

another cut;  uses LENGTH rather than defining its own function, uses ALTER TABLE, and keeps a cache of the last owner to insert a key.

Comment 12

[Neil Deakin]

Dave, did you want someone to review this or is it still in progress?

Thinking about it, I think I prefer creating a new table after all, to ensure compatibility, but just call it 'webappsstore' (no moz_ prefix).

Comment 13

[Dave Camp (:dcamp)]

Attachment: create new table

ok, new one creates webappsstore

Comment 14

[Neil Deakin]

Comment on attachment 250874 [details] [diff] [review]
create new table

>+DOM_MSG_DEF(NS_ERROR_DOM_QUOTA_REACHED, "Quota reached")

This error message should be more specific, like 'persistent storage maximum size reached'
>+static PRInt64
>+GetQuota(const nsACString &domain)

Why pass PRInt64s around when prefs can only support 32-bit values?

>-  nsAutoString value;
>-  rv = gStorageDB->SetKey(mDomain, aKey, aValue, aSecure);
>+  // Get the current domain for quota enforcement
>+  nsCOMPtr<nsIPrincipal> subjectPrincipal;
>+  nsContentUtils::GetSecurityManager()->
>+    GetSubjectPrincipal(getter_AddRefs(subjectPrincipal));
>+
>+  nsCAutoString currentDomain;
>+
>+  if (subjectPrincipal) {
>+    nsCOMPtr<nsIURI> uri;
>+    rv = subjectPrincipal->GetURI(getter_AddRefs(uri));
>+
>+    if (NS_SUCCEEDED(rv) && uri) {
>+      uri->GetAsciiHost(currentDomain);
>+    }
>+  }
>+

What if subjectPrincipal is null? This may be the case when called from c++ as should probably use mDomain instead.

>+  rv = gStorageDB->SetKey(mDomain, aKey, aValue, aSecure,
>+                          NS_ConvertUTF8toUTF16 (currentDomain),
>+                          GetQuota (currentDomain));

Remove the extra spaces before the parantheses.

GetDBValue(const nsAString& aKey, nsAString& aValue, PRBool* aSecure);
>+  GetDBValue(const nsAString& aKey,
>+             nsAString& aValue,
>+             PRBool* aSecure,
>+             nsAString& owner);
> 

Use 'aOwner' not 'owner' here

>+           NS_LITERAL_CSTRING("CREATE TABLE webappsstore ("
>                               "domain TEXT, "
>                               "key TEXT, "
>                               "value TEXT, "
>-                              "secure INTEGER) "));
>+                              "secure INTEGER, "
>+                              "owner STRING)"));

I don't think 'STRING' does anything different than  'TEXT', so let's be consistent here?


>+      // if the index can't be created, there are dup domain/key combos
>+      // in moz_webappstore2, which indicates a bug elsewhere.  Fail to upgrade
>+      // in this case
>+      if (!NS_FAILED(rv)) {

Use:
  if (NS_SUCCEEDED(rv))

>+          rv = mConnection->ExecuteSimpleSQL(
>+                 NS_LITERAL_CSTRING("INSERT OR IGNORE INTO "
>+                                    "webappsstore(domain, key, value, secure) "
>+                                    "SELECT domain, key, value, secure "
>+                                    "FROM moz_webappsstore"));

Should the owner be left blank? Shouldn't it be set to domain?

> nsDOMStorageDB::SetKey(const nsAString& aDomain,
>                        const nsAString& aKey,
>                        const nsAString& aValue,
>-                       PRBool aSecure)
>+                       PRBool aSecure,
>+                       const nsAString& aOwner,
>+                       PRInt64 aQuota)
> {
>   mozStorageStatementScoper >scope(mGetKeyValueStatement);
>+ 
>+  PRInt64 usage;
>+  nsresult rv;
>+  if (!aOwner.IsEmpty()) {

usage isn't initialized to anything if aOwner is empty.

Comment 15

[Dave Camp (:dcamp)]

Attachment: another cut

Comment 16

[Neil Deakin]

Comment on attachment 250905 [details] [diff] [review]
another cut

>@@ -655,18 +668,40 @@ nsDOMStorage::SetDBValue(const >+  // Get the current domain for quota enforcement
>+  nsCOMPtr<nsIPrincipal> subjectPrincipal;
>+  nsContentUtils::GetSecurityManager()->
>+    GetSubjectPrincipal(getter_AddRefs(subjectPrincipal));
>+
>+  nsAutoString currentDomain;
>+
>+  if (subjectPrincipal) {
>+    nsCOMPtr<nsIURI> uri;
>+    rv = subjectPrincipal->GetURI(getter_AddRefs(uri));
>+
>+    if (NS_SUCCEEDED(rv) && uri) {
>+        nsCAutoString currentDomainAscii;
>+        uri->GetAsciiHost(currentDomainAscii);
>+        currentDomain = NS_ConvertUTF8toUTF16(currentDomainAscii);
>+    }
>+  }
>+  
>+  if (currentDomain.IsEmpty()) {
>+      currentDomain = mDomain;
>+  }
>+

Actually, I meant just do this when subjectPrincipal is null. (change this last condition to an else clause)

If the host is null, it should probably just throw a security exception.

Otherwise, it looks ok.

Comment 17

[Dave Camp (:dcamp)]

Attachment: throw security error on empty host

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 251304 [details] [diff] [review]
throw security error on empty host

Looks all good to me. Only a couple of whitespace issues caught my eye when reading this patch.

- In nsDOMStorage::SetDBValue():

+  if (subjectPrincipal) {
+    nsCOMPtr<nsIURI> uri;
+    rv = subjectPrincipal->GetURI(getter_AddRefs(uri));
+
+    if (NS_SUCCEEDED(rv) && uri) {
+        nsCAutoString currentDomainAscii;
+        uri->GetAsciiHost(currentDomainAscii);
+        currentDomain = NS_ConvertUTF8toUTF16(currentDomainAscii);
+    }
+    
+    if (currentDomain.IsEmpty()) {
+        return NS_ERROR_DOM_SECURITY_ERR;
+    }
+  } else {
+      currentDomain = mDomain;
+  }

Fix the mixed 2 and 4 space indentation.

- In nsDOMStorageList::NamedItem():

       if (!nsDOMStorage::CanUseStorage(uri, &sessionOnly))
         return NS_ERROR_DOM_SECURITY_ERR;
-
+      

No need to add this empty whitespace.

sr=jst

Comment 19

[Dave Camp (:dcamp)]

Attachment: fixed whitespace

Comment 20

[Johnny Stenback (:jst)]

Dave's fix landed on the trunk. Marking bug FIXED.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This doesn't prevent a site from getting unlimited storage by setting up virtual domains like domain1.site.com, domain2.site.com, etc, right?

Do we care?

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(What I think we probably should do is hook this up to the effective TLD service and assign storage quotas to effective TLDs:
http://wiki.mozilla.org/Gecko:Effective_TLD_Service
)

Comment 23

[Brad Neuberg]

Can you choose a suitable default size for per site storage (the WHAT WG spec recommends 5 megabytes as a suggestion), and then make sure to have a UI to allow the user to approve going beyond this for each order of magnitude increase? _please_ get this right, so we have it baked into the browser in a good way -- Flash has had this ability for several years now, with the LocalSharedObjects, and it has worked well. Flash gives you 100K without prompting, per site. Then, at 100K, you get a permission dialog:

http://www.gness.com/distrust/img/flash_gen.png

Then, for each order of magnitude increase in size needed (1MB, 10MBs, etc.) the user is reprompted.

I suggest you pretty much clone the Flash UI for this. 

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Permission dialogs are generally useless. Users either don't understand what they're being asked or don't care --- usually both.

As an alternative, I suggest limiting regular sites to some default value. If the user has bookmarked any pages at the site, increase the quota to some higher value. Alternatively, increase the quota based on the number of visits to the site. It might make sense for the quota sizes to be related to disk size.

This discussion should move out of this bug, to a new bug or perhaps to a newsgroup.

Comment 25

[Daniel Veditz [:dveditz]]

(In reply to comment #21)
> This doesn't prevent a site from getting unlimited storage by setting up
> virtual domains like domain1.site.com, domain2.site.com, etc, right?
> 
> Do we care?

Yes, we care: the storage spec explicitly warns about this in a "User agent should consider additional quota mechanisms" paragraph
http://www.whatwg.org/specs/web-apps/current-work/#disk-space

We can care in a different bug, though, and not make this stronger "User agent should not" requirement wait.

Comment 26

[Daniel Veditz [:dveditz]]

filed bug 367373 to cover comment 25

Comment 27

[Jay Patel [:jay]]

Comment on attachment 251851 [details] [diff] [review]
fixed whitespace

Approved for 1.8 branch, a=jay for drivers.

Comment 28

[Johnny Stenback (:jst)]

Fixed for 1.8.

Comment 29

[Jay Patel [:jay]]

Could not find a real world testcase to verify this fix, but Dave tested this with the latest 2.0.0.2 RC1 and verified it works with his set of unit tests.

If anyone finds a web app that uses this feature, please let us know so we can retest.  Thanks!

Comment 30

[Andrew Wooldridge]

Question:

A website has multiple options in setting a storage domain such as 
globalStorage['foo.com']
globalStorage['com']
globalStorage[''] 

How does this limit effect that? I have a web application which uses both a local domain ('mysite.com') and needs also to write to the "global" domain ('') in order to share information with another site (also using the application)- will this 5mb limit span those domains or is it per domain?

Comment 31

[Neil Deakin]

Neither globalStorage['com'] nor globalStorage[''] are supported, so it doesn't affect them at all. ;)

Comment 32

[Neil Deakin]

Bug 371127 added a test for this