Repository navigation
fix(config): match redirects, rewrites and headers sources with path-to-regexp - #4062
james-elicx wants to merge 57 commits into
Conversation
|
/bigbonk review for issues |
@vinext/cloudflare
create-vinext-app
@vinext/types
vinext
@cloudflare/workers-response-store
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
|
Review: 3 findings. Posted three inline findings: one blocking ReDoS issue and two matcher parity bugs. |
|
/bigbonk review for issues |
Performance benchmarksCompared 0 improved · 1 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
Review: 5 findings. Posted five inline findings: one blocking ReDoS bypass and four matcher/config parity defects. Reviewed exact head |
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow timed out. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
|
Review: 2 findings. Posted two inline warnings on exact head |
|
/bigbonk review for issues |
|
Review: 1 findings. Posted one inline warning on source validation parity. Targeted tests pass; branch unchanged. |
|
/bigbonk review for issues |
|
Review: 3 findings. Posted three inline findings: one blocking ReDoS regression and two matcher/config parity defects. Targeted tests pass; branch unchanged. |
|
/bigbonk review for issues |
|
Review: 3 findings. Posted three inline findings: one blocking ReDoS bypass and two client matcher parity defects. Reviewed exact head |
|
/bigbonk review for issues |
|
Review: 4 findings. Posted four inline findings: one blocking ReDoS bypass and three matcher/config parity defects. Targeted tests pass; branch unchanged. |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
|
/bigbonk review for issues |
|
Review: 1 finding. Posted one inline warning for optional unnamed groups bypassing client redirect/rewrite matching. Targeted tests pass; branch unchanged. |
|
/bigbonk review for issues @ask-bonk, on the optional unnamed group finding: I couldn't reproduce it on aacf25c. |
|
LGTM! |
|
Review: 1 findings. Posted one inline warning about rewrite validation running after unsafe dereferences. Reviewed exact |
|
/bigbonk review for issues @ask-bonk, on rewrite validation running after field reads: confirmed and fixed in a094f42. Redirect and rewrite sources are now validated before the @clanky-the-clanker, on the weak skip test: fixed in 0b9823a. "leaves ReDoS-unsafe sources to warn and skip when matched" now also asserts the |
|
Review: 1 findings.
Targeted tests passed: |
|
/bigbonk review for issues @ask-bonk, on the unserializable source: confirmed and fixed in 0f0e0b7. The invalid-source message now quotes a string source as before and shows any other value by its type, for example @clanky-the-clanker, on the |
|
LGTM! |
|
/bigbonk review for issues |
|
LGTM! |
…ub.com/cloudflare/vinext into fix/config-path-to-regexp # Conflicts: # packages/vinext/src/config/config-matchers.ts
|
/bigbonk review for issues |
|
Review: 1 findings.
Targeted matcher tests pass; branch unchanged. |
|
Re the clanky warning about |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
|
LGTM! |
1 participant
Summary
next.config.js
redirects,rewritesandheaderssources were matched by a hand-rolled tokenizer (plus a separateescapeHeaderSourcefor headers), which diverged from Next.js in many shapes. This PR compiles them the way Next.js 16 does at runtime instead: path-to-regexp 6 (strict: true,delimiter: "/", case-insensitive) plusmodifyRouteRegex's optional trailing slash, using the path-to-regexp port vinext already vendors for middleware matchers. Custom param patterns go through the same per-token ReDoS validation middleware matchers use.Supersedes #4029, which fixed one of these divergences by hand.
Fixed divergences (all checked against Next's own compiled
path-to-regexp/getPathMatch)/:locale(en|sv)?/settings/:path*, and the/:nextInternalLocale(...)/...variants i18n generates) never fired under non-default locales: the redirect locale index swallowed sources whose suffix contained params./x/:locale?and/:locale(en|sv)?/x/:path*required the slash before an absent optional param./foo/(bar|baz)/:id) leaked into params..as a param prefix (/files/:name{.:ext}?,/user/:id.:format),{}groups,(\d+)*repeats and/:path*.mdvs/.mdnow follow path-to-regexp.:auth-methodis theauthparam followed by literal-method, as in path-to-regexp's name lexer./api/*is now an invalid source (see Behaviour change)./docs/:rest(.*)on/docs/a/capturesa/, like Next).The redirect locale index now only indexes sources whose result is identical to the compiled source (literal-only suffix, no trailing slash,
\wparam name, plain-word alternatives).Behaviour change: invalid sources fail config loading
Like Next.js's
checkCustomRoutes, which fails the build,resolveNextConfignow throwsInvalid next.config.js <type> source(s)and lists every offending source. A source is invalid when it:/;/api/*);Previously vinext skipped such sources at runtime, so an app that has one will now fail to start or build until the source is fixed. Sources that are valid but rejected by the conservative ReDoS check below are not load errors, because Next accepts them: they are skipped at runtime, and vinext warns about each one once when the config loads (documented in
docs/reference/differences.mdx).ReDoS validation
Each param's custom pattern first goes through the existing per-pattern structural checks (nested repetition, ambiguous alternatives under repetition). The compiled source (and each middleware matcher) is then checked for exponential backtracking with an exact ambiguity test, which replaces the old whole-source repeated-alternative heuristics. It builds the regex's position (Glushkov) automaton, counting paths per edge like the backtracking engine and modelling the
(?!prefix)guards path-to-regexp adds before repeated params. It then looks for a strongly connected component of the automaton's self-product with both a diagonal and an off-diagonal state, or a parallel edge (Allauzen–Mohri–Rastogi). Repeats are unrolled exactly while the automaton fits a 16,384-position budget; past that, repeats over 1,024 positions are read as loops, which can only add ambiguity. Construction, guard expansion and the product search each have a work budget, and a source over budget is rejected assource exceeds the regex analysis budget.The Pages client's synchronous
beforeFilesrewrite fast path keepsconfig-matchersout of the eager router bundle, so it stays a small matcher, but now only models whole-segment literals,:nameand a final:name*/:name+(ASCII case-insensitive), and defers every other shape to the exact async matcher.Known limitations (unchanged)
a/b) rather than Next's arrays, and a missing optional param substitutes as"".^(?!/_next)lookahead;caseSensitiveRoutesis not supported./blog/:slug([a-z0-9]+(?:-[a-z0-9]+)*)(documented indifferences.mdx). fix(regex-safety): measure polynomial ambiguity exactly instead of guessing from nesting #4069 replaces them with the exact analysis, which accepts it./{aaaa…:id}+with ~96+ identical characters, or a ~1,000-character periodic prefix), and repeats whose exact unrolling exceeds the position budget (/{a:id(a{16000})}+X).beforeFilesrule whose later segments rule out the pathname, such as/foo/:id?/baron/foo/baz, still sends that navigation to the async matcher, as onmain./:a(a|aa)/:id(\\1+), is therefore rejected even though the engine compares one captured string./:a(.*)-:b(.*)); that is handled in a follow-up PR stacked on this one.Tests
tests/config-source-matching.test.ts:getPathMatch+modifyRouteRegex;tests/e2e/pages-router/i18n-locale-case-config.spec.ts: parameterized locale-prefixed redirects on the checked-inpages-i18n-public-rewritefixture, with expectations observed on real Next.js 16.2.7./is replaced by the exact ambiguity analysis:/:p([a-z]*)+and similar now compile, while/:p(.*)+and/:path(.*)*/endare still rejected (tests updated for the new messages). A middleware matcher the check rejects still fails, as before.