Repository navigation
feat(auth): add TOTP-based two-factor authentication - #58
Conversation
Adds optional 2FA on top of the existing JWT/password login, using
otplib (RFC 6238 TOTP) and qrcode for the setup QR image.
- New totpSecret/totpEnabled columns on User
- GenerateTotpSecretUseCase / ConfirmTotpSetupUseCase / DisableTotpUseCase /
GetTotpStatusUseCase back a setup-QR-confirm-disable flow in account
settings (beginTotpSetup, confirmTotpSetup, disableTotp, totpEnabled)
- login now returns { success, totpRequired } instead of a plain boolean —
when totpRequired is true, no auth cookies are set and the client must
call the new loginWithTotp(email, password, code) mutation to complete
sign-in (LoginWithTotpUseCase)
- New login.tsx two-step UI (credentials, then a 6-digit code) and a
Two-factor authentication section in account.tsx (QR code + manual key,
confirm, and disable-with-password)
- Verified end-to-end against a live server: register → beginTotpSetup →
confirmTotpSetup with a real generated code → login now requires TOTP →
loginWithTotp completes sign-in
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (52)
WalkthroughAdds TOTP-based two-factor authentication across the API and web application, including encrypted secrets, setup and disable flows, single-use backup codes, rate-limited verification, GraphQL integration, and client-side login/account UI. ChangesTOTP authentication
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant User
participant LoginPage
participant AuthResolver
participant LoginWithTotpUseCase
participant RateLimiter
participant UserRepository
User->>LoginPage: enter email and password
LoginPage->>AuthResolver: login credentials
AuthResolver->>UserRepository: load user
AuthResolver-->>LoginPage: totpRequired
User->>LoginPage: enter TOTP or backup code
LoginPage->>AuthResolver: loginWithTotp credentials and code
AuthResolver->>LoginWithTotpUseCase: verify code
LoginWithTotpUseCase->>RateLimiter: consume verification attempt
LoginWithTotpUseCase-->>AuthResolver: authenticated user
AuthResolver-->>LoginPage: tokens
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)apps/api/package.jsonTraceback (most recent call last): Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mankatcheung
left a comment
mankatcheung
left a comment
There was a problem hiding this comment.
Overview
This PR adds optional TOTP-based 2FA (RFC 6238) using otplib v13 + qrcode, with a full setup/confirm/disable flow on the account page and a two-step login flow on the client. It's well-structured and follows the repo's Clean Architecture conventions closely (domain → port → use case → resolver → Pothos schema → container wiring → codegen'd UI), and test coverage for the new use cases and resolvers is genuinely thorough, including tests that generate real TOTP codes rather than mocking the crypto. The breaking change to login's return type is called out clearly in the PR description.
However, this is a highly security-sensitive change, and there are two gaps that should block merge before this reaches production users: the TOTP secret is stored in plaintext, and there is no rate limiting anywhere in the API, including on the two new endpoints that accept a brute-forceable 6-digit code.
Security Analysis
-
TOTP secret stored in plaintext (CRITICAL).
User.totpSecretis a plainString?column (prisma/schema.prisma, migration20260722223632_add_totp), andGenerateTotpSecretUseCasewrites the raw base32 secret straight to it. Anyone with read access to the DB (backup leak, SQL injection elsewhere, insider, Turso credential leak) can mint valid codes for every enrolled user indefinitely — 2FA is completely defeated. This needs at-rest encryption (e.g., AES-GCM with a key from KMS/env, encrypt in the use case or a repository-level column encryption helper) before this ships, especially since your architecture already treats secrets seriously elsewhere (JWT secrets via env, R2 creds, etc.). -
No rate limiting / brute-force protection anywhere in the API (CRITICAL for this feature). I checked
apps/api/src/http/plugins/andapp.ts— there is no@fastify/rate-limitor any throttling registered at all, and this PR doesn't add any either. A TOTP code is 6 digits (10^6 space) valid for ~30–60s with the tolerance configured here; without per-account/per-IP throttling,loginWithTotpandconfirmTotpSetupare brute-forceable well within a code's validity window by an attacker who already has the password (or, forconfirmTotpSetup, has hijacked a session). This is the single most important gap to close for a 2FA feature — 2FA is specifically meant to raise the bar against exactly this kind of automated attack. At minimum, add a fixed-window or leaky-bucket limiter keyed onemail/userIdforloginWithTotpandconfirmTotpSetup, with backoff/lockout after a handful of failures. -
No backup/recovery codes. There's no mechanism issued at enrollment (single-use hashed recovery codes) for a user who loses their authenticator device. Today the only way out is
disableTotp, which only requires the current password — see next point. Recovery codes are pretty standard for a 2FA feature and worth adding, even as fixed follow-up work. -
disableTotponly requires password, not a TOTP code.DisableTotpUseCasechecksbcrypt.compareagainst the password and doesn't ask for a current TOTP code. Since disabling requires an authenticated session (which, if 2FA is already enabled, means the user already passed TOTP to get that session), the practical risk is lower than it looks, but it does mean a compromised-but-still-valid session plus a leaked password is enough to downgrade the account's security silently and immediately. Consider requiring the current TOTP code (or a recovery code) in addition to the password, matching the "step-up" pattern you'd want for a security-lowering action. -
beginTotpSetup/confirmTotpSetupenrollment doesn't require re-entering the password. Enrollment only needsctx.user(a valid session) — no password re-confirmation, unlikedisableTotpwhich does require it. This is an inconsistency: enabling 2FA (attacker enrolling their own secret on a hijacked session to... actually this doesn't help an attacker much since they'd need the victim's session either way) is lower risk than disabling, but for consistency and defense-in-depth it's worth requiring current password onbeginTotpSetuptoo, the same way you already require it for email/password changes. -
Clock-skew tolerance (
EPOCH_TOLERANCE_S: 30) is reasonable. This is a sensible ±1 step window, not excessively wide — no concern here. -
Library choice is solid.
otplib(v13) with the Noble crypto / Scure base32 plugins is a well-maintained, purpose-built TOTP implementation — good call over hand-rolling HMAC/base32 logic. -
Constant-time comparison: not verifiable from the diff since the comparison happens inside
otplib'sTOTP.verify(). Given otplib is a reputable library this is a lower-priority concern than the two CRITICAL items above, but worth a quick confirmation (or a note in the PR) that verification isn't a naive===on attacker-controlled input at the app layer — from what's in this diff, it isn't (the app layer never does its own string comparison of codes). -
2FA enrollment secret/QR isn't exposed to unauthenticated requests — confirmed good:
beginTotpSetupthrowsUNAUTHORIZEDwithoutctx.user. -
Login response change is safe:
loginno longer sets cookies whentotpRequiredis true, and no tokens are issued untilloginWithTotpsucceeds — correct flow, no partial-auth cookie ever leaks.
Code Quality / Style
- Clean adherence to existing layering:
ILoginWithTotpUseCase/LoginWithTotpUseCase,IGenerateTotpSecretUseCase/GenerateTotpSecretUseCase, etc. all mirror the existing use-case pattern, wired transiently incontainer.tsexactly like the others. infrastructure/auth/totp.tscentralizing theTOTPinstance factory (crypto/base32 plugins, issuer) is a nice small file that avoids repeating plugin wiring across four use cases.- Consistent
Object.assign(new Error(...), { code })error pattern matches the rest of the codebase. TOTP_CONFIGinconstants.tsavoids a magic number for the tolerance — good, matches the "no hardcoded values" convention.- Minor:
GenerateTotpSecretUseCase,ConfirmTotpSetupUseCase,DisableTotpUseCase,GetTotpStatusUseCaseeach re-fetch the user fromuserRepositoryindependently; not a real problem given TRANSIENT lifetime and single-purpose use cases, just flagging there's no shared "load-and-validate-user" helper (would be a nice-to-have simplification, not a blocker). - The account page component (
account.tsx) is getting large with the new TOTP section added inline. Given the "many small files" convention, extracting the TOTP setup/disable UI into its own component (e.g.TotpSection.tsx) would keep the file more focused — not a blocker for this PR size, but worth a follow-up.
Issues & Risks
- Plaintext
totpSecretat rest (blocking). - No rate limiting on
loginWithTotp/confirmTotpSetup(blocking). - No recovery codes — availability/UX gap, not strictly a vulnerability, but expected for a shipped 2FA feature.
disableTotpdoesn't require a current TOTP code/recovery code, only password.beginTotpSetupdoesn't require password re-confirmation (inconsistent withdisableTotp).- Flagged by the author already: merge conflicts likely with #56/#57 on
authMutations.ts/login.tsx.
Suggestions
- Encrypt
totpSecretat rest (symmetric encryption with a key from env/KMS, decrypt only inside the use case right before callingotplib). - Add
@fastify/rate-limit(or equivalent) at least onlogin,loginWithTotp, andconfirmTotpSetup, keyed by account/IP, with lockout/backoff. - Add hashed, single-use backup codes generated at
confirmTotpSetuptime and surfaced once to the user, plus auseRecoveryCodepath inloginWithTotp. - Require the current TOTP code (or a recovery code) for
disableTotp, or at least document why password-only is considered sufficient here. - Consider requiring password re-confirmation on
beginTotpSetupfor consistency withdisableTotp. - Follow-up: extract the account page's TOTP section into its own component as the file grows.
Test Coverage
Strong. New unit tests cover all four user-facing TOTP use cases and LoginWithTotpUseCase, including negative paths (missing user, already enabled, no setup in progress, wrong password, invalid code) and a happy path using a real generated TOTP code rather than a mocked one — this is exactly the kind of test that would catch a broken clock-skew or base32 encoding bug that mocks would hide. Resolver tests (AuthResolver, UserResolver) and web tests (LoginPage, AccountPage) were updated/added consistently. PrismaUserRepository and createTestDb schema were updated for the new columns. No tests exist (naturally, since the feature doesn't exist) for rate limiting or secret encryption — once those are added, they'll need their own coverage.
Verdict
Requesting changes. The TOTP mechanics, architecture, and test coverage are all solid, but plaintext secret storage and the total absence of rate limiting on code-verification endpoints are real security gaps for a feature whose entire purpose is to defend against credential compromise — both should be fixed before merge.
- Encrypt TOTP secrets at rest with AES-256-GCM (scrypt-derived key) instead of storing them in plaintext; require password re-authentication before starting 2FA setup, matching the existing disable-2FA re-auth pattern. - Rate-limit TOTP code verification during login by email and IP to blunt brute-forcing of the 6-digit code. - Issue 10 single-use backup codes on successful 2FA enrollment (shown once), accept them as a login fallback when the TOTP code is wrong, and delete all backup codes when 2FA is disabled.
Resolves conflicts with the just-merged JEF-16 (forgot/reset password) branch: both branches independently added a RateLimiter, RATE_LIMIT constants, and RATE_LIMITED error handling — merged into a single set of shared constants/infrastructure, keeping both use cases.
1 participant
Summary
otplib(v13, with its bundled Noble crypto / Scure base32 plugins) andqrcodefor the setup QR imagetotpSecret/totpEnabledcolumns onUserbeginTotpSetup,confirmTotpSetup,disableTotpmutations + atotpEnabledquery, backed byGenerateTotpSecretUseCase/ConfirmTotpSetupUseCase/DisableTotpUseCase/GetTotpStatusUseCaselogin: it now returns{ success, totpRequired }instead of a plainBoolean. WhentotpRequiredistrue, no auth cookies are set — the client must call the newloginWithTotp(email, password, code)mutation (LoginWithTotpUseCase) to complete sign-inlogin.tsxUI (credentials, then a 6-digit code) and a Two-factor authentication section inaccount.tsxFixes JEF-18
Test plan
pnpm typecheck(api + web)pnpm lint(api + web)pnpm test— 429 API tests + 81 web tests passing, including new coverage forGenerateTotpSecretUseCase,ConfirmTotpSetupUseCase,DisableTotpUseCase,GetTotpStatusUseCase,LoginWithTotpUseCase(using real generated TOTP codes, not mocked crypto),AuthResolver/UserResolver, and the updatedLoginPage/AccountPagepnpm build(all packages)beginTotpSetup→ generated a real TOTP code andconfirmTotpSetup→ confirmedloginnow returnstotpRequired: trueand sets no cookies →loginWithTotpwith a freshly generated code completed sign-in successfullyNote for reviewers
Since
login's return type changed fromBooleanto an object, any other in-flight branch that also touchesauthMutations.ts/login.tsxwill need a small merge conflict resolution — flagging in case it overlaps with other open PRs in this batch (#56, #57).🤖 Generated with Claude Code
https://claude.ai/code/session_01DdEiNRTcUnM6kE5AdFQ3n8
Summary by CodeRabbit