1192435 - Support custom update urls in Web Extensions

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Dave Townsend [:mossop]]

Currently we just always use AMO's update.rdf service. We should be able to load a custom update_url property from applications.gecko and do update checks with it.

Comment 1

[Kris Maglione [:kmag]]

Do we want these to use the current RDF update protocol?

If we're going to support custom update URLs for web extensions, it might be a good opportunity to come up with a simpler protocol. Ideally, just a JSON manifest that needs to be served over HTTPS, and doesn't support signing.

Comment 2

[Andy McKay]

(In reply to Kris Maglione [:kmag] from comment #1)
> Do we want these to use the current RDF update protocol?
> 
> If we're going to support custom update URLs for web extensions, it might be
> a good opportunity to come up with a simpler protocol. Ideally, just a JSON
> manifest that needs to be served over HTTPS, and doesn't support signing.

I'd be up for a simpler (and lower bandwith) update protocol for all add-ons, not just web extensions, but recommend that should be split into a separate project and set of bugs.

Comment 3

[Kris Maglione [:kmag]]

Yeah, it's definitely a separate project. What I'm mainly asking is whether we want to add this feature before it's implemented.

Getting updates working with the current protocol is pretty difficult as it is. Especially when using signed manifests. But this adds an additional complication, since most existing tools only know how to generate update manifests for add-ons with an install.rdf file.

At the very least, I don't want to add this feature until we have better docs for setting up updates. But if we're going to implement a simpler protocol, I'd rather wait for that to be ready too, so that people don't waste time figuring out the old method.

I'd definitely like any new update protocol to work with all extensions. I think that web extensions present a special case, though, since web extensions don't work on older versions of Firefox, so developers of web extensions won't have to worry about supporting both protocols for a single manifest.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

We can't ship WebExtensions to users until they can be updated. We need WebExtensions for e10s. So this has to be done in the short term, and I think the existing RDF-based updater is the only way to do that. Implementing a new update scheme would take too long.

Comment 5

[Kris Maglione [:kmag]]

They can be updated from AMO whether or not we have support for a custom update URL.

I've seen a lot of people get frustrated trying to implement updates with the current scheme. I'm worried that add support for custom updates without making it easy, we're going to make people more upset than if we hadn't implemented custom updates at all. And if we add support and then develop a new scheme later, a lot of developers are going to wind up trying to support both, for the sake of older Firefox versions.

So, if we do stick with the current scheme, but allow updates, we're at least going to need to make it as easy as possible. Which means writing tools to generate manifests for web extensions packages, and probably removing support for signed manifests.

I'm not entirely sure that implementing a new update scheme would be that difficult, though. If the new scheme is a JSON manifest that's similar to the RDF update format, but without signing and RDF arcana, I think we should be able to just replace `parseRDFManifest` in AddonUpdateChecker.jsm with a function to validate the JSON manifest. And I suspect we'll save a lot of headaches doing that sooner rather than later.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Well, as long as it can be done quickly, I'm content.

Comment 7

[Kris Maglione [:kmag]]

I'm proposing to implement a JSON protocol, with update manifests in the following format, but otherwise identical to the current RDF update protocol:

{
  "addon": {
    "id": "addon@id"
  },
  "updates": [
    {
      "version": "1.0",
      "update_link": "https://...",
      [optional if update_link is https] "update_hash": "sha256:...",
      [optional, future] "update_link_key": "pinned key for update link as in Public-Key-Pins header",

      [optional] "update_info_url": "https://...",

      [optional] "multiprocess_compatible": true,

      [optional] "applications": {
        "gecko": {
          [optional] "strict_min_version": "42a1",
          [optional] "strict_max_version": "*", (any value other than "*" implies the strictCompatibility flag)
        }
      }
    }
  ]
}


I chose some of the key names to make the format more similar to our manifest.json format than to install.rdf/update.rdf.

The "update_link_key" property shouldn't be implemented in the first version, but I think it would be a good idea plan on allowing pinned keys for both the update link in manifest.json and the update links in update.json in the future, for people who insist on added security.


Mossop, does this sound good/doable to you?

Comment 8

[Dave Townsend [:mossop]]

I would suggest making it possible to include information about multiple add-ons in a single file, in case developers want to use a shared json for all their add-ons. You could just use an array for that but I think it would be nicer for parsing to do something like:

{
  "addon1@id": {
    "updates": [ ... as above ... ],
  },
  "addon2@id": {
    "updates": [ ... as above ... ],
  }
}

I suggest making the updates array a property of the add-on's object because it leaves us scope to include additional information about the add-on there. For example in the future we could extend this format to replace the current metadata ping and be able to pull metadata and update info in one request.

Comment 9

[Kris Maglione [:kmag]]

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

Comment 10

[Kris Maglione [:kmag]]

I don't think I've ever written an AddonManager test that wasn't broken by mandatory file locking on Windows... https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ba786bab3e8

Comment 11

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1192435: Support updates for WebExtensions. r?Mossop

Bug 1192435: Support updates for WebExtensions. r?Mossop

Comment 12

[Kris Maglione [:kmag]]

This could probably use a few more tests, but most of the functionality should already be covered by tests for other extension types. I tried to cover the basics, and the major ways these updates differ from other extension updates.

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8689688 [details]
MozReview Request: Bug 1192435: Support updates for WebExtensions. r?Mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25685/diff/1-2/

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 8689688 [details]
MozReview Request: Bug 1192435: Support updates for WebExtensions. r?Mossop

https://reviewboard.mozilla.org/r/25685/#review23377

Looks good but I'm not sure why we are adding the restriction that web extensions cannot update to other types. Can you explain?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:862
(Diff revision 2)
> -    return findProp(manifest, "", path.split("."));
> +    let val = findProp(manifest, "", path.split("."), type);

findProp doesn't take the type argument.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:863
(Diff revision 2)
> +    if ({}.toString.call(val) != `[object ${type}]`)

Reads a little wierd. Can we just use typeof here?

::: toolkit/mozapps/extensions/test/xpcshell/test_update_webextensions.js:44
(Diff revision 2)
> +  temp_xpis.push(addonFile);

Since temp_xpis exists in the head file I'd rather we try to only manipulate it from there. Can you add a createTempWebExtensionFile there that does the same thing as createTempXPIFile and then use it here.

::: toolkit/mozapps/extensions/test/xpcshell/test_update_webextensions.js:101
(Diff revision 2)
> +      file = Extension.generateXPI(id, addon);

It looks like this file won't be removed on test cleanup

Comment 15

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/25685/#review23377

WebExtensions are supposed to have restricted privileges. If they can easily replace themselves with fully-privileged XUL add-ons, that privilege restriction is pretty useless, so I don't think we should allow non-AMO updates while it's possible.

> findProp doesn't take the type argument.

Urgh. Forgot to remove that after I refactored.

> Reads a little wierd. Can we just use typeof here?

Unfortunately, no. Not unless we want to accept arrays as objects, anyway, which I don't think is a good idea. This seems to have become the Right Way to do basic JavaScript type checking :(


Anyway, we should hopefully be able to get rid of this after bug 1225715 lands.

> Since temp_xpis exists in the head file I'd rather we try to only manipulate it from there. Can you add a createTempWebExtensionFile there that does the same thing as createTempXPIFile and then use it here.

Fair enough. I was a bit leery of that too.

> It looks like this file won't be removed on test cleanup

It gets moved to the `TmpD/addons` directory, which gets removed after the test.

Comment 16

[Kris Maglione [:kmag]]

Comment on attachment 8689688 [details]
MozReview Request: Bug 1192435: Support updates for WebExtensions. r?Mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25685/diff/2-3/

Comment 17

[Dave Townsend [:mossop]]

https://reviewboard.mozilla.org/r/25685/#review23377

Regardless of whether they update from AMO or not updates still need to be signed so if they upgrade to something with higher privileges then it will have been reviewed as such. My concern is that we want people to switch to WebExtensions. If we tell them that once they switch there is no going back I think there will be less people updating. Have we had a discussion about this somewhere that I missed?

Comment 18

[Kris Maglione [:kmag]]

AMO controls what updates are served for a given add-on, though. Custom update URLs can redirect to an add-on with a different ID, which wasn't approved as an upgrade from a web extension.

I did think about the possible deterrent effect, but in the end, people are going to have to switch either way, because we're going to remove support for XUL extensions. People who need a more gradual shift should probably wait for bug 1215035.

There hasn't been a discussion. We can have one, but it just seemed like an obviously necessary restriction, since the sandboxing is completely worthless without it.

Comment 19

[Dave Townsend [:mossop]]

(In reply to Kris Maglione [:kmag] from comment #18)
> AMO controls what updates are served for a given add-on, though. Custom
> update URLs can redirect to an add-on with a different ID, which wasn't
> approved as an upgrade from a web extension.

Do we care whether an add-on was approved as an update from a web extension? Do we check this during reviews? I don't know why we'd care if the update is safe for users then it shouldn't matter what it updated from. We don't currently tell users that web extensions are safer in any way so it's not like they'll be surprised in any way.

> There hasn't been a discussion. We can have one, but it just seemed like an
> obviously necessary restriction, since the sandboxing is completely
> worthless without it.

I'd like us to have more general buy-in, both because it doesn't seem obvious to me that it is necessary and because I'd like to have something better to refer the first developer that gets bit by it to than something we came up with on the fly.

Comment 20

[Kris Maglione [:kmag]]

(In reply to Dave Townsend [:mossop] from comment #19)
> Do we care whether an add-on was approved as an update from a web extension?

Well, given that I think switching from a WebExtension to a XUL extension
should be treated as a special case, I think so.

> Do we check this during reviews?

Not yet, because we've only just added basic support for WebExtensions to AMO.
I haven't raised it as an issue with the review team, because I was assuming
it would be checked in-product.

> I don't know why we'd care if the update is safe for users then it shouldn't
> matter what it updated from. We don't currently tell users that web
> extensions are safer in any way so it's not like they'll be surprised in any
> way.

We don't yet, but there's been talk about making it easier to install
WebExtensions than privileged extensions, and at some point we're going to
have to start surfacing the permissions that an extension asks for (which is
another thing we're going to have to check during updates). I guess we could
put this check off until then, but I'd rather it be there from the start.

Comment 21

[Blake Winton (:bwinton) (:☕️)]

(In reply to Dave Townsend [:mossop] from comment #19)
> > AMO controls what updates are served for a given add-on, though. Custom
> > update URLs can redirect to an add-on with a different ID, which wasn't
> > approved as an upgrade from a web extension.
> 
> Do we care whether an add-on was approved as an update from a web extension?
> Do we check this during reviews? I don't know why we'd care if the update is
> safe for users then it shouldn't matter what it updated from. We don't
> currently tell users that web extensions are safer in any way so it's not
> like they'll be surprised in any way.

I think this would be an unpleasant surprise if the user chose not to give a WebExtension some of its optional permissions, but the add-on was then updated to a more permissive SDK-based add-on.  I don't know if that automatic-granting-of-previously-denied privileges is serious enough to block this kind of update, but it seems like something to consider.  (Maybe we should put up a warning in those cases?)

There's more info on optional permissions at https://developer.chrome.com/extensions/permissions#manifest

Comment 22

[Dave Townsend [:mossop]]

Comment on attachment 8689688 [details]
MozReview Request: Bug 1192435: Support updates for WebExtensions. r?Mossop

https://reviewboard.mozilla.org/r/25685/#review23641

Comment 23

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/a626c88b80a1d31f7bffb819df499123ba9ebed8
Bug 1192435: Support updates for WebExtensions. r=Mossop

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a626c88b80a1

Comment 25

[Vasilica Tamas, QA [:vtamas]]

Is there any proper way of manually testing this bug? If yes, could you please provide some reliable steps?

Comment 26

[Kris Maglione [:kmag]]

The process is documented here:

https://developer.mozilla.org/en-US/Add-ons/Updates

If it's not clear enough, please let me know, and I'll update the docs.

Comment 27

[Vasilica Tamas, QA [:vtamas]]

I’m not quite sure if I correctly understand how I have to test a webextensions that is not hosted on amo using the update url. Can you please provide me a specific case?