1337627 - Remove support for remote discovery pane in about:addons

Status: RESOLVED FIXED

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

Description

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Loading remote web content in the parent process is dangerous in a post-sandbox world, as it means loading remote content outside the sandbox. About:addons loads remote content, mainly from our own sites, but also remote sites like google analytics. From what i have seen so far, all connections seem to be SSL, so that it a mitigating factor, but we should consider moving this to a content process (either existing or new).

PS Couldn't find a more specific component, please move as appropriate.

Comment 1

[:Gijs (he/him)]

This should be fairly trivial, ie setting remote=true on the <browser> we use to frame the discovery pane. Dave, do you or Andrew have cycles for this?

Comment 2

[Dave Townsend [:mossop]]

I seem to recall that it isn't quite that simple but I'll take a look and see what the state is.

Comment 3

[(no longer active)]

Mossop and I discussed a Gecko bug that he was hitting about this today.  I'll file it tomorrow.

Comment 4

[(no longer active)]

Mossop, can you please post a WIP patch here that demonstrates the exception we were debugging on IRC yesterday?  Thanks!

Comment 5

[Dave Townsend [:mossop]]

Attachment: WIP patch

This is the WIP, really only the change to extensions.xul is necessary and the exception happens when you open the add-ons manager UI.

Comment 6

[Dave Townsend [:mossop]]

For the gecko bug here.

Comment 7

[(no longer active)]

(In reply to Dave Townsend [:mossop] from comment #6)
> For the gecko bug here.

Not sure which patch you are asking for.  The fix will be in bug 1340747.

Comment 8

[(no longer active)]

Dave, Michael posted a patch in bug 1340747 which I just r+ed.  That should unblock you here.  Let me know if you run into any other issues.

Comment 9

[Dave Townsend [:mossop]]

Attachment: Bug 1337627: Load the discovery pane in a remote process.

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

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 8842980 [details]
Bug 1337627: Load the discovery pane in a remote process.

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

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 8842980 [details]
Bug 1337627: Load the discovery pane in a remote process.

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

Comment 12

[Dave Townsend [:mossop]]

Comment on attachment 8842980 [details]
Bug 1337627: Load the discovery pane in a remote process.

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

Comment 13

[Dave Townsend [:mossop]]

I'm not actively working on this.

Comment 14

[David Durst [:ddurst]]

Can this old patch be dusted off now?

Comment 15

[Dave Townsend [:mossop]]

(In reply to David Durst [:ddurst] from comment #14)

Can this old patch be dusted off now?

Someone can for sure, I have no time to work on this though. I seem to remember that the patch basically worked but was hitting some test failures that I never got around to fixing.

Comment 16

[:Gijs (he/him)]

This was disabled on 68 in bug 1540173, right? Should we mark this fixed and file a follow-up to rip out the old code from 70 once we merge?

Comment 17

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

(In reply to :Gijs (he/him) from comment #16)

This was disabled on 68 in bug 1540173, right? Should we mark this fixed and file a follow-up to rip out the old code from 70 once we merge?

Yes, to be precise the new discovery panel has been enabled by default (along with the HTML about:addons) by Bug 1555012.

Marking this one as fixed sounds reasonable to me too (we will definitely not work on any other fixes, the fix we did choose to apply is the "enabling the new rewritten discovery panel" one, and Bug 1555012 has been already landed in beta).

Currently we have Bug 1558982 for the "Remove XUL about:addons" step, we could use that bugzilla issue to follow-up on it, or file a new one specifically related to ripping off the old discovery panel (and the "about:config" preference that allows a user to switch back to it) and make it a blocker for Bug 1558982.

Comment 18

[Rob Wu [:robwu]]

This bug can be closed once we stop supporting extensions.htmlaboutaddons.discover.enabled=false

That includes the removal of toolkit/mozapps/extensions/test/browser/browser_discovery.js
The pref is also used in https://searchfox.org/mozilla-central/rev/e62c920f7f6463239c6634113f8a8351e263b936/toolkit/mozapps/extensions/test/browser/browser_html_discover_view_clientid.js#165,200 , but its purpose in the test can be fulfilled by using extensions.getAddons.showPane instead.

Comment 19

[Andrew Swan [:aswan]]

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

This bug can be closed once we stop supporting extensions.htmlaboutaddons.discover.enabled=false

What's stopping us from doing that?

Comment 20

[:Gijs (he/him)]

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

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

This bug can be closed once we stop supporting extensions.htmlaboutaddons.discover.enabled=false

What's stopping us from doing that?

+ni for this

Comment 21

[Rob Wu [:robwu]]

The extensions.htmlaboutaddons.discover.enabled pref is not set in mail/app/profile/all-thunderbird.js, so Thunderbird is still relying on being able to load some remote content.

They can probably restore their requested functionality in comm-central if they really need it. If the code gets removed, ping the people who were involved with bug 1558860.

Comment 22

[Rob Wu [:robwu]]

(Adjusting summary so that this bug can be used as the parent of bugs related to the remote disco pane)

Comment 23

[:Gijs (he/him)]

Because of the security impact, I'd really like this to go away. What's stopping us from doing that?

Comment 24

[Rob Wu [:robwu]]

I'd like to remove support for remotely hosted discopane.

It's still in the tree because Thunderbird relies on it. But they're on ESR68, and the next ESR is still a few months away.
I want to remove the code for the remotely hosted discopane soonish.

Does Thunderbird already have a viable replacement for the discover pane in about:addons in the planning?

Comment 25

[Mark Striemer [:mstriemer]]

I'm starting to look at moving the sidebar into the HTML document which means this would be the only content in the XUL document. Removing it definitely makes more sense than the work it will take to support it. I think it will need to go in 73.

Comment 26

[Magnus Melin [:mkmelin]]

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

Does Thunderbird already have a viable replacement for the discover pane in about:addons in the planning?

Yep, I filed bug 1600923 to take care of it. Thanks for notifying!

Comment 27

[Mark Striemer [:mstriemer]]

Attachment: Bug 1337627 - Remove remote discovery pane from about:addons r?robwu

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e542391fb8c5
Remove remote discovery pane from about:addons r=robwu,fluent-reviewers

Comment 29

[Narcis Beleuzu [:NarcisB]]

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

Comment 30

[Miruna Curtean]

Hello,

Currently the extensions.htmlaboutaddons.discover.enabled is no longer listed by default in about:config in FF Nightly 73.0a1 20191210095443, while before in FF 72 it was enabled and set as True.
Is it correct to interpret that a sanity check for the about:addons page needs to be performed in order to verify this ticket or are there other test areas to check?
Thank you

Comment 31

[Mark Striemer [:mstriemer]]

I don't think this needs any specific QA. This shouldn't have any effect without having made changes to about:config previously and we didn't really support the code that's been removed.