Forráskód Böngészése

fix: rotate CSRF token on session-id regeneration (SEC_REVIEW F40)

`SessionManager::regenerateId()` rotated the session id on login but
left `$_SESSION['_csrf']` intact. A pre-auth CSRF token leaked over
the privilege boundary — via Referer on a public route, a
sub-resource integrity check, or shared with a less-trusted page —
remained valid for the entire post-auth session lifetime, and could
be replayed against state-changing endpoints once the user's session
was elevated.

`regenerateId()` now drops the `_csrf` slot on both branches: the
HTTP path that calls `session_regenerate_id(true)` and the CLI
`rotateIdUnderCli` fallback. `CsrfMiddleware` already lazily mints a
fresh token on the next request when the slot is missing, and every
call site of `regenerateId()` (LocalLoginController on success,
OidcController initiate + callback) is followed by a 303 / 302
redirect, so the redirect target's GET re-issues a clean token
before any subsequent state-changing request. `clear()` already
wipes `$_SESSION` outright on logout so the rotate-on-id-rotate hook
covers the login direction.

The cleanup is factored into a private `rotateCsrfToken()` so both
regenerateId branches stay symmetric and a future contributor can
extend it (e.g. drop additional pre-auth-only slots) in one place.
A new `KEY_CSRF` constant on `SessionManager` mirrors
`CsrfMiddleware::SESSION_KEY` to avoid a domain → http-layer
dependency on what is otherwise a value-only collaboration.

Regression tests in `ui/tests/Unit/Auth/SessionManagerTest.php`
(`testRegenerateIdRotatesCsrfTokenInCliMode`,
`testRegenerateIdRotatesCsrfTokenInHttpMode`) cover both branches at
unit level. `ui/tests/Integration/Auth/LocalLoginTest.php`
(`testCsrfTokenIsRotatedAcrossLoginPrivilegeBoundary`) drives the
full GET /login → POST /login/local flow through Slim and asserts
the `_csrf` slot is gone after the privilege elevation.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa 4 napja
szülő
commit
30c0604e49

+ 23 - 0
ui/src/Auth/SessionManager.php

@@ -38,6 +38,10 @@ class SessionManager
     private const KEY_FLASH = '_flash';
     private const KEY_NEXT = '_next';
     private const KEY_OIDC = '_oidc';
+    /** Mirrors `App\Http\CsrfMiddleware::SESSION_KEY` — kept duplicated to
+     * avoid a domain → http-layer dependency on what is otherwise a
+     * value-only collaboration. */
+    private const KEY_CSRF = '_csrf';
 
     private readonly bool $cliFallback;
 
@@ -90,6 +94,7 @@ class SessionManager
         }
         if (!($this->headersSentFn)()) {
             session_regenerate_id(true);
+            $this->rotateCsrfToken();
 
             return;
         }
@@ -99,6 +104,24 @@ class SessionManager
             );
         }
         $this->rotateIdUnderCli();
+        $this->rotateCsrfToken();
+    }
+
+    /**
+     * SEC_REVIEW F40: drop the cached CSRF token whenever the session id
+     * rotates so a token leaked over the pre-auth privilege boundary
+     * (e.g. via Referer on a public route, a sub-resource integrity
+     * check, or shared with a less-trusted page) cannot be replayed
+     * post-auth. `CsrfMiddleware` lazily mints a fresh token on the
+     * next request when the slot is missing, so the redirect that
+     * always follows `regenerateId()` (303 to /app/dashboard, /no-access,
+     * /login, etc.) re-issues a clean token on its first protected
+     * GET. `clear()` already wipes `$_SESSION` outright on logout, so
+     * the rotate-on-id-rotate hook covers the login direction.
+     */
+    private function rotateCsrfToken(): void
+    {
+        unset($_SESSION[self::KEY_CSRF]);
     }
 
     public function setUser(UserContext $user): void

+ 32 - 0
ui/tests/Integration/Auth/LocalLoginTest.php

@@ -243,6 +243,38 @@ final class LocalLoginTest extends AppTestCase
         self::assertLessThanOrEqual($remainingBefore + 1, $remainingAfter);
     }
 
+    public function testCsrfTokenIsRotatedAcrossLoginPrivilegeBoundary(): void
+    {
+        // SEC_REVIEW F40: a CSRF token minted on the pre-auth login form
+        // must NOT carry over after a successful login. `regenerateId()`
+        // now drops `_csrf` so the next protected GET mints a fresh
+        // token. An attacker who scraped the pre-auth token (Referer,
+        // sub-resource leak) cannot replay it post-auth.
+        $this->enqueueApiResponse(200, [
+            'user_id' => 1,
+            'role' => 'admin',
+            'email' => null,
+            'display_name' => 'Local Admin',
+            'is_local' => true,
+        ]);
+
+        $this->request('GET', '/login');
+        $preAuthToken = (string) ($_SESSION[CsrfMiddleware::SESSION_KEY] ?? '');
+        self::assertNotEmpty($preAuthToken);
+
+        $body = http_build_query([
+            'csrf_token' => $preAuthToken,
+            'username' => 'admin',
+            'password' => 'test1234',
+        ]);
+        $this->request('POST', '/login/local', [], $body, 'application/x-www-form-urlencoded');
+
+        // After the redirect-to-dashboard the session has been
+        // privilege-elevated; the slot is gone and the next safe
+        // request through CsrfMiddleware mints a fresh token.
+        self::assertArrayNotHasKey(CsrfMiddleware::SESSION_KEY, $_SESSION);
+    }
+
     public function testCsrfMissingIs403(): void
     {
         $this->request('GET', '/login');

+ 42 - 0
ui/tests/Unit/Auth/SessionManagerTest.php

@@ -147,6 +147,48 @@ final class SessionManagerTest extends TestCase
         $sm->clear();
     }
 
+    public function testRegenerateIdRotatesCsrfTokenInCliMode(): void
+    {
+        // SEC_REVIEW F40: a CSRF token minted before a privilege boundary
+        // (e.g. the pre-auth /login form's `_csrf` slot) must NOT carry
+        // over once the session id rotates. CsrfMiddleware lazily mints
+        // a fresh token on the next request when `_csrf` is missing, so
+        // unsetting the slot at rotate-time is enough.
+        $sm = new SessionManager(
+            secureCookie: false,
+            idleSeconds: 28800,
+            absoluteSeconds: 86400,
+            cliFallback: true,
+            headersSentFn: static fn (): bool => true,
+        );
+        $sm->startSession();
+        $_SESSION['_csrf'] = 'pre-auth-token-deadbeef';
+
+        $sm->regenerateId();
+
+        self::assertArrayNotHasKey('_csrf', $_SESSION);
+    }
+
+    public function testRegenerateIdRotatesCsrfTokenInHttpMode(): void
+    {
+        // F40 mirror for the non-CLI branch: when headers haven't been
+        // sent, `regenerateId()` calls `session_regenerate_id(true)` and
+        // must still drop `_csrf`.
+        $sm = new SessionManager(
+            secureCookie: false,
+            idleSeconds: 28800,
+            absoluteSeconds: 86400,
+            cliFallback: false,
+            headersSentFn: static fn (): bool => false,
+        );
+        $sm->startSession();
+        $_SESSION['_csrf'] = 'pre-auth-token-cafebabe';
+
+        $sm->regenerateId();
+
+        self::assertArrayNotHasKey('_csrf', $_SESSION);
+    }
+
     public function testCliFallbackRotatesIdAndPreservesSession(): void
     {
         // In CLI/test mode, `regenerateId()` rotates the session id via the