Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split Set-Cookie header correctly #30560

Merged
merged 6 commits into from
Oct 28, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 6 additions & 7 deletions packages/next/server/next-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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)
}
}

Expand Down
78 changes: 77 additions & 1 deletion packages/next/server/web/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

@styfle styfle Oct 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a bunch of unit tests for this function so we can refactor in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added 👍

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
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
7 changes: 6 additions & 1 deletion test/integration/middleware-core/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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',
])
})
}

Expand Down
143 changes: 143 additions & 0 deletions test/unit/split-cookies-string.test.ts
Original file line number Diff line number Diff line change
@@ -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)
})
})
})