1094577 - csp attribute in manifest doesn't actually apply a CSP for packaged apps

Status: RESOLVED WONTFIX

(Core Graveyard :: DOM: Apps, defect)

Description

[Paul Theriault [:pauljt] (no longer reading bugmail)]

+++ This bug was initially created as a clone of Bug #1021972 +++

So in a similar manner to bug 1021972, we have a problem when installing an app from the marketplace which has a CSP. I'm not sure if this is marketplace related, just a platform bug. I'm still testing the different permutations.


STR:
1. enable marketplace reviewer certs under developer options (2.1+ only)
2. install the app from https://marketplace.allizom.org/app/csp-test-1/
3. Click this inline script link at the bottom of the app

(NOTE: this test is named badly, its actually testing loading a remote script... because ... reasons...)

Expected:
Remote scripts should be blocked since the manifest contains this:
"csp":"default-src *; script-src 'self'; object-src 'none'; style-src 'self';"

Actual:
Jquery is loaded from a CDN despite the CSP.


BTW I'm putting this in DOM:APPS since I can see that there is no CSP set in webapps.json (ie its getting lost somewhere in the upload/install process)

Comment 1

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Attachment: snipped from /data/local/webapps.json

This snippet shows that there is no CSP being installed:
...
"name": "CSP Test",
"csp": "",
...

Comment 2

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Attachment: manifest, as downloaded from Marketplace

Ok so this looks like a platform issue, since the manifest from marketplace still has the csp value intact.

Comment 3

[Paul Theriault [:pauljt] (no longer reading bugmail)]

I've confirmed that this behavior also applies to self-installing packaged apps. I've verified this on device on 2.1 and on simulator in 2.2.

Still looking through webapps.jsm to try to figure out why no csp is ending up in webapps.json....

Comment 4

[[:fabrice] Fabrice Desré]

Can you check if we hit http://mxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.jsm#1088 when installing the app?

Comment 5

[Paul Theriault [:pauljt] (no longer reading bugmail)]

I'll try that.

Could it be the _cloneApp function though? I had a hard time working out what aLocaleManifest was though. (making a debug build at the moment). 

What is app.updateManifest? Is that the mini-manifest? If it is, then I think thats the problem. If not, I'm barking up the wrong tree...

For reference:

2756   _cloneApp: function(aData, aNewApp, aLocaleManifest, aManifest, aId, aLocalId) {
...
2792     appObject.csp = aLocaleManifest.csp || "";
2793     appObject.role = aLocaleManifest.role;


_cloneApp is called in confirmInstall:

2913     let jsonManifest = aData.isPackage ? app.updateManifest : app.manifest;
...
2917     let manifest =
2918       new ManifestHelper(jsonManifest, app.origin, app.manifestURL);
...
2923     let appObject = this._cloneApp(aData, app, manifest, jsonManifest, id, localId);


So my thought was maybe app.updateManifest doesn't have the csp in it...

Comment 6

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Fabrice Desré [:fabrice] from comment #4)
> Can you check if we hit
> http://mxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.jsm#1088 when
> installing the app?

It did not appear to be called. 

But I did confirm that _cloneApp is called, and at this line:

2792     appObject.csp = aLocaleManifest.csp || "";

aLocaleManifest.csp is an empty string. (FWIW aManifest.csp is undefined)

Comment 7

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Oh derp. ConfirmInstall when you confirm, we dont have the proper manifest at this point. I think we want _onDownloadPackage.

My guess is that we just need to add the following line 

3065     app.downloading = false;
3066     app.downloadAvailable = false;
+        app.csp=aManifest.csp
3067 
3068     yield this._saveApps();

Or is there some other function that we should be calling here?

(BTW: I checked and aManifest has the right csp value at this point)

Comment 8

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Just made a build with this change, and testing using my csp-test app from marketplace, it does now get the csp correctly applied. Do you think this is the right way to fix it, or does processManifestForIds need to be called?

Comment 9

[[:fabrice] Fabrice Desré]

I think we could move that to updateAppHandlers() but your fix should work too. One thing we absolutely need though is a new test.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

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