Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#38531 closed enhancement (fixed)

Support for arrays in schema validation and sanitization

Reported by: joehoyle's profile joehoyle Owned by: joehoyle's profile joehoyle
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: REST API Keywords: 2nd-opinion
Focuses: Cc:

Description

Leading into https://github.com/WP-API/WP-API/issues/2859, the main thing to solve here is sanitization / validation support for arrays. If you specify type => array in endpoint args, then an additional items => array( type => string ) is required to specify the schema for the items in the array.

I've implemented a patch to support arrays (and easily objects in the future) by abstracting our schema validation / sanitization a little to supper them being called recursively, which means array validation is quite simple.

Also, I added a good amount of tests for exactly what happens for each type / format in validation.

There was a few leaks that I uncovered when doing this, such as Settings allowing null values, and some places where we were specify type => array in endpoint args, but were not defined the items property. This means we'll get a little better validation off the bat. A couple of other fixes came along with this such as numeric => number typo for min / max support, sanitization for type => number which we had previously missed.

I'd like to get some eyes on this from @rachebaker and @rmccue, this was a little simpler than I had though, so I think we might be able to get this in 4.7.

Once this is in, it will just be a case of allowing type => array to the settings and meta endpoints. Also, this operates at the endpiont args level, so type => array is also supported in collection params.

Attachments (9)

38531.diff (20.2 KB) - added by joehoyle 8 years ago.
38531.2.diff (20.0 KB) - added by pento 8 years ago.
38531.3.diff (15.5 KB) - added by rachelbaker 8 years ago.
Removed var_dump() uses and fixed typo in the header of tests/phpunit/tests/rest-api/rest-schema-validation.php
38531.4.diff (8.1 KB) - added by tharsheblows 8 years ago.
add in support for meta values
38531.5.diff (3.1 KB) - added by tharsheblows 8 years ago.
unit tests for assoc arrays, unknown types and no types in schemas
38531.6.diff (6.3 KB) - added by tharsheblows 8 years ago.
unit tests with post meta and fewer typos
38531.7.diff (8.9 KB) - added by joehoyle 8 years ago.
38531.8.diff (12.4 KB) - added by joehoyle 8 years ago.
38531.9.diff (15.8 KB) - added by tharsheblows 8 years ago.
fix custom schema update, unit tests for that

Download all attachments as: .zip

Change History (37)

@joehoyle
8 years ago

#1 @ChopinBach
8 years ago

This should go into 4.7, minus the var_dump() lines. This is a big step forward for the way the validate/sanitize callbacks are being implemented currently.

@pento
8 years ago

#2 @pento
8 years ago

  • Keywords has-patch has-unit-tests 2nd-opinion added

Oh, this is nice.

I saw there was a bit of discussion around adding CSV and object support here, I'm inclined to bring in this patch as is, then we can have new tickets for adding to it.

38531.2.diff just does a little bit of tidying. I like the look of this change, but I'd like a second opinion from @rachelbaker or @rmccue first. :-)

@rachelbaker
8 years ago

Removed var_dump() uses and fixed typo in the header of tests/phpunit/tests/rest-api/rest-schema-validation.php

#3 @rachelbaker
8 years ago

  • Keywords commit added; 2nd-opinion removed

@pento Looks good to me. I did a little more clean-up in 38531.3.diff that removes the var_dump() debugging, fixes a typo, and some docblock spacing.

#4 @rachelbaker
8 years ago

I created #38583 for handling objects in our schema.

#5 @pento
8 years ago

  • Owner set to pento
  • Status changed from new to assigned

#6 @pento
8 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 39046:

REST API: Add support for arrays in schema validation and sanitization.

By allowing more fine-grained validation and sanitisation of endpoint args, we can ensure the correct data is being passed to endpoints.

This can easily be extended to support new data types, such as CSV fields or objects.

Props joehoyle, rachelbaker, pento.
Fixes #38531.

#7 follow-up: @joehoyle
8 years ago

In the latest patch I think from @rachelbaker we are skipping sanitization if nothing is specified in the items property:

if ( empty( $args['items'] ) ) { 
    return (array) $value; 
}

This means that we are "passing thru by default" which I think is a bad idea for a sanitizing function. IMO if the items is not present we should return an empty, and maybe fire a doing_it_wrong. With this, it would be easy (and some places in the rest api already do this) to specify type => array, and therefor get no sanitization.

Looking for thoughts from @rachelbaker :)

#8 @TimothyBlynJacobs
8 years ago

Doing it this way precludes the function from ever supporting things like $ref which would be immensely useful. Its not too difficult to add, but it would require the full schema to work off of.

#9 @pento
8 years ago

  • Keywords 2nd-opinion added; has-patch has-unit-tests commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

#10 @pento
8 years ago

  • Owner changed from pento to rachelbaker
  • Status changed from reopened to assigned

#11 @rmccue
8 years ago

  • Type changed from defect (bug) to enhancement

#12 @joehoyle
8 years ago

Ping @rachelbaker, I wanted to know your thoughts on the "default to no sanitization", also if there's anything else that needs doing on this ticket?

#13 @tharsheblows
8 years ago

If you want to use rest_validate_value_from_schema for meta values, empty values need to be ok as they're included in the response.

Attached is that change with meta using rest_validate_value_from_schema and rest_sanitize_value_from_schema and allowing arrays, including multiple meta keys with type='array'. The allowed types are whitelisted in WP_REST_Meta_Fields so it fixes this too https://github.com/WP-API/WP-API/issues/2798

