Do not report otherwise excluded cookies as "blocked"

Cookies that are excluded for reasons (e.g. mismatching path or domain)
other than a user's cookie blocking settings (i.e. content settings)
should not be reported as "blocked".

Bug: 1104451
Change-Id: I8bf6dbd3367fcb5f01655dac0e7c20ee52b34c69
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2296009Reviewed-by: Martin Šrámek <msramek@chromium.org>
Reviewed-by: Maksim Orlovich <morlovich@chromium.org>
Reviewed-by: Aaron Colwell <acolwell@chromium.org>
Commit-Queue: Lily Chen <chlily@chromium.org>
Cr-Commit-Position: refs/heads/master@{#790002}

chrome/browser/content_settings/content_settings_browsertest.cc

@@ -51,6 +51,7 @@
 #include "content/public/test/test_utils.h"
 #include "net/cookies/canonical_cookie_test_helpers.h"
 #include "net/dns/mock_host_resolver.h"
+#include "net/test/embedded_test_server/default_handlers.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "net/test/embedded_test_server/http_request.h"
 #include "net/test/embedded_test_server/http_response.h"
@@ -162,13 +163,17 @@ class CookieSettingsTest
   CookieMode WriteMode() const { return GetParam().second; }
 
   void SetUpOnMainThread() override {
+    RegisterDefaultHandlers(embedded_test_server());
     embedded_test_server()->RegisterRequestMonitor(base::BindRepeating(
         &CookieSettingsTest::MonitorRequest, base::Unretained(this)));
+    RegisterDefaultHandlers(&https_server_);
     https_server_.RegisterRequestMonitor(base::BindRepeating(
         &CookieSettingsTest::MonitorRequest, base::Unretained(this)));
     ASSERT_TRUE(embedded_test_server()->Start());
     ASSERT_TRUE(https_server_.Start());
 
+    host_resolver()->AddRule("*", "127.0.0.1");
+
     // CookieStore API is for https only.
     if (ReadMode() == CookieMode::kCookieStoreJS ||
         WriteMode() == CookieMode::kCookieStoreJS)
@@ -252,11 +257,44 @@ class CookieSettingsTest
     return secure_scheme_ ? embedded_test_server() : &https_server_;
   }
 
- private:
   net::EmbeddedTestServer* GetServer() {
     return secure_scheme_ ? &https_server_ : embedded_test_server();
   }
 
+  // Read a cookie by fetching a url and checking what Cookie header (if any) it
+  // saw.
+  std::string HttpReadCookieWithURL(Browser* browser, const GURL& url) {
+    {
+      base::AutoLock auto_lock(cookies_seen_lock_);
+      cookies_seen_.clear();
+    }
+
+    auto* network_context =
+        content::BrowserContext::GetDefaultStoragePartition(browser->profile())
+            ->GetNetworkContext();
+    content::LoadBasicRequest(network_context, url);
+
+    {
+      base::AutoLock auto_lock(cookies_seen_lock_);
+      return cookies_seen_[url];
+    }
+  }
+
+  // Set a cookie by visiting a page that has a Set-Cookie header.
+  void HttpWriteCookieWithURL(Browser* browser, const GURL& url) {
+    auto* frame =
+        browser->tab_strip_model()->GetActiveWebContents()->GetMainFrame();
+    auto* network_context =
+        content::BrowserContext::GetDefaultStoragePartition(browser->profile())
+            ->GetNetworkContext();
+    // Need process & frame ID here for the accessed/blocked cookies lists to be
+    // updated properly.
+    content::LoadBasicRequest(network_context, url,
+                              frame->GetProcess()->GetID(),
+                              frame->GetRoutingID());
+  }
+
+ private:
   // Read a cookie via JavaScript.
   std::string JSReadCookie(Browser* browser) {
     std::string cookies;
@@ -282,25 +320,14 @@ class CookieSettingsTest
         .ExtractString();
   }
 
-  // Read a cookie by fetching a url on the appropriate test server and checking
-  // what Cookie header (if any) it saw.
+  // Read a cookie by fetching the page url (which we should have just navigated
+  // to) and checking what Cookie header (if any) it saw.
   std::string HttpReadCookie(Browser* browser) {
-    {
-      base::AutoLock auto_lock(cookies_seen_lock_);
-      cookies_seen_.clear();
-    }
-
-    auto* network_context =
-        content::BrowserContext::GetDefaultStoragePartition(browser->profile())
-            ->GetNetworkContext();
-    content::LoadBasicRequest(network_context, browser->tab_strip_model()
-                                                   ->GetActiveWebContents()
-                                                   ->GetLastCommittedURL());
-
-    {
-      base::AutoLock auto_lock(cookies_seen_lock_);
-      return cookies_seen_[GetPageURL()];
-    }
+    GURL url = browser->tab_strip_model()
+                   ->GetActiveWebContents()
+                   ->GetLastCommittedURL();
+    EXPECT_EQ(GetPageURL(), url);
+    return HttpReadCookieWithURL(browser, url);
   }
 
   // Set a cookie with JavaScript.
@@ -330,16 +357,7 @@ class CookieSettingsTest
 
   // Set a cookie by visiting a page that has a Set-Cookie header.
   void HttpWriteCookie(Browser* browser) {
-    auto* frame =
-        browser->tab_strip_model()->GetActiveWebContents()->GetMainFrame();
-    auto* network_context =
-        content::BrowserContext::GetDefaultStoragePartition(browser->profile())
-            ->GetNetworkContext();
-    // Need process & frame ID here for the accessed/blocked cookies lists to be
-    // updated properly.
-    content::LoadBasicRequest(network_context, GetSetCookieURL(),
-                              frame->GetProcess()->GetID(),
-                              frame->GetRoutingID());
+    HttpWriteCookieWithURL(browser, GetSetCookieURL());
   }
 
   void MonitorRequest(const net::test_server::HttpRequest& request) {
@@ -458,6 +476,94 @@ IN_PROC_BROWSER_TEST_P(CookieSettingsTest, BlockCookiesUsingExceptions) {
   EXPECT_TRUE(blocked->empty());
 }
 
+// Test that cookies that are considered "blocked" are excluded only due to the
+// content settings blocking (i.e. not for other reasons like domain or path not
+// matching). See https://crbug.com/1104451.
+IN_PROC_BROWSER_TEST_P(CookieSettingsTest,
+                       BlockedCookiesOnlyExcludedDueToBlocking) {
+  // This test only runs in HTTP mode, not with the full parameterized test
+  // suite.
+  if (ReadMode() != CookieMode::kHttp || WriteMode() != CookieMode::kHttp)
+    return;
+
+  // URLs to get cookies from:
+  GURL cookies_present_url(GetServer()->GetURL("a.test", "/simple.html"));
+  GURL cookies_blocked_url(
+      GetServer()->GetURL("sub.a.test", "/echoheader?Cookie"));
+
+  // URLs to set cookies on:
+  GURL set_host_cookie_url(
+      GetServer()->GetURL("a.test", "/set-cookie?host_cookie=1"));
+  GURL set_path_cookie_url(GetServer()->GetURL(
+      "sub.a.test",
+      "/set-cookie?path_cookie=1;domain=a.test;path=/simple.html"));
+  GURL set_included_cookie_url(GetServer()->GetURL(
+      "sub.a.test", "/set-cookie?included_cookie=1;domain=a.test"));
+
+  // No cookies are present prior to setting them.
+  ui_test_utils::NavigateToURL(browser(), cookies_present_url);
+  ASSERT_TRUE(HttpReadCookieWithURL(browser(), cookies_present_url).empty());
+  ui_test_utils::NavigateToURL(browser(), cookies_blocked_url);
+  ASSERT_TRUE(HttpReadCookieWithURL(browser(), cookies_blocked_url).empty());
+
+  // Set the cookies.
+  HttpWriteCookieWithURL(browser(), set_host_cookie_url);
+  HttpWriteCookieWithURL(browser(), set_path_cookie_url);
+  HttpWriteCookieWithURL(browser(), set_included_cookie_url);
+
+  // Verify all cookies are present on |cookies_present_url|.
+  ui_test_utils::NavigateToURL(browser(), cookies_present_url);
+  HttpReadCookieWithURL(browser(), cookies_present_url);
+  browsing_data::CannedCookieHelper* accepted =
+      GetSiteSettingsCookieContainer(browser());
+  browsing_data::CannedCookieHelper* blocked =
+      GetSiteSettingsBlockedCookieContainer(browser());
+  EXPECT_EQ(3u, accepted->GetCookieCount());
+  EXPECT_TRUE(blocked->empty());
+  net::CookieList accepted_cookies = ExtractCookies(accepted);
+  EXPECT_THAT(accepted_cookies,
+              net::MatchesCookieLine(
+                  "included_cookie=1; host_cookie=1; path_cookie=1"));
+
+  // Verify there is only one included cookie for |cookies_blocked_url|.
+  ui_test_utils::NavigateToURL(browser(), cookies_blocked_url);
+  HttpReadCookieWithURL(browser(), cookies_blocked_url);
+  accepted = GetSiteSettingsCookieContainer(browser());
+  blocked = GetSiteSettingsBlockedCookieContainer(browser());
+  EXPECT_EQ(1u, accepted->GetCookieCount());
+  EXPECT_TRUE(blocked->empty());
+  accepted_cookies = ExtractCookies(accepted);
+  EXPECT_THAT(accepted_cookies, net::MatchesCookieLine("included_cookie=1"));
+
+  // Set content settings to block cookies for |cookies_blocked_url|.
+  ui_test_utils::NavigateToURL(browser(), cookies_blocked_url);
+  content_settings::CookieSettings* settings =
+      CookieSettingsFactory::GetForProfile(browser()->profile()).get();
+  settings->SetCookieSetting(cookies_blocked_url, CONTENT_SETTING_BLOCK);
+
+  // Verify all cookies are still present on |cookies_present_url|.
+  ui_test_utils::NavigateToURL(browser(), cookies_present_url);
+  HttpReadCookieWithURL(browser(), cookies_present_url);
+  accepted = GetSiteSettingsCookieContainer(browser());
+  blocked = GetSiteSettingsBlockedCookieContainer(browser());
+  EXPECT_EQ(3u, accepted->GetCookieCount());
+  EXPECT_TRUE(blocked->empty());
+  accepted_cookies = ExtractCookies(accepted);
+  EXPECT_THAT(accepted_cookies,
+              net::MatchesCookieLine(
+                  "included_cookie=1; host_cookie=1; path_cookie=1"));
+
+  // Verify there is only one blocked cookie on |cookies_blocked_url|.
+  ui_test_utils::NavigateToURL(browser(), cookies_blocked_url);
+  HttpReadCookieWithURL(browser(), cookies_blocked_url);
+  accepted = GetSiteSettingsCookieContainer(browser());
+  blocked = GetSiteSettingsBlockedCookieContainer(browser());
+  EXPECT_TRUE(accepted->empty());
+  EXPECT_EQ(1u, blocked->GetCookieCount());
+  net::CookieList blocked_cookies = ExtractCookies(blocked);
+  EXPECT_THAT(blocked_cookies, net::MatchesCookieLine("included_cookie=1"));
+}
+
 IN_PROC_BROWSER_TEST_P(CookieSettingsTest, BlockCookiesAlsoBlocksCacheStorage) {
   ui_test_utils::NavigateToURL(browser(), GetPageURL());
   content_settings::CookieSettings* settings =

content/browser/frame_host/cookie_utils.cc

@@ -56,7 +56,7 @@ void SplitCookiesIntoAllowedAndBlocked(
                            /* blocked_by_policy=*/true});
 
   for (auto& cookie_and_access_result : cookie_details->cookie_list) {
-    if (cookie_and_access_result.access_result.status.HasExclusionReason(
+    if (cookie_and_access_result.access_result.status.HasOnlyExclusionReason(
             net::CookieInclusionStatus::EXCLUDE_USER_PREFERENCES)) {
       blocked->cookie_list.push_back(
           std::move(cookie_and_access_result.cookie));

content/browser/frame_host/cookie_utils.h

@@ -12,6 +12,11 @@ namespace content {
 class RenderFrameHostImpl;
 struct CookieAccessDetails;
 
+// Sorts cookies into allowed (cookies that were included for the access
+// attempt) and blocked (cookies that were excluded solely because they were
+// blocked by the user's preferences). Cookies that are excluded independently
+// of the user's cookie blocking settings are not included in either of the
+// outputs.
 void SplitCookiesIntoAllowedAndBlocked(
     const network::mojom::CookieAccessDetailsPtr& cookie_details,
     CookieAccessDetails* allowed,

net/cookies/cookie_inclusion_status.cc

@@ -51,6 +51,11 @@ bool CookieInclusionStatus::HasExclusionReason(ExclusionReason reason) const {
   return exclusion_reasons_ & GetExclusionBitmask(reason);
 }
 
+bool CookieInclusionStatus::HasOnlyExclusionReason(
+    ExclusionReason reason) const {
+  return exclusion_reasons_ == GetExclusionBitmask(reason);
+}
+
 void CookieInclusionStatus::AddExclusionReason(ExclusionReason reason) {
   exclusion_reasons_ |= GetExclusionBitmask(reason);
   // If the cookie would be excluded for reasons other than the new SameSite

net/cookies/cookie_inclusion_status.h

@@ -184,6 +184,10 @@ class NET_EXPORT CookieInclusionStatus {
   // Whether the given reason for exclusion is present.
   bool HasExclusionReason(ExclusionReason status_type) const;
 
+  // Whether the given reason for exclusion is present, and is the ONLY reason
+  // for exclusion.
+  bool HasOnlyExclusionReason(ExclusionReason status_type) const;
+
   // Add an exclusion reason.
   void AddExclusionReason(ExclusionReason status_type);
 

net/cookies/cookie_inclusion_status_unittest.cc

@@ -32,17 +32,31 @@ TEST(CookieInclusionStatusTest, IncludeStatus) {
 TEST(CookieInclusionStatusTest, ExcludeStatus) {
   int num_exclusion_reasons =
       static_cast<int>(CookieInclusionStatus::NUM_EXCLUSION_REASONS);
+  // Test exactly one exclusion reason and multiple (two) exclusion reasons.
   for (int i = 0; i < num_exclusion_reasons; ++i) {
-    auto reason = static_cast<CookieInclusionStatus::ExclusionReason>(i);
-    CookieInclusionStatus status(reason);
-    EXPECT_TRUE(status.IsValid());
-    EXPECT_FALSE(status.IsInclude());
-    EXPECT_TRUE(status.HasExclusionReason(reason));
+    auto reason1 = static_cast<CookieInclusionStatus::ExclusionReason>(i);
+    CookieInclusionStatus status_one_reason(reason1);
+    EXPECT_TRUE(status_one_reason.IsValid());
+    EXPECT_FALSE(status_one_reason.IsInclude());
+    EXPECT_TRUE(status_one_reason.HasExclusionReason(reason1));
+    EXPECT_TRUE(status_one_reason.HasOnlyExclusionReason(reason1));
+
     for (int j = 0; j < num_exclusion_reasons; ++j) {
       if (i == j)
         continue;
-      EXPECT_FALSE(status.HasExclusionReason(
-          static_cast<CookieInclusionStatus::ExclusionReason>(j)));
+      auto reason2 = static_cast<CookieInclusionStatus::ExclusionReason>(j);
+
+      EXPECT_FALSE(status_one_reason.HasExclusionReason(reason2));
+      EXPECT_FALSE(status_one_reason.HasOnlyExclusionReason(reason2));
+
+      CookieInclusionStatus status_two_reasons = status_one_reason;
+      status_two_reasons.AddExclusionReason(reason2);
+      EXPECT_TRUE(status_two_reasons.IsValid());
+      EXPECT_FALSE(status_two_reasons.IsInclude());
+      EXPECT_TRUE(status_two_reasons.HasExclusionReason(reason1));
+      EXPECT_TRUE(status_two_reasons.HasExclusionReason(reason2));
+      EXPECT_FALSE(status_two_reasons.HasOnlyExclusionReason(reason1));
+      EXPECT_FALSE(status_two_reasons.HasOnlyExclusionReason(reason2));
     }
   }
 }