Check Accept-CH filters before adding a client hint to the request

It was discovered that, when applying Critical-CH client hints, the critical client hints would only be filtered if they did not appear in the Accept-CH response header. However, we apply some filters to the Accept-CH response header before persisting the client hints in the Accept-CH cache. The filters currently are just features guarded by a feature flag, but in the future, we will also want Accept-CH values to be gated by the presence of an Origin Trial (see crbug.com/1226193 for an example). In crrev.com/c/2983659, the same Accept-CH filter was applied on Critical-CH as is applied when persisting client hints in the Accept-CH cache. This was intended as a temporary solution to fix the bug. However, it does not solve for the fact that the filtering logic on Accept-CH values happen in two places (when parsing and persisting the Accept-CH header and when processing the Critical-CH values). This CL unifies the logic of filtering Accept-CH values in one place. Instead of filtering Accept-CH values when persisting the hints in the Accept-CH cache, we persist the Accept-CH hints as given to us in the response header. Only when adding the client hints to the request header do we check for any Accept-CH values that must be filtered before adding to the request header. We use the same logic to also determine if the value(s) in the Critical-CH header should cause a resend of the request with the critical client hints in the request header. Bug: 1231630 Change-Id: Iede700094bb2d6c1d10b1a601d52e2860dd9ae08 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3048882 Commit-Queue: Ali Beyad <abeyad@chromium.org> Reviewed-by: Matt Falkenhagen <falken@chromium.org> Reviewed-by: Bo <boliu@chromium.org> Reviewed-by: Aaron Tagliaboschi <aarontag@chromium.org> Reviewed-by: Justin Novosad <junov@chromium.org> Cr-Commit-Position: refs/heads/master@{#905962}
FairyWorld · Jul 28, 2021 · a6b0fb6 · a6b0fb6
1 parent 3b14ccb
commit a6b0fb6
Show file tree

Hide file tree

Showing 51 changed files with 427 additions and 287 deletions.
diff --git a/android_webview/lib/BUILD.gn b/android_webview/lib/BUILD.gn
@@ -44,6 +44,7 @@ source_set("lib") {
     "//gpu/ipc:gl_in_process_context",
     "//media",
     "//media:media_buildflags",
+    "//third_party/blink/public/common:headers",
     "//ui/base",
     "//ui/events:gesture_detection",
   ]

diff --git a/android_webview/lib/DEPS b/android_webview/lib/DEPS
@@ -11,6 +11,7 @@ include_rules = [
   "+gin/v8_initializer.h",
   "+media/base/media_switches.h",  # For media command line switches.
   "+mojo/core/embedder/embedder.h",
+  "+third_party/blink/public/common/features.h",
   "+ui/events/gesture_detection",
   "+ui/gl",
 ]

diff --git a/android_webview/lib/aw_main_delegate.cc b/android_webview/lib/aw_main_delegate.cc
@@ -63,6 +63,7 @@
 #include "media/base/media_switches.h"
 #include "media/media_buildflags.h"
 #include "services/network/public/cpp/features.h"
+#include "third_party/blink/public/common/features.h"
 #include "ui/base/ui_base_paths.h"
 #include "ui/base/ui_base_switches.h"
 #include "ui/events/gesture_detection/gesture_configuration.h"
@@ -266,7 +267,7 @@ bool AwMainDelegate::BasicStartupComplete(int* exit_code) {
 
     // TODO(crbug.com/921655): Add support for User Agent Client hints on
     // WebView.
-    features.DisableIfNotSet(::features::kUserAgentClientHint);
+    features.DisableIfNotSet(blink::features::kUserAgentClientHint);
 
     // Disabled until viz scheduling can be improved.
     features.DisableIfNotSet(::features::kUseSurfaceLayerForVideoDefault);

diff --git a/chrome/browser/BUILD.gn b/chrome/browser/BUILD.gn
@@ -2318,6 +2318,7 @@ static_library("browser") {
     "//third_party/blink/public:scaled_resources",
     "//third_party/blink/public/common",
     "//third_party/blink/public/common:buildflags",
+    "//third_party/blink/public/common:headers",
     "//third_party/icu",
     "//third_party/leveldatabase",
     "//third_party/libaddressinput",

diff --git a/chrome/browser/client_hints/DEPS b/chrome/browser/client_hints/DEPS
@@ -4,5 +4,4 @@ include_rules = [
   # header-only types, and some selected common code.
   "-third_party/blink",
   "+third_party/blink/public/common",
-  "+third_party/blink/public/platform/web_client_hints_type.h",
 ]
diff --git a/chrome/browser/client_hints/client_hints_browsertest.cc b/chrome/browser/client_hints/client_hints_browsertest.cc
@@ -52,13 +52,17 @@
 #include "services/network/public/cpp/features.h"
 #include "services/network/public/cpp/network_switches.h"
 #include "services/network/public/mojom/web_client_hints_types.mojom-shared.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "third_party/blink/public/common/client_hints/client_hints.h"
 #include "third_party/blink/public/common/web_preferences/web_preferences.h"
 
 namespace {
 
-const unsigned expected_client_hints_number = 13u;
-const int32_t uma_histogram_max_value = 1471228928;
+using ::testing::Eq;
+using ::testing::Optional;
+
+constexpr unsigned expected_client_hints_number = 13u;
+constexpr int32_t uma_histogram_max_value = 1471228928;
 
 // An interceptor that records count of fetches and client hint headers for
 // requests to https://foo.com/non-existing-image.jpg.
@@ -2284,6 +2288,117 @@ IN_PROC_BROWSER_TEST_P(ClientHintsBrowserTest, UseCounter) {
   web_feature_waiter->Wait();
 }
 
+class CriticalClientHintsBrowserTest : public InProcessBrowserTest {
+ public:
+  CriticalClientHintsBrowserTest()
+      : https_server_(net::EmbeddedTestServer::TYPE_HTTPS) {
+    https_server_.ServeFilesFromSourceDirectory(
+        "chrome/test/data/client_hints");
+    https_server_.RegisterRequestMonitor(base::BindRepeating(
+        &CriticalClientHintsBrowserTest::MonitorResourceRequest,
+        base::Unretained(this)));
+    EXPECT_TRUE(https_server_.Start());
+  }
+
+  void SetUp() override {
+    std::unique_ptr<base::FeatureList> feature_list =
+        std::make_unique<base::FeatureList>();
+    // Don't include LangClientHintHeader in the enabled features; we will
+    // verify that the Sec-CH-Lang header is not included.
+    feature_list->InitializeFromCommandLine(
+        "UserAgentClientHint,CriticalClientHint,AcceptCHFrame,"
+        "PrefersColorSchemeClientHintHeader",
+        "");
+    scoped_feature_list_.InitWithFeatureList(std::move(feature_list));
+    InProcessBrowserTest::SetUp();
+  }
+
+  void SetUpOnMainThread() override {
+    host_resolver()->AddRule("*", "127.0.0.1");
+  }
+
+  GURL critical_ch_ua_full_version_url() const {
+    return https_server_.GetURL("/critical_ch_ua_full_version.html");
+  }
+
+  GURL critical_ch_lang_url() const {
+    return https_server_.GetURL("/critical_ch_lang.html");
+  }
+
+  const absl::optional<std::string>& observed_ch_ua_full_version() {
+    base::AutoLock lock(ch_ua_full_version_lock_);
+    return ch_ua_full_version_;
+  }
+
+  const absl::optional<std::string>& observed_ch_lang() {
+    base::AutoLock lock(ch_lang_lock_);
+    return ch_lang_;
+  }
+
+  void MonitorResourceRequest(const net::test_server::HttpRequest& request) {
+    if (request.headers.find("sec-ch-ua-full-version") !=
+        request.headers.end()) {
+      SetChUaFullVersion(request.headers.at("sec-ch-ua-full-version"));
+    }
+    if (request.headers.find("sec-ch-lang") != request.headers.end()) {
+      SetChLang(request.headers.at("sec-ch-lang"));
+    }
+  }
+
+ private:
+  void SetChUaFullVersion(const std::string& ch_ua_full_version) {
+    base::AutoLock lock(ch_ua_full_version_lock_);
+    ch_ua_full_version_ = ch_ua_full_version;
+  }
+
+  void SetChLang(const std::string& ch_lang) {
+    base::AutoLock lock(ch_lang_lock_);
+    ch_lang_ = ch_lang;
+  }
+
+  base::test::ScopedFeatureList scoped_feature_list_;
+  net::EmbeddedTestServer https_server_;
+  base::Lock ch_ua_full_version_lock_;
+  absl::optional<std::string> ch_ua_full_version_
+      GUARDED_BY(ch_ua_full_version_lock_);
+  base::Lock ch_lang_lock_;
+  absl::optional<std::string> ch_lang_ GUARDED_BY(ch_lang_lock_);
+};
+
+// Verify that setting Critical-CH in the response header causes the request to
+// be resent with the client hint included.
+IN_PROC_BROWSER_TEST_F(CriticalClientHintsBrowserTest,
+                       CriticalClientHintInRequestHeader) {
+  blink::UserAgentMetadata ua = embedder_support::GetUserAgentMetadata();
+  // On the first navigation request, the client hints in the Critical-CH
+  // should be set on the request header.
+  ui_test_utils::NavigateToURL(browser(), critical_ch_ua_full_version_url());
+  const std::string expected_ch_ua_full_version = "\"" + ua.full_version + "\"";
+  EXPECT_THAT(observed_ch_ua_full_version(),
+              Optional(Eq(expected_ch_ua_full_version)));
+  EXPECT_EQ(observed_ch_lang(), absl::nullopt);
+}
+
+// Verify that setting Critical-CH in the response header with a client hint
+// that is filtered out of Accept-CH causes the request to *not* be resent and
+// the critical client hint is not included.
+//
+// NB: When the LangClientHintHeader feature is removed, this test needs to be
+// updated.
+IN_PROC_BROWSER_TEST_F(
+    CriticalClientHintsBrowserTest,
+    CriticalClientHintFilteredOutOfAcceptChNotInRequestHeader) {
+  // On the first navigation request, the client hints in the Critical-CH
+  // should be set on the request header, but in this case, the
+  // LangClientHintHeader is not enabled, so the critical client hint won't be
+  // set in the request header.
+  ui_test_utils::NavigateToURL(browser(), critical_ch_lang_url());
+  EXPECT_EQ(observed_ch_lang(), absl::nullopt);
+  // The request should not have been resent, so ch-ua-full-version should also
+  // not be present.
+  EXPECT_EQ(observed_ch_ua_full_version(), absl::nullopt);
+}
+
 class ClientHintsBrowserTestWithEmulatedMedia
     : public DevToolsProtocolTestBase {
  public:

diff --git a/chrome/test/data/client_hints/critical_ch_lang.html b/chrome/test/data/client_hints/critical_ch_lang.html
@@ -0,0 +1,6 @@
+<html>
+<link rel="icon" href="data:;base64,=">
+<head></head>
+Empty file which uses link-rel to disable favicon fetches. The corresponding
+.mock-http-headers sets client hints.
+</html>
diff --git a/chrome/test/data/client_hints/critical_ch_lang.html.mock-http-headers b/chrome/test/data/client_hints/critical_ch_lang.html.mock-http-headers
@@ -0,0 +1,4 @@
+HTTP/1.1 200 OK
+Accept-CH: lang,sec-ch-ua-full-version
+Accept-CH-Lifetime: 3600
+Critical-CH: lang
diff --git a/chrome/test/data/client_hints/critical_ch_ua_full_version.html b/chrome/test/data/client_hints/critical_ch_ua_full_version.html
@@ -0,0 +1,6 @@
+<html>
+<link rel="icon" href="data:;base64,=">
+<head></head>
+Empty file which uses link-rel to disable favicon fetches. The corresponding
+.mock-http-headers sets client hints.
+</html>
diff --git a/chrome/test/data/client_hints/critical_ch_ua_full_version.html.mock-http-headers b/chrome/test/data/client_hints/critical_ch_ua_full_version.html.mock-http-headers
@@ -0,0 +1,4 @@
+HTTP/1.1 200 OK
+Accept-CH: lang,sec-ch-ua-full-version
+Accept-CH-Lifetime: 3600
+Critical-CH: sec-ch-ua-full-version
diff --git a/components/client_hints/browser/client_hints.cc b/components/client_hints/browser/client_hints.cc
@@ -41,7 +41,7 @@ network::NetworkQualityTracker* ClientHints::GetNetworkQualityTracker() {
 
 void ClientHints::GetAllowedClientHintsFromSource(
     const GURL& url,
-    blink::WebEnabledClientHints* client_hints) {
+    blink::EnabledClientHints* client_hints) {
   ContentSettingsForOneType client_hints_rules;
   settings_map_->GetSettingsForOneType(ContentSettingsType::CLIENT_HINTS,
                                        &client_hints_rules);

diff --git a/components/client_hints/browser/client_hints.h b/components/client_hints/browser/client_hints.h
@@ -31,7 +31,7 @@ class ClientHints : public KeyedService,
 
   void GetAllowedClientHintsFromSource(
       const GURL& url,
-      blink::WebEnabledClientHints* client_hints) override;
+      blink::EnabledClientHints* client_hints) override;
 
   bool IsJavaScriptAllowed(const GURL& url) override;
 

diff --git a/components/client_hints/common/BUILD.gn b/components/client_hints/common/BUILD.gn
@@ -9,8 +9,7 @@ source_set("common") {
   ]
   deps = [
     "//components/content_settings/core/common",
-    "//third_party/blink/public:blink_headers",
-    "//third_party/blink/public/common",
+    "//third_party/blink/public/common:headers",
     "//url",
   ]
 }
diff --git a/components/client_hints/common/DEPS b/components/client_hints/common/DEPS
@@ -1,4 +1,4 @@
 include_rules = [
   "+services/network/public/cpp/is_potentially_trustworthy.h",
-  "+third_party/blink/public/platform",
+  "+third_party/blink/public/common/client_hints/enabled_client_hints.h",
 ]
diff --git a/components/client_hints/common/client_hints.cc b/components/client_hints/common/client_hints.cc
@@ -5,15 +5,15 @@
 #include "components/client_hints/common/client_hints.h"
 
 #include "services/network/public/cpp/is_potentially_trustworthy.h"
-#include "third_party/blink/public/platform/web_client_hints_type.h"
+#include "third_party/blink/public/common/client_hints/enabled_client_hints.h"
 #include "url/gurl.h"
 
 namespace client_hints {
 
 void GetAllowedClientHintsFromSource(
     const GURL& url,
     const ContentSettingsForOneType& client_hints_rules,
-    blink::WebEnabledClientHints* client_hints) {
+    blink::EnabledClientHints* client_hints) {
   if (client_hints_rules.empty())
     return;
 

diff --git a/components/client_hints/common/client_hints.h b/components/client_hints/common/client_hints.h
@@ -10,7 +10,7 @@
 class GURL;
 
 namespace blink {
-struct WebEnabledClientHints;
+class EnabledClientHints;
 }
 
 namespace client_hints {
@@ -21,7 +21,7 @@ namespace client_hints {
 void GetAllowedClientHintsFromSource(
     const GURL& url,
     const ContentSettingsForOneType& client_hints_rules,
-    blink::WebEnabledClientHints* client_hints);
+    blink::EnabledClientHints* client_hints);
 
 }  // namespace client_hints
 

diff --git a/content/browser/DEPS b/content/browser/DEPS
@@ -103,9 +103,7 @@ include_rules = [
   "+third_party/blink/public/common",
   "+third_party/blink/public/mojom",
   "+third_party/blink/public/platform/resource_request_blocked_reason.h",
-  "+third_party/blink/public/platform/web_client_hints_type.h",
   "+third_party/blink/public/platform/web_content_security_policy.h",
-  "+third_party/blink/public/common/page/drag_operation.h",
   "+third_party/blink/public/platform/web_fullscreen_video_status.h",
   "+third_party/blink/public/platform/web_text_input_type.h",
   "+third_party/blink/public/platform/mac/web_scrollbar_theme.h",
@@ -115,8 +113,6 @@ include_rules = [
   "+third_party/blink/public/strings/grit/blink_strings.h",
   "+third_party/blink/public/web/web_ax_enums.h",
   "+third_party/blink/public/web/web_console_message.h",
-  "+third_party/blink/public/common/context_menu_data/context_menu_data.h",
-  "+third_party/blink/public/common/widget/device_emulation_params.h",
   "+third_party/blink/public/web/web_drag_status.h",
   "+third_party/blink/public/web/web_serialized_script_value_version.h",
   "+third_party/blink/public/mojom/frame/tree_scope_type.mojom.h",

diff --git a/content/browser/client_hints/client_hints.cc b/content/browser/client_hints/client_hints.cc
@@ -35,11 +35,12 @@
 #include "services/network/public/cpp/network_quality_tracker.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
 #include "third_party/blink/public/common/client_hints/client_hints.h"
+#include "third_party/blink/public/common/client_hints/enabled_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/common/page/page_zoom.h"
 #include "third_party/blink/public/common/permissions_policy/permissions_policy.h"
 #include "third_party/blink/public/common/user_agent/user_agent_metadata.h"
-#include "third_party/blink/public/platform/web_client_hints_type.h"
 #include "ui/display/display.h"
 #include "ui/display/screen.h"
 
@@ -377,17 +378,8 @@ bool IsValidURLForClientHints(const GURL& url) {
   return true;
 }
 
-bool LangClientHintEnabled() {
-  return base::FeatureList::IsEnabled(features::kLangClientHintHeader);
-}
-
-bool PrefersColorSchemeClientHintEnabled() {
-  return base::FeatureList::IsEnabled(
-      features::kPrefersColorSchemeClientHintHeader);
-}
-
 bool UserAgentClientHintEnabled() {
-  return base::FeatureList::IsEnabled(features::kUserAgentClientHint);
+  return base::FeatureList::IsEnabled(blink::features::kUserAgentClientHint);
 }
 
 void AddUAHeader(net::HttpRequestHeaders* headers,
@@ -435,7 +427,7 @@ struct ClientHintsExtendedData {
     delegate->GetAllowedClientHintsFromSource(main_frame_url, &hints);
   }
 
-  blink::WebEnabledClientHints hints;
+  blink::EnabledClientHints hints;
   url::Origin resource_origin;
   bool is_main_frame = false;
   GURL main_frame_url;
@@ -782,13 +774,6 @@ ParseAndPersistAcceptCHForNavigation(
   if (!frame_tree_node->IsMainFrame())
     return absl::nullopt;
 
-  absl::optional<std::vector<network::mojom::WebClientHintsType>> parsed =
-      blink::FilterAcceptCH(headers->accept_ch.value(), LangClientHintEnabled(),
-                            UserAgentClientHintEnabled(),
-                            PrefersColorSchemeClientHintEnabled());
-  if (!parsed.has_value())
-    return absl::nullopt;
-
   base::TimeDelta persist_duration;
   if (IsPermissionsPolicyForClientHintsEnabled()) {
     // JSON cannot store "non-finite" values (i.e. NaN or infinite) so
@@ -799,13 +784,13 @@ ParseAndPersistAcceptCHForNavigation(
   } else {
     persist_duration = headers->accept_ch_lifetime;
     if (persist_duration.is_zero())
-      return parsed;
+      return headers->accept_ch;
   }
 
-  delegate->PersistClientHints(url::Origin::Create(url), parsed.value(),
-                               persist_duration);
+  delegate->PersistClientHints(url::Origin::Create(url),
+                               headers->accept_ch.value(), persist_duration);
 
-  return parsed;
+  return headers->accept_ch;
 }
 
 CONTENT_EXPORT std::vector<::network::mojom::WebClientHintsType>