From 28fa8b15253fddc35bfab47cdc8de96cd9ee9b4d Mon Sep 17 00:00:00 2001 From: kolaente Date: Tue, 3 Nov 2026 19:08:47 +0100 Subject: [PATCH] fix(user): verify the authenticated account's password on email change The password was checked through the login lookup by username or email, which could resolve to a different account, and the email of that account was then changed. Load the account by its ID and check its own password, keeping the re-authentication throttle and the lockout counter. --- pkg/user/update_email.go | 18 ++++++++++++++---- pkg/user/update_email_test.go | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/pkg/user/update_email.go b/pkg/user/update_email.go index 0f8c29130..f1f1c9cfd 100644 --- a/pkg/user/update_email.go +++ b/pkg/user/update_email.go @@ -50,15 +50,25 @@ func EmailUpdateMessage() string { // (kicking off confirmation when the mailer is enabled). Shared by the v1 and // v2 email-update handlers; only HTTP input binding stays in the handlers. func ChangeUserEmail(ctx context.Context, s *xorm.Session, u *User, password, newEmail string) error { - var verified *User - err := ThrottleReauth(u.ID, func() (err error) { - verified, err = CheckUserCredentials(ctx, s, &Login{Username: u.Username, Password: password}) + if password == "" { + return ErrNoUsernamePassword{} + } + + target, err := GetUserByID(s, u.ID) + if err != nil { return err + } + if !target.IsLocalUser() { + return &ErrAccountIsNotLocal{UserID: target.ID} + } + + err = ThrottleReauth(target.ID, func() error { + return CheckUserPasswordCountingFailures(ctx, s, target, password) }) if err != nil { return err } - return UpdateEmail(s, &EmailUpdate{User: verified, NewEmail: newEmail}) + return UpdateEmail(s, &EmailUpdate{User: target, NewEmail: newEmail}) } // UpdateEmail stages the address as PendingEmail until confirmed; with the mailer off it applies immediately. diff --git a/pkg/user/update_email_test.go b/pkg/user/update_email_test.go index 0446ff8ac..fd36b9d00 100644 --- a/pkg/user/update_email_test.go +++ b/pkg/user/update_email_test.go @@ -310,3 +310,36 @@ func TestResendEmailConfirmation(t *testing.T) { assert.Equal(t, before[0].Token, after[0].Token) }) } + +func TestChangeUserEmail(t *testing.T) { + t.Run("verifies the password of the authenticated account", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + require.NoError(t, UpdateUserPassword(s, &User{ID: 2}, "user2password")) + + // The username points at another account, e.g. a stale token claim. + err := ChangeUserEmail(context.Background(), s, &User{ID: 1, Username: "user2"}, "user2password", "new1@example.com") + require.Error(t, err) + assert.True(t, IsErrWrongUsernameOrPassword(err)) + + s2 := db.NewSession() + defer s2.Close() + updated, err := GetUserWithEmail(s2, &User{ID: 1}) + require.NoError(t, err) + assert.Equal(t, "user1@example.com", updated.Email) + assert.Empty(t, updated.PendingEmail) + }) + t.Run("changes the email", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + require.NoError(t, ChangeUserEmail(context.Background(), s, &User{ID: 1}, "12345678", "new1@example.com")) + + updated, err := GetUserWithEmail(s, &User{ID: 1}) + require.NoError(t, err) + assert.Equal(t, "new1@example.com", updated.Email) + }) +}