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

fix: correctly handle Set-Cookie headers #47718

Merged
merged 7 commits into from Mar 31, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
8 changes: 6 additions & 2 deletions packages/next/src/export/worker.ts
Expand Up @@ -8,6 +8,7 @@ import type {
ExportPathMap,
NextConfigComplete,
} from '../server/config-shared'
import type { OutgoingHttpHeaders } from 'http'

// `NEXT_PREBUNDLED_REACT` env var is inherited from parent process,
// then override react packages here for export worker.
Expand Down Expand Up @@ -49,6 +50,7 @@ import { NEXT_DYNAMIC_NO_SSR_CODE } from '../shared/lib/lazy-dynamic/no-ssr-erro
import { mockRequest } from '../server/lib/mock-request'
import { NodeNextRequest } from '../server/base-http/node'
import { isAppRouteRoute } from '../lib/is-app-route-route'
import { toNodeHeaders } from '../server/web/utils'
import { RouteModuleLoader } from '../server/future/helpers/module-loader/route-module-loader'

loadRequireHook()
Expand Down Expand Up @@ -98,7 +100,7 @@ interface ExportPageResults {
fromBuildExportRevalidate?: number | false
fromBuildExportMeta?: {
status?: number
headers?: Record<string, string>
headers?: OutgoingHttpHeaders
}
error?: boolean
ssgNotFound?: boolean
Expand Down Expand Up @@ -411,6 +413,8 @@ export default async function exportPage({
// Call the handler with the request and context from the module.
const response = await module.handle(request, context)

// TODO: (wyattjoh) if cookie headers are present, should we bail?

// we don't consider error status for static generation
// except for 404
// TODO: do we want to cache other status codes?
Expand All @@ -423,7 +427,7 @@ export default async function exportPage({
context.staticGenerationContext.store?.revalidate || false

results.fromBuildExportRevalidate = revalidate
const headers = Object.fromEntries(response.headers)
const headers = toNodeHeaders(response.headers)

if (!headers['content-type'] && body.type) {
headers['content-type'] = body.type
Expand Down
3 changes: 2 additions & 1 deletion packages/next/src/server/base-http/web.ts
@@ -1,4 +1,5 @@
import type { IncomingHttpHeaders, OutgoingHttpHeaders } from 'http'
import { toNodeHeaders } from '../web/utils'

import { BaseNextRequest, BaseNextResponse } from './index'

Expand Down Expand Up @@ -75,7 +76,7 @@ export class WebNextResponse extends BaseNextResponse<WritableStream> {
}

getHeaders(): OutgoingHttpHeaders {
return Object.fromEntries(this.headers.entries())
return toNodeHeaders(this.headers)
}

hasHeader(name: string): boolean {
Expand Down
20 changes: 12 additions & 8 deletions packages/next/src/server/base-server.ts
Expand Up @@ -99,6 +99,7 @@ import { I18NProvider } from './future/helpers/i18n-provider'
import { sendResponse } from './send-response'
import { RouteKind } from './future/route-kind'
import { handleInternalServerErrorResponse } from './future/route-modules/helpers/response-handlers'
import { fromNodeHeaders, toNodeHeaders } from './web/utils'

export type FindComponentsResult = {
components: LoadComponentsReturnType
Expand Down Expand Up @@ -592,12 +593,15 @@ export default abstract class Server<ServerOptions extends Options = Options> {
!val.every((item, idx) => item === middlewareValue[idx])
) {
val = [
...(middlewareValue || []),
...(typeof val === 'string'
? [val]
: Array.isArray(val)
? val
: []),
// TODO: (wyattjoh) find out why this is called multiple times resulting in duplicate cookies being added
...new Set([
...(middlewareValue || []),
...(typeof val === 'string'
? [val]
: Array.isArray(val)
? val
: []),
]),
]
}
}
Expand Down Expand Up @@ -1544,7 +1548,7 @@ export default abstract class Server<ServerOptions extends Options = Options> {
const blob = await response.blob()

// Copy the headers from the response.
const headers = Object.fromEntries(response.headers)
const headers = toNodeHeaders(response.headers)
if (!headers['content-type'] && blob.type) {
headers['content-type'] = blob.type
}
Expand Down Expand Up @@ -1919,7 +1923,7 @@ export default abstract class Server<ServerOptions extends Options = Options> {
req,
res,
new Response(cachedData.body, {
headers: new Headers((cachedData.headers || {}) as any),
headers: fromNodeHeaders(cachedData.headers),
status: cachedData.status || 200,
})
)
Expand Down
52 changes: 29 additions & 23 deletions packages/next/src/server/next-server.ts
Expand Up @@ -1873,14 +1873,6 @@ export default class NextNodeServer extends BaseServer {
onWarning: params.onWarning,
})

const allHeaders = new Headers()

for (let [key, value] of result.response.headers) {
if (key !== 'x-middleware-next') {
allHeaders.append(key, value)
}
}

if (!this.renderOpts.dev) {
result.waitUntil.catch((error) => {
console.error(`Uncaught: middleware waitUntil errored`, error)
Expand All @@ -1890,19 +1882,24 @@ export default class NextNodeServer extends BaseServer {
if (!result) {
this.render404(params.request, params.response, params.parsed)
return { finished: true }
} else {
for (let [key, value] of allHeaders) {
result.response.headers.set(key, value)

if (key.toLowerCase() === 'set-cookie') {
addRequestMeta(
params.request,
'_nextMiddlewareCookie',
splitCookiesString(value)
)
}
}

for (let [key, value] of result.response.headers) {
if (key.toLowerCase() !== 'set-cookie') continue

// Clear existing header.
result.response.headers.delete(key)

// Append each cookie individually.
const cookies = splitCookiesString(value)
for (const cookie of cookies) {
result.response.headers.append(key, cookie)
}

// Add cookies to request meta.
addRequestMeta(params.request, '_nextMiddlewareCookie', cookies)
}

return result
}

Expand Down Expand Up @@ -2038,7 +2035,11 @@ export default class NextNodeServer extends BaseServer {
continue
}
if (key !== 'content-encoding' && value !== undefined) {
res.setHeader(key, value)
if (typeof value === 'number') {
res.setHeader(key, value.toString())
} else {
res.setHeader(key, value)
}
}
}

Expand Down Expand Up @@ -2243,10 +2244,15 @@ export default class NextNodeServer extends BaseServer {
params.res.statusCode = result.response.status
params.res.statusMessage = result.response.statusText

result.response.headers.forEach((value: string, key) => {
// the append handling is special cased for `set-cookie`
// TODO: (wyattjoh) investigate improving this

result.response.headers.forEach((value, key) => {
// The append handling is special cased for `set-cookie`.
if (key.toLowerCase() === 'set-cookie') {
params.res.setHeader(key, value)
// TODO: (wyattjoh) replace with native response iteration when we can upgrade undici
for (const cookie of splitCookiesString(value)) {
params.res.appendHeader(key, cookie)
}
} else {
params.res.appendHeader(key, value)
}
Expand Down
3 changes: 2 additions & 1 deletion packages/next/src/server/response-cache/types.ts
@@ -1,3 +1,4 @@
import type { OutgoingHttpHeaders } from 'http'
import type RenderResult from '../render-result'

export interface ResponseCacheBase {
Expand Down Expand Up @@ -41,7 +42,7 @@ export interface CachedRouteValue {
// expects that type instead of a string
body: Buffer
status: number
headers: Record<string, undefined | string | string[]>
headers: OutgoingHttpHeaders
}

export interface CachedImageValue {
Expand Down
6 changes: 5 additions & 1 deletion packages/next/src/server/send-response.ts
@@ -1,5 +1,6 @@
import type { BaseNextRequest, BaseNextResponse } from './base-http'
import type { NodeNextResponse } from './base-http/node'
import { splitCookiesString } from './web/utils'

/**
* Sends the response on the underlying next response object.
Expand All @@ -23,7 +24,10 @@ export async function sendResponse(
response.headers?.forEach((value, name) => {
// The append handling is special cased for `set-cookie`.
if (name.toLowerCase() === 'set-cookie') {
res.setHeader(name, value)
// TODO: (wyattjoh) replace with native response iteration when we can upgrade undici
for (const cookie of splitCookiesString(value)) {
res.appendHeader(name, cookie)
}
} else {
res.appendHeader(name, value)
}
Expand Down
7 changes: 6 additions & 1 deletion packages/next/src/server/server-route-utils.ts
Expand Up @@ -92,7 +92,12 @@ export const createHeaderRoute = ({
key = compileNonPath(key, params)
value = compileNonPath(value, params)
}
res.setHeader(key, value)

if (key.toLowerCase() === 'set-cookie') {
res.appendHeader(key, value)
} else {
res.setHeader(key, value)
}
}
return { finished: false }
},
Expand Down
6 changes: 4 additions & 2 deletions packages/next/src/server/web/next-url.ts
@@ -1,13 +1,15 @@
import type { OutgoingHttpHeaders } from 'http'
import type { DomainLocale, I18NConfig } from '../config-shared'
import type { I18NProvider } from '../future/helpers/i18n-provider'

import { detectDomainLocale } from '../../shared/lib/i18n/detect-domain-locale'
import { formatNextPathnameInfo } from '../../shared/lib/router/utils/format-next-pathname-info'
import { getHostname } from '../../shared/lib/get-hostname'
import { getNextPathnameInfo } from '../../shared/lib/router/utils/get-next-pathname-info'
import type { I18NProvider } from '../future/helpers/i18n-provider'

interface Options {
base?: string | URL
headers?: { [key: string]: string | string[] | undefined }
headers?: OutgoingHttpHeaders
forceLocale?: boolean
nextConfig?: {
basePath?: string
Expand Down
7 changes: 2 additions & 5 deletions packages/next/src/server/web/types.ts
Expand Up @@ -3,10 +3,7 @@ import type { NextRequest } from './spec-extension/request'
import type { NextFetchEvent } from './spec-extension/fetch-event'
import type { NextResponse } from './spec-extension/response'
import type { CloneableBody } from '../body-streams'

export interface NodeHeaders {
[header: string]: string | string[] | undefined
}
import type { OutgoingHttpHeaders } from 'http'

export interface RequestData {
geo?: {
Expand All @@ -16,7 +13,7 @@ export interface RequestData {
latitude?: string
longitude?: string
}
headers: NodeHeaders
headers: OutgoingHttpHeaders
ip?: string
method: string
nextConfig?: {
Expand Down
25 changes: 17 additions & 8 deletions packages/next/src/server/web/utils.ts
@@ -1,13 +1,16 @@
import type { NodeHeaders } from './types'
import type { OutgoingHttpHeaders } from 'http'

export function fromNodeHeaders(object: NodeHeaders): Headers {
export function fromNodeHeaders(object: OutgoingHttpHeaders): Headers {
const headers = new Headers()
for (let [key, value] of Object.entries(object)) {
const values = Array.isArray(value) ? value : [value]
for (let v of values) {
if (v !== undefined) {
headers.append(key, v)
if (typeof v === 'undefined') continue
if (typeof v === 'number') {
v = v.toString()
}

headers.append(key, v)
}
}
return headers
Expand Down Expand Up @@ -89,13 +92,19 @@ export function splitCookiesString(cookiesString: string) {
return cookiesStrings
}

export function toNodeHeaders(headers?: Headers): NodeHeaders {
const result: NodeHeaders = {}
export function toNodeHeaders(headers?: Headers): OutgoingHttpHeaders {
const result: OutgoingHttpHeaders = {}
const cookies: string[] = []
if (headers) {
for (const [key, value] of headers.entries()) {
result[key] = value
if (key.toLowerCase() === 'set-cookie') {
result[key] = splitCookiesString(value)
// We may have gotten a comma joined string of cookies, or multiple
// set-cookie headers. We need to merge them into one header array
// to represent all the cookies.
cookies.push(...splitCookiesString(value))
result[key] = cookies
} else {
result[key] = value
}
}
}
Expand Down
7 changes: 5 additions & 2 deletions packages/next/src/shared/lib/get-hostname.ts
@@ -1,3 +1,5 @@
import type { OutgoingHttpHeaders } from 'http'

/**
* Takes an object with a hostname property (like a parsed URL) and some
* headers that may contain Host and returns the preferred hostname.
Expand All @@ -6,9 +8,10 @@
*/
export function getHostname(
parsed: { hostname?: string | null },
headers?: { [key: string]: string | string[] | undefined }
headers?: OutgoingHttpHeaders
) {
return ((!Array.isArray(headers?.host) && headers?.host) || parsed.hostname)
?.split(':')[0]
?.toString()
.split(':')[0]
.toLowerCase()
}
12 changes: 12 additions & 0 deletions test/e2e/app-dir/set-cookies/app/api/app/edge/route.js
@@ -0,0 +1,12 @@
export const runtime = 'edge'

import cookies from '../../../../cookies'

export async function GET() {
const headers = new Headers()
for (const cookie of cookies) {
headers.append('set-cookie', cookie)
}

return new Response(null, { headers })
}
@@ -0,0 +1,12 @@
export const runtime = 'experimental-edge'

import cookies from '../../../../cookies'

export async function GET() {
const headers = new Headers()
for (const cookie of cookies) {
headers.append('set-cookie', cookie)
}

return new Response(null, { headers })
}
10 changes: 10 additions & 0 deletions test/e2e/app-dir/set-cookies/app/api/app/node/route.js
@@ -0,0 +1,10 @@
import cookies from '../../../../cookies'

export async function GET() {
const headers = new Headers()
for (const cookie of cookies) {
headers.append('set-cookie', cookie)
}

return new Response(null, { headers })
}
9 changes: 9 additions & 0 deletions test/e2e/app-dir/set-cookies/cookies.mjs
@@ -0,0 +1,9 @@
export default [
'foo=bar; Expires=Wed, 01 Jan 2044 00:00:00 GMT',
'bar=foo; Expires=Wed, 01 Jan 2044 00:00:00 GMT',
]

export const nextConfigHeaders = [
'fooNextConfig=bar; Expires=Wed, 01 Jan 2044 00:00:00 GMT',
'barNextConfig=foo; Expires=Wed, 01 Jan 2044 00:00:00 GMT',
]