From 4b37c8024ea69411097a13e5152dc7cd36a2a849 Mon Sep 17 00:00:00 2001 From: J3vb <192430104+J3vb@users.noreply.github.com> Date: Sun, 19 Jul 2026 08:29:26 +0200 Subject: [PATCH] fix(service): enforce attachment ownership atomically in the link UPDATE (W1-3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- Server/db/attachment_queries.go | 19 ++++++++--- Server/service/message.go | 31 +++++------------- Server/store/memstore.go | 48 +++++++++++++++++++++------ Server/store/sqlite.go | 58 ++++++++++++++++++++------------- Server/store/store.go | 2 +- 5 files changed, 97 insertions(+), 61 deletions(-) diff --git a/Server/db/attachment_queries.go b/Server/db/attachment_queries.go index d8da2d1e..087b4c0c 100644 --- a/Server/db/attachment_queries.go +++ b/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...) diff --git a/Server/service/message.go b/Server/service/message.go index 083a16de..4a76a569 100644 --- a/Server/service/message.go +++ b/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 { diff --git a/Server/store/memstore.go b/Server/store/memstore.go index 88c3324f..5a56a9d8 100644 --- a/Server/store/memstore.go +++ b/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) { diff --git a/Server/store/sqlite.go b/Server/store/sqlite.go index 42062c52..64f7aba5 100644 --- a/Server/store/sqlite.go +++ b/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) diff --git a/Server/store/store.go b/Server/store/store.go index 925a078b..3d6c9df3 100644 --- a/Server/store/store.go +++ b/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) }