Skip to content

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

Open
chinesepowered wants to merge 1 commit into
cloudflare:mainfrom
chinesepowered:fix-rules-engine-placeholder-collision
Open

[workers-shared] Fix _redirects/_headers rules silently dropped when placeholders share a prefix#14890
chinesepowered wants to merge 1 commit into
cloudflare:mainfrom
chinesepowered:fix-rules-engine-placeholder-collision

Conversation

@chinesepowered

@chinesepowered chinesepowered commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Fixes #14887.

When two placeholders in a _redirects or _headers rule share a prefix — e.g. :id and :id_2 — the rule is silently dropped at runtime. Both bugs come from the same cause: substituting on a raw :name string rather than on a placeholder-aware match, so a shorter name matches inside a longer one.

generateRuleRegExp built an invalid regex. Splitting on the literal ":id" also split inside ":id_2", so both placeholders became the same named group:

generateRuleRegExp("/p/:id/:id_2")
  -> SyntaxError: /^\/p\/(?<id>[^/]+)\/(?<id>[^/]+)_2$/: Duplicate capture group name

generateRulesMatcher compiles rules inside a bare catch {}, so this was swallowed and the rule was dropped from compiledRules entirely — no warning, no log, the rule just never matched.

replacer substituted the shorter placeholder first, corrupting the longer one: replacer("/dest/:id/:id_2", { id: "1", id_2: "2" }) returned /dest/1/1_2 rather than /dest/1/2.

Placeholder names are already fully specified by PLACEHOLDER_REGEX (/:([A-Za-z]\w*)/g), so :id_2 is one token and not a :id followed by _2. Both functions now do a single regex-driven pass, matching each placeholder whole. This also fixes a third-order effect of the old sequential loop: a replacement value containing :name could be re-substituted by a later iteration.

I left the bare catch {} in generateRulesMatcher alone — an invalid rule vanishing without a diagnostic is what makes this class of bug hard to find, but changing it is a separate behavioural decision and I didn't want to bundle it in. Happy to follow up if you'd like it.


  • Tests
    • Tests included/updated
    • Automated tests not possible - manual testing has been completed as follows:
    • Additional testing not necessary because:
  • Public documentation
    • Cloudflare docs PR(s):
    • Documentation not necessary because: this makes placeholder matching behave as already documented — :id_2 was always meant to be one placeholder. No syntax or interface change.

Four tests added. Three of them fail without the source change, with exactly the symptoms above:

× it should support placeholders sharing a prefix
    AssertionError: expected [] to deeply equal [ '/dest/1/2' ]
× should not let a placeholder eat a longer one sharing its prefix
    AssertionError: expected '/new/X/Xb' to deeply equal '/new/X/Y'
× should not re-substitute inside an already-replaced value
    AssertionError: expected '/second/second' to deeply equal '/:b/second'

The fourth (should leave placeholders with no replacement alone) passes either way and is there to pin the no-replacement behaviour against the rewrite. Full rules-engine.test.ts passes (15 tests).

Note: asset-worker has 3 pre-existing failures on main (assets-manifest.test.ts, plus two 5s timeouts in the KV retry tests). I confirmed they fail identically with and without this change.


Open in Devin Review

…ules

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.
@changeset-bot

changeset-bot Bot commented Jul 28, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 0091dc8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@cloudflare/workers-shared Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-project-automation github-project-automation Bot moved this to Untriaged in workers-sdk Jul 28, 2026
@workers-devprod
workers-devprod requested review from a team and jamesopstad and removed request for a team July 28, 2026 02:15
@workers-devprod

Copy link
Copy Markdown
Contributor

Codeowners approval required for this PR:

  • @cloudflare/deploy-config
  • @cloudflare/wrangler
Show detailed file reviewers
  • .changeset/rules-engine-placeholder-collision.md: [@cloudflare/wrangler]
  • packages/workers-shared/asset-worker/src/utils/rules-engine.ts: [@cloudflare/deploy-config @cloudflare/wrangler]
  • packages/workers-shared/asset-worker/tests/rules-engine.test.ts: [@cloudflare/deploy-config @cloudflare/wrangler]

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

@pkg-pr-new

pkg-pr-new Bot commented Jul 28, 2026

Copy link
Copy Markdown
@cloudflare/autoconfig

npm i https://pkg.pr.new/@cloudflare/autoconfig@14890

@cloudflare/config

npm i https://pkg.pr.new/@cloudflare/config@14890

create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@14890

@cloudflare/deploy-helpers

npm i https://pkg.pr.new/@cloudflare/deploy-helpers@14890

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@14890

miniflare

npm i https://pkg.pr.new/miniflare@14890

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@14890

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@14890

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@14890

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@14890

@cloudflare/workers-auth

npm i https://pkg.pr.new/@cloudflare/workers-auth@14890

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@14890

@cloudflare/workers-utils

npm i https://pkg.pr.new/@cloudflare/workers-utils@14890

wrangler

npm i https://pkg.pr.new/wrangler@14890

commit: 0091dc8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Untriaged

Development

Successfully merging this pull request may close these issues.

[workers-shared] _redirects/_headers rules with prefix-colliding placeholders are silently dropped

2 participants