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>

Server/service/channel.go

@@ -114,12 +114,6 @@ func (s *ChannelService) HandleTyping(ctx context.Context, userID, channelID int
 		return nil, nil
 	}
 
-	// Per-user-per-channel rate limit.
-	ratKey := auth.Key(auth.Key("typing", userID), channelID)
-	if limiter != nil && !limiter.Allow(ratKey, 1, 3*time.Second) {
-		return nil, nil
-	}
-
 	ch, err := s.st.GetChannel(ctx, channelID)
 	if err != nil || ch == nil {
 		return nil, nil //nolint:nilerr // typing indicators are best-effort; errors silently dropped
@@ -140,6 +134,18 @@ func (s *ChannelService) HandleTyping(ctx context.Context, userID, channelID int
 		return nil, nil // silent drop
 	}
 
+	// Per-user-per-channel rate limit. Built only now that the channel is
+	// known to exist and the caller is authorized to read it (OC-0202): doing
+	// this before resolution let any caller-supplied channel id — including
+	// ids that don't exist or aren't readable — pin a new entry in the
+	// shared, process-wide RateLimiter. RateLimiter.Cleanup only evicts a key
+	// once every timestamp on it is stale, so a stream of forged channel ids
+	// could retain an unbounded number of dead map entries for hours.
+	ratKey := auth.Key(auth.Key("typing", userID), channelID)
+	if limiter != nil && !limiter.Allow(ratKey, 1, 3*time.Second) {
+		return nil, nil
+	}
+
 	return ch, nil
 }
 

Server/service/channel_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"testing"
+	"time"
 
 	"github.com/owncord/server/db"
 	"github.com/owncord/server/permissions"
@@ -137,3 +138,81 @@ func TestHandleTyping_BlockedInDMEmitsNothing(t *testing.T) {
 		t.Fatal("blocked user must not produce a typing broadcast")
 	}
 }
+
+// countingLimiter records every key passed to Allow so a test can assert
+// whether the rate-limit map was ever touched for a given call, without
+// depending on auth.RateLimiter's unexported internals. Allow always grants
+// the request — these tests only care about whether a key was built at all.
+type countingLimiter struct {
+	calls []string
+}
+
+func (c *countingLimiter) Allow(key string, limit int, window time.Duration) bool {
+	c.calls = append(c.calls, key)
+	return true
+}
+
+// TestHandleTyping_NoRateLimitKeyForNonexistentChannel locks OC-0202:
+// HandleTyping used to build the "typing:<uid>:<cid>" rate-limit key and call
+// limiter.Allow BEFORE resolving the channel at all, so any caller-supplied
+// channel id — including ids that don't exist — pinned a new entry in the
+// shared, process-wide RateLimiter. RateLimiter.Cleanup only evicts a key
+// once every timestamp on it is stale, and production runs cleanup with a
+// 6-hour window, so a client sending typing_start for a stream of forged
+// channel ids could retain millions of dead map entries for hours. The key
+// must only be built once the channel is known to exist (and, below,
+// once the caller is authorized to read it) so the key space is bounded to
+// real (user, channel) pairs.
+func TestHandleTyping_NoRateLimitKeyForNonexistentChannel(t *testing.T) {
+	database := newTestDB(t)
+	seedRole(t, database, &db.Role{
+		ID:          permissions.MemberRoleID,
+		Name:        "member",
+		Permissions: permissions.SendMessages | permissions.ReadMessages,
+		Position:    1,
+	})
+	seedUser(t, database, &db.User{ID: 1, Username: "alice"})
+	seedUserRole(t, database, 1, permissions.MemberRoleID)
+	// Deliberately do NOT seed channel 999999 — it must not exist.
+
+	svc := NewChannelService(database, NewPermissionService(database, permissions.NewChecker(database)))
+	limiter := &countingLimiter{}
+
+	ch, err := svc.HandleTyping(context.Background(), 1, 999999, limiter)
+	if err != nil || ch != nil {
+		t.Fatalf("typing on a nonexistent channel must silently drop: ch=%v err=%v", ch, err)
+	}
+	if len(limiter.calls) != 0 {
+		t.Fatalf("HandleTyping built a rate-limit key for a nonexistent channel: calls=%v — "+
+			"every forged channel id pins a new entry in the shared RateLimiter for hours "+
+			"(Cleanup only evicts once every timestamp on the key is stale)", limiter.calls)
+	}
+}
+
+// TestHandleTyping_NoRateLimitKeyWithoutReadPermission extends OC-0202 to an
+// existing channel the caller cannot read: the rate-limit key must still not
+// be built, so the key space stays bounded to channels the user is actually
+// authorized to see typing indicators in.
+func TestHandleTyping_NoRateLimitKeyWithoutReadPermission(t *testing.T) {
+	database := newTestDB(t)
+	seedRole(t, database, &db.Role{
+		ID:          permissions.MemberRoleID,
+		Name:        "member",
+		Permissions: permissions.SendMessages, // no ReadMessages
+		Position:    1,
+	})
+	seedUser(t, database, &db.User{ID: 1, Username: "alice"})
+	seedUserRole(t, database, 1, permissions.MemberRoleID)
+	seedChannel(t, database, &db.Channel{ID: 10, Name: "secret", Type: "text"})
+
+	svc := NewChannelService(database, NewPermissionService(database, permissions.NewChecker(database)))
+	limiter := &countingLimiter{}
+
+	ch, err := svc.HandleTyping(context.Background(), 1, 10, limiter)
+	if err != nil || ch != nil {
+		t.Fatalf("typing without ReadMessages must silently drop: ch=%v err=%v", ch, err)
+	}
+	if len(limiter.calls) != 0 {
+		t.Fatalf("HandleTyping built a rate-limit key before checking ReadMessages permission: calls=%v", limiter.calls)
+	}
+}

