WordPress.org

Make WordPress Core

Opened 12 months ago

Last modified 20 hours ago

#38583 reopened enhancement

Support for objects in schema validation and sanitization

Reported by: rachelbaker Owned by: rmccue
Milestone: 4.9 Priority: high
Severity: major Version: 4.7
Component: REST API Keywords: has-unit-tests needs-patch
Focuses: Cc:

Description

Follow-up on #38531 and support sanitizing and validating objects with our schema. This would allow our Meta and Settings endpoints to accept non-scalar values.
See https://github.com/WP-API/WP-API/issues/2859

Attachments (5)

38583.1.diff (13.0 KB) - added by TimothyBlynJacobs 8 weeks ago.
38583.2.diff (26.8 KB) - added by TimothyBlynJacobs 8 weeks ago.
38583.3.diff (8.7 KB) - added by joehoyle 2 weeks ago.
38583.4.diff (9.4 KB) - added by joehoyle 2 weeks ago.
Add support for stdClass
38583-settings.diff (10.9 KB) - added by joehoyle 2 weeks ago.

Download all attachments as: .zip

Change History (32)

#2 @jason_the_adams
4 months ago

Looking forward to seeing this in a release. So this is waiting for sanitization support, currently?

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


4 months ago

#4 @TimothyBlynJacobs
8 weeks ago

  • Keywords has-patch has-unit-tests dev-feedback added; needs-patch removed

I've added two separate versions.

1) Simply adds object validation. For the properties like content or title which can accept either a structured object or a string, the validation_callback is set to null ( matching sanitization ).

2) Adds object validation and adds support for multiple type options. So content for instance becomes type => [ 'object', 'string'].

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


7 weeks ago

#6 @rmccue
7 weeks ago

  • Milestone changed from Future Release to 4.9
  • Owner set to rmccue
  • Status changed from new to reviewing
  • Type changed from defect (bug) to enhancement
  • Version set to 4.7

#7 @westonruter
4 weeks ago

  • Priority changed from normal to high

Bumping priority to high for visibility and alignment with 4.9 goals, and given proximity to beta 1 deadline.

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


3 weeks ago

#9 @kadamwhite
2 weeks ago

@joehoyle I think you'd be in the best position to review this, do you have bandwidth to do that today so we can consider this for 4.9?

#10 @joehoyle
2 weeks ago

38583.1.diff is looking good to me, I don't think it would be right to go with 38583.2.diff at this point. It could be considered after adding basic object support, but the two should be separate. Thanks for all the work on it though, that's a big patch!

For 38583.1.diff this looks mostly good. I think we shouldn't support additionalProperties for now, the point of the schema is to whitelist data and types into the REST API (and out), so I'm not sure I see a long term path for it; but again, let's get basic object support before putting a nail in that coffin.

I can clean up 38583.1.diff to remove those, test etc.

We also ideally need to look at register_meta / register_setting to make sure they also handle the extra types, else this is only useful to register_rest_field and custom controllers / endpoints.

@joehoyle
2 weeks ago

#11 @joehoyle
2 weeks ago

Added new patch working off 38583.1.diff:

  • Removed support for additionalProperties
  • Fixed sanitize -> validate in comments
  • Will now remove / ignore any properties that are not included in the registered properties. We follow this pattern mostly around the API where over-providing data is just silently ignored.

@joehoyle
2 weeks ago

Add support for stdClass

#12 @joehoyle
2 weeks ago

In 41727:

REST API: Support for objects in schema validation and sanitization.

When registering routes developers can now define their complex objects in the schema and benefit from the automatic validation and sanitization in the REST API. This also paves the way for support for complex object registration via register_meta and register_setting.

See #38583.
Props TimothyBlynJacobs5.

#13 @joehoyle
2 weeks ago

I'm going to keep this ticket up and do a follow-up patch to add support for register_meta and register_setting.

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


2 weeks ago

#15 @joehoyle
2 weeks ago

Added a patch for enabling use of objects in register_setting. This is essentially whitelisting the added type in the settings controller, and also now passes properties from the arg registration into the validation functions.

#16 @kadamwhite
2 weeks ago

In 41758:

REST API: Support objects in settings schema.

Enables register_setting to accept an object as its schema value, allowing settings to accept non-scalar values through the REST API.
This whitelists the added type in the settings controller, and passes properties from argument registration into the validation functions.

