Closed Bug 362446 Opened 14 years ago Closed 14 years ago

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

Categories

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

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: enndeakin)

References

Details

(Keywords: verified1.8.1.2, Whiteboard: [needs testcase])

Attachments

(1 file, 6 obsolete files)

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.
Flags: blocking1.9?
Blocks: 335540
Needless to say, the problem is that this allows websites to fill up the user's hard drive if they choose to be annoying.
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.9?
Assignee: general → enndeakin
Too late for 1.8.0.9/1.8.1.1, let's try to get a handle on this for the next one.
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1-
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9-
Actually, not an issue for 1.8.0.
Flags: wanted1.8.0.x+
Attached patch simple implementation (obsolete) — Splinter Review
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.
*** Bug 363517 has been marked as a duplicate of this bug. ***
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 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...
Attachment #248418 - Flags: review?(enndeakin)
Flags: blocking1.8.1.2+
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.
(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.

> 

Dave, are you working on a newer patch for this bug?
Attached patch another cut (obsolete) — Splinter Review
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.
Attachment #248418 - Attachment is obsolete: true
Attachment #248418 - Flags: review?(enndeakin)
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).
Attached patch create new table (obsolete) — Splinter Review
ok, new one creates webappsstore
Attachment #250874 - Flags: review?(enndeakin)
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.
Attachment #250874 - Flags: review?(enndeakin) → review-
Attached patch another cut (obsolete) — Splinter Review
Attachment #247463 - Attachment is obsolete: true
Attachment #249184 - Attachment is obsolete: true
Attachment #250874 - Attachment is obsolete: true
Attachment #250905 - Flags: review?(enndeakin)
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.
Attachment #250905 - Flags: review?(enndeakin) → review+
Attachment #250905 - Attachment is obsolete: true
Attachment #251304 - Flags: superreview?(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
Attachment #251304 - Flags: superreview?(jst) → superreview+
Attached patch fixed whitespaceSplinter Review
Attachment #251304 - Attachment is obsolete: true
Attachment #251851 - Flags: approval1.8.1.2?
Dave's fix landed on the trunk. Marking bug FIXED.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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?
(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
)
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. 
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.
(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 on attachment 251851 [details] [diff] [review]
fixed whitespace

Approved for 1.8 branch, a=jay for drivers.
Attachment #251851 - Flags: approval1.8.1.2? → approval1.8.1.2+
Fixed for 1.8.
Keywords: fixed1.8.1.2
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!
Whiteboard: [needs testcase]
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?

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


Flags: blocking1.9? → in-testsuite?
Bug 371127 added a test for this
Flags: in-testsuite? → in-testsuite+
Component: DOM: Mozilla Extensions → DOM
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.