Problem/Motivation

When a form is created captcha embeds a csid in the form. This happens regardless of the cacheability of the form meaning that session can be submitted any number of times. The session re-use check skips when the form is "cacheable" for exactly this reason but the valid session validation still runs in captcha_validate() and confirms a session exists.

This leads to a serious problem though. Because the render cache of the form is not tied to the lifetime of the session or the removal of the session, the session id can continue to be served to users well after that session no longer exists.

  Cron cleanup ------------------+
                                 |
                                 v
  +------------------------------+--------------+--------------------+
  |                              |              |                    |
  |      Captcha Session 1       |   BROKEN     | Captcha Session 2  |
  |                              |              |                    |
  +----------------+-------------+--------------+--------------------+
  |                |                            |                    |
  |                |      Cache Lifetime        |                    |
  |                |                            |                    |
  +----------------+----------------------------+--------------------+

There is also likely some weirdness with session id lifetime, browser caches, and open browser windows as well that make this whole setup fairly unworkable.

Proposed resolution

Skip session validation for cacheable captchas.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork captcha-3089263

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

neclimdul created an issue. See original summary.

neclimdul’s picture

Issue summary: View changes

I feel like there might be at least further weirdness and complications with browser caches and how long a user might have a browser window open that make fixing the session validation to work with cacheable forms not very feasible. For example, my first instinct was to add a tag to the form render and clear it during session cleanup. But any form loaded immediately before the cleanup would still fail in the same way after the cleanup since it was given the expiring session id from cache.

neclimdul’s picture

Status: Active » Needs review

Haven't really tested this too much but maybe something like this? Could probably be optimized so skip the query but things start to get a little complicated since $solution is passed to validation functions later regardless of cacheability.

neclimdul’s picture

patch upload disappeared during writing...

agoradesign’s picture

Title: Catcha Session ID broken with cacheable captcha backends » Captcha Session ID broken with cacheable captcha backends

fixing typo in the issue title :)

neclimdul’s picture

FileSize
1.98 KB

Here's a possibly better patch. I did a couple things:
1) Flipped the if/else blocks and reversed the condition. This made documentation easier to write and I think clearer.
2) Added documentation about cacheable check bypass, highlighting that cacheable forms are 100% responsible for response validation.
3) Removed the TODO about D6 and confusion with _captcha_get_posted_captcha_info() behavior and replaced it with a comment about how session invalidation behaves with _captcha_get_posted_captcha_info(). That process might be able to be hardened in a follow up but it seems to work.

This doesn't actually change any logic from #3/#4 and is functionally the same. Its just some additional docs and hygiene related to the fix.

maurizio.ganovelli’s picture

Last patch works well for me. Thx!

agoradesign’s picture

first of all thanks for the work done already, but have you really tested this patch? I've trusted on the existing reviews and just tested, if there ain't any error coming up... but the session stuff cannot really be tested on empty cache. I got reports today, that there are still such errors, and I see them in the log.

I've then added some logging code directly on the production site, and what was missing, is this $captcha_info['cacheable'] information. This key is missing at all (using reCAPTCHA btw). And I really believe that this can never be set. Just have a look at the element class here: https://git.drupalcode.org/project/captcha/blob/8.x-1.x/src/Element/Capt...

Instead, a hidden form element is set for cacheable captchas, see https://git.drupalcode.org/project/captcha/blob/8.x-1.x/src/Element/Capt...

Therefore I was adjusted the patch from #5, where I'm asking for this form value now

maurizio.ganovelli’s picture

I found some errors on user registration form with patch #6. I've applied patch #8: until now no errors, so i've deployed it on a production site. Thank you!

djg_tram’s picture

This needs more work, sadly. I was very happy to see #8 and applied it and was glad to see the problem go away. But I found another, more sinister one: I simply forgot to check the reCAPTCHA today, just provided name and password and the site let me in. Clearing the cache restored the captcha functionality all right. So, clearly, while this patch cleared the error message and the impossibility of logging in, it created a way to sneak in without a captcha.

alex0412’s picture

Is there a version we should downgrade to, as long as this bug is not fixed?

Just checked our git history for the composer.lock file. It seems the bug was (re)introduced in 8.x-1.0-beta2 (5 October 2019).
I'll downgrade to 8.x-1.0-beta1 and see what happens.

agoradesign’s picture

beta2 introduced the cacheable captcha support, so beta1 should theoretically work.. alternatively we could post a workaround patch for reCAPTCHA module that removes that "cacheable" definition from recaptcha in the meantime

pivica’s picture

djg_tram’s picture

Well, yes, and when can we expect that workaround to appear? We're stuck with sites and angry users who can't log in fairly frequently.

