712859 - Add a way to help figure out what CSP is doing (CSP violation errors only appear in the error console)

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

So, CSP is pretty cool. Would be handy if the devtools had a way to help figure out what CSP is blocking/allowing.

Comment 1

[Justin Dolske [:Dolske]]

Even just lumping it into Page Info (*shudder*) would be a fine start. AFAIK this isn't user-visible anywhere yet?

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

CSP already dumps warnings and errors into the JS/error console (shift-ctrl-j)... and if you set "security.csp.debug" to true, you get a bunch more info there.  Those error messages got a little better with the result of bug 600584, but they are what they are and could surely use some design/polish to make them better for devs.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Sid Stamm [:geekboy] from comment #2)
> CSP already dumps warnings and errors into the JS/error console
> (shift-ctrl-j)... and if you set "security.csp.debug" to true, you get a
> bunch more info there.  Those error messages got a little better with the
> result of bug 600584, but they are what they are and could surely use some
> design/polish to make them better for devs.

At the very least we could show these errors in the web console, if we're not doing it already. Furthermore, it might be a good idea to indicate forbidden content in the inspector (fonts, images) and in the debugger (scripts), assuming there's some way to retrieve this information. A suitable indicator could be a stricken through text, a red font, an exclamation mark or an icon with an explanatory tooltip.

Comment 4

[Rob Campbell [:rc] (:robcee)]

yeah, this seems like a fine place to put this sort of information. Sid, Dolske, do you have any test pages for CSP we could use to check the content of the Web Console?

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

There are a bunch in the unit (mochi) tests, but there's this one Brandon set up too:
http://people.mozilla.org/~bsterne/content-security-policy/demo.cgi

Comment 6

[Rob Campbell [:rc] (:robcee)]

ah, that's cool.

With JS messages enabled in the web console we see:

[13:10:18.549] call to eval() blocked by CSP @ http://people.mozilla.org/~bsterne/content-security-policy/tests/script/eval-script-test.js:2

Unfortunately there Error Console reports a lot more. We should at least replicate what's in that list.

e.g., Timestamp: 2011-12-22 13:12:06
Warning: CSP: 'allow' or 'default-src' directive required but not present.  Reverting to "default-src 'none'"

Comment 8

[Paul [:pmac] McLanahan]

+1

Just spent a while trying to figure out why Firefox was ignoring a script tag (tabzilla js). Would have been extremely helpful if this showed in the web console instead of the only indication being its absence from the list under Net.

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

Hey Mark, are you still hacking on this?

Comment 10

[Mark Goodwin [:mgoodwin]]

I am, yes.

Comment 12

[Mark Goodwin [:mgoodwin]]

Attachment: Initial kick at getting CSP violation information in the web developer console.

This currently only deals with CSP violations (not issues with the policy itself). I'll get around to that, but first I need to resolve bug 766569.

Comment 13

[Ian Melven :imelven]

Comment on attachment 634957 [details] [diff] [review]
Initial kick at getting CSP violation information in the web developer console.

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

Looks pretty good to me overall - please create patches with the standard settings in your .hgrc as discussed on irc :)

