added filter parsing that will convert to proper hex values

[AlaricWhitney]

This PR addresses the issues provided in #337

This will take an input filter string, and parse it for values, and convert non-alphanumeric values to hex values before passing it to the compileFilter method. This also adds a public helper function of ParseFilter to help users of this library in the future.

[vetinari]

I suggest instead to use something like

  filter := fmt.Sprintf("(foo=%s)", ldap.EscapeFilter("my(value)+with Special(c\\hars)")

in your code.

[AlaricWhitney]

I just tested out your suggestion, and it doesn't handle strings values that already have \xx hex values (example, CN=Main\5c will output as CN=Main\5c5c).

[AlaricWhitney]

Also, it looks like the checks for go 1.7 - 1.10 is failing due to ../../../go/src/github.com/go-ldap/ldap/debug.go:28: undefined: log.Writer, which isn't touched in this PR. Seeing as this wasn't changed in this PR, should these golang version be checked?

[vetinari]

I just tested out your suggestion, and it doesn't handle strings values that already have \xx hex values (example, CN=Main\5c will output as CN=Main\5c5c).

If it's already escaped, why would you run EscapeFilter then? If your patch can do this, it's questionable, because someone might have an unescaped CN=Main\5c in the value ...

[AlaricWhitney]

I am accounting for it due to the fact that testing on the CompileFilter seems to require it.

[AlaricWhitney]

I am also accounting for it so as to not break compatibility for anyone that already went through the trouble of escaping their filter queries already.

[vetinari]

how do you distinguish between an already escaped string and something like (homeDirectory=\\some-server\cdoe)?

[AlaricWhitney]

how do you distinguish between an already escaped string and something like (homeDirectory=\\some-server\cdoe)?

Good question. As of right now, my patch does not account for this. It only searches for \xx

That being said, I noticed that there are no test cases to account for this. I just tested this without the patch, and it looks like that go-ldap as an app in it's existing state does not account for this:

CompileFilter(`(homeDirectory=\\some-server\cdoe)`) results in 'LDAP Result Code 201 "Filter Compile Error": ldap: invalid characters for escape in filter: encoding/hex: invalid byte: U+005C '\'' (expected error to contain '')

I also checked the existing state of EscapeFilter(`\\some-server\cdoe`) results in \5c\5csome-server\5ccdoe, but doing this would require that there's a no hex characters, as demonstrated before.

Do you have any ideas on how to address this? I could possibly search the string for \\, and ignore the \xx check for the value. I could also ignore the \xx if the hex fails and just let the \ be converted to hex.

Another possibility would be to break compatibility, and not allow \xx hex characters into CompileFilter, and allow it to freely use EscapeFilter without worry, or some variation of it (maybe add a new CompileUnescapedFilter function that would do this?)

[vetinari]

That being said, I noticed that there are no test cases to account for this. I just tested this without the patch, and it looks like that go-ldap as an app in it's existing state does not account for this:

CompileFilter(`(homeDirectory=\\some-server\cdoe)`) results in 'LDAP Result Code 201 "Filter Compile Error": ldap: invalid characters for escape in filter: encoding/hex: invalid byte: U+005C '\'' (expected error to contain '')

I also checked the existing state of EscapeFilter(`\\some-server\cdoe`) results in \5c\5csome-server\5ccdoe, but doing this would require that there's a no hex characters, as demonstrated before.

Right, the only way to solve this unambiguously is to know your data, i.e. if it's already escaped or not. If not, always use EscapeFilter and construct the search string. Having a mixed input cannot be resolved without knowing which parts are already escaped or not.

What you could do is to add a different EscapeFilter which accepts strings like ldapsearch from the openldap distribution does it. This allows you to insert \ before a character that needs to be escaped to make it clear that a character is a literal and not a special, e.g. (cn=*\(\*\)*) would successfully match all cn with (*) inside. All other characters are then escaped when needed. In this one you'd have to use (homeDirectory=\\\\some-server\\cdoe) (IIRC).

Another possibility would be to break compatibility, and not allow \xx hex characters into CompileFilter, and allow it to freely use EscapeFilter without worry, or some variation of it (maybe add a new CompileUnescapedFilter function that would do this?)

I would't break it, adding a function which gives some more convenience which returns a string that the current CompileFilter accepts would do it IMO.

[AlaricWhitney]

I've now pushed up an updated that adds a CompileEscapeFilter function that will read the values of the filter, and run each value through EscapeFilter before passing the output to CompileFilter. This method seems to work, however I did point out in the code that you cannot use wildcard * searches using this method, as EscapeFilter does treat it as a literal * (\2a)

If you want to use a wildcard search, you'll have to use CompileFilter and do your own escapes.

[vetinari]

If you want to use a wildcard search, you'll have to use CompileFilter and do your own escapes.
That's why I suggested a slightly different approach: escape everything - well not numeric and a-z I guess - unless it's a special character (i.e. ()*&|, anything else?) ... but: if that special char (or anything else) is preceded by a \ drop the \ and escape it.

If you really want a special CompileFilter instead of a helper that feeds into it, than I'll hand the discussion over to those who could merge (err... seems I still have that permission, but anyway ;-) ).