MokoConsulting/MokoSuiteClient
0
0
Fork 0
Code Issues 110 Pull Requests Releases 1 Wiki Activity

fix(security+reliability): address PR review — ACL guards, error logging, path traversal
Generic: Repo Health / Access control (push) Successful in 2s
Details
Generic: Repo Health / Site Health (push) Has been skipped
Details
Universal: Auto Version Bump / Version Bump (push) Successful in 9s
Details
Generic: Repo Health / Scripts governance (push) Has been cancelled
Details
Generic: Repo Health / Repository health (push) Has been cancelled
Details
Generic: Repo Health / Report Issues (push) Has been cancelled
Details

Browse Source 
Security:
- Add return after all jsonForbidden() calls (13 methods) to prevent
  ACL bypass if $app->close() fails to terminate
- Add throw after requireAuth() in REST API controller (same pattern)
- Add path traversal guard to AttachmentService::getAbsolutePath()
  using realpath + prefix check

Error handling:
- Log install notification email failures instead of empty catch
- Log DB errors in getUserEmail(), getNotificationConfig(),
  getComponentConfig() instead of silent fallbacks
- Log PHP upload error codes in AttachmentService
- Check Folder::create() return value before upload loop
- Fix searchKb() missing return on short query + log DB errors
- Fix ntfy push to capture curl_error() on connection failure
- Upgrade AutomationEngine inner catch to LOG_ERROR with rule ID
This commit is contained in:
Jonathan Miller
2026-06-18 19:05:31 -05:00
parent ccb76132a5
commit 134b9b3693
7 changed files with 40 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files
source/packages/com_mokosuiteclient/admin/src/Controller/DisplayController.php
+17 -15
View File
@@ -500,6 +500,7 @@ class DisplayController extends BaseController
if (strlen($query) < 3)
{
$this->jsonResponse(['results' => []]);
return;
}
try
@@ -527,7 +528,8 @@ class DisplayController extends BaseController
}
catch (\Throwable $e)
{
$this->jsonResponse(['results' => []]);
Log::add('KB search failed: ' . $e->getMessage(), Log::ERROR, 'mokosuiteclient');
$this->jsonResponse(['results' => [], 'error' => 'Search unavailable']);
}
}
@@ -575,7 +577,7 @@ class DisplayController extends BaseController
public function saveCategory()
{
Session::checkToken() or die(Text::_('JINVALID_TOKEN'));
if (!$this->checkAcl('mokosuiteclient.tickets')) { $this->jsonForbidden(); }
if (!$this->checkAcl('mokosuiteclient.tickets')) { $this->jsonForbidden(); return; }
$input = Factory::getApplication()->getInput();
$db = Factory::getDbo();
$id = $input->getInt('id', 0);
@@ -600,7 +602,7 @@ class DisplayController extends BaseController
public function deleteCategory()
{
Session::checkToken() or die(Text::_('JINVALID_TOKEN'));
if (!$this->checkAcl('mokosuiteclient.tickets')) { $this->jsonForbidden(); }
if (!$this->checkAcl('mokosuiteclient.tickets')) { $this->jsonForbidden(); return; }
$db = Factory::getDbo();
$db->setQuery($db->getQuery(true)->delete('#__mokosuiteclient_ticket_categories')->where('id = ' . Factory::getApplication()->getInput()->getInt('id', 0)))->execute();
$this->jsonResponse(['success' => true, 'message' => 'Category deleted.']);
@@ -609,7 +611,7 @@ class DisplayController extends BaseController
public function reorderCategory()
{
Session::checkToken() or die(Text::_('JINVALID_TOKEN'));
if (!$this->checkAcl('mokosuiteclient.tickets')) { $this->jsonForbidden(); }
if (!$this->checkAcl('mokosuiteclient.tickets')) { $this->jsonForbidden(); return; }
$order = json_decode(Factory::getApplication()->getInput()->getRaw('order', '[]'), true);
if (!is_array($order)) { $this->jsonResponse(['success' => false, 'message' => 'Invalid order']); return; }
$db = Factory::getDbo();
@@ -622,7 +624,7 @@ class DisplayController extends BaseController
public function saveCanned()
{
Session::checkToken() or die(Text::_('JINVALID_TOKEN'));
if (!$this->checkAcl('mokosuiteclient.tickets')) { $this->jsonForbidden(); }
if (!$this->checkAcl('mokosuiteclient.tickets')) { $this->jsonForbidden(); return; }
$input = Factory::getApplication()->getInput();
$db = Factory::getDbo();
$data = (object) [
@@ -640,7 +642,7 @@ class DisplayController extends BaseController
public function deleteCanned()
{
Session::checkToken() or die(Text::_('JINVALID_TOKEN'));
if (!$this->checkAcl('mokosuiteclient.tickets')) { $this->jsonForbidden(); }
if (!$this->checkAcl('mokosuiteclient.tickets')) { $this->jsonForbidden(); return; }
$db = Factory::getDbo();
$db->setQuery($db->getQuery(true)->delete('#__mokosuiteclient_ticket_canned')->where('id = ' . Factory::getApplication()->getInput()->getInt('id', 0)))->execute();
$this->jsonResponse(['success' => true, 'message' => 'Canned response deleted.']);
@@ -649,7 +651,7 @@ class DisplayController extends BaseController
public function reorderCanned()
{
Session::checkToken() or die(Text::_('JINVALID_TOKEN'));
if (!$this->checkAcl('mokosuiteclient.tickets')) { $this->jsonForbidden(); }
if (!$this->checkAcl('mokosuiteclient.tickets')) { $this->jsonForbidden(); return; }
$order = json_decode(Factory::getApplication()->getInput()->getRaw('order', '[]'), true);
if (!is_array($order)) { $this->jsonResponse(['success' => false, 'message' => 'Invalid order']); return; }
$db = Factory::getDbo();
@@ -662,7 +664,7 @@ class DisplayController extends BaseController
public function uploadAttachment()
{
Session::checkToken() or die(Text::_('JINVALID_TOKEN'));
if (!$this->checkAcl('mokosuiteclient.tickets')) { $this->jsonForbidden(); }
if (!$this->checkAcl('mokosuiteclient.tickets')) { $this->jsonForbidden(); return; }
$input = Factory::getApplication()->getInput();
$ticketId = $input->getInt('ticket_id', 0);
$replyId = $input->getInt('reply_id', 0) ?: null;
@@ -675,7 +677,7 @@ class DisplayController extends BaseController
public function downloadAttachment()
{
if (!$this->checkAcl('mokosuiteclient.tickets')) { $this->jsonForbidden(); }
if (!$this->checkAcl('mokosuiteclient.tickets')) { $this->jsonForbidden(); return; }
$id = Factory::getApplication()->getInput()->getInt('id', 0);
$db = Factory::getDbo();
$db->setQuery($db->getQuery(true)->select('*')->from('#__mokosuiteclient_ticket_attachments')->where('id = ' . $id));
@@ -696,7 +698,7 @@ class DisplayController extends BaseController
public function deleteAttachment()
{
Session::checkToken() or die(Text::_('JINVALID_TOKEN'));
if (!$this->checkAcl('mokosuiteclient.tickets')) { $this->jsonForbidden(); }
if (!$this->checkAcl('mokosuiteclient.tickets')) { $this->jsonForbidden(); return; }
$id = Factory::getApplication()->getInput()->getInt('id', 0);
$ok = \Moko\Component\MokoSuiteClient\Administrator\Service\AttachmentService::delete($id);
$this->jsonResponse(['success' => $ok, 'message' => $ok ? 'Attachment deleted' : 'Not found']);
@@ -705,7 +707,7 @@ class DisplayController extends BaseController
public function rateTicket()
{
Session::checkToken() or die(Text::_('JINVALID_TOKEN'));
if (!$this->checkAcl('mokosuiteclient.tickets')) { $this->jsonForbidden(); }
if (!$this->checkAcl('mokosuiteclient.tickets')) { $this->jsonForbidden(); return; }
$input = Factory::getApplication()->getInput();
$ticketId = $input->getInt('ticket_id', 0);
$rating = $input->getInt('rating', 0);
@@ -728,7 +730,7 @@ class DisplayController extends BaseController
public function saveAutomation()
{
Session::checkToken() or die(Text::_('JINVALID_TOKEN'));
if (!$this->checkAcl('core.admin')) { $this->jsonForbidden(); }
if (!$this->checkAcl('core.admin')) { $this->jsonForbidden(); return; }
$input = Factory::getApplication()->getInput();
$db = Factory::getDbo();
$data = (object) [
@@ -749,7 +751,7 @@ class DisplayController extends BaseController
public function deleteAutomation()
{
Session::checkToken() or die(Text::_('JINVALID_TOKEN'));
if (!$this->checkAcl('core.admin')) { $this->jsonForbidden(); }
if (!$this->checkAcl('core.admin')) { $this->jsonForbidden(); return; }
$db = Factory::getDbo();
$db->setQuery($db->getQuery(true)->delete('#__mokosuiteclient_ticket_automation')->where('id = ' . Factory::getApplication()->getInput()->getInt('id', 0)))->execute();
$this->jsonResponse(['success' => true, 'message' => 'Rule deleted.']);
@@ -758,7 +760,7 @@ class DisplayController extends BaseController
public function toggleAutomation()
{
Session::checkToken() or die(Text::_('JINVALID_TOKEN'));
if (!$this->checkAcl('core.admin')) { $this->jsonForbidden(); }
if (!$this->checkAcl('core.admin')) { $this->jsonForbidden(); return; }
$input = Factory::getApplication()->getInput();
$db = Factory::getDbo();
$db->setQuery($db->getQuery(true)->update('#__mokosuiteclient_ticket_automation')
@@ -770,7 +772,7 @@ class DisplayController extends BaseController
public function reorderAutomation()
{
Session::checkToken() or die(Text::_('JINVALID_TOKEN'));
if (!$this->checkAcl('core.admin')) { $this->jsonForbidden(); }
if (!$this->checkAcl('core.admin')) { $this->jsonForbidden(); return; }
$order = json_decode(Factory::getApplication()->getInput()->getRaw('order', '[]'), true);
if (!is_array($order)) { $this->jsonResponse(['success' => false, 'message' => 'Invalid order']); return; }
$db = Factory::getDbo();
source/packages/com_mokosuiteclient/admin/src/Service/AttachmentService.php
+10 -4
View File
@@ -52,8 +52,9 @@ class AttachmentService
$ticketDir = self::STORAGE_DIR . '/' . $ticketId;
if (!is_dir($ticketDir)) {
Folder::create($ticketDir);
if (!is_dir($ticketDir) && !Folder::create($ticketDir)) {
Log::add("Failed to create attachment directory: {$ticketDir}", Log::ERROR, 'mokosuiteclient');
return [];
}
$userId = (int) Factory::getUser()->id;
@@ -62,6 +63,7 @@ class AttachmentService
for ($i = 0, $count = count($files['name']); $i < $count; $i++)
{
if ($files['error'][$i] !== UPLOAD_ERR_OK) {
Log::add("Attachment upload error for '{$files['name'][$i]}': PHP error code {$files['error'][$i]}", Log::WARNING, 'mokosuiteclient');
continue;
}
@@ -127,9 +129,13 @@ class AttachmentService
/**
* Get the absolute filesystem path for an attachment.
*/
public static function getAbsolutePath(object $attachment): string
public static function getAbsolutePath(object $attachment): ?string
{
return self::STORAGE_DIR . '/' . $attachment->filepath;
$path = realpath(self::STORAGE_DIR . '/' . $attachment->filepath);
if ($path === false || !str_starts_with($path, realpath(self::STORAGE_DIR))) {
return null;
}
return $path;
}
/**
source/packages/com_mokosuiteclient/admin/src/Service/AutomationEngine.php
+1 -1
View File
@@ -187,7 +187,7 @@ class AutomationEngine
}
catch (\Throwable $e)
{
Log::add("Automation action {$type} failed: " . $e->getMessage(), Log::WARNING, 'mokosuiteclient');
Log::add("Automation action '{$type}' failed for rule #{$rule->id}: " . $e->getMessage(), Log::ERROR, 'mokosuiteclient');
}
}
}
source/packages/com_mokosuiteclient/admin/src/Service/NotificationService.php
+9 -3
View File
@@ -305,6 +305,7 @@ class NotificationService
}
catch (\Throwable $e)
{
Log::add('Failed to look up email for user ID ' . $userId . ': ' . $e->getMessage(), Log::WARNING, 'mokosuiteclient');
return null;
}
}
@@ -331,6 +332,7 @@ class NotificationService
}
catch (\Throwable $e)
{
Log::add('Failed to load notification config: ' . $e->getMessage(), Log::ERROR, 'mokosuiteclient');
return [];
}
}
@@ -397,12 +399,16 @@ class NotificationService
curl_setopt($ch, CURLOPT_HTTPHEADER, $headers);
curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
curl_setopt($ch, CURLOPT_TIMEOUT, 5);
curl_exec($ch);
$response = curl_exec($ch);
$httpCode = (int) curl_getinfo($ch, CURLINFO_HTTP_CODE);
$curlError = curl_error($ch);
curl_close($ch);
if ($httpCode < 200 || $httpCode >= 300)
if ($response === false)
{
Log::add("Ntfy push connection failed for event {$event}: " . $curlError, Log::WARNING, 'mokosuiteclient');
}
elseif ($httpCode < 200 || $httpCode >= 300)
{
Log::add("Ntfy push failed (HTTP {$httpCode}) for event {$event}", Log::WARNING, 'mokosuiteclient');
}
source/packages/com_mokosuiteclient/api/src/Controller/TicketsController.php
+1
View File
@@ -264,6 +264,7 @@ class TicketsController extends BaseController
$user = Factory::getUser();
if (!$user->authorise($action, $asset)) {
$this->sendJson(403, ['error' => 'Not authorized']);
throw new \RuntimeException('Not authorized', 403);
}
}
source/packages/plg_system_mokosuiteclient/script.php
+1 -1
View File
@@ -527,7 +527,7 @@ class plgSystemMokoSuiteClientInstallerScript implements InstallerScriptInterfac
}
catch (\Exception $e)
{
// Don't break install if email fails
Log::add('Install notification email failed: ' . $e->getMessage(), Log::WARNING, 'mokosuiteclient');
}
}
source/packages/plg_task_mokosuiteclient_tickets/src/Extension/TicketAutomation.php
+1
View File
@@ -251,6 +251,7 @@ class TicketAutomation extends CMSPlugin implements SubscriberInterface
}
catch (\Throwable $e)
{
Log::add('Failed to load component config: ' . $e->getMessage(), Log::ERROR, 'mokosuiteclient');
return [];
}
}