Forms with form tokens fail validation for anonymous users when caching is enabled

Just bumping the version - still happens on 4.7 beta 6, on several sites. We're running them on a FreeBSD server with Lighttpd and APC (PHP) caching enabled.

Just about to test with beta 6 on another server to confirm. I'm tempted to mark this critical as it seems contact forms simply won't work with caching enabled.

Can anyone else reproduce?

OK, this is confirmed on two server set ups now. Steps to reproduce:

1. Install beta 6
2. Create user 1
3. Set up contact form with a single recipient
4. Turn on caching, with 1 hour limit
5. Empty cache
6. Log out
7. Send message from contact form - works.
8. Hit contact page, perhaps one or two tries later, the form fails with a validation error, due to the reasons explained by Dylan above.

Contact forms will not work, it seems for anonymous users on sites with caching enabled.

Just an idea... does that happen with any browser? Does page cache work on any other part of the site?

For some reason, I have been having problems with IE on my sites (4.6 and 4.7), when page cache is enabled.

A page cache issue:
http://drupal.org/node/38213

Not sure if it is caused by the same problem tough.

Happens with Firefox 1.5.0.1 on Archlinux or FreeBSD, and Konqueror on either of those same platforms. Haven't tried other browsers tho. I don't think this is a problem with browser cache - its Drupal which is giving out a cached copy of the contact form, old token and all, to multiple anonymous users. Ie the second or third visitor to the contact form (with a different browser, machine, whatever) will still receive a cached page and old token, which will then be compared to the newer, correct token, resulting in the validation error.

I can't reproduce this with a clean install of HEAD, however updating one of the problem sites to HEAD (from beta 6) doesn't help: the problem still occurs.

(Note: this could be a problem with anonymous comments too, but thats untested.)

Oh, I get it now. I believe what happens is:

The contact form is build by contact_mail_user(), the menu callback attached to user/x/contact. This is a GET request, hence it is cached. When the form is submitted, a POST request is sent, but this one is not cached.

The first user generates its own token (name . email), this is cached. Another user goes to the contact page and it gets the cached page, which contains the token generated by the first user. When second user sends the POST (not cached) the form fails because the token doesn't match.

I see two possible solutions: 1) don't use the token 2) don't cache the contact page. I'm going to guess 2) is preferred.

Well here is part one... There is currently no way to tell the caching mechanism not to cache a page... so...

I just made a fresh install of CVS, activated the contact module, created a site-wide form, activated cache for 1hr, cleared cache, logged out, went to the contact form, and I get, "Validation error, please try again. If this error persists, please contact the site administrator." Kind of ironic, huh. I can send all night long as long as I'm logged in.

I had planned on rolling a patch by now, but I'm still trying to learn #token in FAPI. It's not listed on http://drupaldocs.org/api/head/file/contributions/docs/developer/topics/...

with caching being block by the form API anytime a token is set.

ok the form patch was dirty here is a cumulative patch... (figured out I could put two files as args to cvs diff)

I thought I had seen one of those form patch lines in another critical issue...

For the record, the patch seems to resolve the process I most recently posted. I haven't tried much else and I see lots of discussion about the implementation in IRC.

merlinofchaos pointed out this super cool get set technique using references.... Its less confusing and leads to better function names...

.darrel.

And chx greatly reduced my patch by pointing out that I could merge by page_set_cacheable into the following if... :)...

now chx doesn't like the nifty reference madness, nor the naming...

So here is one with drupal_set nomenclature and more traditional drupal_set static var methodology.

Maybe if I hadn't rewritten this patch five times I could remember that function calls need parenthesis.b

Important discussion in IRC:

|gatsby| dopry , merlinofchaos et al.: I'm still not sure how this is going to help.. Won't this effectively turn off caching for all nodes? The issue does exist in the comment form as well.
|gatsby| s/effectively/potentially
dopry does the comment form have a token?
|gatsby| dopry: yes, and the issue exists there as wel
|gatsby| Only two places that use tokens in core are the contact form and the comment form
dopry then if you want proper caching don't enable anon comments?
dopry I'm not sure how to better address it...
|gatsby| The contact form has its own dedicated page, whereas the comment form can appear on the node page or a separate page. The comment form also has a preview system..
deekayen don't display the comment form on the default node page?
deekayen or loosen the rules on tokens
deekayen or make tokens an option in the site settings
chx tokens as options , sounds good
deekayen leave them off by default, and in the description for the option to enable them, put a notice about disabled caching on comment and contact pages
deekayen then functionality 4.6 -> 4.7 changes only by opt-in

Just to note... I tested merlins return by reference get/set method and the drupal isset get/set method...

dopry@bbox:~$ php perftest.php
test: drupal_set_page_cacheable(FALSE) x 1000000 = 6.91834998131
test: drupal_get_page_cacheable(FALSE) x 1000000 = 7.84996008873
test: page_set_cacheable(FALSE) x 1000000 = 7.5729329586
test: page_get_cacheable(FALSE) x 1000000 = 4.44805192947
dopry@bbox:~$ php perftest.php
test: drupal_set_page_cacheable(FALSE) x 1000000 = 6.93783712387
test: drupal_get_page_cacheable(FALSE) x 1000000 = 7.8637239933
test: page_set_cacheable(FALSE) x 1000000 = 7.84954881668
test: page_get_cacheable(FALSE) x 1000000 = 5.70706295967
dopry@bbox:~$ php perftest.php
test: drupal_set_page_cacheable(FALSE) x 1000000 = 6.89977812767
test: drupal_get_page_cacheable(FALSE) x 1000000 = 9.04702305794
test: page_set_cacheable(FALSE) x 1000000 = 8.76061415672
test: page_get_cacheable(FALSE) x 1000000 = 5.59210300446
dopry@bbox:~$

the return by reference method wins hands down on the get side of things... loses a bit on the set side.
but wins all the sums.

I reckon we should just nix form tokens for the time being until it can coexist with caching. The contact module already has a flood control mechanism. Perhaps the same can be added for the comment.module (for anonymous users) to provide a reasonable degree of control and protection. Sites that require more can look at other options like CAPTCHA (inaccessible as it is) and the troll/spam/bad behaviour modules etc.

Updating title to reflect that this affects more than just the contact module.

Cheers,
-K

Patch removes tokens for anonymous users. Adding flood control to comments is non-critical.

Patch removes all token use (all three instances).

Let's remove tokens completely.

Still applicable to HEAD, tested with thanks to Morbus' bleeding edge keen-ness for disobey.com :)

(14:36:40) leafishpaul: Morbus: what version of Drupal is disobey running on?
(14:36:48) Morbus: leafishpaul: 10 minute ago HEAD ;)

There is very little point in providing an API in core which is broken as it is. See patch in my prev comment.

More.

First tokens was based on IPs but http://drupal.org/node/36591 made us using session_id. Now, the md5 of session_id + secret key the first visitor to visit the cached page is cached. It's beyond me how could we use tokens without a serious performance hit.

I'm going with a +1 for removing them, if its not immediately obvious how to get around this problem. I liked the approach from dopry for excluding certain pages from the cache (I've needed that functionality for sometime), but as has already been mentioned, this is not a valid solution on node pages with comment forms open to anonymous users.

I say +1 for removing them, but I can't comment on the security side of things - I'm not that read up on this area of forms/drupal. What should be put in to replace em, eg flood control? The comment form already has this, and as Zen mentioned perhaps this could be reworked for comment forms etc?

Possibly not a task for 4.7, tho, unless someone thinks removing tokens completely is going to spoil some security features and possibly open a 4.7 site up for potential attacks...

the form tokens were concieved as a spam deterrent and not a security thing. however adding flood to comment is not hard.

Removes tokens altogether and adds chx's comment threshold with a configure form item.

Haven't tried it, but flood_register_event looks like it should register comment, not contact.

I put a -10000 on the removal of tokens, unless someone can explain why they were added, and why it is acceptable to remove them. Pending their explanation I may change my mind to a +1. We are here to fix bugs for 4.7, not remove features. If our implementation of form validation is broken, and it is an expectation for 4.7 we need to fix that before rc. (form validation !== input validation).

My argument for the removal of the tokens would be that they're ineffective. A scripter simply needs to send a GET request to your form generation url before POSTING to circumvent the meager protection provided by form tokens. Form tokens do not actuall provide any flood control, only prevent spoofing. So we need the flood control mechanism regardless.

I'd still like a cache control setting switch though. :)

.darrel.

I just had a quick browse of the patch:

-I don't see the point in implementing comment flood control for authenticated users. We are only trying to restrict spam by anonymous users here. The default hourly threshold can also probably come down.. to say 6.

-The global in drupal_form_validate can go?

-Use drupal_map_assoc for options?

That's about it - I can test/roll another patch when I'm back at my box.

Cheers
-K

http://drupal.org/node/28420 added form tokens. See reasoning there. The reason was, spamers were setting POST op to Submit and thus sidestepped the Preview phase and DoS'd his site. By using tokens, jeremy made the spambots slower and that's about it.

