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
This commit is contained in:
parent
5b4ad4a1c1
commit
450552ddba
7 changed files with 235 additions and 12 deletions
|
@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
@ -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
|
||||
}
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
||||
}
|
||||
|
|
|
@ -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
|
||||
}
|
||||
|
||||
|
|
|
@ -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',
|
||||
])
|
||||
})
|
||||
}
|
||||
|
||||
|
|
143
test/unit/split-cookies-string.test.ts
Normal file
143
test/unit/split-cookies-string.test.ts
Normal file
|
@ -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)
|
||||
})
|
||||
})
|
||||
})
|
Loading…
Reference in a new issue