fix(metadata): render redirect meta-refresh from resolved URL, not re-parsed digest#2057
fix(metadata): render redirect meta-refresh from resolved URL, not re-parsed digest#2057Divkix wants to merge 1 commit into
Conversation
commit: |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: LGTM
Clean, well-isolated fix with a correct root-cause diagnosis. The metadata document-redirect path already holds a fully-resolved digestUrl (basePath applied via applyAppPageRedirectBasePath, percent-encoding preserved through the new URL() round-trip), so rendering the meta-refresh tag directly from it via renderRedirectRefreshMetaTag avoids the lossy parseNextRedirectDigest round-trip (; truncation + decodeURIComponent double-decode). Verified the new URL() round-trip preserves %25 and ; for the asserted cases.
What I verified:
- basePath not double-applied — the metadata path resolves basePath once (app-page-execution.ts:340) and
renderRedirectRefreshMetaTagcorrectly does not callprefixRedirectLocation. The "applies basePath exactly once" test covers this. - Digest path contract preserved —
renderSsrErrorMetaTag(the streaming-render path viacreateSsrErrorMetaRendererin app-ssr-entry.ts:519) still decodes viaparseNextRedirectDigest+prefixRedirectLocation, which is correct since it consumes encoded shim digests. The sharedbuildRedirectRefreshMetahelper keeps both paths in sync on delay/escaping. - 307 bot path unaffected — app-page-execution.ts:412-432 emits
prefixedLocationdirectly in theLocationheader (no digest round-trip), matching the PR's claim that path was already correct. - Flight digest unchanged —
formatNextRedirectDigest/VINEXT_RSC_REDIRECT_HEADERstill use the verbatimdigestUrl. - Single caller of
buildMetadataRedirectHtmlResponse, properly updated to the new{ url, statusCode }signature. vp test runon both affected files: 51 passed.vp check: clean (format, lint, types).
Test coverage is thorough (percent-encoding, ;, 308 delay, basePath-once, cross-origin, HTML-attr escaping) at both the integration (buildAppPageSpecialErrorResponse) and unit (renderRedirectRefreshMetaTag) levels.
No blocking issues.
| * resolved URL directly mirrors Next.js's `make-get-server-inserted-html`, which | ||
| * renders the raw `getURLFromRedirectError(error)` URL verbatim. | ||
| */ | ||
| export function renderRedirectRefreshMetaTag(url: string, status: number): string { |
There was a problem hiding this comment.
Optional: renderRedirectRefreshMetaTag is now a thin one-line wrapper that just forwards to buildRedirectRefreshMeta. Since the only behavioral difference between the two paths is whether prefixRedirectLocation runs (and that's handled at the call sites), you could export buildRedirectRefreshMeta directly and drop the wrapper. The extensive doc comment is valuable, though — if you keep the wrapper purely as a documented public entry point, that's a reasonable call. Non-blocking.
|
Review submitted successfully. SummaryI reviewed PR #2057 (fix for #1977 — metadata redirect meta-refresh URL corruption) and approved it. The fix is correct and well-isolated. The bug was that What I verified:
One non-blocking note: |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
…-parsed digest generateMetadata() redirect()s to URLs with percent-encoded characters or a ';' in the query were corrupted in the streaming-metadata meta-refresh tag: the verbatim NEXT_REDIRECT digest was re-parsed by parseNextRedirectDigest (parts[2] + decodeURIComponent), double-decoding %25 (e.g. /search?q=50%25off -> /search?q=50%off) and truncating at the first ';' (e.g. /a?b=1;c=2 -> /a?b=1). The metadata document-redirect path already holds the fully-resolved URL (digestUrl), so render the refresh tag directly from it via the new renderRedirectRefreshMetaTag() helper instead of round-tripping through a digest that parseNextRedirectDigest mis-decodes. parseNextRedirectDigest and the verbatim RSC-flight digest (formatNextRedirectDigest) are left unchanged, so the shim/encoded-digest and flight paths keep their contracts. Matches Next.js's make-get-server-inserted-html, which emits the raw redirect URL verbatim. Fixes cloudflare#1977.
51577b2 to
5ef677b
Compare
Problem
When
generateMetadata()callsredirect()to a target containing percent-encoded characters or a;in the query (document request, streaming-capable UA), the emitted meta-refresh URL is corrupted:/search?q=50%25off→ double-decoded to/search?q=50%off/a?b=1;c=2→ truncated at the first;to/a?b=1The browser then navigates to a wrong/malformed URL. Next.js handles both correctly — this is a vinext-only defect. Fixes #1977.
Root cause
buildAppPageSpecialErrorResponsealready resolves the redirect target intodigestUrl(a correct, fully-resolved, percent-encoded URL — basePath applied), then builds a verbatimNEXT_REDIRECT;replace;<url>;<status>;digest. For the streaming-metadata document path it fed that digest intorenderSsrErrorMetaTags→parseNextRedirectDigest, which was designed for the encoded digests theredirect()/permanentRedirect()shims emit: it splits on;and keeps onlyparts[2](truncating any;in the URL) anddecodeURIComponents the result (double-decoding%25→%). Feeding it a verbatim digest corrupts the URL.The bug is isolated to this path — the html-limited-bot 307 path already emits the URL correctly in its
Locationheader.Fix
The metadata document-redirect path already holds the resolved URL, so render the refresh tag directly from it via a new
renderRedirectRefreshMetaTag(url, status)helper, skipping the lossy digest round-trip. This mirrors Next.js'smake-get-server-inserted-html, which emits the rawgetURLFromRedirectError(error)URL verbatim (slice-rejoin, no decode).Deliberately left unchanged (each a separate, correct contract):
parseNextRedirectDigest— still correct for its 3 encoded-digest callers (resolveAppPageSpecialError, server-action & route-handler policy) and the streaming-render path (createSsrErrorMetaRenderer), which all consume encoded shim digests.formatNextRedirectDigest/ the RSC-flight digest — contractually verbatim, consumed by the clientdecodeRedirectErrorand asserted by existing tests.Changes
app-ssr-error-meta.ts: extractbuildRedirectRefreshMeta(location, status); add exportedrenderRedirectRefreshMetaTag(url, status);renderSsrErrorMetaTag(digest path) keeps its existing decode behavior.app-page-execution.ts:buildMetadataRedirectHtmlResponsenow takes the resolved{ url, statusCode }and renders directly; the verbatimdigestis still computed for the RSC-flight path.Test plan
redirect('/search?q=50%25off')andredirect('/a?b=1;c=2')fromgenerateMetadata()now produce the exact, intact URL in the meta-refresh tag.tests/app-page-execution.test.ts(percent-encoding,;, 308 delay, basePath applied once, cross-origin) and direct-render unit tests intests/app-ssr-error-meta.test.ts(no decode, no truncation, HTML-escaping). The two core cases fail before the change and pass after.parseNextRedirectDigest/ flight-digest tests unaffected.