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
Comment | File | Size | Author |
---|---|---|---|
#69 | 3089263-69.patch | 14.25 KB | benjifisher |
|
Issue fork captcha-3089263
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:
- 3089263-captcha-session-id changes, plain diff MR !1
Comments
Comment #2
neclimdulI 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.
Comment #3
neclimdulHaven'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.
Comment #4
neclimdulpatch upload disappeared during writing...
Comment #5
agoradesign CreditAttribution: agoradesign commentedfixing typo in the issue title :)
Comment #6
neclimdulHere'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.
Comment #7
maurizio.ganovelliLast patch works well for me. Thx!
Comment #8
agoradesign CreditAttribution: agoradesign commentedfirst 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
Comment #9
maurizio.ganovelliI 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!
Comment #10
djg_tram CreditAttribution: djg_tram commentedThis 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.
Comment #11
alex0412 CreditAttribution: alex0412 commentedIs 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.
Comment #12
agoradesign CreditAttribution: agoradesign commentedbeta2 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
Comment #13
pivica CreditAttribution: pivica as a volunteer commentedComment #14
djg_tram CreditAttribution: djg_tram commentedWell, 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.
Comment #15
agoradesign CreditAttribution: agoradesign commented@djg_tram #3035883-29: CAPTCHA validation error: unknown CAPTCHA session ID
Comment #16
pivica CreditAttribution: pivica as a volunteer commented@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.
Comment #17
djg_tram CreditAttribution: djg_tram commented@agoradesign: Thanks, I hope it'll make into the regular update because I'll forget to repatch the next time around. :-)
Comment #18
agoradesign CreditAttribution: agoradesign commented@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
Comment #19
agoradesign CreditAttribution: agoradesign commentedjust seen the one from #16..that looks good, yes :) but it's only a partial solution though I fear
Comment #20
sokru CreditAttribution: sokru as a volunteer commentedIf you need recaptcha and cache on your site I'd recommend https://www.drupal.org/project/simple_recaptcha
Comment #21
joachim CreditAttribution: joachim commentedIs 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?
Comment #22
joachim CreditAttribution: joachim commentedPatch #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?
Comment #23
joachim CreditAttribution: joachim commentedOk 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:
Comment #24
joachim CreditAttribution: joachim commentedOk 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.
Comment #25
agoradesign CreditAttribution: agoradesign commented@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
Comment #26
TTNT CreditAttribution: TTNT commentedHi, 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!!!
Comment #27
joachim CreditAttribution: joachim commentedIt'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.
Comment #28
joachim CreditAttribution: joachim commented> 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?
Comment #29
freelockIt'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...
Comment #30
joachim CreditAttribution: joachim commented> 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.
Comment #31
caspervoogt CreditAttribution: caspervoogt at Plethora commented#24 solved this for me on two sites I just tested it on.
Comment #32
rgpublicI 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.
Comment #33
Andrew.Dmytriv CreditAttribution: Andrew.Dmytriv as a volunteer and at ZANZARRA Drupal Agency, Drupal Ukraine Community commented#24 works for me.
Comment #34
Petr IllekI'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.
Comment #35
fomenkoandrey CreditAttribution: fomenkoandrey commentederror with captcha ID in drupal 8.8.2
Comment #36
StefanieV CreditAttribution: StefanieV commented#24 fixed the error for me on drupal 8.8.1. Thanks!
Comment #37
acCan 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.Comment #38
acModule 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.Comment #39
kim.pepperBack to Needs Review.
Comment #40
acSo 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
Comment #41
kim.pepperReview the patch and also running this in prod with no issues.
Comment #42
fomenkoandrey CreditAttribution: fomenkoandrey commented#24 doesnt work
Comment #43
kim.pepper@fomenkoandrey can you detail the steps you took and the issues you are having including any error messages?
Comment #44
fomenkoandrey CreditAttribution: fomenkoandrey commentedI 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.
Comment #45
trebormc@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.
Comment #46
trebormcI'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.
Comment #47
snlnz CreditAttribution: snlnz commented@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.
Comment #48
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedSimply rerolling the patch from #24 and fixed D9 compatibility.
Comment #49
akic CreditAttribution: akic commented@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).
Comment #50
akic CreditAttribution: akic commented@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.)
Comment #51
apadernoComment #52
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedWhy were the tests in #48 removed? @akic can you please provide an interdiff and describe your changes.
Comment #53
imclean CreditAttribution: imclean at Digital Ink commented#48 resolves the issue for us.
Comment #54
trebormcHello @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".
Comment #57
Darren OhTest needs to be updated to be compatible with Drupal 9.
Comment #59
volegerAdded 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.
Comment #60
Darren OhI've been using this for a week now without trouble.
Comment #61
joaopauloscho CreditAttribution: joaopauloscho at Zoocha commentedI can also confirm that it's working fine.
Comment #62
gregglesOn 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:
Any feedback from maintainers on changes they'd like before this gets merged?
Comment #63
divyesh19The merge request in #59 works fine.
Can we have the fix released in a new version.
Thank you.
Comment #64
NickDickinsonWildeThe test module should have a core version specified
Comment #65
NickDickinsonWildeComment #66
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedActually it doesn't need to as long as it has
package: Testing
See https://www.drupal.org/node/3070687#test
Comment #67
Darren Oh8.8.2 is not our minimum core version, so the core compatibility must still be specified in test modules.
Comment #68
scambler CreditAttribution: scambler at Zoocha commented#48 has fixed this for my client as well
Comment #69
benjifisherWhen 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.
Comment #70
kevster CreditAttribution: kevster commentedI 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.
Comment #71
benjifisher@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:
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.
Comment #72
TTNT CreditAttribution: TTNT commentedHello! 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!
Comment #73
kim.pepperI 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.
Comment #74
Darren OhI have been running this for months with no issues.
Comment #75
gregglesSame 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.
Comment #76
gueguerreiroI'm adding a +1 here for the RTBC as well. Been running it in production for a while.
Comment #77
japerry@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.
Comment #79
japerryComment #80
agoradesign CreditAttribution: agoradesign commentedGreat news that captcha 1.2 is out now! Thank you very much