get_item function broken in rest-api

[yohannp]

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,

[kadamwhite]

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

[kadamwhite]

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

[TimothyBlynJacobs]

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?

[sorenbronsted]

I will take this one.

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

[sorenbronsted]

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

[sorenbronsted]

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

[TimothyBlynJacobs]

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

[sorenbronsted]

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

[TimothyBlynJacobs]

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.

Added the delete parent check and a testcase for it

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

[yohannp]

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,

[TimothyBlynJacobs]

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.

[kadamwhite]

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.

[kadamwhite]

Reopening for backport consideration in 5.4.1

[whyisjake]

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.

[SergeyBiryukov]

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 :)