From 118e36d3064b24be47dfe4522629c54e9d6f22f0 Mon Sep 17 00:00:00 2001 From: Ben McCann <322311+benmccann@users.noreply.github.com> Date: Fri, 14 Oct 2022 13:02:27 -0700 Subject: [PATCH 1/4] feat: handle static assets in case-sensitive manner --- packages/vite/src/node/preview.ts | 37 ++++++++++++------- .../src/node/server/middlewares/static.ts | 7 +++- packages/vite/src/node/utils.ts | 34 +++++++++++++++++ playground/assets/__tests__/assets.spec.ts | 7 ++++ 4 files changed, 70 insertions(+), 15 deletions(-) diff --git a/packages/vite/src/node/preview.ts b/packages/vite/src/node/preview.ts index d17fc0845326dc..36e47780234b5d 100644 --- a/packages/vite/src/node/preview.ts +++ b/packages/vite/src/node/preview.ts @@ -15,7 +15,7 @@ import { import { openBrowser } from './server/openBrowser' import compression from './server/middlewares/compression' import { proxyMiddleware } from './server/middlewares/proxy' -import { resolveHostname, resolveServerUrls } from './utils' +import { resolveHostname, resolveServerUrls, shouldServe } from './utils' import { printServerUrls } from './logger' import { resolveConfig } from '.' import type { InlineConfig, ResolvedConfig } from '.' @@ -112,21 +112,30 @@ export async function preview( // static assets const distDir = path.resolve(config.root, config.build.outDir) const headers = config.preview.headers - app.use( - previewBase, - sirv(distDir, { - etag: true, - dev: true, - single: config.appType === 'spa', - setHeaders(res) { - if (headers) { - for (const name in headers) { - res.setHeader(name, headers[name]!) - } + const assetServer = sirv(distDir, { + etag: true, + dev: true, + single: config.appType === 'spa', + setHeaders(res) { + if (headers) { + for (const name in headers) { + res.setHeader(name, headers[name]!) } } - }) - ) + } + }) + app.use(previewBase, async (req, res, next) => { + // TODO: why is this necessary? what's screwing up the request URL? + // tons of tests fail without this since we're receiving URLs like //assets/dep-42fa3c.js + const fixedUrl = req.url!.startsWith('//') + ? req.url!.substring(1) + : req.url! + const url = new URL(fixedUrl, 'http://example.com') + if (shouldServe(url, distDir)) { + return assetServer(req, res, next) + } + next() + }) // apply post server hooks from plugins postHooks.forEach((fn) => fn && fn()) diff --git a/packages/vite/src/node/server/middlewares/static.ts b/packages/vite/src/node/server/middlewares/static.ts index c877022a63cde7..07c45a4ee58bea 100644 --- a/packages/vite/src/node/server/middlewares/static.ts +++ b/packages/vite/src/node/server/middlewares/static.ts @@ -14,6 +14,7 @@ import { isInternalRequest, isParentDirectory, isWindows, + shouldServe, slash } from '../../utils' @@ -52,7 +53,11 @@ export function servePublicMiddleware( if (isImportRequest(req.url!) || isInternalRequest(req.url!)) { return next() } - serve(req, res, next) + const url = new URL(req.url!, 'http://example.com') + if (shouldServe(url, dir)) { + return serve(req, res, next) + } + next() } } diff --git a/packages/vite/src/node/utils.ts b/packages/vite/src/node/utils.ts index 2711a29b7eb703..649599fba52c79 100644 --- a/packages/vite/src/node/utils.ts +++ b/packages/vite/src/node/utils.ts @@ -1190,3 +1190,37 @@ export const isNonDriveRelativeAbsolutePath = (p: string): boolean => { if (!isWindows) return p.startsWith('/') return windowsDrivePathPrefixRE.test(p) } + +/** + * Determine if a file is being requested with the correct case, to ensure + * consistent behaviour between dev and prod and across operating systems. + */ +export function shouldServe(url: URL, assetsDir: string): boolean { + const pathname = decodeURIComponent(url.pathname) + const file = assetsDir + pathname + if ( + !fs.existsSync(file) || + (!fs.statSync(file).isDirectory() && + isCaseInsensitiveFS && // can skip case check on Linux + !hasCorrectCase(file, assetsDir)) + ) { + return false + } + return true +} + +/** + * Note that we can't use realpath here, because we don't want to follow + * symlinks. + */ +function hasCorrectCase(file: string, assets: string): boolean { + if (file === assets) return true + + const parent = path.dirname(file) + + if (fs.readdirSync(parent).includes(path.basename(file))) { + return hasCorrectCase(parent, assets) + } + + return false +} diff --git a/playground/assets/__tests__/assets.spec.ts b/playground/assets/__tests__/assets.spec.ts index 768fadf7f35ce5..f8246a4f3418b3 100644 --- a/playground/assets/__tests__/assets.spec.ts +++ b/playground/assets/__tests__/assets.spec.ts @@ -1,3 +1,4 @@ +import fetch from 'node-fetch' import { describe, expect, test } from 'vitest' import { browserLogs, @@ -12,6 +13,7 @@ import { readFile, readManifest, untilUpdated, + viteTestUrl, watcher } from '~utils' @@ -27,6 +29,11 @@ test('should have no 404s', () => { }) }) +test('should get a 404 when using incorrect case', async () => { + expect((await fetch(viteTestUrl + 'icon.png')).status).toBe(200) + expect((await fetch(viteTestUrl + 'ICON.png')).status).toBe(404) +}) + describe('injected scripts', () => { test('@vite/client', async () => { const hasClient = await page.$( From f5d053fb6811b9ed0c75f041c1283932c4d7bf28 Mon Sep 17 00:00:00 2001 From: Ben McCann <322311+benmccann@users.noreply.github.com> Date: Fri, 21 Oct 2022 10:29:04 -0700 Subject: [PATCH 2/4] chore: cleanup todo --- packages/vite/src/node/preview.ts | 8 +------- packages/vite/src/node/server/middlewares/static.ts | 3 +-- packages/vite/src/node/utils.ts | 9 +++++++-- 3 files changed, 9 insertions(+), 11 deletions(-) diff --git a/packages/vite/src/node/preview.ts b/packages/vite/src/node/preview.ts index 36e47780234b5d..8148b34110022b 100644 --- a/packages/vite/src/node/preview.ts +++ b/packages/vite/src/node/preview.ts @@ -125,13 +125,7 @@ export async function preview( } }) app.use(previewBase, async (req, res, next) => { - // TODO: why is this necessary? what's screwing up the request URL? - // tons of tests fail without this since we're receiving URLs like //assets/dep-42fa3c.js - const fixedUrl = req.url!.startsWith('//') - ? req.url!.substring(1) - : req.url! - const url = new URL(fixedUrl, 'http://example.com') - if (shouldServe(url, distDir)) { + if (shouldServe(req.url!, distDir)) { return assetServer(req, res, next) } next() diff --git a/packages/vite/src/node/server/middlewares/static.ts b/packages/vite/src/node/server/middlewares/static.ts index 07c45a4ee58bea..8f6b47f3a7e4bb 100644 --- a/packages/vite/src/node/server/middlewares/static.ts +++ b/packages/vite/src/node/server/middlewares/static.ts @@ -53,8 +53,7 @@ export function servePublicMiddleware( if (isImportRequest(req.url!) || isInternalRequest(req.url!)) { return next() } - const url = new URL(req.url!, 'http://example.com') - if (shouldServe(url, dir)) { + if (shouldServe(req.url!, dir)) { return serve(req, res, next) } next() diff --git a/packages/vite/src/node/utils.ts b/packages/vite/src/node/utils.ts index 649599fba52c79..35cea4592cc79a 100644 --- a/packages/vite/src/node/utils.ts +++ b/packages/vite/src/node/utils.ts @@ -1195,8 +1195,13 @@ export const isNonDriveRelativeAbsolutePath = (p: string): boolean => { * Determine if a file is being requested with the correct case, to ensure * consistent behaviour between dev and prod and across operating systems. */ -export function shouldServe(url: URL, assetsDir: string): boolean { - const pathname = decodeURIComponent(url.pathname) +export function shouldServe(url: string, assetsDir: string): boolean { + // viteTestUrl is set to something like http://localhost:4173/ and then many tests make calls + // like `await page.goto(viteTestUrl + '/example')` giving us URLs beginning with a double slash + const pathname = decodeURIComponent( + new URL(url.startsWith('//') ? url.substring(1) : url, 'http://example.com') + .pathname + ) const file = assetsDir + pathname if ( !fs.existsSync(file) || From 1e6e8349014cdd94081bf0c8bff035294037c234 Mon Sep 17 00:00:00 2001 From: Ben McCann <322311+benmccann@users.noreply.github.com> Date: Fri, 11 Nov 2022 08:23:46 -0800 Subject: [PATCH 3/4] Update packages/vite/src/node/utils.ts Co-authored-by: Bjorn Lu --- packages/vite/src/node/utils.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/vite/src/node/utils.ts b/packages/vite/src/node/utils.ts index e2c9a761eb88ba..596d8210831f56 100644 --- a/packages/vite/src/node/utils.ts +++ b/packages/vite/src/node/utils.ts @@ -1206,8 +1206,8 @@ export function shouldServe(url: string, assetsDir: string): boolean { const file = assetsDir + pathname if ( !fs.existsSync(file) || - (!fs.statSync(file).isDirectory() && - isCaseInsensitiveFS && // can skip case check on Linux + (isCaseInsensitiveFS && // can skip case check on Linux + !fs.statSync(file).isDirectory() && !hasCorrectCase(file, assetsDir)) ) { return false From 14a0b50a74c981ec16a45ee2fa84ee5ad74304d9 Mon Sep 17 00:00:00 2001 From: Ben McCann <322311+benmccann@users.noreply.github.com> Date: Fri, 11 Nov 2022 10:05:44 -0800 Subject: [PATCH 4/4] chore: use path.join --- packages/vite/src/node/utils.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/vite/src/node/utils.ts b/packages/vite/src/node/utils.ts index 596d8210831f56..12e6da1dcd5b3d 100644 --- a/packages/vite/src/node/utils.ts +++ b/packages/vite/src/node/utils.ts @@ -1199,11 +1199,11 @@ export const isNonDriveRelativeAbsolutePath = (p: string): boolean => { export function shouldServe(url: string, assetsDir: string): boolean { // viteTestUrl is set to something like http://localhost:4173/ and then many tests make calls // like `await page.goto(viteTestUrl + '/example')` giving us URLs beginning with a double slash - const pathname = decodeURIComponent( + const pathname = decodeURI( new URL(url.startsWith('//') ? url.substring(1) : url, 'http://example.com') .pathname ) - const file = assetsDir + pathname + const file = path.join(assetsDir, pathname) if ( !fs.existsSync(file) || (isCaseInsensitiveFS && // can skip case check on Linux