Fixes all 109 golangci-lint findings (106 contextcheck, 1 gocritic,
2 gosec) that accumulated after D2 wired dbgen (whose queries take ctx)
under ctx-less db.DB wrappers while CI lint was quota-dead. No nolint
comments added; every finding fixed by genuinely threading context.
- db: all 138 hand-written db.DB methods take ctx first; the dbCtx()
Background shim is deleted; raw Query/QueryRow/Exec/Begin use their
Context variants; the four redundant ctx-less passthroughs removed.
db.Auditor/WriteAudit gain ctx.
- Seams: permissions.Checker (DB iface, HasChannelPerm,
RequireChannelAccess) and the service.Store interface mirror the new
signatures (ws.EventStore and plugin.PluginStore already did).
- Callers: api/admin handlers use r.Context(); ws per-message paths use
the connection ctx via DispatchV2; hub loops and startup wiring use
context.Background(); service methods thread ctx where they have one
and Background where no ctx exists. Public service surface reached by
ctx-holding chains (PermissionService.HasChannelPerm/GetRoleForUser/
RequireChannelAccess, message/dm/block/invite/profile methods) is now
ctx-first.
- Detached (context.WithoutCancel) where cancellation would break an
invariant, found by a 3-lens adversarial review of the diff:
* voice-leave background retries (a dead webhook/connection ctx killed
retry 2 before it ran, leaving ghost capacity-holding voice rows)
* rollbackVoiceJoin's compensating delete (its trigger IS the cancel)
* post-2FA-change DeleteOtherSessions and logout DeleteSession (the
security tail of a committed change must not die with the request)
* all api/ws audit writes (a banned user could suppress their own
login_blocked_banned row by aborting the request mid-bcrypt)
* admin backup VACUUM INTO (an interrupt left a truncated .db that
the backup list presented as restorable)
* post-commit message/edit refetches (a committed message must still
fan out when the sender disconnects)
* hub settings-cache refresh (one dead connection could pin stale
values for the 30s TTL)
- gocritic rangeValCopy fixed (index iteration); gosec G306 excluded in
config with justification (generated source must stay world-readable)
instead of flipping genprotocol output to 0o600.
Verified: gofmt/vet, all four build-tag variants, full suite, deadlock
pass, full -race pass, golangci-lint 0 issues uncapped.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Applied from a deadcode (RTA from mains, all build tags) sweep with
per-symbol adversarial verification:
Deleted (nothing but their own self-tests used them):
- admin.Handler (deprecated since Phase 6; production mounts NewHandler)
plus its two self-tests
- ws.Hub.broadcastVoiceStateUpdate + wrapper + two self-tests (pre-V2
leftover; the live voice_state path is the hub voice routines)
- ws.VoiceLeaveEvent + methods ('retained as scaffolding', never
constructed in production; MsgTypeVoiceLeaveBC stays — live via the
leave routine)
- ws.parseIdentity (production calls parseParticipantIdentity directly;
ParseIdentityForTest now exercises the real parser)
- telemetry.Float64 (String/Int64 are used; the float case is covered by
the otel-tagged internal test, re-addable when a caller appears)
Moved into export_test.go so they leave the production binary (all
callers are same-package tests): the eight ws test-client constructors
and voice/E2EE setters from ws/client.go, admin.SetBackupBaseDir
(new admin/export_test.go), api.SecurityHeaders (test-only wrapper;
production uses SecurityHeadersWithTLS — docs/api.md updated to the
real name). Client.getVoiceJoinToken/setVoiceChID inlined into their
existing ForTest wrappers; TestSetVoiceChID_* self-tests deleted.
Kept after verification: updater.SetBaseURL (11 cross-package test call
sites) and telemetry.resetAppMetricsForInit (live under -tags otel —
untagged deadcode false positive).
Full gate green: gofmt/vet, 4 build-tag variants, full suite, deadlock,
race.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
hasChannelPerm resolved permissions from the connect-time role snapshot (c.user.RoleID), so a user reassigned to a lower role kept the old role's voice privileges (CONNECT_VOICE and the SPEAK/VIDEO grants in the LiveKit token) until reconnect. Resolve the current role via GetRoleForUser(c.userID), matching the V2 handlers. (Security scan F5)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
nhooyr.io/websocket now resolves to github.com/nhooyr/websocket-old and its
README is a one-line deprecation pointing at coder/websocket. Its last three
releases (v1.8.15-17) all shipped on 2024-08-10 as the redirect; the fork has
shipped through 2026-06-15.
The version number decreases (v1.8.17 -> v1.8.15) because both paths tagged in
the same space, but the coder release is ~2 years newer. Import path only; the
9 API symbols used are unchanged.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Port the last three V1 message types to typed V2 handlers, then remove
the V1 registry and fallback path so handleMessage has a single dispatch
generation (audit A-2026-07-09 / backlog item 11). Server-internal only —
the envelope wire format is unchanged, no client/protocol edits.
- chat_command: ChatCommandCmd + constructor (empty/args guards) and a
V2 handler returning an ephemeral Reply plus a channel-routed
PluginBroadcastEvent gated by MessageService.CanPost; PluginDeps reads
the registry live (wired post-construction)
- voice_join/voice_leave: V2 handlers gate parse/rate-limit and hand off
to the still-hub-internal handleVoiceJoin/handleVoiceLeave routines via
new Result.JoinVoice / Result.LeaveVoice appliers (those routines are
also called un-throttled on disconnect and channel switch)
- delete HandlerRegistry.handlers/Register/Dispatch/RegisteredTypes/
IsRegisteredV1/hasV2, the MessageHandler type, and the V1-shadowing
guard; NewHub registers only V2
- tests: per-handler V2 tests + a parity guard asserting every command
constructor has a V2 handler and vice versa (locks the migration shut)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Route the four "must mirror" READ_MESSAGES filters — REST
ListVisibleChannels, ws buildReady, reconnect replay
computeAllowedChannels, and hub RefreshChannelVisibility — through a
single permissions.Checker predicate so a drift can never leak a
private channel (audit A-2026-07-07 / backlog item 3).
- add permissions.Checker.VisibleChannelIDs + ChannelRef (skips dm,
fails closed, admin bypass via HasChannelPermBatch)
- delegate the three batch sites; RefreshChannelVisibility uses
HasChannelPerm instead of its inline EffectivePerms copy
- REST/WS agreement test asserting all three sites yield the identical
non-DM set across admin / member-with-deny / denied-everywhere
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Audit writes stay best-effort — a LogAudit failure must never fail or abort
the request — but a failed write must no longer be silently discarded. Add
db.WriteAudit(auditor, actor, action, targetType, targetID, detail), which
logs a failed write with actor/action/target context (never the detail
string, which may be sensitive) and never propagates the error.
The Auditor interface is satisfied structurally by both *db.DB and the
service-layer Store, so api/admin/ws/service all reach the helper without an
import cycle. Converts all ~26 call sites from `_ = LogAudit(...)` (and the
two backup handlers' inline `if err` blocks) to db.WriteAudit. Pinned by
db/audit_test.go: failure logged and not propagated, success logs nothing,
detail never leaks.
Resolves the repo-wide LogAudit policy question flagged by the D8 note.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Implements the two highest-impact gaps from the client UX spec.
Optimistic send:
- messages.store gains addOptimisticMessage / markSendFailed /
removeOptimistic, and confirmSend now stamps the real id + "sent" on
the ack. addMessage reconciles the broadcast by real id (idempotent,
replay-safe) with a defensive author match, so an echo never
duplicates. Message gains status/correlationId/errorCode.
- ChannelController.performSend renders a pending row immediately and
supports retry / delete-draft (retry preserves attachments).
- MessageList renders pending (dimmed) and failed (reason + Retry /
Delete) rows; the hover action bar is limited to confirmed rows.
- Failures are precise: the server echoes the request id on error
replies (buildErrorMsgWithID), so the dispatcher maps SLOW_MODE /
FORBIDDEN / RATE_LIMITED / BAD_REQUEST to the exact row instead of
dropping the code. An offline send is shown failed, not silently lost.
Composer permission + connection gating:
- The server computes an authoritative per-channel can_send in the ready
payload (channelCanSend mirrors MessageService.checkSendPermission:
READ|SEND, MANAGE_MESSAGES for announcement, admin bypass, channel
overrides). channels.store carries it as Channel.canSend.
- MessageInput gains a disabled-with-reason mode; ChannelController
derives the reason from can_send + channel type + connection status and
disables the composer reactively (announcement read-only, no-permission,
reconnecting) rather than accepting a click and failing. Older servers
that omit can_send default permissive.
Docs: the corresponding "Current gap" callouts in docs/architecture/ux
are updated to reflect the implementation.
Verified: full server suite + client tsc + 3204 unit tests + lint + gofmt.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UA17KPvqGBX3XbXYnMf1rA
Deletes Server/store (SQLiteStore, MemStore, the composed Store
interface) and collapses to a single sqlc-backed db package, executing
the prior audit's P4 "single data layer" direction (finding #6).
SQLiteStore was a pure pass-through to *db.DB, so consumers now depend
on narrow interfaces that *db.DB satisfies directly:
- service.Store (service/datastore.go, renamed from store/store.go)
- ws.EventStore (ws/eventstore.go)
- plugin.PluginStore (plugin/pluginstore.go)
The event- and plugin-KV methods that lived in the store's SQLite
implementation move into the db package (db/event_queries.go,
db/plugin_queries.go), keeping their raw-SQL form.
Tests: the MemStore-based unit tests now run against a real in-memory
SQLite db opened per-test with migrations applied, via package-local
seed helpers. Fault-injection tests embed a real *db.DB and override the
single method under test, preserving error-path coverage. Full server
suite and sqlc-verify are green.
Docs: audit finding #6 and A-2026-07-06 marked resolved; decisions D3
updated; architecture server.md / data-model.md diagrams and prose
updated to the api -> service -> db layering.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UA17KPvqGBX3XbXYnMf1rA
Make 'announcement' a real channel type, resolving the contradiction where
it was documented and offered by the admin API but hard-rejected by the
migration-013 DB triggers.
Model: announcement channels are readable like text channels (same
READ_MESSAGES visibility), but posting is restricted to users with
MANAGE_MESSAGES — no new permission bit, migration, or client permission
plumbing needed.
Server:
- migrations/016: recreate the channel-type triggers to allow
text/voice/announcement/dm.
- service/message.go: checkSendPermission now takes the channel type and
rejects posts to announcement channels from users lacking MANAGE_MESSAGES
(SendMessage + CanPost paths). Added a service test.
- Unread counts: ready-payload builder (ws/serve.go) and
GetChannelUnreadCounts (db) now include announcement channels alongside
text, so they track unread/last-message like text channels.
Client:
- ChannelSidebar renders announcement channels with a megaphone icon
(added to the icon set) instead of the '#' text prefix; they otherwise
behave like text channels (already typed in ChannelType).
Specs + trackers (api.md, protocol.md, schema.md incl. migration 016,
architecture/data-model.md, audit A-2026-07-01, decisions D1) updated.
Verified: go build ./...; go test ./service ./db ./ws ./api ./admin;
sqlc-verify; client tsc + oxlint + prettier clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UA17KPvqGBX3XbXYnMf1rA
Protocol codegen (decision D4, audit A-2026-07-08):
- Add docs/protocol-schema.json as the real single source of truth for
WS message-type constants, making the long-standing 'generated from'
comment in both constant files true.
- Add Server/scripts/genprotocol, a generator emitting both
Server/ws/message_types.go and Client .../lib/protocolTypes.ts
(constants byte-for-byte value-identical to before; only headers,
ordering alignment, and provenance comments changed).
- Add make protocol-generate / protocol-verify and wire protocol-verify
into CI next to sqlc-verify.
Quick wins (decision D8):
- admin: log LogAudit write failures in the backup handlers instead of
discarding them (prior audit #10).
- api: fix self-contradictory upload Cache-Control to 'private,
no-cache' per remediation plan W3-4; drop the now-unused
fileCacheMaxAgeSeconds constant; update test.
- ws: route the hub settings cache through db.GetSetting instead of
inline SQL.
- ws: fix a latent data race — main.go wires SetEventPersister and
SetEventStore after NewRouter has already started the hub Run loop,
which reads those fields on the broadcast/replay paths. They (and
pluginSink, which one test sets post-Run) are now atomic pointers;
the remaining pre-Run-only setters reject late calls with an error
log instead of racing silently.
Update the audit closure table and decisions doc statuses accordingly.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UA17KPvqGBX3XbXYnMf1rA
Three review findings on the #93 feature:
- RefreshChannelVisibility targeted clients by their connect-time role
snapshot; a user whose role changed mid-session was evaluated against the
stale role. Resolve the current role from the DB per client (fail closed).
- Visibility updates are targeted, unsequenced messages, so a client that
disconnected before an override change and later resumed via replay never
converged (stale sidebar until a fresh connect). Track a visibility-change
sequence watermark and force resumes from at/before it onto the
full-ready path.
- The admin SPA interpolated channel/user names into single-quoted JS
strings inside onclick attributes with HTML-escaping only; a name
containing a quote broke out of the string literal (XSS in the admin
panel, reachable by any user allowed to create channels). Add a jsq()
helper (JS-escape then HTML-escape) and use it for every onclick name
interpolation.
Follow-up to #93.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LwtnpHAoSFr1ZibQgQkNQK
A zero-byte or whitespace-only livekit.yaml (truncated write, touch(1)
placeholder) has no auto-generated marker and was permanently treated as a
user-managed config, wedging LiveKit startup with an empty config file.
Follow-up to #111.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LwtnpHAoSFr1ZibQgQkNQK
The read side of channel visibility was already complete — channel_overrides
rows deny ReadMessages and every list/focus/send/voice path enforces them —
but nothing could write those rows. Add the missing write path and UI:
- db: UpsertChannelOverride / DeleteChannelOverride / ListChannelRoleOverrides
(roles LEFT JOIN overrides so the UI gets everything in one call)
- admin API: GET/PUT/DELETE /admin/api/channels/{id}/permissions[/{roleId}]
with unknown permission bits masked via the new permissions.AllPerms,
audit logging, and immediate permission-cache invalidation
- ws: Hub.RefreshChannelVisibility sends targeted channel_create /
channel_delete to connected clients after an override change, unsubscribes
hidden clients from the channel topic, and clears their focus. Sent outside
the sequenced replay path on purpose: a replayed channel_delete would be
filtered by the post-change allowed-channel set, inverting its audience.
- admin panel: per-channel Access modal (lock icon) with per-role
"Can access" checkboxes; unchecking writes deny = ReadMessages|ConnectVoice
Known limits (follow-ups): users offline during a revoke keep a stale
sidebar entry until their next fresh connect (server still denies access),
and users already in a voice channel are not kicked when it goes private.
Closes#93
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LwtnpHAoSFr1ZibQgQkNQK
Servers reachable via both a LAN IP and a public IP could only serve voice
on one of them: config.yaml accepts a single voice.node_ip and OwnCord
regenerates data/livekit.yaml on every start, discarding manual edits.
LiveKit has no multi-IP list, but it does support advertising internal host
candidates alongside the external mapping.
- New voice.advertise_internal_ip (OWNCORD_VOICE_ADVERTISE_INTERNAL_IP):
emits rtc.advertise_internal_ip: true so LAN clients get a reachable
candidate while remote clients keep using node_ip.
- livekit.yaml escape hatch: if the file exists without the auto-generated
marker header, OwnCord leaves it untouched, giving operators access to
every LiveKit option (ips.includes, interfaces, stun_servers, ...). The
generated header documents how to take ownership.
Closes#111
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LwtnpHAoSFr1ZibQgQkNQK
Register and Unregister travelled on two separate channels, and Run's
select picks randomly when both are ready: a fast connect/disconnect could
process the unregister first (a silent no-op for a not-yet-known client)
and then the register — admitting an already-dead connection as a ghost
client that held presence and swallowed broadcasts until the stale sweep
reaped it minutes later. One tagged event channel preserves each
connection's Register→Unregister submission order, making the inversion
structurally impossible. Found via TestHub_ConcurrentRegisterUnregister
failing the P1 gate under -race on windows-latest (2 ghosts after churn);
that test now settles in milliseconds instead of polling out its deadline.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
requireChannelBroadcastAccess went through RequireChannelAccess, whose DM
branch checks only participant membership — a blocked user's plugin
broadcast could reach the person who blocked them — and it issued a raw
GetRoleByID per broadcast, bypassing the permission cache. The gate now
delegates to MessageService.CanPost (extracted over checkSendPermission),
so DM blocks, channel permissions, and future posting policy apply from
exactly one place; fails closed when no service is wired. First brick of
the permission-path unification. MemStore.GetDMRecipient gets an honest
implementation so the block path is testable.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Simulates an 8-participant call: two back-to-back full rotations (7 offers
each) must pass the limiter, while same-target spam still trips it.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A rotation is a burst of one offer per peer (join/leave and the periodic
re-key), but the limiter was keyed per sender at 5/sec — in calls with 6+
participants the 6th+ peer's offer was silently rate-limited, that peer
never received the rotated key, and their audio never decrypted again.
Keying per (sender, target) admits any rotation burst regardless of
channel size while still capping repeated offers at a single victim,
which is the abuse the limit exists for (an offer can force the target to
re-key or disconnect).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
readPump teardown always ran handleVoiceLeave, trusting the joined_at
guard in LeaveVoiceChannelIfMatch to protect replacement sessions. On
reconnect the voice session TRANSFERS to the replacement client with
the same joined_at, so the guard cannot tell the two apart: whenever
teardown snapshotted voiceChID before the transfer zeroed it, the old
connection deleted the replacement's voice_state row (flaked on the
Windows CI runner as TestServeWS_Reconnect_PreservesVoiceState).
Gate voice cleanup on !replaced — the same condition the
presence-offline broadcast four lines down already uses. A genuinely
final disconnect behaves exactly as before, and a stale row from a
crashed replacement is still swept by the fresh-connect cleanup.
Also deflake TestHub_ConcurrentRegisterUnregister: poll for quiescence
with a deadline instead of a fixed 50ms sleep that loses to the -race
scheduler on slow runners.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
golangci-lint had been failing invisibly behind the earlier CI gate
failures. Default-build lint is now clean:
- Delete the unused pre-topic-limiter rate-limit constants, the unused
bluemonday sanitizer, and the dead broadcast variants superseded by
their Low/High counterparts (broadcastExclude,
broadcastToDMParticipants(+Exclude), sendSequencedToUsers,
PubSub.debugDump). Test references were comments only; updated to
name the live variants.
- Separate 'Phase X Step Y' file headers from the package clause with a
blank line so staticcheck ST1000 no longer reads them as malformed
package comments (proper package docs exist in hub.go/manifest.go).
- Add .gitattributes normalizing line endings to LF on checkout —
the Windows CI runner materialized CRLF, which made every
prettier-formatted file fail the format gate.
Known remainder (pre-existing, out of P0 scope): golangci-lint with
-tags wazero reports 3 gosec + 2 staticcheck and -tags otel 1+1; CI
lints the default build. Tracked for the P1 plugin pass.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
newEmitTestHub and NewHubForTest built raw Hub literals without the
topic rate limiter, so deliverBroadcast panicked on a nil receiver and
TestEmitEvents_ChannelEvent_CallsBroadcastToChannel could never pass.
CI never surfaced it because the pipeline died at govulncheck first.
Wire the limiter exactly as NewHub does; no production nil-guard, since
a nil limiter in production would silently disable rate limiting.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Applies fixes for 20 adversarially-verified findings from a whole-codebase
security review (server side). All Go build-tag variants build, `go vet` is
clean, and the suite passes (the sole failing test, ws TestEmitEvents, is a
pre-existing nil-harness failure unrelated to these changes).
High severity:
- auth: close TOCTOU in TOTP verify rate-limit by recording each attempt
atomically up-front (was Check-then-Allow), restoring the per-user
brute-force cap.
- plugin: enforce the CPU/time budget on every WASM guest call via a
WithTimeout context (WithCloseOnContextDone interrupts runaways); the
configured budget was previously parsed but never applied.
- api/waf: inspect request bodies for chunked (ContentLength==-1) requests
so the SQLi/XSS/RCE body rules can no longer be bypassed.
- ws: rate-limit voice_join/voice_leave and voice_e2ee announce/offer, which
fan out to every participant and could force mass disconnects.
Medium severity:
- api: run bcrypt on the unknown-user login path (no || short-circuit) to
remove the timing-based username-enumeration oracle.
- ws: verify LiveKit webhooks via the SDK receiver so the signature is bound
to the body hash (kills forgery/replay).
- authz: require READ_MESSAGES for reactions and for plugin-command
broadcasts; route the latter through RequireChannelAccess.
- api: cache the client-update signature fetch and rate-limit the endpoint.
- service: propagate DeleteOtherSessions failure from ChangePassword instead
of silently reporting success.
- api: trust the rightmost non-proxy X-Forwarded-For entry, not the
client-controllable leftmost one.
- plugin: route auto-registered commands through the conflict-checked
RegisterCommand; pin the DNS-validated IP for host_http dials
(DNS-rebinding TOCTOU).
- api: mark access-controlled downloads private/no-cache + Vary: Origin.
Low severity:
- auth: fail closed when a fully-shaped TOTP ciphertext fails GCM auth
(was returning the ciphertext as plaintext).
- api: apply the livekit-proxy path allowlist to WebSocket upgrades too.
- service: verify attachment ownership before linking (IDOR).
- admin: bound the bootstrap setup invite (5 uses / 24h); re-verify the
update binary hash immediately before rename+spawn (TOCTOU).
- service: require BanMembers + role hierarchy for moderation ban/unban.
chore: stop tracking the stray Server/owncord-server.exe build artifact.
Test infra: add uploader_id to the hand-rolled ws test attachment schemas and
make MemStore.GetAttachmentByID a no-op lookup, matching production/DB behavior.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
contextcheck: add ctx context.Context as first param to ListVisibleChannels,
BlockUser, CreateDM, CreateInvite, UpdateProfile, SendMessage; pass r.Context()
from HTTP handlers and ctx from WS handler; replace context.Background() in
telemetry spans with the propagated ctx.
errcheck: suppress justified Close() errors — defer func(){ _ = rows.Close() }()
in sqlite_events.go (idiomatic; rows.Err() checked), _ = resp.Body.Close() in
host_http.go (body fully consumed), _ = f.Close() in host_ui.go (read-only fd).
gocritic/rangeValCopy: rewrite for _, ch := range all (line 60) to indexed loop
in ChannelService.ListVisibleChannels to avoid 144-byte per-iteration copy.
Clean sweep of every actionable item from the two Copilot review passes
on head 59ae4d8. Grouped by severity:
─── Crash / security (must-fix) ─────────────────────────────────────
1. main.go:140 — telemetryShutdown nil panic.
telemetry.Init can return (nil, err) on the -tags otel skeleton
path; the deferred closure would then call a nil function. Normalise
to a no-op shutdown when Init errors so the defer is always safe.
2. api/upload_handler.go — permSvc nil deref.
MountUploadRoutes + handleServeFile dereference permSvc on every
authenticated file request. Add a fail-fast panic at mount time so
the misconfiguration surfaces at wiring, not on the first 500.
Update upload_handler_test.go to pass a real PermissionService built
on the test DB (the existing tests were missing the argument entirely,
which meant the package wouldn't compile — this fixes the real bug
Copilot flagged).
3. ws/event_persister.go — NewEventPersister nil EventStore panic.
run() dereferences p.store on every flush. Panic at constructor
time instead so the crash happens once at startup rather than
minutes later in a background goroutine.
4. plugin/host_ui.go — serve-time symlink check.
rejectSymlinksUnder only runs at install time, so a symlink created
post-install (accidental or malicious) would be followed by
http.ServeFile and leak host files. Add an os.Lstat + ModeSymlink
check + IsRegular check to AssetHandler on every request. Cheap
relative to the file read and closes the TOCTOU window.
─── Correctness / observability (should-fix) ───────────────────────
5. ws/deps.go:77 — requirePerm hides misconfig as FORBIDDEN.
Previously, nil database, nil perms, or a GetRoleForUser error all
returned ErrCodeForbidden with the same message, making operator
failures indistinguishable from legitimate permission denials.
Split the branches: misconfig + DB error now return ErrCodeInternal
with a server-side slog.Error so operators see the real problem;
FORBIDDEN is reserved for the actual permission-bit check.
6. telemetry/metrics.go — ServiceCallDurationMs renamed to Sec.
Field name said "Ms" but the instrument name was
`service_call_duration_seconds` with unit "s". Renamed the field
and updated all 8 service-layer callers so the struct field and
metric semantics match.
7. ws/event_persister.go — flushEvy typo → flushEvery.
Renamed the field and the one call site in run().
─── Comments out of sync with code ──────────────────────────────────
8. plugin/loader.go — Stat vs Lstat comment.
The comment claimed "Stat (not Lstat)" but the code correctly uses
os.Lstat to detect symlinks. Updated the comment to match the code;
the code was already right.
9. telemetry/telemetry_otel.go — compile claim wrong.
Comment said the file would fail to compile without the upstream
OTel modules, but the skeleton deliberately avoids importing them
and Init returns a runtime error instead. Updated the comment to
reflect actual CI behaviour (the -tags otel build step passes
today but doesn't exercise real telemetry).
─── Nit / polish ────────────────────────────────────────────────────
10. ws/event_pruner.go — startup delay magic constant.
Hard-coded time.Minute made the "run shortly after startup"
behaviour untestable (a test with a 100ms interval would still
wait a full minute). Cap the startup delay by the interval:
min(interval, time.Minute). Documented via a new `maxStartupDelay`
constant.
11. ws/event_pruner_test.go — new file.
Unit coverage for runPrune cutoff correctness, error swallowing,
StartEventPruner nil-store short-circuit, ctx cancellation, and
the interval-bounded startup delay from fix#10. Uses a fakeEventStore
stub that records every prune call and signals the first one so
tests don't sleep.
─── Verification ────────────────────────────────────────────────────
gofmt -l clean. No network access in sandbox so `go vet` and `go test`
could not run; the changes are local and surgical and every touched
file compiles in isolation against the existing signatures.
https://claude.ai/code/session_01UsBsQW2YiA2usk9pnJjAWk
Final in-sandbox completeness pass. Five focused pieces; the remaining
items in PHASE_BC_LOCAL_TODO.md after this commit are all genuinely
local-only (toolchain, network, native deps).
Test coverage (the biggest gap from prior reviews)
- Server/plugin/manifest_test.go — pluginNameRegexp accept/reject table,
validateRelativePath table, oversized version, unknown permission.
- Server/plugin/host_http_test.go — hostAllowed dot-boundary suffix,
empty-entry rejection, case insensitivity, FQDN trailing dot. ipAllowed
table over loopback, RFC1918, RFC4193 (ULA), RFC6598 (CGN), link-local,
multicast, unspecified — both v4 and v6 — plus public-IP accept cases.
- Server/plugin/loader_test.go — rejectSymlinksUnder catches direct and
nested symlinks; scanPluginDirectory rejects a plugin whose entrypoint
is a symlink. Skipped on Windows where symlink creation needs elevation.
- Server/plugin/host_ui_test.go — AssetHandler serves declared files,
rejects undeclared files (404), rejects path traversal, supports nested
asset paths.
- Server/ws/hub_seedseq_test.go — SeedSeq monotonic, never-backwards,
concurrent CAS safety, integration with nextSeq.
- Server/ws/extract_event_type_test.go — table covering happy paths,
control char rejection, escaped quote rejection, length cap (64),
empty/missing/non-JSON inputs.
Plugin install endpoint (closes a real feature gap)
- Server/plugin/registry.go — InstallFromZip extracts a plugin .zip into
a staging directory under cfg.Directory, validates it zip-slip safe
(cleaned-path Rel check), refuses non-regular entries, refuses
symlinks, caps compressed at 16 MiB and uncompressed total at 64 MiB
(each file gated by io.CopyN against the remaining budget). Manifest
is parsed at the staged root, then atomically renamed into the
canonical plugin directory and registered via the existing
installFromDisk path.
- Server/api/plugins_handler.go — POST /install accepts multipart with
one "plugin" file part, http.MaxBytesReader caps the request body,
io.LimitReader caps the in-memory buffer, calls Registry.InstallFromZip,
returns 201 with the new plugin name. The endpoint inherits the Pass 2
admin auth + IP gate (mounted under r.Use(admin.RequireAdminAuth)).
Protocol surface
- Server/ws/serve.go — buildAuthOK now takes replaySource and includes
it in the auth_ok payload as "replay_source": "none" | "buffer" | "db".
Two call sites updated: reconnect path passes the existing local,
fresh-connect path passes "none". Test export updated to pass "none".
CI build-tag matrix
- .github/workflows/ci.yml — three new steps inside server-build-test
build the server with -tags otel, -tags wazero, and -tags otel,wazero.
All three are continue-on-error: true until the upstream OTel and
wazero modules land in go.mod (tracked in PHASE_BC_LOCAL_TODO.md).
Once they do, dropping continue-on-error converts the steps into
hard CI gates against tag-boundary drift.
Documentation
- CHANGELOG.md — new root-level file with curated entries for Phase B,
Phase C, security, and behavioural changes operators must know about
(notably event_persistence.enabled = true by default).
- PHASE_BC_LOCAL_TODO.md — ticks off the install endpoint, the
replay_source field, and the existing event_persistence defaultYAML
entry. The remaining items are toolchain-bound.
After this pass, the in-sandbox completeness ceiling is reached.
Everything still pending requires Go 1.25 toolchain, npm install,
real OTel SDK + wazero modules, sqlc, postgres backend impl, or
tinygo.
https://claude.ai/code/session_01UsBsQW2YiA2usk9pnJjAWk
Eight focused follow-ups from the medium-severity review bucket. All
in-sandbox tractable; no module changes, no new dependencies.
Performance
- Drop the defensive memcpy in EventPersister.Enqueue. The hub already
passes a fresh slice from wrapWithSeq and the copy was happening under
seqMu, serializing broadcast throughput. Documented the no-mutate
contract on the call site.
Observability
- AppMetrics gains WSEventsPersistErrors counter; the persister run loop
bumps both it and the existing WSEventsPersisted counter via cached
metrics handle.
- Hub.persistEvent now extracts the real event type ("chat_message",
"voice_join", ...) from the wrapped JSON envelope via a small
no-allocation byte scan instead of recording the generic
"broadcast"/"channel_broadcast" label.
- Added OTel spans + ServiceCallDurationMs histogram entries on one
public method per remaining service: DMService.CreateDM,
VoiceService.JoinChannel, InviteService.CreateInvite,
ModerationService.BanUser, BlockService.BlockUser,
UserService.UpdateProfile. Mirrors the existing pattern from
MessageService.SendMessage.
Hardening
- plugin/loader now Lstat-walks each plugin directory and rejects any
symlink, plus refuses an entrypoint that is itself a symlink. The
asset handler's prefix check stays as defense in depth.
- ipAllowed (plugin HTTP capability) now rejects RFC6598 carrier-grade
NAT (100.64.0.0/10), closing a gap in net.IP.IsPrivate which only
covers RFC1918 + RFC4193.
- Registry.activateAll syncs Instance.Enabled := true after a successful
activate so callers reading the in-memory flag see the live state.
Documentation
- defaultYAML now documents the new event_persistence, telemetry, and
plugins config blocks with their defaults and one-line descriptions.
- PHASE_BC_LOCAL_TODO.md ticks off five items (defaultYAML docs ×2,
remaining service spans, registry wiring already-fixed in Pass 2).
https://claude.ai/code/session_01UsBsQW2YiA2usk9pnJjAWk
Phase B + C review pass: critical security and correctness fixes.
Security
- S1: plugin admin endpoints now require admin.RequireAdminAuth in addition
to AdminIPRestrict. Previously a LAN attacker on the allowed CIDR could
list/enable/disable/uninstall plugins without a session.
- S2: rewrite plugin HTTPDo allowlist with proper net/url parsing. Empty
entries are ignored, suffix matches require a dot boundary, and a custom
Dialer rejects loopback / RFC1918 / link-local addresses to close the
DNS-rebinding TOCTOU window. Redirects re-validated.
- S3 + #9: manifest Name pinned to ^[a-z0-9][a-z0-9_-]{0,63}$, Entrypoint
and UI tab assets validated against absolute / "..", NUL byte, backslash
and non-canonical paths. Asset handler hardened with filepath.Rel check
for symlink and prefix-without-separator escapes.
- S5: pluginBridge postMessage handler ignores the pluginId in the message
body and uses an e.source -> contentWindow lookup instead, defeating
spoofed messages from same-origin scripts.
- S8: HTTPDo body capped at 5 MiB via io.LimitReader, redirects bounded
to 5 hops.
Correctness
- Critical seq alignment: PersistEvent now takes the hub-assigned seq as a
required parameter so the events table row seq always matches the wrapped
payload seq. Hub seeds its in-memory atomic counter from MAX(events.seq)
on startup. Drops in the persister queue no longer mis-align row vs
payload seq.
- #1: live plugin.Registry constructed in main.go BEFORE NewRouter and
threaded through; admin handler is no longer wired with nil.
- #3: EventPersister.Stop is now safe to call without a prior Start by
tracking a started flag — previously deadlocked waiting on done.
Wiring
- NewRouter signature gains *plugin.Registry; two test callers updated.
- admin.RequireAdminAuth exported as a thin wrapper over the existing
package-private adminAuthMiddleware.
- sqlc query templates updated for the new PersistEvent + GetMaxEventSeq
contracts (sqlite + postgres).
https://claude.ai/code/session_01UsBsQW2YiA2usk9pnJjAWk
ChatDeps, PresenceDeps, ReactionDeps no longer carry *db.DB or
*permissions.Checker — those were only needed before the service
migration. VoiceDeps retains them for voice handlers not yet migrated.
https://claude.ai/code/session_01CBFF3r84ywkJRWwuqw8zD8
- upload_handler: uses PermissionService.HasChannelPerm (removes last
hasChannelPermREST usage)
- profile_handler: delegates to UserService with proper ErrConflict on
duplicate username
- topic_rate_limiter: per-topic throughput caps (100 msg/s default),
wired into deliverBroadcast for channel-scoped broadcasts
- voice_join: subscribes client to VoiceTopic on join
- voice_leave: unsubscribes client from VoiceTopic on leave
https://claude.ai/code/session_01CBFF3r84ywkJRWwuqw8zD8
Client now has three send channels:
- sendHigh (64 slots): DMs, mentions — drained first by writePump
- send (256 slots): chat messages, reactions — drained second
- sendLow (64 slots): typing, presence — drained last, dropped on overflow
writePump drains high-priority messages before checking normal/low.
PubSub gains PublishHigh/PublishLow alongside existing Publish.
EmitEvents routes events by priority:
- High: SequencedDMEvent, UserTargetedEvent
- Normal: ChannelEvent, VoiceChannelEvent
- Low: ExcludeSenderEvent (typing), PresenceEvent
Slow clients get typing/presence dropped first (sendLowMsg silently
drops), then disconnect on normal buffer overflow, ensuring DMs are
never lost to typing indicator backpressure.
https://claude.ai/code/session_01CBFF3r84ywkJRWwuqw8zD8
PublishLowPriority uses trySendMsg to silently drop messages when a
client's buffer is full, instead of disconnecting them. This provides
priority-based backpressure: chat messages use normal Publish (disconnect
on overflow), while ephemeral events like typing indicators and presence
updates use PublishLowPriority (drop on overflow).
https://claude.ai/code/session_01CBFF3r84ywkJRWwuqw8zD8
- Add UserService, DMService, InviteService, BlockService
- Migrate REST handlers (channel, DM, invite, block) to use services
- Remove block_handler.go (merged into dm_handler.go)
- Update all services to accept store.Store instead of *db.DB
- Router creates SQLiteStore and passes to service.New()
Handlers are now thin HTTP adapters: parse request → call service →
map error → write JSON. All business logic lives in the service layer.
https://claude.ai/code/session_01CBFF3r84ywkJRWwuqw8zD8
When a client focuses a channel, subscribe to its pub/sub topic.
When switching channels, unsubscribe from the old topic first.
This completes the pub/sub integration — channel broadcasts now
route only to clients subscribed to the relevant topic.
https://claude.ai/code/session_01CBFF3r84ywkJRWwuqw8zD8
Introduce topic-based PubSub for O(subscribers) message routing:
- Clients subscribe to "global" and "user:{id}" on connect
- Channel broadcasts route through "channel:{id}" topics
- deliverBroadcast() uses PubSub instead of iterating all clients
- UnsubscribeAll on disconnect/kick cleans up subscriptions
- Sequence numbering and replay buffer preserved unchanged
https://claude.ai/code/session_01CBFF3r84ywkJRWwuqw8zD8
Handlers now delegate all business logic (validation, permission checks,
DB operations) to MessageService and ChannelService instead of calling
*db.DB directly. This eliminates logic duplication and enables the
service layer's permission cache.
https://claude.ai/code/session_01CBFF3r84ywkJRWwuqw8zD8
Introduce Server/service/ package with MessageService, ChannelService,
and PermissionService that encapsulate business logic previously
scattered across REST and WS handlers. The PermissionService adds
per-user in-memory caching with TTL-based expiry to eliminate
per-message DB round-trips at scale.
Services are wired into the WS hub via deps structs (strangler-fig
pattern) — existing handlers continue to work unchanged, with service
references available for incremental migration.
https://claude.ai/code/session_01CBFF3r84ywkJRWwuqw8zD8
- Remove unused `ver` param from handleHealth and handleInfo (version
was intentionally removed from unauthenticated endpoints per C-2)
- Rename decodeBase64Loose to validateBase64Loose returning only error,
matching actual usage (all callers only validate, never use the bytes)
- Remove commented-out code flagged by gocritic
- Use bytes.Equal instead of string conversion comparison
- Remove unused buildRateLimitError function
- Remove unnecessary type assertions in e2eeCrypto.ts
- I-1: Add key holder election in Hub (lowest userID per channel); reject
non-key-holder voice_e2ee_offer with NOT_KEY_HOLDER error
- I-2: Accept raw (unpadded) base64 in E2EE announce/offer handlers via
decodeBase64Loose fallback
- I-6: Copy E2EE public key value while h.mu.RLock is held in getClientE2EEPubKey
- I-7: Lower loginRateLimitPerMinute from 60 to 5
- C-1: TOCTOU fix — target channel check held under same lock as client lookup
- C-2: Include is_key_holder bool in voice_token payload so client knows
whether to initiate key distribution
- M-5/M-6: Add ErrCodeBadPayload/ErrCodeNotKeyHolder error constants
- Fix pre-existing api build errors: block_handler.go getUserFromContext,
router.go RequirePermission arg count
- Add user_blocks table to all test DB schemas (ws, api DM)
- Add voice_e2ee_test.go and constants_test.go covering all fixes
Periodic key rotation:
- Key holder rotates room key every 5 minutes for forward secrecy,
independent of participant changes. Timer managed by key holder only.
Offer retry mechanism:
- Non-key-holders re-announce their public key after 10s timeout and
wait 5s more before giving up. Covers lost offers from target
disconnect during async key wrapping.
- Key holder now re-sends room key offer on duplicate announces (peer
may be re-requesting after a missed offer), instead of ignoring them.
Key fingerprint verification:
- New computeKeyFingerprint() in e2eeCrypto.ts — SHA-256 hash of raw
public key formatted as "AB12 CD34 ..." for out-of-band verification.
Can be displayed in UI for MITM detection.
Server hardening:
- Public key size limit tightened from 256 to 128 bytes (P-256
uncompressed = 65 bytes = ~88 base64 chars).
Client hardening:
- WebCrypto availability check at module load — throws descriptive
error if crypto.subtle is unavailable (non-HTTPS context).
- base64ToUint8() now wraps atob() in try-catch with clear error message.
https://claude.ai/code/session_01KKo3RwjdmcNzkgXNfUkgNT
