1539598 - Restrict mozAddonManager's special privileges to calls from the discovery pane

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Description

[Kris Maglione [:kmag]]

mozAddonManager has special powers above those of InstallTrigger to allow one-click installs/enables/disables of add-ons from the discovery pane. Since that functionality isn't meant to be used outside of the discovery pane, we should limit those privileges to usage in the discovery pane, when loaded in about:addons, rather than offering it to AMO in general.

We should also limit it to listed add-ons being installed from AMO, since the discovery pane will never offer unlisted add-ons.

Comment 1

[William Durand [:willdurand]]

mozAddonManager has special powers above those of InstallTrigger to allow one-click installs/enables/disables of add-ons from the discovery pane. Since that functionality isn't meant to be used outside of the discovery pane

Currently, AMO and the current web-based discovery pane use mozAddonManager as the main (and only) mechanism to install add-ons. InstallTrigger is not used anymore since August/September 2018.

Comment 2

[Andrew Swan [:aswan]]

(In reply to William Durand [:willdurand] from comment #1)

Currently, AMO and the current web-based discovery pane use mozAddonManager as the main (and only) mechanism to install add-ons. InstallTrigger is not used anymore since August/September 2018.

Not sure what bearing that has on this bug? InstallTrigger is exposed to all web sites and is publicly documented so we can't remove it. This bug is about changing the behavior of mozAddonManager when it is used outside of the disco pane.

Comment 3

[William Durand [:willdurand]]

(In reply to Andrew Swan [:aswan] from comment #2)

Not sure what bearing that has on this bug?

Sorry, maybe I misunderstood the original issue but the quote thereafter is not entirely accurate and that's what I meant: AMO and disco pane both have enhanced install buttons with the install/remove/enable/disable features. It's the same button on both apps currently.

mozAddonManager has special powers [...] to allow one-click installs/enables/disables of add-ons from the discovery pane

Comment 4

[Andrew Williamson [:eviljeff]]

Also makes

"we should limit those privileges to usage in the discovery pane ... rather than offering it to AMO in general."

a non-starter.

Comment 5

[Kris Maglione [:kmag]]

(In reply to Andrew Williamson [:eviljeff] from comment #4)

Also makes

"we should limit those privileges to usage in the discovery pane ... rather than offering it to AMO in general."

a non-starter.

It is, in fact, a finisher.

Comment 6

[Andrew Swan [:aswan]]

Attachment: Bug 1539598 Require pre-install confirmation on all addons installs from outside about:addons r=kmag

Comment 7

[Andrew Swan [:aswan]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a05ac03465cb38ba5a798cb762ccf20a6635f23
Bug 1539598 Require pre-install confirmation on all addons installs from outside about:addons r=kmag

Comment 8

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/7a05ac03465c

Comment 9

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/75d0bd84d83f
Backed out changeset 7a05ac03465c for turning bug 1531406 into perma on a CLOSED TREE

Comment 10

[Oana Pop-Rus]

Backed out changeset 7a05ac03465c (Bug 1539598) for turning Bug 1531406 into perma

Backout: https://hg.mozilla.org/integration/autoland/rev/75d0bd84d83f607fde8ed552a7c38489e848b249

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=success%2Csuperseded%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&searchStr=android%2C4.3%2Capi16%2B%2Cpgo%2Cmochitests%2Ctest-android-em-4.3-arm7-api-16%2Fpgo-robocop-3%2Cm%28rc3%29&group_state=expanded&tochange=a36718c8163e75b7aa4b2e49870a96d1bae627ff&fromchange=ff6b2d1aff7a8e1bd32b6c7134fdf4704c57034d&selectedJob=238024885

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=238024885&repo=mozilla-inbound&lineNumber=1807

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/75d0bd84d83f

Comment 12

[Andrew Swan [:aswan]]

Nick, can you help me out here? With this patch we now show a confirmation prompt before installing any addons. The existing test of theme installation knows nothing about this prompt so it times out, I could use some help figuring out how to get the test to detect and respond to the prompt.

The prompt is actually launched indirectly through xpcom:
https://searchfox.org/mozilla-central/rev/14dc5b7d8a6da1854b2f9f33f1da77a97368cd54/toolkit/mozapps/extensions/AddonManager.jsm#2575-2584
https://searchfox.org/mozilla-central/rev/14dc5b7d8a6da1854b2f9f33f1da77a97368cd54/mobile/android/components/XPIDialogService.js#16-44

So a terribly hacky solution would just be to override that component for the duration of this test. Can we find something better?

Comment 13

[Nick Alexander :nalexander [he/him]]

(In reply to Andrew Swan [:aswan] from comment #12)

Nick, can you help me out here? With this patch we now show a confirmation prompt before installing any addons. The existing test of theme installation knows nothing about this prompt so it times out, I could use some help figuring out how to get the test to detect and respond to the prompt.

The prompt is actually launched indirectly through xpcom:
https://searchfox.org/mozilla-central/rev/14dc5b7d8a6da1854b2f9f33f1da77a97368cd54/toolkit/mozapps/extensions/AddonManager.jsm#2575-2584
https://searchfox.org/mozilla-central/rev/14dc5b7d8a6da1854b2f9f33f1da77a97368cd54/mobile/android/components/XPIDialogService.js#16-44

So a terribly hacky solution would just be to override that component for the duration of this test. Can we find something better?

I see no examples of interacting with Gecko prompts in the existing Robocop tests. Looking at PromptService.java it's clear that we could watch for those native messages and do appropriate things in a text context, but this is pointless: the GeckoView API is different and already tested.

And given that this Robocop test is already mostly expressed in JS, I'd do exactly as you say and use XPCOM wizardry to mock out the prompt around https://searchfox.org/mozilla-central/source/mobile/android/tests/browser/robocop/testThemeInstall.js#46.

For review, flag :petru, :vbaicu, or (as a last resort) me. Good luck!

Comment 14

[Andrew Swan [:aswan]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7e2018b550c9e0cb80644b23791e673d152ac3f
Bug 1539598 Require pre-install confirmation on all addons installs from outside about:addons r=kmag

Comment 15

[Andreea Pavel [:apavel]]

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

Comment 16

[CosminB]

Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Comment 17

[Andrew Swan [:aswan]]

Manual testing can be pretty simple:

1. Install a theme from the discovery pane ("Get Extensions" in about:addons), no prompt should be shown before installation
2. Install a theme from addons.mozilla.org, a prompt should be shown before installation

Comment 18

[CosminB]

Attachment: Bug1539598.gif

This issue is verified as fixed on Firefox 68.0a1 (20190421212635) under Win 7 64-bit and Mac OS X 10.14.1.

Please see the attached video.

Comment 19

[:Gijs (he/him)]

Comment on attachment 9055647 [details]
Bug 1539598 Require pre-install confirmation on all addons installs from outside about:addons r=kmag

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: mitigating sec-high/sec-crit sandbox escape paths
• User impact if declined: see above
• Fix Landed on Version: 68
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): We end up prompting in more cases, but it's a targeted patch that applies cleanly, and in theory nothing much besides about:addons should be using mozAddonManager anyway. The main risk here for esr is automated test coverage on an aging branch needing special hand-holding for the change in behaviour.
• String or UUID changes made by this patch: nope

Comment 20

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Comment on attachment 9055647 [details]
Bug 1539598 Require pre-install confirmation on all addons installs from outside about:addons r=kmag

Helps mitigate a sandbox escape. Approved for 60.8esr.

Comment 21

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/cc2fe419273435137188f5aeb5025d10a02fafe2

Comment 22

[CosminB]

Attachment: Bug1539598.gif

This issue is verified as fixed on Firefox 60.8.0esr (20190702220624) under Win 7 64-bit and Mac OS X 10.14.1.

Comment 23

[Al Billings [:abillings - ex-MoCo]]

Removing CVE and marking it as "burned" for public purposes.