From 495d7fba896de6ba2fbf09a750973e0e46fa843c Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Fri, 2 Oct 2026 09:15:21 +0100 Subject: [PATCH 1/4] fix(frontend): let the MFA challenge handle its own 401s (#872) The challenge is authenticated by the short-lived MFA token, not by the session. When that token was revoked after five wrong codes, or had expired, the response interceptor treated the 401 as a lost session: it tried to refresh an unrelated session, then reloaded /login. The user landed on an empty password form with no word about what happened. Requests to /auth/mfa/challenge now pass their errors straight to the caller. Every other request keeps the refresh and redirect behaviour. Signed-off-by: alex-dembele --- frontend/src/lib/__tests__/api.test.ts | 34 ++++++++++++++++++++++++++ frontend/src/lib/api.ts | 17 +++++++++++++ 2 files changed, 51 insertions(+) diff --git a/frontend/src/lib/__tests__/api.test.ts b/frontend/src/lib/__tests__/api.test.ts index 747e8f8a..e9fbcddc 100644 --- a/frontend/src/lib/__tests__/api.test.ts +++ b/frontend/src/lib/__tests__/api.test.ts @@ -157,3 +157,37 @@ describe('api interceptor — an expired access cookie (issue 691)', () => { expect(window.location.href).toBe('/login'); }); }); + +// #872 — the MFA challenge is authenticated by its own short-lived token, not by +// the session. Its 401s say "this sign-in attempt is over", which the challenge +// screen explains. Refreshing an unrelated session or reloading /login here +// wiped that explanation and left the user on a blank password form. +describe('api interceptor — the MFA challenge owns its 401s (issue 872)', () => { + for (const code of ['TOKEN_REVOKED', 'TOKEN_EXPIRED', 'TOKEN_INVALID', 'UNAUTHORIZED']) { + it(`neither refreshes nor redirects on ${code}`, async () => { + const post = vi.spyOn(axios, 'post'); + let calls = 0; + const adapter: MockCall = async (config) => { + calls += 1; + throw expired(config, code); + }; + api.defaults.adapter = adapter as never; + + const err = await api.post('/auth/mfa/challenge', { code: '123456' }).catch((e: unknown) => e); + expect((err as { response?: { data?: { code?: string } } }).response?.data?.code).toBe(code); + expect(post).not.toHaveBeenCalled(); + expect(calls).toBe(1); + expect(window.location.href).toBe(''); + }); + } + + it('still sends an ordinary request with a revoked token to /login', async () => { + const adapter: MockCall = async (config) => { + throw expired(config, 'TOKEN_REVOKED'); + }; + api.defaults.adapter = adapter as never; + + await expect(api.get('/auth/mfa/status')).rejects.toBeTruthy(); + expect(window.location.href).toBe('/login'); + }); +}); diff --git a/frontend/src/lib/api.ts b/frontend/src/lib/api.ts index fc0159d0..e093b2f7 100644 --- a/frontend/src/lib/api.ts +++ b/frontend/src/lib/api.ts @@ -75,6 +75,15 @@ const TOKEN_ERROR_CODES = new Set([ // A revoked or invalid token is not recoverable and still goes to /login. const REFRESHABLE_CODES = new Set(['TOKEN_EXPIRED', 'UNAUTHORIZED']); +// Requests authenticated by a credential of their own rather than by the +// session, whose 401s are for the caller to explain, not for the interceptor. +const OWN_CREDENTIAL_PATHS = ['/auth/mfa/challenge']; + +function isOwnCredentialRequest(url: string | undefined): boolean { + const path = String(url ?? ''); + return OWN_CREDENTIAL_PATHS.some((p) => path.includes(p)); +} + // A single in-flight refresh shared by every request that 401s at once, so a // dashboard full of widgets that all expire together triggers ONE /auth/refresh, // not one per widget. Cleared when it settles so a later expiry can refresh @@ -128,6 +137,14 @@ api.interceptors.response.use( const code = error.response?.data?.code; const original = error.config as (typeof error.config & { _retried?: boolean }) | undefined; + // The MFA challenge carries its own short-lived token, not the session. Its + // 401s mean "this sign-in attempt is over", and the challenge screen says so + // and sends the user back to the password (#872). Refreshing an unrelated + // session, or reloading /login, would wipe that explanation. + if (isOwnCredentialRequest(original?.url)) { + return Promise.reject(error); + } + // Transparent refresh-and-retry: an access token that merely EXPIRED is // recoverable from the long-lived refresh cookie. Without this, every // fetch-on-mount widget (dashboard KPIs, score gauge, …) silently rendered From bce734cbab83f76bb30818a6124cc6cda63f9463 Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Fri, 2 Oct 2026 09:20:31 +0100 Subject: [PATCH 2/4] fix(frontend): tell users to sign in again or wait after too many MFA codes (#872) Since #689 the challenge refuses a code in four ways, and the screen said "incorrect code" for all of them. Someone whose sign-in had used its five codes kept typing new ones into a token the server had closed. A spent, revoked or expired challenge now returns to the password form, with the address and password still filled in and a banner saying why. A locked account is told how many minutes to wait; the code field and the button stay disabled until then, and focus comes back to the field when the lock ends. The per-address limit, which gives no duration, reads as "try again in a few minutes". A wrong code is unchanged. The copy lives in authStrings.ts with the rest of the auth screens, in French and English. Signed-off-by: alex-dembele --- frontend/src/features/auth/AuthScreen.tsx | 110 ++++++++-- .../auth/__tests__/challengeRefusal.test.ts | 71 +++++++ .../__tests__/mfaChallengeRefusals.test.tsx | 191 ++++++++++++++++++ frontend/src/features/auth/authStrings.ts | 16 ++ .../src/features/auth/challengeRefusal.ts | 71 +++++++ 5 files changed, 447 insertions(+), 12 deletions(-) create mode 100644 frontend/src/features/auth/__tests__/challengeRefusal.test.ts create mode 100644 frontend/src/features/auth/__tests__/mfaChallengeRefusals.test.tsx create mode 100644 frontend/src/features/auth/challengeRefusal.ts diff --git a/frontend/src/features/auth/AuthScreen.tsx b/frontend/src/features/auth/AuthScreen.tsx index 39cc4dbe..28978204 100644 --- a/frontend/src/features/auth/AuthScreen.tsx +++ b/frontend/src/features/auth/AuthScreen.tsx @@ -18,6 +18,7 @@ import { landingForBusinessRole } from '../../shared/navModel'; import { useUIStore } from '../../store/uiStore'; import { authCopy, providerLabel, type OAuthErrorCode } from './authStrings'; import { challengeMFA, setupMFA, verifyMFA } from './authService'; +import { classifyChallengeRefusal, minutesToWait } from './challengeRefusal'; import { OtpField, sanitiseCode } from '../../shared/ds'; import { AuthLayout } from './AuthLayout'; import { cascade, usePrefersReducedMotion } from './motion'; @@ -35,12 +36,21 @@ const OAUTH_PROVIDERS: { id: 'google' | 'github' | 'azure'; label: string }[] = export function AuthScreen({ initialView = 'login' }: { initialView?: View }) { const [view, setView] = useState(initialView); + // Why the login form is showing, when another screen sent the user to it: + // a challenge that cannot take another code ends on the password (#872). + const [notice, setNotice] = useState(''); return ( {view === 'login' ? ( - setView('register')} /> + setView('register')} /> ) : ( - setView('login')} /> + setView('login')} + onRestart={(message) => { + setNotice(message); + setView('login'); + }} + /> )} ); @@ -57,7 +67,7 @@ function isPasswordResetRequired(err: unknown): boolean { return body?.code === 'password_reset_required'; } -function LoginForm({ onRegister }: { onRegister: () => void }) { +function LoginForm({ notice = '', onRegister }: { notice?: string; onRegister: () => void }) { const navigate = useNavigate(); const [params, setParams] = useSearchParams(); const lang = useUIStore((s) => s.lang); @@ -69,7 +79,7 @@ function LoginForm({ onRegister }: { onRegister: () => void }) { const [password, setPassword] = useState(''); const [show, setShow] = useState(false); const [busy, setBusy] = useState(false); - const [error, setError] = useState(''); + const [error, setError] = useState(notice); // Bumped on every failure so the shake fires again even for an identical // message — otherwise retyping the same wrong password gives no feedback. const [errorNonce, setErrorNonce] = useState(0); @@ -159,7 +169,16 @@ function LoginForm({ onRegister }: { onRegister: () => void }) { return mfa.enrolling ? ( ) : ( - setMfa(null)} /> + setMfa(null)} + onRestart={(message) => { + // The address and password stay filled in: signing in again is one + // click, and the banner says why it is needed. + setMfa(null); + fail(message); + }} + /> ); } @@ -297,7 +316,19 @@ function LoginForm({ onRegister }: { onRegister: () => void }) { // MFA — challenge // --------------------------------------------------------------------------- -function MFAChallenge({ token, onCancel }: { token: string; onCancel: () => void }) { +/** Disables the field this long when the server gave no wait (per-address limit). */ +const UNKNOWN_LOCK_MS = 60_000; + +function MFAChallenge({ + token, + onCancel, + onRestart, +}: { + token: string; + onCancel: () => void; + /** The sign-in attempt is over; only the password can start another one. */ + onRestart: (message: string) => void; +}) { const navigate = useNavigate(); const lang = useUIStore((s) => s.lang); const copy = authCopy(lang); @@ -308,9 +339,36 @@ function MFAChallenge({ token, onCancel }: { token: string; onCancel: () => void const [busy, setBusy] = useState(false); const [error, setError] = useState(''); const [nonce, setNonce] = useState(0); + // Set while the account refuses codes (#872). The field is disabled until + // then: a code typed during the lock is refused without being read. + const [lockedUntil, setLockedUntil] = useState(null); + const codeInput = useRef(null); + const locked = lockedUntil !== null; + + useEffect(() => { + if (lockedUntil === null) return; + const timer = window.setTimeout( + () => { + setLockedUntil(null); + setError(''); + }, + Math.max(0, lockedUntil - Date.now()), + ); + return () => window.clearTimeout(timer); + }, [lockedUntil]); + + // Focus returns to the field once it is enabled again. It cannot be done in + // the timer above: the field is still disabled until React re-renders, and a + // disabled input ignores focus(). + const wasLocked = useRef(false); + useEffect(() => { + if (wasLocked.current && !locked) codeInput.current?.focus(); + wasLocked.current = locked; + }, [locked]); const submit = async (e: React.FormEvent) => { e.preventDefault(); + if (locked) return; setBusy(true); setError(''); try { @@ -321,8 +379,26 @@ function MFAChallenge({ token, onCancel }: { token: string; onCancel: () => void await adoptSession(result.token_pair.access_token); } navigate(landingForBusinessRole(useAuthStore.getState().user?.business_role)); - } catch { - setError(copy.mfaInvalid); + } catch (err) { + const refusal = classifyChallengeRefusal(err); + switch (refusal.kind) { + case 'restart': + onRestart(refusal.reason === 'exhausted' ? copy.mfaExhausted : copy.mfaExpired); + return; + case 'locked': + setLockedUntil( + Date.now() + + (refusal.retryAfterSeconds === null ? UNKNOWN_LOCK_MS : refusal.retryAfterSeconds * 1000), + ); + setError( + refusal.retryAfterSeconds === null + ? copy.mfaLockedUnknown + : copy.mfaLocked(minutesToWait(refusal.retryAfterSeconds)), + ); + break; + default: + setError(copy.mfaInvalid); + } setNonce((n) => n + 1); } finally { setBusy(false); @@ -358,8 +434,10 @@ function MFAChallenge({ token, onCancel }: { token: string; onCancel: () => void pasted with spaces, or with the words around it, used to be rejected while the user could plainly read the digits. */} void @@ -560,7 +638,13 @@ function MFAEnrollment({ token }: { token: string }) { // Register // --------------------------------------------------------------------------- -function RegisterForm({ onLogin }: { onLogin: () => void }) { +function RegisterForm({ + onLogin, + onRestart, +}: { + onLogin: () => void; + onRestart: (message: string) => void; +}) { const navigate = useNavigate(); const lang = useUIStore((s) => s.lang); const copy = authCopy(lang); @@ -661,7 +745,9 @@ function RegisterForm({ onLogin }: { onLogin: () => void }) { return mfa.enrolling ? ( ) : ( - setMfa(null)} /> + // The account exists by now, so starting over means signing in, not + // registering again. + setMfa(null)} onRestart={onRestart} /> ); } diff --git a/frontend/src/features/auth/__tests__/challengeRefusal.test.ts b/frontend/src/features/auth/__tests__/challengeRefusal.test.ts new file mode 100644 index 00000000..41bafb3d --- /dev/null +++ b/frontend/src/features/auth/__tests__/challengeRefusal.test.ts @@ -0,0 +1,71 @@ +// Copyright (c) 2026 OpenDefender Contributors +// SPDX-License-Identifier: AGPL-3.0-only + +import { AxiosError, AxiosHeaders, type AxiosResponse } from 'axios'; +import { describe, expect, it } from 'vitest'; + +import { classifyChallengeRefusal, minutesToWait } from '../challengeRefusal'; + +function refused(status: number, data: unknown, headers: Record = {}): AxiosError { + const response = { + status, + statusText: '', + data, + headers, + config: { headers: new AxiosHeaders() }, + } as AxiosResponse; + return new AxiosError('refused', String(status), response.config, null, response); +} + +describe('classifyChallengeRefusal (#872)', () => { + it('reads a wrong code as a wrong code', () => { + expect(classifyChallengeRefusal(refused(400, { error: 'invalid MFA code' }))).toEqual({ + kind: 'invalid', + }); + }); + + it('sends a spent or revoked token back to the password', () => { + for (const code of ['MFA_CHALLENGE_EXHAUSTED', 'TOKEN_REVOKED']) { + expect(classifyChallengeRefusal(refused(401, { code }))).toEqual({ + kind: 'restart', + reason: 'exhausted', + }); + } + }); + + it('sends an expired or missing token back to the password', () => { + for (const code of ['TOKEN_EXPIRED', 'TOKEN_INVALID', 'UNAUTHORIZED', 'SOMETHING_NEW']) { + expect(classifyChallengeRefusal(refused(401, { code }))).toEqual({ + kind: 'restart', + reason: 'expired', + }); + } + }); + + it('reads the wait from the body first, then from Retry-After', () => { + expect(classifyChallengeRefusal(refused(429, { code: 'MFA_LOCKED', retry_after: 900 }))).toEqual({ + kind: 'locked', + retryAfterSeconds: 900, + }); + expect( + classifyChallengeRefusal(refused(429, { code: 'MFA_LOCKED' }, { 'retry-after': '120' })), + ).toEqual({ kind: 'locked', retryAfterSeconds: 120 }); + }); + + it('treats the per-address limit as a wait of unknown length', () => { + expect(classifyChallengeRefusal(refused(429, { error: true, msg: 'Rate limit exceeded' }))).toEqual({ + kind: 'locked', + retryAfterSeconds: null, + }); + }); + + it('falls back to "invalid" when no response came back', () => { + expect(classifyChallengeRefusal(new Error('network'))).toEqual({ kind: 'invalid' }); + }); + + it('rounds the wait up to whole minutes and never says zero', () => { + expect(minutesToWait(900)).toBe(15); + expect(minutesToWait(61)).toBe(2); + expect(minutesToWait(1)).toBe(1); + }); +}); diff --git a/frontend/src/features/auth/__tests__/mfaChallengeRefusals.test.tsx b/frontend/src/features/auth/__tests__/mfaChallengeRefusals.test.tsx new file mode 100644 index 00000000..a0d54834 --- /dev/null +++ b/frontend/src/features/auth/__tests__/mfaChallengeRefusals.test.tsx @@ -0,0 +1,191 @@ +// Copyright (c) 2026 OpenDefender Contributors +// SPDX-License-Identifier: AGPL-3.0-only +// +// #872 — the MFA code screen after #689. A wrong code still says "incorrect +// code". A sign-in that cannot take another code goes back to the password and +// says why. A locked account says how long to wait and stops taking codes. + +import { AxiosError, AxiosHeaders, type AxiosResponse } from 'axios'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { act, render, screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { MemoryRouter } from 'react-router'; + +import { useUIStore } from '../../../store/uiStore'; +import { authCopy } from '../authStrings'; + +const navigate = vi.fn(); +const login = vi.fn(); +const adoptSession = vi.fn(); +const challengeMFA = vi.fn(); + +vi.mock('react-router', async () => { + const actual = await vi.importActual('react-router'); + return { ...actual, useNavigate: () => navigate }; +}); +vi.mock('../../../lib/api', () => ({ + api: { post: vi.fn(), defaults: { baseURL: '' } }, +})); +vi.mock('../authService', () => ({ + setupMFA: vi.fn(), + verifyMFA: vi.fn(), + challengeMFA: (...a: unknown[]) => challengeMFA(...a), +})); +vi.mock('../../../hooks/useAuthStore', () => { + const state = { + user: undefined, + login: (...a: unknown[]) => login(...a), + adoptSession: (...a: unknown[]) => adoptSession(...a), + }; + const useAuthStore = (sel?: (s: typeof state) => unknown) => (sel ? sel(state) : state); + useAuthStore.getState = () => state; + return { useAuthStore }; +}); + +import { AuthScreen } from '../AuthScreen'; + +function refused(status: number, data: unknown): AxiosError { + const response = { + status, + statusText: '', + data, + headers: {}, + config: { headers: new AxiosHeaders() }, + } as AxiosResponse; + return new AxiosError('refused', String(status), response.config, null, response); +} + +async function reachChallenge() { + const user = userEvent.setup(); + login.mockResolvedValue({ status: 'mfa_required', mfa_token: 'challenge-token' }); + render( + + + , + ); + await user.type(screen.getByTestId('login-email'), 'alix@example.com'); + await user.type(screen.getByTestId('login-password'), 'MotDePasse2026!'); + await user.click(screen.getByTestId('login-submit')); + await screen.findByTestId('mfa-code'); + return user; +} + +async function submitCode(user: ReturnType, code = '123456') { + await user.type(screen.getByTestId('mfa-code'), code); + await user.click(screen.getByTestId('mfa-submit')); +} + +const copy = () => authCopy(useUIStore.getState().lang); + +beforeEach(() => { + vi.clearAllMocks(); + useUIStore.getState().setLang('fr'); +}); + +afterEach(() => { + useUIStore.getState().setLang('fr'); +}); + +describe('MFA challenge refusals (#872)', () => { + it('keeps the code field for a wrong code', async () => { + const user = await reachChallenge(); + challengeMFA.mockRejectedValueOnce(refused(400, { error: 'invalid MFA code' })); + await submitCode(user); + + expect(await screen.findByTestId('auth-error')).toHaveTextContent(copy().mfaInvalid); + expect(screen.getByTestId('mfa-code')).toBeEnabled(); + }); + + it.each(['MFA_CHALLENGE_EXHAUSTED', 'TOKEN_REVOKED'])( + 'sends the user back to the password on %s, with the reason', + async (code) => { + const user = await reachChallenge(); + challengeMFA.mockRejectedValueOnce(refused(401, { code })); + await submitCode(user); + + expect(await screen.findByTestId('login-password')).toHaveValue('MotDePasse2026!'); + expect(screen.queryByTestId('mfa-code')).toBeNull(); + expect(screen.getByTestId('auth-error')).toHaveTextContent(copy().mfaExhausted); + }, + ); + + it('says the step expired when the challenge token did', async () => { + const user = await reachChallenge(); + challengeMFA.mockRejectedValueOnce(refused(401, { code: 'TOKEN_EXPIRED' })); + await submitCode(user); + + await screen.findByTestId('login-password'); + expect(screen.getByTestId('auth-error')).toHaveTextContent(copy().mfaExpired); + }); + + it('starting over reaches the code step again with a fresh token', async () => { + const user = await reachChallenge(); + challengeMFA.mockRejectedValueOnce(refused(401, { code: 'MFA_CHALLENGE_EXHAUSTED' })); + await submitCode(user); + await screen.findByTestId('login-password'); + + login.mockResolvedValueOnce({ status: 'mfa_required', mfa_token: 'fresh-token' }); + await user.click(screen.getByTestId('login-submit')); + await screen.findByTestId('mfa-code'); + challengeMFA.mockResolvedValueOnce({ token_pair: { access_token: 'a' } }); + await submitCode(user); + + await waitFor(() => expect(challengeMFA).toHaveBeenLastCalledWith('123456', 'fresh-token')); + }); + + it('says how long a locked account must wait and stops taking codes', async () => { + const user = await reachChallenge(); + challengeMFA.mockRejectedValueOnce(refused(429, { code: 'MFA_LOCKED', retry_after: 900 })); + await submitCode(user); + + expect(await screen.findByTestId('auth-error')).toHaveTextContent(copy().mfaLocked(15)); + expect(screen.getByTestId('mfa-code')).toBeDisabled(); + expect(screen.getByTestId('mfa-submit')).toBeDisabled(); + }); + + it('gives the code field back when the lock ends', async () => { + // Simulated clock: the lock lasts 15 minutes, and a real wait of even one + // second is flaky when the suite runs in parallel. + vi.useFakeTimers({ shouldAdvanceTime: true }); + try { + const user = await reachChallenge(); + challengeMFA.mockRejectedValueOnce(refused(429, { code: 'MFA_LOCKED', retry_after: 900 })); + await submitCode(user); + expect(await screen.findByTestId('mfa-code')).toBeDisabled(); + + act(() => { + vi.advanceTimersByTime(899_000); + }); + expect(screen.getByTestId('mfa-code')).toBeDisabled(); + + act(() => { + vi.advanceTimersByTime(2_000); + }); + expect(screen.getByTestId('mfa-code')).toBeEnabled(); + expect(screen.queryByTestId('auth-error')).toBeNull(); + expect(screen.getByTestId('mfa-code')).toHaveFocus(); + } finally { + vi.useRealTimers(); + } + }); + + it('reads the per-address limit as a wait of unknown length', async () => { + const user = await reachChallenge(); + challengeMFA.mockRejectedValueOnce(refused(429, { error: true, msg: 'Rate limit exceeded' })); + await submitCode(user); + + expect(await screen.findByTestId('auth-error')).toHaveTextContent(copy().mfaLockedUnknown); + expect(screen.getByTestId('mfa-code')).toBeDisabled(); + }); + + it('speaks English to an English session', async () => { + useUIStore.getState().setLang('en'); + const user = await reachChallenge(); + challengeMFA.mockRejectedValueOnce(refused(401, { code: 'MFA_CHALLENGE_EXHAUSTED' })); + await submitCode(user); + + expect(await screen.findByTestId('auth-error')).toHaveTextContent( + 'Too many wrong codes for this sign-in. Sign in again with your password.', + ); + }); +}); diff --git a/frontend/src/features/auth/authStrings.ts b/frontend/src/features/auth/authStrings.ts index fd4e1c6b..9c3f06a8 100644 --- a/frontend/src/features/auth/authStrings.ts +++ b/frontend/src/features/auth/authStrings.ts @@ -211,6 +211,11 @@ export interface AuthCopy { mfaSubmit: string; mfaUseBackup: string; mfaInvalid: string; + // #872 — refusals that a new code cannot fix. + mfaExhausted: string; + mfaExpired: string; + mfaLocked: (minutes: number) => string; + mfaLockedUnknown: string; mfaSetupFailed: string; mfaEnrolTitle: string; mfaEnrolSubtitle: string; @@ -316,6 +321,12 @@ const fr: AuthCopy = { mfaSubmit: 'Vérifier', mfaUseBackup: 'Utiliser un code de récupération', mfaInvalid: 'Code incorrect. Vérifiez votre application et réessayez.', + mfaExhausted: + 'Trop de codes erronés pour cette connexion. Reconnectez-vous avec votre mot de passe.', + mfaExpired: 'Cette étape de connexion a expiré. Reconnectez-vous avec votre mot de passe.', + mfaLocked: (m) => + `Trop de codes erronés. La vérification est suspendue pour ce compte : réessayez dans ${m} minute${m > 1 ? 's' : ''}.`, + mfaLockedUnknown: 'Trop de tentatives. Réessayez dans quelques minutes.', // Distinct from registerFailed on purpose: at this point the account EXISTS. // Telling the user their sign-up failed sends them back to create it again. mfaSetupFailed: @@ -430,6 +441,11 @@ const en: AuthCopy = { mfaSubmit: 'Verify', mfaUseBackup: 'Use a recovery code', mfaInvalid: 'Incorrect code. Check your app and try again.', + mfaExhausted: 'Too many wrong codes for this sign-in. Sign in again with your password.', + mfaExpired: 'This sign-in step has expired. Sign in again with your password.', + mfaLocked: (m) => + `Too many wrong codes. Verification is paused for this account: try again in ${m} minute${m > 1 ? 's' : ''}.`, + mfaLockedUnknown: 'Too many attempts. Try again in a few minutes.', mfaSetupFailed: 'Could not set up two-factor authentication. Your account exists — reload the page to try again.', mfaEnrolTitle: 'Turn on two-factor authentication', diff --git a/frontend/src/features/auth/challengeRefusal.ts b/frontend/src/features/auth/challengeRefusal.ts new file mode 100644 index 00000000..b956748a --- /dev/null +++ b/frontend/src/features/auth/challengeRefusal.ts @@ -0,0 +1,71 @@ +// Copyright (c) 2026 OpenDefender Contributors +// SPDX-License-Identifier: AGPL-3.0-only +// +// What a refused MFA challenge means for the person at the screen (#872). +// +// Since #689 the server refuses a code in more ways than "wrong code", and each +// one calls for a different move: +// +// 401 MFA_CHALLENGE_EXHAUSTED this sign-in used its five codes +// 401 TOKEN_REVOKED the same, on any later attempt +// 401 TOKEN_EXPIRED / _INVALID the challenge token outlived its five minutes +// 401 UNAUTHORIZED no usable challenge token at all +// 429 MFA_LOCKED the account refuses codes for a while +// 429 (anything else) the per-address limit on the route +// +// Showing "incorrect code" for all of them kept people typing codes into a +// sign-in attempt the server had already closed. + +import axios from 'axios'; + +export type ChallengeRefusal = + /** A wrong code. The same token can take another one. */ + | { kind: 'invalid' } + /** The sign-in attempt is over. Only the password can start a new one. */ + | { kind: 'restart'; reason: 'exhausted' | 'expired' } + /** Codes are refused for a while. `retryAfterSeconds` is null when unknown. */ + | { kind: 'locked'; retryAfterSeconds: number | null }; + +interface RefusalBody { + code?: unknown; + retry_after?: unknown; +} + +function positiveSeconds(v: unknown): number | null { + const n = typeof v === 'string' ? Number.parseInt(v, 10) : v; + return typeof n === 'number' && Number.isFinite(n) && n > 0 ? Math.ceil(n) : null; +} + +export function classifyChallengeRefusal(err: unknown): ChallengeRefusal { + if (!axios.isAxiosError(err) || !err.response) return { kind: 'invalid' }; + + const { status, data, headers } = err.response; + const body = (data ?? {}) as RefusalBody; + const code = typeof body.code === 'string' ? body.code.toUpperCase() : ''; + + if (status === 429) { + const header = (headers as Record | undefined)?.['retry-after']; + return { + kind: 'locked', + retryAfterSeconds: positiveSeconds(body.retry_after) ?? positiveSeconds(header), + }; + } + + if (status !== 401) return { kind: 'invalid' }; + + switch (code) { + case 'MFA_CHALLENGE_EXHAUSTED': + case 'TOKEN_REVOKED': + return { kind: 'restart', reason: 'exhausted' }; + default: + // TOKEN_EXPIRED, TOKEN_INVALID, UNAUTHORIZED, and any 401 not known yet: + // this token will not be accepted again, so asking for another code + // would only fail again. + return { kind: 'restart', reason: 'expired' }; + } +} + +/** Whole minutes to wait, never zero: "0 minutes" reads as "now". */ +export function minutesToWait(seconds: number): number { + return Math.max(1, Math.ceil(seconds / 60)); +} From 1e64f25aab30163ae706cc95ac77edddeadcf140 Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Fri, 2 Oct 2026 12:56:30 +0100 Subject: [PATCH 3/4] fix(frontend): bring the auth error banner and footer to 4.5:1 contrast (#872) axe flagged two serious colour-contrast failures on the sign-in and MFA screens. The error banner's 12.5px text measured 4.46:1 on the light theme, and this change shows that banner in more situations. Its tint goes from 10% to 7%, which measures 4.68:1 light and 5.20:1 dark. The footer year was dimmed with opacity-70 to 3.35:1; it now takes the colour of the rest of the line, which passes. Signed-off-by: alex-dembele --- frontend/src/features/auth/AuthLayout.tsx | 2 +- frontend/src/features/auth/fields.tsx | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/frontend/src/features/auth/AuthLayout.tsx b/frontend/src/features/auth/AuthLayout.tsx index 5b3195dc..ec1c121f 100644 --- a/frontend/src/features/auth/AuthLayout.tsx +++ b/frontend/src/features/auth/AuthLayout.tsx @@ -319,7 +319,7 @@ export function AuthLayout({ children }: AuthLayoutProps) { > {copy.footerCompany} {' '} - · © {new Date().getFullYear()} + · © {new Date().getFullYear()} diff --git a/frontend/src/features/auth/fields.tsx b/frontend/src/features/auth/fields.tsx index c8b1bf36..a027265e 100644 --- a/frontend/src/features/auth/fields.tsx +++ b/frontend/src/features/auth/fields.tsx @@ -29,7 +29,10 @@ export function ErrorBanner({ children }: { children: React.ReactNode }) { data-testid="auth-error" className="flex items-start gap-2 mb-4 px-3 py-2.5 rounded-[11px] text-[12.5px] leading-snug" style={{ - background: 'color-mix(in srgb, var(--critical) 10%, transparent)', + // 7%, not 10%: at 10% the 12.5px text measured 4.46:1 on the light + // theme, under the 4.5:1 floor (axe color-contrast, #872). 7% gives + // 4.68:1 light and 5.20:1 dark. + background: 'color-mix(in srgb, var(--critical) 7%, transparent)', border: '1px solid color-mix(in srgb, var(--critical) 35%, transparent)', color: 'var(--critical)', }} From 8b0e152bafb85b467a55cb24ec6e593981f65f10 Mon Sep 17 00:00:00 2001 From: alex-dembele Date: Fri, 2 Oct 2026 13:10:15 +0100 Subject: [PATCH 4/4] fix(frontend): send an expired MFA enrolment back to the password with a reason (#872) Mandated enrolment runs on a 15-minute MFA_ENROLLMENT token. Once it expired, /auth/mfa/verify answered 401, the response interceptor took it for a lost session, and /login reloaded with no word of explanation. The exemption added for the challenge was keyed on its URL, which could not cover enrolment: /auth/mfa/setup and /verify also run from Settings on a real session, where an expired access token must still refresh. So the exemption now follows the credential instead. authService marks a request ownCredential when it sends an MFA token, and the interceptor leaves only those alone. On the enrolment screen, a refusal no code can fix returns to the password form with "this sign-in step has expired" instead of "incorrect code", whether it happens at setup or at verify. Signed-off-by: alex-dembele --- frontend/src/features/auth/AuthScreen.tsx | 52 +++++++++++------ .../authServiceOwnCredential.test.ts | 47 +++++++++++++++ .../__tests__/mfaChallengeRefusals.test.tsx | 58 ++++++++++++++++++- frontend/src/features/auth/authService.ts | 9 ++- frontend/src/lib/__tests__/api.test.ts | 33 +++++++++-- frontend/src/lib/api.ts | 33 ++++++----- 6 files changed, 191 insertions(+), 41 deletions(-) create mode 100644 frontend/src/features/auth/__tests__/authServiceOwnCredential.test.ts diff --git a/frontend/src/features/auth/AuthScreen.tsx b/frontend/src/features/auth/AuthScreen.tsx index 28978204..5bd58f95 100644 --- a/frontend/src/features/auth/AuthScreen.tsx +++ b/frontend/src/features/auth/AuthScreen.tsx @@ -166,19 +166,16 @@ function LoginForm({ notice = '', onRegister }: { notice?: string; onRegister: ( }; if (mfa) { + // The address and password stay filled in: signing in again is one click, + // and the banner says why it is needed. + const restart = (message: string) => { + setMfa(null); + fail(message); + }; return mfa.enrolling ? ( - + ) : ( - setMfa(null)} - onRestart={(message) => { - // The address and password stay filled in: signing in again is one - // click, and the banner says why it is needed. - setMfa(null); - fail(message); - }} - /> + setMfa(null)} onRestart={restart} /> ); } @@ -486,7 +483,14 @@ function MFAChallenge({ * completes the login in the same step, so the user is not asked for the * password they typed a minute ago. */ -function MFAEnrollment({ token }: { token: string }) { +function MFAEnrollment({ + token, + onRestart, +}: { + token: string; + /** The enrolment token is spent or expired; only the password gets a new one. */ + onRestart: (message: string) => void; +}) { const navigate = useNavigate(); const lang = useUIStore((s) => s.lang); const copy = authCopy(lang); @@ -517,13 +521,19 @@ function MFAEnrollment({ token }: { token: string }) { setQr(r.qr_code); setBackupCodes(r.backup_codes ?? []); }) - .catch(() => { + .catch((err: unknown) => { + // An enrolment token that has expired cannot be retried; send the user + // back to the password for a new one (#872). + if (classifyChallengeRefusal(err).kind === 'restart') { + onRestart(copy.mfaExpired); + return; + } // A setup failure is not a registration failure: the account is already // created. Say what actually went wrong, and allow another attempt. requested.current = null; setError(copy.mfaSetupFailed); }); - }, [token, copy.mfaSetupFailed]); + }, [token, copy.mfaSetupFailed, copy.mfaExpired, onRestart]); const qrSrc = qr.startsWith('data:') ? qr : `data:image/jpeg;base64,${qr}`; @@ -543,7 +553,13 @@ function MFAEnrollment({ token }: { token: string }) { await adoptSession(result.token_pair.access_token); } navigate(landingForBusinessRole(useAuthStore.getState().user?.business_role)); - } catch { + } catch (err) { + // The enrolment token lives 15 minutes. Past that, every code is refused + // whatever it is, so "incorrect code" would be a lie (#872). + if (classifyChallengeRefusal(err).kind === 'restart') { + onRestart(copy.mfaExpired); + return; + } setError(copy.mfaInvalid); setNonce((n) => n + 1); } finally { @@ -741,12 +757,12 @@ function RegisterForm({ // Hand off to the same enrolment/challenge screens the login form uses, rather // than a second implementation that would drift from it. + // The account exists by now, so starting over means signing in, not + // registering again. if (mfa) { return mfa.enrolling ? ( - + ) : ( - // The account exists by now, so starting over means signing in, not - // registering again. setMfa(null)} onRestart={onRestart} /> ); } diff --git a/frontend/src/features/auth/__tests__/authServiceOwnCredential.test.ts b/frontend/src/features/auth/__tests__/authServiceOwnCredential.test.ts new file mode 100644 index 00000000..986fd4a5 --- /dev/null +++ b/frontend/src/features/auth/__tests__/authServiceOwnCredential.test.ts @@ -0,0 +1,47 @@ +// Copyright (c) 2026 OpenDefender Contributors +// SPDX-License-Identifier: AGPL-3.0-only +// +// #872 — the MFA calls that carry their own token must say so, or the response +// interceptor treats their 401s as a lost session and reloads /login. + +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const post = vi.fn(); +vi.mock('../../../lib/api', () => ({ + api: { post: (...a: unknown[]) => post(...a) }, +})); + +import { challengeMFA, setupMFA, verifyMFA } from '../authService'; + +function configOfLastCall(): { ownCredential?: boolean; headers?: Record } | undefined { + return post.mock.calls.at(-1)?.[2] as ReturnType; +} + +beforeEach(() => { + post.mockReset(); + post.mockResolvedValue({ data: {} }); +}); + +describe('MFA requests and the session (#872)', () => { + it('marks the challenge as carrying its own credential', async () => { + await challengeMFA('123456', 'challenge-token'); + expect(configOfLastCall()).toEqual({ + headers: { Authorization: 'Bearer challenge-token' }, + ownCredential: true, + }); + }); + + it('marks mandated enrolment the same way', async () => { + await setupMFA('enrol-token'); + expect(configOfLastCall()?.ownCredential).toBe(true); + await verifyMFA('123456', 'enrol-token'); + expect(configOfLastCall()?.ownCredential).toBe(true); + }); + + it('leaves enrolment from Settings on the session', async () => { + await setupMFA(); + expect(configOfLastCall()).toBeUndefined(); + await verifyMFA('123456'); + expect(configOfLastCall()).toBeUndefined(); + }); +}); diff --git a/frontend/src/features/auth/__tests__/mfaChallengeRefusals.test.tsx b/frontend/src/features/auth/__tests__/mfaChallengeRefusals.test.tsx index a0d54834..104b528e 100644 --- a/frontend/src/features/auth/__tests__/mfaChallengeRefusals.test.tsx +++ b/frontend/src/features/auth/__tests__/mfaChallengeRefusals.test.tsx @@ -18,6 +18,8 @@ const navigate = vi.fn(); const login = vi.fn(); const adoptSession = vi.fn(); const challengeMFA = vi.fn(); +const setupMFA = vi.fn(); +const verifyMFA = vi.fn(); vi.mock('react-router', async () => { const actual = await vi.importActual('react-router'); @@ -27,8 +29,8 @@ vi.mock('../../../lib/api', () => ({ api: { post: vi.fn(), defaults: { baseURL: '' } }, })); vi.mock('../authService', () => ({ - setupMFA: vi.fn(), - verifyMFA: vi.fn(), + setupMFA: (...a: unknown[]) => setupMFA(...a), + verifyMFA: (...a: unknown[]) => verifyMFA(...a), challengeMFA: (...a: unknown[]) => challengeMFA(...a), })); vi.mock('../../../hooks/useAuthStore', () => { @@ -189,3 +191,55 @@ describe('MFA challenge refusals (#872)', () => { ); }); }); + +// Criterion 6: mandated enrolment runs on a 15-minute MFA_ENROLLMENT token. +// Past that, every code is refused, and "incorrect code" would be a lie. +describe('mandated enrolment with an expired token (#872)', () => { + async function reachEnrolment() { + const user = userEvent.setup(); + login.mockResolvedValue({ status: 'mfa_enrollment_required', mfa_token: 'enrol-token' }); + render( + + + , + ); + await user.type(screen.getByTestId('login-email'), 'alix@example.com'); + await user.type(screen.getByTestId('login-password'), 'MotDePasse2026!'); + await user.click(screen.getByTestId('login-submit')); + return user; + } + + beforeEach(() => { + setupMFA.mockResolvedValue({ secret: 'ABCDEF', qr_code: '/9j/raw', backup_codes: [] }); + }); + + it('goes back to the password when the code is sent on an expired token', async () => { + const user = await reachEnrolment(); + verifyMFA.mockRejectedValueOnce(refused(401, { code: 'TOKEN_EXPIRED' })); + await user.click(await screen.findByTestId('mfa-enrol-code')); + await user.paste('123456'); + + expect(await screen.findByTestId('login-password')).toHaveValue('MotDePasse2026!'); + expect(screen.getByTestId('auth-error')).toHaveTextContent(copy().mfaExpired); + }); + + it('goes back to the password when setup itself meets an expired token', async () => { + setupMFA.mockReset(); + setupMFA.mockRejectedValueOnce(refused(401, { code: 'TOKEN_EXPIRED' })); + await reachEnrolment(); + + expect(await screen.findByTestId('login-password')).toBeInTheDocument(); + expect(screen.getByTestId('auth-error')).toHaveTextContent(copy().mfaExpired); + expect(setupMFA).toHaveBeenCalledTimes(1); + }); + + it('still says "incorrect code" for a wrong code on a live token', async () => { + const user = await reachEnrolment(); + verifyMFA.mockRejectedValueOnce(refused(400, { error: 'invalid code' })); + await user.click(await screen.findByTestId('mfa-enrol-code')); + await user.paste('123456'); + + expect(await screen.findByTestId('auth-error')).toHaveTextContent(copy().mfaInvalid); + expect(screen.getByTestId('mfa-enrol-code')).toBeInTheDocument(); + }); +}); diff --git a/frontend/src/features/auth/authService.ts b/frontend/src/features/auth/authService.ts index 7ceeb5a7..6f329ae3 100644 --- a/frontend/src/features/auth/authService.ts +++ b/frontend/src/features/auth/authService.ts @@ -278,7 +278,14 @@ export async function challengeMFA(code: string, mfaToken: string): Promise { }); }); -// #872 — the MFA challenge is authenticated by its own short-lived token, not by -// the session. Its 401s say "this sign-in attempt is over", which the challenge -// screen explains. Refreshing an unrelated session or reloading /login here -// wiped that explanation and left the user on a blank password form. -describe('api interceptor — the MFA challenge owns its 401s (issue 872)', () => { +// #872 — the MFA challenge and mandated enrolment are authenticated by a +// short-lived token of their own, not by the session. Their 401s say "this +// sign-in attempt is over", which the screen explains. Refreshing an unrelated +// session or reloading /login wiped that explanation and left the user on a +// blank password form. The request says so with `ownCredential`, set by +// authService when it sends an MFA token. +describe('api interceptor — requests with their own credential keep their 401s (issue 872)', () => { for (const code of ['TOKEN_REVOKED', 'TOKEN_EXPIRED', 'TOKEN_INVALID', 'UNAUTHORIZED']) { it(`neither refreshes nor redirects on ${code}`, async () => { const post = vi.spyOn(axios, 'post'); @@ -173,7 +175,9 @@ describe('api interceptor — the MFA challenge owns its 401s (issue 872)', () = }; api.defaults.adapter = adapter as never; - const err = await api.post('/auth/mfa/challenge', { code: '123456' }).catch((e: unknown) => e); + const err = await api + .post('/auth/mfa/verify', { code: '123456' }, { ownCredential: true }) + .catch((e: unknown) => e); expect((err as { response?: { data?: { code?: string } } }).response?.data?.code).toBe(code); expect(post).not.toHaveBeenCalled(); expect(calls).toBe(1); @@ -181,6 +185,23 @@ describe('api interceptor — the MFA challenge owns its 401s (issue 872)', () = }); } + it('still refreshes the same endpoint when it runs on the session (Settings)', async () => { + const post = vi + .spyOn(axios, 'post') + .mockResolvedValue({ status: 200, data: { token_pair: { access_token: 't' } } } as never); + let calls = 0; + const adapter: MockCall = async (config) => { + calls += 1; + if (calls === 1) throw expired(config, 'TOKEN_EXPIRED'); + return ok(config, { verified: true }); + }; + api.defaults.adapter = adapter as never; + + await api.post('/auth/mfa/verify', { code: '123456' }); + expect(post).toHaveBeenCalledTimes(1); + expect(calls).toBe(2); + }); + it('still sends an ordinary request with a revoked token to /login', async () => { const adapter: MockCall = async (config) => { throw expired(config, 'TOKEN_REVOKED'); diff --git a/frontend/src/lib/api.ts b/frontend/src/lib/api.ts index e093b2f7..d8b2fe1f 100644 --- a/frontend/src/lib/api.ts +++ b/frontend/src/lib/api.ts @@ -21,6 +21,18 @@ import { markApiFailure, markApiSuccess } from './connection'; */ const baseURL = import.meta.env.VITE_API_URL ?? '/api/v1'; +declare module 'axios' { + interface AxiosRequestConfig { + /** + * The request authenticates with a credential of its own (an MFA token), + * not the session: its 401s are for the caller to explain, and the + * session recovery in the response interceptor leaves them alone (#872). + * Client-side only; never sent. + */ + ownCredential?: boolean; + } +} + export const api = axios.create({ baseURL, headers: { 'Content-Type': 'application/json' }, @@ -75,15 +87,6 @@ const TOKEN_ERROR_CODES = new Set([ // A revoked or invalid token is not recoverable and still goes to /login. const REFRESHABLE_CODES = new Set(['TOKEN_EXPIRED', 'UNAUTHORIZED']); -// Requests authenticated by a credential of their own rather than by the -// session, whose 401s are for the caller to explain, not for the interceptor. -const OWN_CREDENTIAL_PATHS = ['/auth/mfa/challenge']; - -function isOwnCredentialRequest(url: string | undefined): boolean { - const path = String(url ?? ''); - return OWN_CREDENTIAL_PATHS.some((p) => path.includes(p)); -} - // A single in-flight refresh shared by every request that 401s at once, so a // dashboard full of widgets that all expire together triggers ONE /auth/refresh, // not one per widget. Cleared when it settles so a later expiry can refresh @@ -137,11 +140,13 @@ api.interceptors.response.use( const code = error.response?.data?.code; const original = error.config as (typeof error.config & { _retried?: boolean }) | undefined; - // The MFA challenge carries its own short-lived token, not the session. Its - // 401s mean "this sign-in attempt is over", and the challenge screen says so - // and sends the user back to the password (#872). Refreshing an unrelated - // session, or reloading /login, would wipe that explanation. - if (isOwnCredentialRequest(original?.url)) { + // A request that carries its own short-lived MFA token (the challenge, or + // mandated enrolment) is not on the session. Its 401s mean "this sign-in + // attempt is over", and the screen says so and sends the user back to the + // password (#872). Refreshing an unrelated session, or reloading /login, + // would wipe that explanation. The same endpoints called from Settings run + // on the session and keep the recovery below. + if (original?.ownCredential) { return Promise.reject(error); }