Skip cookie check for origins when sending client hints

Skip cookie check for origins when sending client hints.
The cookie check was initially added when there was plan to
allow 3rd party origins to request client hints.
Since client hints are now
restricted to only first party origins, and client hints are
already disabled for origins that do not have JavaScript permissions,
this cookie check is considered redundant. Note that cookie check is not
required by the fetch or client hints spec either.

Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I080fa5257abba15724559a39b4b73ac4f2e12e3e
Bug: 891370
Reviewed-on: https://chromium-review.googlesource.com/c/1253215Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596243}

chrome/browser/client_hints/client_hints.cc

@@ -17,7 +17,6 @@
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/client_hints/client_hints.h"
-#include "components/content_settings/core/browser/cookie_settings.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/content_settings/core/common/content_settings_utils.h"
 #include "content/public/browser/browser_context.h"
@@ -384,23 +383,4 @@ GetAdditionalNavigationRequestClientHintsHeaders(
   return additional_headers;
 }
 
-void RequestBeginning(
-    net::URLRequest* request,
-    scoped_refptr<content_settings::CookieSettings> cookie_settings) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-
-  if (!cookie_settings)
-    return;
-
-  if (cookie_settings->IsCookieAccessAllowed(request->url(),
-                                             request->site_for_cookies())) {
-    return;
-  }
-
-  // If |primary_url| is disallowed from storing cookies, then client hints are
-  // not attached to the requests sent to |primary_url|.
-  for (size_t i = 0; i < blink::kClientHintsHeaderMappingCount; ++i)
-    request->RemoveRequestHeaderByName(blink::kClientHintsHeaderMapping[i]);
-}
-
 }  // namespace client_hints

chrome/browser/client_hints/client_hints.h

@@ -16,13 +16,8 @@ namespace content {
 class BrowserContext;
 }
 
-namespace content_settings {
-class CookieSettings;
-}
-
 namespace net {
 class HttpRequestHeaders;
-class URLRequest;
 }
 
 namespace client_hints {
@@ -49,11 +44,6 @@ GetAdditionalNavigationRequestClientHintsHeaders(
     content::BrowserContext* context,
     const GURL& url);
 
-// Called before |request| goes on the network.
-void RequestBeginning(
-    net::URLRequest* request,
-    scoped_refptr<content_settings::CookieSettings> cookie_settings);
-
 }  // namespace client_hints
 
 #endif  // CHROME_BROWSER_CLIENT_HINTS_CLIENT_HINTS_H_

chrome/browser/client_hints/client_hints_browsertest.cc

@@ -1046,13 +1046,10 @@ IN_PROC_BROWSER_TEST_P(ClientHintsBrowserTest,
   EXPECT_EQ(12u, count_client_hints_headers_seen());
 }
 
