Commit 53a723a7 authored by Mahmoud Aglan's avatar Mahmoud Aglan

docs: add schema vs code mismatch audit to WTF.md

SSHed into Supabase DB and compared all table schemas, RLS policies,
RPCs, enums, indexes, and reward_config rows against what the code
expects. Found 21 mismatches including: service key bypassing RLS
(making RLS useless), missing indexes, missing reward rows,
handleDraw destroying game_state, and complete_match trusting
client-provided winners array.
Co-Authored-By: 's avatarClaude Opus 4.6 <noreply@anthropic.com>
parent 31bbb38b
...@@ -402,3 +402,201 @@ ...@@ -402,3 +402,201 @@
| ludo/scenes/game.js | 11 | ~1450 | 1 per 132 lines | | ludo/scenes/game.js | 11 | ~1450 | 1 per 132 lines |
**Worst file: `multiplayer.js` — 1 WTF every 13 lines.** **Worst file: `multiplayer.js` — 1 WTF every 13 lines.**
---
## SCHEMA vs CODE MISMATCH AUDIT (Zero Mismatches Target)
**Database:** Supabase PostgreSQL on `safe-supabase-db`
**Connection:** PHP uses `supabaseService()` = service_role key (bypasses ALL RLS)
### RLS vs Service Key — THE FUNDAMENTAL PROBLEM
The `matches` table has good RLS:
- INSERT: only if you're white_player_id or black_player_id
- UPDATE: only if you're a player in the match
- SELECT: everyone can see
**BUT** the PHP API uses `supabaseService()` which BYPASSES RLS entirely. So:
- `game.php:handleGameMove` → uses service key → NO auth check → any user can edit any match (**CONFIRMED CRITICAL**)
- `game.php:handleResign` → uses service key → NO auth check → any user can resign any match (**CONFIRMED CRITICAL**)
- `game.php:handleComplete` → uses service key → passes client-provided `$winners` to `complete_match` RPC → any user steals rewards (**CONFIRMED CRITICAL**)
The RLS was designed for client-side Supabase access (anon key). The PHP API was written assuming it IS the security layer but forgot to add its own auth checks.
### Table Existence — Code vs DB
| Table Code Expects | Exists in DB | Status |
|---|---|---|
| `matches` | Yes | OK |
| `matchmaking_queue` | Yes | OK |
| `profiles` | Yes | OK |
| `friendships` | Yes | OK |
| `friend_messages` | Yes | OK |
| `notifications` | Yes | OK |
| `domino_matches` | Yes | OK |
| `ludo_matches` | Yes | OK |
| `player_blocks` | Yes | OK |
| `cheat_reports` | Yes | OK |
| `rating_history` | Yes | OK |
| `player_achievements` | Yes | OK |
| `cosmetics` | Yes | OK |
| `player_cosmetics` | Yes | OK |
| `platform_theme` | Yes | OK |
| `reward_config` | Yes | OK |
| `player_game_ratings` | Yes | OK |
| `mp_log` | Yes | OK |
| `activity_feed` | Yes | OK |
| `groups` / `group_members` | Yes | OK |
| `tournaments` | Yes | OK |
**Result: 0 missing tables. All tables exist.**
### Column Mismatches — Code Expects vs DB Has
| # | File | What Code Expects | What DB Has | Status |
|---|------|---|---|---|
| 1 | `matchmaking.php:59` | `profiles.elo_blitz` | Column EXISTS (default 1200) | OK — but code falls back to `elo_rapid` when null, not when absent |
| 2 | `game.php` | `matches.result` accepts strings | DB is ENUM `match_result` | **MISMATCH** — if code sends value not in enum, INSERT/UPDATE fails silently |
| 3 | `game.php` | `matches.time_control` accepts strings | DB is ENUM `time_control` | **MISMATCH** — custom time controls or typos cause hard failures |
| 4 | `friends.php` | `profiles.numeric_id` (used in tournament check) | **DOES NOT EXIST** | **MISMATCH**`check_opponent_timeout` references `pr.numeric_id` which doesn't exist on profiles |
| 5 | `reward_config` | `chess_win_xp`, `chess_loss_xp` | **NOT IN TABLE** | **MISMATCH**`complete_match` RPC falls back to 5 XP for chess (no xp rows exist for chess) |
| 6 | `reward_config` | `chess_2nd_coins`, `chess_3rd_coins` | **NOT IN TABLE** | **MISMATCH** — chess is 1v1, so "2nd place" keys don't exist; RPC falls back to 5 coins for loss |
### RPC Mismatches — Code Calls vs Function Signature
| # | Code Calls | RPC Exists | Issue |
|---|---|---|---|
| 1 | `complete_match(p_match_id, p_game_key, p_winners, p_reason)` | Yes | OK — but `p_winners` comes from untrusted client (WTF #4 in backend section) |
| 2 | `merge_game_state(p_table, p_match_id, p_patch)` | Yes | OK — atomic merge, handles all 3 tables |
| 3 | `match_heartbeat(p_match_id, p_player_id, p_game_key)` | Yes | OK |
| 4 | `can_player_queue(p_player_id)` | Yes | OK — but called with service key so anyone can check anyone |
| 5 | `log_mp_event(...)` | Yes | OK |
| 6 | `enforce_turn_timeout(...)` | Yes | OK |
| 7 | `award_coins(p_player_id, p_amount, p_reason)` | Yes | OK — called inside `complete_match` |
| 8 | `check_opponent_timeout(p_bracket_id, p_user_numeric_id)` | Yes | **MISMATCH** — references `profiles.numeric_id` which DOESN'T EXIST |
### Index Gaps — Missing Performance Indexes
| # | Table | Missing Index | Why It Matters |
|---|---|---|---|
| 1 | `matches` | No index on `(white_player_id)` or `(black_player_id)` | `find-active-match` does full scan on player lookups |
| 2 | `matches` | No index on `status` | Filtering `in_progress` matches requires full scan |
| 3 | `matches` | No index on `(game_key, status)` | Game-specific queries scan all matches |
| 4 | `matchmaking_queue` | No index on `(game_key, time_control, status)` | Matchmaking opponent search is a full scan |
| 5 | `matchmaking_queue` | No index on `player_id` | `leaveQueue` per-player lookup is a scan |
| 6 | `notifications` | No index on `(user_id, is_read)` | Unread count query scans all user notifications |
| 7 | `friendships` | No index on `addressee_id` | Finding pending requests TO a user scans all friendships |
### Enum Completeness
**`match_result` enum values:**
```
white_wins, black_wins, draw, white_timeout, black_timeout,
white_resign, black_resign, white_abandon, black_abandon,
stalemate, insufficient_material, threefold_repetition,
fifty_moves, mutual_draw, aborted
```
**What code sends:**
- `game.php:handleResign``white_resign` / `black_resign` — OK
- `game.php:handleComplete` → whatever client sends in `result` param — **NO VALIDATION** against enum
- `game.php:handleDraw``mutual_draw` — OK
- `matchmaking.php` → creates with no result (null) — OK (nullable)
**`time_control` enum values:**
```
bullet_1_0, bullet_1_1, bullet_2_1, blitz_3_0, blitz_3_2,
blitz_5_0, blitz_5_3, rapid_10_0, rapid_10_5, rapid_15_10,
rapid_30_0, classical_60_0, classical_90_30, custom
```
**What code sends:**
- `challenge.js` default: `bullet_1_0` — OK
- `friends.js` invite: `blitz_3_0` — OK
- `time-select.js` offers all common ones — OK
- `matchmaking.php` passes client-provided value — **NO VALIDATION** (non-enum value = hard DB error)
### `complete_match` RPC — Security Hole Confirmed
The RPC trusts `p_winners` array completely:
```sql
v_player_id := (p_winners->>v_placement)::uuid;
PERFORM award_coins(v_player_id, v_coins, ...);
UPDATE profiles SET total_wins = total_wins + 1 WHERE id = v_player_id;
```
An attacker can:
1. Call `game.php?action=complete` with `winners: [their_own_uuid]` and any `match_id`
2. Get 50 coins + 1 win + XP credited to their account
3. Repeat indefinitely for infinite coins
**The RPC doesn't verify the player is actually in the match.**
### `merge_game_state` — Emote/Rematch Corruption CONFIRMED
Code in `multiplayer.js` sends emotes/rematch via `action: 'move'` which calls `merge_game_state`. The RPC does:
```sql
SET game_state = COALESCE(game_state, '{}'::jsonb) || p_patch
```
This is a **shallow merge** (`||` operator). If emote payload has any key that overlaps with real game state (e.g., if both have `last_move_at`), the emote overwrites it. But since emotes use their own keys (`emote`, `rematch_request`), in practice it's safe for chess... **UNLESS** `handleDraw` in `game.php:198-204` does a raw PATCH instead of using `merge_game_state`:
```php
// game.php line 199 - REPLACES entire game_state
$sdb->from('matches')->update(['game_state' => json_encode(['draw_accepted' => true])])->eq('id', $matchId)->execute();
```
This **does not merge** — it REPLACES. So `handleDraw` destroys connection heartbeats, emotes, everything.
### `match_heartbeat` — Connection Tracking Lives in `game_state`
The heartbeat RPC stores connection status inside `game_state.connections`. This means:
- Every heartbeat writes to `game_state`
- Every move also writes to `game_state`
- `handleDraw` obliterates `game_state` (including connections)
- After a draw acceptance, heartbeat data is gone → other player appears "abandoned"
### Missing `reward_config` Rows
| Key Pattern | Exists | Impact |
|---|---|---|
| `chess_win_coins` | Yes (50) | OK |
| `chess_loss_coins` | Yes (10) | OK |
| `chess_draw_coins` | Yes (20) | OK — but RPC never uses it (draw = placement 1 of 2) |
| `chess_win_xp` | **NO** | Falls back to 5 XP |
| `chess_loss_xp` | **NO** | Falls back to 5 XP |
| `ludo_win_coins` | Yes (40) | OK |
| `ludo_loss_coins` | **NO** | Falls back to 5 coins (but `ludo_4th_coins` = 5 covers it) |
| `domino_win_coins` | Yes (40) | OK |
| `domino_loss_coins` | Yes (10) | OK |
| `backgammon_*` | **ALL MISSING** | Falls back to 5 coins / 5 XP for everything |
---
## SCHEMA MISMATCH SCORE
| Category | Count | Severity |
|----------|-------|----------|
| Missing auth checks (service key bypasses RLS) | 3 | CRITICAL |
| Missing DB columns referenced by code | 1 | CRITICAL |
| Missing reward_config rows | 6 | MAJOR |
| Missing indexes (performance) | 7 | MAJOR |
| Enum validation missing in code | 2 | MAJOR |
| Draw handler destroys game_state | 1 | CRITICAL |
| complete_match RPC trusts client winners | 1 | CRITICAL |
| **Total mismatches** | **21** | |
### ZERO-MISMATCH FIXES NEEDED
1. **Add auth checks to game.php** — verify `$userId` is white_player_id or black_player_id before any mutation
2. **Add `chess_win_xp`, `chess_loss_xp` to `reward_config`** — currently all chess XP is hardcoded fallback 5
3. **Add all `backgammon_*` reward rows** — backgammon currently awards 5/5 for everything
4. **Fix `handleDraw`** — use `merge_game_state` RPC instead of raw PATCH that obliterates game_state
5. **Add index on `matches(white_player_id)`, `matches(black_player_id)`, `matches(status)`**
6. **Add index on `matchmaking_queue(game_key, time_control, status)`**
7. **Add index on `notifications(user_id, is_read)`**
8. **Add index on `friendships(addressee_id)`**
9. **Validate `time_control` and `result` values before DB writes** — reject non-enum strings
10. **Remove/fix `check_opponent_timeout`** — it references non-existent `profiles.numeric_id`
11. **Validate `p_winners` in `complete_match` RPC** — verify each UUID is actually a player in the match
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment