From 1e0bdaf068ef2af7294d0ae0eff9b10e06cbfbc5 Mon Sep 17 00:00:00 2001 From: Maria A Nunez Date: Tue, 2 Jun 2026 16:11:55 -0400 Subject: [PATCH 1/2] MM-69057: Verify post ownership on inbound shared-channel edit/delete (#36814) The inbound shared-channel sync handler applied edits and deletes from a remote cluster without checking that the existing post belonged to that remote, allowing a remote to modify or delete posts it did not own. Enforce the same ownership check already used for reactions and acknowledgements before editing or deleting a synced post, and add regression tests for the cross-remote cases. Co-authored-by: Cursor --- .../services/sharedchannel/sync_recv.go | 8 +++ .../services/sharedchannel/sync_recv_test.go | 65 +++++++++++++++++++ 2 files changed, 73 insertions(+) diff --git a/server/platform/services/sharedchannel/sync_recv.go b/server/platform/services/sharedchannel/sync_recv.go index 8bb1e66d143..d24db7a8698 100644 --- a/server/platform/services/sharedchannel/sync_recv.go +++ b/server/platform/services/sharedchannel/sync_recv.go @@ -524,6 +524,10 @@ func (scs *Service) upsertSyncPost(post *model.Post, targetChannel *model.Channe ) } } else if post.DeleteAt > 0 { + // make sure the post being deleted is owned by the remote + if rpost.GetRemoteID() != rc.RemoteId { + return nil, fmt.Errorf("post sync failed: %w", ErrRemoteIDMismatch) + } // delete post rpost, appErr = scs.app.DeletePost(rctx, post.Id, post.UserId) if appErr == nil { @@ -533,6 +537,10 @@ func (scs *Service) upsertSyncPost(post *model.Post, targetChannel *model.Channe ) } } else if post.EditAt > rpost.EditAt || post.Message != rpost.Message || post.UpdateAt > rpost.UpdateAt || post.Metadata != nil { + // make sure the post being edited is owned by the remote + if rpost.GetRemoteID() != rc.RemoteId { + return nil, fmt.Errorf("post sync failed: %w", ErrRemoteIDMismatch) + } scs.transformMentionsOnReceive(rctx, post, targetChannel, rc, mentionTransforms) var priority *model.PostPriority var acknowledgements []*model.PostAcknowledgement diff --git a/server/platform/services/sharedchannel/sync_recv_test.go b/server/platform/services/sharedchannel/sync_recv_test.go index 215c845188d..2bd6e4ca8e5 100644 --- a/server/platform/services/sharedchannel/sync_recv_test.go +++ b/server/platform/services/sharedchannel/sync_recv_test.go @@ -299,3 +299,68 @@ func TestUpsertSyncUserStatus(t *testing.T) { mockApp.AssertNotCalled(t, "SaveAndBroadcastStatus") }) } + +func TestUpsertSyncPost(t *testing.T) { + remoteID := model.NewId() + channelID := model.NewId() + channel := &model.Channel{Id: channelID, Type: model.ChannelTypeOpen} + rc := &model.RemoteCluster{RemoteId: remoteID, Name: "test-remote"} + + setup := func(t *testing.T, existing *model.Post) (*Service, *MockAppIface) { + mockPostStore := &mocks.PostStore{} + mockPostStore.On("GetSingle", mock.Anything, existing.Id, true).Return(existing, nil) + + mockStore := &mocks.Store{} + mockStore.On("Post").Return(mockPostStore) + + logger := mlog.CreateConsoleTestLogger(t) + mockServer := &MockServerIface{} + mockServer.On("GetStore").Return(mockStore) + mockServer.On("Log").Return(logger) + + mockApp := &MockAppIface{} + + return &Service{server: mockServer, app: mockApp}, mockApp + } + + t.Run("rejects edit of a post owned by a different remote", func(t *testing.T) { + otherRemoteID := model.NewId() + postID := model.NewId() + existing := &model.Post{Id: postID, ChannelId: channelID, Message: "original", RemoteId: new(otherRemoteID)} + + scs, mockApp := setup(t, existing) + + _, err := scs.upsertSyncPost(&model.Post{Id: postID, ChannelId: channelID, Message: "tampered"}, channel, rc, nil) + + require.Error(t, err) + assert.ErrorIs(t, err, ErrRemoteIDMismatch) + mockApp.AssertNotCalled(t, "UpdatePost", mock.Anything, mock.Anything, mock.Anything) + }) + + t.Run("rejects delete of a post owned by a local user", func(t *testing.T) { + postID := model.NewId() + existing := &model.Post{Id: postID, ChannelId: channelID, Message: "original", RemoteId: nil} + + scs, mockApp := setup(t, existing) + + _, err := scs.upsertSyncPost(&model.Post{Id: postID, ChannelId: channelID, DeleteAt: model.GetMillis()}, channel, rc, nil) + + require.Error(t, err) + assert.ErrorIs(t, err, ErrRemoteIDMismatch) + mockApp.AssertNotCalled(t, "DeletePost", mock.Anything, mock.Anything, mock.Anything) + }) + + t.Run("allows edit of a post owned by the sending remote", func(t *testing.T) { + postID := model.NewId() + existing := &model.Post{Id: postID, ChannelId: channelID, Message: "original", RemoteId: new(remoteID)} + + scs, mockApp := setup(t, existing) + updated := &model.Post{Id: postID, ChannelId: channelID, Message: "updated"} + mockApp.On("UpdatePost", mock.Anything, mock.Anything, mock.Anything).Return(updated, false, (*model.AppError)(nil)) + + _, err := scs.upsertSyncPost(&model.Post{Id: postID, ChannelId: channelID, Message: "updated"}, channel, rc, nil) + + require.NoError(t, err) + mockApp.AssertCalled(t, "UpdatePost", mock.Anything, mock.Anything, mock.Anything) + }) +} From ab31663fce2c6faff4c051ae30e740e4af0dbc93 Mon Sep 17 00:00:00 2001 From: Maria A Nunez Date: Tue, 2 Jun 2026 20:26:42 -0400 Subject: [PATCH 2/2] MM-69010: Validate incoming webhook user membership (#36811) * MM-69010: Validate incoming webhook user membership Incoming webhook creation/update did not verify that the assigned user_id had legitimate access to the target team or channel, allowing a team admin to attribute persisted posts to an arbitrary user. Validate that the assigned user can read the target channel and does not hold privileges the requester lacks at creation, re-check channel access when a hook is moved, and require a shared team before a webhook creates a direct message via an @username payload. Co-authored-by: Cursor * MM-69010: Add regression test for owner+channel update Verify that changing both the channel and the supplied user_id in a single update still validates against the retained owner, since the owner is immutable on update. Co-authored-by: Cursor --------- Co-authored-by: Cursor Co-authored-by: Mattermost Build --- server/channels/api4/webhook.go | 18 +++++++- server/channels/api4/webhook_test.go | 69 ++++++++++++++++++++++++++++ server/channels/app/webhook.go | 34 ++++++++++++++ server/channels/app/webhook_test.go | 30 ++++++++++++ server/i18n/en.json | 8 ++++ 5 files changed, 158 insertions(+), 1 deletion(-) diff --git a/server/channels/api4/webhook.go b/server/channels/api4/webhook.go index 3d85b64d0b2..37d0b6f0e32 100644 --- a/server/channels/api4/webhook.go +++ b/server/channels/api4/webhook.go @@ -64,11 +64,18 @@ func createIncomingHook(c *Context, w http.ResponseWriter, r *http.Request) { return } - if _, err = c.App.GetUser(hook.UserId); err != nil { + var hookUser *model.User + if hookUser, err = c.App.GetUser(hook.UserId); err != nil { c.Err = err return } + if appErr := c.App.ValidateIncomingWebhookUser(c.AppContext, *c.AppContext.Session(), hookUser, channel); appErr != nil { + c.LogAudit("fail - invalid webhook user") + c.Err = appErr + return + } + userId = hook.UserId } @@ -167,6 +174,15 @@ func updateIncomingHook(c *Context, w http.ResponseWriter, r *http.Request) { } } + // Moving the hook must not attribute its owner's posts to a channel they cannot access. + if updatedHook.ChannelId != oldHook.ChannelId { + if appErr := c.App.ValidateIncomingWebhookUserChannelAccess(c.AppContext, oldHook.UserId, channel); appErr != nil { + c.LogAudit("fail - invalid webhook user") + c.Err = appErr + return + } + } + if !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), channel.TeamId, model.PermissionBypassIncomingWebhookChannelLock) { updatedHook.ChannelLocked = true updatedHook.ChannelId = channel.Id diff --git a/server/channels/api4/webhook_test.go b/server/channels/api4/webhook_test.go index c19e826aec9..7c64fb3e684 100644 --- a/server/channels/api4/webhook_test.go +++ b/server/channels/api4/webhook_test.go @@ -192,6 +192,75 @@ func TestCreateIncomingWebhook_BypassTeamPermissions(t *testing.T) { CheckForbiddenStatus(t, resp) } +func TestIncomingWebhookValidateUser(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableIncomingWebhooks = true }) + + defaultRolePermissions := th.SaveDefaultRolePermissions(t) + defer th.RestoreDefaultRolePermissions(t, defaultRolePermissions) + addIncomingWebhookPermissionsWithOthers(t, th, model.TeamAdminRoleId) + + th.LoginTeamAdmin(t) + + t.Run("cannot assign a user who is not a member of the team or channel", func(t *testing.T) { + nonMember := th.CreateUser(t) + + hook := &model.IncomingWebhook{ChannelId: th.BasicChannel.Id, UserId: nonMember.Id} + _, resp, err := th.Client.CreateIncomingWebhook(context.Background(), hook) + require.Error(t, err) + CheckForbiddenStatus(t, resp) + }) + + t.Run("cannot assign a user with higher privileges than the requester", func(t *testing.T) { + th.LinkUserToTeam(t, th.SystemAdminUser, th.BasicTeam) + _, appErr := th.App.AddUserToChannel(th.Context, th.SystemAdminUser, th.BasicChannel, false) + require.Nil(t, appErr) + + hook := &model.IncomingWebhook{ChannelId: th.BasicChannel.Id, UserId: th.SystemAdminUser.Id} + _, resp, err := th.Client.CreateIncomingWebhook(context.Background(), hook) + require.Error(t, err) + CheckForbiddenStatus(t, resp) + }) + + t.Run("can assign a user who is a member of the channel", func(t *testing.T) { + hook := &model.IncomingWebhook{ChannelId: th.BasicChannel.Id, UserId: th.BasicUser2.Id} + created, _, err := th.Client.CreateIncomingWebhook(context.Background(), hook) + require.NoError(t, err) + require.Equal(t, th.BasicUser2.Id, created.UserId) + }) + + t.Run("update cannot move another user's hook to a channel they cannot access", func(t *testing.T) { + hook := &model.IncomingWebhook{ChannelId: th.BasicChannel.Id, UserId: th.BasicUser2.Id} + created, _, err := th.Client.CreateIncomingWebhook(context.Background(), hook) + require.NoError(t, err) + + privateChannel := th.CreatePrivateChannel(t) + created.ChannelId = privateChannel.Id + + _, resp, err := th.Client.UpdateIncomingWebhook(context.Background(), created) + require.Error(t, err) + CheckForbiddenStatus(t, resp) + }) + + t.Run("update validates the retained owner even when the payload also changes the owner", func(t *testing.T) { + hook := &model.IncomingWebhook{ChannelId: th.BasicChannel.Id, UserId: th.BasicUser2.Id} + created, _, err := th.Client.CreateIncomingWebhook(context.Background(), hook) + require.NoError(t, err) + + // The owner is immutable on update, so changing it alongside the channel must not + // let the supplied user stand in for the retained owner's channel access. + privateChannel := th.CreatePrivateChannel(t) + created.ChannelId = privateChannel.Id + created.UserId = th.TeamAdminUser.Id + + _, resp, err := th.Client.UpdateIncomingWebhook(context.Background(), created) + require.Error(t, err) + CheckForbiddenStatus(t, resp) + }) +} + func TestGetIncomingWebhooks(t *testing.T) { mainHelper.Parallel(t) th := Setup(t).InitBasic(t) diff --git a/server/channels/app/webhook.go b/server/channels/app/webhook.go index 68f5655c04e..7699f21a6df 100644 --- a/server/channels/app/webhook.go +++ b/server/channels/app/webhook.go @@ -393,6 +393,29 @@ func (a *App) CreateWebhookPost(rctx request.CTX, userID string, channel *model. return splits[0], nil } +// ValidateIncomingWebhookUser ensures a user being assigned as an incoming webhook's owner can +// legitimately be attributed posts in the target channel: the user must have access to the +// channel and must not hold privileges the requester lacks, so a requester cannot forge posts +// as a non-member or higher-privileged user. +func (a *App) ValidateIncomingWebhookUser(rctx request.CTX, session model.Session, user *model.User, channel *model.Channel) *model.AppError { + if user.IsSystemAdmin() && !a.SessionHasPermissionTo(session, model.PermissionManageSystem) { + return model.NewAppError("ValidateIncomingWebhookUser", "api.webhook.incoming.user_role.app_error", nil, "user_id="+user.Id, http.StatusForbidden) + } + + return a.ValidateIncomingWebhookUserChannelAccess(rctx, user.Id, channel) +} + +// ValidateIncomingWebhookUserChannelAccess ensures the webhook owner can read the channel its +// posts are attributed to, preventing attribution to a user who is not a member of the channel +// (or its team, for open channels). +func (a *App) ValidateIncomingWebhookUserChannelAccess(rctx request.CTX, userID string, channel *model.Channel) *model.AppError { + if hasPermission, _ := a.HasPermissionToChannel(rctx, userID, channel.Id, model.PermissionReadChannelContent); !hasPermission { + return model.NewAppError("ValidateIncomingWebhookUserChannelAccess", "api.webhook.incoming.user_membership.app_error", nil, "user_id="+userID+", channel_id="+channel.Id, http.StatusForbidden) + } + + return nil +} + func (a *App) CreateIncomingWebhookForChannel(creatorId string, channel *model.Channel, hook *model.IncomingWebhook) (*model.IncomingWebhook, *model.AppError) { if !*a.Config().ServiceSettings.EnableIncomingWebhooks { return nil, model.NewAppError("CreateIncomingWebhookForChannel", "api.incoming_webhook.disabled.app_error", nil, "", http.StatusNotImplemented) @@ -804,6 +827,17 @@ func (a *App) HandleIncomingWebhook(rctx request.CTX, hookID string, req *model. if nErr != nil { return model.NewAppError("HandleIncomingWebhook", "web.incoming_webhook.user.app_error", map[string]any{"user": channelName[1:]}, "", http.StatusBadRequest).Wrap(nErr) } + // Only allow a DM target the webhook owner shares a team with, so the stored + // user_id cannot be used to reach users the owner could not message directly. + if hook.UserId != result.Id { + commonTeamIDs, teamErr := a.GetCommonTeamIDsForTwoUsers(hook.UserId, result.Id) + if teamErr != nil { + return teamErr + } + if len(commonTeamIDs) == 0 { + return model.NewAppError("HandleIncomingWebhook", "web.incoming_webhook.permissions.app_error", map[string]any{"user": hook.UserId, "channel": channelName}, "", http.StatusForbidden) + } + } ch, err := a.GetOrCreateDirectChannel(rctx, hook.UserId, result.Id) if err != nil { return err diff --git a/server/channels/app/webhook_test.go b/server/channels/app/webhook_test.go index 75274ead5a3..3b2fe8a9507 100644 --- a/server/channels/app/webhook_test.go +++ b/server/channels/app/webhook_test.go @@ -98,6 +98,36 @@ func TestHandleIncomingWebhookRootId(t *testing.T) { }) } +func TestHandleIncomingWebhookDirectMessage(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic(t) + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableIncomingWebhooks = true }) + + hook, appErr := th.App.CreateIncomingWebhookForChannel(th.BasicUser.Id, th.BasicChannel, &model.IncomingWebhook{ChannelId: th.BasicChannel.Id, ChannelLocked: false}) + require.Nil(t, appErr) + defer func() { + require.Nil(t, th.App.DeleteIncomingWebhook(hook.Id)) + }() + + t.Run("rejects DM to a user the owner shares no team with", func(t *testing.T) { + stranger := th.CreateUser(t) + err := th.App.HandleIncomingWebhook(th.Context, hook.Id, &model.IncomingWebhookRequest{ + Text: "out of team dm", + ChannelName: "@" + stranger.Username, + }) + require.NotNil(t, err) + assert.Equal(t, http.StatusForbidden, err.StatusCode) + }) + + t.Run("allows DM to a user the owner shares a team with", func(t *testing.T) { + err := th.App.HandleIncomingWebhook(th.Context, hook.Id, &model.IncomingWebhookRequest{ + Text: "team dm", + ChannelName: "@" + th.BasicUser2.Username, + }) + require.Nil(t, err) + }) +} + func TestCreateIncomingWebhookForChannel(t *testing.T) { mainHelper.Parallel(t) th := Setup(t).InitBasic(t) diff --git a/server/i18n/en.json b/server/i18n/en.json index be6a00eb8a5..647d0666555 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -5242,6 +5242,14 @@ "id": "api.webhook.create_outgoing.triggers.app_error", "translation": "Either trigger_words or channel_id must be set." }, + { + "id": "api.webhook.incoming.user_membership.app_error", + "translation": "The webhook user must be a member of the target team or channel." + }, + { + "id": "api.webhook.incoming.user_role.app_error", + "translation": "You cannot assign a webhook to a user with higher privileges than your own." + }, { "id": "api.webhook.team_mismatch.app_error", "translation": "Unable to update webhook across teams."