[client-hints] Ensure by-default&legacy hints behave with XO redirects

As a followup to [1], this CL ensures that on-by-default Client Hints
don't get removed from cross-origin redirects.
It similarly makes sure that legacy hints that don't abide to
FeaturePolicy get a free pass on the removal in that case.
Finally, it makes sure that UA-Mobile's FeaturePolicy's default value
is "all".

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

Bug: 911952, 1082072
Change-Id: I1f329a3c24397a287a1cbc333cd0976bb16d640a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2199107
Commit-Queue: Yoav Weiss <yoavweiss@chromium.org>
Reviewed-by: Maksim Orlovich <morlovich@chromium.org>
Reviewed-by: Aaron Tagliaboschi <aarontag@chromium.org>
Cr-Commit-Position: refs/heads/master@{#768549}

third_party/blink/common/client_hints/client_hints.cc

@@ -7,11 +7,13 @@
 #include <utility>
 #include <vector>
 
+#include "base/feature_list.h"
 #include "base/optional.h"
 #include "base/stl_util.h"
 #include "base/strings/string_tokenizer.h"
 #include "base/strings/string_util.h"
 #include "third_party/blink/public/common/feature_policy/feature_policy.h"
+#include "third_party/blink/public/common/features.h"
 #include "url/origin.h"
 
 namespace blink {
@@ -34,11 +36,16 @@ const char* const kClientHintsHeaderMapping[] = {
     "sec-ch-ua-platform-version",
 };
 
+const unsigned kClientHintsNumberOfLegacyHints = 4;
+
 const mojom::FeaturePolicyFeature kClientHintsFeaturePolicyMapping[] = {
+    // Legacy Hints that are sent cross-origin regardless of FeaturePolicy when
+    // kAllowClientHintsToThirdParty is enabled
     mojom::FeaturePolicyFeature::kClientHintDeviceMemory,
     mojom::FeaturePolicyFeature::kClientHintDPR,
     mojom::FeaturePolicyFeature::kClientHintWidth,
     mojom::FeaturePolicyFeature::kClientHintViewportWidth,
+    // End of legacy hints.
     mojom::FeaturePolicyFeature::kClientHintRTT,
     mojom::FeaturePolicyFeature::kClientHintDownlink,
     mojom::FeaturePolicyFeature::kClientHintECT,
@@ -119,6 +126,15 @@ base::Optional<std::vector<network::mojom::WebClientHintsType>> FilterAcceptCH(
   return base::make_optional(std::move(result));
 }
 
+namespace {
+
+bool IsClientHintSentByDefault(network::mojom::WebClientHintsType type) {
+  return (type == network::mojom::WebClientHintsType::kUA ||
+          type == network::mojom::WebClientHintsType::kUAMobile);
+}
+
+}  // namespace
+
 // Add a list of Client Hints headers to be removed to the output vector, based
 // on FeaturePolicy and the url's origin.
 void FindClientHintsToRemove(const FeaturePolicy* feature_policy,
@@ -126,13 +142,22 @@ void FindClientHintsToRemove(const FeaturePolicy* feature_policy,
                              std::vector<std::string>* removed_headers) {
   DCHECK(removed_headers);
   url::Origin origin = url::Origin::Create(url);
-  for (size_t i = 0; i < blink::kClientHintsMappingsCount; ++i) {
-    // TODO(yoav): When FeaturePolicy is not present, we need to conserve the
-    // hints that are sent by default.
-    // TODO(yoav): We need to take legacy hints into account here.
-    if (!feature_policy ||
+  size_t startHint = 0;
+  if (base::FeatureList::IsEnabled(features::kAllowClientHintsToThirdParty)) {
+    // Do not remove any legacy Client Hints
+    startHint = kClientHintsNumberOfLegacyHints;
+  }
+  for (size_t i = startHint; i < blink::kClientHintsMappingsCount; ++i) {
+    // Remove the hint if any is true:
+    // * Feature policy is null (we're in a sync XHR case) and the hint is not
+    // sent by default.
+    // * Feature policy exists and doesn't allow for the hint.
+    if ((!feature_policy &&
+         !IsClientHintSentByDefault(
+             static_cast<network::mojom::WebClientHintsType>(i))) ||
+        (feature_policy &&
          !feature_policy->IsFeatureEnabledForOrigin(
-            blink::kClientHintsFeaturePolicyMapping[i], origin)) {
+             blink::kClientHintsFeaturePolicyMapping[i], origin))) {
       removed_headers->push_back(blink::kClientHintsHeaderMapping[i]);
     }
   }

third_party/blink/common/client_hints/client_hints_unittest.cc

@@ -6,9 +6,12 @@
 
 #include <iostream>
 
+#include "base/test/scoped_feature_list.h"
 #include "services/network/public/mojom/web_client_hints_types.mojom-shared.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/blink/public/common/features.h"
+#include "url/gurl.h"
 
 using testing::UnorderedElementsAre;
 
@@ -91,4 +94,34 @@ TEST(ClientHintsTest, FilterAcceptCH) {
               UnorderedElementsAre(network::mojom::WebClientHintsType::kRtt));
 }
 
+// Checks that the removed header list doesn't include legacy headers nor the
+// on-by-default ones, when the kAllowClientHintsToThirdParty flag is on.
+TEST(ClientHintsTest, FindClientHintsToRemoveLegacy) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(
+      features::kAllowClientHintsToThirdParty);
+  std::vector<std::string> removed_headers;
+  FindClientHintsToRemove(nullptr, GURL(), &removed_headers);
+  EXPECT_THAT(removed_headers,
+              UnorderedElementsAre("rtt", "downlink", "ect", "sec-ch-lang",
+                                   "sec-ch-ua-arch", "sec-ch-ua-platform",
+                                   "sec-ch-ua-model", "sec-ch-ua-full-version",
+                                   "sec-ch-ua-platform-version"));
+}
+
+// Checks that the removed header list includes legacy headers but not the
+// on-by-default ones, when the kAllowClientHintsToThirdParty flag is off.
+TEST(ClientHintsTest, FindClientHintsToRemoveNoLegacy) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndDisableFeature(
+      features::kAllowClientHintsToThirdParty);
+  std::vector<std::string> removed_headers;
+  FindClientHintsToRemove(nullptr, GURL(), &removed_headers);
+  EXPECT_THAT(removed_headers,
+              UnorderedElementsAre(
+                  "device-memory", "dpr", "width", "viewport-width", "rtt",
+                  "downlink", "ect", "sec-ch-lang", "sec-ch-ua-arch",
+                  "sec-ch-ua-platform", "sec-ch-ua-model",
+                  "sec-ch-ua-full-version", "sec-ch-ua-platform-version"));
+}
 }  // namespace blink

third_party/blink/common/feature_policy/feature_policy.cc

@@ -411,7 +411,7 @@ const FeaturePolicy::FeatureList& FeaturePolicy::GetDefaultFeatureList() {
        {mojom::FeaturePolicyFeature::kClientHintUAModel,
         FeatureDefault(FeaturePolicy::FeatureDefault::EnableForSelf)},
        {mojom::FeaturePolicyFeature::kClientHintUAMobile,
-        FeatureDefault(FeaturePolicy::FeatureDefault::EnableForSelf)},
+        FeatureDefault(FeaturePolicy::FeatureDefault::EnableForAll)},
        {mojom::FeaturePolicyFeature::kClientHintUAFullVersion,
         FeatureDefault(FeaturePolicy::FeatureDefault::EnableForSelf)},
        {mojom::FeaturePolicyFeature::kClientHintUAPlatformVersion,

third_party/blink/common/features.cc

@@ -546,6 +546,17 @@ const base::FeatureParam<int> kInstallingServiceWorkerOutstandingThrottledLimit{
 const base::Feature kResamplingScrollEvents{"ResamplingScrollEvents",
                                             base::FEATURE_ENABLED_BY_DEFAULT};
 
+// Enables the device-memory, resource-width, viewport-width and DPR client
+// hints to be sent to third-party origins if the first-party has opted in to
+// receiving client hints, regardless of Feature Policy.
+#if defined(OS_ANDROID)
+const base::Feature kAllowClientHintsToThirdParty{
+    "AllowClientHintsToThirdParty", base::FEATURE_ENABLED_BY_DEFAULT};
+#else
+const base::Feature kAllowClientHintsToThirdParty{
+    "AllowClientHintsToThirdParty", base::FEATURE_DISABLED_BY_DEFAULT};
+#endif
+
 const char kScrollPredictorNameLsq[] = "lsq";
 const char kScrollPredictorNameKalman[] = "kalman";
 const char kScrollPredictorNameLinearFirst[] = "linear_first";

third_party/blink/public/common/features.h

@@ -191,6 +191,11 @@ BLINK_COMMON_EXPORT extern const base::FeatureParam<int>
 // Enables resampling GestureScroll events on compositor thread.
 BLINK_COMMON_EXPORT extern const base::Feature kResamplingScrollEvents;
 
+// Enables the device-memory, resource-width, viewport-width and DPR client
+// hints to be sent to third-party origins if the first-party has opted in to
+// receiving client hints, regardless of Feature Policy.
+BLINK_COMMON_EXPORT extern const base::Feature kAllowClientHintsToThirdParty;
+
 // The type of scroll predictor to use for the resampling scroll events. These
 // values are used as the 'predictor' feature param for
 // |kResamplingScrollEvents|.

third_party/blink/renderer/core/loader/frame_fetch_context.cc

@@ -42,6 +42,7 @@
 #include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h"
 #include "third_party/blink/public/common/client_hints/client_hints.h"
 #include "third_party/blink/public/common/device_memory/approximated_device_memory.h"
+#include "third_party/blink/public/common/features.h"
 #include "third_party/blink/public/mojom/conversions/conversions.mojom-blink.h"
 #include "third_party/blink/public/mojom/feature_policy/feature_policy.mojom-blink.h"
 #include "third_party/blink/public/mojom/fetch/fetch_api_request.mojom-blink.h"
@@ -121,26 +122,15 @@ namespace {
 // If that flag is disabled (the default), then all hints are always sent for
 // first-party subresources, and the kAllowClientHintsToThirdParty feature
 // controls whether some specific hints are sent to third parties. (Only
-// device-memory, resource-width, viewport-width and the limited UA hints are
-// sent under this model). This feature is enabled by default on Android, and
-// disabled by default on all other platforms.
+// device-memory, resource-width, viewport-width and DPR are sent under this
+// model). This feature is enabled by default on Android, and disabled by
+// default on all other platforms.
 //
 // When the runtime flag is enabled, all client hints except UA are controlled
 // entirely by feature policy on all platforms. In that case, hints will
 // generally be sent for first-party resources, and not for third-party
 // resources, unless specifically enabled by policy.
 
-// If kAllowClientHintsToThirdParty is enabled, then device-memory,
-// resource-width and viewport-width client hints can be sent to third-party
-// origins if the first-party has opted in to receiving client hints.
-#if defined(OS_ANDROID)
-const base::Feature kAllowClientHintsToThirdParty{
-    "AllowClientHintsToThirdParty", base::FEATURE_ENABLED_BY_DEFAULT};
-#else
-const base::Feature kAllowClientHintsToThirdParty{
-    "AllowClientHintsToThirdParty", base::FEATURE_DISABLED_BY_DEFAULT};
-#endif
-
 // Determines FetchCacheMode for |frame|. This FetchCacheMode should be a base
 // policy to consider one of each resource belonging to the frame, and should
 // not count resource specific conditions in.
@@ -514,7 +504,7 @@ void FrameFetchContext::AddClientHintsIfNecessary(
   bool is_1p_origin = IsFirstPartyOrigin(request.Url());
 
   if (!RuntimeEnabledFeatures::FeaturePolicyForClientHintsEnabled() &&
-      !base::FeatureList::IsEnabled(kAllowClientHintsToThirdParty) &&
+      !base::FeatureList::IsEnabled(features::kAllowClientHintsToThirdParty) &&
       !is_1p_origin) {
     // No client hints for 3p origins.
     return;
@@ -953,7 +943,7 @@ bool FrameFetchContext::ShouldSendClientHint(
   bool origin_ok;
 
   if (mode == ClientHintsMode::kLegacy &&
-      base::FeatureList::IsEnabled(kAllowClientHintsToThirdParty)) {
+      base::FeatureList::IsEnabled(features::kAllowClientHintsToThirdParty)) {
     origin_ok = true;
   } else if (RuntimeEnabledFeatures::FeaturePolicyForClientHintsEnabled()) {
     origin_ok =

third_party/blink/web_tests/external/wpt/client-hints/accept-ch-stickiness/cross-origin-subresource-redirect-with-fp-delegation.https.html

@@ -9,7 +9,7 @@
 
 <script>
 // Make sure a cross origin subresource that gets redirected with Feature Policy delegation keeps the initial request's Client Hints.
-const test_name = "cross-origin subresource redirect with Feature Policy delegaation";
+const test_name = "cross-origin subresource redirect with Feature Policy delegation";
 verify_subresource_state("resources/accept-ch-and-redir.py?url=" + get_host_info()["HTTPS_REMOTE_ORIGIN"] + "/client-hints/accept-ch-stickiness/resources/expect-received.py", test_name);
 </script>
 </body>

third_party/blink/web_tests/external/wpt/client-hints/accept-ch-stickiness/resources/expect-received.py

@@ -5,7 +5,7 @@ def main(request, response):
     verify_navigation_state() in accept-ch-test.js
     """
 
-    if "device-memory" in request.headers:
+    if "device-memory" in request.headers and "sec-ch-ua" in request.headers and "sec-ch-ua-mobile" in request.headers:
       result = "PASS"
     else:
       result = "FAIL"

third_party/blink/web_tests/virtual/disable-user-agent-client-hint-feature/external/wpt/client-hints/accept-ch-stickiness/cross-origin-iframe-redirect-with-fp-delegation.https-expected.txt

+This is a testharness.js-based test.
+FAIL Iframe redirect with Feature Policy delegation got client hints according to expectations. assert_equals: message from opened frame expected "PASS" but got "FAIL"
+Harness: the test ran to completion.
+

third_party/blink/web_tests/virtual/legacy-client-hints-no-fp-delegation/external/wpt/client-hints/accept-ch-stickiness/cross-origin-subresource-redirect-with-fp-delegation.https-expected.txt → third_party/blink/web_tests/virtual/disable-user-agent-client-hint-feature/external/wpt/client-hints/accept-ch-stickiness/cross-origin-subresource-redirect-with-fp-delegation.https-expected.txt

 This is a testharness.js-based test.
-FAIL cross-origin subresource redirect with Feature Policy delegaation got client hints according to expectations. assert_true: expected true got false
+FAIL cross-origin subresource redirect with Feature Policy delegation got client hints according to expectations. assert_true: expected true got false
 Harness: the test ran to completion.
 

third_party/blink/web_tests/virtual/disable-user-agent-client-hint-feature/external/wpt/client-hints/accept-ch-stickiness/same-origin-navigation-redirect.https-expected.txt

+This is a testharness.js-based test.
+PASS redirect on navigation precondition: Test that the browser does not have client hints preferences cached
+FAIL redirect on navigation got client hints according to expectations. assert_equals: message from opened page expected "PASS" but got "FAIL"
+Harness: the test ran to completion.
+

third_party/blink/web_tests/virtual/disable-user-agent-client-hint-feature/external/wpt/client-hints/accept-ch-stickiness/same-origin-subresource-redirect-opted-in.https-expected.txt

+This is a testharness.js-based test.
+FAIL same-origin subresource redirect with opt-in got client hints according to expectations. assert_true: expected true got false
+Harness: the test ran to completion.
+

third_party/blink/web_tests/virtual/legacy-client-hints-no-fp-delegation/external/wpt/client-hints/accept-ch-stickiness/cross-origin-subresource-redirect.https-expected.txt

+This is a testharness.js-based test.
+FAIL cross-origin subresource redirect got client hints according to expectations. assert_true: expected true got false
+Harness: the test ran to completion.
+

third_party/blink/web_tests/virtual/legacy-client-hints-no-fp-delegation/external/wpt/client-hints/accept-ch-stickiness/cross-origin-syncxhr-redirect.https-expected.txt

+This is a testharness.js-based test.
+FAIL cross-origin sync XHR redirect got client hints according to expectations. assert_true: expected true got false
+Harness: the test ran to completion.
+

third_party/blink/web_tests/virtual/legacy-client-hints/external/wpt/client-hints/accept-ch-stickiness/cross-origin-subresource-redirect.https-expected.txt

+This is a testharness.js-based test.
+FAIL cross-origin subresource redirect got client hints according to expectations. assert_true: expected true got false
+Harness: the test ran to completion.
+

third_party/blink/web_tests/virtual/legacy-client-hints/external/wpt/client-hints/accept-ch-stickiness/cross-origin-syncxhr-redirect.https-expected.txt

+This is a testharness.js-based test.
+FAIL cross-origin sync XHR redirect got client hints according to expectations. assert_true: expected true got false
+Harness: the test ran to completion.
+