Refactor nonce support.

third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp

Index: third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp
diff --git a/third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp b/third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp
index 45a274056be1cfd24fb2b7ad848dd124e8c9e181..4be6f9763603bad9bd28c876195da4cc1d8f64a3 100644
--- a/third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp
+++ b/third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp
@@ -382,36 +382,15 @@ bool isAllowedByAllWithContext(const CSPDirectiveListVector& policies, const Str
     return isAllowed;
 }
 
-template<bool (CSPDirectiveList::*allowed)(const String&, const WTF::OrdinalNumber&, ContentSecurityPolicy::ReportingStatus, const String& content) const>
-bool isAllowedByAllWithContextAndContent(const CSPDirectiveListVector& policies, const String& contextURL, const WTF::OrdinalNumber& contextLine, ContentSecurityPolicy::ReportingStatus reportingStatus, const String& content)
+template <bool (CSPDirectiveList::*allowed)(const String&, const String&, const WTF::OrdinalNumber&, ContentSecurityPolicy::ReportingStatus, const String& content) const>
+bool isAllowedByAllWithContextAndContent(const CSPDirectiveListVector& policies, const String& contextURL, const String& nonce, const WTF::OrdinalNumber& contextLine, ContentSecurityPolicy::ReportingStatus reportingStatus, const String& content)
 {
     bool isAllowed = true;
     for (const auto& policy : policies)
-        isAllowed &= (policy.get()->*allowed)(contextURL, contextLine, reportingStatus, content);
+        isAllowed &= (policy.get()->*allowed)(contextURL, nonce, contextLine, reportingStatus, content);
     return isAllowed;
 }
 
