PlzNavigate: Move navigation-level mixed content checks to the browser.

content/browser/frame_host/mixed_content_navigation_throttle.cc

Index: content/browser/frame_host/mixed_content_navigation_throttle.cc
diff --git a/content/browser/frame_host/mixed_content_navigation_throttle.cc b/content/browser/frame_host/mixed_content_navigation_throttle.cc
new file mode 100644
index 0000000000000000000000000000000000000000..38b75b1442a3f2356ada4555e4248e99f2cab0da
--- /dev/null
+++ b/content/browser/frame_host/mixed_content_navigation_throttle.cc
@@ -0,0 +1,306 @@
+// Copyright 2016 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 "content/browser/frame_host/mixed_content_navigation_throttle.h"
+
+#include "base/strings/stringprintf.h"
+#include "content/browser/frame_host/frame_tree.h"
+#include "content/browser/frame_host/frame_tree_node.h"
+#include "content/browser/frame_host/navigation_handle_impl.h"
+#include "content/browser/frame_host/render_frame_host_delegate.h"
+#include "content/browser/renderer_host/render_view_host_impl.h"
+#include "content/public/browser/content_browser_client.h"
+#include "content/public/common/content_client.h"
+#include "content/public/common/origin_util.h"
+#include "content/public/common/request_context_type.h"
+#include "content/public/common/url_constants.h"
+#include "content/public/common/web_preferences.h"
+#include "net/base/url_util.h"
+#include "third_party/WebKit/public/platform/WebMixedContent.h"
+#include "url/gurl.h"
+#include "url/origin.h"
+#include "url/url_constants.h"
+
+namespace {
+
+using namespace content;
+
+// Should return the same value as SchemeRegistry::shouldTreatURLSchemeAsSecure.
+bool HasPotentiallySecureScheme(const GURL& url) {
+  // TODO(carlosk): find out how to properly handle these secure schemes. Should
+  // statically defined ones in Chrome/embedder come from a shared file? Should
+  // dynamically defined ones from extensions register both with browser and
+  // renderer code? See http://crbug.com/627502.

[Mike West, 2016/07/19 14:39:31]

Super very important nit: http_s_.

[carlosk, 2016/07/19 16:32:16]

Done. But why? Fear of MITM attacks?

+  bool result =
+      // Note: schemes statically defined in secureSchemes()
+      url.SchemeIs(url::kHttpsScheme) || url.SchemeIs(url::kAboutScheme) ||
+      url.SchemeIs(url::kDataScheme) || url.SchemeIs(url::kWssScheme) ||
+      // Note: schemes "dynamically" registered with
+      // SchemeRegistry::registerURLSchemeAsSecure.
+      url.SchemeIs(kChromeUIScheme);

[Mike West, 2016/07/19 14:39:32]

As you note, this isn't pretty. It also doesn't seem to include
`chrome-extension` or `chrome-extension-resource`, which are registered in
`//extensions/renderer/dispatcher.cc` (which, due to the excitement of layering,
you don't even have access to here). I think we're going to need to be a bit
more clever about how we define this list in an extensible way. Perhaps setting
up a registry in //content directly that passes through to //third_party/WebKit
is the right short-term thing to do, but I don't think you're going to be able
to hard-code an exhaustive list without layering violations.

[carlosk, 2016/07/19 16:32:16]

Acknowledged. Then we indeed need to solve http://crbug.com/627502 before
landing this.

+  return result;
+}
+
+// Should return the same value as SecurityOrigin::isLocal and
+// SchemeRegistry::shouldTreatURLSchemeAsLocal.
+bool HasLocalScheme(const GURL& url) {
+  // TODO(carlosk): Same registration problem as described in
+  // HasPotentiallySecureScheme applies here. See http://crbug.com/627502.

[Mike West, 2016/07/19 14:39:31]

S.

[carlosk, 2016/07/19 16:32:15]

Done.

+  bool result =
+      // Note: schemes statically defined in schemesWithUniqueOrigins()
+      url.SchemeIs(url::kAboutScheme) || url.SchemeIs(url::kJavaScriptScheme) ||
+      url.SchemeIs(url::kDataScheme) ||
+      // Note: schemes "dynamically" registered with
+      // SchemeRegistry::registerURLSchemeAsNoAccess. There's no content/
+      // re-definition of kChromeNativeScheme.
+      url.SchemeIs("chrome-native");

[Mike West, 2016/07/19 14:39:32]

Ditto. I'm not an OWNER of this code, but reaching up into the //chrome layer
for a string seems pretty ugly.

[carlosk, 2016/07/19 16:32:16]

Agreed.

+  return result;
+}
+
+// Should return the same value as the resource URL checks made inside
+// MixedContentChecker::isMixedContent.
+bool IsUrlPotentiallySecure(const GURL& url) {
+  // TODO(carlosk): secure origin checks don't match between content and Blink
+  // hence this implementation here instead of a direct call to IsOriginSecure
+  // (in origin_util.cc). See http://crbug.com/629059.

[Mike West, 2016/07/19 14:39:32]

S.

[carlosk, 2016/07/19 16:32:16]

Done.

+  bool is_secure = false;
+
+  // Mimics SecurityOrigin::isSecure.
+  if (HasPotentiallySecureScheme(url) ||
+      (url.SchemeIsFileSystem() &&
+       HasPotentiallySecureScheme(*url.inner_url())) ||
+      (url.SchemeIsBlob() &&
+       HasPotentiallySecureScheme(GURL(url.GetContent()))) ||
+      IsOriginWhiteListedTrustworthy(url)) {
+    is_secure |= true;
+  }
+
+  // Mimics SecurityOrigin::isPotentiallyTrustworthy without duplicating (much)
+  // of the checks already done previously.
+  if (HasLocalScheme(url) || net::IsLocalhost(url.HostNoBrackets()))
+    is_secure |= true;
+
+  // TODO(mkwst): Remove this once 'localhost' is no longer considered
+  // potentially trustworthy.
+  if (is_secure && url.SchemeIs(url::kHttpScheme) &&
+      net::IsLocalHostname(url.HostNoBrackets(), nullptr)) {
+    is_secure = false;
+  }

[Mike West, 2016/07/19 14:39:32]

Should be gone by the time you land this (if I can get the relevant tests
working).

[carlosk, 2016/07/19 16:32:16]

Acknowledged.

+
+  return is_secure;
+}
+
+// This method should return the same results as
+// SchemeRegistry::shouldTreatURLSchemeAsRestrictingMixedContent.
+bool DoesOriginSchemeRestricsMixedContent(const url::Origin& origin) {
+  return origin.scheme() == url::kHttpsScheme;
+}
+
+bool ShouldTreatURLSchemeAsCORSEnabled(const GURL& url) {
+  // TODO(carlosk): for CORS schemes we have the exact same issue as for the
+  // secure schemes above. See callers to and references in
+  // WebSecurityPolicy::registerURLSchemeAsCORSEnabled. See
+  // http://crbug.com/627502.

[Mike West, 2016/07/19 14:39:31]

S.

[carlosk, 2016/07/19 16:32:15]

Done.

+  return
+      // Note: CORS schemes statically defined in CORSEnabledSchemes()
+      url.SchemeIsHTTPOrHTTPS() || url.SchemeIs(url::kDataScheme) ||
+      // Note: CORS schemes "dynamically" registered in
+      // RenderThreadImpl::RegisterSchemes.
+      url.SchemeIs(kChromeUIScheme);
+}
+
+const char* TypeNameFromContext(RequestContextType context) {
+  // Note: the static_cast below is guaranteed by the static asserts in
+  // content/child/web_url_request_util.cc.
+  return blink::WebMixedContent::requestContextName(
+      static_cast<blink::WebURLRequest::RequestContext>(context));
+}
+
+blink::WebMixedContent::ContextType MixedContextTypeFromContext(
+    RequestContextType context,
+    bool block_mixed_plugin_content) {
+  // Note: the static_cast below is guaranteed by the static asserts in
+  // content/child/web_url_request_util.cc.
+  return blink::WebMixedContent::contextTypeFromRequestContext(
+      static_cast<blink::WebURLRequest::RequestContext>(context),
+      block_mixed_plugin_content);
+}
+
+void LogToConsoleAboutFetch(FrameTreeNode* mixed_content_node,
+                            NavigationHandleImpl* handle_impl,
+                            const GURL& url,
+                            bool allowed) {
+  FrameTreeNode* tree_node = handle_impl->frame_tree_node();
+  // TODO(carlosk): we should only use the very tree_node here for the form
+  // submission case. (D)CHECK for that when we get there. For now just check
+  // this is not the root node.
+  DCHECK(tree_node->parent());
+  RequestContextType request_context_type =
+      handle_impl->fetch_request_context_type();
+
+  std::string message = base::StringPrintf(
+      "Mixed Content: The page at '%s' was loaded over HTTPS, but requested an "
+      "insecure %s '%s'. %s",
+      mixed_content_node->current_url().spec().c_str(),
+      TypeNameFromContext(request_context_type), url.spec().c_str(),
+      allowed ? "This content should also be served over HTTPS."
+              : "This request has been blocked; the content must be served "
+                "over HTTPS.");
+  ConsoleMessageLevel messageLevel =
+      allowed ? ConsoleMessageLevel::CONSOLE_MESSAGE_LEVEL_WARNING
+              : ConsoleMessageLevel::CONSOLE_MESSAGE_LEVEL_ERROR;
+  tree_node->current_frame_host()->AddMessageToConsole(messageLevel, message);

[Mike West, 2016/07/19 14:39:32]

s/tree_node/mixed_content_node/ (or `tree_node->parent()`?), right? Because we
might never commit this navigation in the current node.

[carlosk, 2016/07/19 16:32:16]

That makes sense but MixedContentChecker disagrees:

183: bool MixedContentChecker::shouldBlockFetch(LocalFrame* frame, ...
263:         logToConsoleAboutFetch(frame, ...

125: void MixedContentChecker::logToConsoleAboutFetch(LocalFrame* frame, ...
275:     frame->document()->addConsoleMessage(...

WDYT?

+}
+
+}  // namespace
+
+namespace content {
+
+MixedContentNavigationThrottle::MixedContentNavigationThrottle(
+    NavigationHandle* navigation_handle)
+    : NavigationThrottle(navigation_handle) {}
+
+MixedContentNavigationThrottle::~MixedContentNavigationThrottle() {}
+
+ThrottleCheckResult MixedContentNavigationThrottle::WillStartRequest() {
+  NavigationHandleImpl* handle_impl =
+      static_cast<NavigationHandleImpl*>(navigation_handle());
+  FrameTreeNode* node = handle_impl->frame_tree_node();
+
+  // Find the parent node with mixed content, if any.
+  FrameTreeNode* mixed_content_node =
+      InWhichFrameIsContentMixed(node, navigation_handle()->GetURL());
+  if (!mixed_content_node)
+    return ThrottleCheckResult::PROCEED;
+
+  // From this point on we know this is not a main frame navigation and that
+  // there is mixed-content. Now let's decide if it's OK to proceed with it.
+
+  const WebPreferences& prefs =
+      node->current_frame_host()->render_view_host()->GetWebkitPreferences();

[Mike West, 2016/07/19 14:39:32]

The mixed content prefs aren't renderer-specific, but CLI-based, aren't they?
Perhaps you could pass them in when you create the throttle, rather than reading
them for each request?

[carlosk, 2016/07/19 16:32:16]

Some come from CLI, some from the renderer/Blink and currently a couple are
controlled field trials.

They seem to be renderer specific but the fact that they are stored at the
RenderViewHost level makes it so that they can be different for different
RenderViews, even if they live in the same renderer.
I could store them but note that a new instance of this class is created for
each request anyway.

But maybe one thing I can do -- in a small separate patch -- is to add a cont
version of this getter that returns a reference to it instead of a copy. I did
find a few other calls that would benefit from this too. WDYT?

+
+  // If we're in strict mode, we'll automagically fail everything, and
+  // intentionally skip the client/embedder checks in order to prevent degrading
+  // the site's security UI.
+  bool block_all_mixed_content = !!(
+      mixed_content_node->current_replication_state().insecure_request_policy &
+      blink::kBlockAllMixedContent);
+  bool strictMode =
+      prefs.strict_mixed_content_checking || block_all_mixed_content;
+
+  blink::WebMixedContent::ContextType mixed_context_type =
+      MixedContextTypeFromContext(handle_impl->fetch_request_context_type(),
+                                  prefs.block_mixed_plugin_content);
+
+  if (!ShouldTreatURLSchemeAsCORSEnabled(navigation_handle()->GetURL())) {
+    mixed_context_type =
+        blink::WebMixedContent::ContextType::OptionallyBlockable;
+  }
+
+  bool allowed = false;
+  ContentBrowserClient* browser_client = GetContentClient()->browser();
+  RenderFrameHostDelegate* frame_host_delegate =
+      node->current_frame_host()->delegate();
+  switch (mixed_context_type) {
+    case blink::WebMixedContent::ContextType::OptionallyBlockable:
+      allowed =
+          !strictMode &&
+          browser_client->ShouldAllowDisplayingInsecureContent(
+              prefs.allow_displaying_insecure_content,
+              navigation_handle()->GetURL(), handle_impl->GetWebContents());
+      if (allowed)
+        frame_host_delegate->DidDisplayInsecureContent();
+      break;
+
+    case blink::WebMixedContent::ContextType::Blockable: {
+      bool shouldAskEmbedder =
+          !strictMode && (!prefs.strictly_block_blockable_mixed_content ||
+                          prefs.allow_running_insecure_content);
+      allowed =
+          shouldAskEmbedder &&
+          browser_client->ShouldAllowRunningInsecureContent(
+              prefs.allow_running_insecure_content,
+              mixed_content_node->current_origin(),
+              navigation_handle()->GetURL(), handle_impl->GetWebContents());
+      if (allowed) {
+        const GURL& origin_url = mixed_content_node->current_url().GetOrigin();
+        frame_host_delegate->DidRunInsecureContent(
+            origin_url, navigation_handle()->GetURL());
+        browser_client->RecordURLMetric(
+            "ContentSettings.MixedScript.RanMixedScript", origin_url);

[Mike West, 2016/07/19 14:39:31]

I wonder if anyone is looking at these metrics...

[carlosk, 2016/07/19 16:32:16]

Don't we all?

+      }
+      break;
+    }
+
+    case blink::WebMixedContent::ContextType::ShouldBeBlockable:
+      allowed = !strictMode;
+      if (allowed)
+        frame_host_delegate->DidDisplayInsecureContent();
+      break;
+
+    case blink::WebMixedContent::ContextType::NotMixedContent:
+      NOTREACHED();
+      break;
+  };
+
+  LogToConsoleAboutFetch(mixed_content_node, handle_impl,
+                         navigation_handle()->GetURL(), allowed);
+
+  return allowed ? ThrottleCheckResult::PROCEED : ThrottleCheckResult::CANCEL;
+}
+
+ThrottleCheckResult MixedContentNavigationThrottle::WillRedirectRequest() {
+  // Upon redirects the same checks are to be executed as for requests.
+  return MixedContentNavigationThrottle::WillStartRequest();
+}
+
+ThrottleCheckResult MixedContentNavigationThrottle::WillProcessResponse() {
+  // TODO(carlosk): at this point we must check the final security level of
+  // the connection! Does it use an outdated protocol? See
+  // MixedContentChecker::handleCertificateError
+  return ThrottleCheckResult::PROCEED;
+}
+
+FrameTreeNode* MixedContentNavigationThrottle::InWhichFrameIsContentMixed(
+    FrameTreeNode* node,
+    const GURL& url) {
+  // Main frame navigations cannot be mixed content.
+  // TODO(carlosk): except for form submissions which will be dealt with later.
+  if (node->IsMainFrame())
+    return nullptr;
+
+  // If the navigated URL is potentially secure there's no mixed-content (at
+  // request time at least).
+  if (IsUrlPotentiallySecure(url))
+    return nullptr;
+
+  // TODO(carlosk): don't we need to check more than just the immediate parent
+  // and the root? Is it always the case that these two are the only sources for
+  // obtaining the "origin of the security context"? http://crbug.com/623486

[Mike West, 2016/07/19 14:39:32]

S.

[carlosk, 2016/07/19 16:32:15]

Done.

+
+  // If neither the parent nor root frames' origins are secure, there is no
+  // mixed content.
+  FrameTreeNode* mixed_content_node = nullptr;
+
+  // Checks if the root or immediate parent frame's origin are secure.
+  FrameTreeNode* root = node->frame_tree()->root();
+  FrameTreeNode* parent = node->parent();
+  if (DoesOriginSchemeRestricsMixedContent(root->current_origin()))
+    mixed_content_node = root;
+  else if (DoesOriginSchemeRestricsMixedContent(parent->current_origin()))
+    mixed_content_node = parent;
+
+  return mixed_content_node;
+}
+
+// static
+bool MixedContentNavigationThrottle::IsMixedContentForTesting(
+    const GURL& origin_url,
+    const GURL& url) {
+  const url::Origin origin(origin_url);
+  return !IsUrlPotentiallySecure(url) &&
+         DoesOriginSchemeRestricsMixedContent(origin);
+}
+
+}  // namespace content