tls: TLSSocket options not initialized#2614
Uh oh!
There was an error while loading. Please reload this page.
Conversation
doc/api/tls.markdown Outdated
There was a problem hiding this comment.
The true here should include backticks also for consistency.
thefourtheye commented Aug 31, 2015
If Edit: I see that the code relies only on the truthiness/falsiness of |
jhamhaderforce-pushed the tls-socket-default-options branch 2 times, most recently from 9fc32cd to f61c44aCompare
indutny commented Sep 24, 2015
cc @nodejs/crypto |
lib/_tls_wrap.js Outdated
There was a problem hiding this comment.
I don't think that this can possibly suffice. There are also context and various other things that needs to be specified. It can't be possible to omit them, so I don't think that there is any point in providing this default values.
There was a problem hiding this comment.
If you would like to make isServerfalse by default - let's do it explicitly!
There was a problem hiding this comment.
I'll make isServer: false by default (set to false if options is undefined or options.isServer is undefined).
Regarding the context - TLS API says the rest of the options (besides isServer) are optional and seems like _wrapHandle() creates a secureContext if such is not provided.
jhamhaderforce-pushed the tls-socket-default-options branch from f61c44a to 324bb28Compare
jhamhader commented Sep 26, 2015
|
jhamhader commented Oct 8, 2015
@indutny can you confirm? |
indutny commented Oct 13, 2015
Why is it needed? Would it be enough to do |
jhamhader commented Oct 18, 2015
Well, it is not only passed to the C++ layer - it is also used extensively in _tls_wrap.js. but initializing this value with a default boolean (in either way) seems like a harmless and safe thing to do. |
indutny commented Oct 18, 2015
@jhamhader It feels like doing |
jhamhaderforce-pushed the tls-socket-default-options branch from 324bb28 to 7f69c59Comparejhamhader commented Oct 20, 2015
Done |
doc/api/tls.markdown Outdated
There was a problem hiding this comment.
Thanks for the thorough review. Fixed that
Upon creating a TLSSocket object, set the default isServer option to false Updated tls docs and added test-tls-socket-default-options
jhamhaderforce-pushed the tls-socket-default-options branch from 7f69c59 to 62354f6Compareindutny commented Oct 20, 2015
LGTM |
indutny commented Oct 20, 2015
indutny commented Oct 20, 2015
Landed in adfd20b, thank you! |
Upon creating a TLSSocket object, set the default isServer option to false Updated tls docs and added test-tls-socket-default-options PR-URL: #2614 Reviewed-By: Fedor Indutny <[email protected]>
Upon creating a TLSSocket object, set the default isServer option to false Updated tls docs and added test-tls-socket-default-options PR-URL: #2614 Reviewed-By: Fedor Indutny <[email protected]>
MylesBorins commented Oct 23, 2015
LTS? |
MylesBorins commented Oct 23, 2015
/cc @jasnell |
Upon creating a TLSSocket object, set the default isServer option to false Updated tls docs and added test-tls-socket-default-options PR-URL: nodejs#2614 Reviewed-By: Fedor Indutny <[email protected]>
jasnell commented Oct 26, 2015
Landed in v4.x-staging in 590378c |
Upon creating a TLSSocket object, set the default isServer option to false Updated tls docs and added test-tls-socket-default-options PR-URL: #2614 Reviewed-By: Fedor Indutny <[email protected]>
Upon creating a TLSSocket object, set the default isServer option to false Updated tls docs and added test-tls-socket-default-options PR-URL: #2614 Reviewed-By: Fedor Indutny <[email protected]>
7 participants
Upon creating a TLSSocket object without options, default options will be used, which
set the socket as isServer: false
Updated tls docs and added test-tls-socket-default-options
See issue #2394