Make WordPress Core

Opened 7 years ago

Closed 6 years ago

Last modified 5 years ago

#43977 closed defect (bug) (fixed)

Fix error-prone string to array parsing

Reported by: flixos90's profile flixos90 Owned by: flixos90's profile flixos90
Milestone: 5.1 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch has-unit-tests
Focuses: rest-api Cc:

Description

In the REST API (and sometimes in other areas of core too) it's a common procedure to parse a string into an array, splitting at comma characters. I noticed an issue that can easily lead to unexpected behavior when an empty value is passed.

Several functions in core use this behavior: $list = preg_split( '/[\s,]+/', $list );

However, it causes empty strings to be parsed into an array containing an empty string, where I would expect it to just be an empty array. In the REST API, this can cause strange behavior:

  • If you call wp/v2/posts/?include=, WordPress will search for posts with ID 0 (which won't change the response, but is still somewhat clunky).
  • If you call wp/v2/posts/?slug=, WordPress will search for posts with an empty slug (which won't change the response, but is still somewhat clunky).
  • More importantly though: If you call wp/v2/posts/?status=, you will get a strange error saying "Status is forbidden". That is because it validates the empty string in the parsed array against the enum whitelist, with that check obviously failing. It's not a required parameter, so in this case, it should instead just be ignored.

We should look for functions that perform the above parsing procedure and fix them.

Attachments (6)

43977.diff (5.4 KB) - added by sstoqnov 7 years ago.
Add PREG_SPLIT_NO_EMPTY flag to preg_split to avoid arrays with empty values
43977.2.diff (7.2 KB) - added by flixos90 6 years ago.
43977.3.diff (14.9 KB) - added by david.binda 6 years ago.
43977.4.diff (14.8 KB) - added by david.binda 6 years ago.
43977.5.diff (14.8 KB) - added by david.binda 6 years ago.
43977.5.2.diff (15.2 KB) - added by david.binda 6 years ago.

Download all attachments as: .zip

Change History (19)

@sstoqnov
7 years ago

Add PREG_SPLIT_NO_EMPTY flag to preg_split to avoid arrays with empty values

#1 @sstoqnov
7 years ago

  • Keywords needs-patch removed

If we pass PREG_SPLIT_NO_EMPTY to preg_split, it will remove the empty values. I ran few tests that confirmed that.

I will also try to provide unit tests soon.

#2 @flixos90
6 years ago

  • Keywords has-patch added

Thanks for the work on this @sstoqnov!

43977.2.diff is an update that outsources the relevant code into a new function: wp_parse_list() is similar to the existing wp_parse_id_list() and wp_parse_slug_list(), however without applying any additional sanitization on the list items.

The function contains the fix for preg_split() using the PREG_SPLIT_NO_EMPTY flag (good idea @sstoqnov!).

wp_parse_id_list() and wp_parse_slug_list() now internally call wp_parse_list().

All code using preg_split() in such a way was adjusted to either use wp_parse_list(), wp_parse_id_list() or wp_parse_slug_list(), according to what type the respective content is expected to have. The surrounding ! is_array() calls were also removed because the function already takes care of that.

Last edited 6 years ago by flixos90 (previous) (diff)

@flixos90
6 years ago

#3 @david.binda
6 years ago

Running the existing unit tests shows that this change breaks the status param handling in WP_Comment_Query::get_comment_ids covered by Tests_Comment_Query ::test_status_empty_string test.

The handling of empty string passed to the status param was broken previously by changes introduced in [30084] and then hotfixed via [30093].

However, the change also breaks another test, which I'm unsure how to get fixed, as I'm not sure whether the issue is with the test itself, or with the new code.

The WP_Test_REST_Settings_Controller::test_get_item_with_custom_array_setting is failing for the case when the option is non-existent. The current test expects null to be returned for a such setting, but the WP_REST_Settings_Controller::get_item function is casting schema's default null value to an array via WP_REST_Settings_Controller::prepare_value function. The current functionality, for not casting the null to an array is only possible due to the described "unexpected" behaviour of the preg_split call without PREG_SPLIT_NO_EMPTY flag set.

I'm attaching an updated patch which fixes the broken empty status param handling and which adds unit tests covering the new function and extending existing unit tests for covering affected places in the code

@david.binda
6 years ago

#4 @david.binda
6 years ago

Examining the implementation of the WP_REST_Settings_Controller::get_item and WP_REST_Settings_Controller::prepare_value further, it looks like not attempting to parse null by wp_parse_list inside the rest_validate_value_from_schema function should do the trick for preventing current implementation, which feels correct after all.

I'm attaching enhanced diff.

@david.binda
6 years ago

#5 @david.binda
6 years ago

Adding one more unit test for the null passed to rest_validate_value_from_schema function when the schema expects an array. This unit test is passing prior and after those changes.

There are no more unit test issues with this patch now.

@david.binda
6 years ago

#6 @david.binda
6 years ago

I've originally uploaded wrong patch, thus the 43977.5.2.diff :)

#7 @flixos90
6 years ago

  • Milestone changed from 5.0 to 5.1

This ticket was mentioned in Slack in #core-restapi by desrosj. View the logs.


6 years ago

#9 @desrosj
6 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#10 @flixos90
6 years ago

  • Owner set to flixos90
  • Resolution set to fixed
  • Status changed from new to closed

In 44546:

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.

#11 @flixos90
6 years ago

In 44547:

General: Fix existing coding standard violations in bookmark tests, as outlined in [44546].

See #43977.

#12 @keesiemeijer
6 years ago

Similar #44212

Last edited 6 years ago by keesiemeijer (previous) (diff)

#13 @TimothyBlynJacobs
5 years ago

#44304 was marked as a duplicate.

Note: See TracTickets for help on using tickets.