741526 - Mozilla central mozapps implementation ignores Content-Type

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect, P1)

Description

[[e-mail] [:onecyrenus]]

https://developer.mozilla.org/en/Apps/Manifest

The Manifest as specified on mdn should be served with a specific content-type. 

this is ignored in the current moz-central implementation. 

My recommendation would be to either drop this as a requirement from all api's or enforce on mozilla central. 

Having it work on some platform instances, can only lead to confusion

Comment 1

[Ian Bicking (:ianbicking)]

See also: https://bugzilla.mozilla.org/show_bug.cgi?id=712045

Comment 2

[Jason Smith [:jsmith]]

Shouldn't this bug be logged in Core/DOM --> Mozilla Extensions? Myk's documentation for the desktop runtime indicates this.

Comment 3

[Jason Smith [:jsmith]]

David - Is this a blocker for marketplace beta? What's the risk with leaving this in?

Comment 4

[Ian Bicking (:ianbicking)]

The primary risk is that it allows a site with user-contributed content to potentially have a file uploaded that can be used to "install" the site, and potentially sell access to the site.  However, they could not do this through the Marketplace, as it also validates the Content-Type.  So without a viable store that doesn't check the Content-Type there is not much risk currently.

Comment 5

[Jennifer Arguello :ticachica]

Not sure what the user experience is as a result of this issue. The user base for this release is small and the number of apps are not many that fact plus what Ian said in comment #4 make this seem not that risky.

Comment 6

[[e-mail] [:onecyrenus]]

Willing to go along with, just there are some caveats which haven't been explained. 

1) We are assuming that the only place people will install apps is through the marketplace. 

2) There is a developer issue where they  will be allowed to install their app, and then they will try to upload to the marketplace, and the flow won't work for them.

These are fairly minor but we should document this, so it's clear as to what is happening.

Comment 7

[Jason Smith [:jsmith]]

Nominating for k9o - not implementing this results in a large inconsistent user experience between AITC and the desktop implementation, as the following problem can occur:

1. Install an app with the incorrect content-type or encoding
2. Doorhanger appears, select to install

Result - Desktop will allow the app to be installed to the underlying registry and natively, but a sync to the AITC server will fail, as we require content-type and encoding there. This results in a large inconsistent user experience - an app that should have been allowed to be installed was installed, resulting in inconsistent results in how this is handled (desktop implementation vs. AITC implementation). We should aim for consistency, so that the end user does not suffer from unexpected limitation of functionality.

Note: Top apps are affected here, as I've confirmed this problem happens with Lord of Ultima, a tier 1 app.

We also need to fix this for the 1st release of the AITC desktop.

Comment 8

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #7)
> Nominating for k9o - not implementing this results in a large inconsistent
> user experience between AITC and the desktop implementation, as the
> following problem can occur:
> 
> 1. Install an app with the incorrect content-type or encoding
> 2. Doorhanger appears, select to install
> 
> Result - Desktop will allow the app to be installed to the underlying
> registry and natively, but a sync to the AITC server will fail, as we
> require content-type and encoding there. This results in a large
> inconsistent user experience - an app that should have been allowed to be
> installed was installed, resulting in inconsistent results in how this is
> handled (desktop implementation vs. AITC implementation). We should aim for
> consistency, so that the end user does not suffer from unexpected limitation
> of functionality.
> 
> Note: Top apps are affected here, as I've confirmed this problem happens
> with Lord of Ultima, a tier 1 app.
> 
> We also need to fix this for the 1st release of the AITC desktop.

Change this - checking lord of ultima, it looks like it is serving the right content-type. So removing k9o flag and will reopen other bug on this.

Comment 9

[Anant Narayanan [:anant]]

This is a security requirement, and should be implemented ASAP. The Marketplace has been validating the header for a while, but it was recently disabled in bug 778214. I think we should enforce this both in WebRT and the Marketplace for the v1 launch, since it's likely we won't have a solution for bug 712045 before then.

Comment 10

[Jason Smith [:jsmith]]

