Fixed RB-21869 - gate activity report types

app/Http/Controllers/Api/ReportsController.php

@@ -31,21 +31,41 @@ class ReportsController extends Controller
      */
     public function index(FilterRequest $request): JsonResponse|array
     {
+        // Resolve incoming item_type / target_type against the
+        // allowlist BEFORE any downstream use. This is both an authz
+        // gate (denies probing arbitrary classes) and a correctness
+        // fix (Helper::normalizeFullModelName uses ucwords() which
+        // mangles CamelCase names, so `licenseseat` used to Fatal on
+        // App\Models\Licenseseat::withTrashed()). resolveActivityReportType
+        // returns the canonical fully-qualified class string or null.
+        $targetClass = null;
+        $itemClass = null;
+
+        if ($request->filled('target_type')) {
+            $targetClass = $this->resolveActivityReportType($request->input('target_type'));
+            if ($targetClass === null) {
+                return response()->json(Helper::formatStandardApiResponse('error', null, 'Invalid target_type'), 400);
+            }
+        }
+        if ($request->filled('item_type')) {
+            $itemClass = $this->resolveActivityReportType($request->input('item_type'));
+            if ($itemClass === null) {
+                return response()->json(Helper::formatStandardApiResponse('error', null, 'Invalid item_type'), 400);
+            }
+        }
 
         // If the user doesn't have permission to view the item or the target,
         // then they shouldn't be able to see the activity log for that item or target,
         // but if they have the general activity view permission,
         // then they can see all activity logs regardless of the item or target.
-        if ((! Gate::allows('activity.view')) && (($request->filled('target_type') && $request->filled('target_id')) || ($request->filled('item_type') && $request->filled('item_id')))) {
+        if ((! Gate::allows('activity.view')) && (($targetClass && $request->filled('target_id')) || ($itemClass && $request->filled('item_id')))) {
 
-            if (($request->filled('target_type')) && ($request->filled('target_id'))) {
-                $targetClass = Helper::normalizeFullModelName(request()->input('target_type'));
+            if ($targetClass && $request->filled('target_id')) {
                 $target = $targetClass::withTrashed()->find(request()->input('target_id'));
                 $this->authorize('view', $target ?? $targetClass);
             }
 
-            if (($request->filled('item_type')) && ($request->filled('item_id'))) {
-                $itemClass = Helper::normalizeFullModelName(request()->input('item_type'));
+            if ($itemClass && $request->filled('item_id')) {
                 $item = $itemClass::withTrashed()->find(request()->input('item_id'));
                 $this->authorize('view', $item ?? $itemClass);
             }
@@ -56,18 +76,18 @@ class ReportsController extends Controller
 
         $actionlogs = Actionlog::with('item', 'user', 'adminuser', 'target', 'location');
 
-        if (($request->filled('target_type')) && ($request->filled('target_id'))) {
+        if ($targetClass && $request->filled('target_id')) {
             $actionlogs = $actionlogs->where('target_id', '=', $request->input('target_id'))
-                ->where('target_type', '=', Helper::normalizeFullModelName($request->input('target_type')));
+                ->where('target_type', '=', $targetClass);
         }
 
-        if (($request->filled('item_type')) && ($request->filled('item_id'))) {
-            $actionlogs = $actionlogs->where(function ($query) use ($request) {
+        if ($itemClass && $request->filled('item_id')) {
+            $actionlogs = $actionlogs->where(function ($query) use ($request, $itemClass) {
                 $query->where('item_id', '=', $request->input('item_id'))
-                    ->where('item_type', '=', Helper::normalizeFullModelName($request->input('item_type')))
-                    ->orWhere(function ($query) use ($request) {
+                    ->where('item_type', '=', $itemClass)
+                    ->orWhere(function ($query) use ($request, $itemClass) {
                         $query->where('target_id', '=', $request->input('item_id'))
-                            ->where('target_type', '=', Helper::normalizeFullModelName($request->input('item_type')));
+                            ->where('target_type', '=', $itemClass);
                     });
             });
         }
@@ -289,4 +309,28 @@ class ReportsController extends Controller
             'prev_label' => $prevStart->format('M j').' – '.$prevEnd->format('M j'),
         ], $datasets));
     }
+
+    /**
+     * Match caller-supplied item_type / target_type input against the
+     * activity-report allowlist and return the canonical class string.
+     * Delegates short-name → FQCN to Helper::normalizeFullModelName so
+     * this endpoint keeps accepting either shape (`asset` or
+     * `App\Models\Asset`), same as every other call site of that helper.
+     * The final match is case-insensitive against the allowlist to
+     * recover from normalizeFullModelName's ucwords() step, which
+     * mangles CamelCase names (`licenseseat` becomes the nonexistent
+     * App\Models\Licenseseat and would have fatal'd downstream).
+     * Null result means the caller should 400.
+     */
+    private function resolveActivityReportType(string $type): ?string
+    {
+        $candidate = Helper::normalizeFullModelName($type);
+        foreach (self::getActivityReportClassAllowlist() as $allowed) {
+            if (strcasecmp($candidate, $allowed) === 0) {
+                return $allowed;
+            }
+        }
+
+        return null;
+    }
 }

app/Http/Controllers/Controller.php

@@ -26,13 +26,16 @@ namespace App\Http\Controllers;
 use App\Models\Accessory;
 use App\Models\Asset;
 use App\Models\AssetModel;
+use App\Models\Category;
 use App\Models\Company;
 use App\Models\Component;
 use App\Models\Consumable;
 use App\Models\Department;
 use App\Models\License;
+use App\Models\LicenseSeat;
 use App\Models\Location;
 use App\Models\Maintenance;
+use App\Models\Manufacturer;
 use App\Models\Supplier;
 use App\Models\User;
 use App\Traits\DisablesDebugbar;
@@ -115,6 +118,38 @@ abstract class Controller extends BaseController
         License::class => 'licenses',
     ];
 
+    /**
+     * Allowlist of fully-qualified model class strings the activity-report
+     * endpoint (Api\ReportsController::index) accepts as item_type /
+     * target_type. This is a security surface: input flows directly into
+     * a runtime class lookup + polymorphic where-clauses. Without a gate,
+     * a caller can probe arbitrary strings and either trigger a Fatal
+     * Error (Helper::normalizeFullModelName + ucwords mangles CamelCase
+     * so `licenseseat` yields `App\Models\Licenseseat`) or route the
+     * authorization check into an unintended class. Kept as a flat list
+     * rather than merged into $map_object_type because the polymorphic
+     * item_type / target_type columns cover more models than the URL
+     * routing map does (Category, LicenseSeat, Manufacturer aren't in
+     * $map_object_type but do appear in action_logs).
+     */
+    public static $activity_report_class_allowlist = [
+        Accessory::class,
+        Asset::class,
+        AssetModel::class,
+        Category::class,
+        Company::class,
+        Component::class,
+        Consumable::class,
+        Department::class,
+        License::class,
+        LicenseSeat::class,
+        Location::class,
+        Maintenance::class,
+        Manufacturer::class,
+        Supplier::class,
+        User::class,
+    ];
+
     public function __construct()
     {
         view()->share('signedIn', Auth::check());
@@ -157,4 +192,13 @@ abstract class Controller extends BaseController
     {
         return static::$map_class_url_segment;
     }
+
+    /**
+     * Accessor for the activity-report allowlist. See getMapObjectType
+     * for rationale.
+     */
+    public static function getActivityReportClassAllowlist(): array
+    {
+        return static::$activity_report_class_allowlist;
+    }
 }

tests/Feature/Checkouts/Api/CheckoutRequestCounterTest.php

@@ -0,0 +1,99 @@
+<?php
+
+namespace Tests\Feature\Checkouts\Api;
+
+use App\Models\Asset;
+use App\Models\User;
+use Illuminate\Support\Facades\Notification;
+use Tests\TestCase;
+
+class CheckoutRequestCounterTest extends TestCase
+{
+    protected function setUp(): void
+    {
+        parent::setUp();
+        Notification::fake();
+    }
+
+    public function test_cancel_without_active_request_returns_404_and_does_not_touch_counter()
+    {
+        // Reg-test for the cancel-request counter drift: hitting the cancel
+        // endpoint when the caller has no active CheckoutRequest used to
+        // unconditionally decrement requests_counter, which drove the
+        // counter negative and misrepresented pending admin work.
+        $asset = Asset::factory()->requestable()->create(['requests_counter' => 0]);
+        $user = User::factory()->create();
+
+        $this->actingAsForApi($user)
+            ->postJson(route('api.assets.requests.destroy', $asset))
+            ->assertStatus(404)
+            ->assertStatusMessageIs('error');
+
+        $this->assertEquals(0, $asset->fresh()->requests_counter);
+    }
+
+    public function test_duplicate_active_request_returns_409_and_increments_counter_only_once()
+    {
+        // Reg-test for duplicate-active-request counter drift: a second
+        // POST from the same caller used to add a second CheckoutRequest
+        // row AND bump requests_counter a second time. On cancel only one
+        // decrement fired, so the counter and the pending queue drifted
+        // apart.
+        $asset = Asset::factory()->requestable()->create(['requests_counter' => 0]);
+        $user = User::factory()->create();
+
+        $this->actingAsForApi($user)
+            ->postJson(route('api.assets.requests.store', $asset))
+            ->assertOk()
+            ->assertStatusMessageIs('success');
+
+        $this->actingAsForApi($user)
+            ->postJson(route('api.assets.requests.store', $asset))
+            ->assertStatus(409)
+            ->assertStatusMessageIs('error');
+
+        $this->assertEquals(1, $asset->fresh()->requests_counter);
+        $this->assertEquals(
+            1,
+            $asset->requests()->whereNull('canceled_at')->where('user_id', $user->id)->count(),
+            'Second request should not have created a second active CheckoutRequest row.'
+        );
+    }
+
+    public function test_cancel_after_active_request_decrements_counter_by_exactly_one()
+    {
+        // Companion to the two above: a legitimate request-then-cancel
+        // round trip must leave the counter at 0.
+        $asset = Asset::factory()->requestable()->create(['requests_counter' => 0]);
+        $user = User::factory()->create();
+
+        $this->actingAsForApi($user)
+            ->postJson(route('api.assets.requests.store', $asset))
+            ->assertOk();
+
+        $this->assertEquals(1, $asset->fresh()->requests_counter);
+
+        $this->actingAsForApi($user)
+            ->postJson(route('api.assets.requests.destroy', $asset))
+            ->assertOk()
+            ->assertStatusMessageIs('success');
+
+        $this->assertEquals(0, $asset->fresh()->requests_counter);
+    }
+
+    public function test_double_cancel_only_decrements_counter_once()
+    {
+        // Combined regression: request once, cancel twice. The second
+        // cancel must 404 without dragging the counter below zero.
+        $asset = Asset::factory()->requestable()->create(['requests_counter' => 0]);
+        $user = User::factory()->create();
+
+        $this->actingAsForApi($user)->postJson(route('api.assets.requests.store', $asset))->assertOk();
+        $this->actingAsForApi($user)->postJson(route('api.assets.requests.destroy', $asset))->assertOk();
+        $this->actingAsForApi($user)
+            ->postJson(route('api.assets.requests.destroy', $asset))
+            ->assertStatus(404);
+
+        $this->assertEquals(0, $asset->fresh()->requests_counter);
+    }
+}

tests/Feature/Reporting/ActivityReportTest.php

@@ -189,6 +189,40 @@ class ActivityReportTest extends TestCase
 
     }
 
+    public function test_activity_report_normalizes_lowercase_camelcase_input()
+    {
+        // Reg-test for the pre-existing `licenseseat` Fatal Error:
+        // Helper::normalizeFullModelName uses ucwords(), which only
+        // capitalizes the first letter of each space-delimited word.
+        // A lowercase short name like `licenseseat` (which FilterRequest
+        // accepts) came out as the nonexistent App\Models\Licenseseat
+        // and Fatal'd when withTrashed()->find() called the class. The
+        // resolver's case-insensitive lookup now returns the canonical
+        // App\Models\LicenseSeat, so the request succeeds cleanly.
+        $this->actingAsForApi(User::factory()->superuser()->create())
+            ->getJson(route('api.activity.index', [
+                'item_type' => 'licenseseat',
+                'item_id' => 999999,
+            ]))
+            ->assertOk();
+    }
+
+    public function test_activity_report_rejects_types_not_in_form_request_allowlist()
+    {
+        // FilterRequest already rejects arbitrary class names, but
+        // Snipe-IT returns validation failures as HTTP 200 with body
+        // status=error (project convention). Pinning that shape so a
+        // refactor that changes either FilterRequest or the response
+        // envelope shows up in tests before it ships.
+        $this->actingAsForApi(User::factory()->superuser()->create())
+            ->getJson(route('api.activity.index', [
+                'item_type' => 'NotARealClass',
+                'item_id' => 1,
+            ]))
+            ->assertOk()
+            ->assertStatusMessageIs('error');
+    }
+
     public function test_search_matches_action_log_location_name()
     {
         // Activity Report eager-loads and shows the location on each