WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 5 months ago

Last modified 5 months ago

#38989 closed defect (bug) (fixed)

Invalid meta causes warning in WP_Test_REST_Post_Meta_Fields

Reported by: timmydcrawford Owned by: rachelbaker
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch has-unit-tests commit dev-reviewed
Focuses: Cc:

Description

While testing the terms endpoints I was trying to figure out how to properly submit meta values to the API through trial and error, and noticed that if the meta input is not an array, a warning is thrown in the logs:

[29-Nov-2016 23:48:58 UTC] PHP Warning:  array_key_exists() expects parameter 2 to be array, string given in /srv/www/wordpress-develop/src/wp-includes/rest-api/fields/class-wp-rest-meta-fields.php on line 128

We fixed a similar issue in #38553 but I don't believe we can utilize a similar fix here, so in the attached diff I opted to just check that the value is an array. Additionally, the doc block for update_value currently says the first attribute is a WP_REST_Request object, when it is actually the meta argument from the request.

I couldn't find much documentation on meta in the terms endpoints, so I also opted to add some test coverage on the subject as well.

Attachments (6)

fix-meta-warnings.diff (4.3 KB) - added by timmydcrawford 5 months ago.
38989.2.diff (5.6 KB) - added by rachelbaker 5 months ago.
Moves the meta array check into WP_REST_Request->has_valid_params() and includes unit tests
38989.3.diff (4.7 KB) - added by rachelbaker 5 months ago.
Ignore the meta property in a request if it is not an array before trying to update meta
38989.4.diff (5.9 KB) - added by jnylen0 5 months ago.
Use sanitize_callback to verify that meta is an array
38989.5.2.diff (5.6 KB) - added by rachelbaker 5 months ago.
38989.5.diff (5.6 KB) - added by rachelbaker 5 months ago.
Use the validate_callback and renamed the callback method check_meta_is_array()

Download all attachments as: .zip

Change History (21)

#1 @jnylen0
5 months ago

  • Milestone changed from Awaiting Review to 4.7

Thanks for the fix, nice catch on $request vs $meta as well!

  • I think passing a non-array for meta should return an error.
  • Can you add a test for passing a non-array also?

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


5 months ago

#3 @rachelbaker
5 months ago

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

#4 @rachelbaker
5 months ago

  • Keywords needs-refresh has-patch added

A few comments on the proposed solution in fix-meta-warnings.diff:

  1. Checking the meta value type so far down in the request processing means that if I am updating a full object (as in the title and content for a post IN ADDITION to the meta values) the post's title and content would be updated on the post object, but I would receive a WP_Error for my request. We should do this check near the beginning of the create_item() and update_item() methods in the object controllers.
  1. We cannot create a new error message because we are in string freeze for 4.7. Instead, the WP_Error message should match the strings we use with the rest_invalid_param errors.
  1. The unit tests included by @timmydcrawford do not appear to be testing anything in the scope of this ticket. I agree with @jnylen0 that we should unit tests for sending meta values that are "invalid" in a request, but we should also move the unrelated tests to a new ticket. @timmydcrawford would you be able to create a new ticket with your tests?

@rachelbaker
5 months ago

Moves the meta array check into WP_REST_Request->has_valid_params() and includes unit tests

#5 @rachelbaker
5 months ago

  • Keywords has-unit-tests added; needs-refresh removed

@joehoyle What do you think about my approach in 38989.2.diff? There is a limitation in that any request with a meta property now has to be an array. The benefit is that we don't have to add is_array checks in 8 different methods and remember to include the check in any other endpoints that would use meta.

#6 @joehoyle
5 months ago

@rachelbaker hmm I'm not a big fan of this approach. Special casing meta is not nice, and I think this is just a bit too special / one off. I think we should be able to add an arg_options here https://github.com/WordPress/WordPress/blob/master/wp-includes/rest-api/fields/class-wp-rest-meta-fields.php#L393 to specify a custom validate callback for this schema item.

#7 @rachelbaker
5 months ago

  • Keywords needs-refresh added

Our schema ignores the sanitization or validation callbacks in top-level parameters (like the meta object parameter). After discussing with @joehoyle in person, we agree that special-casing meta as proposed in 38989.2.diff is a hacky workaround the real problem.

