fix(app-router): isolate query-dependent ISR HTML#2584
Conversation
|
/bigbonk review for issues |
commit: |
|
Performance benchmarksCompared 0 improved · 1 regressed · 5 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: fix(app-router): isolate query-dependent ISR HTML
Reviewed the diff against the base branch codex/security-cdn-late-dynamic. This is a well-constructed security fix for ISR query-string poisoning. The design is sound, the dev/prod/edge parity is covered, and the targeted test suites pass locally (app-page-render, app-page-cache, app-router-production-server, check). A few observations below — none are blocking.
What the fix does (verified correct)
- Cross-environment access reporting. SSR runs in a separate Vite module graph, so the fix threads a request-local
onSearchParamsAccess()callback throughNavigationContextand reports the result back to the RSC-owned cache lifecycle via asearchParamsAccessedpromise. The promise resolves incleanup, which is wired todeferUntilStreamConsumed— so it only settles after the HTML stream drains. Both consumers (app-page-cache-finalizer.tsand the CDN-verify path inapp-page-render.ts) read the stream to completion before awaiting the promise, so there is no hang and the access flag is accurate (alluseSearchParams()calls have run by flush time). Good. - No hang on the CDN non-
completepath.htmlRender.searchParamsAccessedis only awaited insideif (verification.complete)(app-page-render.ts:1122-1128). When the verifier bails on deadline/maxBytes it hands off the stream and never awaits the promise, so a partially-drained stream can't deadlock. Confirmed againstverifyCdnCacheCandidateStream(complete: trueonly on readerdone). - force-static / dynamic-error modes.
onSearchParamsAccessreturns early for force-static (keeps HTML cacheable, sincepageSearchParamsis already emptied at app-page-dispatch.ts:578-581) and throws the static-generation error fordynamic="error". Correct given vinext's existing model. - Hydration restore.
restoreHydrationNavigationContext(rsc.nav.pathname, window.location.search, params)uses the URL bar as authoritative for this visitor's query while keeping the server-rendered pathname.new URLSearchParams(window.location.search)handles the leading?correctly, and this mirrors the pre-existing call at app-browser-entry.ts:1413. EmptyingnavPayload.searchParamsto[]in the injected bootstrap is consistent sinceapplyRuntimeRscBootstrapno longer reads it.
Non-blocking observations
1. dynamic="error" + client useSearchParams → 500 is a deliberate divergence from Next.js. In Next.js, useSearchParams() in a Client Component during prerender is not a server dynamic API — it triggers a client-side-render bailout to the nearest <Suspense> boundary (see the useSearchParams docs), and does not throw under dynamic="error". This PR makes it throw a 500 (tests/app-router-production-server.test.ts asserts status 500). That's a reasonable defense-in-depth choice for this security fix, but per AGENTS.md ("Fixing Bugs") a divergence from Next.js should be explicitly documented. Consider a code comment noting that vinext intentionally treats client useSearchParams access under dynamic="error" as a hard error rather than a CSR bailout, so a future reader doesn't "fix" it back to Next.js parity and reopen the poisoning hole.
2. New fixtures rely on vinext not enforcing the Suspense requirement. The isr-client-search-force-static and isr-client-search-poison fixtures call useSearchParams() in a client component with no <Suspense> boundary. Real Next.js fails the build here ("Missing Suspense boundary with useSearchParams"), and the CSR-bailout mechanism it documents works by serving the Suspense fallback in static HTML — so there is never a text mismatch. vinext instead renders "none" server-side and swaps to the URL value on the client, which is a React #418 hydration text mismatch. The deleted rsc-context-lazy-stream.test.ts cases documented exactly this #418 concern for the old bootstrap-embed approach. This is a pre-existing vinext characteristic (it doesn't enforce the Suspense rule), not a regression from this PR, and the security behavior is correct either way. But the E2E tests only assert the post-hydration text — they don't guard against console hydration errors. If you want to lock in "no hard hydration error," consider wrapping the fixture client components in <Suspense> (matching how Next.js docs recommend authoring these) so the tests exercise the same shape real apps use.
3. Minor: dead searchParams: [] in the bootstrap payload. buildHeadInjectionHtml now always emits searchParams: [] as [string, string][] and nothing reads it anymore (the browser entry restores from window.location). Harmless, and keeping the field avoids a wider type change, but it's now pure dead data — a short comment or eventual removal would reduce confusion.
Tests
Ran locally on the PR head (92bc746):
vp test run tests/app-page-render.test.ts tests/app-page-cache.test.ts→ 93 passedvp test run tests/app-router-production-server.test.ts→ 71 passedvp checkon all 8 changed source files → clean (format, lint, types)
Overall this is a solid, well-tested fix. The observations above are about documenting the intentional Next.js divergence and hardening the hydration-error surface, not correctness of the security fix itself.
|
Review posted to PR #2584. SummaryI reviewed the diff of #2584 against its base branch The fix is sound and well-tested. It correctly:
Non-blocking observations I flagged:
Tests run locally on head |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): isolate query-dependent ISR HTML
I reviewed the diff against the base branch codex/security-cdn-late-dynamic, traced the RSC/SSR boundary flow, and ran the targeted suites. The core poisoning fix (private query-dependent HTML + hydration restore from window.location.search) is sound. But there is a blocking test failure that contradicts one of the PR's stated goals.
🔴 Blocking: the dynamic = "error" test fails deterministically
tests/app-router-production-server.test.ts:1659 ("rejects client useSearchParams under dynamic = 'error' without poisoning later requests") fails on the PR head (87fc4d8):
× rejects client useSearchParams under dynamic = 'error' without poisoning later requests
AssertionError: expected 200 to be 500
- 500
+ 200
1664| expect(errorRes.status).toBe(500);
Reproduced multiple ways:
- Full file:
vp test run tests/app-router-production-server.test.ts→ 1 failed | 70 passed - Isolated:
vp test run tests/app-router-production-server.test.ts -t "dynamic = 'error'"→ fails on every run
The PR body claims it "preserve[s] force-static and dynamic-error behavior" and the prior review reported "71 passed" for this file — neither is accurate. CI (Vitest) will fail.
Root cause. The dynamic="error" path throws from onSearchParamsAccess (app-ssr-entry.ts:405):
if (incomingNavigationContext.searchParamsAccessMode === "error") {
throw new Error(getAppPageStaticGenerationErrorMessage());
}But onSearchParamsAccess is invoked from the SSR-environment client component (useSearchParams() in navigation.ts:1657), and the fixture — correctly, per the Next.js Suspense requirement — wraps that client component in <Suspense> (tests/fixtures/app-basic/app/isr-client-search-dynamic-error/[slug]/page.tsx). During streaming SSR, React catches the thrown error at the enclosing Suspense boundary, streams the fallback, and the request completes with 200, not 500. The synchronous throw never propagates to handleSsr's catch (app-ssr-entry.ts:776) or to renderAppPageHtmlStreamWithRecovery.
In Next.js, dynamic="error" is enforced during the RSC prerender (server dynamic-API access throws and propagates as a render/build error), not from a client component's SSR pass inside a Suspense boundary. Throwing from the SSR client render can't reliably reject — it just gets absorbed by Suspense.
What to do (pick one):
- Detect the
"error"access mode where it can actually reject the render (RSC/prerender side, or by surfacing the flag to the lifecycle and returning a 500 non-cacheable response) rather thanthrow-ing inside the SSR client render behind Suspense. - If a hard 500 for this case isn't feasible, treat
"error"mode the same as"dynamic"for cacheability (setdidAccessSearchParams = true, keep the HTML private, return 200) and update the test + PR description to reflect that — the security property (no shared HTML) is what matters, and this matches Next.js's CSR-bailout-not-throw behavior more closely.
Either way, the failing test must be green. Do not merge with a red required check.
⚠️ Order-dependent / shared-server test smell
The three new integration tests share the single production server (per the file's existing ISR caching notes). The dynamic-error test's result is independent of ordering here (it fails everywhere), but note that -t "ISR" and -t "search" name filters silently exclude it, which is how "71 passed" was mis-reported. When validating, run the file with no -t filter.
✅ What's correct (verified)
- Cross-environment reporting & no hang.
searchParamsAccessedresolves incleanupviadeferUntilStreamConsumed'sflush. Both consumers drain the stream before awaiting: the finalizer readsstreamForCacheto completion (app-page-cache-finalizer.ts:172) before awaiting at :173; the CDN path only awaits insideif (verification.complete)(app-page-render.ts:1122-1125), andcomplete: trueis set only when the verifier reads todone(app-page-cdn-verification.ts:192-193), which drains the deferred stream and fires cleanup. No deadlock on the CDN bail path. - Poison + force-static. Both integration tests pass. Force-static correctly returns early in
onSearchParamsAccessand stays cacheable; poison correctly keeps per-query HTML private and un-shared. - Hydration restore.
restoreHydrationNavigationContext(rsc.nav.pathname, window.location.search, params)(app-browser-entry.ts:1424) uses the URL bar as authoritative for the visitor's query while keeping the server pathname. EmptyingnavPayload.searchParamsto[](app-ssr-entry.ts:319) is consistent since the browser entry no longer reads it.
Minor
- Fixtures already use
<Suspense>. Contrary to the earlier review's observation #2, all new fixtures (force-static,poison,dynamic-error,query-isr) wrapuseSearchParams()in<Suspense>, matching the Next.js-required shape. That's good — but it's also precisely why thedynamic="error"throw gets swallowed (see blocking issue). - Dead
searchParams: [] as [string, string][]inbuildHeadInjectionHtml(app-ssr-entry.ts:319) is now unread. Harmless; a short comment or removal would reduce confusion.
Test commands run
vp test run tests/app-router-production-server.test.ts→ 1 failed | 70 passedvp test run tests/app-cdn-cache-production.test.ts→ 8 passedvp test run tests/app-page-render.test.ts -t "CDN-managed HTML private"→ 1 passed (note: this unit test stubssearchParamsAccessed: Promise.resolve(true), so it does not exercise the real throw-behind-Suspense path that the integration test exposes)
|
Review posted to PR #2584 as Changes Requested. SummaryI found a blocking issue that the previous review missed: the PR's own test for a stated goal fails deterministically. 🔴 Blocking —
Verified correct: the cross-environment Corrections to the prior review: its claim that fixtures lack I also flagged the order-dependent shared-server test smell and the now-dead |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): isolate query-dependent ISR HTML
Reviewed at head 01acea3 (the PR was substantially reworked since the two prior ask-bonk reviews, which were against 92bc746/87fc4d8). I re-traced the RSC/SSR boundary flow, the client hydration path, and the stream/promise ordering, and ran the targeted suites. The previous blocking issue is resolved and the design is now more correct than the version that throw a 500 for dynamic="error". Approving.
The previous blocker is fixed — and the approach changed for the better
The earlier reviews correctly flagged that throwing a static-generation error from an SSR client render behind <Suspense> gets swallowed by React (200, not 500), so the old dynamic="error" test failed deterministically. This PR abandons that approach entirely. There is no longer an "error" access mode. The access-mode enum is now "dynamic" | "bailout" | "force-static" (navigation-context-state.ts:26), and dynamic="error" cacheable routes now resolve to "bailout" (app-page-dispatch.ts:578-585).
useSearchParams()in bailout mode throwsBailoutToCSRError(navigation.ts:1661-1665), which React catches at the enclosing Suspense boundary and streams the fallback — matching Next.js's documented CSR-bailout behavior rather than diverging from it. The prior AGENTS.md "document the divergence" concern is moot because there is no longer a divergence.prodOnRecoverableErrornow suppresses the expected bailout so it isn't reported as a hydration failure (app-browser-error.ts:85-91), mirroring Next.js.- Server-side
dynamic="error"is still enforced where it can reliably reject: on the RSC prerender when the page-levelsearchParamsprop is read (verified bytests/app-page-dispatch.test.ts:1459, which now assertsdispatchAppPage(...)rejects directly).
I ran the previously-failing file: tests/app-router-production-server.test.ts → 73 passed (0 failed). The dynamic="error" test now correctly asserts a cached 200 with the Suspense fallback, not a 500.
Verified correct
- Cross-environment reporting & no deadlock.
searchParamsAccessedresolves incleanupviadeferUntilStreamConsumed(app-ssr-entry.ts:439, 763-776). The finalizer readsstreamForCacheto completion at app-page-cache-finalizer.ts:172 (which, via thetee()at :154, drains the deferred SSR stream and fires cleanup) before awaiting the promise at :173. The CDN path only awaits insideif (verification.complete)(app-page-render.ts:1127), andcompleteis set only when the verifier reads todone. No hang on either the finalizer or the CDN bail path. - Hydration parity (the subtle part). This is the crux of avoiding React #418.
applyHydrationNavigationSnapshot(app-browser-entry.ts:1387-1405) defers towindow.location.searchonly for"dynamic"mode, but uses the neutral server-rendered params for"bailout"/"force-static". BecauseuseSyncExternalStore's server snapshot (getServerSearchParamsSnapshot) reads the hydration-restored context, the client's initial render matches the SSR output (none/fallback), thengetSearchParamsSnapshotswitches to the live URL after commit (navigation.ts:1339-1360). This turns a would-be hydration mismatch into a post-hydration update. The E2E suite locks this in — every new case assertshydrationErrors).toEqual([])(client-cache.spec.ts, search-params-static.spec.ts). - Rewrite query semantics.
clientSearchParams(the pre-rewrite browser-visible query) is now threaded fromapp-rsc-handler.ts:613through dispatch to the SSR context. Server components'searchParamsprop sees the rewrite destination query while clientuseSearchParams()sees the visible URL — matching Next.js, and covered by therewrite-query-visiblefixture + E2E (server seeshidden=secret, client sees onlyshown=yes, no console errors). getInsertedHTMLguard.useServerInsertedHTMLcallbacks run outside the component request scope, so readinguseSearchParams()there is invalid. The temporary"bailout"mode +finallyrestore (app-ssr-entry.ts:716-727), plus theisBailoutToCSRErrorre-throw inrenderInsertedHtml(app-ssr-entry.ts:241-243), correctly surface this as a 500 (verified by theinserted-errorfixtures in both the unit and E2E suites).- Suspense requirement honored. All new fixtures wrap
useSearchParams()in<Suspense>(e.g.isr-client-search-poison/[slug]/page.tsx), matching the Next.js authoring rule — which is precisely what makes the bailout cacheable and mismatch-free.
Tests run locally (head 01acea3)
tests/app-router-production-server.test.ts→ 73 passedtests/app-page-render.test.ts tests/app-page-cache.test.ts tests/app-page-response.test.ts→ 125 passedtests/app-cdn-cache-production.test.ts tests/app-browser-entry.test.ts→ 236 passedtests/app-page-dispatch.test.ts→ 63 passedtests/nextjs-compat/rsc-context-lazy-stream.test.ts tests/nextjs-compat/use-client-page-pathname.test.ts→ 29 passedvp checkon all 13 changed source files → clean (format, lint, types)
Minor / non-blocking
!options.scriptNoncedisables bailout (app-page-dispatch.ts:584), so a CSP-nonce request on an otherwise-cacheable route falls back to"dynamic"(private, uncached) instead of the CSR-bailout-and-cache path. This is a safe conservative choice, but it's a silent cacheability difference for nonce'd deployments that isn't called out in a comment — a one-line note on why nonce forces the private path would help a future reader.- Dead
searchParams: []payload — now carries an explanatory comment (app-ssr-entry.ts:318-321) addressing the earlier review; fine to leave as-is. - Order-dependent shared-server integration tests remain (as noted in the file's own header). Not introduced by this PR, but worth running the file with no
-tfilter when validating, since name filters can silently exclude the query cases.
Overall: a solid, well-tested security fix that now aligns with Next.js semantics instead of diverging from them. No blocking issues.
|
Review posted to PR #2584 as Approve. SummaryI reviewed at the current head The blocker is fixed by changing the approach, not patching the test. The old design threw a static-generation error from an SSR client render behind Verified correct:
Non-blocking: Also ran: page render/cache/response (125), CDN + browser entry (236), dispatch (63), nextjs-compat hydration (29), and |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): isolate query-dependent ISR HTML
Reviewed at head dc12d30. The PR gained one new commit (dc12d30 "preserve static search shell during ISR") since the prior ask-bonk approval, which was against 01acea3. I focused this pass on that delta, re-traced the ISR regeneration path, and re-ran the targeted suites. The new commit fixes a real gap in the previously-approved version and is correct. Continuing to approve.
What the new commit fixes (verified correct)
Before dc12d30, the ISR background-regeneration render (runAppPageRevalidationContext) set the navigation context without a searchParamsAccessMode (app-page-dispatch.ts:519-523, old). With the mode unset, a Client Component useSearchParams() during regeneration skipped the bailout branch (navigation.ts:1661) and instead fired onSearchParamsAccess() (navigation.ts:1673) — a different code path from the initial render, which uses "bailout". The regenerated artifact could therefore diverge in shape from the initial static shell.
The commit sets searchParamsAccessMode: dynamicConfig === "force-static" ? "force-static" : "bailout" (app-page-dispatch.ts:523) on the regeneration context. Now regeneration throws BailoutToCSRError → React streams the Suspense fallback → the regenerated ISR artifact is the same clean static shell as the initial render. This is exactly what the strengthened E2E test now asserts: the full MISS → STALE → HIT lifecycle all serves the loading shell and never leaks FIRST/SECOND/THIRD_QUERY into shared HTML, while each visitor's client still shows their own live query (client-cache.spec.ts:520-554, dynamic-preload.spec.ts:79-113).
"bailout"default is safe here.renderFreshPageForCacheonly runs for cacheable routes (ISR / prerender);force-dynamicroutes are never cached and never reach this path. Regeneration always uses emptysearchParams(app-page-dispatch.ts:521, 733), so"bailout"produces a clean shell — consistent with the main dispatch path, which also resolves ISR routes to"bailout"(app-page-dispatch.ts:999-1003). No divergence between first render and regeneration.revalidate: 60 → 1on the dynamic-error fixtures is a test-timing change so the STALE→regen→HIT cycle is observable within the E2E window. Sound.
Worker fixture nonce scoping (verified no regression)
The commit narrows the fixture worker's CSP nonce from every request to only /dynamic-preload (fixture/worker/index.mjs:9-18). This is necessary and correct: scriptNonce intentionally forces the private/uncached path (app-page-dispatch.ts:585, app-page-render.ts:1179), so a global nonce would have made the new query-dynamic-error route uncacheable and defeated the HIT assertions.
I checked for collateral damage:
- The existing nonce test still targets
/dynamic-preloadexactly (dynamic-preload.spec.ts:40-69), which is the one pathusesNoncestill covers. ✅ - The other CSP-nonce E2E (
hydration.spec.ts:65) runs against a different server (port 4176 =examples/app-router-cloudflare, nonce gated by?csp-nonce=1in its ownmiddleware.ts), not this fixture (port 4192). Unaffected. ✅
Re-verified from the prior approval (still holds)
- Cross-environment
searchParamsAccessedpromise: no deadlock on either the finalizer or CDN-verify path (both drain the stream before awaiting). dynamic="error"server-side enforcement still happens on the RSC prerender where it can reliably reject.- Hydration parity: bailout/force-static hydrate against neutral server params, then switch to the live URL post-commit — a post-hydration update, not a #418 mismatch. Every new E2E case asserts
hydrationErrors === [].
Tests run locally (head dc12d30)
tests/app-router-production-server.test.ts→ 73 passedtests/app-page-dispatch.test.ts tests/app-page-cache.test.ts→ 100 passedtests/app-page-render.test.ts tests/app-page-response.test.ts tests/app-browser-entry.test.ts→ 313 passedtests/app-cdn-cache-production.test.ts→ 11 passedvp check packages/vinext/src/server/app-page-dispatch.ts→ clean (format, lint, types)
Minor / non-blocking (carried from prior reviews, still applicable)
!options.scriptNoncesilently disables the cache/bailout path (app-page-dispatch.ts:585, app-page-render.ts:1179). This is now visibly load-bearing — the fixture worker had to stop sending a global nonce for the new tests to cache. Safe and conservative, but a one-line comment near the!options.scriptNoncecondition explaining "nonce'd requests take the private/uncached path" would help a future reader connect it to the fixture's nonce scoping.- Order-dependent shared-server integration tests remain; run the file with no
-tfilter when validating (name filters have silently excluded query cases before). - Dead
searchParams: []payload — already carries an explanatory comment; fine to leave.
Overall: the new commit closes a genuine regeneration-shape gap in the previously-approved fix, is well-covered by a strengthened full-lifecycle E2E, and introduces no fixture regressions. No blocking issues.
|
Review posted to PR #2584 as Approve. SummaryThe PR gained a new commit ( The new commit fixes a real gap and is correct. Before it, the ISR background-regeneration render ( No fixture regression from the nonce scoping change. Narrowing the fixture worker's CSP nonce to only Verified Tests run locally on head Non-blocking notes carried forward: the now load-bearing |
1 participant
Summary
Validation
Dependency
Stacked on #2582 for bounded edge-cache eligibility verification.