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