Make WordPress Core

Opened 11 years ago

Last modified 4 years ago

#24447 assigned defect (bug)

Avoid losing data after nonces expire

Reported by: azaozz's profile azaozz Owned by: iseulde's profile iseulde
Milestone: Future Release Priority: normal
Severity: major Version:
Component: Administration Keywords: needs-patch early
Focuses: Cc:

Description

Happens when an admin page containing a form is left open for more than 24 hours and the user decides to submit the form. This is quite rare for most admin pages as the users typically spend short time there. However this can happen on the Edit Post screen too despite that we refresh the basic nonces every wp_nonce_tick (12 hours):

  • The user starts new post.
  • At some point the Internet connection is lost.
  • The user decides to finish later and puts the computer to sleep (closes the laptop, etc.).
  • The user decides to continue writing more than 24 hours after that.

At this point all nonces have expired and cannot be updated as we've missed the previous nonce_tick update.

Attachments (3)

24447.patch (649 bytes) - added by johnbillion 11 years ago.
24447.2.patch (7.8 KB) - added by iseulde 9 years ago.
24447.3.patch (8.6 KB) - added by iseulde 9 years ago.

Download all attachments as: .zip

Change History (40)

#1 @azaozz
11 years ago

If a user hits the AYS (nonce expired) after trying to save from the Edit Post screen, the post data would be saved in session storage. Clicking the AYS link or the back button will load the screen and offer to restore the post. If another user is currently editing the same post, this won't be accessible until the current user takes over.

If we refresh the basic nonces (regardless that they have expired), another user may have edited and/or published the post and the current screen will not reflect that. Saving the post will overwrite it to its current state. Also refreshing the nonces will not refresh any nonces added by plugins.

For all other screens we can (at least) warn the user that the page has expired and needs to be reloaded before any changes can be saved.

#2 @azaozz
11 years ago

Thinking more about this especially for the Edit Post screen: refreshing the nonces fails every time the user's computer goes offline (or to "sleep") whitin 12 hours of loading the screen and stays offline for at least 12 hours (so the total time since loading the screen exceeds 24 hours).

In this case we don't refresh the nonces as we cannot check the old values. Possible solutions:

  • Add a "grace period" for some nonces.
    • May have security implications.
    • Would not cover nonces added by plugins.
    • Even if we extend certain nonces' life to lets say 48 hours, they would still expire and some users may still loose data or at least see the AYS screen.
  • When nonces have expired, ask the user to enter his/her password and override them.
    • Will work on form submission, would be harder to do for ajax requests.
    • May cover nonces added by plugins but not for ajax.
  • Show an error that the page has expired including a link to open the same screen in a new window so the user can copy/paste any unsaved content.

All three options are more or less lame and/or don't solve this completely.

There are other implications of keeping a page open for a long time: a post may have been edited by another user or a setting may have been changed and the current screen won't show this. So even if we make it possible to save changes after an extended period, the user may be overwriting or deleting data. In that terms the third option looks like the right one, perhaps in combination with one of the others.

Other suggestions welcome.

#3 follow-up: @knutsp
11 years ago

Show an error and present the user with two options:

  1. Log in and save the content as a new draft
  2. Log in and discard the content

If option 1 is selected, after login, redirect the user to this draft in the editor.
If option 2 is selected, after login, redirect the user to the current post in the editor.

#4 @aaroncampbell
11 years ago

  • Summary changed from Avoid loosing data after nonces expire to Avoid losing data after nonces expire

#5 in reply to: ↑ 3 @azaozz
11 years ago

Replying to knutsp:

Show an error and present the user with two options:

  1. Log in and save the content as a new draft
  2. Log in and discard the content

Yes, that will work.

Nonces expiration is different than login expiration. The user can still be logged in and the nonces may have expired. In this case 2 can proceed and reload the page.

If 1 is selected we ask for the user's password again and override nonce checking (see the second option in the previous comment). The password will be submitted with the form so we will be logging the user in (or verifying the password) at the same time as saving the submitted form data.

If we go this way we will have to "pre-verify" the entered password in case the user makes a mistake, doable with ajax.

