Closed Bug 769350 Opened 8 years ago Closed 8 years ago

Implement trusted/certified app scheme support

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla16
blocking-kilimanjaro +
blocking-basecamp +

People

(Reporter: ladamski, Assigned: fabrice)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 4 obsolete files)

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'.
Depends on: 769280
Assignee: nobody → fabrice
Duplicate of this bug: 769280
blocking-basecamp: --- → ?
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.
Attached patch Part 2 : app:// protocol handler (obsolete) — Splinter Review
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.
Attachment #639503 - Attachment is obsolete: true
Attachment #639854 - Flags: review?(21)
Attached patch Part 2 : app:// protocol handler (obsolete) — Splinter Review
Attachment #639508 - Attachment is obsolete: true
Attachment #639855 - Flags: review?(21)
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.
blocking-basecamp: ? → +
blocking-kilimanjaro: --- → +
Let's  stick with app://

We need the patch to set the URL right or Jonas' principals work. Jonas?
We can do a bunch of the work without any of the principal work. So no need to wait for that.
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?
Attachment #639854 - Flags: review?(21) → review-
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/'
Attachment #639855 - Flags: review?(21) → review-
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)
Attachment #639854 - Attachment is obsolete: true
Attachment #639855 - Attachment is obsolete: true
Attachment #640825 - Flags: review?(21)
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.
(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 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?
Attachment #640825 - Flags: review?(21) → review+
Fabrice, can you do a pull request with your changes? We will address any review comments on our side.
https://hg.mozilla.org/mozilla-central/rev/f3ada8025e75
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Is there anything to verify here from an end-user perspective?
Whiteboard: [qa?]
(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.
Whiteboard: [qa?] → [qa verification blocked]
(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?
(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.
Whiteboard: [qa verification blocked] → [qa+]
QA Contact: jsmith
QA Contact: jsmith
Whiteboard: [qa+] → [qa-]
No longer depends on: 769568
You need to log in before you can comment on or make changes to this bug.