From cfb66142a9976b0f673e1ae8e258e6d35737a320 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 22 Sep 2026 09:24:00 -0300 Subject: [PATCH 1/9] fix(reload-session): reload session from user_id provided from id token Signed-off-by: smarcet --- app/libs/Auth/AuthService.php | 56 ++++++++++++++----- .../GrantTypes/InteractiveGrantType.php | 19 ++++++- app/libs/Utils/Services/IAuthService.php | 2 +- 3 files changed, 60 insertions(+), 17 deletions(-) diff --git a/app/libs/Auth/AuthService.php b/app/libs/Auth/AuthService.php index 928c5af8..3d113340 100644 --- a/app/libs/Auth/AuthService.php +++ b/app/libs/Auth/AuthService.php @@ -665,32 +665,62 @@ public function getLoggedRPs(): array /** * @param string $jti - * @throws Exception + * @param string|null $user_id + * @return void + * @throws ReloadSessionException */ - public function reloadSession(string $jti): void + public function reloadSession(string $jti, string $user_id = null): void { + $former_session_id = Session::getId(); Log::debug(sprintf("AuthService::reloadSession jti %s", $jti)); $session_id = $this->cache_service->getSingleValue($jti); Log::debug(sprintf("AuthService::reloadSession session_id %s", $session_id)); - if (empty($session_id)) + if (empty($session_id)) { + Log::warning("AuthService::reloadSession session_id is not present at cache"); + if(!is_null($user_id)) { + Log::warning(sprintf("AuthService::reloadSession user id provided %s", $user_id)); + $user = $this->getUserById($user_id); + if (is_null($user)) + throw new ReloadSessionException('user not found!'); + Auth::login($user); + return; + } throw new ReloadSessionException('session not found!'); + } if ($this->cache_service->exists($session_id . "invalid")) { // session was marked as void, check if we are authenticated - if (!Auth::check()) + if (!Auth::check()) { + Session::setId($former_session_id); + Session::start(); throw new ReloadSessionException('user not found!'); + } } - Session::setId(Crypt::decrypt($session_id)); - Session::start(); - if (!Auth::check()) { - $user_id = $this->principal_service->get()->getUserId(); - Log::debug(sprintf("AuthService::reloadSession user_id %s", $user_id)); - $user = $this->getUserById($user_id); - if (is_null($user)) - throw new ReloadSessionException('user not found!'); - Auth::login($user); + try { + Session::setId(Crypt::decrypt($session_id)); + Session::start(); + if (!Auth::check()) { + $session_user_id = $this->principal_service->get()->getUserId(); + Log::debug(sprintf("AuthService::reloadSession user_id %s", $session_user_id)); + $user = $this->getUserById($session_user_id); + if (is_null($user)) + throw new ReloadSessionException('user not found!'); + Auth::login($user); + } + } + catch (ReloadSessionException $ex) { + Log::warning(sprintf("AuthService::reloadSession ex %s", $ex->getMessage())); + Session::setId($former_session_id); + Session::start(); + if(!is_null($user_id)) { + Log::warning(sprintf("AuthService::reloadSession user id provided %s", $user_id)); + $user = $this->getUserById($user_id); + if (is_null($user)) + throw new ReloadSessionException('user not found!'); + Auth::login($user); + } } } diff --git a/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php b/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php index b1dbfa52..993e93a2 100644 --- a/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php +++ b/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php @@ -547,11 +547,24 @@ protected function processUserHint(OAuth2AuthenticationRequest $request, Client $sub = $jwt->getClaimSet()->getSubject(); $user_id = $this->auth_service->unwrapUserId($sub->getString()); $user = $this->auth_service->getUserById($user_id); + $jti = $jwt->getClaimSet()->getJWTID(); - $jti = $jwt->getClaimSet()->getJWTID(); - if(is_null($jti)) throw new InvalidLoginHint('invalid jti!'); + if(is_null($jti)) { + $this->log_service->debug_msg("InteractiveGrantType::processUserHint: jti is null"); + throw new InvalidLoginHint('invalid jti!'); + } + + $this->log_service->debug_msg( + sprintf + ( + "InteractiveGrantType::processUserHint: jwt sub %s user_id %s jti %s", + $sub->getString(), + $user_id, + $jti->getValue() + ) + ); - $this->auth_service->reloadSession($jti->getValue()); + $this->auth_service->reloadSession($jti->getValue(), $user_id); $request->markParamAsProcessed(OAuth2Protocol::OAuth2Protocol_IDTokenHint); } diff --git a/app/libs/Utils/Services/IAuthService.php b/app/libs/Utils/Services/IAuthService.php index a8fc3dda..d151218f 100644 --- a/app/libs/Utils/Services/IAuthService.php +++ b/app/libs/Utils/Services/IAuthService.php @@ -140,7 +140,7 @@ public function getLoggedRPs():array; * @param string $jti * @return void */ - public function reloadSession(string $jti):void; + public function reloadSession(string $jti, string $user_id = null):void; const LOGGED_RELAYING_PARTIES_COOKIE_NAME = 'rps'; From e913375f3f6a6517bb5c67747f345fc93c758c1a Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 22 Sep 2026 10:28:37 -0300 Subject: [PATCH 2/9] fix(oauth2): reject unsigned id_token_hint and stop login loop on hint failure An id_token_hint built as an UnsecuredJWT (alg=none) is neither IJWE nor IJWS, so it skipped the whole signature-verification block and fell straight through to trusting its sub/jti unconditionally. Combined with the reload-session fallback that authenticates by user_id when the session can't be resumed, this let anyone forge an unsigned id_token_hint and get logged in as an arbitrary user_id with no credentials. Now any hint that isn't a verified IJWS is rejected before sub/jti are read. Separately, the id_token_hint param was only marked as processed after a successful reloadSession(). A hint that fails (stale, wrong audience, expired) never got marked, so every subsequent resume of the pending OAuth2 memento reprocessed the same hint, failed again, and logged the user back out - including right after a successful password login redirects back to /oauth2/auth. That turned any invalid hint into a login loop with no way out short of clearing site data. The param is now marked processed as soon as hint processing begins, so a failed hint is retried at most once per attempt. Full test suite green (244 tests, 1173 assertions, 0 failures). Signed-off-by: smarcet --- .../OAuth2/GrantTypes/InteractiveGrantType.php | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php b/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php index 993e93a2..2e6f4470 100644 --- a/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php +++ b/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php @@ -496,6 +496,14 @@ protected function processUserHint(OAuth2AuthenticationRequest $request, Client } else if(!empty($token_hint) && !$request->isProcessedParam(OAuth2Protocol::OAuth2Protocol_IDTokenHint)) { Log::debug("InteractiveGrant::processUserHint processing Token hint..."); + // Mark as processed up front: if verification/reload fails below, the exception + // is caught upstream and the pending memento gets re-serialized with this same + // request. Without this, every resume of that memento (e.g. right after a + // successful password login redirects back to /oauth2/auth) would retry this + // same stale/invalid hint, fail again, and log the user right back out — + // an infinite login loop driven by a hint that can never succeed. + $request->markParamAsProcessed(OAuth2Protocol::OAuth2Protocol_IDTokenHint); + $jwt = BasicJWTFactory::build($token_hint); if($jwt instanceof IJWE) { @@ -544,6 +552,13 @@ protected function processUserHint(OAuth2AuthenticationRequest $request, Client throw new InvalidLoginHint('invalid id_token_hint'); } + if(!$jwt instanceof IJWS) { + // neither IJWE->IJWS nor a plain IJWS: e.g. an unsecured JWT (alg=none). + // Never trust a sub/jti pair that was not cryptographically verified above. + $this->log_service->debug_msg("InteractiveGrantType::processUserHint token hint is not signed/verifiable"); + throw new InvalidLoginHint('id_token_hint must be signed'); + } + $sub = $jwt->getClaimSet()->getSubject(); $user_id = $this->auth_service->unwrapUserId($sub->getString()); $user = $this->auth_service->getUserById($user_id); From 2f326a7d3a32e0a10bb9c61d0591d864b7c173a4 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 22 Sep 2026 10:31:27 -0300 Subject: [PATCH 3/9] test(oauth2): cover unsigned id_token_hint rejection and login-loop fix Adds two unit tests for the fix in e913375: - testProcessUserHintRejectsUnsignedAlgNoneIdTokenHint: builds a real, parseable alg=none id_token_hint (RFC 7519 unsecured JWT) and asserts its forged sub/jti never reach unwrapUserId/getUserById/reloadSession. Fails against the pre-fix code with a TypeError from getUserById receiving the forged sub unchecked, proving the hint used to be trusted before any signature check. - testFailedIdTokenHintIsMarkedProcessedToPreventLoginLoop: asserts the id_token_hint param is marked processed on the request object even when hint processing fails. Fails against the pre-fix code (flag stays false), proving a failed hint used to be retried - and fail, and log the user out - on every resume of the pending OAuth2 memento. Both verified red against the parent commit and green against the fix. Signed-off-by: smarcet --- tests/unit/InteractiveGrantTypeTest.php | 113 ++++++++++++++++++++++++ 1 file changed, 113 insertions(+) diff --git a/tests/unit/InteractiveGrantTypeTest.php b/tests/unit/InteractiveGrantTypeTest.php index 5c10fe39..6a9619e8 100644 --- a/tests/unit/InteractiveGrantTypeTest.php +++ b/tests/unit/InteractiveGrantTypeTest.php @@ -502,6 +502,119 @@ public function testIdTokenHintProcessedParamPersistsThroughMemento(): void ); } + // ----------------------------------------------------------------------- + // Fix: reject an unsigned (alg=none) id_token_hint before trusting it, + // and mark a failed hint as processed so it can't loop the user out of + // a login they just completed. + // ----------------------------------------------------------------------- + + /** + * Builds a compact-serialization "unsecured JWT" (RFC 7519 §6): a real, + * parseable JWS-shaped token with alg=none and no signature segment. + * BasicJWTFactory::build() turns this into an UnsecuredJWT, which is + * neither IJWE nor IJWS - exactly the forgeable shape the fix rejects. + */ + private function buildUnsignedIdTokenHint(array $payload): string + { + $encode = function (array $data): string { + return rtrim(strtr(base64_encode(json_encode($data)), '+/', '-_'), '='); + }; + $header = ['alg' => 'none', 'typ' => 'JWT']; + return $encode($header) . '.' . $encode($payload) . '.'; + } + + /** + * An id_token_hint with alg=none carries no signature at all, so it must + * never be trusted: not for reloadSession's fallback authentication, not + * even to read who it claims to be. Anyone could forge one naming any + * user_id. The fix rejects it (not IJWS) before sub/jti are ever read. + */ + public function testProcessUserHintRejectsUnsignedAlgNoneIdTokenHint(): void + { + $forged_hint = $this->buildUnsignedIdTokenHint([ + 'sub' => '999', + 'jti' => 'forged-jti-attacker-controlled', + 'iss' => 'https://idp.test', + 'aud' => 'test-client-id', + ]); + + $request = $this->buildOIDCRequest([ + OAuth2Protocol::OAuth2Protocol_IDTokenHint => $forged_hint, + ]); + + $this->setupValidClient(); + $this->setupSecurityContext(); + $this->allowCleanupCalls(); + + $this->auth_service->shouldReceive('isUserLogged')->andReturn(false); + $this->auth_service->shouldReceive('getUserAuthenticationResponse') + ->andReturn(IAuthService::AuthenticationResponse_None); + + // The forged sub/jti must never reach account resolution or session + // reload - the hint has to be rejected before either is read. + $this->auth_service->shouldNotReceive('unwrapUserId'); + $this->auth_service->shouldNotReceive('getUserById'); + $this->auth_service->shouldNotReceive('reloadSession'); + + $this->auth_service->shouldReceive('logout')->with(false)->once(); + $this->memento_service->shouldReceive('serialize')->once(); + + $login_redirect = 'login-redirect-response'; + $this->auth_strategy->shouldReceive('doLogin') + ->once() + ->andReturn($login_redirect); + + $result = $this->grant_type->publicHandle($request); + + $this->assertEquals($login_redirect, $result); + } + + /** + * A hint that fails must be marked processed on this same pass. Before + * this fix, the "processed" flag was only set after a *successful* + * reloadSession(), so a stale/invalid hint kept getting re-attempted + * every time the pending OAuth2 memento was resumed - including right + * after a fresh, valid password login redirects back to /oauth2/auth - + * logging the user back out in an infinite loop. + */ + public function testFailedIdTokenHintIsMarkedProcessedToPreventLoginLoop(): void + { + $forged_hint = $this->buildUnsignedIdTokenHint([ + 'sub' => '999', + 'jti' => 'forged-jti-attacker-controlled', + ]); + + $request = $this->buildOIDCRequest([ + OAuth2Protocol::OAuth2Protocol_IDTokenHint => $forged_hint, + ]); + + $this->assertFalse( + $request->isProcessedParam(OAuth2Protocol::OAuth2Protocol_IDTokenHint) + ); + + $this->setupValidClient(); + $this->setupSecurityContext(); + $this->allowCleanupCalls(); + + $this->auth_service->shouldReceive('isUserLogged')->andReturn(false); + $this->auth_service->shouldReceive('getUserAuthenticationResponse') + ->andReturn(IAuthService::AuthenticationResponse_None); + + $this->auth_service->shouldReceive('logout')->with(false)->once(); + $this->memento_service->shouldReceive('serialize')->once(); + $this->auth_strategy->shouldReceive('doLogin')->once()->andReturn('login-response'); + + $this->grant_type->publicHandle($request); + + // The same $request instance handle() mutated: even though the hint + // failed, it must be marked processed so a memento resume of this + // exact request won't retry (and fail, and log out) again. + $this->assertTrue( + $request->isProcessedParam(OAuth2Protocol::OAuth2Protocol_IDTokenHint), + 'a failed id_token_hint must still be marked processed to avoid a login loop' + ); + } + // ----------------------------------------------------------------------- // Normal flow: consent accepted -> successful authorization // ----------------------------------------------------------------------- From ef1bcf04a24de9e9ca19bd524103368a4735924c Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 22 Sep 2026 11:05:34 -0300 Subject: [PATCH 4/9] fix(auth): honor canLogin() and fail closed in reloadSession fallback Addresses 3 CodeRabbit findings on PR #158: - Both fallback Auth::login() calls (empty-cache and post-catch) only checked is_null($user), not $user->canLogin(). getUserById() doesn't filter by account status, so a locked/deactivated/unverified user with a still-valid id_token_hint could bypass the lock via this path, unlike every other login entry point in this file which already gates on canLogin(). - The ReloadSessionException catch block returned normally (no throw) when $user_id was null, silently swallowing a real reload failure and violating the method's own \@throws contract. It now returns only after a successful fallback login, and rethrows otherwise. - Added a \Throwable catch alongside the existing ReloadSessionException one: a DB error inside getUserById() (not a ReloadSessionException) previously escaped uncaught, leaving the caller's session pointed at the swapped-in (possibly stale) session id instead of being restored - the same collateral-damage bug this PR set out to fix, via a different exception type. Full suite green (246 tests, 1176 assertions, 0 failures). Signed-off-by: smarcet --- app/libs/Auth/AuthService.php | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/app/libs/Auth/AuthService.php b/app/libs/Auth/AuthService.php index 3d113340..29d937a1 100644 --- a/app/libs/Auth/AuthService.php +++ b/app/libs/Auth/AuthService.php @@ -681,7 +681,7 @@ public function reloadSession(string $jti, string $user_id = null): void if(!is_null($user_id)) { Log::warning(sprintf("AuthService::reloadSession user id provided %s", $user_id)); $user = $this->getUserById($user_id); - if (is_null($user)) + if (is_null($user) || !$user->canLogin()) throw new ReloadSessionException('user not found!'); Auth::login($user); return; @@ -717,10 +717,19 @@ public function reloadSession(string $jti, string $user_id = null): void if(!is_null($user_id)) { Log::warning(sprintf("AuthService::reloadSession user id provided %s", $user_id)); $user = $this->getUserById($user_id); - if (is_null($user)) + if (is_null($user) || !$user->canLogin()) throw new ReloadSessionException('user not found!'); Auth::login($user); + return; } + throw $ex; + } + catch (\Throwable $ex) { + // Any non-ReloadSessionException failure (e.g. a DB error inside + // getUserById()) must still leave the caller's real session intact. + Session::setId($former_session_id); + Session::start(); + throw $ex; } } From 4a69b0b1f9bb402fe608223b0448e9a1e4ea2fb8 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 22 Sep 2026 12:15:00 -0300 Subject: [PATCH 5/9] fix(oauth2): validate id_token_hint expiration and register IDP principal Addresses the remaining findings on PR #158 (Copilot review): - processUserHint() now rejects an id_token_hint whose exp has passed, right after signature verification. A verified signature only proves who issued the token, not that it's still inside its validity window - the jti cache TTL mirrors the token's lifetime but isn't a reliable substitute for checking exp directly (eviction timing, clock skew). Deliberately NOT validating aud: this hint is the SSO mechanism between different clients of this IDP (client A -> client B), so the token's original audience is expected to differ from the client presenting it. - Both reloadSession() fallback Auth::login() calls now pair with principal_service->clear()+register(), matching every other login entry point in AuthService. Auth::login() alone left the IDP's own principal state (user_id/auth_time/op_browser_state) unset even though Auth::check() would report the user as logged in. Tests: - tests/AuthServiceReloadSessionTest.php (new): unit-tests reloadSession directly (facade-mocked, no DB) for canLogin() enforcement, principal registration on both fallback paths, the \Throwable session-restore path, and the no-fallback-user_id rethrow. - OIDCColdSessionReloadTest::testColdSessionRejectsExpiredIdTokenHint: real end-to-end test with an actually-expired, correctly-signed token (via a configurable near-zero id_token lifetime + sleep, no hand-forged signature). - Both new test groups verified red against the pre-fix code, green against the fix. Full suite green (253 tests, 1250 assertions, 0 failures). Signed-off-by: smarcet --- app/libs/Auth/AuthService.php | 10 + .../GrantTypes/InteractiveGrantType.php | 21 +- tests/AuthServiceReloadSessionTest.php | 266 ++++++++++++++++++ tests/OIDCColdSessionReloadTest.php | 70 +++++ tests/StubServerConfigurationService.php | 4 + 5 files changed, 369 insertions(+), 2 deletions(-) create mode 100644 tests/AuthServiceReloadSessionTest.php diff --git a/app/libs/Auth/AuthService.php b/app/libs/Auth/AuthService.php index 29d937a1..2aecf255 100644 --- a/app/libs/Auth/AuthService.php +++ b/app/libs/Auth/AuthService.php @@ -684,6 +684,11 @@ public function reloadSession(string $jti, string $user_id = null): void if (is_null($user) || !$user->canLogin()) throw new ReloadSessionException('user not found!'); Auth::login($user); + // Auth::login() alone leaves this session's IDP-specific principal + // state (user_id/auth_time/op_browser_state) unset - every other + // login path in this class pairs it with register(). + $this->principal_service->clear(); + $this->principal_service->register($user->getId(), time()); return; } throw new ReloadSessionException('session not found!'); @@ -720,6 +725,11 @@ public function reloadSession(string $jti, string $user_id = null): void if (is_null($user) || !$user->canLogin()) throw new ReloadSessionException('user not found!'); Auth::login($user); + // Auth::login() alone leaves this session's IDP-specific principal + // state (user_id/auth_time/op_browser_state) unset - every other + // login path in this class pairs it with register(). + $this->principal_service->clear(); + $this->principal_service->register($user->getId(), time()); return; } throw $ex; diff --git a/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php b/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php index 2e6f4470..5571a12d 100644 --- a/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php +++ b/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php @@ -59,6 +59,7 @@ use OAuth2\Strategies\IOAuth2AuthenticationStrategy; use utils\exceptions\InvalidCompactSerializationException; use utils\factories\BasicJWTFactory; +use utils\json_types\NumericDate; use Utils\Services\IAuthService; use Utils\Services\ILogService; use phpseclib\Crypt\Random; @@ -559,10 +560,26 @@ protected function processUserHint(OAuth2AuthenticationRequest $request, Client throw new InvalidLoginHint('id_token_hint must be signed'); } - $sub = $jwt->getClaimSet()->getSubject(); + $claim_set = $jwt->getClaimSet(); + + // A verified signature only proves who issued the token, not that it's + // still inside its validity window. The jti cache entry mirrors the + // token's own lifetime but isn't a substitute for checking exp directly + // (eviction timing, clock skew, etc. aren't guaranteed to line up). + // Intentionally NOT checking aud here: this hint is meant to carry SSO + // across different clients of this IDP (e.g. client A -> client B), so + // the token's original audience is expected to differ from the client + // making this request. + $expiration_time = $claim_set->getExpirationTime(); + if(is_null($expiration_time) || $expiration_time->isBefore(NumericDate::now())) { + $this->log_service->debug_msg("InteractiveGrantType::processUserHint token hint is expired"); + throw new InvalidLoginHint('id_token_hint is expired'); + } + + $sub = $claim_set->getSubject(); $user_id = $this->auth_service->unwrapUserId($sub->getString()); $user = $this->auth_service->getUserById($user_id); - $jti = $jwt->getClaimSet()->getJWTID(); + $jti = $claim_set->getJWTID(); if(is_null($jti)) { $this->log_service->debug_msg("InteractiveGrantType::processUserHint: jti is null"); diff --git a/tests/AuthServiceReloadSessionTest.php b/tests/AuthServiceReloadSessionTest.php new file mode 100644 index 00000000..1598b09b --- /dev/null +++ b/tests/AuthServiceReloadSessionTest.php @@ -0,0 +1,266 @@ +register(). + * - A non-ReloadSessionException failure inside the try block (e.g. a DB + * error) must still restore the caller's former session before + * propagating, and the ReloadSessionException catch block must rethrow + * rather than return silently when there is no $user_id to fall back to. + */ +#[\PHPUnit\Framework\Attributes\RunTestsInSeparateProcesses] +#[\PHPUnit\Framework\Attributes\PreserveGlobalState(false)] +final class AuthServiceReloadSessionTest extends PHPUnitTestCase +{ + use MockeryPHPUnitIntegration; + + private AuthService $service; + + private $mock_user_repository; + private $mock_principal_service; + private $mock_cache_service; + + private $auth_mock; + private $session_mock; + private $crypt_mock; + private $log_mock; + + protected function setUp(): void + { + parent::setUp(); + + $this->mock_user_repository = $this->createMock(IUserRepository::class); + $mock_otp_repository = $this->createMock(IOAuth2OTPRepository::class); + $this->mock_principal_service = $this->createMock(IPrincipalService::class); + $mock_user_service = $this->createMock(IUserService::class); + $mock_user_action_service = $this->createMock(IUserActionService::class); + $this->mock_cache_service = $this->createMock(ICacheService::class); + $mock_auth_user_service = $this->createMock(IAuthUserService::class); + $mock_security_context_service = $this->createMock(ISecurityContextService::class); + $mock_tx_service = $this->createMock(ITransactionService::class); + + $this->auth_mock = Mockery::mock('alias:Illuminate\Support\Facades\Auth'); + $this->session_mock = Mockery::mock('alias:Illuminate\Support\Facades\Session'); + $this->crypt_mock = Mockery::mock('alias:Illuminate\Support\Facades\Crypt'); + $this->log_mock = Mockery::mock('alias:Illuminate\Support\Facades\Log'); + + $this->log_mock->shouldReceive('debug')->zeroOrMoreTimes(); + $this->log_mock->shouldReceive('debug_msg')->zeroOrMoreTimes(); + $this->log_mock->shouldReceive('warning')->zeroOrMoreTimes(); + + $this->session_mock->shouldReceive('start')->zeroOrMoreTimes(); + + $this->service = new AuthService( + $this->mock_user_repository, + $mock_otp_repository, + $this->mock_principal_service, + $mock_user_service, + $mock_user_action_service, + $this->mock_cache_service, + $mock_auth_user_service, + $mock_security_context_service, + $mock_tx_service + ); + } + + private function mockUser(int $id, bool $can_login): Mockery\MockInterface + { + $user = Mockery::mock(User::class); + $user->shouldReceive('getId')->andReturn($id); + $user->shouldReceive('canLogin')->andReturn($can_login); + return $user; + } + + // ----------------------------------------------------------------------- + // Cache-miss fallback: reloadSession() falls straight to Auth::login($user) + // when the jti isn't cached at all. + // ----------------------------------------------------------------------- + + public function testCacheMissFallbackRejectsUserThatCannotLogin(): void + { + $this->mock_cache_service->method('getSingleValue')->with('jti-1')->willReturn(null); + + $user = $this->mockUser(42, can_login: false); + $this->mock_user_repository->method('getByIdWithGroups')->with(42)->willReturn($user); + + $this->session_mock->shouldReceive('getId')->once()->andReturn('former-session-id'); + $this->auth_mock->shouldNotReceive('login'); + $this->mock_principal_service->expects($this->never())->method('register'); + + $this->expectException(ReloadSessionException::class); + + $this->service->reloadSession('jti-1', '42'); + } + + public function testCacheMissFallbackRegistersPrincipalOnSuccess(): void + { + $this->mock_cache_service->method('getSingleValue')->with('jti-1')->willReturn(null); + + $user = $this->mockUser(42, can_login: true); + $this->mock_user_repository->method('getByIdWithGroups')->with(42)->willReturn($user); + + $this->session_mock->shouldReceive('getId')->once()->andReturn('former-session-id'); + $this->auth_mock->shouldReceive('login')->once()->with($user); + + $this->mock_principal_service->expects($this->once())->method('clear'); + $this->mock_principal_service->expects($this->once())->method('register')->with(42, $this->isType('int')); + + $this->service->reloadSession('jti-1', '42'); + } + + // ----------------------------------------------------------------------- + // catch(ReloadSessionException): the cached session resumes but has no + // live principal, so the fallback by $user_id kicks in. + // ----------------------------------------------------------------------- + + private function mockFailedSessionResume(): void + { + $this->mock_cache_service->method('getSingleValue')->with('jti-1')->willReturn('encrypted-session-id'); + $this->mock_cache_service->method('exists')->with('encrypted-session-idinvalid')->willReturn(false); + + $this->crypt_mock->shouldReceive('decrypt')->with('encrypted-session-id')->andReturn('decrypted-session-id'); + $this->session_mock->shouldReceive('setId')->with('decrypted-session-id')->zeroOrMoreTimes(); + $this->auth_mock->shouldReceive('check')->andReturn(false); + + $principal = new Principal(); + $principal->setState([0, time(), '']); + $this->mock_principal_service->method('get')->willReturn($principal); + } + + public function testCatchFallbackRejectsUserThatCannotLogin(): void + { + $this->mockFailedSessionResume(); + + $this->session_mock->shouldReceive('getId')->once()->andReturn('former-session-id'); + $this->session_mock->shouldReceive('setId')->with('former-session-id')->once(); + + $user = $this->mockUser(99, can_login: false); + // getByIdWithGroups is called twice: once with 0 (inside the try + // block, from the resumed-but-empty session's principal - throws + // and reaches the catch), then with 99 (the $user_id fallback). + $this->mock_user_repository->method('getByIdWithGroups')->willReturnMap([ + [0, null], + [99, $user], + ]); + + $this->auth_mock->shouldNotReceive('login'); + $this->mock_principal_service->expects($this->never())->method('register'); + + $this->expectException(ReloadSessionException::class); + + $this->service->reloadSession('jti-1', '99'); + } + + public function testCatchFallbackRegistersPrincipalOnSuccess(): void + { + $this->mockFailedSessionResume(); + + $this->session_mock->shouldReceive('getId')->once()->andReturn('former-session-id'); + $this->session_mock->shouldReceive('setId')->with('former-session-id')->once(); + + $user = $this->mockUser(99, can_login: true); + $this->mock_user_repository->method('getByIdWithGroups')->willReturnMap([ + [0, null], + [99, $user], + ]); + + $this->auth_mock->shouldReceive('login')->once()->with($user); + $this->mock_principal_service->expects($this->once())->method('clear'); + $this->mock_principal_service->expects($this->once())->method('register')->with(99, $this->isType('int')); + + $this->service->reloadSession('jti-1', '99'); + } + + /** + * No $user_id fallback was provided: the failed resume must propagate, + * not return as if reloadSession() had succeeded. + */ + public function testCatchRethrowsWhenNoFallbackUserIdProvided(): void + { + $this->mockFailedSessionResume(); + $this->mock_user_repository->method('getByIdWithGroups')->with(0)->willReturn(null); + + $this->session_mock->shouldReceive('getId')->once()->andReturn('former-session-id'); + $this->session_mock->shouldReceive('setId')->with('former-session-id')->once(); + + $this->auth_mock->shouldNotReceive('login'); + + $this->expectException(ReloadSessionException::class); + + $this->service->reloadSession('jti-1'); + } + + // ----------------------------------------------------------------------- + // A non-ReloadSessionException failure (e.g. a DB error) must still + // restore the caller's former session before propagating. + // ----------------------------------------------------------------------- + + public function testNonReloadSessionExceptionRestoresFormerSessionAndRethrows(): void + { + $this->mock_cache_service->method('getSingleValue')->with('jti-1')->willReturn('encrypted-session-id'); + $this->mock_cache_service->method('exists')->with('encrypted-session-idinvalid')->willReturn(false); + + $this->crypt_mock->shouldReceive('decrypt')->with('encrypted-session-id')->andReturn('decrypted-session-id'); + $this->session_mock->shouldReceive('setId')->with('decrypted-session-id')->once(); + $this->auth_mock->shouldReceive('check')->andReturn(false); + + $principal = new Principal(); + $principal->setState([7, time(), 'opbs']); + $this->mock_principal_service->method('get')->willReturn($principal); + + // A DB-layer failure, NOT a ReloadSessionException. + $this->mock_user_repository + ->method('getByIdWithGroups') + ->with(7) + ->willThrowException(new RuntimeException('DB is down')); + + $this->session_mock->shouldReceive('getId')->once()->andReturn('former-session-id'); + $this->session_mock->shouldReceive('setId')->with('former-session-id')->once(); + + $this->auth_mock->shouldNotReceive('login'); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage('DB is down'); + + $this->service->reloadSession('jti-1', '5'); + } +} diff --git a/tests/OIDCColdSessionReloadTest.php b/tests/OIDCColdSessionReloadTest.php index c39cebd7..b767b5b2 100644 --- a/tests/OIDCColdSessionReloadTest.php +++ b/tests/OIDCColdSessionReloadTest.php @@ -65,6 +65,12 @@ protected function prepareForTests(): void Session::start(); } + protected function tearDown(): void + { + unset($_ENV['id.token.lifetime']); + parent::tearDown(); + } + /** * Point the session facade at a brand-new anonymous session WITHOUT * destroying the previous session's server-side data - a real cold @@ -176,4 +182,68 @@ public function testColdSessionReloadFromBackChannelMintedIdTokenHint() $this->assertTrue(Auth::check(), 'reloadSession must leave the user authenticated'); $this->assertEquals($this->user->getId(), Auth::user()->getId()); } + + /** + * A real, correctly-signed id_token_hint whose exp has already passed + * must not authenticate anyone. A verified signature only proves who + * issued the token, not that it's still within its validity window - + * this covers the claim-validation gap raised on PR #158. + */ + public function testColdSessionRejectsExpiredIdTokenHint() + { + // A near-zero id_token lifetime lets a real, back-channel-minted + // token expire almost immediately, without hand-forging a signature. + $_ENV['id.token.lifetime'] = 1; + + $this->be($this->user); + Session::put("openid.authorization.response", IAuthService::AuthorizationResponse_AllowOnce); + + $response = $this->action("POST", "OAuth2\OAuth2ProviderController@auth", $this->authorizeParams()); + parse_str(parse_url($response->getTargetUrl(), PHP_URL_QUERY), $query); + $code = $query['code']; + + $this->startColdSession(); + + $response = $this->action("POST", "OAuth2\OAuth2ProviderController@token", + [ + 'code' => $code, + 'redirect_uri' => self::RedirectUri, + 'grant_type' => OAuth2Protocol::OAuth2Protocol_GrantType_AuthCode, + ], + [], [], [], + ["HTTP_Authorization" => " Basic " . base64_encode(self::ClientId . ':' . self::ClientSecret)]); + + $json = json_decode($response->getContent()); + $id_token_hint = $json->id_token; + $jwt = BasicJWTFactory::build($json->id_token); + if ($jwt instanceof IJWE) { + $recipient_key = RSAJWKFactory::build + ( + new RSAJWKPEMPrivateKeySpecification + ( + TestSeeder::$client_private_key_1, + RSAJWKPEMPrivateKeySpecification::WithoutPassword, + $jwt->getJOSEHeader()->getAlgorithm()->getString() + ) + ); + $recipient_key->setKeyUse(JSONWebKeyPublicKeyUseValues::Encryption)->setId('recipient_public_key'); + $jwt->setRecipientKey($recipient_key); + $id_token_hint = $jwt->getPlainText(); + } + + // Let the 1-second id_token_lifetime actually elapse. + sleep(2); + + $this->startColdSession(); + + $params = $this->authorizeParams(); + $params[OAuth2Protocol::OAuth2Protocol_IDTokenHint] = $id_token_hint; + + $response = $this->action("POST", "OAuth2\OAuth2ProviderController@auth", $params); + + $this->assertResponseStatus(302); + $this->assertTrue(str_contains($response->getTargetUrl(), '/auth/login'), + sprintf('an expired id_token_hint must require login, got %s', $response->getTargetUrl())); + $this->assertFalse(Auth::check(), 'an expired id_token_hint must not authenticate anyone'); + } } diff --git a/tests/StubServerConfigurationService.php b/tests/StubServerConfigurationService.php index 4c8368fd..d5a808c7 100644 --- a/tests/StubServerConfigurationService.php +++ b/tests/StubServerConfigurationService.php @@ -24,6 +24,10 @@ public function getConfigValue($value) return intval($_ENV['access.token.lifetime']); } + if ($value === 'OAuth2.IdToken.Lifetime' && isset($_ENV['id.token.lifetime'])) { + return intval($_ENV['id.token.lifetime']); + } + return parent::getConfigValue($value); } } \ No newline at end of file From dd611cfa21f6a35d2e8d25e1cf1bff5425495e4b Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 22 Sep 2026 12:47:40 -0300 Subject: [PATCH 6/9] fix(oauth2): only unlock id_token_hint sub fallback for IDP-signed hints reloadSession()'s sub-based fallback (login by user_id when the hint's jti is no longer cached) was offered for any id_token_hint whose signature verified, including one verified with a key the client controls: an HS* client secret, a public key the client registered, or its jwks_uri. Such a signature proves the client made the token, not the IDP, so a client operator could mint a hint naming an arbitrary user id and obtain a session for it once the jti lookup missed. processUserHint now tracks whether the signature was verified with the server's own signing key and passes user_id to reloadSession() only in that case. Client-key-verified hints keep the jti-only semantics. Tests: - unit: a client-secret-signed hint reaches reloadSession() with a null user_id; a server-key-signed hint still carries the resolved user_id - e2e: a hint signed with the seeded client's HS512 secret and an unknown jti lands on /auth/login with no authenticated user (was: logged in and sent to consent) --- .../GrantTypes/InteractiveGrantType.php | 16 +- tests/OIDCColdSessionReloadTest.php | 51 +++++ tests/unit/InteractiveGrantTypeTest.php | 186 ++++++++++++++++++ 3 files changed, 252 insertions(+), 1 deletion(-) diff --git a/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php b/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php index 5571a12d..59dc95f4 100644 --- a/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php +++ b/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php @@ -522,6 +522,13 @@ protected function processUserHint(OAuth2AuthenticationRequest $request, Client $payload = $jwt->getPlainText(); $jwt = BasicJWTFactory::build($payload); } + // Only a signature made with this IDP's own private key proves the IDP + // issued the hint. A key the client controls (an HS* client secret, a + // public key the client registered, its jwks_uri) proves the *client* + // made it, so it must never unlock the sub-based fallback in + // reloadSession() - it keeps the jti-only semantics. + $issued_by_this_idp = false; + if($jwt instanceof IJWS) { $this->log_service->debug_msg("InteractiveGrantType::processUserHint token hint is IJWS"); // signed by client ? @@ -545,6 +552,7 @@ protected function processUserHint(OAuth2AuthenticationRequest $request, Client $jwt->getJOSEHeader()->getKeyID()->getValue() ); $jwt->setKey($server_private_sig_key); + $issued_by_this_idp = true; } $verified = $jwt->verify($jwt->getJOSEHeader()->getAlgorithm()->getString()); @@ -596,7 +604,13 @@ protected function processUserHint(OAuth2AuthenticationRequest $request, Client ) ); - $this->auth_service->reloadSession($jti->getValue(), $user_id); + // The sub-based fallback (login by $user_id when the jti is no longer + // cached) is only safe for a hint this IDP is proven to have issued. + $this->auth_service->reloadSession + ( + $jti->getValue(), + $issued_by_this_idp ? $user_id : null + ); $request->markParamAsProcessed(OAuth2Protocol::OAuth2Protocol_IDTokenHint); } diff --git a/tests/OIDCColdSessionReloadTest.php b/tests/OIDCColdSessionReloadTest.php index b767b5b2..cb43cc45 100644 --- a/tests/OIDCColdSessionReloadTest.php +++ b/tests/OIDCColdSessionReloadTest.php @@ -17,14 +17,23 @@ use Illuminate\Support\Facades\Auth; use Illuminate\Support\Facades\Session; use Illuminate\Support\Str; +use jwa\JSONWebSignatureAndEncryptionAlgorithms; use jwe\IJWE; +use jwk\impl\OctetSequenceJWKFactory; +use jwk\impl\OctetSequenceJWKSpecification; use jwk\impl\RSAJWKFactory; use jwk\impl\RSAJWKPEMPrivateKeySpecification; use jwk\JSONWebKeyPublicKeyUseValues; use jws\IJWS; +use jws\impl\specs\JWS_ParamsSpecification; +use jws\JWSFactory; +use jwt\impl\JWTClaimSet; use LaravelDoctrine\ORM\Facades\EntityManager; use OAuth2\OAuth2Protocol; use utils\factories\BasicJWTFactory; +use utils\json_types\JsonValue; +use utils\json_types\NumericDate; +use utils\json_types\StringOrURI; use Utils\Services\IAuthService; use Utils\Services\UtilsServiceCatalog; @@ -246,4 +255,46 @@ public function testColdSessionRejectsExpiredIdTokenHint() sprintf('an expired id_token_hint must require login, got %s', $response->getTargetUrl())); $this->assertFalse(Auth::check(), 'an expired id_token_hint must not authenticate anyone'); } + + /** + * The seeded test client signs its id_tokens with HS512 - i.e. with the + * client secret, a key the client itself holds. A token minted with that + * secret verifies exactly like an IDP-issued one, so it proves nothing + * about who issued it. When its jti is not in the cache (never minted by + * the IDP, or long evicted), reloadSession()'s sub-based fallback must NOT + * turn it into a login for whatever user id the token names. + */ + public function testColdSessionRejectsClientSignedIdTokenHintWhenJtiIsNotCached() + { + $alg = JSONWebSignatureAndEncryptionAlgorithms::HS512; + + $client_jwk = OctetSequenceJWKFactory::build(new OctetSequenceJWKSpecification(self::ClientSecret, $alg)); + $client_jwk->setKeyUse(JSONWebKeyPublicKeyUseValues::Signature); + + $now = time(); + $claim_set = new JWTClaimSet( + new StringOrURI('https://idp.test'), + new StringOrURI((string)$this->user->getId()), + new StringOrURI(self::ClientId), + new NumericDate($now), + new NumericDate($now + 600), + new JsonValue('never-cached-' . Str::random(16)) + ); + + $forged_hint = JWSFactory::build( + new JWS_ParamsSpecification($client_jwk, new StringOrURI($alg), $claim_set) + )->toCompactSerialization(); + + $this->startColdSession(); + + $params = $this->authorizeParams(); + $params[OAuth2Protocol::OAuth2Protocol_IDTokenHint] = $forged_hint; + + $response = $this->action("POST", "OAuth2\OAuth2ProviderController@auth", $params); + + $this->assertResponseStatus(302); + $this->assertTrue(str_contains($response->getTargetUrl(), '/auth/login'), + sprintf('a client-signed id_token_hint with an unknown jti must require login, got %s', $response->getTargetUrl())); + $this->assertFalse(Auth::check(), 'a client-signed id_token_hint must never authenticate anyone by sub'); + } } diff --git a/tests/unit/InteractiveGrantTypeTest.php b/tests/unit/InteractiveGrantTypeTest.php index 6a9619e8..04527627 100644 --- a/tests/unit/InteractiveGrantTypeTest.php +++ b/tests/unit/InteractiveGrantTypeTest.php @@ -13,13 +13,27 @@ * limitations under the License. **/ +use App\libs\OAuth2\Exceptions\ReloadSessionException; use Auth\User; use Exception; use Illuminate\Container\Container; use Illuminate\Support\Facades\Facade; +use jwa\cryptographic_algorithms\DigitalSignatures_MACs_Registry; +use jwa\JSONWebSignatureAndEncryptionAlgorithms; +use jwk\IJWK; +use jwk\impl\OctetSequenceJWKFactory; +use jwk\impl\OctetSequenceJWKSpecification; +use jwk\impl\RSAJWKFactory; +use jwk\impl\RSAJWKPEMPrivateKeySpecification; +use jwk\JSONWebKeyPublicKeyUseValues; +use jws\impl\specs\JWS_ParamsSpecification; +use jws\JWSFactory; +use jwt\impl\JWTClaimSet; use Mockery; use Models\OAuth2\Client; +use Models\OAuth2\ServerPrivateKey; use OAuth2\Models\IClient; +use OAuth2\Models\JWTResponseInfo; use OAuth2\Models\Principal; use OAuth2\Models\SecurityContext; use OAuth2\OAuth2Message; @@ -41,6 +55,10 @@ use OAuth2\Strategies\IOAuth2AuthenticationStrategy; use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; +use Utils\Db\ITransactionService; +use utils\json_types\JsonValue; +use utils\json_types\NumericDate; +use utils\json_types\StringOrURI; use Utils\Services\IAuthService; use Utils\Services\ILogService; @@ -615,6 +633,174 @@ public function testFailedIdTokenHintIsMarkedProcessedToPreventLoginLoop(): void ); } + // ----------------------------------------------------------------------- + // Fix: the sub-based fallback in reloadSession() is only unlocked by a hint + // whose signature was verified with the IDP's own server key. A signature + // that verifies with a client-controlled key (HS* client secret, a public + // key the client registered, its jwks_uri) proves the *client* made it, + // not the IDP, so it must keep the old jti-only semantics. + // ----------------------------------------------------------------------- + + private function buildHintClaimSet(string $sub, string $jti): JWTClaimSet + { + $now = time(); + return new JWTClaimSet( + new StringOrURI('https://idp.test'), + new StringOrURI($sub), + new StringOrURI('test-client-id'), + new NumericDate($now), + new NumericDate($now + 600), + new JsonValue($jti) + ); + } + + /** + * Signs a real JWS with the library so the header round-trips exactly the + * way JWS::verify() re-serializes it. + */ + private function signHint(IJWK $jwk, string $alg, JWTClaimSet $claim_set): string + { + return JWSFactory::build( + new JWS_ParamsSpecification($jwk, new StringOrURI($alg), $claim_set) + )->toCompactSerialization(); + } + + /** + * Common expectations for a hint that verifies but whose reloadSession() + * fails: the request must end at the login page, never authenticated. + */ + private function expectHintReloadFailureEndsAtLogin(): string + { + $this->auth_service->shouldReceive('isUserLogged')->andReturn(false); + $this->auth_service->shouldReceive('getUserAuthenticationResponse') + ->andReturn(IAuthService::AuthenticationResponse_None); + $this->auth_service->shouldReceive('unwrapUserId')->with('999')->andReturn('999'); + $this->auth_service->shouldReceive('getUserById')->with('999')->andReturn(null); + + $this->auth_service->shouldReceive('logout')->with(false)->once(); + $this->memento_service->shouldReceive('serialize')->once(); + + $login_redirect = 'login-redirect-response'; + $this->auth_strategy->shouldReceive('doLogin')->once()->andReturn($login_redirect); + return $login_redirect; + } + + /** + * The client signs its id_tokens with HS512, i.e. with its own client + * secret - the IDP and the client share that key, so a token minted by + * the client verifies exactly like an IDP-issued one. Such a hint must + * reach reloadSession() WITHOUT the sub-based fallback ($user_id null). + */ + public function testClientKeyVerifiedIdTokenHintDoesNotUnlockSubFallback(): void + { + $secret = 'ITc/6Y5N7kOtGKhgITc/6Y5N7kOtGKhgITc/6Y5N7kOtGKhgITc/6Y5N7kOtGKhg'; + $alg = JSONWebSignatureAndEncryptionAlgorithms::HS512; + + $client_jwk = OctetSequenceJWKFactory::build(new OctetSequenceJWKSpecification($secret, $alg)); + $client_jwk->setKeyUse(JSONWebKeyPublicKeyUseValues::Signature); + + $hint = $this->signHint($client_jwk, $alg, $this->buildHintClaimSet('999', 'jti-client-signed')); + + $request = $this->buildOIDCRequest([ + OAuth2Protocol::OAuth2Protocol_IDTokenHint => $hint, + ]); + + $client = $this->setupValidClient(); + $client->shouldReceive('getIdTokenResponseInfo') + ->andReturn(new JWTResponseInfo(DigitalSignatures_MACs_Registry::getInstance()->get($alg))); + $client->shouldReceive('getClientType')->andReturn(IClient::ClientType_Confidential); + $client->shouldReceive('getClientSecret')->andReturn($secret); + + $this->setupSecurityContext(); + $this->allowCleanupCalls(); + $login_redirect = $this->expectHintReloadFailureEndsAtLogin(); + + // Key assertion: the signature was verified with the CLIENT's key, so + // the sub-based fallback must not be offered to reloadSession(). + $this->auth_service->shouldReceive('reloadSession') + ->once() + ->withArgs(function ($jti, $user_id = null) { + return $jti === 'jti-client-signed' && $user_id === null; + }) + ->andThrow(new ReloadSessionException('session not found!')); + + $result = $this->grant_type->publicHandle($request); + + $this->assertEquals($login_redirect, $result); + } + + /** + * Positive control: a hint verified with the IDP's own RS256 server key + * (the client has no registered signing key and no jwks_uri, so the + * client-key lookup throws RecipientKeyNotFoundException) keeps the + * sub-based fallback - reloadSession() receives the resolved user_id. + */ + public function testServerKeyVerifiedIdTokenHintKeepsSubFallback(): void + { + $alg = JSONWebSignatureAndEncryptionAlgorithms::RS256; + + $key_pair = openssl_pkey_new(['private_key_bits' => 2048, 'private_key_type' => OPENSSL_KEYTYPE_RSA]); + openssl_pkey_export($key_pair, $pem); + + $server_jwk = RSAJWKFactory::build( + new RSAJWKPEMPrivateKeySpecification($pem, RSAJWKPEMPrivateKeySpecification::WithoutPassword, $alg) + ); + $server_jwk->setKeyUse(JSONWebKeyPublicKeyUseValues::Signature)->setId('server-sig-key'); + + $hint = $this->signHint($server_jwk, $alg, $this->buildHintClaimSet('999', 'jti-server-signed')); + + $request = $this->buildOIDCRequest([ + OAuth2Protocol::OAuth2Protocol_IDTokenHint => $hint, + ]); + + $client = $this->setupValidClient(); + $client->shouldReceive('getIdTokenResponseInfo') + ->andReturn(new JWTResponseInfo(DigitalSignatures_MACs_Registry::getInstance()->get($alg))); + // No client-controlled signing key anywhere -> RecipientKeyNotFoundException + // -> InteractiveGrantType falls back to the server signing key. + $client->shouldReceive('getCurrentPublicKeyByUse') + ->with(JSONWebKeyPublicKeyUseValues::Signature, $alg) + ->andReturn(null); + $this->jwk_set_reader_service->shouldReceive('read')->with($client)->andReturn(null); + + // ServerSigningKeyFinder resolves ITransactionService through the App facade. + $app = Facade::getFacadeApplication(); + $app->instance('app', $app); + $tx_service = Mockery::mock(ITransactionService::class); + $tx_service->shouldReceive('transaction')->andReturnUsing(function (callable $callback) { + return $callback(); + }); + $app->instance(ITransactionService::class, $tx_service); + + $server_key_alg = Mockery::mock(); + $server_key_alg->shouldReceive('getName')->andReturn($alg); + $server_key = Mockery::mock(ServerPrivateKey::class); + $server_key->shouldReceive('isActive')->andReturn(true); + $server_key->shouldReceive('getAlg')->andReturn($server_key_alg); + $server_key->shouldReceive('toJWK')->andReturn($server_jwk); + $server_key->shouldReceive('markAsUsed')->andReturnNull(); + $this->server_private_key_repository->shouldReceive('getByKeyIdentifier') + ->with('server-sig-key') + ->andReturn($server_key); + + $this->setupSecurityContext(); + $this->allowCleanupCalls(); + $login_redirect = $this->expectHintReloadFailureEndsAtLogin(); + + // Key assertion: the signature was verified with the SERVER key, so + // the sub-based fallback is offered to reloadSession(). + $this->auth_service->shouldReceive('reloadSession') + ->once() + ->withArgs(function ($jti, $user_id = null) { + return $jti === 'jti-server-signed' && $user_id === '999'; + }) + ->andThrow(new ReloadSessionException('user not found!')); + + $result = $this->grant_type->publicHandle($request); + + $this->assertEquals($login_redirect, $result); + } + // ----------------------------------------------------------------------- // Normal flow: consent accepted -> successful authorization // ----------------------------------------------------------------------- From fcc3c7334f4696abb4e4e47e2bce06ff638afb90 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 22 Sep 2026 13:03:55 -0300 Subject: [PATCH 7/9] fix(oauth2): reject id_token_hint with exp == now or without sub MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two claim-validation gaps on a verified id_token_hint: - exp: NumericDate::isBefore() is a strict `<`, so a hint whose exp equals the current second passed the expiry check. RFC 7519 §4.1.4 requires the current time to be strictly before exp; the guard is now `!$expiration_time->isAfter(NumericDate::now())`. - sub: a hint without a sub claim dereferenced null (`$sub->getString()`), raising an Error that the Exception-only catch in mustAuthenticateUser() does not handle, so the request 500'd instead of falling through to the login redirect. Added the same null guard jti and exp already have. Tests (unit, InteractiveGrantTypeTest): a hint with exp = now and a hint without sub are both rejected before reloadSession() and end at the login page. --- .../GrantTypes/InteractiveGrantType.php | 11 +- tests/unit/InteractiveGrantTypeTest.php | 122 ++++++++++++++++++ 2 files changed, 131 insertions(+), 2 deletions(-) diff --git a/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php b/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php index 59dc95f4..eee1dafb 100644 --- a/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php +++ b/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php @@ -578,13 +578,20 @@ protected function processUserHint(OAuth2AuthenticationRequest $request, Client // across different clients of this IDP (e.g. client A -> client B), so // the token's original audience is expected to differ from the client // making this request. + // RFC 7519 §4.1.4: the current time MUST be strictly before exp, so + // exp == now is already expired. $expiration_time = $claim_set->getExpirationTime(); - if(is_null($expiration_time) || $expiration_time->isBefore(NumericDate::now())) { + if(is_null($expiration_time) || !$expiration_time->isAfter(NumericDate::now())) { $this->log_service->debug_msg("InteractiveGrantType::processUserHint token hint is expired"); throw new InvalidLoginHint('id_token_hint is expired'); } - $sub = $claim_set->getSubject(); + $sub = $claim_set->getSubject(); + if(is_null($sub)) { + $this->log_service->debug_msg("InteractiveGrantType::processUserHint: sub is null"); + throw new InvalidLoginHint('invalid sub!'); + } + $user_id = $this->auth_service->unwrapUserId($sub->getString()); $user = $this->auth_service->getUserById($user_id); $jti = $claim_set->getJWTID(); diff --git a/tests/unit/InteractiveGrantTypeTest.php b/tests/unit/InteractiveGrantTypeTest.php index 04527627..3f2847a2 100644 --- a/tests/unit/InteractiveGrantTypeTest.php +++ b/tests/unit/InteractiveGrantTypeTest.php @@ -801,6 +801,128 @@ public function testServerKeyVerifiedIdTokenHintKeepsSubFallback(): void $this->assertEquals($login_redirect, $result); } + // ----------------------------------------------------------------------- + // Claim validation on a verified hint (CodeRabbit threads on PR #158): + // exp must be strictly in the future, and a missing sub must be rejected + // as an InvalidLoginHint instead of dereferencing null. + // ----------------------------------------------------------------------- + + /** + * A confidential client whose id_tokens are HS512-signed with its own + * secret, plus the matching JWK to sign test hints with. + * + * @return array{0: IJWK, 1: string} + */ + private function setupClientSecretSignedClient(): array + { + $secret = 'ITc/6Y5N7kOtGKhgITc/6Y5N7kOtGKhgITc/6Y5N7kOtGKhgITc/6Y5N7kOtGKhg'; + $alg = JSONWebSignatureAndEncryptionAlgorithms::HS512; + + $client = $this->setupValidClient(); + $client->shouldReceive('getIdTokenResponseInfo') + ->andReturn(new JWTResponseInfo(DigitalSignatures_MACs_Registry::getInstance()->get($alg))); + $client->shouldReceive('getClientType')->andReturn(IClient::ClientType_Confidential); + $client->shouldReceive('getClientSecret')->andReturn($secret); + + $jwk = OctetSequenceJWKFactory::build(new OctetSequenceJWKSpecification($secret, $alg)); + $jwk->setKeyUse(JSONWebKeyPublicKeyUseValues::Signature); + + return [$jwk, $alg]; + } + + /** + * Common expectations for a verified hint that must be rejected during + * claim validation: reloadSession() is never reached and the request + * ends at the login page. + */ + private function expectHintRejectedBeforeReload(): string + { + $this->auth_service->shouldReceive('isUserLogged')->andReturn(false); + $this->auth_service->shouldReceive('getUserAuthenticationResponse') + ->andReturn(IAuthService::AuthenticationResponse_None); + + $this->auth_service->shouldNotReceive('unwrapUserId'); + $this->auth_service->shouldNotReceive('getUserById'); + $this->auth_service->shouldNotReceive('reloadSession'); + + $this->auth_service->shouldReceive('logout')->with(false)->once(); + $this->memento_service->shouldReceive('serialize')->once(); + + $login_redirect = 'login-redirect-response'; + $this->auth_strategy->shouldReceive('doLogin')->once()->andReturn($login_redirect); + return $login_redirect; + } + + /** + * RFC 7519 §4.1.4: the current time MUST be strictly before exp. A hint + * whose exp equals the current second is already expired. + * + * NumericDate::now() is wall-clock time, so this test builds the hint + * with exp = time() right before handling it; the whole handle() call + * runs well within one second. + */ + public function testProcessUserHintRejectsIdTokenHintWhoseExpEqualsNow(): void + { + [$jwk, $alg] = $this->setupClientSecretSignedClient(); + + $now = time(); + $claim_set = new JWTClaimSet( + new StringOrURI('https://idp.test'), + new StringOrURI('999'), + new StringOrURI('test-client-id'), + new NumericDate($now - 60), + new NumericDate($now), + new JsonValue('jti-exp-boundary') + ); + $hint = $this->signHint($jwk, $alg, $claim_set); + + $request = $this->buildOIDCRequest([ + OAuth2Protocol::OAuth2Protocol_IDTokenHint => $hint, + ]); + + $this->setupSecurityContext(); + $this->allowCleanupCalls(); + $login_redirect = $this->expectHintRejectedBeforeReload(); + + $result = $this->grant_type->publicHandle($request); + + $this->assertEquals($login_redirect, $result); + } + + /** + * A verified hint without a sub claim must be rejected as an + * InvalidLoginHint (ending at the login page like any other bad hint), + * not blow up dereferencing null - that raises an Error, which the + * Exception-only catch in mustAuthenticateUser() does not handle. + */ + public function testProcessUserHintRejectsIdTokenHintWithoutSub(): void + { + [$jwk, $alg] = $this->setupClientSecretSignedClient(); + + $now = time(); + $claim_set = new JWTClaimSet( + new StringOrURI('https://idp.test'), + null, + new StringOrURI('test-client-id'), + new NumericDate($now), + new NumericDate($now + 600), + new JsonValue('jti-no-sub') + ); + $hint = $this->signHint($jwk, $alg, $claim_set); + + $request = $this->buildOIDCRequest([ + OAuth2Protocol::OAuth2Protocol_IDTokenHint => $hint, + ]); + + $this->setupSecurityContext(); + $this->allowCleanupCalls(); + $login_redirect = $this->expectHintRejectedBeforeReload(); + + $result = $this->grant_type->publicHandle($request); + + $this->assertEquals($login_redirect, $result); + } + // ----------------------------------------------------------------------- // Normal flow: consent accepted -> successful authorization // ----------------------------------------------------------------------- From 63d80501dcc34ead65d06c1e8935ebfedbf85af5 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 22 Sep 2026 13:44:42 -0300 Subject: [PATCH 8/9] fix(oauth2): register hint auth_time on id_token_hint sub fallback The sub-based fallback in AuthService::reloadSession() registered the IDP principal with time(), although the user did not authenticate on that request: they authenticated when the IDP issued the hint. That made shouldForceReLogin() treat a stale authentication as fresh and stamped a false auth_time into the next id_token. reloadSession() now takes an optional auth_time and registers it in both fallback branches (time() stays only as a safety net). InteractiveGrantType::processUserHint() resolves it from the hint's auth_time claim, else its iat, and degrades a server-signed hint that carries neither to jti-only semantics instead of inventing a value. Tests: the two AuthServiceReloadSessionTest success cases assert the exact registered auth_time; the server-key InteractiveGrantTypeTest asserts iat is forwarded, and a new case asserts an explicit auth_time claim wins over iat. --- app/libs/Auth/AuthService.php | 11 +- .../GrantTypes/InteractiveGrantType.php | 21 ++- app/libs/Utils/Services/IAuthService.php | 4 +- tests/AuthServiceReloadSessionTest.php | 11 +- tests/unit/InteractiveGrantTypeTest.php | 122 ++++++++++++++---- 5 files changed, 131 insertions(+), 38 deletions(-) diff --git a/app/libs/Auth/AuthService.php b/app/libs/Auth/AuthService.php index 2aecf255..480191b2 100644 --- a/app/libs/Auth/AuthService.php +++ b/app/libs/Auth/AuthService.php @@ -666,10 +666,11 @@ public function getLoggedRPs(): array /** * @param string $jti * @param string|null $user_id + * @param int|null $auth_time * @return void * @throws ReloadSessionException */ - public function reloadSession(string $jti, string $user_id = null): void + public function reloadSession(string $jti, ?string $user_id = null, ?int $auth_time = null): void { $former_session_id = Session::getId(); Log::debug(sprintf("AuthService::reloadSession jti %s", $jti)); @@ -687,8 +688,10 @@ public function reloadSession(string $jti, string $user_id = null): void // Auth::login() alone leaves this session's IDP-specific principal // state (user_id/auth_time/op_browser_state) unset - every other // login path in this class pairs it with register(). + // The user did not authenticate now: they authenticated when the IDP + // issued the hint, so register that time (time() is only a safety net). $this->principal_service->clear(); - $this->principal_service->register($user->getId(), time()); + $this->principal_service->register($user->getId(), $auth_time ?? time()); return; } throw new ReloadSessionException('session not found!'); @@ -728,8 +731,10 @@ public function reloadSession(string $jti, string $user_id = null): void // Auth::login() alone leaves this session's IDP-specific principal // state (user_id/auth_time/op_browser_state) unset - every other // login path in this class pairs it with register(). + // The user did not authenticate now: they authenticated when the IDP + // issued the hint, so register that time (time() is only a safety net). $this->principal_service->clear(); - $this->principal_service->register($user->getId(), time()); + $this->principal_service->register($user->getId(), $auth_time ?? time()); return; } throw $ex; diff --git a/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php b/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php index eee1dafb..101ab180 100644 --- a/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php +++ b/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php @@ -611,12 +611,29 @@ protected function processUserHint(OAuth2AuthenticationRequest $request, Client ) ); + // The fallback login must carry the authentication time the IDP originally + // attested for this user (auth_time when the RP asked for max_age, else iat), + // not "now": shouldForceReLogin() and the next id_token's auth_time claim + // both read it from the registered principal. + // Note: getClaimByName() returns the stored JsonValue at runtime (see + // JWTClaimSet::addClaim / JWTClaimSetFactory), not a JWTClaim. + $hint_auth_time = null; + $auth_time_claim = $claim_set->getClaimByName(OAuth2Protocol::OAuth2Protocol_AuthTime); + $issued_at = $claim_set->getIssuedAt(); + if(!is_null($auth_time_claim)) + $hint_auth_time = intval($auth_time_claim->getValue()); + else if(!is_null($issued_at)) + $hint_auth_time = intval($issued_at->getValue()); + // The sub-based fallback (login by $user_id when the jti is no longer - // cached) is only safe for a hint this IDP is proven to have issued. + // cached) is only safe for a hint this IDP is proven to have issued AND + // that carries an attested authentication time; otherwise degrade to the + // jti-only semantics rather than inventing an auth_time. $this->auth_service->reloadSession ( $jti->getValue(), - $issued_by_this_idp ? $user_id : null + ($issued_by_this_idp && !is_null($hint_auth_time)) ? $user_id : null, + $hint_auth_time ); $request->markParamAsProcessed(OAuth2Protocol::OAuth2Protocol_IDTokenHint); diff --git a/app/libs/Utils/Services/IAuthService.php b/app/libs/Utils/Services/IAuthService.php index d151218f..cebe3601 100644 --- a/app/libs/Utils/Services/IAuthService.php +++ b/app/libs/Utils/Services/IAuthService.php @@ -138,9 +138,11 @@ public function getLoggedRPs():array; /** * @param string $jti + * @param string|null $user_id user to log in when the cached session can't be resumed (IDP-signed hints only) + * @param int|null $auth_time epoch the IDP originally attested for that user (hint's auth_time, else iat) * @return void */ - public function reloadSession(string $jti, string $user_id = null):void; + public function reloadSession(string $jti, ?string $user_id = null, ?int $auth_time = null):void; const LOGGED_RELAYING_PARTIES_COOKIE_NAME = 'rps'; diff --git a/tests/AuthServiceReloadSessionTest.php b/tests/AuthServiceReloadSessionTest.php index 1598b09b..5622f4b5 100644 --- a/tests/AuthServiceReloadSessionTest.php +++ b/tests/AuthServiceReloadSessionTest.php @@ -141,10 +141,12 @@ public function testCacheMissFallbackRegistersPrincipalOnSuccess(): void $this->session_mock->shouldReceive('getId')->once()->andReturn('former-session-id'); $this->auth_mock->shouldReceive('login')->once()->with($user); + // The principal must be registered with the auth_time the hint attested, + // not with "now" - the user did not authenticate on this request. $this->mock_principal_service->expects($this->once())->method('clear'); - $this->mock_principal_service->expects($this->once())->method('register')->with(42, $this->isType('int')); + $this->mock_principal_service->expects($this->once())->method('register')->with(42, 1700000000); - $this->service->reloadSession('jti-1', '42'); + $this->service->reloadSession('jti-1', '42', 1700000000); } // ----------------------------------------------------------------------- @@ -204,10 +206,11 @@ public function testCatchFallbackRegistersPrincipalOnSuccess(): void ]); $this->auth_mock->shouldReceive('login')->once()->with($user); + // Same contract as the cache-miss branch: register the attested auth_time. $this->mock_principal_service->expects($this->once())->method('clear'); - $this->mock_principal_service->expects($this->once())->method('register')->with(99, $this->isType('int')); + $this->mock_principal_service->expects($this->once())->method('register')->with(99, 1700000000); - $this->service->reloadSession('jti-1', '99'); + $this->service->reloadSession('jti-1', '99', 1700000000); } /** diff --git a/tests/unit/InteractiveGrantTypeTest.php b/tests/unit/InteractiveGrantTypeTest.php index 3f2847a2..9d821cf6 100644 --- a/tests/unit/InteractiveGrantTypeTest.php +++ b/tests/unit/InteractiveGrantTypeTest.php @@ -29,6 +29,7 @@ use jws\impl\specs\JWS_ParamsSpecification; use jws\JWSFactory; use jwt\impl\JWTClaimSet; +use jwt\JWTClaim; use Mockery; use Models\OAuth2\Client; use Models\OAuth2\ServerPrivateKey; @@ -641,17 +642,25 @@ public function testFailedIdTokenHintIsMarkedProcessedToPreventLoginLoop(): void // not the IDP, so it must keep the old jti-only semantics. // ----------------------------------------------------------------------- - private function buildHintClaimSet(string $sub, string $jti): JWTClaimSet + /** + * @param int|null $iat issued-at epoch (defaults to now) + * @param int|null $auth_time optional auth_time claim, as the IDP adds it when max_age was requested + */ + private function buildHintClaimSet(string $sub, string $jti, ?int $iat = null, ?int $auth_time = null): JWTClaimSet { - $now = time(); - return new JWTClaimSet( + $iat = $iat ?? time(); + $claim_set = new JWTClaimSet( new StringOrURI('https://idp.test'), new StringOrURI($sub), new StringOrURI('test-client-id'), - new NumericDate($now), - new NumericDate($now + 600), + new NumericDate($iat), + new NumericDate($iat + 600), new JsonValue($jti) ); + if (!is_null($auth_time)) { + $claim_set->addClaim(new JWTClaim(OAuth2Protocol::OAuth2Protocol_AuthTime, new JsonValue($auth_time))); + } + return $claim_set; } /** @@ -719,7 +728,7 @@ public function testClientKeyVerifiedIdTokenHintDoesNotUnlockSubFallback(): void // the sub-based fallback must not be offered to reloadSession(). $this->auth_service->shouldReceive('reloadSession') ->once() - ->withArgs(function ($jti, $user_id = null) { + ->withArgs(function ($jti, $user_id = null, $auth_time = null) { return $jti === 'jti-client-signed' && $user_id === null; }) ->andThrow(new ReloadSessionException('session not found!')); @@ -736,6 +745,84 @@ public function testClientKeyVerifiedIdTokenHintDoesNotUnlockSubFallback(): void * sub-based fallback - reloadSession() receives the resolved user_id. */ public function testServerKeyVerifiedIdTokenHintKeepsSubFallback(): void + { + [$server_jwk, $alg] = $this->setupServerKeySignedClient(); + + $iat = time() - 30; + $hint = $this->signHint($server_jwk, $alg, $this->buildHintClaimSet('999', 'jti-server-signed', $iat)); + + $request = $this->buildOIDCRequest([ + OAuth2Protocol::OAuth2Protocol_IDTokenHint => $hint, + ]); + + $this->setupSecurityContext(); + $this->allowCleanupCalls(); + $login_redirect = $this->expectHintReloadFailureEndsAtLogin(); + + // Key assertion: the signature was verified with the SERVER key, so + // the sub-based fallback is offered to reloadSession(), and with no + // auth_time claim the hint's iat is the attested authentication time. + $this->auth_service->shouldReceive('reloadSession') + ->once() + ->withArgs(function ($jti, $user_id = null, $auth_time = null) use ($iat) { + return $jti === 'jti-server-signed' && $user_id === '999' && $auth_time === $iat; + }) + ->andThrow(new ReloadSessionException('user not found!')); + + $result = $this->grant_type->publicHandle($request); + + $this->assertEquals($login_redirect, $result); + } + + /** + * When the hint carries an explicit auth_time claim (the IDP adds it + * whenever the original request asked for max_age), that value - not + * iat, and never "now" - is what the fallback must register, so that + * shouldForceReLogin() and the next id_token's auth_time stay truthful. + */ + public function testServerKeyVerifiedIdTokenHintForwardsAuthTimeClaimOverIat(): void + { + [$server_jwk, $alg] = $this->setupServerKeySignedClient(); + + $now = time(); + $iat = $now - 10; + $auth_time = $now - 500; + $hint = $this->signHint( + $server_jwk, + $alg, + $this->buildHintClaimSet('999', 'jti-server-signed-auth-time', $iat, $auth_time) + ); + + $request = $this->buildOIDCRequest([ + OAuth2Protocol::OAuth2Protocol_IDTokenHint => $hint, + ]); + + $this->setupSecurityContext(); + $this->allowCleanupCalls(); + $login_redirect = $this->expectHintReloadFailureEndsAtLogin(); + + $this->auth_service->shouldReceive('reloadSession') + ->once() + ->withArgs(function ($jti, $user_id = null, $received_auth_time = null) use ($auth_time) { + return $jti === 'jti-server-signed-auth-time' + && $user_id === '999' + && $received_auth_time === $auth_time; + }) + ->andThrow(new ReloadSessionException('user not found!')); + + $result = $this->grant_type->publicHandle($request); + + $this->assertEquals($login_redirect, $result); + } + + /** + * An RS256 client with no registered signing key and no jwks_uri, so the + * client-key lookup throws RecipientKeyNotFoundException and + * InteractiveGrantType falls back to the IDP's server signing key. + * + * @return array{0: IJWK, 1: string} the server JWK to sign hints with, and the alg + */ + private function setupServerKeySignedClient(): array { $alg = JSONWebSignatureAndEncryptionAlgorithms::RS256; @@ -747,12 +834,6 @@ public function testServerKeyVerifiedIdTokenHintKeepsSubFallback(): void ); $server_jwk->setKeyUse(JSONWebKeyPublicKeyUseValues::Signature)->setId('server-sig-key'); - $hint = $this->signHint($server_jwk, $alg, $this->buildHintClaimSet('999', 'jti-server-signed')); - - $request = $this->buildOIDCRequest([ - OAuth2Protocol::OAuth2Protocol_IDTokenHint => $hint, - ]); - $client = $this->setupValidClient(); $client->shouldReceive('getIdTokenResponseInfo') ->andReturn(new JWTResponseInfo(DigitalSignatures_MACs_Registry::getInstance()->get($alg))); @@ -783,22 +864,7 @@ public function testServerKeyVerifiedIdTokenHintKeepsSubFallback(): void ->with('server-sig-key') ->andReturn($server_key); - $this->setupSecurityContext(); - $this->allowCleanupCalls(); - $login_redirect = $this->expectHintReloadFailureEndsAtLogin(); - - // Key assertion: the signature was verified with the SERVER key, so - // the sub-based fallback is offered to reloadSession(). - $this->auth_service->shouldReceive('reloadSession') - ->once() - ->withArgs(function ($jti, $user_id = null) { - return $jti === 'jti-server-signed' && $user_id === '999'; - }) - ->andThrow(new ReloadSessionException('user not found!')); - - $result = $this->grant_type->publicHandle($request); - - $this->assertEquals($login_redirect, $result); + return [$server_jwk, $alg]; } // ----------------------------------------------------------------------- From 3cfb61e29e073854eb4ea810fc6c009d00f17526 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 22 Sep 2026 15:48:39 -0300 Subject: [PATCH 9/9] refactor(auth): pass id_token_hint reload semantics as a SessionReloadHint value object reloadSession(string $jti, ?string $user_id, ?int $auth_time) admitted states that mean nothing: a user_id without an attested auth_time, or the reverse. The rule "sub-based fallback only for an IDP-signed hint that carries an auth_time" lived as a ternary at the single call site, and the service kept a time() safety net for an auth_time that could never actually be missing. SessionReloadHint makes those states unrepresentable through two named constructors: jtiOnly() for hints verified with a client-controlled key, and withSubFallback() for hints verified with the IDP's own signing key, which requires both user_id and auth_time. Both fallback branches in reloadSession() now share loginFromReloadHint(), and the time() fallback is gone. user_id is typed int end to end (unwrapUserId() returns a string that getUserById(int) was coercing implicitly). No behaviour change. IAuthService has a single implementer and a single caller; tests updated to build the value object and to match on it. --- app/libs/Auth/AuthService.php | 61 ++++----- .../GrantTypes/InteractiveGrantType.php | 12 +- app/libs/OAuth2/Models/SessionReloadHint.php | 117 ++++++++++++++++++ app/libs/Utils/Services/IAuthService.php | 10 +- tests/AuthServiceReloadSessionTest.php | 15 +-- tests/unit/InteractiveGrantTypeTest.php | 25 ++-- 6 files changed, 183 insertions(+), 57 deletions(-) create mode 100644 app/libs/OAuth2/Models/SessionReloadHint.php diff --git a/app/libs/Auth/AuthService.php b/app/libs/Auth/AuthService.php index 480191b2..02a653a5 100644 --- a/app/libs/Auth/AuthService.php +++ b/app/libs/Auth/AuthService.php @@ -32,6 +32,7 @@ use Models\OAuth2\OAuth2OTP; use OAuth2\Exceptions\InvalidOTPException; use OAuth2\Models\IClient; +use OAuth2\Models\SessionReloadHint; use OAuth2\OAuth2Protocol; use OAuth2\Services\IPrincipalService; use OAuth2\Services\ISecurityContextService; @@ -664,34 +665,22 @@ public function getLoggedRPs(): array } /** - * @param string $jti - * @param string|null $user_id - * @param int|null $auth_time + * @param SessionReloadHint $hint * @return void * @throws ReloadSessionException */ - public function reloadSession(string $jti, ?string $user_id = null, ?int $auth_time = null): void + public function reloadSession(SessionReloadHint $hint): void { $former_session_id = Session::getId(); + $jti = $hint->getJti(); Log::debug(sprintf("AuthService::reloadSession jti %s", $jti)); $session_id = $this->cache_service->getSingleValue($jti); Log::debug(sprintf("AuthService::reloadSession session_id %s", $session_id)); if (empty($session_id)) { Log::warning("AuthService::reloadSession session_id is not present at cache"); - if(!is_null($user_id)) { - Log::warning(sprintf("AuthService::reloadSession user id provided %s", $user_id)); - $user = $this->getUserById($user_id); - if (is_null($user) || !$user->canLogin()) - throw new ReloadSessionException('user not found!'); - Auth::login($user); - // Auth::login() alone leaves this session's IDP-specific principal - // state (user_id/auth_time/op_browser_state) unset - every other - // login path in this class pairs it with register(). - // The user did not authenticate now: they authenticated when the IDP - // issued the hint, so register that time (time() is only a safety net). - $this->principal_service->clear(); - $this->principal_service->register($user->getId(), $auth_time ?? time()); + if($hint->allowsSubFallback()) { + $this->loginFromReloadHint($hint); return; } throw new ReloadSessionException('session not found!'); @@ -722,19 +711,8 @@ public function reloadSession(string $jti, ?string $user_id = null, ?int $auth_t Log::warning(sprintf("AuthService::reloadSession ex %s", $ex->getMessage())); Session::setId($former_session_id); Session::start(); - if(!is_null($user_id)) { - Log::warning(sprintf("AuthService::reloadSession user id provided %s", $user_id)); - $user = $this->getUserById($user_id); - if (is_null($user) || !$user->canLogin()) - throw new ReloadSessionException('user not found!'); - Auth::login($user); - // Auth::login() alone leaves this session's IDP-specific principal - // state (user_id/auth_time/op_browser_state) unset - every other - // login path in this class pairs it with register(). - // The user did not authenticate now: they authenticated when the IDP - // issued the hint, so register that time (time() is only a safety net). - $this->principal_service->clear(); - $this->principal_service->register($user->getId(), $auth_time ?? time()); + if($hint->allowsSubFallback()) { + $this->loginFromReloadHint($hint); return; } throw $ex; @@ -748,6 +726,29 @@ public function reloadSession(string $jti, ?string $user_id = null, ?int $auth_t } } + /** + * Sub-based fallback of reloadSession(): logs in the user an IDP-signed + * id_token_hint names, on the caller's current session. + * @param SessionReloadHint $hint must allow the sub fallback + * @throws ReloadSessionException + */ + private function loginFromReloadHint(SessionReloadHint $hint): void + { + $user_id = $hint->getUserId(); + Log::warning(sprintf("AuthService::loginFromReloadHint user id provided %s", $user_id)); + $user = $this->getUserById($user_id); + if (is_null($user) || !$user->canLogin()) + throw new ReloadSessionException('user not found!'); + Auth::login($user); + // Auth::login() alone leaves this session's IDP-specific principal + // state (user_id/auth_time/op_browser_state) unset - every other + // login path in this class pairs it with register(). + // The user did not authenticate now: they authenticated when the IDP + // issued the hint, so register the time the hint attests. + $this->principal_service->clear(); + $this->principal_service->register($user->getId(), $hint->getAuthTime()); + } + /** * @param string $client_id * @param int $id_token_lifetime diff --git a/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php b/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php index 101ab180..155e3ef3 100644 --- a/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php +++ b/app/libs/OAuth2/GrantTypes/InteractiveGrantType.php @@ -41,6 +41,7 @@ use OAuth2\Heuristics\ServerEncryptionKeyFinder; use OAuth2\Heuristics\ServerSigningKeyFinder; use OAuth2\Models\IClient; +use OAuth2\Models\SessionReloadHint; use OAuth2\Repositories\IClientRepository; use OAuth2\Services\ITokenService; use OAuth2\OAuth2Protocol; @@ -629,12 +630,11 @@ protected function processUserHint(OAuth2AuthenticationRequest $request, Client // cached) is only safe for a hint this IDP is proven to have issued AND // that carries an attested authentication time; otherwise degrade to the // jti-only semantics rather than inventing an auth_time. - $this->auth_service->reloadSession - ( - $jti->getValue(), - ($issued_by_this_idp && !is_null($hint_auth_time)) ? $user_id : null, - $hint_auth_time - ); + $reload_hint = ($issued_by_this_idp && !is_null($hint_auth_time)) + ? SessionReloadHint::withSubFallback($jti->getValue(), intval($user_id), $hint_auth_time) + : SessionReloadHint::jtiOnly($jti->getValue()); + + $this->auth_service->reloadSession($reload_hint); $request->markParamAsProcessed(OAuth2Protocol::OAuth2Protocol_IDTokenHint); } diff --git a/app/libs/OAuth2/Models/SessionReloadHint.php b/app/libs/OAuth2/Models/SessionReloadHint.php new file mode 100644 index 00000000..f99f9891 --- /dev/null +++ b/app/libs/OAuth2/Models/SessionReloadHint.php @@ -0,0 +1,117 @@ +jti = $jti; + $this->user_id = $user_id; + $this->auth_time = $auth_time; + } + + /** + * Hint verified with a client-controlled key: jti-only semantics. + * @param string $jti + * @return SessionReloadHint + */ + public static function jtiOnly(string $jti): self + { + return new self($jti, null, null); + } + + /** + * Hint verified with the IDP's own signing key and carrying an attested + * authentication time (its auth_time claim, else iat). + * @param string $jti + * @param int $user_id + * @param int $auth_time epoch the IDP originally attested for that user + * @return SessionReloadHint + */ + public static function withSubFallback(string $jti, int $user_id, int $auth_time): self + { + return new self($jti, $user_id, $auth_time); + } + + /** + * @return string + */ + public function getJti(): string + { + return $this->jti; + } + + /** + * @return bool + */ + public function allowsSubFallback(): bool + { + return !is_null($this->user_id); + } + + /** + * @return int|null + */ + public function getUserId(): ?int + { + return $this->user_id; + } + + /** + * @return int|null + */ + public function getAuthTime(): ?int + { + return $this->auth_time; + } +} diff --git a/app/libs/Utils/Services/IAuthService.php b/app/libs/Utils/Services/IAuthService.php index cebe3601..0eaed916 100644 --- a/app/libs/Utils/Services/IAuthService.php +++ b/app/libs/Utils/Services/IAuthService.php @@ -17,6 +17,7 @@ use Models\OAuth2\Client; use Models\OAuth2\OAuth2OTP; use OAuth2\Models\IClient; +use OAuth2\Models\SessionReloadHint; use OpenId\Models\IOpenIdUser; /** * Interface IAuthService @@ -137,12 +138,13 @@ public function registerRPLogin(string $client_id); public function getLoggedRPs():array; /** - * @param string $jti - * @param string|null $user_id user to log in when the cached session can't be resumed (IDP-signed hints only) - * @param int|null $auth_time epoch the IDP originally attested for that user (hint's auth_time, else iat) + * Resumes the OP session an id_token_hint refers to (through its cached jti). + * When the hint allows the sub-based fallback and the cached session can't be + * resumed, logs in the user the hint names with the auth_time it attests. + * @param SessionReloadHint $hint * @return void */ - public function reloadSession(string $jti, ?string $user_id = null, ?int $auth_time = null):void; + public function reloadSession(SessionReloadHint $hint):void; const LOGGED_RELAYING_PARTIES_COOKIE_NAME = 'rps'; diff --git a/tests/AuthServiceReloadSessionTest.php b/tests/AuthServiceReloadSessionTest.php index 5622f4b5..bd863ff2 100644 --- a/tests/AuthServiceReloadSessionTest.php +++ b/tests/AuthServiceReloadSessionTest.php @@ -20,6 +20,7 @@ use Mockery; use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration; use OAuth2\Models\Principal; +use OAuth2\Models\SessionReloadHint; use OAuth2\Services\IPrincipalService; use OAuth2\Services\ISecurityContextService; use OpenId\Services\IUserService; @@ -128,7 +129,7 @@ public function testCacheMissFallbackRejectsUserThatCannotLogin(): void $this->expectException(ReloadSessionException::class); - $this->service->reloadSession('jti-1', '42'); + $this->service->reloadSession(SessionReloadHint::withSubFallback('jti-1', 42, 1700000000)); } public function testCacheMissFallbackRegistersPrincipalOnSuccess(): void @@ -146,7 +147,7 @@ public function testCacheMissFallbackRegistersPrincipalOnSuccess(): void $this->mock_principal_service->expects($this->once())->method('clear'); $this->mock_principal_service->expects($this->once())->method('register')->with(42, 1700000000); - $this->service->reloadSession('jti-1', '42', 1700000000); + $this->service->reloadSession(SessionReloadHint::withSubFallback('jti-1', 42, 1700000000)); } // ----------------------------------------------------------------------- @@ -189,7 +190,7 @@ public function testCatchFallbackRejectsUserThatCannotLogin(): void $this->expectException(ReloadSessionException::class); - $this->service->reloadSession('jti-1', '99'); + $this->service->reloadSession(SessionReloadHint::withSubFallback('jti-1', 99, 1700000000)); } public function testCatchFallbackRegistersPrincipalOnSuccess(): void @@ -210,11 +211,11 @@ public function testCatchFallbackRegistersPrincipalOnSuccess(): void $this->mock_principal_service->expects($this->once())->method('clear'); $this->mock_principal_service->expects($this->once())->method('register')->with(99, 1700000000); - $this->service->reloadSession('jti-1', '99', 1700000000); + $this->service->reloadSession(SessionReloadHint::withSubFallback('jti-1', 99, 1700000000)); } /** - * No $user_id fallback was provided: the failed resume must propagate, + * A jti-only hint (no sub fallback): the failed resume must propagate, * not return as if reloadSession() had succeeded. */ public function testCatchRethrowsWhenNoFallbackUserIdProvided(): void @@ -229,7 +230,7 @@ public function testCatchRethrowsWhenNoFallbackUserIdProvided(): void $this->expectException(ReloadSessionException::class); - $this->service->reloadSession('jti-1'); + $this->service->reloadSession(SessionReloadHint::jtiOnly('jti-1')); } // ----------------------------------------------------------------------- @@ -264,6 +265,6 @@ public function testNonReloadSessionExceptionRestoresFormerSessionAndRethrows(): $this->expectException(RuntimeException::class); $this->expectExceptionMessage('DB is down'); - $this->service->reloadSession('jti-1', '5'); + $this->service->reloadSession(SessionReloadHint::withSubFallback('jti-1', 5, 1700000000)); } } diff --git a/tests/unit/InteractiveGrantTypeTest.php b/tests/unit/InteractiveGrantTypeTest.php index 9d821cf6..b3cb544e 100644 --- a/tests/unit/InteractiveGrantTypeTest.php +++ b/tests/unit/InteractiveGrantTypeTest.php @@ -37,6 +37,7 @@ use OAuth2\Models\JWTResponseInfo; use OAuth2\Models\Principal; use OAuth2\Models\SecurityContext; +use OAuth2\Models\SessionReloadHint; use OAuth2\OAuth2Message; use OAuth2\OAuth2Protocol; use OAuth2\Repositories\IClientRepository; @@ -698,7 +699,7 @@ private function expectHintReloadFailureEndsAtLogin(): string * The client signs its id_tokens with HS512, i.e. with its own client * secret - the IDP and the client share that key, so a token minted by * the client verifies exactly like an IDP-issued one. Such a hint must - * reach reloadSession() WITHOUT the sub-based fallback ($user_id null). + * reach reloadSession() WITHOUT the sub-based fallback (a jti-only hint). */ public function testClientKeyVerifiedIdTokenHintDoesNotUnlockSubFallback(): void { @@ -728,8 +729,8 @@ public function testClientKeyVerifiedIdTokenHintDoesNotUnlockSubFallback(): void // the sub-based fallback must not be offered to reloadSession(). $this->auth_service->shouldReceive('reloadSession') ->once() - ->withArgs(function ($jti, $user_id = null, $auth_time = null) { - return $jti === 'jti-client-signed' && $user_id === null; + ->withArgs(function (SessionReloadHint $hint) { + return $hint->getJti() === 'jti-client-signed' && !$hint->allowsSubFallback(); }) ->andThrow(new ReloadSessionException('session not found!')); @@ -742,7 +743,7 @@ public function testClientKeyVerifiedIdTokenHintDoesNotUnlockSubFallback(): void * Positive control: a hint verified with the IDP's own RS256 server key * (the client has no registered signing key and no jwks_uri, so the * client-key lookup throws RecipientKeyNotFoundException) keeps the - * sub-based fallback - reloadSession() receives the resolved user_id. + * sub-based fallback - reloadSession() receives a hint carrying the resolved user_id. */ public function testServerKeyVerifiedIdTokenHintKeepsSubFallback(): void { @@ -764,8 +765,11 @@ public function testServerKeyVerifiedIdTokenHintKeepsSubFallback(): void // auth_time claim the hint's iat is the attested authentication time. $this->auth_service->shouldReceive('reloadSession') ->once() - ->withArgs(function ($jti, $user_id = null, $auth_time = null) use ($iat) { - return $jti === 'jti-server-signed' && $user_id === '999' && $auth_time === $iat; + ->withArgs(function (SessionReloadHint $hint) use ($iat) { + return $hint->getJti() === 'jti-server-signed' + && $hint->allowsSubFallback() + && $hint->getUserId() === 999 + && $hint->getAuthTime() === $iat; }) ->andThrow(new ReloadSessionException('user not found!')); @@ -803,10 +807,11 @@ public function testServerKeyVerifiedIdTokenHintForwardsAuthTimeClaimOverIat(): $this->auth_service->shouldReceive('reloadSession') ->once() - ->withArgs(function ($jti, $user_id = null, $received_auth_time = null) use ($auth_time) { - return $jti === 'jti-server-signed-auth-time' - && $user_id === '999' - && $received_auth_time === $auth_time; + ->withArgs(function (SessionReloadHint $hint) use ($auth_time) { + return $hint->getJti() === 'jti-server-signed-auth-time' + && $hint->allowsSubFallback() + && $hint->getUserId() === 999 + && $hint->getAuthTime() === $auth_time; }) ->andThrow(new ReloadSessionException('user not found!'));