From 636e04827ace6162b69d4825c3a0143a77c494b3 Mon Sep 17 00:00:00 2001 From: blaipr Date: Mon, 24 Aug 2026 00:57:20 +0200 Subject: [PATCH 1/2] fix: a password reset request answers the same either way MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The forgot-my-password endpoint needs no session, so whatever it distinguishes it distinguishes for anybody. It answered four different things: User not found the login does not exist Wrong data it does, but that is not the address on file Unable to reset the password it does, address matches, account disabled or LDAP Request sent it worked That is an oracle over the whole user table, offered unauthenticated: whether a login exists, which address belongs to it — by trying addresses against a login that answers "Wrong data" rather than "User not found" — and whether the account is usable. Tracking rate-limits the guessing; it does not make the answers the same. Half of this had already been closed. The disabled and LDAP refusals were deliberately collapsed into one message, and the test pinning them says why: "the reply does not say which of the two applied, so an unauthenticated caller learns nothing about the account from asking". Collapsing two of the four still left the first question answerable. This finishes it — every outcome now answers "Request sent". The sibling settles the shape rather than it being invented here: Login answers "Wrong login" for an unknown user and a wrong password alike. The rate limit stays distinguishable on purpose. "Attempts exceeded" is about the caller's own behaviour, reveals nothing about any account, and hiding it would leave somebody who had locked themselves out with no way to find out why. What is lost is the message telling an honest user they mistyped their address, and the one telling a disabled user to contact an administrator. Both are recoverable; an enumerable user list is not. The real outcome is still recorded — the exception event still fires and the tracking entry is still added — so an administrator can see what happened and a stranger cannot. Two tests were added beyond updating the existing three: a login that does not exist, which is the half that was still open, and one that the request which succeeds still sends the mail. That second one matters because a response saying "Request sent" whatever happened is also what an endpoint that had quietly stopped sending anything would produce, and the failure path here is deliberately swallowed — so it asserts the mail, at the address on the account, rather than the body. Checked by putting the distinguishable answer back: all four refusal tests fail, and the two success ones do not. --- .../UserPassReset/SaveRequestController.php | 52 ++++++-- .../UserPassReset/SaveRequestRefusalsTest.php | 113 +++++++++++++++--- 2 files changed, 137 insertions(+), 28 deletions(-) diff --git a/src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestController.php b/src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestController.php index 9440eee85..0cc3f25b5 100644 --- a/src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestController.php +++ b/src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestController.php @@ -45,6 +45,32 @@ final class SaveRequestController extends UserPassResetSaveBase { /** + * Ask for a password-recovery link. + * + * Every outcome answers the same thing. This endpoint needs no session, so whatever it + * distinguishes it distinguishes for anybody: it used to answer "User not found" for a login + * that does not exist, "Wrong data" for one that does with the wrong address, "Unable to reset + * the password" for a disabled or LDAP account, and "Request sent" when it worked. That is an + * oracle over the whole user table — whether a login exists, which address is on file for it, + * and whether it is usable — offered to an unauthenticated caller, one request at a time. + * + * Half of it had already been closed: the disabled and LDAP refusals were collapsed into one + * message for exactly this reason, and the test that pinned them says so. Collapsing two of + * four still left the first question answerable, so this finishes it. + * + * The sibling gets it right and settles the shape: Login answers "Wrong login" for an unknown + * user and for a wrong password alike, rather than saying which. + * + * The rate limit is deliberately still distinguishable. "Attempts exceeded" is about the + * caller's own behaviour and reveals nothing about any account, and hiding it would leave + * somebody who has locked themselves out with no way to find out why. + * + * What is lost is the message telling an honest user they mistyped their address, and the one + * telling a disabled user to contact an administrator. Both are recoverable — the address is + * theirs to check, and a disabled account is a conversation with an administrator either way — + * whereas an enumerable user list is not. The real outcome is still recorded in the event log, + * where an administrator can see it and a stranger cannot. + * * @return ActionResponse */ #[Action(ResponseType::JSON)] @@ -52,10 +78,18 @@ public function saveRequestAction(): ActionResponse { try { $this->checkTracking(); + } catch (Exception $e) { + processException($e); + + $this->eventDispatcher->notify(new Event('exception', $e)); + + return ActionResponse::error($e->getMessage()); + } - $login = $this->request->analyzeString('login'); - $email = $this->request->analyzeEmail('email'); + $login = $this->request->analyzeString('login'); + $email = $this->request->analyzeEmail('email'); + try { $userData = $this->userService->getByLogin($login); if ($userData->getEmail() !== $email) { @@ -85,19 +119,19 @@ public function saveRequestAction(): ActionResponse $email, UserPassRecover::getMailMessage($hash, $this->uriContext->getWebUri()) ); - - return ActionResponse::ok( - __u('Request sent'), - [__u('You will receive an email to complete the request shortly.')] - ); } catch (Exception $e) { + // Recorded and counted, not reported. The tracking still runs, so guessing is still + // rate limited; only the answer the guesser gets back is the same either way. processException($e); $this->addTracking(); $this->eventDispatcher->notify(new Event('exception', $e)); - - return ActionResponse::error($e->getMessage()); } + + return ActionResponse::ok( + __u('Request sent'), + [__u('You will receive an email to complete the request shortly.')] + ); } } diff --git a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestRefusalsTest.php b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestRefusalsTest.php index c51108f4e..18474ff86 100644 --- a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestRefusalsTest.php +++ b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestRefusalsTest.php @@ -34,6 +34,7 @@ use SP\Domain\Common\Dtos\QueryResult; use SP\Domain\User\Models\User; use SP\Tests\Support\BodyChecker; +use SP\Application\Notification\Ports\MailService; use SP\Tests\Support\IntegrationTestCase; /** @@ -57,7 +58,7 @@ class SaveRequestRefusalsTest extends IntegrationTestCase * @throws NotFoundExceptionInterface */ #[Test] - #[BodyChecker('outputCheckerWrongData')] + #[BodyChecker('outputCheckerIndistinguishable')] public function anAddressThatIsNotTheAccountsIsRefused() { $this->givenAUser(['email' => self::REGISTERED_EMAIL]); @@ -73,7 +74,7 @@ public function anAddressThatIsNotTheAccountsIsRefused() * @throws NotFoundExceptionInterface */ #[Test] - #[BodyChecker('outputCheckerContactAdministrator')] + #[BodyChecker('outputCheckerIndistinguishable')] public function aDisabledAccountIsRefused() { $this->givenAUser(['email' => self::REGISTERED_EMAIL, 'isDisabled' => true]); @@ -90,7 +91,7 @@ public function aDisabledAccountIsRefused() * @throws NotFoundExceptionInterface */ #[Test] - #[BodyChecker('outputCheckerContactAdministrator')] + #[BodyChecker('outputCheckerIndistinguishable')] public function anLdapAccountIsRefused() { $this->givenAUser(['email' => self::REGISTERED_EMAIL, 'isLdap' => true]); @@ -98,6 +99,84 @@ public function anLdapAccountIsRefused() $this->whenRequesting('someone', self::REGISTERED_EMAIL); } + /** + * A login nobody has is answered exactly like one that exists. + * + * This is the half that was still open. The disabled and LDAP refusals had already been + * collapsed into a single message for this reason; an unknown login still answered "User not + * found" while a known one with the wrong address answered "Wrong data", so the first question + * anybody would ask — does this login exist — was answerable by anyone, unauthenticated. + * + * @throws ContainerExceptionInterface + * @throws Exception + * @throws NotFoundExceptionInterface + */ + #[Test] + #[BodyChecker('outputCheckerIndistinguishable')] + public function aLoginThatDoesNotExistIsAnsweredLikeOneThatDoes() + { + $this->givenNoSuchUser(); + + $this->whenRequesting('nobody', self::REGISTERED_EMAIL); + } + + /** + * And so is a request that actually works, or the answer would separate the successes from + * everything else instead — which tells a caller just as much. + * + * @throws ContainerExceptionInterface + * @throws Exception + * @throws NotFoundExceptionInterface + */ + #[Test] + #[BodyChecker('outputCheckerIndistinguishable')] + public function aRequestThatSucceedsIsAnsweredTheSameWay() + { + $this->givenAUser(['email' => self::REGISTERED_EMAIL]); + + $this->whenRequesting('someone', self::REGISTERED_EMAIL); + } + + /** + * The request that works still sends the mail. + * + * Every outcome now answers the same string, which is the point — but a response that says + * "Request sent" whatever happened is also exactly what an endpoint that had quietly stopped + * sending anything would produce. Nothing else here would notice, because the failure path is + * deliberately swallowed. So this asserts the mail itself, at the address on the account. + * + * @throws ContainerExceptionInterface + * @throws Exception + * @throws NotFoundExceptionInterface + */ + #[Test] + #[BodyChecker('outputCheckerIndistinguishable')] + public function aRequestThatSucceedsStillSendsTheMail() + { + $this->givenAUser(['email' => self::REGISTERED_EMAIL]); + + $sentTo = []; + + $mailService = $this->createStub(MailService::class); + $mailService->method('send')->willReturnCallback( + static function (string $subject, string $to) use (&$sentTo): void { + $sentTo[] = $to; + } + ); + + $this->whenRequesting('someone', self::REGISTERED_EMAIL, [MailService::class => $mailService]); + + self::assertSame([self::REGISTERED_EMAIL], $sentTo, 'the recovery mail must still go out'); + } + + /** + * No row for the login, which is what the service turns into its "User not found". + */ + private function givenNoSuchUser(): void + { + $this->addDatabaseMapperResolver(User::class, new QueryResult([])); + } + /** * @param array $properties */ @@ -114,7 +193,10 @@ private function givenAUser(array $properties): void * @throws Exception * @throws NotFoundExceptionInterface */ - private function whenRequesting(string $login, string $email): void + /** + * @param array $definitionsOverride + */ + private function whenRequesting(string $login, string $email, array $definitionsOverride = []): void { $container = $this->buildContainer( IntegrationTestCase::buildRequest( @@ -122,30 +204,23 @@ private function whenRequesting(string $login, string $email): void 'index.php', ['r' => 'userPassReset/saveRequest'], ['login' => $login, 'email' => $email] - ) + ), + $definitionsOverride ); IntegrationTestCase::runApp($container); } - private function outputCheckerWrongData(string $output): void - { - $json = json_decode($output); - - self::assertNotEquals('OK', $json->status); - self::assertSame('Wrong data', $json->description); - } - /** - * Both refusals answer identically. The reply does not say which of the two applied, so an - * unauthenticated caller learns nothing about the account from asking — which is why the - * assertion is on the exact message rather than on it merely failing. + * Every outcome is asserted against the same string, deliberately. That is the whole point of + * the change these pin: an unauthenticated caller must not be able to tell a login that does + * not exist from one that does, nor either from a request that actually sent a mail. */ - private function outputCheckerContactAdministrator(string $output): void + private function outputCheckerIndistinguishable(string $output): void { $json = json_decode($output); - self::assertNotEquals('OK', $json->status); - self::assertSame('Unable to reset the password', $json->description); + self::assertSame('OK', $json->status); + self::assertSame('Request sent', $json->description); } } From 1767f3826ce73dcf2f22456ed3255f5519dc0648 Mon Sep 17 00:00:00 2001 From: blaipr Date: Mon, 24 Aug 2026 01:04:43 +0200 Subject: [PATCH 2/2] fix: keep counting an attempt that was already over the limit The refusal test from #857 caught this: moving the tracking check into its own try block left the "attempts exceeded" path returning without calling addTracking(), so an attempt made while already blocked was no longer recorded. checkTracking() throws once the limit has been reached, and recording the attempt anyway is what makes further hammering extend the block rather than sit out a window that has stopped growing. It is counted again. The unit test that pinned the old behaviour of the failure path is updated rather than kept: it asserted the exception's message came back, which is the thing this change removes. --- .../UserPassReset/SaveRequestController.php | 5 +++++ .../In/Web/Controllers/UserPassReset/RefusalsTest.php | 11 ++++++++--- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestController.php b/src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestController.php index 0cc3f25b5..a7eb45338 100644 --- a/src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestController.php +++ b/src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestController.php @@ -81,6 +81,11 @@ public function saveRequestAction(): ActionResponse } catch (Exception $e) { processException($e); + // Still counted. checkTracking() throws once the limit is already reached, and + // recording the attempt anyway is what makes further hammering extend the block + // rather than sit out a window that stops growing. + $this->addTracking(); + $this->eventDispatcher->notify(new Event('exception', $e)); return ActionResponse::error($e->getMessage()); 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 e1657f6a8..1101c5798 100644 --- a/tests/Unit/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/RefusalsTest.php +++ b/tests/Unit/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/RefusalsTest.php @@ -139,10 +139,15 @@ public function savingAResetIsRefusedWhenAttemptsAreExceeded(): void /** * What the action does when the work behind it fails, once tracking allows it through. * + * It answers exactly what it answers when the request succeeds. This endpoint is reachable + * without a session, so anything it distinguishes it distinguishes for anybody — a failure + * reported as itself told an unauthenticated caller whether the login existed. The failure is + * still recorded and still counted against the caller; only the answer is the same. + * * @throws Exception */ #[Test] - public function savingARequestReportsAFailureBehindItRatherThanEscaping(): void + public function savingARequestReportsAFailureBehindItTheSameWayAsASuccess(): void { $request = $this->createStub(RequestService::class); $request->method('isAjax')->willReturn(false); @@ -172,8 +177,8 @@ public function savingARequestReportsAFailureBehindItRatherThanEscaping(): void $trackService ))->saveRequestAction(); - self::assertSame(ResponseStatus::ERROR, $response->status); - self::assertSame('the user could not be read', $response->subject); + self::assertSame(ResponseStatus::OK, $response->status); + self::assertSame('Request sent', $response->subject); } /**