diff --git a/.changeset/fix-isr-dollar-zero.md b/.changeset/fix-isr-dollar-zero.md new file mode 100644 index 000000000000..71889defdbd3 --- /dev/null +++ b/.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. \ No newline at end of file diff --git a/packages/integrations/vercel/src/index.ts b/packages/integrations/vercel/src/index.ts index 24b5a4741018..cc7fbf9a058e 100644 --- a/packages/integrations/vercel/src/index.ts +++ b/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 `$`, 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) { diff --git a/packages/integrations/vercel/src/serverless/entrypoint.ts b/packages/integrations/vercel/src/serverless/entrypoint.ts index 29272688ff78..d28c61ac7381 100644 --- a/packages/integrations/vercel/src/serverless/entrypoint.ts +++ b/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. diff --git a/packages/integrations/vercel/test/fixtures/isr-edge-no-middleware/src/pages/[...slug].astro b/packages/integrations/vercel/test/fixtures/isr-edge-no-middleware/src/pages/[...slug].astro new file mode 100644 index 000000000000..002deb42abf1 --- /dev/null +++ b/packages/integrations/vercel/test/fixtures/isr-edge-no-middleware/src/pages/[...slug].astro @@ -0,0 +1,8 @@ + + + Spread + + +

Slug: {Astro.params.slug}

+ + diff --git a/packages/integrations/vercel/test/fixtures/isr/src/pages/[a]/[b].astro b/packages/integrations/vercel/test/fixtures/isr/src/pages/[a]/[b].astro new file mode 100644 index 000000000000..4f8c25046e19 --- /dev/null +++ b/packages/integrations/vercel/test/fixtures/isr/src/pages/[a]/[b].astro @@ -0,0 +1,8 @@ + + + AB + + +

A: {Astro.params.a} B: {Astro.params.b}

+ + diff --git a/packages/integrations/vercel/test/isr-edge-middleware.test.ts b/packages/integrations/vercel/test/isr-edge-middleware.test.ts index 2d838083bdad..ac4f5a429641 100644 --- a/packages/integrations/vercel/test/isr-edge-middleware.test.ts +++ b/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 { + 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 { + 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 () => { diff --git a/packages/integrations/vercel/test/isr.test.ts b/packages/integrations/vercel/test/isr.test.ts index 4f97a1896889..2b3e5da0b280 100644 --- a/packages/integrations/vercel/test/isr.test.ts +++ b/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('

One

'), 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('

A: a B: b

'), 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}`); + } + }); });