@tharsheblows
8 years ago

add in support for meta values

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


8 years ago

#15 @tharsheblows
8 years ago

Unit tests showing unknown types falling through validation and sanitization. nb: I'm not exactly sure what the sanitization return value should be and have not included 'type' => 'object' because I'm not sure how that should be handled.

Currently validating / sanitizing php associative arrays work with a schema 'type' => 'array' even though they're json objects. Not sure if that's an issue or will be confusing but thought I'd mention.

@tharsheblows
8 years ago

unit tests for assoc arrays, unknown types and no types in schemas

#16 @joehoyle
8 years ago

Currently validating / sanitizing php associative arrays work with a schema 'type' => 'array' even though they're json objects. Not sure if that's an issue or will be confusing but thought I'd mention.

Ahh, I think we should error on this case. I'll add a patch. If you send a JSON object for an array, that is. I think we have some kind of wp_is_numeric_array() function somewhere.

#17 @tharsheblows
8 years ago

Attached are unit tests with post meta tests included - they work slightly differently so need separate tests.

In test_type_array_is_associative the schema isn't correct for what the value is but it's an error people might make and sanitization and validation aren't working correctly with that incorrect type.

Use this patch for unit tests instead of the previous one as I corrected some other bits too.

@tharsheblows
8 years ago

unit tests with post meta and fewer typos

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


8 years ago

#19 in reply to: ↑ 7 @rmccue
8 years ago

Replying to joehoyle:

This means that we are "passing thru by default" which I think is a bad idea for a sanitizing function. IMO if the items is not present we should return an empty, and maybe fire a doing_it_wrong. With this, it would be easy (and some places in the rest api already do this) to specify type => array, and therefor get no sanitization.

Per the JSON Schema spec:

Successful validation of an array instance with regards to [additionalItems and items] is determined as follows:

  • if "items" is not present, or its value is an object, validation of the instance always succeeds, regardless of the value of "additionalItems";
  • [...]
  • if the value of "additionalItems" is boolean value false and the value of "items" is an array, the instance is valid if its size is less than, or equal to, the size of "items".

If either keyword is absent, it may be considered present with an empty schema.

Hence, I believe the default behaviour here is correct. It would only fail if items was present, an array, and also additionalItems was set to false.

#20 @rmccue
8 years ago

  • Owner changed from rachelbaker to joehoyle

This needs shepherding, and final changes here. It appears there are two things left to handle in this ticket:

  1. Using the new validation for meta as well.
  2. array allows JSON objects currently.

Correct me if I'm wrong and there's something I'm missing here.

#21 @jnylen0
8 years ago

I believe the default behaviour here is correct.

In that case, we should probably add an item type to the few array args/properties that don't have one:

array arg without item type:
  /wp/v2/posts
  slug
  {"required":false,"description":"Limit result set to posts with one or more specific slugs.","type":"array"}

array arg without item type:
  /wp/v2/pages
  slug
  {"required":false,"description":"Limit result set to posts with one or more specific slugs.","type":"array"}

array arg without item type:
  /wp/v2/media
  slug
  {"required":false,"description":"Limit result set to posts with one or more specific slugs.","type":"array"}

array property in schema without item type:
  /wp/v2/taxonomies
  capabilities
  {"description":"All capabilities used by the resource.","type":"array","context":["edit"],"readonly":true}

array property in schema without item type:
  /wp/v2/taxonomies/(?P<taxonomy>[\w-]+)
  capabilities
  {"description":"All capabilities used by the resource.","type":"array","context":["edit"],"readonly":true}

Source: https://gist.github.com/nylen/9021f6d19157a023a0a8a607a3adb28a

@joehoyle
8 years ago

#22 @joehoyle
8 years ago

Added a patch to address the following:

  • rest_validate_value_from_schema should fail if passed assoc. arrays, as this means it was an object in JSON.
  • rest_sanitize_value_from_schema will now convert arrays to numeric arrays, to not allow unwanted data to slip in via the keys.
  • rest_sanitize_value_from_schema will now cast data to string for type => string.
  • Meta fields are now properly excluded if they are not registered with a valid type.
  • Meta fields' type is now a fall back to the registered meta show_in_rest's type. Seems this was an oversight.
  • Updated tests for meta to include the type in registration.
  • Schemas with unknown types will pass through rest_validate_value_from_schema and rest_sanitize_value_from_schema unchanged. As this is a developer decision, we ideally want to support using different types, whereby developers implement their own sanitization for those. Because we use rest_parse_request_arg in the case of _any_ type being supplied, I think this makes the most sense.

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


8 years ago

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


8 years ago

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


8 years ago

@joehoyle
8 years ago

#26 @joehoyle
8 years ago

Updated patch with:

  • Unit tests for validation and sanitization of post meta fields.
  • WP_REST_Meta_Fields will now validate and sanitize meta values from the schema.
  • WP_REST_Meta_Fields::get_value will now return an array rather than object. This is consistent with other "objects" in API responses, such as content on Posts.

@tharsheblows
8 years ago

fix custom schema update, unit tests for that

#27 @tharsheblows
8 years ago

Custom schema updates were not validating, latest patch fixes that and adds unit tests

#28 @joehoyle
8 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 39222:

REST API: Validate and Sanitize registered meta based off the schema.

With the addition of Array support in our schema validation functions, it's now possible to use these in the meta validation and sanitization steps. Also, this increases the test coverage of using registered via meta the API significantly.

Fixes #38531.
Props rachelbaker, tharsheblows.

Note: See TracTickets for help on using tickets.