Commit 2710cab1 authored by Mahmoud Aglan's avatar Mahmoud Aglan

fix: restore spectating, and make "watch this player" watch the right player

Two problems, one of them mine.

Mine: locking api/game.php's `get` to the two participants broke spectating
outright. Watching a live board is a normal part of a chess event — the profile
"watch" button and the tournament live board both rely on it — and on
championship day it is what most people will be doing. Non-participants now get a
spectator projection: position, clocks, move list, players and result, but no
`my_color` and none of the private game_state (draw offers, heartbeats).

Pre-existing: handleFindActiveMatch ignored the player_id it was given and always
looked up the *caller's* match, so clicking "watch" on someone else silently
opened your own game. It now honours the requested player. The board itself still
goes through `get`, which decides what a spectator may see.

Both lookups are also bounded to the last 12 hours. Production carries 27 matches
stuck in a non-final state since May and July; without that bound, "watch" would
open a game that ended three months ago.

Test-harness hardening in the same commit: pgshim silently ignored any PostgREST
filter it did not implement, so a query with `gte.` matched every row and the test
passed while production filtered correctly. It now implements gte/lte and throws
on anything unrecognised, rather than quietly matching everything.
Co-Authored-By: 's avatarClaude Opus 5 (1M context) <noreply@anthropic.com>
parent fcc74b9f
...@@ -57,10 +57,21 @@ function handleGet($db, string $userId, array $input): void { ...@@ -57,10 +57,21 @@ function handleGet($db, string $userId, array $input): void {
$match = is_array($matches) && !empty($matches) && !isset($matches['error']) ? $matches[0] : null; $match = is_array($matches) && !empty($matches) && !isset($matches['error']) ? $matches[0] : null;
if (!$match) jsonError('Match not found', 404); if (!$match) jsonError('Match not found', 404);
// Only the two players see the live position. Anyone else would be able to
// read a tournament game's FEN while it is still being played.
$isPlayer = ($match['white_player_id'] === $userId || $match['black_player_id'] === $userId); $isPlayer = ($match['white_player_id'] === $userId || $match['black_player_id'] === $userId);
if (!$isPlayer) jsonError('Not authorized for this match', 403);
if ($match['status'] === 'in_progress') {
// Charge the clock of whoever is on move before reporting state, so a
// player who closed the tab still runs out of time like anyone else.
$match = chargeClockAndMaybeFlag($sdb, $match);
}
// Spectators are a normal part of a chess event — the live board is meant to
// be watchable. They get the position and the clocks, but not the private
// side of game_state (draw offers, heartbeats), and no `my_color`, which
// would be meaningless for them.
if (!$isPlayer) {
jsonResponse(spectatorView($match));
}
// `my_color` is the single authoritative answer to "which side am I?". // `my_color` is the single authoritative answer to "which side am I?".
// Every client reads this instead of guessing, which is what allowed two // Every client reads this instead of guessing, which is what allowed two
...@@ -70,13 +81,30 @@ function handleGet($db, string $userId, array $input): void { ...@@ -70,13 +81,30 @@ function handleGet($db, string $userId, array $input): void {
? $match['black_player_id'] ? $match['black_player_id']
: $match['white_player_id']; : $match['white_player_id'];
if ($match['status'] === 'in_progress') { jsonResponse($match);
// Charge the clock of whoever is on move before reporting state, so a }
// player who closed the tab still runs out of time like anyone else.
$match = chargeClockAndMaybeFlag($sdb, $match); /** The public face of a live game: enough to watch it, nothing private. */
function spectatorView(array $match): array {
$public = [];
foreach ([
'id', 'game_key', 'white_player_id', 'black_player_id', 'match_type',
'tournament_id', 'tournament_round', 'status', 'result', 'time_control',
'initial_time_ms', 'increment_ms', 'white_time_remaining_ms', 'black_time_remaining_ms',
'starting_fen', 'current_fen', 'pgn', 'moves', 'move_count',
'started_at', 'completed_at',
] as $field) {
if (array_key_exists($field, $match)) $public[$field] = $match[$field];
} }
jsonResponse($match); // Only the parts of the metadata a spectator needs to render an outcome.
$metadata = normaliseJsonObject($match['metadata'] ?? null);
$public['metadata'] = [
'winner' => $metadata['winner'] ?? null,
'end_reason' => $metadata['end_reason'] ?? null,
];
$public['spectating'] = true;
return $public;
} }
/** /**
...@@ -830,16 +858,30 @@ function handleChessLeave(string $userId, array $input): void { ...@@ -830,16 +858,30 @@ function handleChessLeave(string $userId, array $input): void {
jsonResponse(['success' => true, 'bot_replaced' => true]); jsonResponse(['success' => true, 'bot_replaced' => true]);
} }
/**
* Find a player's current game, so it can be watched.
*
* Callers pass the player they want to watch — the tournament live board and a
* profile's "watch" button both do. The previous version ignored that and always
* used the caller's own id, so watching someone else silently opened *your own*
* game instead. Only the match id and its participants are returned; the board
* itself still goes through handleGet, which decides what a spectator may see.
*/
function handleFindActiveMatch(string $userId, array $input): void { function handleFindActiveMatch(string $userId, array $input): void {
// Only allow finding your own active matches (prevent info disclosure) $playerId = $input['player_id'] ?? $userId;
$playerId = $userId; if (!preg_match('/^[0-9a-f-]{36}$/i', $playerId)) $playerId = $userId;
$sdb = supabaseService(); $sdb = supabaseService();
// Bounded by recency. Production carries matches stuck in_progress since May;
// without this, "watch" would open a game that ended three months ago.
$since = gmdate('c', time() - 12 * 3600);
// Check chess matches // Check chess matches
$chess = $sdb->get('matches', [ $chess = $sdb->get('matches', [
'or' => "(white_player_id.eq.{$playerId},black_player_id.eq.{$playerId})", 'or' => "(white_player_id.eq.{$playerId},black_player_id.eq.{$playerId})",
'status' => 'eq.in_progress', 'status' => 'eq.in_progress',
'started_at' => 'gte.' . $since,
'select' => 'id,game_key,white_player_id,black_player_id', 'select' => 'id,game_key,white_player_id,black_player_id',
'order' => 'started_at.desc', 'order' => 'started_at.desc',
'limit' => 1 'limit' => 1
...@@ -856,6 +898,7 @@ function handleFindActiveMatch(string $userId, array $input): void { ...@@ -856,6 +898,7 @@ function handleFindActiveMatch(string $userId, array $input): void {
// Check ludo matches // Check ludo matches
$ludo = $sdb->get('ludo_matches', [ $ludo = $sdb->get('ludo_matches', [
'status' => 'eq.in_progress', 'status' => 'eq.in_progress',
'started_at' => 'gte.' . $since,
'select' => 'id,players', 'select' => 'id,players',
'order' => 'started_at.desc', 'order' => 'started_at.desc',
'limit' => 10 'limit' => 10
...@@ -875,6 +918,7 @@ function handleFindActiveMatch(string $userId, array $input): void { ...@@ -875,6 +918,7 @@ function handleFindActiveMatch(string $userId, array $input): void {
$domino = $sdb->get('domino_matches', [ $domino = $sdb->get('domino_matches', [
'or' => "(player1_id.eq.{$playerId},player2_id.eq.{$playerId})", 'or' => "(player1_id.eq.{$playerId},player2_id.eq.{$playerId})",
'status' => 'eq.in_progress', 'status' => 'eq.in_progress',
'created_at' => 'gte.' . $since,
'select' => 'id', 'select' => 'id',
'order' => 'created_at.desc', 'order' => 'created_at.desc',
'limit' => 1 'limit' => 1
......
...@@ -49,6 +49,10 @@ class PgShim extends SupabaseClient { ...@@ -49,6 +49,10 @@ class PgShim extends SupabaseClient {
if (!$items) { $clauses[] = 'false'; continue; } if (!$items) { $clauses[] = 'false'; continue; }
$clauses[] = "\"$col\" IN (" . implode(',', array_fill(0, count($items), '?')) . ')'; $clauses[] = "\"$col\" IN (" . implode(',', array_fill(0, count($items), '?')) . ')';
foreach ($items as $i) $vals[] = trim($i, '"'); foreach ($items as $i) $vals[] = trim($i, '"');
} elseif (preg_match('/^lte\.(.*)$/s', $expr, $m)) {
$clauses[] = "\"$col\" <= ?"; $vals[] = $m[1];
} elseif (preg_match('/^gte\.(.*)$/s', $expr, $m)) {
$clauses[] = "\"$col\" >= ?"; $vals[] = $m[1];
} elseif (preg_match('/^lt\.(.*)$/s', $expr, $m)) { } elseif (preg_match('/^lt\.(.*)$/s', $expr, $m)) {
$clauses[] = "\"$col\" < ?"; $vals[] = $m[1]; $clauses[] = "\"$col\" < ?"; $vals[] = $m[1];
} elseif (preg_match('/^gt\.(.*)$/s', $expr, $m)) { } elseif (preg_match('/^gt\.(.*)$/s', $expr, $m)) {
...@@ -67,6 +71,11 @@ class PgShim extends SupabaseClient { ...@@ -67,6 +71,11 @@ class PgShim extends SupabaseClient {
} }
} }
if ($parts) $clauses[] = '(' . implode(' OR ', $parts) . ')'; if ($parts) $clauses[] = '(' . implode(' OR ', $parts) . ')';
} else {
// Never ignore a filter we do not understand. Silently dropping
// one makes a query match far more rows than production would,
// and the test then passes while the real behaviour differs.
throw new RuntimeException("PgShim: unsupported PostgREST filter \"$col=$expr\" — add it rather than letting the query match everything");
} }
} }
return $clauses ? ('WHERE ' . implode(' AND ', $clauses)) : ''; return $clauses ? ('WHERE ' . implode(' AND ', $clauses)) : '';
......
...@@ -75,19 +75,41 @@ async function teardown() { ...@@ -75,19 +75,41 @@ async function teardown() {
} }
console.log(` removed ${created.matches.length} standalone match(es)`); console.log(` removed ${created.matches.length} standalone match(es)`);
// Profiles go last: everything above holds a foreign key to them. The delete
// is then confirmed rather than assumed — an earlier version fired and forgot,
// and left two accounts behind without saying so.
for (const u of created.users) { for (const u of created.users) {
await sb(`rating_history?player_id=eq.${u.id}`, { method: 'DELETE' }); for (const path of [
await sb(`matchmaking_queue?player_id=eq.${u.id}`, { method: 'DELETE' }); `rating_history?player_id=eq.${u.id}`,
await sb(`mp_log?player_id=eq.${u.id}`, { method: 'DELETE' }); `matchmaking_queue?player_id=eq.${u.id}`,
await sb(`matches?white_player_id=eq.${u.id}`, { method: 'DELETE' }); `mp_log?player_id=eq.${u.id}`,
await sb(`matches?black_player_id=eq.${u.id}`, { method: 'DELETE' }); `player_achievements?player_id=eq.${u.id}`,
await sb(`player_achievements?player_id=eq.${u.id}`, { method: 'DELETE' }); `tournament_registrations?player_id=eq.${u.id}`,
await sb(`profiles?id=eq.${u.id}`, { method: 'DELETE' }); `matches?white_player_id=eq.${u.id}`,
`matches?black_player_id=eq.${u.id}`,
`el3ab_tournaments?created_by=eq.${u.id}`,
]) await sb(path, { method: 'DELETE' });
}
let stuck = [];
for (const u of created.users) {
for (let attempt = 0; attempt < 3; attempt++) {
await sb(`profiles?id=eq.${u.id}`, { method: 'DELETE' });
const left = await sb(`profiles?id=eq.${u.id}&select=id`);
if (!Array.isArray(left) || left.length === 0) break;
if (attempt === 2) stuck.push(u.id);
await new Promise(r => setTimeout(r, 400));
}
await fetch(`${SB}/auth/v1/admin/users/${u.id}`, { await fetch(`${SB}/auth/v1/admin/users/${u.id}`, {
method: 'DELETE', headers: { apikey: SK, Authorization: `Bearer ${SK}` }, method: 'DELETE', headers: { apikey: SK, Authorization: `Bearer ${SK}` },
}); });
} }
console.log(` removed ${created.users.length} throwaway account(s)`);
console.log(` removed ${created.users.length - stuck.length} of ${created.users.length} throwaway account(s)`);
if (stuck.length) {
failures.push('teardown left accounts behind');
console.log(` COULD NOT REMOVE: ${stuck.join(', ')} — delete these by hand`);
}
} }
// --------------------------------------------------------------------------- // ---------------------------------------------------------------------------
......
...@@ -103,7 +103,10 @@ check('black is told my_color=b', g.data.my_color === 'b', JSON.stringify(g.data ...@@ -103,7 +103,10 @@ check('black is told my_color=b', g.data.my_color === 'b', JSON.stringify(g.data
check('opponent_id is supplied', g.data.opponent_id === white, g.data.opponent_id); check('opponent_id is supplied', g.data.opponent_id === white, g.data.opponent_id);
g = await call('game.php', { action: 'get', match_id: matchId }, P[2]); g = await call('game.php', { action: 'get', match_id: matchId }, P[2]);
check('a non-participant cannot read the live game', g.status === 403, `status ${g.status}`); check('a spectator can watch the live board', g.status === 200 && g.data.spectating === true, `status ${g.status}`);
check('a spectator sees the position and clocks', !!g.data.current_fen && g.data.white_time_remaining_ms != null);
check('a spectator is not told a colour of their own', g.data.my_color === undefined, String(g.data.my_color));
check('a spectator does not receive private game state', g.data.game_state === undefined);
// --------------------------------------------------------------------------- // ---------------------------------------------------------------------------
console.log('\n=== 3. turn ownership ==='); console.log('\n=== 3. turn ownership ===');
...@@ -472,6 +475,34 @@ await call('game.php', { action: 'resign', match_id: botMatch }, P[2]); ...@@ -472,6 +475,34 @@ await call('game.php', { action: 'resign', match_id: botMatch }, P[2]);
check('a bot game does not change rating', check('a bot game does not change rating',
Number(sql(`SELECT elo_blitz FROM profiles WHERE id='${P[2]}'`)) === beforeElo, 'rating moved'); Number(sql(`SELECT elo_blitz FROM profiles WHERE id='${P[2]}'`)) === beforeElo, 'rating moved');
console.log('\n=== 20. spectating, and finding whose game to watch ===');
sql(`DELETE FROM matchmaking_queue;`);
await call('matchmaking.php', { action: 'queue', game_key: 'chess', time_control: 'rapid_10_0' }, P[0]);
const sm = (await call('matchmaking.php', { action: 'queue', game_key: 'chess', time_control: 'rapid_10_0' }, P[1])).data;
// A draw offer and a heartbeat must not leak to someone watching.
sql(`UPDATE matches SET game_state = '{"draw_offer":"${P[0]}","hb_${P[0]}":123}'::jsonb WHERE id='${sm.match_id}'`);
const watcher = await call('game.php', { action: 'get', match_id: sm.match_id }, P[2]);
check('a standing draw offer is not visible to a spectator',
JSON.stringify(watcher.data).indexOf('draw_offer') === -1, JSON.stringify(watcher.data).slice(0, 160));
// find-active-match must answer about the player asked for, not the caller.
const found = await call('game.php', { action: 'find-active-match', player_id: P[0] }, P[2]);
check('watching another player finds THEIR game, not your own',
found.data.match_id === sm.match_id, `${found.data.match_id} vs ${sm.match_id}`);
// A months-old stuck match must not be offered as something to watch. Age every
// live game this player has, since earlier sections left them in others.
sql(`UPDATE matches SET started_at = now() - interval '90 days'
WHERE status='in_progress' AND (white_player_id='${P[0]}' OR black_player_id='${P[0]}')`);
const oldOne = await call('game.php', { action: 'find-active-match', player_id: P[0] }, P[2]);
check('a long-stale match is not offered for spectating', !oldOne.data.match_id, String(oldOne.data.match_id));
// …but a current one still is.
sql(`UPDATE matches SET started_at = now() WHERE id='${sm.match_id}'`);
const freshOne = await call('game.php', { action: 'find-active-match', player_id: P[0] }, P[2]);
check('a current match is still offered', freshOne.data.match_id === sm.match_id, String(freshOne.data.match_id));
// --------------------------------------------------------------------------- // ---------------------------------------------------------------------------
console.log(''); console.log('');
if (failures.length) { if (failures.length) {
......
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