fix(middleware): align matcher validation#2599
Conversation
commit: |
|
Performance benchmarksCompared 0 improved · 4 regressed · 2 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(middleware): align matcher validation
Reviewed all 30 files. This is a solid, security-focused refactor that replaces two hand-rolled ReDoS heuristics with a deterministic structural analyzer plus a faithful port of path-to-regexp 6.3, and validates matchers at config time. Test coverage is excellent — the new shims.test.ts case compiles every fixture pattern with next/dist/compiled/path-to-regexp and asserts vinext produces the identical RegExp.source/flags, and the auth fixture exercises dev, Node prod, and Workers. I ran tests/shims.test.ts (1214 pass) and tests/middleware-matcher-auth.test.ts (27 pass), plus vp check on the changed source files (clean).
Verified the key parity claims against Next.js canary (.nextjs-ref):
_matchConditionValue/ queryhassemantics matchmatchHasinprepare-destination.ts: empty/omitted value is a presence check requiring a non-empty actual value, duplicate query keys select the last element (value.slice(-1)[0]), and a duplicate key is truthy even when its final value is empty.middleware-path-to-regexp.tstoken output and compiled regex match the vendored path-to-regexp, including thenormalizeMiddlewarePathTokensslash-prefix retry.
No blocking issues found. A few observations:
Observations (non-blocking)
-
Two ReDoS analyzers now coexist.
unsafeTokenReasonruns both the AST-basedanalyzeRegexSafety(which already reports"overlapping sequential repetition") and the separate hand-rolled string scannerhasOverlappingSequentialRepetitioninmiddleware-matcher-pattern.ts:117. Both leaning toward false-positives (fail-closed) is fine as belt-and-suspenders, but it's a maintenance burden — two code paths that must stay in agreement. Worth a follow-up to consolidate on the AST analyzer if it can cover the string-scanner cases, or a comment explaining why both are retained. -
Config
headers/redirects/rewritesstill use the old tokenizer.matchConfigPatterninconfig-matchers.ts:724+retains the legacy hand-rolled tokenizer +extractConstraint, so only middleware matchers gained the path-to-regexp-parity implementation. This is consistent with the PR's scope ("middleware matcher validation"), and those config paths do benefit from the stricter sharedanalyzeRegexSafetyviasafeRegExp. Flagging only so it's a conscious decision — config-route matching still diverges from Next.js's path-to-regexp semantics. -
isSafeRegexis now strictly stricter for all callers. SincesafeRegExppowers config header/redirect/rewrite compilation too, patterns that previously passed the naive nested-quantifier heuristic may now be rejected (returningnull). All call sites I checked handlenullgracefully (fall back to linear scan / non-match / warn), and the rejected patterns are genuinely backtracking-prone, so the direction is safe. Callers relying on previously-accepted pathological patterns would see a behavior change, but that's the intended hardening. -
matchesMiddlewarefail-open shift is correct. Non-array matcher configs and invalid matcher objects now returntrue(middleware runs for all paths) instead offalse. This is the security-correct direction (don't silently bypass an auth guard) and Next.js validates these at build time anyway, so runtime only sees them as defense-in-depth.
Performance note
The benchmark comment reports ~+1.8–3.0% growth across server/client/RSC bundle sizes, attributable to the new 899-line regex-safety.ts + 312-line pattern module + 281-line path-to-regexp. Given AGENTS.md's guidance to keep the common request path lightweight, consider whether regex-safety.ts (only needed at matcher-compile time, which is cached per pattern) can be lazily imported off the hot request path rather than statically pulled into the shared RSC/SSR/worker closures. Not blocking, but the size regression is measurable.
Overall: correct, well-tested, and faithful to Next.js. Approving direction; the items above are follow-ups rather than fixes required for merge.
|
Review posted to PR #2599. SummaryI reviewed all 30 files in PR #2599 ( Verdict: No blocking issues. The PR is correct, well-tested, and faithful to Next.js. What I verified:
Non-blocking observations I raised:
|
Summary
Testing
vp checkvp run vinext#build