From 4bf930e2425c5a84b66cf91359d64ef7201fad2c Mon Sep 17 00:00:00 2001 From: Geovanne Washington Date: Fri, 2 Oct 2026 15:57:48 -0300 Subject: [PATCH 1/7] docs(openapi): document the MFA login lockout and single-use token POST /api/user/mfa/auth now answers 429 with X-Account-Lockout while wrong codes keep the source locked out, and 401 for an unknown or already used token. --- openapi/spec/paths/api@user@mfa@auth.yaml | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/openapi/spec/paths/api@user@mfa@auth.yaml b/openapi/spec/paths/api@user@mfa@auth.yaml index d4f497c008c..695c9e7b33b 100644 --- a/openapi/spec/paths/api@user@mfa@auth.yaml +++ b/openapi/spec/paths/api@user@mfa@auth.yaml @@ -4,6 +4,10 @@ post: description: | Authenticate a user who has MFA enabled. This endpoint should be called after the default authUser endpoint, which generates an `X-MFA-Token` indicating that the user has already authenticated with a password. + + The token logs in once only. An unknown, expired or already used token answers `401`, the same + as a wrong code. Wrong codes count toward the same account lockout as wrong passwords: check the + `X-Account-Lockout` header, which is 0 when there is no active lockout. tags: - cloud - mfa @@ -29,5 +33,17 @@ post: $ref: ../components/responses/403.yaml '404': $ref: ../components/responses/404.yaml + '429': + description: Too Many Requests + headers: + X-Account-Lockout: + schema: + type: integer + description: Indicates the Unix timestamp in seconds of when the account lockout period ends. + example: 1711575401 + content: + application/json: + schema: + $ref: ../components/schemas/apiError.yaml '500': $ref: ../components/responses/500.yaml From 6fb2b3eb0777e267def0859c0283bcd6c8b8c920 Mon Sep 17 00:00:00 2001 From: Geovanne Washington Date: Fri, 2 Oct 2026 15:57:48 -0300 Subject: [PATCH 2/7] fix(ui): show the MFA lockout and its countdown on the code step A 429 from the code step used to read as a wrong code. The form now shows the lockout with the countdown the password form uses, moved to a shared hook, and keeps the MFA token so the user can retry once it ends. Fixes: shellhub-io/team#264 --- .../src/components/auth/SignInForm.tsx | 51 ++---------------- .../src/components/mfa/MfaCodeForm.tsx | 34 ++++++++++-- .../console/src/hooks/useLockoutCountdown.ts | 53 ++++++++++++++++++ .../src/pages/__tests__/MfaLogin.test.tsx | 54 +++++++++++++++++++ ui/apps/console/src/stores/authStore.ts | 10 +++- 5 files changed, 149 insertions(+), 53 deletions(-) create mode 100644 ui/apps/console/src/hooks/useLockoutCountdown.ts diff --git a/ui/apps/console/src/components/auth/SignInForm.tsx b/ui/apps/console/src/components/auth/SignInForm.tsx index a2b5a1304b2..f89c517ab1a 100644 --- a/ui/apps/console/src/components/auth/SignInForm.tsx +++ b/ui/apps/console/src/components/auth/SignInForm.tsx @@ -1,9 +1,10 @@ -import { useState, useEffect, FormEvent } from "react"; +import { useState, FormEvent } from "react"; import { useForm } from "react-hook-form"; import { useNavigate } from "react-router-dom"; import { Button, Callout } from "@shellhub/design-system/primitives"; import { isSdkError } from "@/api/errors"; import { useAuthStore } from "@/stores/authStore"; +import { useLockoutCountdown } from "@/hooks/useLockoutCountdown"; import { landingAfterSignIn } from "@/utils/navigation"; import { FormInputField, @@ -14,52 +15,6 @@ import MfaCodeForm from "@/components/mfa/MfaCodeForm"; import AuthActions, { type AuthLink } from "@/components/auth/AuthActions"; import type { LoginFormValues } from "@/pages/setup/loginResolver"; -interface CountdownState { - display: string; - expired: boolean; - epoch: number | null; -} - -function useLoginCountdown(lockoutEndEpoch: number | null) { - const [state, setState] = useState({ - display: "", - expired: false, - epoch: null, - }); - - useEffect(() => { - if (lockoutEndEpoch === null) return; - - const interval = setInterval(() => { - const diff = lockoutEndEpoch - Date.now() / 1000; - if (diff <= 0) { - clearInterval(interval); - setState({ display: "", expired: true, epoch: lockoutEndEpoch }); - } else if (diff < 60) { - const s = Math.floor(diff); - setState({ - display: `${s} ${s === 1 ? "second" : "seconds"}`, - expired: false, - epoch: lockoutEndEpoch, - }); - } else { - const m = Math.floor(diff / 60); - setState({ - display: `${m} ${m === 1 ? "minute" : "minutes"}`, - expired: false, - epoch: lockoutEndEpoch, - }); - } - }, 1000); - - return () => clearInterval(interval); - }, [lockoutEndEpoch]); - - if (state.epoch !== lockoutEndEpoch) return { display: "", expired: false }; - - return { display: state.display, expired: state.expired }; -} - interface SignInFormProps { redirect: string; submitLabel?: string; @@ -88,7 +43,7 @@ export default function SignInForm({ const [secondFactor, setSecondFactor] = useState(false); const [lockoutEndEpoch, setLockoutEndEpoch] = useState(null); const { display: countdownDisplay, expired: lockoutExpired } = - useLoginCountdown(lockoutEndEpoch); + useLockoutCountdown(lockoutEndEpoch); const { control, handleSubmit, formState } = useForm({ resolver: loginResolver, diff --git a/ui/apps/console/src/components/mfa/MfaCodeForm.tsx b/ui/apps/console/src/components/mfa/MfaCodeForm.tsx index 1f2f2c25a4b..a9d4dd5ec98 100644 --- a/ui/apps/console/src/components/mfa/MfaCodeForm.tsx +++ b/ui/apps/console/src/components/mfa/MfaCodeForm.tsx @@ -1,6 +1,8 @@ -import type { FormEvent } from "react"; +import { useState, type FormEvent } from "react"; import { Button, Callout } from "@shellhub/design-system/primitives"; +import { isSdkError } from "@/api/errors"; import { useAuthStore } from "@/stores/authStore"; +import { useLockoutCountdown } from "@/hooks/useLockoutCountdown"; import { useOtpInput } from "@/hooks/useOtpInput"; import OtpCells from "@/components/mfa/OtpCells"; import AuthActions, { type AuthLink } from "@/components/auth/AuthActions"; @@ -14,7 +16,9 @@ interface MfaCodeFormProps { /** * The second step of a sign-in: the six cells for the authenticator code, exchanged for a full * session. A wrong code clears the cells and leaves the partial session in place, so the user - * can try again without signing in from scratch. `onVerified` runs once the session is full. + * can try again without signing in from scratch. Too many wrong codes lock the form out, with a + * countdown to when the user may try again, still without signing in from scratch. `onVerified` + * runs once the session is full. */ export default function MfaCodeForm({ onVerified, @@ -23,22 +27,44 @@ export default function MfaCodeForm({ }: MfaCodeFormProps) { const otp = useOtpInput(6); const { loginWithMfa, loading, error } = useAuthStore(); + const [lockoutEndEpoch, setLockoutEndEpoch] = useState(null); + const { display: countdownDisplay, expired: lockoutExpired } = + useLockoutCountdown(lockoutEndEpoch); const handleSubmit = async (e: FormEvent) => { e.preventDefault(); if (!otp.isComplete) return; + setLockoutEndEpoch(null); try { await loginWithMfa(otp.getValue()); onVerified(); - } catch { + } catch (err) { + if (isSdkError(err) && err.status === 429) { + const epoch = Number(err.headers.get("x-account-lockout")); + setLockoutEndEpoch(isNaN(epoch) ? null : epoch); + } otp.reset(); } }; return (
void handleSubmit(e)} className="space-y-6"> - {error && {error}} + {lockoutExpired && ( + + Your timeout has finished. Please enter a new code. + + )} + {error && !lockoutExpired && ( + + + {error} + {countdownDisplay ? ( + ({countdownDisplay}) + ) : null} + + + )} diff --git a/ui/apps/console/src/hooks/useLockoutCountdown.ts b/ui/apps/console/src/hooks/useLockoutCountdown.ts new file mode 100644 index 00000000000..fa284d3fac6 --- /dev/null +++ b/ui/apps/console/src/hooks/useLockoutCountdown.ts @@ -0,0 +1,53 @@ +import { useState, useEffect } from "react"; + +interface CountdownState { + display: string; + expired: boolean; + epoch: number | null; +} + +/** + * Counts down to the end of an account lockout, given as the Unix timestamp in seconds that the + * `X-Account-Lockout` header carries. `display` is the time left in whole minutes or seconds, empty + * until the first tick. `expired` turns true once the lockout ends, and both reset when a new + * lockout replaces the old one, so a second lockout never reads as already finished. + */ +export function useLockoutCountdown(lockoutEndEpoch: number | null) { + const [state, setState] = useState({ + display: "", + expired: false, + epoch: null, + }); + + useEffect(() => { + if (lockoutEndEpoch === null) return; + + const interval = setInterval(() => { + const diff = lockoutEndEpoch - Date.now() / 1000; + if (diff <= 0) { + clearInterval(interval); + setState({ display: "", expired: true, epoch: lockoutEndEpoch }); + } else if (diff < 60) { + const s = Math.floor(diff); + setState({ + display: `${s} ${s === 1 ? "second" : "seconds"}`, + expired: false, + epoch: lockoutEndEpoch, + }); + } else { + const m = Math.floor(diff / 60); + setState({ + display: `${m} ${m === 1 ? "minute" : "minutes"}`, + expired: false, + epoch: lockoutEndEpoch, + }); + } + }, 1000); + + return () => clearInterval(interval); + }, [lockoutEndEpoch]); + + if (state.epoch !== lockoutEndEpoch) return { display: "", expired: false }; + + return { display: state.display, expired: state.expired }; +} diff --git a/ui/apps/console/src/pages/__tests__/MfaLogin.test.tsx b/ui/apps/console/src/pages/__tests__/MfaLogin.test.tsx index e5577f5f593..2cc4027ad7f 100644 --- a/ui/apps/console/src/pages/__tests__/MfaLogin.test.tsx +++ b/ui/apps/console/src/pages/__tests__/MfaLogin.test.tsx @@ -1,6 +1,8 @@ import { describe, it, expect, beforeEach, vi } from "vitest"; import { render, screen, fireEvent, waitFor } from "@testing-library/react"; import { MemoryRouter, Route, Routes } from "react-router-dom"; +import { http, HttpResponse } from "msw"; +import { server } from "@/tests/msw"; import { useAuthStore } from "@/stores/authStore"; import { PENDING_DEVICE_CODE_KEY, @@ -35,6 +37,21 @@ function submitCode() { fireEvent.click(screen.getByRole("button", { name: /verify/i })); } +const realLoginWithMfa = useAuthStore.getState().loginWithMfa; + +function lockOutMfa(secondsLeft: number) { + const epoch = Math.floor(Date.now() / 1000) + secondsLeft; + server.use( + http.post("*/api/user/mfa/auth", () => + HttpResponse.json( + {}, + { status: 429, headers: { "x-account-lockout": String(epoch) } }, + ), + ), + ); + useAuthStore.setState({ loginWithMfa: realLoginWithMfa }); +} + describe("MfaLogin", () => { beforeEach(() => { localStorage.removeItem(PENDING_DEVICE_CODE_KEY); @@ -95,6 +112,43 @@ describe("MfaLogin", () => { }); }); + describe("lockout", () => { + it("shows the lockout and its remaining time instead of a wrong-code message", async () => { + lockOutMfa(30); + + renderMfaLogin(); + fillCode(); + submitCode(); + + await screen.findByText(/too many failed attempts/i); + expect( + screen.queryByText("Invalid verification code"), + ).not.toBeInTheDocument(); + await waitFor( + () => expect(screen.getByText(/seconds/i)).toBeInTheDocument(), + { timeout: 2000 }, + ); + expect(useAuthStore.getState().mfaToken).toBe("temp-mfa-token"); + }); + + it("tells the user to try again once the lockout ends", async () => { + lockOutMfa(1); + + renderMfaLogin(); + fillCode(); + submitCode(); + + await screen.findByText(/too many failed attempts/i); + await waitFor( + () => + expect( + screen.getByText(/your timeout has finished/i), + ).toBeInTheDocument(), + { timeout: 4000 }, + ); + }); + }); + it("has link to recovery page", () => { renderMfaLogin(); diff --git a/ui/apps/console/src/stores/authStore.ts b/ui/apps/console/src/stores/authStore.ts index 4946a4821b9..d6cdeadaade 100644 --- a/ui/apps/console/src/stores/authStore.ts +++ b/ui/apps/console/src/stores/authStore.ts @@ -11,6 +11,7 @@ import { type UserOrigin, } from "../client"; import { queryClient } from "../api/queryClient"; +import { isSdkError } from "../api/errors"; import { tearDownChatwoot } from "../hooks/chatwootRuntime"; import { useVaultStore } from "./vaultStore"; import { useTerminalStore } from "./terminalStore"; @@ -235,7 +236,14 @@ export const useAuthStore = create()( mfaEnabled: true, loading: false, }); - } catch { + } catch (err) { + if (isSdkError(err) && err.status === 429) { + set({ + loading: false, + error: "Too many failed attempts. Please wait before trying again.", + }); + throw err; + } set({ loading: false, error: "Invalid verification code" }); throw new Error("Invalid verification code"); } From 386d1e8b118d665aafff607de06ab699288d6cb9 Mon Sep 17 00:00:00 2001 From: Geovanne Washington Date: Fri, 2 Oct 2026 16:08:20 -0300 Subject: [PATCH 3/7] fix(ui): keep the MFA step on a wrong code instead of signing out The MFA code step now answers a wrong code with 401, and the response interceptor treated any 401 outside /api/login as an expired session, so a typo sent the user back to the password step. A 429 without the lockout header also read as an already finished lockout, since Number(null) is 0. The tests run the real auth store through MSW. The countdown ones advance fake time one second per act() call, because the form stores the deadline after the error message renders, so its interval exists only after that commit. --- ui/apps/console/src/api/fetchInterceptors.ts | 6 +- .../src/components/mfa/MfaCodeForm.tsx | 2 +- .../src/pages/__tests__/MfaLogin.test.tsx | 90 +++++++++++++------ 3 files changed, 66 insertions(+), 32 deletions(-) diff --git a/ui/apps/console/src/api/fetchInterceptors.ts b/ui/apps/console/src/api/fetchInterceptors.ts index faf35a67c64..b9a09744e38 100644 --- a/ui/apps/console/src/api/fetchInterceptors.ts +++ b/ui/apps/console/src/api/fetchInterceptors.ts @@ -70,11 +70,13 @@ client.interceptors.response.use((response) => { if (mfaToken) { useAuthStore.getState().setMfaToken(mfaToken); } else { - const isLoginRequest = response.url.includes("/api/login"); + const isSignInStep = + response.url.includes("/api/login") || + response.url.includes("/api/user/mfa/auth"); const isTokenLogin = new URLSearchParams(window.location.search).has( "token", ); - if (!isLoginRequest && !isTokenLogin) { + if (!isSignInStep && !isTokenLogin) { useAuthStore.getState().logout(); window.location.href = "/login"; } diff --git a/ui/apps/console/src/components/mfa/MfaCodeForm.tsx b/ui/apps/console/src/components/mfa/MfaCodeForm.tsx index a9d4dd5ec98..57d2f6449e7 100644 --- a/ui/apps/console/src/components/mfa/MfaCodeForm.tsx +++ b/ui/apps/console/src/components/mfa/MfaCodeForm.tsx @@ -42,7 +42,7 @@ export default function MfaCodeForm({ } catch (err) { if (isSdkError(err) && err.status === 429) { const epoch = Number(err.headers.get("x-account-lockout")); - setLockoutEndEpoch(isNaN(epoch) ? null : epoch); + setLockoutEndEpoch(epoch > 0 ? epoch : null); } otp.reset(); } diff --git a/ui/apps/console/src/pages/__tests__/MfaLogin.test.tsx b/ui/apps/console/src/pages/__tests__/MfaLogin.test.tsx index 2cc4027ad7f..75d2b0e99e5 100644 --- a/ui/apps/console/src/pages/__tests__/MfaLogin.test.tsx +++ b/ui/apps/console/src/pages/__tests__/MfaLogin.test.tsx @@ -1,5 +1,11 @@ -import { describe, it, expect, beforeEach, vi } from "vitest"; -import { render, screen, fireEvent, waitFor } from "@testing-library/react"; +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { + act, + render, + screen, + fireEvent, + waitFor, +} from "@testing-library/react"; import { MemoryRouter, Route, Routes } from "react-router-dom"; import { http, HttpResponse } from "msw"; import { server } from "@/tests/msw"; @@ -37,31 +43,30 @@ function submitCode() { fireEvent.click(screen.getByRole("button", { name: /verify/i })); } -const realLoginWithMfa = useAuthStore.getState().loginWithMfa; - -function lockOutMfa(secondsLeft: number) { - const epoch = Math.floor(Date.now() / 1000) + secondsLeft; +function respondToCode(status: number, headers: Record = {}) { server.use( http.post("*/api/user/mfa/auth", () => - HttpResponse.json( - {}, - { status: 429, headers: { "x-account-lockout": String(epoch) } }, - ), + HttpResponse.json({}, { status, headers }), ), ); - useAuthStore.setState({ loginWithMfa: realLoginWithMfa }); +} + +async function passSeconds(seconds: number) { + for (let i = 0; i < seconds; i++) { + await act(() => vi.advanceTimersByTimeAsync(1000)); + } +} + +function lockOutMfa(secondsLeft: number) { + const epoch = Math.floor(Date.now() / 1000) + secondsLeft; + respondToCode(429, { "x-account-lockout": String(epoch) }); } describe("MfaLogin", () => { beforeEach(() => { localStorage.removeItem(PENDING_DEVICE_CODE_KEY); - useAuthStore.setState({ - token: null, - mfaToken: "temp-mfa-token", - loading: false, - error: null, - loginWithMfa: vi.fn(), - }); + useAuthStore.setState(useAuthStore.getInitialState(), true); + useAuthStore.setState({ mfaToken: "temp-mfa-token" }); }); it("renders MFA login form when mfaToken exists", () => { @@ -112,7 +117,27 @@ describe("MfaLogin", () => { }); }); + it("keeps the user on the code step, with the token, after a wrong code", async () => { + respondToCode(401); + + renderMfaLogin(); + fillCode(6, "9"); + submitCode(); + + await screen.findByText("Invalid verification code"); + expect(useAuthStore.getState().mfaToken).toBe("temp-mfa-token"); + expect(screen.queryByText("Login Page")).not.toBeInTheDocument(); + }); + describe("lockout", () => { + beforeEach(() => { + vi.useFakeTimers({ shouldAdvanceTime: true }); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + it("shows the lockout and its remaining time instead of a wrong-code message", async () => { lockOutMfa(30); @@ -124,10 +149,8 @@ describe("MfaLogin", () => { expect( screen.queryByText("Invalid verification code"), ).not.toBeInTheDocument(); - await waitFor( - () => expect(screen.getByText(/seconds/i)).toBeInTheDocument(), - { timeout: 2000 }, - ); + await passSeconds(1); + expect(screen.getByText(/seconds/i)).toBeInTheDocument(); expect(useAuthStore.getState().mfaToken).toBe("temp-mfa-token"); }); @@ -139,13 +162,22 @@ describe("MfaLogin", () => { submitCode(); await screen.findByText(/too many failed attempts/i); - await waitFor( - () => - expect( - screen.getByText(/your timeout has finished/i), - ).toBeInTheDocument(), - { timeout: 4000 }, - ); + await passSeconds(3); + expect(screen.getByText(/your timeout has finished/i)).toBeInTheDocument(); + }); + + it("shows the lockout without a countdown when the deadline header is missing", async () => { + respondToCode(429); + + renderMfaLogin(); + fillCode(); + submitCode(); + + await screen.findByText(/too many failed attempts/i); + await passSeconds(3); + expect( + screen.queryByText(/your timeout has finished/i), + ).not.toBeInTheDocument(); }); }); From 54a442736570dcdc295344460cdb1519e748c978 Mon Sep 17 00:00:00 2001 From: Geovanne Washington Date: Fri, 2 Oct 2026 16:44:49 -0300 Subject: [PATCH 4/7] docs(openapi): say the MFA lockout is separate from the password one The code step now counts wrong codes apart from wrong passwords, because a correct password resets the password counter and would otherwise clear the MFA one too. --- openapi/spec/paths/api@user@mfa@auth.yaml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/openapi/spec/paths/api@user@mfa@auth.yaml b/openapi/spec/paths/api@user@mfa@auth.yaml index 695c9e7b33b..4341de32016 100644 --- a/openapi/spec/paths/api@user@mfa@auth.yaml +++ b/openapi/spec/paths/api@user@mfa@auth.yaml @@ -6,8 +6,9 @@ post: which generates an `X-MFA-Token` indicating that the user has already authenticated with a password. The token logs in once only. An unknown, expired or already used token answers `401`, the same - as a wrong code. Wrong codes count toward the same account lockout as wrong passwords: check the - `X-Account-Lockout` header, which is 0 when there is no active lockout. + as a wrong code. Wrong codes count toward an account lockout of their own, which a correct + password does not clear: check the `X-Account-Lockout` header, which is 0 when there is no + active lockout. tags: - cloud - mfa From 2950b69064731f47382ea10b4a32e699e086ffba Mon Sep 17 00:00:00 2001 From: Geovanne Washington Date: Fri, 2 Oct 2026 17:04:57 -0300 Subject: [PATCH 5/7] fix(ui): read the lockout header the same way on both sign-in steps The password form parsed X-Account-Lockout with an isNaN check, so a 429 without the header read as a lockout that ended at epoch 0 and showed "Your timeout has finished" at once. Both forms now use lockoutEndFrom, which treats a missing, zero or malformed header as no countdown. --- .../console/src/components/auth/SignInForm.tsx | 8 +++++--- .../console/src/components/mfa/MfaCodeForm.tsx | 8 +++++--- .../console/src/hooks/useLockoutCountdown.ts | 11 +++++++++++ .../console/src/pages/__tests__/Login.test.tsx | 17 ++++++++++++++++- .../src/pages/__tests__/MfaLogin.test.tsx | 7 +++++-- 5 files changed, 42 insertions(+), 9 deletions(-) diff --git a/ui/apps/console/src/components/auth/SignInForm.tsx b/ui/apps/console/src/components/auth/SignInForm.tsx index f89c517ab1a..b99b5e02616 100644 --- a/ui/apps/console/src/components/auth/SignInForm.tsx +++ b/ui/apps/console/src/components/auth/SignInForm.tsx @@ -4,7 +4,10 @@ import { useNavigate } from "react-router-dom"; import { Button, Callout } from "@shellhub/design-system/primitives"; import { isSdkError } from "@/api/errors"; import { useAuthStore } from "@/stores/authStore"; -import { useLockoutCountdown } from "@/hooks/useLockoutCountdown"; +import { + lockoutEndFrom, + useLockoutCountdown, +} from "@/hooks/useLockoutCountdown"; import { landingAfterSignIn } from "@/utils/navigation"; import { FormInputField, @@ -98,8 +101,7 @@ export default function SignInForm({ ); break; case 429: { - const epoch = Number(err.headers.get("x-account-lockout")); - setLockoutEndEpoch(isNaN(epoch) ? null : epoch); + setLockoutEndEpoch(lockoutEndFrom(err.headers)); setError( "Too many failed login attempts. Please wait before trying again.", ); diff --git a/ui/apps/console/src/components/mfa/MfaCodeForm.tsx b/ui/apps/console/src/components/mfa/MfaCodeForm.tsx index 57d2f6449e7..688819673d8 100644 --- a/ui/apps/console/src/components/mfa/MfaCodeForm.tsx +++ b/ui/apps/console/src/components/mfa/MfaCodeForm.tsx @@ -2,7 +2,10 @@ import { useState, type FormEvent } from "react"; import { Button, Callout } from "@shellhub/design-system/primitives"; import { isSdkError } from "@/api/errors"; import { useAuthStore } from "@/stores/authStore"; -import { useLockoutCountdown } from "@/hooks/useLockoutCountdown"; +import { + lockoutEndFrom, + useLockoutCountdown, +} from "@/hooks/useLockoutCountdown"; import { useOtpInput } from "@/hooks/useOtpInput"; import OtpCells from "@/components/mfa/OtpCells"; import AuthActions, { type AuthLink } from "@/components/auth/AuthActions"; @@ -41,8 +44,7 @@ export default function MfaCodeForm({ onVerified(); } catch (err) { if (isSdkError(err) && err.status === 429) { - const epoch = Number(err.headers.get("x-account-lockout")); - setLockoutEndEpoch(epoch > 0 ? epoch : null); + setLockoutEndEpoch(lockoutEndFrom(err.headers)); } otp.reset(); } diff --git a/ui/apps/console/src/hooks/useLockoutCountdown.ts b/ui/apps/console/src/hooks/useLockoutCountdown.ts index fa284d3fac6..0eebf85a13b 100644 --- a/ui/apps/console/src/hooks/useLockoutCountdown.ts +++ b/ui/apps/console/src/hooks/useLockoutCountdown.ts @@ -6,6 +6,17 @@ interface CountdownState { epoch: number | null; } +/** + * Reads the end of an account lockout from a 429 response's `X-Account-Lockout` header, as the Unix + * timestamp in seconds that {@link useLockoutCountdown} takes. A missing, zero or malformed header + * gives `null`, so the form shows the lockout without a countdown rather than one that already ended. + */ +export function lockoutEndFrom(headers: Headers): number | null { + const epoch = Number(headers.get("x-account-lockout")); + + return epoch > 0 ? epoch : null; +} + /** * Counts down to the end of an account lockout, given as the Unix timestamp in seconds that the * `X-Account-Lockout` header carries. `display` is the time left in whole minutes or seconds, empty diff --git a/ui/apps/console/src/pages/__tests__/Login.test.tsx b/ui/apps/console/src/pages/__tests__/Login.test.tsx index 41a9f4cee3d..e74efd8e210 100644 --- a/ui/apps/console/src/pages/__tests__/Login.test.tsx +++ b/ui/apps/console/src/pages/__tests__/Login.test.tsx @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; -import { render, screen, waitFor } from "@testing-library/react"; +import { act, render, screen, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { MemoryRouter } from "react-router-dom"; import { http, HttpResponse } from "msw"; @@ -307,6 +307,21 @@ describe("Login", () => { screen.queryByText(/too many failed login attempts/i), ).not.toBeInTheDocument(); }); + + it("shows the lockout without a countdown when the deadline header is missing", async () => { + vi.useFakeTimers({ shouldAdvanceTime: true }); + setLoginError(429); + + renderLogin(); + await fillAndSubmit(); + + await screen.findByText(/too many failed login attempts/i); + await act(() => vi.advanceTimersByTimeAsync(3000)); + expect( + screen.queryByText(/your timeout has finished/i), + ).not.toBeInTheDocument(); + expect(screen.queryByText(/seconds|minutes/i)).not.toBeInTheDocument(); + }); }); describe("under a browser-translated DOM", () => { diff --git a/ui/apps/console/src/pages/__tests__/MfaLogin.test.tsx b/ui/apps/console/src/pages/__tests__/MfaLogin.test.tsx index 75d2b0e99e5..b5bd73cfaee 100644 --- a/ui/apps/console/src/pages/__tests__/MfaLogin.test.tsx +++ b/ui/apps/console/src/pages/__tests__/MfaLogin.test.tsx @@ -150,7 +150,7 @@ describe("MfaLogin", () => { screen.queryByText("Invalid verification code"), ).not.toBeInTheDocument(); await passSeconds(1); - expect(screen.getByText(/seconds/i)).toBeInTheDocument(); + expect(screen.getByText(/\(2[89] seconds\)/)).toBeInTheDocument(); expect(useAuthStore.getState().mfaToken).toBe("temp-mfa-token"); }); @@ -163,7 +163,9 @@ describe("MfaLogin", () => { await screen.findByText(/too many failed attempts/i); await passSeconds(3); - expect(screen.getByText(/your timeout has finished/i)).toBeInTheDocument(); + expect( + screen.getByText(/your timeout has finished/i), + ).toBeInTheDocument(); }); it("shows the lockout without a countdown when the deadline header is missing", async () => { @@ -178,6 +180,7 @@ describe("MfaLogin", () => { expect( screen.queryByText(/your timeout has finished/i), ).not.toBeInTheDocument(); + expect(screen.queryByText(/seconds|minutes/i)).not.toBeInTheDocument(); }); }); From 98fd1732b49469270ada6b6fac4dea4f0ad733cf Mon Sep 17 00:00:00 2001 From: Geovanne Washington Date: Fri, 2 Oct 2026 17:05:02 -0300 Subject: [PATCH 6/7] docs(openapi): say when MFA login answers 404 and declare its lockout header Cloud sets X-Account-Lockout on every response, as /api/login does, so the 200 declares it too. A valid token whose user is gone still answers 404, and a code sent while another one is being checked answers 401. --- openapi/spec/paths/api@user@mfa@auth.yaml | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/openapi/spec/paths/api@user@mfa@auth.yaml b/openapi/spec/paths/api@user@mfa@auth.yaml index 4341de32016..63fa2f82b13 100644 --- a/openapi/spec/paths/api@user@mfa@auth.yaml +++ b/openapi/spec/paths/api@user@mfa@auth.yaml @@ -6,9 +6,10 @@ post: which generates an `X-MFA-Token` indicating that the user has already authenticated with a password. The token logs in once only. An unknown, expired or already used token answers `401`, the same - as a wrong code. Wrong codes count toward an account lockout of their own, which a correct - password does not clear: check the `X-Account-Lockout` header, which is 0 when there is no - active lockout. + as a wrong code, and so does a code sent while another code for the same user is still being + checked. A token whose user no longer exists answers `404`. Wrong codes count toward an account + lockout of their own, which a correct password does not clear: check the `X-Account-Lockout` + header, which is 0 when there is no active lockout. tags: - cloud - mfa @@ -22,6 +23,12 @@ post: responses: '200': description: Success to authenticate. + headers: + X-Account-Lockout: + schema: + type: integer + description: Indicates the Unix timestamp in seconds of when the account lockout period ends. + example: 0 content: application/json: schema: From 9ac2a9882f844a98ad1225cc64c58478f671fc72 Mon Sep 17 00:00:00 2001 From: Geovanne Washington Date: Fri, 2 Oct 2026 17:46:42 -0300 Subject: [PATCH 7/7] feat(pkg): release a SetNX reservation only while the caller still owns it Cloud's MFA login takes a per-user in-flight guard with SetNX and deletes it when the guess is done. If a request outlives the guard's TTL, another one takes the guard, and a plain Delete from the first frees it, letting a third guess run next to the second. CompareAndDelete runs GET and DEL in one Lua script, so the owner's value decides the delete atomically. The mock was written to match mockery's output by hand, because mockery cannot load the root packages inside the dev container. Refs: shellhub-io/team#264 --- pkg/cache/cache.go | 5 +++ pkg/cache/cache_null.go | 4 ++ pkg/cache/cache_redis.go | 16 ++++++++ pkg/cache/cache_redis_test.go | 37 ++++++++++++++++++ pkg/cache/mocks/mock_cache.go | 72 +++++++++++++++++++++++++++++++++++ 5 files changed, 134 insertions(+) diff --git a/pkg/cache/cache.go b/pkg/cache/cache.go index 3cdbaa6a816..ac1d8a34fa1 100644 --- a/pkg/cache/cache.go +++ b/pkg/cache/cache.go @@ -39,6 +39,11 @@ type Cache interface { // exactly once under concurrent requests, which Get+Set cannot do race-free. SetNX(ctx context.Context, key string, value any, ttl time.Duration) (bool, error) + // CompareAndDelete atomically deletes key only while it still holds value, and reports whether + // it did. It releases a reservation taken with SetNX under a value unique to its owner, so an + // owner whose reservation expired cannot delete the one another caller took after it. + CompareAndDelete(ctx context.Context, key, value string) (bool, error) + // HasAccountLockout reports whether the source is currently blocked from attempting to // log in to a user with the specified userID. It returns the absolute Unix timestamp // in seconds representing the end of the lockout while clock.Now() is before it, or 0 diff --git a/pkg/cache/cache_null.go b/pkg/cache/cache_null.go index 87d6edbd370..8f6f2229076 100644 --- a/pkg/cache/cache_null.go +++ b/pkg/cache/cache_null.go @@ -31,6 +31,10 @@ func (*nullCache) Delete(_ context.Context, _ string) error { return nil } +func (*nullCache) CompareAndDelete(_ context.Context, _, _ string) (bool, error) { + return true, nil +} + func (*nullCache) HasAccountLockout(_ context.Context, _, _ string) (int64, int, error) { return 0, 0, nil } diff --git a/pkg/cache/cache_redis.go b/pkg/cache/cache_redis.go index 498e00e824f..adabb212249 100644 --- a/pkg/cache/cache_redis.go +++ b/pkg/cache/cache_redis.go @@ -74,6 +74,22 @@ func (c *redisCache) SetNX(ctx context.Context, key string, value any, ttl time. return c.client.SetNX(ctx, key, value, ttl).Result() } +var compareAndDeleteScript = redis.NewScript(` +if redis.call("GET", KEYS[1]) == ARGV[1] then + return redis.call("DEL", KEYS[1]) +end +return 0 +`) + +func (c *redisCache) CompareAndDelete(ctx context.Context, key, value string) (bool, error) { + deleted, err := compareAndDeleteScript.Run(ctx, c.client, []string{key}, value).Int() + if err != nil { + return false, err + } + + return deleted == 1, nil +} + // Delete deletes cached value by given key. func (c *redisCache) Delete(ctx context.Context, key string) error { if err := c.cache.Get(ctx, key, nil); errors.Is(err, rediscache.ErrCacheMiss) { diff --git a/pkg/cache/cache_redis_test.go b/pkg/cache/cache_redis_test.go index 9c8a123bd44..d7b9af23aa7 100644 --- a/pkg/cache/cache_redis_test.go +++ b/pkg/cache/cache_redis_test.go @@ -66,3 +66,40 @@ func TestRedisCacheLockoutEndsAtAnnouncedDeadline(t *testing.T) { require.NoError(t, c.Get(ctx, "account-lockout=10.0.0.1:user", &storedDeadline)) assert.Equal(t, "1700000060", storedDeadline) } + +func TestRedisCacheCompareAndDeleteKeepsAnotherOwnersKey(t *testing.T) { + ctx := context.Background() + + redisContainer, err := redis.Run(ctx, "docker.io/valkey/valkey:9.1-alpine") + require.NoError(t, err) + + t.Cleanup(func() { + require.NoError(t, redisContainer.Terminate(ctx)) + }) + + uri, err := redisContainer.ConnectionString(ctx) + require.NoError(t, err) + + c, err := cache.NewRedisCache(uri, 0) + require.NoError(t, err) + + taken, err := c.SetNX(ctx, "guard", "owner", time.Minute) + require.NoError(t, err) + require.True(t, taken) + + deleted, err := c.CompareAndDelete(ctx, "guard", "someone-else") + require.NoError(t, err) + assert.False(t, deleted) + + taken, err = c.SetNX(ctx, "guard", "intruder", time.Minute) + require.NoError(t, err) + assert.False(t, taken) + + deleted, err = c.CompareAndDelete(ctx, "guard", "owner") + require.NoError(t, err) + assert.True(t, deleted) + + taken, err = c.SetNX(ctx, "guard", "next", time.Minute) + require.NoError(t, err) + assert.True(t, taken) +} diff --git a/pkg/cache/mocks/mock_cache.go b/pkg/cache/mocks/mock_cache.go index 37f60cfb529..11e494ca7b2 100644 --- a/pkg/cache/mocks/mock_cache.go +++ b/pkg/cache/mocks/mock_cache.go @@ -47,6 +47,78 @@ func (_m *MockCache) EXPECT() *MockCache_Expecter { return &MockCache_Expecter{mock: &_m.Mock} } +// CompareAndDelete provides a mock function for the type MockCache +func (_mock *MockCache) CompareAndDelete(ctx context.Context, key string, value string) (bool, error) { + ret := _mock.Called(ctx, key, value) + + if len(ret) == 0 { + panic("no return value specified for CompareAndDelete") + } + + var r0 bool + var r1 error + if returnFunc, ok := ret.Get(0).(func(context.Context, string, string) (bool, error)); ok { + return returnFunc(ctx, key, value) + } + if returnFunc, ok := ret.Get(0).(func(context.Context, string, string) bool); ok { + r0 = returnFunc(ctx, key, value) + } else { + r0 = ret.Get(0).(bool) + } + if returnFunc, ok := ret.Get(1).(func(context.Context, string, string) error); ok { + r1 = returnFunc(ctx, key, value) + } else { + r1 = ret.Error(1) + } + return r0, r1 +} + +// MockCache_CompareAndDelete_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'CompareAndDelete' +type MockCache_CompareAndDelete_Call struct { + *mock.Call +} + +// CompareAndDelete is a helper method to define mock.On call +// - ctx context.Context +// - key string +// - value string +func (_e *MockCache_Expecter) CompareAndDelete(ctx any, key any, value any) *MockCache_CompareAndDelete_Call { + return &MockCache_CompareAndDelete_Call{Call: _e.mock.On("CompareAndDelete", ctx, key, value)} +} + +func (_c *MockCache_CompareAndDelete_Call) Run(run func(ctx context.Context, key string, value string)) *MockCache_CompareAndDelete_Call { + _c.Call.Run(func(args mock.Arguments) { + var arg0 context.Context + if args[0] != nil { + arg0 = args[0].(context.Context) + } + var arg1 string + if args[1] != nil { + arg1 = args[1].(string) + } + var arg2 string + if args[2] != nil { + arg2 = args[2].(string) + } + run( + arg0, + arg1, + arg2, + ) + }) + return _c +} + +func (_c *MockCache_CompareAndDelete_Call) Return(b bool, err error) *MockCache_CompareAndDelete_Call { + _c.Call.Return(b, err) + return _c +} + +func (_c *MockCache_CompareAndDelete_Call) RunAndReturn(run func(ctx context.Context, key string, value string) (bool, error)) *MockCache_CompareAndDelete_Call { + _c.Call.Return(run) + return _c +} + // Delete provides a mock function for the type MockCache func (_mock *MockCache) Delete(ctx context.Context, key string) error { ret := _mock.Called(ctx, key)