Skip to content

Commit d7c31a0

Browse files
committed
Properly fix input sanitation for order parameters
Refs #388
1 parent 482c8d8 commit d7c31a0

File tree

3 files changed

+23
-17
lines changed

3 files changed

+23
-17
lines changed

src/DataTable.php

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ class DataTable
6060
public const DEFAULT_TEMPLATE = '@DataTables/datatable_html.html.twig';
6161
public const SORT_ASCENDING = 'asc';
6262
public const SORT_DESCENDING = 'desc';
63+
public const SORT_OPTIONS = [self::SORT_ASCENDING, self::SORT_DESCENDING];
6364

6465
protected ?AdapterInterface $adapter = null;
6566

@@ -174,10 +175,11 @@ public function addOrderBy($column, string $direction = self::SORT_ASCENDING)
174175
if (!$column instanceof AbstractColumn) {
175176
$column = is_int($column) ? $this->getColumn($column) : $this->getColumnByName((string) $column);
176177
}
177-
if (false !== $this->getOption('ordering')) {
178-
$direction = self::SORT_ASCENDING === mb_strtolower($direction) ? self::SORT_ASCENDING : self::SORT_DESCENDING;
179-
$this->options['order'][] = [$column->getIndex(), $direction];
178+
$direction = mb_strtolower($direction);
179+
if (!in_array($direction, self::SORT_OPTIONS, true)) {
180+
throw new \InvalidArgumentException(sprintf('Sort direction must be one of %s', implode(', ', self::SORT_OPTIONS)));
180181
}
182+
$this->options['order'][] = [$column->getIndex(), $direction];
181183

182184
return $this;
183185
}

src/DataTableState.php

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -181,10 +181,11 @@ public function setGlobalSearch(string $globalSearch): static
181181

182182
public function addOrderBy(AbstractColumn $column, string $direction = DataTable::SORT_ASCENDING): static
183183
{
184-
if (false !== $this->dataTable->getOption('ordering')) {
185-
$direction = DataTable::SORT_ASCENDING === mb_strtolower($direction) ? DataTable::SORT_ASCENDING : DataTable::SORT_DESCENDING;
186-
$this->orderBy[] = [$column, $direction];
184+
$direction = mb_strtolower($direction);
185+
if (!in_array($direction, DataTable::SORT_OPTIONS, true)) {
186+
throw new \InvalidArgumentException(sprintf('Sort direction must be one of %s', implode(', ', DataTable::SORT_OPTIONS)));
187187
}
188+
$this->orderBy[] = [$column, $direction];
188189

189190
return $this;
190191
}

tests/Unit/DataTableTest.php

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ public function testDataTableState(): void
9696
$state->setGlobalSearch('foo');
9797
$state->setOrderBy([
9898
[$datatable->getColumn(0), 'asc'],
99-
[$datatable->getColumn(1), 'desc0"XOR(if(now()=sysdate(),sleep(15),0))XOR"Z'], // intentional sql-injection test
99+
[$datatable->getColumn(1), 'desc'],
100100
]);
101101
$state->setColumnSearch($datatable->getColumn(0), 'bar');
102102

@@ -105,7 +105,6 @@ public function testDataTableState(): void
105105
$this->assertSame('foo', $state->getGlobalSearch());
106106
$this->assertCount(2, $state->getOrderBy());
107107
foreach ($state->getOrderBy() as $order) {
108-
// ensure sql-injection failed
109108
$this->assertContains($order[1], [DataTable::SORT_ASCENDING, DataTable::SORT_DESCENDING]);
110109
}
111110
$this->assertSame('bar', $state->getSearchColumns(onlySearchable: false)['foo']['search']);
@@ -146,19 +145,23 @@ public function testDataTableStateSearchColumns(): void
146145
/**
147146
* If ordering is false, ensure columns are not ordered.
148147
*/
149-
public function testDataTablesStateOrdering(): void
148+
public function testSortDirectionValidation(): void
150149
{
150+
$this->expectException(\InvalidArgumentException::class);
151+
$this->expectExceptionMessage('direction must be one of');
152+
151153
$datatable = $this
152-
->createMockDataTable(['ordering' => false])
154+
->createMockDataTable()
153155
->add('foo', TextColumn::class, ['searchable' => true])
154-
->add('bar', TextColumn::class, ['searchable' => false])
155-
->setMethod(Request::METHOD_GET)
156156
;
157-
$datatable->handleRequest(Request::create('/?_dt=' . $datatable->getName()));
158-
159-
$state = $datatable->getState();
160-
$state->addOrderBy($datatable->getColumn(0), DataTable::SORT_DESCENDING);
161-
$this->assertEmpty($state->getOrderBy());
157+
$datatable->handleRequest(Request::create('/foo', Request::METHOD_POST, [
158+
'_dt' => $datatable->getName(),
159+
'draw' => 684,
160+
'order' => [[
161+
'column' => 0,
162+
'dir' => 'foo',
163+
]],
164+
]));
162165
}
163166

164167
public function testPostMethod(): void

0 commit comments

Comments
 (0)