From f70158096a522785c6535058fb2cd25138dad5d7 Mon Sep 17 00:00:00 2001 From: J3vb <192430104+J3vb@users.noreply.github.com> Date: Sun, 30 Aug 2026 10:25:19 +0200 Subject: [PATCH] fix(service): verify-totp reports a store fault as 500, uncounted (OC-0377) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit challengeSecret folded a GetUserByID error into the same 401 "invalid or expired two-factor challenge" an expired challenge earns, and the attempt had already been charged to the per-user totp_fail cap. Split the outcome: a store error logs and returns the new service.ErrTOTPUnavailable (ErrInternal, "two-factor verification temporarily unavailable"); an unknown user or a missing secret still answers 401. The limiter reservation moves after the store read — the rule authenticate already applies — and still precedes the code compare, so the check-then-act it closes stays closed. Characterization row flipped in the same commit: `VerifyTOTPFailurePaths/ user lookup fails -> 500, challenge kept, attempt not counted` — after the fault ten wrong codes still answer 401 (the tenth would be 429 had the fault counted), then the eleventh is refused. `per-user failure cap spans challenges` unchanged. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01A17Uq3d2C36rN82Jitf3wo --- Server/api/auth_characterization_test.go | 29 ++++++++++++++++++------ Server/service/auth.go | 27 ++++++++++++++++------ 2 files changed, 42 insertions(+), 14 deletions(-) diff --git a/Server/api/auth_characterization_test.go b/Server/api/auth_characterization_test.go index ae15ca72..5d15b185 100644 --- a/Server/api/auth_characterization_test.go +++ b/Server/api/auth_characterization_test.go @@ -777,16 +777,31 @@ func TestAuthCharacterization_VerifyTOTPFailurePaths(t *testing.T) { } const challengeGone = "invalid or expired two-factor challenge" - t.Run("user lookup fails -> 401", func(t *testing.T) { + t.Run("user lookup fails -> 500, challenge kept, attempt not counted", func(t *testing.T) { database, router, _, secret := setup(t) pt := loginPartial(t, router, "two", "correctPass1", "", "") hideTable(t, database, "users") - // known: totpChallengeSecret folds a database error into the same 401 - // an expired challenge gets, so a DB fault during the second factor - // reads as a bad code and burns the caller's attempt budget (ledger - // OC-0377). The plan's rule is 5xx for a non-sentinel error; pinned - // as-is for B3-2, fixed in B3-9. - wantErr(t, verify(t, router, pt, totpCode(t, secret), ""), http.StatusUnauthorized, "UNAUTHORIZED", challengeGone) + // OC-0377 (fixed in B3-9): a store fault while loading the challenged + // user is an outage, not a bad challenge — 5xx, the challenge stays + // live, and the attempt is not charged to the per-user totp_fail cap. + wantErr(t, verify(t, router, pt, totpCode(t, secret), ""), http.StatusInternalServerError, "INTERNAL_ERROR", "two-factor verification temporarily unavailable") + if _, err := database.ExecContext(context.Background(), `ALTER TABLE users_gone RENAME TO users`); err != nil { + t.Fatalf("restore users: %v", err) + } + // The cap is 10 per user: ten wrong codes must all still answer 401 + // "invalid two-factor code" — had the faulted attempt counted, the + // tenth would be the 429. The first five ride the surviving challenge + // (which proves it was kept), the next five a fresh one. + attempt := 0 + for _, token := range []string{pt, loginPartial(t, router, "two", "correctPass1", "", "")} { + for range 5 { + attempt++ + wantErr(t, verify(t, router, token, wrongTOTPCode(t, secret), fmt.Sprintf("203.0.113.%d", attempt)), http.StatusUnauthorized, "UNAUTHORIZED", "invalid two-factor code") + } + } + // Negative control: the counter is live — the eleventh attempt is refused. + pt = loginPartial(t, router, "two", "correctPass1", "", "") + wantErr(t, verify(t, router, pt, totpCode(t, secret), "203.0.113.99"), http.StatusTooManyRequests, "RATE_LIMITED", "too many failed attempts, try again later") }) t.Run("secret removed after the challenge was issued -> 401", func(t *testing.T) { database, router, uid, secret := setup(t) diff --git a/Server/service/auth.go b/Server/service/auth.go index 2da0c745..2de715ee 100644 --- a/Server/service/auth.go +++ b/Server/service/auth.go @@ -216,7 +216,10 @@ var ( // Second factor. ErrTOTPChallengeInvalid = &authError{ErrUnauthorized, "invalid or expired two-factor challenge"} ErrTOTPSecretUnreadable = &authError{ErrInternal, "failed to verify two-factor code"} - ErrTOTPCodeInvalid = &authError{ErrUnauthorized, "invalid two-factor code"} + // ErrTOTPUnavailable is a store fault while loading the challenged user + // (OC-0377): an outage, not a bad challenge, so no attempt is charged. + ErrTOTPUnavailable = &authError{ErrInternal, "two-factor verification temporarily unavailable"} + ErrTOTPCodeInvalid = &authError{ErrUnauthorized, "invalid two-factor code"} // ErrTOTPAlreadyEnabled is written by the transport as 409 // TOTP_ALREADY_ENABLED, not the generic CONFLICT code. ErrTOTPAlreadyEnabled = &authError{ErrConflict, "disable 2FA before re-enabling"} @@ -472,6 +475,11 @@ func (s *AuthService) VerifyTOTP(ctx context.Context, partialToken, code string) return nil, ErrTOTPChallengeInvalid } + user, secret, err := s.challengeSecret(ctx, challenge.UserID) + if err != nil { + return nil, err + } + totpRateLimitKey := auth.Key("totp_fail", challenge.UserID) // Atomically record this attempt and reject once the per-user failure cap // is reached. Recording up-front — rather than a read-only Check now and @@ -485,15 +493,13 @@ func (s *AuthService) VerifyTOTP(ctx context.Context, partialToken, code string) // multiplier exists for shared-NAT per-IP limits; scaling a per-user // threshold with it would hand a distributed attacker more guesses. // Mirrors loginUserFailureThreshold staying unscaled in authenticate. + // The reservation sits after the store read above, as in authenticate, + // so an outage does not consume attempts (OC-0377); it still precedes + // the code compare, the check-then-act the up-front record closes. if !s.limiter.Allow(totpRateLimitKey, totpFailureRateLimit, totpFailureWindow) { return nil, ErrTooManyAttempts } - user, secret, err := s.challengeSecret(ctx, challenge.UserID) - if err != nil { - return nil, err - } - if !auth.VerifyTOTPCodeOnce(secret, strings.TrimSpace(code), time.Now().UTC(), user.ID, s.usedCodes) { // The attempt was already recorded atomically up-front via // limiter.Allow; only the per-partial-token counter is advanced here. @@ -522,7 +528,14 @@ func (s *AuthService) VerifyTOTP(ctx context.Context, partialToken, code string) // returns their decrypted TOTP secret. func (s *AuthService) challengeSecret(ctx context.Context, challengeUserID int64) (*db.User, string, error) { user, err := s.st.GetUserByID(ctx, challengeUserID) - if err != nil || user == nil || user.TOTPSecret == nil { + if err != nil { + // GetUserByID answers (nil, nil) for an unknown user, so a non-nil + // error is a store fault: report the outage, not a bad challenge + // (OC-0377). VerifyTOTP records no attempt for it. + slog.Error("verify-totp: GetUserByID failed", "err", err, "user_id", challengeUserID) + return nil, "", ErrTOTPUnavailable + } + if user == nil || user.TOTPSecret == nil { return nil, "", ErrTOTPChallengeInvalid }