fix(server): serve static assets with standard MIME types - #2713
Conversation
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 |
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the MIME-type changes. Overall this is a solid, well-tested improvement: centralizing lookup in contentTypeForPath removes the duplicated path.extname(...).toLowerCase() / ?? "application/octet-stream" pattern in four call sites, the case-insensitive matching is a real fix, and mapping .wasm to application/wasm correctly makes the existing COMPRESSIBLE_TYPES entry reachable. I verified:
- Security is preserved — image passthrough still gates on
isSafeImageContentType/SAFE_IMAGE_CONTENT_TYPES, so the newly-added types (pdf, wasm, mp4, etc.) do NOT loosen the image endpoint. fonts.tsstill consumes the exportedCONTENT_TYPESmap and already lowercases its extension, so it's unaffected.vp checkand both test files pass locally (77 passed).
A couple of non-blocking parity observations below. Nothing here blocks merge.
| ".rsc": "text/x-component", | ||
| ".woff": "font/woff", | ||
| ".woff2": "font/woff2", | ||
| ".xml": "application/xml", |
There was a problem hiding this comment.
Minor Next.js-parity nit (non-blocking): Next.js serve-static delegates to send → mime-types, which appends charset whenever mime.charset(type) returns a value. mime-db marks both image/svg+xml and application/xml with charset: UTF-8, so strict parity would serve image/svg+xml; charset=utf-8 and application/xml; charset=utf-8. This PR emits them bare, which is inconsistent with .csv/.txt/.html (which do get the charset). Serving them without charset is harmless in practice, but since the PR's stated goal is matching the send/mime database it's worth either adding the charset to .svg/.xml or noting the deliberate divergence.
| ".js": "application/javascript; charset=UTF-8", | ||
| ".mjs": "application/javascript; charset=UTF-8", | ||
| ".css": "text/css; charset=UTF-8", | ||
| ".html": "text/html; charset=UTF-8", |
There was a problem hiding this comment.
Style nit (non-blocking): the map is otherwise alphabetized by extension, but .js, .mjs, .css, .html (lines 26-29) sit out of order between .heic and .ico. Consider moving them to their alphabetical slots so the ordering convention stays consistent and future additions are easy to place.
|
Review posted successfully to PR #2713. SummaryI reviewed PR #2713 (serve static assets with standard MIME types) and posted a batch review with a COMMENT verdict — no blocking issues found. What the PR does well:
Verifications I performed:
Two non-blocking nits raised inline:
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the MIME-type refactor. The core change is good: contentTypeForPath removes the duplicated path.extname(...).toLowerCase() / ?? "application/octet-stream" pattern at all four call sites, the case-insensitive matching is a real fix, and .wasm -> application/wasm correctly makes the existing COMPRESSIBLE_TYPES entry reachable.
Verified no regressions in adjacent behavior:
- Compression stays correct — every
COMPRESSIBLE_TYPES.has()check first doesct.split(";")[0].trim()(prod-server.ts:497,708,937), so the newly-added charsets don't break compression eligibility. - Image security preserved — the passthrough still gates on the
SAFE_IMAGE_CONTENT_TYPESallowlist (image-optimization.ts:219) viaisSafeImageContentType, which splits on;. The new non-image types (pdf, wasm, mp4, ...) can't leak through. Note.bmp->image/bmpnewly lets local.bmpimages through the image endpoint (previouslyapplication/octet-stream-> 400); this matches Next.js's allowlist so it's a parity improvement, not a regression. fonts.tsstill reads the exportedCONTENT_TYPESfor woff2/woff/ttf, which are unaffected.
One issue worth fixing before merge (charset casing), plus a couple of non-blocking nits below.
| ".csv": "text/csv; charset=UTF-8", | ||
| ".eot": "application/vnd.ms-fontobject", | ||
| ".gif": "image/gif", | ||
| ".svg": "image/svg+xml", | ||
| ".heic": "image/heic", | ||
| ".js": "application/javascript; charset=UTF-8", | ||
| ".mjs": "application/javascript; charset=UTF-8", | ||
| ".css": "text/css; charset=UTF-8", | ||
| ".html": "text/html; charset=UTF-8", |
There was a problem hiding this comment.
Charset casing regresses an existing value and diverges from the stated send/mime goal.
This PR changes the pre-existing .html entry from text/html; charset=utf-8 (lowercase) to text/html; charset=UTF-8 (uppercase). Every other charset in the codebase is lowercase utf-8 — there are ~28 occurrences (pages-page-response.ts, app-page-stream.ts, http-error-responses.ts, the prod-server 404s at prod-server.ts:1503/2016, etc.). So the newly-served .js/.css/.json assets will carry charset=UTF-8 while HTML page renders and error responses from the same server carry charset=utf-8 — inconsistent on the wire.
It also diverges from the PR's stated parity goal: send derives the charset via mime-types.contentType(), which lowercases it (... + '; charset=' + charset.toLowerCase()), so Next.js emits charset=utf-8. Recommend lowercasing all of these (and reverting the .html change) to restore both internal consistency and send parity:
| ".csv": "text/csv; charset=UTF-8", | |
| ".eot": "application/vnd.ms-fontobject", | |
| ".gif": "image/gif", | |
| ".svg": "image/svg+xml", | |
| ".heic": "image/heic", | |
| ".js": "application/javascript; charset=UTF-8", | |
| ".mjs": "application/javascript; charset=UTF-8", | |
| ".css": "text/css; charset=UTF-8", | |
| ".html": "text/html; charset=UTF-8", | |
| ".csv": "text/csv; charset=utf-8", | |
| ".eot": "application/vnd.ms-fontobject", | |
| ".gif": "image/gif", | |
| ".heic": "image/heic", | |
| ".js": "application/javascript; charset=utf-8", | |
| ".mjs": "application/javascript; charset=utf-8", | |
| ".css": "text/css; charset=utf-8", | |
| ".html": "text/html; charset=utf-8", |
(The same lowercasing should be applied to .json/.map/.txt below, and the corresponding test expectations in tests/serve-static.test.ts and tests/static-file-cache.test.ts.)
| ".pdf": "application/pdf", | ||
| ".png": "image/png", | ||
| ".rsc": "text/x-component", | ||
| ".svg": "image/svg+xml", |
There was a problem hiding this comment.
Parity nit (non-blocking): mime-db marks both image/svg+xml and application/xml with charset: UTF-8, so strict send/mime-types parity would emit image/svg+xml; charset=utf-8 and application/xml; charset=utf-8. As written they're bare, which is inconsistent with .csv/.txt/.html in the same map. Either add the charset or note the deliberate divergence. (isSafeImageContentType splits on ;, so adding it wouldn't affect the SVG security gate.)
| ".gif": "image/gif", | ||
| ".svg": "image/svg+xml", | ||
| ".heic": "image/heic", | ||
| ".js": "application/javascript; charset=UTF-8", |
There was a problem hiding this comment.
Style nit (non-blocking): the map is otherwise alphabetized, but .js/.mjs/.css/.html (lines 26-29) sit between .heic and .ico out of order. Moving them to their alphabetical slots keeps the convention consistent for future additions.
|
Review posted successfully to PR #2713. SummaryI reviewed the MIME-type refactor and posted a batch review (COMMENT verdict). The refactor itself is solid, but I found one issue worth fixing before merge that the prior review missed. Main finding — charset casing (
Non-blocking nits (posted inline):
Verifications that came back clean (no issues):
|
2 participants
Summary
Next.js parity
Next.js static serving delegates MIME lookup to its compiled send/mime database. This ports the relevant common web-asset behavior without adding a runtime dependency.
Test plan