Project review — Security, Architecture & Performance (v1.0.1)

[bornmw]

Comprehensive project review from three perspectives: 🔒 Cybersecurity, 🏛 Architecture, ⚡ Performance.

Scope: v1.0.1 (b25eab8), the WP 7.1 compatibility release. Runtime behavior verified against WordPress 7.0 and 7.1 (Playwright e2e, 18/18 on both).

---

Findings overview

#	Perspective	Severity	Finding
S1	Security	High	XML-RPC "blocking" is a doc claim, not code — the e2e test is tautological
S2	Security	High	REST comments: anonymous posts blocked unconditionally, readme says the opposite
S3	Security	Medium	cardea_difficulty filter applied at challenge time but not verification time
S4	Security	Medium	Distinct wp_die messages = oracle for attackers
S5	Security	Medium	crypto.subtle requires TLS; no fallback → broken comments on plain HTTP
S6	Security	Low	Replay TOCTOU race (inherent to one-shot tokens)
S7	Security	Low	Worker counter wrap at 100M breaks difficulty 7–8 (~98% of d=8 challenges)
A1	Architecture	Medium	Two comment paths, two behaviors: form verifies PoW, REST deny-all
A2	Architecture	Medium	Cardea_Core mixes domain logic with I/O (superglobals, wp_die)
A3	Architecture	Low	Version string triplicated across 3 files
A4	Architecture	Low	composer.json PSR-4 mapping is dead (global classes)
A5	Architecture	Low	Exclusion lists duplicated in make package / make sync-svn
A6	Architecture	Low	E2E blueprint boilerplate ×6; loose text-based assertions
P1	Performance	Medium	~2 wp_options rows per comment, never reaped ("swept by cron" claim is false)
P2	Performance	Medium	Client mining 10–30× slower than needed (full re-hash + per-digest async overhead)
P3	Performance	Low	Unbounded worker CPU burn at high difficulty (see S7)

---

1. Cybersecurity review

Verified strengths

• Stateless challenge generation — HMAC over nonce|timestamp|salt (includes/class-cardea-core.php:98), zero DB writes on page load.
• Challenge endpoint is not a load amplifier — cardea/v1/challenge (:212) costs nonce + random_bytes(16) + one hash_hmac; a bot farming challenges pays exactly the PoW the plugin intends to impose.
• Constant-time signature check (hash_equals, :111); one-shot replay via transient (:165).
• Client-sent difficulty never trusted — hidden cardea_difficulty field ignored server-side (:283).
• uninstall.php cleans transients with a prepared LIKE, covering both _transient_ and _transient_timeout_ rows.

S1 — [High] XML-RPC protection claimed, not implemented

The plugin registers no XML-RPC hooks at all. The e2e "should block XML-RPC comment submissions" (tests/e2e/xmlrpc-comments.spec.js:76) posts wp.newComment — a method that does not exist in WordPress — and treats any failure as success:

const isBlocked = responseBody.toLowerCase().includes('missing challenge fields') ||
                  responseBody.toLowerCase().includes('accepts post requests only') ||
                  response.status() !== 200;

This proves WordPress rejects an unknown method — nothing about Cardea. Meanwhile the readme claims "Smart Pathway Protection … blocks XML-RPC botnets" (readme.txt:57), and the one XML-RPC vector that actually remains (pingback abuse) is deliberately bypassed by design.
Recommendation: implement the claimed protection, test with real XML-RPC fault codes, or remove the claim.

S2 — [High] REST comments deny-all; readme says they're unaffected

verify_rest_comment (class-cardea-core.php:242) returns a 403 WP_Error unconditionally for logged-out non-trackback users — it never verifies a PoW solution, so no client can ever post via wp/v2/comments (Jetpack-style / app-based commenters break silently), while readme.txt:107 states "REST API comments, XML-RPC, and other methods are not affected".
Recommendation: bring the REST path to parity (verify the same fields + solution as the form path) and align the docs.

