WordPress.org

Make WordPress Core

Opened 16 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.3 Priority: normal
Severity: normal Version:
Component: REST API Keywords: has-patch has-unit-tests early needs-dev-note
Focuses: rest-api 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 15 months ago.

Download all attachments as: .zip

Change History (16)

#1 @azaozz
16 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 16 months ago by azaozz (previous) (diff)

#2 @rmccue
16 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
15 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
15 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 attachments. This should always be "do_not_allow" by default.
Version 0, edited 15 months ago by azaozz (next)

#5 follow-up: @danielbachhuber
15 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.

#6 @danielbachhuber
9 months ago

  • Focuses rest-api added

This ticket was mentioned in Slack in #core-restapi by danielbachhuber. View the logs.


9 months ago

#8 in reply to: ↑ 5 @azaozz
9 months ago

  • Milestone changed from 5.0 to 5.1

Replying to danielbachhuber:

always applying do_not_allow in map_meta_cap() will be a breaking change...

Seems this needs another look as it will trip plugins that want to delete revisions (and are checking user caps).

How about introducing another capability: delete_revisions? It would need to be overridden when a (parent) post is deleted, all revisions should be deleted with it. Then it could be true for admins by default (so we don't break existing plugins) but false for editors, authors, and anybody else that can delete a post. Then installs with many editors and authors may want to grant it to other roles or remove it completely.

In any case, this has a potential to introduce regressions in plugins, lets look at it again after 5.0 :)

This ticket was mentioned in Slack in #core-restapi by desrosj. View the logs.


6 months ago

#10 @desrosj
6 months ago

  • Keywords early added
  • Milestone changed from 5.1 to 5.2

The patch still applies cleanly, but let's punt this to 5.2 since it still needs a decision. Getting it in early for testing purposes would also be beneficial, and an early dev-note would also help.

@flixos90 @johnbillion do you have any thoughts on this?

This ticket was mentioned in Slack in #core-restapi by desrosj. View the logs.


6 months ago

#12 @johnbillion
6 months ago

On the surface this looks like a sensible change, but I don't have any capacity to look into it further at the moment.

This ticket was mentioned in Slack in #core-restapi by desrosj. View the logs.


4 months ago

#14 @kadamwhite
4 months ago

  • Milestone changed from 5.2 to 5.3

Punting to 5.3-early after discussion in March 14th REST API component meeting.

#15 @desrosj
2 months ago

  • Keywords needs-dev-note added
Note: See TracTickets for help on using tickets.