Implement and test CanonicalCookie::IsCanonical()

net/cookies/canonical_cookie.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Portions of this code based on Mozilla:
 //   (netwerk/cookie/src/nsCookieService.cpp)
 /* ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
  *
  * The contents of this file are subject to the Mozilla Public License Version
@@ skipping 32 matching lines @@
  * ***** END LICENSE BLOCK ***** */
 
 #include "net/cookies/canonical_cookie.h"
 
 #include "base/format_macros.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #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;
 
 namespace net {
 
 namespace {
 
 const int kVlogSetCookies = 7;
 
@@ skipping 325 matching lines @@
 
   if (IsSecure() != other.IsSecure())
     return IsSecure();
 
   if (IsHttpOnly() != other.IsHttpOnly())
     return IsHttpOnly();
 
   return Priority() < other.Priority();
 }
 
+bool CanonicalCookie::IsCanonical() const {

[mmenke, 2017/05/25 19:49:56]

Is there a single method I should be comparing this logic to?

[Randy Smith (Not in Mondays), 2017/06/07 23:31:40]

I modeled it after SetCookieWithDetails minus the checks against URL.

+  if (name_.empty())

[mmenke, 2017/05/25 19:49:56]

Don't we support "Set-Cookie: =foo" and "Set-Cookie: foo", which both do the
same thing?

[mmenke, 2017/05/25 21:25:21]

Fun table that indicates we support both of these: 
http://inikulin.github.io/cookie-compat/

[Randy Smith (Not in Mondays), 2017/06/07 23:31:40]

Nice catch; my bad.  This snuck in a couple of CLs ago (see DCHECKs added in
CanonicalCookie::Create in https://codereview.chromium.org/2861063003) and
caused http://crbug.com/723734.  I've removed the check  above and add some
tests to make sure this remains true, at least until we explicitly try to change
the behavior.

+    return false;
+
+  if (ParsedCookie::ParseTokenString(name_) != name_ ||
+      ParsedCookie::ParseValueString(value_) != value_ ||
+      ParsedCookie::ParseValueString(domain_) != domain_ ||
+      ParsedCookie::ParseValueString(path_) != path_) {
+    return false;
+  }
+
+  url::CanonHostInfo ignored;
+  if (!url::HostIsIPAddress(domain_) &&
+      CanonicalizeHost(domain_, &ignored).empty()) {

[mmenke, 2017/05/25 19:49:56]

What if CanonicalizeHost(domain_) != domain_?

[Randy Smith (Not in Mondays), 2017/06/07 23:31:40]

Also note that the DCHECK(IsCanonical()) you suggested for CC::Create turned up
the fact that we were (in the case of strange schemes, specifically "foo:" in
the tests) creating cookies with empty domains.  I went back and read
SetCookieWithDetails() more carefully and realized that it actually *allowed*
empty domains despite the line "if (domain.empty()) return false;" part way
through :-J.  I've filed http://crbug.com/730633 to track the WTF aspects of
this, and modified the logic above to allow empty domains.

[Randy Smith (Not in Mondays), 2017/06/07 23:31:40]

Oh, tricky.  So as noted above I'm copying this logic from SetCookieWithDetails
and subtracting out the verification against the URL.  SetCookieWithDetails (in
GetCookieDomainWithSring()) has the above check, and then just adopts the domain
returned by CanonicalizeHost() (prefixed with a "." if is isn't already there
:-} :-J).  I think modifying the incoming domain is incompatible with the sense
of the CC constructor API.  So I think I agree with you that
CanonicalizeHost(domain_) != domain_ should be a failure from this function.  
But that's depending on a property of CanonicalizeHost() we haven't really
depended on before (loosely its idempotence).  I think I'll put that in, and
presume that if there's a problem it'll show up in testing, but if you think of
any gotchas in this space, please raise a flag.

Done.

+    return false;
+  }
+
+  if (path_[0] != '/')
+    return false;

[mmenke, 2017/05/25 19:49:56]

Do we always set path to be non-empty, even when none is provided?

Regardless, this should at least be (path_.empty() || path_[0] != '/')

[Randy Smith (Not in Mondays), 2017/06/07 23:31:40]

Yes, at least at a couple of places in the stack--see
CanonicalCookie::CanonPathWithString.  
Right!  Done.

+
+  return true;
+}
+
 // static
 CanonicalCookie::CookiePrefix CanonicalCookie::GetCookiePrefix(
     const std::string& name) {
   const char kSecurePrefix[] = "__Secure-";
   const char kHostPrefix[] = "__Host-";
   if (base::StartsWith(name, kSecurePrefix, base::CompareCase::SENSITIVE))
     return CanonicalCookie::COOKIE_PREFIX_SECURE;
   if (base::StartsWith(name, kHostPrefix, base::CompareCase::SENSITIVE))
     return CanonicalCookie::COOKIE_PREFIX_HOST;
   return CanonicalCookie::COOKIE_PREFIX_NONE;
@@ skipping 30 matching lines @@
   return true;
 }
 
 std::string CanonicalCookie::DomainWithoutDot() const {
   if (domain_.empty() || domain_[0] != '.')
     return domain_;
   return domain_.substr(1);
 }
 
 }  // namespace net