Merge pull request #398 from the-luap/fix/auth-session-timeout-symmetry

fix(auth): /auth/session must enforce session timeout symmetrically (#350 recurrence)

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);
+  });
+});

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);
+    });
+  });
 });

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
 };

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')

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