From 0091dc863bf81db391311b37186f660067d9264c Mon Sep 17 00:00:00 2001 From: chinesepowered <22500229+chinesepowered@users.noreply.github.com> Date: Mon, 27 Jul 2026 19:14:51 -0700 Subject: [PATCH] [workers-shared] Match whole placeholders when compiling _redirects rules Substituting on the raw `:name` text let a shorter placeholder match inside a longer one sharing its prefix. For `/p/:id/:id_2` this emitted the `id` capture group twice, and the resulting SyntaxError was swallowed by generateRulesMatcher's catch, so the rule was silently dropped. The same collision corrupted replacer's output. Both now do a single regex-driven pass over whole placeholder matches. --- .../rules-engine-placeholder-collision.md | 9 ++++++ .../asset-worker/src/utils/rules-engine.ts | 32 ++++++++++++------- .../asset-worker/tests/rules-engine.test.ts | 31 ++++++++++++++++++ 3 files changed, 60 insertions(+), 12 deletions(-) create mode 100644 .changeset/rules-engine-placeholder-collision.md diff --git a/.changeset/rules-engine-placeholder-collision.md b/.changeset/rules-engine-placeholder-collision.md new file mode 100644 index 00000000000..c0dc00e219e --- /dev/null +++ b/.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. diff --git a/packages/workers-shared/asset-worker/src/utils/rules-engine.ts b/packages/workers-shared/asset-worker/src/utils/rules-engine.ts index 70c790d9c1c..4a2b18f3baf 100644 --- a/packages/workers-shared/asset-worker/src/utils/rules-engine.ts +++ b/packages/workers-shared/asset-worker/src/utils/rules-engine.ts @@ -22,10 +22,14 @@ export type Replacements = Record; 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 `(?…)` 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 + "$"; diff --git a/packages/workers-shared/asset-worker/tests/rules-engine.test.ts b/packages/workers-shared/asset-worker/tests/rules-engine.test.ts index 726b2ff6507..c91ed364dfa 100644 --- a/packages/workers-shared/asset-worker/tests/rules-engine.test.ts +++ b/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: ; rel=preload; as=script, ; 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", () => {