Closed Bug 1203168 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Search, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox43 --- affected
firefox49 --- fixed

People

(Reporter: florian, Assigned: florian)

References

Details

(Whiteboard: [hijacking][fxsearch][UX])

Attachments

(2 files, 7 obsolete files)

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.
Depends on: 1203167
Depends on: 1203180
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).
Rank: 25
Priority: -- → P2
Whiteboard: [hijacking][fxsearch] → [hijacking][fxsearch][UX]
Attached patch WIP v1 (obsolete) — Splinter Review
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.
Assignee: nobody → florian
Status: NEW → ASSIGNED
(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.
Attached patch WIP v2 (obsolete) — Splinter Review
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.
Attachment #8716333 - Attachment is obsolete: true
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?
(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.
Attached patch WIP v3 (obsolete) — Splinter Review
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.
Attachment #8718896 - Attachment is obsolete: true
Attached patch Patch v4 (obsolete) — Splinter Review
Overriding gBrowser.selectedBrowser.loadURIWithFlags is enough to make the test pass with e10s, it stops the load before the process type switch happens.
Attachment #8731758 - Attachment is obsolete: true
Depends on: 1259139
Attached patch Patch v5 (obsolete) — Splinter Review
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).
Attachment #8731776 - Attachment is obsolete: true
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?
Attachment #8754976 - Flags: review?(adw)
Attachment #8754976 - Flags: feedback?(benjamin)
Attachment #8754442 - Attachment is obsolete: true
Attached patch Patch v7 (obsolete) — Splinter Review
Fixed the addEngineWithDetails special case that was causing the test failures, and added an xpcshell test to cover this explicitly.
Attachment #8755378 - Flags: review?(adw)
Attachment #8755378 - Flags: feedback?(benjamin)
Attachment #8754976 - Attachment is obsolete: true
Attachment #8754976 - Flags: review?(adw)
Attachment #8754976 - Flags: feedback?(benjamin)
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.
Attachment #8755378 - Flags: feedback?(benjamin) → feedback+
Fixed the new xpcshell test failure (comment 14) and set the data collection to expire in 53, per comment 15.
Attachment #8755838 - Flags: review?(adw)
Attachment #8755378 - Attachment is obsolete: true
Attachment #8755378 - Flags: review?(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.
Attachment #8755838 - Flags: review?(adw) → review+
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.
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
Flags: needinfo?(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 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".
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.
Flags: needinfo?(florian)
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?
Flags: needinfo?(florian)
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;
+ }
(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?
Attached patch i18n follow-upSplinter Review
Follow-up patch to address comments 25 to 27.
Attachment #8757848 - Flags: feedback?(francesco.lodolo)
(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.
Flags: needinfo?(florian)
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.
Attachment #8757848 - Flags: feedback?(francesco.lodolo) → feedback+
https://hg.mozilla.org/integration/fx-team/rev/00dccad34edae14b16de663c884a36ef2ec575b4
Bug 1203168 - follow-up to improve internationalization of the about:searchreset page, rs=flod.
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.
https://hg.mozilla.org/mozilla-central/rev/00dccad34eda
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
(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
Also, restore vs change wording makes the whole thing suspicious
You need to log in before you can comment on or make changes to this bug.