Make WordPress Core

Opened 3 weeks ago

Last modified 33 hours ago

#49645 reopened defect (bug)

get_item function broken in rest-api

Reported by: yohannp Owned by: kadamwhite
Milestone: 5.4.1 Priority: normal
Severity: major Version: 5.3
Component: REST API Keywords: good-first-bug has-unit-tests has-patch fixed-major
Focuses: Cc:



Due to this commit, a delete permission check has been added to get_item function in class-wp-rest-revisions-controller.php.
It seems this permission check was added by this commit : https://github.com/WordPress/WordPress/commit/00cb4c7dbdd3a0bbf1f7da0898a72e12198a61d5#diff-df4c11238da6b9e0e261b38f895720f1R352. This breaks some preview plugins using WordPress REST API.
Obviously, this seems to be kind of a copy/paste mistake as there is no reason to have a delete_post permission check on a get item...


Change History (15)

#1 @kadamwhite
3 weeks ago

@yohannp Can you give an example of a plugin and action that is broken by this? The commit you link to appears to correctly apply the security check we would want to use for revision deletion, we'd need to know more about the behavior you would expect that now breaks in order to determine whether this was inaccurate.

#2 @kadamwhite
3 weeks ago

Ah sorry, was rushing and misread, that does look like this check's in the incorrect spot.

#3 @TimothyBlynJacobs
3 weeks ago

  • Keywords needs-patch good-first-bug needs-unit-tests added
  • Milestone changed from Awaiting Review to 5.5
  • Version changed from 5.3.2 to 5.3

Chatting with @kadamwhite it looks like that check is meant to be on line 391. In other words, right before checking if the user has permission to delete a revision, make sure they have permission to delete the parent item as well.

Would you be interested in working on a patch?

Last edited 3 weeks ago by TimothyBlynJacobs (previous) (diff)

#4 @sorenbronsted
3 weeks ago

I will take this one.

#6 @sorenbronsted
3 weeks ago

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

I have fixed this in the above PR request, but I am not sure if I have set the correct workflow keyword.

#7 @sorenbronsted
2 weeks ago

The PR says 'undefined' here but it is all checks has passed on the github PR page :-)

#8 @TimothyBlynJacobs
12 days ago

Thanks for the patch @sorenbronsted! We still want to keep that permission check, but it should be done in the delete permission check method.

#9 @sorenbronsted
12 days ago

@TimothyBlynJacobs the method delete_item_permissions_check actually does a permission check so I suppose it is not necessary to do the same permission check in the delete_item again. The test_delete_item testcase covers this scenario.

#10 @TimothyBlynJacobs
12 days ago

Hi @sorenbronsted, it does to a permission check, but the one that was in get was different, it was also checking against the parent resource. I think it'd be good to keep that check. We should introduce a unit test to cover that as well.

This ticket was mentioned in PR #201 on WordPress/wordpress-develop by sorenbronsted.

9 days ago

Added the delete parent check and a testcase for it

Trac ticket: https://core.trac.wordpress.org/ticket/49645

#12 @yohannp
6 days ago

Hi !

Thanks a lot for the fix.
I saw WordPress 5.4 will be out tomorrow, but this fix is not yet merged (https://github.com/WordPress/wordpress-develop/pull/201). As it is breaking some plugins using this API, could you consider merging it to be included in the tomorrow release.


#13 @TimothyBlynJacobs
5 days ago

Hi @yohannp,

Unfortunately the cut off for non-regressions in 5.4 was on March 3rd before the first release candidate.

If there is a 5.4.1 we could try getting it in that release.

#14 @kadamwhite
33 hours ago

  • Owner set to kadamwhite
  • Resolution set to fixed
  • Status changed from new to closed

In 47547:

REST API: Fix revisions controller get_item permission check.

r45812 incorrectly introduced a delete_post permissions check into the get_item method, breaking some plugins which requested revisions when generating previews.

Props sorenbronsted, yohannp, TimothyBlynJacobs.
Fixes #49645.

#15 @kadamwhite
33 hours ago

  • Keywords fixed-major added
  • Milestone changed from 5.5 to 5.4.1
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport consideration in 5.4.1

Note: See TracTickets for help on using tickets.