Bug 1055616 - Skip addons addons without a sourceURI or from a non-secure domain rather than treating them as errors. r=rnewman

services/sync/modules/addonutils.js

@@ -38,21 +38,10 @@ AddonUtilsInternal.prototype = {
    *        Function to be called with result of operation.
    */
   getInstallFromSearchResult:
-    function getInstallFromSearchResult(addon, cb, requireSecureURI=true) {
+    function getInstallFromSearchResult(addon, cb) {
 
     this._log.debug("Obtaining install for " + addon.id);
 
-    // Verify that the source URI uses TLS. We don't allow installs from
-    // insecure sources for security reasons. The Addon Manager ensures that
-    // cert validation, etc is performed.
-    if (requireSecureURI) {
-      let scheme = addon.sourceURI.scheme;
-      if (scheme != "https") {
-        cb(new Error("Insecure source URI scheme: " + scheme), addon.install);
-        return;
-      }
-    }
-
     // We should theoretically be able to obtain (and use) addon.install if
     // it is available. However, the addon.sourceURI rewriting won't be
     // reflected in the AddonInstall, so we can't use it. If we ever get rid
@@ -80,8 +69,6 @@ AddonUtilsInternal.prototype = {
    *   syncGUID - Sync GUID to use for the new add-on.
    *   enabled - Boolean indicating whether the add-on should be enabled upon
    *             install.
-   *   requireSecureURI - Boolean indicating whether to require a secure
-   *     URI to install from. This defaults to true.
    *
    * When complete it calls a callback with 2 arguments, error and result.
    *
@@ -105,10 +92,6 @@ AddonUtilsInternal.prototype = {
     function installAddonFromSearchResult(addon, options, cb) {
     this._log.info("Trying to install add-on from search result: " + addon.id);
 
-    if (options.requireSecureURI === undefined) {
-      options.requireSecureURI = true;
-    }
-
     this.getInstallFromSearchResult(addon, function onResult(error, install) {
       if (error) {
         cb(error, null);
@@ -167,7 +150,7 @@ AddonUtilsInternal.prototype = {
         this._log.error("Error installing add-on: " + Utils.exceptionstr(ex));
         cb(ex, null);
       }
-    }.bind(this), options.requireSecureURI);
+    }.bind(this));
   },
 
   /**
@@ -261,6 +244,7 @@ AddonUtilsInternal.prototype = {
           installedIDs: [],
           installs:     [],
           addons:       [],
+          skipped:      [],
           errors:       []
         };
 
@@ -299,14 +283,20 @@ AddonUtilsInternal.prototype = {
         // ideally send proper URLs, but this solution was deemed too
         // complicated at the time the functionality was implemented.
         for (let addon of addons) {
-          // sourceURI presence isn't enforced by AddonRepository. So, we skip
-          // add-ons without a sourceURI.
-          if (!addon.sourceURI) {
-            this._log.info("Skipping install of add-on because missing " +
-                           "sourceURI: " + addon.id);
+          // Find the specified options for this addon.
+          let options;
+          for (let install of installs) {
+            if (install.id == addon.id) {
+              options = install;
+              break;
+            }
+          }
+          if (!this.canInstallAddon(addon, options)) {
+            ourResult.skipped.push(addon.id);
             continue;
           }
 
+          // We can go ahead and attempt to install it.
           toInstall.push(addon);
 
           // We should always be able to QI the nsIURI to nsIURL. If not, we
@@ -362,10 +352,49 @@ AddonUtilsInternal.prototype = {
     });
   },
 
+  /**
+   * Returns true if we are able to install the specified addon, false
+   * otherwise. It is expected that this will log the reason if it returns
+   * false.
+   *
+   * @param addon
+   *        (Addon) Add-on instance to check.
+   * @param options
+   *        (object) The options specified for this addon. See installAddons()
+   *        for the valid elements.
+   */
+  canInstallAddon(addon, options) {
+    // sourceURI presence isn't enforced by AddonRepository. So, we skip
+    // add-ons without a sourceURI.
+    if (!addon.sourceURI) {
+      this._log.info("Skipping install of add-on because missing " +
+                     "sourceURI: " + addon.id);
+      return false;
+    }
+    // Verify that the source URI uses TLS. We don't allow installs from
+    // insecure sources for security reasons. The Addon Manager ensures
+    // that cert validation etc is performed.
+    let requireSecureURI = true;
+    if (options && options.requireSecureURI !== undefined) {
+      requireSecureURI = options.requireSecureURI;
+    }
+
+    if (requireSecureURI) {
+      let scheme = addon.sourceURI.scheme;
+      if (scheme != "https") {
+        this._log.info(`Skipping install of add-on "${addon.id}" because sourceURI's scheme of "${scheme}" is not trusted`);
+        return false;
+      }
+    }
+    this._log.info(`Add-on "${addon.id}" is able to be installed`);
+    return true;
+  },
+
+
   /**
    * Update the user disabled flag for an add-on.
    *
-   * The supplied callback will ba called when the operation is
+   * The supplied callback will be called when the operation is
    * complete. If the new flag matches the existing or if the add-on
    * isn't currently active, the function will fire the callback
    * immediately. Else, the callback is invoked when the AddonManager

services/sync/modules/engines/addons.js

@@ -298,6 +298,14 @@ AddonsStore.prototype = {
     // engine and the record will try to be applied later.
     let results = cb.wait();
 
+    if (results.skipped.includes(record.addonID)) {
+      this._log.info("Add-on skipped: " + record.addonID);
+      // Just early-return for skipped addons - we don't want to arrange to
+      // try again next time because the condition that caused up to skip
+      // will remain true for this addon forever.
+      return;
+    }
+
     let addon;
     for (let a of results.addons) {
       if (a.id == record.addonID) {

services/sync/tests/unit/test_addon_utils.js

@@ -60,6 +60,9 @@ add_test(function test_handle_empty_source_uri() {
   do_check_true("installedIDs" in result);
   do_check_eq(0, result.installedIDs.length);
 
+  do_check_true("skipped" in result);
+  do_check_true(result.skipped.includes(ID));
+
   server.stop(run_next_test);
 });
 
@@ -79,44 +82,18 @@ add_test(function test_ignore_untrusted_source_uris() {
     let sourceURI = ioService.newURI(s, null, null);
     let addon = {sourceURI: sourceURI, name: "bad", id: "bad"};
 
-    try {
-      let cb = Async.makeSpinningCallback();
-      AddonUtils.getInstallFromSearchResult(addon, cb, true);
-      cb.wait();
-    } catch (ex) {
-      do_check_neq(null, ex);
-      do_check_eq(0, ex.message.indexOf("Insecure source URI"));
-      continue;
-    }
-
-    // We should never get here if an exception is thrown.
-    do_check_true(false);
+    let canInstall = AddonUtils.canInstallAddon(addon);
+    do_check_false(canInstall, "Correctly rejected a bad URL");
   }
 
-  let count = 0;
   for (let s of good) {
     let sourceURI = ioService.newURI(s, null, null);
     let addon = {sourceURI: sourceURI, name: "good", id: "good"};
 
-    // Despite what you might think, we don't get an error in the callback.
-    // The install won't work because the underlying Addon instance wasn't
-    // proper. But, that just results in an AddonInstall that is missing
-    // certain values. We really just care that the callback is being invoked
-    // anyway.
-    let callback = function onInstall(error, install) {
-      do_check_null(error);
-      do_check_neq(null, install);
-      do_check_eq(sourceURI.spec, install.sourceURI.spec);
-
-      count += 1;
-
-      if (count >= good.length) {
-        run_next_test();
-      }
-    };
-
-    AddonUtils.getInstallFromSearchResult(addon, callback, true);
+    let canInstall = AddonUtils.canInstallAddon(addon);
+    do_check_true(canInstall, "Correctly accepted a good URL");
   }
+  run_next_test();
 });
 
 add_test(function test_source_uri_rewrite() {
@@ -151,7 +128,11 @@ add_test(function test_source_uri_rewrite() {
   let server = createAndStartHTTPServer();
 
   let installCallback = Async.makeSpinningCallback();
-  AddonUtils.installAddons([{id: "rewrite@tests.mozilla.org"}], installCallback);
+  let installOptions = {
+    id: "rewrite@tests.mozilla.org",
+    requireSecureURI: false,
+  }
+  AddonUtils.installAddons([installOptions], installCallback);
 
   installCallback.wait();
   do_check_true(installCalled);

services/sync/tests/unit/test_addons_store.js

@@ -414,9 +414,10 @@ add_test(function test_create_bad_install() {
   let record = createRecordForThisApp(guid, id, true, false);
 
   let failed = store.applyIncomingBatch([record]);
-  do_check_eq(1, failed.length);
-  do_check_eq(guid, failed[0]);
-  do_check_eq(sumHistogram("WEAVE_ENGINE_APPLY_FAILURES", { key: "addons" }), 1);
+  // This addon had no source URI so was skipped - but it's not treated as
+  // failure.
+  do_check_eq(0, failed.length);
+  do_check_eq(sumHistogram("WEAVE_ENGINE_APPLY_FAILURES", { key: "addons" }), 0);
 
   let addon = getAddonFromAddonManagerByID(id);
   do_check_eq(null, addon);