Fix nil pointer dereference in UpdateUser (MATTERMOST-SERVER-VF) (#35717)

* Fix nil pointer dereference in UpdateUser after store update

Add nil check on userUpdate result from userService.UpdateUser to prevent
panic when the store returns nil unexpectedly. This fixes a nil pointer
dereference that occurs when accessing userUpdate.New after the store
update call.

Sentry: MATTERMOST-SERVER-VF (14 events)

Co-authored-by: Claude <claude@anthropic.com>

* Add unit test for nil userUpdate guard in UpdateUser

Test verifies that when the store returns (nil, nil) from Update,
the app layer returns an appropriate error instead of panicking
with a nil pointer dereference.

Co-authored-by: Claude <claude@anthropic.com>

* fix: gofmt user_test.go

Co-authored-by: Claude <claude@anthropic.com>

* fix: split nil checks per review feedback, add parallel test execution

Separate userUpdate==nil from userUpdate.New==nil with distinct error
detail strings for easier debugging. Add mainHelper.Parallel(t) to test
for consistency with other mock-based tests.

Addresses review feedback from @JulienTant and @coderabbitai.

Co-authored-by: Claude <claude@anthropic.com>

---------

Co-authored-by: Claude <claude@anthropic.com>
This commit is contained in:
Pavel Zeman
2026-03-23 16:48:22 -04:00
committed by GitHub
co-authored by Claude
parent 2ce50d7c8d
commit f04c3f0071
2 changed files with 43 additions and 0 deletions
+7
View File
@@ -1504,6 +1504,13 @@ func (a *App) UpdateUser(rctx request.CTX, user *model.User, sendNotifications b
}
}
if userUpdate == nil {
return nil, model.NewAppError("UpdateUser", "app.user.update.find.app_error", nil, "received nil update result from store for userId="+user.Id, http.StatusInternalServerError)
}
if userUpdate.New == nil {
return nil, model.NewAppError("UpdateUser", "app.user.update.find.app_error", nil, "received update result with nil New user from store for userId="+user.Id, http.StatusInternalServerError)
}
newUser := userUpdate.New
if (newUser.Username != userUpdate.Old.Username) && (newUser.LastPictureUpdate <= 0) {
+36
View File
@@ -24,6 +24,7 @@ import (
"github.com/mattermost/mattermost/server/public/model"
"github.com/mattermost/mattermost/server/public/shared/request"
oauthgitlab "github.com/mattermost/mattermost/server/v8/channels/app/oauthproviders/gitlab"
"github.com/mattermost/mattermost/server/v8/channels/app/users"
"github.com/mattermost/mattermost/server/v8/channels/store"
storemocks "github.com/mattermost/mattermost/server/v8/channels/store/storetest/mocks"
"github.com/mattermost/mattermost/server/v8/channels/utils/testutils"
@@ -269,6 +270,41 @@ func TestUpdateUser(t *testing.T) {
})
}
func TestUpdateUserNilUpdateResult(t *testing.T) {
mainHelper.Parallel(t)
th := SetupWithStoreMock(t)
fakeUserID := model.NewId()
mockUser := &model.User{
Id: fakeUserID,
Username: "testuser",
Email: "test@example.com",
}
mockUserStore := storemocks.UserStore{}
mockUserStore.On("Get", mock.Anything, mock.Anything).Return(mockUser, nil)
// Simulate a store that returns (nil, nil) — no error but no result
mockUserStore.On("Update", mock.Anything, mock.Anything, mock.Anything).Return(nil, nil)
mockSessionStore := storemocks.SessionStore{}
mockOAuthStore := storemocks.OAuthStore{}
var err error
th.App.ch.srv.userService, err = users.New(users.ServiceConfig{
UserStore: &mockUserStore,
SessionStore: &mockSessionStore,
OAuthStore: &mockOAuthStore,
ConfigFn: th.App.ch.srv.platform.Config,
LicenseFn: th.App.ch.srv.License,
})
require.NoError(t, err)
updatedUser, appErr := th.App.UpdateUser(th.Context, mockUser, false)
require.Nil(t, updatedUser, "expected nil user when store returns nil update")
require.NotNil(t, appErr, "expected error when store returns nil update")
require.Equal(t, "app.user.update.find.app_error", appErr.Id)
}
func TestUpdateUserMissingFields(t *testing.T) {
mainHelper.Parallel(t)
th := Setup(t)