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

NEW
Unassigned

Status

Core Graveyard
DOM: Apps
4 years ago
7 months ago

People

(Reporter: pauljt, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
+++ 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)
(Reporter)

Comment 1

4 years ago
Created attachment 8517905 [details]
snipped from /data/local/webapps.json

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

Comment 2

4 years ago
Created attachment 8517907 [details]
manifest, as downloaded from Marketplace

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

Comment 3

4 years ago
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....
Summary: csp attribute in manifest doesn't actually apply a CSP (when installed via Marketplace) → csp attribute in manifest doesn't actually apply a CSP for packaged apps
(Reporter)

Comment 5

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

Comment 6

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

Comment 7

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

Comment 8

4 years ago
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?
Flags: needinfo?(fabrice)
I think we could move that to updateAppHandlers() but your fix should work too. One thing we absolutely need though is a new test.
Flags: needinfo?(fabrice)

Updated

7 months ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.