fix(image): align next/image API with Next.js 16.0.0#385
fix(image): align next/image API with Next.js 16.0.0#385Divkix wants to merge 15 commits intocloudflare:mainfrom
Conversation
Closes multiple compatibility gaps between vinext's Image shim and
Next.js 16's public API:
- Add blurWidth/blurHeight to StaticImageData interface
- Fix imageSizes default: remove 16 to match [32,48,64,96,128,256,384]
- Fix contentDispositionType default: "inline" → "attachment" (security)
- Add preload prop (Next.js 16 replacement for deprecated priority)
- Add qualities config support with findClosestQuality() rounding
- Apply AVIF quality offset (quality - 20) in handleImageOptimization
- Add deprecated props (onLoadingComplete, layout, objectFit, etc.)
- Widen PlaceholderValue type to accept data:image/ URLs
- Widen width/height to accept string numbers (number | `${number}`)
- Export StaticRequire, StaticImport, ImageLoaderProps, ImageProps types
- Add allowedQualities param to parseImageParams validation
commit: |
There was a problem hiding this comment.
Pull request overview
This PR aligns vinext's next/image shim with the Next.js 16.0.0 API surface. It updates defaults (image sizes, Content-Disposition), adds new props (preload, deprecated compat props, string width/height, data:image/ placeholder), exports new types, adds quality allowlist support (images.qualities), and applies the AVIF quality offset used in Next.js 16.
Changes:
- Aligns
next/imagetypes and props with Next.js 16 (newpreloadprop, deprecated props,PlaceholderValuetype,StaticImport/StaticRequiretypes, string dimensions) - Updates image config defaults:
DEFAULT_IMAGE_SIZESdrops16,Content-Dispositionchanges from"inline"to"attachment"for XSS protection - Adds
images.qualitiesconfig support with client-sidefindClosestQuality()rounding and server-sideparseImageParamsvalidation, plus AVIF quality offset in the optimization handler
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
packages/vinext/src/shims/image.tsx |
Core Next.js 16 API alignment: new types, props, and quality URL building |
packages/vinext/src/server/image-optimization.ts |
DEFAULT_IMAGE_SIZES update, allowedQualities param in parseImageParams, Content-Disposition default, AVIF quality offset |
packages/vinext/src/server/prod-server.ts |
App Router server Content-Disposition default updated to "attachment" |
packages/vinext/src/config/next-config.ts |
Adds qualities field and updates JSDoc for contentDispositionType |
packages/vinext/src/global.d.ts |
Adds __VINEXT_IMAGE_QUALITIES process env type declaration |
packages/vinext/src/index.ts |
Exposes images.qualities via Vite define |
tests/shims.test.ts |
Updates assertions for new defaults (DEFAULT_IMAGE_SIZES, Content-Disposition) |
tests/image-component.test.ts |
New tests for Next.js 16 API surface features |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (allowedWidths && w !== 0 && !allowedWidths.includes(w)) return null; | ||
| // Validate quality (1-100) | ||
| if (Number.isNaN(q) || q < 1 || q > 100) return null; | ||
| if (allowedQualities && !allowedQualities.includes(q)) return null; |
There was a problem hiding this comment.
The new allowedQualities parameter in parseImageParams has no test coverage. The existing test suite in tests/shims.test.ts thoroughly tests every other parseImageParams validation (widths, quality range, URL safety, etc.) but there are no tests verifying that:
- A quality value not in
allowedQualitiesreturnsnull. - A quality value that is in
allowedQualitiesis accepted.
This gap is significant because the quality validation code path (line 79) is also never exercised through handleImageOptimization (see separate comment), making it hard to detect the wiring issue.
packages/vinext/src/shims/image.tsx
Outdated
| */ | ||
| function findClosestQuality(quality: number, qualities?: number[]): number { | ||
| if (!qualities?.length) return quality; | ||
| return qualities.reduce((prev, cur) => (Math.abs(cur - quality) < Math.abs(prev - quality) ? cur : prev), 0); |
There was a problem hiding this comment.
The findClosestQuality reduce call uses 0 as the initial accumulator value (second argument to reduce). Since 0 is not a valid quality value, when all valid quality values in the array are farther from quality than 0 is, the function returns 0 instead of the closest valid quality.
For example, with qualities = [25, 50, 75, 100] and quality = 10:
abs(25 - 10) = 15vsabs(0 - 10) = 10→ 0 wins- Same for all subsequent values
- Returns
0, which is an invalid quality that will cause the image URL to carry?q=0, and be rejected by the server's quality validation (q < 1 → return null).
The initial accumulator should be qualities[0] to ensure a valid quality value is always returned. Since the guard if (!qualities?.length) return quality already ensures the array has at least one element, qualities[0] is always safe here.
| imageConfig?.contentSecurityPolicy ?? IMAGE_CONTENT_SECURITY_POLICY, | ||
| "X-Content-Type-Options": "nosniff", | ||
| "Content-Disposition": imageConfig?.contentDispositionType ?? "inline", | ||
| "Content-Disposition": imageConfig?.contentDispositionType ?? "attachment", |
There was a problem hiding this comment.
The PR correctly changes the default Content-Disposition to "attachment" for the App Router production server (line 601), but the Pages Router production server (startPagesRouterServer, around line 775 in prod-server.ts) was not updated and still defaults to "inline".
This means the security fix is incomplete: images served through the Pages Router's Node.js production server will still use Content-Disposition: inline, leaving XSS-via-image-endpoint open on that code path.
The "Content-Disposition": pagesImageConfig?.contentDispositionType ?? "inline" on line 775 should be ?? "attachment" to match all other call sites.
| export function parseImageParams( | ||
| url: URL, | ||
| allowedWidths?: number[], | ||
| allowedQualities?: number[], | ||
| ): { imageUrl: string; width: number; quality: number } | null { | ||
| const imageUrl = url.searchParams.get("url"); | ||
| if (!imageUrl) return null; |
There was a problem hiding this comment.
allowedQualities was added as a new parameter to parseImageParams, but handleImageOptimization (the main entry point used by all workers) calls parseImageParams(url, allowedWidths) without passing allowedQualities. This means the quality validation added here is never actually enforced through handleImageOptimization. The PR description claims "server-side validation" for qualities, but it is only reachable if callers invoke parseImageParams directly. To fully enforce the allowlist, allowedQualities (or qualities as part of ImageConfig) needs to be threaded through handleImageOptimization.
| @@ -166,10 +166,12 @@ export interface NextConfig { | |||
| imageSizes?: number[]; | |||
There was a problem hiding this comment.
The JSDoc comment for imageSizes still says Defaults to Next.js defaults: [16, 32, 48, 64, 96, 128, 256, 384], but the actual default was changed to [32, 48, 64, 96, 128, 256, 384] (removing 16). This comment should be updated to reflect the new default.
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good work aligning the image API with Next.js 16. The type exports, preload prop, deprecated prop handling, StaticImport/StaticRequire unwrapping, AVIF quality offset, and Content-Disposition default change are all solid.
Two issues need fixing before merge (one correctness bug, one security gap), plus a few minor items.
Security: Pages Router Content-Disposition still defaults to "inline"
packages/vinext/src/server/prod-server.ts:775 was not updated:
"Content-Disposition": pagesImageConfig?.contentDispositionType ?? "inline",This should be ?? "attachment" to match the App Router prod server (line 601), the handleImageOptimization helper (line 176), and all the updated tests. Without this, the XSS-via-image-endpoint mitigation is incomplete for Pages Router production deployments.
Stale JSDoc on imageSizes
packages/vinext/src/config/next-config.ts:165 still says:
Defaults to Next.js defaults: [16, 32, 48, 64, 96, 128, 256, 384]
but the actual default was changed to [32, 48, 64, 96, 128, 256, 384].
allowedQualities not wired through handleImageOptimization (non-blocking)
parseImageParams accepts allowedQualities as a third argument, but handleImageOptimization on line 207 calls parseImageParams(url, allowedWidths) without passing it. The server-side quality validation is unreachable from the main request handler. Since the client-side findClosestQuality rounds quality before building the URL, this is defense-in-depth rather than a correctness issue. Worth either wiring it through or removing the dead code path, but not a blocker.
packages/vinext/src/shims/image.tsx
Outdated
| */ | ||
| function findClosestQuality(quality: number, qualities?: number[]): number { | ||
| if (!qualities?.length) return quality; | ||
| return qualities.reduce((prev, cur) => (Math.abs(cur - quality) < Math.abs(prev - quality) ? cur : prev), 0); |
There was a problem hiding this comment.
Bug: reduce uses 0 as the initial accumulator, which is not a valid quality value and wins the comparison whenever the closest real quality is farther from quality than 0 is.
Example: findClosestQuality(10, [25, 50, 75, 100]) returns 0 because |0 - 10| = 10 < |25 - 10| = 15. That q=0 then gets rejected server-side (q < 1 → null → 400).
The guard on line 139 already ensures qualities has at least one element, so qualities[0] is safe:
| return qualities.reduce((prev, cur) => (Math.abs(cur - quality) < Math.abs(prev - quality) ? cur : prev), 0); | |
| return qualities.reduce((prev, cur) => (Math.abs(cur - quality) < Math.abs(prev - quality) ? cur : prev), qualities[0]); |
| }); | ||
|
|
||
| // ─── Next.js 16 API surface alignment ──────────────────────────────── | ||
|
|
There was a problem hiding this comment.
Missing test coverage: findClosestQuality and allowedQualities server validation have no tests. The findClosestQuality bug (initial accumulator 0) would have been caught by a test like:
describe("findClosestQuality", () => {
it("returns closest allowed quality for low input", () => {
// With qualities [25, 50, 75, 100] and quality 10, should return 25
expect(imageOptimizationUrl("/test.png", 100, 10)).toContain("q=25");
});
});Also consider a parseImageParams test for quality not in allowedQualities returning null.
|
Review submitted successfully. Here's a summary of the feedback: Requested changes on PR #385:
|
- Fix findClosestQuality accumulator bug: use qualities[0] instead of 0 to prevent returning invalid quality when all allowed values are far from input - Fix Pages Router Content-Disposition default from "inline" to "attachment" to close XSS-via-image-endpoint vector - Wire allowedQualities through handleImageOptimization to parseImageParams so server-side quality validation is no longer dead code - Fix stale JSDoc on imageSizes default (remove non-existent 16) - Add findClosestQuality test coverage: low input, exact match, rounding, equidistant tiebreak, single-element array
|
/bigbonk re-review |
There was a problem hiding this comment.
Good work. The second commit addressed all the blocking issues from the prior reviews (the findClosestQuality accumulator bug, the Pages Router Content-Disposition gap, and the stale JSDoc). The implementation is clean and well-tested.
Two non-blocking observations — neither is worth blocking the PR over, but they'd be good follow-ups.
allowedQualities not wired from callers (non-blocking)
handleImageOptimization now accepts allowedQualities as the 5th parameter, but no caller passes it:
deploy.ts:454(App Router worker) — passesallowedWidthsonlydeploy.ts:545(Pages Router worker) — passesallowedWidthsandimageConfigonly- All tests call with at most 4 args
The client-side findClosestQuality rounding means this is defense-in-depth, not a correctness issue — the URL will already carry a valid quality value by the time it hits the server. But the dead code path is confusing. Consider either wiring allowedQualities through from config in a follow-up, or removing the server-side check until it's actually needed.
Remote image path doesn't handle data:image/ placeholder (minor)
The data:image/ placeholder handling was added to the local image path (image.tsx:410-413) and getImageProps (image.tsx:577-579), but the remote image code path (image.tsx:364-365) still only checks placeholder === "blur". A remote image with placeholder="data:image/png;base64,..." won't show the blur background when rendered via @unpic/react. This is a minor edge case, but worth noting for completeness.
| handlers: ImageHandlers, | ||
| allowedWidths?: number[], | ||
| imageConfig?: ImageConfig, | ||
| allowedQualities?: number[], |
There was a problem hiding this comment.
Non-blocking: allowedQualities is accepted here but no caller passes it. deploy.ts:454 and deploy.ts:545 both omit the 5th argument, and the prod-server uses parseImageParams directly without qualities. This makes the server-side quality validation unreachable in practice.
Since findClosestQuality on the client side already rounds to a valid value before building the URL, this isn't a correctness issue. Consider either wiring it through from config in a follow-up or deferring the parameter until callers need it.
|
Review submitted successfully. Here's a summary of what I found: Approved — the PR is in good shape after the second commit addressed all blocking issues from earlier reviews. What was fixed in the second commit
Two non-blocking observations
|
|
I'll work on the 2 non-blocking issues and also on readme and the skills command |
…/ placeholder - Remove `allowedQualities` from `parseImageParams()` and `handleImageOptimization()` — no caller ever passed it, and client-side `findClosestQuality` rounding is sufficient. - Fix remote image code path to handle `data:image/` placeholder strings (Next.js 16), matching the local image and `getImageProps` code paths that already handled this. - Add test for remote image `data:image/` placeholder (49/49 pass). - Update README: next/image coverage, images config details, known limitations wording. - Update TRACKING.md test counts (P5-2: 22→49, total: 230→257). - Update migration skill compatibility doc.
|
/bigbonk re-review |
There was a problem hiding this comment.
Thorough and well-structured alignment with the Next.js 16 image API. The three commits tell a clean story: initial implementation, review feedback fixes, and non-blocking follow-ups. All the blocking issues from prior reviews have been addressed.
A few minor observations below — none are blockers.
packages/vinext/src/shims/image.tsx
Outdated
| function toNumber(value: number | `${number}` | undefined): number | undefined { | ||
| if (value === undefined) return undefined; | ||
| if (typeof value === "number") return value; | ||
| return parseInt(value, 10); |
There was a problem hiding this comment.
Minor: parseInt with radix 10 will parse "200px" as 200 and "" as NaN. The empty string case is harmless (returns undefined via the guard above), but a string like "200px" silently becoming 200 might mask user errors.
Next.js 16 types this as number | \${number}`` so in practice only pure numeric strings are expected — this is fine as-is, just noting the behavior.
packages/vinext/src/shims/image.tsx
Outdated
| blurDataURL ?? (typeof unwrapped === "object" ? unwrapped.blurDataURL : undefined); | ||
|
|
||
| // Both priority and preload trigger eager loading (Next.js 16 compat) | ||
| const isEager = priority || preload; |
There was a problem hiding this comment.
Nit: priority || preload evaluates left-to-right and both are optional booleans defaulting to undefined. When both are falsy, isEager is undefined (falsy), which works correctly in the ternary on line 329. Just noting that isEager is boolean | undefined, not strictly boolean. Not a bug, but !!priority || !!preload or Boolean(priority || preload) would be slightly more explicit if you wanted a strict boolean.
tests/image-component.test.ts
Outdated
| }), | ||
| ); | ||
| expect(html).toContain('loading="eager"'); | ||
| expect(html).toContain('fetchPriority="high"'); |
There was a problem hiding this comment.
Good coverage of preload prop. Consider adding a test that verifies preload and priority together don't conflict (both true should still produce loading="eager" and fetchPriority="high" — not double attributes). Currently the test only tests them independently.
| try { | ||
| // AVIF encodes more efficiently, so use a lower quality value for | ||
| // equivalent perceptual quality. Matches Next.js 16 behavior. | ||
| const adjustedQuality = format === "image/avif" ? Math.max(quality - 20, 1) : quality; |
There was a problem hiding this comment.
The AVIF quality offset (quality - 20) is a significant behavioral change for production image serving. There's no test coverage verifying this adjustment — the existing handleImageOptimization transform test on line 6050 of shims.test.ts only tests WebP (which doesn't get the offset). A test with Accept: image/avif that captures the options passed to transformImage and asserts quality === 55 for input q=75 would lock in this behavior.
Not blocking since this matches documented Next.js 16 behavior, but worth adding to prevent accidental regression.
packages/vinext/src/shims/image.tsx
Outdated
| width={fill ? undefined : imgWidth} | ||
| height={fill ? undefined : imgHeight} | ||
| loading={priority ? "eager" : (loading ?? "lazy")} | ||
| loading={isEager ? "eager" : (loading ?? "lazy")} |
There was a problem hiding this comment.
The custom loader path sets loading and decoding but doesn't set fetchPriority like the local image path does (line 467). In Next.js 16, priority/preload adds fetchPriority="high" regardless of whether a custom loader is used. Minor gap.
| loading={isEager ? "eager" : (loading ?? "lazy")} | |
| loading={isEager ? "eager" : (loading ?? "lazy")} | |
| fetchPriority={isEager ? "high" : undefined} | |
| decoding="async" |
|
Review submitted. Here's a summary of my findings: Approved — the PR is solid after three commits that progressively addressed all prior review feedback. What's good
Minor observations (non-blocking)
|
|
Will work on these and try to match with latest release v16.1 to verify no changes are there so it could be latest |
…/preload tests
The custom loader <img> tag was missing fetchPriority={isEager ? "high" : undefined},
which the local image path already had. This caused priority/preload to set
loading="eager" but not fetchPriority="high" when using a custom loader.
Tests added:
- AVIF quality offset: verifies -20 adjustment (q=75→55) and floor clamp (q=20→1)
- WebP gets no AVIF offset (control test)
- Combined preload + priority props render without conflict
- Custom loader with priority/preload now correctly sets fetchPriority="high"
|
Implemented the Next.js 16 contract-layer pass for What changed:
Verification:
|
|
@james-elicx should i merge this in #364 instead? the vitest is failing because of work to be done in #364 |
|
Hmm I would have assumed we'd be able to do this without adding the new Sharp optimisation... Was it the aligning with next 16 that introduced that problem? Happy to go back a couple commits and merge based on the Bonk review a few commits ago if you want to keep the PRs separate? |
|
Okay, I tried something and this should fix the ci |
|
/bigbonk re-review |
There was a problem hiding this comment.
Review
CI is green, and the core image API alignment with Next.js 16 is well-executed — the getImgProps-style normalization, preload prop, qualities rounding, StaticImport unwrapping, AVIF quality offset, and Content-Disposition default change all look correct and match upstream.
I have a few concerns below, one security issue that should be addressed before merge, and some scope/architecture observations.
Security: fetchRemoteImage forwards all request headers to external hosts
This is the most important issue. The fetch(currentUrl, { headers: request.headers }) call at line 228 forwards the entire original request's headers — including Cookie, Authorization, and any custom tokens — to whatever external host is configured in remotePatterns. An attacker who gets a domain added to remotePatterns (or exploits an overly broad pattern) can exfiltrate session tokens. Next.js only forwards a selective subset of headers (primarily Accept). This should be fixed before merge.
Scope creep: resolveId refactoring
The resolveId hook was changed from the optimized { filter, handler } object form to a plain function, removing the Vite hook filter optimization. This means the hook now runs for every module resolution in the project, not just next/* and virtual:vinext-* imports. The new direct alias resolution, extensionless import resolution for node_modules, and .js variant generation are useful ecosystem fixes but are unrelated to next/image and would benefit from their own PR with targeted testing. Including them here makes the diff harder to review and the risk surface larger.
source.body reuse after transformImage failure
In the catch block at line 343-349, source.body is reused after being passed to transformImage. If transformImage partially consumed the ReadableStream before throwing, the stream is locked/consumed and cannot be passed to a new Response. This will produce a runtime error or truncated response. Consider buffering the source body into an ArrayBuffer before passing it to transformImage, or returning a 500 error in the catch path instead of attempting to serve the original.
isBlockedLocalHost has coverage gaps
The SSRF protection at line 192 misses several common bypass patterns:
0.0.0.0(equivalent to localhost on many systems)- IPv4-mapped IPv6 addresses like
::ffff:127.0.0.1or::ffff:10.0.0.1 - The
fc/fdprefix check at line 197 matches any hostname starting with those letters (e.g.fcbarcelona.com), not just IPv6 ULA addresses
The fc/fd check should verify the string is actually an IPv6 address before matching. Consider checking for : in the hostname.
Minor observations (non-blocking)
-
No test for AVIF quality offset — the
-20adjustment inhandleImageOptimizationline 327 has no test coverage. A test withAccept: image/avifthat verifiesquality === 55for inputq=75would lock in this behavior. -
Remote image fetching is untested —
fetchRemoteImage,fetchSourceImage,isBlockedLocalHost, andtoContentDispositionFilenamehave zero unit test coverage. These are security-critical code paths. -
__NEXT_DATA__type declaration (global.d.ts) — This was added in this PR but is unrelated to image changes. The PR description doesn't mention it.
| } | ||
|
|
||
| const response = await fetch(currentUrl, { | ||
| headers: request.headers, |
There was a problem hiding this comment.
Security issue: Forwarding all request headers to external hosts leaks sensitive data (Cookie, Authorization, custom tokens) to any domain configured in remotePatterns. Next.js only forwards a selective subset.
| headers: request.headers, | |
| const response = await fetch(currentUrl, { | |
| headers: { Accept: request.headers.get("Accept") ?? "*/*" }, | |
| redirect: "manual", | |
| }); |
| headers.set("Cache-Control", getCacheControl(imageConfig)); | ||
| headers.set("Vary", "Accept"); | ||
| setImageSecurityHeaders(headers, params.imageUrl, sourceMediaType ?? null, imageConfig); | ||
| return new Response(source.body, { status: 200, headers }); |
There was a problem hiding this comment.
Bug: source.body was already passed to handlers.transformImage() on line 329. If transformImage partially consumed the ReadableStream before throwing, this stream is locked/consumed and cannot be reused in a new Response. This will cause a runtime error or produce a truncated response.
Consider either:
- Buffering the source into an
ArrayBufferbefore passing totransformImage(so you can reconstruct a fresh stream on failure), or - Returning a 500 error here instead of attempting to serve the already-consumed original.
| if (lower === "::1" || lower.startsWith("fc") || lower.startsWith("fd")) { | ||
| return true; |
There was a problem hiding this comment.
Bug: The fc/fd prefix check matches any hostname starting with those letters, not just IPv6 ULA addresses. For example, fcbarcelona.com or fdny.gov would be incorrectly blocked.
This should verify the string is actually an IPv6 address (contains :) before checking the prefix:
| if (lower === "::1" || lower.startsWith("fc") || lower.startsWith("fd")) { | |
| return true; | |
| if (lower === "::1" || (lower.includes(":") && (lower.startsWith("fc") || lower.startsWith("fd")))) { |
| function isBlockedLocalHost(hostname: string): boolean { | ||
| const lower = hostname.toLowerCase(); | ||
| if (lower === "localhost" || lower.endsWith(".localhost") || lower.endsWith(".local")) { | ||
| return true; | ||
| } | ||
| if (lower === "::1" || lower.startsWith("fc") || lower.startsWith("fd")) { | ||
| return true; | ||
| } | ||
| const ipv4 = lower.match(/^(\d+)\.(\d+)\.(\d+)\.(\d+)$/); | ||
| if (!ipv4) return false; | ||
| const [a, b] = [parseInt(ipv4[1], 10), parseInt(ipv4[2], 10)]; | ||
| return ( | ||
| a === 10 || | ||
| a === 127 || | ||
| (a === 169 && b === 254) || | ||
| (a === 172 && b >= 16 && b <= 31) || | ||
| (a === 192 && b === 168) | ||
| ); | ||
| headers.set("X-Content-Type-Options", "nosniff"); | ||
| headers.set("Content-Disposition", config?.contentDispositionType ?? "inline"); | ||
| } |
There was a problem hiding this comment.
Missing check for 0.0.0.0 — on many systems this is equivalent to localhost. Also missing checks for IPv4-mapped IPv6 addresses (::ffff:127.0.0.1, ::ffff:10.x.x.x, etc.), which are common SSRF bypass techniques.
Consider adding:
if (lower === '0.0.0.0') return true;
if (lower.startsWith('::ffff:')) {
const mapped = lower.slice(7);
// Recurse with the mapped IPv4 address
return isBlockedLocalHost(mapped);
}
packages/vinext/src/index.ts
Outdated
| resolveId(id, importer) { | ||
| // Strip \0 prefix if present — @vitejs/plugin-rsc's generated | ||
| // browser entry imports our virtual module using the already-resolved | ||
| // ID (with \0 prefix). We need to re-resolve it so the client | ||
| // environment's import-analysis can find it. | ||
| const cleanId = id.startsWith("\0") ? id.slice(1) : id; | ||
|
|
||
| // Resolve shim and user aliases directly here instead of relying only | ||
| // on Vite's alias plugin. The dev module runner can otherwise fall | ||
| // back to native package resolution for dependency imports, which | ||
| // breaks ecosystem packages like nuqs and next-intl. | ||
| const directAlias = nextShimMap[cleanId]; | ||
| if (directAlias) { | ||
| return directAlias; | ||
| } | ||
|
|
||
| // Handle extensionless relative imports inside node_modules packages. | ||
| // Some ESM packages (validator/es) publish `./foo` imports without an | ||
| // extension, which Node rejects during SSR if the module runner lets | ||
| // them escape Vite's resolver. | ||
| if ( | ||
| importer && | ||
| cleanId.startsWith(".") && | ||
| path.extname(cleanId) === "" && | ||
| importer.includes("node_modules") | ||
| ) { | ||
| const importerPath = importer.startsWith("\0") ? importer.slice(1) : importer; | ||
| const importerFile = importerPath.split("?")[0]; | ||
| if (path.isAbsolute(importerFile)) { | ||
| const basePath = path.resolve(path.dirname(importerFile), cleanId); | ||
| for (const ext of [".js", ".mjs", ".cjs", ".ts", ".tsx", ".jsx", ".json"]) { | ||
| const fileCandidate = basePath + ext; | ||
| if (fs.existsSync(fileCandidate)) { | ||
| return fileCandidate; | ||
| } | ||
| const indexCandidate = path.join(basePath, `index${ext}`); | ||
| if (fs.existsSync(indexCandidate)) { | ||
| return indexCandidate; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Scope concern: This resolveId refactoring removes the Vite hook filter (filter: { id: /(?:next\/|virtual:vinext-)/ }) and replaces it with a plain function that runs for every module resolution in the project. The new direct alias resolution (line 1353), extensionless import resolution for node_modules (line 1362-1383), and .js variant generation (line 943 in configResolved) are useful ecosystem fixes, but they're unrelated to the next/image API alignment.
Running resolveId on every import (including react, vite, all node_modules, etc.) instead of only next/* and virtual:vinext-* is a performance regression for large projects. I'd suggest splitting this into its own PR where it can be tested independently — especially the fs.existsSync calls on every extensionless relative import from node_modules, which are synchronous I/O in a hot path.
| unoptimized = true; | ||
| } | ||
|
|
||
| const qualityInt = getInt(quality); |
There was a problem hiding this comment.
Nit: qualityInt is passed to generateImgAttrs and then to the loader, but when no quality prop is given, qualityInt is undefined. The default loader handles this via quality ?? 75 in defaultLoader (line 365), but custom loaders receive undefined as the quality property of ImageLoaderProps. This matches Next.js behavior (where quality is optional in ImageLoaderProps), but it's worth noting that custom loaders are responsible for defaulting quality themselves.
|
Review posted successfully. Here's a summary of what I flagged: Security (should fix before merge):
Bug:
Scope concern:
Non-blocking observations:
|
|
Addressed the review items in this branch:
Coverage updates:
Validation run locally:
|
|
/bonk re-review |
There was a problem hiding this comment.
Review
Good work aligning the next/image API surface with Next.js 16. The shared normalizeProps path for <Image> and getImageProps(), the preload link-hint emission via ServerInsertedHTMLContext, the SSRF protection with per-hop redirect checking, the stream-buffering strategy for transform fallback, and the Accept-only header forwarding are all well-designed.
One bug that should be fixed before merge, plus several non-blocking observations.
Bug: App Router worker entry doesn't pass imageConfig to handleImageOptimization
The generated App Router worker entry in generateAppRouterWorkerEntry() (deploy.ts, line 454-460) calls handleImageOptimization(request, handlers, allowedWidths) with only 3 arguments. The 4th parameter (imageConfig) is missing. Compare with:
- Pages Router worker entry (line 563): passes all 4 args including
imageConfig - App Router prod-server (line 612-620): passes
imageConfig - Pages Router prod-server (line 781-788): passes
pagesImageConfig
The generated App Router worker entry also doesn't construct an imageConfig variable from vinextConfig (the Pages Router entry does this at lines 514-530). Additionally, line 452 hardcodes "/_vinext/image" while the Pages Router entry uses imageConfig?.path ?? "/_vinext/image" (line 555).
Impact: All image config settings (remote/local patterns, SVG policy, CSP, qualities validation, cache TTL, dangerouslyAllowLocalIP, custom path) are silently ignored for App Router Cloudflare Worker deployments.
Non-blocking observations
1. minimumCacheTTL default diverges 240x from Next.js — imageConfigDefault.minimumCacheTTL is 14_400 (4 hours) vs Next.js 16's 60 (1 minute). If this is intentional for Cloudflare Workers, add a comment. If not, it should match upstream.
2. findClosestQuality has zero test coverage — TRACKING.md claims 8 tests pass, but no test file contains any reference to findClosestQuality. The tracking entry is phantom. The function is on the critical path for URL generation.
3. OnLoadingComplete not re-exported from "next/image" module — Exported from image.tsx:41 but missing from next-shims.d.ts lines 133-141. Import will fail in TypeScript.
4. isBlockedIpv4 doesn't block 0.0.0.0/8 — The explicit 0.0.0.0 string check catches the exact address, but isBlockedIpv4 doesn't check a === 0. Addresses like 0.1.2.3 (which may resolve to localhost on some systems) bypass the check.
5. onLoadingComplete callback is never invoked — It's destructured and warned about in dev, but the callback itself is silently dropped. In Next.js, it's still wired to fire on image load for backward compat.
6. blurWidth/blurHeight declared but unused — StaticImageData declares these fields but normalizeProps never reads them. Minor feature gap vs Next.js.
| loaderFile: "", | ||
| domains: [], | ||
| disableStaticImages: false, | ||
| minimumCacheTTL: 14_400, |
There was a problem hiding this comment.
Intentional? Next.js 16 defaults minimumCacheTTL to 60 (seconds). This is 14_400 (4 hours) — a 240x difference. If this is a deliberate optimization for Cloudflare Workers (where re-optimization is expensive), it should have a comment explaining the deviation. If not, it should match upstream.
| minimumCacheTTL: 14_400, | |
| minimumCacheTTL: 14_400, // 4 hours — intentionally higher than Next.js default (60s) for Workers |
| | 37-38 | placeholder data URL (Next.js 16): local + remote | PASS | | | ||
| | 39-40 | StaticImport / StaticRequire support | PASS | | | ||
| | 41-42 | type exports (ImageLoaderProps, ImageProps) | PASS | | | ||
| | 43-49 | findClosestQuality (7 tests + 1 equidistant) | PASS | | |
There was a problem hiding this comment.
Phantom test entry. This claims findClosestQuality has 7 tests + 1 equidistant, all PASS. However, no *.test.ts file in the repository contains any reference to findClosestQuality. These tests don't exist. Either add the tests or remove this tracking entry.
| StaticImageData, | ||
| StaticImport, | ||
| StaticRequire, | ||
| }; |
There was a problem hiding this comment.
Minor: OnLoadingComplete is exported from image.tsx (line 41) but missing from this re-export list. Code doing import type { OnLoadingComplete } from "next/image" will get a TypeScript error.
| }; | |
| export type { | |
| ImageProps, | |
| ImageLoader, | |
| ImageLoaderProps, | |
| PlaceholderValue, | |
| StaticImageData, | |
| StaticImport, | |
| StaticRequire, | |
| OnLoadingComplete, | |
| }; |
(Also needs OnLoadingComplete added to the import on line 123.)
| a === 127 || | ||
| (a === 169 && b === 254) || | ||
| (a === 172 && b >= 16 && b <= 31) || | ||
| (a === 192 && b === 168) |
There was a problem hiding this comment.
Low severity: isBlockedIpv4 doesn't check a === 0, so addresses in the 0.0.0.0/8 range (e.g., 0.1.2.3) bypass this. The explicit lower === "0.0.0.0" string check at line 294 only catches the exact address. On some systems, 0.x.x.x addresses resolve to localhost.
| (a === 192 && b === 168) | |
| return ( | |
| a === 0 || | |
| a === 10 || | |
| a === 127 || | |
| (a === 169 && b === 254) || | |
| (a === 172 && b >= 16 && b <= 31) || | |
| (a === 192 && b === 168) | |
| ); |
With this change, the explicit 0.0.0.0 string check at line 294 becomes redundant and can be removed.
packages/vinext/src/shims/image.tsx
Outdated
| if (objectPosition) { | ||
| console.warn(`Image with src "${src}" has legacy prop "objectPosition".`); | ||
| } | ||
| if (onLoadingComplete) { |
There was a problem hiding this comment.
Non-blocking: onLoadingComplete is warned about here but the callback is never actually invoked — it's destructured at line 434 and silently dropped. In Next.js, deprecated onLoadingComplete is still wired to fire when the image loads. Users migrating from older Next.js code will silently lose this behavior.
Consider wiring it through onLoad for backward compat:
onLoad: (e) => {
onLoad?.(e);
if (onLoadingComplete) onLoadingComplete(e.currentTarget);
},
|
Review submitted successfully. Here's a summary of the feedback: Bug (should fix before merge)
Non-blocking observations (with inline comments)
|
- Fix App Router worker entry missing imageConfig: add virtual:vinext-image-config module, pass imageConfig as 4th arg to handleImageOptimization, use imageConfig.path instead of hardcoded /_vinext/image - Block 0.0.0.0/8 range in isBlockedIpv4, remove redundant string check - Wire deprecated onLoadingComplete through onLoad callback - Export OnLoadingComplete type from next/image module declaration - Add findClosestQuality tests (ported from Next.js test suite) - Document minimumCacheTTL matches Next.js 16 upstream default - Document blurWidth/blurHeight as known gap in StaticImageData
|
I think this should be good, worked through issues |
Cover SSRF prevention (IPv4/IPv6 blocking, AWS metadata endpoint), redirect/body-size limits, blurDataURL CSS injection sanitization, config validation edge cases, allowedQualities enforcement, local pattern server enforcement, loader:custom error path, vw-based srcSet filtering, and matchLocalPattern/hasLocalMatch. Exports 5 internal security functions from image-optimization.ts for direct unit testing. Adds 82 new tests across 4 files.
Add 40+ tests covering previously untested image optimization functions: - isSafeImageContentType: all safe types, blocked types, SVG with dangerouslyAllowSVG, content-type with params, null/empty - Security headers via handleImageOptimization: X-Content-Type-Options, Content-Disposition (attachment/inline), CSP header, custom CSP, filename - SVG end-to-end blocking: 400 when disallowed, 200 when allowed, CSP on SVG - negotiateImageFormat: Accept header parsing, format priority, fallback - Cache-Control: default TTL (14400), custom minimumCacheTTL, format - Content-Disposition filename: extension mapping, basename extraction - AVIF quality adjustment: -20 reduction, clamp to 1, no reduction for non-AVIF Also includes oxfmt formatting fixes for image-config and next-config tests.
…mization, and component - Security: SSRF redirect chains, protocol-relative blocking, backslash normalization, cookie/auth isolation, 302/307/308 redirects, 403/404 upstream codes, Vary: Accept, dangerous URL schemes - Optimization: parseImageParams edge cases, null body 404, text/html rejection, transformImage fallback, SVG pass-through verification, Content-Disposition filename edge cases - Component: 20 dev-mode validation errors, fill mode rendering, legacy prop warnings, reverse dimension inference, SVG auto-unoptimized - Document 6 feature gaps as TODOs (recursive URL guard, URL length limit, upstream timeout, animated detection, magic bytes, ETag)
…gaps Implement 6 features from the Next.js 16 image optimizer that were missing: - Content-type detection via magic bytes (16 formats) instead of trusting headers - Animated image detection (GIF/WebP/APNG) to skip optimization - Cache-Control parsing (s-maxage/max-age) from upstream responses - Recursive URL guard preventing /_vinext/image optimization loops - URL length limit (3072 chars) for DoS prevention - Upstream fetch timeout (7s AbortSignal, returns 504) Add 62 new tests across 5 files bringing total image tests to 342.
…generation Port the SVG blur placeholder generation from Next.js 16 to vinext. This function creates URL-encoded SVG strings with Gaussian blur filters for use as image placeholders, supporting blurWidth/blurHeight scaling and objectFit-aware preserveAspectRatio.
- Integrate getImageBlurSvg() for proper SVG blur placeholders with Gaussian blur filter, replacing raw blurDataURL usage - Extract and pass blurWidth/blurHeight from static imports to SVG generator for optimal blur dimensions - Add INVALID_BACKGROUND_SIZE_VALUES handling: map objectFit values like 'fill' → '100% 100%', 'none'/'scale-down' → 'cover' - Add showAltText state param: skip color:transparent when true - Add blurComplete state param: skip placeholder when true - Add layout→style/sizes mapping for 'intrinsic' and 'responsive' - Add ref prop warning and small image (<40x40) placeholder warning - Export OnLoad, PlaceholderStyle types - Remove objectFit/objectPosition from non-fill base style - Export normalizeProps for direct testing
|
Closing for now — the merge of parallel worktree changes introduced too much risk of subtle bugs. Will revisit this alignment work in a future PR with a cleaner approach. |
|
No worries, thanks for your time so far |
Closes #386
Summary
Aligns vinext's
next/imageimplementation with the Next.js 16.0.0 API surface. Any library or app code written against Next.js 16's image API should now work on vinext without modification.Changes
blurWidthandblurHeightoptional fields16→[32, 48, 64, 96, 128, 256, 384](matches Next.js 16)"inline"→"attachment"(security — prevents XSS via image endpoint)preloadprop: New prop replacing deprecatedpriority(both now triggerloading="eager"+fetchPriority="high")qualitiesconfig: Addimages.qualitiessupport withfindClosestQuality()rounding and server-side validationMath.max(quality - 20, 1)for AVIF inhandleImageOptimization(matches Next.js)onLoadingComplete,layout,objectFit,objectPosition,lazyBoundary,lazyRoot"blur" | "empty" | \data:image/${string}``number | \${number}`` with automatic string→number parsingStaticRequire,StaticImport,ImageLoaderProps,ImagePropsparseImageParamsWhat's deferred
localPatterns,minimumCacheTTL,loader/loaderFileglobal config,disableStaticImages,maximumRedirects,dangerouslyAllowLocalIP,images.unoptimizedglobal,@hapi/acceptnegotiation, and strictw/qrequired params — these are either not yet needed or would break existing vinext users.Test plan
pnpm test tests/image-component.test.ts— 41 tests pass (new tests for preload, deprecated props, string dims, data placeholder, StaticImport, type exports)pnpm test tests/image-imports.test.ts— 20 tests passpnpm test tests/shims.test.ts— 577 pass (updated DEFAULT_IMAGE_SIZES and contentDisposition default assertions)pnpm test tests/deploy.test.ts— 193 tests passpnpm test tests/image-config.test.ts— 33 tests passpnpm run typecheck— no new errors (pre-existing__NEXT_DATA__errors only)pnpm run lint— clean