769350 - Implement trusted/certified app scheme support

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Lucas Adamski [:ladamski]]

Trusted and certified apps will be loaded directly from their container via a new scheme.  

The most obvious scheme naming to use is "app:".  If that's too generic we could go with "wapp:" or "owa:".

The URI for an app would look like "app://myapphost.com/", where myapphost.com points to the domain the app is 'homed'.

Comment 2

[[:fabrice] Fabrice Desré]

Attachment: Part 1 : installing apps from .zip packages

This patch adds support to install apps from a zip package. We expect to find the manifest at the root of the package with the "manifest.webapp" name.

There is no signature verification of any kind done yet.

Comment 3

[[:fabrice] Fabrice Desré]

Attachment: Part 2 : app:// protocol handler

app:// protocol handler that loads resources from installed zip packages.

This patch uses a app://UUID/path/to/file.ext scheme, which means that it's not possible link from an outside page or to share these urls.

We discussed with Jonas the use of app://origin.domain.tld/path/to/file.ext instead, and using the manifest url on the window's docshell to retrieve the appropriate package. We're blocked from doing that by the fact that the homescreen needs to access icons from inside the package while being another app itself. An option would be to use <img src="app://..." fromapp="manifesturl"> but I'm not sure if this would be enough though - front-end dev tend to be creative, and sometime load icon from new Image() or using as a background-image css property.

Comment 4

[[:fabrice] Fabrice Desré]

Attachment: Part 1 : installing apps from .zip packages

Comment 5

[[:fabrice] Fabrice Desré]

Attachment: Part 2 : app:// protocol handler

Comment 6

[[:fabrice] Fabrice Desré]

I made changes to gaia's build system in the packaged-apps branch at https://github.com/fabricedesre/gaia/tree/packaged-apps

I can load the system app, and the lockscreen works, but then we fail to load the homescreen. Not sure yet if this could be a simple gaia fix.

Comment 7

[Andreas Gal :gal]

Let's  stick with app://

We need the patch to set the URL right or Jonas' principals work. Jonas?

Comment 8

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

We can do a bunch of the work without any of the principal work. So no need to wait for that.

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 639854 [details] [diff] [review]
Part 1 : installing apps from .zip packages

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

Just fix the error cases. The rest can be done in followups.

