fix(serve): prevent serving unrestricted files

[underfin]

fix: #3281

Description

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.

[lovasoa]

This breaks legitimate usecases when one wants to serve a folder that contains an explicit symlink to another one that is outside the server root.

[antfu]

@lovasoa You can configure it by https://vitejs.dev/config/#server-fsserve-root

[lovasoa]

Yes, but there is no reason to break existing setups in a minor version for something that is not a security vulnerability.

[antfu]

We have explained that in the changelog, and also see #2820. Sorry for the inconvenience, but we do think this is a serious security vulnerability.

[lovasoa]

This breaks existing monorepo setups, and serving files that are explicitly symlinked is not a security vulnerability.

Plus, how would I do if I had symlinked /a/b/c to /d/e/f, and was serving /d/e ? I would have to set the server.fsServe.root to / ? That would be a security issue.

[lovasoa]

we do think this is a serious security vulnerability.

To be clear: I also do agree that the previous behavior of serving any files from anywhere is a serious security vulnerability. But serving files that have been explicitly symlinked in the server root (and in node_modules in particular), is not a vulnerabilty.

[antfu]

Sure, this could indeed be overkill for some scenarios. But at this moment, I kinda think the trade-off is totally worth it. We could not figure out all the edge cases or do the assumptions about which files are safe and which are not.

So, we do open up for contributions to propose more fine-grained configurations API, for example like server.fsServe.allow and server.fsServe.disallow. Or in your case, we could find a way to let Vite detect those symlinks smartly and relaxes the restriction of them.

But that said, they all require some certain discussions, decision making, implementations and testing. The current approach is what we could have to deliver the security fix to users asap. Hope this could make me clear. Thanks

[lovasoa]

I do understand that this fix had to be rushed in order to patch the security issue quickly. Can I open a new issue to track a better fix that would avoid breakage for existing configurations ? Ideally, this should be fixed quickly before more people upgrade to v2.3.x and find themselves fix a broken configuration...

I don't think there is a need for an explicit allow and disallow list, symlinks are a more general solution to the problem.

[antfu]

Can I open a new issue to track a better fix that would avoid breakage for existing configurations ?

Sure, that would be great. Thanks

[lovasoa]

By the way, I love the quote on your github profile, and I think it applies well to this issue :

A ship in harbor is safe, but that is not what ships are built for.

😃

[lubomirblazekcz]

@antfu https://vitejs.dev/config/#server-fsserve-root this documentation is really not helping much, it's not clear from it what should I set in the first place if I want to expose some directories for serving

[lovasoa]

@evromalarkey : I agree that the documentation is confusing.

In the current setup, it should be set to the common parent of all the directories you want to be able to serve. So if you want to serve /home/you/common and your project is in /home/you/projects/myproject, then your vite.config.js config should look like this :

export default {
  server: {
    fsServe: {
      // Allow serving all files starting from two levels above the current directory in the fs hierarchy 
      root: '../../' 
    }
  }
}

[antfu]

Yeah, good point. Would you like to make a PR to update the docs? Thanks

[lubomirblazekcz]

Yeah I understand it now, thanks. Either way I think this change comes with a lot of problems, but it had to be done, I get it.