From 999598831013e0622bb62901594babf55aa6dcfc Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Mon, 7 Jul 2025 14:15:15 -0700 Subject: [PATCH 1/7] Validate filter is string in asset index --- app/Http/Controllers/Api/AssetsController.php | 4 ++- tests/Feature/Assets/Api/AssetIndexTest.php | 30 +++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/app/Http/Controllers/Api/AssetsController.php b/app/Http/Controllers/Api/AssetsController.php index 160f4a120a20..2fdaff1938d8 100644 --- a/app/Http/Controllers/Api/AssetsController.php +++ b/app/Http/Controllers/Api/AssetsController.php @@ -57,7 +57,9 @@ class AssetsController extends Controller */ public function index(Request $request, $action = null, $upcoming_status = null) : JsonResponse | array { - + $this->validate($request, [ + 'filter' => 'string', + ]); // This handles the legacy audit endpoints :( if ($action == 'audit') { diff --git a/tests/Feature/Assets/Api/AssetIndexTest.php b/tests/Feature/Assets/Api/AssetIndexTest.php index 8554edb3d459..b38841fba3eb 100644 --- a/tests/Feature/Assets/Api/AssetIndexTest.php +++ b/tests/Feature/Assets/Api/AssetIndexTest.php @@ -126,6 +126,36 @@ public function testAssetApiIndexReturnsDueOrOverdueForExpectedCheckin() ->assertJson(fn(AssertableJson $json) => $json->has('rows', 5)->etc()); } + /** + * [RB-19910] + */ + public function test_handles_non_string_filter() + { + $this->actingAsForApi(User::factory()->superuser()->create()) + // url encode an array: filter=[assigned_to] + ->getJson(route('api.assets.index') . '?filter%5Bassigned_to%5D') + ->assertOk() + ->assertStatusMessageIs('error') + ->assertMessagesContains('filter'); + } + + public function test_can_filter_results() + { + Asset::factory()->create(['purchase_date' => '2025-07-01', 'order_number' => '123']); + Asset::factory()->create(['purchase_date' => '2025-07-01', 'order_number' => '123']); + Asset::factory()->create(['purchase_date' => '2025-07-01', 'order_number' => '456']); + + $this->actingAsForApi(User::factory()->superuser()->create()) + ->getJson(route('api.assets.index', [ + 'filter' => json_encode([ + 'order_number' => '123', + 'purchase_date' => '2025-07-01', + ]), + ])) + ->assertOk() + ->assertJson(fn(AssertableJson $json) => $json->has('rows', 2)->etc()); + } + public function testAssetApiIndexAdheresToCompanyScoping() { [$companyA, $companyB] = Company::factory()->count(2)->create(); From cdffbe6fd25e1fb2390296d0cccf71d0cbfc3f94 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Mon, 7 Jul 2025 14:19:45 -0700 Subject: [PATCH 2/7] Add rollbar --- tests/Feature/Assets/Api/AssetIndexTest.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/Feature/Assets/Api/AssetIndexTest.php b/tests/Feature/Assets/Api/AssetIndexTest.php index b38841fba3eb..297736f238f9 100644 --- a/tests/Feature/Assets/Api/AssetIndexTest.php +++ b/tests/Feature/Assets/Api/AssetIndexTest.php @@ -127,6 +127,7 @@ public function testAssetApiIndexReturnsDueOrOverdueForExpectedCheckin() } /** + * [RB-17904] * [RB-19910] */ public function test_handles_non_string_filter() From 28f137f66d89ab412c17d7be031bb78f27b48a6f Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Mon, 7 Jul 2025 14:56:01 -0700 Subject: [PATCH 3/7] More accurate parameter type --- app/Http/Controllers/Api/AssetsController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/Http/Controllers/Api/AssetsController.php b/app/Http/Controllers/Api/AssetsController.php index 2fdaff1938d8..803ab5f7e129 100644 --- a/app/Http/Controllers/Api/AssetsController.php +++ b/app/Http/Controllers/Api/AssetsController.php @@ -58,7 +58,7 @@ class AssetsController extends Controller public function index(Request $request, $action = null, $upcoming_status = null) : JsonResponse | array { $this->validate($request, [ - 'filter' => 'string', + 'filter' => 'json', ]); // This handles the legacy audit endpoints :( From 1438bce6f0f2c48b0ddc95382001f40351f94050 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Mon, 7 Jul 2025 15:18:43 -0700 Subject: [PATCH 4/7] Add other cases --- app/Http/Controllers/Api/AssetsController.php | 16 +++++++++++++++- tests/Feature/Assets/Api/AssetIndexTest.php | 15 +++++++++++++-- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/app/Http/Controllers/Api/AssetsController.php b/app/Http/Controllers/Api/AssetsController.php index 803ab5f7e129..ecbfc87cf9bb 100644 --- a/app/Http/Controllers/Api/AssetsController.php +++ b/app/Http/Controllers/Api/AssetsController.php @@ -9,6 +9,7 @@ use App\Models\AccessoryCheckout; use App\Models\CheckoutAcceptance; use App\Models\LicenseSeat; +use Closure; use Illuminate\Database\Eloquent\Builder; use Illuminate\Http\JsonResponse; use Illuminate\Support\Facades\Crypt; @@ -58,7 +59,20 @@ class AssetsController extends Controller public function index(Request $request, $action = null, $upcoming_status = null) : JsonResponse | array { $this->validate($request, [ - 'filter' => 'json', + 'filter' => [ + 'json', + function (string $attribute, mixed $value, Closure $fail) { + if (is_string($value)) { + $value = json_decode($value, true); + } + + foreach ($value as $arrayValue) { + if (is_array($arrayValue)) { + $fail("{$attribute} cannot contain a nested data."); + } + } + }, + ], ]); // This handles the legacy audit endpoints :( diff --git a/tests/Feature/Assets/Api/AssetIndexTest.php b/tests/Feature/Assets/Api/AssetIndexTest.php index 297736f238f9..188349c91973 100644 --- a/tests/Feature/Assets/Api/AssetIndexTest.php +++ b/tests/Feature/Assets/Api/AssetIndexTest.php @@ -7,6 +7,7 @@ use App\Models\User; use Carbon\Carbon; use Illuminate\Testing\Fluent\AssertableJson; +use PHPUnit\Framework\Attributes\DataProvider; use Tests\TestCase; class AssetIndexTest extends TestCase @@ -126,15 +127,25 @@ public function testAssetApiIndexReturnsDueOrOverdueForExpectedCheckin() ->assertJson(fn(AssertableJson $json) => $json->has('rows', 5)->etc()); } + public static function filterValues() + { + return [ + ['filter%5Bassigned_to%5D'], + ['filter[assigned_to][not]=null'], + ['filter={%22assigned_to%22:{%22$ne%22:null}}'] + ]; + } + /** * [RB-17904] * [RB-19910] */ - public function test_handles_non_string_filter() + #[DataProvider('filterValues')] + public function test_handles_non_string_filter($filterString) { $this->actingAsForApi(User::factory()->superuser()->create()) // url encode an array: filter=[assigned_to] - ->getJson(route('api.assets.index') . '?filter%5Bassigned_to%5D') + ->getJson(route('api.assets.index') . '?' . $filterString) ->assertOk() ->assertStatusMessageIs('error') ->assertMessagesContains('filter'); From 303d665ee6515dca4817f14f57377a7a77370754 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Mon, 7 Jul 2025 16:35:14 -0700 Subject: [PATCH 5/7] Simplify controller --- app/Http/Controllers/Api/AssetsController.php | 18 ++--------- app/Rules/FlatArray.php | 32 +++++++++++++++++++ 2 files changed, 35 insertions(+), 15 deletions(-) create mode 100644 app/Rules/FlatArray.php diff --git a/app/Http/Controllers/Api/AssetsController.php b/app/Http/Controllers/Api/AssetsController.php index ecbfc87cf9bb..8260cacf1021 100644 --- a/app/Http/Controllers/Api/AssetsController.php +++ b/app/Http/Controllers/Api/AssetsController.php @@ -9,7 +9,7 @@ use App\Models\AccessoryCheckout; use App\Models\CheckoutAcceptance; use App\Models\LicenseSeat; -use Closure; +use App\Rules\FlatArray; use Illuminate\Database\Eloquent\Builder; use Illuminate\Http\JsonResponse; use Illuminate\Support\Facades\Crypt; @@ -35,8 +35,6 @@ use Illuminate\Support\Facades\Route; use App\View\Label; use Illuminate\Support\Facades\Storage; -use Illuminate\Support\Facades\Validator; - /** * This class controls all actions related to assets for @@ -58,20 +56,10 @@ class AssetsController extends Controller */ public function index(Request $request, $action = null, $upcoming_status = null) : JsonResponse | array { - $this->validate($request, [ + $request->validate([ 'filter' => [ 'json', - function (string $attribute, mixed $value, Closure $fail) { - if (is_string($value)) { - $value = json_decode($value, true); - } - - foreach ($value as $arrayValue) { - if (is_array($arrayValue)) { - $fail("{$attribute} cannot contain a nested data."); - } - } - }, + new FlatArray, ], ]); diff --git a/app/Rules/FlatArray.php b/app/Rules/FlatArray.php new file mode 100644 index 000000000000..75537df33603 --- /dev/null +++ b/app/Rules/FlatArray.php @@ -0,0 +1,32 @@ + 'b'] + * Fails: ['a' => ['b', 'c']] + */ +class FlatArray implements ValidationRule +{ + /** + * Run the validation rule. + * + * @param \Closure(string, ?string=): \Illuminate\Translation\PotentiallyTranslatedString $fail + */ + public function validate(string $attribute, mixed $value, Closure $fail): void + { + if (is_string($value)) { + $value = json_decode($value, true); + } + + foreach ($value as $arrayValue) { + if (is_array($arrayValue)) { + $fail(":attribute cannot contain a nested data."); + } + } + } +} From 084c1ae64cb961d71a8dff976b22b72dcdb76920 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Mon, 7 Jul 2025 16:35:30 -0700 Subject: [PATCH 6/7] Remove old comment --- tests/Feature/Assets/Api/AssetIndexTest.php | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/Feature/Assets/Api/AssetIndexTest.php b/tests/Feature/Assets/Api/AssetIndexTest.php index 188349c91973..57c4594df8a7 100644 --- a/tests/Feature/Assets/Api/AssetIndexTest.php +++ b/tests/Feature/Assets/Api/AssetIndexTest.php @@ -144,7 +144,6 @@ public static function filterValues() public function test_handles_non_string_filter($filterString) { $this->actingAsForApi(User::factory()->superuser()->create()) - // url encode an array: filter=[assigned_to] ->getJson(route('api.assets.index') . '?' . $filterString) ->assertOk() ->assertStatusMessageIs('error') From d27715b0b9e96f33c6a22df4fa269f1aeb560cdf Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Mon, 7 Jul 2025 16:41:21 -0700 Subject: [PATCH 7/7] Add failing test --- tests/Feature/Assets/Api/AssetIndexTest.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/Feature/Assets/Api/AssetIndexTest.php b/tests/Feature/Assets/Api/AssetIndexTest.php index 57c4594df8a7..d6197e231f0e 100644 --- a/tests/Feature/Assets/Api/AssetIndexTest.php +++ b/tests/Feature/Assets/Api/AssetIndexTest.php @@ -132,7 +132,8 @@ public static function filterValues() return [ ['filter%5Bassigned_to%5D'], ['filter[assigned_to][not]=null'], - ['filter={%22assigned_to%22:{%22$ne%22:null}}'] + ['filter={%22assigned_to%22:{%22$ne%22:null}}'], + ['filter=%5B%22a%22%20%3D%3E%20%22b%22%5D'], ]; }