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..d128e881c3035a0ec27bd21af12836748948e3ee 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,50 @@ 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_) ||

[mmenke, 2017/06/16 15:36:03]

Should we just rely on CanonicalizeHost() to do all the sanity checking on the
domain name?  Not sure if we also need the two domain checks here.  Also worth
noting that domain_ can be taken from the URL, instead of as part of a
parameter.

[Randy Smith (Not in Mondays), 2017/06/16 19:40:05]

Hmmm.   I think your second point is the key one, specifically that along the
CC::Create() path, we might end up with a domain that hasn't gone through the
above checks because it was never part of a cookie attribute value, so these
checks might mark as non-canonical a cookie produced by CC::Create().  Ok,
removing checks with a comment.

As per our offline conversation, I'm not going to worry about the implications
of this for paths--it's both more complicated, and not as important, since we
don't send paths back to the server.  

(For the record, if there isn't a path attribute, the path we set is from the
URL to the rightmost '/'; see CanonPathWithString() in canonical_cookie.cc.)

+      !ParsedCookie::IsValidCookieAttributeValue(path_)) {
+    return false;
+  }
+
+  if (!last_access_date_.is_null() && creation_date_.is_null())
+    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;
+
+  switch (GetCookiePrefix(name_)) {
+    case COOKIE_PREFIX_HOST:
+      if (path_ != "/" || domain_.empty() || domain_[0] == '.')
+        return false;
+    // Fall through; host cookies must also be secure.

[mmenke, 2017/06/16 15:36:03]

I suggest not falling through, and trusting in the optimizer.  I think falling
through is much more likely to lead to regressions on modification.

[Randy Smith (Not in Mondays), 2017/06/16 19:40:05]

Oh, you never let me have any fun.  Done.

(This comment should not be interpreted as in any way, shape, or form
*disagreeing* with you :-}.)

+    case COOKIE_PREFIX_SECURE:
+      if (!secure_)
+        return false;

[mmenke, 2017/06/16 15:36:03]

Add tests for this, for both SECURE and HOST cookies?

[Randy Smith (Not in Mondays), 2017/06/16 19:40:05]

Done.

+    // Fall through
+    default:;

[mmenke, 2017/06/16 15:36:03]

I don't think that semi-colon is needed?

[Randy Smith (Not in Mondays), 2017/06/16 19:40:05]

I put in a "break" for consistency (which you may consider worse :-}).

[mmenke, 2017/06/16 19:48:04]

I'm perfectly happy with a break.

+  }
+
+  return true;
+}
+
 // static
 CanonicalCookie::CookiePrefix CanonicalCookie::GetCookiePrefix(
     const std::string& name) {