From d5421738b7f4e52ec5ca7901d9aec49dfc21743e Mon Sep 17 00:00:00 2001 From: Jonathan Miller Date: Sun, 21 Jun 2026 17:21:55 -0500 Subject: [PATCH] =?UTF-8?q?fix:=20address=20PR=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20error=20handling=20and=20safety?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes from code review and silent failure audit: - SnapshotRestoreEngine: catch only duplicate key errors (MySQL 1062) in merge mode, re-throw all other exceptions instead of swallowing - SnapshotRestoreEngine: add json_last_error() check for better error messages on corrupt snapshot files - SnapshotRestoreEngine: log warnings when set_time_limit/ini_set fail - SnapshotEngine: use strlen($json) instead of filesize() to avoid race conditions; catch \Exception instead of \Throwable - SnapshotsController: remove @unlink suppression, add try-catch around delete loop with partial failure reporting - script.php: add user-facing warning when webcron secret generation fails (was silently swallowed, inconsistent with other catch blocks) --- .../language/en-GB/com_mokosuitebackup.ini | 1 + .../src/Controller/SnapshotsController.php | 54 ++++++++++++------- .../src/Engine/SnapshotEngine.php | 4 +- .../src/Engine/SnapshotRestoreEngine.php | 24 +++++++-- source/script.php | 5 ++ 5 files changed, 62 insertions(+), 26 deletions(-) diff --git a/source/packages/com_mokosuitebackup/language/en-GB/com_mokosuitebackup.ini b/source/packages/com_mokosuitebackup/language/en-GB/com_mokosuitebackup.ini index 3908dfc..72366dd 100644 --- a/source/packages/com_mokosuitebackup/language/en-GB/com_mokosuitebackup.ini +++ b/source/packages/com_mokosuitebackup/language/en-GB/com_mokosuitebackup.ini @@ -333,6 +333,7 @@ COM_MOKOJOOMBACKUP_SNAPSHOT_RESTORE_TYPES="Types to restore" COM_MOKOJOOMBACKUP_SNAPSHOT_REPLACE_WARNING="Replace mode will delete all current content of the selected types before restoring from the snapshot. This cannot be undone." COM_MOKOJOOMBACKUP_SNAPSHOTS_N_DELETED="%d snapshot(s) deleted." COM_MOKOJOOMBACKUP_SNAPSHOTS_1_DELETED="1 snapshot deleted." +COM_MOKOJOOMBACKUP_SNAPSHOTS_DELETE_ERRORS="Failed to delete snapshot(s): %s" ; Snapshot ACL COM_MOKOSUITEBACKUP_ACTION_SNAPSHOT_MANAGE="Manage Snapshots" diff --git a/source/packages/com_mokosuitebackup/src/Controller/SnapshotsController.php b/source/packages/com_mokosuitebackup/src/Controller/SnapshotsController.php index 74b5d4d..3aa1678 100644 --- a/source/packages/com_mokosuitebackup/src/Controller/SnapshotsController.php +++ b/source/packages/com_mokosuitebackup/src/Controller/SnapshotsController.php @@ -131,33 +131,49 @@ class SnapshotsController extends AdminController $db = Factory::getDbo(); $deleted = 0; + $errors = []; foreach ($cid as $id) { $id = (int) $id; - // Load record to get file path - $query = $db->getQuery(true) - ->select($db->quoteName('data_file')) - ->from($db->quoteName('#__mokosuitebackup_snapshots')) - ->where($db->quoteName('id') . ' = ' . $id); - $db->setQuery($query); - $dataFile = $db->loadResult(); + try { + // Load record to get file path + $query = $db->getQuery(true) + ->select($db->quoteName('data_file')) + ->from($db->quoteName('#__mokosuitebackup_snapshots')) + ->where($db->quoteName('id') . ' = ' . $id); + $db->setQuery($query); + $dataFile = $db->loadResult(); - // Delete data file - if ($dataFile && is_file($dataFile)) { - @unlink($dataFile); + // Delete data file + if ($dataFile && is_file($dataFile)) { + if (!unlink($dataFile)) { + error_log('MokoSuiteBackup: Failed to delete snapshot file: ' . $dataFile); + } + } + + // Delete record + $query = $db->getQuery(true) + ->delete($db->quoteName('#__mokosuitebackup_snapshots')) + ->where($db->quoteName('id') . ' = ' . $id); + $db->setQuery($query); + $db->execute(); + $deleted++; + } catch (\Exception $e) { + error_log('MokoSuiteBackup: Failed to delete snapshot ' . $id . ': ' . $e->getMessage()); + $errors[] = $id; } - - // Delete record - $query = $db->getQuery(true) - ->delete($db->quoteName('#__mokosuitebackup_snapshots')) - ->where($db->quoteName('id') . ' = ' . $id); - $db->setQuery($query); - $db->execute(); - $deleted++; } - $this->setMessage(Text::plural('COM_MOKOJOOMBACKUP_SNAPSHOTS_N_DELETED', $deleted)); + if (!empty($errors)) { + $this->setMessage( + Text::plural('COM_MOKOJOOMBACKUP_SNAPSHOTS_N_DELETED', $deleted) + . ' ' . Text::sprintf('COM_MOKOJOOMBACKUP_SNAPSHOTS_DELETE_ERRORS', implode(', ', $errors)), + 'warning' + ); + } else { + $this->setMessage(Text::plural('COM_MOKOJOOMBACKUP_SNAPSHOTS_N_DELETED', $deleted)); + } $this->setRedirect(Route::_('index.php?option=com_mokosuitebackup&view=snapshots', false)); } } diff --git a/source/packages/com_mokosuitebackup/src/Engine/SnapshotEngine.php b/source/packages/com_mokosuitebackup/src/Engine/SnapshotEngine.php index 6913c62..4ea16a3 100644 --- a/source/packages/com_mokosuitebackup/src/Engine/SnapshotEngine.php +++ b/source/packages/com_mokosuitebackup/src/Engine/SnapshotEngine.php @@ -139,7 +139,7 @@ class SnapshotEngine throw new \RuntimeException('Failed to write snapshot file: ' . $filePath); } - $fileSize = filesize($filePath); + $fileSize = strlen($json); $this->log('Snapshot saved: ' . $filename . ' (' . number_format($fileSize) . ' bytes)'); // Create database record @@ -174,7 +174,7 @@ class SnapshotEngine ), 'id' => $record->id, ]; - } catch (\Throwable $e) { + } catch (\Exception $e) { $this->log('FATAL: ' . $e->getMessage()); return [ diff --git a/source/packages/com_mokosuitebackup/src/Engine/SnapshotRestoreEngine.php b/source/packages/com_mokosuitebackup/src/Engine/SnapshotRestoreEngine.php index 7e5a05c..6bb514f 100644 --- a/source/packages/com_mokosuitebackup/src/Engine/SnapshotRestoreEngine.php +++ b/source/packages/com_mokosuitebackup/src/Engine/SnapshotRestoreEngine.php @@ -46,8 +46,13 @@ class SnapshotRestoreEngine */ public function restore(int $snapshotId, string $mode = 'replace', array $contentTypes = []): array { - @set_time_limit(0); - @ini_set('memory_limit', '512M'); + if (!@set_time_limit(0)) { + $this->log('WARNING: Could not disable time limit — large restores may timeout'); + } + + if (!@ini_set('memory_limit', '512M')) { + $this->log('WARNING: Could not increase memory limit to 512M'); + } if (!in_array($mode, ['replace', 'merge'])) { return ['success' => false, 'message' => 'Invalid restore mode: ' . $mode]; @@ -85,8 +90,12 @@ class SnapshotRestoreEngine $data = json_decode($json, true); + if (json_last_error() !== JSON_ERROR_NONE) { + return ['success' => false, 'message' => 'Snapshot file contains invalid JSON: ' . json_last_error_msg()]; + } + if (!is_array($data) || empty($data['tables'])) { - return ['success' => false, 'message' => 'Invalid snapshot data format']; + return ['success' => false, 'message' => 'Invalid snapshot data format: missing tables key']; } $snapshotTypes = $data['content_types'] ?? []; @@ -206,11 +215,16 @@ class SnapshotRestoreEngine $db->insertObject($realTable, $obj); } } else { - // Composite key tables — just insert (for merge, we skip duplicates) + // Composite key tables — insert, skip genuine duplicates try { $db->insertObject($realTable, $obj); } catch (\Exception $e) { - // Duplicate key — skip silently in merge mode + if (str_contains($e->getMessage(), 'Duplicate entry') || $e->getCode() === 1062) { + $this->log(' Skipped duplicate in ' . $abstractTable); + continue; + } + + throw $e; } } diff --git a/source/script.php b/source/script.php index bf595f5..f808d1d 100644 --- a/source/script.php +++ b/source/script.php @@ -232,6 +232,11 @@ class Pkg_MokoSuiteBackupInstallerScript $db->execute(); } catch (\Exception $e) { error_log('MokoSuiteBackup: generateWebcronSecret() failed: ' . $e->getMessage()); + Factory::getApplication()->enqueueMessage( + 'MokoSuiteBackup could not generate a random webcron secret. ' + . 'Please set one manually in the component options to secure the webcron endpoint.', + 'warning' + ); } }