Skip to content

Commit 695476e

Browse files
SergeyBiryukovSergeyBiryukov
committed
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
1 parent 8d58139 commit 695476e

File tree

2 files changed

+106
-60
lines changed

2 files changed

+106
-60
lines changed

‎src/wp-includes/comment.php

+75-54
Original file line numberDiff line numberDiff line change
@@ -773,59 +773,7 @@ function wp_allow_comment( $commentdata, $wp_error = false ) {
773773
return new WP_Error( 'comment_flood', $comment_flood_message, 429 );
774774
}
775775

776-
if ( ! empty( $commentdata['user_id'] ) ) {
777-
$user = get_userdata( $commentdata['user_id'] );
778-
$post_author = $wpdb->get_var(
779-
$wpdb->prepare(
780-
"SELECT post_author FROM $wpdb->posts WHERE ID = %d LIMIT 1",
781-
$commentdata['comment_post_ID']
782-
)
783-
);
784-
}
785-
786-
if ( isset( $user ) && ( $commentdata['user_id'] == $post_author || $user->has_cap( 'moderate_comments' ) ) ) {
787-
// The author and the admins get respect.
788-
$approved = 1;
789-
} else {
790-
// Everyone else's comments will be checked.
791-
if ( check_comment(
792-
$commentdata['comment_author'],
793-
$commentdata['comment_author_email'],
794-
$commentdata['comment_author_url'],
795-
$commentdata['comment_content'],
796-
$commentdata['comment_author_IP'],
797-
$commentdata['comment_agent'],
798-
$commentdata['comment_type']
799-
) ) {
800-
$approved = 1;
801-
} else {
802-
$approved = 0;
803-
}
804-
805-
if ( wp_check_comment_disallowed_list(
806-
$commentdata['comment_author'],
807-
$commentdata['comment_author_email'],
808-
$commentdata['comment_author_url'],
809-
$commentdata['comment_content'],
810-
$commentdata['comment_author_IP'],
811-
$commentdata['comment_agent']
812-
) ) {
813-
$approved = EMPTY_TRASH_DAYS ? 'trash' : 'spam';
814-
}
815-
}
816-
817-
/**
818-
* Filters a comment's approval status before it is set.
819-
*
820-
* @since 2.1.0
821-
* @since 4.9.0 Returning a WP_Error value from the filter will short-circuit comment insertion
822-
* and allow skipping further processing.
823-
*
824-
* @param int|string|WP_Error $approved The approval status. Accepts 1, 0, 'spam', 'trash',
825-
* or WP_Error.
826-
* @param array $commentdata Comment data.
827-
*/
828-
return apply_filters( 'pre_comment_approved', $approved, $commentdata );
776+
return wp_check_comment_data( $commentdata );
829777
}
830778

