From 4e4d6a95f869e7fbc037cc0107bcb51ca9fa14dc Mon Sep 17 00:00:00 2001 From: HafizMMoaz Date: Mon, 7 Sep 2026 16:15:06 +0500 Subject: [PATCH] Validate sort/direction against SQL injection on index screens The index screens ordered by a column and direction taken straight from the query string. Eloquent binds values but not identifiers, so ?sort= was interpolated into the SQL: a 500 on a bad column at best, an injection surface at worst. The direction is checked by the framework, which throws on anything that is not asc or desc, so the column is the exploitable half. Call sites now go through a sortSafe macro that keeps the column only if it is a real column on the model's table, and the direction only if it is asc or desc. Each site falls back to the order it already used when no sort was given, so an invalid sort behaves like no sort instead of erroring. The macro is registered in this package's own provider rather than shared. The modules are installed independently and declare no common dependency, so one cannot rely on another having booted. The registration is guarded, so whichever module loads first wins and the definitions are identical. Part of the platform-wide sweep tracked on zerp-pk/zerp#39. --- src/Http/Controllers/DealController.php | 2 +- src/Http/Controllers/DealTaskController.php | 2 +- src/Http/Controllers/LeadController.php | 2 +- src/Http/Controllers/LeadTaskController.php | 2 +- src/Providers/LeadServiceProvider.php | 23 +++++++ tests/SortSafeTest.php | 71 +++++++++++++++++++++ 6 files changed, 98 insertions(+), 4 deletions(-) create mode 100644 tests/SortSafeTest.php diff --git a/src/Http/Controllers/DealController.php b/src/Http/Controllers/DealController.php index 85664ce..d811f71 100644 --- a/src/Http/Controllers/DealController.php +++ b/src/Http/Controllers/DealController.php @@ -100,7 +100,7 @@ public function index() }) ->when(request('stage_id'), fn($q) => $q->where('stage_id', request('stage_id'))) ->when(request('status') !== null && request('status') !== '', fn($q) => $q->where('status', request('status'))) - ->when(request('sort'), fn($q) => $q->orderBy(request('sort'), request('direction', 'asc')), fn($q) => $q->latest()) + ->when(request('sort'), fn($q) => $q->sortSafe(request('sort'), request('direction'), 'created_at', 'desc'), fn($q) => $q->latest()) ->paginate(request('per_page', 10)) ->withQueryString(); diff --git a/src/Http/Controllers/DealTaskController.php b/src/Http/Controllers/DealTaskController.php index d9a1c77..3e55072 100644 --- a/src/Http/Controllers/DealTaskController.php +++ b/src/Http/Controllers/DealTaskController.php @@ -43,7 +43,7 @@ public function index() ->when(request('name'), fn($q) => $q->where('name', 'like', '%' . request('name') . '%')) ->when(request('priority'), fn($q) => $q->where('priority', request('priority'))) ->when(request('status'), fn($q) => $q->where('status', request('status'))) - ->when(request('sort'), fn($q) => $q->orderBy(request('sort'), request('direction', 'asc')), fn($q) => $q->latest()) + ->when(request('sort'), fn($q) => $q->sortSafe(request('sort'), request('direction'), 'created_at', 'desc'), fn($q) => $q->latest()) ->paginate(request('per_page', 10)) ->withQueryString(); diff --git a/src/Http/Controllers/LeadController.php b/src/Http/Controllers/LeadController.php index 4cd002a..9bf6235 100644 --- a/src/Http/Controllers/LeadController.php +++ b/src/Http/Controllers/LeadController.php @@ -114,7 +114,7 @@ public function index() } }) ->when(request('stage_id') && request('stage_id') !== '', fn($q) => $q->where('stage_id', request('stage_id'))) - ->when(request('sort'), fn($q) => $q->orderBy(request('sort'), request('direction', 'asc')), fn($q) => $q->latest()) + ->when(request('sort'), fn($q) => $q->sortSafe(request('sort'), request('direction'), 'created_at', 'desc'), fn($q) => $q->latest()) ->paginate(request('per_page', 10)) ->withQueryString(); diff --git a/src/Http/Controllers/LeadTaskController.php b/src/Http/Controllers/LeadTaskController.php index d893510..4695ebf 100644 --- a/src/Http/Controllers/LeadTaskController.php +++ b/src/Http/Controllers/LeadTaskController.php @@ -43,7 +43,7 @@ public function index() ->when(request('name'), fn($q) => $q->where('name', 'like', '%' . request('name') . '%')) ->when(request('priority'), fn($q) => $q->where('priority', request('priority'))) ->when(request('status'), fn($q) => $q->where('status', request('status'))) - ->when(request('sort'), fn($q) => $q->orderBy(request('sort'), request('direction', 'asc')), fn($q) => $q->latest()) + ->when(request('sort'), fn($q) => $q->sortSafe(request('sort'), request('direction'), 'created_at', 'desc'), fn($q) => $q->latest()) ->paginate(request('per_page', 10)) ->withQueryString(); diff --git a/src/Providers/LeadServiceProvider.php b/src/Providers/LeadServiceProvider.php index 3d791a0..5d1a162 100644 --- a/src/Providers/LeadServiceProvider.php +++ b/src/Providers/LeadServiceProvider.php @@ -2,6 +2,8 @@ namespace Zerp\Lead\Providers; +use Illuminate\Database\Eloquent\Builder; +use Illuminate\Support\Facades\Schema; use Illuminate\Support\ServiceProvider; class LeadServiceProvider extends ServiceProvider @@ -31,6 +33,27 @@ public function boot(): void if (is_dir($migrationsPath)) { $this->loadMigrationsFrom($migrationsPath); } + + // Whitelist orderBy against real table columns + asc|desc so a crafted + // ?sort=/?direction= cannot be interpolated into SQL. Part of the platform-wide sweep tracked on zerp-pk/zerp#39. + // + // Registered here rather than shared: these modules are installed + // independently and declare no common dependency, so a module cannot rely + // on another having booted. The guard keeps whichever loads first. + // + // hasGlobalMacro, not hasMacro: on an Eloquent builder the latter is an + // instance method for per-builder macros and cannot be called statically. + if (! Builder::hasGlobalMacro('sortSafe')) { + Builder::macro('sortSafe', function ($sort, $direction = null, $defaultColumn = 'created_at', $defaultDirection = 'desc') { + $table = $this->getModel()->getTable(); + $column = ($sort && Schema::hasColumn($table, $sort)) ? $sort : $defaultColumn; + $direction = in_array(strtolower((string) $direction), ['asc', 'desc'], true) + ? strtolower($direction) + : $defaultDirection; + + return $this->orderBy($column, $direction); + }); + } } public function register(): void diff --git a/tests/SortSafeTest.php b/tests/SortSafeTest.php new file mode 100644 index 0000000..e3084bf --- /dev/null +++ b/tests/SortSafeTest.php @@ -0,0 +1,71 @@ +id(); + $table->string('name')->nullable(); + $table->timestamps(); + }); + } + + private function query() + { + return (new class extends Model + { + protected $table = 'sortables'; + })->newQuery(); + } + + public function test_an_injected_sort_column_is_discarded(): void + { + $sql = $this->query() + ->sortSafe('name asc, (select sqlite_version()) --', null, 'created_at', 'desc') + ->toSql(); + + $this->assertStringNotContainsString('sqlite_version', $sql); + $this->assertStringNotContainsString('--', $sql); + $this->assertStringContainsString('order by "created_at" desc', $sql); + } + + public function test_an_injected_direction_is_discarded(): void + { + $sql = $this->query() + ->sortSafe('name', 'asc; drop table sortables', 'created_at', 'desc') + ->toSql(); + + $this->assertStringNotContainsString('drop table', $sql); + $this->assertStringContainsString('order by "name" desc', $sql); + } + + public function test_a_real_column_still_sorts(): void + { + $sql = $this->query()->sortSafe('name', 'asc', 'created_at', 'desc')->toSql(); + + $this->assertStringContainsString('order by "name" asc', $sql); + } + + public function test_an_unknown_column_falls_back_to_the_default(): void + { + $sql = $this->query()->sortSafe('not_a_column', 'asc', 'created_at', 'desc')->toSql(); + + $this->assertStringContainsString('order by "created_at" asc', $sql); + } +}