Andrea Marchesini [:baku] Assignee
	Description • 6 years ago

We always report a code sample in CSP Violation events, but, by spec, we should do that only if the corresponding directive contains the 'report-sample' keyword.

https://w3c.github.io/webappsec-csp/#grammardef-report-sample

"If directive’s value contains the expression "'report-sample'", then set violation’s sample to the substring of source containing its first 40 characters."

	Andrea Marchesini [:baku] Assignee
	Comment 1 • 6 years ago

This patch is going to break several tests. I'll upload a new version of it when I have the first results from my try push.

	Andrea Marchesini [:baku] Assignee
	Comment 2 • 6 years ago

green on try and a good test coverage.

	Christoph Kerschbaumer [:ckerschb]
	Comment 3 • 6 years ago

Comment on attachment 8989716 [details] [diff] [review]
csp_reportSample.patch

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

You are on the right track here, but the patch needs a few tweaks here and there. Please look at my inline comment. If something is unclear, feel free to ping me on IRC. Thanks for fixing!

::: dom/security/nsCSPParser.cpp
@@ +484,5 @@
>    }
>  
> +  if (CSP_IsKeyword(mCurToken, CSP_REPORT_SAMPLE)) {
> +    mReportSample = true;
> +    return nullptr;

You have to return a new nsCSPKeywordSrc here. Then you can remove mReortSample and what not entirely.

I am pretty sure as things stand right now the CSPParser would log an error to the console. If not, then something is wrong here.

The other thing that should fail is the toString() method, or also the cspJSON.

Please file a bug in DOM:Security "Test that all supported CSP directives have a valid toString method". Within that test we add a Meta CSP with all direcitves supported and then check that the toString method returns all the directives in the CSP.

	Christoph Kerschbaumer [:ckerschb]
	Comment 6 • 6 years ago

Comment on attachment 8989802 [details] [diff] [review]
csp_reportSample.patch

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

Ultimately this looks good. Please incorporate my suggestion. If you want I can take a final look of course.

::: dom/security/nsCSPParser.cpp
@@ +483,5 @@
>    }
>  
> +  if (CSP_IsKeyword(mCurToken, CSP_REPORT_SAMPLE)) {
> +    return new nsCSPReportSampleSrc();
> +  }

if (CSP_IsKeyword(mCurToken, CSP_REPORT_SAMPLE)) {
  new nsCSPKeywordSrc(CSP_UTF16KeywordToEnum(mCurToken));
}

::: dom/security/nsCSPUtils.h
@@ +352,5 @@
> +      : nsCSPKeywordSrc(CSP_REPORT_SAMPLE)
> +    {}
> +
> +    bool isReportSample() const override
> +      { return true; }

If you make the change in nsCSPParser then you can add that method on nsCSPKeywordSrc directly instead of creating a new class here.

	Pulsebot
	Comment 7 • 6 years ago

Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/90f470be3f1d
Implement report-sample support for CSP directives, r=ckerschb

	Bogdan Tara[:bogdan_tara | bogdant]
	Comment 8 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/90f470be3f1d

	Eric Shepherd [:sheppy]
	Comment 9 • 6 years ago

Submitted BCD PR: https://github.com/mdn/browser-compat-data/pull/2761

And mentioned on Firefox 63 for developers.

	Eric Shepherd [:sheppy]
	Comment 10 • 6 years ago

OK, actually, I need to do more here. I had read something that suggested this was just on one directive but obviously that's not correct. Will update the PR shortly.

	Eric Shepherd [:sheppy]
	Comment 11 • 6 years ago

I have updated the default-src page to include `report-sample`, then rerendered all the other CSP directive pages; the ones that use the same set of keywords pull their list from default-src, so they are now all up to date.

BCD still needs some updating but I am getting advice on the best approach.

	Eric Shepherd [:sheppy]
	Comment 12 • 6 years ago

Finishing this work is now covered by mdn/sprints issue 463: https://github.com/mdn/sprints/issues/463