Instead, we decided for 4.7 we would ignore the meta property in a request if it is not an array, which is better than returning a PHP Warning which results in unparsable JSON data. Updated patch incoming...

For 4.8 @joehoyle wants to fix this in register_rest_field. @joehoyle would you mind creating a ticket for the underlying problem for solving in 4.8?

@rachelbaker
5 months ago

Ignore the meta property in a request if it is not an array before trying to update meta

#8 @rachelbaker
5 months ago

  • Keywords 2nd-opinion added; needs-refresh removed

In 38989.3.diff ignore the meta values of a request if attempting to update meta when meta is not an array. This fixes the bug where currently a PHP warning is returned if in a create/update request `'meta' is not an array.

I would like a 2nd opinion on the possible perception of "breakage" if we implement the longer term fix @joehoyle has in mind.

Explanation of what is going on in this ticket

Send a request to update a post with request data like so:

{ 
'content': "Look at me changing the post_content.", 
'meta': "this is not an array or object of meta keys and values!" 
}

Current behavior:

  • your post would update, so the content would now be "Look at me changing the post_content.".
  • the invalid string in the meta property would trigger a PHP Warning which would result in invalid JSON data for your response (if a site is set to display PHP warnings)
  • the client would have an indication that something went wrong...

Behavior with 38989.3.diff) applied:

  • your post would update, so the content would now be "Look at me changing the post_content.".
  • the invalid string in the meta property would be ignored, and not trigger an error
  • the client would have no indication that something went wrong. :(

Proposed behavior with fixing the underlying problem in 4.8:

  • the client would receive an error noting that meta was an invalid param in the request
  • the post would not update

@jnylen0
5 months ago

Use sanitize_callback to verify that meta is an array

#9 @jnylen0
5 months ago

Replying to rachelbaker:

Our schema ignores the sanitization or validation callbacks in top-level parameters

I think the real issue is that meta is declared with type object, which we don't validate at all yet, no? The full fix will be to support object in schemas in a future version.

Replying to joehoyle:

I think we should be able to add an arg_options here https://github.com/WordPress/WordPress/blob/master/wp-includes/rest-api/fields/class-wp-rest-meta-fields.php#L393 to specify a custom validate callback for this schema item.

Good suggestion, this seems to be working quite well in 38989.4.diff.

Replying to rachelbaker:

I would like a 2nd opinion on the possible perception of "breakage" if we implement the longer term fix @joehoyle has in mind.

I think we should avoid this future behavior change if possible.

#10 @rachelbaker
5 months ago

  • Keywords 2nd-opinion removed

@jnylen0 Your 38989.4.diff looked good, I only had minor feedback:

  • let's use the validate_callback instead because we aren't really sanitizing the array
  • rename the callback check_ to closely match the naming convention of our custom callbacks

In the interest of time I did both of these in 38989.5.diff - is this acceptable? If you agree, I will go ahead and apply the commit keyword so we can move this forward.

@rachelbaker
5 months ago

Use the validate_callback and renamed the callback method check_meta_is_array()

#11 @pento
5 months ago

  • Keywords commit added

#12 @pento
5 months ago

38989.5.diff Looks good, let roll with it.

#13 @rachelbaker
5 months ago

  • Keywords dev-reviewed added

Sitting next to @jnylen0 and got his verbal +1 with the changes in 38989.5.diff so I am considering this dev-reviewed.

#14 @rachelbaker
5 months ago

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

In 39436:

REST API: Return a WP_Error if meta property is not an array.

Fixes bug where a PHP Warning is currently thrown if a client sends a request where meta is not an array value.

Props timmydcrawford, jnylen0, rachelbaker, pento.
Fixes #38989.

#15 @rachelbaker
5 months ago

In 39437:

REST API: Return a WP_Error if meta property is not an array.

Fixes bug where a PHP Warning is currently thrown if a client sends a request where meta is not an array value.

Merges [39436] onto 4.7 branch.

Props timmydcrawford, jnylen0, rachelbaker, pento.
Fixes #38989 for 4.7.

Note: See TracTickets for help on using tickets.