#6 @azaozz
11 years ago

Currently the most "workable" solution for the Edit Post screen is to use session storage to save the title and content (this happens anyway), warn the user and reload the page to refresh the nonces, then offer to restore from session storage. This is pretty much how it works now except the user won't see the AYS screen.

For all other screens we should warn the user (with a modal dialog) and maybe disable any submit buttons.

#7 @johnbillion
11 years ago

A few of us were discussing this in #wordpress-dev after the meeting tonight.

We could fetch a new nonce via an AJAX call that authenticates the user and returns a nonce value for the required action(s). Currently this happens during autosave but you only get a new nonce if your current one is within the expiry period (12-24 hours).

The current autosave nonce needs to be checked here to verify intent to make an autosave, but the other nonces don't need to be verified. Authentication is enough.

We should generate new nonces for update_post, autosave, and whatever others there are (metaboxes etc). The autosave then fires again immediately with the new autosave nonce.

Thoughts? Does this inadvertently circumvent the autosave intent? I don't think it does. It's no different to requesting the edit screen and grabbing the nonce out of the HTML.

Thoughts?

@johnbillion
11 years ago

#8 @johnbillion
11 years ago

24447.patch removes the superfluous nonce check when refreshing nonces.

The nonce refresh has moved into the heartbeat call (it was previously in the autosave call), so I'm not sure how we could immediately re-fire the autosave if its nonce has expired.

Last edited 11 years ago by johnbillion (previous) (diff)

#9 @azaozz
11 years ago

  • Milestone changed from 3.6 to Future Release

Not a regression or new bug, lets revisit in 3.7.

#10 @vloo
10 years ago

I'm not quite sure but this seems like a duplicate of something 5 years old: #9604

Last edited 10 years ago by SergeyBiryukov (previous) (diff)

#11 @SergeyBiryukov
10 years ago

#9604 was marked as a duplicate.

This ticket was mentioned in Slack in #core-editor by iseulde. View the logs.


9 years ago

#13 follow-up: @iseulde
9 years ago

  • Keywords needs-patch added

This something we'll have to deal with before looking into ajax updating/saving for the editor. But this is a very annoying issue regardless. It's not a very good UX if you come back to your post the next day, make some edits, hit save and then see "Are you sure you want to do this?" and panic.

#14 in reply to: ↑ 13 @jdgrimes
9 years ago

Replying to iseulde:

It's not a very good UX if you come back to your post the next day, make some edits, hit save and then see "Are you sure you want to do this?" and panic.

Related: #27916

#16 @iseulde
9 years ago

  • Milestone changed from Future Release to 4.3

Moving for consideration, just like #33098.

#17 @krogsgard
9 years ago

The exact issue that @iseulde notes is what happened to me. I was writing a post, and I guess I stopped for a long time (hours to maybe a day) and came back to it and started writing again. When I attempted to save, I got the "are you sure" page, and freaked out.

The link on that wanted me to go to post-new.php iirc, which scared me and I didn't know how to get back. Eventually, I went to edit posts and then the post editor, and the post had reverted to the previous saved version, which lost about 2,000 words. I downloaded the database and attempted to check Chrome cache and there was no sign.

Oddly enough, the next day, when I was pasting in from an outside editor (having learned my lesson now), when I attempted to save this time it prompted me that I had a newer version than the one showing, and tada, it was back. But I know it was not an actual version bc I had downloaded the whole db to check. @iseulde tells me it was probably in the session storage, not chrome cache (which is what I checked).

The whole situation was very scary and what I really wish was if, instead of the "are you sure" page there was a different style of notification that doesn't make you leave the current page. I think that's how it's supposed to work but apparently there are some edge cases.

#18 @iseulde
9 years ago

The patch on #33098 should fix this issue also.

@iseulde
9 years ago

#19 @iseulde
9 years ago

Experimenting a bit with nonce refreshing being part of the Heartbeat API and being automatic for nonces that are added with wp_nonce_field(). Solves both this issue and #33098.

Last edited 9 years ago by iseulde (previous) (diff)

