Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ldap_search() fails when $attributes array has holes #17280

Closed
dregad opened this issue Dec 27, 2024 · 5 comments · Fixed by #17284
Closed

ldap_search() fails when $attributes array has holes #17280

dregad opened this issue Dec 27, 2024 · 5 comments · Fixed by #17284

Comments

@dregad
Copy link

dregad commented Dec 27, 2024

Description

The following code:

<?php
$ldap_server = 'ldap://localhost';
$root_dn = 'ou=people,dc=example,dc=com';

$ldap = ldap_connect( $ldap_server );
ldap_set_option($ldap, LDAP_OPT_PROTOCOL_VERSION, 3);
ldap_bind($ldap); // Anonymous bind

// Creating an array with a hole in it
$attr = array_unique(['cn', 'uid', 'uid', 'mail']);
$res = ldap_search($ldap, $root_dn, 'uid=admin', $attr);
echo ldap_error($ldap);

Resulted in this output:

Warning: ldap_search(): Array initialization wrong in ./test.php on line 11

Call Stack:
    0.0012     401536   1. {main}() ./test.php:0
    0.1191     401816   2. ldap_search($ldap = class LDAP\Connection {  }, $base = 'ou=people,dc=example,dc=com', $filter = 'uid=admin', $attributes = [0 => 'cn', 1 => 'uid', 3 => 'mail']) ./test.php:11

Success  

But I expected this output instead:

Success

If $attr has no holes in the keys (i.e. remove array_unique() or call array_values()) then it works fine.

Maybe this is normal, but in that case I believe the documentation should clarify that, as it currently does not say anything about $attributes parameter needing to be a list with keys in a strict sequence.

PHP Version

8.2.27, 8.4.2

Operating System

MacOS 14.6.1

@nielsdos
Copy link
Member

Hm right, the general handling of this function was improved on the master branch, but it will still be an issue because zend_array_is_list will return false:

php-src/ext/ldap/ldap.c

Lines 1484 to 1487 in 55afe8b

if (!zend_array_is_list(attributes)) {
zend_argument_value_error(4, "must be a list");
RETURN_THROWS();
}

I believe this can be improved.

@Girgias
Copy link
Member

Girgias commented Dec 27, 2024

Right, I think this can be relaxed and backported.

@dregad
Copy link
Author

dregad commented Dec 27, 2024

Awesome, thanks !

nielsdos added a commit to nielsdos/php-src that referenced this issue Dec 27, 2024
We relax the constraint that the array must be a list. What really
matters is that it only has numeric keys. As shown in the example code,
it's really easy to accidentally create a non-list array, so it makes
sense to relax the constraint.
There are 3 cases left where the array is checked to be a list,
in php_ldap_do_search, but I believe this makes sense to keep because
the indices of those arrays have a meaning because they should match
between different arrays. In that case it will prevent programmer
errors.
@nielsdos
Copy link
Member

I've filed a PR against master, and once that's merged I'll try to backport it.

@nielsdos
Copy link
Member

nielsdos commented Dec 27, 2024

Merged into master, I'll be making a separate PR for the backport.
EDIT: backport PR is here: #17287

nielsdos added a commit to nielsdos/php-src that referenced this issue Dec 27, 2024
nielsdos added a commit to nielsdos/php-src that referenced this issue Dec 27, 2024
nielsdos added a commit that referenced this issue Dec 29, 2024
nielsdos added a commit that referenced this issue Dec 29, 2024
* PHP-8.3:
  Backport fix GH-17280: ldap_search() fails when $attributes array has holes
nielsdos added a commit that referenced this issue Dec 29, 2024
* PHP-8.4:
  Backport fix GH-17280: ldap_search() fails when $attributes array has holes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants