From 44aeb083cc28ac3535b62ca51eb47d0d7d456ea6 Mon Sep 17 00:00:00 2001 From: Chris Frank Date: Tue, 18 Jun 2024 06:58:38 -0700 Subject: [PATCH] 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 Co-authored-by: JJ Kasper --- .../src/server/app-render/action-handler.ts | 47 +++++-- test/e2e/app-dir/actions/app-action.test.ts | 131 +++++++++++++----- .../app-dir/actions/app/client/edge/page.js | 14 +- test/e2e/app-dir/actions/app/client/page.js | 30 ---- .../actions/app/client/redirects/page.js | 40 ++++++ .../app-dir/app-basepath/app/client/action.js | 9 ++ .../app-dir/app-basepath/app/client/page.js | 36 +++++ test/e2e/app-dir/app-basepath/index.test.ts | 112 ++++++++++++++- test/e2e/app-dir/app-basepath/next.config.js | 9 ++ test/lib/browsers/base.ts | 2 +- test/lib/browsers/playwright.ts | 4 + 11 files changed, 349 insertions(+), 85 deletions(-) create mode 100644 test/e2e/app-dir/actions/app/client/redirects/page.js create mode 100644 test/e2e/app-dir/app-basepath/app/client/action.js create mode 100644 test/e2e/app-dir/app-basepath/app/client/page.js diff --git a/packages/next/src/server/app-render/action-handler.ts b/packages/next/src/server/app-render/action-handler.ts index 833007d377..72cf877ed1 100644 --- a/packages/next/src/server/app-render/action-handler.ts +++ b/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) { diff --git a/test/e2e/app-dir/actions/app-action.test.ts b/test/e2e/app-dir/actions/app-action.test.ts index 43f3e651bc..ba46424189 100644 --- a/test/e2e/app-dir/actions/app-action.test.ts +++ b/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() diff --git a/test/e2e/app-dir/actions/app/client/edge/page.js b/test/e2e/app-dir/actions/app/client/edge/page.js index c0b910d73e..5cd4cecad9 100644 --- a/test/e2e/app-dir/actions/app/client/edge/page.js +++ b/test/e2e/app-dir/actions/app/client/edge/page.js @@ -38,10 +38,20 @@ export default function Counter() {
+
+
+
diff --git a/test/e2e/app-dir/actions/app/client/page.js b/test/e2e/app-dir/actions/app/client/page.js index 8bd1e5a1dc..ced2c759f6 100644 --- a/test/e2e/app-dir/actions/app/client/page.js +++ b/test/e2e/app-dir/actions/app/client/page.js @@ -56,36 +56,6 @@ export default function Counter() { > *2 - - -
-
- -
-
- -
+
+
+ +
+
+ +
+ + ) +} diff --git a/test/e2e/app-dir/app-basepath/app/client/action.js b/test/e2e/app-dir/app-basepath/app/client/action.js new file mode 100644 index 0000000000..a5a5d9ae46 --- /dev/null +++ b/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) +} diff --git a/test/e2e/app-dir/app-basepath/app/client/page.js b/test/e2e/app-dir/app-basepath/app/client/page.js new file mode 100644 index 0000000000..e69f60c84f --- /dev/null +++ b/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 ( +
+
+ +
+
+ +
+
+ +
+
+ ) +} diff --git a/test/e2e/app-dir/app-basepath/index.test.ts b/test/e2e/app-dir/app-basepath/index.test.ts index 5096f3556f..c66eaf4e52 100644 --- a/test/e2e/app-dir/app-basepath/index.test.ts +++ b/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) + }) }) diff --git a/test/e2e/app-dir/app-basepath/next.config.js b/test/e2e/app-dir/app-basepath/next.config.js index 1773c854e2..964e24ce9f 100644 --- a/test/e2e/app-dir/app-basepath/next.config.js +++ b/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, + }, + ] + }, } diff --git a/test/lib/browsers/base.ts b/test/lib/browsers/base.ts index e3393c3a79..5cd08c68d9 100644 --- a/test/lib/browsers/base.ts +++ b/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 diff --git a/test/lib/browsers/playwright.ts b/test/lib/browsers/playwright.ts index 93563ed765..8caef1edc8 100644 --- a/test/lib/browsers/playwright.ts +++ b/test/lib/browsers/playwright.ts @@ -55,6 +55,7 @@ export class Playwright extends BrowserInterface { private activeTrace?: string private eventCallbacks: Record 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)