From 8b9a61d992a17df6a377ad15382e80e350c7218f Mon Sep 17 00:00:00 2001
From: Zyra <zyra@localhost>
Date: Thu, 3 Sep 2026 12:09:22 +0000
Subject: [PATCH] Fix #119: whitelist sorteerkolom tegen defineFields() in
 DataFetcher
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Onderzoek naar SQL-injectie via sort_column/filter_column: klassieke
injectie was niet mogelijk omdat DataFetcher::fetch() de sorteerkolom
altijd toetste aan de echte databaseschema-kolommen voordat hij in
orderBy() belandde. Wel bleek de bedoelde whitelist tegen de
resource-fields (defineFields()) dode code: `$col === $sortBy`
vergeleek een Field-object met een string en kon dus nooit slagen,
waardoor élk veld dat toevallig een bestaande kolomnaam is - ook
velden zonder ->sortable() of buiten de resource om - als sorteerkolom
gebruikt kon worden (broken access control, OWASP A01).

Sorteren werkt nu alleen nog op een veld dat exact overeenkomt met een
door defineFields() gedefinieerd, expliciet sortable() veld; de
databaseschema-check blijft als extra verdedigingslaag staan.

Regressietest (tests/Unit/Datatable/DataFetcherTest.php) bewijst:
- een payload als "name); DROP TABLE ...;--" wordt genegeerd en breekt
  niets (bewijs dat er geen SQL-injectie was);
- sorteren op een bestaande maar niet-sortable kolom wordt nu
  geblokkeerd (dit faalde vóór de fix - bewijs van de access-control
  bug);
- sorteren op een wel-sortable veld blijft gewoon werken.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
---
 src/Datatable/DataFetcher.php                 |  25 ++--
 .../Fixtures/Models/DataFetcherTestModel.php  |  21 +++
 .../Resources/DataFetcherTestResource.php     |  33 +++++
 tests/Unit/Datatable/DataFetcherTest.php      | 123 ++++++++++++++++++
 4 files changed, 194 insertions(+), 8 deletions(-)
 create mode 100644 tests/Fixtures/Models/DataFetcherTestModel.php
 create mode 100644 tests/Fixtures/Resources/DataFetcherTestResource.php
 create mode 100644 tests/Unit/Datatable/DataFetcherTest.php

diff --git a/src/Datatable/DataFetcher.php b/src/Datatable/DataFetcher.php
index 1c70ef9..10b9caf 100644
--- a/src/Datatable/DataFetcher.php
+++ b/src/Datatable/DataFetcher.php
@@ -134,18 +134,27 @@ public function fetch(
         }
 
         // ✅ OPTIMIZATION 4: Qualified column names for sorting
