From ac178d318b29ca3a3fd1318cc467b2deb0ca2093 Mon Sep 17 00:00:00 2001 From: mscherer Date: Mon, 19 Jan 2026 09:00:05 +0100 Subject: [PATCH 1/5] Fix release readiness issues for 5.x - Add cycle detection to seed dependency ordering to prevent infinite recursion when seeds have circular dependencies - Add 'unsigned' to valid column options for API consistency with 'signed' - Fix SQL injection vulnerability in MysqlAdapter by replacing addslashes() with proper driver escaping for column comments - Fix non-existent $io->error() method call in DumpCommand (should be err()) - Document SQL Server check constraints as unsupported with improved error messages guiding users to use raw SQL --- src/Command/DumpCommand.php | 2 +- src/Db/Adapter/MysqlAdapter.php | 4 +++- src/Db/Adapter/SqlserverAdapter.php | 19 +++++++++++++--- src/Db/Table/Column.php | 1 + src/Migration/Manager.php | 34 ++++++++++++++++++++++++++--- 5 files changed, 52 insertions(+), 8 deletions(-) diff --git a/src/Command/DumpCommand.php b/src/Command/DumpCommand.php index 0e694b41c..53c79026e 100644 --- a/src/Command/DumpCommand.php +++ b/src/Command/DumpCommand.php @@ -141,7 +141,7 @@ public function execute(Arguments $args, ConsoleIo $io): ?int return self::CODE_SUCCESS; } - $io->error("An error occurred while writing dump file `{$filePath}`"); + $io->err("An error occurred while writing dump file `{$filePath}`"); return self::CODE_ERROR; } diff --git a/src/Db/Adapter/MysqlAdapter.php b/src/Db/Adapter/MysqlAdapter.php index eaca68bdb..0873581a4 100644 --- a/src/Db/Adapter/MysqlAdapter.php +++ b/src/Db/Adapter/MysqlAdapter.php @@ -725,7 +725,9 @@ protected function getRenameColumnInstructions(string $tableName, string $column foreach ($rows as $row) { if (strcasecmp($row['Field'], $columnName) === 0) { $null = $row['Null'] === 'NO' ? 'NOT NULL' : 'NULL'; - $comment = isset($row['Comment']) ? ' COMMENT ' . '\'' . addslashes($row['Comment']) . '\'' : ''; + $comment = isset($row['Comment']) && $row['Comment'] !== '' + ? ' COMMENT ' . $this->getConnection()->getDriver()->schemaValue($row['Comment']) + : ''; // create the extra string by also filtering out the DEFAULT_GENERATED option (MySQL 8 fix) $extras = array_filter( diff --git a/src/Db/Adapter/SqlserverAdapter.php b/src/Db/Adapter/SqlserverAdapter.php index 14602abc8..e1f55a21e 100644 --- a/src/Db/Adapter/SqlserverAdapter.php +++ b/src/Db/Adapter/SqlserverAdapter.php @@ -1105,27 +1105,40 @@ private function updateSQLForIdentityInsert(string $tableName, string $sql): str /** * @inheritDoc + * + * Note: Check constraints are not supported for SQL Server adapter. + * This method returns an empty array. Use raw SQL via execute() if you need + * check constraints on SQL Server. */ protected function getCheckConstraints(string $tableName): array { - // TODO: Implement check constraints for SQL Server return []; } /** * @inheritDoc + * + * @throws \BadMethodCallException Check constraints are not supported for SQL Server. */ protected function getAddCheckConstraintInstructions(TableMetadata $table, CheckConstraint $checkConstraint): AlterInstructions { - throw new BadMethodCallException('Check constraints are not yet implemented for SQL Server adapter'); + throw new BadMethodCallException( + 'Check constraints are not supported for the SQL Server adapter. ' . + 'Use $this->execute() with raw SQL to add check constraints.', + ); } /** * @inheritDoc + * + * @throws \BadMethodCallException Check constraints are not supported for SQL Server. */ protected function getDropCheckConstraintInstructions(string $tableName, string $constraintName): AlterInstructions { - throw new BadMethodCallException('Check constraints are not yet implemented for SQL Server adapter'); + throw new BadMethodCallException( + 'Check constraints are not supported for the SQL Server adapter. ' . + 'Use $this->execute() with raw SQL to drop check constraints.', + ); } /** diff --git a/src/Db/Table/Column.php b/src/Db/Table/Column.php index a16585f9e..98f28b313 100644 --- a/src/Db/Table/Column.php +++ b/src/Db/Table/Column.php @@ -789,6 +789,7 @@ protected function getValidOptions(): array 'update', 'comment', 'signed', + 'unsigned', 'timezone', 'properties', 'values', diff --git a/src/Migration/Manager.php b/src/Migration/Manager.php index 2ce00a172..862e76278 100644 --- a/src/Migration/Manager.php +++ b/src/Migration/Manager.php @@ -1101,18 +1101,46 @@ protected function getSeedDependenciesInstances(SeedInterface $seed): array * Order seeds by dependencies * * @param \Migrations\SeedInterface[] $seeds Seeds + * @param array $visiting Seeds currently being visited (for cycle detection) + * @param array $visited Seeds that have been fully processed * @return \Migrations\SeedInterface[] + * @throws \RuntimeException When a circular dependency is detected */ - protected function orderSeedsByDependencies(array $seeds): array + protected function orderSeedsByDependencies(array $seeds, array $visiting = [], array &$visited = []): array { $orderedSeeds = []; foreach ($seeds as $seed) { $name = $seed->getName(); - $orderedSeeds[$name] = $seed; + + // Skip if already fully processed + if (isset($visited[$name])) { + continue; + } + + // Check for circular dependency + if (isset($visiting[$name])) { + $cycle = array_keys($visiting); + $cycle[] = $name; + throw new RuntimeException( + 'Circular dependency detected in seeds: ' . implode(' -> ', $cycle), + ); + } + + // Mark as currently visiting + $visiting[$name] = true; + $dependencies = $this->getSeedDependenciesInstances($seed); if ($dependencies) { - $orderedSeeds = array_merge($this->orderSeedsByDependencies($dependencies), $orderedSeeds); + $orderedSeeds = array_merge( + $this->orderSeedsByDependencies($dependencies, $visiting, $visited), + $orderedSeeds, + ); } + + // Mark as fully visited and add to result + $visited[$name] = true; + unset($visiting[$name]); + $orderedSeeds[$name] = $seed; } return $orderedSeeds; From a41992331f060e78dd99cf2d292e77b593e571b9 Mon Sep 17 00:00:00 2001 From: mscherer Date: Mon, 19 Jan 2026 09:13:07 +0100 Subject: [PATCH 2/5] Fix additional release readiness issues from deep dive - Restrict unserialize() to safe CakePHP schema classes only - Fix strpos() logic bug by using str_contains() - Validate --source option to prevent path traversal - Initialize $command property to prevent uninitialized access - Fix weak equality (!=) to strict (!==) in Table::saveData() - Fix copy-paste bug in Migrator (was using 'down' instead of 'missing') - Replace assert() with explicit RuntimeException in BaseSeed --- src/BaseSeed.php | 8 ++++++-- src/Command/BakeMigrationDiffCommand.php | 13 +++++++++++-- src/Command/BakeSimpleMigrationCommand.php | 10 +++++++++- src/Db/Table.php | 2 +- src/Migrations.php | 2 +- src/TestSuite/Migrator.php | 2 +- 6 files changed, 29 insertions(+), 8 deletions(-) diff --git a/src/BaseSeed.php b/src/BaseSeed.php index 28902213e..146abc4e1 100644 --- a/src/BaseSeed.php +++ b/src/BaseSeed.php @@ -238,7 +238,9 @@ public function isIdempotent(): bool public function call(string $seeder, array $options = []): void { $io = $this->getIo(); - assert($io !== null, 'Requires ConsoleIo'); + if ($io === null) { + throw new RuntimeException('ConsoleIo is required for calling other seeders.'); + } $io->out(''); $io->out( ' ====' . @@ -285,7 +287,9 @@ protected function runCall(string $seeder, array $options = []): void 'source' => $options['source'], ]); $io = $this->getIo(); - assert($io !== null, 'Missing ConsoleIo instance'); + if ($io === null) { + throw new RuntimeException('ConsoleIo is required for calling other seeders.'); + } $manager = $factory->createManager($io); $manager->seed($seeder); } diff --git a/src/Command/BakeMigrationDiffCommand.php b/src/Command/BakeMigrationDiffCommand.php index cb3ad01e3..4837b7927 100644 --- a/src/Command/BakeMigrationDiffCommand.php +++ b/src/Command/BakeMigrationDiffCommand.php @@ -19,6 +19,7 @@ use Cake\Console\ConsoleIo; use Cake\Console\ConsoleOptionParser; use Cake\Database\Connection; +use Cake\Database\Schema\CachedCollection; use Cake\Database\Schema\CollectionInterface; use Cake\Database\Schema\TableSchema; use Cake\Datasource\ConnectionManager; @@ -485,7 +486,7 @@ protected function checkSync(): bool $lastVersion = $this->migratedItems[0]['version']; $lastFile = end($this->migrationsFiles); - return $lastFile && (bool)strpos($lastFile, (string)$lastVersion); + return $lastFile && str_contains($lastFile, (string)$lastVersion); } return false; @@ -546,7 +547,15 @@ protected function getDumpSchema(Arguments $args): array $this->io->abort($msg); } - return unserialize((string)file_get_contents($path)); + $contents = (string)file_get_contents($path); + + // Use allowed_classes to restrict deserialization to safe CakePHP schema classes + return unserialize($contents, [ + 'allowed_classes' => [ + TableSchema::class, + CachedCollection::class, + ], + ]); } /** diff --git a/src/Command/BakeSimpleMigrationCommand.php b/src/Command/BakeSimpleMigrationCommand.php index 622b61fcc..0def3499a 100644 --- a/src/Command/BakeSimpleMigrationCommand.php +++ b/src/Command/BakeSimpleMigrationCommand.php @@ -25,6 +25,7 @@ use Cake\Utility\Inflector; use DateTime; use DateTimeZone; +use InvalidArgumentException; use Migrations\Util\Util; /** @@ -120,7 +121,14 @@ public function fileName($name): string */ public function getPath(Arguments $args): string { - $migrationFolder = $this->pathFragment . DS . $args->getOption('source') . DS; + $source = (string)$args->getOption('source'); + // Validate source option to prevent path traversal + if (preg_match('/\.\.[\\/\\\\]|^[\\/\\\\]/', $source)) { + throw new InvalidArgumentException( + 'The --source option cannot contain path traversal patterns or absolute paths.', + ); + } + $migrationFolder = $this->pathFragment . DS . $source . DS; $path = ROOT . DS . $migrationFolder; if ($this->plugin) { $path = $this->_pluginPath($this->plugin) . $migrationFolder; diff --git a/src/Db/Table.php b/src/Db/Table.php index 3998627d8..aeeb09066 100644 --- a/src/Db/Table.php +++ b/src/Db/Table.php @@ -955,7 +955,7 @@ public function saveData(): void $c = array_keys($row); foreach ($this->getData() as $row) { $k = array_keys($row); - if ($k != $c) { + if ($k !== $c) { $bulk = false; break; } diff --git a/src/Migrations.php b/src/Migrations.php index bba10eee7..b61bf07cf 100644 --- a/src/Migrations.php +++ b/src/Migrations.php @@ -36,7 +36,7 @@ class Migrations * * @var string */ - protected string $command; + protected string $command = ''; /** * Constructor diff --git a/src/TestSuite/Migrator.php b/src/TestSuite/Migrator.php index a33e95263..a0c30160b 100644 --- a/src/TestSuite/Migrator.php +++ b/src/TestSuite/Migrator.php @@ -199,7 +199,7 @@ protected function shouldDropTables(Migrations $migrations, array $options): boo if (!empty($messages['missing'])) { $hasProblems = true; $output[] = 'Applied but missing migrations:'; - $output = array_merge($output, array_map($itemize, $messages['down'])); + $output = array_merge($output, array_map($itemize, $messages['missing'])); $output[] = ''; } if ($output) { From 28162a06bda7119c0431b7eae57fe9a4b3059c2e Mon Sep 17 00:00:00 2001 From: mscherer Date: Mon, 19 Jan 2026 09:20:21 +0100 Subject: [PATCH 3/5] Add missing CakePHP schema classes to unserialize allowlist TableSchema contains nested Column, Index, Constraint and other schema objects that also need to be allowed for deserialization. --- src/Command/BakeMigrationDiffCommand.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/Command/BakeMigrationDiffCommand.php b/src/Command/BakeMigrationDiffCommand.php index 4837b7927..68bcd71bc 100644 --- a/src/Command/BakeMigrationDiffCommand.php +++ b/src/Command/BakeMigrationDiffCommand.php @@ -20,8 +20,14 @@ use Cake\Console\ConsoleOptionParser; use Cake\Database\Connection; use Cake\Database\Schema\CachedCollection; +use Cake\Database\Schema\CheckConstraint; use Cake\Database\Schema\CollectionInterface; +use Cake\Database\Schema\Column; +use Cake\Database\Schema\Constraint; +use Cake\Database\Schema\ForeignKey; +use Cake\Database\Schema\Index; use Cake\Database\Schema\TableSchema; +use Cake\Database\Schema\UniqueKey; use Cake\Datasource\ConnectionManager; use Cake\Event\Event; use Cake\Event\EventManager; @@ -554,6 +560,12 @@ protected function getDumpSchema(Arguments $args): array 'allowed_classes' => [ TableSchema::class, CachedCollection::class, + Column::class, + Index::class, + Constraint::class, + UniqueKey::class, + ForeignKey::class, + CheckConstraint::class, ], ]); } From 5cb24a33d8f986e1039f62733781d0c89a4adabe Mon Sep 17 00:00:00 2001 From: mscherer Date: Mon, 19 Jan 2026 09:27:12 +0100 Subject: [PATCH 4/5] Fix docblock annotation spacing in SqlserverAdapter --- src/Db/Adapter/SqlserverAdapter.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Db/Adapter/SqlserverAdapter.php b/src/Db/Adapter/SqlserverAdapter.php index e1f55a21e..f1f330dca 100644 --- a/src/Db/Adapter/SqlserverAdapter.php +++ b/src/Db/Adapter/SqlserverAdapter.php @@ -1117,7 +1117,6 @@ protected function getCheckConstraints(string $tableName): array /** * @inheritDoc - * * @throws \BadMethodCallException Check constraints are not supported for SQL Server. */ protected function getAddCheckConstraintInstructions(TableMetadata $table, CheckConstraint $checkConstraint): AlterInstructions @@ -1130,7 +1129,6 @@ protected function getAddCheckConstraintInstructions(TableMetadata $table, Check /** * @inheritDoc - * * @throws \BadMethodCallException Check constraints are not supported for SQL Server. */ protected function getDropCheckConstraintInstructions(string $tableName, string $constraintName): AlterInstructions From 600fdffdb2bb14d3e15c57c3f534b4964edc230b Mon Sep 17 00:00:00 2001 From: mscherer Date: Tue, 20 Jan 2026 05:18:05 +0100 Subject: [PATCH 5/5] Remove unnecessary path traversal validation --- src/Command/BakeSimpleMigrationCommand.php | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/src/Command/BakeSimpleMigrationCommand.php b/src/Command/BakeSimpleMigrationCommand.php index 0def3499a..622b61fcc 100644 --- a/src/Command/BakeSimpleMigrationCommand.php +++ b/src/Command/BakeSimpleMigrationCommand.php @@ -25,7 +25,6 @@ use Cake\Utility\Inflector; use DateTime; use DateTimeZone; -use InvalidArgumentException; use Migrations\Util\Util; /** @@ -121,14 +120,7 @@ public function fileName($name): string */ public function getPath(Arguments $args): string { - $source = (string)$args->getOption('source'); - // Validate source option to prevent path traversal - if (preg_match('/\.\.[\\/\\\\]|^[\\/\\\\]/', $source)) { - throw new InvalidArgumentException( - 'The --source option cannot contain path traversal patterns or absolute paths.', - ); - } - $migrationFolder = $this->pathFragment . DS . $source . DS; + $migrationFolder = $this->pathFragment . DS . $args->getOption('source') . DS; $path = ROOT . DS . $migrationFolder; if ($this->plugin) { $path = $this->_pluginPath($this->plugin) . $migrationFolder;