770099 - Send CSP policy and report information to Web Console

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Mark Goodwin [:mgoodwin]]

We now have CSP directive violations in the Web Console (see bug 712859); the same needs to be done for CSP Policy issues and reporting errors.

Comment 1

[Mark Goodwin [:mgoodwin]]

Attachment: Send CSP policy and report information to Web Console (take 1)

First kick.

I'm not 100% happy with this approach; CSPUtils is exporting a fair amount of stuff that feels like it should be grouped together. Would it make sense to try to tidy this up somehow or are you happy with things as they are?

I've got more to do here but I thought I'd get some feedback on direction before pursuing this further.

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 638347 [details] [diff] [review]
Send CSP policy and report information to Web Console (take 1)

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

I think maybe it make more sense to encapsulate the CSP{Error,Warning,debug} functions within a CSPRep instance.  This way we could avoid passing in the window ID each time (instead just calling cspr.PostError("msg") or whatever we call it), and we wouldn't have to export the new function you've created.  I did a quick search and it looks like only these two files (CSPUtils.jsm and contentSecurityPolicy.js) are the ones that consume these exported functions and both operate on CSPRep objects.

::: content/base/src/CSPUtils.jsm
@@ +205,5 @@
>    var SD = CSPRep.SRC_DIRECTIVES;
>    var UD = CSPRep.URI_DIRECTIVES;
>    var aCSPR = new CSPRep();
>    aCSPR._originalText = aStr;
> +  this._docRequest = docRequest;

This is a factory method, not a constructor, so you'll want to avoid "this" and instead use "aCSPR".  Same throughout the rest of CSPRep.fromString().

@@ +231,5 @@
>            aCSPR._allowInlineScripts = true;
>          else if (opt === "eval-script")
>            aCSPR._allowEval = true;
>          else
> +          CSPWarning(CSPLocalizer.getFormatStr("doNotUnderstandOption", [opt]), InnerWindowFromRequest(this._docRequest));

I recommend calling InnerWindowFromRequest(docRequest) at the top of this function to grab the inner window ID... then just pass that in instead of calling the InnerWindowFromRequest repeatedly throughout this function.

@@ +538,5 @@
>      var SD = CSPRep.SRC_DIRECTIVES;
>      var defaultSrcDir = this._directives[SD.DEFAULT_SRC];
>      if (!defaultSrcDir) {
> +      CSPWarning(CSPLocalizer.getStr("allowOrDefaultSrcRequired"),
> +                 InnerWindowFromRequest(this._docRequest));

Would it make sense to store the InnerWindowID instead of the docRequest?  The request might not live forever, and we probably don't want to keep a handle to it even after the channel closes.

@@ +1507,5 @@
>  };
>  
>  //////////////////////////////////////////////////////////////////////
>  
> +function InnerWindowFromRequest(docRequest){

If you export this, please prefix it with CSP so it's named like the other exported functions.

Comment 3

[Mark Goodwin [:mgoodwin]]

Attachment: Send CSP policy and report information to Web Console (take 2)

I have:
1) addressed previous feedback
2) ensured all other warnings and errors now go to the Web Console.

A (slight) area of unease is that if, for some reason, a CSPSourceList or CSPSource doesn't have a reference to the policy it belongs to, a message might get lost. This shouldn't happen though.

Comment 4

[Mark Goodwin [:mgoodwin]]

