From 23871ae5f38db9279cd7da16f446e19f1cdaf292 Mon Sep 17 00:00:00 2001 From: blaipr Date: Thu, 17 Sep 2026 03:42:26 +0200 Subject: [PATCH] fix: an install rollback drops only the database it created MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MysqlSetup::rollback() ended a failed install with an unconditional DROP DATABASE, and nothing established whose database it was. install/install is unauthenticated by necessity, and Installer::install() calls checkDatabaseAvailability() before anything is created and createDatabase() well after it, with no lock between — so two requests both pass the availability check, the second fails on CREATE SCHEMA because the name is taken, and its rollback dropped the database the first had just finished installing into. config.xml already said installed=1, so the instance claimed to be installed with no schema behind it. Two near-simultaneous requests is all it takes; an impatient double-click on the install button will do it. The same method already drops the runtime user only when $dbUser names one — only when this run created it. The database had no such flag, and now does: Installer sets it the moment createDatabase() returns, and the drop is gated on it. The hazard was understood when this was written — the comment above checkDatabaseAvailability() says a failure there must not trigger a rollback that could touch pre-existing data. It was enforced at the check rather than at the rollback, and only the check is racy. This does not serialise concurrent installs: the loser still fails, correctly, on the schema already existing. It makes losing the race harmless rather than destructive. --- .../Install/Services/Installer.php | 8 ++- .../Install/Services/MysqlSetup.php | 35 +++++++--- .../Install/Services/DatabaseSetupService.php | 7 +- .../Install/Services/InstallerTest.php | 70 +++++++++++++++++++ .../Install/Services/MySQLTest.php | 21 +++++- 5 files changed, 127 insertions(+), 14 deletions(-) diff --git a/src/Application/Install/Services/Installer.php b/src/Application/Install/Services/Installer.php index 3f5a738f3..2f984cfc3 100644 --- a/src/Application/Install/Services/Installer.php +++ b/src/Application/Install/Services/Installer.php @@ -229,12 +229,18 @@ private function install(): void $configData->setDbPass($dbPass); } + $createdDatabase = false; + try { // Inside the try: setupDbUser() already created the runtime user, so a // failure saving the config must roll it back too $this->config->save($configData, false); $this->databaseSetup->createDatabase($dbUser); + + // Only now may a rollback drop it. Until this line the database is either absent or + // somebody else's, and the rollback below used to drop it either way. + $createdDatabase = true; $this->databaseSetup->createDBStructure(); $this->databaseSetup->checkConnection(); @@ -264,7 +270,7 @@ private function install(): void // back over the admin connection $this->databaseConnectionData->refreshFromInstallData($this->installData); - $this->databaseSetup->rollback($dbUser); + $this->databaseSetup->rollback($dbUser, $createdDatabase); throw $e instanceof SPException ? $e diff --git a/src/Application/Install/Services/MysqlSetup.php b/src/Application/Install/Services/MysqlSetup.php index b63f56b02..33e74aeb8 100644 --- a/src/Application/Install/Services/MysqlSetup.php +++ b/src/Application/Install/Services/MysqlSetup.php @@ -345,7 +345,7 @@ public function checkDatabaseExists(): bool * Best-effort: a rollback failure must never mask the error that triggered * it, and one failed statement must not stop the remaining cleanup. */ - public function rollback(?string $dbUser = null): void + public function rollback(?string $dbUser = null, bool $createdDatabase = false): void { try { $dbc = $this->dbStorage->getConnectionSimple(); @@ -385,13 +385,32 @@ public function rollback(?string $dbUser = null): void $this->execBestEffort($dbc, 'SET FOREIGN_KEY_CHECKS = 1'); } else { - $this->execBestEffort( - $dbc, - sprintf( - 'DROP DATABASE IF EXISTS `%s`', - $this->installData->getDbName() - ) - ); + // Only the database this run created, the way the user below is only dropped when + // this run created that. + // + // The drop used to be unconditional, and nothing established whose database it was. + // `install/install` is unauthenticated by necessity, `checkDatabaseAvailability()` + // runs before anything is created and `createDatabase()` well after it, with no lock + // between them — so two requests (an impatient double-click is enough) both pass the + // availability check, the second fails on `CREATE SCHEMA` because the name is now + // taken, and its rollback dropped the database the first had just finished installing + // into. `config.xml` already said `installed=1`, so the instance claimed to be + // installed with no schema behind it, and every later request went to + // `error/databaseError`. + // + // The comment above `checkDatabaseAvailability()` in `Installer::install()` shows the + // hazard was understood — "a failure here must not trigger a rollback, which could + // otherwise touch pre-existing data". This is that same rule, applied where the + // rollback happens rather than where the check does. + if ($createdDatabase) { + $this->execBestEffort( + $dbc, + sprintf( + 'DROP DATABASE IF EXISTS `%s`', + $this->installData->getDbName() + ) + ); + } if ($dbUser) { $this->execBestEffort( diff --git a/src/Domain/Install/Services/DatabaseSetupService.php b/src/Domain/Install/Services/DatabaseSetupService.php index 49e76f490..7dca80dfa 100644 --- a/src/Domain/Install/Services/DatabaseSetupService.php +++ b/src/Domain/Install/Services/DatabaseSetupService.php @@ -73,8 +73,9 @@ public function checkConnection(): void; /** * Roll back the installation in case of failure. * - * Removes the sysPass database and user. Best-effort: it must never - * throw, so it cannot mask the error that triggered it. + * Removes what *this* run created, and only that: the user when `$dbUser` names one, and the + * database when `$createdDatabase` says this run created it. Best-effort: it must never throw, + * so it cannot mask the error that triggered it. */ - public function rollback(?string $dbUser = null): void; + public function rollback(?string $dbUser = null, bool $createdDatabase = false): void; } diff --git a/tests/Unit/Application/Install/Services/InstallerTest.php b/tests/Unit/Application/Install/Services/InstallerTest.php index 2959986ee..4e425b5ba 100644 --- a/tests/Unit/Application/Install/Services/InstallerTest.php +++ b/tests/Unit/Application/Install/Services/InstallerTest.php @@ -395,6 +395,76 @@ public function testHostingModeIsUsed(): void $this->assertEquals($params->getDbAdminPass(), $configData->getDbPass()); } + /** + * A rollback drops the database only when this run was the one that created it. + * + * `install/install` is unauthenticated by necessity, and the drop used to be unconditional — + * nothing established whose database it was. `checkDatabaseAvailability()` runs before anything + * is created and `createDatabase()` well after, with no lock between them, so two requests (an + * impatient double-click is enough) both pass the availability check; the second fails on + * `CREATE SCHEMA` because the name is now taken, and its rollback dropped the database the + * first had just finished installing into. `config.xml` already said `installed=1`, so the + * instance claimed to be installed with no schema behind it. + * + * Here the failure happens *before* `createDatabase()` returns, which is exactly the losing + * request's shape. + * + * @throws InvalidArgumentException + * @throws SPException + */ + public function testARollbackBeforeTheDatabaseWasCreatedDropsNoDatabase(): void + { + // Non-hosting on purpose: the unconditional DROP DATABASE was on that branch. The runtime + // user is created before the try block, so the pair has to be there to destructure. + $this->databaseSetup->method('setupDbUser')->willReturn(['sp_user', 'sp_pass']); + + $this->databaseSetup + ->method('createDatabase') + ->willThrowException(SPException::error('Error while creating the DB')); + + $this->databaseSetup + ->expects($this->once()) + ->method('rollback') + ->with(self::anything(), false); + + $params = $this->getInstallData(); + + $installer = $this->getDefaultInstaller(); + + $this->expectException(SPException::class); + + $installer->run($params); + } + + /** + * ...and it does drop it once this run has created it, or a genuinely failed install would + * leave its own half-built schema behind and refuse every retry. + * + * @throws InvalidArgumentException + * @throws SPException + */ + public function testARollbackAfterTheDatabaseWasCreatedDropsIt(): void + { + $this->databaseSetup->method('setupDbUser')->willReturn(['sp_user', 'sp_pass']); + + $this->databaseSetup + ->method('createDBStructure') + ->willThrowException(SPException::error('Error while creating the DB structure')); + + $this->databaseSetup + ->expects($this->once()) + ->method('rollback') + ->with(self::anything(), true); + + $params = $this->getInstallData(); + + $installer = $this->getDefaultInstaller(); + + $this->expectException(SPException::class); + + $installer->run($params); + } + /** * @throws InvalidArgumentException * @throws SPException diff --git a/tests/Unit/Application/Install/Services/MySQLTest.php b/tests/Unit/Application/Install/Services/MySQLTest.php index 3aef42dcb..3db91731c 100644 --- a/tests/Unit/Application/Install/Services/MySQLTest.php +++ b/tests/Unit/Application/Install/Services/MySQLTest.php @@ -554,7 +554,8 @@ public function testRollbackIsSuccessful(): void $this->pdo->method('quote')->willReturnArgument(0); - $this->mysqlService->rollback($this->configData->getDbUser()); + // createdDatabase: this run made it, so the rollback owns it. + $this->mysqlService->rollback($this->configData->getDbUser(), true); } public function testRollbackIsSuccessfulWithSameDnsHost(): void @@ -585,7 +586,8 @@ public function testRollbackIsSuccessfulWithSameDnsHost(): void $this->pdo->method('quote')->willReturnArgument(0); - $this->mysqlService->rollback($this->configData->getDbUser()); + // createdDatabase: this run made it, so the rollback owns it. + $this->mysqlService->rollback($this->configData->getDbUser(), true); } public function testRollbackIsSuccessfulWithHostingMode(): void @@ -619,6 +621,21 @@ public function testRollbackNeverThrows(): void ->willThrowException(new PDOException('test')); // Best-effort: a rollback failure must not mask the error that triggered it + $this->mysqlService->rollback(null, true); + } + + /** + * A rollback that did not create the database does not drop one. + * + * `install/install` is unauthenticated by necessity, and the drop used to be unconditional — + * nothing established whose database it was. Two requests both pass + * `checkDatabaseAvailability()` before either creates anything, and the one that then loses the + * `CREATE SCHEMA` race used to drop the database the winner had just installed into. + */ + public function testRollbackDropsNoDatabaseItDidNotCreate(): void + { + $this->pdo->expects(self::never())->method('exec'); + $this->mysqlService->rollback(); }