[client-hints] Allow 3P legacy Client-Hints to work with Feature Policy

Currently there are two different flags that control sending Client Hints
to cross-origin resources: Feature Policy delegation (off-by-default) and
a build flag that sends legacy hints on Android.

This CL makes sure that these two flags play nice together, and adds
virtual test suites to exercise them.

BUG: 1022366
Change-Id: I00ed4e9ed5a64e378def3d90227f238b9b3eb374
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1893196
Commit-Queue: Yoav Weiss <yoavweiss@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#716101}

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

@@ -570,14 +570,9 @@ void FrameFetchContext::AddClientHintsIfNecessary(
                                                                 &enabled_hints);
   }
 
-  // TODO(iclelland): If feature policy control over client hints ships, remove
-  // the runtime flag check for the next four hints. Currently, when the
-  // kAllowClientHintsToThirdParty feature is enabled, but the runtime flag is
-  // *not* set, the behaviour is that these four hints will be sent on all
-  // eligible requests. Feature policy control is intended to change that
-  // default.
-
-  if ((!RuntimeEnabledFeatures::FeaturePolicyForClientHintsEnabled() ||
+  // The next 4 hints should be enabled if we're allowing legacy hints to third
+  // parties, or if FeaturePolicy delegation says they are allowed.
+  if ((base::FeatureList::IsEnabled(kAllowClientHintsToThirdParty) ||
        policy->IsFeatureEnabledForOrigin(
            mojom::FeaturePolicyFeature::kClientHintDeviceMemory,
            resource_origin)) &&
@@ -590,7 +585,7 @@ void FrameFetchContext::AddClientHintsIfNecessary(
   }
 
   float dpr = GetDevicePixelRatio();
-  if ((!RuntimeEnabledFeatures::FeaturePolicyForClientHintsEnabled() ||
+  if ((base::FeatureList::IsEnabled(kAllowClientHintsToThirdParty) ||
        policy->IsFeatureEnabledForOrigin(
            mojom::FeaturePolicyFeature::kClientHintDPR, resource_origin)) &&
       ShouldSendClientHint(mojom::WebClientHintsType::kDpr, hints_preferences,
@@ -598,19 +593,7 @@ void FrameFetchContext::AddClientHintsIfNecessary(
     request.SetHttpHeaderField("DPR", AtomicString(String::Number(dpr)));
   }
 
-  if ((!RuntimeEnabledFeatures::FeaturePolicyForClientHintsEnabled() ||
-       policy->IsFeatureEnabledForOrigin(
-           mojom::FeaturePolicyFeature::kClientHintWidth, resource_origin)) &&
-      ShouldSendClientHint(mojom::WebClientHintsType::kResourceWidth,
-                           hints_preferences, enabled_hints)) {
-    if (resource_width.is_set) {
-      float physical_width = resource_width.width * dpr;
-      request.SetHttpHeaderField(
-          "Width", AtomicString(String::Number(ceil(physical_width))));
-    }
-  }
-
-  if ((!RuntimeEnabledFeatures::FeaturePolicyForClientHintsEnabled() ||
+  if ((base::FeatureList::IsEnabled(kAllowClientHintsToThirdParty) ||
        policy->IsFeatureEnabledForOrigin(
            mojom::FeaturePolicyFeature::kClientHintViewportWidth,
            resource_origin)) &&
@@ -622,6 +605,18 @@ void FrameFetchContext::AddClientHintsIfNecessary(
         AtomicString(String::Number(GetFrame()->View()->ViewportWidth())));
   }
 
+  if ((base::FeatureList::IsEnabled(kAllowClientHintsToThirdParty) ||
+       policy->IsFeatureEnabledForOrigin(
+           mojom::FeaturePolicyFeature::kClientHintWidth, resource_origin)) &&
+      ShouldSendClientHint(mojom::WebClientHintsType::kResourceWidth,
+                           hints_preferences, enabled_hints)) {
+    if (resource_width.is_set) {
+      float physical_width = resource_width.width * dpr;
+      request.SetHttpHeaderField(
+          "Width", AtomicString(String::Number(ceil(physical_width))));
+    }
+  }
+
   // TODO(iclelland): If feature policy control over client hints ships, remove
   // the runtime flag check and the 1p origin requirement for the remaining
   // hints. Currently, even when the kAllowClientHintsToThirdParty feature is

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

@@ -648,8 +648,15 @@ TEST_F(FrameFetchContextHintsTest, MonitorDeviceMemorySecureTransport) {
   ExpectHeader("https://www.example.com/1.gif", "Viewport-Width", false, "");
   // Without a feature policy header, the client hints should be sent only to
   // the first party origins.
+  // Device-memory is a legacy hint that's sent on Android regardless of Feature
+  // Policy delegation.
+#if defined(OS_ANDROID)
+  ExpectHeader("https://www.someother-example.com/1.gif", "Device-Memory", true,
+               "4");
+#else
   ExpectHeader("https://www.someother-example.com/1.gif", "Device-Memory",
                false, "");
+#endif
 }
 
 // Verify that client hints are not attached when the resources do not belong to
@@ -982,15 +989,28 @@ TEST_F(FrameFetchContextHintsTest, MonitorSomeHintsFeaturePolicy) {
   // With a feature policy header, the client hints should be sent to the
   // declared third party origins.
   ExpectHeader("https://www.example.net/1.gif", "Device-Memory", true, "4");
+  // Device-memory is a legacy hint that's sent on Android regardless of Feature
+  // Policy delegation.
+#if defined(OS_ANDROID)
+  ExpectHeader("https://www.someother-example.com/1.gif", "Device-Memory", true,
+               "4");
+#else
   ExpectHeader("https://www.someother-example.com/1.gif", "Device-Memory",
                false, "");
+#endif
   // `Sec-CH-UA` is special.
   ExpectHeader("https://www.example.net/1.gif", "Sec-CH-UA", true, "");
 
   // Other hints not declared in the policy are still not attached.
   ExpectHeader("https://www.example.net/1.gif", "downlink", false, "");
   ExpectHeader("https://www.example.net/1.gif", "ect", false, "");
+  // DPR is a legacy hint that's sent on Android regardless of Feature Policy
+  // delegation.
+#if defined(OS_ANDROID)
+  ExpectHeader("https://www.example.net/1.gif", "DPR", true, "1");
+#else
   ExpectHeader("https://www.example.net/1.gif", "DPR", false, "");
+#endif
   ExpectHeader("https://www.example.net/1.gif", "Sec-CH-Lang", false, "");
   ExpectHeader("https://www.example.net/1.gif", "Sec-CH-UA-Arch", false, "");
   ExpectHeader("https://www.example.net/1.gif", "Sec-CH-UA-Platform", false,

third_party/blink/web_tests/VirtualTestSuites

@@ -639,5 +639,15 @@
     "prefix": "not-split-http-cache",
     "bases": ["external/wpt/fetch/http-cache"],
     "args": ["--disable-features=SplitCacheByNetworkIsolationKey"]
+  },
+  {
+    "prefix": "legacy-client-hints",
+    "bases": ["external/wpt/client-hints", "wpt_internal/client-hints"],
+    "args": ["--enable-features=AllowClientHintsToThirdParty"]
+  },
+  {
+    "prefix": "legacy-client-hints-no-fp-delegation",
+    "bases": ["external/wpt/client-hints"],
+    "args": ["--enable-features=AllowClientHintsToThirdParty", "--disable-blink-features=FeaturePolicyForClientHints"]
   }
 ]

third_party/blink/web_tests/virtual/legacy-client-hints-no-fp-delegation/README.md

+This virtual test suite is for testing the legacy Client Hints behavior when Feature Policy delegation is disabled, and see that when the flag is on, legacy hints are sent to third party origins.

third_party/blink/web_tests/virtual/legacy-client-hints-no-fp-delegation/external/wpt/client-hints/accept-ch-feature-policy-navigation.https-expected.txt

+This is a testharness.js-based test.
+FAIL Client hints loaded on cross-origin iframe request with feature policy. promise_test: Unhandled rejection with value: "FAIL Device-Memory True None"
+FAIL Client hints loaded on same-origin iframe request with feature policy. promise_test: Unhandled rejection with value: "FAIL DPR False 1"
+PASS Iframe trying to set Accept-CH-Lifetime.
+FAIL Client hints loaded on cross-origin iframe request with feature policy after attempting to set independently. promise_test: Unhandled rejection with value: "FAIL Device-Memory True None"
+Harness: the test ran to completion.
+

third_party/blink/web_tests/virtual/legacy-client-hints-no-fp-delegation/external/wpt/client-hints/accept_ch.tentative.sub.https-expected.txt

+This is a testharness.js-based test.
+PASS Accept-CH header test
+FAIL Cross-Origin Accept-CH header test assert_false: device-memory-received expected false got true
+Harness: the test ran to completion.
+

third_party/blink/web_tests/virtual/legacy-client-hints-no-fp-delegation/external/wpt/client-hints/accept_ch_feature_policy.tentative.sub.https-expected.txt

+This is a testharness.js-based test.
+FAIL Accept-CH header test assert_false: dpr-received expected false got true
+FAIL Cross-Origin Accept-CH header test assert_false: dpr-received expected false got true
+Harness: the test ran to completion.
+

third_party/blink/web_tests/virtual/legacy-client-hints-no-fp-delegation/external/wpt/client-hints/accept_ch_no_feature_policy.tentative.sub.https-expected.txt

+This is a testharness.js-based test.
+PASS Accept-CH header test
+FAIL Cross-Origin Accept-CH header test assert_false: device-memory-received expected false got true
+Harness: the test ran to completion.
+

third_party/blink/web_tests/virtual/legacy-client-hints-no-fp-delegation/external/wpt/client-hints/http_equiv_accept_ch.tentative.sub.https-expected.txt

+This is a testharness.js-based test.
+PASS Same origin Accept-CH http-equiv test
+FAIL Cross-Origin Accept-CH http-equiv test assert_false: device-memory-received expected false got true
+Harness: the test ran to completion.
+

third_party/blink/web_tests/virtual/legacy-client-hints/README.md

+This virtual test suite is for testing the legacy Client Hints behavior alongside Feature Policy delegation, and see that when the flag is on, legacy hints are sent regardless of Feature Policy opt-in.

third_party/blink/web_tests/virtual/legacy-client-hints/external/wpt/client-hints/accept_ch.tentative.sub.https-expected.txt

+This is a testharness.js-based test.
+PASS Accept-CH header test
+FAIL Cross-Origin Accept-CH header test assert_false: device-memory-received expected false got true
+Harness: the test ran to completion.
+

third_party/blink/web_tests/virtual/legacy-client-hints/external/wpt/client-hints/accept_ch_feature_policy.tentative.sub.https-expected.txt

+This is a testharness.js-based test.
+FAIL Accept-CH header test assert_false: dpr-received expected false got true
+FAIL Cross-Origin Accept-CH header test assert_false: dpr-received expected false got true
+Harness: the test ran to completion.
+

third_party/blink/web_tests/virtual/legacy-client-hints/external/wpt/client-hints/accept_ch_no_feature_policy.tentative.sub.https-expected.txt

+This is a testharness.js-based test.
+PASS Accept-CH header test
+FAIL Cross-Origin Accept-CH header test assert_false: device-memory-received expected false got true
+Harness: the test ran to completion.
+

third_party/blink/web_tests/virtual/legacy-client-hints/external/wpt/client-hints/http_equiv_accept_ch.tentative.sub.https-expected.txt

+This is a testharness.js-based test.
+PASS Same origin Accept-CH http-equiv test
+FAIL Cross-Origin Accept-CH http-equiv test assert_false: device-memory-received expected false got true
+Harness: the test ran to completion.
+

third_party/blink/web_tests/virtual/legacy-client-hints/wpt_internal/client-hints/accept_ch_feature_policy_allow_legacy_hints.tentative.sub.https-expected.txt

+This is a testharness.js-based test.
+PASS Accept-CH header test
+PASS Cross-Origin Accept-CH header test
+Harness: the test ran to completion.
+

third_party/blink/web_tests/wpt_internal/client-hints/accept_ch_feature_policy_allow_legacy_hints.tentative.sub.https-expected.txt

+This is a testharness.js-based test.
+FAIL Accept-CH header test assert_true: dpr-received expected true got false
+FAIL Cross-Origin Accept-CH header test assert_true: dpr-received expected true got false
+Harness: the test ran to completion.
+

third_party/blink/web_tests/wpt_internal/client-hints/accept_ch_feature_policy_allow_legacy_hints.tentative.sub.https.html

+<html>
+<body>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="/common/get-host-info.sub.js"></script>
+<script>
+
+// If the response for the HTML file contains "Accept-CH" in the response
+// headers, then the browser should attach the specified client hints in the
+// HTTP request headers depending on whether the resource is being fetched from
+// the same origin or a different origin. Test this functionality by fetching
+// same-origin and cross-origin resources from this page. The response headers
+// for this page include "Accept-CH: device-memory, dpr, viewport-width, rtt, downlink, ect".
+//
+// echo_client_hints_received.py sets the response headers depending on the set
+// of client hints it receives in the request headers.
+
+promise_test(t => {
+  return fetch(get_host_info()["HTTPS_ORIGIN"] + "/client-hints/echo_client_hints_received.py").then(r => {
+    assert_equals(r.status, 200)
+    // Verify that the browser includes client hints in the headers for a
+    // same-origin fetch which not specifically excluded via Feature-Policy.
+    assert_true(r.headers.has("device-memory-received"), "device-memory-received");
+    assert_true(r.headers.has("dpr-received"), "dpr-received");
+    assert_false(r.headers.has("lang-received"), "lang-received");
+    assert_true(r.headers.has("viewport-width-received"), "viewport-width-received");
+
+    assert_true(r.headers.has("rtt-received"), "rtt-received");
+    var rtt = parseInt(r.headers.get("rtt-received"));
+    assert_greater_than_equal(rtt, 0);
+    assert_less_than_equal(rtt, 3000);
+    assert_equals(rtt % 50, 0, 'rtt must be a multiple of 50 msec');
+
+    assert_true(r.headers.has("downlink-received"), "downlink-received");
+    var downlinkKbps  = r.headers.get("downlink-received") * 1000;
+    assert_greater_than_equal(downlinkKbps, 0);
+    assert_less_than_equal(downlinkKbps, 10000);
+
+    assert_in_array(r.headers.get("ect-received"), ["slow-2g", "2g",
+          "3g", "4g"], 'ect-received is unexpected');
+  });
+}, "Accept-CH header test");
+
+promise_test(t => {
+  return fetch(get_host_info()["HTTPS_REMOTE_ORIGIN"] + "/client-hints/echo_client_hints_received.py").then(r => {
+    assert_equals(r.status, 200)
+    // Verify that the browser includes client hints in the headers for a
+    // cross-origin fetch which are specifically requested via Feature-Policy.
+    // The 3 hints below are added regardless of Feature Policy, due to legacy constraints
+    assert_true(r.headers.has("device-memory-received"), "device-memory-received");
+    assert_true(r.headers.has("dpr-received"), "dpr-received");
+    assert_true(r.headers.has("viewport-width-received"), "viewport-width-received");
+    assert_false(r.headers.has("lang-received"), "lang-received");
+    assert_false(r.headers.has("rtt-received"), "rtt-received");
+    assert_false(r.headers.has("downlink-received"), "downlink-received");
+    assert_false(r.headers.has("ect-received"), "ect-received");
+  });
+}, "Cross-Origin Accept-CH header test");
+
+
+
+</script>
+
+</body>
+</html>

third_party/blink/web_tests/wpt_internal/client-hints/accept_ch_feature_policy_allow_legacy_hints.tentative.sub.https.html.headers

+Accept-CH: device-memory, dpr, viewport-width, rtt, downlink, ect, lang
+Feature-Policy: ch-device-memory *; ch-dpr 'none'; ch-viewport-width 'self'; ch-lang 'none'