Make WordPress Core

Opened 10 years ago

Closed 10 years ago

#38420 closed defect (bug) (fixed)

API Post status parameter does not accept multiple values

Reported by: kadamwhite Owned by: joehoyle
Priority: normal Milestone: 4.7
Component: REST API Version: 4.7
Severity: normal Keywords: has-patch has-unit-tests
Cc: Focuses:

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)

38420.diff (1.4 KB ) - added by kadamwhite 10 years ago.
Patch to permit multiple status values to be passed when querying the posts collection endpoint
38420.2.diff (3.6 KB ) - added by kadamwhite 10 years ago.
Adds unit tests
38420.3.diff (5.4 KB ) - added by websupporter 10 years ago.
38420.4.diff (5.4 KB ) - added by kadamwhite 10 years ago.
Refreshed patch to apply cleanly, and fixed test name in the process
38420.5.diff (13.6 KB ) - added by jnylen0 10 years ago.
Integrated the changes here with my patch from #38580
38420.6.diff (13.4 KB ) - added by jnylen0 10 years ago.
Send entire items array in get_data_for_route response

Download all attachments as: .zip

Change History (20)

@kadamwhite
10 years ago

Patch to permit multiple status values to be passed when querying the posts collection endpoint

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


10 years ago

@kadamwhite
10 years ago

Adds unit tests

#2 @joehoyle
10 years 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
10 years 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.


10 years ago

@kadamwhite
10 years ago

Refreshed patch to apply cleanly, and fixed test name in the process

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


10 years ago

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


10 years ago

#7 @jnylen0
10 years ago

#38580 was marked as a duplicate.

@jnylen0
10 years ago

Integrated the changes here with my patch from #38580

#8 @jnylen0
10 years 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.

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.

Version 0, edited 10 years ago by jnylen0 (next)

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


10 years ago

#10 @rachelbaker
10 years ago

  • Milestone Awaiting Review4.7

This ticket was mentioned in Slack in #core by jnylen. View the logs.


10 years ago

#12 @joehoyle
10 years ago

  • Owner set to joehoyle
  • Status newassigned

@jnylen0
10 years ago

Send entire items array in get_data_for_route response

#13 @jnylen0
10 years ago

Per Slack discussion, 38420.6.diff sends the entire items array as part of the response. Bonus: no more array autovivification.

#14 @joehoyle
10 years ago

  • Resolutionfixed
  • Status assignedclosed

In 39104:

REST API: Support querying for multiple post statuses.

Multiple post statuses can be specified by the usual CSV or array-propper format.

Props jnylen0, kadamwhite, websupporter.
Fixes #38420.

Note: See TracTickets for help on using tickets.