CSP: Deduplicate violation reports before sending.

CSP: Deduplicate violation reports before sending.

Violation reports should be sent once and only once per page load. If a
single line of code generates the same report over and over again, we
should attempt to avoid spamming the report server with not-particularly
valuable duplicates.

For example, if a report-only policy blocks 'eval()', then the following
loop would make a sysadmin somewhere quite unhappy:

    for (i=0;i<Number.MAX_VALUE;i++)
      eval(...);

This patch adds a HashSet<unsigned> to ContentSecurityPolicy, and stores
the hash of the stringified violation report. We check that set just
before handing things off to PingLoader for delivery. If there's a
match, we've already sent the report, and can safely discard it. If not
we send the report, then add it to the list.

Discussed on public-webappsec@w3.org: http://lists.w3.org/Archives/Public/public-webappsec/2013Aug/0000.html

BUG=267453
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155708

[Mike West, 2013-08-02 09:42:44]

Hi Adam, Tom.

I have no idea how to test this change. We don't really have a good mechanism
for intercepting multiple violation reports for a single page load, which makes
it difficult to test that a page generates only one report.

I'd love ideas. :)

-mike

[Tom Sepez, 2013-08-02 18:03:22]

Nice.

https://codereview.chromium.org/21789002/diff/1/Source/core/page/ContentSecur...
File Source/core/page/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/21789002/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:1777: return;
Maybe count number of times we are suppressed here, with console.log() on n == 1
with a message along the lines of "Duplicate violation reports being
suppressed".  That might solve the test case implementation without being too
noisy on the developers looking at the console (though slightly noisy and the
purists will still be annoyed).

https://codereview.chromium.org/21789002/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:1784:
didSendViolationReport(reportObject);
Seems like a shame to have to hash the same string twice, once in shouldSend()
and again in didSend().

https://codereview.chromium.org/21789002/diff/1/Source/core/page/ContentSecur...
File Source/core/page/ContentSecurityPolicy.h (right):

https://codereview.chromium.org/21789002/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.h:157: HashSet<unsigned>
m_violationReportsSent;
Any chance of making this a 64-bit unsigned? Not much downside to a collision,
but still ... guess that depends on whether we have a 64-bit hash function
available.

[abarth-chromium, 2013-08-02 18:04:28]

https://codereview.chromium.org/21789002/diff/1/Source/core/page/ContentSecur...
File Source/core/page/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/21789002/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:1779: RefPtr<FormData> report =
FormData::create(reportObject->toJSONString().utf8());
Rather than calling toJSONString() three times (once here, in
shouldSendViolationReport, and in didSendViolationReport), we should create the
string once around line 1775 and use it in all three places.

https://codereview.chromium.org/21789002/diff/1/Source/core/page/ContentSecur...
File Source/core/page/ContentSecurityPolicy.h (right):

https://codereview.chromium.org/21789002/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.h:157: HashSet<unsigned>
m_violationReportsSent;
There's an "AlreadyHashed" parameter that you can add to the HashSet template
args to let the collection know that the unsigned values are already
well-distributed.  There should be examples around.

[Tom Sepez, 2013-08-02 18:11:40]

https://codereview.chromium.org/21789002/diff/1/Source/core/page/ContentSecur...
File Source/core/page/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/21789002/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:1900: bool
ContentSecurityPolicy::shouldSendViolationReport(PassRefPtr<JSONObject> report)
const
Maybe add a comment that we don't care about collisions to prevent some eager
person down the road from pointing this out in some bug.

[Mike West, 2013-08-05 08:21:28]

https://codereview.chromium.org/21789002/diff/1/Source/core/page/ContentSecur...
File Source/core/page/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/21789002/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:1777: return;
On 2013/08/02 18:03:22, Tom Sepez wrote:
> Maybe count number of times we are suppressed here, with console.log() on n ==
1
> with a message along the lines of "Duplicate violation reports being
> suppressed".  That might solve the test case implementation without being too
> noisy on the developers looking at the console (though slightly noisy and the
> purists will still be annoyed). 

I think I'd be annoyed. :)

What do you think about adding a "didDispatchPingLoader" callback to the
FrameLoaderClient and piping it down into the test results? That's a bunch of
busywork, but it would give us something visible in the test expectations
without writing warnings to the console that developers won't reasonably be
expected to do anything with.

https://codereview.chromium.org/21789002/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:1779: RefPtr<FormData> report =
FormData::create(reportObject->toJSONString().utf8());
On 2013/08/02 18:04:28, abarth wrote:
> Rather than calling toJSONString() three times (once here, in
> shouldSendViolationReport, and in didSendViolationReport), we should create
the
> string once around line 1775 and use it in all three places.

Done.

https://codereview.chromium.org/21789002/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:1784:
didSendViolationReport(reportObject);
On 2013/08/02 18:03:22, Tom Sepez wrote:
> Seems like a shame to have to hash the same string twice, once in shouldSend()
> and again in didSend().

Done.

https://codereview.chromium.org/21789002/diff/1/Source/core/page/ContentSecur...
Source/core/page/ContentSecurityPolicy.cpp:1900: bool
ContentSecurityPolicy::shouldSendViolationReport(PassRefPtr<JSONObject> report)
const
On 2013/08/02 18:11:40, Tom Sepez wrote:
> Maybe add a comment that we don't care about collisions to prevent some eager
> person down the road from pointing this out in some bug.

Done.

[Mike West, 2013-08-05 09:02:38]

PingLoader callback CL is up for review at
https://codereview.chromium.org/22159002. I think that's a better, more general
purpose solution.

[Mike West, 2013-08-07 07:02:41]

Uploaded a new patchset that tests multiple reports. Mind taking another look?

[abarth-chromium, 2013-08-07 18:01:02]

lgtm

[commit-bot: I haz the power, 2013-08-07 18:01:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/21789002/16001

[commit-bot: I haz the power, 2013-08-07 19:30:21]

Change committed as 155708