We're seeing a lot of users on a high-traffic site getting an error about the captcha session being unknown.

I suspect the culprit might be that captcha_cron() removes entries from the captcha_sessions table which are older than 1 day.

However, if PHP is configured to expire sessions after more time than that, then it's possible for a user to submit a form and get an error because the captcha_sessions entry is gone.

For example:

- user goes to the login form, starting a new PHP session and creating an entry in captcha_sessions
- 1 day passes. Cron runs and the captcha_sessions entry is deleted
- user submits the form. They have the same session ID as when the form was first shown, but captcha_validate() will fail because it won't be able to load a record from captcha_sessions.

I think the best fix here is to match the PHP session.gc_maxlifetime setting in captcha_cron() rather than use the arbitrary expiry of 1 day.

CommentFileSizeAuthor
#6 3032742-6.captcha.captchacron-removes-captchasessions-that-may-still-have-open-PHP-sessions.patch1.07 KBpivica
#2 3032742-2.captcha.captchacron-removes-captchasessions-that-may-still-have-open-PHP-sessions.patch1.09 KBjoachim
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim created an issue. See original summary.

joachim’s picture

joachim CreditAttribution: joachim commented
Issue summary: View changes
Status: Active » Needs review
FileSize
3032742-2.captcha.captchacron-removes-captchasessions-that-may-still-have-open-PHP-sessions.patch1.09 KB

Here's a patch.

Status: Needs review » Needs work

The last submitted patch, 2: 3032742-2.captcha.captchacron-removes-captchasessions-that-may-still-have-open-PHP-sessions.patch, failed testing. View results

JeroenT’s picture

JeroenT
Primary language Dutch
Location 🇧🇪
CreditAttribution: JeroenT at iO commented

I'm experiencing the same problem.

Was looking into the test and it's failing because in the function captcha_cron() ini_get('session.gc_maxlifetime') is still returning the default value. Not sure how to fix something like that.

wundo’s picture

wundo CreditAttribution: wundo at Chuva Inc. for Chuva Inc. commented

I think this can be fixed by changing CaptchaCronTestCase from being a functional test to a unit test.

pivica’s picture

pivica CreditAttribution: pivica as a volunteer commented
FileSize
3032742-6.captcha.captchacron-removes-captchasessions-that-may-still-have-open-PHP-sessions.patch1.07 KB

Got this error with latest beta4, not sure is it because of this or something else. Will test this patch and see if it will help. Patch updated against latest dev.

pivica’s picture

pivica CreditAttribution: pivica as a volunteer commented
Status: Needs work » Needs review
marcoscano’s picture

marcoscano
Primary language Portuguese, Brazil
Location Barcelona, Spain
CreditAttribution: marcoscano at Lullabot for State of Georgia commented

Cross-posting in case other people arrive here after googling the error:
The captcha upgrade from beta1 to beta4 introduced #3089263: Captcha Session ID broken with cacheable captcha backends, so you might be seeing the same error but with an unrelated issue.
(Also, see #3035883: CAPTCHA validation error: unknown CAPTCHA session ID)

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper at PreviousNext commented
Status: Needs review » Reviewed & tested by the community

This looks good to me.

ac’s picture

ac
Primary language English
Location Perth
CreditAttribution: ac commented

Can confirm this works.

trebormc’s picture

trebormc
Primary language Catalan
Location Barcelona
CreditAttribution: trebormc as a volunteer and commented

Patch # 6 looks good. At the moment it seems to work.

isholgueras’s picture

isholgueras CreditAttribution: isholgueras at Bluespark commented

I have one concern with Patch #6.

Instead of setting the project session maxlifetime to 1 day, shouldn't be better to get the existing session.gc_maxlifetime or set it to 1 day (or whatever) if null? (well, this have 1440 by default so it can't be null, but you know, something like this).

$session_time = (int) ini_get('session.gc_maxlifetime') ?: 60 * 60 * 24;

I know there are some reasons to have higher or lower session lifetime depending of the project needs.

Besides of this concern, synchronizing both sessions lifetime works for me.

Thanks!

Sorry, I didn't realized the patch was actually doing this and the ini_set was done in the Test. All good to me then :)

PatrickMichael’s picture

PatrickMichael CreditAttribution: PatrickMichael commented

Patch #6 works for me so far. Thanks

KlemenDEV’s picture

KlemenDEV CreditAttribution: KlemenDEV as a volunteer and at Pylo commented

Patch #6 seems to be working so far.

japerry’s picture

japerry
Location KVUO
CreditAttribution: japerry at Acquia commented
Status: Reviewed & tested by the community » Fixed

Looks good. Fixed!

  • japerry committed e1dc28f on 8.x-1.x authored by pivica 
    Issue #3032742 by joachim, pivica: captcha_cron() removes...
japerry’s picture

japerry
Location KVUO
CreditAttribution: japerry at Acquia commented
Status: Reviewed & tested by the community » Fixed
isholgueras’s picture

isholgueras CreditAttribution: isholgueras at Bluespark commented

Hi all,

One question that just came to my mind. Shouldn't we delete all captcha sessions as well for running sites?

function captcha_update_8001(){
  $connection = Database::getConnection();
  $connection->delete('captcha_sessions')->execute();

}

I mean, if there are existing sessions we will still have errors for a few hours or days until captcha_sessions cleans the old catpcha sessions?

Status: Fixed » Closed (fixed)

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