Make cache_form expiration configurable, to mitigate runaway cache_form tables

This sounds pretty good

This patch makes a lot of sense and is a minor non-destructive change that won't affect sites which do not need the alteration since the default value remains at 6 hours.

New patch which only tweaks the comments, but this looks great; a simple solution to what can be a very serious problem.

This would need to go into Drupal 8 first.

Note that we don't have cache_form anymore in D8

It does have a expiration in FormBuilder::setCache() that has a hardcoded expiration.

Note that the "expiration" of temporary files does have the same hardcoded value, which is also being made configurabe #1399846: Make unused file 'cleanup' configurable.

I think those values are the same for a reason, otherwise it could happen that you submit a form after uploading a file and the file has been removed in the meantime. Not sure what to do about it, though.

+1 on making this configurable though.

8: 2091511-cache_form_expiry_to_variable-D8.patch queued for re-testing.

You need to inject the configuration object for that and update the PhpUnit tests.

+++ b/core/modules/system/config/system.performance.yml
@@ -2,6 +2,8 @@ cache:
+  expire:
+    form: 21600

I would swap this around so it is cache.form.expire

Also not sure about naming it cache at all in 8.x as it is currently using key_value_expirable and not the cache API. That said, there's an issue which aims to change that again.

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -390,9 +390,7 @@ public function getCache($form_build_id, &$form_state) {
   +    $expire = \Drupal::config('system.performance')->get('cache.form.expire');
   

   Dare I say it, this should really be injected instead. This form builder constructor is getting kind of crazy.

2. +++ b/core/modules/system/config/system.performance.yml
   +++ b/core/modules/system/config/system.performance.yml
   @@ -2,6 +2,8 @@ cache:
   
   @@ -2,6 +2,8 @@ cache:
   +  form:
   +    expire: 21600
   

   mirroring what berdir mentioned, this is not really coming from cache. Where is the issue to change this? I thought when this was initially converted we had good reasons for this to live in kve instead.

Cross posting #2252763: Views exposed filter form causes enormous form state cache entries

I don't think we need the configFactory part anymore? Can't we do something like : \Drupal::config('system.performance')->get(cache:form:expire)

Will try to reroll.

I have a question. Why don't we have flexibility of setting this per context. I used form my projects on drupal 7

drupal_alter('form_set_cache', $expire, $form, $form_state);

and then altered time needed for cleaning the forms per different page.

This should be largely irrelevant in D8 now. D8 no longer puts forms into the form cache on the first request, only when users actually start to do partial form submissions like uploading images.

This should be largely irrelevant in D8 now.

That being the case, shall we move this back to D7 then and reconsider the patch from #4 (and / or possibly the approach suggested in #46 whereby drupal_alter is used to allow every entry to be tweaked before it goes into cache_form)?

Just adding the variable into form_set_cache would be a substantial improvement for D7.

That's great news that it's fixed in a better way in 8. It could still go in 8, if not just for removing a hardcoded number.

+1 for #4

Re-uploading the exact same patch from #4 to try to get that tested against D7 (instead of the last D8 patch which failed).

trying RTBC again...

This looks like a duplicate of #1286154: Allow custom default form cache expiration/lifetime (which is already marked for Drupal 7 backport). I don't think there's any reason we'd want to make this configurable in Drupal 7 while leaving it unconfigurable in Drupal 8...

subscribe ++

Discussed this briefly with Fabianx at DrupalCon Dublin...

Changing this back to RTBC in the hope that we can get the very simple patch in #57 (which I've checked still applies to 7.x HEAD) committed to D7.

Whilst #1286154: Allow custom default form cache expiration/lifetime shares the same objective as this issue, I don't see how the approach being followed there for D8 applies in D7, unless I'm missing something.

So in the interests of pragmatism and getting this done, I'd suggest that we get this simple change into D7 and keep working on the D8 implementation in the other issue.

I am not sure why the Drupal 8 patch in that issue got so complicated either. Ideally it would just check a configuration setting like this one does...

We can leave this open given that we now do Drupal 7 backports in separate issues anyway, but I think it should be postponed on the other issue. We don't commit fixes to Drupal 7 that aren't in Drupal 8 yet unless there's a really good reason, and I don't see what the reason would be in this case.

+1 to postponing this on the other issue -- I also wonder if making this configurable may have any unintended consecuences in places that still assume the cache lifetime is hardcoded at 6 hours, so perhaps this should go into a 7.60?

#67, a good reason to commit this now is that you can DOS or hard crash a server w/o it, #2341447: Possible Denial of Service Attack with {cache_form}.

This is much less of an issue in 8.x after #2263569: Bypass form caching by default for forms using #ajax. landed - i.e. simply viewing a form never creates a {cache_form} entry in 8.x, only submits, so the number of cache form entries is much more restricted.

I left a comment on the 8.x issue, but since the bug is much more severe in 7.x, I don't see a reason to postpone on the 8.x fix (also I think the 8.x fix should just use $settings, which is equivalent to the variable_get() call here).

Thanks @catch

For D7, can we add this in $settings a $conf variable in default.settings.php with a long comment explaining all the implications of setting this to too high of a value, as well as too low of a value (AJAX requests failing).

I also wonder if we should set this on a per form basis, rather than for all forms on the site at the same time, as from what I understand it's usually only one form that's problematic.

If I understood catch's suggestion correctly, it was that the D8 implementation of this could be similar to the simple variable_get here in D7 if we use Settings::get (which can include a default value in the call). I've put a new patch based on this in #1286154-81: Allow custom default form cache expiration/lifetime.

So I'm not sure if the suggestion is that the D7 implementation here needs to change? stefan.r you're saying there could be an example with comments in settings.php? I suppose so, but not sure how necessary that is.

@mcdruid that's right, I meant a commented out $conf variable (see default.settings.php) -- which shouldn't change the implementation.

The only thing that could slightly change it is if we allowed for the 6 hours to be overridden on a per-form basis (with a variable based on form ID) instead of globally.

#71, rather than doing this to settings, I think adding a new special element would be better. Then people can form_alter, or just define it in their forms. This would help in situations where form IDs are more dynamic (like with webforms? don't remember right now). Something like the attached.

That said, I am not sure what the real benefit would be. I have had the approach in #57 live on client sites for about two years now, one of which had a lot of large, public forms and was getting hammered by bots (see the issue I linked above). Even dropping the TTL to about an hour had a huge impact.

I like the approach in #75.
We still need to document the new variable in settings.default.php.

I can see why some sites might want a different expiry for different forms... That does introduce more complexity though.

On the one hand, using a pattern for a variable names like form_cache_expiry_$form_id (presumably along with a default variable too) would make it easy to tweak for certain forms without writing code.

However, might there be use cases this wouldn't cater for? How about if you want to set a custom expiry for a set of forms based on a pattern e.g. perhaps those with form_id's matching webform_*.

The approach of adding a custom form element might be a better match for examples like that. Or alternatively, a drupal_alter might allow for ultimate flexibility.

Once you're into the realm of having to write code to customise the expiry for given forms though, I'm not sure what the advantage of drupal_alter over a custom form element would be. Not adding new elements to the Form API would perhaps be one.

All that said, here's a patch with just the simple single variable_get (from #57) along with a first pass at a comment block for default.settings.php

I've also tweaked the comment in form.inc slightly.

Ooops - referenced the wrong function in the comments:

  * example to prolong cache_form entries for AJAX forms in cached HTML.
  *
- * @see form_get_cache()
+ * @see form_set_cache()
  * @see system_cron()

Will set back to RTBC assuming tests pass.

The idea behind a per form setting was to narrow the impact -- we'd get rid of the global setting entirely and stick to the 6 hours for non-problematic forms, as other custom/contrib code might assume it's still set to 6 hours. But not 100% sure about this, the current patch may be fine, as long as we clearly spell out that changing the global variable may have unintended effects (ideally listing those as well).

The settings.php comment could also be a bit clearer on the impact of setting this to too low of a value (AJAX would be affected, multi-step forms as well).

Assigning to @mpdonadio to further think about how to proceed here, as we had talked about this on IRC.

Yes, @stefan_r talked about this in IRC. Don't think I have the logs, but I recall what we talked about.

First, a little background. I independently arrived at the same solution we have in #79 during the course of working on a site that led to #2341447: Possible Denial of Service Attack with {cache_form}. So, this isn't necessarily just about a performance improvement or keeping a database small; a runaway {cache_form} can crash a system. In my case, it was caused by a bad bot and led to a severe crash (total database corruption, had to reinstall mysql and restore backups). That truly sucked.

We have three options on the table here, and they aren't necessarily mutually exclusive:

1. Change the TTL to a global setting. This is what #79 does.

2. Make the TTL configurable per form in $settings.php. This would essentially be

$expire = variable_get('form_cache_expiry_' . $form_id, 21600);

3. Add a special form element #cache_expiry, and people could form_alter to their individual needs. #75 shows this.

It is totally possible to combine the above and come up with a flexible solution.

As we decide on a solution, we need to keep a few things in mind.

- This is likely only a problem with a few forms on a few sites. My case was a public node_form where nodes had lots of fields.
- We can't ignore site owners and site builders who may not be or have developers available at the necessary time. I think this rules out #3 above.
- We need to implement something with minimal disruption, even if the disruption is to a small subset of users. Remember the itok patch? Remember the followups because it caused problems for some site owners? We need to avoid that.

I think the last point above should be at the forefront of our minds given the state and maturity of Drupal 7. I think we are reasonable sure that #1 above wouldn't break anything in core alone (though DRUPAL_MAXIMUM_TEMP_FILE_AGE being the same value concerns me a bit). However, our test coverage of Drupal 7 isn't as good as Drupal 8, and we can't be sure if contrib or custom modules have made any assumptions about the expiration being 6 hours. Do we know of any? I don't think so. Is is possible? Definitely. Would the problems that this would cause be easy to debug? I don't think so.

Problems created by this would potentially cause the loss of user-input (not true data loss from the database, but still vanished into the ether), I think we need to extra precautious about side effects.

I think the TTL being 6 hours is way too high, but it has been that way for a long, long time. Given the audience needing this including site owners and working towards the goal of minimal disruption, I think we need to go with option #2 here. So, I am setting this back to Needs Work so we can

- update the hunk in form.inc/form_set_cache to take the $form_id into account
- update and polish the docs in default.settings.php
- i think this warrants a change record, which can also be used in the docs for further reference

Not sure if I will get to a new version of the patch tonight.

The idea behind a per form setting was to narrow the impact -- we'd get rid of the global setting entirely and stick to the 6 hours for non-problematic forms, as other custom/contrib code might assume it's still set to 6 hours. But not 100% sure about this, the current patch may be fine, as long as we clearly spell out that changing the global variable may have unintended effects (ideally listing those as well).

As we're not actually changing the default value from 6 hours, there's not any immediate impact to existing sites. FWIW I think that the comments in settings.php are probably sufficient - AJAX forms are mentioned, for example. I'm not sure it's practical to enumerate every possible consequence of changing the default value.

As mpdonadio mentioned, I suspect that a lot of sites where this has been a problem already have a hack (along the lines of this patch) in place. Getting the simple patch in so that those hacks can be reverted would be great.

Work could then continue on perfecting the implementation of a per-form setting in the follow up issue.

Everyone interested in this issue: if you could also take a look at #2819375: Do not make entries in "cache_form" when viewing forms that use #ajax['callback'] (Drupal 7 port) that would be fantastic. The issue here is still great to have as an option (and is ready to go), but if we can fix this at its root cause, then we could avoid generating the cache entries in the first place in ~9 out of 10 situations.

I put my RTBC + 1, but given the far reaching consequences of the patch, this indeed is an issue that needs David's sign-off.

just my two cents, we've been using this patch in 2 high high traffic sites. RTBC from my point of view

Giving this a gentle bump...

As outlined in #85, the patch from #79 should not actually change anything on existing sites and is therefore low risk.

It does, however, provide the ability to tweak the overall cache_form lifetime setting on sites where that's beneficial, and includes some documentation around this. Lots of big sites have had to do this already as a core hack.

There's a follow up issue for a more tune-able approach e.g. per-form cache lifetimes (linked to in #84). I also agree with @quicksketch that it would be good to address the cause of most cache_form problems, namely ajax forms (#87).

In the meantime though, it would be great to get this simple patch into the next D7 release if at all possible.

So this missed 7.54 at the start of Feb 2017.

I've checked that the patch in #79 still applies.

Is there anything we can do to get it into the next D7 release?

Sorry for the delay, although I think this issue is still waiting for a change record to be written (that's what other people said above), and for the Drupal 8 issue (#1286154: Allow custom default form cache expiration/lifetime)?

This patch looks fine to me, since it is nice and simple and won't change anything for existing sites. For a site that does opt-in, I also think the risk (of conflict with contrib modules) is low, but maybe I'm missing something. (Wouldn't it only be an issue for a site that tried to increase the expiration time, and even then the worst that happens is that the form stops working correctly after 6 hours which is exactly what it already does now?) A change record is still a good idea though, and if a Drupal 7.60 does wind up coming out soon it would be a good release for this to be in (like stefan said in #68).

Small nitpicks: It should be "expiration" rather than "expiry", since Drupal uses American English (https://www.drupal.org/drupalorg/style-guide/content#english) and "expiry" is, in practice, British. Also, "Ajax" rather than "AJAX". I have fixed those in the attached. I am leaving this at RTBC since those are trivial changes.

Added CR; fine with simple approach and second to RTBC.

I made some minor edits, but the change record looks good. Hopefully the Drupal 8 patch will be RTBC and ready to commit soon, in time to get this one into Drupal 7.60 in a month or so.

Why not make this setting available at /admin/config/development/performance? More cache control and monitoring is needed from the UI.

The problem is so prevalent yet undetected that I and many were or still are unaware that their 'cache_form' has grown out of control. With a little more effort the patch could make the setting available from the UI and hey why not even add a report of the size of each cache table and even a link to a web page explaining what cache tables sizes are reasonable.

While we are at it we could even add a cache truncate option to clear certain or even all cache tables on demand or on cycle with cron at particular times during the day. There are a number of articles explaining that if 'cache_form' gets too big then there can be problems with Drupal even clearing the table and then a manual table truncate must be performed. I don't understand why, but I have witnessed this myself. This issue goes into further detail, https://www.drupal.org/node/1506196. This module was also created to try to fix the problem, https://www.drupal.org/project/safe_cache_form_clear.

In my case I installed patch #92 and then assigned $conf['form_cache_expiration'] = 1800; in settings.php. I ran clear all cache and cron numerous times with no impact on the size of 'cache_form'. I assume it is because my 'cache_table' is too big. So I manually truncated the table and will now watch to see if the new 1800 expiration will keep 'cache_form' to a reasonable size.

My experiment in #99 seems to be keeping my 'cache_form' table to about 1000 rows and 50mb.

However, that still seems way to high to me. What is in that 'cache_table' blob anyway? Is that the cached form + the CSRF id per user session? If that is the case I might suggest that the right solution also needs to consider better normalization of the data. The form itself should only be cached once for the entire website and the CSRF id once per user or once per user per form. Then the CSRF id can be removed when the session is over. If all a user's form usage can share the same CSRF id then you could even store the CSRF id in the user sessions table or create a new table for that alone. This solution kinda gets at that point, https://www.drupal.org/project/session_cache_form.

Consider a website with a huge form on the front page to collect insurance applications or whatever now hit by millions of people across the globe. Would 'cache_form' create a row storing the cached form and a unique CSRF id for every user? If that is the case then messing with the expiration date is not enough of a solution.

Here is a script I wrote to monitor this if it helps anyone...

echo -e "MYSQL INFO\n" > ./RUN_AMYSQL_INFO.out
mysql 2>&1 >> ./RUN_AMYSQL_INFO.out <<EOF
SELECT sum(data_length + index_length)/1024/1024 "TOTAL MEGABYTES" FROM information_schema.TABLES;
SELECT '';
SELECT table_schema "DATABASE", sum(data_length + index_length)/1024/1024 "MEGABYTES" FROM information_schema.TABLES GROUP BY table_schema ORDER BY sum(data_length + index_length)/1024/1024 DESC;
SELECT '';
SELECT table_schema "DATABASE", table_name "TABLE", sum(data_length + index_length)/1024/1024 "MEGABYTES" FROM information_schema.TABLES GROUP BY table_schema, table_name ORDER BY sum(data_length + index_length)/1024/1024 DESC LIMIT 20;
EOF
echo -e "\nDONE" >> ./RUN_AMYSQL_INFO.out
cat ./RUN_AMYSQL_INFO.out | mail -s "MYSQL DATABASE MONITOR" youremail@yourdomain.com

@webservant316 This patch affects the expiration time of future cache_form entries, not existing ones. So applying this patch and changing the variable won't affect anything with existing cache entries. That said, they certainly should clear out soon anyway, so if they're not you're probably right that there is another issue such as #1506196: cache_form is never cleared on some sites responsible.

I don't think there should be an admin UI for this in core - it is an edge case and it's easy to mess things up if someone changes this and doesn't know what they're doing. However, there could certainly be a contrib module written to provide a UI. (Looks like there are already some contrib modules that try to deal with problems related to this, including the ones you mentioned and also https://www.drupal.org/project/optimizedb. But they may just be working around an actual bug in core.)

I've never fully understood all the details of the form cache, but I'm pretty sure the entire form does need to be cached so that there's something representing the actual state of the form for that particular user. (I can't really think of a good way to figure out what parts of the form can be shared with other users and which can't.) That said, Ajax requests are a big culprit here, and there's an issue at #2819375: Do not make entries in "cache_form" when viewing forms that use #ajax['callback'] (Drupal 7 port) that is trying to make it so they don't have to use cache_form at all.

Okay thanks.

The size of the 'cache_form' table can be disturbing. It was fully 70% of my entire database. If there is someone who understands the data store in that table perhaps something can be done to economize through normalizing the data. Now it may be too difficult because the processed form is simply cached as a blob. One idea is that the user specific elements of the blob could be represented with a substitution string and the user specific elements store elsewhere in the database. Then when the blob for a form could be shared by all users and the user data substituted back in when needed.

#103, the main problem is that the flexibility of the form system makes it pretty much impossible to say what can and can't be shared between different displays of the "same" form. Consider node forms. Depending on how you have things configured, you can show/hide fields or make then read-only based on just about any criteria (user roles, permissions, other field values, other node values, other database values). This means you can't predict what can be shared in a generic way.

Hmm, yes I see. User perms could allow completely different views of the form. I assumed it was the unique CSRF id that resulted in the need for each user to have their own cache of the form, but there are many other factors also. Is this a case where we want to allow the option for the form to not be cached at all? But then again wouldn't all anonymous users always have the same view of the form, yet I think they each have their own copy of the form in cache_form. Why is that? Anonymous users ought to be able to share the same cached form.

Could an attacker exploit this weakness? Suppose an attacker hit a form repeatedly could they create rows in this table at will fast enough to endanger the system because of bloated table size either harming MySQL or abusing disk space resources? I read one post where a Drupal admin discovered their 'cache_form' table had grown beyond 1Gb with typical use. So why not 300Gb from an attacker?

Title tweaked to reflect this being the D7 issue (D8 being dealt with in #1286154: Allow custom default form cache expiration/lifetime)

@webservant316 - I'm sure many of your points are valid (and some have been discussed before in this issue and elsewhere). However, we're hopefully getting close to getting a simple change into D7 here to mitigate the runaway cache_form problem. I'd argue any wider discussion should happen in a separate issue in order to avoid derailing this RTBC'd one.

I'd also point out that cache_form is quite different in D8 (see the other issue mentioned above) and that D7's at a phase in its development life-cycle when radical changes to the architecture are perhaps quite unlikely. That said, #2819375: Do not make entries in "cache_form" when viewing forms that use #ajax['callback'] (Drupal 7 port) which was mentioned by @quicksketch in #87 may be of some interest to you.

okay thanks.

#1286154: Allow custom default form cache expiration/lifetime is now fixed, which means it unblocks this one.

Tests similar to those added for D8.

These test simple storage and retrieval of a form to / from cache_form (didn't find existing tests specifically for that), plus verify that setting a negative value for form_cache_expiration means that a newly created entry in cache_form is already expired. Unlike in D8 we have to actually clear expired entries from cache_form in order to prevent them being returned.

Test only patch initially; the pair of "expired form" tests should fail.

This time with the actual changes too.

Would be great to get this back to RTBC if possible; the only thing we've added is similar tests to those we added in #1286154: Allow custom default form cache expiration/lifetime (although the implementation's a little different in D7).

Anyone able to do a quick review?

1. +++ b/modules/simpletest/tests/form.test
   @@ -1421,6 +1421,59 @@ class FormsFormStoragePageCacheTestCase extends DrupalWebTestCase {
   +    return array(
   +        'name' => 'Form caching',
   +        'description' => 'Tests storage and retrieval of forms from cache.',
   +        'group' => 'Form API',
   +    );
   

   Nit, two space indent.

2. +++ b/modules/simpletest/tests/form.test
   @@ -1421,6 +1421,59 @@ class FormsFormStoragePageCacheTestCase extends DrupalWebTestCase {
   +    variable_set('form_cache_expiration', -1 * (24 * 60 * 60));
   

   Nifty trick for testing this w/o a sleep.

3. +++ b/modules/simpletest/tests/form_test.module
   @@ -919,6 +919,24 @@ function form_test_storage_page_cache_rebuild($form, &$form_state) {
   +  $form['title'] = array(
   +      '#type' => 'textfield',
   +      '#title' => 'Title',
   +      '#required' => TRUE,
   +  );
   +
   +  $form['submit'] = array(
   +      '#type' => 'submit',
   +      '#value' => 'Save',
   +  );
   

   Nit, two space indent.

Otherwise, looks good to me.

Thanks - fixed the indentation.

Just want to see this test fail w/o the fix before I RTBC it.

OK, I think my hands are clean enough at this point on this to RTBC; if anyone else things otherwise, bump back to Needs Review. This looks good to me, esp the test.

Committed to 7.x - thanks! The tests look great.

I went ahead and published the change notice also.

Seems like this patch is not included in 7.60. Why is that?

The last several D7 releases have been Security fixes only. There are a several commits which have gone into 7.x-dev but that have yet to make it into an actual release. This is one of them.

7.61 perhaps? (tagging accordingly)