Server/service/emoji.go

@@ -141,6 +141,12 @@ func (s *EmojiService) Create(ctx context.Context, actorID int64, rawShortcode,
 
 	created, err := s.st.CreateEmoji(ctx, shortcode, storedAs, mimeType, actorID)
 	if err != nil {
+		if db.IsUniqueConstraintError(err) {
+			// Lost a race with another Create between the check above and this
+			// INSERT -- report the conflict the check would have caught, not a
+			// server fault.
+			return nil, fmt.Errorf("%w: an emoji named :%s: already exists", ErrConflict, shortcode)
+		}
 		return nil, fmt.Errorf("%w: failed to create emoji: %v", ErrInternal, err)
 	}
 

Server/service/emoji_test.go

@@ -192,6 +192,49 @@ func TestEmojiCreate_DuplicateShortcodeIsConflict(t *testing.T) {
 	}
 }
 
+// raceEmojiStore wraps a real *db.DB but always reports no existing emoji for
+// the pre-insert shortcode check, so a concurrent CreateEmoji that already
+// committed the same shortcode is only caught by the table's UNIQUE
+// constraint at INSERT time -- exactly what happens when two CreateEmoji
+// calls race past GetEmojiByShortcode before either INSERT commits.
+type raceEmojiStore struct {
+	*db.DB
+}
+
+func (f *raceEmojiStore) GetEmojiByShortcode(_ context.Context, _ string) (*db.Emoji, error) {
+	return nil, nil
+}
+
+// TestEmojiCreate_RaceOnInsertIsConflict pins OC-0217: when the shortcode
+// check races another Create and the row already exists by the time the
+// INSERT runs, the resulting UNIQUE-constraint error from CreateEmoji must
+// still surface as ErrConflict (matching the sequential duplicate-shortcode
+// path), not ErrInternal.
+func TestEmojiCreate_RaceOnInsertIsConflict(t *testing.T) {
+	database := newTestDB(t)
+	seedRole(t, database, &db.Role{ID: permissions.OwnerRoleID, Name: "Owner",
+		Permissions: permissions.Administrator, Position: permissions.OwnerRolePosition})
+	seedUser(t, database, &db.User{ID: 1})
+	seedUserRole(t, database, 1, permissions.OwnerRoleID)
+
+	checker := permissions.NewChecker(database)
+	svc := NewEmojiService(&raceEmojiStore{DB: database}, NewPermissionService(database, checker))
+
+	// Commit the shortcode directly, bypassing the service's own check, so the
+	// table already holds :wave: when Create runs its (stubbed) check.
+	if _, err := database.CreateEmoji(context.Background(), "wave", "stored-1", "image/png", 1); err != nil {
+		t.Fatalf("seed CreateEmoji: %v", err)
+	}
+
+	_, err := svc.Create(context.Background(), 1, "wave", "stored-2", "image/gif")
+	if !errors.Is(err, ErrConflict) {
+		t.Fatalf("raced Create error = %v, want ErrConflict", err)
+	}
+	if errors.Is(err, ErrInternal) {
+		t.Fatalf("raced Create error = %v, must not be ErrInternal", err)
+	}
+}
+
 func TestEmojiCreate_RejectsBadShortcodeBeforeInsert(t *testing.T) {
 	svc, _ := newEmojiService(t)
 	if _, err := svc.Create(context.Background(), 1, "no spaces", "stored-1", "image/png"); !errors.Is(err, ErrBadRequest) {

Server/service/moderation.go

@@ -130,6 +130,52 @@ func (s *ModerationService) BanUser(ctx context.Context, actorID, targetID int64
 	return nil
 }
 
+// AuthorizeRoleChange runs every ChangeUserRole precondition — MANAGE_ROLES,
+// target existence, the actor-outranks-target rule, role existence, and the
+// assign-below-own-rank rule — without mutating anything, and in the same
+// authorization-before-existence order as every other check in this file (see
+// BanUser): an actor without MANAGE_ROLES learns nothing about which user ids
+// exist. It exists so a caller that also performs another mutation in the
+// same request (the admin PATCH /users/{id} handler, which can ban and
+// role-change in one call) can authorize the role change *before* committing
+// the other mutation: checking only at ChangeUserRole time means a refused
+// role change is discovered only after the ban already landed, leaving a
+// "failed" request half-applied (OC-0215). It returns the validated actor
+// role, target user, and target role so callers that go on to commit (like
+// ChangeUserRole) don't need to re-fetch any of them.
+func (s *ModerationService) AuthorizeRoleChange(ctx context.Context, actorID, targetID, newRoleID int64) (actorRole *db.Role, target *db.User, newRole *db.Role, err error) {
+	if targetID <= 0 {
+		return nil, nil, nil, fmt.Errorf("%w: user_id must be positive", ErrBadRequest)
+	}
+	if actorID == targetID {
+		return nil, nil, nil, fmt.Errorf("%w: cannot change your own role", ErrBadRequest)
+	}
+
+	// Authorization before existence — see BanUser.
+	actorRole, err = s.requirePerm(ctx, actorID, permissions.ManageRoles)
+	if err != nil {
+		return nil, nil, nil, err
+	}
+	target, err = s.st.GetUserByID(ctx, targetID)
+	if err != nil || target == nil {
+		return nil, nil, nil, fmt.Errorf("%w: user not found", ErrNotFound)
+	}
+	if err := s.requireOutranksRole(ctx, actorRole, targetID); err != nil {
+		return nil, nil, nil, err
+	}
+
+	newRole, err = s.st.GetRoleByID(ctx, newRoleID)
+	if err != nil || newRole == nil {
+		return nil, nil, nil, fmt.Errorf("%w: role not found", ErrBadRequest)
+	}
+	// Administrator bypasses permission bits, never the hierarchy: the owner
+	// role is above every admin, so only the owner can grant it.
+	if newRole.Position >= actorRole.Position {
+		return nil, nil, nil, fmt.Errorf("%w: cannot assign a role at or above your own rank", ErrForbidden)
+	}
+	return actorRole, target, newRole, nil
+}
+
 // ChangeUserRole assigns newRoleID to the target user. It enforces
 // MANAGE_ROLES plus two hierarchy rules the admin panel previously had none
 // of: the actor must strictly outrank the target, and may not hand out a role
@@ -142,35 +188,10 @@ func (s *ModerationService) BanUser(ctx context.Context, actorID, targetID int64
 // delete for no reason, since this call already loaded and validated the
 // exact same row under the same request.
 func (s *ModerationService) ChangeUserRole(ctx context.Context, actorID, targetID, newRoleID int64) (*db.Role, error) {
-	if targetID <= 0 {
-		return nil, fmt.Errorf("%w: user_id must be positive", ErrBadRequest)
-	}
-	if actorID == targetID {
-		return nil, fmt.Errorf("%w: cannot change your own role", ErrBadRequest)
-	}
-
-	// Authorization before existence — see BanUser.
-	actorRole, err := s.requirePerm(ctx, actorID, permissions.ManageRoles)
+	_, target, newRole, err := s.AuthorizeRoleChange(ctx, actorID, targetID, newRoleID)
 	if err != nil {
 		return nil, err
 	}
-	target, err := s.st.GetUserByID(ctx, targetID)
-	if err != nil || target == nil {
-		return nil, fmt.Errorf("%w: user not found", ErrNotFound)
-	}
-	if err := s.requireOutranksRole(ctx, actorRole, targetID); err != nil {
-		return nil, err
-	}
-
-	newRole, err := s.st.GetRoleByID(ctx, newRoleID)
-	if err != nil || newRole == nil {
-		return nil, fmt.Errorf("%w: role not found", ErrBadRequest)
-	}
-	// Administrator bypasses permission bits, never the hierarchy: the owner
-	// role is above every admin, so only the owner can grant it.
-	if newRole.Position >= actorRole.Position {
-		return nil, fmt.Errorf("%w: cannot assign a role at or above your own rank", ErrForbidden)
-	}
 
 	if err := s.st.UpdateUserRole(ctx, targetID, newRoleID); err != nil {
 		return nil, fmt.Errorf("%w: failed to update role: %v", ErrInternal, err)