refactor(captcha): serve the cached-page refresh from the provider (#1053) - #1064
Merged
Merged
Conversation
…1053) `DynamicFragments::handle()` called `SecurityService::generate_simple_captcha()` directly, in two places. It is the fifth refresh site: PR2 routed the four retry sites through `with_fresh_challenge()` and missed this one, leaving `challenge_payload()`'s own docblock claiming a consumer it did not have. The endpoint now forwards `CaptchaProvider::resolve()->challenge_payload()` verbatim and names none of the fields. Mapping them would only ever hold for the math challenge — a proof-of-work challenge has no question and no answer hash — so the client dispatches on the payload's `provider` key instead, and leaves a provider it does not recognise alone rather than half-applying it, which would blank a challenge the visitor may already have solved. Scoping moved with it. The client used to query the whole document for labels, tokens and answers separately; it now walks one security block at a time (`.ffc-security-container`, plus any bare `.ffc-captcha-row` from markup cached before that wrapper existed — cached pages being this endpoint's whole audience). That is what keeps two challenges on one page independent, and it matches where the fields actually live: `templates/captcha/math-fields.php` puts the label and both inputs inside the row. Also removes `Shortcodes::get_new_captcha_data()`, a public method whose only consumer was its own test. Test notes. The JS fixtures placed the inputs outside the row, which no render site does; they now mirror the template. Their seed answers were non-numeric, and the real input is `type="number"` — which reads back as '' — so every "blanks the answer" assertion was passing without proving anything. All six captcha tests were verified failing against the previous script. On the PHP side the endpoint is now checked against a mocked `CaptchaProvider` rather than a mocked `generate_simple_captcha()`: the latter passes just as well with the endpoint bypassing the contract, which is the bug being fixed. 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 16:30
rpgmem
enabled auto-merge (squash)
September 5, 2026 16:30
84 tasks
Coverage Report for CI Build 33977777323Warning No base build found for commit Coverage: 89.95%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
15 tasks
rpgmem
added a commit
that referenced
this pull request
Sep 5, 2026
#1053) (#1067) Smoke-testing the ALTCHA-only mode found the widget's container rendering empty on the public CSV download. The bundle was never loaded there: the enqueue sat inside `if ( $has_form || $has_verification )` in `Frontend::enqueue_scripts()`, and that page is neither. The self-scheduling booking form would have been the same — a different class enqueues it — so two of the four surfaces that render the security block were broken. A custom element that never upgrades reports nothing, so the symptom was an empty box with a clean console. This is the trap CLAUDE.md already documents for the batched-export click handler (#783), in enqueue form. Adding `$has_csv_download` to the list would fix the screenshot and leave the defect: the list is what drifts. The provider that renders the widget now enqueues it. `Loader::register_frontend_assets()` registers both scripts — the place whose own docblock says "only registers; actual enqueue happens when shortcodes load their dependencies" — and `AltchaCaptcha::render_fields()` enqueues at render time. Shortcodes render during `the_content`, well before `wp_footer`, and both are footer scripts, so a late enqueue lands. The localisation moved with them, so the `language` attribute and the i18n store key are now produced by one method instead of two that could disagree — a disagreement the widget answers by falling back to English in silence. Both new guards were verified failing against the broken code: the render test reports `Failed asserting that an array contains 'ffc-captcha'`, the registration test the same for 'ffc-altcha'. Test-isolation fallout, and a note for the next person. Stubbing `get_locale` in `LoaderTest` broke 14 tests in two unrelated classes — the same `LabelSorter::locale()` `function_exists` guard that bit in #1064, whose branch depends on whether any earlier test in the process defined that function. Defining it in `tests/bootstrap.php` is not the fix: Patchwork cannot redefine a function declared in an uninstrumented file, so every `Functions\when( 'get_locale' )` starts throwing instead. The two classes now stub it explicitly, and CLAUDE.md records the trap with that dead end. Claude-Session: https://claude.ai/code/session_012XWx9qJdjZdAq8crxM9GCU Co-authored-by: Claude <noreply@anthropic.com>
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 os dois itens que a auditoria pós-merge da PR2 (#1055) encontrou em aberto, listados em #1053. Pré-requisito da PR3: sem isto, o ALTCHA nasceria com um endpoint que devolve desafio matemático.
Summary
DynamicFragments::handle()passa a servir o provider configurado. ChamavaSecurityService::generate_simple_captcha()direto, em dois pontos. É o quinto site de refresh — a PR2 roteou os quatro de retry porwith_fresh_challenge()e não viu este, deixando o docblock dechallenge_payload()afirmando um consumidor que ele não tinha.label/hashsó valeria para o math: um desafio de proof-of-work não tem pergunta nem hash de resposta. O cliente despacha pelo campoproviderdo payload e ignora o que não reconhece, em vez de aplicar meio payload — que apagaria um desafio que o visitante talvez já tenha resolvido.templates/captcha/math-fields.phppõe label e os dois inputs dentro da.ffc-captcha-row.Shortcodes::get_new_captcha_data(), método público cujo único consumidor era o próprio teste.Sobre markup legado
securityBlocks()coleta.ffc-security-containere qualquer.ffc-captcha-rowque esteja fora de uma — markup renderizado antes desse invólucro existir. Páginas em cache são a razão de ser deste endpoint, então ignorar a forma antiga seria justamente falhar no caso que ele existe para atender.Decisão de compatibilidade que vale registrar
O formato do fio mudou (
label/hash→provider/new_label/new_hash). Um navegador que segure uma cópia em cache doffc-dynamic-fragments.jsantigo encontrando o servidor novo escreveriaundefinedno label. Optei por não emitir as duas formas. Em produção o?ver=gira no bump da release e o script antigo não sobrevive; só no ambiente de testes, ondeFFC_VERSIONnão muda entre deploys dedevelop, a janela existe — e um refresh forçado a fecha. Aliases permanentes para proteger um transitório seriam exatamente a indireção que não se paga. Se preferir a rede de proteção, digo e adiciono.Type of change
Sem mudança funcional observável: math é o único provider registrado, então o payload que trafega hoje descreve o mesmo desafio de sempre.
Test plan
composer testpasses locally — suíte completa, 7526 testes / 21677 asserções, verdecomposer lint(PHPStan) passes locally — nível 8 sem erros; WPCS limpo; ESLint zerotestesPHP. O endpoint passou a ser verificado contra um
CaptchaProvidermockado, não contragenerate_simple_captcha()mockado — este último passa igualmente bem com o endpoint furando o contrato, que é justamente o defeito corrigido. A asserção compara o payload inteiro, provando que nada é reformatado no caminho. Dois testes novos cobrem a ramificação por formulário, que não tinha nenhum: um prova que dois formulários recebem tokens distintos, outro que um formulário só não gera mapa.JS. As seis provas de captcha foram verificadas falhando contra o script anterior. Três casos novos: provider desconhecido não toca em nada; markup legado sem
.ffc-security-containercontinua sendo atualizado; dois formulários na página recebem desafios distintos.Duas armadilhas nas fixtures, que a revisão vale a pena conhecer
.ffc-captcha-row— coisa que nenhum site de render faz. Como o script agora escopa por bloco, uma fixture infiel testaria um DOM que o plugin nunca emite. Passaram a espelhar o template.'abc','seed'), e o input real étype="number"— cujo.valuedevolve''para valor não numérico. Ou seja, toda asserção de "limpa a resposta" passava vacuamente. Agora são numéricas, e o motivo está comentado no helper para não voltar.Checklist
npm run build) and the resulting*.min.*files are committedCHANGELOG.mdupdated (andreadme.txt== Changelog ==section if this is a release-bound change) —[Unreleased], emChangedeRemoved; sem bump deFFC_VERSIONModuleBoundaryTestsegue verde sem regenerar baseline:Core\Captchaé sub-namespace deCore, e a arestaFrontend → Corejá existia.Achado registrado à parte
A reescrita expôs um defeito pré-existente, aberto como #1063 e deliberadamente não corrigido aqui: o mapa por formulário só é emitido quando
count($form_ids) > 1, eform_idsconta apenas formulários de certificado. Numa página que misture[ffc_form]com[ffc_self_scheduling]ou[ffc_csv_download]há dois blocos de segurança e umform_idsó, então os dois recebem o mesmo token — e o uso único faz o primeiro envio queimar o do outro. Corrigir exige mudar o protocolo, o que é PR própria; esta preserva o comportamento como estava.🤖 Generated with Claude Code
https://claude.ai/code/session_012XWx9qJdjZdAq8crxM9GCU
Generated by Claude Code