-// Ensure that when cookies are blocked, client hint preferences are not
+// Ensure that even when cookies are blocked, client hint preferences are
 // persisted.
 IN_PROC_BROWSER_TEST_P(ClientHintsBrowserTest,
-                       ClientHintsLifetimeNotPersistedCookiesBlocked) {
-  const GURL gurl_without = GetParam()
-                                ? http_equiv_accept_ch_without_lifetime_url()
-                                : accept_ch_without_lifetime_url();
+                       ClientHintsLifetimePersistedCookiesBlocked) {
   const GURL gurl_with = GetParam() ? http_equiv_accept_ch_with_lifetime()
                                     : accept_ch_with_lifetime_url();
 
@@ -1063,33 +1060,22 @@ IN_PROC_BROWSER_TEST_P(ClientHintsBrowserTest,
 
   // Block cookies.
   HostContentSettingsMapFactory::GetForProfile(browser()->profile())
-      ->SetContentSettingDefaultScope(gurl_without, GURL(),
+      ->SetContentSettingDefaultScope(gurl_with, GURL(),
                                       CONTENT_SETTINGS_TYPE_COOKIES,
                                       std::string(), CONTENT_SETTING_BLOCK);
 
-  // Fetching |gurl_with| should not persist the request for client hints since
-  // cookies are blocked.
-  ui_test_utils::NavigateToURL(browser(), gurl_with);
-  histogram_tester.ExpectTotalCount("ClientHints.UpdateEventCount", 0);
-  HostContentSettingsMapFactory::GetForProfile(browser()->profile())
-      ->GetSettingsForOneType(CONTENT_SETTINGS_TYPE_CLIENT_HINTS, std::string(),
-                              &host_settings);
-  EXPECT_EQ(0u, host_settings.size());
-  VerifyContentSettingsNotNotified();
-
-  // Allow cookies.
-  cookie_settings_->SetCookieSetting(gurl_without, CONTENT_SETTING_ALLOW);
-  // Fetching |gurl_with| should persist the request for client hints since
-  // cookies are allowed.
+  // Fetching |gurl_with| should persist the request for client hints.
   ui_test_utils::NavigateToURL(browser(), gurl_with);
+  histogram_tester.ExpectTotalCount("ClientHints.UpdateEventCount", 1);
   HostContentSettingsMapFactory::GetForProfile(browser()->profile())
       ->GetSettingsForOneType(CONTENT_SETTINGS_TYPE_CLIENT_HINTS, std::string(),
                               &host_settings);
   EXPECT_EQ(1u, host_settings.size());
+  VerifyContentSettingsNotNotified();
 }
 
 IN_PROC_BROWSER_TEST_P(ClientHintsBrowserTest,
-                       ClientHintsLifetimeNotAttachedCookiesBlocked) {
+                       ClientHintsLifetimeAttachedCookiesBlocked) {
   const GURL gurl_with = GetParam() ? http_equiv_accept_ch_with_lifetime()
                                     : accept_ch_with_lifetime_url();
   const GURL gurl_without = GetParam()
@@ -1121,23 +1107,12 @@ IN_PROC_BROWSER_TEST_P(ClientHintsBrowserTest,
                               &host_settings);
   EXPECT_EQ(1u, host_settings.size());
 
-  // Block the cookies: Client hints should not be attached.
+  // Block the cookies: Client hints should be attached.
   HostContentSettingsMapFactory::GetForProfile(browser()->profile())
       ->SetContentSettingDefaultScope(gurl_without, GURL(),
                                       CONTENT_SETTINGS_TYPE_COOKIES,
                                       std::string(), CONTENT_SETTING_BLOCK);
 
-  ui_test_utils::NavigateToURL(browser(),
-                               without_accept_ch_without_lifetime_url());
-  EXPECT_EQ(0u, count_client_hints_headers_seen());
-  VerifyContentSettingsNotNotified();
-
-  // Allow the cookies: Client hints should now be attached.
-  HostContentSettingsMapFactory::GetForProfile(browser()->profile())
-      ->SetContentSettingDefaultScope(gurl_without, GURL(),
-                                      CONTENT_SETTINGS_TYPE_COOKIES,
-                                      std::string(), CONTENT_SETTING_ALLOW);
-
   SetClientHintExpectationsOnMainFrame(true);
   SetClientHintExpectationsOnSubresources(true);
   ui_test_utils::NavigateToURL(browser(),
@@ -1326,10 +1301,10 @@ IN_PROC_BROWSER_TEST_P(ClientHintsBrowserTest,
       ->ClearSettingsForOneType(CONTENT_SETTINGS_TYPE_JAVASCRIPT);
 }
 
-// Ensure that when the cookies is blocked, client hints are not attached to the
+// Ensure that when the cookies is blocked, client hints are attached to the
 // request headers.
 IN_PROC_BROWSER_TEST_P(ClientHintsBrowserTest,
-                       ClientHintsNoLifetimeCookiesNotAllowed) {
+                       ClientHintsLifetimeCookiesNotAllowed) {
   const GURL gurl = GetParam()
                         ? http_equiv_accept_ch_without_lifetime_img_localhost()
                         : accept_ch_without_lifetime_img_localhost();
@@ -1351,34 +1326,10 @@ IN_PROC_BROWSER_TEST_P(ClientHintsBrowserTest,
                                       std::string(), CONTENT_SETTING_BLOCK);
   base::RunLoop().RunUntilIdle();
 
-  ui_test_utils::NavigateToURL(browser(), gurl);
-  EXPECT_EQ(0u, count_client_hints_headers_seen());
-  // Client hints are not attached to third party subresources even though
-  // cookies are allowed only for the first party origin.
-  EXPECT_EQ(0u, third_party_client_hints_count_seen());
-  VerifyContentSettingsNotNotified();
-
-  // Allow cookies.
-  cookie_settings_->SetCookieSetting(gurl, CONTENT_SETTING_ALLOW);
-  base::RunLoop().RunUntilIdle();
-
   SetClientHintExpectationsOnSubresources(true);
   ui_test_utils::NavigateToURL(browser(), gurl);
   EXPECT_EQ(6u, count_client_hints_headers_seen());
-  EXPECT_EQ(2u, third_party_request_count_seen());
-  EXPECT_EQ(0u, third_party_client_hints_count_seen());
-
-  // Block cookies again.
-  SetClientHintExpectationsOnSubresources(false);
-  HostContentSettingsMapFactory::GetForProfile(browser()->profile())
-      ->SetContentSettingDefaultScope(gurl, GURL(),
-                                      CONTENT_SETTINGS_TYPE_COOKIES,
-                                      std::string(), CONTENT_SETTING_BLOCK);
-  base::RunLoop().RunUntilIdle();
-
-  ui_test_utils::NavigateToURL(browser(), gurl);
-  EXPECT_EQ(6u, count_client_hints_headers_seen());
-  EXPECT_EQ(3u, third_party_request_count_seen());
+  EXPECT_EQ(1u, third_party_request_count_seen());
   EXPECT_EQ(0u, third_party_client_hints_count_seen());
 
   // Clear settings.

chrome/browser/client_hints/client_hints_observer.cc

@@ -8,10 +8,8 @@
 
 #include "base/metrics/histogram_macros.h"
 #include "base/values.h"
-#include "chrome/browser/content_settings/cookie_settings_factory.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/profiles/profile.h"
-#include "components/content_settings/core/browser/cookie_settings.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/content_settings/core/common/content_settings_pattern.h"
 #include "components/content_settings/core/common/content_settings_types.h"
@@ -65,14 +63,6 @@ void ClientHintsObserver::PersistClientHints(
   content::BrowserContext* browser_context = rph->GetBrowserContext();
   Profile* profile = Profile::FromBrowserContext(browser_context);
 
-  scoped_refptr<content_settings::CookieSettings> cookie_settings =
-      CookieSettingsFactory::GetForProfile(profile);
-  if (!cookie_settings->IsCookieAccessAllowed(primary_url, primary_url)) {
-    // If |primary_url| is disallowed from storing cookies, then |primary_url|
-    // is also prevented from storing client hints.
-    return;
-  }
-
   HostContentSettingsMap* map =
       HostContentSettingsMapFactory::GetForProfile(profile);
 

chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc

@@ -19,7 +19,6 @@
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
-#include "chrome/browser/client_hints/client_hints.h"
 #include "chrome/browser/component_updater/component_updater_resource_throttle.h"
 #include "chrome/browser/download/download_request_limiter.h"
 #include "chrome/browser/download/download_resource_throttle.h"
@@ -344,7 +343,6 @@ void ChromeResourceDispatcherHostDelegate::RequestBeginning(
   if (safe_browsing_.get())
     safe_browsing_->OnResourceRequest(request);
   ProfileIOData* io_data = ProfileIOData::FromResourceContext(resource_context);
-  client_hints::RequestBeginning(request, io_data->GetCookieSettings());
 
 #if BUILDFLAG(ENABLE_OFFLINE_PAGES) || BUILDFLAG(ENABLE_NACL)
   const ResourceRequestInfo* info = ResourceRequestInfo::ForRequest(request);

testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

@@ -61,12 +61,6 @@
 -NetInternalsTest.netInternalsTimelineViewZoomOut
 -NetInternalsTest.netInternalsTourTabs
 
-# https://crbug.com/853256
--ClientHintsBrowserTest.ClientHintsLifetimeNotAttachedCookiesBlocked/0
--ClientHintsBrowserTest.ClientHintsLifetimeNotAttachedCookiesBlocked/1
--ClientHintsBrowserTest.ClientHintsNoLifetimeCookiesNotAllowed/0
--ClientHintsBrowserTest.ClientHintsNoLifetimeCookiesNotAllowed/1
-
 # https://crbug.com/816684 Track Page Load Metrics.
 -PageLoadMetricsBrowserTest.LoadingMetricsFailed