Bug 1906914 - Fix the max-age cookie attribute parser, r=edgul,devtools-reviewers,nchevobbe

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

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

@@ -148,3 +148,49 @@ add_task(async function testInvalidSameSiteMessage() {
 });
 
 add_task(cleanUp);
+
+add_task(async function testInvalidMaxAgeMessage() {
+  const message1 =
+    "Invalid “max-age“ value for cookie “a”. The attribute is ignored.";
+  const message2 =
+    "Invalid “max-age“ value for cookie “b”. The attribute is ignored.";
+
+  const { hud, tab, win } = await openNewWindowAndConsole(
+    "http://example.org/" + TEST_FILE
+  );
+
+  info("Test cookie messages");
+
+  SpecialPowers.spawn(tab.linkedBrowser, [], () => {
+    content.wrappedJSObject.createCookie("a=1; max-age=abc; samesite=lax");
+    content.wrappedJSObject.createCookie("b=1; max-age=1,2; samesite=lax");
+  });
+
+  const { node } = await waitForMessageByType(hud, COOKIE_GROUP, ".warn");
+  is(
+    node.querySelector(".warning-group-badge").textContent,
+    "2",
+    "The badge has the expected text"
+  );
+
+  await checkConsoleOutputForWarningGroup(hud, [`▶︎⚠ ${COOKIE_GROUP} 2`]);
+
+  info("Open the group");
+  node.querySelector(".arrow").click();
+  await waitFor(() => findWarningMessage(hud, "Cookie"));
+
+  await checkConsoleOutputForWarningGroup(hud, [
+    `▼︎⚠ ${COOKIE_GROUP} 2`,
+    `| ${message1}`,
+    `| ${message2}`,
+  ]);
+
+  // Source map are being resolved in background and we might have
+  // pending request related to this service if we close the window
+  // immeditely. So just wait for these request to finish before proceeding.
+  await hud.toolbox.sourceMapURLService.waitForSourcesLoading();
+
+  await win.close();
+});
+
+add_task(cleanUp);

devtools/client/webconsole/utils/messages.js

