Reset the engine of users having a default engine likely installed by a legacy add-on

VERIFIED FIXED in Firefox 57

Status

()

enhancement
P1
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: florian, Assigned: florian)

Tracking

Trunk
Firefox 59
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox57+ verified, firefox58+ verified, firefox59 verified)

Details

(Whiteboard: [fxsearch])

Attachments

(2 attachments)

(Assignee)

Description

a year ago
Lots of users are using (potentially without even knowing it) search plugins that were installed and set as the default by (sometimes malicious) legacy add-ons.

Now that Firefox 57 only enables WebExtension add-ons, these legacy add-ons are no longer installed, but this doesn't undo the changes made to users' search settings. With the 57 release, it's now time to cleanup.
(Assignee)

Comment 1

a year ago
Summary of the plan:

- this will be a one-time reset, happening when users upgrade.

- users currently having as default an engine of unknown origin (ie. installed before Firefox 45, or by a legacy add-on, or sideloaded by malware) will have their engine silently reset to the built-in default.

- users currently having as default an engine known to come from an https origin will be offered the about:searchreset UI the next time they use one of our search access points. The reason why we need to prompt here is that it's likely that a large portion of these users have intentionally installed the engine themselves through open search discovery, and we don't want to silently undo user choice.

- users currently having as default any of the engines we ship, or an engine shipped in a distribution partner build should be completely unaffected by this.
(Assignee)

Comment 2

a year ago
Posted patch PatchSplinter Review
Attachment #8931166 - Flags: review?(adw)
Comment on attachment 8931166 [details] [diff] [review]
Patch

Review of attachment 8931166 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks for the explanations here and on IRC.

