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: allowed files logic (fix #5416) #5420

Merged
merged 9 commits into from Oct 26, 2021
Merged

Conversation

patak-dev
Copy link
Member

Description

We introduced a new restriction to served files in the serveStaticMiddleware in #5361
@benmccann reported an issue with this restriction in #5416 that we later expanded in a discussion in Vite Land. The problem with the new restriction is that it will restrict paths that don't exist in the filesystem. These paths can be an API call that should be later handled by user middlewares.

Instead of a hard error and 403, this PR takes a note from a @antfu here #5361 (comment). To avoid leaking file names information, it replaces errors by a call to next() so a restricted file and a file that doesn't exist look the same in the browser (404). An error is still emitted to the console.

File existence is also checked for the warning when strict isn't true. I think some false positives were due to this missing check.

Additional context

We should merge this PR and do another patch release before moving to the 2.7 beta.


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 25, 2021 22:39
@patak-dev patak-dev added the p4-important Violate documented behavior or significantly improves performance (priority) label Oct 25, 2021
@antfu
Copy link
Member

antfu commented Oct 25, 2021

I guess 404 is a more secure way to do to prevent file name testing. But given that we are flipping the default to strict, I am a bit concerned about the DX affected by this. With 403 we are able to provide some info in HTML as the response to show this is a feature of Vite. I am not confident that every user is aware of this restriction so I might think returning 404 could be potentially confusing and hard to figure out by the user.

Also, just wondering that's a potential security issue could be if we return 403 based on the file existence?

@patak-dev
Copy link
Member Author

Agree with you about DX being affected here. My worry was that we are leaking the existence of a given file, and that info could be useful. But it is true that this may not justify going against DX. I'll revert that change then so we can merge it

@patak-dev patak-dev requested a review from antfu October 26, 2021 05:46
Shinigami92
Shinigami92 previously approved these changes Oct 26, 2021
packages/vite/src/node/server/middlewares/static.ts Outdated Show resolved Hide resolved
Co-authored-by: Shinigami <chrissi92@hotmail.de>
Shinigami92
Shinigami92 previously approved these changes Oct 26, 2021
Co-authored-by: Shinigami <chrissi92@hotmail.de>
@patak-dev patak-dev merged commit 414bc45 into main Oct 26, 2021
@patak-dev patak-dev deleted the fix/allowed-files-logic branch October 26, 2021 15:45
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.

None yet

3 participants