WordPress.org

Make WordPress Core

Opened 6 months ago

Last modified 5 months ago

#40889 new defect (bug)

REST API: New terms creation with meta causes PHP Notice

Reported by: caercam Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch needs-unit-tests
Focuses: rest-api Cc:

Description

Creating new terms with the REST API works fine, as long as there is no meta attached:

POST https://localhost:8080/wp-json/wp/v2/categories
X-WP-Nonce: 1234567890
Content-Type: application/json
{"name":"A new category","description":"This is a new category.","meta":{"custom_meta":"Custom Meta Value"}}
 -- response --
403 Forbidden

<br />
<b>Notice</b>:  Undefined property: WP_Error::$taxonomy in <b>/var/www/wp47/wp-includes/capabilities.php</b> on line <b>282</b><br />
{"code":"rest_cannot_update","message":"Sorry, you are not allowed to edit the custom_meta custom field.","data":{"key":"custom_meta","status":403}}

For the record, here's what happens as far as I can tell. Probably more detailled than what's really needed, but there's two different issues here so I'd rather be precise.

  1. WP_REST_Terms_Controller::create_item() calls WP_REST_Meta_Fields::update_value() with two arguments: $request['meta'], which is what we want, and $request['id'], which at this point is null, because we are creating a new term, not editing an existing one.
  2. WP_REST_Meta_Fields::update_value() loops through the registered meta keys and calls WP_REST_Meta_Fields::update_meta_value() or WP_REST_Meta_Fields::update_multi_meta_value().
  3. Because WP_REST_Terms_Controller::create_item() casted as integer an ID that does not exist, both meta_value update methods get a $object_id = 0, and use that value to check for permission to edit meta.
  4. Now it's capabilities fairy: current_user_can() calls WP_User::has_cap() which calls map_meta_cap() which checks for the term existence with a simple boolean test on the result of get_term().
  5. But, because of $object_id value being 0, get_term() returns a WP_Error instance, the check mentionned in 4. fails, and we get the "Undefined property" notice that end up in the REST API request response.

TL;DR: Two issues here:

  1. WP_REST_Terms_Controller::create_item() passes an unexisting value to the meta_value upate methods, indirectly triggering a bug in map_meta_cap();
  2. map_meta_cap() does not break as it should if get_term() returns an error.

Attachments (3)

40889.diff (535 bytes) - added by caercam 6 months ago.
40889.2.diff (700 bytes) - added by caercam 6 months ago.
40889.3.diff (1.9 KB) - added by caercam 5 months ago.

Download all attachments as: .zip

Change History (9)

@caercam
6 months ago

#1 @caercam
6 months ago

Added patch and opened a new ticket for map_meta_cap() issue: #40891

#2 @TimothyBlynJacobs
6 months ago

I think a better way to patch this would be to pass the term_id to the meta controller as opposed to updating the request object. This would match both how the update_item callback is written and how the [Post Controller https://github.com/WordPress/WordPress/blob/master/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php#L598] works and would keep the request object unmodified which might lead to some issues otherwise.

For example, a simple approach for writing an additional field for the REST API would be to check against the presence of an id parameter to determine if it is an update or a create request ( matching WP internals ). Checking against the HTTP method would probably be better, but I can easily imagine someone writing code like this.

#3 @caercam
6 months ago

Good point. Using $term->term_id was my initial approach, but then I figured having an undefined $request['id'] might be mistakenly interpreted as a failure to create the term even though it was properly inserted.

You're right a missing $request['id'] value could be used to determine if the term was created, though I'd personally go with a HTTP method check; that being said, WP_REST_Server::EDITABLE also supports POST in addition to PUT and PATCH, which may be cause more confusions yet to distinguish updates from creations. I'll add a patch using​ $term->term_id instead.

@caercam
6 months ago

#4 @caercam
5 months ago

  • Keywords has-patch added

#5 @jnylen0
5 months ago

  • Keywords needs-unit-tests added

It would be good to get a PHPUnit test case that fails before this behavior and passes after. This will both ensure that the bug doesn't return in the future and make the ticket much easier to review and commit.

@caercam
5 months ago

#6 @rmccue
5 months ago

@caercam Just a heads up: uploading a patch doesn't send notifications, so you should be sure to comment when you upload a new patch as well. :)

Patch looks good, but the test should check get_post_meta and confirm the value is actually set, rather than just the response code.

Note: See TracTickets for help on using tickets.