From b106da1ededa27fc8727f2c0e74a9182e6e9c895 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Tue, 5 May 2026 00:12:01 +0200 Subject: [PATCH] fix(auth): /auth/session must enforce session timeout symmetrically (#350 recurrence) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../sessionTimeout.isSessionExpired.test.js | 112 ++++++++++++++++++ .../routes/authSession.symmetry.test.js | 95 ++++++++++++++- backend/src/middleware/sessionTimeout.js | 35 +++++- backend/src/routes/auth.js | 16 +++ frontend/src/hooks/useSessionTimeout.ts | 40 +++++-- 5 files changed, 286 insertions(+), 12 deletions(-) create mode 100644 backend/__tests__/middleware/sessionTimeout.isSessionExpired.test.js diff --git a/backend/__tests__/middleware/sessionTimeout.isSessionExpired.test.js b/backend/__tests__/middleware/sessionTimeout.isSessionExpired.test.js new file mode 100644 index 00000000..0b3180a7 --- /dev/null +++ b/backend/__tests__/middleware/sessionTimeout.isSessionExpired.test.js @@ -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); + }); +}); diff --git a/backend/__tests__/routes/authSession.symmetry.test.js b/backend/__tests__/routes/authSession.symmetry.test.js index b4137a8e..50ba848f 100644 --- a/backend/__tests__/routes/authSession.symmetry.test.js +++ b/backend/__tests__/routes/authSession.symmetry.test.js @@ -101,7 +101,12 @@ jest.mock('../../src/utils/tokenUtils', () => ({ })); 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', () => ({ info: jest.fn(), warn: jest.fn(), @@ -299,4 +304,92 @@ describe('GET /auth/session — symmetry with protected middleware', () => { expect(res.status).toBe(401); 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); + }); + }); }); diff --git a/backend/src/middleware/sessionTimeout.js b/backend/src/middleware/sessionTimeout.js index 1d4fc267..462cd490 100644 --- a/backend/src/middleware/sessionTimeout.js +++ b/backend/src/middleware/sessionTimeout.js @@ -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 endSession(token) { sessions.delete(token); @@ -153,8 +181,9 @@ function getActiveSessions() { return active; } -module.exports = { - sessionTimeoutMiddleware, +module.exports = { + sessionTimeoutMiddleware, + isSessionExpired, endSession, - getActiveSessions + getActiveSessions }; diff --git a/backend/src/routes/auth.js b/backend/src/routes/auth.js index 219d7ad7..fe122e8b 100644 --- a/backend/src/routes/auth.js +++ b/backend/src/routes/auth.js @@ -534,6 +534,22 @@ router.get('/session', async (req, res) => { 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') { try { const event = await db('events') diff --git a/frontend/src/hooks/useSessionTimeout.ts b/frontend/src/hooks/useSessionTimeout.ts index 1def4569..ff22521f 100644 --- a/frontend/src/hooks/useSessionTimeout.ts +++ b/frontend/src/hooks/useSessionTimeout.ts @@ -1,21 +1,45 @@ import { useEffect, useCallback } from 'react'; -import { useAdminAuth } from '../contexts'; import { api } from '../config/api'; // Hook to handle session timeout export const useSessionTimeout = () => { - const { logout } = useAdminAuth(); - const handleSessionTimeout = useCallback((error: any) => { if (error?.response?.data?.code === 'SESSION_TIMEOUT') { - // Clear local auth state - logout(); - // Redirect to login with message - window.location.href = '/admin/login?session=expired'; + // Defense-in-depth for the redirect-loop bug class (issue #350): + // the previous implementation called the AdminAuthContext logout() + // (which dispatches POST /auth/logout fire-and-forget AND has its + // 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 false; - }, [logout]); + }, []); useEffect(() => { // Add response interceptor to handle session timeout