From c8dcfcc93b0fa62a9fc046e43457596960393b14 Mon Sep 17 00:00:00 2001 From: saubyk <39208279+saubyk@users.noreply.github.com> Date: Mon, 3 Aug 2026 11:05:04 -0700 Subject: [PATCH] Address review findings on auth session handling and test wiring --- CLAUDE.md | 6 +- backend/controllers/shared/authenticate.js | 19 +++++- backend/routes/shared/authenticate.js | 5 +- package.json | 4 +- server/controllers/shared/authenticate.ts | 19 +++++- server/routes/shared/authenticate.ts | 5 +- test/backend/authenticate.test.mjs | 72 +++++++++++++++++----- 7 files changed, 107 insertions(+), 23 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 2eb0796b..2b869343 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -92,9 +92,9 @@ Eclair, wired to RTL — for end-to-end testing across all three implementations every credential in it is throwaway. Backend regression tests live in `test/backend/` (plain `node:test`, run against the -compiled `backend/` — run `npm run buildbackend` first or they test stale code). -`npm run test` chains them (`npm run testbackend`) before the frontend Karma/Jasmine specs. -For backend changes, also verify against the fixture and say so in the PR. +compiled `backend/`). `npm run test` compiles the backend, then runs them +(`npm run testbackend`) before the frontend Karma/Jasmine specs, so they never test stale +code. For backend changes, also verify against the fixture and say so in the PR. ## Conventions diff --git a/backend/controllers/shared/authenticate.js b/backend/controllers/shared/authenticate.js index aff8a741..13b64988 100644 --- a/backend/controllers/shared/authenticate.js +++ b/backend/controllers/shared/authenticate.js @@ -48,6 +48,21 @@ const handleMultipleFailedAttemptsError = (failed, currentTime, errMsg) => { } }; export const verifyToken = (twoFAToken) => !!(common.appConfig.secret2FA && common.appConfig.secret2FA !== '' && otplib.authenticator.check(twoFAToken, common.appConfig.secret2FA)); +// Mirrors isAuthenticated: a request carrying a valid session JWT has already +// completed 2FA at login, since tokens are only minted after verification when +// 2FA is enabled. Used to exempt in-app re-authorization (e.g. the password +// prompt before on-chain sends) from the TOTP requirement without opening a +// password-only path. +const hasValidAuthToken = (req) => { + try { + const token = req.headers.authorization.split(' ')[1]; + jwt.verify(token, common.secret_key); + return true; + } + catch (error) { + return false; + } +}; export const authenticateUser = (req, res, next) => { const { authenticateWith, authenticationValue, twoFAToken } = req.body; logger.log({ selectedNode: req.session.selectedNode, level: 'INFO', fileName: 'Authenticate', msg: 'Authenticating User..' }); @@ -92,7 +107,9 @@ export const authenticateUser = (req, res, next) => { // instead of silently skipping verification. The login UI keys its token prompt // on enable2FA, so both fields are consulted — a stale secret with 2FA disabled // must not lock the operator out of a UI that never prompts for a token. - if (common.appConfig.enable2FA && common.appConfig.secret2FA && common.appConfig.secret2FA !== '') { + // Requests with a valid session token (in-app re-authorization, e.g. the + // password prompt before on-chain sends) are exempt from the TOTP requirement. + if (common.appConfig.enable2FA && common.appConfig.secret2FA && common.appConfig.secret2FA !== '' && !hasValidAuthToken(req)) { if (typeof twoFAToken !== 'string' || twoFAToken === '' || !verifyToken(twoFAToken)) { logger.log({ selectedNode: req.session.selectedNode, level: 'ERROR', fileName: 'Authenticate', msg: 'Invalid Token! Failed IP ' + reqIP, error: { error: 'Invalid token.' } }); failed.count = failed.count + 1; diff --git a/backend/routes/shared/authenticate.js b/backend/routes/shared/authenticate.js index 0cbdbbe4..7e1559a2 100644 --- a/backend/routes/shared/authenticate.js +++ b/backend/routes/shared/authenticate.js @@ -1,9 +1,12 @@ import exprs from 'express'; const { Router } = exprs; import { authenticateUser, verifyToken, resetPassword, logoutUser } from '../../controllers/shared/authenticate.js'; +import { isAuthenticated } from '../../utils/authCheck.js'; const router = Router(); router.post('/', authenticateUser); router.post('/token', verifyToken); -router.post('/reset', resetPassword); +// Password changes mint a fresh session token, so the route requires an existing +// authenticated session; the frontend interceptor attaches it for the settings UI. +router.post('/reset', isAuthenticated, resetPassword); router.get('/logout', logoutUser); export default router; diff --git a/package.json b/package.json index ac4d2fd6..3c154d25 100644 --- a/package.json +++ b/package.json @@ -16,8 +16,8 @@ "server": "set NODE_ENV=development&&nodemon --watch backend --watch server ./rtl.js", "serverUbuntu": "NODE_ENV=development nodemon --watch backend --watch server ./rtl.js", "testdev": "ng test --watch=true --code-coverage", - "testbackend": "node --test \"test/backend/*.test.mjs\"", - "test": "npm run testbackend && ng test --watch=false --browsers=ChromeHeadless", + "testbackend": "node --test test/backend/*.test.mjs", + "test": "npm run buildbackend && npm run testbackend && ng test --watch=false --browsers=ChromeHeadless", "lint": "eslint" }, "private": true, diff --git a/server/controllers/shared/authenticate.ts b/server/controllers/shared/authenticate.ts index 945174df..e8917e67 100644 --- a/server/controllers/shared/authenticate.ts +++ b/server/controllers/shared/authenticate.ts @@ -52,6 +52,21 @@ const handleMultipleFailedAttemptsError = (failed, currentTime, errMsg) => { export const verifyToken = (twoFAToken) => !!(common.appConfig.secret2FA && common.appConfig.secret2FA !== '' && (otplib as any).authenticator.check(twoFAToken, common.appConfig.secret2FA)); +// Mirrors isAuthenticated: a request carrying a valid session JWT has already +// completed 2FA at login, since tokens are only minted after verification when +// 2FA is enabled. Used to exempt in-app re-authorization (e.g. the password +// prompt before on-chain sends) from the TOTP requirement without opening a +// password-only path. +const hasValidAuthToken = (req) => { + try { + const token = req.headers.authorization.split(' ')[1]; + jwt.verify(token, common.secret_key); + return true; + } catch (error) { + return false; + } +}; + export const authenticateUser = (req, res, next) => { const { authenticateWith, authenticationValue, twoFAToken } = req.body; logger.log({ selectedNode: req.session.selectedNode, level: 'INFO', fileName: 'Authenticate', msg: 'Authenticating User..' }); @@ -88,7 +103,9 @@ export const authenticateUser = (req, res, next) => { // instead of silently skipping verification. The login UI keys its token prompt // on enable2FA, so both fields are consulted — a stale secret with 2FA disabled // must not lock the operator out of a UI that never prompts for a token. - if (common.appConfig.enable2FA && common.appConfig.secret2FA && common.appConfig.secret2FA !== '') { + // Requests with a valid session token (in-app re-authorization, e.g. the + // password prompt before on-chain sends) are exempt from the TOTP requirement. + if (common.appConfig.enable2FA && common.appConfig.secret2FA && common.appConfig.secret2FA !== '' && !hasValidAuthToken(req)) { if (typeof twoFAToken !== 'string' || twoFAToken === '' || !verifyToken(twoFAToken)) { logger.log({ selectedNode: req.session.selectedNode, level: 'ERROR', fileName: 'Authenticate', msg: 'Invalid Token! Failed IP ' + reqIP, error: { error: 'Invalid token.' } }); failed.count = failed.count + 1; diff --git a/server/routes/shared/authenticate.ts b/server/routes/shared/authenticate.ts index e4785b1f..8359eefa 100644 --- a/server/routes/shared/authenticate.ts +++ b/server/routes/shared/authenticate.ts @@ -1,12 +1,15 @@ import exprs from 'express'; const { Router } = exprs; import { authenticateUser, verifyToken, resetPassword, logoutUser } from '../../controllers/shared/authenticate.js'; +import { isAuthenticated } from '../../utils/authCheck.js'; const router = Router(); router.post('/', authenticateUser); router.post('/token', verifyToken); -router.post('/reset', resetPassword); +// Password changes mint a fresh session token, so the route requires an existing +// authenticated session; the frontend interceptor attaches it for the settings UI. +router.post('/reset', isAuthenticated, resetPassword); router.get('/logout', logoutUser); export default router; diff --git a/test/backend/authenticate.test.mjs b/test/backend/authenticate.test.mjs index fb97e975..14b8d797 100644 --- a/test/backend/authenticate.test.mjs +++ b/test/backend/authenticate.test.mjs @@ -1,6 +1,7 @@ import assert from 'node:assert/strict'; import test from 'node:test'; +import jwt from 'jsonwebtoken'; import * as otplib from 'otplib'; import { authenticateUser } from '../../backend/controllers/shared/authenticate.js'; import { Common } from '../../backend/utils/common.js'; @@ -27,15 +28,19 @@ const setupAppConfig = (enable2FA, secret2FA) => { Common.nodes = []; }; -// Distinct IPs per call: failedLoginAttempts is module-level state in authenticate.js, -// keyed by request IP, so unique values keep the tests isolated from each other. +// failedLoginAttempts is module-level state in authenticate.js, keyed by the request IP +// from common.getRequestIP, which prefers x-forwarded-for (server/utils/common.ts). +// Unique IPs give each call a fresh counter; tests exercising the counter itself pass an +// explicit ip to share one key across calls. let ipCounter = 0; -const mockRequest = (twoFAToken) => { - ipCounter = ipCounter + 1; +const nextIP = () => '10.0.0.' + (ipCounter = ipCounter + 1); +const mockRequest = ({ twoFAToken, ip, authToken, password } = {}) => { + const headers = { 'x-forwarded-for': ip || nextIP() }; + if (authToken) { headers.authorization = 'Bearer ' + authToken; } return { - body: { authenticateWith: 'PASSWORD', authenticationValue: PASSWORD_HASH, twoFAToken: twoFAToken }, + body: { authenticateWith: 'PASSWORD', authenticationValue: password || PASSWORD_HASH, twoFAToken: twoFAToken }, session: {}, - headers: { 'x-forwarded-for': '10.0.0.' + ipCounter }, + headers: headers, connection: {}, socket: {} }; @@ -50,11 +55,13 @@ const mockResponse = () => { return res; }; +const mockSessionToken = () => jwt.sign({ user: 'NODE_USER' }, Common.secret_key); + test('rejects login without a 2FA token when 2FA is enabled', () => { setupAppConfig(true, TOTP_SECRET); for (const missingToken of [undefined, '']) { const res = mockResponse(); - authenticateUser(mockRequest(missingToken), res, null); + authenticateUser(mockRequest({ twoFAToken: missingToken }), res, null); assert.equal(res.statusCode, 401); assert.match(res.body.error, /2FA/); } @@ -63,7 +70,7 @@ test('rejects login without a 2FA token when 2FA is enabled', () => { test('rejects login with an invalid 2FA token when 2FA is enabled', () => { setupAppConfig(true, TOTP_SECRET); const res = mockResponse(); - authenticateUser(mockRequest('000000'), res, null); + authenticateUser(mockRequest({ twoFAToken: '000000' }), res, null); assert.equal(res.statusCode, 401); assert.match(res.body.error, /2FA/); }); @@ -74,7 +81,7 @@ test('rejects a non-string 2FA token when 2FA is enabled', () => { // (digit regex, then strict === against the string token), but the typeof guard keeps the // rejection explicit and independent of otplib internals. const res = mockResponse(); - authenticateUser(mockRequest(['1', '2', '3', '4', '5', '6']), res, null); + authenticateUser(mockRequest({ twoFAToken: ['1', '2', '3', '4', '5', '6'] }), res, null); assert.equal(res.statusCode, 401); assert.match(res.body.error, /2FA/); }); @@ -82,15 +89,52 @@ test('rejects a non-string 2FA token when 2FA is enabled', () => { test('accepts login with a valid 2FA token when 2FA is enabled', () => { setupAppConfig(true, TOTP_SECRET); const res = mockResponse(); - authenticateUser(mockRequest(authenticator.generate(TOTP_SECRET)), res, null); + authenticateUser(mockRequest({ twoFAToken: authenticator.generate(TOTP_SECRET) }), res, null); assert.equal(res.statusCode, 200); assert.equal(typeof res.body.token, 'string'); }); +test('accepts password-only re-authorization from an authenticated session when 2FA is enabled', () => { + // In-app re-authorization (e.g. the password prompt before on-chain sends) carries the + // session JWT via the auth interceptor; that session was itself minted after 2FA. + setupAppConfig(true, TOTP_SECRET); + const res = mockResponse(); + authenticateUser(mockRequest({ twoFAToken: undefined, authToken: mockSessionToken() }), res, null); + assert.equal(res.statusCode, 200); + assert.equal(typeof res.body.token, 'string'); +}); + +test('rejects a wrong password even with an authenticated session when 2FA is enabled', () => { + setupAppConfig(true, TOTP_SECRET); + const res = mockResponse(); + authenticateUser(mockRequest({ twoFAToken: undefined, authToken: mockSessionToken(), password: 'wrong-hash' }), res, null); + assert.equal(res.statusCode, 401); + assert.match(res.body.error, /Invalid Password/); +}); + +test('locks out after five failed 2FA attempts, even for a then-valid token', () => { + setupAppConfig(true, TOTP_SECRET); + const ip = nextIP(); // one shared counter key for every attempt in this test + for (let i = 0; i < 4; i++) { + const res = mockResponse(); + authenticateUser(mockRequest({ twoFAToken: '000000', ip: ip }), res, null); + assert.equal(res.statusCode, 401); + assert.match(res.body.error, /2FA/); + } + const fifth = mockResponse(); + authenticateUser(mockRequest({ twoFAToken: '000000', ip: ip }), fifth, null); + assert.equal(fifth.statusCode, 401); + assert.match(fifth.body.error, /locked/); + const sixth = mockResponse(); + authenticateUser(mockRequest({ twoFAToken: authenticator.generate(TOTP_SECRET), ip: ip }), sixth, null); + assert.equal(sixth.statusCode, 401); + assert.match(sixth.body.error, /locked/); +}); + test('accepts password-only login when 2FA is not configured', () => { setupAppConfig(false, ''); const res = mockResponse(); - authenticateUser(mockRequest(undefined), res, null); + authenticateUser(mockRequest({ twoFAToken: undefined }), res, null); assert.equal(res.statusCode, 200); assert.equal(typeof res.body.token, 'string'); }); @@ -101,7 +145,7 @@ test('accepts a stale token in the request when 2FA is not configured', () => { // with no 2FA configured the token is now ignored entirely. setupAppConfig(false, ''); const res = mockResponse(); - authenticateUser(mockRequest('123456'), res, null); + authenticateUser(mockRequest({ twoFAToken: '123456' }), res, null); assert.equal(res.statusCode, 200); assert.equal(typeof res.body.token, 'string'); }); @@ -111,7 +155,7 @@ test('does not require a token when 2FA is disabled but a stale secret remains', // secret would lock the operator out of a UI that never asks for one. setupAppConfig(false, TOTP_SECRET); const res = mockResponse(); - authenticateUser(mockRequest(undefined), res, null); + authenticateUser(mockRequest({ twoFAToken: undefined }), res, null); assert.equal(res.statusCode, 200); assert.equal(typeof res.body.token, 'string'); }); @@ -121,7 +165,7 @@ test('does not enforce a token when 2FA is enabled without a secret', () => { // verify against an empty secret, so enforcing would lock everyone out. setupAppConfig(true, ''); const res = mockResponse(); - authenticateUser(mockRequest(undefined), res, null); + authenticateUser(mockRequest({ twoFAToken: undefined }), res, null); assert.equal(res.statusCode, 200); assert.equal(typeof res.body.token, 'string'); });