fix: eliminate double middleware execution in hybrid app+pages dev mode#541
Conversation
commit: |
|
/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: fix double middleware execution in hybrid app+pages dev mode
This is a well-designed fix for a real correctness bug. The approach — forwarding middleware results via a request header rather than re-executing — is the right call. The production guard, the comment explaining why, and the regression test converting test.fixme to a live assertion are all appreciated.
I have a few observations, mostly about code hygiene and a minor robustness concern. Nothing blocking.
What looks good
- Forwarding over skipping is the right design. The RSC entry genuinely needs
_mwCtx.headersforapplyMiddlewareRequestHeaders(), so simply suppressing the second run would breakheaders()in Server Components. - Production guard (
NODE_ENV !== "production") prevents trusting attacker-supplied headers in deployed apps. The connect handler is dev-only, so this is correct. - Deferred response headers for hybrid mode avoid double-applying custom headers (once from the connect handler, once from the RSC entry). The six application sites all correctly guard with
if (deferredMwResponseHeaders). __mwCtxAppliedflag falls back cleanly to re-running middleware if parsing fails.- Test update is clean — the
test.fixmetotestconversion with updated comments accurately describes the new behavior. - Header forwarding also correctly covers the "middleware didn't match" case — the connect handler still sets
x-vinext-mw-ctxwith empty data ({"h":[],"s":null,"r":null}), which causes the RSC entry to skip its own matcher evaluation. Both agree the path doesn't match, so this avoids redundant work.
| cleanPathname = __rewriteParsed.pathname; | ||
| url.search = __rewriteParsed.search; | ||
| } | ||
| __mwCtxApplied = true; |
There was a problem hiding this comment.
Nit: __mwCtxApplied = true is inside the try block, which means if anything after JSON.parse succeeds but then throws (e.g., new URL(__mwCtxData.r, request.url) with a malformed rewrite URL at line 1600), the flag won't be set and the RSC entry will fall through to re-execute middleware.
This is arguably the correct behavior — if the forwarded context is broken, re-running middleware is a reasonable safety net. But it means a corrupt r value causes both a console.error and a redundant middleware execution, which could be confusing during debugging.
If the "re-execute on any error" fallback is intentional, a one-line comment clarifying that would help future readers. If not, consider setting the flag right after JSON.parse succeeds and handling the rewrite URL separately.
| const __mwCtxHeader = request.headers.get("x-vinext-mw-ctx"); | ||
| if (__mwCtxHeader) { | ||
| try { | ||
| const __mwCtxData = JSON.parse(__mwCtxHeader); |
There was a problem hiding this comment.
Minor: The header value is parsed via JSON.parse with no size guard. In dev, the connect handler controls this value so it's safe. But if someone's middleware sets many/large headers (e.g., big Set-Cookie values), the JSON-serialized header could approach Node.js's default ~16KB per-header limit (--max-http-header-size), causing silent truncation and a parse error here.
Unlikely in practice and dev-only, so not blocking — but maybe worth a note.
|
|
||
| // External rewrite from beforeFiles — proxy to external URL | ||
| if (isExternalUrl(resolvedUrl)) { | ||
| if (deferredMwResponseHeaders) { | ||
| for (const [key, value] of deferredMwResponseHeaders) { |
There was a problem hiding this comment.
The deferredMwResponseHeaders application pattern (check + loop + appendHeader) is duplicated 6 times across the connect handler. Consider extracting a helper at the top of the handler scope:
const applyDeferredMwHeaders = () => {
if (deferredMwResponseHeaders) {
for (const [key, value] of deferredMwResponseHeaders) {
res.appendHeader(key, value);
}
}
};This would reduce ~30 lines of repetition and make it easier to modify the behavior later (e.g., if you need to null out deferredMwResponseHeaders after first application to prevent double-append on some edge path). Not blocking.
| } | ||
| } | ||
|
|
||
| // Build request context once for has/missing condition checks |
There was a problem hiding this comment.
Confirmed: x-vinext-mw-ctx is always set when hasAppDir && middlewarePath, even when runMiddleware returned { continue: true } with no responseHeaders (matcher didn't match). The payload {"h":[],"s":null,"r":null} correctly causes the RSC entry to set __mwCtxApplied = true and skip re-running middleware. This is the right behavior — the connect handler already evaluated the matcher.
|
The review was submitted successfully. Here's a summary of what I posted: Overall assessment: This is a well-designed fix. The "forwarding over skipping" approach is correct — the RSC entry needs middleware context for Inline comments posted (4):
|
In hybrid app+pages dev setups, middleware ran twice per App Router request — once in the connect handler (SSR env) and again in the RSC entry (RSC env). This caused double side effects for auth checks, logging, analytics, and any middleware state mutations. The connect handler now forwards its middleware results to the RSC entry via an x-vinext-mw-ctx request header containing the response headers, rewrite URL, and rewrite status. The RSC entry reconstructs _mwCtx from the forwarded data instead of re-executing the middleware function. Key details: - Response headers are deferred (not applied to res) in the connect handler when hasAppDir is true, avoiding duplicates. They're applied to res only for Pages routes handled directly by the connect handler. - The RSC plugin constructs its Request from the original HTTP request, not from req.url, so middleware rewrites must be forwarded explicitly (the connect handler's req.url rewrite is invisible to the RSC entry). - Pure App Router (no pages/) and pure Pages Router (no app/) setups are unaffected — they already use a single middleware execution path.
- Guard x-vinext-mw-ctx check with NODE_ENV !== "production" so the
forwarding header cannot be exploited in production builds
- Replace bare catch {} with error logging and fallback to running
middleware normally when the forwarded context is malformed
- Filter x-middleware-next and x-middleware-rewrite from forwarded
header entries to match the RSC entry's inline filtering behavior
- Apply deferred middleware response headers before all three
proxyExternalRewriteNode call sites (beforeFiles, afterFiles,
fallback external rewrites)
- Extract applyDeferredMwHeaders() helper to deduplicate 6 identical inline blocks in the connect handler - Add comment clarifying that __mwCtxApplied flag placement inside try block is intentional — falls back to re-running middleware on error
NathanDrake2406
force-pushed
the
fix/double-middleware-execution
branch
from
9f60eda to
7c18ace
Compare
|
/bigbonk re-review |
There was a problem hiding this comment.
Review: fix double middleware execution in hybrid app+pages dev mode
This is a correct and well-structured fix for a real bug. The approach — forwarding middleware results via x-vinext-mw-ctx rather than re-executing or skipping — is the right design choice. The RSC entry needs the full middleware context (_mwCtx.headers for applyMiddlewareRequestHeaders(), rewrite URL for routing, custom status), so a forwarding mechanism is the only option that preserves correctness.
I verified:
- Early-return paths are correct. Redirects and custom responses (
!result.continue) return before the forwarding header is set — the connect handler handles those fully, RSC entry is never invoked. next()paths correctly skipapplyDeferredMwHeaders(). All threereturn next()calls (lines 2476, 2543, 2556) hand off to the RSC entry, which handles response headers from the forwarded context. Deferred headers are only flushed for Pages-handled responses.- Header filtering is consistent and intentional.
deferredMwResponseHeadersstrips allx-middleware-*(correct for HTTP response), whilemwCtxEntriesonly stripsx-middleware-nextandx-middleware-rewrite— the RSC entry needsx-middleware-request-*forapplyMiddlewareRequestHeaders(). - Production guard is correct.
process.env.NODE_ENV !== "production"ensures the header is only trusted in dev where the connect handler is the sole producer. applyDeferredMwHeadershelper correctly extracted by the author (matching the earlier review suggestion), avoiding repetition across 6 call sites.- Test conversion is clean. The
test.fixme→testchange with updated comments accurately describes the fix.
Three minor observations below, none blocking. Approving.
| const __mwCtxData = JSON.parse(__mwCtxHeader); | ||
| if (__mwCtxData.h && __mwCtxData.h.length > 0) { | ||
| // Note: h may include x-middleware-request-* internal headers so | ||
| // applyMiddlewareRequestHeaders() can unpack them below. | ||
| // processMiddlewareHeaders() strips them before any response. | ||
| _mwCtx.headers = new Headers(); | ||
| for (const [key, value] of __mwCtxData.h) { | ||
| _mwCtx.headers.append(key, value); | ||
| } | ||
| } | ||
| if (__mwCtxData.s != null) { | ||
| _mwCtx.status = __mwCtxData.s; | ||
| } | ||
| // Apply forwarded middleware rewrite so routing uses the rewritten path. | ||
| // The RSC plugin constructs its Request from the original HTTP request, | ||
| // not from req.url, so the connect handler's req.url rewrite is invisible. | ||
| if (__mwCtxData.r) { | ||
| const __rewriteParsed = new URL(__mwCtxData.r, request.url); | ||
| cleanPathname = __rewriteParsed.pathname; | ||
| url.search = __rewriteParsed.search; | ||
| } | ||
| // Flag set after full context application — if any step fails (e.g. malformed | ||
| // rewrite URL), we fall back to re-running middleware as a safety net. | ||
| __mwCtxApplied = true; | ||
| } catch (e) { | ||
| console.error("[vinext] Failed to parse forwarded middleware context:", e); | ||
| } |
There was a problem hiding this comment.
Nit: __mwCtxApplied = true at line 1606 is inside the try block after all context application steps. This means new URL(__mwCtxData.r, request.url) throwing on a malformed rewrite URL causes both a console.error and a fallback to re-executing middleware — which could be confusing during debugging since the JSON was valid (the connect handler did run middleware).
Consider setting the flag right after JSON.parse succeeds, and wrapping the rewrite URL application in its own try/catch. That way: valid JSON = trust it, bad rewrite URL = log clearly but don't re-execute:
| const __mwCtxData = JSON.parse(__mwCtxHeader); | |
| if (__mwCtxData.h && __mwCtxData.h.length > 0) { | |
| // Note: h may include x-middleware-request-* internal headers so | |
| // applyMiddlewareRequestHeaders() can unpack them below. | |
| // processMiddlewareHeaders() strips them before any response. | |
| _mwCtx.headers = new Headers(); | |
| for (const [key, value] of __mwCtxData.h) { | |
| _mwCtx.headers.append(key, value); | |
| } | |
| } | |
| if (__mwCtxData.s != null) { | |
| _mwCtx.status = __mwCtxData.s; | |
| } | |
| // Apply forwarded middleware rewrite so routing uses the rewritten path. | |
| // The RSC plugin constructs its Request from the original HTTP request, | |
| // not from req.url, so the connect handler's req.url rewrite is invisible. | |
| if (__mwCtxData.r) { | |
| const __rewriteParsed = new URL(__mwCtxData.r, request.url); | |
| cleanPathname = __rewriteParsed.pathname; | |
| url.search = __rewriteParsed.search; | |
| } | |
| // Flag set after full context application — if any step fails (e.g. malformed | |
| // rewrite URL), we fall back to re-running middleware as a safety net. | |
| __mwCtxApplied = true; | |
| } catch (e) { | |
| console.error("[vinext] Failed to parse forwarded middleware context:", e); | |
| } | |
| const __mwCtxData = JSON.parse(__mwCtxHeader); | |
| __mwCtxApplied = true; | |
| if (__mwCtxData.h && __mwCtxData.h.length > 0) { | |
| // Note: h may include x-middleware-request-* internal headers so | |
| // applyMiddlewareRequestHeaders() can unpack them below. | |
| // processMiddlewareHeaders() strips them before any response. | |
| _mwCtx.headers = new Headers(); | |
| for (const [key, value] of __mwCtxData.h) { | |
| _mwCtx.headers.append(key, value); | |
| } | |
| } | |
| if (__mwCtxData.s != null) { | |
| _mwCtx.status = __mwCtxData.s; | |
| } | |
| // Apply forwarded middleware rewrite so routing uses the rewritten path. | |
| // The RSC plugin constructs its Request from the original HTTP request, | |
| // not from req.url, so the connect handler's req.url rewrite is invisible. | |
| if (__mwCtxData.r) { | |
| try { | |
| const __rewriteParsed = new URL(__mwCtxData.r, request.url); | |
| cleanPathname = __rewriteParsed.pathname; | |
| url.search = __rewriteParsed.search; | |
| } catch (e) { | |
| console.error("[vinext] Failed to apply forwarded middleware rewrite URL:", e); | |
| } | |
| } |
Not blocking — the current behavior is safe (falls back to re-running), just potentially confusing in debug scenarios.
| } | ||
| } | ||
| } | ||
| req.headers["x-vinext-mw-ctx"] = JSON.stringify({ |
There was a problem hiding this comment.
Minor defensive thought: if the serialized JSON for x-vinext-mw-ctx ever gets large (many middleware-set headers, large cookie values), it could approach Node's default --max-http-header-size limit (16KB). In dev this is unlikely to be a practical issue, but if it ever surfaces, the symptom would be a silent parse error in the RSC entry followed by a redundant middleware re-execution — which at least fails safe. No action needed, just flagging for awareness.
| for (const [key, value] of result.responseHeaders) { | ||
| if (!key.startsWith("x-middleware-")) { | ||
| res.appendHeader(key, value); | ||
| if (hasAppDir) { |
There was a problem hiding this comment.
Observation: When hasAppDir is true and a request ends up hitting a Pages route (e.g., API route at line 2466), applyDeferredMwHeaders() applies the response headers to res, and applyRequestHeadersToNodeRequest() applies the request headers to req.headers. The forwarded x-vinext-mw-ctx header is also on req.headers but is never consumed by the Pages handler — it just passes through harmlessly.
This is correct behavior, just noting for future readers: in the hybrid case, Pages routes get their response headers from deferredMwResponseHeaders, while App Router routes get them from x-vinext-mw-ctx via the RSC entry. Two paths, same result.
|
Review submitted successfully. Here's a summary of what I posted: Verdict: Approved This is a correct and well-designed fix for a real double-execution bug in hybrid app+pages dev mode. The "forwarding over skipping" approach is the right call — the RSC entry genuinely needs the middleware context for Key things I verified:
3 inline comments posted (none blocking):
|
2 participants
Summary
x-vinext-mw-ctxrequest header. The RSC entry reconstructs_mwCtxfrom the forwarded data instead of re-executing the middleware function.pages/) and pure Pages Router (noapp/) setups are unaffected — they already use a single middleware execution path.Key design decisions
_mwCtx.headersforapplyMiddlewareRequestHeaders()(soheaders()in Server Components sees middleware-modified headers). Simply skipping middleware would break this.req.url, so middleware rewrites must be forwarded explicitly in the header payload.hasAppDiris true, middleware response headers are deferred (not applied tores) to avoid duplicates. They're applied only for Pages routes handled directly by the connect handler.x-vinext-mw-ctxcheck is guarded byNODE_ENV !== "production"since the forwarding mechanism is dev-only. In production there is no connect handler, so an attacker-supplied header must not be trusted.Test plan
test.fixmeto live regression test — verifiesmiddlewareInvocationCount === 1for App Router requests in hybrid fixture