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

feat: support files for fs.allow #12863

Merged
merged 10 commits into from Jun 6, 2023
Merged

feat: support files for fs.allow #12863

merged 10 commits into from Jun 6, 2023

Conversation

Ph0tonic
Copy link
Contributor

@Ph0tonic Ph0tonic commented Apr 14, 2023

Description

It's the first time I try to contribute to this project, help is welcome to make it go through. I tried to address issue raised by #5689. So to support file in fs.allow instead of only directories.

I have unfortunately no idea where to write a test for this feature.

Additional context

Fixes #5689


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@stackblitz
Copy link

stackblitz bot commented Apr 14, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@Ph0tonic Ph0tonic marked this pull request as ready for review April 21, 2023 12:37
antfu
antfu previously approved these changes Apr 21, 2023
Copy link
Member

@antfu antfu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The feature make sense to me

@patak-dev patak-dev added the p2-nice-to-have Not breaking anything but nice to have (priority) label Apr 21, 2023
@patak-dev patak-dev added this to the 4.4 milestone Apr 27, 2023
@patak-dev
Copy link
Member

About #12863, it is an edge case but if we had:

fs: {
  allow: [
    '/path/to/custom/allow'

Before this would allow files inside the /path/to/custom/allow folder. After #12863, it will also start allowing a file named /path/to/custom/allow
I don't think nobody will place secrets in a file named as the folder they are allowing but given the security nature of the feature, maybe we should push this PR to Vite 5 instead of merging it in a minor?
I wonder if there is a way for the API to be more explicit too, maybe:

js: {
  allow: {
    dirs: [],
    files: []

In which case we can go ahead with it in 4.4 and we avoid the path as a folder or dir issue. Let's discuss this before merging it.

@ArnaudBarre
Copy link
Member

Does some OS allow to have a folder and file with the same path? Otherwise it would be pretty bad that you added a secret file to a list of allow without any benefits because there is no folder to serve

@patak-dev
Copy link
Member

No, it is as you said, it will allow a file with that name if instead of a folder you have a file named as the path in allow. But seeing it written as in your last comment, I agree it doesn't make any sense for a user to do this. I'm ok merging this PR as is for 4.4 then if others think we are good here.

@bluwy
Copy link
Member

bluwy commented Jun 6, 2023

I agree with going ahead with this too. If they allow /path/to/custom/allow to be served, I think the intent is also that the allow file (if it's named that way) can be served too.

@patak-dev patak-dev removed the on hold label Jun 6, 2023
@patak-dev patak-dev merged commit 4a06e66 into vitejs:main Jun 6, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Vite fs allow not respecting individual files
5 participants