Temporarily send `Sec-Fetch-*` headers via non-secure transport.

After [1], we're stripping `Sec-Fetch-*` headers from non-secure
transport. This is lovely! Except insofar as it also strips the headers
from secure redirect targets. Which is a problem, as it gives attackers
the option of laundering their requests through HTTP to strip the
headers.

This patch removes the secure transport restriction while we figure out
a cleaner way of removing the headers on a per-hop basis.

[1]: https://chromium-review.googlesource.com/c/chromium/src/+/1647354

Bug: 995745, 971938, 964053
Change-Id: Icec4e685902b7be2983bb81b7289ac9b45467782
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1762079
Commit-Queue: Mike West <mkwst@chromium.org>
Reviewed-by: Eric Lawrence [MSFT] <ericlaw@microsoft.com>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#689428}

content/browser/frame_host/navigation_request.cc

@@ -277,7 +277,10 @@ void AddAdditionalRequestHeaders(net::HttpRequestHeaders* headers,
 
   // TODO(mkwst): Extract this logic out somewhere that can be shared between
   // Blink and //content.
-  if (IsFetchMetadataEnabled() && IsOriginSecure(url)) {
+  //
+  // TODO(964053, 989502): These headers ought to be restricted to secure
+  // transport.
+  if (IsFetchMetadataEnabled()) {
     // Navigations that aren't triggerable from the web (e.g. typing in the
     // address bar, or clicking a bookmark) are labeled as user-initiated.
     std::string user_value = has_user_gesture ? "?1" : std::string();

content/browser/loader/browser_initiated_resource_request.cc

@@ -37,7 +37,9 @@ bool IsFetchMetadataDestinationEnabled() {
 
 void SetFetchMetadataHeadersForBrowserInitiatedRequest(
     network::ResourceRequest* resource_request) {
-  if (IsFetchMetadataEnabled() && IsOriginSecure(resource_request->url)) {
+  // TODO(964053, 989502): These headers ought to be restricted to secure
+  // transport.
+  if (IsFetchMetadataEnabled()) {
     // Sec-Fetch-Mode exposes request's mode.
     // https://w3c.github.io/webappsec-fetch-metadata/#sec-fetch-mode-header
     resource_request->headers.SetHeaderIfMissing(

services/network/url_loader.cc

@@ -816,9 +816,6 @@ void URLLoader::OnReceivedRedirect(net::URLRequest* url_request,
     return;
   }
 
-  // We may need to clear out old Sec- prefixed request headers. We'll attempt
-  // to do this before we re-add any.
-  MaybeRemoveSecHeaders(url_request_.get(), redirect_info.new_url);
   SetSecFetchSiteHeader(url_request_.get(), &redirect_info.new_url,
                         *factory_params_);
 

services/network/url_loader_unittest.cc

@@ -2178,11 +2178,14 @@ TEST_F(URLLoaderTest, DowngradeRemovesSecHeaders) {
 
   // We should have removed our special Sec-CH- and Sec-Fetch- prefixed headers
   // and left the others. We are now operating on an un-trustworthy context.
+  //
+  // TODO(mkwst): Reverting these expectations temporarily while resolving
+  // https://crbug.com/995745.
   const auto& request_headers2 = sent_request().headers;
-  EXPECT_EQ(request_headers2.end(), request_headers2.find("Sec-CH-UA"));
+  EXPECT_NE(request_headers2.end(), request_headers2.find("Sec-CH-UA"));
   EXPECT_EQ("Value2", request_headers2.find("Sec-Other-Type")->second);
   EXPECT_EQ("Value3", request_headers2.find("Other-Header")->second);
-  EXPECT_EQ(request_headers2.end(), request_headers2.find("Sec-Fetch-Site"));
+  EXPECT_NE(request_headers2.end(), request_headers2.find("Sec-Fetch-Site"));
 }
 
 // Validate Sec- prefixed headers are properly handled when redirecting from
@@ -2237,20 +2240,26 @@ TEST_F(URLLoaderTest, RedirectChainRemovesAndAddsSecHeaders) {
   // Special Sec-CH- and Sec-Fetch- prefixed headers should have been removed
   // and the others left alone. We are now operating on an un-trustworthy
   // context.
+  //
+  // TODO(mkwst): Reverting these expectations temporarily while resolving
+  // https://crbug.com/995745.
   const auto& request_headers2 = sent_request().headers;
-  EXPECT_EQ(request_headers2.end(), request_headers2.find("Sec-CH-UA"));
+  EXPECT_NE(request_headers2.end(), request_headers2.find("Sec-CH-UA"));
   EXPECT_EQ("Value2", request_headers2.find("Sec-Other-Type")->second);
   EXPECT_EQ("Value3", request_headers2.find("Other-Header")->second);
-  EXPECT_EQ(request_headers2.end(), request_headers2.find("Sec-Fetch-Site"));
+  EXPECT_NE(request_headers2.end(), request_headers2.find("Sec-Fetch-Site"));
 
   // Now follow the final redirect back to a trustworthy destination and
   // re-validate.
+  //
+  // TODO(mkwst): Reverting these expectations temporarily while resolving
+  // https://crbug.com/995745.
   loader->FollowRedirect({}, {}, base::nullopt);
   client()->RunUntilComplete();
   delete_run_loop.Run();
 
   const auto& request_headers3 = sent_request().headers;
-  EXPECT_EQ(request_headers3.end(), request_headers3.find("Sec-CH-UA"));
+  EXPECT_NE(request_headers3.end(), request_headers3.find("Sec-CH-UA"));
   EXPECT_EQ("Value2", request_headers3.find("Sec-Other-Type")->second);
   EXPECT_EQ("Value3", request_headers3.find("Other-Header")->second);
   EXPECT_EQ("cross-site", request_headers3.find("Sec-Fetch-Site")->second);

third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc

@@ -331,8 +331,9 @@ void SetSecFetchHeaders(
     const FetchClientSettingsObject& fetch_client_settings_object) {
   scoped_refptr<SecurityOrigin> url_origin =
       SecurityOrigin::Create(request.Url());
-  if (blink::RuntimeEnabledFeatures::FetchMetadataEnabled() &&
-      url_origin->IsPotentiallyTrustworthy()) {
+  // TODO(964053, 989502): These headers ought to be restricted to secure
+  // transport.
+  if (blink::RuntimeEnabledFeatures::FetchMetadataEnabled()) {
     const char* destination_value =
         GetRequestDestinationFromContext(request.GetRequestContext());
 

third_party/blink/web_tests/TestExpectations

@@ -598,6 +598,23 @@ crbug.com/965491 virtual/layout_ng_experimental/external/wpt/css/vendor-imports/
 crbug.com/845235 virtual/layout_ng_experimental/external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/contain/contain-size-flex-001.html [ Failure ]
 crbug.com/845235 virtual/layout_ng_experimental/external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/contain/contain-size-inline-flex-001.html [ Failure ]
 
+# We're sending `Sec-Fetch-*` headers over non-secure transport until we fix https://crbug.com/995745
+crbug.com/995745 external/wpt/fetch/sec-metadata/fetch.tentative.sub.html [ Failure ]
+crbug.com/995745 external/wpt/fetch/sec-metadata/iframe.tentative.sub.html [ Failure ]
+crbug.com/995745 external/wpt/fetch/sec-metadata/redirect/multiple-redirect-https-downgrade-upgrade.tentative.sub.html [ Failure ]
+crbug.com/995745 external/wpt/fetch/sec-metadata/redirect/redirect-http-upgrade.tentative.sub.html [ Failure ]
+crbug.com/995745 external/wpt/fetch/sec-metadata/script.tentative.sub.html [ Failure ]
+crbug.com/995745 virtual/blink-cors/external/wpt/fetch/sec-metadata/fetch.tentative.sub.html [ Failure ]
+crbug.com/995745 virtual/blink-cors/external/wpt/fetch/sec-metadata/iframe.tentative.sub.html [ Failure ]
+crbug.com/995745 virtual/blink-cors/external/wpt/fetch/sec-metadata/redirect/multiple-redirect-https-downgrade-upgrade.tentative.sub.html [ Failure ]
+crbug.com/995745 virtual/blink-cors/external/wpt/fetch/sec-metadata/redirect/redirect-http-upgrade.tentative.sub.html [ Failure ]
+crbug.com/995745 virtual/blink-cors/external/wpt/fetch/sec-metadata/script.tentative.sub.html [ Failure ]
+crbug.com/995745 virtual/omt-worker-fetch/external/wpt/fetch/sec-metadata/fetch.tentative.sub.html [ Failure ]
+crbug.com/995745 virtual/omt-worker-fetch/external/wpt/fetch/sec-metadata/iframe.tentative.sub.html [ Failure ]
+crbug.com/995745 virtual/omt-worker-fetch/external/wpt/fetch/sec-metadata/redirect/multiple-redirect-https-downgrade-upgrade.tentative.sub.html [ Failure ]
+crbug.com/995745 virtual/omt-worker-fetch/external/wpt/fetch/sec-metadata/redirect/redirect-http-upgrade.tentative.sub.html [ Failure ]
+crbug.com/995745 virtual/omt-worker-fetch/external/wpt/fetch/sec-metadata/script.tentative.sub.html [ Failure ]
+
 # [css-align]
 
 crbug.com/599828 external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/align3/flex-abspos-staticpos-align-content-001.html [ Failure ]
@@ -3446,7 +3463,6 @@ crbug.com/626703 [ Retina ] virtual/blink-cors/external/wpt/fetch/api/request/de
 crbug.com/626703 [ Retina ] virtual/blink-cors/external/wpt/referrer-policy/no-referrer/http-rp/cross-origin/http-http/fetch-request/keep-origin-redirect/generic.http.html [ Timeout ]
 crbug.com/626703 [ Retina ] virtual/blink-cors/external/wpt/fetch/api/cors/cors-preflight-star.any.worker.html [ Timeout ]
 crbug.com/626703 [ Retina ] virtual/blink-cors/external/wpt/referrer-policy/no-referrer/meta-referrer/cross-origin/http-https/img-tag/keep-origin-redirect/generic.http.html [ Timeout ]
-crbug.com/626703 [ Retina ] virtual/blink-cors/external/wpt/fetch/sec-metadata/script.tentative.sub.html [ Timeout ]
 crbug.com/626703 [ Retina ] virtual/blink-cors/external/wpt/fetch/api/basic/request-forbidden-headers.any.worker.html [ Timeout ]
 crbug.com/626703 [ Retina ] virtual/blink-cors/external/wpt/referrer-policy/no-referrer-when-downgrade/http-rp/same-origin/http-https/fetch-request/no-redirect/upgrade-protocol.http.html [ Timeout ]
 crbug.com/626703 [ Retina ] virtual/blink-cors/external/wpt/referrer-policy/no-referrer-when-downgrade/meta-referrer/same-origin/http-http/worker-request/no-redirect/insecure-protocol.http.html [ Timeout ]
@@ -3489,7 +3505,6 @@ crbug.com/626703 [ Retina ] virtual/blink-cors/external/wpt/referrer-policy/no-r
 crbug.com/626703 [ Retina ] virtual/blink-cors/external/wpt/fetch/sec-metadata/fetch-via-serviceworker--respondWith.tentative.https.sub.html [ Timeout ]
 crbug.com/626703 [ Retina ] virtual/blink-cors/external/wpt/fetch/content-type/script.window.html [ Timeout ]
 crbug.com/626703 [ Retina ] virtual/blink-cors/external/wpt/referrer-policy/origin-when-cross-origin/attr-referrer/cross-origin/http-http/a-tag/no-redirect/cross-origin.http.html [ Timeout ]
-crbug.com/626703 [ Retina ] virtual/blink-cors/external/wpt/fetch/sec-metadata/redirect/multiple-redirect-https-downgrade-upgrade.tentative.sub.html [ Timeout ]
 crbug.com/626703 [ Retina ] virtual/blink-cors/external/wpt/fetch/api/response/response-stream-disturbed-1.html [ Timeout ]
 crbug.com/626703 [ Retina ] virtual/blink-cors/external/wpt/fetch/nosniff/importscripts.html [ Timeout ]
 crbug.com/626703 [ Retina ] virtual/blink-cors/external/wpt/referrer-policy/no-referrer-when-downgrade/attr-referrer/cross-origin/http-https/script-tag/swap-origin-redirect/upgrade-protocol.http.html [ Timeout ]

third_party/blink/web_tests/external/wpt/fetch/sec-metadata/redirect/multiple-redirect-https-downgrade-upgrade.tentative.sub.html

@@ -38,8 +38,8 @@
     }, "Https downgrade-upgrade font => No headers");
   });
 
-  promise_test(() =>
-    requestViaImage(secureRedirectURL + encodeURIComponent(insecureRedirectURL + encodeURIComponent("https://{{host}}:{{ports[https][0]}}/common/security-features/subresource/image.py")))
+  promise_test(() => {
+    return requestViaImage(secureRedirectURL + encodeURIComponent(insecureRedirectURL + encodeURIComponent("https://{{host}}:{{ports[https][0]}}/common/security-features/subresource/image.py")))
       .then(result => {
          headers = result.headers;
          got = {
@@ -56,7 +56,8 @@
            "user": undefined,
            "mode": undefined,
          });
-  }), "Https downgrade-upgrade image => No headers");
+      });
+  }, "Https downgrade-upgrade image => No headers");
 </script>
 <script src="https://{{host}}:{{ports[https][0]}}/fetch/api/resources/redirect.py?location=http%3A%2F%2F{{host}}%3A{{ports[http][0]}}%2Ffetch%2Fapi%2Fresources%2Fredirect.py%3Flocation%3Dhttps%253A%252F%252F{{host}}%253A{{ports[https][0]}}%252Ffetch%252Fsec-metadata%252Fresources%252Fecho-as-script.py"></script>
 <script>

third_party/blink/web_tests/external/wpt/fetch/sec-metadata/redirect/redirect-http-upgrade.tentative.sub.html

@@ -37,8 +37,8 @@
     }, "Http upgrade font => No headers");
   });
 
-  promise_test(() =>
-    requestViaImage(insecureRedirectURL + encodeURIComponent("https://{{host}}:{{ports[https][0]}}/common/security-features/subresource/image.py"))
+  promise_test(() => {
+    return requestViaImage(insecureRedirectURL + encodeURIComponent("https://{{host}}:{{ports[https][0]}}/common/security-features/subresource/image.py"))
       .then(result => {
          headers = result.headers;
          got = {
@@ -55,7 +55,8 @@
            "user": undefined,
            "mode": undefined,
          });
-  }), "Http upgrade image => No headers");
+      });
+  }, "Http upgrade image => No headers");
 </script>
 <script src="http://{{host}}:{{ports[http][0]}}/fetch/api/resources/redirect.py?location=https%3A%2F%2F{{host}}%3A{{ports[https][0]}}%2Ffetch%2Fsec-metadata%2Fresources%2Fecho-as-script.py"></script>
 <script>

third_party/blink/web_tests/external/wpt/fetch/sec-metadata/redirect/redirect-https-downgrade.tentative.sub-expected.txt

+This is a testharness.js-based test.
+FAIL redirect-https-downgrade Uncaught SyntaxError: Unexpected token 'return'
+Harness: the test ran to completion.
+

third_party/blink/web_tests/external/wpt/fetch/sec-metadata/redirect/redirect-https-downgrade.tentative.sub.html

@@ -38,7 +38,7 @@
   });
 
   promise_test(() =>
-    requestViaImage(secureRedirectURL + encodeURIComponent("http://{{host}}:{{ports[http][0]}}/common/security-features/subresource/image.py"))
+    return requestViaImage(secureRedirectURL + encodeURIComponent("http://{{host}}:{{ports[http][0]}}/common/security-features/subresource/image.py"))
       .then(result => {
          headers = result.headers;
          got = {

third_party/blink/web_tests/external/wpt/fetch/sec-metadata/resources/redirectTestHelper.sub.js

@@ -78,7 +78,7 @@ function RunCommonRedirectTests(testNamePrefix, urlHelperMethod, expectedResults
       e.onload = e => {
         fetch("/fetch/sec-metadata/resources/record-header.py?retrieve=true&file=" + key)
           .then(response => response.text())
-          .then(text => assert_header_equals(text, expectedResults))
+          .then(t.step_func(text => assert_header_equals(text, expectedResults)))
           .then(_ => resolve())
           .catch(e => reject(e));
         };
@@ -102,7 +102,7 @@ function RunCommonRedirectTests(testNamePrefix, urlHelperMethod, expectedResults
       e.onload = e => {
       fetch("/fetch/sec-metadata/resources/record-header.py?retrieve=true&file=" + key)
         .then(response => response.text())
-        .then(text => assert_header_equals(text, expectedResults))
+        .then(t.step_func(text => assert_header_equals(text, expectedResults)))
         .then(_ => resolve())
         .catch(e => reject(e));
       };
@@ -154,7 +154,7 @@ function RunCommonRedirectTests(testNamePrefix, urlHelperMethod, expectedResults
       e.onload = e => {
         fetch("/fetch/sec-metadata/resources/record-header.py?retrieve=true&file=" + key)
           .then(response => response.text())
-          .then(text => assert_header_equals(text, expectedResults))
+          .then(t.step_func(text => assert_header_equals(text, expectedResults)))
           .then(_ => resolve())
           .catch(e => reject(e));
       };
@@ -171,7 +171,7 @@ function RunCommonRedirectTests(testNamePrefix, urlHelperMethod, expectedResults
       el.onload = t.step_func(_ => {
         fetch("/fetch/sec-metadata/resources/record-header.py?retrieve=true&file=" + key)
           .then(response => response.text())
-          .then(text => assert_header_equals(text, expectedResults))
+          .then(t.step_func(text => assert_header_equals(text, expectedResults)))
           .then(_ => resolve());
       });
       video.appendChild(el);

third_party/blink/web_tests/virtual/blink-cors/external/wpt/fetch/sec-metadata/redirect/multiple-redirect-https-downgrade-upgrade.tentative.sub-expected.txt

-This is a testharness.js-based test.
-PASS Https downgrade-upgrade iframe => No headers
-PASS Https downgrade-upgrade top level navigation => No headers
-PASS Https downgrade-upgrade embed => No headers
-FAIL Https downgrade-upgrade fetch() api => No headers assert_equals: dest expected "" but got "empty"
-PASS Https downgrade-upgrade object => No headers
-PASS Https downgrade-upgrade prefetch => No headers
-PASS Https downgrade-upgrade preload => No headers
-PASS Https downgrade-upgrade stylesheet => No headers
-PASS Https downgrade-upgrade track => No headers
-PASS Https downgrade-upgrade image => No headers
-PASS Https downgrade-upgrade script => No headers
-PASS Https downgrade-upgrade font => No headers
-Harness: the test ran to completion.
-

third_party/blink/web_tests/virtual/blink-cors/external/wpt/fetch/sec-metadata/redirect/redirect-http-upgrade.tentative.sub-expected.txt

-This is a testharness.js-based test.
-PASS Http upgrade iframe => No headers
-PASS Http upgrade top level navigation => No headers
-PASS Http upgrade embed => No headers
-FAIL Http upgrade fetch() api => No headers assert_equals: dest expected "" but got "empty"
-PASS Http upgrade object => No headers
-PASS Http upgrade prefetch => No headers
-PASS Http upgrade preload => No headers
-PASS Http upgrade stylesheet => No headers
-PASS Http upgrade track => No headers
-PASS Http upgrade image => No headers
-PASS Http upgrade script => No headers
-PASS Http upgrade font => No headers
-Harness: the test ran to completion.
-