814136 - Verify the per-app unique identifier and monotonically-increasing version number in packaged apps during app update

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect, P4)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

+++ This bug was initially created as a clone of Bug #814131 +++
+++ This bug was initially created as a clone of Bug #791741 +++

If/when we include the per-app identifier and version number in the packaged apps in bug 791741, then we should verify that, during an app update, that the old app and the new app have the same ID and that the new version is higher (or the same?) as the old version. This way, we guard against the replacement of one app with a totally different app, and we guard against version downgrades.

Note that we need to consider the identity of the appstore as part of the key. Basically, we must assume that each appstore correctly generates unique IDs within its own scope, but we can't assume that there aren't collisions between the IDs used by two different appstores.

Note that this implies that you would not be able to replace an app installed from one appstore with a new version of the app installed from a different appstore, because they would be considered completely different apps.

Comment 1

[Rob Hudson [:robhudson]]

(In reply to Brian Smith (:bsmith) from comment #0)
> Note that we need to consider the identity of the appstore as part of the
> key. Basically, we must assume that each appstore correctly generates unique
> IDs within its own scope, but we can't assume that there aren't collisions
> between the IDs used by two different appstores.

Would the path to the mini-manifest work as a unique per-store identifier? I know this is stored on device since this is how updates work. It's not stored as part of the package but that could change -- perhaps when we mark the package as approved publicly we put the URL into the package itself.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Rob Hudson [:robhudson] from comment #1)
> Would the path to the mini-manifest work as a unique per-store identifier? I
> know this is stored on device since this is how updates work. It's not
> stored as part of the package but that could change -- perhaps when we mark
> the package as approved publicly we put the URL into the package itself.

The client would treat this as an opaque string so it can contain anything you can ensure will be unique per appstore. The choice is up to the appstore team. If you use a URL, the appstore team will have to be careful to ensure that if you ever change the URL structure, then the old URL structure must continue to be used as the unique ID in the JAR for apps that existed before the transition to the new URL structure.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Using the URL would work since the URL of the minimanifest has to remain constant anyway since we're going to be checking for updates on the same URL every time.

Though potentially that might change in the future to allow for things like a store changing URL scheme or some such. So if you already have some other unique identifier that you're using as part of the URL you could also use that. I.e. if your URLs are something like:

https://marketplace.mozilla.org/manifests/<something unique>/manifest.json

then you might as well use just the <something unique> part.

Comment 4

[Andrew Overholt [:overholt]]

Are you going to do this work, Brian?

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I will take it. Potentially Fabrice or others working on app installation could do this too.

Comment 6

[Rob Hudson [:robhudson]]

(In reply to Jonas Sicking (:sicking) from comment #3)
> Using the URL would work since the URL of the minimanifest has to remain
> constant anyway since we're going to be checking for updates on the same URL
> every time.
> 
> Though potentially that might change in the future to allow for things like
> a store changing URL scheme or some such.

Following a 301 to the mini-manifest might be an easy way to allow changing of URLs schemes.

Comment 7

[Andrew Overholt [:overholt]]

Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.

Comment 8

[Alex Keybl [:akeybl]]

Mass Modify: All un-milestoned, unresolved blocking-basecamp+ bugs are being moved into the C3 milestone. Note that the target milestone does not mean that these bugs can't be resolved prior to 12/10, rather C2 bugs should be prioritized ahead of C3 bugs.

Comment 9

[Dietrich Ayala (:dietrich)]

No material updates in >2 weeks. Brian, is this still blocking work for v1? Is it going to be fixed in C3 timeframe?

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Dietrich Ayala (:dietrich) from comment #9)
> No material updates in >2 weeks. Brian, is this still blocking work for v1?
> Is it going to be fixed in C3 timeframe?

I would say this is on the borderline of blocking. The server-side part is already done (though obviously we haven't done an interop test yet). I'm going to work on bug 821207 and the marketplace-side signing issues before this.

Comment 11

[JP Rosevear [:jpr]]

Moving to soft block as per #10.  Will take if you get to it.

Comment 12

[Jason Smith [:jsmith]]

(In reply to JP Rosevear [:jpr] from comment #11)
> Moving to soft block as per #10.  Will take if you get to it.

We've changed processes here. Can the platform guys agree with the Gaia guys to use tracking-b2g18+ for things like what you've described?

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Putting this back to nom. I think the bang for the buck here is pretty high. Also, right now the marketplace could end up using mirrors which aren't serving content over https, which means that we'd be vulnerable to MITM.

The alternative is to ask marketplace to not use mirrors.

Comment 14

[Wil Clouser [:clouserw]]

(In reply to Jonas Sicking (:sicking) from comment #13)
> 
> The alternative is to ask marketplace to not use mirrors.

I actually lied to you earlier.  We switched 100% to our CDNs a while ago and haven't gone back to mirrors.  It's always been an option, but not one we've had to use.  So, at this point, app packages would be coming from our SSL CDN(s).

Comment 15

[Andrew Overholt [:overholt]]

Clearing needinfo on JP as we now know the proper flags to use :)

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

Based on comment 14 about always using CDN with SSL, triage is putting this back into the soft block (b2g18+) state. 

Jonas - Feel free to renom if we've missed part of your concern from comment 13.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

By the way, if we get a super-safe patch here I believe we'd still take it.

Comment 18

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Unassigning to make it clear that I'm not yet working on this, in case somebody else wants to pick it up. I will pick it back up again if nobody else does by the end of the week.

Comment 19

[Antonio Manuel Amaya Calvo (:amac)]

I can take this, but I'm not sure of what you want to do here.

What it's needed here? Just to verify that the install manifest has a package id field, and control/store it and the version number?

Comment 20

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Fabrice is also looking at this. 

Get a signed app from the marketplace, and look at META-INF/ids.json inside the JAR. (There is a "webapi permission test" app that I typically use for testing. ) The unique ID and the version number are within that entry.

The automated tests for packaged app signing are in security/manager/ssl/tests/unit/test_signed_apps.*. If you put the ID-checking code in JARSignatureVerification.cpp then it would be easy to update the existing automated tests to test the logic. However, I suspect that you may find it easier to just do the checks without Webapps.jsm in Javascript code, in which case I don't think we have an automated testing solution yet. In security/manager/tests/unit/test_signed_apps/, there is a script to generate an app and sign it. It can be modified  to create test apps with the ids.json file in them. security/manager/ssl/tests/unit/test_signed_apps.js shows how to import the test signing cert so that we will actually trust the signed apps.

Comment 21

[Antonio Manuel Amaya Calvo (:amac)]

I already have a working signing framework, and a working 'store' to test signed apps. 

Do you want me to generate also the ids.json content or do you have the code to do that on the store already? If it's the second, do you have one that I can use to see the format (name of fields mostly).

I think the easiest way to implement this would be on Webapps.jsm (add that fields to the app database, also for certified/preinstalled apps and after the signature is checked, just read the json, check if it's already on the database and act in consequence). To automatically test it... well, I can cross that river when I reach it ;)

Can I/should I use the unique identifier as directory/app identifier also?

Comment 22

[Antonio Manuel Amaya Calvo (:amac)]

I'm taking this, I'll upload a first version of the test signer in a few and the rest later today

Comment 23

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: WIP, part1. Signed package generator example

So far so well, this just adds a ids.json (passed as parameter with -I) file to the generated signed package file, and of course signs it too.

The file is added as META-INF/ids.json.

I'll get with the code to actually use that now :)

Comment 24

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: WIP, part 2. Signed and identified package installation

This is the (still incomplete and untested, just for general feedback!) second part of the patch, the actual package installation. There are some things still unfinished, will try to finish them later today.

Comment 25

[Antonio Manuel Amaya Calvo (:amac)]

Ok, I uploaded a couple of WIP patches, let me know if that's something like what you had in mind or not.

Comment 26

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: WIP, part 2 V2. Installs applications correctly, adds storeID and storeVersion

This version installs the app correctly with an ids.json. In fact it requires ids.json to be present on signed files. 

So far it just verifies that if there's an app with the same storeId then the version we have is lower than the one in the manifest to allow installation, and that if we have an app with the same manifest origin, the storeId is the same also (although that checks never come out since currently we can't reinstall from the same origin).

I still don't know if that's all you wanted from this or not.

Not putting it up for review until I know if this is what was required

Comment 27

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 706777 [details] [diff] [review]
WIP, part 2 V2. Installs applications correctly, adds storeID and storeVersion

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

::: dom/apps/src/Webapps.jsm
@@ +1679,5 @@
>      let app = aData.app;
>      app.removable = true;
>  
> +    let storeId = app.storeId;
> +    let storeVersion = app.storeVersion || 0;

Isn't this information coming from the mini-manifest at this point?

If so, I think we should avoid putting this information in the mini-manifest, and we should avoid doing these checks here. Instead, we should do the check based solely on the information in META-INF/ids.json after we've verified the signature of the app.

@@ +2191,5 @@
>                    throw "INSTALL_FROM_DENIED";
>                  }
>  
> +
> +                // Bug 814136. Get ids.json if the file is signed

Nit: no need to reference bug numbers. We just use blame/annotate for that.

@@ +2193,5 @@
>  
> +
> +                // Bug 814136. Get ids.json if the file is signed
> +                if (isSigned) {
> +                    // Should this be mandatory for signed apps?

Yes, this should be mandatory. And, I think it is OK to remove the zipReader.hasEntry check, since getInputStream will throw an exception if the entry isn't found. We already have to deal with exceptions from failing to parse the ids.json file anyway.

@@ +2208,5 @@
> +                    app.storeId = aApp.installOrigin + "#" + ids.id;
> +                    app.storeVersion = ids.version;
> +                    debug("app.storeId: "+ app.storeId);
> +
> +                    // Do this app exist already?

I think instead saying "Is the app we just downloaded already installed?" we should be asking "Is the app we just downloaded the same app that we're trying to update." In other words, before we get to this point we should have stored (installedAppStoreId, installedAppStoreVersion) of the existing version of the app being updated somewhere, so that we can do a check like this:

    if (installedAppStoreId !== app.storeId) {
        throw "UPDATE_DENIED_INVALID_STORE_ID";
    }
    if (!app.storeVersion || 
        installedAppStoreVersion < app.storeVersion) {
        throw "MORE_MODERN_VERSION_INSTALLED";   
    }

The thing I'm not sure about is how we should pass (installedAppStoreId, installedAppStoreVersion) into this function, because the data flow in dom/apps is not so clear to me.

Nit: I suggest throwing "WRONG_APP_STORE_ID" "APP_STORE_VERSION_ROLLBACK" instead.

Comment 28

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 705833 [details] [diff] [review]
WIP, part1. Signed package generator example

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

::: security/manager/ssl/tests/unit/test_signed_apps/sign_b2g_app.py
@@ +31,5 @@
>      return nss_ctypes.SEC_PKCS7Encode(p7, None, wincx)
>    finally:
>      nss_ctypes.SEC_PKCS7DestroyContentInfo(p7)
>  
> +# Bug 814136, we receive a ids_json file for the toBeSigned app

Nit: no need to reference bug numbers, since we can use blame/annocate to get it.

@@ +96,5 @@
>            # Add the entry to the manifest we're building
>            mf_entries.append('Name: %s\nSHA1-Digest: %s\n'
>                                  % (name, b64encode(sha1(contents).digest())))
> +    if (ids_json):
> +      ids_contents = ids_json.read(max_entry_uncompressed_len)

I think we should read the entire file, and then fail if the file was longer than max_entry_uncompressed_len. It's basically impossible but it makes the code more clearly correct.

Nit: It would be clearer to factor out the out_zip.writestr() and mf_entries.append() calls above into a function that gets reused here.

@@ +142,5 @@
>    cert_nickname = args.k
>  
>    (wincx, cert) = nss_load_cert(db_dir, password, cert_nickname)
>    try:
> +    sign_zip(args.i, args.o, cert, wincx, args.I)

This is reasonable.

It might be easier for the user (of this tool if you just passed in the ID and version as separate string parameters, and then had sign_b2g_app.json format the JSON itself.

This would also make the program good documentation for people reading it, trying to figure out what ids.json is supposed to contain.

Comment 29

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Brian Smith (:bsmith) from comment #27)
> Comment on attachment 706777 [details] [diff] [review]
> WIP, part 2 V2. Installs applications correctly, adds storeID and
> storeVersion
> 
> Review of attachment 706777 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/apps/src/Webapps.jsm
> @@ +1679,5 @@
> >      let app = aData.app;
> >      app.removable = true;
> >  
> > +    let storeId = app.storeId;
> > +    let storeVersion = app.storeVersion || 0;
> 
> Isn't this information coming from the mini-manifest at this point?

Hmm no, app contains the information we've read from the package and it's about to be stored on the minimanifest (assuming the minimanifest is what's on webapps.json). 


> 
> If so, I think we should avoid putting this information in the
> mini-manifest, and we should avoid doing these checks here. Instead, we
> should do the check based solely on the information in META-INF/ids.json
> after we've verified the signature of the app.

What these checks do is verify that the storeId (using the manifestURL as index) of the previously stored app (if it exists) is the same as the storeId that's on the app variable, which contains what we've just read from the network. And we have to store that info (assuming the checks are correct and either the app is new or the id hasn't changed) on the minimanifest cause otherwise it's lost. 

Or at least I think it works that way :). BTW most of this is a moot point anyway since currently reinstalls are disabled. 

> 
> @@ +2191,5 @@
> >                    throw "INSTALL_FROM_DENIED";
> >                  }
> >  
> > +
> > +                // Bug 814136. Get ids.json if the file is signed
> 
> Nit: no need to reference bug numbers. We just use blame/annotate for that.
> 
> @@ +2193,5 @@
> >  
> > +
> > +                // Bug 814136. Get ids.json if the file is signed
> > +                if (isSigned) {
> > +                    // Should this be mandatory for signed apps?
> 
> Yes, this should be mandatory. And, I think it is OK to remove the
> zipReader.hasEntry check, since getInputStream will throw an exception if
> the entry isn't found. We already have to deal with exceptions from failing
> to parse the ids.json file anyway.

Ok. This way I could launch a prettier exception but will change it.

> 
> @@ +2208,5 @@
> > +                    app.storeId = aApp.installOrigin + "#" + ids.id;
> > +                    app.storeVersion = ids.version;
> > +                    debug("app.storeId: "+ app.storeId);
> > +
> > +                    // Do this app exist already?
> 
> I think instead saying "Is the app we just downloaded already installed?" we
> should be asking "Is the app we just downloaded the same app that we're
> trying to update." In other words, before we get to this point we should
> have stored (installedAppStoreId, installedAppStoreVersion) of the existing
> version of the app being updated somewhere, so that we can do a check like
> this:
> 
>     if (installedAppStoreId !== app.storeId) {
>         throw "UPDATE_DENIED_INVALID_STORE_ID";
>     }
>     if (!app.storeVersion || 
>         installedAppStoreVersion < app.storeVersion) {
>         throw "MORE_MODERN_VERSION_INSTALLED";   
>     }
> 
> The thing I'm not sure about is how we should pass (installedAppStoreId,
> installedAppStoreVersion) into this function, because the data flow in
> dom/apps is not so clear to me.

I think I can reorganize this to move both checks into confirmInstall. As I was doing this now I was using storeId as index here, and manifestURL as index on confirmInstall. If I move the version check into confirmInstall I don't even need (so far) the getAppLocalIdByStoreId function. Although to be honest, going forward I think it would be better to use the storeId as application identifier than the manifestURL. 

But for the time being I think that'll make it clearer.

> 
> Nit: I suggest throwing "WRONG_APP_STORE_ID" "APP_STORE_VERSION_ROLLBACK"
> instead.

Thanks for the feedback. I'll try to get a new version up sometime tomorrow (or considering it's 5AM here, probably later today :) )

Comment 30

[[:fabrice] Fabrice Desré]

You can use aIsUpdate in downloadPackage() to know if we're downloading an update or not. I would rather keep this code there rather than move something to confirmInstall().

The version number is *not* mandatory in the update (aka mini) manifests, so you can't read it from there - but you're not doing that anyway, you're just storing it on the app object we serialize, which is fine.

Last point I wonder about is whether the version number is an integer or can take the usual x.y(.z) form. If it's the former, you should use nsIVersionComparator to do the version number check.

Comment 31

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Fabrice Desré [:fabrice] from comment #30)
> Last point I wonder about is whether the version number is an integer or can
> take the usual x.y(.z) form. If it's the former, you should use
> nsIVersionComparator to do the version number check.

It is just an integer.

Comment 32

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 1. Signed Package generator example

I think this is as final as final does. It includes bsmith's feedback comments. 
I added two ways of including the store id information:
-I path to the ids.json file 
OR
-S storeID -V version

Those options are exclusive but one set of them must be specified.

Comment 33

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part2 V1. Includes comments from the previous feedback

I think this is close to a final version. I added all the needed code to downloadPackage as Fabrice suggested. I think it includes all comments from the previous feedback, except I didn't take out the 
if (!zipReader.hasEntry("META-INF/ids.json")) {
  throw "MISSING_IDS_JSON";
} 
Because I like having explicit errors whenever possible. That said, the WRONG_STORE_ID exception currently includes two different errors I would like it better if we launched different errors (makes it easier to debug if nothing else).

Comment 34

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 706874 [details] [diff] [review]
Part 1. Signed Package generator example

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

::: security/manager/ssl/tests/unit/test_signed_apps/sign_b2g_app.py
@@ +135,5 @@
>    parser.add_argument('-o', action='store', type=argparse.FileType('wb'),
>                              required=True, help="output JAR file (signed)")
> +  parser.add_argument('-I', '--ids-file', action='store', type=argparse.FileType('rb'),
> +                     help="Path to the ids.json file", dest='I')
> +  parser.add_argument('-S', '--storeId', action='store', 

Nit: trailing whitespace.

Comment 35

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 706875 [details] [diff] [review]
Part2 V1. Includes comments from the previous feedback

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

Please keep in mind that I know absolutely nothing about Javascript.

::: dom/apps/src/AppsUtils.jsm
@@ +125,5 @@
> +
> +    return Ci.nsIScriptSecurityManager.NO_APP_ID;
> +  },
> +
> +  getAppStoreVersionByLocalId: function(aApps,aLocalId) {

This function is now dead code and should be removed, right?

::: dom/apps/src/Webapps.jsm
@@ +2010,5 @@
>        self._saveApps();
>        AppDownloadManager.remove(aApp.manifestURL);
>      }
>  
> +    function checkStoreId(aIsUpdate, aStoreId, aStoreVersion) {

I suggest renaming this checkForStoreIdMatch because (as explained below), some of the store ID checking should be done elsewhere.

Also, document the preconditions for this function:

* aStoreId must be a string of the form <installOrigin>#<storeId from ids.json>

* aStoreVersion must be a positive integer.

@@ +2022,5 @@
> +      // 2. else
> +      //   a. We should not have this storeId on the list 
> +      // We're currently launching WRONG_APP_STORE_ID for all the mismatch kind of
> +      // errors, and APP_STORE_VERSION_ROLLBACK for the version error.
> +      // Should we separate WRONG_APP_STORE_ID into three different errors instead?

It seems like a good idea to do that, but it isn't necessary.

@@ +2028,5 @@
> +      // Does an app with this storeID exist already?
> +      let appId = AppsUtils.getAppLocalIdByStoreId(self.webapps, aStoreId);
> +      let isInstalled = appId != Ci.nsIScriptSecurityManager.NO_APP_ID;
> +      if (aIsUpdate) {
> +        // Should we customize the errors to include the storeId?

The formatting of errors happens in Gaia. I think that to get better error messages, we would need to change the error handling interface so that Gaia receives a (JSON) structured object instead of just the error code. But, that's too big of a change to do now.

@@ +2035,5 @@
> +          // cannot be an update
> +          throw "WRONG_APP_STORE_ID";
> +        }
> +        // If version goes to a string (x.y.z) should use 
> +        // nsIVersionComparator instead.

Please remove this comment because we already decided not to support the x.y.z kind of versions.

@@ +2036,5 @@
> +          throw "WRONG_APP_STORE_ID";
> +        }
> +        // If version goes to a string (x.y.z) should use 
> +        // nsIVersionComparator instead.
> +        if (app.storeVersion >= aStoreVersion) {

AFAICT, this will not handle an undefined aStoreVersion correctly (1 > undefined == false in Javascript). I think the JSON parser will give us undefined if there was no version attribute in the JSON.

@@ +2263,5 @@
> +                if (isSigned) {
> +                    if (!zipReader.hasEntry("META-INF/ids.json")) {
> +                        throw "MISSING_IDS_JSON";
> +                    } 
> +                    let idsStream = zipReader.getInputStream("META-INF/ids.json");

Nit: I would write this as:

let idsStream;
try {
   idsStream = zipReader.getInputStream(...);
} catch (e) {
   throw zipReader.hasEntry(...) ? e : "MISSING_IDS_JSON";
}

To avoid doing two lookups in the common case where the archive is valid.

@@ +2268,5 @@
> +                    let ids = 
> +                       JSON.parse(converter.ConvertToUnicode(
> +                         NetUtil.readInputStreamToString(idsStream,
> +                                                         idsStream.available()) || ""));
> +                    let storeId = aApp.installOrigin + "#" + ids.id;

We need to validate that ids.id is a non-empty string before we do this.

I understand that you are trying to take the "relative" store ID in the archive and convert it to an "absolute" store ID, so that there are no collisions between multiple app stores.

But, we didn't really decide whether the scope of the identifiers is "per installOrigin" or "per trust anchor." The punting on this decision is one of the main reasons B2G 1.0 has restricted privileged apps to just the Firefox Marketplace.

So, given all that, really anything is OK for B2G 1.0, including just comparing the "relative" ids (since they must have all come from the same app store).

@@ +2269,5 @@
> +                       JSON.parse(converter.ConvertToUnicode(
> +                         NetUtil.readInputStreamToString(idsStream,
> +                                                         idsStream.available()) || ""));
> +                    let storeId = aApp.installOrigin + "#" + ids.id;
> +                    let storeVersion = ids.version;

We need to check, at least, that ids.version is a positive integer and not undefined or NaN or whatever craziness that JS has.

Comment 36

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 1. Includes whitespace fixes

Carrying r+ from previous version, this one only fixes the trailing whitespace.

Comment 37

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 2 V2. Includes comments from the previous review

I think this takes care of all of bsmith review's comments.

Comment 38

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Brian Smith (:bsmith) from comment #35)
> Comment on attachment 706875 [details] [diff] [review]
> Part2 V1. Includes comments from the previous feedback
> 
> Review of attachment 706875 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please keep in mind that I know absolutely nothing about Javascript.

That about sums up my knowledge of Python :). Thanks for the review.

Comment 39

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 706912 [details] [diff] [review]
Part 2 V2. Includes comments from the previous review

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

Nit: Check splinter to see all the trailing whitespace on the lines you added.

I am not a DOM peer so Fabrice will need to r+ this before we can check it in.

::: dom/apps/src/Webapps.jsm
@@ +2273,5 @@
> +                        NetUtil.readInputStreamToString(
> +                          idsStream, idsStream.available()) || ""));
> +                  if ((!ids.id) || isNaN(ids.version) ||
> +                     (Math.abs(ids.version) !== ids.version) ||
> +                     (Math.floor(ids.version) !== ids.version)) {

<bsmith> Given x, which is some Javascript value, what is the simplest correct way to determine if x is a positive integer (considering undefined, NaN, non-integer numbers, and non-number values)?

<benjamin> bsmith: Number.isInteger(x) && x > 0

Comment 40

[Carmen Jimenez Cabezas]

(In reply to Brian Smith (:bsmith) from comment #39)
> Comment on attachment 706912 [details] [diff] [review]
> Part 2 V2. Includes comments from the previous review
> 
> Review of attachment 706912 [details] [diff] [review]:
> 
> <benjamin> bsmith: Number.isInteger(x) && x > 0

I saw this method but on [1] it's marked as 'experimental' so I went with the old and proved :). If it's ok to use it anyway I'll change it and upload a new patch (with the spaces fixed too) after Fabrice reviews this.

[1] https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Number

Comment 41

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think using isInteger is fine. Especially if you write tests which verify that we reject updates as expected, which we should do anyway ;-)

Comment 42

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Carmen Jiménez Cabezas from comment #40)
> (In reply to Brian Smith (:bsmith) from comment #39)
> > Comment on attachment 706912 [details] [diff] [review]
> > Part 2 V2. Includes comments from the previous review
> > 
> > Review of attachment 706912 [details] [diff] [review]:
> > 
> > <benjamin> bsmith: Number.isInteger(x) && x > 0
> 
> I saw this method but on [1] it's marked as 'experimental' so I went with
> the old and proved :). If it's ok to use it anyway I'll change it and upload
> a new patch (with the spaces fixed too) after Fabrice reviews this.

Sharing computers, the remember me option and my epic level of absent-mindness don't mix well. This was all me, sorry for the mixup.

Comment 43

[[:fabrice] Fabrice Desré]

Comment on attachment 706912 [details] [diff] [review]
Part 2 V2. Includes comments from the previous review

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

::: dom/apps/src/AppsUtils.jsm
@@ +114,5 @@
>  
>      return Ci.nsIScriptSecurityManager.NO_APP_ID;
>    },
>  
> +  getAppLocalIdByStoreId: function(aApps,aStoreId) {

nit: space after the comma.

::: dom/apps/src/Webapps.jsm
@@ +2013,5 @@
>  
> +    // aStoreId must be a string of the form 
> +    //   <installOrigin>#<storeId from ids.json>
> +    // aStoreVersion must be a positive integer.
> +    function checkForStoreIdMatch(aIsUpdate, aStoreId, aStoreVersion) {

No need to pass the aIsUpdate parameter here, it's already in scope from the outer function.

@@ +2027,5 @@
> +      // We're currently launching WRONG_APP_STORE_ID for all the mismatch kind of
> +      // errors, and APP_STORE_VERSION_ROLLBACK for the version error.
> +    
> +      // Does an app with this storeID exist already?
> +      let appId = AppsUtils.getAppLocalIdByStoreId(self.webapps, aStoreId);

Please implement a proper getAppLocalIdByStoreId(aStoreId) method in this file like for the other methods of nsIAppsService.idl

nit: there are some trailing whitespace in the comments.

@@ +2263,5 @@
> +                  let idsStream;
> +                  try {
> +                    idsStream = zipReader.getInputStream("META-INF/ids.json");
> +                  } catch (e) {
> +                    throw zipReader.hasEntry("META-INF/ids.json") ? 

Nit: trailing whitespace. Also, align ? and :

::: dom/interfaces/apps/nsIAppsService.idl
@@ +63,5 @@
> +
> +  /**
> +   * Returns the storeID if the app was installed from a store
> +   */
> +  DOMString getAppLocalIdByStoreId(in DOMString storeID);

You did not implement this method in AppsService.js

Comment 44

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Fabrice Desré [:fabrice] from comment #43)
> Comment on attachment 706912 [details] [diff] [review]
> Part 2 V2. Includes comments from the previous review
> 
> Review of attachment 706912 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > +                  try {
> > +                    idsStream = zipReader.getInputStream("META-INF/ids.json");
> > +                  } catch (e) {
> > +                    throw zipReader.hasEntry("META-INF/ids.json") ? 
> 
> Nit: trailing whitespace. Also, align ? and :

I will, although that'll break the 80 characters limit.

> 
> ::: dom/interfaces/apps/nsIAppsService.idl
> @@ +63,5 @@
> > +
> > +  /**
> > +   * Returns the storeID if the app was installed from a store
> > +   */
> > +  DOMString getAppLocalIdByStoreId(in DOMString storeID);
> 
> You did not implement this method in AppsService.js

Ugh! I had in fact... once upon a time. Must have perished on one of the moves from m-c to b2g18 or back. 

Thanks for the review! I'll upload a new patch once the patch for 836045 lands, because as it stands I can't test it (been making myself crazy trying to find why it wasn't working now... until I remembered the device storage changes)

Comment 45

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 2 V3. Includes comments from the previous review

Comment 46

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 2 V3. Includes comments from the previous review and space fixes

This includes all the changes requested, I think

Comment 47

[[:fabrice] Fabrice Desré]

Comment on attachment 708292 [details] [diff] [review]
Part 2 V3. Includes comments from the previous review and space fixes

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

That's mostly ready!

::: dom/apps/src/Webapps.jsm
@@ +2265,5 @@
> +                    idsStream = zipReader.getInputStream("META-INF/ids.json");
> +                  } catch (e) {
> +                    throw zipReader.hasEntry("META-INF/ids.json") ?
> +                                                                e :
> +                                                                "MISSING_IDS_JSON";

what about formatting as :
throw zipReader.hasEntry("META-INF/ids.json") 
      ? e
      : "MISSING_IDS_JSON";

@@ +2274,5 @@
> +                        NetUtil.readInputStreamToString(
> +                          idsStream, idsStream.available()) || ""));
> +                  if ((!ids.id) || isNaN(ids.version) ||
> +                     (Math.abs(ids.version) !== ids.version) ||
> +                     (Math.floor(ids.version) !== ids.version)) {

Why did you not update that to use isInteger() ?

::: dom/interfaces/apps/mozIApplication.idl
@@ +38,5 @@
> +  /* Store ID if the app is installed from a store */
> +  readonly attribute DOMString storeID;
> +
> +  /* Store version if the app is installed from a store */
> +  readonly attribute unsigned long version;

That should be storeVersion, right?

Comment 48

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 2 V4, with review comments.

This time with the correct integer verification. To answer your question, I didn't use it... cause I totally forgot! Duh me.

Comment 49

[Antonio Manuel Amaya Calvo (:amac)]

Is this still something we want for v1.1? It's marked as tracking b2g18 but the patch has been unmolested for more than two months now. I'm asking now because I've just noticed that this appears as tracking.

If we want to get this done still I can rebase the patch and resubmit it again... otherwise we should just remove the flag.

Comment 50

[[:fabrice] Fabrice Desré]

Oh, my bad for not finishing this review. I'll do it asap.

Comment 51

[[:fabrice] Fabrice Desré]

Comment on attachment 708808 [details] [diff] [review]
Part 2 V4, with review comments.

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

r=me with nits addressed

::: dom/apps/src/AppsUtils.jsm
@@ +64,5 @@
>        installerAppId: aApp.installerAppId || Ci.nsIScriptSecurityManager.NO_APP_ID,
> +      installerIsBrowser: !!aApp.installerIsBrowser,
> +      storeId: aApp.storeId || "",
> +      storeVersion: aApp.storeVersion || 0
> +

Nit: extra blank line

::: dom/interfaces/apps/nsIAppsService.idl
@@ +61,5 @@
>     */
>    DOMString getWebAppsBasePath();
> +
> +  /**
> +   * Returns the storeID if the app was installed from a store

This should be "Returns the localId..."

Comment 52

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 2, V5. Fixed the nits, and rebased to apply cleanly on M-C

Keeping r+ from V4 (r=fabrice).

Comment 53

[Ryan VanderMeulen [:RyanVM]]

Happy to push this, but the commit messages as-given are next to useless. Please use commit messages that describe what the patch is actually doing, not just re-stating the bug title and putting P1/P2 on the end. You don't need to attach new patches, just comment here with what they should be.

Comment 54

[Antonio Manuel Amaya Calvo (:amac)]

Ok, sorry about that. 
P1: Change the signing test scripts to include a version number and a generated or provided package identifier.
P2: Changes on the package installation code to check that a version number and package identifier are provided for signed apps.

Something like that?

Comment 55

[Ryan VanderMeulen [:RyanVM]]

Much better, thanks :-)

https://hg.mozilla.org/projects/birch/rev/63ad3de4e23a
https://hg.mozilla.org/projects/birch/rev/8e1a8611f297

Comment 56

[Antonio Manuel Amaya Calvo (:amac)]

A word of caution, when this lands the signed apps will *require* the new fields or they won't install at all. Signed apps with the new fields install just fine on the old code though (so a signed app that includes the app-uid and version number on the signed package will install correctly on 1.0.1).

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/63ad3de4e23a
https://hg.mozilla.org/mozilla-central/rev/8e1a8611f297

Comment 58

[Antonio Manuel Amaya Calvo (:amac)]

What does this need to land on b2g18? I thought it had all the needed flags but obviously I was wrong :)

Comment 59

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Antonio Manuel Amaya Calvo from comment #58)
> What does this need to land on b2g18? I thought it had all the needed flags
> but obviously I was wrong :)

Click the "details" link for the patches and set the approval‑mozilla‑b2g18 flag. You will get a little questionnaire. Fill out that questionnaire and wait for release drivers to approve it. And/or, set the blocking-b2g: tef+ flag to force the issue, if you think it is critical to ship this.

Comment 60

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 706911 [details] [diff] [review]
Part 1. Includes whitespace fixes

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Adding the ability to add a ids.json with ID and version number to signed apps. 
User impact if declined: Currently there's no store provided ID for packages.
Testing completed: This patch implements the tests for the second part, actually.
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: None

Comment 61

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 740742 [details] [diff] [review]
Part 2, V5. Fixed the nits, and rebased to apply cleanly on M-C

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Currently there's no way for stores to identify a package (the package is identified only by the manifestURL). This patch introduces a way to add a store defined UUID and version number to a signed package.
User impact if declined: 
Testing completed: P1 are the tests for this patch. 
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: No strings changed, changed the UUID for mozIApplication

Comment 62

[Andrew Overholt [:overholt]]

With the flags as they are now, these patches will not be in v1.0.1 but will be in v1.1.  If that's not what you expect, please set blocking-b2g to tef? and provide some comments as to why this should hold the release.  Thanks.

Comment 63

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/7e505d8b92d7
https://hg.mozilla.org/releases/mozilla-b2g18/rev/2164c4e5ecf4

Comment 64

[Bill Walker [:bwalker] [@wfwalker]]

(In reply to Antonio Manuel Amaya Calvo from comment #56)
> A word of caution, when this lands the signed apps will *require* the new
> fields or they won't install at all. Signed apps with the new fields install
> just fine on the old code though (so a signed app that includes the app-uid
> and version number on the signed package will install correctly on 1.0.1).

Antonio, did anyone notify the Firefox Marketplace team about this? It has big implications for them.

Comment 65

[Antonio Manuel Amaya Calvo (:amac)]

Not me :). I've seen there are a couple of bugs where this is being treated though. Bug 871035 is one of them.