831779
/**
@@ -1292,6 +1240,75 @@ function wp_check_comment_data_max_lengths( $comment_data ) {
12921240
return true;
12931241
}
12941242

1243+
/**
1244+
* Checks whether comment data passes internal checks or has disallowed content.
1245+
*
1246+
* @since 6.7.0
1247+
*
1248+
* @global wpdb $wpdb WordPress database abstraction object.
1249+
*
1250+
* @param array $comment_data Array of arguments for inserting a comment.
1251+
* @return int|string|WP_Error The approval status on success (0|1|'spam'|'trash'),
1252+
* WP_Error otherwise.
1253+
*/
1254+
function wp_check_comment_data( $comment_data ) {
1255+
global $wpdb;
1256+
1257+
if ( ! empty( $comment_data['user_id'] ) ) {
1258+
$user = get_userdata( $comment_data['user_id'] );
1259+
$post_author = $wpdb->get_var(
1260+
$wpdb->prepare(
1261+
"SELECT post_author FROM $wpdb->posts WHERE ID = %d LIMIT 1",
1262+
$comment_data['comment_post_ID']
1263+
)
1264+
);
1265+
}
1266+
1267+
if ( isset( $user ) && ( $comment_data['user_id'] == $post_author || $user->has_cap( 'moderate_comments' ) ) ) {
1268+
// The author and the admins get respect.
1269+
$approved = 1;
1270+
} else {
1271+
// Everyone else's comments will be checked.
1272+
if ( check_comment(
1273+
$comment_data['comment_author'],
1274+
$comment_data['comment_author_email'],
1275+
$comment_data['comment_author_url'],
1276+
$comment_data['comment_content'],
1277+
$comment_data['comment_author_IP'],
1278+
$comment_data['comment_agent'],
1279+
$comment_data['comment_type']
1280+
) ) {
1281+
$approved = 1;
1282+
} else {
1283+
$approved = 0;
1284+
}
1285+
1286+
if ( wp_check_comment_disallowed_list(
1287+
$comment_data['comment_author'],
1288+
$comment_data['comment_author_email'],
1289+
$comment_data['comment_author_url'],
1290+
$comment_data['comment_content'],
1291+
$comment_data['comment_author_IP'],
1292+
$comment_data['comment_agent']
1293+
) ) {
1294+
$approved = EMPTY_TRASH_DAYS ? 'trash' : 'spam';
1295+
}
1296+
}
1297+
1298+
/**
1299+
* Filters a comment's approval status before it is set.
1300+
*
1301+
* @since 2.1.0
1302+
* @since 4.9.0 Returning a WP_Error value from the filter will short-circuit comment insertion
1303+
* and allow skipping further processing.
1304+
*
1305+
* @param int|string|WP_Error $approved The approval status. Accepts 1, 0, 'spam', 'trash',
1306+
* or WP_Error.
1307+
* @param array $commentdata Comment data.
1308+
*/
1309+
return apply_filters( 'pre_comment_approved', $approved, $comment_data );
1310+
}
1311+
12951312
/**
12961313
* Checks if a comment contains disallowed characters or words.
12971314
*
@@ -2279,11 +2296,15 @@ function wp_new_comment( $commentdata, $wp_error = false ) {
22792296

22802297
$commentdata['comment_approved'] = wp_allow_comment( $commentdata, $wp_error );
22812298

2299+
if ( is_wp_error( $commentdata['comment_approved'] ) ) {
2300+
return $commentdata['comment_approved'];
2301+
}
2302+
22822303
$commentdata = wp_filter_comment( $commentdata );
22832304

22842305
if ( ! in_array( $commentdata['comment_approved'], array( 'trash', 'spam' ), true ) ) {
22852306
// Validate the comment again after filters are applied to comment data.
2286-
$commentdata['comment_approved'] = wp_allow_comment( $commentdata, $wp_error );
2307+
$commentdata['comment_approved'] = wp_check_comment_data( $commentdata );
22872308
}
22882309

22892310
if ( is_wp_error( $commentdata['comment_approved'] ) ) {

‎tests/phpunit/tests/comment/wpHandleCommentSubmission.php

+31-6
Original file line numberDiff line numberDiff line change
@@ -989,9 +989,8 @@ public function test_disallowed_keys_match_gives_approved_status_of_trash() {
989989

990990
$comment = wp_handle_comment_submission( $data );
991991

992-
$this->assertNotWPError( $comment );
993-
$this->assertInstanceOf( 'WP_Comment', $comment );
994-
$this->assertSame( 'trash', $comment->comment_approved );
992+
$this->assertInstanceOf( 'WP_Comment', $comment, 'The comment was not submitted.' );
993+
$this->assertSame( 'trash', $comment->comment_approved, 'The wrong approved status was returned.' );
995994
}
996995

997996
/**
@@ -1009,8 +1008,34 @@ public function test_disallowed_keys_html_match_gives_approved_status_of_trash()
10091008

10101009
$comment = wp_handle_comment_submission( $data );
10111010

1012-
$this->assertNotWPError( $comment );
1013-
$this->assertInstanceOf( 'WP_Comment', $comment );
1014-
$this->assertSame( 'trash', $comment->comment_approved );
1011+
$this->assertInstanceOf( 'WP_Comment', $comment, 'The comment was not submitted.' );
1012+
$this->assertSame( 'trash', $comment->comment_approved, 'The wrong approved status was returned.' );
1013+
}
1014+
1015+
/**
1016+
* @ticket 61827
1017+
*/
1018+
public function test_disallowed_keys_filtered_html_match_does_not_call_check_comment_flood_action_twice() {
1019+
$data = array(
1020+
'comment_post_ID' => self::$post->ID,
1021+
'comment' => '<a href=http://example.com/>example</a>',
1022+
'author' => 'Comment Author',
1023+
'email' => '[email protected]',
1024+
);
1025+
1026+
update_option( 'disallowed_keys', "href=\\\"http\nfoo" );
1027+
1028+
$pre_comment_approved = new MockAction();
1029+
$check_comment_flood = new MockAction();
1030+
add_filter( 'pre_comment_approved', array( $pre_comment_approved, 'filter' ), 10, 2 );
1031+
add_action( 'check_comment_flood', array( $check_comment_flood, 'action' ), 10, 4 );
1032+
1033+
$comment = wp_handle_comment_submission( $data );
1034+
1035+
$this->assertInstanceOf( 'WP_Comment', $comment, 'The comment was not submitted.' );
1036+
$this->assertSame( 'trash', $comment->comment_approved, 'The wrong approved status was returned.' );
1037+
1038+
$this->assertSame( 2, $pre_comment_approved->get_call_count(), 'The `pre_comment_approved` filter was not called twice.' );
1039+
$this->assertSame( 1, $check_comment_flood->get_call_count(), 'The `check_comment_flood` action was not called exactly once.' );
10151040
}
10161041
}

0 commit comments

Comments
 (0)