@iseulde
9 years ago

#20 follow-up: @iseulde
9 years ago

Added an example for refreshing nonces that are not fields. (wp.media.view.settings.nonce.sendToEditor)

#21 in reply to: ↑ 20 @jdgrimes
9 years ago

Replying to iseulde:

Added an example for refreshing nonces that are not fields. (wp.media.view.settings.nonce.sendToEditor)

Related: #29312

#22 @obenland
9 years ago

  • Owner set to iseulde
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core by mark. View the logs.


9 years ago

#24 @iseulde
9 years ago

  • Keywords early added
  • Milestone changed from 4.3 to Future Release

#25 @majick
9 years ago

Is there any reason to not simply output the posted data - especially the post content - to the screen if the user hits the "Are you sure you want to do this" screen? At least that way they can save any data locally before hitting back or try again - both of which could be bad moves and lose the data... at least this way it mitigates any ensuing panic...

I know from experience this has happened to me many times, and for this reason I've gotten in the practice of *at least* copying the post content to the clipboard before hitting Update to be safe - but this precaution wouldn't be necessary for the end user if the raw post data - again it's mostly the post content - was directly available after submission anyway - without having to rely on the browser cache or more complex solutions which may not ever be 100%. my 2c.

#26 @iseulde
9 years ago

I know this happens to many people, and I'm sorry to hear that.

Is there any reason to not simply output the posted data - especially the post content - to the screen if the user hits the "Are you sure you want to do this" screen?

That would not be a good experience. It'd be better to keep the user on the same page, and warn them, and maybe disable any submit buttons, if we detect nonces that nonces expired through Heartbeat. Or we could, on submit, load the same screen again if nonces expired and either offer to restore locally saved data, or load that data.

Even better would be to keep nonces alive, but this seems to need more discussion.

#27 @majick
9 years ago

Outputting the raw post content may not be a good user experience but neither is the current "Are you sure you want to do this?" screen, which is just unhelpful and out-dated. I agree returning the user to the post writing screen and letting them know the post has not been saved would be better than this, but the locally saved data may still not be as current as the submitted data.

To avoid end user confusion and panic, another alternative solution would be to send the user to a post revisions screen which shows the saved content next to the newly posted content for comparison, inform the user explicitly that their last update was NOT saved, and give the option to update the post with the newly posted data or discard it. Probably easier said than done, but certainly closer to ideal in preventing data loss. I guess my point is, if nothing is actually done with the posted data, something could still be lost when there really is no reason for that to happen.

I realize this ticket is really about nonces and how they could be improved in this case, this alternative could be split off as a new ticket for discussion if it's getting too off-topic. I will leave the nonce side of things as I admit I'm not familiar enough with them to really contribute much on that front.

#28 @pento
8 years ago

Because it's probably going to come up, just noting that this bug got an indirect mention in footnote 4 in this WBW post.

Normally when I write posts, I’m on the internet and WordPress saves the post periodically as I go. Just to be safe, I also copy and paste it into a Word doc every couple paragraphs. In this case, there was no internet because I was on a flight and go-go charges $3,250 to connect to the internet, and I happened to go the whole flight without backing up to a Word doc. At the end of the flight, I closed the laptop, and when I opened it to keep working later that day, somehow WordPress had reverted to the version it had last time it saved and I lost everything I had done on the plane.

#29 @valentinbora
4 years ago

  • Severity changed from normal to major

#30 @valentinbora
4 years ago

  • Milestone set to Future Release

#31 @valentinbora
4 years ago

  • Milestone changed from Future Release to 5.5

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 years ago

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 years ago

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 years ago

#35 @davidbaumwald
4 years ago

@valentinbora Is this still on your list to address early in 5.5?

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 years ago

#37 @davidbaumwald
4 years ago

  • Milestone changed from 5.5 to Future Release

With the WordPress 5.5 cycle officially kicked off, this is being moved to Future Release. If any maintainer or committer wants to assume ownership during a specific cycle, they can update the milestone at that time.

Note: See TracTickets for help on using tickets.