node-ldapjs icon indicating copy to clipboard operation
node-ldapjs copied to clipboard

Not handling error properly for invalid filter value

Open prashanthmadduri opened this issue 5 years ago • 3 comments

Hi Team,

The ldapjs client API is not throwing proper error when passing incorrect filter data.

Below is code snippet:

var ldap = require('ldapjs')
var client = ldap.createClient({
  url: 'ldap://<<ldap host>>:389'
})

client.bind(<<LDAP Admin User>>, <<Password>>, function(err) {
  if (err) {
	  console.log('Error :  ', err)
  } else {
    let searchOpts = {
		scope: 'sub',
		filter: 'cn',
		attributes: []
	}
    client.search('dc=test,dc=com', searchOpts, function(err, res) {
      if (err) {
        console.log('error***************', err)
      } else {
        let count = 1
        res.on('searchEntry', function(entry) {
          console.log('entry  : ' + JSON.stringify(entry.object))
        })
        res.on('page', function(result, cb) {
          console.log('page end', result.controls)
        })
        res.on('error', function(err) {
          console.error('error: ' + err.message)
        })
        res.on('end', function(result) {
          console.log('result: ' + result)
          client.unbind(function(err) {
            if (err) {
                console.log('error unbind : ', err)
            } else {
                console.log('unbind is success')
            }
          })
        })
      }
    })
  }
})

Here, we are passing incorrect filter parameter.

D:\node_modules\ldap-filter\lib\index.js:200
      throw new Error(expr + ' is invalid');
      ^

Error: cn is invalid
    at _buildFilterTree (D:\node_modules\ldap-filter\lib\index.js:200:13)
    at _parseString (D:\node_modules\ldap-filter\lib\index.js:411:17)
    at Object.parse (D:\node_modules\ldap-filter\lib\index.js:422:12)
    at Object.parseString (D:\node_modules\ldapjs\lib\filters\index.js:176:25)
    at Client.search (D:\node_modules\ldapjs\lib\client\client.js:769:30)
    at D:\ApacheDS\apacheds_test_retrieve.js:29:12
    at sendResult (D:\node_modules\ldapjs\lib\client\client.js:1395:12)
    at messageCallback (D:\node_modules\ldapjs\lib\client\client.js:1421:16)
    at Parser.onMessage (D:\node_modules\ldapjs\lib\client\client.js:1089:14)
    at Parser.emit (events.js:198:13)

Here, it is not throwing any error from ldapjs error http://ldapjs.org/errors.html. It seems the error handling is not implemented properly.

prashanthmadduri avatar May 28 '20 14:05 prashanthmadduri

The filter is processed at https://github.com/ldapjs/node-ldapjs/blob/d8c428f1d55eed3b96508bd84616550393336cd3/lib/client/client.js#L562

Which goes to https://github.com/ldapjs/node-ldapjs/blob/d8c428f1d55eed3b96508bd84616550393336cd3/lib/filters/index.js#L178-L184

I suspect this will solve your issue:

try {
  client.search(...)
} catch (cause) {
  // deal with error
}

jsumners avatar May 28 '20 15:05 jsumners

Hi @jsumners

Thank you for your inputs but it is not possible to handle with generic error for invalid filter parameter. If we use try....catch for client.search(...) then how we will implement different error handling mechanism for different errors.

As per our use case we are implementing LDAP client application using ldapjs node module. We have to throw invalid input for incorrect filter data to warn user to re-check on user input, to do this ldapjs should handle the invalid filter error and throw appropriate ldapjs error not generic error or not server error directly.

prashanthmadduri avatar May 29 '20 09:05 prashanthmadduri

A PR is welcome.

jsumners avatar May 29 '20 11:05 jsumners

👋

On February 22, 2023, we released version 3 of this library. As a result, we are closing this issue/pull request.

Please see issue #839 for more information, including how to proceed if you feel this closure is in error.

jsumners avatar Feb 22 '23 19:02 jsumners