Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: restrict static middleware fs access #5361

Merged
merged 7 commits into from Oct 23, 2021

Conversation

patak-dev
Copy link
Member

Description

Fix #5345

This PR modifies the fs-serve playground so the allowed folder is not the same as the root, so we can also test the static middleware file access.

@antfu there are two TODO: in the code:

 // TODO: should use ensureServingAccess(fileUrl, server)
 if (!isFileServingAllowed(fileUrl, server)) {
   return next()
 }

We should use ensureServingAccess so an AccessRestrictedError is thrown and catched later to respond with a 403, but when I tried this the program was terminating unexpectedly and I couldn't figure out why. The current code is skipping the url if it isn't allowed, so the response is a 404. We could evaluate merging as is, since this is a security vulnerability. And improve later, but maybe you see what I missed.

Maybe it isn't a bad idea to enqueue this change for the 2.7 beta if we are going to start the process soon.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@patak-dev patak-dev requested a review from antfu October 19, 2021 20:04
@patak-dev patak-dev added the p4-important Violate documented behavior or significantly improves performance (priority) label Oct 19, 2021
@benmccann
Copy link
Collaborator

benmccann commented Oct 19, 2021

Thank you so much for this fix!

As a higher-level note, having both the public and static middlewares means every public file is accessible via two different URLs, which isn't necessarily desirable. I wonder if it would be good to try to have just a single middleware for statically serving files. (This is all assuming the public directory lives within the project workspace, which is true for SvelteKit, but I'm not sure about Vite projects generally)

@antfu
Copy link
Member

antfu commented Oct 21, 2021

Guess we could release this as a part of 2.6 before we flip the default in 2.7

@AlbertMarashi
Copy link

@antfu sooner the better.

@benmccann benmccann mentioned this pull request Oct 21, 2021
9 tasks
Shinigami92
Shinigami92 previously approved these changes Oct 22, 2021
packages/vite/src/node/server/middlewares/static.ts Outdated Show resolved Hide resolved
@patak-dev patak-dev merged commit 1f4723b into main Oct 23, 2021
@patak-dev patak-dev deleted the fix/static-middleware-fs-access branch October 23, 2021 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p4-important Violate documented behavior or significantly improves performance (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[vulnerability] Vite serves files ignoring fs.allow setting
5 participants