Split Set-Cookie header correctly (#30560)

## Bug

- [x] Related issues linked using `fixes #number`
- [x] Integration tests added
- [ ] Errors have helpful link attached, see `contributing.md`

Fixes #30430 

There's some more discussion in the issue, but in summary:
- web `Headers` implementation combines all header values with `', '`
- For `Set-Cookie` headers, you're supposed to set them as separate values, not combine them
- web `Headers` forbids the use of `Cookie`, `Set-Cookie` and some more headers, so they don't have custom implementation for those, and still joins them with `,`
- We currently just split them using `split(',')`, but this breaks when the header contains a date (expires, max-age) that also includes a `,`

I used this method to split the Set-Cookie header properly: https://www.npmjs.com/package/set-cookie-parser#splitcookiestringcombinedsetcookieheader as suggested [here](whatwg/fetch#973 (comment))

I didn't add it as a dependency, since we only needed that one method and I wasn't sure what the process is for adding dependencies, so I just added the method in the middleware utils

packages/next/server/next-server.ts

@@ -108,6 +108,7 @@ import type { FetchEventResult } from './web/types'
 import type { MiddlewareManifest } from '../build/webpack/plugins/middleware-plugin'
 import type { ParsedNextUrl } from '../shared/lib/router/utils/parse-next-url'
 import type { ParsedUrl } from '../shared/lib/router/utils/parse-url'
+import { toNodeHeaders } from './web/utils'
 
 const getCustomRouteMatcher = pathMatch(true)
 
@@ -1168,13 +1169,11 @@ export default class Server {
 
           result.response.headers.delete('x-middleware-next')
 
-          for (const [key, value] of result.response.headers.entries()) {
-            if (key !== 'content-encoding') {
-              if (key.toLowerCase() === 'set-cookie') {
-                res.setHeader(key, value.split(', '))
-              } else {
-                res.setHeader(key, value)
-              }
+          for (const [key, value] of Object.entries(
+            toNodeHeaders(result.response.headers)
+          )) {
+            if (key !== 'content-encoding' && value !== undefined) {
+              res.setHeader(key, value)
             }
           }
 

packages/next/server/web/utils.ts

@@ -39,9 +39,85 @@ export function toNodeHeaders(headers?: Headers): NodeHeaders {
     for (const [key, value] of headers.entries()) {
       result[key] = value
       if (key.toLowerCase() === 'set-cookie') {
-        result[key] = value.split(', ')
+        result[key] = splitCookiesString(value)
       }
     }
   }
   return result
 }
+
+/*
+  Set-Cookie header field-values are sometimes comma joined in one string. This splits them without choking on commas
+  that are within a single set-cookie field-value, such as in the Expires portion.
+  This is uncommon, but explicitly allowed - see https://tools.ietf.org/html/rfc2616#section-4.2
+  Node.js does this for every header *except* set-cookie - see https://github.com/nodejs/node/blob/d5e363b77ebaf1caf67cd7528224b651c86815c1/lib/_http_incoming.js#L128
+  React Native's fetch does this for *every* header, including set-cookie.
+  
+  Based on: https://github.com/google/j2objc/commit/16820fdbc8f76ca0c33472810ce0cb03d20efe25
+  Credits to: https://github.com/tomball for original and https://github.com/chrusart for JavaScript implementation
+*/
+export function splitCookiesString(cookiesString: string) {
+  var cookiesStrings = []
+  var pos = 0
+  var start
+  var ch
+  var lastComma
+  var nextStart
+  var cookiesSeparatorFound
+
+  function skipWhitespace() {
+    while (pos < cookiesString.length && /\s/.test(cookiesString.charAt(pos))) {
+      pos += 1
+    }
+    return pos < cookiesString.length
+  }
+
+  function notSpecialChar() {
+    ch = cookiesString.charAt(pos)
+
+    return ch !== '=' && ch !== ';' && ch !== ','
+  }
+
+  while (pos < cookiesString.length) {
+    start = pos
+    cookiesSeparatorFound = false
+
+    while (skipWhitespace()) {
+      ch = cookiesString.charAt(pos)
+      if (ch === ',') {
+        // ',' is a cookie separator if we have later first '=', not ';' or ','
+        lastComma = pos
+        pos += 1
+
+        skipWhitespace()
+        nextStart = pos
+
+        while (pos < cookiesString.length && notSpecialChar()) {
+          pos += 1
+        }
+
+        // currently special character
+        if (pos < cookiesString.length && cookiesString.charAt(pos) === '=') {
+          // we found cookies separator
+          cookiesSeparatorFound = true
+          // pos is inside the next cookie, so back up and return it.
+          pos = nextStart
+          cookiesStrings.push(cookiesString.substring(start, lastComma))
+          start = pos
+        } else {
+          // in param ',' or param separator ';',
+          // we continue from that comma
+          pos = lastComma + 1
+        }
+      } else {
+        pos += 1
+      }
+    }
+
+    if (!cookiesSeparatorFound || pos >= cookiesString.length) {
+      cookiesStrings.push(cookiesString.substring(start, cookiesString.length))
+    }
+  }
+
+  return cookiesStrings
+}

test/integration/middleware-core/pages/responses/_middleware.js

@@ -22,7 +22,7 @@ export async function middleware(request, ev) {
 
   // Ensure deep can append to this value
   if (url.searchParams.get('cookie-me') === 'true') {
-    next.headers.append('set-cookie', 'chocochip')
+    next.headers.append('set-cookie', 'bar=chocochip')
   }
 
   // Sends a header

test/integration/middleware-core/pages/responses/deep/_middleware.js

@@ -4,6 +4,6 @@ export async function middleware(request, _event) {
   const next = NextResponse.next()
   next.headers.set('x-deep-header', 'valid')
   next.headers.append('x-append-me', 'deep')
-  next.headers.append('set-cookie', 'oatmeal')
+  next.headers.append('set-cookie', 'foo=oatmeal')
   return next
 }

test/integration/middleware-core/pages/rewrites/_middleware.js

@@ -8,7 +8,7 @@ export async function middleware(request) {
     if (!bucket) {
       bucket = Math.random() >= 0.5 ? 'a' : 'b'
       const response = NextResponse.rewrite(`/rewrites/${bucket}`)
-      response.cookie('bucket', bucket)
+      response.cookie('bucket', bucket, { maxAge: 10000 })
       return response
     }
 

test/integration/middleware-core/test/index.test.js

@@ -115,6 +115,8 @@ function rewriteTests(locale = '') {
     )
     const html = await res.text()
     const $ = cheerio.load(html)
+    // Set-Cookie header with Expires should not be split into two
+    expect(res.headers.raw()['set-cookie']).toHaveLength(1)
     const bucket = getCookieFromResponse(res, 'bucket')
     const expectedText = bucket === 'a' ? 'Welcome Page A' : 'Welcome Page B'
     const browser = await webdriver(
@@ -334,7 +336,10 @@ function responseTests(locale = '') {
     expect(res.headers.get('x-nested-header')).toBe('valid')
     expect(res.headers.get('x-deep-header')).toBe('valid')
     expect(res.headers.get('x-append-me')).toBe('top, deep')
-    expect(res.headers.raw()['set-cookie']).toEqual(['chocochip', 'oatmeal'])
+    expect(res.headers.raw()['set-cookie']).toEqual([
+      'bar=chocochip',
+      'foo=oatmeal',
+    ])
   })
 }
 

test/unit/split-cookies-string.test.ts

@@ -0,0 +1,143 @@
+import { splitCookiesString } from 'next/dist/server/web/utils'
+import cookie, { CookieSerializeOptions } from 'next/dist/compiled/cookie'
+
+function generateCookies(
+  ...cookieOptions: (CookieSerializeOptions & { name: string; value: string })[]
+) {
+  const cookies = cookieOptions.map((opts) =>
+    cookie.serialize(opts.name, opts.value, opts)
+  )
+  return {
+    joined: cookies.join(', '),
+    expected: cookies,
+  }
+}
+
+describe('splitCookiesString', () => {
+  describe('with a single cookie', () => {
+    it('should parse a plain value', () => {
+      const { joined, expected } = generateCookies({
+        name: 'foo',
+        value: 'bar',
+      })
+      const result = splitCookiesString(joined)
+      expect(result).toEqual(expected)
+    })
+
+    it('should parse expires', () => {
+      const { joined, expected } = generateCookies({
+        name: 'foo',
+        value: 'bar',
+        expires: new Date(),
+      })
+      const result = splitCookiesString(joined)
+      expect(result).toEqual(expected)
+    })
+
+    it('should parse max-age', () => {
+      const { joined, expected } = generateCookies({
+        name: 'foo',
+        value: 'bar',
+        maxAge: 10,
+      })
+      const result = splitCookiesString(joined)
+      expect(result).toEqual(expected)
+    })
+
+    it('should parse with all the options', () => {
+      const { joined, expected } = generateCookies({
+        name: 'foo',
+        value: 'bar',
+        expires: new Date(Date.now() + 10 * 1000),
+        maxAge: 10,
+        domain: 'https://foo.bar',
+        httpOnly: true,
+        path: '/path',
+        sameSite: 'lax',
+        secure: true,
+      })
+      const result = splitCookiesString(joined)
+      expect(result).toEqual(expected)
+    })
+  })
+
+  describe('with a mutliple cookies', () => {
+    it('should parse a plain value', () => {
+      const { joined, expected } = generateCookies(
+        {
+          name: 'foo',
+          value: 'bar',
+        },
+        {
+          name: 'x',
+          value: 'y',
+        }
+      )
+      const result = splitCookiesString(joined)
+      expect(result).toEqual(expected)
+    })
+
+    it('should parse expires', () => {
+      const { joined, expected } = generateCookies(
+        {
+          name: 'foo',
+          value: 'bar',
+          expires: new Date(),
+        },
+        {
+          name: 'x',
+          value: 'y',
+          expires: new Date(),
+        }
+      )
+      const result = splitCookiesString(joined)
+      expect(result).toEqual(expected)
+    })
+
+    it('should parse max-age', () => {
+      const { joined, expected } = generateCookies(
+        {
+          name: 'foo',
+          value: 'bar',
+          maxAge: 10,
+        },
+        {
+          name: 'x',
+          value: 'y',
+          maxAge: 10,
+        }
+      )
+      const result = splitCookiesString(joined)
+      expect(result).toEqual(expected)
+    })
+
+    it('should parse with all the options', () => {
+      const { joined, expected } = generateCookies(
+        {
+          name: 'foo',
+          value: 'bar',
+          expires: new Date(Date.now() + 10 * 1000),
+          maxAge: 10,
+          domain: 'https://foo.bar',
+          httpOnly: true,
+          path: '/path',
+          sameSite: 'lax',
+          secure: true,
+        },
+        {
+          name: 'x',
+          value: 'y',
+          expires: new Date(Date.now() + 20 * 1000),
+          maxAge: 20,
+          domain: 'https://x.y',
+          httpOnly: true,
+          path: '/path',
+          sameSite: 'strict',
+          secure: true,
+        }
+      )
+      const result = splitCookiesString(joined)
+      expect(result).toEqual(expected)
+    })
+  })
+})