Closed Bug 1469889 Opened Last year Closed Last year

Provide the ability to block search engines based on their URL or loadPath

Categories

(Firefox :: Search, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox62 --- fixed
firefox63 --- fixed

People

(Reporter: mkaply, Assigned: mkaply)

References

Details

Attachments

(2 files)

We have observed various abuse of search engines by manipulating our search database.

I'm going to provide a framework allowing us to block engines based on the contents of their search URL and possibly their loadPath.
Priority: -- → P1
Status: NEW → ASSIGNED
Comment on attachment 8989203 [details]
Bug 1469889 - Allow blacklisting search engines based on URL.

https://reviewboard.mozilla.org/r/254254/#review261064


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/search/nsSearchService.js:3121
(Diff revision 1)
> +    if (this._blackList.some(code => url.includes(code))) {
> +      LOG("_addEngineToStore: Ignoring blacklisted engine");
> +      Services.telemetry.getHistogramById("SEARCH_SERVICE_COUNTRY_FETCH_CAUSED_SYNC_INIT").add(gInitialized);
> +      this.telemetry.recordEvent("devtools.main", "jump_to_source", "webconsole",
> ++                                       null, { "session_id": this.toolbox.sessionId }
> +      return;

Error: Parsing error: unexpected token return [eslint]
Comment on attachment 8989203 [details]
Bug 1469889 - Allow blacklisting search engines based on URL.

https://reviewboard.mozilla.org/r/254254/#review261266

Looks reasonable, so r+ but we may need to polish this a bit more. The following 2 comments could be addressed in follow-ups.

What happens if all the engines we find in the cache file are blacklisted? Will this result in a situation with 0 available engine, and a fully broken search experience?

::: toolkit/components/search/tests/xpcshell/test_blacklist.js:18
(Diff revision 2)
> +
> +  Services.search.addEngineWithDetails(kSearchEngineID, "", "", "", "get",
> +                                       kSearchEngineURL);
> +
> +  // A blacklisted engine shouldn't be available at all
> +  let engine = Services.search.getEngineByName(kSearchEngineID);

Do you have thoughts about how we should handle attempting to add a blacklisted engine? Especially if the engine is added through open search, it may be surprising for users if we just silently fail to add it without providing any feedback.

For that case specifically, maybe we should filter out the engine before showing the engine through open search discovery.
Attachment #8989203 - Flags: review?(florian) → review+
> What happens if all the engines we find in the cache file are blacklisted? Will this result in a situation with 0 available engine, and a fully broken search experience?

I think if that happens we have much bigger problems :). But I'll see what I can figure out.

> Do you have thoughts about how we should handle attempting to add a blacklisted engine? Especially if the engine is added through open search, it may be surprising for users if we just silently fail to add it without providing any feedback.

Unfortunately we don't download the engine XML before discovery happens so we don't have enough information to do that.
Comment on attachment 8989203 [details]
Bug 1469889 - Allow blacklisting search engines based on URL.

https://reviewboard.mozilla.org/r/254254/#review261306

While discussing this over IRC we found a simple way to handle the "all engines from the cache are blocklisted" case, so let's do it here rather than in a follow-up.
Attachment #8989203 - Flags: review+ → review-
Comment on attachment 8989203 [details]
Bug 1469889 - Allow blacklisting search engines based on URL.

https://reviewboard.mozilla.org/r/254254/#review261344

::: toolkit/components/search/nsSearchService.js:2878
(Diff revisions 2 - 3)
>                         this._visibleDefaultEngines.some(notInCacheVisibleEngines);
>  
> -    if (rebuildCache) {
> +    if (!rebuildCache) {
> +      LOG("_loadEngines: loading from cache directories");
> +      this._loadEnginesFromCache(cache);
> +      if (Object.keys(this._engines).length !== 0) {

nit: you don't need the "!== 0" here.

::: toolkit/components/search/nsSearchService.js:2882
(Diff revisions 2 - 3)
> +      this._loadEnginesFromCache(cache);
> +      if (Object.keys(this._engines).length !== 0) {
> +        LOG("_loadEngines: done using existing cache");
> +        return;
> +      }
> +      LOG("_loadEngines: Invalid engine count. Loading engines from disk.");

It's more the cached engines than the count that's invalid. Maybe "No valid engines found in the cache"?

::: toolkit/components/search/nsSearchService.js
(Diff revisions 2 - 3)
> -    }
> -
> -    LOG("_asyncLoadEngines: loading from cache directories");
> -    this._loadEnginesFromCache(cache);
> -
> -    LOG("_asyncLoadEngines: done");

Should we keep this last logging line? Or "done using rebuilt cache" like you wrote for the sync case?

::: toolkit/components/search/tests/xpcshell/xpcshell.ini:28
(Diff revisions 2 - 3)
>    data/ico-size-16x16-png.ico
>    data/invalid-engine.xml
>    data/install.rdf
>    data/list.json
>    data/search.json
> +  data/search_blacklist.json

This should be marked as a support file for the new test specifically, not for the whole folder. See https://searchfox.org/mozilla-central/source/devtools/server/tests/unit/xpcshell.ini#223 for an example.

::: toolkit/components/search/tests/xpcshell/test_json_cache_blacklist.js:36
(Diff revision 3)
> +
> +  run_next_test();
> +}
> +
> +add_test(function prepare_test_data() {
> +  OS.File.writeAtomic(OS.Path.join(OS.Constants.Path.profileDir, CACHE_FILENAME),

promiseSaveCacheData(cacheTemplate)

::: toolkit/components/search/tests/xpcshell/test_json_cache_blacklist.js:56
(Diff revision 3)
> +
> +    let engines = Services.search.getEngines({});
> +
> +    // Engine list will have been reset to the default,
> +    // Not the one engine in the cache.
> +    Assert.notEqual(engines.length);

I think you wanted to check that length is not '1' and forgot that parameter.

It would also be good to ensure our fallback code worked, so ensuring the list length isn't 0. So maybe just check for > 1.
Attachment #8989203 - Flags: review?(florian) → review-
Comment on attachment 8989203 [details]
Bug 1469889 - Allow blacklisting search engines based on URL.

https://reviewboard.mozilla.org/r/254254/#review261368

Looks good to me.

::: toolkit/components/search/tests/xpcshell/test_json_cache_blacklist.js:29
(Diff revision 4)
> +
> +  // Copy the test engine to the test profile.
> +  let engineTemplateFile = do_get_file("data/engine.xml");
> +  engineTemplateFile.copyTo(engineFile.parent, "test-search-engine.xml");
> +
> +  // The list of visibleDefaultEngines needs to match or the cache will be ignored.

This test seems fragile as if we start ignoring the cache file for any other reason, the test will still pass. Unfortunately I don't have any idea to address this right now.
Attachment #8989203 - Flags: review?(florian) → review+
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/2b3eeaf706ae
Allow blacklisting search engines based on URL. r=florian
https://hg.mozilla.org/mozilla-central/rev/2b3eeaf706ae
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Depends on: 1474667
Comment on attachment 8989203 [details]
Bug 1469889 - Allow blacklisting search engines based on URL.

Approval Request Comment
[Feature/Bug causing the regression]: Add potential for search mitigation
[User impact if declined]: Inability to block hijacked engines
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[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?]: Low risk
[Why is the change risky/not risky?]: Automated test. Doesn't affect regular path.
[String changes made/needed]:
Attachment #8989203 - Flags: approval-mozilla-beta?
Comment on attachment 8989203 [details]
Bug 1469889 - Allow blacklisting search engines based on URL.

This should help us blocklist more easily, let's uplift for beta 8.
Attachment #8989203 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Backed out for ESLint failures in test_json_cache_blacklist.js that I strongly suspect will cause the test to fail as well. Looks like this depends on bug 1463162.
https://hg.mozilla.org/releases/mozilla-beta/rev/36d63961e4f89660ec4268de764873fce274ab91

https://treeherder.mozilla.org/logviewer.html#?job_id=187711520&repo=mozilla-beta
Flags: needinfo?(mozilla)
Well that was dumb. I'll put together a new patch.
Flags: needinfo?(mozilla)
Blocks: 1475989
Comment on attachment 8989203 [details]
Bug 1469889 - Allow blacklisting search engines based on URL.

https://reviewboard.mozilla.org/r/254254/#review264028
Attachment #8989203 - Flags: review+
Attached patch Beta patchSplinter Review
Sorry about that. Here is a beta patch. I do not want to take that other patch via uplift, so this just corrects the test so it is the same as test_json_cache

I've verified the tests pass and eslint works.

This is attached as an hg export from beta.
Flags: needinfo?(mozilla)
You need to log in before you can comment on or make changes to this bug.