From 0813323ac2327441dcd8004ef451d1b28dc6fcdf Mon Sep 17 00:00:00 2001 From: Andrew Stowell Date: Mon, 1 Jun 2026 16:28:57 -0400 Subject: [PATCH] renamed form fields to work better with password managers; fixed bug where an invalid login requests were not being counted as login attempts; preserve username when using central auth --- config/packages/twig.yaml | 1 + config/services.yaml | 3 + docs/example.env | 3 + src/Data/Payload.php | 20 ++--- src/Listener/InterceptListener.php | 2 +- src/Listener/LoginListener.php | 30 ++++--- src/Service/BackupCodeManager.php | 13 +-- src/Service/LoginManager.php | 5 +- templates/_script.html.twig | 140 ++++++++++++++++------------- templates/base.html.twig | 2 +- templates/login.html.twig | 20 +++-- 11 files changed, 136 insertions(+), 103 deletions(-) diff --git a/config/packages/twig.yaml b/config/packages/twig.yaml index 686a3a4..40a0c7c 100644 --- a/config/packages/twig.yaml +++ b/config/packages/twig.yaml @@ -15,3 +15,4 @@ twig: teapot_message: '%env(TEAPOT_MESSAGE)%' too_many_title: '%env(TOO_MANY_TITLE)%' too_many_message: '%env(TOO_MANY_MESSAGE)%' + debug: '%env(SHELL_VERBOSITY)%' diff --git a/config/services.yaml b/config/services.yaml index 73d54c5..844f338 100644 --- a/config/services.yaml +++ b/config/services.yaml @@ -51,6 +51,9 @@ parameters: env(TOO_MANY_TITLE): 'Too many requests' env(TOO_MANY_MESSAGE): 'Try again later' + # --- debug options --- + env(SHELL_VERBOSITY): '0' # set to 3 to log debug + # --- application variables --- app.totp_uri: '%env(TOTP_URI)%' app.cookie_ttl: '%env(COOKIE_TTL)%' diff --git a/docs/example.env b/docs/example.env index 02303ab..4fe39a8 100644 --- a/docs/example.env +++ b/docs/example.env @@ -50,3 +50,6 @@ #TOO_MANY_TITLE='Too many requests' #TOO_MANY_MESSAGE='Try again later' +# --- debug options --- +#SHELL_VERBOSITY=0 # set to "3" to log debug + diff --git a/src/Data/Payload.php b/src/Data/Payload.php index 65bbc4c..77ca1a3 100644 --- a/src/Data/Payload.php +++ b/src/Data/Payload.php @@ -31,11 +31,11 @@ final class Payload { public static function load(InputBag $input): ?Payload { /* convert form data into real data */ - if ($input->has('preauth_nonce') && $input->has('preauth_id') && $input->has('preauth_token')) { + if ($input->has('username') && $input->has('nonce') && $input->has('totp')) { return Payload::create((object)[ - 'id' => $input->get('preauth_id'), - 'nonce' => $input->get('preauth_nonce'), - 'token' => $input->get('preauth_token'), + 'id' => $input->get('username'), + 'nonce' => $input->get('nonce'), + 'token' => $input->get('totp'), 'json' => false, ]); } @@ -44,9 +44,9 @@ final class Payload { public static function create(object $data): ?Payload { /* if missing required fields id, nonce, or token */ - if (strlen($data->id ?? '') < 1 || - strlen($data->nonce ?? '') < 1 || - strlen($data->token ?? '') < 1 + if (strlen(trim($data->id ?? '')) < 1 || + strlen(trim($data->nonce ?? '')) < 1 || + strlen(trim($data->token ?? '')) < 1 ) { /* returns null as the input is invalid */ return null; @@ -54,11 +54,11 @@ final class Payload { /* all input is limited */ $payload = new Payload(); - $payload->id = mb_substr($data->id, 0, 128); - $payload->nonce = mb_substr($data->nonce, 0, 128); + $payload->id = mb_substr(trim($data->id), 0, 128); + $payload->nonce = mb_substr(trim($data->nonce), 0, 128); $payload->json = ($data->json ?? true); $payload->scope = Scope::tryFrom($data->scope ?? '') ?? Scope::Cookie; - $payload->token = mb_substr($data->token, 0, 128); + $payload->token = mb_substr(trim($data->token), 0, 128); return Payload::constrict($payload); } diff --git a/src/Listener/InterceptListener.php b/src/Listener/InterceptListener.php index 7c14f8a..33506b7 100644 --- a/src/Listener/InterceptListener.php +++ b/src/Listener/InterceptListener.php @@ -40,7 +40,7 @@ final readonly class InterceptListener { ) { /* host matches base-domain of auth, but not on auth subdomain, redirect */ $query = http_build_query(['return' => $event->getRequest()->getUri()]); - $event->setResponse(new Response('', Response::HTTP_TEMPORARY_REDIRECT, + $event->setResponse(new Response('', Response::HTTP_SEE_OTHER, ['Location' => "https://{$this->domainManager->getAuthSubdomain()}/?$query"] )); } else { diff --git a/src/Listener/LoginListener.php b/src/Listener/LoginListener.php index e97423f..cc3095e 100644 --- a/src/Listener/LoginListener.php +++ b/src/Listener/LoginListener.php @@ -10,6 +10,7 @@ use App\Service\LoginManager; use App\Trait\CookieNameTrait; use App\Trait\HasLoggerTrait; use App\Trait\MakeNonceTrait; +use App\Trait\StringTrait; use Psr\Cache\InvalidArgumentException; use Symfony\Component\DependencyInjection\Attribute\Target; use Symfony\Component\EventDispatcher\Attribute\AsEventListener; @@ -26,6 +27,7 @@ final readonly class LoginListener { use CookieNameTrait; use HasLoggerTrait; use MakeNonceTrait; + use StringTrait; private RateLimiterFactoryInterface $rateLimiter; @@ -42,7 +44,9 @@ final readonly class LoginListener { /** @throws InvalidArgumentException|LoaderError|RuntimeError|SyntaxError */ #[AsEventListener(priority: 66)] public function onKernelRequest(RequestEvent $event): void { - $payload = null; + $payload = null; + $response = null; + if ($event->getRequest()->headers->has($this->headerName())) { /* if request contains our "X-Preauth" header */ $data = $event->getRequest()->headers->get($this->headerName()); @@ -52,19 +56,20 @@ final readonly class LoginListener { ) { /* if request is a POST to the auth-subdomain */ $payload = Payload::load($event->getRequest()->getPayload()); - } - - if ( ! $payload) { - /* user is not attempting to log in */ + } else { + /* no login attempt detected */ return; } - $response = $this->loginManager->checkToken($payload, $event->getRequest()); + if ($payload) { + /* user sent a valid payload, check it */ + $response = $this->loginManager->checkToken($payload, $event->getRequest()); - /* token or backup-code authentication was successful */ - if ($response) { - $event->setResponse($response); - return; + /* token or backup-code authentication was successful */ + if ($response) { + $event->setResponse($response); + return; + } } /* login attempted but unsuccessful, log and block if needed */ @@ -72,7 +77,7 @@ final readonly class LoginListener { $this->logger->debug("logging failure for: {$event->getRequest()->getClientIp()}"); $event->setResponse($this->makeFailedResponse($limitReached, $payload->json ?? true, - $event->getRequest()->getHost() + $event->getRequest()->getHost(), $this->makeCacheKey($payload ? $payload->id : '') )); } @@ -82,7 +87,7 @@ final readonly class LoginListener { } /** @throws InvalidArgumentException|RuntimeError|SyntaxError|LoaderError */ - private function makeFailedResponse(bool $limited, bool $json, string $host): Response { + private function makeFailedResponse(bool $limited, bool $json, string $host, string $username): Response { if ($limited) { $status = $this->config->teapot() ? Response::HTTP_I_AM_A_TEAPOT : Response::HTTP_TOO_MANY_REQUESTS; @@ -96,6 +101,7 @@ final readonly class LoginListener { 'message' => $message, 'nonce' => $this->makeNonce(), 'post' => $this->domainManager->getAuthSubdomain() === $host, + 'username' => $username, ]; if ($json) { diff --git a/src/Service/BackupCodeManager.php b/src/Service/BackupCodeManager.php index 0ead9c7..d7863ef 100644 --- a/src/Service/BackupCodeManager.php +++ b/src/Service/BackupCodeManager.php @@ -40,12 +40,12 @@ final readonly class BackupCodeManager { $codes = []; for ($i = 0; $i < $count; $i++) { /* output is alphanumeric string of given length */ - $codes[] = str_pad(substr(base_convert(bin2hex(random_bytes($length)), - 16, 36), 0, $length), - $length, '0', STR_PAD_LEFT); + $codes[] = strtolower(str_pad(substr(base_convert(bin2hex( + random_bytes($length) + ), 16, 36), 0, $length), $length, '0', STR_PAD_LEFT)); } $this->saveCodes($codes); - $this->logger->info("generated {$count} backup codes}"); + $this->logger->info("generated {$count} backup codes"); return $codes; } @@ -67,7 +67,10 @@ final readonly class BackupCodeManager { * @return bool true if the code is valid and unused * @throws InvalidArgumentException */ public function verifyAndConsume(string $code): bool { - $backupItem = $this->sessionCache->getItem($this->makeCacheKey(strtolower("backup_$code"))); + /* remove unallowed characters, since backup codes are case-insensitive alphanumeric */ + $backupKey = 'backup_' . preg_replace('/[^a-z0-9]+/', '', strtolower($code)); + $backupItem = $this->sessionCache->getItem($this->makeCacheKey($backupKey)); + $this->logger->debug("checking backup code '{$backupKey}': " . ($backupItem->isHit() ? 'HIT & ' : 'miss & ') . ($backupItem->get() ? 'VALID' : 'invalid')); if ($backupItem->isHit() && $backupItem->get()) { $this->logger->debug("valid backup code"); /* mark backup code as spent */ diff --git a/src/Service/LoginManager.php b/src/Service/LoginManager.php index 1dd4ea7..85dd8f2 100644 --- a/src/Service/LoginManager.php +++ b/src/Service/LoginManager.php @@ -89,10 +89,9 @@ final readonly class LoginManager { "{$request->query->get('return')}" : "{$request->getPathInfo()}{$request->getQueryString()}"; - /* when using central auth, force redirect to use GET method */ + /* force redirect to use GET method (important when using central auth) */ $response->setContent($content) - ->setStatusCode($this->domainManager->getAuthSubdomain() === $request->getHost() ? - Response::HTTP_SEE_OTHER : Response::HTTP_TEMPORARY_REDIRECT) + ->setStatusCode(Response::HTTP_SEE_OTHER) ->headers->set('Location', $location); $response->headers->set('Content-Type', $contentType); } diff --git a/templates/_script.html.twig b/templates/_script.html.twig index 2c31bac..b88b91e 100644 --- a/templates/_script.html.twig +++ b/templates/_script.html.twig @@ -1,68 +1,84 @@ diff --git a/templates/base.html.twig b/templates/base.html.twig index 41ade42..e80ab9e 100644 --- a/templates/base.html.twig +++ b/templates/base.html.twig @@ -4,7 +4,7 @@ {{ env.title }} - {{ include('_style.html.twig') }} + {{- include('_style.html.twig') -}} {% block content %}{% endblock %} diff --git a/templates/login.html.twig b/templates/login.html.twig index 4bb72bf..ba00f27 100644 --- a/templates/login.html.twig +++ b/templates/login.html.twig @@ -3,15 +3,17 @@ {% block content %}

{{ env.title }}

{{ message|default }}

-
- -
-
-
-
+ + +
+
+
+
-{% if not post ?? false %}{{ include('_script.html.twig') }}{% endif %} +{% if not post ?? false %} + {{- include('_script.html.twig') -}} +{% endif %} {% endblock %}