From d61364a24aab25bbdecc93fca718f24cf5631e0b Mon Sep 17 00:00:00 2001 From: Delaney Sylvans Date: Thu, 5 Oct 2023 14:55:59 +0100 Subject: [PATCH] [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 --- server/channels/api4/bot.go | 4 +- server/channels/api4/bot_test.go | 45 +++++++++++++++++++++++ server/channels/api4/channel.go | 14 +++---- server/channels/api4/channel_local.go | 4 +- server/channels/api4/channel_test.go | 32 ++++++++++++++++ server/channels/api4/cloud.go | 18 ++++----- server/channels/api4/data_retention.go | 2 +- server/channels/api4/group.go | 6 +-- server/channels/api4/group_test.go | 13 +++++++ server/channels/api4/hosted_customer.go | 2 +- server/channels/api4/notify_admin.go | 4 +- server/channels/api4/notify_admin_test.go | 11 ++++++ server/channels/api4/saml.go | 2 +- server/channels/api4/system.go | 2 +- 14 files changed, 130 insertions(+), 29 deletions(-) diff --git a/server/channels/api4/bot.go b/server/channels/api4/bot.go index 6c6858a75c..e0ca6016f4 100644 --- a/server/channels/api4/bot.go +++ b/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 } diff --git a/server/channels/api4/bot_test.go b/server/channels/api4/bot_test.go index dd97d58179..6c9eea4ed0 100644 --- a/server/channels/api4/bot_test.go +++ b/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) { diff --git a/server/channels/api4/channel.go b/server/channels/api4/channel.go index a93210a7cd..b703196a8a 100644 --- a/server/channels/api4/channel.go +++ b/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 } diff --git a/server/channels/api4/channel_local.go b/server/channels/api4/channel_local.go index 65f6e204cb..dd0e1c1feb 100644 --- a/server/channels/api4/channel_local.go +++ b/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 } diff --git a/server/channels/api4/channel_test.go b/server/channels/api4/channel_test.go index 37380d1a8b..3bb34555e5 100644 --- a/server/channels/api4/channel_test.go +++ b/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) { diff --git a/server/channels/api4/cloud.go b/server/channels/api4/cloud.go index 5eddeb72b8..5d80b4bc8b 100644 --- a/server/channels/api4/cloud.go +++ b/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 } diff --git a/server/channels/api4/data_retention.go b/server/channels/api4/data_retention.go index 78fad0afba..64d7053e23 100644 --- a/server/channels/api4/data_retention.go +++ b/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 } diff --git a/server/channels/api4/group.go b/server/channels/api4/group.go index d04cf25b23..4be9d5989c 100644 --- a/server/channels/api4/group.go +++ b/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 } diff --git a/server/channels/api4/group_test.go b/server/channels/api4/group_test.go index 56a9db6f51..49a31fa85b 100644 --- a/server/channels/api4/group_test.go +++ b/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) diff --git a/server/channels/api4/hosted_customer.go b/server/channels/api4/hosted_customer.go index f063b30c60..1c962a663f 100644 --- a/server/channels/api4/hosted_customer.go +++ b/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 } diff --git a/server/channels/api4/notify_admin.go b/server/channels/api4/notify_admin.go index fd35b82759..be87b3d7fc 100644 --- a/server/channels/api4/notify_admin.go +++ b/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(¬ifyAdminRequest) - 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(¬ifyAdminRequest) - if err != nil { + if err != nil || notifyAdminRequest == nil { c.SetInvalidParamWithErr("notifyAdminRequest", err) return } diff --git a/server/channels/api4/notify_admin_test.go b/server/channels/api4/notify_admin_test.go index 3b144fc75b..a9ceb6ed61 100644 --- a/server/channels/api4/notify_admin_test.go +++ b/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() diff --git a/server/channels/api4/saml.go b/server/channels/api4/saml.go index 479431274f..4a2e4a2452 100644 --- a/server/channels/api4/saml.go +++ b/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(¶ms) - 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 } diff --git a/server/channels/api4/system.go b/server/channels/api4/system.go index c00a0c0a48..2ff8c80e75 100644 --- a/server/channels/api4/system.go +++ b/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 }