::: content/base/src/CSPUtils.jsm
@@ +51,5 @@
>  
>  };
>  
>  
> +function CSPWarning(aMsg, windowID, aSource, aScriptSample, aLineNum) {

nit : should be aWindowID like the other args

::: content/base/src/contentSecurityPolicy.js
@@ +24,1 @@
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");

curious, why is the gre omitted here (and should there be 3 slashes ? )

@@ +106,5 @@
>  
> +  get windowID() {
> +    let win = NetworkHelper.getWindowForRequest(this._docRequest);
> +    let winUtils = win.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
> +    return winUtils.currentInnerWindowID;

can getting the window for the request ever fail ? can the QI fail ?

Comment 14

[Mark Goodwin [:mgoodwin]]

(In reply to Ian Melven :imelven from comment #13)
> Looks pretty good to me overall - please create patches with the standard
> settings in your .hgrc as discussed on irc :)

These should be OK; I checked my .hgrc:

[defaults]
# ...
diff = -p -U 8
qdiff = -p -U 8

> curious, why is the gre omitted here (and should there be 3 slashes ? )

Ah, that's because I was trying to use devtools stuff in core. See new patch.
 
> can getting the window for the request ever fail ? can the QI fail ?

Yes. See new patch.

Comment 15

[Mark Goodwin [:mgoodwin]]

Attachment: show CSP policy violation messages in the web developer console (take 2)

Comment 16

[Ian Melven :imelven]

Comment on attachment 635685 [details] [diff] [review]
show CSP policy violation messages in the web developer console (take 2)

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

Looks good to me with some minor nits - Mounir will likely have more authoritative suggestions
on what the correct style is :)

::: content/base/src/contentSecurityPolicy.js
@@ +109,5 @@
> +    let loadContext = null;
> +    if (this._docRequest && this._docRequest.notificationCallbacks) {
> +      try {
> +        loadContext = this._docRequest.notificationCallbacks.getInterface(Ci.nsILoadContext);
> +      } catch (ex) { }

i think Mozilla style is 

try {
  ...
} catch (ex) {
}

@@ +112,5 @@
> +        loadContext = this._docRequest.notificationCallbacks.getInterface(Ci.nsILoadContext);
> +      } catch (ex) { }
> +    } else {
> +      if (this._docRequest && this._docRequest.loadGroup
> +                   && this._docRequest.loadGroup.notificationCallbacks) {

if (this._docRequest && this._docRequest.loadGroup && 
    this._docRequest.loadGroup.notificationCallbacks) {

@@ +116,5 @@
> +                   && this._docRequest.loadGroup.notificationCallbacks) {
> +        try {
> +          loadContext = this._docRequest.loadGroup.notificationCallbacks.getInterface(Ci.nsILoadContext);
> +        } catch (ex) { }
> +      }

same style nit as above

@@ +122,5 @@
> +    if (loadContext) {
> +      win = loadContext.associatedWindow;
> +    }
> +    if (win) {
> +      try{

missing space after try

@@ +125,5 @@
> +    if (win) {
> +      try{
> +         let winUtils = win.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
> +         winID = winUtils.currentInnerWindowID;
> +      } catch (ex) { }

as above

Comment 17

[:Gavin Sharp [email: [e-mail]]]

drive by nit: The windowID getter could be simplified with an early return if !this._docRequest (and subsequent early returns if !loadContext or !win).

Comment 18

[Mounir Lamouri (:mounir)]

Comment on attachment 635685 [details] [diff] [review]
show CSP policy violation messages in the web developer console (take 2)

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

I'm not familiar with our CSP code. Boris might be.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 635685 [details] [diff] [review]
show CSP policy violation messages in the web developer console (take 2)

The code in the windowID getter is wrong: it should fall back to the loadgroup if the getInterface on the notificationCallbacks throws.  So it would make more sense like so, imo:

  try {
    loadContext = this._docRequest
                      .notificationCallbacks.getInterface(Ci.nsILoadContext);
  } catch (ex) {
    try {
      loadContext = this._docRequest.loadGroup
                        .notificationCallbacks.getInterface(Ci.nsILoadContext);
    } catch (ex) {
    }
  }

with no other null-checks.  r=me with that change.

Comment 20

[Mark Goodwin [:mgoodwin]]

Attachment: show CSP policy violation messages in the web developer console (take 3)

Tweaked some formatting (in response to Ian's nits) and added some early returns (in response to Gavin's).

Comment 21

[Mark Goodwin [:mgoodwin]]

(In reply to Boris Zbarsky (:bz) from comment #19)
> Comment on attachment 635685 [details] [diff] [review]
> show CSP policy violation messages in the web developer console (take 2)
> 
> The code in the windowID getter is wrong: it should fall back to the
> loadgroup if the getInterface on the notificationCallbacks throws.  So it
> would make more sense like so, imo:
> 
>   try {
>     loadContext = this._docRequest
>                       .notificationCallbacks.getInterface(Ci.nsILoadContext);
>   } catch (ex) {
>     try {
>       loadContext = this._docRequest.loadGroup
>                        
> .notificationCallbacks.getInterface(Ci.nsILoadContext);
>     } catch (ex) {
>     }
>   }
> 
> with no other null-checks.  r=me with that change.

Ah, missed that. I'll sort that out now.

Comment 22

[Mark Goodwin [:mgoodwin]]

Attachment: show CSP policy violation messages in the web developer console (take 4)

Modified as per Boris' suggestion to fall back to the loadgroup.  Also renamed windowID to innerWindowID to better reflect usage elsewhere.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 636418 [details] [diff] [review]
show CSP policy violation messages in the web developer console (take 4)

r=me

Comment 24

[Mark Goodwin [:mgoodwin]]

Comment on attachment 636418 [details] [diff] [review]
show CSP policy violation messages in the web developer console (take 4)

hg.mozilla.org/try/pushloghtml?changeset=c3baa99944ef - I think we're good to go

Comment 25

[Mozilla RelEng Bot]

Try run for c3baa99944ef is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c3baa99944ef
Results (out of 222 total builds):
    success: 194
    warnings: 27
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c3baa99944ef

Comment 26

[Sid Stamm [:geekboy or :sstamm]]

Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/29d7b50bc993

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/29d7b50bc993

Comment 28

[Mark Goodwin [:mgoodwin]]

(In reply to Ed Morley [:edmorley] from comment #27)
> https://hg.mozilla.org/mozilla-central/rev/29d7b50bc993

Reopening; the above only addresses violation information. I also need to address issues with broken directives and report POST failures.

Comment 29

[:Gavin Sharp [email: [e-mail]]]

Can you do that in another bug? Tracking multiple landings in one bug is generally not a good idea.

Comment 30

[Mark Goodwin [:mgoodwin]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #29)
> Can you do that in another bug? Tracking multiple landings in one bug is
> generally not a good idea.

Done; tracking directive and report failures in bug 770099.