tls: warn on NODE_TLS_REJECT_UNAUTHORIZED = '0' #21900
Conversation
lib/_tls_wrap.js
Outdated
| // Arguments: [port,] [host,] [options,] [cb] | ||
| exports.connect = function connect(...args) { | ||
| args = normalizeConnectArgs(args); | ||
| var options = args[0]; | ||
| var cb = args[1]; | ||
| const allowUnauthorized = '0' === process.env.NODE_TLS_REJECT_UNAUTHORIZED; |
There was a problem hiding this comment.
Can we reverse this equality check?: process.env.NODE_TLS_REJECT_UNAUTHORIZED === '0'
lib/_tls_wrap.js
Outdated
| warnOnAllowUnauthorized = false; | ||
| process.emitWarning('Setting the NODE_TLS_REJECT_UNAUTHORIZED ' + | ||
| 'environment variable to \'0\' is considered ' + | ||
| 'insecure.'); |
There was a problem hiding this comment.
«is considered insecure» is too broad, imo.
Perhaps something among the lines «makes tls connections and https requests insecure»?
There was a problem hiding this comment.
Possibly even add "by disabling certificate verification" or so, because my experience is that people using this option almost never actually know what it does... and have just enabled it because a code example suggested it / a StackOverflow answer told them to / their boss told them to / etc.
There was a problem hiding this comment.
Yes, makes tls connections and https requests insecure by disabling certificate verification looks much better to me :-).
Warn on the first request that sets the NODE_TLS_REJECT_UNAUTHORIZED environment variable to '0'. PR-URL: nodejs#21900 Refs: nodejs#21774 Reviewed-By: James M Snell <jasnell@gmail.com>
|
This has no label. Should it be |
|
Yeah, this should have been semver-major I believe. |
|
Yeah, as a new warning, I think semver-major might be appropriate. |
targos
added
the
semver-major
|
OK I added the label |
Warn on the first request that sets the
NODE_TLS_REJECT_UNAUTHORIZEDenvironment variable to'0'.Refs: #21774
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes