Uncaught exception "Illegal unescaped char"

[Frank683]

We have recently migrated from node 8 to node 12 and due to the socket problems of ldapjs we switched from activedirectory module to your activedirectory2@2.0.0-pre.1 because it is maintained and has the necessary fixes to run with node >= 10.

Unfortunately we have one user who crashes the process when he loggs in and his AD groups are retrieved:

Uncaught exception: {"name":"Error","message":"illegal unescaped char: (","stack":"Error: illegal unescaped char: (\n at escapeValue (/home/app/node_modules/ldap-filter/lib/index.js:49:13)\n at parseExpr (/home/app/node_modules/ldap-filter/lib/index.js:167:16)\n at parseFilter (/home/app/node_modules/ldap-filter/lib/index.js:228:14)\n at Object.parse (/home/app/node_modules/ldap-filter/lib/index.js:252:18)\n at Object.parseString (/home/app/node_modules/ldapjs/lib/filters/index.js:180:27)\n at Client.search (/home/app/node_modules/ldapjs/lib/client/client.js:566:30)\n at Searcher.search (/home/app/node_modules/activedirectory2/lib/components/search.js:118:15)\n at search (/home/app/node_modules/activedirectory2/lib/components/search.js:352:19)\n at GroupMembersForDN.getMembers (/home/app/node_modules/activedirectory2/lib/components/getGroupMembershipForDN.js:111:3)\n at getGroupMembershipForDN (/home/app/node_modules/activedirectory2/lib/components/getGroupMembershipForDN.js:157:17)\n at ActiveDirectory.ggmfd [as getGroupMembershipForDN] (/home/app/node_modules/activedirectory2/lib/activedirectory.js:292:10)\n at /home/app/node_modules/activedirectory2/lib/activedirectory.js:489:10\n at /home/app/node_modules/activedirectory2/lib/activedirectory.js:380:5\n at Searcher.callback (/home/app/node_modules/activedirectory2/lib/activedirectory.js:340:5)\n at Searcher.onSearchEnd (/home/app/node_modules/activedirectory2/lib/components/search.js:283:12)\n at EventEmitter.<anonymous> (/home/app/node_modules/activedirectory2/lib/components/search.js:149:12)"}

At the moment I do not have any more insights and I guess it will be hard finding out which is possibly the group name which leads to the crash.

Do you have any idea?

Until we can fix this issue we're unfortunately forced to go back to the "old" version using activedirectoy with a scheduled hourly restart of the process in order not to exceed the max. number of file descriptors (sockets) per process on our linux server.

[jsumners]

