Bug 1323228 - throw an exception if using the storage API without an ID, r?kmag draft
authorEthan Glasser-Camp <eglassercamp@mozilla.com>
Mon, 19 Dec 2016 18:06:05 -0500
changeset 453046 f58d41b5cc2599c5014ccf08dc36933ad55699ee
parent 451111 d4b3146a5567a7ddbcdfa5244945db55616cb8d1
child 540363 6098212296b6dc4fa8936d253586ed7fc73dd077
push id39553
push usereglassercamp@mozilla.com
push dateThu, 22 Dec 2016 19:01:16 +0000
reviewerskmag
bugs1323228
milestone53.0a1
Bug 1323228 - throw an exception if using the storage API without an ID, r?kmag This protection is in ext-storage.js rather than ExtensionStorageSync.jsm because storage.local may one day be migrated to work using the same code as ExtensionStorageSync, but without any syncing event loop. When this happens, we still want to raise but only in the storage.sync case. MozReview-Commit-ID: Jwu9FA5DZA6
toolkit/components/extensions/ext-storage.js
toolkit/components/extensions/test/xpcshell/test_ext_storage.js
toolkit/mozapps/extensions/AddonManager.jsm
toolkit/mozapps/extensions/internal/XPIProvider.jsm
--- a/toolkit/components/extensions/ext-storage.js
+++ b/toolkit/components/extensions/ext-storage.js
@@ -1,22 +1,35 @@
 "use strict";
 
 var {classes: Cc, interfaces: Ci, utils: Cu} = Components;
 
 XPCOMUtils.defineLazyModuleGetter(this, "ExtensionStorage",
                                   "resource://gre/modules/ExtensionStorage.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "ExtensionStorageSync",
                                   "resource://gre/modules/ExtensionStorageSync.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "AddonManagerPrivate",
+                                  "resource://gre/modules/AddonManager.jsm");
 
 Cu.import("resource://gre/modules/ExtensionUtils.jsm");
 var {
   EventManager,
+  ExtensionError,
 } = ExtensionUtils;
 
