949549 - CSP errors/warnings aren't displayed in the app manager

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Bill Walker [:bwalker] [@wfwalker]]

Attachment: lux.zip, app folder containing html and app manifests

Steps to reproduce:

1. Open attached folder as packaged app in app manager using privileged app version of manifest
2. attach to geeksphone peak running 1.2, update and debug app
3. see that console.log works when pressing buttons
4. remove app from phone and app manager
5. Open attached folder as packaged app in app manager using certified app version of manifest
6. attach to geeksphone peak running 1.2, update and debug app
7. see that console.log does NOT work when pressing app buttons

expected results: in (7) console.log works
actual results: in (7) console.log does not work

Comment 1

[Bill Walker [:bwalker] [@wfwalker]]

see also https://developer.mozilla.org/en-US/Firefox_OS/Using_the_App_Manager#Debugging_Certified_Apps

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Other tools have issues in certified mode too, for example the Debugger shows no sources, but they are there if you remove the "type".

I don't think I know enough about what certified mode implies to keep investigating here.  Maybe Alex knows more...?

Comment 3

[Alexandre Poirot [:ochameau]]

I don't see what would prevent tools from working...
The only thing I can think is something broken in webapps actor with all the checks for certified apps pref.

Comment 4

[Alexandre Poirot [:ochameau]]

Oh I looked at the app source code and I just forgot about the CSP!
So your inlined JS code in the html won't work in a certified app because of the default CSP for certified app is more strict than privileged one!

Still, we have something bad here. We don't display CSP warnings, I think there is some displayed in adb logcat. It would help figuring this behavior. So I'll update bug title accordingly.
Bug 821877 tends to say that errors should be in the web console... may be we miss during app startup.

More info here:
https://developer.mozilla.org/en-US/Apps/CSP#Default_Policies
https://developer.mozilla.org/en-US/Marketplace/Publishing/Packaged_apps#Types_of_packaged_apps

Comment 5

[Alexandre Poirot [:ochameau]]

So after some investigation I found that nothing is even displayed in logcat,
unless you set security.csp.debug pref to true.
And even with this pref set, it will only show up in logcat.
CSP are supposed to be sent to the document related web console,
unfortunately, it only works partially, from what I can see in CSP source code,
it seems to be only working on HTTP requests:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#2687
Here scanRequestData() pass a null value for packaged app hosted on app:// protocol.
The request given to scanRequestData allows CSP code to later identify the related window
and send the console message with the window ID.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Ensure that CSP warnings reach webconsole for document hosted on app:// protocol.

One issue was that the nsIContentSecurityPolicy interface
only accepted nsIHttpChannel for scanRequestData.
So that this function will receive null for document hosted
on protocol different than http://, like app://...
The issue with that is that we later use this channel,
(actually, we mainly use the nsIRequest interface)
to figure out to which window this request was related.
That to ensure creating nsIScriptError messages related
to a given inner window ID.
The webconsole finally listen for these messages
via the console service and filter messages by window id.

Comment 7

[Alexandre Poirot [:ochameau]]

Comment on attachment 8349713 [details] [diff] [review]
Ensure that CSP warnings reach webconsole for document hosted on app:// protocol.

Daniel, Would you be a good candidate for reviewing such patch? If not, please forward it to whoever makes sense.
See previous comment for more detail about this modification.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8349713 [details] [diff] [review]
Ensure that CSP warnings reach webconsole for document hosted on app:// protocol.

Punting review to Sid.

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8349713 [details] [diff] [review]
Ensure that CSP warnings reach webconsole for document hosted on app:// protocol.

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

Can you easily write a test for this change?  Something that doesn't currently work because scanRequestData fails to scan the channel...

::: content/base/public/nsIContentSecurityPolicy.idl
@@ +155,5 @@
>    /**
>     * Called after the CSP object is created to fill in the appropriate request
>     * and request header information needed in case a report needs to be sent.
>     */
> +  void scanRequestData(in nsIChannel aChannel);

CSP doesn't make sense for some types of channels... especially those without a principal or referrer.

