661929 - don't append @jetpack to IDs that already have @ in them

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Brian Warner [:warner :bwarner]]

Bug 661376 highlights a limitation in the SDK that fell out of the recent removal of DSA keys. The value in package.json:id used to be the "JID", something like "jid0-ABCDE". The JID identified a specific pubkey. The full add-on ID was built from the JID by appending "@jetpack", because add-on IDs must look like email addresses (or {}-bracketed GUIDs).

Since we gave up on pubkeys, package.json:id could be more accomodating. We could allow authors to put the full add-on ID in this field, instead of merely a value that will be used to derive the full add-on ID. To this end, I think we should consider changing the ID-derivation logic to only append "@jetpack" if the package.json:id field doesn't already have one. This way, authors could use "id": "[e-mail]" in their package.json and expect it to get copied all the way into the final install.rdf .

The bug 661376 problem is a side-effect of this: flightdeck's repackaging service is attempting to take a generated XPI and reverse-engineer the package.json (and other source code), to feed back into 'cfx xpi'. Without this change, the translation process must reject XPIs with install.rdfs with addon-IDs that *don't* end with @jetpack, and must strip the @jetpack when writing out the package.json:id field. If we make this change, the process should copy the addon-ID verbatim from install.rdf into package.json:id, knowing that whatever suffix it uses (maybe @jetpack, maybe @jetpocketlint, etc) will survive the roundtrip.

clouserw has suggested this should probably block SDK-1.0 . At the very least, flightdeck will need a version of the SDK that can do this (we could conceivably ship 1.0 without this change, but make a special version or 1.0.1 or something with the change and let flightdeck use that internally).

Comment 1

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #0)
> (we could
> conceivably ship 1.0 without this change, but make a special version or
> 1.0.1 or something with the change and let flightdeck use that internally).

Indeed, but probably better to fix this in 1.0.

Comment 2

[Brian Warner [:warner :bwarner]]

notes to self about the current code:

* the SDK reads "jid" from package.json:id, and asserts that it *doesn't* end
  in @jetpack
