-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Setting the servername to an empty string '' should disable sending the SNI extension. #1587
Conversation
Can you add a test? |
Yes, of course. I already tried to add a test in the |
I think you can "copy" the test included in nodejs/node@98e9de7. |
Thanks for the hint. I have seen it too late and already added a test. If you think it is better to "copy" the kind of test from node I will do that. |
test/websocket.test.js
Outdated
}); | ||
|
||
it('allows to use empty string to disable sending the SNI extension', () => { | ||
const ws = new WebSocket('wss://127.0.0.1', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please mock tls.connect()
here and remove the describe
wrapper and the beforeEach()
and afterEach()
hook? We will add them if/when needed. Thank you.
I prefer your test. |
Thank you. |
@lpinca Couldn't answer any more, because I had two days off and since today I'm back in office again. Many thanks for your fast reply and your help. Wouldn't it be better to restore the original tls.connect = original;
assert.strictEqual(options.servername, ''); Or does the test runner stop all tests if the first test fails? |
The option
servername
intls.connect(options[, callback])
is the name of the host being connected to, and must be a host name, and not an IP address. If the server is not a multi-homed server and I want to connect with the IP address it is not possible to disable sending the SNI extension by setting the servername to empty string''
. This PR implements the defaulting of theservername
in the same way as it is done in the node.This PR is related to #1347 .
Example
In node v12 with the following code
I get the deprecation warning
[DEP0123]
(node:71346) [DEP0123] DeprecationWarning: Setting the TLS ServerName to an IP address is not permitted by RFC 6066. This will be ignored in a future version.