WordPress.org

Make WordPress Core

Opened 16 months ago

Last modified 3 months ago

#46671 reviewing defect (bug)

wp-admin/revisions.php uses the $post global in the wrong way

Reported by: tofandel Owned by: SergeyBiryukov
Milestone: 5.5 Priority: normal
Severity: major Version: 5.2
Component: Revisions Keywords: has-patch
Focuses: administration Cc:

Description

I found a bug quite interesting when restoring a revision, in wp-admin/revision.php the code is not encapsulated and thus using the globals $post, $revision etc

A problem arrise because of those lines

<?php
if ( ! $post = get_post( $revision->post_parent ) )
                break;
// ...
wp_restore_post_revision( $revision->ID );
$redirect = add_query_arg( array( 'message' => 5, 'revision' => $revision->ID ), get_edit_post_link( $post->ID, 'url' ) );

As you can see the $post global is exposed and then used again after the the wp_restore_post_revision function call which triggers countless hooks

The problem is if the $post global is edited in one of those hooks, then the ID passed to get_edit_post_link is wrong and since post has not been set via WP_Query, the wp_reset_postdata function does nothing, here is a snippet you can use to demonstrate the bug, after restoring a revision you will be redirected to the edit page of the post 4 instead of the post you were editing

<?php
add_action('wp_restore_post_revision', function() {
        global $post;

        $post->ID = 4;
        
        wp_reset_postdata(); //This will do nothing because the post has not been defined through WP_Query
});


To fix this bug I would suggest encapsulating the logic into a function to stop exposing the variables as globals or to rename the variable being used

PS: this design flaw is present in other files as well, but that's the only one I noticed causing issues

Attachments (2)

46671.diff (2.7 KB) - added by markparnell 4 months ago.
Renamed $post variable to $parent to avoid conflicts with global variable
46671.2.diff (3.2 KB) - added by markparnell 3 months ago.

Download all attachments as: .zip

Change History (10)

#2 @SergeyBiryukov
16 months ago

  • Component changed from General to Revisions
  • Focuses administration added

#3 @SergeyBiryukov
16 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

#4 @archon810
12 months ago

I just spent hours trying to figure out why restoring revisions sometimes redirects to the wrong post and after debugging and pulling my hair out arrived at a conclusion that this has to be a bug in WP core.

Then I found this ticket (along with #18408 which is still unresolved after 8 years and may or may not be related).

In our case, we have a hook into transition_post_status, and in this hook, we perform a secondary query using the correct new WP_Query(), have_posts(), and the_post(), followed by wp_reset_postdata().

The last post in this secondary query erroneously sticks around as $post after wp_reset_postdata().

This happens as part of wp-includes/post.php->wp_restore_post_revision()->wp_update_post()->wp_insert_post()->wp_transition_post_status(). The redirect ends up using this incorrect $post.

Here's the call stack:
https://i.imgur.com/06mTD0X.png

Is there anyone who can take a look at properly resolving this bug @SergeyBiryukov?

Thanks.

Last edited 12 months ago by SergeyBiryukov (previous) (diff)

@markparnell
4 months ago

Renamed $post variable to $parent to avoid conflicts with global variable

#5 @markparnell
4 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

#6 @markparnell
3 months ago

  • Keywords needs-testing removed

#7 @tofandel
3 months ago

@markparnell You should probably set the WP Query's post variable to the $parent

<?php
global $wp_query;

$post = $parent;
$wp_query->posts = [$post];
$wp_query->post = $post;
setup_postdata($post);

to avoid breaking existing code and solve the issue with wp_reset_postdata not doing anything

(Though it feels hackish by lack of other way to do this)

Last edited 3 months ago by tofandel (previous) (diff)

#8 @markparnell
3 months ago

Thanks @tofandel. Had to tweak your logic slightly, but 46671.2.diff makes the global post data available again to anything hooked into the restore process while still fixing the original issue.

@markparnell
3 months ago

#9 @SergeyBiryukov
3 months ago

  • Milestone changed from Future Release to 5.5
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing
Note: See TracTickets for help on using tickets.