-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Conversation
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? |
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 |
Co-authored-by: Shinigami <[email protected]>
Co-authored-by: Shinigami <[email protected]>
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?