From 2395a4eabc069c185b8e5d381013cbd1a4db66a9 Mon Sep 17 00:00:00 2001 From: Jonathan Miller Date: Sun, 21 Jun 2026 18:08:58 -0500 Subject: [PATCH] fix: critical and high audit findings (#81) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes all critical and high severity issues from the codebase audit: CRITICAL: - #71: RestoreCommand passed wrong args to RestoreEngine (filepath instead of record ID) — CLI restore was completely broken - #72: JpaUnarchiver path traversal — added traversal rejection and realpath boundary check to prevent writes outside staging dir - #77: RestoreEngine staging path sanitized — $record->tag stripped of non-alphanumeric characters HIGH: - #75: (noted, AkeebaImporter unserialize needs separate refactor) - #76: BackupTable now deletes DB row before file — prevents data loss if DB delete fails - #78: API profiles endpoint now masks sensitive fields (passwords, keys, tokens) with '***' - #79: Webcron handler adds return after sendJsonResponse — prevents execution falling through on non-terminal close() - #80: BackupModel/ProfileModel loadFormData() now casts array to object — prevents TypeError on PHP 8.x form state restore PREFLIGHT HARDENING: - PreflightCheck::run() wrapped in try-catch for DB exceptions - mkdir() failure now includes actual error reason - Unresolved placeholders generate a warning instead of silent return Closes #71, closes #76, closes #77, closes #78, closes #79, closes #80 Ref #72, ref #81 --- .../api/src/Controller/BackupsController.php | 20 +++++++++-- .../src/Engine/JpaUnarchiver.php | 23 ++++++++++++ .../src/Engine/PreflightCheck.php | 35 ++++++++++++++----- .../src/Engine/RestoreEngine.php | 5 +-- .../src/Model/BackupModel.php | 2 +- .../src/Model/ProfileModel.php | 2 +- .../src/Table/BackupTable.php | 19 +++++++--- .../src/Command/RestoreCommand.php | 2 +- .../src/Extension/MokoSuiteBackup.php | 6 ++++ 9 files changed, 94 insertions(+), 20 deletions(-) diff --git a/source/packages/com_mokosuitebackup/api/src/Controller/BackupsController.php b/source/packages/com_mokosuitebackup/api/src/Controller/BackupsController.php index a471b48..16f1473 100644 --- a/source/packages/com_mokosuitebackup/api/src/Controller/BackupsController.php +++ b/source/packages/com_mokosuitebackup/api/src/Controller/BackupsController.php @@ -121,11 +121,27 @@ class BackupsController extends ApiController $data = []; + // Strip sensitive credentials before serialization + $sensitiveFields = [ + 'ftp_password', 'ftp_username', + 's3_access_key', 's3_secret_key', + 'gdrive_client_secret', 'gdrive_refresh_token', + 'encryption_password', 'ntfy_token', + ]; + foreach ($items as $item) { + $safe = clone $item; + + foreach ($sensitiveFields as $field) { + if (isset($safe->$field) && $safe->$field !== '') { + $safe->$field = '***'; + } + } + $data[] = [ 'type' => 'profiles', - 'id' => $item->id, - 'attributes' => $item, + 'id' => $safe->id, + 'attributes' => $safe, ]; } diff --git a/source/packages/com_mokosuitebackup/src/Engine/JpaUnarchiver.php b/source/packages/com_mokosuitebackup/src/Engine/JpaUnarchiver.php index 0bb1aae..3889cfd 100644 --- a/source/packages/com_mokosuitebackup/src/Engine/JpaUnarchiver.php +++ b/source/packages/com_mokosuitebackup/src/Engine/JpaUnarchiver.php @@ -206,6 +206,11 @@ class JpaUnarchiver } } + // Path traversal protection: reject absolute paths and directory traversal + if (str_starts_with($path, '/') || str_starts_with($path, '\\') || str_contains($path, '..')) { + return; // skip malicious entry + } + // Is this a directory? if (substr($path, -1) === '/' || $uncompSize === 0 && $compSize === 0) { $dirPath = $this->outputDir . '/' . $path; @@ -228,6 +233,24 @@ class JpaUnarchiver // Write file $fullPath = $this->outputDir . '/' . $path; + + // Verify resolved path stays within output directory + $realOutput = realpath($this->outputDir); + + if ($realOutput !== false) { + $parentDir = dirname($fullPath); + + if (!is_dir($parentDir)) { + mkdir($parentDir, 0755, true); + } + + $realDest = realpath($parentDir); + + if ($realDest === false || !str_starts_with($realDest, $realOutput)) { + return; // path escapes staging directory + } + } + $parentDir = dirname($fullPath); if (!is_dir($parentDir)) { diff --git a/source/packages/com_mokosuitebackup/src/Engine/PreflightCheck.php b/source/packages/com_mokosuitebackup/src/Engine/PreflightCheck.php index fea8fee..74faef4 100644 --- a/source/packages/com_mokosuitebackup/src/Engine/PreflightCheck.php +++ b/source/packages/com_mokosuitebackup/src/Engine/PreflightCheck.php @@ -38,15 +38,27 @@ class PreflightCheck */ public function run(int $profileId): array { - $db = Factory::getDbo(); + try { + $db = Factory::getDbo(); + } catch (\Exception $e) { + $this->errors[] = 'Cannot connect to database: ' . $e->getMessage(); + + return $this->result(); + } // Load profile - $query = $db->getQuery(true) - ->select('*') - ->from($db->quoteName('#__mokosuitebackup_profiles')) - ->where($db->quoteName('id') . ' = ' . $profileId); - $db->setQuery($query); - $profile = $db->loadObject(); + try { + $query = $db->getQuery(true) + ->select('*') + ->from($db->quoteName('#__mokosuitebackup_profiles')) + ->where($db->quoteName('id') . ' = ' . (int) $profileId); + $db->setQuery($query); + $profile = $db->loadObject(); + } catch (\Exception $e) { + $this->errors[] = 'Cannot load profile: ' . $e->getMessage(); + + return $this->result(); + } if (!$profile) { $this->errors[] = 'Profile not found: #' . $profileId; @@ -111,14 +123,19 @@ class PreflightCheck $resolvedDir = BackupDirectory::resolve($resolver->resolve($configuredDir)); if (BackupDirectory::hasPlaceholders($resolvedDir)) { - // Can't fully validate paths with unresolved placeholders + $this->warnings[] = 'Backup directory contains unresolved placeholders: ' . $resolvedDir + . ' — directory cannot be validated until backup runs'; + return; } if (!is_dir($resolvedDir)) { // Try to create it if (!@mkdir($resolvedDir, 0755, true)) { - $this->errors[] = 'Backup directory does not exist and cannot be created: ' . $resolvedDir; + $lastError = error_get_last(); + $reason = $lastError['message'] ?? 'unknown reason'; + $this->errors[] = 'Backup directory does not exist and cannot be created: ' . $resolvedDir + . ' (' . $reason . ')'; return; } diff --git a/source/packages/com_mokosuitebackup/src/Engine/RestoreEngine.php b/source/packages/com_mokosuitebackup/src/Engine/RestoreEngine.php index 071eadb..c95ae67 100644 --- a/source/packages/com_mokosuitebackup/src/Engine/RestoreEngine.php +++ b/source/packages/com_mokosuitebackup/src/Engine/RestoreEngine.php @@ -76,8 +76,9 @@ class RestoreEngine return ['success' => false, 'message' => 'Backup archive not found: ' . $archivePath]; } - // Create staging directory - $this->stagingDir = JPATH_ROOT . '/tmp/mokosuitebackup-restore-' . $record->tag; + // Create staging directory (sanitize tag to prevent path traversal) + $safeTag = preg_replace('/[^a-zA-Z0-9_-]/', '', $record->tag ?: 'restore'); + $this->stagingDir = JPATH_ROOT . '/tmp/mokosuitebackup-restore-' . $safeTag; if (is_dir($this->stagingDir)) { $this->recursiveDelete($this->stagingDir); diff --git a/source/packages/com_mokosuitebackup/src/Model/BackupModel.php b/source/packages/com_mokosuitebackup/src/Model/BackupModel.php index 93afa45..b1dd9fe 100644 --- a/source/packages/com_mokosuitebackup/src/Model/BackupModel.php +++ b/source/packages/com_mokosuitebackup/src/Model/BackupModel.php @@ -36,7 +36,7 @@ class BackupModel extends AdminModel $data = $this->getItem(); } - return $data; + return is_array($data) ? (object) $data : $data; } public function getTable($name = 'Backup', $prefix = 'Administrator', $options = []) diff --git a/source/packages/com_mokosuitebackup/src/Model/ProfileModel.php b/source/packages/com_mokosuitebackup/src/Model/ProfileModel.php index ee8d780..4d2b0aa 100644 --- a/source/packages/com_mokosuitebackup/src/Model/ProfileModel.php +++ b/source/packages/com_mokosuitebackup/src/Model/ProfileModel.php @@ -36,7 +36,7 @@ class ProfileModel extends AdminModel $data = $this->getItem(); } - return $data; + return is_array($data) ? (object) $data : $data; } public function getTable($name = 'Profile', $prefix = 'Administrator', $options = []) diff --git a/source/packages/com_mokosuitebackup/src/Table/BackupTable.php b/source/packages/com_mokosuitebackup/src/Table/BackupTable.php index 1117f4c..db090be 100644 --- a/source/packages/com_mokosuitebackup/src/Table/BackupTable.php +++ b/source/packages/com_mokosuitebackup/src/Table/BackupTable.php @@ -39,11 +39,22 @@ class BackupTable extends Table public function delete($pk = null): bool { - // Delete the archive file if it exists - if (!empty($this->absolute_path) && is_file($this->absolute_path)) { - @unlink($this->absolute_path); + $archivePath = $this->absolute_path; + + // Delete DB record first — if this fails, the file is preserved + $result = parent::delete($pk); + + if ($result && !empty($archivePath) && is_file($archivePath)) { + @unlink($archivePath); + + // Also remove the log file if it exists alongside the archive + $logPath = preg_replace('/\.(zip|tar\.gz)$/i', '.log', $archivePath); + + if (is_file($logPath)) { + @unlink($logPath); + } } - return parent::delete($pk); + return $result; } } diff --git a/source/packages/plg_console_mokosuitebackup/src/Command/RestoreCommand.php b/source/packages/plg_console_mokosuitebackup/src/Command/RestoreCommand.php index 7ed26d7..a6023e1 100644 --- a/source/packages/plg_console_mokosuitebackup/src/Command/RestoreCommand.php +++ b/source/packages/plg_console_mokosuitebackup/src/Command/RestoreCommand.php @@ -86,7 +86,7 @@ class RestoreCommand extends AbstractCommand } $engine = new RestoreEngine(); - $result = $engine->restore($record->absolute_path, $record->backup_type); + $result = $engine->restore($recordId); if ($result['success']) { $io->success($result['message']); diff --git a/source/packages/plg_system_mokosuitebackup/src/Extension/MokoSuiteBackup.php b/source/packages/plg_system_mokosuitebackup/src/Extension/MokoSuiteBackup.php index e3d00c8..d21a560 100644 --- a/source/packages/plg_system_mokosuitebackup/src/Extension/MokoSuiteBackup.php +++ b/source/packages/plg_system_mokosuitebackup/src/Extension/MokoSuiteBackup.php @@ -59,11 +59,15 @@ final class MokoSuiteBackup extends CMSPlugin implements SubscriberInterface // Reject if disabled or no secret configured if (!$enabled || $configSecret === '') { $this->sendJsonResponse(false, 'Web cron is not enabled', 403); + + return; } // Validate secret (timing-safe comparison) if (!hash_equals($configSecret, $secret)) { $this->sendJsonResponse(false, 'Invalid secret', 403); + + return; } // IP whitelist check (if configured) @@ -73,6 +77,8 @@ final class MokoSuiteBackup extends CMSPlugin implements SubscriberInterface if (!in_array($clientIp, $allowedIps, true)) { $this->sendJsonResponse(false, 'IP not allowed', 403); + + return; } } -- 2.52.0