Add verify-login compatibility route for 2FA #4331
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| import { describe, test } from 'node:test'; | ||
| import assert from 'node:assert/strict'; | ||
| import { ApiError } from '../../middleware/errorHandler.js'; | ||
| import router, { verifyLoginChallenge } from '../twoFactor.js'; | ||
|
|
||
| describe('twoFactor routes', () => { | ||
| test('registers the verify-login compatibility route', () => { | ||
| const verifyLoginRoute = router.stack.find( | ||
| (layer) => layer.route?.path === '/verify-login' && layer.route.methods?.post | ||
| ); | ||
|
|
||
| assert.ok(verifyLoginRoute, 'expected POST /verify-login route to be registered'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('verifyLoginChallenge', () => { | ||
| test('uses TOTP verification by default', async () => { | ||
| const calls = []; | ||
| const req = { | ||
| user: { uid: 'user-123' }, | ||
| body: { token: '123456' }, | ||
| }; | ||
| const res = { | ||
| payload: null, | ||
| json(body) { | ||
| this.payload = body; | ||
| }, | ||
| }; | ||
|
|
||
| await verifyLoginChallenge(req, res, { | ||
| async verifyTotp(uid, token) { | ||
| calls.push(['totp', uid, token]); | ||
| return true; | ||
| }, | ||
| async verifyBackupCode() { | ||
| calls.push(['backup']); | ||
| return true; | ||
| }, | ||
| }); | ||
|
|
||
| assert.deepEqual(calls, [['totp', 'user-123', '123456']]); | ||
| assert.deepEqual(res.payload, { success: true }); | ||
| }); | ||
|
|
||
| test('uses backup-code verification when requested', async () => { | ||
| const calls = []; | ||
| const req = { | ||
| user: { uid: 'user-456' }, | ||
| body: { token: 'ABCD-1234', useBackup: true }, | ||
| }; | ||
| const res = { | ||
| payload: null, | ||
| json(body) { | ||
| this.payload = body; | ||
| }, | ||
| }; | ||
|
|
||
| await verifyLoginChallenge(req, res, { | ||
| async verifyTotp() { | ||
| calls.push(['totp']); | ||
| return true; | ||
| }, | ||
| async verifyBackupCode(uid, code) { | ||
| calls.push(['backup', uid, code]); | ||
| return true; | ||
| }, | ||
| }); | ||
|
|
||
| assert.deepEqual(calls, [['backup', 'user-456', 'ABCD-1234']]); | ||
| assert.deepEqual(res.payload, { success: true }); | ||
| }); | ||
|
|
||
| test('throws the same backup-code error message on invalid backup code', async () => { | ||
| const req = { | ||
| user: { uid: 'user-789' }, | ||
| body: { token: 'BAD-CODE', useBackup: true }, | ||
| }; | ||
| const res = { | ||
| json() { | ||
| assert.fail('res.json should not be called on invalid backup code'); | ||
| }, | ||
| }; | ||
|
|
||
| await assert.rejects( | ||
| verifyLoginChallenge(req, res, { | ||
| async verifyTotp() { | ||
| return true; | ||
| }, | ||
| async verifyBackupCode() { | ||
| return false; | ||
| }, | ||
| }), | ||
| (error) => { | ||
| assert.ok(error instanceof ApiError); | ||
| assert.equal(error.statusCode, 401); | ||
| assert.equal(error.message, 'Invalid backup code'); | ||
| return true; | ||
| } | ||
| ); | ||
| }); | ||
| }); |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,10 +8,24 @@ import { | |
| enable2FASchema, | ||
| tokenOnlySchema, | ||
| backupCodeSchema, | ||
| verifyLoginSchema, | ||
| } from '../schemas/twoFactor.schema.js'; | ||
|
|
||
| const router = express.Router(); | ||
|
|
||
| export const verifyLoginChallenge = async (req, res, deps = twoFactor) => { | ||
| const { token, useBackup = false } = req.body; | ||
| const ok = useBackup | ||
| ? await deps.verifyBackupCode(req.user.uid, token) | ||
| : await deps.verifyTotp(req.user.uid, token); | ||
|
|
||
| if (!ok) { | ||
| throw new ApiError(401, useBackup ? 'Invalid backup code' : 'Invalid or expired code'); | ||
| } | ||
|
|
||
| res.json({ success: true }); | ||
| }; | ||
|
|
||
| // Strict rate limit for code-verification endpoints to prevent brute force | ||
| const verifyLimiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, | ||
|
|
@@ -88,6 +102,12 @@ router.post('/verify-backup', verifyToken, verifyLimiter, validate(backupCodeSch | |
| res.json({ success: true }); | ||
| })); | ||
|
|
||
| // POST /api/auth/2fa/verify-login (rate-limited) | ||
| // Backwards-compatible login endpoint used by older frontend code paths. | ||
| router.post('/verify-login', verifyToken, verifyLimiter, validate(verifyLoginSchema), asyncHandler(async (req, res) => { | ||
| await verifyLoginChallenge(req, res); | ||
| })); | ||
|
|
||
| // POST /api/auth/2fa/backup-codes/regenerate | ||
| // Generates a fresh set of backup codes after verifying the current TOTP. | ||
| router.post('/backup-codes/regenerate', verifyToken, validate(tokenOnlySchema), asyncHandler(async (req, res) => { | ||
|
|
||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Snyk job never reports
skipped; summary will show "Passed" when the token is unset.The
snykjob has no job-levelif, so whenSNYK_TOKENis empty the real steps skip individually but this echo step still runs and the job completes with resultsuccess. Consequentlyneeds.snyk.resultis never'skipped', making the'Skipped'branches in the summary table (Line 179) and PR comment (Lines 197-198) unreachable — the report will read "Passed" even though Snyk never ran. Gate the whole job instead so it is actually skipped.🔧 Skip at the job level so the result is `skipped`
snyk: name: Snyk Vulnerability Scan runs-on: ubuntu-latest + if: ${{ vars.SNYK_ENABLED == 'true' }} # or another job-level gate; secrets can't be read in job-level if env: SNYK_TOKEN: ${{ secrets.SNYK_TOKEN }}Note: secrets cannot be referenced directly in a job-level
if, so route the toggle through a repo/org variable (or a preceding job output) if you want a trueskippedresult.🤖 Prompt for AI Agents