Props joehoyle.
See #38583.

#17 @kadamwhite
2 weeks ago

Leaving this open in case we want to continue to use this ticket for register_meta, but defer to @joehoyle on whether that should at this point be broken out.

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


2 weeks ago

#19 @westonruter
2 weeks ago

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

Let's resolve this as fixed and open a new task/defect for any necessary change for register_meta().

#20 @mnelson4
2 weeks ago

This is a nice feature we will definetely use in our plugin!
Unfortunately, the only reason I learned about it was because it broke our plugin's unit tests! :)

Our custom REST API endpoints accept a query parameterof type "object" which has a pretty complex structure, so we were performing our validation on it later in the request. However, this change now has validation kicking in early and wiping the object right away. So, our previous valid query params are now being marked as invalid.

eg, this is the info in the WP REST API index page about the endpoint

"/ee/v4.8.36/datetimes": {
            "namespace": "ee/v4.8.36",
            "methods": [
                "GET",
                "POST"
            ],
            "endpoints": [
                {
                    "methods": [
                        "GET"
                    ],
                    "args": {
                        "where": {
                            "required": false,
                            "default": [],
                            "type": "object"
                        },
...

A request like mysite.com/wp-json/ee/v4.8.36/datetimes?where[DTT_EVT_start]=2017-01-01T00:00:00 used to only show the datetimes where their DTT_EVT_start property matched January 1st 2017, but this new validation removes that condition because it only considers an empty object to be a valid value for the where arg.

I'm not sure if other plugin developers or users with custom endpoints will experience the same issue.

--- I removed my code suggestion. I now think @TimothyBlynJacobs 's original approach, to have an additionalProperties argument, is best.

Last edited 8 days ago by mnelson4 (previous) (diff)

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


9 days ago

#22 @mnelson4
8 days ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I'm sorry if this isn't the right way to do it, but I reopened this ticket. In slack @TimothyBlynJacobs mentioned

Yeah, the commit for that needs massaging for unlisted properties. The current version in core is the direct opposite of the spec and has the potential to break other code

I found this article that gives a fairly simple explanation of the spec. It says

The additionalProperties keyword is used to control the handling of extra stuff, that is, properties whose names are not listed in the properties keyword. By default any additional properties are allowed.

The additionalProperties keyword may be either a boolean or an object. If additionalProperties is a boolean and set to false, no additional properties will be allowed.

ie, the default should be to ALLOW additional properties, not exclude them.

@TimothyBlynJacobs's original diff had that behaviour (see lines 1071 and 1210) but @joehoyle decided to not include it saying

I think we shouldn't support additionalProperties for now, the point of the schema is to whitelist data and types into the REST API (and out), so I'm not sure I see a long term path for it; but again, let's get basic object support before putting a nail in that coffin.

I appreciate @joehoyle is trying to not add anything unnecessary on this commit. But if additionalProperties is not supported now, and non-whitelised properties are removed, then IMO it does put a nail in the coffin: it goes against the JSON schema specification and breaks backward compatibility. And if we were to later decide we do want to follow the JSON schema specification (namely, allow additional properties by default), we'd have to make another breaking change in order to do that.

So if we don't want to support the additionProperties argument yet, the default should at least be to default to ALLOW non-whitelisted properties.

Thoughts?

#23 @TimothyBlynJacobs
8 days ago

I strongly think that we should support additionalProperties. It, along with patternProperties allows for very useful functionality. I originally only included additionalProperties for the exact reason @mnelson4 mentioned, patternProperties is something that could easily be included later.

Additionally, if we want to whitelist object keys for use in things like meta or settings, then it would be far more preferable to merge the setting schema entry with defaults for the object data type that would contain 'additionalProperties' => false. This way custom endpoints would work properly that already have object validation and you'd have the increased "security" for settings or meta.

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


27 hours ago

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


24 hours ago

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


20 hours ago

#27 @westonruter
20 hours ago

  • Keywords needs-patch added; has-patch dev-feedback removed
  • Severity changed from normal to major

I agree we need to retain this behavior:

ie, the default should be to ALLOW additional properties, not exclude them.

I've had a conversation previously about how adding additionalProperties: true to a schema is good just to be explicit, even though it is the default. If the behavior changes where this is no longer the default, things will break.

Increasing severity as it conflicts with JSON Schema spec and breaks back-compat.

Note: See TracTickets for help on using tickets.