Fix ISR routes using undocumented $0 token in Vercel route dest fields

.changeset/fix-isr-dollar-zero.md

@@ -0,0 +1,5 @@
+---
+'@astrojs/vercel': patch
+---
+
+Fixes a rare issue where ISR pages on Vercel could intermittently be served a cached redirect to a nonsense URL (such as `/$0/`) instead of the page itself. Pages now render correctly, and requests that can't be resolved to a valid path return a `404` instead of a redirect.

packages/integrations/vercel/src/index.ts

@@ -66,13 +66,27 @@ export const VERCEL_EDGE_MIDDLEWARE_FILE = 'vercel-edge-middleware';
 export const NODE_PATH = '_render';
 const MIDDLEWARE_PATH = '_middleware';
 
-// This isn't documented by vercel anywhere, but unlike serverless
-// and edge functions, isr functions are not passed the original path.
-// Instead, we have to use $0 to refer to the regex match from "src".
+// Unlike serverless and edge functions, ISR functions are not passed the
+// original path, so the route rewrite's `dest` carries it via a regex capture
+// group reference. Vercel's router substitutes `$1`–`$9` and `$<letters>`, but
+// not `$0`, so each ISR route's `src` pattern is wrapped in an outer capture
+// group (see `wrapPatternInCaptureGroup`) and referenced as `$1` here. See
+// https://github.com/withastro/astro/issues/18028 for the `$0` failure mode.
 // The path token is appended so the entrypoint can verify the rewrite
 // originated from this build's route table and not from an external caller.
 const getIsrPath = (pathToken: string) =>
-	`/_isr?${ASTRO_PATH_PARAM}=$0&${ASTRO_PATH_TOKEN_PARAM}=${pathToken}`;
+	`/_isr?${ASTRO_PATH_PARAM}=$1&${ASTRO_PATH_TOKEN_PARAM}=${pathToken}`;
+
+/**
+ * Wraps a route regex source in an outer capture group so the full match is
+ * available as `$1` in Vercel's route `dest` substitution. Inner groups shift
+ * to `$2`, `$3`, etc., but `dest` only references `$1`.
+ *
+ * `^\/one\/?$` → `^(\/one\/?)$`
+ */
+function wrapPatternInCaptureGroup(source: string): string {
+	return source.replace(/^\^(.*)\$$/, '^($1)$');
+}
 
 // https://vercel.com/docs/concepts/functions/serverless-functions/runtimes/node-js#node.js-version
 const SUPPORTED_NODE_VERSIONS: Record<
