webapp/bookstack
1
0
Fork 0
mirror of https://github.com/BookStackApp/BookStack.git synced 2025-12-04 05:22:38 +03:00
Code Activity

OIDC: Updated state handling to prevent loss from other requests

Browse Source 
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
This commit is contained in:
Dan Brown
2025-12-03 13:34:00 +00:00
parent 9de294343d
commit adfac3e30e
2 changed files with 47 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files

22
app/Access/Controllers/OidcController.php
View File

@@ -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()
{

39
tests/Auth/OidcTest.php
View File

@@ -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);