From 2cfae4324d13378e7e7a5463c9b61a4be45984ba Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 3 Jun 2026 15:24:47 +0200 Subject: [PATCH] fix(profile): return sentinel errors for wrong password and duplicate email On the profile-update path, wrong current password and duplicate email both fell through to the generic 500 "Internal server error" because UpdateUserProfile returned plain fmt.Errorf strings that the handler did not recognise as client errors. - Add ErrCurrentPasswordIncorrect sentinel to internal/auth/errors.go. - Return the sentinel (not a plain string) from UpdateUserProfile when the current password does not match. - Return the existing ErrEmailInUse sentinel (not a plain string) from updateUserEmail when the address is taken. - Add mapProfileUpdateError in handler_auth.go: wrong password -> 401 with the sentinel message; duplicate email -> 409 with the neutral privacy-safe message "Unable to update email" (does not confirm another account's existence, preventing enumeration). - Add handler tests for both new error paths asserting the correct status code and message. - Add service-layer tests asserting errors.Is on both sentinels so the contract is positively enforced. Closes #929 --- internal/api/handler_auth.go | 22 ++++++++++- internal/api/handler_auth_test.go | 62 ++++++++++++++++++++++++++++++ internal/auth/errors.go | 6 +++ internal/auth/service_user.go | 4 +- internal/auth/service_user_test.go | 37 +++++++++++++++++- 5 files changed, 127 insertions(+), 4 deletions(-) diff --git a/internal/api/handler_auth.go b/internal/api/handler_auth.go index 5d0509578..a039c59b4 100644 --- a/internal/api/handler_auth.go +++ b/internal/api/handler_auth.go @@ -394,12 +394,32 @@ func (h *Handler) updateProfile(ctx context.Context, req *events.LambdaFunctionU // Update profile through auth service if err := h.auth.UpdateUserProfile(ctx, session.UserID, profileReq.Email, currentPassword, newPassword); err != nil { - return nil, err + return nil, mapProfileUpdateError(err) } return map[string]string{"status": "profile updated"}, nil } +// mapProfileUpdateError converts profile-update service errors to the correct +// HTTP status code and a user-facing message. +// +// - ErrCurrentPasswordIncorrect -> 401 with a precise message (the acting +// user is verifying their own credential, so specificity is safe). +// - ErrEmailInUse -> 409 with a neutral message that does NOT confirm whether +// another account holds the address; prevents account enumeration via the +// profile-update path (issue #929). +// - All other errors pass through unchanged for handleRequestError to render +// as 500. +func mapProfileUpdateError(err error) error { + switch { + case errors.Is(err, auth.ErrCurrentPasswordIncorrect): + return NewClientError(401, err.Error()) + case errors.Is(err, auth.ErrEmailInUse): + return NewClientError(409, "Unable to update email") + } + return err +} + // decodeProfilePasswords decodes the optional base64-encoded current and new // passwords from a ProfileUpdateRequest. Pulled out of updateProfile to keep // that function under the cyclomatic limit. diff --git a/internal/api/handler_auth_test.go b/internal/api/handler_auth_test.go index 29a17fd39..cdcb623f1 100644 --- a/internal/api/handler_auth_test.go +++ b/internal/api/handler_auth_test.go @@ -947,6 +947,68 @@ func TestHandler_updateProfile_AcceptsValidEmail(t *testing.T) { mockAuth.AssertCalled(t, "UpdateUserProfile", ctx, "12345678-1234-1234-1234-123456789abc", "new@example.com", "", "") } +// TestHandler_updateProfile_WrongCurrentPassword verifies that a wrong current +// password returned by the auth service is surfaced as 401, not a generic 500 +// (issue #929). The acting user is checking their own credential so a precise +// message is safe and helpful. +func TestHandler_updateProfile_WrongCurrentPassword(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + session := &Session{UserID: "12345678-1234-1234-1234-123456789abc"} + mockAuth.On("ValidateSession", ctx, "test-token").Return(session, nil) + mockAuth.On("UpdateUserProfile", mock.MatchedBy(func(c context.Context) bool { return c != nil }), "12345678-1234-1234-1234-123456789abc", "", "wrongpass", ""). + Return(auth.ErrCurrentPasswordIncorrect) + + handler := &Handler{auth: mockAuth} + + encoded := base64.StdEncoding.EncodeToString([]byte("wrongpass")) + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer test-token"}, + Body: `{"email": "", "current_password": "` + encoded + `"}`, + } + + _, err := handler.updateProfile(ctx, req) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "wrong-password error must be a ClientError, not a 500") + assert.Equal(t, 401, ce.code) + assert.Equal(t, auth.ErrCurrentPasswordIncorrect.Error(), ce.message) +} + +// TestHandler_updateProfile_DuplicateEmail verifies that a duplicate-email +// conflict is surfaced as 409 with a privacy-preserving message that does NOT +// confirm whether another account holds the address (issue #929). +func TestHandler_updateProfile_DuplicateEmail(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + session := &Session{UserID: "12345678-1234-1234-1234-123456789abc"} + mockAuth.On("ValidateSession", ctx, "test-token").Return(session, nil) + mockAuth.On("UpdateUserProfile", mock.MatchedBy(func(c context.Context) bool { return c != nil }), "12345678-1234-1234-1234-123456789abc", "taken@example.com", "mypass", ""). + Return(auth.ErrEmailInUse) + + handler := &Handler{auth: mockAuth} + + encoded := base64.StdEncoding.EncodeToString([]byte("mypass")) + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer test-token"}, + Body: `{"email": "taken@example.com", "current_password": "` + encoded + `"}`, + } + + _, err := handler.updateProfile(ctx, req) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "duplicate-email error must be a ClientError, not a 500") + assert.Equal(t, 409, ce.code) + // Must NOT expose the raw ErrEmailInUse message which would confirm another + // account exists. The privacy-preserving message is "Unable to update email". + assert.Equal(t, "Unable to update email", ce.message) + assert.NotContains(t, ce.message, "already in use", "response must not confirm another account's existence") +} + // TestHandler_resetPassword_DecodesBase64 verifies issue #356: the // resetPassword handler must base64-decode new_password before forwarding to // the service, matching the pattern used by login / change-password / diff --git a/internal/auth/errors.go b/internal/auth/errors.go index 4696cc2ec..3a70fea4b 100644 --- a/internal/auth/errors.go +++ b/internal/auth/errors.go @@ -34,6 +34,12 @@ var ( // permission. Mapped to 403 (issue #907). ErrSelfEscalation = errors.New("cannot escalate your own group membership") + // ErrCurrentPasswordIncorrect is returned by UpdateUserProfile when the + // caller-supplied current password does not match the stored hash. Mapped + // to 401 at the API layer (the acting user is verifying their own + // credential, so a precise message is safe -- issue #929). + ErrCurrentPasswordIncorrect = errors.New("Current password is incorrect") + // MFA login-gate sentinels — used by the login API handler to map // to machine-readable response codes (mfa_required / // invalid_mfa_code) so the frontend can branch on the error class diff --git a/internal/auth/service_user.go b/internal/auth/service_user.go index 6bb0d1546..dc581362c 100644 --- a/internal/auth/service_user.go +++ b/internal/auth/service_user.go @@ -452,7 +452,7 @@ func (s *Service) UpdateUserProfile(ctx context.Context, userID string, email st } if !s.verifyPassword(currentPassword, user.PasswordHash) { - return fmt.Errorf("current password is incorrect") + return ErrCurrentPasswordIncorrect } if err := s.updateUserEmail(ctx, user, email); err != nil { @@ -492,7 +492,7 @@ func (s *Service) updateUserEmail(ctx context.Context, user *User, email string) return err } if existing != nil { - return fmt.Errorf("email already in use") + return ErrEmailInUse } user.Email = email } diff --git a/internal/auth/service_user_test.go b/internal/auth/service_user_test.go index 2f3721e52..f8c110843 100644 --- a/internal/auth/service_user_test.go +++ b/internal/auth/service_user_test.go @@ -2,6 +2,7 @@ package auth import ( "context" + "errors" "fmt" "testing" "time" @@ -794,7 +795,41 @@ func TestService_UpdateUserProfile(t *testing.T) { err := service.UpdateUserProfile(ctx, "user-123", "new@example.com", "WrongPassword", "SecureTest@456") assert.Error(t, err) - assert.Contains(t, err.Error(), "current password is incorrect") + // The handler must receive the sentinel so it can produce a 401 instead + // of a 500. Assert errors.Is in addition to the string check (issue #929). + assert.True(t, errors.Is(err, ErrCurrentPasswordIncorrect), + "UpdateUserProfile wrong-password must return ErrCurrentPasswordIncorrect sentinel") + + mockStore.AssertExpectations(t) + }) + + t.Run("duplicate email returns ErrEmailInUse sentinel", func(t *testing.T) { + // Verifies issue #929: updateUserEmail must return the ErrEmailInUse + // sentinel (not a plain fmt.Errorf string) so the API handler can map + // it to a 409 with a privacy-preserving message. + mockStore := new(MockStore) + mockEmail := new(MockEmailSender) + service := createTestService(mockStore, mockEmail) + + hash, _ := bcrypt.GenerateFromPassword([]byte("OldPassword123"), bcrypt.DefaultCost) + testUser := &User{ + ID: "user-123", + Email: "old@example.com", + PasswordHash: string(hash), + Active: true, + } + otherUser := &User{ + ID: "other-456", + Email: "taken@example.com", + } + + mockStore.On("GetUserByID", ctx, "user-123").Return(testUser, nil).Once() + mockStore.On("GetUserByEmail", ctx, "taken@example.com").Return(otherUser, nil).Once() + + err := service.UpdateUserProfile(ctx, "user-123", "taken@example.com", "OldPassword123", "") + assert.Error(t, err) + assert.True(t, errors.Is(err, ErrEmailInUse), + "UpdateUserProfile duplicate-email must return ErrEmailInUse sentinel") mockStore.AssertExpectations(t) })