Fix issue #8 #9
Fix issue #8 #9
Conversation
|
Please be patient. I need time to review and test. |
Sorry, no problem. Just wanted to know that you have well seen it |
| @@ -11,19 +11,25 @@ tap.test('throws if separator missing', async t => { | |||
| }) | |||
|
|
|||
| tap.test('escapes an initial only string', async t => { | |||
| const expected = { initial: 'f\\28o', final: '', any: [] } | |||
| const expected = { initial: 'f(o', final: '', any: [] } | |||
There was a problem hiding this comment.
This is incorrect. We should see f\\28o here. The issue is really that:
- We need to represent filter strings, i.e. the text humans read, with escape sequences as shown in the original test.
- We need to send the byte representation of those escape sequences to the LDAP server, e.g.
\28should be sent as the byte0x28.
I need to think on how to resolve this. I made a mistake in my interpretation of the specs here.
It may be that we need a new function that will replace escape sequences in values when filters are being serialized to BER format. The escapeFilterValue function is doing what it should be doing: escaping the values for human interpretation as outlined in RFC 4515.
There was a problem hiding this comment.
If I understand well, the problem is that values are escaped twice: here and when converted in BER format. Since javascript can handle utf8 isn't better to just escape once when converting in BER (as it was in ldapjs@2 I guess)?
There was a problem hiding this comment.
@jsumners so what do we do? IMHO, the text needs to be escaped only when sent to the LDAP server, not before, what it is equivalent to this modification (since Javascript have no problem to deal with utf8 symbols). Otherwise, it would be to de-escape the characters before escaping them again
There was a problem hiding this comment.
I have not had time to sit down and investigate the issue in order to give it careful consideration and devise a plan. What I can say right now is that the intention is to represent to the reader of the string what will be sent to the server. In other words if dn.ToString() outputs f(o, but what will be written to the BER block is the set of bytes f0x28o, then that is incorrect; f\28o === f0x28o.
There was a problem hiding this comment.
As we see in lib/string-parsing/parse-expression.test.js, I have added the following test that matches what you are saying:
const result = parse('cn=*réseau*')
t.type(result, SubstringFilter)
t.equal(result.toString(), '(cn=*r\\c3\\a9seau*)')There was a problem hiding this comment.
I think I have a thought of a solution, but it will take time for me to get to it. Essentially, wherever there is the possibility of a human readable string, e.g. a filter string or a DN string, it may be better to have .toString() present that human variant and either .toString(ber = true) or .toBerString() to be used internally when writing the string for an LDAP message. This will not be a short chore.
There was a problem hiding this comment.
I am sorry but I don't really get why it is problematic to do this change, for me it is the equivalent of #7 like we see here, the value is human readable (no escaped characters) and when we call toString we get the escaped string 🤔. Having non escaped characters make it far more easier to use in my opinion, and it is working as it was before the ldapjs@3 update
There was a problem hiding this comment.
Thank you. No, I have not forgotten about this. I have a lot of projects running concurrently. I do not think the solution in #7 was correct, and I need to take time to re-review all of this stuff before I can settle on what is and isn't correct. I suspect that I will be updating the ber.writeString method to handle encoding correctly, but I can't say for sure.
No description provided.