fix(dev): denied requests overly

[sapphi-red]

Description

This line was not matching.

vite/packages/vite/src/node/server/middlewares/static.ts

Line 281 in 6bc8bf6

if (fs.allow.some((uri) => isUriInFilePath(uri, filePath))) return true

• 9df604b: added some comments and renamed some vars to make it clear that some function receives normalized absolute paths (this leads to requests denied more than needed. this does not lead to requests allowed more than needed)
• f73e0c6: removed the check against non-resolved ids as this was no longer needed
• feb3040: changed from checkServingAccess to checkLoadingAccess for deniedServingAccessForTransform as this function receives a path instead of an URL (this leads to requests denied more than needed. this does not lead to requests allowed more than needed)
• 8fe09fd: normalized paths before calling isFileLoadingAllowed (this isn't needed for the fix, but included as it's related to the change in 9df604b)

fixes #19795
fixes #20408
refs #20144

[patak-dev]

/ecosystem-ci run

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/vite@20410

commit: 8fe09fd

[SystemParadox]

I'm just an outside observer and not intimitely familar with all the specifics here so maybe I'm missing something, but I still seriously question why there should be any mention of svgRE in the transform middleware. What is anything specific to SVGs doing here?

The only reason this seems to be here is because the SVG handler further down isn't properly checking the requested path before reading the file on disk. So again, either this is a generic path security problem that should be fixed in a generic way and shouldn't have any mention of SVGs, or it's a specific problem with the SVG handler and it should be fixed there.

[patak-dev]

I'm just an outside observer and not intimitely familar with all the specifics here so maybe I'm missing something, but I still seriously question why there should be any mention of svgRE in the transform middleware. What is anything specific to SVGs doing here?

The only reason this seems to be here is because the SVG handler further down isn't properly checking the requested path before reading the file on disk. So again, either this is a generic path security problem that should be fixed in a generic way and shouldn't have any mention of SVGs, or it's a specific problem with the SVG handler and it should be fixed there.

There are two options. One is stopping denied files from ever entering the pipeline, and the other one is as you say, give the responsibility to every plugin that loads from the file system to do the check.

The idea with adding the check before entering the pipeline was that it would make plugins more secure. The check rawRE.test(id) || urlRE.test(id) || inlineRE.test(id) || svgRE.test(id) was what was needed for internal plugins, but the long term plan was to remove this condition and test every non-virtual id (and we could even remove this check

vite/packages/vite/src/node/server/transformRequest.ts

Line 285 in 6bc8bf6

isFileLoadingAllowed(environment.getTopLevelConfig(), file)

).

This was also added in response to CVEs, so minimal changes were applied because we needed to release them in patches. I don't know now if the long term plan to remove the condition is feasible because there are virtual plugins that don't properly mark their ids with \0 though, and I think it is still a possibility that the current check is removed in favor of moving the checks before each load.

[bluwy]

Changes make sense to me. Nice to see some cleanups too.

[vite-ecosystem-ci]

📝 Ran ecosystem CI on 8fe09fd: Open

suite	result	latest scheduled
histoire	❌ failure	❌ failure
one	❌ failure	❌ failure
vike	❌ failure	❌ failure
vite-plugin-react	✅ success	❌ failure
vite-plugin-rsc	✅ success	❌ failure

✅ nuxt, laravel, astro, analogjs, qwik, vite-plugin-pwa, unocss, quasar, marko, react-router, vite-plugin-cloudflare, vite-setup-catalogue, sveltekit, vite-plugin-vue, storybook, rakkas, waku, vite-environment-examples, vitepress, ladle, vite-plugin-svelte, vuepress, vitest

[patak-dev]

Probably good to wait for a minor for this one?