diff --git a/server/src/Http/Controllers/Api/v1/EntityController.php b/server/src/Http/Controllers/Api/v1/EntityController.php index 0a3d166e6..bc8203920 100644 --- a/server/src/Http/Controllers/Api/v1/EntityController.php +++ b/server/src/Http/Controllers/Api/v1/EntityController.php @@ -271,7 +271,7 @@ protected function singularize(?string $value): ?string return $value ? Utils::singularize($value) : null; } - protected function findPayloadByPublicId(string $publicId): ?Payload + protected function findPayloadByPublicId(?string $publicId): ?Payload { return Payload::where('public_id', $publicId)->first(); } diff --git a/server/src/Http/Controllers/Api/v1/FuelReportController.php b/server/src/Http/Controllers/Api/v1/FuelReportController.php index 3d316cf15..91b31f8e1 100644 --- a/server/src/Http/Controllers/Api/v1/FuelReportController.php +++ b/server/src/Http/Controllers/Api/v1/FuelReportController.php @@ -157,7 +157,7 @@ public function delete($id) return $this->deletedFuelReportResource($fuelReport); } - protected function findDriverRecord(string $id): Driver + protected function findDriverRecord(?string $id): Driver { return Driver::findRecordOrFail($id); } diff --git a/server/src/Http/Controllers/Api/v1/IssueController.php b/server/src/Http/Controllers/Api/v1/IssueController.php index 3d6155f90..55f6d7e39 100644 --- a/server/src/Http/Controllers/Api/v1/IssueController.php +++ b/server/src/Http/Controllers/Api/v1/IssueController.php @@ -158,7 +158,7 @@ public function delete($id) return $this->deletedIssueResource($issue); } - protected function findDriverRecord(string $id): Driver + protected function findDriverRecord(?string $id): Driver { return Driver::findRecordOrFail($id); } diff --git a/server/src/Http/Controllers/Api/v1/OrderController.php b/server/src/Http/Controllers/Api/v1/OrderController.php index 152479dec..bc4054d9a 100644 --- a/server/src/Http/Controllers/Api/v1/OrderController.php +++ b/server/src/Http/Controllers/Api/v1/OrderController.php @@ -1873,7 +1873,7 @@ protected function newPayload(): Payload return new Payload(); } - protected function findDriverByPublicId(string $publicId): ?Driver + protected function findDriverByPublicId(?string $publicId): ?Driver { return Driver::where(['public_id' => $publicId, 'company_uuid' => session('company')])->first(); } diff --git a/server/src/Http/Controllers/Api/v1/ServiceAreaController.php b/server/src/Http/Controllers/Api/v1/ServiceAreaController.php index f8726fd7c..b82fe4185 100644 --- a/server/src/Http/Controllers/Api/v1/ServiceAreaController.php +++ b/server/src/Http/Controllers/Api/v1/ServiceAreaController.php @@ -218,7 +218,7 @@ protected function createBorderFromPoint(Point $point, int $radius) return ServiceArea::createMultiPolygonFromPoint($point, $radius); } - protected function serviceAreaUuid(string $publicId, array $where): ?string + protected function serviceAreaUuid(?string $publicId, array $where): ?string { return Utils::getUuid('service_areas', $where); } diff --git a/server/src/Http/Controllers/Api/v1/TrackingStatusController.php b/server/src/Http/Controllers/Api/v1/TrackingStatusController.php index 6a80f23a6..153c85d1c 100644 --- a/server/src/Http/Controllers/Api/v1/TrackingStatusController.php +++ b/server/src/Http/Controllers/Api/v1/TrackingStatusController.php @@ -187,7 +187,7 @@ protected function getTrackingNumberUuid(string $table, array $where): ?string return Utils::getUuid($table, $where); } - protected function getOrderTrackingNumberUuid(string $orderId): ?string + protected function getOrderTrackingNumberUuid(?string $orderId): ?string { return Order::where('public_id', $orderId)->value('tracking_number_uuid'); } diff --git a/server/src/Http/Controllers/Api/v1/VehicleController.php b/server/src/Http/Controllers/Api/v1/VehicleController.php index 55d53e237..ffaff5dcd 100644 --- a/server/src/Http/Controllers/Api/v1/VehicleController.php +++ b/server/src/Http/Controllers/Api/v1/VehicleController.php @@ -492,7 +492,7 @@ protected function findVehicle(string $id): Vehicle return Vehicle::findRecordOrFail($id); } - protected function findDriver(string $id): Driver + protected function findDriver(?string $id): Driver { return Driver::findRecordOrFail($id); } diff --git a/server/src/Http/Controllers/Api/v1/ZoneController.php b/server/src/Http/Controllers/Api/v1/ZoneController.php index dba4e5af2..51adfe76c 100644 --- a/server/src/Http/Controllers/Api/v1/ZoneController.php +++ b/server/src/Http/Controllers/Api/v1/ZoneController.php @@ -212,7 +212,7 @@ protected function radiusFromRequest(Request $request): int return (int) $request->input('radius', 500); } - protected function serviceAreaUuid(string $publicId, array $where): ?string + protected function serviceAreaUuid(?string $publicId, array $where): ?string { return Utils::getUuid('service_areas', $where); } diff --git a/server/src/Http/Requests/CreateServiceRateRequest.php b/server/src/Http/Requests/CreateServiceRateRequest.php index 304eafe4e..41122c58f 100644 --- a/server/src/Http/Requests/CreateServiceRateRequest.php +++ b/server/src/Http/Requests/CreateServiceRateRequest.php @@ -25,8 +25,13 @@ public function rules(): array return [ 'service_name' => [Rule::requiredIf($this->isMethod('POST')), 'string'], 'service_type' => [Rule::requiredIf($this->isMethod('POST')), 'string'], - 'service_area' => [Rule::exists('service_areas', 'public_id')->whereNull('deleted_at')], - 'zone' => [Rule::exists('zones', 'public_id')->whereNull('deleted_at')], + // `nullable` so an empty relationship is treated as absent rather than + // as an invalid one. Both are optional, but without it `exists` runs + // against the null that ConvertEmptyStringsToNull produced and answers + // "The selected service area is invalid" for a field the caller simply + // did not fill in. + 'service_area' => ['nullable', Rule::exists('service_areas', 'public_id')->whereNull('deleted_at')], + 'zone' => ['nullable', Rule::exists('zones', 'public_id')->whereNull('deleted_at')], 'rate_calculation_method' => [Rule::requiredIf($this->isMethod('POST')), 'string', 'in:fixed_meter,fixed_rate,per_meter,per_drop,algo,parcel'], 'currency' => ['required', 'size:3'], 'base_fee' => ['numeric'], diff --git a/server/tests/ApiEntityControllerContractsTest.php b/server/tests/ApiEntityControllerContractsTest.php index 637274972..4f6efc5b8 100644 --- a/server/tests/ApiEntityControllerContractsTest.php +++ b/server/tests/ApiEntityControllerContractsTest.php @@ -43,7 +43,7 @@ protected function singularize(?string $value): ?string return $value === 'contacts' ? 'contact' : null; } - protected function findPayloadByPublicId(string $publicId): ?Payload + protected function findPayloadByPublicId(?string $publicId): ?Payload { $this->payload?->setAttribute('lookup_id', $publicId); diff --git a/server/tests/ApiFuelReportControllerContractsTest.php b/server/tests/ApiFuelReportControllerContractsTest.php index 3083a5fc3..ec22cc016 100644 --- a/server/tests/ApiFuelReportControllerContractsTest.php +++ b/server/tests/ApiFuelReportControllerContractsTest.php @@ -17,7 +17,7 @@ class FleetOpsApiFuelReportControllerProbe extends FuelReportController public bool $driverNotFound = false; public bool $fuelReportNotFound = false; - protected function findDriverRecord(string $id): Driver + protected function findDriverRecord(?string $id): Driver { if ($this->driverNotFound) { throw new ModelNotFoundException(); diff --git a/server/tests/ApiIssueControllerContractsTest.php b/server/tests/ApiIssueControllerContractsTest.php index 3492a80e6..167c1decf 100644 --- a/server/tests/ApiIssueControllerContractsTest.php +++ b/server/tests/ApiIssueControllerContractsTest.php @@ -17,7 +17,7 @@ class FleetOpsApiIssueControllerProbe extends IssueController public bool $driverNotFound = false; public bool $issueNotFound = false; - protected function findDriverRecord(string $id): Driver + protected function findDriverRecord(?string $id): Driver { if ($this->driverNotFound) { throw new ModelNotFoundException(); diff --git a/server/tests/ApiOrderControllerContractsTest.php b/server/tests/ApiOrderControllerContractsTest.php index fb6860b64..cd5c8a4ff 100644 --- a/server/tests/ApiOrderControllerContractsTest.php +++ b/server/tests/ApiOrderControllerContractsTest.php @@ -80,7 +80,7 @@ protected function newPayload(): Payload return $this->payload = new FleetOpsApiOrderPayloadFake(); } - protected function findDriverByPublicId(string $publicId): ?Fleetbase\FleetOps\Models\Driver + protected function findDriverByPublicId(?string $publicId): ?Fleetbase\FleetOps\Models\Driver { $this->driver ??= new FleetOpsApiOrderDriverFake(); $this->driver->setRawAttributes([ diff --git a/server/tests/ApiServiceAreaZoneControllerContractsTest.php b/server/tests/ApiServiceAreaZoneControllerContractsTest.php index 8809e21a5..bd32bd95b 100644 --- a/server/tests/ApiServiceAreaZoneControllerContractsTest.php +++ b/server/tests/ApiServiceAreaZoneControllerContractsTest.php @@ -23,7 +23,7 @@ class FleetOpsApiServiceAreaControllerProbe extends ApiServiceAreaController public bool $notFound = false; public bool $createThrows = false; - protected function serviceAreaUuid(string $publicId, array $where): ?string + protected function serviceAreaUuid(?string $publicId, array $where): ?string { $this->uuidLookups[] = [$publicId, $where]; @@ -116,7 +116,7 @@ class FleetOpsApiZoneControllerProbe extends ApiZoneController public mixed $queryResults = null; public bool $notFound = false; - protected function serviceAreaUuid(string $publicId, array $where): ?string + protected function serviceAreaUuid(?string $publicId, array $where): ?string { $this->uuidLookups[] = [$publicId, $where]; diff --git a/server/tests/ApiTrackingStatusControllerContractsTest.php b/server/tests/ApiTrackingStatusControllerContractsTest.php index e4682006b..f925820c4 100644 --- a/server/tests/ApiTrackingStatusControllerContractsTest.php +++ b/server/tests/ApiTrackingStatusControllerContractsTest.php @@ -32,7 +32,7 @@ protected function getTrackingNumberUuid(string $table, array $where): ?string return 'tracking-number-uuid'; } - protected function getOrderTrackingNumberUuid(string $orderId): ?string + protected function getOrderTrackingNumberUuid(?string $orderId): ?string { $this->orderLookups[] = $orderId; diff --git a/server/tests/ApiVehicleControllerContractsTest.php b/server/tests/ApiVehicleControllerContractsTest.php index 88c77f948..92a05837e 100644 --- a/server/tests/ApiVehicleControllerContractsTest.php +++ b/server/tests/ApiVehicleControllerContractsTest.php @@ -70,7 +70,7 @@ protected function findVehicle(string $id): Vehicle return $this->vehicle; } - protected function findDriver(string $id): Driver + protected function findDriver(?string $id): Driver { if ($this->driverNotFound) { throw new ModelNotFoundException(); diff --git a/server/tests/Feature/Http/Api/NullRelationshipInputTest.php b/server/tests/Feature/Http/Api/NullRelationshipInputTest.php new file mode 100644 index 000000000..05ccd1de4 --- /dev/null +++ b/server/tests/Feature/Http/Api/NullRelationshipInputTest.php @@ -0,0 +1,191 @@ +has('driver')` is still true — `has()` + * means the key is present, not that it holds anything. Every public + * relationship input therefore arrives as null sooner or later, usually from a + * form that serialises an unselected dropdown as an empty string. + * + * That was fine for years because the lookups were plain queries: + * `where('public_id', null)` matches nothing and the caller's `if ($driver)` + * skips. It broke when a coverage pass extracted one of those queries into + * `findDriverByPublicId(string $publicId)` — the *return* type was made + * nullable, the parameter was not — turning "no driver" into a TypeError on + * `POST /v1/orders`. + * + * The static test below is the one that would have caught it: it holds the + * property rather than the instance, so the next extraction cannot reintroduce + * the class. + */ +function fleetopsNullRelationshipBoot(): SQLiteConnection +{ + $connection = new SQLiteConnection(new PDO('sqlite::memory:')); + $resolver = new ConnectionResolver(['default' => $connection, 'mysql' => $connection]); + $resolver->setDefaultConnection('mysql'); + EloquentModel::setConnectionResolver($resolver); + + app()->instance('db', new class($connection) { + public function __construct(public SQLiteConnection $c) + { + } + + public function connection($name = null): SQLiteConnection + { + return $this->c; + } + + public function __call($method, $arguments) + { + return $this->c->{$method}(...$arguments); + } + }); + app()->instance('db.schema', $connection->getSchemaBuilder()); + DB::clearResolvedInstance('db'); + + $schema = $connection->getSchemaBuilder(); + foreach (['drivers', 'users', 'payloads', 'orders', 'service_areas', 'entities', 'waypoints', 'places'] as $table) { + $schema->create($table, function ($blueprint) { + $blueprint->increments('id'); + foreach (['uuid', 'public_id', 'internal_id', 'company_uuid', 'user_uuid', 'tracking_number_uuid', 'payload_uuid', 'place_uuid', 'name', '_key'] as $column) { + $blueprint->string($column)->nullable(); + } + $blueprint->timestamps(); + $blueprint->timestamp('deleted_at')->nullable(); + }); + } + + session(['company' => 'company-uuid']); + $connection->table('users')->insert(['uuid' => 'user-uuid', 'company_uuid' => 'company-uuid']); + $connection->table('drivers')->insert(['uuid' => 'driver-uuid', 'public_id' => 'driver_real01', 'company_uuid' => 'company-uuid', 'user_uuid' => 'user-uuid']); + $connection->table('payloads')->insert(['uuid' => 'payload-uuid', 'public_id' => 'payload_real01', 'company_uuid' => 'company-uuid']); + $connection->table('orders')->insert(['uuid' => 'order-uuid', 'public_id' => 'order_real01', 'company_uuid' => 'company-uuid', 'tracking_number_uuid' => 'tracking-uuid']); + $connection->table('service_areas')->insert(['uuid' => 'sa-uuid', 'public_id' => 'service_area_r1', 'company_uuid' => 'company-uuid']); + + return $connection; +} + +function fleetopsInvokeSeam(object $controller, string $method, ...$arguments) +{ + $reflection = new ReflectionMethod($controller, $method); + $reflection->setAccessible(true); + + return $reflection->invoke($controller, ...$arguments); +} + +test('every request-fed lookup seam answers rather than raising when handed nothing', function () { + fleetopsNullRelationshipBoot(); + + // Each of these is reached from `$request->input('')`. A null + // means the caller sent an empty relationship, which is a question with an + // answer — "there is no such record" — not a programming error. + expect(fleetopsInvokeSeam(new OrderController(), 'findDriverByPublicId', null))->toBeNull() + ->and(fleetopsInvokeSeam(new EntityController(), 'findPayloadByPublicId', null))->toBeNull() + ->and(fleetopsInvokeSeam(new TrackingStatusController(), 'getOrderTrackingNumberUuid', null))->toBeNull() + ->and(fleetopsInvokeSeam(new ServiceAreaController(), 'serviceAreaUuid', null, ['public_id' => null, 'company_uuid' => 'company-uuid']))->toBeNull() + ->and(fleetopsInvokeSeam(new ZoneController(), 'serviceAreaUuid', null, ['public_id' => null, 'company_uuid' => 'company-uuid']))->toBeNull(); + + // Issue and FuelReport reach their seam only from `create`, where `driver` + // is `required` — validation answers 422 long before a null could arrive. + // Widening them is defence-in-depth for a future call site, so the claim + // here is only the one that matters: whatever they answer, it is not a + // TypeError. A TypeError would escape this block and fail the test. + foreach ([new IssueController(), new FuelReportController()] as $controller) { + try { + fleetopsInvokeSeam($controller, 'findDriverRecord', null); + } catch (ModelNotFoundException) { + // "No such driver" — the call sites already translate this into a 404. + } + } +}); + +test('a real identifier still resolves through the widened seams', function () { + fleetopsNullRelationshipBoot(); + + // Widening the parameter must not have widened the lookup: a present + // identifier resolves exactly as before, and one that does not exist still + // resolves to nothing. + expect(fleetopsInvokeSeam(new OrderController(), 'findDriverByPublicId', 'driver_real01')->uuid)->toBe('driver-uuid') + ->and(fleetopsInvokeSeam(new OrderController(), 'findDriverByPublicId', 'driver_missing'))->toBeNull() + ->and(fleetopsInvokeSeam(new EntityController(), 'findPayloadByPublicId', 'payload_real01')->uuid)->toBe('payload-uuid') + ->and(fleetopsInvokeSeam(new TrackingStatusController(), 'getOrderTrackingNumberUuid', 'order_real01'))->toBe('tracking-uuid') + ->and(fleetopsInvokeSeam(new ServiceAreaController(), 'serviceAreaUuid', 'service_area_r1', ['public_id' => 'service_area_r1', 'company_uuid' => 'company-uuid']))->toBe('sa-uuid'); +}); + +test('no public api lookup seam fed from the request refuses a null identifier', function () { + // The systemic guard. Rather than pinning the eight seams that exist today, + // it derives them: every call of the form `$this->someSeam($request->input(...))` + // in the public v1 controllers, checked for a nullable first parameter. + // + // An extraction that types a new seam `string` fails here, at the commit + // that introduces it, instead of in production on an empty dropdown. + // Exempt only where the *call site* already excludes null, and say which + // guard does it — an exemption without a named guard is how this test would + // rot into a rubber stamp. + $exempt = [ + // Not an identifier lookup — it geocodes a free-text address, and a null + // address has no meaning to geocode. Its call sites guard with + // `$request->isString(...)`, which a null can never satisfy. + 'PlaceController::createPlaceFromGeocodingLookup', + // Both call sites are behind `$request->isArray('payload')`. A payload + // sent as `""` fails that guard and is skipped, never unpacked. + 'OrderController::payloadShapeFromArray', + ]; + + $offenders = []; + $inspected = []; + + foreach (glob(dirname(__DIR__, 4) . '/src/Http/Controllers/Api/v1/*.php') as $path) { + $source = file_get_contents($path); + $controller = basename($path, '.php'); + + preg_match_all('/\$this->([A-Za-z_]+)\(\s*\$request->(?:input|or)\(/', $source, $matches); + + foreach (array_unique($matches[1]) as $seam) { + if (in_array($controller . '::' . $seam, $exempt, true)) { + continue; + } + + if (!preg_match('/(?:protected|public|private) function ' . preg_quote($seam, '/') . '\(([^),]*)/', $source, $signature)) { + continue; + } + + $firstParameter = trim($signature[1]); + $inspected[] = $controller . '::' . $seam; + + // `mixed`, an untyped parameter, and anything explicitly nullable all + // accept null already; only a narrow non-nullable type is a problem. + $accepts = $firstParameter === '' + || str_starts_with($firstParameter, '?') + || str_starts_with($firstParameter, '$') + || str_contains($firstParameter, 'null') + || str_starts_with($firstParameter, 'mixed'); + + if (!$accepts) { + $offenders[] = $controller . '::' . $seam . '(' . $firstParameter . ')'; + } + } + } + + // A guard that scans nothing passes for the wrong reason, so make the scan + // prove itself: the seam this bug was reported against has to be among what + // it looked at. If the glob or the pattern ever stops matching, this fails + // instead of going quietly green. + expect($inspected)->toContain('OrderController::findDriverByPublicId') + ->and($offenders)->toBe([], 'these seams are handed a request value and would raise a TypeError on an empty relationship: ' . implode(', ', $offenders)); +}); diff --git a/server/tests/RequestContractsTest.php b/server/tests/RequestContractsTest.php index 075f0efdb..9cda5fd89 100644 --- a/server/tests/RequestContractsTest.php +++ b/server/tests/RequestContractsTest.php @@ -1041,7 +1041,13 @@ public function parameter($key, $default = null) ->and(ruleStrings($rateRules['peak_hours_calculation_method']))->toContain('required', 'in:percentage,flat') ->and(ruleStrings($rateRules['peak_hours_percent']))->toContain('required', 'integer') ->and(ruleStrings($rateRules['peak_hours_start']))->toContain('required', 'date_format:H:i') - ->and(ruleStrings($rateRules['peak_hours_end']))->toContain('required', 'date_format:H:i'); + ->and(ruleStrings($rateRules['peak_hours_end']))->toContain('required', 'date_format:H:i') + // Both relationships are optional, so an empty one has to read as + // absent. Without `nullable` the `exists` rule runs against the null + // that ConvertEmptyStringsToNull produced and answers "the selected + // service area is invalid" for a field the caller left blank. + ->and(ruleStrings($rateRules['service_area']))->toContain('nullable') + ->and(ruleStrings($rateRules['zone']))->toContain('nullable'); // A street address alone satisfies the name requirement on create, and // updates never require either field since the record already exists diff --git a/server/tests/VehicleControllerHelperContractsTest.php b/server/tests/VehicleControllerHelperContractsTest.php index 3f2c63da4..67a150793 100644 --- a/server/tests/VehicleControllerHelperContractsTest.php +++ b/server/tests/VehicleControllerHelperContractsTest.php @@ -39,7 +39,7 @@ protected function resolveVehicle(string $id): ?Vehicle return $this->vehicle; } - protected function findDriver(string $id): Driver + protected function findDriver(?string $id): Driver { $this->driverLookups[] = $id;