Skip to content

feat: support files for fs.allow#12863

Merged
patak-cat merged 10 commits intovitejs:mainfrom
Ph0tonic:main
Jun 6, 2023
Merged

feat: support files for fs.allow#12863
patak-cat merged 10 commits intovitejs:mainfrom
Ph0tonic:main

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.

@bolt-new-by-stackblitz
Copy link

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-cat patak-cat added the p2-nice-to-have Not breaking anything but nice to have (priority) label Apr 21, 2023
@patak-cat patak-cat added this to the 4.4 milestone Apr 27, 2023
@patak-cat
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-cat
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-cat patak-cat removed the on hold label Jun 6, 2023
@patak-cat patak-cat merged commit 4a06e66 into vitejs:main Jun 6, 2023
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