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