fix(captcha): scope the booking refresh to its form and make the ids unique - #1057
Merged
Conversation
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
…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
rpgmem
marked this pull request as ready for review
September 5, 2026 03:57
rpgmem
enabled auto-merge (squash)
September 5, 2026 03:57
Closed
5 tasks
Coverage Report for CI Build 33942993551Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Warning No base build found for commit Coverage: 89.934%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
84 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.
Fecha #1056. Intercalado entre o PR2 e o PR3 do épico #1053, porque o PR3 mexe no mesmo markup e no mesmo caminho de refresh.
Summary
ffcCalendarFrontend.refreshCaptcha()era global: reescrevia a pergunta em toda.ffc-captcha-rowda página e depois gravava o token novo via$('#ffc_captcha_hash'), que por definição casa só com o primeiro elemento. Com dois formulários montados, uma rejeição em qualquer um deixava o segundo exibindo uma pergunta que seu token não respondia — o visitante respondia o que estava na tela e ouvia que a resposta matemática estava incorreta. Verdade, e inútil como diagnóstico.name, que é o que offc-frontend-helpers.jsjá fazia no caminho do certificado. O$formjá estava em escopo no ponto de chamada, então nada precisou ser reestruturado.ffc-dynamic-fragments,ffc-frontend,ffc-frontend-helpers— casa porname, e nenhum CSS os referencia. Com o calendário convertido, os ids passam a existir só para o par<label for>. Ainda assim, duplicados quebravam a associação que um leitor de tela usa para anunciar um campo obrigatório, entãoMathCaptchaos sufixa por render. Osnamesão o contrato com o servidor e ficam intocados.O cenário não é hipotético:
DynamicFragmentstem um ramo dedicado a múltiplos formulários, emitindo um desafio distinto para cada um.Type of change
Test plan
composer test— 7507 testes, 21650 asserções, verdevendor/bin/phpcs --standard=phpcs.xml.dist includes/ templates/— limpovendor/bin/phpcs --standard=phpcs-tests.xml.dist tests/— limponpm run lint:js— 0 errosnpm run test:js:coverage— 116 arquivos, 1731 testesnpm run build:js— sóffc-calendar-frontend.min.jsmudoucomposer lint(PHPStan) — não executado (o pacote é dist-only e o proxy deste ambiente dá 403 na API do GitHub). Fica para o CI.Os testes novos foram verificados contra o código antigo. Revertendo o corpo do
refreshCaptchapara a versão global, 3 dos 4 falham; com a correção, passam. Eles fixam o defeito, não descrevem a correção. No lado PHP, três casos cobrem ids distintos entre renders, oforapontando para o input do mesmo render, e a estabilidade dosname.Checklist
CHANGELOG.mdatualizado sob[Unreleased] / FixedO commit extra
193955aé um ajuste de@coversque não alcançou a janela do merge do #1055 — meu push saiu segundos depois de você mergear, e a branch já tinha sido deletada. Recuperei por cherry-pick sobre o novodevelop. Ele adicionaSecurityServiceao@coversdoCaptchaProviderTest: o Coveralls reportavarender_security_fields()como descoberto porque@coversfiltra a atribuição, não porque faltasse teste. Vai de 67/73 para 72/73; a linha que sobra é oexitdo guardABSPATH.Sobre a varredura de timers
A issue lista um segundo item: varrer os timers longos por outras instâncias da corrida de teardown corrigida no #1055. Fiz a medição e não fiz a mudança, deliberadamente.
O inventário estático são 16 timers de 1000ms ou mais, e vários vivem em arquivos carregados por testes que não usam fake timers:
ffc-pdf-generator.js:353frontend-offline-and-overlay-hint,pdf-overlay-a11yffc-pdf-generator.js:607ffc-admin.js:118ffc-csv-download.js:88csv-and-rereg-frontend,csv-download-open-earlyffc-reregistration-frontend.js:382csv-and-rereg-frontendffc-frontend-helpers.js:646,651Mas "carrega o arquivo sem fake timers" não é o mesmo que "vaza um timer" — o teste precisa alcançar o caminho que o agenda. E três rodadas da suíte JS completa produziram zero erros não tratados. Nada além da instância já corrigida está vazando de forma observável.
Adicionar fake timers a cinco arquivos que passam, com base numa heurística estática, seria churn com risco de quebrar testes que funcionam. O inventário fica registrado na issue para quem tiver uma reprodução; o checkbox permanece aberto com a medição anexada, não fechado em falso.
Refs #1056, #1053
🤖 Generated with Claude Code
https://claude.ai/code/session_012XWx9qJdjZdAq8crxM9GCU
Generated by Claude Code