[workers-shared] Fix _redirects/_headers rules silently dropped when placeholders share a prefix

.changeset/rules-engine-placeholder-collision.md

@@ -0,0 +1,9 @@
+---
+"@cloudflare/workers-shared": patch
+---
+
+Fix `_redirects`/`_headers` rules being silently dropped when two placeholders share a prefix
+
+A rule such as `/p/:id/:id_2` compiled to a regex containing the `id` capture group twice, because each placeholder was substituted by splitting on its raw `:name` text — which also split inside the longer placeholder. The resulting `SyntaxError: Duplicate capture group name` was swallowed by the rule compiler's `catch`, so the rule was dropped with no diagnostic and simply never fired.
+
+The same prefix collision corrupted substitution into the destination: `replacer("/dest/:id/:id_2", { id: "1", id_2: "2" })` returned `/dest/1/1_2` instead of `/dest/1/2`. Both functions now match whole placeholders in a single pass, which also stops a replacement value that happens to contain `:name` from being substituted again.

packages/workers-shared/asset-worker/src/utils/rules-engine.ts

@@ -22,10 +22,14 @@ export type Replacements = Record<string, string>;
 export type Removals = string[];
 
 export const replacer = (str: string, replacements: Replacements) => {
-	for (const [replacement, value] of Object.entries(replacements)) {
-		str = str.replaceAll(`:${replacement}`, value);
-	}
-	return str;
+	// Substituting one placeholder name at a time would let a shorter name eat a
+	// longer one that shares its prefix (`:id` matching inside `:id_2`), so match
+	// whole placeholders in a single pass instead. Placeholders with no
+	// replacement are left alone.
+	return str.replace(PLACEHOLDER_REGEX, (match, name: string) => {
+		const value = replacements[name];
+		return value !== undefined ? value : match;
+	});
 };
 
 export const generateGlobOnlyRuleRegExp = (rule: string) => {
@@ -49,15 +53,19 @@ export const generateRuleRegExp = (rule: string) => {
 	// e.g. https://:subdomain.domain/ -> https://(here).domain/
 	// e.g. /static/:file -> /static/(image.jpg)
 	// e.g. /blog/:post -> /blog/(an-exciting-post)
-	const host_matches = rule.matchAll(HOST_PLACEHOLDER_REGEX);
-	for (const host_match of host_matches) {
-		rule = rule.split(host_match[0]).join(`(?<${host_match[1]}>[^/.]+)`);
-	}
+	// Each placeholder is substituted as a whole match rather than by splitting on
+	// its raw `:name` text, which would also split inside a longer placeholder
+	// sharing the same prefix and emit the same capture group twice
+	// (e.g. `/p/:id/:id_2` -> a regex with two `(?<id>…)` groups, which throws).
+	rule = rule.replace(
+		HOST_PLACEHOLDER_REGEX,
+		(_match, name: string) => `(?<${name}>[^/.]+)`
+	);
 
-	const path_matches = rule.matchAll(PLACEHOLDER_REGEX);
-	for (const path_match of path_matches) {
-		rule = rule.split(path_match[0]).join(`(?<${path_match[1]}>[^/]+)`);
-	}
+	rule = rule.replace(
+		PLACEHOLDER_REGEX,
+		(_match, name: string) => `(?<${name}>[^/]+)`
+	);
 
 	// Wrap in line terminators to be safe.
 	rule = "^" + rule + "$";

packages/workers-shared/asset-worker/tests/rules-engine.test.ts

@@ -89,6 +89,16 @@ describe("rules engine", () => {
 			matcher({ request: new Request("https://next.my.pages.dev/magic") })
 		).toEqual(["6/my/"]);
 	});
+
+	test("it should support placeholders sharing a prefix", ({ expect }) => {
+		const matcher = generateRulesMatcher(
+			{ "/p/:id/:id_2": "/dest/:id/:id_2" },
+			(match, replacements) => replacer(match, replacements)
+		);
+		expect(
+			matcher({ request: new Request("https://example.com/p/1/2") })
+		).toEqual(["/dest/1/2"]);
+	});
 });
 
 describe("replacer", () => {
@@ -120,6 +130,27 @@ describe("replacer", () => {
 			"Link: </assets/js/main.js>; rel=preload; as=script, </assets/js/lang.js>; rel=preload; as=script"
 		);
 	});
+
+	test("should not let a placeholder eat a longer one sharing its prefix", ({
+		expect,
+	}) => {
+		expect(replacer("/new/:a/:ab", { a: "X", ab: "Y" })).toEqual("/new/X/Y");
+		expect(replacer("/dest/:id/:id_2", { id: "1", id_2: "2" })).toEqual(
+			"/dest/1/2"
+		);
+	});
+
+	test("should leave placeholders with no replacement alone", ({ expect }) => {
+		expect(replacer("/:code/:missing", { code: "123" })).toEqual(
+			"/123/:missing"
+		);
+	});
+
+	test("should not re-substitute inside an already-replaced value", ({
+		expect,
+	}) => {
+		expect(replacer("/:a/:b", { a: ":b", b: "second" })).toEqual("/:b/second");
+	});
 });
 
 describe("static routing rules", () => {