From 1da2aaddf08a7b8a37b39e0c5f1f7a8477198bda Mon Sep 17 00:00:00 2001 From: J3vb <192430104+J3vb@users.noreply.github.com> Date: Thu, 30 Jul 2026 15:01:21 +0200 Subject: [PATCH] fix(service): apply the full read/announcement gate to message edits (F3) EditMessage authorized the non-DM path with permissions.SendMessages alone while every sibling message sink requires ReadMessages plus the mutate bit, so a user denied READ_MESSAGES could still rewrite an old post and have the edit broadcast to the channel. The edit gate now calls the existing checkSendPermission helper and collapses its error into the sink's pre-existing opaque ErrForbidden, so the reply stays a non-oracle. Verified by a panel of agents; the added test fails against the unpatched tree, showing the edit succeeded before the fix. Co-Authored-By: Claude Opus 5 (1M context) --- Server/service/message.go | 18 +++++++- Server/service/message_test.go | 80 ++++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 2 deletions(-) diff --git a/Server/service/message.go b/Server/service/message.go index 01c2bdc9..52f4990d 100644 --- a/Server/service/message.go +++ b/Server/service/message.go @@ -283,7 +283,11 @@ func (s *MessageService) EditMessage(ctx context.Context, userID, msgID int64, r // Channel type for DM-aware permissions. ch, chErr := s.st.GetChannel(ctx, msg.ChannelID) - isDM := chErr == nil && ch != nil && ch.Type == "dm" + chanType := "" + if chErr == nil && ch != nil { + chanType = ch.Type + } + isDM := chanType == "dm" if isDM { ok, dmErr := s.st.IsDMParticipant(ctx, userID, msg.ChannelID) @@ -293,7 +297,17 @@ func (s *MessageService) EditMessage(ctx context.Context, userID, msgID int64, r if blkErr := requireDMNotBlocked(ctx, s.st, userID, msg.ChannelID); blkErr != nil { return nil, blkErr } - } else if !s.perms.HasChannelPerm(ctx, userID, msg.ChannelID, permissions.SendMessages) { + } else if permErr := s.checkSendPermission(ctx, userID, msg.ChannelID, chanType); permErr != nil { + // An edit injects new text into the channel and is fanned out to every + // reader, so it must clear the same gate as a send rather than + // SEND_MESSAGES alone: READ_MESSAGES so a role locked out of a private + // channel (the panel's "Can access" toggle denies + // READ_MESSAGES|CONNECT_VOICE and leaves SEND_MESSAGES intact) cannot + // rewrite its old posts, and the announcement rule so a demoted + // moderator cannot rewrite a trusted broadcast. Mirrors DeleteMessage, + // SetMessagePinned and handleReaction, which already require + // READ_MESSAGES. The reason is collapsed into this sink's single opaque + // error so the reply stays an ownership/permission non-oracle. return nil, fmt.Errorf("%w: cannot edit this message", ErrForbidden) } diff --git a/Server/service/message_test.go b/Server/service/message_test.go index 2a303a7b..2cf923f8 100644 --- a/Server/service/message_test.go +++ b/Server/service/message_test.go @@ -386,6 +386,86 @@ func TestEditMessage_EmptyContentFails(t *testing.T) { } } +// TestEditMessage_DeniedReadCannotEdit locks the channel-lockout invariant on +// the edit sink, alongside TestDeleteMessage_DeniedReadCannotDelete and +// TestSetMessagePinned_DeniedReadCannotPin. Editing fans new text out to every +// reader of the channel, so it must clear the send gate: unchecking "Can +// access" writes deny = READ_MESSAGES|CONNECT_VOICE and leaves SEND_MESSAGES +// intact, and an announcement channel accepts new text only from +// MANAGE_MESSAGES — so a demoted moderator must not be able to rewrite the +// broadcast they posted while privileged. +func TestEditMessage_DeniedReadCannotEdit(t *testing.T) { + database := newTestDB(t) + seedRole(t, database, &db.Role{ + ID: permissions.MemberRoleID, + Name: "member", + Permissions: permissions.SendMessages | permissions.ReadMessages, + Position: 1, + }) + seedRole(t, database, &db.Role{ + ID: permissions.ModeratorRoleID, + Name: "moderator", + Permissions: permissions.SendMessages | permissions.ReadMessages | permissions.ManageMessages, + Position: 10, + }) + seedUser(t, database, &db.User{ID: 1, Username: "alice"}) + seedUser(t, database, &db.User{ID: 2, Username: "mod_bob"}) + seedUserRole(t, database, 1, permissions.MemberRoleID) + seedUserRole(t, database, 2, permissions.ModeratorRoleID) + seedChannel(t, database, &db.Channel{ID: 10, Name: "staff-private", Type: "text"}) + seedChannel(t, database, &db.Channel{ID: 11, Name: "announcements", Type: "announcement"}) + + permSvc := NewPermissionService(database, permissions.NewChecker(database)) + svc := NewMessageService(database, permSvc, nil) + + sent, err := svc.SendMessage(context.Background(), SendMessageParams{ + ChannelID: 10, UserID: 1, Username: "alice", Content: "private discussion", + }) + if err != nil { + t.Fatalf("send: %v", err) + } + announced, err := svc.SendMessage(context.Background(), SendMessageParams{ + ChannelID: 11, UserID: 2, Username: "mod_bob", Content: "payroll is on the 1st", + }) + if err != nil { + t.Fatalf("send announcement: %v", err) + } + + // Admin unchecks "Can access" for the member role: READ_MESSAGES and + // CONNECT_VOICE are denied, SEND_MESSAGES survives the deny mask. + seedChannelOverride(t, database, permissions.MemberRoleID, 10, 0, permissions.ReadMessages|permissions.ConnectVoice) + permSvc.InvalidateChannel(10) + + if _, err := svc.EditMessage(context.Background(), 1, sent.MessageID, "payroll moved to http://evil/"); !errors.Is(err, ErrForbidden) { + t.Fatalf("author denied READ_MESSAGES must not edit: got %v", err) + } + + // The moderator is demoted to member: MANAGE_MESSAGES is gone, so the + // announcement they authored is no longer theirs to rewrite. + seedUserRole(t, database, 2, permissions.MemberRoleID) + permSvc.InvalidateUser(2) + + if _, err := svc.EditMessage(context.Background(), 2, announced.MessageID, "payroll moved to http://evil/"); !errors.Is(err, ErrForbidden) { + t.Fatalf("demoted author must not edit an announcement: got %v", err) + } + + for _, tc := range []struct { + id int64 + want string + }{ + {sent.MessageID, "private discussion"}, + {announced.MessageID, "payroll is on the 1st"}, + } { + msg, err := database.GetMessage(context.Background(), tc.id) + if err != nil || msg == nil { + t.Fatalf("GetMessage(%d): %v", tc.id, err) + } + if msg.Content != tc.want { + t.Fatalf("message %d must survive the refused edit: got %q", tc.id, msg.Content) + } + } +} + func TestDeleteMessage_OwnerCanDelete(t *testing.T) { svc, _ := newTestMessageService(t)