fix(security): key the 2FA code throttle on the user, not the session

The throttle counted per session id, and a fresh password login is a fresh
session, so a stolen password bought unlimited guesses at the code. It now
listens to scheb's ATTEMPT event and keys on the pending token's user
identifier; the reset on success uses the same key.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

src/Security/TwoFactor/TwoFactorThrottle.php

@@ -7,10 +7,9 @@ namespace App\Security\TwoFactor;
 use Scheb\TwoFactorBundle\Security\TwoFactor\Event\TwoFactorAuthenticationEvent;
 use Scheb\TwoFactorBundle\Security\TwoFactor\Event\TwoFactorAuthenticationEvents;
 use Symfony\Component\EventDispatcher\Attribute\AsEventListener;
-use Symfony\Component\HttpFoundation\Request;
-use Symfony\Component\HttpKernel\Event\RequestEvent;
 use Symfony\Component\HttpKernel\Exception\TooManyRequestsHttpException;
 use Symfony\Component\RateLimiter\RateLimiterFactoryInterface;
+use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 
 /**
  * Rate limits the second factor.
@@ -39,24 +38,15 @@ final class TwoFactorThrottle
     /**
      * Refuse before the code is checked, not after.
      *
-     * A limiter consulted on failure only still lets every attempt reach the
-     * verifier; the request has to be stopped on the way in for the limit to
-     * mean anything.
+     * ATTEMPT is dispatched by scheb's authenticator immediately before the
+     * code reaches a provider, so every attempt passes through here and none
+     * reaches the verifier once the limit is spent. It also carries the
+     * half-authenticated token, which is where the key comes from.
      */
