WordPress.org

Make WordPress Core

Opened 12 months ago

Last modified 6 months ago

#44591 new defect (bug)

PHP notice if optional argument isn't passed to map_meta_cap()

Reported by: henry.wright Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: General Keywords: reporter-feedback 2nd-opinion
Focuses: Cc:

Description

map_meta_cap() can take an optional 3rd argument. func_get_args() is used in the function definition to get that argument. The argument is used later on in the function definition. For example $args[0]

If a third argument isn't passed to map_meta_cap(), we will get an undefined offset notice in our debug log pointing to $args[0].

Change History (3)

#1 @johnbillion
12 months ago

  • Keywords reporter-feedback added

Thanks for the report Henry. Can you provide some example code demonstrating the bug please?

#2 @henry.wright
12 months ago

Hi @johnbillion

The entry I'm getting in debug.log is PHP Notice: Undefined offset: 0 in wp-includes/capabilities.php on line 136. I'm still trying to track down the code that is generating it.

#3 @mattheweppelsheimer
6 months ago

  • Keywords 2nd-opinion added

map_meta_cap() doesn't consistently check whether $args keys are set before attempting to access them. Early cases in its switch statement tend to follow this pattern with isset() guards, as in the third line from this snippet:

case 'remove_user':
// In multisite the user must be a super admin to remove themselves.
if ( isset( $args[0] ) && $user_id == $args[0] && ! is_super_admin( $user_id ) ) {
        $caps[] = 'do_not_allow';
} else {
        $caps[] = 'remove_users';
}
break;

Later though, there are several cases where it accesses $args[0] without an isset() first, as in this case which is involved in the Notice @henry.wright reported (line 136 is the second line in this snippet):

case 'edit_page':
        $post = get_post( $args[0] );
        if ( ! $post ) {
                $caps[] = 'do_not_allow';
                break;
        }
// continuation of `case` omitted...

It is possible that core code never executes any of the map_meta_cap() cases that assume $args are set, without setting them. (I think whether or not it does can probably be determined one way or the other with certainty, but I haven't looked into that.) If that is so, then Undefined offset Notices would only come from plugins/themes, and one might argue that this is not Core's problem to solve.

I would argue, however, that this is an area where Core could be made friendlier to plugin/theme developers by going ahead and adding those guards.

One might also argue against adding these guards because they might obscure underlying issues. So I would propose to use _doing_it_wrong() and WP_Error where appropriate.

I'm happy to work on a patch for this, but I'd like to get a Core Committer's thoughts beforehand, in case my analysis is flawed.

Is there any good reason not to revise map_meta_cap() so that it always checks if an $args index is set before accessing it?

Note: See TracTickets for help on using tickets.