From 0b21c03a6e69f8e37b2dfe55c4e753575fc09ac7 Mon Sep 17 00:00:00 2001 From: Luigi Pinca Date: Fri, 16 Jul 2021 18:44:34 +0200 Subject: [PATCH] [fix] Make listeners added via event handler properties independent Prevent the `onclose`, `onerror`, `onmessage`, and `onopen` getters and setters from returning or removing event listeners added with `WebSocket.prototype.addEventListener()`. Also prevent `WebSocket.prototype.removeEventListener()` from removing event listeners added with the `onclose`, `onerror`, `onmessage`, and `onopen` setters. Refs: https://github.com/websockets/ws/issues/1818 --- lib/constants.js | 1 + lib/event-target.js | 63 +++++++++++++++++++++--------------------- lib/websocket.js | 15 ++++++---- test/websocket.test.js | 47 +++++++++++++++++++++++++++++++ 4 files changed, 89 insertions(+), 37 deletions(-) diff --git a/lib/constants.js b/lib/constants.js index dce5dd21a..d691b30a1 100644 --- a/lib/constants.js +++ b/lib/constants.js @@ -4,6 +4,7 @@ module.exports = { BINARY_TYPES: ['nodebuffer', 'arraybuffer', 'fragments'], EMPTY_BUFFER: Buffer.alloc(0), GUID: '258EAFA5-E914-47DA-95CA-C5AB0DC85B11', + kForOnEventAttribute: Symbol('kIsForOnEventAttribute'), kListener: Symbol('kListener'), kStatusCode: Symbol('status-code'), kWebSocket: Symbol('websocket'), diff --git a/lib/event-target.js b/lib/event-target.js index 7230a2081..b135463d4 100644 --- a/lib/event-target.js +++ b/lib/event-target.js @@ -1,6 +1,6 @@ 'use strict'; -const { kListener } = require('./constants'); +const { kForOnEventAttribute, kListener } = require('./constants'); /** * Class representing an event. @@ -127,40 +127,41 @@ const EventTarget = { * the listener would be automatically removed when invoked. * @public */ - addEventListener(type, listener, options) { - function onMessage(data, isBinary) { - listener.call( - this, - new MessageEvent(isBinary ? data : data.toString(), this) - ); - } - - function onClose(code, message) { - listener.call(this, new CloseEvent(code, message.toString(), this)); - } - - function onError(error) { - listener.call(this, new ErrorEvent(error, this)); - } - - function onOpen() { - listener.call(this, new OpenEvent(this)); - } - - const method = options && options.once ? 'once' : 'on'; + addEventListener( + type, + listener, + options = { once: false, [kForOnEventAttribute]: false } + ) { + let wrapper; if (type === 'message') { - onMessage[kListener] = listener; - this[method](type, onMessage); + wrapper = function onMessage(data, isBinary) { + const event = new MessageEvent(isBinary ? data : data.toString(), this); + listener.call(this, event); + }; } else if (type === 'close') { - onClose[kListener] = listener; - this[method](type, onClose); + wrapper = function onClose(code, message) { + listener.call(this, new CloseEvent(code, message.toString(), this)); + }; } else if (type === 'error') { - onError[kListener] = listener; - this[method](type, onError); + wrapper = function onError(error) { + listener.call(this, new ErrorEvent(error, this)); + }; } else if (type === 'open') { - onOpen[kListener] = listener; - this[method](type, onOpen); + wrapper = function onOpen() { + listener.call(this, new OpenEvent(this)); + }; + } else { + return; + } + + wrapper[kForOnEventAttribute] = options[kForOnEventAttribute]; + wrapper[kListener] = listener; + + if (options.once) { + this.once(type, wrapper); + } else { + this.on(type, wrapper); } }, @@ -173,7 +174,7 @@ const EventTarget = { */ removeEventListener(type, handler) { for (const listener of this.listeners(type)) { - if (listener[kListener] === handler) { + if (listener[kListener] === handler && !listener[kForOnEventAttribute]) { this.removeListener(type, listener); break; } diff --git a/lib/websocket.js b/lib/websocket.js index bf33b6ce8..5b5ecd3cf 100644 --- a/lib/websocket.js +++ b/lib/websocket.js @@ -15,6 +15,7 @@ const { BINARY_TYPES, EMPTY_BUFFER, GUID, + kForOnEventAttribute, kListener, kStatusCode, kWebSocket, @@ -524,22 +525,24 @@ Object.defineProperty(WebSocket.prototype, 'CLOSED', { enumerable: true, get() { for (const listener of this.listeners(method)) { - if (listener[kListener]) return listener[kListener]; + if (listener[kForOnEventAttribute]) return listener[kListener]; } return undefined; }, set(handler) { for (const listener of this.listeners(method)) { - // - // Remove only the listeners added via `addEventListener`. - // - if (listener[kListener]) this.removeListener(method, listener); + if (listener[kForOnEventAttribute]) { + this.removeListener(method, listener); + break; + } } if (typeof handler !== 'function') return; - this.addEventListener(method, handler); + this.addEventListener(method, handler, { + [kForOnEventAttribute]: true + }); } }); }); diff --git a/test/websocket.test.js b/test/websocket.test.js index 1af00252b..3883d855a 100644 --- a/test/websocket.test.js +++ b/test/websocket.test.js @@ -2111,6 +2111,11 @@ describe('WebSocket', () => { assert.strictEqual(ws.onclose, NOOP); assert.strictEqual(ws.onerror, NOOP); assert.strictEqual(ws.onopen, NOOP); + + ws.onmessage = 'foo'; + + assert.strictEqual(ws.onmessage, undefined); + assert.strictEqual(ws.listenerCount('onmessage'), 0); }); it('works like the `EventEmitter` interface', (done) => { @@ -2199,6 +2204,40 @@ describe('WebSocket', () => { assert.deepStrictEqual(events, ['open', 'message']); }); + it("doesn't return listeners added with `addEventListener`", () => { + const ws = new WebSocket('ws://localhost', { agent: new CustomAgent() }); + + ws.addEventListener('open', NOOP); + + const listeners = ws.listeners('open'); + + assert.strictEqual(listeners.length, 1); + assert.strictEqual(listeners[0][kListener], NOOP); + + assert.strictEqual(ws.onopen, undefined); + }); + + it("doesn't remove listeners added with `addEventListener`", () => { + const ws = new WebSocket('ws://localhost', { agent: new CustomAgent() }); + + ws.addEventListener('close', NOOP); + ws.onclose = NOOP; + + let listeners = ws.listeners('close'); + + assert.strictEqual(listeners.length, 2); + assert.strictEqual(listeners[0][kListener], NOOP); + assert.strictEqual(listeners[1][kListener], NOOP); + + ws.onclose = NOOP; + + listeners = ws.listeners('close'); + + assert.strictEqual(listeners.length, 2); + assert.strictEqual(listeners[0][kListener], NOOP); + assert.strictEqual(listeners[1][kListener], NOOP); + }); + it('supports the `removeEventListener` method', () => { const ws = new WebSocket('ws://localhost', { agent: new CustomAgent() }); @@ -2257,6 +2296,14 @@ describe('WebSocket', () => { ws.removeEventListener('message', NOOP); assert.deepStrictEqual(ws.listeners('message'), [NOOP]); + + ws.onclose = NOOP; + + assert.strictEqual(ws.listeners('close')[0][kListener], NOOP); + + ws.removeEventListener('close', NOOP); + + assert.strictEqual(ws.listeners('close')[0][kListener], NOOP); }); it('wraps text data in a `MessageEvent`', (done) => {