From 6d20289fcead5c079f364bfe1603daa9b19f5428 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 16 Oct 2026 03:12:00 +0100 Subject: [PATCH] fix(auth): answer a deactivated user's reset token like an unknown token POST /api/auth/reset-password returned 403 "account is deactivated" for a live, unexpired reset token of an admin-deactivated user, while unknown, used and expired tokens get 400. It was the only response that revealed the deactivated state, and it disagreed with the GET status endpoint, which already reports such a token as "used" (#421). ConfirmPasswordReset still refuses the reset (A03-006) and still consumes the token, but now returns ErrInvalidResetToken, the same sentinel the unknown-token paths use, so the handler maps it to the same 400 body. ErrAccountDeactivated and its 403 mapping are removed. Closes #595 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_auth.go | 3 -- internal/api/handler_auth_test.go | 20 ++++---- internal/auth/errors.go | 9 ++-- internal/auth/service_credentials_db_test.go | 2 +- internal/auth/service_password.go | 12 +++-- internal/auth/service_password_test.go | 4 +- .../reset_token_status_deactivated_test.go | 49 +++++++++++++------ 7 files changed, 60 insertions(+), 39 deletions(-) diff --git a/internal/api/handler_auth.go b/internal/api/handler_auth.go index b764724ab..c87bbc35a 100644 --- a/internal/api/handler_auth.go +++ b/internal/api/handler_auth.go @@ -372,9 +372,6 @@ func (h *Handler) resetPassword(ctx context.Context, req *events.LambdaFunctionU pwdResetReq.NewPassword = decoded if err := h.auth.ConfirmPasswordReset(ctx, pwdResetReq); err != nil { - if errors.Is(err, auth.ErrAccountDeactivated) { - return nil, NewClientError(403, err.Error()) - } if errors.Is(err, auth.ErrUserChanged) { return nil, NewClientError(409, err.Error()) } diff --git a/internal/api/handler_auth_test.go b/internal/api/handler_auth_test.go index c25be6b1b..9135bd34e 100644 --- a/internal/api/handler_auth_test.go +++ b/internal/api/handler_auth_test.go @@ -732,18 +732,16 @@ func TestHandler_resetPassword_Error(t *testing.T) { assert.Contains(t, ce.Error(), "invalid or expired reset token") } -// TestHandler_resetPassword_AccountDeactivated is the regression test for -// issue #89 / audit finding A03-006: ConfirmPasswordReset returning -// auth.ErrAccountDeactivated must map to 403, ahead of the generic -// isResetPasswordClientError(400) path -- deleting that branch in the -// handler would leave every OTHER resetPassword test green while this one -// alone catches the regression. -func TestHandler_resetPassword_AccountDeactivated(t *testing.T) { +// TestHandler_resetPassword_InvalidResetToken: the service answers a +// deactivated user's live token with auth.ErrInvalidResetToken (A03-006, +// issue #595), which must map to the same 400 and message as an unknown +// token rather than a 500 or a status that reveals account state. +func TestHandler_resetPassword_InvalidResetToken(t *testing.T) { ctx := context.Background() mockAuth := new(MockAuthService) t.Cleanup(func() { mockAuth.AssertExpectations(t) }) - mockAuth.On("ConfirmPasswordReset", ctx, mock.Anything).Return(auth.ErrAccountDeactivated) + mockAuth.On("ConfirmPasswordReset", ctx, mock.Anything).Return(auth.ErrInvalidResetToken) handler := &Handler{auth: mockAuth} @@ -754,9 +752,9 @@ func TestHandler_resetPassword_AccountDeactivated(t *testing.T) { assert.Nil(t, result) ce, ok := IsClientError(err) - require.True(t, ok, "ErrAccountDeactivated must be wrapped as a client error, not a 500") - assert.Equal(t, 403, ce.code) - assert.Contains(t, ce.Error(), "deactivated") + require.True(t, ok, "ErrInvalidResetToken must be wrapped as a client error, not a 500") + assert.Equal(t, 400, ce.code) + assert.Equal(t, "invalid or expired reset token", ce.Error()) } // A reset that lost a race to a concurrent account change is a 409, not a 500 (issue #493). diff --git a/internal/auth/errors.go b/internal/auth/errors.go index a6868026a..99dda088c 100644 --- a/internal/auth/errors.go +++ b/internal/auth/errors.go @@ -86,10 +86,11 @@ var ( // than the most-permissive default. Mapped to 400 (issue #61). ErrEmptyAPIKeyPermissions = errors.New("at least one permission is required") - // ErrAccountDeactivated: ConfirmPasswordReset refused an admin-deactivated - // account (A03-006). Safe to name, since the caller already holds a valid - // reset token mailed to the account. - ErrAccountDeactivated = errors.New("account is deactivated") + // ErrInvalidResetToken: the reset token is unknown, already used, or + // belongs to an admin-deactivated account (A03-006). The deactivated case + // shares this error so the reset endpoint never reveals account state + // (issue #595). Mapped to 400. + ErrInvalidResetToken = errors.New("invalid or expired reset token") // ErrUserChanged: a field-scoped user write found the row changed since the // caller read it (issue #474). Mapped to 409. diff --git a/internal/auth/service_credentials_db_test.go b/internal/auth/service_credentials_db_test.go index c0c2dc6cd..833db8837 100644 --- a/internal/auth/service_credentials_db_test.go +++ b/internal/auth/service_credentials_db_test.go @@ -413,7 +413,7 @@ func TestIntegration_ResetConfirmRejectsStaleRead(t *testing.T) { err := f.svc.ConfirmPasswordReset(ctx, PasswordResetConfirm{Token: token, NewPassword: credentialRaceNew}) require.ErrorIs(t, err, ErrUserChanged) assert.Equal(t, winner(), f.stored()) - require.ErrorIs(t, f.svc.ConfirmPasswordReset(ctx, PasswordResetConfirm{Token: token, NewPassword: credentialRaceNew}), ErrAccountDeactivated) + require.ErrorIs(t, f.svc.ConfirmPasswordReset(ctx, PasswordResetConfirm{Token: token, NewPassword: credentialRaceNew}), ErrInvalidResetToken) assert.Empty(t, f.stored().PasswordResetToken) }) } diff --git a/internal/auth/service_password.go b/internal/auth/service_password.go index ca1be6ccc..3be2d3027 100644 --- a/internal/auth/service_password.go +++ b/internal/auth/service_password.go @@ -391,12 +391,14 @@ func (s *Service) ConfirmPasswordReset(ctx context.Context, req PasswordResetCon readResetToken, readPasswordHash := user.PasswordResetToken, user.PasswordHash // An admin-deactivated account must not reactivate itself through a reset - // (A03-006). The token is still consumed so the link cannot be replayed. + // (A03-006). The token is still consumed so the link cannot be replayed, + // and the caller gets the unknown-token error so the response does not + // reveal that the account is deactivated (issue #595). if user.DeactivatedAt != nil { if updateErr := s.store.ConsumePasswordResetToken(ctx, user.ID, readResetToken); updateErr != nil { logging.Warnf("Failed to invalidate reset token for deactivated user %s: %v", user.ID, updateErr) } - return ErrAccountDeactivated + return ErrInvalidResetToken } if err := s.processPasswordReset(user, req.NewPassword); err != nil { @@ -514,17 +516,17 @@ func (s *Service) validateResetToken(ctx context.Context, token string) (*User, user, err := s.store.GetUserByResetToken(ctx, tokenHash) if err != nil { if errors.Is(err, pgx.ErrNoRows) { - return nil, fmt.Errorf("invalid or expired reset token") + return nil, ErrInvalidResetToken } return nil, err } if user == nil { - return nil, fmt.Errorf("invalid or expired reset token") + return nil, ErrInvalidResetToken } // Constant-time comparison after fetch to close the SQL-equality timing oracle. if subtle.ConstantTimeCompare([]byte(user.PasswordResetToken), []byte(tokenHash)) != 1 { - return nil, fmt.Errorf("invalid or expired reset token") + return nil, ErrInvalidResetToken } if user.PasswordResetExpiry == nil || time.Now().After(*user.PasswordResetExpiry) { diff --git a/internal/auth/service_password_test.go b/internal/auth/service_password_test.go index 1746a344e..6c85eddfc 100644 --- a/internal/auth/service_password_test.go +++ b/internal/auth/service_password_test.go @@ -697,7 +697,9 @@ func TestService_ConfirmPasswordReset(t *testing.T) { err := service.ConfirmPasswordReset(ctx, req) require.Error(t, err) - assert.ErrorIs(t, err, ErrAccountDeactivated) + // Same error as an unknown token so account state is not revealed (issue #595). + assert.ErrorIs(t, err, ErrInvalidResetToken) + assert.Equal(t, "invalid or expired reset token", err.Error()) // Active must stay false: this is the exact regression this test // guards against. assert.False(t, deactivatedUser.Active) diff --git a/internal/server/reset_token_status_deactivated_test.go b/internal/server/reset_token_status_deactivated_test.go index b90cd3136..4dd97671a 100644 --- a/internal/server/reset_token_status_deactivated_test.go +++ b/internal/server/reset_token_status_deactivated_test.go @@ -87,22 +87,14 @@ func TestHandleRequest_ResetPasswordStatus_DeactivatedUserLooksLikeUnknownToken( "deactivated token must be indistinguishable from an unknown token") } -func TestHandleRequest_ConfirmPasswordReset_DeactivatedUserRefused(t *testing.T) { - ctx := context.Background() - handler, mockStore := newResetTokenHarness(t) - - user := deactivatedResetUser() - mockStore.On("GetUserByResetToken", mock.Anything, hashSessionTokenForTest(deactivatedResetRawToken)). - Return(user, nil).Once() - mockStore.On("ConsumePasswordResetToken", mock.Anything, user.ID, user.PasswordResetToken). - Return(nil).Once() - +func resetConfirmRequest(t *testing.T, token string) *events.LambdaFunctionURLRequest { + t.Helper() body, err := json.Marshal(map[string]string{ - "token": deactivatedResetRawToken, + "token": token, "new_password": base64.StdEncoding.EncodeToString([]byte("Str0ng-Passw0rd-For-Test!")), }) require.NoError(t, err) - resp, err := handler.HandleRequest(ctx, &events.LambdaFunctionURLRequest{ + return &events.LambdaFunctionURLRequest{ Headers: map[string]string{"Content-Type": "application/json"}, Body: string(body), RequestContext: events.LambdaFunctionURLRequestContext{ @@ -111,7 +103,36 @@ func TestHandleRequest_ConfirmPasswordReset_DeactivatedUserRefused(t *testing.T) Path: "/api/auth/reset-password", }, }, - }) + } +} + +// Issue #595: the confirm endpoint refuses a deactivated user's live token +// (A03-006) with the same response as an unknown token, so it does not +// reveal that the account is deactivated. The token is still consumed (the +// ConsumePasswordResetToken expectation) and no password write happens (any +// other store call would fail the mock). +func TestHandleRequest_ConfirmPasswordReset_DeactivatedUserLooksLikeUnknownToken(t *testing.T) { + ctx := context.Background() + handler, mockStore := newResetTokenHarness(t) + + user := deactivatedResetUser() + mockStore.On("GetUserByResetToken", mock.Anything, hashSessionTokenForTest(deactivatedResetRawToken)). + Return(user, nil).Once() + mockStore.On("ConsumePasswordResetToken", mock.Anything, user.ID, user.PasswordResetToken). + Return(nil).Once() + mockStore.On("GetUserByResetToken", mock.Anything, hashSessionTokenForTest("unknown-token")). + Return((*auth.User)(nil), pgx.ErrNoRows).Once() + + deactivated, err := handler.HandleRequest(ctx, resetConfirmRequest(t, deactivatedResetRawToken)) require.NoError(t, err) - assert.Equal(t, 403, resp.StatusCode, "body: %s", resp.Body) + unknown, err := handler.HandleRequest(ctx, resetConfirmRequest(t, "unknown-token")) + require.NoError(t, err) + + require.Equal(t, 400, unknown.StatusCode, "body: %s", unknown.Body) + assert.Equal(t, unknown.StatusCode, deactivated.StatusCode, "body: %s", deactivated.Body) + assert.Equal(t, unknown.Headers, deactivated.Headers, + "deactivated token must be indistinguishable from an unknown token") + assert.JSONEq(t, unknown.Body, deactivated.Body, + "deactivated token must be indistinguishable from an unknown token") + assert.NotContains(t, deactivated.Body, "deactivated") }