Make WordPress Core

Opened 5 years ago

Last modified 4 years ago

#48426 new defect (bug)

Can't update meta via the REST API if an identical value for a field you don't have permission to update is included

Reported by: johnstonphilip's profile johnstonphilip Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.7
Component: Options, Meta APIs Keywords:
Focuses: rest-api Cc:

Description

On WP 5.3-RC2-46575, if you update any meta via the REST API via Gutenberg, all meta gets updated. If any of those meta update attempts fail because of user cap requirements, all meta updates fail because of that user cap requirement. Thus, all registered meta require the same user caps.

This happens because all passed-in meta values are iterated through here:
https://github.com/WordPress/WordPress/blob/master/wp-includes/rest-api/fields/class-wp-rest-meta-fields.php#L135

And then updated here:
https://github.com/WordPress/WordPress/blob/master/wp-includes/rest-api/fields/class-wp-rest-meta-fields.php#L187-L191

From Gutenberg, those update calls fire regardless of whether the value is being changed. I believe this is because the entire Redux store is passed in and iterated over.

This causes the issue I mentioned in regards to capability checking with protected meta. Because you can require a user capability for protected meta through the auth_callback when running register_meta, any unrelated meta could prevent another unrelated piece of meta from being updated.

For example, if User A has cap_a, and tries to update _custom_meta_a, but _custom_meta_b requires cap_b, that user cannot update _custom_meta_a without getting a WP_Error here:

https://github.com/WordPress/WordPress/blob/master/wp-includes/rest-api/fields/class-wp-rest-meta-fields.php#L258

If we only update meta that has been changed, this should help here.

To replicate the problem follow these steps (props @modernnerd):

  1. Install WP 5.3-RC1 via the WP beta tester plugin by choosing “bleeding edge nightlies” at Tools → Beta Testing.
  2. Activate the Twenty Twenty theme (these steps will work with any theme, though).
  3. Add this PHP code to the Twenty Twenty theme's functions.php file to add custom post meta exposed to the REST API:
function custom_register_post_meta() {
	$args = [
		'auth_callback' => 'custom_prefix_require_update_plugins',
		'type'          => 'boolean',
		'single'        => true,
		'show_in_rest'  => true,
	];
	register_meta( 'post', '_custom_post_meta_one', $args );

	$args = [
		'auth_callback' => 'custom_prefix_require_non_existing_cap',
		'type'          => 'boolean',
		'single'        => true,
		'show_in_rest'  => true,
	];
	register_meta( 'post', '_custom_post_meta_two', $args );

}
add_action( 'init', 'custom_register_post_meta' );

function custom_prefix_require_update_plugins() {
	if ( current_user_can( 'update_plugins' ) ) {
		return true;
	} else {
		return false;
	}
}

function custom_prefix_require_non_existing_cap() {
	if ( current_user_can( 'this_cap_does_not_exist' ) ) {
		return true;
	} else {
		return false;
	}
}
  1. Edit a post or page. Make a change to the post content and click “Publish” or “Update” to confirm that changes work as expected.
  1. In the browser console, display the post meta exposed to the REST API by pasting this command and typing enter:
wp.data.select( 'core/editor' ).getEditedPostAttribute( 'meta' );

You should see an object that includes your new custom meta:

{_custom_post_meta_one: false, _custom_post_meta_two: false}

  1. Update _custom_post_meta_one to 'true' with this command (this persists the data to the Redux store, but not the database):
wp.data.dispatch( 'core/editor' ).editPost( { meta: { _custom_post_meta_one :  true } } );

Note that this is just a simplified test case. It emulates a plugin or theme updating meta via the WP API, without having to install a plugin or theme that uses custom post meta and Gutenberg.

  1. Click Update in the post editor to save your post meta changes to the database.

You'll see “Updating failed. Error message: Sorry, you are not allowed to edit the _custom_post_meta_two custom field.”.

The thing is, _custom_post_meta_two wasn't actually updated or changed by the user, yet their lack of permissions for _custom_post_meta_two is preventing them from updating _custom_post_meta_one.

I looked at doing a comparison check prior to saving for the saved value vs the new value, but because the saved value is always a string when coming from the database, you can't do strict checking, which could be problematic. So I am not sure what the fix needs to be here.

Change History (10)

#1 @johnbillion
5 years ago

  • Component changed from General to Posts, Post Types
  • Focuses rest-api added

#2 @TimothyBlynJacobs
5 years ago

  • Component changed from Posts, Post Types to Options, Meta APIs
  • Milestone changed from Awaiting Review to Future Release
  • Summary changed from All registered meta require the same caps on 5.3-RC2-46575 to Can't update meta via the REST API if an identical value for a field you don't have permission to update is included
  • Version changed from trunk to 4.7

#3 @johnstonphilip
5 years ago

@TimothyBlynJacobs Just confirming on this. You changed the title to say "if an identical value for a field". But in my tests the value does not have to be identical to any other value. It can be any value.

#4 @TimothyBlynJacobs
5 years ago

Yep, but I think that you should get back an error response if you are trying to change the value.

#5 @johnstonphilip
5 years ago

Yes you should. That is what is happening currently. What I am focused on here is that the wrong is error is being thrown for the wrong field, preventing a legitimate update. A field that wasn't being updated, is being updated. Not only does this cause the permissions error, but I would imagine also wastes update calls that don't actually need to happen.

#6 @johnstonphilip
5 years ago

Ah, I just understood how you are saying it. You are saying that if you pass an identical value to what it used to be, it is preventing the update of the other unrelated field. My bad! Just took me a while to process.

#7 @TimothyBlynJacobs
5 years ago

Yep exactly. Sorry if I didn't write that clearly!

#8 @TimothyBlynJacobs
4 years ago

@johnstonphilip were you interested in writing a patch for this?

#9 @johnstonphilip
4 years ago

@TimothyBlynJacobs Thanks for the message! Unfortunately I am still stuck on this part:

I looked at doing a comparison check prior to saving for the saved value vs the new value, but because the saved value is always a string when coming from the database, you can't do strict checking, which could be problematic. So I am not sure what the fix needs to be here.

#10 @TimothyBlynJacobs
4 years ago

@johnstonphilip Try using the WP_REST_Meta_Fields::is_meta_value_same_as_stored_value method.

Note: See TracTickets for help on using tickets.