Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

If WordPress user session expires on post edit page, Gutenberg autosave continues resulting in 403s #13509

Open
jayhill90 opened this issue Jan 25, 2019 · 7 comments · May be fixed by #32475
Open
Assignees
Labels
[Feature] Saving Related to saving functionality REST API Interaction Related to REST API [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@jayhill90
Copy link

Describe the bug
If a user's session expires while editing a post, Gutenberg continues to make POST requests to the autosave endpoint.

To Reproduce

  1. Create a new post with some blocks.
  2. Open up chrome inspect tool with LogXMLHttpRequests enabled
  3. Expire your WordPress login session or update the password to do so.
  4. See errors.

Expected behavior
If a session expires, WordPress should no longer try to autosave the post.

Screenshots

Desktop (please complete the following information):

  • OS: OSX
  • Browser Chrome
  • Version: 71

Additional context

  • Running WordPress 5.0.2
@nateinaction
Copy link
Contributor

+1 I would expect the behavior of the editor to stop making failing POSTs to the site and instead give me the option to reconnect.

@gziolo gziolo added [Feature] Saving Related to saving functionality REST API Interaction Related to REST API labels Feb 8, 2019
@jayhill90
Copy link
Author

Users get blacklisted from their hosting provider servers when tabs get left open and sessions expire, thus driving contacts to the respective hosting providers. How can we get this prioritized?

@desaiuditd
Copy link
Member

@jayhill90 @nateinaction Can you please test this scenario in latest Gutenberg plugin? I checked this and I'm not seeing autosave request triggering once the nonce is expired or cookie is cleared. Heartbeat requests are triggered, yes. But not the autosave requests. Can you confirm?

@desrosj
Copy link
Contributor

desrosj commented Jul 18, 2022

Came across this one after @Mamaduka pointed out the issue I opened was a potential duplicate (which I think I agree on).

Here are steps to reproduce this problem.

  1. Log in to an install.
  2. Create a new post.
  3. Open browser console and head to Storage tab (in Firefox, may be different in other browsers).
  4. Select cookies and delete the wordpress_logged_in_MD5HASH cookie where MD5 will represent an MD5 hash of the site URL.
  5. Navigate in the editor, add blocks, etc. (I added an image block and selected an image from the media library) and observe repeated failed requests.

Another scenario to reproduce this is detailed in the ticket that I opened. (#42400)

@Mamaduka
Copy link
Member

Mamaduka commented Aug 10, 2022

Thanks for the detailed reproductions steps, @desrosj. The key for consistently reproducing the issue was only to delete wordpress_logged_in_MD5HASH and not other cookies.

The problem

  • When wp.apiFetch catches rest_cookie_invalid_nonce error it tries to refresh the nonce. See API Fetch: refresh nonces as soon as they expire, then fetch again #16683.
  • If it fails, the rejected promise is returned and calls terminated.
  • If refreshing succeeds, we recursively call wp.apiFetch and return the new promise.
  • With partially missing cookies, nonce refresh endpoint can still return results, and we get an infinite loop.

Possible solutions

  1. Update wp_ajax_rest_nonce to return error if wp_get_session_token is empty.
  2. Track nonce refresh retries in wp.apiFetch and reject promise after max retries.

Pinging a few folks who collaborated on #16683 for more visibility. Cc. @ellatrix, @gziolo, @kadamwhite.

@desrosj
Copy link
Contributor

desrosj commented Aug 25, 2022

Could both solutions be implemented? I'm not familiar with wp.apiFetch, but it seems reasonable to return an error in wp_ajax_rest_nonce() and retry a few times within wp.apiFetch on a failure.

Also, I noticed that wp_ajax_rest_nonce() is the only function in wp-admin/includes/ajax-actions.php that does not use wp_send_json_success()/wp_send_json_error() and instead just uses exit() to simply print the nonce value. I was curious if there was a reason behind this.

Finally, there is a mechanism already in Core for refreshing nonces attached to the Heartbeat API. wp_ajax_heartbeat (source) has a wp_refresh_nonces filter that could be used to add the REST API nonce to the response (see how the heartbeat and post nonces are refreshed).

Addressing these last two points may warrant a new ticket, but if it's not too much of a lift, addressing these items at the same time may be beneficial.

@matzeeable
Copy link

matzeeable commented May 26, 2023

I was able to reproduce the error at one of our customers. He deletes browser cookies from time to time to test certain things. As soon as the cookie is deleted starting with wordpress_logged_in, one runs into a REST API continuous loop.

image
image

I do not know if this can be solved with a check of wp_get_session_token as the error needs to be handled on the frontend.

Track nonce refresh retries in wp.apiFetch and reject promise after max retries.

I think, this is the correct approach. When retrieving rest_cookie_invalid_nonce a second time, we should show a dialog for the user login:

image

Programmatically, this could be solved like this:

let initializedAjaxSend = false;
function waitForValidLogin() {
    if (!wp?.heartbeat) {
        return Promise.reject();
    }

    const $ = jQuery;

    // Trigger the WordPress auth dialog to be shown
    // Unfortunately, wp-auth-check.js does not allow to trigger this via another API like `wp.auth.showLoginDialog()`
    // See https://github.com/WordPress/WordPress/blob/d4c39df30839133f3b9a46b01a229233d8f99f10/wp-includes/js/wp-auth-check.js#L162
    $(document).trigger("heartbeat-tick", [{ "wp-auth-check": false }, "error", null]);

    // Suspend all heartbeat requests as they are not needed atm
    // See https://github.com/WordPress/WordPress/blob/d4c39df30839133f3b9a46b01a229233d8f99f10/wp-includes/js/heartbeat.js#L226-L228
    // $(window).trigger("unload.wp-heartbeat");
    // Unfortunately, we cannot use this event as when the user focus the window again, it requests the admin-ajax.php again
    if (!initializedAjaxSend) {
        initializedAjaxSend = true;
        $(document).ajaxSend((event, jqXHR, { url, data }) => {
            if (
                url &&
                data &&
                url.endsWith("/admin-ajax.php") &&
                data.indexOf("action=heartbeat") > -1 &&
                $("#wp-auth-check-frame").length > 0
            ) {
                jqXHR.abort();
            }
        });
    }

    // Unfortuantely, wp-auth-check.js does not trigger events when auth dialog gets hidden
    // See https://github.com/WordPress/WordPress/blob/d4c39df30839133f3b9a46b01a229233d8f99f10/wp-includes/js/wp-auth-check.js#L110
    return new Promise((resolve) => {
        const checkInterval = setInterval(() => {
            const element = $("#wp-auth-check-frame");
            if (element.length === 0) {
                clearInterval(checkInterval);
                resolve();
            }
        }, 100);
    });
}

Usage:

waitForValidLogin.then(() => {
  console.log("we have a valid login now");
});

Also, I found out that the heartbeat admin-ajax.php response a nonces_expired property. Perhaps this could be useful for future development.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Saving Related to saving functionality REST API Interaction Related to REST API [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants