diff --git a/CLAUDE.md b/CLAUDE.md index 4f33a67ab..380fd7e27 100644 --- a/CLAUDE.md +++ b/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 diff --git a/src/Application/Api/Services/Api.php b/src/Application/Api/Services/Api.php index 111ae84ef..ccbbdbb55 100644 --- a/src/Application/Api/Services/Api.php +++ b/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; diff --git a/src/Application/Auth/Services/Login.php b/src/Application/Auth/Services/Login.php index 70ac72447..c76587e7d 100644 --- a/src/Application/Auth/Services/Login.php +++ b/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(); } } diff --git a/src/Application/Auth/Services/LoginBase.php b/src/Application/Auth/Services/LoginBase.php index 037e9ba67..1b459db3e 100644 --- a/src/Application/Auth/Services/LoginBase.php +++ b/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 * diff --git a/src/Application/Security/Ports/TrackService.php b/src/Application/Security/Ports/TrackService.php index 692395613..50eb5e7a6 100644 --- a/src/Application/Security/Ports/TrackService.php +++ b/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 diff --git a/src/Application/Security/Services/Track.php b/src/Application/Security/Services/Track.php index 15a322c69..b794e5a6a 100644 --- a/src/Application/Security/Services/Track.php +++ b/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 $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([ diff --git a/src/Domain/Security/Ports/TrackRepository.php b/src/Domain/Security/Ports/TrackRepository.php index cf020ce63..5beb7e2bc 100644 --- a/src/Domain/Security/Ports/TrackRepository.php +++ b/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 $ids + * + * @return QueryResult + * @throws QueryException + * @throws ConstraintException + */ + public function deleteByIdBatch(array $ids): QueryResult; + /** * Clears tracks * diff --git a/src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestController.php b/src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestController.php index adf2a8c03..628fd2acd 100644 --- a/src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestController.php +++ b/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(); diff --git a/src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveResetController.php b/src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveResetController.php index 97fcb7310..93a23b253 100644 --- a/src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveResetController.php +++ b/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(); } } } diff --git a/src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/UserPassResetSaveBase.php b/src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/UserPassResetSaveBase.php index 7a4a941d9..627b66ac8 100644 --- a/src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/UserPassResetSaveBase.php +++ b/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 */ diff --git a/src/Infrastructure/Adapter/Out/Security/Repositories/Track.php b/src/Infrastructure/Adapter/Out/Security/Repositories/Track.php index 9e1e117e5..b32a30f4a 100644 --- a/src/Infrastructure/Adapter/Out/Security/Repositories/Track.php +++ b/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 $ids + * + * @return QueryResult + * @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 * diff --git a/tests/Integration/Application/Auth/BruteForceTrackingTest.php b/tests/Integration/Application/Auth/BruteForceTrackingTest.php index 12a2ebc32..c1482188d 100644 --- a/tests/Integration/Application/Auth/BruteForceTrackingTest.php +++ b/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 — diff --git a/tests/Unit/Application/Api/Services/ApiTest.php b/tests/Unit/Application/Api/Services/ApiTest.php index e61f4d0ff..4a8d8ec52 100644 --- a/tests/Unit/Application/Api/Services/ApiTest.php +++ b/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'); diff --git a/tests/Unit/Application/Security/Services/TrackTest.php b/tests/Unit/Application/Security/Services/TrackTest.php index 62cf25b4f..6e4c68b25 100644 --- a/tests/Unit/Application/Security/Services/TrackTest.php +++ b/tests/Unit/Application/Security/Services/TrackTest.php @@ -92,6 +92,118 @@ public function testCheckTracking() $this->assertFalse($this->track->checkTracking($trackRequest)); } + /** + * The attempt is recorded before the others are counted. + * + * Counting first and recording only once an attempt had failed left the whole attempt — a + * bcrypt verify on a login — between the guard and the change it guards, so a burst of attempts + * sent together all counted the same rows and all passed. With this attempt's row in place + * first, whichever of two attempts counts second sees the other one. + * + * @throws InvalidArgumentException + * @throws Exception + */ + public function testTheAttemptIsRecordedBeforeTheOthersAreCounted() + { + $calls = []; + + $this->trackRepository + ->expects($this->once()) + ->method('add') + ->willReturnCallback(function () use (&$calls) { + $calls[] = 'add'; + + return new QueryResult(null, 0, 7); + }); + + $this->trackRepository + ->expects($this->once()) + ->method('getTracksForClientFromTime') + ->willReturnCallback(function () use (&$calls) { + $calls[] = 'count'; + + return new QueryResult([1]); + }); + + $this->track->checkTracking($this->getTrackRequest()); + + $this->assertSame(['add', 'count'], $calls); + } + + /** + * And its own row is not held against it: nine earlier attempts and this one is still under the + * limit of ten, as it was when the count came first. + * + * @throws InvalidArgumentException + * @throws Exception + */ + public function testTheAttemptsOwnRowDoesNotCountAgainstIt() + { + $this->trackRepository->method('add')->willReturn(new QueryResult(null, 0, 7)); + $this->trackRepository + ->method('getTracksForClientFromTime') + ->willReturn(new QueryResult(range(1, 10))); + + $this->assertFalse($this->track->checkTracking($this->getTrackRequest())); + } + + /** + * Releasing withdraws exactly the rows this request's checks recorded, and only once. + * + * @throws InvalidArgumentException + * @throws Exception + */ + public function testReleaseWithdrawsWhatTheChecksRecorded() + { + $lastIds = [41, 42]; + + $this->trackRepository + ->method('add') + ->willReturnCallback(function () use (&$lastIds) { + return new QueryResult(null, 0, array_shift($lastIds)); + }); + $this->trackRepository->method('getTracksForClientFromTime')->willReturn(new QueryResult([1])); + + $deleted = []; + + $this->trackRepository + ->expects($this->once()) + ->method('deleteByIdBatch') + ->willReturnCallback(function (array $ids) use (&$deleted) { + $deleted[] = $ids; + + return new QueryResult(); + }); + + $this->track->checkTracking($this->getTrackRequest()); + $this->track->checkTracking($this->getTrackRequest()); + + $this->track->release(); + $this->track->release(); + + $this->assertSame([[41, 42]], $deleted); + } + + /** + * A release that fails is logged rather than raised: it runs as a request finishes, and + * throwing there would replace the answer the attempt earned. + * + * @throws InvalidArgumentException + * @throws Exception + */ + public function testAFailedReleaseDoesNotReplaceTheAnswer() + { + $this->trackRepository->method('add')->willReturn(new QueryResult(null, 0, 7)); + $this->trackRepository->method('getTracksForClientFromTime')->willReturn(new QueryResult([1])); + $this->trackRepository + ->expects($this->once()) + ->method('deleteByIdBatch') + ->willThrowException(new RuntimeException('test')); + + $this->track->checkTracking($this->getTrackRequest()); + $this->track->release(); + } + /** * @return TrackRequest * @throws InvalidArgumentException diff --git a/tests/Unit/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/RefusalsTest.php b/tests/Unit/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/RefusalsTest.php index 1101c5798..9df96fefc 100644 --- a/tests/Unit/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/RefusalsTest.php +++ b/tests/Unit/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/RefusalsTest.php @@ -86,6 +86,8 @@ public function savingARequestIsRefusedWhenAttemptsAreExceeded(): void $trackService->method('buildTrackRequest')->willReturn($this->trackRequestFor('saveRequest')); $trackService->method('checkTracking')->willReturn(true); $trackService->expects(self::once())->method('add'); + // Refused or not, what the check recorded while the attempt was in flight is withdrawn. + $trackService->expects(self::once())->method('release'); $userService = $this->createMock(UserService::class); $userService->expects(self::never())->method('getByLogin'); @@ -116,6 +118,8 @@ public function savingAResetIsRefusedWhenAttemptsAreExceeded(): void $trackService->method('buildTrackRequest')->willReturn($this->trackRequestFor('saveReset')); $trackService->method('checkTracking')->willReturn(true); $trackService->expects(self::once())->method('add'); + // Refused or not, what the check recorded while the attempt was in flight is withdrawn. + $trackService->expects(self::once())->method('release'); $userPassRecoverService = $this->createMock(UserPassRecoverService::class); $userPassRecoverService->expects(self::never())->method('getUserIdForHash');