FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tls: TLSSocket options not initialized by jhamhader · Pull Request #2614 · nodejs/node · GitHub

/ node Public

tls: TLSSocket options not initialized - #2614

Closed
jhamhader wants to merge 1 commit into
nodejs:masterfrom
jhamhader:tls-socket-default-options
ConversationCommits (1)ChecksFiles changed
Closed

tls: TLSSocket options not initialized#2614
jhamhader wants to merge 1 commit into
nodejs:masterfrom
jhamhader:tls-socket-default-options

Conversation

jhamhader commented Aug 29, 2015

Copy link
Copy Markdown
Contributor

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

targos added the tls Issues and PRs related to the tls subsystem. label Aug 29, 2015
mscdex reviewed Aug 29, 2015
View reviewed changes
Comment thread doc/api/tls.markdown Outdated

mscdex Aug 29, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The true here should include backticks also for consistency.

jhamhader Sep 1, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done

thefourtheye commented Aug 31, 2015

Copy link
Copy Markdown
Contributor

If isServer is not set explicitly then it will be undefined. Does the code really check if the value is false anywhere?

Edit: I see that the code relies only on the truthiness/falsiness of isServer.

jhamhader force-pushed the tls-socket-default-options branch 2 times, most recently from 9fc32cd to f61c44a Compare September 19, 2015 12:17

indutny commented Sep 24, 2015

Copy link
Copy Markdown
Member

cc @nodejs/crypto

indutny reviewed Sep 24, 2015
View reviewed changes
Comment thread lib/_tls_wrap.js Outdated

indutny Sep 24, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

indutny Sep 24, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If you would like to make isServer false by default - let's do it explicitly!

jhamhader Sep 24, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

indutny Sep 24, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Oh... sorry then!

jhamhader force-pushed the tls-socket-default-options branch from f61c44a to 324bb28 Compare September 26, 2015 20:01

jhamhader commented Sep 26, 2015

Copy link
Copy Markdown
Contributor Author
  • Changed isServer to be false by default
  • Updated test to include two kinds of arguments of TLSSocket()

jhamhader commented Oct 8, 2015

Copy link
Copy Markdown
Contributor Author

@indutny can you confirm?

indutny commented Oct 13, 2015

Copy link
Copy Markdown
Member

Why is it needed? Would it be enough to do !!isServer when passing it to the C++ layer?

jhamhader commented Oct 18, 2015

Copy link
Copy Markdown
Contributor Author

Well, it is not only passed to the C++ layer - it is also used extensively in _tls_wrap.js.
Instead of the current change, we could do in TLSSocket():

this._tlsOptions.isServer = !!options.isServer

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

Copy link
Copy Markdown
Member

@jhamhader It feels like doing !! in one place is simpler.

jhamhader force-pushed the tls-socket-default-options branch from 324bb28 to 7f69c59 Compare October 20, 2015 19:06

jhamhader commented Oct 20, 2015

Copy link
Copy Markdown
Contributor Author

Done

indutny reviewed Oct 20, 2015
View reviewed changes
Comment thread doc/api/tls.markdown Outdated

indutny Oct 20, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is this over 80 column limit?

jhamhader Oct 20, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for the thorough review. Fixed that

tls: TLSSocket options default isServer false
62354f6 
Upon creating a TLSSocket object, set the default isServer option to false
Updated tls docs and added test-tls-socket-default-options
jhamhader force-pushed the tls-socket-default-options branch from 7f69c59 to 62354f6 Compare October 20, 2015 19:33

indutny commented Oct 20, 2015

Copy link
Copy Markdown
Member

LGTM

indutny commented Oct 20, 2015

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/543/

indutny commented Oct 20, 2015

Copy link
Copy Markdown
Member

Landed in adfd20b, thank you!

indutny closed this Oct 20, 2015
indutny pushed a commit that referenced this pull request Oct 20, 2015
tls: TLSSocket options default isServer false
adfd20b 
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 <fedor@indutny.com>
rvagg pushed a commit that referenced this pull request Oct 21, 2015
tls: TLSSocket options default isServer false
c64af7d 
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 <fedor@indutny.com>
rvagg mentioned this pull request Oct 21, 2015
Merged
jhamhader mentioned this pull request Oct 21, 2015
Closed

MylesBorins commented Oct 23, 2015

Copy link
Copy Markdown
Contributor

LTS?

MylesBorins commented Oct 23, 2015

Copy link
Copy Markdown
Contributor

/cc @jasnell

jasnell pushed a commit to jasnell/node that referenced this pull request Oct 26, 2015
tls: TLSSocket options default isServer false
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: nodejs#2614
Reviewed-By: Fedor Indutny <fedor@indutny.com>

jasnell commented Oct 26, 2015

Copy link
Copy Markdown
Member

Landed in v4.x-staging in 590378c

jasnell pushed a commit that referenced this pull request Oct 26, 2015
tls: TLSSocket options default isServer false
f6ea99f 
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 <fedor@indutny.com>
rvagg mentioned this pull request Oct 28, 2015
Closed
jasnell pushed a commit that referenced this pull request Oct 29, 2015
tls: TLSSocket options default isServer false
e69c869 
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 <fedor@indutny.com>
jasnell mentioned this pull request Oct 29, 2015
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

tls Issues and PRs related to the tls subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL