diff --git a/backend/__tests__/routes/adminRememberMe.test.js b/backend/__tests__/routes/adminRememberMe.test.js new file mode 100644 index 00000000..32a1f6cd --- /dev/null +++ b/backend/__tests__/routes/adminRememberMe.test.js @@ -0,0 +1,225 @@ +/** + * "Remember me" on the admin login (#1186). + * + * The checkbox shipped with no `checked`, no `onChange` and no place in the + * request body, and `establishAdminSession` hardcoded `expiresIn: '24h'` — so + * it promised a longer session and changed nothing at all. + * + * What matters here is not just that checking it extends the session, but the + * two things that make it safe: an untouched form still gets exactly the 24h + * it always did, and the JWT and the cookie agree about how long that is. If + * they disagree the session either dies early (long cookie, short token) or + * outlives what the user consented to (short cookie, long token). + * + * Every assertion fails on the unfixed code. + */ + +const jwt = require('jsonwebtoken'); +const express = require('express'); +const request = require('supertest'); +const cookieParser = require('cookie-parser'); +const bcrypt = require('bcrypt'); +const { bootCrmDb } = require('../integration/helpers/crmDb'); + +const fsSync = require('fs'); +const osMod = require('os'); +const pathMod = require('path'); + +process.env.NODE_ENV = 'test'; +// Its own database file. bootCrmDb otherwise reuses whatever path is already +// configured, and a leftover from a previous run fails with +// "table `migrations` already exists". +process.env.TEST_DATABASE_PATH = pathMod.join( + fsSync.mkdtempSync(pathMod.join(osMod.tmpdir(), 'picpeak-rememberme-')), 'db.sqlite' +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'test-jwt-secret-not-a-real-key'; + +const tokenUtils = require('../../src/utils/tokenUtils'); + +describe('admin session lifetime honours remember me (#1186)', () => { + // One database for the file, booted before anything that touches it. Both + // groups below depend on it: the login route obviously, and the idle-timeout + // checks because getSessionTimeout() reads app_settings — and falls back to + // its 60-minute default when that read fails, which would let those tests + // pass without ever exercising the configured value. + let app; let db; let cleanup; let isSessionExpired; + // An obvious placeholder, matching what the other suites use + // (setupService.test.js:20). A high-entropy generated value reads like a real + // credential to secret scanning; this does not, and the login route only + // compares it against the hash seeded below. + const PASSWORD = 'Str0ng-Passw0rd!'; + const TIMEOUT_MINUTES = 60; + + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + await db('admin_users').insert({ + username: 'remember-admin', + email: 'remember-admin@example.com', + password_hash: await bcrypt.hash(PASSWORD, 10), + is_active: true, + }); + // Seeded explicitly so the timeout assertions below are measured against a + // known configured value rather than the error fallback. + await db('app_settings').insert({ + setting_key: 'security_session_timeout_minutes', + setting_value: JSON.stringify(TIMEOUT_MINUTES), + setting_type: 'security', + }); + + const authRouter = require('../../src/routes/auth'); + ({ isSessionExpired } = require('../../src/middleware/sessionTimeout')); + app = express(); + app.use(express.json()); + app.use(cookieParser()); + app.use('/api/auth', authRouter); + }, 120000); + + afterAll(async () => { if (cleanup) await cleanup(); }); + + // Minimal res double: enough to record what cookie options were used. + const makeRes = () => { + const cookies = []; + return { + cookies, + cookie: (name, value, options) => cookies.push({ name, value, options }), + // buildCookieOptionsWithExpiry reads the request off res.req in some + // deployments (proxy/secure detection); an empty object is enough. + req: { headers: {}, secure: false }, + }; + }; + + const DAY_MS = 24 * 60 * 60 * 1000; + + test('the two lifetimes are the ones we intend, and the default is unchanged', () => { + expect(tokenUtils.DEFAULT_MAX_AGE_MS).toBe(DAY_MS); + expect(tokenUtils.REMEMBER_ME_MAX_AGE_MS).toBe(30 * DAY_MS); + }); + + test('an unchecked box still gets the historical 24h cookie', () => { + const res = makeRes(); + tokenUtils.setAdminAuthCookie(res, 'a-token'); + expect(res.cookies).toHaveLength(1); + expect(res.cookies[0].options.maxAge).toBe(DAY_MS); + }); + + test('explicitly unchecked is treated the same as absent', () => { + const res = makeRes(); + tokenUtils.setAdminAuthCookie(res, 'a-token', { rememberMe: false }); + expect(res.cookies[0].options.maxAge).toBe(DAY_MS); + }); + + test('checking it extends the cookie to 30 days', () => { + const res = makeRes(); + tokenUtils.setAdminAuthCookie(res, 'a-token', { rememberMe: true }); + expect(res.cookies[0].options.maxAge).toBe(30 * DAY_MS); + }); + + test('a missing token still sets no cookie at all', () => { + const res = makeRes(); + tokenUtils.setAdminAuthCookie(res, null, { rememberMe: true }); + expect(res.cookies).toHaveLength(0); + }); + + describe('the JWT and the cookie agree — through the real login route', () => { + // Driven through POST /api/auth/admin/login rather than a local jwt.sign() + // clone. A clone reproduces the ternary we hope production has, so it goes + // on passing if establishAdminSession regresses to a flat 24h — which is + // exactly the mismatch these assertions claim to guard. + const login = (rememberMe) => { + const body = { username: 'remember-admin', password: PASSWORD }; + if (rememberMe !== undefined) body.remember_me = rememberMe; + return request(app).post('/api/auth/admin/login').send(body); + }; + + const cookieMaxAge = (res) => { + const raw = (res.headers['set-cookie'] || []).find((c) => c.startsWith('admin_token=')); + expect(raw).toBeTruthy(); + const m = /Max-Age=(\d+)/i.exec(raw); + expect(m).toBeTruthy(); + return Number(m[1]) * 1000; + }; + + const tokenLifetime = (res) => { + const raw = (res.headers['set-cookie'] || []).find((c) => c.startsWith('admin_token=')); + const token = decodeURIComponent(raw.split(';')[0].split('=').slice(1).join('=')); + const { iat, exp, rememberMe } = jwt.decode(token); + return { ms: (exp - iat) * 1000, rememberMe }; + }; + + test('unchecked: 24h token, 24h cookie', async () => { + const res = await login(false); + expect(res.status).toBe(200); + expect(tokenLifetime(res).ms).toBe(DAY_MS); + expect(cookieMaxAge(res)).toBe(DAY_MS); + }); + + test('omitted entirely behaves like unchecked', async () => { + const res = await login(undefined); + expect(res.status).toBe(200); + expect(tokenLifetime(res).ms).toBe(DAY_MS); + expect(cookieMaxAge(res)).toBe(DAY_MS); + }); + + test('checked: 30d token, 30d cookie, and the flag is in the payload', async () => { + const res = await login(true); + expect(res.status).toBe(200); + const { ms, rememberMe } = tokenLifetime(res); + expect(ms).toBe(30 * DAY_MS); + expect(cookieMaxAge(res)).toBe(30 * DAY_MS); + // In the payload because the idle-timeout middleware reads it — a 30-day + // token that sessionTimeoutMiddleware still expires after an hour is the + // bug this whole feature would otherwise ship with. + expect(rememberMe).toBe(true); + }); + }); + + describe('the idle timeout respects a remembered session', () => { + // The half that made the feature non-functional: the default idle timeout + // is 60 minutes, so before this a remembered admin was logged out within + // the hour no matter how long their 30-day token said it lived. + const hoursAgoIat = (h) => Math.floor((Date.now() - h * 60 * 60 * 1000) / 1000); + + test('an ordinary session still expires when idle past the timeout', async () => { + const decoded = { id: 1, iat: hoursAgoIat(5) }; + expect(await isSessionExpired('tok-ordinary', decoded)).toBe(true); + }); + + test('a remembered session does not', async () => { + const decoded = { id: 1, iat: hoursAgoIat(5), rememberMe: true }; + expect(await isSessionExpired('tok-remembered', decoded)).toBe(false); + }); + + test('a remembered session survives an idle gap far beyond the timeout', async () => { + // Three days: the case from the report — come back after the weekend. + const decoded = { id: 1, iat: hoursAgoIat(72), rememberMe: true }; + expect(await isSessionExpired('tok-weekend', decoded)).toBe(false); + }); + + test('only an explicit true counts', async () => { + // A truthy-but-not-true value must not buy an exemption. + for (const value of [undefined, false, 'true', 1]) { + const decoded = { id: 1, iat: hoursAgoIat(5), rememberMe: value }; + expect(await isSessionExpired(`tok-${String(value)}`, decoded)).toBe(true); + } + }); + }); + + test('the extended cookie keeps every other security attribute', () => { + // A longer life must not quietly relax httpOnly/sameSite — the whole point + // of opting in is a longer session, not a weaker one. + const short = makeRes(); + const long = makeRes(); + tokenUtils.setAdminAuthCookie(short, 't'); + tokenUtils.setAdminAuthCookie(long, 't', { rememberMe: true }); + + const withoutLifetime = (options) => { + const rest = { ...options }; + delete rest.maxAge; + delete rest.expires; + return rest; + }; + const longRest = withoutLifetime(long.cookies[0].options); + expect(longRest).toEqual(withoutLifetime(short.cookies[0].options)); + expect(longRest.httpOnly).toBe(true); + }); +}); diff --git a/backend/src/middleware/auth.js b/backend/src/middleware/auth.js index a96478b4..4e22b7cf 100644 --- a/backend/src/middleware/auth.js +++ b/backend/src/middleware/auth.js @@ -134,7 +134,12 @@ async function adminAuth(req, res, next) { username: admin.username, email: admin.email, roleId: admin.role_id, - roleName: admin.role_name + roleName: admin.role_name, + // From the token, not the database: it is a property of this session + // rather than of the account (#1186). Carried so a route that reissues + // the token — change-password — can preserve the choice instead of + // silently dropping the session back to 24h. + rememberMe: decoded.rememberMe === true }; req.token = token; // Store token for potential revocation diff --git a/backend/src/middleware/sessionTimeout.js b/backend/src/middleware/sessionTimeout.js index 29c0b315..1a37b96f 100644 --- a/backend/src/middleware/sessionTimeout.js +++ b/backend/src/middleware/sessionTimeout.js @@ -94,6 +94,16 @@ async function sessionTimeoutMiddleware(req, res, next) { return next(); } + // "Remember me" opts out of the IDLE timeout (#1186). Not out of expiry: + // the token still dies on its own 30-day `exp`, and every other control + // (revocation, deactivation, password change) is untouched. Without this + // the checkbox does nothing observable — the default idle timeout is 60 + // minutes, so a remembered admin was logged out the same hour. + if (decoded.rememberMe === true) { + sessions.set(token, Date.now()); + return next(); + } + const now = Date.now(); const lastActivity = sessions.get(token); const timeout = await getSessionTimeout(); @@ -164,6 +174,10 @@ async function sessionTimeoutMiddleware(req, res, next) { // tracks activity; /auth/session is read-only by design. async function isSessionExpired(token, decoded) { if (!token || !decoded || !decoded.id) return false; + // Same exemption as sessionTimeoutMiddleware (#1186) — these two must agree, + // or /auth/session and the request path would disagree about whether the + // caller is still logged in. + if (decoded.rememberMe === true) return false; const now = Date.now(); const timeout = await getSessionTimeout(); const lastActivity = sessions.get(token); diff --git a/backend/src/routes/adminAuth.js b/backend/src/routes/adminAuth.js index 29e98460..fef31a1c 100644 --- a/backend/src/routes/adminAuth.js +++ b/backend/src/routes/adminAuth.js @@ -101,6 +101,9 @@ router.post('/change-password', [ const { currentPassword, newPassword } = req.body; const userId = req.admin.id; + // The session being replaced was or wasn't a remembered one; the new token + // has to inherit that (#1186). + const rememberMe = req.admin.rememberMe === true; // Validate new password strength const passwordValidation = validatePasswordStrength(newPassword); @@ -148,13 +151,19 @@ router.post('/change-password', [ type: 'admin', role: user.role_name, iat: iatAfterPasswordChange, - loginTime: Date.now() + loginTime: Date.now(), + // Must ride along in the payload too, or the reissued session loses its + // exemption from the idle timeout and dies within the hour. + rememberMe }, process.env.JWT_SECRET, { - expiresIn: '24h', + // Carried from the session being replaced (#1186): a password change — + // which is mandatory for new and reset accounts — would otherwise drop a + // remembered admin straight back to 24h. + expiresIn: rememberMe ? '30d' : '24h', issuer: 'picpeak-auth' }); - setAdminAuthCookie(res, newToken); + setAdminAuthCookie(res, newToken, { rememberMe }); // Log activity await logActivity('password_changed', diff --git a/backend/src/routes/auth.js b/backend/src/routes/auth.js index bdeb36f1..b4d6ae78 100644 --- a/backend/src/routes/auth.js +++ b/backend/src/routes/auth.js @@ -45,7 +45,7 @@ const router = express.Router(); * both produce an identical session. `lockoutKey` is the identifier the user * typed (username or email) so success/failure tracking stays in one bucket. */ -async function establishAdminSession(res, admin, ipAddress, userAgent, lockoutKey) { +async function establishAdminSession(res, admin, ipAddress, userAgent, lockoutKey, { rememberMe = false } = {}) { await trackSuccessfulLogin(lockoutKey, ipAddress, userAgent); // A normal login means the first-run wizard is over — the wizard never hits @@ -70,13 +70,21 @@ async function establishAdminSession(res, admin, ipAddress, userAgent, lockoutKe type: 'admin', role: admin.role_name, ip: ipAddress, - loginTime: Date.now() + loginTime: Date.now(), + // In the payload, not just in the expiry, because the idle-timeout + // middleware has to see it: a 30-day token is worth nothing if + // sessionTimeoutMiddleware still logs the session out after an hour. + rememberMe }, process.env.JWT_SECRET, { - expiresIn: '24h', + // "Remember me" (#1186). Opt-in: unchecked behaviour is unchanged at 24h, + // so the longer window only exists where somebody asked for it. The cookie + // below is given the matching max-age — if the two disagree the session + // either dies early or outlives its token. + expiresIn: rememberMe ? '30d' : '24h', issuer: 'picpeak-auth' }); - setAdminAuthCookie(res, token); + setAdminAuthCookie(res, token, { rememberMe }); // A fresh login supersedes any SSO marker a previous session left behind // (#798 phase 3): sessions can die without /logout (deactivation, expiry, @@ -97,15 +105,18 @@ async function establishAdminSession(res, admin, ipAddress, userAgent, lockoutKe }; } -async function completeAdminLogin(req, res, admin, ipAddress, userAgent, lockoutKey) { - const user = await establishAdminSession(res, admin, ipAddress, userAgent, lockoutKey); +async function completeAdminLogin(req, res, admin, ipAddress, userAgent, lockoutKey, { rememberMe = false } = {}) { + const user = await establishAdminSession(res, admin, ipAddress, userAgent, lockoutKey, { rememberMe }); return res.json({ user }); } // Admin login with enhanced security router.post('/admin/login', [ body('username').notEmpty().trim(), - body('password').notEmpty() + body('password').notEmpty(), + // Optional and boolean-coerced: an absent or malformed value means "no", + // so a client that never sends it keeps the 24h session it always had. + body('remember_me').optional().isBoolean().toBoolean() ], async (req, res) => { try { const errors = validationResult(req); @@ -114,6 +125,9 @@ router.post('/admin/login', [ } const { username, password, recaptchaToken } = req.body; + // Validator above coerces this to a real boolean and leaves it undefined + // when absent, so the fallback keeps the historical 24h session (#1186). + const rememberMe = req.body.remember_me === true; const ipAddress = getClientIp(req); const userAgent = req.headers['user-agent'] || ''; @@ -186,7 +200,12 @@ router.post('/admin/login', [ id: admin.id, username: admin.username, type: 'mfa_pending', - loginId: username + loginId: username, + // Carried in the signed token rather than re-sent by the client at the + // verify step: the choice was made at the password prompt, and this + // way the second leg cannot be talked into a longer session than the + // first one asked for. + rememberMe }, process.env.JWT_SECRET, { expiresIn: '5m', issuer: 'picpeak-auth' @@ -194,7 +213,7 @@ router.post('/admin/login', [ return res.json({ mfaRequired: true, mfaToken }); } - return await completeAdminLogin(req, res, admin, ipAddress, userAgent, username); + return await completeAdminLogin(req, res, admin, ipAddress, userAgent, username, { rememberMe }); } catch (error) { errorResponse(res, error, 500, 'Login failed'); } @@ -301,7 +320,12 @@ router.post('/admin/login/mfa', [ { type: 'admin', id: admin.id, name: admin.username } ); - return await completeAdminLogin(req, res, admin, ipAddress, userAgent, lockoutKey); + // Taken from the signed mfa_pending token, not from this request: the + // choice belongs to the password step, and reading it back out of the + // token stops the verify leg asking for a longer session than was agreed. + return await completeAdminLogin(req, res, admin, ipAddress, userAgent, lockoutKey, { + rememberMe: decoded.rememberMe === true, + }); } catch (error) { logger.error('MFA verification error:', error); res.status(500).json({ error: 'Verification failed' }); diff --git a/backend/src/utils/tokenUtils.js b/backend/src/utils/tokenUtils.js index a059aa96..fd1cc33e 100644 --- a/backend/src/utils/tokenUtils.js +++ b/backend/src/utils/tokenUtils.js @@ -9,6 +9,9 @@ const GUEST_COOKIE_PREFIX = 'guest_token_'; const CUSTOMER_COOKIE_NAME = 'customer_token'; const DEFAULT_MAX_AGE_MS = 24 * 60 * 60 * 1000; // 24 hours +// "Remember me" (#1186). Opt-in only: the default stays 24h, so a stolen +// cookie is worth a day unless the operator explicitly asked for longer. +const REMEMBER_ME_MAX_AGE_MS = 30 * 24 * 60 * 60 * 1000; // 30 days /** * Cookie "Secure" flag mode: @@ -109,9 +112,14 @@ function sanitizeSlugForCookie(slug = '') { return String(slug).replace(/[^A-Za-z0-9_-]/g, '_'); } -function setAdminAuthCookie(res, token) { +function setAdminAuthCookie(res, token, { rememberMe = false } = {}) { if (!token) return; - res.cookie(ADMIN_COOKIE_NAME, token, buildCookieOptionsWithExpiry(res)); + // The cookie's lifetime has to track the JWT's, or one outlives the other: + // a 30-day cookie carrying a 24h token means a silent 401 the next morning, + // and a 24h cookie carrying a 30-day token throws away the session the user + // asked to keep. + const maxAge = rememberMe ? REMEMBER_ME_MAX_AGE_MS : DEFAULT_MAX_AGE_MS; + res.cookie(ADMIN_COOKIE_NAME, token, buildCookieOptionsWithExpiry(res, maxAge)); } function clearAdminAuthCookie(res) { @@ -231,6 +239,8 @@ function getGuestTokenFromRequest(req, slug) { module.exports = { ADMIN_COOKIE_NAME, buildCookieOptionsWithExpiry, + DEFAULT_MAX_AGE_MS, + REMEMBER_ME_MAX_AGE_MS, buildClearCookieOptions, GALLERY_COOKIE_NAME, GALLERY_COOKIE_PREFIX, diff --git a/frontend/src/pages/admin/AdminLoginPage.tsx b/frontend/src/pages/admin/AdminLoginPage.tsx index b7b7aafd..21561e86 100644 --- a/frontend/src/pages/admin/AdminLoginPage.tsx +++ b/frontend/src/pages/admin/AdminLoginPage.tsx @@ -25,6 +25,7 @@ export const AdminLoginPage: React.FC = () => { email: '', password: '', }); + const [rememberMe, setRememberMe] = useState(false); const [showPassword, setShowPassword] = useState(false); const [isLoading, setIsLoading] = useState(false); const [errors, setErrors] = useState>({}); @@ -121,7 +122,8 @@ export const AdminLoginPage: React.FC = () => { try { const response = await authService.adminLogin({ ...formData, - recaptchaToken + recaptchaToken, + rememberMe }); // MFA enabled → move to the second step instead of logging in. if (isMfaChallenge(response)) { @@ -345,6 +347,8 @@ export const AdminLoginPage: React.FC = () => {