fix: attempts in flight count against the sign-in limit

CLAUDE.md

@@ -442,6 +442,10 @@ as correct in review. A public link's view limit and the temporary master passwo
 cap were both tested in PHP against a row that had already been read, so two requests arriving
 together both passed — and the attempt counter was written back as `$attempts + 1`, an absolute
 value worked out from that same stale read, so guesses in parallel advanced it by one between them.
+The sign-in limiter had the same gap at a larger scale: it counted, ran the whole attempt (a bcrypt
+verify), and recorded a failure only at the end, so a burst sent together all passed the count. It
+now records the attempt *before* counting and withdraws that row once the attempt is decided —
+`TrackService::release()`, which every door that calls `checkTracking()` must reach in a `finally`.
 The master password's rotation re-encrypted every secret inside a transaction and then stored the
 hash describing them outside it, leaving a vault nobody could open if those last two writes failed.
 `40024210101.sql` made two commits out of one logical change, and DDL commits as it goes, so a

src/Application/Api/Services/Api.php

@@ -96,6 +96,23 @@ public function __construct(
      * @throws Exception
      */
     public function setup(int $actionId): void
+    {
+        try {
+            $this->authenticate($actionId);
+        } finally {
+            // The attempt is decided either way: a failure has recorded itself by now.
+            $this->trackService->release();
+        }
+    }
+
+    /**
+     * Authenticates the request's token for the given action
+     *
+     * @throws ServiceException
+     * @throws SPException
+     * @throws Exception
+     */
+    private function authenticate(int $actionId): void
     {
         $this->status = ApiStatuses::INITIALIZING;
 

src/Application/Auth/Services/Login.php

@@ -131,6 +131,9 @@ public function doLogin(?string $from = null): LoginResponseDto
             return new LoginResponseDto(LoginStatus::OK, $this->getUriForRoute($from ?? 'index'));
         } catch (ServiceException $e) {
             throw AuthException::from($e);
+        } finally {
+            // The attempt is decided either way: a failure has recorded itself by now.
+            $this->releaseTracking();
         }
     }
 

src/Application/Auth/Services/LoginBase.php

@@ -82,6 +82,14 @@ final protected function checkTracking(): void
         }
     }
 
+    /**
+     * Withdraw what checkTracking() recorded while the attempt was in flight
+     */
+    final protected function releaseTracking(): void
+    {
+        $this->trackService->release();
+    }
+
     /**
      * Add a tracking entry
      *

src/Application/Security/Ports/TrackService.php

@@ -64,11 +64,23 @@ public function clear(): bool;
     /**
      * Check the login attempts
      *
-     * @return bool True if delay is performed, false otherwise
+     * The attempt being checked is recorded before the others are counted, so attempts in flight
+     * at the same time count against each other. Every caller must call release() once the
+     * attempt has been decided, whether it succeeded or failed.
+     *
+     * @return bool True if the limit is exceeded, false otherwise
      * @throws Exception
      */
     public function checkTracking(TrackRequest $trackRequest): bool;
 
+    /**
+     * Withdraw what checkTracking() recorded for the attempts this request made
+     *
+     * A failed attempt is recorded by add(), so this leaves the count exactly as it would have
+     * been had the attempts been made one at a time.
+     */
+    public function release(): void;
+
     /**
      * @throws ServiceException
      * @throws ConstraintException

src/Application/Security/Services/Track.php

@@ -56,6 +56,13 @@ final class Track extends Service implements TrackService
     private const TIME_TRACKING              = 600;
     private const TIME_TRACKING_MAX_ATTEMPTS = 10;
 
+    /**
+     * The rows checkTracking() recorded for attempts this request has not yet decided
+     *
+     * @var int[]
+     */
+    private array $inFlight = [];
+
     /**
      * @param TrackRepository<TrackModel> $trackRepository
      */
