[Feature Request] Internal allowlist of `@fs` serving files

meteorlxy commented 3 years ago

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 https://github.com/vitejs/vite/pull/3321#issuecomment-838171665 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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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[bot] commented 3 years ago

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

vitejs / vite