Update cookie value and attribute setting behavior to match other UAs

This is an attempt to bring Chrome's cookie behavior in line with the
majority of other major UAs, and thus are de facto standards. The
specific updates are:
  1. An empty domain attribute value should be ignored as a
     non-attribute rather than setting an empty attribute value.
  2. An empty cookie line (or a cookie line with ignored characters)
     should be treated as setting an empty value for the empty-key.

BUG=601786

Review-Url: https://codereview.chromium.org/2246613003
Cr-Commit-Position: refs/heads/master@{#412608}

chrome/browser/browsing_data/browsing_data_cookie_helper_unittest.cc

@@ -286,10 +286,9 @@ TEST_F(BrowsingDataCookieHelperTest, CannedDomainCookie) {
   helper->AddChangedCookie(origin, origin, "A=1; Domain=.www.google.com",
                            net::CookieOptions());
   // Try adding invalid cookies that will be ignored.
-  helper->AddChangedCookie(origin, origin, std::string(), net::CookieOptions());
-  helper->AddChangedCookie(origin,
-                           origin,
-                           "C=bad guy; Domain=wrongdomain.com",
+  helper->AddChangedCookie(origin, origin, "C=not http; HttpOnly",
+                           net::CookieOptions());
+  helper->AddChangedCookie(origin, origin, "C=bad guy; Domain=wrongdomain.com",
                            net::CookieOptions());
 
   helper->StartFetching(

net/cookies/cookie_store_unittest.h

@@ -465,6 +465,46 @@ TYPED_TEST_P(CookieStoreTest, SetCookieWithDetailsAsync) {
   EXPECT_TRUE(++it == cookies.end());
 }
 
+// The iOS networking stack uses the iOS cookie parser, which we do not
+// control. While it is spec-compliant, that does not match the practical
+// behavior of most UAs in some cases, which we try to replicate. See
+// https://crbug.com/638389 for more information.
+TYPED_TEST_P(CookieStoreTest, EmptyKeyTest) {
+#if !defined(OS_IOS)
+  CookieStore* cs = this->GetCookieStore();
+
+  GURL url1("http://foo1.bar.com");
+  EXPECT_TRUE(this->SetCookie(cs, url1, "foo"));
+  EXPECT_EQ("foo", this->GetCookies(cs, url1));
+
+  // Regression tests for https://crbug.com/601786
+  GURL url2("http://foo2.bar.com");
+  EXPECT_TRUE(this->SetCookie(cs, url2, "foo"));
+  EXPECT_TRUE(this->SetCookie(cs, url2, "\t"));
+  EXPECT_EQ("", this->GetCookies(cs, url2));
+
+  GURL url3("http://foo3.bar.com");
+  EXPECT_TRUE(this->SetCookie(cs, url3, "foo"));
+  EXPECT_TRUE(this->SetCookie(cs, url3, "="));
+  EXPECT_EQ("", this->GetCookies(cs, url3));
+
+  GURL url4("http://foo4.bar.com");
+  EXPECT_TRUE(this->SetCookie(cs, url4, "foo"));
+  EXPECT_TRUE(this->SetCookie(cs, url4, ""));
+  EXPECT_EQ("", this->GetCookies(cs, url4));
+
+  GURL url5("http://foo5.bar.com");
+  EXPECT_TRUE(this->SetCookie(cs, url5, "foo"));
+  EXPECT_TRUE(this->SetCookie(cs, url5, "; bar"));
+  EXPECT_EQ("", this->GetCookies(cs, url5));
+
+  GURL url6("http://foo6.bar.com");
+  EXPECT_TRUE(this->SetCookie(cs, url6, "foo"));
+  EXPECT_TRUE(this->SetCookie(cs, url6, " "));
+  EXPECT_EQ("", this->GetCookies(cs, url6));
+#endif
+}
+
 TYPED_TEST_P(CookieStoreTest, DomainTest) {
   CookieStore* cs = this->GetCookieStore();
   EXPECT_TRUE(this->SetCookie(cs, this->http_www_google_.url(), "A=B"));
@@ -562,6 +602,9 @@ TYPED_TEST_P(CookieStoreTest, InvalidDomainTest) {
 
   // More specific sub-domain than allowed.
   EXPECT_FALSE(this->SetCookie(cs, url_foobar, "a=1; domain=.yo.foo.bar.com"));
+  // Regression test for https://crbug.com/601786
+  EXPECT_FALSE(
+      this->SetCookie(cs, url_foobar, "a=1; domain=.yo.foo.bar.com; domain="));
 
   EXPECT_FALSE(this->SetCookie(cs, url_foobar, "b=2; domain=.foo.com"));
   EXPECT_FALSE(this->SetCookie(cs, url_foobar, "c=3; domain=.bar.foo.com"));
@@ -1374,6 +1417,7 @@ TYPED_TEST_P(CookieStoreTest, DeleteSessionCookie) {
 
 REGISTER_TYPED_TEST_CASE_P(CookieStoreTest,
                            SetCookieWithDetailsAsync,
+                           EmptyKeyTest,
                            DomainTest,
                            DomainWithTrailingDotTest,
                            ValidSubdomainTest,

net/cookies/parsed_cookie.cc

@@ -195,7 +195,7 @@ CookiePriority ParsedCookie::Priority() const {
 }
 
 bool ParsedCookie::SetName(const std::string& name) {
-  if (!IsValidToken(name))
+  if (!name.empty() && !IsValidToken(name))
     return false;
   if (pairs_.empty())
     pairs_.push_back(std::make_pair("", ""));
@@ -363,12 +363,26 @@ void ParsedCookie::ParseTokenValuePairs(const std::string& cookie_line) {
   // Then we can log any unexpected terminators.
   std::string::const_iterator end = FindFirstTerminator(cookie_line);
 
+  // For an empty |cookie_line|, add an empty-key with an empty value, which
+  // has the effect of clearing any prior setting of the empty-key. This is done
+  // to match the behavior of other browsers. See https://crbug.com/601786.
+  if (it == end) {
+    pairs_.push_back(TokenValuePair("", ""));
+    return;
+  }
+
   for (int pair_num = 0; pair_num < kMaxPairs && it != end; ++pair_num) {
     TokenValuePair pair;
 
     std::string::const_iterator token_start, token_end;
-    if (!ParseToken(&it, end, &token_start, &token_end))
-      break;
+    if (!ParseToken(&it, end, &token_start, &token_end)) {
+      // Allow first token to be treated as empty-key if unparsable
+      if (pair_num != 0)
+        break;
+
+      // If parsing failed, start the value parsing at the very beginning.
+      token_start = start;
+    }
 
     if (it == end || *it != '=') {
       // We have a token-value, we didn't have any token name.
@@ -421,17 +435,11 @@ void ParsedCookie::ParseTokenValuePairs(const std::string& cookie_line) {
 }
 
 void ParsedCookie::SetupAttributes() {
-  // Ignore Set-Cookie directive where name and value are both empty.
-  if (pairs_[0].first.empty() && pairs_[0].second.empty()) {
-    pairs_.clear();
-    return;
-  }
-
   // We skip over the first token/value, the user supplied one.
   for (size_t i = 1; i < pairs_.size(); ++i) {
     if (pairs_[i].first == kPathTokenName) {
       path_index_ = i;
-    } else if (pairs_[i].first == kDomainTokenName) {
+    } else if (pairs_[i].first == kDomainTokenName && pairs_[i].second != "") {
       domain_index_ = i;
     } else if (pairs_[i].first == kExpiresTokenName) {
       expires_index_ = i;

net/cookies/parsed_cookie.h

@@ -84,7 +84,8 @@ class NET_EXPORT ParsedCookie {
   // returns as output arguments token_start and token_end to the start and end
   // positions of a cookie attribute token name parsed from the segment, and
   // updates the segment iterator to point to the next segment to be parsed.
-  // If no token is found, the function returns false.
+  // If no token is found, the function returns false and the segment iterator
+  // is set to end.
   static bool ParseToken(std::string::const_iterator* it,
                          const std::string::const_iterator& end,
                          std::string::const_iterator* token_start,

net/cookies/parsed_cookie_unittest.cc

@@ -18,19 +18,29 @@ TEST(ParsedCookieTest, TestBasic) {
   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());
-  ParsedCookie pc2("= ; path=/; secure;");
-  EXPECT_FALSE(pc2.IsValid());
-  ParsedCookie pc3(" =; path=/; secure;");
-  EXPECT_FALSE(pc3.IsValid());
-  ParsedCookie pc4(" = ; path=/; secure;");
-  EXPECT_FALSE(pc4.IsValid());
-  ParsedCookie pc5(" ; path=/; secure;");
-  EXPECT_FALSE(pc5.IsValid());
-  ParsedCookie pc6("; path=/; secure;");
-  EXPECT_FALSE(pc6.IsValid());
+  const struct {
+    const char* cookie;
+    const char* expected_path;
+    bool expect_secure;
+  } kTestCookieLines[]{{"", "", false},     {"     ", "", false},
+                       {"=;", "", false},   {"=; path=/; secure;", "/", true},
+                       {"= ;", "", false},  {"= ; path=/; secure;", "/", true},
+                       {" =;", "", false},  {" =; path=/; secure;", "/", true},
+                       {" = ;", "", false}, {" = ; path=/; secure;", "/", true},
+                       {" ;", "", false},   {" ; path=/; secure;", "/", true},
+                       {";", "", false},    {"; path=/; secure;", "/", true},
+                       {"\t;", "", false},  {"\t; path=/; secure;", "/", true}};
+
+  for (const auto& test : kTestCookieLines) {
+    ParsedCookie pc(test.cookie);
+    EXPECT_TRUE(pc.IsValid());
+    EXPECT_EQ("", pc.Name());
+    EXPECT_EQ("", pc.Value());
+    EXPECT_EQ(test.expected_path, pc.Path());
+    EXPECT_EQ(test.expect_secure, pc.IsSecure());
+  }
 }
 
 TEST(ParsedCookieTest, TestQuoted) {
@@ -221,11 +231,6 @@ TEST(ParsedCookieTest, TooManyPairs) {
 }
 
 // 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');
@@ -237,11 +242,6 @@ TEST(ParsedCookieTest, InvalidTooLong) {
   EXPECT_FALSE(pc2.IsValid());
 }
 
-TEST(ParsedCookieTest, InvalidEmpty) {
-  ParsedCookie pc((std::string()));
-  EXPECT_FALSE(pc.IsValid());
-}
-
 TEST(ParsedCookieTest, EmbeddedTerminator) {
   ParsedCookie pc1("AAA=BB\0ZYX");
   ParsedCookie pc2("AAA=BB\rZYX");
@@ -285,11 +285,11 @@ TEST(ParsedCookieTest, SerializeCookieLine) {
 
 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
@@ -307,10 +307,6 @@ TEST(ParsedCookieTest, SetNameAndValue) {
   EXPECT_EQ("name=value", pc.ToCookieLine());
   EXPECT_TRUE(pc.IsValid());
 
-  EXPECT_FALSE(pc.SetName(std::string()));
-  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());
@@ -320,6 +316,10 @@ TEST(ParsedCookieTest, SetNameAndValue) {
   EXPECT_TRUE(pc.IsValid());
 
   // Set valid name / value
+  EXPECT_TRUE(pc.SetName(std::string()));
+  EXPECT_EQ("=value", pc.ToCookieLine());
+  EXPECT_TRUE(pc.IsValid());
+
   EXPECT_TRUE(pc.SetName("test"));
   EXPECT_EQ("test=value", pc.ToCookieLine());
   EXPECT_TRUE(pc.IsValid());
@@ -415,6 +415,17 @@ TEST(ParsedCookieTest, SetAttributes) {
   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());