From 9574814cf0fff4dc09dbb84524b56d6dc9de703b Mon Sep 17 00:00:00 2001
From: Arthur Schiwon <blizzz@arthur-schiwon.de>
Date: Thu, 2 May 2024 21:07:03 +0200
Subject: [PATCH] fix(loginflow): log more session related data to setting
 state token

- exisiting values
- set values
- retry once more if set and generated token differ
- log session state

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
---
 core/Controller/ClientFlowLoginController.php | 21 +++++++++++++++++++
 .../ClientFlowLoginV2Controller.php           | 21 +++++++++++++++++++
 lib/private/Session/CryptoSessionData.php     | 17 ++++++++++++++-
 lib/private/Session/Internal.php              |  4 ++++
 4 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/core/Controller/ClientFlowLoginController.php b/core/Controller/ClientFlowLoginController.php
index 6a2c0491260..25a887cbec1 100644
--- a/core/Controller/ClientFlowLoginController.php
+++ b/core/Controller/ClientFlowLoginController.php
@@ -135,6 +135,12 @@ class ClientFlowLoginController extends Controller {
 			);
 		}
 
+		$oldStateToken = $this->session->get(self::STATE_NAME);
+		logger('core')->error('Fetching old state token - expected to be null', [
+			'loginFlow' => 'v1',
+			'existingStateToken' => $oldStateToken,
+		]);
+
 		$stateToken = $this->random->generate(
 			64,
 			ISecureRandom::CHAR_LOWER . ISecureRandom::CHAR_UPPER . ISecureRandom::CHAR_DIGITS
@@ -144,6 +150,21 @@ class ClientFlowLoginController extends Controller {
 			'token' => $stateToken,
 		]);
 
+		$setStateToken = $this->session->get(self::STATE_NAME);
+		logger('core')->error('Fetching set state token - expected to match generated one', [
+			'loginFlow' => 'v1',
+			'generatedStateToken' => $stateToken,
+			'setStateToken' => $setStateToken,
+		]);
+
+		if ($stateToken !== $setStateToken) {
+			logger('core')->error('Generate and set state token mismatch, trying to set it again one more time', [
+				'loginFlow' => 'v1',
+				'stateToken' => $stateToken,
+			]);
+			$this->session->set(self::STATE_NAME, $stateToken);
+		}
+
 		$csp = new Http\ContentSecurityPolicy();
 		if ($client) {
 			$csp->addAllowedFormActionDomain($client->getRedirectUri());
diff --git a/core/Controller/ClientFlowLoginV2Controller.php b/core/Controller/ClientFlowLoginV2Controller.php
index 96a700e6244..0c4bc58f7fb 100644
--- a/core/Controller/ClientFlowLoginV2Controller.php
+++ b/core/Controller/ClientFlowLoginV2Controller.php
@@ -113,12 +113,33 @@ class ClientFlowLoginV2Controller extends Controller {
 			return $this->loginTokenForbiddenResponse();
 		}
 
+		$oldStateToken = $this->session->get(self::STATE_NAME);
+		logger('core')->error('Fetching old state token - expected to be null', [
+			'loginFlow' => 'v2',
+			'existingStateToken' => $oldStateToken,
+		]);
+
 		$stateToken = $this->random->generate(
 			64,
 			ISecureRandom::CHAR_LOWER . ISecureRandom::CHAR_UPPER . ISecureRandom::CHAR_DIGITS
 		);
 		$this->session->set(self::STATE_NAME, $stateToken);
 
+		$setStateToken = $this->session->get(self::STATE_NAME);
+		logger('core')->error('Fetching set state token - expected to match generated one', [
+			'loginFlow' => 'v2',
+			'generatedStateToken' => $stateToken,
+			'setStateToken' => $setStateToken,
+		]);
+
+		if ($stateToken !== $setStateToken) {
+			logger('core')->error('Generate and set state token mismatch, trying to set it again one more time', [
+				'loginFlow' => 'v2',
+				'stateToken' => $stateToken,
+			]);
+			$this->session->set(self::STATE_NAME, $stateToken);
+		}
+
 		return new StandaloneTemplateResponse(
 			$this->appName,
 			'loginflowv2/authpicker',
diff --git a/lib/private/Session/CryptoSessionData.php b/lib/private/Session/CryptoSessionData.php
index 85a34267472..7aea557910f 100644
--- a/lib/private/Session/CryptoSessionData.php
+++ b/lib/private/Session/CryptoSessionData.php
@@ -89,12 +89,27 @@ class CryptoSessionData implements \ArrayAccess, ISession {
 	 * @param mixed $value
 	 */
 	public function set(string $key, $value) {
-		if ($this->get($key) === $value) {
+		$existingValue = $this->get($key);
+		if ($existingValue === $value) {
+			if ($key === 'client.flow.v2.state.token' || $key === 'client.flow.state.token') {
+				logger('core')->error('State token value is already present!', [
+					'loginFlow' => str_contains($key, 'v2') ? 'v2' : 'v1',
+					'stateToken' => $value,
+					'existingStateToken' => $existingValue,
+				]);
+			}
 			// Do not write the session if the value hasn't changed to avoid reopening
 			return;
 		}
 
 		$reopened = $this->reopen();
+		if ($key === 'client.flow.v2.state.token' || $key === 'client.flow.state.token') {
+			logger('core')->error('Reporting on whether session was reopened', [
+				'loginFlow' => str_contains($key, 'v2') ? 'v2' : 'v1',
+				'sessionReopened' => $reopened,
+			]);
+		}
+
 		$this->sessionValues[$key] = $value;
 		if ($key === 'client.flow.v2.state.token' || $key === 'client.flow.state.token') {
 			logger('core')->error('Saving state token with session', [
diff --git a/lib/private/Session/Internal.php b/lib/private/Session/Internal.php
index b465bcd3eda..f04531f7c82 100644
--- a/lib/private/Session/Internal.php
+++ b/lib/private/Session/Internal.php
@@ -217,6 +217,10 @@ class Internal extends Session {
 			}
 			return $result;
 		} catch (\Error $e) {
+			$this->logger?->error('trapping a session error', [
+				'loginFlow' => '?',
+				'exception' => $e,
+			]);
 			$this->trapError($e->getCode(), $e->getMessage());
 		}
 	}