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 <[email protected]>
nextcloud · Jul 18, 2024 · 8dca212 · 8dca212
1 parent 3ea5469
commit 8dca212
Show file tree

Hide file tree

Showing 4 changed files with 63 additions and 1 deletion.
diff --git a/core/Controller/ClientFlowLoginController.php b/core/Controller/ClientFlowLoginController.php
@@ -178,6 +178,12 @@ public function showAuthPickerPage(string $clientIdentifier = '', string $user =
 			);
 		}
 
+		$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
@@ -187,6 +193,21 @@ public function showAuthPickerPage(string $clientIdentifier = '', string $user =
 			'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
@@ -125,12 +125,33 @@ public function showAuthPickerPage($user = ''): StandaloneTemplateResponse {
 			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
@@ -109,12 +109,27 @@ protected function initializeSession() {
 	 * @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
@@ -36,6 +36,7 @@
 use OC\Authentication\Exceptions\InvalidTokenException;
 use OC\Authentication\Token\IProvider;
 use OCP\Session\Exceptions\SessionNotAvailableException;
+use function OCP\Log\logger;
 
 /**
  * Class Internal
@@ -222,6 +223,10 @@ private function invoke(string $functionName, array $parameters = [], bool $sile
 				return call_user_func_array($functionName, $parameters);
 			}
 		} catch (\Error $e) {
+			logger('core')->error('trapping a session error', [
+				'loginFlow' => '?',
+				'exception' => $e,
+			]);
 			$this->trapError($e->getCode(), $e->getMessage());
 		}
 	}