From 71a334194a0a4e0ff3165ea01a2e02c502abb62c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Sch=C3=B6ldstr=C3=B6m?= Date: Fri, 31 Jul 2026 08:26:11 -0300 Subject: [PATCH] fix: actually show the rejection message instead of GF's generic banner The message this plugin sets on rejection was never rendered. It was written to `$form['validation_summary_message']`, which Gravity Forms only reads as a *field* context property (form_display.php:5685), never off the form array. `$form['failed_validation_page']` was dead in the same way -- GF tracks the failed page through a by-reference parameter on validate(), not a form key. Both lines looked functional and did nothing. So what a visitor actually saw was GF's unconditional fallback: There was a problem with your submission. Please review the fields below. There are no fields to review -- an ALTCHA rejection highlights nothing. The instruction sends people hunting for a broken field that doesn't exist, and the most suspicious-looking fields take the blame. That is precisely how this was once misdiagnosed as a form-configuration problem on the Snellman consumer-service forms, costing a round of investigation into conditional logic that turned out to be correct. Hook `gform_validation_message` so the message reaches the DOM, and drop the two dead assignments. Only forms this request rejected are affected, so ordinary field errors keep GF's wording -- telling someone with a missing email address to reload the page would be worse than useless. Markup mirrors GFFormDisplay::get_validation_errors_markup(), deriving the `hide_summary` class from the form's own validationSummary setting rather than hardcoding it, so an admin's choice isn't overridden and existing theme styling still applies. Verified end-to-end: untouched before a rejection, replaced after one, and a second form on the same page left alone. Co-Authored-By: Claude Opus 5 (1M context) --- gravityforms-altcha.php | 2 +- src/Integration.php | 65 ++++++++++++-- src/Plugin.php | 2 +- test/bootstrap.php | 41 +++++++++ test/mocked/ValidationMessageTest.php | 124 ++++++++++++++++++++++++++ 5 files changed, 226 insertions(+), 8 deletions(-) create mode 100644 test/mocked/ValidationMessageTest.php diff --git a/gravityforms-altcha.php b/gravityforms-altcha.php index 8c21fed..1adc94f 100644 --- a/gravityforms-altcha.php +++ b/gravityforms-altcha.php @@ -6,7 +6,7 @@ * Plugin Name: ALTCHA for Gravity Forms * Plugin URI: https://github.com/generoi/gravityforms-altcha * Description: Invisible ALTCHA spam protection for Gravity Forms — uses the MIT-licensed altcha-org/altcha PHP library and the ALTCHA widget web component to proof-of-work-verify every form submission with no user interaction. - * Version: 0.6.0 + * Version: 0.6.1 * Requires at least: 6.0 * Requires PHP: 8.2 * Author: Genero diff --git a/src/Integration.php b/src/Integration.php index bc731a5..7c234e8 100644 --- a/src/Integration.php +++ b/src/Integration.php @@ -15,11 +15,23 @@ class Integration */ public const REASON_REPLAY = 'replay'; + /** + * Form ids this request rejected, so the validation banner is only replaced + * for our own rejections and never for ordinary field errors. + * + * Validation and the re-render of the form happen within the same request + * for both standard and AJAX submissions, so per-instance state suffices. + * + * @var array + */ + private array $rejected = []; + public static function register(): void { $instance = new self; add_filter('gform_submit_button', [$instance, 'injectWidget'], 10, 2); add_filter('gform_validation', [$instance, 'validate']); + add_filter('gform_validation_message', [$instance, 'validationMessage'], 10, 2); add_action('gform_enqueue_scripts', [$instance, 'enqueueScripts'], 10, 2); } @@ -52,10 +64,12 @@ public function injectWidget(string $buttonInput, array $form): string /** * Rejects the submission when the proof is missing, malformed, or doesn't - * verify. We attach the error message to the form's `validation_summary` - * so themes that style validation errors at the form level surface it; we - * deliberately don't attach it to a per-field error since there's no - * visible field to point at. + * verify. + * + * There's no visible field to hang the error on, so the message is surfaced + * by replacing the form-level validation banner — see + * {@see self::validationMessage()}. We only note the form id here; the + * banner is rendered later in the same request. * * @param array{is_valid: bool, form: array} $result * @return array{is_valid: bool, form: array} @@ -87,12 +101,51 @@ public function validate(array $result): array Logger::record('altcha', 'fail', $this->failureContext($formId, $reason, $payload, $challenge)); $result['is_valid'] = false; - $result['form']['validation_summary_message'] = $this->errorMessage(); - $result['form']['failed_validation_page'] = $result['form']['page_count'] ?? 1; + $this->rejected[(int) $formId] = true; return $result; } + /** + * Replaces Gravity Forms' generic validation banner when *we* rejected the + * submission. + * + * GF's default banner reads "There was a problem with your submission. + * Please review the fields below." Because an ALTCHA rejection highlights + * no field, that instruction sends people hunting for a broken field that + * doesn't exist — which is exactly how this was once misdiagnosed as a + * form-configuration bug. Our message names the actual recovery action: + * reload and try again. + * + * Only fires for forms we rejected, so genuine field errors keep GF's + * wording. + * + * The markup mirrors `GFFormDisplay::get_validation_errors_markup()`, + * including how it derives the `hide_summary` class from the form's own + * `validationSummary` setting — hardcoding that class would override an + * admin's choice, and GF appends its (here empty) error list based on the + * same setting. + * + * @param string $message the markup GF built + * @param array $form + */ + public function validationMessage(string $message, array $form): string + { + $formId = isset($form['id']) ? (int) $form['id'] : 0; + + if (! isset($this->rejected[$formId])) { + return $message; + } + + $hideSummary = empty($form['validationSummary']) ? ' hide_summary' : ''; + + return sprintf( + '

%s

', + $hideSummary, + esc_html($this->errorMessage()) + ); + } + /** * Builds the non-PII context recorded alongside a rejection. * diff --git a/src/Plugin.php b/src/Plugin.php index 2ecf637..8099fbe 100644 --- a/src/Plugin.php +++ b/src/Plugin.php @@ -4,7 +4,7 @@ class Plugin { - public const VERSION = '0.6.0'; + public const VERSION = '0.6.1'; public const SLUG = 'gravityforms-altcha'; diff --git a/test/bootstrap.php b/test/bootstrap.php index 4ea31e7..a4cd552 100644 --- a/test/bootstrap.php +++ b/test/bootstrap.php @@ -11,3 +11,44 @@ defined('MINUTE_IN_SECONDS') || define('MINUTE_IN_SECONDS', 60); defined('HOUR_IN_SECONDS') || define('HOUR_IN_SECONDS', 3600); defined('DAY_IN_SECONDS') || define('DAY_IN_SECONDS', 86400); + +/* + * Settings extends \GFAddOn, which only exists once Gravity Forms has loaded + * its add-on framework. Anything exercising Integration::shouldProtect() pulls + * Settings in, so give the class a shell to extend. + * + * Deliberately inert: it reports no settings, so a test needing a particular + * protection decision should force it through the documented + * `genero/gravityforms_altcha/should_protect` filter rather than leaning on + * whatever this stub happens to return. + */ +if (! class_exists('GFAddOn')) { + class GFAddOn + { + /** Declared by the real GFAddOn; Settings::__construct() assigns it. */ + protected $_full_path; + + public function __construct() {} + + public static function register(string $class): void {} + + /** @return mixed */ + public function get_plugin_setting(string $name) + { + return null; + } + + /** + * @param array $form + * @return array + */ + public function get_form_settings(array $form): array + { + return []; + } + + public function log_debug(string $message): void {} + + public function log_error(string $message): void {} + } +} diff --git a/test/mocked/ValidationMessageTest.php b/test/mocked/ValidationMessageTest.php new file mode 100644 index 0000000..e48f7f9 --- /dev/null +++ b/test/mocked/ValidationMessageTest.php @@ -0,0 +1,124 @@ +There was a problem with your submission. Please review the fields below."; + + protected function setUp(): void + { + parent::setUp(); + + // Pass filters through to their default, except protection: the + // GFAddOn stub reports no settings, so force it on explicitly. + Functions\when('apply_filters')->alias( + fn (string $hook, $value = null) => $hook === 'genero/gravityforms_altcha/should_protect' + ? true + : $value + ); + Functions\when('plugin_dir_path')->returnArg(1); + Functions\when('plugin_dir_url')->returnArg(1); + Functions\when('get_option')->justReturn('0123456789abcdef0123456789abcdef'); + Functions\when('esc_html')->returnArg(1); + Functions\when('__')->returnArg(1); + Functions\when('sanitize_text_field')->returnArg(1); + Functions\when('wp_unslash')->returnArg(1); + Functions\when('get_transient')->justReturn(false); + Functions\when('set_transient')->justReturn(true); + + Plugin::getInstance(dirname(__DIR__, 2).'/gravityforms-altcha.php'); + } + + /** + * A form we never rejected must keep Gravity Forms' own banner — otherwise + * a plain "email is required" error would tell the visitor to reload the + * page, which is worse than useless. + */ + public function test_leaves_the_banner_alone_for_forms_it_did_not_reject(): void + { + $integration = new Integration; + + $this->assertSame( + self::GF_DEFAULT, + $integration->validationMessage(self::GF_DEFAULT, ['id' => 45]), + ); + } + + public function test_replaces_the_banner_for_a_rejected_form(): void + { + $integration = $this->rejecting(45); + + $message = $integration->validationMessage(self::GF_DEFAULT, ['id' => 45]); + + $this->assertStringNotContainsString('Please review the fields below', $message); + $this->assertStringContainsString('could not be verified', $message); + } + + /** + * Keeps GF's wrapper classes so existing theme styling still applies to the + * replacement. + */ + public function test_replacement_keeps_gravity_forms_markup_hooks(): void + { + $message = $this->rejecting(45)->validationMessage(self::GF_DEFAULT, ['id' => 45]); + + $this->assertStringContainsString('gform_submission_error', $message); + $this->assertStringContainsString('gform-icon--circle-error', $message); + } + + /** + * Two forms on one page: rejecting one must not rewrite the other's banner. + */ + public function test_only_the_rejected_form_is_affected(): void + { + $integration = $this->rejecting(45); + + $this->assertSame( + self::GF_DEFAULT, + $integration->validationMessage(self::GF_DEFAULT, ['id' => 43]), + ); + $this->assertStringContainsString( + 'could not be verified', + $integration->validationMessage(self::GF_DEFAULT, ['id' => 45]), + ); + } + + /** + * Drives a real rejection through validate() so the test depends on the + * actual reject path rather than a hand-set flag. + */ + private function rejecting(int $formId): Integration + { + $integration = new Integration; + + // No `altcha` field posted at all -> REASON_MISSING -> rejected. + unset($_POST[Integration::POST_FIELD]); + + $result = $integration->validate([ + 'is_valid' => true, + 'form' => ['id' => $formId], + ]); + + $this->assertFalse($result['is_valid'], 'expected validate() to reject the submission'); + + return $integration; + } +}