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>
This commit is contained in:
Arthur Schiwon 2024-05-02 21:07:03 +02:00
parent 98fc0a40be
commit 9574814cf0
No known key found for this signature in database
GPG key ID: 7424F1874854DF23
4 changed files with 62 additions and 1 deletions

View file

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

View file

@ -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',

View file

@ -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', [

View file

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