Repository: websockets/ws
Pull request #932 (2016-12-11 15:10:21)

Remove unnecessary `buildHostHeader` function
The `host` header is already correctly built when the `http.ClientRequest` is created.
If the port number is the default one it is not added to the `host` header field value.


Patch:
diff --git a/lib/WebSocket.js b/lib/WebSocket.js
index 794e7469..fc45b08e 100644
--- a/lib/WebSocket.js
+++ b/lib/WebSocket.js
@@ -40,9 +40,6 @@ class WebSocket extends EventEmitter {
     super();
 
     if (typeof protocols === 'object' && !Array.isArray(protocols)) {
-      //
-      // Accept the `options` object as the 2nd argument.
-      //
       options = protocols;
       protocols = null;
     }
@@ -50,13 +47,17 @@ class WebSocket extends EventEmitter {
     if (typeof protocols === 'string') protocols = [protocols];
     if (!Array.isArray(protocols)) protocols = [];
 
-    this._finalize = this.finalize.bind(this);
+    this.readyState = WebSocket.CONNECTING;
+    this.bytesReceived = 0;
+    this.extensions = {};
+    this.protocol = '';
 
+    this._finalize = this.finalize.bind(this);
     this._binaryType = 'nodebuffer';
     this._closeReceived = false;
-    this.bytesReceived = 0;
-    this.readyState = null;
-    this.extensions = {};
+    this._closeTimer = null;
+    this._receiver = null;
+    this._sender = null;
     this._socket = null;
     this._ultron = null;
 
@@ -443,27 +444,6 @@ WebSocket.prototype.addEventListener = EventTarget.addEventListener;
 WebSocket.prototype.removeEventListener = EventTarget.removeEventListener;
 
 module.exports = WebSocket;
-module.exports.buildHostHeader = buildHostHeader;
-
-/**
- * Append port number to Host header, only if specified in the URL and
- * non-default.
- *
- * @param {Boolean} isSecure Specifies whether or not the URL scheme is `wss`
- * @param {String} hostname The hostname portion of the URL
- * @param {Number} port The port portion of the URL
- * @return {String} The field value of the `Host` header
- * @private
- */
-function buildHostHeader (isSecure, hostname, port) {
-  var headerHost = hostname;
-
-  if (headerHost && (isSecure && port !== 443 || !isSecure && port !== 80)) {
-    headerHost = `${headerHost}:${port}`;
-  }
-
-  return headerHost;
-}
 
 /**
  * Initialize a WebSocket server client.
@@ -484,7 +464,6 @@ function initAsServerClient (req, socket, head, options) {
   this.maxPayload = options.maxPayload;
   this.protocol = options.protocol;
 
-  this.readyState = WebSocket.CONNECTING;
   this.upgradeReq = req;
   this._isServer = true;
 
@@ -502,7 +481,7 @@ function initAsServerClient (req, socket, head, options) {
  * @param {String} options.localAddress Local interface to bind for network connections
  * @param {Number} options.protocolVersion Value of the `Sec-WebSocket-Version` header
  * @param {Object} options.headers An object containing request headers
- * @param {String} options.origin Value of the `Sec-WebSocket-Version` header
+ * @param {String} options.origin Value of the `Origin` or `Sec-WebSocket-Origin` header
  * @param {http.Agent} options.agent Use the specified Agent
  * @param {String} options.host Value of the `Host` header
  * @param {Function} options.checkServerIdentity A function to validate the server hostname
@@ -543,6 +522,10 @@ function initAsClient (address, protocols, options) {
     throw new Error('unsupported protocol version');
   }
 
+  this.protocolVersion = options.protocolVersion;
+  this._isServer = false;
+  this.url = address;
+
   const serverUrl = url.parse(address);
   const isUnixSocket = serverUrl.protocol === 'ws+unix:';
 
@@ -550,7 +533,6 @@ function initAsClient (address, protocols, options) {
 
   const isSecure = serverUrl.protocol === 'wss:' || serverUrl.protocol === 'https:';
   const key = crypto.randomBytes(16).toString('base64');
-  const port = serverUrl.port || (isSecure ? 443 : 80);
   const httpObj = isSecure ? https : http;
 
   //
@@ -567,22 +549,11 @@ function initAsClient (address, protocols, options) {
     extensionsOffer[PerMessageDeflate.extensionName] = perMessageDeflate.offer();
   }
 
-  //
-  // Expose state properties.
-  //
-  this.protocolVersion = options.protocolVersion;
-  this.readyState = WebSocket.CONNECTING;
-  this._isServer = false;
-  this.url = address;
-
-  var agent = options.agent;
-
   const requestOptions = {
     host: serverUrl.hostname,
+    port: serverUrl.port,
     path: '/',
-    port,
     headers: {
-      'Host': buildHostHeader(isSecure, serverUrl.hostname, port),
       'Sec-WebSocket-Version': options.protocolVersion,
       'Sec-WebSocket-Key': key,
       'Connection': 'Upgrade',
@@ -621,6 +592,8 @@ function initAsClient (address, protocols, options) {
     }
   }
 
+  var agent = options.agent;
+
   //
   // A custom agent is required for these options.
   //


Test patch:
diff --git a/test/WebSocket.test.js b/test/WebSocket.test.js
index cd484655..ffea6767 100644
--- a/test/WebSocket.test.js
+++ b/test/WebSocket.test.js
@@ -1453,17 +1453,26 @@ describe('WebSocket', function () {
     });
 
     it('excludes default ports from host header', function () {
-      // can't create a server listening on ports 80 or 443
-      // so we need to expose the method that does this
-      const buildHostHeader = WebSocket.buildHostHeader;
-      let host = buildHostHeader(false, 'localhost', 80);
-      assert.strictEqual(host, 'localhost');
-      host = buildHostHeader(false, 'localhost', 88);
-      assert.strictEqual(host, 'localhost:88');
-      host = buildHostHeader(true, 'localhost', 443);
-      assert.strictEqual(host, 'localhost');
-      host = buildHostHeader(true, 'localhost', 8443);
-      assert.strictEqual(host, 'localhost:8443');
+      const httpsAgent = new https.Agent();
+      const httpAgent = new http.Agent();
+      const values = [];
+      let ws;
+
+      httpsAgent.addRequest = httpAgent.addRequest = (req) => {
+        values.push(req._headers.host);
+      };
+
+      ws = new WebSocket('wss://localhost:8443', { agent: httpsAgent });
+      ws = new WebSocket('wss://localhost:443', { agent: httpsAgent });
+      ws = new WebSocket('ws://localhost:88', { agent: httpAgent });
+      ws = new WebSocket('ws://localhost:80', { agent: httpAgent });
+
+      assert.deepStrictEqual(values, [
+        'localhost:8443',
+        'localhost',
+        'localhost:88',
+        'localhost'
+      ]);
     });
   });