fix: throw on missing middleware/proxy exports instead of failing open#203
fix: throw on missing middleware/proxy exports instead of failing open#203southpolesteve merged 4 commits intomainfrom
Conversation
|
/bigbonk review this |
Middleware and proxy files that don't export the expected function (e.g. misspelled export, wrong named export) were silently skipped, letting requests through unprotected. This matches Next.js behavior which throws a ProxyMissingExportError in the same scenario. Changes: - proxy.ts files now require 'proxy' or 'default' export - middleware.ts files now require 'middleware' or 'default' export - All 3 code paths updated (Pages dev, Pages prod, App Router) - Added resolveMiddlewareHandler() with file-type-aware validation - Ported test cases from Next.js proxy-missing-export test suite - Updated AGENTS.md: added required step to search Next.js tests Ported from Next.js: test/e2e/app-dir/proxy-missing-export/ https://github.com/vercel/next.js/blob/canary/test/e2e/app-dir/proxy-missing-export/proxy-missing-export.test.ts
0fcc661 to
50e54eb
Compare
There was a problem hiding this comment.
Review: fix/middleware-export-validation
Summary
Good security fix. The core logic in resolveMiddlewareHandler() is correct, the error messages are clear, and the tests are well-structured. The AGENTS.md addition is a valuable process improvement.
However, there is one dev/prod parity gap that should be addressed before merge.
Issue: Missing validation in server/dev-server.ts (Pages Router dev)
The AGENTS.md explicitly says:
When fixing a bug in any of these files, check whether the same bug exists in the others. Do not leave known bugs as "follow-ups" — fix them in the same PR.
The four request-handling files that must stay in sync:
| File | Updated in this PR? |
|---|---|
server/middleware.ts (shared runner for dev) |
✅ Yes — resolveMiddlewareHandler() added |
server/app-dev-server.ts (App Router dev) |
✅ Yes — inline validation in generated code |
index.ts (Pages Router prod entry) |
✅ Yes — inline validation in generated code |
server/dev-server.ts (Pages Router dev) |
❌ No |
server/prod-server.ts (Pages Router prod server) |
N/A — delegates to generated entry |
cloudflare/worker-entry.ts |
N/A — doesn't exist |
server/dev-server.ts doesn't call resolveMiddlewareHandler() or do any export validation itself. However, looking at the code flow: the Pages Router dev server calls runMiddleware() from middleware.ts (line 236), which now calls resolveMiddlewareHandler() internally. So Pages Router dev is covered via the shared runMiddleware() function.
Wait — re-reading dev-server.ts more carefully, I see that createSSRHandler doesn't call runMiddleware itself. The middleware is run by the Vite plugin's configureServer hook in index.ts, which calls runMiddleware() from middleware.ts directly. So this path is covered. My mistake — the parity is actually fine for the dev paths because they all flow through the shared runMiddleware().
Let me verify: In index.ts's configureServer, middleware is executed via runMiddleware(server, middlewarePath, ...) which calls resolveMiddlewareHandler(). For App Router dev, the generated RSC entry has its own inline validation. For Pages Router prod, the generated server entry has its own inline validation. All three are covered. ✅
Code review of the core changes
server/middleware.ts — isProxyFile() and resolveMiddlewareHandler()
Clean implementation. Two minor observations:
-
isProxyFileusespath.basename(filePath).replace(/\.\w+$/, "")which correctly strips extensions like.ts,.js,.mjs. Good. -
resolveMiddlewareHandleruses??(nullish coalescing) to prefer named export over default:const handler = isProxy ? (mod.proxy ?? mod.default) : (mod.middleware ?? mod.default);
This is correct —
??only falls through onnull/undefined, not on other falsy values. If someone exportsproxy = null, it'll fall through to default, which is the right behavior (null isn't a function).
index.ts (Pages Router prod) and app-dev-server.ts (App Router dev)
Both use || instead of ?? in the generated code:
var middlewareFn = isProxy
? (middlewareModule.proxy || middlewareModule.default)
: (middlewareModule.middleware || middlewareModule.default);This is a minor inconsistency with the shared resolveMiddlewareHandler() which uses ??. In practice the difference only matters if someone exports proxy = 0 or proxy = "" (which would be caught by the typeof handler !== "function" check anyway), so it's not a real bug. But for consistency, consider using ?? in the generated code too — or better yet, import and call resolveMiddlewareHandler directly from the generated entry instead of duplicating the logic.
Tests: The 14 new tests in shims.test.ts are thorough. They cover:
isProxyFilefor both proxy and middleware paths ✅- Named export, default export, and preference order ✅
- Wrong named export (middleware in proxy.ts, proxy in middleware.ts) ✅
- Aliased wrong name ✅
- No exports ✅
- Non-function export ✅
One minor gap: there's no test for resolveMiddlewareHandler when proxy is exported as a non-function in a middleware.ts file (only tested for proxy.ts). But this is covered by the "throws when only 'proxy' is exported" test — since proxy isn't checked at all for middleware.ts files, the behavior is identical regardless of whether it's a function or not.
Deploy workflow change
The deploy-examples.yml change (adding github.event.pull_request.head.repo.full_name == github.repository guards) is unrelated to the middleware fix but makes sense — it prevents fork PRs from failing on missing Cloudflare secrets. This should ideally be a separate commit/PR for clean history, but it's fine to include here.
Verdict
The fix is correct and well-tested. The security hole is properly closed in all code paths. The AGENTS.md addition will help prevent similar issues.
Approve — no blocking issues found.
commit: |
|
I've posted a detailed review on the PR. Here's the summary: Verdict: Approve — no blocking issues found. Key findings:
|
|
…or consistency Matches the shared resolveMiddlewareHandler() which uses nullish coalescing. No behavioral difference since typeof check catches all cases, but keeps the three code paths consistent.
Initial audit documentation for systematic Next.js test suite coverage check. Documents the methodology and initial findings from Priority 1 (error handling) tests. Most gaps from Priority 1 are already covered by PR cloudflare#203.
Summary
Middleware and proxy files that don't export the expected function (e.g. misspelled export, wrong named export, no exports at all) were silently skipped, letting requests through unprotected. This is a security-relevant bug: if someone typos
export function prooxy()in their auth middleware, requests bypass it entirely with no warning.Next.js throws a
ProxyMissingExportErrorin this scenario. This PR matches that behavior.Changes
server/middleware.ts: AddedisProxyFile()andresolveMiddlewareHandler()that validates exports based on file type, then throws if no valid export is foundindex.ts(Pages Router prod): Updated generated code to check correct exports per file type and throw on mismatch (also fixes the missingproxyexport check from Fix Pages prod middleware named proxy export resolution #188)server/app-dev-server.ts(App Router): Same validation in the generated RSC entrytests/shims.test.ts: 14 new tests covering all valid/invalid export combinations for both proxy.ts and middleware.ts filesAGENTS.md: Added "Searching the Next.js Test Suite" as a required workflow step to prevent this class of bug in the futureValidation rules (matching Next.js)
proxy.tsproxy,defaultmiddleware, any other name, no exportmiddleware.tsmiddleware,defaultproxy, any other name, no exportPorted from Next.js
Tests ported from
test/e2e/app-dir/proxy-missing-export/proxy-missing-export.test.ts.