* it computes "bundle_id", which is the jid plus @jetpack . This is written
  into install.rdf as the "em:id" element. It is also passed into
  harness-options.json as "bundleID", but I can't find any uses of it in
  packages/*
* it computes unique_prefix (used for resource: URIs), which is the jid (with
  any stray @ or . replaced with DNS-safe characters)
* it computes harness_contract_id with @mozilla.org/harness-service;1?id=
  plus the jid. This is passed into harness-options.json as
  bootstrap.contractID, and possibly used by some random runtime JS, but it's
  not clear to me how it's actually used, or what constraints are thus
  imposed
* the jid is passed into harness-options.json as "jetpackID". This is used
  by the runtime JS in the following ways:
** widget.js uses it to construct a "unique and stable widget id", as
   "widget:$JID-$WIDGETID"
** require(self).id is the jid, and .uri is "addon:" plus the jid
** securable-module.js CompositeFileSystem derives a URI from the jid
   ("resource://$JID-$NAME-lib/") for performing runtime module searches
   (allows test/ files to use require() search behaviors at runtime even
   though they aren't added to the manifest)
** observer-service.js uses it in an APPLICATION_READY topic name

So the concern with allowing "jid" to include an @something suffix whether
the places that currently use it could tolerate a string with that suffix.
Suppose we make this change:

* the SDK reads package.json:id, appends "@jetpack" iff there isn't already a
  "@" in the value, assigns the result to "jid"
* bundle_id = jid

Then what might break?:

* unique_prefix now has @ in it, which gets replaced with -at- . We need to
  include the @something suffix in unique_prefix to distinguish two addons
  named [e-mail] and [e-mail] (previously the JID was sufficient
  for uniqueness), and uniqueness currently needs to follow the rules of DNS
  hostnames (resource://$UNIQUE_PREFIX-$STUFF/filename.js). So basically URIs
  look a little bit uglier: resource://cool-at-alice-dot-com-$PKG-lib/foo.js
* harness_contract_id: can that tolerate having an @ in the id= arg?
* widget.js widget ids can probably tolerate it
* any addons that use require(self).id will observe a change, but we haven't
  promised anything about what it returns, so if that breaks anything, it's
  arguably their fault
* securable-module.js CompositeFileSystem needs to change to match what
  unique_prefix does
* can observer topics tolerate @ in the names?

Comment 3

[Brian Warner [:warner :bwarner]]

Attachment: first pass: include "@jetpack" suffix in JID

Here's a first cut, which implements the proposal above. URIs are kinda verbose, and I'm not sure that all tests still pass. Take a look and see if it seems reasonable. I still need to add some specific tests of both id="foo" and id="foo@suffix".

Comment 4

[Brian Warner [:warner :bwarner]]

Comment on attachment 537284 [details] [diff] [review]
first pass: include "@jetpack" suffix in JID

Irakli, what do you think?

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 537284 [details] [diff] [review]
first pass: include "@jetpack" suffix in JID

Review of attachment 537284 [details] [diff] [review]:
-----------------------------------------------------------------

Brian looks good, but I think we should stick to `endswith`. Also maybe you can include `unique_prefix` into `harness-options.json` along with `jetpackID` so that `securable-module.js` can use that instead of normalizing jetpackID as you suggested before.

::: python-lib/cuddlefish/__init__.py
@@ +598,5 @@
>  
>      if "id" in target_cfg:
>          jid = target_cfg["id"]
> +        if "@" not in jid:
> +            jid = jid + "@jetpack"

I believe we should stick to `endswith`. I think it was used in order to allow addon developers to port their addon's without requiring id changes.

So if I had addon with id: [e-mail] I will be still able to use that id. With this change we will change id to
[e-mail]@jetpack.

Comment 6

[Brian Warner [:warner :bwarner]]

hm, I don't think "[e-mail]@jetpack" is a legal addon ID, is it? I'd expect that if my package.json had 'id: "[e-mail]"', then that's what the resulting XPI would use, not "[e-mail]@jetpack".

But yeah, passing unique_prefix into harness-options.json might be a good idea. On one hand it feels like revealing internals (with the usual worries about leaky abstractions), but on the other hand I'd rather do that than have runtime code trying to derive the same sort of value and getting it wrong.

Comment 7

[Jeff Balogh (:jbalogh)]

{e20d0cf8-f4b5-c8b4-a8b2-2b9679e08c5a} is also a valid em:id format used by existing add-ons.

Comment 8

[Brian Warner [:warner :bwarner]]

Irakli: take a new look at my https://github.com/warner/addon-sdk/tree/661929-jids branch, I just pushed a change to pass uriPrefix into the runtime. Something is still broken, though, there's one test failing. Any ideas? I thought I passed uriPrefix in through the same places that jetpackID is currently passed, but during that test uriPrefix is showing up as undefined.

Comment 9

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to comment #6)
> hm, I don't think "[e-mail]@jetpack" is a legal addon ID, is it?
> I'd
> expect that if my package.json had 'id: "[e-mail]"', then that's what
> the resulting XPI would use, not "[e-mail]@jetpack".
> 

Sorry ignore my comment, I don't know why I thought it would append @jetpack in case of `[e-mail]` it wont cause `[e-mail]` contains `@`.

Also Jeff made a good point though, in case of id like `{e20d0cf8-f4b5-c8b4-a8b2-2b9679e08c5a}` we will append @jetpack, and we should not.

> But yeah, passing unique_prefix into harness-options.json might be a good
> idea. On one hand it feels like revealing internals (with the usual worries
> about leaky abstractions), but on the other hand I'd rather do that than
> have runtime code trying to derive the same sort of value and getting it
> wrong.

I really hope that runtime search will go away entirely soon, well after 1.0 but still soon!

Comment 10

[Brian Warner [:warner :bwarner]]

I've updated my branch to fix the unit tests (not necessarily as elegantly as possible: I changed test-securable-module.js to pass a dummy uriPrefix: value into each Loader() call). It also changes the reading-data example to use a foo@bar -style ID.

I changed the code to refrain from appending @jetpack to {GUID}-style "id" values, but it turns out that's not sufficient to allow package.json:id to use them: the JID is also used to construct resource: URIs, and {GUID} is *not* valid in that context. We'll need to come up with a different approach to allow {GUID} there.

Comment 11

[Brian Warner [:warner :bwarner]]

Attachment: accept package.json:id with full-qualified

This patch passes a distinct "uriPrefix" into the runtime (via harness-options.json) so the runtime search builds the right URIs. It should tolerate both id="foo" and id="foo@host". It does *not* yet handle id="{GUID}", as the ID is still used to generate a URI prefix, and {} are not valid there.

This patch is also in my https://github.com/warner/addon-sdk/tree/661929-jids branch. I will probably collapse it into a single patch when landing on trunk, to make it easier for myk to cherry-pick it onto the 1.0 branch.

Comment 12

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 537830 [details] [diff] [review]
accept package.json:id with full-qualified

Review of attachment 537830 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

Comment 13

[Brian Warner [:warner :bwarner]]

Landed in https://github.com/mozilla/addon-sdk/commit/2a3a6e3aa2b755eeb28bb253f790525b04042ef6 .

myk, you probably want to cherry-pick that to the 1.0 branch

Comment 14

[Piotr Zalewa [:zalun]]

Will this be pulled in the 1.0?

1.0rc2 still raises:
SDK 1.0rc2 still displays following:
Error: Traceback (most recent call last):
  File "/tmp/tmpyyRWwA/bin/cfx", line 29, in <module>
    cuddlefish.run()
  File "/tmp/tmpyyRWwA/python-lib/cuddlefish/__init__.py", line 595, in run
    assert not jid.endswith("@jetpack")

Comment 15

[Piotr Zalewa [:zalun]]

Sorry - my bad - I was using wrong SDK in the tests
It looks like it's working fine.