1907732 - clearing recent history loses extension's unlimitedStorage permission

Status: RESOLVED FIXED

(WebExtensions :: Storage, defect, P2)

Description

[Denis Lisov]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:130.0) Gecko/20100101 Firefox/130.0

Steps to reproduce:

My extension uses the unlimitedStorage permission and calls navigator.storage.persist(); to ensure the storage bucket is persisted.

Actual results:

Recently I've received an issue report ( https://github.com/brief-rss/brief/issues/549 ) that in Firefox 128 using the "Clear recent history..." dialog this causes the extension pages to ask the user for this permission ("Allow <name> to store data in persistent storage?"). Further testing shows that between Firefox 60 (possibly earlier) and bug 1896949 this behavior could be triggered by clearing recent history with "Site settings" checked. Since bug 1896949 it's also triggered if "Cookies and site data" is checked, which is the default and much more likely to be needed than the off-by-default "Site settings" group.

Expected results:

Clearing site data should not affect storage permissions granted to extensions as extension data isn't considered site data for the purposes of this dialog.

Comment 1

[Denis Lisov]

Looks like the actual storage stays persistent (as in await navigator.storage.persisted() returning true), but for some reason calling navigator.storage.persist() causes the permission prompt to appear.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'WebExtensions::Untriaged' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 3

[Rob Wu [:robwu]]

Like you mentioned, the underlying issue has always been there. The change in bug 1896949 made this worse, by clearing the permissions by default.

According to the source code, the permissions is restored when the browser restarts.

We should ideally not remove this permission when the user clears "Site data".

I wonder whether the use of the browsingData extension API would also result in the revocation of the permission.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

:hsohaney, since you are the author of the regressor, bug 1896949, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1896949

Comment 6

[Paul Zühlcke [:pbz]]

Looks like this is a regression of Bug 1881797 where we aligned behavior between SiteDataManager and clear recent history. Even before Bug 1881797 SiteDataManager cleared those permissions. The bug merely fixed that inconsistency by having "clear recent history" also clear it.

I see three options:

a) Keep things as-is and call this bug WONTFIX. That might be undesired from the web extension side.
b) When clearing persistent-storage permissions skip extension principals. We would still clear those permissions for regular site data clearing. We'd have to check if those permissions are actually stored with extension principals though. Do you have a demo I could use to test this?
c) Only ever clear persistent-storage permissions as part of site settings. This provides nice separation of site data and permission clearing. However I'm not sure if it's desired. Some users might want to clear a sites storage and don't want it to be able to store a lot of site data again. If we make this choice users have to additionally clear site settings for the site or revoke the permission via the permission panel.

Comment 7

[Harshit Sohaney [:hsohaney]]

I'm on PTO until next week, I'll look into this once I'm back!

Comment 8

[Denis Lisov]

(In reply to Paul Zühlcke [:pbz] from comment #6)
[...]

b) When clearing persistent-storage permissions skip extension principals. We would still clear those permissions for regular site data clearing. We'd have to check if those permissions are actually stored with extension principals though. Do you have a demo I could use to test this?

