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

[Feature Request] Internal allowlist of @fs serving files #3373

Closed
meteorlxy opened this issue May 12, 2021 · 6 comments · Fixed by #3784
Closed

[Feature Request] Internal allowlist of @fs serving files #3373

meteorlxy opened this issue May 12, 2021 · 6 comments · Fixed by #3784

Comments

@meteorlxy
Copy link
Contributor

meteorlxy commented May 12, 2021

Description

Since v2.3.0, we restrcit access to filesystem to avoid secrity issues:

See #2820, #2850

However, this may cause potential breaks for projects using vite <2.3.0


There are some different types of fs url requests:

Type A - Files that users want to access intentionally.

For example, user writes import '/@fs/foo/bar/baz.js' explicitly in their code.

Users are responsible for their own code, so these kind of requests should NOT be restricted.

Setting server.fsServe.root could help, but we may only want to allow /@fs/foo/bar/baz.js file, instead of the whole parent path.

Type B - The resolved file path is outsite project root.

Example 1 (bare imports):

The project root is dev/:

export default {
  root: path.resolve(__dirname, 'dev')
}

import 'foo' in the dev/main.ts, then the resolved file is outside project root.

├── node_modules
|   └── foo         # resolved, outside project root
├── dev             # project root
|   ├── index.html
|   └── main.ts     # import 'foo' here
└── vite.config.ts

In addtion, npm link or npm install ../local-path should also cause this problem. (like #3347)

Example 2 (alias):

The project root is dev/, and set alias @some-alias:

export default {
  root: path.resolve(__dirname, 'dev')
  resolve: {
	alias: {
       '@some-alias': path.resolve(__dirname, 'src/index.ts')
    }
  }
}

import '@some-alias' in the dev/main.ts, then the resolved file is outside project root.

├── src
|   └── index.ts    # resolved, outside project root
├── dev             # project root
|   ├── index.html
|   └── main.ts     # import '@some-alias' here
└── vite.config.ts

These files are resolved according to the resolution rules, which should NOT be restricted.

Setting server.fsServe.root could help, but I think we should make it work by default without any extra configs.

Type C - Random traversal via url request

Visiting /@fs/ urls randomly, like #2820, #3281 reported.

This type of requests should be restricted.


I think Type C is the only thing that should be restricted by default, but now we simply restrict all fs requests outside project root.

Proposed Solution

As #3321 (comment) recommended, we can provide a configurable allowlist & disallowlist. This could help, but I think we should make Type A and Type B requests work out of the box.

I propose to also maintain an internal allowlist:

If a fs path is specified explicitly in source code (Type A), or resolved from the source code (Type B), we should add the path into the allowlist.

With the help of the internal allowlist, all intentional fs requests are allowed by default, and random visits are restricted.


In fact, Type A and Type B are somehow the same type: resolved fs path is outside the serve root - no matter it is wrote explicitly or resolved implicitly.

So we actually have two different situations:

  • Intentional fs requests from source code. (Allowed)
  • Unexpected fs requests from random visit. (Restricted)

In short, if we resolved a fs path from the source code, we should add it to the internal allowlist.

@antfu
Copy link
Member

antfu commented May 12, 2021

Sounds good to me. WDYT @vitejs/team?

For the case in type B (commonly see in vitepress), the workspace root should actually be resolved to . instead of ./dev (should serve the dir for the closest package.json if no workspace found)

@patak-dev
Copy link
Member

For type B, I agree, setting a root shouldn't affect the fsServe root.

For type A, AFAIU, users should not be using /@fs/ directly in their code, that is an undocumented internal feature, no? Still, if this is breaking things in the ecosystem because they are already using it, then it is good to allow it (but I don't understand why somebody would use this as the project will not be portable).

@antfu
Copy link
Member

antfu commented May 12, 2021

Still, if this is breaking things in the ecosystem because they are already using it

For the reference, I do that in Slidev to resolve paths from theme. Which is generated based on user's env. But this does not matter to Slidev as the themes are supposed to be lying under node_modules anyway.

@underfin
Copy link
Member

As #3321 (comment) recommended, we can provide a configurable allowlist & disallowlist

I think it is very useful for users.The users should know the loaction of requesting files and configure it if they want to access. Vite dev server should be serious for files access, I think it is a trade-off with work out of box.

For the case in type B (commonly see in vitepress), the workspace root should actually be resolved to . instead of ./dev (should serve the dir for the closest package.json if no workspace found)

@antfu. I don't think the work will help us out, it will add project maintainer cost because we may encouter another case like this. The allowList is enough for many case.

@meteorlxy
Copy link
Contributor Author

meteorlxy commented May 12, 2021

should serve the dir for the closest package.json if no workspace found

Yes, that would help for most cases, but we still need to handle linked deps and local deps.

users should not be using /@fs/ directly in their code, that is an undocumented internal feature, no?

Yes, but tools built on top fo vite may use it, (vitepress, vuepress-vite, slidev as @antfu mentioned) lol:

In fact, Type A and Type B are somehow the same type: resolved fs path is outside the serve root - no matter it is wrote explicitly or resolved implicitly.

In short, if we resolved a fs path from the source code, we should add it to the internal allowlist.

(appended part of this comments to the original issue)

@github-actions
Copy link

This issue gets locked because it has been closed for more than 14 days.

@github-actions github-actions bot locked and limited conversation to collaborators Jul 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants