+++ This bug was initially created as a clone of Bug #769350 +++ A packaged app cannot define a CSP to protect itself. Since the default CSP for trusted and certified apps is quite loose, we should allow applications to specify a CSP in its manifest. I suspect app developers that are trying to write apps that work as both Firefox apps and Chrome packaged apps will want to set their CSP to the Chrome Packaged App default CSP, to minimize compatibility headaches. Similarly, untrusted apps do not have a default CSP applied to them (currently, at least). Allowing a packaged untrusted app to define a CSP is a useful security measure, and it is also useful to make the transition from untrusted to trusted app less error-prone. We should also figure out if there are other things that packaged apps can't do. Jonas pointed out X-Frame-Options and there's also HSTS, but we think those don't apply to apps as long as bug 773886 is fixed. Developer documentation should encourage application developers to set as restrictive CSP as possible in their apps.

One implementation of this (which is why I added this to Core/Networking) would be for an app:// channel to add the default CSP as a header to the channel, to simulate what we'd get from a http:// channel.

Is this a dupe of https://bugzilla.mozilla.org/show_bug.cgi?id=768029

It is. I'll let Brian do the duping so that he can move over whatever information he feels is helpful.

Jonas says he'll speak with Brian about this so I'm removing the Basecamp nom.

Bug 768029 is about the default CSP to apply to certified apps and trusted apps. This bug is about allowing certified, trusted, and untrusted apps to specify their own CSP in their manifest to merge with the default (if any) CSP.

Besides this being something necessary, and not only for privileged and certified apps (which are the ones that currently have a default CSP applied) this would be great to be able to test Gaia apps selectively with the definitive policy. Since there doesn't seem to have been any work on this since July, we can take this if you want. Oh, and FWIW this should be a blocker. Seems kinda silly that we can specify policies for web apps but there's no way to specify them for installed apps.

I concur with Carmen. This is something that should ship in V1... in fact this is something that should be done ASAP because besides being something useful in the long run, in the short run it would help enormously to adapt Gaia apps to the default CSP individually. As it stands now, before being able to test the changes in any other app, both the system app and homescreen have to be fixed (because otherwise when enabling the full default CSP I just get a blank scren after unlocking).

This feature would only further restrict the default CSP policy, not replace/loosen it.

(In reply to Lucas Adamski from comment #8) > This feature would only further restrict the default CSP policy, not > replace/loosen it. True. And that's exactly what's needed currently to test the apps one by one: a selective way of tightening the policy on a by app basis, leaving the current policy as-is so as to not break all the non-adapted apps. That's short term only though. Mid and long term, without this there's no CSP policy for installed apps at all. Which as I said, is a step backwards from what's on the web.

Ah, I see. So you are saying this is necessary for selective Gaia testing, and also for unprivileged packaged apps (privileged apps should be protected by the default policy). If we can get this done quickly enough to be useful for Gaia that's great, but I don't think we can wait for this to land to start updating Gaia apps. basecamp-blocking+ means that we can't ship without this feature which I don't think is true here, even though I agree this feature is desirable.

Yes, exactly that. We're already updating our apps, but the problem is that currently the update is based on should and should not. There's a lot of "this should work" and "that should not work" without a real way to test it, since testing is all or nothing-- and it usually comes down to nothing. We believe we can get this done fast (hope to get a patch by next week). If it's ok, and if someone's going to have time to review it, I can assign this to myself and try and get a working patch ASAP.

Sure if you want to start working on this that would be great; Sid will be back next week and could help with reviews.

I have this almost done. The actual checking of the CSP was easy to do, but I've run into a small problem. It turns out that appsService->GetAppByLocalId() does return an app but it doesn't clone the manifest (and it's even documented in dom/apps/src/AppsUtils.jsm ). So to get the CSP from the manifest I can do one of (maybe there are more options, but I can't think of them ATM): a) Re-read the manifest on each document load (I *do* have the manifestURL). Easy to do, but doesn't look very efficient. b) Modify AppsUtils.jsm so the manifest is cloned also. Also easy to do but I don't know why it wasn't cloned in the first place though (maybe memory saving? ) c) Add an extra property (csp) to mozIDOMApplication. As mentioned, the first option is easy to do, but it's going to be downright awful in performance. The second option is also easy to do, but as I already mentioned I don't know why the manifest isn't being cloned already. And the third option is probably the best, but also requires changes on way more places, including the interface definition. Before starting making any further changes... what option do you prefer for me to implement?

