Update cookie value and attribute setting behavior to match other UAs

net/cookies/parsed_cookie_unittest.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.
 
 #include <string>
 
 #include "net/cookies/cookie_constants.h"
 #include "net/cookies/parsed_cookie.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
 
 TEST(ParsedCookieTest, TestBasic) {
   ParsedCookie pc("a=b");
   EXPECT_TRUE(pc.IsValid());
   EXPECT_FALSE(pc.IsSecure());
   EXPECT_EQ("a", pc.Name());
   EXPECT_EQ("b", pc.Value());
 }
 
+// De facto standard behavior, per https://crbug.com/601786.
 TEST(ParsedCookieTest, TestEmpty) {
   ParsedCookie pc1("=; path=/; secure;");
-  EXPECT_FALSE(pc1.IsValid());
+  EXPECT_TRUE(pc1.IsValid());
   ParsedCookie pc2("= ; path=/; secure;");
-  EXPECT_FALSE(pc2.IsValid());
+  EXPECT_TRUE(pc2.IsValid());
   ParsedCookie pc3(" =; path=/; secure;");
-  EXPECT_FALSE(pc3.IsValid());
+  EXPECT_TRUE(pc3.IsValid());
   ParsedCookie pc4(" = ; path=/; secure;");
-  EXPECT_FALSE(pc4.IsValid());
+  EXPECT_TRUE(pc4.IsValid());
   ParsedCookie pc5(" ; path=/; secure;");
-  EXPECT_FALSE(pc5.IsValid());
+  EXPECT_TRUE(pc5.IsValid());
   ParsedCookie pc6("; path=/; secure;");
-  EXPECT_FALSE(pc6.IsValid());
+  EXPECT_TRUE(pc6.IsValid());
+  ParsedCookie pc7("\t; path=/; secure;");
+  EXPECT_TRUE(pc7.IsValid());

[mmenke, 2016/08/15 21:11:03]

We should check the actual value, too, right?  Also should test that they have
path and secure set (Except in the two cases I suggested adding)

[mmenke, 2016/08/15 21:11:03]

We should also check ";" and "=" (With no extra stuff on the end).

[mmenke, 2016/08/15 21:11:03]

optional: Suggest merging ValidOnlyWhitespace and ValidEmpty with this test,
too, since they're so closely related.

[jww, 2016/08/16 02:24:23]

Good calls all around. I've done all of your suggestions:
  * Added versions of all the tests without the attributes
  * Added value checking for each of them
  * Moved ValidOnlyWhitespace and ValidEmpty into TestEmpty

 }
 
 TEST(ParsedCookieTest, TestQuoted) {
   // These are some quoting cases which the major browsers all
   // handle differently.  I've tested Internet Explorer 6, Opera 9.6,
   // Firefox 3, and Safari Windows 3.2.1.  We originally tried to match
   // Firefox closely, however we now match Internet Explorer and Safari.
   const char* const values[] = {
       // Trailing whitespace after a quoted value.  The whitespace after
       // the quote is stripped in all browsers.
@@ skipping 170 matching lines @@
   ParsedCookie pc1("a=b;" + blankpairs + "secure");
   EXPECT_TRUE(pc1.IsValid());
   EXPECT_TRUE(pc1.IsSecure());
 
   ParsedCookie pc2("a=b;" + blankpairs + ";secure");
   EXPECT_TRUE(pc2.IsValid());
   EXPECT_FALSE(pc2.IsSecure());
 }
 
 // TODO(erikwright): some better test cases for invalid cookies.
-TEST(ParsedCookieTest, InvalidWhitespace) {
-  ParsedCookie pc("    ");
-  EXPECT_FALSE(pc.IsValid());
-}
-
 TEST(ParsedCookieTest, InvalidTooLong) {
   std::string maxstr;
   maxstr.resize(ParsedCookie::kMaxCookieSize, 'a');
 
   ParsedCookie pc1(maxstr);
   EXPECT_TRUE(pc1.IsValid());
 
   ParsedCookie pc2(maxstr + "A");
   EXPECT_FALSE(pc2.IsValid());
 }
 
-TEST(ParsedCookieTest, InvalidEmpty) {
+// De facto standard behavior, per https://crbug.com/601786.
+TEST(ParsedCookieTest, ValidOnlyWhitespace) {
+  ParsedCookie pc("    ");
+  EXPECT_TRUE(pc.IsValid());
+}
+
+// De facto standard behavior, per https://crbug.com/601786.
+TEST(ParsedCookieTest, ValidEmpty) {
   ParsedCookie pc((std::string()));
-  EXPECT_FALSE(pc.IsValid());
+  EXPECT_TRUE(pc.IsValid());
 }
 
 TEST(ParsedCookieTest, EmbeddedTerminator) {
   ParsedCookie pc1("AAA=BB\0ZYX");
   ParsedCookie pc2("AAA=BB\rZYX");
   ParsedCookie pc3("AAA=BB\nZYX");
   EXPECT_TRUE(pc1.IsValid());
   EXPECT_EQ("AAA", pc1.Name());
   EXPECT_EQ("BB", pc1.Value());
   EXPECT_TRUE(pc2.IsValid());
@@ skipping 25 matching lines @@
   const char output[] =
       "ANCUUID=zohNumRKgI0oxyhSsV3Z7D; "
       "expires=Sun, 18-Apr-2027 21:06:29 GMT; "
       "path=/; priority=low";
   ParsedCookie pc(input);
   EXPECT_EQ(output, pc.ToCookieLine());
 }
 
 TEST(ParsedCookieTest, SetNameAndValue) {
   ParsedCookie empty((std::string()));
-  EXPECT_FALSE(empty.IsValid());
-  EXPECT_FALSE(empty.SetDomain("foobar.com"));
+  EXPECT_TRUE(empty.IsValid());
+  EXPECT_TRUE(empty.SetDomain("foobar.com"));
   EXPECT_TRUE(empty.SetName("name"));
   EXPECT_TRUE(empty.SetValue("value"));
-  EXPECT_EQ("name=value", empty.ToCookieLine());
+  EXPECT_EQ("name=value; domain=foobar.com", empty.ToCookieLine());
   EXPECT_TRUE(empty.IsValid());
 
   // We don't test
   //   ParsedCookie invalid("@foo=bar");
   //   EXPECT_FALSE(invalid.IsValid());
   // here because we are slightly more tolerant to invalid cookie names and
   // values that are set by webservers. We only enforce a correct name and
   // value if set via SetName() and SetValue().
 
   ParsedCookie pc("name=value");
   EXPECT_TRUE(pc.IsValid());
 
   // Set invalid name / value.
   EXPECT_FALSE(pc.SetName("@foobar"));
   EXPECT_EQ("name=value", pc.ToCookieLine());
   EXPECT_TRUE(pc.IsValid());
 
   EXPECT_FALSE(pc.SetName(std::string()));

[mmenke, 2016/08/15 21:11:03]

Hrm...we allow unnamed cookies, but SetName doesn't work for them?  Guess that's
another issue.

[jww, 2016/08/16 02:24:23]

Good catch! Indeed, that doesn't make sense, so I've fixed that and updated the
tests.

   EXPECT_EQ("name=value", pc.ToCookieLine());
   EXPECT_TRUE(pc.IsValid());
 
   EXPECT_FALSE(pc.SetValue("foo bar"));
   EXPECT_EQ("name=value", pc.ToCookieLine());
   EXPECT_TRUE(pc.IsValid());
 
   EXPECT_FALSE(pc.SetValue("\"foobar"));
   EXPECT_EQ("name=value", pc.ToCookieLine());
   EXPECT_TRUE(pc.IsValid());
@@ skipping 87 matching lines @@
   EXPECT_FALSE(pc.HasDomain());
   EXPECT_FALSE(pc.HasPath());
   EXPECT_FALSE(pc.HasExpires());
   EXPECT_FALSE(pc.HasMaxAge());
   EXPECT_FALSE(pc.IsSecure());
   EXPECT_FALSE(pc.IsHttpOnly());
   EXPECT_EQ(CookieSameSite::NO_RESTRICTION, pc.SameSite());
   EXPECT_EQ("name2=value2", pc.ToCookieLine());
 }
 
+// Set the domain attribute twice in a cookie line. If the second attribute's
+// value is empty, it shoud be ignored.
+//
+// This is de facto standard behavior, per https://crbug.com/601786.
+TEST(ParsedCookieTest, MultipleDomainAttributes) {
+  ParsedCookie pc1("name=value; domain=foo.com; domain=bar.com");
+  EXPECT_EQ("bar.com", pc1.Domain());
+  ParsedCookie pc2("name=value; domain=foo.com; domain=");
+  EXPECT_EQ("foo.com", pc2.Domain());
+}
+
 TEST(ParsedCookieTest, SetPriority) {
   ParsedCookie pc("name=value");
   EXPECT_TRUE(pc.IsValid());
 
   EXPECT_EQ("name=value", pc.ToCookieLine());
   EXPECT_EQ(COOKIE_PRIORITY_DEFAULT, pc.Priority());
 
   // Test each priority, expect case-insensitive compare.
   EXPECT_TRUE(pc.SetPriority("high"));
   EXPECT_EQ("name=value; priority=high", pc.ToCookieLine());
@@ skipping 166 matching lines @@
   EXPECT_TRUE(pc5.IsValid());
   EXPECT_EQ(pc5_literal, pc5.ToCookieLine());
   EXPECT_TRUE(pc6.IsValid());
   EXPECT_EQ(pc6_literal, pc6.ToCookieLine());
   EXPECT_TRUE(pc7.IsValid());
   EXPECT_EQ(pc7_literal, pc7.ToCookieLine());
   EXPECT_TRUE(pc8.IsValid());
   EXPECT_EQ(pc8_literal, pc8.ToCookieLine());
 }
 }