Skip to content
This repository has been archived by the owner on May 14, 2024. It is now read-only.

illegal unescaped char #621

Closed
antonio-castellon opened this issue Jun 2, 2020 · 13 comments
Closed

illegal unescaped char #621

antonio-castellon opened this issue Jun 2, 2020 · 13 comments

Comments

@antonio-castellon
Copy link

Hi,

I think that we have a problem with the password, that contains characters like:
&%$!() etc.

/opt/pct/node_modules/ldap-filter/lib/index.js:49
      throw new Error('illegal unescaped char: ' + c);
      ^
Error: illegal unescaped char: (
    at escapeValue (/opt/pct/node_modules/ldap-filter/lib/index.js:49:13)
    at parseExpr (/opt/pct/node_modules/ldap-filter/lib/index.js:167:16)
    at parseFilter (/opt/pct/node_modules/ldap-filter/lib/index.js:228:14)
    at Object.parse (/opt/pct/node_modules/ldap-filter/lib/index.js:252:18)
    at Object.parseString (/opt/pct/node_modules/ldapjs/lib/filters/index.js:180:27)
    at Client.search (/opt/pct/node_modules/ldapjs/lib/client/client.js:567:30)
    at ActiveDirectory.search (/opt/pct/node_modules/activedirectory/lib/activedirectory.js:529:10)
    at ActiveDirectory.getGroupMembershipForDN (/opt/pct/node_modules/activedirectory/lib/activedirectory.js:740:10)
    at /opt/pct/node_modules/activedirectory/lib/activedirectory.js:1113:29
    at /opt/pct/node_modules/activedirectory/lib/activedirectory.js:885:5

I just downgraded to version 1.0.2 to fix it, but it could be great if the next release has as well som parser to deal with this typical characters used in strong passwords.

Thanks in advance
Antonio

@jsumners
Copy link
Member

jsumners commented Jun 2, 2020

Would you like to submit a PR?

@UziTech
Copy link
Member

UziTech commented Jun 2, 2020

this is because of updates to ldap-filter in #521

I believe you will have to escape characters in the set ()\*

@UziTech
Copy link
Member

UziTech commented Jun 2, 2020

@delfuego
Copy link

I too am having this issue; in my case, it's in a check of an LDAP group name that has an open-parentheses in it.

@UziTech
Copy link
Member

UziTech commented Jul 24, 2020

@delfuego you should be able to escape the name.

something like:

filter = `(samaccountname=${name.replace(/[()\\*]/g, "\\$&")})`

@delfuego
Copy link

@UziTech For me, the issue turns out to be that this is being triggered by code I don't control in any way. I use the node-activedirectory library to provide AD authentication and authorization services in a custom Passport provider, and it's in that library's check whether a user is in a specific AD group that the issue arises — that code iterates through all the AD groups that the user is in to see if they have any nested groups, and it's in this recursive iteration that the escape doesn't happen.

If this specific situation hits anyone else, I forked the node-activedirectory library, added the proper escaping, and submitted a pull request... I don't know how likely it is that it'll get pulled into the main library, though, the author of that library hasn't really been active in nearly a half-decade. But my fork won't go anywhere, if people want to use it as a dependency in their projects!

@jsumners
Copy link
Member

@delfuego You can see if your issue is resolved by https://www.npmjs.com/package/activedirectory2 (@next version) , but I can assure you that the original library is pretty much unmaintained.

Regardless of the path you choose, the spec is quite clear that these characters should be escaped within values.

@delfuego
Copy link

@jsumners Well HOLY CRAP, how did I not know that activedirectory2 exists?!? This certainly looks promising:

https://github.com/jsumners/node-activedirectory/blob/925c35194c04a5583cb0b46653bff88e518de37c/lib/components/utilities.js#L280

I'll give it a try.

@delfuego
Copy link

@jsumners Works like a charm — and I see that you merged the @next branch into master, which is what I tested!

@jongrubb
Copy link

jongrubb commented Aug 18, 2020

I ran into this error too using activedirectory package. This package has a direct dependency for ldapjs with versioning like"ldapjs": ">= 0.7.1". Which is essentially set to pull the latest version of ldapjs. So, if you do a fresh install of activedirectory, you might encounter this error. This problem is solely due to how activedirectory is using their dependency versioning. So this error was introduced into activedirectory package when ldapjs updated it's breaking changes from 1.0.2 to 2.0.0. So this isn't ldapjs's fault, it's the fault with the dependency versioning of ldapjs used in activedirectory.

My company is using activedirectory right now. This is not an ideal fix, but a quick one if you are trying to get things deployed. Ideal fix would be switching to a more reliable library with correct dependencies.

If you are still using activedirectory package as a dependency for your project and encountered this error, a quick fix is to force a specific version for ldapjs on activedirectory. We are using yarn. You can specifically tell yarn what version of a transient dependency to use for a specific package. Below is how I fixed this issue for activedirectory package for the time being by adding this to your package.json.

  "resolutions": {
    "activedirectory/ldapjs": "1.0.2"
  },

@adrianzielonka
Copy link

@jongrubb thank you! My company is also using this package and this fixed it!

@Gavin152
Copy link

Gavin152 commented Jan 30, 2023

@delfuego Finding this years later. But AD actually has a filter built-in for traversing nested groups. It's not glaringly obvious, though, since it's only available via the filter ID: '1.2.840.113556.1.4.1941'. More info can be found here: LDAP Wiki only reachable via the way-back-machine, unfortunately.

@jsumners
Copy link
Member

⚠️ This issue has been locked due to age. If you have encountered a recent
problem that seems to be covered by this issue, please open a new issue.

Please include a minimal reproducible example
when opening a new issue.

@ldapjs ldapjs locked as resolved and limited conversation to collaborators Mar 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants