Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 13 months ago

#40889 closed defect (bug) (duplicate)

REST API: New terms creation with meta causes PHP Notice

Reported by: caercam's profile caercam Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch
Focuses: rest-api Cc:


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 6 years ago.
40889.2.diff (700 bytes) - added by caercam 6 years ago.
40889.3.diff (1.9 KB) - added by caercam 6 years ago.
40889-4.patch (5.0 KB) - added by theorboman 5 years ago.
Patch with Unit tests för the Categories and Tags controllers

Download all attachments as: .zip

Change History (22)

6 years ago

#1 @caercam
6 years ago

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

#2 @TimothyBlynJacobs
6 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] 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 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.

6 years ago

#4 @caercam
6 years ago

  • Keywords has-patch added

#5 @jnylen0
6 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.

6 years ago

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

5 years ago

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

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

  • Milestone changed from Awaiting Review to 5.0

#9 @danielbachhuber
5 years 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

This ticket was mentioned in PR #3206 on WordPress/wordpress-develop by MaggieCabrera.

13 months ago

  • Keywords has-patch added
  • Theme Json: Don't output double selectors for elements inside blocks #40889
  • Global Styles: Load block CSS conditionally #41160

Trac ticket:

MaggieCabrera commented on PR #3206:

13 months ago

While trying to fix the tests I found a bug that wasn't caught in GB because the test is only on core, not in the GB repo. I'm going to try and patch that so we can add it to this one or we could "hack" the snapshot and then patch afterwards. I'm more inclined to do the former.

MaggieCabrera commented on PR #3206:

13 months ago

This is the fix for the issue that the tests uncovered.

MaggieCabrera commented on PR #3206:

13 months ago

this only needs a review and check what's breaking the tests (they are all passing locally)

SergeyBiryukov commented on PR #3206:

13 months ago

The tests previously failed on PHP 5.6 with a fatal error:
Fatal error: Cannot use isset() on the result of an expression (you can use "null !== expression" instead) in src/wp-includes/class-wp-theme-json.php on line 1526

0015d99 should resolve the issue. We already used array_key_exists( $element, static::VALID_ELEMENT_PSEUDO_SELECTORS ) in one place, and that resolves the error in other instances in my testing.

SergeyBiryukov commented on PR #3206:

13 months ago

Thanks for the PR! Merged in r54118.

ockham commented on PR #3206:

13 months ago

Thanks a lot, folks!

I was alerted that unfortunately, r54118 caused a fatal error on websites running Core trunk and a current version of the Gutenberg plugin 😕

PHP Fatal error:  Cannot redeclare wp_add_global_styles_for_blocks() (previously declared in wp-includes/global-styles-and-settings.php:202) in wp-content/plugins/gutenberg/lib/compat/wordpress-6.1/get-global-styles-and-settings.php on line 12

We'll need to add a ! function_exists() wrapper around the function definition in Gutenberg to avoid this.

Unfortunately, this won't fix the problem immediately: WP sites running Core trunk with the GB plugin active will continue to be affected until a new version of the plugin is released (as planned for next Thursday); or until the change is merged into GB trunk (if the site is running GB trunk). And finally, sites running Core trunk and an older version of GB (that has the unguarded wp_add_global_styles_for_blocks function definition) will continue to be affected 😕

I'm not sure how to best cover the latter case. For mitigation, we might want to consider reverting r54118 (as much as it pains me to suggest that) -- at least until a GB version with the fix is released.


Going forward, we should probably put some strategies in place to avoid collisions like this:

  • Require wp_ function definitions in GB to be guarded with a ! function_exists() check (and ideally enforce with a WPCS lint).
  • Add a check in wordpress-develop's CI to install and enable GB alongside Core (and run some basic tests to make sure it doesn’t fatal).

Anyway, this is something we'll discuss separately. I'll file one or two issues tomorrow.

ockham commented on PR #3206:

13 months ago

Update: Unbeknownst to me, the wp_ prefix has been changed to gutenberg_ in the GB plugin:

ockham commented on PR #3206:

13 months ago

Update #2: Let's hold off on the revert. We might release a Gutenberg point release with this fix as a stop-gap.

Note: See TracTickets for help on using tickets.