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

NEW
Assigned to

Status

()

Core
DOM: Apps
5 years ago
4 years ago

People

(Reporter: krupa, Assigned: fabrice)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

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

Comment 1

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

Updated

5 years ago
Component: Web Apps → DOM: Mozilla Extensions
Product: Firefox → Core
QA Contact: webapps → general
Sounds like a mozapps API bug. Fabrice and Ian - Can you confirm this to be a mozapps API bug?

Updated

5 years ago
OS: Mac OS X → All
Hardware: x86 → All

Comment 3

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

Updated

5 years ago
Blocks: 746465
(Assignee)

Comment 4

5 years ago
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

5 years ago
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.
(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.
Can someone take point on fixing this bug?

Updated

5 years ago
blocking-kilimanjaro: --- → ?

Updated

5 years ago
tracking-firefox15: --- → ?

Updated

5 years ago
blocking-kilimanjaro: ? → ---

Updated

5 years ago
tracking-firefox15: ? → +
Assigning to bz for now, as a module peer, to hopefully find someone to take the lead on deeper investigation of this issue.
Assignee: nobody → bzbarsky

Updated

5 years ago
tracking-firefox15: + → ---
tracking-firefox16: --- → ?
Desktop Apps is now targeting FF 16 - switching the flag to say as such.
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?
Or are we just using this component as a dumping ground for every API someone decides to expose to web pages, basically?
(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!
(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.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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).
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.... ;)
(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?
(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.
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.

Updated

5 years ago
tracking-firefox16: ? → +
tracking-firefox17: --- → +

Updated

5 years ago
No longer blocks: 746465
Component: DOM: Mozilla Extensions → DOM: Apps
Anant - What should we do here in this use case?
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).
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.
Assignee: bzbarsky → anant
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.
Assignee: anant → nobody
blocking-basecamp: --- → ?
tracking-firefox16: + → ---
tracking-firefox17: + → ---
(Assignee)

Updated

5 years ago
Assignee: nobody → fabrice
Basecamp-. We should add something to the dev docs to indicate that we won't support this.
Assignee: fabrice → nobody
blocking-basecamp: ? → ---
Keywords: dev-doc-needed

Updated

5 years ago
Assignee: nobody → fabrice

Updated

5 years ago
Blocks: 783741
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.