@@ -112,8 +119,18 @@ public function clear(): bool
     public function checkTracking(TrackRequest $trackRequest): bool
     {
         try {
+            // Record this attempt first, then count. Counting and recording used to be two
+            // statements with the whole attempt between them — a bcrypt verify on a login, a token
+            // lookup on the API — and a failure was only recorded at the end. So a burst of
+            // attempts sent together all counted the same rows, all passed, and the ten-attempt
+            // limit let through as many guesses as there were workers to run them. With the row
+            // in place before the count, whichever of two attempts counts second sees the other:
+            // the guard and the change are no longer separate. The row is withdrawn by release()
+            // once the attempt is decided; a failure has recorded its own by then.
+            $this->inFlight[] = $this->trackRepository->add($this->buildTrackFrom($trackRequest))->getLastId();
+
             $attempts = $this->trackRepository->getTracksForClientFromTime($this->buildTrackFrom($trackRequest))
-                                              ->getNumRows();
+                                              ->getNumRows() - 1;
 
             if ($attempts >= self::TIME_TRACKING_MAX_ATTEMPTS) {
                 // Answer at once. This used to sleep for a quarter of a second per attempt
@@ -148,6 +165,29 @@ public function checkTracking(TrackRequest $trackRequest): bool
         return false;
     }
 
+    /**
+     * Withdraw the rows checkTracking() recorded for this request's attempts
+     *
+     * A failure to withdraw them is logged rather than raised: it runs as a request finishes, where
+     * throwing would replace the answer the attempt earned, and the rows it leaves behind only count
+     * against the same address until the window passes.
+     */
+    public function release(): void
+    {
+        $ids = $this->inFlight;
+        $this->inFlight = [];
+
+        if ($ids === []) {
+            return;
+        }
+
+        try {
+            $this->trackRepository->deleteByIdBatch($ids);
+        } catch (Exception $e) {
+            processException($e);
+        }
+    }
+
     private function buildTrackFrom(TrackRequest $trackRequest): TrackModel
     {
         return new TrackModel([

src/Domain/Security/Ports/TrackRepository.php

@@ -57,6 +57,17 @@ public function add(TrackModel $track): QueryResult;
      */
     public function unlock(int $id): int;
 
+    /**
+     * Delete the given tracks
+     *
+     * @param non-empty-array<int> $ids
+     *
+     * @return QueryResult<Simple>
+     * @throws QueryException
+     * @throws ConstraintException
+     */
+    public function deleteByIdBatch(array $ids): QueryResult;
+
     /**
      * Clears tracks
      *

src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestController.php

@@ -75,6 +75,19 @@ final class SaveRequestController extends UserPassResetSaveBase
      */
     #[Action(ResponseType::JSON)]
     public function saveRequestAction(): ActionResponse
+    {
+        try {
+            return $this->handleRequest();
+        } finally {
+            // The attempt is decided either way: a failure has recorded itself by now.
+            $this->releaseTracking();
+        }
+    }
+
+    /**
+     * Sends a reset link when the login and email match, answering the same either way
+     */
+    private function handleRequest(): ActionResponse
     {
         try {
             $this->checkTracking();

src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveResetController.php

@@ -91,6 +91,9 @@ public function saveResetAction(): ActionResponse
             $this->eventDispatcher->notify(new Event('exception', $e));
 
             return ActionResponse::error($e->getMessage());
+        } finally {
+            // The attempt is decided either way: a failure has recorded itself by now.
+            $this->releaseTracking();
         }
     }
 }

src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/UserPassResetSaveBase.php

@@ -85,6 +85,14 @@ final protected function checkTracking(): void
         }
     }
 
+    /**
+     * Withdraw what checkTracking() recorded while the attempt was in flight
+     */
+    final protected function releaseTracking(): void
+    {
+        $this->trackService->release();
+    }
+
     /**
      * Add a tracking entry
      */

src/Infrastructure/Adapter/Out/Security/Repositories/Track.php

@@ -88,6 +88,27 @@ public function unlock(int $id): int
         return $this->db->runQuery($queryData)->getAffectedNumRows();
     }
 
+    /**
+     * Delete the given tracks
+     *
+     * @param non-empty-array<int> $ids
+     *
+     * @return QueryResult<Simple>
+     * @throws ConstraintException
+     * @throws QueryException
+     */
+    public function deleteByIdBatch(array $ids): QueryResult
+    {
+        $query = $this->queryFactory
+            ->newDelete()
+            ->from(self::TABLE)
+            ->where('id IN (:ids)', ['ids' => $ids]);
+
+        $queryData = QueryData::build($query)->setOnErrorMessage(__u('Error while removing the track'));
+
+        return $this->db->runQuery($queryData);
+    }
+
     /**
      * Clears tracks
      *

tests/Integration/Application/Auth/BruteForceTrackingTest.php

@@ -27,10 +27,12 @@
 namespace SP\Tests\Integration\Application\Auth;
 
 use DI\ContainerBuilder;
+use Exception;
 use PHPUnit\Framework\Attributes\Group;
 use PHPUnit\Framework\TestCase;
 use Psr\Container\ContainerInterface;
 use SP\Application\Auth\Ports\LoginService;
+use SP\Application\Security\Ports\TrackService;
 use SP\Application\User\Ports\UserProfileService;
 use SP\Application\User\Ports\UserService;
 use SP\Domain\Auth\Services\AuthException;
@@ -284,6 +286,58 @@ private function createUser(string $login, string $pass): void
         );
     }
 
+    /**
+     * Attempts in flight at the same time count against each other.
+     *
+     * The check and the record used to be two statements with the whole attempt between them, and a
+     * failure was recorded only at the end — so attempts sent together all counted the same rows
+     * and all passed, and the limit let through as many guesses as there were workers. Two
+     * containers stand for two requests from one address here, interleaved the way a burst
+     * interleaves them: nine attempts already stand, the first request is checked and is still
+     * verifying when the second is checked. The second must see the first.
+     *
+     * Once both are decided the count is back to what the failures recorded, so a successful
+     * attempt costs nothing afterwards — which is what keeps an office behind one address from
+     * locking itself out by signing in.
+     *
+     * @throws Exception
+     */
+    public function testAttemptsInFlightCountAgainstEachOther(): void
+    {
+        $_SERVER['REMOTE_ADDR'] = sprintf('203.0.113.%d', random_int(1, 254));
+        $source = 'bftest-inflight-' . bin2hex(random_bytes(4));
+
+        $earlier = $this->buildContainer()->get(TrackService::class);
+
+        for ($attempt = 1; $attempt <= 9; $attempt++) {
+            $earlier->add($earlier->buildTrackRequest($source));
+        }
+
+        $first = $this->buildContainer()->get(TrackService::class);
+        $second = $this->buildContainer()->get(TrackService::class);
+
+        self::assertFalse(
+            $first->checkTracking($first->buildTrackRequest($source)),
+            'nine earlier attempts are under the limit of ten'
+        );
+        self::assertTrue(
+            $second->checkTracking($second->buildTrackRequest($source)),
+            'an attempt checked while another is in flight did not count it'
+        );
+
+        $first->release();
+        $second->release();
+
+        $after = $this->buildContainer()->get(TrackService::class);
+
+        self::assertFalse(
+            $after->checkTracking($after->buildTrackRequest($source)),
+            'attempts that recorded no failure still counted once they were decided'
+        );
+
+        $after->release();
+    }
+
     /**
      * Drives one sign-in attempt through the real login service, in a fresh container so its
      * SymfonyRequest (resolved once per container) picks up the REMOTE_ADDR/user/pass given here —

tests/Unit/Application/Api/Services/ApiTest.php

@@ -352,6 +352,9 @@ public function testSetup()
                                  ->with($userData->getUserProfileId())
                                  ->willReturn(UserProfileDataGenerator::factory()->buildUserProfileData());
 
+        // The attempt is decided, so what checkTracking() recorded while it was in flight goes.
+        $this->trackService->expects(self::once())->method('release');
+
         $this->apiService->setup($actionId);
     }
 
@@ -398,6 +401,10 @@ public function testSetupAttemptsExceeded()
             ->with($this->trackRequest)
             ->willReturn(true);
 
+        // Refused attempts are recorded by add(); the in-flight row is withdrawn all the same.
+        $this->trackService->expects(self::once())->method('add')->with($this->trackRequest);
+        $this->trackService->expects(self::once())->method('release');
+
         $this->expectException(ServiceException::class);
         $this->expectExceptionMessage('Attempts exceeded');