Fix R01-N08: idle session timeout + CSRF rotation on login

A signed-in session previously stayed valid until the browser closed
or the 8h `gc_maxlifetime` GC tick fired. A stolen session cookie
paired with the same-session CSRF token was good for hours of
attacker-driven mutations.

`SessionGuard::start()` now drives a 30-minute idle window on
authenticated sessions: any request that lands more than
`IDLE_TIMEOUT_SECONDS` (1800s) after the previous one drops
`user_id` / `login_at` / `last_active` / `csrf_token` and rotates
the session id, so the next gate sees an anonymous session and
redirects to `/auth/login`. Foreign session keys (e.g. the OIDC
library's `state`/`nonce`/PKCE values stored before the bounce to
Entra) are preserved so an in-flight login flow is not killed by a
stale idle clock from a previous logged-in session.

`SessionGuard::login()` now stamps `last_active = time()` and
`unset()`s `csrf_token`, so the token a pre-login attacker may
have captured from the public homepage form cannot be replayed
against the now-authenticated session — the next `csrfToken()`
call mints a fresh `bin2hex(random_bytes(32))` value.

The boundary is `>=` (a session whose `last_active` is exactly
1800s ago is treated as expired). The two helpers
`isIdleExpired(int $lastActive, int $now): bool` and
`expireIdleSession(array &$session, int $now): bool` are public
and pure-static so the policy is testable without spinning up
PHP's session machinery.

New `tests/Auth/SessionGuardTest.php` (9 cases) covers the
constant value, the boundary semantics (zero / 1s / 1799s /
1800s / 2h gaps), the anonymous-session no-op, the
`last_active`-missing seed-on-first-hit branch, the auth-key
drop on idle (with foreign-key survival), the exact-boundary
expiry, the just-fresh case, and the non-int `user_id` defence
that mirrors `currentUserId()`'s contract. Tests: 211 / 562
(was 202 / 533).

Ninth fix from `doc/REVIEW_01.md`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

src/Auth/SessionGuard.php

@@ -14,9 +14,10 @@ use App\Repositories\UserRepository;
  * Session lifecycle + CSRF helpers.
  *
  * Session data:
- *   $_SESSION['user_id']    — int id, set after successful login
- *   $_SESSION['login_at']   — unix timestamp of login
- *   $_SESSION['csrf_token'] — hex token, lazy-created
+ *   $_SESSION['user_id']     — int id, set after successful login
+ *   $_SESSION['login_at']    — unix timestamp of login
+ *   $_SESSION['last_active'] — unix timestamp of last request (R01-N08)
+ *   $_SESSION['csrf_token']  — hex token, lazy-created; rotated on login
  *
  * Intentionally stateless utility methods; the caller hydrates the full User
  * from the repo on demand.
@@ -25,6 +26,15 @@ final class SessionGuard
 {
     public const COOKIE_NAME = 'sp_session';
 
+    /**
+     * R01-N08: maximum allowed gap (seconds) between two requests on a
+     * signed-in session before the session is expired and the user has to
+     * re-authenticate. 30 minutes — much shorter than the historical 8h
+     * `gc_maxlifetime` cookie lifetime, which only governed garbage
+     * collection and not session validity.
+     */
+    public const IDLE_TIMEOUT_SECONDS = 1800;
+
     public static function start(): void
     {
         if (session_status() === PHP_SESSION_ACTIVE) {
@@ -66,6 +76,13 @@ final class SessionGuard
             'secure'   => $secure,
         ]);
         session_start();
+
+        // R01-N08: enforce idle timeout on signed-in sessions. Anonymous
+        // sessions (e.g., the OIDC state/nonce stored before the bounce to
+        // Entra) are left untouched so an in-flight login flow survives.
+        if (self::expireIdleSession($_SESSION, time())) {
+            @session_regenerate_id(true);
+        }
     }
 
     public static function login(User $user): void
@@ -74,8 +91,13 @@ final class SessionGuard
         // Fresh id on privilege change, but preserve any pre-login state
         // (the OIDC client stores its nonce/state in the session).
         session_regenerate_id(true);
-        $_SESSION['user_id']  = $user->id;
-        $_SESSION['login_at'] = time();
+        $_SESSION['user_id']     = $user->id;
+        $_SESSION['login_at']    = time();
+        $_SESSION['last_active'] = time();
+        // R01-N08: rotate the CSRF token on privilege change so a token
+        // captured by a pre-login attacker (e.g. from the public homepage
+        // form) cannot be replayed against the now-authenticated session.
+        unset($_SESSION['csrf_token']);
     }
 
     public static function logout(): void
