Opened 8 years ago
Closed 8 years ago
#38531 closed enhancement (fixed)
Support for arrays in schema validation and sanitization
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (37)
#2
@
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. :-)
@
8 years ago
Removed var_dump()
uses and fixed typo in the header of tests/phpunit/tests/rest-api/rest-schema-validation.php
#3
@
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.
#7
follow-up:
↓ 19
@
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
@
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
@
8 years ago
- Keywords 2nd-opinion added; has-patch has-unit-tests commit removed
- Resolution fixed deleted
- Status changed from closed to reopened
#12
@
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
@
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
This ticket was mentioned in Slack in #core-restapi by tharsheblows. View the logs.
8 years ago
#15
@
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.
#16
@
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
@
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.
This ticket was mentioned in Slack in #core by helen. View the logs.
8 years ago
#19
in reply to:
↑ 7
@
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 specifytype
=>array
, and therefor get no sanitization.
Successful validation of an array instance with regards to [
additionalItems
anditems
] 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
@
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:
- Using the new validation for meta as well.
array
allows JSON objects currently.
Correct me if I'm wrong and there's something I'm missing here.
#21
@
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
#22
@
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 tostring
fortype => 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
andrest_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 userest_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
#26
@
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 ascontent
on Posts.
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.