Process, but do not enforce X-Frame-Options on redirects.

In order to make a reasonable decision about the reasonable-sounding
feature request in https://crbug.com/835465, this patch starts processing
XFO headers on redirect responses in order to collect metrics about how
many requests we'd impact by tightening our enforcement.

Bug: 835465
Change-Id: Ieb4571aae10e31fb61f1ccc245da5eb5dab791ae
Reviewed-on: https://chromium-review.googlesource.com/1023393
Commit-Queue: Mike West <mkwst@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553520}

content/browser/frame_host/ancestor_throttle.cc

@@ -58,7 +58,14 @@ enum XFrameOptionsHistogram {
   // The 'frame-ancestors' CSP directive should take effect instead.
   BYPASS = 8,
 
-  XFRAMEOPTIONS_HISTOGRAM_MAX = BYPASS
+  // Navigation would have been blocked if we applied 'X-Frame-Options' to
+  // redirects.
+  //
+  // TODO(mkwst): Rename this when we make a decision around
+  // https://crbug.com/835465.
+  REDIRECT_WOULD_BE_BLOCKED = 9,
+
+  XFRAMEOPTIONS_HISTOGRAM_MAX = REDIRECT_WOULD_BE_BLOCKED
 };
 
 void RecordXFrameOptionsUsage(XFrameOptionsHistogram usage) {
@@ -98,8 +105,32 @@ std::unique_ptr<NavigationThrottle> AncestorThrottle::MaybeCreateThrottleFor(
 
 AncestorThrottle::~AncestorThrottle() {}
 
+NavigationThrottle::ThrottleCheckResult
+AncestorThrottle::WillRedirectRequest() {
+  // During a redirect, we don't know which RenderFrameHost we'll end up in,
+  // so we can't log reliably to the console. We should be able to work around
+  // this iff we decide to ship the redirect-blocking behavior, but for now
+  // we'll just skip the console-logging bits to collect metrics.
+  NavigationThrottle::ThrottleCheckResult result =
+      ProcessResponseImpl(LoggingDisposition::DO_NOT_LOG_TO_CONSOLE);
+
+  if (result.action() == NavigationThrottle::BLOCK_RESPONSE)
+    RecordXFrameOptionsUsage(REDIRECT_WOULD_BE_BLOCKED);
+
+  // TODO(mkwst): We need to decide whether we'll be able to get away with
+  // tightening the XFO check to include redirect responses once we have a
+  // feel for the REDIRECT_WOULD_BE_BLOCKED numbers we're collecting above.
+  // Until then, we'll allow the response to proceed: https://crbug.com/835465.
+  return NavigationThrottle::PROCEED;
+}
+
 NavigationThrottle::ThrottleCheckResult
 AncestorThrottle::WillProcessResponse() {
+  return ProcessResponseImpl(LoggingDisposition::LOG_TO_CONSOLE);
+}
+
+NavigationThrottle::ThrottleCheckResult AncestorThrottle::ProcessResponseImpl(
+    LoggingDisposition logging) {
   DCHECK(!navigation_handle()->IsInMainFrame());
 
   NavigationHandleImpl* handle =
@@ -116,18 +147,21 @@ AncestorThrottle::WillProcessResponse() {
 
   switch (disposition) {
     case HeaderDisposition::CONFLICT:
-      ParseError(header_value, disposition);
+      if (logging == LoggingDisposition::LOG_TO_CONSOLE)
+        ParseError(header_value, disposition);
       RecordXFrameOptionsUsage(CONFLICT);
       return NavigationThrottle::BLOCK_RESPONSE;
 
     case HeaderDisposition::INVALID:
-      ParseError(header_value, disposition);
+      if (logging == LoggingDisposition::LOG_TO_CONSOLE)
+        ParseError(header_value, disposition);
       RecordXFrameOptionsUsage(INVALID);
       // TODO(mkwst): Consider failing here.
       return NavigationThrottle::PROCEED;
 
     case HeaderDisposition::DENY:
-      ConsoleError(disposition);
+      if (logging == LoggingDisposition::LOG_TO_CONSOLE)
+        ConsoleError(disposition);
       RecordXFrameOptionsUsage(DENY);
       return NavigationThrottle::BLOCK_RESPONSE;
 
@@ -139,7 +173,8 @@ AncestorThrottle::WillProcessResponse() {
       while (parent) {
         if (!parent->current_origin().IsSameOriginWith(current_origin)) {
           RecordXFrameOptionsUsage(SAMEORIGIN_BLOCKED);
-          ConsoleError(disposition);
+          if (logging == LoggingDisposition::LOG_TO_CONSOLE)
+            ConsoleError(disposition);
 
           // TODO(mkwst): Stop recording this metric once we convince other
           // vendors to follow our lead with XFO: SAMEORIGIN processing.

content/browser/frame_host/ancestor_throttle.h

@@ -38,16 +38,21 @@ class CONTENT_EXPORT AncestorThrottle : public NavigationThrottle {
 
   ~AncestorThrottle() override;
 
+  NavigationThrottle::ThrottleCheckResult WillRedirectRequest() override;
   NavigationThrottle::ThrottleCheckResult WillProcessResponse() override;
   const char* GetNameForLogging() override;
 
  private:
+  enum class LoggingDisposition { LOG_TO_CONSOLE, DO_NOT_LOG_TO_CONSOLE };
+
   FRIEND_TEST_ALL_PREFIXES(AncestorThrottleTest, ParsingXFrameOptions);
   FRIEND_TEST_ALL_PREFIXES(AncestorThrottleTest, ErrorsParsingXFrameOptions);
   FRIEND_TEST_ALL_PREFIXES(AncestorThrottleTest,
                            IgnoreWhenFrameAncestorsPresent);
 
   explicit AncestorThrottle(NavigationHandle* handle);
+  NavigationThrottle::ThrottleCheckResult ProcessResponseImpl(
+      LoggingDisposition);
   void ParseError(const std::string& value, HeaderDisposition disposition);
   void ConsoleError(HeaderDisposition disposition);
 

third_party/WebKit/LayoutTests/external/wpt/x-frame-options/redirect.sub.html

+<!DOCTYPE html>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="./support/helper.js"></script>
+<body>
+<script>
+  async_test(t => {
+    var i = document.createElement('iframe');
+    i.src = "./support/redirect.py?value=DENY&url=/x-frame-options/support/xfo.py%3Fvalue%3DALLOWALL";
+
+    wait_for_message_from(i, t)
+      .then(t.step_func_done(e => {
+        assert_equals(e.data, "Loaded");
+        i.remove();
+      }));
+
+    document.body.appendChild(i);
+  }, "XFO on redirect responses is ignored.");
+</script>

third_party/WebKit/LayoutTests/external/wpt/x-frame-options/support/redirect.py

+def main(request, response):
+    response.status = 302
+    response.headers.set("X-Frame-Options", request.GET.first("value"))
+    response.headers.set("Location", request.GET.first("url"))

tools/metrics/histograms/enums.xml

@@ -48506,6 +48506,10 @@ Full version information for the fingerprint enum values:
   <int value="8" label="BYPASS">
     The &quot;frame-ancestors&quot; CSP directive should take effect instead.
   </int>
+  <int value="9" label="REDIRECT_BLOCKED">
+    A redirect response specified an &quot;X-Frame-Options&quot; header that
+    would block the response.
+  </int>
 </enum>
 
 <enum name="XHRPageDismissalState">