Last Comment Bug 712859 - Add a way to help figure out what CSP is doing (CSP violation errors only appear in the error console)
: Add a way to help figure out what CSP is doing (CSP violation errors only app...
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: Firefox 16
Assigned To: Mark Goodwin [:mgoodwin]
:
Mentors:
https://developer.mozilla.org/en/Secu...
: 727132 760847 (view as bug list)
Depends on: 766569
Blocks: 602006
  Show dependency treegraph
 
Reported: 2011-12-21 19:38 PST by Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not)
Modified: 2012-07-02 13:14 PDT (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Initial kick at getting CSP violation information in the web developer console. (6.12 KB, patch)
2012-06-20 09:51 PDT, Mark Goodwin [:mgoodwin]
ian.melven: feedback+
Details | Diff | Splinter Review
show CSP policy violation messages in the web developer console (take 2) (6.31 KB, patch)
2012-06-22 04:03 PDT, Mark Goodwin [:mgoodwin]
bzbarsky: review+
ian.melven: feedback+
Details | Diff | Splinter Review
show CSP policy violation messages in the web developer console (take 3) (6.52 KB, text/plain)
2012-06-25 11:04 PDT, Mark Goodwin [:mgoodwin]
no flags Details
show CSP policy violation messages in the web developer console (take 4) (6.15 KB, patch)
2012-06-25 11:48 PDT, Mark Goodwin [:mgoodwin]
bzbarsky: review+
mozbugs: checkin+
Details | Diff | Splinter Review

Description Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-12-21 19:38:48 PST
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] 2011-12-21 19:42:12 PST
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] 2011-12-21 19:55:07 PST
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 [:past] (away until 7/21) 2011-12-22 02:03:20 PST
(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) 2011-12-22 08:12:48 PST
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] 2011-12-22 08:56:36 PST
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) 2011-12-22 09:13:39 PST
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 7 Panos Astithas [:past] (away until 7/21) 2012-02-15 02:10:23 PST
*** Bug 727132 has been marked as a duplicate of this bug. ***
Comment 8 Paul [:pmac] McLanahan 2012-04-06 13:57:16 PDT
+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] 2012-06-13 09:37:38 PDT
Hey Mark, are you still hacking on this?
Comment 10 Mark Goodwin [:mgoodwin] 2012-06-14 03:05:31 PDT
I am, yes.
Comment 11 James Socol [:jsocol, :james] 2012-06-14 06:42:52 PDT
*** Bug 760847 has been marked as a duplicate of this bug. ***
Comment 12 Mark Goodwin [:mgoodwin] 2012-06-20 09:51:39 PDT
Created attachment 634957 [details] [diff] [review]
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 2012-06-21 16:18:26 PDT
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] 2012-06-22 04:01:44 PDT
(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] 2012-06-22 04:03:44 PDT
Created attachment 635685 [details] [diff] [review]
show CSP policy violation messages in the web developer console (take 2)
Comment 16 Ian Melven :imelven 2012-06-22 11:12:40 PDT
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: gavin@gavinsharp.com] 2012-06-22 22:04:33 PDT
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) 2012-06-25 05:22:02 PDT
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 [:bz] 2012-06-25 08:47:35 PDT
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] 2012-06-25 11:04:18 PDT
Created attachment 636393 [details]
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] 2012-06-25 11:06:46 PDT
(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] 2012-06-25 11:48:41 PDT
Created attachment 636418 [details] [diff] [review]
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 [:bz] 2012-06-25 11:53:33 PDT
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] 2012-06-26 07:37:14 PDT
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 2012-06-26 08:00:34 PDT
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/mgoodwin@mozilla.com-c3baa99944ef
Comment 26 Sid Stamm [:geekboy or :sstamm] 2012-06-26 13:36:22 PDT
Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/29d7b50bc993
Comment 27 Ed Morley [:emorley] 2012-06-27 03:35:53 PDT
https://hg.mozilla.org/mozilla-central/rev/29d7b50bc993
Comment 28 Mark Goodwin [:mgoodwin] 2012-06-29 04:47:57 PDT
(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: gavin@gavinsharp.com] 2012-06-29 09:08:13 PDT
Can you do that in another bug? Tracking multiple landings in one bug is generally not a good idea.
Comment 30 Mark Goodwin [:mgoodwin] 2012-07-02 01:31:12 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.

Note You need to log in before you can comment on or make changes to this bug.