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

Tweak Vite setup so we use the correct working directory #28

Merged
merged 3 commits into from
Jun 15, 2021

Conversation

eirslett
Copy link
Collaborator

@eirslett eirslett commented Jun 11, 2021

We use a heuristic: assume that the working directory/project root for Storybook is the parent directory of options.configDir. I don't think there's a config variable to specify the exact root directory. This lets us tighten security by turning on fsserve.strict.

https://vitejs.dev/config/#server-fsserve-strict

If the heuristic is not good enough to detect people's working directories, they can override it in viteFinal().

@eirslett eirslett requested review from shilman and IanVS June 11, 2021 21:54
Copy link
Member

@IanVS IanVS left a comment

Choose a reason for hiding this comment

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

I've confirmed that the default cache is now being placed in my node_modules/.vite, as I would expect. That alone makes this a big win, I think.

What is the best way to test that the added security of using fsserve.strict is working as intended?

And, I think that before we merge this, there should be some updates to the readme to mention this behavior (the heuristic) and suggest ways to override it if necessary. I can see this being the kind of "magic" that could cause problems if people don't know about it.

// We create a kind of "custom" source root inside this project (yes, inside the node_modules folder)
// so that "iframe.html" resolves to a correct path. (Otherwise, Vite will fail.)
root: path.resolve(__dirname, 'input'),
root: path.resolve(options.configDir, '..'),
Copy link
Member

Choose a reason for hiding this comment

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

I wonder what is a better assumption, this, or cwd. 🤔. Probably this.

@eirslett
Copy link
Collaborator Author

What is the best way to test that the added security of using fsserve.strict is working as intended?

I think we have to test it manually. I tried running Storybook locally with fsserve.strict disabled - I could some files in my filesystem that were no longer possible to access after enabling the setting.

And, I think that before we merge this, there should be some updates to the readme to mention this behavior (the heuristic) and suggest ways to override it if necessary.

Good point, I added some docs now!

@eirslett eirslett requested a review from IanVS June 15, 2021 19:43
README.md Outdated
## Note about working directory

The builder will by default enable Vite's [server.fsServe.strict](https://vitejs.dev/config/#server-fsserve-strict)
option, for increased security. The default `server.fsServe.root` is set to the parent directory of the
Copy link
Member

Choose a reason for hiding this comment

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

It looks like root is being set at the root of the config, not nested under server.fsServe, isn't it? Shouldn't they be changing their top-level root if we guess it wrong?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right! There are two root config options (one top-level and one under server.fsServe) and they behave a bit differently. I updated the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants