From 8579cb5d917b12c87b3ff1c191389c9adea8f97c Mon Sep 17 00:00:00 2001 From: J3vb <192430104+J3vb@users.noreply.github.com> Date: Fri, 14 Aug 2026 14:49:27 +0200 Subject: [PATCH] fix: batch of 25 correctness fixes across server and client (#1370) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * chore(workflows): raise subagent effort tiers (sonnet/haiku to xhigh, prove opus to high) Co-Authored-By: Claude Fable 5 * fix(voice): 6 defect(s) (OC-0098, OC-0004, OC-0005, OC-0006, OC-0007, OC-0020) * fix(db): 1 defect(s) (OC-0096) * fix(admin): 1 defect(s) (OC-0097) * fix(auth): 2 defect(s) (OC-0099, OC-0021) * fix(voice): 1 defect(s) (OC-0018) * fix(admin): 1 defect(s) (OC-0045) * fix(api): 1 defect(s) (OC-0103) * fix(client): 1 defect(s) (OC-0105) * fix(client): 1 defect(s) (OC-0107) * fix(api): 1 defect(s) (OC-0109) * fix(api): 1 defect(s) (OC-0112) * test(admin): compare restore bytes with bytes.Equal Co-Authored-By: Claude Fable 5 * fix(voice): 2 defect(s) (OC-0095, OC-0014) OC-0095: createRoom never called setE2EEEnabled(true), so the full ECDH/HKDF/AES-GCM key exchange completed but frames still reached the SFU in plaintext. OC-0014: token refresh timer was 23h while the server mints LiveKit tokens with a 5-minute TTL, so any reconnect after minute 5 presented an expired token. * fix(profile): 2 defect(s) (OC-0100, OC-0102) * fix(service): 1 defect(s) (OC-0022) Archived channels were only read-only for SendMessage/DeleteMessage. Edit, reaction, pin and purge sinks bypassed the check. Route every write sink through a shared requireChannelWritable gate. * fix(api): 1 defect(s) (OC-0048) * chore(workflows): correct stale model labels in bughunt-fix phase details Co-Authored-By: Claude Fable 5 * fix(client): 1 defect(s) (OC-0015) * fix(voice): 1 defect(s) (OC-0002) * test: fix two CI-only failures in the batch-4 test suite The delete-account broadcast test now observes member_ban on a second client's socket: the hub broadcasts and then force-disconnects the target, so on a slow runner the close could beat the target's own copy of the frame. The observer is also the party the event exists for. The voice e2e mock now echoes the real joined channel id on voice_leave (it hardcoded channel_id 0, which the dispatcher's channel-matched self-leave teardown correctly ignores), and the rejoin test waits for the mock's delayed echoes to settle before clicking the row again — clicking inside the echo window toggled a leave instead of a join. Co-Authored-By: Claude Fable 5 --------- Co-authored-by: Claude Fable 5 --- .claude/workflows/bughunt-fix.harness.mjs | 4 +- .claude/workflows/bughunt-fix.js | 8 +- .claude/workflows/bughunt.js | 4 +- .../src-tauri/src/secret_store.rs | 84 +++++++- .../src/components/VoiceWidget.ts | 28 ++- Client/tauri-client/src/lib/dispatcher.ts | 32 ++- Client/tauri-client/src/lib/livekitE2EE.ts | 184 +++++++++++++++--- Client/tauri-client/src/lib/livekitSession.ts | 48 +++-- .../tauri-client/src/lib/roomEventHandlers.ts | 26 ++- Client/tauri-client/src/stores/voice.store.ts | 26 +++ Client/tauri-client/src/styles/app.css | 5 + Client/tauri-client/tests/e2e/helpers.ts | 8 +- .../tests/e2e/voice-lifecycle.spec.ts | 10 + .../tests/unit/dispatcher.test.ts | 48 +++++ .../tests/unit/livekit-e2ee.test.ts | 145 ++++++++++++++ .../tests/unit/livekit-session.test.ts | 124 +++++++++++- .../tests/unit/room-event-handlers.test.ts | 34 +++- .../tests/unit/voice-widget.test.ts | 31 +++ Server/admin/handlers_backup.go | 19 +- Server/admin/handlers_backup_test.go | 66 +++++++ Server/admin/handlers_users.go | 32 +-- Server/admin/handlers_users_broadcast_test.go | 63 ++++++ Server/api/auth_handler.go | 31 ++- Server/api/auth_handler_test.go | 108 ++++++++++ Server/api/gif_handler.go | 9 +- Server/api/gif_handler_internal_test.go | 30 +++ Server/api/profile_handler.go | 20 +- Server/api/profile_handler_test.go | 31 +++ Server/api/router.go | 14 +- .../router_delete_account_broadcast_test.go | 175 +++++++++++++++++ Server/api/upload_handler.go | 46 +++-- Server/api/upload_handler_test.go | 43 ++++ Server/api/waf.go | 19 ++ Server/api/waf_test.go | 67 +++++++ Server/db/message_queries.go | 12 +- Server/db/message_queries_test.go | 20 ++ Server/main.go | 5 + .../service/archived_channel_readonly_test.go | 128 ++++++++++++ Server/service/message.go | 11 ++ Server/service/message_crud.go | 28 +-- Server/service/message_perms.go | 59 ++++-- Server/service/message_purge.go | 7 + Server/service/message_query.go | 7 + Server/service/message_reactions.go | 8 + Server/service/moderation.go | 26 ++- Server/service/moderation_test.go | 24 +-- Server/service/profile_fields_test.go | 42 ++++ Server/service/user.go | 17 +- Server/ws/voice_dm_access_test.go | 61 ++++++ Server/ws/voice_join.go | 29 +++ 50 files changed, 1935 insertions(+), 171 deletions(-) create mode 100644 Server/api/gif_handler_internal_test.go create mode 100644 Server/api/router_delete_account_broadcast_test.go 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.