From 03f6029ce58f0dcd50b79d2acaff5bb4d92a810d Mon Sep 17 00:00:00 2001 From: Diogo Peralta Cordeiro Date: Sun, 10 Oct 2021 17:41:30 +0100 Subject: [PATCH] [SECURITY] Fix nickname validation and properly allow email auth --- config/packages/security.yaml | 13 ++- social.yaml | 3 +- src/Controller/Security.php | 54 ++++----- src/Entity/LocalUser.php | 4 +- src/Routes/Main.php | 12 +- src/Security/Authenticator.php | 33 +++--- ...on.php => NicknameNotAllowedException.php} | 4 +- .../Exception/NicknameTooShortException.php | 54 --------- src/Util/Exception/NoSuchActorException.php | 30 +++++ src/Util/Nickname.php | 110 ++++++++++-------- templates/cards/navigation/view.html.twig | 6 +- templates/security/login.html.twig | 6 +- tests/Util/NicknameTest.php | 10 +- 13 files changed, 163 insertions(+), 176 deletions(-) rename src/Util/Exception/{NicknameReservedException.php => NicknameNotAllowedException.php} (93%) delete mode 100644 src/Util/Exception/NicknameTooShortException.php create mode 100644 src/Util/Exception/NoSuchActorException.php diff --git a/config/packages/security.yaml b/config/packages/security.yaml index 8eb456caf1..897ccfc7b7 100644 --- a/config/packages/security.yaml +++ b/config/packages/security.yaml @@ -1,10 +1,17 @@ security: # https://symfony.com/doc/current/security.html#where-do-users-come-from-user-providers providers: - users: + local_user: + chain: + providers: [local_user_by_nickname, local_user_by_email] + local_user_by_nickname: entity: class: 'App\Entity\LocalUser' property: 'nickname' + local_user_by_email: + entity: + class: 'App\Entity\LocalUser' + property: 'email' firewalls: dev: pattern: ^/(_(profiler|wdt)|css|images|js)/ @@ -12,12 +19,12 @@ security: main: anonymous: true lazy: true - provider: users + provider: local_user guard: authenticators: - App\Security\Authenticator logout: - path: logout + path: security_logout # where to redirect after logout target: main_all diff --git a/social.yaml b/social.yaml index 82616b1516..f1b0f90e7a 100644 --- a/social.yaml +++ b/social.yaml @@ -149,7 +149,7 @@ parameters: image: "/theme/licenses/cc_by_4.0.png" nickname: - reserved: + blacklisted: - doc - main - avatar @@ -157,7 +157,6 @@ parameters: - settings - admin featured: [] - min_length: 4 password: min_length: 6 diff --git a/src/Controller/Security.php b/src/Controller/Security.php index f39ff08ffe..7c17912b78 100644 --- a/src/Controller/Security.php +++ b/src/Controller/Security.php @@ -5,6 +5,8 @@ namespace App\Controller; use App\Core\Controller; use App\Core\DB\DB; use App\Core\Form; +use App\Util\Exception\NicknameInvalidException; +use LogicException; use function App\Core\I18n\_m; use App\Core\Log; use App\Core\VisibilityScope; @@ -18,11 +20,9 @@ use App\Util\Common; use App\Util\Exception\DuplicateFoundException; use App\Util\Exception\EmailTakenException; use App\Util\Exception\NicknameEmptyException; -use App\Util\Exception\NicknameReservedException; +use App\Util\Exception\NicknameNotAllowedException; use App\Util\Exception\NicknameTakenException; use App\Util\Exception\NicknameTooLongException; -use App\Util\Exception\NicknameTooShortException; -use App\Util\Exception\NotImplementedException; use App\Util\Exception\ServerException; use App\Util\Form\FormFields; use App\Util\Nickname; @@ -57,7 +57,7 @@ class Security extends Controller '_template' => 'security/login.html.twig', 'last_login_id' => $last_login_id, 'error' => $error, - 'notes_fn' => fn () => Note::getAllNotes(VisibilityScope::$instance_scope), + 'notes_fn' => fn () => Note::getAllNotes(VisibilityScope::$instance_scope), ]; } @@ -66,7 +66,7 @@ class Security extends Controller */ public function logout() { - throw new \LogicException('This method can be blank - it will be intercepted by the logout key on your firewall.'); + throw new LogicException('This method can be blank - it will be intercepted by the logout key on your firewall.'); } /** @@ -74,25 +74,24 @@ class Security extends Controller * Register a user, making sure the nickname is not reserved and * possibly sending a confirmation email * - * @param Request $request + * @param Request $request * @param GuardAuthenticatorHandler $guard_handler - * @param Authenticator $authenticator + * @param Authenticator $authenticator * + * @return null|array|Response * @throws EmailTakenException * @throws NicknameTakenException * @throws ServerException * @throws DuplicateFoundException * @throws NicknameEmptyException - * @throws NicknameReservedException + * @throws NicknameNotAllowedException * @throws NicknameTooLongException - * @throws NicknameTooShortException - * @throws NotImplementedException + * @throws NicknameInvalidException * - * @return null|array|Response */ - public function register(Request $request, + public function register(Request $request, GuardAuthenticatorHandler $guard_handler, - Authenticator $authenticator) + Authenticator $authenticator) { $form = Form::create([ ['nickname', TextType::class, [ @@ -101,8 +100,8 @@ class Security extends Controller 'constraints' => [ new NotBlank(['message' => _m('Please enter a nickname')]), new Length([ - 'min' => Common::config('nickname', 'min_length'), - 'minMessage' => _m(['Your nickname must be at least # characters long'], ['count' => Common::config('nickname', 'min_length')]), + 'min' => 1, + 'minMessage' => _m(['Your nickname must be at least # characters long'], ['count' => 1]), 'max' => Nickname::MAX_LEN, 'maxMessage' => _m(['Your nickname must be at most # characters long'], ['count' => Nickname::MAX_LEN]), ]), ], @@ -128,29 +127,16 @@ class Security extends Controller $data = $form->getData(); $data['password'] = $form->get('password')->getData(); - // This will throw the appropriate errors, result ignored - $user = LocalUser::findByNicknameOrEmail($data['nickname'], $data['email']); - if ($user !== null) { + // TODO: ensure there's no user with this email registered already - // If we do find something, there's a duplicate - if ($user->getNickname() === $data['nickname']) { - // Register page feedback on nickname already in use - $this->addFlash('verify_nickname_error', _m('Nickname is already in use on this server.')); - throw new NicknameTakenException; - } else { - // Register page feedback on email already in use - $this->addFlash('verify_email_error', _m('Email is already taken.')); - throw new EmailTakenException; - } - } - - $valid_nickname = Nickname::validate($data['nickname'], check_already_used: false); + // Already used is checked below + $sanitized_nickname = Nickname::normalize($data['nickname'], check_already_used: false); try { // This already checks if the nickname is being used - $actor = Actor::create(['nickname' => $valid_nickname]); + $actor = Actor::create(['nickname' => $sanitized_nickname]); $user = LocalUser::create([ - 'nickname' => $valid_nickname, + 'nickname' => $sanitized_nickname, 'outgoing_email' => $data['email'], 'incoming_email' => $data['email'], 'password' => LocalUser::hashPassword($data['password']), @@ -166,7 +152,7 @@ class Security extends Controller } catch (UniqueConstraintViolationException $e) { // _something_ was duplicated, but since we already check if nickname is in use, we can't tell what went wrong $e = 'An error occurred while trying to register'; - Log::critical($e . " with nickname: '{$valid_nickname}' and email '{$data['email']}'"); + Log::critical($e . " with nickname: '{$sanitized_nickname}' and email '{$data['email']}'"); throw new ServerException(_m($e)); } // @codeCoverageIgnoreEnd diff --git a/src/Entity/LocalUser.php b/src/Entity/LocalUser.php index 5326ebcb1f..3f71acfb0d 100644 --- a/src/Entity/LocalUser.php +++ b/src/Entity/LocalUser.php @@ -308,9 +308,9 @@ class LocalUser extends Entity implements UserInterface * * @return self Returns self if nickname or email found */ - public static function findByNicknameOrEmail(string $nickname, string $email): ?self + public static function getByEmail(string $email): ?self { - $users = DB::findBy('local_user', ['or' => ['nickname' => $nickname, 'outgoing_email' => $email, 'incoming_email' => $email]]); + $users = DB::findBy('local_user', ['or' => ['outgoing_email' => $email, 'incoming_email' => $email]]); switch (count($users)) { case 0: return null; diff --git a/src/Routes/Main.php b/src/Routes/Main.php index 3a3bba9e09..95f4e19b1e 100644 --- a/src/Routes/Main.php +++ b/src/Routes/Main.php @@ -45,12 +45,12 @@ abstract class Main public static function load(RouteLoader $r): void { - $r->connect('login', '/login', [C\Security::class, 'login']); - $r->connect('logout', '/logout', [C\Security::class, 'logout']); - $r->connect('register', '/register', [C\Security::class, 'register']); - $r->connect('check_email', '/check-email', [C\ResetPassword::class, 'checkEmail']); - $r->connect('request_reset_password', '/request-reset-password', [C\ResetPassword::class, 'requestPasswordReset']); - $r->connect('reset_password', '/reset/{token?}', [C\ResetPassword::class, 'reset']); + $r->connect('security_login', '/main/login', [C\Security::class, 'login']); + $r->connect('security_logout', '/main/logout', [C\Security::class, 'logout']); + $r->connect('security_register', '/main/register', [C\Security::class, 'register']); + $r->connect('security_check_email', '/main/check-email', [C\ResetPassword::class, 'checkEmail']); + $r->connect('security_recover_password', '/main/recover-password', [C\ResetPassword::class, 'requestPasswordReset']); + $r->connect('security_recover_password_token', '/main/recover-password/{token?}', [C\ResetPassword::class, 'reset']); $r->connect('root', '/', RedirectController::class, ['defaults' => ['route' => 'main_all']]); $r->connect('main_public', '/main/public', [C\Network::class, 'public']); diff --git a/src/Security/Authenticator.php b/src/Security/Authenticator.php index cb125e167a..ffcad46e24 100644 --- a/src/Security/Authenticator.php +++ b/src/Security/Authenticator.php @@ -20,6 +20,8 @@ namespace App\Security; use App\Core\DB\DB; +use App\Util\Exception\NoSuchActorException; +use App\Util\Exception\NotFoundException; use function App\Core\I18n\_m; use App\Entity\LocalUser; use App\Entity\User; @@ -52,7 +54,7 @@ class Authenticator extends AbstractFormLoginAuthenticator { use TargetPathTrait; - public const LOGIN_ROUTE = 'login'; + public const LOGIN_ROUTE = 'security_login'; private $urlGenerator; private $csrfTokenManager; @@ -70,17 +72,11 @@ class Authenticator extends AbstractFormLoginAuthenticator public function getCredentials(Request $request) { - $credentials = [ - 'nickname' => $request->request->get('nickname'), + return [ + 'nickname_or_email' => $request->request->get('nickname_or_email'), 'password' => $request->request->get('password'), 'csrf_token' => $request->request->get('_csrf_token'), ]; - $request->getSession()->set( - Security::LAST_USERNAME, - $credentials['nickname'] - ); - - return $credentials; } public function getUser($credentials, UserProviderInterface $userProvider) @@ -90,12 +86,17 @@ class Authenticator extends AbstractFormLoginAuthenticator throw new InvalidCsrfTokenException(); } - // $nick = Nickname::normalize($credentials['nickname']); - $nick = $credentials['nickname']; - $user = null; try { - $user = DB::findOneBy('local_user', ['or' => ['nickname' => $nick, 'outgoing_email' => $nick]]); - } catch (Exception $e) { + if (filter_var($credentials['nickname_or_email'], FILTER_VALIDATE_EMAIL) !== false) { + $user = LocalUser::getByEmail($credentials['nickname_or_email']); + } else { + $user = LocalUser::getWithPK(['nickname' => Nickname::normalize($credentials['nickname_or_email'], check_already_used: false)]); + } + if ($user === null) { + throw new NoSuchActorException('No such local user.'); + } + $credentials['nickname'] = $user->getNickname(); + } catch (Exception) { throw new CustomUserMessageAuthenticationException( _m('Invalid login credentials.')); } @@ -118,6 +119,10 @@ class Authenticator extends AbstractFormLoginAuthenticator public function onAuthenticationSuccess(Request $request, TokenInterface $token, $providerKey) { + $request->getSession()->set( + Security::LAST_USERNAME, + $token->getUser()->getNickname() + ); if ($targetPath = $this->getTargetPath($request->getSession(), $providerKey)) { return new RedirectResponse($targetPath); } diff --git a/src/Util/Exception/NicknameReservedException.php b/src/Util/Exception/NicknameNotAllowedException.php similarity index 93% rename from src/Util/Exception/NicknameReservedException.php rename to src/Util/Exception/NicknameNotAllowedException.php index 23793bea99..467ccecf7b 100644 --- a/src/Util/Exception/NicknameReservedException.php +++ b/src/Util/Exception/NicknameNotAllowedException.php @@ -41,11 +41,11 @@ namespace App\Util\Exception; use function App\Core\I18n\_m; -class NicknameReservedException extends NicknameException +class NicknameNotAllowedException extends NicknameException { protected function defaultMessage(): string { // TRANS: Validation error in form for registration, profile and group settings, etc. - return _m('Nickname is reserved.'); + return _m('This nickname is not allowed.'); } } diff --git a/src/Util/Exception/NicknameTooShortException.php b/src/Util/Exception/NicknameTooShortException.php deleted file mode 100644 index 42b358a041..0000000000 --- a/src/Util/Exception/NicknameTooShortException.php +++ /dev/null @@ -1,54 +0,0 @@ -. - -// }}} - -/** - * Nickname too long exception - * - * @category Exception - * @package GNUsocial - * - * @author Zach Copley - * @copyright 2010 StatusNet Inc. - * @author Brion Vibber - * @author Mikael Nordfeldth - * @author Nym Coy - * @copyright 2009-2014 Free Software Foundation, Inc http://www.fsf.org - * @auuthor Daniel Supernault - * @auuthor Diogo Cordeiro - * - * @author Hugo Sales - * @copyright 2018-2021 Free Software Foundation, Inc http://www.fsf.org - * @license https://www.gnu.org/licenses/agpl.html GNU AGPL v3 or later - */ - -namespace App\Util\Exception; - -use function App\Core\I18n\_m; -use App\Util\Common; - -class NicknameTooShortException extends NicknameInvalidException -{ - protected function defaultMessage(): string - { - // TRANS: Validation error in form for registration, profile and group settings, etc. - return _m(['Nickname cannot be less than # character long.'], ['count' => Common::config('nickname', 'min_length')]); - } -} diff --git a/src/Util/Exception/NoSuchActorException.php b/src/Util/Exception/NoSuchActorException.php new file mode 100644 index 0000000000..fd3e215bfa --- /dev/null +++ b/src/Util/Exception/NoSuchActorException.php @@ -0,0 +1,30 @@ +. + +// }}} + +namespace App\Util\Exception; + +class NoSuchActorException extends ClientException +{ + public function __construct(string $m) + { + parent::__construct($m); + } +} diff --git a/src/Util/Nickname.php b/src/Util/Nickname.php index cc924f7d92..68d3300c98 100644 --- a/src/Util/Nickname.php +++ b/src/Util/Nickname.php @@ -25,13 +25,13 @@ use App\Entity\LocalUser; use App\Util\Exception\NicknameEmptyException; use App\Util\Exception\NicknameException; use App\Util\Exception\NicknameInvalidException; -use App\Util\Exception\NicknameReservedException; +use App\Util\Exception\NicknameNotAllowedException; use App\Util\Exception\NicknameTakenException; use App\Util\Exception\NicknameTooLongException; use App\Util\Exception\NicknameTooShortException; use App\Util\Exception\NotImplementedException; use Functional as F; -use Normalizer; +use InvalidArgumentException; /** * Nickname validation @@ -45,9 +45,8 @@ use Normalizer; * @author Mikael Nordfeldth * @author Nym Coy * @copyright 2009-2014 Free Software Foundation, Inc http://www.fsf.org - * @auuthor Daniel Supernault - * @auuthor Diogo Cordeiro - * + * @author Daniel Supernault + * @author Diogo Cordeiro * @author Hugo Sales * @copyright 2018-2021 Free Software Foundation, Inc http://www.fsf.org * @license https://www.gnu.org/licenses/agpl.html GNU AGPL v3 or later @@ -91,6 +90,11 @@ class Nickname */ const WEBFINGER_FMT = '(?:\w+[\w\-\_\.]*)?\w+\@' . URL_REGEX_DOMAIN_NAME; + /** + * Maximum number of characters in a canonical-form nickname. Changes must validate regexs + */ + const MAX_LEN = 64; + /** * Regex fragment for checking a canonical nickname. * @@ -104,12 +108,7 @@ class Nickname * * This, INPUT_FMT and DISPLAY_FMT should not be enclosed in []s. */ - const CANONICAL_FMT = '[0-9a-z]{1,64}'; - - /** - * Maximum number of characters in a canonical-form nickname. Changes must validate regexs - */ - const MAX_LEN = 64; + const CANONICAL_FMT = '[0-9a-z]{1,' . self::MAX_LEN . '}'; /** * Regex with non-capturing group that matches whitespace and some @@ -121,72 +120,78 @@ class Nickname */ const BEFORE_MENTIONS = '(?:^|[\s\.\,\:\;\[\(]+)'; - const CHECK_LOCAL_USER = 1; + const CHECK_LOCAL_USER = 1; const CHECK_LOCAL_GROUP = 2; /** * Check if a nickname is valid or throw exceptions if it's not. * Can optionally check if the nickname is currently in use + * @param string $nickname + * @param bool $check_already_used + * @param int $which + * @param bool $check_is_allowed + * @return bool + * @throws NicknameEmptyException + * @throws NicknameNotAllowedException + * @throws NicknameTakenException + * @throws NicknameTooLongException */ - public static function validate(string $nickname, bool $check_already_used = false, int $which = self::CHECK_LOCAL_USER) + public static function validate(string $nickname, bool $check_already_used = false, int $which = self::CHECK_LOCAL_USER, bool $check_is_allowed = true): bool { - $nickname = trim($nickname); - $length = mb_strlen($nickname); + $length = mb_strlen($nickname); if ($length < 1) { throw new NicknameEmptyException(); - } elseif ($length < Common::config('nickname', 'min_length')) { - // dd($nickname, $length, Common::config('nickname', 'min_length')); - throw new NicknameTooShortException(); } else { if ($length > self::MAX_LEN) { throw new NicknameTooLongException(); - } elseif (self::isReserved($nickname) || Common::isSystemPath($nickname)) { - throw new NicknameReservedException(); + } elseif ($check_is_allowed && self::isBlacklisted($nickname)) { + throw new NicknameNotAllowedException(); } elseif ($check_already_used) { switch ($which) { - case self::CHECK_LOCAL_USER: - $lu = LocalUser::findByNicknameOrEmail($nickname, email: ''); - if ($lu !== null) { - throw new NicknameTakenException($lu->getActor()); - } - break; + case self::CHECK_LOCAL_USER: + $lu = LocalUser::getWithPK(['nickname' => $nickname]); + if ($lu !== null) { + throw new NicknameTakenException($lu->getActor()); + } + break; // @codeCoverageIgnoreStart - case self::CHECK_LOCAL_GROUP: - throw new NotImplementedException(); - break; - default: - throw new \InvalidArgumentException(); + case self::CHECK_LOCAL_GROUP: + throw new NotImplementedException(); + break; + default: + throw new InvalidArgumentException(); // @codeCoverageIgnoreEnd } } } - return $nickname; + return true; } /** - * Normalize an input $nickname, and normalize it to its canonical form. + * Normalize input $nickname to its canonical form and validates it. * The canonical form will be returned, or an exception thrown if invalid. * - * @throws NicknameException (base class) + * @param string $nickname + * @param bool $check_already_used + * @param bool $check_is_allowed + * @return string * @throws NicknameEmptyException * @throws NicknameInvalidException + * @throws NicknameNotAllowedException * @throws NicknameTakenException * @throws NicknameTooLongException - * @throws NicknameTooShortException */ - public static function normalize(string $nickname, bool $check_already_used = true, bool $checking_reserved = false): string + public static function normalize(string $nickname, bool $check_already_used = true, bool $check_is_allowed = true): string { - if (!$checking_reserved) { - $nickname = self::validate($nickname, $check_already_used); - } - $nickname = trim($nickname); $nickname = str_replace('_', '', $nickname); $nickname = mb_strtolower($nickname); - $nickname = Normalizer::normalize($nickname, Normalizer::FORM_C); - if (!self::isCanonical($nickname) && !filter_var($nickname, FILTER_VALIDATE_EMAIL)) { + // We could do UTF-8 normalization (å to a, etc.) with something like Normalizer::normalize($nickname, Normalizer::FORM_C) + // We won't as it could confuse tremendously the user, he must know what is valid and should fix his own input + + if (!self::validate($nickname, $check_already_used, $check_is_allowed) || !self::isCanonical($nickname)) { throw new NicknameInvalidException(); } @@ -201,11 +206,11 @@ class Nickname * * @return bool True if nickname is valid. False if invalid (or taken if $check_already_used == true). */ - public static function isValid(string $nickname, bool $check_already_used = true): bool + public static function isValid(string $nickname, bool $check_already_used = true, bool $check_is_allowed = true): bool { try { - self::normalize($nickname, $check_already_used); - } catch (NicknameException $e) { + self::normalize($nickname, $check_already_used, $check_is_allowed); + } catch (NicknameException) { return false; } @@ -214,6 +219,8 @@ class Nickname /** * Is the given string a valid canonical nickname form? + * @param string $nickname + * @return bool */ public static function isCanonical(string $nickname): bool { @@ -222,15 +229,22 @@ class Nickname /** * Is the given string in our nickname blacklist? + * @param string $nickname + * @return bool + * @throws NicknameEmptyException + * @throws NicknameInvalidException + * @throws NicknameNotAllowedException + * @throws NicknameTakenException + * @throws NicknameTooLongException */ - public static function isReserved(string $nickname): bool + public static function isBlacklisted(string $nickname): bool { - $reserved = Common::config('nickname', 'reserved'); + $reserved = Common::config('nickname', 'blacklist'); if (empty($reserved)) { return false; } return in_array($nickname, array_merge($reserved, F\map($reserved, function ($n) { - return self::normalize($n, check_already_used: false, checking_reserved: true); + return self::normalize($n, check_already_used: false, check_is_allowed: true); }))); } } diff --git a/templates/cards/navigation/view.html.twig b/templates/cards/navigation/view.html.twig index e4f016da4a..9d6760a9d1 100644 --- a/templates/cards/navigation/view.html.twig +++ b/templates/cards/navigation/view.html.twig @@ -89,7 +89,7 @@ Settings - + Logout @@ -100,11 +100,11 @@

Account