If the interface isn't closed I for one would prefer adding the application CSP as a property. It would be cleaner, and it would be more efficient.

I'd really rather not change nsIDOMApplication for this. We're exposing the manifest as a property explicitly so that we don't have to expose everything within it. Adding things to the web platform should only be done if we have very strong reasons to do so since it's API that we and a lot of other people will have to support forever. If you want to create an internal helper function which extracts the CSP given an nsIDOMApplication then that's ok though.

(In reply to Jonas Sicking (:sicking) from comment #15) > I'd really rather not change nsIDOMApplication for this. We're exposing the > manifest as a property explicitly so that we don't have to expose everything > within it. Adding things to the web platform should only be done if we have > very strong reasons to do so since it's API that we and a lot of other > people will have to support forever. I had figured as much... good thing I didn't start changing it then :) And yeah, the manifest is a property, but it's a property that isn't being returned when you get an App object from the registry. It does clone the object, and it doesn't fill out that property (it expressely says so on a comment even, so it's let out intentionally). > > If you want to create an internal helper function which extracts the CSP > given an nsIDOMApplication then that's ok though. Hmm... I could implement a GetCSPByAppId and add it to AppsService. I would probably have to change DOMApplicationRegistry in Webapps.jsm too, though, since as I said otherwise I would just get an empty manifest property. If that's an acceptable solution I'll get to it ASAP.

Adding a manifest property to mozIApplication would be ok I think. It's a chrome-only interface.

Asking review from Sid as per Lucas suggestion in C12.

I finally modified AppsService to add a GetCSPByAppId, as I said. I found a nag though: turns out that currently the manifest isn't being read at all. The attribute is there, but it's empty (and not only not being copied). Turns out that webapps.json doesn't have that included. Curiously enough, for installed apps (apps not preinstalled, that is) the manifest *is* being read. So I fixed that too. Now when the registry is populated from webapps.json it reads the manifest also and populates the adequate property in mozIApplication. I don't know if I should have filed another bug for that and make two different patches, though. Let me know if that's the case.

I would like this to land if possible (even if it has to be marked Basecamp blocker for it to camp) because this way Gaia devs can adapt their apps to what will be the definite CSP policy individually. We're probably going to end making a custom build including this patch for our own devs while this lands... and I'm afraid that if this doesn't land in time what will happen will be one of: 1. We'll ship V1 with the current policy, which offers no protection at all against inline XSS 2. If the definitive policy is activated, most of the apps won't work correctly... and we'll end doing either a rush to fix or going back to 1.

Comment on attachment 670049 [details] [diff] [review] Adds a CSP loaded from the manifest file for an app (any kind of app). Review of attachment 670049 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/Webapps.jsm @@ +116,5 @@ > this.webapps[id].appStatus = Ci.nsIPrincipal.APP_STATUS_INSTALLED; > } > + > + // Webapps.json doesn't include the manifest. And we need it > + // (see bug 773891). So this is a good place to read it... We don't want to save the manifests in webapps.json. Also, there is no need to reimplement that with xhr if you need the manifest (there's a _readManifests() function). See my comment https://bugzilla.mozilla.org/show_bug.cgi?id=773891#c17 for how to proceed.

Changing it to use that function now, but meanwhile, there's something I don't understand. C17 only said it was ok to add a manifest property to mozIApplication, but mozIApplication *already* has that property. It just isn't being returned, or even filled.

Removed the loading code. Now it only adds one extra line to save the manifest that's being read already to process the activities.

