http client error events might be suppressed

[Flarna]

I think this check in http_p.js should be improved as it may result in not re-emitting an error event.

this._events.error is either an Array (if more then 1 listener is installed) or Function (only 1 listener installed).
Currently this check works because the installed listener errorCapturer has exactly 1 named parameter therefore errorCapturer.length === 1.
But if there is a module used which wraps the installed listeners (e.g. for CLS,...) or if once is used to install the listener this is broken. Besides that it relies on internals of EventEmitter (the _events field`).

I recommend to use the public EventEmitter Api to check that <= 1 listener is installed.

Recent NodeJs versions allow to listen on errorMonitor to avoid the need to re-emit in case only 1 listener is present at all.

Edit:
Found that also postgres and mysql is effected.

[willarmiros]

Hi @Flarna,
Thank you for pointing out this potential vulnerability. It appears that errorMonitor is only available in Node 12 and later, but this SDK still supports Node 10. So do you think a valid alternative would be to use the listenerCount API, e.g.

if (this.listenerCount('error') === 1) {
  this.removeListener('error', errorCapturer);
  this.emit('error', e);
}

[Flarna]

What about the following to use errorMonitor if present and fall back to error and reemit if not, e.g.:

req.on(req.errorMonitor || 'errror', errorCapturer)

and in listener:

if ((this.errorMonitor != null) && (this.listenerCount('error') <= 1)) {
  this.removeListener('error', errorCapturer);
  this.emit('error', e);
}

Main issue is that there are other modules out in the wild using the same "if I'm the only listener remove myself and reemit" algorithm which works fine as long as you are alone.

Reason for listenerCount() <= 1 instead === 1 is that I got hit by some module which also patched EventEmitter and there was some race between my and there code ending up in listenerCount reporting 0 even I was in in the middle of my listener :o).

[willarmiros]

Ok I'm fine with that solution of trying errorMonitor and falling back to the event name. We can remove the fallback once Node 10 is EOL. I will make a PR shortly.

You're right about using <= 1. Reading the docs more closely, it looks like for a given event, one listener could remove all listeners, making the listenerCount 0, but the remaining listeners would still be triggered for that event before actually being removed.