If you make this change, you need the implementation of ContentSecurityPolicy to assert that the channel is one of the expected types (nsIHttpChannel or whatever app:// uses).

Comment 10

[Alexandre Poirot [:ochameau]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #9)
> Comment on attachment 8349713 [details] [diff] [review]
> Ensure that CSP warnings reach webconsole for document hosted on app://
> protocol.
> 
> Review of attachment 8349713 [details] [diff] [review]:
> -----------------------------------------------------------------
> CSP doesn't make sense for some types of channels... especially those
> without a principal or referrer.
> 
> If you make this change, you need the implementation of
> ContentSecurityPolicy to assert that the channel is one of the expected
> types (nsIHttpChannel or whatever app:// uses).

app:// requests spawn only nsIChannel, so you won't have a referrer.
But you will still get a principal, as nsIScriptSecurityManager.getChannelPrincipal accepts a nsIChannel.
So I'm not sure we should ensure that the argument is more than just a nsIChannel.
And if we do assert something, it is not very clear what we should assert.
The only thing I can think of is to ensure that getChannelPrincipal returned a valid principal.

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: new patch with test

Testing it wasn't that simple as app:// channel can't be instanciated on xpcshell
that easily. You have to setup various things in order to ensure that Webapps.jsm
works. All these things:
  http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/apps/tests/unit/head_apps.js#70

So I prefered writing a mochitest instead.

https://tbpl.mozilla.org/?tree=Try&rev=76a4b9ec3184

Comment 12

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8359921 [details] [diff] [review]
new patch with test

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

::: content/base/test/mochitest.ini
@@ +580,5 @@
>  [test_xhr_send_readystate.html]
>  [test_xhr_withCredentials.html]
>  [test_file_from_blob.html]
>  [test_warning_for_blocked_cross_site_request.html]
> +[test_bug949549.html]

Looks like this file didn't get "hg add"ed to the patch.  Can you upload a new patch with the test file?

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: path with (hg added) test

Oops. Here is the test file, and a new try:
  https://tbpl.mozilla.org/?tree=Try&rev=6dfc2bb878a5

Comment 14

[Alexandre Poirot [:ochameau]]

Attachment: patch with working tests

So writing such test was even more complex,
as app:// channel only works if the URL matches an App url.
Otherwise it creates a DummyChannel that throws during scanRequestData...

https://tbpl.mozilla.org/?tree=Try&rev=0089e56270af

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: patch with green tests

Ok, so my previous patch had tests only working on b2g,
this one also works on desktop:
  https://tbpl.mozilla.org/?tree=Try&rev=f634ffd8ba0e

Comment 16

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8361085 [details] [diff] [review]
patch with green tests

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

Looks good!

r=me with these minor comments addressed.  Also, run this through try on B2G to make sure it works (the last try run including b2g seems to have gone orange and may not have included the test file).

Please move the test into content/base/test/csp.

::: content/base/test/test_bug949549.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Test for Bug 166235</title>

This bug # needs to be updated.

@@ +15,5 @@
> +  // Ensure that `scanRequestData` doesn't throw with app:// URLs
> +
> +  var Cc = SpecialPowers.Cc;
> +  var Ci = SpecialPowers.Ci;
> +  var Services = SpecialPowers.Services;

You only use each of these once, so no need to declare extra vars (just inline their values).

@@ +20,5 @@
> +
> +  var csp = Cc["@mozilla.org/contentsecuritypolicy;1"]
> +              .createInstance(Ci.nsIContentSecurityPolicy);
> +
> +  var gManifestURL = "http://www.example.com/chrome/dom/tests/mochitest/webapps/apps/basic.webapp";

This should probably be a const.

@@ +23,5 @@
> +
> +  var gManifestURL = "http://www.example.com/chrome/dom/tests/mochitest/webapps/apps/basic.webapp";
> +
> +  SimpleTest.waitForExplicitFinish();
> +  var launchableValue, app;

can launchableValue be declared inside setupTest, or does it need to be in this outer scope?

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: final patch

https://tbpl.mozilla.org/?tree=Try&rev=684f4ac86ee5

Comment 18

[Alexandre Poirot [:ochameau]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #16)
> Comment on attachment 8361085 [details] [diff] [review]
> @@ +23,5 @@
> > +
> > +  var gManifestURL = "http://www.example.com/chrome/dom/tests/mochitest/webapps/apps/basic.webapp";
> > +
> > +  SimpleTest.waitForExplicitFinish();
> > +  var launchableValue, app;
> 
> can launchableValue be declared inside setupTest, or does it need to be in
> this outer scope?

It is being used (as `app`), later, in cleanup function.

Comment 19

[Alexandre Poirot [:ochameau]]

Attachment: final patch

Forgot to move the test to csp sub folder...

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/fc0d812a1856

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fc0d812a1856