Use CanonicalCookie::FromStorage in cookie_manager_mojom_traits.cc.

This is part of a larger cleanup effort of the Chrome cookie codebase
with the long term goal of making the CanonicalCookie constructor
private.

The cookie manager code checks whether the serialized/deserealized
cookie is valid, and since this code is a security boundary we think
this check is worth keeping with test coverage.

In order to do so, we need to remove the DCHECK in

that relied on the check we are removing.

CanonicalCookie: :FromStorage and include new checks at the callsites
Bug: 1102874
Change-Id: I9c9436827d668042ba41229ce8819ef925cb5d6a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2490242Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Reviewed-by: Lily Chen <chlily@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Maksim Orlovich <morlovich@chromium.org>
Commit-Queue: Dylan Cutler <dylancutler@google.com>
Cr-Commit-Position: refs/heads/master@{#820347}

chrome/browser/android/cookies/cookies_fetcher_util.cc

@@ -96,8 +96,6 @@ static void JNI_CookiesFetcher_RestoreCookies(
   std::string domain_str(base::android::ConvertJavaStringToUTF8(env, domain));
   std::string path_str(base::android::ConvertJavaStringToUTF8(env, path));
 
-  // This factory method will DCHECK IsCanonical() to check if the cookie is
-  // valid.
   std::unique_ptr<net::CanonicalCookie> cookie =
       net::CanonicalCookie::FromStorage(
           base::android::ConvertJavaStringToUTF8(env, name),
@@ -112,6 +110,8 @@ static void JNI_CookiesFetcher_RestoreCookies(
           secure, httponly, static_cast<net::CookieSameSite>(same_site),
           static_cast<net::CookiePriority>(priority),
           static_cast<net::CookieSourceScheme>(source_scheme));
+  if (!cookie)
+    return;
 
   // Assume HTTPS - since the cookies are being restored from another store,
   // they have already gone through the strict secure check.

net/cookies/canonical_cookie.cc

@@ -521,7 +521,8 @@ std::unique_ptr<CanonicalCookie> CanonicalCookie::FromStorage(
   std::unique_ptr<CanonicalCookie> cc(std::make_unique<CanonicalCookie>(
       name, value, domain, path, creation, expiration, last_access, secure,
       httponly, same_site, priority, source_scheme));
-  DCHECK(cc->IsCanonical());
+  if (!cc->IsCanonical())
+    return nullptr;
   return cc;
 }
 

net/cookies/canonical_cookie.h

@@ -111,6 +111,8 @@ class NET_EXPORT CanonicalCookie {
   // that was already ingested into the cookie store.
   // This should NOT be used to create a new CanonicalCookie that was not
   // already in the store.
+  // Returns nullptr if the resulting cookie is not canonical,
+  // i.e. cc->IsCanonical() returns false.
   static std::unique_ptr<CanonicalCookie> FromStorage(
       const std::string& name,
       const std::string& value,

net/cookies/canonical_cookie_unittest.cc

@@ -2123,6 +2123,15 @@ TEST(CanonicalCookieTest, FromStorage) {
   EXPECT_EQ(COOKIE_PRIORITY_MEDIUM, cc->Priority());
   EXPECT_EQ(CookieSourceScheme::kSecure, cc->SourceScheme());
   EXPECT_FALSE(cc->IsDomainCookie());
+
+  // Should return nullptr when the cookie is not canonical.
+  // In this case the cookie is not canonical because its name attribute
+  // contains a newline character.
+  EXPECT_FALSE(CanonicalCookie::FromStorage(
+      "A\n", "B", "www.foo.com", "/bar", two_hours_ago, one_hour_from_now,
+      one_hour_ago, false /*secure*/, false /*httponly*/,
+      CookieSameSite::NO_RESTRICTION, COOKIE_PRIORITY_DEFAULT,
+      CookieSourceScheme::kSecure));
 }
 
 TEST(CanonicalCookieTest, IsSetPermittedInContext) {

services/network/public/cpp/cookie_manager_mojom_traits.cc

@@ -390,11 +390,14 @@ bool StructTraits<
   if (!cookie.ReadSourceScheme(&source_scheme))
     return false;
 
-  *out = net::CanonicalCookie(name, value, domain, path, creation_time,
-                              expiry_time, last_access_time, cookie.secure(),
-                              cookie.httponly(), site_restrictions, priority,
-                              source_scheme);
-  return out->IsCanonical();
+  auto cc = net::CanonicalCookie::FromStorage(
+      name, value, domain, path, creation_time, expiry_time, last_access_time,
+      cookie.secure(), cookie.httponly(), site_restrictions, priority,
+      source_scheme);
+  if (!cc)
+    return false;
+  *out = *cc;
+  return true;
 }
 
 bool StructTraits<network::mojom::CookieInclusionStatusDataView,

services/network/restricted_cookie_manager.cc

@@ -389,6 +389,7 @@ void RestrictedCookieManager::SetCanonicalCookie(
       cookie.Name(), cookie.Value(), cookie.Domain(), cookie.Path(), now,
       cookie.ExpiryDate(), now, cookie.IsSecure(), cookie.IsHttpOnly(),
       cookie.SameSite(), cookie.Priority(), source_scheme);
+  DCHECK(sanitized_cookie);
   net::CanonicalCookie cookie_copy = *sanitized_cookie;
 
   net::CookieOptions options =