S3 — [Medium] Difficulty filter not applied at verification

get_difficulty() applies apply_filters( 'cardea_difficulty' ) (:54), but verify_comment_pow re-reads the raw option (:310). A filter lowering difficulty (a documented extension point) would make legitimate commenters fail verification.
Recommendation: verify against the same filtered value used at generation.

S4 — [Medium] Error-message oracle

Five distinct wp_die copies ("Missing challenge fields", "Security check failed", "Challenge has expired", "already been used", "Proof-of-Work verification failed") tell an attacker exactly which check failed.
Recommendation: one generic message.

S5 — [Medium] Plain-HTTP sites break silently

crypto.subtle (assets/js/pow-worker.js:79) is undefined on non-TLS origins; there is no fallback, so the solution field stays empty and the server wp_die()s every submission. The advertised "graceful fallback" only covers a missing Worker.
Recommendation: JS SHA-256 fallback (also fixes P2).

S6 — [Low] Replay TOCTOU

A man-in-the-middle delivering the captured challenge+solution before the legitimate user wins the one-shot race; the user then sees "already been used". Inherent to one-shot tokens — document rather than fix.

S7 — [Low] Worker counter wrap breaks difficulty 7–8

The counter resets at 100,000,000 (assets/js/pow-worker.js:95). Expected first solution: d=7 → 16⁷ ≈ 2.7×10⁸, d=8 → 16⁸ ≈ 4.3×10⁹ — so ~63 % (d=7) / ~98 % (d=8) of challenges cycle 0…100M forever with the user burning a core indefinitely. The admin screen happily accepts 1–8 (class-cardea-admin.php:128).
Recommendation: unbounded counter, or cap difficulty at what the wrap actually supports.

---

2. Architecture review

A1 — [Medium] Two comment paths, two behaviors

Form path: fields → HMAC → PoW → stamp transient. REST path: deny-all, verify nothing. Same purpose, two behaviors. The right shape: one Core::verify( $challenge, $solution ) (pure: signature, window, replay, PoW) + two thin adapters (form handler, rest_pre_insert_comment). The REST adapter cannot be made correct without this refactoring (see S2).

A2 — [Medium] Cardea_Core mixes domain logic with I/O

verify_comment_pow (:269) reads $_POST and calls wp_die inside the domain class; the domain logic is only testable through mocks and only proven by e2e. Split generate/verify (pure) from FormAdapter/RestAdapter and both become unit-testable.

A3 — [Low] Version string triplicated

cardea.php header + CARDEA_VERSION, package.json/lock, and tests/phpunit/bootstrap.php:27 — one bump required three edits. Single source of truth: parse the plugin header.

A4 — [Low] Dead PSR-4 autoload

composer.json maps Cardea\ → includes/, but the classes are global and loaded via require_once. Either namespace the classes (better isolation against other plugins) or drop the mapping.

A5 — [Low] Duplicated exclusion lists

package (zip) and sync-svn (rsync) in the Makefile hand-keep two nearly identical exclusion lists. One wrong edit ships dev artifacts to production. One list, two consumers.

A6 — [Low] E2E boilerplate + weak assertions

Six near-identical runCLI blueprints (five spec files + admin block). A shared fixture would also make WP-version parameterization (e.g. WP_VERSION env) trivial for the "test against N−1 release" discipline. The xmlrpc/trackback specs assert on loose text instead of structured XML-RPC fault codes.

---

3. Performance review

Verified good (server side)

• Page load: 0 queries, 0 writes — the challenge is computed on first focus/input, not on render (assets/js/frontend.js:275).
• Challenge endpoint: wp_create_nonce + random_bytes(16) + one hash_hmac ≈ tens of µs, no DB.
• Verification: one hash('sha256') + hash_equals + in-memory option reads; the only write is set_transient, and only on successful comments.
• Enqueue scoping (singular + comments open + not logged in, class-cardea-frontend.php:65), worker on-demand, terminated on solve/unload — all correct.

