fix(image): preserve middleware ordering for source requests#2554
fix(image): preserve middleware ordering for source requests#2554james-elicx wants to merge 1 commit into
Conversation
|
/bigbonk review for issues |
commit: |
|
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(image): preserve middleware ordering for source requests
Routing image-source requests through the app pipeline so middleware observes them is a solid parity improvement and matches Next.js (fetchInternalImage dispatches through the normal handler). The unit + e2e coverage is good. One regression and a few things worth confirming.
🔴 App Router prod: public-file serving loses static cache, precompressed variants, and ETag/304
In startAppRouterServer, the general request path previously served VINEXT_STATIC_FILE_HEADER-signaled files (i.e. public/ directory files, which "fall through to the RSC handler" per the comment at prod-server.ts:1425-1427) via:
await tryServeStatic(req, res, clientDir, staticFilePath, compress, staticCache, staticResponseHeaders, response.status)The PR replaces this with dispatchAppRequest → createStaticFileResponse (prod-server.ts:1366-1374, :809). createStaticFileResponse does not take staticCache and reimplements a stripped-down slow path. Compared to tryServeStatic, every public-file response now:
- Bypasses
StaticFileCache— doesresolveStaticFile+fsp.readFileon every request instead of the precomputed in-memory lookup. - Loses precompressed
.br/.gz/.zstvariants — on-the-fly gzip viasendWebResponseis the only compression left (more CPU, and no brotli/zstd). - Loses ETag /
If-None-Match→ 304 — public files now always transfer the full body even when the client holds a valid cached copy.
This isn't image-specific: dispatchAppRequest is the main App Router request handler, so this affects every public/ file served by the Node production server. (Cache-Control lifetime is unchanged — both paths use max-age=3600 for non-hashed files — so this is a performance/bandwidth regression, not a cache-correctness one.)
Suggestion: keep the general App Router static path on tryServeStatic(..., compress, staticCache, ...). If you need a Web-Response-returning helper for the image dispatch flow specifically, have it consult staticCache (and honor precompressed variants + ETag) rather than re-reading from disk, or only use createStaticFileResponse for the image-source dispatch and leave the general path on tryServeStatic.
Note: the Pages Router main path still uses tryServeStatic (prod-server.ts:1933); dispatchPagesRequest/createStaticFileResponse there is scoped to image-source fetches only, which are re-served with image cache headers anyway — so the Pages side is fine.
🟡 Non-ok source responses are returned raw (no security headers, redirects pass through)
handleImageOptimization now returns the source response directly when !source.ok (image-optimization.ts:330-332). This is correct and intended for the middleware-401 case the test covers. Two things to confirm against Next.js:
- Security headers are dropped on error responses.
Content-Security-Policy/X-Content-Type-Optionsare only applied to 2xx passthrough/transform responses. A source that returns e.g. an HTML error body now reaches the client withoutX-Content-Type-Options: nosniff. Worth confirming Next.js also forwards error bodies verbatim here (it largely does), but a defense-in-depthnosniffon all image-endpoint responses is cheap. - 3xx sources pass through as redirects.
Response.okis false for 3xx, so a middleware/source redirect on the image-source path now surfaces as a redirect from/_next/image. The e2e test assertslocationis undefined for the 401 case, but there's no test for a source that redirects. Please confirm this matches Next.js behavior (and add a test if the intended behavior is "don't emit a redirect from the optimizer").
🟢 Minor
createStaticFileResponseusesappendforextraHeadersmerge buttryServeStaticused spread/override semantics ({ ...variant.headers, ...extraHeaders }). For the staged/middleware headers passed here that's probably intended, but double-check you don't end up with duplicateContent-Type/Cache-Control— those aresetafterward so they're fine; just flagging the merge-strategy difference.createInternalImageRequestrecursion guard + header preservation is nicely tested. 👍
Verified locally
vp checkon all changed server files — clean.vp test run tests/shims.test.ts -t "handleImageOptimization|recursive image optimizer"— 27 passed.vp test run tests/deploy.test.ts -t "fetchWorkerFilesystemRoute"— 6 passed.
The middleware-ordering fix itself is correct; the main blocker is the App Router public-file static-serving regression.
|
Review posted to PR #2554. Summary of findings: Review Summary1 regression (🔴), 2 items to confirm (🟡), minor notes (🟢) 🔴 App Router prod public-file serving regressionThe PR replaces
Cache lifetime is unchanged ( 🟡 Non-ok source responses returned raw
🟢 The core fix is soundThe middleware-ordering change matches Next.js's All changed files pass |
james-elicx
force-pushed
the
codex/fix-image-middleware-ordering
branch
from
ae8d64d to
66ac5d8
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(image): preserve middleware ordering for source requests
The core direction is right — routing local image-source requests through the app pipeline so middleware observes them matches Next.js's fetchInternalImage, which dispatches the internal source request through routerServerHandler (.nextjs-ref/packages/next/src/server/next-server.ts:721). The recursion guard + header preservation in createInternalImageRequest is clean and well-tested. But I verified the non-ok / error-body behavior against Next.js source and found a deliberate-but-undocumented divergence, plus a header-leak parity gap. I also want to correct one prior review finding that doesn't hold up.
🔴 Non-image / non-ok source responses diverge from Next.js (401 passthrough vs 400 "isn't a valid image")
handleImageOptimization now does if (!source.ok) return source; (image-optimization.ts:330-332), passing the middleware's status/body/headers through verbatim. The new e2e test enshrines this: a middleware 401 on /protected/private.png is asserted to surface as 401 from /_next/image (image-middleware-ordering.spec.ts:24).
Next.js does not do this. Walking the source:
fetchInternalImageonly throws when!mocked.res.statusCode(falsy) or the buffer is empty (next-server.tsvia image-optimizer.ts:1008-1022). A 401 is truthy with a non-empty body, so it is not an error there — the "Authentication required" buffer flows intoimageOptimizer.imageOptimizerrunsdetectContentTypeon that buffer; a text/HTML error body failsupstreamType.startsWith('image/')and throwsImageError(400, "The requested resource isn't a valid image.")(image-optimizer.ts:1104-1117).
Next.js has an explicit test for exactly this shape — a non-image source body returns 400 "The requested resource isn't a valid image.", regardless of the source's own status (.nextjs-ref/test/e2e/image-optimizer/util.ts:1544-1549).
So for a middleware-blocked source, Next.js returns 400, not the raw 401. vinext's previous behavior (404) was also wrong, but this PR swaps one divergence for another and locks it in with a test asserting the divergent status.
Please confirm the intended behavior against Next.js:
- If the goal is Next.js parity, non-image / blocked sources should return
400 "The requested resource isn't a valid image."and the e2e test should assert 400, not 401. - If surfacing the middleware status verbatim is a deliberate, better-than-Next.js choice, that's defensible — but per AGENTS.md it must be a documented divergence (comment + PR note linking the Next.js test), not an accidental one. Right now the code comment ("preserves request headers for middleware") describes the request side, not this response-side behavior.
🟡 Staged middleware headers leak onto the optimized image response (parity gap)
On the success path, createStaticFileResponse appends stagedHeaders (e.g. x-custom-middleware, x-mw-pathname from the fixture middleware) onto the source Response (prod-server.ts:832-838, :1719), and then createPassthroughImageResponse copies all source.headers onto the final /_next/image response (image-optimization.ts:266-271).
Next.js's fetchInternalImage extracts only Content-Type, Cache-Control, and ETag from the internal response (image-optimizer.ts:1024-1027) — it does not forward arbitrary middleware headers to the image response. So this PR causes middleware/staged headers to leak onto image responses where Next.js would not emit them. Worth trimming the passthrough to the content/cache/etag set (or stripping staged headers before passthrough) to match.
🟢 Correction to the prior review's "App Router public-file regression"
The earlier review flagged that the general App Router prod request path was switched from tryServeStatic(...) to dispatchAppRequest → createStaticFileResponse, losing StaticFileCache, precompressed variants, and ETag/304 for all public/ files. I could not reproduce that. The general App Router path still uses tryServeStatic(req, res, clientDir, staticFilePath, compress, staticCache, staticResponseHeaders, response.status) (prod-server.ts:1502-1511). dispatchAppRequest/createStaticFileResponse is invoked only from inside the image-optimization branch (prod-server.ts:1468) to resolve image sources — which are re-served with image cache headers anyway. So there is no general public-file static-serving regression; that finding is a false positive.
(One real consequence remains: a public/ file fetched as an image source is read from disk via createStaticFileResponse rather than the in-memory StaticFileCache, and precompressed variants/ETag aren't used for that intermediate read. That's a minor per-image-source cost, not a general regression, since the final image response replaces those headers.)
🟢 Minor
createStaticFileResponseusesheaders.appendforextraHeaders;Content-Type/Content-Length/Cache-Controlaresetafterward so no duplication there — fine.createInternalImageRequestrecursion guard (base-path-awareisImageOptimizationPathcheck) + header/GET-coercion is nicely tested. 👍
Verified locally
.nextjs-refbehavior traced throughfetchInternalImage→imageOptimizer→detectContentType, and confirmed againsttest/e2e/image-optimizer/util.ts(non-image source → 400).vitest run tests/shims.test.ts -t "handleImageOptimization|recursive image optimizer"— 27 passed.
The middleware-ordering mechanism is a solid parity improvement; the main thing to resolve before merge is the non-ok source status semantics (401 passthrough vs Next.js's 400) and confirming/documenting whichever behavior you intend.
|
Review posted to PR #2554. Summary1 blocker (🔴), 1 parity gap (🟡), plus a correction of the prior review + minor notes (🟢) 🔴 Non-ok source responses diverge from Next.jsThe PR's 🟡 Staged middleware headers leak onto image responses
🟢 Corrected the prior review's headline "regression"The earlier ask-bonk review claimed the general App Router prod path was switched off The middleware-ordering mechanism itself ( |
james-elicx
force-pushed
the
codex/fix-image-middleware-ordering
branch
from
66ac5d8 to
0454301
Compare
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
james-elicx
force-pushed
the
codex/fix-image-middleware-ordering
branch
from
0454301 to
8902280
Compare
There was a problem hiding this comment.
Review: fix(image): preserve middleware ordering for source requests
Re-reviewed the current head (0454301). This revision resolves both blockers from the earlier ask-bonk reviews, and I verified the fixes against the code and tests. The core direction — routing local image-source requests through the app pipeline so middleware observes them — matches Next.js's fetchInternalImage, and the recursion guard + credential-free GET in createInternalImageRequest is clean and well-tested.
✅ Prior blocker #1 (401 vs 400) — fixed
The earlier if (!source.ok) return source; is gone. handleImageOptimization now buffers the source via readImageSource, sniffs the content type by magic bytes, and returns 400 "The requested resource isn't a valid image." when no image type is detected (image-optimization.ts:462-468). The e2e test asserts 400, not 401 (image-middleware-ordering.spec.ts:24). This matches Next.js: a non-image body (e.g. a middleware "Authentication required" 401 body) flows into the optimizer, fails detectContentType, and yields 400. New unit tests lock this in: validates non-success internal source bodies as images, rejects non-success bodies that spoof an image content type, rejects bodyless non-success internal source responses.
✅ Prior blocker #2 (staged/middleware header leak) — fixed
createPassthroughImageResponse now builds a fresh Headers and copies only Content-Type / ETag / Cache-Control / Vary + security headers (image-optimization.ts:364-383), rather than cloning source.headers. So staged middleware headers (x-custom-middleware, x-mw-pathname) and stray Set-Cookie/Location no longer reach the /_next/image response. Directly tested: the passthrough test asserts x-middleware-header: must-not-leak is stripped, the transform test asserts Set-Cookie/Location: /must-not-leak are dropped, and the e2e test asserts x-mw-pathname is undefined.
🟢 Other observations (non-blocking)
Cache-Controlbehavior change is a parity improvement, not a regression. Optimized images move frompublic, max-age=31536000, immutableto the computedimageCacheControl()value:public, max-age=max(minimumCacheTTL ?? 14400, upstreamMaxAge), must-revalidate(image-optimization.ts:385-393). This matches Next.js'smax-age=<ttl>, must-revalidateshape (main was incorrectly serving immutable).- Source is now fully buffered in memory on every image request, capped by
maximumResponseBody(default 50MB). This is required for magic-byte content sniffing and matches Next.js'sfetchInternalImagebuffering + body-size limit, so it's intentional parity — worth being aware of vs. main's streaming passthrough. - Dead export:
IMAGE_CACHE_CONTROL(image-optimization.ts:209) is now unused everywhere (source + tests) after the switch toimageCacheControl(). Its value was also updated for consistency but nothing reads it. Consider removing it (or dropping theexport) to avoid a stale public constant. - Fallback path is noisy but correct. When a transform consumes
source.bodyand the catch-block passthrough throws "body … disturbed or locked", the code refetches (image-optimization.ts:526-551). Tests pass, but sincereadImageSourcealready materializedbytes, this fallback could reuse the buffer instead of refetching + re-sniffing. Pre-existing shape; optional cleanup. - Dev parity note (pre-existing, not introduced here): the dev server still 302-redirects
/_next/imageto the raw asset (index.ts:4684-4703) rather than running the optimizer pipeline, so the middleware-ordering behavior this PR adds is a prod-path fix. The browser's follow-up request to the source path still hits dev middleware, so functional parity holds, but dev doesn't exercise the newcreateInternalImageRequest/dispatch flow.
Cross-seam consistency ✔
The middleware-ordering mechanism is applied consistently across App Router prod (dispatchAppRequest), Pages prod (dispatchPagesRequest), App worker (app-router-entry.ts), and Pages worker (pages-router-entry.ts), with hashed _next/static assets correctly bypassing the pipeline via resolveAppRouterAssetPath/isNextStaticPath/fetchWorkerFilesystemRoute(allowDirect).
Verified locally
npx tsc --noEmit -p packages/vinext/tsconfig.json— clean (exit 0).vitest run tests/shims.test.ts -t "handleImageOptimization|recursive image optimizer|createInternalImageRequest|isImageOptimizationPath"— 41 passed.vitest run tests/deploy.test.ts -t "fetchWorkerFilesystemRoute"— 6 passed.
No blockers. The two earlier findings are resolved and well-covered; remaining notes are minor (dead IMAGE_CACHE_CONTROL export is the only concrete cleanup).
|
Review posted to PR #2554. SummaryI re-reviewed PR #2554 at its current head ( No blockers found. Findings: ✅ Prior blocker #1 (401 vs 400) — fixed. The ✅ Prior blocker #2 (header leak) — fixed. 🟢 Non-blocking notes:
Verified locally: |
|
/bigbonk review for issues |
james-elicx
force-pushed
the
codex/fix-image-middleware-ordering
branch
from
8902280 to
812ea1e
Compare
There was a problem hiding this comment.
Review: fix(image): preserve middleware ordering for source requests
Re-reviewed the current head (812ea1e). The core mechanism — routing local image-source requests through the app pipeline so middleware observes them — matches Next.js's fetchInternalImage, which dispatches the internal source request through the normal request handler (packages/next/src/server/image-optimizer.ts → fetchInternalImage). The recursion guard + credential-free GET in createInternalImageRequest is clean and well-tested.
I independently confirmed both prior ask-bonk blockers are resolved and traced the Next.js source to verify the fixes and look for remaining gaps.
✅ Prior blockers — verified fixed
- 401→400 (non-image body):
if (!source.ok) return source;is gone. The source is buffered viareadImageSource, magic-byte sniffed, and a non-image body returns400 "The requested resource isn't a valid image."(image-optimization.ts:460-466). This matches Next.js exactly:imageOptimizerrunsdetectContentTypeand throwsImageError(400, "The requested resource isn't a valid image.")when!upstreamType.startsWith('image/'). The e2e test asserts 400. - Header leak:
createPassthroughImageResponsenow builds freshHeadersand copies only Content-Type / ETag / Cache-Control / Vary + security headers (image-optimization.ts:362-381). Tests assertx-middleware-header,Set-Cookie,Location, andx-mw-pathnamedon't leak. This is actually stricter than Next.js, which forwards Content-Type/Cache-Control/ETag — vinext's allowlist is a superset of Next's and drops everything else. Good.
🟡 Cache-Control diverges from Next.js getMaxAge in two ways
imageCacheControl (image-optimization.ts:383-391) differs from Next.js's getMaxAge / setResponseHeaders:
-
s-maxageprecedence. Next.js'sgetMaxAgedoesmap.get('s-maxage') || map.get('max-age')—s-maxagealways wins when present. The vinext regex/(?:^|,)\s*(?:s-maxage|max-age)=(\d+)/imatches whichever directive appears first in the string. For an upstreamCache-Control: max-age=60, s-maxage=120, vinext computes60where Next.js computes120. Minor, but a real parity gap for sources that set both. -
immutablepassthrough.imageCacheControlreturns the upstreamCache-Controlverbatim when it containsimmutable(line 385). Next.js never does this for the optimized response — it emitspublic, max-age=<ttl>, must-revalidatefor non-static images, and only usespublic, max-age=315360000, immutablefor the specific_next/static/media/_next/static/immutable/mediastatic-import case (keyed on theisStaticURL prefix, not on the upstream header). Preserving immutable for any upstream that declares it is a reasonable approximation of the static-media case, but it's broader than Next.js and header-driven rather than URL-driven. Worth a comment documenting the intent, or narrowing to the_next/static/mediaprefix to match Next.js'sisStaticcheck.
Neither is a blocker; both are parity nuances. Per AGENTS.md, if the divergence is intentional it should carry a comment.
🟡 Transform-failure fallback re-runs the full request pipeline (middleware side effects fire twice)
This PR changes fetchAsset so the source resolves through the entire app pipeline (middleware included). The transform-failure fallback (image-optimization.ts:522-549) calls handlers.fetchAsset(imageUrl, request) a second time when the first passthrough throws (source.body already consumed/locked by transformImage). I confirmed this path via the shims tests — stderr shows refetching source image firing on the "transform failed after consuming stream" cases.
Before this PR, fetchAsset was a plain ASSETS/filesystem read, so a double-fetch was cheap and side-effect-free. Now it re-dispatches through middleware, so any middleware side effect (logging, rate-limit counters, waitUntil work, auth checks with logging) runs twice for a single /_next/image request whenever a configured optimizer errors. Next.js avoids this entirely: it buffers the upstream once (fetchInternalImage → Buffer.concat) and imageOptimizer falls back to upstreamBuffer in-memory on optimize failure — it never re-fetches.
readImageSource already materializes the full bytes buffer but discards it (only the derived Response is returned). Consider returning the buffer so the fallback reconstructs a fresh Response(bytes, …) instead of re-dispatching. That removes the double-middleware execution and the extra I/O, and matches Next.js's in-memory fallback semantics.
🟢 Cross-seam consistency + minor notes
- App Router worker gates image handling on
getImageOptimizer()(app-router-entry.ts:96), so with no configured optimizer,/_next/imagefalls through to the RSC handler rather than the passthrough path used by the Pages worker (pages-router-entry.ts:160) and Node prod-server (both runhandleConfiguredImageOptimizationunconditionally). This gate is pre-existing (not introduced here) but the divergence is now more visible because this PR routes the other seams through the middleware-aware dispatch. Flagging for a follow-up, not this PR. - The prior review's "dead
IMAGE_CACHE_CONTROLexport" note is now stale — the constant is already gone from source (image-optimization.ts); it only lingers indist/build output, which is expected. allowDirect/resolveDirectAssetsthreading (pages-request-pipeline.ts:63-66, pages-router-entry.ts:170,210) is correctly scoped to the internal image-source dispatch sophase === "direct"filesystem routes resolve only for image sources.- HEAD→GET coercion in
createInternalImageRequest(method: "GET") matches Next.js'sfetchInternalImage.
Verified locally
vitest run tests/shims.test.ts -t "handleImageOptimization|recursive image optimizer|createInternalImageRequest|isImageOptimizationPath"— 41 passed.vitest run tests/deploy.test.ts -t "fetchWorkerFilesystemRoute"— 6 passed.vitest run tests/image-adapters-config.test.ts— 21 passed.- Traced Next.js
image-optimizer.ts(getMaxAge,imageOptimizer,fetchInternalImage,setResponseHeaders) andimage-config.tsdefaults (minimumCacheTTL: 14400,maximumResponseBody: 50_000_000— both match the PR).
No blockers. The two earlier findings are genuinely resolved and well-covered. Remaining items are the two parity nuances in imageCacheControl and the double-middleware execution in the transform-failure fallback — the fallback one is the most worth addressing since it can double-run middleware side effects on a single request.
|
Review posted to PR #2554. SummaryI re-reviewed at head No blockers. Findings: ✅ Prior blockers confirmed fixed — 401→400 for non-image bodies (buffered + magic-byte sniffed, matches Next.js 🟡
🟡 Transform-failure fallback re-runs the full pipeline — because 🟢 Notes: App Router worker's Verified locally: 41 shims + 6 deploy + 21 image-adapters tests pass; Next.js defaults ( |
|
/bigbonk review for issues |
james-elicx
force-pushed
the
codex/fix-image-middleware-ordering
branch
from
812ea1e to
f505210
Compare
There was a problem hiding this comment.
Review: fix(image): preserve middleware ordering for source requests
Re-reviewed at the current head (f505210). Note the head advanced since the last ask-bonk review (812ea1e → f505210), and this newest revision resolves the two parity nuances and the double-middleware concern the prior review left open. I independently verified against Next.js source (.nextjs-ref, image-optimizer.ts). One small real bug remains.
✅ Prior review items — now resolved in this revision
s-maxageprecedence —imageCacheControl(image-optimization.ts:387-397) now parses directives into aMapand doesdirectives.get("s-maxage") ?? directives.get("max-age"), sos-maxagealways wins. Matches Next.jsgetMaxAge. Covered by the new testprefers s-maxage over max-age for image cache lifetime(shims.test.ts:21395).immutablepassthrough — no longer header-driven.immutableis now emitted only for the URL-keyed static-media case viasourceIsStatic(/_next/static/media/,/_next/static/immutable/media/, image-optimization.ts:470-476), matching Next.js'sisStaticURL check rather than trusting an upstream header.- Transform-failure double-middleware execution — the fallback no longer re-dispatches through the pipeline.
readImageSourcenow materializessourceBytesand the fallback reconstructsnew Response(sourceBytes.slice().buffer, …)in-memory (image-optimization.ts:541-552), so middleware side effects fire exactly once. Directly covered byreuses buffered source bytes when transform consumes the stream before failing. - 401→400 and header leak — still fixed (buffered + magic-byte sniff → 400; fresh
Headersallowlist increatePassthroughImageResponse). Confirmed against tests.
🟡 imageCacheControl produces max-age=NaN for a malformed upstream directive
imageCacheControl (image-optimization.ts:394-396) does:
const upstreamMaxAge = directives.get("s-maxage") ?? directives.get("max-age");
const maxAge = Math.max(config?.minimumCacheTTL ?? 14_400, Number(upstreamMaxAge ?? 0));
return `public, max-age=${maxAge}, must-revalidate`;If an upstream/middleware source sets a non-numeric directive value (e.g. Cache-Control: max-age=abc, or a quoted max-age="600"), Number("abc") → NaN, and Math.max(14400, NaN) → NaN, so the image response emits Cache-Control: public, max-age=NaN, must-revalidate.
Next.js's getMaxAge guards exactly this (.nextjs-ref/packages/next/src/server/image-optimizer.ts:778-791): it strips surrounding quotes and does const n = parseInt(age, 10); if (!isNaN(n)) return n; otherwise falls back to 0. vinext should do the same — fall back to 0 (letting minimumCacheTTL win) when the parse yields NaN, and ideally strip quotes to match. Verified locally: Math.max(14400, Number('abc')) → NaN. Minor (only triggers on a malformed source header), and no test covers it — worth a one-line Number.isFinite guard + a fixture.
🟢 Non-blocking notes
- App Router worker still gates on
getImageOptimizer()(app-router-entry.ts:96) while the Pages worker (pages-router-entry.ts:160) and Node prod server runhandleConfiguredImageOptimizationunconditionally (passthrough when no optimizer). This gate is pre-existing, not introduced here, but this PR makes the divergence more visible since the other seams now route through the middleware-aware dispatch. Follow-up, not this PR. sourceBytes.slice().buffer(image-optimization.ts:541) is correct —slice()returns a zero-offset full-span copy so the backingArrayBufferis safe to hand tonew Response.- Cross-seam consistency (App/Pages × Node-prod/Worker) for the
createInternalImageRequestrecursion guard + credential-free GET +allowDirect/static-bypass wiring is applied uniformly and well-tested.
Verified locally
vp checkon all 5 changed server files — clean.vitest run tests/shims.test.ts -t "handleImageOptimization|recursive image optimizer|createInternalImageRequest|isImageOptimizationPath"— 41 passed.vitest run tests/deploy.test.ts -t "fetchWorkerFilesystemRoute"— 6 passed.vitest run tests/image-adapters-config.test.ts— 21 passed.- Traced Next.js
getMaxAge(quote-strip +isNaNguard) to confirm themax-age=NaNdivergence.
No blockers. The middleware-ordering mechanism is a sound, well-tested parity improvement, and this revision closes the earlier parity gaps. The only concrete fix is the NaN guard in imageCacheControl.
|
Review posted to PR #2554. SummaryRe-reviewed at the current head ( No blockers. Findings: ✅ Prior items resolved in this revision:
🟡 One real minor bug: 🟢 Non-blocking: App Router worker's Verified locally: |
james-elicx
force-pushed
the
codex/fix-image-middleware-ordering
branch
2 times, most recently
from
faeba57 to
6fd4724
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(image): preserve middleware ordering for source requests
Reviewed at the current branch head (6fd4724, single commit, +222/-27).
Heads-up on prior reviews: the earlier ask-bonk reviews on this PR described a substantially different implementation (readImageSource buffering, magic-byte sniffing, imageCacheControl, 400 responses, sourceBytes fallback reuse, sourceIsStatic). None of that is in this branch. The branch was reset back to the original single commit, so those "both blockers fixed" conclusions do not apply to the code that is actually up for merge. This review is against what's actually here.
The core direction — routing local image-source requests through the app pipeline so middleware observes them — is correct and matches Next.js's fetchInternalImage. The createInternalImageRequest recursion guard + credential-free GET is clean and well-tested. But there are two real issues plus a cross-seam divergence.
🔴 Middleware / staged headers leak onto the optimized image response (success path)
The fixture middleware sets x-custom-middleware and x-mw-pathname on every matched response (middleware.ts:9,13), and its matcher excludes only _next — so the internal source request for /protected/private.png (and any other public/ image) runs through middleware and those headers ride back on the source response.
On the success path those source headers are then copied verbatim onto the /_next/image response in every seam:
- Node prod —
const headers = new Headers(source.headers)(prod-server.ts:1475 for App Router, :1809 for Pages) copies all loopback-response headers, then only overlays the image security headers.x-mw-pathname,x-custom-middleware, and any middlewareSet-Cookieleak through. - Workers — the source flows into
handleImageOptimization→createPassthroughImageResponse, which doesnew Headers(source.headers)(image-optimization.ts:283) and passes everything through. Same leak for the passthrough / SVG-passthrough / no-optimizer cases.
Next.js's fetchInternalImage extracts only Content-Type, Cache-Control, and ETag from the internal response — it does not forward arbitrary middleware headers to the image response. Before this PR fetchAsset was a plain ASSETS/static read with no middleware headers, so this is a new leak introduced by routing through the pipeline.
The e2e test does not catch this: it only exercises the blocked/404 branch, where source headers are intentionally dropped. There is no success-path assertion that x-mw-pathname / x-custom-middleware are absent from a successfully optimized image. Please trim the passthrough (Node + createPassthroughImageResponse) to the content/cache/etag/security allowlist and add a success-path test asserting middleware headers don't leak.
🟡 401-blocked (non-image) source returns 404, where Next.js returns 400
handleImageOptimization returns 404 "Image not found" for any !source.ok source (image-optimization.ts:334-337), and the Node path does the same (prod-server.ts:1480-1482). The new e2e test locks this in: a middleware 401 on /protected/private.png is asserted to surface as 404 from /_next/image (image-middleware-ordering.spec.ts:22).
Next.js does not do this. A 401-with-body is not treated as a fetch error by fetchInternalImage; the (non-image) body flows into imageOptimizer, detectContentType fails, and Next.js returns 400 "The requested resource isn't a valid image." So vinext returns 404 where Next.js returns 400 — an undocumented divergence that the test enshrines. Per AGENTS.md, either match Next.js (400) or make this a deliberate, documented divergence with a comment + PR note linking the Next.js test. (Note: this branch has none of the buffering/sniffing machinery the prior reviews credited with producing a 400 — the code genuinely returns 404 here.)
🟡 Transform-failure fallback re-runs the full request pipeline (middleware side effects fire twice)
Because fetchAsset now dispatches the source through the whole pipeline (Node: HTTP loopback; workers: recursive handleRequest), the transform-failure fallback at image-optimization.ts:389 calls handlers.fetchAsset(imageUrl, request) a second time when the first passthrough throws (body consumed by transformImage). That re-executes middleware side effects (logging, rate-limit counters, waitUntil, auth) twice for a single /_next/image request whenever a configured optimizer errors. Next.js buffers the upstream once and falls back in-memory — it never re-fetches. This branch has no buffered bytes to reuse, so the fallback is a genuine double-dispatch. Worth addressing given fetchAsset is now middleware-bearing.
🟢 Notes
- Node prod uses an HTTP self-fetch loopback (
fetchInternalImageSource→fetch('http://127.0.0.1:port/...')) while the workers dispatch in-process viahandleRequest. Functionally both run middleware, but the Node loopback opens a real second TCP connection per image request (extra latency + connection pressure, and a potential source of surprises behind connection limits). Flagging the architectural asymmetry; the in-process approach the workers use would be more consistent if feasible for Node. - App Router worker still gates image handling on
getImageOptimizer()(app-router-entry.ts:96), so with no configured optimizer/_next/imagefalls through to the RSC handler, unlike the Pages worker (pages-router-entry.ts) and Node prod which run unconditionally. Pre-existing, but more visible now. internalImageOriginIPv6 bracketing (features.test.ts) and thecreateInternalImageRequestrecursion guard / credential-free GET are nicely tested. 👍
Verified locally
- Confirmed branch head is
6fd4724(single commit); the diff matches+222/-27and contains none of the buffering/sniffing/imageCacheControlcode the prior reviews described. vitest run tests/shims.test.ts -t "credential-free GET|recursive local image optimizer|cancels rejected image source bodies"— 3 passed.- Traced middleware header propagation (fixture matcher excludes
_nextonly) andcreatePassthroughImageResponse/ Nodenew Headers(source.headers)header copy to confirm the success-path leak.
The middleware-ordering mechanism itself is a sound parity improvement. The header leak is the main blocker before merge; the 404-vs-400 divergence and the double-dispatch fallback are the follow-ups.
|
Review posted to PR #2554. SummaryI reviewed at the actual branch head ( Against the code that's actually here: 🔴 Blocker — Middleware/staged headers leak onto the optimized image (success path). Because the source now routes through middleware (which sets 🟡 404 vs 400 divergence. A middleware-blocked (401) non-image source returns 🟡 Transform-failure fallback double-dispatches through the middleware-bearing pipeline (no buffered bytes to reuse in this branch), running middleware side effects twice. 🟢 Notes: Node uses an HTTP self-fetch loopback while workers dispatch in-process (asymmetry); App Router worker still gates on The core middleware-ordering mechanism is sound; the header leak is the main thing to fix before merge. |
|
/bigbonk review for issues |
james-elicx
force-pushed
the
codex/fix-image-middleware-ordering
branch
from
6fd4724 to
35c46a5
Compare
There was a problem hiding this comment.
Review: fix(image): preserve middleware ordering for source requests
Reviewed at the actual branch head (35c46a5, single commit, +309/-46). Important note on prior reviews: the earlier ask-bonk reviews on this thread flip-flopped between two different implementations. Several credited a readImageSource/magic-byte-sniffing/imageCacheControl/sourceIsStatic implementation that is not in this branch; the most recent one (6fd4724) reviewed a version that still had the header leak. This branch (35c46a5) is one commit past that and fixes the header leak. This review is against the code actually up for merge.
The core mechanism — routing local image-source requests through the app pipeline so middleware observes them — is correct and matches Next.js's fetchInternalImage (dispatches the internal source through the normal request handler). Recursion guard + credential-free GET in createInternalImageRequest is clean and well-tested.
✅ Header leak (the last review's blocker) — fixed in this revision
Both seams now build fresh, allowlisted headers instead of cloning source.headers:
- Node prod (App + Pages) uses
imageSourceResponseHeaders(source)(image-optimization.ts:295-302), copying onlyContent-Type/Cache-Control/ETag, then overlaying image security headers (prod-server.ts:1470-1471, :1801-1802). The priornew Headers(source.headers)clone is gone. - Workers —
createPassthroughImageResponsebuilds a freshHeaderscopying onlyContent-Type/ETag(image-optimization.ts:283-293); the transform path derives fromtransformed.headers, not the middleware source.
Well covered: the new e2e success-path test asserts x-custom-middleware/x-mw-pathname are absent on a successfully optimized image (image-middleware-ordering.spec.ts:42-43), and the shims test asserts Set-Cookie/X-Middleware-Header don't leak (shims.test.ts). Good — this closes the one real blocker.
🟡 401-blocked source returns 404, where Next.js returns 400 (undocumented divergence)
For a middleware-blocked (401) source, handleImageOptimization (!source.ok → 404 "Image not found", image-optimization.ts:348-351) and the Node path (prod-server.ts:1472-1474, :1815-1817) return 404. The e2e test enshrines this (image-middleware-ordering.spec.ts:24).
Next.js does not treat a 401-with-body as a fetch error in fetchInternalImage; the (non-image) body flows into imageOptimizer, detectContentType fails, and Next.js returns 400 "The requested resource isn't a valid image." So vinext returns 404 where Next.js returns 400.
Two mitigating facts: (1) the 404 "Image not found" for !source.ok is pre-existing (it predates this PR — it was the genuine "source missing" path); this PR only makes middleware-blocked sources reachable by it. (2) This branch has no body buffering/sniffing, so it can't reproduce Next.js's "sniff then 400" flow without new machinery. Per AGENTS.md this should still be a deliberate, documented divergence — a one-line comment at the !source.ok branch noting "middleware-blocked/non-image sources return 404 here vs Next.js's 400 (no source buffering)" plus a PR note would satisfy the parity-documentation rule. Not a merge blocker, but please don't leave it silent.
🟡 Transform-failure fallback re-dispatches through the middleware-bearing pipeline
fetchAsset now routes the source through the whole pipeline (Node: HTTP loopback; workers: recursive handleRequest). The transform-failure fallback calls handlers.fetchAsset(imageUrl, request) a second time when the first passthrough throws because transformImage consumed source.body (image-optimization.ts:399-415). That re-runs middleware side effects (logging, rate-limit counters, waitUntil, auth) twice for a single /_next/image request whenever a configured optimizer errors. Next.js buffers the upstream once and falls back in-memory — it never re-fetches. This branch has no buffered bytes to reuse, so it's a genuine double-dispatch. This path only fires when a configured transformImage throws, so it's an edge case, but worth a follow-up given fetchAsset is now side-effecting. (The default no-optimizer path serves the passthrough directly and doesn't hit this.)
🟢 Notes (non-blocking)
- Node uses an HTTP self-fetch loopback (
fetchInternalImageSource→fetch('http://127.0.0.1:port/…'), prod-server.ts:1048-1070) while workers dispatch in-process viahandleRequest. Functionally both run middleware. The loopback opens a real second TCP connection per image request; theinternalImageOrigin0.0.0.0→127.0.0.1 / ::→::1 translation + IPv6 bracketing is correct and tested (features.test.ts).redirect: "manual"correctly prevents the optimizer from following a source redirect (surfaces as!source.ok→ 404). Flagging the architectural asymmetry only. x-forwarded-protois derived frompublicUrl.protocolwhich in Node prod is typicallyhttpeven behind an HTTPS-terminating proxy. The source is a local asset so proto rarely matters, but if any middleware branches on proto it'll seehttpon the internal fetch.- App Router worker still gates image handling on
getImageOptimizer()(app-router-entry.ts:96), so with no configured optimizer/_next/imagefalls through to the RSC handler — unlike the Pages worker (pages-router-entry.ts:161) and Node prod which run unconditionally. Pre-existing, but more visible now that the other seams route through the middleware-aware dispatch. - SSRF surface is bounded:
parseImageParams(image-optimization.ts:182-204) rejects absolute/protocol-relative URLs, socreateInternalImageRequestonly ever re-points a local path at the local origin. The recursion guard (isImageOptimizationPathafter decode + basePath strip) is tested for both literal and%5F-encoded_next/image. Good. cancelResponseBodyis now applied on every early-return branch (404/400) so rejected source bodies don't leak — nicely covered by "cancels rejected image source bodies".
Verified locally
npx tsc --noEmit -p packages/vinext/tsconfig.json— clean (exit 0).vitest run tests/shims.test.ts -t "credential-free GET|recursive local image optimizer|cancels rejected image source bodies|does not expose source response headers|handleImageOptimization"— 29 passed.- Traced both Node seams + both worker seams for header handling; confirmed the fresh-
Headersallowlist is applied consistently and the e2e/shims tests cover the success-path no-leak assertion.
The middleware-ordering mechanism is a sound parity improvement and the header leak is genuinely fixed here. No blockers. The two 🟡 items — documenting the 404-vs-400 divergence, and the transform-fallback double-dispatch — are the things worth addressing (or explicitly deferring) before/after merge.
|
Review posted to PR #2554. SummaryI reviewed at the actual branch head ( No blockers. Findings: ✅ Header leak (the last review's 🔴 blocker) — genuinely fixed in this revision. Both Node seams now use 🟡 401-blocked source returns 404 where Next.js returns 400. The e2e test enshrines 404. Mitigating: the 🟡 Transform-failure fallback re-dispatches through the middleware-bearing pipeline — double-runs middleware side effects on the edge case where a configured 🟢 Notes: Node HTTP self-fetch loopback vs in-process worker dispatch asymmetry (IPv6 bracketing correct/tested); Verified locally: |
1 participant
Summary
This change intentionally preserves the optimizer's existing status handling and transform-fallback behavior; it only changes how local source requests are dispatched.
Tests
vp check packages/vinext/src/server/image-optimization.ts packages/vinext/src/server/app-router-entry.ts packages/vinext/src/server/pages-router-entry.ts packages/vinext/src/server/prod-server.ts tests/e2e/pages-router-prod/image-middleware-ordering.spec.ts tests/fixtures/pages-basic/middleware.ts tests/shims.test.ts tests/features.test.tsvp test run tests/shims.test.ts -t "credential-free GET|recursive local image optimizer|cancels rejected image source bodies|does not expose source response headers"vp test run tests/features.test.ts -t "internalImageOrigin brackets IPv6 listener hosts"CI=1 PLAYWRIGHT_PROJECT=pages-router-prod npx playwright test tests/e2e/pages-router-prod/image-middleware-ordering.spec.ts --project=pages-router-prod --workers=1