WordPress.org

Make WordPress Core

Opened 8 months ago

Closed 3 months ago

Last modified 3 months ago

#48818 closed defect (bug) (fixed)

REST API does not check nested required properties

Reported by: TimothyBlynJacobs Owned by: TimothyBlynJacobs
Milestone: 5.5 Priority: normal
Severity: normal Version: 4.9
Component: REST API Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

WP_REST_Server checks if a request is valid using WP_REST_Request::has_valid_params. That function first checks if all required parameters are supplied. A parameter is required if it has the required property specified. The REST API only checks for the existence of the top-level properties. Nested properties are not checked.

For example, given the following schema:

{
  "type": "object",
  "properties": {
    "my_obj": {
      "type": "object",
      "required": true,
      "properties": {
        "my_prop": {
          "type": "string",
          "required": true
        }
      }
    },
    "my_arr": {
      "type": "array",
      "required": true,
      "items": {
        "type": "object",
        "properties": {
          "my_prop": {
            "type": "string",
            "required": true
          }
        }
      }
    }
  }
}

The following data is valid.

{
  "my_obj": {
    "other_prop": "hi"
  },
  "my_arr": [
    {
      "other_prop": "hi"
    }
  ]
}

We could fix this in either WP_REST_Request::has_valid_params or rest_validate_value_from_schema.

Doing it in the request object would have the benefits of consistency with top-level required properties. It would also allow checking all required properties before checking for validity and returning it in one WP_Error object. However, there could be a BC issue if a user has specified a schema using required for child parameters, but then provided a custom validate_callback that didn't actually check that the parameter is required.

If it is in rest_validate_value_from_schema we wouldn't have the BC issue. It'd also be easier to implement since that function already recurses.

We'd also want to make sure that if a parent property isn't required, that child properties aren't required if the parent value is missing entirely. For instance given the following schema, an empty request object should be valid, but {my_prop: {}} wouldn't be.

{
  "type": "object",
  "properties": {
    "my_obj": {
      "type": "object",
      "properties": {
        "my_prop": {
          "type": "string",
          "required": true
        }
      }
    }
  }
}

Change History (13)

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


8 months ago

#2 @circuitbit
7 months ago

Just thought it would be important to mention that according to JSON-Schema spec it's also possible to have a required field as an array of strings containing the properties which are required for that object, so that would be, sort of, expected.

$schema = array(
   'type'       => 'object',
   'properties' => array(
      'first_name' => array( 'type' => 'string' ),
      'last_name'  => array( 'type' => 'string' ),
      'email'      => array( 'type' => 'string' ),
   ),
   'required'   => array( 'first_name', 'last_name', 'email' ),
);

#3 @TimothyBlynJacobs
7 months ago

@circuitbit this is a great point. In WordPress we use v4 of the JSON Schema except for required where we use the v3 syntax. This syntax is nice to work with for core because we do a lot of conditional adding/removing of properties as well as the addition of new properties through things like additional properties and metadata. The simpler boolean syntax allows the requiredness of the property to be entirely communicated through one array definition instead of having to modify the base schema.

That being said,. I do think supporting the array format would be quite nice, particularly for cases where you are documenting a single object property on a schema, as opposed to the schema for the entire resource. I think we could add support for that syntax in the patch for this feature.

#4 @TimothyBlynJacobs
5 months ago

  • Milestone changed from Awaiting Review to 5.5

#5 @sorenbronsted
4 months ago

I will work on this.

This ticket was mentioned in PR #225 on WordPress/wordpress-develop by sorenbronsted.


4 months ago

I have added the required schema check for version 3 and 4 in rest_validate_value_from_schema.

Trac ticket: https://core.trac.wordpress.org/ticket/48818

#7 @sorenbronsted
4 months ago

  • Keywords has-patch has-unit-tests added

#8 @prbot
4 months ago

TimothyBJacobs commented on PR #225:

@sorenbronsted thanks for the patch! Could you rebase this so we only have the commits for your changes?

This ticket was mentioned in PR #234 on WordPress/wordpress-develop by sorenbronsted.


4 months ago

A rebased version of the previous 48818 PR

Trac ticket: https://core.trac.wordpress.org/ticket/48818

This ticket was mentioned in PR #244 on WordPress/wordpress-develop by sorenbronsted.


3 months ago

Fixed according to TimothyBJacobs

Trac ticket: https://core.trac.wordpress.org/ticket/48818

This ticket was mentioned in PR #262 on WordPress/wordpress-develop by TimothyBJacobs.


3 months ago

Previously, the WP_REST_Request object validated that top-level properties were defined, but this did not extend to those object's required properties. This adds validation to rest_validate_value_from_schema() directly.

Both the v3 and v4 JSON Schema syntax for required properties is supported.

Props sorenbronsted.
Fixes #48818.

Trac ticket: https://core.trac.wordpress.org/ticket/48818

#12 @TimothyBlynJacobs
3 months ago

  • Owner set to TimothyBlynJacobs
  • Resolution set to fixed
  • Status changed from new to closed

In 47809:

REST API: Check required properties are provided when validating an object.

Previously, the WP_REST_Request object validated that top-level properties were defined, but this did not extend to those object's required properties. This adds validation to rest_validate_value_from_schema() directly.

Both the v3 and v4 JSON Schema syntax for required properties is supported.

Props sorenbronsted.
Fixes #48818.

#13 @TimothyBlynJacobs
3 months ago

In 47811:

REST API: Add @since entries for rest_validate_value_from_schema().

See #49572, #48818, #44949, #50053, #48820, #49720, #42961, #44975, #43392, #38583.

Note: See TracTickets for help on using tickets.