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

RESOLVED FIXED in mozilla18

Status

()

Core
DOM: Apps
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: jsmith, Assigned: marco)

Tracking

(Blocks: 2 bugs, {dev-doc-needed})

Trunk
mozilla18
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-basecamp:+)

Details

(Whiteboard: [WebAPI:P0], [qa-])

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

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

Updated

5 years ago
blocking-basecamp: --- → ?
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?
Whiteboard: [blocked-on-input Fabrice Desré]
launch_path can never be absolute, and is always relative to the origin of the app.
(Reporter)

Comment 3

5 years ago
(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?
Whiteboard: [blocked-on-input Fabrice Desré]
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.
Blocker.  Can you take this, Anant?  If not, who should?
Assignee: nobody → anant
blocking-basecamp: ? → +
(Reporter)

Comment 6

5 years ago
Anant - Marco is interested in working on this bug. Can he take it?
(Assignee)

Comment 7

5 years ago
Created attachment 658250 [details] [diff] [review]
Patch 1
Assignee: anant → mar.castelluccio
Status: NEW → ASSIGNED
(Assignee)

Comment 8

5 years ago
Created attachment 658251 [details] [diff] [review]
Patch 2

Another option.
(Assignee)

Updated

5 years ago
Attachment #658250 - Flags: review?(fabrice)
(Assignee)

Updated

5 years ago
Attachment #658251 - Flags: review?(fabrice)
Comment on attachment 658250 [details] [diff] [review]
Patch 1

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

I prefer the approach in patch 2.
Attachment #658250 - Flags: review?(fabrice) → review-
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
Attachment #658251 - Flags: review?(fabrice) → review-
Thanks for jumping in Marco!
(Assignee)

Comment 12

5 years ago
Created attachment 658913 [details] [diff] [review]
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).
Attachment #658250 - Attachment is obsolete: true
Attachment #658251 - Attachment is obsolete: true
Attachment #658913 - Flags: review?(fabrice)
Whiteboard: [WebAPI:P0]
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.
(Assignee)

Comment 14

5 years ago
Created attachment 659057 [details] [diff] [review]
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).
Attachment #658913 - Attachment is obsolete: true
Attachment #658913 - Flags: review?(fabrice)
Attachment #659057 - Flags: review?(fabrice)
(Assignee)

Updated

5 years ago
Blocks: 789345
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|
Attachment #659057 - Flags: review?(fabrice) → review-
(Assignee)

Comment 16

5 years ago
Created attachment 663690 [details] [diff] [review]
Patch

Sorry for the delay, I was studying for an exam.
Attachment #659057 - Attachment is obsolete: true
Attachment #663690 - Flags: review?(fabrice)
(Assignee)

Comment 17

5 years ago
Created attachment 663691 [details] [diff] [review]
Tests
Attachment #663691 - Flags: review?(fabrice)
Attachment #663690 - Flags: review?(fabrice) → review+
Attachment #663691 - Flags: review?(fabrice) → review+
(Reporter)

Updated

5 years ago
Keywords: checkin-needed
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
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/47f4395d89dd
https://hg.mozilla.org/mozilla-central/rev/897268730c68
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(Reporter)

Updated

5 years ago
Whiteboard: [WebAPI:P0] → [WebAPI:P0], [qa-]
(Assignee)

Updated

5 years ago
Keywords: dev-doc-needed
(Assignee)

Updated

5 years ago
Blocks: 783741
Depends on: 854975
See Also: → bug 952696
You need to log in before you can comment on or make changes to this bug.