From beff62081d5591c4f1beb9a3993b24ea7473d792 Mon Sep 17 00:00:00 2001 From: Luigi Pinca Date: Sun, 10 Dec 2017 14:50:10 +0100 Subject: [PATCH] [fix] Use status code from close frame if received --- lib/EventTarget.js | 2 +- lib/WebSocket.js | 102 +++++++++----------- test/WebSocket.test.js | 174 +++++------------------------------ test/WebSocketServer.test.js | 46 +++++---- 4 files changed, 97 insertions(+), 227 deletions(-) diff --git a/lib/EventTarget.js b/lib/EventTarget.js index 9d0461a18..c48137b91 100644 --- a/lib/EventTarget.js +++ b/lib/EventTarget.js @@ -55,7 +55,7 @@ class CloseEvent extends Event { constructor (code, reason, target) { super('close', target); - this.wasClean = code === undefined || code === 1000 || (code >= 3000 && code <= 4999); + this.wasClean = target._closeFrameReceived && target._closeFrameSent; this.reason = reason; this.code = code; } diff --git a/lib/WebSocket.js b/lib/WebSocket.js index 72a62e56f..a8c7b99b1 100644 --- a/lib/WebSocket.js +++ b/lib/WebSocket.js @@ -55,11 +55,12 @@ class WebSocket extends EventEmitter { this._binaryType = constants.BINARY_TYPES[0]; this._finalize = this.finalize.bind(this); + this._closeFrameReceived = false; this._closeFrameSent = false; - this._closeMessage = null; + this._closeMessage = ''; this._closeTimer = null; this._finalized = false; - this._closeCode = null; + this._closeCode = 1006; this._receiver = null; this._sender = null; this._socket = null; @@ -126,21 +127,17 @@ class WebSocket extends EventEmitter { this._ultron = new Ultron(socket); this._socket = socket; - // socket cleanup handlers this._ultron.on('close', this._finalize); this._ultron.on('error', this._finalize); this._ultron.on('end', this._finalize); - // ensure that the head is added to the receiver if (head.length > 0) socket.unshift(head); - // subsequent packets are pushed to the receiver this._ultron.on('data', (data) => { this.bytesReceived += data.length; this._receiver.add(data); }); - // receiver event handlers this._receiver.onmessage = (data) => this.emit('message', data); this._receiver.onping = (data) => { this.pong(data, !this._isServer, true); @@ -148,14 +145,22 @@ class WebSocket extends EventEmitter { }; this._receiver.onpong = (data) => this.emit('pong', data); this._receiver.onclose = (code, reason) => { + this._closeFrameReceived = true; this._closeMessage = reason; this._closeCode = code; if (!this._finalized) this.close(code, reason); }; this._receiver.onerror = (error, code) => { - // close the connection when the receiver reports a HyBi error code - this.close(code, ''); + this._closeMessage = ''; + this._closeCode = code; + + // + // Ensure that the error is emitted even if `WebSocket#finalize()` has + // already been called. + // + this.readyState = WebSocket.CLOSING; this.emit('error', error); + this.finalize(true); }; this.readyState = WebSocket.OPEN; @@ -174,7 +179,8 @@ class WebSocket extends EventEmitter { this.readyState = WebSocket.CLOSING; this._finalized = true; - if (!this._socket) return this.emitClose(error); + if (typeof error === 'object') this.emit('error', error); + if (!this._socket) return this.emitClose(); clearTimeout(this._closeTimer); this._closeTimer = null; @@ -190,29 +196,19 @@ class WebSocket extends EventEmitter { this._socket = null; this._sender = null; - this._receiver.cleanup(() => this.emitClose(error)); + this._receiver.cleanup(() => this.emitClose()); this._receiver = null; } /** * Emit the `close` event. * - * @param {(Boolean|Error)} error Indicates whether or not an error occurred * @private */ - emitClose (error) { + emitClose () { this.readyState = WebSocket.CLOSED; - // - // If the connection was closed abnormally (with an error), or if the close - // control frame was not sent or received then the close code must be 1006. - // - if (error || !this._closeFrameSent) { - this._closeMessage = ''; - this._closeCode = 1006; - } - - this.emit('close', this._closeCode || 1006, this._closeMessage || ''); + this.emit('close', this._closeCode, this._closeMessage); if (this.extensions[PerMessageDeflate.extensionName]) { this.extensions[PerMessageDeflate.extensionName].cleanup(); @@ -271,37 +267,35 @@ class WebSocket extends EventEmitter { close (code, data) { if (this.readyState === WebSocket.CLOSED) return; if (this.readyState === WebSocket.CONNECTING) { - if (this._req && !this._req.aborted) { - this._req.abort(); - this.emit('error', new Error('closed before the connection is established')); - this.finalize(true); - } + this._req.abort(); + this.finalize(new Error('closed before the connection is established')); return; } if (this.readyState === WebSocket.CLOSING) { - if (this._closeFrameSent && this._closeCode) this._socket.end(); + if (this._closeFrameSent && this._closeFrameReceived) this._socket.end(); return; } this.readyState = WebSocket.CLOSING; this._sender.close(code, data, !this._isServer, (err) => { - if (this._finalized) return; - - if (err) { - this.emit('error', err); - this.finalize(true); - return; - } + // + // This error is handled by the `'error'` listener on the socket. We only + // want to know if the close frame has been sent here. + // + if (err) return; - if (this._closeCode) this._socket.end(); this._closeFrameSent = true; - // - // Ensure that the connection is cleaned up even when the closing - // handshake fails. - // - this._closeTimer = setTimeout(this._finalize, closeTimeout, true); + if (!this._finalized) { + if (this._closeFrameReceived) this._socket.end(); + + // + // Ensure that the connection is cleaned up even when the closing + // handshake fails. + // + this._closeTimer = setTimeout(this._finalize, closeTimeout, true); + } }); } @@ -391,11 +385,8 @@ class WebSocket extends EventEmitter { terminate () { if (this.readyState === WebSocket.CLOSED) return; if (this.readyState === WebSocket.CONNECTING) { - if (this._req && !this._req.aborted) { - this._req.abort(); - this.emit('error', new Error('closed before the connection is established')); - this.finalize(true); - } + this._req.abort(); + this.finalize(new Error('closed before the connection is established')); return; } @@ -645,8 +636,7 @@ function initAsClient (address, protocols, options) { if (options.handshakeTimeout) { this._req.setTimeout(options.handshakeTimeout, () => { this._req.abort(); - this.emit('error', new Error('opening handshake has timed out')); - this.finalize(true); + this.finalize(new Error('opening handshake has timed out')); }); } @@ -654,15 +644,13 @@ function initAsClient (address, protocols, options) { if (this._req.aborted) return; this._req = null; - this.emit('error', error); - this.finalize(true); + this.finalize(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(new Error(`unexpected server response (${res.statusCode})`)); } }); @@ -683,8 +671,7 @@ function initAsClient (address, protocols, options) { if (res.headers['sec-websocket-accept'] !== digest) { socket.destroy(); - this.emit('error', new Error('invalid server key')); - return this.finalize(true); + return this.finalize(new Error('invalid server key')); } const serverProt = res.headers['sec-websocket-protocol']; @@ -701,8 +688,7 @@ function initAsClient (address, protocols, options) { if (protError) { socket.destroy(); - this.emit('error', new Error(protError)); - return this.finalize(true); + return this.finalize(new Error(protError)); } if (serverProt) this.protocol = serverProt; @@ -721,8 +707,8 @@ function initAsClient (address, protocols, options) { } } catch (err) { socket.destroy(); - this.emit('error', new Error('invalid Sec-WebSocket-Extensions header')); - return this.finalize(true); + this.finalize(new Error('invalid Sec-WebSocket-Extensions header')); + return; } } diff --git a/test/WebSocket.test.js b/test/WebSocket.test.js index 70df7c101..32a4245b8 100644 --- a/test/WebSocket.test.js +++ b/test/WebSocket.test.js @@ -6,8 +6,8 @@ const safeBuffer = require('safe-buffer'); const assert = require('assert'); const crypto = require('crypto'); const https = require('https'); -const dns = require('dns'); const http = require('http'); +const dns = require('dns'); const net = require('net'); const fs = require('fs'); const os = require('os'); @@ -1189,14 +1189,6 @@ describe('WebSocket', function () { }); wss.on('connection', (ws) => { - // - // Remove our `'error'` listener from the socket to ensure that - // `WebSocket#finalize()` is not called before the `Sender#close()` - // callback. - // - const listeners = ws._socket.listeners('error'); - ws._socket.removeListener('error', listeners[listeners.length - 1]); - ws.on('error', (err) => { assert.ok(err instanceof Error); assert.ok(err.message.startsWith('write E')); @@ -1304,39 +1296,6 @@ describe('WebSocket', function () { wss.on('connection', (ws) => ws.close(1013)); }); - it('closes the connection when an error occurs', function (done) { - const server = http.createServer(); - const wss = new WebSocket.Server({ server }); - let closed = false; - - wss.on('connection', (ws) => { - ws.on('error', (err) => { - assert.ok(err instanceof Error); - assert.strictEqual(err.message, 'RSV2 and RSV3 must be clear'); - - ws.on('close', (code, reason) => { - assert.strictEqual(code, 1006); - assert.strictEqual(reason, ''); - - closed = true; - }); - }); - }); - - server.listen(0, () => { - const ws = new WebSocket(`ws://localhost:${server.address().port}`); - - ws.on('open', () => ws._socket.write(Buffer.from([0xa1, 0x00]))); - ws.on('close', (code, reason) => { - assert.strictEqual(code, 1002); - assert.strictEqual(reason, ''); - assert.ok(closed); - - server.close(done); - }); - }); - }); - it('does nothing if the connection is already CLOSED', function (done) { const wss = new WebSocket.Server({ port: 0 }, () => { const port = wss._server.address().port; @@ -1432,75 +1391,6 @@ describe('WebSocket', function () { }); }); - describe('abnormal closures', function () { - it('ignores close frame data if connection is forcibly closed', function (done) { - const wss = new WebSocket.Server({ port: 0 }, () => { - const port = wss._server.address().port; - const ws = new WebSocket(`ws://localhost:${port}`); - const emitClose = ws.emitClose; - const messages = []; - - ws.on('message', (message) => { - messages.push(message); - if (messages.length > 1) ws.terminate(); - }); - - ws.emitClose = (error) => { - assert.deepStrictEqual(messages, ['', '', '']); - assert.strictEqual(ws._closeMessage, 'foo'); - assert.strictEqual(ws._closeCode, 4000); - assert.strictEqual(error, true); - emitClose.call(ws, error); - }; - - ws.on('close', (code, message) => { - assert.strictEqual(message, ''); - assert.strictEqual(code, 1006); - wss.close(done); - }); - }); - - wss.on('connection', (ws) => { - const buf = Buffer.from('81008100810088050fa0666f6f', 'hex'); - ws._socket.write(buf); - }); - }); - - it('closes with 1006 if close frame is received but not sent', function (done) { - const wss = new WebSocket.Server({ - perMessageDeflate: { threshold: 0 }, - port: 0 - }, () => { - const port = wss._server.address().port; - const ws = new WebSocket(`ws://localhost:${port}`); - const emitClose = ws.emitClose; - const messages = []; - - ws.on('message', (message) => messages.push(message)); - - ws.emitClose = (error) => { - assert.deepStrictEqual(messages, ['', '', '']); - assert.strictEqual(ws._closeFrameSent, false); - assert.strictEqual(ws._closeMessage, 'foo'); - assert.strictEqual(ws._closeCode, 4000); - assert.strictEqual(error, undefined); - emitClose.call(ws, error); - }; - - ws.on('close', (code, message) => { - assert.strictEqual(message, ''); - assert.strictEqual(code, 1006); - wss.close(done); - }); - }); - - wss.on('connection', (ws) => { - const buf = Buffer.from('c10100c10100c1010088050fa0666f6f', 'hex'); - ws._socket.end(buf); - }); - }); - }); - describe('WHATWG API emulation', function () { it('should not throw errors when getting and setting', function () { const listener = () => {}; @@ -1677,6 +1567,7 @@ describe('WebSocket', function () { ws.addEventListener('close', (closeEvent) => { assert.ok(closeEvent.wasClean); + assert.strictEqual(closeEvent.reason, ''); assert.strictEqual(closeEvent.code, 1000); wss.close(done); }); @@ -1685,43 +1576,15 @@ describe('WebSocket', function () { wss.on('connection', (client) => client.close(1000)); }); - it('should assign "true" to wasClean when server closes with code 3000', function (done) { - const wss = new WebSocket.Server({ port: 0 }, () => { - const port = wss._server.address().port; - const ws = new WebSocket(`ws://localhost:${port}`); - - ws.addEventListener('close', (closeEvent) => { - assert.ok(closeEvent.wasClean); - wss.close(done); - }); - }); - - wss.on('connection', (client) => client.close(3000)); - }); - - it('should assign "true" to wasClean when server closes with code 4999', function (done) { - const wss = new WebSocket.Server({ port: 0 }, () => { - const port = wss._server.address().port; - const ws = new WebSocket(`ws://localhost:${port}`); - - ws.addEventListener('close', (closeEvent) => { - assert.ok(closeEvent.wasClean); - wss.close(done); - }); - }); - - wss.on('connection', (client) => client.close(4999)); - }); - it('should receive valid CloseEvent when server closes with code 1001', function (done) { const wss = new WebSocket.Server({ port: 0 }, () => { const port = wss._server.address().port; const ws = new WebSocket(`ws://localhost:${port}`); ws.addEventListener('close', (closeEvent) => { - assert.ok(!closeEvent.wasClean); - assert.strictEqual(closeEvent.code, 1001); + assert.ok(closeEvent.wasClean); assert.strictEqual(closeEvent.reason, 'some daft reason'); + assert.strictEqual(closeEvent.code, 1001); wss.close(done); }); }); @@ -2320,24 +2183,33 @@ describe('WebSocket', function () { it('can be used while data is being processed', function (done) { const wss = new WebSocket.Server({ - perMessageDeflate: { threshold: 0 }, + perMessageDeflate: true, port: 0 }, () => { const port = wss._server.address().port; - const ws = new WebSocket(`ws://localhost:${port}`, { - perMessageDeflate: { threshold: 0 } - }); + const ws = new WebSocket(`ws://localhost:${port}`); + const messages = []; - wss.on('connection', (client) => { - for (let i = 0; i < 10; i++) { - client.send('hi'); - } - client.send('hi', () => { + ws.on('message', (message) => { + if (messages.push(message) > 1) return; + + process.nextTick(() => { assert.strictEqual(ws._receiver._state, 5); - ws.on('close', () => wss.close(done)); ws.terminate(); }); }); + + ws.on('close', (code, reason) => { + assert.deepStrictEqual(messages, ['', '', '', '']); + assert.strictEqual(code, 1006); + assert.strictEqual(reason, ''); + wss.close(done); + }); + }); + + wss.on('connection', (ws) => { + const buf = Buffer.from('c10100c10100c10100c10100', 'hex'); + ws._socket.write(buf); }); }); }); diff --git a/test/WebSocketServer.test.js b/test/WebSocketServer.test.js index c367d6117..fc8bbe266 100644 --- a/test/WebSocketServer.test.js +++ b/test/WebSocketServer.test.js @@ -105,23 +105,6 @@ describe('WebSocketServer', function () { ws.on('open', () => new WebSocket(`ws+unix://${sockPath}`)); }); }); - - it('will not crash when it receives an unhandled opcode', function (done) { - const wss = new WebSocket.Server({ port: 0 }, () => { - const port = wss._server.address().port; - const ws = new WebSocket(`ws://localhost:${port}/`); - - ws.on('open', () => ws._socket.write(Buffer.from([0x85, 0x00]))); - }); - - wss.on('connection', (ws) => { - ws.on('error', (err) => { - assert.ok(err instanceof Error); - assert.strictEqual(err.message, 'invalid opcode: 5'); - wss.close(done); - }); - }); - }); }); describe('#close', function () { @@ -878,6 +861,35 @@ describe('WebSocketServer', function () { client.send(data); }); }); + + it('does not crash when it receives an unhandled opcode', function (done) { + let closed = false; + const wss = new WebSocket.Server({ port: 0 }, () => { + const port = wss._server.address().port; + const ws = new WebSocket(`ws://localhost:${port}/`); + + ws.on('open', () => ws._socket.write(Buffer.from([0x85, 0x00]))); + ws.on('close', (code, reason) => { + assert.strictEqual(code, 1006); + assert.strictEqual(reason, ''); + assert.ok(closed); + wss.close(done); + }); + }); + + wss.on('connection', (ws) => { + ws.on('error', (err) => { + assert.ok(err instanceof Error); + assert.strictEqual(err.message, 'invalid opcode: 5'); + + ws.on('close', (code, reason) => { + assert.strictEqual(code, 1002); + assert.strictEqual(reason, ''); + closed = true; + }); + }); + }); + }); }); describe('client properties', function () {