refactor: extract shared request handling from server entry points#258
refactor: extract shared request handling from server entry points#258yunus25jmi1 wants to merge 1 commit intocloudflare:mainfrom
Conversation
commit: |
yunus25jmi1
force-pushed
the
refactor/shared-request-handling
branch
from
14ccb24 to
631e3fc
Compare
|
All checks have passed with all green. Kindly review the changes. @southpolesteve @elithrar |
yunus25jmi1
force-pushed
the
refactor/shared-request-handling
branch
2 times, most recently
from
2cb7c43 to
6624adc
Compare
|
@southpolesteve review the changes. Conflicts resolved. |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Extract shared request handling from server entry points
The direction of this PR is right — deduplicating the inline __-prefixed helpers from the generated code into importable modules is a meaningful improvement. Having a single canonical matchConfigPattern (single-pass tokenizer) instead of the chained .replace() variant is a real fix.
However, there are several issues that need to be addressed before this can merge.
Key Issues
-
Dead code in
request-pipeline.ts— Four of the seven exported functions (guardProtocolRelativeUrl,stripBasePath,normalizeTrailingSlash,processMiddlewareHeaders) are never imported or called anywhere in the codebase. The protocol-relative guard is still inlined inapp-dev-server.tsat line 1125. This new module ships dead code that increases the bundle without any deduplication benefit. Either wire up these functions at the call sites that have the inline versions, or remove them from this PR and add them when they're actually used. -
Behavioral difference in
validateImageUrl— The old code resolved the image URL againstrequest.url(full URL with path+query), while the new code resolves againsturl.origin(scheme+host only). For root-relative paths like/images/foo.pngthis is equivalent, but the origin check insidevalidateImageUrlresolves againsturl.origintoo (line 190), which means the defense-in-depth check is comparingurl.origin === url.origin— it's a tautology. This was also a tautology in the original code (since/images/foo.pngresolved against the same origin will always keep the same origin), so the security posture is unchanged, but it would be good to document why this defense-in-depth check exists despite being tautological for the filtered inputs. -
Extra blank lines in generated code — The removal of the inline functions left orphaned blank lines in the generated entry at two locations (lines 1049-1051 and 1173-1176). Minor, but noticeable in the generated output.
-
safeRegExpremoved from imports but was previously used inindex.ts— The diff removessafeRegExpfrom theconfig-matchers.jsimport inindex.ts(line 29-38). Verify this isn't used elsewhere in the file or re-exported for consumers.
Correctness Observations
- The
validateCsrfOriginsignature change from closure-captured__allowedOriginsto an explicit parameter is clean and correct. The call site at line 1352 passes__allowedOriginsexplicitly. - The
matchRedirect/matchRewrite/matchHeadersfunctions inconfig-matchers.tstake individual rule arrays, which matches how they're called in the generated code (e.g.,matchRedirect(__redirPathname, __configRedirects, __reqCtx)). - The
requestContextFromRequestreplacement for__buildRequestContextis a straightforward 1:1 swap with the same semantics. - The
matchConfigPatternre-export fromindex.ts(line 3749) preserves the public API for external consumers. - Test updates correctly reflect the new function names and are otherwise semantically equivalent.
Suggestions
See inline comments.
| return new Response("404 Not Found", { status: 404 }); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
guardProtocolRelativeUrl is exported but never imported anywhere. The protocol-relative URL guard is still inlined in app-dev-server.ts at line 1125:
if (url.pathname.replaceAll("\\\\" , "/").startsWith("//")) {Either replace the inline check with this function, or remove it from this PR. Shipping dead code in a module intended to deduplicate code is counterproductive.
| return pathname.slice(basePath.length) || "/"; | ||
| } | ||
| return pathname; | ||
| } |
There was a problem hiding this comment.
stripBasePath is exported but never imported anywhere in the codebase. The inline pathname.slice(basePath.length) || "/" pattern is still used directly in app-dev-server.ts (line 1141) and index.ts. Same concern as guardProtocolRelativeUrl — either wire it up or defer to a follow-up PR.
| }); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
normalizeTrailingSlash is exported but never called. The trailing slash logic is still inlined in app-dev-server.ts at lines 1146-1153. Same issue.
| for (const key of keysToDelete) { | ||
| headers.delete(key); | ||
| } | ||
| } |
There was a problem hiding this comment.
processMiddlewareHeaders is exported but never imported. The middleware header unpacking is still done inline in app-dev-server.ts at lines 1260-1267. This is the fourth unused function in this module.
To summarize: only validateCsrfOrigin and validateImageUrl from this file are actually wired up. The other four functions are dead code.
| } | ||
| } | ||
There was a problem hiding this comment.
Stray extra blank lines left behind from removing the inline functions. Two consecutive blank lines here; please collapse to one.
| }); | ||
| return new Response(upstream.body, { status: upstream.status, statusText: upstream.statusText, headers: respHeaders }); | ||
| } | ||
There was a problem hiding this comment.
Same issue — two consecutive blank lines left over from the removal of __proxyExternalRequest and __applyConfigHeaders. Please collapse.
| // Validate the constructed URL's origin hasn't changed (defense in depth). | ||
| const url = new URL(requestUrl); | ||
| const resolvedImg = new URL(imgUrl, url.origin); | ||
| if (resolvedImg.origin !== url.origin) { | ||
| return new Response("Only relative URLs allowed", { status: 400 }); | ||
| } | ||
| return imgUrl; |
There was a problem hiding this comment.
The defense-in-depth origin check is tautological for all inputs that pass the guard above (must start with / but not //). Resolving a root-relative path against url.origin will always produce the same origin. This was also true in the original inline code, but since this is now a standalone function with its own documentation, it would be helpful to add a comment explaining that this check exists as a safety net against future changes to the guard logic, not because it catches anything today.
| import { safeJsonStringify } from "vinext/html"; | ||
| import { getSSRFontLinks as _getSSRFontLinks, getSSRFontStyles as _getSSRFontStylesGoogle, getSSRFontPreloads as _getSSRFontPreloadsGoogle } from "next/font/google"; | ||
| import { getSSRFontStyles as _getSSRFontStylesLocal, getSSRFontPreloads as _getSSRFontPreloadsLocal } from "next/font/local"; | ||
| import { parseCookies } from ${JSON.stringify(path.resolve(__dirname, "config/config-matchers.js").replace(/\\/g, "/"))}; |
There was a problem hiding this comment.
This import path uses path.resolve(__dirname, ...) which produces a platform-dependent absolute path. The existing pattern in the file already does this for other generated imports, so it's consistent. Just noting that this follows the existing convention.
|
Review submitted with REQUEST_CHANGES status. Here's a summary of the key findings: The PR's direction is sound — deduplicating ~250 lines of inline Issues flagged:
Correctness is solid for the parts that are actually wired up: |
…quest-pipeline Deduplicate ~250 lines of inline request-handling logic from the generated App Router RSC entry (app-dev-server.ts) and Pages Router server entry (index.ts) by importing from the canonical config-matchers.ts and a new request-pipeline.ts module. Changes: - Create server/request-pipeline.ts with shared utilities: guardProtocolRelativeUrl, stripBasePath, normalizeTrailingSlash, validateCsrfOrigin, isOriginAllowed, validateImageUrl, processMiddlewareHeaders - app-dev-server.ts: replace 14 inline __-prefixed functions with imports from config-matchers.ts (matchRedirect, matchRewrite, matchHeaders, isExternalUrl, proxyExternalRequest, sanitizeDestination, requestContextFromRequest) and request-pipeline.ts (validateCsrfOrigin, validateImageUrl) - index.ts: replace inline parseCookies, sanitizeDestinationLocal, and matchConfigPattern with imports/re-exports from config-matchers.ts; replace manual RequestContext construction with requestContextFromRequest() - Update tests to reference imported function names instead of old inline __-prefixed names Security fixes: - matchConfigPattern: eliminates chained .replace() divergence (CodeQL flagged) in favor of config-matchers single-pass tokenizer - proxyExternalRequest: uses AbortController+setTimeout (broader runtime support) instead of AbortSignal.timeout() - escapeHeaderSource: uses proper config-matchers implementation instead of simplified chained .replace() Closes cloudflare#254 t status
yunus25jmi1
force-pushed
the
refactor/shared-request-handling
branch
from
6624adc to
439ebd3
Compare
|
@james-elicx Re-review the changes. Kindly merge the PR. |
2 participants
Summary
Deduplicates ~250 lines of inline request-handling logic from the generated App Router RSC entry (\�pp-dev-server.ts) and Pages Router server entry (\index.ts) by importing from the canonical \config-matchers.ts\ and a new
equest-pipeline.ts\ module.
Changes
New file: \server/request-pipeline.ts\
Shared request lifecycle utilities extracted for reuse across entry points:
ormalizeTrailingSlash()\ — redirects to canonical trailing-slash form
\�pp-dev-server.ts\ (~250 lines removed)
Replaced 14 inline __-prefixed functions with imports from \config-matchers.ts\ and
equest-pipeline.ts:
equestContextFromRequest\
\index.ts\
equestContextFromRequest()\
Test updates
Updated \�pp-router.test.ts\ assertions to reference the new imported function names instead of old inline __-prefixed names.
Security Fixes
Verification
Closes #254