Bug 1293067 - Implement a cookie limit warning, r=mayhemer,flod
☠☠ backed out by 0577d24b6ca5 ☠ ☠
authorAndrea Marchesini <amarchesini@mozilla.com>
Wed, 25 Mar 2020 10:38:45 +0000
changeset 520344 8ed41aa3ed4f2611ed2efd0bf835542874155071
parent 520343 3bd0695c40a1751de704452762a584cc74de054c
child 520345 5df9d39b0b98eb17eecad01752b736a28c316cc6
push id37248
push userbtara@mozilla.com
push dateWed, 25 Mar 2020 16:40:49 +0000
treeherdermozilla-central@c5112a7573ac [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmayhemer, flod
bugs1293067
milestone76.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 1293067 - Implement a cookie limit warning, r=mayhemer,flod Differential Revision: https://phabricator.services.mozilla.com/D67329
netwerk/cookie/nsCookieService.cpp
netwerk/cookie/nsCookieService.h
netwerk/locales/en-US/necko.properties
--- a/netwerk/cookie/nsCookieService.cpp
+++ b/netwerk/cookie/nsCookieService.cpp
@@ -3259,16 +3259,27 @@ bool nsCookieService::CanSetCookie(nsIUR
     aCookieData.isSession() = true;
   }
 
   // reject cookie if it's over the size limit, per RFC2109
   if ((aCookieData.name().Length() + aCookieData.value().Length()) >
       kMaxBytesPerCookie) {
     COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, savedCookieHeader,
                       "cookie too big (> 4kb)");
+
+    AutoTArray<nsString, 2> params = {
+        NS_ConvertUTF8toUTF16(aCookieData.name())};
+
+    nsString size;
+    size.AppendInt(kMaxBytesPerCookie);
+    params.AppendElement(size);
+
+    LogMessageToConsole(aChannel, aHostURI, nsIScriptError::warningFlag,
+                        CONSOLE_GENERIC_CATEGORY,
+                        NS_LITERAL_CSTRING("CookieOversize"), params);
     return newCookie;
   }
 
   const char illegalNameCharacters[] = {
       0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0A, 0x0B,
       0x0C, 0x0D, 0x0E, 0x0F, 0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16,
       0x17, 0x18, 0x19, 0x1A, 0x1B, 0x1C, 0x1D, 0x1E, 0x1F, 0x00};
 
@@ -3281,17 +3292,17 @@ bool nsCookieService::CanSetCookie(nsIUR
   // domain & path checks
   if (!CheckDomain(aCookieData, aHostURI, aKey.mBaseDomain,
                    aRequireHostMatch)) {
     COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, savedCookieHeader,
                       "failed the domain tests");
     return newCookie;
   }
 
