fix: Critical and high audit findings (#81) #83

Merged
jmiller merged 1 commits from fix/audit-critical-high into main 2026-06-21 23:10:20 +00:00
9 changed files with 94 additions and 20 deletions
@@ -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,
];
}
@@ -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)) {
@@ -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;
}
@@ -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);
@@ -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 = [])
@@ -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 = [])
@@ -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;
}
}
@@ -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']);
@@ -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;
}
}