909241 - CSP violation reports should be independent of the message in the developer console

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Frederik Braun [:freddy]]

Right now, violation report strings are the same as the strings displayed in the developer console.
If we want to have meaningful messages as well as testsuite compliant reports, this code has to be separated (as discussed in bug 607067, starting at about comment 25).

Comment 1

[Frederik Braun [:freddy]]

Attachment: bug-909241.patch (builds on top of bug 607067)

I have separated the code that emits the report violation from the code that logs to the devconsole (the part about base restrictions was tricky and I'm sure my solution is not the prettiest). Feedback welcome!

I used the W3C test suite to trigger my CSP changes and noticed that this patch also reduces test failures to 1 (yay).

Comment 2

[Frederik Braun [:freddy]]

I pushed bug 607067 & bug 909241 in combination to try:
https://tbpl.mozilla.org/?tree=Try&rev=6920fca145c2

I don't really understand why the "Mochitest 4 opt" tests fail (though it looks a bit too systematical to be caused by somebody else)

Comment 3

[Frederik Braun [:freddy]]

Attachment: bug-909241.patch (builds on top of bug 607067)

Minor changes..

Comment 4

[Frederik Braun [:freddy]]

Attachment: bug-909241.patch

I moved logging back into the main thread, where it was before.
(Thanks to :gabor for explaining the surrounding code.)

Comment 5

[Frederik Braun [:freddy]]

Looks way better on try: https://tbpl.mozilla.org/?tree=Try&rev=9909ca9a4225

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 797142 [details] [diff] [review]
bug-909241.patch

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

Would it make more sense for logViolationDetails to take both the web console string and the violation report string as arguments?  It seems a bit forced to translate the thing sent as a violation report into "dev-friendly" language.  Shouldn't they be similar? 

Please fix the formatting as mentioned below and flag me again for review.

::: content/base/src/contentSecurityPolicy.js
@@ +172,5 @@
>     * @param aLineNum
>     *     source line number of the violation (if available)
>     */
>    logViolationDetails:
> +    function(aViolationType, aSourceFile, aScriptSample, aLineNum) {

Please don't change the indentation level of the function as it makes the diff much harder to read.

@@ +177,5 @@
> +      // allowsInlineScript and allowsEval both return true when report-only mode
> +      // is enabled, resulting in a call to this function. Therefore we need to
> +      // check that the policy was in fact violated before logging any violations
> +      switch (aViolationType) {
> +        case Ci.nsIContentSecurityPolicy.VIOLATION_TYPE_INLINE_STYLE: {

Same here.  Please don't change the indentation style to be different unless you're editing the logic or making it consistent with the rest of the file.  It makes it hard to identify the changes in your patch.  In addition, the case statement bodies don't need braces around them.

@@ +182,5 @@
> +          if (!this._policy.allowsInlineStyles) {
> +            if (CSPRep.SRC_DIRECTIVES_NEW.STYLE_SRC in this._policy._directives) {
> +              var cspContext = CSPRep.SRC_DIRECTIVES_NEW.STYLE_SRC;
> +            }
> +            else {

Most of the file uses "} else {" all on one line, please be consistent with the rest of the file.

@@ +200,5 @@
> +              var cspContext = CSPRep.SRC_DIRECTIVES_NEW.SCRIPT_SRC;
> +            }
> +            else {
> +              var cspContext = CSPRep.SRC_DIRECTIVES_NEW.DEFAULT_SRC;
> +            }

You can rewrite the whole if/else as two lines:
let SD = CSPRep.SRC_DIRECTIVES_NEW;
var cspContext = (SD.SCRIPT_SRC in this._policy._directives) ? SD.SCRIPT_SRC : SD.DEFAULT_SRC;

@@ +205,5 @@
> +            var directive = this._policy._directives[cspContext];
> +            var violatedDirective = cspContext + ' ' + directive.toString();
> +            this._asyncReportViolation('self', null, violatedDirective,
> +              'violated base restriction: Inline Scripts will not execute',
> +              aSourceFile, aScriptSample, aLineNum);

Please line up arguments (same starting column as in the previous version of this function).

@@ +464,5 @@
> +  logToConsole:
> +    function(blockedUri, originalUri, violatedDirective,
> +             aSourceFile, aScriptSample, aLineNum, aObserverSubject) {
> +      switch(aObserverSubject.data) {
> +        case 'violated base restriction: Inline Stylesheets will not apply': {

This patch is a good opportunity to change these strings or make them constants.

Comment 8

[Frederik Braun [:freddy]]

Attachment: bug-909241.patch

And another round...

Comment 9

[Garrett Robinson [:grobinson]]

Comment on attachment 799392 [details] [diff] [review]
bug-909241.patch

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

Drive-by review! I like the direction this is headed, but please see my comments below. Generally this code should be as simple as possible, avoiding unnecessary complexity.

::: content/base/src/contentSecurityPolicy.js
@@ +28,5 @@
>  const WARN_FLAG = Ci.nsIScriptError.warningFlag;
>  const ERROR_FLAG = Ci.nsIScriptError.ERROR_FLAG;
>  
> +const BASE_VIOLATION_STYLE = 'violated base restriction: Inline Stylesheets will not apply';
> +const BASE_VIOLATION_INLINE = 'violated base restriction: Inline Scripts will not execute';

I would name these BASE_VIOLATION_INLINE_STYLE and BASE_VIOLATION_INLINE_SCRIPT to be as clear as possible.

@@ +207,5 @@
> +        var SD = CSPRep.SRC_DIRECTIVES_NEW;
> +        var cspContext = (SD.SCRIPT_SRC in this._policy._directives) ? SD.SCRIPT_SRC : SD.DEFAULT_SRC;
> +        var directive = this._policy._directives[cspContext];
> +        var violatedDirective = cspContext + ' ' + directive.toString();
> +        this._asyncReportViolation('self', null, violatedDirective, BASE_VIOLATION_EVAL,

You should take this chunk of logic that you've repeated 3x and factor it into a function (it should be an "internal function", so prefix it with _). Something like _buildViolatedDirectiveString, with a parameter for the "expected" violated directive.

@@ +447,5 @@
> +   * Logs a meaningful CSP warning to the developer console.
> +   */
> +  logToConsole:
> +    function(blockedUri, originalUri, violatedDirective,
> +             aSourceFile, aScriptSample, aLineNum, aObserverSubject) {

You should do var violatedDirective = null; or similar here (like you do line 463 below). Otherwise unexpected and difficult to debug behavior could result, especially if aObserverSubject.data is somehow not one of the cases you define. See http://stackoverflow.com/questions/1470488/difference-between-using-var-and-not-using-var-in-javascript for background.

@@ +451,5 @@
> +             aSourceFile, aScriptSample, aLineNum, aObserverSubject) {
> +      switch(aObserverSubject.data) {
> +        case BASE_VIOLATION_STYLE: {
> +          violatedDirective = CSPLocalizer.getStr("inlineStyleBlocked");
> +        }

I've never seen {} used around cases in a switch. Does this even work? Since you're not using break statements anywhere, I would expect execution to "fall through" and for all of your violatedDirective's to end up being "scriptFromStringBlocked". Here's more info on the syntax of the switch statement: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/switch

You might consider writing a test for this new behavior, or making sure that the current test suite is working and can catch erroneous cases such as this.

@@ +456,5 @@
> +        case BASE_VIOLATION_INLINE: {
> +          violatedDirective = CSPLocalizer.getStr("inlineScriptBlocked");
> +        }
> +        case BASE_VIOLATION_EVAL: {
> +          violatedDirective = CSPLocalizer.getStr("scriptFromStringBlocked");

Suggestion: there's a perfect correspondence between the BASE_* string constants and the tags for the localized messages. It seems like we could simplify the code if we merged them somehow. Perhaps by simplifying the observerSubjects - I don't know if they need to be the verbose sentences that they are now. Could they be the same as the localized message tags? AFAIK observer subjects are meant to be consumed by code and do not have to be human readable. It might even be an improvement to make them pithy tags like these.

If you did this, you would probably have to change some tests that do regex matching against the observer subjects, but that would not be hard.

I defer to Sid on whether this is a good idea or not.

@@ +458,5 @@
> +        }
> +        case BASE_VIOLATION_EVAL: {
> +          violatedDirective = CSPLocalizer.getStr("scriptFromStringBlocked");
> +        }
> +      }

Nit: I'd put a newline here for readability.

@@ +708,5 @@
>                                   violatedDirective,
>                                   aSourceFile, aScriptSample, aLineNum);
> +        reportSender.logToConsole(blockedContentSource, originalUri,
> +                                  violatedDirective, aSourceFile, aScriptSample,
> +                                  aLineNum, observerSubject);

All of these variables are parameters, but only some of them use Hungarian notation. This is not your fault, but do you think you could fix it in this patch?

Comment 10

[Frederik Braun [:freddy]]

I have a few follow-up questions to some proposed changes. Consider those uncommented accepted :)

(In reply to Garrett Robinson [:grobinson] from comment #9)

> 
> @@ +447,5 @@
> > +   * Logs a meaningful CSP warning to the developer console.
> > +   */
> > +  logToConsole:
> > +    function(blockedUri, originalUri, violatedDirective,
> > +             aSourceFile, aScriptSample, aLineNum, aObserverSubject) {
> 
> You should do var violatedDirective = null; or similar here (like you do
> line 463 below). Otherwise unexpected and difficult to debug behavior could
> result, especially if aObserverSubject.data is somehow not one of the cases
> you define. See
> http://stackoverflow.com/questions/1470488/difference-between-using-var-and-
> not-using-var-in-javascript for background.

Maybe I misunderstand what the code does.. this function is called in different cases. The variable violdatedDirective comes from the calling function and shall only be replaced by a string that mentions
a violation of base restriction, if in fact base restrictions were violated. Otherwise the message will complain about an explicit restriction being violated (as handled in the caller).

> @@ +451,5 @@
> > +             aSourceFile, aScriptSample, aLineNum, aObserverSubject) {
> > +      switch(aObserverSubject.data) {
> > +        case BASE_VIOLATION_STYLE: {
> > +          violatedDirective = CSPLocalizer.getStr("inlineStyleBlocked");
> > +        }
> 
> I've never seen {} used around cases in a switch. Does this even work? Since
> you're not using break statements anywhere, I would expect execution to
> "fall through" and for all of your violatedDirective's to end up being
> "scriptFromStringBlocked". Here's more info on the syntax of the switch
> statement:
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/
> switch
> 
> You might consider writing a test for this new behavior, or making sure that
> the current test suite is working and can catch erroneous cases such as this.

Thanks for spotting this. I clearly just forgot the break statements.

I also had braces around the case statement because a previous version of this used "let" somewhere and I noticed that it's not OK to use the let keyword multiple times for the same identifier in the same block. Even though they might be mutually exclusive (like in some switch/case statements). It took me a while to figure this out and I had to use this snippet to understand: http://jsfiddle.net/AAvPw/ (replacing let with var doesn't work). So I changed all of this to make a bit more sense in my upcoming wip-patch.

I will also make sure to add tests for this behavior. Right now I was using the W3C test suite to make sure things work as intended, but we should have our own tests indeed. 


> 
> @@ +456,5 @@
> > +        case BASE_VIOLATION_INLINE: {
> > +          violatedDirective = CSPLocalizer.getStr("inlineScriptBlocked");
> > +        }
> > +        case BASE_VIOLATION_EVAL: {
> > +          violatedDirective = CSPLocalizer.getStr("scriptFromStringBlocked");
> 
> Suggestion: there's a perfect correspondence between the BASE_* string
> constants and the tags for the localized messages. It seems like we could
> simplify the code if we merged them somehow. Perhaps by simplifying the
> observerSubjects - I don't know if they need to be the verbose sentences
> that they are now. Could they be the same as the localized message tags?
> AFAIK observer subjects are meant to be consumed by code and do not have to
> be human readable. It might even be an improvement to make them pithy tags
> like these.
> 
> If you did this, you would probably have to change some tests that do regex
> matching against the observer subjects, but that would not be hard.
> 
> I defer to Sid on whether this is a good idea or not.
> 

Shouldn't be too hard to implement, as soon as we reached a decision :)


> @@ +708,5 @@
> >                                   violatedDirective,
> >                                   aSourceFile, aScriptSample, aLineNum);
> > +        reportSender.logToConsole(blockedContentSource, originalUri,
> > +                                  violatedDirective, aSourceFile, aScriptSample,
> > +                                  aLineNum, observerSubject);
> 
> All of these variables are parameters, but only some of them use Hungarian
> notation. This is not your fault, but do you think you could fix it in this
> patch?

Just so I don't misunderstand this: The 'a' prefix for arguments is applied to arguments in a function declaration so that I can easily spot variables come from the calling function?

Comment 11

[Garrett Robinson [:grobinson]]

(In reply to Frederik Braun [:freddyb] from comment #10)
> I have a few follow-up questions to some proposed changes. Consider those
> uncommented accepted :)
> 
> (In reply to Garrett Robinson [:grobinson] from comment #9)
> 
> > 
> > @@ +447,5 @@
> > > +   * Logs a meaningful CSP warning to the developer console.
> > > +   */
> > > +  logToConsole:
> > > +    function(blockedUri, originalUri, violatedDirective,
> > > +             aSourceFile, aScriptSample, aLineNum, aObserverSubject) {
> > 
> > You should do var violatedDirective = null; or similar here (like you do
> > line 463 below). Otherwise unexpected and difficult to debug behavior could
> > result, especially if aObserverSubject.data is somehow not one of the cases
> > you define. See
> > http://stackoverflow.com/questions/1470488/difference-between-using-var-and-
> > not-using-var-in-javascript for background.
> 
> Maybe I misunderstand what the code does.. this function is called in
> different cases. The variable violdatedDirective comes from the calling
> function and shall only be replaced by a string that mentions
> a violation of base restriction, if in fact base restrictions were violated.
> Otherwise the message will complain about an explicit restriction being
> violated (as handled in the caller).
> 

You're right. I missed that violatedDirective was a parameter (this is why we use Hungarian notation).

> > @@ +451,5 @@
> > > +             aSourceFile, aScriptSample, aLineNum, aObserverSubject) {
> > > +      switch(aObserverSubject.data) {
> > > +        case BASE_VIOLATION_STYLE: {
> > > +          violatedDirective = CSPLocalizer.getStr("inlineStyleBlocked");
> > > +        }
> > 
> > I've never seen {} used around cases in a switch. Does this even work? Since
> > you're not using break statements anywhere, I would expect execution to
> > "fall through" and for all of your violatedDirective's to end up being
> > "scriptFromStringBlocked". Here's more info on the syntax of the switch
> > statement:
> > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/
> > switch
> > 
> > You might consider writing a test for this new behavior, or making sure that
> > the current test suite is working and can catch erroneous cases such as this.
> 
> Thanks for spotting this. I clearly just forgot the break statements.
> 
> I also had braces around the case statement because a previous version of
> this used "let" somewhere and I noticed that it's not OK to use the let
> keyword multiple times for the same identifier in the same block. Even
> though they might be mutually exclusive (like in some switch/case
> statements). It took me a while to figure this out and I had to use this
> snippet to understand: http://jsfiddle.net/AAvPw/ (replacing let with var
> doesn't work). So I changed all of this to make a bit more sense in my
> upcoming wip-patch.
> 
> I will also make sure to add tests for this behavior. Right now I was using
> the W3C test suite to make sure things work as intended, but we should have
> our own tests indeed. 
> 

Sounds good. Yes, I want our test suite to be on par with the W3C suite eventually :)

> 
> > 
> > @@ +456,5 @@
> > > +        case BASE_VIOLATION_INLINE: {
> > > +          violatedDirective = CSPLocalizer.getStr("inlineScriptBlocked");
> > > +        }
> > > +        case BASE_VIOLATION_EVAL: {
> > > +          violatedDirective = CSPLocalizer.getStr("scriptFromStringBlocked");
> > 
> > Suggestion: there's a perfect correspondence between the BASE_* string
> > constants and the tags for the localized messages. It seems like we could
> > simplify the code if we merged them somehow. Perhaps by simplifying the
> > observerSubjects - I don't know if they need to be the verbose sentences
> > that they are now. Could they be the same as the localized message tags?
> > AFAIK observer subjects are meant to be consumed by code and do not have to
> > be human readable. It might even be an improvement to make them pithy tags
> > like these.
> > 
> > If you did this, you would probably have to change some tests that do regex
> > matching against the observer subjects, but that would not be hard.
> > 
> > I defer to Sid on whether this is a good idea or not.
> > 
> 
> Shouldn't be too hard to implement, as soon as we reached a decision :)
> 

Sid, what do you think about this (reporting short tags like "inlineStyleBlocked" to observers, instead of long strings like "violated base restriction: Inline Stylesheets will not apply")?

> 
> > @@ +708,5 @@
> > >                                   violatedDirective,
> > >                                   aSourceFile, aScriptSample, aLineNum);
> > > +        reportSender.logToConsole(blockedContentSource, originalUri,
> > > +                                  violatedDirective, aSourceFile, aScriptSample,
> > > +                                  aLineNum, observerSubject);
> > 
> > All of these variables are parameters, but only some of them use Hungarian
> > notation. This is not your fault, but do you think you could fix it in this
> > patch?
> 
> Just so I don't misunderstand this: The 'a' prefix for arguments is applied
> to arguments in a function declaration so that I can easily spot variables
> come from the calling function?

That's exactly right. This is more common in C++ code, but it is still useful in Javascript. aSomeVariable is typically used for function (a)rguments.

Comment 12

[Frederik Braun [:freddy]]

Attachment: addressed all review comments except ObserverSubjects

I have addressed everything except changing the ObServerSubject.

Changing them should be fine though: According to MXR the string "violated base restriction:" is never used somewhere else in the codebase. I searched mozilla-central and addons, e.g.: https://mxr.mozilla.org/mozilla-central/search?string=violated%20base%20restriction%3A&filter=^[^\0]*%24

I will now work on some tests for this behavior, since I still hope to get this landed before the next week's merge :)

Comment 13

[Frederik Braun [:freddy]]

The existing tests are unaffected by this patch (see on try <https://tbpl.mozilla.org/?tree=Try&rev=655cdd691f08>).
I think it's mostly because none of the tests currently check the report object. I have filed bug 915065 about this and started digging through the current CSP tests.

Comment 14

[Garrett Robinson [:grobinson]]

Comment on attachment 802855 [details] [diff] [review]
addressed all review comments except ObserverSubjects

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

Looks great! r+ if you make the changes I suggest.

::: content/base/src/contentSecurityPolicy.js
@@ +29,5 @@
>  const ERROR_FLAG = Ci.nsIScriptError.ERROR_FLAG;
>  
> +const BASE_VIOLATION_INLINE_STYLE = 'violated base restriction: Inline Stylesheets will not apply';
> +const BASE_VIOLATION_INLINE_SCRIPT = 'violated base restriction: Inline Scripts will not execute';
> +const BASE_VIOLATION_EVAL = 'violated base restriction: Code will not be created from strings';

Better names for these would {INLINE_SCRIPT,INLINE_STYLE,EVAL}_VIOLATION_OBSERVER_SUBJECT or similar.

@@ +167,5 @@
> +    var SD = CSPRep.SRC_DIRECTIVES_NEW;
> +    var cspContext = (SD[aDirectiveName] in this._policy._directives) ? SD[aDirectiveName] : SD.DEFAULT_SRC;
> +    var directive = this._policy._directives[cspContext];
> +    return cspContext + ' ' + directive.toString();
> +  },

For readability, add newlines surrounding this function.

@@ +190,5 @@
>      switch (aViolationType) {
>      case Ci.nsIContentSecurityPolicy.VIOLATION_TYPE_INLINE_STYLE:
> +      if (!this._policy.allowsInlineStyles) {
> +        var violatedDirective = this._buildViolatedDirectiveString('STYLE_SRC');
> +        this._asyncReportViolation('self', null, violatedDirective, BASE_VIOLATION_INLINE_STYLE,

Nit: you could get even fancier by simply using this._buildViolatedDirectiveString('STYLE_SRC') as the parameter. I prefer this since var violatedDirective isn't used anywhere else. Making this change is up to you.

@@ +443,5 @@
>    /**
> +   * Logs a meaningful CSP warning to the developer console.
> +   */
> +  logToConsole:
> +    function(blockedUri, originalUri, violatedDirective,

Nit: line function up with logToConsole, adjust indentation of parameters accordingly.

@@ +445,5 @@
> +   */
> +  logToConsole:
> +    function(blockedUri, originalUri, violatedDirective,
> +             aSourceFile, aScriptSample, aLineNum, aObserverSubject) {
> +

Nit: unnecessary newline.

Comment 15

[Frederik Braun [:freddy]]

Attachment: bug-909241.patch (final)

Adressed Garrett's comments, carrying over r+ and pushed to try again

Comment 16

[Frederik Braun [:freddy]]

https://tbpl.mozilla.org/?tree=Try&rev=d2c1e305ff1f works.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3eec9be9fcd0

Comment 18

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Garrett Robinson [:grobinson] from comment #11)

> Sid, what do you think about this (reporting short tags like
> "inlineStyleBlocked" to observers, instead of long strings like "violated
> base restriction: Inline Stylesheets will not apply")?

Not that it matters, now that this is landed, but yeah that's fine.  

BTW, this patch caused nontrivial bitrot for my patches in bug 836922 (that I had hoped to land today).  It would have been much easier for me to rebase this patch on top of the ones for bug 836922 and then land them simultaneously.

freddyb: next time can you ping me before setting checkin-needed on a CSP bug in case there's something else more complicated nearly ready to land?

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3eec9be9fcd0

Comment 20

[Frederik Braun [:freddy]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #18)
> BTW, this patch caused nontrivial bitrot for my patches in bug 836922 (that
> I had hoped to land today).  It would have been much easier for me to rebase
> this patch on top of the ones for bug 836922 and then land them
> simultaneously.
> 
> freddyb: next time can you ping me before setting checkin-needed on a CSP
> bug in case there's something else more complicated nearly ready to land?

Sorry; I didn't think of that!
Next time I will check back whether it's safe to land. I didn't mean to ruin your patch :/

Comment 21

[Sid Stamm [:geekboy or :sstamm]]

Oh you didn't ruin my patch, we just had to do it the hard way.  :)  Both landed, it's all good.