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

Status: VERIFIED FIXED

(Firefox :: Search, enhancement, P1)

Description

[Florian Quèze [:florian]]

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.

Comment 1

[Florian Quèze [:florian]]

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.

Comment 2

[Florian Quèze [:florian]]

Attachment: Patch

Comment 3

[Drew Willcoxon :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.

Comment 4

[Florian Quèze [:florian]]

(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

[Pulsebot]

Pushed by [e-mail]:
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

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/707917eb9b1b

Comment 7

[Florian Quèze [:florian]]

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.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8931166 [details] [diff] [review]
Patch

Need that in 58

Comment 9

[Sylvestre Ledru [:Sylvestre]]

https://hg.mozilla.org/releases/mozilla-beta/rev/88def880f9af3df5a6c6cc04a89836a1c9bc798d

Comment 10

[Florian Quèze [:florian]]

Attachment: fix typo in setCharPref

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.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

landed in beta as follow up: 
https://hg.mozilla.org/releases/mozilla-beta/rev/a38e0adbe442

Comment 12

[Mike Kaply [:mkaply]]

Bug to fix to resetToOriginalDefaultEngine is bug 1415727

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e5ac52bbfb0
fix typo in setCharPref, r=Sylvestre.

Comment 14

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/5e5ac52bbfb0

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8931415 [details] [diff] [review]
fix typo in setCharPref

[Triage Comment]
To keep a clean db

Comment 16

[Sylvestre Ledru [:Sylvestre]]

https://hg.mozilla.org/releases/mozilla-release/rev/955145bc82d42ebb2d50f24ebc09be6744ba39d1
https://hg.mozilla.org/releases/mozilla-release/rev/4da8a5e4a6619045790dc08592426958db770c29

Comment 17

[Adrian Florinescu [:aflorinescu]]

Verified - on Beta 58.6 / Nightly59 
(https://docs.google.com/document/d/1H6Wht1fkr28HGyjUGSCW46yvnNzZOsAV1w5oePlK9Gs)

Comment 18

[Adrian Florinescu [:aflorinescu]]

Verified as well on RC 57.0.1/20171128222554 / Update Channel -release-localtest on Ubuntu 16.14/ Windows10 x64.

Comment 19

[sausagefactory0]

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.

Comment 20

[Florian Quèze [:florian]]

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

Comment 21

[Dan Callahan [:callahad]]

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

Comment 22

[Florian Quèze [:florian]]

(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

[Bruce]

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

Comment 24

[sausagefactory0]

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

Comment 25

[Florian Quèze [:florian]]

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

Comment 26

[sausagefactory0]

Yeah that's what happened. And it shouldn't have happened.

Comment 27

[David Michael]

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

[David Michael]

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

Thanks.

Comment 29

[Florian Quèze [:florian]]

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

Comment 30

[dominik.henter]

(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