+function enforceNoTemporaryAddon(extensionId) {
+  const EXCEPTION_MESSAGE =
+        "The storage API will not work with a temporary addon ID. " +
+        "Please add an explicit addon ID to your manifest. " +
+        "For more information see https://bugzil.la/1323228.";
+  if (AddonManagerPrivate.isTemporaryInstallID(extensionId)) {
+    throw new ExtensionError(EXCEPTION_MESSAGE);
+  }
+}
+
 function storageApiFactory(context) {
   let {extension} = context;
   return {
     storage: {
       local: {
         get: function(spec) {
           return ExtensionStorage.get(extension.id, spec);
         },
@@ -28,25 +41,29 @@ function storageApiFactory(context) {
         },
         clear: function() {
           return ExtensionStorage.clear(extension.id);
         },
       },
 
       sync: {
         get: function(spec) {
+          enforceNoTemporaryAddon(extension.id);
           return ExtensionStorageSync.get(extension, spec, context);
         },
         set: function(items) {
+          enforceNoTemporaryAddon(extension.id);
           return ExtensionStorageSync.set(extension, items, context);
         },
         remove: function(keys) {
+          enforceNoTemporaryAddon(extension.id);
           return ExtensionStorageSync.remove(extension, keys, context);
         },
         clear: function() {
+          enforceNoTemporaryAddon(extension.id);
           return ExtensionStorageSync.clear(extension, context);
         },
       },
 
       onChanged: new EventManager(context, "storage.onChanged", fire => {
         let listenerLocal = changes => {
           fire(changes, "local");
         };
--- a/toolkit/components/extensions/test/xpcshell/test_ext_storage.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_storage.js
@@ -1,15 +1,19 @@
 /* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
 /* vim: set sts=2 sw=2 et tw=80: */
 "use strict";
 
 const STORAGE_SYNC_PREF = "webextensions.storage.sync.enabled";
 Cu.import("resource://gre/modules/Preferences.jsm");
 
+add_task(function* setup() {
+  yield ExtensionTestUtils.startAddonManager();
+});
+
 /**
  * Utility function to ensure that all supported APIs for getting are
  * tested.
  *
  * @param {string} areaName
  *        either "local" or "sync" according to what we want to test
  * @param {string} prop
  *        "key" to look up using the storage API
@@ -327,8 +331,39 @@ add_task(function* test_backgroundScript
   yield extension.awaitMessage("test-finished");
 
   extension.sendMessage("test-sync");
   yield extension.awaitMessage("test-finished");
 
   Preferences.reset(STORAGE_SYNC_PREF);
   yield extension.unload();
 });
+
+add_task(function* test_storage_requires_real_id() {
+  async function backgroundScript() {
+    const EXCEPTION_MESSAGE =
+          "The storage API is not available with a temporary addon ID. " +
+          "Please add an explicit addon ID to your manifest. " +
+          "For more information see https://bugzil.la/1323228.";
+
+    await browser.test.assertRejects(browser.storage.sync.set({"foo": "bar"}),
+                                     EXCEPTION_MESSAGE);
+
+    browser.test.notifyPass("exception correct");
+  }
+
+  let extensionData = {
+    background: `(${backgroundScript})(${checkGetImpl})`,
+    manifest: {
+      permissions: ["storage"],
+    },
+    useAddonManager: "temporary",
+  };
+
+  Preferences.set(STORAGE_SYNC_PREF, true);
+
+  let extension = ExtensionTestUtils.loadExtension(extensionData);
+  yield extension.startup();
+  yield extension.awaitFinish("exception correct");
+
+  Preferences.reset(STORAGE_SYNC_PREF);
+  yield extension.unload();
+});
--- a/toolkit/mozapps/extensions/AddonManager.jsm
+++ b/toolkit/mozapps/extensions/AddonManager.jsm
@@ -3066,16 +3066,33 @@ this.AddonManagerPrivate = {
 
   hasUpgradeListener: function(aId) {
     return AddonManagerInternal.upgradeListeners.has(aId);
   },
 
   getUpgradeListener: function(aId) {
     return AddonManagerInternal.upgradeListeners.get(aId);
   },
+
+  /**
+   * Predicate that returns true if we think the given extension ID
+   * might have been generated by XPIProvider.
+   */
+  isTemporaryInstallID: function(extensionId) {
+     if (!gStarted)
+       throw Components.Exception("AddonManager is not initialized",
+                                  Cr.NS_ERROR_NOT_INITIALIZED);
+
+     if (!extensionId || typeof extensionId != "string")
+       throw Components.Exception("extensionId must be a string",
+                                  Cr.NS_ERROR_INVALID_ARG);
+
+    return AddonManagerInternal._getProviderByName("XPIProvider")
+                               .isTemporaryInstallID(extensionId);
+  },
 };
 
 /**
  * This is the public API that UI and developers should be calling. All methods
  * just forward to AddonManagerInternal.
  */
 this.AddonManager = {
   // Constants for the AddonInstall.state property
--- a/toolkit/mozapps/extensions/internal/XPIProvider.jsm
+++ b/toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ -1359,27 +1359,29 @@ function defineSyncGUID(aAddon) {
       delete aAddon.syncGUID;
       aAddon.syncGUID = val;
     },
     configurable: true,
     enumerable: true,
   });
 }
 
+const TEMPORARY_ADDON_SUFFIX = "@temporary-addon";
+
 // Generate a unique ID based on the path to this temporary add-on location.
 function generateTemporaryInstallID(aFile) {
   const hasher = Cc["@mozilla.org/security/hash;1"]
         .createInstance(Ci.nsICryptoHash);
   hasher.init(hasher.SHA1);
   const data = new TextEncoder().encode(aFile.path);
   // Make it so this ID cannot be guessed.
   const sess = TEMP_INSTALL_ID_GEN_SESSION;
   hasher.update(sess, sess.length);
   hasher.update(data, data.length);
-  let id = `${getHashStringForCrypto(hasher)}@temporary-addon`;
+  let id = `${getHashStringForCrypto(hasher)}${TEMPORARY_ADDON_SUFFIX}`;
   logger.info(`Generated temp id ${id} (${sess.join("")}) for ${aFile.path}`);
   return id;
 }
 
 /**
  * Loads an AddonInternal object from an add-on extracted in a directory.
  *
  * @param  aDir
@@ -3963,16 +3965,21 @@ this.XPIProvider = {
     let requireSecureOrigin = Preferences.get(PREF_INSTALL_REQUIRESECUREORIGIN, true);
     let safeSchemes = ["https", "chrome", "file"];
     if (requireSecureOrigin && safeSchemes.indexOf(uri.scheme) == -1)
       return false;
 
     return true;
   },
 
+  // Identify temporary install IDs.
+  isTemporaryInstallID: function(id) {
+    return id.endsWith(TEMPORARY_ADDON_SUFFIX);
+  },
+
   /**
    * Called to get an AddonInstall to download and install an add-on from a URL.
    *
    * @param  aUrl
    *         The URL to be installed
    * @param  aHash
    *         A hash for the install
    * @param  aName