Closed Bug 1336560 Opened 7 years ago Closed 7 years ago

Provide the ability for a distribution to hide search engines

Categories

(Firefox :: Search, defect)

Product:
Firefox
Component:
Search
Version:
51 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: mkaply, Assigned: mkaply)

References

Details

Attachments

(3 files)

Simple patch
1.25 KB, patch
florian
: feedback+
Details | Diff | Splinter Review
Bug 1336560 - Allow distributions to hide search engines.
59 bytes, text/x-review-board-request
florian
: review+
gchang
: approval-mozilla-aurora+
lizzard
: approval-mozilla-beta+
Details
Remove Telemtry line
1.36 KB, patch
Details | Diff | Splinter Review 
Distributions need the ability to completely hide a search engine if necessary.

It should not be able to be restored with "restore defaults"
Attached patch Simple patch — — Splinter Review 
I realize this could theoretically be hijacked, but it's simple and straightforward.

I thought about whether or not we could do something in distribution.js and get the data directly to the search service somehow, but anything I think of still involves a pref.
Attachment #8833458 - Flags: feedback?(florian)
Comment on attachment 8833458 [details] [diff] [review]
Simple patch

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

In terms of hijacking, as long as we use the default branch (which the current patch does), we are not weaker than other spots, so I think it's fine. So, f+ (but will need a test to get an r+).

::: toolkit/components/search/nsSearchService.js
@@ +3681,5 @@
>      }
>  
> +    // Remove any engine names that are supposed to be ignored, primarily
> +    // by distributions
> +    try {

I'm not really excited by this throwing an exception (that we'll catch) during the startup path for the general case. But I don't really want to have this pref listed in about:config either. I guess we'll have to live with this.

