Problem/Motivation

When a user attempts to submit an expired form, the user receives the error "This form is outdated. Reload the page and try again." There are several problems with this message:

  1. Reloading the page does not actually resolve the issue.
  2. The message is confusing: Outdated how and why? Try what again?

Steps to reproduce:

  1. Go to node/add/article and fill out the form without submitting it.
  2. In another browser tab, log out of the site and log back in.
  3. Now try to submit the form. You'll get the error, and if you simply follow the instructions and reload the page, then you will continue to get the error forever.

Proposed resolution

At a minimum, the error message needs to be updated with a more accurate description of the problem and especially accurate instructions on how to proceed.

There are two possibilities:

  1. Regenerate the form
    • Inform the user of the issue.
    • Force the user to regenerate the form
    • Provide a link to do this in the error message.
  2. Generate a new token
    • Inform the user of the problem
    • Retain the user's form values
    • Generate a new token
    • Warn the user about the security implications
    • Prompt the user to either review and confirm the submission or to generate a new form (as in #1).

#1 is clearly undesirable for usability (data could be lost).

#2 has the following potential issues:

  • It risks further confusing the user
  • It may introduce CSRF vulnerabilities
  • It is not clear to the user what the implications of submitting the form are
  • Hidden and similar form elements cannot be inspected by the user to ensure they contain intended values.

Based on the security implications, the consensus is to use solution #1 for now to fix the problem. Patch in #119 implements this change.

Remaining tasks

  • The error message is broken enough that we are introducing a string change (see #114 and #115).
  • Patch in #119 needs review and sign-off from UX maintainers.
  • Once this fix is committed, consider options for a better UX for this in the long term.

User interface changes

String change. Both the error text and workflow for dealing with expired forms will be changed.

Old error:
t('This form is outdated. Reload the page and try again. Contact the site administrator if the problem persists.')
Screenshot of old error

New error:
t('The form has become outdated. Copy any unsaved work in the form below and then <a href="@link">reload this page</a>.', array('@link' => $url))
Screenshot of new error

API changes

None.

Original report by @Simx0r

Got the strangest thing on Drupal 6.1. This error occurred by editing ANY block/page/setting:

Validation error, please try again. If this error persists, please contact the site administrator.

Did not install/remove any modules or settings on the site itself. Only changed an existing block containing some html button with standard code, wich generated this error. When looking at the logfile the following 'out of the ordinairy' 404 appears wich I haven't seen before:
sites/default/files/languages/nl_......js
Again, did not change anything on the Drupal site itself, except editing a existing block with no special code.
Can't delete or edit anything on the site.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

teledyn’s picture

Same here, just now, just today, edited a comment to correct a link, and bam, the site is now read-only apparently. Curious there are no further comments on this thread; google reports 811 reports of this error

teledyn’s picture

this post reports that the problem is transient, that it just "heals itself"

which is a phrase that always reminds me of DiNiro's character in Terry Gilliam's "Brazil" ;)

There are some further links deeper into this website beginning from this post, but they all appear to label the issue as "spooky action at a distance".

This bug report suggests the bug has been in Drupal as far back as 4.x, but even as recent as the 6.x series there does not appear to be any clear consensus on the solution although there is a recommendation to upgrade to the CVS head edition, which would mean the loss of many key modules for many of us. I have requested a work-around fix, let's hope one exists.

seanlee’s picture

I have the same problem. I cannot add/remove any new or existing modules or blocks. Almost EVERYTHING results in a validation error now. This is after I created a custom block with some php code to include ads.

*UPDATE*

I posted a fix for this at http://drupal.org/node/247039#comment-808713
This isn't a bug fix, but rather a work-around that gave me my site back.

-Sean

Simx0r’s picture

Thanks for the tip seanlee. It seems that even correct php code can do nasty things to a website. Since I also use OpenX code (and my website got stuck) this could relate to eachother.

Tpainton-1’s picture

Version: 6.1 » 5.7

Wonderful.. Well I wish it would "JUST HEAL ITSELF" because Im very busy and in trying to send out my products THE LAST THING I NEED IS TO BE HELD UP BY THE CMS!!.

I just got done dealing with crap from my ISP in which I lost 1000's of dollars in sales and I finally get that fixed and now I start havin this error today with Drupal 5.7 and it seems noone know's what the problem is.. and furthermore it goes all the way to 4.7?!?!.

Great.

Hopefully it "Heals itself" in the next 5 minutes so I can get the 1-2 hours of invoicing work done in Ecommerce in time to actually get DONE BY DINNER TIME.

Sorry but this is quite frustrating. Very tough to update client records when I keep getting this ERROR!

drumm’s picture

Status: Active » Closed (works as designed)

Drupal's Form API checks if a form was indeed served to a user by checking a unique token embedded in a hidden field. This can deter spammers, cross-site request forgery attacks, and various other automated trouble. When reporting these issues, we need to know:

* What form were you on? Include the URL; feel free to block out your domain name for privacy.
* Were you logged in or not?
* How is your Drupal caching configured?
* Is there anything else between you and the server which might do caching or otherwise alter the page? Try a different web browser or computer.
* What web browser and version are you using?

If these things are different, chances are you all have different issues which need to be filed separately, despite having the same error message. Include anything else which might be relevant, reviewers will have to attempt to recreate the same error with their own test site and browser.

tonycaine’s picture

I had a similar issue/problem.
using 5.7 and on the blocks page moved an unassigned block to the footer.
resulting in the 'validation error..."

The error persisted when i moved the block back to unassigned and then as i tried to move any other block.

In frustration i both bounced the server and then logged off and back on.
That cleared something and now there is no error and i've moved the block as i first tried.
suppose that's "JUST HEAL(ing) ITSELF" .

cheers all

cbovard’s picture

subscribing

Tpainton-1’s picture

Not sure what this is worth, but if I get this problem, I circumvent it by simply moving to another client computer. This always works. It also explains why customers have called me complaining of the error and yet, I can log on, and do things on the server that they can't do at the exact same time. It sems to be a problem with the connection, not the server.

MGParisi’s picture

Does this infact "Heal itself"? Can anyone confirm this? If so how long do I have to wait!

MGParisi’s picture

If this is BY DESIGN... Then how the hell do I fix it!

Flying Drupalist’s picture

Can we have a list of issues or actions that may prompt this message?

MGParisi’s picture

This is a tough one, and why I didn't have much details. I know I got the "http://drupal.org/node/255972" error and have since uninstalled Abuse for now. I was basically doing a bunch of minor fixes going through lots of modules. I must be running over 100 modules. I did setup a minipanel to use a pager ID of 1 (something I have never done before) so I will see if I can go back and set that to 0. II noticed (at some time) that a user profile (bio) started to show the HTML on his page

I was working in Panels, playing with views. I have a number of views (12) on one page () with a number of views that are inserted because of different modules.. I am also using views add-on and managed FCKEditor and BUEditor at the same time Im testing so Cache scares me (lol), and I am using and upgraded the Live module and also running Abuse which is still giving me http://drupal.org/node/255972 error until I disabled and uninstalled it (after the problem was noted).

I installed image_field Gallary for a moment (not the light box part), but realized that Lightbox2 meets my needs better then that.

My symptoms also include a lack of content and access to content for non-admins. Admins can use the entire site without issue. I have checked permissions, and nothing stands out, though I don't remember ever changing anything either. Early in the day I switched logintoboggan to assign password after email verification (because I found this to be an easier way to verify email) and then during the issue I deleted that access profile,

The only log error I am getting is
"phptemplate.engine was instructed to override the advanced_profile_view_buddylist_of_uid theme function, but no valid template file was found."

Which I haven't looked into (this one is new), and I use to get a message about a missing /themes/zen/text.css that went away.

The one error I do KNOW that it started after was the Abuse error message.

MGParisi’s picture

It is interesting that the 1st responder mentions a failure in editing a comment, and mine was a failure in deleting one do to abuse module. I hope this provides all the info you need.

BTW "phptemplate.engine was instructed to override the advanced_profile_view_buddylist_of_uid theme function, but no valid template file was found." must be an old problem, becuase that view hasnt worked for weeks. That was on my todo list. I got this message every time I add a comment.

El Viejo’s picture

Don't know if it's helpful, but I got the error on a 6.3 development site with OpenX 2.4.8 integrated using the "openads" module. Everything was working just fine until I began attempting to use various options to call ads in blocks via php native to the plugin, or by options other than javascript generated by OpenX as the invocation code.

I do have the plugin within Drupal configured to specify javascript as the method for invoking ads. (At this point I'm not about to touch that to experiment. We'll live with javascript for now).

I cleared the problem by going to the boxes table and simply deleting the body content from the records with invocation code other than javascript. We immediately got the site back, and replacing the code in the now-empty blocks with a javascript invocation from OpenX restored the ad code that apparently caused the problem in the first place.

I did not delve into the Drupal code much, and I don't plan to. It does feel somewhat like there may be an issue with php calling php somewhere in there. We had a sql error simultaneously registered that suggests that the select statement was incompatible with mysql 5.x, which we are using. That seems unlikely, but ....

defconjuan’s picture

Version: 5.7 » 6.4

I got this issue after install of the OpenX and OpenX Manager modules - but not immediately. I installed, configured, and tested it and all was working. Woke up the next morning and got this Validation Error. I updated some modules which updated last night (cck, imagefield, filefield) then I tried again but got the same error. I then left the module updates in place and disabled the OpenX and OpenX Manager modules manually and poof - the message was gone. I want to use these mods but for now, I will just use the OpenX invocation code (sort of like Google Adsense) as a content block until this gets sorted.

khan2ims’s picture

Run the Cron Job. Yes, thats what I do whenever I come acroos the validation error. Simply run the cron job and you will see the error/ warning going away in fumes :-)

