fix: address all PR review findings — error handling, security, validation

Security:
- browseDir restricted to JPATH_ROOT and current user $HOME (not all /home/)
- MokoRestore db_prefix validated with regex to prevent SQL injection
- MokoRestore DB import returns failure when zero statements succeed

Error handling (fatal — would produce corrupt backups):
- BackupEngine/SteppedEngine mkdir() checked, returns error on failure
- SteppedSession save() checked, throws on write failure
- SteppedEngine SQL dump file_put_contents checked, throws on failure
- MokoRestore configuration.php write checked, throws on failure

Error handling (logged — secondary operations):
- BackupEngine dispatchAfterRun catch block logs to error_log
- BackupEngine/SteppedEngine log file write failures logged
- NotificationSender user group email resolution logged
- script.php download key save/restore logged

Operational fixes:
- Cleanup plugin: don't delete DB record if file unlink fails (prevents orphans)
- BackupEngine: count and log skipped unreadable files
- BackupEngine: handle MokoRestore rename failure gracefully
- SteppedEngine: add S3Uploader to stepUpload match (feature parity)

Authored-by: Moko Consulting
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

source/packages/com_mokojoombackup/src/Controller/AjaxController.php

@@ -80,8 +80,24 @@ class AjaxController extends BaseController
 			return;
 		}
 
-		$path = $this->input->getString('path', JPATH_ROOT);
-		$path = realpath($path) ?: $path;
+		$requestPath = $this->input->getString('path', JPATH_ROOT);
+		$path = realpath($requestPath) ?: $requestPath;
+
+		// Security: restrict browsing to site root and current user's home
+		$jRoot   = realpath(JPATH_ROOT);
+		$homeDir = getenv('HOME') ?: (getenv('USERPROFILE') ?: '');
+		$allowed = false;
+
+		if ($jRoot !== false && strpos($path, $jRoot) === 0) {
+			$allowed = true;
+		} elseif ($homeDir !== '' && strpos($path, $homeDir) === 0) {
+			$allowed = true;
+		}
+
+		if (!$allowed) {
+			$this->sendJson(['error' => true, 'message' => 'Access denied: path outside allowed directories']);
+			return;
+		}
 
 		if (!is_dir($path)) {
 			$this->sendJson(['error' => true, 'message' => 'Directory not found: ' . $path]);

source/packages/com_mokojoombackup/src/Engine/BackupEngine.php

@@ -67,7 +67,9 @@ class BackupEngine
 		$this->backupDir = $this->resolveBackupDir($resolver->resolve($configuredDir));
 
 		if (!is_dir($this->backupDir)) {
-			mkdir($this->backupDir, 0755, true);
+			if (!mkdir($this->backupDir, 0755, true)) {
+				return ['success' => false, 'message' => 'Cannot create backup directory: ' . $this->backupDir, 'record_id' => 0];
+			}
 		}
 
 		// Create backup record
@@ -155,14 +157,22 @@ class BackupEngine
 				$filesCount = count($filesToBackup);
 				$this->log('Backing up ' . $filesCount . ' files');
 
+				$skippedFiles = 0;
+
 				foreach ($filesToBackup as $relativePath) {
 					$fullPath = JPATH_ROOT . '/' . $relativePath;
 
 					if (is_file($fullPath) && is_readable($fullPath)) {
 						$archiver->addFile($fullPath, $relativePath);
+					} else {
+						$skippedFiles++;
 					}
 				}
 
+				if ($skippedFiles > 0) {
+					$this->log('WARNING: ' . $skippedFiles . ' files skipped (not readable or missing)');
+				}
+
 				$this->log('Files added to archive');
 
 				// Build manifest for full/differential backups (used by future differentials)
@@ -239,7 +249,9 @@ class BackupEngine
 			// Write log file alongside the archive
 			$logContent = implode("\n", $this->log);
 			$logPath    = preg_replace('/\.(zip|tar\.gz)$/i', '.log', $archivePath);
-			@file_put_contents($logPath, $logContent);
+			if (@file_put_contents($logPath, $logContent) === false) {
+				error_log('MokoJoomBackup: Could not write log file: ' . $logPath);
+			}
 
 			// Final record update
 			$update = (object) [
@@ -493,7 +505,8 @@ class BackupEngine
 
 			$app->getDispatcher()->dispatch('onMokoJoomBackupAfterRun', $event);
 		} catch (\Throwable $e) {
-			// Never let a listener failure break the backup result
+			// Never let a listener failure break the backup result, but log it
+			error_log('MokoJoomBackup: onAfterRun listener error: ' . $e->getMessage());
 		}
 	}
 

source/packages/com_mokojoombackup/src/Engine/MokoRestore.php

@@ -373,7 +373,7 @@ function actionDatabase(array $data): array
     $pdo->exec('SET FOREIGN_KEY_CHECKS = 1');
 
     return [
-        'success'    => true,
+        'success'    => ($statements > 0 || $errors === 0),
         'message'    => "Executed {$statements} statements" . ($errors ? " ({$errors} warnings)" : ''),
         'statements' => $statements,
         'errors'     => $errors,
@@ -625,9 +625,20 @@ 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)) {
+        throw new RuntimeException('Invalid table prefix format');
+    }
 
     return new PDO(
         "mysql:host={$host};dbname={$name};charset=utf8mb4",

source/packages/com_mokojoombackup/src/Engine/NotificationSender.php

@@ -172,6 +172,8 @@ class NotificationSender
 
 			return $db->loadColumn() ?: [];
 		} catch (\Throwable $e) {
+			error_log('MokoJoomBackup: Could not resolve user group emails: ' . $e->getMessage());
+
 			return [];
 		}
 	}

