Bug 157010 - QuotaExceededError when saving to localStorage in private mode
Summary: QuotaExceededError when saving to localStorage in private mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: Safari 9
Hardware: All All
: P2 Major
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-04-25 17:51 PDT by Dima Voytenko
Modified: 2017-08-18 07:05 PDT (History)
14 users (show)

See Also:


Attachments
Patch (45.25 KB, patch)
2017-04-12 14:44 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (45.25 KB, patch)
2017-04-12 15:28 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dima Voytenko 2016-04-25 17:51:53 PDT
`localStorage.setItem` fails in private mode with `QuotaExceededError`. It looks the original bug item for this was https://bugs.webkit.org/show_bug.cgi?id=49329

This is now a clear and "recommended" way for site owners to detect user's private browsing preferences and could be considered a significant privacy issue. See for instance:

http://apple.stackexchange.com/questions/131587/how-can-a-web-site-determine-if-safari-private-browsing-is-turned-on
http://stackoverflow.com/questions/9659103/how-to-detect-users-on-an-iphone-with-private-browsing-enabled

It looks like Safari is the last remaining major browser that still allows leaking of private mode via this approach. The original bug item cited above does not discuss privacy implications. Is it possible to address?
Comment 1 Rick Byers 2016-04-25 18:29:13 PDT
Yeah this is probably bad, will likely lead to some sketchy websites refusing to work (or otherwise complaining to the user) in private mode.  I just filed a related issue on blink: https://crbug.com/606633
Comment 2 Radar WebKit Bug Importer 2016-04-26 19:07:52 PDT
<rdar://problem/25947768>
Comment 3 Brady Eidson 2016-04-27 09:03:31 PDT
I agree this is less than ideal.

However, "pretending to store something permanently" sounds pretty bad, too.

