Conversation
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().
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.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the site backup workflow to prevent silent corruption from being treated as a successful backup, ensuring bad artifacts are detected before they can be uploaded and overwrite good backups.
Changes:
- Capture and validate
mysqldumpexit status and ensure the resulting dump file exists and is non-empty. - Check
mvstaging of the dump intosql/and assert the staged dump exists and is non-empty before archiving. - Add
7z tintegrity verification for produced archives (including optional custom docker-compose archive, with warn-and-exclude semantics).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+692
to
+695
| // 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. |
Member
Author
There was a problem hiding this comment.
Fixed in ac60000 — the comment now references only get_db_size() (which does use escapeshellarg()), since restore_db() uses manual single-quoting. (restore_db() itself is switched to escapeshellarg() in the separate restore PR.)
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.
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.
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.
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.
3003 and 4003 are already taken by EasyEngine#482 (site app directory not found) and EasyEngine#481 (orphaned remote backup delete failure). Use the next free codes: 3005 for the archive integrity failure and 4005 for the dump staging failure.
…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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes three holes in the backup path through which a failed or corrupt backup could upload as "successful" and overwrite a good one.
Fixes
mysqldumpfailure handling depended on a subcommand'sexit(). With shell-command ≥ v1.1.3 (current releases), a failing dump made the in-processee shellcallexit(1): the backup stopped with noError:line and, with--dash-auth, EasyDash got the generic 6000 "interrupted" code. With older shell-command, the>redirect's 0-byte file passed theexists()check and was uploaded as a successful backup. The dump now runs as a childee shell … --command=…viaEE::launch(), started through the runningeebinary by absolute path (the phar, orphp/boot-fs.phpfor a source install, with the current PHP binary) rather than aneelooked up onPATH, so its exit code is captured (EE::run_command()returns void and couldn't), and is validated byreturn_code === 0ANDexistsANDfilesize > 0. On failure the mysqldump error (e.g.1045 Access denied) is shown as a warning before the database-backup error.mvcould ship a database-less backup. After the dump, the stagingmvintosql/was unchecked; if it failed (cross-device, permissions, disk-full, …),sql/was left empty,7z uon an empty dir exits 0, and7z tpassed — a backup with no database shipping as success. Themvresult is now checked and the destination asserted (exists+filesize > 0) before archiving.verify_archive_integrity()runs7z t(treating exit ≥ 2 as failure, so a non-fatal 7z warning doesn't abort) on every archiverclone_upload()ships — the primary<site>.zip,conf.zip(nginx + php), and the optional custom-docker-compose zip (warn-and-exclude, preserving its optional semantics) — aborting before upload if any is corrupt. A custom docker-compose archive whose creation fails is dropped from the backup too.Plus: DB credentials are
escapeshellarg()-quoted in the mysqldump command (matchingrestore_db()/get_db_size()); the comment notes this is best-effort layer-1 quoting (the inneree shellbash -c "…"layer still can't carry arbitrary`/"/$).The WordPress metadata queries written to
meta.jsonand the DB size used by the disk-space pre-check (get_db_size()) now run through the same absoluteeechild. Before, they usedee shellfromPATH, so whereeewasn't onPATH(or was a different EE) every call failed silently:meta.jsonrecorded"wordpressVersion": "-", which a restore can't use, and the database counted as 0 in the pre-check.Error codes
New error codes in this PR:
30054005New error codes across the open backup PRs (none of them is used on
develop):300330043005400340044005Verification
Wrong DB password → the mysqldump error plus
Database backup failed…(code4002), nothing uploaded (before: silent exit 1). A failing stagingmv→Database backup failed while staging the dump file.(code4005), nothing uploaded (before: "Backup created successfully." with an emptysql/in the zip). An archive corrupted after the last write →Backup archive failed integrity verification…(code3005), nothing uploaded (before: uploaded, and7z treports a CRC error). WordPress, PHP-with-DB and HTML sites back up and restore normally, and the archives pass7z t. Witheenot onPATHand a differenteeearlier onPATH, WordPress and PHP-with-DB backups succeed from both a source checkout and a built phar: the dump, metadata and DB-size calls run through the running binary, the othereeis never called, andmeta.jsonrecords the real WordPress version.Tested on Ubuntu 26.04 with EasyEngine 4.12.0 and 7-Zip 26.00, on a real rclone remote (happy paths, restore round trip, custom docker-compose archive) and a local rclone remote (failure cases), plus backups with a minimal
PATHfrom a source checkout and from a phar.