But it needs a more reliable reason as to wht it happens !

kyle_mathews’s picture

Huh. . . this is the weirdest problem. I moved a site from my laptop to the production server and now, all of a sudden, I can't submit *any* forms on my site as they're all suffering from a "Validation error".

Also, I can't log out now either. . . I'm just redirected to the front page. Not sure what's going on here. . .

patricksettle’s picture

I'm pretty sure this is caused by your current session being hosed, or your sessions table is. Attempt to log out and then log back in, or empty the session table in the database, the log back in.

This of course leads to the question what's messing with the session… and why does it think I'm logged in when I'm not?

For that I have no answer at the moment.

patricksettle’s picture

A tweet from eaton also suggests that it may be a messed up anti-CSRF token ( stops x-site nastieness ). When I cleared out my sessions and got a shiny new one I would have gotten a new anti-CSRF token along with it. Which is why it all worked again after I logged back in, and also seems for some to "fix itself" which is really just them getting a new session.

Still though, I don't know what in my setup/usage at the time caused the mix up. Multiple tabs in firefox logging out/in in different tabs? *shrug*

Sholex’s picture

Version: 6.4 » 6.8
Component: base system » poll.module

I had this problem with the Poll module. Users could not vote without getting the error. I ran cron and the problem went away.

dparraz’s picture

Our culprit was Suhosin.

When viewing user permission admin page in Drupal 6.10 update, we were hitting the max var characters limitation.
-----------------------------------------------------------------------------------------
file: suhosin.ini
-----------------------------------------------------------------------------------------

; Defines the maximum number of variables that may be registered through a POST
; request.
suhosin.post.max_vars = 2048

and also

; Defines the maximum number of variables that may be registered through the
; COOKIE, the URL or through a POST request. This setting is also an upper
; limit for the variable origin specific configuration directives.
suhosin.request.max_vars = 2048

********************************************************************

Once these items were given a larger variable setting(200 by default), out validation issue's were gone.

Hope this helps someone!

pixelpreview@gmail.com’s picture

yes it seems that's the problem ... suhosin

http://drupal.org/node/346015

in local, no problems and when i put my site on my production server, i see that my server administrator has installed suhosin and post.max.vars limited to 200.

after change that, no error messages !

thanks dparraz !

MGParisi’s picture

It seems that this is no longer "by design" but instead "needs review" or some other status.

MGParisi’s picture

Status: Closed (works as designed) » Closed (fixed)

woops, wrong page!

MGParisi’s picture

Status: Closed (fixed) » Closed (works as designed)

Resetting status, I meant to edit a different ticket! OOPS!

dww’s picture

Version: 6.8 » 7.x-dev
Component: poll.module » forms system
Priority: Critical » Normal
Status: Closed (works as designed) » Active
Issue tags: +Usability

I know FAPI does this on purpose, but the usability sucks. If the form token is no longer valid for some reason, it just says "Validation error, please try again", but if you keep submitting the form, it's going to keep happening. You have to actually *reload* the page and then it starts working again. Let's call this a FAPI usability bug... There have been a lot of duplicate issues reported about this:

#336963: Validation Error
#459938: "Validation error, please try again. If this error persists, please contact the site administrator."
#503110: Validation error, please try again. If this error persists, please contact the site administrator.
... (and probably lots more)

janis_lv’s picture

Version: 7.x-dev » 6.x-dev

caused Translation module for me.

snorkers’s picture

I had this error crop up on the memory hog that is admin/build/blocks. A quick cache clear and cron run cured it, but still doesn't leave me feeling comfortable with the site currently at beta.

JayKayAu’s picture

Hi all,

I got the same thing, and like others am a teensy bit uncomfortable that this has been bouncing around for so long.

In my case, I had a views slideshow putting up some thumbnails which would open a lightbox2 image when you click on them. The change that I made that triggered this was that I changed the Content:Image field from thumb->original to iframe:thumb->node page.

I get the impression, however, that this information is probably of little use to anyone.

BTW, I ran cron, reloaded the Edit View page, and everything was fine. Didn't have to log out.

himerus’s picture

Just came across this error today on a site setup from a previous database after upgrading core and about 20 modules.

A flush cache fixed the issue for me instantly.
Dont know if I got lucky, or that would solve it for everyone.

mikel1’s picture

Version: 6.x-dev » 6.14

I get this error when I try to change *or* rebuild permissions! I do not have suhosin.

http://troop32bsa.org/phpinfo.php

Help! My site is useless, as permissions are wrong and I can't update them.

syka’s picture

Assigned: Unassigned » syka

The same problem with my installation. I can change or update permissions!!! Any Ides??
Drupa 6.14

dww’s picture

Argh. This issue is not a place for support requests when you hit this error (the solution in 99% of cases is to reload the page containing the form, not to keep pressing the submit button). This issue is about changing the WTF nightmare of usability that this error message continues to generate, as further evidenced by comments #32 and #33...

Please do not comment here if you're seeing this error message. Only comment here if you're working on fixing the underlying usability bug in Drupal that generates this error message. Thanks. ;)

dww’s picture

Title: Validation error, please try again. If this error persists, please contact the site administrator » "Validation error, please try again..." is terrible UI and confuses users

Better title to help avoid confusion about what we're doing here. ;)

Strawman proposal. The new validation error should read:

"Validation error. Please reload this page and fill out the form again. If this error persists, please contact the site administrator."

Thoughts?

snorkers’s picture

I think that's definitely a move in the right direction, as it gives users a way out - rather than offering a brick wall. A UI improvement gets my vote, as addressing the FAPI is probably not going to happen in D6.

This is a palliative, rather than a cure, however.

adshill’s picture

Assigned: syka » Unassigned

I don't think that user meant to assign this to themselves :)

Regarding the message, my feeling is you should never start with a technical description given that this is often an error an end user would see. Also - is validation error the correct way to describe it? Thats maybe another question so I'll just suggest something more like:

