Bug 1532170 - Move search ignorelist data to RemoteSettings. r=mikedeboer,glasserc

Differential Revision: https://phabricator.services.mozilla.com/D25382

--HG--
extra : moz-landing-system : lando

services/settings/dumps/main/hijack-blocklists.json

@@ -0,0 +1 @@
+{"data":[{"schema":1554459974103,"matches":["[https]opensearch.startpageweb.com/bing-search.xml","[https]opensearch.startwebsearch.com/bing-search.xml","[https]opensearch.webstartsearch.com/bing-search.xml","[https]opensearch.webofsearch.com/bing-search.xml","[profile]/searchplugins/Yahoo! Powered.xml","[profile]/searchplugins/yahoo! powered.xml"],"id":"load-paths","last_modified":1554460008541},{"schema":1554320493893,"matches":["hspart=lvs","pc=COSP","clid=2308146","fr=mca","PC=MC0","lavasoft.gosearchresults","securedsearch.lavasoft"],"id":"submission-urls","last_modified":1554459974090}]}

services/settings/dumps/main/moz.build

@@ -4,6 +4,7 @@
 
 FINAL_TARGET_FILES.defaults.settings.main += [
     'example.json',
+    'hijack-blocklists.json',
     'language-dictionaries.json',
     'onboarding.json',
     'sites-classification.json',

toolkit/components/search/SearchService.jsm

@@ -18,6 +18,7 @@ XPCOMUtils.defineLazyModuleGetters(this, {
   setTimeout: "resource://gre/modules/Timer.jsm",
   clearTimeout: "resource://gre/modules/Timer.jsm",
   ExtensionParent: "resource://gre/modules/ExtensionParent.jsm",
+  RemoteSettings: "resource://services-settings/remote-settings.js",
 });
 
 XPCOMUtils.defineLazyServiceGetters(this, {
@@ -189,6 +190,12 @@ const DEFAULT_TAG = "default";
  */
 const SEARCH_LOG_PREFIX = "*** Search: ";
 
+/**
+ * This is the Remote Settings key that we use to get the ignore lists for
+ * engines.
+ */
+const SETTINGS_IGNORELIST_KEY = "hijack-blocklists";
+
 /**
  * Outputs text to the JavaScript console as well as to stdout.
  */
@@ -2638,7 +2645,12 @@ ParseSubmissionResult.prototype = {
 const gEmptyParseSubmissionResult =
       Object.freeze(new ParseSubmissionResult(null, "", -1, 0));
 
-// nsISearchService
+/**
+ * The search service handles loading and maintaining of search engines. It will
+ * also work out the default lists for each locale/region.
+ *
+ * @implements {nsISearchService}
+ */
 function SearchService() {
   this._initObservers = PromiseUtils.defer();
 }
@@ -2662,6 +2674,18 @@ SearchService.prototype = {
   // This is set back to null as soon as the initialization is finished.
   _cacheFileJSON: null,
 
+  /**
+   * Various search engines may be ignored if their submission urls contain a
+   * string that is in the list. The list is controlled via remote settings.
+   */
+  _submissionURLIgnoreList: [],
+
+  /**
+   * Various search engines may be ignored if their load path is contained
+   * in this list. The list is controlled via remote settings.
+   */
+  _loadPathIgnoreList: [],
+
   // If initialization has not been completed yet, perform synchronous
   // initialization.
   // Throws in case of initialization error.
@@ -2708,6 +2732,8 @@ SearchService.prototype = {
       await this._ensureKnownRegionPromise;
     }
 
+    this._setupRemoteSettings().catch(Cu.reportError);
+
     try {
       await this._loadEngines(cache);
     } catch (ex) {
@@ -2729,6 +2755,90 @@ SearchService.prototype = {
     return this._initRV;
   },
 
+  /**
+   * Obtains the remote settings for the search service. This should only be
+   * called from init(). Any subsequent updates to the remote settings are
+   * handled via a sync listener.
+   *
+   * For desktop, the initial remote settings are obtained from dumps in
+   * `services/settings/dumps/main/`. These are not shipped with Android, and
+   * hence the `get` may take a while to return.
+   */
+  async _setupRemoteSettings() {
+    const ignoreListSettings = RemoteSettings(SETTINGS_IGNORELIST_KEY);
+    // Trigger a get of the initial value.
+    const current = await ignoreListSettings.get();
+
+    // Now we have the values, listen for future updates.
+    this._ignoreListListener = this._handleIgnoreListUpdated.bind(this);
+    ignoreListSettings.on("sync", this._ignoreListListener);
+
+    await this._handleIgnoreListUpdated({data: {current}});
+    Services.obs.notifyObservers(null, SEARCH_SERVICE_TOPIC, "settings-update-complete");
+  },
+
+  /**
+   * This handles updating of the ignore list settings, and removing any ignored
+   * engines.
+   *
+   * @param {object} eventData
+   *   The event in the format received from RemoteSettings.
+   */
+  async _handleIgnoreListUpdated(eventData) {
+    LOG("_handleIgnoreListUpdated");
+    const {data: {current}} = eventData;
+
+    for (const entry of current) {
+      if (entry.id == "load-paths") {
+        this._loadPathIgnoreList = [...entry.matches];
+      } else if (entry.id == "submission-urls") {
+        this._submissionURLIgnoreList = [...entry.matches];
+      }
+    }
+
+    // If we have not finished initializing, then we wait for the initialization
+    // to complete.
+    if (!this.isInitialized) {
+      await this._initObservers;
+    }
+    // We try to remove engines manually, as this should be more efficient and
+    // we don't really want to cause a re-init as this upsets unit tests.
+    let engineRemoved = false;
+    for (let name in this._engines) {
+      let engine = this._engines[name];
+      if (this._engineMatchesIgnoreLists(engine)) {
+        await this.removeEngine(engine);
+        engineRemoved = true;
+      }
+    }
+    // If we've removed an engine, and we don't have any left, we need to do
+    // a re-init - it is possible the cache just had one engine in it, and that
+    // is now empty, so we need to load from our main list.
+    if (engineRemoved && !Object.keys(this._engines).length) {
+      this._reInit();
+    }
+  },
+
+  /**
+   * Determines if a given engine matches the ignorelists or not.
+   *
+   * @param {Engine} engine
+   *   The engine to check against the ignorelists.
+   * @returns {boolean}
+   *   Returns true if the engine matches a ignorelists entry.
+   */
+  _engineMatchesIgnoreLists(engine) {
+    if (this._loadPathIgnoreList.includes(engine._loadPath)) {
+      return true;
+    }
+    let url = engine._getURLOfType("text/html")
+                    .getSubmission("dummy", engine).uri.spec.toLowerCase();
+    if (this._submissionURLIgnoreList.some(code => url.includes(code.toLowerCase()))) {
+      return true;
+    }
+    return false;
+  },
+
   _metaData: { },
   setGlobalAttr(name, val) {
     this._metaData[name] = val;
@@ -2926,7 +3036,14 @@ SearchService.prototype = {
     // If we are reiniting, delete previously installed built in
     // extensions that arent in the current engine list.
     for (let id of this._extensions.keys()) {
-      let {extension} = WebExtensionPolicy.getByID(id);
+      let policy = WebExtensionPolicy.getByID(id);
+      if (!policy) {
+        // If we are reiniting due to a remote settings update, we may have
+        // removed all the engines and be rebuilding without the cache. In this
+        // case, we won't have the cached engines loaded, so just skip this check.
+        continue;
+      }
+      let extension = policy.extension;
       if (extension.addonData.builtIn && !engines.some(name => extensionId(name) === id)) {
         this._extensions.delete(id);
       }
@@ -3158,34 +3275,8 @@ SearchService.prototype = {
     return this._batchTask;
   },
 
-  _submissionURLIgnoreList: [
-    "ignore=true",
-    "hspart=lvs",
-    "pc=COSP",
-    "clid=2308146",
-    "fr=mca",
-    "PC=MC0",
-    "lavasoft.gosearchresults",
-    "securedsearch.lavasoft",
-  ],
-
-  _loadPathIgnoreList: [
-    "[other]addEngineWithDetails:searchignore@mozilla.com",
-    "[https]opensearch.startpageweb.com/bing-search.xml",
-    "[https]opensearch.startwebsearch.com/bing-search.xml",
-    "[https]opensearch.webstartsearch.com/bing-search.xml",
-    "[https]opensearch.webofsearch.com/bing-search.xml",
-    "[profile]/searchplugins/Yahoo! Powered.xml",
-    "[profile]/searchplugins/yahoo! powered.xml",
-  ],
-
   _addEngineToStore(engine) {
-    let url = engine._getURLOfType("text/html").getSubmission("dummy", engine).uri.spec.toLowerCase();
-    if (this._submissionURLIgnoreList.some(code => url.includes(code.toLowerCase()))) {
-      LOG("_addEngineToStore: Ignoring engine");
-      return;
-    }
-    if (this._loadPathIgnoreList.includes(engine._loadPath)) {
+    if (this._engineMatchesIgnoreLists(engine)) {
       LOG("_addEngineToStore: Ignoring engine");
       return;
     }
@@ -4691,6 +4782,11 @@ SearchService.prototype = {
   _observersAdded: false,
 
   _removeObservers() {
+    if (this._ignoreListListener) {
+      RemoteSettings(SETTINGS_IGNORELIST_KEY).off("sync", this._ignoreListListener);
+      delete this._ignoreListListener;
+    }
+
     Services.obs.removeObserver(this, SEARCH_ENGINE_TOPIC);
     Services.obs.removeObserver(this, QUIT_APPLICATION_TOPIC);
     Services.obs.removeObserver(this, TOPIC_LOCALES_CHANGE);

toolkit/components/search/tests/xpcshell/head_search.js

@@ -6,6 +6,7 @@ const {XPCOMUtils} = ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm")
 XPCOMUtils.defineLazyModuleGetters(this, {
   FileUtils: "resource://gre/modules/FileUtils.jsm",
   NetUtil: "resource://gre/modules/NetUtil.jsm",
+  RemoteSettings: "resource://services-settings/remote-settings.js",
   SearchTestUtils: "resource://testing-common/SearchTestUtils.jsm",
   Services: "resource://gre/modules/Services.jsm",
   setTimeout: "resource://gre/modules/Timer.jsm",
@@ -457,3 +458,37 @@ function checkCountryResultTelemetry(aExpectedValue) {
     deepEqual(snapshot.values, {});
   }
 }
+
+/**
+ * Provides a basic set of remote settings for use in tests.
+ */
+async function setupRemoteSettings() {
+  const collection = await RemoteSettings("hijack-blocklists").openCollection();
+  await collection.clear();
+  await collection.create({
+    "id": "submission-urls",
+    "matches": [
+      "ignore=true",
+    ],
+  }, { synced: true });
+  await collection.create({
+    "id": "load-paths",
+    "matches": [
+      "[other]addEngineWithDetails:searchignore@mozilla.com",
+    ],
+  }, { synced: true });
+  await collection.db.saveLastModified(42);
+}
+
+/**
+ * Some tests might trigger initialisation which will trigger the search settings
+ * update. We need to make sure we wait for that to finish before we exit, otherwise
+ * it may cause shutdown issues.
+ */
+let updatePromise = SearchTestUtils.promiseSearchNotification("settings-update-complete");
+
+registerCleanupFunction(async () => {
+  if (Services.search.isInitialized) {
+    await updatePromise;
+  }
+});

toolkit/components/search/tests/xpcshell/test_ignorelist.js

@@ -15,25 +15,31 @@ add_task(async function setup() {
   await AddonTestUtils.promiseStartupManager();
 });
 
-add_task(async function test_ignorelistEngineLowerCase() {
-  Assert.ok(!Services.search.isInitialized);
+add_task(async function test_ignoreList() {
+  await setupRemoteSettings();
+
+  Assert.ok(!Services.search.isInitialized,
+    "Search service should not be initialized to begin with.");
+
+  let updatePromise = SearchTestUtils.promiseSearchNotification("settings-update-complete");
 
   await Services.search.addEngineWithDetails(kSearchEngineID1, "", "", "", "get", kSearchEngineURL1);
 
-  // An ignored engine shouldn't be available at all
+  await updatePromise;
+
   let engine = Services.search.getEngineByName(kSearchEngineID1);
-  Assert.equal(engine, null, "Engine should not exist");
+  Assert.equal(engine, null, "Engine with ignored search params should not exist");
 
   await Services.search.addEngineWithDetails(kSearchEngineID2, "", "", "", "get", kSearchEngineURL2);
 
   // An ignored engine shouldn't be available at all
   engine = Services.search.getEngineByName(kSearchEngineID2);
-  Assert.equal(engine, null, "Engine should not exist");
+  Assert.equal(engine, null, "Engine with ignored search params of a different case should not exist");
 
   await Services.search.addEngineWithDetails(kSearchEngineID3, "", "", "", "get",
                                              kSearchEngineURL3, kExtensionID);
 
   // An ignored engine shouldn't be available at all
   engine = Services.search.getEngineByName(kSearchEngineID3);
-  Assert.equal(engine, null, "Engine should not exist");
+  Assert.equal(engine, null, "Engine with ignored extension id should not exist");
 });

toolkit/components/search/tests/xpcshell/test_ignorelist_update.js

@@ -0,0 +1,62 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+const kSearchEngineID1 = "ignorelist_test_engine1";
+const kSearchEngineID2 = "ignorelist_test_engine2";
+const kSearchEngineID3 = "ignorelist_test_engine3";
+const kSearchEngineURL1 = "http://example.com/?search={searchTerms}&ignore=true";
+const kSearchEngineURL2 = "http://example.com/?search={searchTerms}&IGNORE=TRUE";
+const kSearchEngineURL3 = "http://example.com/?search={searchTerms}";
+const kExtensionID = "searchignore@mozilla.com";
+
+add_task(async function setup() {
+  await AddonTestUtils.promiseStartupManager();
+});
+
+add_task(async function test_ignoreList() {
+  Assert.ok(!Services.search.isInitialized,
+    "Search service should not be initialized to begin with.");
+
+  let updatePromise = SearchTestUtils.promiseSearchNotification("settings-update-complete");
+  await Services.search.addEngineWithDetails(kSearchEngineID1, "", "", "", "get", kSearchEngineURL1);
+  await Services.search.addEngineWithDetails(kSearchEngineID2, "", "", "", "get", kSearchEngineURL2);
+  await Services.search.addEngineWithDetails(kSearchEngineID3, "", "", "", "get",
+                                             kSearchEngineURL3, kExtensionID);
+
+  // Ensure that the initial remote settings update from default values is
+  // complete. The defaults do not include the special inclusions inserted below.
+  await updatePromise;
+
+  for (let engineName of [kSearchEngineID1, kSearchEngineID2, kSearchEngineID3]) {
+    Assert.ok(await Services.search.getEngineByName(engineName),
+      `Engine ${engineName} should be present`);
+  }
+
+  // Simulate an ignore list update.
+  await RemoteSettings("hijack-blocklists").emit("sync", {
+    data: {
+      current: [{
+        "id": "load-paths",
+        "schema": 1553857697843,
+        "last_modified": 1553859483588,
+        "matches": [
+          "[other]addEngineWithDetails:searchignore@mozilla.com",
+        ],
+      }, {
+        "id": "submission-urls",
+        "schema": 1553857697843,
+        "last_modified": 1553859435500,
+        "matches": [
+          "ignore=true",
+        ],
+      }],
+    },
+  });
+
+  for (let engineName of [kSearchEngineID1, kSearchEngineID2, kSearchEngineID3]) {
+    Assert.equal(await Services.search.getEngineByName(engineName), null,
+      `Engine ${engineName} should not be present`);
+  }
+});

toolkit/components/search/tests/xpcshell/test_json_cache_ignorelist.js

@@ -11,14 +11,14 @@ var {getAppInfo} = ChromeUtils.import("resource://testing-common/AppInfo.jsm");
 
 var cacheTemplate, appPluginsPath, profPlugins;
 
-add_task(async function setup() {
-  await AddonTestUtils.promiseStartupManager();
-});
-
 /**
  * Test reading from search.json.mozlz4
  */
-function run_test() {
+add_task(async function setup() {
+  await AddonTestUtils.promiseStartupManager();
+
+  await setupRemoteSettings();
+
   let cacheTemplateFile = do_get_file("data/search_ignorelist.json");
   cacheTemplate = readJSONFile(cacheTemplateFile);
   cacheTemplate.buildID = getAppInfo().platformBuildID;
@@ -35,31 +35,29 @@ function run_test() {
   // The list of visibleDefaultEngines needs to match or the cache will be ignored.
   cacheTemplate.visibleDefaultEngines = getDefaultEngineList(false);
 
-  run_next_test();
-}
-
-add_test(function prepare_test_data() {
-  promiseSaveCacheData(cacheTemplate).then(run_next_test);
+  await promiseSaveCacheData(cacheTemplate);
 });
 
 /**
  * Start the search service and confirm the cache was reset
  */
-add_test(function test_cache_rest() {
+add_task(async function test_cache_rest() {
   info("init search service");
 
-  Services.search.init().then(function initComplete(aResult) {
-    info("init'd search service");
-    Assert.ok(Components.isSuccessCode(aResult));
+  let updatePromise = SearchTestUtils.promiseSearchNotification("settings-update-complete");
 
-    Services.search.getEngines().then(engines => {
-      // Engine list will have been reset to the default,
-      // Not the one engine in the cache.
-      // It should have more than one engine.
-      Assert.ok(engines.length > 1);
+  let result = await Services.search.init();
 
-      removeCacheFile();
-      run_next_test();
-    });
-  });
+  Assert.ok(Components.isSuccessCode(result),
+    "Search service should be successfully initialized");
+  await updatePromise;
+
+  const engines = await Services.search.getEngines();
+
+  // Engine list will have been reset to the default,
+  // Not the one engine in the cache.
+  // It should have more than one engine.
+  Assert.greater(engines.length, 1, "Should have more than one engine in the list");
+
+  removeCacheFile();
 });

toolkit/components/search/tests/xpcshell/xpcshell.ini

@@ -57,6 +57,7 @@ skip-if = true # Is confusing
 [test_engine_set_alias.js]
 [test_hasEngineWithURL.js]
 [test_identifiers.js]
+[test_ignorelist_update.js]
 [test_ignorelist.js]
 [test_invalid_engine_from_dir.js]
 [test_json_cache.js]