diff --git a/Client/tauri-client/src-tauri/tauri.conf.json b/Client/tauri-client/src-tauri/tauri.conf.json index c2872b35..3568e03f 100644 --- a/Client/tauri-client/src-tauri/tauri.conf.json +++ b/Client/tauri-client/src-tauri/tauri.conf.json @@ -19,7 +19,7 @@ "decorations": true, "resizable": true, "center": true, - "additionalBrowserArgs": "--autoplay-policy=no-user-gesture-required --use-fake-ui-for-media-stream" + "additionalBrowserArgs": "--disable-features=msWebOOUI,msPdfOOUI,msSmartScreenProtection --autoplay-policy=no-user-gesture-required --use-fake-ui-for-media-stream" } ], "withGlobalTauri": false, diff --git a/Client/tauri-client/src/components/ChannelSidebar.ts b/Client/tauri-client/src/components/ChannelSidebar.ts index 33da100f..20fd0e71 100644 --- a/Client/tauri-client/src/components/ChannelSidebar.ts +++ b/Client/tauri-client/src/components/ChannelSidebar.ts @@ -866,6 +866,24 @@ export function createChannelSidebar(options: ChannelSidebarOptions): MountableC ); unsubscribers.push(unsubAuth); + // canManageChannels()/canModerateVoice() read authStore.user.role and + // channelsStore.roles at render time, but nothing above re-renders when + // either changes on its own — a MEMBER_UPDATE for the signed-in user + // (dispatcher.ts's self-branch) or a ROLES_UPDATE permission-mask edit + // would otherwise leave the category "+", the channel context menu and + // the voice-moderation menu stale until an unrelated channel/voice event + // happened to fire renderChannels() (OC-0142). + const unsubRole = authStore.subscribeSelector( + (s) => s.user?.role ?? "", + () => renderChannels(), + ); + unsubscribers.push(unsubRole); + const unsubRoles = channelsStore.subscribeSelector( + (s) => s.roles, + () => renderChannels(), + ); + unsubscribers.push(unsubRoles); + // Subscribe to UI store for category collapse changes const unsubUi = uiStore.subscribeSelector( (s) => s.collapsedCategories, diff --git a/Client/tauri-client/src/components/QuickSwitcher.ts b/Client/tauri-client/src/components/QuickSwitcher.ts index 846b47e6..610838a2 100644 --- a/Client/tauri-client/src/components/QuickSwitcher.ts +++ b/Client/tauri-client/src/components/QuickSwitcher.ts @@ -152,11 +152,14 @@ export function createQuickSwitcher(options: QuickSwitcherOptions): MountableCom } function handleGlobalKeydown(e: KeyboardEvent): void { - if ((e.ctrlKey || e.metaKey) && e.key === "k") { - e.preventDefault(); - if (root !== null && root.parentNode !== null) { - options.onClose(); - } + // Same case-insensitive, altKey-excluding match as + // OverlayManagers.ts's open handler (OC-0150) — otherwise this close + // path goes dead under CapsLock and AltGr swallows a keystroke for + // nothing. + if (!(e.ctrlKey || e.metaKey) || e.altKey || e.key.toLowerCase() !== "k") return; + e.preventDefault(); + if (root !== null && root.parentNode !== null) { + options.onClose(); } } diff --git a/Client/tauri-client/src/components/message-list/content-parser.ts b/Client/tauri-client/src/components/message-list/content-parser.ts index bb9b24dc..40760031 100644 --- a/Client/tauri-client/src/components/message-list/content-parser.ts +++ b/Client/tauri-client/src/components/message-list/content-parser.ts @@ -168,7 +168,15 @@ export function renderMentions(text: string, info?: MentionInfo): DocumentFragme } // Strip trailing punctuation that is likely sentence-level, not part of the URL const rawUrl = match[0]; - const stripped = rawUrl.replace(/[.,;:!?)]+$/, ""); + let stripped = rawUrl.replace(/[.,;:!?)]+$/, ""); + // Give back one trailing ")" if it balances an unmatched "(" earlier in + // the URL — e.g. https://en.wikipedia.org/wiki/Rust_(programming_language) + // is a real address, not prose wrapped in parens. + if (rawUrl.length > stripped.length && rawUrl[stripped.length] === ")") { + const opens = (stripped.match(/\(/g) ?? []).length; + const closes = (stripped.match(/\)/g) ?? []).length; + if (opens > closes) stripped = stripped + ")"; + } const trailing = rawUrl.slice(stripped.length); const url = stripped || rawUrl; // fallback if stripping emptied it if (isSafeUrl(url)) { diff --git a/Client/tauri-client/src/lib/audioElements.ts b/Client/tauri-client/src/lib/audioElements.ts index 3aed23e5..59bfdf8c 100644 --- a/Client/tauri-client/src/lib/audioElements.ts +++ b/Client/tauri-client/src/lib/audioElements.ts @@ -129,13 +129,20 @@ export class AudioElements { const userId = parseUserId(participant.identity); if (publication.source === Track.Source.ScreenShareAudio) { // Screenshare audio: manage via HTMLAudioElement volume (not participant.setVolume) - for (const el of track.detach()) el.remove(); + // Look up the tracking set before detaching so a fast re-subscribe + // (new TrackSubscribed before the old TrackUnsubscribed lands) drops + // the stale element from the set instead of leaking it forever — same + // hygiene as handleTrackUnsubscribedAudio below. + let audioEls = this.screenshareAudioElements.get(userId); + for (const el of track.detach()) { + el.remove(); + audioEls?.delete(el); + } const audioEl = track.attach(); audioEl.style.display = "none"; document.body.appendChild(audioEl); audioEl.volume = this.getEffectiveScreenshareVolume(userId); audioEl.muted = this.screenshareAudioMutedByUser.get(userId) ?? false; - let audioEls = this.screenshareAudioElements.get(userId); if (audioEls === undefined) { audioEls = new Set(); this.screenshareAudioElements.set(userId, audioEls); diff --git a/Client/tauri-client/src/lib/dispatcher.ts b/Client/tauri-client/src/lib/dispatcher.ts index 14ecbb35..a0eccaa7 100644 --- a/Client/tauri-client/src/lib/dispatcher.ts +++ b/Client/tauri-client/src/lib/dispatcher.ts @@ -1025,10 +1025,16 @@ export function wireDispatcher( // send/reaction rollback, not a capacity refusal) — this is the one // place every remaining server error lands (a rejected fire-and-forget // chat_edit, for one), so it must not be silently dropped just because - // it isn't RATE_LIMITED/FORBIDDEN. Set synchronously, independent of - // the video-rollback lookup below: both paths produce this exact same - // message, so there is nothing left to gate on that lookup resolving. - setTransientError(payload.message || "Server error"); + // it isn't RATE_LIMITED/FORBIDDEN. transientError has exactly one + // reader — ConnectPage's login-screen banner — so writing it here is + // invisible for the whole time the user is in-app (MainPage never + // subscribes) and only resurfaces, stale and out of context, next time + // the login screen mounts (OC-0064). Use the same in-app toast the + // sibling CHANNEL_FULL/VIDEO_LIMIT branches above already use. Fire + // synchronously, independent of the video-rollback lookup below: both + // paths react to this exact same message, so there is nothing left to + // gate on that lookup resolving. + showToast(payload.message || "Server error", "error"); // A server refusal of a voice_camera/voice_screenshare enable other // than VIDEO_LIMIT (FORBIDDEN, RATE_LIMITED, INTERNAL, ...): roll back diff --git a/Client/tauri-client/src/lib/identity.ts b/Client/tauri-client/src/lib/identity.ts index a5313d50..44d293a9 100644 --- a/Client/tauri-client/src/lib/identity.ts +++ b/Client/tauri-client/src/lib/identity.ts @@ -207,9 +207,19 @@ const identityKeyPairCache = new Map>(); /** Composite keyring/memo key scoping the identity keypair by host AND user * id. The keyring commands only take a single opaque `host` string, so the * scope is folded into that one field rather than requiring a Rust-side - * change. */ + * change. + * + * `userId` goes BEFORE `host`, joined with `@` rather than `:` (OC-0118): + * `isValidHost` (api.ts) forbids '@' in any host — DNS name, IPv4, or + * bracketed/bare IPv6 literal — so a scoped key can never collide with a + * legacy host-only account (`identity:{host}`, pre-B3-3) OR with another + * host's literal "host:port" string. The old `${host}:${userId}` format + * had neither guarantee: `identityScopeKey("chat.example", 8443)` produced + * the same string, "chat.example:8443", as the legacy host-only account of + * a *different* server reachable at host "chat.example:8443" — silently + * adopting that server's identity private key. */ function identityScopeKey(host: string, userId: number): string { - return `${host}:${userId}`; + return `${userId}@${host}`; } /** diff --git a/Client/tauri-client/src/lib/livekitE2EE.ts b/Client/tauri-client/src/lib/livekitE2EE.ts index f82ac7ab..7dc8b147 100644 --- a/Client/tauri-client/src/lib/livekitE2EE.ts +++ b/Client/tauri-client/src/lib/livekitE2EE.ts @@ -53,6 +53,12 @@ export class E2EEManager { private _roomKey: Uint8Array | null = null; /** Peer ECDH public keys indexed by userId. */ private _peerPublicKeys: Map = new Map(); + /** Ephemeral keys we've seen superseded for a given peer this session + * (base64), indexed by userId. A signed announce carries no channel/epoch/ + * nonce (F3), so a validly-signed announce replays cleanly — this blocks a + * replay of a key we already moved a peer off of from overwriting their + * current live key (OC-0011). */ + private _retiredPeerKeys: Map> = new Map(); /** This client's long-term ECDSA identity keypair (F3 TOFU), used to sign our * ephemeral announces. Loaded lazily from the OS keyring, cached per session. */ private _identityKeyPair: CryptoKeyPair | null = null; @@ -158,6 +164,7 @@ export class E2EEManager { return false; } this._peerPublicKeys.clear(); + this._retiredPeerKeys.clear(); clearPeerVerifications(); const myPubKeyBase64 = await exportPublicKey(ecdhKeyPair.publicKey); // Build the signed announce up front — this loads the identity key from @@ -668,6 +675,23 @@ export class E2EEManager { return run; } + /** True if `publicKeyBase64` is a key we've already moved this peer off of + * in the current session (see `_retiredPeerKeys`). */ + private isRetiredPeerKey(userId: number, publicKeyBase64: string): boolean { + return this._retiredPeerKeys.get(userId)?.has(publicKeyBase64) ?? false; + } + + /** Record that `publicKeyBase64` is no longer this peer's live key — + * a later announce carrying it again is a replay, not a legitimate change. */ + private retirePeerKey(userId: number, publicKeyBase64: string): void { + const retired = this._retiredPeerKeys.get(userId); + if (retired) { + retired.add(publicKeyBase64); + } else { + this._retiredPeerKeys.set(userId, new Set([publicKeyBase64])); + } + } + private async handleAnnounceInner( userId: number, publicKeyBase64: string, @@ -713,12 +737,42 @@ export class E2EEManager { isDuplicate = true; log.debug("E2EE: duplicate announce — will re-send offer if key holder", { userId }); } else { + // Reject a replay of a key we've already retired for this peer. The + // signed announce message carries no channel/epoch/nonce (F3), so an + // old, validly-signed announce replays cleanly — without this check a + // malicious relay could re-emit a recorded announce and swap the live + // key back to one nobody holds anymore, silently blackholing the peer + // (OC-0011). A genuine peer never reuses an ephemeral key across + // sessions (freshly generated every join), so this never rejects a + // legitimate re-announce. + if (this.isRetiredPeerKey(userId, publicKeyBase64)) { + log.error("E2EE: rejecting replayed peer key announce (previously retired)", { + userId, + }); + return; + } + this.retirePeerKey(userId, existingB64); peerKey = await importPublicKey(publicKeyBase64); log.warn("E2EE: peer public key changed (reconnect?)", { userId }); } } else { + if (this.isRetiredPeerKey(userId, publicKeyBase64)) { + log.error("E2EE: rejecting replayed peer key announce (previously retired)", { userId }); + return; + } peerKey = await importPublicKey(publicKeyBase64); } + // Re-check after the export/import awaits above: a clearState()+rejoin + // landing during either one must not have this stale continuation write + // a torn-down session's peer key into the map a NEW session now owns — + // the generation guard above only covers the window up to verification, + // not this later await (OC-0010). + if (this._sessionGeneration !== myGeneration) { + log.info("E2EE: discarding stale announce (session torn down during key import)", { + userId, + }); + return; + } if (!isDuplicate) { this._peerPublicKeys.set(userId, peerKey); log.info("E2EE: received peer public key", { userId }); @@ -828,6 +882,19 @@ export class E2EEManager { await this.keyProvider.setKey(roomKeyToBase64(this._roomKey)); log.info("E2EE: room key received and applied", { fromUserId }); + // Re-check after the setKey await too: the guard above only covers the + // window up to unwrap, not this call. A teardown-and-rejoin-as-holder + // landing here would otherwise have this stale continuation read the + // NEW session's live _isKeyHolder/_roomKeyResolver below and stand it + // down / resolve it — corrupting a session this attempt no longer owns + // (OC-0010). + if (this._e2eeEpoch !== epochBefore || this._ecdhKeyPair !== keypair) { + log.info("E2EE: discarding stale offer after setKey (epoch or session keypair changed)", { + fromUserId, + }); + return; + } + // Accepting an offer proves the sender is the server-authoritative key // holder (the server gates outgoing offers on IsVoiceKeyHolder), so if we // still think we hold the key, we have been re-elected away — a lower @@ -1179,6 +1246,7 @@ export class E2EEManager { this._ecdhKeyPair = null; this._roomKey = null; this._peerPublicKeys.clear(); + this._retiredPeerKeys.clear(); clearPeerVerifications(); this._isKeyHolder = false; this._rotatingKey = false; diff --git a/Client/tauri-client/src/main.ts b/Client/tauri-client/src/main.ts index cba4210f..31ca8f5c 100644 --- a/Client/tauri-client/src/main.ts +++ b/Client/tauri-client/src/main.ts @@ -36,6 +36,7 @@ import { createCertMismatchModal, createCertFirstUseModal } from "@components/Ce import { reconnectAfterCertAccept } from "@lib/cert-reconnect"; import { createProfileManager, createTauriBackend } from "@lib/profiles"; import type { CertTofuEvent } from "@lib/ws"; +import { saveUserStatus } from "@lib/userStatus"; import { openUrl } from "@tauri-apps/plugin-opener"; import { listen } from "@tauri-apps/api/event"; @@ -243,15 +244,23 @@ ws.onCertMismatch((evt: CertTofuEvent) => { void ws.startCertListener(); // Route the tray's Status submenu (Online/Idle/Do Not Disturb/Offline) into -// the same presence_update wire message the in-app StatusPicker sends -// (UserBar.ts/MainPage.ts's applyPresence) — the Rust side only emitted -// "status-change" with nothing in the webview listening for it. ws.send is a -// safe no-op (logged) when there is no live session, so no auth guard is -// needed here. +// the same path the in-app StatusPicker uses (UserBar.ts): persist through +// saveUserStatus() — lib/userStatus.ts's documented single source of truth — +// before sending the wire message, not just a raw ws.send. Without this the +// tray's choice never reaches loadUserStatus(), so notifications.ts's DND +// gate, autoIdle's "never touch a manual DND/invisible" guard, and +// restoreSavedPresence() on the next reconnect all silently disagree with +// what the tray just set (OC-0037). ws.send is a safe no-op (logged) when +// there is no live session, so no auth guard is needed here. void listen("status-change", (e) => { const status = e.payload; if (status === "online" || status === "idle" || status === "dnd" || status === "offline") { - ws.send({ type: "presence_update", payload: { status } }); + // The tray's legacy "offline" spelling maps to "invisible" the same way + // userStatus.ts migrates an old client's stored "offline" value (see its + // doc comment) — the local pref and the wire message must agree. + const mapped = status === "offline" ? "invisible" : status; + saveUserStatus(mapped); + ws.send({ type: "presence_update", payload: { status: mapped } }); } }); @@ -379,33 +388,12 @@ async function renderPage(pageId: "connect" | "main"): Promise { log.debug("WS state change", { state: wsState }); if (wsState === "connected") { // Stop listening once connected so a later transition can't fire this - // handler again (which would append a second overlay). + // handler again. unsubState(); // Pre-warm the lazily-loaded MainPage chunk (and the LiveKit stack // behind it) so navigating past the connected overlay doesn't wait // on a dynamic import. void import("@pages/MainPage"); - const auth = authStore.getState(); - // Ensure exactly one overlay exists at a time. - connectedOverlay?.destroy(); - connectedOverlay = createConnectedOverlay({ - serverName: auth.serverName ?? host, - username: auth.user?.username ?? username, - motd: auth.motd ?? "", - onReady: () => { - connectedOverlay?.destroy(); - connectedOverlay = null; - router.navigate("main"); - }, - }); - appEl!.appendChild(connectedOverlay.element); - connectedOverlay.show(); - - const unsubReady = ws.on("ready", () => { - unsubReady(); - connectedOverlay?.markReady(); - }); - sessionUnsubs.push(unsubReady); } else if (wsState === "disconnected") { // Terminal non-connected transition (auth_error, cert-mismatch reject, // or intentional disconnect before ever connecting): drop the handler @@ -415,6 +403,38 @@ async function renderPage(pageId: "connect" | "main"): Promise { }); sessionUnsubs.push(unsubState); + // Build the connected overlay from the auth_ok payload itself, not + // authStore: ws.ts fires onStateChange("connected") synchronously BEFORE + // dispatching the auth_ok message that carries server_name/motd + // (setState() then dispatch() in the same handleMessage() call), so + // authStore.setAuth() — run by the dispatcher's own auth_ok handler — + // has not applied yet at that point. Reading straight from the payload + // sidesteps the race instead of racing it (OC-0063). + const unsubAuthOk = ws.on("auth_ok", (payload) => { + unsubAuthOk(); + // Ensure exactly one overlay exists at a time. + connectedOverlay?.destroy(); + connectedOverlay = createConnectedOverlay({ + serverName: payload.server_name ?? host, + username: payload.user.username ?? username, + motd: payload.motd ?? "", + onReady: () => { + connectedOverlay?.destroy(); + connectedOverlay = null; + router.navigate("main"); + }, + }); + appEl!.appendChild(connectedOverlay.element); + connectedOverlay.show(); + + const unsubReady = ws.on("ready", () => { + unsubReady(); + connectedOverlay?.markReady(); + }); + sessionUnsubs.push(unsubReady); + }); + sessionUnsubs.push(unsubAuthOk); + sessionCleanup = () => { for (const unsub of sessionUnsubs) unsub(); sessionUnsubs.length = 0; @@ -521,28 +541,32 @@ async function renderPage(pageId: "connect" | "main"): Promise { log.error("TOTP submit without pending partial token"); return; } - try { - const result = await api.verifyTotp(code, pendingTotpPartialToken); - if (result.token) { - const remember = connectPage.getRememberPassword(); - const savedPassword = remember ? connectPage.getPassword() : undefined; - ensureProfileExists( - pendingTotpHost, - pendingTotpUsername, - remember, - connectPage.getAutoConnect(), - ); - wirePostAuth( - pendingTotpHost, - result.token, - pendingTotpUsername, - savedPassword, - remember, - ); - } - } finally { - // Clear sensitive partial token immediately after use (success or failure) + const result = await api.verifyTotp(code, pendingTotpPartialToken); + if (result.token) { + // Clear the sensitive partial token now that it has been + // exchanged for a real session token. A rejected code must NOT + // clear it here — the TOTP *code* is single-use (the server + // 401s a replay), but the partial token is the short-lived 2FA + // challenge itself and stays valid for a retry. LoginForm keeps + // the TOTP overlay open across a failed verify for exactly that + // retry; clearing this unconditionally (the old `finally`) made + // every retry hit the guard above and silently do nothing. pendingTotpPartialToken = ""; + const remember = connectPage.getRememberPassword(); + const savedPassword = remember ? connectPage.getPassword() : undefined; + ensureProfileExists( + pendingTotpHost, + pendingTotpUsername, + remember, + connectPage.getAutoConnect(), + ); + wirePostAuth( + pendingTotpHost, + result.token, + pendingTotpUsername, + savedPassword, + remember, + ); } }, onAddProfile(name, host) { diff --git a/Client/tauri-client/src/pages/MainPage.ts b/Client/tauri-client/src/pages/MainPage.ts index b072afec..d16e17bf 100644 --- a/Client/tauri-client/src/pages/MainPage.ts +++ b/Client/tauri-client/src/pages/MainPage.ts @@ -519,7 +519,12 @@ export function createMainPage(options: MainPageOptions): MountableComponent { children.push(settingsOverlay); // Quick switcher (Ctrl+K) - const qsManager = createQuickSwitcherManager(() => root); + // Don't fire while the settings panel is on top of it — same guard as + // attachGlobalKeybinds below, reading the same source of truth. + const qsManager = createQuickSwitcherManager( + () => root, + () => uiStore.getState().settingsOpen, + ); unsubscribers.push(qsManager.attach()); // The rest of the shortcuts listed on the settings Keybinds tab. diff --git a/Client/tauri-client/src/pages/connect-page/LoginForm.ts b/Client/tauri-client/src/pages/connect-page/LoginForm.ts index a9a39bd6..50250443 100644 --- a/Client/tauri-client/src/pages/connect-page/LoginForm.ts +++ b/Client/tauri-client/src/pages/connect-page/LoginForm.ts @@ -82,6 +82,11 @@ export function createLoginForm(opts: LoginFormOptions): LoginFormApi { let formState: FormState = "idle"; let formMode: FormMode = "login"; let errorMessage = ""; + // True while a TOTP challenge is outstanding (from showTotp() until it is + // cancelled or resolved). A rejected verify moves formState to "error" for + // the banner/shake, but the overlay must stay up so the code can be + // re-entered — see updateTotpOverlay(). + let totpPending = false; // --- cached DOM references --- let formTitle: HTMLHeadingElement; @@ -521,6 +526,11 @@ export function createLoginForm(opts: LoginFormOptions): LoginFormApi { totpOverlay.classList.remove("totp-overlay--hidden"); totpInput.value = ""; totpInput.focus(); + } else if (formState === "error" && totpPending) { + // A rejected verify lands here — keep the overlay up (and the + // already-entered code in place) instead of dropping the user back on + // the login form with no way to retry. + totpOverlay.classList.remove("totp-overlay--hidden"); } else { totpOverlay.classList.add("totp-overlay--hidden"); } @@ -672,6 +682,7 @@ export function createLoginForm(opts: LoginFormOptions): LoginFormApi { } function handleTotpCancel(): void { + totpPending = false; transitionTo("idle"); } @@ -686,6 +697,7 @@ export function createLoginForm(opts: LoginFormOptions): LoginFormApi { autoConnectOverlayElement: autoConnectOverlay, showTotp(): void { + totpPending = true; transitionTo("totp"); }, @@ -703,6 +715,7 @@ export function createLoginForm(opts: LoginFormOptions): LoginFormApi { }, resetToIdle(): void { + totpPending = false; transitionTo("idle"); }, diff --git a/Client/tauri-client/src/pages/main-page/ChannelController.ts b/Client/tauri-client/src/pages/main-page/ChannelController.ts index df7c90c9..28284b6d 100644 --- a/Client/tauri-client/src/pages/main-page/ChannelController.ts +++ b/Client/tauri-client/src/pages/main-page/ChannelController.ts @@ -119,7 +119,17 @@ export function createChannelController(opts: ChannelControllerOptions): Channel // controller scope does not turn it into a session-long transcript. const draftByCorrelation = new Map< string, - { content: string; replyTo: number | null; attachments: readonly string[] } + { + content: string; + replyTo: number | null; + attachments: readonly string[]; + // Which channel this cid was actually sent to. chat_send_ok and the + // SLOW_MODE error are global ws.on subscriptions with no channel_id of + // their own (OC-0059) — a late frame for a send made in a channel the + // user has since left must not be attributed to whatever channel is + // mounted when it arrives. + channelId: number; + } >(); function destroyChannel(): void { @@ -223,7 +233,7 @@ export function createChannelController(opts: ChannelControllerOptions): Channel // failed row with retry rather than silently dropping the message. const cid = crypto.randomUUID(); addOptimisticMessage({ correlationId: cid, channelId, user, content, replyTo, timestamp }); - draftByCorrelation.set(cid, { content, replyTo, attachments }); + draftByCorrelation.set(cid, { content, replyTo, attachments, channelId }); markSendFailed(cid, "OFFLINE"); return; } @@ -232,7 +242,7 @@ export function createChannelController(opts: ChannelControllerOptions): Channel payload: { channel_id: channelId, content, reply_to: replyTo, attachments }, }); addOptimisticMessage({ correlationId: cid, channelId, user, content, replyTo, timestamp }); - draftByCorrelation.set(cid, { content, replyTo, attachments }); + draftByCorrelation.set(cid, { content, replyTo, attachments, channelId }); } function retrySend(correlationId: string): void { @@ -453,9 +463,23 @@ export function createChannelController(opts: ChannelControllerOptions): Channel }; composerGatingUnsubs.push(stopSlowModeTicker); + // Both chat_send_ok and the SLOW_MODE error are global ws.on + // subscriptions carrying no channel_id of their own — only the + // correlation id ties a frame back to the send that produced it. A send + // made in a channel the user has since left can still be in flight when + // its ack/refusal arrives, and by then this listener belongs to whatever + // channel is newly mounted (OC-0059). Absent/empty correlation ids never + // happen over the real transport, so fall back to attributing to the + // mounted channel rather than silently dropping every ack. + const sentToMountedChannel = (correlationId: string | undefined): boolean => + correlationId === undefined || + correlationId === "" || + draftByCorrelation.get(correlationId)?.channelId === channelId; + // The server accepted a message — the next one is subject to the cooldown. composerGatingUnsubs.push( ws.on("chat_send_ok", (_payload, correlationId) => { + const sameChannel = sentToMountedChannel(correlationId); // An accepted send can never be retried, so its draft is dead weight. // The map is controller-scoped (a failed row outlives a channel // switch, so its draft must too), which means nothing else would ever @@ -464,7 +488,7 @@ export function createChannelController(opts: ChannelControllerOptions): Channel draftByCorrelation.delete(correlationId); } const ch = channelsStore.getState().channels.get(channelId); - if (ch !== undefined && ch.id === channelsStore.getState().activeChannelId) { + if (ch !== undefined && ch.id === channelsStore.getState().activeChannelId && sameChannel) { startSlowMode(ch.slowMode); } }), @@ -472,8 +496,9 @@ export function createChannelController(opts: ChannelControllerOptions): Channel // A refused send restarts the full window: the server's limiter is the // authority on when the next one is allowed. composerGatingUnsubs.push( - ws.on("error", (payload) => { + ws.on("error", (payload, correlationId) => { if (payload.code !== "SLOW_MODE") return; + if (!sentToMountedChannel(correlationId)) return; const ch = channelsStore.getState().channels.get(channelId); if (ch !== undefined) startSlowMode(ch.slowMode); }), diff --git a/Client/tauri-client/src/pages/main-page/OverlayManagers.ts b/Client/tauri-client/src/pages/main-page/OverlayManagers.ts index 22167a18..6f6ff33f 100644 --- a/Client/tauri-client/src/pages/main-page/OverlayManagers.ts +++ b/Client/tauri-client/src/pages/main-page/OverlayManagers.ts @@ -92,6 +92,8 @@ export interface QuickSwitcherManager { export function createQuickSwitcherManager( getRoot: () => HTMLDivElement | null, + /** Optional: suppress the shortcut while another overlay owns input (e.g. Settings). */ + isSuspended?: () => boolean, ): QuickSwitcherManager { let instance: MountableComponent | null = null; @@ -116,13 +118,18 @@ export function createQuickSwitcherManager( function attach(): () => void { const handler = (e: KeyboardEvent): void => { - if ((e.ctrlKey || e.metaKey) && e.key === "k") { - e.preventDefault(); - if (instance !== null) { - close(); - } else { - open(); - } + // Mirrors GlobalKeybinds.ts's guard: `e.key` is layout-dependent and + // uppercases under CapsLock/Shift, so compare case-insensitively; + // exclude altKey so AltGr (reported as ctrlKey+altKey on Windows) + // doesn't swallow a non-US character; and honour the same suspension + // every other app-wide shortcut respects. + if (!(e.ctrlKey || e.metaKey) || e.altKey || e.key.toLowerCase() !== "k") return; + if (isSuspended?.() === true) return; + e.preventDefault(); + if (instance !== null) { + close(); + } else { + open(); } }; document.addEventListener("keydown", handler); diff --git a/Client/tauri-client/src/pages/main-page/SidebarArea.ts b/Client/tauri-client/src/pages/main-page/SidebarArea.ts index d0355e1f..b137641a 100644 --- a/Client/tauri-client/src/pages/main-page/SidebarArea.ts +++ b/Client/tauri-client/src/pages/main-page/SidebarArea.ts @@ -232,9 +232,12 @@ export function createSidebarArea(opts: SidebarAreaOptions): SidebarAreaResult { sidebarWrapper.appendChild(serverHeader); - // Load per-server collapsed category state from localStorage - const initialServerName = authStore.getState().serverName ?? "Server"; - loadCollapsedCategories(initialServerName); + // Load per-server collapsed category state from localStorage, scoped to + // the connected host (not the display name) — the same convention as + // setChannelMutesHost/setNsfwGateHost/setAudioVolumeHost. The display name + // defaults to "OwnCord Server" on every unmodified install, so keying on + // it would collapse two different servers' saved state onto one entry. + loadCollapsedCategories(api.getConfig().host); // Keep server name in sync with auth store const unsubServerName = authStore.subscribeSelector( @@ -541,12 +544,21 @@ export function createSidebarArea(opts: SidebarAreaOptions): SidebarAreaResult { if (channelBeforeDm !== null) { setActiveChannel(channelBeforeDm); channelBeforeDm = null; - } else { - for (const ch of channelsStore.getState().channels.values()) { - if (ch.type === "text") { - setActiveChannel(ch.id); - break; - } + return; + } + // No saved channel — this happens when DM mode was entered without + // going through selectDmConversation (e.g. SidebarDmSection's "View + // all messages" button, which does a bare setSidebarMode). If a real + // non-DM channel is already active, leave it alone instead of + // silently jumping to the first text channel in Map order. + const st = channelsStore.getState(); + const current = + st.activeChannelId !== null ? st.channels.get(st.activeChannelId) : undefined; + if (current !== undefined && current.type !== "dm") return; + for (const ch of channelsStore.getState().channels.values()) { + if (ch.type === "text") { + setActiveChannel(ch.id); + break; } } }, diff --git a/Client/tauri-client/src/stores/auth.store.ts b/Client/tauri-client/src/stores/auth.store.ts index 5294a733..ac865f3b 100644 --- a/Client/tauri-client/src/stores/auth.store.ts +++ b/Client/tauri-client/src/stores/auth.store.ts @@ -8,6 +8,7 @@ import type { UserWithRole } from "@lib/types"; import { resetVoiceStore, voiceStore } from "@stores/voice.store"; import { resetMessagesStore } from "@stores/messages.store"; import { resetChannelsStore } from "@stores/channels.store"; +import { resetBlocksStore } from "@stores/blocks.store"; import { cleanupNotificationAudio } from "@lib/notifications"; import { clearNsfwAcknowledgements } from "@lib/nsfw-gate"; import { createLogger } from "@lib/logger"; @@ -71,7 +72,11 @@ export function setAuth(token: string, user: UserWithRole, serverName: string, m * previous session's messages, and same-account relogin would leave a * permanent hole for messages posted while logged out. Also clears * channelsStore: setChannels' DM-row carry otherwise re-inserts the - * previous server's DM channel rows into the next server's channel map. */ + * previous server's DM channel rows into the next server's channel map. + * Also clears blocksStore: block state is keyed by user id, which (like + * channel/message ids) is only unique per-server — otherwise a previous + * server's blocked-user ids would gate DM composers on the next server + * until the next successful GET /blocks refetch. */ export function clearAuth(reason: LogoutReason = "user"): void { // livekitSession (and the ~1.3 MB livekit-client SDK behind it) is loaded // lazily so it stays out of the startup path. Only import it when there is @@ -91,6 +96,7 @@ export function clearAuth(reason: LogoutReason = "user"): void { resetVoiceStore(); resetMessagesStore(); resetChannelsStore(); + resetBlocksStore(); // NSFW acknowledgements are per-viewer consent, not per-device: without this // the next account signed into the same server inherits the previous user's // acks and the age gate silently never appears for them. Host-scoping the diff --git a/Client/tauri-client/src/stores/blocks.store.ts b/Client/tauri-client/src/stores/blocks.store.ts index d800e5bb..fec91125 100644 --- a/Client/tauri-client/src/stores/blocks.store.ts +++ b/Client/tauri-client/src/stores/blocks.store.ts @@ -64,6 +64,13 @@ export function clearBlockedByThem(): void { ); } +/** Reset both block directions (called on clearAuth — user ids are only + * unique per-server, so a previous server's block list must not carry + * into the next session). */ +export function resetBlocksStore(): void { + blocksStore.setState(() => INITIAL); +} + /** * The composer disable reason for a DM with `recipientId`, or null if unblocked. * blockedByMe takes precedence so the user always sees that they are the blocker. diff --git a/Client/tauri-client/src/styles/app.css b/Client/tauri-client/src/styles/app.css index 9ae476ad..5ae3839a 100644 --- a/Client/tauri-client/src/styles/app.css +++ b/Client/tauri-client/src/styles/app.css @@ -5024,6 +5024,10 @@ ul.md-list-nested { object-fit: contain; } +.video-cell.track-muted video { + visibility: hidden; +} + .video-username { position: absolute; bottom: 8px; diff --git a/Client/tauri-client/tests/unit/audio-elements.test.ts b/Client/tauri-client/tests/unit/audio-elements.test.ts index 0f604094..b8141e5c 100644 --- a/Client/tauri-client/tests/unit/audio-elements.test.ts +++ b/Client/tauri-client/tests/unit/audio-elements.test.ts @@ -127,6 +127,42 @@ describe("AudioElements", () => { expect(audioEl.muted).toBe(true); }); + + it("does not leak the previously-attached element into the tracking set on a fast re-subscribe (OC-0135)", () => { + // LiveKit can fire TrackSubscribed for an already-attached screenshare- + // audio track before the old TrackUnsubscribed lands (fast reconnect). + // The same underlying track's detach() then returns the element from + // the prior attach(), which the handler removes from the DOM but must + // also drop from screenshareAudioElements — otherwise it lives on in + // the Set forever. + let lastEl: HTMLAudioElement | null = null; + const track = { + kind: "audio", + sid: "track-ss-resub", + attach: vi.fn(() => { + const el = document.createElement("audio"); + lastEl = el; + return el; + }), + detach: vi.fn(() => (lastEl === null ? [] : [lastEl])), + }; + const publication = { source: "screenShareAudio" }; + const participant = { identity: "user-42", setVolume: vi.fn() }; + + elements.handleTrackSubscribedAudio(track as any, publication as any, participant as any); + const firstEl = lastEl as unknown as HTMLAudioElement; + + // Re-fire subscribe for the same track before any unsubscribe arrives. + elements.handleTrackSubscribedAudio(track as any, publication as any, participant as any); + const secondEl = lastEl as unknown as HTMLAudioElement; + + const trackedEls = (elements as any).screenshareAudioElements.get( + 42, + ) as Set; + expect(trackedEls.has(firstEl)).toBe(false); + expect(trackedEls.has(secondEl)).toBe(true); + expect(trackedEls.size).toBe(1); + }); }); describe("handleTrackUnsubscribedAudio", () => { diff --git a/Client/tauri-client/tests/unit/auth-store.test.ts b/Client/tauri-client/tests/unit/auth-store.test.ts new file mode 100644 index 00000000..81873cee --- /dev/null +++ b/Client/tauri-client/tests/unit/auth-store.test.ts @@ -0,0 +1,32 @@ +import { describe, it, expect } from "vitest"; +import { clearAuth } from "../../src/stores/auth.store"; +import { + blocksStore, + setUserBlockedByMe, + setUserBlockedByThem, +} from "../../src/stores/blocks.store"; + +describe("clearAuth", () => { + it("resets blocksStore.blockedByMe so the next server's session doesn't inherit it", () => { + // Server A: block user 7. + setUserBlockedByMe(7, true); + expect(blocksStore.getState().blockedByMe.has(7)).toBe(true); + + // Log out (as UserBar disconnect / Settings logout / quick-switch does). + clearAuth(); + + // Server B: user id 7 is an unrelated person. A previous server's block + // must not still gate their DM composer / offer "Unblock" for them. + expect(blocksStore.getState().blockedByMe.has(7)).toBe(false); + expect(blocksStore.getState().blockedByMe.size).toBe(0); + }); + + it("resets blocksStore.blockedByThem too", () => { + setUserBlockedByThem(9, true); + expect(blocksStore.getState().blockedByThem.has(9)).toBe(true); + + clearAuth(); + + expect(blocksStore.getState().blockedByThem.has(9)).toBe(false); + }); +}); diff --git a/Client/tauri-client/tests/unit/channel-controller.test.ts b/Client/tauri-client/tests/unit/channel-controller.test.ts index aef22054..257634f7 100644 --- a/Client/tauri-client/tests/unit/channel-controller.test.ts +++ b/Client/tauri-client/tests/unit/channel-controller.test.ts @@ -1426,6 +1426,56 @@ describe("createChannelController", () => { expect(mockSetDisabled).toHaveBeenLastCalledWith(null); }); + it("does not gate the newly mounted channel with a late ack for a message sent in the previous channel (OC-0059)", () => { + // Channel A has slow mode; channel B does too, with a different value, + // so a misattributed ack is unmistakable. + setChannels([ + { + id: 42, + name: "general", + type: "text", + category: null, + position: 0, + can_send: true, + slow_mode: 5, + }, + { + id: 43, + name: "other", + type: "text", + category: null, + position: 0, + can_send: true, + slow_mode: 7, + }, + ]); + setActiveChannel(42); + const opts = makeOpts(); + let n = 0; + (opts.ws.send as ReturnType).mockImplementation(() => `cid-${++n}`); + const ctrl = createChannelController(opts); + ctrl.mountChannel(42, "general"); + + // cid-1 is channel_focus; the chat_send in A gets cid-2. The ack for it + // does not arrive before the user switches away. + capturedMessageInputOpts.onSend("hello", null, []); + + // Switch to channel B before A's ack arrives. + setActiveChannel(43); + ctrl.mountChannel(43, "other"); + mockSetDisabled.mockClear(); + + // A's late chat_send_ok now arrives; only B's handler is subscribed. + const ackCalls = (opts.ws.on as ReturnType).mock.calls.filter( + (c: unknown[]) => c[0] === "chat_send_ok", + ); + const onAck = ackCalls[ackCalls.length - 1]![1] as (payload: unknown, id?: string) => void; + onAck({ message_id: 7, timestamp: "2024-01-01T00:00:00Z" }, "cid-2"); + + // B was never sent to and must not be gated by A's cooldown. + expect(mockSetDisabled).not.toHaveBeenCalledWith(expect.stringContaining("Slow mode")); + }); + it("stops the countdown when the channel unmounts", () => { vi.useFakeTimers(); try { diff --git a/Client/tauri-client/tests/unit/channel-sidebar.test.ts b/Client/tauri-client/tests/unit/channel-sidebar.test.ts index 39b4e859..8566697e 100644 --- a/Client/tauri-client/tests/unit/channel-sidebar.test.ts +++ b/Client/tauri-client/tests/unit/channel-sidebar.test.ts @@ -1260,6 +1260,70 @@ describe("ChannelSidebar", () => { expect(container.querySelector(".category-add-btn")).toBeNull(); }); + // ── Live role-change repaint (OC-0142) ── + // canManageChannels() is evaluated at render time from authStore.user.role, + // but the sidebar's only authStore subscription selects serverName. A + // MEMBER_UPDATE for the signed-in user (dispatcher.ts writes the new role + // via updateUser) must still cause a repaint without any unrelated event. + + it("repaints channel-management affordances when the signed-in user's own role changes", () => { + const onCreateChannel = vi.fn(); + sidebar.destroy?.(); + authStore.setState(() => ({ + token: "tok", + user: { id: 2, username: "Member", avatar: null, role: "member" }, + serverName: "Test Server", + motd: null, + isAuthenticated: true, + })); + sidebar = createChannelSidebar({ onVoiceJoin, onVoiceLeave, onCreateChannel }); + + setChannels(testChannels); + sidebar.mount(container); + + // Starts as a plain member: no "+" button. + expect(container.querySelector(".category-add-btn")).toBeNull(); + + // Promoted to admin (mirrors dispatcher.ts's MEMBER_UPDATE self-branch, + // which patches authStore via updateUser({ role })) — no channel/voice + // event fires alongside it. + authStore.setState((prev) => ({ + ...prev, + user: prev.user === null ? null : { ...prev.user, role: "admin" }, + })); + authStore.flush(); + + expect(container.querySelector(".category-add-btn")).not.toBeNull(); + }); + + it("repaints channel-management affordances when the role list's permission mask changes", () => { + const onCreateChannel = vi.fn(); + sidebar.destroy?.(); + setRoles([{ id: 3, name: "Moderator", color: null, permissions: Permission.SEND_MESSAGES }]); + authStore.setState(() => ({ + token: "tok", + user: { id: 3, username: "Mod", avatar: null, role: "moderator" }, + serverName: "Test Server", + motd: null, + isAuthenticated: true, + })); + sidebar = createChannelSidebar({ onVoiceJoin, onVoiceLeave, onCreateChannel }); + + setChannels(testChannels); + sidebar.mount(container); + + // Moderator role holds no MANAGE_CHANNELS bit yet. + expect(container.querySelector(".category-add-btn")).toBeNull(); + + // A ROLES_UPDATE grants MANAGE_CHANNELS to the same role (dispatcher.ts's + // ROLES_UPDATE handler replaces the whole list via setRoles) — again with + // no accompanying channel/voice event. + setRoles([{ id: 3, name: "Moderator", color: null, permissions: Permission.MANAGE_CHANNELS }]); + channelsStore.flush(); + + expect(container.querySelector(".category-add-btn")).not.toBeNull(); + }); + // ── Voice user volume context menu ── it("right-click on other user's voice row opens volume context menu", () => { diff --git a/Client/tauri-client/tests/unit/dispatcher.test.ts b/Client/tauri-client/tests/unit/dispatcher.test.ts index e0817f1a..5fa561a7 100644 --- a/Client/tauri-client/tests/unit/dispatcher.test.ts +++ b/Client/tauri-client/tests/unit/dispatcher.test.ts @@ -2348,36 +2348,47 @@ describe("WS Dispatcher", () => { expect(mock.ws.disconnect).toHaveBeenCalled(); }); - it("wires error RATE_LIMITED to transient error", () => { + // OC-0064: transientError has exactly one reader in the whole client — + // ConnectPage's login-screen subscription. Routing the catch-all fallback + // through it means an error raised while the user is in-app (MainPage + // never subscribes) is invisible until the user later lands back on the + // login screen, where it resurfaces stale and out of context. The + // catch-all must use the same in-app toast the sibling CHANNEL_FULL / + // VIDEO_LIMIT branches already use, and must leave transientError alone. + it("wires error RATE_LIMITED to an in-app toast (OC-0064)", () => { + mockShowToast.mockClear(); mock.dispatch("error", { code: "RATE_LIMITED", message: "Too many requests", }); - const error = uiStore.getState().transientError; - expect(error).toBe("Too many requests"); + expect(mockShowToast).toHaveBeenCalledWith("Too many requests", "error"); + expect(uiStore.getState().transientError).toBeNull(); }); - it("wires error FORBIDDEN to transient error", () => { + it("wires error FORBIDDEN to an in-app toast (OC-0064)", () => { + mockShowToast.mockClear(); mock.dispatch("error", { code: "FORBIDDEN", message: "Insufficient permissions", }); - const error = uiStore.getState().transientError; - expect(error).toBe("Insufficient permissions"); + expect(mockShowToast).toHaveBeenCalledWith("Insufficient permissions", "error"); + expect(uiStore.getState().transientError).toBeNull(); }); - it("wires error RATE_LIMITED with empty message uses default", () => { + it("wires error RATE_LIMITED with empty message uses default (OC-0064)", () => { + mockShowToast.mockClear(); mock.dispatch("error", { code: "RATE_LIMITED", message: "" }); - const error = uiStore.getState().transientError; - expect(error).toBe("Server error"); + expect(mockShowToast).toHaveBeenCalledWith("Server error", "error"); + expect(uiStore.getState().transientError).toBeNull(); }); - it("wires error with an unrecognized code to the generic fallback banner", () => { + it("wires error with an unrecognized code to the generic fallback toast (OC-0064)", () => { // The final fallthrough is the one place every unmatched error code // lands (e.g. a rejected fire-and-forget chat_edit) — it must not be // silently dropped just because it isn't RATE_LIMITED/FORBIDDEN. + mockShowToast.mockClear(); uiStore.setState((prev) => ({ ...prev, transientError: null })); mock.dispatch("error", { @@ -2385,19 +2396,22 @@ describe("WS Dispatcher", () => { message: "Something odd", }); - expect(uiStore.getState().transientError).toBe("Something odd"); + expect(mockShowToast).toHaveBeenCalledWith("Something odd", "error"); + expect(uiStore.getState().transientError).toBeNull(); }); - it("wires a BAD_REQUEST error with no pending correlation (e.g. a rejected chat_edit) to a transient error", () => { + it("wires a BAD_REQUEST error with no pending correlation (e.g. a rejected chat_edit) to an in-app toast (OC-0064)", () => { // chat_edit is fire-and-forget: it never enters pendingSends, so a // rejection's envelope id matches nothing above and used to fall through // this handler silently, leaving the user's edited text destroyed with // no error shown (only RATE_LIMITED/FORBIDDEN were bannered). + mockShowToast.mockClear(); uiStore.setState((prev) => ({ ...prev, transientError: null })); mock.dispatch("error", { code: "BAD_REQUEST", message: "Message too long" }, "edit-id-1"); - expect(uiStore.getState().transientError).toBe("Message too long"); + expect(mockShowToast).toHaveBeenCalledWith("Message too long", "error"); + expect(uiStore.getState().transientError).toBeNull(); }); it("wires an error carrying a pending send id to mark that row failed (not a toast)", () => { @@ -3448,8 +3462,11 @@ describe("WS Dispatcher", () => { vi.mocked(mockDisableCamera).mockClear(); vi.mocked(mockDisableScreenshare).mockClear(); uiStore.setState((prev) => ({ ...prev, transientError: null })); + mockShowToast.mockClear(); }); + // OC-0064: the catch-all now toasts in-app instead of latching + // transientError (which only the login screen ever reads). it("rolls back the camera publish on a correlated refusal", async () => { vi.mocked(mockRollbackPendingVideo).mockReturnValue("camera"); @@ -3459,7 +3476,8 @@ describe("WS Dispatcher", () => { expect(mockRollbackPendingVideo).toHaveBeenCalledWith("vid-1"); expect(mockDisableCamera).toHaveBeenCalled(); expect(mockDisableScreenshare).not.toHaveBeenCalled(); - expect(uiStore.getState().transientError).toBe("no permission"); + expect(mockShowToast).toHaveBeenCalledWith("no permission", "error"); + expect(uiStore.getState().transientError).toBeNull(); }); it("rolls back the screenshare publish on a correlated refusal", async () => { @@ -3470,17 +3488,19 @@ describe("WS Dispatcher", () => { expect(mockDisableScreenshare).toHaveBeenCalled(); expect(mockDisableCamera).not.toHaveBeenCalled(); - expect(uiStore.getState().transientError).toBe("Server error"); + expect(mockShowToast).toHaveBeenCalledWith("Server error", "error"); + expect(uiStore.getState().transientError).toBeNull(); }); - it("leaves an uncorrelated refusal as a plain transient error — no rollback", () => { + it("leaves an uncorrelated refusal as a plain in-app toast — no rollback", () => { vi.mocked(mockRollbackPendingVideo).mockReturnValue(undefined); mock.dispatch("error", { code: "FORBIDDEN", message: "nope" }, "unrelated-id"); expect(mockDisableCamera).not.toHaveBeenCalled(); expect(mockDisableScreenshare).not.toHaveBeenCalled(); - expect(uiStore.getState().transientError).toBe("nope"); + expect(mockShowToast).toHaveBeenCalledWith("nope", "error"); + expect(uiStore.getState().transientError).toBeNull(); }); }); }); diff --git a/Client/tauri-client/tests/unit/identity.test.ts b/Client/tauri-client/tests/unit/identity.test.ts index 7515512b..87abceca 100644 --- a/Client/tauri-client/tests/unit/identity.test.ts +++ b/Client/tauri-client/tests/unit/identity.test.ts @@ -25,7 +25,7 @@ import { authStore } from "@stores/auth.store"; /** * Stateful keyring double for the legacy-migration tests: a Map keyed by the * exact `host` string each command receives (the scoped account - * `chat.example:1` and the legacy account `chat.example` are just different + * `1@chat.example` and the legacy account `chat.example` are just different * keys in the same map), so save/delete on one account cannot be confused * with another the way a host-agnostic mock would. */ @@ -169,7 +169,7 @@ describe("getOrCreateIdentityKeyPair", () => { expect(saveCall).toBeDefined(); // Scoped by host AND user id (B3-3) — not just host — so two accounts // signed into the same host never share a keyring blob. - expect((saveCall![1] as { host: string }).host).toBe("chat.example:1"); + expect((saveCall![1] as { host: string }).host).toBe("1@chat.example"); }); it("reloads the persisted keypair on subsequent logins (no regenerate)", async () => { @@ -253,6 +253,26 @@ describe("getOrCreateIdentityKeyPair", () => { expect(await exportPublicKey(userB.publicKey)).not.toBe(await exportPublicKey(userA.publicKey)); }); + it("[OC-0118] a scoped host+userId account never collides with a legacy host-only account for a DIFFERENT host", async () => { + // Pre-B3-3 install on some other server reachable as "chat.example:8443" + // (host string carries an explicit port) stored its identity key under + // the legacy host-only keyring account `identity:chat.example:8443`. A + // completely different server reachable as "chat.example" (port 443) + // signs in as the user whose id happens to be 8443: + // identityScopeKey("chat.example", 8443) must NOT produce the same + // string "chat.example:8443" as that unrelated legacy account, or this + // login silently adopts (and later re-publishes) the other server's + // identity private key. + const otherServerLegacyKey = await generateIdentityKeyPair(); + const otherServerLegacyBlob = await exportIdentityKeyPair(otherServerLegacyKey.privateKey); + const otherServerLegacyPub = await exportPublicKey(otherServerLegacyKey.publicKey); + keyringDouble({ "chat.example:8443": otherServerLegacyBlob }); + + const kp = await getOrCreateIdentityKeyPair("chat.example", 8443); + + expect(await exportPublicKey(kp.publicKey)).not.toBe(otherServerLegacyPub); + }); + it("reports a credential store that accepts the write but drops the value", async () => { invokeMock.mockImplementation((cmd: string) => { if (cmd === "load_identity_key") return Promise.resolve(null); @@ -446,7 +466,7 @@ describe("ensureIdentityKeyPublished (login/ready publish flow)", () => { // The legacy key must be untouched: no adopt-then-delete into a bogus // host:0 scope. expect(store.get("chat.example")).toBe(legacyBlob); - expect(store.has("chat.example:0")).toBe(false); + expect(store.has("0@chat.example")).toBe(false); }); }); @@ -460,7 +480,7 @@ describe("legacy identity key migration (pre-B3-3 host-only account)", () => { const kp = await getOrCreateIdentityKeyPair("chat.example", 1); expect(await exportPublicKey(kp.publicKey)).toBe(legacyPub); - expect(store.get("chat.example:1")).toBe(legacyBlob); + expect(store.get("1@chat.example")).toBe(legacyBlob); // Deleted so it can never be adopted a second time. expect(store.has("chat.example")).toBe(false); }); @@ -476,8 +496,8 @@ describe("legacy identity key migration (pre-B3-3 host-only account)", () => { const second = await getOrCreateIdentityKeyPair("chat.example", 2); expect(await exportPublicKey(second.publicKey)).not.toBe(legacyPub); - expect(store.get("chat.example:2")).toBeDefined(); - expect(store.get("chat.example:2")).not.toBe(legacyBlob); + expect(store.get("2@chat.example")).toBeDefined(); + expect(store.get("2@chat.example")).not.toBe(legacyBlob); }); it("falls back to fresh generation, without throwing, when the legacy blob is corrupt", async () => { @@ -486,8 +506,8 @@ describe("legacy identity key migration (pre-B3-3 host-only account)", () => { const kp = await getOrCreateIdentityKeyPair("chat.example", 1); expect(kp.publicKey).toBeDefined(); - expect(store.get("chat.example:1")).toBeDefined(); - expect(store.get("chat.example:1")).not.toBe("!!not-valid-jwk!!"); + expect(store.get("1@chat.example")).toBeDefined(); + expect(store.get("1@chat.example")).not.toBe("!!not-valid-jwk!!"); }); it("generates fresh, with no delete attempt, when there is no legacy key either (first login)", async () => { @@ -512,6 +532,6 @@ describe("legacy identity key migration (pre-B3-3 host-only account)", () => { await getOrCreateIdentityKeyPair("chat.example", 1); expect(store.get("chat.example")).toBe(legacyBlob); - expect(store.has("chat.example:1")).toBe(false); + expect(store.has("1@chat.example")).toBe(false); }); }); diff --git a/Client/tauri-client/tests/unit/livekit-e2ee.test.ts b/Client/tauri-client/tests/unit/livekit-e2ee.test.ts index c96b557f..473ead1b 100644 --- a/Client/tauri-client/tests/unit/livekit-e2ee.test.ts +++ b/Client/tauri-client/tests/unit/livekit-e2ee.test.ts @@ -87,6 +87,7 @@ import { generateECDHKeyPair, generateRoomKey, importPublicKey, + exportPublicKey, } from "@lib/e2eeCrypto"; import { getOrCreateIdentityKeyPair, getIdentityPin, storeIdentityPin } from "@lib/identity"; import { authStore } from "@stores/auth.store"; @@ -981,6 +982,125 @@ describe("E2EEManager", () => { } }); + // ── Ledger findings OC-0010 / OC-0011 ───────────────────────────────── + + it("[OC-0010] does not stand down a new session's key-holder role when a stale offer's setKey resolves after teardown+rejoin", async () => { + const ws = { send: vi.fn() }; + const mgr = createManager(ws); + + // Session A: we are the holder in channel 1, with PEER_ID's key on file. + await mgr.setupKeyExchange(true, 1); + await mgr.handleAnnounce(PEER_ID, "cGVlcg==", "sig"); + + // An offer from PEER_ID arrives and stalls at the keyProvider.setKey + // await — AFTER the epoch/keypair guard (checked right after unwrap) has + // already passed. + let releaseSetKey!: () => void; + const stalledSetKey = new Promise((resolve) => { + releaseSetKey = resolve; + }); + mockSetKey.mockClear(); + mockSetKey.mockImplementationOnce(() => stalledSetKey); + const offerPromise = mgr.handleOffer(PEER_ID, "enc", "iv"); + await vi.waitFor(() => expect(mockSetKey).toHaveBeenCalled()); + + // Mid-flight: the user leaves channel 1 and rejoins channel 2 as the new + // key holder — a distinct keypair, exactly as real ECDH keygen produces. + mgr.clearState(); + vi.mocked(generateECDHKeyPair).mockResolvedValueOnce({ + publicKey: { type: "chan2-pub" } as unknown as CryptoKey, + privateKey: { type: "chan2-priv" } as unknown as CryptoKey, + }); + await mgr.setupKeyExchange(true, 2); + expect((mgr as unknown as { _isKeyHolder: boolean })._isKeyHolder).toBe(true); + + // The stale (session-1) offer's setKey now resolves. + releaseSetKey(); + await offerPromise; + + // Channel 2's holder role must survive — the stale continuation must not + // stand it down (it re-checks staleness before the setKey await, not + // after — the write happens on the far side of that await). + expect((mgr as unknown as { _isKeyHolder: boolean })._isKeyHolder).toBe(true); + }); + + it("[OC-0010] does not write a stale peer key into a new session's map when clearState()+rejoin lands during the announce's key-import await", async () => { + const ws = { send: vi.fn() }; + const mgr = createManager(ws); + + // Session A: holder in channel 1. + await mgr.setupKeyExchange(true, 1); + + // The announce's importPublicKey stalls — the "final await" before the + // _peerPublicKeys.set write, which today has no re-check after it. + let releaseImport!: (v: CryptoKey) => void; + const stalledImport = new Promise((resolve) => { + releaseImport = resolve; + }); + vi.mocked(importPublicKey).mockReturnValueOnce(stalledImport); + + const announcePromise = mgr.handleAnnounce(PEER_ID, "cGVlcg==", "sig"); + await vi.waitFor(() => expect(importPublicKey).toHaveBeenCalled()); + + // Mid-flight: the user leaves channel 1 and rejoins channel 2. + mgr.clearState(); + await mgr.setupKeyExchange(true, 2); + + // The stale announce's import now resolves. + releaseImport({ type: "stale-peer-key" } as unknown as CryptoKey); + await announcePromise; + + // The new session's peer map must not be polluted by the torn-down + // session's announce. + expect(mgr.peerPublicKeys.has(PEER_ID)).toBe(false); + }); + + it("[OC-0011] rejects a replayed announce carrying a previously-retired peer key instead of overwriting the live key", async () => { + const ws = { send: vi.fn() }; + const mgr = createManager(ws); + await mgr.setupKeyExchange(true, 1); // establishes our keypair + + // Make import/export round-trip faithfully on the announced base64 + // string (the shared mock default returns a fixed constant from + // exportPublicKey regardless of input, which would mask this bug). + vi.mocked(importPublicKey).mockImplementation( + async (b64: string) => ({ type: `peer-key-${b64}` }) as unknown as CryptoKey, + ); + vi.mocked(exportPublicKey).mockImplementation(async (key: CryptoKey) => + (key as unknown as { type: string }).type.replace("peer-key-", ""), + ); + + // Valid base64 (must decode cleanly — rawFromBase64 uses atob() to build + // the signed message bytes). "b2xk"/"bmV3" already prove out elsewhere in + // this suite as distinct valid ephemeral-key payloads. + const KEY_A = "b2xk"; + const KEY_B = "bmV3"; + + try { + // Peer announces key A — accepted as their first (live) key. + await mgr.handleAnnounce(PEER_ID, KEY_A, "sigA"); + expect(mgr.peerPublicKeys.get(PEER_ID)).toEqual({ type: `peer-key-${KEY_A}` }); + + // Peer reconnects and announces a genuinely new key B — a legitimate + // change, so key A is now retired. + await mgr.handleAnnounce(PEER_ID, KEY_B, "sigB"); + expect(mgr.peerPublicKeys.get(PEER_ID)).toEqual({ type: `peer-key-${KEY_B}` }); + + // A malicious relay re-emits the OLD, still validly-signed announce for + // key A. No channel/epoch/nonce binds the signed message, so it + // verifies cleanly — it must still be rejected as a replay, not + // overwrite the live key B. + await mgr.handleAnnounce(PEER_ID, KEY_A, "sigA"); + + expect(mgr.peerPublicKeys.get(PEER_ID)).toEqual({ type: `peer-key-${KEY_B}` }); + } finally { + vi.mocked(importPublicKey).mockImplementation( + async () => ({ type: "public" }) as unknown as CryptoKey, + ); + vi.mocked(exportPublicKey).mockImplementation(async () => "bW9ja2VwaGVtZXJhbA=="); + } + }); + it("[OC-0007] confirms the room key after a reconnect re-announce instead of declaring it fresh unconditionally", async () => { const ws = { send: vi.fn() }; const mgr = createManager(ws); diff --git a/Client/tauri-client/tests/unit/login-form-totp-retry.test.ts b/Client/tauri-client/tests/unit/login-form-totp-retry.test.ts new file mode 100644 index 00000000..6f80377d --- /dev/null +++ b/Client/tauri-client/tests/unit/login-form-totp-retry.test.ts @@ -0,0 +1,76 @@ +// Regression test for OC-0116: a rejected TOTP verify tore down the TOTP +// overlay (transitionTo("error", ...) leaves formState "totp", and +// updateTotpOverlay hides the overlay for every non-"totp" state), so the +// user was dropped back on the login form with no way to re-enter the code. +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { createConnectPage } from "../../src/pages/ConnectPage"; +import type { ConnectPageCallbacks, SimpleProfile } from "../../src/pages/ConnectPage"; + +vi.mock("../../src/lib/credentials", () => ({ + loadCredential: vi.fn().mockResolvedValue(null), +})); + +vi.mock("../../src/components/SettingsOverlay", () => ({ + createSettingsOverlay: () => ({ + mount: vi.fn(), + destroy: vi.fn(), + }), +})); + +function makeCallbacks(overrides: Partial = {}): ConnectPageCallbacks { + return { + onLogin: vi.fn().mockResolvedValue(undefined), + onRegister: vi.fn().mockResolvedValue(undefined), + onTotpSubmit: vi.fn().mockResolvedValue(undefined), + ...overrides, + }; +} + +const testProfiles: SimpleProfile[] = [{ name: "Test Server", host: "localhost:8443" }]; + +describe("LoginForm TOTP retry after a rejected verify", () => { + let container: HTMLDivElement; + + beforeEach(() => { + container = document.createElement("div"); + document.body.appendChild(container); + }); + + afterEach(() => { + container.remove(); + }); + + it("keeps the TOTP overlay open and lets a second code be submitted", async () => { + const onTotpSubmit = vi + .fn() + .mockRejectedValueOnce(new Error("Invalid verification code")) + .mockResolvedValueOnce(undefined); + const page = createConnectPage(makeCallbacks({ onTotpSubmit }), testProfiles); + page.mount(container); + page.showTotp(); + + const totpOverlay = container.querySelector(".totp-overlay") as HTMLDivElement; + const totpInput = container.querySelector(".totp-overlay input") as HTMLInputElement; + const verifyBtn = container.querySelector(".totp-overlay .btn-primary") as HTMLButtonElement; + + totpInput.value = "111111"; + verifyBtn.click(); + + await vi.waitFor(() => expect(onTotpSubmit).toHaveBeenCalledTimes(1)); + // Verify button re-enables once the rejected promise settles. + await vi.waitFor(() => expect(verifyBtn.disabled).toBe(false)); + + // The overlay must stay up so the code can be re-entered, instead of + // being hidden because formState moved to "error". + expect(totpOverlay.classList.contains("totp-overlay--hidden")).toBe(false); + + // A retry with a fresh code must actually reach onTotpSubmit again. + totpInput.value = "222222"; + verifyBtn.click(); + + await vi.waitFor(() => expect(onTotpSubmit).toHaveBeenCalledTimes(2)); + expect(onTotpSubmit).toHaveBeenNthCalledWith(2, "222222"); + + page.destroy?.(); + }); +}); diff --git a/Client/tauri-client/tests/unit/main.test.ts b/Client/tauri-client/tests/unit/main.test.ts new file mode 100644 index 00000000..ebbb63f6 --- /dev/null +++ b/Client/tauri-client/tests/unit/main.test.ts @@ -0,0 +1,223 @@ +/** + * Tests for src/main.ts's post-auth wiring. + * + * main.ts is excluded from unit coverage (vitest.config.ts) — "no seam to + * test below the e2e level; covered by tests/e2e." This file creates one: + * every direct dependency of main.ts that is not needed to observe the two + * behaviors below is stubbed out (mirroring the pattern main-page.test.ts + * uses for MainPage.ts), while ws.ts, authStore, router.ts, safe-render.ts, + * navigation-guard.ts and ConnectedOverlay.ts run for real — so the actual + * event-ordering bug (OC-0063) is exercised, not simulated, and the tray + * listener (OC-0037) is driven through the same Tauri event mock ws.ts's own + * tests use. + * + * Covers: + * - OC-0037: the tray's "status-change" event must persist the choice + * through saveUserStatus() (the documented single source of truth for the + * selected status), not just fire a raw ws.send. + * - OC-0063: the connected overlay must read serverName/motd from the + * auth_ok payload, not from authStore snapshotted before dispatch() has + * run the dispatcher's own auth_ok handler (which is what actually writes + * authStore). + */ +import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; + +// --------------------------------------------------------------------------- +// Tauri API mocks — reuse the ws-mocks.ts event-registry helper so ws.ts's +// real state machine can be driven with simulated Tauri events (same +// mechanism ws-lifecycle.test.ts uses), and so the tray's "status-change" +// listen() call registered by main.ts is capturable via the same +// emitTauriEvent(). +// --------------------------------------------------------------------------- +vi.mock("@tauri-apps/api/core", async () => ({ + invoke: (await import("./helpers/ws-mocks")).mockInvoke, +})); +vi.mock("@tauri-apps/api/event", async () => ({ + listen: (await import("./helpers/ws-mocks")).mockListen, +})); +vi.mock("@tauri-apps/plugin-opener", () => ({ openUrl: vi.fn() })); + +// CSS imports are handled natively by vite/vitest — no mock needed. + +vi.mock("@lib/appearance", () => ({ applyStoredAppearance: vi.fn() })); +vi.mock("@lib/themes", () => ({ restoreTheme: vi.fn() })); +vi.mock("@lib/ptt", () => ({ initPtt: vi.fn().mockResolvedValue(undefined) })); +vi.mock("@lib/logPersistence", () => ({ + initLogPersistence: vi.fn().mockResolvedValue(undefined), + flushLogs: vi.fn().mockResolvedValue(undefined), +})); +vi.mock("@lib/credentials", () => ({ + saveCredential: vi.fn().mockResolvedValue(true), + loadCredential: vi.fn().mockResolvedValue(null), + deleteCredential: vi.fn().mockResolvedValue(undefined), + createUserUpdateCredentialSaver: vi.fn(() => vi.fn()), +})); +vi.mock("@lib/window-state", () => ({ initWindowState: vi.fn().mockResolvedValue(undefined) })); +vi.mock("@lib/deep-link", () => ({ initDeepLinks: vi.fn().mockResolvedValue(undefined) })); +vi.mock("@lib/message-navigation", () => ({ jumpToMessage: vi.fn() })); +vi.mock("@components/CertMismatchModal", () => ({ + createCertMismatchModal: vi.fn(() => ({ mount: vi.fn(), destroy: vi.fn() })), + createCertFirstUseModal: vi.fn(() => ({ mount: vi.fn(), destroy: vi.fn() })), +})); +vi.mock("@lib/cert-reconnect", () => ({ reconnectAfterCertAccept: vi.fn() })); +vi.mock("@lib/profiles", () => ({ + createTauriBackend: vi.fn(() => ({})), + createProfileManager: vi.fn(() => ({ + loadProfiles: vi.fn().mockResolvedValue(undefined), + saveProfiles: vi.fn().mockResolvedValue(undefined), + getAll: vi.fn(() => []), + addProfile: vi.fn((data: unknown) => ({ id: "profile-1", ...(data as object) })), + updateProfile: vi.fn(() => null), + removeProfile: vi.fn(() => true), + getAutoConnectProfile: vi.fn(() => null), + setAutoLogin: vi.fn(), + setLastConnected: vi.fn(), + })), +})); + +// api.ts — only login() is exercised (it drives wirePostAuth); nothing else +// in this flow touches the REST client. +const mockLogin = vi.fn(); +vi.mock("@lib/api", () => ({ + createApiClient: vi.fn(() => ({ + setConfig: vi.fn(), + getConfig: vi.fn(() => ({ host: "" })), + login: (...args: unknown[]) => mockLogin(...args), + getHealth: vi.fn().mockResolvedValue({ version: null, online_users: null }), + })), +})); + +// ConnectPage — captures the real onLogin callback main.ts wires up so the +// test can drive wirePostAuth exactly the way a real login does, without +// building the actual login form DOM. +const capturedConnectCallbacks: { + onLogin?: (host: string, username: string, password: string) => Promise; +} = {}; +vi.mock("@pages/ConnectPage", () => ({ + createConnectPage: vi.fn((callbacks: typeof capturedConnectCallbacks) => { + Object.assign(capturedConnectCallbacks, callbacks); + return { + mount: vi.fn(), + destroy: vi.fn(), + showTotp: vi.fn(), + showConnecting: vi.fn(), + showAutoConnecting: vi.fn(), + showError: vi.fn(), + resetToIdle: vi.fn(), + updateHealthStatus: vi.fn(), + getRememberPassword: vi.fn(() => false), + getAutoConnect: vi.fn(() => false), + getPassword: vi.fn(() => ""), + refreshProfiles: vi.fn(), + selectServer: vi.fn(), + applyInviteLink: vi.fn(), + }; + }), +})); + +// dispatcher.ts pulls in nearly every store/service in the app. Stand in +// with a slim replacement that reproduces the one behavior these tests must +// stay faithful to: the real dispatcher's auth_ok handler calls setAuth() on +// the REAL authStore (imported below, not mocked) — so main.ts's own race +// against that write is exercised unmodified, not sidestepped. +vi.mock("@lib/dispatcher", async () => { + const { authStore, setAuth } = await import("@stores/auth.store"); + return { + wireDispatcher: (ws: { on: (type: string, cb: (payload: unknown) => void) => () => void }) => { + const unsub = ws.on("auth_ok", (payload) => { + const p = payload as { user: unknown; server_name: string; motd: string }; + setAuth(authStore.getState().token ?? "", p.user as never, p.server_name, p.motd); + }); + return () => unsub(); + }, + wireConnectionStatus: vi.fn(() => () => {}), + }; +}); + +import { mockInvoke, eventHandlers, emitTauriEvent } from "./helpers/ws-mocks"; +import { clearAuth } from "@stores/auth.store"; +import { loadUserStatus, loadUserStatusOrigin } from "@lib/userStatus"; + +// --------------------------------------------------------------------------- +// Import the module under test AFTER all mocks are registered. #app must +// exist first: main.ts reads document.getElementById("app") synchronously +// at module top level, and a static `import` line would be hoisted above any +// DOM setup written before it in source order — so this runs inside an async +// beforeAll instead of a top-level import. +// --------------------------------------------------------------------------- +beforeAll(async () => { + document.body.innerHTML = '
'; + await import("../../src/main"); + // Flush the microtask the mocked (async) listen() call resolves on, so the + // "status-change" handler main.ts registers at module load is actually in + // eventHandlers before any test fires it. + await Promise.resolve(); + await Promise.resolve(); +}); + +beforeEach(() => { + vi.useFakeTimers(); + mockInvoke.mockReset().mockResolvedValue(undefined); + localStorage.clear(); + clearAuth(); +}); + +/** Drive a full login → WS connect → auth_ok cycle through the captured + * ConnectPage callback and the real ws.ts client living inside main.ts. */ +async function loginAndReachAuthOk( + host: string, + username: string, + authOkPayload: { user: unknown; server_name: string; motd: string }, +): Promise { + mockLogin.mockResolvedValue({ token: "test-token", requires_2fa: false }); + await capturedConnectCallbacks.onLogin!(host, username, "hunter2"); + await vi.advanceTimersByTimeAsync(10); + emitTauriEvent("ws-state", "open"); + emitTauriEvent("ws-message", JSON.stringify({ type: "auth_ok", payload: authOkPayload })); +} + +describe("main.ts tray status-change listener (OC-0037)", () => { + it("persists a tray-selected status through saveUserStatus, not just the wire", async () => { + expect(eventHandlers.has("status-change")).toBe(true); + + emitTauriEvent("status-change", "dnd"); + + // This is the crux of OC-0037: the tray path must agree with the + // client's own documented "single source of truth" for the selected + // status (lib/userStatus.ts), the same way UserBar's StatusPicker does. + // Before the fix nothing here ever calls saveUserStatus, so this stays + // "online" forever regardless of what the tray sent over the wire. + expect(loadUserStatus()).toBe("dnd"); + expect(loadUserStatusOrigin()).toBe("manual"); + }); + + it("maps the tray's legacy offline value to invisible, matching userStatus.ts's migration", async () => { + emitTauriEvent("status-change", "offline"); + + expect(loadUserStatus()).toBe("invisible"); + }); +}); + +describe("main.ts connected overlay (OC-0063)", () => { + it("shows the auth_ok payload's server_name and motd, not the pre-handshake authStore snapshot", async () => { + await loginAndReachAuthOk("192.168.1.10:8443", "alex", { + user: { id: 1, username: "alex", avatar: null, role: "member" }, + server_name: "My Guild", + motd: "Welcome to My Guild!", + }); + + const overlay = document.querySelector('[data-testid="connected-overlay"]'); + expect(overlay).not.toBeNull(); + + // ws.ts fires onStateChange("connected") synchronously BEFORE dispatching + // the auth_ok message that carries server_name/motd (ws.ts: setState() + // then dispatch() in the same handleMessage() call) — so a handler that + // reads authStore.getState() at that point sees the pre-auth_ok snapshot. + // Reading directly from the payload sidesteps the race. + const motdEl = overlay?.querySelector(".connected-motd"); + expect(motdEl?.textContent).toBe("Welcome to My Guild!"); + + const iconEl = overlay?.querySelector(".connected-srv-icon"); + expect(iconEl?.textContent).toBe("M"); // first letter of "My Guild", not "1" (host) or "" (blank auth) + }); +}); diff --git a/Client/tauri-client/tests/unit/overlay-managers.test.ts b/Client/tauri-client/tests/unit/overlay-managers.test.ts index 3fc6cf33..44a284dd 100644 --- a/Client/tauri-client/tests/unit/overlay-managers.test.ts +++ b/Client/tauri-client/tests/unit/overlay-managers.test.ts @@ -890,6 +890,48 @@ describe("createQuickSwitcherManager", () => { cleanup(); }); + + it("opens on Ctrl+K with CapsLock on (KeyboardEvent.key reports uppercase 'K')", () => { + // OC-0150: `e.key` reflects CapsLock/Shift state. A case-sensitive `=== "k"` + // check means CapsLock (or Ctrl+Shift+K) silently does nothing. + const manager = createQuickSwitcherManager(() => root); + const cleanup = manager.attach(); + + document.dispatchEvent(new KeyboardEvent("keydown", { key: "K", ctrlKey: true })); + + expect(createQuickSwitcher).toHaveBeenCalledOnce(); + + cleanup(); + }); + + it("does not open on AltGr+K (Windows reports AltGr as ctrlKey+altKey)", () => { + // OC-0150: without an altKey exclusion, AltGr-produced characters on + // non-US layouts get swallowed by an unwanted preventDefault(). + const manager = createQuickSwitcherManager(() => root); + const cleanup = manager.attach(); + + document.dispatchEvent(new KeyboardEvent("keydown", { key: "k", ctrlKey: true, altKey: true })); + + expect(createQuickSwitcher).not.toHaveBeenCalled(); + + cleanup(); + }); + + it("does not open while isSuspended() reports true (e.g. settings overlay open)", () => { + // OC-0150: every other global shortcut honours isSuspended; the quick + // switcher never got the guard, so it could stack on top of Settings. + const manager = createQuickSwitcherManager( + () => root, + () => true, + ); + const cleanup = manager.attach(); + + document.dispatchEvent(new KeyboardEvent("keydown", { key: "k", ctrlKey: true })); + + expect(createQuickSwitcher).not.toHaveBeenCalled(); + + cleanup(); + }); }); // --------------------------------------------------------------------------- diff --git a/Client/tauri-client/tests/unit/quick-switcher.test.ts b/Client/tauri-client/tests/unit/quick-switcher.test.ts index 20c441dd..66bd99f7 100644 --- a/Client/tauri-client/tests/unit/quick-switcher.test.ts +++ b/Client/tauri-client/tests/unit/quick-switcher.test.ts @@ -204,6 +204,28 @@ describe("QuickSwitcher", () => { expect(onClose).toHaveBeenCalledOnce(); }); + it("Ctrl+K closes the switcher with CapsLock on (KeyboardEvent.key reports uppercase 'K')", () => { + // OC-0150: the global close handler compares `e.key === "k"` + // case-sensitively, so CapsLock (or Ctrl+Shift+K) leaves the switcher + // stuck open. + switcher.mount(container); + document.dispatchEvent( + new KeyboardEvent("keydown", { key: "K", ctrlKey: true, bubbles: true }), + ); + expect(onClose).toHaveBeenCalledOnce(); + }); + + it("AltGr+K (ctrlKey+altKey) does not close the switcher", () => { + // OC-0150: Windows/WebView2 reports AltGr as ctrlKey+altKey, so without + // an altKey exclusion an AltGr-produced 'k' character both fails to + // reach the composer and closes the switcher underneath it. + switcher.mount(container); + document.dispatchEvent( + new KeyboardEvent("keydown", { key: "k", ctrlKey: true, altKey: true, bubbles: true }), + ); + expect(onClose).not.toHaveBeenCalled(); + }); + it("Enter is a no-op when search returns no results", () => { switcher.mount(container); const input = container.querySelector(".quick-switcher__input") as HTMLInputElement; diff --git a/Client/tauri-client/tests/unit/renderers.test.ts b/Client/tauri-client/tests/unit/renderers.test.ts index e3dc616a..f6d0fbdf 100644 --- a/Client/tauri-client/tests/unit/renderers.test.ts +++ b/Client/tauri-client/tests/unit/renderers.test.ts @@ -1514,6 +1514,29 @@ describe("renderers", () => { expect(container.textContent).toContain("before"); expect(container.textContent).toContain("after"); }); + + it("keeps a balanced trailing paren that is part of the URL", () => { + const url = "https://en.wikipedia.org/wiki/Rust_(programming_language)"; + const fragment = renderMentions(url); + container.appendChild(fragment); + + const link = container.querySelector("a.msg-link") as HTMLAnchorElement; + expect(link).not.toBeNull(); + expect(link.getAttribute("href")).toBe(url); + expect(link.textContent).toBe(url); + // No stray ")" left dangling as separate trailing text + expect(container.textContent).toBe(url); + }); + + it("still strips a genuinely unbalanced trailing paren used as sentence punctuation", () => { + const fragment = renderMentions("(see https://example.com/page)"); + container.appendChild(fragment); + + const link = container.querySelector("a.msg-link") as HTMLAnchorElement; + expect(link).not.toBeNull(); + expect(link.getAttribute("href")).toBe("https://example.com/page"); + expect(container.textContent).toBe("(see https://example.com/page)"); + }); }); // --------------------------------------------------------------------------- diff --git a/Client/tauri-client/tests/unit/sidebar-area.test.ts b/Client/tauri-client/tests/unit/sidebar-area.test.ts index 29a0a2a6..8d11b2d1 100644 --- a/Client/tauri-client/tests/unit/sidebar-area.test.ts +++ b/Client/tauri-client/tests/unit/sidebar-area.test.ts @@ -493,6 +493,39 @@ describe("SidebarArea", () => { }); }); + // ------------------------------------------------------------------------- + // Collapsed category persistence (OC-0085) + // ------------------------------------------------------------------------- + + describe("collapsed category persistence", () => { + afterEach(() => { + localStorage.removeItem("owncord:collapsed:server-a.example.com"); + localStorage.removeItem("owncord:collapsed:OwnCord Server"); + }); + + it("scopes collapsed categories to the connected host, not the server display name", () => { + // Two servers left at the operator default name collide on one + // localStorage entry if persistence is keyed by display name instead + // of host — same reason setChannelMutesHost/setNsfwGateHost/ + // setAudioVolumeHost are all host-scoped. + authStore.setState((prev) => ({ ...prev, serverName: "OwnCord Server" })); + localStorage.setItem("owncord:collapsed:server-a.example.com", JSON.stringify(["General"])); + localStorage.setItem("owncord:collapsed:OwnCord Server", JSON.stringify(["Text Channels"])); + + const opts = defaultOpts(); + (opts.api as unknown as { getConfig: () => { host: string } }).getConfig = () => ({ + host: "server-a.example.com", + }); + + const result = createSidebarArea(opts); + + expect(uiStore.getState().collapsedCategories.has("General")).toBe(true); + expect(uiStore.getState().collapsedCategories.has("Text Channels")).toBe(false); + + cleanup(result); + }); + }); + // ------------------------------------------------------------------------- // Channels mode // ------------------------------------------------------------------------- @@ -1435,6 +1468,65 @@ describe("SidebarArea", () => { cleanup(result); }); + it("onBack keeps the current channel when DM mode was entered without recording channelBeforeDm (OC-0094: 'View all messages' bypass)", () => { + channelsStore.setState((prev) => { + const next = new Map(prev.channels); + next.set(1, { + id: 1, + name: "general", + type: "text", + category: null, + position: 0, + unreadCount: 0, + mentionCount: 0, + lastMessageId: null, + canSend: true, + topic: "", + slowMode: 0, + nsfw: false, + voiceMaxUsers: 0, + voiceMaxVideo: 0, + }); + next.set(2, { + id: 2, + name: "random", + type: "text", + category: null, + position: 0, + unreadCount: 0, + mentionCount: 0, + lastMessageId: null, + canSend: true, + topic: "", + slowMode: 0, + nsfw: false, + voiceMaxUsers: 0, + voiceMaxVideo: 0, + }); + // #random (2) is on screen, and is not first in Map insertion order. + return { ...prev, channels: next, activeChannelId: 2 }; + }); + + // Enter DM mode the way SidebarDmSection's "View all messages" button + // does: a bare setSidebarMode with no selectDmConversation call, so + // channelBeforeDm is never recorded. + uiStore.setState((prev) => ({ ...prev, sidebarMode: "dms" })); + + const result = createSidebarArea(defaultOpts()); + container.appendChild(result.sidebarWrapper); + + const dmSidebarCalls = (createDmSidebar as MockedFn).mock.calls; + const lastCall = dmSidebarCalls[dmSidebarCalls.length - 1]![0]; + lastCall.onBack(); + + expect(uiStore.getState().sidebarMode).toBe("channels"); + // Must not silently jump to #general (1), the first text channel in + // Map iteration order — the user never asked to leave #random. + expect(channelsStore.getState().activeChannelId).toBe(2); + + cleanup(result); + }); + it("onCloseDm removes DM and calls closeDm API", () => { const dm = makeDm({ channelId: 100, diff --git a/Client/tauri-client/tests/unit/tauri-conf-webview2-args.test.ts b/Client/tauri-client/tests/unit/tauri-conf-webview2-args.test.ts new file mode 100644 index 00000000..a282ca17 --- /dev/null +++ b/Client/tauri-client/tests/unit/tauri-conf-webview2-args.test.ts @@ -0,0 +1,31 @@ +// Regression guard for WebView2's default `--disable-features` flag. +// +// Tauri/wry pass `--disable-features=msWebOOUI,msPdfOOUI,msSmartScreenProtection` +// to the WebView2 browser process by default, but setting `additionalBrowserArgs` +// REPLACES that default string rather than appending to it (see +// WindowConfig::additional_browser_args / WebViewBuilder::with_additional_browser_args). +// Our config sets additionalBrowserArgs for autoplay/fake-media-stream, which +// silently re-enables SmartScreen (URL-reputation lookups against Microsoft for +// in-webview navigations/downloads — a leak for a self-hosted, TOFU-pinned +// client) and the msWebOOUI/msPdfOOUI overlays. The dropped default must be +// re-added explicitly. + +import { describe, expect, it } from "vitest"; + +import tauriConf from "../../src-tauri/tauri.conf.json"; + +describe("tauri.conf.json — Windows WebView2 additionalBrowserArgs", () => { + it("keeps wry's default --disable-features flag alongside the custom args", () => { + const win = tauriConf.app.windows[0] as { additionalBrowserArgs?: string }; + expect(win.additionalBrowserArgs).toBeDefined(); + const args = win.additionalBrowserArgs ?? ""; + expect(args).toContain("--disable-features=msWebOOUI,msPdfOOUI,msSmartScreenProtection"); + }); + + it("still passes the autoplay and fake-media-stream flags this client needs", () => { + const win = tauriConf.app.windows[0] as { additionalBrowserArgs?: string }; + const args = win.additionalBrowserArgs ?? ""; + expect(args).toContain("--autoplay-policy=no-user-gesture-required"); + expect(args).toContain("--use-fake-ui-for-media-stream"); + }); +}); diff --git a/Client/tauri-client/tests/unit/video-grid-track-muted-css.test.ts b/Client/tauri-client/tests/unit/video-grid-track-muted-css.test.ts new file mode 100644 index 00000000..453cb6df --- /dev/null +++ b/Client/tauri-client/tests/unit/video-grid-track-muted-css.test.ts @@ -0,0 +1,27 @@ +// jsdom never applies app.css, so a computed-style assertion against the +// rendered tile would pass whether or not the rule exists (see +// appearance-high-contrast.test.ts / status-picker-userbar.test.ts for the +// same pattern). This pins the CSS *source* instead. +// +// VideoGrid.ts's onTrackMute toggles `.track-muted` on the `.video-cell` to +// hide a stalled remote camera's last frame. If app.css has no rule for that +// class, the toggle is a no-op and the viewer keeps seeing a frozen frame +// with no indication the track stalled. +import { readFileSync } from "node:fs"; +import { join } from "node:path"; +import { describe, it, expect } from "vitest"; + +describe("VideoGrid track-muted CSS", () => { + it("app.css hides the video element while .video-cell.track-muted is active", () => { + const css = readFileSync(join(process.cwd(), "src/styles/app.css"), "utf8"); + + // Look for a rule targeting the video (or the cell itself) scoped under + // .video-cell.track-muted -- accept either ordering / whitespace. + const match = /\.video-cell\.track-muted[^{]*\{([^}]*)\}/.exec(css); + expect( + match, + "expected a `.video-cell.track-muted { ... }` (or descendant `video`) rule in app.css " + + "so the mute handler's class toggle actually hides the stalled frame", + ).not.toBeNull(); + }); +}); diff --git a/Server/admin/api.go b/Server/admin/api.go index d09e7ead..df767ee6 100644 --- a/Server/admin/api.go +++ b/Server/admin/api.go @@ -2,6 +2,7 @@ package admin import ( "net/http" + "time" "github.com/go-chi/chi/v5" "github.com/owncord/server/auth" @@ -11,6 +12,48 @@ import ( "github.com/owncord/server/updater" ) +// setupLimiterReapInterval and setupLimiterReapMaxWindow control how often +// the setup endpoint's dedicated rate limiter reaps stale window entries. +// Vars, not consts, so tests can shrink them instead of waiting on the real +// interval (see export_test.go). +var ( + setupLimiterReapInterval = 5 * time.Minute + setupLimiterReapMaxWindow = 15 * time.Minute +) + +// setupLimiterHook, when non-nil, receives the *auth.RateLimiter NewAdminAPI +// creates for the /setup endpoint. Test-only seam: NewAdminAPI returns only +// an http.Handler, so tests otherwise have no way to reach that limiter to +// verify it gets reaped. +var setupLimiterHook func(*auth.RateLimiter) + +// startSetupLimiterReap keeps rl's window map bounded for the life of the +// process. Every distinct source IP that ever hits POST /setup leaves an +// entry that Allow itself only prunes on a repeat call from that same key — +// a one-shot caller's entry sits forever unless something sweeps the whole +// map. api/router.go reaps its own limiter with RateLimiter.StartCleanup, a +// goroutine parked in a ticker select until a stop channel closes — but +// NewAdminAPI has no shutdown hook and is called directly by ~180 tests that +// never capture one, so a parked goroutine here would leak under every +// test's goleak check. time.AfterFunc self-rescheduling avoids that: between +// fires there is no live goroutine, only a runtime timer, so nothing needs +// to stop it. +func startSetupLimiterReap(rl *auth.RateLimiter) { + // Capture the timing once, synchronously, on the caller's goroutine. + // The rescheduled AfterFunc callbacks below must never re-read the + // package vars themselves: those callbacks run on their own goroutine + // indefinitely (nothing stops the chain), so a later test's + // SetSetupLimiterReapTiming restoring the vars on its own goroutine + // would otherwise race an in-flight reap here. + interval, maxWindow := setupLimiterReapInterval, setupLimiterReapMaxWindow + var reap func() + reap = func() { + rl.Cleanup(maxWindow) + time.AfterFunc(interval, reap) + } + time.AfterFunc(interval, reap) +} + // ─── NewAdminAPI ────────────────────────────────────────────────────────────── // NewAdminAPI returns a chi router with all /admin/api/* routes. All routes @@ -32,6 +75,10 @@ func NewAdminAPI(database *db.DB, version string, hub HubBroadcaster, u *updater // Setup endpoints — unauthenticated, only functional when no users exist. setupLimiter := auth.NewRateLimiter() + if setupLimiterHook != nil { + setupLimiterHook(setupLimiter) + } + startSetupLimiterReap(setupLimiter) r.Get("/setup/status", handleSetupStatus(database, setupOpts)) r.Post("/setup", handleSetup(database, setupLimiter, allowedOrigins, hub, setupOpts)) diff --git a/Server/admin/api_test.go b/Server/admin/api_test.go index a516f153..0e478790 100644 --- a/Server/admin/api_test.go +++ b/Server/admin/api_test.go @@ -1479,6 +1479,49 @@ func TestAdminAPI_CreateAPIToken_MissingLabel(t *testing.T) { } } +// TestAdminAPI_CreateAPIToken_NegativeExpiresHours pins OC-0145: a caller that +// asks for a bounded credential (negative expires_hours) must not silently +// receive a permanent one. The `> 0` check in handleCreateAPIToken sends any +// negative value down the nil-expiresAt ("never expires") branch. +func TestAdminAPI_CreateAPIToken_NegativeExpiresHours(t *testing.T) { + database := openAdminTestDB(t) + handler := admin.NewAdminAPI(database, "1.0.0", &mockHub{}, nil, nil, nil, nil, newTestModService(database), newTestRoleService(database)) + token := createAdminUser(t, database) + + w := doRequest(t, handler, http.MethodPost, "/tokens", token, map[string]any{"label": "neg-hours", "expires_hours": -1}) + if w.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400; body: %s", w.Code, w.Body.String()) + } + + tokens, _ := database.ListAPITokens(context.Background()) + for _, tok := range tokens { + if tok.Label == "neg-hours" { + t.Fatalf("negative expires_hours must not mint a token, got %+v", tok) + } + } +} + +// TestAdminAPI_CreateAPIToken_HugeExpiresHours pins OC-0145's overflow half: a +// huge expires_hours must not silently overflow time.Duration into a past +// timestamp and hand back a token that 401s on first use. +func TestAdminAPI_CreateAPIToken_HugeExpiresHours(t *testing.T) { + database := openAdminTestDB(t) + handler := admin.NewAdminAPI(database, "1.0.0", &mockHub{}, nil, nil, nil, nil, newTestModService(database), newTestRoleService(database)) + token := createAdminUser(t, database) + + w := doRequest(t, handler, http.MethodPost, "/tokens", token, map[string]any{"label": "huge-hours", "expires_hours": 3000000}) + if w.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400; body: %s", w.Code, w.Body.String()) + } + + tokens, _ := database.ListAPITokens(context.Background()) + for _, tok := range tokens { + if tok.Label == "huge-hours" { + t.Fatalf("out-of-range expires_hours must not mint a token, got %+v", tok) + } + } +} + func TestAdminAPI_ListAPITokens_OK(t *testing.T) { database := openAdminTestDB(t) handler := admin.NewAdminAPI(database, "1.0.0", &mockHub{}, nil, nil, nil, nil, newTestModService(database), newTestRoleService(database)) diff --git a/Server/admin/export_test.go b/Server/admin/export_test.go index bb03e862..caafa546 100644 --- a/Server/admin/export_test.go +++ b/Server/admin/export_test.go @@ -1,6 +1,34 @@ package admin -import "sync/atomic" +import ( + "sync/atomic" + "time" + + "github.com/owncord/server/auth" +) + +// CaptureSetupLimiter installs h so the next NewAdminAPI call reports the +// *auth.RateLimiter it creates for the /setup endpoint. NewAdminAPI returns +// only an http.Handler, so this is the only way tests can reach that limiter +// to check whether its stale entries get reaped. +func CaptureSetupLimiter(h func(*auth.RateLimiter)) (restore func()) { + prev := setupLimiterHook + setupLimiterHook = h + return func() { setupLimiterHook = prev } +} + +// SetSetupLimiterReapTiming overrides the interval and max-window the setup +// endpoint's rate-limiter reaper uses, so tests don't wait on the real +// 5-minute interval. +func SetSetupLimiterReapTiming(interval, maxWindow time.Duration) (restore func()) { + prevI, prevW := setupLimiterReapInterval, setupLimiterReapMaxWindow + setupLimiterReapInterval = interval + setupLimiterReapMaxWindow = maxWindow + return func() { + setupLimiterReapInterval = prevI + setupLimiterReapMaxWindow = prevW + } +} // SetBackupBaseDir overrides backupBaseDir so tests can point backup handlers // at a temp dir. Lives here so it stays out of the production binary. diff --git a/Server/admin/handlers_tokens.go b/Server/admin/handlers_tokens.go index 8b57bf16..3b0c062c 100644 --- a/Server/admin/handlers_tokens.go +++ b/Server/admin/handlers_tokens.go @@ -63,6 +63,14 @@ func handleCreateAPIToken(database *db.DB) http.HandlerFunc { writeErr(w, http.StatusBadRequest, "BAD_REQUEST", "label is required") return } + // expires_hours=0 means "never expires" (see createTokenRequest doc). + // Negatives must not fall into that same nil-expiresAt branch, and the + // upper bound keeps time.Duration(hours)*time.Hour from overflowing + // int64 nanoseconds into a past timestamp. 87600h = 10 years. + if req.ExpiresHours < 0 || req.ExpiresHours > 24*365*10 { + writeErr(w, http.StatusBadRequest, "BAD_REQUEST", "expires_hours must be between 0 and 87600") + return + } var user *db.User var err error diff --git a/Server/admin/setup_limiter_reap_test.go b/Server/admin/setup_limiter_reap_test.go new file mode 100644 index 00000000..ebfe0b09 --- /dev/null +++ b/Server/admin/setup_limiter_reap_test.go @@ -0,0 +1,65 @@ +package admin_test + +import ( + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/owncord/server/admin" + "github.com/owncord/server/auth" +) + +// TestSetupLimiter_ReapsStaleEntries pins OC-0076: setupLimiter — the +// dedicated auth.RateLimiter behind POST /setup — is never reaped, so a +// distinct one-shot source IP (the common case once the server is already +// configured: every unauthenticated caller 403s but still records a rate +// limit entry before the CreateOwnerIfEmpty check rejects them) leaves a +// windows[] entry that lives forever. Unlike a repeat caller, whose entry +// self-prunes on its next Allow() call, a one-shot caller never revisits its +// key, so only a periodic sweep (RateLimiter.Cleanup) can ever evict it. +func TestSetupLimiter_ReapsStaleEntries(t *testing.T) { + restoreTiming := admin.SetSetupLimiterReapTiming(5*time.Millisecond, 5*time.Millisecond) + defer restoreTiming() + + var limiter *auth.RateLimiter + restoreHook := admin.CaptureSetupLimiter(func(rl *auth.RateLimiter) { limiter = rl }) + defer restoreHook() + + database := openAdminTestDB(t) + handler := admin.NewAdminAPI(database, "1.0.0", &mockHub{}, nil, nil, nil, nil, newTestModService(database), newTestRoleService(database)) + + if limiter == nil { + t.Fatal("setup limiter was not captured — CaptureSetupLimiter hook not wired into NewAdminAPI") + } + + // Simulate 20 distinct source IPs each making one POST /setup request — + // each leaves its own windows[] entry that nothing but a reap can evict. + const n = 20 + for i := range n { + req := httptest.NewRequest(http.MethodPost, "/setup", strings.NewReader(`{}`)) + req.RemoteAddr = fmt.Sprintf("203.0.113.%d:1234", i) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + } + + if wins, _ := limiter.Len(); wins != n { + t.Fatalf("Len().windows = %d immediately after %d one-shot requests, want %d", wins, n, n) + } + + // Wait well past the (shrunk) reap interval + max window for the sweep + // to evict every now-stale entry. + deadline := time.Now().Add(2 * time.Second) + for { + wins, _ := limiter.Len() + if wins == 0 { + return + } + if time.Now().After(deadline) { + t.Fatalf("Len().windows = %d after waiting past the reap interval, want 0 — setupLimiter is never reaped (OC-0076)", wins) + } + time.Sleep(5 * time.Millisecond) + } +} diff --git a/Server/api/auth_handler.go b/Server/api/auth_handler.go index 106e608e..ee3f1b2c 100644 --- a/Server/api/auth_handler.go +++ b/Server/api/auth_handler.go @@ -109,7 +109,7 @@ func MountAuthRoutes(r chi.Router, database *db.DB, limiter *auth.RateLimiter, t r.Route("/api/v1/auth", func(r chi.Router) { r.With(RateLimitMiddleware(registerLimiter, "register:", registerRateLimitPerMinute, time.Minute, trustedProxies)). - Post("/register", handleRegister(database)) + Post("/register", handleRegister(database, trustedProxies)) r.With(RateLimitMiddleware(loginLimiter, "login:", loginRateLimitPerMinute, time.Minute, trustedProxies)). Post("/login", handleLogin(database, limiter, partialStore, trustedProxies)) @@ -142,7 +142,8 @@ func MountAuthRoutes(r chi.Router, database *db.DB, limiter *auth.RateLimiter, t } // handleRegister processes POST /api/v1/auth/register. -func handleRegister(database *db.DB) http.HandlerFunc { +func handleRegister(database *db.DB, trustedProxies []string) http.HandlerFunc { + proxyNets := parseCIDRList(trustedProxies) // W3-3a: parse once at construction return func(w http.ResponseWriter, r *http.Request) { registrationOpen, err := isRegistrationOpen(r.Context(), database) if err != nil { @@ -252,7 +253,7 @@ func handleRegister(database *db.DB) http.HandlerFunc { return } - ip := clientIP(r) + ip := clientIPWithProxies(r, proxyNets) slog.Info("user registered", "username", req.Username, "user_id", uid, "ip", ip) db.WriteAudit(context.WithoutCancel(r.Context()), database, uid, "user_register", "user", uid, "new account created via invite") diff --git a/Server/api/auth_handler_test.go b/Server/api/auth_handler_test.go index 21c40bbb..ab65a25f 100644 --- a/Server/api/auth_handler_test.go +++ b/Server/api/auth_handler_test.go @@ -1746,3 +1746,46 @@ func TestRegister_ExpiredInvite(t *testing.T) { t.Errorf("Register expired invite status = %d, want 400", rr.Code) } } + +// TestRegister_UsesTrustedForwardedIP pins OC-0093: handleRegister must +// resolve the client IP through the same trusted-proxy list handleLogin +// uses, not unconditionally use RemoteAddr. Behind a trusted reverse proxy, +// the sessions.ip row registration creates must record the real client, not +// the proxy's own address — otherwise the same client shows two different +// IPs on the "active sessions" screen depending on whether they registered +// or logged in. +func TestRegister_UsesTrustedForwardedIP(t *testing.T) { + database := newAuthTestDB(t) + limiter := auth.NewRateLimiter() + router := buildAuthRouterWithProxies(database, limiter, []string{"127.0.0.0/8"}) + + ownerID, _ := database.CreateUser(context.Background(), "owner", "hash", 1) + code, _ := database.CreateInvite(context.Background(), ownerID, 1, nil) + + req := httptest.NewRequest(http.MethodPost, "/api/v1/auth/register", bytes.NewReader([]byte( + `{"username":"newuser","password":"securePass1","invite_code":"`+code+`"}`))) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("X-Forwarded-For", "203.0.113.9") + req.RemoteAddr = "127.0.0.1:9999" // the trusted reverse proxy's own hop + rr := httptest.NewRecorder() + router.ServeHTTP(rr, req) + + if rr.Code != http.StatusCreated { + t.Fatalf("Register status = %d, want 201; body = %s", rr.Code, rr.Body.String()) + } + + var resp map[string]any + _ = json.NewDecoder(rr.Body).Decode(&resp) + token, _ := resp["token"].(string) + if token == "" { + t.Fatal("Register response missing token") + } + + sess, err := database.GetSessionByTokenHash(context.Background(), auth.HashToken(token)) + if err != nil || sess == nil { + t.Fatalf("GetSessionByTokenHash: %v", err) + } + if sess.IP != "203.0.113.9" { + t.Errorf("session IP = %q, want the trusted-forwarded client IP %q — registration behind a reverse proxy must not record the proxy's own address", sess.IP, "203.0.113.9") + } +} diff --git a/Server/api/dm_group_handler_test.go b/Server/api/dm_group_handler_test.go index e708e5ce..357e98a7 100644 --- a/Server/api/dm_group_handler_test.go +++ b/Server/api/dm_group_handler_test.go @@ -184,6 +184,22 @@ func TestCreateGroupDM_BlockedCannotAddBlocker(t *testing.T) { } } +func TestCreateGroupDM_RejectsMutuallyBlockedRecipients(t *testing.T) { + database, router, _, tokens := groupFixture(t) + // carol blocks bob; neither blocked alice, so alice (an uninvolved third + // party) must not be able to force them into a shared group DM. + if err := database.BlockUser(context.Background(), 3, 2); err != nil { + t.Fatalf("BlockUser: %v", err) + } + + rr := dmPost(t, router, "/api/v1/dms/group", tokens[0], map[string]any{ + "recipient_ids": []int64{2, 3}, + }) + if rr.Code != http.StatusForbidden { + t.Fatalf("expected 403 when two recipients have blocked each other, got %d: %s", rr.Code, rr.Body.String()) + } +} + // ─── listing ──────────────────────────────────────────────────────────────── func TestListDMs_ReturnsGroupWithParticipants(t *testing.T) { diff --git a/Server/api/upload_handler.go b/Server/api/upload_handler.go index bddc41be..0526a709 100644 --- a/Server/api/upload_handler.go +++ b/Server/api/upload_handler.go @@ -3,7 +3,6 @@ package api import ( "database/sql" "errors" - "fmt" "image" _ "image/gif" _ "image/jpeg" @@ -103,6 +102,27 @@ func isUnsafeInlineMIME(mimeType string) bool { return false } +// safeStorageErrorMessage maps a storage.Save error to a client-safe +// "upload rejected" body. Full detail always goes to slog.Warn at the call +// site — this only decides what crosses the HTTP boundary. storage.Save's +// failure messages are built with fmt.Errorf("... %s", dst) / %w around +// path-bearing OS errors (creating the file, syncing it, or the destination +// resolving outside the storage dir), so echoing them verbatim hands any +// authenticated user the server's absolute storage layout the moment a save +// fails (disk full, permission change, read-only mount). The two validation +// failures below are the only ones that never embed a path, so they're the +// only ones whose detail is forwarded. +func safeStorageErrorMessage(err error) string { + msg := err.Error() + switch { + case strings.HasPrefix(msg, "blocked file type:"), + strings.HasPrefix(msg, "file exceeds maximum size"): + return "upload rejected: " + msg + default: + return "upload rejected" + } +} + // MountUploadRoutes registers upload and file-serving endpoints. // allowedOrigins controls the Access-Control-Allow-Origin header on served files. // @@ -190,7 +210,7 @@ func handleUpload(database *db.DB, store *storage.Storage, limiter *auth.RateLim slog.Warn("file upload rejected", "error", saveErr) writeJSON(w, http.StatusBadRequest, errorResponse{ Error: "BAD_REQUEST", - Message: fmt.Sprintf("upload rejected: %s", saveErr), + Message: safeStorageErrorMessage(saveErr), }) return } diff --git a/Server/api/upload_handler_test.go b/Server/api/upload_handler_test.go index 7ee182ee..502f5ba5 100644 --- a/Server/api/upload_handler_test.go +++ b/Server/api/upload_handler_test.go @@ -568,6 +568,49 @@ func TestUpload_OversizedFileRejected(t *testing.T) { } } +// OC-0137: storage.Save's error strings embed the resolved absolute +// destination path ("creating file %s", "syncing file %s", "resolved path %q +// escapes storage directory"). handleUpload must not forward that text to the +// client — only log it — or any authenticated user who triggers a storage +// failure (disk full, permission change, read-only mount) learns the +// server's absolute storage directory layout. +func TestUpload_StorageErrorDoesNotLeakPath(t *testing.T) { + database := newUploadTestDB(t) + dir := t.TempDir() + store, err := storage.New(dir, 10) + if err != nil { + t.Fatalf("storage.New: %v", err) + } + router := buildUploadRouter(database, store, nil) + token := uploadCreateToken(t, database, "leakuser", 1) + + // Remove the storage directory out from under the already-constructed + // Storage so Save's os.Create fails — this is what a disk-full, + // permission-change, or read-only-mount failure looks like from the + // handler's point of view: a storage-layer error surfaces at Save time. + if err := os.RemoveAll(dir); err != nil { + t.Fatalf("RemoveAll: %v", err) + } + + content := []byte("content that will fail to persist because the storage dir is gone") + rr := doUpload(t, router, token, "file", "leaktest.txt", content) + if rr.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400; body: %s", rr.Code, rr.Body.String()) + } + + var resp map[string]any + if err := json.NewDecoder(rr.Body).Decode(&resp); err != nil { + t.Fatalf("decode: %v", err) + } + message, _ := resp["message"].(string) + if strings.Contains(message, dir) { + t.Fatalf("response message leaks the absolute storage path: %q", message) + } + if strings.ContainsAny(message, `/\`) { + t.Fatalf("response message looks like it contains a filesystem path: %q", message) + } +} + func TestUpload_DBCreateAttachmentFailureDeletesStoredFile(t *testing.T) { database := newUploadTestDB(t) dir := t.TempDir() diff --git a/Server/auth/ratelimit.go b/Server/auth/ratelimit.go index 0962bd36..800a4fa0 100644 --- a/Server/auth/ratelimit.go +++ b/Server/auth/ratelimit.go @@ -2,6 +2,7 @@ package auth import ( "context" + "log/slog" "strconv" "time" @@ -83,6 +84,8 @@ func NewPersistentRateLimiter(store LockoutPersister) *RateLimiter { for i, key := range keys { rl.shardFor(key).lockouts[key] = &lockoutEntry{expiresAt: expiresAt[i]} } + } else { + slog.Warn("ratelimit: failed to load persisted lockouts; starting with none", "err", err) } return rl } diff --git a/Server/auth/ratelimit_persist_test.go b/Server/auth/ratelimit_persist_test.go new file mode 100644 index 00000000..e16bf651 --- /dev/null +++ b/Server/auth/ratelimit_persist_test.go @@ -0,0 +1,48 @@ +package auth_test + +import ( + "context" + "errors" + "log/slog" + "strings" + "testing" + "time" + + "github.com/owncord/server/auth" +) + +// failingLockoutStore reports an error from LoadActiveLockouts, simulating a +// transient DB failure (SQLITE_BUSY, disk I/O error) at startup. +type failingLockoutStore struct{} + +func (failingLockoutStore) UpsertLockout(context.Context, string, time.Time) error { return nil } +func (failingLockoutStore) DeleteLockout(context.Context, string) error { return nil } +func (failingLockoutStore) CleanupExpiredLockouts(context.Context) error { return nil } +func (failingLockoutStore) LoadActiveLockouts(context.Context) ([]string, []time.Time, error) { + return nil, nil, errors.New("database is locked") +} + +// captureLogs redirects the default slog logger to a buffer for the duration +// of fn and returns everything it wrote. Mirrors db/audit_test.go's helper. +func captureLogs(t *testing.T, fn func()) string { + t.Helper() + var buf strings.Builder + prev := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug}))) + defer slog.SetDefault(prev) + fn() + return buf.String() +} + +// TestNewPersistentRateLimiter_LoadErrorIsLogged pins OC-0061: a failed +// LoadActiveLockouts must not be swallowed silently — it has to produce a log +// line so an operator can notice that persisted lockouts were dropped. +func TestNewPersistentRateLimiter_LoadErrorIsLogged(t *testing.T) { + out := captureLogs(t, func() { + auth.NewPersistentRateLimiter(failingLockoutStore{}) + }) + + if !strings.Contains(out, "database is locked") { + t.Errorf("expected log output to mention the load error, got: %q", out) + } +} diff --git a/Server/db/event_queries.go b/Server/db/event_queries.go index 3ee2e785..80955a8a 100644 --- a/Server/db/event_queries.go +++ b/Server/db/event_queries.go @@ -150,6 +150,24 @@ func (d *DB) GetEventsSinceForChannels(ctx context.Context, afterSeq int64, chan return scanEventRows(rows) } +// CountEventsInRange returns the UNFILTERED (all channels) count of events +// with afterSeq < seq <= uptoSeq. seq is the events table's primary key, so +// this can only ever come up short of (uptoSeq - afterSeq), never over — +// callers use that to detect an interior gap left by a lost row (a dropped +// EventPersister enqueue, or a failed row in a batch flush) without having to +// enumerate every seq in the range. +func (d *DB) CountEventsInRange(ctx context.Context, afterSeq, uptoSeq int64) (int64, error) { + var count int64 + err := d.reader.QueryRowContext(ctx, + `SELECT COUNT(*) FROM events WHERE seq > ? AND seq <= ?`, + afterSeq, uptoSeq, + ).Scan(&count) + if err != nil { + return 0, fmt.Errorf("CountEventsInRange: %w", err) + } + return count, nil +} + // GetMaxEventSeq returns the largest seq in the events table, or 0 if empty. func (d *DB) GetMaxEventSeq(ctx context.Context) (int64, error) { var maxSeq sql.NullInt64 diff --git a/Server/db/mention_queries.go b/Server/db/mention_queries.go index 3bba62c8..dd9a867b 100644 --- a/Server/db/mention_queries.go +++ b/Server/db/mention_queries.go @@ -255,9 +255,30 @@ func (d *DB) GetMentionCount(ctx context.Context, userID, channelID int64) (int, return count, nil } -// GetUserIDsByUsernames resolves usernames to ids, keyed by the lowercased -// username. Matching is case-insensitive because users.username is UNIQUE -// COLLATE NOCASE, which makes the column's comparisons case-insensitive too. +// LowerASCII lowercases only ASCII letters ('A'-'Z'), matching the fold +// SQLite's COLLATE NOCASE applies to users.username (see notBannedClause's +// sibling comment above and the migration that declares the column). Go's +// strings.ToLower is Unicode-aware and would fold a non-ASCII uppercase +// letter (e.g. 'É' -> 'é') that NOCASE does not touch, desyncing a Go-side +// lookup key from a query bound against the same column. Every mention +// lookup that builds a key or a query argument from a username must fold +// through this instead of strings.ToLower, or the two folds silently +// disagree on any non-ASCII-uppercase username. +func LowerASCII(s string) string { + b := []byte(s) + for i, c := range b { + if c >= 'A' && c <= 'Z' { + b[i] = c + ('a' - 'A') + } + } + return string(b) +} + +// GetUserIDsByUsernames resolves usernames to ids, keyed by the ASCII-lowered +// username (see LowerASCII). Matching is case-insensitive because +// users.username is UNIQUE COLLATE NOCASE, which makes the column's +// comparisons case-insensitive too -- but ASCII-only, which is why the map +// key folds no harder than that. func (d *DB) GetUserIDsByUsernames(ctx context.Context, usernames []string) (map[string]int64, error) { result := make(map[string]int64) if len(usernames) == 0 { @@ -288,7 +309,7 @@ func (d *DB) GetUserIDsByUsernames(ctx context.Context, usernames []string) (map if scanErr := rows.Scan(&id, &name); scanErr != nil { return nil, fmt.Errorf("GetUserIDsByUsernames scan: %w", scanErr) } - result[strings.ToLower(name)] = id + result[LowerASCII(name)] = id } if rows.Err() != nil { return nil, fmt.Errorf("GetUserIDsByUsernames rows: %w", rows.Err()) diff --git a/Server/db/mention_queries_test.go b/Server/db/mention_queries_test.go index 78d1d5ea..77a9acc6 100644 --- a/Server/db/mention_queries_test.go +++ b/Server/db/mention_queries_test.go @@ -282,6 +282,33 @@ func TestGetUserIDsByUsernames_CaseInsensitive(t *testing.T) { } } +// TestGetUserIDsByUsernames_NonASCIIUppercase locks OC-0131: a username +// holding an uppercase non-ASCII letter (legal per auth.ValidateUsername, +// e.g. "Émile") must resolve through the exact same spelling it was queried +// with. users.username is only COLLATE NOCASE, which folds ASCII A-Z only, so +// the map key this function builds from the returned row must fold no harder +// than that column does -- a Unicode-aware strings.ToLower would fold 'É' to +// 'é' here and desync the key from the caller's (equally ASCII-folded) +// lookup spelling, making the row permanently unreachable by name. +func TestGetUserIDsByUsernames_NonASCIIUppercase(t *testing.T) { + database := newMigratedTestDB(t) + seedMentionFixture(t, database) + ctx := context.Background() + + uid, err := database.CreateUser(ctx, "Émile", "hash", 4) + if err != nil { + t.Fatalf("CreateUser: %v", err) + } + + got, err := database.GetUserIDsByUsernames(ctx, []string{"Émile"}) + if err != nil { + t.Fatalf("GetUserIDsByUsernames: %v", err) + } + if got["Émile"] != uid { + t.Errorf(`result["Émile"] = %d, want %d (map key must match the query spelling for a non-ASCII-uppercase username)`, got["Émile"], uid) + } +} + // TestGetUserIDsByUsernames_LapsedTempBan_StillResolves locks the "reconverged // raw column" fix: nothing clears users.banned when a temp ban's ban_expires // lapses (that's decided lazily, at login, by auth.IsEffectivelyBanned), so a diff --git a/Server/db/message_queries.go b/Server/db/message_queries.go index 661723c6..355e19fe 100644 --- a/Server/db/message_queries.go +++ b/Server/db/message_queries.go @@ -638,8 +638,18 @@ func (d *DB) GetLatestMessageID(ctx context.Context, channelID int64) (int64, er return id, nil } -// GetPinnedMessages returns all pinned messages in a channel in the API response shape, -// including user object, reactions (with me flag), and attachments. +// MaxPinnedMessages bounds how many pinned messages a single channel query +// returns. Without a cap, scanAndEnrichMessages feeds every pinned message ID +// into several `IN (?,?,...)` batch lookups (reactions, attachments, +// mentions); past SQLite's ~32766 bound-parameter limit that fails outright +// ("too many SQL variables"), and the pins endpoint then 500s on every call +// for that channel forever. The cap sits far below that ceiling, with room to +// spare across all three batch queries. +const MaxPinnedMessages = 1000 + +// GetPinnedMessages returns up to MaxPinnedMessages pinned messages in a +// channel, most-recently-pinned first, in the API response shape, including +// user object, reactions (with me flag), and attachments. func (d *DB) GetPinnedMessages(ctx context.Context, channelID int64, requestingUserID int64) ([]MessageAPIResponse, error) { rows, err := d.reader.QueryContext(ctx, `SELECT m.id, m.channel_id, m.user_id, u.username, u.avatar, @@ -647,8 +657,8 @@ func (d *DB) GetPinnedMessages(ctx context.Context, channelID int64, requestingU m.mentions_everyone FROM messages m JOIN users u ON m.user_id = u.id WHERE m.channel_id = ? AND m.pinned = 1 AND m.deleted = 0 - ORDER BY m.id DESC`, - channelID, + ORDER BY m.id DESC LIMIT ?`, + channelID, MaxPinnedMessages, ) if err != nil { return nil, fmt.Errorf("GetPinnedMessages: %w", err) diff --git a/Server/db/message_queries_test.go b/Server/db/message_queries_test.go index ea26486e..101f6b33 100644 --- a/Server/db/message_queries_test.go +++ b/Server/db/message_queries_test.go @@ -1301,6 +1301,39 @@ func TestGetChannelUnreadCounts_IncludesParticipatingDMs(t *testing.T) { } } +// ─── GetPinnedMessages ──────────────────────────────────────────────────────── + +// TestGetPinnedMessages_Capped pins more messages than db.MaxPinnedMessages and +// verifies the query stays bounded instead of returning every pinned row. An +// uncapped GetPinnedMessages feeds an unbounded message-ID slice into the +// shared IN-list batch fetches (reactions/attachments/mentions); past +// SQLite's ~32766 bound-parameter limit that fails every call permanently +// ("too many SQL variables"). This pins the cap well below that ceiling. +func TestGetPinnedMessages_Capped(t *testing.T) { + database := openMigratedMemory(t) + userID := seedUser(t, database, "pinner") + chID := seedChannel(t, database, "pins") + + total := db.MaxPinnedMessages + 5 + for i := range total { + id, err := database.CreateMessage(context.Background(), chID, userID, "msg", nil) + if err != nil { + t.Fatalf("CreateMessage[%d]: %v", i, err) + } + if err := database.SetMessagePinned(context.Background(), id, true); err != nil { + t.Fatalf("SetMessagePinned[%d]: %v", i, err) + } + } + + msgs, err := database.GetPinnedMessages(context.Background(), chID, userID) + if err != nil { + t.Fatalf("GetPinnedMessages: %v", err) + } + if len(msgs) > db.MaxPinnedMessages { + t.Errorf("GetPinnedMessages returned %d pins, want <= MaxPinnedMessages (%d)", len(msgs), db.MaxPinnedMessages) + } +} + func TestGetChannelUnreadCounts_ExcludesForeignDMs(t *testing.T) { database := openMigratedMemory(t) alice := seedUser(t, database, "dmforeignalice") diff --git a/Server/service/channel.go b/Server/service/channel.go index 1b3f3de2..90be13db 100644 --- a/Server/service/channel.go +++ b/Server/service/channel.go @@ -237,13 +237,24 @@ func (s *ChannelService) HandleChannelFocus(ctx context.Context, userID, channel return nil, fmt.Errorf("%w: channel not found", ErrNotFound) } - if ch.Type == "dm" { + switch { + case ch.Type == "dm": ok, err := s.st.IsDMParticipant(ctx, userID, channelID) if err != nil || !ok { return nil, fmt.Errorf("%w: access denied", ErrForbidden) } - } else if !s.perms.HasChannelPerm(ctx, userID, channelID, permissions.ReadMessages) { + case !s.perms.HasChannelPerm(ctx, userID, channelID, permissions.ReadMessages): return nil, fmt.Errorf("%w: access denied", ErrForbidden) + case ch.Archived: + // Archived channels are hidden from every other client surface + // (ListVisibleChannels, ready payload, reconnect replay, voice join — + // see permissions.Checker.VisibleChannelIDs and ws/voice_join.go). + // HasChannelPerm alone doesn't know about the archive flag, so without + // this a socket that still held the id could resubscribe to the live + // topic and advance its own read state on a channel reconnect replay + // then filters back out. channel_focus and mark_read share this one + // service call, so the guard closes both at once (OC-0070). + return nil, fmt.Errorf("%w: channel is archived", ErrForbidden) } // Mark channel as read. latestID == 0 (no undeleted messages) still diff --git a/Server/service/channel_test.go b/Server/service/channel_test.go index cf9c5711..2d006c69 100644 --- a/Server/service/channel_test.go +++ b/Server/service/channel_test.go @@ -41,6 +41,65 @@ func TestListVisibleChannels_OverrideFetchErrorFailsClosed(t *testing.T) { } } +// TestHandleChannelFocus_RefusedInArchivedChannel locks OC-0070: archived +// channels are hidden from every other client surface (ListVisibleChannels, +// the ws ready payload, RefreshChannelVisibility, voice join) but +// HandleChannelFocus never consulted ch.Archived, so a socket that still held +// the channel id could re-subscribe to its live event stream — and advance +// its own read state — on a channel reconnect replay (computeAllowedChannels) +// would then filter out. focus and mark_read share this one service call, so +// gating it here closes both. +func TestHandleChannelFocus_RefusedInArchivedChannel(t *testing.T) { + ctx := context.Background() + database := newTestDB(t) + seedRole(t, database, &db.Role{ + ID: permissions.MemberRoleID, + Name: "member", + Permissions: permissions.SendMessages | permissions.ReadMessages, + Position: 1, + }) + seedUserRole(t, database, 1, permissions.MemberRoleID) + seedChannel(t, database, &db.Channel{ID: 10, Name: "general", Type: "text"}) + + svc := NewChannelService(database, NewPermissionService(database, permissions.NewChecker(database))) + + // Precondition: focus succeeds while the channel is not archived. + if _, err := svc.HandleChannelFocus(ctx, 1, 10); err != nil { + t.Fatalf("precondition: focus on a live channel: %v", err) + } + + if _, err := database.ExecContext(ctx, + `UPDATE channels SET archived = 1 WHERE id = 10`); err != nil { + t.Fatalf("archive channel: %v", err) + } + + _, err := svc.HandleChannelFocus(ctx, 1, 10) + if err == nil { + t.Fatal("HandleChannelFocus on an archived channel succeeded — the socket can still subscribe to its live event stream") + } + if !errors.Is(err, ErrForbidden) { + t.Fatalf("HandleChannelFocus error = %v, want ErrForbidden", err) + } +} + +// TestHandleChannelFocus_DMExemptFromArchiveGate makes sure the archive gate +// above is scoped to non-DM channels only — DMs carry no archived concept. +func TestHandleChannelFocus_DMExemptFromArchiveGate(t *testing.T) { + ctx := context.Background() + database := newTestDB(t) + seedUser(t, database, &db.User{ID: 1, Username: "alice"}) + seedUser(t, database, &db.User{ID: 2, Username: "bob"}) + seedChannel(t, database, &db.Channel{ID: 50, Name: "dm-1-2", Type: "dm"}) + seedDMParticipant(t, database, 50, 1) + seedDMParticipant(t, database, 50, 2) + + svc := NewChannelService(database, NewPermissionService(database, permissions.NewChecker(database))) + + if _, err := svc.HandleChannelFocus(ctx, 1, 50); err != nil { + t.Fatalf("HandleChannelFocus on a DM: %v", err) + } +} + // TestHandleTyping_BlockedInDMEmitsNothing completes the DM-block sweep: a // blocked user could still drive a repeatable typing indicator at the blocker, // because HandleTyping authorized on DM participation alone. Typing is diff --git a/Server/service/dm.go b/Server/service/dm.go index 02ac88ad..bd8a94a5 100644 --- a/Server/service/dm.go +++ b/Server/service/dm.go @@ -247,16 +247,26 @@ func (s *DMService) CreateGroupDM(ctx context.Context, userID int64, recipientID if auth.IsEffectivelyBanned(user) { return nil, fmt.Errorf("%w: recipient not found", ErrNotFound) } - blocked, err := s.st.IsEitherBlocked(ctx, userID, rid) - if err != nil { - return nil, fmt.Errorf("%w: failed to check block status: %v", ErrInternal, err) - } - if blocked { - return nil, fmt.Errorf("%w: cannot add a blocked user to a group DM", ErrForbidden) - } } + // Block-check every pair in the room, not just creator-vs-recipient: + // group DMs are exempt from the send-time block gate (requireDMNotBlocked + // skips groups entirely) on the strength of this creation-time check, so + // two mutually-blocked recipients must not both end up in the same group + // even when neither of them blocked the creator. n <= MaxGroupDMParticipants, + // so the O(n^2) scan is trivial. participantIDs := append([]int64{userID}, unique...) + for i := range participantIDs { + for j := i + 1; j < len(participantIDs); j++ { + blocked, err := s.st.IsEitherBlocked(ctx, participantIDs[i], participantIDs[j]) + if err != nil { + return nil, fmt.Errorf("%w: failed to check block status: %v", ErrInternal, err) + } + if blocked { + return nil, fmt.Errorf("%w: cannot add a blocked user to a group DM", ErrForbidden) + } + } + } ch, err := s.st.CreateGroupDMChannel(ctx, cleanName, participantIDs) if err != nil { slog.Error("DMService.CreateGroupDM", "err", err) diff --git a/Server/service/mentions.go b/Server/service/mentions.go index b72fa2d4..65e72f78 100644 --- a/Server/service/mentions.go +++ b/Server/service/mentions.go @@ -66,7 +66,12 @@ func parseMentionTokens(content string) (tokens []mentionCandidate, everyone, he if m[3] == "@" { continue // address-shaped, e.g. "@bob@example.com" } - raw := strings.ToLower(m[2]) + // db.LowerASCII, not strings.ToLower: usernames.username is only + // COLLATE NOCASE, which folds ASCII A-Z only. A Unicode fold here + // (e.g. 'É' -> 'é') would desync this token from GetUserIDsByUsernames' + // equally ASCII-folded map key, so a username holding an uppercase + // non-ASCII letter could never resolve (OC-0131). + raw := db.LowerASCII(m[2]) switch raw { case everyoneToken: everyone = true diff --git a/Server/service/mentions_test.go b/Server/service/mentions_test.go index ef8da559..1a98d7d7 100644 --- a/Server/service/mentions_test.go +++ b/Server/service/mentions_test.go @@ -168,6 +168,25 @@ func TestSendMessage_CaseInsensitiveUsername(t *testing.T) { } } +// TestSendMessage_NonASCIIUppercaseUsernameResolves locks OC-0131: a username +// holding an uppercase non-ASCII letter is legal (auth.ValidateUsername only +// rejects control/format runes) and must still be @mentionable. Go's +// Unicode-aware strings.ToLower would fold "Émile" to "émile" before the +// lookup ever reaches SQL, but users.username is only COLLATE NOCASE, which +// folds ASCII A-Z only -- so a Unicode-lowered token can never match the +// stored non-ASCII-uppercase row, and the mention silently degrades to plain +// text. +func TestSendMessage_NonASCIIUppercaseUsernameResolves(t *testing.T) { + svc, _, database := newMentionFixture(t) + seedUser(t, database, &db.User{ID: 5, Username: "Émile", Status: "online"}) + seedUserRole(t, database, 5, permissions.MemberRoleID) + + res := sendAs(t, svc, 1, "hey @Émile") + if len(res.Mentions) != 1 || res.Mentions[0] != 5 { + t.Fatalf("mentions = %v, want [5] (Émile must resolve)", res.Mentions) + } +} + func TestSendMessage_UnknownWordStaysText(t *testing.T) { svc, _, database := newMentionFixture(t) diff --git a/Server/service/message_perms.go b/Server/service/message_perms.go index aa33c39e..2f98747f 100644 --- a/Server/service/message_perms.go +++ b/Server/service/message_perms.go @@ -41,10 +41,17 @@ func (s *MessageService) GetAccessibleChannelIDs(ctx context.Context, userID int // Also include DM channels the user participates in. Only the IDs are // needed here, so skip the full DM query's preview/unread work. + // + // A failed lookup must not silently shrink the accessible set to guild + // channels only — SearchMessages (message_query.go) treats this list as + // authoritative and would otherwise report a successful, DM-stripped + // result instead of failing. Same posture as the ws sibling, + // computeAllowedChannels in ws/serve.go. dmIDs, err := s.st.GetUserDMChannelIDs(ctx, userID) - if err == nil { - ids = append(ids, dmIDs...) + if err != nil { + return nil, fmt.Errorf("%w: failed to fetch DM channels: %v", ErrInternal, err) } + ids = append(ids, dmIDs...) return ids, nil } diff --git a/Server/service/message_perms_test.go b/Server/service/message_perms_test.go new file mode 100644 index 00000000..fa378b23 --- /dev/null +++ b/Server/service/message_perms_test.go @@ -0,0 +1,53 @@ +package service + +import ( + "context" + "errors" + "testing" + + "github.com/owncord/server/db" + "github.com/owncord/server/permissions" +) + +// errDMChannelIDsStore wraps a real *db.DB but always fails +// GetUserDMChannelIDs, so GetAccessibleChannelIDs' fail-closed contract +// (OC-0087) is testable. Embedding *db.DB satisfies the service Store +// interface; only the overridden method diverges. +type errDMChannelIDsStore struct { + *db.DB +} + +func (errDMChannelIDsStore) GetUserDMChannelIDs(context.Context, int64) ([]int64, error) { + return nil, errors.New("boom") +} + +// TestGetAccessibleChannelIDs_DMLookupErrorFailsClosed locks OC-0087: a +// transient GetUserDMChannelIDs failure must not silently degrade the +// accessible-channel set to guild channels only. Before the fix the error was +// discarded (`if err == nil { ids = append(...) }`), so GetAccessibleChannelIDs +// returned (nil error, truncated set) and SearchMessages read back a +// successful-but-DM-stripped result — exactly the hole the ws sibling +// (computeAllowedChannels in ws/serve.go) was deliberately hardened against. +func TestGetAccessibleChannelIDs_DMLookupErrorFailsClosed(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) + seedChannel(t, database, &db.Channel{ID: 10, Name: "general", Type: "text"}) + + checker := permissions.NewChecker(database) + svc := NewMessageService(errDMChannelIDsStore{database}, NewPermissionService(database, checker), nil) + + ids, err := svc.GetAccessibleChannelIDs(context.Background(), 1) + if err == nil { + t.Fatalf("expected an error when the DM lookup fails, got ids=%v, nil error", ids) + } + if !errors.Is(err, ErrInternal) { + t.Fatalf("error = %v, want ErrInternal", err) + } +} diff --git a/Server/service/message_reaction_users_test.go b/Server/service/message_reaction_users_test.go index 9a2a514b..26a4687b 100644 --- a/Server/service/message_reaction_users_test.go +++ b/Server/service/message_reaction_users_test.go @@ -155,6 +155,26 @@ func TestGetReactionUsers_ForeignDMIsNotFound(t *testing.T) { } } +// A soft-deleted message must not leak its reactor list. Its siblings in the +// same file/package already refuse a deleted message: handleReaction (this +// file) and GetMessagesAround (message_query.go) both check msg.Deleted, but +// GetReactionUsers had no such guard, so a tombstoned message's reactions +// stayed forever fetchable by direct URL even though the client no longer +// renders the message at all. +func TestGetReactionUsers_DeletedMessageIsNotFound(t *testing.T) { + svc, database := newTestMessageService(t) + msgID := seedReactedMessage(t, svc, database, "👍", 1) + + if err := database.DeleteMessage(context.Background(), msgID, 1, false); err != nil { + t.Fatalf("DeleteMessage: %v", err) + } + + _, err := svc.GetReactionUsers(context.Background(), 1, 10, msgID, "👍") + if !errors.Is(err, ErrNotFound) { + t.Fatalf("err = %v, want ErrNotFound", err) + } +} + // A custom emoji is reacted with as its ":shortcode:" literal, so the longest // shortcode the emoji service will accept has to fit inside the reaction length // cap. Before the cap was derived from MaxShortcodeLen, a 31- or 32-character diff --git a/Server/service/message_reactions.go b/Server/service/message_reactions.go index 69ba94ac..857d680a 100644 --- a/Server/service/message_reactions.go +++ b/Server/service/message_reactions.go @@ -39,7 +39,7 @@ func (s *MessageService) GetReactionUsers(ctx context.Context, userID, channelID } msg, err := s.st.GetMessage(ctx, msgID) - if err != nil || msg == nil || msg.ChannelID != channelID { + if err != nil || msg == nil || msg.ChannelID != channelID || msg.Deleted { return nil, fmt.Errorf("%w: message not found", ErrNotFound) } @@ -102,8 +102,16 @@ func (s *MessageService) handleReaction(ctx context.Context, userID, msgID int64 return nil, fmt.Errorf("%w: cannot react to deleted message", ErrBadRequest) } + // Fail closed, mirroring EditMessage/DeleteMessage (message_crud.go): a + // lookup failure must not fall through to the non-DM permission branch + // below. That branch passes on the base role mask alone + // (READ_MESSAGES|ADD_REACTIONS, no per-channel override exists for a DM), + // skipping both IsDMParticipant and requireDMNotBlocked entirely. ch, chErr := s.st.GetChannel(ctx, msg.ChannelID) - isDM := chErr == nil && ch != nil && ch.Type == "dm" + if chErr != nil || ch == nil { + return nil, fmt.Errorf("%w: cannot react to this message", ErrForbidden) + } + isDM := ch.Type == "dm" // Archived channels are read-only. handleReaction bypasses // checkSendPermission (it runs its own DM/permission branch below), so it diff --git a/Server/service/message_reactions_test.go b/Server/service/message_reactions_test.go index 7fcef7fe..e1293b8c 100644 --- a/Server/service/message_reactions_test.go +++ b/Server/service/message_reactions_test.go @@ -21,6 +21,76 @@ func (errDMParticipantsStore) GetDMParticipantIDs(context.Context, int64) ([]int return nil, errors.New("boom") } +// errGetChannelStore wraps a real *db.DB but fails GetChannel for one +// specific channel id, leaving every other call (including GetMessage) to +// hit the real database. Used to simulate a transient GetChannel error +// mid-request without disturbing the rest of the fixture. +type errGetChannelStore struct { + *db.DB + failChannelID int64 +} + +func (s errGetChannelStore) GetChannel(ctx context.Context, id int64) (*db.Channel, error) { + if id == s.failChannelID { + return nil, errors.New("boom") + } + return s.DB.GetChannel(ctx, id) +} + +// TestHandleReaction_ChannelLookupErrorFailsClosed locks OC-0075: a +// GetChannel error during handleReaction must not be treated as "not a DM". +// Before the fix, isDM := chErr == nil && ch != nil && ch.Type == "dm" quietly +// became false on any lookup error, routing a DM message into the role-based +// permission branch. That branch checks HasChannelPerm against the base role +// mask (no channel-override rows exist for a DM), so any user with the +// ordinary READ_MESSAGES|ADD_REACTIONS member permissions could react inside +// a private DM they are not a participant of, and the reaction would be +// fanned out as a channel event instead of a DM event. +func TestHandleReaction_ChannelLookupErrorFailsClosed(t *testing.T) { + database := newTestDB(t) + seedRole(t, database, &db.Role{ + ID: permissions.MemberRoleID, + Name: "member", + Permissions: permissions.SendMessages | permissions.ReadMessages | permissions.AddReactions, + Position: 1, + }) + seedUser(t, database, &db.User{ID: 1, Username: "alice"}) + seedUser(t, database, &db.User{ID: 2, Username: "bob"}) + seedUser(t, database, &db.User{ID: 3, Username: "mallory"}) + seedUserRole(t, database, 1, permissions.MemberRoleID) + seedUserRole(t, database, 2, permissions.MemberRoleID) + seedUserRole(t, database, 3, permissions.MemberRoleID) + + permSvc := NewPermissionService(database, permissions.NewChecker(database)) + + ch, _, err := database.GetOrCreateDMChannel(context.Background(), 1, 2) + if err != nil { + t.Fatalf("GetOrCreateDMChannel: %v", err) + } + msgID, err := database.CreateMessage(context.Background(), ch.ID, 1, "just us", nil) + if err != nil { + t.Fatalf("CreateMessage: %v", err) + } + + failingStore := errGetChannelStore{DB: database, failChannelID: ch.ID} + svc := NewMessageService(failingStore, permSvc, nil) + + // Mallory is not a DM participant, but carries the ordinary member role's + // READ_MESSAGES|ADD_REACTIONS. With GetChannel failing, a fail-open + // implementation lets this through as a non-DM reaction. + if _, err := svc.AddReaction(context.Background(), 3, msgID, "👍"); err == nil { + t.Fatal("AddReaction must fail when GetChannel errors mid-request, not fall open into the non-DM branch") + } + + counts, err := database.GetReactions(context.Background(), msgID) + if err != nil { + t.Fatalf("GetReactions: %v", err) + } + if len(counts) != 0 { + t.Fatalf("reaction must not be committed for a non-participant when the DM channel lookup failed, got %d reaction rows", len(counts)) + } +} + // TestHandleReaction_DMParticipantFetchErrorFailsClosed locks OC-0069: a DM // reaction must not be persisted with no way to notify anyone. Before the // fix, handleReaction committed AddReaction/RemoveReaction first and only diff --git a/Server/updater/release_singleflight_test.go b/Server/updater/release_singleflight_test.go new file mode 100644 index 00000000..82b0bbf5 --- /dev/null +++ b/Server/updater/release_singleflight_test.go @@ -0,0 +1,58 @@ +package updater + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "sync" + "sync/atomic" + "testing" +) + +// A burst of concurrent CheckForUpdate calls against an expired/cold release +// cache must collapse into a single outbound GitHub fetch. Without +// singleflight, every caller that observes the cache as expired issues its +// own outbound request (OC-0146). +func TestCheckForUpdateCoalescesConcurrentMisses(t *testing.T) { + var hits atomic.Int64 + release := newTestRelease("v2.0.0", "Major update", "https://github.com/J3vb/OwnCord/releases/tag/v2.0.0", + "https://github.com/J3vb/OwnCord/releases/download/v2.0.0") + + mux := http.NewServeMux() + mux.HandleFunc("/repos/J3vb/OwnCord/releases/latest", func(w http.ResponseWriter, r *http.Request) { + hits.Add(1) + w.Header().Set("Content-Type", "application/json") + if err := json.NewEncoder(w).Encode(release); err != nil { + t.Fatalf("encoding release: %v", err) + } + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + u := newTestUpdater(srv.URL, "1.0.0") + + const callers = 25 + var wg sync.WaitGroup + errs := make([]error, callers) + start := make(chan struct{}) + + for i := range callers { + wg.Go(func() { + <-start // release all goroutines together to force a real burst + _, errs[i] = u.CheckForUpdate(context.Background()) + }) + } + close(start) + wg.Wait() + + for i := range callers { + if errs[i] != nil { + t.Fatalf("caller %d: unexpected error: %v", i, errs[i]) + } + } + + if got := hits.Load(); got != 1 { + t.Fatalf("outbound fetches = %d, want exactly 1 (singleflight should coalesce)", got) + } +} diff --git a/Server/updater/updater.go b/Server/updater/updater.go index 0d8a4f92..5c5adad1 100644 --- a/Server/updater/updater.go +++ b/Server/updater/updater.go @@ -90,6 +90,7 @@ type Updater struct { errCacheExpiry time.Time textAssetCache map[string]textAssetCacheEntry textAssetSF singleflight.Group + releaseSF singleflight.Group mu syncutil.Mutex httpClient *http.Client signingKeyText string @@ -153,39 +154,66 @@ func detachFetch(ctx context.Context) (context.Context, context.CancelFunc) { // fetch is detached from ctx (see detachFetch), so cancelling ctx does not // abort it or write a failure into the shared cache. func (u *Updater) CheckForUpdate(ctx context.Context) (UpdateInfo, error) { - now := time.Now() - u.mu.Lock() - if u.cache != nil && now.Before(u.cacheExpiry) { - cached := *u.cache + if info, err, ok := u.lookupRelease(time.Now()); ok { + return info, err + } + + // Coalesce concurrent misses: when the cache TTL expires under load, every + // caller would otherwise issue its own outbound GitHub fetch (OC-0146). + // One flight runs and the rest wait on its result, exactly like + // FetchTextAssetCached's textAssetSF. + // + // The flight is detached from the leader's ctx (see detachFetch): callers + // include the unauthenticated client-update endpoint, so a leader that + // aborts its request must not fail its followers or write its own + // context.Canceled into the shared cache. + v, err, _ := u.releaseSF.Do("latest-release", func() (any, error) { + now := time.Now() + // Re-check: another flight may have filled the cache while we queued. + if info, err, ok := u.lookupRelease(now); ok { + return info, err + } + + fetchCtx, cancel := detachFetch(ctx) + defer cancel() + + info, err := u.fetchLatestRelease(fetchCtx) + if err != nil { + u.mu.Lock() + u.cachedErr = err + u.errCacheExpiry = now.Add(errorCacheTTL) + u.mu.Unlock() + return UpdateInfo{}, err + } + + u.mu.Lock() + u.cache = &info + u.cacheExpiry = now.Add(cacheTTL) + u.cachedErr = nil u.mu.Unlock() - return cached, nil + + return info, nil + }) + if err != nil { + return UpdateInfo{}, err + } + return v.(UpdateInfo), nil +} + +// lookupRelease returns a live cached release or cached error, if either +// exists. The third return value reports whether the cache was live (a +// caller should return the first two values directly); a false miss means +// the caller must fetch. +func (u *Updater) lookupRelease(now time.Time) (UpdateInfo, error, bool) { + u.mu.Lock() + defer u.mu.Unlock() + if u.cache != nil && now.Before(u.cacheExpiry) { + return *u.cache, nil, true } if u.cachedErr != nil && now.Before(u.errCacheExpiry) { - err := u.cachedErr - u.mu.Unlock() - return UpdateInfo{}, err + return UpdateInfo{}, u.cachedErr, true } - u.mu.Unlock() - - fetchCtx, cancel := detachFetch(ctx) - defer cancel() - - info, err := u.fetchLatestRelease(fetchCtx) - if err != nil { - u.mu.Lock() - u.cachedErr = err - u.errCacheExpiry = now.Add(errorCacheTTL) - u.mu.Unlock() - return UpdateInfo{}, err - } - - u.mu.Lock() - u.cache = &info - u.cacheExpiry = now.Add(cacheTTL) - u.cachedErr = nil - u.mu.Unlock() - - return info, nil + return UpdateInfo{}, nil, false } // fetchLatestRelease queries the GitHub API for the latest release and diff --git a/Server/ws/deps.go b/Server/ws/deps.go index 97da312a..6a050d54 100644 --- a/Server/ws/deps.go +++ b/Server/ws/deps.go @@ -86,6 +86,7 @@ type KeyHolderChecker interface { type PluginDeps struct { Registry func() *plugin.Registry MessageSvc *service.MessageService + Limiter *auth.RateLimiter } // VoiceDeps holds dependencies for voice handlers. diff --git a/Server/ws/event_pruner_test.go b/Server/ws/event_pruner_test.go index df70e2f8..87b506ba 100644 --- a/Server/ws/event_pruner_test.go +++ b/Server/ws/event_pruner_test.go @@ -73,6 +73,10 @@ func (*fakeEventStore) GetEventsSinceForChannels(context.Context, int64, []int64 panic("unused") } +func (*fakeEventStore) CountEventsInRange(context.Context, int64, int64) (int64, error) { + panic("unused") +} + func (*fakeEventStore) GetMaxEventSeq(context.Context) (int64, error) { panic("unused") } diff --git a/Server/ws/eventstore.go b/Server/ws/eventstore.go index 156b67b0..b5a4b8d7 100644 --- a/Server/ws/eventstore.go +++ b/Server/ws/eventstore.go @@ -19,6 +19,10 @@ type EventStore interface { PersistEvents(ctx context.Context, events []db.PersistedEvent) (int, error) GetEventsSince(ctx context.Context, afterSeq int64, limit int) ([]db.PersistedEvent, error) GetEventsSinceForChannels(ctx context.Context, afterSeq int64, channelIDs []int64, limit int) ([]db.PersistedEvent, error) + // CountEventsInRange returns the unfiltered count of events with + // afterSeq < seq <= uptoSeq, used to detect an interior gap left by a + // lost row before a persisted range is trusted as a complete replay. + CountEventsInRange(ctx context.Context, afterSeq, uptoSeq int64) (int64, error) PruneEventsOlderThan(ctx context.Context, cutoff time.Time) (int64, error) GetMaxEventSeq(ctx context.Context) (int64, error) } diff --git a/Server/ws/handlers_command.go b/Server/ws/handlers_command.go index b8754c15..ff88587a 100644 --- a/Server/ws/handlers_command.go +++ b/Server/ws/handlers_command.go @@ -14,7 +14,9 @@ import ( "errors" "fmt" "log/slog" + "time" + "github.com/owncord/server/auth" "github.com/owncord/server/plugin" "github.com/owncord/server/service" ) @@ -24,6 +26,15 @@ import ( // the plugin's allocate/dispatch ABI with thousands of strings. const maxCommandArgs = 64 +// pluginCommandRateLimit and pluginCommandWindow cap chat_command frames per +// user (OC-0091). Every other V2 handler is throttled; this one drove a WASM +// guest invocation once per frame with no cap at all. Tighter than chat send +// (10/s) because DispatchCommand does real work per call. +const ( + pluginCommandRateLimit = 5 + pluginCommandWindow = time.Second +) + // handleChatCommandV2 dispatches a slash command to the owning plugin via the // live plugin registry (wired post-construction). It returns: // - a ClientError when no plugin registry is wired, the command is unknown, @@ -35,6 +46,10 @@ func handleChatCommandV2(ctx context.Context, cmd Command, _ ClientInfo, deps an d := deps.(PluginDeps) cc := cmd.(ChatCommandCmd) + if d.Limiter != nil && !d.Limiter.Allow(auth.Key("plugin_cmd", cc.userID), pluginCommandRateLimit, pluginCommandWindow) { + return Result{Error: ClientError{Code: ErrCodeRateLimited, Message: "too many commands"}} + } + var reg *plugin.Registry if d.Registry != nil { reg = d.Registry() diff --git a/Server/ws/handlers_command_test.go b/Server/ws/handlers_command_test.go index 29734e44..04ccd7bc 100644 --- a/Server/ws/handlers_command_test.go +++ b/Server/ws/handlers_command_test.go @@ -111,6 +111,56 @@ func TestChatCommand_MalformedPayload_ReturnsBadRequest(t *testing.T) { } } +// TestChatCommand_RateLimited_ReturnsError verifies that chat_command is +// throttled per-user, same as every other V2 handler (OC-0091): a burst of +// commands beyond the limit must be rejected with RATE_LIMITED instead of +// running DispatchCommand (and therefore the plugin's WASM invocation) once +// per frame with no cap. +func TestChatCommand_RateLimited_ReturnsError(t *testing.T) { + hub, database := newTestHub(t) + send := make(chan []byte, 32) + c := ws.NewTestClient(hub, 1, send) + hub.Register(c) + defer hub.Unregister(c) + + reg, err := plugin.NewRegistry(plugin.Config{Store: database}) + if err != nil { + t.Fatalf("NewRegistry: %v", err) + } + hub.SetPluginRegistry(reg) + + sawRateLimited := false + for i := range 20 { + raw, _ := json.Marshal(map[string]any{ + "type": "chat_command", + "payload": map[string]any{ + "channel_id": int64(1), + "command": "/notexist", + "args": []string{}, + }, + }) + hub.HandleMessageForTest(c, raw) + + select { + case msg := <-send: + var env map[string]any + if err := json.Unmarshal(msg, &env); err != nil { + t.Fatalf("unmarshal: %v", err) + } + payload, _ := env["payload"].(map[string]any) + if payload != nil && payload["code"] == "RATE_LIMITED" { + sawRateLimited = true + } + default: + t.Fatalf("expected a response for message %d", i) + } + } + + if !sawRateLimited { + t.Fatal("expected at least one RATE_LIMITED response within 20 rapid chat_command frames") + } +} + // ─── EventSink.Emit ─────────────────────────────────────────────────────────── // TestEventSink_Emit_DeliversToBroadcaster verifies that Emit calls the wired diff --git a/Server/ws/hub.go b/Server/ws/hub.go index 41e2fea3..c35dbb37 100644 --- a/Server/ws/hub.go +++ b/Server/ws/hub.go @@ -155,6 +155,7 @@ func NewHub(database *db.DB, limiter *auth.RateLimiter, svc *service.Services) * reg.RegisterV2(MsgTypeChatCommand, handleChatCommandV2, PluginDeps{ Registry: func() *plugin.Registry { return h.pluginRegistry }, MessageSvc: h.messageSvc, + Limiter: h.limiter, }) registerVoiceControlsV2(reg, VoiceDeps{ DB: h.db, diff --git a/Server/ws/hub_broadcast.go b/Server/ws/hub_broadcast.go index f886802c..1a679eac 100644 --- a/Server/ws/hub_broadcast.go +++ b/Server/ws/hub_broadcast.go @@ -175,6 +175,15 @@ func (h *Hub) channelReadAudience(ctx context.Context, channelID int64) []int64 "channel_id", channelID, "err", err) return []int64{} } + // Archived channels are hidden from every client regardless of + // permissions, mirroring RefreshChannelVisibility and VisibleChannelIDs. + // Without this, an admin edit to an archived channel (or a voice + // teardown inside one) fans out straight to every connected user whose + // base role holds READ_MESSAGES, none of whom have the channel in their + // ready payload or sidebar. + if ch != nil && ch.Archived { + return []int64{} + } if ch != nil && ch.Type == "dm" { participantIDs, err := h.db.GetDMParticipantIDs(ctx, channelID) if err != nil { diff --git a/Server/ws/hub_broadcast_test.go b/Server/ws/hub_broadcast_test.go index 88cc547a..d0ccaaef 100644 --- a/Server/ws/hub_broadcast_test.go +++ b/Server/ws/hub_broadcast_test.go @@ -1,10 +1,12 @@ package ws_test import ( + "context" "encoding/json" "testing" "time" + "github.com/owncord/server/db" "github.com/owncord/server/ws" ) @@ -138,6 +140,52 @@ func TestHub_BroadcastDropCount(t *testing.T) { } } +// TestHub_ChannelReadAudience_ExcludesArchivedChannel is OC-0073: +// channelReadAudience (shared by BroadcastChannelCreate/Update and the voice +// event / CleanupVoiceForChannel fan-outs) never checked ch.Archived, unlike +// its sibling RefreshChannelVisibility which treats an archived channel as +// invisible to every role. A Member has base READ_MESSAGES with no override, +// so before archiving they are a legitimate audience member for this channel; +// archiving it must remove them from channelReadAudience even though their +// role's READ_MESSAGES grant never changed. +func TestHub_ChannelReadAudience_ExcludesArchivedChannel(t *testing.T) { + hub, database := newTestHub(t) + go hub.Run() + t.Cleanup(hub.Stop) + + member := seedMemberUser(t, database, "archived-audience-member") + send := make(chan []byte, 8) + hub.RegisterNowForTest(ws.NewTestClient(hub, member.ID, send)) + + chID := seedTestChannel(t, database, "will-be-archived") + + ch, err := database.GetChannel(context.Background(), chID) + if err != nil || ch == nil { + t.Fatalf("GetChannel: %v", err) + } + if err := database.AdminUpdateChannel(context.Background(), chID, db.ChannelUpdate{ + Name: ch.Name, + Topic: ch.Topic, + Category: ch.Category, + SlowMode: ch.SlowMode, + Position: ch.Position, + Archived: true, + }); err != nil { + t.Fatalf("AdminUpdateChannel: %v", err) + } + archived, err := database.GetChannel(context.Background(), chID) + if err != nil || archived == nil { + t.Fatalf("GetChannel after archive: %v", err) + } + if !archived.Archived { + t.Fatalf("channel not archived after AdminUpdateChannel") + } + + hub.BroadcastChannelUpdate(archived) + + assertNotReceived(t, send, "member with base READ_MESSAGES on an archived channel") +} + func TestHub_SetEventPersister(t *testing.T) { hub, database := newTestHub(t) diff --git a/Server/ws/hub_sweep.go b/Server/ws/hub_sweep.go index fc4b3b72..016ca489 100644 --- a/Server/ws/hub_sweep.go +++ b/Server/ws/hub_sweep.go @@ -285,6 +285,14 @@ func (h *Hub) hasChannelPermChecked(ctx context.Context, userID, channelID int64 return permissions.EffectiveChannelPerms(role.Permissions, o)&perm == perm, nil } +// cleanupVoiceRaceClearHook, when non-nil, runs immediately before +// CleanupVoiceForChannel clears a still-matching client's voice state. +// Test-only (always nil in production): the window it pins is two separate +// voiceMu acquisitions with no I/O between them, too narrow to land reliably +// by staggering real goroutines, so tests use this hook to reproduce a +// voice_join racing in at exactly that point deterministically. +var cleanupVoiceRaceClearHook func(*Client) + // CleanupVoiceForChannel removes all voice participants from the given channel. // Called when a channel is deleted. func (h *Hub) CleanupVoiceForChannel(channelID int64) { @@ -309,12 +317,25 @@ func (h *Hub) CleanupVoiceForChannel(channelID int64) { slog.Error("CleanupVoiceForChannel LeaveVoiceChannelIfMatch", "err", err, "user_id", vs.UserID, "channel_id", channelID) } - // Clear client voice state and its voice-topic subscription. + // Clear client voice state and its voice-topic subscription. The + // compare (still in this channel?) and the clear must be one atomic + // operation — a getVoiceChID() read followed by a separate + // unconditional clear leaves a window where a concurrent voice_join + // to another channel commits in between and gets silently wiped + // along with its own voice-topic subscription (OC-0050). Mirrors + // sweepStaleVoiceStates' handleVoiceLeaveIfStillIn / + // clearVoiceStateIfMatch and the LiveKit webhook's inline + // compare-and-clear. h.mu.RLock() client, ok := h.clients[vs.UserID] h.mu.RUnlock() - if ok && client.getVoiceChID() == channelID { - h.clearVoiceAndUnsubscribe(client) + if ok { + if cleanupVoiceRaceClearHook != nil { + cleanupVoiceRaceClearHook(client) + } + if _, cleared := client.clearVoiceStateIfMatch(channelID); cleared { + h.pubsub.Unsubscribe(client, VoiceTopic(channelID)) + } } // Remove from LiveKit (best-effort). diff --git a/Server/ws/hub_sweep_test.go b/Server/ws/hub_sweep_test.go index 7f9bde2a..507299ee 100644 --- a/Server/ws/hub_sweep_test.go +++ b/Server/ws/hub_sweep_test.go @@ -198,3 +198,55 @@ func TestSweepStaleVoiceStates_GhostRemovalReelectsKeyHolder(t *testing.T) { t.Error("ghost voice_states row was not removed by the sweep") } } + +// TestCleanupVoiceForChannel_ConcurrentJoinNotClobbered pins OC-0050: +// CleanupVoiceForChannel's client-state clear must be conditional on the +// participant still being in the channel being cleaned up at the moment it +// clears, not just at the moment it read (hub_sweep.go's own comment already +// promises this: "the client-state clear [is] conditional on the participant +// still being in THIS channel"). A voice_join to a different channel landing +// between the read and the clear must survive, exactly as +// TestSweepStaleVoiceStates_EvictionIsScopedToCheckedChannel already proves +// for the sibling sweep. +// +// The vulnerable window (read, then a separate unconditional clear) is two +// back-to-back voiceMu acquisitions with no I/O between them, too narrow to +// land reliably by staggering real goroutines. cleanupVoiceRaceClearHook +// (test-only, nil in production) fires at exactly that point so the test +// reproduces the interleaving deterministically instead of by luck. +func TestCleanupVoiceForChannel_ConcurrentJoinNotClobbered(t *testing.T) { + ctx := context.Background() + database := newHarvestVoiceDB(t) + uid := seedHarvestVoiceUser(t, database, "cleanup-race") + chA := mustCreateVoiceChannel(t, database, "voice-cleanup-a") + chB := mustCreateVoiceChannel(t, database, "voice-cleanup-b") + + if err := database.JoinVoiceChannel(ctx, uid, chA); err != nil { + t.Fatalf("JoinVoiceChannel: %v", err) + } + + h := NewHub(database, auth.NewRateLimiter(), nil) + c := NewTestClient(h, uid, make(chan []byte, 8)) + h.clients[uid] = c + c.setVoiceState(chA, "tok-a") + h.pubsub.Subscribe(c, VoiceTopic(chA)) + + // Simulate handleVoiceJoin's state-setting step (voice_join.go's + // c.setVoiceState + pubsub.Subscribe) landing exactly between + // CleanupVoiceForChannel's read of the client's current voice channel and + // its clear of that state. + cleanupVoiceRaceClearHook = func(client *Client) { + client.setVoiceState(chB, "tok-b") + h.pubsub.Subscribe(client, VoiceTopic(chB)) + } + defer func() { cleanupVoiceRaceClearHook = nil }() + + h.CleanupVoiceForChannel(chA) + + if got := c.getVoiceChID(); got != chB { + t.Fatalf("client voiceChID = %d after a voice_join raced CleanupVoiceForChannel's read-then-clear window, want %d — the newer join must survive, not be silently wiped", got, chB) + } + if !h.SubscribedToVoiceTopicForTest(c, chB) { + t.Error("client lost its new channel's voice-topic subscription to a concurrent CleanupVoiceForChannel clear") + } +} diff --git a/Server/ws/reconnect_interior_gap_test.go b/Server/ws/reconnect_interior_gap_test.go new file mode 100644 index 00000000..ea411b63 --- /dev/null +++ b/Server/ws/reconnect_interior_gap_test.go @@ -0,0 +1,149 @@ +package ws_test + +// reconnect_interior_gap_test.go — regression test for OC-0062: the cold-tier +// replay path checked only for a *prefix* gap (retention pruning ahead of +// last_seq) and a *tail* gap (ring buffer not covering the post-flush tail), +// but never checked for a *hole in the middle* of the persisted range. The +// EventPersister can lose an individual row (a full queue drops silently in +// Enqueue, and a per-row insert failure inside a batch flush is logged but +// never surfaced to the replay path — see event_persister.go), leaving the +// events table with an interior gap. handleReconnect's cold tier must not +// accept that as a complete resume: the client tracks only max(seq), so a +// silently skipped seq can never be requested again. + +import ( + "context" + "encoding/json" + "fmt" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/coder/websocket" + + "github.com/owncord/server/auth" + "github.com/owncord/server/ws" +) + +// TestReconnect_InteriorGap_ForcesFullReady locks the guard that must be added +// to handleReconnect's cold-tier branch: when the persisted rows above +// last_seq have a hole somewhere in the middle (not at the very start, which +// the oldest-seq probe already catches), the cold-tier replay must not be +// delivered as a complete "db" resume. +// +// Setup mirrors TestReconnect_BufferMiss_FallsBackToDBTier, but seq 550 is +// never persisted — simulating a single row the EventPersister lost — while +// every other seq in 501..600 is present. The channel-filtered query +// (channelIDs empty, so only channel_id=0 rows are considered — all of ours +// are global) returns a 99-row result that: +// - is not at the maxColdReplay cap (so the truncation guard doesn't fire) +// - starts at seq 501, i.e. oldest[0].Seq(501) == lastSeq+1(501), so the +// prefix-gap probe passes +// - has its newest row (600) fully covered by the ring buffer tail, so the +// tail-coverage guard passes +// +// Only an unfiltered contiguity check over (last_seq, max_persisted_seq] +// catches the missing 550. +func TestReconnect_InteriorGap_ForcesFullReady(t *testing.T) { + database := openServeTestDB(t) + limiter := auth.NewRateLimiter() + + userID, err := database.CreateUser(context.Background(), "reconnect-gap-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(context.Background(), userID, auth.HashToken(token), "test", "127.0.0.1"); err != nil { + t.Fatalf("CreateSession: %v", err) + } + + // Persist 501..600 EXCEPT 550 — simulating one row the EventPersister + // lost (full-queue drop or a per-row insert failure during flush). + eventStore := openEventStoreDB(t) + bgCtx := context.Background() + for seq := int64(501); seq <= 600; seq++ { + if seq == 550 { + continue + } + payload := fmt.Appendf(nil, `{"seq":%d,"type":"broadcast"}`, seq) + if err := eventStore.PersistEvent(bgCtx, seq, "broadcast", 0, payload); err != nil { + t.Fatalf("PersistEvent seq=%d: %v", seq, err) + } + } + + hub := ws.NewHub(database, limiter, nil) + hub.SetEventStore(eventStore) + go hub.Run() + defer hub.Stop() + + // Ring buffer holds 501..1500, so last_seq=500 misses it and the cold + // tier is consulted; the buffer fully covers everything above the + // newest persisted row (600), so the tail-coverage guard alone would + // wrongly let this replay through. + rb := hub.ReplayBuffer() + dummyPayload := []byte(`{"type":"broadcast"}`) + for seq := uint64(501); seq <= 1500; seq++ { + rb.Push(seq, 0, dummyPayload) + } + if oldest := rb.OldestSeq(); oldest != 501 { + t.Fatalf("pre-condition: expected oldestSeq=501, got %d", oldest) + } + + handler := ws.ServeWS(hub, database, []string{"*"}) + srv := httptest.NewServer(handler) + defer srv.Close() + + wsURL := "ws" + strings.TrimPrefix(srv.URL, "http") + dialCtx, cancel := context.WithTimeout(bgCtx, 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, "") }() + + 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) + } + + // The first message back must NOT be auth_ok with replay_source="db" — + // accepting the 99-row result as a complete resume silently skips seq + // 550 forever, since the client only ever tracks max(seq). + _, msg, err := conn.Read(dialCtx) + if err != nil { + t.Fatalf("read handshake response: %v", err) + } + var resp map[string]any + if err := json.Unmarshal(msg, &resp); err != nil { + t.Fatalf("unmarshal response: %v; raw=%s", err, msg) + } + if resp["type"] == "auth_ok" { + if payloadField, _ := resp["payload"].(map[string]any); payloadField["replay_source"] == "db" { + t.Fatalf("reconnect accepted a cold-tier replay with an interior gap (missing seq 550) as a complete db-tier resume: %s", msg) + } + } + + _, dbTier, fullTier := hub.ReconnectTierStats() + if dbTier != 0 { + t.Errorf("db tier count = %d, want 0: a persisted range with an interior gap was delivered as a complete resume", dbTier) + } + if fullTier != 1 { + t.Errorf("full tier count = %d, want 1: an interior gap in the persisted range must force a full ready re-sync", fullTier) + } +} diff --git a/Server/ws/serve.go b/Server/ws/serve.go index a5b2df0e..9e910c3c 100644 --- a/Server/ws/serve.go +++ b/Server/ws/serve.go @@ -227,24 +227,50 @@ func (h *Hub) handleReconnect( for _, p := range persisted { persistedTail = append(persistedTail, p.Payload) } - // The EventPersister flushes asynchronously, so cold rows can - // lag the live seq: events broadcast after the last flush sit - // only in the ring buffer. Confirm the buffer can cover - // everything above the newest persisted row — the - // authoritative re-read happens atomically with registerNow - // below, but a hole here must still force a full ready - // rather than a replay with a silent gap at its end. maxPersistedSeq = uint64(persisted[len(persisted)-1].Seq) //nolint:gosec // seq is a counter bounded well below MaxInt64 - switch tail := h.ReplayBuffer().EventsSinceFiltered(maxPersistedSeq, allowedChannelIDs); { - case tail != nil: - case atomic.LoadUint64(&h.seq) == maxPersistedSeq: - // Post-restart empty buffer with the hub seq seeded from - // the store max: nothing was broadcast after the last - // persisted row, so the cold rows alone are complete. - default: - slog.Warn("ws handleReconnect: ring buffer cannot cover the post-flush tail, forcing full ready", - "user_id", c.userID, "max_persisted_seq", maxPersistedSeq) + + // persisted is channel-filtered, so a hole in a channel + // outside allowedChannelIDs would slip past a contiguity + // check on persisted itself — and EventPersister can lose a + // row outright (a full queue drops silently in Enqueue, a + // per-row insert failure inside a batch flush is logged but + // never surfaced here; see event_persister.go). Count the + // UNFILTERED range (lastSeq, maxPersistedSeq] and require + // every seq in it to be present. seq is the events table's + // primary key, so the count can only come up short, never + // over. + expectedCount := maxPersistedSeq - lastSeq + switch gapCount, gapErr := es.CountEventsInRange(ctx, int64(lastSeq), int64(maxPersistedSeq)); { //nolint:gosec // bounded well below MaxInt64 + case gapErr != nil: + slog.Warn("ws handleReconnect: cold-tier contiguity probe failed, forcing full ready", + "user_id", c.userID, "err", gapErr) persistedTail = nil + case uint64(gapCount) != expectedCount: //nolint:gosec // bounded well below MaxInt64 + slog.Warn("ws handleReconnect: cold-tier replay has an interior gap, forcing full ready", + "user_id", c.userID, "last_seq", lastSeq, "max_persisted_seq", maxPersistedSeq, + "expected", expectedCount, "found", gapCount) + persistedTail = nil + } + + if persistedTail != nil { + // The EventPersister flushes asynchronously, so cold rows can + // lag the live seq: events broadcast after the last flush sit + // only in the ring buffer. Confirm the buffer can cover + // everything above the newest persisted row — the + // authoritative re-read happens atomically with registerNow + // below, but a hole here must still force a full ready + // rather than a replay with a silent gap at its end. + switch tail := h.ReplayBuffer().EventsSinceFiltered(maxPersistedSeq, allowedChannelIDs); { + case tail != nil: + case atomic.LoadUint64(&h.seq) == maxPersistedSeq: + // Post-restart empty buffer with the hub seq seeded from + // the store max: nothing was broadcast after the last + // persisted row, so the cold rows alone are complete. + default: + slog.Warn("ws handleReconnect: ring buffer cannot cover the post-flush tail, forcing full ready", + "user_id", c.userID, "max_persisted_seq", maxPersistedSeq) + persistedTail = nil + } } if persistedTail != nil { events = persistedTail