Builder-Vite: Fix defaulting to allowing all hosts#30523
Conversation
JReinhold
added
bug
patch:yes
|
View your CI Pipeline Execution ↗ for commit 65548b1.
☁️ Nx Cloud last updated this comment at |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
| @@ -31,9 +31,9 @@ export async function createViteServer(options: Options, devServer: Server) { | |||
|
|
|||
| const ipRegex = /^(?:\d{1,3}\.){3}\d{1,3}$|^(?:[a-fA-F0-9]{1,4}:){7}[a-fA-F0-9]{1,4}$/; | |||
There was a problem hiding this comment.
style: The IP regex will match invalid IPs like 999.999.999.999. Consider using a more robust IP validation method.
| const ipRegex = /^(?:\d{1,3}\.){3}\d{1,3}$|^(?:[a-fA-F0-9]{1,4}:){7}[a-fA-F0-9]{1,4}$/; | |
| const ipRegex = /^(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$|^(?:[a-fA-F0-9]{1,4}:){7}[a-fA-F0-9]{1,4}$/; |
There was a problem hiding this comment.
This regex gets too complex. Please add a comment of what it actually should match.
There was a problem hiding this comment.
I'd love to, but I didnt write it so I dont know 🤷♀️
There was a problem hiding this comment.
@valentinpalkovic @JReinhold I wrote the original PR that introduced this. The reason for the IP Regex is to identify if an ip address was used for the host, instead of a hostname, with the logic to set allowedHosts: true if an ip address was used instead of a hostname. The Regex is for any IPv4 or IPv6 ip address. Note, it looks like this logic was removed, and now no allowed host is set when the host is an ip address, this may be a logical flaw, my original issue was for when I was binding an ip address '0.0.0.0' to host that I couldn't access storybook with a hostname.
There was a problem hiding this comment.
Thanks for the explanation @JSMike. ❤️
Could you open a PR adding the required logic back, with an explanation of which problem it solves? Sounds like you know a lot about this problem already.
| document.getElementById('storybook-root').innerHTML = | ||
| `<p style="color: red; max-width: 70ch">${message.replaceAll( | ||
| '\n', | ||
| '<br/>' | ||
| )}<ul>${docs.map((doc) => `<li><a href='${doc}' target='_blank'>${doc}</a></li>`).join('')}<ul></p>`; |
There was a problem hiding this comment.
logic: HTML injection could be unsafe if hostname contains malicious content. Consider sanitizing hostname before inserting into innerHTML
| document.getElementById('storybook-root').innerHTML = | |
| `<p style="color: red; max-width: 70ch">${message.replaceAll( | |
| '\n', | |
| '<br/>' | |
| )}<ul>${docs.map((doc) => `<li><a href='${doc}' target='_blank'>${doc}</a></li>`).join('')}<ul></p>`; | |
| const sanitizedHostname = hostname.replace(/[<>"'&]/g, ''); // Basic HTML escaping | |
| document.getElementById('storybook-root').innerHTML = | |
| `<p style="color: red; max-width: 70ch">${message.replaceAll( | |
| '\n', | |
| '<br/>' | |
| ).replace(hostname, sanitizedHostname)}<ul>${docs.map((doc) => `<li><a href='${doc}' target='_blank'>${doc}</a></li>`).join('')}</ul></p>`; |
valentinpalkovic
changed the title
shilman
left a comment
shilman
left a comment
There was a problem hiding this comment.
Thanks so much @JReinhold !!! Perfect is the enemy of the good 😉
shilman
added
ci:normal
JReinhold
added
ci:normal
…:storybookjs/storybook into jeppe/partially-revert-vite-allowedhosts
…e-allowedhosts Builder-Vite: Fix defaulting to allowing all hosts (cherry picked from commit 463636d)
github-actions
Bot
added
the
patch:done
|
I'm still seeing this bug, here's a link to reproduce it: Has this patch been released? |
|
@lorenzo-del-rosario I don't think that's related. AFAIK the work here has nothing to do with CORS. |
ndelangen
removed
the
patch:yes
6 participants
What I did
This PR partially reverts #30432 . The default behavior is now to follow Vite, in not allowing any hosts but
localhost. However it's still possible to overwrite this with the--hostCLI flag or inviteFinal.It shows an error crude message in the iframe, but only when visiting the
iframe.htmldirectly, because the manager's loading spinner obscures the iframe:Therefore, it shows the same message in the console, both in the iframe and the manager:
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
/etc/hostsfile (sudo required) and add the following line:127.0.0.1 my-custom.locallocalhost:6006and see that everything worksmy-custom.local:6006and see that the story loads infinitely, but you get the error message in the console.my-custom.local:6006/iframe.htmland see that you get the error message in the DOMyarn storybook:ui --host my-custom.locallocalhost:6006and see that everything worksmy-custom.local:6006and see that everything workscode/.storybook/main.tsand addallowedHosts: ['my-custom.local']to theserverproperty in theviteFinalhook.--hostflaglocalhost:6006and see that everything worksmy-custom.local:6006and see that everything worksThis 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
Based on the provided information, I'll create a concise summary of the key changes in this PR:
This PR modifies Vite server configuration in Storybook to improve security by restricting allowed hosts to localhost by default.
code/builders/builder-vite/src/vite-server.tsto follow Vite's default secure behavior of only allowing localhost accesscode/builders/builder-vite/input/iframe.htmlto display helpful messages when accessing from non-allowed hosts--hostCLI flag orviteFinalconfigurationThe changes represent a security improvement while maintaining flexibility for developers who need to access Storybook from non-localhost addresses.