Conversation
The internal OrderController resolved every order-lifecycle target straight
from a caller-supplied identifier with no company constraint:
Order::where('uuid', $uuid)->first() // cancel
Order::findById($id) // dispatch, schedule, tracker
Order::where('uuid', $uuid)->withoutGlobalScopes() // start
Order::whereIn('uuid', $ids)->get() // bulk-cancel, bulk-dispatch
Driver::whereUuid($uuid)->first() // bulk-assign-driver
Order::whereIn('uuid', $uuids)->update([...]) // bulk-assign-driver
These targets arrive as body/query params rather than bound route parameters,
so nothing upstream narrows them: `fleetbase.protected` runs auth:sanctum plus
AuthorizationGuard, which only checks that the caller holds the named RBAC
capability — it never inspects which company the record belongs to. Any
authenticated user with ordinary "manage orders" rights could therefore cancel,
dispatch, start, schedule or bulk-reassign another organization's orders by
supplying their uuid, which is not secret (tracking links, labels, webhooks).
Generic CRUD on the same model was already safe because it carries its own
explicit company_uuid clause; these hand-rolled lifecycle lookups did not.
Every by-identifier lookup in this controller now goes through a single
`scopedToCompany()` guard that adds the company_uuid constraint and fails the
query closed when no company is in session. Beyond the lifecycle actions, the
same guard now covers update-activity, next-activity, set-destination,
capture-photo, edit-route, ping-driver, proofs, entity/proof subjects,
tracking-number lookup and the import file lookup, which had the identical gap.
Two behavioural notes:
- `cancel()` now rejects an unresolvable order instead of dereferencing null.
`exists:orders,uuid` on CancelOrderRequest is a global existence check, so a
cross-tenant uuid passes validation and has to be refused in the controller.
- `bulkAssignDriver()` resolves the ids through the scoped lookup first, so
orders owned by another company are dropped before the update, and are
neither counted in the response nor queued for driver notification.
`findOrderById()` also takes the identifier as mixed and resolves anything that
is not a non-empty string to null, since it is raw request input.
nextActivity() no longer depends on core-api's findByIdOrFail() raising a
catchable ModelNotFoundException, so its not-found branch is live regardless of
the upstream release; the test documenting that dependency is updated.
Tests: new OrderControllerTenantScopingTest covers, for every patched lookup,
the owning-company hit, the cross-tenant miss (both the uuid and public_id
arms, so a regrouped OR cannot regress), and the no-company fail-closed path,
plus the endpoint-level refusals. OrderController.php is at 853/853 statements
covered with no uncovered lines.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #331 +/- ##
=========================================
Coverage 99.99% 99.99%
- Complexity 11936 11942 +6
=========================================
Files 583 583
Lines 44892 44935 +43
=========================================
+ Hits 44891 44934 +43
Misses 1 1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Internal\v1\OrderControllerresolved every order-lifecycle target straight from a caller-supplied identifier, with no company constraint:cancelPATCH /int/v1/fleet-ops/orders/cancelOrder::where('uuid', $uuid)->first()dispatchOrderPATCH .../orders/dispatchOrder::findById($id)startPATCH .../orders/startOrder/Driver/Payload::where('uuid', …)->withoutGlobalScopes()scheduleOrderPATCH .../orders/scheduleOrder::findById($id);Driver::where('uuid',$id)->orWhere('public_id',$id)bulkCancel/bulkDispatch.../orders/bulk-cancel,bulk-dispatchOrder::whereIn('uuid', $ids)->get()bulkAssignDriver.../orders/bulk-assign-driverDriver::whereUuid($uuid);Order::whereIn('uuid', $uuids)->update([...])These targets arrive as body/query params (
order,ids,driver), not bound route parameters, so nothing upstream narrows them. The route group'sfleetbase.protectedmiddleware runsauth:sanctumplusAuthorizationGuard, and that guard only checks the caller holds the named RBAC capability — it never inspects which company the record belongs to.Net effect: any authenticated user with ordinary "manage orders" rights (a normal dispatcher, not an admin) could cancel, dispatch, start, schedule or bulk-reassign another organization's orders by supplying their uuid. Those uuids are not secret — they appear on shared tracking links, printed labels, driver apps and webhook payloads.
bulk-assign-driverwas the worst case: it could reassign arbitrary victim-company orders to an attacker-controlled driver, in bulk, in one request.Generic CRUD on the same
Ordermodel was already safe, because each of those paths carries its own explicitcompany_uuidclause. These hand-rolled lifecycle lookups did not.Fix
Every by-identifier lookup in this controller now goes through one guard:
A missing company session fails the query closed rather than letting it run unbounded across every tenant. Where a lookup accepts uuid or public_id, the identifier match is grouped so the company constraint applies to both arms — ungrouped it would read as
uuid = ? OR (public_id = ? AND company_uuid = ?)and still resolve across tenants.While in here, the same guard is applied to the adjacent lookups in this controller that had the identical gap:
update-activity,next-activity,set-destination,capture-photo,edit-route,ping-driver,proofs, the entity/proof subject lookups, the tracking-number lookup, and the import file lookup (which could read another company's uploaded spreadsheet).Each helper keeps its existing global-scope posture (
withoutGlobalScopes()is preserved exactly where it already was), so the only behavioural change is the tenant constraint.Behavioural notes for reviewers
cancel()now rejects an unresolvable order instead of dereferencing null.exists:orders,uuidonCancelOrderRequestis a global existence check, so a cross-tenant uuid passes validation and has to be refused in the controller.bulkAssignDriver()resolves ids through the scoped lookup first. Orders owned by another company are dropped before the update, so they are neither counted in the response nor queued for driver notification (previously the count echoed the ids supplied, andNotifyBulkAssignedDriverwould have been queued for them). The existing contract test is updated to seed the orders it expects back.findOrderById()takes the identifier asmixedand resolves anything that is not a non-empty string tonull, since it is raw request input. A non-string body value previously raised aTypeError.nextActivity()no longer depends on upstream behaviour. It resolved viaOrder::findByIdOrFail(), whoseModelNotFoundExceptionnever actually fired in core-api; the not-found branch is now live regardless of the upstream release, and the test that documented the dependency is updated to say so.Tests
New
OrderControllerTenantScopingTestcovers, for every patched lookup:ORcannot regress silently,plus endpoint-level refusals for cancel, dispatch, start, schedule, bulk-cancel, bulk-dispatch, bulk-assign-driver, update-activity, next-activity, set-destination, tracker, eta, edit-route and proofs.
ping-driveris asserted to report a cross-tenant id exactly as it reports an unknown one, so the endpoint cannot be used to probe which ids exist in other tenants.A cross-tenant case is also added to
OrderControllerImportFromFilesTest.Coverage:
OrderController.phpis at 853/853 statements covered, no uncovered lines.Suite: all 465 test files run individually — zero failures.
Static analysis: PHPStan errors for this file go from 301 (baseline) to 298 — the change adds none.
Follow-up outside this repo
fleetbase/core-api'sScopes/CompanyScopedocuments itself as the primary tenant-isolation defence, but it is registered on nothing — the onlyaddGlobalScope(new CompanyScope())call in either repo is inside the scope's own unit test, against a throwaway test model. The explicitcompany_uuidclauses inHasApiModelBehavior/HasApiControllerBehaviorthat their comments label "defence-in-depth" are therefore the only protection on those paths, not a backup — and that misdescription is plausibly how the lifecycle lookups patched here came to be written with no scoping at all.That scope is not a gap to be filled by registering it: it was tried, it destabilised the application, and it was abandoned. It fails open (no constraint during console execution or without a session company), any of the 83
withoutGlobalScopes()call sites inserver/srcdrops it silently, and because global scopes apply to relations and eager loads it turns legitimate cross-company references into silent nulls. It is being removed in fleetbase/core-api#260, which also rewrites the comments so they state that the explicit clause is the protection.Tenant isolation here stays explicit and per-lookup by design. A separate follow-up will add a contract test that flags unscoped tenant-owned lookups, so omission is caught in review rather than relied on.