From 695476ea5e51d1c92f389ac236b138ceeaee8a99 Mon Sep 17 00:00:00 2001 From: Sergey Biryukov Date: Tue, 29 Oct 2024 15:36:56 +0000 Subject: [PATCH] Comments: Use a more precise check for disallowed keys on filtered comment data. The previous approach of running `wp_allow_comment()` twice could have unintended consequences, e.g. the `check_comment_flood` action was also triggered twice, which might lead to false-positive identification of comment flood in case there is some custom callback hooked to it, which is not expecting identical data seeing twice. This commit introduces a new function, `wp_check_comment_data()`, to specifically check for disallowed content before and after comment data is filtered. Follow-up to [59267]. Props david.binda, SergeyBiryukov. See #61827. git-svn-id: https://develop.svn.wordpress.org/trunk@59319 602fd350-edb4-49c9-b593-d223f7449a82 --- src/wp-includes/comment.php | 129 ++++++++++-------- .../comment/wpHandleCommentSubmission.php | 37 ++++- 2 files changed, 106 insertions(+), 60 deletions(-) diff --git a/src/wp-includes/comment.php b/src/wp-includes/comment.php index ce37558d047e3..4360f14d09554 100644 --- a/src/wp-includes/comment.php +++ b/src/wp-includes/comment.php @@ -773,59 +773,7 @@ function wp_allow_comment( $commentdata, $wp_error = false ) { return new WP_Error( 'comment_flood', $comment_flood_message, 429 ); } - if ( ! empty( $commentdata['user_id'] ) ) { - $user = get_userdata( $commentdata['user_id'] ); - $post_author = $wpdb->get_var( - $wpdb->prepare( - "SELECT post_author FROM $wpdb->posts WHERE ID = %d LIMIT 1", - $commentdata['comment_post_ID'] - ) - ); - } - - if ( isset( $user ) && ( $commentdata['user_id'] == $post_author || $user->has_cap( 'moderate_comments' ) ) ) { - // The author and the admins get respect. - $approved = 1; - } else { - // Everyone else's comments will be checked. - if ( check_comment( - $commentdata['comment_author'], - $commentdata['comment_author_email'], - $commentdata['comment_author_url'], - $commentdata['comment_content'], - $commentdata['comment_author_IP'], - $commentdata['comment_agent'], - $commentdata['comment_type'] - ) ) { - $approved = 1; - } else { - $approved = 0; - } - - if ( wp_check_comment_disallowed_list( - $commentdata['comment_author'], - $commentdata['comment_author_email'], - $commentdata['comment_author_url'], - $commentdata['comment_content'], - $commentdata['comment_author_IP'], - $commentdata['comment_agent'] - ) ) { - $approved = EMPTY_TRASH_DAYS ? 'trash' : 'spam'; - } - } - - /** - * Filters a comment's approval status before it is set. - * - * @since 2.1.0 - * @since 4.9.0 Returning a WP_Error value from the filter will short-circuit comment insertion - * and allow skipping further processing. - * - * @param int|string|WP_Error $approved The approval status. Accepts 1, 0, 'spam', 'trash', - * or WP_Error. - * @param array $commentdata Comment data. - */ - return apply_filters( 'pre_comment_approved', $approved, $commentdata ); + return wp_check_comment_data( $commentdata ); } /** @@ -1292,6 +1240,75 @@ function wp_check_comment_data_max_lengths( $comment_data ) { return true; } +/** + * Checks whether comment data passes internal checks or has disallowed content. + * + * @since 6.7.0 + * + * @global wpdb $wpdb WordPress database abstraction object. + * + * @param array $comment_data Array of arguments for inserting a comment. + * @return int|string|WP_Error The approval status on success (0|1|'spam'|'trash'), + * WP_Error otherwise. + */ +function wp_check_comment_data( $comment_data ) { + global $wpdb; + + if ( ! empty( $comment_data['user_id'] ) ) { + $user = get_userdata( $comment_data['user_id'] ); + $post_author = $wpdb->get_var( + $wpdb->prepare( + "SELECT post_author FROM $wpdb->posts WHERE ID = %d LIMIT 1", + $comment_data['comment_post_ID'] + ) + ); + } + + if ( isset( $user ) && ( $comment_data['user_id'] == $post_author || $user->has_cap( 'moderate_comments' ) ) ) { + // The author and the admins get respect. + $approved = 1; + } else { + // Everyone else's comments will be checked. + if ( check_comment( + $comment_data['comment_author'], + $comment_data['comment_author_email'], + $comment_data['comment_author_url'], + $comment_data['comment_content'], + $comment_data['comment_author_IP'], + $comment_data['comment_agent'], + $comment_data['comment_type'] + ) ) { + $approved = 1; + } else { + $approved = 0; + } + + if ( wp_check_comment_disallowed_list( + $comment_data['comment_author'], + $comment_data['comment_author_email'], + $comment_data['comment_author_url'], + $comment_data['comment_content'], + $comment_data['comment_author_IP'], + $comment_data['comment_agent'] + ) ) { + $approved = EMPTY_TRASH_DAYS ? 'trash' : 'spam'; + } + } + + /** + * Filters a comment's approval status before it is set. + * + * @since 2.1.0 + * @since 4.9.0 Returning a WP_Error value from the filter will short-circuit comment insertion + * and allow skipping further processing. + * + * @param int|string|WP_Error $approved The approval status. Accepts 1, 0, 'spam', 'trash', + * or WP_Error. + * @param array $commentdata Comment data. + */ + return apply_filters( 'pre_comment_approved', $approved, $comment_data ); +} + /** * Checks if a comment contains disallowed characters or words. * @@ -2279,11 +2296,15 @@ function wp_new_comment( $commentdata, $wp_error = false ) { $commentdata['comment_approved'] = wp_allow_comment( $commentdata, $wp_error ); + if ( is_wp_error( $commentdata['comment_approved'] ) ) { + return $commentdata['comment_approved']; + } + $commentdata = wp_filter_comment( $commentdata ); if ( ! in_array( $commentdata['comment_approved'], array( 'trash', 'spam' ), true ) ) { // Validate the comment again after filters are applied to comment data. - $commentdata['comment_approved'] = wp_allow_comment( $commentdata, $wp_error ); + $commentdata['comment_approved'] = wp_check_comment_data( $commentdata ); } if ( is_wp_error( $commentdata['comment_approved'] ) ) { diff --git a/tests/phpunit/tests/comment/wpHandleCommentSubmission.php b/tests/phpunit/tests/comment/wpHandleCommentSubmission.php index 9dfee513d53c2..49d54e3da52cd 100644 --- a/tests/phpunit/tests/comment/wpHandleCommentSubmission.php +++ b/tests/phpunit/tests/comment/wpHandleCommentSubmission.php @@ -989,9 +989,8 @@ public function test_disallowed_keys_match_gives_approved_status_of_trash() { $comment = wp_handle_comment_submission( $data ); - $this->assertNotWPError( $comment ); - $this->assertInstanceOf( 'WP_Comment', $comment ); - $this->assertSame( 'trash', $comment->comment_approved ); + $this->assertInstanceOf( 'WP_Comment', $comment, 'The comment was not submitted.' ); + $this->assertSame( 'trash', $comment->comment_approved, 'The wrong approved status was returned.' ); } /** @@ -1009,8 +1008,34 @@ public function test_disallowed_keys_html_match_gives_approved_status_of_trash() $comment = wp_handle_comment_submission( $data ); - $this->assertNotWPError( $comment ); - $this->assertInstanceOf( 'WP_Comment', $comment ); - $this->assertSame( 'trash', $comment->comment_approved ); + $this->assertInstanceOf( 'WP_Comment', $comment, 'The comment was not submitted.' ); + $this->assertSame( 'trash', $comment->comment_approved, 'The wrong approved status was returned.' ); + } + + /** + * @ticket 61827 + */ + public function test_disallowed_keys_filtered_html_match_does_not_call_check_comment_flood_action_twice() { + $data = array( + 'comment_post_ID' => self::$post->ID, + 'comment' => 'example', + 'author' => 'Comment Author', + 'email' => 'comment@example.org', + ); + + update_option( 'disallowed_keys', "href=\\\"http\nfoo" ); + + $pre_comment_approved = new MockAction(); + $check_comment_flood = new MockAction(); + add_filter( 'pre_comment_approved', array( $pre_comment_approved, 'filter' ), 10, 2 ); + add_action( 'check_comment_flood', array( $check_comment_flood, 'action' ), 10, 4 ); + + $comment = wp_handle_comment_submission( $data ); + + $this->assertInstanceOf( 'WP_Comment', $comment, 'The comment was not submitted.' ); + $this->assertSame( 'trash', $comment->comment_approved, 'The wrong approved status was returned.' ); + + $this->assertSame( 2, $pre_comment_approved->get_call_count(), 'The `pre_comment_approved` filter was not called twice.' ); + $this->assertSame( 1, $check_comment_flood->get_call_count(), 'The `check_comment_flood` action was not called exactly once.' ); } }