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

RESOLVED FIXED in Firefox 16

Status

()

Firefox
Developer Tools: Console
P3
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Unfocused, Assigned: mgoodwin)

Tracking

Trunk
Firefox 16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

So, CSP is pretty cool. Would be handy if the devtools had a way to help figure out what CSP is blocking/allowing.
Even just lumping it into Page Info (*shudder*) would be a fine start. AFAIK this isn't user-visible anywhere yet?
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.
(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.
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?
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
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'"
Component: Developer Tools → Developer Tools: Console
Priority: -- → P3
QA Contact: developer.tools → developer.tools.console
Version: unspecified → Trunk
Duplicate of this bug: 727132
+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.
(Assignee)

Updated

5 years ago
Assignee: nobody → mgoodwin
Hey Mark, are you still hacking on this?
(Assignee)

Comment 10

5 years ago
I am, yes.
Duplicate of this bug: 760847
Summary: Add a way to help figure out what CSP is doing → Add a way to help figure out what CSP is doing (CSP errors only appear in the error console)
(Assignee)

Updated

5 years ago
Depends on: 766536
(Assignee)

Updated

5 years ago
Depends on: 766569
(Assignee)

Comment 12

5 years ago
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

5 years ago
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 ?
Attachment #634957 - Flags: feedback+
(Assignee)

Comment 14

5 years ago
(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.
(Assignee)

Comment 15

5 years ago
Created attachment 635685 [details] [diff] [review]
show CSP policy violation messages in the web developer console (take 2)
Attachment #634957 - Attachment is obsolete: true
Attachment #635685 - Flags: feedback?(imelven)
(Assignee)

Updated

5 years ago
Attachment #635685 - Flags: review?(mounir)

Comment 16

5 years ago
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
Attachment #635685 - Flags: feedback?(imelven) → feedback+
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 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.
Attachment #635685 - Flags: review?(mounir) → review?(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.
Attachment #635685 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 20

5 years ago
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).
Attachment #635685 - Attachment is obsolete: true
Attachment #636393 - Flags: review?(mounir)
(Assignee)

Comment 21

5 years ago
(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.
(Assignee)

Comment 22

5 years ago
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.
Attachment #636393 - Attachment is obsolete: true
Attachment #636393 - Flags: review?(mounir)
Attachment #636418 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Attachment #636418 - Attachment is patch: true
Comment on attachment 636418 [details] [diff] [review]
show CSP policy violation messages in the web developer console (take 4)

r=me
Attachment #636418 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 24

5 years ago
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
Attachment #636418 - Flags: checkin?(sstamm)

Comment 25

5 years ago
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
Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/29d7b50bc993
Target Milestone: --- → Firefox 16
Attachment #636418 - Flags: checkin?(sstamm) → checkin+
Blocks: 602006
https://hg.mozilla.org/mozilla-central/rev/29d7b50bc993
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 28

5 years ago
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Can you do that in another bug? Tracking multiple landings in one bug is generally not a good idea.
(Assignee)

Comment 30

5 years ago
(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.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Summary: Add a way to help figure out what CSP is doing (CSP errors only appear in the error console) → Add a way to help figure out what CSP is doing (CSP violation errors only appear in the error console)
(Assignee)

Updated

5 years ago
No longer depends on: 766536
You need to log in before you can comment on or make changes to this bug.