Implement and test CanonicalCookie::IsCanonical()

net/cookies/canonical_cookie.cc

Index: net/cookies/canonical_cookie.cc
diff --git a/net/cookies/canonical_cookie.cc b/net/cookies/canonical_cookie.cc
index 6c81a55dc7df1d768618d9d76b338a0b0fd79974..95972f55805ae327d71c2079293b43cad8bd902e 100644
--- a/net/cookies/canonical_cookie.cc
+++ b/net/cookies/canonical_cookie.cc
@@ -50,10 +50,12 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "net/base/url_util.h"
 #include "net/cookies/cookie_util.h"
 #include "net/cookies/parsed_cookie.h"
 #include "url/gurl.h"
 #include "url/url_canon.h"
+#include "url/url_util.h"
 
 using base::Time;
 using base::TimeDelta;
@@ -228,21 +230,22 @@ std::unique_ptr<CanonicalCookie> CanonicalCookie::Create(
                                                          creation_time,
                                                          server_time);
 
-  CookiePrefix prefix = CanonicalCookie::GetCookiePrefix(parsed_cookie.Name());
-  bool is_cookie_valid =
-      CanonicalCookie::IsCookiePrefixValid(prefix, url, parsed_cookie);
-  CanonicalCookie::RecordCookiePrefixMetrics(prefix, is_cookie_valid);
+  CookiePrefix prefix = GetCookiePrefix(parsed_cookie.Name());
+  bool is_cookie_valid = IsCookiePrefixValid(prefix, url, parsed_cookie);
+  RecordCookiePrefixMetrics(prefix, is_cookie_valid);
   if (!is_cookie_valid) {
     VLOG(kVlogSetCookies)
         << "Create() failed because the cookie violated prefix rules.";
     return nullptr;
   }
 
-  return base::WrapUnique(new CanonicalCookie(
+  std::unique_ptr<CanonicalCookie> cc(base::MakeUnique<CanonicalCookie>(
       parsed_cookie.Name(), parsed_cookie.Value(), cookie_domain, cookie_path,
       creation_time, cookie_expires, creation_time, parsed_cookie.IsSecure(),
       parsed_cookie.IsHttpOnly(), parsed_cookie.SameSite(),
       parsed_cookie.Priority()));
+  DCHECK(cc->IsCanonical());
+  return cc;
 }
 
 bool CanonicalCookie::IsEquivalentForSecureCookieMatching(
@@ -399,6 +402,45 @@ bool CanonicalCookie::FullCompare(const CanonicalCookie& other) const {
   return Priority() < other.Priority();
 }
 
+bool CanonicalCookie::IsCanonical() const {
+  if (ParsedCookie::ParseTokenString(name_) != name_ ||
+      ParsedCookie::ParseValueString(value_) != value_ ||
+      ParsedCookie::ParseValueString(domain_) != domain_ ||
+      ParsedCookie::ParseValueString(path_) != path_ ||
+      !ParsedCookie::IsValidCookieAttributeValue(name_) ||
+      !ParsedCookie::IsValidCookieAttributeValue(value_) ||
+      !ParsedCookie::IsValidCookieAttributeValue(domain_) ||
+      !ParsedCookie::IsValidCookieAttributeValue(path_)) {
+    return false;
+  }
+
+  if (!last_access_date_.is_null() &&
+      (creation_date_.is_null() || creation_date_ > last_access_date_)) {

[mmenke, 2017/06/15 21:30:24]

Also, if there's a time change, creation date could be before
last_access_date....yea, I think it makes sense to remove it.

[Randy Smith (Not in Mondays), 2017/06/16 15:25:21]

Done.  I left the requirement that last_access_date_ be null if creation date
is.

+    return false;
+  }
+
+  url::CanonHostInfo canon_host_info;
+  std::string canonical_domain(CanonicalizeHost(domain_, &canon_host_info));
+  // TODO(rdsmith): This specifically allows for empty domains.  The spec
+  // suggests this is invalid (if a domain attribute is empty, the cookie's
+  // domain is set to the canonicalized request host; see
+  // https://tools.ietf.org/html/rfc6265#section-5.3).  However, it is
+  // needed for Chrome extension cookies.
+  // See http://crbug.com/730633 for more information.
+  if (canonical_domain != domain_)
+    return false;
+
+  if (path_.empty() || path_[0] != '/')
+    return false;
+
+  if (GetCookiePrefix(name_) == COOKIE_PREFIX_HOST &&

[mmenke, 2017/06/15 21:30:24]

Check if IsSecure(), and also check if IsSecure() if the prefix is
CanonicalCookie::COOKIE_PREFIX_SECURE?

[Randy Smith (Not in Mondays), 2017/06/16 15:25:21]

Whoops, didn't realize host cookies had to be secure.  Done.
Also good catch.  Done.

(I may have been over clever in how I wrote the tests--feel free to tell me to
break it out into clear, separate clauses.)

+      (path_ != "/" || domain_.empty() || domain_[0] == '.')) {
+    return false;
+  }
+
+  return true;
+}
+
 // static
 CanonicalCookie::CookiePrefix CanonicalCookie::GetCookiePrefix(
     const std::string& name) {