From 7457767a77b03662c103a658273a46cf78359068 Mon Sep 17 00:00:00 2001 From: Zack Tanner <1939140+ztanner@users.noreply.github.com> Date: Tue, 4 Jun 2024 11:05:11 -0700 Subject: [PATCH] [next]: ensure unmatched action rewrites are routed to correct handler (#11686) User defined rewrites are "normalized" so that our internal rewrites are still properly handled. Before normalizing these rewrites, the Next.js builder will attempt to match server action requests to a`.action` variant. Then the user-defined rewrites flow through the afterFiles normalization ([this part](https://github.com/vercel/vercel/blob/fix/unmatched-action-rewrites/packages/next/src/server-build.ts#L254-L279)) so that when we add `.action` in the builder, we don't drop the suffix. But this normalization can lead to a malformed `dest`. e.g., if I had rewrite like this: ```js { source: '/greedy-rewrite/static/:path*', destination: '/static/:path*', } ``` The builder would go through this flow on an action request to `/greedy-rewrite/static`: 1. It'll attempt to match it to a `.action` output, so `/greedy-rewrite/static` -> `/greedy-rewrite/static.action` 2. The afterFiles normalization will take place, so the original `dest` of `/static/$1` will become `/static/$1$rscsuff` 3. $1 will be an empty string, because it doesn't match the existing capture group. So now `/greedy-rewrite/static.action` -> `/greedy-rewrite/static/.action` 4. `static/.action` is not a valid output, so it'll 404 and the action will break. Existing handling exists for `.rsc` outputs for a similar reason, but only on the index route. I added a similar fix for this in #11688. --- .changeset/mean-peas-visit.md | 5 +++ packages/next/src/server-build.ts | 19 ++++++++++ .../app/edge/static/page.js | 25 +++++++++++++ .../app/page.js | 25 +++++++++++++ .../app/static/page.js | 25 +++++++++++++ .../index.test.js | 37 +++++++++++++++++++ .../next.config.js | 12 ++++++ 7 files changed, 148 insertions(+) create mode 100644 .changeset/mean-peas-visit.md create mode 100644 packages/next/test/fixtures/00-app-dir-actions-experimental-streaming/app/edge/static/page.js create mode 100644 packages/next/test/fixtures/00-app-dir-actions-experimental-streaming/app/page.js create mode 100644 packages/next/test/fixtures/00-app-dir-actions-experimental-streaming/app/static/page.js diff --git a/.changeset/mean-peas-visit.md b/.changeset/mean-peas-visit.md new file mode 100644 index 00000000000..8695d257573 --- /dev/null +++ b/.changeset/mean-peas-visit.md @@ -0,0 +1,5 @@ +--- +'@vercel/next': patch +--- + +ensure unmatched action rewrites are routed to correct handler diff --git a/packages/next/src/server-build.ts b/packages/next/src/server-build.ts index 199fed51d6f..0ddf47cbaf6 100644 --- a/packages/next/src/server-build.ts +++ b/packages/next/src/server-build.ts @@ -2089,6 +2089,25 @@ export async function serverBuild({ // with that routing section ...afterFilesRewrites, + // Ensure that after we normalize `afterFilesRewrites`, unmatched actions are routed to the correct handler + // e.g. /foo/.action -> /foo.action. This should only ever match in cases where we're routing to an action handler + // and the rewrite normalization led to something like /foo/$1$rscsuff, and $1 had no match. + // This is meant to have parity with the .rsc handling below. + ...(hasActionOutputSupport + ? [ + { + src: `${path.posix.join('/', entryDirectory, '/\\.action$')}`, + dest: `${path.posix.join('/', entryDirectory, '/index.action')}`, + check: true, + }, + { + src: `${path.posix.join('/', entryDirectory, '(.+)/\\.action$')}`, + dest: `${path.posix.join('/', entryDirectory, '$1.action')}`, + check: true, + }, + ] + : []), + // ensure non-normalized /.rsc from rewrites is handled ...(appPathRoutesManifest ? [ diff --git a/packages/next/test/fixtures/00-app-dir-actions-experimental-streaming/app/edge/static/page.js b/packages/next/test/fixtures/00-app-dir-actions-experimental-streaming/app/edge/static/page.js new file mode 100644 index 00000000000..70a73e7f203 --- /dev/null +++ b/packages/next/test/fixtures/00-app-dir-actions-experimental-streaming/app/edge/static/page.js @@ -0,0 +1,25 @@ +"use client"; + +import { useState } from "react"; +import { increment } from "../../actions"; + +export default function Home() { + const [count, setCount] = useState(0); + + return ( +
+ {count} + + Static +
+ ); +} diff --git a/packages/next/test/fixtures/00-app-dir-actions-experimental-streaming/app/page.js b/packages/next/test/fixtures/00-app-dir-actions-experimental-streaming/app/page.js new file mode 100644 index 00000000000..60199edbbb8 --- /dev/null +++ b/packages/next/test/fixtures/00-app-dir-actions-experimental-streaming/app/page.js @@ -0,0 +1,25 @@ +"use client"; + +import { useState } from "react"; +import { increment } from "./actions"; + +export default function Home() { + const [count, setCount] = useState(0); + + return ( +
+ {count} + + Static +
+ ); +} diff --git a/packages/next/test/fixtures/00-app-dir-actions-experimental-streaming/app/static/page.js b/packages/next/test/fixtures/00-app-dir-actions-experimental-streaming/app/static/page.js new file mode 100644 index 00000000000..d4dca276762 --- /dev/null +++ b/packages/next/test/fixtures/00-app-dir-actions-experimental-streaming/app/static/page.js @@ -0,0 +1,25 @@ +"use client"; + +import { useState } from "react"; +import { increment } from "../actions"; + +export default function Home() { + const [count, setCount] = useState(0); + + return ( +
+ {count} + + Static +
+ ); +} diff --git a/packages/next/test/fixtures/00-app-dir-actions-experimental-streaming/index.test.js b/packages/next/test/fixtures/00-app-dir-actions-experimental-streaming/index.test.js index 494ebe14fbe..7ffe4905c0f 100644 --- a/packages/next/test/fixtures/00-app-dir-actions-experimental-streaming/index.test.js +++ b/packages/next/test/fixtures/00-app-dir-actions-experimental-streaming/index.test.js @@ -293,6 +293,43 @@ describe(`${__dirname.split(path.sep).pop()}`, () => { expect(res.headers.get('x-edge-runtime')).toBe('1'); } }); + + it('should work when a rewrite greedy matches an action rewrite', async () => { + const targetPath = `${basePath}/static`; + const canonicalPath = `/greedy-rewrite/${basePath}/static`; + const actionId = findActionId(targetPath, runtime); + + const res = await fetch( + `${ctx.deploymentUrl}${canonicalPath}`, + generateFormDataPayload(actionId) + ); + + expect(res.status).toEqual(200); + expect(res.headers.get('x-matched-path')).toBe(targetPath + '.action'); + expect(res.headers.get('content-type')).toBe('text/x-component'); + if (runtime === 'node') { + expect(res.headers.get('x-vercel-cache')).toBe('MISS'); + } else { + expect(res.headers.get('x-edge-runtime')).toBe('1'); + } + }); + }); + + describe('rewrite to index', () => { + it('should work when user has a rewrite to the index route', async () => { + const canonicalPath = '/rewritten-to-index'; + const actionId = findActionId('', 'node'); + + const res = await fetch( + `${ctx.deploymentUrl}${canonicalPath}`, + generateFormDataPayload(actionId) + ); + + expect(res.status).toEqual(200); + expect(res.headers.get('x-matched-path')).toBe('/index.action'); + expect(res.headers.get('content-type')).toBe('text/x-component'); + expect(res.headers.get('x-vercel-cache')).toBe('MISS'); + }); }); describe('pages', () => { diff --git a/packages/next/test/fixtures/00-app-dir-actions-experimental-streaming/next.config.js b/packages/next/test/fixtures/00-app-dir-actions-experimental-streaming/next.config.js index 02977f3ac59..fd6541b69a1 100644 --- a/packages/next/test/fixtures/00-app-dir-actions-experimental-streaming/next.config.js +++ b/packages/next/test/fixtures/00-app-dir-actions-experimental-streaming/next.config.js @@ -9,6 +9,18 @@ module.exports = { source: '/rewrite/edge/rsc/static', destination: '/edge/rsc/static', }, + { + source: '/greedy-rewrite/static/:path*', + destination: '/static/:path*', + }, + { + source: '/greedy-rewrite/edge/static/:path*', + destination: '/edge/static/:path*', + }, + { + source: '/rewritten-to-index', + destination: '/?fromRewrite=1', + }, ]; }, };