From 8dca212439e2cdfd7496831d16c5320dadd48db1 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon 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 --- core/Controller/ClientFlowLoginController.php | 21 +++++++++++++++++++ .../ClientFlowLoginV2Controller.php | 21 +++++++++++++++++++ lib/private/Session/CryptoSessionData.php | 17 ++++++++++++++- lib/private/Session/Internal.php | 5 +++++ 4 files changed, 63 insertions(+), 1 deletion(-) diff --git a/core/Controller/ClientFlowLoginController.php b/core/Controller/ClientFlowLoginController.php index 3d2f1bfb0d4f1..30a868333178f 100644 --- 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 index cef2094168b85..cd5c9c39b6c74 100644 --- 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 index 7f93f3d189de9..763741e14da19 100644 --- 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 index cae139018f896..05115647b8a4e 100644 --- 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()); } }