Perhaps you have ideas on how we could both:
1 - Maintain the contract that localStorage is durable
2 - Not store things on localStorage in private browsing?
Comment 4 Dima Voytenko 2016-04-27 16:33:50 PDT
I believe other browsers simply make localStorage equivalent sessionStorage.
Comment 5 Brady Eidson 2016-04-27 16:48:32 PDT
(In reply to comment #4)
> I believe other browsers simply make localStorage equivalent sessionStorage.

Not entirely accurate, as I assume it's still global...

When we implement an API we try to think it through and reflect on the contract it represents.

localStorage is supposed to be persistent.

So, yes, I already understood that other browsers ignore that constraint.
Comment 6 Dima Voytenko 2016-04-27 17:07:09 PDT
The issue with just ignoring writes is that it's no different really from throwing an exception from the point of view of private mode leak. E.g.

```
function isPrivateMode() {
  try {
    localStorage.setItem('a', 'b');
    return false;
  } catch(e) {
    return true;
  }
}
```

vs

```
function isPrivateMode() {
  localStorage.setItem('a', 'b');
  return localStorage.getItem('a') != 'b';
}
```
Comment 7 Dima Voytenko 2016-04-27 18:17:59 PDT
Also, re:permanent storage. From the page author's point of view, they can only judge permanence of storage per user, which they can alternatively track via cookies. So, resetting localStorage when cookies are reset (as when they close private mode) makes sense.
Comment 8 Rick Byers 2016-04-27 19:04:37 PDT
Right, in chromium I believe the model is that incognito mode is like a separate user profile that is automatically removed when the window is closed.  This doesn't break the API contract around "durability" any more than a user explicitly clearing their browser state (the next visit is like a new user again).
Comment 9 Darin Adler 2016-04-28 08:41:05 PDT
We do understand that Chrome’s Incognito model is "temporary user profile that is removed when window is closed".

Our concern for the Private Browsing features that pre-date Incognito was that if a website stored important, document-like data in localStorage, this because could be a problem, not for the website developer, but for the web browser user. Even though the person is using a private browsing mode they might not understand that closing a window or quitting the web browser has a side effect of deleting all their documents. It seemed that getting an error earlier would help prevent someone from building up some work that then is unexpectedly lost.

In practice, the WebKit engineers may be misunderstanding the type of data that websites would store in localStorage. Or misunderstanding the expectations people will bring to private browsing features.
Comment 10 Brady Eidson 2016-04-28 09:34:52 PDT
(In reply to comment #6)
> The issue with just ignoring writes is that it's no different really from
> throwing an exception from the point of view of private mode leak. E.g.
> 
> ```
> function isPrivateMode() {
>   try {
>     localStorage.setItem('a', 'b');
>     return false;
>   } catch(e) {
>     return true;
>   }
> }
> ```
> 
> vs
> 
> ```
> function isPrivateMode() {
>   localStorage.setItem('a', 'b');
>   return localStorage.getItem('a') != 'b';
> }
> ```

I never suggested ignoring writes. Of course that's silly.
Comment 11 Brady Eidson 2016-04-28 09:48:12 PDT
(In reply to comment #7)
> Also, re:permanent storage. From the page author's point of view, they can
> only judge permanence of storage per user, which they can alternatively
> track via cookies. So, resetting localStorage when cookies are reset (as
> when they close private mode) makes sense.

We thought about this very carefully when we implemented localStorage for the browser before anybody but the Gears plugin

Cookies are a completely different class of storage from localStorage.

Browsers are allowed to make all cookies session cookies, ditch cookies in FIFO order to make room for new ones, ditch big cookies first, ditch cookies in any arbitrary order, decide not to store a cookie "just because it looks weird", etc etc, and still match the spirit of the spec.

LocalStorage was meant to be durable.

All of this said, it's a shame that none of the other browsers considered this the same way.

Note, I'm not *defending* how our behavior reveals private browsing state. Of course that's terrible.

I am lamenting that nobody worked together to try to come up with something better.
Comment 12 Brady Eidson 2016-04-28 09:49:25 PDT
Let me posit this:

If the localStorage area is legitimately full, we're legitimately expected to throw a QuotaExceededError exception.

How can we possibly distinguish that from the "oh, this is webkit in private browsing" detection that is rampant?
Comment 13 Dima Voytenko 2016-04-28 10:14:05 PDT
> If the localStorage area is legitimately full, we're legitimately expected to throw a QuotaExceededError exception.

This probably has to do with probability. I assume page authors simply judge that "if it's a Safari and localStorage.setItem() fails with QuotaExceededError, then it's most likely a private mode". As in "false positives are possible, but rare enough". Obviously, this probably makes things a bit worse for the user.

> nobody worked together to try to come up with something better

I'm not super familiar with the standards process for this kind of spec. It seems to me that in this day and age some discussion of how an API should behave in private mode has to be part of the spec. My quick search shows that it isn't at this time which is disappointing. Would you consider talking to the spec authors to hear their recommendations?
Comment 14 Dima Voytenko 2016-04-28 10:26:59 PDT
For posterity, I filed https://github.com/whatwg/html/issues/1143
Comment 15 Brady Eidson 2016-04-28 11:16:46 PDT
(In reply to comment #13)
> 
> Would you consider talking to the spec authors to hear their recommendations?

I was one of the spec authors. Not listed as an editor, but plenty of feedback given while implementing.

But, it's true - back in 2007-2008 we didn't automatically talk about private browsing, as Safari was one of the few that had it, and IE, for example, didn't.
Comment 16 Dima Voytenko 2016-04-28 13:16:59 PDT
Awesome! I posted above, I filed a bug against the spec: https://github.com/whatwg/html/issues/1143

Looking forward hearing what you decide.
Comment 17 test 2016-07-13 04:54:32 PDT
Safari 10 and Safari Technology Preview 8 not fixed
Comment 18 Brian Burg 2016-11-11 11:19:17 PST
Per discussion with Brady, the fix will be to make localStorage in-memory for ephemeral sessions. This means that in private browsing and automation (i.e., WebDriver), we'll allow read/write and the contents do not persist.
Comment 19 Dima Voytenko 2016-11-11 17:12:30 PST
Awesome! Thanks a lot!
Comment 20 Brady Eidson 2017-04-12 14:44:44 PDT
Created attachment 306941 [details]
Patch
Comment 21 Alex Christensen 2017-04-12 14:50:53 PDT
Comment on attachment 306941 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=306941&action=review

> Source/WebCore/loader/EmptyClients.cpp:680
> +RefPtr<StorageNamespace> EmptyStorageNamespaceProvider::createEphemeralLocalStorageNamespace(Page&, unsigned)

Why doesn't this return a Ref?

> Source/WebKit/Storage/StorageNamespaceImpl.cpp:103
> +    for (auto iter : m_storageAreaMap)

Do we need auto& here?

> Source/WebKit2/WebProcess/Storage/StorageNamespaceImpl.cpp:105
> +    RefPtr<EphemeralStorageArea> copy()

Ref
Comment 22 Brady Eidson 2017-04-12 15:27:22 PDT
(In reply to Alex Christensen from comment #21)
> Comment on attachment 306941 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306941&action=review
> 
> > Source/WebCore/loader/EmptyClients.cpp:680
> > +RefPtr<StorageNamespace> EmptyStorageNamespaceProvider::createEphemeralLocalStorageNamespace(Page&, unsigned)
> 
> Why doesn't this return a Ref?

Because the others don't, and I didn't want to expand the scope of the patch.

> 
> > Source/WebKit/Storage/StorageNamespaceImpl.cpp:103
> > +    for (auto iter : m_storageAreaMap)
> 
> Do we need auto& here?

Sure.

> 
> > Source/WebKit2/WebProcess/Storage/StorageNamespaceImpl.cpp:105
> > +    RefPtr<EphemeralStorageArea> copy()
> 
> Ref

We'll see.
Comment 23 Brady Eidson 2017-04-12 15:28:42 PDT
Created attachment 306946 [details]
Patch
Comment 24 WebKit Commit Bot 2017-04-12 23:38:22 PDT
Comment on attachment 306946 [details]
Patch

Clearing flags on attachment: 306946

Committed r215315: <http://trac.webkit.org/changeset/215315>
Comment 25 WebKit Commit Bot 2017-04-12 23:38:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 David Kilzer (:ddkilzer) 2017-04-13 02:49:26 PDT
<rdar://problem/19197190>
Comment 27 Mahaveer 2017-08-18 04:48:51 PDT
I have updated my mac OS X EI Capitan(10.11.6) safari version to 10.1.2(11603.3.8) but still in browser private mode getting the same error. Can anybody please tell me how could i resolve this?
Comment 28 Brady Eidson 2017-08-18 07:05:43 PDT
(In reply to Mahaveer from comment #27)
> I have updated my mac OS X EI Capitan(10.11.6) safari version to
> 10.1.2(11603.3.8) but still in browser private mode getting the same error.
> Can anybody please tell me how could i resolve this?

AFAIK, nobody claimed this would be fixed on El Cap 10.11.6 or in Safari 10.1.2 on any platform.

Have you tried in a WebKit nightly, Safari Technology Preview, or a macOS High Sierra beta?