1406675 - WebExtension cannot use IndexedDB at all if "Accept cookies from websites" is disabled

Status: VERIFIED FIXED

(WebExtensions :: Storage, defect, P2)

Description

[voltron]

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170926190823

Steps to reproduce:

1. Create a WebExtension that uses IndexedDB in the background page.
2. Witness it working OK if you have setting "Accept cookies from websites" enabled.
3. Now disable the "Accept cookies from websites" setting.


Actual results:

Boom! The WebExtension can no longer use IndexedDB.




Expected results:

IndexedDB can be used regardless of cookie settings. They should have nothing to do with each other.

This is a CRITICAL bug at least from extension developer point of view.

It's also very annoying that "Private Browsing Mode" AND "Never remember history" settings both break IndexedDB. So we have at least three configuration settings that break an essential storage completely. This catches both users and extension developers by surprise in a nasty way.

The workaround for this is to add the extension URL to Exceptions list in the cookie settings but this should not be required and it's really a temporary hack.

Comment 1

[Kohei Yoshino]

Not a WebExtension-specific issue AFAIK.

Comment 2

[Gingerbread Man]

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
20171007220156

Checked with Feedbro 3.29.8.
https://addons.mozilla.org/firefox/addon/feedbroreader/

Also noted by Stylus developers:
https://github.com/openstyles/stylus/issues/151

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

If I read [1] right, it's our expected security restriction. Could you please confirm, Bevis?

[1] https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API/Using_IndexedDB#Security

Comment 4

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #3)
> If I read [1] right, it's our expected security restriction. Could you
> please confirm, Bevis?
> 
> [1]
> https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API/
> Using_IndexedDB#Security

Or should we treat WebExtension differently?

Comment 5

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #4)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #3)
> > If I read [1] right, it's our expected security restriction. Could you
> > please confirm, Bevis?
> > 
> > [1]
> > https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API/
> > Using_IndexedDB#Security
Yes, this is the expected security restriction. Since the content scripts of the extension accesses the same DOM tree of the content window so if the "Accept cookies from websites" is disabled, the access will be denied in the same way of scripts of a web content.
> Or should we treat WebExtension differently?
It seems not reasonable to me to give more privileges to the web extension than the corresponding web content.

Comment 6

[voltron]

Regarding comment 5:

Please understand that this issue should have _nothing_ to do with a) cookies, b) content scripts, c) web content of some web page. 

The use case for most extensions is this: the WebExtension has a background page that is live as long as the browser is open. That background page then fetches data from the internet (e.g. stock prices, articles from an RSS feed, whatever). Then it saves that data into IndexedDB so that it can be show to the user instantly when the user wants to see the data (e.g. clicks the extension icon and e.g. latest N articles of some source as shown). WebExtension background pages should be able to use IndexedDB without limitations.

Current implementation is such that if the user has disabled "Accept cookies from websites", the background page cannot use IndexedDB at all and cannot thus save any data that requires database-like features that IndexedDB offers. chrome.storage.local simply isn't good enough for this.

Comment 7

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Thanks voltron!
I'd like to get our WebExtension folks to triage this and suggest the use case and priority first, then we will see how the IDB implementation should support. For that reason, I move the module to WebExtention to get their attention.

Comment 8

[Andy McKay]

We think WebExtensions should be exempt from that limitation and indexedDB should still work from a background page regardless of cookie settings.

Comment 9

[voltron]

:andym thank you for prioritizing this issue! Any chance to get this fix to FF 57 as well? I guess the public stable 57 release isn't out yet and it will in a way "set the tone" for a lot of things - including WebExtension support since old add-ons won't work anymore. Therefore I see it's very important to get this fixed as soon as possible.

Thank you for your consideration.

Comment 10

[Andy McKay]

That's highly unlikely given the time and people we have available to do it and the high bar that is being set for 57 uplifts at this point. Perhaps someone else has the knowledge, skills and time to move quickly on it.

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1406675 - Allow storages in WebExtensions on customized cookieBehavior and lifetimePolicy prefs.

