1459229 - Calls to navigator.storage.persist should not spawn permission requests when called from extension pages

Status: RESOLVED WONTFIX

(WebExtensions :: Storage, defect, P2)

Description

[Johann Hofmann [:johannh]]

Attachment: Screen Shot 2018-05-04 at 17.29.32.png

Currently calling navigator.storage.persist() from any type of extension page without having the unlimitedStorage permission in the extension manifest spawns the confusing permission dialog attached to this bug.

The "persistent-storage" permission that is given to principals should only be tied to the unlimitedStorage WE permission, to avoid confusing situations like this. I believe that's already how it works for geolocation.

Comment 1

[Johann Hofmann [:johannh]]

Ugh upon further testing geolocation has the same user experience for front-end web-extension:// pages (with the extension ID shown, not the extension name), but somehow background pages get "User denied geolocation prompt" errors. Interesting.

Andrew, did we have a bug filed on replacing the extension ID in permission doorhangers with the extension name?

Comment 2

[Andrew Swan [:aswan]]

(In reply to Johann Hofmann [:johannh] from comment #1)
> Andrew, did we have a bug filed on replacing the extension ID in permission
> doorhangers with the extension name?

I'm not aware of an existing bug for that...
I agree though that persistent-storage and geolocation should be connected to extension permissions.

Comment 3

[Johann Hofmann [:johannh]]

So the reason why permission prompts (presumably also for persistent-storage) don't work in background scripts is that we cancel the call here:

https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/browser/modules/PermissionUI.jsm#278

Which sounds okay to me, except that I still think that at least "persistent-storage" should be only controlled by the "unlimitedStorage" permission...

I can file a bug on showing the proper name.

Comment 4

[Andrew Swan [:aswan]]

(In reply to Johann Hofmann [:johannh] from comment #3)
> Which sounds okay to me, except that I still think that at least
> "persistent-storage" should be only controlled by the "unlimitedStorage"
> permission...

Right, we handle the positive case here:
https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/toolkit/components/extensions/Extension.jsm#1608-1621

Perhaps the removeFromPrincipal() calls should turn into addFromPrincipal() with DENY_ACTION?

Same with the geolocation permission a few lines down?

Comment 5

[Johann Hofmann [:johannh]]

Yeah, we could do that. Either that or catch moz-extension principals somewhere in the permission request code and cancel them. For persistent-storage, in my opinion, it's definitely the right choice to disallow in-context requests from all sources.

I don't think it's the same for geolocation, though. Geolocation requests can be much more contextual (e.g. an add-on that can do lots of things but can optionally also show the weather for your location), so it's actually in favor of user privacy to not force add-ons to include the geolocation permission from the start.

I'm a little worried that permanently giving permission to an extension page gives permission to the entire principal (including background pages), but the best way is probably to resolve to the correct extension name in the permission prompt, which I'm now filing a bug on.

Comment 6

[Johann Hofmann [:johannh]]

(We also have to figure out if there's a better way to spawn permission prompts for browserAction popups than to have it appear in the identity block of random pages...)

Comment 7

[Johann Hofmann [:johannh]]

(In reply to Johann Hofmann [:johannh] from comment #5)
> but the best way is probably to resolve

I meant to write:
> but the best way to solve this is probably

Comment 8

[Andrew Swan [:aswan]]

(In reply to Johann Hofmann [:johannh] from comment #5)
> I don't think it's the same for geolocation, though. Geolocation requests
> can be much more contextual (e.g. an add-on that can do lots of things but
> can optionally also show the weather for your location), so it's actually in
> favor of user privacy to not force add-ons to include the geolocation
> permission from the start.

(In reply to Johann Hofmann [:johannh] from comment #6)
> (We also have to figure out if there's a better way to spawn permission
> prompts for browserAction popups than to have it appear in the identity
> block of random pages...)

I think the solution here would be to let the extension "geolocation" permission be optional (ie, extensions request it explicitly with browser.permissions.request() rather than implicitly asking for it when they call navigator.geolocation.whatever).  That has the restriction that it must be called from a user input handler and will cause a permission prompt with the name and icon of the extension to appear anchored to the extension icon in the identity box in the window where the user interaction took place.

That would require a small amount of work to map adding and removing that permission to the underlying nsIPermissionManager calls, but I think it would address this particular usability issue.

(we should still do bug 1459613 for any other permission prompts that arise from extension pages using DOM APIs that require a prompt)

Comment 9

[Johann Hofmann [:johannh]]

Yes, that sounds like a good solution for Geolocation, but it may be better to do that in a different bug than this one.

Comment 10

[Rob Wu [:robwu]]

Since bug 1459613, we show the name instead of the UUID.

We are inclined to close this bug, because the current behavior matches the expectations from me and :rpl. When the unlimitedStorage permission is present, the permission is automatically granted. Without it, we should fall back to the "normal" behavior that web developers expect, which is to prompt for permissions. Other APIs, geolocation and notifications are already working in this way.

Johann, do you have any objections to closing this bug?

Comment 11

[Johann Hofmann [:johannh]]

Well, if you specifically want to support the use case of "developer does not use unlimitedStorage at first but may optionally ask users later" then we could close it, but personally, I don't think we should close this bug. I have the feeling that our code makes the assumption that extensions with persistent-storage will also have the unlimitedStorage permission and complicating that feels unnecessary.

I'd make it so that the permission is auto-denied without showing a prompt, but again, if you think this is a good use case then feel free to close it.

Comment 12

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

In the WebExtensions Framework internals we do make the opposite assumption for sure (if the extension has the "unlimitedStorage" extension permission, then there should be also the "persistent-storage" site permission set to allowed for the extension origin), but we shouldn't be making any assumption on the opposite.

e.g. on uninstall we do remove the persistent-storage site permission (even if it wasn't added internally by Firefox because of the "unlimitedStorage" extension permission).

As far as I know we do not make any other assumption about the relationship between the two anywhere else (I mean, outside of the WebExtensions internals).

Given that this is consistent with how other webAPIs (geolocation and notifications) currently behave, I would be ok on closing this bug.

(as a side note, If we do change idea in the future and we prefer to prevent site permissions to be requested by the extension pages, I think it would be reasonable to do it also for the other APIs that requires a site permissions and not just for the storage one, as it would be more consistent and less surprising from an extension developer perspective).

Comment 13

[Rob Wu [:robwu]]

I'll close this bug then, for consistency with other content + extension permissions.