diff --git a/.github/workflows/psalm.yml b/.github/workflows/psalm.yml index 4c15203380..65ac279549 100644 --- a/.github/workflows/psalm.yml +++ b/.github/workflows/psalm.yml @@ -39,7 +39,7 @@ jobs: run: | echo "::group::modify composer.json" composer remove --no-update --no-interaction symfony/phpunit-bridge - composer require --no-update psalm/phar phpunit/phpunit php-http/discovery psr/event-dispatcher + composer require --no-update psalm/phar phpunit/phpunit:@stable php-http/discovery psr/event-dispatcher echo "::endgroup::" echo "::group::composer update" composer update --no-progress --ansi diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php index 1d9a2a6647..54f222a84e 100644 --- a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php +++ b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php @@ -502,7 +502,7 @@ class SecurityExtension extends Extension implements PrependExtensionInterface ->replaceArgument(0, $authenticators) ->replaceArgument(2, new Reference($firewallEventDispatcherId)) ->replaceArgument(3, $id) - ->replaceArgument(6, $firewall['required_badges'] ?? []) + ->replaceArgument(7, $firewall['required_badges'] ?? []) ->addTag('monolog.logger', ['channel' => 'security']) ; diff --git a/src/Symfony/Bundle/SecurityBundle/Resources/config/guard.php b/src/Symfony/Bundle/SecurityBundle/Resources/config/guard.php index 60677a94de..f113dec880 100644 --- a/src/Symfony/Bundle/SecurityBundle/Resources/config/guard.php +++ b/src/Symfony/Bundle/SecurityBundle/Resources/config/guard.php @@ -45,6 +45,7 @@ return static function (ContainerConfigurator $container) { abstract_arg('Provider-shared Key'), abstract_arg('Authenticators'), service('logger')->nullOnInvalid(), + param('security.authentication.hide_user_not_found'), ]) ->tag('monolog.logger', ['channel' => 'security']) ; diff --git a/src/Symfony/Bundle/SecurityBundle/Resources/config/security_authenticator.php b/src/Symfony/Bundle/SecurityBundle/Resources/config/security_authenticator.php index ebc9a5fa64..0f7d557354 100644 --- a/src/Symfony/Bundle/SecurityBundle/Resources/config/security_authenticator.php +++ b/src/Symfony/Bundle/SecurityBundle/Resources/config/security_authenticator.php @@ -44,6 +44,7 @@ return static function (ContainerConfigurator $container) { abstract_arg('provider key'), service('logger')->nullOnInvalid(), param('security.authentication.manager.erase_credentials'), + param('security.authentication.hide_user_not_found'), abstract_arg('required badges'), ]) ->tag('monolog.logger', ['channel' => 'security']) diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/CompleteConfigurationTest.php b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/CompleteConfigurationTest.php index 317da3930b..9e8a044923 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/CompleteConfigurationTest.php +++ b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/CompleteConfigurationTest.php @@ -43,7 +43,7 @@ abstract class CompleteConfigurationTest extends TestCase $this->assertEquals(AuthenticatorManager::class, $authenticatorManager->getClass()); // required badges - $this->assertEquals([CsrfTokenBadge::class, RememberMeBadge::class], $authenticatorManager->getArgument(6)); + $this->assertEquals([CsrfTokenBadge::class, RememberMeBadge::class], $authenticatorManager->getArgument(7)); // login link $expiredStorage = $container->getDefinition($expiredStorageId = 'security.authenticator.expired_login_link_storage.main'); diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/AuthenticatorTest.php b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/AuthenticatorTest.php index 5aeb0b8335..5c60294876 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/AuthenticatorTest.php +++ b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/AuthenticatorTest.php @@ -40,7 +40,7 @@ class AuthenticatorTest extends AbstractWebTestCase if ($withinFirewall) { $this->assertJsonStringEqualsJsonString('{"email":"'.$email.'"}', $client->getResponse()->getContent()); } else { - $this->assertJsonStringEqualsJsonString('{"error":"Username could not be found."}', $client->getResponse()->getContent()); + $this->assertJsonStringEqualsJsonString('{"error":"Invalid credentials."}', $client->getResponse()->getContent()); } } diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/FormLoginTest.php b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/FormLoginTest.php index 05b27e82ac..de3c15ab83 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/FormLoginTest.php +++ b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/FormLoginTest.php @@ -142,7 +142,7 @@ class FormLoginTest extends AbstractWebTestCase break; case 2: // Third attempt with unexisting username - $this->assertStringContainsString('Username could not be found.', $text, 'Invalid response on 3rd attempt'); + $this->assertStringContainsString('Invalid credentials.', $text, 'Invalid response on 3rd attempt'); break; case 3: // Fourth attempt : still login throttling ! diff --git a/src/Symfony/Component/Security/Core/Authentication/Provider/UserAuthenticationProvider.php b/src/Symfony/Component/Security/Core/Authentication/Provider/UserAuthenticationProvider.php index 4dfff685a7..a4811faffe 100644 --- a/src/Symfony/Component/Security/Core/Authentication/Provider/UserAuthenticationProvider.php +++ b/src/Symfony/Component/Security/Core/Authentication/Provider/UserAuthenticationProvider.php @@ -14,6 +14,7 @@ namespace Symfony\Component\Security\Core\Authentication\Provider; use Symfony\Component\Security\Core\Authentication\Token\SwitchUserToken; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken; +use Symfony\Component\Security\Core\Exception\AccountStatusException; use Symfony\Component\Security\Core\Exception\AuthenticationException; use Symfony\Component\Security\Core\Exception\AuthenticationServiceException; use Symfony\Component\Security\Core\Exception\BadCredentialsException; @@ -79,7 +80,7 @@ abstract class UserAuthenticationProvider implements AuthenticationProviderInter $this->userChecker->checkPreAuth($user); $this->checkAuthentication($user, $token); $this->userChecker->checkPostAuth($user); - } catch (BadCredentialsException $e) { + } catch (AccountStatusException $e) { if ($this->hideUserNotFoundExceptions) { throw new BadCredentialsException('Bad credentials.', 0, $e); } diff --git a/src/Symfony/Component/Security/Core/Resources/translations/security.fi.xlf b/src/Symfony/Component/Security/Core/Resources/translations/security.fi.xlf index 7b8d232cdd..d0e94545e7 100644 --- a/src/Symfony/Component/Security/Core/Resources/translations/security.fi.xlf +++ b/src/Symfony/Component/Security/Core/Resources/translations/security.fi.xlf @@ -70,6 +70,14 @@ Invalid or expired login link. Virheellinen tai vanhentunut kirjautumislinkki. + + Too many failed login attempts, please try again in %minutes% minute. + Liian monta epäonnistunutta kirjautumisyritystä, yritä uudelleen %minutes% minuutin kuluttua. + + + Too many failed login attempts, please try again in %minutes% minutes. + Liian monta epäonnistunutta kirjautumisyritystä, yritä uudelleen %minutes% minuutin kuluttua. + diff --git a/src/Symfony/Component/Security/Core/Tests/Authentication/Provider/UserAuthenticationProviderTest.php b/src/Symfony/Component/Security/Core/Tests/Authentication/Provider/UserAuthenticationProviderTest.php index 92b71448d5..eb95ec9cea 100644 --- a/src/Symfony/Component/Security/Core/Tests/Authentication/Provider/UserAuthenticationProviderTest.php +++ b/src/Symfony/Component/Security/Core/Tests/Authentication/Provider/UserAuthenticationProviderTest.php @@ -83,7 +83,7 @@ class UserAuthenticationProviderTest extends TestCase public function testAuthenticateWhenPreChecksFails() { - $this->expectException(CredentialsExpiredException::class); + $this->expectException(BadCredentialsException::class); $userChecker = $this->createMock(UserCheckerInterface::class); $userChecker->expects($this->once()) ->method('checkPreAuth') @@ -101,7 +101,7 @@ class UserAuthenticationProviderTest extends TestCase public function testAuthenticateWhenPostChecksFails() { - $this->expectException(AccountExpiredException::class); + $this->expectException(BadCredentialsException::class); $userChecker = $this->createMock(UserCheckerInterface::class); $userChecker->expects($this->once()) ->method('checkPostAuth') @@ -128,7 +128,7 @@ class UserAuthenticationProviderTest extends TestCase ; $provider->expects($this->once()) ->method('checkAuthentication') - ->willThrowException(new BadCredentialsException()) + ->willThrowException(new CredentialsExpiredException()) ; $provider->authenticate($this->getSupportedToken()); diff --git a/src/Symfony/Component/Security/Guard/Firewall/GuardAuthenticationListener.php b/src/Symfony/Component/Security/Guard/Firewall/GuardAuthenticationListener.php index 022538731d..3d3cd1ace5 100644 --- a/src/Symfony/Component/Security/Guard/Firewall/GuardAuthenticationListener.php +++ b/src/Symfony/Component/Security/Guard/Firewall/GuardAuthenticationListener.php @@ -17,7 +17,11 @@ use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpKernel\Event\RequestEvent; use Symfony\Component\Security\Core\Authentication\AuthenticationManagerInterface; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; +use Symfony\Component\Security\Core\Exception\AccountStatusException; use Symfony\Component\Security\Core\Exception\AuthenticationException; +use Symfony\Component\Security\Core\Exception\BadCredentialsException; +use Symfony\Component\Security\Core\Exception\CustomUserMessageAccountStatusException; +use Symfony\Component\Security\Core\Exception\UsernameNotFoundException; use Symfony\Component\Security\Guard\AuthenticatorInterface; use Symfony\Component\Security\Guard\GuardAuthenticatorHandler; use Symfony\Component\Security\Guard\Token\PreAuthenticationGuardToken; @@ -40,12 +44,13 @@ class GuardAuthenticationListener extends AbstractListener private $guardAuthenticators; private $logger; private $rememberMeServices; + private $hideUserNotFoundExceptions; /** * @param string $providerKey The provider (i.e. firewall) key * @param iterable|AuthenticatorInterface[] $guardAuthenticators The authenticators, with keys that match what's passed to GuardAuthenticationProvider */ - public function __construct(GuardAuthenticatorHandler $guardHandler, AuthenticationManagerInterface $authenticationManager, string $providerKey, iterable $guardAuthenticators, LoggerInterface $logger = null) + public function __construct(GuardAuthenticatorHandler $guardHandler, AuthenticationManagerInterface $authenticationManager, string $providerKey, iterable $guardAuthenticators, LoggerInterface $logger = null, bool $hideUserNotFoundExceptions = true) { if (empty($providerKey)) { throw new \InvalidArgumentException('$providerKey must not be empty.'); @@ -56,6 +61,7 @@ class GuardAuthenticationListener extends AbstractListener $this->providerKey = $providerKey; $this->guardAuthenticators = $guardAuthenticators; $this->logger = $logger; + $this->hideUserNotFoundExceptions = $hideUserNotFoundExceptions; } /** @@ -160,6 +166,12 @@ class GuardAuthenticationListener extends AbstractListener $this->logger->info('Guard authentication failed.', ['exception' => $e, 'authenticator' => \get_class($guardAuthenticator)]); } + // Avoid leaking error details in case of invalid user (e.g. user not found or invalid account status) + // to prevent user enumeration via response content + if ($this->hideUserNotFoundExceptions && ($e instanceof UsernameNotFoundException || ($e instanceof AccountStatusException && !$e instanceof CustomUserMessageAccountStatusException))) { + $e = new BadCredentialsException('Bad credentials.', 0, $e); + } + $response = $this->guardHandler->handleAuthenticationFailure($e, $request, $guardAuthenticator, $this->providerKey); if ($response instanceof Response) { diff --git a/src/Symfony/Component/Security/Guard/Tests/Firewall/GuardAuthenticationListenerTest.php b/src/Symfony/Component/Security/Guard/Tests/Firewall/GuardAuthenticationListenerTest.php index 7d1c8e7246..512bf313f7 100644 --- a/src/Symfony/Component/Security/Guard/Tests/Firewall/GuardAuthenticationListenerTest.php +++ b/src/Symfony/Component/Security/Guard/Tests/Firewall/GuardAuthenticationListenerTest.php @@ -19,6 +19,9 @@ use Symfony\Component\HttpKernel\Event\RequestEvent; use Symfony\Component\Security\Core\Authentication\AuthenticationProviderManager; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; use Symfony\Component\Security\Core\Exception\AuthenticationException; +use Symfony\Component\Security\Core\Exception\BadCredentialsException; +use Symfony\Component\Security\Core\Exception\LockedException; +use Symfony\Component\Security\Core\Exception\UsernameNotFoundException; use Symfony\Component\Security\Guard\AuthenticatorInterface; use Symfony\Component\Security\Guard\Firewall\GuardAuthenticationListener; use Symfony\Component\Security\Guard\GuardAuthenticatorHandler; @@ -211,6 +214,54 @@ class GuardAuthenticationListenerTest extends TestCase $listener($this->event); } + /** + * @dataProvider exceptionsToHide + */ + public function testHandleHidesInvalidUserExceptions(AuthenticationException $exceptionToHide) + { + $authenticator = $this->createMock(AuthenticatorInterface::class); + $providerKey = 'my_firewall2'; + + $authenticator + ->expects($this->once()) + ->method('supports') + ->willReturn(true); + $authenticator + ->expects($this->once()) + ->method('getCredentials') + ->willReturn(['username' => 'robin', 'password' => 'hood']); + + $this->authenticationManager + ->expects($this->once()) + ->method('authenticate') + ->willThrowException($exceptionToHide); + + $this->guardAuthenticatorHandler + ->expects($this->once()) + ->method('handleAuthenticationFailure') + ->with($this->callback(function ($e) use ($exceptionToHide) { + return $e instanceof BadCredentialsException && $exceptionToHide === $e->getPrevious(); + }), $this->request, $authenticator, $providerKey); + + $listener = new GuardAuthenticationListener( + $this->guardAuthenticatorHandler, + $this->authenticationManager, + $providerKey, + [$authenticator], + $this->logger + ); + + $listener($this->event); + } + + public function exceptionsToHide() + { + return [ + [new UsernameNotFoundException()], + [new LockedException()], + ]; + } + public function testSupportsReturnFalseSkipAuth() { $authenticator = $this->createMock(AuthenticatorInterface::class); diff --git a/src/Symfony/Component/Security/Http/Authentication/AuthenticatorManager.php b/src/Symfony/Component/Security/Http/Authentication/AuthenticatorManager.php index f18dbf4856..800605d60a 100644 --- a/src/Symfony/Component/Security/Http/Authentication/AuthenticatorManager.php +++ b/src/Symfony/Component/Security/Http/Authentication/AuthenticatorManager.php @@ -18,8 +18,11 @@ use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInt use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; use Symfony\Component\Security\Core\AuthenticationEvents; use Symfony\Component\Security\Core\Event\AuthenticationSuccessEvent; +use Symfony\Component\Security\Core\Exception\AccountStatusException; use Symfony\Component\Security\Core\Exception\AuthenticationException; use Symfony\Component\Security\Core\Exception\BadCredentialsException; +use Symfony\Component\Security\Core\Exception\CustomUserMessageAccountStatusException; +use Symfony\Component\Security\Core\Exception\UsernameNotFoundException; use Symfony\Component\Security\Core\User\UserInterface; use Symfony\Component\Security\Http\Authenticator\AuthenticatorInterface; use Symfony\Component\Security\Http\Authenticator\InteractiveAuthenticatorInterface; @@ -48,12 +51,13 @@ class AuthenticatorManager implements AuthenticatorManagerInterface, UserAuthent private $eraseCredentials; private $logger; private $firewallName; + private $hideUserNotFoundExceptions; private $requiredBadges; /** * @param AuthenticatorInterface[] $authenticators */ - public function __construct(iterable $authenticators, TokenStorageInterface $tokenStorage, EventDispatcherInterface $eventDispatcher, string $firewallName, ?LoggerInterface $logger = null, bool $eraseCredentials = true, array $requiredBadges = []) + public function __construct(iterable $authenticators, TokenStorageInterface $tokenStorage, EventDispatcherInterface $eventDispatcher, string $firewallName, ?LoggerInterface $logger = null, bool $eraseCredentials = true, bool $hideUserNotFoundExceptions = true, array $requiredBadges = []) { $this->authenticators = $authenticators; $this->tokenStorage = $tokenStorage; @@ -61,6 +65,7 @@ class AuthenticatorManager implements AuthenticatorManagerInterface, UserAuthent $this->firewallName = $firewallName; $this->logger = $logger; $this->eraseCredentials = $eraseCredentials; + $this->hideUserNotFoundExceptions = $hideUserNotFoundExceptions; $this->requiredBadges = $requiredBadges; } @@ -251,6 +256,12 @@ class AuthenticatorManager implements AuthenticatorManagerInterface, UserAuthent $this->logger->info('Authenticator failed.', ['exception' => $authenticationException, 'authenticator' => \get_class($authenticator)]); } + // Avoid leaking error details in case of invalid user (e.g. user not found or invalid account status) + // to prevent user enumeration via response content comparison + if ($this->hideUserNotFoundExceptions && ($authenticationException instanceof UsernameNotFoundException || ($authenticationException instanceof AccountStatusException && !$authenticationException instanceof CustomUserMessageAccountStatusException))) { + $authenticationException = new BadCredentialsException('Bad credentials.', 0, $authenticationException); + } + $response = $authenticator->onAuthenticationFailure($request, $authenticationException); if (null !== $response && null !== $this->logger) { $this->logger->debug('The "{authenticator}" authenticator set the failure response.', ['authenticator' => \get_class($authenticator)]); diff --git a/src/Symfony/Component/Security/Http/Tests/Authentication/AuthenticatorManagerTest.php b/src/Symfony/Component/Security/Http/Tests/Authentication/AuthenticatorManagerTest.php index f03bd85bd2..5912869985 100644 --- a/src/Symfony/Component/Security/Http/Tests/Authentication/AuthenticatorManagerTest.php +++ b/src/Symfony/Component/Security/Http/Tests/Authentication/AuthenticatorManagerTest.php @@ -19,6 +19,7 @@ use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInt use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken; use Symfony\Component\Security\Core\Exception\BadCredentialsException; +use Symfony\Component\Security\Core\Exception\UsernameNotFoundException; use Symfony\Component\Security\Core\User\InMemoryUser; use Symfony\Component\Security\Http\Authentication\AuthenticatorManager; use Symfony\Component\Security\Http\Authenticator\InteractiveAuthenticatorInterface; @@ -268,6 +269,26 @@ class AuthenticatorManagerTest extends TestCase $this->assertSame($this->response, $response); } + public function testAuthenticateRequestHidesInvalidUserExceptions() + { + $invalidUserException = new UsernameNotFoundException(); + $authenticator = $this->createMock(InteractiveAuthenticatorInterface::class); + $this->request->attributes->set('_security_authenticators', [$authenticator]); + + $authenticator->expects($this->any())->method('authenticate')->willThrowException($invalidUserException); + + $authenticator->expects($this->any()) + ->method('onAuthenticationFailure') + ->with($this->equalTo($this->request), $this->callback(function ($e) use ($invalidUserException) { + return $e instanceof BadCredentialsException && $invalidUserException === $e->getPrevious(); + })) + ->willReturn($this->response); + + $manager = $this->createManager([$authenticator]); + $response = $manager->authenticateRequest($this->request); + $this->assertSame($this->response, $response); + } + private function createAuthenticator($supports = true) { $authenticator = $this->createMock(InteractiveAuthenticatorInterface::class); @@ -278,6 +299,6 @@ class AuthenticatorManagerTest extends TestCase private function createManager($authenticators, $firewallName = 'main', $eraseCredentials = true, array $requiredBadges = []) { - return new AuthenticatorManager($authenticators, $this->tokenStorage, $this->eventDispatcher, $firewallName, null, $eraseCredentials, $requiredBadges); + return new AuthenticatorManager($authenticators, $this->tokenStorage, $this->eventDispatcher, $firewallName, null, $eraseCredentials, true, $requiredBadges); } }