[pull] master from mattermost:master

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

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)

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

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)

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."

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

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