Ethan Glasser-Camp (:glasserc) Assignee
	Description • 8 years ago

WebExtensions without an ID in their manifest cannot use chrome.storage.sync, or really chrome.storage.local, reliably, because that ID is temporary and changes on every restart of the browser. (WebExtensions downloaded from AMO are signed and therefore have an ID.) Because this behavior is surprising (see bug 1319742), we ought to throw an exception when we detect use with a temporary ID. Because IDs in WebExtensions' manifests are "non-mandatory" but still allowed, a developer can work around this failing by providing an ID in the manifest, which should allow use of this API.

	Andrew Swan [:aswan]
	Comment 1 • 8 years ago

We don't currently have a way to tell from within an extension whether its ID came from the manifest or was assigned during temporary installation.
Perhaps the solution for this bug is just better documentation?

	Luca Greco [:rpl] [:luca] [:lgreco]
	Comment 2 • 8 years ago

I'm Adding some additional detail from a brief discussion that I had with Andrew about Comment 1:

- it is true that the auto-generated addonId are currently always suffixed with '@temporary-addon' (http://searchfox.org/mozilla-central/rev/594937fec2e2fc45fa9308ba2fb964816631f017/toolkit/mozapps/extensions/internal/XPIProvider.jsm#1378)

- but we cannot currently say for sure if the addon has explicitly set its own addonId to 'something@temporary-addon'

- it is possible that in the future we change the way the addon id is generated and the check on the suffix will not be able to detect the temporary addon id properly (and we should absolutely prevent this)

The third one can be covered by a test case of the sync API, to be sure that we don't break the check with changes in the code that generate the temporary addon id.

Anyway, in my opinion, at least in the long run, it would be way better if we could make the "WebExtensions internals" explicitly aware of the source of the addon id (e.g. auto-generated, explicitly assigned by the manifest or explicitly assigned).

	Ethan Glasser-Camp (:glasserc) Assignee
	Comment 3 • 8 years ago

I agree that relying on the format of a string is pretty lame, but I feel like improving the structure of addon IDs is beyond what I'm capable of, so I'd like to use this bug to just implement the following short-term fix:

- Extract the "@temporary-addon" suffix as a constant in XPIProvider.jsm and add a "recognizer" for it, the same way we do with extensions in JS (which are similarly "stringly typed")

- Use the recognizer in the storage code to refuse access to any addon which the recognizer claims is temporary (even if the user explicitly assigned this temporary name to their addon)

- Add a test to ensure that access to storage by an extension without an ID fails

I'd like to defer the more correct fix (updating the WebExtensions internals) to later, presumably by someone who is more qualified.

	:shell escalante
	Comment 4 • 7 years ago

short term hack and correct fix will come later.  File separate bug that links to this and will take follow up.

	Kris Maglione [:kmag]
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8820066 [details]
Bug 1323228 - throw an exception if using the storage API without an ID,

https://reviewboard.mozilla.org/r/99592/#review100302

::: toolkit/components/extensions/ExtensionCommon.jsm:246
(Diff revision 1)
>        message = error.message;
>      } else {
>        Cu.reportError(error);
>      }
>      message = message || "An unexpected error occurred";
> +    dump(`Message was ${message}\n`);

Remove.

::: toolkit/components/extensions/ext-storage.js:9
(Diff revision 1)
> +XPCOMUtils.defineLazyModuleGetter(this, "isTemporaryInstallID",
> +                                  "resource://gre/modules/addons/XPIProvider.jsm");

XPIProvider.jsm is not public. Importing it from outside of AddonManager code is not allowed.

