Просмотр исходного кода

fix: fail-closed on session id rotation when headers already sent (SEC_REVIEW F8)

Both SessionManager::regenerateId() and clear() previously short-circuited
silently when headers_sent() returned true. In HTTP that left the
pre-auth `irdb_session` cookie valid post-login -- classic session
fixation, since an attacker who pre-seeded a victim's cookie could ride
the same session after authentication completed.

Both methods now check an injected headers_sent closure (defaulting to
the real `headers_sent()`); when headers are sent and cliFallback is
false (HTTP), they throw RuntimeException -- Slim surfaces this as a
500 so the operator chases the upstream output bug rather than a silent
auth-state-change-without-id-rotation. Under cliFallback=true (PHPUnit /
PHP_SAPI === 'cli'), a manual rotation path resets session_id() while
preserving $_SESSION so test assertions about authenticated state hold.

cliFallback auto-detects from PHP_SAPI by default; production wiring
needs no change. The headers_sent closure is wired so unit tests can
exercise the HTTP fail-closed path without launching a real web server.

Regression coverage in SessionManagerTest:
- testRegenerateIdThrowsInHttpModeWhenHeadersSent
- testClearThrowsInHttpModeWhenHeadersSent
- testCliFallbackRotatesIdAndPreservesSession

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa 5 дней назад
Родитель
Сommit
f811b25734
2 измененных файлов с 121 добавлено и 3 удалено
  1. 56 3
      ui/src/Auth/SessionManager.php
  2. 65 0
      ui/tests/Unit/Auth/SessionManagerTest.php

+ 56 - 3
ui/src/Auth/SessionManager.php

@@ -17,6 +17,15 @@ namespace App\Auth;
  *
  * `regenerateId()` MUST be called after any auth-state change (login
  * success, logout) to defeat session fixation.
