From 67043e324986cfd0ed23db97baffd34641c9de24 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 12 Aug 2026 12:15:51 -0300 Subject: [PATCH 1/2] fix: MFA-enforced accounts leak enumeration via passwordless login guard order Root cause: UserController::postLogin()'s shouldRequire2FA() guard ran before loginWithOTP() validated the submitted OTP, so a caller without the real code could distinguish MFA-enforced accounts from the rejection message alone - an account-enumeration oracle requiring no credentials. Fix: introduce AuthService::loginWithOTPEnforcing2FA(), which checks shouldRequire2FA() after the OTP is proven valid and before finalizeRedemption()/Auth::login() - so a guessed/invalid code is rejected generically before reaching the account-status branch, and a valid code against an enforced account is rejected before any login side effect (redemption, Auth::login, the Login event / queued PostLoginUser job) fires. loginWithOTP() (used by InteractiveGrantType and TokenService's OAuth2 grants) is unchanged - only UserController::postLogin()'s interactive web login now enforces 2FA at this layer. Avoids a boolean flag parameter (flags-over-objects antipattern) by exposing two explicitly-named public methods delegating to a shared, flag-free resolveOTPUser() helper. Adds a regression test proving an invalid OTP against an MFA-enforced account gets the same generic rejection as any other invalid code. --- app/Http/Controllers/UserController.php | 12 +-- app/libs/Auth/AuthService.php | 96 ++++++++++++++++++------ app/libs/Utils/Services/IAuthService.php | 12 +++ tests/TwoFactorLoginFlowTest.php | 22 ++++++ 4 files changed, 110 insertions(+), 32 deletions(-) diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index fdd3b482..532ad6fb 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -591,20 +591,10 @@ public function postLogin() if ($flow == IAuthService::AuthenticationFlowPasswordless) { - // Passwordless login is single-factor (email access only) and - // must not be usable to satisfy MFA enforcement (SDS idp-mfa.md - // §7.4 / Open Question #3). - $existing_user = $this->auth_service->getUserByUsername($username); - if (!is_null($existing_user) && $existing_user->shouldRequire2FA()) { - throw new AuthenticationException( - "This account requires password and two-factor authentication. Please use the password login option." - ); - } - $client = $this->resolveClientFromMemento(); $otpClaim = OAuth2OTP::fromParams($username, $connection, $password); - $this->auth_service->loginWithOTP($otpClaim, $client); + $this->auth_service->loginWithOTPEnforcing2FA($otpClaim, $client); // A completed login must not leave the OTP screen restorable // on a later refresh - same identity-leakage concern already // fixed for the MFA flow's verify2FA()/verify2FARecovery(). diff --git a/app/libs/Auth/AuthService.php b/app/libs/Auth/AuthService.php index f90af6cd..5473cce2 100644 --- a/app/libs/Auth/AuthService.php +++ b/app/libs/Auth/AuthService.php @@ -271,38 +271,92 @@ public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $ ); // TX-C: resolve or create user, finalize, login - return $this->tx_service->transaction(function () use ($otp, $otpClaim, $client, $remember) { + return $this->tx_service->transaction(function () use ($otp, $client, $remember) { + $user = $this->resolveOTPUser($otp); + $this->finalizeRedemption($otp, $user, $client); + Auth::login($user, $remember); + Log::debug(sprintf("AuthService::loginWithOTP user %s logged in.", $user->getId())); + return $otp; + }); + } - $user = $this->getUserByUsername($otp->getUserName()); + /** + * @param OAuth2OTP $otpClaim + * @param Client|null $client + * @param bool $remember + * @return OAuth2OTP|null + * @throws Exception + */ + public function loginWithOTPEnforcing2FA(OAuth2OTP $otpClaim, ?Client $client = null, bool $remember = false): ?OAuth2OTP + { + Log::debug(sprintf("AuthService::loginWithOTPEnforcing2FA otp %s user %s", $otpClaim->getValue(), $otpClaim->getUserName())); - if (is_null($user)) { - Log::debug(sprintf("AuthService::loginWithOTP user %s does not exist; auto-registering.", $otp->getUserName())); - $user = $this->auth_user_service->registerUser( - [ - 'email' => $otp->getUserName(), - 'email_verified' => true, - 'send_email_verified_notice' => false, - 'active' => true, - ], - $otp - ); - } else if ($user->isActive()) { - $user->verifyEmail(false); - } + $otp = $this->findAndValidateOTP( + $otpClaim->getValue(), + $otpClaim->getUserName(), + $otpClaim->getConnection(), + $otpClaim->getScope(), + $client + ); - if (!$user->canLogin()) { - Log::warning(sprintf("AuthService::loginWithOTP user %s cannot login (not active).", $user->getId())); - throw new AuthenticationException("We are sorry, your username or password does not match an existing record."); + // TX-C: resolve or create user, enforce 2FA, finalize, login + return $this->tx_service->transaction(function () use ($otp, $client, $remember) { + $user = $this->resolveOTPUser($otp); + + // Passwordless login is single-factor (email access only) and must not + // be usable to satisfy MFA enforcement (SDS idp-mfa.md §7.4 / Open + // Question #3). Checked here - after the OTP is proven valid, before + // finalizeRedemption()/Auth::login() - so neither a guessed code nor a + // rejected valid code ever triggers a login side effect (redemption, + // Auth::login, the Login event / queued PostLoginUser job). + if ($user->shouldRequire2FA()) { + throw new AuthenticationException( + "This account requires password and two-factor authentication. Please use the password login option." + ); } $this->finalizeRedemption($otp, $user, $client); - Auth::login($user, $remember); - Log::debug(sprintf("AuthService::loginWithOTP user %s logged in.", $user->getId())); + Log::debug(sprintf("AuthService::loginWithOTPEnforcing2FA user %s logged in.", $user->getId())); return $otp; }); } + /** + * Resolves the user for an already-validated passwordless OTP, auto-registering + * a brand-new email if needed. Does not finalize redemption or log in - callers + * decide that (and whether to enforce 2FA first). + * @param OAuth2OTP $otp + * @return User + * @throws AuthenticationException + */ + private function resolveOTPUser(OAuth2OTP $otp): User + { + $user = $this->getUserByUsername($otp->getUserName()); + + if (is_null($user)) { + Log::debug(sprintf("AuthService::resolveOTPUser user %s does not exist; auto-registering.", $otp->getUserName())); + $user = $this->auth_user_service->registerUser( + [ + 'email' => $otp->getUserName(), + 'email_verified' => true, + 'send_email_verified_notice' => false, + 'active' => true, + ], + $otp + ); + } else if ($user->isActive()) { + $user->verifyEmail(false); + } + + if (!$user->canLogin()) { + Log::warning(sprintf("AuthService::resolveOTPUser user %s cannot login (not active).", $user->getId())); + throw new AuthenticationException("We are sorry, your username or password does not match an existing record."); + } + + return $user; + } + /** * Verifies an OTP against an already-authenticated session user (MFA primitive). * diff --git a/app/libs/Utils/Services/IAuthService.php b/app/libs/Utils/Services/IAuthService.php index ce68d06c..a2ebd235 100644 --- a/app/libs/Utils/Services/IAuthService.php +++ b/app/libs/Utils/Services/IAuthService.php @@ -91,6 +91,18 @@ public function loginUser(User $user, bool $remember): void; */ public function loginWithOTP(OAuth2OTP $otpClaim, ?Client $client = null, bool $remember = false): ?OAuth2OTP; + /** + * Same as loginWithOTP(), but rejects the login when the resolved user has + * MFA enforced - passwordless (email-only) proof is not sufficient for an + * account that requires two-factor authentication. + * @param OAuth2OTP $otpClaim + * @param Client|null $client + * @param bool $remember + * @return OAuth2OTP|null + * @throws AuthenticationException + */ + public function loginWithOTPEnforcing2FA(OAuth2OTP $otpClaim, ?Client $client = null, bool $remember = false): ?OAuth2OTP; + /** * @param string $username diff --git a/tests/TwoFactorLoginFlowTest.php b/tests/TwoFactorLoginFlowTest.php index aa0abd56..457e25e1 100644 --- a/tests/TwoFactorLoginFlowTest.php +++ b/tests/TwoFactorLoginFlowTest.php @@ -213,6 +213,28 @@ public function testEnforcedUserCannotBypassMFAViaPasswordlessLogin(): void $this->assertSame('otp', Session::get('flow'), 'a reload must land back on the OTP screen, not silently fall back to password'); } + public function testInvalidOtpAgainstEnforcedUserDoesNotLeakMFAStatus(): void + { + // Regression guard: the enforcement check used to run BEFORE the OTP was + // validated, so an attacker submitting a garbage/guessed code against an + // MFA-enforced account's email got the distinguishing "requires password + // and two-factor authentication" message without ever proving control of + // the inbox - an account-enumeration oracle. The check must now only be + // reachable after loginWithOTPEnforcing2FA() has proven the code valid, + // so an invalid code gets the same generic rejection for any account. + $this->emitOTP(self::ADMIN_EMAIL); + + $response = $this->postLoginOTP(self::ADMIN_EMAIL, 'not-the-real-code'); + + $this->assertFalse(Auth::check(), 'an invalid code must never authenticate'); + $this->assertResponseStatus(302); + $this->assertStringNotContainsString( + 'two-factor authentication', + Session::get('flash_notice'), + 'an invalid code must not leak that the account is MFA-enforced' + ); + } + public function testNonEnforcedUserStillLogsInViaPasswordlessLogin(): void { $email = $this->createPlainUser(); From e6100e1ca41640848d1528a58e56ea65d55dab7d Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 12 Aug 2026 16:58:59 -0300 Subject: [PATCH 2/2] test: assert the passwordless rejection is indistinguishable, not just unworded testInvalidOtpAgainstEnforcedUserDoesNotLeakMFAStatus only asserted that the flash message lacked the phrase "two-factor authentication". Enumeration is about distinguishability, so that assertion still passed if a future enforced-only branch leaked a differently worded message. It now drives the same invalid code through the same endpoint for the MFA-enforced admin and for a non-enforced control user, and asserts the two rejections are byte-identical. The stale flash is cleared between requests so the comparison cannot read a value against itself. Verified by mutation: restoring the pre-fix guard order with reworded text fails the new assertSame and would have passed the old substring assertion. Also records why testEnforcedUserCannotBypassMFAViaPasswordlessLogin adds no separate "no login side effect" assertions. Both candidates were tried and removed as change detectors that cannot fail: the DB-visible effects (OTP redemption, sibling revocation) are rolled back by the AuthService transaction regardless of where the guard sits, and the session-visible one (Auth::login() and the Login event queuing PostLoginUser) is already caught first by the existing Auth::check() assertion. Both confirmed by mutation. Full file green: OK (58 tests, 380 assertions). --- tests/TwoFactorLoginFlowTest.php | 43 +++++++++++++++++++++++++++++--- 1 file changed, 40 insertions(+), 3 deletions(-) diff --git a/tests/TwoFactorLoginFlowTest.php b/tests/TwoFactorLoginFlowTest.php index 457e25e1..eb6145aa 100644 --- a/tests/TwoFactorLoginFlowTest.php +++ b/tests/TwoFactorLoginFlowTest.php @@ -193,6 +193,15 @@ public function testNonAdminWithoutMFALogsInNormally(): void public function testEnforcedUserCannotBypassMFAViaPasswordlessLogin(): void { + // No separate "no login side effect" assertions here on purpose: the + // DB-visible ones (OTP redemption, sibling revocation) are unreachable + // by construction, because the guard throws inside the AuthService + // transaction and DoctrineTransactionService rolls the whole closure + // back - moving the guard below finalizeRedemption() leaves the OTP + // un-redeemed all the same. The session-visible one (Auth::login() and + // the Login event that queues PostLoginUser) is already covered by the + // Auth::check() assertion below, which is what fails first if the guard + // is moved below Auth::login(). Both were verified by mutation. $this->emitOTP(self::ADMIN_EMAIL); $code = $this->latestOtpCode(self::ADMIN_EMAIL); @@ -222,15 +231,43 @@ public function testInvalidOtpAgainstEnforcedUserDoesNotLeakMFAStatus(): void // the inbox - an account-enumeration oracle. The check must now only be // reachable after loginWithOTPEnforcing2FA() has proven the code valid, // so an invalid code gets the same generic rejection for any account. - $this->emitOTP(self::ADMIN_EMAIL); + // + // Enumeration is about DISTINGUISHABILITY, not about one phrase: merely + // asserting the absence of "two-factor authentication" would still pass + // if some future enforced-only branch leaked a *differently* worded + // message. So the enforced account's rejection is compared byte-for-byte + // against a non-enforced control driven through the same endpoint with + // the same bad code. + $control_email = $this->createPlainUser(); - $response = $this->postLoginOTP(self::ADMIN_EMAIL, 'not-the-real-code'); + $this->emitOTP(self::ADMIN_EMAIL); + $this->postLoginOTP(self::ADMIN_EMAIL, 'not-the-real-code'); $this->assertFalse(Auth::check(), 'an invalid code must never authenticate'); $this->assertResponseStatus(302); + $enforced_notice = Session::get('flash_notice'); + $this->assertNotNull($enforced_notice, 'the enforced account must get a flashed rejection'); + + // Cleared so the control's assertion cannot silently read the enforced + // account's leftover flash and compare a value against itself. + Session::forget('flash_notice'); + + $this->emitOTP($control_email); + $this->postLoginOTP($control_email, 'not-the-real-code'); + + $this->assertFalse(Auth::check(), 'an invalid code must never authenticate the control account either'); + $this->assertResponseStatus(302); + $control_notice = Session::get('flash_notice'); + $this->assertNotNull($control_notice, 'the control account must get a flashed rejection'); + + $this->assertSame( + $control_notice, + $enforced_notice, + 'an invalid code must produce an identical rejection for an enforced and a non-enforced account - any difference is an enumeration oracle' + ); $this->assertStringNotContainsString( 'two-factor authentication', - Session::get('flash_notice'), + $enforced_notice, 'an invalid code must not leak that the account is MFA-enforced' ); }