Bug 1551729 - Revert bug 1548432 - Re-introducing the support for cookies without values, r=mayhemer a=jcristau
authorAndrea Marchesini <amarchesini@mozilla.com>
Wed, 22 May 2019 06:31:22 +0000
changeset 536403 acd3e9c548df552387c5d0c3c827a26d04045799
parent 536402 42530078a02802914ffd9bc5e1388c4d8c754cea
child 536404 37d3185821e914e32603fb808dbb58cbb861fb1a
push id2082
push userffxbld-merge
push dateMon, 01 Jul 2019 08:34:18 +0000
treeherdermozilla-release@2fb19d0466d2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmayhemer, jcristau
bugs1551729, 1548432
milestone68.0
Bug 1551729 - Revert bug 1548432 - Re-introducing the support for cookies without values, r=mayhemer a=jcristau 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
testing/web-platform/meta/cookies/http-state/name-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
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/meta/cookies/http-state/name-tests.html.ini
@@ -0,0 +1,21 @@
+[name-tests.html]
+  [name0025 - name0025]
+    expected: FAIL
+
+  [name0023 - name0023]
+    expected: FAIL
+
+  [name0032 - name0032]
+    expected: FAIL
+
+  [name0031 - name0031]
+    expected: FAIL
+
+  [name0033 - name0033]
+    expected: FAIL
+
+  [name0028 - name0028]
+    expected: FAIL
+
+  [name0017 - name0017]
+    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);