From 9111258461855f77cec82d7294464759bcc0d29d Mon Sep 17 00:00:00 2001 From: Stephen Adamson Date: Thu, 13 Aug 2026 01:07:21 +0100 Subject: [PATCH] fix: 2FA disable now requires the verification code (enforced mode) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The disable flow was broken in enforced (production) mode: the account page posted {code:''} to /api/user/2fa/disable, but the backend mints + validates a code when 2FA is enforced, so the empty code always failed with 400 and a user could never disable 2FA through the UI. In unenforced (dev/mock) mode the backend short-circuits and no code is needed — which is why the user only saw the confirmation dialog and no code prompt. Backend: - New POST /api/user/2fa/disable/code (SendDisableCodeHandler): mints + delivers a fresh disable-flow code via the existing ensurePendingTwoFACode machinery (per-user 1-min mint cooldown, 429 when throttled, 5-attempt lockout preserved on the disable call itself). This is the disable-flow equivalent of /api/user/2fa/setup. Runs unconditionally (no dev short-circuit) so the step is exercisable in dev too. Route mounted in main.go beside the other 2FA routes. - Tests: mints fresh code, reuses valid pending code (hash unchanged), mint-throttled 429 (after the pending code is dropped, as a lockout does), unauthorized 401, unenforced still mints. Frontend (account page): - The disable confirmation now branches on twoFactorRequired: enforced → POST /api/user/2fa/disable/code to mint, then a 6-digit code-entry input + 'Confirm Disable' button that posts the code to /api/user/2fa/disable; unenforced (dev) → unchanged direct disable. Code entry mirrors the enable flow's input styling; mint-throttle 429 / wrong-code 400 / lockout 429 all surface as toasts with the entry kept open for retry. Verification: go test -tags test,dev -count=1 -parallel 8 ./... (all 20 packages ok, 0 failures incl. 5 new 2FA tests), go build ./... and -tags dev, go vet clean, svelte-check 0 errors 0 warnings, env-docs gate OK. --- backend/handlers/user/twofa.go | 41 ++++++++++ backend/handlers/user/twofa_test.go | 95 ++++++++++++++++++++++++ backend/main.go | 1 + frontend/src/routes/account/+page.svelte | 80 +++++++++++++++++++- 4 files changed, 216 insertions(+), 1 deletion(-) diff --git a/backend/handlers/user/twofa.go b/backend/handlers/user/twofa.go index 64e6a3d..c28b121 100644 --- a/backend/handlers/user/twofa.go +++ b/backend/handlers/user/twofa.go @@ -586,6 +586,47 @@ type TwoFADisableRequest struct { Code string `json:"code"` } +// POST /api/user/2fa/disable/code +// Mints + delivers a fresh disable-flow verification code so the frontend can +// show a code-entry step before it calls POST /api/user/2fa/disable. This is +// the disable-flow equivalent of SetupTwoFAHandler: it guarantees a valid +// (unexpired) pending code exists, delivering a fresh one via the same [2FA] +// log channel and persisting only its hash + expiry when none does. The actual +// disable still happens through the existing disable endpoint, which validates +// the entered code under the shared 5-attempt lockout — this handler only +// performs the mint. Fresh-code mints are throttled per-user +// (twoFAMintCooldown), so a password-only attacker cannot loop +// request-code → burn 5 guesses → request-code forever; a throttled request +// returns 429. Like the disable handler, no code is returned in the response +// (the [2FA] log line is the delivery channel), and unlike setup this endpoint +// runs unconditionally — it does not short-circuit on !twoFARequired(), so dev +// environments can exercise the same step (the mint is harmless there). +func SendDisableCodeHandler(w http.ResponseWriter, r *http.Request) { + userID, ok := mw.GetUserID(r.Context()) + if !ok { + http.Error(w, "unauthorized", http.StatusUnauthorized) + return + } + + // The per-user mutex serializes the mint with the disable handler's + // checkTwoFACode critical section so concurrent requests from the same user + // cannot race the cooldown or lockout counters. + st := twoFAAttemptStateFor(userID) + st.mu.Lock() + defer st.mu.Unlock() + + if err := ensurePendingTwoFACode(r, userID, st); err != nil { + if errors.Is(err, errTwoFAMintThrottled) { + http.Error(w, "Too many attempts. Wait before requesting a new code.", http.StatusTooManyRequests) + return + } + log.Printf("failed to prepare 2FA code for disable for user %s: %v", userID, err) + http.Error(w, "server error", http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusOK) +} + // POST /api/user/2fa/disable // Turns 2FA off and clears method + pending fields for the authenticated user. // diff --git a/backend/handlers/user/twofa_test.go b/backend/handlers/user/twofa_test.go index 146ab84..3ee895f 100644 --- a/backend/handlers/user/twofa_test.go +++ b/backend/handlers/user/twofa_test.go @@ -1094,3 +1094,98 @@ func TestTwoFA_ConcurrentCheckTwoFACode_NoDeadlock(t *testing.T) { t.Error(err) } } + +// ============================================================================= +// Disable-flow code mint endpoint (POST /api/user/2fa/disable/code) +// ============================================================================= + +// TestTwoFADisableCode_MintsFreshCode verifies that with no pending code the +// endpoint mints a fresh one and persists it (hash + unexpired expiry), so the +// frontend's disable code-entry step has a delivered code to verify against. +func TestTwoFADisableCode_MintsFreshCode(t *testing.T) { + twofaEnvEnforced(t) + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + + w := performUser2FARequest(t, SendDisableCodeHandler, ctx, http.MethodPost, "/api/user/2fa/disable/code", nil, userID) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + + var pendingHash sql.NullString + var expires sql.NullTime + require.NoError(t, tx.QueryRow(ctx, ` + SELECT two_factor_pending_code_hash, two_factor_pending_code_expires + FROM users WHERE id = $1`, userID).Scan(&pendingHash, &expires)) + require.True(t, pendingHash.Valid, "disable/code must mint a pending code hash") + require.True(t, expires.Valid && expires.Time.After(clock.Now()), "minted code must have a future expiry") +} + +// TestTwoFADisableCode_ReusesValidPendingCode verifies that a valid unexpired +// pending code is reused by ensurePendingTwoFACode (the stored hash is +// unchanged) instead of minting a fresh one. +func TestTwoFADisableCode_ReusesValidPendingCode(t *testing.T) { + twofaEnvEnforced(t) + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + seedPendingTwoFA(t, ctx, tx, userID, "123456") + + w := performUser2FARequest(t, SendDisableCodeHandler, ctx, http.MethodPost, "/api/user/2fa/disable/code", nil, userID) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + + var pendingHash sql.NullString + require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&pendingHash)) + require.True(t, pendingHash.Valid) + require.Equal(t, hashTwoFACode("123456"), pendingHash.String, "existing valid pending code must be reused, not re-minted") +} + +// TestTwoFADisableCode_MintThrottled verifies the per-user mint cooldown: a +// second code request inside twoFAMintCooldown returns 429. The pending code is +// dropped first (as a lockout does) because a still-valid code is reused by +// ensurePendingTwoFACode, which short-circuits the cooldown check. +func TestTwoFADisableCode_MintThrottled(t *testing.T) { + twofaEnvEnforced(t) + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + + // First request mints a fresh code and stamps the per-user cooldown. + w := performUser2FARequest(t, SendDisableCodeHandler, ctx, http.MethodPost, "/api/user/2fa/disable/code", nil, userID) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + + // Drop the pending code so the next request cannot reuse it and must hit + // the cooldown check instead. + _, err = tx.Exec(ctx, `UPDATE users + SET two_factor_pending_code_hash = NULL, two_factor_pending_code_expires = NULL + WHERE id = $1`, userID) + require.NoError(t, err) + + w = performUser2FARequest(t, SendDisableCodeHandler, ctx, http.MethodPost, "/api/user/2fa/disable/code", nil, userID) + require.Equal(t, http.StatusTooManyRequests, w.Code, w.Body.String()) + require.Contains(t, w.Body.String(), "Too many attempts. Wait before requesting a new code.") +} + +// TestTwoFADisableCode_Unauthorized verifies that an unauthenticated request is +// rejected with 401 before any minting happens. +func TestTwoFADisableCode_Unauthorized(t *testing.T) { + w := performUser2FARequest(t, SendDisableCodeHandler, context.Background(), http.MethodPost, "/api/user/2fa/disable/code", nil, "") + require.Equal(t, http.StatusUnauthorized, w.Code) +} + +// TestTwoFADisableCode_UnenforcedStillMints verifies the endpoint mints a +// pending code in unenforced (dev) environments too — the disable handler's dev +// bypass needs no code, but the endpoint still runs unconditionally so the +// code-entry step is exercisable locally. +func TestTwoFADisableCode_UnenforcedStillMints(t *testing.T) { + twofaEnvUnenforced(t) + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + + w := performUser2FARequest(t, SendDisableCodeHandler, ctx, http.MethodPost, "/api/user/2fa/disable/code", nil, userID) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + + var pendingHash sql.NullString + require.NoError(t, tx.QueryRow(ctx, "SELECT two_factor_pending_code_hash FROM users WHERE id = $1", userID).Scan(&pendingHash)) + require.True(t, pendingHash.Valid, "unenforced env must still mint a pending code") +} diff --git a/backend/main.go b/backend/main.go index f624268..d20b000 100644 --- a/backend/main.go +++ b/backend/main.go @@ -482,6 +482,7 @@ func main() { r.With(mw.RequireNonGuest).Post("/user/2fa/setup", user.SetupTwoFAHandler) r.With(mw.RequireNonGuest).Post("/user/2fa/verify", user.VerifyTwoFAHandler) r.With(mw.RequireNonGuest).Post("/user/2fa/disable", user.DisableTwoFAHandler) + r.With(mw.RequireNonGuest).Post("/user/2fa/disable/code", user.SendDisableCodeHandler) r.Delete("/user/account", user.DeleteAccountHandler) r.Get("/user/gdpr-export", user.GetGDPRExportHandler) r.Get("/user/loyalty", user.GetLoyaltyHandler) diff --git a/frontend/src/routes/account/+page.svelte b/frontend/src/routes/account/+page.svelte index 167bb78..c697ff6 100644 --- a/frontend/src/routes/account/+page.svelte +++ b/frontend/src/routes/account/+page.svelte @@ -558,6 +558,10 @@ let twoFADisabling = $state(false); let selectedTwoFA = $state<'none' | 'email' | 'sms'>('none'); let showDisableTwoFADialog = $state(false); + let showDisableCodeEntry = $state(false); + let twoFADisableCode = $state(''); + let twoFASendingDisableCode = $state(false); + let twoFADisableConfirming = $state(false); // Locally-selected 2FA state, behaving as a radio group: none, email, or sms — // never both. Initialised from the saved profile state so the toggles reflect @@ -677,7 +681,60 @@ async function confirmDisableTwoFA() { showDisableTwoFADialog = false; - await disableTwoFA(); + if (authStore.currentUser?.twoFactorRequired) { + await sendDisableCode(); + } else { + await disableTwoFA(); + } + } + + // Enforced environments require a verification code before 2FA can be + // disabled. Mint one first (the backend mints + delivers it), then show + // the code-entry step. + async function sendDisableCode() { + twoFASendingDisableCode = true; + try { + const res = await apiFetch('/api/user/2fa/disable/code', { + method: 'POST', + headers: { 'Content-Type': 'application/json' } + }); + if (res.ok) { + showDisableCodeEntry = true; + twoFADisableCode = ''; + toast.success('Verification code sent'); + } else { + const errText = await res.text(); + toast.error(extractErrorMessage(errText) || 'Failed to send verification code'); + } + } catch { + toast.error('Network error'); + } finally { + twoFASendingDisableCode = false; + } + } + + async function confirmDisableWithCode() { + twoFADisableConfirming = true; + try { + const res = await apiFetch('/api/user/2fa/disable', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ code: twoFADisableCode }) + }); + if (res.ok) { + showDisableCodeEntry = false; + twoFADisableCode = ''; + await authStore.refreshProfile(); + toast.success('Two-factor authentication disabled'); + } else { + const errText = await res.text(); + toast.error(extractErrorMessage(errText) || 'Failed to disable two-factor authentication'); + } + } catch { + toast.error('Network error'); + } finally { + twoFADisableConfirming = false; + } } // Image cropper state @@ -2671,6 +2728,27 @@ {/if} + {#if showDisableCodeEntry} +
+

+ Enter the verification code to disable two-factor authentication. +

+
+ + +
+
+ {/if} + {#if twoFADirty && !twoFASetupPending}