PlzNavigate: Enforce 'frame-src' CSP on the browser.

content/browser/frame_host/csp_context_impl.cc

Index: content/browser/frame_host/csp_context_impl.cc
diff --git a/content/browser/frame_host/csp_context_impl.cc b/content/browser/frame_host/csp_context_impl.cc
new file mode 100644
index 0000000000000000000000000000000000000000..358d43f8b0302bf8adfb100d10f5c0f38a1fa351
--- /dev/null
+++ b/content/browser/frame_host/csp_context_impl.cc
@@ -0,0 +1,47 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <algorithm>
+
+#include "content/browser/frame_host/csp_context_impl.h"
+#include "content/browser/frame_host/frame_tree_node.h"
+#include "url/url_util.h"
+
+namespace content {
+
+CSPContextImpl::CSPContextImpl(FrameTreeNode* frame_tree_node)
+    : frame_tree_node_(frame_tree_node) {
+  DCHECK(frame_tree_node_);
+}
+
+void CSPContextImpl::LogToConsole(const std::string& message) {
+  RenderFrameHostImpl* current_frame_host =
+      frame_tree_node_->render_manager()->current_frame_host();

[nasko, 2017/02/11 00:01:23]

nit: No need to indirect through render_manager() call, the current host is
exposed through the FrameTreeNode itself.

[arthursonzogni, 2017/02/13 16:33:20]

Done.

+
+  if (!current_frame_host)

[alexmos, 2017/02/10 22:59:53]

Is this null check needed?

[arthursonzogni, 2017/02/13 16:33:20]

It is probably not needed indeed for this case(i.e. frame-src)
Maybe in the future if we choose to handle frame-ancestor on the browser-side
and the response is 204-No-content?

Do you prefer to add a DCHECK for the moment to detect some edges cases we are
not aware of?

[alexmos, 2017/02/14 06:57:19]

I haven't ever seen any code null-checking current_frame_host(), out of numerous
call sites, so my assumption is that something else will break very quickly if
it happens to be null.  You could add the DCHECK, but if it's null, we'll just
crash on the next line and we will get some crash reports anyway, so I don't
think it's really needed.

I'm not exactly sure how the 204 works, but that should keep the RFH created
initially (in RFHM::Init), no?

See also this comment in ~RFHM:
  // We should always have a current RenderFrameHost except in some tests.
  SetRenderFrameHost(std::unique_ptr<RenderFrameHostImpl>());

(not sure what tests it's talking about, but I'd rather only include the
null-check if any tests actually need it.)

[arthursonzogni, 2017/02/15 09:26:09]

You are probably right. I will remove the "if (!current_frame_host)".
I initially put this because of this patch:
https://codereview.chromium.org/2606933002/
But it doesn't apply to this case I think.

+    return;
+
+  current_frame_host->AddMessageToConsole(CONSOLE_MESSAGE_LEVEL_ERROR, message);

[nasko, 2017/02/11 00:01:23]

Is the log level always ERROR?

[arthursonzogni, 2017/02/13 16:33:20]

Yes, see all calls to ContentSecurityPolicy::logToConsole(..).

InfoMessageLevel is used one time, but it is for the message:
"The Content-Security-Policy directive {{directive}} is implemented behind a
flag which is currently disabled". We don't parse CSP on the browser-side for
the moment, so I think the log level will be always ERROR.

+}
+
+void CSPContextImpl::ReportViolation(
+    const CSPViolationParams& violation_params) {
+  frame_tree_node_->current_frame_host()->ContentSecurityPolicyViolation(
+      violation_params);
+}
+
+bool CSPContextImpl::SchemeShouldBypass(const base::StringPiece& scheme) {

[alexmos, 2017/02/10 22:59:53]

Perhaps name this SchemeShouldBypassCSP, so it's clear what the scheme should
bypass?  (This is what the Blink version of this does.)

[arthursonzogni, 2017/02/13 16:33:20]

Done.

+  // Blink uses its SchemeRegistry to check if a scheme should be bypassed.
+  // It can't be used on the browser process. It is used for two things:
+  // 1) Bypassing the "chrome-extension" scheme when chrome is built with the
+  //    extensions support.
+  // 2) Bypassing arbitrary scheme for testing purpose only in blink and in V8.
+  // TODO(arthursonzogni): url::GetBypassingCSPScheme() is used instead of the
+  // blink::SchemeRegistry. It contains 1) but not 2).
+  const auto& bypassing_scheme = url::GetCSPBypassingSchemes();

[alexmos, 2017/02/10 22:59:53]

nit: s/bypassing_scheme/bypassing_schemes/

[arthursonzogni, 2017/02/13 16:33:20]

Done.

+  return std::find(bypassing_scheme.begin(), bypassing_scheme.end(), scheme) !=
+         bypassing_scheme.end();
+}
+
+}  // namespace content