diff --git a/docs/REMEDIATION_PLAN.md b/docs/REMEDIATION_PLAN.md index ccc1c27..50dcfa3 100644 --- a/docs/REMEDIATION_PLAN.md +++ b/docs/REMEDIATION_PLAN.md @@ -30,7 +30,7 @@ Key: `S` ≤ half day · `M` ~1–3 days · `L` ~1 week+ · 🔴 launch-blocker - [x] **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.)_ - [x] **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. - [x] **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`). +- [x] **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 on `user_id`.** One `IdentityService.resolve(socket)`; kill `getOrCreateUser(socketId)` keying (`gameModeManager.js:926`); key `activeGames` by `user_id`. diff --git a/src/db/databaseManager.js b/src/db/databaseManager.js index b8f54e7..faa0377 100644 --- a/src/db/databaseManager.js +++ b/src/db/databaseManager.js @@ -143,8 +143,22 @@ class DatabaseManager { const filePath = path.join(migrationsPath, file); const sql = await fs.readFile(filePath, 'utf8'); console.log(`🔄 Running migration: ${file}`); - await this.pool.query(sql); - await this.pool.query(`INSERT INTO schema_migrations (filename) VALUES ($1)`, [file]); + + // Apply the migration AND record it in the ledger atomically in one + // transaction, so a migration can never be applied-but-unrecorded (which + // would re-run a possibly-non-idempotent migration on the next boot). + const client = await this.pool.connect(); + try { + await client.query('BEGIN'); + await client.query(sql); + await client.query(`INSERT INTO schema_migrations (filename) VALUES ($1)`, [file]); + await client.query('COMMIT'); + } catch (migErr) { + await client.query('ROLLBACK').catch(() => {}); + throw migErr; + } finally { + client.release(); + } console.log(`✅ Migration completed: ${file}`); } diff --git a/src/migrations/010_security_constraints.sql b/src/migrations/010_security_constraints.sql index d620fc2..2cd7698 100644 --- a/src/migrations/010_security_constraints.sql +++ b/src/migrations/010_security_constraints.sql @@ -1,34 +1,41 @@ -- Migration 010: Security constraints to prevent invalid data states --- These constraints enforce business rules at the database level as a safety net +-- These constraints enforce business rules at the database level as a safety net. +-- +-- Each ADD CONSTRAINT is guarded by a conname check so the migration is idempotent and +-- safe to re-run / recover from a partial apply. (Postgres has no ADD CONSTRAINT IF NOT +-- EXISTS, and a bare ADD CONSTRAINT on an existing constraint hard-fails, which previously +-- could wedge startup.) --- Ensure credits can never go negative --- This catches any code bugs that might bypass application-level checks -ALTER TABLE users ADD CONSTRAINT users_credits_non_negative - CHECK (credits >= 0); +DO $$ +BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_constraint WHERE conname = 'users_credits_non_negative') THEN + ALTER TABLE users ADD CONSTRAINT users_credits_non_negative CHECK (credits >= 0); + END IF; --- Ensure total_amount_won can never go negative -ALTER TABLE users ADD CONSTRAINT users_total_won_non_negative - CHECK (total_amount_won >= 0); + IF NOT EXISTS (SELECT 1 FROM pg_constraint WHERE conname = 'users_total_won_non_negative') THEN + ALTER TABLE users ADD CONSTRAINT users_total_won_non_negative CHECK (total_amount_won >= 0); + END IF; --- Ensure total_games_played can never go negative -ALTER TABLE users ADD CONSTRAINT users_games_played_non_negative - CHECK (total_games_played >= 0); + IF NOT EXISTS (SELECT 1 FROM pg_constraint WHERE conname = 'users_games_played_non_negative') THEN + ALTER TABLE users ADD CONSTRAINT users_games_played_non_negative CHECK (total_games_played >= 0); + END IF; --- Ensure total_games_won can never go negative -ALTER TABLE users ADD CONSTRAINT users_games_won_non_negative - CHECK (total_games_won >= 0); + IF NOT EXISTS (SELECT 1 FROM pg_constraint WHERE conname = 'users_games_won_non_negative') THEN + ALTER TABLE users ADD CONSTRAINT users_games_won_non_negative CHECK (total_games_won >= 0); + END IF; --- Ensure payout amounts are always positive -ALTER TABLE payouts ADD CONSTRAINT payouts_amount_positive - CHECK (amount > 0); + IF NOT EXISTS (SELECT 1 FROM pg_constraint WHERE conname = 'payouts_amount_positive') THEN + ALTER TABLE payouts ADD CONSTRAINT payouts_amount_positive CHECK (amount > 0); + END IF; --- Ensure payment amounts are always positive -ALTER TABLE payments ADD CONSTRAINT payments_amount_positive - CHECK (expected_amount > 0); + IF NOT EXISTS (SELECT 1 FROM pg_constraint WHERE conname = 'payments_amount_positive') THEN + ALTER TABLE payments ADD CONSTRAINT payments_amount_positive CHECK (expected_amount > 0); + END IF; --- Ensure multiplier is non-negative (0 means no payout) -ALTER TABLE payouts ADD CONSTRAINT payouts_multiplier_non_negative - CHECK (multiplier >= 0); + IF NOT EXISTS (SELECT 1 FROM pg_constraint WHERE conname = 'payouts_multiplier_non_negative') THEN + ALTER TABLE payouts ADD CONSTRAINT payouts_multiplier_non_negative CHECK (multiplier >= 0); + END IF; +END $$; -- Add index for faster payout retry queries CREATE INDEX IF NOT EXISTS idx_payouts_retry_candidates ON payouts (status, retry_count, last_retry_at) diff --git a/src/migrations/018_credits_bigint.sql b/src/migrations/018_credits_bigint.sql new file mode 100644 index 0000000..43abcb0 --- /dev/null +++ b/src/migrations/018_credits_bigint.sql @@ -0,0 +1,10 @@ +-- Migration 018: Widen credit columns to BIGINT +-- +-- users.credits and credit_transactions.amount/balance_after were INTEGER (max ~2.1e9), +-- inconsistent with the BIGINT atomic-money columns and a latent overflow risk since +-- CREDITS_PACKAGES is admin-configurable with no upper bound. ALTER ... TYPE BIGINT is a +-- no-op when the column is already BIGINT, so this is safe to re-run. +ALTER TABLE users ALTER COLUMN credits TYPE BIGINT; + +ALTER TABLE credit_transactions ALTER COLUMN amount TYPE BIGINT; +ALTER TABLE credit_transactions ALTER COLUMN balance_after TYPE BIGINT;