Implement and test CanonicalCookie::IsCanonical()

net/cookies/canonical_cookie.h

 // 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.
 
 #ifndef NET_COOKIES_CANONICAL_COOKIE_H_
 #define NET_COOKIES_CANONICAL_COOKIE_H_
 
 #include <memory>
 #include <string>
 #include <vector>
@@ skipping 126 matching lines @@
   // Returns true if the cookie is less than |other|, considering only name,
   // domain and path. In particular, two equivalent cookies (see IsEquivalent())
   // are identical for PartialCompare().
   bool PartialCompare(const CanonicalCookie& other) const;
 
   // Returns true if the cookie is less than |other|, considering all fields.
   // FullCompare() is consistent with PartialCompare(): cookies sorted using
   // FullCompare() are also sorted with respect to PartialCompare().
   bool FullCompare(const CanonicalCookie& other) const;
 
+  // Return whether this object is a valid CanonicalCookie().  Invalid
+  // cookies may be constructed by the detailed constructor.
+  bool IsCanonical() const;

[mmenke, 2017/05/26 17:02:17]

We should DCHECK on this at the end of CanonicalCookie::Create (Since it should
only be returning canonical cookies).  That should hopefully help us catch most
false negatives, though doesn't help much with potential false positives... 
Maybe we could create a fuzzer for that?  Call CanonicalCookie::Create, and if
it fails, manually construct a CanonicalCookie with the unsafe constructor that
looks much like what Create would have returned on success, and make sure it's
not canonical?

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

DCHECK added.  I agree that a fuzzer might be a way to catch false positives,
but that would be a fair amount of work.  How important do you think it is?

(I tried to add some tests for CC::Create paralleling the ones for
IsCanonical(), but the parsing of the cookie line defeated my attempts.)

[mmenke, 2017/06/08 18:00:23]

So I'm very shaky on the accuracy of this function.

I assume the intention is to validate things the network process receives, which
is why I want some way to automatically validate that this is actually correct.

My opinion is we either need to add automatic validation (DCHECKs, fuzzers, or
something), or I'm not going to be able to sign off on this without a fair bit
of digging.  At which point, I'll probably also once again suggest we also
validate it with DCHECKs, though I may have better informed suggestions on how
to do that.

+
  private:
   FRIEND_TEST_ALL_PREFIXES(CanonicalCookieTest, TestPrefixHistograms);
 
   // The special cookie prefixes as defined in
   // https://tools.ietf.org/html/draft-west-cookie-prefixes
   //
   // This enum is being histogrammed; do not reorder or remove values.
   enum CookiePrefix {
     COOKIE_PREFIX_NONE = 0,
     COOKIE_PREFIX_SECURE,
@@ skipping 28 matching lines @@
   bool httponly_;
   CookieSameSite same_site_;
   CookiePriority priority_;
 };
 
 typedef std::vector<CanonicalCookie> CookieList;
 
 }  // namespace net
 
 #endif  // NET_COOKIES_CANONICAL_COOKIE_H_