Fix middleware next data calls #60785
Conversation
valkum
requested review from
timneutkens,
ijjk,
shuding,
huozhi,
feedthejim,
ztanner and
wyattjoh
as code owners
Stats from current PRDefault BuildGeneral
Client Bundles (main, webpack)
Legacy Client Bundles (polyfills)
Client Pages
Client Build Manifests
Rendered Page Sizes
Edge SSR bundle Size
Middleware size
Next Runtimes
Diff detailsDiff for page.jsDiff too large to display Diff for edge-ssr.jsDiff too large to display Diff for server.runtime.prod.jsDiff too large to display |
Failing test suitesCommit: e5411e1
Expand output● skip-trailing-slash-redirect › should handle external rewrite correctly /chained-rewrite-ssg ● skip-trailing-slash-redirect › should handle external rewrite correctly /chained-rewrite-static ● skip-trailing-slash-redirect › should handle external rewrite correctly /chained-rewrite-ssr ● skip-trailing-slash-redirect › should handle external rewrite correctly /docs/first ● skip-trailing-slash-redirect › should handle external rewrite correctly /docs-ssr/first ● skip-trailing-slash-redirect › should allow rewriting invalid buildId correctly Read more about building and testing Next.js in contributing.md. |
| process.env.NEXT_RUNTIME !== 'edge' && | ||
| req.headers['x-middleware-invoke'] | ||
| ) { | ||
| return false |
There was a problem hiding this comment.
Removing this breaks the functionality that allows middleware to rewrite _next/data paths for a different project/deployment.
Instead of changing this, we should ensure we 404 properly after middleware is invoked, we didn't rewrite/redirect, and the buildId doesn't match at that point.
There was a problem hiding this comment.
First, I am sorry about the failing test cases. I tried to get pnpm build and pnpm test-start working in 3 different envs (macOS (with volta installed), the repo's devcontainer, and a ubuntu VM) with pnpm build complaining about missing dependencies after pnpm i).
Instead of changing this, we should ensure we 404 properly after middleware is invoked, we didn't rewrite/redirect, and the buildId doesn't match at that point.
But the case of a successful rewrite is the culprit here. next.js needs to properly 404 after middleware is invoked, the buildId is different and a rewrite happened.
Clicking Link after a deployment with a different buildId will do the following:
Request to _next/data/....json is made with:
'x-middleware-prefetch': '1'
'x-nextjs-data': '1'
Middleware will rewrite that and will add
'x-invoke-path': '',
'x-invoke-query': '',
'x-invoke-output': '',
'x-middleware-invoke': '1'
before normalizeAndAttachMetadata and thus handleNextDataRequest is called.
Based on these headers the issue is in the branch where handleCatchallRenderRequest is called.
I am not sure if this is the only branch that is affected, though, as I am not very familiar with "middleware to rewrite _next/data paths for a different project/deployment."
What about letting normalizeAndAttachMetadata return [complete, buildIdMismatch], and before handleCatchallRenderRequest check if buildIdMismatch is true and render 404 in that case before executing handleCatchallRenderRequest?
I can provide a reproducible case if you want.
valkum
force-pushed
the
fix-middleware-nextData-calls
branch
from
e5411e1
to
1b6e0f4
Compare
### What? Currently, when a middleware is active and the path results in a 404, the server responds with a 200 status code to data requests. We propose a change to ensure that a 404 status code is returned in these situations, as expected, solving production issues for the navigation router. ### Why? The client data requests (identified with `_next/data` path & `x-nextjs-data` header) get answered the status code 200. The code below is executed when there is a version skew for the data requests (e.g prefetch in production). https://github.com/vercel/next.js/blob/4125069840ca98981f0e7796f55265af04f3e903/packages/next/src/server/lib/router-server.ts#L217-L227 The version skew consists in the client side version identified with the buildId in `__NEXT_DATA__ ` tag to be obsolete compared to the server version (`BUILD_ID` file). In the case of prefetching, this leads to the code above being executed, therefore the `prefetch-reducer.ts` handles the response as valid and sets it in its cache. Which ultimately triggers a navigation with empty prop, resulting in erroneous behaviours reported in issues and in our production websites: https://github.com/vercel/next.js/blob/4125069840ca98981f0e7796f55265af04f3e903/packages/next/src/client/components/router-reducer/reducers/prefetch-reducer.ts#L54-L74 By switching the response to a 404, we trigger this code in the fetch (`fetchServerResponse`). This change prompts a hard navigation (mpaNavigation), effectively refreshing the client version and resynching it with the server version. https://github.com/vercel/next.js/blob/4125069840ca98981f0e7796f55265af04f3e903/packages/next/src/client/components/router-reducer/fetch-server-response.ts#L125-L134 ### How? We simply update the status code to 404 here: https://github.com/vercel/next.js/blob/4125069840ca98981f0e7796f55265af04f3e903/packages/next/src/server/lib/router-server.ts#L223 Closes: #60785 Closes: #59295 Fixes: #47516 --------- Co-authored-by: JJ Kasper <jj@jjsweb.site>
Fixes Bug
When the middleware is involved in rewriting URLs, the nextData handler will not return 404 when the
buildIdchanged. This breaks the Link component which will soft-navigate with empty props.This is broken since 13.4.13 where the handler logic was refactored. There was a fix applied which did not account for involved middlewares.