Builder-Vite: Fix logic related to setting allowedHosts when IP address used #31472
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
| !ipRegex.test(options.host) | ||
| ) { | ||
| config.server.allowedHosts = [options.host.toLowerCase()]; | ||
| if (options.host && !config.server.allowedHosts) { |
There was a problem hiding this comment.
style: Check for undefined/null specifically rather than falsy values to avoid overriding empty arrays
There was a problem hiding this comment.
Empty array should likely be overridden if options.host is set.
|
@JReinhold @valentinpalkovic https://github.com/storybookjs/storybook/pull/30523/files#r2079670800
|
There was a problem hiding this comment.
1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
|
|
||
| if (ipRegex.test(options.host)) { | ||
| // If options.host is set to an IP address then allow all hosts | ||
| config.server.allowedHosts = true; |
There was a problem hiding this comment.
style: Consider adding a warning log when allowing all hosts, as this reduces security
There was a problem hiding this comment.
A warning here would be appropriate, let me know what you'd like the warning message to be.
Or, if you'd prefer to not set allowedHosts to true then this would be a good place to add a warning to inform users that only the direct IP address and localhost will be allowed and that other hostnames will be rejected unless an allowedHosts array is added to the viteFinal.
|
View your CI Pipeline Execution ↗ for commit e0104b5.
☁️ Nx Cloud last updated this comment at |
JSMike
force-pushed
the
builder-vite-allowedhosts
branch
4 times, most recently
from
a7205f3 to
529154e
Compare
JSMike
force-pushed
the
builder-vite-allowedhosts
branch
3 times, most recently
from
7185d02 to
e80df3b
Compare
JSMike
force-pushed
the
builder-vite-allowedhosts
branch
from
e80df3b to
056c553
Compare
JSMike
force-pushed
the
builder-vite-allowedhosts
branch
from
056c553 to
e0104b5
Compare
Related to #30523, #30432, and #30338
What I did
Previous PR set
allowedHoststotrueby default, this was reverted but no longer set theallowedHostsvalue when an IP address was used foroptions.host.This change sets
allowedHoststotrueonly ifallowedHostsis undefined and an IP address is passed as the host. Otherwise no hostname other than 'localhost' would be allowed to access the devserver.Note, this is my personal opinion on what the logic should be, to more closely align with existing functionality prior to the breaking change from Vite. If, for security reasons, this is not desired then updating the PR to add a warning to the console would be another good option, along with additional documentation on setting
allowedHostswhen using the Vite dev-server with Storybook.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>Greptile Summary
This PR enhances Vite server security in Storybook by implementing stricter host access controls while maintaining flexibility for custom configurations.
code/builders/builder-vite/src/vite-server.tsto restrict access to localhost by default, following Vite's secure practices--hostCLI flag orviteFinalconfigThe changes improve security while maintaining backward compatibility and providing clear paths for customization when needed.
💡 (3/5) Reply to the bot's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!