diff --git a/src/Domain/Core/Context/SessionContext.php b/src/Domain/Core/Context/SessionContext.php index d694c0a64..e9a39e423 100644 --- a/src/Domain/Core/Context/SessionContext.php +++ b/src/Domain/Core/Context/SessionContext.php @@ -75,7 +75,7 @@ public function getAuthCompleted(); * * @return ?string */ - public function getTemporaryMasterPass(): ?string; + public function takeTemporaryMasterPass(): ?string; /** * Return the public key diff --git a/src/Infrastructure/Adapter/In/Web/Controllers/ConfigManager/IndexController.php b/src/Infrastructure/Adapter/In/Web/Controllers/ConfigManager/IndexController.php index 221c7efca..f0b5d43d9 100644 --- a/src/Infrastructure/Adapter/In/Web/Controllers/ConfigManager/IndexController.php +++ b/src/Infrastructure/Adapter/In/Web/Controllers/ConfigManager/IndexController.php @@ -442,7 +442,7 @@ protected function getEncryptionConfig(): DataTab ); $template->assign('tempMasterAttempts', $tempMasterAttempts); - $template->assign('tempMasterPass', $this->session->getTemporaryMasterPass()); + $template->assign('tempMasterPass', $this->session->takeTemporaryMasterPass()); $template->assign( 'userGroups', diff --git a/src/Infrastructure/Context/Session.php b/src/Infrastructure/Context/Session.php index ee2a84c23..0f66f3d7f 100644 --- a/src/Infrastructure/Context/Session.php +++ b/src/Infrastructure/Context/Session.php @@ -210,11 +210,27 @@ public function getAuthCompleted(): bool } /** - * Returns the temporary master password + * Takes the temporary master password, and forgets it + * + * It is put here by the administrator who generated it, so that the page rendered after the + * generating request can show it once — there is nowhere else to keep it for that one hop, and + * it is deliberately not persisted anywhere in plaintext. + * + * Reading it clears it. It used to stay for the life of that administrator's session, and the + * Configuration page reads it on every load, so a value meant to be shown once was rendered + * into the HTML again on every later visit — after it had expired, after its recipients had + * used it, and after the master password had been rotated. Nothing anywhere removed it: there + * was no clear for it to call. */ - public function getTemporaryMasterPass(): ?string + public function takeTemporaryMasterPass(): ?string { - return $this->getContextKey('tempmasterpass'); + $tempMasterPass = $this->getContextKey('tempmasterpass'); + + if ($tempMasterPass !== null) { + $this->setContextKey('tempmasterpass', null); + } + + return $tempMasterPass; } /** diff --git a/tests/Unit/Infrastructure/Context/SessionTest.php b/tests/Unit/Infrastructure/Context/SessionTest.php index 3d877b61f..75d22cc26 100644 --- a/tests/Unit/Infrastructure/Context/SessionTest.php +++ b/tests/Unit/Infrastructure/Context/SessionTest.php @@ -397,11 +397,36 @@ public function theTemporaryMasterPasswordIsCarried() { $session = $this->givenAStartedSession(); - self::assertNull($session->getTemporaryMasterPass()); + self::assertNull($session->takeTemporaryMasterPass()); $session->setTemporaryMasterPass('a-temporary-password'); - self::assertSame('a-temporary-password', $session->getTemporaryMasterPass()); + self::assertSame('a-temporary-password', $session->takeTemporaryMasterPass()); + } + + /** + * And carried exactly once. + * + * It lives in the session only so that the page rendered after the generating request can show + * it — there is nowhere else to keep it for that one hop, and it is deliberately not persisted + * in plaintext anywhere. It used to stay for the life of the administrator's session, and + * ConfigManager's index reads it on every load, so a value meant to be shown once was rendered + * into the HTML again on every later visit to the Configuration page: after it had expired, + * after its recipients had used it, and after the master password had been rotated. Nothing + * removed it, because there was nothing that could — no clear existed. + * + * @throws ContextException + * @throws SPException + */ + #[Test] + public function theTemporaryMasterPasswordIsCarriedOnlyOnce() + { + $session = $this->givenAStartedSession(); + + $session->setTemporaryMasterPass('a-temporary-password'); + + self::assertSame('a-temporary-password', $session->takeTemporaryMasterPass()); + self::assertNull($session->takeTemporaryMasterPass(), 'it must not survive being read'); } /**