Conversation
Adds optional TOTP setup, login challenge enforcement, encrypted secret storage, and single-use recovery codes so account sessions can require a second factor after password verification. Made-with: Cursor
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9539836a8e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| CREATE INDEX idx_sessions_user ON sessions(user_id); | ||
| CREATE INDEX idx_sessions_expires ON sessions(expires_at); | ||
|
|
||
| CREATE TABLE user_totp ( |
There was a problem hiding this comment.
These schema changes were added by editing db/migrations/001_init.sql, but the migrator only runs each migration filename once (see migrate in lib/db.js, which skips files already present in schema_migrations). That means existing deployments that already applied 001_init.sql will never create user_totp/mfa_challenges, while this commit now initializes TOTP repositories during app startup, leading to no such table failures on upgrade. The new tables need to be introduced via a new numbered migration file so previously deployed databases are upgraded safely.
Useful? React with 👍 / 👎.
Moves MFA table creation into a new numbered migration so existing databases receive the schema during upgrade instead of relying on an already-applied init migration. Made-with: Cursor
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: adea6c75c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }); | ||
| })); | ||
|
|
||
| router.post('/login/totp', limiters.login, asyncHandler(async (req, res) => { |
There was a problem hiding this comment.
Do not reuse limiters.login on this route without a matching key shape: middleware/rateLimit.js builds login keys from req.body.email, but /auth/login/totp requests do not carry an email, so all second-factor attempts from the same IP collapse into the same login|<ip>| bucket. In shared-IP environments (office NAT, mobile carrier CGNAT), five bad MFA attempts by one user can 429 every other user’s TOTP step for 15 minutes, and rate limiting is no longer scoped to the account/challenge being verified.
Useful? React with 👍 / 👎.
Adds a dedicated MFA login limiter keyed by hashed challenge token plus IP so second-factor attempts do not collapse all shared-IP users into the password-login bucket. Made-with: Cursor
|
@codex review |
Moves the MFA tables back into the pre-production v1 schema so the branch stays aligned with the repository rule that 001_init.sql remains the single source before launch. Made-with: Cursor
|
@codex review Note: this app is not in production yet. Per the schema header in db/migrations/001_init.sql, pre-launch schema additions intentionally belong in 001_init.sql rather than a forward migration. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0f7498c10
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!totp) return res.status(503).json({ error: 'server_misconfigured' }); | ||
| const parsed = TotpCodeSchema.safeParse(req.body); | ||
| if (!parsed.success) return badRequest(res, 'invalid_body'); | ||
| try { | ||
| const out = totp.enableSetup(req.user.id, parsed.data.code); |
There was a problem hiding this comment.
This handler enables MFA using only the current session and CSRF token, and immediately trusts totp.enableSetup(...) without re-verifying the user's password or existing MFA factor. In practice, if an attacker obtains a live session (e.g., stolen cookie on a shared machine), they can run /auth/totp/setup + /auth/totp/enable to enroll their own secret and lock the legitimate user out on the next login. Please require step-up authentication (current password or current TOTP/recovery proof) before allowing MFA enrollment/rotation.
Useful? React with 👍 / 👎.
Blocks session-only MFA enrollment by requiring the current password-derived auth hash before issuing or enabling a TOTP setup secret. Made-with: Cursor
|
@codex review Note: app is still pre-production, so TOTP schema remains intentionally consolidated into db/migrations/001_init.sql instead of adding a forward migration. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1069e87178
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function createChallenge(userId) { | ||
| deleteExpiredChallenges.run(now()); | ||
| const token = crypto.randomBytes(32).toString('hex'); | ||
| insertChallenge.run(hashToken(token), userId, now() + CHALLENGE_TTL_MS, now()); |
There was a problem hiding this comment.
createChallenge always inserts a fresh token for the user but never removes existing unexpired challenges, while verifyChallenge enforces MAX_CHALLENGE_ATTEMPTS per token. That lets an attacker who already knows the password bypass the intended OTP-attempt cap by repeatedly calling /auth/login to mint new challenge tokens and reset the counter (e.g., 25 OTP guesses per 15 minutes from one IP, more when distributed), which materially weakens MFA brute-force protection.
Useful? React with 👍 / 👎.
Ensures each password login replaces prior MFA challenge tokens for the user so OTP attempt limits cannot be reset by minting parallel challenges. Made-with: Cursor
|
@codex review Note: app is still pre-production, so TOTP schema remains intentionally consolidated into db/migrations/001_init.sql instead of adding a forward migration. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdb4304549
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const tokenHash = /^[0-9a-fA-F]{64}$/.test(raw) | ||
| ? crypto.createHash('sha256').update(raw.toLowerCase(), 'utf8').digest('hex') | ||
| : ''; | ||
| return `login-totp|${ipBucket(req)}|${tokenHash}`; |
There was a problem hiding this comment.
The MFA limiter key includes a hash of the raw challengeToken, so an unauthenticated caller can send a different random 64-hex token on each /auth/login/totp request and never hit the max: 5 threshold. In practice this defeats the intended throttle for this endpoint and creates a high-cardinality in-memory limiter keyspace (one bucket per random token for the full window), which is an avoidable DoS surface.
Useful? React with 👍 / 👎.
| ok = consumeRecoveryCode(row, recoveryCode); | ||
| recoveryCodeUsed = ok; |
There was a problem hiding this comment.
Recovery codes are consumed inside verifyChallenge before the login route issues the session cookie; if sessions.issue fails afterward (e.g., transient DB failure), the request returns 500 but the one-time recovery code is already burned. This can strand users during outage windows because repeated retries can exhaust recovery codes without a successful login, so the mutation should be tied to successful session creation.
Useful? React with 👍 / 👎.
Keys TOTP login throttling by IP for endpoint abuse while relying on per-challenge attempt caps, and burns recovery codes only after session creation succeeds. Made-with: Cursor
|
@codex review Note: app is still pre-production, so TOTP schema remains intentionally consolidated into db/migrations/001_init.sql instead of adding a forward migration. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57747111dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const { expiresAt } = issueLoginSession(req, res, user); | ||
| if (out.recoveryCode) { | ||
| totp.consumeUserRecoveryCode(out.userId, out.recoveryCode); | ||
| } |
There was a problem hiding this comment.
This path sets the session cookie and only then tries to burn the recovery code, and it does not enforce that burn succeeded. If consumeUserRecoveryCode throws (e.g., transient DB error) or loses a race in a multi-worker deployment, the login still authenticates while the same recovery code can remain valid for a later challenge, violating the single-use guarantee. Move recovery-code consumption into the same successful verification step (or otherwise make it atomic with login success) before finalizing the session.
Useful? React with 👍 / 👎.
Burns a verified recovery code after challenge/user validation but before issuing the authenticated session, so a failed consumption cannot leave a live session. Made-with: Cursor
|
@codex review Note: app is still pre-production, so TOTP schema remains intentionally consolidated into db/migrations/001_init.sql instead of adding a forward migration. |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
/auth/login/totpwhen TOTP is enabled, issuing session cookies only after the second factor succeeds.Test plan
npm test -- auth.routes.test.jsMade with Cursor