Bug 1752475 - Change nsICookie/CookieStruct to implicitly flip SameSite in getter func r=dveditz,dragana,freddyb

Given that we only support samesite lax/strict/none in our storage schema,
it's useful to introduce a default value, as required by the spec.
However, that would it hard to distinguish between none/lax when we switch
the default.
So, instead of doing that we use the peculiarities of our current schema
to our advantage: There's a "sameSite" attribute and a "rawSameSite"
attribute, where the latter is the literal value we received from the
server. With this patch, we'll interpret the "sameSite" attribute
based on the laxByDefault pref. This also has the advantage that various
front-end code (e.g., in DevTools) is always reading the "sameSite"
value of nsICookies.

Differential Revision: https://phabricator.services.mozilla.com/D137460

devtools/client/webconsole/test/browser/browser_webconsole_warning_group_cookies.js

@@ -38,10 +38,10 @@ add_task(async function testSameSiteCookieMessage() {
       groupLabel:
         "Some cookies are misusing the recommended “SameSite“ attribute",
       message1:
-        "Cookie “a” will be soon rejected because it has the “SameSite” attribute set to “None” or an invalid value, without the “secure” attribute.",
+        "Cookie “a” does not have a proper “SameSite” attribute value. Soon, cookies without the “SameSite” attribute or with an invalid value will be treated as “Lax”. This means that the cookie will no longer be sent in third-party contexts. If your application depends on this cookie being available in such contexts, please add the “SameSite=None“ attribute to it. To know more about the “SameSite“ attribute, read https://developer.mozilla.org/docs/Web/HTTP/Headers/Set-Cookie/SameSite",
       typeMessage1: ".warn",
       message2:
-        "Cookie “b” will be soon rejected because it has the “SameSite” attribute set to “None” or an invalid value, without the “secure” attribute.",
+        "Cookie “b” does not have a proper “SameSite” attribute value. Soon, cookies without the “SameSite” attribute or with an invalid value will be treated as “Lax”. This means that the cookie will no longer be sent in third-party contexts. If your application depends on this cookie being available in such contexts, please add the “SameSite=None“ attribute to it. To know more about the “SameSite“ attribute, read https://developer.mozilla.org/docs/Web/HTTP/Headers/Set-Cookie/SameSite",
     },
   ];
 

dom/security/test/https-first/test_toplevel_cookies.html

