WordPress.org

Make WordPress Core

Opened 2 months ago

Last modified 2 months ago

#43709 assigned defect (bug)

Fix or remove the "delete revision" endpoint

Reported by: azaozz Owned by: danielbachhuber
Milestone: 5.0 Priority: normal
Severity: normal Version:
Component: REST API Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

Allowing the client to delete revisions breaks the "audit trail" functionality. This is not allowed in WordPress and shouldn't be allowed through the API.

Ideally the delete revision endpoint should be removed. Alternatively it can require a specific permission which should be false by default for all user roles.

Attachments (1)

43709.1.diff (4.5 KB) - added by danielbachhuber 2 months ago.

Download all attachments as: .zip

Change History (6)

#1 @azaozz
2 months ago

  • Keywords needs-patch added

This is a regression. Currently revisions can only be deleted (outside the API) from a plugin. Thinking we should keep it that way. The "audit trail" functionality is not that necessary for smaller sites with one or two users but is rather critical for larger sites that have many authors and editors.

Also see https://core.trac.wordpress.org/ticket/43316#comment:71.

Last edited 2 months ago by azaozz (previous) (diff)

#2 @rmccue
2 months ago

We could handle this in map_meta_cap: if the cap is delete_post and the post is a revision, return do_not_allow.

#3 @danielbachhuber
2 months ago

  • Owner set to danielbachhuber
  • Status changed from new to assigned

For the historical context, this was introduced in https://github.com/WP-API/WP-API/pull/1110

I'm +1 for the capability-based approach.

#4 @azaozz
2 months ago

Yeah, capability based approach will make it easier for plugins to enable deleting of revisions. As far as I see the checks should be if the current user can:

  • Delete the parent post.
  • Delete revisions. This should always be "do_not_allow" by default.
Last edited 2 months ago by azaozz (previous) (diff)

#5 @danielbachhuber
2 months ago

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

In 43709.1.diff:

  • do_not_allow is always applied for a delete_post check on a revision.
  • WP_REST_Revisions_Controller->delete_item_permissions_check() also verifies that the user can delete the parent post.
  • Updates tests correspondingly.

Worth noting: always applying do_not_allow in map_meta_cap() will be a breaking change for any code dependent on current_user_can( $revision_post_type_object->cap->delete_post, $revision->ID );. However, if this was the original intent, the change should be graceful.

Note: See TracTickets for help on using tickets.