From 69ee5e99aaeb89d9331da9b355e057d94be0d416 Mon Sep 17 00:00:00 2001 From: Lyra Bot Date: Sun, 27 Sep 2026 11:29:17 +0000 Subject: [PATCH] Route passkey registration through the TOTP check, not the listener Corrects a design error in the previous commit. I had exposed register-begin as a listener operation and gated it on a session cookie, but the approved flow has no session at that point: the whole point is that a valid TOTP code is what authorises registration, and the session is only issued once the new credential has been verified. Two consequences, both bad: - There is no session cookie to check, so the gate could never have worked. It would have been dead code that looked like a security control. - More seriously, a listener-side register-begin would hand out a challenge without proving anything. Anyone could obtain ceremony options and attempt registration. The cookie check was not a weak control; the operation itself was the hole. Registration is now started by LoginManager, after it has verified both the code and the nonce, and its options are returned with the login response. That is the flow in the plan, and it keeps nonce validation in the one place that already enforces it. The capability for register-finish is the single-use ceremonyId, which is server-issued and bound to the identity that passed the check. The listener now serves three operations, and a test pins that register-begin is not one of them. Also adds Payload::$register so the checkbox intent survives from the form to LoginManager, and moves the ceremony marker constant to AppConstants since both LoginManager and the listener now produce marked responses. --- src/AppConstants.php | 14 ++++ src/Data/Payload.php | 13 ++++ src/Listener/PasskeyListener.php | 78 +++++-------------- src/Listener/SecurityHeadersListener.php | 5 +- src/Service/LoginManager.php | 53 +++++++++++-- tests/Unit/Listener/PasskeyListenerTest.php | 48 +++--------- .../Listener/SecurityHeadersListenerTest.php | 8 +- tests/Unit/Service/LoginManagerTest.php | 5 +- 8 files changed, 116 insertions(+), 108 deletions(-) diff --git a/src/AppConstants.php b/src/AppConstants.php index 8bbe172..d2be27a 100644 --- a/src/AppConstants.php +++ b/src/AppConstants.php @@ -22,4 +22,18 @@ final class AppConstants * Also used for cache key truncation. */ public const int MAX_INPUT_LENGTH = 128; + + /** + * Marks a response as WebAuthn ceremony output. + * + * A ceremony reply is the only 2xx this application returns straight to a + * browser — every other 2xx is consumed by the reverse proxy's forward_auth + * check. So it is the only one that needs the no-store treatment, and this + * marker is how `SecurityHeadersListener` recognises it without the caching + * policy being duplicated at each site that produces one. + * + * It lives here rather than on a listener because both `PasskeyListener` + * (finish) and `LoginManager` (the registration hand-off) produce them. + */ + public const string PASSKEY_CEREMONY_MARKER = 'X-Preauth-Ceremony'; } diff --git a/src/Data/Payload.php b/src/Data/Payload.php index 2ced1f8..ffd11de 100644 --- a/src/Data/Payload.php +++ b/src/Data/Payload.php @@ -17,6 +17,17 @@ final class Payload public bool $json; /* should we return json (for the login page) */ public Scope $scope; /* type of access being requested */ + /** + * The caller ticked "register this device as a passkey". + * + * Carried on the payload rather than handled by the listener, because the + * TOTP check is what authorises registration — so the intent has to reach + * `LoginManager`, which is where that check (and the nonce check) already + * happens. Starting a ceremony any earlier would move nonce validation and + * risk spending it twice. + */ + public bool $register = false; + public static function decode(string $base64url): ?self { /* convert the base64url into json string */ @@ -42,6 +53,7 @@ final class Payload 'id' => $input->get('username'), 'nonce' => $input->get('nonce'), 'token' => $input->get('totp'), + 'register' => $input->get('register'), 'json' => false, ]); } @@ -65,6 +77,7 @@ final class Payload $payload->id = mb_substr(trim($data->id), 0, AppConstants::MAX_INPUT_LENGTH); $payload->nonce = mb_substr(trim($data->nonce), 0, AppConstants::MAX_INPUT_LENGTH); $payload->json = ($data->json ?? true); + $payload->register = (bool) ($data->register ?? false); $payload->scope = Scope::tryFrom($data->scope ?? '') ?? Scope::Cookie; $payload->token = mb_substr(trim($data->token), 0, AppConstants::MAX_INPUT_LENGTH); diff --git a/src/Listener/PasskeyListener.php b/src/Listener/PasskeyListener.php index 1782d28..1557aba 100644 --- a/src/Listener/PasskeyListener.php +++ b/src/Listener/PasskeyListener.php @@ -4,17 +4,16 @@ declare(strict_types=1); namespace App\Listener; +use App\AppConstants; use App\ConfigBag; use App\Enum\Scope; use App\Service\DomainInterface; use App\Service\PasskeyInterface; use App\Service\PasskeyPolicyInterface; use App\Service\SessionIssuerInterface; -use App\Trait\CookieNameTrait; use App\Trait\HasLoggerTrait; use App\Trait\MakeNonceTrait; use App\Trait\StringTrait; -use Psr\Cache\CacheItemPoolInterface; use Psr\Cache\InvalidArgumentException; use Symfony\Component\DependencyInjection\Attribute\Target; use Symfony\Component\EventDispatcher\Attribute\AsEventListener; @@ -36,13 +35,21 @@ use Symfony\Component\RateLimiter\RateLimiterFactoryInterface; * returns null and the request would be scored as a failed login — burning a * rate-limit token for every legitimate passkey login. * + * **Three operations, not four.** `register-begin` is deliberately *absent*: a + * registration ceremony may only be started after a valid TOTP code, which is + * presented to `LoginManager` as part of the form submission. `LoginManager` + * therefore starts that ceremony and returns its options with the login + * response. Exposing `register-begin` here would be a way to obtain a challenge + * **without proving anything**, which is the vulnerability rather than the + * feature — there is no session cookie to check at that point either, since the + * whole flow is what *produces* the session. + * * **Every** request carrying the dispatch header gets a response, including * malformed ones. Falling through would let `InterceptListener` render HTML to a * `fetch()` caller. */ final readonly class PasskeyListener { - use CookieNameTrait; use HasLoggerTrait; use MakeNonceTrait; use StringTrait; @@ -55,16 +62,11 @@ final readonly class PasskeyListener */ public const string HEADER = 'X-Preauth-Passkey'; - /** Marks a response as ceremony output, so the caching policy can see it. */ - public const string CEREMONY_MARKER = 'X-Preauth-Ceremony'; + public const string BEGIN_LOGIN = 'login-begin'; - private const string BEGIN_LOGIN = 'login-begin'; + public const string FINISH_LOGIN = 'login-finish'; - private const string FINISH_LOGIN = 'login-finish'; - - private const string BEGIN_REGISTER = 'register-begin'; - - private const string FINISH_REGISTER = 'register-finish'; + public const string FINISH_REGISTER = 'register-finish'; private RateLimiterFactoryInterface $beginLimiter; @@ -73,7 +75,6 @@ final readonly class PasskeyListener public function __construct( #[Target('passkey_begin_burst')] RateLimiterFactoryInterface $beginLimiter, #[Target('login_limiter')] RateLimiterFactoryInterface $loginLimiter, - #[Target('sessionCache')] private CacheItemPoolInterface $sessionCache, private PasskeyInterface $passkeys, private PasskeyPolicyInterface $policy, private SessionIssuerInterface $sessionIssuer, @@ -116,7 +117,6 @@ final readonly class PasskeyListener return $this->ceremonyResponse(match ($operation) { self::BEGIN_LOGIN => $this->beginLogin($request), self::FINISH_LOGIN => $this->finishLogin($request), - self::BEGIN_REGISTER => $this->beginRegistration($request), self::FINISH_REGISTER => $this->finishRegistration($request), default => $this->error('Unknown passkey operation.', $request), }); @@ -131,25 +131,6 @@ final readonly class PasskeyListener return $this->json($this->passkeys->beginLogin()); } - /** - * Registration is only offered to someone who already authenticated: the - * identity comes from the live session, never from the request body, so a - * caller cannot register a passkey for an identity it does not hold. - */ - private function beginRegistration(Request $request): Response - { - $identity = $this->identityFromRequest($request); - if (null === $identity) { - return $this->error('Registration requires a completed login.', $request); - } - - if ($limit = $this->beginBurstExceeded($request)) { - return $limit; - } - - return $this->json($this->passkeys->beginRegistration($identity)); - } - private function finishLogin(Request $request): Response { $credential = $this->passkeys->finishLogin($this->body($request)); @@ -165,6 +146,12 @@ final readonly class PasskeyListener return $this->sessionIssuer->issue($credential->identity, Scope::Cookie, $request, true); } + /** + * The ceremony was authorised by the TOTP-gated hand-off in `LoginManager`, + * so the capability here is the single-use `ceremonyId` itself: it is + * server-issued, stored against the identity that passed the check, and + * consumed on use. + */ private function finishRegistration(Request $request): Response { $credential = $this->passkeys->finishRegistration($this->body($request)); @@ -178,31 +165,6 @@ final readonly class PasskeyListener return $this->sessionIssuer->issue($credential->identity, Scope::Cookie, $request, true); } - /** - * The identity of an already-authenticated caller, for registration. - * - * Read from the live session cookie, so `register-begin` is reachable only by - * someone who has just passed the TOTP check. Null when there is no session. - * - * @throws InvalidArgumentException - */ - private function identityFromRequest(Request $request): ?string - { - $cookie = $request->cookies->get($this->sessionCookieName($this->domainManager)); - if (!\is_string($cookie) || '' === $cookie) { - return null; - } - - $item = $this->sessionCache->getItem($this->makeCacheKey("cookie_$cookie")); - if (!$item->isHit()) { - return null; - } - - $identity = $item->get(); - - return \is_string($identity) && '' !== $identity ? $identity : null; - } - /** * Bounds how many ceremonies one caller can start. * @@ -266,7 +228,7 @@ final readonly class PasskeyListener */ private function ceremonyResponse(Response $response): Response { - $response->headers->set(self::CEREMONY_MARKER, '1'); + $response->headers->set(AppConstants::PASSKEY_CEREMONY_MARKER, '1'); $response->headers->set('Content-Type', 'application/json'); return $response; diff --git a/src/Listener/SecurityHeadersListener.php b/src/Listener/SecurityHeadersListener.php index 7bafa54..ef17df5 100644 --- a/src/Listener/SecurityHeadersListener.php +++ b/src/Listener/SecurityHeadersListener.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace App\Listener; +use App\AppConstants; use App\Service\DomainInterface; use App\Service\PasskeyPolicyInterface; use Symfony\Component\EventDispatcher\Attribute\AsEventListener; @@ -98,8 +99,8 @@ final readonly class SecurityHeadersListener return; } - if ($headers->has(PasskeyListener::CEREMONY_MARKER)) { - $headers->remove(PasskeyListener::CEREMONY_MARKER); + if ($headers->has(AppConstants::PASSKEY_CEREMONY_MARKER)) { + $headers->remove(AppConstants::PASSKEY_CEREMONY_MARKER); $this->applyNoStore($headers); } } diff --git a/src/Service/LoginManager.php b/src/Service/LoginManager.php index 5901d54..80e5b91 100644 --- a/src/Service/LoginManager.php +++ b/src/Service/LoginManager.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace App\Service; +use App\AppConstants; use App\Data\Payload; use App\Enum\Scope; use App\Trait\GetTotpTrait; @@ -13,14 +14,21 @@ use Override; use Psr\Cache\InvalidArgumentException; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; +use Throwable; /** - * Authenticates a TOTP code (or backup code) and, on success, grants access. + * Authenticates a TOTP code (or backup code) and, on success, either grants + * access or starts a passkey registration. * - * The "grant access" half now lives in {@see SessionIssuer} so the passkey - * ceremony produces an identical response. This class keeps the part that is - * genuinely specific to code-based login: verifying the code and enforcing the - * single-use nonce. + * The "grant access" half lives in {@see SessionIssuer} so the passkey ceremony + * produces an identical response. This class keeps the part genuinely specific + * to code-based login: verifying the code and enforcing the single-use nonce. + * + * **Why the registration hand-off lives here.** Ticking "register this device" + * turns the form submission into a registration ceremony, and the TOTP check is + * what authorises it. That check — and the nonce check — already happen here, so + * a ceremony started anywhere earlier would mean validating the nonce somewhere + * new and risking spending it twice. */ final readonly class LoginManager implements LoginInterface { @@ -31,6 +39,7 @@ final readonly class LoginManager implements LoginInterface public function __construct( private BackupCodeInterface $backupCodeManager, private SessionIssuerInterface $sessionIssuer, + private PasskeyInterface $passkeys, ) { } @@ -65,6 +74,12 @@ final readonly class LoginManager implements LoginInterface $nonceItem->expiresAfter(self::NONCE_TTL); /* keep briefly */ $this->nonceCache->save($nonceItem); + /* the code and the nonce are both good from here on */ + + if ($payload->register) { + return $this->startRegistration($payload); + } + return $this->sessionIssuer->issue( $payload->id, $payload->scope, @@ -72,4 +87,32 @@ final readonly class LoginManager implements LoginInterface $payload->json, ); } + + /** + * The registration hand-off: authorisation is already proven, so this issues + * the ceremony options back to the page instead of a session. + * + * `SessionIssuer` is deliberately not involved — the session is granted only + * once the new credential has actually been verified, at `register-finish`. + */ + private function startRegistration(Payload $payload): ?Response + { + try { + $payloadOut = $this->passkeys->beginRegistration($payload->id); + } catch (Throwable) { + /* a ceremony that cannot start must not become a 500 on the login + * page; falling through to the caller's failure path is the same + * treatment a wrong code gets */ + return null; + } + + $response = new Response( + (string) json_encode(['register' => $payloadOut]), + Response::HTTP_OK, + ['Content-Type' => 'application/json'], + ); + $response->headers->set(AppConstants::PASSKEY_CEREMONY_MARKER, '1'); + + return $response; + } } diff --git a/tests/Unit/Listener/PasskeyListenerTest.php b/tests/Unit/Listener/PasskeyListenerTest.php index 619394c..604c8d1 100644 --- a/tests/Unit/Listener/PasskeyListenerTest.php +++ b/tests/Unit/Listener/PasskeyListenerTest.php @@ -4,10 +4,10 @@ declare(strict_types=1); namespace App\Tests\Unit\Listener; +use App\AppConstants; use App\Data\PasskeyCredential; use App\Enum\Scope; use App\Listener\PasskeyListener; -use App\MonitorCacheKeys; use App\Service\DomainInterface; use App\Service\PasskeyInterface; use App\Service\PasskeyPolicyInterface; @@ -41,8 +41,6 @@ final class PasskeyListenerTest extends TestCase private const string AUTH_HOST = 'auth.example.com'; - private ?ArrayAdapter $sessionCache = null; - private function makeListener( ?PasskeyInterface $passkeys = null, bool $available = true, @@ -50,8 +48,6 @@ final class PasskeyListenerTest extends TestCase ?DomainInterface $domainManager = null, int $beginLimit = 30, ): PasskeyListener { - $this->sessionCache = new ArrayAdapter(); - $policy = $this->createStub(PasskeyPolicyInterface::class); $policy->method('isAvailableFor')->willReturn($available); @@ -60,7 +56,6 @@ final class PasskeyListenerTest extends TestCase $listener = new PasskeyListener( new RateLimiterFactory(['id' => 'passkey_begin_burst', 'policy' => 'sliding_window', 'limit' => $beginLimit, 'interval' => '60 seconds'], new InMemoryStorage()), new RateLimiterFactory(['id' => 'login_limiter', 'policy' => 'sliding_window', 'limit' => 2, 'interval' => '60 seconds'], new InMemoryStorage()), - $this->sessionCache, $passkeys ?? $this->createStub(PasskeyInterface::class), $policy, $sessionIssuer ?? $this->createStub(SessionIssuerInterface::class), @@ -252,16 +247,22 @@ final class PasskeyListenerTest extends TestCase $listener->onKernelRequest($event); - self::assertSame('1', $event->getResponse()?->headers->get(PasskeyListener::CEREMONY_MARKER)); + self::assertSame('1', $event->getResponse()?->headers->get(AppConstants::PASSKEY_CEREMONY_MARKER)); } /* ── registration gating ──────────────────────────────────────────── */ /** - * Registration requires a live session: the identity comes from the cookie, - * never from the body, so nobody can register a passkey for another identity. + * /** + * `register-begin` deliberately has no handler here. + * + * A registration ceremony may only start after a valid TOTP code, which is + * presented to `LoginManager` as part of the form submission — so + * `LoginManager` starts it. Exposing it on this listener would hand out a + * challenge without proving anything, which is precisely the hole the + * design closes. This test pins that the operation is *not* honoured. */ - public function test_register_begin_without_a_session_is_refused(): void + public function test_register_begin_is_not_a_listener_operation(): void { $passkeys = $this->createMock(PasskeyInterface::class); $passkeys->expects(self::never())->method('beginRegistration'); @@ -274,33 +275,6 @@ final class PasskeyListenerTest extends TestCase self::assertSame(Response::HTTP_UNAUTHORIZED, $event->getResponse()?->getStatusCode()); } - public function test_register_begin_uses_the_identity_from_the_session_cookie(): void - { - $passkeys = $this->createMock(PasskeyInterface::class); - $passkeys->expects(self::once()) - ->method('beginRegistration') - ->with('lyra') - ->willReturn(['publicKey' => [], 'ceremonyId' => 'cid']); - - $listener = $this->makeListener($passkeys); - - /* seed a live session the way SessionIssuer would have; this has to - * happen after makeListener(), which builds the pool the listener holds */ - $ulid = 'test-ulid'; - $monitor = new MonitorCacheKeys($this->sessionCache); - $item = $monitor->getItem($this->makeCacheKey("cookie_$ulid")); - $item->set('lyra'); - $monitor->save($item); - - $request = $this->ceremonyRequest('register-begin'); - $request->cookies->set('__Http-Domain-Preauth', $ulid); - - $event = $this->makeEvent($request); - $listener->onKernelRequest($event); - - self::assertSame(Response::HTTP_OK, $event->getResponse()?->getStatusCode()); - } - /* ── failures share the login budget (D3) ─────────────────────────── */ /** diff --git a/tests/Unit/Listener/SecurityHeadersListenerTest.php b/tests/Unit/Listener/SecurityHeadersListenerTest.php index 633605f..87b4dd1 100644 --- a/tests/Unit/Listener/SecurityHeadersListenerTest.php +++ b/tests/Unit/Listener/SecurityHeadersListenerTest.php @@ -4,7 +4,7 @@ declare(strict_types=1); namespace App\Tests\Unit\Listener; -use App\Listener\PasskeyListener; +use App\AppConstants; use App\Listener\SecurityHeadersListener; use App\Service\DomainInterface; use App\Service\PasskeyPolicyInterface; @@ -252,11 +252,11 @@ final class SecurityHeadersListenerTest extends TestCase { $listener = $this->makeListener('auth.example.com'); $response = new Response('{}', Response::HTTP_OK); - $response->headers->set(PasskeyListener::CEREMONY_MARKER, '1'); + $response->headers->set(AppConstants::PASSKEY_CEREMONY_MARKER, '1'); $listener->onKernelResponse($this->makeEvent($response)); - self::assertFalse($response->headers->has(PasskeyListener::CEREMONY_MARKER)); + self::assertFalse($response->headers->has(AppConstants::PASSKEY_CEREMONY_MARKER)); self::assertStringContainsString('no-store', (string) $response->headers->get('Cache-Control')); self::assertSame('*', $response->headers->get('Vary')); } @@ -279,6 +279,6 @@ final class SecurityHeadersListenerTest extends TestCase self::assertStringNotContainsString('no-store', $cacheControl); self::assertStringContainsString('max-age=60', $cacheControl); self::assertFalse($response->headers->has('Surrogate-Control')); - self::assertFalse($response->headers->has(PasskeyListener::CEREMONY_MARKER)); + self::assertFalse($response->headers->has(AppConstants::PASSKEY_CEREMONY_MARKER)); } } diff --git a/tests/Unit/Service/LoginManagerTest.php b/tests/Unit/Service/LoginManagerTest.php index 607adb2..d3f76e4 100644 --- a/tests/Unit/Service/LoginManagerTest.php +++ b/tests/Unit/Service/LoginManagerTest.php @@ -9,6 +9,7 @@ use App\Enum\Scope; use App\Service\BackupCodeInterface; use App\Service\DomainManager; use App\Service\LoginManager; +use App\Service\PasskeyInterface; use App\Service\SessionIssuer; use App\Tests\Support\TotpTestHelper; use App\Trait\StringTrait; @@ -45,7 +46,7 @@ final class LoginManagerTest extends TestCase $this->sessionIssuer = new SessionIssuer($this->pool, $this->domainManager, $this->makeConfig(ipTtl: $ipTtl)); $this->sessionIssuer->setLogger(new NullLogger()); - $manager = new LoginManager($this->backupCodeManager, $this->sessionIssuer); + $manager = new LoginManager($this->backupCodeManager, $this->sessionIssuer, $this->createStub(PasskeyInterface::class)); $manager->setConfig($this->makeConfig(ipTtl: $ipTtl)); $manager->setLogger(new NullLogger()); $manager->setNonceCache(new ArrayAdapter()); @@ -375,7 +376,7 @@ final class LoginManagerTest extends TestCase $issuer = new SessionIssuer($pool, $this->domainManager, $this->makeConfig()); $issuer->setLogger(new NullLogger()); - $manager = new LoginManager($this->backupCodeManager, $issuer); + $manager = new LoginManager($this->backupCodeManager, $issuer, $this->createStub(PasskeyInterface::class)); $manager->setConfig($this->makeConfig()); $manager->setLogger(new NullLogger()); $manager->setNonceCache(new ArrayAdapter());