#43977 closed defect (bug) (fixed)
Fix error-prone string to array parsing
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (19)
#1
@
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
@
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.
#3
@
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
#4
@
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.
#5
@
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.
Add PREG_SPLIT_NO_EMPTY flag to preg_split to avoid arrays with empty values