fix(pages-router): pass revalidateReason "on-demand" to gsp/gssp (#1462)#1856
Conversation
#1462) Implements res.revalidate() for Pages Router API routes and wires the on-demand revalidation trigger through to getStaticProps/getServerSideProps context as revalidateReason: "on-demand". res.revalidate(urlPath) issues an internal HEAD request to the target path carrying the x-prerender-revalidate header (mirroring Next.js's api-resolver). The Pages render path (dev-server and prod pages-page-data) detects that header, bypasses the fresh/stale cache-hit short-circuits, and regenerates the entry synchronously with reason "on-demand". The build and stale reasons already worked; this finishes the on-demand case. Closes #1462
|
/bigbonk review for issues |
commit: |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: on-demand revalidateReason wiring (#1462)
The core wiring is clean and the dev/prod (page-handler + page-data) paths are kept in sync, which is good. I have one critical security issue and a couple of smaller parity notes.
🔴 Critical: x-prerender-revalidate is treated as an unauthenticated presence check
In Next.js, x-prerender-revalidate is not a boolean flag — it carries the server's secret previewModeId, and the render path only treats a request as on-demand revalidation when the header value equals that secret:
// api-utils/index.ts — checkIsOnDemandRevalidate
const previewModeId = headers[PRERENDER_REVALIDATE_HEADER]
const isOnDemandRevalidate = previewModeId === previewProps.previewModeId// api-resolver.ts — revalidate()
const headers = { [PRERENDER_REVALIDATE_HEADER]: context.previewModeId, ... }This PR instead sends the literal "1" and gates purely on presence:
api-handler.ts:338/pages-node-compat.ts:328—headers: { [PRERENDER_REVALIDATE_HEADER]: "1" }dev-server.ts:830—(req.headers[PRERENDER_REVALIDATE_HEADER] ?? undefined) !== undefinedpages-page-handler.ts:464—request.headers.has(PRERENDER_REVALIDATE_HEADER)
Because the value is never validated against a secret, any external client can send x-prerender-revalidate: 1 to any ISR page and force synchronous regeneration, bypassing the fresh/stale cache short-circuits. That is a cache-stampede / DoS vector: an attacker can repeatedly defeat ISR caching and pin every request to a full getStaticProps re-render. Next.js's secret-matching is precisely what prevents this.
This should match Next.js: derive/generate a previewModeId secret, have res.revalidate() send it, and have the render path compare the incoming header against it (equivalent to checkIsOnDemandRevalidate). A bare presence check is not safe to ship.
🔶 res.revalidate() success detection diverges from Next.js
api-handler.ts:341 / pages-node-compat.ts treat success as revalidateRes.ok || status === 404. Next.js's helper checks the cache header first, because a successful revalidate can return a non-200:
const cacheHeader = res.headers.get('x-vercel-cache') || res.headers.get('x-nextjs-cache')
if (cacheHeader?.toUpperCase() !== 'REVALIDATED' && res.status !== 200 && !(404 && onlyGenerated)) {
throw new Error(`Invalid response ${res.status}`)
}Two practical gaps:
- A
notFound: trueregeneration returns 404 — Next.js only treats that as success whenunstable_onlyGeneratedis set; this PR always swallows 404. unstable_onlyGenerated(the documented second arg tores.revalidate(urlPath, opts)) is not supported at all. The signature here isrevalidate(urlPath)only.
🔶 Host/protocol fallback for the internal fetch
The internal HEAD request resolves the target via resolveRequestHost(req, "localhost") with no port. When trustProxy is off (the default), resolveRequestHost returns the raw Host header (which normally includes the port, so that's usually fine), but if Host is absent it falls back to bare "localhost" with no port — the loopback fetch would then hit the wrong port and fail. Worth confirming the Host header is always present in the prod-server / Workers entry paths, or threading the real listening address through instead of relying on the header.
Minor
dev-server.ts:830:(req.headers[X] ?? undefined) !== undefinedis justreq.headers[X] !== undefined— the?? undefinedis a no-op.- Test only covers the dev SSR HTML path. Consider also asserting the
__NEXT_DATA__/ data-request path (isDataReq) and a no-op case (a normal request still returns"stale", not"on-demand") to lock in that presence-vs-secret behavior once the secret check lands.
Verification done
Confirmed Next.js behavior against current canary:
packages/next/src/server/api-utils/node/api-resolver.ts(revalidate()sendspreviewModeId)packages/next/src/server/api-utils/index.ts(checkIsOnDemandRevalidatecompares againstpreviewModeId)
The "stale"/"build"/"on-demand" resolution in pages-page-data.ts:651 correctly mirrors render.tsx. The only blocking concern is the missing secret validation.
|
Review posted to PR #1856. Summary of findings: Critical
Notable
Minor
The |
…1462) The on-demand `revalidateReason` detection treated `x-prerender-revalidate` as an unauthenticated presence check and sent the literal value "1". Any external client could send `x-prerender-revalidate: 1` to force synchronous regeneration of any ISR page, bypassing the fresh/stale cache short-circuits — a cache-stampede/DoS vector. Mirror Next.js's `checkIsOnDemandRevalidate`: a request is only treated as on-demand revalidation when the `x-prerender-revalidate` header value EQUALS a secret (the vinext analog of `previewModeId`). Introduce a per-process revalidate secret (`getRevalidateSecret`, 256-bit random hex via Web Crypto) that `res.revalidate()` attaches to its internal loopback request, and a constant-time `isOnDemandRevalidateRequest` equality check on the receiving dev-server and Pages page handler. Header presence alone is never honored. Also fix the test's HTML-reason extraction to avoid CodeQL's incomplete-sanitization / bad-HTML-regex findings, and add a negative test asserting forged `x-prerender-revalidate` values ("1", "", a guess) are rejected while the real secret is accepted.
|
/bigbonk review for issues |
Re-review: on-demand
|
|
Review posted to PR #1856. SummaryThe previously-blocking critical security issue (unauthenticated Remaining (non-blocking) findings:
No new bugs introduced; the core wiring is correct and well-tested. |
…_onlyGenerated (#1462) Address non-blocking bonk review findings: - Extract the two near-identical `res.revalidate()` implementations (`api-handler.ts`, `pages-node-compat.ts`) into a shared `performOnDemandRevalidate` helper so the secret wiring and success detection cannot drift between dev and prod. - Align success detection with Next.js's api-resolver: accept `x-nextjs-cache: REVALIDATED`, status 200, or a 404 when the caller passed `unstable_onlyGenerated`. Add `unstable_onlyGenerated` support (`x-prerender-revalidate-if-generated` header).
|
/bigbonk review for issues |
Re-review: on-demand
|
|
Review posted to PR #1856. SummaryReviewed at HEAD Resolved since last review:
Security gate (carried, still correct) — on-demand revalidation is authorized by constant-time equality against a per-process secret, never header presence. The negative test verifies forged headers are rejected. Minor non-blocking notes:
Verification: |
|
/bigbonk re-review |
There was a problem hiding this comment.
Re-review: on-demand revalidateReason wiring (#1462)
Reviewed at HEAD 9448a6d. The only change since the prior round (0c50788) is the documentation commit 9448a6d, which directly addresses the last remaining minor note. No new issues; the PR is complete and ready.
Resolved since last review
- Dead
REVALIDATEDbranch now documented —performOnDemandRevalidate(pages-revalidate.ts:65-69) now carries a note explaining that vinext's Pages ISR path only emits HIT/MISS/STALE onx-nextjs-cache(never REVALIDATED), so the happy path is the 200 branch, and the REVALIDATED check is retained for Next.js parity / future-proofing. This was the exact one-line note I asked for — a future reader won't assume vinext emits that header.
Security gate (carried — still correct)
- On-demand revalidation is authorized by equality against the per-process secret via constant-time
safeEqual(isr-cache.ts:99-119), never header presence.isOnDemandRevalidateRequestalso rejects arrays (duplicate headers) and empty/absent values outright before comparison (isr-cache.ts:117-119). The negative test (rejects a forged x-prerender-revalidate header) runs first against a primed"stale"entry, so a regression to presence gating would flip the rendered reason to"on-demand"and fail. - Both
res.revalidate()impls (api-handler.ts,pages-node-compat.ts) delegate to the singleperformOnDemandRevalidatehelper, so the secret wiring and success detection can't drift between dev and prod.
Remaining non-blocking follow-ups (unchanged, pre-existing)
- Per-isolate secret on Workers —
getRevalidateSecret()is module-scoped per isolate, so a loopbackfetch()landing on a different isolate would fail auth. Consistent with the default per-isolateMemoryCacheHandler(on-demand revalidation against the default cache is already isolate-local), so the secret design doesn't make it worse. A follow-up to reconcile with the persisted build-time prerender secret for the KV-backed case is still worth tracking — not blocking. - Host fallback — the loopback
HEADfalls back to bare"localhost"(no port) only when theHostheader is entirely absent. NormalHostvalues include the port. Not a regression.
Verification done
vp checkclean onpages-revalidate.ts(the only changed file since last review).vp test run tests/nextjs-compat/revalidate-reason.test.ts— 2/2 pass (forged-header rejection + secret-accepted on-demand).vp test run tests/isr-cache.test.ts— 64/64 pass.
Net: all prior review items are addressed. The implementation is sound, well-tested, and the dev/prod paths are kept in sync. Approving from my side — remaining notes are robustness follow-ups, none blocking.
|
Review posted to PR #1856. SummaryReviewed at HEAD Resolved since last review:
Security gate (carried, still correct): On-demand revalidation is authorized by constant-time Remaining non-blocking follow-ups (pre-existing, unchanged):
Verification: All prior review items are addressed; remaining notes are robustness follow-ups, none blocking. |
…d across isolates) The on-demand ISR revalidation secret was a per-process random value. On Cloudflare Workers, res.revalidate()'s loopback fetch() can land on a different isolate than the sender, so a per-process secret mismatches across isolates and false-rejects legitimate revalidations. Mirror Next.js's previewModeId: generate one 256-bit secret at build time (in the vinext build CLI, once per build, shared across plugin instances like __VINEXT_SHARED_BUILD_ID) and bake it SERVER-ONLY into every server bundle via a Vite define (__VINEXT_REVALIDATE_SECRET). It is byte-for-byte identical in every isolate and never reaches the client bundle. getRevalidateSecret() reads the baked constant at runtime and falls back to a process-shared random secret in dev (single-process, no regression). Also persist revalidateSecret in vinext-server.json for diagnostics/tooling parity with prerenderSecret. Constant-time safeEqual validation and reject-arrays/empty/absent behavior are unchanged. Refs #1462
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-review: build-time revalidate secret (47dddbd)
The new commit since my last review (9448a6d) makes the on-demand revalidation secret build-time and shared across isolates instead of per-process — directly addressing the "per-isolate secret on Workers" follow-up I flagged earlier. The approach (resolve once in the vinext build CLI, share via __VINEXT_SHARED_REVALIDATE_SECRET, bake server-only via define into __VINEXT_REVALIDATE_SECRET, read at runtime in getRevalidateSecret()) mirrors the existing __VINEXT_SHARED_BUILD_ID / __VINEXT_SHARED_RSC_COMPATIBILITY_ID coordination and is the right design. The dev fallback (devRevalidateSecret, lazily generated, single-process) is correct and safeEqual/reject-array/reject-empty behavior is unchanged. vp check is clean and revalidate-reason.test.ts (2/2) + isr-cache.test.ts (64/64) still pass.
Two things I'd want addressed before this lands, plus a minor.
🔶 The security-critical "server-only" property is completely untested
The entire point of this commit's security posture is the claim repeated in every comment: the secret is "baked SERVER-ONLY... never reaches the client bundle." If a regression ever let this define leak into the client bundle, the secret would ship to every browser, and anyone could forge x-prerender-revalidate requests — re-opening the exact cache-stampede/DoS vector the secret was introduced to close (the original critical finding in this PR).
That property has zero test coverage, even though the infrastructure to assert it already exists and is trivial to reuse:
tests/compiler-define.test.ts:111already drivesserverDefinePlugin.configEnvironment("rsc"|"ssr"|"client", ...)and assertsclientResultisnullfor server-only defines. A case that setsprocess.env.__VINEXT_SHARED_REVALIDATE_SECRET, then asserts the secret appears in thersc/ssrdefine maps but theclientenv returnsnull, would lock this in directly.tests/app-router-production-build.test.ts:138already does the full-build version for__VINEXT_SHARED_RSC_COMPATIBILITY_ID—readAllJs(server)contains it,readAllJs(client)contains it. The revalidate secret needs the inverse client assertion:expect(readAllJs(path.join(outDir, "client"))).not.toContain(sharedSecret).
Given this PR's whole history is a security fix, shipping the new server-only-secret mechanism without a test that the secret stays out of the client bundle is the one gap I'd consider blocking.
There's also a latent fragility: tests/compiler-define.test.ts:221 (no-ops the configEnvironment hook when defineServer is not configured) now only passes because __VINEXT_SHARED_REVALIDATE_SECRET happens to be unset in that test's environment. With the env var set, configEnvironment would now return a non-null define and that assertion (expect(rscResult).toBeNull()) would flip. Worth making that test explicitly clear the env var so it can't break under a polluted environment.
🔶 readRevalidateSecret + persisted revalidateSecret are dead code
The commit adds readRevalidateSecret() to build/server-manifest.ts:35 and persists revalidateSecret into vinext-server.json (index.ts:4447), with a doc comment justifying it: "persisted here so the Node/prod-server path (which loads the bundle from disk) ... can recover it without re-deriving it."
But nothing reads it — readRevalidateSecret has no callers (only the definition + the doc references). This is not symmetric with readPrerenderSecret, which is genuinely consumed by prod-server.ts:1067/1375 and run-prerender.ts:231 because the host process (separate from the bundle) must send authenticated prerender requests. The revalidate flow is fully self-contained inside the server bundle: res.revalidate() calls getRevalidateSecret() (baked define) and the receiver checks the same baked define. The host never sends a revalidate request, so it never needs the disk value.
So the doc comment's rationale doesn't hold — the prod-server runs the bundle that already carries the inlined constant; it never calls readRevalidateSecret. Either:
- drop
readRevalidateSecretand the persistedrevalidateSecretfield (less surface, no dead code), or - if it's intended purely for diagnostics/tooling, say exactly that in the comment and don't imply the prod-server path consumes it.
I'd lean toward dropping it until there's a real consumer.
🔹 Minor
index.ts:3911— the hook already gates the secret define withname !== "client", and then a few lines later re-checksif (name === "client") return null;(index.ts:3921). The first guard makes the second redundant for the secret (it's still needed for the userdefineServerentries, so leaving it is fine — just noting the secret can never reach the second check via the client path anyway).- The dev-fallback
getRevalidateSecret()readsprocess.env.__VINEXT_REVALIDATE_SECRETat runtime in dev. In prod this is statically replaced bydefine, but in dev it's a real env lookup — if a user happens to have__VINEXT_REVALIDATE_SECRETset in their shell, dev would adopt it. Harmless and namespaced, but the comment says "absent in dev"; it's "absent unless someone sets it."
Verification done
vp checkclean on all 6 changed source files.vp test run tests/nextjs-compat/revalidate-reason.test.ts— 2/2 pass.vp test run tests/isr-cache.test.ts— 64/64 pass.- Confirmed
readRevalidateSecrethas no callers (onlyreadPrerenderSecretis consumed, byprod-server.ts+run-prerender.ts). - Confirmed the existing
compiler-define.test.ts/app-router-production-build.test.tspatterns make the missing server-only test cheap to add.
Net: the build-time-secret design is correct and resolves the cross-isolate concern. The blocking ask is a test that proves the secret never lands in the client bundle (the security property this PR depends on), plus dropping or re-justifying the unused readRevalidateSecret/persisted field.
|
Review posted to PR #1856. SummaryReviewed the new commit Findings:
The core implementation is correct; the blocking ask is the missing client-exclusion test. |
…nifest reader
Address ask-bonk review of the build-time revalidate secret:
- Add tests pinning the security-critical invariant that the
__VINEXT_REVALIDATE_SECRET define is baked into server environments (rsc/ssr)
but NEVER into the client bundle (the client env returns null outright). A
leak would ship the secret to every browser and re-open the cache-stampede
vector the equality check prevents. Also harden the existing "no-ops when
defineServer unset" test to explicitly clear the env var instead of relying on
it happening to be unset.
- Drop the unused readRevalidateSecret export and the orphaned revalidateSecret
manifest field — the runtime authenticates against the baked define (both
Workers isolates and the Node prod-server run the bundle), so the on-disk copy
had no consumer (knip-flagged dead code).
- Hoist the client early-return in vinext:compiler-define-server so the
server-only guarantee is structural (removes the redundant name check), and
reword the dev-fallback comments ("absent unless set during vinext build").
Refs #1462
|
/bigbonk review for issues |
Re-review: server-only secret test coverage + dead-code cleanup (
|
|
Review posted to PR #1856. SummaryReviewed at HEAD Resolved since last review:
CodeQL alerts (170/171/172) on the test file are addressed by the constrained Verification: LGTM — both blocking items resolved, no regressions. |
2 participants
Finishes #1462. The
buildandstalerevalidate reasons already work; this wires the on-demand path (res.revalidate()/revalidatePath/revalidateTag) to passcontext.revalidateReason: "on-demand"intogetStaticProps/getServerSideProps. Verified by the now-passingrevalidate-reason > should support revalidateReason: "on-demand"case. Closes #1462