943710 - URL query parameters make validation barf

Status: RESOLVED WONTFIX

(Marketplace Graveyard :: Validation, defect, P4)

Description

[Ben Bucksch (:BenB)]

Reproduction:
* manifest.webapp contains:
  "launch_path" : "/app.html?lang=de",
* app.html exists, and works in another submit without the "?lang=de"
* Submit to Marketplace

Actual result:
- The app works fine in the Simulator, and the app does pick up the language parameter and starts in German.
- After upload to Marketplace, I'm getting the error:
---------------------------
Resource in packaged app not found.
Error: A launch_path within a packaged app is referenced, but the path used does not point to a valid item in the package.
Requested resource: app.html?lang=de
---------------------------

Expected result:
No error

Comment 1

[Ben Bucksch (:BenB)]

I suspect that the validation code doesn't understand URL parameters and looks for a filename "app.html?lang=de", which of course doesn't exist.
Firefox OS itself works fine.

Comment 2

[Christopher Van Wiemeersch [:cvan]]

Can you please upload your .zip (containing the aforementioned .manifest file) file to this bug?

Comment 3

[Ben Bucksch (:BenB)]

Attachment: biblehistory-en-0.8.1.zip

wiss pleeeesure

Comment 4

[Christopher Van Wiemeersch [:cvan]]

I'm not sure the exact reasons why you're hardcoding `?lang=de` but you could look at the value of `navigator.language` from JS when `app.html` gets launched and set the language appropriately.

But this is a definite validation bug - many thanks for filing!

http://f.cl.ly/items/2u0k1N302f2T381R1d3a/Screen%20Shot%202013-11-27%20at%202.01.28%20PM.png

Comment 5

[Matt Basta [:basta]]

This was WONTFIXed previously, but I can't find the original bug. `?` is a valid character in filenames inside a zip file, but there's no guarantee the platform will respect that. Allowing query parameters for launch paths inside a packaged app introduces ambiguity.

Since the value is obviously hard-coded anyway, you should simply define it as a constant wherever your code looks for the query parameter.

Comment 6

[Ben Bucksch (:BenB)]

> `?` is a valid character in filenames inside a zip file

While that may be true, it's far more likely that it's a URL parameter. But given that this is just a validator, you can check for both, if you think the "?" filename is important.

Either way, this *is* a bug in the validator, because the thing works in Firefox OS and the validator barfs. It should be easy to fix, too.

Comment 7

[Ben Bucksch (:BenB)]

> Since the value is obviously hard-coded anyway, you should simply define it as a constant wherever your code looks for the query parameter.

This is a cross-platform codebase.

Comment 8

[Matt Basta [:basta]]

> the thing works in Firefox OS

The validator is not Firefox OS-only, and the trade-off is with regard to security (reviewers wouldn't be aware that the file being requested isn't the one that's being loaded by the platform). Should a new version of Firefox OS (or any other apps-ready platform) start to observe this change, apps already submitted to the Marketplace could behave unexpectedly or even maliciously, depending on what type of app and how it's been built.

This is a simple change in the app; that is the solution here.

Comment 9

[Christopher Van Wiemeersch [:cvan]]

Not just Firefox OS, but Firefox everywhere respects a `launch_path` with querystring parameters. People (myself included) have added querystring parameters to the `launch_path` - and for tracking purposes. This app is not necessarily the only app wanting to use querystring parameters in its launch_path.

Comment 10

[Matt Basta [:basta]]

Query string parameters are allowed for hosted apps, just not packaged apps.

Comment 11

[Ben Bucksch (:BenB)]

Matt, why do you keep closing this? Me - a long time Mozilla developer -, and cvan - also a major code contributor -, both think this is a bug. Stop rejecting bug reports, or you won't have much support for your platform left. It's hard enough as-is to code against Firefox OS.

> Query string parameters are allowed for hosted apps, just not packaged apps.

That's just not true. The specs don't say that, and it works fine.

FWIW, loading "file://foo?bar=baz" is perfectly valid, too. Always has.

And no, it's not a security issue. (And I'm a member of the Mozilla security group.)

