1010953 - CSP warnings say a resource was blocked even if the header is just report only

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Frederik Braun [:freddy]]

The CSP messages in the devtools say that a resource was blocked even when it was not (e.g., in report only mode).

This is likely due to my change for CSP messages in bug 607067 :/

(Screenshot: http://i.imgur.com/3R8CPZ1.png)

Comment 1

[Garrett Robinson [:grobinson]]

> This is likely due to my change for CSP messages in bug 607067 :/

I don't think so, it looks like we never differentiated the console messages for violations for CSP vs. CSP-RO.

This should be an easy fix:

1. add some strings to csp.properties with language for report-only headers
2. distinguish CSP from CSP-RO in CSPUtils.jsm::logToConsole by checking the _reportOnlyMode flag on the CSPRep

Do you think have time to tackle this freddyb? Otherwise I can take it, but won't get to it until next week.

Additionally, we'll need to fix this in the new implementation, which is landing very soon.

Comment 2

[Frederik Braun [:freddy]]

Ah, I couldn't find logToConsole in CSPUtils.jsm. It is in contentsecuritypolicy.js! :)
I will report back with a first attempt quite soon.

Comment 3

[Frederik Braun [:freddy]]

Attachment: bug-1010953.patch

This should solve it.
I have tested it locally and it seems to work quite well.
try push: https://tbpl.mozilla.org/?tree=Try&rev=b1406a452ab4

I'm not super happy with the string and open to suggestions.

Comment 4

[Frederik Braun [:freddy]]

CCing dolske and flod since they provided some valuable feedback on the strings in bug 607067, which was quite similar.

Comment 5

[Frederik Braun [:freddy]]

Attachment: bug-1010953.patch

Same patch, using `let` instead of `var`.

Comment 6

[Frederik Braun [:freddy]]

try looks good. no need to re-test with let instead of var ;)

Comment 7

[Garrett Robinson [:grobinson]]

Comment on attachment 8424831 [details] [diff] [review]
bug-1010953.patch

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

r=me with comments addressed.

::: content/base/src/contentSecurityPolicy.js
@@ +644,2 @@
>      } else {
> +      let localizeString = policy._reportOnlyMode ? "CSPROViolation" : "CSPViolation"

Nit: both lines should have a semicolon at the end

::: dom/locales/en-US/chrome/security/csp.properties
@@ +11,5 @@
>  # %2$S is the URI of the resource which violated the directive.
>  CSPViolationWithURI = The page's settings blocked the loading of a resource at %2$S ("%1$S").
> +# LOCALIZATION NOTE (CSPROViolation):
> +# %1$S is the reason why the resource has not been loaded.
> +CSPROViolation = The page's settings observed the loading of a resource ("%1$S"). A CSP report is being sent.

Suggested language: "A violation occurred for a report-only CSP policy. The behavior was allowed, and a CSP report was sent." Note that "loading of a resource" might be confusing for CSPROViolation because it can refer to other violations that do not have to do with resource loads, such as inline script/style blocking. It makes more sense for CSPROViolationWithURI.

Comment 8

[Frederik Braun [:freddy]]

Attachment: bug-1010953.patch

Addressing the string suggestion while carrying over r+ from grobinson.

Comment 9

[Frederik Braun [:freddy]]

Please check in.
A previous version (different localization string) has successfully run on try yesterday, see here: https://tbpl.mozilla.org/?tree=Try&rev=b1406a452ab4

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d29cede016cc

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d29cede016cc

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Freddy, thanks for filing the bug and providing the patch. I just chatted with Garret and we think it would be great to have a testcase for that change though. I think we should have a file loading with 2 policies (one CSP and and CSP-report-only), you can simply load 2 scripts, one that is allowed by CSP and one only by CSP-RO; afterwards just check that we correctly log to the console.

Freddy, any chance you want to write the test as well?

Comment 13

[Frederik Braun [:freddy]]

It seems the test would be very similar to http://dxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/test/browser_webconsole_bug_770099_violation.js wouldn't it?
I will provide the test if you file the bug and assign it to me, deal? ;)

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Frederik Braun [:freddyb] from comment #13)
> It seems the test would be very similar to
> http://dxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/
> test/browser_webconsole_bug_770099_violation.js wouldn't it?
> I will provide the test if you file the bug and assign it to me, deal? ;)

Indeed that would be very similar to that testcase.
And, it's a deal, here is the bugnumber: 1013507

Thanks!