From 63637ddebcca73350384fd773deaa094b8f99a0c Mon Sep 17 00:00:00 2001 From: "Ronald A. Richardson" Date: Wed, 14 Oct 2026 17:54:07 +0800 Subject: [PATCH] fix(manifests): scope manifest and stop lookups to the caller's company The public API and console manifest controllers resolved manifests, stops and drivers by identifier alone, so any authenticated organization could read another's manifests and cancel, delete, re-sequence or complete its stops. Every lookup is now constrained to the session company; stops, which carry no company of their own, are owned through their manifest. A missing company session resolves nothing. Fixes GHSA-m8m5-5qhf-94rv and GHSA-q479-qp9q-mg27. --- .../Controllers/Api/v1/ManifestController.php | 55 ++++++++- .../Internal/v1/ManifestController.php | 31 ++++- .../ApiManifestControllerContractsTest.php | 113 ++++++++++++++---- .../tests/ManifestControllerContractsTest.php | 71 ++++++++++- 4 files changed, 237 insertions(+), 33 deletions(-) diff --git a/server/src/Http/Controllers/Api/v1/ManifestController.php b/server/src/Http/Controllers/Api/v1/ManifestController.php index 9a4bc3d13..32fa15344 100644 --- a/server/src/Http/Controllers/Api/v1/ManifestController.php +++ b/server/src/Http/Controllers/Api/v1/ManifestController.php @@ -9,6 +9,7 @@ use Fleetbase\FleetOps\Models\ManifestStop; use Fleetbase\FleetOps\Support\Utils; use Fleetbase\Http\Controllers\Controller; +use Illuminate\Database\Eloquent\Builder; use Illuminate\Http\Request; /** @@ -189,24 +190,70 @@ public function optimize(Request $request, string $id) /** The driver's manifests, newest first. Separated so it can be stubbed. */ protected static function manifestsFor(Driver $driver) { - return Manifest::where('driver_uuid', $driver->uuid) + return static::scopedToCompany(Manifest::where('driver_uuid', $driver->uuid)) ->with(['driver', 'vehicle']) ->orderBy('scheduled_date', 'desc'); } protected static function findDriverRecord(string $id): ?Driver { - return Driver::where('public_id', $id)->orWhere('uuid', $id)->first(); + /** @var Driver|null $driver */ + $driver = static::scopedToCompany(static::matchingIdentifier(Driver::query(), $id))->first(); + + return $driver; } protected static function findManifest(string $id): ?Manifest { - return Manifest::where('public_id', $id)->orWhere('uuid', $id)->first(); + /** @var Manifest|null $manifest */ + $manifest = static::scopedToCompany(static::matchingIdentifier(Manifest::query(), $id))->first(); + + return $manifest; } + /** + * A stop carries no company of its own, so it belongs to the caller only + * through its manifest. + */ protected static function findStop(string $id): ?ManifestStop { - return ManifestStop::where('public_id', $id)->orWhere('uuid', $id)->first(); + /** @var ManifestStop|null $stop */ + $stop = static::matchingIdentifier(ManifestStop::query(), $id) + ->whereHas('manifest', fn (Builder $query) => static::scopedToCompany($query)) + ->first(); + + return $stop; + } + + /** + * Match a public_id or uuid, grouped so a constraint added afterwards + * applies to both arms — ungrouped it would read as `public_id = ? OR + * (uuid = ? AND company_uuid = ?)` and still resolve another + * organization's records. + */ + protected static function matchingIdentifier(Builder $query, string $id): Builder + { + return $query->where(fn (Builder $query) => $query->where('public_id', $id)->orWhere('uuid', $id)); + } + + /** + * Constrain a query to the caller's company. + * + * Nothing upstream does this: there is no global company scope, and the API + * middleware authenticates the caller without narrowing which records an + * identifier may resolve to. A missing company session fails closed instead + * of letting the query run across every tenant. + */ + protected static function scopedToCompany(Builder $query): Builder + { + $companyUuid = session('company'); + if (!$companyUuid) { + $query->whereRaw('1 = 0'); + + return $query; + } + + return $query->where($query->getModel()->qualifyColumn('company_uuid'), $companyUuid); } /** diff --git a/server/src/Http/Controllers/Internal/v1/ManifestController.php b/server/src/Http/Controllers/Internal/v1/ManifestController.php index 42329337d..9cc2afd3e 100644 --- a/server/src/Http/Controllers/Internal/v1/ManifestController.php +++ b/server/src/Http/Controllers/Internal/v1/ManifestController.php @@ -6,6 +6,7 @@ use Fleetbase\FleetOps\Models\ManifestStop; use Fleetbase\FleetOps\Traits\AuthorizesMethods; use Fleetbase\Http\Controllers\Controller; +use Illuminate\Database\Eloquent\Builder; use Illuminate\Http\JsonResponse; use Illuminate\Http\Request; @@ -158,12 +159,38 @@ protected function manifestQueryForCompany(?string $companyUuid) protected function manifestQueryByPublicId(string $id) { - return Manifest::where('public_id', $id); + return $this->scopedToCompany(Manifest::where('public_id', $id)); } + /** + * A stop carries no company of its own, so it belongs to the caller only + * through its manifest. + */ protected function manifestStopQueryByPublicId(string $id) { - return ManifestStop::where('public_id', $id); + return ManifestStop::where('public_id', $id) + ->whereHas('manifest', fn (Builder $query) => $this->scopedToCompany($query)); + } + + /** + * Constrain a query to the caller's company. + * + * The record arrives as a route identifier, and nothing upstream narrows + * which tenant it may resolve to: there is no global company scope, and + * `fleetbase.protected` only checks that the caller holds the named + * capability. A missing company session fails closed instead of letting + * the query run across every tenant. + */ + protected function scopedToCompany(Builder $query): Builder + { + $companyUuid = session('company'); + if (!$companyUuid) { + $query->whereRaw('1 = 0'); + + return $query; + } + + return $query->where($query->getModel()->qualifyColumn('company_uuid'), $companyUuid); } protected function manifestShowRelations(): array diff --git a/server/tests/ApiManifestControllerContractsTest.php b/server/tests/ApiManifestControllerContractsTest.php index 8f31651ac..75533046a 100644 --- a/server/tests/ApiManifestControllerContractsTest.php +++ b/server/tests/ApiManifestControllerContractsTest.php @@ -570,30 +570,99 @@ public static function callManifestsFor(Driver $driver) } } -test('manifest controller lookups go to the database', function () { +/** + * An in-memory database holding two companies' manifests, stops and drivers, + * so the lookups are asserted against real queries rather than a stub. + */ +function fleetopsManifestTenantDatabase(): Illuminate\Database\SQLiteConnection +{ + $connection = new Illuminate\Database\SQLiteConnection(new PDO('sqlite::memory:'), ':memory:', '', ['driver' => 'sqlite']); + $resolver = new Illuminate\Database\ConnectionResolver(['default' => $connection, 'mysql' => $connection]); + $resolver->setDefaultConnection('mysql'); + Illuminate\Database\Eloquent\Model::setConnectionResolver($resolver); + + $schema = $connection->getSchemaBuilder(); + $tables = [ + 'manifests' => ['uuid', 'public_id', 'company_uuid', 'driver_uuid', 'vehicle_uuid', 'status', 'scheduled_date'], + 'manifest_stops' => ['uuid', 'public_id', 'manifest_uuid', 'status', 'sequence'], + 'drivers' => ['uuid', 'public_id', 'company_uuid', 'user_uuid'], + 'users' => ['uuid', 'public_id', 'company_uuid', 'name'], + ]; + foreach ($tables as $table => $columns) { + $schema->create($table, function ($blueprint) use ($columns) { + $blueprint->increments('id'); + foreach ($columns as $column) { + $blueprint->string($column)->nullable(); + } + $blueprint->timestamps(); + $blueprint->timestamp('deleted_at')->nullable(); + }); + } + + foreach (['a', 'b'] as $tenant) { + $connection->table('users')->insert(['uuid' => "user-{$tenant}", 'company_uuid' => "company-{$tenant}", 'name' => "Driver {$tenant}"]); + $connection->table('drivers')->insert(['uuid' => "driver-{$tenant}-uuid", 'public_id' => "driver_{$tenant}", 'company_uuid' => "company-{$tenant}", 'user_uuid' => "user-{$tenant}"]); + $connection->table('manifests')->insert(['uuid' => "manifest-{$tenant}-uuid", 'public_id' => "manifest_{$tenant}", 'company_uuid' => "company-{$tenant}", 'driver_uuid' => "driver-{$tenant}-uuid", 'status' => 'pending']); + $connection->table('manifest_stops')->insert(['uuid' => "stop-{$tenant}-uuid", 'public_id' => "stop_{$tenant}", 'manifest_uuid' => "manifest-{$tenant}-uuid", 'status' => 'pending', 'sequence' => '1']); + } + + return $connection; +} + +test('manifest controller lookups resolve the caller\'s own records by public id or uuid', function () { + fleetopsManifestTenantDatabase(); + session(['company' => 'company-a']); + + $driver = FleetOpsManifestRealLookupProbe::callFindDriverRecord('driver_a'); + + expect(FleetOpsManifestRealLookupProbe::callFindManifest('manifest_a')?->uuid)->toBe('manifest-a-uuid') + ->and(FleetOpsManifestRealLookupProbe::callFindManifest('manifest-a-uuid')?->public_id)->toBe('manifest_a') + ->and(FleetOpsManifestRealLookupProbe::callFindStop('stop_a')?->uuid)->toBe('stop-a-uuid') + ->and(FleetOpsManifestRealLookupProbe::callFindStop('stop-a-uuid')?->public_id)->toBe('stop_a') + ->and($driver?->uuid)->toBe('driver-a-uuid') + ->and(FleetOpsManifestRealLookupProbe::callFindDriverRecord('driver-a-uuid')?->public_id)->toBe('driver_a') + ->and(FleetOpsManifestRealLookupProbe::callManifestsFor($driver)->pluck('uuid')->all())->toBe(['manifest-a-uuid']); +}); + +test('manifest controller lookups never resolve another organization\'s records', function () { /* - * Exercises the real lookups, which the probe above replaces. Whether the - * environment has a database or not, the call must reach it: a lookup that - * quietly returned null on a connection failure would make a broken - * database indistinguishable from a missing record, and both would answer - * 404. Either outcome is accepted here — what is asserted is that the - * lookup is a query and not a stub. + * GHSA-m8m5-5qhf-94rv: every lookup used to be a bare `public_id OR uuid` + * match, so company A could read company B's manifests and write to its + * stops. Both identifier forms are tried, because an ungrouped OR would + * let one of them slip past the company constraint. */ - $driver = new Driver(); - $driver->setRawAttributes(['uuid' => 'driver-uuid', 'public_id' => 'driver_a'], true); - - $reached = function (callable $lookup): bool { - try { - $lookup(); - } catch (Throwable $e) { - return true; - } + fleetopsManifestTenantDatabase(); + session(['company' => 'company-a']); + + $foreignDriver = new Driver(); + $foreignDriver->setRawAttributes(['uuid' => 'driver-b-uuid', 'public_id' => 'driver_b'], true); + + expect(FleetOpsManifestRealLookupProbe::callFindManifest('manifest_b'))->toBeNull() + ->and(FleetOpsManifestRealLookupProbe::callFindManifest('manifest-b-uuid'))->toBeNull() + ->and(FleetOpsManifestRealLookupProbe::callFindStop('stop_b'))->toBeNull() + ->and(FleetOpsManifestRealLookupProbe::callFindStop('stop-b-uuid'))->toBeNull() + ->and(FleetOpsManifestRealLookupProbe::callFindDriverRecord('driver_b'))->toBeNull() + ->and(FleetOpsManifestRealLookupProbe::callFindDriverRecord('driver-b-uuid'))->toBeNull() + ->and(FleetOpsManifestRealLookupProbe::callManifestsFor($foreignDriver)->get())->toBeEmpty(); +}); - return true; - }; +test('manifest controller refuses another organization\'s manifest and stop at the endpoints', function () { + fleetopsManifestTenantDatabase(); + session(['company' => 'company-a']); + + $controller = new ManifestController(); + + expect($controller->show('manifest_b')->getStatusCode())->toBe(404) + ->and($controller->optimize(new Request(), 'manifest_b')->getStatusCode())->toBe(404) + ->and($controller->forDriver(new Request(), 'driver_b')->getStatusCode())->toBe(404) + ->and($controller->updateStop(new Request(['status' => 'completed', 'meta' => ['tampered' => true]]), 'stop_b')->getStatusCode())->toBe(404); +}); + +test('manifest controller lookups resolve nothing without a company session', function () { + fleetopsManifestTenantDatabase(); + session(['company' => null]); - expect($reached(fn () => FleetOpsManifestRealLookupProbe::callFindManifest('manifest_a')))->toBeTrue() - ->and($reached(fn () => FleetOpsManifestRealLookupProbe::callFindStop('stop_a')))->toBeTrue() - ->and($reached(fn () => FleetOpsManifestRealLookupProbe::callFindDriverRecord('driver_a')))->toBeTrue() - ->and($reached(fn () => FleetOpsManifestRealLookupProbe::callManifestsFor($driver)))->toBeTrue(); + expect(FleetOpsManifestRealLookupProbe::callFindManifest('manifest_a'))->toBeNull() + ->and(FleetOpsManifestRealLookupProbe::callFindStop('stop_a'))->toBeNull() + ->and(FleetOpsManifestRealLookupProbe::callFindDriverRecord('driver_a'))->toBeNull(); }); diff --git a/server/tests/ManifestControllerContractsTest.php b/server/tests/ManifestControllerContractsTest.php index 393953962..bd57c75f1 100644 --- a/server/tests/ManifestControllerContractsTest.php +++ b/server/tests/ManifestControllerContractsTest.php @@ -241,7 +241,13 @@ function fleetopsManifestStopFake(): FleetOpsManifestStopFake ->and($controller->stopQuery->record->freshes[0])->toBe(['place', 'order.trackingNumber']); }); -test('manifest real query helpers and stop endpoints hit the database', function () { +/** + * An in-memory database for the controller's real queries. Rows for a second + * company are seeded alongside the caller's so every lookup is also asserted + * not to cross between them. + */ +function fleetopsManifestDatabase(): Illuminate\Database\SQLiteConnection +{ $connection = new Illuminate\Database\SQLiteConnection(new PDO('sqlite::memory:')); $resolver = new Illuminate\Database\ConnectionResolver(['default' => $connection, 'mysql' => $connection]); $resolver->setDefaultConnection('mysql'); @@ -252,7 +258,7 @@ function fleetopsManifestStopFake(): FleetOpsManifestStopFake $schema = $connection->getSchemaBuilder(); $tables = [ 'manifests' => ['uuid', 'public_id', 'company_uuid', 'driver_uuid', 'vehicle_uuid', 'status', 'name', 'date', 'meta', '_key'], - 'manifest_stops' => ['uuid', 'public_id', 'company_uuid', 'manifest_uuid', 'place_uuid', 'order_uuid', 'waypoint_uuid', 'status', 'sequence', 'notes', 'actual_arrival', 'meta', '_key'], + 'manifest_stops' => ['uuid', 'public_id', 'manifest_uuid', 'place_uuid', 'order_uuid', 'waypoint_uuid', 'status', 'sequence', 'notes', 'actual_arrival', 'meta', '_key'], 'places' => ['uuid', 'public_id', 'company_uuid', 'name', 'location', '_key'], 'orders' => ['uuid', 'public_id', 'company_uuid', 'payload_uuid', 'tracking_number_uuid', 'status', 'meta', '_key'], 'payloads' => ['uuid', 'public_id', 'company_uuid', 'pickup_uuid', 'dropoff_uuid', 'meta', '_key'], @@ -273,7 +279,6 @@ function fleetopsManifestStopFake(): FleetOpsManifestStopFake }); } - session(['company' => 'company-mfr-1']); app()->instance('responsecache', new class { public function __call($method, $arguments) { @@ -284,9 +289,19 @@ public function __call($method, $arguments) config()->set('activitylog.default_auth_driver', 'web'); app()->bind(Illuminate\Contracts\Config\Repository::class, fn () => config()); $connection->table('manifests')->insert(['uuid' => 'manifest-real-1', 'public_id' => 'manifest_realone', 'company_uuid' => 'company-mfr-1', 'status' => 'active']); - $connection->table('manifest_stops')->insert(['uuid' => 'stop-real-1', 'public_id' => 'manifest_stop_realone', 'company_uuid' => 'company-mfr-1', 'manifest_uuid' => 'manifest-real-1', 'place_uuid' => 'place-mfr-1', 'status' => 'pending', 'sequence' => '1', 'meta' => json_encode([])]); + $connection->table('manifest_stops')->insert(['uuid' => 'stop-real-1', 'public_id' => 'manifest_stop_realone', 'manifest_uuid' => 'manifest-real-1', 'place_uuid' => 'place-mfr-1', 'status' => 'pending', 'sequence' => '1', 'meta' => json_encode([])]); $connection->table('places')->insert(['uuid' => 'place-mfr-1', 'company_uuid' => 'company-mfr-1', 'name' => 'Manifest Stop Place']); + $connection->table('manifests')->insert(['uuid' => 'manifest-foreign-1', 'public_id' => 'manifest_foreign', 'company_uuid' => 'company-mfr-2', 'status' => 'active']); + $connection->table('manifest_stops')->insert(['uuid' => 'stop-foreign-1', 'public_id' => 'manifest_stop_foreign', 'manifest_uuid' => 'manifest-foreign-1', 'status' => 'pending', 'sequence' => '1']); + + return $connection; +} + +test('manifest real query helpers and stop endpoints hit the database', function () { + $connection = fleetopsManifestDatabase(); + session(['company' => 'company-mfr-1']); + $controller = new ManifestController(); $helper = function (string $method, ...$arguments) use ($controller) { $reflection = new ReflectionMethod(ManifestController::class, $method); @@ -306,5 +321,51 @@ public function __call($method, $arguments) // Sequence-only updates persist without status transitions $updated = $controller->updateStop(new Request(['sequence' => 7]), 'manifest_stop_realone')->getData(true); - expect((string) $connection->table('manifest_stops')->value('sequence'))->toBe('7'); + expect((string) $connection->table('manifest_stops')->where('uuid', 'stop-real-1')->value('sequence'))->toBe('7'); +}); + +test('manifest endpoints never resolve another organization\'s manifest or stop', function () { + /* + * GHSA-q479-qp9q-mg27: the single-record handlers matched on public_id + * alone, so any organization could read, cancel, delete and rewrite + * another's manifests. Stops have no company of their own and are owned + * through their manifest. + */ + $connection = fleetopsManifestDatabase(); + session(['company' => 'company-mfr-1']); + + $controller = new ManifestController(); + $refused = function (callable $call): bool { + try { + $call(); + } catch (Illuminate\Database\Eloquent\ModelNotFoundException $e) { + return true; + } + + return false; + }; + + expect($refused(fn () => $controller->show('manifest_foreign')))->toBeTrue() + ->and($refused(fn () => $controller->cancel('manifest_foreign')))->toBeTrue() + ->and($refused(fn () => $controller->destroy('manifest_foreign')))->toBeTrue() + ->and($refused(fn () => $controller->showStop('manifest_stop_foreign')))->toBeTrue() + ->and($refused(fn () => $controller->updateStop(new Request(['status' => 'completed', 'sequence' => 9]), 'manifest_stop_foreign')))->toBeTrue() + ->and($connection->table('manifests')->where('uuid', 'manifest-foreign-1')->first())->toMatchArray(['status' => 'active', 'deleted_at' => null]) + ->and($connection->table('manifest_stops')->where('uuid', 'stop-foreign-1')->first())->toMatchArray(['status' => 'pending', 'sequence' => '1']); +}); + +test('manifest lookups resolve nothing without a company session', function () { + fleetopsManifestDatabase(); + session(['company' => null]); + + $controller = new ManifestController(); + $helper = function (string $method, ...$arguments) use ($controller) { + $reflection = new ReflectionMethod(ManifestController::class, $method); + $reflection->setAccessible(true); + + return $reflection->invoke($controller, ...$arguments); + }; + + expect($helper('manifestQueryByPublicId', 'manifest_realone')->count())->toBe(0) + ->and($helper('manifestStopQueryByPublicId', 'manifest_stop_realone')->count())->toBe(0); });