Commit Graph
5 Commits
Author SHA1 Message Date
J3vbandClaude 5202e3fe1e fix: correctness fixes from the 2026-08-20 bug hunt (#1398)
* fix(identity): 2 defect(s) (OC-0192, OC-0197)

OC-0192: bound raw display_name/about/avatar bytes before the quadratic
fixpoint sanitizer runs, in both the REST handler and UserService.UpdateProfile.

OC-0197: sanitize display_name before validateDisplayName so an
HTML-entity-encoded bidi override (e.g. "‮") can no longer pass
validation as ASCII and be decoded into the real character on the way to
storage.

* fix(ws): 1 defect(s) (OC-0196)

A transient DB error during WebSocket auth (session or user lookup) was
collapsed into the terminal auth_error frame, which the client treats as
non-recoverable: it stops reconnecting and clears stored credentials. A
sub-second SQLite hiccup therefore force-logged-out every reconnecting
client with a perfectly valid session. Send a non-terminal INTERNAL error
frame instead so normal backoff/reconnect retries.

* fix(api): 1 defect(s) (OC-0198)

* fix(ws): 1 defect(s) (OC-0200)

normalizeHostForCertCompare now unwraps a bracketed IPv6 literal after the
trailing-":443" strip and before lowercasing, matching tofu::cert_store_key's
normalization order. Without the unwrap, every cert-tofu host equality guard
took the "unrelated host" branch for bracketed-IPv6 servers.

* fix(api): 1 defect(s) (OC-0202)

* fix(admin): 1 defect(s) (OC-0203)

Channel permission override handlers applied requireGrantableOverride only
to the bits being written, so an all-zero PUT or a DELETE could clear a
deny bit the actor's own role does not hold — EffectivePerms =
(rolePerm &^ deny) | allow makes removing a deny an escalation. Both the
role-layer and per-user handlers now check the guard against the bits
already on the row.

* fix(client): 1 defect(s) (OC-0205)

* fix(client): 3 defect(s) (OC-0207, OC-0227, OC-0235)

* fix(client): 1 defect(s) (OC-0208)

* fix(voice): 3 defect(s) (OC-0209, OC-0212, OC-0213)

OC-0209: reject a replayed retired-key announce before verifyPeerAnnounce
runs, so the replay cannot overwrite the peer's displayed verification
status/session fingerprint with the retired key's before being rejected.

OC-0212: buffer an announce blocked as a TOFU pin mismatch and replay it
after a successful rePinPeerIdentity, so re-pinning actually restores the
peer for the live call instead of clearing the badge and leaving them
un-keyed (a mid-call peer never re-announces on its own).

OC-0213: skip retiring a departing peer's key when the local voice roster
still lists them as present — a rejoin announce published straight into
the send queue can overtake the buffered, stale voice_leave, and retiring
a still-live key would reject every later genuine re-announce as a replay.

* fix(ws): 1 defect(s) (OC-0211)

* fix(identity): 1 defect(s) (OC-0214)

The delete-account admin guard counted remaining admins with a raw
`banned = 0` filter, so an admin whose temporary ban had already lapsed
was treated as unusable. Use the shared notBannedClause, appended outside
the Sprintf format string because its strftime verbs (%Y, %H) would
otherwise be parsed as fmt directives.

* fix(client): 1 defect(s) (OC-0215)

* fix(voice): 1 defect(s) (OC-0216)

* fix(client): 1 defect(s) (OC-0217)

* fix(voice): 1 defect(s) (OC-0219)

rollbackVoiceJoin cleared the client's in-memory voiceChID but left its
VoiceTopic subscription in place, so a socket whose join failed after
voiceJoinComplete's Subscribe kept receiving that room's E2EE relays for
the rest of the connection. Use clearVoiceAndUnsubscribe instead, matching
every other path that takes a client out of voice while its WS stays up.

* fix(client): 2 defect(s) (OC-0220, OC-0224)

dmDisplayName: a group DM whose other members have all left keeps a live
is_group row, but the server leaves `recipient` zero-valued, so the empty
username fell through as a blank label. Fall back to a non-empty placeholder.

updateDmLastMessage: a queued chat_message redelivered for an id already
reflected in the `ready` snapshot double-counted the unread badge. Only
increment when the message id advances past lastMessageId.

* fix(client): 1 defect(s) (OC-0221)

Cap queued attachments at the server's 10-attachment limit in the message
composer. Past that the server rejects the whole chat_send frame as a
generic parse error, orphaning already-uploaded attachments; refusing
before the upload starts keeps composer state and the send in sync.

* fix(ws): 1 defect(s) (OC-0222)

handleReconnect built the resume auth_ok before applyConnectStatus settled
c.user.Status, so a resumed client was told its disconnect-time status
(routinely "offline") instead of the status it was coming online as.
Move applyConnectStatus ahead of reconnectWriteReplay, matching
handleFreshConnect's ordering.

* fix(mentions): 1 defect(s) (OC-0223)

* fix(admin): 1 defect(s) (OC-0225)

* fix(client): 1 defect(s) (OC-0226)

* fix(client): 1 defect(s) (OC-0228)

* fix(client): 1 defect(s) (OC-0230)

Route the Logs tab entry counter through renderLogEntries so every render path (filter change, Clear, Refresh, live entry) keeps the count in sync with the list.

* fix(voice): 1 defect(s) (OC-0231)

* fix(client): 1 defect(s) (OC-0232)

Reduce Motion toggle wrote the reduced-motion class directly, fighting the
OS-sync media-query listener that owns it when Sync with OS is on. Route the
side effect through syncOsMotionListener so whichever source owns the class
re-derives it.

* fix(client): 1 defect(s) (OC-0233)

notifyIncomingMessage titled the desktop notification with the raw
payload username, so the popup named the sender differently from the
message row it points at. Resolve the author the same way the message
list does (resolveAuthor over the live membersStore, then
resolveDisplayName).

* fix(client): 1 defect(s) (OC-0234)

* fix(client): 1 defect(s) (OC-0236)

* fix(ws): 1 defect(s) (OC-0237)

* fix(client): 4 defect(s) (OC-0193, OC-0201, OC-0204, OC-0218)

* fix(identity): 1 defect(s) (OC-0195)

Bound free-text profile fields by raw byte length before cleanText's
quadratic sanitizeToFixpoint pass runs, generalizing OC-0192's guard into
cleanTextBounded and applying it to HandlePresenceUpdate's custom_status,
SetCustomStatus, and group DM names.

* fix(dm): 1 defect(s) (OC-0199)

handleCreateDM now broadcasts dm_channel_open to the recipient when a 1:1 DM is newly created, matching handleCreateGroupDM. GetOrCreateDMChannel pre-seeds dm_open_state for both users, so the recipient's later OpenDM reported opened=false and nothing ever told them the DM existed.

* fix(voice): 1 defect(s) (OC-0206)

vad-worklet.js gate timing constants were copied from the setTimeout
fallback's ~16ms poll cadence, but AudioWorkletProcessor.process() runs
once per 128-sample render quantum (~2.667ms at the 48kHz AudioContext).
The mic gate therefore closed ~6x faster than intended (~32ms of silence
instead of ~200ms), with the startup grace and RMS post interval off by
the same factor. Scale the frame counts to render quanta.

* fix(client): 1 defect(s) (OC-0229)

* test(client): assert the real TOFU re-pin outcome and make the pin mock faithful

The e2e journey test asserted that "Trust New Key" makes the peer's verify
badge disappear. That is the behaviour OC-0212 identifies as the defect: a
mid-call peer never re-announces, so clearing the badge left the peer
un-keyed for the rest of the call with nothing on screen. Re-pinning now
replays the announce that was blocked as a mismatch and re-verifies it
against the pin just stored, so assert the peer actually lands verified.

The mock's store_identity_pin was a no-op recorder while get_identity_pin
served a static seed map, so the replayed announce re-read the stale pin and
re-failed — a mismatch the real keyring never produces. Back the pins with a
mutable map so a write is visible to the next read. The unreadable-store
(DC-08) and reject-keeps-blocked paths are unchanged and still pass.

* fix(dm): 1 defect(s) (OC-0194)

Add regression tests pinning the raw-byte bound on group DM names, for
both CreateGroupDM and RenameGroupDM.

The Server/service/dm.go source fix for OC-0194 already landed in
bdbd5ac (fix(identity): 1 defect(s) (OC-0195)), which generalized the
guard into cleanTextBounded and applied it to the group DM name paths
alongside the profile fields. This commit therefore carries the OC-0194
tests only; dm.go is unchanged.

Revert-proof: with dm.go restored to bdbd5ac^ (cleanText before the
rune-count check) both new tests fail — CreateGroupDM returns "recipient
not found" after 222ms and RenameGroupDM accepts the name after 251ms,
against a 150ms budget. With the fix in place both pass in 0.03s.

* fix(ws): 1 defect(s) (OC-0210)

* chore(findings): record the 2026-08-20 hunt's 46 findings as fixed

Appends OC-0192..OC-0237 from the 2026-08-20 converging hunt and marks each
fixed with its commit and the test that pins it. Pre-existing records are
byte-identical; nextId moves 192 -> 238 so the next hunt cannot collide with
these ids.

Every fix was independently revert-proofed: the commit's own source diff is
reverse-applied, its test must go red, and must return green once restored.
43 of 46 carry revertProof "pass" from that mechanical run. Three could not be
checked at file level and were proved by hand at hunk level instead, recorded
as "pass (hand-proved)": OC-0200, whose ws.ts edit no longer reverse-applies
because the merge kept main's equivalent implementation; OC-0215, whose Rust
tests live in-file under #[cfg(test)]; and OC-0194, which stacks on a helper
introduced by an earlier commit. No fix was found to rest on a vacuous test.

OC-0200 additionally carries a note: main fixed that same normalizer
independently while this branch was in flight, so the branch is no longer the
only thing closing it.

* docs: record the dm_channel_open emission on 1:1 DM creation

POST /api/v1/dms now emits dm_channel_open to the recipient when it creates a
channel (it previously emitted nothing on that path), so api.md states it the
way the sibling DM endpoints already state theirs.

The channels/members/DMs UX spec claimed the server broadcast the event "to
both parties" on this flow. That was never true — nothing was broadcast before,
and now only the recipient is sent it; the creator learns the channel from the
response body. This doc lists dispatcher.ts, dm.store.ts, ChannelSidebar.ts,
service/channel.go and dm.go among its sources of truth, all touched here, so
it is corrected in the same change per its maintenance rule.

---------

Co-authored-by: Claude <noreply@anthropic.com>
2026-08-20 20:45:30 +02:00
J3vbandClaude Fable 5 6a26f2a839 fix(server): drain fully before the self-update/restore restart handoff (#1380)
* feat(server): supervisor detection and server.restart_mode config key

RunningUnderSupervisor detects systemd (INVOCATION_ID) and, best-effort,
NSSM (NSSM_SERVICE_NAME — 2.24 does not set it, so NSSM deployments set
the mode explicitly). server.restart_mode (auto|spawn|supervised, default
auto, env OWNCORD_SERVER_RESTART_MODE) selects how a self-restart hands
off after the server drains: exit for the supervisor to relaunch, or
spawn the replacement directly.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ngzj2Rx9UGC35uLHAfErMp

* fix(server): make the self-restart handoff drain fully before starting the successor

The update/restore/wizard restart previously spawned the replacement
while the old server was still serving, then SIGTERMed itself and
hard-exited after 10s. That design failed in every documented deployment
mode: under the shipped systemd unit the spawned child (same cgroup) was
killed when the old main process exited and Restart=on-failure never
relaunched a clean exit; on Windows the self-SIGTERM is unsupported and
silently dropped, so graceful shutdown never ran — hub.GracefulStop (the
only caller of LiveKitProcess.Stop) was skipped, orphaning livekit-server
on TCP 7880/UDP 50000-60000 and dropping queued event/audit batches; and
NSSM's relaunch raced the self-spawned replacement for the database lock.

Admin handlers now perform only the on-disk swap and request a restart
through an injected hook (admin.SetRestartHandoff). The main package's
restart coordinator cancels the parent of run()'s signal.NotifyContext —
the exact drain a SIGTERM triggers, on every platform — and after run()
has fully torn down (listeners closed, hub and LiveKit stopped, queues
flushed, DB closed and its lock released) main() performs the handoff:
spawn the replacement in spawn mode, or exit 0 for the supervisor in
supervised mode. A 90s backstop force-exits a wedged teardown; the
DB-lock and bind retries demote to safety nets.

A three-state guard (idle/busy/restart-pending) serializes update apply,
backup restore, and setup-wizard restarts against each other: concurrent
applies no longer race the same staged .new file or broadcast a spurious
update_aborted, and conflicting requests get 409 UPDATE_IN_PROGRESS /
RESTART_PENDING. The swap being free of process side effects also makes
the apply success path unit-testable for the first time.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ngzj2Rx9UGC35uLHAfErMp

* fix(server): errno-based bind-conflict detection, ACME bind retry, LiveKit Pdeathsig

isAddrInUse now unwraps to the platform errno (EADDRINUSE; WSAEADDRINUSE
10048 on Windows) with the English strings kept only as fallback — the
string-only match never fired on localized Windows, silently disabling
the bind retry. The retry loop is extracted into serveWithBindRetry and
now also covers the ACME :80 challenge server, which previously gave up
on first conflict and stayed dead (breaking HTTP-01 renewals) until the
next restart. The .old-binary boot cleanup retries briefly for the
window where a spawn-mode predecessor has not fully exited. The
companion livekit-server gets Pdeathsig SIGKILL on Linux so a parent
killed without teardown (kill -9, OOM, backstop exit) cannot orphan it
with the voice ports held.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ngzj2Rx9UGC35uLHAfErMp

* docs(deploy): Restart=always unit and per-supervisor restart-mode guidance

Restart=always is what lets the deliberate clean exit after a
self-update/restore relaunch under systemd (systemctl stop is never
auto-restarted; failure exits behave as before). Deployment docs gain
the required NSSM AppEnvironmentExtra line, the Task Scheduler and
Docker restart-policy notes, and the new drain-then-handoff update flow.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ngzj2Rx9UGC35uLHAfErMp

---------

Co-authored-by: Claude <noreply@anthropic.com>
2026-08-16 08:25:40 +02:00
J3vbandClaude Fable 5 f5faf82a60 infra: observability, backups, guardrails, and deployment hardening (#1376)
* docs: add infrastructure roadmap plan

Records the verified recommendations from an infrastructure review in three
tracks: raising the single-instance ceiling, cheap seams for a possible
multi-instance future, and ops hygiene. Includes explicit anti-recommendations
and sequencing. Security-sensitive detail is intentionally excluded per
docs/security.md.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017RtDNHSYWwPKArL8MsRdbj

* feat(server): real health checks and saturation metrics

/api/v1/metrics now exposes signals that were already computed in memory but
never surfaced: reconnect replay tier hits, event-persister counters, SQLite
writer-pool wait stats, aggregate per-client backpressure counters (including
previously invisible low-priority drops), and permission-cache hit/miss.

/health now returns a real verdict: hub dispatch-loop liveness, a bounded
database ping, and a free-disk check, returning 503 with a subsystem reason
when degraded. Checks are cached so the unauthenticated endpoint cannot
amplify load. The hub's panic breaker now exits the process so a supervisor
can restart it, instead of leaving broadcast delivery silently dead while
clients still appear online.

OTel instruments that were declared but never recorded are now wired
(ws_active_connections, ws_broadcast_latency_seconds, ws_messages_total,
ws_events_dropped_total, voice gauges) or removed (db_query_duration_seconds).
Also corrects the docs/api.md description of broadcast_drops, which counts
hub-queue overflow, not client send-queue overflow.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017RtDNHSYWwPKArL8MsRdbj

* feat(server): implement scheduled backups, retention, and backup verification

The backup_schedule and backup_retention settings have existed in the admin
panel and API since the initial schema but were never read by any code. The
15-minute maintenance loop now enforces them: a scheduled backup is taken
when the newest backup on disk is older than the schedule interval (manual
backups reset the clock), and retention prunes backups older than the
configured days while always keeping the newest one.

Backups are now verified with PRAGMA integrity_check immediately after
VACUUM INTO (a failed backup is removed rather than listed as restorable)
and again before a restore may overwrite the live database. A failed VACUUM
INTO also cleans up its partial output file — but never a pre-existing one.

The backup directory is configurable via a new backup.dir key (default
data/backups) so operators can point backups at another disk or an off-host
mount, mirroring the SetDatabasePath plumb.

Restore-handler tests now use real SQLite fixtures (the integrity gate
correctly refuses text files) with the mid-copy failure injected through a
test-only copy hook. Also adds audited gosec suppressions to the Windows
disk-free syscall added in the previous commit, which the Windows lint leg
flagged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017RtDNHSYWwPKArL8MsRdbj

* feat(server): capacity and failure-mode guardrails

- server.max_ws_connections: optional cap on concurrent WebSocket clients,
  checked before the upgrade with a 503 + Retry-After; rejections are counted
  and exposed as ws_conn_rejects in /api/v1/metrics.
- Single-process database lock: an OS-level advisory lock (flock / exclusive
  handle) beside the SQLite file makes a second server process fail fast with
  a clear message instead of silently fighting the first over process-local
  state. A bounded retry covers the self-update/restore restart handoff, and
  the lock mechanism failing (e.g. network filesystems) only warns.
- Disk-space awareness: boot-time warnings for the data and backup volumes,
  plus a disk_free_mb metrics field, via a small cross-platform diskutil
  package (already used by /health).
- Upload storage failures: storage.Save now marks server-side filesystem
  failures with a sentinel (storage.ErrIO); handlers return 507 for those
  instead of blaming the client with a 400, and the emoji route stops echoing
  raw storage errors (which embed absolute paths) into responses.
- Unknown config keys now warn at startup — a typo like admin_alowed_cidrs
  previously kept the default silently while the operator believed the
  setting changed. Never fatal: newer servers tolerate older configs.
- Admin settings honesty: the three stored-but-inert settings (server_icon,
  max_upload_bytes, voice_quality) are shown read-only with a note pointing
  at the real config.yaml keys, instead of pretending to apply.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017RtDNHSYWwPKArL8MsRdbj

* perf(db): write-path efficiency and capacity knobs

- channel_focus/mark_read now skip the read-state UPSERT when the stored row
  already matches (same last_message_id, no mentions) — refocus events fire
  at up to 10/s/user and every no-op write still occupied the single SQLite
  writer connection. The extra existence check runs on the reader pool, which
  doesn't serialize. Same shape as the session-touch throttle.
- DeleteExpiredSessions is now sargable: migration 031 normalizes legacy
  expiry formats to the RFC3339-Z layout the server writes and indexes
  expires_at, replacing the strftime full-table scan that ran on the writer
  every 15 minutes.
- Boot-time ANALYZE runs only when a migration actually applied; unchanged
  schemas get the cheap PRAGMA optimize instead (which also covers
  crash-restarts that never reached the shutdown optimize).
- The read/write SQL router gets a table-driven test with explicit expected
  values (INSERT ... RETURNING must hit the writer despite being :one).
- New knobs, all defaulting to current behavior: database.max_readers,
  security.auth_rate_limit_multiplier (for shared-NAT communities),
  event_persistence.replay_ring_size and replay_cold_limit.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017RtDNHSYWwPKArL8MsRdbj

* fix(server): shutdown lifecycle ordering

- The event pruner and maintenance loop are now joined (bounded) before the
  database closes: bgCtx cancellation used to run AFTER database.Close via
  LIFO defers, contradicting its own comment, and neither goroutine was ever
  waited on — a mid-tick scheduled backup or prune could still hold the
  writer while the pool tore down. StartEventPruner returns a done channel
  with the same join contract EventPersister.Stop already had.
- srv.Shutdown now runs before hub.GracefulStop, so in-flight HTTP handlers'
  broadcasts still reach a live hub and the event persister instead of
  vanishing from the replay/event store across a restart. Shutdown does not
  wait on hijacked WebSocket connections, so the swap adds no delay.
- GracefulStopContext threads the 30s shutdown budget into the hub: the 5s
  client-notice window (matching the countdown clients are shown) ends early
  when the budget expires, and is skipped entirely when nobody is connected —
  early-return startup paths and idle servers no longer sleep 5s for an
  audience of zero.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017RtDNHSYWwPKArL8MsRdbj

* build(deploy): systemd unit, compose hardening, boot-smoked releases, CI polish

- deploy/owncord.service: hardened systemd unit template with the two
  verified caveats encoded (install dir stays writable for self-update under
  ProtectSystem=strict; CAP_NET_BIND_SERVICE for ACME's :80), plus a
  'Linux (systemd)' deployment docs section — the Linux service story was
  previously 'Docker or nothing'.
- New 'Reverse Proxy Topology' docs section with a working nginx snippet and
  the correct signaling-vs-media distinction: /livekit/* is already proxied
  by the server, only WebRTC media ports must be directly reachable.
- docker-compose: log rotation, commented resource limits, and a healthcheck
  backed by a new 'chatserver healthcheck' subcommand (the distroless image
  has no shell) that probes /health without config side effects.
- release.yml: a concurrency group (queue, never cancel), and boot-smoke
  gates — the freshly built server binaries and the Docker image are cold
  booted and probed healthy BEFORE anything is signed or pushed. The release
  feed drives signed self-updates, so a binary that compiles but dies on
  boot previously would have shipped itself to every auto-updating instance.
- ci.yml: client-check/client-tests move to ubuntu with the reasoning
  recorded (no win32 code paths, LF enforced repo-wide); admin-e2e gets a
  written graduation criterion instead of an open-ended non-blocking status.
- docs: Tailscale guide notes the CGNAT range vs the default admin CIDRs;
  architecture overview records presence/voice state as the fifth
  single-instance blocker and the macOS client scope decision.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017RtDNHSYWwPKArL8MsRdbj

* perf(server): measured load tooling, narrowed invalidation, presence coalescing, storage and CIDR seams

- Fix scripts/k6/ws-load.js against the real wire protocol: envelope-wrapped
  frames, correct message types (typing_start, presence_update), the correct
  /api/v1/ws path, and thresholds that fail a run where nobody authenticated
  or went ready — the script had drifted to pre-envelope framing and reported
  100% green while every auth failed on the first frame. A new
  workflow_dispatch-only load-baseline workflow boots a real server, seeds
  users through the setup/invite APIs, runs the script, and uploads the k6
  summary plus a metrics snapshot for before/after comparison.
- Role-scoped channel-override changes now evict only the affected role's
  members from the permission cache (fail-safe: unreadable member list still
  flushes everything). InvalidateAll here repopulated every connected user —
  two reads each — synchronously inside the admin request via
  RefreshChannelVisibility, a stampede that scaled with total population
  rather than the role's size. Same pattern the per-user override endpoints
  already used.
- Connect/disconnect presence broadcasts now pass through a 300ms latest-wins
  coalescer (QueuePresence): each un-coalesced presence change is a sequenced
  global broadcast (an O(clients) fan-out under seqMu), so a reconnect storm
  fired O(users) of them from the connect critical path. A flap inside the
  window collapses to its final state; the wire format, seq ordering, and
  replay behaviour are unchanged, and the delivery path (BroadcastPresence)
  is untouched.
- Storage seam: api handlers now consume a FileStore interface (consumer-side,
  same pattern as service.Store) with Open returning a seekable storage.File —
  writing down the contract (range-request seeks included) an alternative
  backend would have to meet, without building one.
- The metrics surfaces and the LiveKit webhook/health endpoints get their own
  allowlist keys (metrics_allowed_cidrs, livekit_webhook_allowed_cidrs, both
  defaulting to admin_allowed_cidrs), so a central Prometheus scraper or an
  externally-hosted LiveKit no longer requires widening the admin panel's
  perimeter. Startup now also warns when admin_allowed_cidrs is customized
  while trusted_proxies is empty — behind a proxy or container network the
  check would otherwise compare the proxy's private address, not the client's.
- The container healthcheck probe now PINS the server's own certificate from
  disk (VerifyConnection, exact-match) instead of skipping TLS verification,
  addressing the CodeQL finding on the previous commit; WebPKI verification
  is used when no local cert exists (ACME).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017RtDNHSYWwPKArL8MsRdbj

* fix(server): address self-review findings on the hardening branch

Seven fixes from a high-effort review of the full branch diff:

- healthcheck CLI now works under tls.mode acme: it overrides ServerName
  with the configured domain for WebPKI verification instead of pinning a
  cert that doesn't exist (or is stale) in that mode. Previously an ACME
  deployment's container healthcheck failed forever.
- /health pings the READER pool (new db.PingRead): the writer ping queued
  behind a scheduled backup's VACUUM INTO and reported the server degraded
  for the whole backup — which an autoheal watchdog would turn into a
  nightly mid-backup restart.
- /health runs its cached checks under context.WithoutCancel so a probe
  that disconnects mid-request cannot poison the shared cache with a false
  degraded verdict for the next 5 seconds.
- The token CLI uses a new db.OpenShared that skips the single-process
  lock: minting a token against a running server is safe under WAL and was
  a documented workflow the lock had broken.
- The per-user TOTP failure cap is no longer scaled by
  security.auth_rate_limit_multiplier — that knob exists for per-IP limits;
  scaling the only cross-IP brute-force defence multiplied an attacker's
  distributed guess budget. Mirrors the unscaled per-user login threshold.
- A direct presence_update now drops the user's queued entry in the
  connect/disconnect coalescer, so a stale connect-time presence can no
  longer flush 300ms later over the user's fresher chosen status.
- The scheduled-backup filename collision loop breaks on any stat error
  and bounds its suffix probing, instead of spinning the maintenance
  goroutine forever on a persistent EACCES.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017RtDNHSYWwPKArL8MsRdbj

* test(admin): real SQLite fixture for the merged Close-failure restore test

TestHandleRestoreBackup_RestartsWhenCloseFails arrived from main (#1375)
with a plain-text backup fixture; this branch's restore handler verifies
backups with integrity_check before touching the live database, so the text
fixture was (correctly) refused with 400 before the Close-failure branch
under test was reached. Use a real backup via BackupToSafe, matching the
other restore tests.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017RtDNHSYWwPKArL8MsRdbj

---------

Co-authored-by: Claude <noreply@anthropic.com>
2026-08-15 20:50:47 +02:00
J3vbandClaude Opus 5 ea0430c5b0 fix: batch of correctness fixes across server and client (#1375)
* fix(client): 1 defect(s) (OC-0201)

* fix(service): 1 defect(s) (OC-0202)

HandleTyping built the per-user-per-channel rate-limit key before resolving the channel or checking read permission, so forged channel ids could pin unbounded dead entries in the shared process-wide RateLimiter.

* fix(client): 2 defect(s) (OC-0203, OC-0224)

* fix(server): 1 defect(s) (OC-0204)

* fix(ws): 2 defect(s) (OC-0205, OC-0211)

* fix(admin): 2 defect(s) (OC-0209, OC-0212)

* fix(client): 1 defect(s) (OC-0210)

* fix(db): 1 defect(s) (OC-0213)

* fix(ws): 1 defect(s) (OC-0214)

Route handler-driven PresenceEvent through BroadcastToAll instead of BroadcastToAllLow so every source of a user's presence shares one ordered per-client FIFO.

* fix(admin): 1 defect(s) (OC-0215)

PATCH /users/{id} combining banned + role_id committed and broadcast the ban before authorizing the role change, so a refused role change returned an error while leaving the target banned. Authorize the role change up front via the new ModerationService.AuthorizeRoleChange.

* fix(db): 1 defect(s) (OC-0216)

LinkAttachmentsToMessage no longer claims an attachment that is a user's live avatar (users.avatar points at it). Once message_id is set, handleServeFile's avatar branch (gated on ChannelID == nil) is unreachable and the file falls under the message's channel ACL / soft-delete state, permanently disagreeing with users.avatar about who may read it.

* fix(emoji): 1 defect(s) (OC-0217)

* fix(client): 1 defect(s) (OC-0218)

The data-copy phase of an HTTP proxy tunnel was unbounded. Steps 1-2 of
handle_connection (header read, TCP connect, TLS handshake) each run under
a 10s guard, but step 3 called io::copy_bidirectional with no deadline. A
remote that completes the TLS handshake and then neither responds nor
closes parks the spawned connection task, the loopback socket and the
remote TLS session indefinitely: copy_bidirectional only resolves once
BOTH directions finish, so closing the local side alone does not free it.

Wrap the copy in copy_with_deadline, a generic helper bounded by
DATA_PHASE_TIMEOUT (600s). The bound is deliberately far looser than the
10s setup guards because this phase carries the REST body, including
attachment and avatar uploads, so it must reclaim only genuinely stuck
connections rather than merely slow ones. The helper is generic over the
stream types so it can be exercised without a live TLS connection.

Regression test drives two in-memory duplex pairs whose far ends stay
alive, so neither half ever observes EOF and raw copy_bidirectional would
block forever; the test asserts the call resolves on its own deadline with
ErrorKind::TimedOut.

Claude-Session: https://claude.ai/code/session_01ENMDTh8gDLiHCaRFdMYRiL

* fix(ws): 1 defect(s) (OC-0219)

* fix(client): 1 defect(s) (OC-0221)

UpdateNotifier scheduled its deferred update check with a setTimeout whose
handle was never retained, so destroy() could not cancel it. A component torn
down inside the 3s window (page swap / logout) still fired performCheck() and
issued a network update check against the old server URL. Retain the timer
handle and clear it in destroy().

* fix(dm): 1 defect(s) (OC-0222)

* fix(client): 1 defect(s) (OC-0223)

* fix(voice): 1 defect(s) (OC-0225)

The Grant-Microphone retry's .finally hardcoded grantMicBtn.disabled = false, undoing updateFrozen()'s socket-down freeze when the WS socket dropped while the mic permission request was in flight. Delegate the state back to render().

* fix(admin): 1 defect(s) (OC-0226)

handleApplyUpdate broadcasts a 'restarting in 5s' notice before the on-disk
swap. Every failure path in the swap returned silently, leaving clients
counting down to a restart that never happened. Extract the swap into
applyStagedUpdate and send a corrective 'update_aborted' broadcast from a
deferred guard on every path that does not reach the respawn.

* fix(admin): 1 defect(s) (OC-0227)

PATCH /channels/{id} accepted a blank or whitespace-only name, leaving the
channel unidentifiable in clients. updateChannelRequest.validate() now
rejects it the way handleCreateChannel already did.

* fix(identity): 1 defect(s) (OC-0228)

* fix(admin): run deferred cleanup before the update restart exits

The fix batch left three golangci-lint findings and two prettier findings
that CI gates on.

applyStagedUpdate called os.Exit(0) in the same function that defers both
staged.Close() and the corrective "update_aborted" broadcast, so neither
ran (gocritic exitAfterDefer). Return a bool instead and let the caller
exit once those defers have run — on Windows, releasing the staged binary's
file handle is the reason the restart exists at all, so this is a real fix
rather than a lint appeasement. The exported test hook calls the function as
a statement, so the added result does not affect it.

Also modernize a bulk-insert loop to range-over-int, compare backup bytes
with bytes.Equal, and reflow two test files to prettier's output.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ENMDTh8gDLiHCaRFdMYRiL

* test(ws): pin the live presence path against the invisible custom-status leak

OC-0207 and OC-0211 are the same defect at two emitters: hub_broadcast.go's
BroadcastPresence (connect/reconnect) and event.go's presenceEvents (live
presence_update). The fix for OC-0211 closed both sites in one change, but
only the hub_broadcast side got a regression test.

This pins the event.go sibling: an invisible user's real custom status must
be blanked on the PresenceOthersEvent frame while the owner's own
PresenceSelfEvent still carries it. Without it, a later change could reopen
the live path while the committed test kept passing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ENMDTh8gDLiHCaRFdMYRiL

* fix(ws): 1 defect(s) (OC-0206)

* test(ws): silence a contextcheck false positive in the reconnect race test

RefreshChannelVisibility takes no context by design — it is reached through
the admin HubBroadcaster interface, which carries none, so it builds its own
internally. contextcheck flags the call only because the test closure around
it holds a ctx for its override write, so there is nothing to propagate.
Suppress at the call site rather than widen a production interface (and its
mocks) to satisfy a lint in a test.

golangci-lint v2.11.3 (the version ci.yml pins) now reports 0 issues.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ENMDTh8gDLiHCaRFdMYRiL

---------

Co-authored-by: Claude <noreply@anthropic.com>
2026-08-15 16:30:05 +02:00
J3vb db0275a290 fix: batch of 29 correctness fixes across server and client (#1369)
* fix(ws): 2 defect(s) (OC-0013, OC-0140)

* fix(voice): 1 defect(s) (OC-0044)

* fix(ws): 1 defect(s) (OC-0024)

* fix(server): 1 defect(s) (OC-0027)

* fix(ws): 1 defect(s) (OC-0028)

* fix(server): 7 defect(s) (OC-0033, OC-0066, OC-0067, OC-0068, OC-0074, OC-0077, OC-0106)

* fix(ws): 1 defect(s) (OC-0051)

* fix(client): 1 defect(s) (OC-0053)

* fix(client): 1 defect(s) (OC-0055)

* fix(service): 1 defect(s) (OC-0069)

* fix(voice): 1 defect(s) (OC-0072)

* fix(service): 1 defect(s) (OC-0082)

* fix(client): 1 defect(s) (OC-0083)

* fix(plugin): 1 defect(s) (OC-0088)

* fix(plugin): 4 defect(s) (OC-0104, OC-0126, OC-0127, OC-0133)

* fix(admin): 1 defect(s) (OC-0110)

* fix(client): 1 defect(s) (OC-0114)

* fix(api): 1 defect(s) (OC-0139)

* fix(client): 1 defect(s) (OC-0149)

* test(server): adapt existing tests to updated OpenDM and IncrementMentionCounts signatures

* style(plugin): modernize loops and goroutine spawns in race test

* fix(ws): mirror the focus admission gate in the post-subscribe revalidation

* fix(service): detach DM post-commit side effects from the request ctx, fail delete closed, add empty-fan-out fallback

* fix(plugin): preserve enabled intent when upgrade reactivation hits a runtime-less build

* chore(skills): harden bughunt-fix workflow and fold review lessons into bughunt-run/db-change

* Add comprehensive documentation for task-observer skill

- Introduced environments.md to outline activation setup, compaction behavior, and handoff-doc mode.
- Created skill-authoring.md detailing taxonomy, licensing, confidentiality, and editing rules for skill creation.
- Added weekly-review.md for a structured review process of OPEN observations, including scheduled and in-session fallback modes.

* chore(go): pin toolchain go1.26.6 (stdlib CVE fixes flagged by govulncheck)
2026-08-14 10:05:40 +02:00