Make WordPress Core

Opened 4 years ago

Last modified 4 months ago

#52925 new defect (bug)

Autosaves controller: Post checks will never catch invalid IDs

Reported by: coreymckrill's profile coreymckrill Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 5.8
Component: REST API Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

The create_item and create_post_autosave methods both try to check if the id parameter in a request is for a valid post, by calling the get_post function. The problem is that both methods expect that if it's not a valid post, it will return a WP_Error object, when in fact get_post only returns null on failure.

The Posts controller has a protected get_post method that will generate an appropriate WP_Error for this case, but neither the Autosaves, nor its parent Revisions controller has a similar method. Copying that method to the Revisions controller, and then using it in the create_* methods seems like the best approach here.

Change History (9)

#1 @SergeyBiryukov
4 years ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 5.8

#2 @donmhico
4 years ago

  • Keywords has-patch added; needs-patch removed

Created a PR here - https://github.com/WordPress/wordpress-develop/pull/1145 - i'm not sure why the PR is not attached in this ticket. PR still needs unit test though.

This ticket was mentioned in PR #1145 on WordPress/wordpress-develop by donmhico.


4 years ago
#3

This PR copies the implementation of get_post() from WP_REST_Posts_Controller to WP_REST_Autosaves_Controller so create_item() and create_post_autosave() would return the correct WP_Error on get_post() failure.

I'll try to add a unit test when possible.

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

#4 @hermpheus
3 years ago

Hi @donmhico , I have some suggestions for unit tests. I made a pull request into your branch here https://github.com/donmhico/wordpress-develop/pull/1.

#5 @whyisjake
3 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 5.8 to Future Release

I like the changes from @hermpheus. Can we get a unified PR created?

This ticket was mentioned in Slack in #core by whyisjake. View the logs.


3 years ago

#7 @antonvlasenko
4 months ago

I am willing to create a unified PR to help move this ticket forward.
@hermpheus and @donmhico would either of you prefer to take this on?

Last edited 4 months ago by antonvlasenko (previous) (diff)

#8 @antonvlasenko
4 months ago

I'm working on an unified PR for this ticket.

Update: The PR I've submitted is based on the two PRs above, but it updates the code according to the latest changes in the Autosaves controller. Additionally, it contains an updated version of the unit tests that ensures the actual changes being made are covered.

Last edited 4 months ago by antonvlasenko (previous) (diff)

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


4 months ago
#9

  • Keywords has-patch has-unit-tests added; needs-unit-tests needs-patch removed
Note: See TracTickets for help on using tickets.