From 4ff199e14f6db205a777df17baaa425a8704543b Mon Sep 17 00:00:00 2001 From: J3vb <192430104+J3vb@users.noreply.github.com> Date: Fri, 7 Aug 2026 21:20:48 +0200 Subject: [PATCH] fix: resolve 107 verified defects across ws hub, voice/E2EE, db, and client (#1331) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(client): style the user profile popup The popup rendered unstyled: it appeared at the bottom of the page and pushed the rest of the app up, with the avatar drawn as a full-width bar. app.css carried a complete Discord-shaped card under `.user-popup` / `.up-*`, but nothing in the codebase renders those classes — the component emits `.upp-*`. The component had been rewritten with a new prefix and the stylesheet was left pointing at a DOM that no longer existed. With no rule matching, the card stayed `position: static`, so the left/top it computes were discarded and both it and its overlay laid out as ordinary blocks at the end of . Replace the orphaned block with rules for the classes actually rendered, following the same anatomy: banner strip, avatar straddling the banner/body seam inside a ring punched from the card background, panel sections, action row. Everything routes through existing tokens, so the card follows the theme contract. Two latent bugs fixed while there: - Placement guessed a 300px card height and clamped only the top edge, so a member clicked low in the list opened a card that ran off the bottom of the window. Measure the card and clamp both edges. - The avatar has to hang off the body's top edge, but the body scrolls, and `overflow-y: auto` clips horizontally too. Make it a child of the card rather than the body. The fade+scale moves from inline styles into CSS so a `prefers-reduced-motion` override can drop it. Co-Authored-By: Claude Opus 5 (1M context) * fix(client): stop vite watching src-tauri `npm run tauri dev` died on Windows partway through the cargo build: Error: EBUSY: resource busy or locked, watch 'src-tauri\target\debug\deps\owncord_client_lib.dll' Error The "beforeDevCommand" terminated with a non-zero status code. Vite's watcher recursed into `src-tauri/target/`, and the moment cargo wrote the output DLL, node's FSWatcher raised EBUSY as an unhandled error event and killed the vite process. Vite is tauri's `beforeDevCommand`, so its death aborted the whole dev session. The config matched the upstream Tauri vite template in every respect except the `server.watch.ignored` block that template ships with. Add it. Tauri already watches `src-tauri` itself for rebuilds, so nothing is lost. Windows-specific — EBUSY on an open handle is a Windows filesystem semantic, and CI only ever runs `tauri build`, never `tauri dev`, so neither caught it. Co-Authored-By: Claude Opus 5 (1M context) * docs(security): add 2026-08-04 whole-codebase security review (#1326) Read-only security review of the full tree (Go server, admin panel, WASM plugin host, LiveKit voice, Tauri client). No code changes. Three findings, all the same defect class — a security predicate enforced at some members of a handler family but not all: - A-2026-08-01 (HIGH) handleDeleteChannelPermission omits the hierarchy and grantability guards its PUT twin carries, so a MANAGE_CHANNELS holder can clear their own role's channel deny and read private channels. - A-2026-08-02 (HIGH) the admin channel list/patch/delete handlers omit the type == "dm" guard their sibling getPermChannel carries, so the same role can enumerate and irreversibly cascade-delete arbitrary DMs and group DMs. - A-2026-08-03 (MEDIUM) DMService.RingTargets omits the block check the five other DM interaction sinks perform, so a blocked user can ring the person who blocked them. Also records one non-vulnerability observation (backup restore writes to a hardcoded database path, silently no-opping when database.path is customised), the candidates rejected during verification, the areas verified clean, and the areas not examined. Claude-Session: https://claude.ai/code/session_01Q7GUJtdsHHHGs4pSiLn6LJ Co-authored-by: Claude * Full audit: docs/spec refresh + remediation (security fixes, dead-code removal, test & CI gaps) (#1327) * docs: fix server reference docs (api, protocol, server-configuration, deployment) api.md: - Correct the login rate limit: 5/min per IP (was documented as 60/min); document the per-username lockout and lockout persistence (Server/api/constants.go, Server/api/auth_handler.go) - Complete the middleware list to the real 9-entry chain incl. the opt-in Coraza WAF (Server/api/router.go) - Add voice_sessions and broadcast_drops to the metrics sample (Server/api/metrics_handler.go) and document the otel-only Prometheus /metrics mount - Add reference sections for the previously undocumented /admin/api endpoints: setup, stats, users, audit-log, settings, tokens, backups, updates, and the SSE log stream (Server/admin/api.go) protocol.md: - Fix type counts (client->server 26, server->client 37) and add the missing rows: call_ring, call_decline, emoji_update, call_incoming, call_declined - Correct rate limits: voice join/leave 5/1s (was "None"), E2EE offer 64/1s (was 5/1s), and add the call-ring limit (1/3s) - Document the plugin command wire types (chat_command, command_reply, plugin_broadcast) and flag that they sit outside protocol-schema.json server-configuration.md: - Add missing keys: server.waf_* (3), database.type, telemetry.otlp_insecure, and the whole logging section + OWNCORD_LOGGING_LEVEL - Correct plugin-disabled status code to 503 (was 501) deployment.md: - Drop the removed "version" field from the /health sample; add broadcast_drops to the metrics sample; note the distroless non-root image; refresh build version strings Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * docs(architecture): rewrite stale architecture pages against 5630aa1 All six pages carried "Verified against ddc49f0 (2026-07-19)" stamps and had drifted: - websocket.md: delete the false claim that docs/protocol-schema.json does not exist — it is the codegen source of truth (Server/scripts/genprotocol, CI-gated by make protocol-verify); note the hand-declared plugin command family as the one exception; refresh LOC - server.md: fix the websocket dependency (github.com/coder/websocket, not nhooyr.io), refresh LOC (42k/71k), migrations 016 -> 028 - data-model.md: migrations 001-028, 23 -> 26 tables, add api_tokens and channel_user_overrides to the ER diagram, channels.type now includes announcement, note 017/024/027/028 columns; drop the claim that schema.md is 6 migrations behind (it is current) - voice-e2ee.md: drop the stale claim that the E2EE flow is absent from protocol.md (it has a full section); document livekitE2EE.ts/identity.ts and identity-key pinning - client.md: rewrite — Solid beachhead is gone; the HTTP path is now TOFU-pinned through http_proxy.rs (the doc claimed the opposite); shared tofu.rs core with explicit-consent pinning; 9 stores (roles store deleted, blocks + emoji added); refreshed LOC and tooling figures - README.md: 26 tables/001-028; client-architecture.md described as the redirect stub it is; companion-audit links refreshed Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * docs(architecture/ux): align UI flow specs with current client behavior - Cert first-use is a blocking trust modal on status "first_use" — the Rust proxy rejects the first connection until the user confirms (main.ts:146-176, tofu.rs); the specs described the pre-F4/F8 behavior (an 8s banner on "trusted_first_use"). Fixed in README.md and connection-and-auth.md. - Dispatcher event table: add the five missing types (chat_bulk_deleted, roles_update, emoji_update, voice_moved, voice_disconnected) and note that call_incoming/call_declined are page-scoped listeners in MainPage.ts. - channels-members-dms.md: the "no in-client block button" gap is closed (AdminActions.ts context-menu item -> SidebarMemberSection.ts:177-186); document group DMs (MemberPickerModal, 10-participant cap, rename/leave) and per-channel notification mutes (lib/channel-mutes.ts); refresh stale line anchors. - voice-and-e2ee.md: document the actual E2EE verification surface (roster shield badge -> identity-mismatch modal -> rePinPeerIdentity with TOCTOU- safe key capture), noise suppression + fallback, device hot-swap, stream preview, and DM ring/incoming-call flow; drop the nonexistent VoiceChannel.ts reference. - settings-and-admin.md: the "ban should collect a reason" gap is closed (appendBanFlow with reason + duration); document the admin-panel deep-link (lib/admin-panel.ts) and the tray status menu. - messaging.md: correct the pinned-messages empty-state copy and drop the nonexistent components/message-input/ directory reference. - Re-stamp all six specs "Verified against 5630aa1 (2026-08-04)". Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * docs: refresh client-facing and top-level docs - security.md: drop the stale "hardcoded Tenor API key" limitation (the GIF provider is Klipy, proxied server-side with an operator-supplied key — nothing ships in the client bundle); describe credential storage accurately (OS keyring primary, verified writes, DPAPI/ChaCha20 file fallback); complete the audit-log action list against the actual WriteAudit call sites and note that backup restore is not audit-logged; fix the firewall checklist to include the LiveKit media ports (7880-7881/TCP, 50000-60000/UDP) and ACME port 80 - credential-storage.md: probe_credential_store sample now shows the real serialized backend value ("Keyring") and the full variant union - quick-start.md + README.md: refresh build version strings to 1.2.0-alpha.1; README "audits" section now points at the current audit documents - contributing.md: sqlc rows no longer claim a PostgreSQL engine/pgdbgen (removed with the store layer); add the protocol-generate/verify targets Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * docs(plans): add verified status headers and fix stale references Every plan under docs/plans/ now carries a dated status verified against 5630aa1: - Shipped: audit-2026-07-19-decisions (all 13 rows), channel-visibility- unification, http-tofu-proxy, permission-middleware-consolidation (the disclosed ws.channelCanSend copy is still open, now at serve_ready.go:119), security-hardening-remediation, sqlc-adoption, v2-dispatch-migration, tauri-capability-narrowing (DNS-rebinding follow-up still open) - Shipped with corrections: discord-parity — Phase 1's gap table was never re-marked; all six rows have since shipped, including archived channels, which are filtered by permissions.VisibleChannelIDs (checker.go:116-121); named leftovers (role hoist/mentionable, @RoleName mentions, categories as entities, dead-code list) stay open. security-scan-2026-07-22 — all 8 findings closed; two of the four F3 follow-ups have since shipped (safety number rendered in the roster badge; rePinPeerIdentity wired to the identity-mismatch modal), getIdentityPin fail-open remains open; noted the scan artifact directory is not in the repo - Design-only: slash-commands — added staleness notes (migration number 016 now taken, Server/store/ deleted, src/state/ never existed) Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * docs(audits): reconcile prior audit statuses with current CI and code - audit-test-coverage-2026-07-25: T-2026-07-25-21 (HIGH, 229/255 web e2e failing) was fixed by the mock repair but the audit was never updated — now RESOLVED, re-verified by a local 270/270 run at 5630aa1; the CI gate table row updated to match - audit-2026-07-19: carried-over item 11 ("no Playwright job in ci.yml") is resolved — client-e2e (non-blocking, every PR) and the blocking client-e2e-parity job both exist; backlog item 10 marked DONE (client-tests is blocking, Playwright wired) Only status/closure cells were edited; original finding text is untouched. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * docs(changelog): add Unreleased section for post-v1.2.0-alpha.1 fixes Three fixes landed after the release with no changelog home (the file had no Unreleased section at all): the profile-popup styling fix (a308f81), the vite/src-tauri watch fix (cdcfc03), and the AppImage env-key signing fix (9d75890). Also corrects the Deferred-work note that still described the Solid.js removal in the present progressive — it completed 2026-07-19. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * docs(tests): rewrite the e2e issues log against a real suite run The old file was dated 2026-03-18, claimed 209/209 passing (the suite is now 270 tests), and pointed at a plan document that does not exist in the repo. Rewritten from an actual run at 5630aa1: 270/270 web tests green (8.6 min), 15/15 @parity subset green (the blocking CI job), with the suite inventory, CI wiring, the two real open issues (three native specs matched by no playwright.config.native.ts project; client-e2e still non-blocking), and dispositions for every claim the old file carried. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * docs(audit): add 2026-08-04 docs-and-coverage audit report Companion to the same-day security review (disjoint scope). Contains: the verified architecture summary; real test-run results for every runnable suite at 5630aa1 (Go race+deadlock, 4394 client unit tests at 94.66% stmt coverage, 83 Rust tests + clippy, 270/270 web e2e, 15/15 parity, browser smoke — with env-blocked suites named and their compensating CI evidence cited); a 52-row UI/UX flow coverage matrix (30 covered / 21 partial / 1 untested / 0 broken, headline gaps: TOFU flow, E2EE verification, admin panel, updater — all unit-only); per-doc drift findings with the commit that fixed each; reconciliation of all four prior audits and eleven plans (including the orphaned 2026-04-07 #8 resurfaced as DC-11); the dead-code and TODO inventories; and a prioritized DC-01..DC-15 gap list with ordered next steps. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * fix(admin): add hierarchy guard to channel role-override delete (A-2026-08-01) Deleting an override is a permission mutation: removing a deny row restores exactly the access the PUT path refuses to grant, so a MANAGE_CHANNELS holder could unlock a private channel their own role was locked out of. Gate DELETE identically to handlePutChannelPermission: resolve the role (404 when missing), fail closed without an actor role, and refuse targets at or above the actor's position. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * fix(admin): exclude DM channels from the admin channel surface (A-2026-08-02) DMs and group DMs share the channels table and id space with guild channels, but they belong to their participants, not to MANAGE_CHANNELS holders: listing exposed ids and group names of every private conversation, PATCH could silently rename one, and DELETE cascade-destroyed one irreversibly. List now filters type=dm; PATCH and DELETE resolve through getAdminChannel, which answers 404 for DM ids so the surface does not confirm which ids are private conversations. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * fix(service): enforce blocks on DM call rings (A-2026-08-03) RingTargets checked participation but not blocks, so a blocked user could still make the blocker's client ring. Route rings through requireDMNotBlocked like every other DM sink; group DMs stay exempt inside it, matching the send path (blocks are enforced at group creation instead). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * chore(client): delete dead modules (DC audit remediation) All verified unreferenced by any import before deletion: - ServerStrip.ts: removed from the layout when the unified sidebar header landed (SidebarArea.ts); only its own orphaned unit test still used it. The e2e spec already asserted .unified-sidebar-header, so it is renamed to sidebar-header.spec.ts and retitled honestly. - FileUpload.ts: uploads go through api.uploadFile from MessageInput. - lib/reconcile.ts: nothing imports it; the messages store carries its own pending-send reconciliation. - public/rnnoise-worklet.ts: unreferenced duplicate of the .js worklet the runtime actually loads, and public/ ships verbatim into the bundle. - api.getSounds/deleteSound + SoundResponse: the server has no /sounds routes; these called endpoints that do not exist (pairs with the sounds-table drop on the server side). - dm.store incrementDmMention: zero callers; DM mention counts flow from the server mention_count via the dispatcher. This was the one live knip error the CI '|| true' was masking. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * chore(client): retire the tauri-typegen ritual (DC-05) src/generated/ was tauri-typegen output frozen on 2026-04-03: it covered 21 of the 29 IPC commands lib.rs registers, nothing ever imported it (0% coverage), and CI carried a bespoke patch step solely to keep the unused file lint-clean. Delete the directory and every part of the pipeline that existed to feed it: the client-check patch step, the tauri-build generate/patch steps, the tauri.conf.json plugin block, and the inert Cargo.toml build-dependency (build.rs is bare tauri_build::build(); no Rust source references the crate). Cargo.lock shrinks by exactly the typegen subtree — no other resolution changes. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * ci: make knip blocking (DC-06 follow-through) Pre-verified green locally after the dead-module deletions; the config hints knip still prints do not affect its exit code. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * chore(server): drop the dead sounds table (A-2026-07-13) The table shipped in 001 for a soundboard that was never built: no query, model, sqlc definition, route or handler ever referenced it. Migration 029 drops it; the sqlc model regenerates without the Sound struct (sqlc emits a struct per schema table even with zero queries). schema.md, the data-model blueprint, and the 2026-07-19 audit closure table are updated in the same change per the docs maintenance rule. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * chore(server): remove dead WAF wrapper, use protocol constants, fix stale comments - NewWAFMiddleware had no production caller (the router mounts the CRS variant); its doc text folds into NewWAFMiddlewareCRS and the tests call the survivor directly. - serve_auth compares against MsgTypeAuth and the DM-close REST path builds its WS notification from MsgTypeDMChannelClose instead of restating the wire strings, so the generated constants are load-bearing again. - Comment fixes: DatabaseConfig no longer claims Postgres scaffolding that main.go removed; host_ui.go no longer advertises a route that is not mounted (DC-09's sibling); buildReady cites docs/protocol.md, the file PROTOCOL.md was renamed to (DC-09). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * fix(protocol): add the plugin command family to protocol-schema.json (DC-01) chat_command, command_reply and plugin_broadcast were the only wire types outside the schema: the first declared by hand in handlers_command.go, the other two raw string literals, all bypassing the protocol-verify codegen gate. Add the three schema entries (27 c2s / 39 s2c), regenerate both constant files, and swap the hand-rolled declarations for the generated constants. The ws protocol-contract test's exception list is empty now — and stays that way. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * test(client): wire orphaned native specs and typecheck the Playwright layer - dm-system, reconnection and theme-persistence (14 tests) matched no project's testMatch in playwright.config.native.ts, so they had never executed (E2E-ISSUES open issue #1 / DC-03). All three use the persistent fixture + ensureLoggedIn, so they join native-authenticated. - tests/e2e was excluded from tsconfig, leaving 47 spec files with no typechecking anywhere. New tsconfig.e2e.json project (+@types/node for the node-API fixtures), a typecheck:e2e script, and a CI step. The one real error it surfaced is fixed: mockTotpFailure omitted the required simulateWsFlow flag. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * test(client): cover createPromptModal and the external-abort close path modalFactory.ts was the least-covered file in the repo (57.6%): createPromptModal had no tests at all and createModal's external-abort branch never ran with an onClose. Now 100% statements/branches/functions, including the trimmed-submit, legitimate-empty-submit, Enter-preventDefault and no-double-close contracts. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * test(client): e2e-cover the TOFU certificate ceremony (DC-04 slice) The first-use confirmation and mismatch warning are the client's core security ceremony and had no e2e coverage. Six tests drive them through the mocked Tauri event layer: first-use modal content, trust, cancel, modal non-stacking, mismatch fingerprint rows, and disconnect-to-connect- page. The mock now exposes its listener registry so tests can wait for the async cert-tofu registration instead of racing it (validated with --repeat-each=3). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * docs: fix the inaccuracies the 2026-08-04 refresh missed - contributing.md: drop the '-tags postgres' build row (no such tag exists anywhere in Server/), add the four Make targets the 07-25 audit created (test/test-deadlock/cover/cover-all), align the coverage statement with the real gates (client 70%, no Go floor by T-2026-07-25-19), point TS style at architecture/client.md instead of the tombstone, and describe the real dev-branch PR flow. - docs/security.md: reporting section now defers to root SECURITY.md as the canonical policy (it said 48h where SECURITY.md promises 7 days, and described the maintainer's advisory path rather than the reporter's); fixed the updater-key link that resolved to docs/Server/... on GitHub. - audit-2026-04-07.md closure table: #10 and #11 were long-resolved (#10 verified in db/audit.go, #11 exceeded by per-PR e2e jobs), #6 written in future tense for work done 2026-07-19, #7 citing a 113-file count from months ago. - README: Contributing section matched neither ci.yml nor contributing.md (branch from dev, not main); Docs Index gains the six missing live docs; the plugin system joins the feature list; the security row no longer anchors to an aging version string. - server-configuration.md: the env-var table is explicitly a subset — the OWNCORD_
_ scheme covers every key. - mcp-introspect.md: index.mjs is 266 lines, not ~230. - types.ts header cited PROTOCOL.md/API.md/SCHEMA.md, filenames that no longer exist. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * ci: pin claude.yml actions by SHA; add docs checkbox to the PR template claude.yml was the only workflow with unpinned third-party actions — checkout now uses the same v4.2.2 SHA the other workflows pin, and claude-code-action pins the commit the v1 tag resolves to (Dependabot's github-actions ecosystem keeps both fresh). The PR template gains the docs checkbox A-2026-07-03 recommended: the architecture/UX maintenance rule ('a PR changing a diagram's source-of- truth files updates the diagram in the same PR') existed only as prose no process step ever surfaced. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * style(client): prettier-format the cert-tofu spec Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * docs(audit): record the remediation pass and close finding statuses - Security review: A-2026-08-01/02/03 -> RESOLVED with their pinning tests named. - Docs-and-coverage audit: DC statuses updated in place (01/02/03/05 resolved, 04/09 partial with the remainder named, 14's keep-decision recorded) and a remediation addendum added: what shipped, the decisions taken (plugin host API kept, reserved protocol entries kept, e2e soak not shortcut, the 404-on-missing-role semantics note), and the full verification table from real runs — Go race + deadlock suites green, 4 tag builds, client 4360/4360 units at 95.35% coverage, Playwright 276/276 in 8.9 min, parity 15/15. - CHANGELOG Unreleased: security fixes, migration 029, protocol additions, dead-code retirement, CI gates. - E2E-ISSUES: rewritten against the remediation HEAD (276/276), native orphan issue moved to resolved, mock listener-registry note. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * fix(admin): index the channel slice in the DM filter (gocritic rangeValCopy) golangci-lint (CI-only gate) flags the range-value copy of the 152-byte db.Channel struct in the admin list filter. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * chore(client): add .nvmrc pinning Node 20 to match CI (DC-10) Also re-triggers CI: the previous run's windows server job died to a Go runtime unwinder fatal ('traceback did not unwind completely') with no test failure — toolchain flake, and the integration lacks permission to rerun failed jobs. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx --------- Co-authored-by: Claude * fix: 26 bug-hunt findings across WS hub, voice/E2EE, admin, roles, and client (runs 1-3) (#1328) * fix(ws): keep pubsub subscriptions when a replaced client is unsubscribed Both pubsub indexes are keyed by userID, but a reconnect registers a new *Client under that same userID. UnsubscribeAll and Unsubscribe deleted by userID alone, so a kick of the already-replaced connection stripped the live one's topics. The live client stays in h.clients and keeps answering ping/pong, so it never reconnects -- it just silently stops receiving every global, user, and channel broadcast. Guard the forward-index delete in unsubscribeLocked with an identity check and route UnsubscribeAll through it, so the four Unsubscribe call sites (voice_leave, hub_broadcast x2, handlers) and the three UnsubscribeAll ones (kickClient, unregisterNow, registerNow) all share one rule. Co-Authored-By: Claude Fable 5 * fix(ws): mark kicked clients offline instead of reporting them replaced Every kick path deletes the hub entry via kickClient, so the readPump defer's unregisterNow finds nothing and fell through to "return true", conflating absent with replaced. serve_pumps.go then skipped MarkUserDisconnected, the offline presence broadcast, and handleVoiceLeave -- already-connected peers rendered every kicked user as online until that user reconnected and disconnected cleanly. Return exists instead: a different client in the slot is a genuine replacement, an absent entry is a real disconnect. Only serve_pumps.go reads the return value; the five serve.go/hub.go call sites discard it. Co-Authored-By: Claude Fable 5 * fix(ws): force a full ready when cold-tier replay hits the row cap GetEventsSinceForChannels is "ORDER BY seq ASC LIMIT n", so a reconnect gap larger than maxColdReplay returned the oldest 5000 rows and dropped the newest. handleReconnect accepted any non-empty result as a successful resume, and the client only tracks max(seq) with no gap detection -- so it accepted the next live event and silently lost the range in between, including state events (channel/role/member changes) that REST history fetches never repair. Treat a result at the cap as overflow and fall through to the full ready re-sync. An exactly-cap-remaining gap pays one unnecessary full ready. maxColdReplay is hoisted to the package const block so the test can seed exactly enough events to hit it. Co-Authored-By: Claude Fable 5 * fix(ws): re-elect the voice E2EE key holder on the two paths that skipped it updateKeyHolder had only two callers (voice_join, voice_leave), so two paths that remove a participant from voice left voiceKeyHolders naming someone who is gone. IsVoiceKeyHolder then rejects the real lowest-uid participant's rekey offers with NOT_KEY_HOLDER -- which the client does not handle -- after it has already applied its rotated key locally, splitting keys across the room. 1. The LiveKit participant_left webhook (media-only loss, WS stays up) cleared voice state and broadcast voice_leave with no re-election. 2. registerNow's fresh-connect replacement (F5 reload) drops the old connection's voice state without transferring it. handleVoiceLeave never runs there: readPump skips it when replaced, and it early-returns on already-cleared state. Both call updateKeyHolder outside h.mu, since it takes keyHolderMu then h.mu.RLock. The recompute reads live client voice state, so it is idempotent and stays correct when a network reconnect transfers voice state -- locked by TestRegisterNow_KeepsKeyHolderWhenVoiceStateTransfers. Co-Authored-By: Claude Fable 5 * fix(client): stand down as E2EE key holder on offer, keep peer keys on reconnect Two independent key-holder desync bugs in E2EEManager: 1. _isKeyHolder had no demotion path -- set at join, promoted on participant leave, cleared only on voice leave. The server re-elects the lowest userID on every join, so a lower-ID joiner left the incumbent still believing it held the key with an armed 5-minute timer. Its rotations applied the new key locally before the server rejected the offers with NOT_KEY_HOLDER (which the client does not handle), so it went deaf and mute every rotation cycle. Accepting an offer proves the sender is the server-authoritative holder, so treat it as the demotion signal and clear the timer. 2. reannounceForReconnect cleared _peerPublicKeys and peer verifications with nothing able to refill them: handleAnnounce replies with an offer rather than a counter-announce, and the server relays stored peer keys only on voice_join, which an SFU-level reconnect never runs. handleOffer's unknown-peer guard then dropped every later rotation, stranding the reconnector on the pre-reconnect key. The clear was also unnecessary -- peers' keys stay valid when we regenerate our own pair. vitest 4396/4396; typecheck and prettier clean. Co-Authored-By: Claude Fable 5 * fix(tauri): give the ws sender slot connection ownership, bound livekit TLS ws_proxy: state.tx is one slot shared by every ws_connect, and both the install and the teardown clear were unconditional while the mutex was only held in short scoped blocks. A handshake pends up to CONNECT_TIMEOUT and a profile switch starts a second ws_connect without awaiting or cancelling the first, so a stale connect could complete after a newer one was live, emit an untagged "open", and install its sender over the live one -- routing the next auth send to the previously-trusted host, then tearing down the live socket and emitting "closed" while JS believed it was connected. Add a generation counter claimed at ws_connect entry and checked under the slot lock before install, plus same_channel ownership on the teardown clear, mirroring the Arc::ptr_eq guard ptt.rs already uses for ATOMICRACE-001. livekit_proxy: the outbound TcpStream::connect and TLS handshake were bare awaits, while the sibling http_proxy.rs bounds both at 10s. TCP connect is OS-bounded, but a peer that accepts TCP and never answers the ClientHello blocked the task forever. The task holds `local` without polling it, so the SDK closing its side never cancels it, and the detached per-connection tasks survive stop_livekit_proxy -- so they leaked on every SDK retry. cargo test 80 passed; clippy --all-targets -D warnings clean. Co-Authored-By: Claude Fable 5 * fix(ws): keep voice E2EE alive across WS resume and unify voice teardown A network reconnect transferred voiceChID/joinToken to the new connection but left it unsubscribed from voice: (the only transport for voice_e2ee_announce relays) and wiped the announced ECDH key, so a resumed key holder could never offer the room key to later joiners and voice_join replayed nothing for the resumed user. registerNow now transfers the announced key with the voice state and re-subscribes VoiceTopic unconditionally (it is CONNECT_VOICE-gated at join; only the message-stream ChannelTopic needs the READ gate). The LiveKit participant_left webhook and CleanupVoiceForChannel cleared voice state without dropping the voice-topic subscription, leaving the socket receiving another room's announces (which carry no channel_id to filter on) for its lifetime. All take-out-of-voice paths now go through one clearVoiceAndUnsubscribe helper. Co-Authored-By: Claude Fable 5 * fix(ws): route sequenced DMs through the normal FIFO writePump drains sendHigh to exhaustion before send, so a seq-stamped DM on the high queue reached the socket ahead of lower-seq events still queued behind a slow write. The client acks max(seq) and replay is strictly seq > last_seq, so a disconnect in that window silently and permanently lost the overtaken events while auth_ok reported a clean resume. Sequenced frames now share the one per-client FIFO; the high queue remains for unsequenced targeted messages (DM opens, voice tokens). Co-Authored-By: Claude Fable 5 * fix(storage): remove the partial file when Save fails after create The io.Copy and f.Sync error paths returned without deleting the file created for the upload, and the orphan sweep is DB-row-driven, so a write-side failure (ENOSPC, disk I/O error) permanently leaked a partial storage/ with no DB row. One success-flag deferred cleanup now covers every failure path (the oversize branch folds into it), fixing all three callers. Co-Authored-By: Claude Fable 5 * fix(client): close three voice-E2EE ordering gaps and wire the DM mention badge - setupKeyExchange generated the room key AFTER draining queued announces, so a key holder joining an ongoing call sent drained peers no offer — they waited on the 5-minute rotation timer. Keygen now precedes the drain and every drained peer gets its offer immediately. - A key-holder re-election arriving while the elected client was still connecting was dropped (getCurrentChannelId is null for the whole key-exchange wait), stranding the client until timeout ejection. The manager now remembers its channel from setupKeyExchange, and the become-holder rotation resolves a pending room-key wait. - Offers applied concurrently could finish out of order (no epoch on the receiver side), leaving the older key active. handleOffer now chains applications so offers apply strictly in WS delivery order. - incrementDmMention had zero callers: the DM @mention badge (dmStore's mentionCount, the mute-immune signal DmSidebar renders) never fired live, only after a reconnect restored the server count. The dispatcher now bumps it under the same guards as the DM unread count. Co-Authored-By: Claude Fable 5 * fix(tauri): restart the LiveKit proxy when the TOFU pin changes After the user accepted a rotated cert, two stale caches kept every voice rejoin tunneling into the old pin until logout: the Rust reuse branch returned the running listener (which bakes its fingerprint in at spawn) without re-reading certs.json, and ensureLiveKitProxy's port cache never invoked Rust again at all. start_livekit_proxy now loads the stored fingerprint before the reuse check and tears down on host OR pin change, and the TS side invokes it on every join — the reuse branch dedups the unchanged case. Co-Authored-By: Claude Fable 5 * chore(tauri): ignore two unreachable cargo-audit advisories RUSTSEC-2024-0429 (glib 0.18, Linux-only, Variant::array_iter_str never called; no semver-compatible fix exists) and RUSTSEC-2026-0097 (rand 0.7 as a phf_generator build-dep with a fixed seed and no log feature; the pre-release kuchikiki pin blocks the upgrade path). Both entries document their drop condition inline. Co-Authored-By: Claude Fable 5 * fix(ws): transfer the focused channel on WS resume so the message stream survives registerNow's replaced-client branch moved voice state and the E2EE key to the resumed connection but not the focused channel; newClient always starts with channelID == 0 and the client never re-sends channel_focus on a resume, so the ChannelTopic re-subscribe was a no-op and the user silently stopped receiving chat_message until manually switching channels. Transfer the old connection's focused channel, READ-gated and fail-closed like every other ChannelTopic subscription. Co-Authored-By: Claude Fable 5 * fix(ws): always include a voice room's participants in its voice-event audience broadcastVoiceEvent filtered recipients on READ_MESSAGES while voice membership is gated on CONNECT_VOICE alone, so a participant in the gap (e.g. READ revoked mid-call by a channel override) never received the room's voice_state/voice_leave. The client's E2EE key-holder election and forward-secrecy rotation run only off the voice_leave WS event, so a departing key holder was never replaced and new joiners hung until the e2ee_timeout eject. Union the READ audience with the room's current participants; what outsiders may observe is unchanged. Co-Authored-By: Claude Fable 5 * fix(client): tear down the failed reconnect attempt's room instead of leaking it The catch block read this._room, whose typed accessor returns null in the "reconnecting" state — so the failed attempt's freshly created Room was never disconnected and kept all its listeners. livekit-client emits Disconnected synchronously on a failed connect, and in "reconnecting" state the token/channel/url getters all return values, so each leaked room spawned an additional concurrent reconnect loop whose AbortController was discarded and unreachable from leaveVoice. Alias the attempt's room outside the try and clean it up in the catch, mirroring cleanupAbortedReconnect. Co-Authored-By: Claude Fable 5 * fix(admin): evict voice participants before deleting a channel CleanupVoiceForChannel was doc-commented 'Called when a channel is deleted' but had zero production callers, and the voice_states FK cascade wipes the rows it reads — so deleting a voice channel stranded its participants with live client voice state, a voice-topic subscription, and a LiveKit session, and the stale sweeper could never recover them (a nonexistent channel resolves base-role permission bits). Wire the cleanup into handleDeleteChannel BEFORE the row delete, via HubBroadcaster. Also harden the cleanup itself: the row delete and client-state clear are now conditional on the participant still being in the deleted channel, so a user who moved rooms mid-cleanup is untouched, and the evicted participants are always included in their own voice_leave audience (their client state is already cleared, so the participant union in broadcastVoiceEvent cannot see them). Co-Authored-By: Claude Fable 5 * fix(client): stop leaking an E2EE worker and SetKey listener per voice join createRoom spun up a fresh E2EE Worker per Room while the key provider lives for the whole process; livekit's per-room E2EEManager registers a SetKey listener on the provider with no matching removal and never terminates the worker. Every join, channel switch, or failed reconnect attempt therefore permanently added one running worker plus one listener, and every later setKey posted the new room key into every orphaned worker — key material outliving its session. Track the worker on the session: clear provider listeners and terminate the stale worker before each Room, and terminate it in leaveVoice so the last key does not stay resident. Co-Authored-By: Claude Fable 5 * fix(service): reject role position collisions on update, matching create UpdateRole's position branch ran only validatePosition and let an explicit position land on a slot another role holds — while CreateRole refuses exactly that, with a comment explaining why: every hierarchy comparison uses >=/<=, so tied positions read as equal rank and the two roles can no longer manage each other's members. Refuse a position held by a different role with the same ErrBadRequest; re-stating the role's own position stays allowed. Co-Authored-By: Claude Fable 5 * fix(ws): close the window where a dying connection re-takes a pubsub topic Subscribe had no counterpart to unsubscribeLocked's identity guard: an old connection's in-flight handler (a channel_focus mid DB round-trip shares no lock with registerNow) could Subscribe after UnsubscribeAll(old) had run, stealing the topic from its replacement — whose own unsubscribes then skip the entry while publishes go to the closed connection. Subscribe now refuses a client whose send is closed (checked under ps.mu), and registerNow closes the old client's send BEFORE stripping it, so a late Subscribe either sees the closed send and is refused or slipped in earlier and is removed by the subsequent UnsubscribeAll. Co-Authored-By: Claude Fable 5 * refactor(ws): rewrite cold-replay if-else chain as switch (gocritic) Fixes the ifElseChain lint failure on CI for both platforms. Co-Authored-By: Claude Fable 5 * fix(client): restore incrementDmMention deleted as dead code on dev The audit PR (#1327) removed it from dm.store.ts because its only caller lives on this branch (the DM mention badge wiring), which was not merged yet. The rebase was textually clean but left dispatcher.ts calling a function that no longer existed. Co-Authored-By: Claude Fable 5 --------- Co-authored-by: Claude Fable 5 * Audit closure pass: DC-04/06/08/09/12/13/15 (E2EE + updater e2e, fail-closed pin lookup, a11y pass, UX polish) (#1329) * ci(server): run the tag-gated wazero/otel Go tests (DC-06) The build-tag matrix only compiled the otel/wazero variants; the tests behind those tags (plugin/sandbox_wazero_test.go 462 lines, telemetry/telemetry_otel_test.go 214 lines) ran nowhere since they were written (T-2026-07-25-16). Scoped to the two packages that carry tagged files; verified green locally before wiring: go test -tags wazero ./plugin/... and -tags otel ./telemetry/... both pass. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * fix(client): fail closed when the identity pin store is unreadable (DC-08) getIdentityPin collapsed a keyring read error into "no pin stored", so a transient failure sent a pinned peer down the TOFU first-sight path: verifyPeerAnnounce verified against the server-delivered key and then RE-PINNED it — a fail-open a malicious server could exploit by inducing store errors (F3 follow-up 3, plans/security-scan-2026-07-22). getIdentityPin now returns a three-state IdentityPinLookup (pinned/unpinned/unavailable), mirroring how tofu.rs keeps Err distinct from Ok(None) first-use. verifyPeerAnnounce rejects the announce on "unavailable" without any pin write, records the new "unknown" PeerVerification status, and the roster badge renders it as an amber shield-question ("could not check") distinct from the legacy "unverified" state. Pinned by unit tests: pin present, no pin, store error (identity.ts), the fail-closed rejection path (livekit-session), and the badge presentation (channel-sidebar). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * feat(admin): write a backup_restore audit row that survives the restore (DC-09) Backup restore was the one admin mutation with no audit_log row — docs/security.md documented the gap as inherent ("the database is closed as part of the restore"). The row IS writable durably: written synchronously (LogAudit, deliberately not the async WriteAudit fast path) before BackupTo takes the pre-restore safety copy, it is captured inside pre_restore_*.db and survives the file swap forensically. The extended restore test opens the pre-restore backup as a database and asserts the backup_restore row is inside it — proving both the write and its ordering. docs/security.md now documents where the row lives instead of the gap. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * feat(client): channel-delete toast + optimistic reaction toggle (DC-12) Two messaging-surface gaps the UX specs carried as open: - channel_delete on the active channel now toasts "This channel was deleted" alongside the existing redirect (ux/channels-members-dms §1.2) — the redirect alone read as the app spontaneously changing channels. Non-active deletions stay silent. - Reactions toggle optimistically (ux/messaging §5): the pill flips on the click, registered under the send's WS envelope id — the same correlation scheme as the optimistic message rows. updateReaction consumes the matching self-echo instead of re-applying it (the delta-based arithmetic would double-count), other users' echoes apply normally, and an error reply or local transport failure rolls back exactly that toggle via rollbackReaction in the dispatcher's error and send-failure handlers. The pill reverting is the failure feedback. Both spec gap notes flipped to implemented; the stale §2 note claiming the ready payload lacks slow_mode fell in the same edit. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * fix(client): guard the role-change submenu against double-fire (DC-12) Every other destructive admin action already carried an in-flight guard (withConfirmation, unblockRunning, banRunning, purge) — the role-change submenu was the residual: currentRole only updates when the member_update echoes, so a double-click (or a second option clicked while the first PATCH was in flight) fired onChangeRole twice. One shared guard now inerts the whole submenu while a change is running, with the pending class on the clicked option. The settings-and-admin spec's in-flight gap note flips to implemented, and the stale messaging §8 slow-mode note is corrected in the same docs sweep (the countdown shipped with the ready payload's slow_mode — verified against ChannelController.startSlowMode and its tests). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * fix(client): drag-reorder document listeners leak — own them per sidebar signal (DC-12) The shared document mousemove/mouseup pair was reference-counted per attached channel row: a sidebar with N channels took N refs (and N more per re-render) while its single destroy returned exactly one, so the count never reached zero and the listeners plus their activeDrag closure lived for the rest of the process — the KNOWN BUG the drag-reorder test pinned since the 2026-07-25 audit. Ownership is now a Set of AbortSignals (the sidebar's lifetime controller — the @lib/disposable teardown idiom): acquisition is idempotent per signal no matter how many rows attach, release is the signal's abort, and an owner aborted mid-drag clears the in-flight visual state (which the old containerEl comparison never actually matched in production — it compared the category container against channelList). releaseGlobalDragListeners is gone; ChannelSidebar's destroy releases via its existing ac.abort(). The pinning test changes with the fix, as its own comment instructed; the lifecycle block now pins the fixed contract, including re-registration after full teardown. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * test(e2e): E2EE identity-verification and updater journeys (DC-04) The two remaining client-side headline coverage gaps from the 2026-08-04 audit's flow matrix (rows 38 and 49), both driven through the web mock harness: voice-e2ee-verify.spec.ts (6 tests, ux/voice-and-e2ee §7): the peer's announce is real crypto — an ECDSA P-256 identity key signing an ECDH ephemeral key exactly as e2eeCrypto does — so the badge states come out of the production verification path. Covers the verified badge with safety number (+ TOFU pin on first sight), the legacy unverified badge, the mismatch block, the mismatch modal's reject path (peer stays blocked, nothing pinned) and Trust New Key (re-pins the displayed key), and the DC-08 fail-closed 'could not check' badge when the pin store is unreadable. The harness gap that kept this untestable is closed by a voice_join handler that grants a key-holder voice_token plus a WebSocket shim that parks LiveKit's room.connect forever, holding the session stably in 'securing'. updater.spec.ts (4 tests, ux/settings-and-admin §5): no-update silence, banner + Later dismissal, the full banner → download progress (% and MB fallback via real update-progress events) → automatic relaunch journey, and the failure state with Dismiss. There is no restart prompt by design — the applied state IS the relaunch, asserted via the recorded plugin:process|restart invoke. Harness: buildTauriMockScript gains per-test identity-pin config (identityPins / identityPinError) and a window.__invokeLog recording every IPC call so tests can assert side effects with no DOM footprint. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * style(client): prettier-format the drag-reorder module and new e2e specs Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * feat(client): accessibility pass over the modal/overlay stack (DC-13) The repo had exactly one focus-trapped dialog (UserProfilePopup), one aria-live region, and no role="dialog" anywhere else. This pass generalizes that one good implementation into lib/a11y.ts (applyDialogSemantics, trapFocus, focusDialog, setRovingTabindex, enableRovingNavigation) and applies it across the stack — all additive: no DOM classes, testids, structure, or visible text changed. - modalFactory: every factory modal now carries role=dialog + aria-modal, moves focus in on open, restores it on every close path, and Tab-cycles inside; createPromptModal is labelled by its title. - Hand-rolled modals (CertMismatch/CertFirstUse/IdentityMismatch, Create/Edit/DeleteChannel, InviteManager): dialog semantics labelled by their existing headings, focus trap + restore, aria-label on icon-only close buttons, and Escape mapped to each modal's SAFE action (reject on the trust prompts — dismissal must never grant trust; cancel on the channel modals — never the destructive/submit callback). - SettingsOverlay: dialog on the panel, focus in on open/restore on close; the sidebar is a vertical role=tablist with roving tabindex and ArrowUp/Down/Home/End activate-on-focus; the content pane is a tabpanel labelled by the active tab. - QuickSwitcher: dialog + combobox/listbox/option wiring with aria-activedescendant tracking the active row. QuickSwitchOverlay: dialog + keyboard-operable rows (the inert current-server row stays unfocusable on purpose). - EmojiPicker/GifPicker: listbox/option cells with a roving tabindex (Arrow/Home/End move the single Tab stop, Enter/Space activate through the click path). inline-autocomplete: option ids + combobox attrs and aria-activedescendant on the composer textarea — deliberately NOT roving tabindex, since moving DOM focus out of the textarea would break typing (the combobox pattern). - Toast and TypingIndicator are polite live regions (role=status). Tests: +71 unit cases across 18 files (4474 total, all green) pinning roles, traps, restores, Escape safety, and roving behavior; plus an axe-style e2e smoke (a11y-smoke.spec.ts, 5 tests) proving the wiring in the running app — settings tablist + focus restore to the opener, quick switcher combobox, member-picker Tab containment, live regions, and the cert first-use dialog where Escape rejects without trusting. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * docs(ux): replace file:line anchors with symbol references (DC-15) The UX specs cited code as file:line anchors, and a three-week-old snapshot already had 15 of 58 pointing at entirely wrong code (the audit measured 200-700 lines of drift). All 55 remaining anchors across the six spec files now reference the owning symbol instead ("validateForm() in pages/connect-page/LoginForm.ts"), each target verified to exist before rewriting; the stale ones were re-aimed at the correct symbol, not just de-numbered. Zero file:line references remain under docs/architecture/ux/. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * docs(audit): record the 2026-08-05 closure pass; stamp DC statuses in place - audit-2026-08-04-docs-and-coverage.md: DC-06/08/12/13/15 marked resolved, DC-04 and DC-09 further-resolved (admin-panel journey and the handleApplyUpdate TODO are the remainders), matrix rows 38/49 flipped from headline gaps to covered, the §4 UX-problem bullets closed, and a §12 closure addendum records what shipped and the verification runs. - CHANGELOG Unreleased: operator-facing entries for the DC-08 fail-closed fix, the a11y pass, the UX polish, the backup_restore audit row, the tag-gated CI tests, and the new e2e journeys. - E2E-ISSUES.md: fresh full-suite run recorded at this HEAD — 291/291 passed in 9.2 min with zero flaky retries (276 baseline + 6 E2EE + 4 updater + 5 a11y smoke), @parity 15/15; suite inventory now 40 files. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * test(client): fix install-settle race in the updater e2e spec The install settle handles are created only when the app's download_and_install_update invoke reaches the mock wrapper, but the spec called them right after asserting the banner text — which flips synchronously on click, before the invoke's microtask runs. Local runners won that race; CI lost it three attempts in a row (window.__rejectInstall is not a function). Both settle sites now wait for the handles, same pattern as the listener waits the file already uses. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx --------- Co-authored-by: Claude * Final audit closure: admin-panel e2e, container-safe updates, blocking e2e gate, dependency policy (#1330) * ci(server): run the tag-gated wazero/otel Go tests (DC-06) The build-tag matrix only compiled the otel/wazero variants; the tests behind those tags (plugin/sandbox_wazero_test.go 462 lines, telemetry/telemetry_otel_test.go 214 lines) ran nowhere since they were written (T-2026-07-25-16). Scoped to the two packages that carry tagged files; verified green locally before wiring: go test -tags wazero ./plugin/... and -tags otel ./telemetry/... both pass. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * fix(client): fail closed when the identity pin store is unreadable (DC-08) getIdentityPin collapsed a keyring read error into "no pin stored", so a transient failure sent a pinned peer down the TOFU first-sight path: verifyPeerAnnounce verified against the server-delivered key and then RE-PINNED it — a fail-open a malicious server could exploit by inducing store errors (F3 follow-up 3, plans/security-scan-2026-07-22). getIdentityPin now returns a three-state IdentityPinLookup (pinned/unpinned/unavailable), mirroring how tofu.rs keeps Err distinct from Ok(None) first-use. verifyPeerAnnounce rejects the announce on "unavailable" without any pin write, records the new "unknown" PeerVerification status, and the roster badge renders it as an amber shield-question ("could not check") distinct from the legacy "unverified" state. Pinned by unit tests: pin present, no pin, store error (identity.ts), the fail-closed rejection path (livekit-session), and the badge presentation (channel-sidebar). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * feat(admin): write a backup_restore audit row that survives the restore (DC-09) Backup restore was the one admin mutation with no audit_log row — docs/security.md documented the gap as inherent ("the database is closed as part of the restore"). The row IS writable durably: written synchronously (LogAudit, deliberately not the async WriteAudit fast path) before BackupTo takes the pre-restore safety copy, it is captured inside pre_restore_*.db and survives the file swap forensically. The extended restore test opens the pre-restore backup as a database and asserts the backup_restore row is inside it — proving both the write and its ordering. docs/security.md now documents where the row lives instead of the gap. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * feat(client): channel-delete toast + optimistic reaction toggle (DC-12) Two messaging-surface gaps the UX specs carried as open: - channel_delete on the active channel now toasts "This channel was deleted" alongside the existing redirect (ux/channels-members-dms §1.2) — the redirect alone read as the app spontaneously changing channels. Non-active deletions stay silent. - Reactions toggle optimistically (ux/messaging §5): the pill flips on the click, registered under the send's WS envelope id — the same correlation scheme as the optimistic message rows. updateReaction consumes the matching self-echo instead of re-applying it (the delta-based arithmetic would double-count), other users' echoes apply normally, and an error reply or local transport failure rolls back exactly that toggle via rollbackReaction in the dispatcher's error and send-failure handlers. The pill reverting is the failure feedback. Both spec gap notes flipped to implemented; the stale §2 note claiming the ready payload lacks slow_mode fell in the same edit. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * fix(client): guard the role-change submenu against double-fire (DC-12) Every other destructive admin action already carried an in-flight guard (withConfirmation, unblockRunning, banRunning, purge) — the role-change submenu was the residual: currentRole only updates when the member_update echoes, so a double-click (or a second option clicked while the first PATCH was in flight) fired onChangeRole twice. One shared guard now inerts the whole submenu while a change is running, with the pending class on the clicked option. The settings-and-admin spec's in-flight gap note flips to implemented, and the stale messaging §8 slow-mode note is corrected in the same docs sweep (the countdown shipped with the ready payload's slow_mode — verified against ChannelController.startSlowMode and its tests). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * fix(client): drag-reorder document listeners leak — own them per sidebar signal (DC-12) The shared document mousemove/mouseup pair was reference-counted per attached channel row: a sidebar with N channels took N refs (and N more per re-render) while its single destroy returned exactly one, so the count never reached zero and the listeners plus their activeDrag closure lived for the rest of the process — the KNOWN BUG the drag-reorder test pinned since the 2026-07-25 audit. Ownership is now a Set of AbortSignals (the sidebar's lifetime controller — the @lib/disposable teardown idiom): acquisition is idempotent per signal no matter how many rows attach, release is the signal's abort, and an owner aborted mid-drag clears the in-flight visual state (which the old containerEl comparison never actually matched in production — it compared the category container against channelList). releaseGlobalDragListeners is gone; ChannelSidebar's destroy releases via its existing ac.abort(). The pinning test changes with the fix, as its own comment instructed; the lifecycle block now pins the fixed contract, including re-registration after full teardown. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * test(e2e): E2EE identity-verification and updater journeys (DC-04) The two remaining client-side headline coverage gaps from the 2026-08-04 audit's flow matrix (rows 38 and 49), both driven through the web mock harness: voice-e2ee-verify.spec.ts (6 tests, ux/voice-and-e2ee §7): the peer's announce is real crypto — an ECDSA P-256 identity key signing an ECDH ephemeral key exactly as e2eeCrypto does — so the badge states come out of the production verification path. Covers the verified badge with safety number (+ TOFU pin on first sight), the legacy unverified badge, the mismatch block, the mismatch modal's reject path (peer stays blocked, nothing pinned) and Trust New Key (re-pins the displayed key), and the DC-08 fail-closed 'could not check' badge when the pin store is unreadable. The harness gap that kept this untestable is closed by a voice_join handler that grants a key-holder voice_token plus a WebSocket shim that parks LiveKit's room.connect forever, holding the session stably in 'securing'. updater.spec.ts (4 tests, ux/settings-and-admin §5): no-update silence, banner + Later dismissal, the full banner → download progress (% and MB fallback via real update-progress events) → automatic relaunch journey, and the failure state with Dismiss. There is no restart prompt by design — the applied state IS the relaunch, asserted via the recorded plugin:process|restart invoke. Harness: buildTauriMockScript gains per-test identity-pin config (identityPins / identityPinError) and a window.__invokeLog recording every IPC call so tests can assert side effects with no DOM footprint. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * style(client): prettier-format the drag-reorder module and new e2e specs Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * feat(client): accessibility pass over the modal/overlay stack (DC-13) The repo had exactly one focus-trapped dialog (UserProfilePopup), one aria-live region, and no role="dialog" anywhere else. This pass generalizes that one good implementation into lib/a11y.ts (applyDialogSemantics, trapFocus, focusDialog, setRovingTabindex, enableRovingNavigation) and applies it across the stack — all additive: no DOM classes, testids, structure, or visible text changed. - modalFactory: every factory modal now carries role=dialog + aria-modal, moves focus in on open, restores it on every close path, and Tab-cycles inside; createPromptModal is labelled by its title. - Hand-rolled modals (CertMismatch/CertFirstUse/IdentityMismatch, Create/Edit/DeleteChannel, InviteManager): dialog semantics labelled by their existing headings, focus trap + restore, aria-label on icon-only close buttons, and Escape mapped to each modal's SAFE action (reject on the trust prompts — dismissal must never grant trust; cancel on the channel modals — never the destructive/submit callback). - SettingsOverlay: dialog on the panel, focus in on open/restore on close; the sidebar is a vertical role=tablist with roving tabindex and ArrowUp/Down/Home/End activate-on-focus; the content pane is a tabpanel labelled by the active tab. - QuickSwitcher: dialog + combobox/listbox/option wiring with aria-activedescendant tracking the active row. QuickSwitchOverlay: dialog + keyboard-operable rows (the inert current-server row stays unfocusable on purpose). - EmojiPicker/GifPicker: listbox/option cells with a roving tabindex (Arrow/Home/End move the single Tab stop, Enter/Space activate through the click path). inline-autocomplete: option ids + combobox attrs and aria-activedescendant on the composer textarea — deliberately NOT roving tabindex, since moving DOM focus out of the textarea would break typing (the combobox pattern). - Toast and TypingIndicator are polite live regions (role=status). Tests: +71 unit cases across 18 files (4474 total, all green) pinning roles, traps, restores, Escape safety, and roving behavior; plus an axe-style e2e smoke (a11y-smoke.spec.ts, 5 tests) proving the wiring in the running app — settings tablist + focus restore to the opener, quick switcher combobox, member-picker Tab containment, live regions, and the cert first-use dialog where Escape rejects without trusting. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * docs(ux): replace file:line anchors with symbol references (DC-15) The UX specs cited code as file:line anchors, and a three-week-old snapshot already had 15 of 58 pointing at entirely wrong code (the audit measured 200-700 lines of drift). All 55 remaining anchors across the six spec files now reference the owning symbol instead ("validateForm() in pages/connect-page/LoginForm.ts"), each target verified to exist before rewriting; the stale ones were re-aimed at the correct symbol, not just de-numbered. Zero file:line references remain under docs/architecture/ux/. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * docs(audit): record the 2026-08-05 closure pass; stamp DC statuses in place - audit-2026-08-04-docs-and-coverage.md: DC-06/08/12/13/15 marked resolved, DC-04 and DC-09 further-resolved (admin-panel journey and the handleApplyUpdate TODO are the remainders), matrix rows 38/49 flipped from headline gaps to covered, the §4 UX-problem bullets closed, and a §12 closure addendum records what shipped and the verification runs. - CHANGELOG Unreleased: operator-facing entries for the DC-08 fail-closed fix, the a11y pass, the UX polish, the backup_restore audit row, the tag-gated CI tests, and the new e2e journeys. - E2E-ISSUES.md: fresh full-suite run recorded at this HEAD — 291/291 passed in 9.2 min with zero flaky retries (276 baseline + 6 E2EE + 4 updater + 5 a11y smoke), @parity 15/15; suite inventory now 40 files. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * test(client): fix install-settle race in the updater e2e spec The install settle handles are created only when the app's download_and_install_update invoke reaches the mock wrapper, but the spec called them right after asserting the banner text — which flips synchronously on click, before the invoke's microtask runs. Local runners won that race; CI lost it three attempts in a row (window.__rejectInstall is not a function). Both settle sites now wait for the handles, same pattern as the listener waits the file already uses. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * ci: promote client-e2e to blocking (DC-07) The soak is decided: green full-suite runs at 270, 276 and 291 tests across the audit branches, and the one hard failure in the window was a real spec bug a non-blocking job would have let rot. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * feat(server): refuse in-place self-update in container deployments Resolves the long-standing handleApplyUpdate TODO. In a container the running binary is image content: the staged replacement dies with the container and the restart comes back as the old image. RunningInContainer (OWNCORD_CONTAINER authoritative both ways — the shipped Dockerfile sets 1, bind-mount operators can set 0 — with /.dockerenv//run/.containerenv as fallback) now gates POST /admin/api/updates/apply with 503 CONTAINER_DEPLOYMENT before any updater logic, GET /admin/api/updates gains can_apply, and the admin SPA swaps the apply button for an image-upgrade note when it is false. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * docs: adopt the dependency pinning/review policy (DC-11, 2026-04-07 #8) Writes down the policy the lockfiles already enforce: lockfiles authoritative with npm ci-only installs, weekly Dependabot with majors adopted deliberately, per-PR security gates (npm audit on shipped deps, govulncheck, cargo audit, knip), and toolchain-level version pins. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * test(admin): add the admin-panel e2e journey against a real server (DC-04) The admin SPA was the one surface no suite could reach: it is served by the Go server and mocked nowhere. start-server.sh builds and boots a real server (fresh temp data dir, TLS off, loopback) and the journey drives the SPA end to end — first-run wizard creating the owner, dashboard stats, channel create/rename, audit-log rows for both mutations, and sign-out/ sign-in. One shared page keeps the localStorage session across the serial steps, mirroring the native suite's persistent fixture and staying under the 5-logins/min limiter; on a Playwright retry the wizard branch downgrades to login since setup is one-shot server-side. New non-blocking admin-e2e CI job on the same graduation convention client-e2e followed. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx * docs(audit): final closure — every DC finding resolved or deliberately reserved Records the owner-directed closure pass (§13): DC-04 fully (admin journey was the last row), DC-07 (client-e2e blocking), DC-09 fully (container- aware update refusal), DC-11 + 2026-04-07 #8 (dependency policy). Remaining open items are all deliberate: DC-14 reserved protocol entries, the admin-e2e soak graduation, and the accepted/tracked 2026-04 carryovers. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KDaeRAN79nVdgtNX2zJVrx --------- Co-authored-by: Claude * fix(attachments): stop the orphan sweep destroying every avatar and the grace period The 15-minute maintenance sweep deleted attachment rows and their files for any attachment with message_id IS NULL. Avatars are exactly that by design: users.avatar points at the attachment by URL and nothing ever links it to a message (migration 027). Every avatar in the instance was therefore destroyed on the first tick past the grace period, permanently 404ing every profile picture. The query now excludes attachments a user's avatar still points at. Independently, the cutoff was formatted RFC3339 while uploaded_at is written by SQLite as 'YYYY-MM-DD HH:MM:SS'. TEXT comparison is bytewise and ' ' sorts before 'T', so every unlinked upload sharing the cutoff's UTC date was swept regardless of time -- the one-hour grace collapsed to 'immediately'. Rather than fix the format at the one call site, DeleteOrphanedAttachments now takes a time.Time and formats it internally, so no caller can reintroduce the class. Co-Authored-By: Claude Fable 5 * fix(client): send old_password so changing a password can succeed The client posted {current_password, new_password} while the server decodes json:"old_password" (Server/api/profile_handler.go:43). Go's encoding/json does no alias matching, so OldPassword was always empty and every password change returned 400 INVALID_INPUT -- the feature could never work for anyone. docs/api.md and every server test already document old_password. The existing unit test asserted the client's own broken payload, so it passed while the feature was dead; it now asserts the documented server contract. Co-Authored-By: Claude Fable 5 * fix(admin): roll back and restart when a backup restore fails mid-copy copyFile truncates the destination with os.Create before it can know whether the read will succeed. On the restore path the destination is the live database, already closed, so a failure in io.Copy or Sync left a zero-byte chatserver.db, no rollback, and -- because the old code returned before the restart -- a process still answering requests against a closed DB while the response and the server_restart broadcast both claimed a restart was underway. The failure branch now puts the pre-restore safety copy back (saying so honestly in the error, including when the rollback itself fails) and requests the restart the success path already did. Co-Authored-By: Claude Fable 5 * fix(voice): refuse client-side unmute while server-muted by a moderator Push-to-talk called LiveKitSession.setMuted directly, which had no server-mute guard -- only the voice widget's own handler checked. Unmuting re-publishes a fresh mic track, and since MuteParticipantAudio only mutes the track SIDs that exist at mute time while the LiveKit grant still carries the microphone publish source, the SFU accepted it: holding PTT lifted a moderator's mute and never told the server. The guard now lives in setMuted itself, the one entry point every caller shares, so PTT and any future caller are covered. Muting stays allowed. Co-Authored-By: Claude Fable 5 * fix(ws): force a full ready when a client's seq is ahead of the ring buffer EventsSince/EventsSinceFiltered guarded only the lower bound, so a client asking for events newer than anything the buffer ever held got a non-nil empty slice -- which handleReconnect reads as a successful, complete replay. It then registers the client, sends auth_ok with replay_source=buffer and skips ready entirely, leaving stale members, channels and read state until the counter climbs back past the client's remembered value. That disagreement is reachable in normal operation: the hub seeds its counter from GetMaxEventSeq, which is 0 once the 24h pruner has emptied the table, so a restart can reseed seq below a lastSeq clients preserve across reconnects. Both functions now return nil (the existing 'cannot guarantee coverage' signal) when afterSeq exceeds the newest buffered seq, so the caller falls through to the cold tier and the intended full ready. afterSeq == newestSeq remains the legitimate caught-up case and still replays empty. Co-Authored-By: Claude Fable 5 * fix(client): re-send channel_focus on auth_ok so reconnects keep receiving messages channel_focus was sent only by mountChannel, which early-returns when the channel id is unchanged, so a reconnect into the same channel never re-sent it. The server transfers the focused channel from the old connection, but only while that connection is still registered -- readPump's defer unregisters it and drops every topic subscription the moment the server observes the close, about a second before the client's first retry. Any server-observed close (restart, proxy close, network reset) therefore resumed with no ChannelTopic subscription: server channel messages, edits and reactions are delivered exclusively over that topic, so the message stream went silently dead while global events kept arriving and made the connection look healthy. auth_ok fires on every connection including resumes and the full-ready fallback, and it also covers the server-restart case where there is no old state to transfer from. The server's handler is idempotent, so the extra focus on a fresh connect is harmless. Co-Authored-By: Claude Fable 5 * fix(messages): stop persisting HTML-escaped text, safely bluemonday writes text tokens through html.EscapeString, so sanitizeContent persisted and broadcast the escaped form: every apostrophe, quote, ampersand and angle bracket reached other users as a literal entity, and because stored quote lines began with '>' the client's blockquote regex could never fire. cleanText (display names, about, custom status, DM names) had the same bug. Unescaping bluemonday's output alone would be a sanitizer bypass: surviving text tokens can recombine into live markup -- '</script>' reassembles a real end tag. Instead the whole unescape -> Sanitize -> unescape cycle now runs to a fixpoint, so the stored result is by construction stable under re-sanitizing: any '<' that the tokenizer would read as a tag start is stripped rather than re-encoded, and only inert punctuation survives. The loop is bounded by the input length and each pass is non-increasing; measured worst case over pathological tag/entity soup at the 16 KiB input ceiling is under a millisecond. The fuzz sinks are tightened to match the new contract rather than loosened: they now require a tag-like start ('<' + letter or '/') because a bare '<' followed by punctuation is inert plain text under every client render path. The /script>" down to escaped text +// "</script>", and one outer unescape turns that into the literal +// substring "" — inert as *this* pass's output, but a real end +// tag if it were ever sanitized again. Looping to a fixpoint here means +// sanitizeContent's own output is always already stable, so re-running +// it (which the edit path effectively does, on separately-submitted +// content) is a true no-op instead of merely "safe once". +// +// It always terminates: bluemonday only ever removes characters or shortens +// escaped entities back down, so each pass's output length is +// non-increasing. The iteration bound is a defensive backstop pathological +// input can't actually reach, not what makes this safe. +func sanitizeToFixpoint(raw string) string { + s := raw + for i := 0; i <= len(raw); i++ { + next := sanitizePass(s) + if next == s { + return next + } + s = next + } + return s +} + // sanitizeContent validates and sanitizes message content. func sanitizeContent(raw string, allowEmpty bool) (string, error) { if len(raw) > maxMessageLen*4 { return "", fmt.Errorf("%w: message content exceeds maximum length", ErrBadRequest) } - content := sanitizer.Sanitize(raw) + content := sanitizeToFixpoint(raw) if content == "" && !allowEmpty { return "", fmt.Errorf("%w: message content cannot be empty", ErrBadRequest) } diff --git a/Server/service/message_crud.go b/Server/service/message_crud.go index 67281fb0..f9076508 100644 --- a/Server/service/message_crud.go +++ b/Server/service/message_crud.go @@ -45,6 +45,16 @@ 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 { return nil, err @@ -106,6 +116,19 @@ func (s *MessageService) SendMessage(ctx context.Context, p SendMessageParams) ( slog.Warn("MessageService.SendMessage: skipped attachments (not owned, already linked, or missing)", "msg_id", msgID, "user_id", p.UserID, "requested", len(p.AttachmentIDs), "linked", linked) } + if linked == 0 && content == "" { + // sanitizeContent waived the empty-content check purely on the + // requested attachment count, before any link attempt. None of + // them actually linked (all missing, foreign, or already + // linked — e.g. a retry of a partially-completed send), so the + // row that just committed has no content and no attachments. + // Compensate the same way the linkErr path above does, rather + // than broadcasting a blank message. + if delErr := s.st.DeleteMessage(context.WithoutCancel(ctx), msgID, p.UserID, true); delErr != nil { + slog.Error("MessageService.SendMessage DeleteMessage (empty-after-link cleanup)", "err", delErr, "msg_id", msgID) + } + return nil, fmt.Errorf("%w: message content cannot be empty", ErrBadRequest) + } if linked > 0 { attMap, attErr := s.st.GetAttachmentsByMessageIDs(ctx, []int64{msgID}) if attErr != nil { @@ -116,6 +139,25 @@ func (s *MessageService) SendMessage(ctx context.Context, p SendMessageParams) ( } } + // Advance the author's own read state past the message they just sent. + // Both unread queries count "messages with id > my read_states row" and + // neither filters by author, so without this an author's own message + // counts as unread to themselves: post in a channel, navigate away, and + // the next `ready` restates it as an unread badge that never clears until + // something else marks the channel read. + // + // Done here rather than by adding an author filter to the two queries so + // the stored read state stays truthful — you have, in fact, seen your own + // message — and so the fix covers DMs and text channels through one path. + // + // Best-effort: the message is already committed and broadcast-bound, so a + // failure here must not fail the send. The worst case is the pre-existing + // stale-badge behaviour, which the next mark_read corrects. + if err := s.st.UpdateReadState(ctx, p.UserID, p.ChannelID, msgID); err != nil { + slog.Warn("MessageService.SendMessage: could not advance author read state", + "err", err, "user_id", p.UserID, "channel_id", p.ChannelID, "msg_id", msgID) + } + result := &SendMessageResult{ MessageID: msgID, Timestamp: msg.Timestamp, diff --git a/Server/service/message_perms.go b/Server/service/message_perms.go index 9e3a7efc..66640337 100644 --- a/Server/service/message_perms.go +++ b/Server/service/message_perms.go @@ -93,7 +93,8 @@ func (s *MessageService) checkSendPermission(ctx context.Context, userID, channe // of DM channelID have blocked each other in either direction. // // It is the single block-check implementation, called from every DM -// interaction sink — send, edit, react, pin and typing. Enforcing it on the +// interaction sink — send, edit, react, pin, typing and call rings +// (DMService.RingTargets). Enforcing it on the // send path alone left a blocked user an open channel to the blocker: editing // an already-sent message fans MessageEditedDMEvent out to every participant, // so arbitrary new text still reached the person who blocked them, and diff --git a/Server/service/message_test.go b/Server/service/message_test.go index 2cf923f8..18ebccc7 100644 --- a/Server/service/message_test.go +++ b/Server/service/message_test.go @@ -207,6 +207,51 @@ func TestSendMessage_AttachmentOwnershipAtomic(t *testing.T) { } } +func TestSendMessage_EmptyContentAllAttachmentsSkipped(t *testing.T) { + database := newTestDB(t) + seedRole(t, database, &db.Role{ + ID: permissions.MemberRoleID, + Name: "member", + Permissions: permissions.SendMessages | permissions.ReadMessages | permissions.AttachFiles, + Position: 1, + }) + seedUser(t, database, &db.User{ID: 1, Username: "alice", Status: "online"}) + seedUser(t, database, &db.User{ID: 2, Username: "mallory", Status: "online"}) + seedUserRole(t, database, 1, permissions.MemberRoleID) + seedUserRole(t, database, 2, permissions.MemberRoleID) + seedChannel(t, database, &db.Channel{ID: 10, Name: "general", Type: "text"}) + checker := permissions.NewChecker(database) + svc := NewMessageService(database, NewPermissionService(database, checker), nil) + + if err := database.CreateAttachment(context.Background(), "att-foreign", 2, "b.png", "s-b.png", "image/png", 10, nil, nil); err != nil { + t.Fatal(err) + } + + // Empty content is normally rejected, but sanitizeContent waives that + // check whenever attachment ids are requested. Here every requested id + // misses the link UPDATE (foreign owner, plus a nonexistent id), so the + // send must not silently commit and broadcast a blank message. + result, err := svc.SendMessage(context.Background(), SendMessageParams{ + ChannelID: 10, UserID: 1, Username: "alice", RoleName: "member", + Content: "", + AttachmentIDs: []string{"att-foreign", "att-missing"}, + }) + if !errors.Is(err, ErrBadRequest) { + t.Fatalf("err = %v, want ErrBadRequest", err) + } + if result != nil { + t.Fatalf("result = %v, want nil", result) + } + + rows, err := database.GetMessages(context.Background(), 10, 0, 50) + if err != nil { + t.Fatalf("GetMessages: %v", err) + } + if len(rows) != 0 { + t.Fatalf("channel history = %d messages, want 0 (blank message must be compensating-deleted, not visible)", len(rows)) + } +} + func TestSendMessage_EmptyContent(t *testing.T) { svc, _ := newTestMessageService(t) diff --git a/Server/service/profile_fields_test.go b/Server/service/profile_fields_test.go index 0e9f3304..450c6701 100644 --- a/Server/service/profile_fields_test.go +++ b/Server/service/profile_fields_test.go @@ -4,7 +4,10 @@ import ( "context" "errors" "strings" + "sync" + "sync/atomic" "testing" + "time" "github.com/owncord/server/db" "github.com/owncord/server/permissions" @@ -59,6 +62,72 @@ func TestUpdateProfile_SetsAndClearsDisplayNameAndAbout(t *testing.T) { } } +// raceDetectingStore wraps a real Store and records whether two GetUserByID +// calls were ever in flight at once, to prove UpdateProfile's read-merge- +// write serializes per user rather than merely happening to avoid a race +// under a particular timing. +type raceDetectingStore struct { + Store + active int32 + overlap int32 +} + +func (r *raceDetectingStore) GetUserByID(ctx context.Context, id int64) (*db.User, error) { + if atomic.AddInt32(&r.active, 1) > 1 { + atomic.AddInt32(&r.overlap, 1) + } + time.Sleep(5 * time.Millisecond) // widen the window a real race would need + defer atomic.AddInt32(&r.active, -1) + return r.Store.GetUserByID(ctx, id) +} + +func TestUpdateProfile_ConcurrentUpdatesSerializePerUser(t *testing.T) { + database := newTestDB(t) + seedUser(t, database, &db.User{ID: 1, Username: "ada", PasswordHash: "h"}) + rs := &raceDetectingStore{Store: database} + svc := NewUserService(rs) + ctx := context.Background() + + // Simulates PATCH /users/me (sets display_name) racing + // POST /users/me/avatar (sets about, standing in for the avatar column; + // both calls pass the unrelated field's current value the way the real + // handlers do). Without serialization, whichever call's read lands + // between the other's read and write would revert that other call's + // change when it writes its own stale merge. + var wg sync.WaitGroup + wg.Add(2) + go func() { + defer wg.Done() + name := "Ada L." + if _, err := svc.UpdateProfile(ctx, 1, ProfilePatch{Username: "ada", DisplayName: &name}); err != nil { + t.Errorf("UpdateProfile (display_name): %v", err) + } + }() + go func() { + defer wg.Done() + about := "counts on it" + if _, err := svc.UpdateProfile(ctx, 1, ProfilePatch{Username: "ada", About: &about}); err != nil { + t.Errorf("UpdateProfile (about): %v", err) + } + }() + wg.Wait() + + if got := atomic.LoadInt32(&rs.overlap); got != 0 { + t.Errorf("UpdateProfile's read-merge-write overlapped %d times, want 0 (must be serialized per user)", got) + } + + u, err := database.GetUserByID(ctx, 1) + if err != nil { + t.Fatalf("GetUserByID: %v", err) + } + if u.DisplayName == nil || *u.DisplayName != "Ada L." { + t.Errorf("display_name = %v, want %q — must not be reverted by a concurrent update", u.DisplayName, "Ada L.") + } + if u.About == nil || *u.About != "counts on it" { + t.Errorf("about = %v, want %q — must not be reverted by a concurrent update", u.About, "counts on it") + } +} + func TestUpdateProfile_SanitizesAndTrims(t *testing.T) { svc, _ := newUserSvc(t) name := " Ada " diff --git a/Server/service/role.go b/Server/service/role.go index 83e4371e..7eecb4b3 100644 --- a/Server/service/role.go +++ b/Server/service/role.go @@ -6,6 +6,7 @@ import ( "log/slog" "regexp" "strings" + "sync" "github.com/owncord/server/db" "github.com/owncord/server/permissions" @@ -23,6 +24,11 @@ import ( type RoleService struct { st Store perms *PermissionService + // mu serializes the read-check-write mutations: position uniqueness and + // the role cap are enforced against a ListRoles snapshot, not by a DB + // constraint, so two interleaved mutations can both see the same free + // slot. Single-process server — one lock covers every writer. + mu sync.Mutex } // NewRoleService creates a RoleService. @@ -178,6 +184,8 @@ func (s *RoleService) ListRoles(ctx context.Context, actorID int64) ([]RoleWithM // CreateRole creates a role strictly below the actor's own rank. func (s *RoleService) CreateRole(ctx context.Context, actorID int64, in RoleInput) (*db.Role, error) { + s.mu.Lock() + defer s.mu.Unlock() actor, err := s.actorRole(ctx, actorID) if err != nil { return nil, err @@ -265,6 +273,8 @@ func (s *RoleService) CreateRole(ctx context.Context, actorID int64, in RoleInpu // the caller uses that to decide between a cheap roles_update broadcast and a // full visibility re-sync. func (s *RoleService) UpdateRole(ctx context.Context, actorID, roleID int64, in RoleInput) (updated *db.Role, permsChanged bool, err error) { + s.mu.Lock() + defer s.mu.Unlock() actor, err := s.actorRole(ctx, actorID) if err != nil { return nil, false, err @@ -305,6 +315,20 @@ func (s *RoleService) UpdateRole(ctx context.Context, actorID, roleID int64, in if err := validatePosition(actor, position); err != nil { return nil, false, err } + // Positions must stay unique (see CreateRole): tied positions read + // as equal rank in every >=/<= hierarchy comparison. Moving onto a + // slot another role holds is refused; re-stating our own is fine. + if position != role.Position { + existing, err := s.st.ListRoles(ctx) + if err != nil { + return nil, false, fmt.Errorf("%w: failed to list roles: %v", ErrInternal, err) + } + for _, rl := range existing { + if rl.ID != role.ID && rl.Position == position { + return nil, false, fmt.Errorf("%w: position %d is already used by another role", ErrBadRequest, position) + } + } + } } if err := s.st.UpdateRole(ctx, role.ID, name, color, perms, position); err != nil { @@ -329,6 +353,8 @@ func (s *RoleService) UpdateRole(ctx context.Context, actorID, roleID int64, in // default role. Returns the deleted role, the fallback its members landed on, // and the ids of those members so the caller can invalidate and re-sync them. func (s *RoleService) DeleteRole(ctx context.Context, actorID, roleID int64) (deleted, fallback *db.Role, movedUserIDs []int64, err error) { + s.mu.Lock() + defer s.mu.Unlock() actor, err := s.actorRole(ctx, actorID) if err != nil { return nil, nil, nil, err @@ -392,6 +418,8 @@ func (s *RoleService) DeleteRole(ctx context.Context, actorID, roleID int64) (de // the actor (N < actor.Position, enforced by maxRoles), and never collide with // the untouched roles above the actor. func (s *RoleService) ReorderRoles(ctx context.Context, actorID int64, orderedIDs []int64) ([]*db.Role, error) { + s.mu.Lock() + defer s.mu.Unlock() actor, err := s.actorRole(ctx, actorID) if err != nil { return nil, err @@ -445,15 +473,16 @@ func (s *RoleService) ReorderRoles(ctx context.Context, actorID int64, orderedID return updated, nil } -// AffectedUserIDs returns the ids of the users holding roleID. Handlers use it -// to invalidate exactly the permission-cache entries a role edit touches. -func (s *RoleService) AffectedUserIDs(ctx context.Context, roleID int64) []int64 { +// AffectedUserIDs returns the ids of the users holding roleID, and whether the +// lookup succeeded. Handlers use it to invalidate exactly the permission-cache +// entries a role edit touches; on ok=false the caller must fall back to a +// blanket invalidation — a nil list treated as "nobody" silently leaves stale +// masks in place. +func (s *RoleService) AffectedUserIDs(ctx context.Context, roleID int64) ([]int64, bool) { ids, err := s.st.ListUserIDsByRole(ctx, roleID) if err != nil { - // The caller falls back to a blanket invalidation; a partial list here - // would silently leave stale masks in place. slog.Warn("role service: failed to list role members", "role_id", roleID, "err", err) - return nil + return nil, false } - return ids + return ids, true } diff --git a/Server/service/role_test.go b/Server/service/role_test.go index 363aad7d..3d089d23 100644 --- a/Server/service/role_test.go +++ b/Server/service/role_test.go @@ -233,6 +233,30 @@ func TestCreateRole_CannotGrantUnheldBit(t *testing.T) { // ─── Update ────────────────────────────────────────────────────────────────── +// An explicit position already held by ANOTHER role is refused on update just +// as on create: tied positions read as equal rank in every >=/<= hierarchy +// comparison, silently breaking one role's authority over the other's members. +func TestUpdateRole_RejectsExplicitPositionCollision(t *testing.T) { + svc, _ := newRoleCRUDService(t) + + // Owner moves Moderator (60) onto Member's slot (40). + _, _, err := svc.UpdateRole(context.Background(), 1, permissions.ModeratorRoleID, + RoleInput{Position: new(40)}) + if !errors.Is(err, ErrBadRequest) { + t.Fatalf("collision err = %v, want ErrBadRequest", err) + } +} + +// Re-stating a role's own current position is not a collision. +func TestUpdateRole_AllowsKeepingOwnPosition(t *testing.T) { + svc, _ := newRoleCRUDService(t) + + if _, _, err := svc.UpdateRole(context.Background(), 1, permissions.ModeratorRoleID, + RoleInput{Position: new(60)}); err != nil { + t.Fatalf("same-position update: %v", err) + } +} + func TestUpdateRole_PartialBodyLeavesOtherFields(t *testing.T) { svc, _ := newRoleCRUDService(t) @@ -582,12 +606,12 @@ func TestListRoles_CarriesMemberCounts(t *testing.T) { func TestAffectedUserIDs(t *testing.T) { svc, _ := newRoleCRUDService(t) - ids := svc.AffectedUserIDs(context.Background(), permissions.MemberRoleID) - if len(ids) != 2 { - t.Errorf("AffectedUserIDs = %v, want 2 members", ids) + ids, ok := svc.AffectedUserIDs(context.Background(), permissions.MemberRoleID) + if !ok || len(ids) != 2 { + t.Errorf("AffectedUserIDs = %v ok=%v, want 2 members", ids, ok) } - if got := svc.AffectedUserIDs(context.Background(), 9999); len(got) != 0 { - t.Errorf("AffectedUserIDs(missing) = %v, want empty", got) + if got, ok := svc.AffectedUserIDs(context.Background(), 9999); !ok || len(got) != 0 { + t.Errorf("AffectedUserIDs(missing) = %v ok=%v, want empty and ok", got, ok) } } diff --git a/Server/service/sanitize_content_fuzz_test.go b/Server/service/sanitize_content_fuzz_test.go index b9c16d19..8011eae7 100644 --- a/Server/service/sanitize_content_fuzz_test.go +++ b/Server/service/sanitize_content_fuzz_test.go @@ -11,33 +11,42 @@ import ( // living inside an actual surviving tag, case-insensitively, tolerating // whitespace around the '='. // -// The check is deliberately tag-scoped (requires a preceding unclosed '<') -// rather than a bare `\bon\w+\s*=` substring match: bluemonday's -// StrictPolicy strips every tag, so the only way "onerror=" et al. can -// survive into `out` is as inert plain text a user actually typed (e.g. a -// message that reads "use onClick= to bind a handler") — that text renders -// as a text node, not as a live attribute, so it is not an active-content -// sink. A bare substring match flags that benign case as a false positive -// (confirmed via fuzzing: seed "on0=" round-trips unchanged through -// sanitizeContent and is not exploitable). Requiring tag context is what -// actually distinguishes "typed the word onclick=" from "smuggled a live -// onclick attribute". -var onEventAttr = regexp.MustCompile(`(?i)<[^>]*\bon\w+\s*=`) +// The check is deliberately tag-scoped rather than a bare `\bon\w+\s*=` +// substring match, but it is scoped to a *tag-like* start specifically — +// `<` immediately followed by a letter or '/' — not just any `<`. +// sanitizeContent now unescapes bluemonday's output, so a literal '<' CAN +// survive into `out` as plain text a user actually typed (e.g. "5 > 3 && 2 +// < 4" round-trips unchanged). But sanitizeToFixpoint reruns +// unescape-sanitize-unescape until the result stops changing, and a '<' +// followed by a letter (start-tag open) or '/' (end-tag open) is exactly +// the shape bluemonday's tokenizer treats as real markup and strips on the +// next pass — so that adjacency can never be present at a fixpoint. A '<' +// followed by anything else (space, digit, punctuation) isn't a tag +// production at all and is genuinely inert (e.g. a message that reads "use +// onClick= to bind a handler", or "on0=", confirmed via fuzzing to +// round-trip unexploitably) — not a live attribute, so requiring the +// tag-like start is what actually distinguishes "typed the word onclick=" +// from "smuggled a live onclick attribute", now that plain '<' is no longer +// itself proof of nothing dangerous. +var onEventAttr = regexp.MustCompile(`(?i)<[a-z/][^>]*\bon\w+\s*=`) // jsURLInTag matches a javascript: (or similar) pseudo-scheme living inside // an actual surviving tag's attribute — the only shape that is an active -// sink. Like onEventAttr, this is tag-scoped rather than a bare substring -// match: bluemonday's StrictPolicy strips every tag (and HTML-escapes any -// stray '<'), so "javascript:" surviving into `out` at all means it arrived -// as inert plain text (e.g. a message that reads "the demo used -// javascript:void(0) links") — not as a live href. Fuzzing confirmed the -// bare-substring version false-positives on exactly that case (seed -// "jAvAsCript:0"), and the client's own markdown renderer independently -// refuses to autolink a javascript: pseudo-URL (see +// sink. Like onEventAttr, this requires a tag-like start ('<' followed by a +// letter or '/'), not just any '<': since sanitizeContent's outer unescape +// can now leave a literal '<' in inert plain text, a bare '<[^>]*' scope +// would false-positive on typed text like "5 < 10, javascript:void(0)". +// sanitizeToFixpoint's repeated unescape-sanitize-unescape passes guarantee +// a '<' immediately followed by a letter or '/' cannot survive — that shape +// is real markup to bluemonday's tokenizer and gets stripped on the next +// pass — so this pattern only matches the still-impossible live-tag case. +// Fuzzing confirmed the bare-substring version false-positives on plain +// text (seed "jAvAsCript:0"), and the client's own markdown renderer +// independently refuses to autolink a javascript: pseudo-URL (see // tauri-client/tests/unit/content-markdown.test.ts, "does not autolink a // javascript: pseudo-URL"), so plain-text "javascript:" is not exploitable // through any known rendering path. -var jsURLInTag = regexp.MustCompile(`(?i)<[^>]*\bjavascript:`) +var jsURLInTag = regexp.MustCompile(`(?i)<[a-z/][^>]*\bjavascript:`) // FuzzSanitizeContent hammers sanitizeContent with untrusted message content // looking for a case where the "strip everything" bluemonday policy still diff --git a/Server/service/sanitize_content_test.go b/Server/service/sanitize_content_test.go new file mode 100644 index 00000000..e95230cf --- /dev/null +++ b/Server/service/sanitize_content_test.go @@ -0,0 +1,65 @@ +package service + +import ( + "strings" + "testing" +) + +// TestSanitizeContent_PlainTextRoundTrip proves that plain-text punctuation +// survives sanitizeContent unchanged instead of coming back HTML-escaped. +// bluemonday's StrictPolicy writes text tokens through html.EscapeString, so +// without an outer html.UnescapeString, a stored/broadcast message would show +// literal '/>/"/& entities to every client — and a quoted line +// would no longer start with the literal ">" the markdown blockquote regex +// requires. +func TestSanitizeContent_PlainTextRoundTrip(t *testing.T) { + cases := []string{ + `don't > quote "this" & that`, + `a & b`, + `5 > 3 && 2 < 4`, + `> quoted`, + } + for _, in := range cases { + out, err := sanitizeContent(in, false) + if err != nil { + t.Fatalf("sanitizeContent(%q) unexpected error: %v", in, err) + } + if out != in { + t.Fatalf("sanitizeContent(%q) = %q, want unchanged plain text", in, out) + } + } +} + +// TestSanitizeContent_EntitySmugglingBlocked is the security regression test +// for the inner html.UnescapeString: an attacker can encode markup as HTML +// entities so it reaches bluemonday as inert text (which StrictPolicy would +// leave alone), then rely on a naive outer-only unescape to turn it into live +// markup after sanitization. Unescaping BEFORE sanitizing means bluemonday +// sees real markup and strips it, so the smuggled payload must not survive as +// an active