From d90e25d1ffa576f2e527386fad8b73d3e4694e03 Mon Sep 17 00:00:00 2001 From: Stephen Adamson Date: Thu, 20 Aug 2026 16:36:31 +0100 Subject: [PATCH] =?UTF-8?q?fix:=20auth/2FA=20=E2=80=94=20password=20change?= =?UTF-8?q?=20requires=202FA=20gate,=20admin=20self-deletion=20blocked,=20?= =?UTF-8?q?twofa=20JSON=20responses,=20per-IP=20email-verify=20budget,=20O?= =?UTF-8?q?ptionalAuth=20log=20sanitised?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- backend/handlers/auth/local.go | 65 ++++++++++++++++++++++++++++++++ backend/handlers/user/account.go | 20 ++++++++++ backend/handlers/user/profile.go | 32 ++++++++++++++-- backend/handlers/user/twofa.go | 48 +++++++++++------------ backend/mw/auth.go | 2 +- 5 files changed, 139 insertions(+), 28 deletions(-) diff --git a/backend/handlers/auth/local.go b/backend/handlers/auth/local.go index f7a194d..bbd6cf4 100644 --- a/backend/handlers/auth/local.go +++ b/backend/handlers/auth/local.go @@ -155,8 +155,60 @@ type emailVerifyAttempt struct { var ( emailVerifyMu sync.Mutex emailVerifyAttempts = make(map[string]emailVerifyAttempt) + ipVerifyMu sync.Mutex + ipVerifyAttempts = make(map[string]emailVerifyAttempt) ) +// ipAttemptsExhausted reports whether the IP's attempt budget is already +// spent, rejecting the request before any DB work. Uses the same window as +// email-verify tracking with a higher cap (50 per IP per window) as a +// secondary per-IP fallback budget. +func ipAttemptsExhausted(ip string) bool { + ipVerifyMu.Lock() + defer ipVerifyMu.Unlock() + evictStaleIPVerifyAttemptsLocked() + a, ok := ipVerifyAttempts[ip] + return ok && a.count >= 50 +} + +// ipAttemptFailed registers one failed attempt for the IP and reports whether +// the budget for that IP is now exhausted. +func ipAttemptFailed(ip string) bool { + ipVerifyMu.Lock() + defer ipVerifyMu.Unlock() + evictStaleIPVerifyAttemptsLocked() + now := clock.Now() + a := ipVerifyAttempts[ip] + if now.Sub(a.lastAt) > emailVerifyAttemptWindow { + a.count = 0 + } + a.count++ + a.lastAt = now + ipVerifyAttempts[ip] = a + return a.count >= 50 +} + +// ipAttemptsClear drops the budget for an IP after a successful verify. +func ipAttemptsClear(ip string) { + ipVerifyMu.Lock() + delete(ipVerifyAttempts, ip) + ipVerifyMu.Unlock() +} + +// evictStaleIPVerifyAttemptsLocked bounds the IP attempts map. Caller must +// hold ipVerifyMu. +func evictStaleIPVerifyAttemptsLocked() { + if len(ipVerifyAttempts) < emailVerifyMaxTrackedCodes { + return + } + now := clock.Now() + for k, a := range ipVerifyAttempts { + if now.Sub(a.lastAt) > emailVerifyAttemptWindow { + delete(ipVerifyAttempts, k) + } + } +} + // emailVerifyAttemptsExhausted reports whether the key's (a user id, or a // submitted code with no resolvable user) attempt budget is already spent, // rejecting the request before any DB work. @@ -1020,6 +1072,16 @@ func VerifyCodeHandler(w http.ResponseWriter, r *http.Request) { return } + // Per-IP fallback budget: when no user resolves, the attempt budget is + // keyed on the submitted CODE value — rotating codes gives unlimited + // guesses. The per-IP check prevents a single source from exhausting the + // endpoint regardless of code rotation. + ip := mw.ClientIP(r) + if ipAttemptsExhausted(ip) { + http.Error(w, "too many attempts", http.StatusTooManyRequests) + return + } + codeDigest := twofa.Hash(code) var userID string @@ -1036,6 +1098,7 @@ func VerifyCodeHandler(w http.ResponseWriter, r *http.Request) { if errors.Is(err, pgx.ErrNoRows) { // No row with this digest at all — a guess. No user can be // resolved, so the attempt budget stays keyed per submitted code. + ipAttemptFailed(ip) if emailVerifyAttemptFailed(code) { http.Error(w, "too many attempts. request a new code.", http.StatusTooManyRequests) return @@ -1063,6 +1126,7 @@ func VerifyCodeHandler(w http.ResponseWriter, r *http.Request) { } // Code exists but expired — count it against the user's budget too. if !expiresAt.After(clock.Now()) { + ipAttemptFailed(ip) if emailVerifyAttemptFailed(userID) { http.Error(w, "too many attempts. request a new code.", http.StatusTooManyRequests) return @@ -1132,6 +1196,7 @@ func VerifyCodeHandler(w http.ResponseWriter, r *http.Request) { // key the submitted code used on earlier guesses). emailVerifyAttemptsClear(userID) emailVerifyAttemptsClear(code) + ipAttemptsClear(ip) if err := json.NewEncoder(w).Encode(VerificationResponse{Success: true, Message: message}); err != nil { log.Printf("Failed to encode JSON response: %v", err) diff --git a/backend/handlers/user/account.go b/backend/handlers/user/account.go index fed1876..b41e9aa 100644 --- a/backend/handlers/user/account.go +++ b/backend/handlers/user/account.go @@ -338,6 +338,26 @@ func DeleteAccountHandler(w http.ResponseWriter, r *http.Request) { return } + // FIX: admin accounts cannot be self-deleted + if accountRole == "admin" { + http.Error(w, "admin accounts cannot be self-deleted", http.StatusForbidden) + return + } + + // FIX: check for active bookings before guest deletion + if accountRole == "guest" { + var bookingCount int + if err := db.Conn.QueryRow(r.Context(), `SELECT COUNT(*) FROM bookings WHERE user_id = $1 AND status IN ('pending', 'confirmed', 'in_progress', 'pending_release')`, userID).Scan(&bookingCount); err != nil { + log.Printf("Failed to check bookings for guest %s: %v", userID, err) + http.Error(w, "internal error", http.StatusInternalServerError) + return + } + if bookingCount > 0 { + http.Error(w, "cannot delete guest account with active bookings", http.StatusBadRequest) + return + } + } + ctx := r.Context() // Finding 3: deleting an account is irreversible, so the session token alone diff --git a/backend/handlers/user/profile.go b/backend/handlers/user/profile.go index c7860fa..412a015 100644 --- a/backend/handlers/user/profile.go +++ b/backend/handlers/user/profile.go @@ -30,6 +30,7 @@ import ( "crussell/internal/dav" "crussell/internal/images" "crussell/internal/s3" + "crussell/internal/twofa" "crussell/internal/validators" "crussell/mw" @@ -659,8 +660,9 @@ func ListAdminUsersHandler(w http.ResponseWriter, r *http.Request) { } type ChangePasswordRequest struct { - CurrentPassword string `json:"current_password" validate:"required,max=72"` - NewPassword string `json:"new_password" validate:"required,min=6,max=72"` + CurrentPassword string `json:"current_password" validate:"required,max=72"` + NewPassword string `json:"new_password" validate:"required,min=6,max=72"` + VerificationCode string `json:"verification_code"` } func ChangePasswordHandler(w http.ResponseWriter, r *http.Request) { @@ -702,7 +704,8 @@ func ChangePasswordHandler(w http.ResponseWriter, r *http.Request) { } var passwordHash sql.NullString - err := db.Conn.QueryRow(r.Context(), `SELECT password_hash FROM users WHERE id = $1`, userID).Scan(&passwordHash) + var twoFactorEnabled bool + err := db.Conn.QueryRow(r.Context(), `SELECT password_hash, two_factor_enabled FROM users WHERE id = $1`, userID).Scan(&passwordHash, &twoFactorEnabled) if err != nil { if errors.Is(err, pgx.ErrNoRows) { http.Error(w, "user not found", http.StatusNotFound) @@ -765,6 +768,29 @@ func ChangePasswordHandler(w http.ResponseWriter, r *http.Request) { // login-locked-out user can self-recover by changing their password. resetCurrentPasswordFailures(r.Context(), userID) + if twoFARequired() && twoFactorEnabled { + if req.VerificationCode == "" { + http.Error(w, "a two-factor verification code is required to change the password", http.StatusBadRequest) + return + } + switch err := twofa.VerifyForUser(r.Context(), userID, req.VerificationCode, twofa.ConsumeOnVerify); { + case err == nil: + case errors.Is(err, twofa.ErrIncorrect): + http.Error(w, "incorrect verification code", http.StatusBadRequest) + return + case errors.Is(err, twofa.ErrLockedOut): + http.Error(w, "Too many attempts. Request a new code.", http.StatusTooManyRequests) + return + case errors.Is(err, twofa.ErrMissingOrExpired): + http.Error(w, "verification code is missing or has expired", http.StatusBadRequest) + return + default: + log.Printf("Failed to check 2FA pending code for user %s: %v", userID, err) + http.Error(w, "internal server error", http.StatusInternalServerError) + return + } + } + newHash, err := bcrypt.GenerateFromPassword([]byte(req.NewPassword), bcrypt.DefaultCost) if err != nil { log.Printf("Failed to hash new password for user %s: %v", userID, err) diff --git a/backend/handlers/user/twofa.go b/backend/handlers/user/twofa.go index 3b24c22..53b09fa 100644 --- a/backend/handlers/user/twofa.go +++ b/backend/handlers/user/twofa.go @@ -210,7 +210,7 @@ type TwoFAStatusResponse struct { func GetTwoFAStatusHandler(w http.ResponseWriter, r *http.Request) { userID, ok := mw.GetUserID(r.Context()) if !ok { - http.Error(w, "unauthorized", http.StatusUnauthorized) + mw.RespondError(w, http.StatusUnauthorized, "unauthorized") return } @@ -223,7 +223,7 @@ func GetTwoFAStatusHandler(w http.ResponseWriter, r *http.Request) { `, userID).Scan(&enabled, &method) if err != nil { log.Printf("failed to fetch 2FA status for user %s: %v", userID, err) - http.Error(w, "server error", http.StatusInternalServerError) + mw.RespondError(w, http.StatusInternalServerError, "internal server error") return } @@ -254,13 +254,13 @@ type TwoFASetupRequest struct { func SetupTwoFAHandler(w http.ResponseWriter, r *http.Request) { userID, ok := mw.GetUserID(r.Context()) if !ok { - http.Error(w, "unauthorized", http.StatusUnauthorized) + mw.RespondError(w, http.StatusUnauthorized, "unauthorized") return } var req TwoFASetupRequest if err := json.NewDecoder(r.Body).Decode(&req); err != nil { - http.Error(w, "invalid request", http.StatusBadRequest) + mw.RespondError(w, http.StatusBadRequest, "invalid request") return } if req.Method != "email" && req.Method != "sms" { @@ -272,7 +272,7 @@ func SetupTwoFAHandler(w http.ResponseWriter, r *http.Request) { err := db.Conn.QueryRow(r.Context(), `SELECT two_factor_enabled FROM users WHERE id = $1`, userID).Scan(&enabled) if err != nil { log.Printf("failed to check 2FA state for user %s: %v", userID, err) - http.Error(w, "server error", http.StatusInternalServerError) + mw.RespondError(w, http.StatusInternalServerError, "internal server error") return } if enabled { @@ -315,7 +315,7 @@ func SetupTwoFAHandler(w http.ResponseWriter, r *http.Request) { return } log.Printf("failed to store 2FA pending code for user %s: %v", userID, err) - http.Error(w, "server error", http.StatusInternalServerError) + mw.RespondError(w, http.StatusInternalServerError, "internal server error") return } st.SetLastMintAtLocked(now) @@ -403,13 +403,13 @@ func VerifyTwoFACodeForUser(ctx context.Context, userID, code string) error { func VerifyTwoFAHandler(w http.ResponseWriter, r *http.Request) { userID, ok := mw.GetUserID(r.Context()) if !ok { - http.Error(w, "unauthorized", http.StatusUnauthorized) + mw.RespondError(w, http.StatusUnauthorized, "unauthorized") return } var req TwoFAVerifyRequest if err := json.NewDecoder(r.Body).Decode(&req); err != nil { - http.Error(w, "invalid request", http.StatusBadRequest) + mw.RespondError(w, http.StatusBadRequest, "invalid request") return } @@ -417,7 +417,7 @@ func VerifyTwoFAHandler(w http.ResponseWriter, r *http.Request) { // Dev bypass: no code verification in unenforced environments. if err := enableTwoFA(r, userID); err != nil { log.Printf("failed to enable 2FA for user %s: %v", userID, err) - http.Error(w, "server error", http.StatusInternalServerError) + mw.RespondError(w, http.StatusInternalServerError, "internal server error") return } writeTwoFAEnabled(w) @@ -435,7 +435,7 @@ func VerifyTwoFAHandler(w http.ResponseWriter, r *http.Request) { result, err := checkTwoFACode(r, userID, st, req.Code) if err != nil { log.Printf("failed to check 2FA pending code for user %s: %v", userID, err) - http.Error(w, "server error", http.StatusInternalServerError) + mw.RespondError(w, http.StatusInternalServerError, "internal server error") return } switch result { @@ -462,7 +462,7 @@ func VerifyTwoFAHandler(w http.ResponseWriter, r *http.Request) { if err := enableTwoFA(r, userID); err != nil { log.Printf("failed to enable 2FA for user %s: %v", userID, err) - http.Error(w, "server error", http.StatusInternalServerError) + mw.RespondError(w, http.StatusInternalServerError, "internal server error") return } writeTwoFAEnabled(w) @@ -528,7 +528,7 @@ type TwoFADisableRequest struct { func SendDisableCodeHandler(w http.ResponseWriter, r *http.Request) { userID, ok := mw.GetUserID(r.Context()) if !ok { - http.Error(w, "unauthorized", http.StatusUnauthorized) + mw.RespondError(w, http.StatusUnauthorized, "unauthorized") return } @@ -552,7 +552,7 @@ func SendDisableCodeHandler(w http.ResponseWriter, r *http.Request) { return } log.Printf("failed to prepare 2FA code for disable for user %s: %v", userID, err) - http.Error(w, "server error", http.StatusInternalServerError) + mw.RespondError(w, http.StatusInternalServerError, "internal server error") return } w.WriteHeader(http.StatusOK) @@ -588,7 +588,7 @@ func SendDisableCodeHandler(w http.ResponseWriter, r *http.Request) { func SendVerificationCodeHandler(w http.ResponseWriter, r *http.Request) { userID, ok := mw.GetUserID(r.Context()) if !ok { - http.Error(w, "unauthorized", http.StatusUnauthorized) + mw.RespondError(w, http.StatusUnauthorized, "unauthorized") return } @@ -596,7 +596,7 @@ func SendVerificationCodeHandler(w http.ResponseWriter, r *http.Request) { err := db.Conn.QueryRow(r.Context(), `SELECT two_factor_enabled FROM users WHERE id = $1`, userID).Scan(&enabled) if err != nil { log.Printf("failed to check 2FA state for user %s: %v", userID, err) - http.Error(w, "server error", http.StatusInternalServerError) + mw.RespondError(w, http.StatusInternalServerError, "internal server error") return } if !enabled { @@ -625,7 +625,7 @@ func SendVerificationCodeHandler(w http.ResponseWriter, r *http.Request) { return } log.Printf("failed to prepare 2FA code for saved-card charge for user %s: %v", userID, err) - http.Error(w, "server error", http.StatusInternalServerError) + mw.RespondError(w, http.StatusInternalServerError, "internal server error") return } @@ -671,7 +671,7 @@ func AdminSendVerificationCodeHandler(w http.ResponseWriter, r *http.Request) { return } log.Printf("failed to check 2FA state for user %s: %v", targetUserID, err) - http.Error(w, "server error", http.StatusInternalServerError) + mw.RespondError(w, http.StatusInternalServerError, "internal server error") return } if !enabled { @@ -697,7 +697,7 @@ func AdminSendVerificationCodeHandler(w http.ResponseWriter, r *http.Request) { return } log.Printf("failed to prepare 2FA code for user %s: %v", targetUserID, err) - http.Error(w, "server error", http.StatusInternalServerError) + mw.RespondError(w, http.StatusInternalServerError, "internal server error") return } @@ -742,7 +742,7 @@ func AdminSendVerificationCodeHandler(w http.ResponseWriter, r *http.Request) { func DisableTwoFAHandler(w http.ResponseWriter, r *http.Request) { userID, ok := mw.GetUserID(r.Context()) if !ok { - http.Error(w, "unauthorized", http.StatusUnauthorized) + mw.RespondError(w, http.StatusUnauthorized, "unauthorized") return } @@ -755,7 +755,7 @@ func DisableTwoFAHandler(w http.ResponseWriter, r *http.Request) { // Dev bypass: no re-verification in unenforced environments. if err := disableTwoFA(r, userID); err != nil { log.Printf("failed to disable 2FA for user %s: %v", userID, err) - http.Error(w, "server error", http.StatusInternalServerError) + mw.RespondError(w, http.StatusInternalServerError, "internal server error") return } w.WriteHeader(http.StatusOK) @@ -789,14 +789,14 @@ func DisableTwoFAHandler(w http.ResponseWriter, r *http.Request) { return } log.Printf("failed to prepare 2FA code for disable for user %s: %v", userID, err) - http.Error(w, "server error", http.StatusInternalServerError) + mw.RespondError(w, http.StatusInternalServerError, "internal server error") return } result, err := checkTwoFACode(r, userID, st, req.Code) if err != nil { log.Printf("failed to check 2FA pending code for user %s: %v", userID, err) - http.Error(w, "server error", http.StatusInternalServerError) + mw.RespondError(w, http.StatusInternalServerError, "internal server error") return } switch result { @@ -814,7 +814,7 @@ func DisableTwoFAHandler(w http.ResponseWriter, r *http.Request) { if err := disableTwoFA(r, userID); err != nil { log.Printf("failed to disable 2FA for user %s: %v", userID, err) - http.Error(w, "server error", http.StatusInternalServerError) + mw.RespondError(w, http.StatusInternalServerError, "internal server error") return } w.WriteHeader(http.StatusOK) @@ -942,7 +942,7 @@ func AdminRemoveUser2FAHandler(w http.ResponseWriter, r *http.Request) { affected, err := removeUser2FA(r, userID) if err != nil { log.Printf("failed to remove 2FA for user %s: %v", userID, err) - http.Error(w, "server error", http.StatusInternalServerError) + mw.RespondError(w, http.StatusInternalServerError, "internal server error") return } if !affected { diff --git a/backend/mw/auth.go b/backend/mw/auth.go index 47ce572..2e6041b 100644 --- a/backend/mw/auth.go +++ b/backend/mw/auth.go @@ -53,7 +53,7 @@ func OptionalAuth(next http.Handler) http.Handler { userID, role, jti, err := auth.VerifyToken(tokenString, r.Context()) if err != nil { - log.Printf("OptionalAuth: invalid token: %v", err) + log.Printf("OptionalAuth: invalid token ignored") } else { ctx := context.WithValue(r.Context(), UserIDKey, userID) ctx = context.WithValue(ctx, UserRoleKey, role)