There has been an error submitting the form. Please reload this page and fill out the form again. If this problem persists, please contact the site administrator quoting: Validation error.

While probably needs some work I think it's better to keep the jargon to the end rather than the beginning from a usability point of view.

esolano’s picture

Subscribing..

paganwinter’s picture

Subscribing...

jbigras’s picture

Log out/in made the error go away for me. I had two browser windows and working with views. I was working with updating views and using the other browser window to see the effects. Guess it got confused. Logged out, closed all windows and logged back in. Problem solved for me.

pxwee5’s picture

I solved this on my platform ... not sure if this helps but I'll just share what I've got.

Drupal ver 6.16
Problem first noted when I pressed "back" after I did a search on Google Chrome, everything was fine in IE.
I remembered I had my cache settings all disabled and turned off.

So I tried turning all of them on.
Caching Mode "on" 10 minutes
Block Cache Enabled

Then I cleared cache and ran cron job.

Everything was fine now no problem "back-ing" after a search, no error I'm a happy man.

Anybody can confirm? Or am I being too enthusiastic myself LOL

ressa’s picture

Logging out/in also made the error, and made the error go away for me. I had logged in and out, with the Permissions page open in another tab.

When I then went to the Permissions page, made some changes and saved I got the error message. Logging out, closing all tabs and logging in again fixed it.

sepla’s picture

Title: "This form is outdated. Reload the page and try again" is incorrect and confuses users » "Validation error, please try again..." is terrible UI and confuses users
Version: 7.x-dev » 6.14
Priority: Major » Normal
Status: Needs review » Active
Issue tags: -Security improvements, -String freeze, -Security

This means that the token submitted with the form doesn't match the token that Drupal used to build the form, reload the page and re-fill form fields. This should fix the problem.

das-peter’s picture

Hi there,

I was experiencing the same issue on my drupal installation. After some investigation I realized that it has something to do with the way caching is handled. In my case the module Image FUpload (image) and Image FUpload (CCK) are affected.
The module loads the form using "form_get_cache" this method itself uses the caching system to load the form.
When loading something from the cache the caching system does this check:

if ($user->cache > $cache->created) {

And it looks like there's the issue.
if I set $user->cache to 0 before the module loads the form cache everything works.

Since I've changed the code like this, it works like a charm:

    // Load the form from the Form API cache and check if valid
    $oldUserCacheTime = $user->cache;
    //$user->cache = 0;
    $form_state = array('rebuild' => TRUE, 'values' => $_POST); // rebuild option needed to prevent that "_image_node_form_submit" gets executed by drupal_process_form
    if (!($form = form_get_cache($form_build_id, $form_state))) {
      // code based on upload.module (15/08/2008)
      form_set_error('form_token', t('Validation error, please try again. If this error persists, please contact the site administrator.'));
      $output = theme('status_messages');
      drupal_json(array('status' => TRUE, 'data' => $output));
      exit();
    }
    $user->cache = $oldUserCacheTime;

Hope that will help.

Cheers
Peter

a5342346’s picture

subscribing

jdexter’s picture

Salutations:

Not sure this provides any insights - but thought I'd kick it in as this thread was helpful.

Was editing a View from a panel and logged out - then logged in again. Tried to complete edit and it issued message. I reloaded the View edit page and still got the error. Logged out and logged in again and still got message.

Someone mentioned it messed with the sessions, so I logged out again - cleared the browser and logged in again and returned immediated to edit the View again.

When I got back to view it messaged:

"This view is being edited by user Anonymous, and is therefore locked from editing by others. This lock is 23 min 56 sec old. Click here to break this lock."

So .. broke the lock and everything came up roses from there.

Again - not sure it's helpful, but kicking it in just in case.

Cheers and best wishes.

Jon

plcoelho’s picture

Hi,

I just flushed all caches, and the error has gone.
It was flushed, also :)

Cheers,
Paulo

dpiccolo’s picture

In my case this error shows up when I have the formfilter enabled for the content type. Once disabled it doesn't show up anymore.

sun’s picture

Status: Active » Closed (works as designed)

This error does not appear under normal site operation. It means that something is really broken and you have to review and fix the forms on your site. Most probably it is caused by a bogus module.

dww’s picture

Status: Closed (works as designed) » Active

@sun: I wish you were right, but sadly, this isn't just from something being "really broken" or caused by a bogus module. All you need to do is have a window open with an issue for a few days. When you come back later and finally type your reply and try to submit your followup comment, the form cache might have been cleared and your form token is no longer valid, triggering exactly this error. "Please try again" is misleading and wrong -- you can keep pressing "submit" over and over and it'll never work. What you need to do is copy your text, reload the page from scratch and *start over* on the form submission. This is not an edge case. This is still a UX bug.

webchick’s picture

Version: 6.14 » 7.x-dev

I get very urgent e-mail from a clients every once in awhile (usually in ALL CAPS and with lots of !!!!s) freaking out about receiving a message about doing something "illegal" on the site. This threatening-sounding message that ends up showing to hapless end users from time to time due to bugs on the site or because they hit some edge case, particularly with AJAXy stuff, not because they're trying to do something nefarious. It's a useless message for module developers too, as it gives them no indication of what might be wrong and how to fix it.

