755044 - App installs fail if "installs_allowed_from" field value has trailing slashes

Status: RESOLVED WONTFIX

(Core Graveyard :: DOM: Apps, defect)

Description

[krupa raj[:krupa]]

steps to reproduce:
1. Load https://marketplace.mozilla.org/en-US/app/qr-code-generator/
2. Try to install the app

observed behavior:
Firefox fails to install the app if "installs_allowed_from" field value has trailing slashes.i.e.,

"installs_allowed_from": [
   "https://marketplace.mozilla.org"
 ],
is correct but
 "installs_allowed_from": [
   "https://marketplace.mozilla.org/"
 ],
is not.

Marketplace however allows trailing slashes and Firefox should too.

Note: Until we fix this in Firefox, Marketplace will temporarily stop allowing this.

Comment 1

[krupa raj[:krupa]]

For now, we are applying a band-aid in Marketplace by implementing bug 755047

Comment 2

[Jason Smith [:jsmith]]

Sounds like a mozapps API bug. Fabrice and Ian - Can you confirm this to be a mozapps API bug?

Comment 3

[Ian Bicking (:ianbicking)]

Yes, this seems like a mozApps bug, the values should be normalized when checking for matches.

Comment 4

[[:fabrice] Fabrice Desré]

if we consider that installs_allowed_from contains origins (and I think it should), "https://marketplace.mozilla.org/" is invalid. In such cases, should we normalize them to their origin, or discard them?

Comment 5

[Ian Bicking (:ianbicking)]

Explaining the subtle distinction between https://marketplace.mozilla.org/ and https://marketplace.mozilla.org seems like more work and less developer-friendly than just handling this case.  Also I believe we aren't normalizing with respect to case and ports currently, and HTTP://MARKETPLACE.MOZILLA.ORG is a valid origin.  So some normalization is in order.

Comment 6

[Wil Clouser [:clouserw]]

(In reply to krupa raj 82[:krupa] from comment #1)
> For now, we are applying a band-aid in Marketplace by implementing bug 755047

I'd like to see an ETA in this bug before we start talking about temporary patches on AMO.

Comment 7

[Jason Smith [:jsmith]]

Can someone take point on fixing this bug?

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

Assigning to bz for now, as a module peer, to hopefully find someone to take the lead on deeper investigation of this issue.

Comment 9

[Jason Smith [:jsmith]]

Desktop Apps is now targeting FF 16 - switching the flag to say as such.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I have no idea what this bug is talking about... it's not part of the "DOM" as I know it.  Someone care to explain?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Or are we just using this component as a dumping ground for every API someone decides to expose to web pages, basically?

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Boris Zbarsky (:bz) from comment #11)
> Or are we just using this component as a dumping ground for every API
> someone decides to expose to web pages, basically?

Yep!

Comment 13

[Jason Smith [:jsmith]]

(In reply to Boris Zbarsky (:bz) from comment #10)
> I have no idea what this bug is talking about... it's not part of the "DOM"
> as I know it.  Someone care to explain?

It's a mozapps API bug. In the app manifest (https://developer.mozilla.org/en/Apps/Manifest), if you specify trailing slashes on an installs_allowed_from value, then you won't be able to install the app with that manifest.

In regards to who typically handles these bugs - usually it's developers who have worked with the mozapps API on the apps team.

Comment 14

[Jason Smith [:jsmith]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #12)
> (In reply to Boris Zbarsky (:bz) from comment #11)
> > Or are we just using this component as a dumping ground for every API
> > someone decides to expose to web pages, basically?
> 
> Yep!

Sorry about that. I'll followup on the bug to migrate the webapps stuff to a core component (there's a bug on file to take care of this, but traction on it has been slow).

Comment 15

[Boris Zbarsky [:bzbarsky]]

Jason, thanks for the documentation link!

David, any idea who should own this?  You seem to have at least written tests for this stuff in the past.... ;)

Comment 16

[Jason Smith [:jsmith]]

(In reply to Boris Zbarsky (:bz) from comment #15)
> Jason, thanks for the documentation link!
> 
> David, any idea who should own this?  You seem to have at least written
> tests for this stuff in the past.... ;)

Bill Walker and Myk (cc-ed here) would be good people to ask (both of them drive the apps engineering meetings weekly). Bill or Myk - What's your thoughts on this bug?

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Boris Zbarsky (:bz) from comment #11)
> Or are we just using this component as a dumping ground for every API
> someone decides to expose to web pages, basically?

There is Bug 766386.

Comment 18

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

Anant and Jonas are in the W3C working group standardizing the webapps DOM API, so the best owner would be one of them.  cc:ing them.

Comment 19

[Jason Smith [:jsmith]]

Anant - What should we do here in this use case?

Comment 20

[Anant Narayanan [:anant]]

In the spirit of HTML and many other WebAPIs it makes most sense to be lenient about how much we enforce the correctness of an origin. While the presence of a trailing slash is technically incorrect, it is well worth the effort to tolerate it.

However, we should definitely normalize as suggest by Ian, and always store valid origins in any of our code; which includes Firefox, the various WebRTs and the Marketplace (in this particular case, the trailing slash should be stripped before it is stored).

Comment 21

[Boris Zbarsky [:bzbarsky]]

I'm still totally the wrong owner for this.  Picking a victim based on comment 18, with the assumption that he'll reassign as needed.

Comment 22

[Jason Smith [:jsmith]]

Based on the DOM: Apps triage strategy specification, a bug needs to be considered a blocker for basecamp for resources to be allocated to it (to keep the focus on priorities). Based on that, I'm removing the tracking flags given that won't drive whether we decide to fix this in the short-term (I'm unassigning as well).

Given that was in question in the triage, I'm putting this in the nom queue for basecamp triage analysis.

Comment 23

[Jason Smith [:jsmith]]

Basecamp-. We should add something to the dev docs to indicate that we won't support this.

Comment 24

[Mark Giffin [:markg]]

I added a note about not using trailing slashes in installs_allowed_from in the main MDN page about manifests here:

https://developer.mozilla.org/en-US/docs/Apps/Manifest

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Core Graveyard / DOM: Apps is inactive. Closing all bugs in this component.