From c466839a40000446ceab278fbe5fa615fe1404bf Mon Sep 17 00:00:00 2001 From: Jonathan Miller Date: Mon, 15 Jun 2026 01:10:01 -0500 Subject: [PATCH] =?UTF-8?q?fix:=20final=20review=20=E2=80=94=20SQL=20injec?= =?UTF-8?q?tion,=20input=20escaping,=20undefined=20var?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Critical/High: - Fix undefined $configFile → $configPath in from-scratch config path - Escape all user input with addcslashes before interpolating into configuration.php (both regex-replace and HEREDOC paths) - Add getValidatedPrefix() helper — validates db_prefix format before use in SQL table names across all restore functions - fixPackageClientId() now warns user via enqueueMessage on failure - sanitizeConfiguration() logs error on file read failure Medium: - Content-Disposition header uses RFC 6266 rawurlencode (both admin and API download controllers) - Remove @unlink suppression, log warning on failure - viewLog() catch block now logs exception context - writeDefaultHtaccess() checks copy/write, returns status to caller - actionConfig() checks file_put_contents return value --- .../api/src/Controller/BackupsController.php | 2 +- .../src/Controller/BackupsController.php | 2 +- .../src/Engine/MokoRestore.php | 82 ++++++++++++------- 3 files changed, 56 insertions(+), 30 deletions(-) diff --git a/source/packages/com_mokosuitebackup/api/src/Controller/BackupsController.php b/source/packages/com_mokosuitebackup/api/src/Controller/BackupsController.php index b6d4c4a..a471b48 100644 --- a/source/packages/com_mokosuitebackup/api/src/Controller/BackupsController.php +++ b/source/packages/com_mokosuitebackup/api/src/Controller/BackupsController.php @@ -92,7 +92,7 @@ class BackupsController extends ApiController : 'application/zip'; header('Content-Type: ' . $contentType); - header('Content-Disposition: attachment; filename="' . $filename . '"'); + header("Content-Disposition: attachment; filename*=UTF-8''" . rawurlencode($filename)); header('Content-Length: ' . $filesize); header('Cache-Control: no-cache, must-revalidate'); diff --git a/source/packages/com_mokosuitebackup/src/Controller/BackupsController.php b/source/packages/com_mokosuitebackup/src/Controller/BackupsController.php index b937609..a15527e 100644 --- a/source/packages/com_mokosuitebackup/src/Controller/BackupsController.php +++ b/source/packages/com_mokosuitebackup/src/Controller/BackupsController.php @@ -99,7 +99,7 @@ class BackupsController extends AdminController : 'application/zip'; header('Content-Type: ' . $contentType); - header('Content-Disposition: attachment; filename="' . $filename . '"'); + header("Content-Disposition: attachment; filename*=UTF-8''" . rawurlencode($filename)); header('Content-Length: ' . $filesize); header('Cache-Control: no-cache, must-revalidate'); header('Pragma: no-cache'); diff --git a/source/packages/com_mokosuitebackup/src/Engine/MokoRestore.php b/source/packages/com_mokosuitebackup/src/Engine/MokoRestore.php index 6401fa2..30f1944 100644 --- a/source/packages/com_mokosuitebackup/src/Engine/MokoRestore.php +++ b/source/packages/com_mokosuitebackup/src/Engine/MokoRestore.php @@ -411,26 +411,38 @@ function actionConfig(array $data): array $config = file_get_contents($basePath); // Replace all credential and server-specific fields with user input + // Escape all user input for safe interpolation into PHP string literals + $eHost = addcslashes($host, "'\\"); + $eDbName = addcslashes($dbName, "'\\"); + $eDbUser = addcslashes($dbUser, "'\\"); + $eDbPass = addcslashes($dbPass, "'\\"); + $ePrefix = addcslashes($prefix, "'\\"); + $eSite = addcslashes($sitename, "'\\"); + $eLive = addcslashes($livesite, "'\\"); + $eSmtpH = addcslashes($smtpHost, "'\\"); + $eSmtpU = addcslashes($smtpUser, "'\\"); + $eSmtpP = addcslashes($smtpPass, "'\\"); + $replacements = [ - '/\$host\s*=\s*\'[^\']*\'/' => "\$host = '{$host}'", - '/\$db\s*=\s*\'[^\']*\'/' => "\$db = '{$dbName}'", - '/\$user\s*=\s*\'[^\']*\'/' => "\$user = '{$dbUser}'", - '/\$password\s*=\s*\'[^\']*\'/' => "\$password = '" . addcslashes($dbPass, "'\\") . "'", - '/\$dbprefix\s*=\s*\'[^\']*\'/' => "\$dbprefix = '{$prefix}'", + '/\$host\s*=\s*\'[^\']*\'/' => "\$host = '{$eHost}'", + '/\$db\s*=\s*\'[^\']*\'/' => "\$db = '{$eDbName}'", + '/\$user\s*=\s*\'[^\']*\'/' => "\$user = '{$eDbUser}'", + '/\$password\s*=\s*\'[^\']*\'/' => "\$password = '{$eDbPass}'", + '/\$dbprefix\s*=\s*\'[^\']*\'/' => "\$dbprefix = '{$ePrefix}'", '/\$tmp_path\s*=\s*\'[^\']*\'/' => "\$tmp_path = '{$tmpPath}'", '/\$log_path\s*=\s*\'[^\']*\'/' => "\$log_path = '{$logPath}'", - '/\$sitename\s*=\s*\'[^\']*\'/' => "\$sitename = '" . addcslashes($sitename, "'\\") . "'", + '/\$sitename\s*=\s*\'[^\']*\'/' => "\$sitename = '{$eSite}'", '/\$secret\s*=\s*\'[^\']*\'/' => "\$secret = '" . bin2hex(random_bytes(16)) . "'", ]; if ($livesite !== '') { - $replacements['/\$live_site\s*=\s*\'[^\']*\'/'] = "\$live_site = '{$livesite}'"; + $replacements['/\$live_site\s*=\s*\'[^\']*\'/'] = "\$live_site = '{$eLive}'"; } // SMTP — always replace (clears sanitized placeholders even if blank) - $replacements['/\$smtphost\s*=\s*\'[^\']*\'/'] = "\$smtphost = '" . addcslashes($smtpHost, "'\\") . "'"; - $replacements['/\$smtpuser\s*=\s*\'[^\']*\'/'] = "\$smtpuser = '" . addcslashes($smtpUser, "'\\") . "'"; - $replacements['/\$smtppass\s*=\s*\'[^\']*\'/'] = "\$smtppass = '" . addcslashes($smtpPass, "'\\") . "'"; + $replacements['/\$smtphost\s*=\s*\'[^\']*\'/'] = "\$smtphost = '{$eSmtpH}'"; + $replacements['/\$smtpuser\s*=\s*\'[^\']*\'/'] = "\$smtpuser = '{$eSmtpU}'"; + $replacements['/\$smtppass\s*=\s*\'[^\']*\'/'] = "\$smtppass = '{$eSmtpP}'"; // Clear remaining sanitized placeholders (proxy, Redis, DB TLS) $replacements['/\$proxy_user\s*=\s*\'[^\']*\'/'] = "\$proxy_user = ''"; @@ -470,8 +482,15 @@ function actionConfig(array $data): array return ['success' => true, 'message' => $msg]; } - // Create new configuration.php from scratch - $secret = bin2hex(random_bytes(16)); + // Create new configuration.php from scratch — use escaped values + $eHost = addcslashes($host, "'\\"); + $eDbName = addcslashes($dbName, "'\\"); + $eDbUser = addcslashes($dbUser, "'\\"); + $eDbPass = addcslashes($dbPass, "'\\"); + $ePrefix = addcslashes($prefix, "'\\"); + $eSite = addcslashes($sitename, "'\\"); + $eLive = addcslashes($livesite, "'\\"); + $secret = bin2hex(random_bytes(16)); $newConfig = << false, 'message' => 'Failed to write Joomla config file — check directory permissions']; } @@ -654,10 +673,21 @@ HTACCESS; return ''; } +function getValidatedPrefix(array $data): string +{ + $prefix = getValidatedPrefix($data); + + if (!preg_match('/^[a-zA-Z][a-zA-Z0-9_]{0,20}$/', $prefix)) { + throw new RuntimeException('Invalid table prefix format'); + } + + return $prefix; +} + function actionListAdmins(array $data): array { $pdo = getDbConnection($data); - $prefix = $data['db_prefix'] ?? 'moko_'; + $prefix = getValidatedPrefix($data); // Find super admin users (group 8 = Super Users in Joomla) $stmt = $pdo->prepare( @@ -703,7 +733,7 @@ function actionResetAdmin(array $data): array function actionProvision(array $data): array { $pdo = getDbConnection($data); - $prefix = $data['db_prefix'] ?? 'moko_'; + $prefix = getValidatedPrefix($data); $tasks = json_decode($data['tasks'] ?? '[]', true) ?: []; $results = []; @@ -806,12 +836,8 @@ function getDbConnection(array $data): PDO $user = $data['db_user'] ?? ''; $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 (used by callers for table names) + getValidatedPrefix($data); return new PDO( "mysql:host={$host};dbname={$name};charset=utf8mb4",