-  if (!CheckPath(aCookieData, aHostURI)) {
+  if (!CheckPath(aCookieData, aChannel, aHostURI)) {
     COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, savedCookieHeader,
                       "failed the path tests");
     return newCookie;
   }
 
   // magic prefix checks. MUST be run after CheckDomain() and CheckPath()
   if (!CheckPrefixes(aCookieData, potentiallyTurstworthy)) {
     COOKIE_LOGFAILURE(SET_COOKIE, aHostURI, savedCookieHeader,
@@ -3850,102 +3861,100 @@ bool nsCookieService::ParseAttributes(ns
         aCookieData.sameSite() = nsICookie::SAMESITE_STRICT;
         aCookieData.rawSameSite() = nsICookie::SAMESITE_STRICT;
         sameSiteSet = true;
       } else if (tokenValue.LowerCaseEqualsLiteral(kSameSiteNone)) {
         aCookieData.sameSite() = nsICookie::SAMESITE_NONE;
         aCookieData.rawSameSite() = nsICookie::SAMESITE_NONE;
         sameSiteSet = true;
       } else {
-        LogMessageToConsole(aChannel, aHostURI, nsIScriptError::infoFlag,
-                            CONSOLE_GENERIC_CATEGORY,
-                            NS_LITERAL_CSTRING("CookieSameSiteValueInvalid"),
-                            aCookieData.name());
+        LogMessageToConsole(
+            aChannel, aHostURI, nsIScriptError::infoFlag,
+            CONSOLE_GENERIC_CATEGORY,
+            NS_LITERAL_CSTRING("CookieSameSiteValueInvalid"),
+            AutoTArray<nsString, 1>{NS_ConvertUTF8toUTF16(aCookieData.name())});
       }
     }
   }
 
   Telemetry::Accumulate(Telemetry::COOKIE_SAMESITE_SET_VS_UNSET,
                         sameSiteSet ? 1 : 0);
 
   // 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 (!aCookieData.isSecure() &&
       aCookieData.sameSite() == nsICookie::SAMESITE_NONE) {
     if (laxByDefault &&
         StaticPrefs::network_cookie_sameSite_noneRequiresSecure()) {
-      LogMessageToConsole(aChannel, aHostURI, nsIScriptError::infoFlag,
-                          CONSOLE_SAMESITE_CATEGORY,
-                          NS_LITERAL_CSTRING("CookieRejectedNonRequiresSecure"),
-                          aCookieData.name());
+      LogMessageToConsole(
+          aChannel, aHostURI, nsIScriptError::infoFlag,
+          CONSOLE_SAMESITE_CATEGORY,
+          NS_LITERAL_CSTRING("CookieRejectedNonRequiresSecure"),
+          AutoTArray<nsString, 1>{NS_ConvertUTF8toUTF16(aCookieData.name())});
       return newCookie;
     }
 
     // if sameSite=lax by default is disabled, we want to warn the user.
     LogMessageToConsole(
         aChannel, aHostURI, nsIScriptError::warningFlag,
         CONSOLE_SAMESITE_CATEGORY,
         NS_LITERAL_CSTRING("CookieRejectedNonRequiresSecureForBeta"),
-        aCookieData.name(), SAMESITE_MDN_URL);
+        AutoTArray<nsString, 2>{NS_ConvertUTF8toUTF16(aCookieData.name()),
+                                SAMESITE_MDN_URL});
   }
 
   if (aCookieData.rawSameSite() == nsICookie::SAMESITE_NONE &&
       aCookieData.sameSite() == nsICookie::SAMESITE_LAX) {
     if (laxByDefault) {
-      LogMessageToConsole(aChannel, aHostURI, nsIScriptError::infoFlag,
-                          CONSOLE_SAMESITE_CATEGORY,
-                          NS_LITERAL_CSTRING("CookieLaxForced"),
-                          aCookieData.name());
+      LogMessageToConsole(
+          aChannel, aHostURI, nsIScriptError::infoFlag,
+          CONSOLE_SAMESITE_CATEGORY, NS_LITERAL_CSTRING("CookieLaxForced"),
+          AutoTArray<nsString, 1>{NS_ConvertUTF8toUTF16(aCookieData.name())});
     } else {
-      LogMessageToConsole(aChannel, aHostURI, nsIScriptError::warningFlag,
-                          CONSOLE_SAMESITE_CATEGORY,
-                          NS_LITERAL_CSTRING("CookieLaxForcedForBeta"),
-                          aCookieData.name(), SAMESITE_MDN_URL);
+      LogMessageToConsole(
+          aChannel, aHostURI, nsIScriptError::warningFlag,
+          CONSOLE_SAMESITE_CATEGORY,
+          NS_LITERAL_CSTRING("CookieLaxForcedForBeta"),
+          AutoTArray<nsString, 2>{NS_ConvertUTF8toUTF16(aCookieData.name()),
+                                  SAMESITE_MDN_URL});
     }
   }
 
   // Cookie accepted.
   aAcceptedByParser = true;
 
   MOZ_ASSERT(nsCookie::ValidateRawSame(aCookieData));
   return newCookie;
 }
 
 // static
 void nsCookieService::LogMessageToConsole(nsIChannel* aChannel, nsIURI* aURI,
                                           uint32_t aErrorFlags,
                                           const nsACString& aCategory,
                                           const nsACString& aMsg,
-                                          const nsACString& aCookieName,
-                                          const nsAString& aMDNURL) {
+                                          const nsTArray<nsString>& aParams) {
   MOZ_ASSERT(aURI);
 
   nsCOMPtr<HttpBaseChannel> httpChannel = do_QueryInterface(aChannel);
   if (!httpChannel) {
     return;
   }
 
   nsAutoCString uri;
   nsresult rv = aURI->GetSpec(uri);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return;
   }
 
-  AutoTArray<nsString, 1> params = {NS_ConvertUTF8toUTF16(aCookieName)};
-
-  if (!aMDNURL.IsEmpty()) {
-    params.AppendElement(aMDNURL);
-  }
-
   httpChannel->AddConsoleReport(aErrorFlags, aCategory,
                                 nsContentUtils::eNECKO_PROPERTIES, uri, 0, 0,
-                                aMsg, params);
+                                aMsg, aParams);
 }
 
 /******************************************************************************
  * nsCookieService impl:
  * private domain & permission compliance enforcement functions
  ******************************************************************************/
 
 // Get the base domain for aHostURI; e.g. for "www.bbc.co.uk", this would be
@@ -4248,17 +4257,18 @@ nsAutoCString nsCookieService::GetPathFr
   if (lastSlash != firstSlash && lastSlash != kNotFound &&
       lastSlash == (int32_t)(path.Length() - 1)) {
     path.Truncate(lastSlash);
   }
 
   return path;
 }
 
