Clarify what happens when meta_value is `''` in get_users like functions.

[growthwp]

Because I didn't realize that doing this query:

        $all_user_ids = get_users([
            'fields' => 'id',
            'meta_key' => 'secret_access_key',
            'meta_compare' => '=',
            'meta_value' => $dynamic_value_from_frontend
        ]);

Given the credibility, history & robustness of get_users, which entered under the hood of "that thing just works, don't doubt it", if $dynamic_value_from_frontned were to be just a mere'', instead of giving me an empty list of no users, the query would return a list of all users. What happens when you rely on this list for mission-critical things? The worst.

In my case, this list was very crucial for allowing access to customer websites, based on a secret string only the developers, us, know, we are able to access these sites and inherit the identity of our customer (mostly admin accounts). The way we achieve this is we attach that secret phrase to their user_meta and as you can see, we attempt to hit an endpoint that looks at all the users and returns us with the one that matches the string we provided.

Unfortunately, as I've said before, because our assumption, as well as our partners' and personal friends, some of whom are veterans in the industry, was that get_users would forfeit completely if not given a key at all, instead, it just simply decides to ignore the field completely and retrieve based on the key alone.

The more interesting fact is that if we set the meta_value to False, it actually does the check and doesn't return any users.

Here's a PoC: ​https://pastebin.com/vfaVuRp0

Please make it clear that a meta_value that equals to '' is not the same as it being False and that the system defaults to just using the key, skipping all checks. The desired behavior should be:

1. When the meta_value is missing altogether, it means we're searching just for key.
2. When the meta_value is '', it should be treated as a value and searched against.
3. When the meta_value is False, it should be treated as a value and searched against.

I do recognize that it's probably impossible to go back and change this and so, for me at least, it's even more reason to push this very important note to the front.

Please mention this in either the comments for get_users, in the codebase itself or make a phpcs rule for this. In other words, please specify that you must ensure the input for the meta_value is properly checked for before actually being used due to this exact case.

Thank you.

---

In our case, it would've been a disaster if we didn't fuzz around. While I completely understand it's us that should make sure whatever is passed is proper, I believe the community is not aware of this edge case not due to bad coding practices but simply due to the need to implement dynamic queries like this being very rare and as such, overlooked.

We've also notified 4 vendors, one of which is perhaps the biggest in terms of reach about a potential issue with dynamic query building like we had. In the case of a membership plugin that seemed to have all security checks in place, as we did, it resulted in certain users being given access to another user's property.

[growthwp]

PoC

[danomurray]

Just noting that we could have been caught out by this if we hadn't found this ticket, and have been caught out by a similar issue with get_users() when passing an empty array of include IDs

$users = get_users(array('include' => $user_ids,));

If we hadn't caught this in testing it would have caused a nasty bug where all users were emailed instead of a very restricted set.

Would be great to see these issues documented or addressed.