From c163199afe3d2d41b33faed7e9f0c1050198b6fb Mon Sep 17 00:00:00 2001
From: blaipr <blaipr@hotmail.com>
Date: Tue, 18 Aug 2026 22:18:18 +0200
Subject: [PATCH] Store the master password's hash inside the rotation
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Rotating the master password re-encrypts every account, every history row and
every custom field, and those three passes already ran in one transaction so a
failure part-way rolled them all back. The hash describing the new password was
written afterwards:

    $this->repository->transactionAware(function () use ($request) { ... }, $this);

    $this->updateConfig($request->getHash());

A failure in those two config writes — or a process that stopped between them —
left every secret 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 about which half to save. `transactionAware()`
runs on the shared Database, so the hash joins the same transaction as the
secrets it describes and the rotation either happens or does not.

Asserting a rollback through mocks is awkward, so the test hands
`transactionAware()` a double that never runs its closure: nothing inside the
transaction happens, therefore nothing should be written at all. With the hash
saved afterwards it is written regardless — which is exactly the state that
outlives a rollback. The existing test covers the other direction, a re-key
failure leaving the hash alone, so this closes the pair.
---
 src/Application/Crypt/Services/MasterPass.php | 18 +++++++++--
 .../Crypt/Services/MasterPassTest.php         | 31 +++++++++++++++++++
 2 files changed, 47 insertions(+), 2 deletions(-)

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