source/packages/com_mokojoombackup/src/Engine/SteppedBackupEngine.php

@@ -65,7 +65,9 @@ class SteppedBackupEngine
 		$backupDir = $this->resolveBackupDir($resolver->resolve($session->backupDir));
 
 		if (!is_dir($backupDir)) {
-			mkdir($backupDir, 0755, true);
+			if (!mkdir($backupDir, 0755, true)) {
+				return ['error' => true, 'message' => 'Cannot create backup directory: ' . $backupDir];
+			}
 		}
 
 		$now         = date('Y-m-d H:i:s');
@@ -232,11 +234,15 @@ class SteppedBackupEngine
 				. "-- Prefix: " . $db->getPrefix() . "\n\n"
 				. "SET SQL_MODE = \"NO_AUTO_VALUE_ON_ZERO\";\n"
 				. "SET time_zone = \"+00:00\";\n\n";
-			file_put_contents($sqlFile, $header);
+			if (file_put_contents($sqlFile, $header) === false) {
+			throw new \RuntimeException('Cannot write SQL dump: ' . $sqlFile);
+		}
 			$flags = FILE_APPEND;
 		}
 
-		file_put_contents($sqlFile, $sql, $flags);
+		if (file_put_contents($sqlFile, $sql, $flags) === false) {
+			throw new \RuntimeException('Cannot write SQL dump: ' . $sqlFile);
+		}
 		$session->dbSize += strlen($sql);
 
 		$session->tableIndex++;
@@ -369,6 +375,7 @@ class SteppedBackupEngine
 		$uploader = match ($session->remoteStorage) {
 			'ftp'          => new FtpUploader($profile),
 			'google_drive' => new GoogleDriveUploader($profile),
+			's3'           => new S3Uploader($profile),
 			default        => throw new \InvalidArgumentException('Unknown storage: ' . $session->remoteStorage),
 		};
 
@@ -414,7 +421,9 @@ class SteppedBackupEngine
 
 		// Write log file alongside the archive
 		$logPath = preg_replace('/\.(zip|tar\.gz)$/i', '.log', $session->archivePath);
-		@file_put_contents($logPath, $logContent);
+		if (@file_put_contents($logPath, $logContent) === false) {
+			error_log('MokoJoomBackup: Could not write log file: ' . $logPath);
+		}
 
 		$update = (object) [
 			'id'        => $session->recordId,

source/packages/com_mokojoombackup/src/Engine/SteppedSession.php

@@ -65,7 +65,9 @@ class SteppedSession
 		$dir = JPATH_ROOT . '/tmp/mokojoombackup-sessions';
 
 		if (!is_dir($dir)) {
-			mkdir($dir, 0755, true);
+			if (!mkdir($dir, 0755, true)) {
+				throw new \RuntimeException('Cannot create session directory: ' . $dir);
+			}
 		}
 
 		return $dir;
@@ -124,7 +126,9 @@ class SteppedSession
 	public function save(): void
 	{
 		$path = self::getSessionPath($this->sessionId);
-		file_put_contents($path, json_encode(get_object_vars($this), JSON_PRETTY_PRINT));
+		if (file_put_contents($path, json_encode(get_object_vars($this), JSON_PRETTY_PRINT)) === false) {
+			throw new \RuntimeException('Cannot save backup session: ' . $path);
+		}
 	}
 
 	/**

source/packages/plg_system_mokojoombackup/src/Extension/MokoJoomBackup.php

@@ -151,7 +151,9 @@ final class MokoJoomBackup extends CMSPlugin implements SubscriberInterface
 
 		foreach ($expired as $record) {
 			if (!empty($record->absolute_path) && is_file($record->absolute_path)) {
-				@unlink($record->absolute_path);
+				if (!@unlink($record->absolute_path)) {
+					continue; // Don't delete DB record if file can't be removed
+				}
 			}
 
 			$db->setQuery(
@@ -182,7 +184,9 @@ final class MokoJoomBackup extends CMSPlugin implements SubscriberInterface
 
 			foreach ($oldest as $record) {
 				if (!empty($record->absolute_path) && is_file($record->absolute_path)) {
-					@unlink($record->absolute_path);
+					if (!@unlink($record->absolute_path)) {
+					continue; // Do not delete DB record if file cannot be removed
+				}
 				}
 
 				$db->setQuery(

source/script.php

@@ -101,7 +101,7 @@ class Pkg_MokoJoomBackupInstallerScript
 				$this->savedDownloadKey = $key;
 			}
 		} catch (\Throwable $e) {
-			// Not critical
+			error_log('MokoJoomBackup: Could not save download key: ' . $e->getMessage());
 		}
 	}
 
@@ -242,7 +242,7 @@ class Pkg_MokoJoomBackupInstallerScript
 				$db->execute();
 			}
 		} catch (\Throwable $e) {
-			// Not critical
+			error_log('MokoJoomBackup: Could not restore download key: ' . $e->getMessage());
 		}
 	}
 
@@ -278,6 +278,8 @@ class Pkg_MokoJoomBackupInstallerScript
 				'warning'
 			);
 		}
-		catch (\Throwable $e) {}
+		catch (\Throwable $e) {
+			error_log('MokoJoomBackup: License key check failed: ' . $e->getMessage());
+		}
 	}
 }