diff --git a/.claude/workflows/bughunt-fix.harness.mjs b/.claude/workflows/bughunt-fix.harness.mjs index 3f527f40..10db8ac0 100644 --- a/.claude/workflows/bughunt-fix.harness.mjs +++ b/.claude/workflows/bughunt-fix.harness.mjs @@ -254,7 +254,7 @@ scenarios.f6_prove_is_serial = async () => { assert.equal(proveCalls.length, 2) for (const c of proveCalls) { assert.equal(c.opts.model, 'opus') - assert.equal(c.opts.effort, 'medium') + assert.equal(c.opts.effort, 'high') } assert.equal(result.commits.length, 2) assert.deepEqual(result.commits[0], { sha: 'abc1234', file: findings[0].file, ids: ['OC-0001'] }) @@ -425,7 +425,7 @@ scenarios.f10_gate_targets_touched_stacks = async () => { const gateCalls = calls.filter((c) => c.opts.label === 'gate') assert.equal(gateCalls.length, 1, 'ci-check runs once, not per fix') assert.equal(gateCalls[0].opts.model, 'sonnet') - assert.equal(gateCalls[0].opts.effort, 'medium') + assert.equal(gateCalls[0].opts.effort, 'xhigh') assert.match(gatePrompt, /no-experimental-webstorage/, 'client gate command must be spelled out') assert.match(gatePrompt, /go build -tags otel/, 'server gate must cover the tagged build variants') assert.equal(result.gate.passed, true) diff --git a/.claude/workflows/bughunt-fix.js b/.claude/workflows/bughunt-fix.js index 1f643380..4e9ec280 100644 --- a/.claude/workflows/bughunt-fix.js +++ b/.claude/workflows/bughunt-fix.js @@ -5,8 +5,8 @@ export const meta = { phases: [ { title: 'Plan', detail: 'cluster open findings by file' }, { title: 'Fix', detail: 'sonnet/xhigh: one agent per file, test-first, no git' }, - { title: 'Prove', detail: 'sonnet: serial revert-proof then commit per cluster' }, - { title: 'Gate', detail: 'sonnet: ci-check for the touched stacks, once' }, + { title: 'Prove', detail: 'opus/high: serial revert-proof then commit per cluster' }, + { title: 'Gate', detail: 'sonnet/xhigh: ci-check for the touched stacks, once' }, ], } @@ -367,7 +367,7 @@ for (const { cluster, results, union } of fixed) { label: `prove:${cluster.file}`, phase: 'Prove', model: 'opus', - effort: 'medium', + effort: 'high', schema: PROVE_RESULT, }).catch(() => null) @@ -469,7 +469,7 @@ if (commits.length) { `Known false alarm: a windows -race failure inside ws whose stack mentions runtime.scanstack or ` + `runtime.(*unwinder).next is a Go 1.26.5 runtime GC fault, not a real failure - rerun that package ` + `once before reporting it.`, - { label: 'gate', phase: 'Gate', model: 'sonnet', effort: 'medium', schema: GATE_RESULT }, + { label: 'gate', phase: 'Gate', model: 'sonnet', effort: 'xhigh', schema: GATE_RESULT }, ).catch(() => null) // A malformed/missing report (dead agent, or a schema the caller didn't honor) is treated as a // failed gate, same as the null-check pattern in phases 2 and 3 - never crash on shape here. diff --git a/.claude/workflows/bughunt.js b/.claude/workflows/bughunt.js index 3bf0e70a..231016dd 100644 --- a/.claude/workflows/bughunt.js +++ b/.claude/workflows/bughunt.js @@ -454,7 +454,7 @@ const recon = await parallel([ `Count how often each non-test source file changed. Return the 25 most-churned files with their counts, ` + `plus any file that changed in more than 6 distinct commits. High churn = where bugs concentrate.\n` + `Return plain text: one "path count" per line, most-churned first. No commentary.`, - { label: 'recon:churn', phase: 'Recon', model: 'haiku', effort: 'low' }, + { label: 'recon:churn', phase: 'Recon', model: 'haiku', effort: 'xhigh' }, ), () => agent( @@ -464,7 +464,7 @@ const recon = await parallel([ ` (b) every Client/tauri-client/src/**/*.ts (non-test) containing "addEventListener", "setInterval", "setTimeout", or "new AbortController"\n` + ` (c) every Client/tauri-client/src-tauri/src/*.rs containing "unsafe", "Mutex", "RwLock", "spawn", or "unwrap()"\n` + `For each file give the path and a rough hit count. Return plain text grouped under (a)/(b)/(c). No commentary, no analysis.`, - { label: 'recon:surface', phase: 'Recon', model: 'haiku', effort: 'low' }, + { label: 'recon:surface', phase: 'Recon', model: 'haiku', effort: 'xhigh' }, ), ]) const CONTEXT = `\n\n--- RECON: most-churned files (last 8 weeks) ---\n${recon[0] || 'unavailable'}\n\n--- RECON: concurrency & lifecycle surface ---\n${recon[1] || 'unavailable'}\n` diff --git a/Client/tauri-client/src-tauri/src/secret_store.rs b/Client/tauri-client/src-tauri/src/secret_store.rs index fd9be779..dd2ec7c9 100644 --- a/Client/tauri-client/src-tauri/src/secret_store.rs +++ b/Client/tauri-client/src-tauri/src/secret_store.rs @@ -99,7 +99,12 @@ pub fn set(app: &AppHandle, account: &str, secret: &str) -> Result Result<(), String> { + delete_with(account, keyring_delete, |acct| clear_fallback(app, acct)) +} + +/// Core decision logic for [`delete`], with the keyring and fallback removals +/// injected so the branching is testable without a live OS credential store. +fn delete_with( + account: &str, + keyring_delete: impl Fn(&str) -> Result<(), String>, + fallback_clear: impl FnOnce(&str) -> Result<(), String>, +) -> Result<(), String> { let keyring_result = keyring_delete(account); - clear_fallback(app, account); - keyring_result + // `Result::and`'s argument is evaluated eagerly, so `fallback_clear` runs + // regardless of whether the keyring delete succeeded — both stores are + // still cleared even if one errors. Whichever side failed is what gets + // reported: a delete must not read as Ok(()) while either store still + // holds the "deleted" secret. + keyring_result.and(fallback_clear(account)) } // --------------------------------------------------------------------------- @@ -395,19 +414,27 @@ fn get_fallback(app: &AppHandle, account: &str) -> Option { .ok() } -/// Drop any fallback copy of `account`. Best-effort: a failure here is logged, -/// never propagated, because it must not mask the outcome of the real store. -fn clear_fallback(app: &AppHandle, account: &str) { - let Ok(store) = app.store(CREDENTIAL_FALLBACK_STORE) else { - return; - }; +/// Drop any fallback copy of `account`, flushing the removal to disk. +/// +/// Returns the flush error to the caller instead of only logging it: a +/// `delete()` that reported success while this failed to flush would leave +/// the sealed secret on disk to resurrect the "deleted" credential on the +/// next read. Callers where the keyring copy is authoritative (a `set()` +/// recovering from a stale fallback) may still discard the `Err` themselves. +fn clear_fallback(app: &AppHandle, account: &str) -> Result<(), String> { + let store = app + .store(CREDENTIAL_FALLBACK_STORE) + .map_err(|e| format!("failed to open credential fallback store: {e}"))?; // `delete` reports whether a key was present; only flush when one was, so // the common healthy path does not rewrite the file on every save. if store.delete(account) { if let Err(e) = store.save() { - log::warn!("failed to flush credential fallback removal for '{account}': {e}"); + return Err(format!( + "failed to flush credential fallback removal for '{account}': {e}" + )); } } + Ok(()) } // --------------------------------------------------------------------------- @@ -554,6 +581,43 @@ mod tests { assert!(cleared.get(), "a recovered machine must clear any stale fallback copy"); } + // -- delete_with: finding "delete must not report success while the + // fallback copy survives on disk to resurrect a deleted secret" -- + + #[test] + fn delete_with_propagates_a_fallback_flush_failure() { + // The bug: a delete that removed the keyring entry but failed to + // flush the fallback file's removal must not report Ok(()) — the + // sealed secret is still on disk and comes back on the next launch. + let result = delete_with("acct", |_| Ok(()), |_| Err("disk full".to_string())); + assert_eq!(result, Err("disk full".to_string())); + } + + #[test] + fn delete_with_clears_the_fallback_even_when_the_keyring_delete_fails() { + use std::cell::Cell; + let fallback_cleared = Cell::new(false); + let result = delete_with( + "acct", + |_| Err("keyring delete failed".to_string()), + |_| { + fallback_cleared.set(true); + Ok(()) + }, + ); + assert_eq!(result, Err("keyring delete failed".to_string())); + assert!( + fallback_cleared.get(), + "delete must still clear the fallback even when the keyring delete errors" + ); + } + + #[test] + fn delete_with_succeeds_when_both_stores_clear() { + let result = delete_with("acct", |_| Ok(()), |_| Ok(())); + assert_eq!(result, Ok(())); + } + #[cfg(windows)] #[test] fn dpapi_round_trips_and_rejects_foreign_entropy() { diff --git a/Client/tauri-client/src/components/VoiceWidget.ts b/Client/tauri-client/src/components/VoiceWidget.ts index 383efcc1..547baf73 100644 --- a/Client/tauri-client/src/components/VoiceWidget.ts +++ b/Client/tauri-client/src/components/VoiceWidget.ts @@ -185,15 +185,31 @@ export function createVoiceWidget(options: VoiceWidgetOptions): MountableCompone } /** Header E2EE status: dynamic label + a persistent "secured" lock once the - * room key is ready (docs/architecture/ux/voice-and-e2ee.md §2). */ - function updateStatus(status: VoiceStatus): void { + * room key is ready (docs/architecture/ux/voice-and-e2ee.md §2). + * `encryptionDegraded` (OC-0002) is the SDK's own signal — via + * RoomEvent.EncryptionError, wired in livekitSession.ts's createRoom() — + * that the E2EE worker died after the key exchange already succeeded. + * voiceStatus alone reaches "connected" in that case, so the badge must + * never claim "Secured" from voiceStatus in isolation: it renders a + * distinct, still-visible not-secured warning instead of just hiding. */ + function updateStatus(status: VoiceStatus, encryptionDegraded: boolean): void { if (statusLabel !== null) { setText(statusLabel, STATUS_LABELS[status]); statusLabel.classList.toggle("vw-securing", status === "securing"); statusLabel.classList.toggle("vw-reconnecting", status === "reconnecting"); } if (securedBadge !== null) { - securedBadge.style.display = status === "connected" ? "inline-flex" : "none"; + const connected = status === "connected"; + const degraded = connected && encryptionDegraded; + securedBadge.classList.toggle("vw-secured--degraded", degraded); + if (degraded) { + setText(securedBadge, "⚠️ Unsecured"); + securedBadge.title = "End-to-end encryption failed — this call may not be protected"; + } else { + setText(securedBadge, "🔒 Secured"); + securedBadge.title = "End-to-end encrypted"; + } + securedBadge.style.display = connected ? "inline-flex" : "none"; } } @@ -228,7 +244,7 @@ export function createVoiceWidget(options: VoiceWidgetOptions): MountableCompone root.classList.add("visible"); startStatsPoller(); startElapsedTimer(); - updateStatus(voice.voiceStatus); + updateStatus(voice.voiceStatus, voice.encryptionDegraded === true); updateFrozen(uiStore.getState().connectionStatus); // Channel name. A DM call resolves through the DM store rather than the @@ -464,6 +480,7 @@ export function createVoiceWidget(options: VoiceWidgetOptions): MountableCompone screenshare: s.localScreenshare, listenOnly: s.listenOnly, voiceStatus: s.voiceStatus, + encryptionDegraded: s.encryptionDegraded === true, }), () => render(), (a, b) => @@ -475,7 +492,8 @@ export function createVoiceWidget(options: VoiceWidgetOptions): MountableCompone a.camera === b.camera && a.screenshare === b.screenshare && a.listenOnly === b.listenOnly && - a.voiceStatus === b.voiceStatus, + a.voiceStatus === b.voiceStatus && + a.encryptionDegraded === b.encryptionDegraded, ), ); // Freeze controls reactively when the WS socket drops (§3 connection status). diff --git a/Client/tauri-client/src/lib/dispatcher.ts b/Client/tauri-client/src/lib/dispatcher.ts index 80e760eb..14ecbb35 100644 --- a/Client/tauri-client/src/lib/dispatcher.ts +++ b/Client/tauri-client/src/lib/dispatcher.ts @@ -817,8 +817,16 @@ export function wireDispatcher( void handleParticipantLeft(payload.user_id); if (shouldTeardownSession) void leaveVoice(false); }); - // Clear local voice state if the current user was removed (kick/disconnect) - if (isSelf) { + // Clear local voice state only for the same channel-match case as the + // LiveKit teardown above. A channel switch optimistically moves the + // store's currentChannelId to the NEW channel before the server + // responds (VoiceCallbacks.onVoiceJoin); the server always leaves the + // OLD channel first, so an unconditional clear here would blank the + // store back to null on every switch — hiding the whole voice widget + // (including its leave/mute controls) until a later voice_state + // happens to restore it, or forever if the switch then fails + // server-side. + if (shouldTeardownSession) { leaveVoiceChannel(); } }), @@ -938,7 +946,17 @@ export function wireDispatcher( }); if (payload.code === "BANNED") { // Banned users must not reconnect — show error and force logout. + // The server answers a ban with a generic `error` frame (not + // `auth_error`), so ws.ts never sets intentionalClose for this path. + // main.ts's authStore subscriber would normally do that teardown, + // but it only runs once the router has reached "main" — during + // login / auto-login / the connected-overlay window it hasn't, so + // left to that subscriber alone the client redials the same banned + // token via scheduleReconnect() forever (OC-0107). Disconnect here + // directly: it's idempotent with that subscriber's own + // ws.disconnect() and covers every router state, not just "main". setTransientError(payload.message || "You have been banned"); + ws.disconnect(); clearAuth(); return; } @@ -984,10 +1002,12 @@ export function wireDispatcher( // refusal earns no voice_leave (there was no previous channel to // leave), so nothing else clears that optimistic state — the sidebar // is left keyed on a channel with no LiveKit session. A channel - // *switch* refusal doesn't need this: the server always leaves the - // old channel first, whose self voice_leave already reset - // voiceStatus to idle before this error arrives, so the guard is a - // no-op there. + // *switch* refusal hits the same guard: the self voice_leave for the + // OLD channel that precedes it no longer resets voiceStatus (OC-0015 + // — that voice_leave's channel no longer matches the already-updated + // currentChannelId, so it must not tear down the NEW channel's + // optimistic state either), so voiceStatus is still "joining" when + // this error lands and the guard clears it here instead. if (voiceStore.getState().voiceStatus === "joining") { leaveVoiceChannel(); } diff --git a/Client/tauri-client/src/lib/livekitE2EE.ts b/Client/tauri-client/src/lib/livekitE2EE.ts index 98ff1aff..f82ac7ab 100644 --- a/Client/tauri-client/src/lib/livekitE2EE.ts +++ b/Client/tauri-client/src/lib/livekitE2EE.ts @@ -216,6 +216,20 @@ export class E2EEManager { // processed with no offer sent) and gets its offer sent below. this._ecdhKeyPair = ecdhKeyPair; + if (this._isKeyHolder) { + // Announce our (signed) key BEFORE draining queued announces. The + // drain below sends each drained peer a voice_e2ee_offer, and the + // receiver can only unwrap it once it has OUR ephemeral public key on + // file — which only our own announce provides. The relayed announce + // and our offer land in the SAME inbound WS queue on the peer's side + // (voice_join relay and the offer send both go through the peer's + // c.send queue), so the send order here is the delivery order there. + // Announcing AFTER the drain (the old order) meant every existing + // participant's handleOfferInner discarded our offer as "unknown + // peer" and never recovered short of the 5-minute rotation (OC-0098). + this.deps.getWs()?.send({ type: "voice_e2ee_announce", payload: announcePayload }); + } + // Drain any announces that arrived before our keypair was ready. These // are existing participants whose keys the server relayed during // voice_join sync — run them through the normal verifying receive path @@ -227,10 +241,7 @@ export class E2EEManager { log.info("E2EE: drained queued announce", { userId: qId }); } - if (this._isKeyHolder) { - // Announce our (signed) key so existing participants can see us. - this.deps.getWs()?.send({ type: "voice_e2ee_announce", payload: announcePayload }); - } else { + if (!this._isKeyHolder) { // Wait for the key holder to send us the room key via voice_e2ee_offer. // This promise resolves when handleOffer() sets _roomKey. log.info("E2EE: waiting for room key from key holder", { channelId }); @@ -339,6 +350,50 @@ export class E2EEManager { return; } this.deps.getWs()?.send({ type: "voice_e2ee_announce", payload: reconnectAnnounce }); + + // Non-key-holders: nothing here waits for, times out, or retries the + // holder's confirming offer — the caller (connectAndSetup's reconnect + // path) marks the call "Secured" the moment room.connect() resolves, + // regardless of whether the re-applied (possibly stale, rotated-during- + // the-outage) room key was ever confirmed current. Arm a bounded, + // non-blocking check: if nothing has replaced this room key by the time + // it fires, log it (there was previously no observable signal at all) + // and retry the announce once — a real bound short of the 5-minute + // periodic rotation (OC-0007). Holders don't need this: their own key + // IS the current one. + this.clearReconnectConfirmTimer(); + const roomKeyAtReconnect = this._roomKey; + // Only meaningful when we actually re-applied a pre-existing key — with + // none yet, there is nothing that could have gone "stale" and this is + // just the ordinary first-offer wait (setupKeyExchange's own concern). + if (!this._isKeyHolder && roomKeyAtReconnect !== null) { + this._reconnectConfirmTimer = setTimeout(() => { + this._reconnectConfirmTimer = null; + if ( + this._ecdhKeyPair !== pair || + this._roomKey !== roomKeyAtReconnect || + this._isKeyHolder + ) { + return; // superseded, already reconfirmed by a fresh offer, or re-elected holder + } + log.error("E2EE: room key not reconfirmed after reconnect — may be stale, re-announcing", { + channelId: this._channelId, + }); + this.deps.getWs()?.send({ type: "voice_e2ee_announce", payload: reconnectAnnounce }); + }, E2EEManager.RECONNECT_CONFIRM_MS); + } + } + + /** How long to wait after a reconnect re-announce before treating a + * non-holder's re-applied room key as unconfirmed (see reannounceForReconnect). */ + private static readonly RECONNECT_CONFIRM_MS = 5_000; + private _reconnectConfirmTimer: ReturnType | null = null; + + private clearReconnectConfirmTimer(): void { + if (this._reconnectConfirmTimer !== null) { + clearTimeout(this._reconnectConfirmTimer); + this._reconnectConfirmTimer = null; + } } // ── Identity signing (F3 TOFU) ────────────────────────────────────────── @@ -805,6 +860,20 @@ export class E2EEManager { } } + /** Server-side cap is voiceE2EEOfferRateLimit = 64 offers per (sender, + * channel) per second (Server/ws/voice_e2ee.go) — a whole rotation's + * offers can exceed it in a large channel, and everything past the cap is + * dropped with no client-side signal, starving the same tail peers (in + * stable Map insertion order) on every subsequent rotation (OC-0005). + * Stay under it with margin rather than reading the limit back from the + * server. */ + private static readonly OFFER_RATE_LIMIT_PER_SEC = 60; + /** ponytail: fixed batch+sleep pacing, not a token bucket — the server + * window is a flat per-second cap, so "send 60, wait a bit over a + * second" is the whole algorithm needed. Upgrade if the cap ever becomes + * variable or sub-second. */ + private static readonly OFFER_RATE_WINDOW_MS = 1_100; + /** * Wrap the room key for each peer and send an offer, one at a time. Bails * out (without sending further offers) as soon as a concurrent keypair @@ -813,12 +882,18 @@ export class E2EEManager { * peer and would otherwise silently strand them on the stale key until the * next rotation (finding v045). Shared by the become-holder distribution, * its late-arrival (H3) pass, and the periodic rotation loop. + * + * Paces sends at OFFER_RATE_LIMIT_PER_SEC per OFFER_RATE_WINDOW_MS to stay + * under the server's per-(sender,channel) rate limit (OC-0005) — without + * this, a rotation in a large channel silently drops every offer past the + * cap, and the same tail peers stay stranded on the old key forever. */ private async distributeRoomKey( keypair: CryptoKeyPair, roomKey: Uint8Array, peers: Iterable<[number, CryptoKey]>, ): Promise { + let sentInWindow = 0; for (const [peerId, peerKey] of peers) { if (this._ecdhKeyPair !== keypair || this._roomKey !== roomKey) { log.warn("E2EE: aborting key distribution — keypair/room key changed mid-loop", { @@ -826,6 +901,19 @@ export class E2EEManager { }); return; } + if (sentInWindow >= E2EEManager.OFFER_RATE_LIMIT_PER_SEC) { + await new Promise((resolve) => setTimeout(resolve, E2EEManager.OFFER_RATE_WINDOW_MS)); + sentInWindow = 0; + if (this._ecdhKeyPair !== keypair || this._roomKey !== roomKey) { + log.info( + "E2EE: aborting key distribution — keypair/room key changed during pacing pause", + { + peerId, + }, + ); + return; + } + } const { encryptedKey, iv } = await wrapRoomKey(keypair.privateKey, peerKey, roomKey); if (this._ecdhKeyPair !== keypair || this._roomKey !== roomKey) { log.info("E2EE: discarding stale room-key offer (keypair/room key changed during wrap)", { @@ -837,9 +925,44 @@ export class E2EEManager { type: "voice_e2ee_offer", payload: { target_user_id: peerId, encrypted_key: encryptedKey, iv }, }); + sentInWindow++; } } + /** + * Bump the epoch and install a fresh room key on the shared key provider — + * the two mutations every rotation path performs before distributing to + * peers. Shared so the session-generation guard lives in one place instead + * of being duplicated (or omitted) at each call site (OC-0006). + * + * If a clearState()/rejoin supersedes this rotation while the `setKey` + * await is in flight, the call has already been issued to the shared + * keyProvider and cannot be un-sent — so on resume this re-applies + * whatever room key the NOW-current session holds (if any), self-healing + * the provider instead of silently leaving it on our abandoned key. Narrow + * (it needs two setKey calls to resolve out of order), but real. + * + * Returns the new room key, or null if superseded — callers must skip + * their own distribution step in that case. + */ + private async rotateRoomKey(): Promise { + const myGeneration = this._sessionGeneration; + this._e2eeEpoch++; + const roomKey = generateRoomKey(); + this._roomKey = roomKey; + await this.keyProvider.setKey(roomKeyToBase64(roomKey)); + if (this._sessionGeneration !== myGeneration) { + log.error( + "E2EE: rotation superseded while setKey was in flight — re-applying the live session's key", + ); + if (this._roomKey) { + await this.keyProvider.setKey(roomKeyToBase64(this._roomKey)); + } + return null; + } + return roomKey; + } + /** * Handle a participant leaving the voice channel. If we become the new key * holder, rotate the room key and distribute to remaining peers. If we are @@ -861,16 +984,26 @@ export class E2EEManager { const state = voiceStore.getState(); const channelUsers = state.voiceUsers.get(channelId); - if (!channelUsers || channelUsers.size === 0) return; + const myUserId = authStore.getState().user?.id ?? 0; - // Elect key holder: lowest user_id among remaining participants. - let lowestUserId = Infinity; - for (const uid of channelUsers.keys()) { - if (uid < lowestUserId) lowestUserId = uid; + // Elect key holder: lowest user_id among remaining participants. The + // local roster comes from voice_state broadcasts, including our own — + // which can arrive AFTER a peer's voice_leave when we joined the channel + // concurrently with their departure (the server already elected us; our + // own broadcast is still queued behind theirs on the hub). Seed the + // roster with our own id unconditionally so that race can't leave the + // channel's roster entry empty/missing and silently skip election + // (OC-0004) — the join that provoked it would otherwise time out 15s + // later with no recovery. + let lowestUserId = myUserId !== 0 ? myUserId : Infinity; + if (channelUsers) { + for (const uid of channelUsers.keys()) { + if (uid < lowestUserId) lowestUserId = uid; + } } + if (lowestUserId === Infinity) return; // nobody known yet, including ourselves const wasKeyHolder = this._isKeyHolder; - const myUserId = authStore.getState().user?.id ?? 0; if (myUserId !== 0 && lowestUserId === myUserId && !wasKeyHolder) { // A rotation is already in flight (e.g. we stood down mid-rotation @@ -895,9 +1028,12 @@ export class E2EEManager { // Rotate the room key — generate a new one and distribute to all remaining peers. try { - this._e2eeEpoch++; - this._roomKey = generateRoomKey(); - await this.keyProvider.setKey(roomKeyToBase64(this._roomKey)); + const roomKey = await this.rotateRoomKey(); + if (roomKey === null) { + // Superseded while setKey was in flight — the now-current session + // owns its own key-holder role and rotation; nothing left to do. + return; + } log.info("E2EE: rotated room key", { channelId, epoch: this._e2eeEpoch }); // A client elected while still waiting inside setupKeyExchange has a @@ -909,14 +1045,13 @@ export class E2EEManager { this._roomKeyRejector = null; } - // Snapshot peers (and the keypair/room key) before the async loop — - // new peers that arrive during wrapping are handled by the - // post-rotation check below. + // Snapshot peers (and the keypair) before the async loop — new peers + // that arrive during wrapping are handled by the post-rotation check + // below. const keypair = this._ecdhKeyPair; - const roomKey = this._roomKey; const peersSnapshot = new Map(this._peerPublicKeys); - if (keypair && roomKey) { + if (keypair) { await this.distributeRoomKey(keypair, roomKey, peersSnapshot); log.info("E2EE: distributed rotated key to peers", { peerCount: peersSnapshot.size, @@ -993,14 +1128,16 @@ export class E2EEManager { this._rotatingKey = true; try { - this._e2eeEpoch++; - this._roomKey = generateRoomKey(); - await this.keyProvider.setKey(roomKeyToBase64(this._roomKey)); + const roomKey = await this.rotateRoomKey(); + if (roomKey === null) { + // Superseded while setKey was in flight — the now-current session + // owns its own key-holder role and rotation; nothing left to do. + return; + } log.info("E2EE: periodic key rotation", { channelId, epoch: this._e2eeEpoch }); const keypair = this._ecdhKeyPair; - const roomKey = this._roomKey; - if (keypair && roomKey) { + if (keypair) { const peerCount = this._peerPublicKeys.size; // Pass the live map (not a snapshot): peers that arrive mid-loop are // still visited, matching the original behavior — only the @@ -1049,6 +1186,7 @@ export class E2EEManager { this._e2eeEpoch = 0; this._pendingAnnounces.length = 0; this.clearKeyRotationTimer(); + this.clearReconnectConfirmTimer(); // Reject (not resolve) so waiting setupKeyExchange sees a failure, not a // silent success with no room key. if (this._roomKeyRejector) { diff --git a/Client/tauri-client/src/lib/livekitSession.ts b/Client/tauri-client/src/lib/livekitSession.ts index de860cea..dcba6b50 100644 --- a/Client/tauri-client/src/lib/livekitSession.ts +++ b/Client/tauri-client/src/lib/livekitSession.ts @@ -373,7 +373,7 @@ export class LiveKitSession { * through the process-lifetime key provider's setKey fan-out. */ private _e2eeWorker: Worker | null = null; - private createRoom(): Room { + private async createRoom(): Promise { // livekit's per-room E2EEManager registers a SetKey listener on the // shared key provider and never removes it; only those managers // subscribe, so clear them all before the new Room re-registers. @@ -412,6 +412,15 @@ export class LiveKitSession { worker: this._e2eeWorker, }, }); + // OC-0095: the Room constructor only wires up the E2EEManager — it never + // enables encryption. Without this, LocalParticipant.encryptionType stays + // NONE, the worker's encode transform takes the disabled passthrough + // branch, and every frame reaches the SFU in plaintext even though the + // full ECDH/HKDF/AES-GCM key exchange above completed successfully. + // Safe to call before connect(): the manager just records the enabled + // flag and it's a no-op today for the "" pre-connect identity, then wires + // up for real once the SignalConnected handler has the real identity. + await newRoom.setE2EEEnabled(true); newRoom.on(RoomEvent.TrackSubscribed, this._eventHandlers.handleTrackSubscribed); newRoom.on(RoomEvent.TrackUnsubscribed, this._eventHandlers.handleTrackUnsubscribed); newRoom.on(RoomEvent.Disconnected, this._eventHandlers.handleDisconnected); @@ -421,6 +430,9 @@ export class LiveKitSession { this._eventHandlers.handleAudioPlaybackChanged, ); newRoom.on(RoomEvent.LocalTrackPublished, this._eventHandlers.handleLocalTrackPublished); + // OC-0002: the only SDK-level signal that the E2EE worker died after the + // key exchange already succeeded — see roomEventHandlers.ts for detail. + newRoom.on(RoomEvent.EncryptionError, this._eventHandlers.handleEncryptionError); attachDiagnosticListeners(newRoom); return newRoom; @@ -483,7 +495,8 @@ export class LiveKitSession { // room: this._room is null while state is "reconnecting". let attemptRoom: Room | null = null; try { - const newRoom = this.createRoom(); + // oxlint-disable-next-line no-await-in-loop -- sequential reconnect: must create+arm E2EE before connect + const newRoom = await this.createRoom(); attemptRoom = newRoom; const cleanupAbortedReconnect = async (): Promise => { newRoom.removeAllListeners(); @@ -710,8 +723,11 @@ export class LiveKitSession { // --- Token refresh --- - /** Token refresh interval: 23 hours (refresh 1h before 24h TTL expiry). */ - private static readonly TOKEN_REFRESH_MS = 23 * 60 * 60 * 1000; + /** Token refresh interval: 4 minutes (refresh 1 min before the server's + * 5-minute TTL expiry — see Server/ws/livekit.go tokenTTL). Must stay + * below that TTL or a network blip after minute 5 hands attemptAutoReconnect + * an already-expired token and every reconnect attempt fails (OC-0014). */ + private static readonly TOKEN_REFRESH_MS = 4 * 60 * 1000; private startTokenRefreshTimer(): void { this.clearTokenRefreshTimer(); @@ -778,12 +794,12 @@ export class LiveKitSession { // rotate the token on an active connection. We store the fresh token so // that reconnection (auto-reconnect or manual rejoin) uses it, but the // live session continues with the original token. This means: - // - Sessions longer than the 4h TTL remain connected (LiveKit keeps - // active connections alive) but lose the ability to reconnect after a - // network blip once the original token expires. - // - The 23h refresh timer ensures a fresh token is always ready + // - Sessions longer than the server's 5-minute TTL remain connected + // (LiveKit keeps active connections alive) but lose the ability to + // reconnect after a network blip once the original token expires. + // - The 4-minute refresh timer ensures a fresh token is always ready // *before* the original expires, so reconnects within the window work. - // See also: Server/ws/livekit.go tokenTTL constant. + // See also: Server/ws/livekit.go tokenTTL constant (5 * time.Minute). if (token && this._state.type === "connected") { this.setState({ ...this._state, latestToken: token }); } else if (token && this._state.type === "reconnecting") { @@ -930,7 +946,14 @@ export class LiveKitSession { directUrl?: string, isKeyHolder?: boolean, ): Promise { - if (this._room !== null) this.leaveVoice(false); + // Also tear down (and abort) an in-flight reconnect: `_room` reads null + // for the whole "reconnecting" state, so a join issued while the LiveKit + // auto-reconnect loop is running would otherwise skip leaveVoice(false) + // entirely — meaning _e2ee.clearState() never runs, and setupKeyExchange + // below inherits the PREVIOUS channel's residual _isKeyHolder via its + // OR-with-server-value guard, joining the new channel as a phantom key + // holder the server never elected (OC-0020). + if (this._room !== null || this._state.type === "reconnecting") this.leaveVoice(false); // Draw the next generation from the monotonic instance counter (never // re-derived from `_state`) and embed it into the "connecting" state. // Any newer call to connectAndSetup() will produce a strictly larger @@ -947,7 +970,7 @@ export class LiveKitSession { // been claimed by a newer attempt). let localRoom: Room | null = null; try { - localRoom = this.createRoom(); + localRoom = await this.createRoom(); this._audioPipeline.setRoom(localRoom); this._audioElements.setRoom(localRoom); this._deviceManager.setRoom(localRoom); @@ -1070,7 +1093,8 @@ export class LiveKitSession { } if (localRoom === null) throw connectErr; localRoom.removeAllListeners(); - localRoom = this.createRoom(); + // oxlint-disable-next-line no-await-in-loop -- sequential retry: must arm E2EE before the next connect attempt + localRoom = await this.createRoom(); this._audioPipeline.setRoom(localRoom); this._audioElements.setRoom(localRoom); this._deviceManager.setRoom(localRoom); diff --git a/Client/tauri-client/src/lib/roomEventHandlers.ts b/Client/tauri-client/src/lib/roomEventHandlers.ts index c2c3f103..753f2965 100644 --- a/Client/tauri-client/src/lib/roomEventHandlers.ts +++ b/Client/tauri-client/src/lib/roomEventHandlers.ts @@ -8,7 +8,12 @@ import { type LocalTrackPublication, DisconnectReason, } from "livekit-client"; -import { voiceStore, setSpeakers, leaveVoiceChannel } from "@stores/voice.store"; +import { + voiceStore, + setSpeakers, + leaveVoiceChannel, + setEncryptionDegraded, +} from "@stores/voice.store"; import { createLogger } from "@lib/logger"; import { parseUserId } from "@lib/livekitSession"; import type { AudioElements } from "@lib/audioElements"; @@ -66,6 +71,7 @@ export interface RoomEventHandlers { readonly handleActiveSpeakersChanged: (speakers: Participant[]) => void; readonly handleAudioPlaybackChanged: () => void; readonly handleDisconnected: (reason?: DisconnectReason) => void; + readonly handleEncryptionError: (error: Error, participant?: Participant) => void; readonly removeAutoplayUnlock: () => void; } @@ -201,6 +207,23 @@ export function createRoomEventHandlers(deps: RoomEventDeps): RoomEventHandlers if (isUnexpected) deps.getOnErrorCallback()?.("Voice connection lost — disconnected"); }; + /** OC-0002: livekit-client's E2eeManager emits RoomEvent.EncryptionError + * when the per-room E2EE worker dies (onWorkerError — CSP blocking a + * lazily-loaded chunk, WASM load failure, WebView2 quirk) or when an + * encrypted track arrives on a room without encryption enabled. Either + * way the room key exchange can have already succeeded and voiceStatus + * can already read "connected" — this is the SDK's only signal that the + * encoder itself is not actually protecting frames, so it must reach the + * store the Secured badge reads rather than staying invisible. + */ + const handleEncryptionError = (error: Error, participant?: Participant): void => { + log.error("LiveKit E2EE encryption error — call may not be secured", { + error, + participant: participant?.identity, + }); + setEncryptionDegraded(true); + }; + return { handleLocalTrackPublished, handleTrackSubscribed, @@ -208,6 +231,7 @@ export function createRoomEventHandlers(deps: RoomEventDeps): RoomEventHandlers handleActiveSpeakersChanged, handleAudioPlaybackChanged, handleDisconnected, + handleEncryptionError, removeAutoplayUnlock, }; } diff --git a/Client/tauri-client/src/stores/voice.store.ts b/Client/tauri-client/src/stores/voice.store.ts index 932a2eb2..07cbc6e7 100644 --- a/Client/tauri-client/src/stores/voice.store.ts +++ b/Client/tauri-client/src/stores/voice.store.ts @@ -92,6 +92,16 @@ export interface VoiceState { /** Voice-session lifecycle status (drives the widget's connecting/securing/ * secured indicators). Written from livekitSession.ts. */ readonly voiceStatus: VoiceStatus; + /** True once livekit-client reports a live E2EE encryption failure + * (RoomEvent.EncryptionError — the per-room worker died after the room key + * exchange already succeeded, e.g. a lazily-loaded worker chunk blocked by + * CSP or a WASM load failure). voiceStatus alone reaches "connected" in + * that case, so the Secured badge must also read this flag rather than + * deriving "secured" from voiceStatus === "connected" in isolation. + * Written from roomEventHandlers.ts; latches until the next join/leave. + * Always written by the store; optional only so the many inline VoiceState + * test fixtures need not restate it (same convention as localServerMuted). */ + readonly encryptionDegraded?: boolean; /** Per-peer E2EE identity verification (F3 TOFU), keyed by userId. The store * always sets it; optional only so the many inline VoiceState test fixtures * need not restate it. */ @@ -112,6 +122,7 @@ const INITIAL_STATE: VoiceState = { joinedAt: null, listenOnly: false, voiceStatus: "idle", + encryptionDegraded: false, peerVerifications: new Map(), }; @@ -133,6 +144,7 @@ export function resetVoiceStore(): void { joinedAt: null, listenOnly: false, voiceStatus: "idle", + encryptionDegraded: false, peerVerifications: new Map(), })); } @@ -265,6 +277,9 @@ export function joinVoiceChannel(channelId: number): void { // Optimistic: the widget shows "Connecting…" the moment the user clicks, // before the voice_token round-trip. livekitSession advances it from here. voiceStatus: "joining", + // A fresh join starts clean — any degraded flag belongs to the + // previous session's worker, not this one. + encryptionDegraded: false, }; }); } @@ -280,6 +295,7 @@ export function leaveVoiceChannel(): void { // Server mute lives with the voice session; a new session starts clean. localServerMuted: false, localServerDeafened: false, + encryptionDegraded: false, }; const channelId = prev.currentChannelId; if (channelId === null || currentUserId === 0) { @@ -309,6 +325,16 @@ export function setVoiceStatus(status: VoiceStatus): void { ); } +/** Mark whether the live E2EE encryption has degraded (OC-0002) — set true + * from roomEventHandlers.ts when the room reports RoomEvent.EncryptionError + * (the SDK's own signal that the per-room worker died). Single-writer aside + * from the join/leave resets above, which clear it for a fresh session. */ +export function setEncryptionDegraded(degraded: boolean): void { + voiceStore.setState((prev) => + prev.encryptionDegraded === degraded ? prev : { ...prev, encryptionDegraded: degraded }, + ); +} + /** Toggle local mute state. */ export function setLocalMuted(muted: boolean): void { voiceStore.setState((prev) => ({ diff --git a/Client/tauri-client/src/styles/app.css b/Client/tauri-client/src/styles/app.css index 16054974..8ca4355e 100644 --- a/Client/tauri-client/src/styles/app.css +++ b/Client/tauri-client/src/styles/app.css @@ -482,6 +482,11 @@ font-size: 11px; font-weight: 600; } +/* OC-0002: the E2EE worker died after the key exchange already succeeded — + still shown (never silently hidden), but never green/"Secured" again. */ +.vw-secured.vw-secured--degraded { + color: var(--red, #f23f43); +} .vw-timer { color: var(--green); font-size: 11px; diff --git a/Client/tauri-client/tests/e2e/helpers.ts b/Client/tauri-client/tests/e2e/helpers.ts index fe1e0ecc..0e8122f6 100644 --- a/Client/tauri-client/tests/e2e/helpers.ts +++ b/Client/tauri-client/tests/e2e/helpers.ts @@ -356,6 +356,10 @@ export function voiceWsHandlers(): Array<{ type: string; handler: string }> { type: "voice_join", handler: ` var p = parsed.payload; + // Remember the joined channel so the voice_leave echo can carry the + // real channel id, like the server does (dispatcher's self-leave + // teardown is gated on a channel match, so a wrong id is ignored). + globalThis.__mockVoiceChannel = p.channel_id; setTimeout(function() { // Full VoiceStatePayload shape — the server always sends username // (and the flag fields); the sidebar renders user.username directly, @@ -377,10 +381,12 @@ export function voiceWsHandlers(): Array<{ type: string; handler: string }> { { type: "voice_leave", handler: ` + var ch = globalThis.__mockVoiceChannel || 0; + globalThis.__mockVoiceChannel = 0; setTimeout(function() { __tauriEmitEvent("ws-message", JSON.stringify({ type: "voice_leave", - payload: { user_id: 1, channel_id: 0 } + payload: { user_id: 1, channel_id: ch } })); }, 50); `, diff --git a/Client/tauri-client/tests/e2e/voice-lifecycle.spec.ts b/Client/tauri-client/tests/e2e/voice-lifecycle.spec.ts index 76e223ab..eb91858f 100644 --- a/Client/tauri-client/tests/e2e/voice-lifecycle.spec.ts +++ b/Client/tauri-client/tests/e2e/voice-lifecycle.spec.ts @@ -348,6 +348,16 @@ test.describe("Voice WS flow", () => { await disconnectBtn.click(); await expect(widget).not.toHaveClass(/visible/, { timeout: 5_000 }); + // Wait for the mock's delayed voice_state/voice_leave echoes to be + // processed before re-joining: the hidden-widget assertion above passes + // on the synchronous store clear, but the join's voice_state echo briefly + // re-populates the store until the leave echo clears it again. Clicking + // the row inside that window toggles a LEAVE instead of a join. Our own + // roster entry disappearing is the settle signal for both echoes. + await expect(page.locator(".voice-user-item", { hasText: "testuser" })).toHaveCount(0, { + timeout: 5_000, + }); + // Re-join await joinVoiceChannelByName(page, "Voice Chat"); }); diff --git a/Client/tauri-client/tests/unit/dispatcher.test.ts b/Client/tauri-client/tests/unit/dispatcher.test.ts index 128e8c92..e0817f1a 100644 --- a/Client/tauri-client/tests/unit/dispatcher.test.ts +++ b/Client/tauri-client/tests/unit/dispatcher.test.ts @@ -2068,6 +2068,33 @@ describe("WS Dispatcher", () => { expect(mockLeaveVoice).not.toHaveBeenCalled(); }); + // OC-0015: a channel switch optimistically moves currentChannelId to the + // NEW channel (VoiceCallbacks.onVoiceJoin) before the server responds. The + // server always leaves the OLD channel first, so the self voice_leave for + // that old channel must not blank the store back to null — that would hide + // the entire voice widget (mute/leave controls included) while a session + // may still be live. Same guard as the LiveKit teardown above. + it("does not blank voiceStore.currentChannelId on a self voice_leave for a channel mid-switch away from", async () => { + authStore.setState((prev) => ({ + ...prev, + user: { id: 5, username: "me", avatar: null, role: "member" }, + })); + // Optimistically already moved to the new channel (7); the incoming + // voice_leave is for the old channel (3). + voiceStore.setState((prev) => ({ + ...prev, + currentChannelId: 7, + })); + + mock.dispatch("voice_leave", { + channel_id: 3, + user_id: 5, + }); + await vi.runAllTimersAsync(); + + expect(voiceStore.getState().currentChannelId).toBe(7); + }); + it("mirrors a moderator mute/deafen into the local flags and honors it", async () => { authStore.setState((prev) => ({ ...prev, @@ -2300,6 +2327,27 @@ describe("WS Dispatcher", () => { expect(error).toBe("You have been banned"); }); + it("wires error BANNED to disconnect the ws client (OC-0107: without this the banned token reconnects forever)", () => { + authStore.setState((prev) => ({ + ...prev, + isAuthenticated: true, + user: { id: 1, username: "banned-user", avatar: null, role: "member" }, + })); + + mock.dispatch("error", { + code: "BANNED", + message: "You have been banned from this server", + }); + + // clearAuth() alone flips isAuthenticated, but main.ts's authStore + // subscriber only tears down the ws (and cancels the reconnect loop) when + // the router is already on "main". During login / auto-login / the + // connected-overlay window it is not, so the BANNED handler itself must + // call ws.disconnect() to set intentionalClose and stop scheduleReconnect + // from redialing with the now-cleared but still-cached banned token. + expect(mock.ws.disconnect).toHaveBeenCalled(); + }); + it("wires error RATE_LIMITED to transient error", () => { mock.dispatch("error", { code: "RATE_LIMITED", diff --git a/Client/tauri-client/tests/unit/livekit-e2ee.test.ts b/Client/tauri-client/tests/unit/livekit-e2ee.test.ts index d9318bf9..c96b557f 100644 --- a/Client/tauri-client/tests/unit/livekit-e2ee.test.ts +++ b/Client/tauri-client/tests/unit/livekit-e2ee.test.ts @@ -85,6 +85,7 @@ import { roomKeyToBase64, wrapRoomKey, generateECDHKeyPair, + generateRoomKey, importPublicKey, } from "@lib/e2eeCrypto"; import { getOrCreateIdentityKeyPair, getIdentityPin, storeIdentityPin } from "@lib/identity"; @@ -860,4 +861,148 @@ describe("E2EEManager", () => { // rather than sign/scope under a placeholder id. expect((announces[0] as any).payload.signature).toBeUndefined(); }); + + // ── Ledger findings OC-0098 / OC-0004 / OC-0006 / OC-0005 / OC-0007 ── + + it("[OC-0098] sends its own announce before offering the room key to a drained peer", async () => { + const ws = { send: vi.fn() }; + const mgr = createManager(ws); + + // A peer's announce arrives (relayed by voice_join sync) before our own + // keypair is ready — it queues. + await mgr.handleAnnounce(PEER_ID, "cGVlcg==", "sig"); + expect(mgr.pendingAnnounces).toHaveLength(1); + + // We become key holder and drain the queued announce, which sends that + // peer a voice_e2ee_offer. The peer can only unwrap it if it already has + // OUR ephemeral public key on file — which only our own announce + // provides. Both land in the same inbound WS queue on the peer's side, + // so the order we send them in here IS the order they arrive there. + await mgr.setupKeyExchange(true, 1); + + const calls = ws.send.mock.calls.map((c) => c[0] as { type: string }); + const announceIndex = calls.findIndex((m) => m.type === "voice_e2ee_announce"); + const offerIndex = calls.findIndex((m) => m.type === "voice_e2ee_offer"); + expect(announceIndex).toBeGreaterThanOrEqual(0); + expect(offerIndex).toBeGreaterThanOrEqual(0); + expect(announceIndex).toBeLessThan(offerIndex); + }); + + it("[OC-0004] elects us key holder on a participant-left even when our own voice_state hasn't landed in the roster yet", async () => { + const ws = { send: vi.fn() }; + const mgr = createManager(ws); // getCurrentChannelId() => 1 + // mockVoiceState.voiceUsers has NO entry for channel 1: our own + // voice_state broadcast hasn't landed (it's still queued behind the + // leaver's on the server), and the leaver's own roster entry was just + // deleted by removeVoiceUser before this handler runs. + expect(mockVoiceState.voiceUsers.has(1)).toBe(false); + + await mgr.handleParticipantLeft(PEER_ID); + + // Client-side election must still run using our own authenticated id + // (uid 1) even though the local roster has nothing recorded for the + // channel yet — otherwise the server's promotion is silently missed and + // the join that provoked it times out 15s later. + expect(mgr.epoch).toBe(1); + expect(mockSetKey).toHaveBeenCalledWith("mock-room-key-base64"); + }); + + it("[OC-0006] self-heals the shared key provider when a rotation's setKey resolves after clearState() tore the session down", async () => { + const ws = { send: vi.fn() }; + const mgr = createManager(ws); + mockVoiceState.voiceUsers.set(1, new Map([[1, {}]])); + + await mgr.setupKeyExchange(true, 1); // epoch 1, holder + + vi.mocked(roomKeyToBase64).mockImplementation((k: Uint8Array) => `key-${k[0]}`); + try { + vi.mocked(generateRoomKey).mockReturnValueOnce(new Uint8Array(32).fill(9)); // this rotation's key + let releaseStale!: () => void; + const staleSetKey = new Promise((resolve) => { + releaseStale = resolve; + }); + mockSetKey.mockImplementationOnce(() => staleSetKey); + + const rotationPromise = mgr.rotateKeyPeriodically(); + await vi.waitFor(() => expect(mockSetKey).toHaveBeenCalledWith("key-9")); + + // The session is torn down (user hit Disconnect) while that setKey + // call is still in flight, and a brand-new session becomes holder + // with its OWN key before the stale call resolves. + mgr.clearState(); + vi.mocked(generateRoomKey).mockReturnValueOnce(new Uint8Array(32).fill(7)); // live session's key + mockSetKey.mockClear(); + await mgr.setupKeyExchange(true, 2); + expect(mockSetKey).toHaveBeenCalledWith("key-7"); + mockSetKey.mockClear(); + + // The abandoned rotation's setKey call now resolves. + releaseStale(); + await rotationPromise; + + // The shared key provider must end up on the LIVE session's key, not + // silently left on the abandoned one — narrow race, but real: nothing + // else re-applies the live key once the stale call lands. + expect(mockSetKey).toHaveBeenCalledWith("key-7"); + } finally { + vi.mocked(roomKeyToBase64).mockImplementation(() => "mock-room-key-base64"); + } + }); + + it("[OC-0005] paces room-key offers so a large channel's rotation stays under the server's per-second rate limit", async () => { + const ws = { send: vi.fn() }; + const mgr = createManager(ws); + mockVoiceState.voiceUsers.set(1, new Map([[1, {}]])); + await mgr.setupKeyExchange(true, 1); // holder + + // Seed 62 peers directly — well past the server's 64/sec cap for a + // single rotation's worth of offers. + for (let i = 0; i < 62; i++) { + mgr.peerPublicKeys.set(1000 + i, { type: `peer-${i}` } as unknown as CryptoKey); + } + ws.send.mockClear(); + + vi.useFakeTimers(); + try { + const rotationPromise = mgr.rotateKeyPeriodically(); + // Let every microtask-bound send that doesn't need a real timer run. + await vi.advanceTimersByTimeAsync(0); + const sentBeforePause = sendsOfType(ws, "voice_e2ee_offer").length; + // Must not have blown through the whole 62 in one burst. + expect(sentBeforePause).toBeLessThan(62); + expect(sentBeforePause).toBeGreaterThan(0); + + await vi.advanceTimersByTimeAsync(2000); + await rotationPromise; + + expect(sendsOfType(ws, "voice_e2ee_offer")).toHaveLength(62); + } finally { + vi.useRealTimers(); + } + }); + + 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); + // Non-key-holder session with an already-established room key from + // before the (simulated) disconnect. + await mgr.setupKeyExchange(true, 1); + await mgr.handleAnnounce(PEER_ID, "cGVlcg==", "sig"); // peer key known, so the offer below is accepted + await mgr.handleOffer(PEER_ID, "enc", "iv"); // stands us down — now a non-holder + ws.send.mockClear(); + + vi.useFakeTimers(); + try { + await mgr.reannounceForReconnect(); + // Nothing has confirmed the re-applied key is current yet. If the + // holder's fresh offer never arrives, this must not be a silent, + // unbounded wait for the next 5-minute rotation — it must retry. + await vi.advanceTimersByTimeAsync(6000); + + const announces = sendsOfType(ws, "voice_e2ee_announce"); + expect(announces.length).toBeGreaterThan(1); // the reconnect announce PLUS a retry + } finally { + vi.useRealTimers(); + } + }); }); diff --git a/Client/tauri-client/tests/unit/livekit-session.test.ts b/Client/tauri-client/tests/unit/livekit-session.test.ts index 540fff32..a0c2c9a8 100644 --- a/Client/tauri-client/tests/unit/livekit-session.test.ts +++ b/Client/tauri-client/tests/unit/livekit-session.test.ts @@ -21,6 +21,7 @@ const mockRoom = vi.hoisted(() => ({ disconnect: vi.fn().mockResolvedValue(undefined), on: vi.fn().mockReturnThis(), removeAllListeners: vi.fn(), + setE2EEEnabled: vi.fn().mockResolvedValue(undefined), localParticipant: { setMicrophoneEnabled: vi.fn().mockResolvedValue(undefined), setCameraEnabled: vi.fn().mockResolvedValue(undefined), @@ -45,6 +46,7 @@ vi.mock("livekit-client", () => ({ Disconnected: "disconnected", ActiveSpeakersChanged: "activeSpeakersChanged", AudioPlaybackStatusChanged: "audioPlaybackStatusChanged", + EncryptionError: "encryptionError", LocalTrackPublished: "localTrackPublished", }, Track: { @@ -108,6 +110,7 @@ vi.mock("@stores/voice.store", () => ({ setPeerVerification: vi.fn(), clearPeerVerification: vi.fn(), clearPeerVerifications: vi.fn(), + setEncryptionDegraded: vi.fn(), })); const mockInvoke = vi.hoisted(() => @@ -203,6 +206,7 @@ import { setVoiceStatus, setPeerVerification, clearPeerVerifications, + setEncryptionDegraded, } from "@stores/voice.store"; import { getIdentityPin, storeIdentityPin } from "@lib/identity"; import { verifyEphemeralKeySignature } from "@lib/e2eeCrypto"; @@ -984,6 +988,53 @@ describe("LiveKitSession", () => { expect(setVoiceStatus).toHaveBeenCalledWith("connected"); }); + + it("[OC-0020] does not carry a stale key-holder promotion into a channel switch made while reconnecting", async () => { + session.setServerHost("localhost:7880"); + session.setWsClient({ send: vi.fn() } as any); + + let disconnectedHandler: ((reason?: number) => void) | undefined; + mockRoom.on.mockImplementation((event: string, handler: any) => { + if (event === "disconnected") disconnectedHandler = handler; + return mockRoom; + }); + + // Join channel 1 as key holder. + await session.handleVoiceToken("test-token", "/livekit", 1, "ws://localhost:7880", true); + expect((session as any)._e2ee["_isKeyHolder"]).toBe(true); + + // The SFU connection drops — auto-reconnect starts, but the WS socket + // (and thus the sidebar) is unaffected, so the user can still switch + // voice channels while this is in flight. + disconnectedHandler!(/* SERVER_SHUTDOWN */ 1); + expect((session as any)._state.type).toBe("reconnecting"); + + // The user switches to channel 2, which already has a lower-uid + // participant — the server elects someone else and sends + // is_key_holder=false. + const joinPromise = session.handleVoiceToken( + "token-2", + "/livekit", + 2, + "ws://localhost:7880", + false, + ); + // Let the synchronous prefix of setupKeyExchange (keypair generation + + // announce signing — mocked async fns, no real delay) run without + // needing to fast-forward the non-holder wait-for-offer timers. + await vi.advanceTimersByTimeAsync(0); + + // The stale promotion from channel 1 must not leak into channel 2's + // election: connectAndSetup only tore down E2EE state via `_room !== + // null`, which reads null while "reconnecting", so clearState() never + // ran and the residual _isKeyHolder=true survived into this call. + expect((session as any)._e2ee["_isKeyHolder"]).toBe(false); + + // Let the (correctly non-holder) wait time out and the join settle so + // nothing is left dangling for later tests. + await vi.advanceTimersByTimeAsync(20_000); + await joinPromise; + }); }); describe("handleVoiceTokenRefresh", () => { @@ -1001,8 +1052,9 @@ describe("LiveKitSession", () => { session.handleVoiceTokenRefresh("new-token"); expect((session as any)._state.latestToken).toBe("new-token"); - // Timer restarted: the 23h refresh timer is re-armed on every refresh. - expect(setTimeoutSpy).toHaveBeenCalledWith(expect.any(Function), 23 * 60 * 60 * 1000); + // Timer restarted: the refresh timer is re-armed on every refresh. + // OC-0014: must stay under the server's 5-minute token TTL. + expect(setTimeoutSpy).toHaveBeenCalledWith(expect.any(Function), 4 * 60 * 1000); }); it("handles undefined token", () => { @@ -2425,6 +2477,48 @@ describe("LiveKitSession", () => { expect(worker.terminate).toHaveBeenCalled(); }); + + // OC-0095: room.setE2EEEnabled(true) was never called anywhere, so the + // key exchange (ECDH/HKDF/AES-GCM, TOFU identity, key-holder rotation) + // ran end-to-end but every media frame still reached the SFU in + // plaintext — the local cryptor stayed disabled. + it("enables E2EE on the room created by createRoom (OC-0095)", async () => { + mockRoom.setE2EEEnabled.mockClear(); + + await (session as any).createRoom(); + + expect(mockRoom.setE2EEEnabled).toHaveBeenCalledWith(true); + }); + + it("enables E2EE on the room used for a normal voice join", async () => { + mockRoom.setE2EEEnabled.mockClear(); + session.setServerHost("localhost:7880"); + mockRoom.connect.mockResolvedValue(undefined); + + await session.handleVoiceToken("token", "/livekit", 1, "ws://localhost:7880", true); + + expect(mockRoom.setE2EEEnabled).toHaveBeenCalledWith(true); + }); + + // OC-0002: a dead E2EE worker (CSP block, WASM load failure, WebView2 + // quirk) fails asynchronously, after keyProvider.setKey already resolved + // and voiceStatus already reached "connected" — livekit-client's own + // signal for this is RoomEvent.EncryptionError (E2eeManager.onWorkerError). + // Nothing subscribed to it, so the Secured badge had no way to ever know. + it("wires an EncryptionError listener onto the room so a dead worker cannot stay invisible (OC-0002)", async () => { + mockRoom.on.mockClear(); + (setEncryptionDegraded as ReturnType).mockClear(); + + await (session as any).createRoom(); + + const call = mockRoom.on.mock.calls.find((c: unknown[]) => c[0] === "encryptionError"); + expect(call).toBeDefined(); + + const handler = call![1] as (error: Error) => void; + handler(new Error("e2ee worker crashed")); + + expect(setEncryptionDegraded).toHaveBeenCalledWith(true); + }); }); describe("attemptAutoReconnect (lifecycle)", () => { @@ -2804,6 +2898,28 @@ describe("LiveKitSession", () => { }); describe("token refresh timer", () => { + // OC-0014: the server mints LiveKit tokens with a 5-minute TTL + // (Server/ws/livekit.go tokenTTL). If the client's only periodic + // refresh fires later than that, any reconnect attempt after the first + // 5 minutes of a call presents an already-expired token and fails. + it("refreshes the token before the server's 5-minute TTL expires (OC-0014)", async () => { + const mockWs = { send: vi.fn() } as any; + session.setWsClient(mockWs); + session.setServerHost("localhost:7880"); + + mockRoom.connect.mockResolvedValue(undefined); + await session.handleVoiceToken("token", "/livekit", 1, "ws://localhost:7880", true); + + mockWs.send.mockClear(); + + await vi.advanceTimersByTimeAsync(5 * 60 * 1000); + + expect(mockWs.send).toHaveBeenCalledWith({ + type: "voice_token_refresh", + payload: {}, + }); + }); + it("fires after TOKEN_REFRESH_MS and sends voice_token_refresh WS message", async () => { const mockWs = { send: vi.fn() } as any; session.setWsClient(mockWs); @@ -2814,7 +2930,7 @@ describe("LiveKitSession", () => { mockWs.send.mockClear(); - await vi.advanceTimersByTimeAsync(23 * 60 * 60 * 1000 + 100); + await vi.advanceTimersByTimeAsync(4 * 60 * 1000 + 100); expect(mockWs.send).toHaveBeenCalledWith({ type: "voice_token_refresh", @@ -2857,7 +2973,7 @@ describe("LiveKitSession", () => { mockWs.send.mockClear(); (session as any).clearTokenRefreshTimer(); - await vi.advanceTimersByTimeAsync(23 * 60 * 60 * 1000 + 100); + await vi.advanceTimersByTimeAsync(4 * 60 * 1000 + 100); expect(mockWs.send).not.toHaveBeenCalledWith( expect.objectContaining({ type: "voice_token_refresh" }), diff --git a/Client/tauri-client/tests/unit/room-event-handlers.test.ts b/Client/tauri-client/tests/unit/room-event-handlers.test.ts index a86c3259..eed128a7 100644 --- a/Client/tauri-client/tests/unit/room-event-handlers.test.ts +++ b/Client/tauri-client/tests/unit/room-event-handlers.test.ts @@ -142,7 +142,12 @@ function participant(identity: string): RemoteParticipant { } beforeEach(() => { - voiceStore.setState((prev) => ({ ...prev, localMuted: false, localDeafened: false })); + voiceStore.setState((prev) => ({ + ...prev, + localMuted: false, + localDeafened: false, + encryptionDegraded: false, + })); vi.stubGlobal( "MediaStream", class { @@ -510,6 +515,33 @@ describe("handleAudioPlaybackChanged", () => { }); }); +// ── handleEncryptionError (OC-0002) ──────────────────────────────────────── +// +// livekit-client's E2eeManager emits RoomEvent.EncryptionError when the +// per-room E2EE worker dies (onWorkerError) — the ECDH/HKDF key exchange can +// still succeed while the worker that actually encrypts frames is dead. With +// nothing subscribed to this event, that failure was invisible: voiceStatus +// still reaches "connected" and the widget's Secured badge lit up regardless. + +describe("handleEncryptionError", () => { + it("marks encryption degraded in the voice store so the Secured badge can react", () => { + const h = build(); + expect(voiceStore.getState().encryptionDegraded).toBe(false); + + h.handlers.handleEncryptionError(new Error("worker crashed")); + + expect(voiceStore.getState().encryptionDegraded).toBe(true); + }); + + it("marks encryption degraded even when no participant is attributed", () => { + const h = build(); + + h.handlers.handleEncryptionError(new Error("worker crashed"), undefined); + + expect(voiceStore.getState().encryptionDegraded).toBe(true); + }); +}); + // ── handleDisconnected ───────────────────────────────────────────────────── describe("handleDisconnected", () => { diff --git a/Client/tauri-client/tests/unit/voice-widget.test.ts b/Client/tauri-client/tests/unit/voice-widget.test.ts index a8e88018..9809cc77 100644 --- a/Client/tauri-client/tests/unit/voice-widget.test.ts +++ b/Client/tauri-client/tests/unit/voice-widget.test.ts @@ -723,6 +723,37 @@ describe("VoiceWidget", () => { widget.destroy?.(); }); + // OC-0002: the badge was derived purely from voiceStatus === "connected", + // never from the SDK's live encryption state — a dead E2EE worker that + // fails asynchronously (after the key exchange and voiceStatus already + // reached "connected") had no way to ever un-light the badge. + it("shows a not-secured warning instead of the Secured badge once encryption degrades", () => { + setVoiceChannel(1, []); + + const widget = createVoiceWidget({ + onDisconnect: vi.fn(), + onMuteToggle: vi.fn(), + onDeafenToggle: vi.fn(), + onCameraToggle: vi.fn(), + onScreenshareToggle: vi.fn(), + }); + widget.mount(container); + + setVoiceStatus("connected"); + const secured = container.querySelector('[data-testid="vw-secured"]') as HTMLElement; + expect(secured.textContent).toContain("Secured"); + + voiceStore.setState((prev) => ({ ...prev, encryptionDegraded: true })); + voiceStore.flush(); + + // Still shown (so the warning is not silently missed), but no longer + // claiming "Secured" — a dead worker must never look like a live one. + expect(secured.style.display).toBe("inline-flex"); + expect(secured.textContent).not.toContain("Secured"); + + widget.destroy?.(); + }); + it("shows 'Reconnecting voice…' during a voice reconnect", () => { setVoiceChannel(1, []); diff --git a/Server/admin/handlers_backup.go b/Server/admin/handlers_backup.go index ab0f7780..051be108 100644 --- a/Server/admin/handlers_backup.go +++ b/Server/admin/handlers_backup.go @@ -41,6 +41,23 @@ func init() { } } +// dbFilePath is the live SQLite database file that "Restore backup" +// overwrites. It defaults to the historical "data/chatserver.db" but must be +// pointed at cfg.Database.Path via SetDatabasePath before the server starts +// serving requests (main.go, right after db.Open): without that call, a +// server configured with a non-default database.path would open its real +// database at cfg.Database.Path while restore keeps copying backups over an +// unrelated (possibly newly created) file at the default path, reporting +// success while the live database is never touched. +var dbFilePath = filepath.Join("data", "chatserver.db") + +// SetDatabasePath points the restore handler at the SQLite file the server +// actually opened. Call once at startup with cfg.Database.Path; tests use it +// to point restore at an isolated temp file. +func SetDatabasePath(path string) { + dbFilePath = path +} + // ─── Backup Handlers ───────────────────────────────────────────────────────── func handleBackup(database *db.DB) http.Handler { @@ -174,7 +191,7 @@ func handleRestoreBackup(database *db.DB, hub HubBroadcaster) http.Handler { return } - dbPath := filepath.Join("data", "chatserver.db") + dbPath := dbFilePath actor := actorFromContext(r) // Audit the restore BEFORE the pre-restore safety copy is taken, and diff --git a/Server/admin/handlers_backup_test.go b/Server/admin/handlers_backup_test.go index 1afb53ee..35296354 100644 --- a/Server/admin/handlers_backup_test.go +++ b/Server/admin/handlers_backup_test.go @@ -1,6 +1,7 @@ package admin_test import ( + "bytes" "context" "encoding/json" "net/http" @@ -474,6 +475,71 @@ func TestHandleRestoreBackup_AbortsWithoutSafetyBackup(t *testing.T) { } } +// TestHandleRestoreBackup_UsesConfiguredDatabasePath verifies that the +// restore handler writes to the SQLite file the server was actually +// configured to use (SetDatabasePath), not a hardcoded "data/chatserver.db". +// A server with database.path set to anything else must not have its real +// database silently left untouched by a "successful" restore (OC-0097). +func TestHandleRestoreBackup_UsesConfiguredDatabasePath(t *testing.T) { + tmpDir := chdirTemp(t) + database := openAdminTestDB(t) + handler := admin.NewAdminAPI(database, "1.0.0", &mockHub{}, nil, nil, nil, nil, newTestModService(database), newTestRoleService(database)) + token := createAdminUser(t, database) + + backupDir := filepath.Join(tmpDir, "data", "backups") + if err := os.MkdirAll(backupDir, 0o750); err != nil { + t.Fatalf("MkdirAll backups: %v", err) + } + + // Configure a non-default database path, as an operator would via + // database.path in config.yaml. + customDBPath := filepath.Join(tmpDir, "custom", "oc.db") + if err := os.MkdirAll(filepath.Dir(customDBPath), 0o750); err != nil { + t.Fatalf("MkdirAll custom db dir: %v", err) + } + if err := os.WriteFile(customDBPath, []byte("original live contents"), 0o644); err != nil { + t.Fatalf("WriteFile custom db: %v", err) + } + admin.SetDatabasePath(customDBPath) + t.Cleanup(func() { admin.SetDatabasePath(filepath.Join("data", "chatserver.db")) }) + + backupName := "chatserver_20240101_120000.db" + backupContent := []byte("restored contents") + if err := os.WriteFile(filepath.Join(backupDir, backupName), backupContent, 0o644); err != nil { + t.Fatalf("WriteFile backup: %v", err) + } + + restarted, restoreHook := admin.StubRestart() + defer restoreHook() + + w := doRequest(t, handler, http.MethodPost, "/backups/"+backupName+"/restore", token, nil) + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body: %s", w.Code, w.Body.String()) + } + + deadline := time.Now().Add(2 * time.Second) + for !restarted() && time.Now().Before(deadline) { + time.Sleep(10 * time.Millisecond) + } + if !restarted() { + t.Error("restore did not request a process restart") + } + + got, err := os.ReadFile(customDBPath) + if err != nil { + t.Fatalf("ReadFile(%q): %v", customDBPath, err) + } + if !bytes.Equal(got, backupContent) { + t.Errorf("configured database file content = %q, want %q — restore wrote to the wrong path", got, backupContent) + } + + // The hardcoded default path must NOT have been created/touched. + defaultPath := filepath.Join(tmpDir, "data", "chatserver.db") + if _, err := os.Stat(defaultPath); err == nil { + t.Error("restore wrote to the hardcoded default database path instead of the configured one") + } +} + // TestHandleRestoreBackup_NotFound verifies that restoring a missing backup // returns 404. func TestHandleRestoreBackup_NotFound(t *testing.T) { diff --git a/Server/admin/handlers_users.go b/Server/admin/handlers_users.go index 4584f974..32fad417 100644 --- a/Server/admin/handlers_users.go +++ b/Server/admin/handlers_users.go @@ -182,25 +182,31 @@ func handlePatchUser(database *db.DB, hub HubBroadcaster, permInvalidator Permis writeErr(w, http.StatusInternalServerError, "INTERNAL_ERROR", "moderation service unavailable") return } - if err := mod.ChangeUserRole(r.Context(), actor, id, *req.RoleID); err != nil { + newRole, err := mod.ChangeUserRole(r.Context(), actor, id, *req.RoleID) + if err != nil { writeModerationErr(w, err) return } if permInvalidator != nil { permInvalidator.InvalidateUser(id) } - if role, err := database.GetRoleByID(r.Context(), *req.RoleID); err == nil && role != nil { - if hub != nil { - hub.BroadcastMemberUpdate(id, role.Name) - // BroadcastMemberUpdate only revokes subscriptions the new - // role can no longer read (hub_broadcast.go's - // revokeUnreadableChannels); it never grants the ones the - // new role newly gained READ_MESSAGES on. Without this, - // a promoted user's sidebar is missing channels until - // their next reconnect, unlike a role permission edit or - // a role delete, which both re-derive visibility fully. - hub.RefreshAllChannelVisibility() - } + // Use the role ChangeUserRole already loaded and validated rather + // than re-reading it: a re-read can race a concurrent role delete + // (or a transient read error) and silently skip this whole + // fan-out, leaving the demoted user's socket subscribed to + // channels it can no longer read (OC-0045). The role change + // itself already committed, so the fan-out must not be + // conditional on anything past that point. + if hub != nil { + hub.BroadcastMemberUpdate(id, newRole.Name) + // BroadcastMemberUpdate only revokes subscriptions the new + // role can no longer read (hub_broadcast.go's + // revokeUnreadableChannels); it never grants the ones the + // new role newly gained READ_MESSAGES on. Without this, + // a promoted user's sidebar is missing channels until + // their next reconnect, unlike a role permission edit or + // a role delete, which both re-derive visibility fully. + hub.RefreshAllChannelVisibility() } } diff --git a/Server/admin/handlers_users_broadcast_test.go b/Server/admin/handlers_users_broadcast_test.go index c60c18e7..5a57974b 100644 --- a/Server/admin/handlers_users_broadcast_test.go +++ b/Server/admin/handlers_users_broadcast_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/owncord/server/admin" + "github.com/owncord/server/db" ) // unbanMockHub wraps mockHub (admin/api_test.go) and additionally implements @@ -87,3 +88,65 @@ func TestAdminAPI_PatchUser_RoleChangeRefreshesVisibility(t *testing.T) { t.Fatalf("expected a member_update for the role change, got %+v", hub.memberUpdates) } } + +// roleDeletingInvalidator simulates a second admin deleting the just-assigned +// role in the window between ModerationService.ChangeUserRole committing and +// handlePatchUser's own re-read of that role (used only for its name in the +// member_update payload). It hooks PermissionInvalidator.InvalidateUser +// because handlePatchUser calls that exactly once, synchronously, right +// after ChangeUserRole succeeds and right before the vulnerable re-read. +type roleDeletingInvalidator struct { + database *db.DB + deleteRoleID, fallbackRoleID int64 +} + +func (r *roleDeletingInvalidator) InvalidateUser(int64) { + if _, err := r.database.DeleteRoleReassigning(context.Background(), r.deleteRoleID, r.fallbackRoleID); err != nil { + panic(err) // test setup bug, not the behavior under test + } +} +func (r *roleDeletingInvalidator) InvalidateAll() {} + +// A role demotion's live-subscription revocation (BroadcastMemberUpdate -> +// revokeUnreadableChannels) and visibility re-derivation +// (RefreshAllChannelVisibility) must run whenever ChangeUserRole actually +// committed the role change, not only when a second, purely-cosmetic re-read +// of the role (done only to get its name) happens to still succeed. If the +// role is deleted out from under that re-read — a real admin racing a role +// deletion against this handler, or a transient read error — the demoted +// user's socket must not be left subscribed to channels it can no longer +// read (OC-0045). +func TestAdminAPI_PatchUser_RoleChangeBroadcastsEvenIfRoleReReadFails(t *testing.T) { + database := openAdminTestDB(t) + hub := &mockHub{} + invalidator := &roleDeletingInvalidator{database: database, deleteRoleID: 2, fallbackRoleID: 3} + handler := admin.NewAdminAPI(database, "1.0.0", hub, nil, nil, nil, invalidator, newTestModService(database), newTestRoleService(database)) + token := createAdminUser(t, database) + + targetUID, _ := database.CreateUser(context.Background(), "roleracetarget", "hash", 3) + + // Promote the target to role 2 ("Admin"). The handler's own + // InvalidateUser hook fires mid-request and deletes role 2 (reassigning + // the target back to role 3 first, exactly like a real admin's DELETE + // /admin/api/roles/2 would), so by the time handlePatchUser re-reads + // role 2 for its name, GetRoleByID returns (nil, nil). + w := doRequest(t, handler, http.MethodPatch, "/users/"+itoa(targetUID), token, map[string]any{ + "role_id": 2, + }) + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body: %s", w.Code, w.Body.String()) + } + + if hub.allVisibilityRefreshes != 1 { + t.Fatalf("RefreshAllChannelVisibility calls = %d, want 1 (role change committed regardless of the re-read)", hub.allVisibilityRefreshes) + } + found := false + for _, mu := range hub.memberUpdates { + if mu.userID == targetUID { + found = true + } + } + if !found { + t.Fatalf("expected a member_update for the role change despite the concurrent role deletion, got %+v", hub.memberUpdates) + } +} diff --git a/Server/api/auth_handler.go b/Server/api/auth_handler.go index f922bf47..106e608e 100644 --- a/Server/api/auth_handler.go +++ b/Server/api/auth_handler.go @@ -9,17 +9,25 @@ import ( "net/http" "strings" "time" + "unicode/utf8" "github.com/go-chi/chi/v5" "github.com/microcosm-cc/bluemonday" "github.com/owncord/server/auth" "github.com/owncord/server/db" "github.com/owncord/server/permissions" + "github.com/owncord/server/service" ) // sanitizer strips all HTML from user-supplied strings before storage. var sanitizer = bluemonday.StrictPolicy() +// maxLoginUsernameLen bounds the username accepted by handleLogin, mirroring +// auth.ValidateUsername's 32-rune cap on registered usernames. Enforced +// before the value is ever used to build a RateLimiter map key — see the +// check in handleLogin for why. +const maxLoginUsernameLen = 32 + // genericAuthError is returned for all login/register failures to avoid // revealing whether a username exists. var genericAuthError = errorResponse{ @@ -177,7 +185,13 @@ func handleRegister(database *db.DB) http.HandlerFunc { return } - req.Username = strings.TrimSpace(sanitizer.Sanitize(req.Username)) + // F: use the fixpoint sanitizer (service.SanitizeText), not the bare + // sanitizer.Sanitize below — Sanitize's output is always HTML-escaped + // (' -> ', & -> &, " -> "), so a plain call here would store + // a different string than what handleLogin looks up (which only + // trims), permanently locking out any username containing one of + // those characters. See service.SanitizeText's doc comment. + req.Username = strings.TrimSpace(service.SanitizeText(req.Username)) req.InviteCode = strings.TrimSpace(req.InviteCode) if req.Username == "" || req.Password == "" || req.InviteCode == "" { @@ -304,6 +318,21 @@ func handleLogin(database *db.DB, limiter *auth.RateLimiter, partialStore *auth. return } + // F: reject an over-long username before it is ever used to build a + // RateLimiter map key below (unameKey, failKey, userFailKey, lockout + // keys). Unlike registration, login has no account to validate + // against yet, so nothing else bounds this value — an unauthenticated + // caller could otherwise pin an arbitrarily large, body-sized string + // as a retained key (Cleanup only evicts it after hours). Mirrors the + // same 32-rune cap auth.ValidateUsername enforces at registration. + if utf8.RuneCountInString(req.Username) > maxLoginUsernameLen { + writeJSON(w, http.StatusBadRequest, errorResponse{ + Error: "INVALID_INPUT", + Message: "username is too long", + }) + return + } + ip := clientIPWithProxies(r, proxyNets) // Check per-IP lockout first. diff --git a/Server/api/auth_handler_test.go b/Server/api/auth_handler_test.go index 4a093bc3..21c40bbb 100644 --- a/Server/api/auth_handler_test.go +++ b/Server/api/auth_handler_test.go @@ -8,6 +8,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "strings" "sync" "testing" "testing/fstest" @@ -1107,6 +1108,113 @@ func TestLogin_UsernameIsStillTrimmed(t *testing.T) { } } +// TestRegister_UsernameNotHTMLEscaped pins OC-0099: handleRegister must not +// persist an HTML-escaped username. A bare bluemonday sanitizer.Sanitize call +// HTML-escapes survivors (' -> ', & -> &, " -> "), so a name like +// "O'Brien" would be stored as "O'Brien" — different from what the user +// typed and from what handleLogin looks up (which only trims). +func TestRegister_UsernameNotHTMLEscaped(t *testing.T) { + database := newAuthTestDB(t) + limiter := auth.NewRateLimiter() + router := buildAuthRouter(database, limiter) + + ownerID, _ := database.CreateUser(context.Background(), "owner2", "hash", 1) + code, _ := database.CreateInvite(context.Background(), ownerID, 1, nil) + + rr := postJSON(t, router, "/api/v1/auth/register", map[string]string{ + "username": "O'Brien", + "password": "securePass1", + "invite_code": code, + }) + 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) + user, _ := resp["user"].(map[string]any) + if got, want := user["username"], "O'Brien"; got != want { + t.Errorf("registered username = %q, want %q (must not be HTML-escaped)", got, want) + } + + stored, err := database.GetUserByUsername(context.Background(), "O'Brien") + if err != nil || stored == nil { + t.Fatalf("GetUserByUsername(%q) = (%v, %v), want a match", "O'Brien", stored, err) + } +} + +// TestLogin_UsernameWithApostropheSucceeds pins OC-0099 end-to-end: a user +// who registers with an apostrophe/quote/ampersand in their name must be able +// to log back in with the exact same name. Before the fix, handleRegister +// stored the HTML-escaped form while handleLogin looked up the raw form, so +// this login permanently 401s for any such account. +func TestLogin_UsernameWithApostropheSucceeds(t *testing.T) { + database := newAuthTestDB(t) + limiter := auth.NewRateLimiter() + router := buildAuthRouter(database, limiter) + + ownerID, _ := database.CreateUser(context.Background(), "owner3", "hash", 1) + code, _ := database.CreateInvite(context.Background(), ownerID, 1, nil) + + regRR := postJSON(t, router, "/api/v1/auth/register", map[string]string{ + "username": "O'Brien", + "password": "securePass1", + "invite_code": code, + }) + if regRR.Code != http.StatusCreated { + t.Fatalf("Register status = %d, want 201; body = %s", regRR.Code, regRR.Body.String()) + } + + loginRR := postJSON(t, router, "/api/v1/auth/login", map[string]string{ + "username": "O'Brien", + "password": "securePass1", + }) + if loginRR.Code != http.StatusOK { + t.Fatalf("Login with registered username status = %d, want 200; body = %s", loginRR.Code, loginRR.Body.String()) + } + var resp map[string]any + _ = json.NewDecoder(loginRR.Body).Decode(&resp) + if resp["token"] == nil { + t.Error("Login response missing token") + } +} + +// TestLogin_OversizedUsernameRejectedBeforeRateLimiterKey pins OC-0021: +// handleLogin never length-checks req.Username before using it to build +// RateLimiter map keys ("login_user_fail:"+username, "login_user_lock:"+...). +// An unauthenticated caller could otherwise pin an arbitrarily large key in +// the limiter's maps (retained for hours by Cleanup's window) on every +// attempt. The oversized username must be rejected with 400 before any such +// key is ever recorded, mirroring the same 32-rune cap register enforces via +// auth.ValidateUsername before touching anything. +func TestLogin_OversizedUsernameRejectedBeforeRateLimiterKey(t *testing.T) { + database := newAuthTestDB(t) + limiter := auth.NewRateLimiter() + router := buildAuthRouter(database, limiter) + + hugeUsername := strings.Repeat("a", 1<<20) // 1 MiB, as in the repro + + rr := postJSON(t, router, "/api/v1/auth/login", map[string]string{ + "username": hugeUsername, + "password": "anypass123", + }) + + if rr.Code != http.StatusBadRequest { + t.Errorf("Login oversized username status = %d, want 400; body = %s", rr.Code, rr.Body.String()) + } + + // The route's own per-IP RateLimitMiddleware ("login:"+ip) legitimately + // records one small, IP-bounded window entry regardless of this fix. + // What must NOT happen is handleLogin additionally recording a second + // entry keyed on the 1 MiB username itself (failKey/userFailKey) — that + // unbounded entry is the actual leak, so anything beyond the single + // expected IP entry means the oversized username reached key-building + // code before being rejected. + if windows, lockouts := limiter.Len(); windows > 1 || lockouts != 0 { + t.Errorf("RateLimiter retained state after oversized-username login: windows=%d lockouts=%d, want at most 1/0", windows, lockouts) + } +} + // ─── Rate limiting integration test ────────────────────────────────────────── func TestRegister_RateLimit(t *testing.T) { diff --git a/Server/api/gif_handler.go b/Server/api/gif_handler.go index 36ae4766..e77ed5fa 100644 --- a/Server/api/gif_handler.go +++ b/Server/api/gif_handler.go @@ -207,12 +207,17 @@ func fetchGIFs(r *http.Request, upstreamURL, apiKey string, limit int) ([]gifRes // errGIFUpstream marks a non-200 upstream response. var errGIFUpstream = errors.New("gif proxy: upstream error") -// redactKey removes the API key from a string destined for the logs. +// redactKey removes the API key from a string destined for the logs. It +// matches both the literal key and its percent-encoded query-string form +// (url.Error embeds the encoded request URL, and params.Encode() escapes any +// character outside [A-Za-z0-9-_.~] — common in base64-style keys) so an +// encoded form is caught too. func redactKey(s, apiKey string) string { if apiKey == "" { return s } - return strings.ReplaceAll(s, apiKey, "[REDACTED]") + s = strings.ReplaceAll(s, apiKey, "[REDACTED]") + return strings.ReplaceAll(s, url.QueryEscape(apiKey), "[REDACTED]") } // parseGIFLimit parses and validates the `limit` query param. An empty value diff --git a/Server/api/gif_handler_internal_test.go b/Server/api/gif_handler_internal_test.go new file mode 100644 index 00000000..6a6e141b --- /dev/null +++ b/Server/api/gif_handler_internal_test.go @@ -0,0 +1,30 @@ +package api + +import ( + "net/url" + "strings" + "testing" +) + +// OC-0109: url.Error embeds the upstream request URL, which is built with +// params.Encode() — every character outside [A-Za-z0-9-_.~] is percent +// escaped. redactKey compared only against the literal (decoded) key, so a +// key containing such a character (common in base64-style keys: '+', '/', +// '=') survived into the log line in its encoded form. +func TestRedactKeyMatchesPercentEncodedForm(t *testing.T) { + apiKey := "ab+cd/ef=" + encoded := url.QueryEscape(apiKey) + msg := `Get "https://api.klipy.com/v2/featured?key=` + encoded + `&limit=20": dial tcp: lookup api.klipy.com: no such host` + + got := redactKey(msg, apiKey) + + if strings.Contains(got, encoded) { + t.Fatalf("redactKey left the percent-encoded API key in the message: %q", got) + } + if strings.Contains(got, apiKey) { + t.Fatalf("redactKey left the literal API key in the message: %q", got) + } + if !strings.Contains(got, "[REDACTED]") { + t.Fatalf("redactKey did not redact anything: %q", got) + } +} diff --git a/Server/api/profile_handler.go b/Server/api/profile_handler.go index 7d93417e..ede247d0 100644 --- a/Server/api/profile_handler.go +++ b/Server/api/profile_handler.go @@ -177,7 +177,14 @@ func handleUpdateProfile(svc *service.Services, broadcaster ProfileBroadcaster) return } - req.Username = strings.TrimSpace(sanitizer.Sanitize(req.Username)) + // Use the fixpoint sanitizer (service.SanitizeText), not the bare + // sanitizer.Sanitize below — Sanitize's output is always + // HTML-escaped, so a plain apostrophe would be persisted as ' + // and login (which never re-escapes) would look the account up + // under a name that no longer matches. See service.SanitizeText's + // doc comment and the register path (auth_handler.go), which + // already canonicalizes the same way. + req.Username = strings.TrimSpace(service.SanitizeText(req.Username)) if req.Username == "" { writeJSON(w, http.StatusBadRequest, errorResponse{ Error: "INVALID_INPUT", Message: "username is required", @@ -571,9 +578,16 @@ func handleUploadAvatar( } avatarURL := service.AvatarFileURL(fileID) + // Username is deliberately omitted (left at its zero value): user + // here is a snapshot AuthMiddleware read at the start of the + // request, before the multipart parse / image decode / disk write + // above — all of which take long enough for a concurrent + // PATCH /users/me rename to land first. Sending that stale value + // would revert the rename; UpdateProfile treats an empty Username + // as "leave it alone", the same contract DisplayName/About already + // have via nil. updated, err := svc.Users.UpdateProfile(r.Context(), user.ID, service.ProfilePatch{ - Username: user.Username, - Avatar: &avatarURL, + Avatar: &avatarURL, }) if err != nil { // The column never moved, so the file and its row are orphans. diff --git a/Server/api/profile_handler_test.go b/Server/api/profile_handler_test.go index 8cf75e06..a7159678 100644 --- a/Server/api/profile_handler_test.go +++ b/Server/api/profile_handler_test.go @@ -133,6 +133,37 @@ func TestUpdateProfile_EmptyUsername(t *testing.T) { } } +// OC-0100: a rename must canonicalize identically to how login looks the +// username up. A bare bluemonday sanitizer.Sanitize call HTML-escapes +// survivor punctuation instead of leaving it as typed, so a name with an +// apostrophe would be persisted as e.g. "O'Brien" — unreachable by the +// literal name on the next login. +func TestUpdateProfile_UsernameWithApostropheIsNotEscaped(t *testing.T) { + database := newAuthTestDB(t) + router := buildProfileRouter(database) + token := profileCreateToken(t, database, "apostropheuser", 4) + + rr := patchJSON(t, router, "/api/v1/users/me", token, map[string]string{ + "username": "O'Brien", + }) + if rr.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body = %s", rr.Code, rr.Body.String()) + } + + var resp map[string]any + _ = json.NewDecoder(rr.Body).Decode(&resp) + if resp["username"] != "O'Brien" { + t.Errorf("username = %v, want %q (must not be HTML-escaped)", resp["username"], "O'Brien") + } + + // The row itself must be reachable by the literal name — that is exactly + // what a future login looks up. + u, err := database.GetUserByUsername(context.Background(), "O'Brien") + if err != nil || u == nil { + t.Fatalf("GetUserByUsername(%q) = %v, %v — rename escaped the username and would lock the account out on next login", "O'Brien", u, err) + } +} + func TestUpdateProfile_UsernameTaken(t *testing.T) { database := newAuthTestDB(t) router := buildProfileRouter(database) diff --git a/Server/api/router.go b/Server/api/router.go index 614cd166..a9b86edc 100644 --- a/Server/api/router.go +++ b/Server/api/router.go @@ -100,11 +100,8 @@ func NewRouter(cfg *config.Config, database *db.DB, ver string, logBuf *admin.Ri // removed in D3). svc := service.New(database, limiter) - // Auth routes: register, login, logout, me. - MountAuthRoutes(r, database, limiter, cfg.Server.TrustedProxies, totpKey) - - // Profile routes are mounted after hub creation (below) so the hub can - // broadcast user_update events for real-time profile changes. + // Auth routes are mounted after hub creation (below) so self-service + // account deletion can broadcast member_ban like the admin ban path does. // Invite management routes (require MANAGE_INVITES permission). MountInviteRoutes(r, database, svc) @@ -141,6 +138,13 @@ func NewRouter(cfg *config.Config, database *db.DB, ver string, logBuf *admin.Ri hub := ws.NewHub(database, limiter, svc) getOnlineUsers = func() int { return hub.ClientCount() } + // Auth routes: register, login, logout, me. Mounted with the hub as the + // AuthBroadcaster so DELETE /api/v1/auth/account (self-service account + // deletion) fans out member_ban and force-disconnects the deleted user's + // own socket, exactly like the admin ban path does for the same + // anonymise-and-ban DB state. + MountAuthRoutes(r, database, limiter, cfg.Server.TrustedProxies, totpKey, hub) + // Phase C Step 9 — wire plugin registry and event sink into the hub. // nil pluginRegistry means plugins are disabled; the hub no-ops cleanly. if pluginRegistry != nil { diff --git a/Server/api/router_delete_account_broadcast_test.go b/Server/api/router_delete_account_broadcast_test.go new file mode 100644 index 00000000..93f76c46 --- /dev/null +++ b/Server/api/router_delete_account_broadcast_test.go @@ -0,0 +1,175 @@ +package api_test + +// router_delete_account_broadcast_test.go pins the production wiring for +// OC-0048: NewRouter (router.go) must pass the WS hub to MountAuthRoutes as +// its optional AuthBroadcaster so self-service account deletion fans out +// member_ban exactly like the admin ban path does. MountAuthRoutes is called +// before the hub exists in router.go, so the only production call site used +// to omit the broadcaster entirely — handleDeleteAccount's +// `if broadcaster != nil` guard was never taken outside tests that construct +// their own fake broadcaster (see auth_handler_delete_broadcast_test.go, +// which only proves the handler itself works when a broadcaster IS passed). +// This test drives the real api.NewRouter wiring end to end, including a live +// WebSocket connection, so it fails if the router ever again forgets to wire +// the hub through. +// +// The broadcast is observed on a SECOND user's connection, not the deleted +// user's own: BroadcastMemberBan enqueues the broadcast and then force- +// disconnects the target, and on a slow runner the disconnect can close the +// target's socket before its copy of the frame is flushed. The observer +// socket has no such race — and it is the party the event exists for (every +// other client must drop the deleted member). + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/coder/websocket" + + "github.com/owncord/server/api" + "github.com/owncord/server/auth" + "github.com/owncord/server/config" + "github.com/owncord/server/db" +) + +// dialAndAuthWS opens a WS connection against srv and completes the auth +// handshake for token, failing the test on any step. +func dialAndAuthWS(t *testing.T, srv *httptest.Server, token string) *websocket.Conn { + t.Helper() + + dialCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + wsURL := "ws" + strings.TrimPrefix(srv.URL, "http") + "/api/v1/ws" + conn, dialResp, dialErr := websocket.Dial(dialCtx, wsURL, nil) + if dialResp != nil && dialResp.Body != nil { + defer dialResp.Body.Close() //nolint:errcheck // test cleanup + } + if dialErr != nil { + t.Fatalf("websocket.Dial: %v", dialErr) + } + t.Cleanup(func() { _ = conn.Close(websocket.StatusNormalClosure, "") }) + + authMsg := map[string]any{ + "type": "auth", + "payload": map[string]any{"token": token}, + } + raw, _ := json.Marshal(authMsg) + if err := conn.Write(dialCtx, websocket.MessageText, raw); err != nil { + t.Fatalf("write auth: %v", err) + } + + _, authOKMsg, err := conn.Read(dialCtx) + if err != nil { + t.Fatalf("read auth_ok: %v", err) + } + var authOK map[string]any + if err := json.Unmarshal(authOKMsg, &authOK); err != nil { + t.Fatalf("unmarshal auth_ok: %v; raw=%s", err, authOKMsg) + } + if authOK["type"] != "auth_ok" { + t.Fatalf("expected auth_ok, got %v; raw=%s", authOK["type"], authOKMsg) + } + return conn +} + +func TestNewRouter_DeleteAccount_BroadcastsMemberBanOverWS(t *testing.T) { + database, err := db.Open(":memory:") + if err != nil { + t.Fatalf("db.Open: %v", err) + } + t.Cleanup(func() { _ = database.Close() }) + if err := db.Migrate(database); err != nil { + t.Fatalf("db.Migrate: %v", err) + } + + cfg := &config.Config{ + Server: config.ServerConfig{ + Name: "Test Server", + Port: 8443, + DataDir: t.TempDir(), + AllowedOrigins: []string{"*"}, + }, + } + + handler, _, cleanup := api.NewRouter(cfg, database, "test", nil, nil) + t.Cleanup(cleanup) + + newUserSession := func(username string) (int64, string) { + hash, _ := auth.HashPassword("correctPass1") + // role_id=4 ("Member") so the last-admin check doesn't block deletion. + uid, err := database.CreateUser(context.Background(), username, hash, 4) + if err != nil { + t.Fatalf("CreateUser(%s): %v", username, err) + } + token, err := auth.GenerateToken() + if err != nil { + t.Fatalf("GenerateToken: %v", err) + } + if _, err := database.CreateSession(context.Background(), uid, auth.HashToken(token), "test", "127.0.0.1"); err != nil { + t.Fatalf("CreateSession: %v", err) + } + return uid, token + } + + doomedUID, doomedToken := newUserSession("routerdeletebroadcast") + _, observerToken := newUserSession("routerdeletebroadcastobs") + + srv := httptest.NewServer(handler) + t.Cleanup(srv.Close) + + // The doomed user connects so the hub also has a socket to force-close; + // the observer connects to witness the fan-out. + _ = dialAndAuthWS(t, srv, doomedToken) + observer := dialAndAuthWS(t, srv, observerToken) + + // Self-delete over the real HTTP handler, same as the client would. + req := httptest.NewRequest(http.MethodDelete, "/api/v1/auth/account", + strings.NewReader(`{"password":"correctPass1"}`)) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer "+doomedToken) + req.RemoteAddr = "127.0.0.1:9999" + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + if rr.Code != http.StatusNoContent { + t.Fatalf("DELETE /api/v1/auth/account status = %d, want 204; body = %s", rr.Code, rr.Body.String()) + } + + // The hub must fan out member_ban for the deleted user to the observer's + // still-open connection. Loop with an overall deadline: other broadcasts + // (presence, member updates) may arrive first. + deadline := time.Now().Add(10 * time.Second) + sawMemberBan := false + for time.Now().Before(deadline) { + readCtx, readCancel := context.WithTimeout(context.Background(), 2*time.Second) + _, msg, readErr := observer.Read(readCtx) + readCancel() + if readErr != nil { + break + } + var frame struct { + Type string `json:"type"` + Payload struct { + UserID int64 `json:"user_id"` + } `json:"payload"` + } + if err := json.Unmarshal(msg, &frame); err != nil { + continue + } + if frame.Type == "member_ban" && frame.Payload.UserID == doomedUID { + sawMemberBan = true + break + } + } + + if !sawMemberBan { + t.Fatal("no member_ban WS broadcast for the deleted user observed on a second client — " + + "router.go's MountAuthRoutes call must pass the hub as the optional " + + "AuthBroadcaster (mount it after ws.NewHub, not before)") + } +} diff --git a/Server/api/upload_handler.go b/Server/api/upload_handler.go index 64b25506..bddc41be 100644 --- a/Server/api/upload_handler.go +++ b/Server/api/upload_handler.go @@ -299,6 +299,29 @@ func handleServeFile(database *db.DB, store *storage.Storage, allowedOrigins []s // ── Access control ────────────────────────────────────────────── isAdmin := role != nil && permissions.HasAdmin(role.Permissions) + // DM participation is required of everyone, including admins — this + // matches every other DM read gate in the codebase (requireChannelRead, + // PermissionService.RequireChannelAccess, checkSendPermission), none of + // which have an admin bypass. Checked ahead of the `!isAdmin` block so + // the admin bypass below cannot skip it. + if aa.ChannelID != nil && aa.ChannelType == "dm" { + if user == nil { + writeJSON(w, http.StatusForbidden, errorResponse{ + Error: "FORBIDDEN", + Message: "you do not have access to this file", + }) + return + } + ok, dmErr := database.IsDMParticipant(r.Context(), user.ID, *aa.ChannelID) + if dmErr != nil || !ok { + writeJSON(w, http.StatusForbidden, errorResponse{ + Error: "FORBIDDEN", + Message: "you do not have access to this file", + }) + return + } + } + if !isAdmin { if aa.ChannelID == nil { // An unlinked attachment that some user's avatar points at is @@ -330,25 +353,10 @@ func handleServeFile(database *db.DB, store *storage.Storage, allowedOrigins []s }) return } - } else { - // Linked attachment — check channel permissions. - if aa.ChannelType == "dm" { - if user == nil { - writeJSON(w, http.StatusForbidden, errorResponse{ - Error: "FORBIDDEN", - Message: "you do not have access to this file", - }) - return - } - ok, dmErr := database.IsDMParticipant(r.Context(), user.ID, *aa.ChannelID) - if dmErr != nil || !ok { - writeJSON(w, http.StatusForbidden, errorResponse{ - Error: "FORBIDDEN", - Message: "you do not have access to this file", - }) - return - } - } else if user == nil || !permSvc.HasChannelPerm(r.Context(), user.ID, *aa.ChannelID, permissions.ReadMessages) { + } else if aa.ChannelType != "dm" { + // Linked attachment in a guild channel — check channel + // permissions. The DM case is handled unconditionally above. + if user == nil || !permSvc.HasChannelPerm(r.Context(), user.ID, *aa.ChannelID, permissions.ReadMessages) { writeJSON(w, http.StatusForbidden, errorResponse{ Error: "FORBIDDEN", Message: "you do not have access to this file", diff --git a/Server/api/upload_handler_test.go b/Server/api/upload_handler_test.go index 200a37c5..7ee182ee 100644 --- a/Server/api/upload_handler_test.go +++ b/Server/api/upload_handler_test.go @@ -1397,3 +1397,46 @@ func TestServeFile_LinkedToDM_NonParticipantForbidden(t *testing.T) { t.Errorf("status = %d, want 403 for DM non-participant", rr2.Code) } } + +// OC-0112: the admin bypass in handleServeFile must not cover the DM +// participant check. Every sibling DM read gate (requireChannelRead, +// PermissionService.RequireChannelAccess, checkSendPermission) denies a +// non-participant Administrator just like anyone else — the file route must +// match, not open every private DM to anyone holding the admin bit. +func TestServeFile_LinkedToDM_AdminNonParticipantForbidden(t *testing.T) { + database := newUploadTestDB(t) + store := newUploadTestStorage(t) + router := buildUploadRouter(database, store, nil) + token1 := uploadCreateToken(t, database, "dmadminowner", 4) + _ = uploadCreateToken(t, database, "dmadminpartner", 4) + adminToken := uploadCreateToken(t, database, "dmadminoutsider", 1) // Owner (admin), not a participant + + // Upload a file. + content := []byte("dm attachment content for admin non-participant forbidden test") + rr := doUpload(t, router, token1, "file", "dmadminsecret.txt", content) + if rr.Code != http.StatusCreated { + t.Fatalf("upload: %d; body: %s", rr.Code, rr.Body.String()) + } + var resp map[string]any + _ = json.NewDecoder(rr.Body).Decode(&resp) + fileID := resp["id"].(string) + + // Create DM channel with two participants (not the admin). + _, err := database.ExecContext(context.Background(), `INSERT INTO channels (id, name, type) VALUES (1, 'dm-1', 'dm')`) + if err != nil { + t.Fatalf("insert channel: %v", err) + } + var ownerID, partnerID int64 + _ = database.QueryRowContext(context.Background(), `SELECT id FROM users WHERE username = 'dmadminowner'`).Scan(&ownerID) + _ = database.QueryRowContext(context.Background(), `SELECT id FROM users WHERE username = 'dmadminpartner'`).Scan(&partnerID) + _, _ = database.ExecContext(context.Background(), `INSERT INTO dm_participants (user_id, channel_id) VALUES (?, 1)`, ownerID) + _, _ = database.ExecContext(context.Background(), `INSERT INTO dm_participants (user_id, channel_id) VALUES (?, 1)`, partnerID) + _, _ = database.ExecContext(context.Background(), `INSERT INTO messages (id, channel_id, user_id, content) VALUES (1, 1, ?, 'hi')`, ownerID) + _, _ = database.ExecContext(context.Background(), `UPDATE attachments SET message_id = 1 WHERE id = ?`, fileID) + + // Admin who is not a DM participant must still be denied. + rr2 := doServeFile(t, router, fileID, adminToken, nil) + if rr2.Code != http.StatusForbidden { + t.Errorf("status = %d, want 403 for admin who is not a DM participant", rr2.Code) + } +} diff --git a/Server/api/waf.go b/Server/api/waf.go index 5068bd4c..c7170235 100644 --- a/Server/api/waf.go +++ b/Server/api/waf.go @@ -93,6 +93,16 @@ func newCRSWAF(paranoiaLevel int, block bool, onMatch func(types.MatchedRule)) ( # CRS numbering scheme. SecRule REQUEST_URI "@beginsWith /api/v1/uploads" "id:1001,phase:1,pass,nolog,ctl:requestBodyAccess=Off,ctl:ruleRemoveById=920420" + # Same exclusion for the other routes with a larger-than-1-MiB + # app-level cap (see bodyCapExemptPrefixes in constants.go, and + # the mirrored inline-engine rules 900004/900005): keeps this + # engine's SecRequestBodyLimitAction ProcessPartial from + # evaluating rules against a truncated buffer, and 920420 + # from rejecting their non-default content types (application/zip, + # raw image bytes) under CRS blocking mode. + SecRule REQUEST_URI "@beginsWith /api/v1/admin/plugins/install" "id:1002,phase:1,pass,nolog,ctl:requestBodyAccess=Off,ctl:ruleRemoveById=920420" + SecRule REQUEST_URI "@beginsWith /api/v1/users/me/avatar" "id:1003,phase:1,pass,nolog,ctl:requestBodyAccess=Off,ctl:ruleRemoveById=920420" + Include @owasp_crs/*.conf # Engine mode: DetectionOnly logs matches without interrupting; @@ -241,6 +251,15 @@ func newWAFMiddleware(paranoiaLevel int, crsMode string, onCRSMatch func(types.M # Exclude file upload endpoint from body inspection (binary content) SecRule REQUEST_URI "@beginsWith /api/v1/uploads" "id:900003,phase:1,pass,nolog,ctl:requestBodyAccess=Off" + + # Exclude the other routes with a larger-than-1-MiB app-level + # cap too (see bodyCapExemptPrefixes in constants.go: 16 MiB + # plugin installs, 2 MiB avatars). Without this, coraza's + # default SecRequestBodyLimitAction (Reject) 413s any body + # that reaches this engine's 1 MiB SecRequestBodyLimit before + # the app's own, larger limit is ever consulted. + SecRule REQUEST_URI "@beginsWith /api/v1/admin/plugins/install" "id:900004,phase:1,pass,nolog,ctl:requestBodyAccess=Off" + SecRule REQUEST_URI "@beginsWith /api/v1/users/me/avatar" "id:900005,phase:1,pass,nolog,ctl:requestBodyAccess=Off" `, paranoiaLevel)), ) if err != nil { diff --git a/Server/api/waf_test.go b/Server/api/waf_test.go index 2cd8b2e8..7370a772 100644 --- a/Server/api/waf_test.go +++ b/Server/api/waf_test.go @@ -89,6 +89,73 @@ func TestWAFMiddleware_BlocksScannerUserAgent(t *testing.T) { } } +// Routes exempted from the app's global 1 MiB body cap (bodyCapExemptPrefixes +// in constants.go) must also be exempted from the inline WAF engine's own +// SecRequestBodyLimit, or coraza's default SecRequestBodyLimitAction (Reject) +// 413s the request as soon as its buffer hits 1 MiB — well below these +// routes' documented, larger caps. +func TestWAFMiddleware_AllowsLargePluginInstallBody(t *testing.T) { + requestBody := strings.Repeat("A", 2*1024*1024) // 2 MiB; within the 16 MiB plugin-install cap + middleware := NewWAFMiddlewareCRS(2, CRSModeDetect) + + called := false + handler := middleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + called = true + body, err := io.ReadAll(r.Body) + if err != nil { + t.Fatalf("ReadAll: %v", err) + } + if len(body) != len(requestBody) { + t.Fatalf("body len = %d, want %d", len(body), len(requestBody)) + } + w.WriteHeader(http.StatusNoContent) + })) + + req := httptest.NewRequest(http.MethodPost, "/api/v1/admin/plugins/install", strings.NewReader(requestBody)) + req.Header.Set("Content-Type", "application/zip") + req.RemoteAddr = "127.0.0.1:9999" + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + + if !called { + t.Fatalf("expected downstream handler to be called, got status %d body %s", rr.Code, rr.Body.String()) + } + if rr.Code != http.StatusNoContent { + t.Fatalf("status = %d, want 204; body = %s", rr.Code, rr.Body.String()) + } +} + +func TestWAFMiddleware_AllowsLargeAvatarUploadBody(t *testing.T) { + requestBody := strings.Repeat("A", 1_100_000) // >1 MiB; within the 2 MiB avatar cap + middleware := NewWAFMiddlewareCRS(2, CRSModeDetect) + + called := false + handler := middleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + called = true + body, err := io.ReadAll(r.Body) + if err != nil { + t.Fatalf("ReadAll: %v", err) + } + if len(body) != len(requestBody) { + t.Fatalf("body len = %d, want %d", len(body), len(requestBody)) + } + w.WriteHeader(http.StatusNoContent) + })) + + req := httptest.NewRequest(http.MethodPost, "/api/v1/users/me/avatar", strings.NewReader(requestBody)) + req.Header.Set("Content-Type", "image/png") + req.RemoteAddr = "127.0.0.1:9999" + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + + if !called { + t.Fatalf("expected downstream handler to be called, got status %d body %s", rr.Code, rr.Body.String()) + } + if rr.Code != http.StatusNoContent { + t.Fatalf("status = %d, want 204; body = %s", rr.Code, rr.Body.String()) + } +} + func TestWAFMiddleware_PreservesReadableBodyForDownstream(t *testing.T) { const requestBody = `{"message":"hello world"}` middleware := NewWAFMiddlewareCRS(2, CRSModeDetect) diff --git a/Server/db/message_queries.go b/Server/db/message_queries.go index ee8b956c..661723c6 100644 --- a/Server/db/message_queries.go +++ b/Server/db/message_queries.go @@ -28,13 +28,21 @@ func messageFromGen(m dbgen.Message) *Message { } // sanitizeFTSQuery strips FTS5 operator characters from user input to prevent -// query injection. Only allows letters, digits, spaces, and hyphens. +// query injection. Only allows letters, digits, and spaces through unchanged; +// '-' is folded to a space rather than kept, because in FTS5's MATCH grammar +// '-' is not a bareword character -- it introduces a column filter +// ("-col: expr"), so keeping it turns "well-known" into a filter on a +// nonexistent column "known" and SQLite errors instead of matching. Folding +// to a space (rather than dropping it) still matches the indexed tokens. func sanitizeFTSQuery(q string) string { var sb strings.Builder sb.Grow(len(q)) for _, r := range q { - if unicode.IsLetter(r) || unicode.IsDigit(r) || r == ' ' || r == '-' { + switch { + case unicode.IsLetter(r) || unicode.IsDigit(r) || r == ' ': sb.WriteRune(r) + case r == '-': + sb.WriteRune(' ') } } result := strings.TrimSpace(sb.String()) diff --git a/Server/db/message_queries_test.go b/Server/db/message_queries_test.go index 8810b360..ea26486e 100644 --- a/Server/db/message_queries_test.go +++ b/Server/db/message_queries_test.go @@ -700,6 +700,26 @@ func TestSearchMessages_PinnedMessageStaysSearchable(t *testing.T) { } } +// OC-0096: sanitizeFTSQuery kept '-' as an allowed bareword character, but in +// FTS5's MATCH grammar '-' introduces a column filter ("-col: expr"), so +// "well-known" parses as "well" followed by a filter on a nonexistent column +// "known" and SQLite raises "no such column: known" instead of matching. +func TestSearchMessages_HyphenatedQuery(t *testing.T) { + database := openMigratedMemory(t) + userID := seedUser(t, database, "hyphenuser") + chID := seedChannel(t, database, "hyphench") + + _, _ = database.CreateMessage(context.Background(), chID, userID, "a well-known fact", nil) + + results, err := database.SearchMessages(context.Background(), "well-known", nil, 10) + if err != nil { + t.Fatalf("SearchMessages(%q): %v", "well-known", err) + } + if len(results) != 1 { + t.Errorf("expected 1 result, got %d", len(results)) + } +} + // ─── UpdateReadState ────────────────────────────────────────────────────────── func TestUpdateReadState_Upsert(t *testing.T) { diff --git a/Server/main.go b/Server/main.go index 753d1594..a91e5798 100644 --- a/Server/main.go +++ b/Server/main.go @@ -132,6 +132,11 @@ func run(log *slog.Logger, logBuf *admin.RingBuffer, levelVar *slog.LevelVar) er } defer database.Close() //nolint:errcheck + // The admin "Restore backup" handler needs the real database file path: + // without this, it falls back to a hardcoded "data/chatserver.db" and + // silently no-ops on any server with a configured database.path. + admin.SetDatabasePath(cfg.Database.Path) + if err := db.Migrate(database); err != nil { return fmt.Errorf("running migrations: %w", err) } diff --git a/Server/service/archived_channel_readonly_test.go b/Server/service/archived_channel_readonly_test.go index 368f7c78..73375d89 100644 --- a/Server/service/archived_channel_readonly_test.go +++ b/Server/service/archived_channel_readonly_test.go @@ -4,6 +4,8 @@ import ( "context" "errors" "testing" + + "github.com/owncord/server/db" ) // `archived` used to be consulted only by the visibility predicate @@ -63,3 +65,129 @@ func TestSendMessage_AllowedAfterUnarchive(t *testing.T) { t.Fatalf("SendMessage after unarchive: %v", err) } } + +// OC-0022: SendMessage's archived gate lived only on SendMessage. EditMessage +// routed its non-DM permission check through checkSendPermission, which +// carried no archived check at all, so an author who still held a message id +// could keep injecting arbitrary new text into an archived channel — fanned +// out to every reader as chat_edited — even though a fresh chat_send was +// refused. The gate must be shared, not re-implemented per sink. +func TestEditMessage_RefusedInArchivedChannel(t *testing.T) { + svc, database := newTestMessageService(t) + ctx := context.Background() + + sent, err := svc.SendMessage(ctx, SendMessageParams{ + ChannelID: 10, UserID: 1, Username: "alice", RoleName: "member", Content: "original", + }) + if err != nil { + t.Fatalf("send: %v", err) + } + + if _, err := database.ExecContext(ctx, + `UPDATE channels SET archived = 1 WHERE id = 10`); err != nil { + t.Fatalf("archive channel: %v", err) + } + + if _, err := svc.EditMessage(ctx, 1, sent.MessageID, "slipped past the archive"); !errors.Is(err, ErrForbidden) { + t.Fatalf("EditMessage in an archived channel: err = %v, want ErrForbidden", err) + } + + msg, err := database.GetMessage(ctx, sent.MessageID) + if err != nil || msg == nil { + t.Fatalf("GetMessage: %v", err) + } + if msg.Content != "original" { + t.Fatalf("content must survive a refused edit against an archived channel, got %q", msg.Content) + } +} + +// OC-0022: handleReaction (AddReaction/RemoveReaction) bypasses +// checkSendPermission entirely, so it needs its own archived gate. A reaction +// fans a live reaction_update out to every reader just like a send or edit. +func TestAddReaction_RefusedInArchivedChannel(t *testing.T) { + svc, database := newTestMessageService(t) + ctx := context.Background() + + sent, err := svc.SendMessage(ctx, SendMessageParams{ + ChannelID: 10, UserID: 1, Username: "alice", RoleName: "member", Content: "react to me", + }) + if err != nil { + t.Fatalf("send: %v", err) + } + + if _, err := database.ExecContext(ctx, + `UPDATE channels SET archived = 1 WHERE id = 10`); err != nil { + t.Fatalf("archive channel: %v", err) + } + + if _, err := svc.AddReaction(ctx, 1, sent.MessageID, "👍"); !errors.Is(err, ErrForbidden) { + t.Fatalf("AddReaction in an archived channel: err = %v, want ErrForbidden", err) + } + + reactors, err := database.GetReactionUsers(ctx, sent.MessageID, "👍", db.MaxReactionUsers) + if err != nil { + t.Fatalf("GetReactionUsers: %v", err) + } + if len(reactors) != 0 { + t.Fatalf("reaction must not persist against an archived channel, got %d reactors", len(reactors)) + } +} + +// OC-0022: SetMessagePinned also bypasses checkSendPermission, so a +// MANAGE_MESSAGES holder could still pin/unpin in an archived channel. +func TestSetMessagePinned_RefusedInArchivedChannel(t *testing.T) { + svc, database := newPurgeService(t) // seeds user 2 with MANAGE_MESSAGES on channel 10 + ctx := context.Background() + + sent, err := svc.SendMessage(ctx, SendMessageParams{ + ChannelID: 10, UserID: 1, Username: "alice", RoleName: "member", Content: "pin me", + }) + if err != nil { + t.Fatalf("send: %v", err) + } + + if _, err := database.ExecContext(ctx, + `UPDATE channels SET archived = 1 WHERE id = 10`); err != nil { + t.Fatalf("archive channel: %v", err) + } + + if err := svc.SetMessagePinned(ctx, 2, 10, sent.MessageID, true); !errors.Is(err, ErrForbidden) { + t.Fatalf("SetMessagePinned in an archived channel: err = %v, want ErrForbidden", err) + } + + pinned, err := database.GetPinnedMessages(ctx, 10, 2) + if err != nil { + t.Fatalf("GetPinnedMessages: %v", err) + } + if len(pinned) != 0 { + t.Fatalf("pin must not persist against an archived channel, got %d pinned", len(pinned)) + } +} + +// OC-0022: PurgeMessages also bypasses checkSendPermission, so a +// MANAGE_MESSAGES holder could still bulk-delete an archived channel's +// history. +func TestPurgeMessages_RefusedInArchivedChannel(t *testing.T) { + svc, database := newPurgeService(t) + ctx := context.Background() + ids := seedPurgeMessages(t, database, 10, 3) + + if _, err := database.ExecContext(ctx, + `UPDATE channels SET archived = 1 WHERE id = 10`); err != nil { + t.Fatalf("archive channel: %v", err) + } + + if _, err := svc.PurgeMessages(ctx, 2, 10, 3, 0); !errors.Is(err, ErrForbidden) { + t.Fatalf("PurgeMessages against an archived channel: err = %v, want ErrForbidden", err) + } + + for _, id := range ids { + msg, err := database.GetMessage(ctx, id) + if err != nil || msg == nil { + t.Fatalf("GetMessage(%d): %v", id, err) + } + if msg.Deleted { + t.Fatalf("message %d must survive a purge attempt against an archived channel", id) + } + } +} diff --git a/Server/service/message.go b/Server/service/message.go index beab5a88..8c8b3e5e 100644 --- a/Server/service/message.go +++ b/Server/service/message.go @@ -204,6 +204,17 @@ func sanitizeToFixpoint(raw string) string { return s } +// SanitizeText runs raw through the same unescape-sanitize-fixpoint pipeline +// as message content and profile free-text fields (see sanitizeToFixpoint): +// it strips HTML but leaves survivors as typed instead of persisting them as +// literal '/>/& entities the way a bare bluemonday sanitizer.Sanitize +// call would. Exported for call sites outside this package that sanitize a +// single free-text field before storage — e.g. the username field on +// registration, which must canonicalize identically to how lookups treat it. +func SanitizeText(raw string) string { + return sanitizeToFixpoint(raw) +} + // sanitizeContent validates and sanitizes message content. func sanitizeContent(raw string, allowEmpty bool) (string, error) { if len(raw) > maxMessageLen*4 { diff --git a/Server/service/message_crud.go b/Server/service/message_crud.go index 9d1070c9..8b8318a4 100644 --- a/Server/service/message_crud.go +++ b/Server/service/message_crud.go @@ -45,18 +45,10 @@ func (s *MessageService) SendMessage(ctx context.Context, p SendMessageParams) ( isDM := ch.Type == "dm" - // Archived channels are read-only. Until now `archived` was consulted only - // by the visibility predicate (VisibleChannelIDs / RefreshChannelVisibility), - // so it hid the channel without protecting it: any caller that still held - // the id — a custom client, or a stock client racing the channel_delete — - // could keep posting into an archive indefinitely. History stays readable; - // only writes are refused. - if !isDM && ch.Archived { - return nil, fmt.Errorf("%w: channel is archived", ErrForbidden) - } - - // Permission check. - if err := s.checkSendPermission(ctx, p.UserID, p.ChannelID, ch.Type); err != nil { + // Permission check. Also refuses a write against an archived channel — see + // requireChannelWritable in message_perms.go, the shared gate every + // message write sink routes through. + if err := s.checkSendPermission(ctx, p.UserID, ch); err != nil { return nil, err } @@ -290,7 +282,7 @@ func (s *MessageService) EditMessage(ctx context.Context, userID, msgID int64, r if blkErr := requireDMNotBlocked(ctx, s.st, userID, msg.ChannelID); blkErr != nil { return nil, blkErr } - } else if permErr := s.checkSendPermission(ctx, userID, msg.ChannelID, chanType); permErr != nil { + } else if permErr := s.checkSendPermission(ctx, userID, ch); permErr != nil { // An edit injects new text into the channel and is fanned out to every // reader, so it must clear the same gate as a send rather than // SEND_MESSAGES alone: READ_MESSAGES so a role locked out of a private @@ -379,11 +371,11 @@ func (s *MessageService) DeleteMessage(ctx context.Context, userID, msgID int64) } isDM := ch.Type == "dm" - // Archived channels are read-only, mirroring SendMessage's gate - // (message_crud.go:54): history stays visible, but a member or moderator - // must not be able to mutate it by deleting a message out of the archive. - if !isDM && ch.Archived { - return nil, fmt.Errorf("%w: channel is archived", ErrForbidden) + // Archived channels are read-only — see requireChannelWritable in + // message_perms.go, the shared gate every message write sink routes + // through. + if err := requireChannelWritable(ch); err != nil { + return nil, err } var isMod bool diff --git a/Server/service/message_perms.go b/Server/service/message_perms.go index 66640337..aa33c39e 100644 --- a/Server/service/message_perms.go +++ b/Server/service/message_perms.go @@ -59,36 +59,73 @@ func (s *MessageService) CanPost(ctx context.Context, userID, channelID int64) e if err != nil || ch == nil { return fmt.Errorf("%w: channel not found", ErrNotFound) } - return s.checkSendPermission(ctx, userID, channelID, ch.Type) + return s.checkSendPermission(ctx, userID, ch) } -// checkSendPermission validates send permission for a channel of the given -// type. Announcement channels are readable by anyone with READ_MESSAGES but -// only postable by users with MANAGE_MESSAGES (posting is restricted to -// moderators/admins); all other non-DM channels require SEND_MESSAGES. -func (s *MessageService) checkSendPermission(ctx context.Context, userID, channelID int64, chanType string) error { - isDM := chanType == "dm" +// checkSendPermission validates send permission for ch. Announcement channels +// are readable by anyone with READ_MESSAGES but only postable by users with +// MANAGE_MESSAGES (posting is restricted to moderators/admins); all other +// non-DM channels require SEND_MESSAGES. Also enforces requireChannelWritable, +// so every caller — SendMessage, EditMessage, CanPost — refuses an archived +// channel without re-implementing that check itself. +func (s *MessageService) checkSendPermission(ctx context.Context, userID int64, ch *db.Channel) error { + isDM := ch.Type == "dm" if isDM { - ok, err := s.st.IsDMParticipant(ctx, userID, channelID) + ok, err := s.st.IsDMParticipant(ctx, userID, ch.ID) if err != nil { return fmt.Errorf("%w: failed to check DM participation: %v", ErrInternal, err) } if !ok { return fmt.Errorf("%w: not a participant in this DM", ErrForbidden) } - return requireDMNotBlocked(ctx, s.st, userID, channelID) + return requireDMNotBlocked(ctx, s.st, userID, ch.ID) } - if !s.perms.HasChannelPerm(ctx, userID, channelID, permissions.ReadMessages|permissions.SendMessages) { + if err := requireChannelWritable(ch); err != nil { + return err + } + if !s.perms.HasChannelPerm(ctx, userID, ch.ID, permissions.ReadMessages|permissions.SendMessages) { return fmt.Errorf("%w: missing SEND_MESSAGES permission", ErrForbidden) } // Announcement channels: posting is restricted to users who can manage // messages, even though everyone with READ_MESSAGES can view them. - if chanType == "announcement" && !s.perms.HasChannelPerm(ctx, userID, channelID, permissions.ManageMessages) { + if ch.Type == "announcement" && !s.perms.HasChannelPerm(ctx, userID, ch.ID, permissions.ManageMessages) { return fmt.Errorf("%w: announcement channels require MANAGE_MESSAGES to post", ErrForbidden) } return nil } +// requireChannelWritable refuses a write against an archived non-DM channel. +// `archived` used to be consulted only by the visibility predicate +// (VisibleChannelIDs / RefreshChannelVisibility), so it hid a channel without +// protecting it: any caller that still held the id — a custom client, or a +// stock client racing the channel_delete that archiving triggers — could keep +// posting, editing, reacting, pinning, or bulk-deleting in an archive +// indefinitely. History stays readable; only writes are refused. +// +// DMs carry no archive flag/concept and are exempt. ch == nil is treated as +// "nothing to check" rather than a panic — the caller's own nil handling (a +// failed channel lookup) decides what happens next. +// +// Single shared gate for every write sink: checkSendPermission (so +// SendMessage, EditMessage and CanPost inherit it), plus DeleteMessage, +// handleReaction, SetMessagePinned and PurgeMessages, which route their own +// permission checks and so call it directly instead. +func requireChannelWritable(ch *db.Channel) error { + if ch == nil || ch.Type == "dm" || !ch.Archived { + return nil + } + return fmt.Errorf("%w: channel is archived", ErrForbidden) +} + +// RequireDMNotBlocked is the exported form of requireDMNotBlocked so callers +// outside the service package (voice join/token-refresh, ws/voice_join.go) +// can share this single block-check implementation — same group-DM exemption, +// same "lookup failure is not a block" posture — instead of reimplementing it +// against the raw DB. st only needs to be a Store; *db.DB satisfies it. +func RequireDMNotBlocked(ctx context.Context, st Store, userID, channelID int64) error { + return requireDMNotBlocked(ctx, st, userID, channelID) +} + // requireDMNotBlocked reports ErrBlocked when userID and the other participant // of DM channelID have blocked each other in either direction. // diff --git a/Server/service/message_purge.go b/Server/service/message_purge.go index 998f4057..becc57dd 100644 --- a/Server/service/message_purge.go +++ b/Server/service/message_purge.go @@ -52,6 +52,13 @@ func (s *MessageService) PurgeMessages(ctx context.Context, userID, channelID in if ch.Type == "dm" { return nil, fmt.Errorf("%w: bulk delete is not available in direct messages", ErrForbidden) } + // Archived channels are read-only. PurgeMessages bypasses + // checkSendPermission (it runs its own MANAGE_MESSAGES check below), so it + // needs the shared gate directly — see requireChannelWritable in + // message_perms.go. + if err := requireChannelWritable(ch); err != nil { + return nil, err + } if !s.perms.HasChannelPerm(ctx, userID, channelID, permissions.ReadMessages|permissions.ManageMessages) { return nil, fmt.Errorf("%w: missing MANAGE_MESSAGES permission", ErrForbidden) } diff --git a/Server/service/message_query.go b/Server/service/message_query.go index 690272dd..bd040783 100644 --- a/Server/service/message_query.go +++ b/Server/service/message_query.go @@ -205,6 +205,13 @@ func (s *MessageService) SetMessagePinned(ctx context.Context, userID, channelID if err != nil || ch == nil { return fmt.Errorf("%w: channel not found", ErrNotFound) } + // Archived channels are read-only. SetMessagePinned bypasses + // checkSendPermission (it runs its own DM/permission branch below), so it + // needs the shared gate directly — see requireChannelWritable in + // message_perms.go. + if err := requireChannelWritable(ch); err != nil { + return err + } if ch.Type == "dm" { ok, err := s.st.IsDMParticipant(ctx, userID, channelID) if err != nil || !ok { diff --git a/Server/service/message_reactions.go b/Server/service/message_reactions.go index 7bc50428..69ba94ac 100644 --- a/Server/service/message_reactions.go +++ b/Server/service/message_reactions.go @@ -105,6 +105,14 @@ func (s *MessageService) handleReaction(ctx context.Context, userID, msgID int64 ch, chErr := s.st.GetChannel(ctx, msg.ChannelID) isDM := chErr == nil && ch != nil && ch.Type == "dm" + // Archived channels are read-only. handleReaction bypasses + // checkSendPermission (it runs its own DM/permission branch below), so it + // needs the shared gate directly — see requireChannelWritable in + // message_perms.go. + if err := requireChannelWritable(ch); err != nil { + return nil, err + } + var participantIDs []int64 if isDM { ok, dmErr := s.st.IsDMParticipant(ctx, userID, msg.ChannelID) diff --git a/Server/service/moderation.go b/Server/service/moderation.go index 93400fa6..829b08b8 100644 --- a/Server/service/moderation.go +++ b/Server/service/moderation.go @@ -135,39 +135,45 @@ func (s *ModerationService) BanUser(ctx context.Context, actorID, targetID int64 // of: the actor must strictly outrank the target, and may not hand out a role // positioned at or above their own — otherwise any admin could promote anyone // (including themselves via a second account) to Owner. -func (s *ModerationService) ChangeUserRole(ctx context.Context, actorID, targetID, newRoleID int64) error { +// +// It returns the role that was assigned so callers (the member_update +// broadcast and visibility refresh, in particular) can use it directly +// instead of re-reading it: a re-read is racing a possible concurrent role +// delete for no reason, since this call already loaded and validated the +// exact same row under the same request. +func (s *ModerationService) ChangeUserRole(ctx context.Context, actorID, targetID, newRoleID int64) (*db.Role, error) { if targetID <= 0 { - return fmt.Errorf("%w: user_id must be positive", ErrBadRequest) + return nil, fmt.Errorf("%w: user_id must be positive", ErrBadRequest) } if actorID == targetID { - return fmt.Errorf("%w: cannot change your own role", ErrBadRequest) + return nil, fmt.Errorf("%w: cannot change your own role", ErrBadRequest) } // Authorization before existence — see BanUser. actorRole, err := s.requirePerm(ctx, actorID, permissions.ManageRoles) if err != nil { - return err + return nil, err } target, err := s.st.GetUserByID(ctx, targetID) if err != nil || target == nil { - return fmt.Errorf("%w: user not found", ErrNotFound) + return nil, fmt.Errorf("%w: user not found", ErrNotFound) } if err := s.requireOutranksRole(ctx, actorRole, targetID); err != nil { - return err + return nil, err } newRole, err := s.st.GetRoleByID(ctx, newRoleID) if err != nil || newRole == nil { - return fmt.Errorf("%w: role not found", ErrBadRequest) + return nil, fmt.Errorf("%w: role not found", ErrBadRequest) } // Administrator bypasses permission bits, never the hierarchy: the owner // role is above every admin, so only the owner can grant it. if newRole.Position >= actorRole.Position { - return fmt.Errorf("%w: cannot assign a role at or above your own rank", ErrForbidden) + return nil, fmt.Errorf("%w: cannot assign a role at or above your own rank", ErrForbidden) } if err := s.st.UpdateUserRole(ctx, targetID, newRoleID); err != nil { - return fmt.Errorf("%w: failed to update role: %v", ErrInternal, err) + return nil, fmt.Errorf("%w: failed to update role: %v", ErrInternal, err) } // Drop the target's cached role immediately: without this a demotion keeps // granting the old bits (and the old rank) for up to permCacheTTL. @@ -178,7 +184,7 @@ func (s *ModerationService) ChangeUserRole(ctx context.Context, actorID, targetI fmt.Sprintf("changed %s role to %s", target.Username, newRole.Name)) slog.Info("role changed", "actor_id", actorID, "target_id", targetID, "new_role_id", newRoleID) - return nil + return newRole, nil } // ForceLogout revokes every session of the target user (the client's "Kick"). diff --git a/Server/service/moderation_test.go b/Server/service/moderation_test.go index f7e086ea..32843c16 100644 --- a/Server/service/moderation_test.go +++ b/Server/service/moderation_test.go @@ -115,11 +115,11 @@ func TestChangeUserRole_RequiresManageRoles(t *testing.T) { svc, database := newTestRoleService(t) // A member without MANAGE_ROLES is refused... - if err := svc.ChangeUserRole(context.Background(), 4, 5, 3); !errors.Is(err, ErrForbidden) { + if _, err := svc.ChangeUserRole(context.Background(), 4, 5, 3); !errors.Is(err, ErrForbidden) { t.Fatalf("member role change: want ErrForbidden, got %v", err) } // ...and gets Forbidden, not NotFound, for a missing target. - if err := svc.ChangeUserRole(context.Background(), 4, 999, 3); !errors.Is(err, ErrForbidden) { + if _, err := svc.ChangeUserRole(context.Background(), 4, 999, 3); !errors.Is(err, ErrForbidden) { t.Fatalf("unauthorized probe of missing id: want ErrForbidden, got %v", err) } if got := roleIDOf(t, database, 5); got != 4 { @@ -131,25 +131,25 @@ func TestChangeUserRole_CannotAssignAtOrAboveOwnRank(t *testing.T) { svc, database := newTestRoleService(t) // The hole this closes: an Administrator could promote anyone to Owner. - if err := svc.ChangeUserRole(context.Background(), 2, 4, 1); !errors.Is(err, ErrForbidden) { + if _, err := svc.ChangeUserRole(context.Background(), 2, 4, 1); !errors.Is(err, ErrForbidden) { t.Fatalf("admin promoting to owner: want ErrForbidden, got %v", err) } // Equal rank is refused too — an admin cannot mint another admin. - if err := svc.ChangeUserRole(context.Background(), 2, 4, 2); !errors.Is(err, ErrForbidden) { + if _, err := svc.ChangeUserRole(context.Background(), 2, 4, 2); !errors.Is(err, ErrForbidden) { t.Fatalf("admin assigning own rank: want ErrForbidden, got %v", err) } if got := roleIDOf(t, database, 4); got != 4 { t.Fatalf("member role changed to %d despite refusal", got) } // Strictly below own rank is allowed. - if err := svc.ChangeUserRole(context.Background(), 2, 4, 3); err != nil { + if _, err := svc.ChangeUserRole(context.Background(), 2, 4, 3); err != nil { t.Fatalf("admin promoting to mod: %v", err) } if got := roleIDOf(t, database, 4); got != 3 { t.Fatalf("role after promotion = %d, want 3", got) } // The owner outranks the admin role, so the owner may grant it. - if err := svc.ChangeUserRole(context.Background(), 1, 5, 2); err != nil { + if _, err := svc.ChangeUserRole(context.Background(), 1, 5, 2); err != nil { t.Fatalf("owner promoting to admin: %v", err) } } @@ -158,26 +158,26 @@ func TestChangeUserRole_HierarchyAndValidation(t *testing.T) { svc, database := newTestRoleService(t) // A moderator holding MANAGE_ROLES still cannot touch a higher-ranked user. - if err := svc.ChangeUserRole(context.Background(), 3, 2, 4); !errors.Is(err, ErrForbidden) { + if _, err := svc.ChangeUserRole(context.Background(), 3, 2, 4); !errors.Is(err, ErrForbidden) { t.Fatalf("mod demoting an admin: want ErrForbidden, got %v", err) } if got := roleIDOf(t, database, 2); got != 2 { t.Fatalf("admin role changed to %d despite refusal", got) } // Nor the owner. - if err := svc.ChangeUserRole(context.Background(), 3, 1, 4); !errors.Is(err, ErrForbidden) { + if _, err := svc.ChangeUserRole(context.Background(), 3, 1, 4); !errors.Is(err, ErrForbidden) { t.Fatalf("mod demoting the owner: want ErrForbidden, got %v", err) } // Self-service promotion is a bad request regardless of authority. - if err := svc.ChangeUserRole(context.Background(), 2, 2, 1); !errors.Is(err, ErrBadRequest) { + if _, err := svc.ChangeUserRole(context.Background(), 2, 2, 1); !errors.Is(err, ErrBadRequest) { t.Fatalf("self role change: want ErrBadRequest, got %v", err) } // A nonexistent role is a bad request, not a 500. - if err := svc.ChangeUserRole(context.Background(), 1, 4, 9999); !errors.Is(err, ErrBadRequest) { + if _, err := svc.ChangeUserRole(context.Background(), 1, 4, 9999); !errors.Is(err, ErrBadRequest) { t.Fatalf("unknown role id: want ErrBadRequest, got %v", err) } // Authorized actor gets a real NotFound for a missing target. - if err := svc.ChangeUserRole(context.Background(), 1, 999, 4); !errors.Is(err, ErrNotFound) { + if _, err := svc.ChangeUserRole(context.Background(), 1, 999, 4); !errors.Is(err, ErrNotFound) { t.Fatalf("missing target: want ErrNotFound, got %v", err) } } @@ -185,7 +185,7 @@ func TestChangeUserRole_HierarchyAndValidation(t *testing.T) { func TestChangeUserRole_AuditWritten(t *testing.T) { svc, database := newTestRoleService(t) - if err := svc.ChangeUserRole(context.Background(), 1, 4, 3); err != nil { + if _, err := svc.ChangeUserRole(context.Background(), 1, 4, 3); err != nil { t.Fatalf("owner role change: %v", err) } entries, err := database.GetAuditLog(context.Background(), 10, 0) diff --git a/Server/service/profile_fields_test.go b/Server/service/profile_fields_test.go index 450c6701..01c1d195 100644 --- a/Server/service/profile_fields_test.go +++ b/Server/service/profile_fields_test.go @@ -128,6 +128,48 @@ func TestUpdateProfile_ConcurrentUpdatesSerializePerUser(t *testing.T) { } } +// OC-0102: an avatar-only patch must not carry a username at all, so a stale +// pre-lock snapshot (handleUploadAvatar captures user.Username before the +// multipart parse / image decode / disk write, well before this function's +// per-user lock) can never overwrite a rename that commits in the meantime. +// UpdateProfile's read-merge-write already treats DisplayName/About this +// way (nil-vs-non-nil pointer); Username needs the same "unspecified means +// leave it alone" contract via its own zero value, since it is a plain +// string rather than a pointer. +func TestUpdateProfile_AvatarOnlyPatchDoesNotOverwriteUsername(t *testing.T) { + svc, database := newUserSvc(t) + ctx := context.Background() + + // Models a concurrent PATCH /users/me rename that lands and commits + // first. + if _, err := svc.UpdateProfile(ctx, 1, ProfilePatch{Username: "bob"}); err != nil { + t.Fatalf("rename UpdateProfile: %v", err) + } + + // An avatar-only caller supplies no username intent at all — Username is + // left at its zero value, the way handleUploadAvatar's ProfilePatch must + // after the fix (it no longer fills Username from its stale snapshot). + avatarURL := "/api/v1/files/abc" + u, err := svc.UpdateProfile(ctx, 1, ProfilePatch{Avatar: &avatarURL}) + if err != nil { + t.Fatalf("avatar-only UpdateProfile: %v", err) + } + if u.Username != "bob" { + t.Fatalf("username = %q, want %q — an avatar-only patch must not revert a concurrent rename", u.Username, "bob") + } + if u.Avatar == nil || *u.Avatar != avatarURL { + t.Fatalf("avatar = %v, want %q — the avatar itself must still be applied", u.Avatar, avatarURL) + } + + stored, err := database.GetUserByID(ctx, 1) + if err != nil { + t.Fatalf("GetUserByID: %v", err) + } + if stored.Username != "bob" { + t.Fatalf("stored username = %q, want %q", stored.Username, "bob") + } +} + func TestUpdateProfile_SanitizesAndTrims(t *testing.T) { svc, _ := newUserSvc(t) name := " Ada " diff --git a/Server/service/user.go b/Server/service/user.go index 80e109f4..95f64fcd 100644 --- a/Server/service/user.go +++ b/Server/service/user.go @@ -159,6 +159,17 @@ func (s *UserService) UpdateProfile(ctx context.Context, userID int64, patch Pro if err != nil || current == nil { return nil, fmt.Errorf("%w: user not found", ErrNotFound) } + // An empty Username means "unspecified", the same as a nil + // DisplayName/About pointer — merged against the current row rather + // than written verbatim. This is what lets an avatar-only caller + // (handleUploadAvatar) leave username alone without handing over a + // snapshot that could be stale by the time this call lands: PATCH + // /users/me always validates and rejects an empty username before + // calling in, so "" never reaches here as a real rename request. + username := patch.Username + if username == "" { + username = current.Username + } avatar := current.Avatar if patch.Avatar != nil { avatar = nullable(*patch.Avatar) @@ -166,7 +177,7 @@ func (s *UserService) UpdateProfile(ctx context.Context, userID int64, patch Pro displayName := resolveOptional(patch.DisplayName, current.DisplayName) about := resolveOptional(patch.About, current.About) - if err := s.st.UpdateUserProfile(ctx, userID, patch.Username, avatar, displayName, about); err != nil { + if err := s.st.UpdateUserProfile(ctx, userID, username, avatar, displayName, about); err != nil { if db.IsUniqueConstraintError(err) { return nil, fmt.Errorf("%w: username is already taken", ErrConflict) } @@ -178,8 +189,8 @@ func (s *UserService) UpdateProfile(ctx context.Context, userID int64, patch Pro } // Audit rows must survive a request canceled after the write committed. db.WriteAudit(context.WithoutCancel(ctx), s.st, userID, "profile_update", "user", userID, - fmt.Sprintf("username=%s", patch.Username)) - slog.Info("profile updated", "user_id", userID, "username", patch.Username) + fmt.Sprintf("username=%s", username)) + slog.Info("profile updated", "user_id", userID, "username", username) return user, nil } diff --git a/Server/ws/voice_dm_access_test.go b/Server/ws/voice_dm_access_test.go index 58d7dbab..7f23e82c 100644 --- a/Server/ws/voice_dm_access_test.go +++ b/Server/ws/voice_dm_access_test.go @@ -216,6 +216,67 @@ func TestVoiceJoin_DMCall_VoiceStateNotLeakedToThirdConnectedUser(t *testing.T) } } +// OC-0018: voice_join into a 1:1 DM had no block gate. Every other 1:1-DM +// interaction sink (send, edit, react, pin, typing, call_ring) routes through +// service.requireDMNotBlocked; voice was the one gap. Blocking never touches +// dm_participants (service/block.go), so IsDMParticipant still passes a +// blocked user straight through into the blocker's DM voice room. +func TestVoiceJoin_DMBlocked_Refused(t *testing.T) { + hub, database := newVoiceHub(t) + alice := seedMemberUser(t, database, "dmblock-alice") + bob := seedMemberUser(t, database, "dmblock-bob") + dmID := seedDMChannel(t, database, alice.ID, bob.ID) + + if err := database.BlockUser(context.Background(), bob.ID, alice.ID); err != nil { + t.Fatalf("BlockUser: %v", err) + } + + send := make(chan []byte, 32) + c := ws.NewTestClientWithUser(hub, alice, 0, send) + hub.Register(c) + waitRegistered(t, hub, c) + + hub.HandleMessageForTest(c, voiceJoinMsg(dmID)) + + assertNoVoiceToken(t, drainChanTimeout(send, 200*time.Millisecond)) + + state, err := database.GetVoiceState(context.Background(), alice.ID) + if err != nil { + t.Fatalf("GetVoiceState: %v", err) + } + if state != nil { + t.Fatalf("blocked user was persisted into the DM's voice channel (%d)", state.ChannelID) + } +} + +// Second entry point: a block imposed mid-session must also evict on the next +// token refresh, not just refuse the initial join. Alice joins while still +// unblocked (so the join succeeds and a real voice_states row exists), then +// bob blocks her; the refresh must re-check and evict rather than keep +// minting fresh SFU room-join credentials for the old session. +func TestVoiceTokenRefresh_DMBlocked_Refused(t *testing.T) { + hub, database := newVoiceHub(t) + alice := seedMemberUser(t, database, "dmblockrefresh-alice") + bob := seedMemberUser(t, database, "dmblockrefresh-bob") + dmID := seedDMChannel(t, database, alice.ID, bob.ID) + + send := make(chan []byte, 32) + c := ws.NewTestClientWithUser(hub, alice, 0, send) + hub.Register(c) + waitRegistered(t, hub, c) + + hub.HandleMessageForTest(c, voiceJoinMsg(dmID)) + drainChanTimeout(send, 50*time.Millisecond) + + if err := database.BlockUser(context.Background(), bob.ID, alice.ID); err != nil { + t.Fatalf("BlockUser: %v", err) + } + + hub.HandleMessageForTest(c, voiceTokenRefreshMsg()) + + assertNoVoiceToken(t, drainChanTimeout(send, 200*time.Millisecond)) +} + func TestVoiceJoin_GroupDMNonParticipant_Refused(t *testing.T) { hub, database := newVoiceHub(t) alice := seedMemberUser(t, database, "grpvoice-x-alice") diff --git a/Server/ws/voice_join.go b/Server/ws/voice_join.go index f602ef79..c83be25e 100644 --- a/Server/ws/voice_join.go +++ b/Server/ws/voice_join.go @@ -10,6 +10,7 @@ import ( "github.com/owncord/server/auth" "github.com/owncord/server/db" "github.com/owncord/server/permissions" + "github.com/owncord/server/service" ) // Voice join/leave rate limits. voice_join and voice_leave each fan out a @@ -84,6 +85,21 @@ func (h *Hub) handleVoiceJoin(ctx context.Context, c *Client, payload json.RawMe return } + // A blocked user is still a DM participant — blocking never touches + // dm_participants (service/block.go), so the CONNECT_VOICE + IsDMParticipant + // gate above passes them straight through into the blocker's DM voice room. + // Every other 1:1-DM interaction sink (send, edit, react, pin, typing, + // call_ring) already routes through this same check + // (service.requireDMNotBlocked); voice was the one gap. Group DMs are + // exempt inside it, matching every other sink. h.db satisfies + // service.Store directly, so no MessageService wiring is needed here. + if ch.Type == "dm" { + if err := service.RequireDMNotBlocked(ctx, h.db, c.userID, channelID); err != nil { + c.sendMsg(buildErrorMsg(ErrCodeForbidden, "cannot join voice: blocked")) + return + } + } + // Archived channels are hidden from every client and their voice states are // dropped from `ready`, but `archived` was consulted only by the visibility // predicate — so a caller still holding the id could join the room of a @@ -422,6 +438,19 @@ func handleVoiceTokenRefreshV2(ctx context.Context, cmd Command, info ClientInfo } } + // Same block gate as voice_join (voice_join.go, OC-0018): a block imposed + // mid-session must not let the refresh keep minting a fresh SFU credential + // for a DM the other participant has since blocked. RequireDMNotBlocked is + // a safe no-op for a non-DM channelID (no dm_participants row to match), so + // this needs no channel-type fetch of its own. d.DB satisfies service.Store + // directly. + if err := service.RequireDMNotBlocked(ctx, d.DB, userID, channelID); err != nil { + return Result{ + Error: ClientError{Code: ErrCodeForbidden, Message: "cannot refresh voice token: blocked"}, + LeaveVoice: true, + } + } + // With a PermissionService these three are cache hits after the gate above // populated the user's entry — the refresh drops from ~9 DB reads to at // most one channel-row lookup.