From 95f85e213f121c046447d438f5b5d7b1dba1f75e 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] test(service): cover atomic attachment-ownership link semantics (W1-3) Mechanical signature updates for LinkAttachmentsToMessage callsites, plus: db-level OwnershipGuard test (owned links, foreign never links, legacy NULL-uploader claimable, nonexistent skipped) and a service-level SendMessage test proving skip semantics end-to-end including the already-linked retry path. Co-Authored-By: Claude Fable 5 --- Server/db/attachment_queries_test.go | 50 +++++++++++++++++++-- Server/db/coverage_boost_test.go | 2 +- Server/service/message_test.go | 66 ++++++++++++++++++++++++++++ 3 files changed, 114 insertions(+), 4 deletions(-) diff --git a/Server/db/attachment_queries_test.go b/Server/db/attachment_queries_test.go index ab7339e4..4048b63c 100644 --- a/Server/db/attachment_queries_test.go +++ b/Server/db/attachment_queries_test.go @@ -57,7 +57,7 @@ func TestGetAttachmentByID_Found(t *testing.T) { func TestLinkAttachmentsToMessage_Empty(t *testing.T) { database := openMigratedMemory(t) - n, err := database.LinkAttachmentsToMessage(1, nil) + n, err := database.LinkAttachmentsToMessage(1, 1, nil) if err != nil { t.Fatalf("LinkAttachmentsToMessage(nil): %v", err) } @@ -84,7 +84,7 @@ func TestLinkAttachmentsToMessage_LinksUnlinked(t *testing.T) { } } - n, err := database.LinkAttachmentsToMessage(msgID, []string{"att-a", "att-b"}) + n, err := database.LinkAttachmentsToMessage(msgID, userID, []string{"att-a", "att-b"}) if err != nil { t.Fatalf("LinkAttachmentsToMessage: %v", err) } @@ -113,7 +113,7 @@ func TestLinkAttachmentsToMessage_SkipsAlreadyLinked(t *testing.T) { ) // Try to re-link to a different message — should skip (WHERE message_id IS NULL). - n, err := database.LinkAttachmentsToMessage(msg2, []string{"att-linked"}) + n, err := database.LinkAttachmentsToMessage(msg2, userID, []string{"att-linked"}) if err != nil { t.Fatalf("LinkAttachmentsToMessage: %v", err) } @@ -122,6 +122,50 @@ func TestLinkAttachmentsToMessage_SkipsAlreadyLinked(t *testing.T) { } } +// TestLinkAttachmentsToMessage_OwnershipGuard locks the atomic IDOR guard +// (W1-3): the link UPDATE itself enforces ownership, so a foreign attachment +// can never be claimed, legacy NULL-uploader rows remain claimable, and +// nonexistent ids are skipped without failing the statement. +func TestLinkAttachmentsToMessage_OwnershipGuard(t *testing.T) { + database := openMigratedMemory(t) + owner := seedUser(t, database, "att-owner") + other := seedUser(t, database, "att-other") + chID := seedChannel(t, database, "att-owner-ch") + msgID, _ := database.CreateMessage(chID, owner, "attachment carrier", nil) + + if err := database.CreateAttachment("att-owned", owner, "o.txt", "s-o.txt", "text/plain", 1, nil, nil); err != nil { + t.Fatalf("CreateAttachment att-owned: %v", err) + } + if err := database.CreateAttachment("att-foreign", other, "f.txt", "s-f.txt", "text/plain", 1, nil, nil); err != nil { + t.Fatalf("CreateAttachment att-foreign: %v", err) + } + // Legacy row from before uploader tracking: uploader_id IS NULL. + if _, err := database.Exec( + `INSERT INTO attachments (id, filename, stored_as, mime_type, size) + VALUES ('att-legacy', 'l.txt', 's-l.txt', 'text/plain', 1)`, + ); err != nil { + t.Fatalf("inserting legacy attachment: %v", err) + } + + n, err := database.LinkAttachmentsToMessage(msgID, owner, + []string{"att-owned", "att-foreign", "att-legacy", "att-missing"}) + if err != nil { + t.Fatalf("LinkAttachmentsToMessage: %v", err) + } + if n != 2 { + t.Errorf("expected 2 linked (owned + legacy), got %d", n) + } + if att, _ := database.GetAttachmentByID("att-owned"); att.MessageID == nil || *att.MessageID != msgID { + t.Error("owner's unlinked attachment should link") + } + if att, _ := database.GetAttachmentByID("att-foreign"); att.MessageID != nil { + t.Error("another user's attachment must never link (IDOR guard)") + } + if att, _ := database.GetAttachmentByID("att-legacy"); att.MessageID == nil { + t.Error("legacy NULL-uploader attachment should be claimable") + } +} + // ─── GetAttachmentsByMessageIDs ────────────────────────────────────────────── func TestGetAttachmentsByMessageIDs_Empty(t *testing.T) { diff --git a/Server/db/coverage_boost_test.go b/Server/db/coverage_boost_test.go index 4e7c7c9c..6202bafc 100644 --- a/Server/db/coverage_boost_test.go +++ b/Server/db/coverage_boost_test.go @@ -460,7 +460,7 @@ func TestDeleteOrphanedAttachments_KeepsLinked(t *testing.T) { // Create attachment and link it to a message. _ = database.CreateAttachment("linked-1", userID, "file.txt", "stored-linked.txt", "text/plain", 100, nil, nil) msgID, _ := database.CreateMessage(chID, userID, "with attachment", nil) - _, _ = database.LinkAttachmentsToMessage(msgID, []string{"linked-1"}) + _, _ = database.LinkAttachmentsToMessage(msgID, userID, []string{"linked-1"}) files, err := database.DeleteOrphanedAttachments("2099-01-01T00:00:00Z") if err != nil { diff --git a/Server/service/message_test.go b/Server/service/message_test.go index 599164a1..2edd32a3 100644 --- a/Server/service/message_test.go +++ b/Server/service/message_test.go @@ -56,6 +56,72 @@ func TestSendMessage_Valid(t *testing.T) { } } +// TestSendMessage_AttachmentOwnershipAtomic locks the W1-3 semantics: the +// link UPDATE itself enforces ownership, so a foreign, already-linked, or +// nonexistent attachment is skipped (never linked) while the message still +// sends — no check-then-link race, and retries cannot hard-fail. +func TestSendMessage_AttachmentOwnershipAtomic(t *testing.T) { + ms := store.NewMemStore() + ms.SeedRole(&db.Role{ + ID: permissions.MemberRoleID, + Name: "member", + Permissions: permissions.SendMessages | permissions.ReadMessages | permissions.AttachFiles, + Position: 1, + }) + ms.SeedUserRole(1, permissions.MemberRoleID) + ms.SeedUserRole(2, permissions.MemberRoleID) + ms.SeedUser(&db.User{ID: 1, Username: "alice", Status: "online"}) + ms.SeedUser(&db.User{ID: 2, Username: "mallory", Status: "online"}) + ms.SeedChannel(&db.Channel{ID: 10, Name: "general", Type: "text"}) + checker := permissions.NewChecker(ms) + svc := NewMessageService(ms, NewPermissionService(ms, checker), nil) + + if err := ms.CreateAttachment("att-own", 1, "a.png", "s-a.png", "image/png", 10, nil, nil); err != nil { + t.Fatal(err) + } + if err := ms.CreateAttachment("att-foreign", 2, "b.png", "s-b.png", "image/png", 10, nil, nil); err != nil { + t.Fatal(err) + } + + result, err := svc.SendMessage(context.Background(), SendMessageParams{ + ChannelID: 10, UserID: 1, Username: "alice", RoleName: "member", + Content: "with files", + AttachmentIDs: []string{"att-own", "att-foreign", "att-missing"}, + }) + if err != nil { + t.Fatalf("SendMessage: %v", err) + } + if result.MessageID <= 0 { + t.Fatal("message should persist even when some attachments are skipped") + } + + own, _ := ms.GetAttachmentByID("att-own") + if own.MessageID == nil || *own.MessageID != result.MessageID { + t.Error("sender's own attachment should be linked to the new message") + } + foreign, _ := ms.GetAttachmentByID("att-foreign") + if foreign.MessageID != nil { + t.Error("another user's attachment must never be linked (IDOR guard)") + } + + // A retry naming the now-linked attachment must still send. + retry, err := svc.SendMessage(context.Background(), SendMessageParams{ + ChannelID: 10, UserID: 1, Username: "alice", RoleName: "member", + Content: "retry", + AttachmentIDs: []string{"att-own"}, + }) + if err != nil { + t.Fatalf("retry with already-linked attachment should still send: %v", err) + } + if retry.MessageID <= 0 { + t.Fatal("retry should persist a message") + } + own2, _ := ms.GetAttachmentByID("att-own") + if own2.MessageID == nil || *own2.MessageID != result.MessageID { + t.Error("already-linked attachment must stay linked to the original message") + } +} + func TestSendMessage_EmptyContent(t *testing.T) { svc, _ := newTestMessageService()