Closed
Bug 362446
Opened 18 years ago
Closed 18 years ago
webapps storage should have size limits on the storage allowed per site
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bzbarsky, Assigned: enndeakin)
References
Details
(Keywords: verified1.8.1.2, Whiteboard: [needs testcase])
Attachments
(1 file, 6 obsolete files)
24.67 KB,
patch
|
jay
:
approval1.8.1.2+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•18 years ago
|
Flags: blocking1.9?
Reporter | ||
Comment 1•18 years ago
|
||
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?
Updated•18 years ago
|
Assignee: general → enndeakin
Comment 2•18 years ago
|
||
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-
Assignee | ||
Comment 4•18 years ago
|
||
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•18 years ago
|
||
*** Bug 363517 has been marked as a duplicate of this bug. ***
Comment 6•18 years ago
|
||
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•18 years ago
|
||
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)
Updated•18 years ago
|
Flags: blocking1.8.1.2+
Assignee | ||
Comment 8•18 years ago
|
||
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•18 years ago
|
||
(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. >
Assignee | ||
Comment 10•18 years ago
|
||
Dave, are you working on a newer patch for this bug?
Comment 11•18 years ago
|
||
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)
Assignee | ||
Comment 12•18 years ago
|
||
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•18 years ago
|
||
ok, new one creates webappsstore
Attachment #250874 -
Flags: review?(enndeakin)
Assignee | ||
Comment 14•18 years ago
|
||
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-
Comment 15•18 years ago
|
||
Attachment #247463 -
Attachment is obsolete: true
Attachment #249184 -
Attachment is obsolete: true
Attachment #250874 -
Attachment is obsolete: true
Attachment #250905 -
Flags: review?(enndeakin)
Assignee | ||
Comment 16•18 years ago
|
||
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+
Comment 17•18 years ago
|
||
Attachment #250905 -
Attachment is obsolete: true
Attachment #251304 -
Flags: superreview?(jst)
Comment 18•18 years ago
|
||
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+
Comment 19•18 years ago
|
||
Attachment #251304 -
Attachment is obsolete: true
Attachment #251851 -
Flags: approval1.8.1.2?
Comment 20•18 years ago
|
||
Dave's fix landed on the trunk. Marking bug FIXED.
Status: NEW → RESOLVED
Closed: 18 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 )
Comment 23•18 years ago
|
||
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.
Comment 25•18 years ago
|
||
(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•18 years ago
|
||
filed bug 367373 to cover comment 25
Comment 27•18 years ago
|
||
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+
Updated•17 years ago
|
Keywords: fixed1.8.1.2 → verified1.8.1.2
Comment 29•17 years ago
|
||
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]
Comment 30•17 years ago
|
||
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?
Assignee | ||
Comment 31•17 years ago
|
||
Neither globalStorage['com'] nor globalStorage[''] are supported, so it doesn't affect them at all. ;)
Reporter | ||
Updated•17 years ago
|
Flags: blocking1.9? → in-testsuite?
Assignee | ||
Comment 32•17 years ago
|
||
Bug 371127 added a test for this
Flags: in-testsuite? → in-testsuite+
Updated•11 years ago
|
Component: DOM: Mozilla Extensions → DOM
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•