Split out CookieEffectiveSameSite enum from CookieSameSite

This makes CookieSameSite into two separate enums, CookieSameSite
(representing the SameSite attribute value of the parsed cookie), and
CookieEffectiveSameSite (representing the SameSite mode to be applied
to inclusion/exclusion of that cookie).

Previously they were the same enum, which resulted in overlapping sets
of enum values that were possibly valid for one but not the other.
This change removes the need to check whether a value is valid, because
only valid semantically values are now included in the respective enums.

Bug: None
Change-Id: I467e7ac0551511e1afea74828886f40a57065d2f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1808019
Commit-Queue: Lily Chen <chlily@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#697462}

chrome/browser/extensions/api/cookies/cookies_helpers.cc

@@ -103,7 +103,6 @@ Cookie CreateCookie(const net::CanonicalCookie& canonical_cookie,
   cookie.secure = canonical_cookie.IsSecure();
   cookie.http_only = canonical_cookie.IsHttpOnly();
 
-  DCHECK(net::IsValidSameSiteValue(canonical_cookie.SameSite()));
   switch (canonical_cookie.SameSite()) {
     case net::CookieSameSite::NO_RESTRICTION:
       cookie.same_site = api::cookies::SAME_SITE_STATUS_NO_RESTRICTION;
@@ -118,8 +117,6 @@ Cookie CreateCookie(const net::CanonicalCookie& canonical_cookie,
     case net::CookieSameSite::UNSPECIFIED:
       cookie.same_site = api::cookies::SAME_SITE_STATUS_UNSPECIFIED;
       break;
-    default:
-      NOTREACHED();
   }
 
   cookie.session = !canonical_cookie.IsPersistent();

content/browser/devtools/protocol/network_handler.cc

@@ -129,7 +129,6 @@ std::unique_ptr<Network::Cookie> BuildCookie(
           .SetSession(!cookie.IsPersistent())
           .Build();
 
-  DCHECK(net::IsValidSameSiteValue(cookie.SameSite()));
   switch (cookie.SameSite()) {
     case net::CookieSameSite::STRICT_MODE:
       devtools_cookie->SetSameSite(Network::CookieSameSiteEnum::Strict);
@@ -143,7 +142,7 @@ std::unique_ptr<Network::Cookie> BuildCookie(
     case net::CookieSameSite::NO_RESTRICTION:
       devtools_cookie->SetSameSite(Network::CookieSameSiteEnum::None);
       break;
-    default:
+    case net::CookieSameSite::UNSPECIFIED:
       break;
   }
   return devtools_cookie;

net/cookies/canonical_cookie.cc

@@ -110,7 +110,7 @@ uint32_t GetBitmask(
 
 void ApplySameSiteCookieWarningToStatus(
     CookieSameSite samesite,
-    CookieSameSite effective_samesite,
+    CookieEffectiveSameSite effective_samesite,
     bool is_secure,
     CookieOptions::SameSiteCookieContext context,
     CanonicalCookie::CookieInclusionStatus* status) {
@@ -121,7 +121,7 @@ void ApplySameSiteCookieWarningToStatus(
   }
   // This will overwrite the previous warning but it is more specific so that
   // is ok.
-  if (effective_samesite == CookieSameSite::LAX_MODE_ALLOW_UNSAFE &&
+  if (effective_samesite == CookieEffectiveSameSite::LAX_MODE_ALLOW_UNSAFE &&
       context ==
           CookieOptions::SameSiteCookieContext::SAME_SITE_LAX_METHOD_UNSAFE) {
     status->set_warning(CanonicalCookie::CookieInclusionStatus::
@@ -433,16 +433,16 @@ CanonicalCookie::CookieInclusionStatus CanonicalCookie::IncludeForRequestURL(
   if (!IsOnPath(url.path()))
     status.AddExclusionReason(CookieInclusionStatus::EXCLUDE_NOT_ON_PATH);
   // Don't include same-site cookies for cross-site requests.
-  CookieSameSite effective_same_site = GetEffectiveSameSite();
+  CookieEffectiveSameSite effective_same_site = GetEffectiveSameSite();
   switch (effective_same_site) {
-    case CookieSameSite::STRICT_MODE:
+    case CookieEffectiveSameSite::STRICT_MODE:
       if (options.same_site_cookie_context() <
           CookieOptions::SameSiteCookieContext::SAME_SITE_STRICT) {
         status.AddExclusionReason(
             CookieInclusionStatus::EXCLUDE_SAMESITE_STRICT);
       }
       break;
-    case CookieSameSite::LAX_MODE:
+    case CookieEffectiveSameSite::LAX_MODE:
       if (options.same_site_cookie_context() <
           CookieOptions::SameSiteCookieContext::SAME_SITE_LAX) {
         // Log metrics for a cookie that would have been included under the
@@ -464,7 +464,7 @@ CanonicalCookie::CookieInclusionStatus CanonicalCookie::IncludeForRequestURL(
       }
       break;
     // TODO(crbug.com/990439): Add a browsertest for this behavior.
-    case CookieSameSite::LAX_MODE_ALLOW_UNSAFE:
+    case CookieEffectiveSameSite::LAX_MODE_ALLOW_UNSAFE:
       DCHECK(SameSite() == CookieSameSite::UNSPECIFIED);
       if (options.same_site_cookie_context() <
           CookieOptions::SameSiteCookieContext::SAME_SITE_LAX_METHOD_UNSAFE) {
@@ -513,9 +513,9 @@ CanonicalCookie::CookieInclusionStatus CanonicalCookie::IsSetPermittedInContext(
     status.AddExclusionReason(CookieInclusionStatus::EXCLUDE_HTTP_ONLY);
   }
 
-  CookieSameSite effective_same_site = GetEffectiveSameSite();
+  CookieEffectiveSameSite effective_same_site = GetEffectiveSameSite();
   switch (effective_same_site) {
-    case CookieSameSite::STRICT_MODE:
+    case CookieEffectiveSameSite::STRICT_MODE:
       // This intentionally checks for `< SAME_SITE_LAX`, as we allow
       // `SameSite=Strict` cookies to be set for top-level navigations that
       // qualify for receipt of `SameSite=Lax` cookies.
@@ -528,8 +528,8 @@ CanonicalCookie::CookieInclusionStatus CanonicalCookie::IsSetPermittedInContext(
             CookieInclusionStatus::EXCLUDE_SAMESITE_STRICT);
       }
       break;
-    case CookieSameSite::LAX_MODE:
-    case CookieSameSite::LAX_MODE_ALLOW_UNSAFE:
+    case CookieEffectiveSameSite::LAX_MODE:
+    case CookieEffectiveSameSite::LAX_MODE_ALLOW_UNSAFE:
       if (options.same_site_cookie_context() <
           CookieOptions::SameSiteCookieContext::SAME_SITE_LAX) {
         if (SameSite() == CookieSameSite::UNSPECIFIED) {
@@ -611,17 +611,15 @@ bool CanonicalCookie::IsCanonical() const {
       break;
   }
 
-  if (!IsValidSameSiteValue(same_site_))
-    return false;
-
   return true;
 }
 
 bool CanonicalCookie::IsEffectivelySameSiteNone() const {
-  return GetEffectiveSameSite() == CookieSameSite::NO_RESTRICTION;
+  return GetEffectiveSameSite() == CookieEffectiveSameSite::NO_RESTRICTION;
 }
 
-CookieSameSite CanonicalCookie::GetEffectiveSameSiteForTesting() const {
+CookieEffectiveSameSite CanonicalCookie::GetEffectiveSameSiteForTesting()
+    const {
   return GetEffectiveSameSite();
 }
 
@@ -700,25 +698,27 @@ bool CanonicalCookie::IsCookiePrefixValid(CanonicalCookie::CookiePrefix prefix,
   return true;
 }
 
-CookieSameSite CanonicalCookie::GetEffectiveSameSite() const {
-  CookieSameSite effective_same_site = SameSite();
-  // If a cookie does not have a SameSite attribute, the effective SameSite
-  // mode depends on the SameSiteByDefaultCookies setting and whether the cookie
-  // is short-lived.
-  if (SameSite() == CookieSameSite::UNSPECIFIED) {
-    effective_same_site = cookie_util::IsSameSiteByDefaultCookiesEnabled()
-                              ? (IsRecentlyCreated(kLaxAllowUnsafeMaxAge)
-                                     ? CookieSameSite::LAX_MODE_ALLOW_UNSAFE
-                                     : CookieSameSite::LAX_MODE)
-                              : CookieSameSite::NO_RESTRICTION;
+CookieEffectiveSameSite CanonicalCookie::GetEffectiveSameSite() const {
+  switch (SameSite()) {
+    // If a cookie does not have a SameSite attribute, the effective SameSite
+    // mode depends on the SameSiteByDefaultCookies setting and whether the
+    // cookie is recently-created.
+    case CookieSameSite::UNSPECIFIED:
+      return cookie_util::IsSameSiteByDefaultCookiesEnabled()
+                 ? (IsRecentlyCreated(kLaxAllowUnsafeMaxAge)
+                        ? CookieEffectiveSameSite::LAX_MODE_ALLOW_UNSAFE
+                        : CookieEffectiveSameSite::LAX_MODE)
+                 : CookieEffectiveSameSite::NO_RESTRICTION;
+    case CookieSameSite::NO_RESTRICTION:
+      return CookieEffectiveSameSite::NO_RESTRICTION;
+    case CookieSameSite::LAX_MODE:
+      return CookieEffectiveSameSite::LAX_MODE;
+    case CookieSameSite::STRICT_MODE:
+      return CookieEffectiveSameSite::STRICT_MODE;
+    // TODO(crbug.com/989171): Replace this with FirstParty{Lax,Strict}.
+    case CookieSameSite::EXTENDED_MODE:
+      return CookieEffectiveSameSite::LAX_MODE;
   }
-
-  // TODO(crbug.com/989171): Replace this with FirstParty{Lax,Strict}.
-  if (SameSite() == CookieSameSite::EXTENDED_MODE)
-    effective_same_site = CookieSameSite::LAX_MODE;
-
-  DCHECK(IsValidEffectiveSameSiteValue(effective_same_site));
-  return effective_same_site;
 }
 
 bool CanonicalCookie::IsRecentlyCreated(base::TimeDelta age_threshold) const {

net/cookies/canonical_cookie.h

@@ -220,7 +220,7 @@ class NET_EXPORT CanonicalCookie {
   // SameSite restrictions).
   bool IsEffectivelySameSiteNone() const;
 
-  CookieSameSite GetEffectiveSameSiteForTesting() const;
+  CookieEffectiveSameSite GetEffectiveSameSiteForTesting() const;
 
   // Returns the cookie line (e.g. "cookie1=value1; cookie2=value2") represented
   // by |cookies|. The string is built in the same order as the given list.
@@ -269,7 +269,7 @@ class NET_EXPORT CanonicalCookie {
   // are considering using this method, consider whether you should use
   // IncludeForRequestURL() or IsSetPermittedInContext() instead of doing the
   // SameSite computation yourself.
-  CookieSameSite GetEffectiveSameSite() const;
+  CookieEffectiveSameSite GetEffectiveSameSite() const;
 
   // Returns whether the cookie was created at most |age_threshold| ago.
   bool IsRecentlyCreated(base::TimeDelta age_threshold) const;

net/cookies/cookie_constants.cc

@@ -54,7 +54,6 @@ CookiePriority StringToCookiePriority(const std::string& priority) {
 }
 
 std::string CookieSameSiteToString(CookieSameSite same_site) {
-  DCHECK(IsValidSameSiteValue(same_site));
   switch (same_site) {
     case CookieSameSite::LAX_MODE:
       return kSameSiteLax;
@@ -66,9 +65,6 @@ std::string CookieSameSiteToString(CookieSameSite same_site) {
       return kSameSiteExtended;
     case CookieSameSite::UNSPECIFIED:
       return kSameSiteUnspecified;
-    default:
-      NOTREACHED();
-      return "INVALID";
   }
 }
 
@@ -98,7 +94,6 @@ CookieSameSite StringToCookieSameSite(const std::string& same_site,
   } else if (same_site == "") {
     *samesite_string = CookieSameSiteString::kEmptyString;
   }
-  DCHECK(IsValidSameSiteValue(samesite));
   return samesite;
 }
 
@@ -106,34 +101,4 @@ void RecordCookieSameSiteAttributeValueHistogram(CookieSameSiteString value) {
   UMA_HISTOGRAM_ENUMERATION("Cookie.SameSiteAttributeValue", value);
 }
 
-bool IsValidSameSiteValue(CookieSameSite value) {
-  switch (value) {
-    case CookieSameSite::UNSPECIFIED:
-    case CookieSameSite::NO_RESTRICTION:
-    case CookieSameSite::LAX_MODE:
-    case CookieSameSite::STRICT_MODE:
-    case CookieSameSite::EXTENDED_MODE:
-      return true;
-    case CookieSameSite::LAX_MODE_ALLOW_UNSAFE:
-      return false;
-  }
-  NOTREACHED();
-  return false;
-}
-
-bool IsValidEffectiveSameSiteValue(CookieSameSite value) {
-  switch (value) {
-    case CookieSameSite::NO_RESTRICTION:
-    case CookieSameSite::LAX_MODE:
-    case CookieSameSite::LAX_MODE_ALLOW_UNSAFE:
-    case CookieSameSite::STRICT_MODE:
-      return true;
-    case CookieSameSite::UNSPECIFIED:
-    case CookieSameSite::EXTENDED_MODE:
-      return false;
-  }
-  NOTREACHED();
-  return false;
-}
-
 }  // namespace net

net/cookies/cookie_constants.h

@@ -26,19 +26,27 @@ enum CookiePriority {
 // See https://tools.ietf.org/html/draft-ietf-httpbis-cookie-same-site-00
 // and https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis for
 // information about same site cookie restrictions.
-// Note: Some values are allowed for a cookie's SameSite field (what literally
-// came in the Set-Cookie line), and some are allowed for the effective SameSite
-// (the actual rules to be applied when deciding whether the cookie can be
-// accessed). Some are only allowed for one but not the other.
+// These values are allowed for the SameSite field of a cookie. They mostly
+// correspond to CookieEffectiveSameSite values.
 // Note: Don't renumber, as these values are persisted to a database.
 enum class CookieSameSite {
-  UNSPECIFIED = -1,    // Allowed for SameSite only.
-  NO_RESTRICTION = 0,  // Allowed for SameSite and effective SameSite.
-  LAX_MODE = 1,        // Allowed for SameSite and effective SameSite.
-  STRICT_MODE = 2,     // Allowed for SameSite and effective SameSite.
-  EXTENDED_MODE = 3,   // (Not implemented) Allowed for SameSite only.
-  // Same as LAX_MODE, except cookie is also sent if the HTTP method is unsafe.
-  LAX_MODE_ALLOW_UNSAFE = 4,  // Allowed for effective SameSite only.
+  UNSPECIFIED = -1,
+  NO_RESTRICTION = 0,
+  LAX_MODE = 1,
+  STRICT_MODE = 2,
+  EXTENDED_MODE = 3,  // TODO(chlily): Remove or gate behind flag.
+};
+
+// These are the enforcement modes that may be applied to a cookie when deciding
+// inclusion/exclusion. They mostly correspond to CookieSameSite values.
+enum class CookieEffectiveSameSite {
+  NO_RESTRICTION = 0,
+  LAX_MODE = 1,
+  STRICT_MODE = 2,
+  LAX_MODE_ALLOW_UNSAFE = 3,
+
+  // Keep last, used for histograms.
+  COUNT
 };
 
 // Used for histograms only. Do not renumber. Keep in sync with enums.xml.
@@ -99,9 +107,6 @@ StringToCookieSameSite(const std::string& same_site,
 NET_EXPORT void RecordCookieSameSiteAttributeValueHistogram(
     CookieSameSiteString value);
 
-NET_EXPORT bool IsValidSameSiteValue(CookieSameSite value);
-NET_EXPORT bool IsValidEffectiveSameSiteValue(CookieSameSite value);
-
 }  // namespace net
 
 #endif  // NET_COOKIES_COOKIE_CONSTANTS_H_

net/cookies/cookie_monster_unittest.cc

@@ -3421,25 +3421,27 @@ TEST_F(CookieMonsterTest, CookiesWithoutSameSiteMustBeSecure) {
     std::string cookie_line;
     CanonicalCookie::CookieInclusionStatus expected_set_cookie_result;
     // Only makes sense to check if result is INCLUDE:
-    CookieSameSite expected_effective_samesite = CookieSameSite::UNSPECIFIED;
+    CookieEffectiveSameSite expected_effective_samesite =
+        CookieEffectiveSameSite::NO_RESTRICTION;
     base::TimeDelta creation_time_delta = base::TimeDelta();
   } test_cases[] = {
       // Feature enabled:
       // Cookie set from a secure URL with SameSite enabled is not rejected.
       {true, true, "A=B; SameSite=Lax",
-       CanonicalCookie::CookieInclusionStatus(), CookieSameSite::LAX_MODE},
+       CanonicalCookie::CookieInclusionStatus(),
+       CookieEffectiveSameSite::LAX_MODE},
       // Cookie set from a secure URL which is defaulted into Lax is not
       // rejected.
       {true, true, "A=B",  // recently-set session cookie.
        CanonicalCookie::CookieInclusionStatus(),
-       CookieSameSite::LAX_MODE_ALLOW_UNSAFE, kShortAge},
+       CookieEffectiveSameSite::LAX_MODE_ALLOW_UNSAFE, kShortAge},
       {true, true, "A=B",  // not-recently-set session cookie.
-       CanonicalCookie::CookieInclusionStatus(), CookieSameSite::LAX_MODE,
-       kLongAge},
+       CanonicalCookie::CookieInclusionStatus(),
+       CookieEffectiveSameSite::LAX_MODE, kLongAge},
       // Cookie set from a secure URL with SameSite=None and Secure is set.
       {true, true, "A=B; SameSite=None; Secure",
        CanonicalCookie::CookieInclusionStatus(),
-       CookieSameSite::NO_RESTRICTION},
+       CookieEffectiveSameSite::NO_RESTRICTION},
       // Cookie set from a secure URL with SameSite=None but not specifying
       // Secure is rejected.
       {true, true, "A=B; SameSite=None",
@@ -3452,34 +3454,35 @@ TEST_F(CookieMonsterTest, CookiesWithoutSameSiteMustBeSecure) {
       // rejected.
       {true, false, "A=B",  // recently-set session cookie.
        CanonicalCookie::CookieInclusionStatus(),
-       CookieSameSite::LAX_MODE_ALLOW_UNSAFE, kShortAge},
+       CookieEffectiveSameSite::LAX_MODE_ALLOW_UNSAFE, kShortAge},
       {true, false, "A=B",  // not-recently-set session cookie.
-       CanonicalCookie::CookieInclusionStatus(), CookieSameSite::LAX_MODE,
-       kLongAge},
+       CanonicalCookie::CookieInclusionStatus(),
+       CookieEffectiveSameSite::LAX_MODE, kLongAge},
       {true, false, "A=B; Max-Age=1000000",  // recently-set persistent cookie.
        CanonicalCookie::CookieInclusionStatus(),
-       CookieSameSite::LAX_MODE_ALLOW_UNSAFE, kShortAge},
+       CookieEffectiveSameSite::LAX_MODE_ALLOW_UNSAFE, kShortAge},
       {true, false,
        "A=B; Max-Age=1000000",  // not-recently-set persistent cookie.
-       CanonicalCookie::CookieInclusionStatus(), CookieSameSite::LAX_MODE,
-       kLongAge},
+       CanonicalCookie::CookieInclusionStatus(),
+       CookieEffectiveSameSite::LAX_MODE, kLongAge},
 
       // Feature not enabled (but SameSiteByDefaultCookies is still enabled):
       // Cookie set from a secure URL with SameSite enabled is not rejected.
       {false, true, "A=B; SameSite=Lax",
-       CanonicalCookie::CookieInclusionStatus(), CookieSameSite::LAX_MODE},
+       CanonicalCookie::CookieInclusionStatus(),
+       CookieEffectiveSameSite::LAX_MODE},
       // Cookie set from a secure URL which is defaulted into Lax is not
       // rejected.
       {false, true, "A=B",  // recently-set session cookie.
        CanonicalCookie::CookieInclusionStatus(),
-       CookieSameSite::LAX_MODE_ALLOW_UNSAFE, kShortAge},
+       CookieEffectiveSameSite::LAX_MODE_ALLOW_UNSAFE, kShortAge},
       {false, true, "A=B",  // not-recently-set session cookie.
-       CanonicalCookie::CookieInclusionStatus(), CookieSameSite::LAX_MODE,
-       kLongAge},
+       CanonicalCookie::CookieInclusionStatus(),
+       CookieEffectiveSameSite::LAX_MODE, kLongAge},
       // Cookie set from a secure URL with SameSite=None and Secure is set.
       {false, true, "A=B; SameSite=None; Secure",
        CanonicalCookie::CookieInclusionStatus(),
-       CookieSameSite::NO_RESTRICTION},
+       CookieEffectiveSameSite::NO_RESTRICTION},
       // Cookie set from an insecure URL with SameSite=None (which can't ever be
       // secure because it's an insecure URL) is NOT rejected, because
       // CookiesWithoutSameSiteMustBeSecure is not enabled.
@@ -3488,15 +3491,15 @@ TEST_F(CookieMonsterTest, CookiesWithoutSameSiteMustBeSecure) {
            std::vector<
                CanonicalCookie::CookieInclusionStatus::ExclusionReason>(),
            CanonicalCookie::CookieInclusionStatus::WARN_SAMESITE_NONE_INSECURE),
-       CookieSameSite::NO_RESTRICTION},
+       CookieEffectiveSameSite::NO_RESTRICTION},
       // Cookie set from an insecure URL which is defaulted into Lax is not
       // rejected.
       {false, false, "A=B",  // recently-set session cookie.
        CanonicalCookie::CookieInclusionStatus(),
-       CookieSameSite::LAX_MODE_ALLOW_UNSAFE, kShortAge},
+       CookieEffectiveSameSite::LAX_MODE_ALLOW_UNSAFE, kShortAge},
       {false, false, "A=B",  // not-recently-set session cookie.
-       CanonicalCookie::CookieInclusionStatus(), CookieSameSite::LAX_MODE,
-       kLongAge},
+       CanonicalCookie::CookieInclusionStatus(),
+       CookieEffectiveSameSite::LAX_MODE, kLongAge},
   };
 
   auto cm = std::make_unique<CookieMonster>(nullptr, nullptr);

net/extras/sqlite/sqlite_persistent_cookie_store.cc

@@ -481,7 +481,6 @@ enum DBCookieSameSite {
 };
 
 DBCookieSameSite CookieSameSiteToDBCookieSameSite(CookieSameSite value) {
-  DCHECK(IsValidSameSiteValue(value));
   switch (value) {
     case CookieSameSite::NO_RESTRICTION:
       return kCookieSameSiteNoRestriction;
@@ -493,12 +492,7 @@ DBCookieSameSite CookieSameSiteToDBCookieSameSite(CookieSameSite value) {
       return kCookieSameSiteExtended;
     case CookieSameSite::UNSPECIFIED:
       return kCookieSameSiteUnspecified;
-    default:
-      break;
   }
-
-  NOTREACHED();
-  return kCookieSameSiteUnspecified;
 }
 
 CookieSameSite DBCookieSameSiteToCookieSameSite(DBCookieSameSite value) {
@@ -520,8 +514,6 @@ CookieSameSite DBCookieSameSiteToCookieSameSite(DBCookieSameSite value) {
       samesite = CookieSameSite::UNSPECIFIED;
       break;
   }
-
-  DCHECK(IsValidSameSiteValue(samesite));
   return samesite;
 }