1597751 - Default permissions are not imported for private browsing and user contexts

Status: RESOLVED FIXED

(Core :: Permission Manager, defect, P3)

Description

[David Durst [:ddurst]]

In Nightly 72, in a private window, attempting to install the XPI from private-network.firefox.com presents the "You are attempting to install an add-on from private-network.firefox.com" doorhanger, despite the domain being whitelisted since 70.

This is not reproducible in 70 or 71, but I don't have a regression range. CC to baku for awareness and next steps.

Marking this as confidential just because permissions-related things always make me nervous, and more so if this extends to other aspects of FPN.

Comment 1

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

A changes to the permissions that landed in 72 and that may be triggering this is Bug 1422056 (at least that is one that I'm aware of and did come to my mind right now).

I haven't run a mozregress to confirm that it really is (or looked to the patches landed from that to see if they may be able to trigger what described by David), but in the meantime it seems worth to ask an opinion about it to :pbz and :johannhn

Comment 2

[Paul Zühlcke [:pbz]]

Thanks Luca!

As you mentioned, this is a regression of Bug 1422056. It should only affect Beta and Nightly, not Release See pref config here.
The permission is added by origin here: https://searchfox.org/mozilla-central/rev/652014ca1183c56bc5f04daf01af180d4e50a91c/browser/app/permissions#24. Since the origin string does not include any origin attributes, the permission is only applied for the normal window.

An intermediate fix would be to add (probably also for the other entries):

origin	install	1	https://private-network.firefox.com^privateBrowsingId=1
origin	install	1	https://fpn.firefox.com^privateBrowsingId=1

Ideally though, permissions added via this file should be set for all origin attributes. We should look into this, especially when we are going to isolate by user context and private browsing in release at some point.

Comment 3

[(no longer active)]

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

An intermediate fix would be to add (probably also for the other entries):

origin	install	1	https://private-network.firefox.com^privateBrowsingId=1
origin	install	1	https://fpn.firefox.com^privateBrowsingId=1

That would mean we would have the same bug for users using containers...

I think the correct solution is to modify nsPermissionManager::_DoImport() to get it to save this permission for all of the combinations of {normalWindow, privateWindow}x{user's default containers}. You can use do_ImportModule() to import ContextualIdentityService.jsm and iterate over the available public identities (see here for an example on how to do this).

Comment 4

[Paul Zühlcke [:pbz]]

Thanks Ehsan!

(In reply to :ehsan akhgari from comment #3)

I think the correct solution is to modify nsPermissionManager::_DoImport() to get it to save this permission for all of the combinations of {normalWindow, privateWindow}x{user's default containers}.

I'll work on a patch for that.

I don't think this needs to be confidential since it only affects pre-release channels and can't be abused.

Comment 5

[(no longer active)]

I agree.

Comment 6

[Paul Zühlcke [:pbz]]

Attachment: Bug 1597751 - nsPermissionManager: Import default permissions for all userContexts and privateBrowsing. r=ehsan

Comment 7

[Paul Zühlcke [:pbz]]

Attachment: Bug 1597751 - Import default permissions for privateBrowsing. r=timhuang!

This patch adds a method to import default permissions for private browsing and containers.
Private browsing permissions are imported after default perms have been imported from file.
User context permissions are imported once the ContextualIdentityService has been initialized.

Comment 8

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

Hi Paul,
I wanted to double-check with you if the changes we landed in Firefox 85 from Bug 1681331 would be enough to fix this issue related to the FPN addon installation flow even once Bug 1680237 is going to be landed.
(the changes we did apply from Bug 1681331 should allow us to host privileged-signed extension xpi files on 3rd party websites without making that installation flow to trigger the "3rd party website addon install" doorhanger, which I think is the reason for the install site permission added for the https://fpn.firefox.com host, and the reason to import that site permission also for all userContexts and privateBrowsing, but it would be a replacement to give install site permissions to non AMO websites, but not for any of the other site permissions)

Comment 9

[Paul Zühlcke [:pbz]]

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #8)

Hi Paul,
I wanted to double-check with you if the changes we landed in Firefox 85 from Bug 1681331 would be enough to fix this issue related to the FPN addon installation flow even once Bug 1680237 is going to be landed.
(the changes we did apply from Bug 1681331 should allow us to host privileged-signed extension xpi files on 3rd party websites without making that installation flow to trigger the "3rd party website addon install" doorhanger, which I think is the reason for the install site permission added for the https://fpn.firefox.com host, and the reason to import that site permission also for all userContexts and privateBrowsing, but it would be a replacement to give install site permissions to non AMO websites, but not for any of the other site permissions)

Yes, I think Bug 1681331 fixes this for the addon case, since you're not checking for the "install" permission anymore? Bug 1680237 is really just a pref flip for the permission check, so it should also work in private browsing in release.
However, we still need to import the other default permissions for user contexts + private browsing.

Comment 10

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

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

Yes, I think Bug 1681331 fixes this for the addon case, since you're not checking for the "install" permission anymore? Bug 1680237 is really just a pref flip for the permission check, so it should also work in private browsing in release.
However, we still need to import the other default permissions for user contexts + private browsing.

yep, I completely agree, this issue is still valid and can eventually just be repurposed as is for the general site permissions issue (for repurposed I mean just "changing the title to don't specifically refer to the private-network.firefox.com install permission").

Comment 11

[Paul Zühlcke [:pbz]]

Thanks for the review! While working on the test, I've noticed that we no longer run the import on the main thread (Bug 1363541). Does that mean we don't have to postpone access to ContextualIdentityService and and can import the user context default permissions directly?
This would make the code simpler and also allow us to unit test the default permission import for user context.
The tests in browser/base/content/test/performance/browser_startup* seem to pass with the direct import, while they failed when I last tried it about a year ago.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe98ac52cf7c
Import default permissions for privateBrowsing. r=timhuang

Comment 13

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/fe98ac52cf7c

Comment 14

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

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.