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

Fix issue #8 #9

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix issue #8 #9

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions lib/string-parsing/escape-substring.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
'use strict'

const escapeFilterValue = require('../utils/escape-filter-value')

/**
* In an extensible filter, the righthand size of the filter can have
* substrings delimeted by `*` characters, e.g. `foo=*foo*bar*baz*`. This
Expand All @@ -25,9 +23,9 @@ module.exports = function escapeSubstring (str) {
throw Error('extensible filter delimiter missing')
}

out.initial = escapeFilterValue(fields.shift())
out.final = escapeFilterValue(fields.pop())
Array.prototype.push.apply(out.any, fields.map(escapeFilterValue))
out.initial = fields.shift()
out.final = fields.pop()
Array.prototype.push.apply(out.any, fields)

return out
}
12 changes: 9 additions & 3 deletions lib/string-parsing/escape-substring.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: [] }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect. We should see f\\28o here. The issue is really that:

  1. We need to represent filter strings, i.e. the text humans read, with escape sequences as shown in the original test.
  2. We need to send the byte representation of those escape sequences to the LDAP server, e.g. \28 should be sent as the byte 0x28.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@PositroniumJS PositroniumJS Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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*)')

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsumners happy new year!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const result = escapeSubstring('f(o*')
t.strictSame(expected, result)
})

tap.test('escapes string with initial and final', async t => {
const expected = { initial: 'f\\28o', final: 'bar', any: [] }
const expected = { initial: 'f(o', final: 'bar', any: [] }
const result = escapeSubstring('f(o*bar')
t.strictSame(expected, result)
})

tap.test('escapes string with initial, final, and any', async t => {
const expected = { initial: 'f\\28o', final: 'b\\29f', any: ['bar', 'baz'] }
const expected = { initial: 'f(o', final: 'b)f', any: ['bar', 'baz'] }
const result = escapeSubstring('f(o*bar*baz*b)f')
t.strictSame(expected, result)
})

tap.test('escapes string with any only and containing a non ascii character', async t => {
const expected = { initial: '', final: '', any: ['réseau'] }
const result = escapeSubstring('*réseau*')
t.strictSame(expected, result)
})
9 changes: 9 additions & 0 deletions lib/string-parsing/parse-expression.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,5 +102,14 @@ tap.test('parses filter with non-ascii characters', t => {
t.equal(result.toString(), '(cn=\\c3\\b8)')
})

t.test('*réseau*', async t => {
const result = parse('cn=*réseau*')
t.type(result, SubstringFilter)
t.equal(result.toString(), '(cn=*r\\c3\\a9seau*)')
t.equal(result.initial, '')
t.equal(result.final, '')
t.strictSame(result.any, ['réseau'])
})

t.end()
})