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.

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?

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

(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?

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?

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

Another option.

Comment on attachment 658250 [details] [diff] [review] Patch 1 Review of attachment 658250 [details] [diff] [review]: ----------------------------------------------------------------- I prefer the approach in patch 2.

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

Thanks for jumping in Marco!

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 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.

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 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|

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

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

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