Skip to content

Commit 9f62702

Browse files
rpgmemclaude
andauthored
fix(captcha): scope the booking refresh to its form and make the ids unique (#1057)
* test: attribute the security-fields renderer to the class it exercises Coveralls reported `SecurityService` at 25% patch coverage — 2 of the 8 changed lines — with the whole body of `render_security_fields()` uncovered, even though `test_render_security_fields_composes_honeypot_and_challenge` calls it and asserts on its output. The method was never untested; the report was filtered. `@covers` restricts attribution to the classes it names, and this test class named only the two captcha ones, so everything the composition tests executed inside `SecurityService` was discarded. Adding it to `@covers` — with the `class_exists()` preload CLAUDE.md prescribes for the pcov gotcha — takes the file from 67/73 to 72/73; the one line left is the `exit` in the ABSPATH guard, unreachable by construction. No assertion changed. This makes the coverage report describe what the suite actually does, so a later reader does not "add a test" for a covered method or read it as dead. Refs #1053 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012XWx9qJdjZdAq8crxM9GCU * fix(captcha): scope the booking refresh to its form and make the ids unique The plugin supports several forms on one page on purpose — DynamicFragments has a branch that mints a distinct challenge per form — but the captcha did not hold up under it. `ffcCalendarFrontend.refreshCaptcha()` was page-global. It rewrote the question in *every* `.ffc-captcha-row` on the page, then wrote the new token through `$('#ffc_captcha_hash')`, which by definition matches only the first element. With two forms up, a rejection in either one left the second displaying a question its token did not answer: the visitor answered what was on screen and was told the math answer is incorrect — true, and useless as a diagnosis. It now scopes to the submitted form and matches by `name`, which is what `ffc-frontend-helpers.js` already did on the certificate path. `$form` was already in scope at the call site, so nothing had to be restructured. That leaves the ids used only for the `<label for>` pair, and a census confirmed it: every other consumer — ffc-dynamic-fragments, ffc-frontend, ffc-frontend-helpers — matches by `name`, and no CSS references them. Duplicate ids still broke the label association a screen reader needs to announce a required field, so `MathCaptcha` now suffixes them per render. The `name` attributes are the contract with the server and are untouched. Three of the four new JS tests fail against the previous implementation and pass against this one, so they pin the defect rather than describing the fix. Also carries a test-attribution fix that missed the #1055 merge window: that class `@covers` SecurityService, taking the file from 67/73 to 72/73 — the method was always tested, the report was filtered. On the timer sweep this issue also lists: the inventory is 16 timers of 1000ms or more, several loaded by tests that do not fake timers. But three runs of the full JS suite produced zero unhandled errors, so nothing beyond the instance already fixed is observably leaking. Adding fake timers to five passing test files on a static heuristic would be churn; the inventory is recorded on the issue for whoever has a reproduction. Refs #1056, #1053 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012XWx9qJdjZdAq8crxM9GCU --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent c1f9085 commit 9f62702

8 files changed

Lines changed: 199 additions & 22 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,10 @@ The format follows [Keep a Changelog] (https://keepachangelog.com/en/1.1.0/).
1515

1616
- Internal (#1053) — o captcha passa a ter um contrato de estratégia (`CaptchaProviderInterface` + `CaptchaProvider::resolve()`), com o desafio matemático atrás dele. Os 6 sites de verificação e os 4 de retry não mudam: `validate_security_fields()` continua sendo o ponto único e agora delega a metade captcha. As duas cópias do bloco de segurança viraram uma, em `templates/`.
1717

18+
### Fixed
19+
20+
- **O captcha se atrapalhava com dois formulários na mesma página** (#1056): o refresh do agendamento reescrevia a pergunta em *todos* os formulários mas, casando por id, trocava o token só do primeiro — o segundo passava a exibir uma pergunta que seu token não respondia. Agora é escopado ao formulário e casa por `name`. Os ids do captcha passam a ser únicos por render, corrigindo também a associação `<label for>` para leitores de tela.
21+
1822
## [6.22.0] (2026-09-04) — `aa5ba5b`
1923

2024
### Deprecated

‎assets/js/ffc-calendar-frontend.js‎

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -371,7 +371,7 @@
371371
$submitBtn.data('submitting', false);
372372
if (err && err.fromServer) {
373373
if (err.data && err.data.refresh_captcha) {
374-
self.refreshCaptcha(err.data.new_label, err.data.new_hash);
374+
self.refreshCaptcha($form, err.data.new_label, err.data.new_hash);
375375
}
376376
self.showError(err.message || ffcCalendar.strings.error);
377377
} else {
@@ -507,16 +507,29 @@
507507
},
508508

509509
/**
510-
* Refresh captcha with new question
510+
* Refresh captcha with new question, scoped to the submitted form.
511+
*
512+
* Was page-global (#1056): it rewrote the question in *every* captcha
513+
* row on the page but, matching by id, updated only the *first* hidden
514+
* token. With two forms on one page — a configuration the plugin
515+
* supports on purpose — the second ended up showing the new question
516+
* while holding the old token, so answering what was on screen failed
517+
* with "the math answer is incorrect", which is true and useless.
518+
*
519+
* Now scoped to $form and matched by name, like ffc-frontend-helpers.
520+
*
521+
* @param {Object} $form jQuery object for the form being refreshed.
522+
* @param {string} newLabel New challenge question.
523+
* @param {string} newHash New challenge token.
511524
*/
512-
refreshCaptcha: function(newLabel, newHash) {
513-
if (!newLabel || !newHash) {
525+
refreshCaptcha: function($form, newLabel, newHash) {
526+
if (!$form || !$form.length || !newLabel || !newHash) {
514527
return;
515528
}
516529

517-
$('.ffc-captcha-row .ffc-captcha-label-text').text(newLabel);
518-
$('#ffc_captcha_hash').val(newHash);
519-
$('#ffc_captcha_ans').val('').focus();
530+
$form.find('.ffc-captcha-row .ffc-captcha-label-text').text(newLabel);
531+
$form.find('input[name="ffc_captcha_hash"]').val(newHash);
532+
$form.find('input[name="ffc_captcha_ans"]').val('').focus();
520533
},
521534

522535
/**

‎assets/js/ffc-calendar-frontend.min.js‎

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎includes/core/captcha/class-ffc-math-captcha.php‎

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,18 @@ class MathCaptcha implements CaptchaProviderInterface {
3636
*/
3737
public const ID = 'math';
3838

39+
/**
40+
* Renders served so far in this request.
41+
*
42+
* Ids have to be unique per page and the renderer has no form context, so
43+
* the instance number comes from here. It is only ever used to build the
44+
* `<label for>` pair — no script looks these ids up, they all match by
45+
* `name` — so its value carries no meaning beyond being distinct.
46+
*
47+
* @var int
48+
*/
49+
private static int $instances = 0;
50+
3951
/**
4052
* {@inheritDoc}
4153
*
@@ -53,8 +65,12 @@ public function id(): string {
5365
public function render_fields(): string {
5466
$challenge = SecurityService::generate_simple_captcha();
5567

56-
$ffc_captcha_label = (string) $challenge['label'];
57-
$ffc_captcha_token = (string) $challenge['hash'];
68+
++self::$instances;
69+
70+
$ffc_captcha_label = (string) $challenge['label'];
71+
$ffc_captcha_token = (string) $challenge['hash'];
72+
$ffc_captcha_ans_id = 'ffc_captcha_ans_' . self::$instances;
73+
$ffc_captcha_hash_id = 'ffc_captcha_hash_' . self::$instances;
5874

5975
ob_start();
6076
include FFC_PLUGIN_DIR . 'templates/captcha/math-fields.php';
@@ -63,6 +79,18 @@ public function render_fields(): string {
6379
return false === $html ? '' : $html;
6480
}
6581

82+
/**
83+
* Reset the per-request instance counter.
84+
*
85+
* Test seam: the counter is static, so ids would keep climbing across
86+
* test methods and assertions on a specific id would depend on run order.
87+
*
88+
* @return void
89+
*/
90+
public static function reset_instances(): void {
91+
self::$instances = 0;
92+
}
93+
6694
/**
6795
* {@inheritDoc}
6896
*

‎templates/captcha/math-fields.php‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,21 +4,30 @@
44
*
55
* Extracted from the two inline copies that had drifted apart (#1053 PR2).
66
*
7+
* The ids carry a per-render suffix (#1056): the plugin supports several forms
8+
* on one page — `DynamicFragments` has a branch dedicated to it — and fixed ids
9+
* duplicated across them, which breaks the `<label for>` association a screen
10+
* reader relies on to announce a required field. Nothing reads these ids
11+
* programmatically: every script matches the inputs by `name`, scoped to the
12+
* form. The `name` attributes are the contract with the server and never change.
13+
*
714
* @package FreeFormCertificate
815
* @since 6.23.0
916
*
10-
* @var string $ffc_captcha_label Challenge question, already translated.
11-
* @var string $ffc_captcha_token Signed, expiring challenge token.
17+
* @var string $ffc_captcha_label Challenge question, already translated.
18+
* @var string $ffc_captcha_token Signed, expiring challenge token.
19+
* @var string $ffc_captcha_ans_id Unique id for the answer input.
20+
* @var string $ffc_captcha_hash_id Unique id for the hidden token input.
1221
*/
1322

1423
if ( ! defined( 'ABSPATH' ) ) {
1524
exit;
1625
}
1726
?>
1827
<div class="ffc-captcha-row">
19-
<label for="ffc_captcha_ans">
28+
<label for="<?php echo esc_attr( $ffc_captcha_ans_id ); ?>">
2029
<span class="ffc-captcha-label-text"><?php echo esc_html( $ffc_captcha_label ); ?></span> <span class="required">*</span>
2130
</label>
22-
<input type="number" name="ffc_captcha_ans" id="ffc_captcha_ans" class="ffc-input" required aria-required="true">
23-
<input type="hidden" name="ffc_captcha_hash" id="ffc_captcha_hash" value="<?php echo esc_attr( $ffc_captcha_token ); ?>">
31+
<input type="number" name="ffc_captcha_ans" id="<?php echo esc_attr( $ffc_captcha_ans_id ); ?>" class="ffc-input" required aria-required="true">
32+
<input type="hidden" name="ffc_captcha_hash" id="<?php echo esc_attr( $ffc_captcha_hash_id ); ?>" value="<?php echo esc_attr( $ffc_captcha_token ); ?>">
2433
</div>

‎tests/Unit/CaptchaProviderTest.php‎

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
*
1818
* @covers \FreeFormCertificate\Core\Captcha\CaptchaProvider
1919
* @covers \FreeFormCertificate\Core\Captcha\MathCaptcha
20+
* @covers \FreeFormCertificate\Core\SecurityService
2021
*/
2122
class CaptchaProviderTest extends TestCase {
2223

@@ -44,8 +45,10 @@ protected function setUp(): void {
4445
// a test method.
4546
class_exists( '\FreeFormCertificate\Core\Captcha\CaptchaProvider' );
4647
class_exists( '\FreeFormCertificate\Core\Captcha\MathCaptcha' );
48+
class_exists( '\FreeFormCertificate\Core\SecurityService' );
4749

4850
CaptchaProvider::reset();
51+
MathCaptcha::reset_instances();
4952

5053
$this->settings = array();
5154
$this->transients = array();
@@ -85,6 +88,7 @@ function ( string $key, $value ): bool {
8588

8689
protected function tearDown(): void {
8790
CaptchaProvider::reset();
91+
MathCaptcha::reset_instances();
8892
Monkey\tearDown();
8993
parent::tearDown();
9094
}
@@ -143,11 +147,46 @@ public function test_math_render_fields_emits_the_challenge_inputs(): void {
143147
$html = ( new MathCaptcha() )->render_fields();
144148

145149
$this->assertStringContainsString( 'ffc-captcha-row', $html );
146-
$this->assertStringContainsString( 'ffc_captcha_ans', $html );
147-
$this->assertStringContainsString( 'ffc_captcha_hash', $html );
150+
$this->assertStringContainsString( 'name="ffc_captcha_ans"', $html );
151+
$this->assertStringContainsString( 'name="ffc_captcha_hash"', $html );
148152
$this->assertMatchesRegularExpression( '/value="\d+\.[0-9a-f]{16}\.[0-9a-f]{64}"/', $html );
149153
}
150154

155+
public function test_math_render_fields_gives_each_instance_distinct_ids(): void {
156+
// The plugin supports several forms on one page (DynamicFragments has a
157+
// branch for it). Fixed ids duplicated across them and broke the
158+
// `<label for>` association a screen reader needs (#1056).
159+
$provider = new MathCaptcha();
160+
161+
$first = $provider->render_fields();
162+
$second = $provider->render_fields();
163+
164+
preg_match_all( '/id="(ffc_captcha_(?:ans|hash)_\d+)"/', $first . $second, $m );
165+
166+
$this->assertCount( 4, $m[1], 'expected two ids per render' );
167+
$this->assertSame( $m[1], array_unique( $m[1] ), 'ids must not repeat across renders' );
168+
}
169+
170+
public function test_math_render_fields_points_the_label_at_its_own_input(): void {
171+
$provider = new MathCaptcha();
172+
$provider->render_fields();
173+
$html = $provider->render_fields();
174+
175+
preg_match( '/<label for="([^"]+)"/', $html, $label );
176+
preg_match( '/name="ffc_captcha_ans" id="([^"]+)"/', $html, $input );
177+
178+
$this->assertNotEmpty( $label[1] );
179+
$this->assertSame( $label[1], $input[1], 'label must reference the input in the same render' );
180+
}
181+
182+
public function test_math_render_fields_keeps_the_name_attributes_stable(): void {
183+
// The names are the contract with the server; only the ids vary.
184+
$html = ( new MathCaptcha() )->render_fields();
185+
186+
$this->assertStringContainsString( 'name="ffc_captcha_ans"', $html );
187+
$this->assertStringContainsString( 'name="ffc_captcha_hash"', $html );
188+
}
189+
151190
public function test_math_render_fields_omits_the_honeypot(): void {
152191
// The honeypot is provider-independent and belongs to the caller.
153192
$this->assertStringNotContainsString( 'ffc_honeypot_trap', ( new MathCaptcha() )->render_fields() );

‎tests/Unit/FrontendShortcodesTest.php‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -181,12 +181,12 @@ public function test_generate_security_fields_contains_honeypot_and_captcha(): v
181181
$this->assertStringContainsString( 'ffc_captcha_ans', $html );
182182
$this->assertStringContainsString( 'ffc_captcha_hash', $html );
183183

184-
// The hidden field carries the signed, expiring token (6.23.0). It also
185-
// carries an id since PR2 unified the two drifted copies of this block
186-
// on the self-scheduling variant — `ffc-calendar-frontend.js` refreshes
187-
// the token through `#ffc_captcha_hash`, so the id has to stay.
184+
// The hidden field carries the signed, expiring token (6.23.0). Its id
185+
// is per-render since #1056 — several forms can share a page, and fixed
186+
// ids duplicated across them — so match the suffix, not a literal. No
187+
// script looks the id up; they all match by `name`.
188188
$this->assertMatchesRegularExpression(
189-
'/name="ffc_captcha_hash" id="ffc_captcha_hash" value="\d+\.[0-9a-f]{16}\.[0-9a-f]{64}"/',
189+
'/name="ffc_captcha_hash" id="ffc_captcha_hash_\d+" value="\d+\.[0-9a-f]{16}\.[0-9a-f]{64}"/',
190190
$html
191191
);
192192

‎tests/js/calendar-frontend.test.js‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -406,3 +406,87 @@ describe('ffc-calendar-frontend.js — submitBooking nonce plumbing', () => {
406406
expect(payload).toContain('date=2026-05-20');
407407
});
408408
});
409+
410+
describe('ffc-calendar-frontend.js — refreshCaptcha scoping (#1056)', () => {
411+
beforeEach(() => {
412+
reset();
413+
installGlobals();
414+
loadScript('assets/js/ffc-calendar-frontend.js');
415+
});
416+
417+
function mountTwoForms() {
418+
// Two booking forms on one page — a configuration the plugin supports
419+
// on purpose: DynamicFragments has a branch that mints a distinct
420+
// challenge per form. Ids are per-instance since #1056, so the markup
421+
// mirrors that: same `name`, different `id`.
422+
document.body.innerHTML = `
423+
<form class="ffc-booking-form" id="form-a">
424+
<div class="ffc-captcha-row">
425+
<label for="ffc_captcha_ans_1"><span class="ffc-captcha-label-text">1 + 1</span></label>
426+
<input type="number" name="ffc_captcha_ans" id="ffc_captcha_ans_1">
427+
<input type="hidden" name="ffc_captcha_hash" id="ffc_captcha_hash_1" value="token-a">
428+
</div>
429+
</form>
430+
<form class="ffc-booking-form" id="form-b">
431+
<div class="ffc-captcha-row">
432+
<label for="ffc_captcha_ans_2"><span class="ffc-captcha-label-text">2 + 2</span></label>
433+
<input type="number" name="ffc_captcha_ans" id="ffc_captcha_ans_2">
434+
<input type="hidden" name="ffc_captcha_hash" id="ffc_captcha_hash_2" value="token-b">
435+
</div>
436+
</form>`;
437+
return {
438+
$a: window.$('#form-a'),
439+
$b: window.$('#form-b'),
440+
};
441+
}
442+
443+
it('updates only the form it was given', () => {
444+
const { $a, $b } = mountTwoForms();
445+
446+
window.ffcCalendarFrontend.refreshCaptcha($a, '9 + 9', 'token-a2');
447+
448+
expect($a.find('.ffc-captcha-label-text').text()).toBe('9 + 9');
449+
expect($a.find('input[name="ffc_captcha_hash"]').val()).toBe('token-a2');
450+
451+
// The regression: the label used to be rewritten page-wide while only
452+
// the first token was replaced, so this form would display a question
453+
// its token did not answer.
454+
expect($b.find('.ffc-captcha-label-text').text()).toBe('2 + 2');
455+
expect($b.find('input[name="ffc_captcha_hash"]').val()).toBe('token-b');
456+
});
457+
458+
it('keeps label and token coherent in the untouched form', () => {
459+
const { $a, $b } = mountTwoForms();
460+
461+
window.ffcCalendarFrontend.refreshCaptcha($b, '7 + 7', 'token-b2');
462+
463+
// Each form's displayed question must belong to the token it carries.
464+
expect($a.find('.ffc-captcha-label-text').text()).toBe('1 + 1');
465+
expect($a.find('input[name="ffc_captcha_hash"]').val()).toBe('token-a');
466+
expect($b.find('.ffc-captcha-label-text').text()).toBe('7 + 7');
467+
expect($b.find('input[name="ffc_captcha_hash"]').val()).toBe('token-b2');
468+
});
469+
470+
it('clears the answer field of the refreshed form only', () => {
471+
const { $a, $b } = mountTwoForms();
472+
$a.find('input[name="ffc_captcha_ans"]').val('11');
473+
$b.find('input[name="ffc_captcha_ans"]').val('22');
474+
475+
window.ffcCalendarFrontend.refreshCaptcha($a, '3 + 3', 'token-a3');
476+
477+
expect($a.find('input[name="ffc_captcha_ans"]').val()).toBe('');
478+
expect($b.find('input[name="ffc_captcha_ans"]').val()).toBe('22');
479+
});
480+
481+
it('is a no-op without a form or without challenge data', () => {
482+
const { $a } = mountTwoForms();
483+
484+
window.ffcCalendarFrontend.refreshCaptcha(null, '5 + 5', 'x');
485+
window.ffcCalendarFrontend.refreshCaptcha($a, '', 'x');
486+
window.ffcCalendarFrontend.refreshCaptcha($a, '5 + 5', '');
487+
window.ffcCalendarFrontend.refreshCaptcha(window.$('#nope'), '5 + 5', 'x');
488+
489+
expect($a.find('.ffc-captcha-label-text').text()).toBe('1 + 1');
490+
expect($a.find('input[name="ffc_captcha_hash"]').val()).toBe('token-a');
491+
});
492+
});

0 commit comments

Comments
 (0)