WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 2 weeks ago

#44591 accepted defect (bug)

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

Reported by: henry.wright Owned by: SergeyBiryukov
Milestone: 5.7 Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch needs-refresh
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].

Attachments (1)

44591.diff (4.6 KB) - added by jeherve 3 months ago.
_doing_it_wrong notice when forgetting to pass an arg for a capability that requires one

Download all attachments as: .zip

Change History (17)

#1 @johnbillion
2 years ago

  • Keywords reporter-feedback added

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

#2 @henry.wright
2 years 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 follow-up: @mattheweppelsheimer
2 years 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?

#4 @SergeyBiryukov
3 months ago

#48415 was marked as a duplicate.

#5 @SergeyBiryukov
3 months ago

  • Component changed from General to Users

#6 in reply to: ↑ 3 @SergeyBiryukov
3 months ago

  • Keywords needs-patch added; reporter-feedback 2nd-opinion removed
  • Milestone changed from Awaiting Review to 5.6

Previously: #13905, #23377, #30821, #48415.

[34113] / #13905 added some sanity checks, but they're not preventing the notice.

As noted in comment:2:ticket:48415:

If you're checking whether the current user can publish posts in general, current_user_can( 'publish_posts' ) should be used (note the plural posts), which does not require a post ID.

If you're checking whether they can publish a particular post, current_user_can( 'publish_post', $post_id ) should be used, which does require a post ID.

Checking current_user_can( 'publish_post' ) without passing in a post ID seems like a developer error, so the notice is legitimate.

Replying to mattheweppelsheimer:

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.

Yes, I think a _doing_it_wrong() notice similar to the ones added in [34091] or [47178] would be a better experience here. Let's add that :)

#7 @SergeyBiryukov
3 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

@jeherve
3 months ago

_doing_it_wrong notice when forgetting to pass an arg for a capability that requires one

#8 @jeherve
3 months ago

  • Keywords has-patch added; needs-patch removed

I ran into this in comment:20:ticket50913, and a _doing_it_wrong() notice would definitely have sped up my debugging steps, that's a good idea.

Is the patch attached what you had in mind?

#9 follow-up: @henry.wright
8 weeks ago

I disagree with _doing_it_wrong. My reasoning is the argument is optional. Optional indicates the function should be callable without passing a third argument

I agree though, a type of guard is necessary

#10 in reply to: ↑ 9 @SergeyBiryukov
8 weeks ago

Replying to henry.wright:

I disagree with _doing_it_wrong. My reasoning is the argument is optional. Optional indicates the function should be callable without passing a third argument

Although the $args parameter is optional for the map_meta_cap() function, it's not really optional for some particular checks that do require an object ID (as noted in comment:23:ticket:50913):

  • current_user_can( 'delete_post', $post_id )
  • current_user_can( 'edit_post', $post_id )
  • current_user_can( 'read_post', $post_id )
  • current_user_can( 'publish_post', $post_id )
  • current_user_can( 'edit_comment', $comment_id )
  • etc.

These all require a post ID to check, and will cause a PHP notice if the ID is not provided. Performing these checks without passing in a post ID (or a comment ID, in case of edit_comment) is a developer error, so I think a _doing_it_wrong() notice here would be the most appropriate way to notify the developer or site owner that something is wrong and doesn't work as expected.

#11 @henry.wright
8 weeks ago

using current_user_can() with a single argument isn't a developer error according to the function definition. it is a developer error to attempt to use a non-existing second argument in the function definition though

in my opinion the developer calling these functions isn't doing it wrong but the developer writing the function definition may be doing it wrong depending on whether they attempt to access a variable not set

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


4 weeks ago

#13 @hellofromTonya
4 weeks ago

  • Keywords needs-refresh added

Per scrub, Sergey notes:

The patch needs some adjustments for i18n, I'll refresh

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


3 weeks ago

#15 @hellofromTonya
3 weeks ago

  • Milestone changed from 5.6 to Future Release

With 5.6 RC1 tomorrow, it's too late in the cycle. Punting this ticket.

If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

#16 @SergeyBiryukov
2 weeks ago

  • Milestone changed from Future Release to 5.7
Note: See TracTickets for help on using tickets.