Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#49645 closed defect (bug) (fixed)

get_item function broken in rest-api

Reported by: yohannp's profile yohannp Owned by: kadamwhite's profile 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:

Description

Hello,

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...

Thanks,

Change History (19)

#1 @kadamwhite
4 years 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
4 years ago

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

#3 @TimothyBlynJacobs
4 years 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?

Version 0, edited 4 years ago by TimothyBlynJacobs (next)

#4 @sorenbronsted
4 years ago

I will take this one.

#6 @sorenbronsted
4 years 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
4 years ago

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

#8 @TimothyBlynJacobs
4 years 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
4 years 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
4 years 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.


4 years ago
#11

Added the delete parent check and a testcase for it

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

#12 @yohannp
4 years 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.

Thanks,

#13 @TimothyBlynJacobs
4 years 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
4 years 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
4 years 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

This ticket was mentioned in Slack in #core-committers by whyisjake. View the logs.


4 years ago

This ticket was mentioned in Slack in #core by whyisjake. View the logs.


4 years ago

#18 follow-up: @whyisjake
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 47562:

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.

Bring this commit back to the 5.4 branch.

Props sorenbronsted, yohannp, TimothyBlynJacobs.

Fixes #49645.

#19 in reply to: ↑ 18 @SergeyBiryukov
4 years ago

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

Bring this commit back to the 5.4 branch.

@whyisjake Minor nitpick: it's not quite clear that "this commit" refers to [47547] and not to [45812], which is mentioned as incorrect and breaking plugins.

It would be better to use the format recommended in the Backporting Commits handbook article in the future:

Props sorenbronsted, yohannp, TimothyBlynJacobs.
Merges [47547] to the 5.4 branch.
Fixes #49645.

Thanks :)

Note: See TracTickets for help on using tickets.