Closed Bug 1323228 Opened 3 years ago Closed 3 years ago

chrome.storage.sync should throw an exception if using a temporary addon ID

Categories

(WebExtensions :: Untriaged, defect, P2)

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla53

People

(Reporter: glasserc, Assigned: glasserc)

References

Details

(Whiteboard: triaged)

Attachments

(1 file)

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.
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?
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).
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.
short term hack and correct fix will come later.  File separate bug that links to this and will take follow up.
Priority: -- → P2
Whiteboard: triaged
Assignee: nobody → eglassercamp
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)`
Attachment #8820066 - Flags: review?(kmaglione+bmo)
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.
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.
Attachment #8820066 - Flags: review?(kmaglione+bmo) → review+
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.
Keywords: checkin-needed
seems there is a open issue in mozreview - can this be fixed so that we can use autoland, thanks!
Flags: needinfo?(eglassercamp)
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!
Flags: needinfo?(eglassercamp)
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0dab09cc7ea5
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1326230
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.
Status: RESOLVED → VERIFIED
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.
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!
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" }`.
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!
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.