Hide other players' letters and the bag from game payloads - #53
Conversation
There was a problem hiding this comment.
Pull request overview
This PR prevents game-state payloads from leaking hidden information (other players’ hands and the exact bag/draw order) by sanitizing letters per recipient before sending GAME_UPDATED updates or serving GET /game/:id.
Changes:
- Added
sanitizeGame(game, userId)to remove other players’ hands and maskletters.potwhile preserving pot length. - Added
emitGameUpdated(server, gameId, game)to emit per-socket sanitizedGAME_UPDATEDmessages in the game room. - Updated game routes and socket login flow to use sanitized game payloads (including optional auth for
GET /game/:id).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| services/sanitizeGame.ts | Introduces per-recipient game sanitization and per-socket GAME_UPDATED emission. |
| routers/game.ts | Switches game update broadcasts and GET /game/:id to send sanitized game payloads (with optional auth parsing). |
| handlers.ts | Sanitizes the initial game payload sent to a socket after successful login. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2693a3a to
6e6b14a
Compare
6e6b14a to
c787f47
Compare
c787f47 to
4eb28e1
Compare
4eb28e1 to
4c7f944
Compare
| * players' letters are removed. Anonymous recipients get no hand. | ||
| */ | ||
| export const sanitizeGame = (game: Game, userId: number | null) => { | ||
| const json = game.toJSON(); |
There was a problem hiding this comment.
It converts the Sequelize model instance into a plain object. We need that to safely spread it ({ ...json, letters, ... }) and read fields like json.letters / json.turnOrder: spreading a Sequelize instance directly doesn't expose the column values (they live behind getters / dataValues), so without toJSON() the rebuilt object would be missing the game data.
There was a problem hiding this comment.
I still don't understand why we didn't need this before. Also, does it mean that this JSON object is any? Can we type it more strictly?
There was a problem hiding this comment.
Why it wasn't needed before: previously we passed the raw Sequelize game instance straight into socket.emit/res.send, and the JSON serialization that socket.io/Express do calls the instance's toJSON() automatically on the way out - so the plain-object conversion always happened, just implicitly at send time. Now we rebuild the object server-side to mask the letters, so we need a plain, spreadable object up front, hence the explicit toJSON().
On typing: you're right, it was effectively any. Fixed in 428a3c2 - it's now cast to a GameJson interface where the fields we read (letters, turnOrder, turn, previousLetters, putLetters) are typed and a string index signature carries the rest of the game fields through the spread. sourceLetters is typed { [key: string]: string[] } too.
There was a problem hiding this comment.
what's the difference between GameJson and Sequelize game type?
There was a problem hiding this comment.
Agreed, and both done in d8b9661.
Difference between GameJson and the Sequelize Game type: Game is the model instance type - it carries the Sequelize machinery (update, save, toJSON, dataValues, association setters). game.toJSON() returns only the plain column values, no methods, so typing that result as Game would be a lie (you can't call .update() on it), and we rebuild it into a fresh plain object anyway.
Stricter: you're right that my hand-written interface was too loose - the [key: string]: unknown index signature meant any typo or unknown field passed silently. It's now derived from the model:
type GameJson = InferAttributes<Game>;
type GameLetters = GameJson["letters"];InferAttributes (sequelize 6.37) gives exactly the model's data attributes and drops the methods, so there's a single source of truth and no index signature - json.somethingUnknown is now a compile error. letters uses the model's own type too, so the hand is keyed by the numeric user id like the model declares (same JSON on the wire).
Comment above the type removed.
Previously, every GAME_UPDATED emit and the unauthenticated GET /game/:id returned the full Game row, including all players' hands, the pot contents and its draw order, so anyone could cheat by inspecting the payload. After this change, game payloads are sanitized per recipient: each player receives only their own hand, the pot is masked to an array of the same length (the client only uses its size), and previousLetters/putLetters are included only for the player who made the current turn. GET /game/:id supports optional authentication so logged-in players still receive their hand; anonymous spectators get none.
4c7f944 to
4e49989
Compare
Addresses review: apply the shared getBearerToken helper to the other places that parsed the Authorization header inline, so the confirm-email and profile routes no longer duplicate the Bearer parsing.
Addresses review: game.toJSON() was implicitly any. Type it as a GameJson interface - the fields sanitizeGame reads (letters, turnOrder, turn, previousLetters, putLetters) are typed, with an index signature carrying the remaining game fields through the spread.
Addresses review: GameJson was hand-written with an index signature, so unknown fields passed silently. It is now InferAttributes<Game>, derived from the model, and the letters object uses the model's own type.
Problem
Every
GAME_UPDATEDemit and the unauthenticatedGET /game/:idreturned the fullGamerow, which includesletters— every player's hand plus thepot(the bag contents and its exact draw order). Any player (or any anonymous visitor hittingGET /game/:id) could read opponents' tiles and the upcoming draw order straight from the payload, making the game trivially cheatable.Fix
Added
services/sanitizeGame.ts:sanitizeGame(game, userId)returns a plain object whereletterscontains only the recipient's own hand; thepotis replaced by an array of the same length filled with""(the client only readsletters.pot.length);previousLetters/putLettersare included only for the player who made the turn currently on the board. Anonymous recipients (userId === null) get no hand.emitGameUpdated(server, gameId, game)iterates the game room and emitsGAME_UPDATEDsanitised per socket, using each socket'splayerId.Wired it into every place that previously broadcast or returned a raw game: the turn/approve/undo/change/start routes now call
emitGameUpdated,GET /game/:idsanitises with an optionally-authenticated user id (newgetRequestUserIdhelper, so logged-in players still get their hand while spectators get none), and the socket-login path inhandlers.tssanitises with the authenticated user's id.Client compatibility: the client reads
game.letters[user.id]for its own hand andgame.letters.pot.lengthfor the bag counter only, so the masked pot and per-recipient hand keep it working unchanged.