WordPress.org

Make WordPress Core

Opened 4 months ago

Last modified 4 months ago

#53386 new defect (bug)

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

Reported by: chrisvanpatten Owned by:
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 (4)

#1 @TimothyBlynJacobs
4 months 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
4 months 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.


4 months ago

  • 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
4 months 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
Note: See TracTickets for help on using tickets.