-bool nsCookieService::CheckPath(CookieStruct& aCookieData, nsIURI* aHostURI) {
+bool nsCookieService::CheckPath(CookieStruct& aCookieData, nsIChannel* aChannel,
+                                nsIURI* aHostURI) {
   // if a path is given, check the host has permission
   if (aCookieData.path().IsEmpty() || aCookieData.path().First() != '/') {
     aCookieData.path() = GetPathFromURI(aHostURI);
 
 #if 0
   } else {
     /**
      * The following test is part of the RFC2109 spec.  Loosely speaking, it says that a site
@@ -4270,19 +4280,33 @@ bool nsCookieService::CheckPath(CookieSt
     nsAutoCString pathFromURI;
     if (NS_FAILED(aHostURI->GetPathQueryRef(pathFromURI)) ||
         !StringBeginsWith(pathFromURI, aCookieData.path())) {
       return false;
     }
 #endif
   }
 
-  if (aCookieData.path().Length() > kMaxBytesPerPath ||
-      aCookieData.path().Contains('\t'))
+  if (aCookieData.path().Length() > kMaxBytesPerPath) {
+    AutoTArray<nsString, 2> params = {
+        NS_ConvertUTF8toUTF16(aCookieData.name())};
+
+    nsString size;
+    size.AppendInt(kMaxBytesPerPath);
+    params.AppendElement(size);
+
+    LogMessageToConsole(aChannel, aHostURI, nsIScriptError::warningFlag,
+                        CONSOLE_GENERIC_CATEGORY,
+                        NS_LITERAL_CSTRING("CookiePathOversize"), params);
     return false;
+  }
+
+  if (aCookieData.path().Contains('\t')) {
+    return false;
+  }
 
   return true;
 }
 
 // CheckPrefixes
 //
 // Reject cookies whose name starts with the magic prefixes from
 // https://tools.ietf.org/html/draft-ietf-httpbis-cookie-prefixes-00
--- a/netwerk/cookie/nsCookieService.h
+++ b/netwerk/cookie/nsCookieService.h
@@ -302,17 +302,17 @@ class nsCookieService final : public nsI
                               mozilla::net::CookieStruct& aCookieData,
                               nsACString& aExpires, nsACString& aMaxage,
                               bool& aAcceptedByParser);
   bool RequireThirdPartyCheck();
   static bool CheckDomain(mozilla::net::CookieStruct& aCookieData,
                           nsIURI* aHostURI, const nsCString& aBaseDomain,
                           bool aRequireHostMatch);
   static bool CheckPath(mozilla::net::CookieStruct& aCookieData,
-                        nsIURI* aHostURI);
+                        nsIChannel* aChannel, nsIURI* aHostURI);
   static bool CheckPrefixes(mozilla::net::CookieStruct& aCookieData,
                             bool aSecureRequest);
   static bool GetExpiry(mozilla::net::CookieStruct& aCookieData,
                         const nsACString& aExpires, const nsACString& aMaxage,
                         int64_t aServerTime, int64_t aCurrentTime,
                         bool aFromHttp);
   void RemoveAllFromMemory();
   already_AddRefed<nsIArray> PurgeCookies(int64_t aCurrentTimeInUsec);
@@ -348,18 +348,17 @@ class nsCookieService final : public nsI
   nsresult RemoveCookiesFromExactHost(
       const nsACString& aHost,
       const mozilla::OriginAttributesPattern& aPattern);
 
   static void LogMessageToConsole(nsIChannel* aChannel, nsIURI* aURI,
                                   uint32_t aErrorFlags,
                                   const nsACString& aCategory,
                                   const nsACString& aMsg,
-                                  const nsACString& aCookieName,
-                                  const nsAString& aMDNURL = VoidString());
+                                  const nsTArray<nsString>& aParams);
 
   // cached members.
   nsCOMPtr<nsICookiePermission> mPermissionService;
   nsCOMPtr<mozIThirdPartyUtil> mThirdPartyUtil;
   nsCOMPtr<nsIEffectiveTLDService> mTLDService;
   nsCOMPtr<nsIIDNService> mIDNService;
   nsCOMPtr<mozIStorageService> mStorageService;
 
--- a/netwerk/locales/en-US/necko.properties
+++ b/netwerk/locales/en-US/necko.properties
@@ -54,8 +54,12 @@ CookieRejectedNonRequiresSecure=Cookie “%1$S” rejected because it has the “sameSite=none” attribute but is missing the “secure” attribute.
 # LOCALIZATION NOTE(CookieRejectedNonRequiresSecureForBeta): %1$S is the cookie name. %2$S is a URL. Do not localize "sameSite", "sameSite=none" and "secure".
 CookieRejectedNonRequiresSecureForBeta=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(CookieLaxForced): %1$S is the cookie name. Do not localize "sameSite", "lax" and "sameSite=lax".
 CookieLaxForced=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(CookieLaxForcedForBeta): %1$S is the cookie name. %2$S is a URL. Do not localize "sameSite", "lax" and "sameSite=lax", "sameSite=none".
 CookieLaxForcedForBeta=Cookie “%1$S” 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 %2$S
 # LOCALIZATION NOTE: %1$S is cookie name. Do not localize "sameSite", "lax", "strict" and "none"
 CookieSameSiteValueInvalid=Invalid “sameSite“ value for cookie “%1$S”. The supported values are: “lax“, “strict“, “none“.
+# LOCALIZATION NOTE (CookieOversize): %1$S is the cookie name. %2$S is the number of bytes. "B" means bytes.
+CookieOversize=Cookie “%1$S” is invalid because its size is too big. Max size is %2$S B.
+# LOCALIZATION NOTE (CookiePathOversiz): %1$S is the cookie name. %2$S is the number of bytes. "B" means bytes.
+CookiePathOversize=Cookie “%1$S” is invalid because its path size is too big. Max size is %2$S B.