Make WordPress Core

Opened 7 months ago

Closed 6 months ago

Last modified 5 months ago

#59875 closed defect (bug) (fixed)

Revisions controller: get_item can return a revision whose parent does not match the `parent` route fragment

Reported by: ramonopoly's profile ramonopoly Owned by: adamsilverstein's profile adamsilverstein
Milestone: 6.5 Priority: normal
Severity: normal Version:
Component: REST API Keywords: has-unit-tests has-patch commit
Focuses: rest-api Cc:

Description (last modified by ramonopoly)

When fetching a single revision from the WP_REST_Revisions_Controller, it's possible for get_item to return a revision whose parent does not match the parent in the REST URL.

For example:

'/wp/v2/posts/${ someParent }/revisions/${ revisionOfSomeOtherParent }'

So long as someParent has a matching post type, and revisionOfSomeOtherParent is a revision post type, I'll get the revision object of revisionOfSomeOtherParent, whose parent is another id.

The consequence is that for any post type, I can return the revision of different parent post of that same type just by knowing the revision's post ID.

It's a bit of an edge case but one, perhaps, that could be guarded against for example by checking that parent.id === revision.parent or something.

The risk is that there might be some sites that, for whatever reason, probably unintentionally, are expecting values back from a mismatched combination of parentId and revisionId.

The argument for dismissing this concern is that, in such as case, the URL is not representational of the resource, and so it is not RESTful.

What do folks think?

First noticed here: https://github.com/WordPress/gutenberg/pull/55827#discussion_r1385977711

Related Slack discussion: https://wordpress.slack.com/archives/C02RQC26G/p1699504213232439

Props to @andrewserong for helping to track down the bug

Change History (20)

This ticket was mentioned in PR #5655 on WordPress/wordpress-develop by @ramonopoly.


7 months ago
#1

  • Keywords has-patch added

Adding some tests to check whether get_item can return a revision whose parent does not match the parent route fragment.

Run npm run test:php -- --filter WP_Test_REST_Revisions_Controller

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

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


7 months ago

#3 @ramonopoly
7 months ago

I've updated the attached patch with a fix.

#4 @adamsilverstein
7 months ago

thanks for the bug report @ramonopoly - I'll take a look. Adding some unit tests to validate the fix would be helpful.

Version 0, edited 7 months ago by adamsilverstein (next)

#5 @adamsilverstein
7 months ago

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

#6 @ramonopoly
7 months ago

  • Description modified (diff)
  • Type changed from enhancement to defect (bug)

#7 @ramonopoly
7 months ago

  • Description modified (diff)

@ramonopoly commented on PR #5655:


7 months ago
#8

Thanks, folks. I appreciate the testing and reviews!

#9 @adamsilverstein
7 months ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to 6.5

Nice work @ramonopoly - the PR looks good to me, and I verified the tests fail (and the parent post is returned) when running the new test without the fix. Milestoning this for 6.5.

As far as I can tell this bug has existed since the revisions endpoint was introduced, does that seem right @danielbachhuber?

#10 @danielbachhuber
7 months ago

@adamsilverstein I agree with your assessment. I imagine it was simply an oversight at the time.

#11 @adamsilverstein
7 months ago

ps @ramonopoly reviewing the linked Gutenberg PR, I'm curious what capability or extensibility was missing in the core revisions controller that made you need to create an entirely new controller for the global style revisions? are you hoping to bring new capabilities back to the core controller?

cc: @TimothyBlynJacobs

@ramonopoly commented on PR #5655:


7 months ago
#13

Has this change been tested with https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/rest-api/endpoints/class-wp-rest-template-revisions-controller.php controller that extends this controller?

Good point. I can add a relevant unit test to the WP_REST_Template_Revisions_Controller test suite.

@ramonopoly commented on PR #5655:


7 months ago
#14

I can confirm that we'll need to overwrite get_item() in WP_REST_Template_Revisions_Controller to account for the wp_id > parent relationship between template record and their revisions.

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/6458278/83ad9a56-ef09-48d4-85c2-7d7eff74c2fc

I'll do this and add a test to this PR since it's related, and also check any other post types that extend this controller.

@ramonopoly commented on PR #5655:


7 months ago
#15

I can confirm that we'll need to overwrite get_item() in WP_REST_Template_Revisions_Controller to account for the wp_id > parent relationship between template record and their revisions.

Actually, withdraw that, I should have checked WP_REST_Revisions_Controller::get_item before posting.

The check in get_item that this PR introduces does so by comparing the results of get_post for both the parent and the revision.

The results of get post is independent of the of the WP_REST_Template_Revisions_Controller which performs all the wp_id faffery.

Regardless, WP_REST_Template_Revisions_Controller has unit tests for get_item, which would have failed otherwise.

I've also double checked other registered post types, none of which, wp_template and wp_template_part notwithstanding, define a custom revisions_rest_controller_class.

I'd submit, therefore, that this PR is fine as it is.

@ramonopoly commented on PR #5655:


6 months ago
#16

Thanks a lot for the feedback @spacedmonkey! 🙇🏻 I'll get it done today or tomorrow.

#17 @isabel_brison
6 months ago

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

In 57222:

REST API: check parent and revision ids match before retrieving revision.

Adds a condition to check that parent id matches revision parent id in WP_REST_Revisions_Controller get_item method.

Props ramonopoly, adamsilverstein, danielbachhuber, spacedmonkey, andrewserong.
Fixes #59875.

@isabel_brison commented on PR #5655:


6 months ago
#18

Committed in r57222.

#19 @SergeyBiryukov
5 months ago

In 57268:

Tests: Remove some leftover debugging in WP_REST_Revisions_Controller tests.

Follow-up to [57222].

See #59875.

#20 @ramonopoly
5 months ago

@SergeyBiryukov Thanks for spotting those var_dumps!

I must have been wearing my old glasses that day 🤦🏼‍♂️

Note: See TracTickets for help on using tickets.