Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 4 years ago

#43709 closed defect (bug) (fixed)

Fix or remove the "delete revision" endpoint

Reported by: azaozz's profile azaozz Owned by: kadamwhite's profile kadamwhite
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: REST API Keywords: has-patch has-unit-tests early has-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 (3)

43709.1.diff (4.5 KB) - added by danielbachhuber 6 years ago.
43709.diff (5.1 KB) - added by TimothyBlynJacobs 5 years ago.
43709.3.diff (5.0 KB) - added by kadamwhite 5 years ago.
Correct a PHPCS error around strict array comparison.

Download all attachments as: .zip

Change History (32)

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

#2 @rmccue
6 years 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
6 years 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
6 years 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 6 years ago by azaozz (previous) (diff)

#5 follow-up: @danielbachhuber
6 years 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
5 years ago

  • Focuses rest-api added

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


5 years ago

#8 in reply to: ↑ 5 @azaozz
5 years 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.


5 years ago

#10 @desrosj
5 years 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.


5 years ago

#12 @johnbillion
5 years 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.


5 years ago

#14 @kadamwhite
5 years 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
5 years ago

  • Keywords needs-dev-note added

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


5 years ago

#17 @TimothyBlynJacobs
5 years ago

@danielbachhuber are you still interested in working on this?

#18 @danielbachhuber
5 years ago

  • Owner danielbachhuber deleted

@TimothyBlynJacobs Not particularly, to be honest. I remember being reasonably confident in 43709.1.diff at the time though.

#19 @kadamwhite
5 years ago

@TimothyBlynJacobs Do you have an appetite to take this on, or is anybody else so inclined? We're still reasonably enough in the "early" phase for 5.3 but if we want to do this for the current cycle, we should do it in the next week or two.

I can't estimate from where I'm standing how impactful the proposed breaking change would be, but I'm generally in favor of @danielbachhuber's approach. Adding a new capability feels like an unneeded overstep but I have less working experience with the capabilities system than others on this thread.

#20 @dlh
5 years ago

Unless @TimothyBlynJacobs is on it, I can take a shot at this!

#21 @TimothyBlynJacobs
5 years ago

That'd be brilliant!

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


5 years ago

#23 @dlh
5 years ago

From what I understand of this issue, I'm inclined to agree with the approach in 43709.1.diff.

First, if it's the case that deleting revisions isn't supposed to be allowed without a plugin, then the fact that the change to map_meta_cap() isn't already in core strikes me as a bug, separate even from the REST endpoint.

It's true that changing the mapping for revisions is a backwards-compatibility break. I don't have the ability to search the plugin repo for the potential impact of such a break.

However, as @danielbachhuber says, the change should be graceful, and it would be straightforward for a plugin that wants to override the change to do so.

It also repairs a flaw, in that should core ever check the $delete_post capability for a revision, the check has the potential to return true when it would be assumed to return false.

Second, I agree with @kadamwhite to be disinclined to adding a new capability. The new capability would have to be a meta capability — see ticket:45423#comment:10 And as a meta capability, it would just be mapped to do_not_allow anyway to meet the requirement that no one be allowed to delete revisions.

(If new capabilities for revisions are going to be pursued, perhaps that could begin by setting the revision capability_type [and providing backwards-compatibility] to facilitate plugins distributing all the different post capabilities?)

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


5 years ago

#25 @kadamwhite
5 years ago

The patch looks good, but I'm seeing this error:

1) WP_Test_REST_Revisions_Controller::test_delete_item_cannot_delete_parent
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'rest_cannot_delete'
+'rest_cannot_read'

#26 @TimothyBlynJacobs
5 years ago

I uploaded a patch to fix the test failure. It was using the contributor id which does not have permission to edit the post and so is blocked from deleting the revision earlier on.

I also made it return the rest_cannot_delete error code instead of the generic rest_forbidden.

@kadamwhite
5 years ago

Correct a PHPCS error around strict array comparison.

#27 @kadamwhite
5 years ago

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

In 45812:

REST API: Prevent deletion of post revisions.

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.
While not recommended, a plugin may opt-in to the previous behavior by setting a custom 'delete_post' capability for the revisions post type.

Props dlh, danielbachhuber, TimothyBlynJacobs, azaozz, kadamwhite.
Fixes #43709.

#28 @kadamwhite
5 years ago

In 45820:

Add @ticket annotations for [45812].

Props birgire.
See #43709.

#29 @kadamwhite
4 years ago

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