ldap_escape does not encode leading/trailing spaces. #14
Conversation
| @@ -80,7 +80,7 @@ public function testLdapEscape($subject, $ignore, $flags, $expected) | |||
| public function provideLdapEscapeValues() | |||
| { | |||
| return array( | |||
| array('foo=bar(baz)*', null, p::LDAP_ESCAPE_DN, 'foo\3dbar(baz)*'), | |||
There was a problem hiding this comment.
Why do you remove this test? It is in no way related to this PR.
There was a problem hiding this comment.
I added to it to include the leading/trailing spaces and a carriage return, as that was the test that seemed to be dealing strictly with escaping a DN. However, if you want, I can leave that test alone and include a completely separate line for testing a DN with leading/trailing spaces and a carriage return.
There was a problem hiding this comment.
Actually, the escape tests were backported from the tests from the official PHP interpreter.
See https://github.com/php/php-src/tree/master/ext/ldap/tests
For maintenance's sake, you should create a new test case line.
Cheers! :)
|
The test has been separated along with a little explanation. Let me know if there's anything else to do. |
|
LGTM 👍 |
|
Thank you @ChadSikorra. |
This PR was squashed before being merged into the 2.8 branch (closes #16842). Discussion ---------- [Ldap] Escape carriage returns in LDAP DNs. Depends upon this commit in polyfill: symfony/polyfill#14 | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Carriage returns are supposed to be escaped in a LDAP DN. Leading and trailing spaces should be encoded as well. The spaces were taken care of in the polyfill implementation of `ldap_escape`, but the actual PHP function doesn't do the same. So I moved that logic within the component function and removed it from the polyfill function. Commits ------- 2243db4 [Ldap] Escape carriage returns in LDAP DNs.
This PR was merged into the 1.1-dev branch. Discussion ---------- Add CHANGELOG Fixes #32 * v1.1.1 * bug #40 [Apcu] Load APCUIterator only when APCIterator exists (nicolas-grekas) * bug #37 [Iconv] Fix wrong use in bootstrap.php (tucksaun) * bug #31 Fix class_uses polyfill (WouterJ) * v1.1.0 * feature #22 [APCu] A new polyfill for the legacy APC users (nicolas-grekas) * bug #28 [Php70] Workaround https://bugs.php.net/63206 (nicolas-grekas) * v1.0.1 * bug #14 ldap_escape does not encode leading/trailing spaces. (ChadSikorra) * bug #17 Fix #16 - gzopen() / gzopen64() - 32 bit builds of Ubuntu 14.04 (fisharebest) * v1.0.0 * Hello symfony/polyfill Commits ------- 1781007 Add CHANGELOG
The
ldap_escapefunction in this library is actually more RFC compliant at the moment than the official function. This removes the encoding for a leading/trailing space for escaping a DN. Also, a carriage return should be encoded when escaping a DN but the official function doesn't do that either (neither does the one here, but I put it in the test anyhow). I guess a bug report should probably be opened for the PHP function on their bug tracker.The escape function in the LDAP component should probably be updated to account for the additional encoding mentioned above, but it doesn't appear to have any tests at all at the moment?