Files
wownerogue/docs/REMEDIATION_PLAN.md
T
jwinterm 2e6f1a6335 fix: admin refund actually sends funds (was calling a nonexistent method)
The admin "send funds" refund path called walletRPCService.sendPayment(),
which does not exist — guarded by a typeof check, so it silently never sent
while still reporting the refund as processed.

Now uses the real processPayout() (validates the address and transfers,
returning { success, txHash }), and refunds the amount the user actually paid
(received_amount, recorded at confirmation) rather than the expected amount,
falling back to expected for older records.
2026-05-31 10:02:19 -04:00

9.7 KiB
Raw Blame History

Wownerogue Remediation & Cleanup Plan

Derived from the deep code review (May 2026). Goal: fix every BAD/UGLY finding, remove the spaghetti, and reach a real-funds launch gate. Phases are ordered so launch-blockers get surgical fixes first, then correctness hardening, then structural cleanup, then a test/ops overhaul to lock it in.

Guiding principles

  1. Stabilize before restructuring. Fix exploitable money bugs as minimal patches, ship, then refactor.
  2. Single source of truth for the three things that bite: money (one BigInt module), identity (one user_id), randomness (one per-game seeded RNG).
  3. Defense in depth. Every check at the DB and app level; every render escapes and the server escapes.
  4. Every change ships with a test that fails before and passes after.

Key: S ≤ half day · M ~1–3 days · L ~1 week+ · 🔴 launch-blocker · 🟠 pre-launch · 🟡 post-launch