If the validator rejects valid, working code, that's a validator bug. A validator must help, not get in the way and prevent perfectly good working code.

Comment 12

[Matt Basta [:basta]]

> The specs don't say that

The spec doesn't mention URI format at all. I've gone ahead and updated the manifest spec on MDN to reflect the validator's behavior:

https://developer.mozilla.org/en-US/Apps/Developing/Manifest#launch_path

> If the validator rejects valid, working code, that's a validator bug.

The validator rejects plenty of perfectly valid, working code. We reject any code that makes it difficult for our human reviewers to determine what an app is truly doing or code that violates Marketplace guidelines.

I'm not going to fix this issue. It's intended behavior, and it's extraordinarily simple to work around inside an app (it's a static string sending data to a static script within the same zipball). It might be kind of shitty, but it doesn't make anything that you could want to do impossible or even hamper the development process in any real way. As a member of the Mozilla security group, you should have a deep understanding that security isn't single-faceted, and we shouldn't rely on a single means of preventing abusive content from reaching our users--in this case, assuming that the behavior of URIs within proprietary packaged apps won't unexpectedly change or regress.

Feel free to keep this bug open if you like.

Comment 13

[Ben Bucksch (:BenB)]

What is this? Kindergarten? You don't get to edit specifications to fit your views.

> The validator rejects plenty of perfectly valid, working code.

Then the validator must be fixed.

It must help. Not even a policeman is allowed to reject or arrest people just because he doesn't like them. He can't just make up laws as he pleases. With power comes responsibility.

You can't interfere with the affairs of others. This is *my* app. Your validator has no business telling me how I should write code. It should only alert me of bugs and security issues. This is neither. If you think it is, prove it. Otherwise it's a validator bug.

You just made up a law that you think should exist, but that doesn't.

> simple to work around inside an app (it's a static string sending data to a static script
> within the same zipball).

So, here you're tell me how to write my app. That's not your place.

I have an elegant solution that works fine, and I'm actually very happy about that.

> I'm not going to fix this issue.

My problem is that you're hindering others to do so.

Comment 14

[Ben Bucksch (:BenB)]

