more errors not passed to callbacks

nazrhyn commented 8 years ago

Issue

We found another error case that isn't bubbling up properly through the err parameter to the callback. When the search filter is syntactically incorrect, an example call stack looks like this:

Error: (userPrincipalName=user@company.com has unbalanced parentheses
    at matchParens (...\node_modules\ldap-filter\lib\index.js:49:11)
    at _buildFilterTree (...\node_modules\ldap-filter\lib\index.js:155:18)
    at _parseString (...\node_modules\ldap-filter\lib\index.js:411:17)
    at Object.module.exports.parse (...\node_modules\ldap-filter\lib\index.js:422:12)
    at Object.parseString (...\node_modules\ldapjs\lib\filters\index.js:176:25)
    at Client.search (...\node_modules\ldapjs\lib\client\client.js:769:30)
    at ...\node_modules\ldapauth-fork\lib\ldapauth.js:197:23
    at ...\node_modules\ldapauth-fork\lib\ldapauth.js:178:12

ldap-filter is throwing when the filter parser detects syntax errors.

Ultimately, it seems like this library needs to just change to be, overall, more resilient to errors lower in the stack of its interactions with the lower-level LDAP modules. This issue, in our case, affects us through our use of vesse/passport-ldapauth.

Do you have any plans to address these error handling issues that keep cropping up in different flavors?

nazrhyn commented 8 years ago

I've opened mcavage/node-ldapjs#327 at the source, as well. Even if it is fixed, given that we've seen this problem before, I might still err on the side of more robust error handling.

vesse commented 8 years ago

I have long acknowledged the need for better error handling but haven't really invested time in it since I don't know a trivial fix that would handle all the errors.

Your previous PR would probably solve most cases that occur when trying to connect or search, but it looks to me that if there was eg. a socket error at some point when there is no authenticate call being made the emitted error would still leak? If the clients are assigned error listeners in the constructor no errors would leak, but then there is no access to the user callback either.

The original author started a rewrite that emits some errors, but going down that road would only lead to passport-ldapauth having problems with how to catch emitted errors and pass them to user callback (note though, that since passport-ldapauth currently creates a new client on every call this shouldn't be a problem).

I really am open for good ideas, so please share your thoughts.

nazrhyn commented 8 years ago

Thanks for the reply!

It seems like the biggest problem is the one I was confirming my understanding of with this code:

(gist with the full code)

try {
    doer.execute('cracker');
} catch (e) {
    console.error('CAUGHT! >', e);
}

That try/catch won't do a thing if there's an async gap somewhere in what Doer#execute does. So, even if we wrapped all of the interactions with the ldapjs library in try/catch blocks, if we uncovered another error they weren't properly handling that wasn't contiguous with the original call, it would end up unhandled again.

Handling emissions of the error event doesn't even seem like a panacea, as the error we found originates from this line:

if (str.charAt(ndx) !== ')')
  throw new Error(str + ' has unbalanced parentheses');

Following the call stack all the way back up to the Client class, there's no construct in place to handle this exception at all, which means it's going to bubble all the way out!

It's definitely a tricky prospect when you can't trust the other modules you're using to behave properly. If I were you, I would definitely be tempted to just say that it's an ldapjs issue because it doesn't behave properly with regard to error transmission.

Looking at the changes you linked to in the original node-ldapauth, I feel like making the thing into an EventEmitter just introduces some of the same confusions present in ldapjs. We've already seen situations where the error event is emitted, but then the nodeback isn't called with the same error. Supporting both seems to mean that one has to effectively handle each error in two different ways! I am admittedly still somewhat new to the Node.js world (only about a year and a half of experience, at this point); is it common to support both simultaneously?

(I'll cut off the rambling/stream-of-consciousness here so we can have a two-sided conversation :smile:.)

vesse commented 7 years ago

I have finally made ldapauth-fork into an EventEmitter so the errors ldapjs emits are now re-emitted (and thus can be handled in eg. passport-ldapauth). I didn't do anything about this search filter exception (or others that may be hiding there), I too think that they should be considered as programming errors since you would notice that on the first login attempt and then probably fixed the filter straight away.

I don't know how common it is to use both EventEmitter and callbacks, but at least from the top of my head I don't remember having used other such libraries.

vesse / node-ldapauth-fork

more errors not passed to callbacks #30

Related

Issue