Bug 1551729 - Revert bug 1548432 - Re-introducing the support for cookies without values, r=mayhemer
☠☠ backed out by 086a7a5fa9eb ☠ ☠
authorAndrea Marchesini <amarchesini@mozilla.com>
Tue, 21 May 2019 08:57:21 +0000
changeset 474703 754426d70d2870cc8cdb4814f547b4e476648ff9
parent 474702 b2b3da42e524731c6f03655451445669a2501c5a
child 474704 9bcf8d6124584b76919e7c1018427e96d47029d1
push id113168
push userrmaries@mozilla.com
push dateTue, 21 May 2019 16:39:23 +0000
treeherdermozilla-inbound@3c0f78074b72 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmayhemer
bugs1551729, 1548432
milestone69.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1551729 - Revert bug 1548432 - Re-introducing the support for cookies without values, r=mayhemer This patch reverts the second part of bug 1548432. Differential Revision: https://phabricator.services.mozilla.com/D31267
browser/components/originattributes/test/browser/browser_favicon_firstParty.js
browser/components/originattributes/test/browser/browser_favicon_userContextId.js
browser/components/privatebrowsing/test/browser/browser_privatebrowsing_favicon.js
browser/components/sessionstore/test/browser_sessionStoreContainer.js
dom/browser-element/mochitest/file_browserElement_CookiesNotThirdParty.html
netwerk/cookie/nsCookieService.cpp
netwerk/cookie/nsCookieService.h
netwerk/test/TestCookie.cpp
testing/web-platform/meta/cookies/http-state/chromium-tests.html.ini
testing/web-platform/meta/cookies/http-state/general-tests.html.ini
toolkit/components/thumbnails/test/thumbnails_background.sjs
--- a/browser/components/originattributes/test/browser/browser_favicon_firstParty.js
+++ b/browser/components/originattributes/test/browser/browser_favicon_firstParty.js
@@ -178,18 +178,18 @@ async function assignCookiesUnderFirstPa
   });
 
   BrowserTestUtils.removeTab(tabInfo.tab);
 }
 
 async function generateCookies(aThirdParty) {
   // we generate two different cookies for two first party domains.
   let cookies = [];
-  cookies.push(Math.random().toString() + "=1");
-  cookies.push(Math.random().toString() + "=1");
+  cookies.push(Math.random().toString());
+  cookies.push(Math.random().toString());
 
   let firstSiteURL;
   let secondSiteURL;
 
   if (aThirdParty) {
     // Add cookies into the third party site with different first party domain.
     firstSiteURL = THIRD_PARTY_SITE;
     secondSiteURL = THIRD_PARTY_SITE;
--- a/browser/components/originattributes/test/browser/browser_favicon_userContextId.js
+++ b/browser/components/originattributes/test/browser/browser_favicon_userContextId.js
@@ -123,18 +123,18 @@ function waitOnFaviconLoaded(aFaviconURL
     (uri, attr, value, id) => attr === Ci.nsINavHistoryObserver.ATTRIBUTE_FAVICON &&
                               value === aFaviconURL,
     "history");
 }
 
 async function generateCookies(aHost) {
   // we generate two different cookies for two userContextIds.
   let cookies = [];
-  cookies.push(Math.random().toString() + "=1");
-  cookies.push(Math.random().toString() + "=1");
+  cookies.push(Math.random().toString());
+  cookies.push(Math.random().toString());
 
   // Then, we add cookies into the site for 'personal' and 'work'.
   let tabInfoA = await openTabInUserContext(aHost, USER_CONTEXT_ID_PERSONAL);
   let tabInfoB = await openTabInUserContext(aHost, USER_CONTEXT_ID_WORK);
 
   await ContentTask.spawn(tabInfoA.browser, cookies[0], async function(value) {
     content.document.cookie = value;
   });
--- a/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_favicon.js
+++ b/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_favicon.js
@@ -184,18 +184,18 @@ add_task(async function test_favicon_pri
 
   // Create a private browsing window.
   let privateWindow = await BrowserTestUtils.openNewBrowserWindow({ private: true });
   let pageURI = makeURI(TEST_PAGE);
 
   // Generate two random cookies for non-private window and private window
   // respectively.
   let cookies = [];
-  cookies.push(Math.random().toString() + "=1");
-  cookies.push(Math.random().toString() + "=1");
+  cookies.push(Math.random().toString());
+  cookies.push(Math.random().toString());
 
   // Open a tab in private window and add a cookie into it.
   await assignCookies(privateWindow.gBrowser, TEST_SITE, cookies[0]);
 
   // Open a tab in non-private window and add a cookie into it.
   await assignCookies(gBrowser, TEST_SITE, cookies[1]);
 
   // Add the observer earlier in case we don't capture events in time.
--- a/browser/components/sessionstore/test/browser_sessionStoreContainer.js
+++ b/browser/components/sessionstore/test/browser_sessionStoreContainer.js
@@ -109,17 +109,17 @@ add_task(async function test() {
     "set": [ [ "privacy.userContext.enabled", true ] ],
   });
 
   Services.cookies.removeAll();
 
   for (let userContextId of Object.keys(USER_CONTEXTS)) {
     // Load the page in 3 different contexts and set a cookie
     // which should only be visible in that context.
-    let cookie = USER_CONTEXTS[userContextId] + "=1";
+    let cookie = USER_CONTEXTS[userContextId];
 
     // Open our tab in the given user context.
     let { tab, browser } = await openTabInUserContext(userContextId);
 
     await Promise.all([
       waitForNewCookie(),
       ContentTask.spawn(browser, cookie,
         passedCookie => content.document.cookie = passedCookie),
--- a/dom/browser-element/mochitest/file_browserElement_CookiesNotThirdParty.html
+++ b/dom/browser-element/mochitest/file_browserElement_CookiesNotThirdParty.html
@@ -1,20 +1,20 @@
 <html>
 <body>
 file_browserElement_CookiesNotThirdParty.html
 
 <script type='text/javascript'>
 if (location.search != "?step=2") {
   // Step 1: Set a cookie.
-  document.cookie = "file_browserElement_CookiesNotThirdParty=1";
+  document.cookie = "file_browserElement_CookiesNotThirdParty";
   alert("next");
 } else {
   // Step 2: Read the cookie.
-  if (document.cookie == "file_browserElement_CookiesNotThirdParty=1") {
+  if (document.cookie == "file_browserElement_CookiesNotThirdParty") {
     alert("success: got the correct cookie");
   } else {
     alert('failure: got unexpected cookie: "' + document.cookie + '"');
   }
 
   alert("finish");
 }
 </script>
--- a/netwerk/cookie/nsCookieService.cpp
+++ b/netwerk/cookie/nsCookieService.cpp
@@ -3234,18 +3234,17 @@ bool nsCookieService::CanSetCookie(nsIUR
   aCookieAttributes.expiryTime = INT64_MAX;
 
   // aCookieHeader is an in/out param to point to the next cookie, if
   // there is one. Save the present value for logging purposes
   nsDependentCString savedCookieHeader(aCookieHeader);
 
   // newCookie says whether there are multiple cookies in the header;
   // so we can handle them separately.
-  bool discard = false;
-  bool newCookie = ParseAttributes(aCookieHeader, aCookieAttributes, discard);
+  bool newCookie = ParseAttributes(aCookieHeader, aCookieAttributes);
 
   // Collect telemetry on how often secure cookies are set from non-secure
   // origins, and vice-versa.
   //
   // 0 = nonsecure and "http:"
   // 1 = nonsecure and "https:"
   // 2 = secure and "http:"
   // 3 = secure and "https:"
@@ -3381,17 +3380,17 @@ bool nsCookieService::CanSetCookie(nsIUR
       if (NS_FAILED(rv) || isThirdParty) {
         COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, savedCookieHeader,
                           "failed the samesite tests");
         return newCookie;
       }
     }
   }
 
-  aSetCookie = !discard;
+  aSetCookie = true;
   return newCookie;
 }
 
 // processes a single cookie, and returns true if there are more cookies
 // to be processed
 bool nsCookieService::SetCookieInternal(nsIURI* aHostURI,
                                         const mozilla::net::nsCookieKey& aKey,
                                         bool aRequireHostMatch,
@@ -3782,18 +3781,17 @@ bool nsCookieService::GetTokenValue(nsAC
   }
   return false;
 }
 
 // Parses attributes from cookie header. expires/max-age attributes aren't
 // folded into the cookie struct here, because we don't know which one to use
 // until we've parsed the header.
 bool nsCookieService::ParseAttributes(nsDependentCString& aCookieHeader,
-                                      nsCookieAttributes& aCookieAttributes,
-                                      bool& aDiscard) {
+                                      nsCookieAttributes& aCookieAttributes) {
   static const char kPath[] = "path";
   static const char kDomain[] = "domain";
   static const char kExpires[] = "expires";
   static const char kMaxage[] = "max-age";
   static const char kSecure[] = "secure";
   static const char kHttpOnly[] = "httponly";
   static const char kSameSite[] = "samesite";
   static const char kSameSiteLax[] = "lax";
@@ -3803,48 +3801,39 @@ bool nsCookieService::ParseAttributes(ns
   nsACString::const_char_iterator cookieStart, cookieEnd;
   aCookieHeader.BeginReading(cookieStart);
   aCookieHeader.EndReading(cookieEnd);
 
   aCookieAttributes.isSecure = false;
   aCookieAttributes.isHttpOnly = false;
   aCookieAttributes.sameSite = nsICookie2::SAMESITE_UNSET;
 
-  aDiscard = false;
-
   nsDependentCSubstring tokenString(cookieStart, cookieStart);
   nsDependentCSubstring tokenValue(cookieStart, cookieStart);
   bool newCookie, equalsFound;
 
   // extract cookie <NAME> & <VALUE> (first attribute), and copy the strings.
   // if we find multiple cookies, return for processing
   // note: if there's no '=', we assume token is <VALUE>. this is required by
   //       some sites (see bug 169091).
   // XXX fix the parser to parse according to <VALUE> grammar for this case
   newCookie = GetTokenValue(cookieStart, cookieEnd, tokenString, tokenValue,
                             equalsFound);
   if (equalsFound) {
     aCookieAttributes.name = tokenString;
     aCookieAttributes.value = tokenValue;
-    if (aCookieAttributes.name.IsEmpty()) {
-      aDiscard = true;
-    }
   } else {
-    aDiscard = true;
+    aCookieAttributes.value = tokenString;
   }
 
   // extract remaining attributes
   while (cookieStart != cookieEnd && !newCookie) {
     newCookie = GetTokenValue(cookieStart, cookieEnd, tokenString, tokenValue,
                               equalsFound);
 
-    if (aDiscard) {
-      continue;
-    }
-
     if (!tokenValue.IsEmpty()) {
       tokenValue.BeginReading(tempBegin);
       tokenValue.EndReading(tempEnd);
     }
 
     // decide which attribute we have, and copy the string
     if (tokenString.LowerCaseEqualsLiteral(kPath))
       aCookieAttributes.path = tokenValue;
--- a/netwerk/cookie/nsCookieService.h
+++ b/netwerk/cookie/nsCookieService.h
@@ -336,18 +336,17 @@ class nsCookieService final : public nsI
   void UpdateCookieInList(nsCookie* aCookie, int64_t aLastAccessed,
                           mozIStorageBindingParamsArray* aParamsArray);
   static bool GetTokenValue(nsACString::const_char_iterator& aIter,
                             nsACString::const_char_iterator& aEndIter,
                             nsDependentCSubstring& aTokenString,
                             nsDependentCSubstring& aTokenValue,
                             bool& aEqualsFound);
   static bool ParseAttributes(nsDependentCString& aCookieHeader,
-                              nsCookieAttributes& aCookieAttributes,
-                              bool& aDiscard);
+                              nsCookieAttributes& aCookie);
   bool RequireThirdPartyCheck();
   static bool CheckDomain(nsCookieAttributes& aCookie, nsIURI* aHostURI,
                           const nsCString& aBaseDomain, bool aRequireHostMatch);
   static bool CheckPath(nsCookieAttributes& aCookie, nsIURI* aHostURI);
   static bool CheckPrefixes(nsCookieAttributes& aCookie, bool aSecureRequest);
   static bool GetExpiry(nsCookieAttributes& aCookie, int64_t aServerTime,
                         int64_t aCurrentTime, bool aFromHttp);
   void RemoveAllFromMemory();
--- a/netwerk/test/TestCookie.cpp
+++ b/netwerk/test/TestCookie.cpp
@@ -615,35 +615,35 @@ TEST(TestCookie, TestCookieMain)
   GetACookie(cookieService, "http://parser.test/", nullptr, cookie);
   EXPECT_TRUE(CheckResult(cookie.get(), MUST_BE_NULL));
   SetACookie(cookieService, "http://parser.test/", nullptr,
              "test=\"fubar! = foo;bar\\\";\" parser; domain=.parser.test; "
              "max-age=6\nfive; max-age=2.63,",
              nullptr);
   GetACookie(cookieService, "http://parser.test/", nullptr, cookie);
   EXPECT_TRUE(CheckResult(cookie.get(), MUST_CONTAIN, R"(test="fubar! = foo)"));
-  EXPECT_TRUE(CheckResult(cookie.get(), MUST_NOT_CONTAIN, "five"));
+  EXPECT_TRUE(CheckResult(cookie.get(), MUST_CONTAIN, "five"));
   SetACookie(cookieService, "http://parser.test/", nullptr,
              "test=kill; domain=.parser.test; max-age=0 \n five; max-age=0",
              nullptr);
   GetACookie(cookieService, "http://parser.test/", nullptr, cookie);
   EXPECT_TRUE(CheckResult(cookie.get(), MUST_BE_NULL));
 
   // test the handling of VALUE-only cookies (see bug 169091),
   // i.e. "six" should assume an empty NAME, which allows other VALUE-only
   // cookies to overwrite it
   SetACookie(cookieService, "http://parser.test/", nullptr, "six", nullptr);
   GetACookie(cookieService, "http://parser.test/", nullptr, cookie);
-  EXPECT_TRUE(CheckResult(cookie.get(), MUST_BE_NULL));
+  EXPECT_TRUE(CheckResult(cookie.get(), MUST_EQUAL, "six"));
   SetACookie(cookieService, "http://parser.test/", nullptr, "seven", nullptr);
   GetACookie(cookieService, "http://parser.test/", nullptr, cookie);
-  EXPECT_TRUE(CheckResult(cookie.get(), MUST_BE_NULL));
+  EXPECT_TRUE(CheckResult(cookie.get(), MUST_EQUAL, "seven"));
   SetACookie(cookieService, "http://parser.test/", nullptr, " =eight", nullptr);
   GetACookie(cookieService, "http://parser.test/", nullptr, cookie);
-  EXPECT_TRUE(CheckResult(cookie.get(), MUST_BE_NULL));
+  EXPECT_TRUE(CheckResult(cookie.get(), MUST_EQUAL, "eight"));
   SetACookie(cookieService, "http://parser.test/", nullptr, "test=six",
              nullptr);
   GetACookie(cookieService, "http://parser.test/", nullptr, cookie);
   EXPECT_TRUE(CheckResult(cookie.get(), MUST_CONTAIN, "test=six"));
 
   // *** path ordering tests
 
   // test that cookies are returned in path order - longest to shortest.
--- a/testing/web-platform/meta/cookies/http-state/chromium-tests.html.ini
+++ b/testing/web-platform/meta/cookies/http-state/chromium-tests.html.ini
@@ -1,7 +1,19 @@
 [chromium-tests.html]
+  [chromium0009 - chromium0009]
+    expected: FAIL
+
   [chromium0006 - chromium0006]
     expected: FAIL
 
+  [chromium0012 - chromium0012]
+    expected: FAIL
+
+  [disabled-chromium0022 - disabled-chromium0022]
+    expected: FAIL
+
+  [chromium0010 - chromium0010]
+    expected: FAIL
+
   [chromium0007 - chromium0007]
     expected: FAIL
 
--- a/testing/web-platform/meta/cookies/http-state/general-tests.html.ini
+++ b/testing/web-platform/meta/cookies/http-state/general-tests.html.ini
@@ -1,4 +1,18 @@
 [general-tests.html]
+  [0021 - Ignore cookie without key in all 'Set-Cookie'.]
+    expected: FAIL
+
+  [0023 - Ignore cookies without '=' in all 'Set-Cookie'.]
+    expected: FAIL
+
+  [0026 - Ignore malformed cookies in all 'Set-Cookie' v2.]
+    expected: FAIL
+
+  [0027 - Ignore malformed cookies in all 'Set-Cookie' v3.]
+    expected: FAIL
+
   [0028 - [INVALID EXPECTATION\] Ignore malformed cookies in all 'Set-Cookie' v4.]
     expected: FAIL
 
+  [0004 - Ignore cookie without key.]
+    expected: FAIL
--- a/toolkit/components/thumbnails/test/thumbnails_background.sjs
+++ b/toolkit/components/thumbnails/test/thumbnails_background.sjs
@@ -14,23 +14,23 @@ function handleRequest(req, resp) {
   let opts = {};
   try {
     opts = JSON.parse(decodeURIComponent(req.queryString));
   }
   catch (err) {}
 
   let setCookieScript = "";
   if (opts.setRedCookie) {
-    resp.setHeader("Set-Cookie", "red=1", false);
-    setCookieScript = '<script>document.cookie="red=1";</script>';
+    resp.setHeader("Set-Cookie", "red", false);
+    setCookieScript = '<script>document.cookie="red";</script>';
   }
 
   if (opts.setGreenCookie) {
-    resp.setHeader("Set-Cookie", "green=1", false);
-    setCookieScript = '<script>document.cookie="green=1";</script>';
+    resp.setHeader("Set-Cookie", "green", false);
+    setCookieScript = '<script>document.cookie="green";</script>';
   }
 
   if (opts.iframe) {
     setCookieScript += '<iframe src="' + opts.iframe + '" />';
   }
 
   if (opts.xhr) {
     setCookieScript += `
@@ -38,24 +38,24 @@ function handleRequest(req, resp) {
          var req = new XMLHttpRequest();
          req.open("GET", "${opts.xhr}", true);
          req.send();
       </script>
     `;
   }
 
   if (req.hasHeader("Cookie") &&
-      req.getHeader("Cookie").split(";").indexOf("red=1") >= 0) {
+      req.getHeader("Cookie").split(";").indexOf("red") >= 0) {
     resp.write('<html style="background: #f00;">' + setCookieScript + '</html>');
     resp.finish();
     return;
   }
 
   if (req.hasHeader("Cookie") &&
-      req.getHeader("Cookie").split(";").indexOf("green=1") >= 0) {
+      req.getHeader("Cookie").split(";").indexOf("green") >= 0) {
     resp.write('<html style="background: #0f0;">' + setCookieScript + '</html>');
     resp.finish();
     return;
   }
 
   if (opts.redirect) {
     resp.setHeader("Location", opts.redirect);
     resp.setStatusLine(null, 303, null);