It's a stupid situation, and IMO we should fix it, at least in D7.

webchick’s picture

Oh, and +100 for #35. We should probably note that doing so is going to remove their form entry thusfar though. Hm...

sun’s picture

Status: Active » Postponed (maintainer needs more info)

So does drupal.org implement a workaround for this issue? I just happened to have this situation, but was able to simply submit the form again, and it successfully submitted.

dww’s picture

Status: Postponed (maintainer needs more info) » Active

d.o has no workaround that I know of.

sun’s picture

Status: Active » Needs review
FileSize
873 bytes
dww’s picture

Status: Needs review » Needs work

Couldn't the text read:

Please <a href="">reload this page</a> ...

to give you a link to the local page (as I had in comment #35)? Or is that invalid XHTML or something? ;)

sun’s picture

Status: Needs work » Needs review

Modern browsers keep your user input upon reloading a page.

yoroy’s picture

FileSize
814 bytes

Easy on the 'pleases', please ;-) #679890: Using "Please" in the interface

sun’s picture

Status: Needs review » Reviewed & tested by the community

Heh. I was 100% sure that it would be an issue, but didn't know how to fix it. Thanks, yoroy! :)

So let's improve that error message for D7.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

David_Rothstein’s picture

Title: "Validation error, please try again..." is terrible UI and confuses users » "This form is outdated. Reload the page and try again" is incorrect and confuses users
Status: Fixed » Active

Modern browsers keep your user input upon reloading a page.

Yes, that is the problem. They keep your user input, but they also keep your out-of-date form token, so no matter how many times you reload the page you will always get the error message. To submit it, you'd need to go to your browser's address bar and physically revisit the page.

So the text here is still misleading and incorrect.

We could fix it with @dww's suggestion of linking the page to itself (although of course that results in the form being emptied out and losing all your work). In #655388: Many ways to lose data on form input in the overlay @cwgordon7 and others were working on a way to use JavaScript to persist the form data - could be worth looking at, although probably isn't directly applicable here.

Really it should be possible to do this in the form API though? (All we want is to build the same form, but with the submitted $_POST values used as the defaults, and a new token.)

****
BTW, simple steps to reproduce this issue:
1. Go to node/add/article and fill out the form without submitting it.
2. In another browser tab, log out of the site and log back in.
3. Now try to submit the form. You'll get the error, and if you simply follow the instructions and reload the page, then (on a modern browser, I tested specifically with Firefox 3.6) you will continue to get the error forever.

David_Rothstein’s picture

Status: Active » Needs review
FileSize
1.14 KB

Hm, is it actually as simple as this?

Perhaps needs more thought, but I don't think this affects the form API's CSRF protection, since we are still making sure that the form fails validation and forcing them to re-submit it manually if they actually intended to. Maybe I'm missing something stupid, though.

sun’s picture

Status: Needs review » Needs work

Needs to be #default_value instead of #value. But otherwise, perhaps it's that simple. Since we already validated the token and threw a form error, we should be safe to re-generate the token for the form that is still in validation.

The only possible drawback I can see is that other code must not trigger any action based on the form_token value. But since we're only updating $form and not $form_state['values'], we should be set.

To prevent what I just said, we have to set this at the end of drupal_validate_form() instead.

sun’s picture

Additionally, please note that Heine already suggested in #576276: Abort validation when the token validation fails that we can basically skip the entire form validation in case the token is invalid. That might as well be rolled into this patch.

David_Rothstein’s picture

I actually did try it originally with #default_value rather than #value, figuring that was the correct way. But it didn't work when I did that. I am not totally sure why, but only #value seemed to work.

sun’s picture

Assigned: Unassigned » sun
Status: Needs work » Needs review
FileSize
7.36 KB

So, yeah. This.

Status: Needs review » Needs work
Issue tags: -Usability

The last submitted patch, drupal.form-validate-token.66.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Usability

The last submitted patch, drupal.form-validate-token.66.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
11.39 KB

So this one should pass. The only failing tests were fake form submissions in form.test, which manually try to resemble a full form processing. (Need to be fixed in a separate issue to use the proper APIs, because we have to test our APIs, not some odd hacks.)

Status: Needs review » Needs work

The last submitted patch, drupal.form-validate-token.70.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
14.05 KB

Alright, let's skip the idea of entirely skipping form validation and just do the required here.

--
Also required is to speed up tests. I'm sick of waiting for test results, so this patch also massively speeds up for Form API tests.

sun’s picture

sun’s picture

Assigned: sun » Unassigned
Status: Needs review » Needs work

Although the latest patch works, we have to roll back the originally committed patch in #58. Fixing this bug is D8 material by now.

sun’s picture

Priority: Normal » Major
Issue tags: +Security improvements, +Security

We either need to roll back the already committed patch, or make sure that the follow-up patch is committed, which automatically rebuilds the form token. However, the follow-up needs a careful security review.

The current state does not make sense, because it suggests the user to resolve the problem through an action that most often does not resolve the problem.

Bojhan’s picture

Status: Needs work » Needs review

Makes sense, needs review then? for the security part

sun.core’s picture

David_Rothstein’s picture

Issue tags: +String freeze

I'm pretty convinced this is secure. One more opinion/review would be very desirable, though.

A couple thoughts on the latest patch:

