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); });