Skip to content

fix(backup): restore no longer wipes the database on Windows (#249) #251

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
HugoFara merged 1 commit into from
Jul 25, 2026
Merged

fix(backup): restore no longer wipes the database on Windows (#249) #251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,34 @@ ones are marked like "v1.0.0-fork".
the same way `SqlFileParser` does since #241. This is what failed the Windows
PHPUnit jobs. Verified end-to-end against MeCab 0.996 and covered by tests
that assert LF, CRLF and CR-only output tokenize identically.
* **Restoring a backup wiped the database and reported success** (#249): on a
Windows host, "Restore from Backup" emptied the install and restored nothing,
while the page still said "Database restored". `Restore::restoreFile` split
the dump into statements on `';' . PHP_EOL` — `";\r\n"` on Windows — but LWT
always writes `";\n"`, so the split never matched, every statement piled up
in the accumulator and the statement list came back empty. The restore then
dropped every table and replayed nothing. This is the same defect fixed in
`SqlFileParser` for #241; the restore path was missed. Line endings are now
normalized before splitting, so a dump restores identically regardless of
which OS wrote or reads it. Three further hardening changes make the failure
non-destructive if it ever recurs: a dump that parses to zero statements is
refused *before* any table is dropped, the restore's own report (query and
record counts) is shown verbatim instead of a flat "Database restored", and a
final statement not followed by a newline is no longer discarded.
* **Restore left the database in pieces when foreign keys were enabled**
(#249): a dump's `CREATE TABLE` statements come from `SHOW CREATE TABLE`, so
they carry their `FOREIGN KEY` clauses, but they are replayed in backup-table
order rather than dependency order (`feed_links` references `news_feeds` yet
is created before it). With foreign-key checks on, every such statement
failed with errno 1215 — on a 12-table backup, 11 of 12 `CREATE TABLE`s were
lost. Checks are now suspended for the replay and enforced again once every
table exists.
* **Choosing a restore file did nothing** (#249): the file field stayed empty
and the "Restore from Backup" button stayed disabled, because the inline
`@change` handler used optional chaining
(`$event.target.files[0]?.name || ''`), which `@alpinejs/csp` cannot parse —
it threw "CSP Parser Error: Unexpected token" on every selection. The handler
moved into the `backupManager` component as `selectFile()`.

### Changed

Expand Down
2 changes: 1 addition & 1 deletion src/Modules/Admin/Application/AdminFacade.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ public function saveAndClearSession(string $key, string $value): void
* @param array{name: string, type: string, tmp_name: string, error: int, size: int}|null $fileData
* Validated file data from InputValidator::getUploadedFile()
*
* @return array{success: bool, error: ?string}
* @return array{success: bool, error: ?string, message?: string}
*/
public function restoreFromUpload(?array $fileData): array
{
Expand Down
7 changes: 5 additions & 2 deletions src/Modules/Admin/Application/UseCases/Backup/RestoreFromUpload.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,10 @@ public function __construct(BackupRepositoryInterface $repository)
* @param array{name: string, type: string, tmp_name: string, error: int, size: int}|null $fileData
* Validated file data from InputValidator::getUploadedFile()
*
* @return array{success: bool, error: ?string}
* @return array{success: bool, error: ?string, message?: string}
* On success, `message` carries the restore's own report (query
* and record counts), which the caller should show verbatim
* rather than substituting a generic "restored" string.
*/
public function execute(?array $fileData): array
{
Expand Down Expand Up @@ -97,6 +100,6 @@ public function execute(?array $fileData): array
if (str_starts_with($message, 'Error:')) {
return ['success' => false, 'error' => $message];
}
return ['success' => true, 'error' => null];
return ['success' => true, 'error' => null, 'message' => $message];
}
}
8 changes: 7 additions & 1 deletion src/Modules/Admin/Http/AdminController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,13 @@ public function backup(array $params): void
$result = $this->adminFacade->restoreFromUpload(
InputValidator::getUploadedFile('thefile')
);
$message = $result['success'] ? 'Database restored' : ($result['error'] ?? 'Restore failed');
// Show the restore's own report rather than a flat "Database
// restored": its query/record counts are the only signal an admin
// has that the dump actually replayed. A restore that parsed zero
// statements used to look identical to a real one here (#249).
$message = $result['success']
? ($result['message'] ?? 'Database restored')
: ($result['error'] ?? 'Restore failed');
} elseif ($this->hasParam('backup')) {
$this->adminFacade->downloadBackup();
// downloadBackup exits, so we never reach here
Expand Down
2 changes: 1 addition & 1 deletion src/Modules/Admin/Views/backup.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@
<div class="file has-name is-fullwidth">
<label class="file-label">
<input class="file-input" type="file" name="thefile"
@change="fileName = $event.target.files[0]?.name || ''"
@change="selectFile($event)"
accept=".sql,.gz,.sql.gz">
<span class="file-cta">
<span class="file-icon">
Expand Down
218 changes: 150 additions & 68 deletions src/Shared/Infrastructure/Database/Restore.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,49 +83,33 @@ private static function dropAllLwtTables(): void
}

/**
* Restore the database from a file.
* Read a backup stream and split it into individual SQL statements.
*
* @param resource $handle Backup file handle
* @param string $title File title
* @param bool $validateSql Whether to validate SQL statements (default true)
* Statement boundaries are matched platform-independently. LWT always
* writes ";\n", but this used to split on ';' . PHP_EOL, which is
* ";\r\n" on a Windows host and therefore never matched: every
* statement piled up in the accumulator and the caller received an
* empty list, dropped every table and replayed nothing (issue #249).
* Line endings are now normalised exactly as
* SqlFileParser::parseFile() does, so a dump restores identically
* regardless of which OS wrote it or reads it.
*
* @return string Human-readable status message
* The handle is always closed before returning.
*
* @since 2.0.3-fork Function was broken
* @since 2.5.3-fork Function repaired
* @since 2.7.0-fork $handle should be an *uncompressed* file.
* @since 2.9.1-fork It can read SQL with more or less than one instruction a line
* @since 3.0.0 Added SQL validation for security hardening
* @param resource $handle Backup file handle (may be a gzopen handle)
* @param string $title File title, used in error messages
*
* @return array{queries: list<string>, error: string|null}
*
* @since 3.2.2-fork Extracted from restoreFile() and made EOL-agnostic
*/
public static function restoreFile($handle, string $title, bool $validateSql = true): string
private static function parseBackupStream($handle, string $title): array
{
// Multi-user safety guard: the rest of this function drops every
// LWT table and replaces it with the dump's contents, which would
// wipe every other user's data. Refuse the restore when more
// than one active user exists. The admin must take the multi-
// user mode down first if they really mean it (or use the
// per-user "Empty Database" + manual backup/restore recipe in
// docs).
$contextError = self::validateMultiUserRestoreContext();
if ($contextError !== null) {
fclose($handle);
return $contextError;
}

$message = "";
$hasErrors = false;
$install_status = [
"queries" => 0,
"successes" => 0,
"errors" => 0,
"drops" => 0,
"inserts" => 0,
"creates" => 0
];
$start = true;
$curr_content = '';
$queries_list = [];
$curr_content = '';
$start = true;
$bytesRead = 0;
$error = null;

while ($stream = fgets($handle)) {
// Gzip-bomb defense: cap the total decompressed bytes we
Expand All @@ -134,23 +118,26 @@ public static function restoreFile($handle, string $title, bool $validateSql = t
// certainly malicious or a runaway.
$bytesRead += strlen($stream);
if ($bytesRead > self::MAX_DECOMPRESSED_BYTES) {
$message = "Error: $title Restore file exceeds the "
$error = "Error: $title Restore file exceeds the "
. intdiv(self::MAX_DECOMPRESSED_BYTES, 1024 * 1024)
. " MB decompressed-size limit (likely a gzip bomb).";
$install_status["errors"] = 1;
$hasErrors = true;
break;
}

// Normalise line endings before anything is matched against
// them. formatValueForSqlOutput() escapes CR inside values via
// mysqli_real_escape_string, so a raw CR in a dump is only ever
// a line terminator and is safe to rewrite.
$stream = str_replace(["\r\n", "\r"], "\n", $stream);

// Check file header
if ($start) {
if (
!str_starts_with($stream, "-- lwt-backup-")
&& !str_starts_with($stream, "-- lwt-exp_version-backup-")
) {
$message = "Error: Invalid $title Restore file " .
$error = "Error: Invalid $title Restore file " .
"(possibly not created by LWT backup)";
$install_status["errors"] = 1;
$hasErrors = true;
break;
}
$start = false;
Expand All @@ -164,7 +151,7 @@ public static function restoreFile($handle, string $title, bool $validateSql = t
// Add stream to accumulator
$curr_content .= $stream;
// Get queries
$queries = explode(';' . PHP_EOL, $curr_content);
$queries = explode(";\n", $curr_content);
// Replace line by remainders of the last element (incomplete line)
$curr_content = array_pop($queries);

Expand All @@ -173,12 +160,81 @@ public static function restoreFile($handle, string $title, bool $validateSql = t
}
}

if (!feof($handle) && !$hasErrors) {
$message = "Error: cannot read the end of the demo file!";
$reachedEof = feof($handle);
fclose($handle);

if ($error !== null) {
return ['queries' => [], 'error' => $error];
}

if (!$reachedEof) {
return [
'queries' => [],
'error' => "Error: cannot read the end of the $title file!",
];
}

// Flush a trailing statement that is not followed by a newline, so a
// dump whose final line lacks its EOL still restores completely.
$tail = trim(rtrim(trim($curr_content), ';'));
if ($tail !== '') {
$queries_list[] = $tail;
}

return ['queries' => $queries_list, 'error' => null];
}

/**
* Restore the database from a file.
*
* @param resource $handle Backup file handle
* @param string $title File title
* @param bool $validateSql Whether to validate SQL statements (default true)
*
* @return string Human-readable status message
*
* @since 2.0.3-fork Function was broken
* @since 2.5.3-fork Function repaired
* @since 2.7.0-fork $handle should be an *uncompressed* file.
* @since 2.9.1-fork It can read SQL with more or less than one instruction a line
* @since 3.0.0 Added SQL validation for security hardening
* @since 3.2.2-fork Refuses to drop anything when the dump yields no
* statements; replays with FK checks suspended so FK-carrying
* CREATE TABLEs no longer fail on dependency order
*/
public static function restoreFile($handle, string $title, bool $validateSql = true): string
{
// Multi-user safety guard: the rest of this function drops every
// LWT table and replaces it with the dump's contents, which would
// wipe every other user's data. Refuse the restore when more
// than one active user exists. The admin must take the multi-
// user mode down first if they really mean it (or use the
// per-user "Empty Database" + manual backup/restore recipe in
// docs).
$contextError = self::validateMultiUserRestoreContext();
if ($contextError !== null) {
fclose($handle);
return $contextError;
}

$message = "";
$hasErrors = false;
$install_status = [
"queries" => 0,
"successes" => 0,
"errors" => 0,
"drops" => 0,
"inserts" => 0,
"creates" => 0
];

$parsed = self::parseBackupStream($handle, $title);
$queries_list = $parsed['queries'];
if ($parsed['error'] !== null) {
$message = $parsed['error'];
$install_status["errors"] = 1;
$hasErrors = true;
}
fclose($handle);

// Validate all queries before executing any (security hardening)
if ($validateSql && !$hasErrors) {
Expand All @@ -196,6 +252,18 @@ public static function restoreFile($handle, string $title, bool $validateSql = t
}
}

// A dump that yielded no statements must never reach
// dropAllLwtTables(): dropping every table and then replaying nothing
// leaves the install empty with no way back. That is exactly what the
// PHP_EOL statement split did on Windows (issue #249) — silently, and
// while reporting success. Refuse instead, so any future parser bug
// costs an error message rather than the user's data.
if (!$hasErrors && $queries_list === []) {
$message = "Error: $title Restore file contains no SQL statements";
$install_status["errors"] = 1;
$hasErrors = true;
}

// Drop all existing tables first to ensure a clean slate
// This prevents issues with partial state from previous attempts
if (!$hasErrors) {
Expand All @@ -205,32 +273,46 @@ public static function restoreFile($handle, string $title, bool $validateSql = t
// Now run all queries
$connection = Globals::getDbConnection();
if (!$hasErrors && $connection !== null) {
foreach ($queries_list as $query) {
$sql_line = trim(
str_replace("\r", "", str_replace("\n", "", $query))
);
if ($sql_line != "") {
if (!str_starts_with($query, '-- ')) {
$res = mysqli_query(
$connection,
$query
);
$install_status["queries"]++;
if ($res == false) {
$install_status["errors"]++;
$hasErrors = true;
} else {
$install_status["successes"]++;
if (str_starts_with($query, "INSERT INTO")) {
$install_status["inserts"]++;
} elseif (str_starts_with($query, "DROP TABLE")) {
$install_status["drops"]++;
} elseif (str_starts_with($query, "CREATE TABLE")) {
$install_status["creates"]++;
// A dump's CREATE TABLE statements are taken from SHOW CREATE
// TABLE, so they carry their FOREIGN KEY clauses — but they are
// emitted in backup-table order, not dependency order (feed_links
// references news_feeds yet is created before it). With FK checks
// on, every such CREATE fails with errno 1215 and the restore
// leaves the install in pieces. dropAllLwtTables() restores checks
// to 1 on the way out, so they have to be suspended again here for
// the replay; the constraints are enforced again afterwards, once
// every table exists.
Connection::execute("SET FOREIGN_KEY_CHECKS = 0");
try {
foreach ($queries_list as $query) {
$sql_line = trim(
str_replace("\r", "", str_replace("\n", "", $query))
);
if ($sql_line != "") {
if (!str_starts_with($query, '-- ')) {
$res = mysqli_query(
$connection,
$query
);
$install_status["queries"]++;
if ($res == false) {
$install_status["errors"]++;
$hasErrors = true;
} else {
$install_status["successes"]++;
if (str_starts_with($query, "INSERT INTO")) {
$install_status["inserts"]++;
} elseif (str_starts_with($query, "DROP TABLE")) {
$install_status["drops"]++;
} elseif (str_starts_with($query, "CREATE TABLE")) {
$install_status["creates"]++;
}
}
}
}
}
} finally {
Connection::execute("SET FOREIGN_KEY_CHECKS = 1");
}
}

Expand Down
Loading