Review commit: https://reviewboard.mozilla.org/r/194034/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194034/

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1406675 - Add new WebExtensions test for cookieBehavior and lifetimePolicy prefs.

Review commit: https://reviewboard.mozilla.org/r/194036/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194036/

Comment 13

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8922883 [details]
Bug 1406675 - Allow storages in WebExtensions on customized cookieBehavior and lifetimePolicy prefs.

Hi Andrew, 
as briefly described in the summary of this issue, currently a WebExtension is unable to use indexedDB (from its own extension pages, e.g. its own background page) when the "network.cookie.cookieBehavior" is set to nsICookieService::BEHAVIOR_REJECT (and the same is going to happen also for localStorage).

I started to look into this issue and it seems to me that nsContentUtils::InternalStorageAllowedForPrincipal could be a reasonable place to make the changes needed to allow a WebExtension to store its data when the above preference has been set, but at the same time I want to be absolutely sure that it will not allow it for additional scenarios that should not be allowed (and this looks definitely a sensible place).

Do you mind to take a brief look at the proposed change for an initial feedback? 

Thanks in advance!

In the second patch attached to the same mozreview request there are some additional webextension xpcshell tests that have created for it:

one test is to ensure that indexedDB and localStorage are allowed from a background page when the above preference is not set to ACCEPT, while the other test is to ensure that a content script can’t use a webpage’s indexedDB or localStorage if the webpage is not allowed to use it (by the same setting).

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8922883 [details]
Bug 1406675 - Allow storages in WebExtensions on customized cookieBehavior and lifetimePolicy prefs.

This makes sense to me and seems like the right place to do it given that it's a given that all webextensions should have persistent storage capabilities.  I understand the private-browsing issues are addressed via https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/incognito and the strategy of controlling what windows/tabs the WebExtension can perceive, rather than doing something like forbidding the WebExtension access to persistence.  I also understand that could change when Firefox gets to implementing that.

Restating:
- IndexedDB checks happen 1 of 2 ways:
  - For Windows: nsGlobalWindow::GetIndexedDB calls IDBFactory::CreateForWindow calls IDBFactory::AllowedForWindowInternal calls nsContentUtils::StorageAllowedForWindow directly.  The return value is compared against eDeny.
  - For Workers: WorkerGlobalScope::GetIndexedDB calls WorkerPrivate::IsStorageAllowed which is the value stashed in the WorkerLoadInfo by WorkerPrivate::GetLoadInfo which calls nsContentUtils::StorageAllowedForWindow and compares for > eDeny.  (For nested workers, the value is just directly propagated for the parent).
- Private browsing checks happen in FactoryOp::CheckPermission when the actual requests are made, not when the binding is accessed.


It's great to have those tests.  I'd suggesting considering adding checks from Workers for completeness and so that anyone creating new tests who consults the tests you're adding will realize they probably want to run their tests in a worker too.  (This specific binding-explosion test is unlikely to diverge, but workers are much more likely to bring out edge cases.)

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

Hm, so looking at bug 1313401 which covers LocalStorage and its mention of session-scoped storage, perhaps we want to go a step further and simply return StorageAccess:eAllow where you currently set isAddonPrincipal.  This avoids LocalStorage using its wacky IsSessionOnly mode.  (Session-only mode is like private browsing mode, but primed from the previously persisted LocalStorage state which must have been written before the setting was enabled.)

Note that although StorageAccess::ePrivateBrowsing is a thing and this change will cause eAllow to be returned where ePrivateBrowsing would previously be returned, this won't actually change the behavior of either LocalStorage or IndexedDB as it relates to private browsing mode *unless you've ensured that they don't have an mPrivateBrowsingId set on their OriginAttributes or change the manner in which docshells report private browsing mode*.
- LocalStorage gets its concept of private browsing from nsGlobalWindow::IsPrivateBrowsing() which gets it from nsILoadContext::UsePrivateBrowsing that it QI's off of the docshell (and is implemented by the docshell).  This is because the system principal for a private browsing window doesn't get a distinct principal with a private browsing id set on it, it's just the system principal.  So the private-ness has to come from the docshell.  Otherwise this would result in co-mingling of localStorage between chrome-privileged non-private-browsing and chrome-privileged private-browsing, which I guess is a thing.  (There's an assertion that documents this well at https://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#3841.  To re-state, because it's a little confusing, nsDocShell has its own mPrivateBrowsingId which may not be the same as the principal in the case of a chrome document.)
- IndexedDB similarly gets its private browsing mode off the nsILoadContext it QIs off the nsIWebNavigation it do_GetInterface()s off the nsPIDOMWindowInner (which is nsGlobalWindow::GetOuterWindowInternal()->mDocShell QI'd to an nsIWebNavigation).  It looks like there's an incorrect MOZ_DIAGNOSTIC_ASSERT about the OriginAttribute here, so I suspect we have no Chrome consumers that currently try and use IDB under PrivateBrowsing because they'd crash instead of get the expected runtime failure (since we don't support IDB under private browsing).

So in order to address the private-browsing issue, I think you're going to need to ensure the docshells that host the webextensions don't update their origin attributes to indicate private browsing or potentially a set of related changes.  An entry point to look at is https://searchfox.org/mozilla-central/rev/1ebd2eff44617df3b82eea7d2f3ca1b60cc591a0/docshell/base/nsDocShell.cpp#2322.  I would suggest talking to/needinfo-ing :smaug next on all of this.  :smaug will also need to be the reviewer for your actual final patch.  :baku may also have opinions.

Comment 16

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8922883 [details]
Bug 1406675 - Allow storages in WebExtensions on customized cookieBehavior and lifetimePolicy prefs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194034/diff/1-2/

Comment 17

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8922884 [details]
Bug 1406675 - Add new WebExtensions test for cookieBehavior and lifetimePolicy prefs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194036/diff/1-2/

Comment 18

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8922883 [details]
Bug 1406675 - Allow storages in WebExtensions on customized cookieBehavior and lifetimePolicy prefs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194034/diff/2-3/

Comment 19

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8922884 [details]
Bug 1406675 - Add new WebExtensions test for cookieBehavior and lifetimePolicy prefs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194036/diff/2-3/

Comment 20

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8922884 [details]
Bug 1406675 - Add new WebExtensions test for cookieBehavior and lifetimePolicy prefs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194036/diff/3-4/

Comment 21

[Luca Greco [:rpl] [:luca] [:lgreco]]

Thanks Andrew,
your previous feedback comments have been very useful, and based on your suggestion about the localStorage session-only mode:

- I can confirm that Bug 1313401 is not related to the "permanent private browsing mode", but it is actually related to the "network.cookie.lifetimePolicy" about:config preference, which is the preference that is set to 2 when the user choose 'Use custom settings for History' and select 'Keep until: I close Firefox' in the about:preferences#privacy page)

- we can cover the issues with both "network.cookie.cookieBehavior" and "network.cookie.lifetimePolicy" using a less invasive strategy based on the "cookie" site permission, with an outcome similar to returning StorageAccess:eAllow in the previous patch as suggested in comment 15, but without special casing the entire moz-extension protocol:

  - nsContentUtils::InternalStorageAllowedForPrincipal internally calls nsContentUtils::GetCookieBehaviorForPrincipal

    https://searchfox.org/mozilla-central/rev/30ead7d1ae5bf95b8bc0fd67b950cd46ca05e32c/dom/base/nsContentUtils.cpp#9183,9212    
 
  - nsContentUtils::GetCookieBehaviorForPrincipal will use the "cookie" site permission to override the cookieBehavior 
    and the lifetimePolicy globally configured from the "about:config" preference (and the default "cookie" site 
    permission value):
    
    https://searchfox.org/mozilla-central/rev/30ead7d1ae5bf95b8bc0fd67b950cd46ca05e32c/dom/base/nsContentUtils.cpp#9129-9132,9137-9138

    https://searchfox.org/mozilla-central/rev/30ead7d1ae5bf95b8bc0fd67b950cd46ca05e32c/browser/modules/SitePermissions.jsm#572-583

The new patch (attachment 8922883 [details]) uses this new strategy: 
it ensures that the extension principal has the "cookie" permission starting from when the extension is installed/updated (and removes the permission when the extension is uninstalled), this way any change applied from the user to the cookieBehavior and lifetimePolicy preferences does not affect the extensions (same for Bug 1363860, which is going to allow an extension to change this settings globally and by origin).

In the other patch (attachment 8922884 [details]) there is the updated test file, which now covers the worker scenario as well as a new test which covers "lifetimePolicy = 2" (that is Bug 1313401).

Besides these two settings, there are two more privacy settings which are exposed to the user in "about:preferences#privacy" and affects the extension storages:

- permanent private browsing mode:
  - extension pages' localStorage not persisted (cache cleared once the last private browsing window is closed)
  - extension pages' indexedDB.open raises an exception (like any webpage opened in a private browsing window)

- “Firefox will use custom settings for history” -> “Clear history when Firefox closes”
  (privacy.sanitize.sanitizeOnShutdown)
  - extension pages' indexedDB not affected (works as usually, data is persisted and reloaded between browser restarts)
  - extension pages' localStorage cleared completely with all the other localStorage data

I've filed this last one as Bug 1416219 and added some additional detail about it on its summary.

I also took a brief look at the "permanent private browsing mode" scenario and I agree that the best option seems to be to ensure that the extension pages are not in private browsing mode, but that is a change that would be better to split into its own issue (separately from Bug 1313401, Bug 1406675 and Bug 1416219) and discussed in much more detail (e.g. what about an extension tab page opened in a permanent private browsing window? should still be non private? what about the extension tab page loads a webpage inside an iframe?).

Comment 22

[Andrew Swan [:aswan]]

Comment on attachment 8922884 [details]
Bug 1406675 - Add new WebExtensions test for cookieBehavior and lifetimePolicy prefs.

https://reviewboard.mozilla.org/r/194036/#review203918

A few nits, mostly looking for clarification on the last test.  Also, any reason not to fold this into the first patch on this bug?

::: toolkit/components/extensions/test/xpcshell/test_ext_cookieBehaviors.js:43
(Diff revision 4)
> +    background,
> +    files: {
> +      "worker.js": function worker() {
> +        this.onmessage = () => {
> +          try {
> +            void indexedDB;

what about localStorage here?

::: toolkit/components/extensions/test/xpcshell/test_ext_cookieBehaviors.js:79
(Diff revision 4)
> +  await test_bg_page_allowed_storage();
> +});
> +
> +add_task(async function test_content_script_on_cookieBehaviorReject() {
> +  Services.prefs.setIntPref("network.cookie.cookieBehavior", COOKIE_BEHAVIOR_REJECT);
> +  do_register_cleanup(() => {

I'm pretty sure these run after all the tests in the file have run, and this is identical to the cleanup function in the previous test.  I'm not really sure how I would change it though, its not harmful to repeat it and it means that if somebody copies a test case to another file, they'll get the right cleanup code with it...

::: toolkit/components/extensions/test/xpcshell/test_ext_cookieBehaviors.js:95
(Diff revision 4)
> +    );
> +
> +    browser.test.assertThrows(
> +      () => localStorage,
> +        /The operation is insecure/,
> +      "a content script can't use indexedDB from a page where it is disallowed"

indexedDB -> localStorage

::: toolkit/components/extensions/test/xpcshell/test_ext_cookieBehaviors.js:154
(Diff revision 4)
> +  // the extension is running.
> +  const {
> +    isSessionOnly,
> +    storedItem,
> +    storedKeysLength,
> +  } = await ContentTask.spawn(addonBrowser, uuid, async (extUUID) => {

Why does this have to run in the extension process?  Shouldn't localStorage be available from any process?

In any case, why does this specific test actually check the contents of localStorage when previous tests just checked that the symbol `localStorage` was present?  If this is about persisting storage across sessions, can we actually mimic ending the current session and starting a new one?

Comment 23

[Andrew Swan [:aswan]]

Comment on attachment 8922883 [details]
Bug 1406675 - Allow storages in WebExtensions on customized cookieBehavior and lifetimePolicy prefs.

https://reviewboard.mozilla.org/r/194034/#review203922

This looks good to me from the webextensions side.  I know you've already discussed this with :asuth but please also get an official r+ from him before landing.

Comment 24

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8922884 [details]
Bug 1406675 - Add new WebExtensions test for cookieBehavior and lifetimePolicy prefs.

https://reviewboard.mozilla.org/r/194036/#review203918

Sure, there are no reason to not fold this patch into the first one right now.

> what about localStorage here?

iirc localStorage is not available in a worker.

> I'm pretty sure these run after all the tests in the file have run, and this is identical to the cleanup function in the previous test.  I'm not really sure how I would change it though, its not harmful to repeat it and it means that if somebody copies a test case to another file, they'll get the right cleanup code with it...

yeah, that's true, as an alternative approach I could add something like

```
add_task(function cleanup_cookieBehavior_prefs() {
  Services.prefs.clearUserPref("network.cookie.cookieBehavior");
});
```

after this test.

> Why does this have to run in the extension process?  Shouldn't localStorage be available from any process?
> 
> In any case, why does this specific test actually check the contents of localStorage when previous tests just checked that the symbol `localStorage` was present?  If this is about persisting storage across sessions, can we actually mimic ending the current session and starting a new one?

While the previous test are checking that accessing localStorage when "cookieBehavior = 2" (reject) does not raise a security error, this additional test case is testing that when lifetimePolicy = 2" (which switch localStorage to run in a "session only" mode, which means that localStorage is initialized from the data stored on disk but any update to this data is never persisted).

(the lifetimePolicy doesn't have any effect on the indexedDB storage and so we don't need to also test indexedDB here).

I tried a number of strategy to be able to reliably test this scenario (so that it fails consistently when the fix is not applied and passes consistently when the fix is there) and this is the strategy that finally worked as expected.

Nevertheless, I was also initially hoping that I could just retrive the storage and makes the assertions on it all from the main process, but when I tried that approach, Services.domStorageManager.getStorage was returning null when runnning in "oop extensions" mode :-(

(I also tried with the domStorage testing messages, e.g. to force the data to be flushed on disk but it wasn't enough, but I can give it another shot, though)

Comment 25

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8922884 [details]
Bug 1406675 - Add new WebExtensions test for cookieBehavior and lifetimePolicy prefs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194036/diff/4-5/

Comment 26

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8922884 [details]
Bug 1406675 - Add new WebExtensions test for cookieBehavior and lifetimePolicy prefs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194036/diff/5-6/

Comment 27

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8922884 [details]
Bug 1406675 - Add new WebExtensions test for cookieBehavior and lifetimePolicy prefs.

https://reviewboard.mozilla.org/r/194036/#review203918

> While the previous test are checking that accessing localStorage when "cookieBehavior = 2" (reject) does not raise a security error, this additional test case is testing that when lifetimePolicy = 2" (which switch localStorage to run in a "session only" mode, which means that localStorage is initialized from the data stored on disk but any update to this data is never persisted).
> 
> (the lifetimePolicy doesn't have any effect on the indexedDB storage and so we don't need to also test indexedDB here).
> 
> I tried a number of strategy to be able to reliably test this scenario (so that it fails consistently when the fix is not applied and passes consistently when the fix is there) and this is the strategy that finally worked as expected.
> 
> Nevertheless, I was also initially hoping that I could just retrive the storage and makes the assertions on it all from the main process, but when I tried that approach, Services.domStorageManager.getStorage was returning null when runnning in "oop extensions" mode :-(
> 
> (I also tried with the domStorage testing messages, e.g. to force the data to be flushed on disk but it wasn't enough, but I can give it another shot, though)

I've reworked the new test case (the one related to the localStorage session-only mode) to retrieve and make the assertions on the extension DOMStorage instance without resorts to `ContentTask.spawn`, by using `domStorageManager.precacheStorage(...)` instead of `domStorageManager.getStorage(...)` (because as described above when running in "remote-webextensions" mode, at least in an xpcshell test, `domStorageManager.getStorage(...)` is returning `undefined` instead of the storage instance).

Comment 28

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8922884 [details]
Bug 1406675 - Add new WebExtensions test for cookieBehavior and lifetimePolicy prefs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194036/diff/6-7/

Comment 29

[Andrew Swan [:aswan]]

Comment on attachment 8922884 [details]
Bug 1406675 - Add new WebExtensions test for cookieBehavior and lifetimePolicy prefs.

https://reviewboard.mozilla.org/r/194036/#review206050

::: toolkit/components/extensions/test/xpcshell/test_ext_cookieBehaviors.js:15
(Diff revision 7)
> +const server = createHttpServer();
> +server.registerDirectory("/data/", do_get_file("data"));
> +
> +const BASE_URL = `http://localhost:${server.identity.primaryPort}/data`;
> +
> +async function test_bg_page_allowed_storage() {

nit: the name is a little misleading since this also tests a web worker, maybe just add a comment?

Comment 30

[Andrew Swan [:aswan]]

Comment on attachment 8922884 [details]
Bug 1406675 - Add new WebExtensions test for cookieBehavior and lifetimePolicy prefs.

https://reviewboard.mozilla.org/r/194036/#review206052

Whoops, mean to say:
Looks good, thanks.  Please squash the two patches together and also get a review from :asuth

Comment 31

[Anthony Lieuallen]

https://github.com/greasemonkey/greasemonkey/issues/2598

This is breaking Greasemonkey 4, which uses IndexedDB.  Can I tell my users that Firefox 58 will fix this?

Comment 32

[Yandy]

(In reply to Anthony Lieuallen from comment #31)
> Can I tell my users that Firefox 58 will fix this?

The nightly builds haven't even fixed this issue yet. The issue also includes private browsing mode.

1. Enable "Always use private browsing mode"
2. Install uBlock Origin and update it's 3rd party filters
3. Restart Firefox and all of the filters need to be updated again

Chrome allows filters to be updated in incognito windows, so Firefox should too. I hope to see this issue fixed within the next month.

Comment 33

[Andy McKay]

Assuming this can land on mozilla-central next week, I'd support an uplift of this patch to 58, if Luca is  cool with it.

Comment 34

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8922883 [details]
Bug 1406675 - Allow storages in WebExtensions on customized cookieBehavior and lifetimePolicy prefs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194034/diff/3-4/

Comment 35

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8922883 [details]
Bug 1406675 - Allow storages in WebExtensions on customized cookieBehavior and lifetimePolicy prefs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194034/diff/4-5/

Comment 36

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8922883 [details]
Bug 1406675 - Allow storages in WebExtensions on customized cookieBehavior and lifetimePolicy prefs.

Hi Andrew,
Do you mind to take a brief look to the last version of this patch for a final sign-off? 

in particular related to the following pieces:

- added worker to the cookieBehavior test case, line 23-32 and 42-52 on the new test file from this patch

- how we test the extension pages' localStorage mode (to ensure that the extension pages' localStorage is not in session only mode when the lifetimePolicy has been set to 2) by retrieving the extension page's localStorage.isSession property, which is accessible from the chrome privileged code (it turned out that there is no need to QI the localStorage object), at line 193 of the new test file from this patch

- adding the "cookie" permission for the moz-extension principal, so that the webextensions would not be affected by a globally customized cookieBehavior and lifetimePolicy preference, at line 1394 of Extension.jsm as changed by this patch.

Comment 37

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8922883 [details]
Bug 1406675 - Allow storages in WebExtensions on customized cookieBehavior and lifetimePolicy prefs.

I'm clearing this review request because I've just noticed a failure, while reviewing the oranges from the last push to try, which is unfortunately a side-effect related to the piece of this patch that adds the "cookie" permission for the moz-extension principal:

The cookie permission appears in the Site Permission when an extension pages is opened as a tab,
the extension identity indication in the url bar contains the icon related to the site permissions:

- https://public-artifacts.taskcluster.net/CEIukuoVSeCVwKYgtTf0Jg/0/public/test_info//mozilla-test-fail-screenshot_KztcZI.png

which makes browser_ext_identity_indication.js test to fail:

- https://treeherder.mozilla.org/logviewer.html#?job_id=146166311&repo=try&lineNumber=2904

Besides the test failure, the other issue is that a user can currently remove the cookie permission from there (and that permission is currently added only on install or upgrades) from the Site Permission popup (which to be fair is also true for the unlimitedStorage permission).

This issue and the related failure have to be fixed before landing this patch,
e.g. using one of the following strategies:

1) going back to whitelist the moz-extension scheme at a lower level (similarly to the initial versions of the attached patches)

2) changing the behavior of the SitePermissions dialog for the moz-extension protocol (e.g. by not allowing to remove the cookie permission and the unlimitedStorage permissions from there, given that they are added and removed based on an extension permission)

The option (1) has probably the "advantage" of being a strategy that has been already discussed and evaluated in this issue, while the option (2) is likely to require additional discussion to evaluate the final desired behavior.

Comment 38

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8922883 [details]
Bug 1406675 - Allow storages in WebExtensions on customized cookieBehavior and lifetimePolicy prefs.

https://reviewboard.mozilla.org/r/194034/#review206486

Love the high quality tests.  Thank you!

::: toolkit/components/extensions/test/xpcshell/test_ext_cookieBehaviors.js:6
(Diff revision 5)
> +const COOKIE_BEHAVIOR_REJECT_FOREIGN = 1;
> +const COOKIE_BEHAVIOR_REJECT = 2;
> +const COOKIE_BEHAVIOR_LIMIT_FOREIGN = 3;

nit: Although the values can't really change, I think it's preferable to copy the values directly from Ci.nsICookieService.{BEHAVIOR_REJECT_FOREIGN,BEHAVIOR_REJECT,BEHAVIOR_LIMIT_FOREIGN}.  (This will help code indexers like searchfox, too.)

Comment 39

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8922883 [details]
Bug 1406675 - Allow storages in WebExtensions on customized cookieBehavior and lifetimePolicy prefs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194034/diff/5-6/

Comment 40

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8922883 [details]
Bug 1406675 - Allow storages in WebExtensions on customized cookieBehavior and lifetimePolicy prefs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194034/diff/6-7/

Comment 41

[Andrew Sutherland [:asuth] (he/him)]

(Whoops on the mid-air.)

Ah, so I had misread some intent in the change to permissions.  If you don't want the permissions to be user-revocable or controlled on a per-extensions basis, then indeed I think you want to go back to the earlier changes to nsContentUtils::InternalStorageAllowedForPrincipal.  But if there are eventual desires to allow web-extensions to be limited, I think the permissions approach is an option that works and already supports change notifications for responding to those changes in permissions.

It may also be appropriate to have QuotaManager::IsOriginInternal return true so that the origin is treated as persistent storage.  This will avoid prompts and quota eviction for the extension's origin under QuotaManager-managed storage.  However, that may also require a schema rev to run an upgrade pass and then we have the downgrade edge-cases we still need to worry about.

Comment 42

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Andrew Sutherland [:asuth] from comment #41)
> (Whoops on the mid-air.)
> 
> Ah, so I had misread some intent in the change to permissions.  If you don't
> want the permissions to be user-revocable or controlled on a per-extensions
> basis, then indeed I think you want to go back to the earlier changes to
> nsContentUtils::InternalStorageAllowedForPrincipal.  But if there are
> eventual desires to allow web-extensions to be limited, I think the
> permissions approach is an option that works and already supports change
> notifications for responding to those changes in permissions.
> 
> It may also be appropriate to have QuotaManager::IsOriginInternal return
> true so that the origin is treated as persistent storage.  This will avoid
> prompts and quota eviction for the extension's origin under
> QuotaManager-managed storage.  However, that may also require a schema rev
> to run an upgrade pass and then we have the downgrade edge-cases we still
> need to worry about.

How about the strategy implemented in the following change on the patch?

- https://reviewboard.mozilla.org/r/194034/diff/5-6/

it implements the same behavior of the previous version (which was adding the "cookie" site permission
on the moz-extension principal), but it is applied at a lower level in the nsContentUtils::GetCookieBehaviorForPrincipal
method, which basically ensures that all the principals related to the active extensions get the same cookieBehavior and lifetimePolicy that they would get by setting the ALLOW_ACTION as the "cookie" SitePermission.

Comment 43

[Andrew Sutherland [:asuth] (he/him)]

The logic is good, in particular:
- Avoiding letting the default cookies behavior influence the decision.
- Avoiding the permissions check which can potentially involve synchronous I/O or cause misleading debug output if it looks like we actually honor the permissions set in the permissions manager.
- Reducing the permutation space of the function by treating the addons special case like explicit permissions.  (Versus directly returning allowed... since the flow of the function is eAllow being further restricted, returning eAllow outright is potentially dangerous.)

However, I think the logic wants to be in nsContentUtils::InternalStorageallowedForPrincipal like in earlier rev's.  That way all the special-case decisions are in one method (null principals, about: schemes, etc.).  Setting lifetimePolicy/behavior to hard-coded values works for me in an addons-specific if-branch that avoids GetCookieBebhaviorForPrincipal being invoked (and is equivalent to your logic) would seem to work.  r=asuth for that.

Comment 44

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8922883 [details]
Bug 1406675 - Allow storages in WebExtensions on customized cookieBehavior and lifetimePolicy prefs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194034/diff/7-8/

Comment 45

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8922883 [details]
Bug 1406675 - Allow storages in WebExtensions on customized cookieBehavior and lifetimePolicy prefs.

Hi Andrew,
Thanks a lot for the feedback in Comment 43, I've just applied the changes proposed and I definitely agree that this is a cleaner and clearer than the last version of this patch.

This should be the last round of review ;-) (and only the changes on nsContentUtils.cpp have been actually changed in this version, the test case is exactly the same from the previous versions).

Comment 46

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8922883 [details]
Bug 1406675 - Allow storages in WebExtensions on customized cookieBehavior and lifetimePolicy prefs.

https://reviewboard.mozilla.org/r/194034/#review211122

Woo!!

Comment 47

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8922883 [details]
Bug 1406675 - Allow storages in WebExtensions on customized cookieBehavior and lifetimePolicy prefs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194034/diff/8-9/

Comment 48

[Luca Greco [:rpl] [:luca] [:lgreco]]

Patch rebased on a recent m-c tip, and applied a minor tweak to fix the "error: unused variable 'policy'" on building with -Werror=unused-variable.

Last push to try (which looks good):

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfd0937183ae4f53c8084cfd2f4d0fa10fa08fe8

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8445570689af
Allow storages in WebExtensions on customized cookieBehavior and lifetimePolicy prefs. r=asuth,aswan

Comment 50

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/8445570689af

Comment 51

[Julien Cristau [:jcristau]]

Probably too late for 58, though feel free to reset to affected and request uplift if you disagree.

Comment 54

[CosminB]

Attachment: Bug1406675.gif

I can reproduce this issue on Firefox 58.0.2 (20180206200532) under Windows 7 64-bit and Mac OS 10.13.2.

This issue is verified as fixed on Firefox 60.0a1 (20180220103456) and Firefox 59.0b11 (20180219114835) under Windows 7 64-bit and Mac OS X 10.13.2.

I unchecked the “Accept cookies from websites” option, then I updated the 3rd-party filters from uBlock Origin and after I restarted the browser:
- On Firefox 58.0.2  the filters list needs to be updated again.
- On Firefox 60.0a1 the filters list remains updated.

For the window.localStorage part I have not yet found an extension.