diff --git a/Client/tauri-client/src-tauri/src/http_proxy.rs b/Client/tauri-client/src-tauri/src/http_proxy.rs index a0b2a2a7..f5d443be 100644 --- a/Client/tauri-client/src-tauri/src/http_proxy.rs +++ b/Client/tauri-client/src-tauri/src/http_proxy.rs @@ -272,6 +272,51 @@ fn rewrite_request_headers(raw: &[u8], remote_host: &str) -> String { modified } +/// Bracket-aware split of a `remote_host` string into (hostname, port). +/// Defaults to port 443 (standard HTTPS) when none is specified. +/// +/// A leading `[` consumes up to the matching `]` as the hostname, so a +/// bracketed IPv6 literal parses correctly whether or not it carries an +/// explicit port (`[::1]`, `[::1]:8443`). Without brackets, a single +/// trailing colon is a `host:port` split — but a *bare* (unbracketed) IPv6 +/// literal contains more than one colon, and RFC 3986 gives it no way to +/// carry a port without brackets, so that case is returned whole with the +/// default port instead of being mis-split on its last colon. +fn split_host_port(remote_host: &str) -> Result<(&str, &str), String> { + if let Some(rest) = remote_host.strip_prefix('[') { + let (host, tail) = rest + .split_once(']') + .ok_or_else(|| format!("unterminated '[' in remote_host '{remote_host}'"))?; + let port = tail.strip_prefix(':').unwrap_or("443"); + Ok((host, port)) + } else { + match remote_host.rsplit_once(':') { + Some((host, port)) if !host.contains(':') => Ok((host, port)), + _ => Ok((remote_host, "443")), + } + } +} + +/// Derive the TLS `ServerName` (SNI) and the TCP dial target from a +/// `remote_host` string. Mirrors `livekit_proxy::parse_server_name`'s +/// bracket handling. +fn resolve_remote_target(remote_host: &str) -> Result<(ServerName<'static>, String), String> { + let (hostname, port) = split_host_port(remote_host)?; + let server_name = if let Ok(ip) = hostname.parse::() { + ServerName::IpAddress(ip.into()) + } else { + ServerName::try_from(hostname.to_string()) + .map_err(|e| format!("invalid server name '{hostname}': {e}"))? + }; + + let dial_target = if hostname.contains(':') { + format!("[{hostname}]:{port}") + } else { + format!("{hostname}:{port}") + }; + Ok((server_name, dial_target)) +} + /// Handle one proxied connection: /// 1. Read the request headers from the loopback side /// 2. TLS-connect to the remote and run the TOFU check (store/emit/reject) @@ -321,20 +366,7 @@ async fn handle_connection( .with_no_client_auth(); let connector = tokio_rustls::TlsConnector::from(Arc::new(tls_config)); - let (raw_hostname, _port) = remote_host.rsplit_once(':').unwrap_or((remote_host, "443")); - let hostname = raw_hostname.trim_start_matches('[').trim_end_matches(']'); - let server_name = if let Ok(ip) = hostname.parse::() { - ServerName::IpAddress(ip.into()) - } else { - ServerName::try_from(hostname.to_string()) - .map_err(|e| format!("invalid server name '{hostname}': {e}"))? - }; - - let dial_target = if remote_host.contains(':') { - remote_host.to_string() - } else { - format!("{remote_host}:443") - }; + let (server_name, dial_target) = resolve_remote_target(remote_host)?; let tcp = timeout(Duration::from_secs(10), TcpStream::connect(&dial_target)) .await .map_err(|_| Box::::from("TCP connect timed out"))??; @@ -496,6 +528,41 @@ mod tests { assert!(validate_remote_host("[::1]:8443").is_ok()); } + // OC-0021: IPv6 hosts that are not in the exact `[addr]:port` shape must + // still resolve to a valid ServerName and a dialable host:port target. + + #[test] + fn resolve_remote_target_handles_bracketed_ipv6_without_port() { + let (server_name, dial_target) = resolve_remote_target("[2001:db8::1]") + .expect("bracketed IPv6 without a port must parse"); + assert!(matches!(server_name, ServerName::IpAddress(_))); + assert_eq!(dial_target, "[2001:db8::1]:443"); + } + + #[test] + fn resolve_remote_target_handles_bare_ipv6_without_port() { + let (server_name, dial_target) = + resolve_remote_target("2001:db8::1").expect("bare IPv6 without a port must parse"); + assert!(matches!(server_name, ServerName::IpAddress(_))); + assert_eq!(dial_target, "[2001:db8::1]:443"); + } + + #[test] + fn resolve_remote_target_still_handles_bracketed_ipv6_with_port() { + let (server_name, dial_target) = resolve_remote_target("[2001:db8::1]:8443") + .expect("bracketed IPv6 with a port must parse"); + assert!(matches!(server_name, ServerName::IpAddress(_))); + assert_eq!(dial_target, "[2001:db8::1]:8443"); + } + + #[test] + fn resolve_remote_target_still_handles_plain_hostname_and_port() { + let (server_name, dial_target) = + resolve_remote_target("example.com:8443").expect("hostname:port must parse"); + assert!(matches!(server_name, ServerName::DnsName(_))); + assert_eq!(dial_target, "example.com:8443"); + } + #[test] fn rewrite_replaces_host_and_forces_close() { let raw = b"GET /api/v1/health HTTP/1.1\r\nHost: 127.0.0.1:5000\r\nAccept: */*\r\n\r\n"; diff --git a/Client/tauri-client/src/components/MessageList.ts b/Client/tauri-client/src/components/MessageList.ts index bc9612bf..042f3a7b 100644 --- a/Client/tauri-client/src/components/MessageList.ts +++ b/Client/tauri-client/src/components/MessageList.ts @@ -984,6 +984,15 @@ export function createMessageList(options: MessageListOptions): MessageListCompo renderedStart = -1; renderWindow(); + // renderWindow's own rapid-rebuild breaker can return before reassigning + // renderedStart (it stays -1, the value forced above) when too many + // rebuilds have fired in the last 2s. When that happens the DOM was never + // rebuilt for this target — report a failed jump rather than computing a + // localIdx against a sentinel and flashing/reporting success for a row + // that never rendered. This lets callers (e.g. MessageJump) fall back to + // fetching the around-window instead of treating this as a landed jump. + if (renderedStart < 0) return false; + // Briefly highlight the target message element if (contentContainer !== null) { const localIdx = idx - renderedStart; diff --git a/Client/tauri-client/src/lib/dispatcher.ts b/Client/tauri-client/src/lib/dispatcher.ts index a0eccaa7..c1e8115b 100644 --- a/Client/tauri-client/src/lib/dispatcher.ts +++ b/Client/tauri-client/src/lib/dispatcher.ts @@ -83,6 +83,21 @@ import { addDmToChannelsStore } from "@pages/main-page/SidebarDmHelpers"; const log = createLogger("dispatcher"); +/** + * OC-0024: serverClockSkewMs (see wireDispatcher) starts at 0 and is only + * ever sampled from a frame the replay check itself already accepted as + * live — so if the channel is quiet between login and the first reconnect, + * the skew is never sampled, and a lagging/skewed server clock then makes + * every genuinely live message look like a replay for as long as real + * elapsed time takes to exceed the drift (which can be unbounded). A + * replayed burst is delivered as a burst immediately after auth_ok, so cap + * how long a frame can be classified as a replay by wall-clock distance from + * the handshake as well as by timestamp — that bounds a cold (unsampled) + * skew's worst case to this window instead of the whole drift, while still + * covering the burst's actual delivery window with room to spare. + */ +const REPLAY_GATE_WINDOW_MS = 5_000; + /** Lazily import the LiveKit session module. livekit-client (~1.3 MB) is kept * out of the entry chunk; voice handlers load it on first use. Once a voice * flow has started the module is cached, so this resolves in a microtask. */ @@ -90,6 +105,33 @@ function livekitSession(): Promise { return import("@lib/livekitSession"); } +/** + * Honor a moderator's mute/deafen locally. Mute is also enforced at the SFU, + * but deafen governs what WE play back, so the client is the only place it + * can take effect (Server/ws/voice_moderation.go: "enforced by the target's + * client honoring server_deafened"). Both apply through one lazy import so + * the two effects cannot land in different ticks. + * + * Called from both the incremental VOICE_STATE path and the full-resync + * READY path (a WS drop that outlives the LiveKit session can mean a + * moderator mute/deafen issued while disconnected is only ever delivered via + * `ready`'s voice_states, never a voice_state the client could have missed). + * The `!voice.localMuted`/`!voice.localDeafened` guards make it safe to call + * from either path — or both, on the same session — without redundantly + * re-invoking the livekit calls once already applied. + */ +function enforceModeratorAudioState(serverMuted: boolean, serverDeafened: boolean): void { + const voice = voiceStore.getState(); + const applyDeafen = serverDeafened && !voice.localDeafened; + const applyMute = serverMuted && !voice.localMuted; + if (applyDeafen || applyMute) { + void livekitSession().then(({ setDeafened, setMuted }) => { + if (applyDeafen) setDeafened(true); + if (applyMute) setMuted(true); + }); + } +} + /** Map one DM participant from the wire shape to the store's. */ function mapDmUser(u: DmChannelPayload["recipient"]): DmChannel["recipient"] { return { @@ -238,13 +280,26 @@ export function wireDispatcher( // reload always starts idle — exactly the stale case), while any other // status means livekitSession is driving a session right now. const currentUserId = authStore.getState().user?.id ?? 0; - const inVoicePerReady = - currentUserId !== 0 && payload.voice_states.some((vs) => vs.user_id === currentUserId); + const selfVoiceState = + currentUserId !== 0 + ? payload.voice_states.find((vs) => vs.user_id === currentUserId) + : undefined; const voiceSessionActive = voiceStore.getState().voiceStatus !== "idle"; - if (inVoicePerReady && !voiceSessionActive) { + if (selfVoiceState !== undefined && !voiceSessionActive) { log.warn("Stale voice state detected in ready payload — sending voice_leave"); ws.send({ type: "voice_leave", payload: {} }); leaveVoiceChannel(); + } else if (selfVoiceState !== undefined) { + // A LiveKit session survived a WS drop that outlived it (nothing + // tears voice down on a socket drop alone) — OC-0014: this full + // resync is the only place a moderator mute/deafen issued while we + // were disconnected ever reaches us, since the mustFullResync tier + // that produced this `ready` never replays the voice_state that + // would otherwise have carried it. + enforceModeratorAudioState( + selfVoiceState.server_muted === true, + selfVoiceState.server_deafened === true, + ); } // F3: publish our long-term identity public key so peers can pin+verify @@ -535,8 +590,11 @@ export function wireDispatcher( // preceded it, unlike a genuinely new live message — compared in // server-clock terms (see serverClockSkewMs above) so a lagging or // skewed server clock cannot make a live message look like a replay. + // The wall-clock window additionally bounds a cold (never-sampled) + // skew's damage — see REPLAY_GATE_WINDOW_MS. const isReplayFrame = lastReconnectHandshakeAt !== null && + Date.now() - lastReconnectHandshakeAt < REPLAY_GATE_WINDOW_MS && Date.parse(payload.timestamp) < lastReconnectHandshakeAt - serverClockSkewMs; if (!isReplayFrame) { notifyIncomingMessage(payload); @@ -752,19 +810,7 @@ export function wireDispatcher( const currentUserId = authStore.getState().user?.id ?? 0; if (payload.user_id !== currentUserId) return; joinVoiceChannel(payload.channel_id); - // Honor a moderator's mute/deafen locally. Mute is also enforced at the - // SFU, but deafen governs what WE play back, so the client is the only - // place it can take effect. Both apply through one lazy import so the - // two effects cannot land in different ticks. - const voice = voiceStore.getState(); - const applyDeafen = payload.server_deafened === true && !voice.localDeafened; - const applyMute = payload.server_muted === true && !voice.localMuted; - if (applyDeafen || applyMute) { - void livekitSession().then(({ setDeafened, setMuted }) => { - if (applyDeafen) setDeafened(true); - if (applyMute) setMuted(true); - }); - } + enforceModeratorAudioState(payload.server_muted === true, payload.server_deafened === true); }), ); @@ -789,6 +835,29 @@ export function wireDispatcher( // cleared the store; this only surfaces the reason. unsubs.push( ws.on(S.VOICE_DISCONNECTED, (payload) => { + // OC-0031: this can arrive well after the kick already tore the + // session down at the SFU (queued behind a backed-up outbound send + // buffer) — by the time it's delivered the user may have already + // rejoined this channel or another. Guard on channel match, same as + // the sibling VOICE_LEAVE handler below (shouldTeardownSession) — a + // stale frame for a channel already left must not kill a newer join. + // Read the store before leaveVoiceChannel() below clears + // currentChannelId. + // + // OC-0033: on the ordinary kick path, the server sends voice_leave to + // the leaver (finishVoiceLeave) BEFORE handleVoiceModKickV2 sends this + // voice_disconnected, so the sibling VOICE_LEAVE handler has typically + // already nulled currentChannelId by the time this arrives. That's not + // the OC-0031 staleness (a rejoin into a *different* channel) — treat + // a cleared store as not-stale so the kick reason still gets shown. + const cur = voiceStore.getState().currentChannelId; + const stale = cur !== null && cur !== payload.channel_id; + if (stale) { + log.info("Ignoring stale voice_disconnected for a channel already left", { + channelId: payload.channel_id, + }); + return; + } log.info("Disconnected from voice by a moderator", { channelId: payload.channel_id }); void livekitSession().then(({ leaveVoice }) => leaveVoice(false)); leaveVoiceChannel(); @@ -1016,9 +1085,30 @@ export function wireDispatcher( if (payload.code === "VIDEO_LIMIT") { showToast(payload.message || "That voice channel has reached its video limit", "error"); // max_video has no SFU-level enforcement — the server only refuses the - // DB write. Without this rollback the already-published camera track - // keeps streaming to everyone while voice_state says camera=false. - void livekitSession().then(({ disableCamera }) => disableCamera()); + // DB write. Without this rollback the already-published track keeps + // streaming to everyone while voice_state says camera/screenshare is + // off. voice_controls.go routes both a refused voice_camera AND a + // refused voice_screenshare enable through the same shared + // enableVideoSlot cap check, so this code is not camera-specific — + // correlate by envelope id, exactly like the generic rollback below, + // instead of assuming it's always the camera. A bare VIDEO_LIMIT with + // no id (older server / no correlation available) still falls back + // to the camera, the only kind this branch used to handle. + if (id !== undefined) { + void import("@lib/screenShare").then(({ rollbackPendingVideo }) => { + const kind = rollbackPendingVideo(id); + // undefined means this id no longer correlates to anything + // pending (superseded by a later enable of the same kind) — the + // refusal is stale and there is nothing to roll back. It must + // never be treated as "it was the camera". + if (kind === undefined) return; + void livekitSession().then(({ disableCamera, disableScreenshare }) => + kind === "screen" ? disableScreenshare() : disableCamera(), + ); + }); + } else { + void livekitSession().then(({ disableCamera }) => disableCamera()); + } return; } // Every remaining code has no dedicated handler above (not a pending diff --git a/Client/tauri-client/src/lib/livekitE2EE.ts b/Client/tauri-client/src/lib/livekitE2EE.ts index 7dc8b147..dd0c2b4e 100644 --- a/Client/tauri-client/src/lib/livekitE2EE.ts +++ b/Client/tauri-client/src/lib/livekitE2EE.ts @@ -826,13 +826,26 @@ export class E2EEManager { * Handle a voice_e2ee_offer from the server — the key holder has sent us * the encrypted room key. Unwrap it and apply to the E2EE key provider. * Offers are applied one at a time, in delivery order. + * + * Chained through _announceChain first (OC-0002): handleAnnounceInner only + * stores the sender's ECDH key after several awaits (identity-pin lookup, + * signature verification, key import), while handleOfferInner's first + * statement is a synchronous _peerPublicKeys lookup. An offer dispatched + * immediately behind that same sender's announce — the OC-0098 send order + * guarantees exactly this WS delivery order — would otherwise reach the + * lookup before the announce applied, and be dropped as "unknown peer" + * with no retry until the next 5-minute rotation. Waiting on the announce + * chain reproduces WS delivery order exactly: the announce is enqueued on + * it before the offer's frame is even dispatched. No deadlock risk: + * handleAnnounceInner never awaits the offer chain and never rejects (it + * catches internally), and clearState() resets both chains together. */ handleOffer(fromUserId: number, encryptedKeyBase64: string, ivBase64: string): Promise { // handleOfferInner never rejects (it catches internally), so the chain // cannot wedge on a failed offer. - const run = this._offerChain.then(() => - this.handleOfferInner(fromUserId, encryptedKeyBase64, ivBase64), - ); + const run = this._offerChain + .then(() => this._announceChain) + .then(() => this.handleOfferInner(fromUserId, encryptedKeyBase64, ivBase64)); this._offerChain = run; return run; } @@ -1042,9 +1055,23 @@ export class E2EEManager { * insertion order (which is not guaranteed to match server join order). */ async handleParticipantLeft(userId: number): Promise { - const hadPeerKey = this._peerPublicKeys.has(userId); + const departingKey = this._peerPublicKeys.get(userId); + const hadPeerKey = departingKey !== undefined; this._peerPublicKeys.delete(userId); clearPeerVerification(userId); + // Retire the departing peer's key (OC-0020): _retiredPeerKeys is the only + // defense against replay of a validly-signed announce (the signed + // message carries no channel/epoch/nonce, F3) and handleAnnounceInner + // only records a retirement on an in-session key CHANGE. Without this, a + // peer that leaves and rejoins with a fresh key is neither live nor + // retired on their pre-leave key — a replay of the recorded old announce + // then passes both guards and overwrites the peer's live key with one + // whose private half no longer exists (blackholing them). A genuine + // rejoin always mints a fresh ECDH pair (setupKeyExchange, + // reannounceForReconnect), so this never rejects a legitimate re-announce. + if (departingKey) { + this.retirePeerKey(userId, await exportPublicKey(departingKey)); + } const channelId = this._channelId ?? this.deps.getCurrentChannelId(); if (!channelId) return; diff --git a/Client/tauri-client/src/lib/livekitSession.ts b/Client/tauri-client/src/lib/livekitSession.ts index aa1cb9f9..69de270f 100644 --- a/Client/tauri-client/src/lib/livekitSession.ts +++ b/Client/tauri-client/src/lib/livekitSession.ts @@ -139,6 +139,13 @@ export class LiveKitSession { private tokenRefreshTimer: ReturnType | null = null; /** BUG-146: Guard timer — fires if the server never responds to voice_token_refresh. */ private tokenRefreshTimeoutTimer: ReturnType | null = null; + /** OC-0029: epoch ms of the last voice_token_refresh actually sent. The + * server budgets this request to 1 per 60s per user (Server/ws/voice_join.go); + * requestTokenRefresh() has multiple independent callers (the 4-minute + * timer AND attemptAutoReconnect's post-recovery refresh) that can land + * within seconds of each other, so this shared entry point — not each + * caller — is what enforces the budget. 0 (never sent) never blocks. */ + private _lastTokenRefreshSentAt = 0; /** Max auto-reconnect attempts before giving up and showing error. */ private static readonly MAX_RECONNECT_ATTEMPTS = 2; private static readonly RECONNECT_DELAY_MS = 3000; @@ -800,7 +807,19 @@ export class LiveKitSession { log.debug("Skipping token refresh — no active session"); return; } + // OC-0029: the server refuses more than 1 voice_token_refresh per 60s + // per user (ErrCodeRateLimited). requestTokenRefresh() is called both by + // the routine 4-minute timer and by attemptAutoReconnect's unconditional + // post-recovery refresh, which can land only seconds after the timer's + // own refresh — without this guard the second request is rejected and + // surfaces as a bare "token refresh rate limited" error toast right as + // the user's call recovers. + if (Date.now() - this._lastTokenRefreshSentAt < 60_000) { + log.debug("Skipping token refresh — one was already sent within the last 60s"); + return; + } log.info("Requesting voice token refresh"); + this._lastTokenRefreshSentAt = Date.now(); this.ws.send({ type: "voice_token_refresh", payload: {} }); // NOTE: startTokenRefreshTimer is called from handleVoiceTokenRefresh // (the server response handler), not here, to avoid scheduling two @@ -978,18 +997,31 @@ export class LiveKitSession { this.onRemoteVideoRemovedCallback = null; } - /** Post-connect-checkpoint cleanup for a superseded connectAndSetup attempt - * (checkpoints 3-5, after this attempt already installed its room into the - * shared "connected" state). By the time one of these fires, a NEWER - * attempt may have already claimed `_state` (and torn down THIS attempt's - * room via its own entry-point leaveVoice(false)) — so this must disconnect - * only the passed-in localRoom, mirroring checkpoint 2, and must never call + /** Checkpoint cleanup for a superseded connectAndSetup attempt, used at + * every "return \"superseded\"" site in that function. By the time one of + * these fires, a NEWER attempt may have already claimed `_state` (and torn + * down THIS attempt's room via its own entry-point leaveVoice(false)) — so + * this must disconnect only the passed-in localRoom and must never call * the global leaveVoice()/touch `_state`, or it tears down whichever * session currently occupies `_state`, which now belongs to the newer - * attempt. */ + * attempt. + * + * OC-0006: also re-syncs the extracted modules (DeviceManager/AudioPipeline + * /AudioElements) when nobody newer owns `_state`. Earlier checkpoints + * (1/2, the key-exchange failure, and the retry-backoff check) fire before + * this attempt ever reaches "connected" — if the supersession was a plain + * leaveVoice() (state now "idle") that landed while this attempt's own + * lines above had already wired the modules to `localRoom`, that leave's + * own syncModuleRooms() ran too early and got undone by the later wiring, + * leaving DeviceManager's devicechange listener armed on a Room that will + * never connect. The condition is required — an unconditional sync would + * null the modules out from under a newer attempt that already ran its own + * wiring but has not yet reached "connected" (it never re-wires after that + * point). */ private disconnectSupersededLocalRoom(localRoom: Room): void { localRoom.removeAllListeners(); localRoom.disconnect().catch((err) => log.debug("Failed to disconnect superseded room", err)); + if (this._state.type === "idle") this.syncModuleRooms(); } /** Shared connect-with-retry + post-connect setup used by both the primary @@ -1011,7 +1043,19 @@ export class LiveKitSession { // below inherits the PREVIOUS channel's residual _isKeyHolder via its // OR-with-server-value guard, joining the new channel as a phantom key // holder the server never elected (OC-0020). - if (this._room !== null || this._state.type === "reconnecting") this.leaveVoice(false); + if (this._room !== null || this._state.type === "reconnecting") { + this.leaveVoice(false); + } else if (this._state.type === "connecting") { + // OC-0001: the pending-join drain loop (handleVoiceToken) re-enters + // this function while `_state` is still "connecting" — there is no + // room to disconnect and no reconnect AC to abort, so the branch above + // never fires, but a discarded prior attempt (e.g. the e2ee_timeout / + // checkpoint-2 queued-join paths below) can still leave residual E2EE + // state (_isKeyHolder, keypair, peer keys) behind for THIS attempt to + // inherit via setupKeyExchange's OR-with-server-value guard. Clear it + // explicitly since leaveVoice() itself never runs on this path. + this._e2ee.clearState(); + } // Draw the next generation from the monotonic instance counter (never // re-derived from `_state`) and embed it into the "connecting" state. // Any newer call to connectAndSetup() will produce a strictly larger @@ -1044,6 +1088,7 @@ export class LiveKitSession { myGeneration, currentGeneration: this._state.type === "connecting" ? this._state.joinGeneration : "n/a", }); + this.disconnectSupersededLocalRoom(localRoom); return "superseded"; } @@ -1070,17 +1115,35 @@ export class LiveKitSession { channelId, myGeneration, }); + this.disconnectSupersededLocalRoom(localRoom); return "superseded"; } - this.onErrorCallback?.("e2ee_timeout"); - // The exchange timed out BEFORE room.connect(): no SFU participant - // exists, so no LiveKit webhook will ever clean up, and the server - // registered the join when it sent voice_token. Send voice_leave and - // leave the store's voice channel (like the reconnect-exhausted give-up - // path) or the stale row ghosts forever and can wedge the channel's - // key-holder election. - this.leaveVoice(true); - leaveVoiceChannel(); + // OC-0010: a channel switch queued during the wait (handleVoiceToken's + // pendingJoin branch) preserves this attempt's type/joinGeneration, so + // the ownership check above cannot distinguish it from "no newer join + // is coming". Treating it as a genuine failure here would send + // voice_leave with no channel id — deleting the QUEUED join's + // voice_states row, not this timed-out attempt's — and would drop the + // pendingJoin itself by transitioning to idle before the drain loop + // ever reads it. Only run the give-up cleanup when nothing is queued. + if (this._state.pendingJoin === null) { + this.onErrorCallback?.("e2ee_timeout"); + // The exchange timed out BEFORE room.connect(): no SFU participant + // exists, so no LiveKit webhook will ever clean up, and the server + // registered the join when it sent voice_token. Send voice_leave and + // leave the store's voice channel (like the reconnect-exhausted give-up + // path) or the stale row ghosts forever and can wedge the channel's + // key-holder election. + this.leaveVoice(true); + leaveVoiceChannel(); + } else { + // Leave state as "connecting" with pendingJoin intact so the finally + // block and handleVoiceToken's drain loop can run the queued join. + // Clear this attempt's own E2EE residue (keypair/_isKeyHolder/etc.) + // so the queued join does not inherit it — entry-point leaveVoice(false) + // never runs for that next call since `_room` is null here (OC-0001). + this._e2ee.clearState(); + } return false; } @@ -1097,10 +1160,7 @@ export class LiveKitSession { currentGeneration: this._state.type === "connecting" ? this._state.joinGeneration : "n/a", }); - localRoom.removeAllListeners(); - localRoom - .disconnect() - .catch((err) => log.debug("Failed to disconnect superseded room", err)); + this.disconnectSupersededLocalRoom(localRoom); return "superseded"; } @@ -1147,6 +1207,7 @@ export class LiveKitSession { channelId, attempt, }); + if (localRoom !== null) this.disconnectSupersededLocalRoom(localRoom); return "superseded"; } if (localRoom === null) throw connectErr; @@ -1301,7 +1362,16 @@ export class LiveKitSession { isKeyHolder?: boolean, ): Promise { const s = this._state; - if (s.type === "connected" && s.channelId === channelId && s.room.state === "connected") { + // OC-0015: livekit-client's own internal reconnect (network blip on the + // SFU signal socket) moves Room.state through "signalReconnecting" / + // "reconnecting" without ever emitting RoomEvent.Disconnected — the only + // event this session listens for — so `_state` stays "connected" the + // whole time. A routine 4-minute refresh token landing in that window + // must still take the lightweight refresh path instead of falling + // through to a full teardown+rejoin of a session that is about to + // recover on its own; only a room the SDK has fully given up on + // ("disconnected") should be treated as needing a real reconnect here. + if (s.type === "connected" && s.channelId === channelId && s.room.state !== "disconnected") { this.handleVoiceTokenRefresh(token); return; } @@ -1317,6 +1387,22 @@ export class LiveKitSession { log.warn("handleVoiceToken: already connecting, queued latest join request", { channelId }); return; } + // OC-0009: a voice_token can arrive after the user already left this + // channel (e.g. Disconnect fired before the voice_join/voice_token round + // trip returned) — `_state` alone cannot tell, since a leave that landed + // before any connectAndSetup() ever started leaves `_state` at "idle" + // either way. voiceStore.currentChannelId is the one place the leave is + // recorded independent of this session's own lifecycle: joinVoiceChannel() + // always sets it before the request that produced this token was sent, + // and leaveVoiceChannel() nulls it, so a mismatch here means the token is + // stale. Connecting anyway would silently rejoin the SFU and republish + // the mic for a call the UI, store, and server all consider ended. + if (voiceStore.getState().currentChannelId !== channelId) { + log.info("handleVoiceToken: voice_token for a channel we already left — ignoring", { + channelId, + }); + return; + } await this.connectAndSetup(token, url, channelId, directUrl, isKeyHolder); // Drain pending joins iteratively to avoid unbounded recursion when // rapid channel switches queue multiple requests. @@ -1338,7 +1424,7 @@ export class LiveKitSession { if ( cur.type === "connected" && cur.channelId === pChannelId && - cur.room.state === "connected" + cur.room.state !== "disconnected" ) { this.handleVoiceTokenRefresh(pToken); } else { @@ -1437,6 +1523,10 @@ export class LiveKitSession { } this._pendingReconnectFields = null; this.clearTokenRefreshTimer(); + // OC-0029: a fresh join must never inherit the outgoing session's refresh + // budget — otherwise a rejoin shortly after a leave could get silently + // throttled for up to 60s with no refresh sent at all. + this._lastTokenRefreshSentAt = 0; this._audioPipeline.teardownAudioPipeline(); this._eventHandlers.removeAutoplayUnlock(); // OC-0042: bump first, mirroring doDisableCamera/doDisableScreenshare — diff --git a/Client/tauri-client/src/lib/screenShare.ts b/Client/tauri-client/src/lib/screenShare.ts index 5a194ef0..7b60c1cb 100644 --- a/Client/tauri-client/src/lib/screenShare.ts +++ b/Client/tauri-client/src/lib/screenShare.ts @@ -271,6 +271,15 @@ export async function enableCamera(state: CameraTrackState, deps: VideoTrackDeps deps.reapplyAudioPipeline(); log.info("Camera enabled", { quality, maxBitrate: CAMERA_PUBLISH_BITRATES[quality] }); } catch (err) { + if ((state.generation ?? 0) !== generation) { + // A disableCamera (and possibly a newer enableCamera) already ran to + // completion while this attempt's device acquisition/publish was in + // flight. This attempt's own track, if any, was already released by + // that disable — touching shared state now would stop/clear a live + // track that belongs to a newer, successful enable. + log.warn("Superseded camera enable failed — leaving newer attempt's state alone", err); + return; + } // BUG-100: Stop the created track to release the camera if publish failed. if (state.manualCameraTrack !== null) { state.manualCameraTrack.stop(); @@ -410,6 +419,15 @@ export async function enableScreenshare( deps.reapplyAudioPipeline(); log.info("Screenshare enabled", { quality, fps: effectiveFps, maxBitrate }); } catch (err) { + if ((state.generation ?? 0) !== generation) { + // A disableScreenshare (and possibly a newer enableScreenshare) already + // ran to completion while this attempt's capture/publish was in + // flight. This attempt's own tracks, if any, were already released by + // that disable — calling stopManualScreenTracks now would unpublish + // and stop tracks that belong to a newer, successful enable. + log.warn("Superseded screenshare enable failed — leaving newer attempt's state alone", err); + return; + } // BUG-100 (+ partial-publish-failure hardening): release every created // track, not just stop() it — a track already published before a later // one in the batch fails (all quality presets request audio alongside diff --git a/Client/tauri-client/src/lib/ws.ts b/Client/tauri-client/src/lib/ws.ts index 69a9e55f..3a6b627c 100644 --- a/Client/tauri-client/src/lib/ws.ts +++ b/Client/tauri-client/src/lib/ws.ts @@ -542,6 +542,15 @@ export function createWsClient() { try { await tauriInvoke("ws_connect", { url: wsUrl }); } catch (err) { + if (gen !== wsGeneration) { + // A disconnect() (or a newer connect()) landed while we were + // suspended on the Tauri IPC round trip — this rejection belongs to + // a superseded attempt (the Rust proxy deliberately rejects a + // handshake it displaced with "superseded by a newer connection"). + // The newer attempt may already be connected; do not act on it. + log.debug("ws_connect rejection from superseded attempt, ignoring", err); + return; + } log.error("ws_connect failed", err); proxyOpen = false; diff --git a/Client/tauri-client/src/main.ts b/Client/tauri-client/src/main.ts index 31ca8f5c..d0ccded1 100644 --- a/Client/tauri-client/src/main.ts +++ b/Client/tauri-client/src/main.ts @@ -655,6 +655,17 @@ async function renderPage(pageId: "connect" | "main"): Promise { runHealthChecks(connectPage, getProfileList()); } + // Consume any pending skip-auto-login flag on THIS mount regardless of + // which branch below returns early. It is the single source of truth + // for "an explicit logout just happened, don't auto-login" (set by the + // isAuthenticated subscriber further down), and every connect-page + // mount — quick-switch included — must clear it here or it survives in + // sessionStorage and goes on to suppress an unrelated, later + // clearAuth("server_shutdown") auto-login that deliberately does NOT + // re-set it (OC-0028). + const skipAutoLogin = sessionStorage.getItem("owncord:skip-auto-login") !== null; + sessionStorage.removeItem("owncord:skip-auto-login"); + // Quick-switch: if the user switched servers via the overlay, auto-select // the target server profile so they can reconnect with one click. const quickSwitchTarget = sessionStorage.getItem("owncord:quick-switch-target"); @@ -677,8 +688,7 @@ async function renderPage(pageId: "connect" | "main"): Promise { // Suppressing the attempt removes the race instead of relying on the // delete being dispatched early enough to win it — and an auto-login // immediately after an explicit logout is wrong regardless of timing. - if (sessionStorage.getItem("owncord:skip-auto-login") !== null) { - sessionStorage.removeItem("owncord:skip-auto-login"); + if (skipAutoLogin) { return; } diff --git a/Client/tauri-client/src/pages/MainPage.ts b/Client/tauri-client/src/pages/MainPage.ts index d16e17bf..7859c332 100644 --- a/Client/tauri-client/src/pages/MainPage.ts +++ b/Client/tauri-client/src/pages/MainPage.ts @@ -619,7 +619,7 @@ export function createMainPage(options: MainPageOptions): MountableComponent { const ringing = ringCtrl?.current(); if (ringing === null || ringing === undefined) return; if (payload.user_id === ringing.fromUserId) { - ringCtrl?.cancel(ringing.channelId); + ringCtrl?.cancel(payload.channel_id); } }), ); diff --git a/Client/tauri-client/src/pages/connect-page/LoginForm.ts b/Client/tauri-client/src/pages/connect-page/LoginForm.ts index 50250443..68d6ed54 100644 --- a/Client/tauri-client/src/pages/connect-page/LoginForm.ts +++ b/Client/tauri-client/src/pages/connect-page/LoginForm.ts @@ -672,6 +672,12 @@ export function createLoginForm(opts: LoginFormOptions): LoginFormApi { try { await onTotpSubmit(code); + // Verify succeeded — the challenge is resolved, so drop the latch. + // Otherwise any later, unrelated error (e.g. the post-auth WS connect + // failing) would hit the `formState === "error" && totpPending` branch + // in updateTotpOverlay() and re-open this now-dead overlay, whose + // partial token has already been consumed by main.ts. + totpPending = false; } catch (err) { const message = err instanceof Error ? err.message : "Verification failed."; transitionTo("error", message); diff --git a/Client/tauri-client/src/pages/main-page/VideoModeController.ts b/Client/tauri-client/src/pages/main-page/VideoModeController.ts index c71c003d..6ee2e8cb 100644 --- a/Client/tauri-client/src/pages/main-page/VideoModeController.ts +++ b/Client/tauri-client/src/pages/main-page/VideoModeController.ts @@ -53,6 +53,14 @@ export function createVideoModeController(opts: VideoModeControllerOptions): Vid let localTileAdded = false; let localScreenshareTileAdded = false; let focusedTileId: number | null = null; + /** The channel currentChannelId was on the previous checkVideoMode() call. + * A voice channel switch (A -> B) moves currentChannelId directly from A + * to B without ever passing through null (joinVoiceChannel is + * optimistic), so clearStreams() must key off any change of channel id, + * not just the transition to null — otherwise remote tiles from the old + * channel persist as dead MediaStreams and keep hasStreams() true + * forever (OC-0012). */ + let lastChannelId: number | null = null; /** Set when the user explicitly dismisses the grid while local video is * still on (switching to a text channel). Without this, checkVideoMode() * re-opens the grid the moment any remote peer's camera/screenshare @@ -104,17 +112,23 @@ export function createVideoModeController(opts: VideoModeControllerOptions): Vid function checkVideoMode(): void { const voice = voiceStore.getState(); const channelId = voice.currentChannelId; + // Clear stale remote tiles on ANY change of channel — a real leave + // (channelId -> null) and a direct A -> B switch both need it, since + // VoiceCallbacks.onVoiceJoin moves currentChannelId straight from the + // old channel to the new one without ever passing through null. + // currentChannelId stays unchanged across auto-reconnect, so this does + // not fire on reconnect (B1-8, OC-0012). + if (channelId !== lastChannelId) { + if (lastChannelId !== null) { + videoGrid.clearStreams(); + } + lastChannelId = channelId; + } if (channelId === null) { // Not a dismissal: leaving voice can clear currentChannelId before // localCamera/localScreenshare go false, and this early return skips // the reset below — showChat() here would strand userDismissedVideo // set and suppress auto-open for the next session. - // - // This is a real leave (not a reconnect — currentChannelId stays set - // during auto-reconnect), so clear any remote tiles left over from the - // ended session too (B1-8) — otherwise they persist as dead - // MediaStreams and keep hasStreams() true for the next join. - videoGrid.clearStreams(); closeVideoGrid(); return; } @@ -222,6 +236,7 @@ export function createVideoModeController(opts: VideoModeControllerOptions): Vid localTileAdded = false; localScreenshareTileAdded = false; userDismissedVideo = false; + lastChannelId = null; } return { diff --git a/Client/tauri-client/tests/unit/dispatcher.test.ts b/Client/tauri-client/tests/unit/dispatcher.test.ts index 5fa561a7..45559ae2 100644 --- a/Client/tauri-client/tests/unit/dispatcher.test.ts +++ b/Client/tauri-client/tests/unit/dispatcher.test.ts @@ -281,6 +281,49 @@ describe("WS Dispatcher", () => { expect(voiceStore.getState().voiceUsers.size).toBe(1); }); + // OC-0014: a moderator deafen/mute has no SFU equivalent — the client is + // the only place it takes effect (via setDeafened/setMuted, which gate + // remote-audio subscription). The VOICE_STATE handler enforces this, but a + // full-ready resync (a WS drop that outlives the LiveKit session, followed + // by a mustFullResync reconnect) only ever delivers the mute/deafen via + // `ready`'s voice_states, never a voice_state the client can miss. Without + // enforcement on this path too, the widget shows the user as deafened while + // every remote publication stays subscribed and audible. + it("enforces a moderator server-deafen/mute from the ready (full-resync) payload", async () => { + authStore.setState((prev) => ({ + ...prev, + user: { id: 5, username: "me", avatar: null, role: "member" }, + })); + // The LiveKit session survived the WS drop (nothing tears voice down on a + // socket drop alone) — voiceStatus is NOT idle, so the "stale voice + // state" defense-in-depth branch below must not fire and swallow this. + voiceStore.setState((prev) => ({ + ...prev, + currentChannelId: 3, + voiceStatus: "connected", + })); + + mock.dispatch("ready", { + channels: [], + members: [{ id: 5, username: "me", avatar: null, role: "member", status: "online" }], + voice_states: [ + { + channel_id: 3, + user_id: 5, + muted: true, + deafened: true, + server_muted: true, + server_deafened: true, + }, + ], + roles: [], + }); + await vi.runAllTimersAsync(); + + expect(vi.mocked(mockSetDeafened)).toHaveBeenCalledWith(true); + expect(vi.mocked(mockSetMuted)).toHaveBeenCalledWith(true); + }); + it("wires chat_message to messages store", () => { mock.dispatch("chat_message", { id: 100, @@ -480,6 +523,56 @@ describe("WS Dispatcher", () => { expect.objectContaining({ content: "live after reconnect, server clock still lagging" }), ); }); + + // OC-0024: serverClockSkewMs starts at 0 and is only ever sampled inside + // the "accepted as live" branch it itself gates — so if the channel is + // quiet between login and the first reconnect, nothing ever seeds it. A + // few seconds later a genuinely live message shows up with a lagging + // server timestamp and gets misclassified as a replay forever (the + // classification that misfires is the same one that would have fixed + // it). This must not depend on the reconnect having happened recently — + // a real fix bounds exposure by wall-clock distance from the handshake, + // not by ever successfully sampling the skew on this connection. + it("does not permanently blackhole live messages after a reconnect when the skew was never sampled (cold start)", () => { + const driftMs = 30 * 60 * 1000; // server clock reads 30 minutes behind + const t0 = Date.now(); + + // First connect — the channel is quiet, so no chat_message arrives and + // serverClockSkewMs is never sampled. + mock.dispatch("auth_ok", { + user: { id: 1, username: "alex", avatar: null, role: "admin" }, + server_name: "TestServer", + motd: "", + }); + + // Wi-Fi blips a few seconds later; ws.ts reconnects. + vi.setSystemTime(t0 + 5000); + const handshakeAt = Date.now(); + mock.dispatch("auth_ok", { + user: { id: 1, username: "alex", avatar: null, role: "admin" }, + server_name: "TestServer", + motd: "", + }); + + // A genuinely live message arrives well after the reconnect burst + // would have finished — its server timestamp is 30 minutes behind + // because the self-hosted server has no NTP. + vi.setSystemTime(handshakeAt + 10_000); + mock.dispatch("chat_message", { + id: 1, + channel_id: 1, + user: { id: 2, username: "bob", avatar: null }, + content: "live well after reconnect, skew was never sampled", + reply_to: null, + attachments: [], + timestamp: new Date(Date.now() - driftMs).toISOString(), + }); + + expect(mockNotifyIncomingMessage).toHaveBeenCalledTimes(1); + expect(mockNotifyIncomingMessage).toHaveBeenCalledWith( + expect.objectContaining({ content: "live well after reconnect, skew was never sampled" }), + ); + }); }); describe("mention counts", () => { @@ -2175,6 +2268,53 @@ describe("WS Dispatcher", () => { expect(voiceStore.getState().currentChannelId).toBeNull(); }); + // OC-0031: voice_disconnected can arrive from behind a backed-up outbound + // queue well after the kick already tore the LiveKit session down at the + // SFU — by the time it's finally delivered, the user may have already + // rejoined (this channel or another). Its sibling VOICE_LEAVE handler + // guards on channel match for exactly this staleness; voice_disconnected + // must too, or the stale frame kills the freshly established session. + it("does not tear down a fresher voice session for a stale queued voice_disconnected", async () => { + vi.mocked(mockLeaveVoice).mockClear(); + mockShowToast.mockClear(); + authStore.setState((prev) => ({ + ...prev, + user: { id: 5, username: "me", avatar: null, role: "member" }, + })); + // A newer session is live in channel 9; the queued voice_disconnected is + // for the old channel 3 the kick already evicted us from. + voiceStore.setState((prev) => ({ ...prev, currentChannelId: 9 })); + + mock.dispatch("voice_disconnected", { channel_id: 3, reason: "kicked" }); + await vi.runAllTimersAsync(); + + expect(mockLeaveVoice).not.toHaveBeenCalled(); + expect(voiceStore.getState().currentChannelId).toBe(9); + expect(mockShowToast).not.toHaveBeenCalled(); + }); + + // OC-0033: on the ordinary kick path, the server's finishVoiceLeave + // broadcasts voice_leave to the leaver BEFORE handleVoiceModKickV2 ever + // sends voice_disconnected, so the sibling VOICE_LEAVE handler above has + // already nulled currentChannelId by the time this event lands. A cleared + // store is not the same staleness OC-0031 guards against (a rejoin into a + // *different* channel) and must not swallow the kick toast — it's the only + // explanation the user gets for being dropped from the call. + it("still surfaces the kick toast after the sibling voice_leave already cleared the store", async () => { + mockShowToast.mockClear(); + authStore.setState((prev) => ({ + ...prev, + user: { id: 5, username: "me", avatar: null, role: "member" }, + })); + // voice_leave for this same kick already cleared the store. + voiceStore.setState((prev) => ({ ...prev, currentChannelId: null })); + + mock.dispatch("voice_disconnected", { channel_id: 3, reason: "kicked by a moderator" }); + await vi.runAllTimersAsync(); + + expect(mockShowToast).toHaveBeenCalledWith("kicked by a moderator", "error"); + }); + it("wires voice_config to voice store", () => { mock.dispatch("voice_config", { channel_id: 3, @@ -3502,6 +3642,40 @@ describe("WS Dispatcher", () => { expect(mockShowToast).toHaveBeenCalledWith("nope", "error"); expect(uiStore.getState().transientError).toBeNull(); }); + + // OC-0032: voice_controls.go now routes a refused voice_screenshare + // enable through the same enableVideoSlot cap check as the camera, so + // VIDEO_LIMIT can correlate to a "screen" pending enable, not just + // "camera". Rolling back the camera unconditionally would tear down a + // working camera and leave the refused screen tracks published. + it("rolls back the screenshare publish, not the camera, when VIDEO_LIMIT refuses a screenshare enable", async () => { + vi.mocked(mockRollbackPendingVideo).mockReturnValue("screen"); + + mock.dispatch("error", { code: "VIDEO_LIMIT", message: "" }, "vid-screen-1"); + await vi.runAllTimersAsync(); + + expect(mockRollbackPendingVideo).toHaveBeenCalledWith("vid-screen-1"); + expect(mockDisableScreenshare).toHaveBeenCalled(); + expect(mockDisableCamera).not.toHaveBeenCalled(); + }); + + // OC-0035: a screenshare enable can be superseded (disable, re-enable) + // before its VIDEO_LIMIT refusal arrives — registerPendingVideoEnable + // deletes the prior entry for that kind, so rollbackPendingVideo(id) + // returns undefined for the now-stale id. undefined means "roll back + // nothing" (the refusal no longer correlates to anything pending), never + // "it was the camera" — falling through to disableCamera() would tear + // down a working camera the user never touched. + it("does nothing when VIDEO_LIMIT correlates to a superseded id (kind undefined)", async () => { + vi.mocked(mockRollbackPendingVideo).mockReturnValue(undefined); + + mock.dispatch("error", { code: "VIDEO_LIMIT", message: "" }, "vid-superseded-1"); + await vi.runAllTimersAsync(); + + expect(mockRollbackPendingVideo).toHaveBeenCalledWith("vid-superseded-1"); + expect(mockDisableCamera).not.toHaveBeenCalled(); + expect(mockDisableScreenshare).not.toHaveBeenCalled(); + }); }); }); diff --git a/Client/tauri-client/tests/unit/livekit-e2ee.test.ts b/Client/tauri-client/tests/unit/livekit-e2ee.test.ts index 473ead1b..3f74e8df 100644 --- a/Client/tauri-client/tests/unit/livekit-e2ee.test.ts +++ b/Client/tauri-client/tests/unit/livekit-e2ee.test.ts @@ -1125,4 +1125,90 @@ describe("E2EEManager", () => { vi.useRealTimers(); } }); + + // ── Ledger findings OC-0002 / OC-0020 ───────────────────────────────── + + it("[OC-0002] applies an offer that arrives while its sender's own announce is still verifying, instead of dropping it as an unknown peer", async () => { + const ws = { send: vi.fn() }; + const mgr = createManager(ws); + // We were previously key holder (mirrors B in the repro): own keypair + + // room key already established. + await mgr.setupKeyExchange(true, 1); + mockSetKey.mockClear(); + vi.mocked(unwrapRoomKey).mockClear(); + + // Stall the identity-pin lookup inside verifyPeerAnnounce so the + // announce is still mid-flight (queued on _announceChain, not yet + // applied to _peerPublicKeys) when the offer from the SAME sender + // arrives right behind it — exactly the WS delivery order OC-0098 + // guarantees the sender used. + let releasePin!: (v: { status: "unpinned" }) => void; + const stalledPin = new Promise<{ status: "unpinned" }>((resolve) => { + releasePin = resolve; + }); + vi.mocked(getIdentityPin).mockReturnValueOnce(stalledPin); + + const announcePromise = mgr.handleAnnounce(PEER_ID, "cGVlcg==", "sig"); + await vi.waitFor(() => expect(getIdentityPin).toHaveBeenCalled()); + + // The offer is dispatched immediately behind the announce, before the + // announce has stored the peer's ECDH key. + const offerPromise = mgr.handleOffer(PEER_ID, "enc", "iv"); + + releasePin({ status: "unpinned" }); + await Promise.all([announcePromise, offerPromise]); + + // The offer must have been applied once the announce (which arrived + // first) finished verifying — not silently dropped as "unknown peer" + // with no retry until the next 5-minute rotation. + expect(mgr.peerPublicKeys.has(PEER_ID)).toBe(true); + expect(unwrapRoomKey).toHaveBeenCalled(); + expect(mockSetKey).toHaveBeenCalledWith("mock-room-key-base64"); + }); + + it("[OC-0020] retires a departed peer's key on leave, so a replay of it after they rejoin with a fresh key cannot resurrect it", async () => { + const ws = { send: vi.fn() }; + const mgr = createManager(ws); + await mgr.setupKeyExchange(true, 1); // establishes our keypair, holder + + // Make import/export round-trip faithfully on the announced base64 + // string (the shared mock default returns a fixed constant regardless + // of input, which would mask this bug). + vi.mocked(importPublicKey).mockImplementation( + async (b64: string) => ({ type: `peer-key-${b64}` }) as unknown as CryptoKey, + ); + vi.mocked(exportPublicKey).mockImplementation(async (key: CryptoKey) => + (key as unknown as { type: string }).type.replace("peer-key-", ""), + ); + + const KEY_A = "b2xk"; + const KEY_B = "bmV3"; + + try { + // Peer announces key A — accepted as their first (live) key. + await mgr.handleAnnounce(PEER_ID, KEY_A, "sigA"); + expect(mgr.peerPublicKeys.get(PEER_ID)).toEqual({ type: `peer-key-${KEY_A}` }); + + // Peer leaves the channel. + await mgr.handleParticipantLeft(PEER_ID); + expect(mgr.peerPublicKeys.has(PEER_ID)).toBe(false); + + // Peer rejoins and announces a fresh key B. + await mgr.handleAnnounce(PEER_ID, KEY_B, "sigB"); + expect(mgr.peerPublicKeys.get(PEER_ID)).toEqual({ type: `peer-key-${KEY_B}` }); + + // A malicious relay re-emits the pre-leave, still validly-signed + // announce for key A. No channel/epoch/nonce binds the signed + // message, so it verifies cleanly — it must still be rejected as a + // replay of a retired key, not overwrite the live key B. + await mgr.handleAnnounce(PEER_ID, KEY_A, "sigA"); + + expect(mgr.peerPublicKeys.get(PEER_ID)).toEqual({ type: `peer-key-${KEY_B}` }); + } finally { + vi.mocked(importPublicKey).mockImplementation( + async () => ({ type: "public" }) as unknown as CryptoKey, + ); + vi.mocked(exportPublicKey).mockImplementation(async () => "bW9ja2VwaGVtZXJhbA=="); + } + }); }); diff --git a/Client/tauri-client/tests/unit/livekit-session.test.ts b/Client/tauri-client/tests/unit/livekit-session.test.ts index c1813424..396070ec 100644 --- a/Client/tauri-client/tests/unit/livekit-session.test.ts +++ b/Client/tauri-client/tests/unit/livekit-session.test.ts @@ -10,6 +10,13 @@ const mockVoiceState = vi.hoisted(() => ({ localCamera: false, localScreenshare: false, pttGated: false, + // OC-0009: the real store's currentChannelId is set by joinVoiceChannel() + // before the voice_join/voice_token round trip that produces the token a + // test then hands to handleVoiceToken — default it to the channel id used + // by the overwhelming majority of existing calls (1) so those tests don't + // each need to restate it; tests that exercise a different channel (or the + // "already left" guard itself) set this explicitly. + currentChannelId: 1 as number | null, })); /** Backing cell for the mocked voice.store PTT-poller-live flag. Boxed so the @@ -318,6 +325,7 @@ describe("LiveKitSession", () => { mockVoiceState.localCamera = false; mockVoiceState.localScreenshare = false; mockVoiceState.pttGated = false; + mockVoiceState.currentChannelId = 1; session = new LiveKitSession(); // Reset mockRoom state mockRoom.state = "connected"; @@ -1013,7 +1021,9 @@ describe("LiveKitSession", () => { // The user switches to channel 2, which already has a lower-uid // participant — the server elects someone else and sends - // is_key_holder=false. + // is_key_holder=false. joinVoiceChannel(2) always runs before this + // token's round trip in the real app (OC-0009's guard reads it back). + mockVoiceState.currentChannelId = 2; const joinPromise = session.handleVoiceToken( "token-2", "/livekit", @@ -1574,6 +1584,7 @@ describe("LiveKitSession", () => { it("sets currentChannelId to null after leave", async () => { session.setServerHost("localhost:7880"); session.setWsClient({ send: vi.fn() } as any); + mockVoiceState.currentChannelId = 5; await session.handleVoiceToken("tok", "/lk", 5, "ws://localhost:7880", true); expect((session as any)._state.channelId).toBe(5); @@ -1947,6 +1958,7 @@ describe("LiveKitSession", () => { session.setOnError(errorCb); mockVoiceState.localMuted = false; mockVoiceState.localDeafened = false; + mockVoiceState.currentChannelId = 7; await session.handleVoiceToken("tok", "/lk", 7, "ws://localhost:7880", true); vi.clearAllMocks(); @@ -3499,4 +3511,274 @@ describe("LiveKitSession", () => { expect(offerSends(ws)).toHaveLength(0); }); }); + + // ----------------------------------------------------------------------- + // bughunt-fix wave 2: OC-0001, OC-0006, OC-0009, OC-0010, OC-0015, OC-0029 + // ----------------------------------------------------------------------- + + describe("[OC-0001] pending-join drain re-enters connectAndSetup without E2EE teardown", () => { + it("does not carry a stale key-holder promotion into a join drained while still 'connecting'", async () => { + session.setServerHost("localhost:7880"); + session.setWsClient({ send: vi.fn() } as any); + + // Channel A: connect() stalls so we can queue a join for channel B + // before A's attempt reaches its own supersession checkpoint. + const connectA = createDeferred(); + mockRoom.connect + .mockImplementationOnce(() => connectA.promise) + .mockResolvedValueOnce(undefined); + + const joinPromise = session.handleVoiceToken( + "token-a", + "/livekit", + 1, + "ws://localhost:7880", + true, // key holder for channel A + ); + // Let E2EE key-exchange (key-holder path — no wait) and room.connect() + // start; A's attempt is now stalled inside room.connect(). + await vi.advanceTimersByTimeAsync(0); + expect((session as any)._e2ee["_isKeyHolder"]).toBe(true); + + // The user switches to channel B before A's connect() resolves. The + // server elects a different key holder for B. + mockVoiceState.currentChannelId = 2; + await session.handleVoiceToken("token-b", "/livekit", 2, "ws://localhost:7880", false); + expect((session as any)._state.type).toBe("connecting"); + expect((session as any)._state.pendingJoin?.channelId).toBe(2); + + // Observe _isKeyHolder at the exact moment channel B's own key exchange + // begins — before any later leaveVoice()/timeout path could mask a + // residual value left over from A by resetting it via a different route. + let isKeyHolderAtBStart: boolean | undefined; + const keyExchangeSpy = vi + .spyOn((session as any)._e2ee, "setupKeyExchange") + .mockImplementation(async () => { + isKeyHolderAtBStart = (session as any)._e2ee["_isKeyHolder"]; + return true; // fast-path: pretend the room key is already available + }); + + // A's connect() now resolves — connectAndSetup(A) discards its own + // room in favor of the queued B join (the queuedJoin branch) and + // returns false, leaving state "connecting" with pendingJoin=B intact + // so handleVoiceToken's drain loop runs it next. + connectA.resolve(undefined); + await joinPromise; + + // The residual key-holder promotion from channel A must not have + // leaked into channel B's own (correctly non-holder) election. + expect(isKeyHolderAtBStart).toBe(false); + + keyExchangeSpy.mockRestore(); + }); + }); + + describe("[OC-0006] connectAndSetup supersession checkpoints re-sync module room wiring", () => { + it("unwires DeviceManager/AudioPipeline/AudioElements when checkpoint 1 fires after a concurrent leaveVoice that landed during room creation", async () => { + session.setServerHost("localhost:7880"); + session.setWsClient({ send: vi.fn() } as any); + + // Stall createRoom()'s own `await newRoom.setE2EEEnabled(true)` — this + // is BEFORE connectAndSetup's module-wiring lines (which run right + // after createRoom() resolves) have executed at all. + const e2eeDeferred = createDeferred(); + mockRoom.setE2EEEnabled.mockImplementationOnce(() => e2eeDeferred.promise); + + const resultPromise = (session as any).connectAndSetup( + "token-1", + "/livekit", + 1, + "ws://localhost:7880", + true, + ); + await vi.advanceTimersByTimeAsync(0); + + // A concurrent Disconnect click runs leaveVoice() here. `_room` reads + // null (state is "connecting", no room installed yet), so there is + // nothing to unwire — this only proves the leave itself ran cleanly. + session.leaveVoice(false); + expect((session as any)._deviceManager.room).toBeNull(); + + // createRoom() now resolves. connectAndSetup's module-wiring lines run + // UNCONDITIONALLY right after, re-wiring the modules to a room that + // state ("idle", from the leave above) says nobody wants any more. + // resolveLiveKitUrl resolves synchronously for a local host, so + // checkpoint 1 fires immediately after — it must detect the leave and + // leave the modules unwired rather than stranding them on a room that + // will never connect. + e2eeDeferred.resolve(undefined); + const result = await resultPromise; + + expect(result).toBe("superseded"); + expect((session as any)._deviceManager.room).toBeNull(); + }); + }); + + describe("[OC-0009] handleVoiceToken ignores a voice_token for a channel already left", () => { + it("does not connect when the token arrives after the user left before connectAndSetup ever started", async () => { + session.setServerHost("localhost:7880"); + session.setWsClient({ send: vi.fn() } as any); + // The user clicked Disconnect (leaveVoiceChannel nulls this) before the + // voice_join/voice_token round trip for the earlier join returned. + mockVoiceState.currentChannelId = null; + + await session.handleVoiceToken("stale-token", "/livekit", 1, "ws://localhost:7880", true); + + expect(mockRoom.connect).not.toHaveBeenCalled(); + expect((session as any)._state.type).toBe("idle"); + }); + + it("does not connect when the token is for a channel the user has since switched away from", async () => { + session.setServerHost("localhost:7880"); + session.setWsClient({ send: vi.fn() } as any); + mockVoiceState.currentChannelId = 2; + + await session.handleVoiceToken("stale-token", "/livekit", 1, "ws://localhost:7880", true); + + expect(mockRoom.connect).not.toHaveBeenCalled(); + expect((session as any)._state.type).toBe("idle"); + }); + + it("still connects when the token matches the channel the user currently wants", async () => { + session.setServerHost("localhost:7880"); + session.setWsClient({ send: vi.fn() } as any); + mockVoiceState.currentChannelId = 1; + + await session.handleVoiceToken("fresh-token", "/livekit", 1, "ws://localhost:7880", true); + + expect(mockRoom.connect).toHaveBeenCalledWith("ws://localhost:7880", "fresh-token"); + expect((session as any)._state.type).toBe("connected"); + }); + }); + + describe("[OC-0010] e2ee-timeout cleanup honors a queued pendingJoin", () => { + it("does not send voice_leave / leaveVoiceChannel and preserves the queued join when the key exchange times out with a join queued", async () => { + const ws = { send: vi.fn() }; + session.setServerHost("localhost:7880"); + session.setWsClient(ws as any); + + // Force setupKeyExchange to report a genuine timeout while a newer + // join has ALREADY been queued — mirrors handleVoiceToken's queuing + // branch, which preserves this attempt's type/joinGeneration. + const keyExchangeSpy = vi + .spyOn((session as any)._e2ee, "setupKeyExchange") + .mockImplementation(async () => { + (session as any)._state = { + ...(session as any)._state, + pendingJoin: { + token: "token-b", + url: "/livekit-b", + channelId: 2, + directUrl: undefined, + }, + }; + return false; + }); + + const result = await (session as any).connectAndSetup( + "token-a", + "/livekit", + 1, + "ws://localhost:7880", + false, + ); + + expect(result).toBe(false); + // Must NOT run the give-up cleanup — that would send a voice_leave + // that carries no channel id (acting on whichever channel the queued + // join is about to occupy) and would discard the queued join entirely. + expect(ws.send).not.toHaveBeenCalledWith({ type: "voice_leave", payload: {} }); + expect(leaveVoiceChannel).not.toHaveBeenCalled(); + // The queued join must survive so handleVoiceToken's drain loop can run it. + expect((session as any)._state.type).toBe("connecting"); + expect((session as any)._state.pendingJoin?.channelId).toBe(2); + + keyExchangeSpy.mockRestore(); + }); + }); + + describe("[OC-0015] token refresh survives livekit-client's own internal reconnect", () => { + it("takes the refresh fast path when Room.state is mid-internal-reconnect instead of tearing down the session", async () => { + session.setServerHost("localhost:7880"); + session.setWsClient({ send: vi.fn() } as any); + mockRoom.connect.mockResolvedValue(undefined); + await session.handleVoiceToken("token-1", "/livekit", 1, "ws://localhost:7880", true); + expect((session as any)._state.type).toBe("connected"); + + // livekit-client's own internal reconnect (a signal-socket blip) — + // RoomEvent.Disconnected is NOT emitted for this, so `_state` stays + // "connected", but Room.state moves off "connected". + mockRoom.state = "signalReconnecting"; + mockRoom.connect.mockClear(); + + const refreshSpy = vi.spyOn(session, "handleVoiceTokenRefresh"); + await session.handleVoiceToken("token-2", "/livekit", 1, "ws://localhost:7880", true); + + expect(refreshSpy).toHaveBeenCalledWith("token-2"); + // Must NOT re-run the full teardown+rejoin — that would drop the E2EE + // session and could eject the user if no offer arrives in time. + expect(mockRoom.connect).not.toHaveBeenCalled(); + + refreshSpy.mockRestore(); + }); + }); + + describe("[OC-0029] requestTokenRefresh throttles to the server's 1-per-60s budget", () => { + it("does not resend voice_token_refresh within 60s of the previous one", async () => { + const mockWs = { send: vi.fn() } as any; + session.setWsClient(mockWs); + session.setServerHost("localhost:7880"); + mockRoom.connect.mockResolvedValue(undefined); + await session.handleVoiceToken("token", "/livekit", 1, "ws://localhost:7880", true); + + mockWs.send.mockClear(); + (session as any).requestTokenRefresh(); + expect(mockWs.send).toHaveBeenCalledWith({ type: "voice_token_refresh", payload: {} }); + + mockWs.send.mockClear(); + // Mirrors OC-0029's repro: auto-reconnect's unconditional post-recovery + // refresh landing ~13s after the 4-minute timer's own refresh. + await vi.advanceTimersByTimeAsync(13_000); + (session as any).requestTokenRefresh(); + + expect(mockWs.send).not.toHaveBeenCalled(); + }); + + it("allows a refresh again once the 60s budget window has passed", async () => { + const mockWs = { send: vi.fn() } as any; + session.setWsClient(mockWs); + session.setServerHost("localhost:7880"); + mockRoom.connect.mockResolvedValue(undefined); + await session.handleVoiceToken("token", "/livekit", 1, "ws://localhost:7880", true); + + mockWs.send.mockClear(); + (session as any).requestTokenRefresh(); + expect(mockWs.send).toHaveBeenCalledTimes(1); + + mockWs.send.mockClear(); + await vi.advanceTimersByTimeAsync(60_100); + (session as any).requestTokenRefresh(); + + expect(mockWs.send).toHaveBeenCalledWith({ type: "voice_token_refresh", payload: {} }); + }); + + it("does not throttle a fresh join shortly after leaveVoice resets the budget", async () => { + const mockWs = { send: vi.fn() } as any; + session.setWsClient(mockWs); + session.setServerHost("localhost:7880"); + mockRoom.connect.mockResolvedValue(undefined); + await session.handleVoiceToken("token", "/livekit", 1, "ws://localhost:7880", true); + + (session as any).requestTokenRefresh(); + session.leaveVoice(false); + + mockVoiceState.currentChannelId = 1; + await session.handleVoiceToken("token-2", "/livekit", 1, "ws://localhost:7880", true); + mockWs.send.mockClear(); + + (session as any).requestTokenRefresh(); + + expect(mockWs.send).toHaveBeenCalledWith({ type: "voice_token_refresh", payload: {} }); + }); + }); }); diff --git a/Client/tauri-client/tests/unit/login-form-totp-success-latch.test.ts b/Client/tauri-client/tests/unit/login-form-totp-success-latch.test.ts new file mode 100644 index 00000000..9a2a255b --- /dev/null +++ b/Client/tauri-client/tests/unit/login-form-totp-success-latch.test.ts @@ -0,0 +1,80 @@ +// Regression test for OC-0027: the LoginForm `totpPending` latch is set by +// showTotp() and is only cleared by handleTotpCancel()/resetToIdle() — never +// on a *successful* verify. If a later, unrelated error arrives (e.g. the WS +// auth handshake fails after a successful TOTP verify), updateTotpOverlay()'s +// `formState === "error" && totpPending` branch re-opens the dead 2FA overlay +// over the error banner, and the user is stuck: onTotpSubmit's guard in +// main.ts silently no-ops because the partial token was already consumed. +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { createConnectPage } from "../../src/pages/ConnectPage"; +import type { ConnectPageCallbacks, SimpleProfile } from "../../src/pages/ConnectPage"; + +vi.mock("../../src/lib/credentials", () => ({ + loadCredential: vi.fn().mockResolvedValue(null), +})); + +vi.mock("../../src/components/SettingsOverlay", () => ({ + createSettingsOverlay: () => ({ + mount: vi.fn(), + destroy: vi.fn(), + }), +})); + +function makeCallbacks(overrides: Partial = {}): ConnectPageCallbacks { + return { + onLogin: vi.fn().mockResolvedValue(undefined), + onRegister: vi.fn().mockResolvedValue(undefined), + onTotpSubmit: vi.fn().mockResolvedValue(undefined), + ...overrides, + }; +} + +const testProfiles: SimpleProfile[] = [{ name: "Test Server", host: "localhost:8443" }]; + +describe("LoginForm TOTP latch after a successful verify", () => { + let container: HTMLDivElement; + + beforeEach(() => { + container = document.createElement("div"); + document.body.appendChild(container); + }); + + afterEach(() => { + container.remove(); + }); + + it("does not re-open the TOTP overlay for an error that arrives after a successful verify", async () => { + const onTotpSubmit = vi.fn().mockResolvedValue(undefined); + const page = createConnectPage(makeCallbacks({ onTotpSubmit }), testProfiles); + page.mount(container); + page.showTotp(); + + const totpOverlay = container.querySelector(".totp-overlay") as HTMLDivElement; + const totpInput = container.querySelector(".totp-overlay input") as HTMLInputElement; + const verifyBtn = container.querySelector(".totp-overlay .btn-primary") as HTMLButtonElement; + + totpInput.value = "111111"; + verifyBtn.click(); + + await vi.waitFor(() => expect(onTotpSubmit).toHaveBeenCalledTimes(1)); + await vi.waitFor(() => expect(verifyBtn.disabled).toBe(false)); + + // Verify succeeded — nothing rejected. The caller (main.ts) would now be + // driving the post-auth WS connect, e.g. via wirePostAuth. That connect + // can still fail (rotated JWT key, revoked token, ban) and surface an + // unrelated error through the same showError() path ConnectPage wires up + // to the uiStore transientError subscription. + page.showError("Connection failed: unauthorized"); + + // The dead 2FA overlay must NOT come back — the token it would collect + // can never be submitted again (the partial token was already consumed + // on success), so re-showing it strands the user with no working control + // except Cancel. + expect(totpOverlay.classList.contains("totp-overlay--hidden")).toBe(true); + + const errorBanner = container.querySelector(".error-banner") as HTMLDivElement; + expect(errorBanner.classList.contains("visible")).toBe(true); + + page.destroy?.(); + }); +}); diff --git a/Client/tauri-client/tests/unit/main-page.test.ts b/Client/tauri-client/tests/unit/main-page.test.ts index 31c41582..b963bf8e 100644 --- a/Client/tauri-client/tests/unit/main-page.test.ts +++ b/Client/tauri-client/tests/unit/main-page.test.ts @@ -501,6 +501,33 @@ describe("MainPage — video grid, DM profile panel, calls, settings", () => { expect(banner.style.display).toBe("none"); }); + it("does not cancel an incoming ring on a voice_leave for a different channel from the ringer (OC-0011)", () => { + const ws = fakeWs(); + uiStore.setState((prev) => ({ ...prev, connectionStatus: "connected" })); + + page = createMainPage({ ws, api: fakeApi() }); + page.mount(container); + + // Alice (10) rings this client's DM (channel 50). + ws.emit("call_incoming", { channel_id: 50, from_user: 10, username: "alice" }); + + const banner = document.querySelector('[data-testid="incoming-call-banner"]') as HTMLElement; + expect(banner.style.display).not.toBe("none"); + + // Alice also happens to be sitting in an unrelated server voice channel + // (99) and leaves it. Same user, wrong channel: this must not silence + // the DM ring — only a voice_leave for the ring's own channel (50) may. + ws.emit("voice_leave", { channel_id: 99, user_id: 10 }); + + expect(banner.style.display).not.toBe("none"); + + // Alice leaving the ring's own channel (the DM she was calling from) + // still cancels it. + ws.emit("voice_leave", { channel_id: 50, user_id: 10 }); + + expect(banner.style.display).toBe("none"); + }); + it("clears settingsOpen on destroy so the next page (e.g. ConnectPage after logout) doesn't inherit a stale open overlay", () => { page = createMainPage({ ws: fakeWs(), api: fakeApi() }); page.mount(container); diff --git a/Client/tauri-client/tests/unit/main.test.ts b/Client/tauri-client/tests/unit/main.test.ts index ebbb63f6..a5e0fb3c 100644 --- a/Client/tauri-client/tests/unit/main.test.ts +++ b/Client/tauri-client/tests/unit/main.test.ts @@ -20,7 +20,7 @@ * run the dispatcher's own auth_ok handler (which is what actually writes * authStore). */ -import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; // --------------------------------------------------------------------------- // Tauri API mocks — reuse the ws-mocks.ts event-registry helper so ws.ts's @@ -76,12 +76,18 @@ vi.mock("@lib/profiles", () => ({ })); // api.ts — only login() is exercised (it drives wirePostAuth); nothing else -// in this flow touches the REST client. +// in this flow touches the REST client. getConfig()/setConfig() track a real +// host so OC-0028's test can reproduce the isAuthenticated subscriber's +// `api.getConfig().host` read (main.ts:776) after a login sets it via +// `api.setConfig({ host })` (main.ts:515). const mockLogin = vi.fn(); +const mockApiState = { host: "" }; vi.mock("@lib/api", () => ({ createApiClient: vi.fn(() => ({ - setConfig: vi.fn(), - getConfig: vi.fn(() => ({ host: "" })), + setConfig: vi.fn((cfg: { host?: string; token?: string }) => { + if (cfg.host !== undefined) mockApiState.host = cfg.host; + }), + getConfig: vi.fn(() => ({ host: mockApiState.host })), login: (...args: unknown[]) => mockLogin(...args), getHealth: vi.fn().mockResolvedValue({ version: null, online_users: null }), })), @@ -115,6 +121,15 @@ vi.mock("@pages/ConnectPage", () => ({ }), })); +// MainPage.ts pulls in the whole chat/voice UI stack. OC-0028 needs a real +// "main" -> "connect" round trip through the router (main.ts only navigates +// away from "connect" via the connected overlay's onReady -> router.navigate +// ("main") callback), but doesn't care what MainPage renders, so stand in +// with the same lightweight shape main-page.test.ts's own mocks return. +vi.mock("@pages/MainPage", () => ({ + createMainPage: vi.fn(() => ({ mount: vi.fn(), destroy: vi.fn() })), +})); + // dispatcher.ts pulls in nearly every store/service in the app. Stand in // with a slim replacement that reproduces the one behavior these tests must // stay faithful to: the real dispatcher's auth_ok handler calls setAuth() on @@ -221,3 +236,53 @@ describe("main.ts connected overlay (OC-0063)", () => { expect(iconEl?.textContent).toBe("M"); // first letter of "My Guild", not "1" (host) or "" (blank auth) }); }); + +describe("main.ts connect-page skip-auto-login flag (OC-0028)", () => { + afterEach(() => { + sessionStorage.clear(); + mockApiState.host = ""; + }); + + it("consumes owncord:skip-auto-login on a quick-switch mount, not just on a plain logout", async () => { + // Reach "main" for host A via an ordinary login — the quick-switch + // overlay (SidebarArea.ts) can only fire from a live session. + await loginAndReachAuthOk("server-a.example:8443", "alex", { + user: { id: 1, username: "alex", avatar: null, role: "member" }, + server_name: "Server A", + motd: "", + }); + emitTauriEvent("ws-message", JSON.stringify({ type: "ready", payload: {} })); + // ConnectedOverlay.markReady() fires onReady after READY_DELAY_MS (800ms), + // which calls router.navigate("main") — main.ts's only route away from + // "connect", needed so a later navigate("connect") is a real transition + // and not a same-page no-op. + await vi.advanceTimersByTimeAsync(800); + + // Quick-switch overlay's flow (SidebarArea.ts:756-760): stash the target + // host, then log out via a bare clearAuth() (reason defaults to "user"). + // The isAuthenticated subscriber below (main.ts:750-788) turns that into + // a stored "owncord:skip-auto-login" flag, since host is set and + // logoutReason !== "server_shutdown". + sessionStorage.setItem("owncord:quick-switch-target", "server-b.example:8443"); + clearAuth(); + + // authStore notifications are microtask-deferred (see store.ts), and the + // connect page's own load-profiles IIFE awaits a mocked (but still + // native-Promise) loadProfiles() before reaching the quick-switch check — + // flush both hops. + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + + // Quick-switch consumed its own key... + expect(sessionStorage.getItem("owncord:quick-switch-target")).toBeNull(); + // ...and per OC-0028 must ALSO consume skip-auto-login on this same + // mount. Before the fix, the quick-switch branch returns early (line 669) + // without ever reaching the skip-auto-login read/remove at line 680-683, + // so the flag set by the clearAuth() above survives indefinitely — and + // would go on to suppress the auto-login that a later, unrelated + // clearAuth("server_shutdown") deliberately relies on. + expect(sessionStorage.getItem("owncord:skip-auto-login")).toBeNull(); + }); +}); diff --git a/Client/tauri-client/tests/unit/message-list.test.ts b/Client/tauri-client/tests/unit/message-list.test.ts index 860bef35..dc43f408 100644 --- a/Client/tauri-client/tests/unit/message-list.test.ts +++ b/Client/tauri-client/tests/unit/message-list.test.ts @@ -720,4 +720,40 @@ describe("MessageList", () => { expect(rowAfterReset!.textContent).toContain("v25"); }); }); + + describe("scrollToMessage vs renderWindow rebuild breaker", () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it("does not report success when renderWindow's own >30-in-2s breaker drops the rebuild", () => { + const many = Array.from({ length: 100 }, (_, i) => makeMessage({ id: i + 1 })); + setMessages(1, many); + msgList.mount(container); // mount's renderAll -> renderWindow: rebuild count = 1 + + // Every scrollToMessage call forces renderedStart = -1 and calls + // renderWindow() directly, bypassing renderAll's own (lower) rapid-fire + // limit. 29 more calls bring the shared renderWindow rebuild counter to + // 30 (still under the >30 breaker), each one landing normally. + for (let i = 0; i < 29; i++) { + expect(msgList.scrollToMessage(i + 1)).toBe(true); + } + + // The 30th call pushes the counter to 31 and trips the breaker inside + // renderWindow: it returns before reassigning renderedStart/renderedEnd + // or touching the DOM, so the target (far outside the last rendered + // window) never actually renders. + const result = msgList.scrollToMessage(90); + + // The rebuild did not happen — the row is not in the DOM — so this must + // be reported as a failed jump (matching the "false if the message is + // not in the loaded window" contract), not a false "true". + expect(container.querySelector('[data-testid="message-90"]')).toBeNull(); + expect(result).toBe(false); + }); + }); }); diff --git a/Client/tauri-client/tests/unit/screen-share-tracks.test.ts b/Client/tauri-client/tests/unit/screen-share-tracks.test.ts index db0a067b..80a53fa1 100644 --- a/Client/tauri-client/tests/unit/screen-share-tracks.test.ts +++ b/Client/tauri-client/tests/unit/screen-share-tracks.test.ts @@ -360,6 +360,42 @@ describe("enableCamera", () => { expect(state.manualCameraTrack).toBeNull(); expect(voiceStore.getState().localCamera).toBe(false); }); + + it("does not clear a newer attempt's track when a superseded enable's device acquisition rejects (OC-0007)", async () => { + const rig = fakeRoom(); + const deps = fakeDeps(rig.room); + const trackB = fakeVideoTrack(); + let rejectA!: (err: unknown) => void; + const promiseA = new Promise((_, reject) => { + rejectA = reject; + }); + createLocalVideoTrack.mockReturnValueOnce(promiseA).mockResolvedValueOnce(trackB); + const state = { manualCameraTrack: null as LocalVideoTrack | null }; + + // Attempt A starts (generation 0) and is stuck awaiting device acquisition. + const enablingA = enableCamera(state, deps); + await vi.waitFor(() => { + expect(createLocalVideoTrack).toHaveBeenCalledTimes(1); + }); + + // The user cycles the camera off and back on while A is still pending: + // disable bumps the generation, then a fresh enable (B) captures the new + // generation, resolves immediately, and goes fully live. + await disableCamera(state, deps); + await enableCamera(state, deps); + expect(state.manualCameraTrack).toBe(trackB); + expect(voiceStore.getState().localCamera).toBe(true); + + // Now A's stale createLocalVideoTrack call finally rejects (e.g. device + // contention). A's catch block must recognise it was superseded and + // leave B's live track and state alone. + rejectA(new DOMException("busy", "NotReadableError")); + await enablingA; + + expect(trackB.stop).not.toHaveBeenCalled(); + expect(state.manualCameraTrack).toBe(trackB); + expect(voiceStore.getState().localCamera).toBe(true); + }); }); // ── disableCamera ────────────────────────────────────────────────────────── @@ -669,6 +705,42 @@ describe("enableScreenshare", () => { expect(state.manualScreenTracks).toEqual([]); expect(voiceStore.getState().localScreenshare).toBe(false); }); + + it("does not tear down a newer share when a superseded enable's capture rejects (OC-0007)", async () => { + const rig = fakeRoom(); + const deps = fakeDeps(rig.room); + const videoB = fakeVideoTrack(); + let rejectA!: (err: unknown) => void; + const promiseA = new Promise((_, reject) => { + rejectA = reject; + }); + createLocalScreenTracks.mockReturnValueOnce(promiseA).mockResolvedValueOnce([videoB]); + const state = { manualScreenTracks: [] as LocalTrack[] }; + + // Attempt A starts (generation 0) and is stuck awaiting the OS picker. + const enablingA = enableScreenshare(state, deps); + await vi.waitFor(() => { + expect(createLocalScreenTracks).toHaveBeenCalledTimes(1); + }); + + // The user cycles the share off and back on while A is still pending: + // disable bumps the generation, then a fresh enable (B) captures the new + // generation, resolves immediately, and goes fully live. + await disableScreenshare(state, deps); + await enableScreenshare(state, deps); + expect(state.manualScreenTracks).toEqual([videoB]); + expect(voiceStore.getState().localScreenshare).toBe(true); + + // Now A's stale createLocalScreenTracks call finally rejects. A's catch + // block must recognise it was superseded and leave B's live tracks and + // state alone. + rejectA(new Error("capture failed")); + await enablingA; + + expect(videoB.stop).not.toHaveBeenCalled(); + expect(state.manualScreenTracks).toEqual([videoB]); + expect(voiceStore.getState().localScreenshare).toBe(true); + }); }); // ── disableScreenshare ───────────────────────────────────────────────────── diff --git a/Client/tauri-client/tests/unit/video-mode-controller.test.ts b/Client/tauri-client/tests/unit/video-mode-controller.test.ts index 4340f991..acebc8a6 100644 --- a/Client/tauri-client/tests/unit/video-mode-controller.test.ts +++ b/Client/tauri-client/tests/unit/video-mode-controller.test.ts @@ -683,6 +683,63 @@ describe("createVideoModeController", () => { expect(vg.clearStreams).not.toHaveBeenCalled(); }); + it("clears videoGrid streams on a voice channel switch (A -> B) even though currentChannelId never passes through null (OC-0012)", () => { + // VoiceCallbacks.onVoiceJoin moves currentChannelId straight from the + // old channel to the new one (joinVoiceChannel is optimistic), so the + // channelId === null branch never runs on a switch. Remote tiles left + // over from channel A must still be cleared when we land in channel B. + const usersA = new Map([ + [1, { userId: 1, camera: false, screenshare: false, username: "me" }], + ]); + mockVoiceStoreGetState.mockReturnValue( + makeVoiceState({ currentChannelId: 10, voiceUsers: new Map([[10, usersA]]) }), + ); + + const vg = makeVideoGrid(); + const ctrl = createVideoModeController({ + slots: makeSlots(), + videoGrid: vg, + getCurrentUserId: () => 1, + }); + + ctrl.checkVideoMode(); + expect(vg.clearStreams).not.toHaveBeenCalled(); + + // Switch straight to channel B — currentChannelId goes 10 -> 20, never + // through null. + const usersB = new Map([ + [1, { userId: 1, camera: false, screenshare: false, username: "me" }], + ]); + mockVoiceStoreGetState.mockReturnValue( + makeVoiceState({ currentChannelId: 20, voiceUsers: new Map([[20, usersB]]) }), + ); + ctrl.checkVideoMode(); + + expect(vg.clearStreams).toHaveBeenCalledTimes(1); + }); + + it("does not clear videoGrid streams across repeated checkVideoMode calls for the same channel (auto-reconnect)", () => { + const users = new Map([ + [1, { userId: 1, camera: false, screenshare: false, username: "me" }], + ]); + mockVoiceStoreGetState.mockReturnValue( + makeVoiceState({ currentChannelId: 10, voiceUsers: new Map([[10, users]]) }), + ); + + const vg = makeVideoGrid(); + const ctrl = createVideoModeController({ + slots: makeSlots(), + videoGrid: vg, + getCurrentUserId: () => 1, + }); + + ctrl.checkVideoMode(); + ctrl.checkVideoMode(); + ctrl.checkVideoMode(); + + expect(vg.clearStreams).not.toHaveBeenCalled(); + }); + it("closeVideoGrid clears the videoGrid's own focus state, not just the controller's", () => { const vg = makeVideoGrid(); const ctrl = createVideoModeController({ diff --git a/Client/tauri-client/tests/unit/ws-lifecycle.test.ts b/Client/tauri-client/tests/unit/ws-lifecycle.test.ts index 7f3d0132..c46580b4 100644 --- a/Client/tauri-client/tests/unit/ws-lifecycle.test.ts +++ b/Client/tauri-client/tests/unit/ws-lifecycle.test.ts @@ -595,6 +595,88 @@ describe("wsGeneration stale listener guard", () => { }); }); +describe("connect() catch guards against a superseded ws_connect rejection", () => { + let client: ReturnType; + + beforeEach(() => { + vi.useFakeTimers(); + mockInvoke.mockReset(); + mockListen.mockClear(); + eventHandlers.clear(); + client = createWsClient(); + }); + + afterEach(() => { + client.disconnect(); + vi.useRealTimers(); + }); + + // OC-0026: connect()'s two earlier await points (ensureTauriApis, + // setupEventListeners) both guard with `if (gen !== wsGeneration) return;` + // before acting on their resumption, but the `await tauriInvoke("ws_connect")` + // catch did not. The Rust proxy deliberately rejects a superseded handshake + // with "superseded by a newer connection" (ws_proxy.rs install_sender), so a + // stale attempt A's rejection — arriving after a newer attempt B already + // reached "connected" — must not tear B's live connection back down. + it("does not flip a live connection back to reconnecting when a stale attempt's ws_connect rejects with 'superseded'", async () => { + let rejectStaleWsConnect: ((err: Error) => void) | null = null; + let staleCallSeen = false; + + mockInvoke.mockImplementation(async (cmd: string) => { + if (cmd === "ws_connect" && !staleCallSeen) { + staleCallSeen = true; + return new Promise((_resolve, reject) => { + rejectStaleWsConnect = reject; + }); + } + return undefined; + }); + + // Attempt A: connect() suspends on the Rust handshake (ws_connect pends + // up to CONNECT_TIMEOUT in the real proxy). + client.connect({ host: "localhost:8443", token: "tA" }); + await vi.advanceTimersByTimeAsync(10); + expect(rejectStaleWsConnect).not.toBeNull(); + + // Attempt B supersedes A before A's handshake settles — e.g. a + // cert-accept retry, or logout immediately followed by re-login. + client.connect({ host: "localhost:8443", token: "tB" }); + await vi.advanceTimersByTimeAsync(10); + + // B completes its handshake and reaches "connected". + 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: "", + }, + }), + ); + expect(client.getState()).toBe("connected"); + + // A's stale ws_connect now rejects with the real error the Rust proxy + // sends for a superseded handshake. + rejectStaleWsConnect!(new Error("superseded by a newer connection")); + await vi.advanceTimersByTimeAsync(10); + + // The live connection must not be knocked back into "reconnecting" by + // the superseded attempt's rejection. + expect(client.getState()).toBe("connected"); + + // No reconnect timer should have been armed by the stale catch — + // advancing well past any backoff must not trigger a redial of the + // still-healthy connection. + mockInvoke.mockClear(); + await vi.advanceTimersByTimeAsync(60_000); + const reconnectCalls = mockInvoke.mock.calls.filter((c) => c[0] === "ws_connect"); + expect(reconnectCalls).toHaveLength(0); + }); +}); + describe("disconnect() cancelling an in-flight connect()", () => { let client: ReturnType; let originalMockListenImpl: (typeof mockListen)["getMockImplementation"] extends () => infer R diff --git a/Server/admin/logstream.go b/Server/admin/logstream.go index ba213814..7a392c65 100644 --- a/Server/admin/logstream.go +++ b/Server/admin/logstream.go @@ -5,6 +5,7 @@ import ( "crypto/rand" "encoding/hex" "encoding/json" + "errors" "fmt" "log/slog" "net/http" @@ -474,6 +475,24 @@ func handleLogStream(database *db.DB, ringBuf *RingBuffer) http.HandlerFunc { w.WriteHeader(http.StatusOK) flusher.Flush() + // This handler streams through the ordinary ResponseWriter (no + // Hijack), so it is otherwise subject to http.Server.WriteTimeout: + // net/http sets the connection's write deadline exactly once, when + // request headers are read, and nothing about writing more data + // later extends it. Without clearing it here, every write past that + // deadline (including the keepalive ticks below) silently times out + // — the caller discards write errors, per SSE convention, since a + // client that vanishes is detected via ctx.Done() instead — so the + // stream goes silently dead and the client eventually sees the + // connection close, then reconnects and replays the full backfill. + // SetWriteDeadline(zero) clears the deadline on HTTP/1 and cancels + // the per-stream deadline timer on HTTP/2; ErrNotSupported means the + // ResponseWriter doesn't sit over a real connection (e.g. in tests), + // which is fine to ignore. + if err := http.NewResponseController(w).SetWriteDeadline(time.Time{}); err != nil && !errors.Is(err, http.ErrNotSupported) { + slog.Warn("log stream: failed to clear write deadline; stream may be cut by WriteTimeout", "err", err) + } + // Snapshot the backfill and subscribe to new entries atomically: the // per-entry principalStillAuthorized() check below is a DB round-trip, // so the backfill loop is slow enough that a Snapshot()-then-Subscribe() @@ -492,7 +511,8 @@ func handleLogStream(database *db.DB, ringBuf *RingBuffer) http.HandlerFunc { } flusher.Flush() - // Keepalive ticker to avoid WriteTimeout (30s). + // Keepalive ticker against intermediary/proxy idle timeouts (the + // connection's own WriteTimeout was already neutralized above). keepalive := time.NewTicker(15 * time.Second) defer keepalive.Stop() diff --git a/Server/admin/logstream_test.go b/Server/admin/logstream_test.go index 9ebc1ea6..d1709967 100644 --- a/Server/admin/logstream_test.go +++ b/Server/admin/logstream_test.go @@ -6,6 +6,7 @@ import ( "net/http" "net/http/httptest" "testing" + "time" "github.com/owncord/server/auth" "github.com/owncord/server/db" @@ -161,3 +162,87 @@ func TestHandleLogStream_BackfillStopsAfterAPITokenRevocation(t *testing.T) { t.Fatalf("expected backfill to stop after first entry once the API token was revoked, wrote %d entries; body = %s", writer.writeCount, writer.buffer.String()) } } + +// TestHandleLogStream_SurvivesServerWriteTimeout pins the bug described in +// OC-0013: the handler writes SSE through the ordinary ResponseWriter (no +// Hijack), so on a real http.Server the connection's write deadline is set +// exactly once, when headers are read (net/http's conn.readRequest), from +// srv.WriteTimeout. Nothing in the handler extends that deadline, so once it +// elapses every further write on the connection silently times out (the +// handler discards write errors) and the client stops receiving anything. +// +// This must run against a real http.Server (httptest.NewUnstartedServer), +// not httptest.NewRequest/httptest.ResponseRecorder, because a +// ResponseRecorder has no underlying connection to enforce a write deadline +// on and so cannot reproduce the failure. +func TestHandleLogStream_SurvivesServerWriteTimeout(t *testing.T) { + database := newLogStreamTestDB(t) + logBuf := NewRingBuffer(64) + + userID, err := database.CreateUser(context.Background(), "owner", "hash", 1) + if err != nil { + t.Fatalf("CreateUser: %v", err) + } + token, err := auth.GenerateToken() + if err != nil { + t.Fatalf("GenerateToken: %v", err) + } + tokenHash := auth.HashToken(token) + if _, err := database.CreateSession(context.Background(), userID, tokenHash, "test", "127.0.0.1"); err != nil { + t.Fatalf("CreateSession: %v", err) + } + + ticket, err := logTickets.issue(tokenHash) + if err != nil { + t.Fatalf("issue ticket: %v", err) + } + + mux := http.NewServeMux() + mux.HandleFunc("/logs/stream", handleLogStream(database, logBuf)) + + srv := httptest.NewUnstartedServer(mux) + // A short stand-in for main.go's srv.WriteTimeout: 30 * time.Second, so + // the test doesn't have to wait 30s for the deadline to elapse. + srv.Config.WriteTimeout = 200 * time.Millisecond + srv.Start() + defer srv.Close() + + resp, err := http.Get(srv.URL + "/logs/stream?ticket=" + ticket) + if err != nil { + t.Fatalf("GET: %v", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Fatalf("status = %d, want 200", resp.StatusCode) + } + + // Let the connection's write deadline (set once, at request-header time) + // elapse before asking the handler to write anything else. + time.Sleep(400 * time.Millisecond) + + logBuf.Write(LogEntry{Timestamp: "2026-08-15T00:00:00Z", Level: "info", Message: "after-timeout", Source: "test"}) + + type readResult struct { + data []byte + err error + } + resultCh := make(chan readResult, 1) + body := resp.Body + go func() { + buf := make([]byte, 4096) + n, rerr := body.Read(buf) + resultCh <- readResult{data: buf[:n], err: rerr} + }() + + select { + case res := <-resultCh: + if res.err != nil { + t.Fatalf("expected the post-timeout log entry to be delivered, got a read error instead (connection was severed by WriteTimeout): %v", res.err) + } + if !bytes.Contains(res.data, []byte("after-timeout")) { + t.Fatalf("expected the post-timeout entry in the stream, got: %q", res.data) + } + case <-time.After(3 * time.Second): + t.Fatal("timed out waiting for the post-WriteTimeout log entry; stream appears severed by http.Server.WriteTimeout") + } +} diff --git a/Server/api/avatar_handler_test.go b/Server/api/avatar_handler_test.go index fb637830..c662bdb2 100644 --- a/Server/api/avatar_handler_test.go +++ b/Server/api/avatar_handler_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "os" "strings" "testing" @@ -170,6 +171,47 @@ func TestUploadAvatar_RequiresAuthAndAFile(t *testing.T) { } } +// TestUploadAvatar_StorageErrorDoesNotLeakPath pins the same contract +// upload_handler.go's safeStorageErrorMessage enforces for the plain-file +// upload route: a storage.Save failure (disk full, permission change, +// read-only mount) must never hand the client the server's absolute +// storage path. handleUploadAvatar currently forwards saveErr verbatim. +func TestUploadAvatar_StorageErrorDoesNotLeakPath(t *testing.T) { + database := newUploadTestDB(t) + dir := t.TempDir() + store, err := storage.New(dir, 10) + if err != nil { + t.Fatalf("storage.New: %v", err) + } + router := buildAvatarRouter(database, store) + token := uploadCreateToken(t, database, "avatar_leakuser", 4) + + // Remove the storage directory out from under the already-constructed + // Storage so Save's os.Create fails — this is what a disk-full, + // permission-change, or read-only-mount failure looks like from the + // handler's point of view: a storage-layer error surfaces at Save time. + if err := os.RemoveAll(dir); err != nil { + t.Fatalf("RemoveAll: %v", err) + } + + rr := doAvatarUpload(t, router, token, "me.png", makePNGBytes(t, 32, 32)) + if rr.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400; body: %s", rr.Code, rr.Body.String()) + } + + var resp map[string]any + if err := json.NewDecoder(rr.Body).Decode(&resp); err != nil { + t.Fatalf("decode: %v", err) + } + message, _ := resp["message"].(string) + if strings.Contains(message, dir) { + t.Fatalf("response message leaks the absolute storage path: %q", message) + } + if strings.ContainsAny(message, `/\`) { + t.Fatalf("response message looks like it contains a filesystem path: %q", message) + } +} + func TestUploadAvatar_NotMountedWithoutStorage(t *testing.T) { database := newUploadTestDB(t) r := chi.NewRouter() diff --git a/Server/api/profile_handler.go b/Server/api/profile_handler.go index ede247d0..9613d347 100644 --- a/Server/api/profile_handler.go +++ b/Server/api/profile_handler.go @@ -560,7 +560,7 @@ func handleUploadAvatar( if saveErr != nil { slog.Warn("avatar upload rejected by storage", "error", saveErr) writeJSON(w, http.StatusBadRequest, errorResponse{ - Error: "BAD_REQUEST", Message: fmt.Sprintf("upload rejected: %s", saveErr), + Error: "BAD_REQUEST", Message: safeStorageErrorMessage(saveErr), }) return } diff --git a/Server/api/router.go b/Server/api/router.go index a9b86edc..8299aaef 100644 --- a/Server/api/router.go +++ b/Server/api/router.go @@ -166,10 +166,20 @@ func NewRouter(cfg *config.Config, database *db.DB, ver string, logBuf *admin.Ri // download happens in the background inside Start). if cfg.Voice.LiveKitBinaryPath != "" || cfg.Voice.AutoDownloadLiveKit { proc := ws.NewLiveKitProcess(&cfg.Voice, &cfg.TLS, cfg.Server.DataDir) + // Register the process with the hub BEFORE calling Start(), and + // keep it registered even if Start() fails (OC-0019). The only + // consumer of h.lkProcess is the voice_join guard + // (`h.lkProcess != nil && !h.lkProcess.IsRunning()`), which reads + // a nil process as "LiveKit is externally managed, don't check". + // That is the wrong reading here: OwnCord was told to manage + // LiveKit and failed to launch it, so joins must fail closed via + // IsRunning() == false, not be waved through with no SFU + // running. IsRunning() is false for a proc whose Start() never + // got as far as spawning cmd, and Hub.Stop's lkProcess.Stop() is + // safe to call on a never-started proc. + hub.SetLiveKitProcess(proc) if startErr := proc.Start(); startErr != nil { slog.Error("failed to start LiveKit process", "error", startErr) - } else { - hub.SetLiveKitProcess(proc) } } } diff --git a/Server/api/router_livekit_process_test.go b/Server/api/router_livekit_process_test.go new file mode 100644 index 00000000..2ab72729 --- /dev/null +++ b/Server/api/router_livekit_process_test.go @@ -0,0 +1,150 @@ +package api_test + +// router_livekit_process_test.go pins the production wiring for OC-0019: +// when OwnCord is configured to manage its own companion LiveKit process +// (voice.auto_download_livekit or voice.livekit_binary set) and +// LiveKitProcess.Start() fails synchronously (e.g. generateConfig rejects an +// operator-chosen credential containing a YAML-unsafe character), NewRouter +// must still register the process with the hub so the voice_join guard in +// ws/voice_join.go — `if h.lkProcess != nil && !h.lkProcess.IsRunning()` — +// fails CLOSED. Before the fix, router.go skipped hub.SetLiveKitProcess on +// the Start() error path, leaving h.lkProcess nil, which reads as "LiveKit +// is externally managed" and lets voice_join proceed with no SFU running at +// all: a voice_states row gets persisted, a LiveKit JWT gets minted, and +// voice_state fans out to every client for a room nothing is serving. +// +// This test drives the real api.NewRouter wiring end to end over a live +// WebSocket connection, so it fails if the router ever again drops the +// process on a failed Start(). + +import ( + "context" + "encoding/json" + "net/http/httptest" + "testing" + "time" + + "github.com/coder/websocket" + + "github.com/owncord/server/api" + "github.com/owncord/server/auth" + "github.com/owncord/server/config" + "github.com/owncord/server/db" +) + +// voiceJoinWSMsg builds a raw voice_join WebSocket frame for the given channel. +func voiceJoinWSMsg(channelID int64) []byte { + raw, _ := json.Marshal(map[string]any{ + "type": "voice_join", + "payload": map[string]any{"channel_id": channelID}, + }) + return raw +} + +func TestNewRouter_LiveKitProcessStartFailure_VoiceJoinFailsClosed(t *testing.T) { + database, err := db.Open(":memory:") + if err != nil { + t.Fatalf("db.Open: %v", err) + } + t.Cleanup(func() { _ = database.Close() }) + if err := db.Migrate(database); err != nil { + t.Fatalf("db.Migrate: %v", err) + } + + cfg := &config.Config{ + Server: config.ServerConfig{ + Name: "Test Server", + Port: 8443, + DataDir: t.TempDir(), + AllowedOrigins: []string{"*"}, + }, + Voice: config.VoiceConfig{ + // Non-default, non-empty credentials so NewLiveKitClient succeeds + // and hub.SetLiveKit runs (router.go:162) — voice looks + // "configured". The colon in the secret is the ordinary + // credential character that trips livekit_process.go's unsafeYAML + // check inside generateConfig, so proc.Start() fails + // synchronously, before any goroutine or network call. + LiveKitAPIKey: "test-livekit-key-oc0019", + LiveKitAPISecret: "prod:livekit:secret-at-least-32-chars-long", + LiveKitURL: "ws://localhost:7880", + AutoDownloadLiveKit: true, + }, + } + + handler, _, cleanup := api.NewRouter(cfg, database, "test", nil, nil) + t.Cleanup(cleanup) + + // role_id=1 -> Owner, so CONNECT_VOICE is granted and the test isolates + // the LiveKit-process guard rather than a permission check. + uid, err := database.CreateUser(context.Background(), "oc0019voiceuser", "hash", 1) + if err != nil { + t.Fatalf("CreateUser: %v", err) + } + token, err := auth.GenerateToken() + if err != nil { + t.Fatalf("GenerateToken: %v", err) + } + if _, err := database.CreateSession(context.Background(), uid, auth.HashToken(token), "test", "127.0.0.1"); err != nil { + t.Fatalf("CreateSession: %v", err) + } + + channelID, err := database.CreateChannel(context.Background(), "oc0019-voice", "voice", "", "", 0) + if err != nil { + t.Fatalf("CreateChannel: %v", err) + } + + srv := httptest.NewServer(handler) + t.Cleanup(srv.Close) + + conn := dialAndAuthWS(t, srv, token) + + writeCtx, writeCancel := context.WithTimeout(context.Background(), 5*time.Second) + defer writeCancel() + if err := conn.Write(writeCtx, websocket.MessageText, voiceJoinWSMsg(channelID)); err != nil { + t.Fatalf("write voice_join: %v", err) + } + + // Read frames until we see a response to the join attempt (skipping the + // "ready" hydration frame and any other unrelated broadcasts), or time out. + deadline := time.Now().Add(10 * time.Second) + var lastFrame map[string]any + for time.Now().Before(deadline) { + readCtx, readCancel := context.WithTimeout(context.Background(), 2*time.Second) + _, msg, readErr := conn.Read(readCtx) + readCancel() + if readErr != nil { + break + } + var frame map[string]any + if err := json.Unmarshal(msg, &frame); err != nil { + continue + } + switch frame["type"] { + case "ready", "voice_config": + continue + case "error", "voice_token": + lastFrame = frame + } + if lastFrame != nil { + break + } + } + + if lastFrame == nil { + t.Fatal("no error or voice_token response observed for voice_join before the deadline") + } + + if lastFrame["type"] != "error" { + t.Fatalf("voice_join with a LiveKit process that failed to start must be rejected, got type=%v frame=%v — "+ + "router.go must register the LiveKitProcess with the hub even when proc.Start() fails, "+ + "so ws/voice_join.go's `h.lkProcess != nil && !h.lkProcess.IsRunning()` guard can fail closed "+ + "instead of reading a dropped process as \"externally managed, don't check\"", + lastFrame["type"], lastFrame) + } + + payload, _ := lastFrame["payload"].(map[string]any) + if payload["code"] != "VOICE_ERROR" { + t.Fatalf("expected error code VOICE_ERROR, got %v (frame=%v)", payload["code"], lastFrame) + } +} diff --git a/Server/db/dbgen/querier.go b/Server/db/dbgen/querier.go index 969a5ad7..50b709f3 100644 --- a/Server/db/dbgen/querier.go +++ b/Server/db/dbgen/querier.go @@ -13,14 +13,20 @@ import ( type Querier interface { AddReaction(ctx context.Context, arg AddReactionParams) error AdminUpdateChannel(ctx context.Context, arg AdminUpdateChannelParams) error - ApplyVoiceServerDeafen(ctx context.Context, userID int64) error - ApplyVoiceServerMute(ctx context.Context, userID int64) error + ApplyVoiceServerDeafen(ctx context.Context, arg ApplyVoiceServerDeafenParams) (sql.Result, error) + // Scoped to channel_id as well as user_id: the moderator's authorization is + // checked against a channel snapshot several round trips before this write + // lands, so an unscoped `WHERE user_id = ?` would follow the target onto + // whatever channel their row points at by then -- including a DM call the + // moderator was never authorized against (OC-0005). :execresult so the + // caller can tell a real no-op (target moved) from a normal apply. + ApplyVoiceServerMute(ctx context.Context, arg ApplyVoiceServerMuteParams) (sql.Result, error) BanUser(ctx context.Context, arg BanUserParams) error BlockUser(ctx context.Context, arg BlockUserParams) error CleanupExpiredLockouts(ctx context.Context, expiresAt string) error ClearAllVoiceStates(ctx context.Context) error - ClearVoiceServerDeafen(ctx context.Context, userID int64) error - ClearVoiceServerMute(ctx context.Context, userID int64) error + ClearVoiceServerDeafen(ctx context.Context, arg ClearVoiceServerDeafenParams) (sql.Result, error) + ClearVoiceServerMute(ctx context.Context, arg ClearVoiceServerMuteParams) (sql.Result, error) ClearVoiceState(ctx context.Context, userID int64) error CloseDM(ctx context.Context, arg CloseDMParams) error CountActiveCameras(ctx context.Context, channelID int64) (int64, error) @@ -60,8 +66,13 @@ type Querier interface { DeleteSessionByToken(ctx context.Context, token string) error DisablePlugin(ctx context.Context, id int64) error EditMessageContent(ctx context.Context, arg EditMessageContentParams) (Message, error) + // Camera and screenshare share one voice_max_video budget: a channel capped + // at N simultaneous video streams must not let a camera publish ignore + // screenshare occupants (or vice versa), so both gates count the same + // `camera = 1 OR screenshare = 1` slot usage (OC-0023). EnableCameraIfUnderLimit(ctx context.Context, arg EnableCameraIfUnderLimitParams) (sql.Result, error) EnablePlugin(ctx context.Context, id int64) error + EnableScreenshareIfUnderLimit(ctx context.Context, arg EnableScreenshareIfUnderLimitParams) (sql.Result, error) EvictOldestSessions(ctx context.Context, arg EvictOldestSessionsParams) error ForceLogoutUser(ctx context.Context, userID int64) error // Auth-hot lookup: returns the token only if it is neither revoked nor expired, diff --git a/Server/db/dbgen/voice.sql.go b/Server/db/dbgen/voice.sql.go index bd334081..a8dfb8f9 100644 --- a/Server/db/dbgen/voice.sql.go +++ b/Server/db/dbgen/voice.sql.go @@ -10,22 +10,37 @@ import ( "database/sql" ) -const applyVoiceServerDeafen = `-- name: ApplyVoiceServerDeafen :exec -UPDATE voice_states SET server_deafened = 1, deafened = 1 WHERE user_id = ? +const applyVoiceServerDeafen = `-- name: ApplyVoiceServerDeafen :execresult +UPDATE voice_states SET server_deafened = 1, deafened = 1 WHERE user_id = ? AND channel_id = ? ` -func (q *Queries) ApplyVoiceServerDeafen(ctx context.Context, userID int64) error { - _, err := q.db.ExecContext(ctx, applyVoiceServerDeafen, userID) - return err +type ApplyVoiceServerDeafenParams struct { + UserID int64 `json:"userId"` + ChannelID int64 `json:"channelId"` } -const applyVoiceServerMute = `-- name: ApplyVoiceServerMute :exec -UPDATE voice_states SET server_muted = 1, muted = 1 WHERE user_id = ? +func (q *Queries) ApplyVoiceServerDeafen(ctx context.Context, arg ApplyVoiceServerDeafenParams) (sql.Result, error) { + return q.db.ExecContext(ctx, applyVoiceServerDeafen, arg.UserID, arg.ChannelID) +} + +const applyVoiceServerMute = `-- name: ApplyVoiceServerMute :execresult + +UPDATE voice_states SET server_muted = 1, muted = 1 WHERE user_id = ? AND channel_id = ? ` -func (q *Queries) ApplyVoiceServerMute(ctx context.Context, userID int64) error { - _, err := q.db.ExecContext(ctx, applyVoiceServerMute, userID) - return err +type ApplyVoiceServerMuteParams struct { + UserID int64 `json:"userId"` + ChannelID int64 `json:"channelId"` +} + +// Scoped to channel_id as well as user_id: the moderator's authorization is +// checked against a channel snapshot several round trips before this write +// lands, so an unscoped `WHERE user_id = ?` would follow the target onto +// whatever channel their row points at by then -- including a DM call the +// moderator was never authorized against (OC-0005). :execresult so the +// caller can tell a real no-op (target moved) from a normal apply. +func (q *Queries) ApplyVoiceServerMute(ctx context.Context, arg ApplyVoiceServerMuteParams) (sql.Result, error) { + return q.db.ExecContext(ctx, applyVoiceServerMute, arg.UserID, arg.ChannelID) } const clearAllVoiceStates = `-- name: ClearAllVoiceStates :exec @@ -37,22 +52,30 @@ func (q *Queries) ClearAllVoiceStates(ctx context.Context) error { return err } -const clearVoiceServerDeafen = `-- name: ClearVoiceServerDeafen :exec -UPDATE voice_states SET server_deafened = 0 WHERE user_id = ? +const clearVoiceServerDeafen = `-- name: ClearVoiceServerDeafen :execresult +UPDATE voice_states SET server_deafened = 0 WHERE user_id = ? AND channel_id = ? ` -func (q *Queries) ClearVoiceServerDeafen(ctx context.Context, userID int64) error { - _, err := q.db.ExecContext(ctx, clearVoiceServerDeafen, userID) - return err +type ClearVoiceServerDeafenParams struct { + UserID int64 `json:"userId"` + ChannelID int64 `json:"channelId"` } -const clearVoiceServerMute = `-- name: ClearVoiceServerMute :exec -UPDATE voice_states SET server_muted = 0 WHERE user_id = ? +func (q *Queries) ClearVoiceServerDeafen(ctx context.Context, arg ClearVoiceServerDeafenParams) (sql.Result, error) { + return q.db.ExecContext(ctx, clearVoiceServerDeafen, arg.UserID, arg.ChannelID) +} + +const clearVoiceServerMute = `-- name: ClearVoiceServerMute :execresult +UPDATE voice_states SET server_muted = 0 WHERE user_id = ? AND channel_id = ? ` -func (q *Queries) ClearVoiceServerMute(ctx context.Context, userID int64) error { - _, err := q.db.ExecContext(ctx, clearVoiceServerMute, userID) - return err +type ClearVoiceServerMuteParams struct { + UserID int64 `json:"userId"` + ChannelID int64 `json:"channelId"` +} + +func (q *Queries) ClearVoiceServerMute(ctx context.Context, arg ClearVoiceServerMuteParams) (sql.Result, error) { + return q.db.ExecContext(ctx, clearVoiceServerMute, arg.UserID, arg.ChannelID) } const clearVoiceState = `-- name: ClearVoiceState :exec @@ -76,9 +99,10 @@ func (q *Queries) CountActiveCameras(ctx context.Context, channelID int64) (int6 } const enableCameraIfUnderLimit = `-- name: EnableCameraIfUnderLimit :execresult + UPDATE voice_states SET camera = 1 WHERE voice_states.user_id = ? AND voice_states.channel_id = ? - AND (SELECT COUNT(*) FROM voice_states AS vs2 WHERE vs2.channel_id = ? AND vs2.camera = 1) < ? + AND (SELECT COUNT(*) FROM voice_states AS vs2 WHERE vs2.channel_id = ? AND (vs2.camera = 1 OR vs2.screenshare = 1)) < ? ` type EnableCameraIfUnderLimitParams struct { @@ -88,6 +112,10 @@ type EnableCameraIfUnderLimitParams struct { ChannelID_3 int64 `json:"channelId3"` } +// Camera and screenshare share one voice_max_video budget: a channel capped +// at N simultaneous video streams must not let a camera publish ignore +// screenshare occupants (or vice versa), so both gates count the same +// `camera = 1 OR screenshare = 1` slot usage (OC-0023). func (q *Queries) EnableCameraIfUnderLimit(ctx context.Context, arg EnableCameraIfUnderLimitParams) (sql.Result, error) { return q.db.ExecContext(ctx, enableCameraIfUnderLimit, arg.UserID, @@ -97,6 +125,28 @@ func (q *Queries) EnableCameraIfUnderLimit(ctx context.Context, arg EnableCamera ) } +const enableScreenshareIfUnderLimit = `-- name: EnableScreenshareIfUnderLimit :execresult +UPDATE voice_states SET screenshare = 1 +WHERE voice_states.user_id = ? AND voice_states.channel_id = ? + AND (SELECT COUNT(*) FROM voice_states AS vs2 WHERE vs2.channel_id = ? AND (vs2.camera = 1 OR vs2.screenshare = 1)) < ? +` + +type EnableScreenshareIfUnderLimitParams struct { + UserID int64 `json:"userId"` + ChannelID int64 `json:"channelId"` + ChannelID_2 int64 `json:"channelId2"` + ChannelID_3 int64 `json:"channelId3"` +} + +func (q *Queries) EnableScreenshareIfUnderLimit(ctx context.Context, arg EnableScreenshareIfUnderLimitParams) (sql.Result, error) { + return q.db.ExecContext(ctx, enableScreenshareIfUnderLimit, + arg.UserID, + arg.ChannelID, + arg.ChannelID_2, + arg.ChannelID_3, + ) +} + const getAllVoiceStates = `-- name: GetAllVoiceStates :many SELECT vs.user_id, vs.channel_id, u.username, vs.muted, vs.deafened, vs.speaking, diff --git a/Server/db/queries/sqlite/voice.sql b/Server/db/queries/sqlite/voice.sql index 06b8ec26..3687da6d 100644 --- a/Server/db/queries/sqlite/voice.sql +++ b/Server/db/queries/sqlite/voice.sql @@ -74,22 +74,39 @@ UPDATE voice_states SET camera = ? WHERE user_id = ?; -- name: UpdateVoiceScreenshare :exec UPDATE voice_states SET screenshare = ? WHERE user_id = ?; --- name: ApplyVoiceServerMute :exec -UPDATE voice_states SET server_muted = 1, muted = 1 WHERE user_id = ?; +-- Scoped to channel_id as well as user_id: the moderator's authorization is +-- checked against a channel snapshot several round trips before this write +-- lands, so an unscoped `WHERE user_id = ?` would follow the target onto +-- whatever channel their row points at by then -- including a DM call the +-- moderator was never authorized against (OC-0005). :execresult so the +-- caller can tell a real no-op (target moved) from a normal apply. --- name: ClearVoiceServerMute :exec -UPDATE voice_states SET server_muted = 0 WHERE user_id = ?; +-- name: ApplyVoiceServerMute :execresult +UPDATE voice_states SET server_muted = 1, muted = 1 WHERE user_id = ? AND channel_id = ?; --- name: ApplyVoiceServerDeafen :exec -UPDATE voice_states SET server_deafened = 1, deafened = 1 WHERE user_id = ?; +-- name: ClearVoiceServerMute :execresult +UPDATE voice_states SET server_muted = 0 WHERE user_id = ? AND channel_id = ?; --- name: ClearVoiceServerDeafen :exec -UPDATE voice_states SET server_deafened = 0 WHERE user_id = ?; +-- name: ApplyVoiceServerDeafen :execresult +UPDATE voice_states SET server_deafened = 1, deafened = 1 WHERE user_id = ? AND channel_id = ?; + +-- name: ClearVoiceServerDeafen :execresult +UPDATE voice_states SET server_deafened = 0 WHERE user_id = ? AND channel_id = ?; + +-- Camera and screenshare share one voice_max_video budget: a channel capped +-- at N simultaneous video streams must not let a camera publish ignore +-- screenshare occupants (or vice versa), so both gates count the same +-- `camera = 1 OR screenshare = 1` slot usage (OC-0023). -- name: EnableCameraIfUnderLimit :execresult UPDATE voice_states SET camera = 1 WHERE voice_states.user_id = ? AND voice_states.channel_id = ? - AND (SELECT COUNT(*) FROM voice_states AS vs2 WHERE vs2.channel_id = ? AND vs2.camera = 1) < ?; + AND (SELECT COUNT(*) FROM voice_states AS vs2 WHERE vs2.channel_id = ? AND (vs2.camera = 1 OR vs2.screenshare = 1)) < ?; + +-- name: EnableScreenshareIfUnderLimit :execresult +UPDATE voice_states SET screenshare = 1 +WHERE voice_states.user_id = ? AND voice_states.channel_id = ? + AND (SELECT COUNT(*) FROM voice_states AS vs2 WHERE vs2.channel_id = ? AND (vs2.camera = 1 OR vs2.screenshare = 1)) < ?; -- name: ClearVoiceState :exec DELETE FROM voice_states WHERE user_id = ?; diff --git a/Server/db/voice_queries.go b/Server/db/voice_queries.go index f695b5fa..2684f5e6 100644 --- a/Server/db/voice_queries.go +++ b/Server/db/voice_queries.go @@ -188,36 +188,46 @@ func (d *DB) UpdateVoiceDeafen(ctx context.Context, userID int64, deafened bool) return nil } -// SetVoiceServerMute applies or clears the moderator-imposed mute. Applying it -// also sets muted so the client state matches immediately; clearing it leaves -// muted alone, so a user who was muted before the moderator acted stays muted -// until they unmute themselves. -func (d *DB) SetVoiceServerMute(ctx context.Context, userID int64, serverMuted bool) error { - var err error +// SetVoiceServerMute applies or clears the moderator-imposed mute, scoped to +// channelID -- the channel the caller's authorization check passed for. +// Applying it also sets muted so the client state matches immediately; +// clearing it leaves muted alone, so a user who was muted before the +// moderator acted stays muted until they unmute themselves. +// +// Reports matched=false when the target's voice_states row is no longer in +// channelID (OC-0005): a channel switch racing the moderator's DB round +// trips must not let this write land on whatever channel the target moved +// to, including a DM call nobody was authorized against. The row is left +// untouched in that case, same as if the write had never happened. +func (d *DB) SetVoiceServerMute(ctx context.Context, userID, channelID int64, serverMuted bool) (matched bool, err error) { + var res sql.Result if serverMuted { - err = d.q.ApplyVoiceServerMute(ctx, userID) + res, err = d.q.ApplyVoiceServerMute(ctx, dbgen.ApplyVoiceServerMuteParams{UserID: userID, ChannelID: channelID}) } else { - err = d.q.ClearVoiceServerMute(ctx, userID) + res, err = d.q.ClearVoiceServerMute(ctx, dbgen.ClearVoiceServerMuteParams{UserID: userID, ChannelID: channelID}) } if err != nil { - return fmt.Errorf("SetVoiceServerMute: %w", err) + return false, fmt.Errorf("SetVoiceServerMute: %w", err) } - return nil + n, _ := res.RowsAffected() + return n > 0, nil } -// SetVoiceServerDeafen applies or clears the moderator-imposed deafen. -// Mirrors SetVoiceServerMute, including the asymmetric handling of deafened. -func (d *DB) SetVoiceServerDeafen(ctx context.Context, userID int64, serverDeafened bool) error { - var err error +// SetVoiceServerDeafen applies or clears the moderator-imposed deafen, scoped +// to channelID. Mirrors SetVoiceServerMute, including the asymmetric handling +// of deafened and the channel-scoped matched result. +func (d *DB) SetVoiceServerDeafen(ctx context.Context, userID, channelID int64, serverDeafened bool) (matched bool, err error) { + var res sql.Result if serverDeafened { - err = d.q.ApplyVoiceServerDeafen(ctx, userID) + res, err = d.q.ApplyVoiceServerDeafen(ctx, dbgen.ApplyVoiceServerDeafenParams{UserID: userID, ChannelID: channelID}) } else { - err = d.q.ClearVoiceServerDeafen(ctx, userID) + res, err = d.q.ClearVoiceServerDeafen(ctx, dbgen.ClearVoiceServerDeafenParams{UserID: userID, ChannelID: channelID}) } if err != nil { - return fmt.Errorf("SetVoiceServerDeafen: %w", err) + return false, fmt.Errorf("SetVoiceServerDeafen: %w", err) } - return nil + n, _ := res.RowsAffected() + return n > 0, nil } // ClearVoiceState removes a user's voice state on disconnect. @@ -291,6 +301,28 @@ func (d *DB) UpdateVoiceScreenshare(ctx context.Context, userID int64, screensha return nil } +// EnableScreenshareIfUnderLimit atomically enables a user's screenshare only +// if the channel has not yet reached maxVideo active video streams — camera +// and screenshare draw from the same voice_max_video budget (OC-0023). +// Returns true if the screenshare was enabled, false if the limit was +// already reached. +func (d *DB) EnableScreenshareIfUnderLimit(ctx context.Context, userID, channelID int64, maxVideo int) (bool, error) { + res, err := d.q.EnableScreenshareIfUnderLimit(ctx, dbgen.EnableScreenshareIfUnderLimitParams{ + UserID: userID, + ChannelID: channelID, + ChannelID_2: channelID, + ChannelID_3: int64(maxVideo), + }) + if err != nil { + return false, fmt.Errorf("EnableScreenshareIfUnderLimit: %w", err) + } + rows, err := res.RowsAffected() + if err != nil { + return false, fmt.Errorf("EnableScreenshareIfUnderLimit RowsAffected: %w", err) + } + return rows > 0, nil +} + // CountChannelVoiceUsers returns the number of users currently in the given // voice channel. func (d *DB) CountChannelVoiceUsers(ctx context.Context, channelID int64) (int, error) { diff --git a/Server/db/voice_queries_test.go b/Server/db/voice_queries_test.go index 7cfa0963..3163ded1 100644 --- a/Server/db/voice_queries_test.go +++ b/Server/db/voice_queries_test.go @@ -768,3 +768,116 @@ func TestVoice_LeaveVoiceChannelIfMatch_DoesNotDeleteSameChannelRejoin(t *testin t.Fatalf("replacement join token = %q, want %q", current.JoinedAt, second.JoinedAt) } } + +// ─── SetVoiceServerMute / SetVoiceServerDeafen scoping (OC-0005) ──────────── +// +// ApplyVoiceServerMute/ClearVoiceServerMute and their deafen equivalents +// match on `WHERE user_id = ?` alone. A moderator's mute/deafen command is +// authorized against a channel snapshot (voiceModTarget + requireTargetInChannel +// in ws/voice_moderation.go), but the DB write that follows several round +// trips later is not scoped to that channel: if the target's voice_states row +// has since moved to a different channel — including a DM call the moderator +// was never authorized against — the unscoped write still lands on it. + +func TestVoice_SetVoiceServerMute_ScopedToChannel(t *testing.T) { + database := newVoiceTestDB(t) + ctx := context.Background() + userID := seedVoiceUser(t, database, "scope-mute-user") + chanA := seedVoiceChannel(t, database, "vc-scope-mute-a") + chanB := seedVoiceChannel(t, database, "vc-scope-mute-b") + + if err := database.JoinVoiceChannel(ctx, userID, chanA); err != nil { + t.Fatalf("JoinVoiceChannel A: %v", err) + } + // Simulate the race: the user's row moves to channel B — a channel nobody's + // mute command was authorized against — before the write below lands. + if err := database.JoinVoiceChannel(ctx, userID, chanB); err != nil { + t.Fatalf("JoinVoiceChannel B: %v", err) + } + + // A mute authorized against chanA (the channel a stale requireTargetInChannel + // snapshot showed) must not land on the row now in chanB. + matched, err := database.SetVoiceServerMute(ctx, userID, chanA, true) + if err != nil { + t.Fatalf("SetVoiceServerMute: %v", err) + } + if matched { + t.Error("SetVoiceServerMute matched=true against channel A after the user moved to channel B") + } + + state, err := database.GetVoiceState(ctx, userID) + if err != nil || state == nil { + t.Fatalf("GetVoiceState: %v", err) + } + if state.ChannelID != chanB { + t.Fatalf("test setup broken: want user in channel B, got %d", state.ChannelID) + } + if state.ServerMuted { + t.Error("ServerMuted = true, want false: an unscoped write must not follow the user to a channel nobody authorized the mute against") + } + + // The scoped write must still succeed when the channel does match. + matched, err = database.SetVoiceServerMute(ctx, userID, chanB, true) + if err != nil { + t.Fatalf("SetVoiceServerMute (matching channel): %v", err) + } + if !matched { + t.Error("SetVoiceServerMute matched=false for the channel the user is actually in") + } + state, err = database.GetVoiceState(ctx, userID) + if err != nil || state == nil { + t.Fatalf("GetVoiceState: %v", err) + } + if !state.ServerMuted { + t.Error("ServerMuted = false, want true: a scoped write against the correct channel must still apply") + } +} + +func TestVoice_SetVoiceServerDeafen_ScopedToChannel(t *testing.T) { + database := newVoiceTestDB(t) + ctx := context.Background() + userID := seedVoiceUser(t, database, "scope-deafen-user") + chanA := seedVoiceChannel(t, database, "vc-scope-deafen-a") + chanB := seedVoiceChannel(t, database, "vc-scope-deafen-b") + + if err := database.JoinVoiceChannel(ctx, userID, chanA); err != nil { + t.Fatalf("JoinVoiceChannel A: %v", err) + } + if err := database.JoinVoiceChannel(ctx, userID, chanB); err != nil { + t.Fatalf("JoinVoiceChannel B: %v", err) + } + + matched, err := database.SetVoiceServerDeafen(ctx, userID, chanA, true) + if err != nil { + t.Fatalf("SetVoiceServerDeafen: %v", err) + } + if matched { + t.Error("SetVoiceServerDeafen matched=true against channel A after the user moved to channel B") + } + + state, err := database.GetVoiceState(ctx, userID) + if err != nil || state == nil { + t.Fatalf("GetVoiceState: %v", err) + } + if state.ChannelID != chanB { + t.Fatalf("test setup broken: want user in channel B, got %d", state.ChannelID) + } + if state.ServerDeafened { + t.Error("ServerDeafened = true, want false: an unscoped write must not follow the user to a channel nobody authorized the deafen against") + } + + matched, err = database.SetVoiceServerDeafen(ctx, userID, chanB, true) + if err != nil { + t.Fatalf("SetVoiceServerDeafen (matching channel): %v", err) + } + if !matched { + t.Error("SetVoiceServerDeafen matched=false for the channel the user is actually in") + } + state, err = database.GetVoiceState(ctx, userID) + if err != nil || state == nil { + t.Fatalf("GetVoiceState: %v", err) + } + if !state.ServerDeafened { + t.Error("ServerDeafened = false, want true: a scoped write against the correct channel must still apply") + } +} diff --git a/Server/ws/export_test.go b/Server/ws/export_test.go index 969de0c9..3ceee54a 100644 --- a/Server/ws/export_test.go +++ b/Server/ws/export_test.go @@ -380,6 +380,15 @@ func BuildCallSignalForTest(msgType string, channelID, fromUserID int64, usernam // HandleWebhookParticipantLeftForTest exposes handleWebhookParticipantLeft for // external tests so they can simulate LiveKit webhook events without HTTP. func (h *Hub) HandleWebhookParticipantLeftForTest(userID int64, channelID int64, joinToken string) { + h.HandleWebhookParticipantLeftWithContextForTest(context.Background(), userID, channelID, joinToken) +} + +// HandleWebhookParticipantLeftWithContextForTest is +// HandleWebhookParticipantLeftForTest with a caller-supplied context, so +// external tests can simulate the webhook HTTP handler's request context +// (e.g. already-cancelled, as it would be after the webhook sender hangs up) +// instead of always running with context.Background(). +func (h *Hub) HandleWebhookParticipantLeftWithContextForTest(ctx context.Context, userID int64, channelID int64, joinToken string) { identity := fmt.Sprintf("user-%d:%s", userID, joinToken) roomName := fmt.Sprintf("channel-%d", channelID) event := &livekit.WebhookEvent{ @@ -391,7 +400,7 @@ func (h *Hub) HandleWebhookParticipantLeftForTest(userID int64, channelID int64, Name: roomName, }, } - h.handleWebhookParticipantLeft(context.Background(), event) + h.handleWebhookParticipantLeft(ctx, event) } // HandleWebhookParticipantJoinedForTest exposes handleWebhookParticipantJoined diff --git a/Server/ws/hub_broadcast.go b/Server/ws/hub_broadcast.go index 1a679eac..0c44b754 100644 --- a/Server/ws/hub_broadcast.go +++ b/Server/ws/hub_broadcast.go @@ -153,6 +153,29 @@ func (h *Hub) broadcastChannelScopedTo(channelID int64, msg []byte, recipients [ // duration of the call. Mirrors RefreshChannelVisibility, which resolves // visibility the same way. func (h *Hub) channelReadAudience(ctx context.Context, channelID int64) []int64 { + return h.channelReadAudienceImpl(ctx, channelID, false) +} + +// channelReadAudienceIgnoringArchived is channelReadAudience without the +// Archived short-circuit (OC-0022). CleanupVoiceForChannel's only two +// callers (admin/handlers_channels.go's archive and delete paths) always +// commit archived=1 to the channel before evicting its voice participants — +// deliberately, per admin/api_test.go's +// TestAdminAPI_DeleteChannel_ArchivesBeforeVoiceCleanup, so a concurrent +// voice_join sees the archived gate. That means channelReadAudience's own +// Archived check, evaluated from CleanupVoiceForChannel, always sees the +// channel already archived and always returns nobody: the voice_leave that +// should tell every bystander who could see the room a moment ago that the +// call ended never reaches them, only the evicted participants themselves +// (added back by CleanupVoiceForChannel's own loop). This resolves that same +// pre-archival READ audience for exactly that one broadcast, leaving every +// other channelReadAudience call site (and its archived-channel behavior) +// untouched. +func (h *Hub) channelReadAudienceIgnoringArchived(ctx context.Context, channelID int64) []int64 { + return h.channelReadAudienceImpl(ctx, channelID, true) +} + +func (h *Hub) channelReadAudienceImpl(ctx context.Context, channelID int64, ignoreArchived bool) []int64 { h.mu.RLock() userIDs := make([]int64, 0, len(h.clients)) for uid := range h.clients { @@ -180,8 +203,9 @@ func (h *Hub) channelReadAudience(ctx context.Context, channelID int64) []int64 // Without this, an admin edit to an archived channel (or a voice // teardown inside one) fans out straight to every connected user whose // base role holds READ_MESSAGES, none of whom have the channel in their - // ready payload or sidebar. - if ch != nil && ch.Archived { + // ready payload or sidebar. ignoreArchived opts a caller out of this + // specific check only — see channelReadAudienceIgnoringArchived. + if ch != nil && ch.Archived && !ignoreArchived { return []int64{} } if ch != nil && ch.Type == "dm" { diff --git a/Server/ws/hub_sweep.go b/Server/ws/hub_sweep.go index 016ca489..c64852bc 100644 --- a/Server/ws/hub_sweep.go +++ b/Server/ws/hub_sweep.go @@ -218,6 +218,28 @@ func (h *Hub) sweepStaleVoiceStates() { h.mu.RUnlock() for _, s := range stale { + if sweepStaleVoiceJoinRaceHook != nil { + sweepStaleVoiceJoinRaceHook(s.userID, s.channelID, s.joinedAt) + } + + // Re-check the live client immediately before deleting (OC-0017). + // voice_join.go commits the row before it calls c.setVoiceState + // (BUG-088's ordering), so a join can commit and get snapshotted as a + // ghost above — with c.setVoiceState landing after that snapshot but + // before this delete runs. If the client's current voice state now + // agrees with the row we are about to delete, the join has caught up + // and this is no longer stale: deleting it would leave the client + // "in voice" in memory with no DB row, the one ghost state nothing + // else heals. + h.mu.RLock() + liveClient, liveOK := h.clients[s.userID] + h.mu.RUnlock() + if liveOK { + if liveChID, liveJoinedAt := liveClient.getVoiceState(); liveChID == s.channelID && liveJoinedAt == s.joinedAt { + continue + } + } + // Channel-conditional delete: only removes the row if it still points // at the channel we snapshotted. If the user rejoined or moved between // the snapshot and now, the delete is a no-op and we skip the broadcast. @@ -249,6 +271,18 @@ func (h *Hub) sweepStaleVoiceStates() { } } +// sweepStaleVoiceJoinRaceHook, when non-nil, runs once per stale entry, +// immediately before sweepStaleVoiceStates acts on it. Test-only (always nil +// in production): it pins the BUG-088 follow-on window (OC-0017) where a +// voice_join's DB commit (voice_join.go's JoinVoiceChannelIfCapacity) and its +// c.setVoiceState call are not atomic — a join can commit its row, get +// snapshotted as a ghost by the h.clients scan above (the joiner's client +// still shows voiceChID 0 at that instant), and only call c.setVoiceState +// after the snapshot but before this loop deletes the row it just committed. +// Too narrow a window to land reliably by staggering real goroutines, so +// tests use this hook to reproduce it deterministically. +var sweepStaleVoiceJoinRaceHook func(userID, channelID int64, joinedAt string) + // hasChannelPermChecked is hasChannelPerm's error-aware counterpart: it // distinguishes a genuine permission denial (role missing, or the effective // permission bits don't include perm) from a DB read failure, by inlining the @@ -350,7 +384,15 @@ func (h *Hub) CleanupVoiceForChannel(channelID int64) { // state is already cleared, so broadcastVoiceEvent's participant union // cannot see them): the voice_leave is what drives their own E2EE // teardown, and voice membership never required READ_MESSAGES. - audience := h.channelReadAudience(ctx, channelID) + // + // Both callers of CleanupVoiceForChannel commit archived=1 to this + // channel before evicting (OC-0022) — deliberately, so a concurrent + // voice_join sees the archived gate — which means plain + // channelReadAudience always finds the channel already archived and + // always returns nobody here. Use the ignoring-archived resolver so the + // bystanders who could see this channel and its voice roster a moment + // ago still learn the call ended, not just the evicted participants. + audience := h.channelReadAudienceIgnoringArchived(ctx, channelID) seen := make(map[int64]struct{}, len(audience)) for _, uid := range audience { seen[uid] = struct{}{} diff --git a/Server/ws/hub_sweep_oc_findings_test.go b/Server/ws/hub_sweep_oc_findings_test.go new file mode 100644 index 00000000..cd18bdeb --- /dev/null +++ b/Server/ws/hub_sweep_oc_findings_test.go @@ -0,0 +1,153 @@ +package ws + +// Tests for the 2026-08 bughunt-fix wave 2 findings in hub_sweep.go: +// OC-0017 (a voice_join's DB-commit-to-setVoiceState window lets the stale +// sweep delete a row that caught up before the delete ran) and OC-0022 +// (CleanupVoiceForChannel's voice_leave fan-out reaches nobody but the +// evicted participants because both production callers archive the channel +// first, and channelReadAudience returns an empty audience for archived +// channels). + +import ( + "context" + "testing" + + "github.com/owncord/server/auth" +) + +// TestSweepStaleVoiceStates_JoinCatchesUpDuringDeleteWindow pins OC-0017. +// +// voice_join.go commits the voice_states row (JoinVoiceChannelIfCapacity) +// before it calls c.setVoiceState (BUG-088's ordering). If the stale-voice +// sweep's h.clients scan runs in that exact window, it snapshots the joiner +// as a ghost (c.getVoiceChID() is still 0, the row already says otherwise). +// The joiner's c.setVoiceState can then land — narrowing the client's state +// to agree with the very row about to be deleted — before the sweep's delete +// loop reaches that entry. sweepStaleVoiceJoinRaceHook reproduces that +// interleaving deterministically: it fires once per stale entry, at the +// point in the loop where the real race would land. +// +// Before the fix, the sweep deleted the row unconditionally once it was +// snapshotted stale, leaving the client "in voice" in memory with no DB row +// — the one ghost state nothing else heals. +func TestSweepStaleVoiceStates_JoinCatchesUpDuringDeleteWindow(t *testing.T) { + ctx := context.Background() + database := newHarvestVoiceDB(t) + uid := seedHarvestVoiceUser(t, database, "sweep-join-race") + chID := mustCreateVoiceChannel(t, database, "voice-join-race") + + // The row a real voice_join would have committed via + // JoinVoiceChannelIfCapacity, before its c.setVoiceState call runs. + if err := database.JoinVoiceChannel(ctx, uid, chID); err != nil { + t.Fatalf("JoinVoiceChannel: %v", err) + } + vs, err := database.GetVoiceState(ctx, uid) + if err != nil || vs == nil { + t.Fatalf("GetVoiceState: %v", err) + } + + h := NewHub(database, auth.NewRateLimiter(), nil) + c := NewTestClient(h, uid, make(chan []byte, 8)) + h.clients[uid] = c + // c.voiceChID is still 0 here — exactly like the joiner's client at the + // instant between JoinVoiceChannelIfCapacity's commit and setVoiceState. + + sweepStaleVoiceJoinRaceHook = func(userID, channelID int64, joinedAt string) { + if userID == uid { + // Simulates voice_join.go:253's c.setVoiceState landing inside + // the sweep's snapshot-to-delete window. + c.setVoiceState(channelID, joinedAt) + } + } + defer func() { sweepStaleVoiceJoinRaceHook = nil }() + + h.sweepStaleVoiceStates() + + gotChID, gotToken := c.getVoiceState() + if gotChID != chID || gotToken != vs.JoinedAt { + t.Fatalf("client voice state = (%d, %q) after the sweep raced a catching-up join, want (%d, %q) untouched", + gotChID, gotToken, chID, vs.JoinedAt) + } + row, err := database.GetVoiceState(ctx, uid) + if err != nil { + t.Fatalf("GetVoiceState after sweep: %v", err) + } + if row == nil { + t.Fatal("sweepStaleVoiceStates deleted a voice_states row whose client caught up before the delete ran — leaving the client in voice in memory with no DB row") + } +} + +// TestCleanupVoiceForChannel_NotifiesReadAudienceOfArchivedChannel pins +// OC-0022. +// +// Both production callers of CleanupVoiceForChannel (admin/handlers_channels.go's +// archive and delete paths, per admin/api_test.go's +// TestAdminAPI_DeleteChannel_ArchivesBeforeVoiceCleanup) commit archived=1 to +// the channel before calling it. channelReadAudience (OC-0073) treats any +// archived channel as invisible to every role and returns an empty audience, +// so CleanupVoiceForChannel's voice_leave broadcast reaches only the evicted +// participants themselves (appended back in by its own loop) — every other +// connected user who could see the channel and its voice roster a moment +// earlier hears nothing, and keeps the departed participants in their client +// voice store indefinitely. +func TestCleanupVoiceForChannel_NotifiesReadAudienceOfArchivedChannel(t *testing.T) { + ctx := context.Background() + database := newHarvestVoiceDB(t) + userA := seedHarvestVoiceUser(t, database, "cleanup-audience-a") + userB := seedHarvestVoiceUser(t, database, "cleanup-audience-b") + bystander := seedHarvestVoiceUser(t, database, "cleanup-audience-c") + chID := mustCreateVoiceChannel(t, database, "voice-cleanup-audience") + + if err := database.JoinVoiceChannel(ctx, userA, chID); err != nil { + t.Fatalf("JoinVoiceChannel(A): %v", err) + } + if err := database.JoinVoiceChannel(ctx, userB, chID); err != nil { + t.Fatalf("JoinVoiceChannel(B): %v", err) + } + + h := NewHub(database, auth.NewRateLimiter(), nil) + cA := NewTestClient(h, userA, make(chan []byte, 8)) + cB := NewTestClient(h, userB, make(chan []byte, 8)) + // bystander has READ_MESSAGES on the channel (harvestVoiceRoleID grants it + // directly, no override) but is not a voice participant. + cBystander := NewTestClient(h, bystander, make(chan []byte, 8)) + h.clients[userA] = cA + h.clients[userB] = cB + h.clients[bystander] = cBystander + cA.setVoiceState(chID, "tok-a") + cB.setVoiceState(chID, "tok-b") + + // Mirrors both production callers: archive before evicting. + if _, err := database.ExecContext(ctx, `UPDATE channels SET archived = 1 WHERE id = ?`, chID); err != nil { + t.Fatalf("archive channel: %v", err) + } + ch, err := database.GetChannel(ctx, chID) + if err != nil || ch == nil || !ch.Archived { + t.Fatalf("channel not archived before cleanup, GetChannel = %+v, err = %v", ch, err) + } + + h.CleanupVoiceForChannel(chID) + + recipients := map[int64]int{} +drain: + for { + select { + case bm := <-h.broadcast: + for _, uid := range bm.recipients { + recipients[uid]++ + } + default: + break drain + } + } + + if recipients[bystander] == 0 { + t.Errorf("bystander with READ_MESSAGES on the channel received no voice_leave broadcast after CleanupVoiceForChannel archived it first; recipients = %v", recipients) + } + if recipients[userA] == 0 { + t.Errorf("evicted participant A missing from its own voice_leave broadcast; recipients = %v", recipients) + } + if recipients[userB] == 0 { + t.Errorf("evicted participant B missing from its own voice_leave broadcast; recipients = %v", recipients) + } +} diff --git a/Server/ws/livekit.go b/Server/ws/livekit.go index fcd45738..61677d46 100644 --- a/Server/ws/livekit.go +++ b/Server/ws/livekit.go @@ -107,22 +107,32 @@ func (c *LiveKitClient) GenerateToken( CanSubscribe: &canSubscribe, } + // Use CanPublishSources to restrict which track types the user may + // publish. This supersedes CanPublish and prevents SFU-level bypass. + // + // SPEAK_VOICE (microphone), USE_VIDEO (camera) and SHARE_SCREEN (screen + // share) are independent permission bits — a channel override can deny + // SPEAK_VOICE while still granting USE_VIDEO/SHARE_SCREEN (OC-0016). The + // source list is therefore built from all three independently; CanPublish + // is only used as a hard deny when none of them grant anything, since + // LiveKit's GetCanPublishSource treats CanPublish=false as an override + // that blocks every source regardless of CanPublishSources. + var sources []string if canPublish { - // Use CanPublishSources to restrict which track types the user may - // publish. This supersedes CanPublish and prevents SFU-level bypass. - sources := []string{"microphone"} - if canVideo { - sources = append(sources, "camera") - } - if canScreenShare { - sources = append(sources, "screen_share", "screen_share_audio") - } + sources = append(sources, "microphone") + } + if canVideo { + sources = append(sources, "camera") + } + if canScreenShare { + sources = append(sources, "screen_share", "screen_share_audio") + } + if len(sources) > 0 { grant.CanPublishSources = sources - grant.CanPublishData = &canPublish } else { grant.CanPublish = &canPublish - grant.CanPublishData = &canPublish } + grant.CanPublishData = &canPublish at.SetVideoGrant(grant). SetIdentity(identity). diff --git a/Server/ws/livekit_test.go b/Server/ws/livekit_test.go index ad61e6d9..d1122a55 100644 --- a/Server/ws/livekit_test.go +++ b/Server/ws/livekit_test.go @@ -12,6 +12,8 @@ import ( "time" "github.com/go-chi/chi/v5" + "github.com/livekit/protocol/auth" + "github.com/livekit/protocol/livekit" "github.com/owncord/server/config" "github.com/owncord/server/permissions" "github.com/owncord/server/ws" @@ -180,6 +182,61 @@ func TestGenerateToken_DifferentPermissions(t *testing.T) { } } +// TestGenerateToken_VideoAndScreenShareGrantedWithoutSpeakVoice locks OC-0016: +// SPEAK_VOICE, USE_VIDEO and SHARE_SCREEN are independent permission bits +// (EffectiveChannelPerms resolves each per-bit), so a channel override can +// deny SPEAK_VOICE while still granting USE_VIDEO/SHARE_SCREEN — e.g. a +// presentation channel where only video is wanted. handleVoiceCameraV2 and +// handleVoiceScreenshareV2 gate only on USE_VIDEO/SHARE_SCREEN respectively, +// so the LiveKit token must carry a matching per-source grant instead of a +// blanket CanPublish=false that blocks every source, camera and screen share +// included, once SPEAK_VOICE is denied. +func TestGenerateToken_VideoAndScreenShareGrantedWithoutSpeakVoice(t *testing.T) { + t.Parallel() + + cfg := &config.VoiceConfig{ + LiveKitAPIKey: "test-key", + LiveKitAPISecret: "test-secret-that-is-long-enough-for-hmac", + LiveKitURL: "ws://localhost:7880", + } + + client, err := ws.NewLiveKitClient(cfg) + if err != nil { + t.Fatalf("NewLiveKitClient: %v", err) + } + + // canPublish=false (SPEAK_VOICE denied), canVideo=true, canScreenShare=true. + token, err := client.GenerateToken(1, "presenter", 10, "join-token-3", false, true, true, true) + if err != nil { + t.Fatalf("GenerateToken: %v", err) + } + + verifier, err := auth.ParseAPIToken(token) + if err != nil { + t.Fatalf("ParseAPIToken: %v", err) + } + _, grants, err := verifier.Verify(cfg.LiveKitAPISecret) + if err != nil { + t.Fatalf("Verify: %v", err) + } + if grants.Video == nil { + t.Fatal("expected a video grant in the token") + } + + if !grants.Video.GetCanPublishSource(livekit.TrackSource_CAMERA) { + t.Error("expected camera to be publishable when USE_VIDEO is granted, even though SPEAK_VOICE is denied") + } + if !grants.Video.GetCanPublishSource(livekit.TrackSource_SCREEN_SHARE) { + t.Error("expected screen_share to be publishable when SHARE_SCREEN is granted, even though SPEAK_VOICE is denied") + } + if !grants.Video.GetCanPublishSource(livekit.TrackSource_SCREEN_SHARE_AUDIO) { + t.Error("expected screen_share_audio to be publishable when SHARE_SCREEN is granted, even though SPEAK_VOICE is denied") + } + if grants.Video.GetCanPublishSource(livekit.TrackSource_MICROPHONE) { + t.Error("expected microphone NOT to be publishable when SPEAK_VOICE is denied") + } +} + // --------------------------------------------------------------------------- // livekit_process.go tests // --------------------------------------------------------------------------- @@ -615,6 +672,63 @@ func TestWebhook_ParticipantLeft_LeaverWithoutReadStillNotified(t *testing.T) { } } +// TestWebhook_ParticipantLeft_SurvivesCancelledRequestContext locks OC-0018: +// the webhook handler must not tie its teardown broadcast to the triggering +// HTTP request's context. Every sibling teardown path detaches before doing +// cleanup work (readPump's defer and unregisterFailedHandshake use +// context.WithoutCancel in serve_pumps.go/serve.go, rollbackVoiceJoin uses it +// in voice_join.go, the hub sweeps use context.Background in hub_sweep.go) — +// the webhook handler alone passed r.Context() straight through. If the +// webhook sender (LiveKit) hangs up mid-request, net/http cancels that +// context; channelReadAudience's GetChannel call then fails and +// channelReadAudience fails closed to []int64{} (hub_broadcast.go), silently +// dropping any observer who has READ_MESSAGES on the channel but is not +// currently in the room from the voice_leave audience. Unlike the DB row, +// nothing ever re-emits that missed broadcast, so the observer's UI shows the +// departed participant forever. +func TestWebhook_ParticipantLeft_SurvivesCancelledRequestContext(t *testing.T) { + t.Parallel() + hub, database := newVoiceHub(t) + + chanID := seedVoiceChan(t, database, "webhook-ctxcancel-ch") + + leaver := seedVoiceOwner(t, database, "webhook-ctxcancel-leaver") + observer := seedVoiceOwner(t, database, "webhook-ctxcancel-observer") + + 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) + leaverClient := ws.NewTestClient(hub, leaver.ID, leaverSend) + ws.SetClientVoiceStateForTest(leaverClient, chanID, vs.JoinedAt) + hub.RegisterNowForTest(leaverClient) + + // The observer has READ_MESSAGES on the channel (Owner role bypasses + // channel_overrides) but is not in the room — exactly the audience member + // channelReadAudience's role scan exists to reach, and the only one an + // empty-audience fail-close silently drops. + observerSend := make(chan []byte, 16) + observerClient := ws.NewTestClient(hub, observer.ID, observerSend) + hub.RegisterNowForTest(observerClient) + + // Simulate net/http cancelling the request context because the webhook + // sender (LiveKit) hung up before the handler finished — exactly what + // r.Context() looks like by the time a slow cleanup path reads it. + cancelledCtx, cancel := context.WithCancel(context.Background()) + cancel() + + hub.HandleWebhookParticipantLeftWithContextForTest(cancelledCtx, leaver.ID, chanID, vs.JoinedAt) + + if got := countVoiceLeaves(observerSend, 200*time.Millisecond); got == 0 { + t.Error("observer with READ_MESSAGES but outside the room received no voice_leave when the webhook's request context was already cancelled — the teardown broadcast must detach from the triggering request context") + } +} + // --------------------------------------------------------------------------- // livekit_process.go – generateConfig tests // --------------------------------------------------------------------------- diff --git a/Server/ws/livekit_webhook.go b/Server/ws/livekit_webhook.go index d7269508..1f097c48 100644 --- a/Server/ws/livekit_webhook.go +++ b/Server/ws/livekit_webhook.go @@ -99,6 +99,17 @@ func parseRoomChannelID(roomName string) (int64, error) { } func (h *Hub) handleWebhookParticipantJoined(ctx context.Context, event *livekit.WebhookEvent) { + // Detach from the triggering HTTP request before doing any cleanup work, + // mirroring every sibling teardown path (readPump's defer and + // unregisterFailedHandshake use context.WithoutCancel in serve.go / + // serve_pumps.go, rollbackVoiceJoin uses it in voice_join.go, the hub + // sweeps use context.Background in hub_sweep.go). Without this, a webhook + // sender (LiveKit) that hangs up mid-request cancels r.Context(), and the + // rogue-participant GetVoiceState/RemoveParticipant calls below would + // either wrongly skip (treating a cancelled read as a transient error, ok) + // or fail outright instead of completing the eviction. + ctx = context.WithoutCancel(ctx) + p := event.GetParticipant() room := event.GetRoom() if p == nil || room == nil { @@ -168,6 +179,21 @@ func (h *Hub) handleWebhookParticipantJoined(ctx context.Context, event *livekit } func (h *Hub) handleWebhookParticipantLeft(ctx context.Context, event *livekit.WebhookEvent) { + // Detach from the triggering HTTP request before doing any cleanup work + // (OC-0018), mirroring every sibling teardown path (readPump's defer and + // unregisterFailedHandshake use context.WithoutCancel in serve.go / + // serve_pumps.go, rollbackVoiceJoin uses it in voice_join.go, the hub + // sweeps use context.Background in hub_sweep.go). Without this, a webhook + // sender (LiveKit) that hangs up mid-request cancels r.Context(), which + // makes channelReadAudience's GetChannel call fail and fail closed to an + // empty audience (hub_broadcast.go) — silently dropping the voice_leave + // for anyone who has READ_MESSAGES on the channel but is not currently in + // the room. Unlike the DB row, no sweep ever re-emits that missed + // broadcast. The same cancellation would also make both + // LeaveVoiceChannelIfMatch branches below fail on their synchronous first + // attempt. + ctx = context.WithoutCancel(ctx) + p := event.GetParticipant() room := event.GetRoom() if p == nil || room == nil { diff --git a/Server/ws/oc_0023_screenshare_video_limit_test.go b/Server/ws/oc_0023_screenshare_video_limit_test.go new file mode 100644 index 00000000..db753a45 --- /dev/null +++ b/Server/ws/oc_0023_screenshare_video_limit_test.go @@ -0,0 +1,154 @@ +package ws + +// OC-0023: voice_max_video is enforced only for camera publishes. +// handleVoiceScreenshareV2 never checks the channel's VIDEO_LIMIT cap before +// writing UpdateVoiceScreenshare, and EnableCameraIfUnderLimit's slot-count +// subquery only ever counted `camera = 1` rows, so the two publish kinds +// neither share a budget nor respect each other's occupancy of it. + +import ( + "context" + "testing" + + "github.com/owncord/server/db" + "github.com/owncord/server/permissions" +) + +// oc0023VideoLimitRoleID is a dedicated, non-seeded role carrying every +// permission bit these tests exercise (connect, speak, camera, screenshare), +// so the test does not depend on what the migrations grant the defaults. +const oc0023VideoLimitRoleID = int64(210) + +func newOC0023VideoLimitDB(t *testing.T) *db.DB { + t.Helper() + database, err := db.Open(":memory:") + if err != nil { + t.Fatalf("db.Open: %v", err) + } + if err := db.Migrate(database); err != nil { + t.Fatalf("Migrate: %v", err) + } + t.Cleanup(func() { _ = database.Close() }) + if _, err := database.ExecContext(context.Background(), + `INSERT INTO roles (id, name, color, permissions, position, is_default) + VALUES (?, 'oc-0023-video', NULL, ?, 5, 0)`, + oc0023VideoLimitRoleID, + permissions.ReadMessages|permissions.ConnectVoice|permissions.SpeakVoice|permissions.UseVideo|permissions.ShareScreen, + ); err != nil { + t.Fatalf("seed oc-0023-video role: %v", err) + } + return database +} + +func seedOC0023VideoLimitUser(t *testing.T, database *db.DB, username string) int64 { + t.Helper() + uid, err := database.CreateUser(context.Background(), username, "hash", int(oc0023VideoLimitRoleID)) + if err != nil { + t.Fatalf("CreateUser %s: %v", username, err) + } + return uid +} + +// mustCreateVideoCappedChannel creates a voice channel and sets its +// voice_max_video cap via the same AdminUpdateChannel path an admin PATCH +// takes, so GetChannel(...).VoiceMaxVideo comes back real rather than faked. +func mustCreateVideoCappedChannel(t *testing.T, database *db.DB, name string, maxVideo int) int64 { + t.Helper() + chID, err := database.CreateChannel(context.Background(), name, "voice", "", "", 0) + if err != nil { + t.Fatalf("CreateChannel %s: %v", name, err) + } + if err := database.AdminUpdateChannel(context.Background(), chID, db.ChannelUpdate{ + Name: name, + VoiceMaxVideo: maxVideo, + }); err != nil { + t.Fatalf("AdminUpdateChannel %s: %v", name, err) + } + return chID +} + +// A channel capped at one simultaneous video stream already has a camera +// publisher occupying the slot. A second user's voice_screenshare(true) must +// be refused with VIDEO_LIMIT, exactly like a second camera enable would be. +// Today handleVoiceScreenshareV2 performs no cap check at all, so this +// currently enables the screenshare and returns no error. +func TestHandleVoiceScreenshareV2_RefusedWhenCameraSlotFull(t *testing.T) { + ctx := context.Background() + database := newOC0023VideoLimitDB(t) + chID := mustCreateVideoCappedChannel(t, database, "capped-room", 1) + + userA := seedOC0023VideoLimitUser(t, database, "cam-holder") + userB := seedOC0023VideoLimitUser(t, database, "share-hopeful") + if err := database.JoinVoiceChannel(ctx, userA, chID); err != nil { + t.Fatalf("JoinVoiceChannel A: %v", err) + } + if err := database.JoinVoiceChannel(ctx, userB, chID); err != nil { + t.Fatalf("JoinVoiceChannel B: %v", err) + } + + d := VoiceDeps{DB: database, Permissions: permissions.NewChecker(database)} + + camRes := handleVoiceCameraV2(ctx, VoiceCameraCmd{userID: userA, enabled: true}, ClientInfo{UserID: userA, VoiceChannelID: chID}, d) + if camRes.Error != nil { + t.Fatalf("user A camera enable under an empty cap should succeed, got error: %+v", camRes.Error) + } + + ssRes := handleVoiceScreenshareV2(ctx, VoiceScreenshareCmd{userID: userB, enabled: true}, ClientInfo{UserID: userB, VoiceChannelID: chID}, d) + if ssRes.Error == nil { + t.Fatal("voice_screenshare succeeded with the channel's single video slot already held by a camera publisher — VIDEO_LIMIT was never checked") + } + if ce, ok := ssRes.Error.(ClientError); !ok || ce.Code != ErrCodeVideoLimit { + t.Errorf("error = %+v, want ClientError{Code: %q}", ssRes.Error, ErrCodeVideoLimit) + } + + vs, err := database.GetVoiceState(ctx, userB) + if err != nil || vs == nil { + t.Fatalf("GetVoiceState B: %v", err) + } + if vs.Screenshare { + t.Error("user B's screenshare flag was set to true despite the VIDEO_LIMIT refusal") + } +} + +// Symmetrically: a channel capped at one slot already occupied by a +// screenshare must refuse a second user's camera enable. Today +// EnableCameraIfUnderLimit's slot-count subquery only counts `camera = 1` +// rows, so a screensharing user is invisible to it and the camera enable +// wrongly succeeds. +func TestHandleVoiceCameraV2_RefusedWhenScreenshareSlotFull(t *testing.T) { + ctx := context.Background() + database := newOC0023VideoLimitDB(t) + chID := mustCreateVideoCappedChannel(t, database, "capped-room-2", 1) + + userA := seedOC0023VideoLimitUser(t, database, "share-holder") + userB := seedOC0023VideoLimitUser(t, database, "cam-hopeful") + if err := database.JoinVoiceChannel(ctx, userA, chID); err != nil { + t.Fatalf("JoinVoiceChannel A: %v", err) + } + if err := database.JoinVoiceChannel(ctx, userB, chID); err != nil { + t.Fatalf("JoinVoiceChannel B: %v", err) + } + + d := VoiceDeps{DB: database, Permissions: permissions.NewChecker(database)} + + ssRes := handleVoiceScreenshareV2(ctx, VoiceScreenshareCmd{userID: userA, enabled: true}, ClientInfo{UserID: userA, VoiceChannelID: chID}, d) + if ssRes.Error != nil { + t.Fatalf("user A screenshare enable under an empty cap should succeed, got error: %+v", ssRes.Error) + } + + camRes := handleVoiceCameraV2(ctx, VoiceCameraCmd{userID: userB, enabled: true}, ClientInfo{UserID: userB, VoiceChannelID: chID}, d) + if camRes.Error == nil { + t.Fatal("voice_camera succeeded with the channel's single video slot already held by a screenshare publisher — the slot-count query ignores screenshare rows") + } + if ce, ok := camRes.Error.(ClientError); !ok || ce.Code != ErrCodeVideoLimit { + t.Errorf("error = %+v, want ClientError{Code: %q}", camRes.Error, ErrCodeVideoLimit) + } + + vs, err := database.GetVoiceState(ctx, userB) + if err != nil || vs == nil { + t.Fatalf("GetVoiceState B: %v", err) + } + if vs.Camera { + t.Error("user B's camera flag was set to true despite the VIDEO_LIMIT refusal") + } +} diff --git a/Server/ws/voice_controls.go b/Server/ws/voice_controls.go index 29be78fb..49fa74a6 100644 --- a/Server/ws/voice_controls.go +++ b/Server/ws/voice_controls.go @@ -103,32 +103,11 @@ func handleVoiceCameraV2(ctx context.Context, cmd Command, info ClientInfo, deps } // Enforce MaxVideo limit when enabling camera using an atomic check-and-update. + // Camera and screenshare draw from the same voice_max_video budget + // (OC-0023), so this gate is shared with handleVoiceScreenshareV2 below. if enabled { - ch, chErr := d.DB.GetChannel(ctx, voiceChID) - if chErr != nil { - // Fail closed: an unreadable channel row is not "no cap - // configured" — falling through to the unconditional enable - // bypasses the per-channel video limit. - slog.Error("handleVoiceCameraV2 GetChannel", "err", chErr, "channel_id", voiceChID) - return Result{Error: ClientError{Code: ErrCodeInternal, Message: "failed to check video limit"}} - } - if ch != nil && ch.VoiceMaxVideo > 0 { - ok, limitErr := d.DB.EnableCameraIfUnderLimit(ctx, userID, voiceChID, ch.VoiceMaxVideo) - if limitErr != nil { - slog.Error("handleVoiceCameraV2 EnableCameraIfUnderLimit", "err", limitErr, "channel_id", voiceChID) - return Result{Error: ClientError{Code: ErrCodeInternal, Message: "failed to check video limit"}} - } - if !ok { - return Result{Error: ClientError{ - Code: ErrCodeVideoLimit, - Message: fmt.Sprintf("maximum %d video streams reached", ch.VoiceMaxVideo), - }} - } - } else { - if err := d.DB.UpdateVoiceCamera(ctx, userID, true); err != nil { - slog.Error("ws handleVoiceCameraV2 UpdateVoiceCamera", "err", err, "user_id", userID) - return Result{Error: ClientError{Code: ErrCodeInternal, Message: "failed to update camera state"}} - } + if r := enableVideoSlot(ctx, d, userID, voiceChID, d.DB.EnableCameraIfUnderLimit, d.DB.UpdateVoiceCamera, "handleVoiceCameraV2", "camera"); r != nil { + return *r } } else { if err := d.DB.UpdateVoiceCamera(ctx, userID, false); err != nil { @@ -171,15 +150,71 @@ func handleVoiceScreenshareV2(ctx context.Context, cmd Command, info ClientInfo, } } - if err := d.DB.UpdateVoiceScreenshare(ctx, userID, enabled); err != nil { - slog.Error("ws handleVoiceScreenshareV2 UpdateVoiceScreenshare", "err", err, "user_id", userID) - return Result{Error: ClientError{Code: ErrCodeInternal, Message: "failed to update screenshare state"}} + // Enforce the same voice_max_video budget handleVoiceCameraV2 enforces — + // camera and screenshare are both "video streams" against one cap + // (OC-0023): a screenshare must not be able to occupy a slot the cap + // intended to deny it, and must not be invisible to the camera gate's + // count either (enableVideoSlot's atomic query counts both fields). + if enabled { + if r := enableVideoSlot(ctx, d, userID, voiceChID, d.DB.EnableScreenshareIfUnderLimit, d.DB.UpdateVoiceScreenshare, "handleVoiceScreenshareV2", "screenshare"); r != nil { + return *r + } + } else { + if err := d.DB.UpdateVoiceScreenshare(ctx, userID, false); err != nil { + slog.Error("ws handleVoiceScreenshareV2 UpdateVoiceScreenshare", "err", err, "user_id", userID) + return Result{Error: ClientError{Code: ErrCodeInternal, Message: "failed to update screenshare state"}} + } } slog.Debug("voice screenshare changed", "user_id", userID, "enabled", enabled, "channel_id", voiceChID) return voiceStateBroadcast(ctx, d, userID) } +// enableVideoSlot enforces the channel's shared voice_max_video budget +// before applying a camera or screenshare enable (OC-0023: camera and +// screenshare draw from the same per-channel slot count, so neither publish +// kind can bypass the cap by hiding from the other's count). tryReserve is +// the atomic check-and-update for the specific field being enabled — +// EnableCameraIfUnderLimit or EnableScreenshareIfUnderLimit, both of which +// count `camera = 1 OR screenshare = 1` rows against the cap. unconditionalSet +// applies the same field's plain update when the channel carries no cap. +func enableVideoSlot( + ctx context.Context, + d VoiceDeps, + userID, voiceChID int64, + tryReserve func(ctx context.Context, userID, channelID int64, maxVideo int) (bool, error), + unconditionalSet func(ctx context.Context, userID int64, enabled bool) error, + logPrefix, kind string, +) *Result { + ch, chErr := d.DB.GetChannel(ctx, voiceChID) + if chErr != nil { + // Fail closed: an unreadable channel row is not "no cap + // configured" — falling through to the unconditional enable + // bypasses the per-channel video limit. + slog.Error(logPrefix+" GetChannel", "err", chErr, "channel_id", voiceChID) + return &Result{Error: ClientError{Code: ErrCodeInternal, Message: "failed to check video limit"}} + } + if ch != nil && ch.VoiceMaxVideo > 0 { + ok, limitErr := tryReserve(ctx, userID, voiceChID, ch.VoiceMaxVideo) + if limitErr != nil { + slog.Error(logPrefix+" EnableIfUnderLimit", "err", limitErr, "channel_id", voiceChID) + return &Result{Error: ClientError{Code: ErrCodeInternal, Message: "failed to check video limit"}} + } + if !ok { + return &Result{Error: ClientError{ + Code: ErrCodeVideoLimit, + Message: fmt.Sprintf("maximum %d video streams reached", ch.VoiceMaxVideo), + }} + } + return nil + } + if err := unconditionalSet(ctx, userID, true); err != nil { + slog.Error("ws "+logPrefix+" "+kind+" unconditional enable", "err", err, "user_id", userID) + return &Result{Error: ClientError{Code: ErrCodeInternal, Message: "failed to update " + kind + " state"}} + } + return nil +} + // refuseIfServerSilenced refuses a self-unmute (deafen=false) or self-undeafen // (deafen=true) while the corresponding moderator-imposed flag is set. A read // error is not a denial: it is reported as INTERNAL so an operator sees it diff --git a/Server/ws/voice_handlers_test.go b/Server/ws/voice_handlers_test.go index c5075bbb..19450e97 100644 --- a/Server/ws/voice_handlers_test.go +++ b/Server/ws/voice_handlers_test.go @@ -1138,10 +1138,10 @@ func TestVoice_Join_SwitchChannel_PreservesServerMute(t *testing.T) { hub.HandleMessageForTest(c, voiceJoinMsg(chanA)) drainChanTimeout(send, 30*time.Millisecond) - if err := database.SetVoiceServerMute(context.Background(), user.ID, true); err != nil { + if _, err := database.SetVoiceServerMute(context.Background(), user.ID, chanA, true); err != nil { t.Fatalf("SetVoiceServerMute: %v", err) } - if err := database.SetVoiceServerDeafen(context.Background(), user.ID, true); err != nil { + if _, err := database.SetVoiceServerDeafen(context.Background(), user.ID, chanA, true); err != nil { t.Fatalf("SetVoiceServerDeafen: %v", err) } diff --git a/Server/ws/voice_join.go b/Server/ws/voice_join.go index f741f46f..d7ecb609 100644 --- a/Server/ws/voice_join.go +++ b/Server/ws/voice_join.go @@ -33,6 +33,16 @@ func validVoiceQuality(q string) bool { return ok } +// voiceJoinPostTokenRaceHook, when non-nil, runs immediately after +// GenerateToken succeeds and before the minted token is checked for +// supersession / handed to the client. Test-only (always nil in production): +// GenerateToken is a local JWT mint with no I/O, so the window it pins (a +// concurrent eviction landing between token generation and delivery, OC-0008) +// is too narrow to land reliably by staggering real goroutines. Mirrors +// cleanupVoiceRaceClearHook (hub_sweep.go), used the same way for the +// analogous CleanupVoiceForChannel race. +var voiceJoinPostTokenRaceHook func(*Client) + // handleVoiceJoin processes a voice_join message. // 1. Parses channel_id. // 2. Checks CONNECT_VOICE permission. @@ -247,12 +257,12 @@ func (h *Hub) handleVoiceJoin(ctx context.Context, c *Client, payload json.RawMe // fail the join, matching every other SetVoiceServerMute/Deafen call site. if wasServerMuted || wasServerDeafened { if wasServerMuted { - if err := h.db.SetVoiceServerMute(ctx, c.userID, true); err != nil { + if _, err := h.db.SetVoiceServerMute(ctx, c.userID, channelID, true); err != nil { slog.Error("ws handleVoiceJoin SetVoiceServerMute (restore)", "err", err, "user_id", c.userID) } } if wasServerDeafened { - if err := h.db.SetVoiceServerDeafen(ctx, c.userID, true); err != nil { + if _, err := h.db.SetVoiceServerDeafen(ctx, c.userID, channelID, true); err != nil { slog.Error("ws handleVoiceJoin SetVoiceServerDeafen (restore)", "err", err, "user_id", c.userID) } } @@ -322,6 +332,37 @@ func (h *Hub) handleVoiceJoin(ctx context.Context, c *Client, payload json.RawMe c.sendMsg(buildErrorMsg(ErrCodeInternal, "failed to generate voice token")) return } + if voiceJoinPostTokenRaceHook != nil { + voiceJoinPostTokenRaceHook(c) + } + + // OC-0008: a concurrent eviction (voice_mod_kick/move via + // DisconnectFromVoiceInChannel, the CONNECT_VOICE revocation sweep, or + // CleanupVoiceForChannel) can land anywhere between c.setVoiceState + // (BUG-088, above) and here — all of them delete the voice_states row + // and clear the client's in-memory state, then call RemoveParticipant, + // which no-ops because this join has never reached the SFU yet + // (GenerateToken is a local JWT mint, no LiveKit round trip). The tail + // guard below used to be the only check, but by then the token had + // already been queued for delivery — the client ends up with a live + // 5-minute RoomJoin credential for a membership the server just decided + // does not exist, and connects to the SFU with it regardless of what + // happens after. Re-check here, immediately before the credential + // leaves the process, and withhold it if superseded. + if curChID, curToken := c.getVoiceState(); curChID != channelID || curToken != state.JoinedAt { + slog.Info("ws handleVoiceJoin: join superseded before token delivery", + "user_id", c.userID, "channel_id", channelID, "current_channel_id", curChID) + // Best-effort defense in depth: this join has not reached the SFU + // (see above), so this is normally a no-op, but it closes the + // sliver of time between this check and c.sendMsg below the same + // way every other eviction path's RemoveParticipant call does. + rbCtx := context.WithoutCancel(ctx) + if err := h.livekit.RemoveParticipant(rbCtx, channelID, c.userID, state.JoinedAt); err != nil { + slog.Warn("ws handleVoiceJoin: RemoveParticipant after supersession failed (may already be gone)", + "err", err, "user_id", c.userID, "channel_id", channelID) + } + return + } // Send both proxy path and direct URL. The client uses direct_url // when on localhost (avoids self-signed TLS issues with WebView // fetch) and falls back to the /livekit proxy for remote clients. @@ -337,11 +378,17 @@ func (h *Hub) handleVoiceJoin(ctx context.Context, c *Client, payload json.RawMe // after the DB row committed — which also means a concurrent eviction (the // revocation sweep, a participant_left webhook, a moderator kick/move) can // now land on THIS join instance while the token round trip above is in - // flight. Those all clear the client's voice state and delete the row after - // deciding against it, so completing the join here would resurrect a - // membership that was deliberately torn down: subscribed to the voice - // topic and broadcast as present, with no row behind it. Their decision - // wins; a same-instance state is the only thing this join may finish. + // flight. The check inside the h.livekit block above (OC-0008) already + // withholds the token itself in that case; this is the tail guard for + // everything downstream of it (voice topic subscription, the joiner's own + // voice_state broadcast) when no token round trip ran at all (h.livekit == + // nil is unreachable in practice — handleVoiceJoin returns earlier — but + // kept here as the single completion gate for both paths). Those evictors + // all clear the client's voice state and delete the row after deciding + // against it, so completing the join here would resurrect a membership + // that was deliberately torn down: subscribed to the voice topic and + // broadcast as present, with no row behind it. Their decision wins; a + // same-instance state is the only thing this join may finish. if curChID, curToken := c.getVoiceState(); curChID != channelID || curToken != state.JoinedAt { slog.Info("ws handleVoiceJoin: join superseded before completion", "user_id", c.userID, "channel_id", channelID, "current_channel_id", curChID) diff --git a/Server/ws/voice_join_token_race_test.go b/Server/ws/voice_join_token_race_test.go new file mode 100644 index 00000000..bec37afa --- /dev/null +++ b/Server/ws/voice_join_token_race_test.go @@ -0,0 +1,115 @@ +package ws + +// voice_join_token_race_test.go — regression test for OC-0008. +// +// handleVoiceJoin used to hand the client its LiveKit token (voice_join.go, +// the c.sendMsg(buildVoiceToken(...)) call) BEFORE checking whether the join +// had been superseded by a concurrent eviction (voice_mod_kick/move, the +// CONNECT_VOICE revocation sweep, CleanupVoiceForChannel). Those evictors all +// run while the joiner's goroutine is still inside the permission checks and +// GenerateToken call: they clear the client's in-memory voice state, delete +// the voice_states row, and call RemoveParticipant — which no-ops because the +// join has not reached the SFU yet. The token that goes out afterward is +// therefore a live 5-minute RoomJoin credential for a user the server just +// decided is not in the channel, and the client connects to the SFU with it. +// +// GenerateToken itself is a local JWT mint with no I/O, so the window between +// it and c.sendMsg is too narrow to land by staggering real goroutines. +// voiceJoinPostTokenRaceHook (test-only, nil in production) fires at exactly +// that point, mirroring the existing cleanupVoiceRaceClearHook pattern +// (hub_sweep.go) used to pin the analogous CleanupVoiceForChannel race. + +import ( + "context" + "encoding/json" + "testing" + "time" + + "github.com/owncord/server/auth" + "github.com/owncord/server/config" + "github.com/owncord/server/db" +) + +// TestVoiceJoin_SupersededDuringTokenGeneration_WithholdsToken pins OC-0008: +// if the client's voice state no longer matches this join instance by the +// time GenerateToken returns, the minted token must never reach the client. +func TestVoiceJoin_SupersededDuringTokenGeneration_WithholdsToken(t *testing.T) { + database := newHarvestVoiceDB(t) + uid := seedHarvestVoiceUser(t, database, "join-race-victim") + chID := mustCreateVoiceChannel(t, database, "voice-join-race") + + lk, err := NewLiveKitClient(&config.VoiceConfig{ + LiveKitAPIKey: "test-api-key-race-0008", + LiveKitAPISecret: "test-api-secret-race-0008-xyz", + LiveKitURL: "ws://127.0.0.1:1", // never dialed: GenerateToken is local + }) + if err != nil { + t.Fatalf("NewLiveKitClient: %v", err) + } + + h := NewHub(database, auth.NewRateLimiter(), nil) + h.SetLiveKit(lk) + + send := make(chan []byte, 8) + c := NewTestClient(h, uid, send) + c.user = &db.User{ID: uid, Username: "join-race-victim"} + h.mu.Lock() + h.clients[uid] = c + h.mu.Unlock() + + // Simulate a moderator's kick landing between GenerateToken returning and + // the token being handed to the client: clear the client's in-memory + // voice state and delete the DB row, exactly as + // DisconnectFromVoiceInChannel -> handleVoiceLeaveIfStillIn -> + // clearVoiceStateIfMatch / LeaveVoiceChannelIfMatch do. + var hookRan bool + voiceJoinPostTokenRaceHook = func(client *Client) { + hookRan = true + if _, cleared := client.clearVoiceStateIfMatch(chID); !cleared { + t.Errorf("hook: client voice state did not match channel %d at hook time", chID) + } + state, err := database.GetVoiceState(context.Background(), uid) + if err != nil { + t.Fatalf("hook: GetVoiceState: %v", err) + } + if state != nil { + if _, err := database.LeaveVoiceChannelIfMatch(context.Background(), uid, chID, state.JoinedAt); err != nil { + t.Fatalf("hook: LeaveVoiceChannelIfMatch: %v", err) + } + } + } + defer func() { voiceJoinPostTokenRaceHook = nil }() + + payload, _ := json.Marshal(map[string]any{"channel_id": chID}) + h.handleVoiceJoin(context.Background(), c, json.RawMessage(payload)) + + if !hookRan { + t.Fatal("voiceJoinPostTokenRaceHook never fired — test setup is broken, not exercising the join path") + } + + msgs := drainChan(send, 100*time.Millisecond) + for _, m := range msgs { + var env struct { + Type string `json:"type"` + } + if err := json.Unmarshal(m, &env); err != nil { + continue + } + if env.Type == "voice_token" { + t.Errorf("client received a voice_token for a join the server had already evicted mid-flight — got message %s", m) + } + } + + // The DB row must stay gone: the join must not resurrect what the + // concurrent eviction just tore down. + if state, err := database.GetVoiceState(context.Background(), uid); err != nil { + t.Fatalf("GetVoiceState after join: %v", err) + } else if state != nil { + t.Errorf("voice_states row for user %d still present after a join superseded mid-flight, want it to stay deleted", uid) + } + + // The client's own in-memory voice state must also stay cleared. + if gotCh := c.getVoiceChID(); gotCh != 0 { + t.Errorf("client voiceChID = %d after a join superseded mid-flight, want 0 (cleared by the eviction, not resurrected)", gotCh) + } +} diff --git a/Server/ws/voice_moderation.go b/Server/ws/voice_moderation.go index a7a62e7b..56ff14b9 100644 --- a/Server/ws/voice_moderation.go +++ b/Server/ws/voice_moderation.go @@ -178,10 +178,18 @@ func handleVoiceModMuteV2(ctx context.Context, cmd Command, info ClientInfo, dep return *r } - if err := d.DB.SetVoiceServerMute(ctx, c.TargetID(), c.Muted()); err != nil { + matched, err := d.DB.SetVoiceServerMute(ctx, c.TargetID(), state.ChannelID, c.Muted()) + if err != nil { slog.Error("ws handleVoiceModMuteV2 SetVoiceServerMute", "err", err, "target_id", c.TargetID()) return Result{Error: ClientError{Code: ErrCodeInternal, Message: "failed to update server mute"}} } + if !matched { + // The target's row moved off state.ChannelID between requireTargetInChannel's + // snapshot and this write (OC-0005) -- same refusal requireTargetInChannel + // itself gives for the non-racing case, so the write never follows the + // target onto a channel (including a DM call) nobody authorized it against. + return Result{Error: ClientError{Code: ErrCodeVoiceError, Message: "user is not in that voice channel"}} + } if d.Mod != nil { if err := d.Mod.MuteParticipant(ctx, state.ChannelID, c.TargetID(), state.JoinedAt, c.Muted()); err != nil { slog.Warn("ws handleVoiceModMuteV2 MuteParticipant failed", @@ -197,6 +205,14 @@ func handleVoiceModMuteV2(ctx context.Context, cmd Command, info ClientInfo, dep return voiceStateBroadcast(ctx, d, c.TargetID()) } +// voiceModDeafenPreMuteRaceHook, when non-nil, runs immediately after the +// deafen write matches and before the implied-mute write that follows it — +// the one-statement-wide window a concurrent channel switch would need to +// land in for OC-0034. Test-only (nil in production), mirroring the +// voiceJoinPostTokenRaceHook / cleanupVoiceRaceClearHook pattern used to pin +// the analogous races elsewhere. +var voiceModDeafenPreMuteRaceHook func(ctx context.Context, d VoiceDeps, targetID int64) + // handleVoiceModDeafenV2 processes a voice_mod_deafen command. Deafen has no // SFU equivalent (it is about what the target plays back), so it is enforced by // the target's client honoring server_deafened plus the server refusing their @@ -216,10 +232,21 @@ func handleVoiceModDeafenV2(ctx context.Context, cmd Command, info ClientInfo, d return *r } - if err := d.DB.SetVoiceServerDeafen(ctx, c.TargetID(), c.Deafened()); err != nil { + deafenMatched, err := d.DB.SetVoiceServerDeafen(ctx, c.TargetID(), state.ChannelID, c.Deafened()) + if err != nil { slog.Error("ws handleVoiceModDeafenV2 SetVoiceServerDeafen", "err", err, "target_id", c.TargetID()) return Result{Error: ClientError{Code: ErrCodeInternal, Message: "failed to update server deafen"}} } + if !deafenMatched { + // The target's row moved off state.ChannelID between requireTargetInChannel's + // snapshot and this write (OC-0005) -- refuse exactly as requireTargetInChannel + // itself does for the non-racing case, before the implied mute below can + // touch a channel nobody authorized it against. + return Result{Error: ClientError{Code: ErrCodeVoiceError, Message: "user is not in that voice channel"}} + } + if voiceModDeafenPreMuteRaceHook != nil { + voiceModDeafenPreMuteRaceHook(ctx, d, c.TargetID()) + } // A server deafen implies a server mute at the SFU: a deafened user must // not keep talking into a room they cannot hear. Lifting the deafen must // lift that implied mute too, or the target stays SFU-muted and refused @@ -227,8 +254,11 @@ func handleVoiceModDeafenV2(ctx context.Context, cmd Command, info ClientInfo, d // single bool with no way to tell "explicit" from "deafen-implied" apart, // so an explicit-mute-then-deafen sequence has both lifted together by an // undeafen — accepted as the simplest correct behavior given the schema. - if err := d.DB.SetVoiceServerMute(ctx, c.TargetID(), c.Deafened()); err != nil { - slog.Error("ws handleVoiceModDeafenV2 SetVoiceServerMute", "err", err, "target_id", c.TargetID()) + muteMatched, err := d.DB.SetVoiceServerMute(ctx, c.TargetID(), state.ChannelID, c.Deafened()) + if err != nil || !muteMatched { + if err != nil { + slog.Error("ws handleVoiceModDeafenV2 SetVoiceServerMute", "err", err, "target_id", c.TargetID()) + } // The deafen write above already committed as its own statement (no // transaction spans the two — a single UPDATE covering both columns // needs a db-change; see cross_batch). Best-effort undo it rather @@ -236,13 +266,53 @@ func handleVoiceModDeafenV2(ctx context.Context, cmd Command, info ClientInfo, d // is not SFU-muted yet still refuses the target's own undeafen // (refuseIfServerSilenced), for a deafen nobody was ever told about. // Detached from ctx — the cancellation that most likely caused the - // failure above (the moderator's socket dropping mid-request) must - // not also abort the rollback. - if compErr := d.DB.SetVoiceServerDeafen(context.WithoutCancel(ctx), c.TargetID(), !c.Deafened()); compErr != nil { - slog.Error("ws handleVoiceModDeafenV2 SetVoiceServerDeafen rollback failed", - "err", compErr, "target_id", c.TargetID()) + // failure above (the moderator's socket dropping mid-request, or the + // target moving off state.ChannelID between the two writes) must not + // also abort the rollback. + // + // Re-read the row's CURRENT channel rather than reusing the stale + // state.ChannelID snapshot: when the mismatch above was caused by + // the target switching channels (not leaving voice), the row is no + // longer on state.ChannelID, so a rollback scoped to that stale + // channel matches zero rows and silently no-ops -- exactly the case + // this rollback exists to handle (OC-0034). Clearing a restriction + // is safe on whatever channel the row is actually on now; if the + // row is gone entirely (target left voice), there is nothing left + // to roll back. + // + // The rollback value is the OPPOSITE of the request (!c.Deafened()), + // so which channel it is safe to scope to depends on which + // direction it runs: + // - request was a DEAFEN (c.Deafened()==true): rollback CLEARS. + // Clearing a restriction can never authorize anything the + // target wasn't already free of, so following the row to + // cur.ChannelID is safe -- this is the OC-0034 case above. + // - request was an UNDEAFEN (c.Deafened()==false): rollback + // APPLIES a restriction. Scoping an apply to cur.ChannelID + // would stamp it onto whatever channel the row now points at, + // including one voiceModTarget never authorized the actor + // against (OC-0036) -- the exact hazard channel-scoping exists + // to prevent for the ordinary write path. Scope to + // state.ChannelID (the channel that WAS authorized) instead, + // so a moved/rejoined target simply matches zero rows. + compCtx := context.WithoutCancel(ctx) + if cur, gErr := d.DB.GetVoiceState(compCtx, c.TargetID()); gErr != nil { + slog.Error("ws handleVoiceModDeafenV2 GetVoiceState for rollback", + "err", gErr, "target_id", c.TargetID()) + } else if cur != nil { + rollbackChannelID := cur.ChannelID + if !c.Deafened() { + rollbackChannelID = state.ChannelID + } + if _, compErr := d.DB.SetVoiceServerDeafen(compCtx, c.TargetID(), rollbackChannelID, !c.Deafened()); compErr != nil { + slog.Error("ws handleVoiceModDeafenV2 SetVoiceServerDeafen rollback failed", + "err", compErr, "target_id", c.TargetID()) + } } - return Result{Error: ClientError{Code: ErrCodeInternal, Message: "failed to update server deafen"}} + if err != nil { + return Result{Error: ClientError{Code: ErrCodeInternal, Message: "failed to update server deafen"}} + } + return Result{Error: ClientError{Code: ErrCodeVoiceError, Message: "user is not in that voice channel"}} } if d.Mod != nil { if err := d.Mod.MuteParticipant(ctx, state.ChannelID, c.TargetID(), state.JoinedAt, c.Deafened()); err != nil { diff --git a/Server/ws/voice_moderation_deafen_race_test.go b/Server/ws/voice_moderation_deafen_race_test.go new file mode 100644 index 00000000..560a07d1 --- /dev/null +++ b/Server/ws/voice_moderation_deafen_race_test.go @@ -0,0 +1,215 @@ +package ws + +// voice_moderation_deafen_race_test.go — regression test for OC-0034. +// +// handleVoiceModDeafenV2 implies a server mute when it applies a server +// deafen. The two writes are separate statements (no transaction spans +// them), so when the second (the mute) fails to match because the target's +// voice_states row moved to a different channel in between, the handler +// best-effort rolls back the deafen it just applied. That rollback used to +// pass the same stale channel snapshot that just failed to match the mute +// write, so it also matched zero rows and silently no-opped — leaving the +// target server_deafened=1 with server_muted=0, on their new channel, with +// no SFU mute in effect yet still refused their own undeafen +// (refuseIfServerSilenced). +// +// The window is one SQLite statement wide and cannot be landed by staggering +// real goroutines, so voiceModDeafenPreMuteRaceHook (test-only, nil in +// production) fires at exactly that point, mirroring the existing +// voiceJoinPostTokenRaceHook / cleanupVoiceRaceClearHook pattern. + +import ( + "context" + "testing" + + "github.com/owncord/server/db" +) + +// deafenRaceRoleAdmin / deafenRaceRoleMember reuse the default seeded roles +// (see migrations/001_initial_schema.sql): Admin holds MUTE_MEMBERS at +// position 80, Member sits below it at position 40, which is what +// voiceModTarget's outrank check needs. +const ( + deafenRaceRoleAdmin = 2 + deafenRaceRoleMember = 4 +) + +func newDeafenRaceDB(t *testing.T) *db.DB { + t.Helper() + database, err := db.Open(":memory:") + if err != nil { + t.Fatalf("db.Open: %v", err) + } + if err := db.Migrate(database); err != nil { + t.Fatalf("Migrate: %v", err) + } + t.Cleanup(func() { _ = database.Close() }) + return database +} + +func seedDeafenRaceUser(t *testing.T, database *db.DB, username string, roleID int) int64 { + t.Helper() + uid, err := database.CreateUser(context.Background(), username, "hash", roleID) + if err != nil { + t.Fatalf("CreateUser %s: %v", username, err) + } + return uid +} + +func mustCreateDeafenRaceChannel(t *testing.T, database *db.DB, name string) int64 { + t.Helper() + chID, err := database.CreateChannel(context.Background(), name, "voice", "", "", 0) + if err != nil { + t.Fatalf("CreateChannel %s: %v", name, err) + } + return chID +} + +// TestVoiceModDeafen_RollbackFollowsTargetChannelMove pins OC-0034: the +// compensating deafen-clear must be scoped to the target's CURRENT channel, +// not the stale channel snapshot the mute write just failed to match +// against, or the rollback silently no-ops when the mismatch was caused by a +// channel move rather than the target leaving voice entirely. +func TestVoiceModDeafen_RollbackFollowsTargetChannelMove(t *testing.T) { + database := newDeafenRaceDB(t) + ctx := context.Background() + + chanA := mustCreateDeafenRaceChannel(t, database, "vc-deafen-race-a") + chanB := mustCreateDeafenRaceChannel(t, database, "vc-deafen-race-b") + actorID := seedDeafenRaceUser(t, database, "deafen-race-admin", deafenRaceRoleAdmin) + targetID := seedDeafenRaceUser(t, database, "deafen-race-member", deafenRaceRoleMember) + + if err := database.JoinVoiceChannel(ctx, targetID, chanA); err != nil { + t.Fatalf("JoinVoiceChannel: %v", err) + } + + // Fire exactly between the deafen write (which matches, since the target + // is still on chanA at that point) and the implied-mute write: move the + // target's row to chanB, which is what makes the mute write fail to + // match against the chanA snapshot the handler is holding. + var hookRan bool + voiceModDeafenPreMuteRaceHook = func(ctx context.Context, d VoiceDeps, targetID int64) { + hookRan = true + if err := d.DB.JoinVoiceChannel(ctx, targetID, chanB); err != nil { + t.Fatalf("hook: JoinVoiceChannel to chanB: %v", err) + } + } + defer func() { voiceModDeafenPreMuteRaceHook = nil }() + + cmd := VoiceModDeafenCmd{userID: actorID, channelID: chanA, targetID: targetID, deafened: true} + info := ClientInfo{UserID: actorID} + deps := VoiceDeps{DB: database} + + result := handleVoiceModDeafenV2(ctx, cmd, info, deps) + + if !hookRan { + t.Fatal("voiceModDeafenPreMuteRaceHook never fired — test setup is broken, not exercising the race window") + } + clientErr, ok := result.Error.(ClientError) + if !ok { + t.Fatalf("result error = %#v (%T), want a ClientError", result.Error, result.Error) + } + if clientErr.Code != ErrCodeVoiceError { + t.Fatalf("result error code = %q, want %q (target moved channels mid-request)", clientErr.Code, ErrCodeVoiceError) + } + + state, err := database.GetVoiceState(ctx, targetID) + if err != nil { + t.Fatalf("GetVoiceState: %v", err) + } + if state == nil { + t.Fatal("target's voice_states row disappeared") + } + if state.ChannelID != chanB { + t.Fatalf("test setup broken: target channel = %d, want %d (chanB)", state.ChannelID, chanB) + } + if state.ServerDeafened { + t.Error("ServerDeafened = true after the mismatched mute write, want false: " + + "the compensating rollback must clear it on the target's CURRENT channel, " + + "not the stale channel snapshot that already failed to match") + } + if state.ServerMuted { + t.Error("ServerMuted = true, want false: the implied-mute write never matched, so it must not have applied") + } +} + +// TestVoiceModDeafen_UndeafenRollbackDoesNotApplyOnUnauthorizedChannel pins +// OC-0036: when the moderator's command is an UNDEAFEN (c.Deafened() == +// false), the compensating rollback runs in the opposite direction from +// TestVoiceModDeafen_RollbackFollowsTargetChannelMove above -- it APPLIES a +// server deafen, not clears one. Scoping that apply to the target's CURRENT +// channel (cur.ChannelID, re-read after the race) stamps a moderator +// restriction onto a channel voiceModTarget never authorized the actor +// against, exactly the hazard SetVoiceServerDeafen's channel scoping exists +// to prevent for the ordinary (non-rollback) write path. The rollback must +// instead scope the APPLY direction to the channel that WAS authorized +// (state.ChannelID), so a target who moved channels mid-request simply ends +// up with the rollback matching zero rows -- the safe outcome. +func TestVoiceModDeafen_UndeafenRollbackDoesNotApplyOnUnauthorizedChannel(t *testing.T) { + database := newDeafenRaceDB(t) + ctx := context.Background() + + chanA := mustCreateDeafenRaceChannel(t, database, "vc-undeafen-race-a") + chanB := mustCreateDeafenRaceChannel(t, database, "vc-undeafen-race-b") + actorID := seedDeafenRaceUser(t, database, "undeafen-race-admin", deafenRaceRoleAdmin) + targetID := seedDeafenRaceUser(t, database, "undeafen-race-member", deafenRaceRoleMember) + + if err := database.JoinVoiceChannel(ctx, targetID, chanA); err != nil { + t.Fatalf("JoinVoiceChannel: %v", err) + } + // Seed a pre-existing server deafen on chanA so the command below is a + // genuine undeafen (the ordinary direction: volume-menu.ts sends + // !mod.serverDeafened, and toggling off is the common case). + if matched, err := database.SetVoiceServerDeafen(ctx, targetID, chanA, true); err != nil || !matched { + t.Fatalf("seed SetVoiceServerDeafen: matched=%v err=%v", matched, err) + } + + // Fire exactly between the deafen-clear write (which matches, since the + // target is still on chanA at that point) and the implied-mute write: + // move the target's row to chanB, which is what makes the mute write + // fail to match against the chanA snapshot the handler is holding. + var hookRan bool + voiceModDeafenPreMuteRaceHook = func(ctx context.Context, d VoiceDeps, targetID int64) { + hookRan = true + if err := d.DB.JoinVoiceChannel(ctx, targetID, chanB); err != nil { + t.Fatalf("hook: JoinVoiceChannel to chanB: %v", err) + } + } + defer func() { voiceModDeafenPreMuteRaceHook = nil }() + + // deafened: false -- an UNDEAFEN, the opposite direction from the sibling + // test above. + cmd := VoiceModDeafenCmd{userID: actorID, channelID: chanA, targetID: targetID, deafened: false} + info := ClientInfo{UserID: actorID} + deps := VoiceDeps{DB: database} + + result := handleVoiceModDeafenV2(ctx, cmd, info, deps) + + if !hookRan { + t.Fatal("voiceModDeafenPreMuteRaceHook never fired — test setup is broken, not exercising the race window") + } + clientErr, ok := result.Error.(ClientError) + if !ok { + t.Fatalf("result error = %#v (%T), want a ClientError", result.Error, result.Error) + } + if clientErr.Code != ErrCodeVoiceError { + t.Fatalf("result error code = %q, want %q (target moved channels mid-request)", clientErr.Code, ErrCodeVoiceError) + } + + state, err := database.GetVoiceState(ctx, targetID) + if err != nil { + t.Fatalf("GetVoiceState: %v", err) + } + if state == nil { + t.Fatal("target's voice_states row disappeared") + } + if state.ChannelID != chanB { + t.Fatalf("test setup broken: target channel = %d, want %d (chanB)", state.ChannelID, chanB) + } + if state.ServerDeafened { + t.Error("ServerDeafened = true on chanB after the mismatched mute write, want false: " + + "the compensating rollback re-applies a deafen (the command was an undeafen), which " + + "must only ever land on the channel voiceModTarget actually authorized (chanA) -- " + + "stamping it onto chanB, a channel nobody authorized the actor against, is OC-0036") + } +}