Allow tabs and reject other CTLs in cookie tests

Updates the cookie control character tests to match the recent updates to RFC6265bis - specifically, the tab character is now permitted, but all other control characters cause the entire cookie to be rejected (previously \x00, \x0D, and \x0A caused the cookie line being parsed to be truncated instead of rejecting the whole cookie line). Bug: 1237551 Change-Id: I94e13af43efd641f7b5a3706471ea0c91a0be2f1 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3084521 Reviewed-by: Steven Bingler <bingler@chromium.org> Commit-Queue: Andrew Williams <awillia@google.com> Cr-Commit-Position: refs/heads/main@{#921856}
web-platform-tests · Sep 15, 2021 · e87cd77 · e87cd77
1 parent c299dd9
commit e87cd77
Show file tree

Hide file tree

Showing 4 changed files with 137 additions and 53 deletions.
diff --git a/cookies/attributes/attributes-ctl.sub.html b/cookies/attributes/attributes-ctl.sub.html
@@ -0,0 +1,114 @@
+<!doctype html>
+<html>
+  <head>
+    <meta charset=utf-8>
+    <title>Test cookie attribute parsing with control characters</title>
+    <meta name=help href="https://tools.ietf.org/html/rfc6265#section-5.2">
+    <meta name="timeout" content="long">
+    <script src="/resources/testharness.js"></script>
+    <script src="/resources/testharnessreport.js"></script>
+    <script src="/resources/testdriver.js"></script>
+    <script src="/resources/testdriver-vendor.js"></script>
+    <script src="/cookies/resources/cookie-test.js"></script>
+  </head>
+  <body>
+    <div id=log></div>
+    <script>
+      const host = "{{host}}";
+      const path = "/cookies/attributes";
+
+      // Tests for control characters (CTLs) in a cookie's attribute values.
+      // CTLs are defined by RFC 5234 to be %x00-1F / %x7F.
+      const CTLS = getCtlCharacters();
+
+      // All CTLs, with the exception of %x09 (the tab character), should
+      // cause the cookie to be rejected.
+      // In these tests we rely on subsequent attributes with the same name
+      // overriding the earlier one. In the cases where the control character
+      // should cause the entire cookie line to be rejected, if the control
+      // character were not present the cookie line should be one that
+      // would not be rejected. That way, if the attribute value is ignored
+      // instead of the cookie line being rejected, the test will catch it.
+      for (const ctl of CTLS) {
+        // NOTE: 'expected' below is only expected in the case of the tab
+        // character. Otherwise, '' is expected.
+        const controlCharacterAttributeTests = [
+          {
+            cookie: `test${ctl.code}domain=t; Domain=test${ctl.chr}.co; Domain=${host};`,
+            expected: `test${ctl.code}domain=t`,
+            name: `Cookie with %x${ctl.code.toString(16)} in Domain attribute value is handled correctly.`,
+          },
+          {
+            cookie: `test${ctl.code}domain2=t; Domain=${host}${ctl.chr};`,
+            expected: `test${ctl.code}domain2=t`,
+            name: `Cookie with %x${ctl.code.toString(16)} after Domain attribute value is handled correctly.`,
+          },
+          {
+            cookie: `test${ctl.code}path=t; Path=/te${ctl.chr}st; Path=${path}`,
+            expected: `test${ctl.code}path=t`,
+            name: `Cookie with %x${ctl.code.toString(16)} in Path attribute value is handled correctly.`,
+          },
+          {
+            cookie: `test${ctl.code}path2=t; Path=${path}${ctl.chr};`,
+            expected: `test${ctl.code}path2=t`,
+            name: `Cookie with %x${ctl.code.toString(16)} after Path attribute value is handled correctly.`,
+          },
+          {
+            cookie: `test${ctl.code}maxage=t; Max-Age=10${ctl.chr}00; Max-Age=1000;`,
+            expected: `test${ctl.code}maxage=t`,
+            name: `Cookie with %x${ctl.code.toString(16)} in Max-Age attribute value is handled correctly.`,
+          },
+          {
+            cookie: `test${ctl.code}maxage2=t; Max-Age=1000${ctl.chr};`,
+            expected: `test${ctl.code}maxage2=t`,
+            name: `Cookie with %x${ctl.code.toString(16)} after Max-Age attribute value is handled correctly.`,
+          },
+          {
+            cookie: `test${ctl.code}expires=t; Expires=Fri, 01 Jan 20${ctl.chr}38 00:00:00 GMT; ` +
+              'Expires=Fri, 01 Jan 2038 00:00:00 GMT;',
+            expected: `test${ctl.code}expires=t`,
+            name: `Cookie with %x${ctl.code.toString(16)} in Expires attribute value is handled correctly.`,
+          },
+          {
+            cookie: `test${ctl.code}expires2=t; Expires=Fri, 01 Jan 2038 00:00:00 GMT${ctl.chr};`,
+            expected: `test${ctl.code}expires2=t`,
+            name: `Cookie with %x${ctl.code.toString(16)} after Expires attribute value is handled correctly.`,
+          },
+          {
+            cookie: `test${ctl.code}secure=t; Sec${ctl.chr}ure;`,
+            expected: `test${ctl.code}secure=t`,
+            name: `Cookie with %x${ctl.code.toString(16)} in Secure attribute is handled correctly.`,
+          },
+          {
+            cookie: `test${ctl.code}secure2=t; Secure${ctl.chr};`,
+            expected: `test${ctl.code}secure2=t`,
+            name: `Cookie with %x${ctl.code.toString(16)} after Secure attribute is handled correctly.`,
+          },
+          {
+            cookie: `test${ctl.code}httponly=t; Http${ctl.chr}Only;`,
+            expected: `test${ctl.code}httponly=t`,
+            name: `Cookie with %x${ctl.code.toString(16)} in HttpOnly attribute is handled correctly.`,
+          },
+          {
+            cookie: `test${ctl.code}samesite=t; SameSite=No${ctl.chr}ne; SameSite=None;`,
+            expected: `test${ctl.code}samesite=t`,
+            name: `Cookie with %x${ctl.code.toString(16)} in SameSite attribute value is handled correctly.`,
+          },
+          {
+            cookie: `test${ctl.code}samesite2=t; SameSite=None${ctl.chr};`,
+            expected: `test${ctl.code}samesite2=t`,
+            name: `Cookie with %x${ctl.code.toString(16)} after SameSite attribute value is handled correctly.`,
+          },
+        ];
+
+        for (const test of controlCharacterAttributeTests) {
+          if (ctl.code === 0x09) {
+            domCookieTest(test.cookie, test.expected, test.name);
+          } else {
+            domCookieTest(test.cookie, "", test.name);
+          }
+        }
+      }
+    </script>
+  </body>
+</html>
diff --git a/cookies/name/name-ctl.html b/cookies/name/name-ctl.html
@@ -16,32 +16,21 @@
     <script>
       // Tests for control characters (CTLs) in a cookie's name.
       // CTLs are defined by RFC 5234 to be %x00-1F / %x7F.
-      const {TERMINATING_CTLS, CTLS} = getCtlCharacters();
+      const CTLS = getCtlCharacters();
 
-      // Test that terminating CTLs truncate the cookie string.
-      for (const ctl of TERMINATING_CTLS) {
-        domCookieTest(
-            `test${ctl.code}${ctl.chr}name=${ctl.code}`,
-            `test${ctl.code}`,
-            `Cookie with %x${ctl.code.toString(16)} in name is truncated.`);
-      }
-
-      // Test that other CTLs result in cookie rejection.
+      // All CTLs, with the exception of %x09 (the tab character), should
+      // cause the cookie to be rejected.
       for (const ctl of CTLS) {
-        domCookieTest(
+        if (ctl.code === 0x09) {
+          domCookieTest(
+            `test${ctl.code}${ctl.chr}name=${ctl.code}`,
+            `test${ctl.code}${ctl.chr}name=${ctl.code}`,
+            `Cookie with %x${ctl.code.toString(16)} in name is accepted.`);
+        } else {
+          domCookieTest(
             `test${ctl.code}${ctl.chr}name=${ctl.code}`,
             '',
             `Cookie with %x${ctl.code.toString(16)} in name is rejected.`);
-      }
-
-      // Test that truncation due to terminating CTLs occurs first.
-      for (const termCtl of TERMINATING_CTLS) {
-        for (const ctl of CTLS) {
-          domCookieTest(
-              `test${ctl.code}term${termCtl.chr}na${ctl.chr}me=${ctl.code}`,
-              `test${ctl.code}term`,
-              `Cookie with %x${ctl.code.toString(16)} after ` +
-                  `%x${termCtl.code.toString(16)} in name is truncated.`);
         }
       }
     </script>

diff --git a/cookies/resources/cookie-test.js b/cookies/resources/cookie-test.js
@@ -128,20 +128,12 @@ function domCookieTest(cookie, expectedValue, name) {
   }, name);
 }
 
-// Returns two arrays of control characters along with their ASCII codes. The
-// TERMINATING_CTLS should result in termination of the cookie string. The
-// remaining CTLS should result in rejection of the cookie. Control characters
-// are defined by RFC 5234 to be %x00-1F / %x7F.
+// Returns an array of control characters along with their ASCII codes. Control
+// characters are defined by RFC 5234 to be %x00-1F / %x7F.
 function getCtlCharacters() {
-  const termCtlCodes = [0x00 /* NUL */, 0x0A /* LF */, 0x0D /* CR */];
   const ctlCodes = [...Array(0x20).keys()]
-                       .filter(i => termCtlCodes.indexOf(i) === -1)
                        .concat([0x7F]);
-  return {
-    TERMINATING_CTLS:
-        termCtlCodes.map(i => ({code: i, chr: String.fromCharCode(i)})),
-    CTLS: ctlCodes.map(i => ({code: i, chr: String.fromCharCode(i)}))
-  };
+  return ctlCodes.map(i => ({ code: i, chr: String.fromCharCode(i) }))
 }
 
 // Returns a cookie string with name set to "t" * nameLength and value

diff --git a/cookies/value/value-ctl.html b/cookies/value/value-ctl.html
@@ -16,32 +16,21 @@
     <script>
       // Tests for control characters (CTLs) in a cookie's value.
       // CTLs are defined by RFC 5234 to be %x00-1F / %x7F.
-      const {TERMINATING_CTLS, CTLS} = getCtlCharacters();
+      const CTLS = getCtlCharacters();
 
-      // Test that terminating CTLs truncate the cookie string.
-      for (const ctl of TERMINATING_CTLS) {
-        domCookieTest(
-            `test=${ctl.code}${ctl.chr}value`,
-            `test=${ctl.code}`,
-            `Cookie with %x${ctl.code.toString(16)} in value is truncated.`);
-      }
-
-      // Test that other CTLs result in cookie rejection.
+      // All CTLs, with the exception of %x09 (the tab character), should
+      // cause the cookie to be rejected.
       for (const ctl of CTLS) {
-        domCookieTest(
+        if (ctl.code === 0x09) {
+          domCookieTest(
+            `test=${ctl.code}${ctl.chr}value`,
+            `test=${ctl.code}${ctl.chr}value`,
+            `Cookie with %x${ctl.code.toString(16)} in value is accepted.`);
+        } else {
+          domCookieTest(
             `test=${ctl.code}${ctl.chr}value`,
             '',
             `Cookie with %x${ctl.code.toString(16)} in value is rejected.`);
-      }
-
-      // Test that truncation due to terminating CTLs occurs first.
-      for (const termCtl of TERMINATING_CTLS) {
-        for (const ctl of CTLS) {
-          domCookieTest(
-              `test=${ctl.code}term${termCtl.chr}va${ctl.chr}lue`,
-              `test=${ctl.code}term`,
-              `Cookie with %x${ctl.code.toString(16)} after ` +
-                  `%x${termCtl.code.toString(16)} in value is truncated.`);
         }
       }
     </script>