-    #[AsEventListener(event: RequestEvent::class, priority: 8)]
-    public function onRequest(RequestEvent $event): void
+    #[AsEventListener(event: TwoFactorAuthenticationEvents::ATTEMPT)]
+    public function onAttempt(TwoFactorAuthenticationEvent $event): void
     {
-        if (false === $event->isMainRequest()) {
-            return;
-        }
-
-        $request = $event->getRequest();
-
-        if ('app_2fa_login_check' !== $request->attributes->get('_route')) {
-            return;
-        }
-
-        $key = $this->keyFor($request);
+        $key = $this->keyFor($event->getToken());
 
         if (null === $key) {
             return;
@@ -65,7 +55,8 @@ final class TwoFactorThrottle
         if (false === $this->twoFactorCodeLimiter->create($key)->consume()->isAccepted()) {
             // A 429 rather than a redirect back to the form: the form would
             // invite another attempt, and this is the one answer a script
-            // cannot usefully retry.
+            // cannot usefully retry. An HTTP exception, not an authentication
+            // one, so the authenticator's failure handler never sees it.
             throw new TooManyRequestsHttpException(
                 null,
                 'Too many two-factor attempts. Try again later.',
@@ -83,7 +74,7 @@ final class TwoFactorThrottle
     #[AsEventListener(event: TwoFactorAuthenticationEvents::SUCCESS)]
     public function onSuccess(TwoFactorAuthenticationEvent $event): void
     {
-        $key = $this->keyFor($event->getRequest());
+        $key = $this->keyFor($event->getToken());
 
         if (null === $key) {
             return;
@@ -93,26 +84,26 @@ final class TwoFactorThrottle
     }
 
     /**
-     * The session stands in for the user identity.
+     * The pending user's identifier, never the session.
      *
-     * The code form runs before authentication completes, so there is no
-     * settled user on the request — but the half-authenticated state is pinned
-     * to the session, which is as good a key here and does not require reaching
-     * into token storage from a request listener.
+     * This used to key on the session id, which is the one thing the attacker
+     * controls: a fresh password login is a fresh session and a fresh five
+     * guesses, so a stolen password bought unlimited attempts at the code. The
+     * user identifier on the two-factor token is the account being attacked,
+     * and it survives any number of re-logins.
      *
-     * Both callers derive the key the same way on purpose: consuming under the
-     * session and resetting under the username would mean the reset never
-     * cleared what the consume counted, and the limit would look like it worked
-     * while never actually being lifted.
+     * Both callers derive the key the same way on purpose: consuming under one
+     * key and resetting under another would mean the reset never cleared what
+     * the consume counted.
      */
-    private function keyFor(Request $request): ?string
+    private function keyFor(TokenInterface $token): ?string
     {
-        $sessionId = $request->hasSession() ? $request->getSession()->getId() : null;
+        $identifier = $token->getUserIdentifier();
 
-        if (null === $sessionId || '' === $sessionId) {
+        if ('' === $identifier) {
             return null;
         }
 
-        return 'session:' . $sessionId;
+        return 'user:' . mb_strtolower($identifier);
     }
 }

tests/Security/TwoFactor/TwoFactorThrottleTest.php

@@ -0,0 +1,118 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Security\TwoFactor;
+
+use App\Entity\User\User;
+use App\Service\User\TwoFactor\TwoFactorEnrolment;
+use Doctrine\ORM\EntityManagerInterface;
+use OTPHP\TOTP;
+use Symfony\Bundle\FrameworkBundle\KernelBrowser;
+use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
+use Symfony\Component\PasswordHasher\Hasher\UserPasswordHasherInterface;
+
+/**
+ * The code throttle belongs to the account, not to the session.
+ *
+ * It used to be keyed on the session id, so signing in with the password again
+ * — a new session — handed a stolen password another five guesses, as many
+ * times as the attacker cared to log in. Driven through the real forms because
+ * the key is decided by what survives between two logins.
+ */
+final class TwoFactorThrottleTest extends WebTestCase
+{
+    private const string PASSWORD = 'correct-horse-battery-staple';
+
+    private KernelBrowser $client;
+    private EntityManagerInterface $em;
+    private User $user;
+
+    protected function setUp(): void
+    {
+        $this->client = static::createClient();
+        $this->client->disableReboot();
+
+        $this->em = static::getContainer()->get(EntityManagerInterface::class);
+        $this->em->getConnection()->beginTransaction();
+
+        $this->user = $this->enrolledUser();
+    }
+
+    protected function tearDown(): void
+    {
+        if (true === $this->em->getConnection()->isTransactionActive()) {
+            $this->em->getConnection()->rollBack();
+        }
+
+        parent::tearDown();
+    }
+
+    public function testANewSessionForTheSameUserIsStillThrottled(): void
+    {
+        // The limit is five, spent across two sessions. Split three and two
+        // because five wrong codes also trip the password form's own
+        // login_throttling for this username and address, which would stop the
+        // second login before the code form is reached — an attacker changes
+        // address, so that is not the protection under test.
+        $this->signInWithPassword();
+        $this->wrongCodes(3);
+
+        // Start over as the attacker would: a fresh session, the password again.
+        $this->client->getCookieJar()->clear();
+        $this->signInWithPassword();
+        $this->wrongCodes(2);
+
+        $this->client->request('POST', '/2fa_check', ['_auth_code' => '000000']);
+
+        self::assertSame(429, $this->client->getResponse()->getStatusCode(), 'a new session reset the code throttle');
+    }
+
+    private function wrongCodes(int $count): void
+    {
+        for ($i = 0; $i < $count; ++$i) {
+            $this->client->request('POST', '/2fa_check', ['_auth_code' => '000000']);
+            self::assertNotSame(429, $this->client->getResponse()->getStatusCode(), 'throttled before the limit');
+        }
+    }
+
+    private function signInWithPassword(): void
+    {
+        $crawler = $this->client->request('GET', '/login');
+        $token   = (string) $crawler->filter('input[name="_csrf_token"]')->attr('value');
+
+        $this->client->followRedirects(true);
+
+        try {
+            $this->client->request('POST', '/login', [
+                'email'       => $this->user->email,
+                'password'    => self::PASSWORD,
+                '_csrf_token' => $token,
+            ]);
+        } finally {
+            $this->client->followRedirects(false);
+        }
+
+        self::assertSame('/2fa', parse_url($this->client->getRequest()->getUri(), PHP_URL_PATH), 'expected the code prompt');
+    }
+
+    private function enrolledUser(): User
+    {
+        $user            = new User();
+        $user->email     = 'throttle-'.bin2hex(random_bytes(6)).'@plmail.test';
+        $user->nameFirst = 'Two';
+        $user->nameLast  = 'Factor';
+        $user->password  = static::getContainer()
+            ->get(UserPasswordHasherInterface::class)
+            ->hashPassword($user, self::PASSWORD);
+
+        $this->em->persist($user);
+        $this->em->flush();
+
+        $enrolment = static::getContainer()->get(TwoFactorEnrolment::class);
+        $enrolment->begin($user);
+        $enrolment->confirm($user, TOTP::create($user->totpSecret, 30, 'sha1', 6)->now());
+
+        return $user;
+    }
+}