fix(service): enforce attachment ownership atomically in the link UPDATE (W1-3)

The per-attachment GetAttachmentByID pre-check loop was a check-then-link
TOCTOU (the same race pattern this branch fixes elsewhere), an N+1 on the
hot send path, and a hard ErrForbidden for legit retries naming an
already-linked attachment. Ownership now lives in the one UPDATE that
links: `AND message_id IS NULL AND (uploader_id = ? OR uploader_id IS
NULL)` — a foreign attachment can never be claimed, legacy NULL-uploader
rows stay claimable, and skipped rows (foreign/linked/missing) are logged
but never fail the send, so retries can't hard-fail. Subsumes W2-4; the
MemStore (nil,nil) GetAttachmentByID contortion is replaced by a real
map-backed attachment store so the guard is testable (W3-5).

Companion commit updates test callsites and adds ownership coverage.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Server/db/attachment_queries.go

@@ -91,23 +91,32 @@ func (d *DB) GetAttachmentWithChannel(id string) (*AttachmentAccess, error) {
 }
 
 // LinkAttachmentsToMessage sets message_id on attachments that are currently
-// unlinked (message_id IS NULL). Returns the number of rows updated.
-// Uses WHERE message_id IS NULL to prevent double-linking in a race.
-func (d *DB) LinkAttachmentsToMessage(messageID int64, attachmentIDs []string) (int64, error) {
+// unlinked (message_id IS NULL) and owned by uploaderID. Legacy rows with
+// uploader_id IS NULL are treated as unowned and may be claimed by any
+// sender. Rows that are already linked, owned by another user, or
+// nonexistent are skipped rather than errors, so a client retry of a
+// partially-completed send cannot fail the whole message. This single UPDATE
+// is the atomic attachment-IDOR guard for message sends: ownership is
+// enforced in the same statement that links, so there is no check-then-link
+// race. Returns the number of rows updated.
+func (d *DB) LinkAttachmentsToMessage(messageID, uploaderID int64, attachmentIDs []string) (int64, error) {
 	if len(attachmentIDs) == 0 {
 		return 0, nil
 	}
 
 	placeholders := make([]string, len(attachmentIDs))
-	args := make([]any, 0, len(attachmentIDs)+1)
+	args := make([]any, 0, len(attachmentIDs)+2)
 	args = append(args, messageID)
 	for i, id := range attachmentIDs {
 		placeholders[i] = "?"
 		args = append(args, id)
 	}
+	args = append(args, uploaderID)
 
 	query := fmt.Sprintf( //nolint:gosec // G201: placeholder interpolation, not user input
-		`UPDATE attachments SET message_id = ? WHERE id IN (%s) AND message_id IS NULL`,
+		`UPDATE attachments SET message_id = ?
+		 WHERE id IN (%s) AND message_id IS NULL
+		   AND (uploader_id = ? OR uploader_id IS NULL)`,
 		strings.Join(placeholders, ","),
 	)
 	res, err := d.sqlDB.Exec(query, args...)

Server/service/message.go

@@ -173,26 +173,6 @@ func (s *MessageService) SendMessage(ctx context.Context, p SendMessageParams) (
 		}
 	}
 
-	// Verify attachment ownership before persisting, to prevent hijacking
-	// another user's unlinked upload (IDOR). Any referenced attachment that
-	// exists must belong to the sender (uploader_id == p.UserID) and must not
-	// already be linked to a message (message_id IS NULL). Nonexistent IDs are
-	// ignored — LinkAttachmentsToMessage silently skips them. Checked before
-	// CreateMessage so a failed ownership check never persists a message.
-	for _, aid := range p.AttachmentIDs {
-		att, attErr := s.st.GetAttachmentByID(aid)
-		if attErr != nil {
-			slog.Error("MessageService.SendMessage GetAttachmentByID", "err", attErr, "attachment_id", aid)
-			return nil, fmt.Errorf("%w: failed to verify attachment ownership", ErrInternal)
-		}
-		if att == nil {
-			continue // nonexistent — the link query will skip it
-		}
-		if att.UploaderID == nil || *att.UploaderID != p.UserID || att.MessageID != nil {
-			return nil, fmt.Errorf("%w: attachment not owned by sender or already linked", ErrForbidden)
-		}
-	}
-
 	// Persist message.
 	msgID, err := s.st.CreateMessage(p.ChannelID, p.UserID, content, p.ReplyTo)
 	if err != nil {
@@ -200,10 +180,13 @@ func (s *MessageService) SendMessage(ctx context.Context, p SendMessageParams) (
 		return nil, fmt.Errorf("%w: failed to save message", ErrInternal)
 	}
 
-	// Link attachments.
+	// Link attachments. Ownership is enforced atomically inside the link
+	// UPDATE itself (uploader match + still unlinked), so another user's
+	// upload, an already-linked attachment, or a nonexistent id is skipped by
+	// the statement — no check-then-link race and no N+1 pre-verification.
 	var attachments []db.AttachmentInfo
 	if len(p.AttachmentIDs) > 0 {
-		linked, linkErr := s.st.LinkAttachmentsToMessage(msgID, p.AttachmentIDs)
+		linked, linkErr := s.st.LinkAttachmentsToMessage(msgID, p.UserID, p.AttachmentIDs)
 		if linkErr != nil {
 			slog.Error("MessageService.SendMessage LinkAttachments", "err", linkErr, "msg_id", msgID)
 			// Cleanup: soft-delete the message.
@@ -212,6 +195,10 @@ func (s *MessageService) SendMessage(ctx context.Context, p SendMessageParams) (
 			}
 			return nil, fmt.Errorf("%w: failed to send message with attachments", ErrInternal)
 		}
+		if linked < int64(len(p.AttachmentIDs)) {
+			slog.Warn("MessageService.SendMessage: skipped attachments (not owned, already linked, or missing)",
+				"msg_id", msgID, "user_id", p.UserID, "requested", len(p.AttachmentIDs), "linked", linked)
+		}
 		if linked > 0 {
 			attMap, attErr := s.st.GetAttachmentsByMessageIDs([]int64{msgID})
 			if attErr != nil {

Server/store/memstore.go

@@ -39,6 +39,10 @@ type MemStore struct {
 	blocks map[int64]map[int64]bool
 	// userID -> channelID -> lastReadMessageID
 	readStates map[int64]map[int64]int64
+	// attachment id -> row. Tracks uploader_id/message_id so the atomic
+	// link-ownership guard is exercised against a store that really records
+	// ownership instead of a (nil, nil) stub.
+	attachments map[string]*db.Attachment
 
 	// Phase B Step 7 / Phase C Step 9 — events + plugin KV. Lazily initialised
 	// via ensureEvents() so existing tests that constructed a bare MemStore
@@ -58,6 +62,7 @@ func NewMemStore() *MemStore {
 		channelOverrides: make(map[int64]map[int64]db.ChannelOverride),
 		reactions:        make(map[int64]map[int64]map[string]bool),
 		dmParticipants:   make(map[int64]map[int64]bool),
+		attachments:      make(map[string]*db.Attachment),
 		blocks:           make(map[int64]map[int64]bool),
 		readStates:       make(map[int64]map[int64]int64),
 	}
@@ -273,8 +278,26 @@ func (m *MemStore) GetLatestMessageID(channelID int64) (int64, error) {
 	return latest, nil
 }
 
-func (m *MemStore) LinkAttachmentsToMessage(_ int64, _ []string) (int64, error) {
-	return 0, nil
+// LinkAttachmentsToMessage mirrors the SQL guard in db.LinkAttachmentsToMessage:
+// only unlinked attachments owned by uploaderID (or legacy rows with a nil
+// uploader) are claimed; everything else is skipped, not an error.
+func (m *MemStore) LinkAttachmentsToMessage(messageID, uploaderID int64, attachmentIDs []string) (int64, error) {
+	m.mu.Lock()
+	defer m.mu.Unlock()
+	var n int64
+	for _, id := range attachmentIDs {
+		att, ok := m.attachments[id]
+		if !ok || att.MessageID != nil {
+			continue
+		}
+		if att.UploaderID != nil && *att.UploaderID != uploaderID {
+			continue
+		}
+		mid := messageID
+		att.MessageID = &mid
+		n++
+	}
+	return n, nil
 }
 
 func (m *MemStore) GetAttachmentsByMessageIDs(_ []int64) (map[int64][]db.AttachmentInfo, error) {
@@ -681,16 +704,21 @@ func (m *MemStore) ListBlockedUsers(_ int64) ([]int64, error) {
 
 // ---------- AttachmentStore ----------
 
-func (m *MemStore) CreateAttachment(_ string, _ int64, _, _, _ string, _ int64, _, _ *int) error {
-	panic("memstore: not implemented: CreateAttachment")
+func (m *MemStore) CreateAttachment(id string, uploaderID int64, filename, storedAs, mimeType string, size int64, _, _ *int) error {
+	m.mu.Lock()
+	defer m.mu.Unlock()
+	uid := uploaderID
+	m.attachments[id] = &db.Attachment{
+		ID: id, UploaderID: &uid, Filename: filename,
+		StoredAs: storedAs, MimeType: mimeType, Size: size,
+	}
+	return nil
 }
 
-func (m *MemStore) GetAttachmentByID(_ string) (*db.Attachment, error) {
-	// MemStore does not track attachments (CreateAttachment is unsupported and
-	// LinkAttachmentsToMessage is a no-op), so every lookup is "not found".
-	// Returning (nil, nil) rather than panicking keeps the attachment-ownership
-	// check in MessageService.SendMessage consistent with the no-op link path.
-	return nil, nil
+func (m *MemStore) GetAttachmentByID(id string) (*db.Attachment, error) {
+	m.mu.Lock()
+	defer m.mu.Unlock()
+	return m.attachments[id], nil
 }
 
 func (m *MemStore) GetAttachmentWithChannel(_ string) (*db.AttachmentAccess, error) {

Server/store/sqlite.go

@@ -109,8 +109,8 @@ func (s *SQLiteStore) GetChannelUnreadCounts(userID int64) (map[int64]db.Channel
 func (s *SQLiteStore) GetLatestMessageID(channelID int64) (int64, error) {
 	return s.db.GetLatestMessageID(channelID)
 }
-func (s *SQLiteStore) LinkAttachmentsToMessage(messageID int64, attachmentIDs []string) (int64, error) {
-	return s.db.LinkAttachmentsToMessage(messageID, attachmentIDs)
+func (s *SQLiteStore) LinkAttachmentsToMessage(messageID, uploaderID int64, attachmentIDs []string) (int64, error) {
+	return s.db.LinkAttachmentsToMessage(messageID, uploaderID, attachmentIDs)
 }
 func (s *SQLiteStore) GetAttachmentsByMessageIDs(msgIDs []int64) (map[int64][]db.AttachmentInfo, error) {
 	return s.db.GetAttachmentsByMessageIDs(msgIDs)
@@ -118,7 +118,7 @@ func (s *SQLiteStore) GetAttachmentsByMessageIDs(msgIDs []int64) (map[int64][]db
 
 // ── ChannelStore ────────────────────────────────────────────────────────────
 
-func (s *SQLiteStore) ListChannels() ([]db.Channel, error) { return s.db.ListChannels() }
+func (s *SQLiteStore) ListChannels() ([]db.Channel, error)      { return s.db.ListChannels() }
 func (s *SQLiteStore) GetChannel(id int64) (*db.Channel, error) { return s.db.GetChannel(id) }
 func (s *SQLiteStore) CreateChannel(name, chanType, category, topic string, position int) (int64, error) {
 	return s.db.CreateChannel(name, chanType, category, topic, position)
@@ -126,8 +126,10 @@ func (s *SQLiteStore) CreateChannel(name, chanType, category, topic string, posi
 func (s *SQLiteStore) UpdateChannel(id int64, name, topic string, slowMode int) error {
 	return s.db.UpdateChannel(id, name, topic, slowMode)
 }
-func (s *SQLiteStore) DeleteChannel(id int64) error           { return s.db.DeleteChannel(id) }
-func (s *SQLiteStore) SetChannelSlowMode(id int64, sm int) error { return s.db.SetChannelSlowMode(id, sm) }
+func (s *SQLiteStore) DeleteChannel(id int64) error { return s.db.DeleteChannel(id) }
+func (s *SQLiteStore) SetChannelSlowMode(id int64, sm int) error {
+	return s.db.SetChannelSlowMode(id, sm)
+}
 func (s *SQLiteStore) SetChannelVoiceMaxUsers(id int64, max int) error {
 	return s.db.SetChannelVoiceMaxUsers(id, max)
 }
@@ -192,21 +194,25 @@ func (s *SQLiteStore) DeleteSession(tokenHash string) error { return s.db.Delete
 func (s *SQLiteStore) DeleteOtherSessions(userID, keepSessionID int64) (int64, error) {
 	return s.db.DeleteOtherSessions(userID, keepSessionID)
 }
-func (s *SQLiteStore) DeleteExpiredSessions() error   { return s.db.DeleteExpiredSessions() }
-func (s *SQLiteStore) DeleteSessionByID(sid, uid int64) error { return s.db.DeleteSessionByID(sid, uid) }
-func (s *SQLiteStore) TouchSession(tokenHash string) error    { return s.db.TouchSession(tokenHash) }
+func (s *SQLiteStore) DeleteExpiredSessions() error { return s.db.DeleteExpiredSessions() }
+func (s *SQLiteStore) DeleteSessionByID(sid, uid int64) error {
+	return s.db.DeleteSessionByID(sid, uid)
+}
+func (s *SQLiteStore) TouchSession(tokenHash string) error { return s.db.TouchSession(tokenHash) }
 func (s *SQLiteStore) ListUserSessions(userID int64) ([]db.Session, error) {
 	return s.db.ListUserSessions(userID)
 }
-func (s *SQLiteStore) ForceLogoutUser(userID int64) error  { return s.db.ForceLogoutUser(userID) }
+func (s *SQLiteStore) ForceLogoutUser(userID int64) error { return s.db.ForceLogoutUser(userID) }
 func (s *SQLiteStore) GetUserSessions(userID int64) ([]db.Session, error) {
 	return s.db.GetUserSessions(userID)
 }
 
 // ── RoleStore ───────────────────────────────────────────────────────────────
 
-func (s *SQLiteStore) GetRoleByID(id int64) (*db.Role, error)         { return s.db.GetRoleByID(id) }
-func (s *SQLiteStore) GetRoleForUser(userID int64) (*db.Role, error)  { return s.db.GetRoleForUser(userID) }
+func (s *SQLiteStore) GetRoleByID(id int64) (*db.Role, error) { return s.db.GetRoleByID(id) }
+func (s *SQLiteStore) GetRoleForUser(userID int64) (*db.Role, error) {
+	return s.db.GetRoleForUser(userID)
+}
 func (s *SQLiteStore) GetUserWithRole(userID int64) (*db.User, *db.Role, error) {
 	return s.db.GetUserWithRole(userID)
 }
@@ -217,10 +223,10 @@ func (s *SQLiteStore) ListRoles() ([]*db.Role, error) { return s.db.ListRoles()
 func (s *SQLiteStore) CreateInvite(createdBy int64, maxUses int, expiresAt *time.Time) (string, error) {
 	return s.db.CreateInvite(createdBy, maxUses, expiresAt)
 }
-func (s *SQLiteStore) GetInvite(code string) (*db.Invite, error)  { return s.db.GetInvite(code) }
-func (s *SQLiteStore) ListInvites() ([]*db.Invite, error)         { return s.db.ListInvites() }
-func (s *SQLiteStore) UseInviteAtomic(code string) error          { return s.db.UseInviteAtomic(code) }
-func (s *SQLiteStore) RevokeInvite(code string) error             { return s.db.RevokeInvite(code) }
+func (s *SQLiteStore) GetInvite(code string) (*db.Invite, error) { return s.db.GetInvite(code) }
+func (s *SQLiteStore) ListInvites() ([]*db.Invite, error)        { return s.db.ListInvites() }
+func (s *SQLiteStore) UseInviteAtomic(code string) error         { return s.db.UseInviteAtomic(code) }
+func (s *SQLiteStore) RevokeInvite(code string) error            { return s.db.RevokeInvite(code) }
 
 // ── VoiceStore ──────────────────────────────────────────────────────────────
 
@@ -240,15 +246,21 @@ func (s *SQLiteStore) GetVoiceState(userID int64) (*db.VoiceState, error) {
 func (s *SQLiteStore) GetChannelVoiceStates(channelID int64) ([]db.VoiceState, error) {
 	return s.db.GetChannelVoiceStates(channelID)
 }
-func (s *SQLiteStore) GetAllVoiceStates() ([]db.VoiceState, error)   { return s.db.GetAllVoiceStates() }
-func (s *SQLiteStore) UpdateVoiceMute(userID int64, m bool) error    { return s.db.UpdateVoiceMute(userID, m) }
-func (s *SQLiteStore) UpdateVoiceDeafen(userID int64, d bool) error  { return s.db.UpdateVoiceDeafen(userID, d) }
-func (s *SQLiteStore) ClearVoiceState(userID int64) error            { return s.db.ClearVoiceState(userID) }
-func (s *SQLiteStore) ClearAllVoiceStates() error                    { return s.db.ClearAllVoiceStates() }
+func (s *SQLiteStore) GetAllVoiceStates() ([]db.VoiceState, error) { return s.db.GetAllVoiceStates() }
+func (s *SQLiteStore) UpdateVoiceMute(userID int64, m bool) error {
+	return s.db.UpdateVoiceMute(userID, m)
+}
+func (s *SQLiteStore) UpdateVoiceDeafen(userID int64, d bool) error {
+	return s.db.UpdateVoiceDeafen(userID, d)
+}
+func (s *SQLiteStore) ClearVoiceState(userID int64) error { return s.db.ClearVoiceState(userID) }
+func (s *SQLiteStore) ClearAllVoiceStates() error         { return s.db.ClearAllVoiceStates() }
 func (s *SQLiteStore) CountActiveCameras(channelID int64) (int, error) {
 	return s.db.CountActiveCameras(channelID)
 }
-func (s *SQLiteStore) UpdateVoiceCamera(userID int64, c bool) error { return s.db.UpdateVoiceCamera(userID, c) }
+func (s *SQLiteStore) UpdateVoiceCamera(userID int64, c bool) error {
+	return s.db.UpdateVoiceCamera(userID, c)
+}
 func (s *SQLiteStore) EnableCameraIfUnderLimit(userID, channelID int64, maxVideo int) (bool, error) {
 	return s.db.EnableCameraIfUnderLimit(userID, channelID, maxVideo)
 }
@@ -267,7 +279,7 @@ func (s *SQLiteStore) GetOrCreateDMChannel(user1ID, user2ID int64) (*db.Channel,
 func (s *SQLiteStore) GetUserDMChannels(userID int64) ([]db.DMChannelInfo, error) {
 	return s.db.GetUserDMChannels(userID)
 }
-func (s *SQLiteStore) OpenDM(userID, channelID int64) error { return s.db.OpenDM(userID, channelID) }
+func (s *SQLiteStore) OpenDM(userID, channelID int64) error  { return s.db.OpenDM(userID, channelID) }
 func (s *SQLiteStore) CloseDM(userID, channelID int64) error { return s.db.CloseDM(userID, channelID) }
 func (s *SQLiteStore) IsDMParticipant(userID, channelID int64) (bool, error) {
 	return s.db.IsDMParticipant(userID, channelID)
@@ -314,7 +326,7 @@ func (s *SQLiteStore) DeleteOrphanedAttachments(cutoff string) ([]string, error)
 
 // ── AdminStore ──────────────────────────────────────────────────────────────
 
-func (s *SQLiteStore) UserCount() (int64, error)             { return s.db.UserCount() }
+func (s *SQLiteStore) UserCount() (int64, error)                { return s.db.UserCount() }
 func (s *SQLiteStore) GetServerStats() (*db.ServerStats, error) { return s.db.GetServerStats() }
 func (s *SQLiteStore) ListAllUsers(limit, offset int) ([]db.UserWithRole, error) {
 	return s.db.ListAllUsers(limit, offset)

Server/store/store.go

@@ -58,7 +58,7 @@ type MessageStore interface {
 	UpdateReadState(userID, channelID, lastReadMessageID int64) error
 	GetChannelUnreadCounts(userID int64) (map[int64]db.ChannelUnread, error)
 	GetLatestMessageID(channelID int64) (int64, error)
-	LinkAttachmentsToMessage(messageID int64, attachmentIDs []string) (int64, error)
+	LinkAttachmentsToMessage(messageID, uploaderID int64, attachmentIDs []string) (int64, error)
 	GetAttachmentsByMessageIDs(msgIDs []int64) (map[int64][]db.AttachmentInfo, error)
 }