1029691 - Manifest not being checked on APK installation

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P1)

Description

[Martyn Haigh (:mhaigh)]

When installing a webapp on desktop the manifest is first downloaded, then various checks are carried out including receipts, install from denied location, manifest validity, reinstallation etc

It seems that the flow of the APK installation has become confused and in the process we're not validating the manifest.  We currently download the APK and then download the manifest.  We should be following the original flow of downloading and checking the manifest before downing the APK.

Comment 1

[Martyn Haigh (:mhaigh)]

Attachment: Download and verify manifest before APK file

Changed the flow of the APK installation process slightly so that we download and verify the manifest before attempting to download and install the APK

Comment 2

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

Comment on attachment 8447399 [details] [diff] [review]
Download and verify manifest before APK file

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

This is heading in the right direction!  But if we delay installation until the installApp function inside doInstall, then we no longer need the webapps-runtime-install and webapps-runtime-install-package notifications (and their preprocessor branches) at all.

We need only move the WebappManager code that handles those messages to a webapps-ask-install handler.  We'll just have to be careful to audit the existing paths that can lead to that handler (like autoInstall) and modify them accordingly.

Comment 3

[Martyn Haigh (:mhaigh)]

Moving the contents of the 'install' and 'installPackage' functions within WebappManager.jsm to the 'askInstall' functions feels like we're giving that function too many responsibilities.

Comment 4

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

When a page requests app installation, the desktop runtime does:

  DOMApplicationRegistry.doInstall/Package
  "webapps-ask-install"
  WebappManager.doInstall
  prompt user to confirm install
  DOMApplicationRegistry.confirmInstall

And the FxOS runtime does something similar.

But our runtime does:

  "webapps-runtime-install/-package"
  WebappManager.install/Package
  natively prompt user to confirm APK install
  WebappManager.autoInstall
  DOMApplicationRegistry.doInstall/Package
  "webapps-ask-install"
  WebappManager.askInstall
  DOMApplicationRegistry.confirmInstall

So "askInstall" is a misnomer in our case, as it doesn't prompt the user to install the app, which has already been installed.  In fact, almost all it does is call confirmInstall.  It's a pass-through no-op.

I think our runtime should do:

  DOMApplicationRegistry.doInstall/Package
  "webapps-ask-install"
  WebappManager.askInstall
  WebappManager.install/Package
  natively prompt user to confirm APK install
  WebappManager.autoInstall
  DOMApplicationRegistry.checkInstall/Package
  DOMApplicationRegistry.confirmInstall

Or, in the case of a sideloaded app:

  WebappManager.autoInstall
  DOMApplicationRegistry.checkInstall/Package
  DOMApplicationRegistry.confirmInstall

Where DOMApplicationRegistry.checkInstall/Package are new functions that factor out the validation code from doInstall/Package, so the installation flow runs the same checks both when a page requests app installation (on the original app manifest) and after installing the APK (on the manifest in the package).

Comment 5

[Martyn Haigh (:mhaigh)]

Attachment: Download and verify manifest before APK file

Refactored code as suggested

Comment 6

[Martyn Haigh (:mhaigh)]

https://tbpl.mozilla.org/?tree=Try&rev=b2e2a1c39a1b

Comment 7

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

Comment on attachment 8458641 [details] [diff] [review]
Download and verify manifest before APK file

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

