Skip to content

Commit 2c99464

Browse files
committed
Fix XSS vulnerability related to list pagination
Special thanks to Ulaş Deniz İlhan from Zigrin Security for discovering and reporting this vulnerability.
1 parent f5b2c1e commit 2c99464

File tree

2 files changed

+68
-35
lines changed

2 files changed

+68
-35
lines changed

Diff for: classes/controller/AdminController.php

+7-11
Original file line numberDiff line numberDiff line change
@@ -957,14 +957,15 @@ public function getList(
957957
$this->ensureListIdDefinition();
958958

959959
/* Manage default params values */
960-
$useLimit = true;
961960
if ($limit === false) {
962961
$useLimit = false;
963-
} elseif (empty($limit)) {
964-
if (isset($this->context->cookie->{$this->list_id.'_pagination'}) && $this->context->cookie->{$this->list_id.'_pagination'}) {
965-
$limit = (int)$this->context->cookie->{$this->list_id.'_pagination'};
962+
} else {
963+
$useLimit = true;
964+
$limit = HelperList::resolvePagination($this->list_id, $this->context->cookie, $this->_pagination, $this->_default_pagination);
965+
if ($limit !== $this->_default_pagination) {
966+
$this->context->cookie->{$this->list_id.'_pagination'} = $limit;
966967
} else {
967-
$limit = (int)$this->_default_pagination;
968+
unset($this->context->cookie->{$this->list_id.'_pagination'});
968969
}
969970
}
970971

@@ -975,12 +976,7 @@ public function getList(
975976
$orderBy = $this->resolveOrderBy($orderBy);
976977
$orderWay = $this->resolveOrderWay($orderWay);
977978

978-
$limit = Tools::getIntValue($this->list_id.'_pagination', $limit);
979-
if (in_array($limit, $this->_pagination) && $limit != $this->_default_pagination) {
980-
$this->context->cookie->{$this->list_id.'_pagination'} = $limit;
981-
} else {
982-
unset($this->context->cookie->{$this->list_id.'_pagination'});
983-
}
979+
984980

985981
/* Check params validity */
986982
if (!Validate::isOrderBy($orderBy) || !Validate::isOrderWay($orderWay)) {

Diff for: classes/helper/HelperList.php

+61-24
Original file line numberDiff line numberDiff line change
@@ -309,42 +309,22 @@ public function displayListHeader()
309309
}
310310

311311
/* Determine total page number */
312-
$pagination = $this->_default_pagination;
313-
$cookie = $this->context->cookie;
314-
if (in_array(Tools::getIntValue($this->list_id.'_pagination'), $this->_pagination)) {
315-
$pagination = Tools::getIntValue($this->list_id.'_pagination');
316-
} elseif (isset($cookie->{$this->list_id.'_pagination'}) && $cookie->{$this->list_id.'_pagination'}) {
317-
$pagination = $cookie->{$this->list_id.'_pagination'};
318-
}
319-
312+
$pagination = $this->getSelectedPagination();
320313
$totalPages = max(1, ceil($this->listTotal / $pagination));
321314

322315
$identifier = Tools::getIsset($this->identifier) ? '&'.$this->identifier.'='.Tools::getIntValue($this->identifier) : '';
323-
// $order = '';
324-
// if (Tools::getIsset($this->table.'Orderby')) {
325-
// $order = '&'.$this->table.'Orderby='.urlencode($this->orderBy).'&'.$this->table.'Orderway='.urlencode(strtolower($this->orderWay));
326-
// }
327316

328317
$action = $this->currentIndex.$identifier.'&token='.$token.'#'.$this->list_id;
329318

330319
/* Determine current page number */
331320
$page = Tools::getIntValue('submitFilter'.$this->list_id);
332-
333-
if (!$page) {
321+
if ($page <= 0) {
334322
$page = 1;
335323
}
336-
337324
if ($page > $totalPages) {
338325
$page = $totalPages;
339326
}
340-
341-
$this->page = (int) $page;
342-
343-
/* Choose number of results per page */
344-
$selectedPagination = Tools::getValue(
345-
$this->list_id.'_pagination',
346-
$cookie->{$this->list_id . '_pagination'} ?? $this->_default_pagination
347-
);
327+
$this->page = (int)$page;
348328

349329
if (is_null($this->table_id) && $this->position_identifier && Tools::getIntValue($this->position_identifier, 1)) {
350330
$this->table_id = substr($this->identifier, 3, strlen($this->identifier));
@@ -357,6 +337,7 @@ public function displayListHeader()
357337
$prefix = str_replace(['admin', 'controller'], '', mb_strtolower((string)$this->controller_name));
358338
$ajax = false;
359339
$controller = $this->getController();
340+
$cookie = $this->context->cookie;
360341
foreach ($this->fields_list as $key => $params) {
361342
if (!isset($params['type'])) {
362343
$params['type'] = 'text';
@@ -441,7 +422,7 @@ public function displayListHeader()
441422
'page' => $page,
442423
'simple_header' => $this->simple_header,
443424
'total_pages' => $totalPages,
444-
'selected_pagination' => $selectedPagination,
425+
'selected_pagination' => $this->getSelectedPagination(),
445426
'pagination' => $this->_pagination,
446427
'list_total' => $this->listTotal,
447428
'sql' => str_replace('\n', ' ', str_replace('\r', '', (string)$this->sql)),
@@ -1022,4 +1003,60 @@ public function displayDefaultLink($token, $id, $name = null)
10221003

10231004
return $tpl->fetch();
10241005
}
1006+
1007+
/**
1008+
* @return int
1009+
*/
1010+
protected function getSelectedPagination()
1011+
{
1012+
return static::resolvePagination($this->list_id, $this->context->cookie, $this->_pagination, $this->_default_pagination);
1013+
}
1014+
1015+
/**
1016+
* @param string $listId
1017+
* @param Cookie $cookie
1018+
* @param array $pagination
1019+
* @param int $defaultPagination
1020+
*
1021+
* @return int
1022+
*/
1023+
public static function resolvePagination(string $listId, Cookie $cookie, array $pagination, int $defaultPagination)
1024+
{
1025+
if ($pagination) {
1026+
$value = static::resolvePaginationValue($listId, $cookie, $defaultPagination);
1027+
if (in_array($value, $pagination)) {
1028+
return $value;
1029+
}
1030+
if (in_array($defaultPagination, $pagination)) {
1031+
return $defaultPagination;
1032+
}
1033+
return $pagination[0];
1034+
} else {
1035+
trigger_error("Pagination not set for list $listId", E_USER_WARNING);
1036+
return 20;
1037+
}
1038+
}
1039+
1040+
/**
1041+
* @param string $listId
1042+
* @param Cookie $cookie
1043+
* @param int $defaultPagination
1044+
*
1045+
* @return int
1046+
*/
1047+
protected static function resolvePaginationValue(string $listId, Cookie $cookie, int $defaultPagination)
1048+
{
1049+
$paginationKey = $listId.'_pagination';
1050+
$pagination = Tools::getIntValue($paginationKey);
1051+
if ($pagination > 0) {
1052+
return $pagination;
1053+
}
1054+
if (isset($cookie->{$paginationKey})) {
1055+
$pagination = (int)$cookie->{$paginationKey};
1056+
if ($pagination > 0) {
1057+
return $pagination;
1058+
}
1059+
}
1060+
return $defaultPagination;
1061+
}
10251062
}

0 commit comments

Comments
 (0)