diff --git a/Client/tauri-client/src-tauri/src/http_proxy.rs b/Client/tauri-client/src-tauri/src/http_proxy.rs index f5d443be..680db082 100644 --- a/Client/tauri-client/src-tauri/src/http_proxy.rs +++ b/Client/tauri-client/src-tauri/src/http_proxy.rs @@ -447,7 +447,7 @@ async fn handle_connection( // ── 3. Forward request + bidirectional copy ────────────────────────── tls.write_all(modified.as_bytes()).await?; - match io::copy_bidirectional(&mut local, &mut tls).await { + match copy_with_deadline(&mut local, &mut tls, DATA_PHASE_TIMEOUT).await { Ok((to_remote, from_remote)) => { debug!( "[http_proxy] connection closed: {}B sent, {}B received", @@ -461,6 +461,40 @@ async fn handle_connection( Ok(()) } +/// Bound for the data-copy phase of a tunneled connection (step 3 above). +/// The header read, TCP connect, and TLS handshake phases all use a tight +/// 10s guard, but this phase carries the actual REST body — including +/// attachment/avatar uploads — so it needs a much more generous bound. 600s +/// only reclaims a connection that is genuinely stuck (e.g. a remote that +/// completes the TLS handshake and then neither responds nor closes), not +/// one that is merely slow. +const DATA_PHASE_TIMEOUT: Duration = Duration::from_secs(600); + +/// Run `io::copy_bidirectional` under a deadline. Without this, a remote +/// that completes the TLS handshake and then stalls forever (neither +/// responding nor closing) parks the spawned connection task — and both the +/// loopback socket and the remote TLS session — indefinitely; closing the +/// local side alone does not free it, since `copy_bidirectional` only +/// resolves once BOTH directions finish. Generic over the stream types so it +/// can be unit-tested without a live TLS connection. +async fn copy_with_deadline( + local: &mut A, + remote: &mut B, + dur: Duration, +) -> io::Result<(u64, u64)> +where + A: io::AsyncRead + io::AsyncWrite + Unpin + ?Sized, + B: io::AsyncRead + io::AsyncWrite + Unpin + ?Sized, +{ + match timeout(dur, io::copy_bidirectional(local, remote)).await { + Ok(result) => result, + Err(_) => Err(io::Error::new( + io::ErrorKind::TimedOut, + "data phase timed out", + )), + } +} + // --------------------------------------------------------------------------- // Tests // --------------------------------------------------------------------------- @@ -592,4 +626,35 @@ mod tests { assert!(out.contains("Content-Length: 2\r\n")); assert!(out.ends_with("\r\n\r\n")); } + + // OC-0218: the data phase of a tunneled request (step 3 in + // `handle_connection`) must not be able to hang forever. A remote that + // completes the TLS handshake and then neither responds nor closes must + // eventually be reclaimed, the same way the header-read/connect/handshake + // phases already are (10s guards above). Simulate that stall with two + // in-memory duplex pairs where neither peer ever writes or disconnects, + // so raw `io::copy_bidirectional` would block forever. + #[tokio::test] + async fn copy_with_deadline_reclaims_a_stalled_connection() { + // Keep both "far" ends alive (bound, not `_`) so neither duplex half + // observes EOF — this is what makes the connection "stalled" rather + // than "closed". + let (mut local_near, _local_far) = tokio::io::duplex(64); + let (mut remote_near, _remote_far) = tokio::io::duplex(64); + + // An outer safety bound: if `copy_with_deadline` does not honor its + // own deadline, fail fast instead of hanging the test suite forever. + let outcome = tokio::time::timeout( + Duration::from_secs(5), + copy_with_deadline(&mut local_near, &mut remote_near, Duration::from_millis(50)), + ) + .await + .expect( + "copy_with_deadline must resolve on its own deadline; the data phase must not hang \ + indefinitely on a stalled remote (OC-0218)", + ); + + let err = outcome.expect_err("a stalled remote must surface as a timeout error, not Ok"); + assert_eq!(err.kind(), io::ErrorKind::TimedOut); + } } diff --git a/Client/tauri-client/src/components/MessageList.ts b/Client/tauri-client/src/components/MessageList.ts index 042f3a7b..7e0e571d 100644 --- a/Client/tauri-client/src/components/MessageList.ts +++ b/Client/tauri-client/src/components/MessageList.ts @@ -323,6 +323,21 @@ export function createMessageList(options: MessageListOptions): MessageListCompo if (item === undefined) return `idx-${index}`; if (item.kind === "divider") return `div-${item.timestamp}`; if (item.kind === "new-divider") return "new-divider"; + // Every unconfirmed optimistic row (addOptimisticMessage) carries + // id: 0 until confirmSend stamps the real id, so keying purely on + // message.id would collide two or more pending rows onto the same + // "msg-0" cache entry — measureRendered would overwrite one row's + // measured height with another's, and the next Fenwick rebuild + // (rebuildItems / tryAppendMessages) would seed both rows' tree slots + // from that single, wrong value. correlationId is unique per pending + // send and stable across the row's lifetime, so key on that instead + // while id is still the 0 sentinel; fall back to the row's own index + // in the vanishingly unlikely case correlationId is also absent. + if (item.message.id === 0) { + return item.message.correlationId !== null + ? `msg-c-${item.message.correlationId}` + : `idx-${index}`; + } return `msg-${item.message.id}`; } diff --git a/Client/tauri-client/src/components/UpdateNotifier.ts b/Client/tauri-client/src/components/UpdateNotifier.ts index 881cb57e..549475fe 100644 --- a/Client/tauri-client/src/components/UpdateNotifier.ts +++ b/Client/tauri-client/src/components/UpdateNotifier.ts @@ -31,6 +31,7 @@ export function createUpdateNotifier(options: UpdateNotifierOptions): MountableC let container: Element | null = null; let banner: HTMLDivElement | null = null; let dismissed = false; + let checkTimer: ReturnType | null = null; async function performCheck(): Promise { if (dismissed) return; @@ -123,12 +124,17 @@ export function createUpdateNotifier(options: UpdateNotifierOptions): MountableC function mount(target: Element): void { container = target; // Delay the check slightly so the main UI renders first - setTimeout(() => { + checkTimer = setTimeout(() => { + checkTimer = null; void performCheck(); }, 3000); } function destroy(): void { + if (checkTimer !== null) { + clearTimeout(checkTimer); + checkTimer = null; + } removeBanner(); container = null; } diff --git a/Client/tauri-client/src/components/UserBar.ts b/Client/tauri-client/src/components/UserBar.ts index 0034026f..1ce68dec 100644 --- a/Client/tauri-client/src/components/UserBar.ts +++ b/Client/tauri-client/src/components/UserBar.ts @@ -21,10 +21,21 @@ import { import { avatarInitial, isRenderableAvatar, resolveDisplayName } from "@lib/avatar"; import { fetchImageAsDataUrl, resolveServerUrl } from "@components/message-list/attachments"; import type { WsClient } from "@lib/ws"; +import type { PresenceSender } from "@lib/presence"; export interface UserBarOptions { readonly onDisconnect?: () => void; readonly ws?: WsClient | null; + /** + * The session's single shared presence sender (MainPage owns the instance + * and threads it to every producer — auto-idle, the settings Account tab, + * and this picker). Sending straight through `ws` instead would bypass the + * presence rate limiter's client-side token *and* its retry, so a frame + * the server drops (1 update / 10s, keyed by user id — service/ + * channel.go) is lost for the rest of the session instead of retried + * (OC-0210). Required, alongside `ws`, for the picker to be enabled. + */ + readonly presenceSender?: PresenceSender | null; } /** Status labels for the line under the username. */ @@ -135,11 +146,21 @@ export function createUserBar(options?: UserBarOptions): MountableComponent { }); // The picker is usable only when the socket is live (store-backed status, - // docs/architecture/ux §3) AND a ws client was provided to send through — - // without a send path, selecting a status would be a silent no-op. + // docs/architecture/ux §3) AND a presence sender was provided to send + // through — without one, selecting a status would either be a silent + // no-op or (worse) bypass the shared presence rate limiter and its retry + // (OC-0210). `ws` is checked too since a sender without a live socket + // behind it is not meaningfully usable either. const canSetStatus = (): boolean => { const ws = options?.ws; - return ws !== undefined && ws !== null && uiStore.getState().connectionStatus === "connected"; + const sender = options?.presenceSender; + return ( + ws !== undefined && + ws !== null && + sender !== undefined && + sender !== null && + uiStore.getState().connectionStatus === "connected" + ); }; statusPicker = createStatusPicker({ @@ -150,21 +171,20 @@ export function createUserBar(options?: UserBarOptions): MountableComponent { onStatusChange: (status: UserStatus) => { saveUserStatus(status); updateFromState(); - const ws = options?.ws; - if (ws !== null && ws !== undefined && canSetStatus()) { + const sender = options?.presenceSender; + if (sender !== null && sender !== undefined && canSetStatus()) { // No custom_status field: a plain status change must leave whatever - // text the user set standing. - ws.send({ type: "presence_update", payload: { status } } as never); + // text the user set standing. Routed through the shared sender + // (not ws.send directly) so a frame the presence limiter's window + // rejects is retried instead of lost — see @lib/presence. + sender.send(status); } }, onCustomStatusChange: (text: string) => { saveCustomStatus(text); - const ws = options?.ws; - if (ws !== null && ws !== undefined && canSetStatus()) { - ws.send({ - type: "presence_update", - payload: { status: loadUserStatus(), custom_status: text }, - } as never); + const sender = options?.presenceSender; + if (sender !== null && sender !== undefined && canSetStatus()) { + sender.send(loadUserStatus(), text); } }, }); diff --git a/Client/tauri-client/src/components/VoiceWidget.ts b/Client/tauri-client/src/components/VoiceWidget.ts index 547baf73..971bc5c7 100644 --- a/Client/tauri-client/src/components/VoiceWidget.ts +++ b/Client/tauri-client/src/components/VoiceWidget.ts @@ -456,8 +456,14 @@ export function createVoiceWidget(options: VoiceWidgetOptions): MountableCompone } void retryMicPermission().finally(() => { if (grantMicBtn) { - grantMicBtn.disabled = false; setText(grantMicBtn, "Grant Microphone"); + // Delegate the disabled/title state back to render(), which + // re-runs updateFrozen() — the single authority for the + // socket-down freeze. Hardcoding `disabled = false` here would + // silently re-enable this button (and drop its stale title) + // even while the WS socket is still down and every sibling + // control remains frozen. + render(); } }); }, diff --git a/Client/tauri-client/src/components/settings/helpers.ts b/Client/tauri-client/src/components/settings/helpers.ts index 109e342a..fcc39976 100644 --- a/Client/tauri-client/src/components/settings/helpers.ts +++ b/Client/tauri-client/src/components/settings/helpers.ts @@ -68,6 +68,14 @@ export const THEMES = { export type ThemeName = keyof typeof THEMES; +// Union of every CSS custom property any built-in theme sets. Used by +// applyTheme to clear a previous theme's tokens before applying a new one, +// without touching inline properties owned by other code (e.g. --accent, +// --font-size). +const THEME_KEYS: ReadonlySet = new Set( + Object.values(THEMES).flatMap((theme) => Object.keys(theme)), +); + // --------------------------------------------------------------------------- // Accessible toggle creation // --------------------------------------------------------------------------- @@ -114,9 +122,17 @@ export function createToggle( // --------------------------------------------------------------------------- export function applyTheme(name: ThemeName): void { - // Apply CSS variables for the theme (keeps existing behavior for inline var overrides) const theme = THEMES[name]; const root = document.documentElement; + // Clear every key any built-in theme owns first, so switching to a theme + // that sets fewer keys (e.g. light -> dark) doesn't leave the previous + // theme's tokens stuck on , outranking tokens.css's :root defaults + // via inline-style specificity. Keys owned by other code (--accent, + // --font-size) are not in THEME_KEYS and are left untouched. + for (const key of THEME_KEYS) { + root.style.removeProperty(key); + } + // Apply CSS variables for the theme (keeps existing behavior for inline var overrides) for (const [key, value] of Object.entries(theme)) { root.style.setProperty(key, value); } diff --git a/Client/tauri-client/src/lib/dispatcher.ts b/Client/tauri-client/src/lib/dispatcher.ts index c1e8115b..7db8941b 100644 --- a/Client/tauri-client/src/lib/dispatcher.ts +++ b/Client/tauri-client/src/lib/dispatcher.ts @@ -366,9 +366,25 @@ export function wireDispatcher( if (activeAfterReady !== null && getMessages !== undefined) { invalidateLoadedMessageWindows(); getMessages(activeAfterReady, { limit: 50 }) - .then((resp) => setMessages(activeAfterReady, resp.messages, resp.has_more)) + .then((resp) => { + // OC-0203: the user can switch (or the active channel can be + // cleared) while this fetch is in flight. Writing the snapshot + // unconditionally would re-add a channel the user already left + // to loadedChannels with a pre-resync-era snapshot — + // MessageController.loadMessages then short-circuits on + // isChannelLoaded() forever, so the hole this whole resync + // block exists to close becomes permanent instead. Only the + // channel still on screen when the response lands may accept + // it. + if (channelsStore.select((s) => s.activeChannelId) !== activeAfterReady) return; + setMessages(activeAfterReady, resp.messages, resp.has_more); + }) .catch((err) => { log.warn("Failed to reload message history after resync", { error: String(err) }); + // Same staleness guard as the .then above — a rejection for a + // channel the user already left must not flag it load-errored; + // that channel's own mount/retry path owns its state now. + if (channelsStore.select((s) => s.activeChannelId) !== activeAfterReady) return; // The invalidate above already dropped this channel's window, // so a silent catch would leave a mounted MessageList showing // its "no messages yet" welcome state — indistinguishable from @@ -1058,6 +1074,33 @@ export function wireDispatcher( if (id !== undefined && rollbackReaction(id)) { return; } + // OC-0224: the sidebar/widget optimistically writes currentChannelId + // before the server answers voice_join (VoiceCallbacks.onVoiceJoin, + // voiceStatus="joining"). A first-time join refusal earns no + // voice_leave (there was no previous channel to leave), so nothing + // else ever clears that optimistic state — setVoiceStatus("idle") only + // runs inside LiveKitSession.leaveVoice(). handleVoiceJoin can refuse + // for CHANNEL_FULL, VOICE_ERROR, FORBIDDEN, NOT_FOUND, BAD_REQUEST, + // RATE_LIMITED, ALREADY_JOINED, or INTERNAL — this used to only roll + // back CHANNEL_FULL, leaving the sidebar keyed on a channel with no + // LiveKit session for every other refusal. voice_join's error replies + // carry no envelope id to correlate against (Server/ws/voice_join.go + // always answers with buildErrorMsg, never buildErrorMsgWithID), so — + // unlike the pendingSends/pendingReactions correlation above — this + // can't be scoped to "the refusal that answered this specific join"; + // it runs once, ahead of every code-specific branch below, for any + // error that lands while a join is outstanding. A channel *switch* + // refusal hits the same guard: the self voice_leave for the OLD + // channel that precedes it no longer resets voiceStatus (OC-0015 — + // that voice_leave's channel no longer matches the already-updated + // currentChannelId, so it must not tear down the NEW channel's + // optimistic state either), so voiceStatus is still "joining" when + // this error lands and the guard clears it here instead. An + // already-established session is never in "joining", so this never + // touches a live voice call. + if (voiceStore.getState().voiceStatus === "joining") { + leaveVoiceChannel(); + } // Voice capacity refusals. The server owns the limits (voice_max_users / // voice_max_video) and refuses the join or the camera; the client never // pre-blocks the click, because its copy of the participant list can lag @@ -1066,20 +1109,6 @@ export function wireDispatcher( // with an explanation buried in the log. if (payload.code === "CHANNEL_FULL") { showToast(payload.message || "That voice channel is full", "error"); - // The sidebar/widget optimistically writes currentChannelId before - // the server answers (VoiceCallbacks.onVoiceJoin). A first-time join - // refusal earns no voice_leave (there was no previous channel to - // leave), so nothing else clears that optimistic state — the sidebar - // is left keyed on a channel with no LiveKit session. A channel - // *switch* refusal hits the same guard: the self voice_leave for the - // OLD channel that precedes it no longer resets voiceStatus (OC-0015 - // — that voice_leave's channel no longer matches the already-updated - // currentChannelId, so it must not tear down the NEW channel's - // optimistic state either), so voiceStatus is still "joining" when - // this error lands and the guard clears it here instead. - if (voiceStore.getState().voiceStatus === "joining") { - leaveVoiceChannel(); - } return; } if (payload.code === "VIDEO_LIMIT") { diff --git a/Client/tauri-client/src/lib/presence.ts b/Client/tauri-client/src/lib/presence.ts new file mode 100644 index 00000000..74e8ad86 --- /dev/null +++ b/Client/tauri-client/src/lib/presence.ts @@ -0,0 +1,84 @@ +/** + * Shared sender for `presence_update` — the token bucket, the coalescing + * retry, and the local optimistic update all live here exactly once so that + * every producer (auto-idle, the settings Account tab, the UserBar status + * picker) agrees with the server's own limiter instead of each guessing + * independently. + * + * The server enforces a single per-user budget (1 update / 10s, keyed by + * user id — service/channel.go) regardless of which client surface sent the + * frame. A `RateLimiter` created fresh per call site cannot predict that + * shared budget: two producers each starting from a full bucket can both + * believe they have a free token when the server has exactly one, so the + * second frame the server actually receives gets silently dropped + * (ErrRateLimited, no DB write, no broadcast) with nothing left to correct + * it (OC-0210). Callers MUST share one `PresenceSender` — built from one + * `RateLimiter` instance — for the lifetime of a session, the same way + * MainPage.ts's `limiters` are already shared across its chat/typing/ + * reaction/voice producers. + */ + +import type { WsClient } from "./ws"; +import type { RateLimiter } from "./rate-limiter"; +import type { UserStatus } from "./types"; +import { updatePresence } from "@stores/members.store"; +import { authStore } from "@stores/auth.store"; +import { loadUserStatus } from "./userStatus"; + +export interface PresenceSender { + /** + * Send (or, if the shared limiter's window is closed, queue) a presence + * change. Omit `customStatus` to leave whatever custom-status text the + * server already has standing — that is what every caller except an + * explicit custom-status commit wants. + */ + send(status: UserStatus, customStatus?: string): void; + /** Cancel any pending retry. Call on teardown of the owning session. */ + destroy(): void; +} + +/** + * Build a `PresenceSender` bound to one `ws` and one `RateLimiter`. Callers + * that want to share a budget (which is every real caller — see module + * doc) must construct this once and pass the same instance to each + * producer, rather than calling this factory once per producer. + */ +export function createPresenceSender(ws: WsClient, limiter: RateLimiter): PresenceSender { + let retry: ReturnType | null = null; + + function send(status: UserStatus, customStatus?: string): void { + const userId = authStore.getState().user?.id ?? 0; + if (userId !== 0) { + updatePresence(userId, status, customStatus); + } + if (retry !== null) { + clearTimeout(retry); + retry = null; + } + if (limiter.tryConsume()) { + if (customStatus === undefined) { + ws.send({ type: "presence_update", payload: { status } }); + } else { + ws.send({ type: "presence_update", payload: { status, custom_status: customStatus } }); + } + } else { + // The window is still closed from an earlier send (any producer's) — + // retry once it reopens instead of dropping this one silently. + // Re-reads loadUserStatus() at fire time so a burst of calls in + // between coalesces onto a single retry carrying the latest value. + retry = setTimeout(() => { + retry = null; + send(loadUserStatus(), customStatus); + }, limiter.getRemainingMs()); + } + } + + function destroy(): void { + if (retry !== null) { + clearTimeout(retry); + retry = null; + } + } + + return { send, destroy }; +} diff --git a/Client/tauri-client/src/lib/ws.ts b/Client/tauri-client/src/lib/ws.ts index 3a6b627c..cd1854b3 100644 --- a/Client/tauri-client/src/lib/ws.ts +++ b/Client/tauri-client/src/lib/ws.ts @@ -394,18 +394,26 @@ export function createWsClient() { // "trusted" → no action } - async function setupEventListeners(): Promise { - if (tauriListen === null) return; + // Registers this attempt's Tauri event listeners and returns the unsub + // handles it created, WITHOUT touching the shared `eventUnsubs` array. + // Ownership of those handles (splicing them into `eventUnsubs`, or tearing + // them down if this attempt turns out to be stale) is the caller's job — + // see connect(). This keeps a still-in-flight attempt's registrations from + // ever being visible to (and therefore clearable by) another attempt that + // resumes around the same time; see OC-0219. + async function setupEventListeners(): Promise void>> { + if (tauriListen === null) return []; // Capture generation so stale listeners from a previous connect() are no-ops. const gen = wsGeneration; + const ownUnsubs: Array<() => void> = []; // Server messages const unsubMsg = await tauriListen("ws-message", (e) => { if (gen !== wsGeneration) return; handleMessage(e.payload as string); }); - eventUnsubs.push(unsubMsg); + ownUnsubs.push(unsubMsg); // Connection state changes from Rust const unsubState = await tauriListen("ws-state", (e) => { @@ -454,31 +462,36 @@ export function createWsClient() { } } }); - eventUnsubs.push(unsubState); + ownUnsubs.push(unsubState); // Errors const unsubErr = await tauriListen("ws-error", (e) => { if (gen !== wsGeneration) return; log.warn("WebSocket error (proxy)", { error: e.payload }); }); - eventUnsubs.push(unsubErr); + ownUnsubs.push(unsubErr); // Register the global cert-tofu listener on first connect (idempotent). // startCertListener() registers the same listener at app bootstrap so // first-use/mismatch events are also caught during the connect page's health - // checks, before any WS connection exists. + // checks, before any WS connection exists. Deliberately NOT part of + // ownUnsubs/eventUnsubs — it is a singleton for the app's lifetime, not + // scoped to any one connect() attempt. if (certListenerUnsub === null) { certListenerUnsub = await tauriListen("cert-tofu", (e) => { handleCertTofu(e.payload as CertTofuEvent); }); } + + return ownUnsubs; } - function cleanupEventListeners(): void { - for (const unsub of eventUnsubs) { + // Invokes each unsub handle in `unsubs`, tolerating handles that throw or + // return a rejected promise (the Tauri resource may already have been + // invalidated after disconnect). + function unsubscribeAll(unsubs: ReadonlyArray<() => void>): void { + for (const unsub of unsubs) { try { - // Unsub may return a rejected promise if the Tauri resource - // was already invalidated after disconnect — safe to ignore. const result = unsub() as unknown; if (result instanceof Promise) { result.catch((err) => { @@ -489,6 +502,10 @@ export function createWsClient() { // Sync errors also safe to ignore. } } + } + + function cleanupEventListeners(): void { + unsubscribeAll(eventUnsubs); eventUnsubs.length = 0; } @@ -527,17 +544,24 @@ export function createWsClient() { attempt: reconnectAttempt, }); - // Set up event listeners before connecting + // Set up event listeners before connecting. setupEventListeners() hands + // back only the handles THIS attempt registered — they are not spliced + // into the shared `eventUnsubs` until the gen check below confirms this + // attempt is still current. That ownership split is what stops a stale + // attempt's cleanup (just below) from ever reaching a newer attempt's + // listeners, even if the newer attempt finished registering its own + // listeners while this one was still suspended above (OC-0219). cleanupEventListeners(); - await setupEventListeners(); + const ownUnsubs = await setupEventListeners(); if (gen !== wsGeneration) { // Cancelled while awaiting the Tauri IPC round trips inside - // setupEventListeners(). Tear down the listeners this (now-stale) - // attempt just registered instead of leaving them until the next - // connect() happens to clean them up. - cleanupEventListeners(); + // setupEventListeners(). Tear down only the listeners THIS (now-stale) + // attempt just registered — never the shared eventUnsubs array, which + // may already hold a newer attempt's live listeners by now. + unsubscribeAll(ownUnsubs); return; } + eventUnsubs.push(...ownUnsubs); try { await tauriInvoke("ws_connect", { url: wsUrl }); diff --git a/Client/tauri-client/src/pages/MainPage.ts b/Client/tauri-client/src/pages/MainPage.ts index 7859c332..0024578e 100644 --- a/Client/tauri-client/src/pages/MainPage.ts +++ b/Client/tauri-client/src/pages/MainPage.ts @@ -19,8 +19,8 @@ import { initToast, teardownToast, showToast } from "@lib/toast"; import { logout } from "@lib/logout"; import { authStore, clearAuth, updateUser } from "@stores/auth.store"; import { closeSettings, uiStore } from "@stores/ui.store"; -import { updatePresence } from "@stores/members.store"; import { loadUserStatus } from "@lib/userStatus"; +import { createPresenceSender } from "@lib/presence"; import { startAutoIdle, type AutoIdleController } from "@lib/autoIdle"; import { channelsStore, getActiveChannel } from "@stores/channels.store"; import { dmStore, dmDisplayName } from "@stores/dm.store"; @@ -122,6 +122,16 @@ export function createMainPage(options: MainPageOptions): MountableComponent { const limiters = createRateLimiterSet(); + // The one presence_update sender for this session — owns the limiter + // token, the drop-window retry, and the local optimistic update, so + // every producer (auto-idle, the settings Account tab via applyPresence + // below, and the UserBar status picker it's threaded to through + // SidebarArea) shares the exact same budget the server enforces (1 + // update / 10s, keyed by user id — service/channel.go). A limiter created + // per producer instead cannot predict that shared, cross-surface budget + // (OC-0210). + const presenceSender = createPresenceSender(ws, limiters.presence); + let container: Element | null = null; let root: HTMLDivElement | null = null; @@ -147,11 +157,6 @@ export function createMainPage(options: MainPageOptions): MountableComponent { * minutes. Started once the socket is up, torn down with the page. */ let autoIdle: AutoIdleController | null = null; - // Pending retry for a presence_update the 1-per-10s limiter dropped (see - // applyPresence below). Module-scoped so a second dropped frame can - // supersede the first instead of stacking retries. - let presenceRetry: ReturnType | null = null; - // Toast container for user-facing error feedback let toast: ToastContainer | null = null; @@ -196,26 +201,12 @@ export function createMainPage(options: MainPageOptions): MountableComponent { * online fires unthrottled milliseconds after its own idle transition * (autoIdle.ts) — routinely losing the token race. Dropping that frame * silently would leave the server, and everyone else's member list, - * stuck on "idle" with nothing left to correct it. Retry once the window - * reopens instead, re-reading the status at that time so a burst of - * calls in between coalesces onto one retry carrying the latest value. */ + * stuck on "idle" with nothing left to correct it. `presenceSender` + * retries once the window reopens instead, re-reading the status at that + * time so a burst of calls in between coalesces onto one retry carrying + * the latest value — see @lib/presence. */ function applyPresence(status: UserStatus): void { - const userId = getCurrentUserId(); - if (userId !== 0) { - updatePresence(userId, status); - } - if (presenceRetry !== null) { - clearTimeout(presenceRetry); - presenceRetry = null; - } - if (limiters.presence.tryConsume()) { - ws.send({ type: "presence_update", payload: { status } }); - } else { - presenceRetry = setTimeout(() => { - presenceRetry = null; - applyPresence(loadUserStatus()); - }, limiters.presence.getRemainingMs()); - } + presenceSender.send(status); } /** @@ -385,6 +376,7 @@ export function createMainPage(options: MainPageOptions): MountableComponent { ws, api, limiters, + presenceSender, getRoot: () => root, getToast: () => toast, onWatchStream: (userId) => { @@ -809,10 +801,7 @@ export function createMainPage(options: MainPageOptions): MountableComponent { closeActiveLightbox(); autoIdle?.destroy(); autoIdle = null; - if (presenceRetry !== null) { - clearTimeout(presenceRetry); - presenceRetry = null; - } + presenceSender.destroy(); channelCtrl?.destroyChannel(); channelCtrl = null; diff --git a/Client/tauri-client/src/pages/main-page/SidebarArea.ts b/Client/tauri-client/src/pages/main-page/SidebarArea.ts index b137641a..366e8b1b 100644 --- a/Client/tauri-client/src/pages/main-page/SidebarArea.ts +++ b/Client/tauri-client/src/pages/main-page/SidebarArea.ts @@ -10,6 +10,7 @@ import type { MountableComponent } from "@lib/safe-render"; import type { WsClient } from "@lib/ws"; import type { ApiClient } from "@lib/api"; import type { RateLimiterSet } from "@lib/rate-limiter"; +import type { PresenceSender } from "@lib/presence"; import type { ToastContainer } from "@components/Toast"; import { createChannelSidebar } from "@components/ChannelSidebar"; import { createDmSidebar } from "@components/DmSidebar"; @@ -57,6 +58,11 @@ export interface SidebarAreaOptions { readonly ws: WsClient; readonly api: ApiClient; readonly limiters: RateLimiterSet; + /** MainPage's single shared presence sender — threaded to UserBar so its + * status picker shares the same limiter budget and retry as auto-idle + * and the settings Account tab instead of sending straight through `ws` + * (OC-0210; see @lib/presence). */ + readonly presenceSender: PresenceSender; readonly getRoot: () => HTMLDivElement | null; readonly getToast: () => ToastContainer | null; readonly onWatchStream?: (userId: number) => void; @@ -78,7 +84,7 @@ export interface SidebarAreaResult { // --------------------------------------------------------------------------- export function createSidebarArea(opts: SidebarAreaOptions): SidebarAreaResult { - const { ws, api, limiters, getRoot, getToast } = opts; + const { ws, api, limiters, presenceSender, getRoot, getToast } = opts; const children: MountableComponent[] = []; const unsubscribers: Array<() => void> = []; @@ -785,7 +791,7 @@ export function createSidebarArea(opts: SidebarAreaOptions): SidebarAreaResult { // --------------------------------------------------------------------------- const userBarSlot = createElement("div", {}); - const userBar = createUserBar({ onDisconnect: openQuickSwitch, ws }); + const userBar = createUserBar({ onDisconnect: openQuickSwitch, ws, presenceSender }); userBar.mount(userBarSlot); children.push(userBar); sidebarWrapper.appendChild(userBarSlot); diff --git a/Client/tauri-client/tests/unit/dispatcher.test.ts b/Client/tauri-client/tests/unit/dispatcher.test.ts index 45559ae2..071747fe 100644 --- a/Client/tauri-client/tests/unit/dispatcher.test.ts +++ b/Client/tauri-client/tests/unit/dispatcher.test.ts @@ -1378,6 +1378,71 @@ describe("WS Dispatcher", () => { expect(isChannelLoaded(1)).toBe(true); }); + // OC-0203: the refetch above is fired-and-forgotten against whatever + // channel was active when the resync `ready` arrived — but the user can + // switch channels before the HTTP response lands. The continuation must + // re-check that the fetched channel is still the active one before + // writing setMessages, or it resurrects a stale pre-resync snapshot for + // a channel the user already left (and, worse, re-marks it "loaded" so + // MessageController.loadMessages never refetches it again on return). + it("does not write a stale resync snapshot for a channel the user switched away from mid-refetch", async () => { + cleanup(); + const listBlocks = vi.fn().mockResolvedValue({ blocked_user_ids: [] }); + let resolveGetMessages: + ((resp: { messages: MessageResponse[]; has_more: boolean }) => void) | null = null; + const getMessages = vi.fn().mockImplementation( + () => + new Promise<{ messages: MessageResponse[]; has_more: boolean }>((resolve) => { + resolveGetMessages = resolve; + }), + ); + cleanup = wireDispatcher(mock.ws, { listBlocks, getMessages }); + + channelsStore.setState((prev) => ({ ...prev, activeChannelId: 1 })); + setMessages(1, [storedMessage(10)], false); + setMessages(2, [storedMessage(20, 2)], false); + const readyChannels = [ + { id: 1, name: "general", type: "text" as const, category: null, position: 0 }, + { id: 2, name: "other", type: "text" as const, category: null, position: 0 }, + ]; + + // First ready: initial connect. + mock.dispatch("ready", { + channels: readyChannels, + members: [], + voice_states: [], + roles: [], + dm_channels: [], + }); + + // Second ready: a full-ready resync. The refetch for channel 1 (the + // active channel at the time) starts but does not resolve yet. + mock.dispatch("ready", { + channels: readyChannels, + members: [], + voice_states: [], + roles: [], + dm_channels: [], + }); + expect(getMessages).toHaveBeenCalledWith(1, { limit: 50 }); + expect(isChannelLoaded(1)).toBe(false); + + // The user switches to channel 2 before that refetch resolves. + channelsStore.setState((prev) => ({ ...prev, activeChannelId: 2 })); + + // The stale channel-1 refetch now resolves with its pre-resync-era + // snapshot. + resolveGetMessages!({ messages: [storedMessage(900)], has_more: false }); + await Promise.resolve(); + await Promise.resolve(); + + // Channel 1 must stay invalidated — writing the stale snapshot here + // would re-add it to loadedChannels, permanently hiding every message + // posted to it while the user was looking at channel 2. + expect(isChannelLoaded(1)).toBe(false); + expect(getChannelMessages(1)).toEqual([]); + }); + // BUG: invalidateLoadedMessageWindows() ran unconditionally, but the // refetch below it only runs when there's a resolvable active channel // AND api.getMessages exists (api is a Partial<...>, so it may be @@ -3588,6 +3653,33 @@ describe("WS Dispatcher", () => { expect(voiceStore.getState().currentChannelId).toBe(5); expect(voiceStore.getState().voiceStatus).toBe("connected"); }); + + // OC-0224: CHANNEL_FULL is not the only refusal voice_join can get back. + // handleVoiceJoin also answers with VOICE_ERROR (LiveKit down/unconfigured), + // FORBIDDEN (blocked / revoked CONNECT_VOICE), NOT_FOUND, BAD_REQUEST + // (archived channel), RATE_LIMITED, ALREADY_JOINED, and INTERNAL (token + // mint failure) — every one of those used to fall through to the + // catch-all with no rollback, leaving voiceStatus stuck at "joining" + // forever (setVoiceStatus("idle") only ever runs inside + // LiveKitSession.leaveVoice(), and a refused first-time join gets no + // voice_leave to trigger it). + it("rolls back the optimistic join on a voice_join refusal other than CHANNEL_FULL", () => { + voiceStore.setState((prev) => ({ ...prev, currentChannelId: 5, voiceStatus: "joining" })); + + mock.dispatch("error", { code: "VOICE_ERROR", message: "voice is not configured" }); + + expect(voiceStore.getState().currentChannelId).toBeNull(); + expect(voiceStore.getState().voiceStatus).toBe("idle"); + }); + + it("rolls back the optimistic join on FORBIDDEN (revoked CONNECT_VOICE / blocked)", () => { + voiceStore.setState((prev) => ({ ...prev, currentChannelId: 5, voiceStatus: "joining" })); + + mock.dispatch("error", { code: "FORBIDDEN", message: "missing CONNECT_VOICE permission" }); + + expect(voiceStore.getState().currentChannelId).toBeNull(); + expect(voiceStore.getState().voiceStatus).toBe("idle"); + }); }); // A server refusal of voice_camera/voice_screenshare (FORBIDDEN, diff --git a/Client/tauri-client/tests/unit/message-list-optimistic-height-key.test.ts b/Client/tauri-client/tests/unit/message-list-optimistic-height-key.test.ts new file mode 100644 index 00000000..615f9380 --- /dev/null +++ b/Client/tauri-client/tests/unit/message-list-optimistic-height-key.test.ts @@ -0,0 +1,179 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; + +// jsdom does not provide ResizeObserver — stub it so MessageList can mount. +if (typeof globalThis.ResizeObserver === "undefined") { + globalThis.ResizeObserver = class { + observe(): void { + /* noop */ + } + unobserve(): void { + /* noop */ + } + disconnect(): void { + /* noop */ + } + } as unknown as typeof ResizeObserver; +} + +import { createMessageList } from "@components/MessageList"; +import type { MessageListOptions } from "@components/MessageList"; +import { messagesStore } from "@stores/messages.store"; +import { membersStore } from "@stores/members.store"; +import type { Message } from "@stores/messages.store"; + +// Unique markers so the offsetHeight stub below can tell the two unconfirmed +// (id: 0) optimistic rows apart by content, since both currently collide on +// the cache key "msg-0". +const MARK_A = "ZZMARKAAA"; +const MARK_B = "ZZMARKBBB"; +const HEIGHT_A = 111; +const HEIGHT_B = 222; +const HEIGHT_DEFAULT = 10; + +function resetStores(): void { + messagesStore.setState(() => ({ + messagesByChannel: new Map(), + pendingSends: new Map(), + loadedChannels: new Set(), + hasMore: new Map(), + historyLoadState: new Map(), + detachedChannels: new Set(), + })); + membersStore.setState(() => ({ + members: new Map(), + typingUsers: new Map(), + })); +} + +function makeMessage(overrides: Partial & { id: number }): Message { + return { + channelId: 1, + user: { id: 1, username: "Alice", avatar: null }, + content: `Message ${overrides.id}`, + replyTo: null, + attachments: [], + reactions: [], + pinned: false, + editedAt: null, + deleted: false, + timestamp: "2024-01-15T12:00:00Z", + status: "sent", + correlationId: null, + errorCode: null, + ...overrides, + }; +} + +function setMessages(channelId: number, messages: Message[]): void { + messagesStore.setState((prev) => { + const next = new Map(prev.messagesByChannel); + next.set(channelId, messages); + return { ...prev, messagesByChannel: next }; + }); +} + +describe("MessageList height cache key for unconfirmed optimistic rows", () => { + let container: HTMLDivElement; + let msgList: ReturnType; + let options: MessageListOptions; + let offsetHeightDescriptor: PropertyDescriptor | undefined; + + beforeEach(() => { + resetStores(); + container = document.createElement("div"); + document.body.appendChild(container); + options = { + channelId: 1, + channelName: "general", + currentUserId: 1, + onScrollTop: vi.fn(), + onReplyClick: vi.fn(), + onEditClick: vi.fn(), + onDeleteClick: vi.fn(), + onReactionClick: vi.fn(), + onPinClick: vi.fn(), + }; + + // jsdom never lays anything out, so offsetHeight is always 0. Stub it so + // MessageList's real measurement path (measureRendered) has distinct, + // deterministic heights to record for each top-level rendered item: + // the two markers stand in for the two unconfirmed rows, everything + // else (day dividers, confirmed messages) gets a uniform default. + offsetHeightDescriptor = Object.getOwnPropertyDescriptor(HTMLElement.prototype, "offsetHeight"); + Object.defineProperty(HTMLElement.prototype, "offsetHeight", { + configurable: true, + get(this: HTMLElement) { + const text = this.textContent ?? ""; + if (text.includes(MARK_A)) return HEIGHT_A; + if (text.includes(MARK_B)) return HEIGHT_B; + return HEIGHT_DEFAULT; + }, + }); + }); + + afterEach(() => { + msgList.destroy?.(); + container.remove(); + if (offsetHeightDescriptor) { + Object.defineProperty(HTMLElement.prototype, "offsetHeight", offsetHeightDescriptor); + } + }); + + it("keeps two unconfirmed (id: 0) optimistic rows' measured heights distinct across a tree rebuild", () => { + // Two unconfirmed optimistic rows, both id: 0 (as addOptimisticMessage + // produces before confirmSend stamps a real id), distinguished only by + // correlationId and content/measured height. + const rowA = makeMessage({ id: 0, correlationId: "corr-a", content: MARK_A }); + const rowB = makeMessage({ id: 0, correlationId: "corr-b", content: MARK_B }); + const confirmed = Array.from({ length: 40 }, (_, i) => + makeMessage({ id: 1000 + i, content: `Confirmed ${i}` }), + ); + + setMessages(1, [rowA, rowB, ...confirmed]); + msgList = createMessageList(options); + msgList.mount(container); + + // Initial mount positions the render window at the tail (wasAtBottom is + // always true in jsdom), so rowA/rowB are not yet in the DOM and not yet + // measured. Force them into view and measured by jumping to rowA (id 0 + // resolves to the first such row) — OVERSCAN(20) around index 1 (rowA, + // right after the single leading day divider) also covers rowB at index + // 2, so both get real, distinct measurements written to the shared + // height cache: heightCache["msg-0"] ends up holding rowB's height, + // last-measured-wins, per the bug's own description. + expect(msgList.scrollToMessage(0)).toBe(true); + + // Now grow the channel at the tail while the render window is NOT at the + // tail (renderedEnd stopped at rowA's OVERSCAN window, well short of the + // 43-item list). This is a pure suffix extension, so it takes the fast + // "tryAppendMessages" path, which re-seeds a fresh Fenwick tree from the + // (colliding) height cache for every index WITHOUT re-rendering/ + // remeasuring rowA or rowB (they are outside the appended tail and the + // window is not at the tail, so tryAppendMessages skips remeasurement). + const grown = [ + rowA, + rowB, + ...confirmed, + ...Array.from({ length: 5 }, (_, i) => makeMessage({ id: 2000 + i, content: `New ${i}` })), + ]; + setMessages(1, grown); + messagesStore.flush(); + + // A confirmed message that was already measured (index 3..21 window, + // i.e. one of the first 19 "confirmed" rows) sits after both rowA and + // rowB. Its offset-before is the sum of every item ahead of it: the one + // leading day divider (HEIGHT_DEFAULT) + rowA + rowB + N confirmed rows + // at HEIGHT_DEFAULT each. If the cache collision corrupted rowA's slot + // in the tree, that offset is inflated by (HEIGHT_B - HEIGHT_A). + const target = confirmed[5]!; // id 1005, virtual index 3 + 5 = 8 + expect(msgList.scrollToMessage(target.id)).toBe(true); + + const root = container.querySelector(".messages-container") as HTMLDivElement; + const expectedCorrect = HEIGHT_DEFAULT + HEIGHT_A + HEIGHT_B + 5 * HEIGHT_DEFAULT; + + // This is the assertion the bug breaks: with the shared "msg-0" cache + // key, rowA's tree slot gets re-seeded from rowB's cached height instead + // of its own, inflating the offset by (HEIGHT_B - HEIGHT_A) = 111. + expect(root.scrollTop).toBe(expectedCorrect); + }); +}); diff --git a/Client/tauri-client/tests/unit/settings-helpers.test.ts b/Client/tauri-client/tests/unit/settings-helpers.test.ts index 2c06efb2..37de7cbd 100644 --- a/Client/tauri-client/tests/unit/settings-helpers.test.ts +++ b/Client/tauri-client/tests/unit/settings-helpers.test.ts @@ -127,6 +127,47 @@ describe("settings/helpers", () => { expect(root.style.getPropertyValue("--bg-primary")).toBe("#ffffff"); }); + it("switching away from light clears the light-only tokens instead of leaving them stuck on ", () => { + // OC-0201: applyTheme only ever *sets* the keys present in the new + // theme and never clears keys the previous theme set. THEMES.light + // defines ~24 custom properties while dark/midnight/neon-glow define + // only 4, so switching light -> dark must leave zero light-only + // tokens behind on document.documentElement. + applyTheme("light"); + const root = document.documentElement; + expect(root.style.getPropertyValue("--bg-input")).toBe("#ebedef"); + + applyTheme("dark"); + + // The 4 keys dark actually owns must reflect dark's values. + expect(root.style.getPropertyValue("--bg-primary")).toBe("#313338"); + expect(root.style.getPropertyValue("--text-normal")).toBe("#dbdee1"); + + // Every light-only token must be cleared, not left stuck at its + // light-mode value (which would outrank the :root CSS default via + // inline-style specificity). + const lightOnlyKeys = Object.keys(THEMES.light).filter((k) => !(k in THEMES.dark)); + expect(lightOnlyKeys.length).toBeGreaterThan(0); + for (const key of lightOnlyKeys) { + expect( + root.style.getPropertyValue(key), + `${key} must be cleared after switching to dark`, + ).toBe(""); + } + }); + + it("does not clear unrelated inline custom properties like --accent or --font-size", () => { + const root = document.documentElement; + root.style.setProperty("--accent", "#00c8ff"); + root.style.setProperty("--font-size", "18px"); + + applyTheme("light"); + applyTheme("dark"); + + expect(root.style.getPropertyValue("--accent")).toBe("#00c8ff"); + expect(root.style.getPropertyValue("--font-size")).toBe("18px"); + }); + it("light theme overrides the dark-mode input/border/interactive tokens so composer and form fields aren't dark-on-dark", () => { // OC-0043: the light theme only overrode 4 of ~45 tokens. --bg-input // (used by .message-input-box, .msg-textarea, .form-input, .reply-bar-inner) diff --git a/Client/tauri-client/tests/unit/sidebar-area.test.ts b/Client/tauri-client/tests/unit/sidebar-area.test.ts index 8d11b2d1..a16395c7 100644 --- a/Client/tauri-client/tests/unit/sidebar-area.test.ts +++ b/Client/tauri-client/tests/unit/sidebar-area.test.ts @@ -308,6 +308,10 @@ function defaultOpts(): SidebarAreaOptions { voice: { tryConsume: vi.fn().mockReturnValue(true) }, voiceVideo: { tryConsume: vi.fn().mockReturnValue(true) }, } as unknown as SidebarAreaOptions["limiters"], + presenceSender: { + send: vi.fn(), + destroy: vi.fn(), + } as unknown as SidebarAreaOptions["presenceSender"], getRoot: vi.fn().mockReturnValue(document.createElement("div")), getToast: vi.fn().mockReturnValue({ show: vi.fn() }), }; diff --git a/Client/tauri-client/tests/unit/status-picker-userbar.test.ts b/Client/tauri-client/tests/unit/status-picker-userbar.test.ts index 2432759d..a3cfab2d 100644 --- a/Client/tauri-client/tests/unit/status-picker-userbar.test.ts +++ b/Client/tauri-client/tests/unit/status-picker-userbar.test.ts @@ -9,6 +9,8 @@ import { uiStore, setConnectionStatus } from "@stores/ui.store"; import { createUserBar } from "@components/UserBar"; import { loadUserStatus, saveUserStatus } from "@lib/userStatus"; +import { createPresenceSender } from "@lib/presence"; +import { createPresenceLimiter } from "@lib/rate-limiter"; import type { WsClient } from "@lib/ws"; function setAuthState(user: { username: string } | null, isAuthenticated: boolean): void { @@ -21,6 +23,15 @@ function setAuthState(user: { username: string } | null, isAuthenticated: boolea })); } +/** A ws plus a real (unconsumed) presence sender bound to it — what + * SidebarArea actually threads into UserBar in production. */ +function userBarOptsWithPresence(ws: WsClient): { + ws: WsClient; + presenceSender: ReturnType; +} { + return { ws, presenceSender: createPresenceSender(ws, createPresenceLimiter()) }; +} + function createMockWs(state: "connected" | "disconnected" = "connected"): WsClient { let currentState = state; const stateListeners = new Set<(s: string) => void>(); @@ -90,7 +101,7 @@ describe("StatusPicker wired to UserBar", () => { it("selecting a status sends presence_update WS message", () => { setAuthState({ username: "alice" }, true); const ws = createMockWs("connected"); - comp = createUserBar({ ws }); + comp = createUserBar(userBarOptsWithPresence(ws)); comp.mount(container); // Open picker @@ -108,6 +119,50 @@ describe("StatusPicker wired to UserBar", () => { expect(sentMsg.payload.status).toBe("idle"); }); + // OC-0210: every other presence producer (auto-idle, the settings Account + // tab) shares one PresenceSender/RateLimiter through MainPage so a frame + // the server's 1-per-10s presence limiter (service/channel.go) drops gets + // retried instead of lost. The UserBar picker must go through that same + // shared sender, not straight to ws.send, or a token another producer just + // spent makes the server silently drop the picker's frame with nothing to + // correct it. + it("queues (does not drop) a status change when the shared presence limiter's window is already closed", () => { + setAuthState({ username: "alice" }, true); + vi.useFakeTimers(); + try { + const ws = createMockWs("connected"); + // The same PresenceSender instance MainPage threads to every producer + // — pre-spend its single token exactly as auto-idle or the settings + // tab would moments before the user opens the picker. + const presenceSender = createPresenceSender(ws, createPresenceLimiter()); + presenceSender.send("idle"); + expect(ws.send).toHaveBeenCalledOnce(); + (ws.send as ReturnType).mockClear(); + + comp = createUserBar({ ws, presenceSender }); + comp.mount(container); + + const dot = container.querySelector(".status-picker-dot") as HTMLElement; + dot.click(); + const options = container.querySelectorAll(".status-picker-option"); + (options[0] as HTMLElement).click(); // "Online" + + // The server's window is still closed — the frame must be queued, not + // sent straight down the socket and lost if it's rejected. + expect(ws.send).not.toHaveBeenCalled(); + + // Once the window reopens, the queued change must still go out. + vi.advanceTimersByTime(10_000); + + expect(ws.send).toHaveBeenCalledOnce(); + const sentMsg = (ws.send as ReturnType).mock.calls[0]![0]; + expect(sentMsg.type).toBe("presence_update"); + expect(sentMsg.payload.status).toBe("online"); + } finally { + vi.useRealTimers(); + } + }); + it("status picker is disabled when WS is disconnected", () => { setAuthState({ username: "alice" }, true); setConnectionStatus("disconnected"); @@ -124,7 +179,7 @@ describe("StatusPicker wired to UserBar", () => { it("status picker reacts to a connection status change through the store", async () => { setAuthState({ username: "alice" }, true); const ws = createMockWs("connected"); - comp = createUserBar({ ws }); + comp = createUserBar(userBarOptsWithPresence(ws)); comp.mount(container); const wrap = container.querySelector("[data-testid='status-picker-wrap']") as HTMLElement; diff --git a/Client/tauri-client/tests/unit/update-notifier.test.ts b/Client/tauri-client/tests/unit/update-notifier.test.ts index 45a22bd3..d45a4500 100644 --- a/Client/tauri-client/tests/unit/update-notifier.test.ts +++ b/Client/tauri-client/tests/unit/update-notifier.test.ts @@ -136,3 +136,45 @@ describe("createUpdateNotifier download progress", () => { expect(unhandled).not.toHaveBeenCalled(); }); }); + +// --------------------------------------------------------------------------- +// Deferred check timer lifecycle +// --------------------------------------------------------------------------- + +describe("createUpdateNotifier deferred check timer", () => { + let host: HTMLElement; + + beforeEach(() => { + vi.useFakeTimers(); + vi.clearAllMocks(); + mockCheckForUpdate.mockResolvedValue({ available: false, version: null, body: null }); + host = document.createElement("div"); + document.body.appendChild(host); + }); + + afterEach(() => { + vi.useRealTimers(); + host.remove(); + }); + + it("does not check for updates when destroyed before the delayed check fires", async () => { + const notifier = createUpdateNotifier({ serverUrl: "https://s.example" }); + notifier.mount(host); + + // Page swap / logout tears the component down inside the 3s window. + notifier.destroy?.(); + await vi.advanceTimersByTimeAsync(3000); + + expect(mockCheckForUpdate).not.toHaveBeenCalled(); + }); + + it("still checks for updates when the component stays mounted", async () => { + const notifier = createUpdateNotifier({ serverUrl: "https://s.example" }); + notifier.mount(host); + + await vi.advanceTimersByTimeAsync(3000); + + expect(mockCheckForUpdate).toHaveBeenCalledWith("https://s.example"); + notifier.destroy?.(); + }); +}); diff --git a/Client/tauri-client/tests/unit/voice-widget.test.ts b/Client/tauri-client/tests/unit/voice-widget.test.ts index 9809cc77..d3ddfd9d 100644 --- a/Client/tauri-client/tests/unit/voice-widget.test.ts +++ b/Client/tauri-client/tests/unit/voice-widget.test.ts @@ -849,4 +849,55 @@ describe("VoiceWidget", () => { widget.destroy?.(); }); + + // OC-0225: the Grant-Microphone retry's `.finally` used to hardcode + // `grantMicBtn.disabled = false`, undoing updateFrozen's socket-down + // freeze if the WS socket dropped while the permission request was in + // flight. + it("keeps 'Grant Microphone' frozen if the WS socket drops while a mic request is in flight", async () => { + setVoiceChannel(1, []); + voiceStore.setState((prev) => ({ ...prev, listenOnly: true })); + + let resolveMic: () => void = () => {}; + mockRetryMicPermission.mockImplementationOnce( + () => + new Promise((resolve) => { + resolveMic = resolve; + }), + ); + + const widget = createVoiceWidget({ + onDisconnect: vi.fn(), + onMuteToggle: vi.fn(), + onDeafenToggle: vi.fn(), + onCameraToggle: vi.fn(), + onScreenshareToggle: vi.fn(), + }); + widget.mount(container); + + const grantBtn = container.querySelector(".vw-grant-mic") as HTMLButtonElement; + grantBtn.click(); + expect(grantBtn.disabled).toBe(true); + + // WS socket drops while the permission request (OS/browser prompt) is + // still pending. + setConnectionStatus("reconnecting"); + uiStore.flush(); + expect(grantBtn.disabled).toBe(true); + expect(grantBtn.title).toBe("Reconnecting…"); + + // Permission request settles (retryMicPermission always resolves, even + // on a denied prompt, per its internal try/catch). + resolveMic(); + await vi.waitFor(() => { + expect(grantBtn.textContent).toBe("Grant Microphone"); + }); + + // The socket is still down: the button must stay frozen with the + // reconnecting reason, not silently re-enabled. + expect(grantBtn.disabled).toBe(true); + expect(grantBtn.title).toBe("Reconnecting…"); + + widget.destroy?.(); + }); }); diff --git a/Client/tauri-client/tests/unit/ws-lifecycle.test.ts b/Client/tauri-client/tests/unit/ws-lifecycle.test.ts index c46580b4..8b53643f 100644 --- a/Client/tauri-client/tests/unit/ws-lifecycle.test.ts +++ b/Client/tauri-client/tests/unit/ws-lifecycle.test.ts @@ -743,6 +743,103 @@ describe("disconnect() cancelling an in-flight connect()", () => { }); }); +describe("overlapping connect() attempts (OC-0219)", () => { + let client: ReturnType; + let originalMockListenImpl: (typeof mockListen)["getMockImplementation"] extends () => infer R + ? R + : never; + + beforeEach(() => { + vi.useFakeTimers(); + mockInvoke.mockReset(); + mockInvoke.mockResolvedValue(undefined); + originalMockListenImpl = mockListen.getMockImplementation()!; + mockListen.mockClear(); + eventHandlers.clear(); + client = createWsClient(); + }); + + afterEach(() => { + mockListen.mockImplementation(originalMockListenImpl!); + client.disconnect(); + vi.useRealTimers(); + }); + + // OC-0219: eventUnsubs is a single client-scoped array shared by every + // connect() attempt. If a stale attempt A resumes inside setupEventListeners() + // after a newer attempt B has already registered its own listeners into that + // same shared array, A's stale-branch cleanup must tear down only the + // listeners A itself just registered — not B's. Otherwise B's connection + // opens with nobody listening: no auth frame is ever sent, ws-state "closed" + // is never observed either, and the socket wedges with no reconnect. + it("does not tear down a newer connect()'s listeners when a stale attempt's setupEventListeners resumes later", async () => { + let releaseFirstMsgListen: (() => void) | null = null; + let firstMsgListenSeen = false; + + mockListen.mockImplementation( + async (event: string, handler: (e: { payload: unknown }) => void) => { + if (event === "ws-message" && !firstMsgListenSeen) { + firstMsgListenSeen = true; + // Pause attempt A here — mirrors A being suspended inside + // setupEventListeners()'s Tauri IPC round trips while a newer + // connect() attempt B runs all the way to completion. + await new Promise((resolve) => { + releaseFirstMsgListen = resolve; + }); + } + return originalMockListenImpl!(event, handler); + }, + ); + + // Attempt A: suspends inside its first tauriListen("ws-message", ...) call. + client.connect({ host: "localhost:8443", token: "tA" }); + await vi.advanceTimersByTimeAsync(10); + expect(releaseFirstMsgListen).not.toBeNull(); + + // Attempt B supersedes A (e.g. a reconnect timer firing alongside a + // fresh connect()) and runs to completion — registers its own listeners + // and calls ws_connect — while A is still suspended. + client.connect({ host: "localhost:8443", token: "tB" }); + await vi.advanceTimersByTimeAsync(10); + + // Resume A. It notices it is stale (gen mismatch) and tears down + // listeners — this must remove only the listeners it just registered, + // not B's live ones. + releaseFirstMsgListen!(); + await vi.advanceTimersByTimeAsync(10); + + // B's underlying (mock) connection now reports open. If A's stale + // cleanup wiped B's ws-state listener, nothing observes this and the + // auth frame is never sent. + emitTauriEvent("ws-state", "open"); + await vi.advanceTimersByTimeAsync(10); + + const authSends = mockInvoke.mock.calls.filter( + (c) => + c[0] === "ws_send" && + typeof c[1]?.message === "string" && + (c[1].message as string).includes('"type":"auth"'), + ); + expect(authSends.length).toBeGreaterThanOrEqual(1); + + // Complete the handshake and confirm B reaches "connected" — proof its + // ws-message listener also survived A's stale cleanup. + emitTauriEvent( + "ws-message", + JSON.stringify({ + type: "auth_ok", + payload: { + user: { id: 1, username: "b", avatar: null, role: "admin" }, + server_name: "S", + motd: "", + }, + }), + ); + + expect(client.getState()).toBe("connected"); + }); +}); + describe("heartbeat proxyOpen guard", () => { let client: ReturnType; diff --git a/Server/admin/channels_archive_voice_test.go b/Server/admin/channels_archive_voice_test.go index fa65639d..cfdfaa18 100644 --- a/Server/admin/channels_archive_voice_test.go +++ b/Server/admin/channels_archive_voice_test.go @@ -48,7 +48,11 @@ func TestAdminAPI_PatchChannel_UnarchiveDoesNotCleanVoice(t *testing.T) { token := createAdminUser(t, database) chID, _ := database.AdminCreateChannel(context.Background(), "unarchive-voice", "voice", "", "", 0) - if err := database.AdminUpdateChannel(context.Background(), chID, db.ChannelUpdate{Archived: true}); err != nil { + // AdminUpdateChannel replaces the full row, so the seed must carry the + // name along with Archived: true — leaving it zero-valued would blank the + // channel's name directly at the DB layer, bypassing the handler's own + // validation and leaving the row in a state the HTTP surface never allows. + if err := database.AdminUpdateChannel(context.Background(), chID, db.ChannelUpdate{Name: "unarchive-voice", Archived: true}); err != nil { t.Fatalf("seed archived channel: %v", err) } diff --git a/Server/admin/export_test.go b/Server/admin/export_test.go index caafa546..84098395 100644 --- a/Server/admin/export_test.go +++ b/Server/admin/export_test.go @@ -1,10 +1,12 @@ package admin import ( + "errors" "sync/atomic" "time" "github.com/owncord/server/auth" + "github.com/owncord/server/db" ) // CaptureSetupLimiter installs h so the next NewAdminAPI call reports the @@ -34,6 +36,29 @@ func SetSetupLimiterReapTiming(interval, maxWindow time.Duration) (restore func( // at a temp dir. Lives here so it stays out of the production binary. func SetBackupBaseDir(dir string) { backupBaseDir = dir } +// StubCloseError makes the next handleRestoreBackup call's database.Close() +// return err instead of actually closing the pools, so tests can exercise the +// Close-failure branch without a genuine driver-level close error (see +// dbCloser's doc comment for why that's not otherwise reachable in a test). +func StubCloseError(msg string) (restore func()) { + closeMu.Lock() + prev := dbCloser + dbCloser = func(*db.DB) error { return errors.New(msg) } + closeMu.Unlock() + return func() { + closeMu.Lock() + dbCloser = prev + closeMu.Unlock() + } +} + +// ApplyStagedUpdate exposes applyStagedUpdate (the on-disk swap + respawn +// logic behind POST /updates/apply's background goroutine) so tests can drive +// its abort paths directly with fake filesystem paths, instead of exercising +// the full HTTP handler — which resolves exePath via os.Executable() and +// would rename/replace the running test binary itself. +var ApplyStagedUpdate = applyStagedUpdate + // StubRestart replaces the process-restart hook for the duration of a test and // returns a func reporting whether a restart was requested. Without this the // restore handler would respawn and os.Exit the test binary. diff --git a/Server/admin/handlers_backup.go b/Server/admin/handlers_backup.go index 051be108..6a973816 100644 --- a/Server/admin/handlers_backup.go +++ b/Server/admin/handlers_backup.go @@ -237,9 +237,18 @@ func handleRestoreBackup(database *db.DB, hub HubBroadcaster) http.Handler { slog.Warn("database restored from backup — closing DB", "actor_id", actor, "backup", name) - if err := database.Close(); err != nil { - slog.Error("failed to close database before restore", "err", err) + if err := closeDatabase(database); err != nil { + // database.Close() closes the writer and reader pools regardless of + // the error it returns (Server/db/db.go), so this process cannot + // serve anything more either way — every other failure path below + // (copyFile failing, and the success path itself) respawns for + // exactly that reason. The live database file is still intact here + // (copyFile hasn't run yet), so the respawned process comes back on + // the pre-restore data rather than leaving clients pinned on + // "Reconnecting..." against a process that never actually restarts. + slog.Error("failed to close database before restore — restarting anyway, DB pools are closed either way", "err", err) writeErr(w, http.StatusInternalServerError, "INTERNAL_ERROR", "failed to close database") + go requestRestart("backup_restore_close_failed") return } @@ -290,6 +299,25 @@ var ( restartSelf = restartProcess ) +// dbCloser is swappable in tests to simulate database.Close() returning an +// error. modernc.org/sqlite's sqlite3_close_v2 essentially never fails on a +// normally-open connection, so there is no portable way to provoke a genuine +// Close() error from a real driver in a unit test; this seam lets tests +// exercise that branch directly. Guarded like restartSelf, for the same +// reason (swap happens on the test goroutine, read on the handler's). +var ( + closeMu sync.Mutex + dbCloser = func(database *db.DB) error { return database.Close() } +) + +// closeDatabase invokes the current close hook. +func closeDatabase(database *db.DB) error { + closeMu.Lock() + fn := dbCloser + closeMu.Unlock() + return fn(database) +} + // requestRestart invokes the current restart hook. func requestRestart(reason string) { restartMu.Lock() diff --git a/Server/admin/handlers_backup_test.go b/Server/admin/handlers_backup_test.go index 35296354..de48e7dc 100644 --- a/Server/admin/handlers_backup_test.go +++ b/Server/admin/handlers_backup_test.go @@ -421,6 +421,53 @@ func TestHandleRestoreBackup_RollsBackWhenCopyFails(t *testing.T) { } } +// TestHandleRestoreBackup_RestartsWhenCloseFails verifies OC-0209: a failed +// database.Close() must still schedule a process restart. database.Close() +// closes the writer and reader pools regardless of the error it returns +// (Server/db/db.go), and the server_restart broadcast already went out to +// every client before Close() is even called — so a process that answers 500 +// here without respawning leaves clients pinned on "Reconnecting..." forever +// while the process quietly keeps failing every request with a closed DB. +func TestHandleRestoreBackup_RestartsWhenCloseFails(t *testing.T) { + tmpDir := chdirTemp(t) + database := openAdminTestDB(t) + handler := admin.NewAdminAPI(database, "1.0.0", &mockHub{}, nil, nil, nil, nil, newTestModService(database), newTestRoleService(database)) + token := createAdminUser(t, database) + + backupDir := filepath.Join(tmpDir, "data", "backups") + if err := os.MkdirAll(backupDir, 0o750); err != nil { + t.Fatalf("MkdirAll backups: %v", err) + } + dbPath := filepath.Join(tmpDir, "data", "chatserver.db") + if err := os.WriteFile(dbPath, []byte("original live contents"), 0o600); err != nil { + t.Fatalf("WriteFile live db: %v", err) + } + backupName := "chatserver_20240103_120000.db" + if err := os.WriteFile(filepath.Join(backupDir, backupName), []byte("replacement contents"), 0o644); err != nil { + t.Fatalf("WriteFile backup: %v", err) + } + + restarted, restoreRestartHook := admin.StubRestart() + defer restoreRestartHook() + restoreCloseHook := admin.StubCloseError("simulated close failure") + defer restoreCloseHook() + + w := doRequest(t, handler, http.MethodPost, "/backups/"+backupName+"/restore", token, nil) + + if w.Code != http.StatusInternalServerError { + t.Fatalf("status = %d, want 500; body: %s", w.Code, w.Body.String()) + } + + deadline := time.Now().Add(2 * time.Second) + for !restarted() && time.Now().Before(deadline) { + time.Sleep(10 * time.Millisecond) + } + if !restarted() { + t.Error("a failed database.Close() did not request a process restart, " + + "leaving a live server answering requests against closed DB pools") + } +} + // TestHandleRestoreBackup_AbortsWithoutSafetyBackup verifies the restore fails // closed when the pre-restore backup can't be written: the panel promises that // safety copy, and overwriting the live database without one is unrecoverable. diff --git a/Server/admin/handlers_channels.go b/Server/admin/handlers_channels.go index d9789bec..3edb8bef 100644 --- a/Server/admin/handlers_channels.go +++ b/Server/admin/handlers_channels.go @@ -170,6 +170,8 @@ type updateChannelRequest struct { // caller sending -1 meant something, and silently storing 0 would hide it. func (r updateChannelRequest) validate() string { switch { + case strings.TrimSpace(r.Name) == "": + return "name is required" case r.SlowMode < 0 || r.SlowMode > maxSlowModeSeconds: return fmt.Sprintf("slow_mode must be between 0 and %d seconds", maxSlowModeSeconds) case r.VoiceMaxUsers < 0 || r.VoiceMaxUsers > maxVoiceLimit: diff --git a/Server/admin/handlers_channels_test.go b/Server/admin/handlers_channels_test.go index f7e21fc1..dc110ddb 100644 --- a/Server/admin/handlers_channels_test.go +++ b/Server/admin/handlers_channels_test.go @@ -255,6 +255,48 @@ func TestPatchChannel_RejectsOutOfRangeValues(t *testing.T) { } } +// PATCH must reject a blank name the same way POST does (handleCreateChannel, +// line 104): updateChannelRequest.validate() only bounded the numeric fields, +// so a whitespace-only name could slip through PATCH and leave the channel +// unidentifiable in every client's sidebar. +func TestPatchChannel_RejectsEmptyName(t *testing.T) { + cases := []struct { + name string + value string + }{ + {"empty string", ""}, + {"whitespace only", " "}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + handler, token, database := newChannelTestAPI(t) + id := newChannel(t, handler, token, "general", "text") + + w := doRequest(t, handler, http.MethodPatch, fmt.Sprintf("/channels/%d", id), token, map[string]any{ + "name": tc.value, + }) + if w.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400; body: %s", w.Code, w.Body.String()) + } + var resp map[string]string + if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { + t.Fatalf("unmarshal error body: %v", err) + } + if resp["error"] != "INVALID_INPUT" { + t.Errorf("error code = %q, want INVALID_INPUT", resp["error"]) + } + + ch, err := database.GetChannel(context.Background(), id) + if err != nil || ch == nil { + t.Fatalf("GetChannel after refused patch: ch=%v err=%v", ch, err) + } + if ch.Name != "general" { + t.Errorf("channel name after refused patch = %q, want unchanged %q", ch.Name, "general") + } + }) + } +} + // The boundary values themselves are legal — an off-by-one in validate() that // refused 21600 or 99 would silently cap what the clients offer. func TestPatchChannel_AcceptsBoundaryValues(t *testing.T) { diff --git a/Server/admin/handlers_users.go b/Server/admin/handlers_users.go index 32fad417..11cb493e 100644 --- a/Server/admin/handlers_users.go +++ b/Server/admin/handlers_users.go @@ -123,11 +123,34 @@ func handlePatchUser(database *db.DB, hub HubBroadcaster, permInvalidator Permis return } + // Authorize the role change before applying anything else. Without + // this pre-flight, a PATCH combining banned + role_id would commit + // and broadcast the ban first and only then attempt the role change: + // if that role change was then refused (missing MANAGE_ROLES, or the + // new role outranks the actor), the handler reported the whole + // request as failed while the target was in fact banned, audited, + // and already dropped from every connected client's member list + // (OC-0215). Running every ChangeUserRole precondition up front, + // before either mutation lands, keeps the PATCH all-or-nothing from + // the caller's perspective. + if req.RoleID != nil { + if mod == nil { + // Fail closed rather than fall back to an unchecked UPDATE. + writeErr(w, http.StatusInternalServerError, "INTERNAL_ERROR", "moderation service unavailable") + return + } + if _, _, _, err := mod.AuthorizeRoleChange(r.Context(), actor, id, *req.RoleID); err != nil { + writeModerationErr(w, err) + return + } + } + // Ban/unban first: it routes through ModerationService, which enforces // BAN_MEMBERS + role hierarchy (the admin-auth perimeter alone does // not — any admin-panel actor could previously ban the owner). The - // service also audits and refuses before the role change runs, so a - // rejected ban never leaves a half-applied PATCH behind. + // role change, if requested, was already authorized above, so a ban + // committing here cannot be followed by a refused role change leaving + // a half-applied PATCH behind. if req.Banned != nil { if mod == nil { // Fail closed rather than fall back to an unchecked UPDATE. @@ -173,10 +196,12 @@ func handlePatchUser(database *db.DB, hub HubBroadcaster, permInvalidator Permis } if req.RoleID != nil { - // Routed through ModerationService, which enforces MANAGE_ROLES, - // the actor-outranks-target rule, and the assign-below-own-rank - // rule (without it any admin could promote anyone to Owner), and - // writes the audit row. + // Routed through ModerationService, which re-runs the same + // MANAGE_ROLES, actor-outranks-target, and assign-below-own-rank + // checks the AuthorizeRoleChange pre-flight above already passed + // (a second pass, not a redundant one: it catches anything that + // changed in the window between the pre-flight and here, e.g. a + // concurrent role delete), then commits and writes the audit row. if mod == nil { // Fail closed rather than fall back to an unchecked UPDATE. writeErr(w, http.StatusInternalServerError, "INTERNAL_ERROR", "moderation service unavailable") diff --git a/Server/admin/handlers_users_atomic_test.go b/Server/admin/handlers_users_atomic_test.go new file mode 100644 index 00000000..1b651688 --- /dev/null +++ b/Server/admin/handlers_users_atomic_test.go @@ -0,0 +1,55 @@ +package admin_test + +import ( + "context" + "net/http" + "testing" + + "github.com/owncord/server/admin" +) + +// A PATCH combining banned + role_id must be all-or-nothing: if the role +// change is refused, the ban must not have been committed either. Before the +// fix, handlePatchUser applied and broadcast the ban first and only then +// attempted the role change, so a moderator with BAN_MEMBERS but not +// MANAGE_ROLES could send one PATCH that the API reports as a 403 failure +// while the target ends up banned, audited, and dropped from every connected +// client's member list anyway (OC-0215). +func TestAdminAPI_PatchUser_RefusedRoleChangeDoesNotLeaveBanCommitted(t *testing.T) { + database := openAdminTestDB(t) + hub := &mockHub{} + handler := admin.NewAdminAPI(database, "1.0.0", hub, nil, nil, nil, nil, newTestModService(database), newTestRoleService(database)) + + // Moderator: BAN_MEMBERS (and everything below bit 20), but not + // MANAGE_ROLES (bit 24) — moderatorMask is perm_gates_test.go's constant + // for exactly this shape, seeded at position 60 (below Admin's 80). + _, modToken := createRoleUser(t, database, 10, "Moderator", moderatorMask, 60, "moduser") + + targetUID, err := database.CreateUser(context.Background(), "atomictarget", "hash", 3) // Member, position 40 + if err != nil { + t.Fatalf("CreateUser target: %v", err) + } + + w := doRequest(t, handler, http.MethodPatch, "/users/"+itoa(targetUID), modToken, map[string]any{ + "banned": true, + "ban_reason": "spam", + "role_id": 2, // Admin role — moderator lacks MANAGE_ROLES to grant it + }) + if w.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403 (moderator lacks MANAGE_ROLES); body: %s", w.Code, w.Body.String()) + } + + target, err := database.GetUserByID(context.Background(), targetUID) + if err != nil { + t.Fatalf("GetUserByID: %v", err) + } + if target == nil { + t.Fatalf("target user disappeared") + } + if target.Banned { + t.Fatalf("target.Banned = true, want false: the refused role change must not leave the ban committed") + } + if len(hub.memberBanIDs) != 0 { + t.Fatalf("BroadcastMemberBan calls = %v, want none: no ban should have been broadcast for a request the API reported as failed", hub.memberBanIDs) + } +} diff --git a/Server/admin/update_handlers.go b/Server/admin/update_handlers.go index 9f72c348..95c021b0 100644 --- a/Server/admin/update_handlers.go +++ b/Server/admin/update_handlers.go @@ -114,56 +114,89 @@ func handleApplyUpdate(u *updater.Updater, hub HubBroadcaster, _ string) http.Ha hub.BroadcastServerRestart("update", 5) } time.Sleep(5 * time.Second) - - // TOCTOU guard: open the staged binary once, verify its hash - // through that handle, and commit (rename) that exact file. - // Commit fails if the path was swapped after verification, so - // the bytes verified are the bytes spawned. - staged, err := updater.OpenVerifiedBinary(newPath, stagedHash) - if err != nil { - slog.Error("update: staged binary re-verification failed, aborting update", "error", err) - return + if applyStagedUpdate(hub, exePath, oldPath, newPath, stagedHash) { + // Every deferred cleanup inside applyStagedUpdate has run by + // now, which is why the exit lives out here. + os.Exit(0) // fallback if the SIGTERM handler didn't exit } - defer staged.Close() //nolint:errcheck - - // Rename: current -> .old, verified staged binary -> current - _ = os.Remove(oldPath) // remove any stale .old - if err := os.Rename(exePath, oldPath); err != nil { - slog.Error("update: rename current to old failed", "error", err) - return - } - if err := staged.Commit(exePath); err != nil { - slog.Error("update: committing staged binary failed, restoring original binary", "error", err) - // Whatever is at exePath now (if anything) is not the verified - // binary; restoring .old replaces it. - if restoreErr := os.Rename(oldPath, exePath); restoreErr != nil { - slog.Error("update: CRITICAL — recovery rename also failed, server binary may be missing", - "restore_error", restoreErr, "original_error", err, - "old_path", oldPath, "exe_path", exePath) - if hub != nil { - hub.BroadcastServerRestart("update_failed", 0) - } - } - return - } - - // Spawn new process. - if err := updater.SpawnDetached(exePath, os.Args[1:]); err != nil { - slog.Error("update: spawn new process failed", "error", err) - return - } - - // Signal the process to shut down gracefully before exiting. - // We use SIGTERM on Unix to trigger the graceful shutdown handler - // in main.go. On Windows, os.Exit is unavoidable because the - // process must release its file lock on the binary. - slog.Info("update: new process spawned, shutting down current process") - if p, err := os.FindProcess(os.Getpid()); err == nil { - _ = p.Signal(syscall.SIGTERM) - // Give graceful shutdown a few seconds before force-killing. - time.Sleep(10 * time.Second) - } - os.Exit(0) // fallback if SIGTERM handler didn't exit }() }) } + +// applyStagedUpdate performs the on-disk swap (verified staged binary -> +// exePath) and spawns the replacement process. The caller has already +// broadcast "restarting in 5s" to every connected client before invoking +// this, so every return path that does NOT end in a successful respawn must +// correct that promise — otherwise the client's restart banner counts down +// to a permanent "Reconnecting..." over a connection that never actually +// dropped (OC-0226). The deferred broadcast below covers all such paths +// (verification failure, rename failure, commit failure, spawn failure) with +// one guard instead of one broadcast per failure branch; it is cancelled by +// setting restarting=true immediately before the process commits to +// respawning. +// It reports whether the process is committed to exiting for the replacement. +// The exit itself belongs to the caller: calling os.Exit here would skip both +// deferred cleanups below (the staged-file handle and the corrective +// broadcast), and on Windows releasing that handle is the very thing the +// restart is for. +func applyStagedUpdate(hub HubBroadcaster, exePath, oldPath, newPath, stagedHash string) bool { + restarting := false + defer func() { + if !restarting && hub != nil { + hub.BroadcastServerRestart("update_aborted", 0) + } + }() + + // TOCTOU guard: open the staged binary once, verify its hash + // through that handle, and commit (rename) that exact file. + // Commit fails if the path was swapped after verification, so + // the bytes verified are the bytes spawned. + staged, err := updater.OpenVerifiedBinary(newPath, stagedHash) + if err != nil { + slog.Error("update: staged binary re-verification failed, aborting update", "error", err) + return false + } + defer staged.Close() //nolint:errcheck + + // Rename: current -> .old, verified staged binary -> current + _ = os.Remove(oldPath) // remove any stale .old + if err := os.Rename(exePath, oldPath); err != nil { + slog.Error("update: rename current to old failed", "error", err) + return false + } + if err := staged.Commit(exePath); err != nil { + slog.Error("update: committing staged binary failed, restoring original binary", "error", err) + // Whatever is at exePath now (if anything) is not the verified + // binary; restoring .old replaces it. + if restoreErr := os.Rename(oldPath, exePath); restoreErr != nil { + slog.Error("update: CRITICAL — recovery rename also failed, server binary may be missing", + "restore_error", restoreErr, "original_error", err, + "old_path", oldPath, "exe_path", exePath) + } + return false + } + + // Spawn new process. + if err := updater.SpawnDetached(exePath, os.Args[1:]); err != nil { + slog.Error("update: spawn new process failed", "error", err) + return false + } + + // The replacement process is spawned: from here on this process is + // committed to shutting down for it, so the "restarting" promise made at + // the top of handleApplyUpdate's goroutine is about to come true. Cancel + // the deferred corrective broadcast. + restarting = true + + // Signal the process to shut down gracefully before exiting. + // We use SIGTERM on Unix to trigger the graceful shutdown handler + // in main.go. On Windows, os.Exit is unavoidable because the + // process must release its file lock on the binary. + slog.Info("update: new process spawned, shutting down current process") + if p, err := os.FindProcess(os.Getpid()); err == nil { + _ = p.Signal(syscall.SIGTERM) + // Give graceful shutdown a few seconds before force-killing. + time.Sleep(10 * time.Second) + } + return true +} diff --git a/Server/admin/update_handlers_test.go b/Server/admin/update_handlers_test.go index 67ca4652..db90f109 100644 --- a/Server/admin/update_handlers_test.go +++ b/Server/admin/update_handlers_test.go @@ -2,9 +2,13 @@ package admin_test import ( "context" + "crypto/sha256" + "encoding/hex" "encoding/json" "net/http" "net/http/httptest" + "os" + "path/filepath" "testing" "github.com/owncord/server/admin" @@ -404,3 +408,84 @@ func TestAdminAPI_ApplyUpdate_ContainerOptOut(t *testing.T) { t.Errorf("error code = %q, want UPDATE_UNAVAILABLE (container guard must step aside)", resp["error"]) } } + +// ─── OC-0226: aborted apply must correct the earlier restart promise ──────── +// +// handleApplyUpdate's background goroutine broadcasts "server restarting in +// 5s" as its very first action, before any of the on-disk swap actually +// happens. If the swap then fails, every connected client is left believing +// a restart is underway (ServerBanner counts down to a permanent +// "Reconnecting..." state) with no corrective signal ever sent. These tests +// call the swap logic directly — admin.ApplyStagedUpdate — with inputs +// engineered to fail at different points, and assert a corrective +// "update_aborted" broadcast follows. They deliberately do not exercise the +// success path: that ends in os.Exit(0), which would kill the test binary. + +// TestApplyStagedUpdate_VerifyFails_BroadcastsAbort covers the earliest abort +// point: the staged binary re-verification (OpenVerifiedBinary) fails +// because the staged file was never written. +func TestApplyStagedUpdate_VerifyFails_BroadcastsAbort(t *testing.T) { + dir := t.TempDir() + exePath := filepath.Join(dir, "chatserver") + if err := os.WriteFile(exePath, []byte("old binary"), 0o755); err != nil { + t.Fatalf("writing fake exe: %v", err) + } + oldPath := exePath + ".old" + newPath := exePath + ".new" // deliberately never written + + hub := &mockHub{} + admin.ApplyStagedUpdate(hub, exePath, oldPath, newPath, "0000000000000000000000000000000000000000000000000000000000000000") + + if len(hub.restartCalls) != 1 { + t.Fatalf("restartCalls = %d, want 1 (corrective broadcast after abort); got %+v", len(hub.restartCalls), hub.restartCalls) + } + if hub.restartCalls[0].reason == "update" { + t.Fatalf("only broadcast was the original 'restarting' promise (%+v); no corrective broadcast was sent after the abort", hub.restartCalls[0]) + } + + // The original binary must be untouched: verification failed before any + // filesystem mutation. + got, err := os.ReadFile(exePath) + if err != nil || string(got) != "old binary" { + t.Errorf("exePath contents = %q, err=%v; want original binary untouched", got, err) + } +} + +// TestApplyStagedUpdate_RenameToOldFails_BroadcastsAbort covers the second +// abort point: the staged binary verifies fine, but renaming the current +// executable to its .old backup fails (exePath does not exist). +func TestApplyStagedUpdate_RenameToOldFails_BroadcastsAbort(t *testing.T) { + dir := t.TempDir() + exePath := filepath.Join(dir, "chatserver") // deliberately never created + oldPath := exePath + ".old" + newPath := exePath + ".new" + + content := []byte("verified staged bytes") + if err := os.WriteFile(newPath, content, 0o755); err != nil { + t.Fatalf("writing staged binary: %v", err) + } + sum := sha256.Sum256(content) + stagedHash := hex.EncodeToString(sum[:]) + + hub := &mockHub{} + admin.ApplyStagedUpdate(hub, exePath, oldPath, newPath, stagedHash) + + if len(hub.restartCalls) != 1 { + t.Fatalf("restartCalls = %d, want 1 (corrective broadcast after abort); got %+v", len(hub.restartCalls), hub.restartCalls) + } + if hub.restartCalls[0].reason == "update" { + t.Fatalf("only broadcast was the original 'restarting' promise (%+v); no corrective broadcast was sent after the abort", hub.restartCalls[0]) + } +} + +// TestApplyStagedUpdate_NilHub_NoPanic verifies the corrective-broadcast +// guard does not dereference a nil hub (update checking with no ws.Hub is a +// supported configuration — see handleApplyUpdate's nil checks). +func TestApplyStagedUpdate_NilHub_NoPanic(t *testing.T) { + dir := t.TempDir() + exePath := filepath.Join(dir, "chatserver") + oldPath := exePath + ".old" + newPath := exePath + ".new" // never written -> verification fails + + admin.ApplyStagedUpdate(nil, exePath, oldPath, newPath, "0000000000000000000000000000000000000000000000000000000000000000") +} diff --git a/Server/api/dm_handler.go b/Server/api/dm_handler.go index da2496a9..6e408361 100644 --- a/Server/api/dm_handler.go +++ b/Server/api/dm_handler.go @@ -356,9 +356,19 @@ func handleRenameGroupDM(svc *service.Services, broadcaster DMBroadcaster) http. return } - participantIDs, pErr := svc.Channels.GetDMParticipantIDs(r.Context(), channelID) - if pErr == nil { - broadcastDMOpen(r.Context(), svc, broadcaster, channelID, participantIDs) + // The rename has already committed at this point, so this lookup must + // survive the caller's request context being cancelled right after + // that commit (client disconnect mid-handler) — same reasoning as + // broadcastDMOpen's own context.WithoutCancel, and the failure must be + // logged rather than silently dropping the fan-out (participants would + // keep rendering the stale name with no compensating resync, since + // dm_channel_open is unsequenced/targeted and can't be replayed). + bgCtx := context.WithoutCancel(r.Context()) + participantIDs, pErr := svc.Channels.GetDMParticipantIDs(bgCtx, channelID) + if pErr != nil { + slog.Error("handleRenameGroupDM: participant lookup failed", "err", pErr, "channel_id", channelID) + } else { + broadcastDMOpen(bgCtx, svc, broadcaster, channelID, participantIDs) } summary, sErr := svc.DMs.DMSummaryFor(r.Context(), user.ID, channelID) diff --git a/Server/api/dm_handler_rename_participant_lookup_test.go b/Server/api/dm_handler_rename_participant_lookup_test.go new file mode 100644 index 00000000..c121748b --- /dev/null +++ b/Server/api/dm_handler_rename_participant_lookup_test.go @@ -0,0 +1,151 @@ +package api_test + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + + "github.com/go-chi/chi/v5" + "github.com/owncord/server/api" + "github.com/owncord/server/auth" + "github.com/owncord/server/db" + "github.com/owncord/server/service" +) + +// cancelAfterArm is a context.Context whose Done()/Err() behave as +// "never cancelled" until Cancel() is called, at which point they behave as +// an ordinary cancelled context from then on. It simulates a request context +// that gets cancelled *partway through* handling a request (e.g. the client +// disconnecting right after a DB commit), deterministically rather than via +// a wall-clock race. +type cancelAfterArm struct { + context.Context + armed atomic.Bool + done chan struct{} +} + +func newCancelAfterArm(parent context.Context) *cancelAfterArm { + return &cancelAfterArm{Context: parent, done: make(chan struct{})} +} + +func (c *cancelAfterArm) Cancel() { + if c.armed.CompareAndSwap(false, true) { + close(c.done) + } +} + +func (c *cancelAfterArm) Done() <-chan struct{} { + if c.armed.Load() { + return c.done + } + return nil +} + +func (c *cancelAfterArm) Err() error { + if c.armed.Load() { + return context.Canceled + } + return nil +} + +// cancelOnLookupStore wraps the real *db.DB. Its GetDMParticipantIDs cancels +// reqCtx — standing in for the client hanging up immediately after the +// rename's DB commit, i.e. right when the handler goes to look up +// participants for the fan-out — and then performs the real lookup with +// whatever ctx it was handed. If the caller passed r.Context() straight +// through, the lookup itself observes the cancellation and fails; if the +// caller detached it first (context.WithoutCancel), the lookup is unaffected +// and succeeds. This is exactly OC-0222's repro. +// +// Because reqCtx is also the *http.Request's own context, every later +// r.Context()-based call in the handler (e.g. the final DMSummaryFor) is +// realistically affected too — matching a real dropped connection, where +// everything downstream of the disconnect point shares the same fate. Only +// the fan-out (broadcastDMOpen / MarkVisibilityChanged) is this finding's +// concern; the eventual HTTP response is moot once the client is gone, so +// the test does not assert on it. +type cancelOnLookupStore struct { + *db.DB + reqCtx *cancelAfterArm +} + +func (s *cancelOnLookupStore) GetDMParticipantIDs(ctx context.Context, channelID int64) ([]int64, error) { + s.reqCtx.Cancel() + return s.DB.GetDMParticipantIDs(ctx, channelID) +} + +// OC-0222: handleRenameGroupDM's post-rename fan-out — the per-viewer +// dm_channel_open refresh *and* the visibility-watermark bump nested inside +// broadcastDMOpen — is gated on a participant lookup that (before the fix) +// runs on the still-cancellable r.Context(). The rename has already +// committed by then, so if the request context is cancelled in the gap +// (client disconnects right after the write), the lookup fails and the +// entire fan-out is silently skipped: survivors keep rendering the stale +// name, and since dm_channel_open is unsequenced/targeted, only a full +// resync — never a warm reconnect's seq replay — would repair it. +func TestRenameGroupDM_FanOutSurvivesContextCancelledAfterCommit(t *testing.T) { + database := newDMTestDB(t) + bc := &watermarkVoiceBroadcaster{mockBroadcaster: &mockBroadcaster{}} + + // Build the group DM up front over an ordinary router/context so fixture + // setup is unaffected by the special context used for the rename request + // itself. + setupRouter := chi.NewRouter() + setupSvc := service.New(database, auth.NewRateLimiter()) + api.MountDMRoutes(setupRouter, database, setupSvc, bc) + tokens := []string{ + dmCreateToken(t, database, "rn_alice", 4), + dmCreateToken(t, database, "rn_bob", 4), + dmCreateToken(t, database, "rn_carol", 4), + } + group := decodeDMInfo(t, dmPost(t, setupRouter, "/api/v1/dms/group", tokens[0], map[string]any{ + "recipient_ids": []int64{2, 3}, + })) + bc.sent = nil + bc.markCalls = 0 + + // Now build a router whose ChannelService cancels the *request's own* + // context the instant the post-rename participant lookup runs. + reqCtx := newCancelAfterArm(context.Background()) + renameRouter := chi.NewRouter() + renameSvc := service.New(database, auth.NewRateLimiter()) + renameSvc.Channels = service.NewChannelService(&cancelOnLookupStore{DB: database, reqCtx: reqCtx}, renameSvc.Permissions) + api.MountDMRoutes(renameRouter, database, renameSvc, bc) + + body, _ := json.Marshal(map[string]any{"name": "Renamed after disconnect"}) + req := httptest.NewRequest(http.MethodPatch, fmt.Sprintf("/api/v1/dms/%d", group.ChannelID), bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer "+tokens[1]) + req.RemoteAddr = "127.0.0.1:9999" + req = req.WithContext(reqCtx) + rr := httptest.NewRecorder() + renameRouter.ServeHTTP(rr, req) + + if !reqCtx.armed.Load() { + t.Fatal("test bug: the request context was never armed/cancelled — this run does not exercise the repro") + } + + // The rename mutation itself must have committed — the finding is + // explicitly about the fan-out after a successful commit, not about the + // commit itself. Read it back independently of rr's (possibly errored, + // and irrelevant once the "client" is gone) HTTP response. + ch, err := database.GetChannel(context.Background(), group.ChannelID) + if err != nil || ch == nil { + t.Fatalf("GetChannel after rename: %v (ch=%v)", err, ch) + } + if ch.Name != "Renamed after disconnect" { + t.Fatalf("expected the rename to have committed despite the later context cancellation, got name %q", ch.Name) + } + + if len(bc.sent) != 3 { + t.Errorf("expected all 3 participants notified of the rename despite the post-commit context cancellation, got %d sends", len(bc.sent)) + } + if bc.markCalls < 1 { + t.Errorf("MarkVisibilityChanged calls = %d, want at least 1: without it a warm reconnect after the dropped fan-out can never observe the rename via seq replay", bc.markCalls) + } +} diff --git a/Server/api/router.go b/Server/api/router.go index 8299aaef..f61a8b2f 100644 --- a/Server/api/router.go +++ b/Server/api/router.go @@ -4,6 +4,7 @@ package api import ( "context" "encoding/json" + "fmt" "log/slog" "net/http" "net/url" @@ -33,6 +34,34 @@ import ( // pluginRegistry may be nil — in that case the plugin admin endpoints respond // with 503 on lifecycle calls and an empty list on read. func NewRouter(cfg *config.Config, database *db.DB, ver string, logBuf *admin.RingBuffer, pluginRegistry *plugin.Registry) (http.Handler, *ws.Hub, func()) { + // Load (or auto-generate) the AES-256 key for TOTP secret encryption + // (M1). Done first, before any other setup, so a fatal failure here + // (below) doesn't leave background goroutines or partially-mounted + // routes behind. + totpKey, totpKeyErr := auth.LoadOrGenerateTOTPKey(cfg.Server.DataDir) + if totpKeyErr != nil { + if cfg.Server.DataDir != "" { + // A configured data directory means this is a real deployment — + // main.go creates cfg.Server.DataDir before calling NewRouter, so + // by this point LoadOrGenerateTOTPKey only fails for a malformed + // OWNCORD_TOTP_KEY or a corrupt/truncated totp.key file, never for + // a missing directory. (The zero-value "" DataDir used by handler + // tests that never touch TOTP crypto is exempted below so the + // existing test suite keeps passing.) + // + // Continuing here would leave totpKey nil: every AES call in + // auth.EncryptTOTPSecret/DecryptTOTPSecret then hits + // aes.NewCipher(nil) and 500s, so every 2FA-enabled account + // (including the owner) would be locked out of login and unable + // to re-enroll, forever, while /health kept reporting OK. Refuse + // to start instead. + panic(fmt.Sprintf("api: failed to load TOTP encryption key: %v", totpKeyErr)) + } + slog.Error("failed to load TOTP encryption key", "error", totpKeyErr) + // Fall through — only reachable when DataDir is unset; TOTP handlers + // cannot encrypt/decrypt until a data directory is configured. + } + r := chi.NewRouter() // Middleware stack. @@ -86,15 +115,6 @@ func NewRouter(cfg *config.Config, database *db.DB, ver string, logBuf *admin.Ri r.Get("/info", handleInfo(cfg)) }) - // Load (or auto-generate) the AES-256 key for TOTP secret encryption (M1). - totpKey, totpKeyErr := auth.LoadOrGenerateTOTPKey(cfg.Server.DataDir) - if totpKeyErr != nil { - slog.Error("failed to load TOTP encryption key", "error", totpKeyErr) - // Fall through — handlers will still work but cannot encrypt/decrypt. - // This should not happen in practice since LoadOrGenerateTOTPKey - // auto-generates a key when none exists. - } - // Service layer — centralizes business logic for REST and WS handlers. // *db.DB satisfies service.Store directly (the store abstraction was // removed in D3). diff --git a/Server/api/router_totp_key_fatal_test.go b/Server/api/router_totp_key_fatal_test.go new file mode 100644 index 00000000..a2dbb730 --- /dev/null +++ b/Server/api/router_totp_key_fatal_test.go @@ -0,0 +1,59 @@ +package api_test + +import ( + "testing" + + "github.com/owncord/server/api" + "github.com/owncord/server/config" + "github.com/owncord/server/db" +) + +// TestNewRouterRefusesToStartWithMalformedTOTPKey pins OC-0228: a malformed +// OWNCORD_TOTP_KEY (or a corrupt totp.key file) must stop the server from +// coming up rather than let it boot with totpKey == nil. A nil key silently +// breaks every AES call in EncryptTOTPSecret/DecryptTOTPSecret, so every +// 2FA-enabled account — including the owner — would be permanently locked +// out of login (POST /api/v1/auth/verify-totp) and re-enrollment (POST +// /api/v1/users/me/totp/confirm) with a 500, while /health still reports OK. +func TestNewRouterRefusesToStartWithMalformedTOTPKey(t *testing.T) { + // Not valid hex — auth.LoadOrGenerateTOTPKey returns a hard error for + // this instead of silently falling back to auto-generation. + t.Setenv("OWNCORD_TOTP_KEY", "zz") + + database, err := db.Open(":memory:") + if err != nil { + t.Fatalf("db.Open error: %v", err) + } + if err := db.Migrate(database); err != nil { + t.Fatalf("db.Migrate error: %v", err) + } + t.Cleanup(func() { _ = database.Close() }) + + cfg := &config.Config{ + Server: config.ServerConfig{ + Name: "Test Server", + Port: 8443, + // A real, non-empty data dir — as every production deployment + // has (config default is "data", and main.go creates it before + // calling NewRouter) — distinguishes this from the zero-value + // DataDir used by unrelated handler tests that never touch TOTP + // crypto and must keep passing. + DataDir: t.TempDir(), + }, + } + + panicked := false + func() { + defer func() { + if recover() != nil { + panicked = true + } + }() + api.NewRouter(cfg, database, "test", nil, nil) + }() + + if !panicked { + t.Fatal("NewRouter did not refuse to start with a malformed OWNCORD_TOTP_KEY; " + + "it booted with a nil AES key, so verify-totp and totp/confirm would 500 forever") + } +} diff --git a/Server/db/admin_queries.go b/Server/db/admin_queries.go index c6e4a901..de15407a 100644 --- a/Server/db/admin_queries.go +++ b/Server/db/admin_queries.go @@ -5,6 +5,7 @@ import ( "database/sql" "errors" "fmt" + "os" "path/filepath" "strings" @@ -403,8 +404,28 @@ func (d *DB) BackupToSafe(ctx context.Context, path, safeRoot string) error { return fmt.Errorf("BackupToSafe: path contains forbidden sequence %q", "--") } + // VACUUM INTO refuses to write over an existing destination on its own, + // but only after it has already created (and, on failure below, would + // otherwise abandon) the file. Check explicitly and return before the + // exec so the failure branch below can tell "this call created the file" + // (safe to remove) from "the file was already there" (a same-second + // timestamp collision, or an operator-chosen name) without ever deleting + // something that predates this call. + if _, statErr := os.Stat(absClean); statErr == nil { + return fmt.Errorf("BackupToSafe: destination %q already exists", absClean) + } else if !errors.Is(statErr, os.ErrNotExist) { + return fmt.Errorf("BackupToSafe: checking destination %q: %w", absClean, statErr) + } + _, err = d.writer.ExecContext(ctx, fmt.Sprintf("VACUUM INTO '%s'", absClean)) if err != nil { + // An interrupted VACUUM INTO (ENOSPC, EIO, a canceled/expired ctx, ...) + // leaves a truncated file at absClean. Since the existence check above + // already proved nothing was there before this call, whatever exists + // now was created by this exec and is safe to remove — leaving it + // behind would let handleListBackups offer a truncated, unrestorable + // .db as a normal backup (OC-0212). + _ = os.Remove(absClean) return fmt.Errorf("BackupToSafe: %w", err) } return nil diff --git a/Server/db/admin_queries_test.go b/Server/db/admin_queries_test.go index 3b4f2aa3..bcf0c375 100644 --- a/Server/db/admin_queries_test.go +++ b/Server/db/admin_queries_test.go @@ -1,6 +1,7 @@ package db_test import ( + "bytes" "context" "fmt" "os" @@ -8,6 +9,7 @@ import ( "strings" "testing" "testing/fstest" + "time" "github.com/owncord/server/db" ) @@ -902,3 +904,104 @@ func TestBackupToSafe_RejectsTraversal(t *testing.T) { t.Error("BackupToSafe should reject path outside safe root") } } + +// TestBackupToSafe_CleansUpPartialFileOnFailure verifies that a failed +// VACUUM INTO does not leave a truncated .db file behind (OC-0212). A real +// ENOSPC/EIO failure is hard to trigger portably in a unit test, so this +// forces the same outcome — VACUUM INTO fails after it has already created +// the destination file — with a context deadline so tight that the vacuum of +// a non-trivial database is interrupted mid-copy. handleListBackups would +// otherwise offer this leftover file as a restorable backup. +func TestBackupToSafe_CleansUpPartialFileOnFailure(t *testing.T) { + tmpDir := t.TempDir() + dbPath := filepath.Join(tmpDir, "src.db") + + database, err := db.Open(dbPath) + if err != nil { + t.Fatalf("db.Open: %v", err) + } + t.Cleanup(func() { _ = database.Close() }) + + // Enough rows that VACUUM INTO takes long enough to still be running + // when the 1ms deadline below fires, so the destination file exists + // (created, then abandoned mid-copy) at the moment ExecContext returns. + if _, err := database.SQLDb().Exec("CREATE TABLE bulk(x TEXT)"); err != nil { + t.Fatalf("CREATE TABLE bulk: %v", err) + } + tx, err := database.SQLDb().Begin() + if err != nil { + t.Fatalf("Begin: %v", err) + } + stmt, err := tx.Prepare("INSERT INTO bulk(x) VALUES (?)") + if err != nil { + t.Fatalf("Prepare: %v", err) + } + for i := range 300000 { + if _, err := stmt.Exec(i); err != nil { + t.Fatalf("insert bulk row %d: %v", i, err) + } + } + _ = stmt.Close() + if err := tx.Commit(); err != nil { + t.Fatalf("Commit: %v", err) + } + + backupDir := filepath.Join(tmpDir, "backups") + if err := os.MkdirAll(backupDir, 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + backupPath := filepath.Join(backupDir, "partial.db") + + ctx, cancel := context.WithTimeout(context.Background(), 1*time.Millisecond) + defer cancel() + + if err := database.BackupToSafe(ctx, backupPath, backupDir); err == nil { + t.Fatal("BackupToSafe() under a 1ms deadline unexpectedly succeeded") + } + + if _, statErr := os.Stat(backupPath); statErr == nil { + t.Error("BackupToSafe left a truncated backup file behind after failing — " + + "handleListBackups would offer it as restorable") + } else if !os.IsNotExist(statErr) { + t.Fatalf("unexpected error statting backup path: %v", statErr) + } +} + +// TestBackupToSafe_DoesNotDeleteExistingFileOnCollision guards the corollary +// of the fix for OC-0212: cleanup on failure must remove only a file this +// call itself created. VACUUM INTO refuses to write over a destination that +// already exists, and a same-second timestamp collision (or an operator +// re-running a backup to a name they chose) must not let failure-cleanup +// destroy the file that was already sitting there. +func TestBackupToSafe_DoesNotDeleteExistingFileOnCollision(t *testing.T) { + tmpDir := t.TempDir() + dbPath := filepath.Join(tmpDir, "src.db") + + database, err := db.Open(dbPath) + if err != nil { + t.Fatalf("db.Open: %v", err) + } + t.Cleanup(func() { _ = database.Close() }) + + backupDir := filepath.Join(tmpDir, "backups") + if err := os.MkdirAll(backupDir, 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + backupPath := filepath.Join(backupDir, "collide.db") + want := []byte("pre-existing backup contents") + if err := os.WriteFile(backupPath, want, 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + if err := database.BackupToSafe(context.Background(), backupPath, backupDir); err == nil { + t.Fatal("BackupToSafe() should refuse to overwrite an existing destination") + } + + got, err := os.ReadFile(backupPath) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if !bytes.Equal(got, want) { + t.Errorf("pre-existing backup file was modified: got %q, want %q", got, want) + } +} diff --git a/Server/db/attachment_queries.go b/Server/db/attachment_queries.go index 8be937ea..e4a9032b 100644 --- a/Server/db/attachment_queries.go +++ b/Server/db/attachment_queries.go @@ -103,12 +103,17 @@ func (d *DB) GetAttachmentWithChannel(ctx context.Context, id string) (*Attachme // LinkAttachmentsToMessage sets message_id on attachments that are currently // unlinked (message_id IS NULL) and owned by uploaderID. Legacy rows with // uploader_id IS NULL are treated as unowned and may be claimed by any -// sender. Rows that are already linked, owned by another user, or -// nonexistent are skipped rather than errors, so a client retry of a -// partially-completed send cannot fail the whole message. This single UPDATE -// is the atomic attachment-IDOR guard for message sends: ownership is -// enforced in the same statement that links, so there is no check-then-link -// race. Returns the number of rows updated. +// sender. Rows that are already linked, owned by another user, currently +// serving as a live avatar (users.avatar points at them), or nonexistent are +// skipped rather than errors, so a client retry of a partially-completed send +// cannot fail the whole message. Excluding live avatars keeps +// handleServeFile's avatar branch (gated on ChannelID == nil) reachable: once +// message_id is set that branch is dead and the file falls under the +// message's channel ACL / soft-delete state instead, permanently splitting +// from what users.avatar still names (OC-0216). This single UPDATE is the +// atomic attachment-IDOR guard for message sends: ownership is enforced in +// the same statement that links, so there is no check-then-link race. +// Returns the number of rows updated. func (d *DB) LinkAttachmentsToMessage(ctx context.Context, messageID, uploaderID int64, attachmentIDs []string) (int64, error) { if len(attachmentIDs) == 0 { return 0, nil @@ -126,7 +131,8 @@ func (d *DB) LinkAttachmentsToMessage(ctx context.Context, messageID, uploaderID query := fmt.Sprintf( //nolint:gosec // G201: placeholder interpolation, not user input `UPDATE attachments SET message_id = ? WHERE id IN (%s) AND message_id IS NULL - AND (uploader_id = ? OR uploader_id IS NULL)`, + AND (uploader_id = ? OR uploader_id IS NULL) + AND NOT EXISTS (SELECT 1 FROM users u WHERE u.avatar = '/api/v1/files/' || attachments.id)`, strings.Join(placeholders, ","), ) res, err := d.writer.ExecContext(ctx, query, args...) diff --git a/Server/db/attachment_queries_test.go b/Server/db/attachment_queries_test.go index b1a18e47..2ece7026 100644 --- a/Server/db/attachment_queries_test.go +++ b/Server/db/attachment_queries_test.go @@ -167,6 +167,40 @@ func TestLinkAttachmentsToMessage_OwnershipGuard(t *testing.T) { } } +// TestLinkAttachmentsToMessage_SkipsLiveAvatar locks OC-0216: an attachment +// that is currently a user's live avatar (users.avatar points at it) must +// never be claimable by a message. Once message_id is set, handleServeFile's +// avatar branch becomes unreachable (it is gated on ChannelID == nil) and the +// file falls under the message's channel ACL / soft-delete state instead, so +// the avatar permanently disagrees with users.avatar about who may read it. +func TestLinkAttachmentsToMessage_SkipsLiveAvatar(t *testing.T) { + database := openMigratedMemory(t) + owner := seedUser(t, database, "avatar-owner") + chID := seedChannel(t, database, "avatar-owner-ch") + msgID, _ := database.CreateMessage(context.Background(), chID, owner, "attachment carrier", nil) + + if err := database.CreateAttachment(context.Background(), "att-avatar", owner, "a.png", "s-a.png", "image/png", 1, nil, nil); err != nil { + t.Fatalf("CreateAttachment att-avatar: %v", err) + } + if _, err := database.ExecContext(context.Background(), + `UPDATE users SET avatar = ? WHERE id = ?`, + "/api/v1/files/att-avatar", owner, + ); err != nil { + t.Fatalf("setting avatar: %v", err) + } + + n, err := database.LinkAttachmentsToMessage(context.Background(), msgID, owner, []string{"att-avatar"}) + if err != nil { + t.Fatalf("LinkAttachmentsToMessage: %v", err) + } + if n != 0 { + t.Errorf("expected 0 rows linked (live avatar must be skipped), got %d", n) + } + if att, _ := database.GetAttachmentByID(context.Background(), "att-avatar"); att.MessageID != nil { + t.Error("live avatar attachment must never link to a message (OC-0216)") + } +} + // ─── GetAttachmentsByMessageIDs ────────────────────────────────────────────── func TestGetAttachmentsByMessageIDs_Empty(t *testing.T) { diff --git a/Server/db/migrate.go b/Server/db/migrate.go index 37eebd9a..0f451fee 100644 --- a/Server/db/migrate.go +++ b/Server/db/migrate.go @@ -109,14 +109,25 @@ func sqlFilenames(fsys fs.FS) ([]string, error) { return names, nil } -// seedExistingDatabase inserts all migration filenames into schema_versions -// without executing them. This is called once when upgrading a pre-tracking -// database. +// seedExistingDatabase creates schema_versions (if absent) and inserts all +// migration filenames into it without executing them, atomically. This is +// called once when upgrading a pre-tracking database. +// +// The CREATE TABLE runs inside the same transaction as the INSERTs — SQLite +// DDL is transactional — so a failure or interruption partway through +// leaves no schema_versions table behind at all, rather than an empty one. +// An empty-but-present table would make the next MigrateFS call believe +// tracking is already in place, permanently skip seeding, and replay every +// migration against the live, already-populated database. func seedExistingDatabase(d *DB, filenames []string) error { tx, err := d.writer.Begin() if err != nil { return fmt.Errorf("begin seed tx: %w", err) } + if _, execErr := tx.Exec(createSchemaVersions); execErr != nil { + _ = tx.Rollback() + return fmt.Errorf("creating schema_versions in seed tx: %w", execErr) + } for _, name := range filenames { if _, execErr := tx.Exec( "INSERT INTO schema_versions (version) VALUES (?)", name, @@ -134,24 +145,25 @@ func seedExistingDatabase(d *DB, filenames []string) error { // MigrateFS runs tracked migrations from the provided FS. // // Behaviour: -// 1. Create schema_versions if absent. -// 2. If this is the first run with tracking on an existing database (users -// table exists but schema_versions was just created), seed all filenames -// so they are not re-executed. -// 3. For each .sql file in lexicographic order: skip if already recorded, -// otherwise execute the SQL and record the filename. +// 1. If this is the first run with tracking on an existing database (no +// schema_versions table yet, but the "users" table already exists), +// atomically create schema_versions and seed it with every filename so +// none of them are re-executed. Creation and seeding happen in one +// transaction: a failure or interruption partway through leaves no +// schema_versions table behind, so the next run retries seeding instead +// of silently treating tracking as already in place. +// 2. Otherwise, create schema_versions if absent (idempotent — the correct +// state for a fresh database is an empty tracking table) and apply any +// .sql file in lexicographic order that is not yet recorded. func MigrateFS(database *DB, fsys fs.FS) error { - // Determine tracking state before we create schema_versions. + // Determine tracking state before touching schema_versions at all — the + // seeding path below must be the one to create it, atomically with the + // seed rows, so do not call ensureSchemaVersions before this check. svExists, err := schemaVersionsExists(database) if err != nil { return err } - // Create the tracking table (idempotent). - if err := ensureSchemaVersions(database); err != nil { - return err - } - // Collect filenames first — needed for both seeding and normal application. filenames, err := sqlFilenames(fsys) if err != nil { @@ -170,6 +182,13 @@ func MigrateFS(database *DB, fsys fs.FS) error { } } + // Non-seeding paths: schema_versions already exists, or this is a fresh + // database with no prior schema — either way, an idempotent create is + // the correct next step before applying migrations normally. + if err := ensureSchemaVersions(database); err != nil { + return err + } + // Normal path: apply any migration not yet recorded. for _, name := range filenames { applied, applyErr := isApplied(database, name) diff --git a/Server/db/migrate_test.go b/Server/db/migrate_test.go index 2509a3ee..ca3ec5cf 100644 --- a/Server/db/migrate_test.go +++ b/Server/db/migrate_test.go @@ -344,6 +344,94 @@ func TestMigrate_SeedDoesNotReRunMigrations(t *testing.T) { } } +// TestMigrate_InterruptedSeedDoesNotOrphanTrackingTable pins OC-0213: +// schema_versions must not be created outside the seed transaction. If it +// is, an interrupted/failed first-run seed (process killed, OOM, disk +// full — anything that keeps the seed transaction from committing) leaves +// an empty schema_versions table behind. On the next start, +// schemaVersionsExists() reports true, the seeding branch is skipped +// forever, and every migration in the set is replayed against the live, +// already-populated database — including destructive ones. +// +// This test simulates the interruption with PRAGMA max_page_count: it caps +// the database's page budget so MigrateFS's seed transaction runs out of +// room partway through recording filenames, exactly like a crash mid-seed. +// It then lifts the cap (as a real restart would have headroom again) and +// calls MigrateFS a second time, verifying that seeding — not destructive +// execution — is what happens. +func TestMigrate_InterruptedSeedDoesNotOrphanTrackingTable(t *testing.T) { + database := openMemory(t) + ctx := context.Background() + + // Simulate a pre-tracking existing database: the "users" sentinel table + // triggers the seeding heuristic, and carries data that the destructive + // migration below would wipe if it were ever executed instead of seeded. + if _, err := database.ExecContext(ctx, + "CREATE TABLE IF NOT EXISTS users (id INTEGER PRIMARY KEY, name TEXT)", + ); err != nil { + t.Fatalf("setup users table: %v", err) + } + if _, err := database.ExecContext(ctx, + "INSERT INTO users (id, name) VALUES (1, 'admin')", + ); err != nil { + t.Fatalf("setup admin row: %v", err) + } + + // A large migration set: one file is destructive (drops and recreates + // users, losing the row above), and hundreds of harmless, idempotent + // files pad the seed transaction out so a tight page budget is + // guaranteed to run out partway through — not on the very first insert, + // not never. + pairs := make([]string, 0, 2*502) + pairs = append(pairs, + "000_destroy_users.sql", + "DROP TABLE IF EXISTS users; CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT);", + ) + for i := 1; i <= 500; i++ { + pairs = append(pairs, + fmt.Sprintf("%04d_noop.sql", i), + "CREATE TABLE IF NOT EXISTS placeholder (id INTEGER PRIMARY KEY);", + ) + } + fsys := simpleFS(pairs...) + + var basePages int + if err := database.QueryRowContext(ctx, "PRAGMA page_count").Scan(&basePages); err != nil { + t.Fatalf("PRAGMA page_count: %v", err) + } + if _, err := database.ExecContext(ctx, fmt.Sprintf("PRAGMA max_page_count = %d", basePages+3)); err != nil { + t.Fatalf("PRAGMA max_page_count: %v", err) + } + + // First "startup": the seed transaction is interrupted partway through. + if err := db.MigrateFS(database, fsys); err == nil { + t.Fatal("MigrateFS() under the page-budget constraint: expected an error simulating an interrupted seed, got nil") + } + + // Lift the constraint — the next real startup would run on a machine + // with headroom restored. + if _, err := database.ExecContext(ctx, "PRAGMA max_page_count = 4294967294"); err != nil { + t.Fatalf("PRAGMA max_page_count reset: %v", err) + } + + // Second "startup" (the retry). If the interrupted seed above left an + // orphaned, empty schema_versions table behind, MigrateFS now believes + // tracking is already in place, skips seeding entirely, and applies + // every migration for real — including 000_destroy_users.sql. + if err := db.MigrateFS(database, fsys); err != nil { + t.Fatalf("MigrateFS() second run error: %v", err) + } + + var name string + err := database.QueryRowContext(ctx, "SELECT name FROM users WHERE id = 1").Scan(&name) + if err != nil { + t.Fatalf("users row id=1 is gone: 000_destroy_users.sql was executed instead of seeded — an interrupted seed orphaned an empty schema_versions table: %v", err) + } + if name != "admin" { + t.Errorf("users.name = %q, want %q — users table appears to have been recreated", name, "admin") + } +} + // TestMigrate_SchemaVersionsAppliedAtRecorded verifies that applied_at is // populated for every recorded migration. func TestMigrate_SchemaVersionsAppliedAtRecorded(t *testing.T) { diff --git a/Server/main.go b/Server/main.go index a91e5798..90061f5c 100644 --- a/Server/main.go +++ b/Server/main.go @@ -212,17 +212,7 @@ func run(log *slog.Logger, logBuf *admin.RingBuffer, levelVar *slog.LevelVar) er // ── 5c. Wire event persistence (Phase B Step 7) ──────────────────────── if cfg.EventPersistence.Enabled && hub != nil { - // Seed the hub's in-memory seq counter from the persisted MAX(seq) - // so wrapped-payload seqs stay monotonic across restarts. Without - // this, the events table accumulates rows whose payload seqs reset - // to 1 after every restart, breaking the reconnect "events since - // last_seq" contract. - if maxSeq, seedErr := database.GetMaxEventSeq(bgCtx); seedErr != nil { - log.Warn("event persistence: failed to read MAX(events.seq); starting hub seq from 0", "error", seedErr) - } else if maxSeq > 0 { - hub.SeedSeq(uint64(maxSeq)) - log.Info("event persistence: seeded hub seq from persisted events", "seq", maxSeq) - } + seedHubReplayState(bgCtx, hub, database, log) persister := ws.NewEventPersister( database, @@ -421,6 +411,39 @@ func run(log *slog.Logger, logBuf *admin.RingBuffer, levelVar *slog.LevelVar) er return nil } +// seedHubReplayState restores the hub's monotonic seq counter from the +// persisted MAX(events.seq) so wrapped-payload seqs stay monotonic across +// restarts. Without this, the events table accumulates rows whose payload +// seqs reset to 1 after every restart, breaking the reconnect "events since +// last_seq" contract. +// +// It also forces every client resuming from at or before that restored seq +// onto the full-ready path for this boot. h.seq is persisted and restored +// here, but the paired watermark that tells a resuming client whether a +// channel-visibility change happened since its last_seq +// (visibilityChangeSeq) is in-memory only and always starts at 0 on a fresh +// process — see ws/hub_events.go's mustFullResync. Channel-visibility +// changes made to an offline client (RefreshChannelVisibility, +// revokeUnreadableChannels) are sent as targeted, unsequenced messages that +// are never written to the events table, so replay can never recover them. +// Without the MarkVisibilityChanged call below, a client resuming with +// last_seq at or before the pre-restart max sails straight through +// mustFullResync's zeroed watermark and can silently miss a visibility +// change it should have converged on. +func seedHubReplayState(ctx context.Context, hub *ws.Hub, database *db.DB, log *slog.Logger) { + maxSeq, seedErr := database.GetMaxEventSeq(ctx) + if seedErr != nil { + log.Warn("event persistence: failed to read MAX(events.seq); starting hub seq from 0", "error", seedErr) + return + } + if maxSeq <= 0 { + return + } + hub.SeedSeq(uint64(maxSeq)) + log.Info("event persistence: seeded hub seq from persisted events", "seq", maxSeq) + hub.MarkVisibilityChanged() +} + // isAddrInUse checks if an error is an "address already in use" error. func isAddrInUse(err error) bool { return err != nil && (strings.Contains(err.Error(), "address already in use") || strings.Contains(err.Error(), "Only one usage of each socket address")) diff --git a/Server/main_test.go b/Server/main_test.go index 6a7892a3..80254db8 100644 --- a/Server/main_test.go +++ b/Server/main_test.go @@ -1,13 +1,23 @@ package main import ( + "context" + "encoding/json" + "fmt" "io" "log/slog" + "net/http/httptest" + "strings" "testing" + "time" + "github.com/coder/websocket" "go.uber.org/goleak" "github.com/owncord/server/admin" + "github.com/owncord/server/auth" + "github.com/owncord/server/db" + "github.com/owncord/server/ws" ) // TestRun_ServeErrorReturn_StopsHubDispatchGoroutine pins OC-0027: @@ -46,3 +56,109 @@ func TestRun_ServeErrorReturn_StopsHubDispatchGoroutine(t *testing.T) { t.Fatalf("hub dispatch goroutine (and, in production, its LiveKit process) leaked after run() returned early: %v", err) } } + +// TestSeedHubReplayState_ForcesFullResyncForOfflineClient pins OC-0204: +// h.seq is persisted (events table) and restored at startup via SeedSeq, but +// its paired in-memory watermark (visibilityChangeSeq) always starts at 0 on +// a fresh process. mustFullResync short-circuits on `w > 0`, so without also +// forcing the watermark forward at startup, every client resuming from a +// last_seq at or before the just-restored max sails through mustFullResync +// and gets an ordinary tiered replay — even though a channel-visibility +// change made to it while offline (RefreshChannelVisibility, +// revokeUnreadableChannels) was sent only as a targeted, unsequenced message +// that was never persisted and can never be recovered by that replay. +// +// This seeds a DB with a contiguous run of persisted events (simulating a +// prior boot that reached seq 520), then calls seedHubReplayState exactly as +// run() does, then reconnects a client with last_seq=500 (<= the restored +// max) and asserts the resume is forced onto the full-ready tier. Before the +// fix, last_seq=500 converges via the ordinary DB cold-tier replay instead +// (the persisted run 501..520 is contiguous and complete), silently proving +// the bug: a resume that must be forced full sails through unforced. +func TestSeedHubReplayState_ForcesFullResyncForOfflineClient(t *testing.T) { + database, err := db.Open(":memory:") + if err != nil { + t.Fatalf("db.Open: %v", err) + } + defer database.Close() //nolint:errcheck + if err := db.Migrate(database); err != nil { + t.Fatalf("db.Migrate: %v", err) + } + + ctx := context.Background() + + // Simulate the prior boot: 20 persisted global (channel_id=0) events at + // seqs 501..520, contiguous and complete — exactly the shape that lets + // handleReconnect's DB-tier contiguity/tail checks succeed today. + for seq := int64(501); seq <= 520; seq++ { + payload := fmt.Appendf(nil, `{"seq":%d,"type":"broadcast"}`, seq) + if err := database.PersistEvent(ctx, seq, "broadcast", 0, payload); err != nil { + t.Fatalf("PersistEvent seq=%d: %v", seq, err) + } + } + + userID, err := database.CreateUser(ctx, "seed-replay-user", "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(ctx, userID, auth.HashToken(token), "test", "127.0.0.1"); err != nil { + t.Fatalf("CreateSession: %v", err) + } + + limiter := auth.NewRateLimiter() + hub := ws.NewHub(database, limiter, nil) + go hub.Run() + defer hub.Stop() + + // The exact startup call run() makes once event persistence is enabled — + // no ring-buffer events are pushed, so a resuming client's replay can + // only be satisfied via the DB cold tier or forced full. + log := slog.New(slog.NewTextHandler(io.Discard, nil)) + seedHubReplayState(ctx, hub, database, log) + hub.SetEventStore(database) + + handler := ws.ServeWS(hub, database, []string{"*"}) + srv := httptest.NewServer(handler) + defer srv.Close() + + wsURL := "ws" + strings.TrimPrefix(srv.URL, "http") + dialCtx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + + conn, dialResp, dialErr := websocket.Dial(dialCtx, wsURL, nil) + if dialResp != nil && dialResp.Body != nil { + _ = dialResp.Body.Close() + } + if dialErr != nil { + t.Fatalf("websocket.Dial: %v", dialErr) + } + defer func() { _ = conn.Close(websocket.StatusNormalClosure, "") }() + + // last_seq=500 predates the restored max (520): a client whose sidebar + // missed a targeted visibility change while offline must be forced onto + // the full-ready path to converge. + authMsg := map[string]any{ + "type": "auth", + "payload": map[string]any{ + "token": token, + "last_seq": uint64(500), + }, + } + raw, _ := json.Marshal(authMsg) + if err := conn.Write(dialCtx, websocket.MessageText, raw); err != nil { + t.Fatalf("write auth: %v", err) + } + if _, _, err := conn.Read(dialCtx); err != nil { + t.Fatalf("read handshake response: %v", err) + } + + bufTier, dbTier, fullTier := hub.ReconnectTierStats() + if fullTier != 1 { + t.Fatalf("reconnect tiers (buffer=%d db=%d full=%d): want full=1 — a client resuming from before a restart-restored seq must be forced onto the full-ready path, since an offline visibility change is never recoverable by replay", + bufTier, dbTier, fullTier) + } +} diff --git a/Server/service/channel.go b/Server/service/channel.go index 90be13db..89fdff2b 100644 --- a/Server/service/channel.go +++ b/Server/service/channel.go @@ -114,12 +114,6 @@ func (s *ChannelService) HandleTyping(ctx context.Context, userID, channelID int return nil, nil } - // Per-user-per-channel rate limit. - ratKey := auth.Key(auth.Key("typing", userID), channelID) - if limiter != nil && !limiter.Allow(ratKey, 1, 3*time.Second) { - return nil, nil - } - ch, err := s.st.GetChannel(ctx, channelID) if err != nil || ch == nil { return nil, nil //nolint:nilerr // typing indicators are best-effort; errors silently dropped @@ -140,6 +134,18 @@ func (s *ChannelService) HandleTyping(ctx context.Context, userID, channelID int return nil, nil // silent drop } + // Per-user-per-channel rate limit. Built only now that the channel is + // known to exist and the caller is authorized to read it (OC-0202): doing + // this before resolution let any caller-supplied channel id — including + // ids that don't exist or aren't readable — pin a new entry in the + // shared, process-wide RateLimiter. RateLimiter.Cleanup only evicts a key + // once every timestamp on it is stale, so a stream of forged channel ids + // could retain an unbounded number of dead map entries for hours. + ratKey := auth.Key(auth.Key("typing", userID), channelID) + if limiter != nil && !limiter.Allow(ratKey, 1, 3*time.Second) { + return nil, nil + } + return ch, nil } diff --git a/Server/service/channel_test.go b/Server/service/channel_test.go index 2d006c69..f9a65862 100644 --- a/Server/service/channel_test.go +++ b/Server/service/channel_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "testing" + "time" "github.com/owncord/server/db" "github.com/owncord/server/permissions" @@ -137,3 +138,81 @@ func TestHandleTyping_BlockedInDMEmitsNothing(t *testing.T) { t.Fatal("blocked user must not produce a typing broadcast") } } + +// countingLimiter records every key passed to Allow so a test can assert +// whether the rate-limit map was ever touched for a given call, without +// depending on auth.RateLimiter's unexported internals. Allow always grants +// the request — these tests only care about whether a key was built at all. +type countingLimiter struct { + calls []string +} + +func (c *countingLimiter) Allow(key string, limit int, window time.Duration) bool { + c.calls = append(c.calls, key) + return true +} + +// TestHandleTyping_NoRateLimitKeyForNonexistentChannel locks OC-0202: +// HandleTyping used to build the "typing::" rate-limit key and call +// limiter.Allow BEFORE resolving the channel at all, so any caller-supplied +// channel id — including ids that don't exist — pinned a new entry in the +// shared, process-wide RateLimiter. RateLimiter.Cleanup only evicts a key +// once every timestamp on it is stale, and production runs cleanup with a +// 6-hour window, so a client sending typing_start for a stream of forged +// channel ids could retain millions of dead map entries for hours. The key +// must only be built once the channel is known to exist (and, below, +// once the caller is authorized to read it) so the key space is bounded to +// real (user, channel) pairs. +func TestHandleTyping_NoRateLimitKeyForNonexistentChannel(t *testing.T) { + database := newTestDB(t) + seedRole(t, database, &db.Role{ + ID: permissions.MemberRoleID, + Name: "member", + Permissions: permissions.SendMessages | permissions.ReadMessages, + Position: 1, + }) + seedUser(t, database, &db.User{ID: 1, Username: "alice"}) + seedUserRole(t, database, 1, permissions.MemberRoleID) + // Deliberately do NOT seed channel 999999 — it must not exist. + + svc := NewChannelService(database, NewPermissionService(database, permissions.NewChecker(database))) + limiter := &countingLimiter{} + + ch, err := svc.HandleTyping(context.Background(), 1, 999999, limiter) + if err != nil || ch != nil { + t.Fatalf("typing on a nonexistent channel must silently drop: ch=%v err=%v", ch, err) + } + if len(limiter.calls) != 0 { + t.Fatalf("HandleTyping built a rate-limit key for a nonexistent channel: calls=%v — "+ + "every forged channel id pins a new entry in the shared RateLimiter for hours "+ + "(Cleanup only evicts once every timestamp on the key is stale)", limiter.calls) + } +} + +// TestHandleTyping_NoRateLimitKeyWithoutReadPermission extends OC-0202 to an +// existing channel the caller cannot read: the rate-limit key must still not +// be built, so the key space stays bounded to channels the user is actually +// authorized to see typing indicators in. +func TestHandleTyping_NoRateLimitKeyWithoutReadPermission(t *testing.T) { + database := newTestDB(t) + seedRole(t, database, &db.Role{ + ID: permissions.MemberRoleID, + Name: "member", + Permissions: permissions.SendMessages, // no ReadMessages + Position: 1, + }) + seedUser(t, database, &db.User{ID: 1, Username: "alice"}) + seedUserRole(t, database, 1, permissions.MemberRoleID) + seedChannel(t, database, &db.Channel{ID: 10, Name: "secret", Type: "text"}) + + svc := NewChannelService(database, NewPermissionService(database, permissions.NewChecker(database))) + limiter := &countingLimiter{} + + ch, err := svc.HandleTyping(context.Background(), 1, 10, limiter) + if err != nil || ch != nil { + t.Fatalf("typing without ReadMessages must silently drop: ch=%v err=%v", ch, err) + } + if len(limiter.calls) != 0 { + t.Fatalf("HandleTyping built a rate-limit key before checking ReadMessages permission: calls=%v", limiter.calls) + } +} diff --git a/Server/service/emoji.go b/Server/service/emoji.go index 16a53751..4ebec287 100644 --- a/Server/service/emoji.go +++ b/Server/service/emoji.go @@ -141,6 +141,12 @@ func (s *EmojiService) Create(ctx context.Context, actorID int64, rawShortcode, created, err := s.st.CreateEmoji(ctx, shortcode, storedAs, mimeType, actorID) if err != nil { + if db.IsUniqueConstraintError(err) { + // Lost a race with another Create between the check above and this + // INSERT -- report the conflict the check would have caught, not a + // server fault. + return nil, fmt.Errorf("%w: an emoji named :%s: already exists", ErrConflict, shortcode) + } return nil, fmt.Errorf("%w: failed to create emoji: %v", ErrInternal, err) } diff --git a/Server/service/emoji_test.go b/Server/service/emoji_test.go index b86e20e8..78c2a21a 100644 --- a/Server/service/emoji_test.go +++ b/Server/service/emoji_test.go @@ -192,6 +192,49 @@ func TestEmojiCreate_DuplicateShortcodeIsConflict(t *testing.T) { } } +// raceEmojiStore wraps a real *db.DB but always reports no existing emoji for +// the pre-insert shortcode check, so a concurrent CreateEmoji that already +// committed the same shortcode is only caught by the table's UNIQUE +// constraint at INSERT time -- exactly what happens when two CreateEmoji +// calls race past GetEmojiByShortcode before either INSERT commits. +type raceEmojiStore struct { + *db.DB +} + +func (f *raceEmojiStore) GetEmojiByShortcode(_ context.Context, _ string) (*db.Emoji, error) { + return nil, nil +} + +// TestEmojiCreate_RaceOnInsertIsConflict pins OC-0217: when the shortcode +// check races another Create and the row already exists by the time the +// INSERT runs, the resulting UNIQUE-constraint error from CreateEmoji must +// still surface as ErrConflict (matching the sequential duplicate-shortcode +// path), not ErrInternal. +func TestEmojiCreate_RaceOnInsertIsConflict(t *testing.T) { + database := newTestDB(t) + seedRole(t, database, &db.Role{ID: permissions.OwnerRoleID, Name: "Owner", + Permissions: permissions.Administrator, Position: permissions.OwnerRolePosition}) + seedUser(t, database, &db.User{ID: 1}) + seedUserRole(t, database, 1, permissions.OwnerRoleID) + + checker := permissions.NewChecker(database) + svc := NewEmojiService(&raceEmojiStore{DB: database}, NewPermissionService(database, checker)) + + // Commit the shortcode directly, bypassing the service's own check, so the + // table already holds :wave: when Create runs its (stubbed) check. + if _, err := database.CreateEmoji(context.Background(), "wave", "stored-1", "image/png", 1); err != nil { + t.Fatalf("seed CreateEmoji: %v", err) + } + + _, err := svc.Create(context.Background(), 1, "wave", "stored-2", "image/gif") + if !errors.Is(err, ErrConflict) { + t.Fatalf("raced Create error = %v, want ErrConflict", err) + } + if errors.Is(err, ErrInternal) { + t.Fatalf("raced Create error = %v, must not be ErrInternal", err) + } +} + func TestEmojiCreate_RejectsBadShortcodeBeforeInsert(t *testing.T) { svc, _ := newEmojiService(t) if _, err := svc.Create(context.Background(), 1, "no spaces", "stored-1", "image/png"); !errors.Is(err, ErrBadRequest) { diff --git a/Server/service/moderation.go b/Server/service/moderation.go index 829b08b8..af69e6fc 100644 --- a/Server/service/moderation.go +++ b/Server/service/moderation.go @@ -130,6 +130,52 @@ func (s *ModerationService) BanUser(ctx context.Context, actorID, targetID int64 return nil } +// AuthorizeRoleChange runs every ChangeUserRole precondition — MANAGE_ROLES, +// target existence, the actor-outranks-target rule, role existence, and the +// assign-below-own-rank rule — without mutating anything, and in the same +// authorization-before-existence order as every other check in this file (see +// BanUser): an actor without MANAGE_ROLES learns nothing about which user ids +// exist. It exists so a caller that also performs another mutation in the +// same request (the admin PATCH /users/{id} handler, which can ban and +// role-change in one call) can authorize the role change *before* committing +// the other mutation: checking only at ChangeUserRole time means a refused +// role change is discovered only after the ban already landed, leaving a +// "failed" request half-applied (OC-0215). It returns the validated actor +// role, target user, and target role so callers that go on to commit (like +// ChangeUserRole) don't need to re-fetch any of them. +func (s *ModerationService) AuthorizeRoleChange(ctx context.Context, actorID, targetID, newRoleID int64) (actorRole *db.Role, target *db.User, newRole *db.Role, err error) { + if targetID <= 0 { + return nil, nil, nil, fmt.Errorf("%w: user_id must be positive", ErrBadRequest) + } + if actorID == targetID { + return nil, nil, nil, fmt.Errorf("%w: cannot change your own role", ErrBadRequest) + } + + // Authorization before existence — see BanUser. + actorRole, err = s.requirePerm(ctx, actorID, permissions.ManageRoles) + if err != nil { + return nil, nil, nil, err + } + target, err = s.st.GetUserByID(ctx, targetID) + if err != nil || target == nil { + return nil, nil, nil, fmt.Errorf("%w: user not found", ErrNotFound) + } + if err := s.requireOutranksRole(ctx, actorRole, targetID); err != nil { + return nil, nil, nil, err + } + + newRole, err = s.st.GetRoleByID(ctx, newRoleID) + if err != nil || newRole == nil { + return nil, nil, nil, fmt.Errorf("%w: role not found", ErrBadRequest) + } + // Administrator bypasses permission bits, never the hierarchy: the owner + // role is above every admin, so only the owner can grant it. + if newRole.Position >= actorRole.Position { + return nil, nil, nil, fmt.Errorf("%w: cannot assign a role at or above your own rank", ErrForbidden) + } + return actorRole, target, newRole, nil +} + // ChangeUserRole assigns newRoleID to the target user. It enforces // MANAGE_ROLES plus two hierarchy rules the admin panel previously had none // of: the actor must strictly outrank the target, and may not hand out a role @@ -142,35 +188,10 @@ func (s *ModerationService) BanUser(ctx context.Context, actorID, targetID int64 // delete for no reason, since this call already loaded and validated the // exact same row under the same request. func (s *ModerationService) ChangeUserRole(ctx context.Context, actorID, targetID, newRoleID int64) (*db.Role, error) { - if targetID <= 0 { - return nil, fmt.Errorf("%w: user_id must be positive", ErrBadRequest) - } - if actorID == targetID { - return nil, fmt.Errorf("%w: cannot change your own role", ErrBadRequest) - } - - // Authorization before existence — see BanUser. - actorRole, err := s.requirePerm(ctx, actorID, permissions.ManageRoles) + _, target, newRole, err := s.AuthorizeRoleChange(ctx, actorID, targetID, newRoleID) if err != nil { return nil, err } - target, err := s.st.GetUserByID(ctx, targetID) - if err != nil || target == nil { - return nil, fmt.Errorf("%w: user not found", ErrNotFound) - } - if err := s.requireOutranksRole(ctx, actorRole, targetID); err != nil { - return nil, err - } - - newRole, err := s.st.GetRoleByID(ctx, newRoleID) - if err != nil || newRole == nil { - return nil, fmt.Errorf("%w: role not found", ErrBadRequest) - } - // Administrator bypasses permission bits, never the hierarchy: the owner - // role is above every admin, so only the owner can grant it. - if newRole.Position >= actorRole.Position { - return nil, fmt.Errorf("%w: cannot assign a role at or above your own rank", ErrForbidden) - } if err := s.st.UpdateUserRole(ctx, targetID, newRoleID); err != nil { return nil, fmt.Errorf("%w: failed to update role: %v", ErrInternal, err) diff --git a/Server/ws/emit.go b/Server/ws/emit.go index d0d91972..f77cae3a 100644 --- a/Server/ws/emit.go +++ b/Server/ws/emit.go @@ -55,12 +55,18 @@ func (h *Hub) EmitEvents(ctx context.Context, events []Event) { // lookup dies with it rather than outliving the request. h.broadcastVoiceEvent(ctx, e.VisibleChannelID(), e.Payload()) case BroadcastAllEvent: - // Check concrete type: presence is low-priority, others are normal. - if _, isPresence := ev.(PresenceEvent); isPresence { - h.BroadcastToAllLow(e.Payload()) - } else { - h.BroadcastToAll(e.Payload()) - } + // Normal priority for everything, including presence: connect and + // disconnect presence for the same user already go out via + // hub.BroadcastToAll (serve.go, serve_pumps.go, hub_broadcast.go). + // Splitting handler-driven presence onto the low-priority queue + // put it in a different per-client FIFO than those, so writePump + // (which always drains normal strictly before low) could deliver + // a newer connect/disconnect frame before an older presence_update + // still sitting in the low queue — leaving the observer's final + // view of that user's status stale. Routing everything through + // BroadcastToAll keeps every source of one user's presence in a + // single ordered, seq-stamped, replayable stream. + h.BroadcastToAll(e.Payload()) default: slog.Warn("EmitEvents: unknown event type", "type", fmt.Sprintf("%T", ev)) } diff --git a/Server/ws/emit_presence_priority_test.go b/Server/ws/emit_presence_priority_test.go new file mode 100644 index 00000000..218d8274 --- /dev/null +++ b/Server/ws/emit_presence_priority_test.go @@ -0,0 +1,69 @@ +package ws + +// emit_presence_priority_test.go — regression test for OC-0214: handler-driven +// presence (presence_update, from PresenceEvent) used to go out on the +// low-priority send queue via BroadcastToAllLow, while connect/disconnect +// presence for the very same user goes out on the normal-priority queue via +// BroadcastToAll. writePump always drains normal strictly before low, so an +// observer with both queued ends up seeing whichever frame happens to be +// normal-priority last, regardless of which one is actually newer — the two +// sources of truth for one user's presence were never in a single FIFO +// together. BroadcastAllEvent's own doc comment (event.go) says it "routes to +// Hub.BroadcastToAll"; PresenceEvent silently violated that. + +import ( + "context" + "testing" + "time" +) + +// TestEmitEvents_PresenceEvent_UsesNormalPriorityQueue pins the fix: a +// handler-driven PresenceEvent routed through the BroadcastAllEvent case must +// land on the client's normal-priority queue (the same one connect/disconnect +// presence uses via hub.BroadcastToAll), never on the low-priority queue. +// +// Before the fix, emit.go special-cased PresenceEvent onto +// h.BroadcastToAllLow, so this test observes the frame on c.sendLow instead +// of c.send and fails. +func TestEmitEvents_PresenceEvent_UsesNormalPriorityQueue(t *testing.T) { + h := newEmitTestHub() + + // Built directly (not via the emit_test.go helpers) so send and sendLow + // are DISTINCT channels — the shared-channel helpers in export_test.go are + // unified "for test observability" and would mask exactly the queue-split + // this test needs to detect. + c := &Client{ + hub: h, + ctx: context.Background(), + userID: 1, + send: make(chan []byte, 8), + sendHigh: make(chan []byte, 8), + sendLow: make(chan []byte, 8), + } + h.clients[1] = c + h.pubsub.Subscribe(c, TopicGlobal) + + // BroadcastToAll (normal priority) goes through the async hub.broadcast + // channel, so the hub loop must be running to deliver it. + go h.Run() + defer h.Stop() + + payload := []byte(`{"type":"presence_update","user_id":1,"status":"idle"}`) + h.EmitEvents(context.Background(), []Event{PresenceEvent{payload: payload}}) + + normalMsgs := drainChan(c.send, 200*time.Millisecond) + lowMsgs := drainChan(c.sendLow, 50*time.Millisecond) + + if len(normalMsgs) != 1 { + t.Errorf("expected handler-driven presence on the normal-priority queue "+ + "(same FIFO as connect/disconnect presence), got %d normal messages, %d low messages", + len(normalMsgs), len(lowMsgs)) + } + if len(lowMsgs) != 0 { + t.Errorf("handler-driven presence must not go out on the low-priority queue: "+ + "writePump drains normal strictly before low, so a presence_update queued "+ + "there can be delivered after a later connect/disconnect presence frame on "+ + "the normal queue, leaving the observer's final view stale; got %d low messages", + len(lowMsgs)) + } +} diff --git a/Server/ws/event.go b/Server/ws/event.go index d9372382..bb9a57fe 100644 --- a/Server/ws/event.go +++ b/Server/ws/event.go @@ -284,7 +284,14 @@ func presenceEvents(userID int64, status string, customStatus *string) []Event { return []Event{ PresenceOthersEvent{ excludeUserID: userID, - payload: buildPresenceMsg(userID, public, customStatus), + // customStatus is blanked, not passed through: the status here + // already collapsed to "offline" (public != status), and the real + // free-text status would be a tell that this "offline" member is + // actually online. Mirrors hub_broadcast.go's BroadcastPresence, + // the connect/reconnect sibling of this same event, and + // db.MemberSummary.ForViewer, which blanks the same field the + // same way for the ready payload. + payload: buildPresenceMsg(userID, public, nil), }, PresenceSelfEvent{ targetUserID: userID, diff --git a/Server/ws/event_presence_test.go b/Server/ws/event_presence_test.go new file mode 100644 index 00000000..5fed90df --- /dev/null +++ b/Server/ws/event_presence_test.go @@ -0,0 +1,70 @@ +package ws + +// event_presence_test.go — regression test for OC-0211's event.go sibling. +// +// presenceEvents is the live presence_update path (handlePresenceV2 -> +// presenceEvents), the sibling of hub_broadcast.go's BroadcastPresence for +// the connect/reconnect path. Both built the public PresenceOthersEvent +// frame with the raw customStatus passed straight through, so an invisible +// user setting a custom status live leaked the same text this whole feature +// exists to hide: every other client would see {status:"offline", +// custom_status:""}, a combination that discloses the member is +// actually online. + +import ( + "encoding/json" + "testing" + + "github.com/owncord/server/db" +) + +// presenceEnvelope mirrors the {"type":...,"payload":{...}} shape buildJSON +// produces for a presence message. +type presenceEnvelope struct { + Payload struct { + Status string `json:"status"` + CustomStatus *string `json:"custom_status"` + } `json:"payload"` +} + +func TestPresenceEvents_InvisibleBlanksCustomStatusForOthers(t *testing.T) { + text := "in a meeting" + events := presenceEvents(99, db.StatusInvisible, &text) + + var sawOthers, sawSelf bool + for _, e := range events { + switch ev := e.(type) { + case PresenceOthersEvent: + sawOthers = true + var env presenceEnvelope + if err := json.Unmarshal(ev.Payload(), &env); err != nil { + t.Fatalf("unmarshal PresenceOthersEvent payload: %v", err) + } + if env.Payload.Status != db.StatusOffline { + t.Errorf("PresenceOthersEvent status = %q, want %q", env.Payload.Status, db.StatusOffline) + } + if env.Payload.CustomStatus != nil { + t.Errorf("PresenceOthersEvent custom_status = %v, want nil (leaked invisible user's real status text to every observer)", *env.Payload.CustomStatus) + } + case PresenceSelfEvent: + sawSelf = true + var env presenceEnvelope + if err := json.Unmarshal(ev.Payload(), &env); err != nil { + t.Fatalf("unmarshal PresenceSelfEvent payload: %v", err) + } + if env.Payload.Status != db.StatusInvisible { + t.Errorf("PresenceSelfEvent status = %q, want %q", env.Payload.Status, db.StatusInvisible) + } + // The owner must still see their own real custom status. + if env.Payload.CustomStatus == nil || *env.Payload.CustomStatus != text { + t.Errorf("PresenceSelfEvent custom_status = %v, want %q", env.Payload.CustomStatus, text) + } + } + } + if !sawOthers { + t.Fatal("presenceEvents did not produce a PresenceOthersEvent for an invisible status change") + } + if !sawSelf { + t.Fatal("presenceEvents did not produce a PresenceSelfEvent for an invisible status change") + } +} diff --git a/Server/ws/hub_broadcast.go b/Server/ws/hub_broadcast.go index 0c44b754..83ff91b7 100644 --- a/Server/ws/hub_broadcast.go +++ b/Server/ws/hub_broadcast.go @@ -296,6 +296,16 @@ func (h *Hub) BroadcastChannelDelete(channelID int64) { h.BroadcastToAll(buildChannelDelete(channelID)) } +// refreshChannelVisibilityRaceHook, when non-nil, runs once per connected +// user after RefreshChannelVisibility resolves that user's visibility for ch +// but before it re-resolves and acts on the live client. Test-only (always +// nil in production): the window it pins spans one or two DB round trips per +// client (the permission lookup below), too fast to land a real reconnect +// goroutine inside reliably, so tests use this hook to reproduce a reconnect +// racing in at exactly that point deterministically. Mirrors the established +// voiceJoinPostTokenRaceHook / cleanupVoiceRaceClearHook pattern. +var refreshChannelVisibilityRaceHook func(userID int64) + // RefreshChannelVisibility re-evaluates which connected clients may see ch // after a channel_overrides change and sends targeted channel_create / // channel_delete messages so sidebars converge without a reconnect. Clients @@ -311,6 +321,19 @@ func (h *Hub) RefreshChannelVisibility(ch *db.Channel) { return } + // Bump the watermark immediately, before the h.clients snapshot below and + // the (potentially slow — up to two DB round trips per connected client) + // fan-out loop that follows it. A reconnect handshake re-checks this + // watermark right before it registers (OC-0206); bumping only at the end, + // after the loop, left a window where that re-check could still observe + // the pre-change value even though this function's snapshot — taken next + // — will never include a client that registers mid-loop. Ratcheted + // upward only (see bumpVisibilityWatermark), so this is a no-op whenever + // a concurrent writer already pushed the watermark higher; the trailing + // bump below still runs and covers any change to h.seq made during the + // loop itself. + h.bumpVisibilityWatermark() + h.mu.RLock() clients := make([]*Client, 0, len(h.clients)) for _, c := range h.clients { @@ -401,21 +424,42 @@ func (h *Hub) RefreshChannelVisibility(ch *db.Channel) { } visible = userVisible(fresh.ID, fresh.RoleID) } + + if refreshChannelVisibilityRaceHook != nil { + refreshChannelVisibilityRaceHook(c.user.ID) + } + + // Re-resolve the live client immediately before acting: the permission + // lookups above (a PermissionService call, or two DB round trips in the + // bare-hub branch) give a reconnect room to replace this user's *Client + // in h.clients with a new connection under the same user ID. Acting on + // the stale snapshot pointer c would target a dead socket, and + // Unsubscribe would be a no-op — unsubscribeLocked's identity guard + // leaves a topic alone when the current holder differs from the client + // passed in — stranding the replacement with a subscription (or a + // missing one) exactly inverted from what this fan-out just decided. + // A nil result means the user disconnected entirely since the + // snapshot; nothing to act on. + live := h.GetClient(c.user.ID) + if live == nil { + continue + } + if visible { // Idempotent add on the client; also refreshes channel metadata. // Addressed per client so it can carry this recipient's own // can_send verdict — the whole point of this fan-out is that a // permission change just made those verdicts diverge. - c.sendMsg(buildChannelCreateFor(ch, userCanSend(c.user.ID, c.user.RoleID))) + live.sendMsg(buildChannelCreateFor(ch, userCanSend(c.user.ID, c.user.RoleID))) continue } - c.sendMsg(buildChannelDelete(ch.ID)) - h.pubsub.Unsubscribe(c, ChannelTopic(ch.ID)) - c.mu.Lock() - if c.channelID == ch.ID { - c.channelID = 0 + live.sendMsg(buildChannelDelete(ch.ID)) + h.pubsub.Unsubscribe(live, ChannelTopic(ch.ID)) + live.mu.Lock() + if live.channelID == ch.ID { + live.channelID = 0 } - c.mu.Unlock() + live.mu.Unlock() } // Clients not connected right now missed the targeted sends above. Move @@ -523,7 +567,14 @@ func (h *Hub) BroadcastPresence(userID int64, status string, customStatus *strin h.BroadcastToAll(buildPresenceMsg(userID, status, customStatus)) return } - h.broadcastExcludeLow(0, userID, buildPresenceMsg(userID, public, customStatus)) + // The public frame's status already collapsed to db.BroadcastStatus, but + // customStatus does not: passing it through verbatim would tell every + // other client an "offline" member's real free-text status, which is a + // tell that they are actually online. Blank it explicitly (not omitted — + // presencePayload.CustomStatus has no omitempty) so the client clears any + // cached text, matching what db.MemberSummary.ForViewer already does for + // the ready payload's member list. + h.broadcastExcludeLow(0, userID, buildPresenceMsg(userID, public, nil)) h.SendToUser(userID, buildPresenceMsg(userID, status, customStatus)) } @@ -558,6 +609,14 @@ func (h *Hub) revokeUnreadableChannels(userID int64) { // socket is closed below, converges via the full-ready path. defer h.bumpVisibilityWatermark() + // Also bump immediately, before the h.clients lookup below and the + // per-topic DB loop (a GetChannel round trip per revoked topic) that + // follows it — see RefreshChannelVisibility's matching early bump and + // OC-0206. Ratcheted upward only, so this is a no-op whenever a + // concurrent writer already pushed the watermark higher; the deferred + // bump above still covers every return path, including the early ones. + h.bumpVisibilityWatermark() + if h.db == nil { return } diff --git a/Server/ws/hub_refresh_visibility_race_test.go b/Server/ws/hub_refresh_visibility_race_test.go new file mode 100644 index 00000000..fe93717c --- /dev/null +++ b/Server/ws/hub_refresh_visibility_race_test.go @@ -0,0 +1,120 @@ +package ws + +// hub_refresh_visibility_race_test.go — regression test for OC-0205. +// +// RefreshChannelVisibility snapshots h.clients once, then for every entry +// resolves the user's CURRENT visibility via one or two DB round trips +// (h.db.GetUserByID + h.db.GetRoleByID in the bare-hub branch exercised +// here, or a PermissionService lookup otherwise) before acting on the +// snapshotted *Client pointer with sendMsg / Unsubscribe / a channelID +// clear. A reconnect landing during those per-client lookups replaces the +// snapshotted client with a new connection under the same user ID — +// h.clients[userID] now points at the new client, and PubSub.Unsubscribe's +// identity guard silently no-ops when asked to strip a topic from a client +// that is no longer the current holder. Acting on the stale pointer +// therefore reaches a dead socket and leaves the live replacement with +// whatever subscription/channelID it already had, exactly inverted from +// what the fan-out just decided. +// +// The DB round trips are too fast to land a real reconnect goroutine inside +// reliably, so refreshChannelVisibilityRaceHook (test-only, nil in +// production) fires at exactly that point, mirroring the established +// voiceJoinPostTokenRaceHook / cleanupVoiceRaceClearHook pattern used to pin +// the analogous races elsewhere in this package. + +import ( + "context" + "encoding/json" + "testing" + "time" + + "github.com/owncord/server/auth" + "github.com/owncord/server/permissions" +) + +func TestRefreshChannelVisibility_ReconnectDuringFanOutActsOnLiveClient(t *testing.T) { + ctx := context.Background() + database := newHarvestVoiceDB(t) + uid := seedHarvestVoiceUser(t, database, "refresh-race-user") + chID := mustCreateVoiceChannel(t, database, "refresh-race-channel") + ch, err := database.GetChannel(ctx, chID) + if err != nil || ch == nil { + t.Fatalf("GetChannel: %v", err) + } + + // Bare hub (svc=nil): h.perms is nil, so RefreshChannelVisibility takes the + // GetUserByID+GetRoleByID branch this test targets. + h := NewHub(database, auth.NewRateLimiter(), nil) + + user, err := database.GetUserByID(ctx, uid) + if err != nil || user == nil { + t.Fatalf("GetUserByID: %v", err) + } + + sendA := make(chan []byte, 8) + a := NewTestClientWithUser(h, user, chID, sendA) + h.RegisterNowForTest(a) + if !h.SubscribedToChannelTopicForTest(a, chID) { + t.Fatal("setup: original client not subscribed to its focused channel") + } + + // Revoke READ_MESSAGES for the harvest-voice role on this channel — this is + // the channel_overrides change that makes RefreshChannelVisibility decide + // the fan-out target must lose the channel. + if err := database.UpsertChannelOverride(ctx, chID, harvestVoiceRoleID, 0, permissions.ReadMessages); err != nil { + t.Fatalf("UpsertChannelOverride: %v", err) + } + + sendB := make(chan []byte, 8) + var hookRan bool + refreshChannelVisibilityRaceHook = func(userID int64) { + if userID != uid { + return + } + hookRan = true + // Simulate a reconnect landing exactly between the permission lookup + // above and the send/unsubscribe below: a fresh connection replaces + // the original in h.clients under the same user ID, exactly as + // registerNow does for a real reconnect. + b := NewTestClientWithUser(h, user, chID, sendB) + h.RegisterNowForTest(b) + } + defer func() { refreshChannelVisibilityRaceHook = nil }() + + h.RefreshChannelVisibility(ch) + + if !hookRan { + t.Fatal("refreshChannelVisibilityRaceHook never fired — test setup is broken, not exercising the race window") + } + + // The live replacement, not the stale snapshot pointer, must receive the + // channel_delete. + select { + case raw := <-sendB: + var env struct { + Type string `json:"type"` + } + if err := json.Unmarshal(raw, &env); err != nil { + t.Fatalf("unmarshal message to replacement client: %v", err) + } + if env.Type != "channel_delete" { + t.Errorf("replacement client got type %q, want channel_delete", env.Type) + } + case <-time.After(time.Second): + t.Error("replacement client received nothing — RefreshChannelVisibility acted on the stale, replaced connection instead") + } + + // Look up the live client via the hub rather than the hook's closure + // variable, so the assertion reflects what RefreshChannelVisibility + // actually left behind. + live := h.GetClient(uid) + if live == nil { + t.Fatal("no client registered for user after RefreshChannelVisibility") + } + if h.SubscribedToChannelTopicForTest(live, chID) { + t.Error("replacement client is still subscribed to the channel topic RefreshChannelVisibility decided it must lose") + } + if got := live.getChannelID(); got != 0 { + t.Errorf("replacement client channelID = %d, want 0 (focus must clear on the live client, not a dead one)", got) + } +} diff --git a/Server/ws/presence_invisible_test.go b/Server/ws/presence_invisible_test.go index 6523c913..be502d48 100644 --- a/Server/ws/presence_invisible_test.go +++ b/Server/ws/presence_invisible_test.go @@ -168,6 +168,58 @@ func TestBroadcastPresence_InvisibleSplitsSelfFromEveryoneElse(t *testing.T) { } } +// TestBroadcastPresence_InvisibleBlanksCustomStatusForObservers pins OC-0211: +// BroadcastPresence maps an invisible user's *status* to "offline" for the +// public frame but used to pass customStatus through verbatim, so every +// other connected client received {status:"offline", custom_status:""} — the surviving text is a tell that the "offline" member is +// actually online, exactly what db.MemberSummary.ForViewer deliberately +// blanks for the ready payload. This is the connect/reconnect path +// (announceConnectPresence -> BroadcastPresence), reached whenever an +// invisible user with a saved custom status connects or reconnects. +func TestBroadcastPresence_InvisibleBlanksCustomStatusForObservers(t *testing.T) { + hub, database := newTestHub(t) + go hub.Run() + t.Cleanup(hub.Stop) + + ghost := seedOwnerUser(t, database, "bc-ghost-cs") + other := seedOwnerUser(t, database, "bc-other-cs") + ghostCh := make(chan []byte, 8) + otherCh := make(chan []byte, 8) + gc := ws.NewTestClientWithUser(hub, ghost, 0, ghostCh) + oc := ws.NewTestClientWithUser(hub, other, 0, otherCh) + hub.Register(gc) + hub.Register(oc) + waitRegistered(t, hub, gc) + waitRegistered(t, hub, oc) + + text := "in a meeting" + hub.BroadcastPresence(ghost.ID, db.StatusInvisible, &text) + + self := readPresence(ghostCh, 500*time.Millisecond) + if self == nil { + t.Fatal("owner received no presence message") + } + // The owner must still see their own real custom status. + if self["custom_status"] != text { + t.Errorf("owner custom_status = %v, want %q", self["custom_status"], text) + } + + seen := readPresence(otherCh, 500*time.Millisecond) + if seen == nil { + t.Fatal("other client received no presence message") + } + if seen["status"] != db.StatusOffline { + t.Errorf("other sees status = %v, want offline", seen["status"]) + } + // The leak: an observer must never see the real custom status text + // alongside a collapsed-to-offline status — that combination discloses + // that the member is actually online. + if seen["custom_status"] != nil { + t.Errorf("other sees custom_status = %v, want null (leaked invisible user's real status text)", seen["custom_status"]) + } +} + func TestBroadcastPresence_NonInvisibleGoesToEveryoneUnchanged(t *testing.T) { hub, database := newTestHub(t) go hub.Run() diff --git a/Server/ws/reconnect_visibility_race_test.go b/Server/ws/reconnect_visibility_race_test.go new file mode 100644 index 00000000..65a0b1d5 --- /dev/null +++ b/Server/ws/reconnect_visibility_race_test.go @@ -0,0 +1,157 @@ +package ws + +// reconnect_visibility_race_test.go — regression test for OC-0206. +// +// handleReconnect reads the visibility watermark exactly once, at the very +// top of the handshake (mustFullResync(lastSeq)), then spends the rest of +// the handshake — computeAllowedChannels, plus on a cold-tier resume several +// more DB round trips — before registerNow finally subscribes the client and +// makes it reachable to RefreshChannelVisibility's / revokeUnreadableChannels's +// h.clients fan-out. A visibility change landing in that window is missed +// twice over: the fan-out can't see a client that isn't registered yet, and +// the earlier watermark check has already passed, so nothing forces the +// connection back onto the full-ready path — it resumes via replay holding +// permissions computed before the change. +// +// The DB round trips inside a real reconnect are too fast to reliably land a +// concurrent goroutine inside that window (see the identical justification +// on refreshChannelVisibilityRaceHook in hub_refresh_visibility_race_test.go), +// so handleReconnectPreRegisterRaceHook pins it deterministically instead. + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/coder/websocket" + + "github.com/owncord/server/auth" + "github.com/owncord/server/permissions" +) + +func TestHandleReconnect_VisibilityChangeDuringHandshake_ForcesFullReady(t *testing.T) { + ctx := context.Background() + database := newHarvestVoiceDB(t) + uid := seedHarvestVoiceUser(t, database, "reconnect-visibility-race-user") + chID := mustCreateVoiceChannel(t, database, "reconnect-visibility-race-channel") + ch, err := database.GetChannel(ctx, chID) + if err != nil || ch == nil { + t.Fatalf("GetChannel: %v", err) + } + user, err := database.GetUserByID(ctx, uid) + if err != nil || user == nil { + t.Fatalf("GetUserByID: %v", err) + } + + h := NewHub(database, auth.NewRateLimiter(), nil) + + // Precondition: the channel starts out READ-visible to this user. + allowedBefore, err := h.computeAllowedChannels(ctx, database, user) + if err != nil { + t.Fatalf("computeAllowedChannels: %v", err) + } + if !allowedBefore[chID] { + t.Fatalf("precondition: channel %d must start out READ-visible", chID) + } + + // Seed the ring buffer so a buffer-tier replay is available for last_seq=2, + // and seed h.seq to match its newest entry — bumpVisibilityWatermark reads + // h.seq (the hub's broadcast counter), not the raw seqs pushed directly + // into the ring buffer below, so without this the watermark could never + // move past 0 and mustFullResync would never trip. + rb := h.ReplayBuffer() + rb.Push(1, chID, []byte(`{"seq":1,"type":"chat_message"}`)) + rb.Push(2, chID, []byte(`{"seq":2,"type":"chat_message"}`)) + rb.Push(3, chID, []byte(`{"seq":3,"type":"chat_message"}`)) + h.SeedSeq(3) + const lastSeq = uint64(2) + + if h.mustFullResync(lastSeq) { + t.Fatalf("precondition: mustFullResync must be false before any visibility change") + } + + // Deliberately no pre-registered client for uid: this is a genuine + // reconnect, exactly like the real socket that already dropped and was + // already removed from h.clients. + c := NewTestClientWithUser(h, user, 0, make(chan []byte, 8)) + + // A real server-side *websocket.Conn so the (buggy) success path's writes + // (auth_ok + replay) succeed instead of panicking on a nil conn. + connCh := make(chan *websocket.Conn, 1) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + conn, acceptErr := websocket.Accept(w, r, nil) + if acceptErr != nil { + return + } + connCh <- conn + })) + defer srv.Close() + + dialCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + clientConn, dialResp, dialErr := websocket.Dial(dialCtx, "ws"+strings.TrimPrefix(srv.URL, "http"), nil) + if dialResp != nil && dialResp.Body != nil { + _ = dialResp.Body.Close() + } + if dialErr != nil { + t.Fatalf("dial: %v", dialErr) + } + defer func() { _ = clientConn.Close(websocket.StatusNormalClosure, "") }() + + var conn *websocket.Conn + select { + case conn = <-connCh: + case <-time.After(5 * time.Second): + t.Fatal("server never accepted the connection") + } + defer func() { _ = conn.Close(websocket.StatusNormalClosure, "") }() + + // Fires once, deep inside the handshake — after mustFullResync's initial + // check and after computeAllowedChannels already snapshotted the + // still-permissive allowed set. Revoke READ_MESSAGES and run the exact + // fan-out RefreshChannelVisibility performs for a real admin edit: since c + // is not registered yet, the targeted channel_delete reaches nobody. + var hookRan bool + handleReconnectPreRegisterRaceHook = func() { + hookRan = true + if overrideErr := database.UpsertChannelOverride(ctx, chID, harvestVoiceRoleID, 0, permissions.ReadMessages); overrideErr != nil { + t.Fatalf("UpsertChannelOverride: %v", overrideErr) + } + // nolint:contextcheck // RefreshChannelVisibility takes no context by + // design: it is reached through the admin HubBroadcaster interface, + // which carries none, so it builds its own internally. contextcheck + // only flags it here because this closure happens to hold a ctx for + // the override write above; there is nothing to propagate. + h.RefreshChannelVisibility(ch) + } + defer func() { handleReconnectPreRegisterRaceHook = nil }() + + handled, startPumps := h.handleReconnect(ctx, conn, c, database, lastSeq) + + if !hookRan { + t.Fatal("handleReconnectPreRegisterRaceHook never fired — test setup is broken, not exercising the race window") + } + + // A change that happened this deep into the handshake was never delivered + // to this connection (it wasn't registered yet) and must instead force a + // fall-through to the full-ready path, not a resume carrying stale + // permissions. + if handled { + t.Errorf("handleReconnect: handled=true after a visibility change landed mid-handshake, want false (fall through to handleFreshConnect)") + } + if startPumps { + t.Errorf("handleReconnect: startPumps=true after a visibility change landed mid-handshake, want false") + } + if live := h.GetClient(uid); live != nil { + t.Errorf("handleReconnect registered the client with permissions computed before the mid-handshake visibility change") + } + + // Sanity: the watermark itself must reflect the change, or nothing above + // could ever have caught it. + if w := h.visibilityChangeSeq.Load(); w == 0 { + t.Fatalf("test setup: visibilityChangeSeq never moved off 0, the hook's RefreshChannelVisibility call did not bump it") + } +} diff --git a/Server/ws/serve.go b/Server/ws/serve.go index 9e910c3c..7ffdea3f 100644 --- a/Server/ws/serve.go +++ b/Server/ws/serve.go @@ -118,6 +118,16 @@ func (h *Hub) upgradeAndAuth( return c, lastSeq, nil } +// handleReconnectPreRegisterRaceHook, when non-nil, runs once inside +// handleReconnect's h.seqMu critical section immediately before the +// mustFullResync re-check that guards registerNow. Test-only (nil in +// production); a real visibility change lands too fast relative to the DB +// round trips above to reliably land a concurrent goroutine in this window, +// so tests use this hook to pin it deterministically instead — mirrors the +// refreshChannelVisibilityRaceHook / voiceJoinPostTokenRaceHook pattern used +// for the analogous races elsewhere in this package (OC-0206). +var handleReconnectPreRegisterRaceHook func() + // handleReconnect attempts to resume a client via replay. Its two return // values are independent signals for ServeWS: // - handled reports whether this function owns the outcome of the @@ -362,6 +372,27 @@ func (h *Hub) handleReconnect( return false, false } } + if handleReconnectPreRegisterRaceHook != nil { + handleReconnectPreRegisterRaceHook() + } + // Re-check the watermark one last time, right before registerNow makes + // this connection reachable. RefreshChannelVisibility and + // revokeUnreadableChannels both iterate h.clients to fan out a targeted, + // unsequenced channel_create/channel_delete — a snapshot this + // still-mid-handshake connection is absent from — and both only bump the + // watermark afterward. Without this re-check, a visibility change that + // lands anywhere between the entry check above and here is missed twice: + // the fan-out can't reach an unregistered client, and the entry check has + // already passed, so nothing else catches it before this resume commits + // to permissions computed before the change (OC-0206). + if h.mustFullResync(lastSeq) { + h.seqMu.Unlock() + slog.Warn("ws handleReconnect: visibility changed during handshake, forcing full ready", + "user_id", c.userID, "last_seq", lastSeq) + h.reconnectTierFull.Add(1) + telemetry.NewAppMetrics().WSReconnectTierTotal.Add(ctx, 1, telemetry.String("tier", "full")) + return false, false + } h.registerNow(c, allowedChannelIDs) h.seqMu.Unlock()