diff --git a/lib/private/DB/MigrationService.php b/lib/private/DB/MigrationService.php index 761fa5398b69e..f7be4a7fd824d 100644 --- a/lib/private/DB/MigrationService.php +++ b/lib/private/DB/MigrationService.php @@ -198,17 +198,32 @@ public function getAvailableVersions(): array { } protected function sortMigrations(string $a, string $b): int { - preg_match('/(\d+)Date(\d+)/', basename($a), $matchA); - preg_match('/(\d+)Date(\d+)/', basename($b), $matchB); - if (!empty($matchA) && !empty($matchB)) { - $versionA = (int)$matchA[1]; - $versionB = (int)$matchB[1]; - if ($versionA !== $versionB) { - return ($versionA < $versionB) ? -1 : 1; - } - return strnatcmp($matchA[2], $matchB[2]); + [$versionA, $dateA] = $this->parseMigrationVersion($a); + [$versionB, $dateB] = $this->parseMigrationVersion($b); + + if ($versionA !== $versionB) { + return $versionA <=> $versionB; + } + + return strcmp($dateA, $dateB); + } + + /** + * @return array{int, string} + */ + private function parseMigrationVersion(string $value): array { + $name = basename($value); + + if (preg_match('/^Version(\d{1,16})Date(\d{14})\.php$/', $name, $matches) !== 1 + && preg_match('/^(\d{1,16})Date(\d{14})$/', $name, $matches) !== 1 + ) { + throw new \InvalidArgumentException( + 'Invalid migration version "' . $value . '" for app "' . $this->getApp() + . '". Expected "Date".' + ); } - return strnatcmp(basename($a), basename($b)); + + return [(int)$matches[1], $matches[2]]; } /** @@ -232,6 +247,7 @@ protected function findMigrations(): array { usort($files, $this->sortMigrations(...)); $migrations = []; + $migrationFiles = []; foreach ($files as $file) { $className = basename($file, '.php'); @@ -241,6 +257,14 @@ protected function findMigrations(): array { "Cannot load a migrations with the name '$version' because it is a reserved number" ); } + if (isset($migrationFiles[$version])) { + throw new \InvalidArgumentException( + "Cannot load migration '$version' for app '{$this->appName}' because it is defined by both " + . "'{$migrationFiles[$version]}' and '$file'" + ); + } + + $migrationFiles[$version] = $file; $migrations[$version] = sprintf('%s\\%s', $this->migrationsNamespace, $className); } diff --git a/tests/lib/DB/MigrationServiceTest.php b/tests/lib/DB/MigrationServiceTest.php index 0366344a0cbb9..120a779a7d105 100644 --- a/tests/lib/DB/MigrationServiceTest.php +++ b/tests/lib/DB/MigrationServiceTest.php @@ -21,6 +21,7 @@ use OC\DB\SchemaWrapper; use OCP\App\AppPathNotFoundException; use OCP\IDBConnection; +use OCP\ITempManager; use OCP\Migration\IMigrationStep; use OCP\Server; use PHPUnit\Framework\Attributes\DataProvider; @@ -69,6 +70,60 @@ public function testCore(): void { $this->assertEquals('test_oc_migrations', $migrationService->getMigrationsTableName()); } + public static function dataInvalidMigrationFileName(): array { + return [ + 'missing version' => ['VersionDate20200819121721.php'], + 'non-numeric version' => ['VersionFooDate20200819121721.php'], + 'invalid separator' => ['Version10000Data20200819121721.php'], + 'short timestamp' => ['Version10000Date2020081912172.php'], + 'non-numeric timestamp' => ['Version10000Date2020081912172A.php'], + 'unexpected suffix' => ['Version10000Date20200819121721Extra.php'], + ]; + } + + #[DataProvider('dataInvalidMigrationFileName')] + public function testGetAvailableVersionsRejectsInvalidIdentifier(string $fileName): void { + $directory = Server::get(ITempManager::class)->getTemporaryFolder(); + self::assertNotFalse(\touch($directory . '/' . $fileName)); + + $migrationService = $this->createMigrationServiceForDirectory($directory); + + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage($fileName); + + $migrationService->getAvailableVersions(); + } + + public function testGetAvailableVersionsRejectsDuplicateIdentifiers(): void { + $directory = Server::get(ITempManager::class)->getTemporaryFolder(); + $firstDirectory = $directory . '/first'; + $secondDirectory = $directory . '/second'; + + self::assertTrue(\mkdir($firstDirectory)); + self::assertTrue(\mkdir($secondDirectory)); + + $fileName = 'Version10000Date20200819121721.php'; + $firstFile = $firstDirectory . '/' . $fileName; + $secondFile = $secondDirectory . '/' . $fileName; + + self::assertNotFalse(\touch($firstFile)); + self::assertNotFalse(\touch($secondFile)); + + $migrationService = $this->createMigrationServiceForDirectory($directory); + + try { + $migrationService->getAvailableVersions(); + self::fail('Expected duplicate migration identifiers to be rejected'); + } catch (\InvalidArgumentException $e) { + self::assertStringContainsString( + '10000Date20200819121721', + $e->getMessage(), + ); + self::assertStringContainsString($firstFile, $e->getMessage()); + self::assertStringContainsString($secondFile, $e->getMessage()); + } + } + public function testExecuteUnknownStep(): void { $this->expectException(\InvalidArgumentException::class); $this->expectExceptionMessage('Version 20170130180000 is unknown.'); @@ -222,22 +277,8 @@ public function testGetMigration($alias, $expected): void { public function testGetMigratedVersionsSortsByVersionThenDate(): void { /** @var Connection $db */ $db = Server::get(IDBConnection::class); - $appId = 'migration_sort_' . bin2hex(random_bytes(8)); - - $migrationService = new class('testing', $db, $appId) extends MigrationService { - public function __construct( - string $appName, - Connection $connection, - private string $migrationApp, - ) { - parent::__construct($appName, $connection); - } - - #[\Override] - public function getApp(): string { - return $this->migrationApp; - } - }; + $appId = 'migration_sort_' . \bin2hex(\random_bytes(8)); + $migrationService = $this->createMigrationServiceForApp($db, $appId); // Ensure the migrations table exists before inserting the fixtures. self::assertSame([], $migrationService->getMigratedVersions()); @@ -264,13 +305,35 @@ public function getApp(): string { '20000Date20240718031959', ], $migrationService->getMigratedVersions()); } finally { - $qb = $db->getQueryBuilder(); - $qb->delete('migrations') - ->where($qb->expr()->eq( - 'app', - $qb->createNamedParameter($appId), - )) - ->executeStatement(); + $this->deleteMigrationVersions($db, $appId); + } + } + + #[Group('DB')] + public function testGetMigratedVersionsRejectsInvalidIdentifier(): void { + /** @var Connection $db */ + $db = Server::get(IDBConnection::class); + $appId = 'migration_invalid_' . \bin2hex(\random_bytes(8)); + $migrationService = $this->createMigrationServiceForApp($db, $appId); + + // Ensure the migrations table exists before inserting the fixture. + self::assertSame([], $migrationService->getMigratedVersions()); + + $invalidVersion = '10000Data20200819121721'; + + try { + $db->insertIfNotExist('*PREFIX*migrations', [ + 'app' => $appId, + 'version' => $invalidVersion, + ]); + + $migrationService->getMigratedVersions(); + self::fail('Expected an invalid stored migration identifier to be rejected'); + } catch (\InvalidArgumentException $e) { + self::assertStringContainsString($invalidVersion, $e->getMessage()); + self::assertStringContainsString($appId, $e->getMessage()); + } finally { + $this->deleteMigrationVersions($db, $appId); } } @@ -317,6 +380,24 @@ public function testMigrate(): void { ], $calls); } + public function testMigrateRejectsInvalidTargetIdentifier(): void { + $migrationService = $this->getMockBuilder(MigrationService::class) + ->onlyMethods(['getMigratedVersions', 'findMigrations', 'executeStep']) + ->setConstructorArgs(['testing', $this->db]) + ->getMock(); + + $migrationService->method('getMigratedVersions')->willReturn([]); + $migrationService->method('findMigrations')->willReturn([ + '10000Date20200819121721' => 'A', + ]); + $migrationService->expects(self::never())->method('executeStep'); + + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('invalid-target'); + + $migrationService->migrate('invalid-target'); + } + #[DataProvider('dataEnsureNamingConstraintsTableName')] public function testEnsureNamingConstraintsTableName(string $name, int $prefixLength, bool $tableExists, bool $throws): void { if ($throws) { @@ -1019,4 +1100,46 @@ public function testEnsureOracleConstraintsStringLength4000(): void { $this->migrationService->ensureOracleConstraints($sourceSchema, $schema); } + + private function createMigrationServiceForDirectory(string $directory): MigrationService { + $migrationService = new MigrationService('testing', $this->db); + + $property = new \ReflectionProperty( + MigrationService::class, + 'migrationsPath', + ); + $property->setValue($migrationService, $directory); + + return $migrationService; + } + + private function createMigrationServiceForApp( + Connection $db, + string $appId, + ): MigrationService { + return new class('testing', $db, $appId) extends MigrationService { + public function __construct( + string $appName, + Connection $connection, + private string $migrationApp, + ) { + parent::__construct($appName, $connection); + } + + #[\Override] + public function getApp(): string { + return $this->migrationApp; + } + }; + } + + private function deleteMigrationVersions(Connection $db, string $appId): void { + $qb = $db->getQueryBuilder(); + $qb->delete('migrations') + ->where($qb->expr()->eq( + 'app', + $qb->createNamedParameter($appId), + )) + ->executeStatement(); + } }