fix: set cookies followed by redirect() (#49965)

- Fixes #49237 fix NEXT-1120 Co-authored-by: Wyatt Johnson <[email protected]>
vercel · May 19, 2023 · 4b4925f · 4b4925f
1 parent ea9b1f5
commit 4b4925f
Show file tree

Hide file tree

Showing 12 changed files with 129 additions and 80 deletions.
diff --git 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<U extends string> = Error & {
   digest: `${typeof REDIRECT_ERROR_CODE};${RedirectType};${U}`
+  mutableCookies: ResponseCookies
 }
 
 export function getRedirectError(
   url: string,
   type: RedirectType
 ): RedirectError<typeof url> {
-  // eslint-disable-next-line no-throw-literal
-  const error = new Error(REDIRECT_ERROR_CODE)
-  ;(
-    error as RedirectError<typeof url>
-  ).digest = `${REDIRECT_ERROR_CODE};${type};${url}`
-  return error as RedirectError<typeof url>
+  const error = new Error(REDIRECT_ERROR_CODE) as RedirectError<typeof url>
+  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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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<InstanceType<typeof ResponseCookies>['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
@@ -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
@@ -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<InstanceType<typeof ResponseCookies>['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
@@ -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
@@ -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
@@ -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
@@ -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)