Files
wownerogue/docs/REMEDIATION_PLAN.md
T
jwinterm 41676fa072 refactor: unify identity resolution on stable user_id (anon_token)
There were two identity systems: SessionManager keyed on the stable, UNIQUE
anon_token, and getOrCreateUser keyed on the mutable, non-unique socket_id.
The latter could create a duplicate "orphan" user row (or resolve the wrong
row) when the socket_id lookup raced the session's socket_id update —
desyncing credits and payouts between the two rows for one human.

- getOrCreateUser now resolves through SessionManager's stable identity first
  (anon_token -> users.id), re-selecting a fresh row by stable id, and only
  falls back to the legacy socket_id lookup when no session exists. The
  last-resort create now warns, since it indicates a missing session.
- gameModeManager.sessionManager is wired in SocketHandlers so the
  session-first path is active in production.
- anon_token is already UNIQUE (migration 003), so the stable-identity
  constraint already exists; no new migration needed.

Re-keying the in-memory activeGames map by user_id is deferred (invasive;
already mitigated by stamping dbUserId on the game in the disconnect fix).

Adds identityResolution.test.js proving no duplicate row is created when a
session exists. Full suite: 17 suites / 136 tests.
2026-05-31 10:15:08 -04:00

11 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; idempotent 010; atomic migration runner. ✅ Migration 018 widens users.credits + credit_transactions.amount/balance_after to BIGINT. Migration 010 rewritten with conname-guarded DO blocks (re-runnable, no startup wedge). Runner now applies each migration and records the ledger row in one transaction (no applied-but-unrecorded state). Verified end-to-end: all 18 migrations applied cleanly to a throwaway Postgres DB, re-run skipped via the ledger, credits confirmed bigint, all CHECKs present.

Phase 2 — Identity, abuse & concurrency (🟠)

  • 2.1 L — Unify identity resolution on stable user_id. ✅ getOrCreateUser now resolves through SessionManager's stable anon_token→id identity first (re-selecting a fresh row by stable id), falling back to the legacy socket_id lookup only when no session exists — eliminating the duplicate "orphan" user row that caused credit/payout desync. gameModeManager.sessionManager wired in SocketHandlers. anon_token is already UNIQUE (migration 003), so the stable-identity constraint exists. New identityResolution.test.js proves no duplicate row is created when a session exists. Deferred: re-keying the in-memory activeGames map by user_id (invasive; already mitigated in 0.5 by stamping dbUserId on the game) — tracked as a follow-up.
  • 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.