Multisite is_super_admin call during app password validation can lead to infinite loop

[chrisvanpatten]

In multisite, the following code sample leads to an infinite loop when validating an application password:

<?php

\add_filter(
        'wp_is_application_passwords_available_for_user',
        fn( bool $available, \WP_User $user ): bool => user_can( $user, 'app_password_cap' ),
        10,
        2,
);

This is because, in multisite, the user_can call leads to an is_super_admin call, which in turn leads to a wp_get_current_user call which ultimately triggers wp_is_application_passwords_available_for_user… starting the whole process over again.

(Worth noting that a very similar example to the above is included in the ​Application Passwords Integration Guide, so this use-case should be a supported one. For anyone who encounters this, a workaround is to remove your hook before you call user_can, and add it back after.)

[TimothyBlynJacobs]

To fix this, I think we need to make is_super_admin accept a WP_User object, that when passed will use that instead of trying to query for the current user. Then, WP_User::has_cap can pass in it's instance instead of the id.

Semi related: #28020.

Cc @peterwilsoncc.

[peterwilsoncc]

Changing the start if is_super_admin() to the following appears to remove the infinite loop:

<?php
function is_super_admin( $user_id = false ) {
        if ( ! $user_id ) {
                $user = wp_get_current_user();
        } else {
                $user = get_userdata( $user_id );
        }

        // etc 
}

In #28020 get_userdata( /* current user ID */ ) started returning the same user object as wp_get_current_user() so there is no need for is_super_admin() to do the same check.

I think it would be good to change is_super_admin() to accept a WP_User object non-the-less but given it's possible to fix this without changing the signature of the function I'd rather take the above approach on this ticket.

I'll put together a pull request once I've done a little more manual testing.

https://core.trac.wordpress.org/ticket/53386

Prevent infinite loop on MS if wp_is_application_passwords_available_for_user includes a user cap check.

Bypass wp_get_current_user() in is_super_admin() when possible.

In #28020 get_userdata() and wp_get_current_user() started returning the same object so there is no need to duplicate the check used in the is_super_admin() function.

---

I need some assistance creating the infinite loop in the tests. I couldn't get one going prior to the patch being applied.

[peterwilsoncc]

In the ​linked pull request:

• always use get_userdata() within is_super_admin() following #28020
• it still needs unit tests, I couldn't get them to loop in multisite prior to the patch being applied using the filter above
• manual tests suggest the patch works but I'd rather not risk a merge without tests

[TimothyBlynJacobs]

I agree the simpler approach is best. I've added a test that fails without the fix, and passes with the fix. @chrisvanpatten Can you confirm the fix works for you?

[TimothyBlynJacobs]

In 52157:

Users: Prevent infinite loop when using capability checks during determine_current_user on multisite.

On multisite, when checking if a user has a certain capability WordPress makes an additional check to see if the user is a super admin. The is_super_admin() function contained a call to wp_get_current_user() so as the global current user object could be used if it matched the queried user id.

This would cause an infinite loop if a hook attached to the determine_current_user filter was itself making a permission check. For example when limiting who can use the Application Passwords feature based on their capabilities.

Since [50790] the WP_User instance for the current user is shared between wp_get_current_user() and get_userdata(). This means we can remove the wp_get_current_user call from is_super_admin() while still retaining the same behavior.

Props chrisvanpatten, peterwilsoncc.
Fixes #53386.

https://core.trac.wordpress.org/changeset/52157