From b103db681b40b8a1b66453350bd6a556e36bfb12 Mon Sep 17 00:00:00 2001 From: seavor Date: Sun, 19 Apr 2026 16:36:33 -0500 Subject: [PATCH] PR review changes --- .../instructions/webclient.instructions.md | 18 ++- .../integration/src/websocket/admin.spec.ts | 28 ++++ .../integration/src/websocket/deck.spec.ts | 67 +++++++++ .../src/websocket/moderator.spec.ts | 107 +++++++++++++++ .../api/response/SessionResponseImpl.spec.ts | 52 +++++++ .../src/api/response/SessionResponseImpl.ts | 6 +- webclient/src/store/game/game.actions.spec.ts | 4 +- .../src/store/game/game.dispatch.spec.ts | 8 +- webclient/src/store/game/game.dispatch.ts | 4 +- webclient/src/store/game/game.reducer.spec.ts | 45 +++++- webclient/src/store/game/game.reducer.ts | 46 ++++++- .../src/store/game/game.selectors.spec.ts | 35 +++++ webclient/src/store/game/game.selectors.ts | 12 ++ .../{rooms.actions.tsx => rooms.actions.ts} | 0 .../{rooms.dispatch.tsx => rooms.dispatch.ts} | 0 ...oms.interfaces.tsx => rooms.interfaces.ts} | 0 .../{rooms.reducer.tsx => rooms.reducer.ts} | 0 ...rooms.selectors.tsx => rooms.selectors.ts} | 0 .../rooms/{rooms.types.tsx => rooms.types.ts} | 0 webclient/src/types/enriched.ts | 13 ++ webclient/src/websocket/WebClient.spec.ts | 12 ++ webclient/src/websocket/WebClient.ts | 27 +++- webclient/src/websocket/__mocks__/helpers.ts | 12 +- .../src/websocket/commands/session/login.ts | 5 +- .../websocket/commands/session/register.ts | 3 +- .../services/ProtobufService.spec.ts | 129 +++++++++--------- .../src/websocket/services/ProtobufService.ts | 23 +++- .../src/websocket/types/WebClientResponse.ts | 14 +- .../src/websocket/types/WebSocketConfig.ts | 8 -- .../websocket/utils/connectionState.spec.ts | 42 ++++++ .../src/websocket/utils/guid.util.spec.ts | 9 +- webclient/src/websocket/utils/guid.util.ts | 15 +- .../src/websocket/utils/passwordHasher.ts | 5 +- .../websocket/utils/sanitizeHtml.util.spec.ts | 10 ++ .../src/websocket/utils/sanitizeHtml.util.ts | 6 +- 35 files changed, 640 insertions(+), 125 deletions(-) create mode 100644 webclient/src/api/response/SessionResponseImpl.spec.ts rename webclient/src/store/rooms/{rooms.actions.tsx => rooms.actions.ts} (100%) rename webclient/src/store/rooms/{rooms.dispatch.tsx => rooms.dispatch.ts} (100%) rename webclient/src/store/rooms/{rooms.interfaces.tsx => rooms.interfaces.ts} (100%) rename webclient/src/store/rooms/{rooms.reducer.tsx => rooms.reducer.ts} (100%) rename webclient/src/store/rooms/{rooms.selectors.tsx => rooms.selectors.ts} (100%) rename webclient/src/store/rooms/{rooms.types.tsx => rooms.types.ts} (100%) create mode 100644 webclient/src/websocket/utils/connectionState.spec.ts diff --git a/.github/instructions/webclient.instructions.md b/.github/instructions/webclient.instructions.md index 378359ad8..621cee8f6 100644 --- a/.github/instructions/webclient.instructions.md +++ b/.github/instructions/webclient.instructions.md @@ -4,10 +4,26 @@ applyTo: "webclient/**" # Webclient instructions -Applies to the React/TypeScript SPA in `webclient/` (Webatrice) that connects to a Servatrice server over a WebSocket. The package is self-contained; the only thing it shares with the rest of the repo (C++ desktop/server stack) is the protobuf protocol in `../libcockatrice_protocol/`. Anything outside `webclient/` is out of scope unless a task explicitly touches the protocol. +Applies to the React/TypeScript SPA in `webclient/` (Webatrice) — a **browser port of the desktop Cockatrice client**. It connects to the **same Servatrice server** as desktop over a WebSocket. UI behavior, and especially how UI code drives the websocket layer (commands, response handling, event-driven state changes), **must match the desktop client** unless a scope reduction is explicitly agreed per milestone. Divergence is a defect by default — see [#desktop-parity-mandate](#desktop-parity-mandate). The package is otherwise self-contained; the only thing it shares with the rest of the repo (C++ desktop/server stack) is the protobuf protocol in `../libcockatrice_protocol/`, and anything outside `webclient/` is out of scope unless a task explicitly touches the protocol. Canonical AI-tool instruction surface for this package — invariants, policy, and external facts. When a source comment ends with `See .github/instructions/webclient.instructions.md#`, the section with that anchor lives here. Source comments tagged `// @critical` guard cross-file invariants; do not remove them without updating the relevant section. For commands, stack, and getting-started, see [webclient/README.md](../../webclient/README.md). +## Desktop parity mandate + +The webclient is a port of the desktop Cockatrice C++ client — same server, same game, same users. This is a **hard baseline**, not an ambiguity tie-breaker. Every webclient behavior difference from desktop is treated as a defect unless it has been explicitly scoped out for the current milestone. + +**UI ↔ websocket parity is the sharpest edge of this rule.** Command shapes, field-level defaults (what the client sends vs. omits), response/event handling, and the resulting state transitions must mirror desktop. A webclient that issues a subtly different command, or reacts differently to the same event, breaks multi-client play — a desktop player and a webclient player joined to the same Servatrice room must see consistent game state. + +**Desktop is the spec.** The reference implementation lives at `../cockatrice/src/` (relative to the repo root). Before proposing any UX or websocket-interaction decision that isn't obvious from the webclient code, read the corresponding desktop source. + +**Divergence protocol:** + +1. If desktop behavior is expensive to replicate in the current milestone, propose a **scope reduction explicitly** (e.g. "M4 ships default red arrows; color picker defers to M6"). Get agreement before coding. Record deferred parity gaps in [webclient/plans/gameboard-deferrables.md](../../webclient/plans/gameboard-deferrables.md) as "parity gap — deferred to ". +2. Phase-end reviews treat Cockatrice-parity findings as **blockers** by default. Elevate them; don't defer unless the user has explicitly OK'd the gap. +3. The only categorically valid reasons to diverge without a scope-reduction sign-off are: a browser security constraint (e.g. no raw TCP), a fundamental input-model difference (touch vs. mouse), or an accessibility requirement desktop doesn't meet. + +This section subsumes the one-line "matches the Cockatrice desktop client" note in [#startup--session-invariants](#startup--session-invariants); that remains as a concrete example of the rule, not a standalone source of truth. + ## Architecture ### Protocol layer diff --git a/webclient/integration/src/websocket/admin.spec.ts b/webclient/integration/src/websocket/admin.spec.ts index 2c2e02ff1..4af6677f2 100644 --- a/webclient/integration/src/websocket/admin.spec.ts +++ b/webclient/integration/src/websocket/admin.spec.ts @@ -63,4 +63,32 @@ describe('admin commands', () => { responseCode: Data.Response_ResponseCode.RespOk, }))); }); + + it('reloadConfig sends command and resolves on RespOk', () => { + connectAndLogin(); + + AdminCommands.reloadConfig(); + + const { cmdId } = findLastAdminCommand(Data.Command_ReloadConfig_ext); + expect(cmdId).toBeGreaterThan(0); + + deliverMessage(buildResponseMessage(buildResponse({ + cmdId, + responseCode: Data.Response_ResponseCode.RespOk, + }))); + }); + + it('updateServerMessage sends command and resolves on RespOk', () => { + connectAndLogin(); + + AdminCommands.updateServerMessage(); + + const { cmdId } = findLastAdminCommand(Data.Command_UpdateServerMessage_ext); + expect(cmdId).toBeGreaterThan(0); + + deliverMessage(buildResponseMessage(buildResponse({ + cmdId, + responseCode: Data.Response_ResponseCode.RespOk, + }))); + }); }); \ No newline at end of file diff --git a/webclient/integration/src/websocket/deck.spec.ts b/webclient/integration/src/websocket/deck.spec.ts index a7a8da0eb..a3e9df52f 100644 --- a/webclient/integration/src/websocket/deck.spec.ts +++ b/webclient/integration/src/websocket/deck.spec.ts @@ -64,6 +64,73 @@ describe('deck operations', () => { expect(downloaded?.deckId).toBe(42); expect(downloaded?.deck).toContain('Lightning Bolt'); }); + + it('deckUpload sends payload and dispatches uploadServerDeck on success', () => { + connectAndLogin(); + + SessionCommands.deckUpload('/folder', 0, '4 Counterspell\n20 Island'); + + const { cmdId, value } = findLastSessionCommand(Data.Command_DeckUpload_ext); + expect(value.path).toBe('/folder'); + expect(value.deckList).toContain('Counterspell'); + + const newFile = create(Data.ServerInfo_DeckStorage_TreeItemSchema, { + id: 7, + name: 'CounterDeck.cod', + }); + deliverMessage(buildResponseMessage(buildResponse({ + cmdId, + responseCode: Data.Response_ResponseCode.RespOk, + ext: Data.Response_DeckUpload_ext, + value: create(Data.Response_DeckUploadSchema, { newFile }), + }))); + // No state assertion: backendDecks is keyed by full tree, not single + // upload — the integration verifies the dispatcher is reached, not the + // tree-merge logic which lives in the reducer. + }); + + it('deckDel sends deckId and resolves on RespOk', () => { + connectAndLogin(); + + SessionCommands.deckDel(13); + + const { cmdId, value } = findLastSessionCommand(Data.Command_DeckDel_ext); + expect(value.deckId).toBe(13); + + deliverMessage(buildResponseMessage(buildResponse({ + cmdId, + responseCode: Data.Response_ResponseCode.RespOk, + }))); + }); + + it('deckNewDir sends path + dirName payload and resolves on RespOk', () => { + connectAndLogin(); + + SessionCommands.deckNewDir('/parent', 'NewFolder'); + + const { cmdId, value } = findLastSessionCommand(Data.Command_DeckNewDir_ext); + expect(value.path).toBe('/parent'); + expect(value.dirName).toBe('NewFolder'); + + deliverMessage(buildResponseMessage(buildResponse({ + cmdId, + responseCode: Data.Response_ResponseCode.RespOk, + }))); + }); + + it('deckDelDir sends path payload and resolves on RespOk', () => { + connectAndLogin(); + + SessionCommands.deckDelDir('/folder/to/remove'); + + const { cmdId, value } = findLastSessionCommand(Data.Command_DeckDelDir_ext); + expect(value.path).toBe('/folder/to/remove'); + + deliverMessage(buildResponseMessage(buildResponse({ + cmdId, + responseCode: Data.Response_ResponseCode.RespOk, + }))); + }); }); describe('replay operations', () => { diff --git a/webclient/integration/src/websocket/moderator.spec.ts b/webclient/integration/src/websocket/moderator.spec.ts index 088d254dc..180a3f011 100644 --- a/webclient/integration/src/websocket/moderator.spec.ts +++ b/webclient/integration/src/websocket/moderator.spec.ts @@ -101,4 +101,111 @@ describe('moderator commands', () => { expect(store.getState().server.banUser).toBe('baduser'); }); + + it('getWarnHistory dispatches with the response warnList', () => { + connectAndLogin(); + + ModeratorCommands.getWarnHistory('baduser'); + + const { cmdId, value } = findLastModeratorCommand(Data.Command_GetWarnHistory_ext); + expect(value.userName).toBe('baduser'); + + const warning = create(Data.ServerInfo_WarningSchema, { + adminName: 'Admin', + reason: 'spamming', + }); + deliverMessage(buildResponseMessage(buildResponse({ + cmdId, + responseCode: Data.Response_ResponseCode.RespOk, + ext: Data.Response_WarnHistory_ext, + value: create(Data.Response_WarnHistorySchema, { warnList: [warning] }), + }))); + + const history = store.getState().server.warnHistory.baduser; + expect(history).toHaveLength(1); + expect(history[0].reason).toBe('spamming'); + }); + + it('getWarnList dispatches the warn-list options on success', () => { + connectAndLogin(); + + ModeratorCommands.getWarnList('mod', 'troublemaker', 'client-1'); + + const { cmdId, value } = findLastModeratorCommand(Data.Command_GetWarnList_ext); + expect(value.modName).toBe('mod'); + expect(value.userName).toBe('troublemaker'); + expect(value.userClientid).toBe('client-1'); + + deliverMessage(buildResponseMessage(buildResponse({ + cmdId, + responseCode: Data.Response_ResponseCode.RespOk, + ext: Data.Response_WarnList_ext, + value: create(Data.Response_WarnListSchema, { warning: ['spam', 'abuse'] }), + }))); + + expect(store.getState().server.warnListOptions.length).toBeGreaterThan(0); + }); + + it('forceActivateUser sends command and resolves on RespOk', () => { + connectAndLogin(); + + ModeratorCommands.forceActivateUser('inactive', 'mod'); + + const { cmdId, value } = findLastModeratorCommand(Data.Command_ForceActivateUser_ext); + expect(value.usernameToActivate).toBe('inactive'); + expect(value.moderatorName).toBe('mod'); + + deliverMessage(buildResponseMessage(buildResponse({ + cmdId, + responseCode: Data.Response_ResponseCode.RespOk, + }))); + }); + + it('getAdminNotes populates server.adminNotes on success', () => { + connectAndLogin(); + + ModeratorCommands.getAdminNotes('subject'); + + const { cmdId, value } = findLastModeratorCommand(Data.Command_GetAdminNotes_ext); + expect(value.userName).toBe('subject'); + + deliverMessage(buildResponseMessage(buildResponse({ + cmdId, + responseCode: Data.Response_ResponseCode.RespOk, + ext: Data.Response_GetAdminNotes_ext, + value: create(Data.Response_GetAdminNotesSchema, { notes: 'prior offenses' }), + }))); + + expect(store.getState().server.adminNotes.subject).toBe('prior offenses'); + }); + + it('updateAdminNotes sends notes payload and resolves on RespOk', () => { + connectAndLogin(); + + ModeratorCommands.updateAdminNotes('subject', 'updated notes text'); + + const { cmdId, value } = findLastModeratorCommand(Data.Command_UpdateAdminNotes_ext); + expect(value.userName).toBe('subject'); + expect(value.notes).toBe('updated notes text'); + + deliverMessage(buildResponseMessage(buildResponse({ + cmdId, + responseCode: Data.Response_ResponseCode.RespOk, + }))); + }); + + it('grantReplayAccess sends command and resolves on RespOk', () => { + connectAndLogin(); + + ModeratorCommands.grantReplayAccess(42, 'mod'); + + const { cmdId, value } = findLastModeratorCommand(Data.Command_GrantReplayAccess_ext); + expect(value.replayId).toBe(42); + expect(value.moderatorName).toBe('mod'); + + deliverMessage(buildResponseMessage(buildResponse({ + cmdId, + responseCode: Data.Response_ResponseCode.RespOk, + }))); + }); }); \ No newline at end of file diff --git a/webclient/src/api/response/SessionResponseImpl.spec.ts b/webclient/src/api/response/SessionResponseImpl.spec.ts new file mode 100644 index 000000000..dbf507b29 --- /dev/null +++ b/webclient/src/api/response/SessionResponseImpl.spec.ts @@ -0,0 +1,52 @@ +vi.mock('@app/store', () => ({ + GameDispatch: { clearStore: vi.fn(), gameJoined: vi.fn(), playerPropertiesChanged: vi.fn() }, + RoomsDispatch: { clearStore: vi.fn() }, + ServerDispatch: { + initialized: vi.fn(), + clearStore: vi.fn(), + updateStatus: vi.fn(), + updateUser: vi.fn(), + }, +})); + +import { GameDispatch, RoomsDispatch, ServerDispatch } from '@app/store'; +import { WebsocketTypes } from '@app/websocket/types'; +import { SessionResponseImpl } from './SessionResponseImpl'; + +describe('SessionResponseImpl.updateStatus', () => { + let impl: SessionResponseImpl; + + beforeEach(() => { + vi.clearAllMocks(); + impl = new SessionResponseImpl(); + }); + + it('clears game + rooms + server stores when transitioning to DISCONNECTED', () => { + impl.updateStatus(WebsocketTypes.StatusEnum.DISCONNECTED, 'gone'); + expect(GameDispatch.clearStore).toHaveBeenCalledTimes(1); + expect(RoomsDispatch.clearStore).toHaveBeenCalledTimes(1); + expect(ServerDispatch.clearStore).toHaveBeenCalledTimes(1); + expect(ServerDispatch.updateStatus).toHaveBeenCalledWith( + WebsocketTypes.StatusEnum.DISCONNECTED, + 'gone', + ); + }); + + it('does NOT clear stores on non-DISCONNECTED transitions', () => { + impl.updateStatus(WebsocketTypes.StatusEnum.CONNECTED, 'connected'); + expect(GameDispatch.clearStore).not.toHaveBeenCalled(); + expect(RoomsDispatch.clearStore).not.toHaveBeenCalled(); + expect(ServerDispatch.clearStore).not.toHaveBeenCalled(); + expect(ServerDispatch.updateStatus).toHaveBeenCalledWith( + WebsocketTypes.StatusEnum.CONNECTED, + 'connected', + ); + }); + + it('does NOT clear stores on LOGGED_IN transition', () => { + impl.updateStatus(WebsocketTypes.StatusEnum.LOGGED_IN, 'in'); + expect(GameDispatch.clearStore).not.toHaveBeenCalled(); + expect(RoomsDispatch.clearStore).not.toHaveBeenCalled(); + expect(ServerDispatch.clearStore).not.toHaveBeenCalled(); + }); +}); diff --git a/webclient/src/api/response/SessionResponseImpl.ts b/webclient/src/api/response/SessionResponseImpl.ts index 150a942f7..261541ad9 100644 --- a/webclient/src/api/response/SessionResponseImpl.ts +++ b/webclient/src/api/response/SessionResponseImpl.ts @@ -2,10 +2,10 @@ import { Data } from '@app/types'; import { WebsocketTypes } from '@app/websocket/types'; import { GameDispatch, RoomsDispatch, ServerDispatch } from '@app/store'; -type LoginSuccess = WebsocketTypes.WebSocketSessionResponseOverrides['Response_Login']; -type PendingActivation = WebsocketTypes.WebSocketSessionResponseOverrides['Response']; +type LoginSuccess = WebsocketTypes.LoginSuccessContext; +type PendingActivation = WebsocketTypes.PendingActivationContext; -export class SessionResponseImpl implements WebsocketTypes.ISessionResponse { +export class SessionResponseImpl implements WebsocketTypes.ISessionResponse { initialized(): void { ServerDispatch.initialized(); } diff --git a/webclient/src/store/game/game.actions.spec.ts b/webclient/src/store/game/game.actions.spec.ts index 35d175ea6..2b9e995b5 100644 --- a/webclient/src/store/game/game.actions.spec.ts +++ b/webclient/src/store/game/game.actions.spec.ts @@ -50,8 +50,8 @@ describe('Actions', () => { }); it('playerLeft', () => { - const action = Actions.playerLeft({ gameId: 1, playerId: 2 }); - expect(action.payload).toEqual({ gameId: 1, playerId: 2 }); + const action = Actions.playerLeft({ gameId: 1, playerId: 2, reason: 3, timeReceived: 42 }); + expect(action.payload).toEqual({ gameId: 1, playerId: 2, reason: 3, timeReceived: 42 }); }); it('playerPropertiesChanged', () => { diff --git a/webclient/src/store/game/game.dispatch.spec.ts b/webclient/src/store/game/game.dispatch.spec.ts index bf6d91254..8d1c24953 100644 --- a/webclient/src/store/game/game.dispatch.spec.ts +++ b/webclient/src/store/game/game.dispatch.spec.ts @@ -57,9 +57,13 @@ describe('Dispatch', () => { expect(mockDispatch).toHaveBeenCalledWith(Actions.playerJoined({ gameId: 1, playerProperties: props })); }); - it('playerLeft dispatches Actions.playerLeft()', () => { + it('playerLeft dispatches Actions.playerLeft() with reason + timeReceived', () => { + const nowSpy = vi.spyOn(Date, 'now').mockReturnValue(9999); Dispatch.playerLeft(1, 2, 3); - expect(mockDispatch).toHaveBeenCalledWith(Actions.playerLeft({ gameId: 1, playerId: 2 })); + expect(mockDispatch).toHaveBeenCalledWith( + Actions.playerLeft({ gameId: 1, playerId: 2, reason: 3, timeReceived: 9999 }), + ); + nowSpy.mockRestore(); }); it('playerPropertiesChanged dispatches Actions.playerPropertiesChanged()', () => { diff --git a/webclient/src/store/game/game.dispatch.ts b/webclient/src/store/game/game.dispatch.ts index 294f4831a..d63436252 100644 --- a/webclient/src/store/game/game.dispatch.ts +++ b/webclient/src/store/game/game.dispatch.ts @@ -31,8 +31,8 @@ export const Dispatch = { store.dispatch(Actions.playerJoined({ gameId, playerProperties })); }, - playerLeft: (gameId: number, playerId: number, _reason: number) => { - store.dispatch(Actions.playerLeft({ gameId, playerId })); + playerLeft: (gameId: number, playerId: number, reason: number) => { + store.dispatch(Actions.playerLeft({ gameId, playerId, reason, timeReceived: Date.now() })); }, playerPropertiesChanged: (gameId: number, playerId: number, properties: Data.ServerInfo_PlayerProperties) => { diff --git a/webclient/src/store/game/game.reducer.spec.ts b/webclient/src/store/game/game.reducer.spec.ts index 6a467f145..b9bc0f227 100644 --- a/webclient/src/store/game/game.reducer.spec.ts +++ b/webclient/src/store/game/game.reducer.spec.ts @@ -147,10 +147,49 @@ describe('2B: Game state & player management', () => { it('PLAYER_LEFT → removes player from game.players', () => { const state = makeState(); - const result = gamesReducer(state, Actions.playerLeft({ gameId: 1, playerId: 1 })); + const result = gamesReducer( + state, + Actions.playerLeft({ gameId: 1, playerId: 1, reason: 3, timeReceived: 1000 }), + ); expect(result.games[1].players[1]).toBeUndefined(); }); + it('PLAYER_LEFT → emits a GameMessage with the formatted reason string', () => { + const state = makeState(); + state.games[1].players[1].properties = makePlayerProperties({ + playerId: 1, + userInfo: { name: 'Alice' }, + }); + const before = state.games[1].messages.length; + + const result = gamesReducer( + state, + Actions.playerLeft({ gameId: 1, playerId: 1, reason: 2, timeReceived: 1234 }), + ); + + const msgs = result.games[1].messages; + expect(msgs.length).toBe(before + 1); + const added = msgs[msgs.length - 1]; + expect(added.playerId).toBe(1); + expect(added.timeReceived).toBe(1234); + expect(added.message).toBe('Alice has left the game (kicked by game host or moderator).'); + }); + + it.each([ + [1, 'reason unknown'], + [2, 'kicked by game host or moderator'], + [3, 'player left the game'], + [4, 'player disconnected from server'], + ])('PLAYER_LEFT reason=%i → log text includes "%s"', (reason, fragment) => { + const state = makeState(); + const result = gamesReducer( + state, + Actions.playerLeft({ gameId: 1, playerId: 1, reason, timeReceived: 0 }), + ); + const last = result.games[1].messages[result.games[1].messages.length - 1]; + expect(last.message).toContain(fragment); + }); + it('PLAYER_PROPERTIES_CHANGED → replaces properties on existing player', () => { const state = makeState(); const newProps = makePlayerProperties({ playerId: 1, conceded: true }); @@ -1040,7 +1079,9 @@ describe('2L: Null-guard / missing entity early-returns', () => { it('PLAYER_LEFT with unknown gameId → state unchanged', () => { const state = makeState(); - expect(gamesReducer(state, Actions.playerLeft({ gameId: UNKNOWN_GAME, playerId: 1 }))).toBe(state); + expect( + gamesReducer(state, Actions.playerLeft({ gameId: UNKNOWN_GAME, playerId: 1, reason: 3, timeReceived: 0 })), + ).toBe(state); }); it('updatePlayer guard: PLAYER_PROPERTIES_CHANGED with unknown gameId → state unchanged', () => { diff --git a/webclient/src/store/game/game.reducer.ts b/webclient/src/store/game/game.reducer.ts index 62e06da21..9fad16f6b 100644 --- a/webclient/src/store/game/game.reducer.ts +++ b/webclient/src/store/game/game.reducer.ts @@ -5,6 +5,21 @@ import { GamesState } from './game.interfaces'; export const MAX_GAME_MESSAGES = 1000; +// Mirrors Event_Leave.LeaveReason values (1=OTHER, 2=USER_KICKED, +// 3=USER_LEFT, 4=USER_DISCONNECTED); kept in sync with desktop +// `GameEventHandler::getLeaveReason` in game_event_handler.cpp. +const LEAVE_REASON_MESSAGES: Record = { + 1: 'reason unknown', + 2: 'kicked by game host or moderator', + 3: 'player left the game', + 4: 'player disconnected from server', +}; + +function formatLeaveMessage(playerName: string, reason: number): string { + const reasonText = LEAVE_REASON_MESSAGES[reason] ?? LEAVE_REASON_MESSAGES[1]; + return `${playerName} has left the game (${reasonText}).`; +} + /** Converts the proto ServerInfo_Player[] array into the keyed PlayerEntry map. */ function normalizePlayers(playerList: Data.ServerInfo_Player[]): { [playerId: number]: Enriched.PlayerEntry } { const players: { [playerId: number]: Enriched.PlayerEntry } = {}; @@ -158,12 +173,28 @@ export const gamesSlice = createSlice({ }; }, - playerLeft: (state, action: PayloadAction<{ gameId: number; playerId: number }>) => { - const { gameId, playerId } = action.payload; + playerLeft: ( + state, + action: PayloadAction<{ gameId: number; playerId: number; reason: number; timeReceived: number }>, + ) => { + const { gameId, playerId, reason, timeReceived } = action.payload; const game = state.games[gameId]; - if (game) { - delete game.players[playerId]; + if (!game) { + return; } + const player = game.players[playerId]; + const playerName = player?.properties.userInfo?.name ?? 'Unknown player'; + + if (game.messages.length >= MAX_GAME_MESSAGES) { + game.messages = game.messages.slice(game.messages.length - MAX_GAME_MESSAGES + 1); + } + game.messages.push({ + playerId, + message: formatLeaveMessage(playerName, reason), + timeReceived, + }); + + delete game.players[playerId]; }, playerPropertiesChanged: ( @@ -393,8 +424,8 @@ export const gamesSlice = createSlice({ return; } - const deckZone = player.zones['deck']; - const handZone = player.zones['hand']; + const deckZone = player.zones[Enriched.ZoneName.DECK]; + const handZone = player.zones[Enriched.ZoneName.HAND]; if (!handZone) { return; } @@ -475,6 +506,9 @@ export const gamesSlice = createSlice({ game.messages.push({ playerId, message, timeReceived }); }, + // Intentional no-op: action-type placeholders so UI/middleware can + // subscribe to notifications without mutating store state. Removing + // them would silently drop the dispatch path; keep the empty bodies. zoneShuffled: (_state, _action: PayloadAction<{ gameId: number; playerId: number; data: Data.Event_Shuffle }>) => {}, zoneDumped: (_state, _action: PayloadAction<{ gameId: number; playerId: number; data: Data.Event_DumpZone }>) => {}, dieRolled: (_state, _action: PayloadAction<{ gameId: number; playerId: number; data: Data.Event_RollDie }>) => {}, diff --git a/webclient/src/store/game/game.selectors.spec.ts b/webclient/src/store/game/game.selectors.spec.ts index 4a04fa802..bd047ce86 100644 --- a/webclient/src/store/game/game.selectors.spec.ts +++ b/webclient/src/store/game/game.selectors.spec.ts @@ -112,6 +112,41 @@ describe('Selectors', () => { expect(Selectors.getActivePhase(rootState(state), 1)).toBe(3); }); + it('getHostId → returns hostId from game', () => { + const state = makeState({ games: { 1: makeGameEntry({ hostId: 7 }) } }); + expect(Selectors.getHostId(rootState(state), 1)).toBe(7); + }); + + it('getHostId → returns undefined for unknown gameId', () => { + const state = makeState(); + expect(Selectors.getHostId(rootState(state), 999)).toBeUndefined(); + }); + + it('getSecondsElapsed → returns secondsElapsed from game', () => { + const state = makeState({ games: { 1: makeGameEntry({ secondsElapsed: 314 }) } }); + expect(Selectors.getSecondsElapsed(rootState(state), 1)).toBe(314); + }); + + it('getJudge → returns judge flag from game', () => { + const state = makeState({ games: { 1: makeGameEntry({ judge: true }) } }); + expect(Selectors.getJudge(rootState(state), 1)).toBe(true); + }); + + it('getJudge → returns false when game not found', () => { + const state = makeState(); + expect(Selectors.getJudge(rootState(state), 999)).toBe(false); + }); + + it('getResuming → returns resuming flag from game', () => { + const state = makeState({ games: { 1: makeGameEntry({ resuming: true }) } }); + expect(Selectors.getResuming(rootState(state), 1)).toBe(true); + }); + + it('getResuming → returns false when game not found', () => { + const state = makeState(); + expect(Selectors.getResuming(rootState(state), 999)).toBe(false); + }); + it('isStarted → returns true when game is started', () => { const state = makeState({ games: { 1: makeGameEntry({ started: true }) } }); expect(Selectors.isStarted(rootState(state), 1)).toBe(true); diff --git a/webclient/src/store/game/game.selectors.ts b/webclient/src/store/game/game.selectors.ts index 5e217dd11..bef650e86 100644 --- a/webclient/src/store/game/game.selectors.ts +++ b/webclient/src/store/game/game.selectors.ts @@ -80,6 +80,18 @@ export const Selectors = { getActivePhase: ({ games }: State, gameId: number): number | undefined => games.games[gameId]?.activePhase, + getHostId: ({ games }: State, gameId: number): number | undefined => + games.games[gameId]?.hostId, + + getSecondsElapsed: ({ games }: State, gameId: number): number | undefined => + games.games[gameId]?.secondsElapsed, + + getJudge: ({ games }: State, gameId: number): boolean => + games.games[gameId]?.judge ?? false, + + getResuming: ({ games }: State, gameId: number): boolean => + games.games[gameId]?.resuming ?? false, + isStarted: ({ games }: State, gameId: number): boolean => games.games[gameId]?.started ?? false, diff --git a/webclient/src/store/rooms/rooms.actions.tsx b/webclient/src/store/rooms/rooms.actions.ts similarity index 100% rename from webclient/src/store/rooms/rooms.actions.tsx rename to webclient/src/store/rooms/rooms.actions.ts diff --git a/webclient/src/store/rooms/rooms.dispatch.tsx b/webclient/src/store/rooms/rooms.dispatch.ts similarity index 100% rename from webclient/src/store/rooms/rooms.dispatch.tsx rename to webclient/src/store/rooms/rooms.dispatch.ts diff --git a/webclient/src/store/rooms/rooms.interfaces.tsx b/webclient/src/store/rooms/rooms.interfaces.ts similarity index 100% rename from webclient/src/store/rooms/rooms.interfaces.tsx rename to webclient/src/store/rooms/rooms.interfaces.ts diff --git a/webclient/src/store/rooms/rooms.reducer.tsx b/webclient/src/store/rooms/rooms.reducer.ts similarity index 100% rename from webclient/src/store/rooms/rooms.reducer.tsx rename to webclient/src/store/rooms/rooms.reducer.ts diff --git a/webclient/src/store/rooms/rooms.selectors.tsx b/webclient/src/store/rooms/rooms.selectors.ts similarity index 100% rename from webclient/src/store/rooms/rooms.selectors.tsx rename to webclient/src/store/rooms/rooms.selectors.ts diff --git a/webclient/src/store/rooms/rooms.types.tsx b/webclient/src/store/rooms/rooms.types.ts similarity index 100% rename from webclient/src/store/rooms/rooms.types.tsx rename to webclient/src/store/rooms/rooms.types.ts diff --git a/webclient/src/types/enriched.ts b/webclient/src/types/enriched.ts index cc3de313c..a950ddfdc 100644 --- a/webclient/src/types/enriched.ts +++ b/webclient/src/types/enriched.ts @@ -61,6 +61,19 @@ export interface PlayerEntry { arrows: { [arrowId: number]: ServerInfo_Arrow }; } +// Canonical wire values for `ZoneEntry.name`. Server-defined and stable. +export const ZoneName = { + TABLE: 'table', + GRAVE: 'grave', + EXILE: 'rfg', + HAND: 'hand', + DECK: 'deck', + SIDEBOARD: 'sb', + STACK: 'stack', +} as const; + +export type ZoneNameValue = typeof ZoneName[keyof typeof ZoneName]; + export interface ZoneEntry { name: string; /** ZoneType enum value (0=Private, 1=Public, 2=Hidden). */ diff --git a/webclient/src/websocket/WebClient.spec.ts b/webclient/src/websocket/WebClient.spec.ts index d2fdb9b5e..7bb855a67 100644 --- a/webclient/src/websocket/WebClient.spec.ts +++ b/webclient/src/websocket/WebClient.spec.ts @@ -189,6 +189,18 @@ describe('WebClient', () => { vi.advanceTimersByTime(5000); expect(wsMockInstance.close).toHaveBeenCalled(); }); + + it('closes the prior in-flight socket on rapid re-click', () => { + const { instances } = installMockWebSocket(); + // The fresh installMockWebSocket replaces the stub from beforeEach so + // we observe the next two constructions in isolation. + client.testConnect(target); + const first = instances[instances.length - 1]; + expect(first.close).not.toHaveBeenCalled(); + + client.testConnect(target); + expect(first.close).toHaveBeenCalled(); + }); }); describe('disconnect', () => { diff --git a/webclient/src/websocket/WebClient.ts b/webclient/src/websocket/WebClient.ts index ec628440e..21cd46456 100644 --- a/webclient/src/websocket/WebClient.ts +++ b/webclient/src/websocket/WebClient.ts @@ -1,5 +1,8 @@ import { ping } from './commands/session'; import { CLIENT_OPTIONS } from './config'; +import { GameEvents } from './events/game'; +import { RoomEvents } from './events/room'; +import { SessionEvents } from './events/session'; import type { ConnectTarget } from './types/WebClientConfig'; import type { IWebClientRequest } from './types/WebClientRequest'; import type { IWebClientResponse } from './types/WebClientResponse'; @@ -22,6 +25,7 @@ export class WebClient { protobuf: ProtobufService; socket: WebSocketService; status: StatusEnum; + private testSocket: WebSocket | null = null; constructor( public request: IWebClientRequest, @@ -46,7 +50,8 @@ export class WebClient { { send: (data) => this.socket.send(data), isOpen: () => this.socket.checkReadyState(WebSocket.OPEN), - } + }, + { game: GameEvents, room: RoomEvents, session: SessionEvents }, ); this.socket.message$.subscribe((message: MessageEvent) => { @@ -64,24 +69,42 @@ export class WebClient { } public testConnect(target: ConnectTarget) { + // A prior test connection still in flight when the user re-clicks would + // otherwise leak the socket until its keepalive timeout. Close eagerly. + if (this.testSocket) { + this.testSocket.close(); + this.testSocket = null; + } + const protocol = window.location.hostname === 'localhost' ? 'ws' : 'wss'; const { host, port } = target; const socket = new WebSocket(`${protocol}://${host}:${port}`); socket.binaryType = 'arraybuffer'; + this.testSocket = socket; const timeout = setTimeout(() => socket.close(), CLIENT_OPTIONS.keepalive); + const clearIfActive = () => { + if (this.testSocket === socket) { + this.testSocket = null; + } + }; + socket.onopen = () => { clearTimeout(timeout); this.response.session.testConnectionSuccessful(); socket.close(); + clearIfActive(); }; socket.onerror = () => { this.response.session.testConnectionFailed(); + clearIfActive(); }; - socket.onclose = () => {}; + socket.onclose = () => { + clearIfActive(); + }; } public disconnect() { diff --git a/webclient/src/websocket/__mocks__/helpers.ts b/webclient/src/websocket/__mocks__/helpers.ts index 7ef2ed718..0173c9b43 100644 --- a/webclient/src/websocket/__mocks__/helpers.ts +++ b/webclient/src/websocket/__mocks__/helpers.ts @@ -20,13 +20,21 @@ export function makeMockWebSocketInstance() { export function installMockWebSocket() { const originalWebSocket = (globalThis as any).WebSocket; const mockInstance = makeMockWebSocketInstance(); + const instances: ReturnType[] = [mockInstance]; + let firstCall = true; const MockWS = vi.fn(function MockWebSocket() { - return mockInstance; + if (firstCall) { + firstCall = false; + return mockInstance; + } + const next = makeMockWebSocketInstance(); + instances.push(next); + return next; }) as any; MockWS.OPEN = 1; MockWS.CLOSED = 3; (globalThis as any).WebSocket = MockWS; - return { MockWS, mockInstance, restore: () => { + return { MockWS, mockInstance, instances, restore: () => { (globalThis as any).WebSocket = originalWebSocket; } }; } diff --git a/webclient/src/websocket/commands/session/login.ts b/webclient/src/websocket/commands/session/login.ts index fc290ceda..8f194d650 100644 --- a/webclient/src/websocket/commands/session/login.ts +++ b/webclient/src/websocket/commands/session/login.ts @@ -47,7 +47,7 @@ export function login(options: ConnectTarget & LoginParams, password?: string, p WebClient.instance.response.session.updateBuddyList(buddyList); WebClient.instance.response.session.updateIgnoreList(ignoreList); WebClient.instance.response.session.updateUser(userInfo); - WebClient.instance.response.session.loginSuccessful({ ...resp, hashedPassword: loginConfig.hashedPassword }); + WebClient.instance.response.session.loginSuccessful({ hashedPassword: loginConfig.hashedPassword }); listUsers(); listRooms(); @@ -71,11 +71,10 @@ export function login(options: ConnectTarget & LoginParams, password?: string, p onLoginError('Login failed: missing client ID'), [Response_ResponseCode.RespContextError]: () => onLoginError('Login failed: server error'), - [Response_ResponseCode.RespAccountNotActivated]: (raw) => + [Response_ResponseCode.RespAccountNotActivated]: () => onLoginError('Login failed: account not activated', () => { WebClient.instance.response.session.accountAwaitingActivation({ - ...raw, host: options.host, port: options.port, userName: options.userName, diff --git a/webclient/src/websocket/commands/session/register.ts b/webclient/src/websocket/commands/session/register.ts index 42eee407e..c1dde363a 100644 --- a/webclient/src/websocket/commands/session/register.ts +++ b/webclient/src/websocket/commands/session/register.ts @@ -45,10 +45,9 @@ export function register(options: ConnectTarget & RegisterParams, password?: str }, password, passwordSalt); WebClient.instance.response.session.registrationSuccess(); }, - [Response_ResponseCode.RespRegistrationAcceptedNeedsActivation]: (raw) => { + [Response_ResponseCode.RespRegistrationAcceptedNeedsActivation]: () => { updateStatus(StatusEnum.DISCONNECTED, 'Registration accepted, awaiting activation'); WebClient.instance.response.session.accountAwaitingActivation({ - ...raw, host: options.host, port: options.port, userName: options.userName, diff --git a/webclient/src/websocket/services/ProtobufService.spec.ts b/webclient/src/websocket/services/ProtobufService.spec.ts index 44825a057..1f5cf8d7e 100644 --- a/webclient/src/websocket/services/ProtobufService.spec.ts +++ b/webclient/src/websocket/services/ProtobufService.spec.ts @@ -7,25 +7,13 @@ vi.mock('@bufbuild/protobuf', async (importOriginal) => ({ setExtension: vi.fn(), })); -vi.mock('../events/game', () => ({ - GameEvents: [], -})); - -vi.mock('../events/room', () => ({ - RoomEvents: [], -})); - -vi.mock('../events/session', () => ({ - SessionEvents: [], -})); - import { create, fromBinary, hasExtension, getExtension } from '@bufbuild/protobuf'; import type { GenExtension } from '@bufbuild/protobuf/codegenv2'; -import { ProtobufService } from './ProtobufService'; -import { GameEvents } from '../events/game'; -import { RoomEvents } from '../events/room'; -import { SessionEvents } from '../events/session'; +import { ProtobufService, type EventRegistries } from './ProtobufService'; +import type { GameExtensionRegistry } from '../events/game'; +import type { RoomExtensionRegistry } from '../events/room'; +import type { SessionExtensionRegistry } from '../events/session'; import type { AdminCommand, @@ -55,6 +43,12 @@ type ProtobufInternal = ProtobufService & { }; let mockSocket: { isOpen: ReturnType; send: ReturnType }; +let gameEvents: GameExtensionRegistry; +let roomEvents: RoomExtensionRegistry; +let sessionEvents: SessionExtensionRegistry; +let registries: EventRegistries; + +const makeService = () => new ProtobufService(mockSocket, registries); beforeEach(() => { mockSocket = { @@ -62,10 +56,13 @@ beforeEach(() => { send: vi.fn(), }; - // Reset event registries - (GameEvents as any).length = 0; - (RoomEvents as any).length = 0; - (SessionEvents as any).length = 0; + // Per-test registries inject empty handlers; tests that exercise dispatch + // push their own mock entries. This is what the old `(GameEvents as any).length = 0` + // hack approximated, now expressed as constructor injection. + gameEvents = []; + roomEvents = []; + sessionEvents = []; + registries = { game: gameEvents, room: roomEvents, session: sessionEvents }; }); describe('ProtobufService', () => { @@ -78,7 +75,7 @@ describe('ProtobufService', () => { describe('resetCommands', () => { it('resets cmdId and pendingCommands', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); service.sendSessionCommand(sessionExt, vi.fn()); expect((service as ProtobufInternal).cmdId).toBe(1); service.resetCommands(); @@ -89,7 +86,7 @@ describe('ProtobufService', () => { describe('sendCommand', () => { it('increments cmdId and stores callback', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); const cb = vi.fn(); service.sendCommand(create(CommandContainerSchema), cb); expect((service as ProtobufInternal).cmdId).toBe(1); @@ -97,14 +94,14 @@ describe('ProtobufService', () => { }); it('sends encoded data when socket is OPEN', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); mockSocket.isOpen.mockReturnValue(true); service.sendCommand(create(CommandContainerSchema), vi.fn()); expect(mockSocket.send).toHaveBeenCalled(); }); it('does not register callback or increment cmdId when transport is closed', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); mockSocket.isOpen.mockReturnValue(false); const cb = vi.fn(); service.sendCommand(create(CommandContainerSchema), cb); @@ -114,13 +111,13 @@ describe('ProtobufService', () => { }); it('returns true when command is sent', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); const result = service.sendCommand(create(CommandContainerSchema), vi.fn()); expect(result).toBe(true); }); it('returns false when transport is closed', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); mockSocket.isOpen.mockReturnValue(false); const result = service.sendCommand(create(CommandContainerSchema), vi.fn()); expect(result).toBe(false); @@ -129,7 +126,7 @@ describe('ProtobufService', () => { describe('send*Command when transport is closed', () => { it('calls onError when sendSessionCommand is dropped', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); mockSocket.isOpen.mockReturnValue(false); const onError = vi.fn(); service.sendSessionCommand(sessionExt, {}, { onError }); @@ -137,7 +134,7 @@ describe('ProtobufService', () => { }); it('calls onError when sendRoomCommand is dropped', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); mockSocket.isOpen.mockReturnValue(false); const onError = vi.fn(); service.sendRoomCommand(42, roomExt, {}, { onError }); @@ -145,7 +142,7 @@ describe('ProtobufService', () => { }); it('calls onError when sendGameCommand is dropped', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); mockSocket.isOpen.mockReturnValue(false); const onError = vi.fn(); service.sendGameCommand(7, gameExt, {}, { onError }); @@ -153,7 +150,7 @@ describe('ProtobufService', () => { }); it('calls onError when sendModeratorCommand is dropped', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); mockSocket.isOpen.mockReturnValue(false); const onError = vi.fn(); service.sendModeratorCommand(moderatorExt, {}, { onError }); @@ -161,7 +158,7 @@ describe('ProtobufService', () => { }); it('calls onError when sendAdminCommand is dropped', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); mockSocket.isOpen.mockReturnValue(false); const onError = vi.fn(); service.sendAdminCommand(adminExt, {}, { onError }); @@ -169,7 +166,7 @@ describe('ProtobufService', () => { }); it('does not throw when command is dropped with no options', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); mockSocket.isOpen.mockReturnValue(false); expect(() => service.sendSessionCommand(sessionExt, {})).not.toThrow(); }); @@ -177,14 +174,14 @@ describe('ProtobufService', () => { describe('sendSessionCommand', () => { it('stores callback and increments cmdId', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); service.sendSessionCommand(sessionExt, {}); expect((service as ProtobufInternal).cmdId).toBe(1); expect((service as ProtobufInternal).pendingCommands.get(1)).toBeTypeOf('function'); }); it('invokes onResponse with raw response when the pending command is triggered', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); const cb = vi.fn(); service.sendSessionCommand(sessionExt, {}, { onResponse: cb }); @@ -195,7 +192,7 @@ describe('ProtobufService', () => { }); it('does not throw when no callback is provided and pending command is triggered', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); service.sendSessionCommand(sessionExt, {}); const storedCb = (service as ProtobufInternal).pendingCommands.get(1)!; @@ -205,13 +202,13 @@ describe('ProtobufService', () => { describe('sendRoomCommand', () => { it('stores callback and increments cmdId', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); service.sendRoomCommand(42, roomExt, {}); expect((service as ProtobufInternal).cmdId).toBe(1); }); it('invokes onResponse with raw response when the pending command is triggered', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); const cb = vi.fn(); service.sendRoomCommand(42, roomExt, {}, { onResponse: cb }); @@ -222,7 +219,7 @@ describe('ProtobufService', () => { }); it('does not throw when no callback is provided and pending command is triggered', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); service.sendRoomCommand(42, roomExt, {}); const storedCb = (service as ProtobufInternal).pendingCommands.get(1)!; @@ -232,13 +229,13 @@ describe('ProtobufService', () => { describe('sendGameCommand', () => { it('stores callback and increments cmdId', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); service.sendGameCommand(7, gameExt, {}); expect((service as ProtobufInternal).cmdId).toBe(1); }); it('invokes onResponse with raw response when the pending command is triggered', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); const cb = vi.fn(); service.sendGameCommand(7, gameExt, {}, { onResponse: cb }); @@ -249,7 +246,7 @@ describe('ProtobufService', () => { }); it('does not throw when no callback is provided and pending command is triggered', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); service.sendGameCommand(7, gameExt, {}); const storedCb = (service as ProtobufInternal).pendingCommands.get(1)!; @@ -259,13 +256,13 @@ describe('ProtobufService', () => { describe('sendModeratorCommand', () => { it('stores callback and increments cmdId', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); service.sendModeratorCommand(moderatorExt, {}); expect((service as ProtobufInternal).cmdId).toBe(1); }); it('invokes onResponse with raw response when the pending command is triggered', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); const cb = vi.fn(); service.sendModeratorCommand(moderatorExt, {}, { onResponse: cb }); @@ -276,7 +273,7 @@ describe('ProtobufService', () => { }); it('does not throw when no callback is provided and pending command is triggered', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); service.sendModeratorCommand(moderatorExt, {}); const storedCb = (service as ProtobufInternal).pendingCommands.get(1)!; @@ -286,13 +283,13 @@ describe('ProtobufService', () => { describe('sendAdminCommand', () => { it('stores callback and increments cmdId', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); service.sendAdminCommand(adminExt, {}); expect((service as ProtobufInternal).cmdId).toBe(1); }); it('invokes onResponse with raw response when the pending command is triggered', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); const cb = vi.fn(); service.sendAdminCommand(adminExt, {}, { onResponse: cb }); @@ -303,7 +300,7 @@ describe('ProtobufService', () => { }); it('does not throw when no callback is provided and pending command is triggered', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); service.sendAdminCommand(adminExt, {}); const storedCb = (service as ProtobufInternal).pendingCommands.get(1)!; @@ -313,7 +310,7 @@ describe('ProtobufService', () => { describe('handleMessageEvent', () => { it('routes RESPONSE message to processServerResponse', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); const cb = vi.fn(); (service as ProtobufInternal).cmdId = 1; (service as ProtobufInternal).pendingCommands.set(1, cb); @@ -331,7 +328,7 @@ describe('ProtobufService', () => { }); it('routes ROOM_EVENT message', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); const processRoomEvent = vi.spyOn(service as ProtobufInternal, 'processRoomEvent'); vi.mocked(fromBinary).mockReturnValue( @@ -345,7 +342,7 @@ describe('ProtobufService', () => { }); it('routes SESSION_EVENT message', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); const processSessionEvent = vi.spyOn(service as ProtobufInternal, 'processSessionEvent'); vi.mocked(fromBinary).mockReturnValue( @@ -359,7 +356,7 @@ describe('ProtobufService', () => { }); it('routes GAME_EVENT_CONTAINER message', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); const processGameEvent = vi.spyOn(service as ProtobufInternal, 'processGameEvent'); vi.mocked(fromBinary).mockReturnValue( @@ -373,7 +370,7 @@ describe('ProtobufService', () => { }); it('warns on unknown message types (default case)', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); vi.mocked(fromBinary).mockReturnValue( @@ -388,13 +385,13 @@ describe('ProtobufService', () => { }); it('does nothing when decoded message is null', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); vi.mocked(fromBinary).mockReturnValue(null!); expect(() => service.handleMessageEvent({ data: new ArrayBuffer(0) } as MessageEvent)).not.toThrow(); }); it('catches and logs decode errors', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); vi.mocked(fromBinary).mockImplementation(() => { throw new Error('decode error'); @@ -407,7 +404,7 @@ describe('ProtobufService', () => { describe('processGameEvent', () => { it('returns early when container has no eventList', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); vi.mocked(hasExtension).mockReturnValue(false); (service as ProtobufInternal).processGameEvent(null, {}); expect(hasExtension).not.toHaveBeenCalled(); @@ -418,8 +415,8 @@ describe('ProtobufService', () => { const mockExt = {} as GenExtension; const payload = { someData: 1 }; - (GameEvents as any).push([mockExt, handler]); - const service = new ProtobufService(mockSocket); + (gameEvents as any).push([mockExt, handler]); + const service = makeService(); vi.mocked(hasExtension).mockReturnValue(true); vi.mocked(getExtension).mockReturnValue(payload); @@ -436,8 +433,8 @@ describe('ProtobufService', () => { const mockExt = {} as GenExtension; const payload = { someData: 1 }; - (GameEvents as any).push([mockExt, handler]); - const service = new ProtobufService(mockSocket); + (gameEvents as any).push([mockExt, handler]); + const service = makeService(); vi.mocked(hasExtension).mockReturnValue(true); vi.mocked(getExtension).mockReturnValue(payload); @@ -452,7 +449,7 @@ describe('ProtobufService', () => { describe('processServerResponse', () => { it('returns early when response is undefined', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); (service as ProtobufInternal).pendingCommands.set(1, vi.fn()); (service as ProtobufInternal).processServerResponse(undefined); expect((service as ProtobufInternal).pendingCommands.size).toBe(1); @@ -461,7 +458,7 @@ describe('ProtobufService', () => { describe('processRoomEvent', () => { it('returns early when event is undefined', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); vi.mocked(hasExtension).mockReturnValue(false); (service as ProtobufInternal).processRoomEvent(undefined); expect(hasExtension).not.toHaveBeenCalled(); @@ -472,8 +469,8 @@ describe('ProtobufService', () => { const mockExt = {} as GenExtension; const payload = { roomData: 1 }; - (RoomEvents as any).push([mockExt, handler]); - const service = new ProtobufService(mockSocket); + (roomEvents as any).push([mockExt, handler]); + const service = makeService(); vi.mocked(hasExtension).mockReturnValue(true); vi.mocked(getExtension).mockReturnValue(payload); @@ -486,7 +483,7 @@ describe('ProtobufService', () => { describe('processSessionEvent', () => { it('returns early when event is undefined', () => { - const service = new ProtobufService(mockSocket); + const service = makeService(); vi.mocked(hasExtension).mockReturnValue(false); (service as ProtobufInternal).processSessionEvent(undefined); expect(hasExtension).not.toHaveBeenCalled(); @@ -497,8 +494,8 @@ describe('ProtobufService', () => { const mockExt = {} as GenExtension; const payload = { sessionData: 1 }; - (SessionEvents as any).push([mockExt, handler]); - const service = new ProtobufService(mockSocket); + (sessionEvents as any).push([mockExt, handler]); + const service = makeService(); vi.mocked(hasExtension).mockReturnValue(true); vi.mocked(getExtension).mockReturnValue(payload); diff --git a/webclient/src/websocket/services/ProtobufService.ts b/webclient/src/websocket/services/ProtobufService.ts index 23ab52f06..086c9cf41 100644 --- a/webclient/src/websocket/services/ProtobufService.ts +++ b/webclient/src/websocket/services/ProtobufService.ts @@ -23,9 +23,9 @@ import { type RoomEvent, } from '@app/generated'; -import { GameEvents } from '../events/game'; -import { RoomEvents } from '../events/room'; -import { SessionEvents } from '../events/session'; +import type { GameExtensionRegistry } from '../events/game'; +import type { RoomExtensionRegistry } from '../events/room'; +import type { SessionExtensionRegistry } from '../events/session'; import type { GameEventMeta } from '../types/WebSocketConfig'; import { type CommandOptions, handleResponse } from './command-options'; @@ -34,11 +34,20 @@ export interface SocketTransport { isOpen(): boolean; } +export interface EventRegistries { + game: GameExtensionRegistry; + room: RoomExtensionRegistry; + session: SessionExtensionRegistry; +} + export class ProtobufService { private cmdId = 0; private pendingCommands = new Map void>(); - constructor(private transport: SocketTransport) {} + constructor( + private transport: SocketTransport, + private events: EventRegistries, + ) {} public resetCommands() { this.cmdId = 0; @@ -171,7 +180,7 @@ export class ProtobufService { if (!event) { return; } - for (const [ext, handler] of RoomEvents) { + for (const [ext, handler] of this.events.room) { if (hasExtension(event, ext)) { handler(getExtension(event, ext), event); return; @@ -183,7 +192,7 @@ export class ProtobufService { if (!event) { return; } - for (const [ext, handler] of SessionEvents) { + for (const [ext, handler] of this.events.session) { if (hasExtension(event, ext)) { handler(getExtension(event, ext), undefined); return; @@ -207,7 +216,7 @@ export class ProtobufService { forcedByJudge: forcedByJudge ?? 0, }; - for (const [ext, handler] of GameEvents) { + for (const [ext, handler] of this.events.game) { if (hasExtension(event, ext)) { handler(getExtension(event, ext), meta); break; diff --git a/webclient/src/websocket/types/WebClientResponse.ts b/webclient/src/websocket/types/WebClientResponse.ts index 4000021ee..05b74dbc6 100644 --- a/webclient/src/websocket/types/WebClientResponse.ts +++ b/webclient/src/websocket/types/WebClientResponse.ts @@ -1,12 +1,9 @@ import type { - Response_Login, - Response, Response_GetGamesOfUser, Response_DeckList, Response_DeckDownload, Response_ReplayDownload, Response_WarnList, - ResponseMap, Event_RoomSay, Event_GameJoined, Event_GameStateChanged, @@ -45,17 +42,17 @@ import type { } from '@app/generated'; import type { StatusEnum } from './StatusEnum'; +import type { LoginSuccessContext, PendingActivationContext } from './SignalContexts'; import type { KeyOf, - WebSocketSessionResponseOverrides, WebSocketRoomResponseOverrides, } from './WebSocketConfig'; -export interface ISessionResponse { +export interface ISessionResponse { initialized(): void; connectionAttempted(): void; clearStore(): void; - loginSuccessful(result: T[KeyOf]): void; + loginSuccessful(options: LoginSuccessContext): void; loginFailed(): void; connectionFailed(): void; testConnectionSuccessful(): void; @@ -73,7 +70,7 @@ export interface ISessionResponse]): void; + accountAwaitingActivation(options: PendingActivationContext): void; accountActivationSuccess(): void; accountActivationFailed(): void; registrationRequiresEmail(): void; @@ -179,10 +176,9 @@ export interface IModeratorResponse { } export interface IWebClientResponse< - S extends ResponseMap = WebSocketSessionResponseOverrides, R extends RoomEventMap = WebSocketRoomResponseOverrides, > { - session: ISessionResponse; + session: ISessionResponse; room: IRoomResponse; game: IGameResponse; admin: IAdminResponse; diff --git a/webclient/src/websocket/types/WebSocketConfig.ts b/webclient/src/websocket/types/WebSocketConfig.ts index 8037dcf2d..bf3240dab 100644 --- a/webclient/src/websocket/types/WebSocketConfig.ts +++ b/webclient/src/websocket/types/WebSocketConfig.ts @@ -1,9 +1,6 @@ import type { GameEventContext, - Response_Login, - Response, Event_RoomSay, - ResponseMap, RoomEventMap, } from '@app/generated'; @@ -18,11 +15,6 @@ export interface GameEventMeta { forcedByJudge: number; } -export interface WebSocketSessionResponseOverrides extends ResponseMap { - Response_Login: Response_Login & { hashedPassword?: string }; - Response: Response & { host: string; port: string; userName: string }; -} - export interface WebSocketRoomResponseOverrides extends RoomEventMap { Event_RoomSay: Event_RoomSay & { timeReceived: number }; } diff --git a/webclient/src/websocket/utils/connectionState.spec.ts b/webclient/src/websocket/utils/connectionState.spec.ts new file mode 100644 index 000000000..a01086268 --- /dev/null +++ b/webclient/src/websocket/utils/connectionState.spec.ts @@ -0,0 +1,42 @@ +import { setPendingOptions, consumePendingOptions } from './connectionState'; +import type { WebSocketConnectOptions } from '../types/ConnectOptions'; + +const opts = (over: Partial = {}): WebSocketConnectOptions => ({ + type: 'login', + host: 'h', + port: '1', + userName: 'u', + ...over, +} as unknown as WebSocketConnectOptions); + +describe('connectionState', () => { + beforeEach(() => { + // Drain any value lingering from prior tests. + consumePendingOptions(); + }); + + it('returns null when nothing has been set', () => { + expect(consumePendingOptions()).toBeNull(); + }); + + it('round-trips a value through set → consume', () => { + const value = opts({ host: 'a' }); + setPendingOptions(value); + expect(consumePendingOptions()).toBe(value); + }); + + it('consume is one-shot — second call returns null', () => { + setPendingOptions(opts()); + expect(consumePendingOptions()).not.toBeNull(); + expect(consumePendingOptions()).toBeNull(); + }); + + it('a second set replaces the prior value (no queue semantics)', () => { + const first = opts({ host: 'a' }); + const second = opts({ host: 'b' }); + setPendingOptions(first); + setPendingOptions(second); + expect(consumePendingOptions()).toBe(second); + expect(consumePendingOptions()).toBeNull(); + }); +}); diff --git a/webclient/src/websocket/utils/guid.util.spec.ts b/webclient/src/websocket/utils/guid.util.spec.ts index 4af74a720..d5554b371 100644 --- a/webclient/src/websocket/utils/guid.util.spec.ts +++ b/webclient/src/websocket/utils/guid.util.spec.ts @@ -10,8 +10,13 @@ describe('guid', () => { expect(guid()).toMatch(uuidPattern); }); - it('returns deterministic value when Math.random is mocked', () => { - const spy = vi.spyOn(Math, 'random').mockReturnValue(0.5); + it('returns deterministic value when crypto.getRandomValues is mocked', () => { + const spy = vi.spyOn(crypto, 'getRandomValues').mockImplementation((buf: any) => { + for (let i = 0; i < buf.length; i++) { + buf[i] = 0x1234; + } + return buf; + }); const result = guid(); expect(result).toBe(guid()); spy.mockRestore(); diff --git a/webclient/src/websocket/utils/guid.util.ts b/webclient/src/websocket/utils/guid.util.ts index 73b17d719..ae365c66c 100644 --- a/webclient/src/websocket/utils/guid.util.ts +++ b/webclient/src/websocket/utils/guid.util.ts @@ -1,8 +1,15 @@ -function s4(): string { - const s4 = Math.floor((1 + Math.random()) * 0x10000); - return s4.toString(16).substring(1); +function s4(buf: Uint16Array, idx: number): string { + // Mask to 16 bits then OR 0x10000 so the leading nibble is always present + // (guarantees 4 hex digits without padding logic). + const v = (buf[idx] & 0xffff) | 0x10000; + return v.toString(16).substring(1); } export function guid(): string { - return s4() + s4() + '-' + s4() + '-' + s4() + '-' + s4() + '-' + s4() + s4() + s4(); + const buf = new Uint16Array(8); + crypto.getRandomValues(buf); + return ( + s4(buf, 0) + s4(buf, 1) + '-' + s4(buf, 2) + '-' + s4(buf, 3) + '-' + + s4(buf, 4) + '-' + s4(buf, 5) + s4(buf, 6) + s4(buf, 7) + ); } diff --git a/webclient/src/websocket/utils/passwordHasher.ts b/webclient/src/websocket/utils/passwordHasher.ts index e89302244..fdf8cb3ad 100644 --- a/webclient/src/websocket/utils/passwordHasher.ts +++ b/webclient/src/websocket/utils/passwordHasher.ts @@ -17,9 +17,12 @@ export const hashPassword = (salt: string, password: string): string => { export const generateSalt = (): string => { const characters = '0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz' + const bytes = new Uint8Array(SALT_LENGTH); + crypto.getRandomValues(bytes); + let salt = ''; for (let i = 0; i < SALT_LENGTH; i++) { - salt += characters.charAt(Math.floor(Math.random() * characters.length)); + salt += characters.charAt(bytes[i] % characters.length); } return salt; diff --git a/webclient/src/websocket/utils/sanitizeHtml.util.spec.ts b/webclient/src/websocket/utils/sanitizeHtml.util.spec.ts index 8cf5d90f3..90a861928 100644 --- a/webclient/src/websocket/utils/sanitizeHtml.util.spec.ts +++ b/webclient/src/websocket/utils/sanitizeHtml.util.spec.ts @@ -64,6 +64,16 @@ describe('sanitizeHtml', () => { expect(result).not.toContain('src="javascript:'); }); + it('strips ftp: scheme from img src (scheme-hardening vs desktop)', () => { + const result = sanitizeHtml(''); + expect(result).not.toContain('ftp://'); + }); + + it('preserves https: scheme on img src', () => { + const result = sanitizeHtml(''); + expect(result).toContain('src="https://example.com/img.png"'); + }); + it('strips onerror from img while keeping safe src', () => { const result = sanitizeHtml(''); expect(result).not.toContain('onerror'); diff --git a/webclient/src/websocket/utils/sanitizeHtml.util.ts b/webclient/src/websocket/utils/sanitizeHtml.util.ts index b8deb0ec7..e5e240127 100644 --- a/webclient/src/websocket/utils/sanitizeHtml.util.ts +++ b/webclient/src/websocket/utils/sanitizeHtml.util.ts @@ -8,10 +8,14 @@ DOMPurify.addHook('afterSanitizeAttributes', (node) => { }); export function sanitizeHtml(msg: string): string { + // Desktop Cockatrice renders MOTD via Qt QTextBrowser with no sanitization; + // web client hardens via a DOMPurify tag/attr allowlist and restricts URIs + // to https/http (ftp is effectively dead in modern browsers and would only + // broaden the attack surface for a hostile server). return DOMPurify.sanitize(msg, { ALLOWED_TAGS: ['br', 'a', 'img', 'center', 'b', 'font'], ALLOWED_ATTR: ['href', 'color', 'rel', 'target', 'src', 'alt'], ADD_URI_SAFE_ATTR: ['color'], - ALLOWED_URI_REGEXP: /^(?:(?:https?|ftp):)/i, + ALLOWED_URI_REGEXP: /^https?:/i, }); }