fix(middleware): classify Pages data requests by URL - #2039
Conversation
commit: |
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(middleware): classify Pages data requests by URL
I reviewed the diff, traced the dev / Node-prod / Worker request paths, and ran the targeted suites. Verified locally:
vp test run tests/pages-data-route.test.ts tests/pages-request-pipeline.test.ts— 71 passedvp test run tests/entry-templates.test.ts tests/deploy.test.ts— 303 passedvp test run tests/pages-router.test.ts -t "does not treat a normal URL...|adds x-nextjs-rewrite for a real data URL"— 4 passedvp checkon all 7 changed source files — clean
The core change is sound: classifying _next/data requests by trusted URL normalization instead of the forgeable inbound x-nextjs-data: 1 header. The fix is applied consistently across all three Pages Router entry points (index.ts dev, prod-server.ts, deploy.ts worker), and the reproduction (header-only request producing a spurious soft-redirect) is now covered by ported Next.js tests in both the dev and prod-server integration suites.
I confirmed the basePath-stripping order is consistent in both the Worker (deploy.ts:642-658) and Node prod (prod-server.ts:1672-1704): basePath is stripped before data normalization, so the bare-page normalization is correct and hadBasePath is threaded into wrapMiddlewareWithBasePath as before. The double-normalization in pages-page-handler.ts:303 is a no-op on an already-normalized request (URL is now /page, so isNextDataPathname is false), and asPath resolves to the page path either way — no regression.
Non-blocking observations
1. The new basePath parameter on normalizePagesDataRequest is never exercised by production code.
All three real callers strip basePath upstream and call the helper with the default basePath = "":
- Worker
normalizeDataRequest(request)→__normalizePagesDataRequest(request, buildId)(entries/pages-server-entry.ts:236-238) pages-page-handler.ts:303→normalizePagesDataRequest(request, buildId)prod-server.ts/index.tsdon't use the helper at all (inlineparseNextDataPathname)
So the new branches (pages-data-route.ts:204-205, 226-228) are only covered by the unit tests added in tests/pages-data-route.test.ts, not by any integration path. This is harmless but is dead capability in practice — consider either wiring a caller through it or dropping the parameter to avoid an untested code path drifting out of sync.
2. App Router still trusts the x-nextjs-data header for the soft-redirect protocol (app-rsc-handler.ts:1047). This is the same class of issue this PR fixes for Pages Router. It's explicitly out of scope per the PR description (wave2 candidate 2, Pages only), but worth tracking as a follow-up so the App Router path doesn't retain the forgeable classification.
3. The ordering guard in tests/deploy.test.ts is a source-text indexOf assertion. It's brittle by nature, but I confirmed both anchor strings are unique in index.ts, so it reliably enforces the "delegate before mutating _next/data URLs" invariant. Fine as-is.
Overall this looks correct and well-tested for its stated scope. Nothing blocking.
|
Review posted to PR #2039. SummaryI reviewed PR #2039 ( What I verified:
Non-blocking findings I flagged:
Nothing blocking. |
vinext parity: Pages middleware data requests
Date: 2026-06-15
Worktree:
/Users/jamesanderson/.codex/worktrees/wave2-middleware-data-requests/vinextBranch:
codex/wave2-middleware-data-requestsBase:
origin/mainata3d2f921Scope: wave2 candidate 2 only; cacheComponents/PPR/resume excluded.
Next.js reference
Ported exact behavior from:
test/e2e/middleware-general/test/index.test.tsshould not treat as _next/data request with just headershould add a rewrite header on data requests for rewritestest/e2e/middleware-general/app/middleware.js/ssr-pagerewrite to/ssr-page-2Reproduction on origin/main
A disposable detached worktree at
a3d2f921ran the header-only redirect assertion against the existing Pages middleware fixture.The inbound
x-nextjs-data: 1header incorrectly opted an ordinary URL into the data soft-redirect protocol.Final implementation
/_next/data/...jsonURL parsing.normalizeDataRequest()helper and normalizes beforerunPagesRequest, preserving config redirect/header ordering and stale-build-ID JSON 404 behavior./_next/dataURLs, so the Worker receives the original trusted URL.x-nextjs-rewriteonly for trusted real data rewrites.x-middleware-rewriteand forgedx-nextjs-datavalues are not exposed or trusted.Independent review
Multiple independent
codex review --uncommittedpasses identified and drove fixes for:Final architecture moves Worker data normalization before the shared request pipeline, resolving all four findings.
Exact validations
Green final gate:
Also completed successfully before finalization:
The package build completed with its existing unresolved virtual/private import warnings treated as externals.