agoradesign’s picture

pivica’s picture

@djg_tram or try this patch #3032742-6: captcha_cron() removes captcha_sessions that may still have open PHP sessions, i don't have this problem any more for now after that patch is applied.

djg_tram’s picture

@agoradesign: Thanks, I hope it'll make into the regular update because I'll forget to repatch the next time around. :-)

agoradesign’s picture

@djg_tram - nope, that patch in #15 is only a workaround. It is theoretically ok for recaptcha to get cached. It's still the captcha module's caching logic that fails. but preventing recaptcha from being cached is the easiest solution for the moment imho

agoradesign’s picture

just seen the one from #16..that looks good, yes :) but it's only a partial solution though I fear

sokru’s picture

If you need recaptcha and cache on your site I'd recommend https://www.drupal.org/project/simple_recaptcha

joachim’s picture

Issue tags: +Needs tests

Is this something that can have tests written for it?

I am reproducing this locally as follows:

1. Use a form that normally is cached, and enable the ReCaptcha captcha on it. (I made a custom form with just a checkbox and a submit bottom to make things simpler. I also added output of the csid to the form by adding a #markup element to \Drupal\captcha\Element\Captcha.)
2. Load the page.
3. Note the latest csid in the {captcha_sessions} table.
4. Load the form in another browser session (close the anonymous window, or use another browser). Note the same csid is used. Note that in the {cache_page} table, the page is cached, with an expiry of -1.
5. Delete the rows from the {captcha_sessions} table, simulating the action of a cron run.
6. Reload the page and try to submit the form: the error occurs. Note that the same csid is still being used. So from this point on, ALL users who hit this page will be getting the form with the obsolete csid.

> This leads to a serious problem though. Because the render cache of the form is not tied to the lifetime of the session or the removal of the session, the session id can continue to be served to users well after that session no longer exists.

Based on my investigation above, it's the page cache of the form rather than the render cache. Though does that make much of a difference?

joachim’s picture

Patch #8 fixes my scenario, but I can see that it's still using the *same* csid that doesn't have a row in the {captcha_sessions} table. Is that ok?

joachim’s picture

Ok so this seems to allow for the case I'm describing, there no solution is found in the {captcha_sessions} table, but because the form is cacheable that's ok:

  // If the form is cacheable where all solution validation is handed off or if
  // we found a session with a solution then continue with validation.
  $is_cacheable = (bool) $form_state->getValue('captcha_cacheable', FALSE);
  if ($is_cacheable || $solution !== FALSE) {
joachim’s picture

Ok here's some tests that show the patch #8 fixes the problem when cron clears a {captcha_sessions} CSID that is still cached in a form.

As you can see from the setup in the test, a cacheable captcha type currently MUST provide a 'captcha_validate' callback, as otherwise the default captcha_validate_strict_equality() will fail validation when there is no solution to load from {captcha_sessions}.

Having written the test for this, it occurs to me that a better solution might be to define a cache tag for CSIDs, and set that on the form, and then invalidate all the cache tag for the CSIDs that we delete in hook_cron(). However, I think that approach would still allow this problem to occur when the *form* cache is used (rather than the page cache) and the CSID is gone.

Also, from my reading of form cache code, that could only happen if the CSID was deleted between the form being first loaded and then submitted, which is a much rarer occurrence.

agoradesign’s picture

@joachim thx for the tests.. I still believe that this fixes at least a part of the problem, but we should take comment #10 seriously, as there still may be some problems!? I use to include both patches, the one from #8 here + the workaround patch from the other issue that unmarks recaptcha as cacheable. I'm however unsure, if I ever experienced the described problem in #10 - unfortunately I have nearly no time to play around with that atm. So I guess, I was just applying both to be on the safe side.

One interesting info would be @djg_tram, if you have turned on the static page_cache module? I've already seen destroying a lot of dynamic stuff on sites. You often can't rely on caching working as expected imho

TTNT’s picture

Hi, can anyone confirm if this is a captcha or a recaptcha problem? Updated to 8.8 and got these errors on multiple sites. I've set them all to "math" now, but does that actually fix it? Or do I need to disable captcha entirely untill there is a solution? Thanks!!!

joachim’s picture

It's a problem with Captcha module, which is specific to captcha types that declare themselves as being cacheable. Recaptcha module's recaptcha module is one such type. Captcha module itself doesn't have any cacheable types. So the math type will work fine, for instance.

joachim’s picture

> But I found another, more sinister one: I simply forgot to check the reCAPTCHA today, just provided name and password and the site let me in. Clearing the cache restored the captcha functionality all right.

I've been trying to reproduce this, and I can't at all.

With caches enabled, I've tried both a custom form and the user login form, using different sessions which would share the cached page.

I've also tried disabling the captcha on the login form, then submitting the form to warm the cache, then enabling the captcha point. When the form is reloaded, the captcha shows.

@djg_tram have you managed to reproduce this yourself? Could you give more details about your caching setup?

freelock’s picture

It's not a problem with the captcha showing -- it's a problem with the captcha validation.

We've now hit this on a bunch of sites, and last week we patched Recaptcha across all our sites to declare it non-cacheable until this is resolved.

What we saw was that everything looked fine until you attempted to submit a form with a recaptcha -- you get an "invalid recaptcha session" error when you submit.

I was able to confirm this problem, but have not gone through exact steps to reproduce -- I would think submitting from 2 - 3 different browsers should do it...

joachim’s picture

> What we saw was that everything looked fine until you attempted to submit a form with a recaptcha -- you get an "invalid recaptcha session" error when you submit.

The 'invalid recaptcha session' error on form submission is what everyone else on this issue was getting, me included. The patch I attached fixes it.

What I was asking about was what @djg_tram reported, which was that the form would allow submission with the repcatcha uncompleted. I've not managed to reproduce that and I'd like more information about it.

caspervoogt’s picture

#24 solved this for me on two sites I just tested it on.

rgpublic’s picture

I just received a complaint from a customer that all of a sudden no forms could be submitted anymore with message "Captcha validation error: Unknown CAPTCHA session ID". I couldn't reproduce the error but applying patch #24 immediately resolved the problem.

Andrew.Dmytriv’s picture

#24 works for me.

Petr Illek’s picture

I'm using the patch from #24 from 22nd of December. Suddenly the error reappeared on the site last week.
Cron is running regularly.
I've to manually clear cache using drush to be able to log in.

fomenkoandrey’s picture

error with captcha ID in drupal 8.8.2

StefanieV’s picture

#24 fixed the error for me on drupal 8.8.1. Thanks!

ac’s picture

Status: Needs review » Needs work

Can confirm that #24 does not ultimately work. After an extended period of time, the initial error returns. Can't be sure this is true anymore due to an error in our patch testing.

ac’s picture

Priority: Major » Critical

Module is broken in its current state. Until this is fixed it can't properly be used in production as far as I can tell.Can't be sure this is true anymore due to an error in our patch testing.

kim.pepper’s picture

Status: Needs work » Needs review

Back to Needs Review.

ac’s picture

So far #24 is working well in conjunction with this https://www.drupal.org/project/captcha/issues/3032742#comment-13364556

We haven't properly tested #24 in isolation so I will leave it for someone else to mark that as RTBC

kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community

Review the patch and also running this in prod with no issues.

fomenkoandrey’s picture

#24 doesnt work

kim.pepper’s picture

@fomenkoandrey can you detail the steps you took and the issues you are having including any error messages?

fomenkoandrey’s picture

I wrote about errors here, the next post I reported an error in the new drupal version and applied screenshots.
https://www.drupal.org/project/recaptcha/issues/3035883#comment-13448613
https://prnt.sc/r01ccm
error with Captcha Session ID.

errors are present on all sites (3), core and plugins - updated to the latest version.

recaptha is currently disabled due to the impossibility of using this module.

trebormc’s picture

Status: Reviewed & tested by the community » Needs work

@joachim # 28
I confirm @djg_tram's problem in comment # 10

1. apply patch
2. drush cr to empty caches
3. login to the drupal from the login form (where the recaptcha appears)
4. logout
5. go back to login, but without clicking on the recaptcha
6. Caches have to be emptied again to make recaptcha validation work again. And we go back to the loop of point 3

This is allowing you to skip the recaptcha validation !!!
The captcha is visible, but it is not necessary to do them to be able to login !!!!

For what it's worth, our website uses CloudFlare (purge and cloudflare modules) to improve Drupal caches.

trebormc’s picture

I'm not sure if it's a problem with the captcha module or the recaptcha module, but with caches they don't work.

In the end I am using:
https://www.drupal.org/project/simple_recaptcha
That does allow you to have active caches.

snlnz’s picture

@trebormc - awesome man, thanks for the heads up, fixed my issues where I couldn't get a captcha working at all with a D8 site using Drupal BOA. Definitely an issue in the module as simple_recaptcha works perfectly.

akic’s picture

@trebormc @djg_tram What were your captcha persistence settings when you recreated the captcha bypass? I'm wondering if a success could be persisted to the (cached) session, resulting in it being skippable later. If that's the problem, the patch should probably also prevent cacheable captcha types from persisting their solutions (and also always run captcha_validate).

akic’s picture

@trebormc @djg_tram What were your captcha persistence settings when you recreated the captcha bypass?

I'm wondering if a success could be persisted to the (cached) session, resulting in it being skippable later. If that's the problem, it might be fixed by completely ignoring captcha_sessions for cacheable types. (Which is probably for the best anyway, since a cacheable type shouldn't care about the session and it eliminates needing to worry about the correctness of the cache and/or session state.)

apaderno’s picture

Status: Needs work » Needs review
acbramley’s picture

Why were the tests in #48 removed? @akic can you please provide an interdiff and describe your changes.

imclean’s picture

#48 resolves the issue for us.

trebormc’s picture

Hello @akic,
I say it from memory, but the configuration I had was the one that comes by default with the captcha module, I only activated the recaptcha module in the login form.
I think that the default persistence is to "omit in multi-step".

Darren Oh made their first commit to this issue’s fork.

Darren Oh’s picture

Issue tags: -Needs tests

Test needs to be updated to be compatible with Drupal 9.

voleger made their first commit to this issue’s fork.

voleger’s picture

Added test compatibility fixes, also the changes into the cacheability test. Added 1 call to the front page to prepare the cache before the assertion is the page is cached.

Darren Oh’s picture

Status: Needs review » Reviewed & tested by the community

I've been using this for a week now without trouble.

joaopauloscho’s picture

I can also confirm that it's working fine.

greggles’s picture

On a site that was experiencing this problem daily I deployed the attached merge request and it's solved the problem.

For folks using composer-patches, here's a potential snippet to use:


            "drupal/captcha": {
                "Captcha Session ID broken with cacheable captcha backends": "https://git.drupalcode.org/project/captcha/-/merge_requests/1.diff"
            }

Any feedback from maintainers on changes they'd like before this gets merged?

divyesh19’s picture

The merge request in #59 works fine.
Can we have the fix released in a new version.
Thank you.

NickDickinsonWilde’s picture

Status: Reviewed & tested by the community » Needs work

The test module should have a core version specified

NickDickinsonWilde’s picture

Status: Needs work » Needs review
acbramley’s picture

The test module should have a core version specified

Actually it doesn't need to as long as it has package: Testing

See https://www.drupal.org/node/3070687#test

Darren Oh’s picture

8.8.2 is not our minimum core version, so the core compatibility must still be specified in test modules.

scambler’s picture

#48 has fixed this for my client as well

benjifisher’s picture

When applying a patch with composer, I prefer to use a fixed diff instead of the latest version of the merge request (MR). So I am attaching the current https://git.drupalcode.org/project/captcha/-/merge_requests/1.patch as a patch.

kevster’s picture

I tried the latest patch but it broke the site:

The website encountered an unexpected error. Please try again later.
ParseError: syntax error, unexpected 'else' (T_ELSE) in Drupal\Core\Extension\Extension->load() (line 552 of modules/contrib/captcha/captcha.module).

I need to check the patch applied properly in phpstorm - it told me it had.

benjifisher’s picture

@kevster:

What version of the module are you patching?

When I apply the patch from #69 to the latest development version (from git), Line 552 is the third line of this snippet:

        // Record success.
        \Drupal::database()->update('captcha_sessions')
          ->condition('csid', $csid)
          ->fields(['status' => CAPTCHA_STATUS_SOLVED])
          ->expression('attempts', 'attempts + 1')
          ->execute();

When I apply the patch to 8.x-1.1, Line 552 is the second line of that snippet. I do not see any syntax errors there.

TTNT’s picture

Hello! Are there any plans for a stable release solving this issue? I understand this is required to get recaptcha to work in Drupal, and there are no (maintained, stable) alternatives for all I know. Thanks a lot!

kim.pepper’s picture

I have been granted maintainer privileges, but don't use this module, or know much about how it works. I'm happy to create a new release if there is consensus that this patch solves the problem and is well reviewed.

Darren Oh’s picture

Status: Needs review » Reviewed & tested by the community

I have been running this for months with no issues.

greggles’s picture

Same as Darren Oh in #74: runninng this for months, it fixes a critical bug, I haven't identified any problems. I agree this is RTBC.

gueguerreiro’s picture

I'm adding a +1 here for the RTBC as well. Been running it in production for a while.

japerry’s picture

@kim.pepper, I'm working on a few other issues at the moment, and can get this one merged in as well. Waiting for a few things to pass tests before making a release.

  • japerry committed fb84dda on 8.x-1.x authored by benjifisher
    Issue #3089263 by Darren Oh, joachim, neclimdul, voleger, agoradesign,...
japerry’s picture

Status: Reviewed & tested by the community » Fixed
agoradesign’s picture

Great news that captcha 1.2 is out now! Thank you very much

Status: Fixed » Closed (fixed)

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