Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#46671 closed defect (bug) (fixed)

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

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


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

if ( ! $post = get_post( $revision->post_parent ) )
// ...
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

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 (4)

46671.diff (2.7 KB) - added by markparnell 4 years ago.
Renamed $post variable to $parent to avoid conflicts with global variable
46671.2.diff (3.2 KB) - added by markparnell 4 years ago.
46671.3.diff (3.7 KB) - added by SergeyBiryukov 4 years ago.
46671.4.diff (744 bytes) - added by SergeyBiryukov 4 years ago.

Download all attachments as: .zip

Change History (14)

#2 @SergeyBiryukov
5 years ago

  • Component changed from General to Revisions
  • Focuses administration added

#3 @SergeyBiryukov
5 years ago

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

#4 @archon810
5 years 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 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:

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


Version 1, edited 5 years ago by archon810 (previous) (next) (diff)

4 years ago

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

#5 @markparnell
4 years ago

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

#6 @markparnell
4 years ago

  • Keywords needs-testing removed

#7 @tofandel
4 years ago

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

global $wp_query;

$post = $parent;
$wp_query->posts = [$post];
$wp_query->post = $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 4 years ago by tofandel (previous) (diff)

#8 @markparnell
4 years 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.

4 years ago

#9 @SergeyBiryukov
4 years ago

  • Milestone changed from Future Release to 5.5
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#10 @SergeyBiryukov
4 years ago

Just noting 46671.2.diff didn't work in my testing, as there are some other functions in the file depending on the $post global:

  • get_edit_post_link()
  • _draft_or_post_title()
  • wp_print_revision_templates()

46671.3.diff seems to work, however it also appears to be a lot of effort to move away from the global that we ultimately have to set up again.

I don't think setup_postdata() is necessary here, or that wp_reset_postdata() not working as expected is a concern here, because the hooks in wp_insert_post() and wp_transition_post_status() should have all the necessary context without relying on the $post global. Since these hooks are not in the WordPress Loop, any functions hooked to them should not expect the $post global to be available.

So it looks like the only thing to fix here is to make sure core doesn't break if a plugin sets up the $post global for its own purposes, either intentionally or accidentally, as in comment:4.

46671.4.diff does just that, using the approach already applied in wp-admin/edit-form-blocks.php.

#11 @SergeyBiryukov
4 years ago

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

In 48625:

Revisions: Ensure the global $post remains the same after revision is restored.

Because wp_insert_post() and wp_transition_post_status() are called during the process, plugins can unexpectedly modify $post.

Props markparnell, tofandel, archon810, SergeyBiryukov.
Fixes #46671.

Note: See TracTickets for help on using tickets.