Phase 0 — Stop the bleeding (launch-blockers)

  • 0.1 🔴 S — Verify amount received ≥ expected before confirming. ✅ Gate on status.complete in paymentHandlers.js; underpaid → stay pending + payment_underpaid warning (once). received_amount now recorded for single-game + credits. Regression test added (payment_flow.integration.test.js). 113 tests green.
  • 0.2 🔴 M — Make provably-fair real. ✅ Per-game seeded RNG (seedToInt) threaded through dungeon.js (Digger reseed + all Math.random() → rng), monster.js (seededShuffle), game.js (chase + generation). DungeonGenerator.regenerateFromSeed + layoutFingerprint; /api/verify regenerates & returns fingerprint. Determinism test suite added (provablyFair.test.js). Verifier can now reproduce the exact dungeon from the seed.
  • 0.3 🔴 M — Fix batch-payout tx_hash collision + double-pay. ✅ Migration 015 drops the payouts tx_hash unique index (batches legitimately share a tx_hash) and extends the one-per-game guard to cover processing. Batch completion now happens in ONE transaction via id = ANY (no row stranded mid-batch); BIGINT stat math (no float). Ambiguous batch failures → needs_review (retry service won't re-send) + operator alert. New batchPayout.test.js covers both paths.
  • 0.4 🔴 S — Harden identity transport. ✅ Client sends anon_token in handshake auth (not the URL query → no proxy/referer/access-log leakage); server reads auth first with a query-string fallback for older clients. Token is now rotated on every resume (rotateToken), so a leaked token is invalidated when the real owner reconnects; client already persists the returned token. Rotation + old-token-replay-rejection test added to security.test.js.
  • 0.5 🟠 S — Single-game disconnect recovery. ✅ recoverPendingPayments now also recovers confirmed-but-unconsumed single_game payments (no games.payment_id link) by granting the equivalent credits, idempotently (per-payment reason key, row-locked). Disconnect preservation made deterministic: the stable dbUserId is stamped on the game at creation, so suspend/restore never needs a fragile disconnect-time DB lookup — every game (paid or free) is reconnectable whenever a session exists; the drop-branch is now only the genuine no-session edge. New singleGameRecovery.test.js (grant / consumed / idempotent).
  • 0.6 🟠 S — Client + admin XSS and CSP. ✅ Single shared escapeHtml helper (html/js/core/escapeHtml.js, escapes all five chars incl. both quotes); leaderboard delegates to it. Chat sinks (onChatBroadcast/onChatHistory) now escape username + message. Admin panel escapes user-controlled fields (payout address, socket id, player name, status) and its local escapeHtml now escapes quotes (safe in title="…"). CSP + X-Content-Type-Options/X-Frame-Options/Referrer-Policy headers added via middleware in index.js (connect-src 'self' keeps Socket.IO working; nonce-based tightening tracked as Phase 4.4).

Phase 1 — Money correctness hardening (🟠)

  • 1.1 M — src/money/atomic.js BigInt module. ✅ Exact integer money math (toBig/toSafe/sum/add/mulByDecimal/format). calculatePayout now computes base * multiplier via BigInt (half-up rounding, narrows to number only when exact). walletRPCService batch total + fee sum use money.sum instead of float reduce. Property tests (moneyAtomic.test.js) prove exactness on amounts above 2^53 where the old float path drifted.
  • 1.2 S — Snapshot payout config at game start. ✅ Migration 016 adds payout_escape_amount/payout_treasure_amount (BIGINT) + multipliers to games. Both start paths stamp the resolved payout amounts (via _computePayoutSnapshot); completeGame pays from the snapshot (falls back to live calc only for pre-snapshot games). New payoutSnapshot.test.js proves a mid-game config change can't alter an in-flight payout.
  • 1.3 S — Close payout uniqueness holes. ✅ processing covered in the unique index (migration 015) and in the in-code existing-payout guard. Migration 017 adds idempotent NOT VALID CHECK constraints on payments.status, payments.payment_type, and payouts.status so a typo'd status can't silently orphan a money row (enforces new writes, won't choke on legacy rows). (games.status/game_mode CHECKs deferred — transient states, lower money-impact.)
  • 1.4 S — Fix retry stat guard + non-atomic tx_hash write. ✅ The "already on-chain" path used a NOT EXISTS guard against the row it had just marked completed (always skipped → never counted); replaced with a conditional UPDATE … WHERE status <> 'completed' RETURNING so stats count exactly once. Successful retries now store tx_hash + mark completed + count stats in ONE transaction (no stranded-tx_hash window). New payoutRetry.test.js covers count-once, no-double-count, and atomic completion.
  • 1.5 S — Fix dead refund send-path. ✅ The admin refund called a nonexistent walletRPCService.sendPayment() (guarded by a typeof check), so refunds were silently never sent while still reporting success. Now uses the real processPayout() (validates address + transfers, returns {success, txHash}) and refunds the actually received amount (received_amount, falling back to expected).
  • 1.6 S — credits → BIGINT; make migration 010 idempotent (src/migrations/010_security_constraints.sql); per-migration transactions in runner (src/db/databaseManager.js:146).

Phase 2 — Identity, abuse & concurrency (🟠)

  • 2.1 L — Unify identity on user_id. One IdentityService.resolve(socket); kill getOrCreateUser(socketId) keying (gameModeManager.js:926); key activeGames by user_id.
  • 2.2 M — Reconnect-proof rate limiting. Key on user_id/IP; IP-limit chat; set trust proxy; de-dup limit configs (src/network/rateLimiter.js:80).
  • 2.3 M — Input validation + leak fixes. Validate socket payloads (connectionHandler.js:90); TTL-evict mempoolNotified (socketHandlers.js:223); evict sessions on disconnect; wire all dispose() into shutdown(); snapshot in checkGamesTimeout (socketHandlers.js:1012).

Phase 3 — Game integrity (🟠)

  • 3.1 S — Uniform time budget (elapsed/fixed-block, not "next block"); fix early-entry (src/network/queueHandler.js:241).
  • 3.2 S — Reachability guarantee entrance→treasure→exit (src/game/dungeon.js).
  • 3.3 S — Swap/passthrough collision fix (src/game/game.js:181).

Phase 4 — De-spaghetti / structural cleanup (🟡)

  • 4.1 L — Break up index.js into app.js, routes/{public,user,admin,auth}.js, views/verify.js, server.js.
  • 4.2 M — Kill duplicates. Merge rpcService+rpccalls; delete dead handlePlayerMove (socketHandlers.js:244); unify payout_address widths; remove QueryValidator theater (databaseManager.js:21).
  • 4.3 M — Config as immutable snapshots; stop mutating process.env (gameModeManager.js:189).
  • 4.4 M — Frontend de-spaghetti. Shared escapeHtml/dom.js; kill double-declares + dead _appendMessage; explicit init ordering; upgrade/drop jQuery; gate broadcast logging (broadcastManager.js:33).
  • 4.5 S — Delete stale test/ scratch files.

Phase 5 — Test & ops overhaul (🟠 — the launch gate)

  • 5.1 L — Real money-path tests against Postgres (no stubbed withTransaction): under/overpay, double-confirm, batch dispatch, credit race, retry idempotency; property-test money/atomic.js.
  • 5.2 S — Run real wownero-wallet-rpc integration test (open in TODO since day one).
  • 5.3 S — environmentValidator hard-stop (process.exit(1)); add admin-key/mainnet/simulated-block checks (src/index.js:33).
  • 5.4 S — Reconcile docs (README vs .env.example house odds); document real provably-fair.
  • 5.5 M — Durability: DB/Redis-backed idempotency + rate limiter.
  • 5.6 S — TLS via reverse proxy.

Sequencing

Build money/atomic.js (1.1) and per-game RNG (0.2) early — Phases 1/3/4 depend on them.

Phase 0 (days) → freeze features, ship → Phase 1 (+5.1/5.2 in parallel)
→ Phase 2 → Phase 3 → Phase 4 (safe now) → finish Phase 5 → LAUNCH GATE

Launch gate (real funds)

  • Under/overpayment correct + reconciled (0.1)
  • Provably-fair reproduces dungeon, or claim removed (0.2)
  • No double-payout under batch+retry, test-proven (0.3, 1.3, 1.4, 5.1)
  • All money math BigInt, property-tested (1.1)
  • One identity, impersonation closed (0.4, 2.1)
  • XSS closed + CSP (0.6)
  • Real wallet-rpc integration test executed (5.2)
  • Validator hard-stops on misconfig (5.3)
  • TLS in front (5.6)

Estimate: ~3–4 weeks to launch gate; Phase 0 in days.