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." 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) + }) +}