Skip to content
Permalink
Browse files

wip security fix

  • Loading branch information...
AlexVanderbist committed Apr 9, 2019
1 parent 6177f86 commit 3aa483b63c79d9fabcb8653fe837a7736eb93bea
@@ -0,0 +1,37 @@
<?php
namespace Spatie\QueryBuilder;
use Spatie\QueryBuilder\Exceptions\InvalidColumnName;
class ColumnNameSanitizer
{
/**
* Based on maximum column name length.
*/
public const MAX_COLUMN_NAME_LENGTH = 64;
/**
* Column names are alphanumeric strings that can contain
* underscores (`_`) but can't start with a number.
*/
private const VALID_COLUMN_NAME_REGEX = "/^(?![0-9])[A-Za-z0-9_-]*$/";
public static function sanitize(string $column): string
{
if (strlen($column) > self::MAX_COLUMN_NAME_LENGTH) {
throw InvalidColumnName::columnNameTooLong($column, self::MAX_COLUMN_NAME_LENGTH);
}
if (! preg_match(self::VALID_COLUMN_NAME_REGEX, $column)) {
throw InvalidColumnName::invalidCharacters($column);
}
return $column;
}
public static function sanitizeArray(array $columns): array
{
return array_map([self::class, 'sanitize'], $columns);
}
}
@@ -4,6 +4,7 @@
use Illuminate\Support\Str;
use Illuminate\Support\Collection;
use Spatie\QueryBuilder\ColumnNameSanitizer;
use Spatie\QueryBuilder\Exceptions\InvalidFieldQuery;
trait AddsFieldsToQuery
@@ -35,15 +36,19 @@ public function allowedFields($fields): self
public function parseFields()
{
$this->addFieldsToQuery($this->request->fields());
$this->addFieldsToQuery($this->getRequestedFields());
}
protected function addFieldsToQuery(Collection $fields)
{
$modelTableName = $this->getModel()->getTable();
if ($modelFields = $fields->get($modelTableName)) {
$this->select($this->prependFieldsWithTableName($modelFields, $modelTableName));
$sanitizedFields = ColumnNameSanitizer::sanitizeArray($modelFields);
$prependedFields = $this->prependFieldsWithTableName($sanitizedFields, $modelTableName);
$this->select($prependedFields);
}
}
@@ -56,12 +61,12 @@ protected function prependFieldsWithTableName(array $fields, string $tableName):
protected function getFieldsForIncludedTable(string $relation): array
{
return $this->request->fields()->get($relation, []);
return $this->getRequestedFields()->get($relation, []);
}
protected function guardAgainstUnknownFields()
{
$fields = $this->request->fields()
$fields = $this->getRequestedFields()
->map(function ($fields, $model) {
$tableName = Str::snake(preg_replace('/-/', '_', $model));
@@ -78,4 +83,9 @@ protected function guardAgainstUnknownFields()
throw InvalidFieldQuery::fieldsNotAllowed($diff, $this->allowedFields);
}
}
protected function getRequestedFields(): Collection
{
return $this->request->fields();
}
}
@@ -2,6 +2,8 @@
namespace Spatie\QueryBuilder\Concerns;
use Spatie\QueryBuilder\ColumnNameSanitizer;
use Spatie\QueryBuilder\Exceptions\InvalidColumnName;
use Spatie\QueryBuilder\Sort;
use Illuminate\Support\Collection;
use Spatie\QueryBuilder\Exceptions\InvalidSortQuery;
@@ -103,12 +105,8 @@ protected function findSort(string $property): ?Sort
protected function addDefaultSorts()
{
$this->allowedSorts = collect($this->request->sorts($this->defaultSorts))
$this->allowedSorts = $this->request->sorts()
->map(function ($sort) {
if ($sort instanceof Sort) {
return $sort;
}
return Sort::field(ltrim($sort, '-'));
});
@@ -0,0 +1,28 @@
<?php
namespace Spatie\QueryBuilder\Exceptions;
use Illuminate\Http\Response;
class InvalidColumnName extends InvalidQuery
{
/** @var string */
public $column;
public function __construct(string $column, string $message)
{
$this->column = $column;
parent::__construct(Response::HTTP_BAD_REQUEST, $message);
}
public static function columnNameTooLong(string $column, int $maxLength = 64)
{
return new static($column, "Given column name `{$column}` exceeds the maximum column name length of {$maxLength} characters.");
}
public static function invalidCharacters(string $column)
{
return new static($column, 'Column name may contain only alphanumerics or underscores, and may not begin with a digit.');
}
}
@@ -71,14 +71,6 @@ public function fields(): Collection
});
}
/**
* @return array|string|null
*/
public function sort()
{
return $this->query(config('query-builder.parameters.sort'));
}
public function sorts(): Collection
{
$sortParts = $this->sort();
@@ -29,7 +29,7 @@ public function __construct(string $property, $sortClass, ?string $columnName =
$this->defaultDirection = static::parsePropertyDirection($property);
$this->columnName = $columnName ?? $this->property;
$this->columnName = ColumnNameSanitizer::sanitize($columnName ?? $this->property);
}
public static function parsePropertyDirection(string $property): string
@@ -4,6 +4,7 @@
use DB;
use Illuminate\Http\Request;
use Spatie\QueryBuilder\Exceptions\InvalidColumnName;
use Spatie\QueryBuilder\QueryBuilder;
use Spatie\QueryBuilder\Tests\Models\TestModel;
use Spatie\QueryBuilder\Tests\Models\RelatedModel;
@@ -129,6 +130,8 @@ public function it_wont_use_sketchy_field_requests()
'fields' => ['test_models' => 'id->"\')from test_models--injection'],
]);
$this->expectException(InvalidColumnName::class);
DB::enableQueryLog();
QueryBuilder::for(TestModel::class, $request)->get();
@@ -3,6 +3,7 @@
namespace Spatie\QueryBuilder\Tests;
use Illuminate\Http\Request;
use Spatie\QueryBuilder\Exceptions\InvalidColumnName;
use Spatie\QueryBuilder\Sort;
use Illuminate\Support\Facades\DB;
use Spatie\QueryBuilder\QueryBuilder;
@@ -90,7 +91,7 @@ public function it_will_throw_an_exception_if_a_sort_property_is_not_allowed()
/** @test */
public function an_invalid_sort_query_exception_contains_the_unknown_and_allowed_sorts()
{
$exception = new InvalidSortQuery(collect(['unknown sort']), collect(['allowed sort']));
$exception = InvalidSortQuery::sortsNotAllowed(collect(['unknown sort']), collect(['allowed sort']));
$this->assertEquals(['unknown sort'], $exception->unknownSorts->all());
$this->assertEquals(['allowed sort'], $exception->allowedSorts->all());
@@ -111,6 +112,8 @@ public function it_wont_sort_if_no_sort_query_parameter_is_given()
/** @test */
public function it_wont_sort_sketchy_sort_requests()
{
$this->expectException(InvalidColumnName::class);
$this
->createQueryFromSortRequest('id->"\') asc --injection')
->get();

0 comments on commit 3aa483b

Please sign in to comment.
You can’t perform that action at this time.