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); + } +}