Opened 2 months ago
Closed 7 weeks ago
#38420 closed defect (bug) (fixed)
API Post status parameter does not accept multiple values
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Milestone: | 4.7 | Priority: | normal |
| Severity: | normal | Version: | 4.7 |
| Component: | REST API | Keywords: | has-patch has-unit-tests |
| Focuses: | Cc: |
Description
In the schema for the posts parameter we specify, "Limit result set to posts assigned a specific status; can be comma-delimited list of status types." However, the actual sanitization function we are using is sanitize_key, which does not properly parse array or comma-delimited values. This improper sanitization contributes to #38417
The change in the attached path switches this parameter to use wp_parse_slug_list to properly interpret and sanitize arrays of stati, whether provided comma,separated or status[]=array&status[]=syntax (or plain string values).
I'm not sure how to best update the validation function to handle this input.
Attachments (6)
Change History (20)
@kadamwhite
2 months ago
This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.
2 months ago
#2
@joehoyle
2 months ago
@kadamwhite if we are going to switch to an array, we need to remove the enum addition here, as the two are not compatible. However, we need need to then validate them somewhere else.
It's worth mentioning, this would be the best thing for us to support:
<?php ... array( 'type' => 'array', 'items' => array( 'enum' => array( 'publish', .... ) ) ) ...
#3
@websupporter
2 months ago
38420.3.diff extends the patch for validation.
In order to follow @joehoyle suggestion, I did also update the class-wp-rest-server.php to whitelist items. I am not quite sure, which toughts are involved in the whitelisting. I was rather strict and did only whiteliste [items][type] and [items][enum].
<?php ... array( 'type' => 'array', 'items' => array( 'enum' => array( 'publish', .... ), 'type' => 'string', ) ) ...
The validate_user_can_query_private_statuses() is extended like in https://github.com/danielbachhuber/wordpress-develop/pull/4 (return rest_validate_request_arg( $value, $request, $param );)
I added the type because I was not quite sure, if there could be a list of integer enums. Right now, we do not have this, but can enums by definition contain numbers? In this case, we could run into a problem like the rest_is_boolean() problem, so I wanted to restrict the current array-enum-handling only to strings.
This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.
2 months ago
This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.
2 months ago
This ticket was mentioned in Slack in #core-restapi by jnylen. View the logs.
8 weeks ago
#8
@jnylen0
7 weeks ago
- Keywords has-patch has-unit-tests added
I added the type because I was not quite sure if there could be a list of integer enums
I've kept this change, and also added the type parameter for all arguments in the namespace index responses because there is no other way to tell that the status parameter accepts a list of values when listing posts (in the item schema exposed by ?context=help, status is just a string). Note this is a larger change, but I think it is the right thing to do.
I did not add the default enum validation from 38420.4.diff ("Handle enum arrays") because this is a bit of a special case and it needs custom validation logic anyway. I'm not sure if we should go ahead and add automatic enum array validation here or not. Edit: Scratch that bit, I got mixed up while writing this. The reason I didn't include the validation logic from the previous patch is that it already works in trunk - see test_type_array_with_enum in the latest patch.
Since the last patch(es) I've also fixed this change for media handling, added all the tests I could think of, and ran through the logic on a couple of test sites.
This ticket was mentioned in Slack in #core-restapi by jnylen. View the logs.
7 weeks ago
#10
@rachelbaker
7 weeks ago
- Milestone changed from Awaiting Review to 4.7
This ticket was mentioned in Slack in #core by jnylen. View the logs.
7 weeks ago
#13
@jnylen0
7 weeks ago
Per Slack discussion, 38420.6.diff sends the entire items array as part of the response. Bonus: no more array autovivification.
Patch to permit multiple status values to be passed when querying the posts collection endpoint