fix(backup): verify DB dump and archive integrity to prevent silent corrupt backups#483
Open
mrrobot47 wants to merge 3 commits into
Open
fix(backup): verify DB dump and archive integrity to prevent silent corrupt backups#483mrrobot47 wants to merge 3 commits into
mrrobot47 wants to merge 3 commits into
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.
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 silent-data-loss holes in the backup path so a failed/corrupt backup can never upload as "successful" and overwrite a good one.
Fixes
mysqldumpfailure produced a silent empty backup. The dump runs via a shell>redirect, which truncates/creates the target before mysqldump runs — so a failed dump left a 0-byte file that passed the oldfs->exists()-only check, got archived, and uploaded as success (after whichcleanup_old_backups()could prune the good ones). The dump now runs viaEE::launch('ee shell … --command=…')so its exit code is captured (EE::run_command()returns void and couldn't), and is validated byreturn_code === 0ANDexistsANDfilesize > 0.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.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`/"/$).Verification
php -lpasses. Reviewed by two independent reviewers (correctness + design); both empirically confirmed in containers thatee shell --commandpropagates the inner exit code, that a failedmv/emptysql/slips past7z t(the merge-blocker, now fixed), and that the exit-code check catches mid-stream mysqldump failures (OOM→137, lost-connection→3, disk-full→5).Merge note
Built on
developindependently of the EasyDash callback PR (#481); both happened to introduce error code4003for different failures, so whichever merges second should renumber its4003to a free code (trivial). This PR's4003= "database dump failed to stage" (ERROR_TYPE_DATABASE).