From 6693d39e0db9f3596a1c2202efe080d0ff4b5e03 Mon Sep 17 00:00:00 2001 From: blaipr Date: Thu, 24 Sep 2026 19:55:06 +0200 Subject: [PATCH] fix: a master password is held to its minimum at every door --- CLAUDE.md | 2 + src/Application/Crypt/Services/MasterPass.php | 32 +++++++++++++++ .../Install/Services/Installer.php | 9 +--- .../ConfigEncryption/SaveController.php | 9 ++++ .../ConfigEncryption/SaveRefusalsTest.php | 39 ++++++++++++++++++ .../Crypt/Services/MasterPassTest.php | 41 ++++++++++++++++--- 6 files changed, 120 insertions(+), 12 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 140341523..edfc136ff 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -476,6 +476,8 @@ wrote `private` / `privateGroup` as sent; it is now `Account::privacyAllowedFor( the account will actually have. The account manager's grid left out an account private to somebody else, while its delete and bulk edit acted on whatever ids were posted; `Account::assertNotWithheldAsPrivate()` guards those. +The master password's eleven-character minimum was enforced only by the installer (and in bytes); +`MasterPass::assertLongEnough()` now holds the web settings, their hash-only option and the CLI to it. It runs the other way too, and that is the more useful half: the API re-reads the user on every request and refuses a disabled one, while the web trusted what login had put in the session — so disabling an account stopped its token at once and left its browser session working, and since the diff --git a/src/Application/Crypt/Services/MasterPass.php b/src/Application/Crypt/Services/MasterPass.php index b1d685074..eba701c64 100644 --- a/src/Application/Crypt/Services/MasterPass.php +++ b/src/Application/Crypt/Services/MasterPass.php @@ -35,6 +35,8 @@ use SP\Domain\Common\Services\ServiceException; use SP\Application\Config\Ports\ConfigService; use SP\Domain\Core\Exceptions\ConstraintException; +use SP\Domain\Core\Exceptions\InvalidArgumentException; +use SP\Domain\Core\Exceptions\SPException; use SP\Domain\Core\Exceptions\QueryException; use SP\Domain\Crypt\Dtos\UpdateMasterPassRequest; use SP\Application\Crypt\Ports\MasterPassService; @@ -52,6 +54,11 @@ final class MasterPass extends Service implements MasterPassService public const PARAM_MASTER_PASS_TIME = 'lastupdatempass'; public const PARAM_MASTER_PASS_HASH = 'masterPwd'; + /** + * The shortest master password accepted, in characters + */ + public const MIN_LENGTH = 11; + public function __construct( Application $application, private readonly ConfigService $configService, @@ -93,6 +100,29 @@ public function checkMasterPassword(string $masterPassword): bool return false; } + /** + * Refuse a master password shorter than MIN_LENGTH characters. + * + * Every account secret is sealed with the master password, so its minimum is the floor under + * the whole vault. The installer enforced it and nothing else did: the web's encryption + * settings (both the full rotation and the "hash only" option) and `sp:updateMasterPassword` + * accepted any non-empty value, so an installation could be re-keyed to `a` the day after it + * was set up. The installer also measured it with strlen(), in bytes, so four CJK characters + * (twelve bytes) passed a rule that promises eleven characters. + * + * @throws InvalidArgumentException + */ + public static function assertLongEnough(?string $masterPassword): void + { + if (mb_strlen($masterPassword ?? '') < self::MIN_LENGTH) { + throw new InvalidArgumentException( + __u('Master password too short'), + SPException::ERROR, + sprintf(__u('The Master Password length need to be at least %d characters'), self::MIN_LENGTH) + ); + } + } + /** * Re-encrypts everything under a new master password, or leaves it all as it was. * @@ -129,6 +159,8 @@ public function changeMasterPassword(UpdateMasterPassRequest $request): void throw ServiceException::error(__u('Ey, this is a DEMO!!')); } + self::assertLongEnough($request->getNewMasterPass()); + $this->repository->transactionAware( function () use ($request) { $this->accountMasterPasswordService->updateMasterPassword($request); diff --git a/src/Application/Install/Services/Installer.php b/src/Application/Install/Services/Installer.php index 2f984cfc3..64d772874 100644 --- a/src/Application/Install/Services/Installer.php +++ b/src/Application/Install/Services/Installer.php @@ -27,6 +27,7 @@ namespace SP\Application\Install\Services; +use SP\Application\Crypt\Services\MasterPass; use Exception; use SP\Application\Config\Ports\ConfigFileService; use SP\Application\Config\Ports\ConfigService; @@ -145,13 +146,7 @@ private function checkData(): void ); } - if (strlen($this->installData->getMasterPassword() ?? '') < 11) { - throw new InvalidArgumentException( - __u('Master password too short'), - SPException::CRITICAL, - __u('The Master Password length need to be at least 11 characters') - ); - } + MasterPass::assertLongEnough($this->installData->getMasterPassword()); if ($this->installData->getMasterPassword() !== $this->installData->getMasterPasswordRepeat()) { throw new InvalidArgumentException( diff --git a/src/Infrastructure/Adapter/In/Web/Controllers/ConfigEncryption/SaveController.php b/src/Infrastructure/Adapter/In/Web/Controllers/ConfigEncryption/SaveController.php index a70a7d6d2..5b3ab482c 100644 --- a/src/Infrastructure/Adapter/In/Web/Controllers/ConfigEncryption/SaveController.php +++ b/src/Infrastructure/Adapter/In/Web/Controllers/ConfigEncryption/SaveController.php @@ -42,6 +42,7 @@ use SP\Domain\Crypt\Dtos\UpdateMasterPassRequest; use SP\Application\Crypt\Ports\MasterPassService; use SP\Application\Crypt\Services\MasterPass; +use SP\Domain\Core\Exceptions\InvalidArgumentException; use SP\Domain\Core\Exceptions\NoSuchItemException; use SP\Infrastructure\Adapter\In\Web\Controllers\SimpleControllerBase; use SP\Infrastructure\Adapter\In\Web\Controllers\Helpers\SimpleControllerHelper; @@ -100,6 +101,14 @@ public function saveAction(): ActionResponse return ActionResponse::error(__u('Master passwords do not match')); } + // Asked here as well as in the rotation, because the "hash only" option below never + // reaches it. + try { + MasterPass::assertLongEnough($newMasterPass); + } catch (InvalidArgumentException $e) { + return ActionResponse::error($e->getMessage(), $e->getHint()); + } + if (!$this->masterPassService->checkMasterPassword($currentMasterPass)) { return ActionResponse::error(__u('The current master password does not match')); } diff --git a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/ConfigEncryption/SaveRefusalsTest.php b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/ConfigEncryption/SaveRefusalsTest.php index d492b39f7..a0e5eccbe 100644 --- a/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/ConfigEncryption/SaveRefusalsTest.php +++ b/tests/Integration/Infrastructure/Adapter/In/Web/Controllers/ConfigEncryption/SaveRefusalsTest.php @@ -191,6 +191,40 @@ public function aDemoInstanceRefusesIt() $this->whenSaving($this->form(), demo: true); } + /** + * A new master password is held to the minimum the installer set, here as there. Every secret + * is sealed with it, so this was a way to re-key the whole vault to `a`. + * + * @throws ContainerExceptionInterface + * @throws Exception + * @throws NotFoundExceptionInterface + */ + #[Test] + #[BodyChecker('outputCheckerTooShort')] + public function aNewPasswordShorterThanTheMinimumIsRefused() + { + $this->whenSaving($this->form(['new_masterpass' => 'short_pass', 'new_masterpass_repeat' => 'short_pass'])); + } + + /** + * Including by the "hash only" option, which replaces the stored hash without the rotation + * and so never reaches the service's own check. + * + * @throws ContainerExceptionInterface + * @throws Exception + * @throws NotFoundExceptionInterface + */ + #[Test] + #[BodyChecker('outputCheckerTooShort')] + public function aShortPasswordIsRefusedWhenOnlyTheHashChanges() + { + $this->whenSaving( + $this->form( + ['new_masterpass' => 'short_pass', 'new_masterpass_repeat' => 'short_pass', 'no_account_change' => 'true'] + ) + ); + } + /** * The form as it is submitted when everything is right. * @@ -258,6 +292,11 @@ private function outputCheckerNotConfirmed(string $output): void self::assertSame('The password update must be confirmed', json_decode($output)->description); } + private function outputCheckerTooShort(string $output): void + { + self::assertSame('Master password too short', json_decode($output)->description); + } + private function outputCheckerSame(string $output): void { self::assertSame('Passwords are the same', json_decode($output)->description); diff --git a/tests/Unit/Application/Crypt/Services/MasterPassTest.php b/tests/Unit/Application/Crypt/Services/MasterPassTest.php index b42685bdc..4f10eafbf 100644 --- a/tests/Unit/Application/Crypt/Services/MasterPassTest.php +++ b/tests/Unit/Application/Crypt/Services/MasterPassTest.php @@ -38,6 +38,7 @@ use SP\Application\Config\Ports\ConfigService; use SP\Domain\Core\Exceptions\ConstraintException; use SP\Domain\Core\Exceptions\QueryException; +use SP\Domain\Core\Exceptions\InvalidArgumentException; use SP\Domain\Crypt\Dtos\UpdateMasterPassRequest; use SP\Application\Crypt\Services\MasterPass; use SP\Application\CustomField\Ports\CustomFieldCryptService; @@ -172,7 +173,7 @@ public function testChangeMasterPassword() ->method('transactionAware') ->with(self::withResolveCallableCallback()); - $request = new UpdateMasterPassRequest('123', '456', $hash); + $request = new UpdateMasterPassRequest('123', 'a_new_master_pass', $hash); $this->accountMasterPasswordService ->expects(self::once()) @@ -215,7 +216,7 @@ public function testChangeMasterPassword() */ public function testTheStoredHashIsWrittenInsideTheTransaction(): void { - $request = new UpdateMasterPassRequest('123', '456', self::$faker->sha1()); + $request = new UpdateMasterPassRequest('123', 'a_new_master_pass', self::$faker->sha1()); // No withResolveCallableCallback(): the closure is handed over and never invoked. $this->repository @@ -245,7 +246,7 @@ public function testChangeMasterPasswordAbortedOnError(): void ->method('transactionAware') ->with(self::withResolveCallableCallback()); - $request = new UpdateMasterPassRequest('123', '456', $hash); + $request = new UpdateMasterPassRequest('123', 'a_new_master_pass', $hash); $this->accountMasterPasswordService ->expects(self::once()) @@ -310,7 +311,7 @@ public function testADemoInstanceRefusesTheRotation(): void $this->expectException(ServiceException::class); $this->expectExceptionMessage('Ey, this is a DEMO!!'); - $this->masterPass->changeMasterPassword(new UpdateMasterPassRequest('old', 'new', self::$faker->sha1())); + $this->masterPass->changeMasterPassword(new UpdateMasterPassRequest('old', 'a_new_master_pass', self::$faker->sha1())); } /** @@ -342,7 +343,7 @@ static function (string $param) use (&$saved): bool { ); try { - $this->masterPass->changeMasterPassword(new UpdateMasterPassRequest('old', 'new', self::$faker->sha1())); + $this->masterPass->changeMasterPassword(new UpdateMasterPassRequest('old', 'a_new_master_pass', self::$faker->sha1())); } catch (ServiceException) { // asserted above; this test is about what reached the config } @@ -368,4 +369,34 @@ protected function setUp(): void $this->repository ); } + /** + * The minimum is counted in characters, as its message promises: four CJK characters are + * twelve bytes, which a byte count let past a rule of eleven. + */ + public function testTheMinimumLengthIsCountedInCharacters(): void + { + MasterPass::assertLongEnough(str_repeat('a', MasterPass::MIN_LENGTH)); + + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('Master password too short'); + + MasterPass::assertLongEnough('密码很长的'); + } + + /** + * The rotation refuses a new master password under the minimum before it touches anything — + * `sp:updateMasterPassword` reaches it with no other check on the way. + * + * @throws ServiceException + */ + public function testChangeMasterPasswordRefusesAShortPasswordBeforeReEncryptingAnything(): void + { + $this->accountMasterPasswordService->expects(self::never())->method('updateMasterPassword'); + $this->customFieldCryptService->expects(self::never())->method('updateMasterPassword'); + + $this->expectException(InvalidArgumentException::class); + + $this->masterPass->changeMasterPassword(new UpdateMasterPassRequest('old', 'short', self::$faker->sha1())); + } + }