fix: handle malformed location header when using next/navigation redirect API (#3332)

Author: pieh

src/run/augment-next-response.ts

@@ -0,0 +1,66 @@
+import type { ServerResponse } from 'node:http'
+import { isPromise } from 'node:util/types'
+
+import type { NextApiResponse } from 'next'
+
+import type { RequestContext } from './handlers/request-context.cjs'
+
+type ResRevalidateMethod = NextApiResponse['revalidate']
+
+function isRevalidateMethod(
+  key: string,
+  nextResponseField: unknown,
+): nextResponseField is ResRevalidateMethod {
+  return key === 'revalidate' && typeof nextResponseField === 'function'
+}
+
+function isAppendHeaderMethod(
+  key: string,
+  nextResponseField: unknown,
+): nextResponseField is ServerResponse['appendHeader'] {
+  return key === 'appendHeader' && typeof nextResponseField === 'function'
+}
+
+// Needing to proxy the response object to intercept:
+//  - the revalidate call for on-demand revalidation on page routes
+//  - prevent .appendHeader calls for location header to add duplicate values
+export const augmentNextResponse = (response: ServerResponse, requestContext: RequestContext) => {
+  return new Proxy(response, {
+    get(target: ServerResponse, key: string) {
+      const originalValue = Reflect.get(target, key)
+      if (isRevalidateMethod(key, originalValue)) {
+        return function newRevalidate(...args: Parameters<ResRevalidateMethod>) {
+          requestContext.didPagesRouterOnDemandRevalidate = true
+
+          const result = originalValue.apply(target, args)
+          if (result && isPromise(result)) {
+            requestContext.trackBackgroundWork(result)
+          }
+
+          return result
+        }
+      }
+
+      if (isAppendHeaderMethod(key, originalValue)) {
+        return function patchedAppendHeader(...args: Parameters<ServerResponse['appendHeader']>) {
+          if (typeof args[0] === 'string' && (args[0] === 'location' || args[0] === 'Location')) {
+            let existing = target.getHeader('location')
+            if (typeof existing !== 'undefined') {
+              if (!Array.isArray(existing)) {
+                existing = [String(existing)]
+              }
+              if (existing.includes(String(args[1]))) {
+                // if we already have that location header - bail early
+                // appendHeader should return the target for chaining
+                return target
+              }
+            }
+          }
+
+          return originalValue.apply(target, args)
+        }
+      }
+      return originalValue
+    },
+  })
+}

src/run/handlers/server.ts

@@ -5,6 +5,7 @@ import type { Context } from '@netlify/functions'
 import type { Span } from '@netlify/otel/opentelemetry'
 import type { WorkerRequestHandler } from 'next/dist/server/lib/types.js'
 
+import { augmentNextResponse } from '../augment-next-response.js'
 import { getRunConfig, setRunConfig } from '../config.js'
 import {
   adjustDateHeader,
@@ -13,7 +14,6 @@ import {
   setCacheTagsHeaders,
   setVaryHeaders,
 } from '../headers.js'
-import { nextResponseProxy } from '../revalidate.js'
 import { setFetchBeforeNextPatchedIt } from '../storage/storage.cjs'
 
 import { getLogger, type RequestContext } from './request-context.cjs'
@@ -97,7 +97,7 @@ export default async (
 
     disableFaultyTransferEncodingHandling(res as unknown as ComputeJsOutgoingMessage)
 
-    const resProxy = nextResponseProxy(res, requestContext)
+    const resProxy = augmentNextResponse(res, requestContext)
 
     // We don't await this here, because it won't resolve until the response is finished.
     const nextHandlerPromise = nextHandler(req, resProxy).catch((error) => {

src/run/revalidate.ts

@@ -0,0 +1,11 @@
+import { redirect } from 'next/navigation'
+
+const Page = async () => {
+  return redirect(`/app-redirect/dest`)
+}
+
+export const generateStaticParams = async () => {
+  return [{ slug: 'prerendered' }]
+}
+
+export default Page

tests/fixtures/simple/app/app-redirect/[slug]/page.js

@@ -0,0 +1,9 @@
+export default function Page() {
+  return (
+    <main>
+      <h1>Hello next/navigation#redirect target</h1>
+    </main>
+  )
+}
+
+export const dynamic = 'force-static'

tests/fixtures/simple/app/app-redirect/dest/page.js

@@ -108,6 +108,8 @@ test<FixtureTestContext>('Test that the simple next app is working', async (ctx)
       shouldHaveAppRouterNotFoundInPrerenderManifest() ? '/_not-found' : undefined,
       '/api/cached-permanent',
       '/api/cached-revalidate',
+      '/app-redirect/dest',
+      '/app-redirect/prerendered',
       '/config-redirect',
       '/config-redirect/dest',
       '/config-rewrite',
@@ -445,6 +447,31 @@ test.skipIf(hasDefaultTurbopackBuilds())<FixtureTestContext>(
   },
 )
 
+// below version check is not exact (not located exactly when, but Next.js had a bug for prerender pages that should redirect would not work correctly)
+// currently only know that 13.5.1 and 14.2.35 doesn't have correct response (either not a 307 or completely missing 'location' header), while 15.5.9 has 307 response with location header
+test.skipIf(nextVersionSatisfies('<15.0.0'))<FixtureTestContext>(
+  `app router page that uses next/navigation#redirect works when page is prerendered`,
+  async (ctx) => {
+    await createFixture('simple', ctx)
+    await runPlugin(ctx)
+
+    const response = await invokeFunction(ctx, { url: `/app-redirect/prerendered` })
+
+    expect(response.statusCode).toBe(307)
+    expect(response.headers['location']).toBe('/app-redirect/dest')
+  },
+)
+
+test<FixtureTestContext>(`app router page that uses next/navigation#redirect works when page is NOT prerendered`, async (ctx) => {
+  await createFixture('simple', ctx)
+  await runPlugin(ctx)
+
+  const response = await invokeFunction(ctx, { url: `/app-redirect/non-prerendered` })
+
+  expect(response.statusCode).toBe(307)
+  expect(response.headers['location']).toBe('/app-redirect/dest')
+})
+
 describe('next patching', async () => {
   const { cp: originalCp, appendFile } = (await vi.importActual(
     'node:fs/promises',