Implement clearing of LocalStorage in browsingData API

VERIFIED FIXED in Firefox -esr52

Status

enhancement
P5
normal
VERIFIED FIXED
2 years ago
11 months ago

People

(Reporter: wisniewskit, Assigned: wisniewskit)

Tracking

({dev-doc-complete})

unspecified
mozilla57
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr5257+ verified, firefox57 verified, firefox58 verified, firefox59 verified)

Details

(Whiteboard: [browsingData]triaged)

Attachments

(3 attachments)

Assignee

Description

2 years ago
+++ This bug was initially created as a clone of Bug #1333050 +++

Chrome's browsingData API allows for clearing of IndexedDB and LocalStorage for all visited sites. This has not yet been implemented in Firefox. However, the LocalStorage case will require further changes to support the "since" case. As such the localStorage case has been split out into this bug, so that we can land support for the indexedDB case separately.

Comment 1

2 years ago
I think we can implement localStorage without support for "since", it would really be helpful.
Assignee

Comment 2

2 years ago
I'm for it, but :bsilverman said in bug 1333050 comment 4 that he'd rather that we don't implement it without support for "since". Bob, are you okay with implementing it halfway now?
Flags: needinfo?(bob.silverberg)
I could go either way. If there's a feeling that it would be valuable without "since" support then I suppose it's okay to implement it that way now. It's doubtful the work to support "since" in localStorage will happen anytime soon.
Flags: needinfo?(bob.silverberg)
Assignee

Comment 4

2 years ago
Tim, I'm guessing that addons like Self-Destructing Cookies would benefit from this feature even without "since"? What do you think?
Flags: needinfo?(ntim.bugs)

Comment 5