(FWIW, yes, I *can* fix this in my app, but it would mean restructuring it - because it's also a webapp, with the same code - and having code being generated by build scripts - which I try very hard to avoid, and have been able to avoid for everything else so far.)

Comment 15

[Ben Bucksch (:BenB)]

(And using navigator.language will cause outright errors, if the system language doesn't match the language of the app's content.
The manifest must be adapted to the content anyway, and I try to keep all the information about the content in this one place. I think that's a reasonable thing to do.)

Comment 16

[Matt Basta [:basta]]

This is not a P1, it is intended behavior, and this is no place for name calling.

Comment 17

[Ben Bucksch (:BenB)]

FTR, I suggest as fix:

* Before validating the file name, strip "?" and "#" and everything after it.
  var filename = launch_path.replace(/[#\?].*/, "");
* *If* you want to allow file names that include a "?" (which I think isn't wise as a filename,
  but Matt said he wants to support that), you can test for both:
  | var path_only = launch_path.replace(/[#\?].*/, "");
  | if ( !file(launch_path).exists()) && !file(path_only).exists())
  |   throw "Launch path not found";
* *If* you're worried about a specific security problem, e.g. certain escape parameters,
  we can check for them. RFC2396 specifies which characters are valid in a URL,
  and we can reject the forbidden characters. This should be fairly easy, too.

If you have a security concern, please be specific what you're thinking about - which threat case and which vulnerability -, so that we can protect against it.

Comment 18

[Matt Basta [:basta]]

As I have said before, there's nothing here to "fix". This message is entirely intentional and was added deliberately. 

Launch paths inside packaged apps are not true URLs and their behavior is undocumented and undefined. Just because it works as expected doesn't mean it should or will continue to work the same way.

The validator does not simply check for correctness, it checks for future compatibility and prevents deceptive practices and obfuscation techniques used to hide code from reviewers.

Comment 19

[Ben Bucksch (:BenB)]

From every I could read, launch_path is a relative URL. Relative URLs can include queries and anchors.

I have not seen any substantiation why a "?" in there is wrong, apart from your personal option. I'm sorry, but I disagree with your opinion.

This is an important capability to pass information from the manifest to the app.
I don't want to build 5 versions of the app, with different code for each, but use the same code and just change the manifest for each variant. I think that's a valid concern.

Comment 20

[Matt Basta [:basta]]

I'm sorry, but I disagree with your opinion that this is an important capability. This is a convenience that happens to work, but there's no guarantee that it will continue to work or that it even should work to begin with.

The field is named "launch_path", not "launch_url". There's no documentation that I've been able to find that even refers to launch_path within a packaged app as a URL. In fact, DXR shows that the path is treated *as a path* within the B2G source:

http://dxr.mozilla.org/mozilla-central/source/browser/devtools/app-manager/app-validator.js#144

Note the leading dot, which wouldn't behave as expected in a traditional URL context. This is used in other places, as well.

> I don't want to build 5 versions of the app, with different code for each,
> but use the same code and just change the manifest for each variant.

This limitation certainly does not prevent you from building a single version of your app. You can include a local settings file, you can read in the manifest from within the package and add an additional custom field, you can use a hash (which are allowed), etc. This single check does not block you from using one of the many other ways of achieving identical results.

Comment 21

[Ben Bucksch (:BenB)]

http://www.w3.org/2012/sysapps/manifest/#launch_path-member

> 3.7 launch_path member
> The launch_path localizable member is a relative URL that represents a resource
> within the origin of the application that is loaded when the application is launched. 

This clearly says "relative URL".

(The spec also mentions packaged apps at (exactly) one point, so it does apply to them.)

Unless there is the spec specifically says that this is not valid for packaged apps, this is true for them as well - per spec.

From the the point of view of an app, how I write my code, my packaged app lives at a local URL (file: or resource or something similar), not in a file. Of course most URLs are eventually resolved to files, but that doesn't prevent Apache from serving "index.html?lang=de" by loading file "index.html". Claiming anything else would be just wrong. I expect the same here. In fact, even "file://foo/index.html?lang=de" works as expected, so there's no reason why it shouldn't here.

Again, if you have more than your personal opinion to offer, some factual reasons why this can't be made to work, go ahead.

Otherwise, the official w3c spec stands.

Comment 22

[Matt Basta [:basta]]

That is not an official W3 spec (it hasn't even moved past editor's draft), and it is not the spec that's implemented in Firefox.

If citations of the Gecko source isn't factual enough for you, then I don't know what constitutes fact.

> I expect the same here.

Your opinion has been noted.

Comment 23

[Ben Bucksch (:BenB)]

Fact is that the app works
* as a packaged app in Firefox OS
* locally with file: (since decades)
* on the webserver (since decades)
and
according to the webapp spec, it's valid.

All tried in comment 17 is to be constructive.

Comment 24

[Ben Bucksch (:BenB)]

Please let's stop this argument, as there is no point in arguing further.
I'll simply wait for someone to fix this. I would appreciate, if you didn't block it.

Comment 25

[Matt Basta [:basta]]

I fully intend to r- any commit that removes that check from the validator.

Comment 26

[Ben Bucksch (:BenB)]

I appreciate the hostility. Gives me a really warm feeling about Firefox OS. Thank you.

Comment 27

[:Harald Kirschner :digitarald]

With the new manifest spec supporting start_url instead of launch_path this decision should be revisited. Partners are using query params to configure packaged apps.

It is a black hole in the spec and we shouldn't be more restrictive than necessary in the validator to users from future breaking changes that we can't foresee.

Comment 28

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

We're also using query parameters in our own apps, like j2me.js, where I'm working around this issue in pull 851 <https://github.com/andreasgal/j2me.js/pull/851>.

Comment 29

[David Durst [:ddurst]]

Mark is on PTO. If we're talking about URLs and not paths, a lot of this (poorly handled) historical stuff goes away, and it may warrant a new bug to make the goal clearer. Especially if we're on a timeline.

[That said, we are dealing with two different validators, which would appear to need to be synced. We can plug this hole for now, but we should address the root cause (competing validations) in short order after that.]

Comment 30

[Mathieu Pillard [:mat]]

The w3c manifest spec (still a draft, and not implemented by us atm) supports start_url instead of launch_path... but that spec doesn't specify anything for packaged apps and therefore doesn't really apply here.

I'd argue that if there is a black hole in the spec, that's a good reason to be more restrictive - we simply don't know if the platform is going to continue handling it the way it does, or if it even works like that across all supported platforms (all Firefox OS versions, Firefox Desktop for Win/Lin/Mac, Firefox for Android) right now.

Comment 31

[Ben Bucksch (:BenB)]

I don't think that there's a black hole in the spec. The relevant specs actually state that this is allowed, and there's nothing stating the opposite, so it's valid.

> we simply don't know if the platform is going to continue handling it the way it does

This functionality is needed, we already have 2 apps that need this (mine and Myk's). If the platform changes, then these changes need to be made so that this continues to work. I see no reason why it should be difficult. Please note that this work just fine right now.

FWIW, I have abandonned FirefoxOS due to this bug and the esp. reaction here.

Comment 32

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

FWIW, my app has worked around the limitation.  Nevertheless, it would certainly be useful to be able to specify query parameters in the launch path again.

Comment 33

[Ben Bucksch (:BenB)]

Current spec revision:
http://www.w3.org/2008/webapps/manifest/#start_url-member

> The start_url member is a string that represents the start URL,
> which is URL that the developer would prefer the user agent load
> when the user launches the web application

Given the definition, it still is a relative URL.

A URL may contain a query string, so this is clearly and unambiguously allowed.

Comment 34

[David Durst [:ddurst]]

Ben, there are a few of us inheriting this thread, so please don't assume that the comments (and attitude) of Basta are still relevant to this conversation -- I/we don't.

start_url is a better solution than launch_path (imo). Platform support and interpretation and spec adoption are out of our hands. The fact that we're talking about packaged apps puts us (all) in territory with no good guidance.

I share Mat's reluctance to be more permissive on something that is loosely defined, because once we introduce something, we have to support it forever.

[FWIW, in large measure, this is a conversation about the discrepancy between two validators, and trying to determine which one takes precedence. I can't speak to the future path of the validators.]

That said, I will NI Mark, to see what his take on it is.

Comment 35

[David Durst [:ddurst]]

(In reply to Harald Kirschner :digitarald from comment #27)
> With the new manifest spec supporting start_url instead of launch_path this
> decision should be revisited. Partners are using query params to configure
> packaged apps.
> 
> It is a black hole in the spec and we shouldn't be more restrictive than
> necessary in the validator to users from future breaking changes that we
> can't foresee.

It's clear that start_url is a URL, and shouldn't be anything but. It's unclear what launch_path is (though one can easily say a URL is expected for a hosted app), so it has opened the door to use becoming the definition. But we have to acknowledge that there is risk in assuming that something not specifically stated as a URL can be treated as one, and that risk is assuming by the app author, not a validator. The validator can and should only act according to defined rules.

And as Harald notes, start_url is supported in the new spec instead of launch_path. This puts us in a(nother) situation of No Perfect Answer.

Proposed:
  1) we *require* start_url;
  2) we *require* that if there is a launch_path stated, its value must match the value entered for start_url;
  3) we do not allow apps that have a value only for launch_path.

This of course requires that launch_path allow query params, but also allows us to guard against changes on the platform in light of changes to the spec. And so this would seem to satisfy both extremes with a reasonable compromise.

Thoughts? Objections?

Comment 36

[:Harald Kirschner :digitarald]

I would put `start_url` in a bucket of features that Marketplace requires to support the new spec and keep this issue separated.

With the new spec on the horizon I don't see the need to unnecessarily fence in `launch_path` against future breaking changes; which seems to be the only argument against not allowing query params.