::: dom/apps/src/Webapps.jsm
@@ +2078,5 @@
> +    xhr.addEventListener("load", (function() {
> +      if (xhr.status == 200) {
> +        if (AppsUtils.checkManifestContentType(app.installOrigin, app.origin,
> +                                                xhr.getResponseHeader("content-type"))) {
> +          deferred.resolve(xhr);

Here and in downloadUpdateManifest, instead of resolving to the entire XHR, resolve to a tuple of [manifest, etag] that the caller can destructure.

@@ +2098,2 @@
>  
> +  sendError: function(aMm, aError, aData, aReturnValue, aMessage) {

Nit: there are a bunch of functions in this script with aMm and aData parameters in the order aData, aMm; so in the interest of consistency, make these the first two parameters to sendError, and put aData first.

Also, make this an internal function _sendError, as it doesn't need to be exposed outside DOMApplicationRegistry.

@@ +2110,5 @@
> +    let sendInstallError = function(aError) {
> +      this.sendError(aMm, aError, aData, "Webapps:Install:Return:KO",
> +                     "Error installing app from: " + app.installOrigin +
> +                     ": " + aError);
> +    }.bind(this);

I understand that the sendInstallError function enables this code to log the exact same error as before this change, but it also means passing a function around, which I'd like to avoid doing, as the whole purpose of defining a top-level sendError function is to avoid that.

And changing the message isn't a functional change.  So let's have sendError log a more generic message and call it directly from all the call sites instead of passing sendInstallError around.  The message can be:

  Cu.reportError("Error: " + aError + " (aReturnValue)");

@@ +2144,3 @@
>  
> +
> +  checkPackageManifest: function(aApp, aSendError) {

Nit: this might be confused for a function that checks the manifest inside the package, whereas it actually checks the "update manifest" that is downloaded by downloadUpdateManifest; so I would call this checkUpdateManifest.

Comment 8

[Martyn Haigh (:mhaigh)]

Attachment: Implement Android Log debug method which only prints out when Fennec is debuggable

Comment 9

[Martyn Haigh (:mhaigh)]

Have adjusted based on feedback but I wasn't sure of the best way of passing a lot of params around for the _sendError function.  Would welcome some feedback.

Comment 10

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

Comment on attachment 8460987 [details] [diff] [review]
Implement Android Log debug method which only prints out when Fennec is debuggable

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

::: dom/apps/src/Webapps.jsm
@@ +1649,5 @@
>      let app = this.webapps[id];
>  
>      // We cannot update an app that does not exists.
>      if (!app) {
> +      this._sendError(errorParams, "NO_SUCH_APP");

I would do:

  this._sendError(aData, aMm, "Webapps:CheckForUpdate:Return:KO", "NO_SUCH_APP");

Or, if you want to avoid repeating the literal string:

  const ERROR_RETURN_VALUE = "Webapps:CheckForUpdate:Return:KO";
  …
  this._sendError(aData, aMm, ERROR_RETURN_VALUE, "NO_SUCH_APP");

@@ +2099,2 @@
>  
> +  _sendError: function([aData, aMm, aReturnValue], aError) {

The first parameter of nsIMessageManager is called aMessageName:

http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIMessageManager.idl?rev=d62df3aa085b#274

So I would call this the same.

Comment 11

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

(In reply to Myk Melez [:myk] [@mykmelez] from comment #10)
> > +  _sendError: function([aData, aMm, aReturnValue], aError) {
> 
> The first parameter of nsIMessageManager is called aMessageName:
> 
> http://mxr.mozilla.org/mozilla-central/source/content/base/public/
> nsIMessageManager.idl?rev=d62df3aa085b#274
> 
> So I would call this the same.

Erm, to clarify: I was referring to the aReturnValue parameter.  I would call it aMessageName (and, if you put it into a constant, I would call that ERROR_MESSAGE_NAME).

Comment 12

[Martyn Haigh (:mhaigh)]

Attachment: Download and verify manifest before APK file

I've removed the destructuring assignment from the _sendError function and adjusted all the calls.  Also changed browser.js as it wasn't sending through the message manager before

https://tbpl.mozilla.org/?tree=Try&rev=ec71de0dd0f5

Comment 13

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

Comment on attachment 8465522 [details] [diff] [review]
Download and verify manifest before APK file

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

::: dom/apps/src/Webapps.jsm
@@ +2106,4 @@
>      delete aApp.manifest;
>    }),
>  
> +  checkManifest: function(aData, aMm, aMessageName, aApp) {

Erm, aMessageName is a good name for the _sendError parameter, since it's obvious that it's an error message name.  But that isn't clear here, in checkManifest.  So I would call this parameter something like aErrorMessageName, i.e.:

 checkManifest: function(aData, aMm, aErrorMessageName, aApp) {

Comment 14

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

As Martyn is off working on another project now, I'll take over finishing up and landing this patch.

Comment 15

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

Per bug 1235869, we're going to disable the Android web runtime, so we won't fix this bug in it.

(This is part of a bulk resolution of bugs in the Firefox for Android::Web Apps component, from which I attempted to exclude bugs that are not specific to the runtime, but it's possible that I included one accidentally.  If so, I'm sorry, and please reopen the bug!)