fix: the login form carries a request token too

src/Infrastructure/Crypt/Csrf.php

@@ -55,40 +55,66 @@ public function __construct(
     }
 
     /**
-     * Check for CSRF token on POST requests
+     * Check for CSRF token on state-changing requests
+     *
+     * This used to begin with `isLoggedIn()`, and `initialize()` below minted a token only for a
+     * session that was already authenticated — so every request made before signing in was
+     * unprotected, the sign-in itself above all.
+     *
+     * That is login CSRF, and it is not a theoretical one here. A cross-site form posting
+     * `user` and `pass` to `login/login` signs the victim's browser into the *attacker's* account:
+     * `analyzeEncrypted()` falls back to the raw value when what it got is not RSA ciphertext, so
+     * the attacker does not even need the installation's public key. SameSite does not help — it
+     * governs whether an existing cookie is sent, and this attack does not need the victim's
+     * cookie, it needs the response to set a new one. The victim then goes on filing accounts and
+     * passwords into a vault the attacker can open.
+     *
+     * A session with no token now fails a state-changing request rather than passing it. That is
+     * what makes the check mean anything before sign-in: a browser that has never loaded a page of
+     * ours has no token, which is exactly the request that should be refused.
      */
     public function check(): bool
     {
         $method = $this->request->getMethod();
         $with = $this->request->getHeader('X-Requested-With');
 
-        if ($this->context->isLoggedIn()
-            && ($sessionToken = $this->context->getCSRF()) !== null
-            && ($method === Method::POST
-                || ($method === Method::GET && $with === 'XMLHttpRequest'))
-        ) {
-            $token = $this->request->getHeader('X-CSRF');
+        $changesState = $method === Method::POST
+                        || ($method === Method::GET && $with === 'XMLHttpRequest');
 
-            if (empty($token) || !hash_equals($sessionToken, $token)) {
-                logger('Invalid CSRF token', 'ERROR');
+        if (!$changesState) {
+            return true;
+        }
+
+        $sessionToken = $this->context->getCSRF();
 
-                return false;
-            }
+        if ($sessionToken === null) {
+            logger('No CSRF token for this session', 'ERROR');
 
-            logger('CSRF token OK');
+            return false;
         }
 
+        $token = $this->request->getHeader('X-CSRF');
+
+        if (empty($token) || !hash_equals($sessionToken, $token)) {
+            logger('Invalid CSRF token', 'ERROR');
+
+            return false;
+        }
+
+        logger('CSRF token OK');
+
         return true;
     }
 
     /**
      * Initialize the CSRF token
+     *
+     * For any session, not only an authenticated one. The token has to exist before the request
+     * that needs it, and the request that needs it most is the sign-in.
      */
     public function initialize(): void
     {
-        if ($this->context->isLoggedIn()
-            && $this->context->getCSRF() === null
-        ) {
+        if ($this->context->getCSRF() === null) {
             $this->context->setCSRF(bin2hex(random_bytes(self::TOKEN_BYTES)));
 
             logger('CSRF token set');

tests/Integration/Infrastructure/Adapter/In/Web/Controllers/UserProfile/UserProfileTest.php

@@ -549,6 +549,9 @@ private function contextWithActorProfile(ProfileData $actorProfile, bool $isAdmi
     {
         $context = self::createStub(SessionContext::class);
         $context->method('isLoggedIn')->willReturn(true);
+        // A real session holds a request token; without one the guard has nothing to check
+        // against, which is the state it now refuses rather than waves through.
+        $context->method('getCSRF')->willReturn(self::CSRF_TOKEN);
         $context->method('getAuthCompleted')->willReturn(true);
         $context->method('getUserData')->willReturn($this->getUserDataDto()->mutate(['isAdminApp' => $isAdminApp]));
         $context->method('getUserProfile')->willReturn($actorProfile);

tests/Integration/Infrastructure/Adapter/In/Web/Controllers/UserSettingsGeneral/UserSettingsGeneralTest.php

@@ -88,6 +88,9 @@ protected function getContext(): SessionContext|Stub
     {
         $context = self::createStub(SessionContext::class);
         $context->method('isLoggedIn')->willReturn(true);
+        // A real session holds a request token; without one the guard has nothing to check
+        // against, which is the state it now refuses rather than waves through.
+        $context->method('getCSRF')->willReturn(self::CSRF_TOKEN);
         $context->method('getAuthCompleted')->willReturn(true);
         $context->method('getUserData')->willReturn($this->getUserDataDto());
         $context->method('getUserProfile')->willReturn($this->getUserProfile());

tests/Unit/Infrastructure/Crypt/CsrfTest.php

@@ -59,9 +59,8 @@ public static function httpMethodDataProvider(): array
     public function testInitialize()
     {
         $this->sessionContext
-            ->expects(self::once())
-            ->method('isLoggedIn')
-            ->willReturn(true);
+            ->expects(self::never())
+            ->method('isLoggedIn');
 
         $this->sessionContext
             ->expects(self::once())
@@ -94,7 +93,6 @@ public function testInitializeIssuesADifferentTokenEachTime()
     {
         $tokens = [];
 
-        $this->sessionContext->method('isLoggedIn')->willReturn(true);
         $this->sessionContext->method('getCSRF')->willReturn(null);
         $this->sessionContext
             ->expects(self::exactly(2))
@@ -133,11 +131,6 @@ public function testCheckWithValidToken(Method $method, string $header)
             ->with(...self::withConsecutive(['X-Requested-With'], ['X-CSRF']))
             ->willReturn($header, $sessionToken);
 
-        $this->sessionContext
-            ->expects(self::once())
-            ->method('isLoggedIn')
-            ->willReturn(true);
-
         $this->sessionContext
             ->expects(self::once())
             ->method('getCSRF')
@@ -172,11 +165,6 @@ public function testCheckIsBoundToTheSessionTokenOnly(Method $method, string $he
             ->expects(self::never())
             ->method('getClientAddress');
 
-        $this->sessionContext
-            ->expects(self::once())
-            ->method('isLoggedIn')
-            ->willReturn(true);
-
         $this->sessionContext
             ->expects(self::once())
             ->method('getCSRF')
@@ -202,11 +190,6 @@ public function testCheckWithNoToken(Method $method, string $header)
             ->with(...self::withConsecutive(['X-Requested-With'], ['X-CSRF']))
             ->willReturn($header, '');
 
-        $this->sessionContext
-            ->expects(self::once())
-            ->method('isLoggedIn')
-            ->willReturn(true);
-
         $this->sessionContext
             ->expects(self::once())
             ->method('getCSRF')
@@ -216,54 +199,64 @@ public function testCheckWithNoToken(Method $method, string $header)
     }
 
     /**
-     * @return void
+     * A request that changes nothing is not checked.
+     *
+     * A plain GET without the ajax header is a page being read, and the token cannot travel on one
+     * — there is no header to put it in.
      */
-    public function testCheckWithNoLogin()
+    public function testCheckLetsAPlainReadThrough()
     {
-        $this->requestInterface
-            ->expects(self::once())
-            ->method('getMethod')
-            ->willReturn(Method::GET);
-
+        $this->requestInterface->expects(self::once())->method('getMethod')->willReturn(Method::GET);
         $this->requestInterface
             ->expects(self::once())
             ->method('getHeader')
             ->with('X-Requested-With')
-            ->willReturn('test');
+            ->willReturn('');
 
-        $this->sessionContext
-            ->expects(self::once())
-            ->method('isLoggedIn')
-            ->willReturn(false);
+        $this->sessionContext->expects(self::never())->method('getCSRF');
 
         self::assertTrue($this->csrf->check());
     }
 
     /**
-     * @return void
+     * A session with no token fails a state-changing request rather than passing it.
+     *
+     * This is the case that used to be waved through, and with it every request made before
+     * signing in — the sign-in itself above all, which is login CSRF: a cross-site form posting a
+     * username and password signs the victim's browser into the attacker's account, and they go on
+     * filing passwords into a vault the attacker can open. A browser that has never loaded a page
+     * of ours holds no token, which is exactly the request to refuse.
      */
-    public function testCheckWithNoCsrf()
+    #[DataProvider('httpMethodDataProvider')]
+    public function testCheckRefusesAStateChangingRequestWithNoSessionToken(Method $method, string $header)
     {
-        $this->requestInterface
-            ->expects(self::once())
-            ->method('getMethod')
-            ->willReturn(Method::GET);
-
+        $this->requestInterface->expects(self::once())->method('getMethod')->willReturn($method);
         $this->requestInterface
             ->expects(self::once())
             ->method('getHeader')
             ->with('X-Requested-With')
-            ->willReturn('test');
+            ->willReturn($header);
 
-        $this->sessionContext
-            ->expects(self::once())
-            ->method('isLoggedIn')
-            ->willReturn(true);
+        $this->sessionContext->expects(self::once())->method('getCSRF')->willReturn(null);
 
-        $this->sessionContext
-            ->expects(self::once())
-            ->method('getCSRF')
-            ->willReturn(null);
+        self::assertFalse($this->csrf->check());
+    }
+
+    /**
+     * Being signed in is not what decides it, either way. The token is the whole check.
+     */
+    #[DataProvider('httpMethodDataProvider')]
+    public function testCheckDoesNotAskWhetherTheSessionIsSignedIn(Method $method, string $header)
+    {
+        $token = str_repeat('a', 64);
+
+        $this->requestInterface->method('getMethod')->willReturn($method);
+        $this->requestInterface
+            ->method('getHeader')
+            ->willReturnMap([['X-Requested-With', $header], ['X-CSRF', $token]]);
+
+        $this->sessionContext->expects(self::never())->method('isLoggedIn');
+        $this->sessionContext->method('getCSRF')->willReturn($token);
 
         self::assertTrue($this->csrf->check());
     }