Skip to content

Commit 2ba394f

Browse files
gharlangharlan
committed
fix: apply sort request for columns marked sortable after list construction
Since the sort column is validated against the sortable columns (GHSA-4f5f-j737-pm58), sorting via column headers stopped working: the query runs in the rex_list constructor, but setColumnSortable() is called afterwards, so the whitelist was always empty at that point. When a sort request is pending, the constructor now fetches only the field names (via a free LIMIT 0 metadata query) and defers the data query to get(), where all sortable columns are registered (including those added via the REX_LIST_GET extension point).
1 parent fb008b5 commit 2ba394f

2 files changed

Lines changed: 121 additions & 3 deletions

File tree

‎redaxo/src/core/lib/list.php‎

Lines changed: 49 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,11 @@ class rex_list implements rex_url_provider_interface
6666
// --------- List Attributes
6767
/** @var string */
6868
private $name;
69+
private string $query;
70+
/** @var array<string, 'asc'|'desc'> */
71+
private array $defaultSort;
72+
private string $executedQuery;
73+
private bool $dataLoaded = false;
6974
/** @var array<string, string|int> */
7075
private array $params;
7176
private int $rows;
@@ -139,6 +144,8 @@ protected function __construct($query, $rowsPerPage = 30, $listName = null, $deb
139144
$this->debug = $debug;
140145
$this->sql->setDebug($this->debug);
141146
$this->name = $listName;
147+
$this->query = $query;
148+
$this->defaultSort = $defaultSort;
142149
$this->caption = '';
143150
$this->rows = 0;
144151
$this->params = [];
@@ -186,9 +193,14 @@ protected function __construct($query, $rowsPerPage = 30, $listName = null, $deb
186193
}
187194

188195
// --------- Load Data
189-
$this->sql->setQuery($this->prepareQuery($query, $defaultSort));
190-
if (self::DISABLE_PAGINATION === $rowsPerPage) {
191-
$this->rows = (int) $this->sql->getRows();
196+
if (rex_request('list', 'string') == $this->getName() && '' !== rex_request('sort', 'string', '')) {
197+
// The requested sort column can not be validated yet since the sortable columns are registered
198+
// after construction (getSortColumn() checks against them). Fetch only the field names here,
199+
// the data is loaded in get() after the sortable columns are known.
200+
$this->executedQuery = 'SELECT * FROM (' . $query . ') t LIMIT 0';
201+
$this->sql->setQuery($this->executedQuery);
202+
} else {
203+
$this->loadData();
192204
}
193205

194206
foreach ($this->sql->getFieldnames() as $columnName) {
@@ -933,13 +945,37 @@ private static function prepareCountQuery(string $query): string
933945
return 'SELECT COUNT(*) AS `rows` FROM (' . $query . ') t';
934946
}
935947

948+
/**
949+
* Executes the query unless it already ran and the effective query is still the same
950+
* (it changes when columns are marked as sortable after construction).
951+
*/
952+
private function loadData(): void
953+
{
954+
$query = $this->prepareQuery($this->query, $this->defaultSort);
955+
if ($this->dataLoaded && $query === $this->executedQuery) {
956+
return;
957+
}
958+
959+
$this->dataLoaded = true;
960+
$this->executedQuery = $query;
961+
$this->sql->setQuery($query);
962+
963+
if (null === $this->pager) {
964+
$this->rows = (int) $this->sql->getRows();
965+
}
966+
}
967+
936968
/**
937969
* Gibt die Anzahl der Zeilen zurück, welche vom ursprüngliche SQL Statement betroffen werden.
938970
*
939971
* @return int
940972
*/
941973
public function getRows()
942974
{
975+
if (!$this->dataLoaded) {
976+
$this->loadData();
977+
}
978+
943979
return $this->rows;
944980
}
945981

@@ -1184,6 +1220,12 @@ public function getColumnLink($columnName, $columnValue, $params = [])
11841220
*/
11851221
public function getValue($column)
11861222
{
1223+
// only initial loading here, no unconditional loadData(): the query (and with it the
1224+
// result pointer) must not change anymore while the rows are iterated in get()
1225+
if (!$this->dataLoaded) {
1226+
$this->loadData();
1227+
}
1228+
11871229
return $this->customColumns[$column] ?? $this->sql->getValue($column);
11881230
}
11891231

@@ -1205,6 +1247,10 @@ public function get()
12051247
{
12061248
rex_extension::registerPoint(new rex_extension_point('REX_LIST_GET', $this, [], true));
12071249

1250+
// By now the sortable columns are registered (including via the extension point above),
1251+
// so the deferred data query can run with the validated ORDER BY
1252+
$this->loadData();
1253+
12081254
$s = "\n";
12091255

12101256
// Form vars

‎redaxo/src/core/tests/list_test.php‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,78 @@ public function testGetSortColumnIgnoredForOtherList(): void
5050
unset($_REQUEST['list'], $_REQUEST['sort']);
5151
}
5252

53+
public function testSortableColumnRegisteredAfterConstructionIsApplied(): void
54+
{
55+
$table = 'rex_tests_list';
56+
57+
$sql = rex_sql::factory();
58+
$sql->setQuery('DROP TABLE IF EXISTS `' . $table . '`');
59+
$sql->setQuery('CREATE TABLE `' . $table . '` (
60+
`id` INT NOT NULL AUTO_INCREMENT,
61+
`name` VARCHAR(255) NOT NULL,
62+
PRIMARY KEY (`id`)
63+
) ENGINE = InnoDB');
64+
$sql->setQuery('INSERT INTO `' . $table . '` (`name`) VALUES ("aaa"), ("zzz")');
65+
66+
$_REQUEST['list'] = 'sortlist';
67+
$_REQUEST['sort'] = 'name';
68+
$_REQUEST['sorttype'] = 'desc';
69+
70+
try {
71+
// setColumnSortable() is called after the constructor already executed the query,
72+
// the requested sort order must still be applied (https://github.com/redaxo/core/issues/6585)
73+
$list = rex_list::factory('SELECT id, name FROM ' . $table, rex_list::DISABLE_PAGINATION, 'sortlist');
74+
$list->setColumnSortable('name');
75+
76+
$html = $list->get();
77+
78+
$posZzz = strpos($html, 'zzz');
79+
$posAaa = strpos($html, 'aaa');
80+
self::assertNotFalse($posZzz);
81+
self::assertNotFalse($posAaa);
82+
self::assertLessThan($posAaa, $posZzz, 'rows must be sorted desc by name');
83+
self::assertSame(2, $list->getRows());
84+
self::assertSame(['id', 'name'], $list->getColumnNames());
85+
} finally {
86+
unset($_REQUEST['list'], $_REQUEST['sort'], $_REQUEST['sorttype']);
87+
$sql->setQuery('DROP TABLE `' . $table . '`');
88+
}
89+
}
90+
91+
public function testNonSortableSortParamFallsBackToDefaultOrder(): void
92+
{
93+
$table = 'rex_tests_list';
94+
95+
$sql = rex_sql::factory();
96+
$sql->setQuery('DROP TABLE IF EXISTS `' . $table . '`');
97+
$sql->setQuery('CREATE TABLE `' . $table . '` (
98+
`id` INT NOT NULL AUTO_INCREMENT,
99+
`name` VARCHAR(255) NOT NULL,
100+
PRIMARY KEY (`id`)
101+
) ENGINE = InnoDB');
102+
$sql->setQuery('INSERT INTO `' . $table . '` (`name`) VALUES ("aaa"), ("zzz")');
103+
104+
$_REQUEST['list'] = 'sortlist';
105+
$_REQUEST['sort'] = 'name';
106+
$_REQUEST['sorttype'] = 'desc';
107+
108+
try {
109+
// "name" is never marked as sortable, so the requested sort must be ignored
110+
$list = rex_list::factory('SELECT id, name FROM ' . $table, rex_list::DISABLE_PAGINATION, 'sortlist', defaultSort: ['id' => 'asc']);
111+
112+
$html = $list->get();
113+
114+
$posZzz = strpos($html, 'zzz');
115+
$posAaa = strpos($html, 'aaa');
116+
self::assertNotFalse($posZzz);
117+
self::assertNotFalse($posAaa);
118+
self::assertLessThan($posZzz, $posAaa, 'rows must keep the default sort order');
119+
} finally {
120+
unset($_REQUEST['list'], $_REQUEST['sort'], $_REQUEST['sorttype']);
121+
$sql->setQuery('DROP TABLE `' . $table . '`');
122+
}
123+
}
124+
53125
private function createListWithSortableColumn(string $name, string $sortableColumn): rex_list
54126
{
55127
$list = (new ReflectionClass(rex_list::class))->newInstanceWithoutConstructor();

0 commit comments

Comments
 (0)