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

content/browser/frame_host/mixed_content_navigation_throttle.h

+// Copyright 2016 The Chromium Authors. All rights reserved.

[nasko, 2017/01/12 18:32:37]

nit: We are now 2017 ;).

[carlosk, 2017/01/21 02:54:59]

Done for all added files.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CONTENT_BROWSER_FRAME_HOST_MIXED_CONTENT_NAVIGATION_THROTTLE_H_
+#define CONTENT_BROWSER_FRAME_HOST_MIXED_CONTENT_NAVIGATION_THROTTLE_H_
+
+#include <set>
+
+#include "base/gtest_prod_util.h"
+#include "base/macros.h"
+#include "content/common/content_export.h"
+#include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/navigation_throttle.h"
+#include "content/public/common/request_context_type.h"
+#include "third_party/WebKit/public/platform/WebMixedContentContextType.h"
+
+namespace content {
+
+class FrameTreeNode;
+struct WebPreferences;
+
+using ThrottleCheckResult = NavigationThrottle::ThrottleCheckResult;
+
+// Responsible for browser-process-side mixed-content security. It is only

[nasko, 2017/01/12 18:32:37]

nit: security checks.

[carlosk, 2017/01/21 02:54:59]

Done.

+// enabled if PlzNavigate is and checks only for frame-level resource loads (aka
+// navigation loads). Sub-resources fetches are checked in the renderer process
+// by MixedContentChecker. Changes to this class might need to be reflected on
+// its renderer counterpart.
+//
+// Current mixed content W3C draft that drives this implementation:
+// https://w3c.github.io/webappsec-mixed-content/
+class MixedContentNavigationThrottle : public NavigationThrottle {
+ public:
+  MixedContentNavigationThrottle(NavigationHandle* navigation_handle);
+  ~MixedContentNavigationThrottle() override;
+
+  ThrottleCheckResult WillStartRequest() override;
+

[nasko, 2017/01/12 18:32:37]

nit: No need for empty spaces, can cluster all three with a "//
NavigationThrottle " comment on top. This is a pattern used in a lot of files to
combine together all methods being overriden from a base class.

[carlosk, 2017/01/21 02:54:58]

Done.

+  ThrottleCheckResult WillRedirectRequest() override;
+
+  ThrottleCheckResult WillProcessResponse() override;
+
+ private:
+  FRIEND_TEST_ALL_PREFIXES(MixedContentNavigationThrottleTest, IsMixedContent);
+
+  // Copy of all mixed content related values from the blink::UseCounter enum.
+  enum UseCounterFeature {
+    MIXED_CONTENT_FORMS_SUBMITTED = 441,

[nasko, 2017/01/12 18:32:37]

Can we use static_assert to ensure we don't accidentally make a breaking change?

[carlosk, 2017/01/21 02:54:58]

See:
https://codereview.chromium.org/1905033002/diff/970001/content/browser/frame_...

+    MIXED_CONTENT_PRIVATE_HOSTNAME_IN_PUBLIC_HOSTNAME = 530,
+    MIXED_CONTENT_PRESENT = 609,
+    MIXED_CONTENT_BLOCKABLE = 610,
+    MIXED_CONTENT_AUDIO = 611,
+    MIXED_CONTENT_DOWNLOAD = 612,
+    MIXED_CONTENT_FAVICON = 613,
+    MIXED_CONTENT_IMAGE = 614,
+    MIXED_CONTENT_INTERNAL = 615,
+    MIXED_CONTENT_PLUGIN = 616,
+    MIXED_CONTENT_PREFETCH = 617,
+    MIXED_CONTENT_VIDEO = 618,
+    MIXED_CONTENT_IN_NON_HTTPS_FRAME_THAT_RESTRICTS_MIXED_CONTENT = 661,
+    MIXED_CONTENT_IN_SECURE_FRAME_THAT_DOES_NOT_RESTRICT_MIXED_CONTENT = 662,
+    MIXED_CONTENT_WEB_SOCKET = 663,
+    MIXED_CONTENT_FORM_PRESENT = 665,
+    MIXED_CONTENT_BLOCKABLE_ALLOWED = 896,
+    BLOCKABLE_MIXED_CONTENT_IN_SUBFRAME_BLOCKED = 966,
+  };

[Mike West, 2017/01/13 13:20:00]

I know we talked about this a while ago, but looking at the code, I think I'm
convinced that it does more harm than good to try to jam these metrics into the
existing use counter. Especially given that this only triggers in redirect
cases, I don't think the value of the information we're gathering here justifies
the complexity.

The only metric here I'm actively evaluating is
`MIXED_CONTENT_PRIVATE_HOSTNAME_IN_PUBLIC_HOSTNAME`, which I don't think this
patch actually affects (as we're gathering it after a response comes in).

I'd suggest removing `UseCounterFeature`, as well as `mixed_content_features_`
and its usage throughout.

I do want to keep the CSP reporting, as that's essential to helping developers
migrate from HTTP to HTTPS, but that doesn't rely on the usecounter code at all.

[carlosk, 2017/01/21 02:54:59]

Why do you think this triggers only on redirects? There is potential for feature
values to be recorded for any navigation that is checked by this class, even if
mixed content is not found. For instance see InWhichFrameIsContentMixed: it is
executed before the decision on the existence of mixed content and might already
have recorded feature usage to mixed_content_features_.

In light of this information, are you still in favor of removing?

And just to make the decision easier, this is the list of features that might
actually be reported:

MIXED_CONTENT_BLOCKABLE
MIXED_CONTENT_BLOCKABLE_ALLOWED
MIXED_CONTENT_IN_NON_HTTPS_FRAME_THAT_RESTRICTS_MIXED_CONTENT
MIXED_CONTENT_IN_SECURE_FRAME_THAT_DOES_NOT_RESTRICT_MIXED_CONTENT
MIXED_CONTENT_INTERNAL
MIXED_CONTENT_PREFETCH
MIXED_CONTENT_PRESENT

And if this should be finally kept then maybe I should trim down that enum to
only include these?
OK, this should be correctly done with the whole mixedContentFound(ByTheBrowser)
IPC and call chain.

[Mike West, 2017/01/25 11:37:30]

Because I think you should remove the early-exit for frame navigations that
we're talking about in `MixedContentChecker.cpp` in order to make sure that we
don't change the ordering of MIX and HSTS, which has the side-effect of ensuring
that we'd only hit this throttle for redirects (or cases in which the user
explicitly allowed mixed content, which is vanishingly small). :)
Yup!

[carlosk, 2017/02/08 02:59:02]

I trimmed down the constants list to only contain the used ones.

+
+  // Checks if a navigation should be blocked or not due to mixed content.
+  bool ShouldBlockNavigation(bool for_redirect);
+
+  // Returns the parent frame where mixed content exists for the provided data
+  // or nullptr if there is no mixed content.
+  FrameTreeNode* InWhichFrameIsContentMixed(FrameTreeNode* node,
+                                            const GURL& url);
+
+  // Updates the renderer about any Blink feature usage.
+  void MaybeSendBlinkFeatureUsageReport();
+
+  // Records basic mixed content "feature" usage when any kind of mixed is

[nasko, 2017/01/12 18:32:37]

nit: s/mixed/mixed content/

[carlosk, 2017/01/21 02:54:59]

Done.

+  // found.
+  void ReportBasicMixedContentFeatures(
+      RequestContextType request_context_type,
+      blink::WebMixedContentContextType mixed_content_context_type,
+      const WebPreferences& prefs);
+
+  static bool CONTENT_EXPORT IsMixedContentForTesting(const GURL& origin_url,

[nasko, 2017/01/12 18:32:37]

Why is this method CONTENT_EXPORT and private at the same time?

[carlosk, 2017/01/21 02:54:59]

Because its unit test friend-ed in line 45 needs the export to be able to call
this method.

+                                                      const GURL& url);
+
+  // Keeps track of mixed content features (as defined in blink::UseCounter)
+  // encountered while running one of the navigation throttling steps. These
+  // values are reported to the respective renderer process after each mixed
+  // content check is finished.
+  std::set<int> mixed_content_features_;
+
+  DISALLOW_COPY_AND_ASSIGN(MixedContentNavigationThrottle);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_FRAME_HOST_MIXED_CONTENT_NAVIGATION_THROTTLE_H_