The default bug view has changed. See this FAQ.

Implement trusted/certified app scheme support

RESOLVED FIXED in mozilla16

Status

()

Core
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ladamski, Assigned: fabrice)

Tracking

unspecified
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-kilimanjaro:+, blocking-basecamp:+)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

5 years ago
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'.
(Reporter)

Updated

5 years ago
Depends on: 769280
(Assignee)

Updated

5 years ago
Assignee: nobody → fabrice
(Reporter)

Updated

5 years ago
Duplicate of this bug: 769280
(Reporter)

Updated

5 years ago
blocking-basecamp: --- → ?
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
Created attachment 639854 [details] [diff] [review]
Part 1 : installing apps from .zip packages
Attachment #639503 - Attachment is obsolete: true
Attachment #639854 - Flags: review?(21)
(Assignee)

Comment 5

5 years ago
Created attachment 639855 [details] [diff] [review]
Part 2 : app:// protocol handler
Attachment #639508 - Attachment is obsolete: true
Attachment #639855 - Flags: review?(21)
(Assignee)

Comment 6

5 years ago
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: --- → +

Comment 7

5 years ago
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-
Depends on: 769568
(Assignee)

Comment 11

5 years ago
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)
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.
(Assignee)

Comment 13

5 years ago
(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.
(Assignee)

Comment 16

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3ada8025e75
https://hg.mozilla.org/mozilla-central/rev/f3ada8025e75
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Is there anything to verify here from an end-user perspective?
Whiteboard: [qa?]
(Assignee)

Comment 19

5 years ago
(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.

Updated

5 years ago
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?
Depends on: 773884
Depends on: 773886
Blocks: 773894
Depends on: 773898
(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.

Updated

5 years ago
Whiteboard: [qa verification blocked] → [qa+]

Updated

5 years ago
QA Contact: jsmith

Updated

5 years ago
QA Contact: jsmith
Whiteboard: [qa+] → [qa-]
(Reporter)

Updated

5 years ago
No longer depends on: 769568
You need to log in before you can comment on or make changes to this bug.