@@ -108,8 +108,7 @@ async function runTest() {
 
 SpecialPowers.pushPrefEnv({ set: [
     ["dom.security.https_first", true],
-    // Bug 1617611: Fix all the tests broken by "cookies SameSite=lax by default"
-    ["network.cookie.sameSite.laxByDefault", false],
+    ["network.cookie.sameSite.noneRequiresSecure", false],
   ]}, runTest);
 
 </script>

modules/libpref/init/StaticPrefList.yaml

@@ -9844,7 +9844,7 @@
   mirror: always
 
 - name: network.cookie.sameSite.laxByDefault
-  type: bool
+  type: RelaxedAtomicBool
   value: @IS_EARLY_BETA_OR_EARLIER@
   mirror: always
 

netwerk/cookie/Cookie.cpp

@@ -4,6 +4,7 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "Cookie.h"
+#include "CookieStorage.h"
 #include "mozilla/Encoding.h"
 #include "mozilla/dom/ToJSValue.h"
 #include "mozilla/StaticPrefs_network.h"
@@ -56,14 +57,10 @@ already_AddRefed<Cookie> Cookie::Create(
     gLastCreationTime = cookie->mData.creationTime();
   }
 
-  // If sameSite is not a sensible value, assume strict
-  if (cookie->mData.sameSite() < 0 ||
-      cookie->mData.sameSite() > nsICookie::SAMESITE_STRICT) {
-    cookie->mData.sameSite() = nsICookie::SAMESITE_STRICT;
-  }
-
-  // If rawSameSite is not a sensible value, assume equal to sameSite.
-  if (!Cookie::ValidateRawSame(cookie->mData)) {
+  // If sameSite/rawSameSite values aren't sensible reset to Default
+  // cf. 5.4.7 in draft-ietf-httpbis-rfc6265bis-09
+  if (!Cookie::ValidateSameSite(cookie->mData)) {
+    cookie->mData.sameSite() = nsICookie::SAMESITE_LAX;
     cookie->mData.rawSameSite() = nsICookie::SAMESITE_NONE;
   }
 
@@ -204,9 +201,16 @@ Cookie::GetExpires(uint64_t* aExpires) {
 }
 
 // static
-bool Cookie::ValidateRawSame(const CookieStruct& aCookieData) {
-  return aCookieData.rawSameSite() == aCookieData.sameSite() ||
-         aCookieData.rawSameSite() == nsICookie::SAMESITE_NONE;
+bool Cookie::ValidateSameSite(const CookieStruct& aCookieData) {
+  // For proper migration towards a laxByDefault world,
+  // sameSite is initialized to LAX even though the server
+  // has never sent it.
+  if (aCookieData.rawSameSite() == aCookieData.sameSite()) {
+    return aCookieData.rawSameSite() >= nsICookie::SAMESITE_NONE &&
+           aCookieData.rawSameSite() <= nsICookie::SAMESITE_STRICT;
+  }
+  return aCookieData.rawSameSite() == nsICookie::SAMESITE_NONE &&
+         aCookieData.sameSite() == nsICookie::SAMESITE_LAX;
 }
 
 already_AddRefed<Cookie> Cookie::Clone() const {

netwerk/cookie/Cookie.h

@@ -46,7 +46,7 @@ class Cookie final : public nsICookie {
 
  public:
   // Returns false if rawSameSite has an invalid value, compared to sameSite.
-  static bool ValidateRawSame(const CookieStruct& aCookieData);
+  static bool ValidateSameSite(const CookieStruct& aCookieData);
 
   // Generate a unique and monotonically increasing creation time. See comment
   // in Cookie.cpp.

netwerk/cookie/CookieService.cpp

@@ -1194,7 +1194,13 @@ bool CookieService::CanSetCookie(
 
   // If the new cookie is same-site but in a cross site context,
   // browser must ignore the cookie.
-  if ((aCookieData.sameSite() != nsICookie::SAMESITE_NONE) &&
+  bool laxByDefault =
+      StaticPrefs::network_cookie_sameSite_laxByDefault() &&
+      !nsContentUtils::IsURIInPrefList(
+          aHostURI, "network.cookie.sameSite.laxByDefault.disabledHosts");
+  auto effectiveSameSite =
+      laxByDefault ? aCookieData.sameSite() : aCookieData.rawSameSite();
+  if ((effectiveSameSite != nsICookie::SAMESITE_NONE) &&
       aIsForeignAndNotAddon) {
     COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, savedCookieHeader,
                       "failed the samesite tests");
@@ -1363,14 +1369,17 @@ bool CookieService::GetTokenValue(nsACString::const_char_iterator& aIter,
   return false;
 }
 
-static inline void SetSameSiteDefaultAttribute(CookieStruct& aCookieData,
-                                               bool laxByDefault) {
+static inline void SetSameSiteAttributeDefault(CookieStruct& aCookieData) {
+  // Set cookie with SameSite attribute that is treated as Default
+  // and doesn't requires changing the DB schema.
+  aCookieData.sameSite() = nsICookie::SAMESITE_LAX;
   aCookieData.rawSameSite() = nsICookie::SAMESITE_NONE;
-  if (laxByDefault) {
-    aCookieData.sameSite() = nsICookie::SAMESITE_LAX;
-  } else {
-    aCookieData.sameSite() = nsICookie::SAMESITE_NONE;
-  }
+}
+
+static inline void SetSameSiteAttribute(CookieStruct& aCookieData,
+                                        int32_t aValue) {
+  aCookieData.sameSite() = aValue;
+  aCookieData.rawSameSite() = aValue;
 }
 
 // Parses attributes from cookie header. expires/max-age attributes aren't
@@ -1403,11 +1412,7 @@ bool CookieService::ParseAttributes(nsIConsoleReportCollector* aCRC,
   aCookieData.isSecure() = false;
   aCookieData.isHttpOnly() = false;
 
-  bool laxByDefault =
-      StaticPrefs::network_cookie_sameSite_laxByDefault() &&
-      !nsContentUtils::IsURIInPrefList(
-          aHostURI, "network.cookie.sameSite.laxByDefault.disabledHosts");
-  SetSameSiteDefaultAttribute(aCookieData, laxByDefault);
+  SetSameSiteAttributeDefault(aCookieData);
 
   nsDependentCSubstring tokenString(cookieStart, cookieStart);
   nsDependentCSubstring tokenValue(cookieStart, cookieStart);
@@ -1457,17 +1462,14 @@ bool CookieService::ParseAttributes(nsIConsoleReportCollector* aCRC,
 
     } else if (tokenString.LowerCaseEqualsLiteral(kSameSite)) {
       if (tokenValue.LowerCaseEqualsLiteral(kSameSiteLax)) {
-        aCookieData.sameSite() = nsICookie::SAMESITE_LAX;
-        aCookieData.rawSameSite() = nsICookie::SAMESITE_LAX;
+        SetSameSiteAttribute(aCookieData, nsICookie::SAMESITE_LAX);
       } else if (tokenValue.LowerCaseEqualsLiteral(kSameSiteStrict)) {
-        aCookieData.sameSite() = nsICookie::SAMESITE_STRICT;
-        aCookieData.rawSameSite() = nsICookie::SAMESITE_STRICT;
+        SetSameSiteAttribute(aCookieData, nsICookie::SAMESITE_STRICT);
       } else if (tokenValue.LowerCaseEqualsLiteral(kSameSiteNone)) {
-        aCookieData.sameSite() = nsICookie::SAMESITE_NONE;
-        aCookieData.rawSameSite() = nsICookie::SAMESITE_NONE;
+        SetSameSiteAttribute(aCookieData, nsICookie::SAMESITE_NONE);
       } else {
-        // Reset to defaults if unknown token value (see Bug 1682450)
-        SetSameSiteDefaultAttribute(aCookieData, laxByDefault);
+        // Reset to Default if unknown token value (see Bug 1682450)
+        SetSameSiteAttributeDefault(aCookieData);
         CookieLogging::LogMessageToConsole(
             aCRC, aHostURI, nsIScriptError::infoFlag, CONSOLE_SAMESITE_CATEGORY,
             "CookieSameSiteValueInvalid2"_ns,
@@ -1479,29 +1481,32 @@ bool CookieService::ParseAttributes(nsIConsoleReportCollector* aCRC,
   // re-assign aCookieHeader, in case we need to process another cookie
   aCookieHeader.Assign(Substring(cookieStart, cookieEnd));
 
-  // If same-site is set to 'none' but this is not a secure context, let's abort
-  // the parsing.
+  // If same-site is explicitly set to 'none' but this is not a secure context,
+  // let's abort the parsing.
   if (!aCookieData.isSecure() &&
       aCookieData.sameSite() == nsICookie::SAMESITE_NONE) {
-    if (laxByDefault &&
-        StaticPrefs::network_cookie_sameSite_noneRequiresSecure()) {
+    if (StaticPrefs::network_cookie_sameSite_noneRequiresSecure()) {
       CookieLogging::LogMessageToConsole(
-          aCRC, aHostURI, nsIScriptError::infoFlag, CONSOLE_SAMESITE_CATEGORY,
+          aCRC, aHostURI, nsIScriptError::errorFlag, CONSOLE_SAMESITE_CATEGORY,
           "CookieRejectedNonRequiresSecure2"_ns,
           AutoTArray<nsString, 1>{NS_ConvertUTF8toUTF16(aCookieData.name())});
       return newCookie;
     }
 
-    // if SameSite=Lax by default is disabled, we want to warn the user.
+    // Still warn about the missing Secure attribute when not enforcing.
     CookieLogging::LogMessageToConsole(
         aCRC, aHostURI, nsIScriptError::warningFlag, CONSOLE_SAMESITE_CATEGORY,
-        "CookieRejectedNonRequiresSecureForBeta2"_ns,
+        "CookieRejectedNonRequiresSecureForBeta3"_ns,
         AutoTArray<nsString, 2>{NS_ConvertUTF8toUTF16(aCookieData.name()),
                                 SAMESITE_MDN_URL});
   }
 
   if (aCookieData.rawSameSite() == nsICookie::SAMESITE_NONE &&
       aCookieData.sameSite() == nsICookie::SAMESITE_LAX) {
+    bool laxByDefault =
+        StaticPrefs::network_cookie_sameSite_laxByDefault() &&
+        !nsContentUtils::IsURIInPrefList(
+            aHostURI, "network.cookie.sameSite.laxByDefault.disabledHosts");
     if (laxByDefault) {
       CookieLogging::LogMessageToConsole(
           aCRC, aHostURI, nsIScriptError::infoFlag, CONSOLE_SAMESITE_CATEGORY,
@@ -1519,7 +1524,7 @@ bool CookieService::ParseAttributes(nsIConsoleReportCollector* aCRC,
   // Cookie accepted.
   aAcceptedByParser = true;
 
-  MOZ_ASSERT(Cookie::ValidateRawSame(aCookieData));
+  MOZ_ASSERT(Cookie::ValidateSameSite(aCookieData));
   return newCookie;
 }
 

netwerk/cookie/CookieServiceChild.cpp

@@ -288,6 +288,7 @@ void CookieServiceChild::RecordDocumentCookie(Cookie* aCookie,
           cookie->Expiry() == aCookie->Expiry() &&
           cookie->IsSecure() == aCookie->IsSecure() &&
           cookie->SameSite() == aCookie->SameSite() &&
+          cookie->RawSameSite() == aCookie->RawSameSite() &&
           cookie->IsSession() == aCookie->IsSession() &&
           cookie->IsHttpOnly() == aCookie->IsHttpOnly()) {
         cookie->SetLastAccessed(aCookie->LastAccessed());

netwerk/cookie/CookieStorage.cpp

@@ -495,6 +495,7 @@ void CookieStorage::AddCookie(nsIConsoleReportCollector* aCRC,
           oldCookie->IsSession() == aCookie->IsSession() &&
           oldCookie->IsHttpOnly() == aCookie->IsHttpOnly() &&
           oldCookie->SameSite() == aCookie->SameSite() &&
+          oldCookie->RawSameSite() == aCookie->RawSameSite() &&
           oldCookie->SchemeMap() == aCookie->SchemeMap() &&
           // We don't want to perform this optimization if the cookie is
           // considered stale, since in this case we would need to update the

netwerk/cookie/test/unit/test_getCookieSince.js

@@ -5,7 +5,7 @@ const cs = Cc["@mozilla.org/cookieService;1"].getService(Ci.nsICookieService);
 const cm = cs.QueryInterface(Ci.nsICookieManager);
 
 function setCookie(name, url) {
-  let value = `${name}=${Math.random()}; Path=/; Max-Age=1000; sameSite=none`;
+  let value = `${name}=${Math.random()}; Path=/; Max-Age=1000; sameSite=none; Secure`;
   info(`Setting cookie ${value} for ${url.spec}`);
 
   let channel = NetUtil.newChannel({
@@ -37,9 +37,6 @@ add_task(async function() {
     true
   );
 
-  // Bug 1617611 - Fix all the tests broken by "cookies SameSite=Lax by default"
-  Services.prefs.setBoolPref("network.cookie.sameSite.laxByDefault", false);
-
   await setCookie("A", Services.io.newURI("https://example.com/A/"));
   await sleep();
 
@@ -72,6 +69,4 @@ add_task(async function() {
 
   someCookies = cm.getCookiesSince(cookies[3].creationTime + 1);
   Assert.equal(someCookies.length, 0, "We retrieve some cookies");
-
-  Services.prefs.clearUserPref("network.cookie.sameSite.laxByDefault");
 });

netwerk/locales/en-US/necko.properties

@@ -50,8 +50,8 @@ CookieAllowedForFpiByHeuristic=Storage access automatically granted for First-Pa
 
 # LOCALIZATION NOTE(CookieRejectedNonRequiresSecure2): %1$S is the cookie name. Do not localize "SameSite=None" and "secure".
 CookieRejectedNonRequiresSecure2=Cookie “%1$S” rejected because it has the “SameSite=None” attribute but is missing the “secure” attribute.
-# LOCALIZATION NOTE(CookieRejectedNonRequiresSecureForBeta2): %1$S is the cookie name. %2$S is a URL. Do not localize "SameSite", "SameSite=None" and "secure".
-CookieRejectedNonRequiresSecureForBeta2=Cookie “%1$S” will be soon rejected because it has the “SameSite” attribute set to “None” or an invalid value, without the “secure” attribute. To know more about the “SameSite“ attribute, read %2$S
+# LOCALIZATION NOTE(CookieRejectedNonRequiresSecureForBeta3): %1$S is the cookie name. %2$S is a URL. Do not localize "SameSite", "SameSite=None" and "secure".
+CookieRejectedNonRequiresSecureForBeta3=Cookie “%1$S” will be soon rejected because it has the “SameSite” attribute set to “None” without the “secure” attribute. To know more about the “SameSite“ attribute, read %2$S
 # LOCALIZATION NOTE(CookieLaxForced2): %1$S is the cookie name. Do not localize "SameSite", "Lax" and "SameSite=Lax".
 CookieLaxForced2=Cookie “%1$S” has “SameSite” policy set to “Lax” because it is missing a “SameSite” attribute, and “SameSite=Lax” is the default value for this attribute.
 # LOCALIZATION NOTE(CookieLaxForcedForBeta2): %1$S is the cookie name. %2$S is a URL. Do not localize "SameSite", "Lax" and "SameSite=Lax", "SameSite=None".

netwerk/test/gtest/TestCookie.cpp

@@ -1024,7 +1024,7 @@ TEST(TestCookie, SameSiteLax)
 
   cookie = static_cast<Cookie*>(cookies[0].get());
   EXPECT_EQ(cookie->RawSameSite(), nsICookie::SAMESITE_NONE);
-  EXPECT_EQ(cookie->SameSite(), nsICookie::SAMESITE_NONE);
+  EXPECT_EQ(cookie->SameSite(), nsICookie::SAMESITE_LAX);
 }
 
 TEST(TestCookie, OnionSite)

toolkit/components/url-classifier/tests/mochitest/test_classify_by_default.html

@@ -94,8 +94,6 @@ async function cleanup(topLevelSite = TEST_TOP_SITE) {
   // Ensure we clear the stylesheet cache so that we re-classify.
   SpecialPowers.DOMWindowUtils.clearSharedStyleSheetCache();
 
-  SpecialPowers.clearUserPref("network.cookie.sameSite.laxByDefault");
-
   await clearServiceWorker();
 }
 
@@ -110,8 +108,6 @@ async function runTests() {
     ["dom.serviceWorkers.exemptFromPerDomainMax", true],
     ["dom.serviceWorkers.enabled", true],
     ["dom.serviceWorkers.testing.enabled", true],
-    // Bug 1617611: Fix all the tests broken by "cookies SameSite=lax by default"
-    ["network.cookie.sameSite.laxByDefault", false],
   ]});
 
   await classifierHelper.waitForInit();

toolkit/components/url-classifier/tests/mochitest/trackerFrame.sjs

@@ -31,7 +31,7 @@ function handleRequest(aRequest, aResponse) {
     aResponse.setHeader("Content-Type", "text/plain", false);
 
     // Prepare the cookie
-    aResponse.setHeader("Set-Cookie", "cookie=1234");
+    aResponse.setHeader("Set-Cookie", "cookie=1234; SameSite=None; Secure");
     aResponse.setHeader(
       "Access-Control-Allow-Origin",
       aRequest.getHeader("Origin"),