843802 - Don't accept an appType parameter when sideloading apps.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[[:fabrice] Fabrice Desré]

Attachment: patch

Currently the remote sideloading actor honors an "appType" parameter that was designed to let change the application status easily. Unfortunately that can lead to hard to debug conflicts when it's different from the status declared in the manifest.

This patch removes the appType parameter and look for the app status in the manifest itself instead.

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 716754 [details] [diff] [review]
patch

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

Looks good.
r=me with the comments addressed, please.

::: b2g/chrome/content/dbg-webapps-actors.js
@@ +85,5 @@
>          message: aMsg
>        });
>    },
>  
> +  _getAppType: function wa_actorGetAppType(aManifest) {

We only need the manifest 'type' member, so there is no need to get the whole manifest object as a function parameter.

@@ +109,5 @@
>            let manFile = aDir.clone();
>            manFile.append("manifest.webapp");
> +          DOMApplicationRegistry._loadJSONAsync(manFile, function(aManifest) {
> +            if (!aManifest) {
> +              self._sendError("Error Parsing manifest.webapp", aId);

It seems that we need to early return here...

@@ +117,5 @@
> +
> +            // In production builds, don't allow installation of certified apps.
> +#ifdef MOZ_OFFICIAL
> +            if (appType == Ci.nsIPrincipal.APP_STATUS_CERTIFIED) {
> +              self._sendError("Installing certified apps is not allowed.", aId);

... and here.

@@ +137,3 @@
>  
> +              if (!aMetadata.origin) {
> +                self._sendError("Missing 'origin' propery in metadata.json", aId);

typo: "property"

@@ +191,4 @@
>  
> +          DOMApplicationRegistry._loadJSONAsync(manFile, function(aManifest) {
> +            if (!aManifest) {
> +              self._sendError("Error Parsing manifest.webapp", aId);

return;

@@ +198,5 @@
>  
> +            // In production builds, don't allow installation of certified apps.
> +#ifdef MOZ_OFFICIAL
> +            if (appType == Ci.nsIPrincipal.APP_STATUS_CERTIFIED) {
> +              self._sendError("Installing certified apps is not allowed.", aId);

return;

@@ +209,5 @@
> +              origin: origin,
> +              installOrigin: origin,
> +              manifestURL: origin + "/manifest.webapp",
> +              appStatus: appType
> +            }

missing ;

Comment 2

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87be4954d11b

Comment 3

[[:fabrice] Fabrice Desré]

Comment on attachment 716754 [details] [diff] [review]
patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
This patch only deals with a developer facing feature used to sideload apps. It removes a parameter that was designed to allow easy tweaking of the application privilege level, but that ended up leading unexpected app states (eg, privileged permissions but no CSP). We now check everything at install time on the device instead of trusting the sideloading client.

Comment 4

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/87be4954d11b

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/32b133952b6f