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:
- Reloading the page does not actually resolve the issue.
- The message is confusing: Outdated how and why? Try what again?
Steps to reproduce:
- Go to node/add/article and fill out the form without submitting it.
- In another browser tab, log out of the site and log back in.
- 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:
- 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.
- 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.
Comment | File | Size | Author |
---|---|---|---|
#119 | 240828-119.patch | 929 bytes | xjm |
#119 | old_message.png | 16.71 KB | xjm |
#119 | new_message.png | 17.41 KB | xjm |
#108 | message-only-240828-108.patch | 969 bytes | xjm |
#104 | message-only-240828-104.patch | 882 bytes | xjm |
Comments
Comment #1
teledyn CreditAttribution: teledyn commentedSame 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
Comment #2
teledyn CreditAttribution: teledyn commentedthis 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.
Comment #3
seanlee CreditAttribution: seanlee commentedI 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
Comment #4
Simx0r CreditAttribution: Simx0r commentedThanks 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.
Comment #5
Tpainton-1 CreditAttribution: Tpainton-1 commentedWonderful.. 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!
Comment #6
drummDrupal'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.
Comment #7
tonycaine CreditAttribution: tonycaine commentedI 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
Comment #8
cbovard CreditAttribution: cbovard commentedsubscribing
Comment #9
Tpainton-1 CreditAttribution: Tpainton-1 commentedNot 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.
Comment #10
MGParisi CreditAttribution: MGParisi commentedDoes this infact "Heal itself"? Can anyone confirm this? If so how long do I have to wait!
Comment #11
MGParisi CreditAttribution: MGParisi commentedIf this is BY DESIGN... Then how the hell do I fix it!
Comment #12
Flying Drupalist CreditAttribution: Flying Drupalist commentedCan we have a list of issues or actions that may prompt this message?
Comment #13
MGParisi CreditAttribution: MGParisi commentedThis 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.
Comment #14
MGParisi CreditAttribution: MGParisi commentedIt 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.
Comment #15
El Viejo CreditAttribution: El Viejo commentedDon'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 ....
Comment #16
defconjuan CreditAttribution: defconjuan commentedI 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.
Comment #17
khan2ims CreditAttribution: khan2ims commentedRun 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 !
Comment #18
kyle_mathews CreditAttribution: kyle_mathews commentedHuh. . . 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. . .
Comment #19
patricksettle CreditAttribution: patricksettle commentedI'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.
Comment #20
patricksettle CreditAttribution: patricksettle commentedA 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*
Comment #21
Sholex CreditAttribution: Sholex commentedI had this problem with the Poll module. Users could not vote without getting the error. I ran cron and the problem went away.
Comment #22
dparraz CreditAttribution: dparraz commentedOur 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!
Comment #23
pixelpreview@gmail.com CreditAttribution: pixelpreview@gmail.com commentedyes 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 !
Comment #24
MGParisi CreditAttribution: MGParisi commentedIt seems that this is no longer "by design" but instead "needs review" or some other status.
Comment #25
MGParisi CreditAttribution: MGParisi commentedwoops, wrong page!
Comment #26
MGParisi CreditAttribution: MGParisi commentedResetting status, I meant to edit a different ticket! OOPS!
Comment #27
dwwI 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)
Comment #28
janis_lv CreditAttribution: janis_lv commentedcaused Translation module for me.
Comment #29
snorkers CreditAttribution: snorkers commentedI 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.
Comment #30
JayKayAu CreditAttribution: JayKayAu commentedHi 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.
Comment #31
himerus CreditAttribution: himerus commentedJust 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.
Comment #32
mikel1 CreditAttribution: mikel1 commentedI 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.
Comment #33
syka CreditAttribution: syka commentedThe same problem with my installation. I can change or update permissions!!! Any Ides??
Drupa 6.14
Comment #34
dwwArgh. 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. ;)
Comment #35
dwwBetter 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?
Comment #36
snorkers CreditAttribution: snorkers commentedI 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.
Comment #37
adshill CreditAttribution: adshill commentedI 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.
Comment #38
esolano CreditAttribution: esolano commentedSubscribing..
Comment #39
paganwinter CreditAttribution: paganwinter commentedSubscribing...
Comment #40
jbigras CreditAttribution: jbigras commentedLog 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.
Comment #41
pxwee5 CreditAttribution: pxwee5 commentedI 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
Comment #42
ressa CreditAttribution: ressa commentedLogging 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.
Comment #43
sepla CreditAttribution: sepla commentedThis 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.
Comment #44
das-peter CreditAttribution: das-peter commentedHi 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:
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:
Hope that will help.
Cheers
Peter
Comment #45
a5342346 CreditAttribution: a5342346 commentedsubscribing
Comment #46
jdexter CreditAttribution: jdexter commentedSalutations:
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
Comment #47
plcoelho CreditAttribution: plcoelho commentedHi,
I just flushed all caches, and the error has gone.
It was flushed, also :)
Cheers,
Paulo
Comment #48
dpiccolo CreditAttribution: dpiccolo commentedIn my case this error shows up when I have the formfilter enabled for the content type. Once disabled it doesn't show up anymore.
Comment #49
sunThis 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.
Comment #50
dww@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.
Comment #51
webchickI 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.
Comment #52
webchickOh, and +100 for #35. We should probably note that doing so is going to remove their form entry thusfar though. Hm...
Comment #53
sunSo 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.
Comment #54
dwwd.o has no workaround that I know of.
Comment #55
sunComment #56
dwwCouldn't the text read:
to give you a link to the local page (as I had in comment #35)? Or is that invalid XHTML or something? ;)
Comment #57
sunModern browsers keep your user input upon reloading a page.
Comment #58
yoroy CreditAttribution: yoroy commentedEasy on the 'pleases', please ;-) #679890: Using "Please" in the interface
Comment #59
sunHeh. 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.
Comment #60
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #61
David_Rothstein CreditAttribution: David_Rothstein commentedYes, 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.
Comment #62
David_Rothstein CreditAttribution: David_Rothstein commentedHm, 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.
Comment #63
sunNeeds 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.
Comment #64
sunAdditionally, 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.
Comment #65
David_Rothstein CreditAttribution: David_Rothstein commentedI 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.
Comment #66
sunSo, yeah. This.
Comment #68
sun#66: drupal.form-validate-token.66.patch queued for re-testing.
Comment #70
sunSo 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.)
Comment #72
sunAlright, 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.
Comment #73
sunMoved additional clean-ups into #913086: Allow modules to provide default configuration for running tests
Comment #74
sunAlthough the latest patch works, we have to roll back the originally committed patch in #58. Fixing this bug is D8 material by now.
Comment #75
sunWe 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.
Comment #76
Bojhan CreditAttribution: Bojhan commentedMakes sense, needs review then? for the security part
Comment #77
sun.core CreditAttribution: sun.core commented#73: drupal.form-validate-token.73.patch queued for re-testing.
Comment #78
David_Rothstein CreditAttribution: David_Rothstein commentedI'm pretty convinced this is secure. One more opinion/review would be very desirable, though.
A couple thoughts on the latest patch:
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:
No reason to remove that blank line - it is a totally separate block of code, right?
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.
Comment #79
David_Rothstein CreditAttribution: David_Rothstein commentedOr even simpler:
Comment #80
David_Rothstein CreditAttribution: David_Rothstein commentedFixed 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."
Comment #81
sunI 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.
Comment #82
EvanDonovan CreditAttribution: EvanDonovan commentedI think this is a great string improvement. Not expert on the security implications though.
Comment #83
webchickIf 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.
Comment #84
chx CreditAttribution: chx commentedBasically 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.
Comment #85
Heine CreditAttribution: Heine commentedSo, how does the user know what "action" is going to take place? At the very least, all form values should be discarded.
Comment #86
sunUgh. 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.
Comment #87
David_Rothstein CreditAttribution: David_Rothstein commentedAgreed 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.
Comment #90
astrojim CreditAttribution: astrojim commentedI'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.
Comment #91
astrojim CreditAttribution: astrojim commentedI'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.
Comment #92
David_Rothstein CreditAttribution: David_Rothstein commentedThat sounds like an unrelated problem. You can create a new issue for that if necessary.
Comment #93
sunComment #94
David_Rothstein CreditAttribution: David_Rothstein commentedMy 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.]"
Comment #95
xjmEdit: make that several thoughts:
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.
Comment #96
EvanDonovan CreditAttribution: EvanDonovan commentedIn 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.
Comment #97
xjm@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.
Comment #98
xjmHere's what I did (with patch applied):
admin/modules
in one browser tab.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.
Comment #99
xjmWith nodes, you get both error messages:
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.
Comment #99.0
xjmUpdated issue summary.
Comment #100
EvanDonovan CreditAttribution: EvanDonovan commentedIf #98 is a separate issue, then is this back to "needs review"?
Comment #101
xjmI 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?
Comment #102
xjmHow about:
Comment #102.0
xjmRemoved remark about accidentally reverting settings; that is actually a separate, pre-existing issue.
Comment #103
Bojhan CreditAttribution: Bojhan commentedSo 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.
Comment #104
xjmAttached 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).
Comment #105
David_Rothstein CreditAttribution: David_Rothstein commentedI 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.
Comment #106
catchHmm I would probably use current_path() and http://api.drupal.org/api/drupal/includes--common.inc/function/drupal_ge....
Comment #107
xjmDoes
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.Comment #108
xjmAddressing #105/106.
Comment #108.0
xjmData is only lost if they do not copy/paste it.
Comment #109
xjmTalked with jhogdon about how we might do this without breaking strings. We could leave the original, and add a paragraph, e.g.:
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:
And that is terribly confusing. The user is given three different instructions, in the wrong order. :(
Comment #110
xjmImportant note for testing this.
The browser reload button does not work for non-overlay forms in:
(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.
Comment #111
Bojhan CreditAttribution: Bojhan commentedI am not sure what to review here.
Comment #112
sandie CreditAttribution: sandie commentedI'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 :
Any advice? may be something missed here.
Tq
Comment #113
Bojhan CreditAttribution: Bojhan commentedNeeds 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.
Comment #114
Gábor HojtsyI think the original text is so harmful that we should not hold down this change for translations.
Comment #115
Bojhan CreditAttribution: Bojhan commentedThanks 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."
Comment #116
xjmThe 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?
Comment #117
yoroy CreditAttribution: yoroy commented"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?
Comment #118
xjmComment #119
xjmAttached 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.
Comment #119.0
xjmRemoved copy/pasted "I."
Comment #120
xjmUpdated summary.
Comment #121
Bojhan CreditAttribution: Bojhan commentedIt would be lovely to get some peer reviews before we mark this RTBC.
Comment #122
BTMash CreditAttribution: BTMash commentedThe 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.
Comment #123
catchI'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.
Comment #124
Dries CreditAttribution: Dries commentedCommitted to 7.x and 8.x. Thanks all.
Comment #126
xjmComment #127
TanvirAhmad CreditAttribution: TanvirAhmad commentedLook at for a solution http://drupaldr.blogspot.com/2012/04/form-has-become-outdated-copy-any.html
Comment #128
den33775 CreditAttribution: den33775 commentedI 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! :)
Comment #129
den33775 CreditAttribution: den33775 commentedI 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).
Comment #129.0
den33775 CreditAttribution: den33775 commentedUpdated issue summary.