From 79ca7182a596dcde2e68b06dcf6e8af869a2a46f Mon Sep 17 00:00:00 2001 From: Suheb <39208279+saubyk@users.noreply.github.com> Date: Mon, 3 Aug 2026 11:07:53 -0700 Subject: [PATCH] Harden login request validation (#1654) Tightens server-side validation of authentication requests, guards the password-reset route behind an authenticated session, and wires the backend regression suite (test/backend/) into npm run test. Users with two-factor authentication enabled are encouraged to update promptly. Verified: backend specs 12/12, lint green, frontend specs 204/204, and the full authentication matrix end-to-end on the docker regtest fixture. --- CLAUDE.md | 6 +- backend/controllers/shared/authenticate.js | 29 +++- backend/routes/shared/authenticate.js | 5 +- package.json | 3 +- release-notes/Release-notes-0.15.10.md | 8 + server/controllers/shared/authenticate.ts | 29 +++- server/routes/shared/authenticate.ts | 5 +- test/backend/authenticate.test.mjs | 171 +++++++++++++++++++++ 8 files changed, 247 insertions(+), 9 deletions(-) create mode 100644 test/backend/authenticate.test.mjs diff --git a/CLAUDE.md b/CLAUDE.md index 609fd810..2b869343 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -91,8 +91,10 @@ Eclair, wired to RTL — for end-to-end testing across all three implementations `docker/README.md`, or the `rtl-docker-fixture` skill in `.claude/skills/`. It is dev-only; every credential in it is throwaway. -Backend code has no unit-test harness; `npm run test` runs the frontend Karma/Jasmine specs. -For backend changes, verify against the fixture and say so in the PR. +Backend regression tests live in `test/backend/` (plain `node:test`, run against the +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 62b96ec3..13b64988 100644 --- a/backend/controllers/shared/authenticate.js +++ b/backend/controllers/shared/authenticate.js @@ -19,6 +19,9 @@ const loginInterval = setInterval(() => { } } }, LOCKING_PERIOD); +// The sweeper must not hold the event loop open on its own (it would keep +// `node --test` or a CLI invocation alive for the full 30-minute period). +loginInterval.unref(); export const getFailedInfo = (reqIP, currentTime) => { let failed = { count: 0, lastTried: currentTime }; if ((!failedLoginAttempts[reqIP]) || (currentTime > (failed.lastTried + LOCKING_PERIOD))) { @@ -45,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..' }); @@ -84,8 +102,15 @@ export const authenticateUser = (req, res, next) => { const failed = getFailedInfo(reqIP, currentTime); const password = authenticationValue; if (common.appConfig.rtlPass === password && failed.count < ALLOWED_LOGIN_ATTEMPTS) { - if (twoFAToken && twoFAToken !== '') { - if (!verifyToken(twoFAToken)) { + // Gate on the server-side 2FA configuration, not on the request: when 2FA is + // enabled a token is mandatory, so a request omitting twoFAToken is rejected + // 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. + // 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; failed.lastTried = currentTime; 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 85a1b510..3c154d25 100644 --- a/package.json +++ b/package.json @@ -16,7 +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", - "test": "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/release-notes/Release-notes-0.15.10.md b/release-notes/Release-notes-0.15.10.md index efafa0a1..3082992c 100644 --- a/release-notes/Release-notes-0.15.10.md +++ b/release-notes/Release-notes-0.15.10.md @@ -3,6 +3,14 @@ This document collects the changes that go into the 0.15.10 release. Each PR merged for this release should add its entry under the appropriate section below. +## Bug Fixes + +- **Auth: harden login request validation** + ([#1654](https://github.com/Ride-The-Lightning/RTL/pull/1654)). + Tightens server-side validation of authentication requests and adds regression coverage + (`test/backend/authenticate.test.mjs`). Users who have two-factor authentication enabled + are encouraged to update promptly. + ## Code Health - **Batch dependency update resolving the open Dependabot security PRs** diff --git a/server/controllers/shared/authenticate.ts b/server/controllers/shared/authenticate.ts index 252f6b88..e8917e67 100644 --- a/server/controllers/shared/authenticate.ts +++ b/server/controllers/shared/authenticate.ts @@ -21,6 +21,9 @@ const loginInterval = setInterval(() => { } } }, LOCKING_PERIOD); +// The sweeper must not hold the event loop open on its own (it would keep +// `node --test` or a CLI invocation alive for the full 30-minute period). +loginInterval.unref(); export const getFailedInfo = (reqIP, currentTime) => { let failed = { count: 0, lastTried: currentTime }; @@ -49,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..' }); @@ -80,8 +98,15 @@ export const authenticateUser = (req, res, next) => { const failed = getFailedInfo(reqIP, currentTime); const password = authenticationValue; if (common.appConfig.rtlPass === password && failed.count < ALLOWED_LOGIN_ATTEMPTS) { - if (twoFAToken && twoFAToken !== '') { - if (!verifyToken(twoFAToken)) { + // Gate on the server-side 2FA configuration, not on the request: when 2FA is + // enabled a token is mandatory, so a request omitting twoFAToken is rejected + // 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. + // 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; failed.lastTried = currentTime; 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 new file mode 100644 index 00000000..14b8d797 --- /dev/null +++ b/test/backend/authenticate.test.mjs @@ -0,0 +1,171 @@ +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'; + +const { authenticator } = otplib; + +const TOTP_SECRET = 'JBSWY3DPEHPK3PXP'; +const PASSWORD_HASH = 'hashed-password'; + +const setupAppConfig = (enable2FA, secret2FA) => { + Common.appConfig = { + defaultNodeIndex: 0, + selectedNodeIndex: 0, + rtlConfFilePath: '', + dbDirectoryPath: '', + rtlPass: PASSWORD_HASH, + allowPasswordUpdate: true, + enable2FA: enable2FA, + secret2FA: secret2FA, + SSO: { rtlSSO: 0, rtlCookiePath: '', logoutRedirectLink: '', cookieValue: '' }, + nodes: [] + }; + Common.selectedNode = null; + Common.nodes = []; +}; + +// 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 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 || PASSWORD_HASH, twoFAToken: twoFAToken }, + session: {}, + headers: headers, + connection: {}, + socket: {} + }; +}; + +const mockResponse = () => { + const res = { statusCode: null, body: null }; + res.status = (code) => { + res.statusCode = code; + return { json: (body) => { res.body = body; } }; + }; + 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({ twoFAToken: missingToken }), res, null); + assert.equal(res.statusCode, 401); + assert.match(res.body.error, /2FA/); + } +}); + +test('rejects login with an invalid 2FA token when 2FA is enabled', () => { + setupAppConfig(true, TOTP_SECRET); + const res = mockResponse(); + authenticateUser(mockRequest({ twoFAToken: '000000' }), res, null); + assert.equal(res.statusCode, 401); + assert.match(res.body.error, /2FA/); +}); + +test('rejects a non-string 2FA token when 2FA is enabled', () => { + setupAppConfig(true, TOTP_SECRET); + // A JSON body can carry an array/object/number. otplib 12.0.1 coerces and rejects these + // (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({ twoFAToken: ['1', '2', '3', '4', '5', '6'] }), res, null); + assert.equal(res.statusCode, 401); + assert.match(res.body.error, /2FA/); +}); + +test('accepts login with a valid 2FA token when 2FA is enabled', () => { + setupAppConfig(true, TOTP_SECRET); + const res = mockResponse(); + 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({ twoFAToken: undefined }), res, null); + assert.equal(res.statusCode, 200); + assert.equal(typeof res.body.token, 'string'); +}); + +test('accepts a stale token in the request when 2FA is not configured', () => { + // Pins an intentional behavior change: previously a non-empty twoFAToken with no + // configured secret was rejected (verifyToken short-circuits on the empty secret); + // with no 2FA configured the token is now ignored entirely. + setupAppConfig(false, ''); + const res = mockResponse(); + authenticateUser(mockRequest({ twoFAToken: '123456' }), res, null); + assert.equal(res.statusCode, 200); + assert.equal(typeof res.body.token, 'string'); +}); + +test('does not require a token when 2FA is disabled but a stale secret remains', () => { + // The login UI prompts only when enable2FA is set, so enforcing a token on a stale + // secret would lock the operator out of a UI that never asks for one. + setupAppConfig(false, TOTP_SECRET); + const res = mockResponse(); + authenticateUser(mockRequest({ twoFAToken: undefined }), res, null); + assert.equal(res.statusCode, 200); + assert.equal(typeof res.body.token, 'string'); +}); + +test('does not enforce a token when 2FA is enabled without a secret', () => { + // Divergence is only reachable via a crafted settings update; a token could never + // verify against an empty secret, so enforcing would lock everyone out. + setupAppConfig(true, ''); + const res = mockResponse(); + authenticateUser(mockRequest({ twoFAToken: undefined }), res, null); + assert.equal(res.statusCode, 200); + assert.equal(typeof res.body.token, 'string'); +});