Replicate Content-Security-Policy into remote frame proxies.

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 950512fc3bd9385c4a1d926f40470d5a51b68772..a9c76664873b4674660b761ec155825b675088b8 100644
--- a/third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp
+++ b/third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp
@@ -31,6 +31,7 @@
 #include "core/dom/Document.h"
 #include "core/dom/SandboxFlags.h"
 #include "core/events/SecurityPolicyViolationEvent.h"
+#include "core/frame/FrameClient.h"
 #include "core/frame/LocalDOMWindow.h"
 #include "core/frame/LocalFrame.h"
 #include "core/frame/UseCounter.h"
@@ -42,6 +43,7 @@
 #include "core/inspector/ConsoleMessage.h"
 #include "core/inspector/InspectorInstrumentation.h"
 #include "core/loader/DocumentLoader.h"
+#include "core/loader/FrameLoaderClient.h"
 #include "core/loader/PingLoader.h"
 #include "platform/JSONValues.h"
 #include "platform/ParsingUtilities.h"
@@ -157,13 +159,20 @@ void ContentSecurityPolicy::bindToExecutionContext(ExecutionContext* executionCo
     applyPolicySideEffectsToExecutionContext();
 }
 
+void ContentSecurityPolicy::bindToSecurityOrigin(const SecurityOrigin& securityOrigin)

[Mike West, 2016/05/11 06:21:04]

Nit: Now about naming this something like `setupSelf`? "Bind to security origin"
sounds like something different than what this method is actually doing.

[Łukasz Anforowicz, 2016/05/11 23:14:48]

Good point - this name wasn't quite right.  Done.

+{
+    // Ensure that 'self' processes correctly.
+    m_selfProtocol = securityOrigin.protocol();
+    m_selfSource = new CSPSource(this, m_selfProtocol, securityOrigin.host(), securityOrigin.port(), String(), CSPSource::NoWildcard, CSPSource::NoWildcard);
+}
+
 void ContentSecurityPolicy::applyPolicySideEffectsToExecutionContext()
 {
     ASSERT(m_executionContext);
-    ASSERT(getSecurityOrigin());
-    // Ensure that 'self' processes correctly.
-    m_selfProtocol = getSecurityOrigin()->protocol();
-    m_selfSource = new CSPSource(this, m_selfProtocol, getSecurityOrigin()->host(), getSecurityOrigin()->port(), String(), CSPSource::NoWildcard, CSPSource::NoWildcard);
+
+    SecurityOrigin* securityOrigin = m_executionContext->securityContext().getSecurityOrigin();
+    ASSERT(securityOrigin);
+    bindToSecurityOrigin(*securityOrigin);
 
     if (didSetReferrerPolicy())
         m_executionContext->setReferrerPolicy(m_referrerPolicy);
@@ -182,8 +191,8 @@ void ContentSecurityPolicy::applyPolicySideEffectsToExecutionContext()
         if (m_insecureRequestsPolicy == SecurityContext::InsecureRequestsUpgrade) {
             UseCounter::count(document, UseCounter::UpgradeInsecureRequestsEnabled);
             document->setInsecureRequestsPolicy(m_insecureRequestsPolicy);
-            if (!getSecurityOrigin()->host().isNull())
-                document->addInsecureNavigationUpgrade(getSecurityOrigin()->host().impl()->hash());
+            if (!securityOrigin->host().isNull())
+                document->addInsecureNavigationUpgrade(securityOrigin->host().impl()->hash());
         }
 
         for (const auto& consoleMessage : m_consoleMessages)
@@ -255,7 +264,15 @@ void ContentSecurityPolicy::didReceiveHeader(const String& header, ContentSecuri
         applyPolicySideEffectsToExecutionContext();
 }
 
-void ContentSecurityPolicy::addPolicyFromHeaderValue(const String& header, ContentSecurityPolicyHeaderType type, ContentSecurityPolicyHeaderSource source)
+void ContentSecurityPolicy::replicateHeader(const String& header, ContentSecurityPolicyHeaderType type, ContentSecurityPolicyHeaderSource source)
+{
+    // Replication should only happen for remote frames.
+    ASSERT(!m_executionContext);

[Mike West, 2016/05/11 06:21:04]

We create CSP without an execution context for local frames as well (in
`DocumentLoader::responseReceived`). Eventually we'll be able to get rid of that
mechanism by moving everything to the browser, but until then it's not clear to
me what the assertion needs to be.

[Łukasz Anforowicz, 2016/05/11 23:14:48]

I've just removed the assert.

+
+    addPolicyFromHeaderValue(header, type, source, false);
+}
+
+void ContentSecurityPolicy::addPolicyFromHeaderValue(const String& header, ContentSecurityPolicyHeaderType type, ContentSecurityPolicyHeaderSource source, bool notifyFrameLoaderClient)
 {
     // If this is a report-only header inside a <meta> element, bail out.
     if (source == ContentSecurityPolicyHeaderSourceMeta && type == ContentSecurityPolicyHeaderTypeReport) {
@@ -263,6 +280,15 @@ void ContentSecurityPolicy::addPolicyFromHeaderValue(const String& header, Conte
         return;
     }
 
+    // Notify about the new header, so that it can be reported back to the
+    // browser process.  This is needed in order to:
+    // 1) now / short-term: replicate CSP directives (i.e. frame-src) to OOPIFs.
+    // 2) not-yet / long-term: CSP can be enforced by the browser.

[alexmos, 2016/05/11 19:46:41]

Might be good to add a link to the bug for moving CSP to the browser (I think
it's 376522?).  For 2), do you know how this would reconcile with parsing CSP
HTTP headers in the browser?

[Łukasz Anforowicz, 2016/05/11 23:14:48]

Done.

I am having second thoughts about this comment... maybe I should just remove
stuff from "This is needed in order to..." forward?
This is an interesting question.  nasko@ or mkwst@ might probably have a better
idea on how to accomplish this, but I thought about:
1. moving parsing, storing, checking code into //content/common layer.
2. rewriting blink::ContentSecurityPolicy to use some kind of a delegate to do
parsing, storing, checking via //content layer (yucky?)
3. reuse //content/common code to do parsing, storing, checking in the browser
as well.

+    if (notifyFrameLoaderClient && document() && document()->frame()) {
+        static_cast<FrameLoaderClient*>(document()->frame()->client())
+            ->didAddContentSecurityPolicy(header, type, source);

[alexmos, 2016/05/11 19:46:41]

Does this also work with pending navigations that are canceled?  E.g., would it
be at all possible to have a pending navigation that sends a CSP http header
before commit but then never commits due to an error?  Can 
DocumentLoader::responseReceived lead to a DidFailProvisionalLoad after applying
the CSP header?  I'm basically curious whether we need to delay replicating CSP
headers until a successful commit.

[Łukasz Anforowicz, 2016/05/13 17:29:15]

This is a great question - thanks for raising this.

In patchset 7, we are resetting the old CSP headers (and processing new HTTP
headers in NavigationHandle::WillProcessResponse) right when we know it is okay
to say ReadyToCommitNavigation().  Before that point we should have processed
any CSP from the not-yet-commited page (because HTTP headers are dropped on the
floor on Blink side + because Blink couldn't yet process CSP from <meta>
elements because we haven't yet sent to Blink the body of the HTTP response). 
After that point we are commited to the navigation, so it is okay to forget the
old CSP headers.

OTOH, there are still some questions left:

1. Can a non-http navigation clobber old headers too early - i.e. via Blink
notifications that happen too early / before commit.  In particular about:blank
navigation is a bit special because Blink copies CSP headers from the parent in
this case [1].

2. In patchset 7 I've removed the call to
frame_tree_node->ResetContentSecurityPolicy() that in earlier patchsets was done
from NavigatorImpl::DidNavigate.  This means that this call is now only
happening for HTTP responses, because otherwise
NavigationHandle::WillProcessResponse won't get involved (right?).  So - there
are 2 subquestions here:

2a. Should navigating from http to non-http location reset CSP for a frame?

2b. What would be a good place to call
frame_tree_node->ResetContentSecurityPolicy() in this case (place that would
happen earlier than NavigationHandle::WillProcessResponse because otherwise we
would clobber CSP the browser found in the headers).

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Łukasz Anforowicz, 2016/05/13 23:31:43]

Nick / Charlie - could you please also look at Alex's question?

+    }
+
     Vector<UChar> characters;
     header.appendTo(characters);
 
@@ -710,11 +736,6 @@ bool ContentSecurityPolicy::didSetReferrerPolicy() const
     return false;
 }
 
-SecurityOrigin* ContentSecurityPolicy::getSecurityOrigin() const
-{
-    return m_executionContext->securityContext().getSecurityOrigin();
-}
-
 const KURL ContentSecurityPolicy::url() const
 {
     return m_executionContext->contextURL();
@@ -796,6 +817,15 @@ static void gatherSecurityPolicyViolationEventData(SecurityPolicyViolationEventI
 void ContentSecurityPolicy::reportViolation(const String& directiveText, const String& effectiveDirective, const String& consoleMessage, const KURL& blockedURL, const Vector<String>& reportEndpoints, const String& header, ViolationType violationType, LocalFrame* contextFrame)
 {
     ASSERT(violationType == URLViolation || blockedURL.isEmpty());
+
+    // FIXME: Support sending reports from OOPIFs (or move CSP child-src and
+    // frame-src checks to the browser).

[Mike West, 2016/05/11 06:21:04]

Nit: Prefer `TODO(...)` to FIXME.

Nit: Can you point to a bug here so that we don't lose track of the work?

[Łukasz Anforowicz, 2016/05/11 23:14:48]

Done.

+    if (!m_executionContext && !contextFrame) {
+        ASSERT(equalIgnoringCase(effectiveDirective, ContentSecurityPolicy::ChildSrc)
+            || equalIgnoringCase(effectiveDirective, ContentSecurityPolicy::FrameSrc));
+        return;
+    }
+
     ASSERT((m_executionContext && !contextFrame) || (equalIgnoringCase(effectiveDirective, ContentSecurityPolicy::FrameAncestors) && contextFrame));
 
     // FIXME: Support sending reports from worker.