Reject cookies with empty names and values.

As discussed in [1], cookies with empty names and empty values should be
rejected. This patch removes the carveout made in https://crbug.com/601786,
and adjusts unittests accordingly.

This patch does not change the WPT expectations; we'll do that at the same
time we change the spec. In the meantime, we'll check in local expectations
matching the behavior we believe is correct.

[1]: https://github.com/httpwg/http-extensions/issues/159#issuecomment-569233866

Bug: 1037996, 601786
Change-Id: I53319cee385efff019b313479184236c53b1d783
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1982549Reviewed-by: Lily Chen <chlily@chromium.org>
Commit-Queue: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/master@{#729403}

net/cookies/cookie_store_unittest.h

@@ -779,31 +779,31 @@ TYPED_TEST_P(CookieStoreTest, EmptyKeyTest) {
   EXPECT_TRUE(this->SetCookie(cs, url1, "foo"));
   EXPECT_EQ("foo", this->GetCookies(cs, url1));
 
-  // Regression tests for https://crbug.com/601786
+  // Cookies with neither name nor value (e.g. `Set-Cookie: =`) are ignored.
   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));
+  EXPECT_FALSE(this->SetCookie(cs, url2, "\t"));
+  EXPECT_EQ("foo", 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));
+  EXPECT_FALSE(this->SetCookie(cs, url3, "="));
+  EXPECT_EQ("foo", 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));
+  EXPECT_FALSE(this->SetCookie(cs, url4, ""));
+  EXPECT_EQ("foo", 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));
+  EXPECT_FALSE(this->SetCookie(cs, url5, "; bar"));
+  EXPECT_EQ("foo", 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));
+  EXPECT_FALSE(this->SetCookie(cs, url6, " "));
+  EXPECT_EQ("foo", this->GetCookies(cs, url6));
 #endif
 }
 

net/cookies/parsed_cookie.cc

@@ -361,13 +361,9 @@ 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("", ""));
+  // Exit early for an empty cookie string.
+  if (it == end)
     return;
