Repository: websockets/ws
Pull request #1587 (2019-06-14 13:33:10)

Setting the servername to an empty string '' should disable sending the SNI extension.
The option `servername` in [`tls.connect(options[, callback])`](https://nodejs.org/api/tls.html#tls_tls_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 the `servername` in the same way as it is done in the [node](https://github.com/nodejs/node/blob/master/lib/_http_agent.js#L161-L162). 

This PR is related to #1347 .

**Example**
In node v12 with the following code
```js
new WebSocket('wss://127.0.0.1', {
  ca: fs.readFileSync('ssl/ca.pem'),
  servername: '',
})
```
I get the deprecation warning [`[DEP0123]`](https://nodejs.org/api/deprecations.html#deprecations_dep0123_setting_the_tls_servername_to_an_ip_address)
```bash
(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.
```


lpinca: Can we add a test?
holgerkoser: Yes, of course. I already tried to add a test in the [`SSL`](https://github.com/websockets/ws/blob/master/test/websocket.test.js#L1922) but I not found an easy way to intercept the `tls.connect` call. I normally use [`proxyquire`](https://github.com/thlorenz/proxyquire) for this but don't wanted to add a new dependency for such a small change. I try to find an alternative simple solution for a test.
lpinca: I think you can "copy" the test included in https://github.com/nodejs/node/commit/98e9de7db930e505884ca8d0ca6588afbe43f127.
holgerkoser: 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.
lpinca: I prefer your test.

Patch:
diff --git a/lib/websocket.js b/lib/websocket.js
index 070b3a0e..e17d6c1a 100644
--- a/lib/websocket.js
+++ b/lib/websocket.js
@@ -668,7 +668,11 @@ function netConnect(options) {
  */
 function tlsConnect(options) {
   options.path = undefined;
-  options.servername = options.servername || options.host;
+
+  if (!options.servername && options.servername !== '') {
+    options.servername = options.host;
+  }
+
   return tls.connect(options);
 }
 
diff --git a/package.json b/package.json
index acd97588..b7128330 100644
--- a/package.json
+++ b/package.json
@@ -35,7 +35,7 @@
     "bufferutil": "^4.0.1",
     "coveralls": "^3.0.3",
     "eslint": "^5.16.0",
-    "eslint-config-prettier": "^4.1.0",
+    "eslint-config-prettier": "^5.0.0",
     "eslint-plugin-prettier": "^3.0.1",
     "mocha": "^6.1.3",
     "nyc": "^14.0.0",


Test patch:
diff --git a/test/websocket.test.js b/test/websocket.test.js
index 90ff9612..64755021 100644
--- a/test/websocket.test.js
+++ b/test/websocket.test.js
@@ -6,6 +6,7 @@ const assert = require('assert');
 const crypto = require('crypto');
 const https = require('https');
 const http = require('http');
+const tls = require('tls');
 const fs = require('fs');
 const { URL } = require('url');
 
@@ -2040,6 +2041,18 @@ describe('WebSocket', () => {
         });
       });
     }).timeout(4000);
+
+    it('allows to disable sending the SNI extension', (done) => {
+      const original = tls.connect;
+
+      tls.connect = (options) => {
+        assert.strictEqual(options.servername, '');
+        tls.connect = original;
+        done();
+      };
+
+      const ws = new WebSocket('wss://127.0.0.1', { servername: '' });
+    });
   });
 
   describe('Request headers', () => {