From 08564b5a80071aec579c63d1e0fb36412778cc85 Mon Sep 17 00:00:00 2001 From: Felix Arntz Date: Thu, 10 Jan 2019 21:05:50 +0000 Subject: [PATCH] General: Fix problematic string to array parsing. WordPress has historically often used code like `preg_split( '/[\s,]+/', $var )` to parse a string of comma-separated values into an array. However, this approach was causing an empty string to not be parsed into an empty array as expected, but rather into an array with the empty string as its sole element. This was among other areas causing problems in the REST API where passing an empty request parameter could cause that request to fail because, instead of it being ignored, that parameter would be compared against the valid values for it, which typically do not include an empty string. Props david.binda, sstoqnov. Fixes #43977. git-svn-id: https://develop.svn.wordpress.org/trunk@44546 602fd350-edb4-49c9-b593-d223f7449a82 --- src/wp-includes/bookmark.php | 18 +++--- src/wp-includes/class-wp-comment-query.php | 18 +++--- src/wp-includes/functions.php | 30 ++++++---- src/wp-includes/post.php | 2 +- src/wp-includes/rest-api.php | 10 ++-- .../endpoints/class-wp-rest-controller.php | 2 +- tests/phpunit/tests/bookmark/getBookmarks.php | 57 +++++++++++++++++++ tests/phpunit/tests/functions.php | 24 ++++++++ .../tests/rest-api/rest-controller.php | 35 ++++++++++-- .../rest-api/rest-schema-sanitization.php | 2 + .../tests/rest-api/rest-schema-validation.php | 5 ++ 11 files changed, 159 insertions(+), 44 deletions(-) diff --git a/src/wp-includes/bookmark.php b/src/wp-includes/bookmark.php index b0de276df4..32499541d4 100644 --- a/src/wp-includes/bookmark.php +++ b/src/wp-includes/bookmark.php @@ -171,13 +171,13 @@ function get_bookmarks( $args = '' ) { $r['exclude'] = ''; //ignore exclude, category, and category_name params if using include $r['category'] = ''; $r['category_name'] = ''; - $inclinks = preg_split( '/[\s,]+/', $r['include'] ); + $inclinks = wp_parse_id_list( $r['include'] ); if ( count( $inclinks ) ) { foreach ( $inclinks as $inclink ) { if ( empty( $inclusions ) ) { - $inclusions = ' AND ( link_id = ' . intval( $inclink ) . ' '; + $inclusions = ' AND ( link_id = ' . $inclink . ' '; } else { - $inclusions .= ' OR link_id = ' . intval( $inclink ) . ' '; + $inclusions .= ' OR link_id = ' . $inclink . ' '; } } } @@ -188,13 +188,13 @@ function get_bookmarks( $args = '' ) { $exclusions = ''; if ( ! empty( $r['exclude'] ) ) { - $exlinks = preg_split( '/[\s,]+/', $r['exclude'] ); + $exlinks = wp_parse_id_list( $r['exclude'] ); if ( count( $exlinks ) ) { foreach ( $exlinks as $exlink ) { if ( empty( $exclusions ) ) { - $exclusions = ' AND ( link_id <> ' . intval( $exlink ) . ' '; + $exclusions = ' AND ( link_id <> ' . $exlink . ' '; } else { - $exclusions .= ' AND link_id <> ' . intval( $exlink ) . ' '; + $exclusions .= ' AND link_id <> ' . $exlink . ' '; } } } @@ -223,13 +223,13 @@ function get_bookmarks( $args = '' ) { $category_query = ''; $join = ''; if ( ! empty( $r['category'] ) ) { - $incategories = preg_split( '/[\s,]+/', $r['category'] ); + $incategories = wp_parse_id_list( $r['category'] ); if ( count( $incategories ) ) { foreach ( $incategories as $incat ) { if ( empty( $category_query ) ) { - $category_query = ' AND ( tt.term_id = ' . intval( $incat ) . ' '; + $category_query = ' AND ( tt.term_id = ' . $incat . ' '; } else { - $category_query .= ' OR tt.term_id = ' . intval( $incat ) . ' '; + $category_query .= ' OR tt.term_id = ' . $incat . ' '; } } } diff --git a/src/wp-includes/class-wp-comment-query.php b/src/wp-includes/class-wp-comment-query.php index af05326915..95ab3a6b35 100644 --- a/src/wp-includes/class-wp-comment-query.php +++ b/src/wp-includes/class-wp-comment-query.php @@ -482,9 +482,11 @@ class WP_Comment_Query { // 'status' accepts an array or a comma-separated string. $status_clauses = array(); - $statuses = $this->query_vars['status']; - if ( ! is_array( $statuses ) ) { - $statuses = preg_split( '/[\s,]+/', $statuses ); + $statuses = wp_parse_list( $this->query_vars['status'] ); + + // Empty 'status' should be interpreted as 'all'. + if ( empty( $statuses ) ) { + $statuses = array( 'all' ); } // 'any' overrides other statuses. @@ -517,14 +519,10 @@ class WP_Comment_Query { // User IDs or emails whose unapproved comments are included, regardless of $status. if ( ! empty( $this->query_vars['include_unapproved'] ) ) { - $include_unapproved = $this->query_vars['include_unapproved']; + $include_unapproved = wp_parse_list( $this->query_vars['include_unapproved'] ); - // Accepts arrays or comma-separated strings. - if ( ! is_array( $include_unapproved ) ) { - $include_unapproved = preg_split( '/[\s,]+/', $include_unapproved ); - } - - $unapproved_ids = $unapproved_emails = array(); + $unapproved_ids = array(); + $unapproved_emails = array(); foreach ( $include_unapproved as $unapproved_identifier ) { // Numeric values are assumed to be user ids. if ( is_numeric( $unapproved_identifier ) ) { diff --git a/src/wp-includes/functions.php b/src/wp-includes/functions.php index 39de1470e2..c790f3df39 100644 --- a/src/wp-includes/functions.php +++ b/src/wp-includes/functions.php @@ -3822,6 +3822,22 @@ function wp_parse_args( $args, $defaults = '' ) { return $r; } +/** + * Cleans up an array, comma- or space-separated list of scalar values. + * + * @since 5.1.0 + * + * @param array|string $list List of values. + * @return array Sanitized array of values. + */ +function wp_parse_list( $list ) { + if ( ! is_array( $list ) ) { + return preg_split( '/[\s,]+/', $list, -1, PREG_SPLIT_NO_EMPTY ); + } + + return $list; +} + /** * Clean up an array, comma- or space-separated list of IDs. * @@ -3831,9 +3847,7 @@ function wp_parse_args( $args, $defaults = '' ) { * @return array Sanitized array of IDs. */ function wp_parse_id_list( $list ) { - if ( ! is_array( $list ) ) { - $list = preg_split( '/[\s,]+/', $list ); - } + $list = wp_parse_list( $list ); return array_unique( array_map( 'absint', $list ) ); } @@ -3847,15 +3861,9 @@ function wp_parse_id_list( $list ) { * @return array Sanitized array of slugs. */ function wp_parse_slug_list( $list ) { - if ( ! is_array( $list ) ) { - $list = preg_split( '/[\s,]+/', $list ); - } + $list = wp_parse_list( $list ); - foreach ( $list as $key => $value ) { - $list[ $key ] = sanitize_title( $value ); - } - - return array_unique( $list ); + return array_unique( array_map( 'sanitize_title', $list ) ); } /** diff --git a/src/wp-includes/post.php b/src/wp-includes/post.php index f3696341da..565c243bbc 100644 --- a/src/wp-includes/post.php +++ b/src/wp-includes/post.php @@ -5088,7 +5088,7 @@ function get_pages( $args = array() ) { $author_query = ''; if ( ! empty( $r['authors'] ) ) { - $post_authors = preg_split( '/[\s,]+/', $r['authors'] ); + $post_authors = wp_parse_list( $r['authors'] ); if ( ! empty( $post_authors ) ) { foreach ( $post_authors as $post_author ) { diff --git a/src/wp-includes/rest-api.php b/src/wp-includes/rest-api.php index 1c8bb4620e..e0807e7696 100644 --- a/src/wp-includes/rest-api.php +++ b/src/wp-includes/rest-api.php @@ -679,7 +679,7 @@ function rest_filter_response_fields( $response, $server, $request ) { $data = $response->get_data(); - $fields = is_array( $request['_fields'] ) ? $request['_fields'] : preg_split( '/[\s,]+/', $request['_fields'] ); + $fields = wp_parse_list( $request['_fields'] ); if ( 0 === count( $fields ) ) { return $response; @@ -1109,8 +1109,8 @@ function rest_get_avatar_sizes() { */ function rest_validate_value_from_schema( $value, $args, $param = '' ) { if ( 'array' === $args['type'] ) { - if ( ! is_array( $value ) ) { - $value = preg_split( '/[\s,]+/', $value ); + if ( ! is_null( $value ) ) { + $value = wp_parse_list( $value ); } if ( ! wp_is_numeric_array( $value ) ) { /* translators: 1: parameter, 2: type name */ @@ -1253,9 +1253,7 @@ function rest_sanitize_value_from_schema( $value, $args ) { if ( empty( $args['items'] ) ) { return (array) $value; } - if ( ! is_array( $value ) ) { - $value = preg_split( '/[\s,]+/', $value ); - } + $value = wp_parse_list( $value ); foreach ( $value as $index => $v ) { $value[ $index ] = rest_sanitize_value_from_schema( $v, $args['items'] ); } diff --git a/src/wp-includes/rest-api/endpoints/class-wp-rest-controller.php b/src/wp-includes/rest-api/endpoints/class-wp-rest-controller.php index 6dc971517d..d610504692 100644 --- a/src/wp-includes/rest-api/endpoints/class-wp-rest-controller.php +++ b/src/wp-includes/rest-api/endpoints/class-wp-rest-controller.php @@ -532,7 +532,7 @@ abstract class WP_REST_Controller { if ( ! isset( $request['_fields'] ) ) { return $fields; } - $requested_fields = is_array( $request['_fields'] ) ? $request['_fields'] : preg_split( '/[\s,]+/', $request['_fields'] ); + $requested_fields = wp_parse_list( $request['_fields'] ); if ( 0 === count( $requested_fields ) ) { return $fields; } diff --git a/tests/phpunit/tests/bookmark/getBookmarks.php b/tests/phpunit/tests/bookmark/getBookmarks.php index 13f87957ff..c605a9f0c5 100644 --- a/tests/phpunit/tests/bookmark/getBookmarks.php +++ b/tests/phpunit/tests/bookmark/getBookmarks.php @@ -84,4 +84,61 @@ class Tests_Bookmark_GetBookmarks extends WP_UnitTestCase { $this->assertEqualSets( $found1, $found2 ); $this->assertTrue( $num_queries < $wpdb->num_queries ); } + + public function test_exclude_param_gets_properly_parsed_as_list() { + $bookmarks = self::factory()->bookmark->create_many( 3 ); + + $found = get_bookmarks( + array( + 'exclude' => ',,', + ) + ); + $found_ids = array(); + foreach( $found as $bookmark ) { + $found_ids[] = $bookmark->link_id; + } + + // equal sets != same order. + $this->assertEqualSets( $bookmarks, $found_ids ); + } + + public function test_include_param_gets_properly_parsed_as_list() { + $bookmarks = self::factory()->bookmark->create_many( 3 ); + + $found = get_bookmarks( + array( + 'include' => ',,', + ) + ); + $found_ids = array(); + foreach( $found as $bookmark ) { + $found_ids[] = $bookmark->link_id; + } + + // equal sets != same order. + $this->assertEqualSets( $bookmarks, $found_ids ); + } + + public function test_category_param_propelry_gets_parsed_as_list() { + $bookmarks = self::factory()->bookmark->create_many( 3 ); + $categories = self::factory()->term->create_many( 3, array( + 'taxonomy' => 'link_category', + ) ); + $add = wp_add_object_terms( $bookmarks[0], $categories[0], 'link_category' ); + $add = wp_add_object_terms( $bookmarks[1], $categories[1], 'link_category' ); + $add = wp_add_object_terms( $bookmarks[2], $categories[2], 'link_category' ); + + $found = get_bookmarks( + array( + 'category' => ',,', + ) + ); + $found_ids = array(); + foreach( $found as $bookmark ) { + $found_ids[] = $bookmark->link_id; + } + + // equal sets != same order. + $this->assertEqualSets( $bookmarks, $found_ids ); + } } diff --git a/tests/phpunit/tests/functions.php b/tests/phpunit/tests/functions.php index efbc88a762..7dcea34533 100644 --- a/tests/phpunit/tests/functions.php +++ b/tests/phpunit/tests/functions.php @@ -534,6 +534,30 @@ class Tests_Functions extends WP_UnitTestCase { update_option( 'blog_charset', $orig_blog_charset ); } + /** + * @ticket 43977 + * @dataProvider data_wp_parse_list + */ + function test_wp_parse_list( $expected, $actual ) { + $this->assertSame( $expected, array_values( wp_parse_list( $actual ) ) ); + } + + function data_wp_parse_list() { + return array( + array( array( '1', '2', '3', '4' ), '1,2,3,4' ), + array( array( 'apple', 'banana', 'carrot', 'dog' ), 'apple,banana,carrot,dog' ), + array( array( '1', '2', 'apple', 'banana' ), '1,2,apple,banana' ), + array( array( '1', '2', 'apple', 'banana' ), '1, 2,apple,banana' ), + array( array( '1', '2', 'apple', 'banana' ), '1,2,apple,,banana' ), + array( array( '1', '2', 'apple', 'banana' ), ',1,2,apple,banana' ), + array( array( '1', '2', 'apple', 'banana' ), '1,2,apple,banana,' ), + array( array( '1', '2', 'apple', 'banana' ), '1,2 ,apple,banana' ), + array( array(), '' ), + array( array(), ',' ), + array( array(), ',,' ), + ); + } + /** * @dataProvider data_wp_parse_id_list */ diff --git a/tests/phpunit/tests/rest-api/rest-controller.php b/tests/phpunit/tests/rest-api/rest-controller.php index 8052cd4c14..6301e64d9d 100644 --- a/tests/phpunit/tests/rest-api/rest-controller.php +++ b/tests/phpunit/tests/rest-api/rest-controller.php @@ -203,7 +203,10 @@ class WP_Test_REST_Controller extends WP_Test_REST_TestCase { $this->assertEquals( 'a', $args['somedefault']['default'] ); } - public function test_get_fields_for_response() { + /** + * @dataProvider data_get_fields_for_response, + */ + public function test_get_fields_for_response( $param, $expected ) { $controller = new WP_REST_Test_Controller(); $request = new WP_REST_Request( 'GET', '/wp/v2/testroute' ); $fields = $controller->get_fields_for_response( $request ); @@ -221,14 +224,34 @@ class WP_Test_REST_Controller extends WP_Test_REST_TestCase { ), $fields ); - $request->set_param( '_fields', 'somestring,someinteger' ); + $request->set_param( '_fields', $param ); $fields = $controller->get_fields_for_response( $request ); - $this->assertEquals( + $this->assertEquals( $expected, $fields ); + } + + public function data_get_fields_for_response() { + return array( array( - 'somestring', - 'someinteger', + 'somestring,someinteger', + array( + 'somestring', + 'someinteger', + ), + ), + array( + ',,', + array( + 'somestring', + 'someinteger', + 'someboolean', + 'someurl', + 'somedate', + 'someemail', + 'someenum', + 'someargoptions', + 'somedefault', + ), ), - $fields ); } diff --git a/tests/phpunit/tests/rest-api/rest-schema-sanitization.php b/tests/phpunit/tests/rest-api/rest-schema-sanitization.php index b3a3a4966b..6dc08644d5 100644 --- a/tests/phpunit/tests/rest-api/rest-schema-sanitization.php +++ b/tests/phpunit/tests/rest-api/rest-schema-sanitization.php @@ -110,6 +110,7 @@ class WP_Test_REST_Schema_Sanitization extends WP_UnitTestCase { ); $this->assertEquals( array( 1, 2 ), rest_sanitize_value_from_schema( '1,2', $schema ) ); $this->assertEquals( array( 1, 2, 0 ), rest_sanitize_value_from_schema( '1,2,a', $schema ) ); + $this->assertEquals( array( 1, 2 ), rest_sanitize_value_from_schema( '1,2,', $schema ) ); } public function test_type_array_with_enum() { @@ -134,6 +135,7 @@ class WP_Test_REST_Schema_Sanitization extends WP_UnitTestCase { ); $this->assertEquals( array( 'ribs', 'chicken' ), rest_sanitize_value_from_schema( 'ribs,chicken', $schema ) ); $this->assertEquals( array( 'chicken', 'coleslaw' ), rest_sanitize_value_from_schema( 'chicken,coleslaw', $schema ) ); + $this->assertEquals( array( 'chicken', 'coleslaw' ), rest_sanitize_value_from_schema( 'chicken,coleslaw,', $schema ) ); } public function test_type_array_is_associative() { diff --git a/tests/phpunit/tests/rest-api/rest-schema-validation.php b/tests/phpunit/tests/rest-api/rest-schema-validation.php index 5cd31779a5..df16a894ad 100644 --- a/tests/phpunit/tests/rest-api/rest-schema-validation.php +++ b/tests/phpunit/tests/rest-api/rest-schema-validation.php @@ -120,6 +120,7 @@ class WP_Test_REST_Schema_Validation extends WP_UnitTestCase { ); $this->assertTrue( rest_validate_value_from_schema( array( 1 ), $schema ) ); $this->assertWPError( rest_validate_value_from_schema( array( true ), $schema ) ); + $this->assertWPError( rest_validate_value_from_schema( null, $schema ) ); } public function test_type_array_nested() { @@ -145,6 +146,8 @@ class WP_Test_REST_Schema_Validation extends WP_UnitTestCase { $this->assertTrue( rest_validate_value_from_schema( '1', $schema ) ); $this->assertTrue( rest_validate_value_from_schema( '1,2,3', $schema ) ); $this->assertWPError( rest_validate_value_from_schema( 'lol', $schema ) ); + $this->assertTrue( rest_validate_value_from_schema( '1,,', $schema ) ); + $this->assertTrue( rest_validate_value_from_schema( '', $schema ) ); } public function test_type_array_with_enum() { @@ -169,6 +172,8 @@ class WP_Test_REST_Schema_Validation extends WP_UnitTestCase { ); $this->assertTrue( rest_validate_value_from_schema( 'ribs,chicken', $schema ) ); $this->assertWPError( rest_validate_value_from_schema( 'chicken,coleslaw', $schema ) ); + $this->assertTrue( rest_validate_value_from_schema( 'ribs,chicken,', $schema ) ); + $this->assertTrue( rest_validate_value_from_schema( '', $schema ) ); } public function test_type_array_is_associative() {