-  }
 
   for (int pair_num = 0; it != end; ++pair_num) {
     TokenValuePair pair;
@@ -412,9 +408,16 @@ void ParsedCookie::ParseTokenValuePairs(const std::string& cookie_line) {
     // OK, we're finished with a Token/Value.
     pair.second = std::string(value_start, value_end);
 
+    // Ignore cookies with neither name nor value.
+    if (pair_num == 0 && (pair.first.empty() && pair.second.empty())) {
+      pairs_.clear();
+      break;
+    }
+
     // From RFC2109: "Attributes (names) (attr) are case-insensitive."
     if (pair_num != 0)
       pair.first = base::ToLowerASCII(pair.first);
+
     // Ignore Set-Cookie directives contaning control characters. See
     // http://crbug.com/238041.
     if (!IsValidCookieAttributeValue(pair.first) ||

net/cookies/parsed_cookie_unittest.cc

@@ -18,28 +18,14 @@ TEST(ParsedCookieTest, TestBasic) {
   EXPECT_EQ("b", pc.Value());
 }
 
-// De facto standard behavior, per https://crbug.com/601786.
 TEST(ParsedCookieTest, TestEmpty) {
-  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());
+  const char* kTestCookieLines[]{"",    "     ", "=",     "=;",  " =;",
+                                 "= ;", " = ;",  ";",     " ;",  " ; ",
+                                 "\t",  "\t;",   "\t=\t", "\t=", "=\t"};
+
+  for (const char* test : kTestCookieLines) {
+    ParsedCookie pc(test);
+    EXPECT_FALSE(pc.IsValid());
   }
 }
 
@@ -284,13 +270,13 @@ TEST(ParsedCookieTest, SerializeCookieLine) {
 }
 
 TEST(ParsedCookieTest, SetNameAndValue) {
-  ParsedCookie empty((std::string()));
-  EXPECT_TRUE(empty.IsValid());
-  EXPECT_TRUE(empty.SetDomain("foobar.com"));
-  EXPECT_TRUE(empty.SetName("name"));
-  EXPECT_TRUE(empty.SetValue("value"));
-  EXPECT_EQ("name=value; domain=foobar.com", empty.ToCookieLine());
-  EXPECT_TRUE(empty.IsValid());
+  ParsedCookie cookie("a=b");
+  EXPECT_TRUE(cookie.IsValid());
+  EXPECT_TRUE(cookie.SetDomain("foobar.com"));
+  EXPECT_TRUE(cookie.SetName("name"));
+  EXPECT_TRUE(cookie.SetValue("value"));
+  EXPECT_EQ("name=value; domain=foobar.com", cookie.ToCookieLine());
+  EXPECT_TRUE(cookie.IsValid());
 
   // We don't test
   //   ParsedCookie invalid("@foo=bar");

third_party/blink/web_tests/external/wpt/cookies/http-state/general-tests-expected.txt

@@ -18,14 +18,14 @@ PASS 0016 - Keep non-alphabetic key order.
 PASS 0017 - Keep order if comma-separated.
 PASS 0018 - Ignore keys after semicolon.
 PASS 0019 - Ignore attributes after semicolon.
-FAIL 0020 - Ignore cookies without key and value. assert_equals: expected "a=b; c=d" but got "a=b; ; c=d"
+PASS 0020 - Ignore cookies without key and value.
 FAIL 0021 - Ignore cookie without key in all 'Set-Cookie'. assert_equals: expected "a=b; c=d" but got "a=b; x; c=d"
 PASS 0022 - Set cookie without value in all 'Set-Cookie'.
-PASS 0023 - Ignore cookies without '=' in all 'Set-Cookie'.
-PASS 0024 - Ignore malformed cookies in all 'Set-Cookie'.
-PASS 0025 - Ignore cookies with ';' in all 'Set-Cookie'.
-PASS 0026 - Ignore malformed cookies in all 'Set-Cookie' v2.
+FAIL 0023 - Ignore cookies without '=' in all 'Set-Cookie'. assert_equals: expected "" but got "foo"
+FAIL 0024 - Ignore malformed cookies in all 'Set-Cookie'. assert_equals: expected "" but got "foo"
+FAIL 0025 - Ignore cookies with ';' in all 'Set-Cookie'. assert_equals: expected "" but got "foo"
+FAIL 0026 - Ignore malformed cookies in all 'Set-Cookie' v2. assert_equals: expected "" but got "foo"
 FAIL 0027 - Ignore malformed cookies in all 'Set-Cookie' v3. assert_equals: expected "" but got "bar"
-FAIL 0028 - [INVALID EXPECTATION] Ignore malformed cookies in all 'Set-Cookie' v4. assert_equals: expected "Set-Cookie: foo\nSet-Cookie:" but got ""
+FAIL 0028 - Ignore malformed cookies in all 'Set-Cookie' v4. assert_equals: expected "" but got "foo"
 Harness: the test ran to completion.
 

third_party/blink/web_tests/external/wpt/cookies/http-state/general-tests.html

@@ -49,8 +49,7 @@
         {file: "0025", name: "Ignore cookies with ';' in all 'Set-Cookie'."},
         {file: "0026", name: "Ignore malformed cookies in all 'Set-Cookie' v2."},
         {file: "0027", name: "Ignore malformed cookies in all 'Set-Cookie' v3."},
-        // TODO(fhorschig): Ask about 0028's expectations ... should be empty?
-        {file: "0028", name: "[INVALID EXPECTATION] Ignore malformed cookies in all 'Set-Cookie' v4."},
+        {file: "0028", name: "Ignore malformed cookies in all 'Set-Cookie' v4."},
       ];
 
       for (const i in TEST_CASES) {

third_party/blink/web_tests/external/wpt/cookies/http-state/resources/cookie-http-state-template.js

@@ -39,7 +39,7 @@ function expireCookie(name, expiry_date, path) {
   name = name || "";
   expiry_date = expiry_date || "Thu, 01 Jan 1970 00:00:00 UTC";
   path = path || getLocalResourcesPath();
-  document.cookie = name + "=; expires=" + expiry_date + "; path=" + path + ";";
+  document.cookie = name + "=value; expires=" + expiry_date + "; path=" + path + ";";
 }
 
 /* Captures a snapshot of cookies with |parse| and allows to diff it with a

third_party/blink/web_tests/external/wpt/cookies/http-state/resources/test-files/0026-test

 Set-Cookie: foo
-Set-Cookie:
+Set-Cookie:    

third_party/blink/web_tests/external/wpt/cookies/http-state/resources/test-files/0028-expected

-Set-Cookie: foo
-Set-Cookie:

third_party/blink/web_tests/external/wpt/cookies/http-state/resources/test-files/0028-test

 Set-Cookie: foo
-Set-Cookie:
+Set-Cookie: 	

third_party/blink/web_tests/external/wpt/lint.whitelist

@@ -94,6 +94,7 @@ TRAILING WHITESPACE: server-timing/resources/parsing/*
 TRAILING WHITESPACE: webvtt/parsing/file-parsing/support/*.vtt
 TRAILING WHITESPACE: webvtt/parsing/file-parsing/tests/support/*.vtt
 TRAILING WHITESPACE: xhr/resources/headers-some-are-empty.asis
+TRAILING WHITESPACE: cookies/http-state/resources/test-files/*
 
 # Intentional use of print statements
 PRINT STATEMENT: dom/nodes/Document-createElement-namespace-tests/generate.py