[Bug]: Hardcoded host for server channel url prevents usage of custom host

[KhodeN]

Describe the bug

Code in storybook/code/lib/core-server/src/utils/server-address.ts has the method

export const getServerChannelUrl = (port: number, { https }: { https?: boolean }) => {
  return `${https ? 'wss' : 'ws'}://localhost:${port}/storybook-server-channel`;
};

with hardcoded host.

This code doesn't work for custom host (and SSL).
It generates the errors in console like:

WebSocket connection to 'wss://localhost:6006/storybook-server-channel' failed

To Reproduce

Just start storybook with custom `--host` and `--https`

System

Environment Info:

  System:
    OS: macOS 12.6.1
    CPU: (10) arm64 Apple M1 Pro
  Binaries:
    Node: 16.13.1 - ~/.nvm/versions/node/v16.13.1/bin/node
    npm: 8.1.2 - ~/.nvm/versions/node/v16.13.1/bin/npm
  Browsers:
    Firefox: 107.0
    Safari: 16.1
  npmPackages:
    @storybook/addon-actions: ^6.5.13 => 6.5.13 
    @storybook/addon-essentials: ^6.5.13 => 6.5.13 
    @storybook/addon-interactions: ^6.5.13 => 6.5.13 
    @storybook/addon-links: ^6.5.13 => 6.5.13 
    @storybook/builder-vite: ^0.2.5 => 0.2.5 
    @storybook/react: ^6.5.13 => 6.5.13 
    @storybook/testing-library: ^0.0.13 => 0.0.13 
    @storybook/testing-react: ^1.3.0 => 1.3.0

Additional context

No response

[krossekrabbe]

Can confirm, the WSS port and host must be configurable individually and independent from the server host and port!

If sb runs behind a proxy / load balancer, you might want the server to run on localhost:8080. That does not mean that WSS requests made by the browser should go to wss://localhost:8080. Instead you want your proxy / loadbalancer here, e.g. wss://storybook.domain:443 which then redirects traffic internally to the storybook server on 8080.

[medihack]

I can confirm this, too. I would like to use Storybook in a Cloud IDE (Gitpod or Github Codespaces), but WSS needs to be configurable for that.

[shilman]

cc @tmeasday

[tmeasday]

Seems like a simple change to read host from options in getServerChannelUrl if set. Would someone like to send a PR?

[medihack]

I think we should not only make it configurable, but also give it a better default (not just a fixed localhost). For example, in Next.js it is also not configurable, but has a nice default (at least I never had trouble with it). I took a quick look at the source code of how they do it (simply searched for "wss"):

getSocketProtocol

export function getSocketProtocol(assetPrefix: string): string {
  let protocol = window.location.protocol

  try {
    // assetPrefix is a url
    protocol = new URL(assetPrefix).protocol
  } catch (_) {}

  return protocol === 'http:' ? 'ws' : 'wss'
}

useWebsocket

export function useWebsocket(assetPrefix: string) {
  const webSocketRef = useRef<WebSocket>()

  useEffect(() => {
    if (webSocketRef.current) {
      return
    }

    const { hostname, port } = window.location
    const protocol = getSocketProtocol(assetPrefix)
    const normalizedAssetPrefix = assetPrefix.replace(/^\/+/, '')

    let url = `${protocol}://${hostname}:${port}${
      normalizedAssetPrefix ? `/${normalizedAssetPrefix}` : ''
    }`

    if (normalizedAssetPrefix.startsWith('http')) {
      url = `${protocol}://${normalizedAssetPrefix.split('://')[1]}`
    }

    webSocketRef.current = new window.WebSocket(`${url}/_next/webpack-hmr`)
  }, [assetPrefix])

  return webSocketRef
}

[krossekrabbe]

Yes, using host and port based on window.location like @medihack suggests would also solve the deployed-behind-proxy-issue I had described.

^{If you're wondering why a dev server is being deployed behind a proxy: We use kubernetes also for local development, so for development the dev server is being deployed, for testing & prod an nginx. I guess it's not a too unusual scenerio these days.}

[tmeasday]

@ndelangen what are your thoughts on this change (calculating server URLs in the browser, rather than setting them in the build)?

[ndelangen]

Why would the server channel be used in a deployed storybook?
It's only supposed to be used in dev-mode?

There should be no harm in making it use the same origin?

[tmeasday]

@ndelangen I don't think we are talking about "deployed" SBs here, but instead in-development SBs that are hosted online, for instance like GitHub codespaces type situations.

[ndelangen]

aha, sure. seems allright to me to change that to match the current origin 👍

[crashtech]

Are there no workarounds so far?

[KhodeN]

Are there no workarounds so far?

I have used the https://www.npmjs.com/package/patch-package for patched the code directly (write my own host).
It's seems stupid, but works)

[krossekrabbe]

Are there no workarounds so far?

I have used the https://www.npmjs.com/package/patch-package for patched the code directly (write my own host). It's seems stupid, but works)

Not a too bad idea, when using yarn, patch functionality is even built in. I forget about this constantly.

[kaelig]

@KhodeN @krossekrabbe A blog post (or even a gist) about how you implemented this workaround would be immensely useful to folks in the community who are trying to advocate internally for remote development environments such as GitHub Codespaces 🙏🏻

[medihack]

Can we have a real fix before the final release of v7? The fix looks so easy (just use window.location), and it affects anyone working in a cloud IDE.

[ndelangen]

I think the issue is here:

storybook/code/lib/core-server/src/utils/server-address.ts

Lines 19 to 21 in 441338a

	export const getServerChannelUrl = (port: number, { https }: { https?: boolean }) => {
	return `${https ? 'wss' : 'ws'}://localhost:${port}/storybook-server-channel`;
	};

We pass this value to the manager, the full URL.

[medihack]

I think the issue is here:

storybook/code/lib/core-server/src/utils/server-address.ts

Lines 19 to 21 in 441338a

	export const getServerChannelUrl = (port: number, { https }: { https?: boolean }) => {
	return `${https ? 'wss' : 'ws'}://localhost:${port}/storybook-server-channel`;
	};

We pass this value to the manager, the full URL.

Exactly, and this would fix it

export const getServerChannelUrl = () => {
  const { hostname, port, protocol } = window.location
  return `${protocol === 'https:' ? 'wss' : 'ws'}://${hostname}:${port}`
}

I think It doesn't need to be configurable as it is client-side only (but this could also be easily implemented).

[ndelangen]

That code is called in node, which has no window

[medihack]

That code is called in node, which has no window

You are right. Sorry for the quick shot :-( It is indeed used on the server by build-dev.ts and then given to the client as SERVER_CHANNEL_URL. Maybe it would be better to not use that variable on the client then, but the window.location as above and leave the server code alone (or make hostname configurable as well there).

[ndelangen]

The browser needs to know if it should connect to ws:// or wss://, and it has to know the port..
.. but tracing the code up, the port always matches the current port:

storybook/code/lib/core-server/src/build-dev.ts

Line 66 in d2d6f9d

options.serverChannelUrl = getServerChannelUrl(port, options);

So we could just drop that part.

And it's likely save to assume the protocol should be wss:// if location.protocol is https://.
So it would seem that passing SERVER_CHANNEL_URL might be really redundant?

I'll look @tmeasday into this discussion.

Tom, do you think we could drop passing the SERVER_CHANNEL_URL to the preview, and instead rely on code like this in the preview itself:

export const getServerChannelUrl = () => {
  const { hostname, port, protocol } = window.location
  return `${protocol === 'https:' ? 'wss' : 'ws'}://${hostname}:${port}/storybook-server-channel`
}

We could hard-code /storybook-server-channel, or continue to pass it from the server to the preview still, but I wonder if there's a point in doing so. We could hard-code it in a shared package, so we don't have to change a hard-coded strin in 2 places.

[tmeasday]

I think that all sounds pretty reasonable.

[ndelangen]

@medihack would you be open to creating a PR changing the above?
I could pair with you on it, if you'd like.

FYI, the PR would not be merged before the 7.0 release, we're in a feature freeze.

[medihack]

@ndelangen Ok, I can have a look and see with what I can come up in the upcoming weeks.

[shilman]

Jiminy cricket!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.1.0-alpha.8 containing PR #22055 that references this issue. Upgrade today to the @future NPM tag to try it out!

npx sb@next upgrade --tag future

[shilman]

Ermahgerd!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.0.14 containing PR #22055 that references this issue. Upgrade today to the @latest NPM tag to try it out!

npx sb@latest upgrade

[kevinvalk]

Not sure if it is worth to re-open but the PR #22055 does not take a different base URL into account. So if you would be running with:

  ...
  async viteFinal(config) {
    return mergeConfig(config, {
      base: '/storybook',
    });
  },
  ...

It will fail due https://github.com/storybookjs/storybook/pull/22055/files#diff-3a2950085f45f23c305d6ec255529302b1d70eb2a078a8111a6c9375159898b6R89 not taking this /storybook url into account