::: dom/apps/src/Webapps.jsm
@@ +218,5 @@
>    denyInstall: function(aData) {
> +    let packageId = aData.app.packageId;
> +    if (packageId) {
> +      let dir = FileUtils.getDir("TmpD", ["webapps", packageId],
> +                                 true, true);

FileUtils.get[Dir/File] where the only difference is packageId and is call in a lot of places. Maybe a helper would be nice.

@@ +261,5 @@
> +      // and delete the temp directory.
> +      if (app.packageId) {
> +        let appFile = FileUtils.getFile("TmpD", ["webapps", app.packageId, "application.zip"], 
> +                                        true, true);
> +        appFile.moveTo(dir, "application.zip");

'application.zip' will prevent you to install 2 packages app at the same time. This can be addressed in a followup.

@@ +358,5 @@
> +    // - add the new app to the registry.
> +    // If we fail at any step, we backout the previous ones and return an error.
> +
> +    let id;
> +    let manifestURL = "jar:" + aData.url + "!manifest.webapp";

So 'manifest.webapp' is the new install.rdf? :)

@@ +412,5 @@
> +      let ostream = FileUtils.openSafeFileOutputStream(zipFile);
> +      NetUtil.asyncCopy(aInput, ostream, function (aResult) {
> +        if (!Components.isSuccessCode(aResult)) {
> +          // We failed to save the zip.
> +          cleanup("NETWORK_ERROR");

This is not really a NETWORK_ERROR

@@ +436,5 @@
> +                        .createInstance(Ci.nsIZipReader);
> +        try {
> +          zipReader.open(zipFile);
> +          if (!zipReader.hasEntry("manifest.webapp")) {
> +            throw "No manifest.webapp found.";

Don't you want to call cleanup("INVALID_MANIFEST") here instead?

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 639855 [details] [diff] [review]
Part 2 : app:// protocol handler

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

::: dom/apps/src/Webapps.jsm
@@ +182,5 @@
>          this.installPackage(msg);
>          break;
> +      case "Webapps:GetBasePath":
> +        return FileUtils.getFile(DIRECTORY_NAME, ["webapps"], true).path;
> +        break;

A new case for an helper! :)

::: netwerk/protocol/app/AppProtocolHandler.js
@@ +29,5 @@
> +
> +  scheme: "app",
> +  defaultPort: -1,
> +  protocolFlags: Ci.nsIProtocolHandler.URI_STD |
> +                 Ci.nsIProtocolHandler.URI_NOAUTH |

Do you need both URI_STD and URI_NOAUTH? Sounds like you want to choose between them.

@@ +31,5 @@
> +  defaultPort: -1,
> +  protocolFlags: Ci.nsIProtocolHandler.URI_STD |
> +                 Ci.nsIProtocolHandler.URI_NOAUTH |
> +                 Ci.nsIProtocolHandler.URI_LOADABLE_BY_ANYONE |
> +                 Ci.nsIProtocolHandler.URI_IS_LOCAL_RESOURCE,

I don't feel comfortable with those flags. Does LOADABLE_BY_ANYONE should not be URI_IS_LOCAL_FILE or URI_DANGEROUS_TO_LOAD?

@@ +41,5 @@
> +
> +    return this._basePath;
> +  },
> +
> +  newURI: function(aSpec, aOriginCharset, aBaseURI) {

nit: add names to your anonymous functions

@@ +52,5 @@
> +
> +  newChannel: function(aURI) {
> +    // We map app://ABCDEF/path/to/file.ext to
> +    // jar:file:///path/to/profile/webapps/ABCDEF/application.zip!/path/to/file.ext
> +    let noScheme = aURI.spec.substring(6);

let filePath = aURI.filePath;

@@ +63,5 @@
> +      appId = noScheme.substring(0, firstSlash);
> +      fileSpec = noScheme.substring(firstSlash);
> +    }
> +
> +    // Simulates default behavior off http servers.

nit: off -> of

@@ +66,5 @@
> +
> +    // Simulates default behavior off http servers.
> +    if (fileSpec == "/") {
> +        fileSpec = "/index.html";
> +    }

This will not work if you have 'app://123456/foo/'

Comment 11

[[:fabrice] Fabrice Desré]

Attachment: app:// protocol handler

I addressed comments on the previous patch and added the following:

- support for #anchors in urls
- fixed same origin issues by adding a setAppURI() method on nsIJARChannel to force the uri of a jar: channel to be the app one.

There are still some issues running gaia, but I think they are on the gaia side (eg, loading webapi.js differently to get mozL10n to work)

Comment 12

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

Viven: Any chance you could try to hunt down the gaia issues mentioned in the previous patch? We'd really like to get this in sometime this week.

Comment 13

[[:fabrice] Fabrice Desré]

(In reply to Jonas Sicking (:sicking) from comment #12)
> Viven: Any chance you could try to hunt down the gaia issues mentioned in
> the previous patch? We'd really like to get this in sometime this week.

I fixed them in https://github.com/fabricedesre/gaia/tree/packaged-apps

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 640825 [details] [diff] [review]
app:// protocol handler

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

::: netwerk/protocol/app/AppProtocolHandler.js
@@ +29,5 @@
> +  defaultPort: -1,
> +  // Using the same flags as the JAR protocol handler.
> +  protocolFlags2: Ci.nsIProtocolHandler.URI_NORELATIVE |
> +                  Ci.nsIProtocolHandler.URI_NOAUTH |
> +                  Ci.nsIProtocolHandler.URI_LOADABLE_BY_ANYONE,

LOADABLE_BY_ANYONE -> DANGEROUS_TO_LOAD?

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Fabrice, can you do a pull request with your changes? We will address any review comments on our side.

Comment 16

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3ada8025e75

Comment 17

[Ed Morley [:emorley]]

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

Comment 18

[Jason Smith [:jsmith]]

Is there anything to verify here from an end-user perspective?

Comment 19

[[:fabrice] Fabrice Desré]

(In reply to Jason Smith [:jsmith] from comment #18)
> Is there anything to verify here from an end-user perspective?

Not before https://github.com/mozilla-b2g/gaia/pull/2330 is merged.

Comment 20

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

(In reply to Vivien Nicolas (:vingtetun) from comment #14)
> Comment on attachment 640825 [details] [diff] [review]
> app:// protocol handler
> 
> Review of attachment 640825 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/app/AppProtocolHandler.js
> @@ +29,5 @@
> > +  defaultPort: -1,
> > +  // Using the same flags as the JAR protocol handler.
> > +  protocolFlags2: Ci.nsIProtocolHandler.URI_NORELATIVE |
> > +                  Ci.nsIProtocolHandler.URI_NOAUTH |
> > +                  Ci.nsIProtocolHandler.URI_LOADABLE_BY_ANYONE,
> 
> LOADABLE_BY_ANYONE -> DANGEROUS_TO_LOAD?

Why wasn't this change made before the patch was checked in?

Also, why URI_NORELATIVE? We need to support relative links, right?

Dan, you are more familiar with the security implications of protocol handler flags than I am. Could you review that part of this (already-checked-in) patch to make sure it is sensible?

Comment 21

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

(In reply to Brian Smith (:bsmith) from comment #20)
> (In reply to Vivien Nicolas (:vingtetun) from comment #14)
> > Comment on attachment 640825 [details] [diff] [review]
> > app:// protocol handler
> > 
> > Review of attachment 640825 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: netwerk/protocol/app/AppProtocolHandler.js
> > @@ +29,5 @@
> > > +  defaultPort: -1,
> > > +  // Using the same flags as the JAR protocol handler.
> > > +  protocolFlags2: Ci.nsIProtocolHandler.URI_NORELATIVE |
> > > +                  Ci.nsIProtocolHandler.URI_NOAUTH |
> > > +                  Ci.nsIProtocolHandler.URI_LOADABLE_BY_ANYONE,
> > 
> > LOADABLE_BY_ANYONE -> DANGEROUS_TO_LOAD?
> 
> Why wasn't this change made before the patch was checked in?

Bug 773886.

> Also, why URI_NORELATIVE? We need to support relative links, right?

Bug 773898.