-
Notifications
You must be signed in to change notification settings - Fork 703
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
Check if SSL key and certificate files exist #673
Conversation
key: fs.readFileSync(keyPath), | ||
cert: fs.readFileSync(certPath) | ||
}, app).listen(config.port, config.host); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer two separate messages for each of the files. And do a negative check, then you dont need an else
.
Just wondering, does it explode if the file exists but it's not valid? If so might want to add an extra check for that. |
Good idea. I'll look into this. |
@xPaw, in #416, @toXel asked if we preferred an |
@xPaw yes it crashes if the files are not valid:
I could add a
to catch it and stop the server with a better message. But then if the server couldn't be created because of some other error, it will give the user a wrong error message... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine by me, let it crash on invalid certs, unless node has a function to validate them (which I doubt)
@xPaw Nope I haven't found a function for it. There's a pem npm module with a |
@@ -36,9 +36,19 @@ module.exports = function() { | |||
server = server.createServer(app).listen(config.port, config.host); | |||
} else { | |||
server = require("spdy"); | |||
let keyPath = Helper.expandHome(config.https.key); | |||
let certPath = Helper.expandHome(config.https.certificate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change these from let
to const
please?
After addressing my comment, can you squash your PR as well please? :) |
0fee528
to
5b6f5d5
Compare
Done :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job @toXel, thanks!
Check if SSL key and certificate files exist
It's a first attempt. Please suggest changes.
This fixes #416.