Problem/Motivation

We are caching a page in varnish under the same cid for authenticated and anonymous users. This page contains an (AJAX) add to cart form which creates the following issue:
1. Let's start with an empty varnish cache and empty {cache_form}.
2. As uid0 go to the page. The add to cart form gets cached in {cache_form} without a #cache_form attribute (see form_set_cache() ) and therefore without a #authcache_immutable attribute (see authcache_form_cacheobject_presave() ). The page html gets cached in varnish, the form_build_id in it.
3. As uid0 I reload the page. I get the page with the form_build_id. When I change product attributes, the AJAX fires. authcache_form_cacheobject_load() runs but since there's no #authcache_immutable set, it does not change the form. The form works as it should.
4. Now I log in and go to the same page. Logged in users are still cached by varnish so I get the html (with the same form_build_id as uid0) from varnish.
5. I try to change product attributes. AJAX fires, form_get_cache() fetches the form from the cache and just like with the uid0 case, authcache_form_cacheobject_load() does not do anything with it. So there's no #cache_token on the form, which will break fetching $form_state in form_get_cache() . I get an empty AJAX response.

Note that this only happens if the page gets cached in varnish for an uid0 request. If a logged in user triggers varnish caching, uid0 can still use the add to cart form since drupal_get_token() in authcache_form_cacheobject_load() and drupal_valid_token() in form_get_cache() works for uid0 users.

Proposed resolution

Always set $object->data['#authcache_immutable'] = TRUE; in authcache_form_cacheobject_presave() (even for forms without #cache_token ).

CommentFileSizeAuthor
#18 interdiff.txt2.14 KBznerol
#18 2813885-18.diff4.4 KBznerol
#14 interdiff.txt718 bytesznerol
#14 2813885-12.diff5.06 KBznerol
#10 2813885-9.patch5.06 KBznerol
#5 interdiff-2-5.txt649 bytesczigor
#5 authcache-2813885-5-immutable.patch1.17 KBczigor
#2 authcache-2813885-2-immutable.patch837 bytesczigor
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

czigor created an issue. See original summary.

czigor’s picture

czigor CreditAttribution: czigor commented
FileSize
authcache-2813885-2-immutable.patch837 bytes

First go, without updating related comments.

czigor’s picture

czigor CreditAttribution: czigor commented
Status: Active » Needs review
czigor’s picture

czigor CreditAttribution: czigor commented
Issue summary: View changes
czigor’s picture

czigor CreditAttribution: czigor commented
FileSize
authcache-2813885-5-immutable.patch1.17 KB
interdiff-2-5.txt649 bytes

Tested this and works. Added a comment.

czigor’s picture

czigor CreditAttribution: czigor commented
Issue summary: View changes
czigor’s picture

czigor CreditAttribution: czigor commented
Issue summary: View changes
czigor’s picture

czigor CreditAttribution: czigor commented
Issue summary: View changes
znerol’s picture

znerol CreditAttribution: znerol commented
Related issues: +#2702423: Commerce Add to Cart Form ajax and ESI / Varnish fails

This looks like a follow-up on #2702423: Commerce Add to Cart Form ajax and ESI / Varnish fails.

znerol’s picture

znerol CreditAttribution: znerol commented
FileSize
2813885-9.patch5.06 KB

I think you are right. What if we move the whole immutability-flag-business into one place (authcache_form_form_alter)? It might help people to better understand what is actually going on.

Status: Needs review » Needs work

The last submitted patch, 10: 2813885-9.patch, failed testing.

The last submitted patch, 10: 2813885-9.patch, failed testing.

The last submitted patch, 10: 2813885-9.patch, failed testing.

znerol’s picture

znerol CreditAttribution: znerol commented
Status: Needs work » Needs review
FileSize
2813885-12.diff5.06 KB
interdiff.txt718 bytes

The last submitted patch, 10: 2813885-9.patch, failed testing.

czigor’s picture

czigor CreditAttribution: czigor commented
Status: Needs review » Needs work 
+++ b/modules/authcache_form/authcache_form.module
@@ -116,13 +84,37 @@ function authcache_form_form_alter(&$form, &$form_state, $form_id) {
+  elseif (_authcache_form_allow_p13n($form_id)) {

This has come one level up. Shouldn't this be still inside the authcache_page_is_cacheable() condition?

Otherwise moving the logic to the form_alter() makes sense to me.

znerol’s picture

znerol CreditAttribution: znerol commented

There is a check for authcache_page_is_cacheable() inside _authcache_form_after_build() anyway. So I figured we might just drop that condition here.

znerol’s picture

znerol CreditAttribution: znerol commented
Status: Needs work » Needs review
FileSize
2813885-18.diff4.4 KB
interdiff.txt2.14 KB

Maybe it really does not make much sense to modify that logic. But maybe move the immutable flag stuff (form cache issues) such that the form token stuff (CSRF issues) is close together.

czigor’s picture

czigor CreditAttribution: czigor commented
Status: Needs review » Reviewed & tested by the community

Looks good to me!

das-peter’s picture

das-peter CreditAttribution: das-peter at Cando commented

Looks good to me. On my site everything seems to work just fine with the patch.
And I like it that it consolidates things.
Only nitpick I found is in an untouched part:

result, forms and their assaciated form state having that flag set are

assaciated -> associated

So I'd say let's get this in :)

  • znerol committed e11612c on 7.x-2.x 
    Issue #2813885 by znerol, czigor: Fetching form from cache_form fails
znerol’s picture

znerol CreditAttribution: znerol commented
Status: Reviewed & tested by the community » Fixed

Thanks taking the time to report, test and fix this issue.

Status: Fixed » Closed (fixed)

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