fix: 2FA disable now requires the verification code (enforced mode)
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.
This commit is contained in:
@@ -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.
|
||||
//
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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 @@
|
||||
</div>
|
||||
{/if}
|
||||
|
||||
{#if showDisableCodeEntry}
|
||||
<div class="mt-3">
|
||||
<p class="mb-2 text-sm text-gray-600">
|
||||
Enter the verification code to disable two-factor authentication.
|
||||
</p>
|
||||
<div class="flex items-center gap-2">
|
||||
<Input
|
||||
type="text"
|
||||
inputmode="numeric"
|
||||
maxlength={6}
|
||||
placeholder="6-digit code"
|
||||
bind:value={twoFADisableCode}
|
||||
disabled={twoFADisableConfirming}
|
||||
/>
|
||||
<Button disabled={twoFADisableConfirming} onclick={confirmDisableWithCode}>
|
||||
{twoFADisableConfirming ? 'Disabling...' : 'Confirm Disable'}
|
||||
</Button>
|
||||
</div>
|
||||
</div>
|
||||
{/if}
|
||||
|
||||
{#if twoFADirty && !twoFASetupPending}
|
||||
<Button
|
||||
class="mt-3"
|
||||
|
||||
Reference in New Issue
Block a user