CSP: Deduplicate violation reports before sending.

Source/core/page/ContentSecurityPolicy.cpp

Index: Source/core/page/ContentSecurityPolicy.cpp
diff --git a/Source/core/page/ContentSecurityPolicy.cpp b/Source/core/page/ContentSecurityPolicy.cpp
index 941543170e464d2178bc842d0cb8a653fadd9452..2b4511fdb08a92939495f797a3463a102415ba35 100644
--- a/Source/core/page/ContentSecurityPolicy.cpp
+++ b/Source/core/page/ContentSecurityPolicy.cpp
@@ -1723,7 +1723,7 @@ static void gatherSecurityPolicyViolationEventData(SecurityPolicyViolationEventI
     }
 }
 
-void ContentSecurityPolicy::reportViolation(const String& directiveText, const String& effectiveDirective, const String& consoleMessage, const KURL& blockedURL, const Vector<KURL>& reportURIs, const String& header, const String& contextURL, const WTF::OrdinalNumber& contextLine, ScriptState* state) const
+void ContentSecurityPolicy::reportViolation(const String& directiveText, const String& effectiveDirective, const String& consoleMessage, const KURL& blockedURL, const Vector<KURL>& reportURIs, const String& header, const String& contextURL, const WTF::OrdinalNumber& contextLine, ScriptState* state)
 {
     logToConsole(consoleMessage, contextURL, contextLine, state);
 
@@ -1773,10 +1773,15 @@ void ContentSecurityPolicy::reportViolation(const String& directiveText, const S
     RefPtr<JSONObject> reportObject = JSONObject::create();
     reportObject->setObject("csp-report", cspReport.release());
 
+    if (!shouldSendViolationReport(reportObject))
+        return;

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

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). 

[Mike West, 2013/08/05 08:21:29]

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.

+
     RefPtr<FormData> report = FormData::create(reportObject->toJSONString().utf8());

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

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.

[Mike West, 2013/08/05 08:21:29]

Done.

 
     for (size_t i = 0; i < reportURIs.size(); ++i)
         PingLoader::sendViolationReport(frame, reportURIs[i], report, PingLoader::ContentSecurityPolicyViolationReport);
+
+    didSendViolationReport(reportObject);

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

Seems like a shame to have to hash the same string twice, once in shouldSend()
and again in didSend().

[Mike West, 2013/08/05 08:21:29]

Done.

 }
 
 void ContentSecurityPolicy::reportUnsupportedDirective(const String& name) const
@@ -1892,4 +1897,14 @@ bool ContentSecurityPolicy::shouldBypassMainWorld(ScriptExecutionContext* contex
     return false;
 }
 
+bool ContentSecurityPolicy::shouldSendViolationReport(PassRefPtr<JSONObject> report) const

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

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:29]

Done.

+{
+    return !m_violationReportsSent.contains(report->toJSONString().impl()->hash());
+}
+
+void ContentSecurityPolicy::didSendViolationReport(PassRefPtr<JSONObject> report)
+{
+    m_violationReportsSent.add(report->toJSONString().impl()->hash());
 }
+
+} // namespace WebCore