Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#53386 closed defect (bug) (fixed)

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

Reported by: chrisvanpatten's profile chrisvanpatten Owned by: timothyblynjacobs's profile TimothyBlynJacobs
Milestone: 5.9 Priority: normal
Severity: normal Version: 5.6
Component: Users Keywords: needs-unit-tests has-patch
Focuses: rest-api Cc:

Description

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

Change History (7)

#1 @TimothyBlynJacobs
3 years ago

  • Component changed from Application Passwords to Users
  • Focuses rest-api added
  • Milestone changed from Awaiting Review to 5.9

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.

#2 @peterwilsoncc
3 years ago

  • Keywords needs-patch needs-unit-tests added
  • Version set to 5.6

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.

This ticket was mentioned in PR #1391 on WordPress/wordpress-develop by peterwilsoncc.


3 years ago
#3

  • Keywords has-patch added; needs-patch removed

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.

#4 @peterwilsoncc
3 years ago

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

#5 @TimothyBlynJacobs
2 years ago

  • Owner set to TimothyBlynJacobs
  • Status changed from new to assigned

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?

#6 @TimothyBlynJacobs
2 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

Note: See TracTickets for help on using tickets.