1203168 - ask the user for confirmation when searching with a default search engine of unknown origin

Status: RESOLVED FIXED

(Firefox :: Search, defect, P2)

Description

[Florian Quèze [:florian]]

Once bug 1203167 is fixed, we should know how user-installed engines have been installed.

If we don't know how an engine was installed (eg. because it was already installed before we fixed bug 1203167) and it's currently set as the default, we should show an interstitial page asking the user to confirm, or revert to the original default.

The exact design and wording of the interstitial page needs to be figured out by UX.

Comment 1

[:shell escalante]

need to get UX portion of how to present this to the users.  this will be what we do once the next round of preventative work is done (starting that in 44).

Comment 2

[Florian Quèze [:florian]]

Attachment: WIP v1

This is implemented based on the mockup at http://people.mozilla.org/~shorlander/search-settings-reset/search-settings-reset-01.html and already works.

Left to do:
- record the user interactions with the page in Telemetry
- tests
- polish to get this in shapre for reviews.

Comment 3

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #2)

> Left to do:

- a whitelist of known good engines that aren't shipped by default, but still shouldn't trigger the prompt. I'm planning to look at Telemetry data to see what legitimate alternative engines are popular among our users.

Comment 4

[Florian Quèze [:florian]]

Attachment: WIP v2

I think this is mostly ready for review. It contains a test for the back-end (search service) part. I still need to write a front-end test for the about:searchreset page. Whether we want to have a whitelist or not (and if we want one how we build it) needs some discussion.

Comment 5

[Marco Castelluccio [:marco]]

Comment on attachment 8718896 [details] [diff] [review]
WIP v2

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

::: browser/app/profile/firefox.js
@@ +389,5 @@
>  #else
>  pref("browser.search.redirectWindowsSearch", false);
>  #endif
>  
> +pref("browser.search.reset.enabled", true);

Couldn't a malicious addon (or external software) just set this pref to false and then change the users' search engine pref?
Or do we have other features that prevent this?

Comment 6

[Florian Quèze [:florian]]

