[wp-trac] [WordPress Trac] #49641: Clarify what happens when meta_value is `''` in get_users like functions.

[WordPress Trac]

#49641: Clarify what happens when meta_value is `''` in get_users like functions.
-------------------------+-------------------------------------
 Reporter:  growthwp     |       Owner:  (none)
     Type:  enhancement  |      Status:  new
 Priority:  normal       |   Milestone:  Awaiting Review
Component:  Users        |     Version:
 Severity:  normal       |  Resolution:
 Keywords:  needs-docs   |     Focuses:  docs, coding-standards
-------------------------+-------------------------------------
Changes (by SergeyBiryukov):

 * keywords:  needs-dev-note => needs-docs
 * component:  Query => Users


Old description:

> 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.

New description:

 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.

--

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/49641#comment:1>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform