SA-CORE-2014-002 forward port only checks internal cache

To clarify: "enabling the page cache" was done:

1. in Drupal 7 via the cache setting (variable_get('cache', 0), the "Cache pages for anonymous users" checkbox on the performance settings page)
2. in Drupal 8 via the system.performance.cache.page.use_internal config key (the "Use internal page cache" checkbox on the performance settings page)

So far, they're the same, and we're fine. Note that on the performance settings page in both 7 and 8, you have the ability to configure the max-age to be used in Cache-Control headers emitted by Drupal.

But what's different, is that in Drupal 7, you had to enable the page cache to get the maximum age you had configured to actually be used. So, effectively, you had to enable the page cache in Drupal 7 to get the headers you need for a reverse proxy to have any effect at all. That was strange and annoying, and therefore it was fixed in Drupal 8, where that is no longer required: just configuring a maximum age will make it show up on any cacheable page (e.g. frontpage as the anonymous user).

So, the patch was ported 1:1 from Drupal 7 to 8, but because it is now possible that even without Drupal's page cache being enabled reverse proxies will be caching Drupal pages, the "immutable" flag is now no longer guaranteed to be set when necessary.

(See http://www.wunderkraut.com/blog/caching-with-varnish-drupal-7-and-cache-..., http://www.rklawson.com/blog/2012/04/14/caching-drupal-7-varnish-apc-and... for info about Drupal 7.)

Ideas so far for fixing this:

1. Set the "immutable" flag for all anonymous users. (But then modules like authcache need to take extra precautions… which is probably fine.)
2. Make any page that is cacheable (see the logic for calculating $is_cacheable in \Drupal\Core\EventSubscriber\FinishResponseSubscriber::onRespond()) set the "immutable" flag.

I think we also want this tag?

Was going for a quick response and it grew...

So option 1 we'd be enforcing the rule in code that only anonymous forms can ever be cached. That's definitely true for common use cases at least.
Pros:
Simple
Cons:
Makes assumptions about site caching behavior (though seemingly reasonable ones)
Would likely require patching core to change the behavior

So option 2 assumes that if a page is cacheable, it is being cached somewhere. That is also a pretty reasonable assumption.
Pros:

• Relies on an extensible system that defines site behavior, does not make assumptions about site cache behaviors

Cons:

• Relies on an extensible system that defines site behavior.
• Resolving behavior is more complex then checking for anonymous user
• Complexity could affect performance?
• Possibility of serving immutable form id's to non cached pages?

I am not sure it will be a used much but I like the idea of assuming that the page is being cached if its cacheable. Assuming the questions in the cons can be resolved or crossed out. Performance might need a quick profile and I can't think of anything immediately wrong with the non-cached immutable form id but maybe someone else knows of something?

Side note, isn't the logic in \Drupal\Core\EventSubscriber\FinishResponseSubscriber::onRespond() basically the same logic we already have in \Drupal\Core\Form\FormCache::isPageCacheable()?

https://www.drupal.org/SA-CORE-2014-002 says:

Drupal's form API has built-in support for temporary storage of form state, for example user input. This is often used on multi-step forms, and is required on Ajax-enabled forms in order to allow the Ajax calls to access and update interim user input on the server.

When pages are cached for anonymous users (either by Drupal or by an external system), form state may leak between anonymous users. As a consequence there is a chance that interim form input recorded for one anonymous user (which may include sensitive or private information, depending on the nature of the form) will be disclosed to other users interacting with the same form at the same time. This especially affects multi-step Ajax forms because the window of opportunity (i.e. the time span between user input and final form submission) is indeterminable.

This vulnerability is mitigated by the fact that Drupal core does not expose any such forms to anonymous users by default. However, contributed modules or individual sites which leverage the Drupal Form API under the aforementioned conditions might be vulnerable.

(Emphasis mine.)

In other words, there is a problem when:

1. dealing with a form that stores things in form state (a minority of forms do this)
2. different users (A and B, not user C on 2 different devices) are given the exact same form (with the same form build ID)
3. only something like https://www.drupal.org/project/authcache would run into problems, because only then do authenticated users get the same form. Such modules can just implement hook_form_alter() and set the immutable flag — so no patching to core would be necessary for them; they'd only have to take care of it themselves, just like in Drupal 7 (see https://www.drupal.org/node/2242659).
4. AFAICT there is no problem with setting the immutable flag unnecessarily (in case anonymous pages are actually all generated independently). It just means that some unnecessary work is done.

The upside of not going for option 2, is that it automatically works with any reverse proxies — because Drupal's page cache policies cannot model how those behave, since they're external to Drupal. Varnish, CDNs, internet provider proxies, and even a search engine's cache.

1. dealing with a form that stores things in form state (a minority of forms do this)

Unless you use Ajax forms (e.g. Drupal Commerce).

2. different users (A and B, not user C on 2 different devices) are given the exact same form (with the same form build ID

Applies if you use any server side page caching method.

3. only something like https://www.drupal.org/project/authcache would run into problems, because only then do authenticated users get the same form.

Regrettably not unless you never expose any Ajax forms on cached pages.

4. AFAICT there is no problem with setting the immutable flag unnecessarily (in case anonymous pages are actually all generated independently). It just means that some unnecessary work is done.

Partially agree. It is desirable to not set it after the form cache has been cloned and the form-build id has been replaced for a particular user/session.

Unless you use Ajax forms (e.g. Drupal Commerce).

Yes, but a minority of forms uses AJAX forms. I just wanted to indicate: it's only a subset of forms that uses FormState, i.e. not 100% of forms use it.

Applies if you use any server side page caching method.

Or even a reverse proxy! i.e. not necessarily the origin server, and not necessarily under your control.

Regrettably not unless you never expose any Ajax forms on cached pages.

What do you mean?

Wim, your final comment in #5 implies option 2 doesn't work out of the box with reverse proxies. If we use the exact same logic that defines how Drupal tells proxies if pages are cacheable how would it not work out of the box with proxies?

#10: because a reverse proxy may have its own configuration (think VCL files). In other words: it may not simply respect Cache-Control headers.

@Wim Leers, when using gateway caches, you normally also install the respective integration module. This is mainly in order to make cache-flushing work. With Drupal 8 and cache-tags, it will be even more important that the backend triggers the proper ban()s. Therefore I do not think that core can/should do anything more than what has been implemented in the original SA for D7.

Right, while I wrote #11, I was also wondering whether my argument was valid. I agree with you (#12 & #10) that my argument is pointless, because if we go down that route, you can go so far as saying that the reverse proxy is caching responses for authenticated users too, in which case… you're utterly screwed.

So, basically, yes, both options 1 and 2 in #2 & #4 are viable.

Except… (!!!) that we can only know about the request cache policy. Not the response cache policy. Because the response cache policy includes the page cache kill switch. And while building a form, we cannot possibly know whether that killswitch is going to be activated.
Therefore, we cannot actually know whether a response will be cacheable. And therefore option 1 (anonymous) is safer than option 2 (which requires knowing whether the response being built is going to be cacheable, which is impossible to know while building a form).

(That's what I noticed while working on #5, but I completely forgot to explain that. Wim--)

The default of Akamai usually is to cache everything for 10 min and ignore any and all cache control headers (btw.).

Only Edge-Control is respected.

To the issue:

- I think I agree with catch that form state should be tied to the SESSION of the user.

For getting we would of course never open a SESSION, but for progressing through a workflow - e.g. multi step form - we would.

On the hand the same can be achieved without SESSION, but similar to the approach within cacheable_csrf we need to:

- Distinguish between creation and execution of a form
- Executing a form can store state, creation cannot

Any variation in form creation needs to be done in the same way, but that is achievable:

a) because if the original was coming from SESSION, we know the SESSION and can do the same
b) If it comes from a cookie, the cookie is the same.
b) if it comes from e.g. a GET parameter for an anonymous user, the GET can be stored as hidden text - the security level is the same.

On the other hand our normal forms already work like that, so its just the AJAX special case that would need to be changed in this way.

While 'hacky' in a way, cacheable_csrf has shown it is possible to re-create the form fully and not depend on any cache, so can we add the necessary API's restrictions to make this all simpler?

Just some more food for thought.

To ensure I understand things: #8 and #14 basically are saying that another option (let's call it option 3) is to remove the immutable flag altogether, by tying form state to $_SESSION?

The immutable flag was just the least intrusive way to fix the issue in D6/D7. All the better if we can find a fix for D8 which works regardless of whether the resulting page is cached or not.

#15 Not yet clear how, but lets make it a goal to kill form_state. That solves also a lot of memory problems related to serialization.

The core committers talked and agree this is a critical, since it's a security issue. Tagging.

So here it is with session for form state cache entries stored in $_SESSION instead of key-value expiry
Just for giggles.

+++ b/core/lib/Drupal/Core/Form/FormCache.php
@@ -212,7 +217,11 @@ public function setCache($form_build_id, $form, FormStateInterface $form_state)
+      $session = $this->requestStack->getCurrentRequest()->getSession();

Sometimes $session can be NULL, any pointers on how to boot a new one?

The session is populated in the Session middleware (for normal requests going through $kernel->handle()) and in prepareLegacyRequest() for old procedural code. Currently the session is not available from the request in the installer (that is taken care of in #2228393: Decouple session from cookie based user authentication).

The session is never available from the request when running from the command line (PHP_SAPI === 'cli'). As a result it will not be there on the simpletest parent site when running from the command line and/or when the Request on top of the stack was instantiated manually.

I will take a look at the test failures.

The session is never available from the request when running from the command line (PHP_SAPI === 'cli'). As a result it will not be there on the simpletest parent site when running from the command line and/or when the Request on top of the stack was instantiated manually.

Indeed. I tested some of these failing tests from the command line, and they failed, but work when run through the Simpletest module's UI (not sure if all of them do or not).

I can reproduce the failures in Drupal\user\Tests\UserAdminTest through the Simpletest UI (but not through manual testing of the form).

The fatal error is a bit baffling, somehow the FieldItemDataDefinition::$fieldDefinition is being changed from a proper field definition object to an array:

Array
(
    [storage] => Drupal\field\FieldStorageConfigStorage
    [access] => Drupal\Core\Entity\EntityAccessControlHandler
)

. It doesn't happen during instantiation of the object either, since that code relies on it being a proper object.

Making up a new tag, since Daniel cited this as something that can't really be moved on until a direction is chosen.

NOTE: once #2263569: Forms using #ajax generate cached form data for every user on every request lands, this issue will be fixed as well :)

Not necessarily true. Even if we remove core's usages of custom #ajax['url'], we still need to consider whether to leave it as an option for contrib. But, tim.plunkett and I have been discussing an approach that would fix this issue for such cases. He'll post a POC of that here for feedback once it's sufficiently promising.

Huh? I remember discussions concluding that the "immutable" flag becomes obsolete with #2263569: Bypass form caching by default for forms using #ajax.?

Those discussions might have been based on outdated assumptions of how that issue would be solved. But, the "immutable" flag will be obsolete with the approach that Tim might post here, so maybe you're thinking of discussions about that? These issues were kind of blended together when we discussed them at Drupalcon LA and since.

Also, you might have been in a discussion that assumed we'd remove #ajax['url'] and all other $form_state->setCached() ability on GET requests, even for contrib, which maybe we should, and which would then solve this issue. But that's not part of the current scope of #2263569: Bypass form caching by default for forms using #ajax., and should get its own issue to evaluate the pros and cons of.

Updated the issue summary with 3 options on how to proceed. Unpostponing this issue, since there's nothing blocking us from choosing an option. If we choose option 1 or 2, we'll need to postpone on the corresponding issue, but my recommendation is option 3, so that a supported upgrade path isn't held up on this.

Implemented suggestion nr.3 and expanded the documentation a bit.

Can we try what happens when we always set the immutable flag?

Discussed with the core maintainers this morning and we decided this does not need to block an upgrade path.

back to option 3

I see, so we basically go the 3) way + test coverage which is IMHO the most important part of the issue now.
Would it be possible to provide a test only patch, so we could compare the test together with #2502785: Remove support for $form_state->setCached() for GET requests

Yes, lets add some tests, which is a valuable actionable item for the London sprint.

Added some test coverage

Such noob

Nice! So the question is now, do we get this IN and remove the alter code once #2502785: Remove support for $form_state->setCached() for GET requests is in?

Does someone has an opinion about it?

I think it makes sense to add this in as-is, because after #2502785: Remove support for $form_state->setCached() for GET requests, it might make sense to open a non-critical follow-up to remove all traces of $form_state->getBuildInfo()['immutable'], of which this is just one.

Yeah to independent babysteps and actual progress instead of dependency chains!

Had a look at the test coverage and it makes sense for me.
We already have additional test coverage in \Drupal\system\Tests\Form\FormStoragePageCacheTest::testRebuildFormStorageOnCachedPage,
which ensures that immutability works still in general, so IMHO we are fine here.

RTBC + 1