@@ +3686,5 @@
> +      let ignoredJAREngines = Services.prefs.getDefaultBranch(null)
> +                                      .getCharPref(BROWSER_SEARCH_PREF + "ignoredJAREngines")
> +                                      .split(",");
> +      engineNames = engineNames.filter(function(engine) {
> +                                         return !ignoredJAREngines.includes(engine);

This can be simplified
  e => !ignoredJAREngines.includes(e)

So I think the one case where we'll get in trouble is if all engines get hidden. Can we skip the filter call if ignoredJAREngines.length >= engineNames.length?

@@ +3687,5 @@
> +                                      .getCharPref(BROWSER_SEARCH_PREF + "ignoredJAREngines")
> +                                      .split(",");
> +      engineNames = engineNames.filter(function(engine) {
> +                                         return !ignoredJAREngines.includes(engine);
> +                                       });      

nit: trailing whitespace
Attachment #8833458 - Flags: feedback?(florian) → feedback+
Comment on attachment 8833458 [details] [diff] [review]
Simple patch

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

::: toolkit/components/search/nsSearchService.js
@@ +3681,5 @@
>      }
>  
> +    // Remove any engine names that are supposed to be ignored, primarily
> +    // by distributions
> +    try {

Or maybe:
  let branch = Services.prefs.getDefaultBranch(BROWSER_SEARCH_PREF);
  if (branch.getPrefType("ignoredJAREngines") == branch.PREF_STRING) {
    branch.getCharPref(...
> So I think the one case where we'll get in trouble is if all engines get hidden. Can we skip the filter call if ignoredJAREngines.length >= engineNames.length?

I don't think that will work because we could put 10 engines in ignoredJAREngines intending for them to be hidden on different language versions of Firefox. So in that case, ignoredJAREngines.length would always be >= engineNames.lengt;

Unfortunately we don't have the default engine at this point, so we can't make a decision as to which engine we should keep.
(In reply to Mike Kaply [:mkaply] from comment #4)

> I don't think that will work because we could put 10 engines in
> ignoredJAREngines intending for them to be hidden on different language
> versions of Firefox.

So you can filter ignoredJAREngines to only keep engines that are part of engineNames before comparing the length, or maybe it's easier to just put the result of engineNames.filter(...) in a different variable, and ignore that result if it's an empty array.
>  maybe it's easier to just put the result of engineNames.filter(...) in a different variable, and ignore that result if it's an empty array.

That's what I was thinking. I'll do that and create the test.
[Tracking Requested - why for this release]: We have some contractual reasons we need to hide engines. Would be nice to have this if we can make it happen.
tracking-firefox52: --- → ?
(In reply to Mike Kaply [:mkaply] from comment #7)
> [Tracking Requested - why for this release]: We have some contractual
> reasons we need to hide engines. Would be nice to have this if we can make
> it happen.

I don't think it's reasonable to want to uplift this to beta, especially after letting this bug sleep for more than 10 days.

(In reply to Florian Quèze [:florian] [:flo] (PTO until February 27) from comment #2)
>
> In terms of hijacking, as long as we use the default branch (which the
> current patch does), we are not weaker than other spots, so I think it's
> fine. So, f+ (but will need a test to get an r+).

When I wrote this, I was thinking about hijackers trying to hide all of the built-in plugins so that the search plugin they inject becomes automatically the default.

There's another case I should have considered: hijackers hiding the engine they are specifically interested in so that they can install a plugin with the same name but different affiliation codes. I think we should attempt to mitigate the risk of this happening. Possible things to do:
- take hidden engines into account when doing the duplicate check before installing another plugin
- report the value of this pref through telemetry (so that we can notice if it becomes abused)
- restrict the impact of this pref to installations where there's a distribution id.
(In reply to Florian Quèze [:florian] [:flo] (PTO until February 27) from comment #9)
> I don't think it's reasonable to want to uplift this to beta, especially
> after letting this bug sleep for more than 10 days.

Yeah, you're right. Removed flag.

> 
> (In reply to Florian Quèze [:florian] [:flo] (PTO until February 27) from
> comment #2) Possible things to do:
> - take hidden engines into account when doing the duplicate check before
> installing another plugin

What installation method were you thinking? Plugin or distribution?

> - report the value of this pref through telemetry (so that we can notice if
> it becomes abused)

I honestly don't have a good understanding of how add things to telemetry. Do you know a good place to start?

> - restrict the impact of this pref to installations where there's a
> distribution id.

This to me seems the most logical thing to do. We already have an easy way to check for this.
tracking-firefox52: ? → ---
(In reply to Mike Kaply [:mkaply] from comment #10)

> > (In reply to Florian Quèze [:florian] [:flo] (PTO until February 27) from
> > comment #2) Possible things to do:
> > - take hidden engines into account when doing the duplicate check before
> > installing another plugin
> 
> What installation method were you thinking? Plugin or distribution?

Not distribution; every other method. I mostly had in mind discovery through open search, but the one where hijacking will matter the most is .xml files dropped in a searchplugins folder of the user's profile.

> > - report the value of this pref through telemetry (so that we can notice if
> > it becomes abused)
> 
> I honestly don't have a good understanding of how add things to telemetry.
> Do you know a good place to start?

https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe should be a good general introduction.

For related code in the search service, have a look at the _recordEngineTelemetry and getDefaultEngineInfo functions.
> Not distribution; every other method. I mostly had in mind discovery through open search, but the one where hijacking will matter the most is .xml files dropped in a searchplugins folder of the user's profile.

I thought that wasn't possible anymore with the new search lz4 file.
(In reply to Mike Kaply [:mkaply] from comment #12)
> > Not distribution; every other method. I mostly had in mind discovery through open search, but the one where hijacking will matter the most is .xml files dropped in a searchplugins folder of the user's profile.
> 
> I thought that wasn't possible anymore with the new search lz4 file.

We do a one-time import when creating the lz4 file. Hijackers can just delete the lz4 file to force an import at the next startup.

At some point in the future we'll remove the import code, but I don't think we have decided when that'll happen. It may need to wait for another esr cycle.
Depends on: 1343943
Comment on attachment 8839646 [details]
Bug 1336560 - Allow distributions to hide search engines.

https://reviewboard.mozilla.org/r/114216/#review121024

I had unpublished review comments here, looks like I forgot to hit the "publish" button on Friday, sorry about that :-(.

r- to get the "review requested" email again when you will update the patch so that I can see what you decide for async and/or sync tests, but otherwise I think this looks OK.

::: toolkit/components/search/tests/xpcshell/test_sync_disthidden.js:19
(Diff revision 2)
> +  // engines or the resource URL won't be used
> +  let url = "resource://test/data/";
> +  let resProt = Services.io.getProtocolHandler("resource")
> +                        .QueryInterface(Ci.nsIResProtocolHandler);
> +  resProt.setSubstitution("search-plugins",
> +                          Services.io.newURI(url));

nit: this fits on a single 80 chars line.

::: toolkit/components/search/tests/xpcshell/test_sync_disthidden.js:24
(Diff revision 2)
> +                          Services.io.newURI(url));
> +
> +  run_next_test();
> +}
> +
> +add_task(function* test_defaultresourceicon() {

This name doesn't seem to match what the test does.

::: toolkit/components/search/tests/xpcshell/test_sync_disthidden.js:25
(Diff revision 2)
> +
> +  run_next_test();
> +}
> +
> +add_task(function* test_defaultresourceicon() {
> +  yield asyncInit();

asyncInit in a test named sync seems strange.
Do you need to test both the sync and async init code paths?
Attachment #8839646 - Flags: review?(florian) → review-
Comment on attachment 8839646 [details]
Bug 1336560 - Allow distributions to hide search engines.

https://reviewboard.mozilla.org/r/114216/#review123070
Attachment #8839646 - Flags: review?(florian) → review+
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/01b927dcb4af
Allow distributions to hide search engines. r=florian
Backed out for failing eslint in test_sync_disthidden.js (semicolon too many):

https://hg.mozilla.org/integration/autoland/rev/c336cb189edb340db94c3e93cad9f727d9986139

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=01b927dcb4af261acaccc6e591198a5c8b11cf0c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=84389346&repo=autoland

>TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/components/search/tests/xpcshell/test_sync_disthidden.js:32:2 | Unnecessary semicolon. (no-extra-semi)
The other Eslint failure is from the parent changeset.
Flags: needinfo?(mozilla)
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/5d3d03119fa1
Allow distributions to hide search engines. r=florian
https://hg.mozilla.org/mozilla-central/rev/5d3d03119fa1
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Comment on attachment 8839646 [details]
Bug 1336560 - Allow distributions to hide search engines.

Approval Request Comment
[Feature/Bug causing the regression]: Allow distributions to hide engines
[User impact if declined]: No user impact, required by some partners
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only affects partner builds with the pref set.
[String changes made/needed]:

I know this is an odd one to request for 53. I'll be happy to provide more information outside of the bug as to why we need this.
Flags: needinfo?(mozilla)
Attachment #8839646 - Flags: approval-mozilla-beta?
Attachment #8839646 - Flags: approval-mozilla-aurora?
Comment on attachment 8839646 [details]
Bug 1336560 - Allow distributions to hide search engines.

This is for partner purpose and also include tests. Aurora54+.

Hi :mkaply,
Can you give us more information about why we need this in Beta53?
Flags: needinfo?(mozilla)
Attachment #8839646 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
The telemetry part of this patch won't work in Aurora. 

Not sure if you want to have me back out and submit a new patch or just remove the Telemetry.jsm line.

Sorry about that.
Flags: needinfo?(mozilla)
Bustage fix.

The Telemetry change won't work in Aurora (or Beta)
> Can you give us more information about why we need this in Beta53?

Due to various contractual requirements, we need the ability to remove specific search engines from custom distributions.
(In reply to Mike Kaply [:mkaply] from comment #29)
> > Can you give us more information about why we need this in Beta53?
> 
> Due to various contractual requirements, we need the ability to remove
> specific search engines from custom distributions.

This doesn't say why this is urgent. We decided not to uplift this to 52 because it was scary to have this go straight to late beta, and then you left the patch untouched for about 2 weeks between comment 10 and comment 14. That doesn't seem like something handled as an emergency.
(In reply to Mike Kaply [:mkaply] from comment #24)

> [Why is the change risky/not risky?]: Only affects partner builds with the
> pref set.

Have we considered the impact on the profile of users switching back and forth between a partner build and a non-partner build?
The reason the bug went untouched is because I had to implement a telemetry patch to accommodate your request. I also have other job responsibilities besides this bug.

As far as switching between partner and non partner, that is quite an edge case and not in scope here.

If release management would like to discuss why this is important, I'm available. Suffice to say for contractual reasons we need to get this in.

We still have four weeks before release.
Comment on attachment 8839646 [details]
Bug 1336560 - Allow distributions to hide search engines.

I'm ok with giving this a shot for 53. It should land for the beta 6 build this Thursday.
Attachment #8839646 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: