Repository navigation
Marking the server as secure when only using httpsServerOptions #1605
Description
Bug Report
When creating a server by only using httpsServerOptions, the server is not mark as secure. This cause the subsequent listen to report using http:// instead of https://.
This doesn't affect servers created while declaring httpsServerOptions directly in the options object, only if you put them all in httpsServerOptions. Also, as far as I know, it doesn't seem to affect functionality, because clients can still connect via HTTPS and the req.isSecure() method returns true. It's only for fetching the URL from the url property of the restify server that seems to be affected.
Restify Version
6.3.4
Node.js Version
9.5.0
Expected behaviour
Should be listening on https://127.0.0.1:8443.
Actual behaviour
Is listening on http://127.0.0.1:8443.
Repro case
const restify = require('restify');
const server = restify.createServer({
name: 'simple-server',
version: '1.0.0',
httpsServerOptions: {
}
});
server.listen(8443, 'localhost', () => {
// output => simple-server listening at http://127.0.0.1:8443
console.log('%s listening at %s', server.name, server.url);
});Cause
The cause is found in /lib/server.js, at line 169. When creating the HTTPS server, this.secure should be set to true.
Are you willing and able to fix this?
Yes, check this commit: https://github.com/wolfmah/node-restify/commit/9573125008a0c05bb8bf4152f2258dafd66631d9
Activity
kevinpeno commentedon Apr 13, 2018
I'd like to help out where I can on this. A few Qs:
Should this be applied to all maintained versions? Which versions are still maintained when considering [Critical] labels?
I noted during a code review that the path prior to the
options.httpsServerOptionschecks regular options for(options.cert || options.certificate) && options.keyand then manually creates the "secure" options to pass tohttps.createServer. I wonder if it would be better to change this entire scheme to follow the patterns similar tospdyandhttp2and only allowhttpsifoptions.httpsis set (for clarity).This would be a breaking change to deprecate both options above that currently exist and make the options match the newer formats (which version gets this breaking change?). For current and older versions, a shim to match
options.httpsServerOptionstooptions.httpsand replace the current(options.cert || options.certificate) && options.keywith a mapping tooptions.httpsas well. Perhaps a depreciation warning on start?DonutEspresso commentedon May 14, 2018
Hello @kevinpeno, sorry for the radio silence! Core team has been swamped on other things this past month. For something like this that is primarily ergonomics, we can aim to fix on the latest major version and not worry about the older versions. Thank you very much for the offer to help!
I think your approach sounds great! I'd keep the shim in place for 7.x and we can remove it for 8.x. Deprecation warning is also 💯 Any concerns @hekike ?
hekike commentedon May 14, 2018
Sounds good to me.
kevinpeno commentedon May 14, 2018
Alright. I'll get a PR together. Thanks for the feedback!