feat(ui): add two-factor authentication (TOTP) support

[luannmoreira]

Summary

Adds two-factor authentication (TOTP) support to the admin UI, allowing users to secure their accounts with
authenticator apps like Google Authenticator, Authy, or 1Password.

Key Features:

• MFA Setup - Users can enable 2FA from their profile page with QR code scanning
• Recovery Codes - Generates 10 single-use backup codes for account recovery
• MFA Login Flow - Dedicated login page with 6-digit code input (supports paste)
• Recovery Flow - Users can regain access using backup codes if they lose their TOTP device
• Seamless Navigation - Programmatic routing without page reloads preserves auth state

Implementation Details:

• Auth flow handles MFA challenges (401 with x-mfa-token header) without page reloads
• MFA tokens are stored in memory only (not persisted) for security
• Recovery codes displayed once during setup with copy/download options
• Countdown timer shows recovery code expiry (7 days after use)

Dependencies Added:

• qrcode.react - QR code generation for authenticator app setup
• otpauth - TOTP URI formatting (otpauth://totp/...)

Closes https://github.com/shellhub-io/team/issues/62

[github-actions]

Claude finished @luannmoreira's task in 9m 15s —— View job

---

Code Review Complete

Reviewed 18 files across code quality, security, testing, TypeScript/React patterns, and architecture. 14 inline issues posted (2 critical, 4 high, 7 medium, 1 low).

Additional notes

MFA endpoints are cloud/enterprise-only: All /api/user/mfa/* endpoints are registered exclusively in cloud/internal/cloud/routes/routes.go. The community edition API has no MFA route handlers, meaning the entire MFA UI will produce 404 errors in CE. The Profile page MFA section needs a feature gate (similar to how FeatureGate is used for firewall rules) to hide MFA when running community edition, or the MFA endpoints need to be added to the core API.

No test coverage for any new MFA code: The PR adds ~2100 lines of new code with zero tests. The existing authStore.test.ts and interceptors.test.ts don't cover the new MFA actions or the modified 401 handling. Critical test gaps include:

• loginWithMfa, recoverWithCode, and MFA-aware login flow in authStore
• 401 interceptor branch for x-mfa-token header
• useCountdown hook (time-sensitive security logic)
• MFA login/recover page components

Code duplication: OTP input handling (state, validation, auto-advance, backspace) is duplicated across 3 components. Footer links are duplicated across 3 pages. Download/copy utility logic is duplicated across 2 modals. Consider extracting shared OtpInput component, AuthPageFooter, and downloadTextFile utility.

MfaEnableDrawer is 552 lines: Manages 4 distinct steps with separate state, handlers, and UI. Consider extracting each step into its own component.

---

If you've addressed the feedback and want a new review, tag @shellhub-io/admin and a team member can trigger it.

[otavio]

/review

[github-actions]

low -- as string | undefined type assertion on header value

The as string | undefined cast bypasses type checking. Axios headers values can be string | number | boolean | null | undefined (depending on version). A safer approach:

Suggested change

	expiresAt: (response.headers["x-expires-at"] as string | undefined) || "",
	expiresAt: String(response.headers["x-expires-at"] ?? ""),

Or use a type guard:

const raw = response.headers["x-expires-at"];
const expiresAt = typeof raw === "string" ? raw : "";

[github-actions]

medium -- requestMfaReset store action name shadows the imported API function

The store action requestMfaReset on line 228 has the same name as the imported API function from ../api/mfa on line 12. Inside the store action body, requestMfaReset(identifier) on line 231 resolves to the outer import (which is correct), but this naming collision is fragile and confusing. If someone refactors and accidentally uses get().requestMfaReset(identifier) thinking it's the API call, they'd create infinite recursion.

Consider renaming either the import or the store action:

import { requestMfaReset as apiRequestMfaReset } from "../api/mfa";

This is consistent with the existing pattern on line 7 (updatePassword as apiUpdatePassword).

[github-actions]

medium -- same shadowing issue: completeMfaReset store action vs. imported API function

Same as the requestMfaReset shadowing above. The store action completeMfaReset shadows the import from ../api/mfa line 13. The inner call on line 252 resolves correctly to the import, but this is another source of confusion.

Suggested change

	completeMfaReset: async (mainEmailCode: string, recoveryEmailCode: string) => {
	const { mfaResetUserId } = get();
	if (!mfaResetUserId) {
	set({ error: "Invalid reset session. Please start over." });
	throw new Error("No user ID available");
	}
	set({ loading: true, error: null });
	try {
	const data = await completeMfaReset(mfaResetUserId, {
	main_email_code: mainEmailCode,
	// At the import (line 13), rename:
	import {
	validateMfa,
	recoverMfa,
	requestMfaReset as apiRequestMfaReset,
	completeMfaReset as apiCompleteMfaReset,
	} from "../api/mfa";

[github-actions]

medium -- handleSaveRecoveryEmail called from onClick on a <button>, not from <form onSubmit>; the FormEvent parameter is potentially undefined

The handleSaveRecoveryEmail function signature expects (e: FormEvent) and calls e.preventDefault(). However, on line 214 it is used as an onClick handler. A MouseEvent is not a FormEvent. This works at runtime because both have preventDefault(), but:

1. The TypeScript types are mismatched (React may not flag this due to how event handlers are typed, but it's semantically wrong).
2. If the button is inside a <form> (which it is -- it's in the Drawer footer), clicking it could also trigger form submission since there's no type="button" attribute on this button.

Consider adding type="button" to the button to prevent accidental form submission, or restructuring to use form submission.

[github-actions]

low -- err parameter caught but never used

The catch (err) captures the error but only calls console.error with a string literal. Either use the caught error in the log message or remove the binding:

Suggested change

	} catch (err) {
	// Show error to user - they may need to re-authenticate
	console.error("Failed to disable MFA");
	// Close modal and let user see error from the API
	setShowTimeoutModal(false);
	}
	} catch {
	// Show error to user - they may need to re-authenticate
	console.error("Failed to disable MFA");
	// Close modal and let user see error from the API
	setShowTimeoutModal(false);
	}

[github-actions]

medium -- broad Zustand selector pulling 8 properties, including user, username, and mfaToken that are only used for computing identifier and a redirect guard

This component destructures 8 fields from useAuthStore inline, which means it re-renders on ANY store change (since object destructuring from the full store creates a new object reference each time).

Consider using atomic selectors for the frequently-changing fields:

const loading = useAuthStore((s) => s.loading);
const error = useAuthStore((s) => s.error);
const mfaRecoveryExpiry = useAuthStore((s) => s.mfaRecoveryExpiry);
const recoverWithCode = useAuthStore((s) => s.recoverWithCode);
const updateMfaStatus = useAuthStore((s) => s.updateMfaStatus);

Or use Zustand's useShallow to do a shallow comparison:

import { useShallow } from "zustand/react/shallow";
const { recoverWithCode, loading, error, ... } = useAuthStore(
  useShallow((s) => ({ ... }))
);

[github-actions]

medium -- same broad Zustand selector pattern as MfaRecover

This destructures 6 fields inline from useAuthStore, causing re-renders on any store change. Same recommendation as MfaRecover: use individual selectors or useShallow.

[github-actions]

medium -- broad Zustand selector: { user, username, requestMfaReset } via object destructure

This destructures from the full store, meaning the component re-renders on ANY store state change (e.g., loading, error, token changes). Since MfaDisableDialog only needs user, username, and requestMfaReset, use individual selectors:

Suggested change

	const { user, username, requestMfaReset } = useAuthStore();
	const user = useAuthStore((s) => s.user);
	const username = useAuthStore((s) => s.username);
	const requestMfaReset = useAuthStore((s) => s.requestMfaReset);

[github-actions]

medium -- broad Zustand selector on Profile page

Same pattern: destructuring 6 fields from useAuthStore inline causes re-renders on any store change. The fetchUser function reference is stable (it's defined in the store), but name, username, email, recoveryEmail, and mfaEnabled all being read from a single destructure means this component re-renders when unrelated store fields (like loading, error, mfaToken) change.

Use individual selectors or useShallow:

const name = useAuthStore((s) => s.name);
const username = useAuthStore((s) => s.username);
// etc.

[github-actions]

low -- error parameter in handleGenerateMfa shadows the component-level error state variable

The catch (error) on line 98 shadows the error state from useState on line 34. While this does not cause a runtime bug (the catch block only uses setError and throw error), it makes the code harder to reason about. Consider renaming:

Suggested change

	const handleGenerateMfa = async () => {
	try {
	const data: MfaGenerateResponse = await generateMfa();
	setQrLink(data.link);
	setSecret(data.secret);
	setRecoveryCodes(data.recovery_codes);
	} catch (error) {
	setError("Failed to generate MFA codes");
	throw error; // Re-throw so callers know it failed
	}
	const handleGenerateMfa = async () => {
	try {
	const data: MfaGenerateResponse = await generateMfa();
	setQrLink(data.link);
	setSecret(data.secret);
	setRecoveryCodes(data.recovery_codes);
	} catch (err) {
	setError("Failed to generate MFA codes");
	throw err; // Re-throw so callers know it failed
	}
	};

[github-actions]

medium -- onClose in useEffect dependency array can cause stale closure or re-registration

If onClose is not wrapped in useCallback by the parent, a new function reference is passed on every parent render, causing this useEffect to remove and re-add the keydown listener repeatedly.

Looking at the parent (MfaRecover.tsx line 61), handleCloseModal is defined as a plain function inside the component body, so it IS recreated every render.

Consider either:

1. Wrapping handleCloseModal in useCallback in the parent, or
2. Using a ref for the onClose callback in this component to avoid re-registering the listener:

const onCloseRef = useRef(onClose);
onCloseRef.current = onClose;

useEffect(() => {
  const handleEscape = (e: KeyboardEvent) => {
    if (e.key === "Escape" && !disabling) {
      onCloseRef.current();
    }
  };
  if (open) {
    document.addEventListener("keydown", handleEscape);
  }
  return () => document.removeEventListener("keydown", handleEscape);
}, [open, disabling]);

[github-actions]

low -- mfaToken header value has implicit any type from Axios headers

error.response.headers["x-mfa-token"] returns string | number | boolean | string[] | undefined depending on the Axios version. The value is then passed to setMfaToken(mfaToken) which expects string | null. There's no runtime issue because header values are typically strings, but TypeScript doesn't enforce this.

Consider adding a type guard:

const mfaToken = error.response.headers["x-mfa-token"];
if (typeof mfaToken === "string" && isLoginEndpoint) {

[github-actions]

[Security -- MEDIUM] Recovery-window disable sends empty payload -- always fails

disableMfa({}) sends an empty JSON body to PUT /api/user/mfa/disable. The backend's DisableMFA handler requires either code (TOTP) or recovery_code to authorize the disable. When both are empty strings, the backend's switch statement hits the default case and returns NewErrUnauthorizedMFA(nil).

This means the "Disable MFA" button in the MfaRecoveryTimeoutModal always fails during the recovery window.

The backend was designed so that the same recovery code used for RecoverMFA is cached for 10 minutes, allowing the user to reuse it for DisableMFA. But the frontend discards the recovery code after successful recovery (line 41 in MfaRecover.tsx clears recoveryCode state).

Fix: Preserve the recovery code used for account recovery and pass it to disableMfa:

Suggested change

	const handleDisableMfa = async () => {
	try {
	// During recovery window, backend validates via session token
	// No need to re-send the already-consumed recovery code
	await disableMfa({});
	// During recovery window, re-send the recovery code that was cached by the backend
	// The backend accepts the same recovery code within 10 minutes of recovery
	await disableMfa({ recovery_code: recoveryCode });

This requires keeping recoveryCode in state (do not clear it on line 41) until after the recovery window expires or MFA is disabled.

[github-actions]

[Security -- LOW] console.error leaks full error object

This console.error logs the full error object, which may include Axios response data, headers, or internal server details that could aid an attacker inspecting the browser console.

Suggested change

	console.error("Failed to disable MFA during recovery window:", error);
	console.error("Failed to disable MFA during recovery window");

Omit the error parameter or log only a safe subset (e.g., status code).

[github-actions]

[Security -- MEDIUM] mfaEnabled persisted to localStorage is trivially spoofable

mfaEnabled is included in the partialize output, which means it is stored in localStorage under shellhub-session. A user (or XSS payload) can set mfaEnabled: false in localStorage to suppress MFA-related UI indicators (e.g., the recovery timeout modal, security badges, etc.).

While this is not an authentication bypass (the server enforces MFA), it creates a misleading security posture in the UI. If any frontend logic ever gates sensitive operations on mfaEnabled client-side, it would become exploitable.

Recommendation: Either:

1. Remove mfaEnabled from partialize (derive it from a fresh fetchUser call on app load), or
2. If it must be persisted for offline display, clearly document that it must never be used for access control decisions.

[github-actions]

[Security -- LOW] Blob URL containing recovery codes could linger in memory

URL.revokeObjectURL(url) is called synchronously after a.click(), but a.click() triggers an async download. If an exception occurs between a.click() and URL.revokeObjectURL(url), the Blob URL containing plaintext recovery codes leaks.

Consider wrapping the cleanup in a finally block with a small delay:

Suggested change

	URL.revokeObjectURL(url);
	URL.revokeObjectURL(url);

to:

try {
  a.click();
} finally {
  document.body.removeChild(a);
  setTimeout(() => URL.revokeObjectURL(url), 100);
}

[github-actions]

This entire component is dead code:

1. It is never imported or rendered by any page or parent component (only its test file references it).
2. codes is initialized as useState<string[]>([]) with the setter discarded, so codes is always [].
3. Since codes.length === 0 is always true, the "Codes Display" branch (lines 81-151) is unreachable. This makes ExclamationTriangleIcon, handleDownload, and handleCopy dead imports.

Suggestion: Remove this component and its test file, or if it has a future purpose, add a TODO and remove from this PR.

[github-actions]

Dead store action: completeMfaReset is defined here but never called from any component. MfaResetComplete.tsx calls the API function directly and then uses setCompleteSession.

This means the transient fields (mfaResetUserId, mfaResetIdentifier, pendingMfaUser) that this method cleans up are never cleared by the actual page that performs the reset.

Either use this store action from MfaResetComplete.tsx (preferred -- keeps state management centralized), or remove this dead method.

[github-actions]

Type mismatch: this function expects FormEvent and calls e.preventDefault(), but it is bound via onClick={handleSaveRecoveryEmail} (line 214) which passes a MouseEvent, not a FormEvent. While preventDefault() exists on both types so this won't crash at runtime, it's a type-safety issue.

Same issue applies to handleEnableMfa at line 126 (called via onClick at line 261).

Consider removing the FormEvent parameter from both functions since they aren't form submit handlers.

[github-actions]

This is a hook in name only -- it uses no React hooks (useState, useEffect, useRef, etc.). The use prefix by convention signals that React hook rules apply, but none do here.

Because it's structured as a hook, handleDownload and handleCopy are recreated on every render in every consumer. Consider converting to a plain utility module.

[github-actions]

Naming inconsistency: all other MFA routes use the mfa- prefix (/mfa-login, /mfa-recover, /mfa-reset-request) but this uses /reset-mfa. Consider /mfa-reset-complete for consistency. Note: since this URL is likely generated by backend email links, the backend must be updated in tandem.

[github-actions]

Loose URL matching: includes("/api/login") would also match /api/login-audit, /api/login/callback, etc. Similarly, includes("/api/user/mfa") on line 76 is overly broad. Consider stricter matching: error.config?.url === "/api/login" and startsWith for MFA endpoints.

[github-actions]

The err parameter is captured (line 53) but not passed to console.error, making debugging harder. The actual error details are lost. Should be: console.error("Failed to disable MFA during recovery window:", err);

[github-actions]

[Code Quality - Medium] After removing the regeneration feature, this component is effectively dead code:

1. codes is initialized as useState<string[]>([]) with no setter, so codes.length is always 0.
2. The entire codes-display branch (lines 81-151 with Download/Copy buttons) is unreachable.
3. The useRecoveryCodeActions import on line 6 is unused (no path ever calls handleDownload or handleCopy).
4. Most importantly, no page or component imports this modal — it's only referenced by its own test file.

The component currently renders a static warning message about codes not being viewable. Consider either:

• Removing the component entirely if "View Codes" from the Profile page was removed
• Wiring it up from Profile.tsx if the intent is to show this informational modal, and cleaning up the dead code branches

[github-actions]

[TypeScript - Medium] handleSaveRecoveryEmail is typed to accept FormEvent but is called as a button onClick handler (line 214), which passes a MouseEvent<HTMLButtonElement>. The e.preventDefault() call still works at runtime (both types have it), but the type signature is misleading.

Same issue with handleEnableMfa at line 126 — typed as FormEvent but called via onClick at line 261.

Either change the type to React.SyntheticEvent (common base), or split the logic:

Suggested change

	const handleSaveRecoveryEmail = async (e: FormEvent) => {
	e.preventDefault();
	const handleSaveRecoveryEmail = async (e: React.SyntheticEvent) => {

[github-actions]

[Security - Low] console.error("Failed to disable MFA during recovery window:", error) logs the full error object, which may contain server response data, headers, or internal details visible in browser devtools. The same pattern was flagged and fixed in MfaRecover.tsx (now just logs a string), but this instance was missed.

Suggested change

	console.error("Failed to disable MFA during recovery window:", error);
	console.error("Failed to disable MFA during recovery window");

[github-actions]

[React - Medium] onClose is included in the useEffect dependency array, but the parent component (MfaRecover.tsx) defines handleCloseModal as a plain inline function that is recreated every render. This causes the escape-key listener to be removed and re-added on every parent re-render.

Either wrap handleCloseModal in useCallback in the parent, or use a ref here to avoid the dependency:

const onCloseRef = useRef(onClose);
onCloseRef.current = onClose;

useEffect(() => {
  const handleEscape = (e: KeyboardEvent) => {
    if (e.key === "Escape" && !disabling) {
      onCloseRef.current();
    }
  };
  if (open) document.addEventListener("keydown", handleEscape);
  return () => document.removeEventListener("keydown", handleEscape);
}, [open, disabling]);

[github-actions]

[Accessibility - Low] This dialog is missing role="dialog", aria-modal="true", and aria-labelledby attributes on the dialog container. MfaRecoveryTimeoutModal (line 59-63) correctly includes all three. For consistency and screen reader support, add the ARIA attributes to the dialog panel:

<div
  className="relative bg-surface ..."
  role="dialog"
  aria-modal="true"
  aria-labelledby="disable-mfa-title"
>

And add id="disable-mfa-title" to the heading at line 100.

[github-actions]

[Code Quality - Medium] The store actions requestMfaReset and completeMfaReset (line 243) shadow the identically-named API imports from ../../api/mfa (line 13). The codebase already handles this pattern elsewhere — updatePassword is aliased as apiUpdatePassword on line 7.

If someone refactors the store action to call the import directly (e.g., requestMfaReset(identifier)), it would cause infinite recursion instead of a compile error, because the local name resolves to the store action itself.

Apply the same aliasing pattern:

import {
  requestMfaReset as apiRequestMfaReset,
  completeMfaReset as apiCompleteMfaReset,
  // ...
} from "../api/mfa";

[github-actions]

[Code Quality - Low] URL.revokeObjectURL(url) is called synchronously after a.click(), but click() triggers an asynchronous download. If an exception interrupts between appendChild and revokeObjectURL, the Blob URL containing plaintext recovery codes leaks in memory. Wrap cleanup in a try/finally:

Suggested change

	const url = URL.createObjectURL(blob);
	const a = document.createElement("a");
	a.href = url;
	a.download = "shellhub-recovery-codes.txt";
	document.body.appendChild(a);
	a.click();
	document.body.removeChild(a);
	URL.revokeObjectURL(url);
	const url = URL.createObjectURL(blob);
	const a = document.createElement("a");
	a.href = url;
	a.download = "shellhub-recovery-codes.txt";
	document.body.appendChild(a);
	try {
	a.click();
	} finally {
	document.body.removeChild(a);
	URL.revokeObjectURL(url);
	}

Also: this module exports a useRecoveryCodeActions "hook" that uses zero React hooks internally. Consider renaming it to a plain utility (e.g., recoveryCodeActions.ts) to follow the convention that use* prefixes are reserved for actual hooks.