From 71a5eb1e0039d764a498c570845e3635a8b18af7 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Thu, 30 Mar 2023 15:31:26 -0600 Subject: [PATCH 1/5] fix: fixes set-cookie header issues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - some runtimes combined these headers with ',', this was wrong 🙌 --- packages/next/src/server/next-server.ts | 14 ++++++++-- packages/next/src/server/send-response.ts | 4 ++- .../set-cookies/app/api/app/edge/route.js | 14 ++++++++++ .../app/api/app/experimental-edge/route.js | 14 ++++++++++ .../set-cookies/app/api/app/node/route.js | 12 ++++++++ test/e2e/app-dir/set-cookies/cookies.js | 4 +++ test/e2e/app-dir/set-cookies/next.config.js | 8 ++++++ .../set-cookies/pages/api/pages/edge.js | 14 ++++++++++ .../pages/api/pages/experimental-edge.js | 14 ++++++++++ .../set-cookies/pages/api/pages/node.js | 9 ++++++ .../app-dir/set-cookies/set-cookies.test.ts | 28 +++++++++++++++++++ 11 files changed, 131 insertions(+), 4 deletions(-) create mode 100644 test/e2e/app-dir/set-cookies/app/api/app/edge/route.js create mode 100644 test/e2e/app-dir/set-cookies/app/api/app/experimental-edge/route.js create mode 100644 test/e2e/app-dir/set-cookies/app/api/app/node/route.js create mode 100644 test/e2e/app-dir/set-cookies/cookies.js create mode 100644 test/e2e/app-dir/set-cookies/next.config.js create mode 100644 test/e2e/app-dir/set-cookies/pages/api/pages/edge.js create mode 100644 test/e2e/app-dir/set-cookies/pages/api/pages/experimental-edge.js create mode 100644 test/e2e/app-dir/set-cookies/pages/api/pages/node.js create mode 100644 test/e2e/app-dir/set-cookies/set-cookies.test.ts diff --git a/packages/next/src/server/next-server.ts b/packages/next/src/server/next-server.ts index ded15c59e9f0..523e75adc5d1 100644 --- a/packages/next/src/server/next-server.ts +++ b/packages/next/src/server/next-server.ts @@ -1877,7 +1877,14 @@ export default class NextNodeServer extends BaseServer { for (let [key, value] of result.response.headers) { if (key !== 'x-middleware-next') { - allHeaders.append(key, value) + // TODO: (wyattjoh) replace with native response iteration when we can upgrade undici + if (key.toLowerCase() === 'set-cookie') { + for (const setCookie of splitCookiesString(value)) { + allHeaders.append(key, setCookie) + } + } else { + allHeaders.append(key, value) + } } } @@ -2244,9 +2251,10 @@ export default class NextNodeServer extends BaseServer { params.res.statusMessage = result.response.statusText result.response.headers.forEach((value: string, key) => { - // the append handling is special cased for `set-cookie` + // 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 + params.res.setHeader(key, splitCookiesString(value)) } else { params.res.appendHeader(key, value) } diff --git a/packages/next/src/server/send-response.ts b/packages/next/src/server/send-response.ts index b5fe049f54b0..5b7c93406aeb 100644 --- a/packages/next/src/server/send-response.ts +++ b/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. @@ -23,7 +24,8 @@ 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 + res.setHeader(name, splitCookiesString(value)) } else { res.appendHeader(name, value) } diff --git a/test/e2e/app-dir/set-cookies/app/api/app/edge/route.js b/test/e2e/app-dir/set-cookies/app/api/app/edge/route.js new file mode 100644 index 000000000000..bcbfb7f2c283 --- /dev/null +++ b/test/e2e/app-dir/set-cookies/app/api/app/edge/route.js @@ -0,0 +1,14 @@ +// @ts-check + +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 }) +} diff --git a/test/e2e/app-dir/set-cookies/app/api/app/experimental-edge/route.js b/test/e2e/app-dir/set-cookies/app/api/app/experimental-edge/route.js new file mode 100644 index 000000000000..5d5de025a2f6 --- /dev/null +++ b/test/e2e/app-dir/set-cookies/app/api/app/experimental-edge/route.js @@ -0,0 +1,14 @@ +// @ts-check + +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 }) +} diff --git a/test/e2e/app-dir/set-cookies/app/api/app/node/route.js b/test/e2e/app-dir/set-cookies/app/api/app/node/route.js new file mode 100644 index 000000000000..07e9e98eb67c --- /dev/null +++ b/test/e2e/app-dir/set-cookies/app/api/app/node/route.js @@ -0,0 +1,12 @@ +// @ts-check + +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 }) +} diff --git a/test/e2e/app-dir/set-cookies/cookies.js b/test/e2e/app-dir/set-cookies/cookies.js new file mode 100644 index 000000000000..405589a069ba --- /dev/null +++ b/test/e2e/app-dir/set-cookies/cookies.js @@ -0,0 +1,4 @@ +export default [ + 'foo=bar; Expires=Wed, 01 Jan 2044 00:00:00 GMT', + 'bar=foo; Expires=Wed, 01 Jan 2044 00:00:00 GMT', +] diff --git a/test/e2e/app-dir/set-cookies/next.config.js b/test/e2e/app-dir/set-cookies/next.config.js new file mode 100644 index 000000000000..bf49894afd40 --- /dev/null +++ b/test/e2e/app-dir/set-cookies/next.config.js @@ -0,0 +1,8 @@ +/** + * @type {import('next').NextConfig} + */ +const nextConfig = { + experimental: { appDir: true }, +} + +module.exports = nextConfig diff --git a/test/e2e/app-dir/set-cookies/pages/api/pages/edge.js b/test/e2e/app-dir/set-cookies/pages/api/pages/edge.js new file mode 100644 index 000000000000..f7980393616e --- /dev/null +++ b/test/e2e/app-dir/set-cookies/pages/api/pages/edge.js @@ -0,0 +1,14 @@ +// @ts-check + +export const config = { runtime: 'edge' } + +import cookies from '../../../cookies' + +export default async function handler() { + const headers = new Headers() + for (const cookie of cookies) { + headers.append('set-cookie', cookie) + } + + return new Response(null, { headers }) +} diff --git a/test/e2e/app-dir/set-cookies/pages/api/pages/experimental-edge.js b/test/e2e/app-dir/set-cookies/pages/api/pages/experimental-edge.js new file mode 100644 index 000000000000..bff6458b77ac --- /dev/null +++ b/test/e2e/app-dir/set-cookies/pages/api/pages/experimental-edge.js @@ -0,0 +1,14 @@ +// @ts-check + +export const config = { runtime: 'experimental-edge' } + +import cookies from '../../../cookies' + +export default async function handler() { + const headers = new Headers() + for (const cookie of cookies) { + headers.append('set-cookie', cookie) + } + + return new Response(null, { headers }) +} diff --git a/test/e2e/app-dir/set-cookies/pages/api/pages/node.js b/test/e2e/app-dir/set-cookies/pages/api/pages/node.js new file mode 100644 index 000000000000..633c63457992 --- /dev/null +++ b/test/e2e/app-dir/set-cookies/pages/api/pages/node.js @@ -0,0 +1,9 @@ +// @ts-check + +import cookies from '../../../cookies' + +export default async function handler(_req, res) { + res.setHeader('set-cookie', cookies) + + res.json(null) +} diff --git a/test/e2e/app-dir/set-cookies/set-cookies.test.ts b/test/e2e/app-dir/set-cookies/set-cookies.test.ts new file mode 100644 index 000000000000..cdc42ed56b1c --- /dev/null +++ b/test/e2e/app-dir/set-cookies/set-cookies.test.ts @@ -0,0 +1,28 @@ +import { createNextDescribe } from 'e2e-utils' + +import cookies from './cookies' + +createNextDescribe( + 'set-cookies', + { + files: __dirname, + }, + ({ next }) => { + describe.each(['edge', 'experimental-edge', 'node'])( + 'for %s runtime', + (runtime) => { + describe.each(['pages', 'app'])('for /%s', (dir) => { + it('should set two set-cookie headers', async () => { + const res = await next.fetch(`/api/${dir}/${runtime}`) + + const headers: ReadonlyArray = + res.headers.getSetCookie?.() ?? res.headers.raw()['set-cookie'] + + expect(headers).toHaveLength(2) + expect(headers).toEqual(cookies) + }) + }) + } + ) + } +) From 9b9448ee0684872346fdb29acdad62cde7593c02 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Thu, 30 Mar 2023 16:47:42 -0600 Subject: [PATCH 2/5] fix: support set-cookie headers set by next.config.js --- packages/next/src/server/base-http/index.ts | 2 ++ packages/next/src/server/base-http/node.ts | 15 +++++++++ packages/next/src/server/base-http/web.ts | 16 ++++++++++ packages/next/src/server/next-server.ts | 18 ++++++++++- packages/next/src/server/send-response.ts | 4 ++- .../next/src/server/server-route-utils.ts | 2 +- .../set-cookies/app/api/app/edge/route.js | 2 -- .../app/api/app/experimental-edge/route.js | 2 -- .../set-cookies/app/api/app/node/route.js | 2 -- test/e2e/app-dir/set-cookies/cookies.js | 4 --- test/e2e/app-dir/set-cookies/cookies.mjs | 9 ++++++ test/e2e/app-dir/set-cookies/next.config.js | 8 ----- test/e2e/app-dir/set-cookies/next.config.mjs | 32 +++++++++++++++++++ .../set-cookies/pages/api/pages/edge.js | 2 -- .../pages/api/pages/experimental-edge.js | 2 -- .../set-cookies/pages/api/pages/node.js | 4 +-- .../app-dir/set-cookies/set-cookies.test.ts | 23 ++++++++++--- 17 files changed, 115 insertions(+), 32 deletions(-) delete mode 100644 test/e2e/app-dir/set-cookies/cookies.js create mode 100644 test/e2e/app-dir/set-cookies/cookies.mjs delete mode 100644 test/e2e/app-dir/set-cookies/next.config.js create mode 100644 test/e2e/app-dir/set-cookies/next.config.mjs diff --git a/packages/next/src/server/base-http/index.ts b/packages/next/src/server/base-http/index.ts index f72cdb3f34ae..1f2d8ad041e2 100644 --- a/packages/next/src/server/base-http/index.ts +++ b/packages/next/src/server/base-http/index.ts @@ -33,6 +33,8 @@ export abstract class BaseNextResponse { constructor(public destination: Destination) {} + abstract getHeaders(): Record + /** * Sets a value for the header overwriting existing values */ diff --git a/packages/next/src/server/base-http/node.ts b/packages/next/src/server/base-http/node.ts index 4cb033d0ca5b..f1970b3658bc 100644 --- a/packages/next/src/server/base-http/node.ts +++ b/packages/next/src/server/base-http/node.ts @@ -79,6 +79,21 @@ export class NodeNextResponse extends BaseNextResponse { this._res.statusMessage = value } + getHeaders(): Record { + const headers = this._res.getHeaders() + const result: Record = {} + + for (const key in headers) { + let value = headers[key] + if (typeof value === 'undefined') continue + + if (typeof value === 'number') result[key] = value.toString() + else result[key] = value + } + + return result + } + setHeader(name: string, value: string | string[]): this { this._res.setHeader(name, value) return this diff --git a/packages/next/src/server/base-http/web.ts b/packages/next/src/server/base-http/web.ts index e6705089c262..500938606d77 100644 --- a/packages/next/src/server/base-http/web.ts +++ b/packages/next/src/server/base-http/web.ts @@ -1,4 +1,5 @@ import type { IncomingHttpHeaders } from 'http' +import { splitCookiesString } from '../web/utils' import { BaseNextRequest, BaseNextResponse } from './index' @@ -55,6 +56,21 @@ export class WebNextResponse extends BaseNextResponse { super(transformStream.writable) } + getHeaders(): Record { + const result: Record = {} + + for (const [name, value] of this.headers.entries()) { + // TODO: (wyattjoh) replace with native response iteration when we can upgrade undici + if (name.toLowerCase() === 'set-cookie') { + result[name] = splitCookiesString(value) + } else { + result[name] = value + } + } + + return result + } + setHeader(name: string, value: string | string[]): this { this.headers.delete(name) for (const val of Array.isArray(value) ? value : [value]) { diff --git a/packages/next/src/server/next-server.ts b/packages/next/src/server/next-server.ts index 523e75adc5d1..1e0936f0a849 100644 --- a/packages/next/src/server/next-server.ts +++ b/packages/next/src/server/next-server.ts @@ -1875,6 +1875,20 @@ export default class NextNodeServer extends BaseServer { const allHeaders = new Headers() + // Copy over the response headers from the original response. These headers + // could be set in middleware or by the Next.js router. + const responseHeaders = params.response.getHeaders() + for (const key of Object.keys(responseHeaders)) { + const value = responseHeaders[key] + if (Array.isArray(value)) { + for (const v of value) { + allHeaders.append(key, v) + } + } else { + allHeaders.append(key, value) + } + } + for (let [key, value] of result.response.headers) { if (key !== 'x-middleware-next') { // TODO: (wyattjoh) replace with native response iteration when we can upgrade undici @@ -2254,7 +2268,9 @@ export default class NextNodeServer extends BaseServer { // The append handling is special cased for `set-cookie`. if (key.toLowerCase() === 'set-cookie') { // TODO: (wyattjoh) replace with native response iteration when we can upgrade undici - params.res.setHeader(key, splitCookiesString(value)) + for (const cookie of splitCookiesString(value)) { + params.res.appendHeader(key, cookie) + } } else { params.res.appendHeader(key, value) } diff --git a/packages/next/src/server/send-response.ts b/packages/next/src/server/send-response.ts index 5b7c93406aeb..a722f74bcfb4 100644 --- a/packages/next/src/server/send-response.ts +++ b/packages/next/src/server/send-response.ts @@ -25,7 +25,9 @@ export async function sendResponse( // The append handling is special cased for `set-cookie`. if (name.toLowerCase() === 'set-cookie') { // TODO: (wyattjoh) replace with native response iteration when we can upgrade undici - res.setHeader(name, splitCookiesString(value)) + for (const cookie of splitCookiesString(value)) { + res.appendHeader(name, cookie) + } } else { res.appendHeader(name, value) } diff --git a/packages/next/src/server/server-route-utils.ts b/packages/next/src/server/server-route-utils.ts index 5be9abb43b64..698412b1a432 100644 --- a/packages/next/src/server/server-route-utils.ts +++ b/packages/next/src/server/server-route-utils.ts @@ -92,7 +92,7 @@ export const createHeaderRoute = ({ key = compileNonPath(key, params) value = compileNonPath(value, params) } - res.setHeader(key, value) + res.appendHeader(key, value) } return { finished: false } }, diff --git a/test/e2e/app-dir/set-cookies/app/api/app/edge/route.js b/test/e2e/app-dir/set-cookies/app/api/app/edge/route.js index bcbfb7f2c283..4e93d886b94c 100644 --- a/test/e2e/app-dir/set-cookies/app/api/app/edge/route.js +++ b/test/e2e/app-dir/set-cookies/app/api/app/edge/route.js @@ -1,5 +1,3 @@ -// @ts-check - export const runtime = 'edge' import cookies from '../../../../cookies' diff --git a/test/e2e/app-dir/set-cookies/app/api/app/experimental-edge/route.js b/test/e2e/app-dir/set-cookies/app/api/app/experimental-edge/route.js index 5d5de025a2f6..dfce639be7ff 100644 --- a/test/e2e/app-dir/set-cookies/app/api/app/experimental-edge/route.js +++ b/test/e2e/app-dir/set-cookies/app/api/app/experimental-edge/route.js @@ -1,5 +1,3 @@ -// @ts-check - export const runtime = 'experimental-edge' import cookies from '../../../../cookies' diff --git a/test/e2e/app-dir/set-cookies/app/api/app/node/route.js b/test/e2e/app-dir/set-cookies/app/api/app/node/route.js index 07e9e98eb67c..d9b7f3e1e4dd 100644 --- a/test/e2e/app-dir/set-cookies/app/api/app/node/route.js +++ b/test/e2e/app-dir/set-cookies/app/api/app/node/route.js @@ -1,5 +1,3 @@ -// @ts-check - import cookies from '../../../../cookies' export async function GET() { diff --git a/test/e2e/app-dir/set-cookies/cookies.js b/test/e2e/app-dir/set-cookies/cookies.js deleted file mode 100644 index 405589a069ba..000000000000 --- a/test/e2e/app-dir/set-cookies/cookies.js +++ /dev/null @@ -1,4 +0,0 @@ -export default [ - 'foo=bar; Expires=Wed, 01 Jan 2044 00:00:00 GMT', - 'bar=foo; Expires=Wed, 01 Jan 2044 00:00:00 GMT', -] diff --git a/test/e2e/app-dir/set-cookies/cookies.mjs b/test/e2e/app-dir/set-cookies/cookies.mjs new file mode 100644 index 000000000000..34bd8323b78b --- /dev/null +++ b/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', +] diff --git a/test/e2e/app-dir/set-cookies/next.config.js b/test/e2e/app-dir/set-cookies/next.config.js deleted file mode 100644 index bf49894afd40..000000000000 --- a/test/e2e/app-dir/set-cookies/next.config.js +++ /dev/null @@ -1,8 +0,0 @@ -/** - * @type {import('next').NextConfig} - */ -const nextConfig = { - experimental: { appDir: true }, -} - -module.exports = nextConfig diff --git a/test/e2e/app-dir/set-cookies/next.config.mjs b/test/e2e/app-dir/set-cookies/next.config.mjs new file mode 100644 index 000000000000..80df0f2d6c91 --- /dev/null +++ b/test/e2e/app-dir/set-cookies/next.config.mjs @@ -0,0 +1,32 @@ +import { nextConfigHeaders } from './cookies.mjs' + +const headers = nextConfigHeaders.map((header) => ({ + key: 'Set-Cookie', + value: header, +})) + +console.log('debug:next.config.mjs:headers', headers) + +/** + * @type {import('next').NextConfig} + */ +const config = { + experimental: { appDir: true }, + async headers() { + return [ + { + source: '/:path*', + has: [ + { + type: 'query', + key: 'next-config-headers', + value: 'true', + }, + ], + headers, + }, + ] + }, +} + +export default config diff --git a/test/e2e/app-dir/set-cookies/pages/api/pages/edge.js b/test/e2e/app-dir/set-cookies/pages/api/pages/edge.js index f7980393616e..ff6b0e120919 100644 --- a/test/e2e/app-dir/set-cookies/pages/api/pages/edge.js +++ b/test/e2e/app-dir/set-cookies/pages/api/pages/edge.js @@ -1,5 +1,3 @@ -// @ts-check - export const config = { runtime: 'edge' } import cookies from '../../../cookies' diff --git a/test/e2e/app-dir/set-cookies/pages/api/pages/experimental-edge.js b/test/e2e/app-dir/set-cookies/pages/api/pages/experimental-edge.js index bff6458b77ac..80132349ceb4 100644 --- a/test/e2e/app-dir/set-cookies/pages/api/pages/experimental-edge.js +++ b/test/e2e/app-dir/set-cookies/pages/api/pages/experimental-edge.js @@ -1,5 +1,3 @@ -// @ts-check - export const config = { runtime: 'experimental-edge' } import cookies from '../../../cookies' diff --git a/test/e2e/app-dir/set-cookies/pages/api/pages/node.js b/test/e2e/app-dir/set-cookies/pages/api/pages/node.js index 633c63457992..605117aa1c45 100644 --- a/test/e2e/app-dir/set-cookies/pages/api/pages/node.js +++ b/test/e2e/app-dir/set-cookies/pages/api/pages/node.js @@ -1,9 +1,7 @@ -// @ts-check - import cookies from '../../../cookies' export default async function handler(_req, res) { - res.setHeader('set-cookie', cookies) + res.appendHeader('set-cookie', cookies) res.json(null) } diff --git a/test/e2e/app-dir/set-cookies/set-cookies.test.ts b/test/e2e/app-dir/set-cookies/set-cookies.test.ts index cdc42ed56b1c..1474f5458e67 100644 --- a/test/e2e/app-dir/set-cookies/set-cookies.test.ts +++ b/test/e2e/app-dir/set-cookies/set-cookies.test.ts @@ -1,6 +1,13 @@ import { createNextDescribe } from 'e2e-utils' -import cookies from './cookies' +import cookies, { nextConfigHeaders } from './cookies.mjs' + +function getSetCookieHeaders(res: globalThis.Response): ReadonlyArray { + return ( + (res.headers as any).getSetCookie?.() ?? + (res.headers as any).raw()['set-cookie'] + ) +} createNextDescribe( 'set-cookies', @@ -13,13 +20,21 @@ createNextDescribe( (runtime) => { describe.each(['pages', 'app'])('for /%s', (dir) => { it('should set two set-cookie headers', async () => { - const res = await next.fetch(`/api/${dir}/${runtime}`) + let res = await next.fetch(`/api/${dir}/${runtime}`) - const headers: ReadonlyArray = - res.headers.getSetCookie?.() ?? res.headers.raw()['set-cookie'] + let headers = getSetCookieHeaders(res) expect(headers).toHaveLength(2) expect(headers).toEqual(cookies) + + res = await next.fetch( + `/api/${dir}/${runtime}?next-config-headers=true` + ) + + headers = getSetCookieHeaders(res) + + expect(headers).toHaveLength(4) + expect(headers).toEqual([...nextConfigHeaders, ...cookies]) }) }) } From 82cc1ed973035e2c9363afda0b6a56515efd8364 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Thu, 30 Mar 2023 17:17:03 -0600 Subject: [PATCH 3/5] fix: repaired types --- packages/next/src/server/base-http/index.ts | 2 - packages/next/src/server/base-http/node.ts | 15 -------- packages/next/src/server/base-http/web.ts | 23 +++-------- packages/next/src/server/next-server.ts | 40 +++++++++----------- packages/next/src/server/web/next-url.ts | 6 ++- packages/next/src/server/web/types.ts | 7 +--- packages/next/src/server/web/utils.ts | 18 +++++---- packages/next/src/shared/lib/get-hostname.ts | 7 +++- test/e2e/app-dir/set-cookies/next.config.mjs | 2 - 9 files changed, 45 insertions(+), 75 deletions(-) diff --git a/packages/next/src/server/base-http/index.ts b/packages/next/src/server/base-http/index.ts index af38846fcae2..786f55293056 100644 --- a/packages/next/src/server/base-http/index.ts +++ b/packages/next/src/server/base-http/index.ts @@ -33,8 +33,6 @@ export abstract class BaseNextResponse { constructor(public destination: Destination) {} - abstract getHeaders(): Record - /** * Sets a value for the header overwriting existing values */ diff --git a/packages/next/src/server/base-http/node.ts b/packages/next/src/server/base-http/node.ts index dcee6be8fa3f..8bda81681a6f 100644 --- a/packages/next/src/server/base-http/node.ts +++ b/packages/next/src/server/base-http/node.ts @@ -80,21 +80,6 @@ export class NodeNextResponse extends BaseNextResponse { this._res.statusMessage = value } - getHeaders(): Record { - const headers = this._res.getHeaders() - const result: Record = {} - - for (const key in headers) { - let value = headers[key] - if (typeof value === 'undefined') continue - - if (typeof value === 'number') result[key] = value.toString() - else result[key] = value - } - - return result - } - setHeader(name: string, value: string | string[]): this { this._res.setHeader(name, value) return this diff --git a/packages/next/src/server/base-http/web.ts b/packages/next/src/server/base-http/web.ts index 500938606d77..563b79aa845b 100644 --- a/packages/next/src/server/base-http/web.ts +++ b/packages/next/src/server/base-http/web.ts @@ -1,5 +1,5 @@ -import type { IncomingHttpHeaders } from 'http' -import { splitCookiesString } from '../web/utils' +import type { IncomingHttpHeaders, OutgoingHttpHeaders } from 'http' +import { toNodeHeaders } from '../web/utils' import { BaseNextRequest, BaseNextResponse } from './index' @@ -56,21 +56,6 @@ export class WebNextResponse extends BaseNextResponse { super(transformStream.writable) } - getHeaders(): Record { - const result: Record = {} - - for (const [name, value] of this.headers.entries()) { - // TODO: (wyattjoh) replace with native response iteration when we can upgrade undici - if (name.toLowerCase() === 'set-cookie') { - result[name] = splitCookiesString(value) - } else { - result[name] = value - } - } - - return result - } - setHeader(name: string, value: string | string[]): this { this.headers.delete(name) for (const val of Array.isArray(value) ? value : [value]) { @@ -90,6 +75,10 @@ export class WebNextResponse extends BaseNextResponse { return this.headers.get(name) ?? undefined } + getHeaders(): OutgoingHttpHeaders { + return toNodeHeaders(this.headers) + } + hasHeader(name: string): boolean { return this.headers.has(name) } diff --git a/packages/next/src/server/next-server.ts b/packages/next/src/server/next-server.ts index 1e0936f0a849..cfe9b9114273 100644 --- a/packages/next/src/server/next-server.ts +++ b/packages/next/src/server/next-server.ts @@ -70,7 +70,7 @@ import { normalizePagePath } from '../shared/lib/page-path/normalize-page-path' import { loadComponents } from './load-components' import isError, { getProperError } from '../lib/is-error' import { FontManifest } from './font-utils' -import { splitCookiesString, toNodeHeaders } from './web/utils' +import { fromNodeHeaders, splitCookiesString, toNodeHeaders } from './web/utils' import { relativizeURL } from '../shared/lib/router/utils/relativize-url' import { prepareDestination } from '../shared/lib/router/utils/prepare-destination' import { getMiddlewareRouteMatcher } from '../shared/lib/router/utils/middleware-route-matcher' @@ -1873,32 +1873,22 @@ export default class NextNodeServer extends BaseServer { onWarning: params.onWarning, }) - const allHeaders = new Headers() - // Copy over the response headers from the original response. These headers // could be set in middleware or by the Next.js router. - const responseHeaders = params.response.getHeaders() - for (const key of Object.keys(responseHeaders)) { - const value = responseHeaders[key] - if (Array.isArray(value)) { - for (const v of value) { - allHeaders.append(key, v) - } - } else { - allHeaders.append(key, value) - } - } + const allHeaders = new Headers( + fromNodeHeaders(params.response.getHeaders()) + ) for (let [key, value] of result.response.headers) { - if (key !== 'x-middleware-next') { - // TODO: (wyattjoh) replace with native response iteration when we can upgrade undici - if (key.toLowerCase() === 'set-cookie') { - for (const setCookie of splitCookiesString(value)) { - allHeaders.append(key, setCookie) - } - } else { - allHeaders.append(key, value) + if (key === 'x-middleware-next') continue + + // TODO: (wyattjoh) replace with native response iteration when we can upgrade undici + if (key.toLowerCase() === 'set-cookie') { + for (const setCookie of splitCookiesString(value)) { + allHeaders.append(key, setCookie) } + } else { + allHeaders.append(key, value) } } @@ -2059,7 +2049,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) + } } } diff --git a/packages/next/src/server/web/next-url.ts b/packages/next/src/server/web/next-url.ts index d74dc1d58065..a0704a1db980 100644 --- a/packages/next/src/server/web/next-url.ts +++ b/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 diff --git a/packages/next/src/server/web/types.ts b/packages/next/src/server/web/types.ts index 9aec76179da2..403baecc89da 100644 --- a/packages/next/src/server/web/types.ts +++ b/packages/next/src/server/web/types.ts @@ -3,10 +3,7 @@ import type { NextRequest } from '../web/spec-extension/request' import type { NextFetchEvent } from '../web/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?: { @@ -16,7 +13,7 @@ export interface RequestData { latitude?: string longitude?: string } - headers: NodeHeaders + headers: OutgoingHttpHeaders ip?: string method: string nextConfig?: { diff --git a/packages/next/src/server/web/utils.ts b/packages/next/src/server/web/utils.ts index 9249d9704f16..165749c7b176 100644 --- a/packages/next/src/server/web/utils.ts +++ b/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 @@ -89,13 +92,14 @@ 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 = {} if (headers) { for (const [key, value] of headers.entries()) { - result[key] = value if (key.toLowerCase() === 'set-cookie') { result[key] = splitCookiesString(value) + } else { + result[key] = value } } } diff --git a/packages/next/src/shared/lib/get-hostname.ts b/packages/next/src/shared/lib/get-hostname.ts index 0f642aa97eab..ff40ed42e117 100644 --- a/packages/next/src/shared/lib/get-hostname.ts +++ b/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. @@ -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() } diff --git a/test/e2e/app-dir/set-cookies/next.config.mjs b/test/e2e/app-dir/set-cookies/next.config.mjs index 80df0f2d6c91..be80ea961eac 100644 --- a/test/e2e/app-dir/set-cookies/next.config.mjs +++ b/test/e2e/app-dir/set-cookies/next.config.mjs @@ -5,8 +5,6 @@ const headers = nextConfigHeaders.map((header) => ({ value: header, })) -console.log('debug:next.config.mjs:headers', headers) - /** * @type {import('next').NextConfig} */ From 5c055ab15736e4f42ae0db1838418d8768f828bd Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Thu, 30 Mar 2023 18:44:05 -0600 Subject: [PATCH 4/5] fix: handle cases with edge/middleware better --- packages/next/src/server/base-server.ts | 15 +++--- packages/next/src/server/next-server.ts | 52 +++++++------------ .../next/src/server/server-route-utils.ts | 7 ++- 3 files changed, 35 insertions(+), 39 deletions(-) diff --git a/packages/next/src/server/base-server.ts b/packages/next/src/server/base-server.ts index 28c0df342759..76d10de0c749 100644 --- a/packages/next/src/server/base-server.ts +++ b/packages/next/src/server/base-server.ts @@ -590,12 +590,15 @@ export default abstract class Server { !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 + : []), + ]), ] } } diff --git a/packages/next/src/server/next-server.ts b/packages/next/src/server/next-server.ts index cfe9b9114273..b1e480bbf9ab 100644 --- a/packages/next/src/server/next-server.ts +++ b/packages/next/src/server/next-server.ts @@ -70,7 +70,7 @@ import { normalizePagePath } from '../shared/lib/page-path/normalize-page-path' import { loadComponents } from './load-components' import isError, { getProperError } from '../lib/is-error' import { FontManifest } from './font-utils' -import { fromNodeHeaders, splitCookiesString, toNodeHeaders } from './web/utils' +import { splitCookiesString, toNodeHeaders } from './web/utils' import { relativizeURL } from '../shared/lib/router/utils/relativize-url' import { prepareDestination } from '../shared/lib/router/utils/prepare-destination' import { getMiddlewareRouteMatcher } from '../shared/lib/router/utils/middleware-route-matcher' @@ -1873,25 +1873,6 @@ export default class NextNodeServer extends BaseServer { onWarning: params.onWarning, }) - // Copy over the response headers from the original response. These headers - // could be set in middleware or by the Next.js router. - const allHeaders = new Headers( - fromNodeHeaders(params.response.getHeaders()) - ) - - for (let [key, value] of result.response.headers) { - if (key === 'x-middleware-next') continue - - // TODO: (wyattjoh) replace with native response iteration when we can upgrade undici - if (key.toLowerCase() === 'set-cookie') { - for (const setCookie of splitCookiesString(value)) { - allHeaders.append(key, setCookie) - } - } else { - allHeaders.append(key, value) - } - } - if (!this.renderOpts.dev) { result.waitUntil.catch((error) => { console.error(`Uncaught: middleware waitUntil errored`, error) @@ -1901,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 } @@ -2258,7 +2244,9 @@ 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) => { + // 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') { // TODO: (wyattjoh) replace with native response iteration when we can upgrade undici diff --git a/packages/next/src/server/server-route-utils.ts b/packages/next/src/server/server-route-utils.ts index 698412b1a432..9cef9bf532cd 100644 --- a/packages/next/src/server/server-route-utils.ts +++ b/packages/next/src/server/server-route-utils.ts @@ -92,7 +92,12 @@ export const createHeaderRoute = ({ key = compileNonPath(key, params) value = compileNonPath(value, params) } - res.appendHeader(key, value) + + if (key.toLowerCase() === 'set-cookie') { + res.appendHeader(key, value) + } else { + res.setHeader(key, value) + } } return { finished: false } }, From e49ce219ce59c1faa47eed60d416cbffd770c835 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Thu, 30 Mar 2023 20:12:29 -0600 Subject: [PATCH 5/5] fix: handled case when page is cached --- packages/next/src/export/worker.ts | 8 ++++++-- packages/next/src/server/base-server.ts | 5 +++-- packages/next/src/server/response-cache/types.ts | 3 ++- packages/next/src/server/web/utils.ts | 7 ++++++- 4 files changed, 17 insertions(+), 6 deletions(-) diff --git a/packages/next/src/export/worker.ts b/packages/next/src/export/worker.ts index 19732f8f93af..cb166e94038a 100644 --- a/packages/next/src/export/worker.ts +++ b/packages/next/src/export/worker.ts @@ -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. @@ -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' loadRequireHook() @@ -97,7 +99,7 @@ interface ExportPageResults { fromBuildExportRevalidate?: number | false fromBuildExportMeta?: { status?: number - headers?: Record + headers?: OutgoingHttpHeaders } error?: boolean ssgNotFound?: boolean @@ -410,6 +412,8 @@ export default async function exportPage({ // Call the handler with the request and context from the module. const response = await module.route.handler.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? @@ -422,7 +426,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 diff --git a/packages/next/src/server/base-server.ts b/packages/next/src/server/base-server.ts index 76d10de0c749..b7d399041e5c 100644 --- a/packages/next/src/server/base-server.ts +++ b/packages/next/src/server/base-server.ts @@ -97,6 +97,7 @@ import { I18NProvider } from './future/helpers/i18n-provider' import { sendResponse } from './send-response' import { RouteKind } from './future/route-kind' import { handleInternalServerErrorResponse } from './future/helpers/response-handlers' +import { fromNodeHeaders, toNodeHeaders } from './web/utils' export type FindComponentsResult = { components: LoadComponentsReturnType @@ -1544,7 +1545,7 @@ export default abstract class Server { 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 } @@ -1919,7 +1920,7 @@ export default abstract class Server { req, res, new Response(cachedData.body, { - headers: new Headers((cachedData.headers || {}) as any), + headers: fromNodeHeaders(cachedData.headers), status: cachedData.status || 200, }) ) diff --git a/packages/next/src/server/response-cache/types.ts b/packages/next/src/server/response-cache/types.ts index 3b81c4ba0d78..2ca7f700e955 100644 --- a/packages/next/src/server/response-cache/types.ts +++ b/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 { @@ -41,7 +42,7 @@ export interface CachedRouteValue { // expects that type instead of a string body: Buffer status: number - headers: Record + headers: OutgoingHttpHeaders } export interface CachedImageValue { diff --git a/packages/next/src/server/web/utils.ts b/packages/next/src/server/web/utils.ts index 165749c7b176..a87a225d4920 100644 --- a/packages/next/src/server/web/utils.ts +++ b/packages/next/src/server/web/utils.ts @@ -94,10 +94,15 @@ export function splitCookiesString(cookiesString: string) { export function toNodeHeaders(headers?: Headers): OutgoingHttpHeaders { const result: OutgoingHttpHeaders = {} + const cookies: string[] = [] if (headers) { for (const [key, value] of headers.entries()) { 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 }