(In reply to Marco Castelluccio [:marco] from comment #5)

> > +pref("browser.search.reset.enabled", true);
> 
> Couldn't a malicious addon (or external software) just set this pref to
> false and then change the users' search engine pref?

Yes. The intent is to not enable this prompt until add-on signing is required on the release channel. Otherwise a malicious add-on can indeed just change the default value of that pref.

> Or do we have other features that prevent this?

Note that the patch reads the value of the pref on the default pref branch, so external software writing to the prefs.js file of the user's profile can't affect this behavior.

Comment 7

[Florian Quèze [:florian]]

Attachment: WIP v3

Now with a browser-chrome test for the about:searchreset page. Unfortunately it doesn't work with e10s, waitForDocLoadAndStopIt promises never resolve; probably because about:searchreset is loaded in the chrome process and search result pages load in the content process.

Comment 8

[Florian Quèze [:florian]]

Attachment: Patch v4

Overriding gBrowser.selectedBrowser.loadURIWithFlags is enough to make the test pass with e10s, it stops the load before the process type switch happens.

Comment 9

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6633b64c95a4

Comment 10

[Florian Quèze [:florian]]

Attachment: Patch v5

Unbitrotted and updated to match the latest mockup at http://people.mozilla.org/~shorlander/search-settings-restore/search-settings-restore-01.html (ie. use final strings, and add the dropdown to pick an engine to set as the new default).

Comment 11

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=668676517c1c

Comment 12

[Florian Quèze [:florian]]

Attachment: ask the user for confirmation when searching with a default search engine of unknown origin, r?

Requesting review from Drew for the code, and from Benjamin for the added telemetry probe. Never expiring this doesn't feel exactly right, but we'll likely want to know how many times release users have seen this prompt and how they reacted to it for several releases; can you advise on which version number to use for expiring the probe?

Comment 13

[Florian Quèze [:florian]]

Attachment: Patch v7

Fixed the addEngineWithDetails special case that was causing the test failures, and added an xpcshell test to cover this explicitly.

Comment 14

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=45dc878f4d8e

Comment 15

[Benjamin Smedberg]

Comment on attachment 8755378 [details] [diff] [review]
Patch v7

Assuming this is landing for 49, I typically suggest a 6-month/4-release cycle, so this would expire in 53.

Comment 16

[Florian Quèze [:florian]]

Attachment: ask the user for confirmation when searching with a default search engine of unknown origin, r?

Fixed the new xpcshell test failure (comment 14) and set the data collection to expire in 53, per comment 15.

Comment 17

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c46fc11397bc

Comment 18

[Drew Willcoxon :adw]

Comment on attachment 8755838 [details] [diff] [review]
ask the user for confirmation when searching with a default search engine of unknown origin, r?

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

Looks good to me, works as advertised.

Comment 19

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/08053d1e2cc1f3b6e6901e4aedaf122c4e1de27b
Bug 1203168 - ask the user for confirmation when searching with a default search engine of unknown origin, data-review=bsmedberg, r=adw.

Comment 20

[Carsten Book [:Tomcat]]

sorry had to back this out for lint failures like:
 TEST-UNEXPECTED-ERROR | browser/components/search/test/browser_aboutSearchReset.js:167:13 | Parsing error: Unexpected token promiseTabLoadEvent (null) 

https://treeherder.mozilla.org/logviewer.html#?job_id=9591568&repo=fx-team

Comment 21

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/df3281895c93

Comment 22

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/dd3e899cfa818fcbb47b3d7432e0977e835702ee
Bug 1203168 - ask the user for confirmation when searching with a default search engine of unknown origin, data-review=bsmedberg, r=adw.

Comment 23

[Florian Quèze [:florian]]

Comment on attachment 8755838 [details] [diff] [review]
ask the user for confirmation when searching with a default search engine of unknown origin, r?

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

::: browser/components/search/test/browser_aboutSearchReset.js
@@ +149,5 @@
> +
> +function test()
> +{
> +  waitForExplicitFinish();
> +  Task.spawn(function () {

The lint warning was because this should have been "function*" instead of "function".

Comment 24

[Florian Quèze [:florian]]

I pushed the patch with the feature pref'ed on. This should be in Firefox 49. We landed the Telemetry probe to count how often the prompt may be shown in Firefox 48 (bug 1259139).

When 48 will be half way through beta, I should check the results of the probe from bug 1259139 on the aurora and beta channel. At that point, if the numbers don't look reasonable, we'll still have time to tweak the behavior associated to the prompt and uplift changes to 49/Aurora.

When 49 will be half way through beta, I should check Telemetry again to verify the results of the probe added by the patch here, and compare the values of the probe from bug 1259139 before/after the prompt landed. At that point, it will be too late to fix things, but we'll still have enough time to pref off the feature if things don't look right.

Comment 25

[Francesco Lodolo [:flod]]

I would really add a note to searchreset.selector.label to explain that the string is followed by a dropdown list with the available search engines.

Having said that, is there a way to force this dialog to appear for testing?

Comment 26

[magicp]

Could you please add the following rules in searchReset.css for RTL? Thank you in advance.

+ select:-moz-dir(rtl) {
+ 	background-position: calc(100% - 8px) center, 4px center;
+ }

+ option:-moz-dir(rtl) {
+   background-position: calc(100% - 8px) center;
+ }

Comment 27

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #25)
> I would really add a note to searchreset.selector.label to explain that the
> string is followed by a dropdown list with the available search engines.
> 
> Having said that, is there a way to force this dialog to appear for testing?

And I completely forgot one more question: is the hard-coded Firefox, instead of &brandShortName; wanted in searchreset.pageInfo1?

Comment 28

[Florian Quèze [:florian]]

Attachment: i18n follow-up

Follow-up patch to address comments 25 to 27.

Comment 29

[Florian Quèze [:florian]]

(In reply to Francesco Lodolo [:flod] from comment #25)

> Having said that, is there a way to force this dialog to appear for testing?

Loading about:searchreset using the location bar works.


(In reply to magicp from comment #26)
> Could you please add the following rules in searchReset.css for RTL?

Sure, thanks for providing the fix!


(In reply to Francesco Lodolo [:flod] from comment #27)

> And I completely forgot one more question: is the hard-coded Firefox,
> instead of &brandShortName; wanted in searchreset.pageInfo1?

It was a copy/paste mistake; I copied the final strings from the UX mockup.

Comment 30

[Francesco Lodolo [:flod]]

Comment on attachment 8757848 [details] [diff] [review]
i18n follow-up

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

Technically we should change the ID, but only 3 locales translated that string. If we manage to land this quickly, we should be good.
https://transvision.mozfr.org/string/?entity=browser/chrome/browser/aboutSearchReset.dtd:searchreset.pageInfo1&repo=central

I'll keep an eye on the string, also CCed both localizers.

Comment 31

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/00dccad34edae14b16de663c884a36ef2ec575b4
Bug 1203168 - follow-up to improve internationalization of the about:searchreset page, rs=flod.

Comment 32

[RickieES aka Ricardo Palomares]

That's OK for me. I'll change the translation this afternoon for es-ES; I would have caught it up anyway as my L10n tool chain detects changes in original en-US strings even if entity name is kept the same.

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/00dccad34eda

Comment 34

[Carsten Book [:Tomcat]]

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

Comment 35

[Stefan Plewako [:stef]]

(In reply to Florian Quèze [:florian] [:flo] from comment #31)
> https://hg.mozilla.org/integration/fx-team/rev/
> 00dccad34edae14b16de663c884a36ef2ec575b4
> Bug 1203168 - follow-up to improve internationalization of the
> about:searchreset page, rs=flod.

Actually this makes the searchreset.pageInfo1 string less localizable sadly

Comment 36

[Stefan Plewako [:stef]]

Also, restore vs change wording makes the whole thing suspicious