From 23fb0f85016d256e3048f16c25b2b7412f7d9b07 Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 24 Aug 2026 09:59:40 -0400 Subject: [PATCH 1/5] fix(db): reject invalid and duplicate migration identifiers Validate migration names against the supported version and timestamp format. Reject overlapping identifiers discovered in multiple migration files instead of silently overwriting one of them. This hardens migrations by preventing them from running in a non-deterministic order when an unsupported migration identifier mix might produce a plausible but incorrect migration order. Assisted-by: GitHubCopilot:gpt-5.6-sol Signed-off-by: Josh --- lib/private/DB/MigrationService.php | 44 ++++++++++++++++++++++------- 1 file changed, 34 insertions(+), 10 deletions(-) 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); } From fe51eefea5e23b6ba561870604a87cfc9b1efa7a Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 24 Aug 2026 10:14:16 -0400 Subject: [PATCH 2/5] test(db): reject malformed migration filenames Assisted-by: Copilot:gpt-5.6-sol Signed-off-by: Josh --- tests/lib/DB/MigrationServiceTest.php | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests/lib/DB/MigrationServiceTest.php b/tests/lib/DB/MigrationServiceTest.php index 0366344a0cbb9..ec063b9f72030 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,30 @@ 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 testExecuteUnknownStep(): void { $this->expectException(\InvalidArgumentException::class); $this->expectExceptionMessage('Version 20170130180000 is unknown.'); From 514d7af5649f671339cc0693b615077f1a01d8e0 Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 24 Aug 2026 10:16:18 -0400 Subject: [PATCH 3/5] test(db): reject overlapping identifiers Signed-off-by: Josh --- tests/lib/DB/MigrationServiceTest.php | 30 +++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/tests/lib/DB/MigrationServiceTest.php b/tests/lib/DB/MigrationServiceTest.php index ec063b9f72030..c2a8ebdff0c11 100644 --- a/tests/lib/DB/MigrationServiceTest.php +++ b/tests/lib/DB/MigrationServiceTest.php @@ -94,6 +94,36 @@ public function testGetAvailableVersionsRejectsInvalidIdentifier(string $fileNam $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.'); From c54c292a77643ca48ab231cca3fda0d536551b27 Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 24 Aug 2026 10:23:39 -0400 Subject: [PATCH 4/5] test(db): reject malformed database identifiers Signed-off-by: Josh --- tests/lib/DB/MigrationServiceTest.php | 96 ++++++++++++++++++++------- 1 file changed, 73 insertions(+), 23 deletions(-) diff --git a/tests/lib/DB/MigrationServiceTest.php b/tests/lib/DB/MigrationServiceTest.php index c2a8ebdff0c11..ebfeaa3c777e8 100644 --- a/tests/lib/DB/MigrationServiceTest.php +++ b/tests/lib/DB/MigrationServiceTest.php @@ -277,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()); @@ -319,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); } } @@ -1074,4 +1082,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(); + } } From a8f1397c0fcf573b7d989496dedbec02c61d7b9e Mon Sep 17 00:00:00 2001 From: Josh Date: Mon, 24 Aug 2026 10:25:50 -0400 Subject: [PATCH 5/5] test(db): reject malformed explicit targets For sortMigrations() Assisted-by: Copilot:gpt-5.6-sol Signed-off-by: Josh --- tests/lib/DB/MigrationServiceTest.php | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/lib/DB/MigrationServiceTest.php b/tests/lib/DB/MigrationServiceTest.php index ebfeaa3c777e8..120a779a7d105 100644 --- a/tests/lib/DB/MigrationServiceTest.php +++ b/tests/lib/DB/MigrationServiceTest.php @@ -380,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) {