refactor(captcha): put the math challenge behind a strategy contract - #1055
Merged
Conversation
The captcha is about to have two implementations that must coexist — a
proof-of-work challenge needs JavaScript and a secure context, and the math one
does not, so math stays as the no-JS path and as the rollback that needs no
redeploy. This introduces the seam ahead of that, with no behaviour change.
`CaptchaProviderInterface` has four methods: `id()`, `render_fields()`,
`verify()` and `challenge_payload()`. `MathCaptcha` implements it over the
existing generator and verifier — it wraps `SecurityService`, it does not
reimplement it. `CaptchaProvider::resolve()` reads the `captcha_provider`
setting and falls back to math on an unrecognised value: a typo in an option
must not take the public forms down.
Scope is the captcha only. The honeypot is provider-independent, so it stays in
`SecurityService`, which composes the two — which is why the six verification
sites are untouched: `validate_security_fields()` is still the single
chokepoint and now delegates its captcha half. The four retry sites that built
`{refresh_captcha, new_label, new_hash}` inline now call
`with_fresh_challenge()`, so they become provider-aware for free.
The security block existed twice — once via `Shortcodes`, once inline in the
self-scheduling booking form — and the copies had already drifted: the hidden
input carried an id in one and not the other. Both now render
`templates/security-fields.php`, which composes the honeypot with the
provider's own `templates/captcha/math-fields.php`. The id is what survives:
`ffc-calendar-frontend.js` refreshes the token through `#ffc_captcha_hash`.
`ffc-dynamic-fragments.js` gated its whole refresh on four selectors, one of
them `.ffc-captcha-row`. That row is math-specific and disappears under a
provider that renders no arithmetic, and the `[ffc_csv_download]` page has no
other marker — so nonce refresh would have stopped there silently and surfaced
later as a random "security check failed" on cached pages. It now also matches
`.ffc-security-container`, the wrapper every provider renders.
`DynamicFragments` still mints through `generate_simple_captcha()` on purpose:
its `label`/`hash` fragment shape is math-specific, and a proof-of-work widget
fetches its own challenge rather than reading it from a fragment. Whether that
endpoint keeps a captcha payload at all is the ALTCHA PR's call, so inventing a
mapping here would be churn.
Refs #1053
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012XWx9qJdjZdAq8crxM9GCU
CI's Vitest job failed on this PR with `ReferenceError: window is not defined` raised from `ffc-csv-download.js` `hideOverlay()`, while all 1731 tests passed. The completion path schedules two nested timers: an outer MIN_DISPLAY one that paints "Download complete!", and a nested 2000ms cleanup that hides the overlay and removes the iframe. This test drained only the outer one — a real 5ms wait — and left the cleanup pending. Whichever finished first, the test file or that timer, decided the outcome: when the file won, the callback ran against a torn-down jsdom and Vitest reported it as an unhandled error, failing the run. It reproduces in CI (75s) and not locally across three runs of the same `vitest run --coverage`, which is what a teardown race looks like: nothing about it is deterministic, and it was latent before this branch — the diff touches neither script in the stack, and the test does not reference the selector the branch changed. What the branch plausibly did was shift run timing enough to flip it. The fix is the pattern the cleanup-timer test right below already uses: fake timers, advance past MIN_DISPLAY to assert the completion status and the injected iframe, then advance through the cleanup so nothing outlives the test. No assertion was weakened or removed. Refs #1053 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012XWx9qJdjZdAq8crxM9GCU
This was referenced Sep 5, 2026
Closed
Coverage Report for CI Build 33940990510Warning No base build found for commit Coverage: 89.918%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
rpgmem
marked this pull request as ready for review
September 5, 2026 03:23
rpgmem
pushed a commit
that referenced
this pull request
Sep 5, 2026
…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
17 tasks
rpgmem
added a commit
that referenced
this pull request
Sep 5, 2026
…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>
14 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR2 do épico #1053. Zero mudança de comportamento — é a costura que faz o PR3 (ALTCHA) ser uma classe nova em vez de uma alteração em 10 sites.
Summary
CaptchaProviderInterfacecom quatro métodos:id(),render_fields(),verify()echallenge_payload().MathCaptchao implementa sobre o gerador e verificador existentes — envolve oSecurityService, não o reimplementa.CaptchaProvider::resolve()lê o settingcaptcha_providere cai em math num valor não reconhecido: um typo numa option não pode derrubar os formulários públicos.SecurityService, que compõe os dois — e é por isso que os 6 sites de verificação ficam intocados:validate_security_fields()segue sendo o ponto único e agora delega a metade captcha. Os 4 sites de retry que montavam{refresh_captcha, new_label, new_hash}à mão passam a chamarwith_fresh_challenge(), ficando provider-aware de graça.Shortcodese inline no formulário de agendamento — e as cópias já tinham divergido: o input oculto tinhaidnuma e não na outra. As duas agora renderizamtemplates/security-fields.php, que compõe o honeypot com otemplates/captcha/math-fields.phpdo provider.Type of change
Test plan
composer test— 7504 testes, 21643 asserções, verdevendor/bin/phpcs --standard=phpcs.xml.dist includes/ templates/— limpovendor/bin/phpcs --standard=phpcs-tests.xml.dist tests/— limponpx eslint assets/js— 0 errosnpx vitest run— 116 arquivos, 1731 testesnpm run build:js— sóffc-dynamic-fragments.min.jsmudou, build determinísticocomposer lint(PHPStan) — não executado:phpstan/phpstané dist-only e o proxy deste ambiente devolve 403 na API do GitHub. Fica para o CI.Novo
tests/Unit/CaptchaProviderTest.php(18 casos) cobre o resolver, a estratégia math e a composição noSecurityService.Checklist
CHANGELOG.mdatualizado sob[Unreleased] / ChangedDuas correções incluídas
Bug latente no
ffc-dynamic-fragments.js. Ele gateia todo o refresh em quatro seletores, um deles.ffc-captcha-row. Essa linha é específica do math e desaparece sob um provider que não renderiza aritmética — e a página do[ffc_csv_download]não tem nenhum outro marcador. O refresh de nonce pararia ali em silêncio, aparecendo depois como um "falha na verificação de segurança" aleatório em página cacheada. Passa a casar também com.ffc-security-container, o wrapper que todo provider renderiza. Entra aqui, antes de o modo que o dispararia existir.Divergência entre as duas cópias. O
idno input oculto é o que sobrevive à unificação, porque offc-calendar-frontend.js:518atualiza o token via#ffc_captcha_hash. O formulário de certificado ganha umidque não tinha — inócuo, e o teste correspondente foi ajustado com a razão inline.Notas para revisão
DynamicFragmentscontinua chamandogenerate_simple_captcha()de propósito. O formatolabel/hashdo fragmento é específico do math, e um widget de proof-of-work busca o próprio desafio em vez de lê-lo de um fragmento. Se aquele endpoint mantém payload de captcha é decisão do PR3 — inventar um mapeamento aqui seria churn que o PR3 apagaria.ModuleBoundaryTestnão precisa de regeneração.Core\Captcha\*continua no móduloCore, e a arestaCore>Settingsque oresolve()usa já está na baseline (linha 46).captcha_providerainda não está declarado emSettings::get_default_settings()— ele entra com a aba de configuração no PR4. Até lá o default do read-site é o único valor em jogo, e oSettingsDefaultsTestsó compara chaves presentes nos dois lados, então a ausência não é divergência.Piso de cobertura: medi e não mexi. A cobertura global está em 89,60% contra piso 86 — folga de 3,6pp, dentro da tolerância de ≤5pp do
CLAUDE.md. Subir para 87 não compra nada e arrisca flake na medição por shards do CI. O item fica fechado com a medição registrada, não silenciosamente pulado.Um defeito pré-existente que este PR não introduz nem corrige: numa página com dois formulários, ambos emitem
id="ffc_captcha_ans", duplicando ids e quebrando a associação<label for>. Já era assim antes (oidnoanssempre existiu nas duas cópias); a unificação apenas o torna mais visível. Vale uma issue à parte — corrigir exige sufixar ids por formulário e ajustar offc-calendar-frontend.js, o que sairia do escopo de "zero mudança de comportamento".Refs #1053
🤖 Generated with Claude Code
https://claude.ai/code/session_012XWx9qJdjZdAq8crxM9GCU
Generated by Claude Code