User without post lock can overwrite changes of user with lock via autosave

[jhart35]

I work on a website with a large number of writers and editors. We've had issues with users reporting that titles, content, etc. have been reverting unintentionally. We tracked the issue down to a situation in which a user has had the post taken over on them, but doesn't close the tab, and the tab, despite showing the Takeover modal, continues to autosave in the background, overwriting whatever the next user is doing.

The issue is here: ​https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/rest-api/endpoints/class-wp-rest-autosaves-controller.php#L219-L230

There should be a check for the current user having the post lock before saving the autosave data directly to the database.

[adamsilverstein]

@jhart35 thanks for raising this issue and for the instructions on how to reproduce. I'm not sure if the fix belongs in the REST controller or in Gutenberg where the autosave is triggered (and maybe shouldn't be). Altering the REST API endpoint might still result in overwriting if the post lock saving collides with a save.

[jhart35]

@adamsilverstein Thanks for the reply! There's another issue coming for Gutenberg. I first attempted to fix the problem there, but listening for the post lock takeover and then setting a lock on autosaving. But the lock wasn't respected and the post continue to autosave. I found that there were perhaps more permutations there than I wanted to try and account for.

For our website, I ended up adding a filter to rest_request_before_callbacks to check if the user had the post lock and, if not, returning an error. My gut says that the safer answer (that doesn't show an error to a user like my solution) is to add an extra conditional on Line 223 in the above referenced file and just create an autosave rather than calling wp_update_post when the saving user doesn't have the lock.

On the Gutenberg side, I did find that, due to timing issues, the coupling of an autosave call when the takeover appears alone can cause a similar issue where the user taking over misses some saved changes and then overwrites them. But certainly, it seems like somewhere, WP should be checking the post_lock before actually updating the post.

[sathyapulse]

I can reproduce the problem with the steps mentioned above with both WordPress 5.9 and 6.0 versions. I tested the changes with the patch 55659.diff and confirmed that it addresses the issue with autosaves.

Adding onto the fix added by ​sathyapulse . Check for the existence of the wp_check_post_lock function and require the file necessary if we don't have it. The unit test provided can confirm the fix works properly.

Fixes https://core.trac.wordpress.org/ticket/55659

[primetimejas]

I was able to replicate the issue and verify that the fix works. The PR was created to add onto what @sathyapulse contributed. In the meantime we are using the hotfix below, props to @sathyapulse for help with the interim fix as well! I work on a project with a lot of editors, so this has been an issue for us for awhile, we are really happy to have a fix.

<?php
/**
 * Fix autosave updates issue.
 *
 * @see https://core.trac.wordpress.org/ticket/55659
 *
 *
 * @param WP_REST_Response|WP_HTTP_Response|WP_Error|mixed $response Result to send to the client.
 *                                                                   Usually a WP_REST_Response or WP_Error.
 * @param array                                            $handler  Route handler used for the request.
 * @param WP_REST_Request                                  $request  Request used to generate the response.
 *
 * @return mixed|\WP_Error
 */
function fix_autosave_post_updates( $response, $handler, $request ) {

        // Check for the response ID.
        if ( empty( $request['id'] ) ) {
                return $response;
        }

        // Ensure we have a valid post.
        $post = get_post( $request['id'] );
        if ( ! $post instanceof WP_Post ) {
                        return $response;
        }

        // Check that we are in the correct context.
        if ( 'POST' !== $request->get_method() ) {
                        return $response;
        }

        // Ensure this request is coming from the autosaves route.
        $autosave_route = '/wp/v2/' . get_post_type( $post ) . '/' . $post->ID . '/autosaves';
        if ( $autosave_route !== $request->get_route() ) {
                return $response;
        }

        // The wp_check_post_lock function does not exist until we include it.
        if ( ! function_exists( 'wp_check_post_lock' ) ) {
                        require_once( ABSPATH . 'wp-admin/includes/post.php' );
        }

        $user_id = get_current_user_id();

        /**
         * If this is draft, the post author is the current user and the post is locked then return an error, stopping autosave.
         * Note: Only the original author can autosave, this is defined in class-wp-rest-autosaves.controller.php
         *
         * wp_check_post_lock() will return false if the post is locked to the current user, so if it is locked to the current user we can continue with autosaving.
         *
         * So in a scenario where the post has been taken over by another user "User B" but the original author left their browser tab open:
         *      wp_check_post_lock() would return false for "User B" that has taken over the post since the post is not locked for them.
         *      wp_check_post_lock() would return the user ID of "User B" for the original author since the post is locked to "User B".
         *
         * Our issue happens because core does not check post lock and the original author's browser tab initiates an autosave, overwriting changes from "User B".
        */
        if ( ( 'draft' === $post->post_status || 'auto-draft' === $post->post_status ) && (int) $post->post_author === (int) $user_id && false !== wp_check_post_lock( $post->ID ) ) {
                        return new WP_Error(
                                        'rest_forbidden',
                                        'Sorry, you are not allowed to update the post with autosave since it is locked by another user.',
                                        array( 'status' => 400 )
                        );
        }

        return $response;
}
add_filter( 'rest_request_before_callbacks', 'fix_autosave_post_updates', 10, 3 );

[joemcgill]

Was showing this to @kadamwhite and @TimothyBlynJacobs at WCUS contributor day and we were able to confirm the bug. As @adamsilverstein mentioned earlier, I think it's debatable whether this is the responsibility of the REST controller or if it should be the responsibility of the editor to disable autosaves, but this seems like a valid approach.

[kadamwhite]

The patch makes sense to me and I think it's reasonable to add this safeguard to the API whether or not it gets additionally fixed on the editor side.

[antpb]

In 54130:

Autosave/REST API: Block autosaving from overwriting changes when locked from editing.

Previously when a user was locked from editing a post in the block editor, autosave functionality was allowed to overwrite changes made by the editor that has taken control. This patch honors the lock status keeping autosave from conflicitng with other content editors.

Props jhart35, adamsilverstein, sathyapulse, chanthaboune, primetimejas, joemcgill, kadamwhite.
Fixes #55659.