Add TOTP 2FA with security hardening (encryption, recovery codes, HttpOnly cookies) - #147
Open
vmfasimoes wants to merge 2 commits into
Open
vmfasimoes wants to merge 2 commits into
vmfasimoes wants to merge 2 commits into
Conversation
- Login flow now issues a short-lived temp token (5 min) when 2FA is enabled; full JWT is only issued after successful TOTP verification - Admin panel: per-user 2FA management (setup, view QR, force-disable) - Users table shows 2FA status badge (On/Off) per row - Edit user modal includes 2FA section with QR code setup and viewer - Sensitive TOTP fields (totpSecret, totpPendingSecret) stripped from GET /api/auth/users responses
…imiting
Security hardening applied to the existing TOTP 2FA feature:
1. Fix unsafe JWT fallback in production
- JWT_SECRET now throws at startup if missing in production
- Development uses a per-restart random secret with a console warning
2. Remove TOTP secret retrieval endpoint
- Removed GET /users/:id/2fa/qr that returned the active secret
- Replaced with POST /users/:id/2fa/reset, which immediately invalidates
the current secret and returns only a new QR code (secret never exposed)
3. Rate-limit TOTP verification
- Added express-rate-limit (5 attempts / 15 min, skipSuccessfulRequests)
on POST /2fa/verify to prevent brute-force attacks
- Fixed trust proxy from true to 1 so IP-based limiting is not bypassable
4. Encrypt TOTP secrets at rest (AES-256-GCM)
- New encryptSecret / decryptSecret helpers in server/auth.js
- All writes to totpSecret / totpPendingSecret go through encryptSecret()
- All reads before TOTP verification go through decryptSecret()
- Stored format: enc:<iv>:<authTag>:<ciphertext> (hex-encoded)
- TOTP_ENCRYPTION_KEY env var required in production; dev falls back to
plaintext with a warning (graceful migration via enc: prefix detection)
5. Single-use recovery codes
- generateRecoveryCodes() produces 8 codes (XXXXX-XXXXX format, bcrypt-hashed)
- Returned to the user in plaintext only once, at 2FA enrollment
- matchRecoveryCode() verifies and the used slot is nulled (single-use)
- /2fa/verify accepts either a TOTP code or a recovery code in the same field
- Disable / reset flows clear stored codes; GET /users strips recoveryCodes
- Login page adds "Use a recovery code" toggle to switch input mode
6. Migrate auth tokens from localStorage to HttpOnly cookies
- cookie-parser added; JWT strategy reads req.cookies.token first, then
Authorization header (backward-compatible)
- Login, setup, /2fa/verify, and OIDC callback set an HttpOnly cookie;
logout calls res.clearCookie()
- OIDC callback no longer appends ?token= to the redirect URL
- Frontend api.js uses credentials: same-origin; removed all
localStorage.getItem/setItem('authToken') and Authorization: Bearer headers
- auth.js init() now always calls /me (cookie sent automatically)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR introduces TOTP-based two-factor authentication (RFC 6238) with full security hardening. It is split across two commits: the initial 2FA implementation and a subsequent security hardening pass addressing all issues raised in the first review.
What was implemented
Initial implementation
speakeasy— RFC 6238 compliant, replaces the incompatibleotplibv13totpSecret,totpPendingSecret) stripped fromGET /api/auth/usersresponsesSecurity hardening (6 points)
1. Safe JWT secret handling
JWT_SECRETnow throws at startup if missing inNODE_ENV=productioncrypto.randomBytes(32)secret with a console warning2. TOTP secret retrieval endpoint removed
GET /users/:id/2fa/qr(which returned the active TOTP secret) was removed entirelyPOST /users/:id/2fa/reset, which immediately invalidates the current secret and returns only a new QR code — the active secret is never returned over the API3. Rate limiting on TOTP verification
express-rate-limitapplied toPOST /2fa/verify: 5 failed attempts per 15 minutes per IP (skipSuccessfulRequests: true)app.set('trust proxy', true)changed totrust proxy: 1— prevents trivial IP spoofing viaX-Forwarded-Forthat would bypass rate limiting4. TOTP secrets encrypted at rest (AES-256-GCM)
encryptSecret()/decryptSecret()helpers inserver/auth.jsenc:<iv_hex>:<authTag_hex>:<ciphertext_hex>(authenticated encryption)totpSecret/totpPendingSecretpass throughencryptSecret()decryptSecret()TOTP_ENCRYPTION_KEYenv var (32-byte hex) required in production; dev stores plaintext with a warningenc:prefix are accepted as-is (dev / legacy)5. Single-use recovery codes
generateRecoveryCodes()produces 8 codes inXXXXX-XXXXXformat (hex, bcrypt-hashed, stored server-side)matchRecoveryCode()iterates hashed codes and returns the matched index; used slots are set tonull(single-use enforced)POST /2fa/verifyaccepts either a 6-digit TOTP code or a recovery code in the same fieldrecoveryCodes;GET /usersstrips the field from responsesXXXXX-XXXXX)6. Auth tokens migrated from localStorage to HttpOnly cookies
cookie-parsermiddleware added toserver/index.jsreq.cookies.tokenfirst, thenAuthorization: Bearer(backward-compatible with API clients)POST /login), initial setup (POST /setup),POST /2fa/verify, and the OIDC callback all set anHttpOnly; SameSite=Laxcookie; logout callsres.clearCookie()?token=to the redirect URLapi.jsusescredentials: 'same-origin'; alllocalStorage.getItem/setItem('authToken')andAuthorization: Bearerheader logic removedauth.jsinit()now always callsGET /api/auth/me(cookie is sent automatically by the browser); no more token presence check before the requestFiles changed
server/auth.jsserver/routes/auth.jsserver/index.jscookie-parsermiddleware,trust proxy: 1public/js/api.jsAuthorizationheader and localStorage token management;credentials: same-originpublic/js/app.jscheckAuthand logout rewritten to use cookie (no localStorage)public/js/auth.jsinit()always calls/me; removedAPI.setTokencallspublic/js/pages/Settings.jspublic/login.htmlpublic/index.html?token=SSO handlerTest plan
/; no token visible in DevTools → Application → Local Storagerequires2fa: true; step 2 with correct TOTP code sets cookie and redirectsXXXXX-XXXXXcode; login succeeds; same code cannot be used a second timetotpSecretvalue should start withenc:(not a plain base32 string)/; no?token=in URLPOST /api/auth/logout; cookie is cleared; subsequent requests to/api/auth/mereturn 401