Issue #3089263 by Darren Oh, joachim, neclimdul, voleger, agoradesign,...

Issue #3089263 by Darren Oh, joachim, neclimdul, voleger, agoradesign, acbramley, NickDickinsonWilde, benjifisher, akic: Captcha Session ID broken with cacheable captcha backends

captcha.module

@@ -482,70 +482,33 @@ function captcha_validate($element, FormStateInterface &$form_state) {
   // TODO: is this correct in all cases: see comments in previous revisions?
   $csid = $captcha_info['captcha_sid'];
 
-  $solution = \Drupal::database()
-    ->select('captcha_sessions', 'cs')
-    ->fields('cs', ['solution'])
-    ->condition('csid', $csid)
-    ->execute()
-    ->fetchField();
-
   // Bypass captcha validation if access attribute value is false.
   if (empty($captcha_info['access'])) {
     return FALSE;
   }
 
-  // @todo: what is the result when there is no entry for
-  // the captcha_session? in D6 it was FALSE, what in D7?
-  if ($solution === FALSE) {
-    // Unknown challenge_id.
-    // TODO: this probably never happens anymore now that there is detection
-    // for CAPTCHA session reuse attacks in _captcha_get_posted_captcha_info().
-    $form_state->setErrorByName('captcha', t('CAPTCHA validation error: unknown CAPTCHA session ID. Contact the site administrator if this problem persists.'));
-    \Drupal::logger('CAPTCHA')->error(
-      'CAPTCHA validation error: unknown CAPTCHA session ID (%csid).',
-      ['%csid' => var_export($csid, TRUE)]);
-  }
-  else {
-    // Get CAPTCHA validate function or fall back on strict equality.
+  // 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) {
+    // Completely ignore the captcha_sessions table, since the captcha_sid can get reused by the cache.
+
+    $solution = FALSE;
     $captcha_validate = $element['#captcha_validate'];
     if (!function_exists($captcha_validate)) {
-      $captcha_validate = 'captcha_validate_strict_equality';
+      // Cacheable CAPTCHAs must provide their own validation function.
+      $form_state->setErrorByName('captcha', t('CAPTCHA configuration error: Contact the site administrator.'));
+      \Drupal::logger('CAPTCHA')->error(
+        'CAPTCHA configuration error: cacheable CAPTCHA type %challenge did not provide a validation function.',
+        ['%challenge' => $captcha_info['captcha_type']]);
     }
     // Check the response with the CAPTCHA validation function.
     // Apart from the traditional expected $solution and received $response,
     // we also provide the CAPTCHA $element and $form_state
     // arrays for more advanced use cases.
-    if ($captcha_validate($solution, $captcha_response, $element, $form_state)) {
-
-      // Get the CAPTCHA persistence setting.
-      $captcha_persistence = \Drupal::config('captcha.settings')
-        ->get('persistence');
-
-      if (in_array($captcha_persistence,
-        [
-          CAPTCHA_PERSISTENCE_SKIP_ONCE_SUCCESSFUL,
-          CAPTCHA_PERSISTENCE_SKIP_ONCE_SUCCESSFUL_PER_FORM_TYPE,
-        ])) {
-        // Only save the success in $_SESSION if it is actually needed for
-        // further validation in _captcha_required_for_user(). Setting
-        // this kills the page cache so let's not be cavalier about it.
-        $_SESSION['captcha_success_form_ids'][$form_id] = $form_id;
-      }
-
-      // Record success.
-      \Drupal::database()->update('captcha_sessions')
-        ->condition('csid', $csid)
-        ->fields(['status' => CAPTCHA_STATUS_SOLVED])
-        ->expression('attempts', 'attempts + 1')
-        ->execute();
-    }
-    else {
+    if (!$captcha_validate($solution, $captcha_response, $element, $form_state)) {
       // Wrong answer.
-      \Drupal::database()->update('captcha_sessions')
-        ->condition('csid', $csid)
-        ->expression('attempts', 'attempts + 1')
-        ->execute();
-
       $form_state->setErrorByName('captcha_response', t('The answer you entered for the CAPTCHA was not correct.'));
       // Update wrong response counter.
       if (\Drupal::config('captcha.settings')->get('enable_stats', FALSE)) {
@@ -557,17 +520,97 @@ function captcha_validate($element, FormStateInterface &$form_state) {
         ->get('log_wrong_responses', FALSE)
       ) {
         \Drupal::logger('CAPTCHA')->notice(
-          '%form_id post blocked by CAPTCHA module: challenge %challenge (by module %module), user answered "@response", but the solution was "@solution".',
+          '%form_id post blocked by CAPTCHA module: challenge %challenge (by module %module).',
           [
             '%form_id' => $form_id,
-            '@response' => $captcha_response,
-            '@solution' => $solution,
             '%challenge' => $captcha_info['captcha_type'],
             '%module' => $captcha_info['module'],
           ]);
       }
     }
   }
+  else {
+    $solution = \Drupal::database()
+      ->select('captcha_sessions', 'cs')
+      ->fields('cs', ['solution'])
+      ->condition('csid', $csid)
+      ->execute()
+      ->fetchField();
+
+    if ($solution !== FALSE) {
+      // Get CAPTCHA validate function or fall back on strict equality.
+      $captcha_validate = $element['#captcha_validate'];
+      if (!function_exists($captcha_validate)) {
+        $captcha_validate = 'captcha_validate_strict_equality';
+      }
+      // Check the response with the CAPTCHA validation function.
+      // Apart from the traditional expected $solution and received $response,
+      // we also provide the CAPTCHA $element and $form_state
+      // arrays for more advanced use cases.
+      if ($captcha_validate($solution, $captcha_response, $element, $form_state)) {
+
+        // Get the CAPTCHA persistence setting.
+        $captcha_persistence = \Drupal::config('captcha.settings')
+          ->get('persistence');
+
+        if (in_array($captcha_persistence,
+          [
+            CAPTCHA_PERSISTENCE_SKIP_ONCE_SUCCESSFUL,
+            CAPTCHA_PERSISTENCE_SKIP_ONCE_SUCCESSFUL_PER_FORM_TYPE,
+          ])) {
+          // Only save the success in $_SESSION if it is actually needed for
+          // further validation in _captcha_required_for_user(). Setting
+          // this kills the page cache so let's not be cavalier about it.
+          $_SESSION['captcha_success_form_ids'][$form_id] = $form_id;
+        }
+
+        // Record success.
+        \Drupal::database()->update('captcha_sessions')
+          ->condition('csid', $csid)
+          ->fields(['status' => CAPTCHA_STATUS_SOLVED])
+          ->expression('attempts', 'attempts + 1')
+          ->execute();
+      }
+      else {
+        // Wrong answer.
+        \Drupal::database()->update('captcha_sessions')
+          ->condition('csid', $csid)
+          ->expression('attempts', 'attempts + 1')
+          ->execute();
+
+        $form_state->setErrorByName('captcha_response', t('The answer you entered for the CAPTCHA was not correct.'));
+        // Update wrong response counter.
+        if (\Drupal::config('captcha.settings')->get('enable_stats', FALSE)) {
+          Drupal::state()->set('captcha.wrong_response_counter', Drupal::state()
+            ->get('captcha.wrong_response_counter', 0) + 1);
+        }
+
+        if (\Drupal::config('captcha.settings')
+          ->get('log_wrong_responses', FALSE)
+        ) {
+          \Drupal::logger('CAPTCHA')->notice(
+            '%form_id post blocked by CAPTCHA module: challenge %challenge (by module %module), user answered "@response", but the solution was "@solution".',
+            [
+              '%form_id' => $form_id,
+              '@response' => $captcha_response,
+              '@solution' => $solution,
+              '%challenge' => $captcha_info['captcha_type'],
+              '%module' => $captcha_info['module'],
+            ]);
+        }
+      }
+    }
+    else {
+      // If the session is gone and we can't confirm a solution error.
+      // Note: _captcha_get_posted_captcha_info() validates and triggers session
+      // rebuilds for re-use attacks during element processing so this should be
+      // rare if it ever happens.
+      $form_state->setErrorByName('captcha', t('CAPTCHA validation error: unknown CAPTCHA session ID. Contact the site administrator if this problem persists.'));
+      \Drupal::logger('CAPTCHA')->error(
+        'CAPTCHA validation error: unknown CAPTCHA session ID (%csid).',
+        ['%csid' => var_export($csid, TRUE)]);
+    }
+  }
 }
 
 /**

modules/captcha_test/captcha_test.info.yml

+name: 'Captcha Test'
+type: module
+description: 'Provides captcha types for tests.'
+package: Testing
+core_version_requirement: ^8.8 || ^9
+hidden: true

modules/captcha_test/captcha_test.module

+<?php
+
+/**
+ * @file
+ * Contains hook implementations for the Captcha Test module.
+ */
+
+/**
+ * Implements hook_captcha().
+ */
+function captcha_test_captcha($op, $captcha_type = '') {
+  switch ($op) {
+    case 'list':
+      return [];
+
+    case 'generate':
+      if ($captcha_type === 'TestCacheable') {
+        // A cacheable Captcha type.
+        $result = [
+          'cacheable' => TRUE,
+          // Cacheable captcha types need to provide a custom validation
+          // callback that doesn't care about the solution, because a form can
+          // be shown containing a cached CSID that has since been deleted
+          // from the {captcha_sessions} table.
+          'captcha_validate' => 'captcha_test_captcha_captcha_validation',
+          'solution' => 'Test 123',
+          'form' => [],
+        ];
+        $result['form']['captcha_response'] = [
+          '#type' => 'textfield',
+          '#title' => t('Test one two three'),
+          '#required' => TRUE,
+        ];
+
+        return $result;
+      }
+  }
+}
+
+/**
+ * Validation callback.
+ */
+function captcha_test_captcha_captcha_validation() {
+  return TRUE;
+}

tests/src/Functional/CaptchaCacheTest.php

@@ -2,6 +2,8 @@
 
 namespace Drupal\Tests\captcha\Functional;
 
+use Drupal\Core\Database\Database;
+
 /**
  * Tests CAPTCHA caching on various pages.
  *
@@ -14,12 +16,16 @@ class CaptchaCacheTest extends CaptchaWebTestBase {
    *
    * @var array
    */
-  public static $modules = ['block', 'image_captcha'];
+  protected static $modules = [
+    'block',
+    'image_captcha',
+    'captcha_test',
+  ];
 
   /**
    * {@inheritdoc}
    */
-  public function setUp() {
+  public function setUp(): void {
     parent::setUp();
 
     $this->drupalPlaceBlock('user_login_block', ['id' => 'login']);
@@ -69,4 +75,48 @@ class CaptchaCacheTest extends CaptchaWebTestBase {
     $this->assertResponse(200);
   }
 
+  /**
+   * Tests a cacheable captcha type.
+   */
+  public function testCacheableCaptcha() {
+    $web_assert = $this->assertSession();
+
+    // Enable captcha on login block with a cacheable captcha.
+    captcha_set_form_id_setting('user_login_form', 'captcha_test/TestCacheable');
+
+    // Warm up the caches.
+    $this->drupalGet('');
+
+    // Let's check if the page is cached.
+    $this->drupalGet('');
+    static::assertSame('HIT', $this->drupalGetHeader('X-Drupal-Cache'), 'Cache enabled');
+
+    $edit = [
+      'name' => $this->normalUser->getDisplayName(),
+      'pass' => $this->normalUser->pass_raw,
+      'captcha_response' => 'Test 123',
+    ];
+    $this->submitForm($edit, 'Log in');
+    $web_assert->addressEquals('user/' . $this->normalUser->id());
+
+    // Simulate a cron run that deletes the {captcha_session} data.
+    $connection = Database::getConnection();
+    $connection->delete('captcha_sessions')->execute();
+
+    // Log out and reload the form. Because the captcha is cacheable, the form
+    // is retrieved from the render cache, and contains the same CSID as
+    // previously.
+    $this->drupalLogout();
+    $this->drupalGet('');
+    static::assertSame('HIT', $this->drupalGetHeader('X-Drupal-Cache'), 'Cache enabled');
+
+    $edit = [
+      'name' => $this->normalUser->getDisplayName(),
+      'pass' => $this->normalUser->pass_raw,
+      'captcha_response' => 'Test 123',
+    ];
+    $this->submitForm($edit, 'Log in');
+    $web_assert->addressEquals('user/' . $this->normalUser->id());
+  }
+
 }

tests/src/Functional/CaptchaCronTest.php

@@ -17,7 +17,7 @@ class CaptchaCronTest extends BrowserTestBase {
    *
    * @var array
    */
-  public static $modules = ['captcha'];
+  protected static $modules = ['captcha'];
 
   /**
    * {@inheritdoc}

tests/src/Functional/CaptchaTest.php

@@ -14,7 +14,7 @@ class CaptchaTest extends CaptchaWebTestBase {
    *
    * @var array
    */
-  public static $modules = ['block', 'captcha_long_form_id_test'];
+  protected static $modules = ['block', 'captcha_long_form_id_test'];
 
   /**
    * Testing the protection of the user log in form.

tests/src/Functional/CaptchaWebTestBase.php

@@ -44,7 +44,7 @@ abstract class CaptchaWebTestBase extends BrowserTestBase {
    *
    * @var array
    */
-  public static $modules = ['captcha', 'comment'];
+  protected static $modules = ['captcha', 'comment'];
 
   /**
    * {@inheritdoc}