[MM-54024] Handle JSON nulls when unmarshalling in api4 (#24656)

* handle JSON nulls when unmarshalling in api4

* catch more cases of unhandled JSON null

* fix lint issues

* remove unnecessary checks

* add bot tests for null values

* fix bot_test lint

* add channel null tests

* add group null tests

* add notify_admin null tests

---------

Co-authored-by: Mattermost Build <build@mattermost.com>

server/channels/api4/bot.go

@@ -27,7 +27,7 @@ func (api *API) InitBot() {
 func createBot(c *Context, w http.ResponseWriter, r *http.Request) {
 	var botPatch *model.BotPatch
 	err := json.NewDecoder(r.Body).Decode(&botPatch)
-	if err != nil {
+	if err != nil || botPatch == nil {
 		c.SetInvalidParamWithErr("bot", err)
 		return
 	}
@@ -83,7 +83,7 @@ func patchBot(c *Context, w http.ResponseWriter, r *http.Request) {
 
 	var botPatch *model.BotPatch
 	err := json.NewDecoder(r.Body).Decode(&botPatch)
-	if err != nil {
+	if err != nil || botPatch == nil {
 		c.SetInvalidParamWithErr("bot", err)
 		return
 	}

server/channels/api4/bot_test.go

@@ -133,6 +133,23 @@ func TestCreateBot(t *testing.T) {
 		CheckErrorID(t, err, "api.context.permissions.app_error")
 	})
 
+	t.Run("create bot with null value", func(t *testing.T) {
+		th := Setup(t).InitBasic()
+		defer th.TearDown()
+		defer th.RestoreDefaultRolePermissions(th.SaveDefaultRolePermissions())
+
+		th.AddPermissionToRole(model.PermissionCreateBot.Id, model.TeamUserRoleId)
+		th.App.UpdateUserRoles(th.Context, th.BasicUser.Id, model.TeamUserRoleId, false)
+		th.App.UpdateConfig(func(cfg *model.Config) {
+			*cfg.ServiceSettings.EnableBotAccountCreation = true
+		})
+
+		var bot *model.Bot
+
+		_, resp, err := th.Client.CreateBot(context.Background(), bot)
+		require.Error(t, err)
+		CheckBadRequestStatus(t, resp)
+	})
 }
 
 func TestPatchBot(t *testing.T) {
@@ -474,6 +491,34 @@ func TestPatchBot(t *testing.T) {
 
 		require.Equal(t, th.BasicUser.Id, patchedBot.OwnerId)
 	})
+
+	t.Run("patch with null bot", func(t *testing.T) {
+		th := Setup(t).InitBasic()
+		defer th.TearDown()
+		defer th.RestoreDefaultRolePermissions(th.SaveDefaultRolePermissions())
+
+		th.AddPermissionToRole(model.PermissionCreateBot.Id, model.TeamUserRoleId)
+		th.AddPermissionToRole(model.PermissionManageBots.Id, model.TeamUserRoleId)
+		th.App.UpdateUserRoles(th.Context, th.BasicUser.Id, model.TeamUserRoleId, false)
+		th.App.UpdateConfig(func(cfg *model.Config) {
+			*cfg.ServiceSettings.EnableBotAccountCreation = true
+		})
+
+		createdBot, resp, err := th.Client.CreateBot(context.Background(), &model.Bot{
+			Username:    GenerateTestUsername(),
+			DisplayName: "a bot",
+			Description: "bot",
+		})
+		require.NoError(t, err)
+		CheckCreatedStatus(t, resp)
+		defer th.App.PermanentDeleteBot(createdBot.UserId)
+
+		var botPatch *model.BotPatch
+
+		_, resp1, err1 := th.Client.PatchBot(context.Background(), createdBot.UserId, botPatch)
+		require.Error(t, err1)
+		CheckBadRequestStatus(t, resp1)
+	})
 }
 
 func TestGetBot(t *testing.T) {

server/channels/api4/channel.go

@@ -85,7 +85,7 @@ func (api *API) InitChannel() {
 func createChannel(c *Context, w http.ResponseWriter, r *http.Request) {
 	var channel *model.Channel
 	err := json.NewDecoder(r.Body).Decode(&channel)
-	if err != nil {
+	if err != nil || channel == nil {
 		c.SetInvalidParamWithErr("channel", err)
 		return
 	}
@@ -312,7 +312,7 @@ func patchChannel(c *Context, w http.ResponseWriter, r *http.Request) {
 	}
 	var patch *model.ChannelPatch
 	err := json.NewDecoder(r.Body).Decode(&patch)
-	if err != nil {
+	if err != nil || patch == nil {
 		c.SetInvalidParamWithErr("channel", err)
 		return
 	}
@@ -495,7 +495,7 @@ func createDirectChannel(c *Context, w http.ResponseWriter, r *http.Request) {
 func searchGroupChannels(c *Context, w http.ResponseWriter, r *http.Request) {
 	var props *model.ChannelSearch
 	err := json.NewDecoder(r.Body).Decode(&props)
-	if err != nil {
+	if err != nil || props == nil {
 		c.SetInvalidParamWithErr("channel_search", err)
 		return
 	}
@@ -1099,7 +1099,7 @@ func searchChannelsForTeam(c *Context, w http.ResponseWriter, r *http.Request) {
 
 	var props *model.ChannelSearch
 	err := json.NewDecoder(r.Body).Decode(&props)
-	if err != nil {
+	if err != nil || props == nil {
 		c.SetInvalidParamWithErr("channel_search", err)
 		return
 	}
@@ -1138,7 +1138,7 @@ func searchArchivedChannelsForTeam(c *Context, w http.ResponseWriter, r *http.Re
 
 	var props *model.ChannelSearch
 	err := json.NewDecoder(r.Body).Decode(&props)
-	if err != nil {
+	if err != nil || props == nil {
 		c.SetInvalidParamWithErr("channel_search", err)
 		return
 	}
@@ -1172,7 +1172,7 @@ func searchArchivedChannelsForTeam(c *Context, w http.ResponseWriter, r *http.Re
 func searchAllChannels(c *Context, w http.ResponseWriter, r *http.Request) {
 	var props *model.ChannelSearch
 	err := json.NewDecoder(r.Body).Decode(&props)
-	if err != nil {
+	if err != nil || props == nil {
 		c.SetInvalidParamWithErr("channel_search", err)
 		return
 	}
@@ -2236,7 +2236,7 @@ func convertGroupMessageToChannel(c *Context, w http.ResponseWriter, r *http.Req
 	}
 
 	var gmConversionRequest *model.GroupMessageConversionRequestBody
-	if err := json.NewDecoder(r.Body).Decode(&gmConversionRequest); err != nil {
+	if err := json.NewDecoder(r.Body).Decode(&gmConversionRequest); err != nil || gmConversionRequest == nil {
 		c.SetInvalidParamWithErr("body", err)
 		return
 	}

server/channels/api4/channel_local.go

@@ -40,7 +40,7 @@ func (api *API) InitChannelLocal() {
 func localCreateChannel(c *Context, w http.ResponseWriter, r *http.Request) {
 	var channel *model.Channel
 	err := json.NewDecoder(r.Body).Decode(&channel)
-	if err != nil {
+	if err != nil || channel == nil {
 		c.SetInvalidParamWithErr("channel", err)
 		return
 	}
@@ -288,7 +288,7 @@ func localPatchChannel(c *Context, w http.ResponseWriter, r *http.Request) {
 
 	var patch *model.ChannelPatch
 	err := json.NewDecoder(r.Body).Decode(&patch)
-	if err != nil {
+	if err != nil || patch == nil {
 		c.SetInvalidParamWithErr("channel", err)
 		return
 	}

server/channels/api4/channel_test.go

@@ -125,6 +125,14 @@ func TestCreateChannel(t *testing.T) {
 		require.NoError(t, err)
 	})
 
+	t.Run("null value", func(t *testing.T) {
+		var channel *model.Channel
+		_, resp, err = client.CreateChannel(context.Background(), channel)
+
+		require.Error(t, err)
+		CheckBadRequestStatus(t, resp)
+	})
+
 	// Test posting Garbage
 	r, err := client.DoAPIPost(context.Background(), "/channels", "garbage")
 	require.Error(t, err, "expected error")
@@ -319,6 +327,12 @@ func TestPatchChannel(t *testing.T) {
 	client := th.Client
 	team := th.BasicTeam
 
+	var nullPatch *model.ChannelPatch
+
+	_, nullResp, err := client.PatchChannel(context.Background(), th.BasicChannel.Id, nullPatch)
+	require.Error(t, err)
+	CheckBadRequestStatus(t, nullResp)
+
 	patch := &model.ChannelPatch{
 		Name:        new(string),
 		DisplayName: new(string),
@@ -1408,6 +1422,15 @@ func TestSearchChannels(t *testing.T) {
 	defer th.TearDown()
 	client := th.Client
 
+	t.Run("Search using null value", func(t *testing.T) {
+		var nullSearch *model.ChannelSearch
+
+		_, resp, err := client.SearchChannels(context.Background(), th.BasicTeam.Id, nullSearch)
+
+		require.Error(t, err)
+		CheckBadRequestStatus(t, resp)
+	})
+
 	search := &model.ChannelSearch{Term: th.BasicChannel.Name}
 
 	channels, _, err := client.SearchChannels(context.Background(), th.BasicTeam.Id, search)
@@ -1921,6 +1944,15 @@ func TestSearchGroupChannels(t *testing.T) {
 	_, resp, err := client.SearchAllChannels(context.Background(), search)
 	require.Error(t, err)
 	CheckUnauthorizedStatus(t, resp)
+
+	t.Run("search with null value", func(t *testing.T) {
+		var search *model.ChannelSearch
+		client.Login(context.Background(), th.BasicUser.Username, th.BasicUser.Password)
+		_, resp, err := client.SearchGroupChannels(context.Background(), search)
+
+		require.Error(t, err)
+		CheckBadRequestStatus(t, resp)
+	})
 }
 
 func TestDeleteChannel(t *testing.T) {

server/channels/api4/cloud.go

@@ -140,7 +140,7 @@ func changeSubscription(c *Context, w http.ResponseWriter, r *http.Request) {
 	}
 
 	var subscriptionChange *model.SubscriptionChange
-	if err = json.Unmarshal(bodyBytes, &subscriptionChange); err != nil {
+	if err = json.Unmarshal(bodyBytes, &subscriptionChange); err != nil || subscriptionChange == nil {
 		c.Err = model.NewAppError("Api4.changeSubscription", "api.cloud.app_error", nil, "", http.StatusBadRequest).Wrap(err)
 		return
 	}
@@ -219,7 +219,7 @@ func requestCloudTrial(c *Context, w http.ResponseWriter, r *http.Request) {
 	// this value will not be empty when both emails (user admin and CWS customer) are not business email and
 	// a new business email was provided via the request business email modal
 	var startTrialRequest *model.StartCloudTrialRequest
-	if err = json.Unmarshal(bodyBytes, &startTrialRequest); err != nil {
+	if err = json.Unmarshal(bodyBytes, &startTrialRequest); err != nil || startTrialRequest == nil {
 		c.Err = model.NewAppError("Api4.requestCloudTrial", "api.cloud.app_error", nil, "", http.StatusBadRequest).Wrap(err)
 		return
 	}
@@ -261,7 +261,7 @@ func validateBusinessEmail(c *Context, w http.ResponseWriter, r *http.Request) {
 
 	var emailToValidate *model.ValidateBusinessEmailRequest
 	err = json.Unmarshal(bodyBytes, &emailToValidate)
-	if err != nil {
+	if err != nil || emailToValidate == nil {
 		c.Err = model.NewAppError("Api4.requestCloudTrial", "api.cloud.app_error", nil, "", http.StatusBadRequest).Wrap(err)
 		return
 	}
@@ -353,7 +353,7 @@ func getSelfHostedProducts(c *Context, w http.ResponseWriter, r *http.Request) {
 	if !c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionSysconsoleReadBilling) {
 		sanitizedProducts := []model.UserFacingProduct{}
 		err = json.Unmarshal(byteProductsData, &sanitizedProducts)
-		if err != nil {
+		if err != nil || sanitizedProducts == nil {
 			c.Err = model.NewAppError("Api4.getSelfHostedProducts", "api.cloud.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
 			return
 		}
@@ -531,7 +531,7 @@ func updateCloudCustomer(c *Context, w http.ResponseWriter, r *http.Request) {
 	}
 
 	var customerInfo *model.CloudCustomerInfo
-	if err = json.Unmarshal(bodyBytes, &customerInfo); err != nil {
+	if err = json.Unmarshal(bodyBytes, &customerInfo); err != nil || customerInfo == nil {
 		c.Err = model.NewAppError("Api4.updateCloudCustomer", "api.cloud.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
 		return
 	}
@@ -574,7 +574,7 @@ func updateCloudCustomerAddress(c *Context, w http.ResponseWriter, r *http.Reque
 	}
 
 	var address *model.Address
-	if err = json.Unmarshal(bodyBytes, &address); err != nil {
+	if err = json.Unmarshal(bodyBytes, &address); err != nil || address == nil {
 		c.Err = model.NewAppError("Api4.updateCloudCustomerAddress", "api.cloud.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
 		return
 	}
@@ -656,7 +656,7 @@ func confirmCustomerPayment(c *Context, w http.ResponseWriter, r *http.Request)
 	}
 
 	var confirmRequest *model.ConfirmPaymentMethodRequest
-	if err = json.Unmarshal(bodyBytes, &confirmRequest); err != nil {
+	if err = json.Unmarshal(bodyBytes, &confirmRequest); err != nil || confirmRequest == nil {
 		c.Err = model.NewAppError("Api4.confirmCustomerPayment", "api.cloud.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
 		return
 	}
@@ -762,7 +762,7 @@ func handleCWSWebhook(c *Context, w http.ResponseWriter, r *http.Request) {
 	defer r.Body.Close()
 
 	var event *model.CWSWebhookPayload
-	if err = json.Unmarshal(bodyBytes, &event); err != nil {
+	if err = json.Unmarshal(bodyBytes, &event); err != nil || event == nil {
 		c.Err = model.NewAppError("Api4.handleCWSWebhook", "api.cloud.app_error", nil, err.Error(), http.StatusInternalServerError)
 		return
 	}
@@ -881,7 +881,7 @@ func selfServeDeleteWorkspace(c *Context, w http.ResponseWriter, r *http.Request
 	}
 
 	var deleteRequest *model.WorkspaceDeletionRequest
-	if err = json.Unmarshal(bodyBytes, &deleteRequest); err != nil {
+	if err = json.Unmarshal(bodyBytes, &deleteRequest); err != nil || deleteRequest == nil {
 		c.Err = model.NewAppError("Api4.selfServeDeleteWorkspace", "api.cloud.app_error", nil, err.Error(), http.StatusInternalServerError)
 		return
 	}

server/channels/api4/data_retention.go

@@ -347,7 +347,7 @@ func searchChannelsInPolicy(c *Context, w http.ResponseWriter, r *http.Request)
 	c.RequirePolicyId()
 	var props *model.ChannelSearch
 	err := json.NewDecoder(r.Body).Decode(&props)
-	if err != nil {
+	if err != nil || props == nil {
 		c.SetInvalidParamWithErr("channel_search", err)
 		return
 	}

server/channels/api4/group.go

@@ -153,7 +153,7 @@ func createGroup(c *Context, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	var group *model.GroupWithUserIds
-	if err := json.NewDecoder(r.Body).Decode(&group); err != nil {
+	if err := json.NewDecoder(r.Body).Decode(&group); err != nil || group == nil {
 		c.SetInvalidParamWithErr("group", err)
 		return
 	}
@@ -1256,7 +1256,7 @@ func addGroupMembers(c *Context, w http.ResponseWriter, r *http.Request) {
 	}
 
 	var newMembers *model.GroupModifyMembers
-	if err := json.NewDecoder(r.Body).Decode(&newMembers); err != nil {
+	if err := json.NewDecoder(r.Body).Decode(&newMembers); err != nil || newMembers == nil {
 		c.SetInvalidParamWithErr("addGroupMembers", err)
 		return
 	}
@@ -1315,7 +1315,7 @@ func deleteGroupMembers(c *Context, w http.ResponseWriter, r *http.Request) {
 	}
 
 	var deleteBody *model.GroupModifyMembers
-	if err := json.NewDecoder(r.Body).Decode(&deleteBody); err != nil {
+	if err := json.NewDecoder(r.Body).Decode(&deleteBody); err != nil || deleteBody == nil {
 		c.SetInvalidParamWithErr("deleteGroupMembers", err)
 		return
 	}

server/channels/api4/group_test.go

@@ -80,6 +80,10 @@ func TestCreateGroup(t *testing.T) {
 
 	th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuProfessional, "ldap"))
 
+	_, resp, err := th.SystemAdminClient.CreateGroup(context.Background(), nil)
+	require.Error(t, err)
+	CheckBadRequestStatus(t, resp)
+
 	group, _, err := th.SystemAdminClient.CreateGroup(context.Background(), g)
 	require.NoError(t, err)
 
@@ -1582,6 +1586,11 @@ func TestAddMembersToGroup(t *testing.T) {
 
 	th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuProfessional))
 
+	//Empty group members returns bad request
+	_, resp, nullErr := th.SystemAdminClient.UpsertGroupMembers(context.Background(), group.Id, nil)
+	require.Error(t, nullErr)
+	CheckBadRequestStatus(t, resp)
+
 	groupMembers, response, upsertErr := th.SystemAdminClient.UpsertGroupMembers(context.Background(), group.Id, members)
 	require.NoError(t, upsertErr)
 	CheckOKStatus(t, response)
@@ -1654,6 +1663,10 @@ func TestDeleteMembersFromGroup(t *testing.T) {
 
 	th.App.Srv().SetLicense(model.NewTestLicenseSKU(model.LicenseShortSkuProfessional))
 
+	_, resp, nullErr := th.SystemAdminClient.DeleteGroupMembers(context.Background(), group.Id, nil)
+	require.Error(t, nullErr)
+	CheckBadRequestStatus(t, resp)
+
 	groupMembers, response, deleteErr := th.SystemAdminClient.DeleteGroupMembers(context.Background(), group.Id, members)
 	require.NoError(t, deleteErr)
 	CheckOKStatus(t, response)

server/channels/api4/hosted_customer.go

@@ -119,7 +119,7 @@ func selfHostedCustomer(c *Context, w http.ResponseWriter, r *http.Request) {
 	}
 
 	var form *model.SelfHostedCustomerForm
-	if err = json.Unmarshal(bodyBytes, &form); err != nil {
+	if err = json.Unmarshal(bodyBytes, &form); err != nil || form == nil {
 		c.Err = model.NewAppError(where, "api.cloud.app_error", nil, "", http.StatusBadRequest).Wrap(err)
 		return
 	}

server/channels/api4/notify_admin.go

@@ -13,7 +13,7 @@ import (
 func handleNotifyAdmin(c *Context, w http.ResponseWriter, r *http.Request) {
 	var notifyAdminRequest *model.NotifyAdminToUpgradeRequest
 	err := json.NewDecoder(r.Body).Decode(&notifyAdminRequest)
-	if err != nil {
+	if err != nil || notifyAdminRequest == nil {
 		c.SetInvalidParamWithErr("notifyAdminRequest", err)
 		return
 	}
@@ -36,7 +36,7 @@ func handleTriggerNotifyAdminPosts(c *Context, w http.ResponseWriter, r *http.Re
 
 	var notifyAdminRequest *model.NotifyAdminToUpgradeRequest
 	err := json.NewDecoder(r.Body).Decode(&notifyAdminRequest)
-	if err != nil {
+	if err != nil || notifyAdminRequest == nil {
 		c.SetInvalidParamWithErr("notifyAdminRequest", err)
 		return
 	}

server/channels/api4/notify_admin_test.go

@@ -13,6 +13,17 @@ import (
 )
 
 func TestNotifyAdmin(t *testing.T) {
+	t.Run("error when notifying with empty data", func(t *testing.T) {
+		th := Setup(t).InitBasic().InitLogin()
+		defer th.TearDown()
+
+		statusCode, err := th.Client.NotifyAdmin(context.Background(), nil)
+
+		require.Error(t, err)
+		require.Equal(t, http.StatusBadRequest, statusCode)
+
+	})
+
 	t.Run("error when plan is unknown when notifying on upgrade", func(t *testing.T) {
 		th := Setup(t).InitBasic().InitLogin()
 		defer th.TearDown()

server/channels/api4/saml.go

@@ -272,7 +272,7 @@ func resetAuthDataToEmail(c *Context, w http.ResponseWriter, r *http.Request) {
 	}
 	var params *ResetAuthDataParams
 	jsonErr := json.NewDecoder(r.Body).Decode(&params)
-	if jsonErr != nil {
+	if jsonErr != nil || params == nil {
 		c.Err = model.NewAppError("resetAuthDataToEmail", "model.utils.decode_json.app_error", nil, "", http.StatusBadRequest).Wrap(jsonErr)
 		return
 	}

server/channels/api4/system.go

@@ -347,7 +347,7 @@ func queryLogs(c *Context, w http.ResponseWriter, r *http.Request) {
 
 	var logFilter *model.LogFilter
 	err := json.NewDecoder(r.Body).Decode(&logFilter)
-	if err != nil {
+	if err != nil || logFilter == nil {
 		c.Err = model.NewAppError("queryLogs", "api.system.logs.invalidFilter", nil, "", http.StatusInternalServerError)
 		return
 	}