::: toolkit/components/extensions/ext-storage.js:21
(Diff revision 1)
>  
> +function enforceNoTemporaryAddon(extensionId) {
> +  const EXCEPTION_MESSAGE =
> +        "The storage.sync API is not available with a temporary addon ID. " +
> +        "Please add an explicit addon ID to your manifest. " +
> +        "For more information see https://bugzilla.mozilla.org/show_bug.cgi?id=1323228.";

`https://bugzil.la/1323228`

::: toolkit/components/extensions/ext-storage.js:23
(Diff revision 1)
> +  const EXCEPTION_MESSAGE =
> +        "The storage.sync API is not available with a temporary addon ID. " +
> +        "Please add an explicit addon ID to your manifest. " +
> +        "For more information see https://bugzilla.mozilla.org/show_bug.cgi?id=1323228.";
> +  if (isTemporaryInstallID(extensionId)) {
> +    throw new ExtensionUtils.ExtensionError(EXCEPTION_MESSAGE);

Add this to the destructuring assignment at line 14, and use `new ExtensionError(...)`

::: toolkit/components/extensions/test/xpcshell/test_ext_storage.js:7
(Diff revision 1)
>  /* 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");
> +const {generateTemporaryInstallID} = Cu.import("resource://gre/modules/addons/XPIProvider.jsm");

Not necessary.

::: toolkit/components/extensions/test/xpcshell/test_ext_storage.js:349
(Diff revision 1)
> +    try {
> +      await browser.storage.sync.set({"foo": "bar"});
> +    } catch(e) {
> +      exception = e;
> +    }

Nit: `await browser.test.assertRejects`

Same for below.

::: toolkit/components/extensions/test/xpcshell/test_ext_storage.js:369
(Diff revision 1)
> +      applications: {
> +        gecko: {
> +          id: generateTemporaryInstallID({path: "abcd"}),
> +        },
> +      },

Remove this, add `useAddonManager: "temporary"` to the top-level extensionData object.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:12
(Diff revision 1)
>  const Cc = Components.classes;
>  const Ci = Components.interfaces;
>  const Cr = Components.results;
>  const Cu = Components.utils;
>  
> -this.EXPORTED_SYMBOLS = ["XPIProvider"];
> +this.EXPORTED_SYMBOLS = ["XPIProvider", "isTemporaryInstallID"];

This should not be exported by default. If it needs to be exported at all, it should be as a property of XPIProvider. But since XPIProvider is not public, it should probably be on AddonManagerPrivate instead.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1386
(Diff revision 1)
>    return id;
>  }
>  
> +// Identify temporary install IDs.
> +function isTemporaryInstallID(id) {
> +  return id.substring(id.length - TEMPORARY_ADDON_SUFFIX.length) == TEMPORARY_ADDON_SUFFIX;

`id.endsWith(TEMPORARY_ADDON_SUFFIX)`

	Ethan Glasser-Camp (:glasserc) Assignee
	Comment 8 • 7 years ago
mozreview-review-reply

Comment on attachment 8820066 [details]
Bug 1323228 - throw an exception if using the storage API without an ID,

https://reviewboard.mozilla.org/r/99592/#review100302

> `id.endsWith(TEMPORARY_ADDON_SUFFIX)`

Oops, thanks! I knew there was probably a better way to do this, but I was afraid that it had some kind of wacky JS name like `includes`. I meant to come back and clean this up, but forgot.

	Kris Maglione [:kmag]
	Comment 9 • 7 years ago
mozreview-review

Comment on attachment 8820066 [details]
Bug 1323228 - throw an exception if using the storage API without an ID,

https://reviewboard.mozilla.org/r/99592/#review100422

::: toolkit/mozapps/extensions/AddonManager.jsm:3089
(Diff revision 2)
> +     if (!extensionId || typeof extensionId != "string")
> +       throw Components.Exception("extensionId must be a string",
> +                                  Cr.NS_ERROR_INVALID_ARG);
> +
> +    return AddonManagerInternal._getProviderByName("XPIProvider")
> +                                .isTemporaryInstallID(extensionId);

Nit: One extra space.

	Ethan Glasser-Camp (:glasserc) Assignee
	Comment 10 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b6688806494

	Ethan Glasser-Camp (:glasserc) Assignee
	Comment 12 • 7 years ago

After more vigorous discussion in #webextensions, we made the decision to only raise exceptions if an addon tries to use storage.sync, not storage.local, with a temporary ID. This is because many, if not most, addons use storage.local, and throwing an exception every time an addon used storage.local would make developing such an addon more difficult with no clear benefit.

	Ethan Glasser-Camp (:glasserc) Assignee
	Comment 14 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=632e3ff6f72e

	Ethan Glasser-Camp (:glasserc) Assignee
	Comment 16 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f58d41b5cc25

	Carsten Book [:Tomcat]
	Comment 17 • 7 years ago

seems there is a open issue in mozreview - can this be fixed so that we can use autoland, thanks!

	Ethan Glasser-Camp (:glasserc) Assignee
	Comment 18 • 7 years ago

Oops, that got fixed a while ago, but I forgot I needed to explicitly mark it as fixed in mozreview. Should be fine now, thanks!

	Pulsebot
	Comment 19 • 7 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0dab09cc7ea5
throw an exception if using the storage API without an ID, r=kmag

	Phil Ringnalda (:philor)
	Comment 20 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/0dab09cc7ea5

	Vasilica Tamas, QA [:vtamas]
	Comment 21 • 7 years ago

Confirm that the following error is successfully displayed in Browser Console while installing a webextension without id in Firefox 53.0a1 (2016-12-29) under Windows 10 64-bit and Ubuntu 16.04 32-bit: 
    “Unchecked lastError value: Error: 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.                      ExtensionCommon.jsm:265”

I also noticed that after installing a webextension which uses a temporary addon ID, there is no longer data/data fields displayed in add-on details tab from about:addons and it seems to be regressed by this bug. Therefore, I filed Bug 1326230
 
Since the other issue is tracked separately I am marking this bug as Verified.

	Dominik Moritz
	Comment 22 • 7 years ago

This doesn't seem to work if I use the callback API.

    browser.storage.sync.get({foo: 'test'}, console.log)


However, it all works perfectly fine for the local storage api.

    browser.storage.local.get({foo: 'test'}, console.log)
    undefined
    Object { foo: "test" }

I'm on Firefox 54.0.

	Ethan Glasser-Camp (:glasserc) Assignee
	Comment 23 • 7 years ago

Hi Dominik, I don't think this behavior is related to this bug, which is about whether you are allowed to use a temporary add-on ID. Anyhow, if you want to use a callback API, I think you should be using chrome.storage.sync instead of browser.storage.sync (which is the promise-based API). Hope this helps!

	Dominik Moritz
	Comment 24 • 7 years ago

Hi Ethan, you are right. I got confused by that. However, the callback based API for sync is still incorrect. 

    chrome.storage.sync.get({foo: 'test'}, console.log)

passes undefined to the callback. However

    chrome.storage.local.get({foo: 'test'}, console.log)

works correctly and passes `{ foo: "test" }`.

	Ethan Glasser-Camp (:glasserc) Assignee
	Comment 25 • 7 years ago

I wasn't able to reproduce the behavior you describe on current Nightly, but if you are still having it, you should probably open a new bug, because it isn't related to this bug. Thanks!