Validate that placed tiles come from the player's own hand - #54
Conversation
There was a problem hiding this comment.
Pull request overview
Reintroduces server-side validation in makeTurn to ensure a player can only place tiles they actually possess in their hand (including correct handling of wildcard tiles), closing a cheating vector where a modified client could place arbitrary letters.
Changes:
- Adds a multiset-based “available letters” check to validate every placed tile is spendable from the player’s current hand.
- Removes the old commented-out (non-working) ownership-check block and related dead comment.
- Runs the new check after the wildcard-on-board swap reconciliation so legitimate swaps remain valid.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| row.forEach((cell) => { | ||
| if (!cell) { | ||
| return; | ||
| } | ||
| let neededLetter = null; | ||
| if (typeof cell === "string") { | ||
| if (cell[0] === "*" && cell.length === 2) { | ||
| neededLetter = "*"; | ||
| } else if (cell[0] !== "*" && cell.length === 1) { | ||
| neededLetter = cell; | ||
| } | ||
| } | ||
| if (neededLetter !== null && availableLetters[neededLetter]) { | ||
| availableLetters[neededLetter] -= 1; | ||
| } else { | ||
| ok = false; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Both good points, thanks.
-
Falsy-cell bypass: fixed. The empty test now uses
cell === null(and the top-level pass check usesletter !== null), so a non-null falsy value (0,false,"") is no longer skipped/treated as a pass; it falls through to the type/length checks and is rejected. This matches hownewBoardtreatsuserBoard[y][x] !== nullas a placement, so nothing non-null can slip onto the board unvalidated. -
Wildcard represented-letter (
"*$"): left as-is on purpose. That check is about the represented letter's legality (does it belong to the alphabet), not about tile ownership, which is this PR's scope: the player does own a*. Words in this game aren't dictionary-validated server-side anyway (they're approved by the other players), so a wildcard representing a non-letter has no scoring advantage. Worth a separate follow-up for input hygiene, but out of scope here to keep this PR atomic.
a972fbd to
97c86d1
Compare
| // check that every placed letter comes from the user's hand, | ||
| // counting duplicates: a wildcard cell ("*x") consumes a "*" | ||
| // from the hand, any other cell consumes the letter itself | ||
| if (ok) { |
There was a problem hiding this comment.
Fixed. The swap loop no longer mutates on an invalid swap: it computes isValidSwap = index !== -1 && typeof boardCell === 'string' && boardCell[0] === '*' && boardCell[1] === letter, and on !isValidSwap it sets ok = false and returns before any splice/board write. That removes both the splice(-1, 1, '*') hand corruption and the currentGameBoard[y][x][1] dereference on a non-wildcard/empty target cell. Legitimate swaps are unchanged.
| // a placement is any non-null cell (matching newBoard/ownership below), | ||
| // so a board of only non-null junk is not mistaken for a pass | ||
| if (!userBoard.some((row) => row.some((letter) => letter !== null))) { |
There was a problem hiding this comment.
Fixed. validateRequestBody now enforces the exact grid: userBoard must be 15 rows, each an array of 15 cells, each cell null or a string, else 400. That closes the ragged/short-board path where newBoard read missing cells as undefined (or threw on a missing row). Confirmed against the client, which sends a 15x15 grid with empty cells converted from '' to null before submit, so legit turns and all-null passes still validate.
97c86d1 to
ac69fbf
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
services/turn.ts:156
- If
userBoardis treated as containingnullcells (and is validated that way), this explicit callback parameter type should also allownull; otherwise it can become a TypeScript type error / hide null-handling in this check.
!userBoard.some((row, y) =>
row.some(
(letter: string, x) =>
letter !== null && game.board[y][x] !== null
| @@ -28,6 +28,8 @@ interface RequestBody { | |||
| wildCardOnBoard: { [x: string]: { [x: string]: string } }; | |||
| } | |||
|
|
|||
| const BOARD_SIZE = 15; | |||
There was a problem hiding this comment.
Fixed. RequestBody.userBoard is now typed (string | null)[][], matching that empty cells are null after validation.
| // a placement is any non-null cell (matching newBoard/ownership below), | ||
| // so a board of only non-null junk is not mistaken for a pass | ||
| if (!userBoard.some((row) => row.some((letter) => letter !== null))) { |
There was a problem hiding this comment.
Fixed. makeTurn's userBoard parameter (and the empty-cell callback annotation) are now (string | null)[][], so the type matches the runtime contract the validator enforces. Type-only change, no behaviour impact.
ac69fbf to
736b56e
Compare
| res: Response, | ||
| next: NextFunction | ||
| ) { | ||
| const body = req.body as RequestBody; |
There was a problem hiding this comment.
Fixed. validateRequestBody now rejects a non-object body up front: if (!req.body || typeof req.body !== 'object' || Array.isArray(req.body)) return res.status(400)..., so a JSON null (or array/primitive) body gives a controlled 400 instead of throwing on the property access.
| } | ||
| } | ||
|
|
||
| if ("wildCardOnBoard" in body) { |
There was a problem hiding this comment.
Fixed. When wildCardOnBoard is absent it now defaults to {} in the validator (and RequestBody.wildCardOnBoard is typed optional to match), so makeTurn's Object.keys(wildCardOnBoard) never receives undefined. A null value still returns a controlled 400.
736b56e to
9c45cd7
Compare
| } | ||
| } | ||
|
|
||
| if ("wildCardOnBoard" in body) { |
There was a problem hiding this comment.
Fixed. validateRequestBody now rejects arrays explicitly at both levels (Array.isArray(body.wildCardOnBoard) and Array.isArray(innerObj)), returning 400, consistent with the non-object body rejection. So an array can't reach makeTurn's Object.keys/index logic. (The swap guard already rejected such input via isValidSwap, but rejecting at the boundary is cleaner.)
9c45cd7 to
049ce70
Compare
Previously, the check that a turn only used letters from the player's
hand was commented out with a TODO, so a modified client could place
any tiles (letters not held, or more copies than held) and keep its
real hand; the downstream subtract silently retained the unmatched
letters.
After this change, makeTurn counts the player's post-swap hand and
requires every placed cell to be backed by a held tile: a wildcard
cell ("*x") consumes a "*", any other cell consumes that letter, and
duplicates are counted, so over-spending or placing unheld tiles is
rejected.
049ce70 to
bf256bc
Compare
Problem
In
makeTurnthe check that a turn only used tiles from the player's own hand was commented out, with a TODO saying it didn't work because of board wildcards. So a modified client could place tiles it didn't hold — letters not in hand, or more copies of a letter than it held — and the downstreamsubtract(userLetters, putLetters)would silently keep the unmatched tiles, letting a player score with tiles they never had and keep their real hand.Fix
Reinstated the ownership check in
services/turn.ts, running after the existing wildcard-on-board swap loop (which has already adjusteduserLetters, splicing out a real letter and inserting a*for each legitimate swap):"*x", 2 chars) consumes a*; a real-letter cell (1 char) consumes that letter; anything else (malformed) is rejectedDuplicates are counted, so over-spending a letter is caught. This is consistent with the downstream
subtract, which removes the same placed tiles from the hand. Legitimate turns, including legitimate wildcard swaps, are unaffected. Also removed the dead commented-out block and a stray closing-brace comment.