From 4b4925f4e8cf1d386ffa3f161c06fc60aa136d77 Mon Sep 17 00:00:00 2001 From: Steven Date: Thu, 18 May 2023 20:17:19 -0400 Subject: [PATCH] fix: set cookies followed by `redirect()` (#49965) - Fixes #49237 fix NEXT-1120 Co-authored-by: Wyatt Johnson <633002+wyattjoh@users.noreply.github.com> --- .../next/src/client/components/redirect.ts | 17 +++--- .../src/server/app-render/action-handler.ts | 11 ++++ .../next/src/server/app-render/app-render.tsx | 10 ++++ .../request-async-storage-wrapper.ts | 2 +- .../helpers/resolve-handler-error.ts | 2 +- .../future/route-modules/app-route/module.ts | 53 +++---------------- .../helpers/response-handlers.ts | 40 ++++++-------- .../adapters/request-cookies.ts | 43 ++++++++++++++- .../app-routes/app-custom-routes.test.ts | 2 +- .../app/enable-and-redirect/route.ts | 7 +++ .../draft-mode/draft-mode-edge.test.ts | 11 ++++ .../draft-mode/draft-mode-node.test.ts | 11 ++++ 12 files changed, 129 insertions(+), 80 deletions(-) create mode 100644 test/e2e/app-dir/draft-mode/app/enable-and-redirect/route.ts diff --git a/packages/next/src/client/components/redirect.ts b/packages/next/src/client/components/redirect.ts index 36f733004aca5..5c002a8842277 100644 --- a/packages/next/src/client/components/redirect.ts +++ b/packages/next/src/client/components/redirect.ts @@ -1,3 +1,6 @@ +import { requestAsyncStorage } from './request-async-storage' +import type { ResponseCookies } from '../../server/web/spec-extension/cookies' + const REDIRECT_ERROR_CODE = 'NEXT_REDIRECT' export enum RedirectType { @@ -7,18 +10,20 @@ export enum RedirectType { type RedirectError = Error & { digest: `${typeof REDIRECT_ERROR_CODE};${RedirectType};${U}` + mutableCookies: ResponseCookies } export function getRedirectError( url: string, type: RedirectType ): RedirectError { - // eslint-disable-next-line no-throw-literal - const error = new Error(REDIRECT_ERROR_CODE) - ;( - error as RedirectError - ).digest = `${REDIRECT_ERROR_CODE};${type};${url}` - return error as RedirectError + const error = new Error(REDIRECT_ERROR_CODE) as RedirectError + error.digest = `${REDIRECT_ERROR_CODE};${type};${url}` + const requestStore = requestAsyncStorage.getStore() + if (requestStore) { + error.mutableCookies = requestStore.mutableCookies + } + return error } /** diff --git a/packages/next/src/server/app-render/action-handler.ts b/packages/next/src/server/app-render/action-handler.ts index b934b841c7dba..e333785df4db2 100644 --- a/packages/next/src/server/app-render/action-handler.ts +++ b/packages/next/src/server/app-render/action-handler.ts @@ -22,6 +22,7 @@ import { FlightRenderResult } from './flight-render-result' import { ActionResult } from './types' import { ActionAsyncStorage } from '../../client/components/action-async-storage' import { filterReqHeaders, forbiddenHeaders } from '../lib/server-ipc/utils' +import { appendMutableCookies } from '../web/spec-extension/adapters/request-cookies' function nodeToWebReadableStream(nodeReadable: import('stream').Readable) { if (process.env.NEXT_RUNTIME !== 'edge') { @@ -377,6 +378,16 @@ export async function handleAction({ ) } + if (err.mutableCookies) { + const headers = new Headers() + + // If there were mutable cookies set, we need to set them on the + // response. + if (appendMutableCookies(headers, err.mutableCookies)) { + res.setHeader('set-cookie', Array.from(headers.values())) + } + } + res.setHeader('Location', redirectUrl) res.statusCode = 303 return new RenderResult('') diff --git a/packages/next/src/server/app-render/app-render.tsx b/packages/next/src/server/app-render/app-render.tsx index f1a9c2f5a6fab..b526e80ed1834 100644 --- a/packages/next/src/server/app-render/app-render.tsx +++ b/packages/next/src/server/app-render/app-render.tsx @@ -72,6 +72,7 @@ import { import { handleAction } from './action-handler' import { NEXT_DYNAMIC_NO_SSR_CODE } from '../../shared/lib/lazy-dynamic/no-ssr-error' import { warn } from '../../build/output/log' +import { appendMutableCookies } from '../web/spec-extension/adapters/request-cookies' export const isEdgeRuntime = process.env.NEXT_RUNTIME === 'edge' @@ -1505,6 +1506,15 @@ export async function renderToHTMLOrFlight( } if (isRedirectError(err)) { res.statusCode = 307 + if (err.mutableCookies) { + const headers = new Headers() + + // If there were mutable cookies set, we need to set them on the + // response. + if (appendMutableCookies(headers, err.mutableCookies)) { + res.setHeader('set-cookie', Array.from(headers.values())) + } + } res.setHeader('Location', getURLFromRedirectError(err)) } diff --git a/packages/next/src/server/async-storage/request-async-storage-wrapper.ts b/packages/next/src/server/async-storage/request-async-storage-wrapper.ts index 4d91f7f778065..53ea482fe96ab 100644 --- a/packages/next/src/server/async-storage/request-async-storage-wrapper.ts +++ b/packages/next/src/server/async-storage/request-async-storage-wrapper.ts @@ -41,7 +41,7 @@ function getMutableCookies( res: ServerResponse | BaseNextResponse | undefined ): ResponseCookies { const cookies = new RequestCookies(HeadersAdapter.from(headers)) - return MutableRequestCookiesAdapter.seal(cookies, res) + return MutableRequestCookiesAdapter.wrap(cookies, res) } export type RequestContext = { diff --git a/packages/next/src/server/future/route-modules/app-route/helpers/resolve-handler-error.ts b/packages/next/src/server/future/route-modules/app-route/helpers/resolve-handler-error.ts index 1e10a462feda0..293c53a342857 100644 --- a/packages/next/src/server/future/route-modules/app-route/helpers/resolve-handler-error.ts +++ b/packages/next/src/server/future/route-modules/app-route/helpers/resolve-handler-error.ts @@ -16,7 +16,7 @@ export function resolveHandlerError(err: any): Response | false { } // This is a redirect error! Send the redirect response. - return handleTemporaryRedirectResponse(redirect) + return handleTemporaryRedirectResponse(redirect, err.mutableCookies) } if (isNotFoundError(err)) { diff --git a/packages/next/src/server/future/route-modules/app-route/module.ts b/packages/next/src/server/future/route-modules/app-route/module.ts index 38a61a5995ef8..8ea2e64317bc1 100644 --- a/packages/next/src/server/future/route-modules/app-route/module.ts +++ b/packages/next/src/server/future/route-modules/app-route/module.ts @@ -31,10 +31,8 @@ import { RouteKind } from '../../route-kind' import * as Log from '../../../../build/output/log' import { autoImplementMethods } from './helpers/auto-implement-methods' import { getNonStaticMethods } from './helpers/get-non-static-methods' -import { SYMBOL_MODIFY_COOKIE_VALUES } from '../../../web/spec-extension/adapters/request-cookies' -import { ResponseCookies } from '../../../web/spec-extension/cookies' -import { HeadersAdapter } from '../../../web/spec-extension/adapters/headers' import { PrerenderManifest } from '../../../../build' +import { appendMutableCookies } from '../../../web/spec-extension/adapters/request-cookies' /** * AppRouteRouteHandlerContext is the context that is passed to the route @@ -358,54 +356,19 @@ export class AppRouteRouteModule extends RouteModule< // It's possible cookies were set in the handler, so we need // to merge the modified cookies and the returned response // here. - // TODO: Move this into a helper function. const requestStore = this.requestAsyncStorage.getStore() if (requestStore && requestStore.mutableCookies) { - const modifiedCookieValues = ( - requestStore.mutableCookies as any - )[SYMBOL_MODIFY_COOKIE_VALUES] as NonNullable< - ReturnType['get']> - >[] - if (modifiedCookieValues.length) { - // Return a new response that extends the response with - // the modified cookies as fallbacks. `res`' cookies - // will still take precedence. - const resCookies = new ResponseCookies( - HeadersAdapter.from(res.headers) + const headers = new Headers(res.headers) + if ( + appendMutableCookies( + headers, + requestStore.mutableCookies ) - const returnedCookies = resCookies.getAll() - - // Set the modified cookies as fallbacks. - for (const cookie of modifiedCookieValues) { - resCookies.set(cookie) - } - // Set the original cookies as the final values. - for (const cookie of returnedCookies) { - resCookies.set(cookie) - } - - const responseHeaders = new Headers({}) - // Set all the headers except for the cookies. - res.headers.forEach((value, key) => { - if (key.toLowerCase() !== 'set-cookie') { - responseHeaders.append(key, value) - } - }) - // Set the final cookies, need to append cookies one - // at a time otherwise it might not work in some browsers. - resCookies.getAll().forEach((cookie) => { - const tempCookies = new ResponseCookies(new Headers()) - tempCookies.set(cookie) - responseHeaders.append( - 'Set-Cookie', - tempCookies.toString() - ) - }) - + ) { return new Response(res.body, { status: res.status, statusText: res.statusText, - headers: responseHeaders, + headers, }) } } diff --git a/packages/next/src/server/future/route-modules/helpers/response-handlers.ts b/packages/next/src/server/future/route-modules/helpers/response-handlers.ts index 7fc2948e23ce8..699c29e92fe94 100644 --- a/packages/next/src/server/future/route-modules/helpers/response-handlers.ts +++ b/packages/next/src/server/future/route-modules/helpers/response-handlers.ts @@ -1,37 +1,29 @@ -export function handleTemporaryRedirectResponse(url: string): Response { - return new Response(null, { - status: 302, - statusText: 'Found', - headers: { - location: url, - }, - }) +import { appendMutableCookies } from '../../../web/spec-extension/adapters/request-cookies' +import { ResponseCookies } from '../../../web/spec-extension/cookies' + +export function handleTemporaryRedirectResponse( + url: string, + mutableCookies: ResponseCookies +): Response { + const headers = new Headers({ location: url }) + + appendMutableCookies(headers, mutableCookies) + + return new Response(null, { status: 307, headers }) } export function handleBadRequestResponse(): Response { - return new Response(null, { - status: 400, - statusText: 'Bad Request', - }) + return new Response(null, { status: 400 }) } export function handleNotFoundResponse(): Response { - return new Response(null, { - status: 404, - statusText: 'Not Found', - }) + return new Response(null, { status: 404 }) } export function handleMethodNotAllowedResponse(): Response { - return new Response(null, { - status: 405, - statusText: 'Method Not Allowed', - }) + return new Response(null, { status: 405 }) } export function handleInternalServerErrorResponse(): Response { - return new Response(null, { - status: 500, - statusText: 'Internal Server Error', - }) + return new Response(null, { status: 500 }) } diff --git a/packages/next/src/server/web/spec-extension/adapters/request-cookies.ts b/packages/next/src/server/web/spec-extension/adapters/request-cookies.ts index b4b0daa08ce33..1441b859e3cd8 100644 --- a/packages/next/src/server/web/spec-extension/adapters/request-cookies.ts +++ b/packages/next/src/server/web/spec-extension/adapters/request-cookies.ts @@ -40,14 +40,53 @@ export class RequestCookiesAdapter { } } -export const SYMBOL_MODIFY_COOKIE_VALUES = Symbol.for('next.mutated.cookies') +const SYMBOL_MODIFY_COOKIE_VALUES = Symbol.for('next.mutated.cookies') + +function getModifiedCookieValues(cookies: ResponseCookies): ResponseCookie[] { + const modified: ResponseCookie[] | undefined = (cookies as unknown as any)[ + SYMBOL_MODIFY_COOKIE_VALUES + ] + if (!modified || !Array.isArray(modified) || modified.length === 0) { + return [] + } + + return modified +} + +export function appendMutableCookies( + headers: Headers, + mutableCookies: ResponseCookies +): boolean { + const modifiedCookieValues = getModifiedCookieValues(mutableCookies) + if (modifiedCookieValues.length === 0) { + return false + } + + // Return a new response that extends the response with + // the modified cookies as fallbacks. `res`' cookies + // will still take precedence. + const resCookies = new ResponseCookies(headers) + const returnedCookies = resCookies.getAll() + + // Set the modified cookies as fallbacks. + for (const cookie of modifiedCookieValues) { + resCookies.set(cookie) + } + + // Set the original cookies as the final values. + for (const cookie of returnedCookies) { + resCookies.set(cookie) + } + + return true +} type ResponseCookie = NonNullable< ReturnType['get']> > export class MutableRequestCookiesAdapter { - public static seal( + public static wrap( cookies: RequestCookies, res: ServerResponse | BaseNextResponse | undefined ): ResponseCookies { diff --git a/test/e2e/app-dir/app-routes/app-custom-routes.test.ts b/test/e2e/app-dir/app-routes/app-custom-routes.test.ts index f6e10c04deb4b..2b63b14929073 100644 --- a/test/e2e/app-dir/app-routes/app-custom-routes.test.ts +++ b/test/e2e/app-dir/app-routes/app-custom-routes.test.ts @@ -394,7 +394,7 @@ createNextDescribe( redirect: 'manual', }) - expect(res.status).toEqual(302) + expect(res.status).toEqual(307) expect(res.headers.get('location')).toEqual('https://nextjs.org/') expect(await res.text()).toBeEmpty() }) diff --git a/test/e2e/app-dir/draft-mode/app/enable-and-redirect/route.ts b/test/e2e/app-dir/draft-mode/app/enable-and-redirect/route.ts new file mode 100644 index 0000000000000..27ac245daea8e --- /dev/null +++ b/test/e2e/app-dir/draft-mode/app/enable-and-redirect/route.ts @@ -0,0 +1,7 @@ +import { draftMode } from 'next/headers' +import { redirect } from 'next/navigation' + +export function GET() { + draftMode().enable() + return redirect('/some-other-page') +} diff --git a/test/e2e/app-dir/draft-mode/draft-mode-edge.test.ts b/test/e2e/app-dir/draft-mode/draft-mode-edge.test.ts index 88c251dfc8344..28e365bb1ec7c 100644 --- a/test/e2e/app-dir/draft-mode/draft-mode-edge.test.ts +++ b/test/e2e/app-dir/draft-mode/draft-mode-edge.test.ts @@ -25,6 +25,17 @@ createNextDescribe( expect(Cookie).toBeDefined() }) + it('should have set-cookie header with redirect location', async () => { + const res = await next.fetch('/enable-and-redirect', { + redirect: 'manual', + }) + expect(res.status).toBe(307) + expect(res.headers.get('location')).toContain('/some-other-page') + const h = res.headers.get('set-cookie') || '' + const c = h.split(';').find((c) => c.startsWith('__prerender_bypass')) + expect(c).toBeDefined() + }) + it('should be enabled from page when draft mode enabled', async () => { const opts = { headers: { Cookie } } const $ = await next.render$('/', {}, opts) diff --git a/test/e2e/app-dir/draft-mode/draft-mode-node.test.ts b/test/e2e/app-dir/draft-mode/draft-mode-node.test.ts index 658c812786451..27b178b1bd7c7 100644 --- a/test/e2e/app-dir/draft-mode/draft-mode-node.test.ts +++ b/test/e2e/app-dir/draft-mode/draft-mode-node.test.ts @@ -36,6 +36,17 @@ createNextDescribe( expect(Cookie).toBeDefined() }) + it('should have set-cookie header with redirect location', async () => { + const res = await next.fetch('/enable-and-redirect', { + redirect: 'manual', + }) + expect(res.status).toBe(307) + expect(res.headers.get('location')).toContain('/some-other-page') + const h = res.headers.get('set-cookie') || '' + const c = h.split(';').find((c) => c.startsWith('__prerender_bypass')) + expect(c).toBeDefined() + }) + it('should genenerate rand when draft mode enabled', async () => { const opts = { headers: { Cookie } } const $ = await next.render$('/', {}, opts)