Last Comment Bug 769350 - Implement trusted/certified app scheme support
: Implement trusted/certified app scheme support
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: [:fabrice] Fabrice Desré
:
:
Mentors:
: 769280 (view as bug list)
Depends on: 769280 773884 773886 773898
Blocks: privileged-apps 773894
  Show dependency treegraph
 
Reported: 2012-06-28 10:36 PDT by Lucas Adamski [:ladamski]
Modified: 2012-08-28 08:59 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+


Attachments
Part 1 : installing apps from .zip packages (15.05 KB, patch)
2012-07-05 16:18 PDT, [:fabrice] Fabrice Desré
no flags Details | Diff | Splinter Review
Part 2 : app:// protocol handler (5.22 KB, patch)
2012-07-05 16:25 PDT, [:fabrice] Fabrice Desré
no flags Details | Diff | Splinter Review
Part 1 : installing apps from .zip packages (15.15 KB, patch)
2012-07-06 16:59 PDT, [:fabrice] Fabrice Desré
21: review-
Details | Diff | Splinter Review
Part 2 : app:// protocol handler (5.36 KB, patch)
2012-07-06 17:00 PDT, [:fabrice] Fabrice Desré
21: review-
Details | Diff | Splinter Review
app:// protocol handler (7.75 KB, patch)
2012-07-10 15:32 PDT, [:fabrice] Fabrice Desré
21: review+
Details | Diff | Splinter Review

Description Lucas Adamski [:ladamski] 2012-06-28 10:36:40 PDT
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 1 Lucas Adamski [:ladamski] 2012-07-04 01:47:08 PDT
*** Bug 769280 has been marked as a duplicate of this bug. ***
Comment 2 [:fabrice] Fabrice Desré 2012-07-05 16:18:10 PDT
Created attachment 639503 [details] [diff] [review]
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é 2012-07-05 16:25:21 PDT
Created attachment 639508 [details] [diff] [review]
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é 2012-07-06 16:59:46 PDT
Created attachment 639854 [details] [diff] [review]
Part 1 : installing apps from .zip packages
Comment 5 [:fabrice] Fabrice Desré 2012-07-06 17:00:22 PDT
Created attachment 639855 [details] [diff] [review]
Part 2 : app:// protocol handler
Comment 6 [:fabrice] Fabrice Desré 2012-07-06 17:09:14 PDT
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 2012-07-08 19:07:15 PDT
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 2012-07-08 21:29:20 PDT
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) 2012-07-09 07:26:19 PDT
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) 2012-07-09 07:52:56 PDT
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é 2012-07-10 15:32:30 PDT
Created attachment 640825 [details] [diff] [review]
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 2012-07-10 21:44:37 PDT
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é 2012-07-11 01:48:21 PDT
(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) 2012-07-11 03:44:22 PDT
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) 2012-07-11 03:45:10 PDT
Fabrice, can you do a pull request with your changes? We will address any review comments on our side.
Comment 16 [:fabrice] Fabrice Desré 2012-07-11 08:39:36 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3ada8025e75
Comment 17 Ed Morley [:emorley] 2012-07-12 09:39:51 PDT
https://hg.mozilla.org/mozilla-central/rev/f3ada8025e75
Comment 18 Jason Smith [:jsmith] 2012-07-12 10:22:51 PDT
Is there anything to verify here from an end-user perspective?
Comment 19 [:fabrice] Fabrice Desré 2012-07-12 10:27:51 PDT
(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?) 2012-07-13 16:17:46 PDT
(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?) 2012-07-13 18:33:04 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.