fix(middleware): match Pages data request metadata#2239
Conversation
|
/bigbonk review for issues |
commit: |
|
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: matched-path data-request metadata parity
I reviewed the diff against Next.js behavior and ran the focused suites (pages-request-pipeline, app-rsc-handler, middleware-runtime — 165 passing; the dev i18n features test passing). The core of this PR is solid and the security framing is correct.
What's good
- Dropping the spoofable header is the right call and matches Next.js. Removing
hasDataRequestHeader = rawRequest.headers.get("x-nextjs-data") === "1"and relying only on the trustedpagesDataNormalization?.isDataReqclassification mirrors Next.js, which derivesisNextDataRequestfrom URL normalization, not headers. The two new App Router tests (forged-header config redirect, forged-header middleware) lock this in well. __isDataexactly matches Next.js. TheObject.defineProperty(nextRequest, "__isData", { enumerable: false, value: true })inmiddleware-runtime.ts:321is byte-for-byte the Next.js descriptor inpackages/next/src/server/web/adapter.ts, and it's gated on the trustedisDataRequest, which corresponds to Next'sisNextDataRequest. Good parity.isDataRequestvsisMiddlewareDataRequestsplit inapp-rsc-handleris principled — the soft-redirect/__isDataprotocol only fires on the trusted classification, and the new test asserts forged headers no longer leak into middleware.- dev/prod parity for the success path is handled correctly. Because dev omits
renderPagefrompipelineDeps(pipeline emits intents), the pipeline's matched-path logic can't run in dev, so the header is set directly increateSSRHandler(dev-server.ts:1445). The prodmatchPageRoutealready returns the full route object includingpattern, somatchedPathnameForRouteworks in prod too.
Issues
1. (non-blocking) request.__isData is exposed at runtime but not in the NextRequest type. The fixture tests/fixtures/pages-basic/middleware.ts:40 reads request.__isData, and the shim class in packages/vinext/src/shims/next-shims.d.ts:531 has no such member and no index signature. Typechecking the fixture's own tsconfig reports:
tests/fixtures/pages-basic/middleware.ts(40,69): error TS2339: Property '__isData' does not exist on type 'NextRequest'.
This does not fail CI (vp check doesn't typecheck fixtures), but any end user writing the same Next.js-style middleware under strict TS would hit this error. Next.js exposes __isData and users rely on it for testing. Suggest adding readonly __isData?: boolean; to the NextRequest shim class so the runtime feature is reflected in the type.
2. (worth confirming) dev data-miss does not get the middleware soft-200 + matched-path treatment. The new behavior — when middleware ran and status is null/200/404, a _next/data miss returns 200 {} with x-nextjs-matched-path — is implemented in the Pages pipeline (pages-request-pipeline.ts:698) and App Router (app-rsc-handler.ts:889), both gated on deps.renderPage/runMiddleware. In dev, renderPage is omitted so the pipeline branch can't run, and the dev-server's own miss handler (dev-server.ts:545) still returns a plain 404 {} with no x-nextjs-matched-path, even when middleware is present. The added e2e/unit tests cover the rewrite-success and prod miss cases but not a dev data-miss-with-middleware. If Next.js returns the soft-200 here, this is a dev/prod parity gap; please confirm against the middleware-general suite and either replicate it in dev-server.ts or document why dev differs.
Verified, no action needed
matchedPathnameForRoutei18n branches are correct: explicit-locale path →/<locale><pattern>; default-locale path →normalizeDefaultLocalePathnameprepends the default locale, matching Next's always-localizedx-nextjs-matched-path.- Pages miss uses original
pathnameand App Router usescanonicalPathname— both intentionally the original request path, consistent with each other. buildNextDataNotFoundResponse()header copy inapp-rsc-handler.ts:895correctly preserves the deployment-id header before overriding status/body.- The dev
matchPageRoutei18n normalization (index.ts:4091) now matches the prodmatchPageRoute(pages-server-entry.ts:315) — this actually closes a prior dev i18n matching gap rather than introducing drift.
|
/bigbonk review for issues |
|
Review posted to PR #2239. Summary of my findings: Review summaryThe PR is well-constructed and the security framing is correct. I ran the focused suites (165 tests passing) and verified the implementation against Next.js source. Strong points:
Two issues flagged:
I also explicitly verified the i18n locale-prefixing logic, the original-vs-resolved pathname choices, the deployment-id header preservation, and that the dev |
|
/bigbonk review for issues |
Review: Pages data-request metadata parity (follow-up on the prior
|
|
/bigbonk review for issues |
|
Review posted to PR #2239. Summary of findings: Review summaryI re-reviewed against Next.js
I also confirmed the spoofable-header removal, The security model is right; items 1 and 2 are the kind of silent divergence (encoded into passing tests) that AGENTS.md warns about — worth confirming against a real Next.js |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: Pages data-request metadata parity — verified against Next.js canary source
I cloned vercel/next.js@canary and traced the actual emit paths for x-nextjs-matched-path, x-nextjs-redirect, and the data-miss soft-200 protocol, then re-ran the focused suites (165 passing). The security framing (dropping the spoofable x-nextjs-data header, gating on trusted isDataReq, byte-identical __isData descriptor) is correct and well covered.
Three of the prior ask-bonk findings deserve correction/confirmation. I disagree with item 1, confirm item 2, and confirm item 3.
1. (NOT a bug — prior review was wrong) Dynamic data routes correctly emit the route pattern /blog/[slug]
The prior review claimed Next.js emits the interpolated path (/blog/123) and pointed at interpolateDynamicPath (base-server.ts:1386). That is the wrong code path — it's the minimal-mode branch that consumes an upstream x-matched-path header to re-derive params, not the path that emits the x-nextjs-matched-path response header on a normal middleware data request.
The actual emit paths are base-server.ts:2205-2208 (success) and 2733-2736 (miss), both using the local pathname variable. That pathname is set from match.definition.pathname at the render dispatch (base-server.ts:2644-2647):
const result = await this.renderPageComponent(
{ ...ctx, pathname: match.definition.pathname, ... },and RouteDefinition.pathname is documented as "The pathname (including dynamic placeholders)" (route-definitions/route-definition.ts:15-17) — i.e. the bracket pattern /blog/[slug].
So vinext's matchedPathnameForRoute() → patternToNextFormat(route.pattern) and the test assertion /blog/[slug] (tests/pages-request-pipeline.test.ts:277) match Next.js exactly. No change needed. The dev success path (dev-server.ts:1450) is also correct.
2. (confirmed parity gap) Config redirects emit a soft-200; Next.js sends a real 307/308
pages-request-pipeline.ts:299-307 converts next.config.js redirects on trusted data requests into status: 200 + x-nextjs-redirect, locked in by tests/pages-request-pipeline.test.ts:96 (status 200).
In Next.js, config-file redirects resolve in resolve-routes.ts:800-826 and return a real redirect status — statusCode: getRedirectStatus(route) (307/308) with a normal Location — with no _next/data special-casing. x-nextjs-redirect is emitted only by the middleware adapter (web/adapter.ts). The client handles middleware-present data redirects at the HTTP-status level for [301,302,307,308] (shared/lib/router/router.ts:534-538).
Both paths happen to work on the client (the soft-200 x-nextjs-redirect branch is also handled at router.ts:304-326), so this isn't a functional break — but it changes the observable HTTP status (307/308 → 200) for config redirects and bakes that into a passing test. This is exactly the "silent divergence encoded as correct" pattern AGENTS.md warns about. Recommend keeping the soft-redirect protocol scoped to middleware-issued redirects and letting config redirects keep their real 307/308, unless you can point to a Next.js test that soft-redirects config redirects on data requests.
3. (confirmed dev/prod parity gap) i18n data-miss matched-path drops the locale in dev
For a _next/data miss with middleware:
- Prod (
pages-request-pipeline.ts:708):matchResolvedPathname(pathname)→normalizeDefaultLocalePathname, which preserves an explicit non-default locale (/fr/missingstays/fr/missing,normalizeDefaultLocalePathnameline 92) and prefixes the default locale (/missing→/en/missing). - Dev (
dev-server.ts:555): useslocaleStrippedUrl, so/fr/missing→/missing— locale dropped.
Next.js's miss branch (base-server.ts:2733-2736) is `${locale ? '/'+locale : ''}${pathname}` — locale-prefixed, matching prod. Note dev's own success path (dev-server.ts:1450) already does `${locale ? '/'+locale : ''}${patternToNextFormat(route.pattern)}`, so the miss path is inconsistent with both prod and dev-success. Recommend making dev-server.ts:555 locale-prefixed (e.g. `${locale ? '/'+locale : ''}${localeStrippedUrl}`) and adding a dev i18n data-miss test — there's currently no coverage for it (the only dev i18n data test, features.test.ts:1412, is the success path).
Minor (non-blocking): request.__isData missing from the NextRequest shim type
Runtime exposes __isData (middleware-runtime.ts:325) and the fixture reads it (tests/fixtures/pages-basic/middleware.ts:40), but the NextRequest shim class (shims/next-shims.d.ts:531) doesn't declare it. CI doesn't typecheck fixtures so this is green, but strict-TS users writing the same middleware hit TS2339. Add readonly __isData?: boolean;.
Verified, no action needed
- Spoofable
x-nextjs-dataremoval + trustedisDataReqgating matches Next.js (isNextDataRequestis URL-derived). __isDatadescriptor is byte-identical toweb/adapter.ts.- App Router miss (
app-rsc-handler.ts:887-900) uses originalcanonicalPathname, preserves the deployment-id header, and gates on realrunMiddleware+ null/200/404 — matchesbase-server.ts:2726-2729. - Dev
matchPageRoutei18n normalization (index.ts) now mirrors prod, closing a prior gap.
Net: Item 1 is fine as-is (and the prior review was mistaken). Items 2 and 3 are real divergences worth resolving (or deliberately documenting) before merge; both are encoded into passing tests today.
|
Review posted to PR #2239. I verified all claims against a fresh clone of FindingsThe security framing is correct — dropping the spoofable Three parity items, with a correction to the prior reviews:
Plus one minor non-blocking note: Net: item 1 needs no change (prior review was mistaken); items 2 and 3 are real divergences worth resolving or deliberately documenting before merge. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: Pages data-request metadata parity — re-review after ece4cdc
I re-reviewed the full diff against vercel/next.js@canary (freshly cloned) and re-ran the focused suites: 165 passing (pages-request-pipeline, app-rsc-handler, middleware-runtime), plus the Pages Router + features integration suites (654 passing) and vp check clean on all changed source files. The two open parity items from my prior review are now fixed, and the disputed item 1 is confirmed correct.
The three prior findings are resolved in ece4cdc
-
Config redirect soft-200 → real 307/308 (fixed). The
isDataReq || isDataRequestsoft-200 branch was removed from bothpages-request-pipeline.ts(config redirects now fall through to the realredirect.permanent ? 308 : 307) andapp-rsc-handler.ts:556-571. Thex-nextjs-redirectsoft-redirect protocol now lives only inmiddleware-runtime.ts:184-191(dataRedirectResponse), gated on the trustedoptions.isDataRequestat line 409 — which matches Next.js, wherex-nextjs-redirectis emitted exclusively by the middleware adapter (web/adapter.ts) and config redirects keep their real status (resolve-routes.ts). Tests updated accordingly (pages-request-pipeline.test.ts:96,app-rsc-handler.test.ts:601now assert 308 + realLocation). -
Dev i18n data-miss now locale-prefixed (fixed).
dev-server.ts:555changed fromlocaleStrippedUrlto`${locale ? `/${locale}` : ""}${localeStrippedUrl}`, matching Next.jsbase-server.ts:2734(`${locale ? '/'+locale : ''}${pathname}`) and prod'smatchResolvedPathname(pathname). Two new dev tests pin it:/fr/missing→/fr/missing,/missing→/en/missing(features.test.ts:1424,:1438), both passing. -
__isDatashim type (fixed).readonly __isData?: boolean;added to theNextRequestclass innext-shims.d.ts:531; the fixturemiddleware.ts:40read no longer tripsTS2339.
Item 1 (dynamic data route pattern) — confirmed correct as-is, no change needed
I traced the emit path in the freshly cloned canary. The success-path x-nextjs-matched-path is set at base-server.ts:2205-2207 from the local pathname, which is assigned match.definition.pathname at the render dispatch (base-server.ts:2647). RouteDefinition.pathname is documented as "The pathname (including dynamic placeholders)" (route-definitions/route-definition.ts:15-17) — i.e. the bracket pattern. So vinext's matchedPathnameForRoute() → patternToNextFormat(route.pattern) and the /blog/[slug] assertion (pages-request-pipeline.test.ts) match Next.js. The interpolateDynamicPath path (base-server.ts:1386) is the minimal-mode branch that consumes an upstream header, not the emit path. No change.
Security model — verified solid
- Spoofable
x-nextjs-dataheader read is gone;entry-templates.test.ts:1024asserts the generated entry no longer containsrequest.headers.get("x-nextjs-data"). Classification is URL-derived (isDataReq), matching Next.jsisNextDataRequest. __isData(middleware-runtime.ts:321-326) is byte-identical to Next.js'sadapter.tsdescriptor and gated on the trusted classification.- The new e2e (
middleware.spec.ts) proves it end-to-end: a forgedx-nextjs-data: 1on a non-data path returns 200 (no throw), while a real_next/dataURL returns 500 —request.__isDatais trusted, not header-derived. - App Router miss (
app-rsc-handler.ts:881-894) preserves the deployment-id header (buildNextDataNotFoundResponsesets it, copied vianew Headers(response.headers)) and gates on realrunMiddleware+ null/200/404, matchingbase-server.ts:2726-2729. hasMiddlewareis now threaded consistently through all three adapters (devindex.ts:4088, prodprod-server.ts:1526/1793, workerdeploy.ts:690) and gates the Pages soft-miss atpages-request-pipeline.ts:692.
One minor, non-blocking note (dev-only edge case)
The dev miss matched-path can include a query string where prod and Next.js do not. dev-server.ts:555-556 builds the header from localeStrippedUrl, and for _next/data requests the dev adapter preserves the query into the page URL (index.ts:3909-3910 → pagePathname + qs), so resolvePagesI18nRequest(...).url carries it. A miss like /_next/data/<id>/missing.json?foo=bar would emit x-nextjs-matched-path: /en/missing?foo=bar in dev, whereas prod uses the query-free matchResolvedPathname(pathname) and Next.js uses the query-free pathname (base-server.ts:2734). matchRoute strips the query internally (route-matching.ts:50) so routing is unaffected — only the header value differs. Worth a one-line .split("?")[0] on the dev path for exact parity, but it's an edge case with no test coverage either way.
Net
The PR is in good shape: the security framing is correct, the success/miss/redirect plumbing now matches Next.js, dev/prod parity is closed for the locale-prefix case, and the previously-disputed dynamic-route item is confirmed correct. Only the minor dev query-string edge case remains, and it's non-blocking.
Co-authored-by: james-elicx <james-elicx@users.noreply.github.com>
|
Review posted to PR #2239. Summary of my findings: Review summaryI re-reviewed the full diff against a fresh The three prior open findings are now fixed in commit
Item 1 (dynamic route pattern) confirmed correct as-is — verified Security model verified solid — spoofable header read gone, One minor non-blocking note: dev's miss matched-path can include a query string (e.g. Net: the PR is in good shape — security framing correct, plumbing matches Next.js, parity gaps closed. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: Pages data-request metadata parity — re-review
I re-reviewed the full diff against the changed source, ran the focused suites (165 passing: pages-request-pipeline, app-rsc-handler, middleware-runtime), and traced the dev/prod/App-Router parity paths. The core of the PR is in good shape and the parity items raised by prior reviews are resolved. I found one real issue the prior reviews missed.
Issue: two stray node_modules symlinks committed (should be removed)
This PR adds two tracked symlinks that look like accidental git adds from a local dev environment, unrelated to the middleware change:
tests/e2e/app-front-redirect-issue/fixture/node_modules→../../../fixtures/app-basic/node_modulestests/fixtures/pages-basepath-dev/node_modules→../pages-basic/node_modules
Reasons to drop them:
- Unrelated to the PR.
pages-basepath-devisn't referenced by any test added in this PR (no test imports/uses it), andapp-front-redirect-issueis a pre-existing e2e fixture that has nothing to do with Pages data-request metadata. Both symlinks were introduced in commit0ae6371("Reviewed PR #2239: fixes confirmed"). - Targets are gitignored.
node_modules/is in.gitignore; the symlink targets (fixtures/app-basic/node_modules,pages-basic/node_modules) won't exist in a fresh clone or CI checkout, so the symlinks dangle. They only "work" on a machine wherepnpm installhas already populated those dirs. - No precedent. These are the only tracked
node_modulessymlinks in the repo — every other fixture relies on the workspace install, not a committed symlink.
Suggest git rm both and, if a shared node_modules is genuinely needed for a new fixture's dev run, rely on the workspace install / hoisting rather than committing a symlink into gitignored space.
Verified correct — no action needed
- Security framing is right. Dropping the spoofable
x-nextjs-dataheader read (hasDataRequestHeader) and gating exclusively on the trusted URL-derivedisDataReq/isPagesDataRequestclassification matches Next.js (isNextDataRequestis URL-derived).entry-templates.test.tsasserts the generated entry no longer reads the header. The e2e proves it: a forgedx-nextjs-data: 1on a non-data path returns 200, a real_next/dataURL throws → 500. __isDatadescriptor (middleware-runtime.ts:321) is byte-identical to Next.jsweb/adapter.tsand gated on the trusted classification. The shim type now declaresreadonly __isData?: boolean;(next-shims.d.ts:530), closing theTS2339gap from the prior review.- Config-redirect soft-200 was correctly removed from both
pages-request-pipeline.tsandapp-rsc-handler.ts; config redirects now keep their real 307/308, andx-nextjs-redirectlives only in the middleware adapter — matching Next.js. New tests (pages-request-pipeline.test.ts:96, the forged-header test) lock this in. - dev/prod/App-Router miss parity is consistent. Pages prod uses
matchResolvedPathname(pathname), App Router usesmatchPathname(canonicalPathname)— both arenormalizeDefaultLocalePathname, so they agree. Dev now uses`${locale ? `/${locale}` : ""}${localeStrippedUrl}`and the three new dev i18n tests (/fr/missing→/fr/missing,/missing→/en/missing, success →/fr/posts/[id]) pin the locale-prefixed behavior, matching prod. hasMiddlewaregating is equivalent across adapters. Pages gates the soft-miss ondeps.hasMiddleware; App Router gates onoptions.runMiddlewarepresence — and the generated App entry only definesrunMiddlewarewhenmiddlewarePathexists, so the two conditions match. The miss-status gate (null | 200 | 404) matches Next.jsbase-server.ts.- Dynamic-route pattern (
/blog/[slug]) emitted viapatternToNextFormat(route.pattern)is correct as-is —RouteDefinition.pathname(the emit source in Next.js) is the bracket pattern, not the interpolated path. The earlier-review dispute on this is settled.
Net
The PR is functionally sound and parity-correct. The only thing worth fixing before merge is removing the two stray node_modules symlinks — they're unrelated to the change, dangle in fresh checkouts, and set a precedent no other fixture follows. Non-blocking from a behavior standpoint, but they shouldn't land.
|
Review posted to PR #2239. SummaryI re-reviewed the full diff, ran the focused suites (165 passing), and traced dev/prod/App-Router parity. The PR is functionally sound — the security framing is correct and the parity items from prior reviews are resolved. One real issue the prior reviews missed: the PR commits two stray
These were added in commit Verified correct (no action needed): the spoofable-header removal, byte-identical |
1 participant
Summary
_next/dataclassification to middleware without trusting spoofable headersValidation
vp checkand final independent review: cleanNext.js parity
Targets
middleware-general/test/node-runtime.test.tsand related middleware-general assertions from deploy-suite run 27858251903. Investigation used vinext fixtures; no raw Next.js E2E was run locally.