1011058 - CSP in C++: Use different console message to indicate report only mode

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

Bug 1010953 indicates that there is no indicator in the console message that a CSP violation is just REPORT-ONLY. We should fix that for the new CSP implementation.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1011058.patch

Bug 1010953 introduced different console messages for 'report-only' and 'regular' CSP violations. The attached  patch introduces the same separation for the new CSP implementation.

Keep in mind, that the test (introduced in bug 1013507) still does not pass, because the old implementation also prints the default port when reporting a violated url directive, whereas the new implementation only prints the port in case it's not the default port (which makes more sense in my opinion). I updated the test in the patch_queue, which we can take care of once the test splitting patch (bug 988616) landed.

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8428395 [details] [diff] [review]
bug_1011058.patch

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

As much as I don't like bloating the constructor with yet more arguments, I don't immediately see a better appropriate way without passing around references to the nsCSPPolicy instance (yuck).  

This needs test coverage before landing (which means, we'll want to update this patch with any required test changes once bug 988616 lands).

While this all looks good (and r=me on these parts), I'm clearing the review flag because I want to see the patch updated to include test coverage before r+ing it.  Please re-flag me once you incorporate test coverage.

::: content/base/src/nsCSPContext.cpp
@@ +890,5 @@
> +
> +        const char16_t* msg = mReportOnlyFlag ?
> +                              NS_LITERAL_STRING("CSPROViolationWithURI").get() :
> +                              NS_LITERAL_STRING("CSPViolationWithURI").get();
> +        CSP_LogLocalizedStr(msg, params, ArrayLength(params),

since msg is only used once, I think you can just move the ternary operator statement into the args list below.

@@ +947,5 @@
>                                     const nsAString& aScriptSample,
>                                     uint32_t aLineNum)
>  {
> +  NS_ENSURE_ARG_MAX(aViolatedPolicyIndex, mPolicies.Length() - 1);
> +  bool reportOnlyFlag = mPolicies[aViolatedPolicyIndex]->getReportOnlyFlag();

Do we need to declare reportOnlyFlag, or can we just put the call to getReportOnlyFlag into the args list of the constructor below?

@@ +953,4 @@
>    NS_DispatchToMainThread(new CSPReportSenderRunnable(aBlockedContentSource,
>                                                        aOriginalURI,
>                                                        aViolatedPolicyIndex,
> +                                                      reportOnlyFlag,

nit: prefix with "a".

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

Actually, it looks like the browser console tests will catch this:
browser/devtools/webconsole/test/browser_webconsole_bug_1010953_cspro.js

I caught this in bug 949533 comment 6, so this might be enough coverage.  Chris, if you think this is enough test coverage, fix my minors and flag me again for review.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Changes in bug 949533 includes updating the test which we are relying on for this patch to work. Adding dependcy so we can finish this bug as soon as 949533 lands.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> Changes in bug 949533 includes updating the test which we are relying on for
> this patch to work. Adding dependcy so we can finish this bug as soon as
> 949533 lands.

Oh, I forgot to mention that I do think that the devtools test provides enough coverage in terms of testing.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1011058_v2.patch

Addressed Sid's review comments. Once again, the test for this change landed in bug 1013507.

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #2)
> @@ +953,4 @@
> >    NS_DispatchToMainThread(new CSPReportSenderRunnable(aBlockedContentSource,
> >                                                        aOriginalURI,
> >                                                        aViolatedPolicyIndex,
> > +                                                      reportOnlyFlag,
> 
> nit: prefix with "a".

reportOnlyFlag was actually a local variable, hence no "a" as prefix :-) Anyway, the new version does not use the local variable any longer.

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> reportOnlyFlag was actually a local variable, hence no "a" as prefix :-)
> Anyway, the new version does not use the local variable any longer.

obviously.  *facepalm*

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8444821 [details] [diff] [review]
bug_1011058_v2.patch

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

Yep, looks good.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1011058_v3.patch

Adding sstamm as a reviewer and carrying over r+.

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=824b5446963b

Comment 11

[Sid Stamm [:geekboy or :sstamm]]

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

Comment 12

[Carsten Book [:Tomcat]]

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