Skip to content

Fix everything a full review of the plugin found - #4

Merged
AllTerrainDeveloper merged 1 commit into
mainfrom
review/full-plugin-review
Aug 16, 2026
Merged

AllTerrainDeveloper merged 1 commit into
mainfrom
review/full-plugin-review

Conversation

@AllTerrainDeveloper

Copy link
Copy Markdown
Owner

A full review of the plugin, top to bottom, done the way a WordPress.org reviewer and a security reviewer would each do it — and then every confirmed finding fixed, with a regression test standing behind each fix.

Scope of the review

Three questions were asked of the codebase:

  1. Would WordPress.org accept it? — bundled core code, improper core loading, packaging leaks, Plugin Check.
  2. Does it actually work? — not just the test suites, but a live install exercised end to end in wp-env: build a form, render it, submit it with and without JavaScript, read the entry, open every admin surface.
  3. Is the code right? — five parallel review passes over every PHP include and TypeScript module (request surface, engines, admin/misc PHP, builder TS, frontend TS), each finding verified against the source before it was accepted.

1. WordPress.org compliance — clean

  • No WordPress core is bundled or imported. The only core file loaded is wp-admin/includes/image.php for attachment metadata — the documented, accepted pattern. No CDN assets, no shadowed core functions, no wp-load.php shortcuts.
  • Packaging is sound. bin/ships.mjs keeps vendor/, node_modules/, src/, tests/ and every dotfile out of the zip.
  • Plugin Check (the tool reviewers actually run) found two real errors in the shipped tree: both were phpcs:ignore comments in uninstall.php naming a sniff code that doesn't exist (directory_rmdir instead of file_system_operations_rmdir), so the errors they were meant to acknowledge were never suppressed. Fixed. The readme title is aligned with the plugin header, clearing the name-mismatch warning. The shipped tree now reports zero errors.

2. Functionality — verified live, one launch-blocking bug found

TypeScript compiles clean; all bundles build. But the live walk found what no suite had:

  • The Entries and Analytics screens were dead on a default WordPress install. On plain permalinks (the out-of-the-box setting) the REST base already carries ?rest_route=…, and every path that brought its own query string produced a second ? — Core answered rest_no_route and the admin windows showed only an error. Fixed with a query-aware join in api.ts, re-verified in the browser on plain permalinks.
  • Analytics was unreachable without OpenStation. The readme promises every feature works as an ordinary admin page; reporting — a headline feature — was registered only as a desktop window. There is now a Forms → Analytics submenu page mounting the same bundle, verified against the demo data.
  • The full pipeline works: template → rendered form → no-JS POST → validation → stored entry → confirmation, and the same again through the JavaScript fetch path.

3. Code review — ~35 confirmed defects, all fixed

Money and trust

  • A visitor who made a calculation formula fail kept their own posted total — the exact tampering the server recomputation exists to stop. A failed formula now stores empty, never the client's number. And making a formula fail was trivial: a value like 0.00001 stringified to scientific notation, which the tokenizer rejects. Both the PHP and TS engines now substitute plain decimals; the shared conformance table has cases for both.
  • The "Other" choice option was entirely broken: the typed answer replaced the marker before validation, so the whitelist rejected every legitimate use of the feature.

Abuse resistance

  • The arithmetic spam challenge signature had no time component — one captured pair replayed forever. Now hour-bucketed.
  • The role gate only ran for logged-in visitors: logging out was the way past it.
  • Rejected submissions left their uploaded files behind as orphaned attachments — an unauthenticated disk-fill. They are deleted on every failure path now.
  • The public save-and-resume endpoint created a new row per tokenless call, unbounded. Now rate-limited per IP (atf_partial_rate_limit filter, HTTP 429 past the cap), and a resume token can no longer overwrite another form's partial or be corrupted on update.
  • The analytics REST route read the wrong parameter name in its permission callback, so per-form entry restrictions applied everywhere else did not apply to analytics.
  • One huge number typed into an unbounded number field froze the Analytics window for every admin who opened it (one DOM bucket per integer of the range). Histograms now cap at 50 buckets and fractional answers land in the right bar.

Builder data loss

  • Inspector edits made after any autosave mutated an orphaned object and were silently lost.
  • Dragging a card downward landed it one slot above the insertion marker; Alt+ArrowDown was a pure no-op, so keyboard users could not move a field down at all.
  • Undo after "create from template" restored the previous form's fields and autosaved them under the new form's id.
  • Concurrent saves could resolve out of order and destroy edits made mid-flight; notification/confirmation ids collided after delete-then-add; theme switches kept stale overrides on the published form while the preview showed none.

Quieter correctness

  • Repeater rows collided after delete-then-add (a whole row's answers lost on submit), and composite/checkbox sub-fields inside repeaters were never namespaced, so rows overwrote each other.
  • Multi-select image choice stored an empty string — every selection lost.
  • Enter on page 1 of a multi-page form submitted the whole form when later pages were optional.
  • PHP 8 fatals: schema settings persisted raw (arrays where strings belong reached strtotime()/wp_kses_post()), and redirect confirmations rawurlencode()d arrays from visitor-influenced query templates.
  • Multisite uninstall cleaned only the main site; duplicate honeypot DOM ids when a form rendered twice; merge-tag samples switched on field-type slugs that don't exist; stale entries races, page-overflow after bulk delete, and a subscription that kept fetching into a detached DOM after the window closed.

Verification

Check Result
PHPUnit (wp-env) 535 tests, 2,639 assertions — green (was 516)
vitest 242 tests — green (was 204)
tsc --noEmit clean
PHPCS (WordPress standards) 0 errors
Plugin Check, shipped tree 0 errors
Live smoke, pretty & plain permalinks builder, entries, analytics, theme studio, front-end submit — all working

docs/hooks-reference.md and docs/architecture.md are updated for the new public surface (atf_partial_rate_limit, atf_upload_overrides, the hour-bucketed challenge, the Analytics admin page), and the readme's test count now tells the truth (777).

Product verdict

The pitch survives scrutiny: every headline claim in the readme has a working implementation behind it, and the builder is genuinely pleasant to use. The gaps worth closing next are adoption gaps, not correctness ones — importers from the plugins people are switching away from, a deliverability note in the FAQ, and friendlier entry-list rows.

🤖 Generated with Claude Code

https://claude.ai/code/session_0119U4sRRWGcTQdwYreTwpAp

A complete review pass before submission: WordPress.org compliance,
live end-to-end verification in wp-env, and a code review across every
PHP include and TypeScript module, with each confirmed defect fixed and
locked in by a regression test. 535 PHPUnit + 242 vitest tests pass;
Plugin Check reports zero errors on the shipped tree.

The headline fixes: a tampered client total could survive a formula
failure; scientific notation broke formula substitution in both engines;
the "Other" choice option was rejected by its own whitelist; every
query-carrying REST path 404ed on plain-permalink sites, taking the
Entries and Analytics screens down on a default install; and Analytics
was only reachable inside OpenStation despite the readme's promise, so
it is now an ordinary Forms submenu page too.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0119U4sRRWGcTQdwYreTwpAp
@AllTerrainDeveloper
AllTerrainDeveloper merged commit 164b3f5 into main Aug 16, 2026
3 checks passed
@AllTerrainDeveloper
AllTerrainDeveloper deleted the review/full-plugin-review branch August 16, 2026 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant