From 329495fc1672dd0310e237b8d68f088eaade2fb6 Mon Sep 17 00:00:00 2001 From: snipe Date: Fri, 7 Aug 2026 17:33:43 +0100 Subject: [PATCH] Fixed RB-21869 - gate activity report types --- .../Controllers/Api/ReportsController.php | 68 ++++++++++--- app/Http/Controllers/Controller.php | 44 +++++++++ .../Api/CheckoutRequestCounterTest.php | 99 +++++++++++++++++++ .../Feature/Reporting/ActivityReportTest.php | 34 +++++++ 4 files changed, 233 insertions(+), 12 deletions(-) create mode 100644 tests/Feature/Checkouts/Api/CheckoutRequestCounterTest.php diff --git a/app/Http/Controllers/Api/ReportsController.php b/app/Http/Controllers/Api/ReportsController.php index 593afdebf6..ae61437a60 100644 --- a/app/Http/Controllers/Api/ReportsController.php +++ b/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; + } } diff --git a/app/Http/Controllers/Controller.php b/app/Http/Controllers/Controller.php index c406e8e561..dfb64b25fd 100644 --- a/app/Http/Controllers/Controller.php +++ b/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; + } } diff --git a/tests/Feature/Checkouts/Api/CheckoutRequestCounterTest.php b/tests/Feature/Checkouts/Api/CheckoutRequestCounterTest.php new file mode 100644 index 0000000000..921b969910 --- /dev/null +++ b/tests/Feature/Checkouts/Api/CheckoutRequestCounterTest.php @@ -0,0 +1,99 @@ +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); + } +} diff --git a/tests/Feature/Reporting/ActivityReportTest.php b/tests/Feature/Reporting/ActivityReportTest.php index bbd6d6ea6f..756c98847a 100644 --- a/tests/Feature/Reporting/ActivityReportTest.php +++ b/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