Conversation
CompanyScope was an attempt to enforce tenant isolation with a single global Eloquent scope. It was abandoned because it destabilised the application, and it has been dead code since: the only `addGlobalScope(new CompanyScope())` anywhere across core-api and the extension packages was inside this scope's own unit test, against a throwaway test-only model, and the `withoutCompanyScope()` macro it registered was likewise only ever called from that test. Leaving it in place was worse than unused, because its docblock claimed to be "the primary defence against the cross-tenant IDOR vulnerability (GHSA-3wj9-hh56-7fw7)" and `HasApiModelBehavior` repeated that claim. The explicit `company_uuid` clauses that actually provide the protection were therefore labelled "defence-in-depth", as though a real layer sat behind them. It does not, and reading the code as though it did is how tenant-scoped lookups get written with no scoping at all — see fleetbase/fleetops#331, where the order lifecycle actions resolved their targets by caller-supplied uuid with no company constraint of any kind. The design could not have served as that primary defence: - It bailed out during console execution, so isolation was absent in queue workers, the scheduler and artisan — the same model reached through the same code was protected in a web request and unprotected in a job. - It bailed out when no session company was set, i.e. it failed open in exactly the contexts where a session is missing (webhooks, public tracking, installer, token flows before session setup). - Any `withoutGlobalScopes()` call dropped it as a side effect of dropping an unrelated scope. fleetops `server/src` alone has 83 such call sites. - Global scopes also apply to relations, eager loads and `whereHas`, so legitimate cross-company references resolved to null rather than raising, surfacing as cascading nulls far from the query that caused them. Tenant isolation stays where it already is: an explicit, visible `company_uuid` clause at each lookup. The comments at those clauses are rewritten to say they are the tenant constraint rather than a second layer, and the trait docblock records why there is no global scope so one does not get reintroduced. No behaviour change: the class was registered nowhere. `ExpiryScope` is untouched — it is opt-in per model via `Expirable`, single-concern, and carries no authorization meaning.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #260 +/- ##
===========================================
Coverage 100.00% 100.00%
+ Complexity 6765 6757 -8
===========================================
Files 398 397 -1
Lines 22554 22534 -20
===========================================
- Hits 22554 22534 -20
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.
What
Deletes
src/Scopes/CompanyScope.phpand its unit test, and rewrites the comments that pointed at it.No behaviour change — the class was registered on nothing.
Why
CompanyScopewas an attempt to enforce tenant isolation with a single global Eloquent scope. It was abandoned because it destabilised the application, and it has been dead code since. Verified across core-api and every extension package:addGlobalScope(new CompanyScope())anywhere was inside this scope's own unit test, against a throwaway test-only model;withoutCompanyScope()macro it registered was likewise only ever called from that same test;ReportQueryConverter::applyCompanyScope()is its own method; samsara'sSamsaraCompanyScopeis a middleware).Leaving it in place was worse than merely unused, because its docblock claimed to be "the primary defence against the cross-tenant IDOR vulnerability (GHSA-3wj9-hh56-7fw7)", and
HasApiModelBehaviorrepeated the claim:So the explicit
company_uuidclauses that are actually the protection were labelled "defence-in-depth", as though a real layer sat behind them. Reading the code as though it did is how tenant-owned lookups get written with no scoping at all — see fleetbase/fleetops#331, where the order lifecycle actions resolved their targets straight from a caller-supplied uuid with no company constraint of any kind.Why it could never have been that primary defence
Worth recording, because the docblock was persuasive:
if (app()->runningInConsole()) return;if (empty($companyUuid)) return;withoutGlobalScopes()server/srcalone has 83 such call sites.whereHasnullinstead of raising — cascading nulls surfacing far from the query that caused them. This is the destabilisation.A control with three "apply nothing" early returns cannot be a floor. Authorization that silently returns fewer rows is indistinguishable from "no data" at the call site, which is what makes this failure mode so expensive to debug.
What replaces it
Nothing new — tenant isolation stays where it already effectively was: an explicit, visible
company_uuidclause at each lookup. This PR only stops the comments from lying about it:getById()'s docblock no longer claims the query arrives pre-scoped;findRecordOrFail()'s// create query — CompanyScope global scope is applied automaticallyloses the false half;HasApiModelBehaviortrait docblock gains a short "Tenant isolation" section recording that there is deliberately no global scope, and why, so one does not get reintroduced.ExpiryScopeis untouched. It is a legitimate global scope: opt-in per model viaExpirable, single-concern, no session dependency, no authorization meaning.Verification
vendor/bin/pest). The removed test's cases are the only ones gone.php-cs-fixer --dry-runclean across all 732 files.srchas 12,444 pre-existing errors atlevel: maxboth before and after; the deleted file contributed exactly 1, so the count goes down by 1 and the comment edits cannot affect analysis.reportUnmatchedIgnoredErrorsis safe — there are noignoreErrorsentries and no baseline file referencing the deleted path.Notes
mainbecause norelease/*branch is currently open (v1.6.62 is merged). Happy to retarget if you would rather this ride a release branch, and to add a RELEASE.md entry there.