fix(middleware): fail closed for unsafe matchers

[james-elicx]

Summary

• fail closed when a middleware matcher is rejected by the ReDoS safety check
• distinguish unsafe matcher rejection from malformed regex syntax
• warn once that rejected matchers cause middleware to run for all paths
• add regression coverage for direct, cached, array, object, and runtime middleware execution paths

Why

A matcher rejected as potentially unsafe previously fell back to a literal comparison. For regex-style matcher sources, that comparison could never match the request pathname, so middleware could be skipped entirely.

Rejected unsafe matchers now conservatively execute middleware for every path. Malformed regex syntax retains the existing literal-match fallback, avoiding an unrelated global middleware expansion.

Testing

• vp test run tests/shims.test.ts
• vp check packages/vinext/src/server/middleware-matcher.ts packages/vinext/src/config/config-matchers.ts tests/shims.test.ts
• git diff --check

[james-elicx]

/bigbonk review for issues

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2078

npm i https://pkg.pr.new/vinext@2078

commit: c805aa6

[github-actions]

Example	Preview	Production	Original
app-router-cloudflare	preview	production
pages-router-cloudflare	preview	production
app-router-playground	preview	production	original
realworld-api-rest	preview	production
nextra-docs-template	preview	production
benchmarks	preview	production
hackernews	preview	production	original
workers-cache	preview	production
web	preview	production

[ask-bonk]

Review: fail closed for unsafe middleware matchers

Reviewed the change end-to-end (source, tests, behavior parity, dev/prod paths). This is a solid, well-targeted security fix. Verified locally: vp check clean on both source files, and all 21 middleware matcher tests plus the new middleware runner test pass.

What works well

• Correct fix for the real bug. Previously a rejected unsafe matcher fell through safeRegExp → null → literal pathname === pattern comparison. For regex-style sources that literal comparison can never match the request path, so middleware was silently skipped — bypassing request guards. Routing the rejection through the new UNSAFE_MATCHER_PATTERN sentinel so matchPattern returns true (run middleware for all paths) is the right fail-closed behavior.
• Good separation of the two failure modes. Distinguishing unsafe regex (isSafeRegex false → fail closed) from malformed syntax (safeRegExp returns null → literal fallback) is the key insight, and it's covered directly by the does not run globally for malformed regex syntax test (matchPattern("/public", "/admin(") === false). This avoids an unintended global middleware expansion for typo'd patterns.
• Scope is correctly limited to middleware. Config-level safeRegExp consumers (headers/redirects/rewrites in config-matchers.ts) intentionally keep returning null (fail-open / no-match). The only config-matchers.ts change is the cosmetic warning wording ("Ignoring" → "Rejecting"), which no test depends on. Good call not to change config fail-open semantics in this PR.
• Test coverage matches the PR description. Direct (matchPattern), cached (warn-once assertion), array, object-source, and full runtime (runMiddleware returning the 403) paths are all exercised. The cache-rejection path correctly verifies warnSpy is called exactly once.
• Array/object matcher semantics are sound. An unsafe string matcher short-circuits to true; an unsafe object source matches the path but still honors has/missing gating. That's a reasonable fail-closed-but-still-conditional behavior.

Minor (non-blocking) observations

• Redundant isSafeRegex evaluation. compileMatcherRegExp calls isSafeRegex(regexPattern) and then, on the safe path, calls safeRegExp(regexPattern) which runs isSafeRegex again internally. Results are cached per pattern in _mwPatternCache, so this only costs one extra scan on first compile — harmless, but could be tidied if you want to avoid the double pass.
• Warning wording divergence. The middleware warning says "Rejecting … middleware matcher" and the config warning says "Rejecting … regex pattern". That's intentional and reads fine; just flagging that the two messages now share the "Rejecting" verb but describe different fallback behavior (run-for-all vs. ignore).

Verification

• vp check packages/vinext/src/server/middleware-matcher.ts packages/vinext/src/config/config-matchers.ts → clean (format + lint + types)
• vp test run tests/shims.test.ts -t "middleware matcher" → 21 passed
• vp test run tests/shims.test.ts -t "unsafe" → 5 passed

LGTM. No blocking issues.

[ask-bonk]

Review posted to PR #2078.

Summary

I reviewed PR #2078 (fix(middleware): fail closed for unsafe matchers) thoroughly — examining the source changes, tests, dev/prod parity, and behavior against the cited Next.js references.

Verdict: LGTM, no blocking issues.

Key findings

The fix correctly addresses a real security gap:

• Before: unsafe (ReDoS) matchers fell through safeRegExp → null → literal pathname === pattern. For regex-style sources that literal comparison never matches, so middleware was silently skipped, bypassing request guards.
• After: a new UNSAFE_MATCHER_PATTERN sentinel makes matchPattern return true (run middleware for all paths) — proper fail-closed behavior.

What I verified:

• The two failure modes are cleanly distinguished: unsafe regex → fail closed; malformed syntax → literal fallback (no unintended global expansion).
• Scope correctly limited to middleware; config headers/redirects/rewrites keep their fail-open semantics.
• The runtime path (middleware-runtime.ts → matchesMiddleware) inherits the fix, so dev/prod parity holds.
• Test coverage matches the PR description (direct, cached/warn-once, array, object-source, full runMiddleware).
• vp check clean; 21 middleware-matcher tests + 5 "unsafe" tests pass.

Only two non-blocking notes: a redundant isSafeRegex evaluation (cached, harmless) and the cosmetic warning-wording change. No tests depend on the old "Ignoring" wording.

github run