Conversation
Harden the restore path against silent data loss when a backup is partial or corrupt and when a destructive step fails: - Verify the downloaded archive before any destructive extraction. The download target was reused if it already existed on disk, so a truncated archive from an interrupted run was silently extracted right after `rm -rf <app>/*`. Compare the local archive size against the remote `rclone size` value (now threaded from pre_restore_check) and run `unzip -t`; re-download once and abort if still invalid. - Check the result of critical restore steps that EE::run_command/EE::exec do not exit on: `wp config set DB_*`, `wp core download`, the `.sql` unzip, and the mysql import. These now run via `ee shell` (whose return code reflects the in-container failure) and abort with EE::error instead of reporting success. The DB import keeps its stderr (2>&1) for a real diagnostic; mysql never echoes the password, so no secret leaks. - Fix the uploads-symlink dance: set $uploads_moved only when the `mv` actually succeeds, and check each subsequent mv/unzip, so a failed move no longer leads to `rm -rf wp-content` destroying the live uploads dir. - Validate meta.json/metadata.json after json_decode (decoded + required keys present/numeric) before use, so a missing/corrupt file no longer silently installs the latest WordPress or bypasses the disk-space and match guards.
Polish the restore-safety hardening based on review: - Drop the dead size comparison in is_backup_archive_valid(): `rclone size` measures the whole remote folder, not the single archive, so it can never detect a truncated archive. `unzip -t` is the sole real integrity guard now; remove the now-unused $remote_backup_size property (the numeric `bytes` validation for the disk-space guard is kept). - Correct the DB-credential comments: escapeshellarg/single-quoting only guards layer-1 word-splitting; the inner `ee shell` `bash -c "$command"` wrapper (a pre-existing limitation) still re-exposes $, backticks and ", so special-char credentials are not fully carried through. Default EE creds are alphanumeric. - Recover the orphaned uploads symlink on the wp-content extraction abort path: recreate wp-content if needed and move the set-aside symlink back before erroring, so a retry can still find it. On the move-back failure path the error message now states where the symlink remains. - Tolerate a DB-less backup in restore_wp(): `unzip` exits 11 when the archive has no sql/ member, which is not an error (integrity already verified); skip the DB import in that case and abort only on other non-zero codes.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens ee site restore so it verifies backup integrity before destructive operations and reliably aborts when critical restore steps fail, preventing partial/incorrect restores from being reported as success.
Changes:
- Added archive pre-validation (
unzip -t) with a one-time re-download fallback, aborting on corrupt/truncated backups before any wipe/extract. - Introduced a checked execution helper for in-container commands so failures in
wp/mysqlsteps abort the restore. - Improved safety around wp-content/uploads symlink handling and added metadata validation for
meta.json,metadata.json, andrclone size --json.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // wrapper (a pre-existing limitation) still re-exposes $, backticks and ", so a | ||
| // password containing those is not fully carried through. EE's DB credentials | ||
| // are alphanumeric by default, so this only affects operator-set special chars. | ||
| $restore_command = sprintf( "mysql --skip-ssl -u '%s' -p'%s' -h '%s' '%s' < '%s' 2>&1", $db_user, $db_password, $db_host, $db_name, $sql_path ); |
Member
Author
There was a problem hiding this comment.
Fixed in a2badc6 — restore_db() now uses escapeshellarg() per value, matching get_db_size(). (Layer-1 only; the inner ee shell bash -c "$command" wrapper remains a pre-existing limitation, as noted in the comment.)
Replace restore_db()'s manual single-quoting of the mysql command (`-u '%s' -p'%s' …`) with escapeshellarg() per value, matching get_db_size() and maybe_restore_wp_config(). The manual quoting broke on any credential, host or db-name containing a single quote or whitespace; `-p%s` with escapeshellarg still glues the password to `-p` with no space, as mysql requires. This only fixes layer-1 word-splitting/quote handling; the inner `ee shell` `bash -c "$command"` wrapper (a pre-existing limitation) still cannot carry arbitrary $/backtick/" in a value.
… download run_checked_shell_command() runs the command through a child `ee shell`, which wraps it in `bash -c "..."` on the host. With the `\$` escape dropped, the host shell expanded $(which wp) (empty on hosts without wp-cli), so `wp core download` failed with "Could not open input file: core" and every WordPress restore aborted. Restore the escape so the container's shell does the substitution.
…tion pre_restore_check() already downloads the archive, so ensure_valid_backup_archive() found it on disk and ran a full `unzip -t` twice on every restore (once to decide whether to re-download, once more as the final check), doubling the read of a potentially multi-GB archive. Test it once and re-test only after a fresh download. Also pass -qq so the per-entry listing isn't captured and written to ee.log.
The checked restore steps launched a child `ee shell` found through PATH, so a restore where `ee` isn't on PATH aborted (for php/html sites only at the DB import, after the app files were already replaced), and `bin/ee` from source silently ran the steps through the stable phar. Launch the running phar, or `php/boot-fs.php` for a source checkout, with the current PHP binary instead.
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
Makes
ee site restorefail safely instead of destroying or half-restoring a live site, and stops it from reporting success when a step actually failed.Fixes
rm -rf <app>/*/ removingwp-content, so a truncated or corrupt archive could wipe the site and then fail to restore.ensure_valid_backup_archive()now runsunzip -tqq(full CRC check, quiet) before any destructive step; on failure it re-downloads once, re-tests, and aborts rather than extracting a corrupt archive over the live site. The archive is tested once and re-tested only after a fresh download. (A size-vs-rclone sizecheck was considered and dropped — that figure covers the whole remote folder, so it can't detect a truncated single archive;unzip -tis the real guard.)wp config setor DB import stopped the restore with exit 1 but no error message (the in-processee shellexits).wp config set DB_*,wp core download, and themysqlimport now run viarun_checked_shell_command()(a childee shell, 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, whose return code reflects the inner failure) and abort with a specific error on non-zero; the twounzipsteps now check their result too. The core download's$(which wp)is escaped so it expands inside the container, not on the host.$uploads_movedis set only when themvactually succeeds (sorm -rf wp-contentcan't delete the live uploads), each subsequent step is checked, and on the abort pathrestore_moved_uploads()puts the set-aside symlink back so a failed restore doesn't strand the site's uploads reference.meta.json(WP version),metadata.json(site-type/public-dir match), andrclone size --json(bytes) are validated for decode + required keys before use — closing the silent paths where an empty version madewp core downloadfail or fetch an unintended release, andnull['bytes']made the disk-space guard a no-op.A DB-less backup is handled gracefully (the SQL
unzipexit 11 = "no member" is treated as "no DB", matchingrestore_site), and the DB-cred comments are accurate about the pre-existingbash -c "…"shell layer (escapeshellarg guards layer-1 word-splitting only; EE creds are alphanumeric by default).Known limitation
For PHP sites with a database, the app files are replaced before the DB import, so a failed DB import aborts with an error but leaves the restored files with the old database. In the WordPress failure cases tested (corrupt archive, invalid SQL, missing
meta.json) the site is left unchanged.Verification
WordPress and PHP-with-DB restores succeed (content, uploads and DB markers come back). A truncated local archive is re-downloaded and the restore succeeds. A truncated remote archive, a dump with invalid SQL and a backup without
meta.jsoneach abort with a specific error and leave the site unchanged. A restore without--idstill picks the latest backup. Witheenot onPATHand a differenteeearlier onPATH, WordPress and PHP-with-DB restores succeed, every checked step runs through the running binary, and the othereeis never called.Tested on Ubuntu 26.04 with EasyEngine 4.12.0, on a real rclone remote (WordPress and PHP-with-DB restores, including restores with a minimal
PATH) and a local rclone remote with deliberately damaged backups.Merge note
The child
eeis built byget_ee_shell_command(), the same helper #483 adds for the database dump. The two PRs merge cleanly into a single copy of it.