+        //
+        // 🔒 Beveiliging: $sortBy komt rechtstreeks van de gebruiker (request
+        // input, zie BuildoraDataTableController::json()). Het mag daarom
+        // NOOIT direct in de query terechtkomen. We whitelisten $sortBy
+        // eerst tegen de door de resource gedefinieerde velden
+        // (defineFields()) en staan alleen sorteren toe op velden die
+        // expliciet met ->sortable() zijn gemarkeerd. Pas daarna wordt de
+        // bijbehorende, ontwikkelaar-gedefinieerde kolomnaam gebruikt, en
+        // enkel als die kolom ook echt in het databaseschema bestaat.
         if (!empty($sortBy)) {
-            $sortColumn = collect($this->columns)->first(fn ($col) =>
-                (is_array($col) ? ($col['name'] ?? null) : $col) === $sortBy
+            $sortField = collect($this->columns)->first(
+                fn ($col) => $col instanceof Field && $col->name === $sortBy
             );
 
-            $sortColumnName = is_array($sortColumn)
-                ? ($sortColumn['search_column'] ?? $sortColumn['name'] ?? null)
-                : $sortBy;
+            if ($sortField instanceof Field && $sortField->sortable) {
+                $sortColumnName = $sortField->getSearchColumn();
 
-            if (in_array($sortColumnName, $databaseColumns)) {
-                // Use qualified column name to avoid ambiguity with joins
-                $query->orderBy("{$modelInstance->getTable()}.{$sortColumnName}", $sortDirection);
+                if (!str_contains($sortColumnName, '.') && in_array($sortColumnName, $databaseColumns, true)) {
+                    // Use qualified column name to avoid ambiguity with joins
+                    $query->orderBy("{$modelInstance->getTable()}.{$sortColumnName}", $sortDirection);
+                }
             }
         }
 
diff --git a/tests/Fixtures/Models/DataFetcherTestModel.php b/tests/Fixtures/Models/DataFetcherTestModel.php
new file mode 100644
index 0000000..f912c16
--- /dev/null
+++ b/tests/Fixtures/Models/DataFetcherTestModel.php
@@ -0,0 +1,21 @@
+<?php
+
+namespace Ginkelsoft\Buildora\Tests\Fixtures\Models;
+
+use Ginkelsoft\Buildora\Traits\HasBuildora;
+use Illuminate\Database\Eloquent\Model;
+
+/**
+ * Minimaal Eloquent-model, uitsluitend gebruikt om DataFetcher tegen een
+ * echte (sqlite) database te kunnen testen.
+ */
+class DataFetcherTestModel extends Model
+{
+    use HasBuildora;
+
+    protected $table = 'df_test_items';
+
+    protected $guarded = [];
+
+    public $timestamps = false;
+}
diff --git a/tests/Fixtures/Resources/DataFetcherTestResource.php b/tests/Fixtures/Resources/DataFetcherTestResource.php
new file mode 100644
index 0000000..995e7d7
--- /dev/null
+++ b/tests/Fixtures/Resources/DataFetcherTestResource.php
@@ -0,0 +1,33 @@
+<?php
+
+namespace Ginkelsoft\Buildora\Tests\Fixtures\Resources;
+
+use Ginkelsoft\Buildora\Fields\Field;
+use Ginkelsoft\Buildora\Fields\Types\TextField;
+use Ginkelsoft\Buildora\Resources\BuildoraResource;
+use Ginkelsoft\Buildora\Tests\Fixtures\Models\DataFetcherTestModel;
+
+/**
+ * Minimale resource, uitsluitend gebruikt om DataFetcher te testen.
+ *
+ * - "name" is expliciet sorteerbaar gemaakt.
+ * - "secret_score" bestaat wel als echte kolom in de database, maar is
+ *   bewust NIET als sorteerbaar gemarkeerd. Deze regressietest bewijst dat
+ *   DataFetcher dat respecteert: alleen expliciet gedefinieerde, sorteerbare
+ *   velden mogen als sorteerkolom gebruikt worden.
+ */
+class DataFetcherTestResource extends BuildoraResource
+{
+    public static function modelClass(): string
+    {
+        return DataFetcherTestModel::class;
+    }
+
+    public function defineFields(): array
+    {
+        return [
+            TextField::make('name', 'Name'), // TextField is standaard sortable()
+            Field::make('secret_score', 'Secret score'), // bewust niet sortable
+        ];
+    }
+}
diff --git a/tests/Unit/Datatable/DataFetcherTest.php b/tests/Unit/Datatable/DataFetcherTest.php
new file mode 100644
index 0000000..0108c54
--- /dev/null
+++ b/tests/Unit/Datatable/DataFetcherTest.php
@@ -0,0 +1,123 @@
+<?php
+
+namespace Ginkelsoft\Buildora\Tests\Unit\Datatable;
+
+use Ginkelsoft\Buildora\Datatable\DataFetcher;
+use Ginkelsoft\Buildora\Support\SchemaCache;
+use Ginkelsoft\Buildora\Tests\Fixtures\Models\DataFetcherTestModel;
+use Ginkelsoft\Buildora\Tests\Fixtures\Resources\DataFetcherTestResource;
+use Ginkelsoft\Buildora\Tests\TestCase;
+use Illuminate\Support\Facades\DB;
+use Illuminate\Support\Facades\Schema;
+
+/**
+ * Issue #119: is DataFetcher kwetsbaar voor SQL injectie via de
+ * sort/filter-kolomnaam?
+ *
+ * Bevinding: klassieke SQL-injectie via de kolomnaam was niet mogelijk,
+ * omdat de opgegeven sorteerkolom altijd getoetst werd aan de echte
+ * kolomnamen van de tabel (SchemaCache::getColumnListing) voordat hij in de
+ * query terechtkwam; een payload als `name); DROP TABLE users;--` matcht
+ * geen bestaande kolomnaam en wordt dus genegeerd.
+ *
+ * Wel bleek de bedoelde whitelist tegen de resource's defineFields() dode
+ * code: de vergelijking `$col === $sortBy` kan nooit slagen omdat $col een
+ * Field-object is (geen string/array), waardoor elk willekeurig veld dat
+ * toevallig een bestaande databasekolom is - ook velden die niet met
+ * ->sortable() zijn gemarkeerd, of die helemaal niet in de resource zijn
+ * gedefinieerd - gebruikt kon worden als sorteerkolom. Dat is een broken
+ * access control-gat (OWASP A01), gefixt door pas te sorteren op een veld
+ * dat exact overeenkomt met een door de resource gedefinieerd EN expliciet
+ * sortable() veld.
+ */
+class DataFetcherTest extends TestCase
+{
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        Schema::dropIfExists('df_test_items');
+        Schema::create('df_test_items', function ($table) {
+            $table->id();
+            $table->string('name');
+            $table->integer('secret_score')->default(0);
+        });
+
+        DataFetcherTestModel::query()->insert([
+            ['name' => 'Banaan', 'secret_score' => 3],
+            ['name' => 'Appel', 'secret_score' => 1],
+            ['name' => 'Citroen', 'secret_score' => 2],
+        ]);
+
+        SchemaCache::forget('df_test_items');
+    }
+
+    protected function tearDown(): void
+    {
+        Schema::dropIfExists('df_test_items');
+        parent::tearDown();
+    }
+
+    protected function makeFetcher(): DataFetcher
+    {
+        $resource = DataFetcherTestResource::make();
+        $fields = $resource->resolveFields($resource->getModelInstance());
+
+        return new DataFetcher(DataFetcherTestResource::class, $fields);
+    }
+
+    /** @test */
+    public function sql_injectie_payload_in_sortby_wordt_genegeerd_en_breekt_de_tabel_niet(): void
+    {
+        $payload = 'name); DROP TABLE df_test_items;--';
+
+        $result = $this->makeFetcher()->fetch(sortBy: $payload);
+
+        // De payload mag nergens tot een query-fout leiden en de tabel moet
+        // gewoon blijven bestaan (het bewijs dat er geen injectie plaatsvond).
+        $this->assertTrue(Schema::hasTable('df_test_items'));
+        $this->assertSame(3, $result->total());
+    }
+
+    /** @test */
+    public function sorteren_op_een_niet_sortable_veld_wordt_genegeerd_ook_al_bestaat_de_kolom(): void
+    {
+        // "secret_score" bestaat echt in de database, maar is in de resource
+        // bewust niet als sortable() gemarkeerd. De fix moet dit blokkeren,
+        // ook al zou de oude "bestaat de kolom?"-check dit toestaan.
+        $result = $this->makeFetcher()->fetch(sortBy: 'secret_score', sortDirection: 'asc');
+
+        // Zonder sortering blijft de invoegvolgorde (per id) behouden i.p.v.
+        // gesorteerd te worden op secret_score (wat Appel, Citroen, Banaan
+        // zou opleveren).
+        $this->assertSame(['Banaan', 'Appel', 'Citroen'], $this->namesOf($result));
+    }
+
+    /** @test */
+    public function sorteren_op_een_wel_sortable_veld_werkt_gewoon(): void
+    {
+        $result = $this->makeFetcher()->fetch(sortBy: 'name', sortDirection: 'asc');
+
+        $this->assertSame(['Appel', 'Banaan', 'Citroen'], $this->namesOf($result));
+    }
+
+    /**
+     * @param \Illuminate\Contracts\Pagination\Paginator $result
+     * @return array<int, string>
+     */
+    protected function namesOf($result): array
+    {
+        return collect($result->items())
+            ->map(function ($resource) {
+                foreach ($resource->getFields() as $field) {
+                    if ($field->name === 'name') {
+                        return $field->value;
+                    }
+                }
+
+                return null;
+            })
+            ->values()
+            ->all();
+    }
+}