It looks to me like the unescaped character is (. It's likely this function is not interpreting a DN correctly:

https://github.com/jsumners/node-activedirectory/blob/5a519c9334c9ec88ba78666d621d40a9d833b355/lib/components/utilities.js#L259-L333

Unfortunately, without a reproducible example, that is the most I can offer.

[jurjendijkstra]

Actually, I don't think so, because this function parseDistinguishedName does escape the '(' character.

In lib/components/parseRangeAttributes.js there is also a function parseDistinguishedName . It does not escape as much as the one in utilities.js.

I have been wondering why the two functions are different. Perhaps they should be identical. (Well they are different because I edited one and did not have the guts to edit the other too. Before that they were identical).

[Frank683]

@jsumners I don't know how to provide an example without disclosing any company or user data - which I can't do. What I can tell is that the user who caused the exception has a DN with ( and ) characters in it. None of his group names do contain these characters.

So what @jurjendijkstra wrote before makes sense to me and could be it. Can you verify that?

[jurjendijkstra]

@Frank683 maybe you can best verify it yourself, after all you have the AD server with that data. Just copy function parseDistinguishedName from utilities.js to parseRangeAttributes.js and try if it works. If it works (my guess is yes it works) then somehow tidy up the code so it is not a copy/pasted function.

[jsumners]

An example doesn't have to include sensitive data. Just a replication of the error condition.

[Frank683]

The below part of function parseDistinguishedName in utilities.js causes the problem:

 } else {
      // comma was not a component separater but was embedded in a componentvalue
      component[i - 1] += '\\'
    }

If we have some DN like

CN=Doe\, John (Test),OU=Technicians,OU=Users,OU=Local Resources,OU=DEDUS,DC=abc,DC=dom the part John (Test) after the comma - which is by the way already escaped when it comes into this function - is not treated anymore due to this if-else construct.

So refering to the source code comment: even when the comma was embedded in a componentvalue the componentvalue must be checked for any further characters to be escaped.

In our AD the Comma already has the escape character:

Callstack and locals:

The problem is indeed the opening bracket which is not escaped.

[jsumners]

Would you be willing to submit a PR to fix the issue?

[Frank683]

Would you be willing to submit a PR to fix the issue?

Basically yes. What I'm not sure about is the "double" escaping of an already escaped comma as done in the else block. Since this is not the case in the "original" activedirectory project from which you forked I guess you introduced this? Shall we keep it as it is and just make sure that the componentvalue is scanned for further characters which need escaping?

[jsumners]

Yes, this is a fork of the original activedirectory module.

I cannot remember all of the details within this library. What I can say is that it significantly diverges from that original module. This version includes many fixes (and clearly its own bugs).

I defer to @jurjendijkstra's judgement on how to best update the parseDistinguishedName function. He is much more familiar with it than I am at this point.

[jurjendijkstra]

Yes I did this version of the parseDistinguishedName function, but I am absolutely certain that I have tested with an AD with users who have comma's in their name like "Doe, John". I am surprised the comma is already escaped.

[Frank683]

I am surprised the comma is already escaped.

Could be that the escaping happens in some routine which is called before the result arrives in function getGroupMembershipForDN but as you can see from my screenshot of our AD it does display the backslash escaping the comma in the DN.

[jurjendijkstra]

Sometimes I wonder, how are users created in AD? Sometimes in the AD admin GUI. Sometimes they are synchronized from AzureAD. Sometimes they are synchronized from some Human Resources application. I can not imagine that a human operator would take the effort to enter "Doe\, John" instead of "Doe, John". I don't know where the slash came from, could be one of those sync sources. Anyway I would like to do some testing and see what happens, I have a little AD server in which I have all sorts of weird user names. What is your case? I think you try to get the list of groups for a particular user? (myself I only query the other way around: query the members of a particular group)

[jurjendijkstra]

haha I typed Doe slash comma space John but github removed the slash from the comment

[Frank683]

I can not imagine that a human operator would take the effort to enter "Doe, John" instead of "Doe, John". I don't know where the slash came from, could be one of those sync sources.

They don't. The backslash is not present in the "normal" user's view of the object:

It's only visible in the DN displayed in the attribute editor of the windows AD tool:

CN:

DN:

What is your case? I think you try to get the list of groups for a particular user?

Yes, the user loggs in and we retrieve all his group memberships.

[jurjendijkstra]

I have tested with my domain controller: In the "Active Directory Users and Computers" GUI I have added a user named "Doe, John (Test)" and made him member of group "erpusers". That group is member of group "accounting". The userPrincipalName for that new user is "jdoe@test.vanmeijel.org".

function getGroupMembershipForUser("jdoe@test.vanmeijel.org", ..... correctly returns the group memberships: "erpusers" and "accounting". That's correct.

But when I call function findUser("jdoe@test.vanmeijel.org", .... it returns this:

{
  "dn": "CN=Doe\\, John (Test),CN=Users,DC=test,DC=vanmeijel,DC=org",
  "distinguishedName": "CN=Doe\\, John (Test),CN=Users,DC=test,DC=vanmeijel,DC=org",
  "userPrincipalName": "jdoe@test.vanmeijel.org",
  "sAMAccountName": "jdoe",
  "whenCreated": "20200404094050.0Z",
  "pwdLastSet": "132304668504714966",
  "userAccountControl": "512",
  "cn": "Doe, John (Test)",
  "displayName": "Doe, John (Test)"
}

I hope Github renders it correctly, it is: "Doe backslash backslash comma space John". Note this is the output of JSON.stringify(user,null,2) so one of the backslashes is added by JSON.stringify

When I take user.dn from the findUser result and use it as the first parameter in another pair of getGroupMembershipForUser and findUser calls, then the output is just identical.

So I have not been able to replicate the problem. Nothe I have also experimented a bit with the two parseDistinguishedName functions in utilities.js and parseRangeAttributes.js: I have swapped them, made them identical but none of that makes a difference in this testcase. I have also tested without the component[i - 1] += '\' line you have pointed me to, but that only made things worse.

So what else can I do?

[jurjendijkstra]

I notice now that wrote you are using activedirectory2@2.0.0-pre.1. I have now tested that and confirm that 2.0.0-pre.1 does indeed throw error 'illegal unescaped char'. Not just in nodejs 12, I have tested with nodejs 8 too.

In my previous comment I was using activedirecory2 release 1.3. That works fine for me. So it has to be a regression between 1.3 and 2.0.0-pre.1

[Frank683]

@jurjendijkstra 1.3 uses ^1.0.1 of ldapjs which itself uses 0.2.2 of ldap-filter. The escapeValue function which throws the error was introduced later with this commit:

https://github.com/pfmooney/node-ldap-filter/commit/adb5c5c695016ec2e5f0c6b28693269fcc0929c0

Anyway I will try to fix the escaping in activedirectory2 and provide a unit test too.

[devpascoe]

Is this fix only for finding users? Because i am experiencing the same issue with getUsersForGroup As a workaround i can replace my group name param ( and ) chars with '\\28' and '\\29' in my own code before sending as param to getUsersForGroup group name which seems to be working.

Looks like within lib/components/getUsersForGroup.js the line (currently 148) says ad.findGroup(groupAttributes, this.groupName, (err, group) => { I'm thinking it might be fine to change to ad.findGroup(groupAttributes, utils.parseDistinguishedName(this.groupName), (err, group) => {

i made a cheeky function anyway to parse the group name input param. function parseSafe(str) { // reference ActiveDirectory lib/components/utilities parseDistinguishedName return str .replace(/*/g, "\\2A") .replace(/(/g, "\\28") .replace(/)/g, "\\29") .replace(/+/g, "\+") .replace(/\</g, "\<") .replace(/>/g, "\>") .replace(/\;/g, "\;") .replace(/\"/g, '\"') .replace(/\=/g, "\="); }