P1 — [Medium] wp_options grows ~2 rows per comment, forever

set_transient( 'cardea_used_' . $signature ) keys are unique per comment and never read again; WordPress deletes a transient only when an expired key is fetched, so the rows persist indefinitely. The readme claims "expired tokens are automatically swept by WordPress cron" (readme.txt:50, :125, :134) — the plugin registers no cron, and core has no transient sweeper.
Recommendation: one capped option holding recent used signatures (prune on write), or a real wp_schedule sweep — and fix the copy.

P2 — [Medium] Client mining 10–30× slower than needed

The worker Promise.alls 1000 crypto.subtle.digest calls per batch, re-encoding and re-hashing the full challenge + counter string every time (assets/js/pow-worker.js:69) — per-digest async overhead plus redundant work. Classic fix: fix the counter width (zero-padded) so the message length is constant → precompute the SHA-256 state up to the final block once, then process only the final block per counter with a compact synchronous JS SHA-256. Wire format unchanged (server hashes string . solution as today). Expected: difficulty-4 solves from ~1–5 s → < 100 ms. Bonus: removes the TLS dependency (S5).

P3 — [Low] Unbounded worker CPU at hard difficulties

A stuck d=8 user burns 100 % of a core indefinitely (see S7) — cap total attempts and surface a "refresh the page for a new challenge" state instead of looping.

---

Suggested actions (priority order)

• S1 — Fix or defuse the XML-RPC claim; replace the tautological test with real fault-code assertions
• S7 — Remove the 100M counter wrap (breaks difficulty 7–8)
• P1 — Cap the replay store (single pruned option); correct the "swept by cron" claim
• S2 / A1 — REST parity: verify the PoW on the REST path like the form (single Core::verify)
• S3 — Apply cardea_difficulty filter consistently at verification
• P2 / S5 — Fixed-width counter + precomputed prefix state in the worker (JS fallback for non-TLS)
• S4 — Single generic error message
• A3 — Single source of truth for the version string
• A5 / A6 — Unify exclusion lists + deduplicate e2e fixtures

---

Review of bornmw/cardea @ b25eab8 (v1.0.1). Behavior verified against WordPress 7.0 and 7.1 (Playwright e2e, 18/18 on both). File references use v1.0.1 line numbers.

[bornmw]

All 16 findings are addressed via three stacked PRs (merge order: #2 → #3 → #4). Fixes + pushbacks are documented in each PR description.

• security: REST PoW parity, unified failure message, accurate auth docs and tests #2 — security: REST PoW parity, unified failure message, accurate XML-RPC/REST docs + verified tests. Pushback: no XML-RPC "botnet block" — core exposes no anonymous XML-RPC comment method; a blanket block would break the intentional pingback bypass.
• architecture: extract Cardea_Comment_Gate, single version source, unified excludes, shared e2e fixtures #3 — architecture: Cardea_Core slimmed to a pure PoW domain; new Cardea_Comment_Gate; version parsed from cardea.php in the test bootstrap; dead PSR-4 mapping removed; one Makefile exclusion list for zip + SVN; shared e2e fixture with WP_VERSION env. Pushback: no class namespacing (unnecessary compat-breaking diff for a released plugin).
• performance: bounded replay store, synchronous JS SHA-256 miner, accurate cost docs #4 — performance: bounded self-pruning replay store (single cardea_used option, cap 1024) replacing 2 transient rows per comment; synchronous JS SHA-256 miner (no crypto.subtle → works on plain HTTP, unbounded counter, wire format unchanged); difficulty solve-time guidance; jest now tests the real worker. Pushbacks: no padded-counter/"prefix precompute" (wire format is hash(challenge . counter); the 55-byte challenge straddles a block boundary — nothing to precompute), no cron sweep, no hard attempt cap (documented solve times instead).

Notable extra finding from #4's e2e: WordPress 7.1 has removed set_option() (verified via function_exists in a live WP 7.1 Playground). The replay store writes via update_option(), the current write API present in all supported versions.