Fix Pages prod middleware named proxy export resolution#188
Conversation
|
/bigbonk review please |
|
Thanks for the PR, yeah this looks like a legit issue. Will work on merging |
There was a problem hiding this comment.
Review: Fix Pages prod middleware named proxy export resolution
This is a clean, well-targeted fix for a real production-only bug. The core issue is confirmed: on main, the Pages Router production entry codegen (index.ts:665) generated middlewareModule.default || middlewareModule.middleware, missing the middlewareModule.proxy fallback that was already present in both middleware.ts:196 (dev runtime) and app-dev-server.ts:1295 (App Router codegen). This means a proxy.ts using export function proxy(...) (named export, no default) would silently be ignored in Pages Router production builds — a dangerous parity gap.
After this fix, all four middleware resolution sites are consistent:
middleware.ts:196— dev runtime:mod.default ?? mod.proxy ?? mod.middleware(already correct on main)app-dev-server.ts:1295— App Router RSC codegen:middlewareModule.default || middlewareModule.proxy || middlewareModule.middleware(already correct on main)index.ts:665— Pages Router prod codegen: now includesmiddlewareModule.proxy(this PR)prod-server.ts— callsrunMiddlewarefrom the generated entry, so inherits the fix
The test coverage is good:
- Integration test that builds a real Pages Router app with named
proxyexport and verifies the redirect fires - Precedence test confirming
default > proxy > middlewareordering - Unit test for the dev-server
runMiddlewarepath with a namedproxymock - Codegen assertion for the App Router generated entry
Two minor notes below, but nothing blocking.
| } | ||
| }); | ||
|
|
||
| it("produces client bundle with page chunks and SSR manifest", async () => { |
There was a problem hiding this comment.
Nit: It might be worth adding a third test case that verifies a proxy.js with only a named middleware export (no default, no proxy) also works through the generated prod entry. That would fully exercise the third fallback position in the chain. Not a blocker though — the current tests already cover the important cases.
| it("supports named proxy export in generated middleware dispatch", () => { | ||
| const code = generateRscEntry("/tmp/test/app", minimalRoutes, "/tmp/proxy.ts", [], null, "", false); | ||
| expect(code).toContain( | ||
| "middlewareModule.default || middlewareModule.proxy || middlewareModule.middleware", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
This test asserts on the generated code string, which is sufficient for catching regressions. Note that the App Router codegen already had proxy support on main — this test is purely additive insurance, which is a good thing.
|
Review submitted. Here's a summary of what I found: Verdict: Approved The PR fixes a confirmed production-only bug where the Pages Router production codegen ( The fix is a single-line change plus doc/warning text updates, with solid test coverage: two integration tests (named proxy execution + export precedence), one unit test, and one codegen assertion. |
commit: |
Thanks! Glad it helped. We actually caught this with a @cubic-dev-ai codebase scans - running 1000s of agents to flag critical issues. It caught a bunch of other stuff too - not sure if you'd want to look into them or if this repo isn't really made for production-use |
|
@southpolesteve could we also get a CVE + bug bounty for this? |
summary
we found a production-only mismatch in how vinext runs
proxy.tsfor Pages Router apps.if an app exported
proxy.tsasexport function proxy(...)(named export), the generated production middleware runner ignored it and acted like no middleware existed. this means requests could continue straight to routes that were expected to be gated by proxy logic.why this is a problem
teams commonly put auth/authz checks in
proxy.ts(redirect unauthenticated users, block private paths, tenant checks, etc).before this fix, those checks could silently not run in Pages production output when using named
proxy. that is teh dangerous part: no crash, just skipped protection.why lowercase
proxywe resolve the export by exact name, and JavaScript export names are case-sensitive. Next.js 16 also documents/uses
proxyin lowercase. soproxymust be matched exactly;ProxyorPROXYare different identifiers.what changed
default || proxy || middlewareproxysupportproxyexecution in generated Pages prod entrydefault > proxy > middleware)