Closed Bug 949549 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Security, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: bwalker, Assigned: ochameau)

References

Details

Attachments

(2 files, 6 obsolete files)

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
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...?
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.
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
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.
Summary: App Manager can install certified apps but then app JS doesn't run or console.log doesn't work → CSP errors/warnings aren't displayed in the app manager
Assignee: nobody → poirot.alex
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.
Component: Developer Tools: App Manager → Security
Product: Firefox → Core
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.
Attachment #8349713 - Flags: review?(dholbert)
Comment on attachment 8349713 [details] [diff] [review]
Ensure that CSP warnings reach webconsole for document hosted on app:// protocol.

Punting review to Sid.
Attachment #8349713 - Flags: review?(dholbert) → review?(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).
Attachment #8349713 - Flags: review?(sstamm)
(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.
Attached patch new patch with test (obsolete) — Splinter Review
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
Attachment #8359921 - Flags: review?(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?
Attachment #8359921 - Flags: review?(sstamm)
Attached patch path with (hg added) test (obsolete) — Splinter Review
Oops. Here is the test file, and a new try:
  https://tbpl.mozilla.org/?tree=Try&rev=6dfc2bb878a5
Attachment #8349713 - Attachment is obsolete: true
Attachment #8359921 - Attachment is obsolete: true
Attachment #8360017 - Flags: review?(sstamm)
Attached patch patch with working tests (obsolete) — Splinter Review
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
Attachment #8360017 - Attachment is obsolete: true
Attachment #8360017 - Flags: review?(sstamm)
Attachment #8360363 - Flags: review?(sstamm)
Attached patch patch with green tests (obsolete) — Splinter Review
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
Attachment #8360363 - Attachment is obsolete: true
Attachment #8360363 - Flags: review?(sstamm)
Attachment #8361085 - Flags: review?(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?
Attachment #8361085 - Flags: review?(sstamm) → review+
Attached patch final patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=684f4ac86ee5
Attachment #8361085 - Attachment is obsolete: true
Attachment #8362542 - Flags: review+
(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.
Attached patch final patchSplinter Review
Forgot to move the test to csp sub folder...
Attachment #8362542 - Attachment is obsolete: true
Attachment #8362575 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/fc0d812a1856
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/fc0d812a1856
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla29
Duplicate of this bug: 792214
You need to log in before you can comment on or make changes to this bug.