From 8b899eaaff463aa01238f08272988ec065ca6a5b Mon Sep 17 00:00:00 2001 From: snipe Date: Mon, 17 Aug 2026 22:30:50 +0100 Subject: [PATCH] Backup/Restore: - Fixed #19506 and #19506 --- .env.docker | 9 +- .env.example | 3 + app/Console/Commands/RestoreFromBackup.php | 80 ++++++++++---- config/database.php | 46 +++++++- .../RestoreFromBackupBinaryPickerTest.php | 102 ++++++++++++++++++ 5 files changed, 216 insertions(+), 24 deletions(-) create mode 100644 tests/Feature/Console/RestoreFromBackupBinaryPickerTest.php diff --git a/.env.docker b/.env.docker index 6a7bc2b3cb..c6d9dbee3f 100644 --- a/.env.docker +++ b/.env.docker @@ -26,7 +26,9 @@ PUBLIC_FILESYSTEM_DISK=local_public # -------------------------------------------- # REQUIRED: DATABASE SETTINGS # -------------------------------------------- -DB_CONNECTION=mysql +# The docker-compose db service uses MariaDB, so we route through the mariadb +# driver so spatie's backup uses mariadb-dump + --skip-ssl. +DB_CONNECTION=mariadb DB_HOST=db DB_SOCKET=null DB_PORT='3306' @@ -36,7 +38,10 @@ DB_PASSWORD=changeme1234 MYSQL_ROOT_PASSWORD=changeme1234 DB_PREFIX=null DB_DUMP_PATH='/usr/bin' -DB_DUMP_SKIP_SSL=true +# Left off by default: the composed mariadb service has no TLS configured, +# so mariadb-dump connects unencrypted regardless. Opt in only if your +# server actually requires an SSL-skip flag. +DB_DUMP_SKIP_SSL=false DB_DUMP_SINGLE_TRANSACTION=false DB_CHARSET=utf8mb4 DB_COLLATION=utf8mb4_unicode_ci diff --git a/.env.example b/.env.example index 0083b36c79..a2e972879a 100644 --- a/.env.example +++ b/.env.example @@ -22,6 +22,9 @@ PUBLIC_FILESYSTEM_DISK=local_public # -------------------------------------------- # REQUIRED: DATABASE SETTINGS # -------------------------------------------- +# Set DB_CONNECTION=mariadb if your server is MariaDB. This makes the +# backup/restore tooling invoke mariadb / mariadb-dump instead of the +# deprecated mysql / mysqldump symlinks. DB_CONNECTION=mysql DB_HOST=127.0.0.1 DB_SOCKET=null diff --git a/app/Console/Commands/RestoreFromBackup.php b/app/Console/Commands/RestoreFromBackup.php index aad9a3d404..5049ebabfc 100644 --- a/app/Console/Commands/RestoreFromBackup.php +++ b/app/Console/Commands/RestoreFromBackup.php @@ -202,6 +202,28 @@ class RestoreFromBackup extends Command parent::__construct(); } + /** + * Pick the DB client binary to invoke for a given driver. Prefers the + * driver-native name (mariadb for the mariadb driver, mysql otherwise), + * falls back to the other name if the preferred binary is missing. + * Returns null if neither exists in the given path. + */ + public static function pickDbClientBinary(string $driver, string $binaryPath): ?string + { + $ext = \DIRECTORY_SEPARATOR === '\\' ? '.exe' : ''; + $preferred = $driver === 'mariadb' ? 'mariadb' : 'mysql'; + $fallback = $preferred === 'mariadb' ? 'mysql' : 'mariadb'; + + foreach ([$preferred, $fallback] as $name) { + $candidate = rtrim($binaryPath, \DIRECTORY_SEPARATOR) . \DIRECTORY_SEPARATOR . $name . $ext; + if (file_exists($candidate)) { + return $candidate; + } + } + + return null; + } + /** * Execute the console command. * @@ -225,8 +247,12 @@ class RestoreFromBackup extends Command return $this->error('Data loss not confirmed'); } - if (config('database.default') != 'mysql') { - return $this->error('DB_CONNECTION must be MySQL in order to perform a restore. Detected: '.config('database.default')); + $connectionName = config('database.default'); + $connectionConfig = config("database.connections.$connectionName"); + $driver = $connectionConfig['driver'] ?? null; + + if (!in_array($driver, ['mysql', 'mariadb'], true)) { + return $this->error('DB_CONNECTION must be MySQL or MariaDB in order to perform a restore. Detected driver: ' . ($driver ?? 'unknown') . " (connection: $connectionName)"); } $za = new ZipArchive; @@ -430,7 +456,8 @@ class RestoreFromBackup extends Command $sql_contents = $za->getStream($sql_stat['name']); // maybe copy *THIS* thing? if ($sql_contents === false) { - $this->error("Unable to open SQL file: " . $sql_stat['name']); + $this->error('Unable to open SQL file: ' . $sql_stat['name']); + return -1; } @@ -456,26 +483,29 @@ class RestoreFromBackup extends Command $pipes = []; $env_vars = getenv(); - $env_vars['MYSQL_PWD'] = config('database.connections.mysql.password'); - // TODO notes: we are stealing the dump_binary_path (which *probably* also has your copy of the mysql binary in it. But it might not, so we might need to extend this) - // we unilaterally prepend a slash to the `mysql` command. This might mean your path could look like /blah/blah/blah//mysql - which should be fine. But maybe in some environments it isn't? - $mysql_binary = config('database.connections.mysql.dump.dump_binary_path').\DIRECTORY_SEPARATOR.'mysql'.(\DIRECTORY_SEPARATOR == '\\' ? '.exe' : ''); - if (! file_exists($mysql_binary)) { - return $this->error("mysql tool at: '$mysql_binary' does not exist, cannot restore. Please edit DB_DUMP_PATH in your .env to point to a directory that contains the mysqldump and mysql binary"); + $env_vars['MYSQL_PWD'] = $connectionConfig['password']; + + $binaryPath = $connectionConfig['dump']['dump_binary_path'] ?? ''; + $client_binary = static::pickDbClientBinary($driver, $binaryPath); + if ($client_binary === null) { + $preferred = $driver === 'mariadb' ? 'mariadb' : 'mysql'; + + return $this->error("DB client binary '$preferred' not found in DB_DUMP_PATH ('$binaryPath'). Please edit DB_DUMP_PATH in your .env to point to a directory that contains the mysql/mariadb client binary."); } - $proc_results = proc_open("$mysql_binary -h " . - escapeshellarg(config('database.connections.mysql.host')) . + + $proc_results = proc_open(escapeshellarg($client_binary) . ' -h ' . + escapeshellarg($connectionConfig['host']) . ' --batch ' . ' --binary-mode ' . - ' -u ' . escapeshellarg(config('database.connections.mysql.username')) . ' ' . - ' -P ' . escapeshellarg(config('database.connections.mysql.port')) . ' ' . - escapeshellarg(config('database.connections.mysql.database')), // yanked -p since we pass via ENV + ' -u ' . escapeshellarg($connectionConfig['username']) . ' ' . + ' -P ' . escapeshellarg($connectionConfig['port']) . ' ' . + escapeshellarg($connectionConfig['database']), // yanked -p since we pass via ENV [0 => ['pipe', 'r'], 1 => ['pipe', 'w'], 2 => ['pipe', 'w']], $pipes, null, $env_vars); // this is not super-duper awesome-secure, but definitely more secure than showing it on the CLI, or dropping temporary files with passwords in them. if ($proc_results === false) { - return $this->error('Unable to invoke mysql via CLI'); + return $this->error('Unable to invoke DB client via CLI'); } try { @@ -497,17 +527,31 @@ class RestoreFromBackup extends Command } } catch (\Exception $e) { Log::error('Error during restore!!!! '.$e->getMessage()); - // FIXME - put these back and/or put them in the right places?! - $err_out = fgets($pipes[1]); - $err_err = fgets($pipes[2]); + // Drain both pipes fully so the DB client's own diagnostic ends up in + // the log instead of just the downstream "broken pipe". Then include + // the process exit code so root-cause is visible without strace. + // The deprecation warning about maria-db is a red herring, and not the actual problem. + // "Deprecated program name. It will be removed in a future release, use '/usr/bin/mariadb' instead" + // was drowning out the actual problem. + stream_set_blocking($pipes[1], false); + stream_set_blocking($pipes[2], false); + $err_out = stream_get_contents($pipes[1]) ?: ''; + $err_err = stream_get_contents($pipes[2]) ?: ''; + fclose($pipes[0]); + fclose($pipes[1]); + fclose($pipes[2]); + $exit_code = proc_close($proc_results); Log::error('Error OUTPUT: '.$err_out); $this->info($err_out); Log::error('Error ERROR : '.$err_err); $this->error($err_err); + Log::error("DB client exited with code $exit_code"); + $this->error("DB client exited with code $exit_code"); throw $e; } if (! feof($sql_contents) || $bytes_read == 0) { $this->error('Not at end of file for sql file, or zero bytes read. aborting!'); + return -1; } diff --git a/config/database.php b/config/database.php index 749571e4e5..f773e6a04b 100755 --- a/config/database.php +++ b/config/database.php @@ -21,10 +21,16 @@ $dump_options = [ // that require --single-transaction (e.g. no LOCK TABLES privilege). $dump_options['use_single_transaction'] = (env('DB_DUMP_SINGLE_TRANSACTION', 'false') === 'true'); -// For modern versions of mysqldump, use --ssl-mode=DISABLED -if (env('DB_DUMP_SKIP_SSL') == 'true') { - // Correctly add the option as a string to the 'add_extra_option' key. - $dump_options['add_extra_option'] = '--ssl-mode=DISABLED'; +// Opt-in flag for skipping SSL on the dump connection. Only add the key when +// it's true - spatie's processExtraDumpParameters has a bug where a `false` +// value routes through callMethodOnDumper's `if (!$methodValue)` branch, which +// calls setSkipSsl() with no args, and that setter defaults to true. So an +// explicit false would silently become true. When opted in, spatie emits the +// correct per-DBMS flag: --ssl-mode=DISABLED for MySQL 8.4+, --skip-ssl for +// older MySQL and MariaDB. Do NOT set this via add_extra_option; that route +// ships the raw flag as-is and mariadb-dump rejects ssl-mode. +if (env('DB_DUMP_SKIP_SSL') === 'true') { + $dump_options['skip_ssl'] = true; } return [ @@ -113,6 +119,38 @@ return [ ]) : [], ], + // Use this connection (DB_CONNECTION=mariadb) when the server is + // MariaDB. Spatie's backup picks the MariaDb dumper for this driver, + // which invokes mariadb-dump instead of the deprecated mysqldump + // symlink and uses --skip-ssl instead of --ssl-mode=DISABLED. + 'mariadb' => [ + 'driver' => 'mariadb', + 'host' => env('DB_HOST', 'localhost'), + 'port' => (int) env('DB_PORT', 3306), + 'database' => env('DB_DATABASE', 'forge'), + 'username' => env('DB_USERNAME', 'forge'), + 'password' => env('DB_PASSWORD', ''), + 'charset' => env('DB_CHARSET', 'utf8mb4'), + 'collation' => env('DB_COLLATION', 'utf8mb4_unicode_ci'), + 'prefix' => env('DB_PREFIX', null), + 'strict' => false, + 'engine' => 'InnoDB', + 'unix_socket' => env('DB_SOCKET', ''), + 'dump' => $dump_options, + 'dump_command_timeout' => 60 * 5, + 'dump_using_single_transaction' => true, + 'options' => (env('DB_SSL')) ? ((env('DB_SSL_IS_PAAS')) ? [ + PDO::MYSQL_ATTR_SSL_CA => env('DB_SSL_CA_PATH'), + PDO::MYSQL_ATTR_SSL_VERIFY_SERVER_CERT => env('DB_SSL_VERIFY_SERVER', false), + ] : [ + PDO::MYSQL_ATTR_SSL_KEY => env('DB_SSL_KEY_PATH'), + PDO::MYSQL_ATTR_SSL_CERT => env('DB_SSL_CERT_PATH'), + PDO::MYSQL_ATTR_SSL_CA => env('DB_SSL_CA_PATH'), + PDO::MYSQL_ATTR_SSL_CIPHER => env('DB_SSL_CIPHER'), + PDO::MYSQL_ATTR_SSL_VERIFY_SERVER_CERT => env('DB_SSL_VERIFY_SERVER', false), + ]) : [], + ], + 'pgsql' => [ 'driver' => 'pgsql', 'host' => env('DB_HOST', 'localhost'), diff --git a/tests/Feature/Console/RestoreFromBackupBinaryPickerTest.php b/tests/Feature/Console/RestoreFromBackupBinaryPickerTest.php new file mode 100644 index 0000000000..8ec9525c94 --- /dev/null +++ b/tests/Feature/Console/RestoreFromBackupBinaryPickerTest.php @@ -0,0 +1,102 @@ +tmpDir = sys_get_temp_dir().'/snipeit-restore-picker-'.uniqid(); + mkdir($this->tmpDir, 0755, true); + } + + protected function tearDown(): void + { + array_map('unlink', glob($this->tmpDir.'/*') ?: []); + rmdir($this->tmpDir); + parent::tearDown(); + } + + private function makeBinary(string $name): void + { + touch($this->tmpDir.\DIRECTORY_SEPARATOR.$name.(\DIRECTORY_SEPARATOR === '\\' ? '.exe' : '')); + } + + public function test_mariadb_driver_prefers_mariadb_binary(): void + { + $this->makeBinary('mysql'); + $this->makeBinary('mariadb'); + + $picked = RestoreFromBackup::pickDbClientBinary('mariadb', $this->tmpDir); + + $this->assertSame($this->tmpDir.\DIRECTORY_SEPARATOR.'mariadb'.(\DIRECTORY_SEPARATOR === '\\' ? '.exe' : ''), $picked); + } + + public function test_mysql_driver_prefers_mysql_binary(): void + { + $this->makeBinary('mysql'); + $this->makeBinary('mariadb'); + + $picked = RestoreFromBackup::pickDbClientBinary('mysql', $this->tmpDir); + + $this->assertSame($this->tmpDir.\DIRECTORY_SEPARATOR.'mysql'.(\DIRECTORY_SEPARATOR === '\\' ? '.exe' : ''), $picked); + } + + public function test_mariadb_driver_falls_back_to_mysql_binary_when_mariadb_missing(): void + { + $this->makeBinary('mysql'); + + $picked = RestoreFromBackup::pickDbClientBinary('mariadb', $this->tmpDir); + + $this->assertSame($this->tmpDir.\DIRECTORY_SEPARATOR.'mysql'.(\DIRECTORY_SEPARATOR === '\\' ? '.exe' : ''), $picked); + } + + public function test_mysql_driver_falls_back_to_mariadb_binary_when_mysql_missing(): void + { + $this->makeBinary('mariadb'); + + $picked = RestoreFromBackup::pickDbClientBinary('mysql', $this->tmpDir); + + $this->assertSame($this->tmpDir.\DIRECTORY_SEPARATOR.'mariadb'.(\DIRECTORY_SEPARATOR === '\\' ? '.exe' : ''), $picked); + } + + public function test_returns_null_when_no_binary_exists(): void + { + $picked = RestoreFromBackup::pickDbClientBinary('mysql', $this->tmpDir); + + $this->assertNull($picked); + } + + public function test_trailing_separator_on_path_does_not_double(): void + { + $this->makeBinary('mariadb'); + + $picked = RestoreFromBackup::pickDbClientBinary('mariadb', $this->tmpDir.\DIRECTORY_SEPARATOR); + + $this->assertSame($this->tmpDir.\DIRECTORY_SEPARATOR.'mariadb'.(\DIRECTORY_SEPARATOR === '\\' ? '.exe' : ''), $picked); + } + + public function test_config_exposes_mariadb_connection_with_correct_driver(): void + { + $this->assertSame('mariadb', config('database.connections.mariadb.driver')); + } + + public function test_skip_ssl_is_absent_from_dump_config_by_default(): void + { + // Regression: spatie's processExtraDumpParameters iterates the dump + // config and treats a `false` value as "call the setter with no args", + // which for setSkipSsl(bool $x = true) silently sets skipSsl to TRUE. + // The key must be absent from the array (not set to false) when opted + // out, matching the old add_extra_option gating shape. + $this->assertArrayNotHasKey('skip_ssl', config('database.connections.mysql.dump')); + $this->assertArrayNotHasKey('skip_ssl', config('database.connections.mariadb.dump')); + $this->assertArrayNotHasKey('add_extra_option', config('database.connections.mysql.dump')); + $this->assertArrayNotHasKey('add_extra_option', config('database.connections.mariadb.dump')); + } +}