From d3b09832ab700c40964aa165fd8f9ac7c8b8d45d Mon Sep 17 00:00:00 2001 From: blaipr Date: Wed, 26 Aug 2026 23:04:09 +0200 Subject: [PATCH] fix: a temporary master password is shown once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `TemporaryMasterPass::create()` puts the generated key into the issuing administrator's session, so that the page rendered after the generating request can show it. There is nowhere else to keep it for that one hop — it is deliberately never persisted in plaintext, only its bcrypt hash and a blob that is useless without it. Nothing ever removed it again. There was no clear to call: the whole codebase has exactly one set and one get for `tempmasterpass`. And `ConfigManager\IndexController` reads it on every load of the Configuration page — `getGridTabs()` builds every tab the user can reach in one pass, so it does not even depend on which tab is being viewed. So a value meant to be shown once was rendered into the HTML again on every later visit, for the life of that session: after it had expired, after its recipients had used it, and after the master password had been rotated, none of which touches the session copy. Reading it now takes it. The method is `takeTemporaryMasterPass()` rather than a getter with a surprise in it, since the name is the only thing that stops the next caller assuming otherwise — and there is exactly one caller. What this does not change: the value is one global secret for the instance, and the group picker on the issuing form chooses who it is *mailed* to, not who may use it — `checkKey()` takes no user and `LoginMasterPass::loadTemporary()` checks none. That is sysPass's design, one shared master password with a per-user wrapped copy, not a missing check. Checked by dropping the clear: the new test fails on the second read. --- src/Domain/Core/Context/SessionContext.php | 2 +- .../ConfigManager/IndexController.php | 2 +- src/Infrastructure/Context/Session.php | 22 ++++++++++++-- .../Infrastructure/Context/SessionTest.php | 29 +++++++++++++++++-- 4 files changed, 48 insertions(+), 7 deletions(-) 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'); } /**