Skip to content

Builder-Vite: Fix defaulting to allowing all hosts #30523

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.

Sign up for GitHub

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

Merged
JReinhold merged 6 commits into from
Feb 14, 2025
Merged

Builder-Vite: Fix defaulting to allowing all hosts #30523

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8b0fb30
don't default to server.allowedHosts = true
JReinhold Feb 12, 2025
ec1ef51
show error log in console and DOM when vite app could not load
JReinhold Feb 13, 2025
de265a7
Merge branch 'next' into jeppe/partially-revert-vite-allowedhosts
JReinhold Feb 13, 2025
e549d42
Merge branch 'next' into jeppe/partially-revert-vite-allowedhosts
shilman Feb 13, 2025
fc7404d
fix lint and check
JReinhold Feb 14, 2025
65548b1
Merge branch 'jeppe/partially-revert-vite-allowedhosts' of github.com…
JReinhold Feb 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 28 additions & 1 deletion code/builders/builder-vite/input/iframe.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,33 @@
<!-- [BODY HTML SNIPPET HERE] -->
<div id="storybook-root"></div>
<div id="storybook-docs"></div>
<script type="module" src="virtual:/@storybook/builder-vite/vite-app.js"></script>
<script>
// eslint-disable-next-line no-underscore-dangle, @typescript-eslint/naming-convention
function __onViteAppLoadingError(event) {
const hostname = globalThis.location.hostname;
if (hostname !== 'localhost' && globalThis.CONFIG_TYPE === 'DEVELOPMENT') {
const message = `Failed to load the Storybook preview file 'vite-app.js':

It looks like you're visiting the Storybook development server on another hostname than localhost: '${hostname}', but you haven't configured the necessary security features to support this.
Please re-run your Storybook development server with the '--host ${hostname}' flag, or manually configure your Vite allowedHosts configuration with viteFinal.

See:`;
const docs = [
'https://storybook.js.org/docs/api/cli-options#dev',
'https://storybook.js.org/docs/api/main-config/main-config-vite-final',
'https://vite.dev/config/server-options.html#server-allowedhosts',
];
console.error(`${message}\n${docs.map((doc) => `- ${doc}`).join('\n')}`);

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>`;
return;
}
}
</script>
<script type="module" src="virtual:/@storybook/builder-vite/vite-app.js" onerror="__onViteAppLoadingError(event)"></script>
</body>
</html>
10 changes: 7 additions & 3 deletions code/builders/builder-vite/src/vite-server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,13 @@ 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}$/;
Copy link
Contributor

@greptile-apps greptile-apps bot Feb 13, 2025

Choose a reason for hiding this comment

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

style: The IP regex will match invalid IPs like 999.999.999.999. Consider using a more robust IP validation method.

Suggested change
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}$/;

Copy link
Contributor

@valentinpalkovic valentinpalkovic Feb 13, 2025

Choose a reason for hiding this comment

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

This regex gets too complex. Please add a comment of what it actually should match.

Copy link
Contributor Author

@JReinhold JReinhold Feb 13, 2025

Choose a reason for hiding this comment

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

I'd love to, but I didnt write it so I dont know 🤷‍♀️

Copy link
Contributor

@JSMike JSMike May 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

@JReinhold JReinhold May 8, 2025

Choose a reason for hiding this comment

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

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.


config.server.allowedHosts =
commonCfg.server?.allowedHosts ??
(options.host && !ipRegex.test(options.host) ? [options.host.toLowerCase()] : true);
if (
!(config.server.allowedHosts as string[])?.length &&
options.host &&
!ipRegex.test(options.host)
) {
config.server.allowedHosts = [options.host.toLowerCase()];
}

const finalConfig = await presets.apply('viteFinal', config, options);

Expand Down