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) })