-      form_set_error('form_token', t('This form is outdated. Reload the page and try again. Contact the site administrator if the problem persists.'));
...
+    form_set_error('form_token', t('This form was outdated and has been reloaded automatically. Try to submit it again. Contact the site administrator if the problem persists.'));

Though I am responsible for the latest version of that text, I no longer like it so much :)

As long as we are changing strings, keeping the "contact the site administrator" part around really doesn't make much sense. What value does that part of the message even add??

Also, "Try to submit it again" now seems a little blunt to me - especially since if they actually are the victim of a CSRF attack, that is the one (rare) case where they shouldn't try to submit it again.

How about this:

"The changes were not saved because the form was outdated. If you still want to perform this action, submit the form again."

+    $reset_token = TRUE;
   }
-
   _form_validate($form, $form_state, $form_id);

No reason to remove that blank line - it is a totally separate block of code, right?

+  // If the session token did not match the current user's session, reset the
+  // token, so the user is able to submit the form by submitting it again.
+  if (isset($reset_token)) {

I'd recommend using !empty() here. Even though there is currently no way that $reset_token can ever be FALSE, I think it keeps the code more robust and readable to explicitly make sure that the variable evaluates to TRUE.

David_Rothstein’s picture

Title: "Validation error, please try again..." is terrible UI and confuses users » "This form is outdated. Reload the page and try again" is incorrect and confuses users
Version: 6.14 » 7.x-dev
Priority: Normal » Major
Status: Active » Needs review
Issue tags: +Security improvements, +String freeze, +Security

Or even simpler:

The form could not be submitted because it was outdated. If you still want to perform this action, submit the form again.

David_Rothstein’s picture

Fixed the issues in #78. For the text, this uses the simplest yet:

"The form was outdated. If you still want to perform this action, submit the form again."

sun’s picture

Status: Needs review » Reviewed & tested by the community

I also spent quite some time thinking through possibilities of how this automatic token reset could imply a security risk, but couldn't think of any.

Thus, I think that we can commit this patch.

EvanDonovan’s picture

I think this is a great string improvement. Not expert on the security implications though.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

If we're going to monkey around with FAPI internals that deal with CSRF protection, we need review from a much wider set of folks, including half the security team.

chx’s picture

Basically what happens now ... someon CSRF attacks you and between you and the attack is a warning? I like reloading the form to throw the CSRF crap out the door. A message containing a link that reloads the page sounds like a better plan to me.

Heine’s picture

So, how does the user know what "action" is going to take place? At the very least, all form values should be discarded.

sun’s picture

all form values should be discarded

Ugh. Compared to that, the current situation of disallowing the submission for eternity is preferred then. At least, that allows to open the URL/form in a new browser tab and copy/paste your form values into the new form.

I'm currently running into this problem at least three times a day due to #978310: drupal.org forms expire way too early -- and I'd really kill myself if I'd lose my entire issue follow-up comment/work each time it happens.

David_Rothstein’s picture

Agreed with @sun in #86.

Regarding #84 and #85, if an attacker is able to fool someone into actually clicking the "submit" button, they don't necessarily even have to do an actual CSRF attempt any more. There are other ways I can get someone to visit a form that has evil things already prefilled in the form fields (and in that case the form will display without a warning message at all). Maybe we need to think about this more.

astrojim’s picture

Title: "This form is outdated. Reload the page and try again" is incorrect and confuses users » "This form is outdated. Reload the page and try again" appears when performing routine admin

I'm seeing this error on 7. I've applied patch given in #80, with no joy. We are running on a load balanced pair of servers, could this be related? This is just whilst performing routine changes on admin/config/media/file-system.

astrojim’s picture

I'm seeing this error on 7. I've applied patch given in #80, with no joy. We are running on a load balanced pair of servers, could this be related? This is just whilst performing routine changes on admin/config/media/file-system.

David_Rothstein’s picture

Title: "This form is outdated. Reload the page and try again" appears when performing routine admin » "This form is outdated. Reload the page and try again" is incorrect and confuses users

That sounds like an unrelated problem. You can create a new issue for that if necessary.

sun’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
David_Rothstein’s picture

My somewhat cryptic comment in #87 referred to the Prepopulate module.

Now that the security issue surrounding that is fixed (http://drupal.org/node/1182968), it's worth pointing out that the approach that module now takes is to prevent certain types of form fields (e.g. #type = 'hidden') from ever being prepopulated via the URL, since it isn't obvious to the user what they are submitting in those cases.

For this issue, if we commit the patch here we'd effectively be making the form API like Prepopulate module (and could similarly clear out 'hidden' and other fields if we wanted to); an attacker can prepopulate the form with whatever they want, but no actual attack is possible unless the user is convinced to click the Submit button themselves.

Alternatively, we could just give the user an optional way to clear out the form if they want to? This basically allows the user to decide if they got here via CSRF, for example:

"The form was outdated and could not be submitted. If you intended to perform this action, you can submit the form again. [And then a final sentence: We add some security warning and provide a link to reload the page with form values empty.]"

xjm’s picture

Edit: make that several thoughts:

  1. The text "this form is outdated" seems vague and confusing to me. @David_Rothstein's suggested text seems more clear, but I still don't know that a user would understand the implications.
  2. If I understand correctly, we're just creating a new token blindly. What if the content has changed between the original form load and the attempted submission? Will the user still get the "content modified by another user" message? If not, they might inadvertently revert things.
  3. It seems to me that, to make an informed decision about what "performing this action" is, the user would need to see and compare his or her submitted values with what the defaults would be if a new form were generated. (Edit: and that still leaves aside what to do about hidden values.)

In general, the token regeneration thingy makes me nervous.

Edit: Also, I added a summary. It could probably use more details under both proposed solution and remaining tasks.

EvanDonovan’s picture

In re: #2 of the last comment, isn't that a message specifically just on node forms? It seems like this code addresses other forms, but not that particular text. But I'm not sure.

The point about hidden values is a good one...not sure about that.

Regardless of whether a new token is generated, and it made possible for someone to submit the form, the help message for this situation definitely is improved by DavidRothstein's patch.

xjm’s picture

@EvanDonovan -- I guess maybe we should just test what happens with the patch when the "original" entity or values change between submissions. It could be nodes, settings forms, etc. I'll try that.

xjm’s picture

Status: Needs review » Needs work

Here's what I did (with patch applied):

  1. Opened admin/modules in one browser tab.
  2. Logged out and back in in another browser tab to emulate an expired token.
  3. In a different browser, logged in as a different user and enabled the color module.
  4. In the first browser window, enabled several contrib modules. Got the new message. Simply resubmitted the form, inadvertently disabling color module again. I didn't have any indication the color module had since been enabled, nor even any indication that I'd disabled it with my new submission.

This pattern applies to any form, anywhere. Users might change live content or settings without realizing it. Based on this, and even leaving aside the security considerations, I think the solution needs work.

xjm’s picture

With nodes, you get both error messages:

The form was outdated. If you still want to perform this action, submit the form again.
The content on this page has either been modified by another user, or you have already submitted modifications using this form. As a result, your changes cannot be saved.

However, it does still prevent you from overwriting the content on a second submission, with only the second error message. So I guess that isn't a concern.

I also realized the case in #98 with an accidental settings change happens with or without an expired token, so I guess that's a separate, pre-existing issue.

xjm’s picture

Issue summary: View changes

Updated issue summary.

EvanDonovan’s picture

If #98 is a separate issue, then is this back to "needs review"?

xjm’s picture

I think NW based on #85/87/94. I don't think blindly providing a new token is the right solution and I really don't think most users would understand the implications of resubmitting the form.

Really there are two issues here: one is making the error message not-wrong and providing the user a way to make the form functional again. The second is providing a better workflow for expired forms in general.

If we show them the form with their submitted values (but no new token), it's no worse than it already is in terms of the potential for data loss. They still have the chance to copy/paste fields that contain work. Could we reduce the scope to the first problem so that can be fixed and backported, and consider the workflow improvement a feature request?

xjm’s picture

How about:

Your submission cannot be saved because the form is no longer current. Copy any unsaved work in the form below and then reload the form.

xjm’s picture

Issue summary: View changes

Removed remark about accidentally reverting settings; that is actually a separate, pre-existing issue.

Bojhan’s picture

So yhea, the text is oke. There is no easy way around asking them to do something manually, but the appending action of pasting is not in the text.

I'm a bit split whether fixing this only for a small portion is actually wise, but I will leave that for others to discuss.

xjm’s picture

Status: Needs work » Needs review
FileSize
882 bytes

Attached patch simply gives the user instructions that will actually work, including a link that loads a clean form with a new token. The user has a chance to copy existing work, like sun says in #86, but Heine's concerns from #85 are still addressed.

I tested the behavior for both overlay and regular forms in Firefox and Chrome on OS X, and confirmed that the link loads a new form properly (with a new token, but no potentially dangerous field values from a potentially bogus submission).

David_Rothstein’s picture

I think using $item['href'] will miss out on query parameters in the URL. Would request_uri() work here instead?

I agree it makes sense to get this in first and then discuss the other improvement later.

catch’s picture

xjm’s picture

Does current_path() include the fragment (e.g. #overlay=blah)? Edit: realized this is a silly question; of course it will load in overlay or not as appropriate.

xjm’s picture

FileSize
969 bytes

Addressing #105/106.

xjm’s picture

Issue summary: View changes

Data is only lost if they do not copy/paste it.

xjm’s picture

Talked with jhogdon about how we might do this without breaking strings. We could leave the original, and add a paragraph, e.g.:

This form is outdated. Reload the page and try again.

If the problem persists, copy any unsaved work below and then load a new form.

There's still a new string to translate, though. I am not sure which way would be best.

Edit: Except, argh. I forgot about the "contact site administrator" bit. So the best we could do is:

This form is outdated. Reload the page and try again. Contact the site administrator if the problem persists.

You can also copy any unsaved work below and then load a new form.

And that is terribly confusing. The user is given three different instructions, in the wrong order. :(

xjm’s picture

Important note for testing this.

The browser reload button does not work for non-overlay forms in:

  • FF
  • Chrome
  • Safari

(All tested on Mac.)

However, the reload button does appear to work for forms in overlay, for all three browsers above.

So be sure to test any solution in both overlay and non-overlay forms. (I re-confirmed that a link as in my patch does work in both contexts.)

Edit: updated with Safari.

Bojhan’s picture

I am not sure what to review here.

sandie’s picture

I'm not sure is this same issue or not, using Drupal 7.7. error occure when create new and edit existing node with customized tpl.php to render the form, shows error like this :

[X] "This form is outdated. Reload the page and try again. Contact the site administrator if the problem persists."

I have try to:
1. "Clear cache" serveral times
2. logout then login again
3. try another browsers

Here is what I try to render the node form using custom ticket-node-form.tpl.php :

<?php print render($form['form_id']); ?>
<?php print render($form['form_build_id']); ?>

<?php print render ($form['title']);?>
<?php print render ($form['field_server']);?>
<table width=300px><tr><td width=10> <?php print render ($form['field_jenis_pekerjaan']);?></td><td width=35>
<?php print render ($form['field_prioritas']);?></td><td width=20>
<?php print render ($form['field_sumber']);?></td><td>
<?php print render ($form['field_status']);?></td></tr></table>
<?php print render ($form['field_detail_pekerjaan']);?>
<?php print render ($form['field_file']);?>
<?php print drupal_render($form['actions']); ?>

Any advice? may be something missed here.

Tq

Bojhan’s picture

Needs a screenshot, we can leave this open after the initial fix has gone in. We can break strings for D8, so lets keep in mind that this is primarily/solely a D7 fix.

Gábor Hojtsy’s picture

I think the original text is so harmful that we should not hold down this change for translations.

Bojhan’s picture

Thanks Gabor for the comment, I have done several inquiries with dutch translators about this too. Given the fact that the actual error text is just plain wrong, and could cause loss of data - we should just create one sentence that correctly explains it.

Webchick also noted that we are now arguing between seeing part of the error message not translated, to seeing the whole message not translated. I feel that both directions will create a jarring experience, and with the harmfulness in mind its better to go for a full change.

What about:

"This form is outdated. Copy any unsaved work below and then load a new form."

xjm’s picture

The text I used in #108 was:
t('Your submission cannot be saved because the form is no longer current. Copy any unsaved work in the form below and then <a href="@link">reload the form</a>.', array('@link' => $url)

The word "outdated" seemed confusing to me, so if we're changing the string, we might as well make it as good as possible. What do you think?

yoroy’s picture

"This form has become outdated. Copy any unsaved work below, then reload this page."

"has become" suggests a bit more "something happened in the mean time"

Can we say reload the page instead of form? Less precise but easier to grasp what should be done?

xjm’s picture

Assigned: Unassigned » xjm
xjm’s picture

FileSize
17.41 KB
16.71 KB
929 bytes

Attached patch uses yoroy's text from #117. He and Bojhan and I hashed it out on IRC and this is probably the best we can do. (Although I do like his proposed alternative: "s*** happened,plz reload".)

I've also attached screenshots comparing the old error message and the new one introduced by this patch.

xjm’s picture

Issue summary: View changes

Removed copy/pasted "I."

xjm’s picture

Updated summary.

Bojhan’s picture

It would be lovely to get some peer reviews before we mark this RTBC.

BTMash’s picture

The changes look good to me. I tested it out and it makes sense (in some ways, I feel like it would be nice if it also said something like 'contact the site admin if the problem persists' in the event that there is some other sort of issue happening on the site with that form but all in all, good) and the patch looks sound to me.

catch’s picture

Status: Needs review » Reviewed & tested by the community

I'm sure I've been a 'site admin' on a site where this happened, and I was just "wtf, reload the form", so happy for that to be removed ;)

Looks great.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x and 8.x. Thanks all.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

xjm’s picture

Assigned: xjm » Unassigned
TanvirAhmad’s picture

den33775’s picture

I got this message with 7.x-3.1 and 7.x-3.x-dev still. (Don't worry, reason documented below, with solution/reason).

I had noticed it does not happen with the admin account at all, but does happen with a non-admin account.

I had gone through privileges with a dev/test role, adding priv after priv until I isolated the fix to the privilege: "???"

Having added all privs to the dev role it still existed.

So I looked at my Organic Groups Global Permissions screen (admin/config/group/permissions).

In here, it restricts content (type) access by field.

I found that two new fields I'd added had not been granted (ticked) to OG admin role. They were test fields so I deleted them and the error went away. It would have gone away if I had ticked the access permission boxes in OG global permissions too as I had a similar weird error recently where adding a field made the field available to admin only but no other role, even though all other fields were available. All down to wonderful OG module's ability to restrict access at field level. But then if you don't get this, it leaves you with all sorts of very strange errors (like this one).

Anyway, that's my story for anyone that still has this error - check field level access controls. There is also a module called Field Permissions which does the same thing but outside OGs, so this could also be the culprit maybe, if you use this module.

I solved my own problem here, but the problem had same error as in this thread so thought I'd share.

Best wishes, hope it helps someone! :)

den33775’s picture

I noticed that not allowing access to a field (due to OG file permissions restriction) as in this error, also caused a bad knock on effect in my Ubercart Recurring (tax) module. http://drupal.org/node/1572300 for details.

It seems that restricting field access is incompatible with several commonly used modules, so personally I'm not going to use field restrictions in future (and will remember to always set access to any new fields via OG file permissions, since I need to use OG).

den33775’s picture

Issue summary: View changes

Updated issue summary.