@@ -124,6 +146,56 @@ final class SessionGuard
         return (string) $_SESSION['csrf_token'];
     }
 
+    /**
+     * R01-N08: pure-static idle-window check. Public + parameterised so the
+     * boundary (1800s) and the `>=` semantics can be tested without spinning
+     * up a real PHP session.
+     */
+    public static function isIdleExpired(int $lastActive, int $now): bool
+    {
+        return ($now - $lastActive) >= self::IDLE_TIMEOUT_SECONDS;
+    }
+
+    /**
+     * R01-N08: apply the idle policy to a session array.
+     *
+     * Returns true when the caller should rotate the session id (the
+     * authenticated state was just dropped). Pre-login sessions — those
+     * with no integer `user_id` — are left untouched, so an in-flight
+     * OIDC bounce is not killed by the idle clock running on stale
+     * `last_active` data from a previous logged-in session that has
+     * since signed out.
+     *
+     * On a still-fresh signed-in session the caller's `last_active`
+     * timestamp is bumped to `$now` so the idle window slides forward.
+     *
+     * @param array<string, mixed> $session  Mutated in-place ($_SESSION).
+     */
+    public static function expireIdleSession(array &$session, int $now): bool
+    {
+        $userId = $session['user_id'] ?? null;
+        if (!is_int($userId)) {
+            return false;
+        }
+
+        $lastActive = $session['last_active'] ?? null;
+        if (is_int($lastActive) && self::isIdleExpired($lastActive, $now)) {
+            // Drop only the authenticated keys; leave foreign state
+            // (e.g. the OIDC library's nonce/state/PKCE) intact so a
+            // re-login flow is not crippled by the timeout.
+            unset(
+                $session['user_id'],
+                $session['login_at'],
+                $session['last_active'],
+                $session['csrf_token'],
+            );
+            return true;
+        }
+
+        $session['last_active'] = $now;
+        return false;
+    }
+
     /** Returns true when the request is GET/HEAD (not guarded) or the token matches. */
     public static function verifyCsrf(Request $req): bool
     {

tests/Auth/SessionGuardTest.php

@@ -0,0 +1,176 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Auth;
+
+use App\Auth\SessionGuard;
+use App\Tests\TestCase;
+
+/**
+ * Pure-static unit tests for the R01-N08 idle-timeout helpers on
+ * SessionGuard. These cover the boundary semantics of `isIdleExpired`
+ * and the in-place mutation contract of `expireIdleSession` on a
+ * vanilla PHP array, so the tests don't depend on the global
+ * `$_SESSION`, cookies, or `session_start()` state.
+ *
+ * The CSRF rotation behaviour of `login()` is asserted indirectly
+ * through `expireIdleSession`'s "drop csrf_token on idle" assertion —
+ * the same `unset()` semantics are reused by the login() body to
+ * force a fresh token on the next `csrfToken()` call.
+ */
+final class SessionGuardTest extends TestCase
+{
+    public function testIdleTimeoutConstantIsThirtyMinutes(): void
+    {
+        // Locks the policy so an accidental change shows up here as well as
+        // in the boundary tests below.
+        self::assertSame(1800, SessionGuard::IDLE_TIMEOUT_SECONDS);
+    }
+
+    public function testIsIdleExpiredFreshAndStaleBoundary(): void
+    {
+        $base = 1_700_000_000;
+
+        self::assertFalse(
+            SessionGuard::isIdleExpired($base, $base),
+            'zero gap is fresh',
+        );
+        self::assertFalse(
+            SessionGuard::isIdleExpired($base, $base + 1),
+            '1s gap is fresh',
+        );
+        self::assertFalse(
+            SessionGuard::isIdleExpired($base, $base + 1799),
+            '29:59 is still fresh',
+        );
+        self::assertTrue(
+            SessionGuard::isIdleExpired($base, $base + 1800),
+            'exactly the threshold is expired (>= boundary)',
+        );
+        self::assertTrue(
+            SessionGuard::isIdleExpired($base, $base + 7200),
+            '2h gap is expired',
+        );
+    }
+
+    public function testExpireIdleSessionLeavesAnonymousSessionAlone(): void
+    {
+        $now    = 1_700_000_000;
+        $before = ['oidc_state' => 'abc123', 'oidc_nonce' => 'xyz789'];
+
+        $session = $before;
+        $rotate  = SessionGuard::expireIdleSession($session, $now);
+
+        self::assertFalse($rotate);
+        self::assertSame(
+            $before,
+            $session,
+            'anonymous session: helper must not bump last_active or rotate',
+        );
+    }
+
+    public function testExpireIdleSessionBumpsLastActiveOnFreshHit(): void
+    {
+        $now     = 1_700_000_000;
+        $session = [
+            'user_id'     => 42,
+            'login_at'    => $now - 600,
+            'last_active' => $now - 600,
+            'csrf_token'  => 'deadbeef',
+        ];
+
+        $rotate = SessionGuard::expireIdleSession($session, $now);
+
+        self::assertFalse($rotate, '10-min idle is well within the window');
+        self::assertSame(42, $session['user_id']);
+        self::assertSame($now, $session['last_active'], 'last_active slides forward');
+        self::assertSame('deadbeef', $session['csrf_token'], 'csrf_token preserved');
+    }
+
+    public function testExpireIdleSessionBumpsLastActiveWhenMissing(): void
+    {
+        // A session that was created before the R01-N08 patch shipped will
+        // have user_id but no last_active key. The helper must seed it
+        // (not clobber the user with a phantom expiry).
+        $now     = 1_700_000_000;
+        $session = ['user_id' => 7, 'login_at' => $now - 60];
+
+        $rotate = SessionGuard::expireIdleSession($session, $now);
+
+        self::assertFalse($rotate);
+        self::assertSame($now, $session['last_active']);
+        self::assertSame(7, $session['user_id']);
+    }
+
+    public function testExpireIdleSessionDropsAuthKeysWhenIdle(): void
+    {
+        $now     = 1_700_000_000;
+        $session = [
+            'user_id'     => 42,
+            'login_at'    => $now - 7200,
+            'last_active' => $now - SessionGuard::IDLE_TIMEOUT_SECONDS,
+            'csrf_token'  => 'deadbeef',
+            // Foreign keys must survive — the OIDC library can store
+            // pre-login state under arbitrary keys.
+            'oidc_state'  => 'abc123',
+        ];
+
+        $rotate = SessionGuard::expireIdleSession($session, $now);
+
+        self::assertTrue($rotate, 'caller must rotate the session id');
+        self::assertArrayNotHasKey('user_id', $session);
+        self::assertArrayNotHasKey('login_at', $session);
+        self::assertArrayNotHasKey('last_active', $session);
+        self::assertArrayNotHasKey('csrf_token', $session);
+        self::assertSame(
+            'abc123',
+            $session['oidc_state'] ?? null,
+            'foreign session state must survive the timeout',
+        );
+    }
+
+    public function testExpireIdleSessionExactlyAtBoundaryExpires(): void
+    {
+        // The boundary is `>=` so a session whose last_active is exactly
+        // IDLE_TIMEOUT_SECONDS ago is treated as expired.
+        $now     = 1_700_000_000;
+        $session = [
+            'user_id'     => 42,
+            'last_active' => $now - SessionGuard::IDLE_TIMEOUT_SECONDS,
+            'csrf_token'  => 'deadbeef',
+        ];
+
+        self::assertTrue(SessionGuard::expireIdleSession($session, $now));
+        self::assertArrayNotHasKey('user_id', $session);
+    }
+
+    public function testExpireIdleSessionOneSecondBeforeBoundaryIsFresh(): void
+    {
+        $now     = 1_700_000_000;
+        $session = [
+            'user_id'     => 42,
+            'last_active' => $now - (SessionGuard::IDLE_TIMEOUT_SECONDS - 1),
+            'csrf_token'  => 'deadbeef',
+        ];
+
+        self::assertFalse(SessionGuard::expireIdleSession($session, $now));
+        self::assertSame(42, $session['user_id']);
+        self::assertSame($now, $session['last_active']);
+    }
+
+    public function testExpireIdleSessionIgnoresNonIntUserId(): void
+    {
+        // `currentUserId()` defends with `is_int($id) ? $id : null`, so a
+        // bogus string user_id is effectively anonymous. Mirror that
+        // contract here — no expiry, no last_active bump.
+        $now     = 1_700_000_000;
+        $session = ['user_id' => 'not-an-int', 'last_active' => $now - 9999];
+
+        $rotate = SessionGuard::expireIdleSession($session, $now);
+
+        self::assertFalse($rotate);
+        self::assertSame('not-an-int', $session['user_id']);
+        self::assertSame($now - 9999, $session['last_active']);
+    }
+}