fix(app-router): reject middleware control responses in route handlers#939
Conversation
App Route handlers currently treat NextResponse.next() and NextResponse.rewrite() as ordinary 200 responses. That diverges from Next.js, where those helpers are middleware control-flow signals and are rejected after a route handler returns. The route-handler execution path now validates returned responses before cache policy or response finalization runs. Focused unit and integration tests cover the invalid next and rewrite helper responses.
commit: |
There was a problem hiding this comment.
Pull request overview
Aligns Vinext App Route handler behavior with Next.js by rejecting middleware control-flow NextResponse outputs (next()/rewrite()) when they are returned from route handlers, preventing invalid responses from being cached or finalized as successful handler output.
Changes:
- Add a post-handler validation (
assertSupportedAppRouteHandlerResponse) to reject responses containingx-middleware-rewriteorx-middleware-next: 1. - Invoke the validation immediately after
runAppRouteHandler()returns inexecuteAppRouteHandler(). - Add unit + integration coverage and fixture routes that return invalid
NextResponse.next()/NextResponse.rewrite().
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/fixtures/app-basic/app/api/invalid-next-response-rewrite/route.ts | Adds a fixture route that incorrectly returns NextResponse.rewrite() from an App Route handler. |
| tests/fixtures/app-basic/app/api/invalid-next-response-next/route.ts | Adds a fixture route that incorrectly returns NextResponse.next() from an App Route handler. |
| tests/app-router.test.ts | Integration test asserting both invalid routes return empty-body 500. |
| tests/app-route-handler-execution.test.ts | Unit test asserting rejection path reports the correct error and avoids cache writes. |
| packages/vinext/src/server/app-route-handler-response.ts | Introduces assertSupportedAppRouteHandlerResponse() and the associated error messages. |
| packages/vinext/src/server/app-route-handler-execution.ts | Calls the new assertion immediately after the user handler returns. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good change — this is a real parity gap and the fix is well-placed. The validation lives in the right layer (response helpers, not generated entries), the error messages match Next.js, and test coverage is solid at both unit and integration levels. A couple of observations below.
ISR background regeneration path: app-route-handler-cache.ts:94 calls runAppRouteHandler() directly (not executeAppRouteHandler()) during background ISR regeneration, so the new assertion doesn't run there. If a route handler that returns NextResponse.next() somehow gets into the ISR regen path, the invalid response would be cached silently. In practice this is unlikely — a handler returning NextResponse.next() would fail on the initial request before ever being cached — but it's worth noting for completeness. Not blocking.
| const NEVER_CACHE_CONTROL = "private, no-cache, no-store, max-age=0, must-revalidate"; | ||
|
|
||
| const APP_ROUTE_REWRITE_ERROR = | ||
| "NextResponse.rewrite() was used in an app route handler, this is not currently supported. Please remove the invocation to continue."; |
There was a problem hiding this comment.
Nit: Next.js uses "in a app route handler" (grammatically wrong but that's their exact string). Vinext changed it to "in an app route handler". This is fine as a deliberate improvement — just noting it's a conscious divergence from upstream, not a copy error. If anyone later diffs against Next.js error strings for parity auditing, this will show up.
Either way is defensible. If you want exact parity for grep-ability against the Next.js source:
| "NextResponse.rewrite() was used in an app route handler, this is not currently supported. Please remove the invocation to continue."; | |
| "NextResponse.rewrite() was used in a app route handler, this is not currently supported. Please remove the invocation to continue."; |
But I'm also fine keeping the grammar fix — your call.
There was a problem hiding this comment.
Restored exact upstream wording in 19ef0e2b so the error strings remain grep-able against the Next.js source. The grammar is awkward, but this path is intentionally matching the compatibility oracle.
| expect(response.status).toBe(500); | ||
| await expect(response.text()).resolves.toBe(""); | ||
| expect(reportedErrors.map((error) => error.message)).toEqual([testCase.message]); | ||
| expect(wroteCache).toBe(false); |
There was a problem hiding this comment.
Good coverage. The test verifies all the important properties: 500 status, empty body, error reported, no cache write, and request context cleanup. The finally block around errorSpy.mockRestore() is also correct.
One minor suggestion: consider adding a test case for the boundary condition where x-middleware-next has a value other than "1" (e.g., "0" or "true") to confirm those are not rejected. This would document the exact semantics and guard against future over-broad matching. Not blocking.
There was a problem hiding this comment.
Added the boundary coverage in 19ef0e2b. assertSupportedAppRouteHandlerResponse() now has a focused unit test showing x-middleware-next: "0" and "true" are allowed, while "1" is rejected.
|
Review submitted successfully. Here's a summary of the review: Overall: This is a well-executed fix for a real Next.js parity gap. The approach is sound — validation in the response helper layer, invoked right after the handler returns but before cache/cookie/response merging. Key observations posted:
|
Stale App Route handler regeneration called runAppRouteHandler directly and could serialize a middleware control response into the ISR cache. That bypassed the validation used by the live route-handler execution path. Run the same response assertion before background regeneration writes a new APP_ROUTE cache entry, restore exact Next.js error wording for parity, and cover the x-middleware-next boundary semantics.
|
Addressed the ISR background regeneration concern in |
|
/bigbonk review |
There was a problem hiding this comment.
Clean, well-scoped fix. The validation is placed in the right layer (response helpers), the header matching semantics are consistent with how vinext checks these headers everywhere else (has() for rewrite, === "1" for next), and the error messages match upstream Next.js. The ISR background regen path is covered — the assertion at app-route-handler-cache.ts:106 throws into __triggerBackgroundRegeneration's .catch(), which logs and prevents the invalid response from being cached while the stale response has already been returned to the client.
Test coverage is thorough across all three levels: unit (response helper boundary conditions), execution (full executeAppRouteHandler flow verifying 500 status, empty body, error reporting, no cache write, and context cleanup), ISR regen (stale served, invalid regen rejected and not cached), and integration (dev server fixture routes).
No issues found.
|
Review submitted. The PR is approved — it's a clean fix with correct placement, consistent semantics, and thorough test coverage across all relevant code paths. |
What this changes
App Route handlers now reject
NextResponse.next()andNextResponse.rewrite()responses instead of treating their internal middleware headers as ordinary successful route-handler output.Why
These helpers are middleware control-flow responses. Next.js rejects them after an App Route handler returns:
NextResponse.rewrite()App Route rejection in Next.jsNextResponse.next()App Route rejection in Next.jsNextResponse.next()returning 500Vinext previously accepted those responses and returned
200, which silently diverged from Next.js and could cache or finalize an invalid route-handler response.Approach
The validation lives in the typed route-handler response helper and is invoked from
executeAppRouteHandler()immediately after the user handler returns. That keeps the generated App RSC entry unchanged and prevents invalid middleware-control responses from reaching cache policy, cookie finalization, or response merging.The check matches Next.js semantics:
x-middleware-rewritex-middleware-next: 1500responseValidation
vp test run tests/app-route-handler-execution.test.ts tests/app-router.test.ts -t "rejects middleware control responses"failed before the implementation with200responsesvp fmtvp test run tests/app-route-handler-execution.test.ts tests/app-router.test.tsvp checkRisks / follow-ups
No generated App RSC entry code was added. This intentionally preserves middleware use of
NextResponse.next()andNextResponse.rewrite()while rejecting only App Route handler returns.