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

[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

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

[ramonopoly]

I've updated the ​attached patch with a fix.

[adamsilverstein]

thanks for the bug report @ramonopoly - I'll take a look. Thanks for adding tests!

Thanks, folks. I appreciate the testing and reviews!

[adamsilverstein]

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?

[danielbachhuber]

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

[adamsilverstein]

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

@ramonjd 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?

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.

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.

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.

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.

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

[isabel_brison]

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.

Committed in r57222.

[SergeyBiryukov]

In 57268:

Tests: Remove some leftover debugging in WP_REST_Revisions_Controller tests.

Follow-up to [57222].

See #59875.

[ramonopoly]

@SergeyBiryukov Thanks for spotting those var_dumps!

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