From 772a814fbe999b41dae0bf8ae6b09a5c079c4a40 Mon Sep 17 00:00:00 2001 From: Luigi Pinca Date: Tue, 10 Jan 2017 14:43:50 +0100 Subject: [PATCH] [fix] Abort the request if `close` is called while connecting (#956) Fixes #388 --- lib/WebSocket.js | 66 ++++++++----------- test/WebSocket.test.js | 122 ++++++++++++++++++++++------------- test/WebSocketServer.test.js | 38 ++++++----- 3 files changed, 122 insertions(+), 104 deletions(-) diff --git a/lib/WebSocket.js b/lib/WebSocket.js index 1abbb7407..0da5c0f78 100644 --- a/lib/WebSocket.js +++ b/lib/WebSocket.js @@ -262,10 +262,10 @@ class WebSocket extends EventEmitter { */ close (code, data) { if (this.readyState === WebSocket.CLOSED) return; - if (this.readyState === WebSocket.CONNECTING) { - this.readyState = WebSocket.CLOSED; - return; + this._req.abort(); + this.emit('error', new Error('closed before the connection is established')); + return this.finalize(true); } if (this.readyState === WebSocket.CLOSING) { @@ -374,6 +374,11 @@ class WebSocket extends EventEmitter { */ terminate () { if (this.readyState === WebSocket.CLOSED) return; + if (this.readyState === WebSocket.CONNECTING) { + this._req.abort(); + this.emit('error', new Error('closed before the connection is established')); + return this.finalize(true); + } if (this._socket) { this.readyState = WebSocket.CLOSING; @@ -385,8 +390,6 @@ class WebSocket extends EventEmitter { // clearTimeout(this._closeTimer); this._closeTimer = setTimeout(this._finalize, closeTimeout, true); - } else if (this.readyState === WebSocket.CONNECTING) { - this.finalize(true); } } } @@ -618,43 +621,32 @@ function initAsClient (address, protocols, options) { if (agent) requestOptions.agent = agent; - const req = httpObj.get(requestOptions); + this._req = httpObj.get(requestOptions); + + this._req.on('error', (error) => { + if (this._req.aborted) return; - req.on('error', (error) => { this.emit('error', error); - this.finalize(error); + this.finalize(true); }); - req.on('response', (res) => { - var error; - - if (!this.emit('unexpected-response', req, res)) { - error = new Error(`unexpected server response (${res.statusCode})`); - req.abort(); - this.emit('error', error); + this._req.on('response', (res) => { + if (!this.emit('unexpected-response', this._req, res)) { + this._req.abort(); + this.emit('error', new Error(`unexpected server response (${res.statusCode})`)); + this.finalize(true); } - - this.finalize(error); }); - req.on('upgrade', (res, socket, head) => { - if (this.readyState === WebSocket.CLOSED) { - // client closed before server accepted connection - this.emit('close'); - this.removeAllListeners(); - socket.end(); - return; - } - + this._req.on('upgrade', (res, socket, head) => { const digest = crypto.createHash('sha1') .update(key + GUID, 'binary') .digest('base64'); if (res.headers['sec-websocket-accept'] !== digest) { + socket.destroy(); this.emit('error', new Error('invalid server key')); - this.removeAllListeners(); - socket.end(); - return; + return this.finalize(true); } const serverProt = res.headers['sec-websocket-protocol']; @@ -670,30 +662,28 @@ function initAsClient (address, protocols, options) { } if (protError) { + socket.destroy(); this.emit('error', new Error(protError)); - this.removeAllListeners(); - socket.end(); - return; + return this.finalize(true); } if (serverProt) this.protocol = serverProt; const serverExtensions = Extensions.parse(res.headers['sec-websocket-extensions']); + if (perMessageDeflate && serverExtensions[PerMessageDeflate.extensionName]) { try { perMessageDeflate.accept(serverExtensions[PerMessageDeflate.extensionName]); } catch (err) { + socket.destroy(); this.emit('error', new Error('invalid extension parameter')); - this.removeAllListeners(); - socket.end(); - return; + return this.finalize(true); } + this.extensions[PerMessageDeflate.extensionName] = perMessageDeflate; } + this._req = null; this.setSocket(socket, head); - - req.removeAllListeners(); - agent = null; }); } diff --git a/test/WebSocket.test.js b/test/WebSocket.test.js index 242fb294a..debaf5d0b 100644 --- a/test/WebSocket.test.js +++ b/test/WebSocket.test.js @@ -95,39 +95,28 @@ describe('WebSocket', function () { wss.on('connection', (ws) => ws.send('foobar')); }); - it('#url exposes the server url', function (done) { - server.createServer(++port, (srv) => { - const url = `ws://localhost:${port}`; - const ws = new WebSocket(url); - - assert.strictEqual(ws.url, url); + it('#url exposes the server url', function () { + const url = `ws://localhost:${port}`; + const ws = new WebSocket(url, { agent: new CustomAgent() }); - ws.on('close', () => srv.close(done)); - ws.close(); - }); + assert.strictEqual(ws.url, url); }); - it('#protocolVersion exposes the protocol version', function (done) { - server.createServer(++port, (srv) => { - const ws = new WebSocket(`ws://localhost:${port}`); - - assert.strictEqual(ws.protocolVersion, 13); - - ws.on('close', () => srv.close(done)); - ws.close(); + it('#protocolVersion exposes the protocol version', function () { + const ws = new WebSocket(`ws://localhost:${port}`, { + agent: new CustomAgent() }); + + assert.strictEqual(ws.protocolVersion, 13); }); describe('#bufferedAmount', function () { - it('defaults to zero', function (done) { - server.createServer(++port, (srv) => { - const ws = new WebSocket(`ws://localhost:${port}`); - - assert.strictEqual(ws.bufferedAmount, 0); - - ws.on('close', () => srv.close(done)); - ws.close(); + it('defaults to zero', function () { + const ws = new WebSocket(`ws://localhost:${port}`, { + agent: new CustomAgent() }); + + assert.strictEqual(ws.bufferedAmount, 0); }); it('defaults to zero upon "open"', function (done) { @@ -203,15 +192,12 @@ describe('WebSocket', function () { }); describe('#readyState', function () { - it('defaults to connecting', function (done) { - server.createServer(++port, (srv) => { - const ws = new WebSocket(`ws://localhost:${port}`); - - assert.strictEqual(ws.readyState, WebSocket.CONNECTING); - - ws.on('close', () => srv.close(done)); - ws.close(); + it('defaults to connecting', function () { + const ws = new WebSocket(`ws://localhost:${port}`, { + agent: new CustomAgent() }); + + assert.strictEqual(ws.readyState, WebSocket.CONNECTING); }); it('set to open once connection is established', function (done) { @@ -236,7 +222,7 @@ describe('WebSocket', function () { srv.close(done); }); - ws.close(1001); + ws.on('open', () => ws.close(1001)); }); }); @@ -249,7 +235,7 @@ describe('WebSocket', function () { srv.close(done); }); - ws.terminate(); + ws.on('open', () => ws.terminate()); }); }); }); @@ -308,26 +294,68 @@ describe('WebSocket', function () { }); describe('connection establishing', function () { - it('can disconnect before connection is established', function (done) { - server.createServer(++port, (srv) => { + it('can terminate before connection is established (1/2)', function (done) { + const wss = new WebSocketServer({ port: ++port }, () => { const ws = new WebSocket(`ws://localhost:${port}`); ws.on('open', () => assert.fail(null, null, 'connect shouldnt be raised here')); - ws.on('close', () => srv.close(done)); + ws.on('error', (err) => { + assert.ok(err instanceof Error); + assert.strictEqual(err.message, 'closed before the connection is established'); + ws.on('close', () => wss.close(done)); + }); ws.terminate(); }); }); - it('can close before connection is established', function (done) { - server.createServer(++port, (srv) => { + it('can terminate before connection is established (2/2)', function (done) { + const wss = new WebSocketServer({ + verifyClient: (info, cb) => setTimeout(cb, 300, true), + port: ++port + }, () => { const ws = new WebSocket(`ws://localhost:${port}`); ws.on('open', () => assert.fail(null, null, 'connect shouldnt be raised here')); - ws.on('close', () => srv.close(done)); + ws.on('error', (err) => { + assert.ok(err instanceof Error); + assert.strictEqual(err.message, 'closed before the connection is established'); + ws.on('close', () => wss.close(done)); + }); + setTimeout(() => ws.terminate(), 150); + }); + }); + + it('can close before connection is established (1/2)', function (done) { + const wss = new WebSocketServer({ port: ++port }, () => { + const ws = new WebSocket(`ws://localhost:${port}`); + + ws.on('open', () => assert.fail(null, null, 'connect shouldnt be raised here')); + ws.on('error', (err) => { + assert.ok(err instanceof Error); + assert.strictEqual(err.message, 'closed before the connection is established'); + ws.on('close', () => wss.close(done)); + }); ws.close(1001); }); }); + it('can close before connection is established (2/2)', function (done) { + const wss = new WebSocketServer({ + verifyClient: (info, cb) => setTimeout(cb, 300, true), + port: ++port + }, () => { + const ws = new WebSocket(`ws://localhost:${port}`); + + ws.on('open', () => assert.fail(null, null, 'connect shouldnt be raised here')); + ws.on('error', (err) => { + assert.ok(err instanceof Error); + assert.strictEqual(err.message, 'closed before the connection is established'); + ws.on('close', () => wss.close(done)); + }); + setTimeout(() => ws.close(1001), 150); + }); + }); + it('can handle error before request is upgraded', function (done) { // Here, we don't create a server, to guarantee that the connection will // fail before the request is upgraded @@ -803,15 +831,17 @@ describe('WebSocket', function () { }); it('before connect should pass error through callback, if present', function (done) { - server.createServer(++port, (srv) => { + const wss = new WebSocketServer({ port: ++port }, () => { const ws = new WebSocket(`ws://localhost:${port}`); - ws.send('hi', (error) => { - assert.ok(error instanceof Error); - srv.close(done); - ws.terminate(); + ws.send('hi', (err) => { + assert.ok(err instanceof Error); + assert.strictEqual(err.message, 'not opened'); + ws.on('close', () => wss.close(done)); }); }); + + wss.on('connection', (ws) => ws.close()); }); it('without data should be successful', function (done) { diff --git a/test/WebSocketServer.test.js b/test/WebSocketServer.test.js index cceb53f94..3579dcf8f 100644 --- a/test/WebSocketServer.test.js +++ b/test/WebSocketServer.test.js @@ -202,10 +202,9 @@ describe('WebSocketServer', function () { const ws = new WebSocket(`ws://localhost:${port}`); }); - wss.on('connection', (client) => { + wss.on('connection', (ws) => { assert.strictEqual(wss.clients.size, 1); - wss.close(); - done(); + wss.close(done); }); }); @@ -213,12 +212,13 @@ describe('WebSocketServer', function () { const wss = new WebSocketServer({ port: ++port, clientTracking: false }, () => { assert.strictEqual(wss.clients, undefined); const ws = new WebSocket(`ws://localhost:${port}`); + + ws.on('open', () => ws.close()); }); - wss.on('connection', (client) => { + wss.on('connection', (ws) => { assert.strictEqual(wss.clients, undefined); - wss.close(); - done(); + ws.on('close', () => wss.close(done)); }); }); @@ -226,14 +226,13 @@ describe('WebSocketServer', function () { const wss = new WebSocketServer({ port: ++port }, () => { const ws = new WebSocket(`ws://localhost:${port}`); - wss.on('connection', (client) => { - client.on('close', () => { - assert.strictEqual(wss.clients.size, 0); - wss.close(); - done(); - }); + ws.on('open', () => ws.terminate()); + }); - ws.close(); + wss.on('connection', (ws) => { + ws.on('close', () => { + assert.strictEqual(wss.clients.size, 0); + wss.close(done); }); }); }); @@ -242,14 +241,13 @@ describe('WebSocketServer', function () { const wss = new WebSocketServer({ port: ++port }, () => { const ws = new WebSocket(`ws://localhost:${port}`); - wss.on('connection', (client) => { - client.on('close', () => { - assert.strictEqual(wss.clients.size, 0); - wss.close(); - done(); - }); + ws.on('open', () => ws.close()); + }); - ws.close(); + wss.on('connection', (ws) => { + ws.on('close', () => { + assert.strictEqual(wss.clients.size, 0); + wss.close(done); }); }); });