786710 - The launch_path of a manifest allows for absolute URLs to launch an app at - this should not be allowed

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Jason Smith [:jsmith]]

Example manifest:

{
  "name":"Test App ({subdomain})",
  "description":"This app has been automatically generated by testmanifest.com",
  "version":"1.0",
  "icons":{
    "16":"http://testmanifest.com/icon-16.png",
    "48":"http://testmanifest.com/icon-48.png",
    "128":"http://testmanifest.com/icon-128.png"
  },
  "installs_allowed_from":[
    "*"
  ],
  "launch_path": "http://www.google.com",
  "developer":{
    "name":"Gregory Koberger",
    "url":"http://gkoberger.net"
  }
}

Expected:

This app should fail to install - relative URLs should only be allowed in the launch_path.

Actual:

The app successfully can be installed. Launching the app starts at the absolute URL in the launch_path.

Comment 1

[Andrew Overholt [:overholt]]

What does the spec say on this?  Is this something we need to decide now so we aren't faced with trying to change this later?

Comment 2

[Anant Narayanan [:anant]]

launch_path can never be absolute, and is always relative to the origin of the app.

Comment 3

[Jason Smith [:jsmith]]

(In reply to Andrew Overholt [:overholt] from comment #1)
> What does the spec say on this?  Is this something we need to decide now so
> we aren't faced with trying to change this later?

Bill Walker I know was concerned about this bug existing, as am I. This essentially removes a key point behind the app manifest - starting within the app origin. Therefore, I think this blocks basecamp. Anant - Do you agree or disagree?

Comment 4

[Anant Narayanan [:anant]]

Yes, absolute paths should not be allowed in launch_path. Especially since we won't support multiple apps per origin for basecamp, this should be fixed.

Comment 5

[Andrew Overholt [:overholt]]

Blocker.  Can you take this, Anant?  If not, who should?

Comment 6

[Jason Smith [:jsmith]]

Anant - Marco is interested in working on this bug. Can he take it?

Comment 7

[Marco Castelluccio [:marco]]

Attachment: Patch 1

Comment 8

[Marco Castelluccio [:marco]]

Attachment: Patch 2

Another option.

Comment 9

[[:fabrice] Fabrice Desré]

Comment on attachment 658250 [details] [diff] [review]
Patch 1

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

I prefer the approach in patch 2.

Comment 10

[[:fabrice] Fabrice Desré]

Comment on attachment 658251 [details] [diff] [review]
Patch 2

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

There are a couple of issues with this patch in its current form:
- entry_points launch paths must also be checked.
- the checkManifest method is growing a bit too much to be replicated in the .js and in the .jsm

Comment 11

[Anant Narayanan [:anant]]

Thanks for jumping in Marco!

Comment 12

[Marco Castelluccio [:marco]]

Attachment: Patch

I've moved the checkManifest function to the AppsUtils.jsm module and imported the module in Webapps.js.

I've added some tests:
> invalid_launch_path
The manifest contains an absolute launch path.

> invalid_entry_point
The manifest contains an absolute launch path for an entry point.

> invalid_locale_entry_point
The manifest contains an absolute launch path for a locale entry point.

> wild_crazy_with_launch_paths
A correct manifest with relative launch paths (launch_path and entry_points launch paths).

Comment 13

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

Comment on attachment 658913 [details] [diff] [review]
Patch

Marco: I just landed bug 785545, which significantly rewrote the test code in dom/tests/mochitest/webapps/, so you'll need to update the test code in this patch to account for that.  Note in particular that there is no longer a "wild crazy" webapp, just a "basic" one, and wild_crazy_with_launch_paths.webapp should be simply launch_paths.webapp.

Comment 14

[Marco Castelluccio [:marco]]

Attachment: Patch with updated tests

The launch_paths test is just a basic test, we can improve it in another bug (that I'll file soon).
I hope I didn't forgot anything during the change between the two test methods (I have to say the new method is a lot better).

Comment 15

[[:fabrice] Fabrice Desré]

Comment on attachment 659057 [details] [diff] [review]
Patch with updated tests

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

I haven't looked at the tests in detail yet. Could you split up your patch in 2 parts (implementation and tests) ?

::: dom/apps/src/AppsUtils.jsm
@@ +129,5 @@
> +  checkManifest: function(aManifest, aInstallOrigin) {
> +    if (aManifest.name == undefined)
> +      return false;
> +
> +    function cbOrigin(aOrigin) {

nit: the function name is not very descriptive.

@@ +154,5 @@
>    }
>  }
> +
> +// Non exported utility functions
> +

Nit: you could move them in checkManifest()

@@ +166,5 @@
> +}
> +
> +function checkAbsoluteEntryPoint(entryPoints) {
> +  for (let name in entryPoints) {
> +    if (entryPoints[name].path && isAbsolute(entryPoints[name].path)) {

this is |launch_path|, not |path|

Comment 16

[Marco Castelluccio [:marco]]

Attachment: Patch

Sorry for the delay, I was studying for an exam.

Comment 17

[Marco Castelluccio [:marco]]

Attachment: Tests

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Green on Try.
https://tbpl.mozilla.org/?tree=Try&rev=b5de07d07bc2

https://hg.mozilla.org/integration/mozilla-inbound/rev/47f4395d89dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/897268730c68

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/47f4395d89dd
https://hg.mozilla.org/mozilla-central/rev/897268730c68