Exclude Cookie with non-ascii value in Cookie name or value

Guarded by net::feature flag DisallowNonAsciiCookies,
when a cookie's name or value contains a non-ascii value,
this cookie will be excluded. This feature is disabled by
default.

Bug: 40061459
Change-Id: Ib853b730568065a5a73d699a1cac51c548c72a40
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6123584
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Reviewed-by: Steven Bingler <bingler@chromium.org>
Commit-Queue: Amarjot Gill <amarjotgill@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1404400}

content/public/common/content_switch_dependent_feature_overrides.cc

@@ -96,6 +96,9 @@ GetSwitchDependentFeatureOverrides(const base::CommandLine& command_line) {
       {switches::kEnableExperimentalCookieFeatures,
        std::cref(net::features::kEnableSchemeBoundCookies),
        base::FeatureList::OVERRIDE_ENABLE_FEATURE},
+      {switches::kEnableExperimentalCookieFeatures,
+       std::cref(net::features::kDisallowNonAsciiCookies),
+       base::FeatureList::OVERRIDE_ENABLE_FEATURE},
 
       // Test behavior for third-party cookie phaseout.
       {network::switches::kTestThirdPartyCookiePhaseout,

net/base/features.cc

@@ -502,6 +502,12 @@ BASE_FEATURE(kEnableSchemeBoundCookies,
              "EnableSchemeBoundCookies",
              base::FEATURE_DISABLED_BY_DEFAULT);
 
+// Disallows cookies to have non ascii values in their name or value.
+NET_EXPORT BASE_DECLARE_FEATURE(kDisallowNonAsciiCookies);
+BASE_FEATURE(kDisallowNonAsciiCookies,
+             "kDisallowNonAsciiCookies",
+             base::FEATURE_DISABLED_BY_DEFAULT);
+
 BASE_FEATURE(kTimeLimitedInsecureCookies,
              "TimeLimitedInsecureCookies",
              base::FEATURE_DISABLED_BY_DEFAULT);

net/base/features.h

@@ -541,6 +541,9 @@ NET_EXPORT BASE_DECLARE_FEATURE(kEnablePortBoundCookies);
 // enables domain cookie shadowing protection.
 NET_EXPORT BASE_DECLARE_FEATURE(kEnableSchemeBoundCookies);
 
+// Disallows cookies to have non ascii values in their name or value.
+NET_EXPORT BASE_DECLARE_FEATURE(kDisallowNonAsciiCookies);
+
 // Enables expiration duration limit (3 hours) for cookies on insecure websites.
 // This feature is a no-op unless kEnableSchemeBoundCookies is enabled.
 NET_EXPORT BASE_DECLARE_FEATURE(kTimeLimitedInsecureCookies);

net/cookies/canonical_cookie.cc

@@ -459,6 +459,14 @@ std::unique_ptr<CanonicalCookie> CanonicalCookie::Create(
       parsed_cookie.IsHttpOnly(), samesite, parsed_cookie.Priority(),
       cookie_partition_key, source_scheme, source_port, source_type);
 
+  // Check if name or value contains any non-ascii values, exclude if they do.
+  if (base::FeatureList::IsEnabled(features::kDisallowNonAsciiCookies)) {
+    if (!base::IsStringASCII(cc->Name()) || !base::IsStringASCII(cc->Value())) {
+      status->AddExclusionReason(
+          CookieInclusionStatus::EXCLUDE_DISALLOWED_CHARACTER);
+    }
+  }
+
   // TODO(chlily): Log metrics.
   if (!cc->IsCanonical()) {
     status->AddExclusionReason(
@@ -469,7 +477,7 @@ std::unique_ptr<CanonicalCookie> CanonicalCookie::Create(
   RecordCookieSameSiteAttributeValueHistogram(samesite_string);
 
   // These metrics capture whether or not a cookie has a Non-ASCII character in
-  // it.
+  // it, except if kDisallowNonAsciiCookies is enabled.
   UMA_HISTOGRAM_BOOLEAN("Cookie.HasNonASCII.Name",
                         !base::IsStringASCII(cc->Name()));
   UMA_HISTOGRAM_BOOLEAN("Cookie.HasNonASCII.Value",
@@ -524,6 +532,14 @@ std::unique_ptr<CanonicalCookie> CanonicalCookie::CreateSanitizedCookie(
         net::CookieInclusionStatus::EXCLUDE_DISALLOWED_CHARACTER);
   }
 
+  // Check if name or value contains any non-ascii values, exclude if they do.
+  if (base::FeatureList::IsEnabled(features::kDisallowNonAsciiCookies)) {
+    if (!base::IsStringASCII(name) || !base::IsStringASCII(value)) {
+      status->AddExclusionReason(
+          CookieInclusionStatus::EXCLUDE_DISALLOWED_CHARACTER);
+    }
+  }
+
   // Validate name and value against character set and size limit constraints.
   // If IsValidCookieNameValuePair identifies that `name` and/or `value` are
   // invalid, it will add an ExclusionReason to `status`.
@@ -947,6 +963,13 @@ bool CanonicalCookie::IsCanonicalForFromStorage() const {
     return false;
   }
 
+  // Check if name or value contains any non-ascii values, fail if they do.
+  if (base::FeatureList::IsEnabled(features::kDisallowNonAsciiCookies)) {
+    if (!base::IsStringASCII(Name()) || !base::IsStringASCII(Value())) {
+      return false;
+    }
+  }
+
   url::CanonHostInfo canon_host_info;
   std::string canonical_domain(CanonicalizeHost(Domain(), &canon_host_info));
 

net/cookies/canonical_cookie_unittest.cc

@@ -336,6 +336,169 @@ TEST(CanonicalCookieTest, CreateInvalidUrl) {
       CookieInclusionStatus::ExclusionReason::EXCLUDE_FAILURE_TO_STORE));
 }
 
+// Test that when net feature kDisallowNonAsciiCookies is enabled a cookie with
+// a non-ascii value in its name or value is properly excluded.
+TEST(CanonicalCookieTest, CreateNonAsciiCookieNameAndValue) {
+  base::Time creation_time = base::Time::Now();
+  std::optional<base::Time> server_time = std::nullopt;
+  CookieInclusionStatus status;
+  std::unique_ptr<CanonicalCookie> cc;
+
+  // Feature is not enabled yet so cookie with non-ascii name or value should
+  // still be included.
+  cc = CanonicalCookie::Create(GURL("https://www.foo.com/path"), "€=2",
+                               creation_time, server_time,
+                               /*cookie_partition_key=*/std::nullopt,
+                               CookieSourceType::kUnknown, &status);
+  EXPECT_TRUE(cc.get());
+  EXPECT_TRUE(status.IsInclude());
+  cc = CanonicalCookie::Create(GURL("https://www.foo.com/path"), "A=€",
+                               creation_time, server_time,
+                               /*cookie_partition_key=*/std::nullopt,
+                               CookieSourceType::kUnknown, &status);
+  EXPECT_TRUE(cc.get());
+  EXPECT_TRUE(status.IsInclude());
+
+  cc = CanonicalCookie::Create(GURL("https://www.foo.com/path"), "€=€",
+                               creation_time, server_time,
+                               /*cookie_partition_key=*/std::nullopt,
+                               CookieSourceType::kUnknown, &status);
+  EXPECT_TRUE(cc.get());
+  EXPECT_TRUE(status.IsInclude());
+
+  // Since feature is not enabled this should be true.
+  EXPECT_TRUE(cc->IsCanonical());
+
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitWithFeatures(
+      {net::features::kDisallowNonAsciiCookies}, {});
+
+  // Now that feature is enabled this should return false.
+  EXPECT_FALSE(cc->IsCanonical());
+
+  // Valid cookie which should be included.
+  cc = CanonicalCookie::Create(GURL("https://www.foo.com/path"), "A=2",
+                               creation_time, server_time,
+                               /*cookie_partition_key=*/std::nullopt,
+                               CookieSourceType::kUnknown, &status);
+  EXPECT_TRUE(cc.get());
+  EXPECT_TRUE(status.IsInclude());
+  // Now that feature is enabled this cookie will be excluded.
+  cc = CanonicalCookie::Create(GURL("https://www.foo.com/path"), "€=2",
+                               creation_time, server_time,
+                               /*cookie_partition_key=*/std::nullopt,
+                               CookieSourceType::kUnknown, &status);
+  EXPECT_FALSE(cc.get());
+  EXPECT_TRUE(status.HasExclusionReason(
+      CookieInclusionStatus::EXCLUDE_DISALLOWED_CHARACTER));
+
+  // Now that feature is enabled this cookie will be excluded.
+  cc = CanonicalCookie::Create(GURL("https://www.foo.com/path"), "A=€",
+                               creation_time, server_time,
+                               /*cookie_partition_key=*/std::nullopt,
+                               CookieSourceType::kUnknown, &status);
+  EXPECT_FALSE(cc.get());
+  EXPECT_TRUE(status.HasExclusionReason(
+      CookieInclusionStatus::EXCLUDE_DISALLOWED_CHARACTER));
+
+  // Now that feature is enabled this cookie will be excluded.
+  cc = CanonicalCookie::Create(GURL("https://www.foo.com/path"), "€=€",
+                               creation_time, server_time,
+                               /*cookie_partition_key=*/std::nullopt,
+                               CookieSourceType::kUnknown, &status);
+  EXPECT_FALSE(cc.get());
+  EXPECT_TRUE(status.HasExclusionReason(
+      CookieInclusionStatus::EXCLUDE_DISALLOWED_CHARACTER));
+
+  cc = CanonicalCookie::CreateSanitizedCookie(
+      GURL("https://www.foo.com"), "A", "B", std::string(), "/foo",
+      base::Time(), base::Time(), base::Time(), false /*secure*/,
+      false /*httponly*/, CookieSameSite::NO_RESTRICTION,
+      COOKIE_PRIORITY_DEFAULT, std::nullopt /*partition_key*/, &status);
+}
+
+// Test that when net feature kDisallowNonAsciiCookies is enabled a cookie with
+// a non-ascii value in its name or value is properly excluded.
+TEST(CanonicalCookieTest, CreateSanitizedNonAsciiCookieNameAndValue) {
+  CookieInclusionStatus status;
+  std::unique_ptr<CanonicalCookie> cc;
+
+  // Feature is not enabled yet so cookie with non-ascii name or value should
+  // still be included.
+  cc = CanonicalCookie::CreateSanitizedCookie(
+      GURL("https://www.foo.com"), "€", "A", std::string(), "/foo",
+      base::Time(), base::Time(), base::Time(), false /*secure*/,
+      false /*httponly*/, CookieSameSite::NO_RESTRICTION,
+      COOKIE_PRIORITY_DEFAULT, std::nullopt /*partition_key*/, &status);
+
+  EXPECT_TRUE(cc.get());
+  EXPECT_TRUE(status.IsInclude());
+
+  cc = CanonicalCookie::CreateSanitizedCookie(
+      GURL("https://www.foo.com"), "A", "€", std::string(), "/foo",
+      base::Time(), base::Time(), base::Time(), false /*secure*/,
+      false /*httponly*/, CookieSameSite::NO_RESTRICTION,
+      COOKIE_PRIORITY_DEFAULT, std::nullopt /*partition_key*/, &status);
+  EXPECT_TRUE(cc.get());
+  EXPECT_TRUE(status.IsInclude());
+
+  cc = CanonicalCookie::CreateSanitizedCookie(
+      GURL("https://www.foo.com"), "€", "€", std::string(), "/foo",
+      base::Time(), base::Time(), base::Time(), false /*secure*/,
+      false /*httponly*/, CookieSameSite::NO_RESTRICTION,
+      COOKIE_PRIORITY_DEFAULT, std::nullopt /*partition_key*/, &status);
+  EXPECT_TRUE(cc.get());
+  EXPECT_TRUE(status.IsInclude());
+
+  // Since feature is not enabled this should be true.
+  EXPECT_TRUE(cc->IsCanonical());
+
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitWithFeatures(
+      {net::features::kDisallowNonAsciiCookies}, {});
+
+  // Now that feature is enabled this should return false.
+  EXPECT_FALSE(cc->IsCanonical());
+
+  // Valid cookie which should be included.
+  cc = CanonicalCookie::CreateSanitizedCookie(
+      GURL("https://www.foo.com"), "A", "B", std::string(), "/foo",
+      base::Time(), base::Time(), base::Time(), false /*secure*/,
+      false /*httponly*/, CookieSameSite::NO_RESTRICTION,
+      COOKIE_PRIORITY_DEFAULT, std::nullopt /*partition_key*/, &status);
+  EXPECT_TRUE(cc.get());
+  EXPECT_TRUE(status.IsInclude());
+  // Now that feature is enabled this cookie will be excluded.
+  cc = CanonicalCookie::CreateSanitizedCookie(
+      GURL("https://www.foo.com"), "€", "2", std::string(), "/foo",
+      base::Time(), base::Time(), base::Time(), false /*secure*/,
+      false /*httponly*/, CookieSameSite::NO_RESTRICTION,
+      COOKIE_PRIORITY_DEFAULT, std::nullopt /*partition_key*/, &status);
+  EXPECT_FALSE(cc.get());
+  EXPECT_TRUE(status.HasExclusionReason(
+      CookieInclusionStatus::EXCLUDE_DISALLOWED_CHARACTER));
+
+  // Now that feature is enabled this cookie will be excluded.
+  cc = CanonicalCookie::CreateSanitizedCookie(
+      GURL("https://www.foo.com"), "A", "€", std::string(), "/foo",
+      base::Time(), base::Time(), base::Time(), false /*secure*/,
+      false /*httponly*/, CookieSameSite::NO_RESTRICTION,
+      COOKIE_PRIORITY_DEFAULT, std::nullopt /*partition_key*/, &status);
+  EXPECT_FALSE(cc.get());
+  EXPECT_TRUE(status.HasExclusionReason(
+      CookieInclusionStatus::EXCLUDE_DISALLOWED_CHARACTER));
+
+  // Now that feature is enabled this cookie will be excluded.
+  cc = CanonicalCookie::CreateSanitizedCookie(
+      GURL("https://www.foo.com"), "€", "€", std::string(), "/foo",
+      base::Time(), base::Time(), base::Time(), false /*secure*/,
+      false /*httponly*/, CookieSameSite::NO_RESTRICTION,
+      COOKIE_PRIORITY_DEFAULT, std::nullopt /*partition_key*/, &status);
+  EXPECT_FALSE(cc.get());
+  EXPECT_TRUE(status.HasExclusionReason(
+      CookieInclusionStatus::EXCLUDE_DISALLOWED_CHARACTER));
+}
+
 // Test that a cookie string with an empty domain attribute generates a
 // canonical host cookie.
 TEST(CanonicalCookieTest, CreateHostCookieFromString) {

tools/metrics/histograms/metadata/cookie/histograms.xml

@@ -591,7 +591,9 @@ chromium-metrics-reviews@google.com.
     If a cookie's {CookieField} field contains a non-ASCII character.
     Informally, this indicates if the field contains a (non-ASCII) Unicode
     character. Logged whenever a cookie is created via
-    CanonicalCookie::Create().
+    CanonicalCookie::Create(). When kDisallowNonAsciiCookies is enabled and a
+    cookie's {CookieField} field contains a non-ASCII character, this metric
+    will not be recorded.
   </summary>
   <token key="CookieField">
     <variant name="Name"/>