refactor(app-rsc): extract response finalisation into server/app-rsc-response-finalizer#1035
Conversation
commit: |
NathanDrake2406
force-pushed
the
refactor/app-rsc-response-finalizer
branch
from
8ee121a to
0582af5
Compare
The RSC entry handler inlined all URL parsing, pathname decoding, basePath checking, RSC detection, and header sanitization directly in its _handleRequest template. That made the logic impossible to unit-test, hard to audit for security, and duplicated normalizeMountedSlotsHeader in three places. Create server/app-rsc-request-normalization.ts to own the full normalization pipeline as a single, typed function (normalizeRscRequest). The function returns a discriminated Response | NormalizedRscRequest, encoding early exits (400/404) in the type rather than as throw-then-catch pairs. The ordering within the function is security-critical and documented step by step: 1. Parse URL 2. Protocol-relative guard (on raw pathname, before normalizePath collapses //) 3. Strict segment-wise percent-decode (malformed encoding -> 400) 4. normalizePath (collapse //, resolve . and .. segments) 5. basePath check + strip (missing prefix -> 404; /__vinext/ bypasses) 6/7. RSC detection and cleanPathname 8. Sanitize X-Vinext-Interception-Context (strip null bytes) 9. Normalize x-vinext-mounted-slots (dedup, sort) Consolidate normalizeMountedSlotsHeader: the function existed independently in isr-cache.ts and as a private copy in app-elements.ts. Both now import from the new module. isr-cache.ts re-exports it for callers already importing from there. 43 new behavior tests cover the security-sensitive paths: protocol-relative variants (literal, backslash, %5C, %2F), malformed percent sequences, basePath enforcement, %2F preservation, RSC detection by suffix and Accept header, interception context sanitization, and mounted-slots normalization.
NathanDrake2406
force-pushed
the
refactor/app-rsc-response-finalizer
branch
from
0582af5 to
58f114a
Compare
Config header application runs inline in the generated RSC entry handler()
as a template string with no tests. Every response variant (page, route
handler, server action, not-found) goes through handler(), but the shared
finalisation logic cannot be tested or reused — a classic rot factory.
Create server/app-rsc-response-finalizer.ts that owns:
- Skip 3xx redirect responses. Response.redirect() produces immutable Headers
that throw on write; Next.js also deliberately excludes config headers from
redirects.
- Normalize the request pathname (segment-wise percent-decode + collapse)
before matching config header source patterns.
- Strip basePath at a segment boundary before matching. The old inline code
used pathname.startsWith(basePath) which incorrectly matched /app2/page when
basePath is /app; stripBasePath() enforces the separator boundary.
- Apply matching config headers via applyConfigHeadersToResponse, preserving
middleware-set header precedence.
The generated entry now calls __finalizeAppRscResponse(response, request, {...})
in place of ~15 lines of template logic, and drops the now-unused
applyConfigHeadersToResponse and normalizePathnameForRouteMatch imports from
the generated code.
Tests cover: header applied to 200, empty configHeaders no-op, source
pattern miss, immutable 307 no throw, mutable 308 header skipped, basePath
strip, basePath segment-boundary (regression for startsWith bug), nested
basePath, has-condition match, has-condition miss.
NathanDrake2406
force-pushed
the
refactor/app-rsc-response-finalizer
branch
from
58f114a to
d5e9b24
Compare
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean extraction that follows the established pattern of moving runtime behavior out of generated entry templates into typed, testable modules. Verified all 10 tests pass, the modified app-router snapshot test passes, and no dangling references remain from removed imports.
The basePath segment-boundary bug fix (startsWith -> stripBasePath) is a real correctness improvement — good catch.
One minor observation below (not blocking).
| response: Response, | ||
| request: Request, | ||
| options: FinalizeAppRscResponseOptions, | ||
| ): Response { |
There was a problem hiding this comment.
The old inline code had a null guard: if (response && response.headers && ...). This extraction drops it, typing response as Response (non-nullable).
I verified this is correct — _handleRequest has exhaustive return paths and always returns a Response. The old guard was defensive coding in untyped generated JS, and the typed module properly eliminates it.
Just noting for posterity that this was checked.
| const url = new URL(request.url); | ||
| let pathname: string; | ||
| try { | ||
| pathname = normalizePath(normalizePathnameForRouteMatch(url.pathname)); |
There was a problem hiding this comment.
Nit: the comment on line 50 mentions normalizePathnameForRouteMatchStrict but the function called here is normalizePathnameForRouteMatch (non-strict). The strict variant is used earlier in the request pipeline for the 400 response. The comment is accurate about why the catch is safe (the strict check already ran), but the naming mismatch could confuse a future reader skimming this file.
Consider:
| pathname = normalizePath(normalizePathnameForRouteMatch(url.pathname)); | |
| pathname = normalizePath(normalizePathnameForRouteMatch(url.pathname)); | |
| } catch { | |
| // Malformed percent-encoding. The earlier request normalisation step | |
| // (normalizePathnameForRouteMatchStrict in app-rsc-request-normalization) | |
| // already returned 400 for truly-malformed paths. This catch exists as a | |
| // safety net for edge cases; keep the historical raw-path fallback rather | |
| // than crashing the response. |
|
Review submitted with approval. Here's a summary of what I found: Approved. This is a well-executed extraction that moves response finalisation logic from an inline template string in the generated RSC entry into a dedicated typed module ( Key findings:
|
Based on #1034
What this changes
Extracts the App Router response finalisation step from an inline template string in the generated
handler()function into a dedicated typed module:server/app-rsc-response-finalizer.ts.The extracted module owns:
Response.redirect()creates immutableHeadersthat throwTypeError: Cannot modify immutable headerson write. Next.js also excludes config headers from redirects.applyConfigHeadersToResponsewithout overwriting middleware-set headers.The generated entry is reduced to a single delegation call.
Why
Every response variant (page, route handler, server action, metadata, not-found) passes through
handler(). The shared finalisation logic lived in a template string with no direct tests and no reuse path. Per the architecture rule: codegen describes app shape; normal modules implement behaviour.Bug fixed in the same change: the inline basePath strip used
pathname.startsWith(basePath), which is a string-prefix match./app2/pagewithbasePath=/appwould be incorrectly stripped to/2/page, potentially causing wrong config headers to apply.stripBasePath()enforces a path-separator boundary.Refs #1020.
Approach
New module imports from existing typed helpers only:
applyConfigHeadersToResponsefromserver/request-pipeline.ts(source)stripBasePathfromutils/base-path.tsfor segment-boundary correctnessnormalizePath+normalizePathnameForRouteMatchfor pathname normalisationGenerated entry drops two imports (
applyConfigHeadersToResponse,normalizePathnameForRouteMatch) that are now internal to the helper.Validation
New test file
tests/app-rsc-response-finalizer.test.ts— 10 tests covering:configHeaders: []is a no-op (returns same response object)Response.redirect()) does not throwstartsWithbug)has-condition match applies headerhas-condition miss skips headerEntry template snapshot updated automatically by pre-commit hook.
vp checkclean.Risks / follow-ups
mergeMiddlewareResponseHeadersin the plain-text 404 path inside_handleRequestis not yet moved; that path is simpler and lower risk. Can be a follow-up if desired.server/prod-server.ts) has its own header finalisation path that is separately out of scope for this PR.