[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: websockets/ws#1818
th37rose · Jul 29, 2021 · c6c2567 · c6c2567
1 parent ac50d6a
commit c6c2567
Show file tree

Hide file tree

Showing 4 changed files with 89 additions and 37 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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) => {