From 450552ddbadd050b78d665448127c54b7e05c5fa Mon Sep 17 00:00:00 2001 From: George Karagkiaouris Date: Thu, 28 Oct 2021 13:46:58 -0400 Subject: [PATCH] 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](https://github.com/whatwg/fetch/issues/973#issuecomment-559678813) 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 | 13 +- packages/next/server/web/utils.ts | 78 +++++++++- .../pages/responses/_middleware.js | 2 +- .../pages/responses/deep/_middleware.js | 2 +- .../pages/rewrites/_middleware.js | 2 +- .../middleware-core/test/index.test.js | 7 +- test/unit/split-cookies-string.test.ts | 143 ++++++++++++++++++ 7 files changed, 235 insertions(+), 12 deletions(-) create mode 100644 test/unit/split-cookies-string.test.ts diff --git a/packages/next/server/next-server.ts b/packages/next/server/next-server.ts index 6b8db6173814..b4ce74c36472 100644 --- a/packages/next/server/next-server.ts +++ b/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) } } diff --git a/packages/next/server/web/utils.ts b/packages/next/server/web/utils.ts index 1f789163d66a..7829d0136039 100644 --- a/packages/next/server/web/utils.ts +++ b/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 +} diff --git a/test/integration/middleware-core/pages/responses/_middleware.js b/test/integration/middleware-core/pages/responses/_middleware.js index 6329017ec0d6..7a6f66ee09bd 100644 --- a/test/integration/middleware-core/pages/responses/_middleware.js +++ b/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 diff --git a/test/integration/middleware-core/pages/responses/deep/_middleware.js b/test/integration/middleware-core/pages/responses/deep/_middleware.js index 28974fdeb128..1e9a129cc9bb 100644 --- a/test/integration/middleware-core/pages/responses/deep/_middleware.js +++ b/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 } diff --git a/test/integration/middleware-core/pages/rewrites/_middleware.js b/test/integration/middleware-core/pages/rewrites/_middleware.js index 291b2be08af2..166bd6475165 100644 --- a/test/integration/middleware-core/pages/rewrites/_middleware.js +++ b/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 } diff --git a/test/integration/middleware-core/test/index.test.js b/test/integration/middleware-core/test/index.test.js index d359dfa15f50..026951dc81c5 100644 --- a/test/integration/middleware-core/test/index.test.js +++ b/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', + ]) }) } diff --git a/test/unit/split-cookies-string.test.ts b/test/unit/split-cookies-string.test.ts new file mode 100644 index 000000000000..b78368fdc9fb --- /dev/null +++ b/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) + }) + }) +})