Clean up conversions from CanonicalCookie::Domain() to domain (per RFC)

In CanonicalCookie, the string member |domain_| is a combination of the
cookie's domain (as defined in RFC 6265bis) and the negation of the
cookie's host-only-flag, which is indicated by the presence of a leading
dot. The Domain() accessor returns the |domain_| string whereas many
consumers actually want the RFC's definition of the cookie domain,
leading to many of them manually stripping off a leading dot.

This CL moves CanonicalCookie::DomainWithoutDot() from private to public
so that users can directly access the domain without worrying about how
it's internally represented, and organizes some functions in cookie_util
to avoid redundant string operations.

Bug: 1071291
Change-Id: If1db8ece3d436956696951ed4eafeb6fa3784bfd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2151310Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Reviewed-by: Maksim Orlovich <morlovich@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Commit-Queue: Lily Chen <chlily@chromium.org>
Cr-Commit-Position: refs/heads/master@{#760191}

chrome/browser/android/cookies/cookies_fetcher_util.cc

@@ -39,9 +39,6 @@ void OnCookiesFetchFinished(const net::CookieList& cookies) {
 
   int index = 0;
   for (auto i = cookies.cbegin(); i != cookies.cend(); ++i) {
-    std::string domain = i->Domain();
-    if (domain.length() > 1 && domain[0] == '.')
-      domain = domain.substr(1);
     ScopedJavaLocalRef<jobject> java_cookie = Java_CookiesFetcher_createCookie(
         env, base::android::ConvertUTF8ToJavaString(env, i->Name()),
         base::android::ConvertUTF8ToJavaString(env, i->Value()),

chrome/browser/browsing_data/cookies_tree_model.cc

@@ -45,6 +45,7 @@
 #include "extensions/buildflags/buildflags.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "net/cookies/canonical_cookie.h"
+#include "net/cookies/cookie_util.h"
 #include "net/url_request/url_request_context.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
@@ -1612,13 +1613,14 @@ void CookiesTreeModel::PopulateCookieInfoWithFilter(
   notifier->StartBatchUpdate();
   for (auto it = container->cookie_list_.begin();
        it != container->cookie_list_.end(); ++it) {
-    std::string domain = it->Domain();
-    if (domain.length() > 1 && domain[0] == '.')
-      domain = domain.substr(1);
-
     // Cookies ignore schemes, so group all HTTP and HTTPS cookies together.
-    GURL source(std::string(url::kHttpScheme) + url::kStandardSchemeSeparator +
-                domain + "/");
+    // TODO(crbug.com/1031721): This will not be true when Scheme-Bound Cookies
+    // is implemented. Investigate whether passing it->SourceScheme() instead of
+    // false is appropriate here.
+    GURL source = (it->Domain() == ".")
+                      ? GURL("http://./")
+                      : net::cookie_util::CookieOriginToURL(
+                            it->Domain(), false /* is_https */);
 
     if (filter.empty() || (CookieTreeHostNode::TitleForUrl(source)
                                .find(filter) != base::string16::npos)) {

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

@@ -161,14 +161,14 @@ void GetAllCookiesFromManager(
 }
 
 GURL GetURLFromCanonicalCookie(const net::CanonicalCookie& cookie) {
-  const std::string& domain_key = cookie.Domain();
-  const std::string scheme =
-      cookie.IsSecure() ? url::kHttpsScheme : url::kHttpScheme;
-  const std::string host =
-      base::StartsWith(domain_key, ".", base::CompareCase::SENSITIVE)
-          ? domain_key.substr(1)
-          : domain_key;
-  return GURL(scheme + url::kStandardSchemeSeparator + host + "/");
+  // This is only ever called for CanonicalCookies that have come from a
+  // CookieStore, which means they should not have an empty domain. Only file
+  // cookies are allowed to have empty domains, and those are only permitted on
+  // Android, and hopefully not for much longer (see crbug.com/582985).
+  DCHECK(!cookie.Domain().empty());
+
+  return net::cookie_util::CookieOriginToURL(cookie.Domain(),
+                                             cookie.IsSecure());
 }
 
 void AppendMatchingCookiesFromCookieListToVector(

components/browsing_data/content/local_shared_objects_container.cc

@@ -100,15 +100,11 @@ size_t LocalSharedObjectsContainer::GetObjectCountForDomain(
        it != origin_cookies_set_map.end(); ++it) {
     const canonical_cookie::CookieHashSet* cookie_list = it->second.get();
     for (const auto& cookie : *cookie_list) {
-      // Strip leading '.'s.
-      std::string cookie_domain = cookie.Domain();
-      if (cookie_domain[0] == '.')
-        cookie_domain = cookie_domain.substr(1);
       // The |domain_url| is only created in order to use the
       // SameDomainOrHost method below. It does not matter which scheme is
       // used as the scheme is ignored by the SameDomainOrHost method.
-      GURL domain_url(std::string(url::kHttpScheme) +
-                      url::kStandardSchemeSeparator + cookie_domain);
+      GURL domain_url = net::cookie_util::CookieOriginToURL(
+          cookie.Domain(), false /* is_https */);
 
       if (origin.SchemeIsHTTPOrHTTPS() && SameDomainOrHost(origin, domain_url))
         ++count;

net/cookies/canonical_cookie.cc

@@ -421,6 +421,10 @@ std::unique_ptr<CanonicalCookie> CanonicalCookie::CreateSanitizedCookie(
   return cc;
 }
 
+std::string CanonicalCookie::DomainWithoutDot() const {
+  return cookie_util::CookieDomainAsHost(domain_);
+}
+
 bool CanonicalCookie::IsEquivalentForSecureCookieMatching(
     const CanonicalCookie& ecc) const {
   return (name_ == ecc.Name() && (ecc.IsDomainMatch(DomainWithoutDot()) ||
@@ -923,12 +927,6 @@ bool CanonicalCookie::IsRecentlyCreated(base::TimeDelta age_threshold) const {
   return (base::Time::Now() - creation_date_) <= age_threshold;
 }
 
-std::string CanonicalCookie::DomainWithoutDot() const {
-  if (domain_.empty() || domain_[0] != '.')
-    return domain_;
-  return domain_.substr(1);
-}
-
 CanonicalCookie::CookieInclusionStatus::CookieInclusionStatus()
     : exclusion_reasons_(0u), warning_reasons_(0u) {}
 

net/cookies/canonical_cookie.h

@@ -106,6 +106,10 @@ class NET_EXPORT CanonicalCookie {
 
   const std::string& Name() const { return name_; }
   const std::string& Value() const { return value_; }
+  // We represent the cookie's host-only-flag as the absence of a leading dot in
+  // Domain(). See IsDomainCookie() and IsHostCookie() below.
+  // If you want the "cookie's domain" as described in RFC 6265bis, use
+  // DomainWithoutDot().
   const std::string& Domain() const { return domain_; }
   const std::string& Path() const { return path_; }
   const base::Time& CreationDate() const { return creation_date_; }
@@ -124,6 +128,10 @@ class NET_EXPORT CanonicalCookie {
     return !domain_.empty() && domain_[0] == '.'; }
   bool IsHostCookie() const { return !IsDomainCookie(); }
 
+  // Returns the cookie's domain, with the leading dot removed, if present.
+  // This corresponds to the "cookie's domain" as described in RFC 6265bis.
+  std::string DomainWithoutDot() const;
+
   bool IsExpired(const base::Time& current) const {
     return !expiry_date_.is_null() && current >= expiry_date_;
   }
@@ -304,9 +312,6 @@ class NET_EXPORT CanonicalCookie {
   // Returns whether the cookie was created at most |age_threshold| ago.
   bool IsRecentlyCreated(base::TimeDelta age_threshold) const;
 
-  // Returns the cookie's domain, with the leading dot removed, if present.
-  std::string DomainWithoutDot() const;
-
   std::string name_;
   std::string value_;
   std::string domain_;

net/cookies/cookie_deletion_info.cc

@@ -28,12 +28,8 @@ bool DomainMatchesDomains(const net::CanonicalCookie& cookie,
   // If the cookie's domain is is not parsed as belonging to a registry
   // (e.g. for IP addresses or internal hostnames) an empty string will be
   // returned.  In this case, use the domain in the cookie.
-  if (effective_domain.empty()) {
-    if (cookie.IsDomainCookie())
-      effective_domain = cookie.Domain().substr(1);
-    else
-      effective_domain = cookie.Domain();
-  }
+  if (effective_domain.empty())
+    effective_domain = cookie.DomainWithoutDot();
 
   return match_domains.count(effective_domain) != 0;
 }

net/cookies/cookie_monster.cc

@@ -1657,9 +1657,7 @@ std::string CookieMonster::GetKey(base::StringPiece domain) {
   if (effective_domain.empty())
     effective_domain = std::string(domain);
 
-  if (!effective_domain.empty() && effective_domain[0] == '.')
-    return effective_domain.substr(1);
-  return effective_domain;
+  return cookie_util::CookieDomainAsHost(effective_domain);
 }
 
 bool CookieMonster::HasCookieableScheme(const GURL& url) {

net/cookies/cookie_util.cc

@@ -21,6 +21,7 @@
 #include "net/base/url_util.h"
 #include "net/http/http_util.h"
 #include "url/gurl.h"
+#include "url/url_constants.h"
 
 namespace net {
 namespace cookie_util {
@@ -74,12 +75,6 @@ bool SaturatedTimeFromUTCExploded(const base::Time::Exploded& exploded,
   return false;
 }
 
-std::string CookieDomainAsHost(const std::string& cookie_domain) {
-  if (DomainIsHostOnly(cookie_domain))
-    return cookie_domain;
-  return cookie_domain.substr(1);
-}
-
 CookieOptions::SameSiteCookieContext::CrossSchemeness ComputeSchemeChange(
     const GURL& url,
     const SiteForCookies& site_for_cookies) {
@@ -130,6 +125,12 @@ bool DomainIsHostOnly(const std::string& domain_string) {
   return (domain_string.empty() || domain_string[0] != '.');
 }
 
+std::string CookieDomainAsHost(const std::string& cookie_domain) {
+  if (DomainIsHostOnly(cookie_domain))
+    return cookie_domain;
+  return cookie_domain.substr(1);
+}
+
 std::string GetEffectiveDomain(const std::string& scheme,
                                const std::string& host) {
   if (scheme == "http" || scheme == "https" || scheme == "ws" ||
@@ -139,9 +140,7 @@ std::string GetEffectiveDomain(const std::string& scheme,
         registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
   }
 
-  if (!DomainIsHostOnly(host))
-    return host.substr(1);
-  return host;
+  return CookieDomainAsHost(host);
 }
 
 bool GetCookieDomainWithString(const GURL& url,
@@ -331,18 +330,19 @@ GURL CookieOriginToURL(const std::string& domain, bool is_https) {
   if (domain.empty())
     return GURL();
 
-  const std::string scheme = is_https ? "https" : "http";
-  const std::string host = domain[0] == '.' ? domain.substr(1) : domain;
-  return GURL(scheme + "://" + host);
+  const std::string scheme = is_https ? url::kHttpsScheme : url::kHttpScheme;
+  return GURL(scheme + url::kStandardSchemeSeparator +
+              CookieDomainAsHost(domain) + "/");
 }
 
 GURL SimulatedCookieSource(const CanonicalCookie& cookie,
                            const std::string& source_scheme) {
-  if (cookie.Domain().empty() || source_scheme.empty())
+  // Note: cookie.DomainWithoutDot() could be empty for e.g. file cookies.
+  if (cookie.DomainWithoutDot().empty() || source_scheme.empty())
     return GURL();
 
-  return GURL(source_scheme + "://" + CookieDomainAsHost(cookie.Domain()) +
-              cookie.Path());
+  return GURL(source_scheme + url::kStandardSchemeSeparator +
+              cookie.DomainWithoutDot() + cookie.Path());
 }
 
 bool IsDomainMatch(const std::string& domain, const std::string& host) {

net/cookies/cookie_util.h

@@ -48,6 +48,13 @@ NET_EXPORT bool GetCookieDomainWithString(const GURL& url,
 // i.e. it doesn't begin with a leading '.' character.
 NET_EXPORT bool DomainIsHostOnly(const std::string& domain_string);
 
+// If |cookie_domain| is nonempty and starts with a "." character, this returns
+// the substring of |cookie_domain| without the leading dot. (Note only one
+// leading dot is stripped, if there are multiple.) Otherwise it returns
+// |cookie_domain|. This is useful for converting from CanonicalCookie's
+// representation of a cookie domain to the RFC's notion of a cookie's domain.
+NET_EXPORT std::string CookieDomainAsHost(const std::string& cookie_domain);
+
 // Parses the string with the cookie expiration time (very forgivingly).
 // Returns the "null" time on failure.
 //

net/cookies/test_cookie_access_delegate.cc

@@ -48,7 +48,7 @@ void TestCookieAccessDelegate::SetIgnoreSameSiteRestrictionsScheme(
 std::string TestCookieAccessDelegate::GetKeyForDomainValue(
     const std::string& domain) const {
   DCHECK(!domain.empty());
-  return domain[0] == '.' ? domain.substr(1) : domain;
+  return cookie_util::CookieDomainAsHost(domain);
 }
 
 }  // namespace net