From ea117b6b3f2a988eadb5c8638c49e61acfdaac09 Mon Sep 17 00:00:00 2001 From: Riddhesh Sanghvi Date: Mon, 29 Jun 2026 22:02:50 +0530 Subject: [PATCH 1/3] 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/3] 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/3] 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.