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: fs-serve import graph awareness #3784

Merged
merged 11 commits into from
Jun 25, 2021
Merged

Conversation

antfu
Copy link
Member

@antfu antfu commented Jun 13, 2021

Description

resolve #2820, resolve #3373

  • Accessing restricted files now emits warning in console.
  • New server.fsServe.allow options is introduced to have multiple safe roots
  • Imports from "safe modules" are marked as "safe"

Additional context


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 Commit 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.

Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

Tests also seem to not pass 🙁

packages/vite/src/node/plugins/resolve.ts Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/resolve.ts Outdated Show resolved Hide resolved
packages/vite/src/node/server/middlewares/static.ts Outdated Show resolved Hide resolved
@antfu antfu changed the title fix: fs-serve import map @antfu fix: fs-serve import graph awareness Jun 13, 2021
@antfu antfu changed the title @antfu fix: fs-serve import graph awareness fix: fs-serve import graph awareness Jun 13, 2021
@antfu antfu marked this pull request as ready for review June 13, 2021 14:24
@antfu
Copy link
Member Author

antfu commented Jun 13, 2021

Force pushed to make commit history cleaner.

Code changes: 95dbcf4
Test: 3b8f5

@Shinigami92
Copy link
Member

Failing test on windows

@antfu
Copy link
Member Author

antfu commented Jun 13, 2021

Finally 🙃

Co-authored-by: Shinigami <chrissi92@hotmail.de>
Shinigami92
Shinigami92 previously approved these changes Jun 13, 2021
@patak-dev
Copy link
Member

Great to have proper tests for this feature now ❤️

  • Accessing restricted files now emits warning in console.

+1

  • New server.fsServe.dirs options is introduced to have multiple safe roots

If I recall correctly from Marvin, this is also the approach that WMR took. But I can't find the option in their docs now.

Does this mean that if you yarn link something, you need to add it to the dirs list? Doesn't these linked imports fall into the Imports from "safe modules" are marked as "safe"? If not, is there a way to make linked files also work?
Maybe by also marking as safe the real paths for symlinks (using realpath for example). If there is a performance penalty maybe this could be behind a fsServe.allowSymlinks options, but hoping that this could be on by default and people could opt-out (we are already using it for handling source maps, looks like it is included as a dependency that works better than node's default).

  • Currently this serves same functionality as root, should we unify them with allow: string | string[]?

I would prefer that we deprecate root and only include one option. Your proposal sounds good to me allow: string | string[], and it also leaves the door open to add allow: string | string[] | (path: string, server: ViteServer) => boolean and let people implement their own logic. I think this is good to enable users to customize fsServe without Vite taking extra complexity in core. And we could also add a fsServer.disallow if people want to ban certain directories. As an extra pro, we'll avoid future confusion between root and fsServe.root, but this is not a hard reason.

  • Imports from "safe modules" are marked as "safe"

This is awesome

@antfu
Copy link
Member Author

antfu commented Jun 14, 2021

Does this mean that if you yarn link something, you need to add it to the dirs list?

No, they should be automated marked as safe I think. The @vite/client thing is a bit special I guess. Tho I will try to find a way to make it more general so we can have this PR without new options and left it for the future PRs.

Imports from "safe modules" are marked as "safe"

This should work theoretically, however I can't still be 100% sure. It will be great if you can link it to your real project and test out :)

@antfu antfu added the p3-minor-bug An edge case that only affects very specific usage (priority) label Jun 14, 2021
@patak-dev
Copy link
Member

Tested this in windows in a Vite App and it works fine when yarn linking vite 👍🏼

@antfu
Copy link
Member Author

antfu commented Jun 15, 2021

After some digging, it's still not easy to generalize handling for @vite/client. So I think maybe it's worthwhile to introduce the multiple allow dir list in this PR.

As a result, I renamed it to fsServe.allow: string[] and soft deprecated the fsServe.root options. Still not sure if it's a good name tho.

packages/vite/src/node/server/index.ts Show resolved Hide resolved
Comment on lines 141 to +144
* Default to false at this moment, will enabled by default in the future versions.
*
* @expiremental
* @default undefined
Copy link
Member

Choose a reason for hiding this comment

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

Is the default false or undefined?

Copy link
Member Author

Choose a reason for hiding this comment

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

undefined. false to disable the warning

Copy link
Member

@Shinigami92 Shinigami92 Jun 15, 2021

Choose a reason for hiding this comment

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

Then we should change the descriptive text in line 141 to something less confusing

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point

packages/vite/src/node/server/index.ts Show resolved Hide resolved
@meteorlxy
Copy link
Contributor

This PR brings new options, so it would be better to mark as feat instead of fix

@antfu antfu changed the title fix: fs-serve import graph awareness feat: fs-serve import graph awareness Jun 15, 2021
@antfu antfu merged commit c45a02f into vitejs:main Jun 25, 2021
@antfu antfu deleted the fix/fs-serve-map branch June 25, 2021 18:06
@antfu antfu mentioned this pull request Oct 21, 2021
9 tasks
aleclarson pushed a commit to aleclarson/vite that referenced this pull request Nov 8, 2021
Co-authored-by: Shinigami <chrissi92@hotmail.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Internal allowlist of @fs serving files Unrestricted directory traversal with @fs
4 participants