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

[henry.wright]

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

[johnbillion]

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

[henry.wright]

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.

[mattheweppelsheimer]

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?

[SergeyBiryukov]

#48415 was marked as a duplicate.

[SergeyBiryukov]

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

[jeherve]

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

[jeherve]

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?

[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

I agree though, a type of guard is necessary

[SergeyBiryukov]

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.

[henry.wright]

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

[hellofromTonya]

Per scrub, Sergey notes:

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

[hellofromTonya]

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.

[SergeyBiryukov]

#52216 was marked as a duplicate.

[a223123131]

This bug is 2 years old... why isn't it solved?

[a223123131]

Such an old bug still not solved.... an then skipped from 5.7 to 5.8. :-( I would recommend to skip it to 8.5

[johnbillion]

This really isn't a bug that anybody should be experiencing often, let alone over a period of years. If you are, then the third party code in question needs to be fixed because this is not a bug in WordPress but a bug in the code that's performing a capability check without all the required parameters.

I agree that triggering a _doing_it_wrong() in this situation will make it clearer to the developer what the problem is, but the third party code that triggers the problem still needs to be fixed.

[JeffPaul]

Approach on a desired patch has been set by @johnbillion 3 months ago, but given no patch/PR on this and 5.8 Beta 1 approaching next week I'm going to punt this to Future Release. Once a patch/PR is ready, this ticket can be moved back to a numbered milestone.

[jeherve]

Replying to JeffPaul:

Approach on a desired patch has been set by @johnbillion 3 months ago, but given no patch/PR on this and 5.8 Beta 1 approaching next week I'm going to punt this to Future Release. Once a patch/PR is ready, this ticket can be moved back to a numbered milestone.

I've refreshed the patch I had submitted above that was introducing _doing_it_wrong(), and attempted to fix any i18n issues that may have been discussed in bug scrub:

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

This should now be ready for another review.

[jeherve]

Refreshed patch, also attempting to fix any i18n phpcs warnings in the process.

[jeherve]

Refreshed patch with 5.9.0 version numbers

[hellofromTonya]

With 5.9 Beta 1 tomorrow and the patch needing tests and review of the message, moving this to 6.0.

[hellofromTonya]

Whoops sorry forgot to move the milestone.

[azouamauriac]

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

You're right, but, Optional doesn't not mean always. So it's the responsibility of developer who is calling the function to know in which context, he is, and do things accordingly. I think doing_it_wrong is accurate.

[azouamauriac]

44591-3.diff LGTM I'll just suggest that as the guard's code is repetitive if we can wrap it into a function to respect DRY principe it will be good.

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

[peterwilsoncc]

In the ​linked pull request:

44591-3.diff​ with the following changes:

• version updated to 6.0.0
• removed escaping of translations, core trusts translations
• added unit tests to ensure the affected caps contain do_not_allow and throw a doing it wrong
• fixed test_taxonomy_meta_capabilities_with_non_existent_terms to expect the error

I'll just suggest that as the guard's code is repetitive if we can wrap it into a function to respect DRY principe it will be good.

@azouamauriac you're right, it's repetitious. For now I think it's fine to keep the repetition rather than set up a specific function to wrap _doing_it_wrong(). It's a borderline call, so please feel free to push back on this if you wish.

Thanks for the refreshed PR!

Just noting that inserting a type of content, such as "post" or "comment", into a generic string like ...you must always check it against a specific %2$s is not translatable.

A general best practice in WordPress is to avoid post type and taxonomy names in generic strings due to i18n concerns and structural differences in languages. See some previous discussions for reference:

• https://core.trac.wordpress.org/ticket/17609#comment:3
• https://core.trac.wordpress.org/ticket/19099#comment:1
• https://core.trac.wordpress.org/ticket/37895

To clarify a bit more, while both post or comment and ...you must always check it against a specific %2$s string itself are translatable, this or any other generic string like that is not _correctly_ translatable. It does not account for different word forms or declensions in other languages, so no matter how you translate it, the result is still incorrect.

To fix this, separate strings for each type of content should be used instead:

• When checking for the "%s" capability, you must always check it against a specific post.
• When checking for the "%s" capability, you must always check it against a specific page.
• When checking for the "%s" capability, you must always check it against a specific comment.
• When checking for the "%s" capability, you must always check it against a specific term.

I can work on this in a bit.

To clarify a bit more, while both post or comment and ...you must always check it against a specific %2$s string itself are translatable, this or any other generic string like that is not _correctly_ translatable. It does not account for different word forms or declensions in other languages, so no matter how you translate it, the result is still incorrect.

Thanks for the help with i18n, I've pushed some changes in 67770b17c541bdc105cd9f7a3d4155da7819ce0e but please feel free to push to my repository if further work is needed. I'm also happy to make such changes if needs be, the more I know about this as a native English speaker, the better.

Thanks for the refresh! This looks good to me in terms of i18n now 👍

I've pushed some minor formatting changes so that the structure of these blocks is consistent across the function, and is more in line with similar messages elsewhere in core. This includes:

• Only using numbered placeholders if there are multiple placeholders in a string.
• Using <code> tags instead of double quotes for code fragments like the post type name or capability name.
• Removing esc_html() from $cap, as it's not escaped in the existing messages, and neither is the post type or post status. Happy to add the escaping back if necessary, but I think it should be done consistently, whether escaped or not.

Sorry for being a bit late with this review. Since we're now in soft string freeze after Beta 3, I guess we should move this to 6.1.

[peterwilsoncc]

As beta 3 was the ​soft string freeze, I'm moving this to the 6.1 milestone.

The updated pull request was approved overnight so it's ready for commit.

[SergeyBiryukov]

In 53408:

Users: Fail gracefully when checking mapped capabilities without providing the required object ID.

This avoids an Undefined array key 0 PHP warning for current_user_can() capability checks that require a specific object to check against but an object ID was not passed.

A _doing_it_wrong() notice is also added, so that developers and site administrators are aware that the capability mapping is failing in the absence of the required object ID.

The list of mapped capabilities that require an object ID:

• delete_post / delete_page
• edit_post / edit_page
• read_post / read_page
• publish_post
• edit_(post|comment|term|user)_meta / delete_*_meta / add_*_meta
• edit_comment
• edit_term / delete_term / assign_term

Follow-up to [34091], [34113], [47178].

Props jeherve, peterwilsoncc, henry.wright, johnbillion, mattheweppelsheimer, hellofromTonya, JeffPaul, azouamauriac, Ninos Ego, TobiasBg, wpsmith, GaryJ, nacin, johnstonphilip, azaozz, SergeyBiryukov.
Fixes #44591.

Merged in https://core.trac.wordpress.org/changeset/53408.