You can use Brief version 2.7.2 ( https://addons.mozilla.org/firefox/downloads/file/4281119/brief-2.7.2.xpi ) as an example addon. The extension has unlimitedStorage and calls navigator.storage.persist() both from the background page (on extension startup) and from its main UI page (on load). The main UI page can be opened by clicking the extension browser action button.
My workflow while bisecting was like the following:

• Open UI (Extensions button - Brief)
• Clear recent history
• Refresh the UI page and check whether it displays the "Allow <name> to store data in persistent storage?" permission request.

Comment 9

[Paul Zühlcke [:pbz]]

Thanks!

I think the root issue here is that we create permission manager permissions on startup based on the extension permission (Please correct me if I got this wrong). Those permissions can be cleared during runtime. The extension permissions can't be cleared AFAIC. Ideally we would directly check extension permissions rather than going via the permission manager. With the current system extension permissions and permission manager permissions can get out of sync.

Rob, is my description accurate?

Comment 10

[Rob Wu [:robwu]]

(In reply to Paul Zühlcke [:pbz] from comment #9)

Thanks!

I think the root issue here is that we create permission manager permissions on startup based on the extension permission (Please correct me if I got this wrong). Those permissions can be cleared during runtime. The extension permissions can't be cleared AFAIC. Ideally we would directly check extension permissions rather than going via the permission manager. With the current system extension permissions and permission manager permissions can get out of sync.

Rob, is my description accurate?

Yes, for unlimitedStorage this is definitely true. There are other extensions tied to content permissions that may be revoked at runtime via the extension permissions API ("geolocation" extension permission -> "geo" content permission), but I am not too concerned with that (there is a bitrot patch at https://phabricator.services.mozilla.com/D50070 to control the extension permission toggled -> content permission update direction, not the other way around which is the issue of this bug).

Ideally we would directly check extension permissions rather than going via the permission manager.

One complication with this is that the usual way to synchronously check the availability of the permission is via principal.addonPolicy.hasPermission("unlimitedStorage"). This generally works well, unless principal.addonPolicy is null, which can be if the extension has not started yet, has unloaded (e.g. at browser shutdown), or is disabled (e.g. by the user). In the latter case you would have to map the UUID to the extension ID, and query the AddonManager for the addon to determine whether unlimitedStorage is part of userPermissions.permissions - but this operation is async because AddonManager.getAddonById is async. Is this acceptable?

Comment 11

[Paul Zühlcke [:pbz]]

Hi Harshit, see comment 6
B) seems like the easiest fix for now if we can get that to work.

Comment 12

[Harshit Sohaney [:hsohaney]]

So the goal is to exempt extension permissions from clearing when clearing "site data" but keep the behavior the same when clearing site settings.

Is that correct?

Comment 13

[Paul Zühlcke [:pbz]]

Correct :-)
Maybe you can test with the extension in comment 8

Comment 14

[Denis Lisov]

If comment 3 is correct in that the permission is always restored on browser restart, I'm not really sure whether dropping it when the user clears site settings makes sense.

Comment 15

[Paul Zühlcke [:pbz]]

You could argue both ways. Either that the user wants to clear all permissions when they clear site settings or that extensions are this parallel system that shouldn't get cleared. I don't have strong feelings about either since it's really more a of a stop-gap solution.

Generally I don't love that we're special casing things in ClearDataService when it's really the extension code that should address this.

(In reply to Rob Wu [:robwu] from comment #10)

(In reply to Paul Zühlcke [:pbz] from comment #9)

Thanks!

I think the root issue here is that we create permission manager permissions on startup based on the extension permission (Please correct me if I got this wrong). Those permissions can be cleared during runtime. The extension permissions can't be cleared AFAIC. Ideally we would directly check extension permissions rather than going via the permission manager. With the current system extension permissions and permission manager permissions can get out of sync.

Rob, is my description accurate?

Yes, for unlimitedStorage this is definitely true. There are other extensions tied to content permissions that may be revoked at runtime via the extension permissions API ("geolocation" extension permission -> "geo" content permission), but I am not too concerned with that (there is a bitrot patch at https://phabricator.services.mozilla.com/D50070 to control the extension permission toggled -> content permission update direction, not the other way around which is the issue of this bug).

Ideally we would directly check extension permissions rather than going via the permission manager.

One complication with this is that the usual way to synchronously check the availability of the permission is via principal.addonPolicy.hasPermission("unlimitedStorage"). This generally works well, unless principal.addonPolicy is null, which can be if the extension has not started yet, has unloaded (e.g. at browser shutdown), or is disabled (e.g. by the user). In the latter case you would have to map the UUID to the extension ID, and query the AddonManager for the addon to determine whether unlimitedStorage is part of userPermissions.permissions - but this operation is async because AddonManager.getAddonById is async. Is this acceptable?

As an alternative, could the extension code listen for permission changes and re-add the extension permissions as needed? The permission manager dispatches a "perm-changed" observer message here: https://searchfox.org/mozilla-central/rev/e637cd67f98ed4272d13a96db9a7674689122dcb/extensions/permissions/PermissionManager.cpp#3035-3041

Comment 16

[Rob Wu [:robwu]]

(In reply to Paul Zühlcke [:pbz] from comment #15)

As an alternative, could the extension code listen for permission changes and re-add the extension permissions as needed? The permission manager dispatches a "perm-changed" observer message here: https://searchfox.org/mozilla-central/rev/e637cd67f98ed4272d13a96db9a7674689122dcb/extensions/permissions/PermissionManager.cpp#3035-3041

If it does not result in dataloss, sounds acceptable to me. Is restoring a permission upon removal enough to maintain the desired behavior without chance of evicting over-quota items?

Comment 17

[Paul Zühlcke [:pbz]]

(In reply to Rob Wu [:robwu] from comment #16)

(In reply to Paul Zühlcke [:pbz] from comment #15)

As an alternative, could the extension code listen for permission changes and re-add the extension permissions as needed? The permission manager dispatches a "perm-changed" observer message here: https://searchfox.org/mozilla-central/rev/e637cd67f98ed4272d13a96db9a7674689122dcb/extensions/permissions/PermissionManager.cpp#3035-3041

If it does not result in dataloss, sounds acceptable to me. Is restoring a permission upon removal enough to maintain the desired behavior without chance of evicting over-quota items?

I don't see anything in the code that would listen for the persistent-storage permission. Andrew, do you know?

Comment 18

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

QuotaManager tracks the grant of navigator.storage.persist() durably once granted in the metadata for the origin. If QuotaManager is told to wipe the contents of the origin, the persistent bit will be removed. The "persistent-storage" permission only impacts the "should we prompt" decision when navigator.storage.persist() is invoked; if the permission is granted, the bit will be set without prompting.

Note that there is also an explicit API, nsIQuotaManagerService::persist that can also be used externally. SiteDataTestUtils.persist set the permission and calls the QMS API, for example.

Comment 19

[Paul Zühlcke [:pbz]]

Thanks! Sounds like re-adding the permission should work then. Reading your comment I was also thinking whether the extension management code could make sure persistend storage grants are ready whenever the extension uses the API. It's either that or the QM taking into account the extension "permissions" too.
Lots of ways to go about this!

Comment 20

[Rob Wu [:robwu]]

:hsohaney are you going to implement the suggestion above?

Comment 21

[Harshit Sohaney [:hsohaney]]

I'm currently on PTO, but am looking into what the best solution would be here. I'll have something by the end of this week!

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1881797

Comment 23

[Harshit Sohaney [:hsohaney]]

Attachment: Bug 1907732 - (part 1) Exempt addon principals from clearing with site data. r?pbz

Comment 24

[Harshit Sohaney [:hsohaney]]

Attachment: Bug 1907732 - (part 2) Tests for exempting addon principals from clearing with site data. r?pbz

Depends on D219825

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

I'm inclined to let it ride to release with 131, but once these patches land, if you think they are a good candidate for beta 130, please feel free to request uplift. Thanks!

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea9466322e09
(part 1) Exempt addon principals from clearing with site data. r=pbz,robwu
https://hg.mozilla.org/integration/autoland/rev/a3bfe94d2f35
(part 2) Tests for exempting addon principals from clearing with site data. r=pbz,robwu

Comment 27

[tszentpeteri]

https://hg.mozilla.org/mozilla-central/rev/ea9466322e09
https://hg.mozilla.org/mozilla-central/rev/a3bfe94d2f35

Comment 28

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:hsohaney, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox130 to wontfix.

For more information, please visit BugBot documentation.

Comment 29

[Harshit Sohaney [:hsohaney]]

This isn't too critical a change, doesn't need an uplift imo!