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 1 commit
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
3 changes: 2 additions & 1 deletion 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 { splitCookiesString } from './web/utils'

const getCustomRouteMatcher = pathMatch(true)

Expand Down Expand Up @@ -1171,7 +1172,7 @@ export default class Server {
for (const [key, value] of result.response.headers.entries()) {
if (key !== 'content-encoding') {
if (key.toLowerCase() === 'set-cookie') {
res.setHeader(key, value.split(', '))
res.setHeader(key, splitCookiesString(value))
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.

We need this added to toNodeHeaders() too:

if (key.toLowerCase() === 'set-cookie') {
result[key] = value.split(', ')
}

} else {
res.setHeader(key, value)
}
Expand Down
76 changes: 76 additions & 0 deletions packages/next/server/web/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,79 @@ export function toNodeHeaders(headers?: Headers): NodeHeaders {
}
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 @@ -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
2 changes: 2 additions & 0 deletions 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