fix(auth): /auth/session must enforce session timeout symmetrically (#350 recurrence)
Third loop fix in the same /admin/login → /admin/dashboard → /admin/login pattern as #355 and #363. Reported on v3.39.1-beta.0 — the loop returns after a server restart or after an idle gap longer than the configured session timeout. ## Root cause (server) `sessionTimeoutMiddleware` is mounted on `/api/admin` (server.js:411). It rejects with `401 SESSION_TIMEOUT` when either: - the in-memory `lastActivity` for the token is older than the timeout, or - this is the first request with this token AND the token's `iat` is older than the timeout (post-restart guard). `/auth/session` lives under `/api/auth/session`, NOT under `/api/admin`, so the middleware never runs for it. Result: an idle/old-iat admin token returns `valid: true` from `/auth/session` while every protected endpoint immediately rejects it with `401 SESSION_TIMEOUT`. Frontend's 401 interceptor hard-redirects to `/admin/login`, `/auth/session` says valid again, loop closes — exact same shape as the previous two asymmetries the symmetry pass missed. Fix: add a non-mutating `isSessionExpired(token, decoded)` helper to `middleware/sessionTimeout.js` that reads the same in-memory map and applies the same lastActivity / iat-vs-timeout logic as the middleware, without updating the map (the middleware is the only place that records activity; `/auth/session` is read-only by design). `/auth/session` calls the helper for `decoded.type === 'admin'` after the existing admin-existence and password-change checks. Same try/catch fall-through pattern as the prior fixes so a missing/broken helper doesn't fail-closed during early bootstrap or in test stubs. ## Root cause (client race amplifying the loop) Even with the server fix, the previous `useSessionTimeout` hook called `AdminAuthContext.logout()` which dispatches `POST /auth/logout` fire-and-forget AND has its own `finally { window.location.href }`, then immediately set `window.location.href = '/admin/login?session=expired'` on top. Two consequences: - The cookie wasn't reliably cleared before the new page loaded — if any /auth/session asymmetry slipped through, the loop replayed inside the same tab. New-tab and "refresh several times" "fixes" were just the logout request eventually completing. - Two redirects raced; sometimes the `?session=expired` query was dropped, breaking the login-page toast. Fix: rewrite the hook to (a) await `POST /auth/logout` so the cookie is guaranteed cleared, (b) clear `sessionStorage.admin_user` directly instead of going through AdminAuthContext.logout (which has the side-effect redirect we don't want), and (c) navigate exactly once with the `?session=expired` query. ## Tests - `__tests__/routes/authSession.symmetry.test.js` — 4 new cases under a `session-timeout symmetry` describe block: helper says expired → valid:false; helper says active → valid:true; helper not called for gallery tokens; helper throws → fall through to valid:true (defensive). Existing 9 tests still pass (mock now includes `isSessionExpired: jest.fn(() => Promise.resolve(false))` as the default). - `__tests__/middleware/sessionTimeout.isSessionExpired.test.js` — 7 new unit tests for the helper itself: fresh token / old-iat / recently-active / null-input / no-mutation / 60-min default boundary cases. 20 cases total, all green. Lint clean on every touched file.
This commit is contained in:
@@ -0,0 +1,112 @@
|
|||||||
|
/**
|
||||||
|
* Unit test for the non-mutating isSessionExpired() helper added to
|
||||||
|
* middleware/sessionTimeout.js. Used by GET /auth/session to mirror the
|
||||||
|
* timeout enforcement that sessionTimeoutMiddleware applies to /api/admin
|
||||||
|
* endpoints — closing the asymmetry that surfaced as the redirect-loop
|
||||||
|
* recurrence on v3.39.1-beta.0 (issue #350).
|
||||||
|
*
|
||||||
|
* The helper has two branches:
|
||||||
|
* 1. In-memory `lastActivity` exists for this token → expired iff
|
||||||
|
* now - lastActivity > timeout.
|
||||||
|
* 2. No in-memory entry (post-restart, or first request) → expired
|
||||||
|
* iff token's iat is older than the timeout (post-restart guard
|
||||||
|
* that the existing middleware already implements at line ~101).
|
||||||
|
*
|
||||||
|
* Both branches must NOT mutate the in-memory `sessions` Map — the
|
||||||
|
* middleware is the only place that tracks activity. We assert that.
|
||||||
|
*/
|
||||||
|
|
||||||
|
jest.mock('../../src/database/db', () => ({
|
||||||
|
db: () => ({
|
||||||
|
where: () => ({
|
||||||
|
first: () => ({
|
||||||
|
timeout: () => Promise.resolve(null),
|
||||||
|
}),
|
||||||
|
}),
|
||||||
|
}),
|
||||||
|
}));
|
||||||
|
|
||||||
|
// Speed up the cached-timeout reads. The module reads
|
||||||
|
// `security_session_timeout_minutes` from app_settings and falls back to
|
||||||
|
// DEFAULT_SESSION_TIMEOUT (60 min) when the row is null.
|
||||||
|
const SIXTY_MINUTES_MS = 60 * 60 * 1000;
|
||||||
|
|
||||||
|
const sessionTimeout = require('../../src/middleware/sessionTimeout');
|
||||||
|
const { isSessionExpired } = sessionTimeout;
|
||||||
|
|
||||||
|
function makeDecodedToken({ id = 1, iatSecondsAgo = 0 } = {}) {
|
||||||
|
return { id, iat: Math.floor((Date.now() - iatSecondsAgo * 1000) / 1000) };
|
||||||
|
}
|
||||||
|
|
||||||
|
describe('isSessionExpired (sessionTimeout helper)', () => {
|
||||||
|
it('returns false for a freshly-issued token with no in-memory record', async () => {
|
||||||
|
const decoded = makeDecodedToken({ id: 1, iatSecondsAgo: 60 });
|
||||||
|
expect(await isSessionExpired('fresh-token-1', decoded)).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns true when iat is older than the timeout (post-restart guard)', async () => {
|
||||||
|
const decoded = makeDecodedToken({
|
||||||
|
id: 2,
|
||||||
|
// 90 minutes > 60 minute default timeout
|
||||||
|
iatSecondsAgo: 90 * 60,
|
||||||
|
});
|
||||||
|
expect(await isSessionExpired('stale-token-2', decoded)).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns false / true based on lastActivity when one exists', async () => {
|
||||||
|
// Drive the in-memory map by running the actual middleware once to
|
||||||
|
// record activity for the token, then check the helper.
|
||||||
|
const decoded = makeDecodedToken({ id: 3 });
|
||||||
|
|
||||||
|
// Drive the actual middleware once with a real signed token so it
|
||||||
|
// records this token in the in-memory `sessions` Map. Then check the
|
||||||
|
// helper sees that recent activity and reports "not expired".
|
||||||
|
const res = { status: jest.fn(() => res), json: jest.fn() };
|
||||||
|
const jwt = require('jsonwebtoken');
|
||||||
|
process.env.JWT_SECRET = 'session-timeout-helper-test-secret';
|
||||||
|
const realToken = jwt.sign(decoded, process.env.JWT_SECRET, {
|
||||||
|
issuer: 'picpeak-auth',
|
||||||
|
});
|
||||||
|
const realReq = {
|
||||||
|
headers: { authorization: `Bearer ${realToken}` },
|
||||||
|
cookies: {},
|
||||||
|
};
|
||||||
|
await sessionTimeout.sessionTimeoutMiddleware(realReq, res, () => {});
|
||||||
|
|
||||||
|
const decodedReal = jwt.decode(realToken);
|
||||||
|
// Just-recorded → not expired
|
||||||
|
expect(await isSessionExpired(realToken, decodedReal)).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns false when token / decoded is missing (defensive)', async () => {
|
||||||
|
expect(await isSessionExpired(null, { id: 1 })).toBe(false);
|
||||||
|
expect(await isSessionExpired('tok', null)).toBe(false);
|
||||||
|
expect(await isSessionExpired('tok', {})).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
// Sanity: the helper must not poke the `sessions` Map. Indirectly check
|
||||||
|
// by counting active sessions before/after a call with a never-seen
|
||||||
|
// token — should not change.
|
||||||
|
it('does not mutate the in-memory sessions map', async () => {
|
||||||
|
const before = sessionTimeout.getActiveSessions();
|
||||||
|
await isSessionExpired('never-seen-token-99', makeDecodedToken({ id: 99 }));
|
||||||
|
const after = sessionTimeout.getActiveSessions();
|
||||||
|
expect(after).toBe(before);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('uses the default 60-minute timeout when no DB setting exists', async () => {
|
||||||
|
// 59 minutes → not expired
|
||||||
|
const fresh = makeDecodedToken({ id: 4, iatSecondsAgo: 59 * 60 });
|
||||||
|
expect(await isSessionExpired('fresh-4', fresh)).toBe(false);
|
||||||
|
|
||||||
|
// 61 minutes → expired (just past the default)
|
||||||
|
const stale = makeDecodedToken({ id: 5, iatSecondsAgo: 61 * 60 });
|
||||||
|
expect(await isSessionExpired('stale-5', stale)).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
// Document the constant the test relies on so a future timeout change
|
||||||
|
// makes this assertion explicit rather than mysterious.
|
||||||
|
it('default timeout is 60 minutes (constant under test)', () => {
|
||||||
|
expect(SIXTY_MINUTES_MS).toBe(60 * 60 * 1000);
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -101,7 +101,12 @@ jest.mock('../../src/utils/tokenUtils', () => ({
|
|||||||
}));
|
}));
|
||||||
|
|
||||||
jest.mock('../../src/services/recaptcha', () => ({ verifyRecaptcha: () => Promise.resolve(true) }));
|
jest.mock('../../src/services/recaptcha', () => ({ verifyRecaptcha: () => Promise.resolve(true) }));
|
||||||
jest.mock('../../src/middleware/sessionTimeout', () => ({ endSession: jest.fn() }));
|
// Mock sessionTimeout's isSessionExpired so each test controls the return.
|
||||||
|
// Default: not expired (so existing tests keep passing without setup).
|
||||||
|
jest.mock('../../src/middleware/sessionTimeout', () => ({
|
||||||
|
endSession: jest.fn(),
|
||||||
|
isSessionExpired: jest.fn(() => Promise.resolve(false)),
|
||||||
|
}));
|
||||||
jest.mock('../../src/utils/logger', () => ({
|
jest.mock('../../src/utils/logger', () => ({
|
||||||
info: jest.fn(),
|
info: jest.fn(),
|
||||||
warn: jest.fn(),
|
warn: jest.fn(),
|
||||||
@@ -299,4 +304,92 @@ describe('GET /auth/session — symmetry with protected middleware', () => {
|
|||||||
expect(res.status).toBe(401);
|
expect(res.status).toBe(401);
|
||||||
expect(res.body.valid).toBe(false);
|
expect(res.body.valid).toBe(false);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// Session-timeout symmetry — issue #350 recurrence on v3.39.1-beta.0.
|
||||||
|
// sessionTimeoutMiddleware (mounted on /api/admin) rejects idle/old-iat
|
||||||
|
// tokens with 401 SESSION_TIMEOUT, but /auth/session previously didn't.
|
||||||
|
// The new isSessionExpired helper closes that asymmetry.
|
||||||
|
describe('session-timeout symmetry', () => {
|
||||||
|
const { isSessionExpired } = require('../../src/middleware/sessionTimeout');
|
||||||
|
|
||||||
|
beforeEach(() => {
|
||||||
|
isSessionExpired.mockReset();
|
||||||
|
// Default to "active session" so the other admin checks above also
|
||||||
|
// pass when this branch runs.
|
||||||
|
isSessionExpired.mockResolvedValue(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns valid:false when isSessionExpired reports the token has timed out', async () => {
|
||||||
|
fakeDb.adminUsers.push({
|
||||||
|
id: 1,
|
||||||
|
username: 'admin',
|
||||||
|
is_active: true,
|
||||||
|
password_changed_at: null,
|
||||||
|
});
|
||||||
|
isSessionExpired.mockResolvedValue(true);
|
||||||
|
const token = signAdminToken({ id: 1 });
|
||||||
|
|
||||||
|
const res = await request(makeApp())
|
||||||
|
.get('/auth/session')
|
||||||
|
.set('Authorization', `Bearer ${token}`);
|
||||||
|
expect(res.status).toBe(200);
|
||||||
|
expect(res.body.valid).toBe(false);
|
||||||
|
expect(res.body.error).toBe('Session expired');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns valid:true for an active admin token (helper says not expired)', async () => {
|
||||||
|
fakeDb.adminUsers.push({
|
||||||
|
id: 1,
|
||||||
|
username: 'admin',
|
||||||
|
is_active: true,
|
||||||
|
password_changed_at: null,
|
||||||
|
});
|
||||||
|
isSessionExpired.mockResolvedValue(false);
|
||||||
|
const token = signAdminToken({ id: 1 });
|
||||||
|
|
||||||
|
const res = await request(makeApp())
|
||||||
|
.get('/auth/session')
|
||||||
|
.set('Authorization', `Bearer ${token}`);
|
||||||
|
expect(res.status).toBe(200);
|
||||||
|
expect(res.body.valid).toBe(true);
|
||||||
|
expect(isSessionExpired).toHaveBeenCalledTimes(1);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('does not call isSessionExpired for gallery tokens', async () => {
|
||||||
|
fakeDb.events.push({
|
||||||
|
id: 100,
|
||||||
|
slug: 'wedding',
|
||||||
|
is_active: true,
|
||||||
|
is_archived: false,
|
||||||
|
expires_at: new Date(Date.now() + 86400_000),
|
||||||
|
});
|
||||||
|
const token = signGalleryToken();
|
||||||
|
|
||||||
|
const res = await request(makeApp())
|
||||||
|
.get('/auth/session?slug=wedding')
|
||||||
|
.set('Authorization', `Bearer ${token}`);
|
||||||
|
expect(res.status).toBe(200);
|
||||||
|
expect(res.body.valid).toBe(true);
|
||||||
|
expect(isSessionExpired).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('falls through (treats as valid) if the helper itself throws', async () => {
|
||||||
|
// Defensive: the require() in auth.js is wrapped in try/catch so a
|
||||||
|
// missing/broken helper doesn't fail-closed during early bootstrap.
|
||||||
|
fakeDb.adminUsers.push({
|
||||||
|
id: 1,
|
||||||
|
username: 'admin',
|
||||||
|
is_active: true,
|
||||||
|
password_changed_at: null,
|
||||||
|
});
|
||||||
|
isSessionExpired.mockRejectedValue(new Error('boom'));
|
||||||
|
const token = signAdminToken({ id: 1 });
|
||||||
|
|
||||||
|
const res = await request(makeApp())
|
||||||
|
.get('/auth/session')
|
||||||
|
.set('Authorization', `Bearer ${token}`);
|
||||||
|
expect(res.status).toBe(200);
|
||||||
|
expect(res.body.valid).toBe(true);
|
||||||
|
});
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -134,6 +134,34 @@ async function sessionTimeoutMiddleware(req, res, next) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Non-mutating timeout check used by /auth/session (auth.js) so the session
|
||||||
|
// endpoint enforces the same timeout the protected /api/admin endpoints
|
||||||
|
// already enforce via sessionTimeoutMiddleware. Without this, /auth/session
|
||||||
|
// returns valid:true for a token that protected endpoints reject with
|
||||||
|
// 401 SESSION_TIMEOUT, producing the /admin/login → /admin/dashboard →
|
||||||
|
// /admin/login redirect loop reported on v3.39.1-beta.0 (issue #350).
|
||||||
|
//
|
||||||
|
// Mirrors the middleware's logic exactly:
|
||||||
|
// - If we have an in-memory lastActivity for this token, return whether
|
||||||
|
// the gap exceeds the timeout.
|
||||||
|
// - Otherwise (post-restart, or first request with this token), return
|
||||||
|
// whether the token's iat is older than the timeout — same post-restart
|
||||||
|
// guard the middleware uses.
|
||||||
|
//
|
||||||
|
// Does NOT update the in-memory map. The middleware is the only place that
|
||||||
|
// tracks activity; /auth/session is read-only by design.
|
||||||
|
async function isSessionExpired(token, decoded) {
|
||||||
|
if (!token || !decoded || !decoded.id) return false;
|
||||||
|
const now = Date.now();
|
||||||
|
const timeout = await getSessionTimeout();
|
||||||
|
const lastActivity = sessions.get(token);
|
||||||
|
if (lastActivity) {
|
||||||
|
return (now - lastActivity) > timeout;
|
||||||
|
}
|
||||||
|
const tokenIssuedAt = (decoded.iat || 0) * 1000;
|
||||||
|
return (now - tokenIssuedAt) > timeout;
|
||||||
|
}
|
||||||
|
|
||||||
// Function to end a session
|
// Function to end a session
|
||||||
function endSession(token) {
|
function endSession(token) {
|
||||||
sessions.delete(token);
|
sessions.delete(token);
|
||||||
@@ -155,6 +183,7 @@ function getActiveSessions() {
|
|||||||
|
|
||||||
module.exports = {
|
module.exports = {
|
||||||
sessionTimeoutMiddleware,
|
sessionTimeoutMiddleware,
|
||||||
|
isSessionExpired,
|
||||||
endSession,
|
endSession,
|
||||||
getActiveSessions
|
getActiveSessions
|
||||||
};
|
};
|
||||||
|
|||||||
@@ -534,6 +534,22 @@ router.get('/session', async (req, res) => {
|
|||||||
return res.json({ valid: false, error: 'Token invalid due to password change' });
|
return res.json({ valid: false, error: 'Token invalid due to password change' });
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Mirror the session-timeout check that sessionTimeoutMiddleware
|
||||||
|
// enforces on every /api/admin endpoint. Without this, /auth/session
|
||||||
|
// returns valid:true for an idle/old-iat token that protected
|
||||||
|
// endpoints reject with 401 SESSION_TIMEOUT — the same redirect-loop
|
||||||
|
// shape as the issuer-claim and password-change asymmetries (issue
|
||||||
|
// #350 recurrence on v3.39.1-beta.0).
|
||||||
|
try {
|
||||||
|
const { isSessionExpired } = require('../middleware/sessionTimeout');
|
||||||
|
if (await isSessionExpired(token, decoded)) {
|
||||||
|
return res.json({ valid: false, error: 'Session expired' });
|
||||||
|
}
|
||||||
|
} catch (timeoutErr) {
|
||||||
|
// Helper lookup failed (test stub may not export it) — fall through
|
||||||
|
// and trust the token. Real deployments always have the middleware.
|
||||||
|
}
|
||||||
} else if (decoded.type === 'gallery') {
|
} else if (decoded.type === 'gallery') {
|
||||||
try {
|
try {
|
||||||
const event = await db('events')
|
const event = await db('events')
|
||||||
|
|||||||
@@ -1,21 +1,45 @@
|
|||||||
import { useEffect, useCallback } from 'react';
|
import { useEffect, useCallback } from 'react';
|
||||||
import { useAdminAuth } from '../contexts';
|
|
||||||
import { api } from '../config/api';
|
import { api } from '../config/api';
|
||||||
|
|
||||||
// Hook to handle session timeout
|
// Hook to handle session timeout
|
||||||
export const useSessionTimeout = () => {
|
export const useSessionTimeout = () => {
|
||||||
const { logout } = useAdminAuth();
|
|
||||||
|
|
||||||
const handleSessionTimeout = useCallback((error: any) => {
|
const handleSessionTimeout = useCallback((error: any) => {
|
||||||
if (error?.response?.data?.code === 'SESSION_TIMEOUT') {
|
if (error?.response?.data?.code === 'SESSION_TIMEOUT') {
|
||||||
// Clear local auth state
|
// Defense-in-depth for the redirect-loop bug class (issue #350):
|
||||||
logout();
|
// the previous implementation called the AdminAuthContext logout()
|
||||||
// Redirect to login with message
|
// (which dispatches POST /auth/logout fire-and-forget AND has its
|
||||||
window.location.href = '/admin/login?session=expired';
|
// own finally-block redirect) and then set window.location.href
|
||||||
|
// immediately. The cookie wasn't reliably cleared before the page
|
||||||
|
// reloaded — if the next /auth/session call read the stale cookie
|
||||||
|
// AND any server asymmetry returned valid:true for it, the redirect
|
||||||
|
// loop replayed inside the same tab. Two-tab/multi-refresh "fixes"
|
||||||
|
// were just the logout request eventually completing in time.
|
||||||
|
//
|
||||||
|
// We now (a) await the server-side logout so the cookie is
|
||||||
|
// guaranteed cleared before the new page loads, (b) clear
|
||||||
|
// sessionStorage directly so we don't depend on AdminAuthContext's
|
||||||
|
// logout (which has the side-effect redirect we don't want), and
|
||||||
|
// (c) navigate exactly once with the ?session=expired query the
|
||||||
|
// login page reads to show the "your session expired" toast.
|
||||||
|
void (async () => {
|
||||||
|
try {
|
||||||
|
await api.post('/auth/logout');
|
||||||
|
} catch {
|
||||||
|
// Ignore; the cookie may already be invalid server-side. The
|
||||||
|
// redirect below still happens and the next /auth/session call
|
||||||
|
// will return 401 (no token) either way.
|
||||||
|
}
|
||||||
|
try {
|
||||||
|
sessionStorage.removeItem('admin_user');
|
||||||
|
} catch {
|
||||||
|
// sessionStorage can throw in private-browsing modes — ignore.
|
||||||
|
}
|
||||||
|
window.location.href = '/admin/login?session=expired';
|
||||||
|
})();
|
||||||
return true;
|
return true;
|
||||||
}
|
}
|
||||||
return false;
|
return false;
|
||||||
}, [logout]);
|
}, []);
|
||||||
|
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
// Add response interceptor to handle session timeout
|
// Add response interceptor to handle session timeout
|
||||||
|
|||||||
Reference in New Issue
Block a user