From adfac3e30ebb5a407cb72e3546f0756ab63f298d Mon Sep 17 00:00:00 2001
From: Dan Brown <development@danb.me>
Date: Wed, 3 Dec 2025 13:34:00 +0000
Subject: [PATCH] OIDC: Updated state handling to prevent loss from other
 requests

Which was occuring in chrome, where background requests to the PWA
manifest, or opensearch, endpoint caused OIDC to fail due to lost state
since it was only flashed to the session.
This persists it with a manual TTL.

Added tests to cover.
Manually tested against Azure.
For #5929
---
 app/Access/Controllers/OidcController.php | 22 +++++++------
 tests/Auth/OidcTest.php                   | 39 ++++++++++++++++++++---
 2 files changed, 47 insertions(+), 14 deletions(-)

diff --git a/app/Access/Controllers/OidcController.php b/app/Access/Controllers/OidcController.php
index 055d4c140..654ed6928 100644
--- a/app/Access/Controllers/OidcController.php
+++ b/app/Access/Controllers/OidcController.php
@@ -9,11 +9,9 @@ use Illuminate\Http\Request;
 
 class OidcController extends Controller
 {
-    protected OidcService $oidcService;
-
-    public function __construct(OidcService $oidcService)
-    {
-        $this->oidcService = $oidcService;
+    public function __construct(
+        protected OidcService $oidcService
+    ) {
         $this->middleware('guard:oidc');
     }
 
@@ -30,7 +28,7 @@ class OidcController extends Controller
             return redirect('/login');
         }
 
-        session()->flash('oidc_state', $loginDetails['state']);
+        session()->put('oidc_state', time() . ':' . $loginDetails['state']);
 
         return redirect($loginDetails['url']);
     }
@@ -41,10 +39,16 @@ class OidcController extends Controller
      */
     public function callback(Request $request)
     {
-        $storedState = session()->pull('oidc_state');
         $responseState = $request->query('state');
+        $splitState =  explode(':', session()->pull('oidc_state', ':'), 2);
+        if (count($splitState) !== 2) {
+            $splitState = [null, null];
+        }
 
-        if ($storedState !== $responseState) {
+        [$storedStateTime, $storedState] = $splitState;
+        $threeMinutesAgo = time() - 3 * 60;
+
+        if (!$storedState || $storedState !== $responseState || intval($storedStateTime) < $threeMinutesAgo) {
             $this->showErrorNotification(trans('errors.oidc_fail_authed', ['system' => config('oidc.name')]));
 
             return redirect('/login');
@@ -62,7 +66,7 @@ class OidcController extends Controller
     }
 
     /**
-     * Log the user out then start the OIDC RP-initiated logout process.
+     * Log the user out, then start the OIDC RP-initiated logout process.
      */
     public function logout()
     {
diff --git a/tests/Auth/OidcTest.php b/tests/Auth/OidcTest.php
index a0db1c2ba..710e53757 100644
--- a/tests/Auth/OidcTest.php
+++ b/tests/Auth/OidcTest.php
@@ -138,7 +138,7 @@ class OidcTest extends TestCase
     {
         // Start auth
         $this->post('/oidc/login');
-        $state = session()->get('oidc_state');
+        $state = explode(':', session()->get('oidc_state'), 2)[1];
 
         $transactions = $this->mockHttpClient([$this->getMockAuthorizationResponse([
             'email' => 'benny@example.com',
@@ -190,6 +190,35 @@ class OidcTest extends TestCase
         $this->assertSessionError('Login using SingleSignOn-Testing failed, system did not provide successful authorization');
     }
 
+    public function test_callback_works_even_if_other_request_made_by_session()
+    {
+        $this->mockHttpClient([$this->getMockAuthorizationResponse([
+            'email' => 'benny@example.com',
+            'sub'   => 'benny1010101',
+        ])]);
+
+        $this->post('/oidc/login');
+        $state = explode(':', session()->get('oidc_state'), 2)[1];
+
+        $this->get('/');
+
+        $resp = $this->get("/oidc/callback?code=SplxlOBeZQQYbYS6WxSbIA&state={$state}");
+        $resp->assertRedirect('/');
+    }
+
+    public function test_callback_fails_if_state_timestamp_is_too_old()
+    {
+        $this->post('/oidc/login');
+        $state = explode(':', session()->get('oidc_state'), 2)[1];
+        session()->put('oidc_state', (time() - 60 * 4) . ':' . $state);
+
+        $this->get('/');
+
+        $resp = $this->get("/oidc/callback?code=SplxlOBeZQQYbYS6WxSbIA&state={$state}");
+        $resp->assertRedirect('/login');
+        $this->assertSessionError('Login using SingleSignOn-Testing failed, system did not provide successful authorization');
+    }
+
     public function test_dump_user_details_option_outputs_as_expected()
     {
         config()->set('oidc.dump_user_details', true);
@@ -797,7 +826,7 @@ class OidcTest extends TestCase
     {
         // Start auth
         $resp = $this->post('/oidc/login');
-        $state = session()->get('oidc_state');
+        $state = explode(':', session()->get('oidc_state'), 2)[1];
 
         $pkceCode = session()->get('oidc_pkce_code');
         $this->assertGreaterThan(30, strlen($pkceCode));
@@ -825,7 +854,7 @@ class OidcTest extends TestCase
     {
         config()->set('oidc.display_name_claims', 'first_name|last_name');
         $this->post('/oidc/login');
-        $state = session()->get('oidc_state');
+        $state = explode(':', session()->get('oidc_state'), 2)[1];
 
         $client = $this->mockHttpClient([
             $this->getMockAuthorizationResponse(['name' => null]),
@@ -973,7 +1002,7 @@ class OidcTest extends TestCase
         ]);
 
         $this->post('/oidc/login');
-        $state = session()->get('oidc_state');
+        $state = explode(':', session()->get('oidc_state'), 2)[1];
         $client = $this->mockHttpClient([$this->getMockAuthorizationResponse([
             'groups' => [],
         ])]);
@@ -999,7 +1028,7 @@ class OidcTest extends TestCase
     protected function runLogin($claimOverrides = [], $additionalHttpResponses = []): TestResponse
     {
         $this->post('/oidc/login');
-        $state = session()->get('oidc_state');
+        $state = explode(':', session()->get('oidc_state'), 2)[1] ?? '';
         $this->mockHttpClient([$this->getMockAuthorizationResponse($claimOverrides), ...$additionalHttpResponses]);
 
         return $this->get('/oidc/callback?code=SplxlOBeZQQYbYS6WxSbIA&state=' . $state);