From 6b88e4c915b0d4c7e7de2477d0a2d19dec3bb7c6 Mon Sep 17 00:00:00 2001 From: HafizMMoaz Date: Mon, 7 Sep 2026 16:15:41 +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. --- .../Controllers/Api/ProjectApiController.php | 2 +- .../Controllers/Api/TaskApiController.php | 2 +- src/Http/Controllers/ProjectBugController.php | 4 +- src/Http/Controllers/ProjectController.php | 2 +- .../Controllers/ProjectReportController.php | 2 +- .../Controllers/ProjectTaskController.php | 2 +- src/Providers/TasklyServiceProvider.php | 23 ++++++ tests/SortSafeTest.php | 71 +++++++++++++++++++ 8 files changed, 100 insertions(+), 8 deletions(-) create mode 100644 tests/SortSafeTest.php diff --git a/src/Http/Controllers/Api/ProjectApiController.php b/src/Http/Controllers/Api/ProjectApiController.php index cb976c0..8c27f40 100644 --- a/src/Http/Controllers/Api/ProjectApiController.php +++ b/src/Http/Controllers/Api/ProjectApiController.php @@ -50,7 +50,7 @@ public function index(Request $request) } }) ->when($request->status && in_array($request->status, ['Ongoing', 'Onhold', 'Finished']), fn($q) => $q->where('status', $request->status)) - ->when($request->sort, fn($q) => $q->orderBy($request->sort, $request->get('direction', 'asc')), fn($q) => $q->latest()) + ->when($request->sort, fn($q) => $q->sortSafe($request->sort, $request->get('direction'), 'created_at', 'desc'), fn($q) => $q->latest()) ->paginate($request->get('per_page', 10)); $items->getCollection()->transform(function ($project) { diff --git a/src/Http/Controllers/Api/TaskApiController.php b/src/Http/Controllers/Api/TaskApiController.php index c6c06b5..1c18787 100644 --- a/src/Http/Controllers/Api/TaskApiController.php +++ b/src/Http/Controllers/Api/TaskApiController.php @@ -47,7 +47,7 @@ public function index(Request $request) }) ->when($request->project_id, fn($q) => $q->where('project_id', $request->project_id)) ->when($request->status && in_array($request->status, ['High', 'Medium', 'Low']), fn($q) => $q->where('priority', $request->status)) - ->when($request->sort, fn($q) => $q->orderBy($request->sort, $request->get('direction', 'asc')), fn($q) => $q->latest()); + ->when($request->sort, fn($q) => $q->sortSafe($request->sort, $request->get('direction'), 'created_at', 'desc'), fn($q) => $q->latest()); $items = $items->get(); $items->transform(function ($task) { diff --git a/src/Http/Controllers/ProjectBugController.php b/src/Http/Controllers/ProjectBugController.php index 35a8fe5..dc43866 100644 --- a/src/Http/Controllers/ProjectBugController.php +++ b/src/Http/Controllers/ProjectBugController.php @@ -58,9 +58,7 @@ public function index(Request $request) $query->where('priority', $request->priority); } - $sortField = $request->get('sort', 'created_at'); - $sortDirection = $request->get('direction', 'desc'); - $query->orderBy($sortField, $sortDirection); + $query->sortSafe($request->get('sort'), $request->get('direction'), 'created_at', 'desc'); $perPage = $request->get('per_page', 10); $bugs = $query->paginate($perPage); diff --git a/src/Http/Controllers/ProjectController.php b/src/Http/Controllers/ProjectController.php index e98391a..8b9a3e5 100644 --- a/src/Http/Controllers/ProjectController.php +++ b/src/Http/Controllers/ProjectController.php @@ -67,7 +67,7 @@ public function index() ->when(request('status'), fn($q) => $q->where('status', request('status'))) ->when(request('date'), fn($q) => $q->where('start_date', '<=', request('date'))->where('end_date', '>=', request('date'))) - ->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/ProjectReportController.php b/src/Http/Controllers/ProjectReportController.php index 7b3b427..b0c9b25 100644 --- a/src/Http/Controllers/ProjectReportController.php +++ b/src/Http/Controllers/ProjectReportController.php @@ -41,7 +41,7 @@ public function index(Request $request) ->when($request->get('name'), fn($q) => $q->where('name', 'like', '%' . $request->get('name') . '%')) ->when($request->get('status'), fn($q) => $q->where('status', $request->get('status'))) ->when($request->get('date'), fn($q) => $q->where('start_date', '<=', $request->get('date'))->where('end_date', '>=', $request->get('date'))) - ->when($request->get('sort'), fn($q) => $q->orderBy($request->get('sort'), $request->get('direction', 'asc')), fn($q) => $q->latest()) + ->when($request->get('sort'), fn($q) => $q->sortSafe($request->get('sort'), $request->get('direction'), 'created_at', 'desc'), fn($q) => $q->latest()) ->paginate($request->get('per_page', 10)) ->withQueryString(); diff --git a/src/Http/Controllers/ProjectTaskController.php b/src/Http/Controllers/ProjectTaskController.php index 6e68c02..2b6772d 100644 --- a/src/Http/Controllers/ProjectTaskController.php +++ b/src/Http/Controllers/ProjectTaskController.php @@ -54,7 +54,7 @@ public function index(Request $request) $tasks = $query->when($projectId, fn($q) => $q->where('project_id', $projectId)) ->when(request('title'), fn($q) => $q->where('title', 'like', '%' . request('title') . '%')) ->when(request('priority'), fn($q) => $q->where('priority', request('priority'))) - ->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/TasklyServiceProvider.php b/src/Providers/TasklyServiceProvider.php index bcde66b..f55dd96 100644 --- a/src/Providers/TasklyServiceProvider.php +++ b/src/Providers/TasklyServiceProvider.php @@ -2,6 +2,8 @@ namespace Zerp\Taskly\Providers; +use Illuminate\Database\Eloquent\Builder; +use Illuminate\Support\Facades\Schema; use Illuminate\Support\ServiceProvider; class TasklyServiceProvider 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..1fdffd7 --- /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); + } +}