Fix internal route redirection with absolute urls outside basePath (#64604)

When performing a redirect() with an absolute path, action-handler
attempts to detect whether the resource is hosted by NextJS. If we
believe it is, we then attempt to stream it.

Previously we were not accounting for basePath which caused absolute
redirects to resources on the same host, but not underneath the
basePath, to be resolved by NextJS. Since the resource is outside the
basePath we resolve a 404 page which returns back as `text/x-component`
and is thus streamed back to the client within the original POST
request.

This PR adds a check for the presence of the basePath within absolute
redirect URLs. This fixes the above problem.

fixes #64413
fixes #64557

---------

Signed-off-by: Chris Frank <chris@cfrank.org>
Co-authored-by: JJ Kasper <jj@jjsweb.site>

packages/next/src/server/app-render/action-handler.ts

@@ -239,6 +239,36 @@ async function createForwardedActionResponse(
   return RenderResult.fromStatic('{}')
 }
 
+/**
+ * Returns the parsed redirect URL if we deem that it is hosted by us.
+ *
+ * We handle both relative and absolute redirect URLs.
+ *
+ * In case the redirect URL is not relative to the application we return `null`.
+ */
+function getAppRelativeRedirectUrl(
+  basePath: string,
+  host: Host,
+  redirectUrl: string
+): URL | null {
+  if (redirectUrl.startsWith('/')) {
+    // Make sure we are appending the basePath to relative URLS
+    return new URL(`${basePath}${redirectUrl}`, 'http://n')
+  }
+
+  const parsedRedirectUrl = new URL(redirectUrl)
+
+  if (host?.value !== parsedRedirectUrl.host) {
+    return null
+  }
+
+  // At this point the hosts are the same, just confirm we
+  // are routing to a path underneath the `basePath`
+  return parsedRedirectUrl.pathname.startsWith(basePath)
+    ? parsedRedirectUrl
+    : null
+}
+
 async function createRedirectRenderResult(
   req: BaseNextRequest,
   res: BaseNextResponse,
@@ -252,14 +282,15 @@ async function createRedirectRenderResult(
   // If we're redirecting to another route of this Next.js application, we'll
   // try to stream the response from the other worker path. When that works,
   // we can save an extra roundtrip and avoid a full page reload.
-  // When the redirect URL starts with a `/`, or to the same host as application,
-  // we treat it as an app-relative redirect.
-  const parsedRedirectUrl = new URL(redirectUrl, 'http://n')
-  const isAppRelativeRedirect =
-    redirectUrl.startsWith('/') ||
-    (originalHost && originalHost.value === parsedRedirectUrl.host)
+  // When the redirect URL starts with a `/` or is to the same host, under the
+  // `basePath` we treat it as an app-relative redirect;
+  const appRelativeRedirectUrl = getAppRelativeRedirectUrl(
+    basePath,
+    originalHost,
+    redirectUrl
+  )
 
-  if (isAppRelativeRedirect) {
+  if (appRelativeRedirectUrl) {
     if (!originalHost) {
       throw new Error(
         'Invariant: Missing `host` header from a forwarded Server Actions request.'
@@ -278,7 +309,7 @@ async function createRedirectRenderResult(
       process.env.__NEXT_PRIVATE_ORIGIN || `${proto}://${originalHost.value}`
 
     const fetchUrl = new URL(
-      `${origin}${basePath}${parsedRedirectUrl.pathname}${parsedRedirectUrl.search}`
+      `${origin}${appRelativeRedirectUrl.pathname}${appRelativeRedirectUrl.search}`
     )
 
     if (staticGenerationStore.revalidatedTags) {

test/e2e/app-dir/actions/app-action.test.ts

@@ -809,25 +809,61 @@ describe('app-dir action handling', () => {
       }, 'Prefix: HELLO, WORLD')
     })
 
-    it('should handle redirect to a relative URL in a single pass', async () => {
-      const browser = await next.browser('/client/edge')
+    it.each(['relative', 'absolute'])(
+      `should handle calls to redirect() with a %s URL in a single pass`,
+      async (redirectType) => {
+        const initialPagePath = '/client/redirects'
+        const destinationPagePath = '/redirect-target'
 
-      await waitFor(3000)
+        const browser = await next.browser(initialPagePath)
 
-      let requests = []
+        const requests: Request[] = []
+        const responses: Response[] = []
 
-      browser.on('request', (req: Request) => {
-        requests.push(new URL(req.url()).pathname)
-      })
+        browser.on('request', (req: Request) => {
+          const url = req.url()
 
-      await browser.elementByCss('#redirect').click()
+          if (
+            url.includes(initialPagePath) ||
+            url.includes(destinationPagePath)
+          ) {
+            requests.push(req)
+          }
+        })
 
-      // no other requests should be made
-      expect(requests).toEqual(['/client/edge'])
-    })
+        browser.on('response', (res: Response) => {
+          const url = res.url()
 
-    it('should handle regular redirects', async () => {
-      const browser = await next.browser('/client/edge')
+          if (
+            url.includes(initialPagePath) ||
+            url.includes(destinationPagePath)
+          ) {
+            responses.push(res)
+          }
+        })
+
+        await browser.elementById(`redirect-${redirectType}`).click()
+        await check(() => browser.url(), `${next.url}${destinationPagePath}`)
+
+        expect(await browser.waitForElementByCss('#redirected').text()).toBe(
+          'redirected'
+        )
+
+        // no other requests should be made
+        expect(requests).toHaveLength(1)
+        expect(responses).toHaveLength(1)
+
+        const request = requests[0]
+        const response = responses[0]
+
+        expect(request.url()).toEqual(`${next.url}${initialPagePath}`)
+        expect(request.method()).toEqual('POST')
+        expect(response.status()).toEqual(303)
+      }
+    )
+
+    it('should handle calls to redirect() with external URLs', async () => {
+      const browser = await next.browser('/client/redirects')
 
       await browser.elementByCss('#redirect-external').click()
 
@@ -876,36 +912,57 @@ describe('app-dir action handling', () => {
       await check(() => browser.elementByCss('#count').text(), '2')
     })
 
-    it('should handle redirect to a relative URL in a single pass', async () => {
-      let responseCode: number
-      const browser = await next.browser('/client', {
-        beforePageLoad(page) {
-          page.on('response', async (res: Response) => {
-            const headers = await res.allHeaders()
-            if (headers['x-action-redirect']) {
-              responseCode = res.status()
-            }
-          })
-        },
-      })
+    it.each(['relative', 'absolute'])(
+      `should handle calls to redirect() with a %s URL in a single pass`,
+      async (redirectType) => {
+        const initialPagePath = '/client/redirects'
+        const destinationPagePath = '/redirect-target'
 
-      await waitFor(3000)
+        const browser = await next.browser(initialPagePath)
 
-      let requests = []
+        const requests: Request[] = []
+        const responses: Response[] = []
 
-      browser.on('request', (req: Request) => {
-        requests.push(new URL(req.url()).pathname)
-      })
+        browser.on('request', (req: Request) => {
+          const url = req.url()
 
-      await browser.elementByCss('#redirect').click()
+          if (
+            url.includes(initialPagePath) ||
+            url.includes(destinationPagePath)
+          ) {
+            requests.push(req)
+          }
+        })
 
-      // no other requests should be made
-      expect(requests).toEqual(['/client'])
-      await check(() => responseCode, 303)
-    })
+        browser.on('response', (res: Response) => {
+          const url = res.url()
 
-    it('should handle regular redirects', async () => {
-      const browser = await next.browser('/client')
+          if (
+            url.includes(initialPagePath) ||
+            url.includes(destinationPagePath)
+          ) {
+            responses.push(res)
+          }
+        })
+
+        await browser.elementById(`redirect-${redirectType}`).click()
+        await check(() => browser.url(), `${next.url}${destinationPagePath}`)
+
+        // no other requests should be made
+        expect(requests).toHaveLength(1)
+        expect(responses).toHaveLength(1)
+
+        const request = requests[0]
+        const response = responses[0]
+
+        expect(request.url()).toEqual(`${next.url}${initialPagePath}`)
+        expect(request.method()).toEqual('POST')
+        expect(response.status()).toEqual(303)
+      }
+    )
+
+    it('should handle calls to redirect() with external URLs', async () => {
+      const browser = await next.browser('/client/redirects')
 
       await browser.elementByCss('#redirect-external').click()
 

test/e2e/app-dir/actions/app/client/edge/page.js

@@ -38,10 +38,20 @@ export default function Counter() {
       </button>
       <form>
         <button
-          id="redirect"
+          id="redirect-relative"
           formAction={() => redirectAction('/redirect-target')}
         >
-          redirect
+          redirect to a relative URL
+        </button>
+      </form>
+      <form>
+        <button
+          id="redirect-absolute"
+          formAction={() =>
+            redirectAction(`${location.origin}/redirect-target`)
+          }
+        >
+          redirect to a absolute URL
         </button>
       </form>
       <form>

test/e2e/app-dir/actions/app/client/page.js

@@ -56,36 +56,6 @@ export default function Counter() {
       >
         *2
       </button>
-      <form>
-        <button
-          id="redirect"
-          formAction={() => redirectAction('/redirect-target')}
-        >
-          redirect
-        </button>
-      </form>
-      <form>
-        <button
-          id="redirect-external"
-          formAction={() =>
-            redirectAction(
-              'https://next-data-api-endpoint.vercel.app/api/random?page'
-            )
-          }
-        >
-          redirect external
-        </button>
-      </form>
-      <form>
-        <button
-          id="redirect-absolute"
-          formAction={() =>
-            redirectAction(location.origin + '/redirect-target')
-          }
-        >
-          redirect internal with domain
-        </button>
-      </form>
       <form>
         <button
           id="redirect-pages"

test/e2e/app-dir/actions/app/client/redirects/page.js

@@ -0,0 +1,40 @@
+'use client'
+
+import { redirectAction } from '../actions'
+
+export default function Page() {
+  return (
+    <div>
+      <form>
+        <button
+          id="redirect-relative"
+          formAction={() => redirectAction('/redirect-target')}
+        >
+          redirect relative
+        </button>
+      </form>
+      <form>
+        <button
+          id="redirect-external"
+          formAction={() =>
+            redirectAction(
+              'https://next-data-api-endpoint.vercel.app/api/random?page'
+            )
+          }
+        >
+          redirect external
+        </button>
+      </form>
+      <form>
+        <button
+          id="redirect-absolute"
+          formAction={() =>
+            redirectAction(location.origin + '/redirect-target')
+          }
+        >
+          redirect internal with domain
+        </button>
+      </form>
+    </div>
+  )
+}

test/e2e/app-dir/app-basepath/app/client/action.js

@@ -0,0 +1,9 @@
+'use server'
+
+import 'server-only'
+
+import { redirect } from 'next/navigation'
+
+export async function redirectAction(path) {
+  redirect(path)
+}

test/e2e/app-dir/app-basepath/app/client/page.js

@@ -0,0 +1,36 @@
+'use client'
+
+import { redirectAction } from './action'
+
+export default function Page() {
+  return (
+    <div>
+      <form>
+        <button
+          id="redirect-relative"
+          formAction={() => redirectAction('/another')}
+        >
+          redirect internal with relative path
+        </button>
+      </form>
+      <form>
+        <button
+          id="redirect-absolute-internal"
+          formAction={() => redirectAction(location.origin + '/base/another')}
+        >
+          redirect internal with domain (including basePath)
+        </button>
+      </form>
+      <form>
+        <button
+          id="redirect-absolute-external"
+          formAction={() =>
+            redirectAction(location.origin + '/outsideBasePath')
+          }
+        >
+          redirect external with domain (excluding basePath)
+        </button>
+      </form>
+    </div>
+  )
+}

test/e2e/app-dir/app-basepath/index.test.ts

@@ -1,19 +1,15 @@
 import { nextTestSetup } from 'e2e-utils'
-import { retry } from 'next-test-utils'
+import { check, retry } from 'next-test-utils'
+import type { Request, Response } from 'playwright'
 
 describe('app dir - basepath', () => {
-  const { next, skipped } = nextTestSetup({
+  const { next } = nextTestSetup({
     files: __dirname,
-    skipDeployment: true,
     dependencies: {
       sass: 'latest',
     },
   })
 
-  if (skipped) {
-    return
-  }
-
   it('should successfully hard navigate from pages -> app', async () => {
     const browser = await next.browser('/base/pages-path')
     await browser.elementByCss('#to-another').click()
@@ -94,4 +90,106 @@ describe('app dir - basepath', () => {
       })
     }
   )
+
+  it.each([
+    { redirectType: 'relative', buttonId: 'redirect-relative' },
+    { redirectType: 'absolute', buttonId: 'redirect-absolute-internal' },
+  ])(
+    `should properly stream an internal server action redirect() with a $redirectType URL`,
+    async ({ buttonId }) => {
+      const initialPagePath = '/base/client'
+      const destinationPagePath = '/base/another'
+
+      const requests: Request[] = []
+      const responses: Response[] = []
+
+      const browser = await next.browser(initialPagePath)
+
+      browser.on('request', (req: Request) => {
+        const url = req.url()
+
+        if (
+          url.includes(initialPagePath) ||
+          url.includes(destinationPagePath)
+        ) {
+          requests.push(req)
+        }
+      })
+
+      browser.on('response', (res: Response) => {
+        const url = res.url()
+
+        if (
+          url.includes(initialPagePath) ||
+          url.includes(destinationPagePath)
+        ) {
+          responses.push(res)
+        }
+      })
+
+      await browser.elementById(buttonId).click()
+      await check(() => browser.url(), /\/base\/another/)
+
+      expect(await browser.waitForElementByCss('#page-2').text()).toBe(`Page 2`)
+
+      // verify that the POST request was only made to the action handler
+      expect(requests).toHaveLength(1)
+      expect(responses).toHaveLength(1)
+
+      const request = requests[0]
+      const response = responses[0]
+
+      expect(request.method()).toEqual('POST')
+      expect(request.url()).toEqual(`${next.url}${initialPagePath}`)
+      expect(response.status()).toEqual(303)
+    }
+  )
+
+  it('should redirect externally when encountering absolute URLs on the same host outside the basePath', async () => {
+    const initialPagePath = '/base/client'
+    const destinationPagePath = '/outsideBasePath'
+
+    const requests: Request[] = []
+    const responses: Response[] = []
+
+    const browser = await next.browser(initialPagePath)
+
+    browser.on('request', (req: Request) => {
+      const url = req.url()
+
+      if (!url.includes('_next')) {
+        requests.push(req)
+      }
+    })
+
+    browser.on('response', (res: Response) => {
+      const url = res.url()
+
+      if (!url.includes('_next')) {
+        responses.push(res)
+      }
+    })
+
+    await browser.elementById('redirect-absolute-external').click()
+    await check(() => browser.url(), /\/outsideBasePath/)
+
+    // We expect to see two requests, first a POST invoking the server
+    // action. And second a GET request resolving the redirect.
+    expect(requests).toHaveLength(2)
+    expect(responses).toHaveLength(2)
+
+    const [firstRequest, secondRequest] = requests
+    const [firstResponse, secondResponse] = responses
+
+    expect(firstRequest.method()).toEqual('POST')
+    expect(firstRequest.url()).toEqual(`${next.url}${initialPagePath}`)
+
+    expect(secondRequest.method()).toEqual('GET')
+    expect(secondRequest.url()).toEqual(`${next.url}${destinationPagePath}`)
+
+    expect(firstResponse.status()).toEqual(303)
+    // Since this is an external request to a resource outside of NextJS
+    // we expect to see a seperate request resolving the external URL.
+    expect(secondResponse.status()).toEqual(200)
+  })
 })

test/e2e/app-dir/app-basepath/next.config.js

@@ -1,3 +1,12 @@
 module.exports = {
   basePath: '/base',
+  async rewrites() {
+    return [
+      {
+        source: '/outsideBasePath',
+        destination: 'https://example.vercel.sh/',
+        basePath: false,
+      },
+    ]
+  },
 }

test/lib/browsers/base.ts

@@ -1,4 +1,4 @@
-export type Event = 'request'
+export type Event = 'request' | 'response'
 
 /**
  * This is the base Browser interface all browser

test/lib/browsers/playwright.ts

@@ -55,6 +55,7 @@ export class Playwright extends BrowserInterface {
   private activeTrace?: string
   private eventCallbacks: Record<Event, Set<(...args: any[]) => void>> = {
     request: new Set(),
+    response: new Set(),
   }
   private async initContextTracing(url: string, context: BrowserContext) {
     if (!tracePlaywright) {
@@ -237,6 +238,9 @@ export class Playwright extends BrowserInterface {
     page.on('request', (req) => {
       this.eventCallbacks.request.forEach((cb) => cb(req))
     })
+    page.on('response', (res) => {
+      this.eventCallbacks.response.forEach((cb) => cb(res))
+    })
 
     if (opts?.disableCache) {
       // TODO: this doesn't seem to work (dev tools does not check the box as expected)