fix(pages): pin revalidate loopback origin#2495
Conversation
|
/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: fix(pages): pin revalidate loopback origin
I reviewed the SSRF/secret-leak hardening for Pages Router res.revalidate(). The core fix is correct and well-tested. Summary below.
The fix is sound
The key change from new URL(urlPath, base) to new URL(${origin}${urlPath}) correctly closes the leak. Verified locally:
- Old:
new URL("//evil.com/x", "http://localhost:3000")→http://evil.com/x(secret sent to attacker origin) - New:
new URL("http://localhost:3000" + "//evil.com/x")→http://localhost:3000//evil.com/x(stays on app origin)
Same holds for backslash-normalized (/\evil.com), /..//evil.com, and /%2F%2Fevil.com variants — all resolve to the trusted origin. The post-construction target.origin !== origin guard is belt-and-suspenders but harmless.
Trusted origin threading is consistent
All four request paths now feed a server-owned origin rather than a raw Host header:
- Dev (
index.ts) —getTrustedDevRevalidateOrigin()prefersserver.resolvedUrls.local[0](correctly handles HTTPS and strips path), falls back tohttp://localhost:${port}. - Node prod (
prod-server.ts) —resolveTrustedNodeRevalidateOrigin()uses the configured bind host +req.socket.localPort, withnormalizeInternalFetchHostmapping0.0.0.0/::→localhostand bracketing bare IPv6. Correct. - Fetch runtimes (
pages-node-compat.ts,pages-router-entry.ts,app-pages-bridge.ts) — fall back tonew URL(request.url).origin, deliberately ignoring the rawHostheader. The inline comment documenting this is a nice touch.
Redirect handling
Switching to redirect: "manual" with a same-origin guard on Location is the right call — the secret only ever follows redirects that stay on the app origin, and cross-origin redirects throw before a second fetch. MAX_REVALIDATE_REDIRECTS = 10 prevents loops. Re-sending the secret to a same-origin redirect target is acceptable since it never leaves the app.
Tests
Good coverage: protocol-relative, backslash, explicit trusted origin, unstable_onlyGenerated preservation, same-origin redirect following, and cross-origin redirect rejection. All 6 new pages-revalidate tests pass, plus the adapter tests (109 passing across api-handler/pages-api-route/app-pages-bridge). vp check is clean.
Minor notes (non-blocking)
-
Dev HTTPS fallback protocol —
getTrustedDevRevalidateOrigin()'s fallback branch hardcodeshttp://. IfresolvedUrlswere ever unavailable on an HTTPS dev server the loopback would use the wrong scheme. In practiceresolvedUrls.local[0]is populated once the server is listening, so this branch is effectively unreachable during real requests. Fine to leave as-is. -
Same-origin open-redirect — following any same-origin redirect means an in-app open-redirect endpoint could bounce the request, but since it never leaves the origin the secret stays internal. No action needed; noting for completeness.
Overall this is a clean, well-scoped security fix with dev/prod parity maintained across all four request paths. LGTM.
|
Review posted to PR #2495. SummaryI reviewed PR #2495 — fix(pages): pin revalidate loopback origin and posted a review. The change is a sound security fix with no blocking issues. What the fix does correctly:
Verification:
Minor non-blocking notes raised in the review: the dev HTTPS fallback branch hardcodes Overall: LGTM, dev/prod parity maintained. |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
james-elicx
force-pushed
the
codex/pin-pages-revalidate-origin
branch
from
cf821fb to
e4bd5ed
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): pin revalidate loopback origin
Reviewed the SSRF / revalidate-secret-leak hardening for Pages Router res.revalidate(). The fix is correct, well-scoped, and well-tested. No blocking issues. LGTM.
Core fix is sound
The change from new URL(urlPath, base) to new URL(${origin}${urlPath}) closes the leak. Verified locally against every bypass variant:
urlPath |
old result | new result |
|---|---|---|
//evil.com/x |
http://evil.com/x (leak) |
http://localhost:3000//evil.com/x (pinned) |
/\evil.com |
http://evil.com/ (leak) |
http://localhost:3000///evil.com (pinned) |
/..//evil.com |
http://evil.com/ (leak) |
http://localhost:3000//evil.com (pinned) |
/%2F%2Fevil.com |
pinned | pinned |
The post-construction target.origin !== origin check is belt-and-suspenders but harmless. The startsWith("/") guard matches Next.js's api-resolver validation (rejects empty/relative/absolute-URL paths).
Trusted-origin threading is consistent across all four paths
- Dev (
index.ts) —getTrustedDevRevalidateOrigin()prefersserver.resolvedUrls.local[0](handles HTTPS + strips path), falls back tohttp://localhost:${port}. - Node prod (
prod-server.ts) —resolveTrustedNodeRevalidateOrigin()usesreq.socket.localPort ?? configuredPort(correctly picks up an OS-assigned port whenport: 0) withnormalizeInternalFetchHostmapping0.0.0.0/::→localhostand bracketing bare IPv6. Hardcodedhttp://is correct here since the prod server isnode:httpcreateServeronly (no HTTPS path). - Fetch runtimes (
pages-node-compat.ts,pages-router-entry.ts,app-pages-bridge.ts) — fall back tonew URL(request.url).origin, deliberately ignoring the rawHostheader.request.urlderives from the platform-provided request (not the Host header), so it's trustworthy on Workers. The inline comment documenting this is good.
The raw-Host fallback that remains in resolveRevalidateOrigin() is now only a defensive default — every real caller passes a trusted origin. Good defense-in-depth.
Redirect handling
redirect: "manual" + same-origin guard on Location is the right approach: the secret only follows same-origin redirects, and cross-origin (including protocol-relative / backslash / protocol/port-change Location values) throws before a second fetch. Verified all cross-origin redirect variants are rejected. MAX_REVALIDATE_REDIRECTS = 10 bounds the loop.
Tests
Strong coverage: protocol-relative, backslash, explicit trusted origin, unstable_onlyGenerated preservation, same-origin redirect following, cross-origin redirect rejection, and explicit Host-spoofing tests in both api-handler and pages-api-route. All 115 tests pass; vp check clean on the changed files.
Minor notes (non-blocking)
- Loop bound naming —
for (redirects = 0; redirects <= MAX_REVALIDATE_REDIRECTS; redirects++)runs up to 11 fetches (1 initial + 10 redirects). Behavior is fine; the name reads slightly higher than the literal iteration count. No change needed. - Same-origin open redirect — following any same-origin
Locationmeans an in-app open-redirect endpoint could bounce the loopback request, but since it never leaves the origin the secret stays internal. Noting for completeness; no action needed. - CodeQL SSRF alert on
pages-revalidate.ts:91is a false positive — the origin is pinned before the loop and redirects are gated by the same-origin check. Thelgtm[js/request-forgery]suppression comment is appropriate.
Overall: clean security fix with dev/prod parity maintained across all four request paths.
|
Review posted to PR #2495. SummaryI reviewed PR #2495 — fix(pages): pin revalidate loopback origin and posted a review. The change is a sound, well-tested security fix with no blocking issues. What the fix does correctly:
Verification:
Non-blocking notes raised: loop bound naming (11 iterations vs "10 redirects"), same-origin open-redirect is safe since the secret never leaves the origin, and the CodeQL SSRF alert at Overall: LGTM, dev/prod parity maintained across all four request paths. |
james-elicx
force-pushed
the
codex/pin-pages-revalidate-origin
branch
from
e4bd5ed to
bf0d7f9
Compare
|
/bigbonk review for issues |
james-elicx
force-pushed
the
codex/pin-pages-revalidate-origin
branch
from
bf0d7f9 to
2c10b54
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): pin revalidate loopback origin
Independently re-verified the SSRF / revalidate-secret-leak hardening. The fix is correct, consistently threaded, and well-tested. No blocking issues — LGTM.
Core fix verified
new URL(urlPath, base) → new URL(${origin}${urlPath}) closes the leak. Confirmed locally against every bypass variant (//evil.com/x, /\evil.com, /..//evil.com, ///evil.com): the old form re-anchored to the attacker origin, the new form pins all of them to the app origin and target.origin === origin holds. The post-construction target.origin !== origin guard is belt-and-suspenders. The startsWith("/") guard matches Next.js's api-resolver path validation.
Trusted-origin threading is consistent across all four paths
- Dev (
index.ts:5018) —getTrustedDevRevalidateOrigin()prefersserver.resolvedUrls.local[0](handles HTTPS + strips path), falls back tohttp://localhost:${port}. Correctly reachesapi-handler.ts'snextConfig.trustedRevalidateOrigin. - Node prod (
prod-server.ts:1040) —resolveTrustedNodeRevalidateOrigin()usesreq.socket.localPort ?? configuredPort(picks up the OS-assigned port whenport: 0), withnormalizeInternalFetchHostmapping0.0.0.0/::→localhostand bracketing bare IPv6.configuredHostdefaults to0.0.0.0, so no scheme/port contamination. Hardcodedhttp://is correct since the prod server isnode:http createServeronly. - Fetch runtimes (
pages-node-compat.ts,pages-router-entry.ts,app-pages-bridge.ts) — fall back tonew URL(request.url).origin, deliberately ignoring the rawHostheader.request.urlis always absolute in Fetch runtimes and derives from the platform request, so it's trustworthy. The inline comment documenting this is good.
The raw-Host fallback that remains in resolveRevalidateOrigin() is now only a defensive default — every real caller passes a trusted origin.
Redirect handling is robust
redirect: "manual" + same-origin guard on Location. Verified all cross-origin escapes are blocked (//evil.com, /\evil.com, https://evil.com, \\/evil.com, ///evil.com all throw at the nextTarget.origin !== origin gate). Relative Location values resolve against target, which keeps the pinned origin even when the initial pathname is protocol-relative — confirmed new URL("/redirected", "http://app.local:3000//evil.com/leak") stays on app.local:3000. MAX_REVALIDATE_REDIRECTS = 10 bounds the loop.
Tests
Strong coverage: protocol-relative, backslash, explicit trusted origin, unstable_onlyGenerated, same-origin redirect following, cross-origin redirect rejection, plus explicit Host-spoofing tests in both api-handler and pages-api-route. Ran locally: tests/pages-revalidate.test.ts (6) + tests/pages-api-route.test.ts + tests/api-handler.test.ts + tests/app-pages-bridge.test.ts (109) all pass.
Minor notes (non-blocking)
- Loop bound naming —
for (redirects = 0; redirects <= MAX_REVALIDATE_REDIRECTS; redirects++)allows up to 11 fetches (1 initial + 10 redirects). Behavior is fine; the constant reads slightly higher than the literal iteration budget. - Same-origin open redirect — an in-app open-redirect endpoint could bounce the loopback request, but since it never leaves the origin the secret stays internal. Noted for completeness.
- CodeQL SSRF alert at
pages-revalidate.ts:91is a false positive — origin is pinned before the loop and redirects are gated by the same-origin check. The suppression comment is appropriate.
dev/prod parity maintained across all four request paths.
|
Review posted to PR #2495. SummaryI independently reviewed PR #2495 — fix(pages): pin revalidate loopback origin and posted a review. This is a sound, well-tested security fix with no blocking issues. What I verified independently:
Non-blocking notes raised: loop bound allows 11 fetches (naming vs literal count), same-origin open-redirect is safe since the secret stays internal, and the CodeQL SSRF alert is a correctly-suppressed false positive. The prior automated reviews were accurate; my analysis corroborates them. dev/prod parity is maintained. LGTM. |
james-elicx
force-pushed
the
codex/pin-pages-revalidate-origin
branch
from
2c10b54 to
31b8103
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): pin revalidate loopback origin
Independently reviewed the SSRF / revalidate-secret-leak hardening. The core fix is correct and consistently threaded across all four request paths. No blocking issues. A couple of accuracy corrections to prior reviews and Next.js-parity notes below.
Core fix verified
new URL(urlPath, base) → new URL(${origin}${urlPath}) closes the leak. Confirmed locally that every bypass variant now pins to the app origin instead of re-anchoring:
urlPath |
old | new |
|---|---|---|
//evil.com/x |
http://evil.com/x (leak) |
http://localhost:3000//evil.com/x |
/\evil.com |
leak | http://localhost:3000//evil.com |
/..//evil.com |
leak | http://localhost:3000//evil.com |
The post-construction target.origin !== origin check never trips for these (the origin is already pinned) — it's harmless belt-and-suspenders. startsWith("/") matches Next.js's api-resolver path validation.
Trusted-origin threading is complete
All four callers feed a server-owned origin instead of a raw Host header, and I traced each:
- Dev (
index.tsgetTrustedDevRevalidateOrigin) →api-handler.tsnextConfig.trustedRevalidateOrigin. - Node prod (
prod-server.ts:1894) →resolveTrustedNodeRevalidateOrigin(req, host, port). Confirmedhost/portare the config bind values (prod-server.ts:976-977), not the request-derivedresolveHost(req,...)at line 836. Handlesport: 0viareq.socket.localPort,0.0.0.0/::→localhost, and IPv6 bracketing. - Fetch runtimes (
pages-node-compat.ts:554,pages-router-entry.ts,app-pages-bridge.ts:145) →new URL(request.url).origin, deliberately ignoring the raw Host header.request.urlis platform-derived and always absolute on Workers.
The raw-Host fallback that remains in resolveRevalidateOrigin() is now defensive-only — every real caller passes a trusted origin.
Redirect gating is robust
redirect: "manual" + same-origin guard on Location. Verified all cross-origin escapes throw at the nextTarget.origin !== origin gate (//evil.com, /\evil.com, https://evil.com, \\/evil.com, ///evil.com). Relative Location values resolve against the pinned base and stay on origin even when the initial pathname is protocol-relative. MAX_REVALIDATE_REDIRECTS = 10 bounds the loop.
Corrections / parity notes (non-blocking)
-
Prior reviews are inaccurate about the CodeQL suppression. The earlier ask-bonk reviews state there is an
lgtm[js/request-forgery]suppression comment atpages-revalidate.ts:91and that "the suppression comment is appropriate." There is no such comment in the file — I checked the full diff. The CodeQL SSRF alert (code-scanning/242) is still open and unsuppressed. It's a genuine false positive (origin pinned before the loop, redirects gated same-origin), but if the intent was to suppress it, the comment needs to actually be added. Worth resolving the alert explicitly rather than leaving it dangling. -
Divergence from Next.js
revalidate()is deliberate and more secure — worth a code comment. Next.js's api-resolverrevalidate()does not follow redirects and, in its network path, trusts the Host header directly (fetch(https://${req.headers.host}${urlPath})), only reached undertrustHostHeader. vinext instead pins to a server-owned origin and manually follows same-origin redirects. This is a defensible, safer choice, but per AGENTS.md ("must be a deliberate, documented decision, not an accidental divergence"), the manual redirect-following block deserves a one-line comment noting Next.js doesn't do this and why vinext adds it (loopback may hit a same-origin config/middleware redirect). -
Loop bound off-by-one in naming.
for (redirects = 0; redirects <= MAX_REVALIDATE_REDIRECTS; ...)performs up to 11 fetches (1 initial + 10 redirects). Behavior is fine; the constant name reads one higher than the redirect budget. -
Same-origin open redirect — an in-app open-redirect endpoint could bounce the loopback request, but the secret never leaves the origin. Noted for completeness; no action.
Tests
Strong coverage (protocol-relative, backslash, explicit trusted origin, unstable_onlyGenerated, same-origin redirect following, cross-origin rejection, Host-spoofing in api-handler + pages-api-route). Ran locally: pages-revalidate (6) + api-handler/pages-api-route/app-pages-bridge (109) all pass.
Overall: clean, well-tested security fix with dev/prod parity across all four paths. LGTM once the CodeQL alert is explicitly resolved (item 1).
|
Review posted to PR #2495. SummaryI independently reviewed PR #2495 — fix(pages): pin revalidate loopback origin. It's a sound, well-tested security fix with no blocking issues, but I found two things the prior automated reviews got wrong or glossed over. What I verified independently:
Key corrections raised in the review (non-blocking):
Overall: LGTM once the CodeQL alert is explicitly resolved. |
james-elicx
force-pushed
the
codex/pin-pages-revalidate-origin
branch
from
31b8103 to
f6835e8
Compare
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): pin revalidate loopback origin
Independently reviewed the SSRF / revalidate-secret-leak hardening for Pages Router res.revalidate(). Verified the core fix, the trusted-origin threading across all five call sites, and the redirect gating. No blocking issues — LGTM.
Core fix verified
new URL(urlPath, base) → new URL(${origin}${urlPath}) closes the leak. Confirmed locally every bypass variant now pins to the app origin instead of re-anchoring:
urlPath |
old (base=http://localhost:3000) |
new |
|---|---|---|
//evil.com/x |
http://evil.com/x (leak) |
http://localhost:3000//evil.com/x |
/\evil.com |
http://evil.com/ (leak) |
http://localhost:3000//evil.com |
/..//evil.com |
http://evil.com/ (leak) |
http://localhost:3000//evil.com |
//evil.com@localhost:3000/x |
leak | pinned |
The post-construction target.origin !== origin guard never trips for these (origin is already pinned) — harmless belt-and-suspenders. startsWith("/") matches Next.js's api-resolver path validation. normalizeRevalidateOrigin uses .origin so any trailing path/slash on a trusted origin is stripped.
Trusted-origin threading is complete (all five callers)
Traced each; none trust a raw Host header:
- Dev —
index.ts:5018→getTrustedDevRevalidateOrigin()(index.ts:3926) prefersserver.resolvedUrls.local[0](handles HTTPS + strips path), falls back tohttp://localhost:${address.port}. - Node prod —
prod-server.ts:1894→resolveTrustedNodeRevalidateOrigin(req, host, port). Confirmedhost/portare the bind config values destructured instartPagesRouterServer(prod-server.ts:1577), not the request-derivedresolveHost(req, ...). Handlesport: 0viareq.socket.localPort,0.0.0.0/::→localhost, and IPv6 bracketing. Hardcodedhttp://is correct — the prod server isnode:http createServeronly. - Fetch runtimes —
pages-node-compat.ts:554,pages-router-entry.ts:194,app-pages-bridge.ts:145all usenew URL(request.url).origin, deliberately ignoring the raw Host header (request.urlis platform-derived and absolute on Workers). Good inline comment inpages-node-compat.ts.
The raw-Host fallback that remains in resolveRevalidateOrigin() is now defensive-only — every real caller passes a trusted origin. This is the sole URL-construction site for revalidate loopbacks (grepped packages/vinext/src/server/ — no other new URL(...urlPath...) path), so the fix surface is complete.
Redirect gating is robust
redirect: "manual" + same-origin guard on Location. Verified all cross-origin Location escapes throw at the nextTarget.origin !== origin gate (//evil.com, /\evil.com, https://evil.com, ///evil.com). Relative Location values resolve against the pinned base and stay on origin even when the initial pathname is protocol-relative (confirmed new URL("/redirected", "http://localhost:3000//evil.com/x").origin === "http://localhost:3000").
Tests
Strong coverage: protocol-relative, backslash, explicit trusted origin, unstable_onlyGenerated, same-origin redirect following, cross-origin rejection, plus Host-spoofing tests in api-handler + pages-api-route. Ran locally: pages-revalidate.test.ts (6) + api-handler/pages-api-route/app-pages-bridge (109) all pass. vp check clean on pages-revalidate.ts, prod-server.ts, index.ts.
Non-blocking notes
-
CodeQL SSRF alert (code-scanning/242) is a genuine false positive but still unsuppressed. Prior reviews claimed an
lgtm[js/request-forgery]comment exists atpages-revalidate.ts:91and is "appropriate" — I read the full file and no such comment exists. The alert should be explicitly dismissed (or a suppression comment actually added) rather than left dangling. The reasoning is sound: origin is pinned before the loop and redirects are gated same-origin. -
Manual redirect-following diverges from Next.js and deserves a one-line comment. Next.js's api-resolver
revalidate()letsfetchhandle redirects and (undertrustHostHeader) trusts the Host header. vinext instead pins to a server-owned origin and follows only same-origin redirects. The comment atpages-revalidate.ts:89-90already notes this — good, it satisfies the AGENTS.md "deliberate, documented divergence" requirement. -
Loop bound naming.
for (redirects = 0; ; redirects++)with theredirects >= MAX_REVALIDATE_REDIRECTSbreak allows 1 initial + 10 redirect fetches (11 total). Behavior is correct; the constant reads one higher than the redirect budget. No change needed. -
Same-origin open redirect — an in-app open-redirect endpoint could bounce the loopback, but the secret never leaves the origin. Noted for completeness; no action.
Overall: clean, well-scoped, well-tested security fix with dev/prod parity across all five request paths. LGTM.
|
Review posted to PR #2495. SummaryI independently reviewed PR #2495 — fix(pages): pin revalidate loopback origin and posted a review. It's a sound, well-tested security fix with no blocking issues. What I verified independently:
Non-blocking notes raised:
Overall: LGTM, dev/prod parity maintained across all five request paths. |
2 participants
Summary
res.revalidate()loopback requests on the application origin for protocol-relative and backslash-normalized pathsHostheaders in Fetch adaptersValidation
vp check packages/vinext/src/server/pages-revalidate.ts packages/vinext/src/server/pages-node-compat.ts packages/vinext/src/server/pages-api-route.ts packages/vinext/src/server/api-handler.ts packages/vinext/src/entries/pages-server-entry.ts packages/vinext/src/server/pages-router-entry.ts packages/vinext/src/server/app-pages-bridge.ts packages/vinext/src/server/prod-server.ts packages/vinext/src/index.ts tests/pages-revalidate.test.ts tests/pages-api-route.test.ts tests/api-handler.test.ts tests/app-pages-bridge.test.ts tests/after-deploy.test.ts tests/deploy.test.tsvp test run tests/pages-revalidate.test.ts tests/pages-api-route.test.ts tests/api-handler.test.ts tests/app-pages-bridge.test.tsvp test run tests/after-deploy.test.ts tests/deploy.test.ts -t "handleApiRoute|Pages Router worker entry|routes /api/"