Skip to content

Commit

Permalink
Fix issue with redirect() duplicating basePath in absolute URLs
Browse files Browse the repository at this point in the history
When encountering a absolute URL within a redirect() call from a server
action we were duplicating the basePath when it was present.

This commit moves responsibility for including the basePath to
`getAppRelativeRedirectUrl` which solves the issue.

Signed-off-by: Chris Frank <chris@cfrank.org>
  • Loading branch information
cfrank committed May 24, 2024
1 parent b289119 commit fef4757
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 17 deletions.
15 changes: 6 additions & 9 deletions packages/next/src/server/app-render/action-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -251,17 +251,14 @@ function getAppRelativeRedirectUrl(
host: Host,
redirectUrl: string
): URL | null {
if (!host) {
return null
}

const parsedRedirectUrl = new URL(redirectUrl, 'http://n')

if (redirectUrl.startsWith('/')) {
return parsedRedirectUrl
// Make sure we are appending the basePath to relative URLS
return new URL(`${basePath}${redirectUrl}`, 'http://n')
}

if (host.value !== parsedRedirectUrl.host) {
const parsedRedirectUrl = new URL(redirectUrl)

if (host?.value !== parsedRedirectUrl.host) {
return null
}

Expand Down Expand Up @@ -312,7 +309,7 @@ async function createRedirectRenderResult(
process.env.__NEXT_PRIVATE_ORIGIN || `${proto}://${originalHost.value}`

const fetchUrl = new URL(
`${origin}${basePath}${appRelativeRedirectUrl.pathname}${appRelativeRedirectUrl.search}`
`${origin}${appRelativeRedirectUrl.pathname}${appRelativeRedirectUrl.search}`
)

if (staticGenerationStore.revalidatedTags) {
Expand Down
27 changes: 21 additions & 6 deletions test/e2e/app-dir/actions/app-action.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -775,21 +775,36 @@ describe('app-dir action handling', () => {
it.each(['relative', 'absolute'])(
`should handle calls to redirect() with a %s URL in a single pass`,
async (redirectType) => {
const browser = await next.browser('/client/edge')
const initialPagePath = '/client/redirects'
const destinationPagePath = '/redirect-target'

const browser = await next.browser(initialPagePath)

const requests: Request[] = []
const responses: Response[] = []

browser.on('request', (req: Request) => {
requests.push(req)
const url = req.url()

if (!url.includes('_next')) {
requests.push(req)
}
})

browser.on('response', (res: Response) => {
responses.push(res)
const url = res.url()

if (!url.includes('_next')) {
responses.push(res)
}
})

await browser.elementById(`redirect-${redirectType}`).click()
await check(() => browser.url(), /\/redirect-target/)
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)
Expand All @@ -798,14 +813,14 @@ describe('app-dir action handling', () => {
const request = requests[0]
const response = responses[0]

expect(request.url()).toEqual(`${next.url}/client/edge`)
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/edge')
const browser = await next.browser('/client/redirects')

await browser.elementByCss('#redirect-external').click()

Expand Down
6 changes: 4 additions & 2 deletions test/e2e/app-dir/app-basepath/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,8 @@ describe('app dir - basepath', () => {
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)
Expand All @@ -160,15 +162,15 @@ describe('app dir - basepath', () => {
browser.on('request', (req: Request) => {
const url = req.url()

if (url.includes(initialPagePath) || url.includes(destinationPagePath)) {
if (!url.includes('_next')) {
requests.push(req)
}
})

browser.on('response', (res: Response) => {
const url = res.url()

if (url.includes(initialPagePath) || url.includes(destinationPagePath)) {
if (!url.includes('_next')) {
responses.push(res)
}
})
Expand Down

0 comments on commit fef4757

Please sign in to comment.