Builder-Vite: Add logic to set allowedHosts #30432
Open
+11
−5
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![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
There was a problem hiding this comment.
LGTM
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
|
View your CI Pipeline Execution ↗ for commit 7d4b70c.
☁️ Nx Cloud last updated this comment at |
JSMike
force-pushed
the
issue-30338-vite-allowedHosts
branch
from
9645c02 to
a58dc85
Compare
JSMike
commented
Jan 31, 2025
| @@ -29,6 +29,12 @@ export async function createViteServer(options: Options, devServer: Server) { | |||
| optimizeDeps: await getOptimizeDeps(commonCfg, options), | |||
| }; | |||
|
|
|||
| const ipRegex = /^(?:\d{1,3}\.){3}\d{1,3}$|^(?:[a-fA-F0-9]{1,4}:){7}[a-fA-F0-9]{1,4}$/; | |||
|
|
|||
| config.server.allowedHosts = | |||
There was a problem hiding this comment.
Logic overview:
- If the vite config sets
server.allowedHoststhen use that value. - If
options.hostwas passed and is not an IP address then restrict to only allow the hostnameoptions.host. This needs to be lowercase or it will fail to resolve, so cast to lowercase. - Otherwise default to allow any hostname.
shilman
reviewed
Feb 2, 2025
There was a problem hiding this comment.
@JSMike Thanks so much for putting this together!!
A few questions:
- Have you tested with an older version of Vite? Will it just ignore the option in that case?
- In the fallback case of
allowedHosts = true, is that a security issue? Or it doesn't really apply because we're only running in dev?
shilman
added
maintenance
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Update yarn lock to use version of
vitethat hasallowedHostsinServerOptions.Add logic to pass forward
server.allowedHostsfrom user's vite config, otherwise check if server is bound to value that should defaultallowedHoststotrue, otherwise use setallowedHoststo contain specifiedhost.Closes #30338
What I did
Add logic to define
allowedHostseither by passing forward config from user's vite config, or setting default. This required updating some of the yarn lock to use a version that contained the type forallowedHosts, but remained in respected semver range. This change should have no impact on prior versions because theallowedHostsvalue will be ignored by vite.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!
yarn task --task sandbox --start-from auto --template lit-vite/default-tshostnameinstead oflocalhost, by default this should load as expected.{ server: { allowedHosts: ['localhost'] } }yarn run storybookfrom your sandboxhostnameas the URL and the preview should fail to load, as expected, since it is no longer an allowed hostname.localhostas the URL and the preview should load.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
Let me provide a clear and focused summary of this pull request:
Adds logic to handle
allowedHostsconfiguration in Vite server setup to fix hostname accessibility issues in Storybook after Vite 6 upgrade.allowedHostsconfiguration incode/builders/builder-vite/src/vite-server.tsto preserve user-defined settingsallowedHosts: truefor common local addresses (::/0.0.0.0/127.0.0.1)allowedHostsarray when not using common local addressesServerOptionsandAddressInfointerfacesThe changes fix issue #30338 where Storybook was inaccessible via local hostnames after upgrading to Vite 6.
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!