Make CookiesWithoutSameSiteMustBeSecure reject regardless of source

This CL changes the CookiesWithoutSameSiteMustBeSecure flag's behavior.
Previously, a SameSite=None cookie set without Secure, would be treated
as Secure if set from a secure context, or rejected if set from an
insecure context. This CL changes that to always reject such a cookie
regardless of source scheme.

Bug: 954551
Change-Id: Ie035ebc97425f855665b81419ac717173e2dcba5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1594693
Commit-Queue: Lily Chen <chlily@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/master@{#656409}

chrome/browser/flag_descriptions.cc

@@ -288,8 +288,7 @@ extern const char kCookiesWithoutSameSiteMustBeSecureName[] =
 extern const char kCookiesWithoutSameSiteMustBeSecureDescription[] =
     "If enabled, cookies without SameSite restrictions must also be Secure. If "
     "a cookie without SameSite restrictions is set without the Secure "
-    "attribute, it will be treated as Secure (if set from a secure URL), or "
-    "rejected (if set from an insecure URL). This flag only has an effect if "
+    "attribute, it will be rejected. This flag only has an effect if "
     "\"SameSite by default cookies\" is also enabled.";
 
 const char kCreditCardAssistName[] = "Credit Card Assisted Filling";

net/cookies/canonical_cookie.h

@@ -166,7 +166,6 @@ class NET_EXPORT CanonicalCookie {
   // '/login' and '/' do not match '/login/en').
   bool IsEquivalentForSecureCookieMatching(const CanonicalCookie& ecc) const;
 
-  void SetSecure(bool is_secure) { secure_ = is_secure; }
   void SetLastAccessDate(const base::Time& date) {
     last_access_date_ = date;
   }

net/cookies/cookie_monster.cc

@@ -1210,25 +1210,18 @@ void CookieMonster::SetCanonicalCookie(std::unique_ptr<CanonicalCookie> cc,
 
   // If both SameSiteByDefaultCookies and CookiesWithoutSameSiteMustBeSecure
   // are enabled, non-SameSite cookies without the Secure attribute will be
-  // treated as secure if set from a secure context, or rejected if set from an
-  // insecure context.
+  // rejected.
   if (base::FeatureList::IsEnabled(features::kSameSiteByDefaultCookies) &&
       base::FeatureList::IsEnabled(
           features::kCookiesWithoutSameSiteMustBeSecure) &&
       cc->GetEffectiveSameSite() == CookieSameSite::NO_RESTRICTION &&
       !cc->IsSecure()) {
-    if (!secure_source) {
-      DVLOG(net::cookie_util::kVlogSetCookies)
-          << "SetCookie() rejecting insecure cookie with SameSite=None.";
-      status = CanonicalCookie::CookieInclusionStatus::
-          EXCLUDE_SAMESITE_NONE_INSECURE;
-      MaybeRunCookieCallback(std::move(callback), status);
-      return;
-    }
     DVLOG(net::cookie_util::kVlogSetCookies)
-        << "SetCookie() treating cookie without SameSite restrictions as "
-           "secure.";
-    cc->SetSecure(true);
+        << "SetCookie() rejecting insecure cookie with SameSite=None.";
+    status =
+        CanonicalCookie::CookieInclusionStatus::EXCLUDE_SAMESITE_NONE_INSECURE;
+    MaybeRunCookieCallback(std::move(callback), status);
+    return;
   }
 
   const std::string key(GetKey(cc->Domain()));

net/cookies/cookie_monster_unittest.cc

@@ -3273,8 +3273,7 @@ TEST_F(CookieMonsterTest, CookiesWithoutSameSiteMustBeSecure) {
          features::kCookiesWithoutSameSiteMustBeSecure} /* enabled_features */,
         {} /* disabled_features */);
 
-    // Cookie set from a secure URL with SameSite enabled is not forced to be
-    // secure.
+    // Cookie set from a secure URL with SameSite enabled is not rejected.
     result = SetCookieReturnStatus(cm.get(), secure_url, "A=B; SameSite=Lax");
     EXPECT_EQ(CanonicalCookie::CookieInclusionStatus::INCLUDE, result);
     CookieList cookies = GetAllCookiesForURL(cm.get(), secure_url);
@@ -3282,8 +3281,8 @@ TEST_F(CookieMonsterTest, CookiesWithoutSameSiteMustBeSecure) {
     EXPECT_EQ(CookieSameSite::LAX_MODE, cookies[0].SameSite());
     EXPECT_FALSE(cookies[0].IsSecure());
 
-    // Cookie set from a secure URL which defaults into LAX_MODE is not forced
-    // to be secure.
+    // Cookie set from a secure URL which defaults into LAX_MODE is not
+    // rejected.
     result = SetCookieReturnStatus(cm.get(), secure_url, "A=B");
     EXPECT_EQ(CanonicalCookie::CookieInclusionStatus::INCLUDE, result);
     cookies = GetAllCookiesForURL(cm.get(), secure_url);
@@ -3292,15 +3291,22 @@ TEST_F(CookieMonsterTest, CookiesWithoutSameSiteMustBeSecure) {
     EXPECT_EQ(CookieSameSite::LAX_MODE, cookies[0].GetEffectiveSameSite());
     EXPECT_FALSE(cookies[0].IsSecure());
 
-    // Cookie set from a secure URL with SameSite=None but not specifying Secure
-    // is converted to a secure cookie.
-    result = SetCookieReturnStatus(cm.get(), secure_url, "A=B; SameSite=None");
+    // Cookie set from a secure URL with SameSite=None and Secure is set.
+    result = SetCookieReturnStatus(cm.get(), secure_url,
+                                   "A=B; SameSite=None; Secure");
     EXPECT_EQ(CanonicalCookie::CookieInclusionStatus::INCLUDE, result);
     cookies = GetAllCookiesForURL(cm.get(), secure_url);
-    ASSERT_EQ(1u, cookies.size());  // We overwrote the previous cookie.
+    ASSERT_EQ(1u, cookies.size());
     EXPECT_EQ(CookieSameSite::NO_RESTRICTION, cookies[0].SameSite());
     EXPECT_TRUE(cookies[0].IsSecure());
 
+    // Cookie set from a secure URL with SameSite=None but not specifying Secure
+    // is rejected.
+    result = SetCookieReturnStatus(cm.get(), secure_url, "A=B; SameSite=None");
+    EXPECT_EQ(
+        CanonicalCookie::CookieInclusionStatus::EXCLUDE_SAMESITE_NONE_INSECURE,
+        result);
+
     // Cookie set from an insecure URL with SameSite=None (which can't ever be
     // secure because it's an insecure URL) is rejected.
     result =
@@ -3310,7 +3316,7 @@ TEST_F(CookieMonsterTest, CookiesWithoutSameSiteMustBeSecure) {
         result);
 
     // Cookie set from an insecure URL which defaults into LAX_MODE is not
-    // forced to be secure.
+    // rejected.
     result = SetCookieReturnStatus(cm.get(), insecure_url, "A=B");
     EXPECT_EQ(CanonicalCookie::CookieInclusionStatus::INCLUDE, result);
     cookies = GetAllCookiesForURL(cm.get(), insecure_url);
@@ -3329,8 +3335,7 @@ TEST_F(CookieMonsterTest, CookiesWithoutSameSiteMustBeSecure) {
         {features::
              kCookiesWithoutSameSiteMustBeSecure} /* disabled_features */);
 
-    // Cookie set from a secure URL with SameSite enabled is not forced to be
-    // secure.
+    // Cookie set from a secure URL with SameSite enabled is not rejected.
     result = SetCookieReturnStatus(cm.get(), secure_url, "A=B; SameSite=Lax");
     EXPECT_EQ(CanonicalCookie::CookieInclusionStatus::INCLUDE, result);
     CookieList cookies = GetAllCookiesForURL(cm.get(), secure_url);
@@ -3338,8 +3343,8 @@ TEST_F(CookieMonsterTest, CookiesWithoutSameSiteMustBeSecure) {
     EXPECT_EQ(CookieSameSite::LAX_MODE, cookies[0].SameSite());
     EXPECT_FALSE(cookies[0].IsSecure());
 
-    // Cookie set from a secure URL which defaults into LAX_MODE is not forced
-    // to be secure.
+    // Cookie set from a secure URL which defaults into LAX_MODE is not
+    // rejected.
     result = SetCookieReturnStatus(cm.get(), secure_url, "A=B");
     EXPECT_EQ(CanonicalCookie::CookieInclusionStatus::INCLUDE, result);
     cookies = GetAllCookiesForURL(cm.get(), secure_url);
@@ -3348,8 +3353,17 @@ TEST_F(CookieMonsterTest, CookiesWithoutSameSiteMustBeSecure) {
     EXPECT_EQ(CookieSameSite::LAX_MODE, cookies[0].GetEffectiveSameSite());
     EXPECT_FALSE(cookies[0].IsSecure());
 
+    // Cookie set from a secure URL with SameSite=None and Secure is set.
+    result = SetCookieReturnStatus(cm.get(), secure_url,
+                                   "A=B; SameSite=None; Secure");
+    EXPECT_EQ(CanonicalCookie::CookieInclusionStatus::INCLUDE, result);
+    cookies = GetAllCookiesForURL(cm.get(), secure_url);
+    ASSERT_EQ(1u, cookies.size());
+    EXPECT_EQ(CookieSameSite::NO_RESTRICTION, cookies[0].SameSite());
+    EXPECT_TRUE(cookies[0].IsSecure());
+
     // Cookie set from a secure URL with SameSite=None but not specifying Secure
-    // is NOT converted to a secure cookie.
+    // is NOT rejected.
     result = SetCookieReturnStatus(cm.get(), secure_url, "A=B; SameSite=None");
     EXPECT_EQ(CanonicalCookie::CookieInclusionStatus::INCLUDE, result);
     cookies = GetAllCookiesForURL(cm.get(), secure_url);
@@ -3368,7 +3382,7 @@ TEST_F(CookieMonsterTest, CookiesWithoutSameSiteMustBeSecure) {
     EXPECT_FALSE(cookies[0].IsSecure());
 
     // Cookie set from an insecure URL which defaults into LAX_MODE is not
-    // forced to be secure.
+    // rejected.
     result = SetCookieReturnStatus(cm.get(), insecure_url, "A=B");
     EXPECT_EQ(CanonicalCookie::CookieInclusionStatus::INCLUDE, result);
     cookies = GetAllCookiesForURL(cm.get(), insecure_url);