Repository navigation
Answer the WordPress.org review: consent, permissions, and sanitising early - #47
Merged
Merged
Conversation
… early Four findings from the second review round, each fixed at its root: - Akismet is now off by default on every form. Sending a submission — the answers, the sender's IP and user agent — to Akismet's servers is the site owner's per-form decision, never a default the plugin makes for them. The builder's checkbox now says exactly what switching it on sends. - The abilities ask their permission question about the form in hand. The get-form, list-entries and form-report gates pass the input's form_id to the alltfo_can_read_entries filter, list-forms filters what it lists per form, and submit-form requires an authenticated user — the minted time-trap signature was only ever honest on a channel that identifies its caller. - The public /track route bumps only published forms, so it can no longer probe or inflate drafts, trash or archives. - The submission request is deep-sanitised at the top of the pipeline, before the alltfo_before_submission action or anything else reads a byte of it; each answer is still sanitised again through its own field type. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DVAaHFURDGeBzeuWHZ2dcP
The no-JS fallback kses'd the confirmation at output while the AJAX path handed the resolved string straight to the bundle's innerHTML, trusting the field-type sanitisers alone to have stripped every tag from the values the merge tags splice in. They do — but two paths to one string deserve one armour, so the message is now kses'd at resolution, where both paths read it. With a regression test that submits a script-and-onerror payload through a merge-tagged confirmation and asserts the payload is gone entirely. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DVAaHFURDGeBzeuWHZ2dcP
wp_http_validate_url() ran once, on the configured URL — and then wp_remote_post() followed any redirect the endpoint chose to answer with, internal addresses included. wp_safe_remote_post() re-validates each hop, which closes the 302-to-somewhere-private gap without changing what a well-behaved webhook sees. With the plugin's first webhook transport test, pinning reject_unsafe_urls and the HMAC signature header. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DVAaHFURDGeBzeuWHZ2dcP
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
1 participant
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.
The second WP.org review round found four issues. Each is fixed at its root, with a regression test.
🔴 Phoning home — Akismet on by default (Guidelines 7 & 9)
New forms enabled Akismet by default, so a site that already had Akismet configured transmitted submission contents, IPs and user agents without anyone opting in for that form.
alltfo_default_schema()now shipsspam.akismet => false. Switching it on is a per-form decision by the site owner — which is whatreadme.txtalways claimed under External services; the default now matches the claim.hooks-reference.md),README.md,readme.txtand thespam.phpheader updated to match.🔴 Permission callbacks on abilities and the
/trackrouteget-form/list-entries/form-report— the permission callback now receives the ability input (the Abilities API passes it whenever an input schema is declared) and asksalltfo_can_read_entries( $form_id )about that form, so the per-form filter a site uses to confine a department actually confines these abilities.list-formsfilters what it lists through the same gate.submit-form— no longer__return_true. It requires an authenticated user, because the ability mints a valid time-trap signature and that liberty is only honest on a channel that identifies its caller. Anonymous visitors still have the rendered form and the public REST/submitroute, where every anti-bot check runs whole. Any logged-in user may submit — submitting is what visitors do./track— the handler now requirespublishstatus, so the public route can neither probe nor inflate drafts, trash or archives. It still answers 200 regardless, never confirming what exists.docs/abilities.mdpermission table updated to match.🔴 Sanitize early
The raw
$_POSTreached thealltfo_before_submissionaction before field-type sanitisation ran. A newalltfo_sanitize_request()deep-sanitises the whole request — keys included — at the top ofalltfo_process_submission(), before any code reads a byte of it, covering the no-JS POST path, the REST route and the abilities in one place. Each answer is still sanitised again through its own field type; this pass is about never holding dirty data, that one about the shape each field expects. The sanitiser is registered with PHPCS as acustomSanitizingFunctionsentry, so the previously-ignored sniff now passes on its own.Tests
Six new tests: Akismet default is off (a regression here is a privacy bug), logged-out callers are refused everywhere including submit-form, a subscriber can submit, the per-form filter confines get-form/form-report/list-forms,
/trackbumps only published forms, and the request arrives sanitised at the first hook.738 PHPUnit ✓ · 372 vitest ✓ · PHPCS 0 errors (no new warnings) · Plugin Check: no errors🤖 Generated with Claude Code
https://claude.ai/code/session_01DVAaHFURDGeBzeuWHZ2dcP