+ *
+ * SEC_REVIEW F8: `regenerateId()` and `clear()` previously short-circuited
+ * silently when `headers_sent()` was true. That left the pre-auth cookie
+ * intact post-login — classic session fixation. Both methods now
+ * fail-closed under HTTP (throw `\RuntimeException`, surfaced by Slim as
+ * a 500 so the operator notices the upstream output bug) and use a manual
+ * id-rotation path under CLI/test (`$cliFallback === true`) where
+ * `headers_sent()` is structurally always true and there's no real cookie
+ * to send.
  */
 class SessionManager
 {
@@ -29,11 +38,20 @@ class SessionManager
     private const KEY_NEXT = '_next';
     private const KEY_OIDC = '_oidc';
 
+    private readonly bool $cliFallback;
+
+    /** @var \Closure(): bool */
+    private \Closure $headersSentFn;
+
     public function __construct(
         private readonly bool $secureCookie = true,
         private readonly int $idleSeconds = 28800,
         private readonly int $absoluteSeconds = 86400,
+        ?bool $cliFallback = null,
+        ?\Closure $headersSentFn = null,
     ) {
+        $this->cliFallback = $cliFallback ?? (PHP_SAPI === 'cli');
+        $this->headersSentFn = $headersSentFn ?? static fn (): bool => headers_sent();
     }
 
     public function startSession(): void
@@ -41,7 +59,7 @@ class SessionManager
         if (session_status() === PHP_SESSION_ACTIVE) {
             return;
         }
-        if (headers_sent()) {
+        if (($this->headersSentFn)()) {
             // Tests run without HTTP — fall back to a manual session id so
             // SessionManager remains usable in CLI tests.
             if (session_id() === '') {
@@ -69,9 +87,17 @@ class SessionManager
         if (session_status() !== PHP_SESSION_ACTIVE) {
             return;
         }
-        if (!headers_sent()) {
+        if (!($this->headersSentFn)()) {
             session_regenerate_id(true);
+
+            return;
         }
+        if (!$this->cliFallback) {
+            throw new \RuntimeException(
+                'cannot regenerate session id: response headers already sent',
+            );
+        }
+        $this->rotateIdUnderCli();
     }
 
     public function setUser(UserContext $user): void
@@ -101,9 +127,20 @@ class SessionManager
     public function clear(): void
     {
         $_SESSION = [];
-        if (session_status() === PHP_SESSION_ACTIVE && !headers_sent()) {
+        if (session_status() !== PHP_SESSION_ACTIVE) {
+            return;
+        }
+        if (!($this->headersSentFn)()) {
             session_regenerate_id(true);
+
+            return;
         }
+        if (!$this->cliFallback) {
+            throw new \RuntimeException(
+                'cannot regenerate session id on clear(): response headers already sent',
+            );
+        }
+        $this->rotateIdUnderCli();
     }
 
     public function flash(string $type, string $message): void
@@ -181,6 +218,22 @@ class SessionManager
         return $out;
     }
 
+    /**
+     * CLI/test rotation: PHP can't send Set-Cookie because there is no
+     * real HTTP response, but the in-process session id is still rotated
+     * so test assertions about "session was renewed" hold. The current
+     * `$_SESSION` contents are preserved so flash/auth state survives
+     * the rotation (matching `session_regenerate_id(true)` semantics).
+     */
+    private function rotateIdUnderCli(): void
+    {
+        $data = $_SESSION;
+        @session_write_close();
+        session_id(bin2hex(random_bytes(16)));
+        @session_start();
+        $_SESSION = $data;
+    }
+
     /**
      * Drop the session if it's been idle past `idleSeconds` or older than
      * `absoluteSeconds` since auth. SPEC §M08.2.

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

@@ -106,6 +106,71 @@ final class SessionManagerTest extends TestCase
         self::assertNull($sm->getUser());
     }
 
+    public function testRegenerateIdThrowsInHttpModeWhenHeadersSent(): void
+    {
+        // SEC_REVIEW F8: in HTTP mode (cliFallback=false), `regenerateId()`
+        // must NOT silently no-op when headers are already sent — that would
+        // leave a pre-auth cookie valid post-login (classic session
+        // fixation). It must fail-closed by throwing; Slim surfaces this as
+        // a 500 and the operator chases the upstream output bug.
+        $sm = new SessionManager(
+            secureCookie: false,
+            idleSeconds: 28800,
+            absoluteSeconds: 86400,
+            cliFallback: false,
+            headersSentFn: static fn (): bool => true,
+        );
+        $sm->startSession();
+
+        $this->expectException(\RuntimeException::class);
+        $this->expectExceptionMessage('headers already sent');
+        $sm->regenerateId();
+    }
+
+    public function testClearThrowsInHttpModeWhenHeadersSent(): void
+    {
+        // F8 mirror: `clear()` (used by logout) must also fail-closed
+        // rather than silently leaving the old session id valid.
+        $sm = new SessionManager(
+            secureCookie: false,
+            idleSeconds: 28800,
+            absoluteSeconds: 86400,
+            cliFallback: false,
+            headersSentFn: static fn (): bool => true,
+        );
+        $sm->startSession();
+        $sm->setUser(new UserContext(1, 'Alice', 'admin', null, UserContext::SOURCE_LOCAL));
+
+        $this->expectException(\RuntimeException::class);
+        $this->expectExceptionMessage('headers already sent');
+        $sm->clear();
+    }
+
+    public function testCliFallbackRotatesIdAndPreservesSession(): void
+    {
+        // In CLI/test mode, `regenerateId()` rotates the session id via the
+        // manual path and preserves the existing `$_SESSION` contents — so
+        // login-flow assertions about authenticated state survive the
+        // rotation, matching `session_regenerate_id(true)` semantics.
+        $sm = new SessionManager(
+            secureCookie: false,
+            idleSeconds: 28800,
+            absoluteSeconds: 86400,
+            cliFallback: true,
+            headersSentFn: static fn (): bool => true,
+        );
+        $sm->startSession();
+        $sm->setUser(new UserContext(7, 'Bob', 'admin', 'b@example.com', UserContext::SOURCE_LOCAL));
+        $oldId = session_id();
+
+        $sm->regenerateId();
+
+        self::assertNotSame($oldId, session_id(), 'session id was not rotated');
+        $u = $sm->getUser();
+        self::assertNotNull($u);
+        self::assertSame(7, $u->userId);
+    }
+
     private function mgr(int $idle = 28800): SessionManager
     {
         return new SessionManager(secureCookie: false, idleSeconds: $idle, absoluteSeconds: 86400);