Skip to content

Commit

Permalink
[fix] Make listeners added via event handler properties independent
Browse files Browse the repository at this point in the history
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: #1818
  • Loading branch information
lpinca committed Jul 16, 2021
1 parent 1bd93f0 commit 0b21c03
Show file tree
Hide file tree
Showing 4 changed files with 89 additions and 37 deletions.
1 change: 1 addition & 0 deletions lib/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
Expand Down
63 changes: 32 additions & 31 deletions lib/event-target.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use strict';

const { kListener } = require('./constants');
const { kForOnEventAttribute, kListener } = require('./constants');

/**
* Class representing an event.
Expand Down Expand Up @@ -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);
}
},

Expand All @@ -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;
}
Expand Down
15 changes: 9 additions & 6 deletions lib/websocket.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ const {
BINARY_TYPES,
EMPTY_BUFFER,
GUID,
kForOnEventAttribute,
kListener,
kStatusCode,
kWebSocket,
Expand Down Expand Up @@ -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
});
}
});
});
Expand Down
47 changes: 47 additions & 0 deletions test/websocket.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down Expand Up @@ -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() });

Expand Down Expand Up @@ -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) => {
Expand Down

0 comments on commit 0b21c03

Please sign in to comment.