From 590ca0ff94e958d26e3103114ea22c34f1e2fff9 Mon Sep 17 00:00:00 2001 From: Sergey Biryukov Date: Tue, 17 May 2022 18:59:24 +0000 Subject: [PATCH] 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. git-svn-id: https://develop.svn.wordpress.org/trunk@53408 602fd350-edb4-49c9-b593-d223f7449a82 --- src/wp-includes/capabilities.php | 192 +++++++++++++++++++++- tests/phpunit/tests/user.php | 2 +- tests/phpunit/tests/user/capabilities.php | 1 + tests/phpunit/tests/user/mapMetaCap.php | 48 ++++++ 4 files changed, 236 insertions(+), 7 deletions(-) diff --git a/src/wp-includes/capabilities.php b/src/wp-includes/capabilities.php index 2d5cd9a958..5eea17197a 100644 --- a/src/wp-includes/capabilities.php +++ b/src/wp-includes/capabilities.php @@ -73,6 +73,25 @@ function map_meta_cap( $cap, $user_id, ...$args ) { break; case 'delete_post': case 'delete_page': + if ( ! isset( $args[0] ) ) { + if ( 'delete_post' === $cap ) { + /* translators: %s: Capability name. */ + $message = __( 'When checking for the %s capability, you must always check it against a specific post.' ); + } else { + /* translators: %s: Capability name. */ + $message = __( 'When checking for the %s capability, you must always check it against a specific page.' ); + } + + _doing_it_wrong( + __FUNCTION__, + sprintf( $message, '' . $cap . '' ), + '6.1.0' + ); + + $caps[] = 'do_not_allow'; + break; + } + $post = get_post( $args[0] ); if ( ! $post ) { $caps[] = 'do_not_allow'; @@ -92,7 +111,18 @@ function map_meta_cap( $cap, $user_id, ...$args ) { $post_type = get_post_type_object( $post->post_type ); if ( ! $post_type ) { /* translators: 1: Post type, 2: Capability name. */ - _doing_it_wrong( __FUNCTION__, sprintf( __( 'The post type %1$s is not registered, so it may not be reliable to check the capability "%2$s" against a post of that type.' ), $post->post_type, $cap ), '4.4.0' ); + $message = __( 'The post type %1$s is not registered, so it may not be reliable to check the capability %2$s against a post of that type.' ); + + _doing_it_wrong( + __FUNCTION__, + sprintf( + $message, + '' . $post->post_type . '', + '' . $cap . '' + ), + '4.4.0' + ); + $caps[] = 'edit_others_posts'; break; } @@ -146,6 +176,25 @@ function map_meta_cap( $cap, $user_id, ...$args ) { // edit_others_posts. case 'edit_post': case 'edit_page': + if ( ! isset( $args[0] ) ) { + if ( 'edit_post' === $cap ) { + /* translators: %s: Capability name. */ + $message = __( 'When checking for the %s capability, you must always check it against a specific post.' ); + } else { + /* translators: %s: Capability name. */ + $message = __( 'When checking for the %s capability, you must always check it against a specific page.' ); + } + + _doing_it_wrong( + __FUNCTION__, + sprintf( $message, '' . $cap . '' ), + '6.1.0' + ); + + $caps[] = 'do_not_allow'; + break; + } + $post = get_post( $args[0] ); if ( ! $post ) { $caps[] = 'do_not_allow'; @@ -163,7 +212,18 @@ function map_meta_cap( $cap, $user_id, ...$args ) { $post_type = get_post_type_object( $post->post_type ); if ( ! $post_type ) { /* translators: 1: Post type, 2: Capability name. */ - _doing_it_wrong( __FUNCTION__, sprintf( __( 'The post type %1$s is not registered, so it may not be reliable to check the capability "%2$s" against a post of that type.' ), $post->post_type, $cap ), '4.4.0' ); + $message = __( 'The post type %1$s is not registered, so it may not be reliable to check the capability %2$s against a post of that type.' ); + + _doing_it_wrong( + __FUNCTION__, + sprintf( + $message, + '' . $post->post_type . '', + '' . $cap . '' + ), + '4.4.0' + ); + $caps[] = 'edit_others_posts'; break; } @@ -215,6 +275,25 @@ function map_meta_cap( $cap, $user_id, ...$args ) { break; case 'read_post': case 'read_page': + if ( ! isset( $args[0] ) ) { + if ( 'read_post' === $cap ) { + /* translators: %s: Capability name. */ + $message = __( 'When checking for the %s capability, you must always check it against a specific post.' ); + } else { + /* translators: %s: Capability name. */ + $message = __( 'When checking for the %s capability, you must always check it against a specific page.' ); + } + + _doing_it_wrong( + __FUNCTION__, + sprintf( $message, '' . $cap . '' ), + '6.1.0' + ); + + $caps[] = 'do_not_allow'; + break; + } + $post = get_post( $args[0] ); if ( ! $post ) { $caps[] = 'do_not_allow'; @@ -232,7 +311,18 @@ function map_meta_cap( $cap, $user_id, ...$args ) { $post_type = get_post_type_object( $post->post_type ); if ( ! $post_type ) { /* translators: 1: Post type, 2: Capability name. */ - _doing_it_wrong( __FUNCTION__, sprintf( __( 'The post type %1$s is not registered, so it may not be reliable to check the capability "%2$s" against a post of that type.' ), $post->post_type, $cap ), '4.4.0' ); + $message = __( 'The post type %1$s is not registered, so it may not be reliable to check the capability %2$s against a post of that type.' ); + + _doing_it_wrong( + __FUNCTION__, + sprintf( + $message, + '' . $post->post_type . '', + '' . $cap . '' + ), + '4.4.0' + ); + $caps[] = 'edit_others_posts'; break; } @@ -249,7 +339,18 @@ function map_meta_cap( $cap, $user_id, ...$args ) { $status_obj = get_post_status_object( get_post_status( $post ) ); if ( ! $status_obj ) { /* translators: 1: Post status, 2: Capability name. */ - _doing_it_wrong( __FUNCTION__, sprintf( __( 'The post status %1$s is not registered, so it may not be reliable to check the capability "%2$s" against a post with that status.' ), get_post_status( $post ), $cap ), '5.4.0' ); + $message = __( 'The post status %1$s is not registered, so it may not be reliable to check the capability %2$s against a post with that status.' ); + + _doing_it_wrong( + __FUNCTION__, + sprintf( + $message, + '' . get_post_status( $post ) . '', + '' . $cap . '' + ), + '5.4.0' + ); + $caps[] = 'edit_others_posts'; break; } @@ -268,6 +369,20 @@ function map_meta_cap( $cap, $user_id, ...$args ) { } break; case 'publish_post': + if ( ! isset( $args[0] ) ) { + /* translators: %s: Capability name. */ + $message = __( 'When checking for the %s capability, you must always check it against a specific post.' ); + + _doing_it_wrong( + __FUNCTION__, + sprintf( $message, '' . $cap . '' ), + '6.1.0' + ); + + $caps[] = 'do_not_allow'; + break; + } + $post = get_post( $args[0] ); if ( ! $post ) { $caps[] = 'do_not_allow'; @@ -277,7 +392,18 @@ function map_meta_cap( $cap, $user_id, ...$args ) { $post_type = get_post_type_object( $post->post_type ); if ( ! $post_type ) { /* translators: 1: Post type, 2: Capability name. */ - _doing_it_wrong( __FUNCTION__, sprintf( __( 'The post type %1$s is not registered, so it may not be reliable to check the capability "%2$s" against a post of that type.' ), $post->post_type, $cap ), '4.4.0' ); + $message = __( 'The post type %1$s is not registered, so it may not be reliable to check the capability %2$s against a post of that type.' ); + + _doing_it_wrong( + __FUNCTION__, + sprintf( + $message, + '' . $post->post_type . '', + '' . $cap . '' + ), + '4.4.0' + ); + $caps[] = 'edit_others_posts'; break; } @@ -297,7 +423,33 @@ function map_meta_cap( $cap, $user_id, ...$args ) { case 'delete_user_meta': case 'add_user_meta': $object_type = explode( '_', $cap )[1]; - $object_id = (int) $args[0]; + + if ( ! isset( $args[0] ) ) { + if ( 'post' === $object_type ) { + /* translators: %s: Capability name. */ + $message = __( 'When checking for the %s capability, you must always check it against a specific post.' ); + } elseif ( 'comment' === $object_type ) { + /* translators: %s: Capability name. */ + $message = __( 'When checking for the %s capability, you must always check it against a specific comment.' ); + } elseif ( 'term' === $object_type ) { + /* translators: %s: Capability name. */ + $message = __( 'When checking for the %s capability, you must always check it against a specific term.' ); + } else { + /* translators: %s: Capability name. */ + $message = __( 'When checking for the %s capability, you must always check it against a specific user.' ); + } + + _doing_it_wrong( + __FUNCTION__, + sprintf( $message, '' . $cap . '' ), + '6.1.0' + ); + + $caps[] = 'do_not_allow'; + break; + } + + $object_id = (int) $args[0]; $object_subtype = get_object_subtype( $object_type, $object_id ); @@ -392,6 +544,20 @@ function map_meta_cap( $cap, $user_id, ...$args ) { } break; case 'edit_comment': + if ( ! isset( $args[0] ) ) { + /* translators: %s: Capability name. */ + $message = __( 'When checking for the %s capability, you must always check it against a specific comment.' ); + + _doing_it_wrong( + __FUNCTION__, + sprintf( $message, '' . $cap . '' ), + '6.1.0' + ); + + $caps[] = 'do_not_allow'; + break; + } + $comment = get_comment( $args[0] ); if ( ! $comment ) { $caps[] = 'do_not_allow'; @@ -532,6 +698,20 @@ function map_meta_cap( $cap, $user_id, ...$args ) { case 'edit_term': case 'delete_term': case 'assign_term': + if ( ! isset( $args[0] ) ) { + /* translators: %s: Capability name. */ + $message = __( 'When checking for the %s capability, you must always check it against a specific term.' ); + + _doing_it_wrong( + __FUNCTION__, + sprintf( $message, '' . $cap . '' ), + '6.1.0' + ); + + $caps[] = 'do_not_allow'; + break; + } + $term_id = (int) $args[0]; $term = get_term( $term_id ); if ( ! $term || is_wp_error( $term ) ) { diff --git a/tests/phpunit/tests/user.php b/tests/phpunit/tests/user.php index 67ab0f04f7..e2025c614a 100644 --- a/tests/phpunit/tests/user.php +++ b/tests/phpunit/tests/user.php @@ -1974,7 +1974,7 @@ class Tests_User extends WP_UnitTestCase { ) ); - // _doing_wrong() should be called because the filter callback + // _doing_it_wrong() should be called because the filter callback // adds a item with a 'name' that is the same as one generated by core. $this->setExpectedIncorrectUsage( 'wp_user_personal_data_exporter' ); add_filter( 'wp_privacy_additional_user_profile_data', array( $this, 'export_additional_user_profile_data_with_dup_name' ) ); diff --git a/tests/phpunit/tests/user/capabilities.php b/tests/phpunit/tests/user/capabilities.php index 71a4a12d59..cc16567576 100644 --- a/tests/phpunit/tests/user/capabilities.php +++ b/tests/phpunit/tests/user/capabilities.php @@ -1595,6 +1595,7 @@ class Tests_User_Capabilities extends WP_UnitTestCase { $editor = self::$users['editor']; + $this->setExpectedIncorrectUsage( 'map_meta_cap' ); foreach ( $caps as $cap ) { // `null` represents a non-existent term ID. $this->assertFalse( user_can( $editor->ID, $cap, null ) ); diff --git a/tests/phpunit/tests/user/mapMetaCap.php b/tests/phpunit/tests/user/mapMetaCap.php index b45e6260a2..fb26eca8b5 100644 --- a/tests/phpunit/tests/user/mapMetaCap.php +++ b/tests/phpunit/tests/user/mapMetaCap.php @@ -3,6 +3,7 @@ /** * @group user * @group capabilities + * @covers ::map_meta_cap */ class Tests_User_MapMetaCap extends WP_UnitTestCase { @@ -410,4 +411,51 @@ class Tests_User_MapMetaCap extends WP_UnitTestCase { $this->assertSame( array( 'manage_options' ), $caps ); } + + /** + * @dataProvider data_meta_caps_throw_doing_it_wrong_without_required_argument_provided + * @ticket 44591 + * + * @param string $cap The meta capability requiring an argument. + */ + public function test_meta_caps_throw_doing_it_wrong_without_required_argument_provided( $cap ) { + $admin_user = self::$user_id; + $this->setExpectedIncorrectUsage( 'map_meta_cap' ); + $this->assertContains( 'do_not_allow', map_meta_cap( $cap, $admin_user ) ); + } + + /** + * Data provider. + * + * @return array[] Test parameters { + * @type string $cap The meta capability requiring an argument. + * } + */ + public function data_meta_caps_throw_doing_it_wrong_without_required_argument_provided() { + return array( + array( 'delete_post' ), + array( 'delete_page' ), + array( 'edit_post' ), + array( 'edit_page' ), + array( 'read_post' ), + array( 'read_page' ), + array( 'publish_post' ), + array( 'edit_post_meta' ), + array( 'delete_post_meta' ), + array( 'add_post_meta' ), + array( 'edit_comment_meta' ), + array( 'delete_comment_meta' ), + array( 'add_comment_meta' ), + array( 'edit_term_meta' ), + array( 'delete_term_meta' ), + array( 'add_term_meta' ), + array( 'edit_user_meta' ), + array( 'delete_user_meta' ), + array( 'add_user_meta' ), + array( 'edit_comment' ), + array( 'edit_term' ), + array( 'delete_term' ), + array( 'assign_term' ), + ); + } }