WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 14 months ago

#40889 closed defect (bug) (duplicate)

REST API: New terms creation with meta causes PHP Notice

Reported by: caercam Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords:
Focuses: rest-api Cc:
PR Number:

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 (4)

40889.diff (535 bytes) - added by caercam 2 years ago.
40889.2.diff (700 bytes) - added by caercam 2 years ago.
40889.3.diff (1.9 KB) - added by caercam 2 years ago.
40889-4.patch (5.0 KB) - added by theorboman 17 months ago.
Patch with Unit tests för the Categories and Tags controllers

Download all attachments as: .zip

Change History (13)

@caercam
2 years ago

#1 @caercam
2 years ago

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

#2 @TimothyBlynJacobs
2 years 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
2 years 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
2 years ago

#4 @caercam
2 years ago

  • Keywords has-patch added

#5 @jnylen0
2 years 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
2 years ago

#6 @rmccue
2 years 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.

@theorboman
17 months ago

Patch with Unit tests för the Categories and Tags controllers

#7 @theorboman
17 months ago

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

@rmccue @caercam here's the patch I wrote at contributor's day for this with unit tests for the tags and categories controllers.

#8 @swissspidy
16 months ago

  • Milestone changed from Awaiting Review to 5.0

#9 @danielbachhuber
14 months ago

  • Keywords has-patch has-unit-tests removed
  • Milestone 5.0 deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Thanks for your work on the patches, @theorboman @caercam

This particular issue ended up being fixed by #44834

Note: See TracTickets for help on using tickets.