::: browser/components/nsBrowserGlue.js
@@ +2199,5 @@
> +        if (currentEngine._loadPath.startsWith("[https]")) {
> +          Services.prefs.setCharPref("browser.search.reset.status", "pending");
> +        } else {
> +          // Can't call resetToOriginalDefaultEngine because it doesn't
> +          // unhide the engine.

Hmm, I see.  That seems weird.  That method is called by two UI points that allow the user to get back into the default state, the search pref pane and the self-support service.  Shouldn't it handle the possibility that the original default is hidden?

I mention it in case you think so too -- to be handled in another bug of course.
Attachment #8931166 - Flags: review?(adw) → review+
(Assignee)

Comment 4

a year ago
(In reply to Drew Willcoxon :adw from comment #3)
> Comment on attachment 8931166 [details] [diff] [review]
> Patch
> 
> Review of attachment 8931166 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, thanks for the explanations here and on IRC.

Thanks for the quick review!

> ::: browser/components/nsBrowserGlue.js
> @@ +2199,5 @@
> > +        if (currentEngine._loadPath.startsWith("[https]")) {
> > +          Services.prefs.setCharPref("browser.search.reset.status", "pending");
> > +        } else {
> > +          // Can't call resetToOriginalDefaultEngine because it doesn't
> > +          // unhide the engine.
> 
> Hmm, I see.  That seems weird.  That method is called by two UI points that
> allow the user to get back into the default state, the search pref pane and
> the self-support service.  Shouldn't it handle the possibility that the
> original default is hidden?
> 
> I mention it in case you think so too -- to be handled in another bug of
> course.

I agree. I didn't try to change this because it seems the other consumers are correctly taking care of unhiding the plugin before calling resetToOriginalDefaultEngine, and I didn't want to risk affecting them. We can cleanup in a follow-up.

Comment 5

a year ago
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/707917eb9b1b
Reset the engine of users having a default engine likely installed by a legacy add-on, r=adw.

Comment 6

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/707917eb9b1b
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
(Assignee)

Comment 7

a year ago
Comment on attachment 8931166 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: Not a regression. After dropping legacy add-ons in 57 we forgot to cleanup the search plugins.
[User impact if declined]: Using potentially unwanted search engines.
[Is this code covered by automated tests?]: The search service and about:searchreset changes are covered by tests. Unfortunately, the migration code path used to trigger the reset is not covered by automated test.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: Yes. Adrien is already on it.
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: Well...
[Why is the change risky/not risky?]: I did my best to minimize the risks, but we are introducing new behaviors so I can't be fully confident without this having baked in pre-releases. I think it should be fine though :-).
[String changes made/needed]: none.
Attachment #8931166 - Flags: approval-mozilla-release?
Attachment #8931166 - Flags: approval-mozilla-beta?
Comment on attachment 8931166 [details] [diff] [review]
Patch

Need that in 58
Attachment #8931166 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 10

a year ago
The previous patch is setting an integer pref with the value "silent". Surprisingly this works, it just saves an int pref with the value 0, and then the reading code also works because it provides a default value... But it would be better to fix this typo.
Bug to fix to resetToOriginalDefaultEngine is bug 1415727
Attachment #8931166 - Flags: approval-mozilla-release? → approval-mozilla-release+
Comment on attachment 8931415 [details] [diff] [review]
fix typo in setCharPref

[Triage Comment]
To keep a clean db
Attachment #8931415 - Flags: approval-mozilla-release+
Attachment #8931415 - Flags: approval-mozilla-beta+
Whiteboard: [fxsearch]
Verified as well on RC 57.0.1/20171128222554 / Update Channel -release-localtest on Ubuntu 16.14/ Windows10 x64.
Status: RESOLVED → VERIFIED
Am I right that in practice "likely installed by a legacy add-on" means "anything other than Google"?

Because that's what was done for me — I had not used any search-related add-ons and FF still tried to convince me to reset search provider to google.
If it's a method to get paid, IMO this could be more honest.
(Assignee)

Comment 20

a year ago
(In reply to sausagefactory0 from comment #19)
> Am I right that in practice "likely installed by a legacy add-on" means
> "anything other than Google"?

No, not at all. It means installed before Firefox 45, or installed by a legacy add-on, or by malware.

> Because that's what was done for me — I had not used any search-related
> add-ons and FF still tried to convince me to reset search provider to google.

If you believe you are in neither of the 3 cases I mentioned above and would like me to check what happened for you, you can set your default engine back to what it was before the reset, then go to about:telemetry, and in the "Environment Data" section, look for the defaultSearchEngineData.loadPath line. Show me what's listed there (either in a comment here, or email it to me if you don't want to share it publicly). You can also go in about:config and check the value of the browser.search.reset.status preference.
(In reply to Florian Quèze [:florian] from comment #20)
> (In reply to sausagefactory0 from comment #19)
> > Am I right that in practice "likely installed by a legacy add-on" means
> > "anything other than Google"?
> 
> No, not at all. It means installed before Firefox 45, or installed by a
> legacy add-on, or by malware.

What bucket do OpenSearch plugins, installed via `window.external.AddSearchProvider(url)`, fall into?

I'm seeing reports that we're incorrectly prompting users who had intentionally set a new default engine via that method.
(Assignee)

Comment 22

a year ago
(In reply to Dan Callahan [:callahad] from comment #21)
> (In reply to Florian Quèze [:florian] from comment #20)
> > (In reply to sausagefactory0 from comment #19)
> > > Am I right that in practice "likely installed by a legacy add-on" means
> > > "anything other than Google"?
> > 
> > No, not at all. It means installed before Firefox 45, or installed by a
> > legacy add-on, or by malware.
> 
> What bucket do OpenSearch plugins, installed via
> `window.external.AddSearchProvider(url)`, fall into?
> 
> I'm seeing reports that we're incorrectly prompting users who had
> intentionally set a new default engine via that method.

Depends on the url, if the url starts with https we will prompt. If it starts with something else (chrome://, file://, http://, data:, ...) we'll reset silently without prompting.

To be very clear: we prompt when we believe the user may have customized intentionally, we reset without prompting if it's unlikely to be the user's intent.

Comment 23

a year ago
(In reply to Dan Callahan [:callahad] from comment #21)
> I'm seeing reports that we're incorrectly prompting users who had
> intentionally set a new default engine via that method.

If you were talking about people with StartPage as their default engine having it reset, then it is likely because their homepage (https://www.startpage.com/) serves the OpenSearch XML over http. As per the patch, resetting if the search plugin was served over http should be expected behaviour.

Strangely, their dedicated 'Add to Firefox' page `https://www.startpage.com/eng/download-startpage-plugin.html#hmb` actually does serve the XML over https, so people who installed it by going to that page (instead of from the home page) shouldn't be having their search engine reset, and indeed it wasn't in my testing.

See https://imgur.com/a/bpBOT for the relevant parts of the source of the two web pages.
(In reply to Florian Quèze [:florian] from comment #20)
> If you believe you are in neither of the 3 cases I mentioned above and would
> like me to check what happened for you, you can set your default engine back
> to what it was before the reset, then go to about:telemetry, and in the
> "Environment Data" section, look for the defaultSearchEngineData.loadPath
> line. Show me what's listed there (either in a comment here, or email it to
> me if you don't want to share it publicly). You can also go in about:config
> and check the value of the browser.search.reset.status preference.

It's "[https]addons.mozilla.org/startpage-https.xml"
(Assignee)

Comment 25

a year ago
(In reply to sausagefactory0 from comment #24)
> (In reply to Florian Quèze [:florian] from comment #20)
> > If you believe you are in neither of the 3 cases I mentioned above and would
> > like me to check what happened for you, you can set your default engine back
> > to what it was before the reset, then go to about:telemetry, and in the
> > "Environment Data" section, look for the defaultSearchEngineData.loadPath
> > line. Show me what's listed there (either in a comment here, or email it to
> > me if you don't want to share it publicly). You can also go in about:config
> > and check the value of the browser.search.reset.status preference.
> 
> It's "[https]addons.mozilla.org/startpage-https.xml"

This should show you an about:searchreset prompt, not reset it silently for you.
Yeah that's what happened. And it shouldn't have happened.

Comment 27

a year ago
A few questions:

1. The OpenSearch XML file format is a standard used by many browsers, and Firefox continues to support it. What is the logic in considering this a “legacy add-on” that will be reset (in future versions as well), while Firefox continues to support new installations with this standard?
2. How does Mozilla expect to inform/transition all the users that have very intentionally installed an engine through OpenSearch XML, to the WebExtension of that same publisher? It seems unfair and not candid to (silently!) move those people to Google.
3. Firefox tries to reset every search provider in its shortlist to Google, except for Google. What would cause Firefox to worry that even the search providers it has shortlisted (aside from Google) are sneaking malware onto user machines?
4. The content of the Startpage OpenSearch XML file points to an HTTPS url for searches. If there is anything to be gleaned from the prefix of a URL, wouldn’t the prefix of the search string (rather than the installation file) be a better indication of the user’s intent?
5. Why will this repeatedly reset (often silently) until Release 59, even after users add and re-add the OpenSearch plugin?
6. Why the rush to insert this invasive change without public announcement or discussion? Why the silent resets when so many intended installations are affected?

And a few requests, as this is silently affecting so many intentional installations and (without Firefox action) will do so again and again:

A. Could Firefox please fix the “[https]addons.mozilla.org” bug that is illustrated above?
B. And prevent the resets from continuing, at least for Startpage users? Whether directly (if STARTPAGE) or otherwise (if search-target-url =~ /HTTPS/)
C. In the next update, for users who were silently moved away from Startpage without their knowledge, who have not updated it since — could Firefox please switch them back?
D. If you are going to move people away from OpenSearch XML, please be fair and offer a way to automatically migrate users to the WebExtension of that publisher.

Thank you.

Comment 28

a year ago
@Florian, could you please reply to the above requests and questions?

Thanks.
Flags: needinfo?(florian)
(Assignee)

Comment 29

a year ago
(In reply to David Michael from comment #27)

1. OpenSearch XML files are not considered "legacy add-ons". This is just a misunderstanding.

2. WebExtension is a replacement for legacy add-ons. If a legacy add-on was installing a search plugin, and that add-on has a newer WebExtension version, it has been automatically updated to the WebExtension version.

3. I don't understand what led you to think "Firefox tries to reset every search provider in its shortlist to Google", but it's not what we are doing.

4. It's really how the search plugin got installed that matters here.

5. "Why will this repeatedly reset (often silently) until Release 59" not sure what makes you assert that. The code landed here implements a one-time reset.

6. With the launch of Firefox 57, we disabled all legacy add-ons. They were a known source of malware. This unfortunately wasn't enough to ensure a correct search experience for our users of the new Firefox, and we had to cleanup the search settings leftover by now-disabled add-ons. We used telemetry data to decide who was going to receive a reset. The overwhelming majority of users who received a silent reset were using a search plugin they had not intentionally selected. For a relatively small subset of users, we couldn't say confidently if the search engine they were using was their choice or not, so we asked these users to confirm with a one-time prompt.

This is an implementation bug, not a place to discuss future steps. The last few comments here seem to confirm that the changes are working as designed. This is my last comment in this bug. Please file new bugs if you have feature requests.
Flags: needinfo?(florian)
(Assignee)

Updated

a year ago
Depends on: 1423811

Comment 30

a year ago
(In reply to Florian Quèze [:florian] from comment #29)
> (In reply to David Michael from comment #27)
> 
> 1. OpenSearch XML files are not considered "legacy add-ons". This is just a
> misunderstanding.
> 
> 2. WebExtension is a replacement for legacy add-ons. If a legacy add-on was
> installing a search plugin, and that add-on has a newer WebExtension
> version, it has been automatically updated to the WebExtension version.
> 
> 3. I don't understand what led you to think "Firefox tries to reset every
> search provider in its shortlist to Google", but it's not what we are doing.
> 
> 4. It's really how the search plugin got installed that matters here.
> 
> 5. "Why will this repeatedly reset (often silently) until Release 59" not
> sure what makes you assert that. The code landed here implements a one-time
> reset.
> 
> 6. With the launch of Firefox 57, we disabled all legacy add-ons. They were
> a known source of malware. This unfortunately wasn't enough to ensure a
> correct search experience for our users of the new Firefox, and we had to
> cleanup the search settings leftover by now-disabled add-ons. We used
> telemetry data to decide who was going to receive a reset. The overwhelming
> majority of users who received a silent reset were using a search plugin
> they had not intentionally selected. For a relatively small subset of users,
> we couldn't say confidently if the search engine they were using was their
> choice or not, so we asked these users to confirm with a one-time prompt.
> 
> This is an implementation bug, not a place to discuss future steps. The last
> few comments here seem to confirm that the changes are working as designed.
> This is my last comment in this bug. Please file new bugs if you have
> feature requests.

2. Seems to not be true for me. I create a corresponding bug ticket here: https://bugzilla.mozilla.org/show_bug.cgi?id=1426081
(Assignee)

Updated

a year ago
Duplicate of this bug: 1022903
Duplicate of this bug: 1423759
You need to log in before you can comment on or make changes to this bug.