2 years ago
(In reply to Thomas Wisniewski from comment #4)
> Tim, I'm guessing that addons like Self-Destructing Cookies would benefit
> from this feature even without "since"? What do you think?

Yes, totally agree. I would implement the feature without 'since' here, then file another bug to get 'since' support implemented.
Flags: needinfo?(ntim.bugs)
Assignee

Comment 6

2 years ago
Bob, would :ntim's agreement in comment 5 be enough, or should we discuss this further before I write a patch to implement localStorage-clearing without "since"?
Flags: needinfo?(bob.silverberg)
I think it's fine to go ahead. Please also open a bug that depends on this bug which asks to add "since" support as Tim suggested in comment 5.
Flags: needinfo?(bob.silverberg)
Comment hidden (mozreview-request)
Assignee

Comment 9

2 years ago
Here is my attempt at this; hopefully it's reasonably close to acceptable.

Shane, I'm not 100% sure who would be best to review the localStorage-related code here... would you have any ideas?

I will create related bugs for adding "since" and "hostnames" support for localStorage soon.

Comment 10

2 years ago
\o/ Thanks!
Comment on attachment 8883427 [details]
Bug 1355576 - Add ability to clear all localStorage with the browsingData API;

Bob should review browsingData changes.
Attachment #8883427 - Flags: review?(bob.silverberg)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8883427 [details]
Bug 1355576 - Add ability to clear all localStorage with the browsingData API;

https://reviewboard.mozilla.org/r/154330/#review159662

::: browser/components/extensions/ext-browsingData.js:121
(Diff revision 1)
> +  }
> +
> +  let stGetScopes;
> +  try {
> +    stGetScopes =
> +      db.createStatement("SELECT DISTINCT scope FROM webappsstore2 LIMIT -1");

I don't like db code like this in webext API land.  At the very least this should be in a localstorage api somewhere.

Comment 13

2 years ago
mozreview-review
Comment on attachment 8883427 [details]
Bug 1355576 - Add ability to clear all localStorage with the browsingData API;

https://reviewboard.mozilla.org/r/154330/#review160030

This is a good start, Thomas. Thanks!

Just a few nits and suggestions for changes to the test, but the biggest issue is that we need someone to review the localStorage code, and maybe they can point you to something that already exists in m-c to do this, or somewhere that code could be added to support this.

::: browser/components/extensions/ext-browsingData.js:85
(Diff revision 1)
>  
>  const clearHistory = options => {
>    return sanitizer.items.history.clear(makeRange(options));
>  };
>  
> +function* LocalStorageIterator() {

I agree with Shane. Ideally this would use code that exists elsewhere in the tree that interacts with localStorage, and if no such thing exists, and there's no good place for this code, it should at least be extracted out into a module.

It sounds like the top priority is to find a localStorage peer to review this part of the code, but I'm not sure who that is.

::: browser/components/extensions/ext-browsingData.js:143
(Diff revision 1)
> +    stGetScopes.finalize();
> +    db.asyncClose();
> +  }
> +}
> +
> +let clearLocalStorage = Task.async(function* (options) {

This should be `const clearLocalStorage = async function(options) {`, similar to other functions below it.

::: browser/components/extensions/test/browser/browser_ext_browsingData_localStorage.js:5
(Diff revision 1)
> +function setupLocalStorage(browser) {
> +  return ContentTask.spawn(browser, null, function* () {
> +    content.localStorage.setItem("key", "value");
> +  });
> +}
> +
> +function checkLocalStorageExists(browser) {
> +  return ContentTask.spawn(browser, null, function* () {
> +    return content.localStorage.getItem("key") === "value";
> +  });
> +}

Do you need to do this, or could the setting and checking of localStorage be done via a content script in the test extension?

::: browser/components/extensions/test/browser/browser_ext_browsingData_localStorage.js:38
(Diff revision 1)
> +    const host1 = "http://example.com";
> +    let tab1 = await BrowserTestUtils.openNewForegroundTab(gBrowser, host1);
> +    let browser1 = gBrowser.getBrowserForTab(tab1);
> +
> +    const host2 = "http://example.net";
> +    let tab2 = await BrowserTestUtils.openNewForegroundTab(gBrowser, host2);
> +    let browser2 = gBrowser.getBrowserForTab(tab2);

Maybe this stuff could be done in the test extension's background script, via the `tabs` API, rather than in the test?

::: browser/components/extensions/test/browser/browser_ext_browsingData_localStorage.js:46
(Diff revision 1)
> +    await setupLocalStorage(browser1);
> +    await setupLocalStorage(browser2);

As per my comment above, if you had a content script you could send it a message to set the localStorage at this point.

::: browser/components/extensions/test/browser/browser_ext_browsingData_localStorage.js:56
(Diff revision 1)
> +    todo(await checkLocalStorageExists(browser1), true);
> +    todo(await checkLocalStorageExists(browser2), false);

Why would one of these be true and one false at this point?

::: browser/components/extensions/test/browser/browser_ext_browsingData_localStorage.js:63
(Diff revision 1)
> +    tab1 = await BrowserTestUtils.openNewForegroundTab(gBrowser, host1);
> +    browser1 = gBrowser.getBrowserForTab(tab1);
> +    tab2 = await BrowserTestUtils.openNewForegroundTab(gBrowser, host2);
> +    browser2 = gBrowser.getBrowserForTab(tab2);

I'm not clear on why you are creating and destroying tabs at different points in this test. Couldn't you just keep the tabs around for the entire length of the test? Also, as above, I think this might be cleaner if you used the `tabs` API for this.
Attachment #8883427 - Flags: review?(bob.silverberg) → review-
Comment hidden (mozreview-request)
Assignee

Comment 15

2 years ago
I've taken another stab at this, and found a more solid way to clear the localStorage by hooking up a new Services.obs.notifyObservers signal handler, which does what the cookie-changed/cookie-cleared topics do, but only for localStorage (as the cookie ones are overloaded to also clear cookies and sessionStorage).

This approach also clears the cached localStorages for open tabs, which lets me revise the test to not bother closing/reopening tabs. I've addressed your other review notes as well (removing the "since" code entirely, as it might as well be added in the ticket that adds support for "since").

Comment 16

2 years ago
I know everyone has their busy lives, but the FF57 merge date is in less than two weeks. Users really need an alternative to Self Destructing Cookies (discontinued), which is what CAD offers:

https://github.com/mrdokenny/Cookie-AutoDelete/issues/44

For me participating in further Nightly channel is dependent on the "this" bug and https://bugzilla.mozilla.org/show_bug.cgi?id=1340511

The missing features of browsingData API are privacy essential. A key subject of Mozilla is privacy. Which is why I usually visit these two trackers regularly. 

Please I beg you to review Mr Wisniewski's attachment so there is a realistic chance FF57 will allow users to destroy Evercookies automatically with the webextension I've linked above, the code relies on these bugs.
(In reply to Thomas Wisniewski from comment #15)
> I've taken another stab at this, and found a more solid way to clear the
> localStorage by hooking up a new Services.obs.notifyObservers signal
> handler, which does what the cookie-changed/cookie-cleared topics do, but
> only for localStorage (as the cookie ones are overloaded to also clear
> cookies and sessionStorage).
> 
> This approach also clears the cached localStorages for open tabs, which lets
> me revise the test to not bother closing/reopening tabs. I've addressed your
> other review notes as well (removing the "since" code entirely, as it might
> as well be added in the ticket that adds support for "since").

I see you updated the patch, but didn't respond to or mark any of my issues in MozReview. It makes re-reviewing easier if I can see which of my issues have been fixed and/or dropped. Would you mind doing that Thomas?
Flags: needinfo?(wisniewskit)
Assignee

Comment 18

2 years ago
Sure, they were all basically "fixed", so I've marked them as such (the patch uses a different technique to clear LS, and I corrected the other issues with the test/etc). I just can't be 100% sure if this new approach is the best way to do things without review from an appropriate peer. It seems to me that it's much closer than the first patch, though.
Flags: needinfo?(wisniewskit)

Comment 19

2 years ago
mozreview-review
Comment on attachment 8883427 [details]
Bug 1355576 - Add ability to clear all localStorage with the browsingData API;

https://reviewboard.mozilla.org/r/154330/#review164760

This looks good to me, Thomas, as far as the WebExtensions part goes. Someone else should review the code that actually clears the storage. Just one small comment about the test, but r+ otherwise. Nice work.

::: browser/components/extensions/test/browser/browser_ext_browsingData_localStorage.js:59
(Diff revision 2)
> +
> +  function content() {
> +    browser.runtime.onMessage.addListener(msg => {
> +      if (msg === "resetLocalStorage") {
> +        localStorage.clear();
> +        localStorage.setItem("test", "test");

Might it be worthwhile to check localStorage at some point to make sure the value you set is actually there before clearing? Otherwise the check in `checkLocalStorageCleared` could be a false positive.
Attachment #8883427 - Flags: review?(bob.silverberg) → review+
Comment hidden (mozreview-request)
Assignee

Comment 21

2 years ago
Alright, I've added that check. I also added :janv to the list of reviewers, in the hopes that he can at least direct us to a DOM Storage peer who can take a look.

Jan, sorry that I forgot to add a more detailed comment in the commit message, but could you please review the non-WebExtension localStorage changes here to see if they pass muster?
Flags: needinfo?(jvarga)
(In reply to Shane Caraveo (:mixedpuppy) from comment #12)
> Comment on attachment 8883427 [details]
> Bug 1355576 - Add ability to clear all localStorage with the browsingData
> API;
> 
> https://reviewboard.mozilla.org/r/154330/#review159662
> 
> ::: browser/components/extensions/ext-browsingData.js:121
> (Diff revision 1)
> > +  }
> > +
> > +  let stGetScopes;
> > +  try {
> > +    stGetScopes =
> > +      db.createStatement("SELECT DISTINCT scope FROM webappsstore2 LIMIT -1");
> 
> I don't like db code like this in webext API land.  At the very least this
> should be in a localstorage api somewhere.

I'm glad that you removed the SQL statement, it's really not a good idea to do it directly here.
I'm looking at the patch now.
Flags: needinfo?(jvarga)

Comment 23

2 years ago
mozreview-review
Comment on attachment 8883427 [details]
Bug 1355576 - Add ability to clear all localStorage with the browsingData API;

https://reviewboard.mozilla.org/r/154330/#review170568

Looks good.
Attachment #8883427 - Flags: review?(jvarga) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 28

2 years ago
Thanks a bunch, janv! The patch still applies cleanly to moz-central, and a try-run looks fine (aside from an eslint issue that I fixed): https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e3bd233a12eb4dc783b12b0d5df828278e27246

Requesting check-in.



PS: Sorry for the review-spam, I didn't realize that "mach eslint" would alter a file that would become part of the commit, and I stumbled around reverting it :S).
Keywords: checkin-needed

Comment 29

2 years ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5020a2dcb8a7
Add ability to clear all localStorage with the browsingData API; r=bsilverberg,janv
Keywords: checkin-needed

Updated

2 years ago
Assignee: nobody → wisniewskit
Comment hidden (mozreview-request)
Assignee

Comment 32

2 years ago
This was a trivial fix; I had just missed an unexpected sanity test (localStorage is now supported, so the test failed). I'm doing a new try-run here in case I missed something else: https://treeherder.mozilla.org/#/jobs?repo=try&revision=49524974de5f2342c273d4badba426c027cd8b5e
Assignee

Comment 33

2 years ago
The try-run isn't showing anything else that I can see would be affected by this patch, so I'm re-requesting check-in with the latest version.
Keywords: checkin-needed

Comment 34

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1da9f146635c
Add ability to clear all localStorage with the browsingData API; r=bsilverberg,janv
Keywords: checkin-needed

Comment 35

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1da9f146635c
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee

Comment 36

2 years ago
It just occurred to me that we might want to also have this API throw "NotImplementedError" when the caller tries to use it with the "since" option, as right now it will just clear *all* of the localStorages instead regardless of what they pass in for "since". Bob, do you agree? If so, should I just make a new ticket for that or reopen this one and add a quick patch?
Flags: needinfo?(bob.silverberg)
Assignee

Updated

2 years ago
Blocks: 1388428
(In reply to Thomas Wisniewski from comment #36)
> It just occurred to me that we might want to also have this API throw
> "NotImplementedError" when the caller tries to use it with the "since"
> option, as right now it will just clear *all* of the localStorages instead
> regardless of what they pass in for "since". Bob, do you agree? If so,
> should I just make a new ticket for that or reopen this one and add a quick
> patch?

This is an interesting question. There are two other categories that do not implement `since`: cache and serviceWorkers. Cache also does not support `since` on Chrome, and we have documented that on MDN [1], but it's unclear whether `since` is supported on Chrome for serviceWorkers, and we do not seem to have documented the fact that it's unsupported on Firefox. [2]

We could simply document that `since` is not supported for localStorage, but if it is supported on Chrome (the documentation [3] doesn't indicate either way, but I would assume it does based on that) then maybe we do need to throw an exception if someone tries to pass a value for `since` when clearing localStorage. Because a value of 0 for `since` is the same as no value, we should probably allow that, but if they try to pass a non-zero value it might make sense to throw.

I'm not really sure which way we should go on that, so I'm needinfoing Andy to see what he thinks.


[1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browsingData/removeCache
[2] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browsingData/DataTypeSet
[3] https://developer.chrome.com/extensions/browsingData#method-removeLocalStorage
Flags: needinfo?(bob.silverberg) → needinfo?(amckay)
Assignee

Comment 38

2 years ago
Note that in my patch in bug 1388428, I'm having it just reject the Promise with an "unsupported" message, as I see that being done elsewhere in WebExtension code. Maybe that's not the right solution, but it seemed reasonable.

Comment 39

2 years ago
In comment 36, it sounds like if a developer does pass since, the side effect can be unexpected clearing all data. That sounds bad, so raising unsupported as mentioned in comment 37 and comment 38 sounds like a good solution.
Flags: needinfo?(amckay)

Updated

2 years ago
Keywords: dev-doc-needed
I've written a page on this: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browsingData/removeLocalStorage

Please let me know if this covers it.
Flags: needinfo?(wisniewskit)
Assignee

Comment 41

2 years ago
Yes, that looks fine, thanks. (Sorry for the ni? delay!)
Flags: needinfo?(wisniewskit)

Comment 42

2 years ago
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(wisniewskit)
Nothing that we're discussing uplift of this to ESR in bug 1047098, but that sounds a little risky to me. 
Thomas, Jan, what do you think?
Flags: needinfo?(jvarga)
Flags: needinfo?(jvarga)
Comment on attachment 8925780 [details] [diff] [review]
patch for ESR52

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is needed for bug 1047098 which already has ESR52 approval.
User impact if declined: See bug 1047098.
Fix Landed on Version: 57
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: None
Attachment #8925780 - Flags: approval-mozilla-esr52?
Comment on attachment 8925780 [details] [diff] [review]
patch for ESR52

Let's give this a try since we're aiming to fix the fingerprinting issue in bug 1047098.
Attachment #8925780 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Assignee

Comment 48

2 years ago
FWIW, I agree on trying to get it in if it helps with bug 1047098.
Flags: needinfo?(wisniewskit)

Comment 49

2 years ago
Posted image clear storage.gif
Issue reproduced on Firefox 55.0a1 (20170411030208) under Wind 10 64-bit. 

This issue is verified as fixed on Firefox 57.0 (20171112125346), Firefox 58.0b3 (20171113130450) and 59.0a1 (20171113220112) under Wind 10 64-bit and Mac OS X 10.13.

Updated

11 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.