fix: named captures in has fields not applying to route dest (#449)

.changeset/famous-dragons-joke.md

@@ -0,0 +1,5 @@
+---
+'@cloudflare/next-on-pages': patch
+---
+
+Fix named capture groups in `has` entries in the build output config not applying to route destination paths.

packages/next-on-pages/templates/_worker.js/routes-matcher.ts

@@ -5,7 +5,7 @@ import {
 	applyHeaders,
 	applyPCREMatches,
 	applySearchParams,
-	hasField,
+	checkhasField,
 	getNextPhase,
 	isUrl,
 	matchPCRE,
@@ -91,7 +91,7 @@ export class RoutesMatcher {
 	private checkRouteMatch(
 		route: VercelSource,
 		checkStatus?: boolean,
-	): MatchPCREResult | undefined {
+	): { routeMatch: MatchPCREResult; routeDest?: string } | undefined {
 		const srcMatch = matchPCRE(route.src, this.path, route.caseSensitive);
 		if (!srcMatch.match) return;
 
@@ -109,15 +109,25 @@ export class RoutesMatcher {
 			url: this.url,
 			cookies: this.cookies,
 			headers: this.reqCtx.request.headers,
+			routeDest: route.dest,
 		};
 
 		// All `has` conditions must be met - skip if one is not met.
-		if (route.has?.find(has => !hasField(has, hasFieldProps))) {
+		if (
+			route.has?.find(has => {
+				const result = checkhasField(has, hasFieldProps);
+				if (result.newRouteDest) {
+					// If the `has` condition had a named capture to update the destination, update it.
+					hasFieldProps.routeDest = result.newRouteDest;
+				}
+				return !result.valid;
+			})
+		) {
 			return;
 		}
 
 		// All `missing` conditions must not be met - skip if one is met.
-		if (route.missing?.find(has => hasField(has, hasFieldProps))) {
+		if (route.missing?.find(has => checkhasField(has, hasFieldProps).valid)) {
 			return;
 		}
 
@@ -126,7 +136,7 @@ export class RoutesMatcher {
 			return;
 		}
 
-		return srcMatch;
+		return { routeMatch: srcMatch, routeDest: hasFieldProps.routeDest };
 	}
 
 	/**
@@ -428,8 +438,11 @@ export class RoutesMatcher {
 		phase: VercelPhase,
 		rawRoute: VercelSource,
 	): Promise<CheckRouteStatus> {
-		const route = this.getLocaleFriendlyRoute(rawRoute, phase);
-		const routeMatch = this.checkRouteMatch(route, phase === 'error');
+		const localeFriendlyRoute = this.getLocaleFriendlyRoute(rawRoute, phase);
+		const { routeMatch, routeDest } =
+			this.checkRouteMatch(localeFriendlyRoute, phase === 'error') ?? {};
+
+		const route: VercelSource = { ...localeFriendlyRoute, dest: routeDest };
 
 		// If this route doesn't match, continue to the next one.
 		if (!routeMatch?.match) return 'skip';

packages/next-on-pages/templates/_worker.js/utils/matcher.ts

@@ -1,46 +1,86 @@
+import { applyPCREMatches, matchPCRE } from './pcre';
+
 type HasFieldRequestProperties = {
 	url: URL;
 	cookies: Record<string, string>;
 	headers: Headers;
+	routeDest?: string;
 };
 
 /**
- * Checks if a Vercel source route's `has` record conditions match a request.
+ * Checks if a Vercel source route's `has` record conditions match a request, and whether the request
+ * destination should be updated based on the `has` record.
  *
  * @param has The `has` record conditions to check against the request.
  * @param requestProperties The request properties to check against.
- * @returns Whether the request matches the `has` record conditions.
+ * @returns Whether the request matches the `has` record conditions, and the new destination if it changed.
  */
-export function hasField(
+export function checkhasField(
 	has: VercelHasField,
-	{ url, cookies, headers }: HasFieldRequestProperties,
-): boolean {
+	{ url, cookies, headers, routeDest }: HasFieldRequestProperties,
+): { valid: boolean; newRouteDest?: string } {
 	switch (has.type) {
 		case 'host': {
-			return url.hostname === has.value;
+			return { valid: url.hostname === has.value };
 		}
 		case 'header': {
 			if (has.value !== undefined) {
-				return !!headers.get(has.key)?.match(has.value);
+				return getHasFieldPCREMatchResult(
+					has.value,
+					headers.get(has.key),
+					routeDest,
+				);
 			}
 
-			return headers.has(has.key);
+			return { valid: headers.has(has.key) };
 		}
 		case 'cookie': {
 			const cookie = cookies[has.key];
 
-			if (has.value !== undefined) {
-				return !!cookie?.match(has.value);
+			if (cookie && has.value !== undefined) {
+				return getHasFieldPCREMatchResult(has.value, cookie, routeDest);
 			}
 
-			return cookie !== undefined;
+			return { valid: cookie !== undefined };
 		}
 		case 'query': {
 			if (has.value !== undefined) {
-				return !!url.searchParams.get(has.key)?.match(has.value);
+				return getHasFieldPCREMatchResult(
+					has.value,
+					url.searchParams.get(has.key),
+					routeDest,
+				);
 			}
 
-			return url.searchParams.has(has.key);
+			return { valid: url.searchParams.has(has.key) };
 		}
 	}
 }
+
+/**
+ * Gets the has field PCRE match results, and tries to apply any named capture groups to a
+ * route destination.
+ *
+ * @param hasValue The has field value to match against.
+ * @param foundValue The value found in the request.
+ * @param routeDest Destination to apply match to.
+ * @returns Whether the match is valid, and the destination with the match applied.
+ */
+function getHasFieldPCREMatchResult(
+	hasValue: string,
+	foundValue: string | null,
+	routeDest?: string,
+): { valid: boolean; newRouteDest?: string } {
+	const { match, captureGroupKeys } = matchPCRE(hasValue, foundValue);
+
+	if (routeDest && match && captureGroupKeys.length) {
+		return {
+			valid: !!match,
+			newRouteDest: applyPCREMatches(routeDest, match, captureGroupKeys, {
+				namedOnly: true,
+			}),
+		};
+	}
+
+	return { valid: !!match };
+}

packages/next-on-pages/templates/_worker.js/utils/pcre.ts

@@ -19,9 +19,13 @@ export type MatchPCREResult = {
  */
 export function matchPCRE(
 	expr: string,
-	val: string,
+	val: string | undefined | null,
 	caseSensitive?: boolean,
 ): MatchPCREResult {
+	if (val === null || val === undefined) {
+		return { match: null, captureGroupKeys: [] };
+	}
+
 	const flag = caseSensitive ? '' : 'i';
 	const captureGroupKeys: string[] = [];
 
@@ -37,15 +41,23 @@ export function matchPCRE(
  * @param rawStr String to process.
  * @param match Matches from the PCRE matcher.
  * @param captureGroupKeys Named capture group keys from the PCRE matcher.
+ * @param opts Options for applying the PCRE matches.
  * @returns The processed string with replaced parameters.
  */
 export function applyPCREMatches(
 	rawStr: string,
 	match: RegExpMatchArray,
 	captureGroupKeys: string[],
+	{ namedOnly }: { namedOnly?: boolean } = {},
 ): string {
-	return rawStr.replace(/\$([a-zA-Z0-9_]+)/g, (_, key) => {
+	return rawStr.replace(/\$([a-zA-Z0-9_]+)/g, (originalValue, key) => {
 		const index = captureGroupKeys.indexOf(key);
+
+		// If we only want named capture groups, and the key is not found, return the original value.
+		if (namedOnly && index === -1) {
+			return originalValue;
+		}
+
 		// If the extracted key does not exist as a named capture group from the matcher, we can
 		// reasonably assume it's a number and return the matched index. Fallback to an empty string.
 		return (index === -1 ? match[parseInt(key, 10)] : match[index + 1]) || '';

packages/next-on-pages/tests/templates/utils/matcher.test.ts

@@ -1,101 +1,140 @@
 import { describe, test, expect } from 'vitest';
 import { parse } from 'cookie';
-import { hasField } from '../../../templates/_worker.js/utils';
+import { checkhasField } from '../../../templates/_worker.js/utils';
 
 type HasFieldTestCase = {
 	name: string;
 	has: VercelHasField;
-	expected: boolean;
+	dest?: string;
+	expected: { valid: boolean; newRouteDest?: string };
 };
 
 const req = new Request(
-	'https://test.com/index?queryWithValue=value&queryWithoutValue=',
+	'https://test.com/index?queryWithValue=value&queryWithoutValue=&source=query',
 	{
 		headers: {
+			source: 'header',
 			headerWithValue: 'value',
 			headerWithoutValue: undefined as unknown as string,
-			cookie: 'cookieWithValue=value; cookieWithoutValue=',
+			cookie: 'cookieWithValue=value; cookieWithoutValue=; source=cookie',
 		},
 	},
 );
 const url = new URL(req.url);
 const cookies = parse(req.headers.get('cookie') ?? '');
 
-describe('hasField', () => {
+describe('checkhasField', () => {
 	const testCases: HasFieldTestCase[] = [
 		{
 			name: 'host: valid host returns true',
 			has: { type: 'host', value: 'test.com' },
-			expected: true,
+			expected: { valid: true },
 		},
 		{
 			name: 'host: invalid host returns false',
 			has: { type: 'host', value: 'test2.com' },
-			expected: false,
+			expected: { valid: false },
 		},
 		{
 			name: 'header: has with key+value match returns true',
 			has: { type: 'header', key: 'headerWithValue', value: 'value' },
-			expected: true,
+			expected: { valid: true },
 		},
 		{
 			name: 'header: has with key+value mismatch returns false',
 			has: { type: 'header', key: 'headerWithValue', value: 'value2' },
-			expected: false,
+			expected: { valid: false },
 		},
 		{
 			name: 'header: has with key match returns true',
 			has: { type: 'header', key: 'headerWithoutValue' },
-			expected: true,
+			expected: { valid: true },
 		},
 		{
 			name: 'header: has with key but no value mismatch returns false',
 			has: { type: 'header', key: 'headerWithoutValue', value: 'value' },
-			expected: false,
+			expected: { valid: false },
 		},
 		{
 			name: 'cookie: has with key+value match returns true',
 			has: { type: 'cookie', key: 'cookieWithValue', value: 'value' },
-			expected: true,
+			expected: { valid: true },
 		},
 		{
 			name: 'cookie: has with key+value mismatch returns false',
 			has: { type: 'cookie', key: 'cookieWithValue', value: 'alt-value' },
-			expected: false,
+			expected: { valid: false },
 		},
 		{
 			name: 'cookie: has with key match returns true',
 			has: { type: 'cookie', key: 'cookieWithValue' },
-			expected: true,
+			expected: { valid: true },
 		},
 		{
 			name: 'query: has with key+value match returns true',
 			has: { type: 'query', key: 'queryWithValue', value: 'value' },
-			expected: true,
+			expected: { valid: true },
 		},
 		{
 			name: 'query: has with key+value mismatch returns false',
 			has: { type: 'query', key: 'queryWithValue', value: 'alt-value' },
-			expected: false,
+			expected: { valid: false },
 		},
 		{
 			name: 'query: has with key match returns true',
 			has: { type: 'query', key: 'queryWithoutValue' },
-			expected: true,
+			expected: { valid: true },
 		},
 		{
 			name: 'query: has with key but no value mismatch returns false',
 			has: { type: 'query', key: 'queryWithoutValue', value: 'value' },
-			expected: false,
+			expected: { valid: false },
+		},
+		{
+			name: 'query: has with named capture returns a new dest on match',
+			has: { type: 'query', key: 'source', value: '(?<source>.*)' },
+			dest: '/source/$source',
+			expected: { valid: true, newRouteDest: '/source/query' },
+		},
+		{
+			name: 'query: has with named capture does not update missing named captures in dest',
+			has: { type: 'query', key: 'source', value: '(?<source>.*)' },
+			dest: '/source/$source/$age',
+			expected: { valid: true, newRouteDest: '/source/query/$age' },
+		},
+		{
+			name: 'query: has with named capture return valid on match when key is not in dest',
+			has: { type: 'query', key: 'source', value: '(?<source>.*)' },
+			dest: '/source/$age',
+			expected: { valid: true, newRouteDest: '/source/$age' },
+		},
+		{
+			name: 'query: has with named capture does not return dest on no matches',
+			has: { type: 'query', key: 'invalidKey', value: '(?<source>.*)' },
+			dest: '/source/$source',
+			expected: { valid: false, newRouteDest: undefined },
+		},
+		{
+			name: 'header: has with named capture returns a new dest on match',
+			has: { type: 'header', key: 'source', value: '(?<source>.*)' },
+			dest: '/source/$source',
+			expected: { valid: true, newRouteDest: '/source/header' },
+		},
+		{
+			name: 'cookie: has with named capture returns a new dest on match',
+			has: { type: 'cookie', key: 'source', value: '(?<source>.*)' },
+			dest: '/source/$source',
+			expected: { valid: true, newRouteDest: '/source/cookie' },
 		},
 	];
 
 	testCases.forEach(testCase => {
 		test(testCase.name, () => {
-			const result = hasField(testCase.has, {
+			const result = checkhasField(testCase.has, {
 				url,
 				cookies,
 				headers: req.headers,
+				routeDest: testCase.dest,
 			});
 			expect(result).toEqual(testCase.expected);
 		});