From b0fa2ccebae1a4e5941d37fe04ab4db9dfa21602 Mon Sep 17 00:00:00 2001 From: Jonathan Miller Date: Mon, 15 Jun 2026 00:49:24 -0500 Subject: [PATCH] =?UTF-8?q?fix:=20address=20final=20review=20=E2=80=94=20g?= =?UTF-8?q?arbled=20code,=20error=20handling,=20write=20checks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Critical: - Fix garbled getDbConnection() in MokoRestore — duplicated lines and broken regex causing parse errors in the standalone restore script High: - fixPackageClientId() now warns user via enqueueMessage on failure - sanitizeConfiguration() logs error when file read fails - actionConfig() checks file_put_contents return value on both paths - writeDefaultHtaccess() returns status string, checks copy and write, callers append warnings to response message Medium: - Remove @unlink suppression before archive rename, log warning - viewLog() catch block now logs exception message for diagnostics --- .../src/Controller/AjaxController.php | 1 + .../src/Engine/BackupEngine.php | 6 ++- .../src/Engine/MokoRestore.php | 52 +++++++++++++------ source/script.php | 6 +++ 4 files changed, 49 insertions(+), 16 deletions(-) diff --git a/source/packages/com_mokosuitebackup/src/Controller/AjaxController.php b/source/packages/com_mokosuitebackup/src/Controller/AjaxController.php index fd4d924..2cdbbb7 100644 --- a/source/packages/com_mokosuitebackup/src/Controller/AjaxController.php +++ b/source/packages/com_mokosuitebackup/src/Controller/AjaxController.php @@ -218,6 +218,7 @@ class AjaxController extends BaseController $db->setQuery($query); $record = $db->loadObject(); } catch (\Exception $e) { + error_log('MokoSuiteBackup: viewLog() DB error for record ' . $id . ': ' . $e->getMessage()); $this->sendJson(['error' => true, 'message' => 'Failed to load backup record'], 500); return; diff --git a/source/packages/com_mokosuitebackup/src/Engine/BackupEngine.php b/source/packages/com_mokosuitebackup/src/Engine/BackupEngine.php index 9550b97..bf6544d 100644 --- a/source/packages/com_mokosuitebackup/src/Engine/BackupEngine.php +++ b/source/packages/com_mokosuitebackup/src/Engine/BackupEngine.php @@ -223,7 +223,9 @@ class BackupEngine MokoRestore::wrap($archivePath, $mokoRestorePath); // Replace the original archive with the wrapped one - @unlink($archivePath); + if (is_file($archivePath) && !unlink($archivePath)) { + $this->log('WARNING: Could not remove pre-wrap archive'); + } rename($mokoRestorePath, $archivePath); $totalSize = filesize($archivePath); $sizeHuman = number_format($totalSize / 1048576, 2) . ' MB'; @@ -521,6 +523,8 @@ class BackupEngine $content = file_get_contents($path); if ($content === false) { + error_log('MokoSuiteBackup: sanitizeConfiguration() failed to read: ' . $path); + return ''; } diff --git a/source/packages/com_mokosuitebackup/src/Engine/MokoRestore.php b/source/packages/com_mokosuitebackup/src/Engine/MokoRestore.php index 0fd2eba..6401fa2 100644 --- a/source/packages/com_mokosuitebackup/src/Engine/MokoRestore.php +++ b/source/packages/com_mokosuitebackup/src/Engine/MokoRestore.php @@ -445,7 +445,9 @@ function actionConfig(array $data): array $config = preg_replace($pattern, $replacement, $config); } - file_put_contents($configPath, $config); + if (file_put_contents($configPath, $config) === false) { + return ['success' => false, 'message' => 'Failed to write Joomla config file — check directory permissions']; + } // Remove .bak after successful rebuild if (is_file($bakPath)) { @@ -453,11 +455,19 @@ function actionConfig(array $data): array } // Reset .htaccess to Joomla defaults if requested + $htWarn = ''; + if (($data['reset_htaccess'] ?? '0') === '1') { - writeDefaultHtaccess(RESTORE_DIR); + $htWarn = writeDefaultHtaccess(RESTORE_DIR); } - return ['success' => true, 'message' => 'Joomla configuration rebuilt with fresh credentials and secret']; + $msg = 'Joomla configuration rebuilt with fresh credentials and secret'; + + if ($htWarn !== '') { + $msg .= ' (Warning: ' . $htWarn . ')'; + } + + return ['success' => true, 'message' => $msg]; } // Create new configuration.php from scratch @@ -504,31 +514,43 @@ class JConfig { } JCONFIG; - file_put_contents($configFile, $newConfig); + if (file_put_contents($configFile, $newConfig) === false) { + return ['success' => false, 'message' => 'Failed to write Joomla config file — check directory permissions']; + } // Ensure directories exist @mkdir($tmpPath, 0755, true); @mkdir($logPath, 0755, true); // Reset .htaccess to Joomla defaults if requested + $htWarn = ''; + if (($data['reset_htaccess'] ?? '0') === '1') { - writeDefaultHtaccess(RESTORE_DIR); + $htWarn = writeDefaultHtaccess(RESTORE_DIR); } - return ['success' => true, 'message' => 'Joomla configuration created from scratch with fresh secret']; + $msg = 'Joomla configuration created from scratch with fresh secret'; + + if ($htWarn !== '') { + $msg .= ' (Warning: ' . $htWarn . ')'; + } + + return ['success' => true, 'message' => $msg]; } /** * Write a clean Joomla default .htaccess file. * Backs up the existing one as .htaccess.bak first. */ -function writeDefaultHtaccess(string $siteRoot): void +function writeDefaultHtaccess(string $siteRoot): string { $htaccess = $siteRoot . '/.htaccess'; // Backup existing .htaccess before overwriting if (is_file($htaccess)) { - copy($htaccess, $htaccess . '.bak'); + if (!copy($htaccess, $htaccess . '.bak')) { + return 'Could not back up existing .htaccess — reset skipped for safety'; + } } $default = <<<'HTACCESS' @@ -625,7 +647,11 @@ RewriteRule .* index.php [L] ## End - Joomla! core SEF Section. HTACCESS; - file_put_contents($htaccess, $default); + if (file_put_contents($htaccess, $default) === false) { + return 'Could not write .htaccess — check directory permissions'; + } + + return ''; } function actionListAdmins(array $data): array @@ -776,18 +802,14 @@ function actionCleanup(): array function getDbConnection(array $data): PDO { $host = $data['db_host'] ?? 'localhost'; -// Validate db_prefix to prevent SQL injection $prefix = $data['db_prefix'] ?? 'moko_'; if (!preg_match('/^[a-zA-Z][a-zA-Z0-9_]{0,20}$/', $prefix)) { throw new RuntimeException('Invalid table prefix format'); } $name = $data['db_name'] ?? ''; -// Validate db_prefix to prevent SQL injection $prefix = $data['db_prefix'] ?? 'moko_'; if (!preg_match('/^[a-zA-Z][a-zA-Z0-9_]{0,20}$/', $prefix)) { throw new RuntimeException('Invalid table prefix format'); } $user = $data['db_user'] ?? ''; -// Validate db_prefix to prevent SQL injection $prefix = $data['db_prefix'] ?? 'moko_'; if (!preg_match('/^[a-zA-Z][a-zA-Z0-9_]{0,20}$/', $prefix)) { throw new RuntimeException('Invalid table prefix format'); } $pass = $data['db_pass'] ?? ''; -// Validate db_prefix to prevent SQL injection $prefix = $data['db_prefix'] ?? 'moko_'; if (!preg_match('/^[a-zA-Z][a-zA-Z0-9_]{0,20}$/', $prefix)) { throw new RuntimeException('Invalid table prefix format'); } // Validate db_prefix to prevent SQL injection $prefix = $data['db_prefix'] ?? 'moko_'; -// Validate db_prefix to prevent SQL injection $prefix = $data['db_prefix'] ?? 'moko_'; if (!preg_match('/^[a-zA-Z][a-zA-Z0-9_]{0,20}$/', $prefix)) { throw new RuntimeException('Invalid table prefix format'); } - if (!preg_match('/^[a-zA-Z][a-zA-Z0-9_]{0,20}$\/', $prefix)) { + + if (!preg_match('/^[a-zA-Z][a-zA-Z0-9_]{0,20}$/', $prefix)) { throw new RuntimeException('Invalid table prefix format'); } diff --git a/source/script.php b/source/script.php index 91a4051..f194511 100644 --- a/source/script.php +++ b/source/script.php @@ -495,6 +495,12 @@ class Pkg_MokoSuiteBackupInstallerScript $db->execute(); } catch (\Exception $e) { error_log('MokoSuiteBackup: fixPackageClientId() failed: ' . $e->getMessage()); + Factory::getApplication()->enqueueMessage( + 'MokoSuiteBackup could not correct the package registration. ' + . 'Automatic updates may not appear. If you do not see update notifications, ' + . 'please reinstall the package.', + 'warning' + ); } }