From e72a007041a9eb362bdbb7059a72ce2328eb1672 Mon Sep 17 00:00:00 2001 From: Jonathan Miller Date: Sun, 7 Jun 2026 09:11:39 -0500 Subject: [PATCH] =?UTF-8?q?fix:=20address=20PR=20review=20=E2=80=94=20erro?= =?UTF-8?q?r=20logging,=20ACL=20check,=20fetch=20error=20handling?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Log failures in protectBackupDir() and protectWebAccessibleDir() instead of silently suppressing with @ (security-critical .htaccess writes) - Add error_log() to empty catch blocks in boot() and syncMenuIcons() - Add core.manage ACL check to checkDir() AJAX endpoint - Surface opendir() failures in browseDir() with warning message - Add HTTP status check (r.ok) to JS fetch calls before parsing JSON - Log temp SQL file deletion failures in SteppedBackupEngine --- .../src/Controller/AjaxController.php | 19 +++++++++++++++++-- .../src/Engine/BackupEngine.php | 8 ++++++-- .../src/Engine/SteppedBackupEngine.php | 12 ++++++++---- .../src/Extension/MokoJoomBackupComponent.php | 2 +- .../src/Field/FolderPickerField.php | 4 ++-- .../src/Table/ProfileTable.php | 8 ++++++-- source/script.php | 2 +- 7 files changed, 41 insertions(+), 14 deletions(-) diff --git a/source/packages/com_mokojoombackup/src/Controller/AjaxController.php b/source/packages/com_mokojoombackup/src/Controller/AjaxController.php index 87f6656..72259c6 100644 --- a/source/packages/com_mokojoombackup/src/Controller/AjaxController.php +++ b/source/packages/com_mokojoombackup/src/Controller/AjaxController.php @@ -109,6 +109,7 @@ class AjaxController extends BaseController // that could contain a backup folder (e.g., /home/user/backups) $dirs = []; $handle = @opendir($path); + $warning = null; if ($handle) { while (($entry = readdir($handle)) !== false) { @@ -127,18 +128,26 @@ class AjaxController extends BaseController } closedir($handle); + } else { + $warning = 'Cannot read directory contents (check permissions)'; } usort($dirs, fn($a, $b) => strcasecmp($a['name'], $b['name'])); $parent = dirname($path); - $this->sendJson([ + $response = [ 'error' => false, 'current' => $path, 'parent' => ($parent !== $path) ? $parent : null, 'dirs' => $dirs, - ]); + ]; + + if ($warning !== null) { + $response['warning'] = $warning; + } + + $this->sendJson($response); } /** @@ -205,6 +214,12 @@ class AjaxController extends BaseController return; } + if (!$this->app->getIdentity()->authorise('core.manage', 'com_mokojoombackup')) { + $this->sendJson(['error' => true, 'message' => 'Access denied']); + + return; + } + $rawPath = trim($this->input->getString('path', '')); if ($rawPath === '') { diff --git a/source/packages/com_mokojoombackup/src/Engine/BackupEngine.php b/source/packages/com_mokojoombackup/src/Engine/BackupEngine.php index 37f22cf..58a6089 100644 --- a/source/packages/com_mokojoombackup/src/Engine/BackupEngine.php +++ b/source/packages/com_mokojoombackup/src/Engine/BackupEngine.php @@ -530,13 +530,17 @@ class BackupEngine $htaccess = $dir . '/.htaccess'; if (!is_file($htaccess)) { - @file_put_contents($htaccess, "Order deny,allow\nDeny from all\n"); + if (@file_put_contents($htaccess, "Order deny,allow\nDeny from all\n") === false) { + error_log('MokoJoomBackup: Could not create .htaccess in backup directory: ' . $dir); + } } $index = $dir . '/index.html'; if (!is_file($index)) { - @file_put_contents($index, ''); + if (@file_put_contents($index, '') === false) { + error_log('MokoJoomBackup: Could not create index.html in backup directory: ' . $dir); + } } } diff --git a/source/packages/com_mokojoombackup/src/Engine/SteppedBackupEngine.php b/source/packages/com_mokojoombackup/src/Engine/SteppedBackupEngine.php index 582b3b7..0eae1aa 100644 --- a/source/packages/com_mokojoombackup/src/Engine/SteppedBackupEngine.php +++ b/source/packages/com_mokojoombackup/src/Engine/SteppedBackupEngine.php @@ -317,8 +317,8 @@ class SteppedBackupEngine $zip->close(); // Clean up temp SQL file - if (is_file($sqlFile)) { - @unlink($sqlFile); + if (is_file($sqlFile) && !@unlink($sqlFile)) { + error_log('MokoJoomBackup: Could not delete temp SQL file: ' . $sqlFile); } $totalSize = file_exists($session->archivePath) ? filesize($session->archivePath) : 0; @@ -572,13 +572,17 @@ class SteppedBackupEngine $htaccess = $dir . '/.htaccess'; if (!is_file($htaccess)) { - @file_put_contents($htaccess, "Order deny,allow\nDeny from all\n"); + if (@file_put_contents($htaccess, "Order deny,allow\nDeny from all\n") === false) { + error_log('MokoJoomBackup: Could not create .htaccess in backup directory: ' . $dir); + } } $index = $dir . '/index.html'; if (!is_file($index)) { - @file_put_contents($index, ''); + if (@file_put_contents($index, '') === false) { + error_log('MokoJoomBackup: Could not create index.html in backup directory: ' . $dir); + } } } diff --git a/source/packages/com_mokojoombackup/src/Extension/MokoJoomBackupComponent.php b/source/packages/com_mokojoombackup/src/Extension/MokoJoomBackupComponent.php index 0575936..10cd7f0 100644 --- a/source/packages/com_mokojoombackup/src/Extension/MokoJoomBackupComponent.php +++ b/source/packages/com_mokojoombackup/src/Extension/MokoJoomBackupComponent.php @@ -39,7 +39,7 @@ class MokoJoomBackupComponent extends MVCComponent . ' #menu a[href*="com_mokojoombackup"][href*="view=profiles"] .sidebar-item-title::before { content: "\f013"; }' ); } catch (\Throwable $e) { - // Non-critical + error_log('MokoJoomBackup: boot() CSS injection failed: ' . $e->getMessage()); } } } diff --git a/source/packages/com_mokojoombackup/src/Field/FolderPickerField.php b/source/packages/com_mokojoombackup/src/Field/FolderPickerField.php index 7a9e801..17bcaee 100644 --- a/source/packages/com_mokojoombackup/src/Field/FolderPickerField.php +++ b/source/packages/com_mokojoombackup/src/Field/FolderPickerField.php @@ -183,7 +183,7 @@ class FolderPickerField extends FormField body: form, headers: { 'X-Requested-With': 'XMLHttpRequest' } }) - .then(function(r) { return r.json(); }) + .then(function(r) { if (!r.ok) throw new Error('Server error (HTTP ' + r.status + ')'); return r.json(); }) .then(function(data) { if (data.error) { setStatus('text-danger', 'icon-unpublish', data.message || 'Error', null); @@ -241,7 +241,7 @@ class FolderPickerField extends FormField body: form, headers: { 'X-Requested-With': 'XMLHttpRequest' } }) - .then(function(r) { return r.json(); }) + .then(function(r) { if (!r.ok) throw new Error('Server error (HTTP ' + r.status + ')'); return r.json(); }) .then(function(data) { if (data.error) { tree.textContent = data.message || 'Error loading directory'; diff --git a/source/packages/com_mokojoombackup/src/Table/ProfileTable.php b/source/packages/com_mokojoombackup/src/Table/ProfileTable.php index 218c73b..b712670 100644 --- a/source/packages/com_mokojoombackup/src/Table/ProfileTable.php +++ b/source/packages/com_mokojoombackup/src/Table/ProfileTable.php @@ -65,13 +65,17 @@ class ProfileTable extends Table $htaccess = $resolved . '/.htaccess'; if (!is_file($htaccess)) { - @file_put_contents($htaccess, "Order deny,allow\nDeny from all\n"); + if (@file_put_contents($htaccess, "Order deny,allow\nDeny from all\n") === false) { + error_log('MokoJoomBackup: Could not create .htaccess in: ' . $resolved); + } } $index = $resolved . '/index.html'; if (!is_file($index)) { - @file_put_contents($index, ''); + if (@file_put_contents($index, '') === false) { + error_log('MokoJoomBackup: Could not create index.html in: ' . $resolved); + } } } } diff --git a/source/script.php b/source/script.php index ac511e1..18865d7 100644 --- a/source/script.php +++ b/source/script.php @@ -243,7 +243,7 @@ class Pkg_MokoJoomBackupInstallerScript $db->setQuery($query); $db->execute(); } catch (\Throwable $e) { - // Non-critical + error_log('MokoJoomBackup: syncMenuIcons() failed: ' . $e->getMessage()); } }