@@ -506,7 +520,7 @@ export default function vercelAdapter({
 							});
 
 							if (!excludeRouteFromIsr) {
-								const src = route.patternRegex.source;
+								let src = route.patternRegex.source;
 								const isInternal =
 									src.startsWith('^\\/_image') || src.startsWith('^\\/_server-islands');
 
@@ -516,6 +530,10 @@ export default function vercelAdapter({
 								} else if (_middlewareEntryPoint) {
 									// The middleware has to run before the cache is consulted.
 									dest = MIDDLEWARE_PATH;
+								} else {
+									// `dest` references `$1`, so wrap `src` to make the full
+									// match the first capture group.
+									src = wrapPatternInCaptureGroup(src);
 								}
 
 								if (!route.isPrerendered) {

packages/integrations/vercel/src/serverless/entrypoint.ts

@@ -19,16 +19,29 @@ export default {
 		const url = new URL(request.url);
 		const middlewareSecretHeader = request.headers.get(ASTRO_MIDDLEWARE_SECRET_HEADER);
 		const hasValidMiddlewareSecret = middlewareSecretHeader === middlewareSecret;
-		let realPath = undefined;
+		let realPath: string | null = null;
+		let overrideTrusted = false;
 		if (hasValidMiddlewareSecret) {
+			overrideTrusted = true;
 			realPath = request.headers.get(ASTRO_PATH_HEADER);
 		} else if (url.searchParams.get(ASTRO_PATH_TOKEN_PARAM) === middlewareSecret) {
 			// ISR functions only receive the target path via the URL, so the route
 			// rewrite carries the build's path token alongside it. Only honor the
 			// override when that token matches, otherwise the path is ignored and
 			// the request is served as `/_isr`.
+			overrideTrusted = true;
 			realPath = url.searchParams.get(ASTRO_PATH_PARAM);
 		}
+		// A trusted override must carry an absolute path: every route pattern is
+		// anchored on `^\/` and the edge middleware forwards `request.url` minus
+		// its origin. When Vercel fails to substitute the `$1` reference in an ISR
+		// rewrite, the literal `$1` lands here; resolving it would render a
+		// nonsense pathname, and with a catch-all page the internal `/_isr` URL
+		// itself would render as a 200. Fail closed with an explicit 404 instead
+		// of falling through to `/_isr`. https://github.com/withastro/astro/issues/18028
+		if (overrideTrusted && (realPath === null || !realPath.startsWith('/'))) {
+			return new Response('Not Found', { status: 404 });
+		}
 		if (typeof realPath === 'string') {
 			// The header carries the client's whole path, query included; the route
 			// rewrite carries only the pathname and leaves the query on this request.

packages/integrations/vercel/test/fixtures/isr-edge-no-middleware/src/pages/[...slug].astro

@@ -0,0 +1,8 @@
+<html>
+	<head>
+		<title>Spread</title>
+	</head>
+	<body>
+		<h1>Slug: {Astro.params.slug}</h1>
+	</body>
+</html>

packages/integrations/vercel/test/fixtures/isr/src/pages/[a]/[b].astro

@@ -0,0 +1,8 @@
+<html>
+	<head>
+		<title>AB</title>
+	</head>
+	<body>
+		<h1>A: {Astro.params.a} B: {Astro.params.b}</h1>
+	</body>
+</html>

packages/integrations/vercel/test/isr-edge-middleware.test.ts

@@ -239,7 +239,98 @@ describe('ISR with edge middleware requested but no middleware file', () => {
 
 	it('routes straight to the ISR function', { timeout: 30000 }, async () => {
 		const { routes } = await getVercelConfig(fixture);
-		assert.match(destOf(routes, '^/$') ?? '', /^\/_isr\?/);
+		assert.match(destOf(routes, '^(/)$') ?? '', /^\/_isr\?/);
+	});
+
+	it('wraps spread-route patterns in the capture group $1 references', {
+		timeout: 30000,
+	}, async () => {
+		const { routes } = await getVercelConfig(fixture);
+		assert.match(destOf(routes, '^((?:/(.*?))?/?)$') ?? '', /^\/_isr\?/);
+	});
+
+	it('captures the full pathname as group 1 of the emitted root and spread patterns', {
+		timeout: 30000,
+	}, async () => {
+		const { routes } = await getVercelConfig(fixture);
+		const emitted = new Set(routes.map((route) => route.src));
+		const shapes: Array<[src: string, pathname: string]> = [
+			['^(/)$', '/'],
+			['^((?:/(.*?))?/?)$', '/a/b'],
+		];
+		for (const [src, pathname] of shapes) {
+			assert.ok(emitted.has(src), `expected the config to emit ${src}`);
+			const match = new RegExp(src).exec(pathname);
+			assert.ok(match, `${src} must match ${pathname}`);
+			assert.equal(match[1], pathname, `group 1 of ${src} on ${pathname}`);
+		}
+	});
+
+	/** The build's path token, read back out of an ISR route rewrite. */
+	async function readPathToken(): Promise<string> {
+		const { routes } = await getVercelConfig(fixture);
+		const isrRoute = routes.find(
+			(route: { dest?: string }) =>
+				typeof route.dest === 'string' && route.dest.startsWith('/_isr?'),
+		);
+		return new URL(isrRoute!.dest, 'https://example.com').searchParams.get('x_astro_path_token')!;
+	}
+
+	async function isrFetch(path: string): Promise<Response> {
+		const functionConfig = JSON.parse(
+			await fixture.readFile('../.vercel/output/functions/_isr.func/.vc-config.json'),
+		);
+		const entry = new URL(
+			`../.vercel/output/functions/_isr.func/${functionConfig.handler}`,
+			fixture.config.outDir,
+		);
+		const module = await import(entry.href);
+		return module.default.fetch(new Request(`https://example.com${path}`));
+	}
+
+	// The fixture's catch-all ([...slug].astro) renders every pathname, so a
+	// trusted override that is ignored falls through to `/_isr` and would return
+	// 200 here. These are the regression tests for the fail-closed guard: a
+	// trusted but malformed override must 404, not render the catch-all.
+	it('returns 404 for a trusted override that does not start with /', {
+		timeout: 30000,
+	}, async () => {
+		const token = await readPathToken();
+		// $0 is the reported failure value (issue #18028); $1 is what a failing
+		// substitution leaves after the root fix; `relative` is any other
+		// non-absolute value.
+		for (const badPath of ['$0', '$1', 'relative', '']) {
+			const response = await isrFetch(`/_isr?x_astro_path=${badPath}&x_astro_path_token=${token}`);
+			assert.equal(response.status, 404, `expected 404 for x_astro_path=${badPath}`);
+			assert.equal(
+				response.headers.get('location'),
+				null,
+				`no redirect for x_astro_path=${badPath}`,
+			);
+			assert.equal(
+				(await response.text()).includes('Slug:'),
+				false,
+				`catch-all must not render for x_astro_path=${badPath}`,
+			);
+		}
+	});
+
+	it('returns 404 for a trusted override with no path at all', {
+		timeout: 30000,
+	}, async () => {
+		const token = await readPathToken();
+		const response = await isrFetch(`/_isr?x_astro_path_token=${token}`);
+		assert.equal(response.status, 404);
+		assert.equal(response.headers.get('location'), null);
+	});
+
+	it('still renders a valid trusted override on the catch-all fixture', {
+		timeout: 30000,
+	}, async () => {
+		const token = await readPathToken();
+		const response = await isrFetch(`/_isr?x_astro_path=/cached/42&x_astro_path_token=${token}`);
+		assert.equal(response.status, 200);
+		assert.equal((await response.text()).includes('Slug: cached/42'), true);
 	});
 
 	it('builds no middleware function', { timeout: 30000 }, async () => {

packages/integrations/vercel/test/isr.test.ts

@@ -68,12 +68,16 @@ describe('ISR', () => {
 				dest: '_render',
 			},
 			{
-				src: '^/one/?$',
-				dest: '/_isr?x_astro_path=$0&x_astro_path_token=$TOKEN',
+				src: '^(/one/?)$',
+				dest: '/_isr?x_astro_path=$1&x_astro_path_token=$TOKEN',
 			},
 			{
-				src: '^/404/?$',
-				dest: '/_isr?x_astro_path=$0&x_astro_path_token=$TOKEN',
+				src: '^(/([^/]+?)/([^/]+?)/?)$',
+				dest: '/_isr?x_astro_path=$1&x_astro_path_token=$TOKEN',
+			},
+			{
+				src: '^(/404/?)$',
+				dest: '/_isr?x_astro_path=$1&x_astro_path_token=$TOKEN',
 			},
 			{
 				dest: '_render',
@@ -156,4 +160,92 @@ describe('ISR', () => {
 		assert.equal(response.status, 200);
 		assert.equal((await response.text()).includes('<h1>One</h1>'), true);
 	});
+
+	it('honors x_astro_path for dynamic routes when the valid path token is present', {
+		timeout: 30000,
+	}, async () => {
+		const isrFunction = await loadIsrFunction();
+		const token = await readPathToken();
+		const response = await isrFunction.default.fetch(
+			new Request(`https://example.com/_isr?x_astro_path=/a/b&x_astro_path_token=${token}`),
+		);
+		assert.equal(response.status, 200);
+		assert.equal((await response.text()).includes('<h1>A: a B: b</h1>'), true);
+	});
+
+	it('returns 404 for a trusted override that does not start with /', {
+		timeout: 30000,
+	}, async () => {
+		const isrFunction = await loadIsrFunction();
+		const token = await readPathToken();
+		// Values Vercel leaves behind when it fails to substitute the $1 capture
+		// group reference in an ISR rewrite (issue #18028), plus other non-`/`
+		// values. Without the guard these resolve to nonsense pathnames like /$1
+		// and, with trailingSlash 'always', a cacheable redirect to /$1/.
+		for (const badPath of ['$1', '$0', 'relative']) {
+			const response = await isrFunction.default.fetch(
+				new Request(`https://example.com/_isr?x_astro_path=${badPath}&x_astro_path_token=${token}`),
+			);
+			assert.equal(response.status, 404, `expected 404 for x_astro_path=${badPath}`);
+			assert.equal(
+				response.headers.get('location'),
+				null,
+				`no redirect for x_astro_path=${badPath}`,
+			);
+		}
+	});
+
+	it('uses $1 (not $0) in ISR route dest', { timeout: 30000 }, async () => {
+		const deploymentConfig = JSON.parse(await fixture.readFile('../.vercel/output/config.json'));
+		const isrRoutes = deploymentConfig.routes.filter(
+			(route: { dest?: string }) =>
+				typeof route.dest === 'string' && route.dest.startsWith('/_isr?'),
+		);
+		assert.ok(isrRoutes.length > 0, 'expected at least one ISR route');
+		for (const route of isrRoutes) {
+			assert.ok(
+				route.dest.includes('x_astro_path=$1'),
+				`ISR dest must reference $1, got: ${route.dest}`,
+			);
+			assert.ok(
+				!route.dest.includes('x_astro_path=$0'),
+				`ISR dest must not reference $0, got: ${route.dest}`,
+			);
+			// The $1 reference requires the full match to be group 1, so the
+			// pattern is wrapped in an outer capture group.
+			assert.ok(
+				route.src.startsWith('^(') && route.src.endsWith(')$'),
+				`ISR src must wrap the pattern in an outer capture group, got: ${route.src}`,
+			);
+		}
+	});
+
+	it('captures the full pathname as group 1 of every emitted ISR pattern', {
+		timeout: 30000,
+	}, async () => {
+		const deploymentConfig = JSON.parse(await fixture.readFile('../.vercel/output/config.json'));
+		const emittedSrcs = new Set(
+			deploymentConfig.routes
+				.filter(
+					(route: { dest?: string }) =>
+						typeof route.dest === 'string' && route.dest.startsWith('/_isr?'),
+				)
+				.map((route: { src: string }) => route.src),
+		);
+		// For each emitted pattern, Vercel substitutes group 1 for $1 in `dest`:
+		// it must be the full pathname for every route shape.
+		const shapes: Array<[src: string, pathname: string]> = [
+			['^(/one/?)$', '/one'],
+			['^(/one/?)$', '/one/'],
+			['^(/([^/]+?)/([^/]+?)/?)$', '/a/b'],
+			['^(/([^/]+?)/([^/]+?)/?)$', '/a/b/'],
+			['^(/404/?)$', '/404/'],
+		];
+		for (const [src, pathname] of shapes) {
+			assert.ok(emittedSrcs.has(src), `expected the config to emit ${src}`);
+			const match = new RegExp(src).exec(pathname);
+			assert.ok(match, `${src} must match ${pathname}`);
+			assert.equal(match[1], pathname, `group 1 of ${src} on ${pathname}`);
+		}
+	});
 });