fix(captcha): stop the CSV info screen from spending the challenge (#1061) - #1062
Merged
Merged
Conversation
…1061) The public CSV download is a two-request flow that validated the same captcha twice: the info screen (`PublicCsvDownload::ajax_info()`) and the download itself (`PublicFormsExportSource::authorize_start()`), which re-posts the payload the info screen serialised. That was free while tokens were replayable. Since #1054 they are single use, so the info screen burned the token and the download rejected the answer the visitor had just been told was correct. Separate checking from spending, so the challenge is consumed by the action it authorises rather than by the metadata read that precedes it: - ChallengeStore::is_spent() — the read-only half of the ledger. - SecurityService::peek_simple_captcha() / peek_security_fields() — check without redeeming. An already-spent token is refused here too, or the contradiction would merely move one request downstream. - CaptchaProviderInterface::peek() — in the contract, not one strategy: a proof-of-work solution is replayable until the server records it, exactly as the math token is, so ALTCHA will need this as well. `ajax_info()` now peeks. `authorize_start()` and the no-JS `handle_request()` are single-request paths and keep consuming, which preserves what #1054 closed: a captured (answer, token) pair is still worth exactly one download. 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 15:27
rpgmem
enabled auto-merge (squash)
September 5, 2026 15:27
Coverage Report for CI Build 33974768944Warning No base build found for commit Coverage: 89.923%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.
Closes #1061.
Summary
PublicCsvDownload::ajax_info()) e o download (PublicFormsExportSource::authorize_start()), que re-posta o payload serializado pela tela de detalhes. Isso saía de graça enquanto o token era replayável; desde o security: bind the math captcha to an expiry and spend it on redemption #1054 ele é de uso único, então a tela 1 o queimava e a tela 2 recusava a resposta que o visitante acabara de ver aceita.ajax_info()confere;authorize_start()e o caminho sem JShandle_request()consomem.peek()entra no contrato (CaptchaProviderInterface), não numa estratégia: uma solução de proof-of-work é replayável até o servidor registrá-la, exatamente como o token matemático, então o ALTCHA vai precisar disso igualmente.A propriedade fechada pelo #1054 permanece intacta: um par
(resposta, token)capturado continua valendo um download — o mesmo que vale para um visitante legítimo.Type of change
Test plan
composer testpasses locally — suíte completa, 7525 testes / 21680 asserções, verdecomposer lint(PHPStan) passes locally — nível 8, sem erros nos arquivos tocados; WPCS limpo (regra principal ephpcs-tests.xml.dist)Cobertura nova, em duas camadas:
Unidade —
ChallengeStore::is_spent()(não escreve no ledger; um proof conferido duas vezes ainda pode ser resgatado),MathCaptcha::peek()(aceita resposta correta, recusa errada, deixa o desafio gastável, recusa um já gasto) eSecurityService::peek_security_fields()(mantém o gate do honeypot; não consome, enquantovalidate_security_fields()continua consumindo).Handler —
test_ajax_info_peeks_at_the_captcha_instead_of_spending_itprova queajax_info()chamapeek_security_fields()e nuncavalidate_security_fields(). Verifiquei que essa guarda falha quando o código regride (revertendo a chamada, o teste passa a receberjson_error).Checklist
npm run build) and the resulting*.min.*files are committed — n/a, a correção é só PHPCHANGELOG.mdupdated (andreadme.txt== Changelog ==section if this is a release-bound change) — entrada em[Unreleased] → Fixed; sem bump deFFC_VERSION, que pertence à releaseNota de revisão
Vale olhar com atenção o
includes/frontend/class-ffc-public-csv-download.php:handle_request()eajax_info()têm blocos de captcha quase idênticos, e um primeiro patch meu trocou o errado — o que teria feito o caminho sem JS conferir sem nunca gastar o token, reabrindo o replay exatamente onde ele importa. Os comentários em cada bloco agora dizem qual é qual e por quê.🤖 Generated with Claude Code
https://claude.ai/code/session_012XWx9qJdjZdAq8crxM9GCU
Generated by Claude Code