diff --git a/src/Application/User/Services/UserPassRecover.php b/src/Application/User/Services/UserPassRecover.php index 737744236..4a41a5fb1 100644 --- a/src/Application/User/Services/UserPassRecover.php +++ b/src/Application/User/Services/UserPassRecover.php @@ -29,6 +29,8 @@ use SP\Application\Application; use SP\Domain\Core\Messages\MailMessage; use SP\Domain\Common\Providers\Password; +use SP\Domain\Config\Ports\ConfigDataInterface; +use SP\Domain\Core\Bootstrap\UriContextInterface; use SP\Domain\Common\Services\Service; use SP\Domain\Common\Services\ServiceException; use SP\Domain\Core\Exceptions\ConstraintException; @@ -66,8 +68,33 @@ public function __construct( parent::__construct($application); } - public static function getMailMessage(string $hash, string $baseUri): MailMessage - { + /** + * The mail carrying a one-time reset link, and the address that link points at. + * + * The base URI is decided here rather than taken from the caller, because both callers used to + * pass `UriContextInterface::getWebUri()` — which prefers `Forwarded` / `X-Forwarded-Host`, + * headers supplied by whoever made the request, with no `setTrustedProxies()` anywhere to + * restrict them. Verified against the running instance: `X-Forwarded-Host: evil.example.com` + * comes straight back out of the application. + * + * `saveRequestAction()` needs no session, so an unauthenticated caller who knows a login and + * its email address could choose the host in the mail the real user then receives — a + * legitimate message, from the real installation, whose link hands the one-time hash to + * somebody else. + * + * Six other link builders in this application already prefer the configured application URL + * (`AccountHelper`, `ViewLinkController`, `PublicLinkViewBase`, `AccountSearchItem`, + * `Template`, `Account\SaveRequestController`); these two were the exception. The fallback is + * the unforwarded host rather than `getWebUri()`, so the header cannot choose it even when no + * application URL has been configured. + */ + public static function getMailMessage( + string $hash, + ConfigDataInterface $configData, + UriContextInterface $uriContext + ): MailMessage { + $baseUri = $configData->getApplicationUrl() ?: $uriContext->getUnforwardedWebUri(); + $mailMessage = new MailMessage(); $mailMessage->setTitle(__('Password Change')); $mailMessage->addDescription(__('A request for changing your user password has been done.')); diff --git a/src/Domain/Core/Bootstrap/UriContextInterface.php b/src/Domain/Core/Bootstrap/UriContextInterface.php index 19170f85a..fe3134d1c 100644 --- a/src/Domain/Core/Bootstrap/UriContextInterface.php +++ b/src/Domain/Core/Bootstrap/UriContextInterface.php @@ -37,6 +37,11 @@ interface UriContextInterface */ public function getWebUri(): string; + /** + * The same URI, built without consulting `Forwarded` / `X-Forwarded-*`. + */ + public function getUnforwardedWebUri(): string; + /** * The current request path relative to the application root (e.g. files/index.php) * diff --git a/src/Domain/Http/Ports/RequestService.php b/src/Domain/Http/Ports/RequestService.php index 09235bc2c..db5c1de83 100644 --- a/src/Domain/Http/Ports/RequestService.php +++ b/src/Domain/Http/Ports/RequestService.php @@ -100,6 +100,11 @@ public function verifySignature(string $key, ?string $param = null): void; */ public function getHttpHost(): string; + /** + * The host this request actually arrived at, without consulting the forwarded headers. + */ + public function getHttpHostIgnoringForwarding(): string; + /** * Return forward data per RFC 7239 * diff --git a/src/Infrastructure/Adapter/In/Web/Controllers/User/UserSaveBase.php b/src/Infrastructure/Adapter/In/Web/Controllers/User/UserSaveBase.php index 4d8869d1b..7e5bb4b34 100644 --- a/src/Infrastructure/Adapter/In/Web/Controllers/User/UserSaveBase.php +++ b/src/Infrastructure/Adapter/In/Web/Controllers/User/UserSaveBase.php @@ -122,7 +122,7 @@ final protected function checkChangeUserPass(int $userId, User $userData): void $this->mailService->send( __('Password Change'), $userData->getEmail() ?? '', - UserPassRecover::getMailMessage($hash, $this->uriContext->getWebUri()) + UserPassRecover::getMailMessage($hash, $this->configData, $this->uriContext) ); } } diff --git a/src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestController.php b/src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestController.php index a7eb45338..adf2a8c03 100644 --- a/src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestController.php +++ b/src/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestController.php @@ -122,7 +122,7 @@ public function saveRequestAction(): ActionResponse $this->mailService->send( __('Password Change'), $email, - UserPassRecover::getMailMessage($hash, $this->uriContext->getWebUri()) + UserPassRecover::getMailMessage($hash, $this->configData, $this->uriContext) ); } catch (Exception $e) { // Recorded and counted, not reported. The tracking still runs, so guessing is still diff --git a/src/Infrastructure/Bootstrap/UriContext.php b/src/Infrastructure/Bootstrap/UriContext.php index 1c96bd5c7..ef46e28d2 100644 --- a/src/Infrastructure/Bootstrap/UriContext.php +++ b/src/Infrastructure/Bootstrap/UriContext.php @@ -36,12 +36,14 @@ private string $subUri; private string $webRoot; private string $webUri; + private string $unforwardedWebUri; public function __construct(RequestService $request) { $this->subUri = $this->buildSubUri($request); $this->webRoot = $this->buildWebRoot($request); $this->webUri = $request->getHttpHost() . $this->webRoot; + $this->unforwardedWebUri = $request->getHttpHostIgnoringForwarding() . $this->webRoot; } private function buildSubUri(RequestService $request): string @@ -62,6 +64,21 @@ private function buildWebRoot(RequestService $request): string return ''; } + /** + * The same URI, built without consulting `Forwarded` / `X-Forwarded-*`. + * + * `getWebUri()` prefers those headers so that an installation behind a reverse proxy reports + * the address its users actually type. They are supplied by whoever made the request, though, + * and nothing here calls `setTrustedProxies()` — verified against the running instance, where + * `X-Forwarded-Host: evil.example.com` comes straight back out. That is the right trade for a + * displayed URL and the wrong one for a link that is mailed to somebody else and carries a + * one-time token, which is what this exists for. + */ + public function getUnforwardedWebUri(): string + { + return $this->unforwardedWebUri; + } + public function getWebUri(): string { return $this->webUri; diff --git a/src/Infrastructure/Http/Services/Request.php b/src/Infrastructure/Http/Services/Request.php index 0153a093a..28a2a36aa 100644 --- a/src/Infrastructure/Http/Services/Request.php +++ b/src/Infrastructure/Http/Services/Request.php @@ -387,6 +387,19 @@ public function getHttpHost(): string return strtolower(sprintf('%s://%s', $forwarded['proto'], $forwarded['host'])); } + return $this->getHttpHostIgnoringForwarding(); + } + + /** + * The host this request actually arrived at, without consulting the forwarded headers. + * + * Those headers are supplied by whoever made the request — nothing here calls + * `setTrustedProxies()` — so `getHttpHost()` answers whatever a caller asks it to. That is + * acceptable for a URL the same caller is about to be shown, and not for one that is mailed to + * somebody else with a one-time token in it. + */ + public function getHttpHostIgnoringForwarding(): string + { /** @noinspection HttpUrlsUsage */ $protocol = 'http://'; diff --git a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestControllerTest.php b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestControllerTest.php index 38f197be8..41eff8cb2 100644 --- a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestControllerTest.php +++ b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/UserPassReset/SaveRequestControllerTest.php @@ -30,6 +30,8 @@ use PHPUnit\Framework\MockObject\Exception; use Psr\Container\ContainerExceptionInterface; use Psr\Container\NotFoundExceptionInterface; +use SP\Application\Notification\Ports\MailService; +use SP\Domain\Core\Messages\MailMessage; use SP\Domain\User\Models\User as UserModel; use SP\Domain\Common\Dtos\QueryResult; use SP\Tests\Support\Generators\UserDataGenerator; @@ -78,4 +80,62 @@ public function testSaveRequestBuildsTheResetMailWithoutFataling(): void $this->expectOutputRegex('/"status":"OK","description":"Request sent"/'); } + + /** + * The mailed link points where the installation lives, not where the caller said it does. + * + * `saveRequestAction()` needs no session, and the base URI came from + * `UriContextInterface::getWebUri()`, which prefers `Forwarded` / `X-Forwarded-Host`. Nothing + * in this application calls `setTrustedProxies()`, so those headers are whatever the caller + * sent — verified against the running instance, where `X-Forwarded-Host: evil.example.com` + * comes straight back out. + * + * So an unauthenticated caller who knew a login and its email address could choose the host in + * the message the real user then received: a genuine mail, from the real installation, whose + * link hands the one-time hash to somebody else's server. + * + * @throws ContainerExceptionInterface + * @throws Exception + * @throws NotFoundExceptionInterface + */ + public function testTheResetLinkIgnoresAForwardedHost(): void + { + $login = 'resetme'; + $email = 'resetme@example.com'; + + $userData = UserDataGenerator::factory()->buildUserData()->mutate( + ['login' => $login, 'email' => $email, 'isDisabled' => false, 'isLdap' => false] + ); + + $this->addDatabaseMapperResolver(UserModel::class, new QueryResult([$userData])); + + $sent = null; + $mailService = $this->createStub(MailService::class); + $mailService->method('send')->willReturnCallback( + static function (string $subject, string|array $to, MailMessage $mailMessage) use (&$sent): void { + $sent = $mailMessage->composeText(); + } + ); + + $container = $this->buildContainer( + IntegrationTestCase::buildRequest( + 'post', + 'index.php', + ['r' => 'userPassReset/saveRequest'], + ['login' => $login, 'email' => $email], + [], + self::CSRF_TOKEN, + ['HTTP_X_FORWARDED_HOST' => 'evil.example.com', 'HTTP_X_FORWARDED_PROTO' => 'https'] + ), + [MailService::class => $mailService] + ); + + IntegrationTestCase::runApp($container); + + self::assertNotNull($sent, 'the reset mail has to be sent for this to say anything'); + self::assertStringNotContainsString('evil.example.com', $sent); + self::assertStringContainsString('localhost', $sent); + + $this->expectOutputRegex('/"status":"OK","description":"Request sent"/'); + } }