diff --git a/src/Application/Crypt/Services/MasterPass.php b/src/Application/Crypt/Services/MasterPass.php index 44b64b410..52fbe9c69 100644 --- a/src/Application/Crypt/Services/MasterPass.php +++ b/src/Application/Crypt/Services/MasterPass.php @@ -93,6 +93,20 @@ public function checkMasterPassword(string $masterPassword): bool } /** + * Re-encrypts everything under a new master password, or leaves it all as it was. + * + * The hash belongs inside the transaction with the secrets it describes. The three + * re-encryption passes were already rolled back together, but the hash was stored afterwards, + * so a failure in those two writes — or a process that stopped between them — left every + * account, history row and custom field re-keyed to the new password while the application + * went on believing the old one. `checkMasterPassword()` compares against that hash, so the + * new password is refused and the old one opens nothing: an instance nobody can unlock, and no + * way back without editing the database by hand. + * + * The ordering was not a choice between which half to save. `transactionAware()` runs on the + * shared `Database`, so these writes join the same transaction as the re-encryption, and the + * rotation either happens or does not. + * * @throws Exception */ public function changeMasterPassword(UpdateMasterPassRequest $request): void @@ -102,11 +116,11 @@ function () use ($request) { $this->accountMasterPasswordService->updateMasterPassword($request); $this->accountMasterPasswordService->updateHistoryMasterPassword($request); $this->customFieldCryptService->updateMasterPassword($request); + + $this->updateConfig($request->getHash()); }, $this ); - - $this->updateConfig($request->getHash()); } /** diff --git a/tests/Unit/Application/Crypt/Services/MasterPassTest.php b/tests/Unit/Application/Crypt/Services/MasterPassTest.php index b14679fde..cc6a067d8 100644 --- a/tests/Unit/Application/Crypt/Services/MasterPassTest.php +++ b/tests/Unit/Application/Crypt/Services/MasterPassTest.php @@ -197,6 +197,37 @@ public function testChangeMasterPassword() $this->masterPass->changeMasterPassword($request); } + /** + * The stored hash is written inside the transaction, with the secrets it describes. + * + * The three re-encryption passes were already rolled back together, but the hash was saved + * after the commit — so a failure in those two writes, or a process that stopped between them, + * left every account, history row and custom field re-keyed to the new password while the + * application went on believing the old one. `checkMasterPassword()` compares against that + * hash, so the new password is refused and the old one opens nothing. + * + * Asserted by giving `transactionAware()` a double that never runs its closure: nothing inside + * the transaction happens, so nothing at all should be written. With the hash saved afterwards + * it is written regardless, which is exactly the state that outlives a rollback. + * + * @throws Exception + */ + public function testTheStoredHashIsWrittenInsideTheTransaction(): void + { + $request = new UpdateMasterPassRequest('123', '456', self::$faker->sha1()); + + // No withResolveCallableCallback(): the closure is handed over and never invoked. + $this->repository + ->expects(self::once()) + ->method('transactionAware'); + + $this->configService + ->expects(self::never()) + ->method('save'); + + $this->masterPass->changeMasterPassword($request); + } + /** * Regression: when one of the re-key sub-services throws (simulating a partial * re-key failure), changeMasterPassword must propagate the exception and must NOT