I will work on a solution, then -- so that you can preview a patch without a token but can not submit without it. The token will be inserted during the preview phase and checked during submit. Preview can not be cached anyways because that's a result of POST. This will do for comment. contact_mail_user has flood control to prevent DoS. contact_mail_page -- I do not yet know.

Does what Zen asked for.

Old default value

Let's see this version.

Now, this patch fixes what the title says. Exactly.

chx's patch with flood control and optional token setting with docs.

Adds some flood control for contact.module. Needs testing.

Uglier looking patch to make prettier code and correct return value for contact_mail_page_submit().

More accurate flood message for contact_mail_page_submit().

chx: I removed an erroneous extra ) from your last patch.

+1 for this one so far. Does exactly what is says on the tin. Will be testing on all our 4.7 sites soon.

@deekayen - can we keep this simple to try and get this fix, and perhaps submit the flood stuff as a separate patch/issue?

Another one with a watchdog warning for validation errors. Providing they're less rare now (arf), perhaps site admins would like to be notified. Couple of things:

1. Whats the opinion on whether this is worth watchdogging (yikes)?
2. Should we output the form id here?

Ta, all.

Previous patch had inconsitent use of checks on $user->uid for flooding. This patch flood controls comments and contact.module for anonymous and registered users with separate options.

D'oh. I didn't see the other patches from paul. Checking on them

Ok, ignore my last patch then, moving flood options to: http://drupal.org/node/54244

+1 for the patch in comment-46...

It solve the problem with the conflict b/t form tokens and caching.
I don't think we need the watchdog error.

@deekayen: cheers!

@dopry - fair enough. Any one else care to comment?

Marking #46 RTBC.

#46, yes yes

#46

I know there's already some happy agreement. Even I voted, but there's a little part of patch #42 I'd like to see kept to make the token optional for comments.

i'm just tuning into this one. nice work, everybody.

regarding #55 - hate double negatives like this: define('COMMENT_FORM_TOKEN_DISABLED', 0); please use positive variable names and constants such as: define('COMMENT_FORM_TOKEN_ENABLED', TRUE);

As alluded to earlier, the usual way to insert dynamic info (like targeted ads) into static pages is to use javascript. remember that javascript can do dynamic stuff on a static page ... the javascript can even do a lightweight AJAX call to the server to get more info. So i think that there are solutions to this problem but we don't have to explore them now. just change that constant name please.

oops. only #55 needs work. the earlier consensus patch (#46) looks good code wise, and appears to have lots of testers reporting positive results.

I am unable to test/create patches atm: looking at the #46 patch - can we please also add some comments? I also think that an addition to the settings tab and help hook of the comment module (recommending that previews be on) will also be worthwhile. If the default value for previews is 'optional', it should probably be changed to 'required'.

Cheers :)
-K

Variable renamed and tokens for comments off by default (keep similar 4.6 functionality).

@Zen - default for comments is preview required already

...or, with constant renamed, tokens for comments on by default for new spam-deterring functionality.

Patch #46 with some added docs. I'm still a little squirmy about building in the tokens as a requirement for a core module. The whole feature seems contrib-ish to me to start with.

deekayen: I don't think we want to scare away admins with jargon and the nitty-gritties :)

fapi comment: "Unset the form token for anonymous users if the form is on a cached GET page. This avoids validation errors due to conflicts between cached and uncached form tokens."

comment module:

  $form['posting_settings']['comment_preview'] = array(
    '#type' => 'radios',
    '#title' => t('Preview comment'),
    '#default_value' => variable_get('comment_preview', COMMENT_PREVIEW_REQUIRED),
    '#options' => array(t('Optional'), t('Required')),
  );

Add description text above along the lines of: 'It is recommended that preview settings always be set to required for additional protection against spam.'

And possibly for help:

'Comment forms are generated with embedded tokens that provide protection against spam. For best results, preview mode for comment forms should always be set to required. This is especially important if anonymous users are allowed to post comments without approval.'

Hope that helps :)
Thanks,
-K

#46, please, if others have tested.

Re #61 - I don't think the code comments are needed, and the waffle about tokens for the comments admin/help page certainly doesn't need to be shown to users.

Plus, this thread is getting messy. Can we please just get the simplest fix in, and worry about flooding and other features later?

I am a bit confused about the many comments. I don't see why we should have tokens if we dont use the cache and not if we do. Since everybody will want to use the cache that's quite pointless.

What if the token was also cached?

Token option moved to: http://drupal.org/node/54360. To clear up the confusion I made, this issue is waiting for patch #46 to be committed.