-template<CSPDirectiveList::NoncePolicyDisposition (CSPDirectiveList::*allowed)(const String&) const>
-bool isAllowedByAllWithNonce(const CSPDirectiveListVector& policies, const String& nonce)
-{
-    bool isExplicitlyAllowed = false;
-    for (const auto& policy : policies) {
-        // TODO(mkwst): We skip report-only policies here, because the result is used more or
-        // less as a bypass in ScriptLoader. If we return true, we don't apply policy, but
-        // we only return true if all policies match. This is a temporary workaround; a
-        // better fix would be to delay the nonce processing until such time as the whitelist
-        // processing fails. https://crbug.com/611652
-        if (policy.get()->headerType() == ContentSecurityPolicyHeaderTypeEnforce) {
-            CSPDirectiveList::NoncePolicyDisposition policyDisposition = (policy.get()->*allowed)(nonce);
-            if (policyDisposition == CSPDirectiveList::NoncePolicyDisposition::Denied)
-                return false;
-            if (policyDisposition == CSPDirectiveList::NoncePolicyDisposition::Allowed)
-                isExplicitlyAllowed = true;
-        }
-    }
-    return isExplicitlyAllowed;
-}
-
 template<bool (CSPDirectiveList::*allowed)(const CSPHashValue&, ContentSecurityPolicy::InlineType) const>
 bool isAllowedByAllWithHash(const CSPDirectiveListVector& policies, const CSPHashValue& hashValue, ContentSecurityPolicy::InlineType type)
 {
@@ -433,6 +412,18 @@ bool isAllowedByAllWithURL(const CSPDirectiveListVector& policies, const KURL& u
     return isAllowed;
 }
 
+template <bool (CSPDirectiveList::*allowFromURLWithNonce)(const KURL&, const String& nonce, RedirectStatus, ContentSecurityPolicy::ReportingStatus) const>
+bool isAllowedByAllWithURLWithNonce(const CSPDirectiveListVector& policies, const KURL& url, const String& nonce, RedirectStatus redirectStatus, ContentSecurityPolicy::ReportingStatus reportingStatus)
+{
+    if (SchemeRegistry::schemeShouldBypassContentSecurityPolicy(url.protocol()))
+        return true;
+
+    bool isAllowed = true;
+    for (const auto& policy : policies)
+        isAllowed &= (policy.get()->*allowFromURLWithNonce)(url, nonce, redirectStatus, reportingStatus);
+    return isAllowed;
+}
+
 template<bool (CSPDirectiveList::*allowed)(LocalFrame*, const KURL&, ContentSecurityPolicy::ReportingStatus) const>
 bool isAllowedByAllWithFrame(const CSPDirectiveListVector& policies, LocalFrame* frame, const KURL& url, ContentSecurityPolicy::ReportingStatus reportingStatus)
 {
@@ -490,16 +481,16 @@ bool ContentSecurityPolicy::allowInlineEventHandler(const String& source, const
     return isAllowedByAllWithContext<&CSPDirectiveList::allowInlineEventHandlers>(m_policies, contextURL, contextLine, reportingStatus);
 }
 
-bool ContentSecurityPolicy::allowInlineScript(const String& contextURL, const WTF::OrdinalNumber& contextLine, const String& scriptContent, ContentSecurityPolicy::ReportingStatus reportingStatus) const
+bool ContentSecurityPolicy::allowInlineScript(const String& contextURL, const String& nonce, const WTF::OrdinalNumber& contextLine, const String& scriptContent, ContentSecurityPolicy::ReportingStatus reportingStatus) const
 {
-    return isAllowedByAllWithContextAndContent<&CSPDirectiveList::allowInlineScript>(m_policies, contextURL, contextLine, reportingStatus, scriptContent);
+    return isAllowedByAllWithContextAndContent<&CSPDirectiveList::allowInlineScript>(m_policies, contextURL, nonce, contextLine, reportingStatus, scriptContent);
 }
 
-bool ContentSecurityPolicy::allowInlineStyle(const String& contextURL, const WTF::OrdinalNumber& contextLine, const String& styleContent, ContentSecurityPolicy::ReportingStatus reportingStatus) const
+bool ContentSecurityPolicy::allowInlineStyle(const String& contextURL, const String& nonce, const WTF::OrdinalNumber& contextLine, const String& styleContent, ContentSecurityPolicy::ReportingStatus reportingStatus) const
 {
     if (m_overrideInlineStyleAllowed)
         return true;
-    return isAllowedByAllWithContextAndContent<&CSPDirectiveList::allowInlineStyle>(m_policies, contextURL, contextLine, reportingStatus, styleContent);
+    return isAllowedByAllWithContextAndContent<&CSPDirectiveList::allowInlineStyle>(m_policies, contextURL, nonce, contextLine, reportingStatus, styleContent);
 }
 
 bool ContentSecurityPolicy::allowEval(ScriptState* scriptState, ContentSecurityPolicy::ReportingStatus reportingStatus, ContentSecurityPolicy::ExceptionStatus exceptionStatus) const
@@ -554,19 +545,9 @@ bool ContentSecurityPolicy::allowPluginTypeForDocument(const Document& document,
     return true;
 }
 
-bool ContentSecurityPolicy::allowScriptFromSource(const KURL& url, RedirectStatus redirectStatus, ContentSecurityPolicy::ReportingStatus reportingStatus) const
-{
-    return isAllowedByAllWithURL<&CSPDirectiveList::allowScriptFromSource>(m_policies, url, redirectStatus, reportingStatus);
-}
-
-bool ContentSecurityPolicy::allowScriptWithNonce(const String& nonce) const
+bool ContentSecurityPolicy::allowScriptFromSource(const KURL& url, const String& nonce, RedirectStatus redirectStatus, ContentSecurityPolicy::ReportingStatus reportingStatus) const
 {
-    return isAllowedByAllWithNonce<&CSPDirectiveList::allowScriptNonce>(m_policies, nonce);
-}
-
-bool ContentSecurityPolicy::allowStyleWithNonce(const String& nonce) const
-{
-    return isAllowedByAllWithNonce<&CSPDirectiveList::allowStyleNonce>(m_policies, nonce);
+    return isAllowedByAllWithURLWithNonce<&CSPDirectiveList::allowScriptFromSource>(m_policies, url, nonce, redirectStatus, reportingStatus);
 }
 
 bool ContentSecurityPolicy::allowScriptWithHash(const String& source, InlineType type) const
@@ -579,7 +560,7 @@ bool ContentSecurityPolicy::allowStyleWithHash(const String& source, InlineType
     return checkDigest<&CSPDirectiveList::allowStyleHash>(source, type, m_styleHashAlgorithmsUsed, m_policies);
 }
 
-bool ContentSecurityPolicy::allowRequest(WebURLRequest::RequestContext context, const KURL& url, RedirectStatus redirectStatus, ReportingStatus reportingStatus) const
+bool ContentSecurityPolicy::allowRequest(WebURLRequest::RequestContext context, const KURL& url, const String& nonce, RedirectStatus redirectStatus, ReportingStatus reportingStatus) const
 {
     switch (context) {
     case WebURLRequest::RequestContextAudio:
@@ -608,7 +589,7 @@ bool ContentSecurityPolicy::allowRequest(WebURLRequest::RequestContext context,
     case WebURLRequest::RequestContextImport:
     case WebURLRequest::RequestContextScript:
     case WebURLRequest::RequestContextXSLT:
-        return allowScriptFromSource(url, redirectStatus, reportingStatus);
+        return allowScriptFromSource(url, nonce, redirectStatus, reportingStatus);
     case WebURLRequest::RequestContextManifest:
         return allowManifestFromSource(url, redirectStatus, reportingStatus);
     case WebURLRequest::RequestContextServiceWorker:
@@ -616,7 +597,7 @@ bool ContentSecurityPolicy::allowRequest(WebURLRequest::RequestContext context,
     case WebURLRequest::RequestContextWorker:
         return allowWorkerContextFromSource(url, redirectStatus, reportingStatus);
     case WebURLRequest::RequestContextStyle:
-        return allowStyleFromSource(url, redirectStatus, reportingStatus);
+        return allowStyleFromSource(url, nonce, redirectStatus, reportingStatus);
     case WebURLRequest::RequestContextCSPReport:
     case WebURLRequest::RequestContextDownload:
     case WebURLRequest::RequestContextHyperlink:
@@ -660,11 +641,11 @@ bool ContentSecurityPolicy::allowImageFromSource(const KURL& url, RedirectStatus
     return isAllowedByAllWithURL<&CSPDirectiveList::allowImageFromSource>(m_policies, url, redirectStatus, reportingStatus);
 }
 
-bool ContentSecurityPolicy::allowStyleFromSource(const KURL& url, RedirectStatus redirectStatus, ContentSecurityPolicy::ReportingStatus reportingStatus) const
+bool ContentSecurityPolicy::allowStyleFromSource(const KURL& url, const String& nonce, RedirectStatus redirectStatus, ContentSecurityPolicy::ReportingStatus reportingStatus) const
 {
     if (SchemeRegistry::schemeShouldBypassContentSecurityPolicy(url.protocol(), SchemeRegistry::PolicyAreaStyle))
         return true;
-    return isAllowedByAllWithURL<&CSPDirectiveList::allowStyleFromSource>(m_policies, url, redirectStatus, reportingStatus);
+    return isAllowedByAllWithURLWithNonce<&CSPDirectiveList::allowStyleFromSource>(m_policies, url, nonce, redirectStatus, reportingStatus);
 }
 
 bool ContentSecurityPolicy::allowFontFromSource(const KURL& url, RedirectStatus redirectStatus, ContentSecurityPolicy::ReportingStatus reportingStatus) const
@@ -697,7 +678,7 @@ bool ContentSecurityPolicy::allowWorkerContextFromSource(const KURL& url, Redire
     // CSP 1.1 moves workers from 'script-src' to the new 'child-src'. Measure the impact of this backwards-incompatible change.
     if (Document* document = this->document()) {
         UseCounter::count(*document, UseCounter::WorkerSubjectToCSP);
-        if (isAllowedByAllWithURL<&CSPDirectiveList::allowChildContextFromSource>(m_policies, url, redirectStatus, SuppressReport) && !isAllowedByAllWithURL<&CSPDirectiveList::allowScriptFromSource>(m_policies, url, redirectStatus, SuppressReport))
+        if (isAllowedByAllWithURL<&CSPDirectiveList::allowChildContextFromSource>(m_policies, url, redirectStatus, SuppressReport) && !isAllowedByAllWithURLWithNonce<&CSPDirectiveList::allowScriptFromSource>(m_policies, url, AtomicString(), redirectStatus, SuppressReport))
             UseCounter::count(*document, UseCounter::WorkerAllowedByChildBlockedByScript);
     }
 
@@ -859,18 +840,9 @@ void ContentSecurityPolicy::reportViolation(const String& directiveText, const S
     if (!document)
         return;
 
-    LocalFrame* frame = document->frame();
-    if (!frame)
-        return;
-
     SecurityPolicyViolationEventInit violationData;
     gatherSecurityPolicyViolationEventData(violationData, document, directiveText, effectiveDirective, blockedURL, header, redirectStatus, violationType, contextLine);
 
-    frame->localDOMWindow()->enqueueDocumentEvent(SecurityPolicyViolationEvent::create(EventTypeNames::securitypolicyviolation, violationData));
-
-    if (reportEndpoints.isEmpty())
-        return;
-
     // TODO(mkwst): Obviously, we shouldn't hit this check, as extension-loaded
     // resources should be allowed regardless. We apparently do, however, so
     // we should at least stop spamming reporting endpoints. See
@@ -908,20 +880,23 @@ void ContentSecurityPolicy::reportViolation(const String& directiveText, const S
 
     if (!shouldSendViolationReport(stringifiedReport))
         return;
+    didSendViolationReport(stringifiedReport);

[estark, 2016/06/02 18:57:31]

Why did this move up here? Seems odd to call something called didX() immediately
*before* doing X, and especially because we might not end up actually sending
the report (if there is no frame() or no report endpoints).

[Mike West, 2016/06/04 06:30:56]

This moved because I wanted to write unit tests that verified that reports were
being sent, and ensuring that the HashSet of report data is populated seemed
like the lowest-friction approach to getting that done. I agree that it's not
terribly elegant, though. I'll look into doing this more cleanly.

 
     RefPtr<EncodedFormData> report = EncodedFormData::create(stringifiedReport.utf8());
 
-    for (const String& endpoint : reportEndpoints) {
-        // If we have a context frame we're dealing with 'frame-ancestors' and we don't have our
-        // own execution context. Use the frame's document to complete the endpoint URL, overriding
-        // its URL with the blocked document's URL.
-        ASSERT(!contextFrame || !m_executionContext);
-        ASSERT(!contextFrame || equalIgnoringCase(effectiveDirective, FrameAncestors));
-        KURL url = contextFrame ? frame->document()->completeURLWithOverride(endpoint, blockedURL) : completeURL(endpoint);
-        PingLoader::sendViolationReport(frame, url, report, PingLoader::ContentSecurityPolicyViolationReport);
+    if (LocalFrame* frame = document->frame()) {

[estark, 2016/06/02 18:57:31]

nit: early return would be more consistent with chromium style

if (!frame)
  return;
frame->...

[Mike West, 2016/06/04 06:30:56]

Done.

+        frame->localDOMWindow()->enqueueDocumentEvent(SecurityPolicyViolationEvent::create(EventTypeNames::securitypolicyviolation, violationData));
+
+        for (const String& endpoint : reportEndpoints) {
+            // If we have a context frame we're dealing with 'frame-ancestors' and we don't have our
+            // own execution context. Use the frame's document to complete the endpoint URL, overriding
+            // its URL with the blocked document's URL.
+            DCHECK(!contextFrame || !m_executionContext);
+            DCHECK(!contextFrame || equalIgnoringCase(effectiveDirective, FrameAncestors));
+            KURL url = contextFrame ? frame->document()->completeURLWithOverride(endpoint, blockedURL) : completeURL(endpoint);
+            PingLoader::sendViolationReport(frame, url, report, PingLoader::ContentSecurityPolicyViolationReport);
+        }
     }
-
-    didSendViolationReport(stringifiedReport);
 }
 
 void ContentSecurityPolicy::reportMixedContent(const KURL& mixedURL, RedirectStatus redirectStatus)