From 7be9ccd2f96459e108b7d995979c123f1f1d282c Mon Sep 17 00:00:00 2001 From: J3vb <192430104+J3vb@users.noreply.github.com> Date: Fri, 14 Aug 2026 16:15:32 +0200 Subject: [PATCH] fix: batch of 22 correctness fixes across server and client (#1371) * fix(voice): 4 defect(s) (OC-0008, OC-0009, OC-0042, OC-0080) Guard LiveKit session state against supersession: bump the camera/screen generation in leaveVoice and teardownForReconnect so an in-flight enable discards its track, bail out of restoreLocalVoiceState when a newer room claimed _room mid-await, and recheck isStateConnected in the auto-reconnect tail. * fix(ws): 1 defect(s) (OC-0019) * fix(db): 1 defect(s) (OC-0023) * fix(ws): 1 defect(s) (OC-0029) * fix(ws): 1 defect(s) (OC-0032) * fix(voice): 1 defect(s) (OC-0034) * fix(admin): 1 defect(s) (OC-0035) * fix(service): 2 defect(s) (OC-0036, OC-0128) * fix(voice): 2 defect(s) (OC-0038, OC-0065) OC-0038: the LiveKit participant_left webhook cleared the leaver's own client voice state before broadcasting voice_leave, so the broadcast audience (READ_MESSAGES holders union still-in-the-room participants) could no longer see them. Voice membership is gated on CONNECT_VOICE alone, so a participant without READ_MESSAGES never learned the server had torn down their call. Extracted finishVoiceLeave's audience logic into broadcastVoiceEventWithLeaver and used it on the webhook path. OC-0065: handleWebhookParticipantJoined OR'd a GetVoiceState read error into the same branch as "no matching row", so a transient DB failure ejected a legitimate participant from the SFU mid-call. Now the read error is logged and the check skipped, matching sweepStaleVoiceStates. * fix(client): 1 defect(s) (OC-0041) * fix(client): 1 defect(s) (OC-0043) * fix(client): 1 defect(s) (OC-0046) * fix(client): 1 defect(s) (OC-0047) * fix(client): 1 defect(s) (OC-0049) * fix(client): 1 defect(s) (OC-0108) * fix(client): 2 defect(s) (OC-0111, OC-0143) OC-0111: retry a presence_update dropped by the 1-per-10s limiter once the window reopens, so auto-idle's return-to-online does not leave the server and every other client stuck on idle. OC-0143: pass apiConfig.host to the DM profile sidebar so per-user notes are scoped per server, matching channel mutes, the NSFW gate and volume. * test(ws): align aborted-switch test with OC-0034 no-resurrect behavior The fix agent rewrote this pre-existing test (it locked the buggy restore path) but the prove agent left it out of c67d25ed; committed state alone failed go test ./ws/ without it. Co-Authored-By: Claude Fable 5 --------- Co-authored-by: Claude Fable 5 --- .../components/message-list/attachments.ts | 10 +- .../src/components/message-list/renderers.ts | 8 +- .../src/components/settings/helpers.ts | 24 +++ Client/tauri-client/src/lib/livekitSession.ts | 65 ++++++ Client/tauri-client/src/lib/screenShare.ts | 5 +- Client/tauri-client/src/lib/types.ts | 8 + Client/tauri-client/src/lib/ws.ts | 11 + Client/tauri-client/src/pages/MainPage.ts | 37 +++- .../tauri-client/src/stores/members.store.ts | 14 +- Client/tauri-client/src/styles/app.css | 14 +- Client/tauri-client/src/styles/base.css | 2 +- .../unit/appearance-high-contrast.test.ts | 50 +++++ .../tests/unit/attachments-auth.test.ts | 21 ++ .../tests/unit/base-font-size-css.test.ts | 39 ++++ .../tests/unit/livekit-session.test.ts | 198 ++++++++++++++++++ .../tauri-client/tests/unit/main-page.test.ts | 132 +++++++++++- .../tests/unit/members.store.test.ts | 30 +++ .../tauri-client/tests/unit/renderers.test.ts | 22 ++ .../tests/unit/settings-helpers.test.ts | 30 +++ .../tests/unit/ws-reconnect.test.ts | 142 +++++++++++++ Server/admin/api_test.go | 37 ++++ Server/admin/handlers_channels.go | 27 +++ Server/db/auth_queries_test.go | 35 ++++ Server/db/dbgen/users.sql.go | 2 +- Server/db/queries/sqlite/users.sql | 2 +- Server/service/message_crud.go | 27 ++- Server/service/message_crud_test.go | 96 +++++++++ Server/ws/harvest_s5_internal_test.go | 30 ++- Server/ws/hub.go | 17 ++ Server/ws/hub_broadcast.go | 29 +++ Server/ws/livekit_test.go | 50 +++++ Server/ws/livekit_webhook.go | 22 +- Server/ws/livekit_webhook_joined_test.go | 37 ++++ Server/ws/serve.go | 8 + Server/ws/serve_pumps.go | 8 +- Server/ws/serve_pumps_reconnect_race_test.go | 165 +++++++++++++++ Server/ws/serve_ready.go | 9 +- .../ws/serve_ready_error_propagation_test.go | 83 ++++++++ Server/ws/voice_handlers_test.go | 61 ++++++ Server/ws/voice_join.go | 23 +- Server/ws/voice_leave.go | 28 +-- 41 files changed, 1569 insertions(+), 89 deletions(-) create mode 100644 Client/tauri-client/tests/unit/appearance-high-contrast.test.ts create mode 100644 Client/tauri-client/tests/unit/base-font-size-css.test.ts create mode 100644 Server/ws/serve_pumps_reconnect_race_test.go create mode 100644 Server/ws/serve_ready_error_propagation_test.go diff --git a/Client/tauri-client/src/components/message-list/attachments.ts b/Client/tauri-client/src/components/message-list/attachments.ts index fe92dfaf..2a2e261e 100644 --- a/Client/tauri-client/src/components/message-list/attachments.ts +++ b/Client/tauri-client/src/components/message-list/attachments.ts @@ -32,9 +32,15 @@ window.addEventListener("owncord:pref-change", ((e: CustomEvent<{ key: string }> /** Module-level server host for resolving relative attachment URLs. */ let _serverHost: string | null = null; -/** Set the server host (called once from MainPage on connect). */ +/** Set the server host (called once from MainPage on connect). + * Strips a trailing default-HTTPS ":443" and lowercases, mirroring + * normalizeHostForCertCompare in lib/ws.ts and cert_store_key in + * src-tauri/src/tofu.rs — config hosts are stored verbatim (e.g. + * "Example.COM:443") but WHATWG URL drops the default port for https:, + * so isServerUrl's host comparison must normalize the same way or a + * ":443"-suffixed host never matches its own resolved URLs. */ export function setServerHost(host: string): void { - _serverHost = host.toLowerCase(); + _serverHost = host.replace(/:443$/, "").toLowerCase(); } /** Resolve a potentially relative URL to a full URL using the server host. */ diff --git a/Client/tauri-client/src/components/message-list/renderers.ts b/Client/tauri-client/src/components/message-list/renderers.ts index bee8592e..3fe5eb2d 100644 --- a/Client/tauri-client/src/components/message-list/renderers.ts +++ b/Client/tauri-client/src/components/message-list/renderers.ts @@ -179,7 +179,13 @@ export function renderMessage( opts: MessageListOptions, signal: AbortSignal, ): HTMLDivElement { - if (msg.user.username === "System") { + // id 0 is the reserved sentinel for server-synthesized system rows (DB + // user ids are AUTOINCREMENT starting at 1, so no real account can ever + // hold it). Dispatching on the username alone let any account that + // registered the display name "System" render with no author, no role + // colour and no moderation controls — indistinguishable from a genuine + // server notice. + if (msg.user.id === 0 && msg.user.username === "System") { return renderSystemMessage(msg); } diff --git a/Client/tauri-client/src/components/settings/helpers.ts b/Client/tauri-client/src/components/settings/helpers.ts index 9df8c4cd..109e342a 100644 --- a/Client/tauri-client/src/components/settings/helpers.ts +++ b/Client/tauri-client/src/components/settings/helpers.ts @@ -39,6 +39,30 @@ export const THEMES = { "--bg-secondary": "#f2f3f5", "--bg-tertiary": "#e3e5e8", "--text-normal": "#313338", + // OC-0043: the 4 keys above are all this theme used to set. Every other + // surface/text/border/interactive token then fell through to tokens.css's + // dark defaults, so widgets painting --text-normal (now dark) on top of + // e.g. --bg-input (still dark) rendered as unreadable dark-on-dark. + "--bg-input": "#ebedef", + "--bg-hover": "#e8e9ed", + "--bg-active": "#dcdfe4", + "--bg-modifier-hover": "rgba(0, 0, 0, 0.06)", + "--bg-modifier-active": "rgba(0, 0, 0, 0.08)", + "--bg-modifier-selected": "rgba(0, 0, 0, 0.1)", + "--text-muted": "#5c5e66", + "--text-faint": "#747f8d", + "--text-micro": "#949ba4", + "--header-primary": "#060607", + "--header-secondary": "#4e5058", + "--interactive-normal": "#4e5058", + "--interactive-hover": "#23272a", + "--interactive-active": "#000000", + "--interactive-muted": "#c7ccd1", + "--channel-icon": "#6d6f78", + "--border": "#e3e5e8", + "--border-strong": "#cbccd1", + "--scrollbar-thin-thumb": "#cdcfd4", + "--scrollbar-auto-thumb": "#cdcfd4", }, } as const; diff --git a/Client/tauri-client/src/lib/livekitSession.ts b/Client/tauri-client/src/lib/livekitSession.ts index dcba6b50..aa1cb9f9 100644 --- a/Client/tauri-client/src/lib/livekitSession.ts +++ b/Client/tauri-client/src/lib/livekitSession.ts @@ -36,6 +36,7 @@ import { enableScreenshare as doEnableScreenshare, disableScreenshare as doDisableScreenshare, stopManualScreenTracks, + bumpGeneration, getLocalCameraStream as doGetLocalCameraStream, getLocalScreenshareStream as doGetLocalScreenshareStream, getRemoteVideoStream as doGetRemoteVideoStream, @@ -344,6 +345,14 @@ export class LiveKitSession { this.ws.send({ type: "voice_screenshare", payload: { enabled: false } }); } } + // OC-0080: bump first, mirroring doDisableCamera/doDisableScreenshare + // — a concurrent enableCamera()/enableScreenshare() still awaiting + // device acquisition (getUserMedia/getDisplayMedia/publishTrack) when + // an unexpected disconnect fires must detect it was superseded and + // discard its track instead of publishing onto the room about to be + // torn down for auto-reconnect. + bumpGeneration(this._cameraState); + bumpGeneration(this._screenState); // BUG-098: Stop leaked camera/screen tracks before room is nulled. stopManualCameraTrack(this._cameraState, this._room); stopManualScreenTracks(this._screenState, this._room); @@ -575,6 +584,23 @@ export class LiveKitSession { .catch((err) => log.debug("Failed to start audio after reconnect", err)); // oxlint-disable-next-line no-await-in-loop -- sequential reconnect: must restore voice state after connect await this.restoreLocalVoiceState("reconnect"); + + // OC-0009: mirrors connectAndSetup's post-connect checkpoints + // (3/4/5) — this tail keeps awaiting (restoreLocalVoiceState, + // switchActiveDevice) after already installing "connected" into the + // shared state, so `reconnectSuperseded` (which expects "reconnecting") + // can no longer tell a still-current attempt from a superseded one. + // A newer connectAndSetup()/attemptAutoReconnect() may have since + // claimed `_state` for a different channel; isStateConnected() reads + // through a method call so it always sees the live value. + if (!this.isStateConnected(channelId)) { + log.info("Auto-reconnect: superseded after restoreLocalVoiceState — aborting tail", { + channelId, + }); + this.disconnectSupersededLocalRoom(newRoom); + return; + } + // BUG-099: Reapply saved audio devices after reconnect (matches initial join path). const savedInput = loadPref("audioInputDevice", ""); if (savedInput) { @@ -584,6 +610,15 @@ export class LiveKitSession { log.warn("Reconnect: saved input device unavailable, using default", err); } } + + if (!this.isStateConnected(channelId)) { + log.info("Auto-reconnect: superseded after audioinput switch — aborting tail", { + channelId, + }); + this.disconnectSupersededLocalRoom(newRoom); + return; + } + const savedOutput = loadPref("audioOutputDevice", ""); if (savedOutput) { try { @@ -592,6 +627,15 @@ export class LiveKitSession { log.warn("Reconnect: saved output device unavailable, using default", err); } } + + if (!this.isStateConnected(channelId)) { + log.info("Auto-reconnect: superseded after audiooutput switch — aborting tail", { + channelId, + }); + this.disconnectSupersededLocalRoom(newRoom); + return; + } + this._audioPipeline.setupAudioPipeline(); this.reapplyMuteGain(); this.startTokenRefreshTimer(); @@ -875,6 +919,20 @@ export class LiveKitSession { } } + // OC-0008: setMicrophoneEnabled above can block for seconds on the + // browser's mic-permission prompt. If a newer session claimed `_room` + // while this call was suspended there (the user switched channels, or an + // auto-reconnect installed a fresh room), the writes below re-read + // `this._room` fresh (applyMicMuteState) instead of the `room` captured + // at the top of this call — applying THIS call's stale muted/deafened + // decision to that newer room would mute/unmute or resubscribe audio on + // a session that never asked for it. Bail out once the captured room is + // no longer the live one; the newer session owns its own state from here. + if (this._room !== room) { + log.info("restoreLocalVoiceState: superseded mid-call — discarding stale mute/deafen state"); + return; + } + // Always enforce mute at the track level even if no pipeline exists yet. // setMicrophoneEnabled(false) doesn't guarantee mediaStreamTrack.enabled=false, // and renegotiation when a new participant joins can bring a track back alive. @@ -1381,6 +1439,13 @@ export class LiveKitSession { this.clearTokenRefreshTimer(); this._audioPipeline.teardownAudioPipeline(); this._eventHandlers.removeAutoplayUnlock(); + // OC-0042: bump first, mirroring doDisableCamera/doDisableScreenshare — + // a concurrent enableCamera()/enableScreenshare() still awaiting device + // acquisition (getUserMedia/getDisplayMedia/publishTrack) when the user + // leaves voice must detect it was superseded and discard its track + // instead of publishing onto the room this leave already disconnected. + bumpGeneration(this._cameraState); + bumpGeneration(this._screenState); // Clean up manually published tracks. stopManualCameraTrack(this._cameraState, this._room); stopManualScreenTracks(this._screenState, this._room); diff --git a/Client/tauri-client/src/lib/screenShare.ts b/Client/tauri-client/src/lib/screenShare.ts index 1de7314b..5a194ef0 100644 --- a/Client/tauri-client/src/lib/screenShare.ts +++ b/Client/tauri-client/src/lib/screenShare.ts @@ -161,7 +161,10 @@ interface GenerationGuarded { generation?: number; } -function bumpGeneration(state: GenerationGuarded): void { +/** Exported so callers that stop manual tracks outside disableCamera/ + * disableScreenshare (leaveVoice, teardownForReconnect) can supersede an + * in-flight enable() the same way — see the doc comment above. */ +export function bumpGeneration(state: GenerationGuarded): void { state.generation = (state.generation ?? 0) + 1; } diff --git a/Client/tauri-client/src/lib/types.ts b/Client/tauri-client/src/lib/types.ts index bbd61c97..9af6b749 100644 --- a/Client/tauri-client/src/lib/types.ts +++ b/Client/tauri-client/src/lib/types.ts @@ -256,6 +256,14 @@ export interface AuthOkPayload { readonly user: UserWithRole; readonly server_name: string; readonly motd: string; + /** + * Which reconnection tier served this auth_ok — "none" (fresh connect or + * full re-sync), "buffer" (ring-buffer resume), or "db" (persisted-event + * resume). Absent on some older servers. ws.ts resets its lastSeq + * watermark on "none" since a full re-sync means the server's own seq + * counter may have restarted below the client's stale watermark. + */ + readonly replay_source?: "none" | "buffer" | "db"; } export interface AuthErrorPayload { diff --git a/Client/tauri-client/src/lib/ws.ts b/Client/tauri-client/src/lib/ws.ts index bda148e4..69a9e55f 100644 --- a/Client/tauri-client/src/lib/ws.ts +++ b/Client/tauri-client/src/lib/ws.ts @@ -312,6 +312,17 @@ export function createWsClient() { lastSeq, }); } + // A full re-sync ("none") means the server built this ready state from + // scratch — its own seq counter may have restarted below our stale + // watermark (event persistence disabled, pruned events table, restored + // DB). Keeping the old watermark would make every future reconnect + // request a range the server can silently satisfy as a complete resume + // once its counter climbs back through it, dropping the events in + // between. Reset so the next sequenced frame re-adopts the server's + // current epoch via the normal seq > lastSeq update (OC-0032). + if (msg.payload.replay_source === "none") { + lastSeq = 0; + } // Clear dedup cache — replay is complete replayDedup = null; setState("connected"); diff --git a/Client/tauri-client/src/pages/MainPage.ts b/Client/tauri-client/src/pages/MainPage.ts index d85c6d93..b072afec 100644 --- a/Client/tauri-client/src/pages/MainPage.ts +++ b/Client/tauri-client/src/pages/MainPage.ts @@ -147,6 +147,11 @@ export function createMainPage(options: MainPageOptions): MountableComponent { * minutes. Started once the socket is up, torn down with the page. */ let autoIdle: AutoIdleController | null = null; + // Pending retry for a presence_update the 1-per-10s limiter dropped (see + // applyPresence below). Module-scoped so a second dropped frame can + // supersede the first instead of stacking retries. + let presenceRetry: ReturnType | null = null; + // Toast container for user-facing error feedback let toast: ToastContainer | null = null; @@ -181,24 +186,35 @@ export function createMainPage(options: MainPageOptions): MountableComponent { const status = loadUserStatus(); const serverStatus = authStore.getState().user?.status; if (serverStatus === status) return; - const userId = getCurrentUserId(); - if (userId !== 0) { - updatePresence(userId, status); - } - if (limiters.presence.tryConsume()) { - ws.send({ type: "presence_update", payload: { status } }); - } + applyPresence(status); } /** Send a presence change and reflect it locally. Shared by the settings - * tab, the user bar and the auto-idle timer so all three agree. */ + * tab, the user bar and the auto-idle timer so all three agree. + * + * The presence limiter is 1 token per 10s, and auto-idle's return-to- + * online fires unthrottled milliseconds after its own idle transition + * (autoIdle.ts) — routinely losing the token race. Dropping that frame + * silently would leave the server, and everyone else's member list, + * stuck on "idle" with nothing left to correct it. Retry once the window + * reopens instead, re-reading the status at that time so a burst of + * calls in between coalesces onto one retry carrying the latest value. */ function applyPresence(status: UserStatus): void { const userId = getCurrentUserId(); if (userId !== 0) { updatePresence(userId, status); } + if (presenceRetry !== null) { + clearTimeout(presenceRetry); + presenceRetry = null; + } if (limiters.presence.tryConsume()) { ws.send({ type: "presence_update", payload: { status } }); + } else { + presenceRetry = setTimeout(() => { + presenceRetry = null; + applyPresence(loadUserStatus()); + }, limiters.presence.getRemainingMs()); } } @@ -259,6 +275,7 @@ export function createMainPage(options: MainPageOptions): MountableComponent { about: null, joinDate: null, }, + host: apiConfig.host ?? "", onClose: () => { dmProfileSidebar?.destroy?.(); dmProfileSidebar = null; @@ -787,6 +804,10 @@ export function createMainPage(options: MainPageOptions): MountableComponent { closeActiveLightbox(); autoIdle?.destroy(); autoIdle = null; + if (presenceRetry !== null) { + clearTimeout(presenceRetry); + presenceRetry = null; + } channelCtrl?.destroyChannel(); channelCtrl = null; diff --git a/Client/tauri-client/src/stores/members.store.ts b/Client/tauri-client/src/stores/members.store.ts index d8ea10a1..637b84db 100644 --- a/Client/tauri-client/src/stores/members.store.ts +++ b/Client/tauri-client/src/stores/members.store.ts @@ -27,11 +27,13 @@ export interface Member { export interface MembersState { readonly members: ReadonlyMap; readonly typingUsers: ReadonlyMap>; // channelId -> Set - /** Monotonic counter bumped only when membership or a member's role changes - * (setMembers/addMember/removeMember/updateMemberRole). Subscribers that - * only care about role composition (e.g. MessageList role colors) select - * this instead of rebuilding a role map on every presence/typing update. - * Optional only so the many inline test fixtures need not restate it. */ + /** Monotonic counter bumped when membership, a member's role, or a member's + * profile (username/displayName/avatar) changes (setMembers/addMember/ + * removeMember/updateMemberRole/updateMemberProfile). Subscribers that only + * care about identity, not presence/typing (e.g. MessageList repainting + * author names and avatars), select this instead of rebuilding on every + * presence/typing update. Optional only so the many inline test fixtures + * need not restate it. */ readonly roleRevision?: number; } @@ -149,7 +151,7 @@ export function updateMemberProfile(userId: number, patch: MemberProfilePatch): ? existing.identityPublicKey : patch.identityPublicKey, }); - return { ...prev, members: next }; + return { ...prev, members: next, roleRevision: (prev.roleRevision ?? 0) + 1 }; }); } diff --git a/Client/tauri-client/src/styles/app.css b/Client/tauri-client/src/styles/app.css index 8ca4355e..9ae476ad 100644 --- a/Client/tauri-client/src/styles/app.css +++ b/Client/tauri-client/src/styles/app.css @@ -5227,10 +5227,16 @@ ul.md-list-nested { } /* Accessibility: high contrast */ -.high-contrast { - --text-normal: #ffffff; - --text-muted: #cccccc; - --bg-active: rgba(255, 255, 255, 0.15); +/* applyTheme() and applyThemeByName() write theme tokens as *inline* styles + on documentElement (built-in themes) and body (custom themes) -- an + inline declaration beats a plain class rule on the same element, so these + overrides must be !important and must target both elements or they're a + silent no-op. */ +.high-contrast, +.high-contrast body { + --text-normal: #ffffff !important; + --text-muted: #cccccc !important; + --bg-active: rgba(255, 255, 255, 0.15) !important; } /* Accessibility: large font */ diff --git a/Client/tauri-client/src/styles/base.css b/Client/tauri-client/src/styles/base.css index 18300c70..dacd5597 100644 --- a/Client/tauri-client/src/styles/base.css +++ b/Client/tauri-client/src/styles/base.css @@ -19,7 +19,7 @@ body { body { font-family: var(--font-body); - font-size: 14px; + font-size: var(--font-size, 14px); color: var(--text-normal); background: var(--bg-tertiary); -webkit-font-smoothing: antialiased; diff --git a/Client/tauri-client/tests/unit/appearance-high-contrast.test.ts b/Client/tauri-client/tests/unit/appearance-high-contrast.test.ts new file mode 100644 index 00000000..ac1a7922 --- /dev/null +++ b/Client/tauri-client/tests/unit/appearance-high-contrast.test.ts @@ -0,0 +1,50 @@ +// jsdom never applies app.css, so a computed-style assertion here would pass +// whether or not the rules exist (see status-picker-userbar.test.ts for the +// same pattern). Instead this pins the CSS *source*. +// +// applyTheme() (components/settings/helpers.ts) writes theme tokens -- +// including --text-normal -- as an *inline* style on document.documentElement. +// An inline declaration always beats a plain class rule on the same element, +// so `.high-contrast { --text-normal: ... }` can never win against it: the +// High Contrast toggle's headline promise (pure-white body text) is a no-op +// unless the override is `!important`. +// +// applyThemeByName() (lib/themes.ts) does the same thing for custom themes, +// except it writes the inline override on document.body instead of +// documentElement -- so the override also needs a selector that reaches body +// while high-contrast is active, not just one that targets html. +import { readFileSync } from "node:fs"; +import { join } from "node:path"; +import { describe, it, expect } from "vitest"; + +describe("high-contrast CSS overrides beat inline theme styles", () => { + const css = readFileSync(join(process.cwd(), "src/styles/app.css"), "utf8"); + + function highContrastRuleBody(): string { + const match = /\.high-contrast[^{]*\{([^}]*)\}/.exec(css); + expect(match, "expected a `.high-contrast { ... }` rule in app.css").not.toBeNull(); + return match![1]!; + } + + it("declares --text-normal, --text-muted, and --bg-active with !important", () => { + const body = highContrastRuleBody(); + + for (const prop of ["--text-normal", "--text-muted", "--bg-active"]) { + const declaration = new RegExp(`${prop}\\s*:[^;]*;`).exec(body); + expect(declaration, `expected a declaration for ${prop}`).not.toBeNull(); + expect( + declaration![0], + `${prop} must be !important -- otherwise applyTheme()'s inline style on ` + + `documentElement always wins and the toggle does nothing`, + ).toMatch(/!important/); + } + }); + + it("also reaches document.body, where custom-theme inline overrides live", () => { + expect( + css, + "expected a selector reaching body while .high-contrast is active (e.g. " + + "`.high-contrast body`), otherwise custom themes' inline body vars never see the override", + ).toMatch(/\.high-contrast[^{,]*body\s*[{,]/); + }); +}); diff --git a/Client/tauri-client/tests/unit/attachments-auth.test.ts b/Client/tauri-client/tests/unit/attachments-auth.test.ts index c17b374e..3dd0f788 100644 --- a/Client/tauri-client/tests/unit/attachments-auth.test.ts +++ b/Client/tauri-client/tests/unit/attachments-auth.test.ts @@ -43,6 +43,7 @@ vi.stubGlobal("indexedDB", { import { clearAttachmentCaches, fetchImageAsDataUrl, + isTrustedServerUrl, setServerHost, } from "../../src/components/message-list/attachments"; @@ -98,4 +99,24 @@ describe("attachment fetch authentication", () => { expect(ensureHttpProxyMock).not.toHaveBeenCalled(); expect(fetchMock).toHaveBeenCalledWith("https://cdn.external.example/image.png"); }); + + it("still routes through the TOFU proxy with a bearer token when the host is stored with an explicit :443", async () => { + setServerHost("chat.example.com:443"); + getTokenMock.mockReturnValue("session-token"); + ensureHttpProxyMock.mockResolvedValue("http://127.0.0.1:49812"); + fetchMock.mockResolvedValue(imageResponse()); + + const result = await fetchImageAsDataUrl("https://chat.example.com/api/v1/files/abc-789"); + + expect(result).not.toBeNull(); + expect(ensureHttpProxyMock).toHaveBeenCalledWith("chat.example.com"); + expect(fetchMock).toHaveBeenCalledWith("http://127.0.0.1:49812/api/v1/files/abc-789", { + headers: { Authorization: "Bearer session-token" }, + }); + }); + + it("treats a :443-suffixed stored host as trusted for the port-less resolved URL", () => { + setServerHost("chat.example.com:443"); + expect(isTrustedServerUrl("https://chat.example.com/api/v1/files/abc")).toBe(true); + }); }); diff --git a/Client/tauri-client/tests/unit/base-font-size-css.test.ts b/Client/tauri-client/tests/unit/base-font-size-css.test.ts new file mode 100644 index 00000000..34bfd90b --- /dev/null +++ b/Client/tauri-client/tests/unit/base-font-size-css.test.ts @@ -0,0 +1,39 @@ +// jsdom never applies base.css (see appearance-high-contrast.test.ts for the +// same pattern), so this pins the CSS *source* rather than computed style. +// +// Three code paths write the `--font-size` custom property: +// - applyStoredAppearance() (lib/appearance.ts) at startup +// - the Appearance-tab Font Size slider (components/settings/AppearanceTab.ts) +// - the Accessibility-tab "Large Font" toggle, via the `.large-font` class +// (app.css: `.large-font { --font-size: 18px; }`) +// but nothing in the stylesheets ever *reads* var(--font-size) -- base.css +// hardcodes `body { font-size: 14px }` as a literal. Both controls persist +// state and change nothing rendered. +import { readFileSync } from "node:fs"; +import { join } from "node:path"; +import { describe, it, expect } from "vitest"; + +describe("body font-size consumes the --font-size custom property", () => { + const css = readFileSync(join(process.cwd(), "src/styles/base.css"), "utf8"); + + function bodyRuleBody(): string { + // The bare `body { ... }` rule (not the `html,\nbody { ... }` reset above + // it) -- require a preceding closing brace so we don't match mid-selector + // "html,\nbody {". + const match = /(?<=\})\s*body\s*\{([^}]*)\}/.exec(css); + expect(match, "expected a `body { ... }` rule in base.css").not.toBeNull(); + return match![1]!; + } + + it("declares font-size via var(--font-size, ...), not a hardcoded literal", () => { + const body = bodyRuleBody(); + const declaration = /font-size\s*:[^;]*;/.exec(body); + expect(declaration, "expected a font-size declaration on body").not.toBeNull(); + expect( + declaration![0], + "body's font-size must read var(--font-size, ...) -- otherwise the Font Size " + + "slider and the Large Font accessibility toggle write --font-size and nothing " + + "renders differently", + ).toMatch(/var\(--font-size\b/); + }); +}); diff --git a/Client/tauri-client/tests/unit/livekit-session.test.ts b/Client/tauri-client/tests/unit/livekit-session.test.ts index a0c2c9a8..c1813424 100644 --- a/Client/tauri-client/tests/unit/livekit-session.test.ts +++ b/Client/tauri-client/tests/unit/livekit-session.test.ts @@ -27,6 +27,7 @@ const mockRoom = vi.hoisted(() => ({ setCameraEnabled: vi.fn().mockResolvedValue(undefined), getTrackPublication: vi.fn().mockReturnValue(undefined), unpublishTrack: vi.fn().mockResolvedValue(undefined), + publishTrack: vi.fn().mockResolvedValue(undefined), trackPublications: new Map(), identity: "user-1", }, @@ -189,6 +190,7 @@ globalThis.Worker = vi.fn(function (this: { terminate: () => void }) { }) as unknown as typeof Worker; // Now import +import { createLocalVideoTrack } from "livekit-client"; import { parseUserId, LiveKitSession, @@ -1224,6 +1226,74 @@ describe("LiveKitSession", () => { }); }); + describe("leaveVoice camera/screenshare generation guard (OC-0042)", () => { + it("discards a camera track whose device-acquisition await resolves after leaveVoice() ran", async () => { + session.setServerHost("localhost:7880"); + session.setWsClient({ send: vi.fn() } as any); + await session.handleVoiceToken("test-token", "/livekit", 1, "ws://localhost:7880", true); + + let resolveTrack!: (t: { kind: string; mediaStreamTrack: unknown; stop: () => void }) => void; + (createLocalVideoTrack as any).mockReturnValueOnce( + new Promise((resolve) => { + resolveTrack = resolve; + }), + ); + + // enableCamera() captures room + the pre-bump generation, then blocks + // on the camera permission prompt (createLocalVideoTrack). + const enabling = session.enableCamera(); + await vi.advanceTimersByTimeAsync(0); + + // The user leaves voice while that prompt is still pending. + session.leaveVoice(false); + + // Permission is granted after the leave. + resolveTrack({ kind: "video", mediaStreamTrack: {}, stop: vi.fn() }); + await enabling; + + // Without a generation bump in leaveVoice(), the stale enable would + // still publish onto the room leaveVoice() already disconnected. + expect(mockRoom.localParticipant.publishTrack).not.toHaveBeenCalled(); + }); + }); + + describe("teardownForReconnect camera/screenshare generation guard (OC-0080)", () => { + it("discards a camera track whose device-acquisition await resolves after an unexpected disconnect", async () => { + session.setServerHost("localhost:7880"); + session.setWsClient({ send: vi.fn() } as any); + + let disconnectedHandler: ((reason?: number) => void) | undefined; + mockRoom.on.mockImplementation((event: string, handler: any) => { + if (event === "disconnected") disconnectedHandler = handler; + return mockRoom; + }); + + await session.handleVoiceToken("test-token", "/livekit", 1, "ws://localhost:7880", true); + expect(disconnectedHandler).toBeDefined(); + + let resolveTrack!: (t: { kind: string; mediaStreamTrack: unknown; stop: () => void }) => void; + (createLocalVideoTrack as any).mockReturnValueOnce( + new Promise((resolve) => { + resolveTrack = resolve; + }), + ); + + const enabling = session.enableCamera(); + await vi.advanceTimersByTimeAsync(0); + + // An unexpected disconnect fires teardownForReconnect while the camera + // permission prompt is still pending. + disconnectedHandler!(/* SERVER_SHUTDOWN */ 1); + + resolveTrack({ kind: "video", mediaStreamTrack: {}, stop: vi.fn() }); + await enabling; + + // Without a generation bump in teardownForReconnect, the stale enable + // would publish onto the room being torn down for auto-reconnect. + expect(mockRoom.localParticipant.publishTrack).not.toHaveBeenCalled(); + }); + }); + describe("handleDisconnected during initial connect", () => { it("does not null the room when connecting flag is true", async () => { session.setServerHost("localhost:7880"); @@ -1991,6 +2061,67 @@ describe("LiveKitSession", () => { }); }); + describe("restoreLocalVoiceState supersession guard (OC-0008)", () => { + it("does not apply a stale mute decision to a newer session's room", async () => { + mockVoiceState.localMuted = true; + mockVoiceState.localDeafened = false; + + let resolveMic!: () => void; + const roomA = { + localParticipant: { + setMicrophoneEnabled: vi.fn( + () => + new Promise((resolve) => { + resolveMic = resolve; + }), + ), + }, + removeAllListeners: vi.fn(), + disconnect: vi.fn().mockResolvedValue(undefined), + } as any; + const roomB = { + localParticipant: { + setMicrophoneEnabled: vi.fn().mockResolvedValue(undefined), + }, + removeAllListeners: vi.fn(), + disconnect: vi.fn().mockResolvedValue(undefined), + } as any; + + (session as any)._state = { + type: "connected", + room: roomA, + channelId: 1, + latestToken: "token-a", + lastUrl: "/livekit", + lastDirectUrl: undefined, + }; + + const restoring = (session as any).restoreLocalVoiceState("join"); + + await vi.advanceTimersByTimeAsync(0); + expect(roomA.localParticipant.setMicrophoneEnabled).toHaveBeenCalledWith(false); + + // A newer session takes over with a DIFFERENT room (e.g. the user + // switched channels while channel A's mic-permission call was pending). + (session as any)._state = { + type: "connected", + room: roomB, + channelId: 2, + latestToken: "token-b", + lastUrl: "/livekit", + lastDirectUrl: undefined, + }; + + resolveMic(); + await restoring; + + // Channel A's stale muted=true decision must not touch room B — that + // is exactly what applyMicMuteState(true) would do by re-reading + // `this._room` fresh instead of the room this call captured. + expect(roomB.localParticipant.setMicrophoneEnabled).not.toHaveBeenCalled(); + }); + }); + // ----------------------------------------------------------------------- // Mutant-killing tests: delegation methods // ----------------------------------------------------------------------- @@ -2897,6 +3028,73 @@ describe("LiveKitSession", () => { }); }); + describe("attemptAutoReconnect tail supersession guard (OC-0009)", () => { + // reconnectSuperseded() is only checked up through room.connect(); once + // the success branch sets state to "connected" it stops being usable + // (state is legitimately no longer "reconnecting" for a still-current + // attempt too). The tail must instead recheck via isStateConnected(), + // the same helper connectAndSetup's post-connect checkpoints use. + it("does not touch a newer session's timer or send a stale token refresh when superseded mid-tail", async () => { + (session as any)._state = { + type: "reconnecting", + channelId: 5, + latestToken: "reconnect-token", + lastUrl: "/livekit", + lastDirectUrl: "ws://localhost:7880", + ac: new AbortController(), + }; + session.setServerHost("localhost:7880"); + const sendSpy = vi.fn(); + session.setWsClient({ send: sendSpy } as any); + + let resolveMic!: () => void; + mockRoom.localParticipant.setMicrophoneEnabled.mockImplementationOnce( + () => + new Promise((resolve) => { + resolveMic = resolve; + }), + ); + const startTimerSpy = vi.spyOn(session as any, "startTokenRefreshTimer"); + + const ac = new AbortController(); + const reconnectPromise = (session as any).attemptAutoReconnect( + "reconnect-token", + "/livekit", + 5, + "ws://localhost:7880", + ac.signal, + ); + + // Pass the reconnect delay and let the attempt reach "connected" for + // channel 5, where it stalls inside restoreLocalVoiceState's mic-permission await. + await vi.advanceTimersByTimeAsync(3100); + expect((session as any)._state.type).toBe("connected"); + expect((session as any)._state.channelId).toBe(5); + + // A newer join supersedes it: the user switched to channel 9 while + // channel 5's reconnect tail was still stalled. + (session as any)._state = { + type: "connected", + room: mockRoom, + channelId: 9, + latestToken: "token-9", + lastUrl: "/livekit", + lastDirectUrl: "ws://localhost:7880", + }; + sendSpy.mockClear(); + + resolveMic(); + await reconnectPromise; + + // The stale channel-5 tail must not re-arm the shared timer or send a + // token-refresh request against channel 9's live session. + expect(startTimerSpy).not.toHaveBeenCalled(); + expect(sendSpy).not.toHaveBeenCalledWith({ type: "voice_token_refresh", payload: {} }); + + startTimerSpy.mockRestore(); + }); + }); + describe("token refresh timer", () => { // OC-0014: the server mints LiveKit tokens with a 5-minute TTL // (Server/ws/livekit.go tokenTTL). If the client's only periodic diff --git a/Client/tauri-client/tests/unit/main-page.test.ts b/Client/tauri-client/tests/unit/main-page.test.ts index 26b87f53..31c41582 100644 --- a/Client/tauri-client/tests/unit/main-page.test.ts +++ b/Client/tauri-client/tests/unit/main-page.test.ts @@ -55,8 +55,17 @@ vi.mock("@lib/audioElements", () => ({ setAudioVolumeHost: mockSetAudioVolumeHost, })); +const { capturedAutoIdleOptions } = vi.hoisted(() => ({ + capturedAutoIdleOptions: { + current: null as null | { onStatusChange: (status: string) => void }, + }, +})); + vi.mock("@lib/autoIdle", () => ({ - startAutoIdle: vi.fn(() => ({ destroy: vi.fn() })), + startAutoIdle: (options: { onStatusChange: (status: string) => void }) => { + capturedAutoIdleOptions.current = options; + return { destroy: vi.fn() }; + }, })); const { @@ -162,6 +171,7 @@ import type { WsClient, WsListener, ConnectionState } from "../../src/lib/ws"; import type { ApiClient } from "../../src/lib/api"; import type { ServerMessage } from "../../src/lib/types"; import { openImageLightbox } from "../../src/components/message-list/media"; +import { saveUserStatus } from "../../src/lib/userStatus"; function resetStores(): void { channelsStore.setState(() => ({ channels: new Map(), activeChannelId: null, roles: [] })); @@ -520,4 +530,124 @@ describe("MainPage — video grid, DM profile panel, calls, settings", () => { expect(document.body.querySelector(".image-lightbox")).toBeNull(); }); + + it("scopes DM profile notes to the connected host, like channel mutes and the NSFW gate (OC-0143)", () => { + channelsStore.setState((prev) => { + const ch = new Map(prev.channels); + ch.set(60, dmChannel(60, "dm-carol")); + return { ...prev, channels: ch, activeChannelId: 60 }; + }); + dmStore.setState(() => ({ + channels: [ + { + channelId: 60, + recipient: { id: 5, username: "carol", avatar: "", status: "online" }, + participants: [{ id: 5, username: "carol", avatar: "", status: "online" }], + name: "carol", + isGroup: false, + lastMessageId: null, + lastMessage: "", + lastMessageAt: "", + unreadCount: 0, + mentionCount: 0, + }, + ], + })); + + const hostedApi = { + getConfig: () => ({ host: "chat.example.com" }), + getReactionUsers: vi.fn(async () => ({ users: [] })), + } as unknown as ApiClient; + + page = createMainPage({ ws: fakeWs(), api: hostedApi }); + page.mount(container); + + const chatAreaOpts = mockCreateChatArea.mock.calls[0]![0]; + chatAreaOpts.onToggleDmProfile(); + + const noteEl = capturedChatAreaRef.current!.dmProfileSlot.querySelector( + '[data-testid="dps-note"]', + ) as HTMLTextAreaElement; + noteEl.value = "owes me money"; + noteEl.dispatchEvent(new Event("input")); + + try { + // Server A's note about user 5 must land under a key scoped to server A + // — the same host scoping already applied to channel mutes, the NSFW + // gate and per-user volume (setChannelMutesHost/setNsfwGateHost/ + // setAudioVolumeHost, all called with apiConfig.host above). + expect(localStorage.getItem("owncord:dm-note:chat.example.com:5")).toBe("owes me money"); + // And it must not have gone to the legacy unscoped key, which server + // B's unrelated user 5 would also read from. + expect(localStorage.getItem("owncord:dm-note:5")).toBeNull(); + } finally { + localStorage.removeItem("owncord:dm-note:chat.example.com:5"); + localStorage.removeItem("owncord:dm-note:5"); + } + }); +}); + +describe("MainPage — presence", () => { + let container: HTMLDivElement; + let page: ReturnType; + + beforeEach(() => { + resetStores(); + // Match the client's own idea of the signed-in user's status to the + // freshly-reset (default "online") saved preference, so the mount-time + // restoreSavedPresence() call (MainPage.ts:340) is the no-op it + // documents itself as being, and doesn't spend the single presence + // token before this test gets to exercise applyPresence directly. + authStore.setState((prev) => ({ + ...prev, + user: prev.user === null ? null : { ...prev.user, status: "online" }, + })); + capturedAutoIdleOptions.current = null; + container = document.createElement("div"); + document.body.appendChild(container); + vi.useFakeTimers(); + }); + + afterEach(() => { + page?.destroy?.(); + container.remove(); + vi.useRealTimers(); + localStorage.removeItem("owncord:settings:userStatus"); + localStorage.removeItem("owncord:settings:userStatusOrigin"); + }); + + it("retries the return-to-online presence_update once the limiter's window reopens instead of dropping it forever (OC-0111)", () => { + const ws = fakeWs(); + page = createMainPage({ ws, api: fakeApi() }); + page.mount(container); + + const onStatusChange = capturedAutoIdleOptions.current!.onStatusChange; + + // Auto-idle fires idle after ten quiet minutes; this consumes the single + // presence token (1 per 10s, rate-limiter.ts). + saveUserStatus("idle", "auto"); + onStatusChange("idle"); + expect(ws.send).toHaveBeenCalledWith({ + type: "presence_update", + payload: { status: "idle" }, + }); + (ws.send as ReturnType).mockClear(); + + // The very first mouse event after that flips back to online milliseconds + // later (autoIdle.ts's unthrottled return-to-activity path) — the token + // is still gone, so the frame cannot go out immediately. + saveUserStatus("online", "manual"); + onStatusChange("online"); + expect(ws.send).not.toHaveBeenCalled(); + + // Once the limiter's 10s window reopens, the deferred "online" frame must + // still go out — without a retry the server and every other client stay + // stuck on "idle" forever with no further trigger to correct it. + vi.advanceTimersByTime(10_000); + + expect(ws.send).toHaveBeenCalledWith({ + type: "presence_update", + payload: { status: "online" }, + }); + }); }); diff --git a/Client/tauri-client/tests/unit/members.store.test.ts b/Client/tauri-client/tests/unit/members.store.test.ts index 0d09288c..28217cfa 100644 --- a/Client/tauri-client/tests/unit/members.store.test.ts +++ b/Client/tauri-client/tests/unit/members.store.test.ts @@ -5,6 +5,7 @@ import { addMember, removeMember, updateMemberRole, + updateMemberProfile, updatePresence, setTyping, clearTyping, @@ -191,6 +192,35 @@ describe("members store", () => { }); }); + describe("updateMemberProfile", () => { + it("bumps roleRevision so MessageList's only members subscription re-renders", () => { + // MessageList subscribes solely to roleRevision (MessageList.ts:897-903) + // to know when to repaint rendered rows. A username/avatar/nickname + // change must bump it the same as every other mutator does, or a + // rename never repaints already-rendered messages. + setMembers([MEMBER_ALICE]); + const before = membersStore.getState().roleRevision ?? 0; + updateMemberProfile(1, { username: "alice2", avatar: "alice2.png" }); + expect(membersStore.getState().roleRevision ?? 0).toBe(before + 1); + }); + + it("updates username, avatar, and displayName of an existing member", () => { + setMembers([MEMBER_ALICE]); + updateMemberProfile(1, { username: "alice2", avatar: "alice2.png", displayName: "Al" }); + const member = membersStore.getState().members.get(1)!; + expect(member.username).toBe("alice2"); + expect(member.avatar).toBe("alice2.png"); + expect(member.displayName).toBe("Al"); + }); + + it("returns same state for unknown userId", () => { + setMembers([MEMBER_ALICE]); + const before = membersStore.getState(); + updateMemberProfile(999, { username: "ghost", avatar: null }); + expect(membersStore.getState()).toBe(before); + }); + }); + describe("updatePresence", () => { it("updates status of an existing member", () => { setMembers([MEMBER_ALICE]); diff --git a/Client/tauri-client/tests/unit/renderers.test.ts b/Client/tauri-client/tests/unit/renderers.test.ts index 951b4585..e3dc616a 100644 --- a/Client/tauri-client/tests/unit/renderers.test.ts +++ b/Client/tauri-client/tests/unit/renderers.test.ts @@ -814,6 +814,28 @@ describe("renderers", () => { ac.abort(); }); + + it("does not treat a real registered user named 'System' as a system notice", () => { + // Real user ids are DB autoincrement and start at 1 — only the + // synthetic sentinel row (id 0) is a genuine system message. A user + // who registered the display name "System" must render normally, + // with an author name and full moderation controls. + const msg = makeMessage({ + user: { id: 42, username: "System", avatar: null }, + content: "Your session was flagged — re-enter your password at evil.example", + }); + const ac = new AbortController(); + const el = renderMessage(msg, false, [msg], makeOpts(), ac.signal); + container.appendChild(el); + + expect(container.querySelector(".system-msg")).toBeNull(); + expect(container.querySelector(".msg-author")).not.toBeNull(); + // The hover action bar (react/reply/pin/…) is only built for real + // messages — renderSystemMessage returns before it exists at all. + expect(container.querySelector('[data-testid="msg-reply-1"]')).not.toBeNull(); + + ac.abort(); + }); }); // --------------------------------------------------------------------------- diff --git a/Client/tauri-client/tests/unit/settings-helpers.test.ts b/Client/tauri-client/tests/unit/settings-helpers.test.ts index 77ec490a..2c06efb2 100644 --- a/Client/tauri-client/tests/unit/settings-helpers.test.ts +++ b/Client/tauri-client/tests/unit/settings-helpers.test.ts @@ -126,6 +126,36 @@ describe("settings/helpers", () => { const root = document.documentElement; expect(root.style.getPropertyValue("--bg-primary")).toBe("#ffffff"); }); + + it("light theme overrides the dark-mode input/border/interactive tokens so composer and form fields aren't dark-on-dark", () => { + // OC-0043: the light theme only overrode 4 of ~45 tokens. --bg-input + // (used by .message-input-box, .msg-textarea, .form-input, .reply-bar-inner) + // was never overridden, so it kept tokens.css's dark default while + // --text-normal flipped to dark text -- unreadable dark-on-dark. + applyTheme("light"); + const root = document.documentElement; + const darkDefaults: Record = { + "--bg-input": "#383a40", + "--bg-hover": "#35373c", + "--bg-active": "#404249", + "--border": "#3f4147", + "--border-strong": "#4e5058", + "--text-muted": "#949ba4", + "--text-faint": "#80848e", + "--text-micro": "#6d6f78", + "--header-primary": "#f2f3f5", + "--header-secondary": "#b5bac1", + "--interactive-normal": "#b5bac1", + "--interactive-hover": "#dbdee1", + "--interactive-active": "#fff", + "--interactive-muted": "#4e5058", + }; + for (const [token, darkValue] of Object.entries(darkDefaults)) { + const applied = root.style.getPropertyValue(token); + expect(applied, `${token} must be set by the light theme`).not.toBe(""); + expect(applied, `${token} must not keep its dark-mode value`).not.toBe(darkValue); + } + }); }); describe("THEMES", () => { diff --git a/Client/tauri-client/tests/unit/ws-reconnect.test.ts b/Client/tauri-client/tests/unit/ws-reconnect.test.ts index 7b658ee8..c187f53f 100644 --- a/Client/tauri-client/tests/unit/ws-reconnect.test.ts +++ b/Client/tauri-client/tests/unit/ws-reconnect.test.ts @@ -208,6 +208,148 @@ describe("lastSeq tracking", () => { expect(authMsg.payload.last_seq).toBe(25); }); + it("resets lastSeq when auth_ok reports replay_source: none (full resync)", async () => { + client.connect({ host: "localhost:8443", token: "t" }); + await vi.advanceTimersByTimeAsync(10); + emitTauriEvent("ws-state", "open"); + + // Initial connect — lastSeq climbs to 5000 via live traffic (no seq on + // auth_ok itself, matching the real server: h.buildAuthOK never sets a + // top-level "seq" field). + emitTauriEvent( + "ws-message", + JSON.stringify({ + type: "auth_ok", + payload: { + user: { id: 1, username: "a", avatar: null, role: "admin" }, + server_name: "S", + motd: "", + replay_source: "none", + }, + }), + ); + emitTauriEvent( + "ws-message", + JSON.stringify({ + type: "chat_message", + seq: 5000, + payload: { + id: 1, + channel_id: 1, + user: { id: 1, username: "a", avatar: null }, + content: "hi", + reply_to: null, + attachments: [], + timestamp: "2026-01-01T00:00:00Z", + }, + }), + ); + + // Socket drops; server restarted meanwhile with its seq counter reset + // (event_persistence.enabled=false), so the reconnect's replay tier is a + // full resync — auth_ok comes back with replay_source: "none" again. + emitTauriEvent("ws-state", "closed"); + await vi.advanceTimersByTimeAsync(1100); + emitTauriEvent("ws-state", "open"); + + emitTauriEvent( + "ws-message", + JSON.stringify({ + type: "auth_ok", + payload: { + user: { id: 1, username: "a", avatar: null, role: "admin" }, + server_name: "S", + motd: "", + replay_source: "none", + }, + }), + ); + + // A subsequent drop must send last_seq=0 (adopting the server's new + // epoch), not the stale 5000 watermark from the old epoch. + emitTauriEvent("ws-state", "closed"); + mockInvoke.mockClear(); + await vi.advanceTimersByTimeAsync(2100); // 2nd attempt = 2s backoff + emitTauriEvent("ws-state", "open"); + + const authCall = mockInvoke.mock.calls.find( + (c) => + c[0] === "ws_send" && + typeof c[1]?.message === "string" && + (c[1].message as string).includes('"type":"auth"'), + ); + expect(authCall).toBeDefined(); + const authMsg = JSON.parse((authCall![1] as { message: string }).message); + expect(authMsg.payload.last_seq).toBe(0); + }); + + it("does NOT reset lastSeq when auth_ok reports replay_source: buffer/db (real resume)", async () => { + client.connect({ host: "localhost:8443", token: "t" }); + await vi.advanceTimersByTimeAsync(10); + emitTauriEvent("ws-state", "open"); + + emitTauriEvent( + "ws-message", + JSON.stringify({ + type: "auth_ok", + payload: { + user: { id: 1, username: "a", avatar: null, role: "admin" }, + server_name: "S", + motd: "", + replay_source: "none", + }, + }), + ); + emitTauriEvent( + "ws-message", + JSON.stringify({ + type: "chat_message", + seq: 42, + payload: { + id: 1, + channel_id: 1, + user: { id: 1, username: "a", avatar: null }, + content: "hi", + reply_to: null, + attachments: [], + timestamp: "2026-01-01T00:00:00Z", + }, + }), + ); + + emitTauriEvent("ws-state", "closed"); + await vi.advanceTimersByTimeAsync(1100); + emitTauriEvent("ws-state", "open"); + + // A real resume from the ring buffer/DB — must NOT reset lastSeq. + emitTauriEvent( + "ws-message", + JSON.stringify({ + type: "auth_ok", + payload: { + user: { id: 1, username: "a", avatar: null, role: "admin" }, + server_name: "S", + motd: "", + replay_source: "buffer", + }, + }), + ); + + emitTauriEvent("ws-state", "closed"); + mockInvoke.mockClear(); + await vi.advanceTimersByTimeAsync(2100); + emitTauriEvent("ws-state", "open"); + + const authCall = mockInvoke.mock.calls.find( + (c) => + c[0] === "ws_send" && + typeof c[1]?.message === "string" && + (c[1].message as string).includes('"type":"auth"'), + ); + const authMsg = JSON.parse((authCall![1] as { message: string }).message); + expect(authMsg.payload.last_seq).toBe(42); + }); + it("should reset lastSeq to 0 on intentional disconnect", async () => { client.connect({ host: "localhost:8443", token: "t" }); await vi.advanceTimersByTimeAsync(10); diff --git a/Server/admin/api_test.go b/Server/admin/api_test.go index a979fa88..a516f153 100644 --- a/Server/admin/api_test.go +++ b/Server/admin/api_test.go @@ -709,6 +709,43 @@ func TestAdminAPI_DeleteChannel_CleansVoiceBeforeDBDelete(t *testing.T) { } } +// A voice_join racing the delete window must be refused, not silently create +// a voice_states row the FK cascade then wipes out from under it (OC-0035): +// CleanupVoiceForChannel snapshots participants ONCE, up front, so a join +// that lands after that snapshot but before AdminDeleteChannel's cascade +// leaves the joiner's hub-side voice state and LiveKit session orphaned with +// nothing left to clean it up. handleDeleteChannel must close that window the +// same way the archive path does (handlePatchChannel): persist archived=true +// BEFORE evicting current participants, so voice_join's archived gate +// (ws/voice_join.go) refuses any concurrent join that reads the channel row +// during cleanup. +func TestAdminAPI_DeleteChannel_ArchivesBeforeVoiceCleanup(t *testing.T) { + database := openAdminTestDB(t) + hub := &mockHub{} + handler := admin.NewAdminAPI(database, "1.0.0", hub, nil, nil, nil, nil, newTestModService(database), newTestRoleService(database)) + token := createAdminUser(t, database) + + chID, _ := database.AdminCreateChannel(context.Background(), "del-race", "voice", "", "", 0) + + archivedAtCleanup := false + hub.onVoiceCleanup = func(channelID int64) { + ch, err := database.GetChannel(context.Background(), channelID) + if err != nil || ch == nil { + t.Fatalf("GetChannel during cleanup: %v", err) + } + archivedAtCleanup = ch.Archived + } + + w := doRequest(t, handler, http.MethodDelete, "/channels/"+itoa(chID), token, nil) + if w.Code != http.StatusNoContent { + t.Fatalf("status = %d, want 204; body: %s", w.Code, w.Body.String()) + } + + if !archivedAtCleanup { + t.Errorf("channel.Archived at CleanupVoiceForChannel time = false, want true — a concurrent voice_join would not be refused by the archived gate") + } +} + func TestAdminAPI_DeleteChannel_NotFound(t *testing.T) { database := openAdminTestDB(t) handler := admin.NewAdminAPI(database, "1.0.0", &mockHub{}, nil, nil, nil, nil, newTestModService(database), newTestRoleService(database)) diff --git a/Server/admin/handlers_channels.go b/Server/admin/handlers_channels.go index b7897e73..d9789bec 100644 --- a/Server/admin/handlers_channels.go +++ b/Server/admin/handlers_channels.go @@ -281,6 +281,33 @@ func handleDeleteChannel(database *db.DB, hub HubBroadcaster) http.HandlerFunc { } id := existing.ID + // Mark the channel archived BEFORE evicting participants, mirroring the + // archive path (handlePatchChannel): CleanupVoiceForChannel snapshots + // voice participants ONCE, up front, so a voice_join racing this delete + // could otherwise read the still-live channel row, pass the archived + // gate (ws/voice_join.go), and insert a voice_states row after the + // snapshot but before AdminDeleteChannel's cascade — leaving that + // joiner's hub-side voice state and LiveKit session orphaned with no + // DB row left for any sweep to find (OC-0035). Persisting archived=1 + // first makes voice_join's existing archived check refuse that join + // outright, the same way it already refuses one racing an archive. + if !existing.Archived { + if err := database.AdminUpdateChannel(r.Context(), id, db.ChannelUpdate{ + Name: existing.Name, + Topic: existing.Topic, + Category: existing.Category, + SlowMode: existing.SlowMode, + Position: existing.Position, + Archived: true, + NSFW: existing.NSFW, + VoiceMaxUsers: existing.VoiceMaxUsers, + VoiceMaxVideo: existing.VoiceMaxVideo, + }); err != nil { + writeErr(w, http.StatusInternalServerError, "INTERNAL_ERROR", "failed to delete channel") + return + } + } + // Evict voice participants BEFORE deleting the row: the voice_states // FK cascade wipes the rows the cleanup reads, and the stale sweeper // cannot recover participants of a channel that no longer exists. diff --git a/Server/db/auth_queries_test.go b/Server/db/auth_queries_test.go index 879609ee..912b2533 100644 --- a/Server/db/auth_queries_test.go +++ b/Server/db/auth_queries_test.go @@ -774,6 +774,41 @@ func TestListMembers_ExcludesBanned(t *testing.T) { } } +// TestListMembers_LapsedTempBan_StillIncluded locks the same "reconverged raw +// column" fix that GetUserIDsByUsernames and ListMentionTargetsByRoles already +// carry (db/mention_queries.go's notBannedClause): nothing clears users.banned +// when a temp ban's ban_expires lapses (that's decided lazily, at login, by +// auth.IsEffectivelyBanned), so a raw `banned = 0` filter leaves a reinstated +// user permanently absent from the member roster even though they can log in +// and post again. +func TestListMembers_LapsedTempBan_StillIncluded(t *testing.T) { + database := newTestDB(t) + _, _ = database.CreateUser(context.Background(), "member_visible", "hash", 4) + id2, _ := database.CreateUser(context.Background(), "member_lapsed_ban", "hash", 4) + + past := time.Now().Add(-1 * time.Hour) + if err := database.BanUser(context.Background(), id2, "temp ban", &past); err != nil { + t.Fatalf("BanUser: %v", err) + } + + members, err := database.ListMembers(context.Background()) + if err != nil { + t.Fatalf("ListMembers: %v", err) + } + found := false + for _, m := range members { + if m.Username == "member_lapsed_ban" { + found = true + } + } + if !found { + t.Error("a lapsed temp ban must not hide the user from the member roster") + } + if len(members) != 2 { + t.Errorf("ListMembers() = %d, want 2 (lapsed ban must not hide the member)", len(members)) + } +} + func TestListMembers_SortedByUsername(t *testing.T) { database := newTestDB(t) _, _ = database.CreateUser(context.Background(), "zeta_user", "hash", 4) diff --git a/Server/db/dbgen/users.sql.go b/Server/db/dbgen/users.sql.go index 231cd2ac..25d66a29 100644 --- a/Server/db/dbgen/users.sql.go +++ b/Server/db/dbgen/users.sql.go @@ -128,7 +128,7 @@ SELECT u.id, u.username, u.avatar, u.status, LOWER(r.name), u.identity_public_ke u.display_name, u.custom_status FROM users u JOIN roles r ON u.role_id = r.id -WHERE u.banned = 0 +WHERE (u.banned = 0 OR (u.ban_expires IS NOT NULL AND replace(u.ban_expires, ' ', 'T') <= strftime('%Y-%m-%dT%H:%M:%SZ', 'now'))) ORDER BY u.username ASC ` diff --git a/Server/db/queries/sqlite/users.sql b/Server/db/queries/sqlite/users.sql index 13221b0a..4da254d1 100644 --- a/Server/db/queries/sqlite/users.sql +++ b/Server/db/queries/sqlite/users.sql @@ -55,7 +55,7 @@ SELECT u.id, u.username, u.avatar, u.status, LOWER(r.name), u.identity_public_ke u.display_name, u.custom_status FROM users u JOIN roles r ON u.role_id = r.id -WHERE u.banned = 0 +WHERE (u.banned = 0 OR (u.ban_expires IS NOT NULL AND replace(u.ban_expires, ' ', 'T') <= strftime('%Y-%m-%dT%H:%M:%SZ', 'now'))) ORDER BY u.username ASC; -- name: CountUsers :one diff --git a/Server/service/message_crud.go b/Server/service/message_crud.go index 8b8318a4..e3ac427d 100644 --- a/Server/service/message_crud.go +++ b/Server/service/message_crud.go @@ -52,14 +52,6 @@ func (s *MessageService) SendMessage(ctx context.Context, p SendMessageParams) ( return nil, err } - // Slow mode (non-DM only). - if !isDM && ch.SlowMode > 0 && !s.perms.HasChannelPerm(ctx, p.UserID, p.ChannelID, permissions.ManageMessages) { - slowKey := auth.Key(auth.Key("slow", p.UserID), p.ChannelID) - if s.limiter != nil && !s.limiter.Allow(slowKey, 1, time.Duration(ch.SlowMode)*time.Second) { - return nil, fmt.Errorf("%w: channel has %ds slow mode", ErrSlowMode, ch.SlowMode) - } - } - // Validate and sanitize content. content, err := sanitizeContent(p.Content, len(p.AttachmentIDs) > 0) if err != nil { @@ -73,6 +65,19 @@ func (s *MessageService) SendMessage(ctx context.Context, p SendMessageParams) ( } } + // Slow mode (non-DM only). Deliberately checked last, after content and + // attachment validation: Allow() below records the cooldown timestamp the + // instant it returns true, so a send that fails validation after this + // point must not have already spent the once-per-window token — that + // would lock the composer for up to ch.SlowMode seconds for a send that + // never actually posted anything. + if !isDM && ch.SlowMode > 0 && !s.perms.HasChannelPerm(ctx, p.UserID, p.ChannelID, permissions.ManageMessages) { + slowKey := auth.Key(auth.Key("slow", p.UserID), p.ChannelID) + if s.limiter != nil && !s.limiter.Allow(slowKey, 1, time.Duration(ch.SlowMode)*time.Second) { + return nil, fmt.Errorf("%w: channel has %ds slow mode", ErrSlowMode, ch.SlowMode) + } + } + // Resolve mentions against the sanitized content, before the insert, so the // row and its mention set are written together. Unknown @words and an // unauthorized @everyone resolve to nothing and stay plain text. @@ -122,7 +127,11 @@ func (s *MessageService) SendMessage(ctx context.Context, p SendMessageParams) ( return nil, fmt.Errorf("%w: message content cannot be empty", ErrBadRequest) } if linked > 0 { - attMap, attErr := s.st.GetAttachmentsByMessageIDs(ctx, []int64{msgID}) + // Detached from ctx for the same reason as the compensating deletes + // above: the link already committed, so a request ctx canceled the + // instant it returns (sender disconnects right after) must not turn + // a successful attachment-only send into a blank broadcast bubble. + attMap, attErr := s.st.GetAttachmentsByMessageIDs(context.WithoutCancel(ctx), []int64{msgID}) if attErr != nil { slog.Error("MessageService.SendMessage GetAttachments", "err", attErr) } else { diff --git a/Server/service/message_crud_test.go b/Server/service/message_crud_test.go index 1e4ec556..756cd976 100644 --- a/Server/service/message_crud_test.go +++ b/Server/service/message_crud_test.go @@ -9,8 +9,10 @@ package service import ( "context" "errors" + "strings" "testing" + "github.com/owncord/server/auth" "github.com/owncord/server/db" "github.com/owncord/server/permissions" ) @@ -306,3 +308,97 @@ func TestDeleteMessage_FailsClosedWhenChannelLookupErrors(t *testing.T) { t.Fatalf("message must survive the refused delete; GetMessage: msg=%v err=%v", msg, err) } } + +// OC-0036: slow mode's cooldown token is spent by limiter.Allow, which must +// only run once the send has passed content/attachment validation. Consuming +// it earlier means a send that gets rejected for an unrelated reason (content +// too long, in this case) still locks the composer for the full slow-mode +// window even though nothing was ever posted. +func TestSendMessage_SlowModeNotConsumedByFailedContentValidation(t *testing.T) { + _, database := newTestMessageService(t) + if err := database.SetChannelSlowMode(context.Background(), 10, 3600); err != nil { + t.Fatalf("SetChannelSlowMode: %v", err) + } + checker := permissions.NewChecker(database) + permSvc := NewPermissionService(database, checker) + svc := NewMessageService(database, permSvc, auth.NewRateLimiter()) + ctx := context.Background() + + overLong := strings.Repeat("a", maxMessageLen+1) + if _, err := svc.SendMessage(ctx, SendMessageParams{ + ChannelID: 10, UserID: 1, Username: "alice", Content: overLong, + }); !errors.Is(err, ErrBadRequest) { + t.Fatalf("over-length send: err = %v, want ErrBadRequest", err) + } + + // The rejected send above must not have spent the once-per-hour slow-mode + // token: a valid, short send immediately after should still go through. + if _, err := svc.SendMessage(ctx, SendMessageParams{ + ChannelID: 10, UserID: 1, Username: "alice", Content: "hi", + }); err != nil { + t.Fatalf("SendMessage right after a rejected over-length send: %v — slow mode must only be "+ + "charged once a send clears content/attachment validation, not before", err) + } +} + +// disconnectAfterLinkStore models a client whose connection drops the instant +// LinkAttachmentsToMessage commits — mirrors disconnectAfterWriteStore but for +// the attachment path. GetAttachmentsByMessageIDs is overridden to fail +// whenever handed an already-canceled context, so a test can tell whether the +// post-link attachment read used the (canceled) request ctx or a detached one. +type disconnectAfterLinkStore struct { + Store + cancel context.CancelFunc +} + +func (s disconnectAfterLinkStore) LinkAttachmentsToMessage(ctx context.Context, messageID, uploaderID int64, attachmentIDs []string) (int64, error) { + n, err := s.Store.LinkAttachmentsToMessage(ctx, messageID, uploaderID, attachmentIDs) + s.cancel() + return n, err +} + +func (s disconnectAfterLinkStore) GetAttachmentsByMessageIDs(ctx context.Context, msgIDs []int64) (map[int64][]db.AttachmentInfo, error) { + if ctx.Err() != nil { + return nil, ctx.Err() + } + return s.Store.GetAttachmentsByMessageIDs(ctx, msgIDs) +} + +// OC-0128: a sender whose connection drops the instant the attachment link +// commits must still get the linked attachment back on the broadcast result — +// not a message with no content and no attachments. The post-link read must +// run on a detached ctx, the same way the compensating deletes in SendMessage +// already do. +func TestSendMessage_AttachmentsSurviveSenderDisconnectAfterLink(t *testing.T) { + _, database := newTestMessageService(t) + // Grant ATTACH_FILES on top of the base member perms newTestMessageService seeds. + seedRole(t, database, &db.Role{ + ID: permissions.MemberRoleID, + Name: "member", + Permissions: permissions.SendMessages | permissions.ReadMessages | permissions.AddReactions | permissions.AttachFiles, + Position: 1, + }) + if _, err := database.ExecContext(context.Background(), + `INSERT INTO attachments (id, uploader_id, filename, stored_as, mime_type, size) + VALUES (?, ?, ?, ?, ?, ?)`, + "att-1", 1, "photo.png", "stored-photo.png", "image/png", 100, + ); err != nil { + t.Fatalf("seed attachment: %v", err) + } + checker := permissions.NewChecker(database) + permSvc := NewPermissionService(database, checker) + + ctx, cancel := context.WithCancel(context.Background()) + svc := NewMessageService(disconnectAfterLinkStore{Store: database, cancel: cancel}, permSvc, nil) + + result, err := svc.SendMessage(ctx, SendMessageParams{ + ChannelID: 10, UserID: 1, Username: "alice", AttachmentIDs: []string{"att-1"}, + }) + if err != nil { + t.Fatalf("SendMessage: %v", err) + } + if len(result.Attachments) != 1 { + t.Fatalf("Attachments = %v, want 1 — a disconnect right after the attachment link commits must not "+ + "broadcast a blank message bubble", result.Attachments) + } +} diff --git a/Server/ws/harvest_s5_internal_test.go b/Server/ws/harvest_s5_internal_test.go index e7ac9052..05f1462e 100644 --- a/Server/ws/harvest_s5_internal_test.go +++ b/Server/ws/harvest_s5_internal_test.go @@ -108,11 +108,19 @@ func TestSweepStaleVoiceStates_EvictionIsScopedToCheckedChannel(t *testing.T) { } } -// When a voice channel switch aborts because the old row's delete failed, -// the abort branch restores the in-memory voice state — and must restore the -// voice-topic subscription and key-holder entry torn down with it, or the -// client silently misses every voice_e2ee relay for the session it is still in. -func TestHandleVoiceJoin_AbortedSwitchRestoresVoiceTopicSubscription(t *testing.T) { +// When a voice channel switch aborts because the old row's delete failed, the +// abort branch used to restore the in-memory voice state, voice-topic +// subscription and key-holder entry torn down by the leave that preceded it. +// OC-0034: that restore was itself the bug. handleVoiceLeave's +// finishVoiceLeave always broadcasts voice_leave for the old channel to the +// leaver themselves (voice_leave.go), and the client tears its own session +// down on a self voice_leave — so by the time the abort branch runs, every +// client including this user's own has already forgotten the old membership. +// Restoring the server's view of it resurrects a session nobody else +// believes exists, with no re-broadcast to tell them otherwise. The fix +// leaves the client's voice state cleared on abort so it agrees with the +// voice_leave already sent; the periodic sweep reaps the orphaned DB row. +func TestHandleVoiceJoin_AbortedSwitchDoesNotResurrectVoiceTopicSubscription(t *testing.T) { ctx := context.Background() database := newHarvestVoiceDB(t) uid := seedHarvestVoiceUser(t, database, "abort-switch") @@ -160,14 +168,14 @@ func TestHandleVoiceJoin_AbortedSwitchRestoresVoiceTopicSubscription(t *testing. h.handleVoiceJoin(ctx, c, json.RawMessage(fmt.Sprintf(`{"channel_id": %d}`, chB))) - if got := c.getVoiceChID(); got != chA { - t.Fatalf("aborted switch left client voice state at %d, want restored channel %d", got, chA) + if got := c.getVoiceChID(); got != 0 { + t.Fatalf("aborted switch resurrected client voice state at channel %d, want 0 — voice_leave for channel %d was already broadcast to this client (OC-0034)", got, chA) } - if !h.SubscribedToVoiceTopicForTest(c, chA) { - t.Error("aborted switch did not re-subscribe the client to its channel's voice topic — every voice_e2ee relay for the restored session is silently dropped") + if h.SubscribedToVoiceTopicForTest(c, chA) { + t.Error("aborted switch re-subscribed the client to a voice topic for a channel it already received voice_leave for") } - if !h.IsVoiceKeyHolder(chA, uid) { - t.Error("aborted switch left the key-holder map without the channel's only participant") + if h.IsVoiceKeyHolder(chA, uid) { + t.Error("aborted switch left the client named as key holder for a channel it already left") } } diff --git a/Server/ws/hub.go b/Server/ws/hub.go index 944fe9d2..41e2fea3 100644 --- a/Server/ws/hub.go +++ b/Server/ws/hub.go @@ -545,6 +545,23 @@ func (h *Hub) unregisterNow(c *Client) bool { return exists } +// shouldMarkOffline reports whether a disconnect teardown should run +// MarkUserDisconnected and broadcast an offline presence for c's user. +// +// `replaced` (unregisterNow's return, sampled once at the start of teardown) +// is necessary but not sufficient: both readPump's defer and +// unregisterFailedHandshake sample it BEFORE handleVoiceLeave, which can +// block for seconds (DB delete, audience scan, a LiveKit call bounded by +// lkTimeout=5s). A reconnect landing during that window registers a new +// client for the same user and is invisible to the stale boolean, so the +// dead connection's teardown would otherwise mark the live session offline +// (OC-0019). Re-checking h.clients at decision time closes that gap: any +// entry present once c has been removed is necessarily a newer connection — +// unregisterNow only ever deletes c's own slot, never someone else's. +func (h *Hub) shouldMarkOffline(c *Client, replaced bool) bool { + return !replaced && h.GetClient(c.userID) == nil +} + // ClientCount returns the number of currently registered clients (test helper). func (h *Hub) ClientCount() int { h.mu.RLock() diff --git a/Server/ws/hub_broadcast.go b/Server/ws/hub_broadcast.go index 7ad8d734..f886802c 100644 --- a/Server/ws/hub_broadcast.go +++ b/Server/ws/hub_broadcast.go @@ -79,6 +79,35 @@ func (h *Hub) broadcastVoiceEvent(ctx context.Context, channelID int64, msg []by h.broadcastChannelScopedTo(channelID, msg, audience, "voice event") } +// broadcastVoiceEventWithLeaver is broadcastVoiceEvent extended to guarantee +// leaverID is in the audience even though the caller has already cleared +// their client-side voice state — which means broadcastVoiceEvent's own +// still-in-the-room participant union can no longer see them. Every path +// that tears down a voice participant whose client state is cleared before +// the voice_leave goes out needs this: voice membership is gated on +// CONNECT_VOICE alone, so a leaver without READ_MESSAGES on the channel +// would otherwise never learn the server already ended their call. Mirrors +// CleanupVoiceForChannel's per-batch leaver union, for the single-leaver case. +func (h *Hub) broadcastVoiceEventWithLeaver(ctx context.Context, channelID int64, msg []byte, leaverID int64) { + audience := h.channelReadAudience(ctx, channelID) + seen := make(map[int64]struct{}, len(audience)+1) + for _, uid := range audience { + seen[uid] = struct{}{} + } + h.mu.RLock() + for uid, c := range h.clients { + if _, ok := seen[uid]; !ok && c.getVoiceChID() == channelID { + seen[uid] = struct{}{} + audience = append(audience, uid) + } + } + h.mu.RUnlock() + if _, ok := seen[leaverID]; !ok { + audience = append(audience, leaverID) + } + h.broadcastChannelScopedTo(channelID, msg, audience, "voice event") +} + // broadcastChannelScoped enqueues msg for exactly the connected clients whose // current role may READ channelID, tagged with that channel id so reconnect // replay filters it too (EventsSinceFiltered replays a channelID of 0 diff --git a/Server/ws/livekit_test.go b/Server/ws/livekit_test.go index 067ebc8c..ad61e6d9 100644 --- a/Server/ws/livekit_test.go +++ b/Server/ws/livekit_test.go @@ -13,6 +13,7 @@ import ( "github.com/go-chi/chi/v5" "github.com/owncord/server/config" + "github.com/owncord/server/permissions" "github.com/owncord/server/ws" ) @@ -565,6 +566,55 @@ func TestWebhook_ParticipantLeft_ClearsE2EEState_OnMatch(t *testing.T) { } } +// TestWebhook_ParticipantLeft_LeaverWithoutReadStillNotified locks OC-0038: +// voice membership is gated on CONNECT_VOICE alone, so a participant can be +// in a voice channel without READ_MESSAGES on it. The webhook-driven teardown +// clears the leaver's own client voice state before broadcasting, so +// broadcastVoiceEvent's audience — (READ_MESSAGES holders) ∪ (still-in-the- +// room participants) — can no longer see them, and they never learn the +// server already tore down their call. finishVoiceLeave and +// CleanupVoiceForChannel both add the leaver to the audience for exactly +// this reason; the webhook path must too. +func TestWebhook_ParticipantLeft_LeaverWithoutReadStillNotified(t *testing.T) { + t.Parallel() + hub, database := newVoiceHub(t) + + chanID := seedVoiceChannel(t, database, "webhook-noread-ch") + + // Role 3 (Moderator) carries CONNECT_VOICE in its default mask but lacks + // the Administrator bit, so a channel-scoped READ_MESSAGES deny actually + // applies — an Owner/Admin role would bypass channel_overrides entirely. + if _, err := database.CreateUser(context.Background(), "webhook-noread-user", "hash", 3); err != nil { + t.Fatalf("CreateUser: %v", err) + } + leaver, err := database.GetUserByUsername(context.Background(), "webhook-noread-user") + if err != nil || leaver == nil { + t.Fatalf("GetUserByUsername: %v", err) + } + if err := database.UpsertChannelOverride(context.Background(), chanID, 3, 0, permissions.ReadMessages); err != nil { + t.Fatalf("UpsertChannelOverride: %v", err) + } + + if err := database.JoinVoiceChannel(context.Background(), leaver.ID, chanID); err != nil { + t.Fatalf("JoinVoiceChannel: %v", err) + } + vs, err := database.GetVoiceState(context.Background(), leaver.ID) + if err != nil || vs == nil { + t.Fatalf("GetVoiceState: %v (nil=%v)", err, vs == nil) + } + + leaverSend := make(chan []byte, 16) + c := ws.NewTestClient(hub, leaver.ID, leaverSend) + ws.SetClientVoiceStateForTest(c, chanID, vs.JoinedAt) + hub.RegisterNowForTest(c) + + hub.HandleWebhookParticipantLeftForTest(leaver.ID, chanID, vs.JoinedAt) + + if got := countVoiceLeaves(leaverSend, 200*time.Millisecond); got == 0 { + t.Error("the leaver's own client, denied READ_MESSAGES on the voice channel, received no voice_leave after the LiveKit webhook tore down its own session") + } +} + // --------------------------------------------------------------------------- // livekit_process.go – generateConfig tests // --------------------------------------------------------------------------- diff --git a/Server/ws/livekit_webhook.go b/Server/ws/livekit_webhook.go index c323d03e..d7269508 100644 --- a/Server/ws/livekit_webhook.go +++ b/Server/ws/livekit_webhook.go @@ -129,7 +129,18 @@ func (h *Hub) handleWebhookParticipantJoined(ctx context.Context, event *livekit // so we remove the rogue participant from LiveKit. if h.db != nil { state, stateErr := h.db.GetVoiceState(ctx, userID) - if stateErr != nil || state == nil || state.ChannelID != channelID { + if stateErr != nil { + // A transient read failure (I/O error, lock contention, a + // maintenance window) is not proof of a rogue participant — + // treating it as one would eject a legitimate participant from + // the SFU on a single bad read. Mirrors sweepStaleVoiceStates' + // hasChannelPermChecked guard: skip and let the participant be; + // a later webhook retry or sweep tick resolves it. + slog.Error("livekit webhook: GetVoiceState failed, skipping rogue-participant check", + "error", stateErr, "user_id", userID, "channel_id", channelID) + return + } + if state == nil || state.ChannelID != channelID { slog.Warn("livekit webhook: rogue participant_joined — no matching voice state, removing", "user_id", userID, "channel_id", channelID) if h.livekit != nil { @@ -226,7 +237,14 @@ func (h *Hub) handleWebhookParticipantLeft(ctx context.Context, event *livekit.W // rejected with NOT_KEY_HOLDER. Safe here: no locks are held. h.updateKeyHolder(channelID) - h.broadcastVoiceEvent(ctx, channelID, buildVoiceLeave(channelID, userID)) + // The leaver's own client state was just cleared above, so + // broadcastVoiceEvent's still-in-the-room union can no longer see + // them — without broadcastVoiceEventWithLeaver's extra term, a + // participant without READ_MESSAGES on this channel (voice + // membership needs only CONNECT_VOICE) never learns the server + // already tore down their call. Mirrors finishVoiceLeave and + // CleanupVoiceForChannel, which add the leaver for the same reason. + h.broadcastVoiceEventWithLeaver(ctx, channelID, buildVoiceLeave(channelID, userID), userID) slog.Info("livekit webhook: cleaned up stale voice state", "user_id", userID, "channel_id", channelID) diff --git a/Server/ws/livekit_webhook_joined_test.go b/Server/ws/livekit_webhook_joined_test.go index 3d4525e7..a50f6705 100644 --- a/Server/ws/livekit_webhook_joined_test.go +++ b/Server/ws/livekit_webhook_joined_test.go @@ -104,6 +104,43 @@ func TestWebhook_ParticipantJoined_ValidJoinAccepted(t *testing.T) { } } +// TestWebhook_ParticipantJoined_TransientReadErrorDoesNotEvict locks OC-0065: +// a GetVoiceState read failure must not be treated as proof of a rogue +// participant. sweepStaleVoiceStates already draws this distinction via +// hasChannelPermChecked ("a transient read failure ... is not a revocation"); +// the webhook path OR'd stateErr into the same branch as "no matching row", +// so a transient DB error (SQLITE_BUSY, an I/O blip) ejected a legitimate +// participant from the SFU mid-call. +func TestWebhook_ParticipantJoined_TransientReadErrorDoesNotEvict(t *testing.T) { + hub, database := newVoiceHub(t) + user := seedVoiceOwner(t, database, "joined-dberr-user") + chanID := seedVoiceChan(t, database, "joined-dberr-ch") + + if err := database.JoinVoiceChannel(context.Background(), user.ID, chanID); err != nil { + t.Fatalf("JoinVoiceChannel: %v", err) + } + + // Fault-inject exactly the GetVoiceState read: renaming the table out from + // under the query makes it return a genuine DB error instead of the + // sql.ErrNoRows GetVoiceState collapses to (nil, nil) for a real "no + // membership" case. + if _, err := database.ExecContext(context.Background(), + `ALTER TABLE voice_states RENAME TO voice_states_offline`); err != nil { + t.Fatalf("rename voice_states: %v", err) + } + + logs := captureLogs(t) + + hub.HandleWebhookParticipantJoinedForTest( + participantIdentityFor(user.ID, "some-token"), + roomNameFor(chanID), + ) + + if out := logs(); strings.Contains(out, "rogue participant_joined") { + t.Errorf("a transient GetVoiceState error was treated as a rogue participant and evicted; log:\n%s", out) + } +} + func TestWebhook_ParticipantJoined_WrongChannelFlagged(t *testing.T) { hub, database := newVoiceHub(t) user := seedVoiceOwner(t, database, "joined-wrongch-user") diff --git a/Server/ws/serve.go b/Server/ws/serve.go index 0efef744..a5b2df0e 100644 --- a/Server/ws/serve.go +++ b/Server/ws/serve.go @@ -452,6 +452,14 @@ func (h *Hub) unregisterFailedHandshake(ctx context.Context, c *Client) { if voiceChID != 0 { h.handleVoiceLeave(cleanupCtx, c) } + } + // shouldMarkOffline re-checks h.clients rather than trusting the + // `replaced` snapshot alone: it was sampled before handleVoiceLeave, + // which can block for seconds, so a reconnect landing during that window + // would otherwise be invisible here and mark the live session's user + // offline (OC-0019, mirrored from readPump's defer in serve_pumps.go). + if h.shouldMarkOffline(c, replaced) { + cleanupCtx := context.WithoutCancel(ctx) _ = h.db.MarkUserDisconnected(cleanupCtx, c.userID) // custom_status is nil, not c.user.CustomStatus: see the identical // note in serve_pumps.go's readPump defer — that field is an diff --git a/Server/ws/serve_pumps.go b/Server/ws/serve_pumps.go index f2f22db7..d4a7b29f 100644 --- a/Server/ws/serve_pumps.go +++ b/Server/ws/serve_pumps.go @@ -183,7 +183,13 @@ func readPump(ctx context.Context, conn *websocket.Conn, hub *Hub, c *Client) { } slog.Info("websocket disconnected", attrs...) - if !replaced { + // shouldMarkOffline re-checks h.clients instead of trusting + // `replaced` alone: that flag was sampled before handleVoiceLeave, + // which can block for seconds, so a reconnect landing during that + // window would otherwise be invisible here and this dead + // connection's teardown would mark the live session's user + // offline (OC-0019). + if hub.shouldMarkOffline(c, replaced) { // A real disconnect is offline for everyone, the user // included, so this path needs no invisible mapping. The row, // however, keeps a *chosen* status (idle/dnd/invisible) diff --git a/Server/ws/serve_pumps_reconnect_race_test.go b/Server/ws/serve_pumps_reconnect_race_test.go new file mode 100644 index 00000000..06130145 --- /dev/null +++ b/Server/ws/serve_pumps_reconnect_race_test.go @@ -0,0 +1,165 @@ +package ws + +// serve_pumps_reconnect_race_test.go — regression test for OC-0019. +// +// readPump's defer snapshots `replaced := hub.unregisterNow(c)` BEFORE running +// hub.handleVoiceLeave, which can block for seconds (DB delete, audience scan, +// a LiveKit RemoveParticipant HTTP call bounded by lkTimeout=5s). The stale +// `replaced` boolean is then reused, unchecked, to decide whether to run +// MarkUserDisconnected and broadcast an offline presence. A reconnect that +// registers during that window is invisible to the stale flag: the dead +// socket's teardown marks the *live* session's user offline. + +import ( + "bytes" + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/coder/websocket" + lkproto "github.com/livekit/protocol/livekit" + "google.golang.org/protobuf/proto" + + "github.com/owncord/server/auth" + "github.com/owncord/server/config" + "github.com/owncord/server/db" +) + +// TestReadPump_ReconnectDuringVoiceCleanup_DoesNotMarkUserOffline reproduces +// the finding's repro: a client's socket drops while it holds a voice +// session, its readPump defer starts tearing down (unregisterNow already +// removed it from the hub), and — while handleVoiceLeave is still blocked on +// the LiveKit call — the same user reconnects and takes the hub slot. The +// defer must not go on to mark that user offline once it resumes. +func TestReadPump_ReconnectDuringVoiceCleanup_DoesNotMarkUserOffline(t *testing.T) { + database := newHarvestVoiceDB(t) + uid := seedHarvestVoiceUser(t, database, "reconnect-race") + chID := mustCreateVoiceChannel(t, database, "voice-race") + + ctx := context.Background() + if err := database.JoinVoiceChannel(ctx, uid, chID); err != nil { + t.Fatalf("JoinVoiceChannel: %v", err) + } + if err := database.UpdateUserStatus(ctx, uid, "online"); err != nil { + t.Fatalf("UpdateUserStatus: %v", err) + } + + // Fake LiveKit server: holds the RemoveParticipant response until the + // test releases it, giving full control over handleVoiceLeave's window. + reachedLiveKit := make(chan struct{}) + proceed := make(chan struct{}) + lkSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + close(reachedLiveKit) + <-proceed + body, _ := proto.Marshal(&lkproto.RemoveParticipantResponse{}) + w.Header().Set("Content-Type", "application/protobuf") + w.WriteHeader(http.StatusOK) + _, _ = w.Write(body) + })) + defer lkSrv.Close() + + lk, err := NewLiveKitClient(&config.VoiceConfig{ + LiveKitAPIKey: "testkeytestkeytest", + LiveKitAPISecret: "testsecrettestsecrettestsecret", + LiveKitURL: "ws://" + lkSrv.Listener.Addr().String(), + }) + if err != nil { + t.Fatalf("NewLiveKitClient: %v", err) + } + + h := NewHub(database, auth.NewRateLimiter(), nil) + h.SetLiveKit(lk) + + c := NewTestClient(h, uid, make(chan []byte, 8)) + c.user = &db.User{ID: uid, Status: "online"} + c.setVoiceState(chID, "tok-race") + h.clients[uid] = c + + // Real server-side *websocket.Conn, closed immediately so readPump's + // first Read fails and its defer runs — mirrors + // serve_reconnect_double_teardown_test.go's setup. + connCh := make(chan *websocket.Conn, 1) + wsSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + conn, acceptErr := websocket.Accept(w, r, nil) + if acceptErr != nil { + return + } + _ = conn.CloseNow() + connCh <- conn + })) + defer wsSrv.Close() + + dialCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + clientConn, resp, dialErr := websocket.Dial(dialCtx, "ws"+strings.TrimPrefix(wsSrv.URL, "http"), nil) + if resp != nil && resp.Body != nil { + _ = resp.Body.Close() + } + if dialErr != nil { + t.Fatalf("dial: %v", dialErr) + } + defer func() { _ = clientConn.Close(websocket.StatusNormalClosure, "") }() + + var conn *websocket.Conn + select { + case conn = <-connCh: + case <-time.After(5 * time.Second): + t.Fatal("server never accepted the connection") + } + + done := make(chan struct{}) + go func() { + readPump(context.Background(), conn, h, c) + close(done) + }() + + // Wait until the defer is blocked inside handleVoiceLeave's LiveKit call — + // unregisterNow has already run and sampled replaced=false. + select { + case <-reachedLiveKit: + case <-time.After(5 * time.Second): + t.Fatal("readPump's defer never reached the LiveKit RemoveParticipant call") + } + + // The user reconnects while the old connection's teardown is still in + // flight: a fresh client takes the (now-empty) hub slot for the same + // user, exactly as registerNow does for a real reconnect. + newClient := NewTestClient(h, uid, make(chan []byte, 8)) + newClient.user = &db.User{ID: uid, Status: "online"} + h.registerNow(newClient, map[int64]bool{}) + + // Let handleVoiceLeave's LiveKit call complete so the old defer resumes. + close(proceed) + + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("readPump did not return after the LiveKit call completed") + } + + if got := h.GetClient(uid); got != newClient { + t.Fatalf("hub client for user %d = %p after old connection's teardown, want the reconnected client %p", uid, got, newClient) + } + + var offlineBroadcasts int + for len(h.broadcast) > 0 { + bm := <-h.broadcast + if bytes.Contains(bm.msg, []byte(`"status":"offline"`)) { + offlineBroadcasts++ + } + } + if offlineBroadcasts != 0 { + t.Errorf("got %d offline presence broadcasts after a reconnect raced the old connection's voice cleanup, want 0 — the live session was stamped offline", offlineBroadcasts) + } + + user, err := database.GetUserByID(ctx, uid) + if err != nil { + t.Fatalf("GetUserByID: %v", err) + } + if user.Status != "online" { + t.Errorf("user status = %q after the reconnect race, want %q — the dead socket's teardown overwrote the live session's status", user.Status, "online") + } +} diff --git a/Server/ws/serve_ready.go b/Server/ws/serve_ready.go index e6209f47..7cab1316 100644 --- a/Server/ws/serve_ready.go +++ b/Server/ws/serve_ready.go @@ -152,8 +152,7 @@ func (h *Hub) buildReady(ctx context.Context, database *db.DB, userID int64, rol members, err := database.ListMembers(ctx) if err != nil { - slog.Warn("buildReady ListMembers", "err", err) - members = []db.MemberSummary{} + return nil, fmt.Errorf("buildReady ListMembers: %w", err) } members = h.presentableMembers(members, userID) @@ -187,8 +186,7 @@ func (h *Hub) buildReady(ctx context.Context, database *db.DB, userID int64, rol // Per-user unread counts. unreadMap, err := database.GetChannelUnreadCounts(ctx, userID) if err != nil { - slog.Warn("buildReady GetChannelUnreadCounts", "err", err) - unreadMap = map[int64]db.ChannelUnread{} + return nil, fmt.Errorf("buildReady GetChannelUnreadCounts: %w", err) } // Build protocol-compliant channel objects (strip extra fields). @@ -241,8 +239,7 @@ func (h *Hub) buildReady(ctx context.Context, database *db.DB, userID int64, rol // this a DM voice call's voice_state rows would never make it into ready. dmChannels, err := database.GetUserDMChannels(ctx, userID) if err != nil { - slog.Warn("buildReady GetUserDMChannels", "err", err) - dmChannels = []db.DMChannelInfo{} + return nil, fmt.Errorf("buildReady GetUserDMChannels: %w", err) } // GetUserDMChannels computes unread from read_states but carries no mention // count, so a DM mention badge used to vanish on every reconnect. The diff --git a/Server/ws/serve_ready_error_propagation_test.go b/Server/ws/serve_ready_error_propagation_test.go new file mode 100644 index 00000000..249c2290 --- /dev/null +++ b/Server/ws/serve_ready_error_propagation_test.go @@ -0,0 +1,83 @@ +package ws_test + +// serve_ready_error_propagation_test.go — regression test for finding +// OC-0029: buildReady downgraded ListMembers, GetChannelUnreadCounts, and +// GetUserDMChannels failures to a slog.Warn plus an empty value, then still +// built and returned a normal `ready` frame. `ready` is the protocol's +// authoritative full-state snapshot -- dispatcher.ts treats an empty +// dm_channels as "the server always sends this field, so empty means no open +// DMs" and wipes dmStore (and the active channel, if a DM was open) on that +// basis -- so a transient DB error on any of these three queries was +// indistinguishable on the wire from "you genuinely have none". +// ListChannels/ListRoles/GetChannelOverridesFor already do the right thing +// (return the error and abort the handshake so the client retries); these +// three should too. +// +// Each subtest fault-injects exactly one of the three queries by dropping +// the SQLite table only that query (and nothing earlier in buildReady's call +// order) depends on, then asserts buildReady fails instead of shipping a +// falsely-empty snapshot. + +import ( + "context" + "testing" +) + +// TestBuildReady_PropagatesListMembersError drops `users`, which ListMembers +// joins against but which nothing earlier in buildReady (ListChannels, +// ListRoles) touches. +func TestBuildReady_PropagatesListMembersError(t *testing.T) { + hub, database := newServeHub(t) + user := seedServeUser(t, database, "ready-err-members") + role, err := database.GetRoleByID(context.Background(), user.RoleID) + if err != nil || role == nil { + t.Fatalf("GetRoleByID: %v", err) + } + + if _, err := database.ExecContext(context.Background(), `DROP TABLE users`); err != nil { + t.Fatalf("drop users: %v", err) + } + + if _, err := hub.BuildReadyWithRoleForTest(database, user.ID, role); err == nil { + t.Fatal("buildReady must fail the handshake when ListMembers errors, not silently ship an empty member list as if the server genuinely has none") + } +} + +// TestBuildReady_PropagatesUnreadCountsError drops `read_states`, which only +// GetChannelUnreadCounts (and, further down the function, GetUserDMChannels) +// reads -- ListChannels, ListRoles and ListMembers do not. +func TestBuildReady_PropagatesUnreadCountsError(t *testing.T) { + hub, database := newServeHub(t) + user := seedServeUser(t, database, "ready-err-unread") + role, err := database.GetRoleByID(context.Background(), user.RoleID) + if err != nil || role == nil { + t.Fatalf("GetRoleByID: %v", err) + } + + if _, err := database.ExecContext(context.Background(), `DROP TABLE read_states`); err != nil { + t.Fatalf("drop read_states: %v", err) + } + + if _, err := hub.BuildReadyWithRoleForTest(database, user.ID, role); err == nil { + t.Fatal("buildReady must fail the handshake when GetChannelUnreadCounts errors, not silently ship every channel with unread_count/mention_count zeroed") + } +} + +// TestBuildReady_PropagatesDMChannelsError drops `dm_open_state`, which only +// GetUserDMChannels reads -- nothing else in buildReady's call chain does. +func TestBuildReady_PropagatesDMChannelsError(t *testing.T) { + hub, database := newServeHub(t) + user := seedServeUser(t, database, "ready-err-dms") + role, err := database.GetRoleByID(context.Background(), user.RoleID) + if err != nil || role == nil { + t.Fatalf("GetRoleByID: %v", err) + } + + if _, err := database.ExecContext(context.Background(), `DROP TABLE dm_open_state`); err != nil { + t.Fatalf("drop dm_open_state: %v", err) + } + + if _, err := hub.BuildReadyWithRoleForTest(database, user.ID, role); err == nil { + t.Fatal("buildReady must fail the handshake when GetUserDMChannels errors, not silently ship dm_channels: [] as if the user genuinely has none") + } +} diff --git a/Server/ws/voice_handlers_test.go b/Server/ws/voice_handlers_test.go index 365dc604..c5075bbb 100644 --- a/Server/ws/voice_handlers_test.go +++ b/Server/ws/voice_handlers_test.go @@ -1207,6 +1207,67 @@ func TestVoice_Join_SameChannel_IsIdempotent(t *testing.T) { } } +// TestVoice_Join_AbortedSwitch_DoesNotResurrectPhantomSession pins OC-0034: +// when a voice-channel switch's pre-switch leave leaves the old voice_states +// row in place (e.g. a missing join token short-circuits the delete via +// leaveVoiceChannelWithRetry's empty-token guard in voice_leave.go), +// handleVoiceJoin aborts the switch. finishVoiceLeave has already broadcast +// voice_leave for the old channel to every client that can see it — including +// the leaver itself, which finishVoiceLeave always adds to the audience — so +// every client, this one's own session included, has already torn the old +// membership down. Restoring the client's local voice state on abort +// resurrects a session nobody else believes exists anymore. The fix is to +// leave the client's local state cleared so it agrees with the voice_leave it +// already received; the stale DB row then disagrees with every connected +// client's voiceChID and the periodic sweep reaps it. +func TestVoice_Join_AbortedSwitch_DoesNotResurrectPhantomSession(t *testing.T) { + hub, database := newVoiceHub(t) + user := seedVoiceOwner(t, database, "abort-switch-user") + chanA := seedVoiceChan(t, database, "vc-abort-a") + chanB := seedVoiceChan(t, database, "vc-abort-b") + + send := make(chan []byte, 32) + c := ws.NewTestClientWithUser(hub, user, chanA, send) + hub.Register(c) + waitRegistered(t, hub, c) + + // Join channel A normally. + hub.HandleMessageForTest(c, voiceJoinMsg(chanA)) + drainChanTimeout(send, 30*time.Millisecond) + + stateA, _ := database.GetVoiceState(context.Background(), user.ID) + if stateA == nil || stateA.ChannelID != chanA { + t.Fatalf("user should be in channel A, got %+v", stateA) + } + + // Simulate the repro from the finding: the client's local join token has + // gone missing (e.g. a prior partial failure) while its voice channel ID + // still agrees with the DB row. leaveVoiceChannelWithRetry's empty-token + // guard then skips the DELETE entirely, so the pre-switch leave silently + // no-ops and the old row survives. + ws.SetClientVoiceStateForTest(c, chanA, "") + + // Attempt to switch to channel B. handleVoiceLeave runs first (broadcasts + // voice_leave for chanA to the leaver, per finishVoiceLeave), the DB + // delete is skipped, and handleVoiceJoin's stale-state check aborts the + // switch. + hub.HandleMessageForTest(c, voiceJoinMsg(chanB)) + + // Confirm the abort branch actually triggered: the old row must still be + // present in the DB. + stillA, _ := database.GetVoiceState(context.Background(), user.ID) + if stillA == nil || stillA.ChannelID != chanA { + t.Fatalf("test setup broken: expected stale row in chanA, got %+v", stillA) + } + + if got := ws.GetClientVoiceChIDForTest(c); got != 0 { + t.Errorf("aborted switch resurrected a phantom session: client voice channel = %d, want 0 (voice_leave for chanA was already broadcast to this client, including itself)", got) + } + if hub.SubscribedToVoiceTopicForTest(c, chanA) { + t.Error("aborted switch re-subscribed the client to chanA's voice topic after voice_leave was already broadcast for it") + } +} + // ─── voice leave on disconnect ──────────────────────────────────────────────── // TestVoice_Leave_OnDisconnect verifies that handleVoiceLeave cleans up diff --git a/Server/ws/voice_join.go b/Server/ws/voice_join.go index c83be25e..f741f46f 100644 --- a/Server/ws/voice_join.go +++ b/Server/ws/voice_join.go @@ -181,15 +181,20 @@ func (h *Hub) handleVoiceJoin(ctx context.Context, c *Client, payload json.RawMe if vs != nil { slog.Warn("handleVoiceJoin: stale voice state persists after leave, aborting switch", "user_id", c.userID, "stale_channel", vs.ChannelID, "target_channel", channelID) - // Restore client voice state so the user knows they're still in the - // old channel. The failed leave already dropped the voice-topic - // subscription and key-holder entry, and voice state and topic - // subscription must move as a pair (see clearVoiceAndUnsubscribe) - // — without them the restored session silently misses every - // voice_e2ee relay for its channel. - c.setVoiceState(vs.ChannelID, vs.JoinedAt) - h.pubsub.Subscribe(c, VoiceTopic(vs.ChannelID)) - h.updateKeyHolder(vs.ChannelID) + // OC-0034: do NOT restore the client's local voice state here. + // handleVoiceLeave above already broadcast voice_leave for the old + // channel to every client that can see it — including this one, + // since finishVoiceLeave always adds the leaver to the audience — + // so every client, this user's own session included, has already + // torn the old membership down (dispatcher.ts runs leaveVoice on a + // self voice_leave). Restoring c.voiceChID/the topic subscription + // would resurrect a session nobody else believes exists anymore, + // while the stale DB row (this branch's trigger) stays orphaned. + // Leaving the client cleared keeps it consistent with the + // voice_leave it just received: the row now disagrees with every + // connected client's voiceChID, so sweepStaleVoiceStates reaps it + // (re-broadcasting voice_leave, harmlessly) within one tick, and + // the user_id-PK upsert lets the user rejoin immediately. c.sendMsg(buildErrorMsg(ErrCodeInternal, "voice channel switch failed — please try again")) return } diff --git a/Server/ws/voice_leave.go b/Server/ws/voice_leave.go index 9e9507ba..6f52ef5c 100644 --- a/Server/ws/voice_leave.go +++ b/Server/ws/voice_leave.go @@ -70,33 +70,11 @@ func (h *Hub) finishVoiceLeave(ctx context.Context, c *Client, oldChID int64, ol } // Audience = broadcastVoiceEvent's (READ ∪ still-in-the-room) plus the - // leaver themselves. The union of the room's remaining participants is - // what broadcastVoiceEvent provides and must be kept: voice membership is - // gated on CONNECT_VOICE alone, so a participant without READ would - // otherwise miss the departure and keep a stale E2EE key holder. The extra - // term is the leaver: the caller has already cleared their client voice + // leaver themselves: the caller has already cleared their client voice // state, so that union can no longer see them, yet for a server-initiated // eviction (revocation sweep, moderator kick/move, token-refresh refusal) - // this voice_leave IS their only teardown signal. Mirrors - // CleanupVoiceForChannel, which appends the evicted participants for - // exactly the same reason. - audience := h.channelReadAudience(ctx, oldChID) - seen := make(map[int64]struct{}, len(audience)+1) - for _, uid := range audience { - seen[uid] = struct{}{} - } - h.mu.RLock() - for uid, other := range h.clients { - if _, ok := seen[uid]; !ok && other.getVoiceChID() == oldChID { - seen[uid] = struct{}{} - audience = append(audience, uid) - } - } - h.mu.RUnlock() - if _, ok := seen[c.userID]; !ok { - audience = append(audience, c.userID) - } - h.broadcastChannelScopedTo(oldChID, buildVoiceLeave(oldChID, c.userID), audience, "voice event") + // this voice_leave IS their only teardown signal. + h.broadcastVoiceEventWithLeaver(ctx, oldChID, buildVoiceLeave(oldChID, c.userID), c.userID) // Re-elect key holder now that this user has left the channel. h.updateKeyHolder(oldChID)