I just realised that the #46 patch is in many ways self-defeating. If I'm not wrong, the spammer will now just be able to change the 'op' value to 'Post+comment' and since there is no form token to worry about, fapi will pass it through. Please correct me if I'm wrong.

Resetting to active.

Cheers
-K

Can't we re-use the spam protection that contact.module uses?

@zen, with tokens a spammer just has to GET {comment form}; parse TOKEN; POST {comment form}
anyway.

I made an argument for just removing tokens outright, since they are ineffective. If that is the communal agreement we can strip the buggers out. I just didn't want to see them removed without community review, and specific documetation as to why the were removed...

.back to my hole

.darrel.

Maybe we should try to get input from the people who proposed these tokens. IIRC Jeremy from kerneltrap.org as involved.

if i recall correctly, jeremy said that this would slow down the hackers a ton because they actually have to wait for the page to return (so bot can get the token). in 4.6 we say that preview is required but actually nothing enforces that so spammersw were just sending a 'submit' op instead of preview. in 4.7 though, i'm prettty sure that form api might not let you fake the op like that.

anyway, i think these tokens are a weak solution and i would not be sad to see them go.

Pork-free token removal. I'll be happy to help someone on a token contrib module after 4.7 is out the door.

I like this version, too.

I will check whether form API lets you or not forge the POST op.

Someone just pinged me about this issue. I'm offline today, so unable to contribute but want to quickly answer a comment:

I made an argument for just removing tokens outright, since they are ineffective. If that is the communal agreement we can strip the buggers out.

On KernelTrap.org I was getting upwards of 10,000 spam comments an hour prior to implementing tokens. Once implemented, this dropped to less than a dozen a day. I don't call that ineffective.

Tokens were added before the forms api, and worked fine at that time. The new implementation within the formapi runs into this caching bug. The solution looks to be as simple as adding tokens during preview, and validating during submit. If a form doesn't require previews, then it effectively doesn't require tokens either.

Please don't blindly remove this functionality just because it was broken by other changes to core since the time it was added.

Based on Jeremy's comments, I'm setting this down to code needs work. He's right; it IS a proven deterrent against certain forms of spam and it WAS working okay prior to the forms API change.

Ok, I think I've come to a conclusion wrt this issue:

1) If chx says, that you can't fake op anymore, we don't need tokens and can remove them.

2) If chx says we can still fake ops, we can discuss to fix that, and then remove tokens.

3) if 1 and 2 fail we will keep tokens and apply patch #46.

@killes - that looks like a good plan. seems that op validation is generally useful and i hope we go that route (if not available already)

OK guys, Here's the op checker and the tokens removed.

token removal credits goes to dekayen.

damn. I only tested you can't forge Preview into Submit. But, you can't Submit either. And I do not think it's something which is easy to fix without something like that token thing: how could the submit know that the previous page was a preview page, so submit is now legal?

As the op checker failed, I am including here #46 rerolled in a correct format and this is what we should use. Not the best solution, but there is no better.

ops, that was a -p1 patch.

"how could the submit know that the previous page was a preview page, so submit is now legal?"

thats not the way i was thinking about it. submit is a valid op because that button appeared when the form was last rendered. I haven't looked at code, but I don't understand why an op checker is difficult? the rule is that only buttons which appear on the form are valid ops.

thats not the way i was thinking about it. submit is a valid op because that button appeared when the form was last rendered. I haven't looked at code, but I don't understand why an op checker is difficult? the rule is that only buttons which appear on the form are valid ops.

Just verifying that the "$op" is valid in the sense that a button with that $op existed on the last form rendered is not sufficient. Without tokens, spammers can script form POSTs without actually viewing a rendered form. That is, without takens a spammer can load a comment form once and then post comments all over the website on any node or reply without ever loading the form again. (This is how they were able to generate >10,000 spam comments an hour on my website, roasting my CPU) With tokens, the spammer has to load the comment form at least once for every comment/node it wishes to reply to, as the token is unique for each different form rendered. Tokens are much more than an op validator.

" only buttons which appear on the form are valid ops" -- that's true. Now, how do you know that submit button should be added to the current forms array? That depends on what was the previous page. As a matter of fact, currently all submits are op fakes... look at node_form_add_preview:

  if (variable_get('node_preview', 0) && (form_get_errors() || $op != t('Preview'))) {
    unset($form['submit']);
  }

if the node_preview is mandatory and $op == t('Submit'); this code _will_ fire and remove the submit button...

chx explained this in detail via IRC. an op checker is not possible unless we white list some ops (like 'Submit'). op checker is a dead end. bring back the tokens ... ready to commit

applied