@@ -1025,9 +1025,12 @@ function isTrackingProtectionMessage(message) {
  */
 function isCookieMessage(message) {
   const { category } = message;
-  return ["cookiesCHIPS", "cookiesOversize", "cookieSameSite"].includes(
-    category
-  );
+  return [
+    "cookiesCHIPS",
+    "cookiesOversize",
+    "cookieSameSite",
+    "cookieInvalidAttribute",
+  ].includes(category);
 }
 
 /**

netwerk/cookie/CookieParser.cpp

@@ -6,10 +6,12 @@
 #include "CookieParser.h"
 #include "CookieLogging.h"
 
+#include "mozilla/CheckedInt.h"
 #include "mozilla/dom/nsMixedContentBlocker.h"
 #include "mozilla/glean/GleanMetrics.h"
 #include "mozilla/net/Cookie.h"
 #include "mozilla/StaticPrefs_network.h"
+#include "mozilla/TextUtils.h"
 #include "nsIConsoleReportCollector.h"
 #include "nsIScriptError.h"
 #include "nsIURI.h"
@@ -23,6 +25,7 @@ constexpr auto CONSOLE_CHIPS_CATEGORY = "cookiesCHIPS"_ns;
 constexpr auto CONSOLE_OVERSIZE_CATEGORY = "cookiesOversize"_ns;
 constexpr auto CONSOLE_REJECTION_CATEGORY = "cookiesRejection"_ns;
 constexpr auto CONSOLE_SAMESITE_CATEGORY = "cookieSameSite"_ns;
+constexpr auto CONSOLE_INVALID_ATTRIBUTE_CATEGORY = "cookieInvalidAttribute"_ns;
 constexpr auto SAMESITE_MDN_URL =
     "https://developer.mozilla.org/docs/Web/HTTP/Headers/Set-Cookie/"
     u"SameSite"_ns;
@@ -158,8 +161,8 @@ CookieParser::~CookieParser() {
 
   for (const char* attribute : mWarnings.mAttributeOverwritten) {
     CookieLogging::LogMessageToConsole(
-        mCRC, mHostURI, nsIScriptError::warningFlag, CONSOLE_OVERSIZE_CATEGORY,
-        "CookieAttributeOverwritten"_ns,
+        mCRC, mHostURI, nsIScriptError::warningFlag,
+        CONSOLE_INVALID_ATTRIBUTE_CATEGORY, "CookieAttributeOverwritten"_ns,
         AutoTArray<nsString, 2>{NS_ConvertUTF8toUTF16(mCookieData.name()),
                                 NS_ConvertUTF8toUTF16(attribute)});
   }
@@ -171,6 +174,13 @@ CookieParser::~CookieParser() {
         AutoTArray<nsString, 1>{NS_ConvertUTF8toUTF16(mCookieData.name())});
   }
 
+  if (mWarnings.mInvalidMaxAgeAttribute) {
+    CookieLogging::LogMessageToConsole(
+        mCRC, mHostURI, nsIScriptError::infoFlag,
+        CONSOLE_INVALID_ATTRIBUTE_CATEGORY, "CookieInvalidMaxAgeAttribute"_ns,
+        AutoTArray<nsString, 1>{NS_ConvertUTF8toUTF16(mCookieData.name())});
+  }
+
   if (mWarnings.mSameSiteNoneRequiresSecureForBeta) {
     CookieLogging::LogMessageToConsole(
         mCRC, mHostURI, nsIScriptError::warningFlag, CONSOLE_SAMESITE_CATEGORY,
@@ -641,6 +651,52 @@ bool CookieParser::CheckPrefixes(CookieStruct& aCookieData,
   return true;
 }
 
+bool CookieParser::ParseMaxAgeAttribute(const nsACString& aMaxage,
+                                        int64_t* aValue) {
+  MOZ_ASSERT(aValue);
+
+  if (aMaxage.IsEmpty()) {
+    return false;
+  }
+
+  nsACString::const_char_iterator iter;
+  aMaxage.BeginReading(iter);
+
+  nsACString::const_char_iterator end;
+  aMaxage.EndReading(end);
+
+  // Negative values mean that the cookie is already expired. Don't bother to
+  // parse.
+  if (*iter == '-') {
+    *aValue = INT64_MIN;
+    return true;
+  }
+
+  CheckedInt<int64_t> value(0);
+
+  for (; iter != end; ++iter) {
+    if (!mozilla::IsAsciiDigit(*iter)) {
+      mWarnings.mInvalidMaxAgeAttribute = true;
+      return false;
+    }
+
+    value *= 10;
+    if (!value.isValid()) {
+      *aValue = INT64_MAX;
+      return true;
+    }
+
+    value += *iter - '0';
+    if (!value.isValid()) {
+      *aValue = INT64_MAX;
+      return true;
+    }
+  }
+
+  *aValue = value.value();
+  return true;
+}
+
 bool CookieParser::GetExpiry(CookieStruct& aCookieData,
                              const nsACString& aExpires,
                              const nsACString& aMaxage, int64_t aCurrentTime,
@@ -659,26 +715,22 @@ bool CookieParser::GetExpiry(CookieStruct& aCookieData,
    * Note: We need to consider accounting for network lag here, per RFC.
    */
   // check for max-age attribute first; this overrides expires attribute
-  if (!aMaxage.IsEmpty()) {
-    // obtain numeric value of maxageAttribute
-    int64_t maxage;
-    int32_t numInts = PR_sscanf(aMaxage.BeginReading(), "%lld", &maxage);
-
-    // default to session cookie if the conversion failed
-    if (numInts != 1) {
-      return true;
-    }
-
-    // if this addition overflows, expiryTime will be less than currentTime
-    // and the cookie will be expired - that's okay.
-    if (maxageCap) {
-      aCookieData.expiry() = aCurrentTime + std::min(maxage, maxageCap);
+  int64_t maxage = 0;
+  if (ParseMaxAgeAttribute(aMaxage, &maxage)) {
+    if (maxage == INT64_MIN || maxage == INT64_MAX) {
+      aCookieData.expiry() = maxage;
     } else {
-      aCookieData.expiry() = aCurrentTime + maxage;
+      CheckedInt<int64_t> value(aCurrentTime);
+      value += maxageCap ? std::min(maxage, maxageCap) : maxage;
+
+      aCookieData.expiry() = value.isValid() ? value.value() : INT64_MAX;
     }
 
-    // check for expires attribute
-  } else if (!aExpires.IsEmpty()) {
+    return false;
+  }
+
+  // check for expires attribute
+  if (!aExpires.IsEmpty()) {
     PRTime expires;
 
     // parse expiry time
@@ -699,14 +751,13 @@ bool CookieParser::GetExpiry(CookieStruct& aCookieData,
       aCookieData.expiry() = expires / int64_t(PR_USEC_PER_SEC);
     }
 
-    // default to session cookie if no attributes found.  Here we don't need to
-    // enforce the maxage cap, because session cookies are short-lived by
-    // definition.
-  } else {
-    return true;
+    return false;
   }
 
-  return false;
+  // default to session cookie if no attributes found.  Here we don't need to
+  // enforce the maxage cap, because session cookies are short-lived by
+  // definition.
+  return true;
 }
 
 // returns true if 'a' is equal to or a subdomain of 'b',

netwerk/cookie/CookieParser.h

@@ -63,6 +63,9 @@ class CookieParser final {
     return mCookieData;
   }
 
+  // Public for testing
+  bool ParseMaxAgeAttribute(const nsACString& aMaxage, int64_t* aValue);
+
  private:
   static bool GetTokenValue(nsACString::const_char_iterator& aIter,
                             nsACString::const_char_iterator& aEndIter,
@@ -73,9 +76,9 @@ class CookieParser final {
   bool ParseAttributes(nsCString& aCookieHeader, nsACString& aExpires,
                        nsACString& aMaxage, bool& aAcceptedByParser);
 
-  static bool GetExpiry(CookieStruct& aCookieData, const nsACString& aExpires,
-                        const nsACString& aMaxage, int64_t aCurrentTime,
-                        bool aFromHttp);
+  bool GetExpiry(CookieStruct& aCookieData, const nsACString& aExpires,
+                 const nsACString& aMaxage, int64_t aCurrentTime,
+                 bool aFromHttp);
 
   bool CheckPath();
   bool CheckAttributeSize(const nsACString& currentValue,
@@ -101,6 +104,7 @@ class CookieParser final {
     nsTArray<const char*> mAttributeOverwritten;
 
     bool mInvalidSameSiteAttribute = false;
+    bool mInvalidMaxAgeAttribute = false;
     bool mSameSiteNoneRequiresSecureForBeta = false;
     bool mSameSiteLaxForced = false;
     bool mSameSiteLaxForcedForBeta = false;

netwerk/cookie/test/unit/test_rawSameSite.js

@@ -72,16 +72,16 @@ add_task(async _ => {
 
   let tests = [
     {
-      cookie: "foo=b;max-age=3600, c=d;path=/; sameSite=strict",
+      cookie: "foo=b;max-age=3600;path=/; sameSite=strict",
       sameSite: 2,
       rawSameSite: 2,
     },
     {
-      cookie: "foo=b;max-age=3600, c=d;path=/; sameSite=lax",
+      cookie: "foo=b;max-age=3600;path=/; sameSite=lax",
       sameSite: 1,
       rawSameSite: 1,
     },
-    { cookie: "foo=b;max-age=3600, c=d;path=/", sameSite: 1, rawSameSite: 0 },
+    { cookie: "foo=b;max-age=3600;path=/", sameSite: 1, rawSameSite: 0 },
   ];
 
   for (let i = 0; i < tests.length; ++i) {

netwerk/locales/en-US/necko.properties

@@ -62,6 +62,8 @@ CookieLaxForced2=Cookie “%1$S” has “SameSite” policy set to “Lax” be
 CookieLaxForcedForBeta2=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(CookieSameSiteValueInvalid2): %1$S is cookie name. Do not localize "SameSite", "Lax", "Strict" and "None"
 CookieSameSiteValueInvalid2=Invalid “SameSite“ value for cookie “%1$S”. The supported values are: “Lax“, “Strict“, “None“.
+# LOCALIZATION NOTE(CookieInvalidMaxAgeAttribute): %1$S is cookie name. Do not localize "max-age".
+CookieInvalidMaxAgeAttribute=Invalid “max-age“ value for cookie “%1$S”. The attribute is ignored.
 # 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 (CookieRejectedByPermissionManager): %1$S is the cookie response header.

netwerk/test/gtest/TestCookie.cpp

@@ -25,7 +25,9 @@
 #include "mozilla/Unused.h"
 #include "mozilla/net/CookieJarSettings.h"
 #include "Cookie.h"
+#include "CookieParser.h"
 #include "nsIURI.h"
+#include "nsIConsoleReportCollector.h"
 
 using namespace mozilla;
 using namespace mozilla::net;
@@ -35,7 +37,6 @@ static NS_DEFINE_CID(kPrefServiceCID, NS_PREFSERVICE_CID);
 
 // various pref strings
 static const char kCookiesPermissions[] = "network.cookie.cookieBehavior";
-static const char kPrefCookieQuotaPerHost[] = "network.cookie.quotaPerHost";
 static const char kCookiesMaxPerHost[] = "network.cookie.maxPerHost";
 
 #define OFFSET_ONE_WEEK int64_t(604800) * PR_USEC_PER_SEC
@@ -1125,3 +1126,80 @@ TEST(TestCookie, BlockUnicode)
   EXPECT_NS_SUCCEEDED(cookieMgr->RemoveAll());
   Preferences::ClearUser("network.cookie.blockUnicode");
 }
+
+TEST(TestCookie, MaxAgeParser)
+{
+  nsCOMPtr<nsIURI> uri;
+  NS_NewURI(getter_AddRefs(uri), "https://maxage.net");
+
+  nsCOMPtr<nsIIOService> service = do_GetIOService();
+
+  nsCOMPtr<nsIChannel> channel;
+  Unused << service->NewChannelFromURI(
+      uri, nullptr, nsContentUtils::GetSystemPrincipal(),
+      nsContentUtils::GetSystemPrincipal(), 0, nsIContentPolicy::TYPE_DOCUMENT,
+      getter_AddRefs(channel));
+
+  nsCOMPtr<nsIConsoleReportCollector> crc = do_QueryInterface(channel);
+
+  CookieParser cp(crc, uri);
+
+  int64_t value;
+  EXPECT_FALSE(cp.ParseMaxAgeAttribute(""_ns, &value));
+
+  EXPECT_TRUE(cp.ParseMaxAgeAttribute("0"_ns, &value));
+  EXPECT_EQ(value, 0);
+
+  EXPECT_TRUE(cp.ParseMaxAgeAttribute("1"_ns, &value));
+  EXPECT_EQ(value, 1);
+
+  EXPECT_TRUE(cp.ParseMaxAgeAttribute("1234"_ns, &value));
+  EXPECT_EQ(value, 1234);
+
+  EXPECT_TRUE(cp.ParseMaxAgeAttribute("00000000000000001234"_ns, &value));
+  EXPECT_EQ(value, 1234);
+
+  EXPECT_TRUE(cp.ParseMaxAgeAttribute("-1234"_ns, &value));
+  EXPECT_EQ(value, INT64_MIN);
+
+  {
+    nsCString str;
+    for (int i = 0; i < 1024; ++i) {
+      str.Append("9");
+    }
+    EXPECT_TRUE(cp.ParseMaxAgeAttribute(str, &value));
+    EXPECT_EQ(value, INT64_MAX);
+  }
+
+  EXPECT_FALSE(cp.ParseMaxAgeAttribute("1234a"_ns, &value));
+  EXPECT_FALSE(cp.ParseMaxAgeAttribute("12a34"_ns, &value));
+  EXPECT_FALSE(cp.ParseMaxAgeAttribute("12🌊34"_ns, &value));
+
+  nsresult rv;
+  nsCOMPtr<nsICookieManager> cookieMgr =
+      do_GetService(NS_COOKIEMANAGER_CONTRACTID, &rv);
+  ASSERT_NS_SUCCEEDED(rv);
+
+  EXPECT_NS_SUCCEEDED(cookieMgr->RemoveAll());
+
+  nsCOMPtr<nsICookieService> cookieService =
+      do_GetService(kCookieServiceCID, &rv);
+  ASSERT_NS_SUCCEEDED(rv);
+
+  SetACookie(cookieService, "http://maxage.net/", "a=1; max-age=1234");
+
+  nsCString cookieStr;
+  GetACookie(cookieService, "http://maxage.net/", cookieStr);
+  EXPECT_TRUE(CheckResult(cookieStr.get(), MUST_EQUAL, "a=1"));
+
+  nsTArray<RefPtr<nsICookie>> cookies;
+  EXPECT_NS_SUCCEEDED(cookieMgr->GetCookies(cookies));
+  EXPECT_EQ(cookies.Length(), (uint64_t)1);
+
+  Cookie* cookie = static_cast<Cookie*>(cookies[0].get());
+  EXPECT_FALSE(cookie->IsSession());
+
+  SetACookie(cookieService, "http://maxage.net/", "a=1; max-age=-1");
+  GetACookie(cookieService, "http://maxage.net/", cookieStr);
+  EXPECT_TRUE(CheckResult(cookieStr.get(), MUST_EQUAL, ""));
+}

testing/web-platform/meta/cookies/size/attributes.www.sub.html.ini

@@ -1,6 +0,0 @@
-[attributes.www.sub.html]
-  [Max length Max-Age attribute value (1024 bytes) doesn't cause cookie rejection]
-    expected: FAIL
-
-  [Max length negative Max-Age attribute value (1024 bytes) doesn't get ignored]
-    expected: FAIL