Closed Bug 1337627 Opened 4 years ago Closed 1 year ago

Remove support for remote discovery pane in about:addons

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla73
Tracking Status
firefox73 --- fixed

People

(Reporter: pauljt, Assigned: mstriemer)

References

(Blocks 1 open bug)

Details

(Whiteboard: triaged)

Attachments

(2 files, 1 obsolete file)

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.
Component: General → DOM: Content Processes
Product: Firefox → Core
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?
Component: DOM: Content Processes → Add-ons Manager
Flags: needinfo?(dtownsend)
Product: Core → Toolkit
I seem to recall that it isn't quite that simple but I'll take a look and see what the state is.
Assignee: nobody → dtownsend
Flags: needinfo?(dtownsend)
Mossop and I discussed a Gecko bug that he was hitting about this today.  I'll file it tomorrow.
Flags: needinfo?(ehsan)
Depends on: 1340747
Mossop, can you please post a WIP patch here that demonstrates the exception we were debugging on IRC yesterday?  Thanks!
Flags: needinfo?(ehsan) → needinfo?(dtownsend)
Attached patch WIP patch (obsolete) — Splinter Review
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.
Flags: needinfo?(dtownsend)
For the gecko bug here.
Flags: needinfo?(ehsan)
(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.
Flags: needinfo?(ehsan)
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.
Whiteboard: triaged
Blocks: 1344053
Attachment #8839578 - Attachment is obsolete: true
I'm not actively working on this.
Assignee: dtownsend → nobody

Can this old patch be dusted off now?

Flags: needinfo?(dtownsend)
Priority: -- → P1

(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.

Flags: needinfo?(dtownsend)
Assignee: nobody → mstriemer
See Also: → 1540173

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?

Flags: needinfo?(mstriemer)
Flags: needinfo?(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.

Flags: needinfo?(lgreco)
Blocks: 1558982
Flags: needinfo?(mstriemer)
Priority: P1 → P3

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.

(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?

(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

Flags: needinfo?(rob)

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.

Flags: needinfo?(rob)

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

Depends on: 1571125
Summary: about:addons loads remote web content in the parent process → Remove support for remote discovery pane in about:addons

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

Flags: needinfo?(mstriemer)

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?

Flags: needinfo?(mstriemer) → needinfo?(mkmelin+mozilla)

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.

Depends on: 1600923

(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!

Flags: needinfo?(mkmelin+mozilla)
Pushed by mstriemer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e542391fb8c5
Remove remote discovery pane from about:addons r=robwu,fluent-reviewers
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla73

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

Flags: needinfo?(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.

Flags: needinfo?(mstriemer) → qe-verify-
You need to log in before you can comment on or make changes to this bug.