Skip to content

Load SSL cert if environment variables exist #17

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.

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
merged 1 commit into from
Jul 19, 2017
Merged

Conversation

craineum
Copy link

@craineum craineum commented Jul 19, 2017

The following environment variables are inspected:

If HTTPS is set to true and SSL_KEY_PATH && SSL_CERT_PATH are set, we try to load the certs via webpack devServer.

This was needed to get past browser non-trusted self signed certs to have a nice happy green browser experience. See more here: facebook#2759 (comment)

@craineum craineum requested review from randycoulman and lexun July 19, 2017 18:56
Copy link

@randycoulman randycoulman left a comment

Choose a reason for hiding this comment

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

A question and a (non-blocking) suggestion. 👍 for offering to do a PR upstream (I found your comment when looking into why this PR was necessary). Might be worth linking to that issue from here for future reference.

Overall, LGTM!

} else {
return false;
}
})(process.env.HTTPS, process.env.SSL_KEY_PATH, process.env.SSL_CERT_PATH);

Choose a reason for hiding this comment

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

Curious why you're using an IIFE here. I like the way the code looks, but just curious about the decision to go this way.

Copy link
Author

Choose a reason for hiding this comment

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

I didn't want to call the function down below. I really wanted these methods to be in react-dev-utils and then pass in the results to this method from start.js as variable name https. Maybe that is too preemptive here.

Choose a reason for hiding this comment

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

The problem is that start.js is not the only place that this config is imported, and the other importer doesn't have the necessary context, so what's here is good.

const host = process.env.HOST || '0.0.0.0';
// ZEAL: ssl file loading
const sslPath = function(path) {

Choose a reason for hiding this comment

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

The name sslPath doesn't communicate to me what this is doing. Maybe readSslFile or something?

@craineum craineum merged commit f133770 into zeal Jul 19, 2017
@craineum craineum deleted the ssl_cert_support branch July 19, 2017 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants