Marking the server as secure when only using httpsServerOptions

[wolfmah]

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

[kevinpeno]

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.httpsServerOptions checks regular options for (options.cert || options.certificate) && options.key and then manually creates the "secure" options to pass to https.createServer. I wonder if it would be better to change this entire scheme to follow the patterns similar to spdy and http2 and only allow https if options.https is 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.httpsServerOptions to options.https and replace the current (options.cert || options.certificate) && options.key with a mapping to options.https as well. Perhaps a depreciation warning on start?

[DonutEspresso]

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]

Sounds good to me.

[kevinpeno]

Alright. I'll get a PR together. Thanks for the feedback!