(In reply to Mark Goodwin [:mgoodwin] from comment #3)
> Created attachment 639138 [details] [diff] [review]

I am working on a set of mochitests to verify warnings are finding their way to the Web Developer Console. I note there are currently no tests to ensure warnings find their way to the error console, but as the point of this bug is to provide this functionality (and I've replaced the old CSPError and CSPWarning functions), this seems appropriate.

Comment 5

[Mark Goodwin [:mgoodwin]]

Having spoken to Mihai about this, I think it makes sense to keep the mochitests as a separate patch; Attachment #639138 [details] [diff] relates to core and the web console tests, being dependent on Web Console, belong in devtools.

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 639138 [details] [diff] [review]
Send CSP policy and report information to Web Console (take 2)

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

Starting to look really good.  Watch your line length.... should be < 80 if possible (wrap long ones).  I think I'd like to see another iteration.

::: content/base/src/CSPUtils.jsm
@@ -89,1 @@
>  function CSPdebug(aMsg) {

What about CSPdebug?  From a quick grep, it looks like it's only called by contentSecurityPolicy.js and this file... does it make sense to also encapsulate this?

@@ +211,5 @@
>      // ALLOW DIRECTIVE //////////////////////////////////////////////////
>      // parse "allow" as equivalent to "default-src", at least until the spec
>      // stabilizes, at which time we can stop parsing "allow"
>      if (dirname === CSPRep.ALLOW_DIRECTIVE) {
> +      var dv = CSPSourceList.fromString(dirvalue, aCSPR._policy, self, true);

Hm, I think you want aCSPR here, not aCSPR._policy. Right now you're passing in a string and not a CSPRep (aCSPR._policy is a string).  This should be the same for all calls to fromString (pass in the rep).

@@ +482,5 @@
>                             && aCSPRep.allowsEvalInScripts;
>  
>      newRep._allowInlineScripts = this.allowsInlineScripts 
>                             && aCSPRep.allowsInlineScripts;
> +    newRep._innerWindowID = this._innerWindowID ? this._innerWindowID : aCSPRep._innerWindowID;

Nit: line too long, please wrap or use if().

@@ +552,5 @@
> +    var consoleMsg = Components.classes["@mozilla.org/scripterror;1"]
> +                               .createInstance(Components.interfaces.nsIScriptError);
> +    consoleMsg.initWithWindowID(textMessage, aSource, aScriptLine, aLineNum, 0,
> +                                Components.interfaces.nsIScriptError.warningFlag,
> +                                "Content Security Policy", this._innerWindowID);

Hm, we should probably skip the consoleMsg if there's no innerWindowID, right?

@@ +596,5 @@
>   * Factory to create a new CSPSourceList, parsed from a string.
>   *
>   * @param aStr
>   *        string rep of a CSP Source List
> + * @param aPolicy

This parameter name should reflect its type, e.g., "aCSPRep" or "aCSP".

@@ +621,4 @@
>    }
>  
>    var slObj = new CSPSourceList();
> +  slObj._policy = aPolicy;

Again, the variable should reflect the type.  _policy is used in the IDL as a string representation of the policy... maybe change _policy to _myCSPRep or something.  (You'll probably have to rename my poorly named _policy in contentSecurityPolicy.js too).

@@ +827,5 @@
>   *  - CSPSource (clone)
>   * @param aData 
>   *        string, nsURI, or CSPSource
> + * @param aPolicy
> + *        The policy this source belongs to

Policy or CSPRep?  I think you want CSPRep here.

@@ +836,5 @@
>   *        appropriate values to inherit when they are omitted from the source.
>   * @returns
>   *        an instance of CSPSource
>   */
> +CSPSource.create = function(aData, aPolicy, self, enforceSelfChecks) {

If you put "aPolicy" at the end of the list of arguments, callers can simply leave it out and it will be undefined within the function.  You won't have to modify as many of the tests if you do this.

@@ +1277,5 @@
>      else if (that._port === this._port)
>        newSource._port = this._port;
>      else {
> +      if(this._policy) {
> +        this._policy.error(CSPLocalizer.getFormatStr("notIntersectPort", [this.toString(), that.toString()]));

What happens if there's an slObj that doesn't have a reference to a CSPRep, or if this._policy is null?  We should still fire an error, just not display it in the web console.  Perhaps it would make sense to rig this to call (new CSPRep()).warn() if there's no reference to the parent CSPRep.

Another option would be to add warn and error to CSPSource.prototype and have those do a work-around if there's no CSPRef reference (as there isn't in the tests).

Comment 7

[Mark Goodwin [:mgoodwin]]

Attachment: Some mochitests

Mihai, is this the right approach for webconsole tests?

Comment 8

[Mihai Sucan [:msucan]]

Comment on attachment 643881 [details] [diff] [review]
Some mochitests

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

Test looks good but I am not sure about the expected multiple .webconsole-msg-warn messages. You could do hud.jsterm.clearOutput() at the beginning of both testPolicyURIMessage() and testViolationMessage() functions, such that you only expect one warning message in each test. Should simplify the test.

Please let me know if you have any questions. Thanks!

::: browser/devtools/webconsole/test/browser_webconsole_bug_770099_bad_policyuri.js
@@ +39,5 @@
> +    {
> +      name: "CSP policy URI warning displayed successfully",
> +      validatorFn: function() {
> +        let i = undefined;
> +        let nodes = aOutputNode.querySelectorAll(".webconsole-msg-warn");

How many csp warnings do you expect here to be displayed in this specific test?

If you expect only one, then use querySelector() and you don't need to loop over any list.

@@ +54,5 @@
> +      successFn: function() {
> +        //tests on the urlnode
> +        let i = undefined;
> +        let node = undefined;
> +        let nodes =  aOutputNode.querySelectorAll(".webconsole-msg-warn");

Same concern here.

@@ +72,5 @@
> +function testViolationMessage(){ 
> +  var aOutputNode = hud.outputNode;
> +  setTimeout(function() {
> +    content.document.body.innerHTML = '<script>alert(123)</script>' ;
> +  },1000);

Please do not use timeouts.

@@ +76,5 @@
> +  },1000);
> +
> +  var img = content.document.createElement('img');
> +  img.src = 'http://some.example.com/test.png'
> +  content.document.body.appendChild(img);

Why isn't this in the html page you load? Does this img element need to be added from the test?

@@ +83,5 @@
> +    {
> +      name: "CSP policy URI warning displayed successfully",
> +      validatorFn: function() {
> +        let i = undefined;
> +        let nodes = aOutputNode.querySelectorAll(".webconsole-msg-warn");

Same question about the number of warnings you'd expect.

@@ +101,5 @@
> +      successFn: function() {
> +        //tests on the urlnode
> +        let i = undefined;
> +        let node = undefined;
> +        let nodes =  aOutputNode.querySelectorAll(".webconsole-msg-warn");

Same concern.

@@ +117,5 @@
> +
> +      failureFn: finishTest,
> +    }
> +  );
> +  testPolicyURIMessage();

waitForSuccess() is async, which means it immediately returns and both functions will execute simultaneously. I suggest you call testPolicyURIMessage() after testViolationMessage() completes (in successFn).

@@ +120,5 @@
> +  );
> +  testPolicyURIMessage();
> +}
> +
> +function testSeverity(node) {

Is this used anywhere?

::: browser/devtools/webconsole/test/test_bug_770099_bad_policy_uri.html
@@ +2,5 @@
> +<html>
> +<head>
> +  <meta charset="UTF-8">
> +  <title>Test for Bug 770099 - bad policy-uri</title>
> +</head>

Please add the public domain license boilerplate to the header.

Comment 9

[Mark Goodwin [:mgoodwin]]

Attachment: Some mochitests

Split files into a file per test, removed debug, settimeout, etc.

Comment 10

[Mark Goodwin [:mgoodwin]]

Attachment: Some mochitests

Also added license boilerplate to html

Comment 11

[Mihai Sucan [:msucan]]

Comment on attachment 647949 [details] [diff] [review]
Some mochitests

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

The policy test always fails for me:

TEST-PASS | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_webconsole_bug_770099_bad_policyuri.js | CSP policy URI warning displayed successfully
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_webconsole_bug_770099_bad_policyuri.js | Test timed out

Giving r+ with the comments below addressed. Please make sure you always run the tests. Thank you!

::: browser/devtools/webconsole/test/browser_webconsole_bug_770099_bad_policyuri.js
@@ +45,5 @@
> +      },
> +
> +      successFn: function() {
> +        //tests on the urlnode
> +        let i = undefined;

is this used?

@@ +47,5 @@
> +      successFn: function() {
> +        //tests on the urlnode
> +        let i = undefined;
> +        let node =  aOutputNode.querySelector(".webconsole-msg-error");
> +        if(node.textContent && (-1 != node.textContent.indexOf('Can\'t fetch policy'))) {

You want "can't" not "Can't". This test always fails.

@@ +49,5 @@
> +        let i = undefined;
> +        let node =  aOutputNode.querySelector(".webconsole-msg-error");
> +        if(node.textContent && (-1 != node.textContent.indexOf('Can\'t fetch policy'))) {
> +          ok(true,"we got a CSP message!");
> +          finishTest();

What if the indexOf test fails? no call to finishTest()?

I suggest you always call finishTest().


You can do something easier to read:

isnot(node.textContent.indexOf("foo"), -1, "CSP message content");

::: browser/devtools/webconsole/test/browser_webconsole_bug_770099_violation.js
@@ +49,5 @@
> +        let i = undefined;
> +        let node =  aOutputNode.querySelector(".webconsole-msg-warn");
> +        if(node.textContent && (-1 != node.textContent.indexOf('violated'))) {
> +          ok(true,"we got a CSP message!");
> +          finishTest();

same suggestions as for the other test file.

::: browser/devtools/webconsole/test/test_bug_770099_bad_policy_uri.html
@@ +8,5 @@
> +</head>
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=770099">Mozilla Bug 770099</a>
> +<p id="display"></p>
> +<div id="content" style="display: none">

Are these two empty elements needed?

::: browser/devtools/webconsole/test/test_bug_770099_violation.html
@@ +13,5 @@
> +</div>
> +<pre id="test">
> +<script type="application/javascript">
> +
> +</script>

Same question.

Comment 12

[Mark Goodwin [:mgoodwin]]

(In reply to Mihai Sucan [:msucan] from comment #11)
> You want "can't" not "Can't". This test always fails.

Aha, I had a patch for bug 770176 in my queue...

Comment 13

[Mark Goodwin [:mgoodwin]]

Attachment: Some mochitests

Resolved issues above

Comment 14

[Mark Goodwin [:mgoodwin]]

Attachment: Send CSP policy and report information to Web Console (take 3)

Comment 15

[Mark Goodwin [:mgoodwin]]

(In reply to Sid Stamm [:geekboy] from comment #6)
> What about CSPdebug?  From a quick grep, it looks like it's only called by
> contentSecurityPolicy.js and this file... does it make sense to also
> encapsulate this?

I don't think so. I want to make sure we can still get CSPDebug if there's no windowID or if something goes wrong in setting CSPRep on CSPSources, etc.

> Hm, we should probably skip the consoleMsg if there's no innerWindowID,
> right?

Again, I'd rather the messages still appear in the JS error console if there's a problem with the innerWindowID. I've added a path for no innerWindowID and explicitly called init instead.


> If you put "aPolicy" at the end of the list of arguments, callers can simply
> leave it out and it will be undefined within the function.  You won't have
> to modify as many of the tests if you do this.

I would rather aPolicy was required (I can't think of a reason why you'd want these without a CSPRep associated) which is why I've not left it at the end of the arg. list and why it's not listed as optional in the docs - there were some existing tests and I wasn't sure of the correct way to mock up a CSPRep when needed so I left it undefined.

> Perhaps it would make sense to rig this to call (new
> CSPRep()).warn() if there's no reference to the parent CSPRep.

I've gone for this option

Comment 16

[Mihai Sucan [:msucan]]

Comment on attachment 648127 [details] [diff] [review]
Some mochitests

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

Looks good, giving r+. Thanks for your updates!

Please make sure the patch does not have any trailing whitespaces.

::: browser/devtools/webconsole/test/Makefile.in
@@ +105,5 @@
>  	browser_webconsole_bug_704295.js \
>  	browser_webconsole_bug_658368_time_methods.js \
>  	browser_webconsole_bug_622303_persistent_filters.js \
> +        browser_webconsole_bug_770099_bad_policyuri.js \
> +        browser_webconsole_bug_770099_violation.js \

nit: these two new files do not align with the others.

@@ +185,5 @@
>  	test-file-location.js \
>  	test-bug-658368-time-methods.html \
>  	test-webconsole-error-observer.html \
>  	test-for-of.html \
> +        test_bug_770099_violation.html \

same here.

::: browser/devtools/webconsole/test/browser_webconsole_bug_770099_bad_policyuri.js
@@ +44,5 @@
> +        return aOutputNode.querySelector(".webconsole-msg-error");
> +      },
> +
> +      successFn: function() {
> +        //tests on the urlnode

nit: his is not an urlnode.

::: browser/devtools/webconsole/test/browser_webconsole_bug_770099_violation.js
@@ +44,5 @@
> +        return aOutputNode.querySelector(".webconsole-msg-warn");
> +      },
> +
> +      successFn: function() {
> +        //tests on the urlnode

ditto.

Comment 17

[Mark Goodwin [:mgoodwin]]

Attachment: Send CSP policy and report information to Web Console (take 4) - now with less bitrot

Comments as per previous comment 15.

Comment 18

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 655014 [details] [diff] [review]
Send CSP policy and report information to Web Console (take 4) - now with less bitrot

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

Overall, looks good, so I am getting nit-picky.  Most of my comments are style-related, but there is one thing I'd like to see changed:

Factor out the calls to error() and warning() ... this is the pattern you use for checking if there's a CSPR object before calling CSPR.error() and creating a new one when there's not.  Make it a global function and then call it from everywhere to trigger error() and warn() to minimize the number of replicas of "if(aCSPR) { } else { new CSPR() }".

This is really close!  Please fix the nits and change that, and flag me one last time.

::: content/base/src/CSPUtils.jsm
@@ +10,5 @@
>   * separate file for testing purposes.
>   */
>  
>  const Cu = Components.utils;
> +const Ci = Components.interfaces;

If you're gonna make this shortcut, please update as many instances of Components.interfaces in this file to be Ci instead.

@@ +515,5 @@
>  
>      newRep._allowInlineScripts = this.allowsInlineScripts 
>                             && aCSPRep.allowsInlineScripts;
>  
> +    newRep._innerWindowID = this._innerWindowID ? 

Nit: please remove trailing whitespace from this line.

@@ +566,5 @@
>     */
>    get allowsInlineScripts () {
>      return this._allowInlineScripts;
>    },
> +  

Nit: there are spaces on this blank line that need removing.

@@ +585,5 @@
> +
> +    var consoleMsg = Components.classes["@mozilla.org/scripterror;1"]
> +                               .createInstance(Ci.nsIScriptError);
> +    if(this._innerWindowID) {
> +      consoleMsg.initWithWindowID(textMessage, aSource, aScriptLine, aLineNum, 

Nit: trailing whitespace.

@@ +590,5 @@
> +                                  0, Ci.nsIScriptError.warningFlag,
> +                                  "Content Security Policy",
> +                                  this._innerWindowID);
> +    }
> +    else {

Nit: please put else keyword on same line as previous close brace to follow the main style used in this file.

@@ +646,5 @@
>   *
>   * @param aStr
>   *        string rep of a CSP Source List
> + * @param aCSPRep
> + *        the CSPRep to which this souce list belongs

Can aCSPRep be null?  If so, mention that here and explain what using "null" causes.

@@ +889,5 @@
>   *  - CSPSource (clone)
>   * @param aData 
>   *        string, nsURI, or CSPSource
> + * @param aCSPRep
> + *        The policy this source belongs to

Nit: this is not a "policy", but a CSPRep.  Also mention the thing about passing in null for this if "null" is allowable.

@@ +922,5 @@
>   *
>   * @param aURI
>   *        nsIURI rep of a URI
> + * @param aCSPRep
> + *        The policy this source belongs to

Same with this as previous comment about a aCSPRep parameter.

@@ +1008,5 @@
>   *
>   * @param aStr
>   *        string rep of a CSP Source
> + * @param aCSPRep
> + *        the policy this CSPSource belongs to

Same with this as previous comment about a aCSPRep parameter.

@@ +1080,5 @@
>      if (!portMatch) {
>        // gets the default port for the given scheme
>        defPort = Services.io.getProtocolHandler(sObj._scheme).defaultPort;
>        if (!defPort) {
> +        if(aCSPRep){

Nit: space after "if" and between ) and { for style consistency.  Please check this throughout your changes.

@@ +1336,5 @@
> +      if(this._CSPRep) {
> +        this._CSPRep.error(msg);
> +      } else {
> +        (new CSPRep()).error(msg);
> +      }

I'm seeing lots of instances of this pattern.  Can we abstract it out into a function that maps a CSPRep value (or null) and a message onto what to do?

If you abstract that out as a pair of top-level functions inside this module (not exported, but can be used throughout this file), you can use those functions for all errors and warnings -- including the calls to .error() that don't have the else clause (see line 1040).

@@ +1581,5 @@
>  };
>  
>  //////////////////////////////////////////////////////////////////////
>  
> +function InnerWindowFromRequest(docRequest) {

Nit: Please begin the function name with lower case.

@@ +1591,5 @@
> +  } catch (ex) {
> +    try {
> +      loadContext = docRequest.loadGroup.notificationCallbacks.getInterface(Ci.nsILoadContext);
> +    } catch (ex) {
> +    }

You don't do anything with the exception here... put a comment in the catch block to say why (or handle it). Same with the other empty catch block in this function.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Mark & Sid, can we get this in for Fx17? I'd like to land the patch for bug 602006 but this is blocking it.

Comment 20

[Mark Goodwin [:mgoodwin]]

(In reply to Jared Wein [:jaws] from comment #19)
> I'd like to land the patch for bug 602006 but this is blocking it.

Are you sure nothing else needs the Error Console? It might be worth checking with the devtools people to make sure there's nothing (else) outstanding.

Comment 21

[Mark Goodwin [:mgoodwin]]

Attachment: Send CSP policy and report information to Web Console (take 5)

All issues in comment 18 addressed.

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 662232 [details] [diff] [review]
Send CSP policy and report information to Web Console (take 5)

Looks good, addresses all sid's previous review comments. r=dveditz

Comment 23

[Mark Goodwin [:mgoodwin]]

Attachment: Some mochitests

Comment 24

[Mark Goodwin [:mgoodwin]]

Attachment: Send CSP policy and report information to Web Console (take 6)

Now with less bitrot

Comment 25

[Mark Goodwin [:mgoodwin]]

Attachment: Some mochitests (now with less trailing spaces)

Comment 26

[Mark Goodwin [:mgoodwin]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=1fbac9f37db0

Comment 27

[Mihai Sucan [:msucan]]

Comment on attachment 666173 [details] [diff] [review]
Some mochitests (now with less trailing spaces)

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

Looks good. Thank you Mark!

::: browser/devtools/webconsole/test/browser_webconsole_bug_770099_bad_policyuri.js
@@ +3,5 @@
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + *
> + * Contributor(s):
> + *  Mark Goodwin <[e-mail]>

Unfortunately, there is no contributors license section. This is legacy stuff.

@@ +14,5 @@
> +
> +let hud = undefined;
> +
> +function test() {
> +  addTab("data:text/html,Web Console CSP bad policy URI test");

To avoid needless warnings from showing up in the error console and in the test run output, please add ;charset=utf8 to the data URI: data:text/html;charset=utf8,....

(some of these comments apply to the other files as well)

@@ +34,5 @@
> +  testPolicyURIMessage();
> +}
> +
> +function testPolicyURIMessage() {
> +  var aOutputNode = hud.outputNode;

nit: let outputNode?

@@ +45,5 @@
> +      },
> +
> +      successFn: function() {
> +        //tests on the urlnode
> +        let node =  aOutputNode.querySelector(".webconsole-msg-error");

nit: double space after =

Comment 28

[Mark Goodwin [:mgoodwin]]

Attachment: Some mochitests (nits addressed)

(In reply to Mihai Sucan [:msucan] from comment #27)
> Comment on attachment 666173 [details] [diff] [review]
> nit: <snip/>

Nits addressed - can I carry the r+?

Comment 29

[Mihai Sucan [:msucan]]

(In reply to Mark Goodwin [:mgoodwin] from comment #28)
> Created attachment 666373 [details] [diff] [review]
> Some mochitests (nits addressed)
> 
> (In reply to Mihai Sucan [:msucan] from comment #27)
> > Comment on attachment 666173 [details] [diff] [review]
> > nit: <snip/>
> 
> Nits addressed - can I carry the r+?

Yes.

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 666044 [details] [diff] [review]
Send CSP policy and report information to Web Console (take 6)

Still looks good, r=dveditz

Comment 31

[Mark Goodwin [:mgoodwin]]

Comment on attachment 666373 [details] [diff] [review]
Some mochitests (nits addressed)

Thanks Mihai

Comment 32

[Joe Walker [:jwalker]]

Green on try:
https://tbpl.mozilla.org/?tree=Try&rev=1fbac9f37db0

Landing on inbound:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=84e28049280d

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1f0367f9f1b6
https://hg.mozilla.org/mozilla-central/rev/84e28049280d