fix: code review fixes — security, 0-coord bug, BOM, ACL
Joomla: Extension CI / Release Readiness Check (pull_request) Failing after 4s
Universal: PR Check / Branch Policy (pull_request) Failing after 2s
Joomla: Extension CI / Lint & Validate (pull_request) Successful in 9s
Universal: PR Check / Secret Scan (pull_request) Successful in 5s
Universal: PR Check / Validate PR (pull_request) Failing after 4s
Universal: Auto Version Bump / Version Bump (push) Successful in 10s
Generic: Repo Health / Access control (pull_request) Successful in 2s
Generic: Repo Health / Site Health (pull_request) Has been skipped
Joomla: Metadata Validation / Validate Joomla Metadata (pull_request) Successful in 52s
Joomla: Extension CI / Tests (PHP 8.2) (pull_request) Has been cancelled
Joomla: Extension CI / Tests (PHP 8.3) (pull_request) Has been cancelled
Joomla: Extension CI / PHPStan Analysis (pull_request) Has been cancelled
Joomla: Extension CI / Build RC Pre-Release (pull_request) Has been cancelled
Universal: PR Check / Build RC Package (pull_request) Has been cancelled
Universal: PR Check / Report Issues (pull_request) Has been cancelled
Generic: Repo Health / Scripts governance (pull_request) Has been cancelled
Generic: Repo Health / Repository health (pull_request) Has been cancelled
Generic: Repo Health / Report Issues (pull_request) Has been cancelled

Security:
- ACL check (core.create) in ImportController before processing
- File extension validation (.csv/.txt only) on upload
- Website href restricted to http/https scheme (prevents javascript: XSS)

Bug fixes:
- Fix 0.0 coordinate rejection: use null checks instead of != 0.0
  (coordinates at equator/prime meridian are valid locations)
- Fix Haversine guard using !== null instead of PHP truthiness
- Fix geocoding result check: isset+is_numeric instead of !empty
- Strip UTF-8 BOM from first CSV header (fixes Excel-generated imports)
- Cap radius at 25000 to prevent unreasonable distance queries

Authored-by: Moko Consulting
This commit is contained in:
Jonathan Miller
2026-06-23 12:07:03 -05:00
parent 7ef9a23ef8
commit c8a3c58495
5 changed files with 47 additions and 11 deletions
@@ -33,6 +33,15 @@ class ImportController extends BaseController
{
Session::checkToken() or jexit(Text::_('JINVALID_TOKEN'));
// ACL check — user must have create permission
if (!$this->app->getIdentity()->authorise('core.create', 'com_mokosuitestorelocator'))
{
$this->setMessage(Text::_('JLIB_APPLICATION_ERROR_CREATE_RECORD_NOT_PERMITTED'), 'error');
$this->setRedirect(Route::_('index.php?option=com_mokosuitestorelocator&view=locations', false));
return;
}
/** @var \Moko\Component\MokoSuiteStoreLocator\Administrator\Model\ImportModel $model */
$model = $this->getModel('Import', 'Administrator');
@@ -49,6 +58,17 @@ class ImportController extends BaseController
return;
}
// Validate file extension
$ext = strtolower(pathinfo($csvFile['name'], PATHINFO_EXTENSION));
if ($ext !== 'csv' && $ext !== 'txt')
{
$this->setMessage(Text::_('COM_MOKOJOOMSTORELOCATOR_IMPORT_INVALID_FILE'), 'error');
$this->setRedirect(Route::_('index.php?option=com_mokosuitestorelocator&view=import', false));
return;
}
$result = $model->processImport($csvFile['tmp_name'], $delimiter);
if ($result['imported'] > 0)
@@ -77,9 +77,14 @@ class ImportModel extends BaseDatabaseModel
$file->setFlags(SplFileObject::READ_CSV | SplFileObject::SKIP_EMPTY | SplFileObject::DROP_NEW_LINE);
$file->setCsvControl($delimiter);
// Read and map headers
// Read and map headers (strip UTF-8 BOM from Excel exports)
$headers = $file->fgetcsv();
if (!empty($headers[0]))
{
$headers[0] = ltrim($headers[0], "\xEF\xBB\xBF");
}
if (!$headers || \count($headers) < 2)
{
$result['errors'][] = 'Invalid CSV headers.';
@@ -160,7 +160,8 @@ class LocationModel extends AdminModel
$results = json_decode($response->body, true);
if (!empty($results[0]['lat']) && !empty($results[0]['lon']))
if (isset($results[0]['lat']) && is_numeric($results[0]['lat'])
&& isset($results[0]['lon']) && is_numeric($results[0]['lon']))
{
return [
'lat' => round((float) $results[0]['lat'], 8),
@@ -67,22 +67,32 @@ class LocationsModel extends ListModel
$state = $app->input->getString('state', '');
$this->setState('filter.state', $state);
$lat = $app->input->getFloat('lat', 0.0);
$lng = $app->input->getFloat('lng', 0.0);
$latRaw = $app->input->getString('lat', '');
$lngRaw = $app->input->getString('lng', '');
$radius = $app->input->getInt('radius', 0);
$radiusUnit = $app->input->getString('radius_unit', 'miles');
if ($lat >= -90 && $lat <= 90 && $lat != 0.0)
if ($latRaw !== '' && is_numeric($latRaw))
{
$this->setState('filter.lat', $lat);
$lat = (float) $latRaw;
if ($lat >= -90 && $lat <= 90)
{
$this->setState('filter.lat', $lat);
}
}
if ($lng >= -180 && $lng <= 180 && $lng != 0.0)
if ($lngRaw !== '' && is_numeric($lngRaw))
{
$this->setState('filter.lng', $lng);
$lng = (float) $lngRaw;
if ($lng >= -180 && $lng <= 180)
{
$this->setState('filter.lng', $lng);
}
}
if ($radius > 0)
if ($radius > 0 && $radius <= 25000)
{
$this->setState('filter.radius', $radius);
}
@@ -149,7 +159,7 @@ class LocationsModel extends ListModel
$lng = $this->getState('filter.lng');
$radius = $this->getState('filter.radius');
if ($lat && $lng && $radius)
if ($lat !== null && $lng !== null && $radius)
{
$unit = $this->getState('filter.radius_unit', 'miles');
$earthRadius = ($unit === 'km') ? 6371 : 3959;
@@ -86,7 +86,7 @@ $item = $this->item;
</div>
<?php endif; ?>
<?php if ($item->website) : ?>
<?php if ($item->website && preg_match('#^https?://#i', $item->website)) : ?>
<div>
<strong><?php echo Text::_('COM_MOKOJOOMSTORELOCATOR_FIELD_WEBSITE'); ?>:</strong>
<a href="<?php echo $this->escape($item->website); ?>" itemprop="url" target="_blank" rel="noopener">