From ea117b6b3f2a988eadb5c8638c49e61acfdaac09 Mon Sep 17 00:00:00 2001 From: Riddhesh Sanghvi Date: Mon, 29 Jun 2026 22:02:50 +0530 Subject: [PATCH 1/9] fix(backup): harden backup integrity against silent data loss Three related fixes in Site_Backup_Restore.php: - backup_db(): the mysqldump shell redirect (`> file`) creates/truncates the target before mysqldump runs, so a failed dump left a 0-byte file that passed exists() and was uploaded as a "successful" backup. Capture the dump's exit code (via `ee shell` through EE::launch) and assert filesize() > 0 before treating the dump as valid. - Add a `7z t` integrity test (verify_archive_integrity()) on the primary site/wp-content archive after creation and before rclone_upload(), so a corrupt archive aborts instead of overwriting a good remote backup. - Escape DB user/password/host/name with escapeshellarg() in the mysqldump command, matching restore_db()/get_db_size(). --- src/helper/Site_Backup_Restore.php | 47 +++++++++++++++++++++++++----- 1 file changed, 40 insertions(+), 7 deletions(-) diff --git a/src/helper/Site_Backup_Restore.php b/src/helper/Site_Backup_Restore.php index a287b639..ef41ba83 100644 --- a/src/helper/Site_Backup_Restore.php +++ b/src/helper/Site_Backup_Restore.php @@ -488,9 +488,32 @@ private function backup_site_dir( $backup_dir ) { EE::error( 'Failed to create backup archive. Please check disk space and file permissions.' ); } + $this->verify_archive_integrity( $backup_file ); + return $backup_file; } + /** + * Run `7z t` on a freshly-created backup archive and abort if it is corrupt. + * + * Catches silently-truncated/corrupt archives before they are uploaded, so a + * broken backup never replaces a good one in remote storage. + * + * @param string $archive Absolute path to the archive to test. + */ + private function verify_archive_integrity( $archive ) { + if ( EE::exec( sprintf( '7z t %s', escapeshellarg( $archive ) ) ) ) { + return; + } + + $this->capture_error( + sprintf( 'Backup archive failed integrity check: %s', $archive ), + self::ERROR_TYPE_FILESYSTEM, + 3003 + ); + EE::error( 'Backup archive failed integrity verification. Aborting before upload to avoid overwriting a good backup.' ); + } + private function backup_wp_content_dir( $backup_dir ) { EE::log( 'Backing up site files.' ); EE::log( 'This may take some time.' ); @@ -572,6 +595,8 @@ private function backup_wp_content_dir( $backup_dir ) { EE::error( 'Failed to create backup archive. Please check disk space and file permissions.' ); } + $this->verify_archive_integrity( $backup_file ); + return $backup_file; } @@ -655,17 +680,25 @@ private function backup_db( $backup_dir ) { $this->fs->mkdir( $backup_dir . '/sql' ); - $backup_command = sprintf( 'mysqldump --skip-ssl -u %s -p%s -h %s --single-transaction %s > /var/www/htdocs/%s', $db_user, $db_password, $db_host, $db_name, $sql_filename ); - $args = [ 'shell', $this->site_data['site_url'] ]; - $assoc_args = [ 'command' => $backup_command ]; - $options = [ 'skip-tty' => true ]; + // Escape DB credentials: command runs through a container shell (matches restore_db()/get_db_size()). + $backup_command = sprintf( + 'mysqldump --skip-ssl -u %s -p%s -h %s --single-transaction %s > /var/www/htdocs/%s', + escapeshellarg( $db_user ), + escapeshellarg( $db_password ), + escapeshellarg( $db_host ), + escapeshellarg( $db_name ), + $sql_filename + ); - EE::run_command( $args, $assoc_args, $options ); + // Launch via `ee shell` so the dump's exit code is captured. The shell `>` redirect + // creates/truncates the target before mysqldump runs, so a failed dump leaves a 0-byte + // file that passes exists(); rely on the exit code + filesize instead. + $dump_result = EE::launch( sprintf( 'ee shell %s --skip-tty --command=%s', escapeshellarg( $this->site_data['site_url'] ), escapeshellarg( $backup_command ) ) ); $sql_dump_path = EE_ROOT_DIR . '/sites/' . $this->site_data['site_url'] . '/app/htdocs/' . $sql_filename; - // Check if database dump was created successfully - if ( ! $this->fs->exists( $sql_dump_path ) ) { + // A 0-byte or missing dump, or a non-zero exit, means the backup failed. + if ( 0 !== $dump_result->return_code || ! $this->fs->exists( $sql_dump_path ) || filesize( $sql_dump_path ) <= 0 ) { $this->capture_error( sprintf( 'Database backup failed for database: %s', $db_name ), self::ERROR_TYPE_DATABASE, From e93a6c8f0adddab5ba097d3552f141a5849cab2f Mon Sep 17 00:00:00 2001 From: Riddhesh Sanghvi Date: Mon, 29 Jun 2026 22:30:11 +0530 Subject: [PATCH 2/9] fix(backup): close DB-less backup gap and widen archive integrity checks Follow-up hardening from review of the backup-integrity changes: - backup_db(): the dump-staging `mv` ran with its return value ignored. A failed mv (cross-device, permissions, disk-full, etc.) left sql/ empty; `7z u` on an empty dir exits 0 and the new `7z t` check passes, so a backup with NO database shipped as "successful". Now check the mv return and assert the destination exists and is non-empty, failing loudly otherwise. Also escapeshellarg the mv arguments. - verify_archive_integrity(): use EE::launch with `return_code < 2` instead of EE::exec, so a non-fatal 7z warning (exit 1) no longer aborts the whole backup. Matches every other 7z call in this file. - Extend integrity verification to the config archives: backup_nginx_conf() and backup_php_conf() now verify conf.zip, and the optional custom docker-compose archive is integrity-tested too (warn-and-exclude, since that archive is optional) so no archive ships to remote storage unverified. - Soften the mysqldump credential-escaping comment: it is best-effort layer-1 quoting; the inner `ee shell` bash -c wrapper still cannot carry arbitrary shell metacharacters. --- src/helper/Site_Backup_Restore.php | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/src/helper/Site_Backup_Restore.php b/src/helper/Site_Backup_Restore.php index ef41ba83..20333080 100644 --- a/src/helper/Site_Backup_Restore.php +++ b/src/helper/Site_Backup_Restore.php @@ -463,6 +463,10 @@ private function maybe_backup_custom_docker_compose( $backup_dir ) { // This is optional, so we just log a warning instead of failing if ( $result->return_code >= 2 ) { EE::warning( 'Failed to backup custom docker-compose directory. Continuing with backup.' ); + } elseif ( EE::launch( sprintf( '7z t %s', escapeshellarg( $custom_docker_compose_dir_archive ) ) )->return_code >= 2 ) { + // Optional archive: warn (and drop the corrupt zip) instead of aborting the whole backup. + EE::warning( 'Custom docker-compose archive failed integrity check. Excluding it from the backup.' ); + $this->fs->remove( $custom_docker_compose_dir_archive ); } } } @@ -502,7 +506,8 @@ private function backup_site_dir( $backup_dir ) { * @param string $archive Absolute path to the archive to test. */ private function verify_archive_integrity( $archive ) { - if ( EE::exec( sprintf( '7z t %s', escapeshellarg( $archive ) ) ) ) { + // 7z exit codes: 0=success, 1=warning (non-fatal), 2+=fatal error. + if ( EE::launch( sprintf( '7z t %s', escapeshellarg( $archive ) ) )->return_code < 2 ) { return; } @@ -618,6 +623,8 @@ private function backup_nginx_conf( $backup_dir ) { ); EE::error( 'Failed to create nginx configuration backup archive. Please check disk space and file permissions.' ); } + + $this->verify_archive_integrity( $backup_file ); } private function backup_php_conf( $backup_dir ) { @@ -638,6 +645,8 @@ private function backup_php_conf( $backup_dir ) { ); EE::error( 'Failed to create PHP configuration backup archive. Please check disk space and file permissions.' ); } + + $this->verify_archive_integrity( $backup_file ); } private function backup_html( $backup_dir ) { @@ -680,7 +689,10 @@ private function backup_db( $backup_dir ) { $this->fs->mkdir( $backup_dir . '/sql' ); - // Escape DB credentials: command runs through a container shell (matches restore_db()/get_db_size()). + // Best-effort layer-1 quoting of DB credentials (consistent with restore_db()/get_db_size()). + // NOTE: the value still passes through a second double-quoted `bash -c "$command"` layer + // inside `ee shell` that escapeshellarg cannot protect, so a password containing ` " or $ + // can still break the dump. Fully hardening that inner wrapper is out of scope here. $backup_command = sprintf( 'mysqldump --skip-ssl -u %s -p%s -h %s --single-transaction %s > /var/www/htdocs/%s', escapeshellarg( $db_user ), @@ -707,7 +719,19 @@ private function backup_db( $backup_dir ) { EE::error( 'Database backup failed. Please check database credentials and connectivity.' ); } - EE::exec( sprintf( 'mv %s %s', $sql_dump_path, $sql_file ) ); + // A failed mv (cross-device, permissions, disk-full, etc.) would leave sql/ empty; + // `7z u` on an empty dir exits 0 and `7z t` passes, shipping a DB-less "successful" + // backup. Fail loudly unless the dump actually landed and is non-empty. + if ( ! EE::exec( sprintf( 'mv %s %s', escapeshellarg( $sql_dump_path ), escapeshellarg( $sql_file ) ) ) + || ! $this->fs->exists( $sql_file ) || filesize( $sql_file ) <= 0 ) { + $this->capture_error( + sprintf( 'Failed to stage database dump for database: %s', $db_name ), + self::ERROR_TYPE_DATABASE, + 4003 + ); + EE::error( 'Database backup failed while staging the dump file.' ); + } + $backup_command = sprintf( 'cd %s && 7z u -mx=1 %s sql', $backup_dir, $backup_file ); $result = EE::launch( $backup_command ); From ac60000122f9717d50a96f0cc8b3c38958365cf3 Mon Sep 17 00:00:00 2001 From: Riddhesh Sanghvi Date: Mon, 29 Jun 2026 23:25:54 +0530 Subject: [PATCH 3/9] docs(backup): correct DB-credential quoting comment The comment claimed consistency with restore_db(), which uses manual single-quoting rather than escapeshellarg(); reference get_db_size() (which does) so the note isn't misleading. --- src/helper/Site_Backup_Restore.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/helper/Site_Backup_Restore.php b/src/helper/Site_Backup_Restore.php index 20333080..3d387d46 100644 --- a/src/helper/Site_Backup_Restore.php +++ b/src/helper/Site_Backup_Restore.php @@ -689,7 +689,7 @@ private function backup_db( $backup_dir ) { $this->fs->mkdir( $backup_dir . '/sql' ); - // Best-effort layer-1 quoting of DB credentials (consistent with restore_db()/get_db_size()). + // Best-effort layer-1 quoting of DB credentials (consistent with get_db_size()). // NOTE: the value still passes through a second double-quoted `bash -c "$command"` layer // inside `ee shell` that escapeshellarg cannot protect, so a password containing ` " or $ // can still break the dump. Fully hardening that inner wrapper is out of scope here. From bfbcdaee897f0f211398e2b10fd9c2a70034ebf1 Mon Sep 17 00:00:00 2001 From: Riddhesh Sanghvi Date: Thu, 24 Sep 2026 10:43:47 +0000 Subject: [PATCH 4/9] fix(backup): show mysqldump stderr when the database dump fails Running the dump through EE::launch captures its stderr, so the actual mysqldump error (access denied, disk full, lost connection) was no longer shown; only the generic failure message was. Print it as a warning on failure. --- src/helper/Site_Backup_Restore.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/helper/Site_Backup_Restore.php b/src/helper/Site_Backup_Restore.php index 3d387d46..5794e94b 100644 --- a/src/helper/Site_Backup_Restore.php +++ b/src/helper/Site_Backup_Restore.php @@ -711,6 +711,10 @@ private function backup_db( $backup_dir ) { // A 0-byte or missing dump, or a non-zero exit, means the backup failed. if ( 0 !== $dump_result->return_code || ! $this->fs->exists( $sql_dump_path ) || filesize( $sql_dump_path ) <= 0 ) { + // EE::launch captures the dump's stderr, so show it or the cause is lost. + if ( '' !== trim( $dump_result->stderr ) ) { + EE::warning( trim( $dump_result->stderr ) ); + } $this->capture_error( sprintf( 'Database backup failed for database: %s', $db_name ), self::ERROR_TYPE_DATABASE, From babb280622978ce0a2176bbf45b91ad209d88f2f Mon Sep 17 00:00:00 2001 From: Riddhesh Sanghvi Date: Thu, 24 Sep 2026 10:43:53 +0000 Subject: [PATCH 5/9] fix(backup): drop a failed custom docker-compose archive from the backup When 7z exited with a fatal code, a partial user-docker-compose.zip could stay in the backup directory and be uploaded unverified. Remove it, as the integrity-check branch already does. --- src/helper/Site_Backup_Restore.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/helper/Site_Backup_Restore.php b/src/helper/Site_Backup_Restore.php index 5794e94b..7be795ee 100644 --- a/src/helper/Site_Backup_Restore.php +++ b/src/helper/Site_Backup_Restore.php @@ -463,6 +463,7 @@ private function maybe_backup_custom_docker_compose( $backup_dir ) { // This is optional, so we just log a warning instead of failing if ( $result->return_code >= 2 ) { EE::warning( 'Failed to backup custom docker-compose directory. Continuing with backup.' ); + $this->fs->remove( $custom_docker_compose_dir_archive ); } elseif ( EE::launch( sprintf( '7z t %s', escapeshellarg( $custom_docker_compose_dir_archive ) ) )->return_code >= 2 ) { // Optional archive: warn (and drop the corrupt zip) instead of aborting the whole backup. EE::warning( 'Custom docker-compose archive failed integrity check. Excluding it from the backup.' ); From 716eb26941afd5e79dac0e435f7b38635987ca4e Mon Sep 17 00:00:00 2001 From: Riddhesh Sanghvi Date: Thu, 24 Sep 2026 10:44:10 +0000 Subject: [PATCH 6/9] docs(backup): tighten database dump comments --- src/helper/Site_Backup_Restore.php | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/src/helper/Site_Backup_Restore.php b/src/helper/Site_Backup_Restore.php index 7be795ee..3ddffae2 100644 --- a/src/helper/Site_Backup_Restore.php +++ b/src/helper/Site_Backup_Restore.php @@ -690,10 +690,7 @@ private function backup_db( $backup_dir ) { $this->fs->mkdir( $backup_dir . '/sql' ); - // Best-effort layer-1 quoting of DB credentials (consistent with get_db_size()). - // NOTE: the value still passes through a second double-quoted `bash -c "$command"` layer - // inside `ee shell` that escapeshellarg cannot protect, so a password containing ` " or $ - // can still break the dump. Fully hardening that inner wrapper is out of scope here. + // ee shell re-wraps this in bash -c "...", so a password containing ` " or $ can still break the dump. $backup_command = sprintf( 'mysqldump --skip-ssl -u %s -p%s -h %s --single-transaction %s > /var/www/htdocs/%s', escapeshellarg( $db_user ), @@ -703,14 +700,11 @@ private function backup_db( $backup_dir ) { $sql_filename ); - // Launch via `ee shell` so the dump's exit code is captured. The shell `>` redirect - // creates/truncates the target before mysqldump runs, so a failed dump leaves a 0-byte - // file that passes exists(); rely on the exit code + filesize instead. + // Launched to get the exit code: the `>` redirect leaves a 0-byte file even when mysqldump fails. $dump_result = EE::launch( sprintf( 'ee shell %s --skip-tty --command=%s', escapeshellarg( $this->site_data['site_url'] ), escapeshellarg( $backup_command ) ) ); $sql_dump_path = EE_ROOT_DIR . '/sites/' . $this->site_data['site_url'] . '/app/htdocs/' . $sql_filename; - // A 0-byte or missing dump, or a non-zero exit, means the backup failed. if ( 0 !== $dump_result->return_code || ! $this->fs->exists( $sql_dump_path ) || filesize( $sql_dump_path ) <= 0 ) { // EE::launch captures the dump's stderr, so show it or the cause is lost. if ( '' !== trim( $dump_result->stderr ) ) { @@ -724,9 +718,7 @@ private function backup_db( $backup_dir ) { EE::error( 'Database backup failed. Please check database credentials and connectivity.' ); } - // A failed mv (cross-device, permissions, disk-full, etc.) would leave sql/ empty; - // `7z u` on an empty dir exits 0 and `7z t` passes, shipping a DB-less "successful" - // backup. Fail loudly unless the dump actually landed and is non-empty. + // If mv fails, `7z u`/`7z t` still pass on the empty sql/ dir and a DB-less backup would ship. if ( ! EE::exec( sprintf( 'mv %s %s', escapeshellarg( $sql_dump_path ), escapeshellarg( $sql_file ) ) ) || ! $this->fs->exists( $sql_file ) || filesize( $sql_file ) <= 0 ) { $this->capture_error( From 9498ae126725d8b628debe8958d6457194a548b7 Mon Sep 17 00:00:00 2001 From: Riddhesh Sanghvi Date: Thu, 24 Sep 2026 16:37:09 +0000 Subject: [PATCH 7/9] fix(backup): run the database dump through the running ee binary The dump's child `ee shell` was resolved through PATH, so a backup run where `ee` isn't on PATH (or where PATH points at a different EE version, e.g. `bin/ee` from source spawning the stable phar) failed with a misleading database error. Launch the running phar, or `php/boot-fs.php` for a source checkout, with the current PHP binary instead. --- src/helper/Site_Backup_Restore.php | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/src/helper/Site_Backup_Restore.php b/src/helper/Site_Backup_Restore.php index 3ddffae2..988bdc6f 100644 --- a/src/helper/Site_Backup_Restore.php +++ b/src/helper/Site_Backup_Restore.php @@ -330,6 +330,28 @@ private function run_wp_cli_command( $command, $skip_plugins_themes = false ) { return empty( $clean_output ) ? '-' : $clean_output; } + /** + * Build a child `ee shell` command that runs the current EE binary, not whatever `ee` is on PATH. + * + * @param string $command Command to run inside the site's php container. + * + * @return string + */ + private function get_ee_shell_command( $command ) { + $ee_script = class_exists( 'Phar' ) ? \Phar::running( false ) : ''; + if ( '' === $ee_script ) { + $ee_script = EE_ROOT . '/php/boot-fs.php'; + } + + return sprintf( + '%s %s shell %s --skip-tty --command=%s', + escapeshellarg( \EE\Utils\get_php_binary() ), + escapeshellarg( $ee_script ), + escapeshellarg( $this->site_data['site_url'] ), + escapeshellarg( $command ) + ); + } + private function backup_site_details( $backup_dir ) { $backup_data = []; @@ -701,7 +723,7 @@ private function backup_db( $backup_dir ) { ); // Launched to get the exit code: the `>` redirect leaves a 0-byte file even when mysqldump fails. - $dump_result = EE::launch( sprintf( 'ee shell %s --skip-tty --command=%s', escapeshellarg( $this->site_data['site_url'] ), escapeshellarg( $backup_command ) ) ); + $dump_result = EE::launch( $this->get_ee_shell_command( $backup_command ) ); $sql_dump_path = EE_ROOT_DIR . '/sites/' . $this->site_data['site_url'] . '/app/htdocs/' . $sql_filename; From d1fb8408f5f2736692fc15f8f2f2fe1191ff9db8 Mon Sep 17 00:00:00 2001 From: Riddhesh Sanghvi Date: Thu, 24 Sep 2026 16:37:09 +0000 Subject: [PATCH 8/9] fix(backup): renumber integrity and dump staging error codes 3003 and 4003 are already taken by #482 (site app directory not found) and #481 (orphaned remote backup delete failure). Use the next free codes: 3005 for the archive integrity failure and 4005 for the dump staging failure. --- src/helper/Site_Backup_Restore.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/helper/Site_Backup_Restore.php b/src/helper/Site_Backup_Restore.php index 988bdc6f..c8f0b6a1 100644 --- a/src/helper/Site_Backup_Restore.php +++ b/src/helper/Site_Backup_Restore.php @@ -537,7 +537,7 @@ private function verify_archive_integrity( $archive ) { $this->capture_error( sprintf( 'Backup archive failed integrity check: %s', $archive ), self::ERROR_TYPE_FILESYSTEM, - 3003 + 3005 ); EE::error( 'Backup archive failed integrity verification. Aborting before upload to avoid overwriting a good backup.' ); } @@ -746,7 +746,7 @@ private function backup_db( $backup_dir ) { $this->capture_error( sprintf( 'Failed to stage database dump for database: %s', $db_name ), self::ERROR_TYPE_DATABASE, - 4003 + 4005 ); EE::error( 'Database backup failed while staging the dump file.' ); } From ba0dfdb5afa6de6bead637f6a3553d918d512fe8 Mon Sep 17 00:00:00 2001 From: Riddhesh Sanghvi Date: Thu, 24 Sep 2026 16:55:40 +0000 Subject: [PATCH 9/9] fix(backup): run the metadata and db-size shells through the running ee binary The WordPress metadata queries and the pre-backup DB size check still launched `ee shell` through PATH. Where `ee` isn't on PATH (or is a different EE), every call failed silently: meta.json got `"wordpressVersion": "-"`, which a restore can't use, and the DB size counted as 0 in the disk-space check. Use the same absolute child as the dump. --- src/helper/Site_Backup_Restore.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/helper/Site_Backup_Restore.php b/src/helper/Site_Backup_Restore.php index c8f0b6a1..491a787f 100644 --- a/src/helper/Site_Backup_Restore.php +++ b/src/helper/Site_Backup_Restore.php @@ -324,7 +324,7 @@ private function run_wp_cli_command( $command, $skip_plugins_themes = false ) { $shell_command .= ' --skip-plugins --skip-themes '; } $shell_command .= $command; - $output = EE::launch( "ee shell " . $this->site_data['site_url'] . " --skip-tty --command=\"$shell_command\"" ); + $output = EE::launch( $this->get_ee_shell_command( $shell_command ) ); $clean_output = trim( $output->stdout ); return empty( $clean_output ) ? '-' : $clean_output; @@ -1285,7 +1285,7 @@ private function get_db_size() { $command = sprintf( "mysql --skip-ssl -u %s -p%s -h %s %s < /var/www/htdocs/db_size_query.sql", $user, $password, $host, $db_name ); - $output = EE::launch( "ee shell " . $this->site_data['site_url'] . " --skip-tty --command=\"$command\"" ); + $output = EE::launch( $this->get_ee_shell_command( $command ) ); $this->fs->remove( $query_file );