WordPress.org

Make WordPress Core

Opened 8 months ago

Last modified 4 months ago

#46671 new defect (bug)

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

Reported by: tofandel Owned by:
Milestone: Future Release Priority: normal
Severity: major Version: 5.2
Component: Revisions Keywords: needs-patch
Focuses: administration Cc:
PR Number:

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

Change History (3)

#2 @SergeyBiryukov
8 months ago

  • Component changed from General to Revisions
  • Focuses administration added

#3 @SergeyBiryukov
8 months ago

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

#4 @archon810
4 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 https://core.trac.wordpress.org/ticket/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.

Version 1, edited 4 months ago by archon810 (previous) (next) (diff)
Note: See TracTickets for help on using tickets.