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 / query has semantics match matchHas in prepare-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.ts token output and compiled regex match the vendored path-to-regexp, including the normalizeMiddlewarePathTokens slash-prefix retry.

No blocking issues found. A few observations:

Observations (non-blocking)

1. Two ReDoS analyzers now coexist. unsafeTokenReason runs both the AST-based analyzeRegexSafety (which already reports "overlapping sequential repetition") and the separate hand-rolled string scanner hasOverlappingSequentialRepetition in middleware-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.

2. Config headers/redirects/rewrites still use the old tokenizer. matchConfigPattern in config-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 shared analyzeRegexSafety via safeRegExp. Flagging only so it's a conscious decision — config-route matching still diverges from Next.js's path-to-regexp semantics.

3. isSafeRegex is now strictly stricter for all callers. Since safeRegExp powers config header/redirect/rewrite compilation too, patterns that previously passed the naive nested-quantifier heuristic may now be rejected (returning null). All call sites I checked handle null gracefully (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.

4. matchesMiddleware fail-open shift is correct. Non-array matcher configs and invalid matcher objects now return true (middleware runs for all paths) instead of false. 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: fix(middleware): align unsafe matcher validation

Re-reviewed after the base was updated with new commits since the last /bigbonk pass (b02592b perf(runtime): lazy-load config matchers, 0382586 fix(navigation): preserve rewrite prefetch reuse, and the 82ef911 merge from main). Focused on what changed relative to the earlier review: the module extraction + lazy-import performance work, the isExternalUrl consolidation, and the HAS_CONFIG_* / HAS_CLIENT_REWRITES build-define gating.

Verdict: No blocking issues. The security-relevant matcher/regex-safety logic is unchanged from the previously-approved state; the new commits are correctness-preserving refactors and perf gating.

What I verified this pass

• Tests green: tests/middleware-matcher-auth.test.ts (27 pass, dev/Node/Worker), tests/shims.test.ts (1225 pass, including the byte-identical path-to-regexp parity assertion against next/dist/compiled/path-to-regexp), tests/link.test.ts (123 pass), tests/app-rsc-response-finalizer.test.ts + tests/request-pipeline.test.ts (122 pass). vp check clean on all new source modules.

• isExternalUrl consolidation is semantics-preserving. The new utils/external-url.ts (/^[a-z][a-z0-9+.-]*:/i + startsWith("//")) is equivalent to both prior implementations it replaces: the config-matchers.ts copy (identical regex) and the shims' isAbsoluteOrProtocolRelativeUrl (/^[a-zA-Z][a-zA-Z\d+\-.]*?:/ requires a letter start, scheme chars, then a mandatory : — the ? non-greedy flag is inert because : is required). All call sites in app-rsc-handler.ts, app-middleware.ts, app-server-action-execution.ts, navigation.ts, and router.ts now route through the shared helper.

• finalizeAppRscResponse async conversion is fully awaited. It became async (lazy-imports config-headers.ts). Its only non-test caller (app-rsc-handler.ts:1421) returns the promise from an async callback wrapped in runWithPrerenderWorkUnit, so it's awaited correctly. All test call sites were updated to await.

• HAS_CONFIG_* / HAS_CLIENT_REWRITES gating fails safe. Every define is read as process.env.X !== "false", so the feature defaults to enabled in dev/test where the define is absent. The gates only short-circuit config header/redirect/rewrite work when the app genuinely has none configured, and each is paired with a && options.X.length guard, so behavior is identical to unconditional execution (matchHeaders/matchRedirect over an empty list is a no-op).

• link.tsx lazy hybrid-owner import is null-safe. The module is imported only when HAS_PAGES_ROUTER || HAS_CLIENT_REWRITES, and every hybridRouteOwner! dereference is guarded by the matching individual flag, so the non-null assertions can't fault. The new linkPrefetchNavigationEpoch cancellation correctly no-ops a pointer-intent prefetch that lost the module-load race to a click navigation in the same turn.

• extractMiddlewareMatcherConfigValue build validation is sound. Returning the raw static value (rejecting only UNSUPPORTED_STATIC_VALUE) lets validateMiddlewareMatcherPatterns throw on malformed matcher objects at build time instead of silently dropping them. Arrays containing any non-static element (spread/computed) collapse to UNSUPPORTED_STATIC_VALUE as a whole, so no partial/misleading validation occurs.

Observations (non-blocking, carried forward)

1. Two ReDoS analyzers still coexist — analyzeRegexSafety (AST) and the hand-rolled hasOverlappingSequentialRepetition string scanner in middleware-matcher-pattern.ts:117. Both fail-closed, which is fine as belt-and-suspenders, but it remains a maintenance burden worth consolidating in a follow-up.

2. Config headers/redirects/rewrites still use the legacy tokenizer (matchConfigPattern in config-matchers.ts); only middleware matchers gained path-to-regexp parity. Consistent with the PR scope, and those paths do get the stricter shared analyzeRegexSafety via safeRegExp.

3. Bundle-size delta improved since last pass. The perf refactor (lazy-loading config-matchers.ts off the hot RSC/SSR path) turned the earlier ~+1.8–3.0% regression into net wins on RSC entry closure (-3.9%), server bundle (-2.3%), and client entry (-2.3%); only the client bundle grew (+2.9%). This directly addresses the perf note from the prior review.

Overall: correct, well-tested, faithful to Next.js, and the new commits meaningfully improve the perf profile. The items above are follow-ups, not merge blockers.

Review: fix(middleware): align unsafe matcher validation

Verdict: One blocking issue — a reproducible concurrency crash in the built Node production server that intermittently turns a middleware auth guard into a 500 instead of enforcing it. The matcher/regex-safety logic and Next.js parity are otherwise correct.

Blocking: matchesMiddleware lazy import races module init under concurrency

tests/middleware-matcher-auth.test.ts — the test added by this PR — is flaky. Running the built Node production server case in isolation it fails on ~2 of every 3 runs:

[vinext] Server error: TypeError: Cannot read properties of undefined (reading 'get')

The failing path varies between runs (/archive/2024-07-10, /codes/ABCD, /bracket-shorthand/a1z9, …) because assertAuthGuard fires all 18 protected requests concurrently via Promise.all. Affected requests return 500 instead of 403, i.e. the middleware auth guard silently fails open under load right after server start.

Runs in isolation (each a fresh builder.buildApp() + prod server):

Run 1: 1 failed | 26 passed
Run 2: 27 passed
Run 3: 1 failed | 26 passed
Run 4: 1 failed  (/codes/ABCD -> 500)
Run 5: 1 failed  (/archive/2024-07-10 -> 500)

Root cause — circular-dependency initialization order exposed by the perf lazy-import refactor.

b02592b perf(runtime): lazy-load config matchers changed executeMiddleware in server/middleware-runtime.ts:302 to:

const { matchesMiddleware } = await import("./middleware-matcher.js");

In the production build this puts middleware-matcher in its own chunk that statically imports the entry (import { N as e, W as t } from "../../index.js"), while index.js imports the chunk only dynamically. That is a cycle: index.js --(dynamic)--> matcher chunk --(static)--> index.js.

The chunk's module-scope cache is emitted as a hoisted var:

// middleware-matcher-*.js — decompiled
var C = {…}, w = Symbol("unsafe matcher pattern"), T = new Map;   // T = _mwPatternCache, at byte ~7044/7991
function A(e,n){ … let i = T.get(r); … }                          // matchPattern -> T.get throws
export { E as matchesMiddleware };

Under the first concurrent burst of await import(...), matchPattern (A) executes before the top-level T = new Map assignment runs, so T is still the hoisted undefined and T.get(r) throws. The throw is not caught by executeMiddleware's middleware try/catch (it happens in matchesMiddleware, before middlewareFn runs), so it propagates to the top-level handler and becomes a generic 500. Stack:

at A  (middleware-matcher chunk)   // matchPattern: T.get(r)
at D  (middleware-matcher chunk)   // matchMatcherPattern
at E  (middleware-matcher chunk)   // matchesMiddleware
at ma (index.js)                   // executeMiddleware, right after `await import(...)`

Confirmed fix. Reverting just this one call site to a static import (and dropping the dynamic import) makes the same isolated test pass 3/3:

// server/middleware-runtime.ts
import { type MatcherConfig, matchesMiddleware } from "./middleware-matcher.js";
// …and delete `const { matchesMiddleware } = await import("./middleware-matcher.js");`

matchesMiddleware is on the middleware hot path and its transitive cost (matcher-pattern + regex-safety) is only pulled in when an app actually has middleware, so a static import here does not meaningfully expand the common no-middleware request path — but it does eliminate the entry⇄chunk cycle that causes the race. If lazy loading must be kept for bundle-size reasons, the alternative is to break the cycle so the matcher chunk does not statically import the entry (e.g. move matchesMiddleware + its cache into a leaf module with no back-edge to index.js), and to initialize _mwPatternCache in a way that cannot be observed as undefined (a hoisted function-scoped accessor rather than a top-level var read).

Note: the two prior /bigbonk passes reported this file as "27 pass". It passes single-shot often enough to look green; the failure only shows reliably when the case is run in isolation and/or repeatedly, so CI's serial full-suite run can easily miss it. This is a real dev/prod-parity defect (the dev server case is unaffected because there is no chunked cycle), so it should not merge as-is.

Non-blocking observations (carried forward + verified)

• Query has/missing parity is correct. I re-verified matchSingleCondition's query branch and _matchConditionValue against Next.js matchHas in prepare-destination.ts: omitted/empty value is a presence check requiring a non-empty actual value; duplicate keys are truthy even when the final element is empty; the value regex tests value.slice(-1)[0]. URLSearchParams.getAll yields [""] for ?foo= and ["",""] for ?foo=&foo=, so vinext's queryValues.length > 1 shortcut matches Node+Next.js semantics exactly.
• isExternalUrl consolidation into utils/external-url.ts is semantics-preserving vs both prior copies.
• Two ReDoS analyzers still coexist (analyzeRegexSafety AST + the hand-rolled hasOverlappingSequentialRepetition string scanner). Both fail-closed; worth consolidating in a follow-up.
• Config headers/redirects/rewrites still use the legacy tokenizer; only middleware matchers gained path-to-regexp parity. Consistent with PR scope.

tests/shims.test.ts (1225 pass, including the byte-identical path-to-regexp parity assertion) is green. The blocker above is the only thing preventing approval.

Review: fix(middleware): align unsafe matcher validation

Verdict: No blocking issues. The concurrency crash flagged in the previous /bigbonk pass is fixed.

Blocker from last pass is resolved

The prior review's blocker — the built Node production server auth test intermittently returning 500 instead of 403 because executeMiddleware lazy-import()ed middleware-matcher.js, creating an entry⇄chunk cycle whose top-level _mwPatternCache = new Map() could be observed as undefined under the first concurrent burst — has been addressed by commit 31f9049 fix(middleware): avoid matcher chunk initialization race. It reverts the single dynamic import back to a static one:

// server/middleware-runtime.ts
import { matchesMiddleware, type MatcherConfig } from "./middleware-matcher.js";
// (removed: const { matchesMiddleware } = await import("./middleware-matcher.js");)

This is exactly the fix the prior review recommended: it breaks the cycle so the matcher cache can no longer be read before initialization, and since matchesMiddleware's transitive cost is only pulled in when an app actually has middleware, it does not meaningfully bloat the common no-middleware path.

Verified: I ran tests/middleware-matcher-auth.test.ts in isolation 4/4 times (each a fresh builder.buildApp() + prod server) — all 27 pass every run, including the concurrent 18-request assertAuthGuard burst that previously failed ~2/3 of the time. The race is gone.

What else I verified this pass

• vp check clean — format, lint, type-checks across all 1136 files.
• Tests green: tests/shims.test.ts (incl. the byte-identical path-to-regexp parity assertion vs next/dist/compiled/path-to-regexp), tests/link.test.ts, tests/request-pipeline.test.ts, tests/app-rsc-response-finalizer.test.ts (1470 total), plus tests/build-optimization.test.ts (173).
• isExternalUrl consolidation is semantics-preserving. I empirically confirmed utils/external-url.ts (/^[a-z][a-z0-9+.-]*:/i + // prefix) is byte-for-byte equivalent to the isAbsoluteOrProtocolRelativeUrl it replaces across a 32-case matrix (schemes, protocol-relative, javascript:, data:, invalid leaders, etc.). The *? non-greedy in the old regex is inert because : is required.
• config-headers.ts extraction is complete. Every caller of applyConfigHeadersToResponse/applyConfigHeadersToHeaderRecord (app-rsc-handler, pages-request-pipeline, finalizer, and the test) now imports from the new module; the deleted request-pipeline copies left no dangling references.
• finalizeAppRscResponse async conversion is fully awaited at its sole non-test caller (app-rsc-handler.ts:1421), returned from the async runWithPrerenderWorkUnit callback.
• HAS_CONFIG_* / HAS_CLIENT_REWRITES gating fails safe — each define is read !== "false" (defaults enabled in dev/test), paired with a length guard, so gated behavior is identical to unconditional execution.
• link.tsx lazy hybrid-owner import is null-safe — the module is loaded only under HAS_PAGES_ROUTER || HAS_CLIENT_REWRITES, and every ! deref is guarded by the matching individual flag. The new linkPrefetchNavigationEpoch correctly no-ops a pointer-intent prefetch that lost the module-load race to a click nav in the same turn.
• Query has/missing parity with Next.js matchHas (prepare-destination.ts) is unchanged and correct: omitted/empty value is a presence check requiring a non-empty actual value; duplicate keys are truthy even when the final element is empty; the value regex tests the last element.
• Build-time matcher validation is sound — extractMiddlewareMatcherConfigValue returns the raw static value so validateMiddlewareMatcherPatterns throws on malformed matcher objects at build time; non-static values collapse to unsupported and skip validation cleanly.
• ReDoS fail-closed coverage in middleware-matcher-redos-child.ts is thorough (near-miss backtracking, overlapping sequential repetition, ambiguous alternatives, bounded-sequence quadratics, and a linear-time large-alternation assertion).

Non-blocking observations (carried forward)

1. Two ReDoS analyzers still coexist — analyzeRegexSafety (AST) and the hand-rolled hasOverlappingSequentialRepetition string scanner in middleware-matcher-pattern.ts:117. Both fail-closed; worth consolidating in a follow-up.
2. Config headers/redirects/rewrites still use the legacy tokenizer (matchConfigPattern in config-matchers.ts); only middleware matchers gained path-to-regexp parity. Consistent with PR scope, and those paths still get the stricter shared analyzeRegexSafety via safeRegExp.
3. Perf refactor is a net win on bundle size (RSC entry closure -3.9%, server -2.3%, client entry -2.3%; only the client bundle grew +2.9%), addressing the earlier size-regression note.

The blocker is fixed and I could not reproduce any regression. This is ready to merge.