Added HTTPS option for dev server #735
Conversation
There was a problem hiding this comment.
Thanks @doctyper! The code looks great.
I am wondering what the use case for this feature is? Normally I would recommend using build-storybook and then serving the artifacts with static file serving. If there is a use case for using the development server with https, then I'd be up for merging this since it doesn't add too much complexity to the code.
src/server/index.js
Outdated
| } | ||
|
|
||
| const sslOptions = { | ||
| ca: fs.readFileSync(program.sslCa, 'utf-8'), |
There was a problem hiding this comment.
@doctyper looking at the docs, it seems like ca should be an array, and also is only used for self-signed certificates?
https://nodejs.org/api/https.html
https://nodejs.org/api/tls.html#tls_tls_createserver_options_secureconnectionlistener
If it's straightforward can you provide simple instructions for me to test this?
|
@arbesfeld Thanks. The use case is pretty straightforward. We have an HTTPS-everywhere policy @nfl. Some of our components change state depending on user login state, but this requires access to cookies. Since our cookies are secure, they can only be read from a domain with a valid SSL cert. Hence the need. I think I'll eventually stub that out so that I don't need actual cookies to display my component's state in Storybook, but this was a pain point during my prototype phase where I just wanted something quick and dirty to build my component. I'm sure similar use-cases exist for other SSL-sensitive APIs like Service Workers, LocalStorage, SessionStorage, etc. |
|
@shilman Oh interesting. It works fine as a string. I'll update the code to handle either case, and make |
Per the Node docs, the `ca` value should be an Array: https://nodejs.org/api/tls.html#tls_tls_createserver_options_secureconnectionlistener
… into feat/storybook-https
|
@shilman Done. Also, it's out of scope for this PR, but I was thinking about adding support for a |
src/server/index.js
Outdated
| } | ||
|
|
||
| const sslOptions = { | ||
| ca: program.sslCa.map(ca => fs.readFileSync(ca, 'utf-8')), |
There was a problem hiding this comment.
@doctyper I think you need to make this line conditional:
/Users/shilman/projects/react-storybook/dist/server/index.js:108
ca: _commander2.default.sslCa.map(function (ca) {
^
TypeError: Cannot read property 'map' of undefined
...
There was a problem hiding this comment.
Repro:
MBP:react-storybook shilman$ ./dist/server/index.js --https --ssl-key ~/tmp/a.txt --ssl-cert ~/tmp/b.txt --port 6003
|
@doctyper Interesting idea about the config file. I agree that the growing list of command-line args is unwieldy, but I think the I agree that an alternative solution is outside the scope of this PR. |
|
@shilman I can add the new args to the list of env vars if you'd like. Not a problem. |
|
@doctyper I'm fine either way regarding the env vars, just wanted to point out the inconsistency if it wasn't intentional. Thanks for the quick fixes and the great feature!! |
|
@arbesfeld In addition to @doctyper's use case mentioned above, I think once we open up the server-side plugins as discussed in the video chat yesterday, there will be a lot of cases where one will want to run the server under HTTPS rather than serving from static files. (Whether this is best done by extending storybook-server or just pointing your client-side plugins at some completely different server is debatable.) |
|
I was thinking, maybe at some point in the future we could create middleware/routers for stuff like express. That way people would be in full control of the web-app and storybook would just be a route on that app. |
This pull request introduces four CLI flags allowing users to serve Storybook over HTTPS:
--https Serve Storybook over HTTPS. Note: You must provide your own certificate information. --ssl-ca <ca> Provide an SSL certificate authority. (Required with --https) --ssl-cert <cert> Provide an SSL certificate. (Required with --https) --ssl-key <key> Provide an SSL key. (Required with --https)