(In reply to Carmen Jiménez Cabezas from comment #22) > Changing it to use that function now, but meanwhile, there's something I > don't understand. C17 only said it was ok to add a manifest property to > mozIApplication, but mozIApplication *already* has that property. It just > isn't being returned, or even filled. Well, if you only need the CSP, just add a DOMString csp property to mozIApplication, like we added a name property instead of using the full manifest.

(In reply to Fabrice Desré [:fabrice] from comment #24) > (In reply to Carmen Jiménez Cabezas from comment #22) > > Changing it to use that function now, but meanwhile, there's something I > > don't understand. C17 only said it was ok to add a manifest property to > > mozIApplication, but mozIApplication *already* has that property. It just > > isn't being returned, or even filled. > > Well, if you only need the CSP, just add a DOMString csp property to > mozIApplication, like we added a name property instead of using the full > manifest. Er... that was exactly what Carmen proposed in C13... and I actually liked more that option too (on C14) but then Jonas shot that down in C15. So Carmen did as Jonas wanted and let the mozIApplication interface alone. Before doing any more changes to this, are you two (Fabrice and Jonas) ok with changing the mozIApplication interface, or do we let it as it stands on this version? And just out of curiosity, why is manifest a property of mozIApplication if it isn't being populated?

(In reply to Antonio Manuel Amaya Calvo from comment #25) > (In reply to Fabrice Desré [:fabrice] from comment #24) > > (In reply to Carmen Jiménez Cabezas from comment #22) > > > Changing it to use that function now, but meanwhile, there's something I > > > don't understand. C17 only said it was ok to add a manifest property to > > > mozIApplication, but mozIApplication *already* has that property. It just > > > isn't being returned, or even filled. > > > > Well, if you only need the CSP, just add a DOMString csp property to > > mozIApplication, like we added a name property instead of using the full > > manifest. > > Er... that was exactly what Carmen proposed in C13... and I actually liked > more that option too (on C14) but then Jonas shot that down in C15. So > Carmen did as Jonas wanted and let the mozIApplication interface alone. No, what Carmen proposed was to add csp to mozIDOMApplication (which is exposed to content), not to mozIApplication (which is chrome only). > Before doing any more changes to this, are you two (Fabrice and Jonas) ok > with changing the mozIApplication interface, or do we let it as it stands on > this version? I'm ok for changes to mozIApplicatio. > And just out of curiosity, why is manifest a property of mozIApplication if > it isn't being populated? It's there just because it inherits from mozIDOMApplication, but we don't want to carry the manifest around on these objects.

Modified it to store only the CSP manifest value. Instead of modifying mozIApplication interface to add a new property, I store it on the manifest.csp field. That way if the manifest is stored at some point it'll work the same way, and we don't need to add new properties.

Comment on attachment 670302 [details] [diff] [review] Modified so as to not store the whole manifest Review of attachment 670302 [details] [diff] [review]: ----------------------------------------------------------------- Please add the csp property on mozIApplication. I don't want to have a half defined manifest field just for that. There is no issue updating this IDL. ::: dom/apps/src/AppsServiceChild.jsm @@ +68,5 @@ > return AppsUtils.getAppLocalIdByManifestURL(this.webapps, aManifestURL); > }, > > + getCSPByLocalId: function(aLocalId) { > + debug("getCSPByLocalId:" + aLocalId); Nit: indentation is wrong, and you have a trailing whitespace. ::: dom/apps/src/AppsUtils.jsm @@ +94,5 @@ > + debug("getCSPByLocalId " + aLocalId); > + for (let id in aApps) { > + let app = aApps[id]; > + if (app.localId == aLocalId) { > + return (app.manifest && app.manifest.csp)?app.manifest.csp:""; Please add spaces around operators. ::: dom/apps/src/Webapps.jsm @@ +350,5 @@ > this._readManifests(aIds, (function registerManifests(aResults) { > aResults.forEach(function registerManifest(aResult) { > let app = this.webapps[aResult.id]; > let manifest = aResult.manifest; > app.name = manifest.name; why not just add: app.csp = manifest.csp || ""; @@ +355,5 @@ > this._registerSystemMessages(manifest, app); > this._registerActivities(manifest, app); > + if (app.manifest === undefined) > + app.manifest={}; > + app.manifest.csp=aResult.manifest.csp?aResult.manifest.csp:""; This block is #ifdef MOZ_SYS_MSG, so this will not work on some platforms (currently fx desktop and android). @@ +1380,5 @@ > return AppsUtils.getAppByManifestURL(this.webapps, aManifestURL); > }, > > + getCSPByLocalId: function(aLocalId) { > + debug("getCSPByLocalId:" + aLocalId); nit: indentation is wrong, and you have a trailing whitespace.

Comment on attachment 670302 [details] [diff] [review] Modified so as to not store the whole manifest Review of attachment 670302 [details] [diff] [review]: ----------------------------------------------------------------- Someone else should review the webapps code. ::: content/base/src/nsDocument.cpp @@ +2485,5 @@ > + if (appsService) { > + uint32_t appId; > + > + if ( NS_SUCCEEDED(principal->GetAppId(&appId)) ) { > + appsService->GetCSPByLocalId(appId, cspHeaderValue); What if GetAppId returns NO_APP_ID (I think this happens for web pages, APP_STATUS_NOT_INSTALLED). Would the value of cspHeaderValue get overwritten? We don't wanna do that to web pages who send the HTTP header. This code should execute *only* if the document represents an installed app (e.g., applyAppDefaultCSP || appStatus == APP_STATUS_INSTALLED).

I think this addresses all review comments for the last version. Now it works also if MOZ_SYS_MSG is defined and if it isn't defined.

Comment on attachment 670513 [details] [diff] [review] New version, addressing review comments Review of attachment 670513 [details] [diff] [review]: ----------------------------------------------------------------- We're getting close! You're missing a couple of places where you have to set the app.csp property in Webapps.jsm : - in updateHostedApp() - in confirmInstall() I'd like see an updated version. ::: dom/interfaces/apps/mozIApplication.idl @@ +26,5 @@ > /* Name copied from the manifest */ > readonly attribute DOMString name; > + > + /* CSP copied from the manifest */ > + readonly attribute DOMString csp; You also need to update the UUID of the interface.

(In reply to Fabrice Desré [:fabrice] from comment #31) > Comment on attachment 670513 [details] [diff] [review] > New version, addressing review comments > > Review of attachment 670513 [details] [diff] [review]: > ----------------------------------------------------------------- > > We're getting close! > > You're missing a couple of places where you have to set the app.csp property > in Webapps.jsm : > - in updateHostedApp() > - in confirmInstall() Thanks for your comments :) I'll get to that now, but meanwhile... > > + > > + /* CSP copied from the manifest */ > > + readonly attribute DOMString csp; > > You also need to update the UUID of the interface. I don't know how to do this. What do I use to update the UUID? And is it enough to update it on the IDL or do I have to hunt for it on the rest of the files also?

(In reply to Carmen Jiménez Cabezas from comment #32) > > You also need to update the UUID of the interface. > > I don't know how to do this. What do I use to update the UUID? And is it > enough to update it on the IDL or do I have to hunt for it on the rest of > the files also? You can use a local uuid generator (eg. uuigen on linux), or ask on irc.mozilla.org to firebot to generate one for you (firebot: uuid). Here's one you can use: 8ac7827f-f982-40fb-be11-ba16dd665635 There are no other places to look for.

Ok, I'll use that one then. One thing though, I also changed nsIAppsService. Do I need to generate a new uuid for that too?

Added the missing mozIApplication.csp updates and generated a new UUID for mozIApplication and nsIAppsService. Will add an interdiff also.

Comment on attachment 670532 [details] [diff] [review] Adressed the feedback comments Review of attachment 670532 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nit addressed (no need to re-request review) ::: dom/interfaces/apps/nsIAppsService.idl @@ +49,5 @@ > + /** > + * Returns the CSP associated to this localId. > + */ > + DOMString getCSPByLocalId(in unsigned long localId); > + Nit: remove blank lines

This version only changes over the previous one is the nit from Fabrice's review. Requesting review by Sid again for the CSP/nsDocument related changes.

Those are the test applications I used to verify the change. They're just the same small HTML with inline scripts, and the only difference between both is that one doesn't have a CSP property and the other has what will be the definitive CSP value. So one works and the other doesn't.

Comment on attachment 670727 [details] [diff] [review] New version, only with nit changes (removed blank lines) Review of attachment 670727 [details] [diff] [review]: ----------------------------------------------------------------- I would prefer to see some automated tests committed with this patch instead of the manual ones, but the logic in InitCSP() looks good to me. Are you going to write automated tests for this? ::: content/base/src/nsDocument.cpp @@ +2481,5 @@ > + // That way we don't have to change the rest of the function logic > + if (applyAppDefaultCSP || appStatus == nsIPrincipal::APP_STATUS_INSTALLED) { > + nsCOMPtr<nsIAppsService> appsService = > + do_GetService(APPS_SERVICE_CONTRACTID); > + Please remove trailing whitespace from this blank line (and the one two lines below it).

(In reply to Sid Stamm [:geekboy] from comment #41) > Comment on attachment 670727 [details] [diff] [review] > New version, only with nit changes (removed blank lines) > > Review of attachment 670727 [details] [diff] [review]: > ----------------------------------------------------------------- > > I would prefer to see some automated tests committed with this patch instead > of the manual ones, but the logic in InitCSP() looks good to me. Are you > going to write automated tests for this? > Ok, thanks for the review. I will upload a new versión with the requested changes in a little while. I would like to add some automated test too, but I'm afraid I don't know how to do them.

Check out the patch in bug 768029 for how I tested CSP for apps. You'll probably have to add another pre-installed manifest (or tweak the manifests) in automation.py.in to have a CSP, then run a test against an app for that manifest. It's important to add tests for things like this to our test suite so we don't introduce regressions. This is the type of thing I can imagine regressing too since it depends on both nsDocument and the app runtime.

As I said before, and as it came out again on bug 801783, I think we should mark this as blocking-basecamp because additionally to the benefits at mid and long term for installed apps (which currently can't specify any kind of CSP), short term this will be a godsend to allow modifying Gaia apps to comply with the CSP policy.

(In reply to Sid Stamm [:geekboy] from comment #43) > Check out the patch in bug 768029 for how I tested CSP for apps. You'll > probably have to add another pre-installed manifest (or tweak the manifests) > in automation.py.in to have a CSP, then run a test against an app for that > manifest. > > It's important to add tests for things like this to our test suite so we > don't introduce regressions. This is the type of thing I can imagine > regressing too since it depends on both nsDocument and the app runtime. I agree and I think I see how to add the tests for this functionality. I'll do that tomorrow morning (22.30 here already :) ). Do you mind I add them to your test files, or do you prefer if I just copy and modify them? And more important... once I've added the automatic tests, how do I run them? Do a normal make execute the tests?

You can add to the files without bug numbers in the file names -- maybe make new files for the ones that have a bug number in their name (to make it clearer what breaks if these tests break). You can run the tests like a chrome mochitest (https://developer.mozilla.org/en-US/docs/Chrome_tests).

This new version includes all the review changes (there is no change with the previous patch) and includes also automated testing, as requested. On the automated testing I ran into something I believe is a bug on SpecialPowers.pushPrefEnv. The patch ran well if running it stand alone but failed when ran twice, or ran after the test for bug 768029. The cause is that the second time SpecialPowers.pushPrefEnv is called (even on a different page) an exception ("Error getting pref") is raised. I implemented a workaround so the test ran correctly now even when ran after 768029, or ran twice. Note that if for some reason the order is inverted and the test for 768029 is ran after this one, it will fail unless SpecialPowers.pusPrefEnv is fixed.

Comment on attachment 672012 [details] [diff] [review] Patch including all review comments and automated tests Review of attachment 672012 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/test/chrome/test_csp_bug773891.html @@ +221,5 @@ > + ["security.apps.certified.CSP.default", DEFAULT_CSP_CERT]]}, > + function() { gTestRunner.next(); }); > +} catch (e) { > + // This happens for some reason when pushPrefEnv has been called already with those same values... > + // so I'll just set them manually. Note that this is far from safe... Might need to add an onUnload? So having pushPrevEnv in a previous test file is causing this problem? You could reset the prefs in _checkForFinish(). Not resetting could potentially cause issues with future CSP-related tests.

jmaher: can you look at the pushPrefEnv() issues in comment 49? I'm not sure I understand why this is happening.

(In reply to Sid Stamm [:geekboy] from comment #51) > Comment on attachment 672012 [details] [diff] [review] > Patch including all review comments and automated tests > > Review of attachment 672012 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/base/test/chrome/test_csp_bug773891.html > @@ +221,5 @@ > > + ["security.apps.certified.CSP.default", DEFAULT_CSP_CERT]]}, > > + function() { gTestRunner.next(); }); > > +} catch (e) { > > + // This happens for some reason when pushPrefEnv has been called already with those same values... > > + // so I'll just set them manually. Note that this is far from safe... Might need to add an onUnload? > > So having pushPrevEnv in a previous test file is causing this problem? As far as I've seen, yes. It happens if this test is run after your test, or if I load this test twice (or your test twice, for that matter). I think the error I'm getting is launched in [1] when it tries to read the previous value of the preference (for restoring it later?). Since the first time it works I think what's happening is that when it should restore the value it's actually erasing it. But that's a working hyphotesis only since I haven't actually checked it :). > > You could reset the prefs in _checkForFinish(). Not resetting could > potentially cause issues with future CSP-related tests. I can change that if needed, but I didn't do it already because I thought that exception is something that shouldn't happen, and that code should not be neccesary if/when SpecialPowers doesn't launch that exception. [1] https://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js#734

I see in the patch there is a use of specialpowers.pushPrefEnv(...) wrapped in a try/except. In the except clause there is a specialpowers.setBoolPref(...) and a couple other set prefs. Could it be that we are setting these prefs?

No, I added that try/except precisely because the pushPrefEnv was giving me an uncaught exception. Without the try block the test works as long as it's executed just once. If it's executed twice, or if it's executed after the test for bug 768029 (that has a pushPrefEnv for the same preferences), then it gives an exception. The exception block is there as a workaround till pushPrefEnv doesn't launch an exception while setting the var :)

This patch include all the previous review comments, automated tests, and restores the previous values of the preferences upon test exit.

oh, this bug raised an interesting problem, and an easy solution to pushPrefEnv. We were returning an error from pushPrefEnv because the type was a STRING and we were not able to handle that. I don't know if this has changed over time (last 1.5 years) or if for some reason when I implemented this I overlooked the obvious.

Comment on attachment 672389 [details] [diff] [review] fix specialPowers.pushPrefEnv to work with this test case (1.0) Review of attachment 672389 [details] [diff] [review]: ----------------------------------------------------------------- A better fix would be to change http://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js#515 to match the values we send everywhere else.

addressed previous review comment.

What else is needed to land this? I would get to get it landed ASAP if only so we can stop generating custom builds for our devs and also so we can post to dev-gaia how the apps adaptation to CSP can be done.

Assuming: * attachment 672414 [details] [diff] [review] fixes the pushPrefEnv stuff * and we can carry over fabrice's review onto the current patch Then I think we just need to: * remove the try block in the test (see comment 55), * double-check it works (try run, etc) * and land it.

From what I've seen there was no code changes on what Fabrice reviewed or on what you reviewed, the only changes on the two last versions were to add the automatic tests, and to make the automatic test restore the original preference values. And I assume Carmen won't mind removing what she added yesterday if the pushPrefEnv fix works. That said, I don't think the pushPrefEnv fix working should be a condition to land this patch (except that part of course). That was a completely unrelated bug that if it can be fixed quickly here it's great but if it can't should go into it's own bug and not condition the acceptance of this one. After all, it's unrelated to the functionality this patch adds.

The fix to pushPrefEnv is r+ so this can land with this patch as soon as the tests are cleaned up. No drama here.

landed the pushprefenv: https://hg.mozilla.org/integration/mozilla-inbound/rev/0d24747f4648

I'm not including an interdiff this time since I had to rebase Webapps.jsm and interdiff doesn't like that. In any case, the only change between this version and the r+ version is the automated test, as Antonio said the rest of the code hasn't been modified.

Again the same old question :) What needs to be done still to land this?

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0dc54155f65 Unfortunately I missed that the checking comment in the patch wasn't actually describing the patch. For future reference, writing a proper checkin comment helps the person who's checking in your patch quite a bit. Thanks for doing this work though!! Awesome that we'll be shipping with this.

Why is this [leave-open] ?

I think that was from when the pushPrefEnv patch landed and that this can be safely closed now.

https://hg.mozilla.org/releases/mozilla-aurora/rev/efa0e95cdae1 https://hg.mozilla.org/releases/mozilla-aurora/rev/2cfb5d2e385a

What documentation is needed?

(In reply to Carmen Jiménez Cabezas from comment #74) > What documentation is needed? Dev docs is needed here because this is an app manifest property being added here to my understanding, which 3rd party app developers should be able to understand how to use it.

(In reply to Jason Smith [:jsmith] from comment #75) > (In reply to Carmen Jiménez Cabezas from comment #74) > > What documentation is needed? > > Dev docs is needed here because this is an app manifest property being added > here to my understanding, which 3rd party app developers should be able to > understand how to use it. Cool, I'll be happy to provide that. I assume that[1] is the correct place to add this information, isn't it? [1] https://developer.mozilla.org/en-US/docs/Apps/Manifest

(In reply to Carmen Jiménez Cabezas from comment #76) > (In reply to Jason Smith [:jsmith] from comment #75) > > (In reply to Carmen Jiménez Cabezas from comment #74) > > > What documentation is needed? > > > > Dev docs is needed here because this is an app manifest property being added > > here to my understanding, which 3rd party app developers should be able to > > understand how to use it. > > Cool, I'll be happy to provide that. I assume that[1] is the correct place > to add this information, isn't it? > > [1] https://developer.mozilla.org/en-US/docs/Apps/Manifest Yup, that's correct.

I've added the pertinent information to the documentation wiki already. Please check if there's something else missing.

(In reply to Carmen Jiménez Cabezas from comment #78) > I've added the pertinent information to the documentation wiki already. > Please check if there's something else missing. Yup, I see it. I'll puts needsinfo on our dev-docs lead do a review of it. Mark - Can you take a look at the update in the app manifest doc for the addition of the csp field?

(In reply to Jason Smith [:jsmith] from comment #79) > (In reply to Carmen Jiménez Cabezas from comment #78) > > I've added the pertinent information to the documentation wiki already. > > Please check if there's something else missing. > > Yup, I see it. I'll puts needsinfo on our dev-docs lead do a review of it. > Mark - Can you take a look at the update in the app manifest doc for the > addition of the csp field? Yes, that looks good Carmen, thanks.

Is this qa- still? Is there something else I should do?

(In reply to Carmen Jiménez Cabezas from comment #81) > Is this qa- still? Is there something else I should do? Oh. qa- is meant my tracking specifically - that means I'm not going to explicitly verify the bug, mainly because I don't usually do verifications when I see sufficient automation land with the patch. In other words - your good, no worries.

Comment on attachment 673181 [details] [diff] [review] Removed the exception treatment on the tests The Webapps.jsm changes in this bug seem to be broken if MOZ_SYS_MSG is not set (i.e. for desktop firefox builds). I get: Timestamp: 2012 12 07 10:39:25 PM Error: DOMApplicationRegistry: Could not parse JSON: {snip} ReferenceError: manifest is not defined registerManifest@resource://gre/modules/Webapps.jsm:155 and indeed the code in registerAppsHandlers uses an undefined variable "manifest".

Looks like that's bug 818674.