(In reply to Anant Narayanan [:anant] from comment #9)
> This is a security requirement, and should be implemented ASAP. The
> Marketplace has been validating the header for a while, but it was recently
> disabled in bug 778214. I think we should enforce this both in WebRT and the
> Marketplace for the v1 launch, since it's likely we won't have a solution
> for bug 712045 before then.

I could see an argument for blocking in the fact that we shouldn't assume that marketplace will always protect this requirement even if bug 778214 gets backed out, cause for example, maybe the OEM decides to include their own marketplace? Although I know when we originally did a bug scrub of these bugs, this one wasn't chosen on the blocker list probably because marketplace enforcing it. Therefore, I'm kinda in the middle on this one on whether to block or not.

Comment 11

[Andrew Overholt [:overholt]]

While this is an important thing to do before first launch of the Marketplace, would we hold the first release of B2G for it?

Comment 12

[Anant Narayanan [:anant]]

Yes, this is an important security measure. Given that the Marketplace recently stopped checking for the Content-Type, it is doubly important that the m-c implementation does.

Comment 13

[Andrew Overholt [:overholt]]

Thanks for the input, Anant.  Marking as a blocker.

Comment 14

[Andrew Overholt [:overholt]]

Anant, is this something you can take?  If not, do you have any suggestions for an owner?  Thanks.

Comment 15

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 16

[[:fabrice] Fabrice Desré]

Comment on attachment 663724 [details] [diff] [review]
Patch

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

Sorry for the late review.
Since we landed support for updates, we also need to add a content-type check there. And we must decide if we want/need a mime type for the mini-manifests that are used for packaged apps.

Comment 17

[[:fabrice] Fabrice Desré]

Jonas, any opinion here? I'm not convinced at all that we need this content-type check, but I won't fight either...

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

For both packaged and hosted apps, we should enforce a Content-Type if the URL passed to .install/.installPackaged is a cross-origin URL. For same-origin URLs we should not enforce a content-type.

Comment 19

[Andrew Overholt [:overholt]]

Marco, do you have everything you need to proceed with this bug?  Are you still able to do this work?

Comment 20

[Alex Keybl [:akeybl]]

Milestoning for C2 (deadline of 12/10), as this meets the criteria of "known bugs with LOE:M".

Comment 21

[Marco Castelluccio [:marco]]

Attachment: Patch v2

So, if I understand correctly, the content-type check during the update isn't needed because the same origin check doesn't apply there. Right?

Sorry for the delay, I've been really busy recently.

Comment 22

[[:fabrice] Fabrice Desré]

Comment on attachment 681726 [details] [diff] [review]
Patch v2

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

Sorry for the late review Marco. That looks good, let me know if you need me to land the patch.

Comment 23

[Jason Smith [:jsmith]]

(In reply to Fabrice Desré [:fabrice] from comment #22)
> Comment on attachment 681726 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 681726 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the late review Marco. That looks good, let me know if you need me
> to land the patch.

I'd say let's just get this landed now given that's a basecamp+ blocker.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e64cd799b7b6

Comment 25

[Jonathan Griffin (:jgriffin)]

This totally breaks B2G; it dies during startup with:

E/GeckoConsole(   43): [JavaScript Error: "SyntaxError: missing : after property id" {file: "resource://gre/modules/AppsUtils.jsm" line: 187}]

Comment 26

[Ryan VanderMeulen [:RyanVM]]

So this basically blew up the world when it landed. Backed out. Please don't request re-landing until you've got a green Try link to go with it.
https://hg.mozilla.org/integration/mozilla-inbound/rev/34e6bda0ba0e

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&pusher=[e-mail]

Comment 27

[Dietrich Ayala (:dietrich)]

Marco or Fabrice, when can you update this patch? We're 7 days out from the end of C2 so need to get this landed as soon as possible.

Comment 28

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v3

Try build: https://tbpl.mozilla.org/?tree=Try&rev=081c9cfde4c7

Comment 29

[Fernando Jiménez Moreno [:ferjm]]

Tests are locally passing: 

0 INFO SimpleTest START
1 INFO TEST-START | chrome://mochitests/content/chrome/dom/apps/tests/test_apps_service.xul
2 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/apps/tests/test_apps_service.xul | Should be able to get the Apps Service
3 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/apps/tests/test_apps_service.xul | getAppByManifestURL() should be a method in nsIAppsService
4 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/apps/tests/test_apps_service.xul | getAppByManifestURL() should return null for an empty string manifest url
5 INFO TEST-END | chrome://mochitests/content/chrome/dom/apps/tests/test_apps_service.xul | finished in 81ms
6 INFO TEST-START | Shutdown
7 INFO Passed: 3
8 INFO Failed: 0
9 INFO Todo:   0
10 INFO SimpleTest FINISHED

Comment 30

[Fernando Jiménez Moreno [:ferjm]]

I didn't noticed the tests located at dom/tests/mochitests/apps/ :

9736 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul | origin is not in installs_allowed_from - got INVALID_MANIFEST, expected INSTALL_FROM_DENIED
9737 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul | installation error: INVALID_MANIFEST 

I'll take a look at them tomorrow afternoon if no one else does it before.

Comment 31

[Jason Smith [:jsmith]]

Switching assignee given that ferjm is working on this now.

Comment 32

[[:fabrice] Fabrice Desré]

Attachment: correct patch

SO, the tests were missing some ^headers^ file to set the correct content-type...

Comment 33

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7bb787b0d740

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7bb787b0d740

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/55e7b7ee8d30
https://hg.mozilla.org/releases/mozilla-beta/rev/3194d333e195