Reroll latest patch to latest version

Reroll latest patch to latest version of reCAPTCHA module

added 1 commit

• e08b4249 - PHPCS fixes

Compare with previous version

I think this needs to expose the option for the admin user to specify the bot likelihood score that's appropriate for their site. Google's documentation says the default is anything >= 0.5 is human.

$form['general']['recaptcha_is_human_score'] = [
      '#type' => 'textfield',
      '#title' => $this->t('ReCAPTCHA v3 Google Human Score Threshold'),
      '#description' => $this->t('ReCAPTCHA v3 returns a score (1.0 is very likely a good interaction, 0.0 is very likely a bot). By default, a threshold of 0.5 or more is human. See https://developers.google.com/recaptcha/docs/v3#interpreting_the_score'),
      '#maxlength' => 255,
      '#size' => 64,
      '#default_value' => $config->get('recaptcha_is_human_score'),
    ];

In addition to checking if the $api_response['tokenProperties']['valid'] == true, this needs to check the bot score to effectively block bots.

Something like this:

if ($api_response_valid && $this->isHumanScore($score)) {

    /**
     * reCAPTCHA v3 returns a score (1.0 is very likely a good interaction, 0.0 is very likely a bot). 
     * Based on the score, you can take variable action in the context of your site. 
     * reCAPTCHA learns by seeing real traffic on your site. 
     * For this reason, scores in a staging environment or soon after implementing may differ from production. 
     * By default, you can use a threshold of 0.5.
     * https://developers.google.com/recaptcha/docs/v3#interpreting_the_score
     */
    private function isHumanScore(float $score) : bool {
        return $score >= (float) $config->get('recaptcha_is_human_score') ?? 0.5;
    }

changed this line in version 4 of the diff

recaptcha_is_human_score: 0.5

recaptcha_is_human_score:
  type: float
  label: 'Bot likelihood score'

->set('recaptcha_is_human_score', $form_state->getValue('recaptcha_is_human_score'))

added 1 commit

• cf902c3d - Adapt JS API functions for reCAPTCHA enterprise

Compare with previous version

added 1 commit

• 313ac17f - Account for bot score

Compare with previous version

This hook is going to load the recaptcha/recaptcha library on every page, even pages where there is no v2 captcha widget being generated/rendered. So for performance reasons of loading an unnecessary script on all pages, I think this hook_page_attachments needs to be removed. The drupalSettings should be placed where the recaptcha/recaptcha library is already being attached, namely in the recaptcha_captcha function like this:

$captcha['form']['recaptcha_widget'] = [
  '#markup' => '<div' . new Attribute($attributes) . '></div>',
  '#suffix' => $noscript,
  '#attached' => [
    'library' => [
      'recaptcha/recaptcha',
      'recaptcha/google.recaptcha_' . \Drupal::service('language_manager')->getCurrentLanguage()->getId(),
     ],
     'drupalSettings' => [
       'recaptcha' => [
         'use_enterprise' => $config->get('use_enterprise'),
       ],
     ],
   ],
   '#cache' => [
     'tags' => ['library_info'],
     'contexts' => ['languages'],
   ],
];

I think you need to pass in drupalSettings here, otherwise it's undefined.

(function ($, Drupal, drupalSettings) {
  // Stuff
})(jQuery, Drupal, drupalSettings); // <— check this line

I'm glad you found these sections of JavaScript that definitely needed to use grecaptcha.enterprise! Honestly, after setting breakpoints and testing this code, I think this file needs some major TLC to bring it up to standard with how Drupal behaviors should work with external dependencies. Here are some issues/proposed fixes I see with it:

• The drupalRecaptchaOnload callback function does the same thing as the Drupal behavior. This is duplicate and unnecessary logic. With a little effort, this could be DRY-er with less repetitive, reusable logic split out into utility functions. I ended up choosing the Drupal behavior to own the single responsibility of rendering the g-recaptcha element because it's better integrated with the BigPipe load order of executing attached Drupal behaviors.
• I've removed the dependency on jQuery by using native DOM functions to check and add to the classList.
• I added a dependency on once so the same code doesn't execute multiple times every time a BigPipe component renders.
• Using the once library keeps this code idempotent (safe to call multiple times without side effect), which is important with Drupal behaviors because of how BigPipe rendering executes attached behaviors multiple times. It also avoids unnecessary DOM traversal by only selecting the elements you need within the proper context, rather than traversing through the entire body.
• There are some deeply nested ifs that make the current code not very readable. That can be fixed by returning early more often. This allows it to exit early if there’s nothing to do, prevents unnecessary logic from running, and reduces indentation and "pyramid of doom" nesting.
• There is a lot of repetitive code around those ifs checking the use_enterprise flag. This new code I'm proposing avoids duplicating almost identical logic for enterprise vs non-enterprise.
• There are a few spots where some simple, defensive undefined checks need to be added to make the runtime behavior safer, like drupalSettings.recaptcha?.use_enterprise ?? false
• With this library's Drupal behavior being dependent on the external google.recaptcha_en library loading and initializing first, there are a few places where race conditions can occur. There needs to be a thin layer of dependency load order management so things load and execute in the correct order. The drupalRecaptchaOnload callback function is perfect for this, which can reliably give the "green light" to the Drupal behavior to render the widget after the grecaptcha library is ready and all the BigPipe components have loaded. There's also a built-in function called grecaptcha.enterprise.ready, which serves the same-ish purpose, but isn't necessary since this new code I'm proposing relies on the callback function to say that grecaptcha is ready. Without handling this race condition properly, you run the risk of trying to use grecaptcha before it's ready and trust me... that is not a fun time...
• 
• The new code uses a waitForRecaptchaReady Promise that resolves when the drupalSettings.recaptcha?._grecaptchaReady flag has been set to true by the drupalRecaptchaOnload callback, which is called from the explicit render in recaptcha_library_info_build.
• I tested this new code along with the rest of your recent changes to this patch and confirmed it's requiring the bot score to be above 0.5 to consider it a successful submission, which is awesome!
• 

Proposed new js/recaptcha.js:

/**
 * @file
 * Contains the definition of the behavior recaptcha.
 */
(function (Drupal, drupalSettings, once) {
  const useEnterprise = drupalSettings.recaptcha?.use_enterprise ?? false;

  // Utility function to wait for grecaptcha to be ready
  function waitForRecaptchaReady(checkFn, interval = 50, maxWait = 10000) {
    return new Promise((resolve, reject) => {
      const start = Date.now();

      const check = () => {
        if (checkFn()) {
          resolve();
        } else if (Date.now() - start >= maxWait) {
          reject(new Error('grecaptcha did not become ready in time'));
        } else {
          setTimeout(check, interval);
        }
      };

      check();
    });
  }

  Drupal.behaviors.recaptcha = {
    attach(context) {
      waitForRecaptchaReady(() => {
        return drupalSettings.recaptcha?._grecaptchaReady === true;
      })
      .then(() => {
        once('recaptchaBehavior', '.g-recaptcha', context).forEach((el) => {
          const grecaptchaObj = useEnterprise ? grecaptcha.enterprise : grecaptcha;

          if (!el.classList.contains('recaptcha-processed')) {
            grecaptchaObj.render(el, el.dataset);
            el.classList.add('recaptcha-processed');
          }
        });
      })
      .catch((error) => {
        console.error(error);
      });
    },
  };

  // Only set the flag when grecaptcha is ready. Don't render. The Drupal behavior handles rendering.
  window.drupalRecaptchaOnload = function () {
    drupalSettings.recaptcha._grecaptchaReady = true;
  };
})(Drupal, drupalSettings, once);

Also just want to point out that the new code I'm suggesting above doesn't use grecaptcha.reset(); like the original code did. It appears that was trying to prevent duplicate rendering of the same reCAPTCHA widget. This problem would have been caused by the code not being idempotent for a few reasons:

• BigPipe loading content chunks asynchronously and then calling Drupal.attachBehaviors() again and again on .recaptcha-processed elements.
• A manual call to grecaptcha.render() via the Google callback drupalRecaptchaOnload conflicting with the timing of BigPipe having already rendered it.

When that was happening before, you'd see this error:

Uncaught (in promise) Error: reCAPTCHA has already been rendered in this element

The original code tried to sidestep the "already rendered" error by calling reset() first, which clears out the widget so it can be re-rendered — a bit of a brute-force fallback.

However, that still doesn't make the behavior idempotent, and it's not a great fix because:

• reset() reverts the current widget back to its unverified state, possibly confusing users or breaking form validation state.
• It's wasteful — resetting and rendering again is unnecessary if the widget is already there and working.

That was needed before because:

• It didn't correctly guard against multiple behavior runs using once().
• It didn't properly coordinate behavior execution timing with the drupalRecaptchaOnload callback.
• So it had to say: "if it's already here, reset and re-render it just in case."

With the new version I'm suggesting:

• It uses once() to ensure there's only one render per element per JS execution per page load.
• It defers Drupal behavior execution until grecaptcha is ready, via the _grecaptchaReady flag.
• So it only calls render() if it's not already rendered.

This way it no longer needs to call reset() and the behavior is now idempotent, and that's the ideal scenario.

To go along with the big comment I made about the refactored version of recaptcha.js, I removed the dependency on jQuery and added the dependency on once like this:

recaptcha:
  js:
    js/recaptcha.js: {}
  dependencies:
    - core/drupal
    - core/drupalSettings
    - core/once

mentioned in merge request !41

closed