From 5b8e48d4f6e507c81fdf95f119cd0c46c8d511b0 Mon Sep 17 00:00:00 2001 From: "Ronald A. Richardson" Date: Fri, 18 Sep 2026 17:00:20 +0800 Subject: [PATCH] Remove the abandoned CompanyScope global scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/Scopes/CompanyScope.php | 116 ------------------ src/Traits/HasApiControllerBehavior.php | 5 +- src/Traits/HasApiModelBehavior.php | 49 +++++--- tests/Unit/Scopes/CompanyScopeTest.php | 150 ------------------------ 4 files changed, 36 insertions(+), 284 deletions(-) delete mode 100644 src/Scopes/CompanyScope.php delete mode 100644 tests/Unit/Scopes/CompanyScopeTest.php diff --git a/src/Scopes/CompanyScope.php b/src/Scopes/CompanyScope.php deleted file mode 100644 index f8df4917..00000000 --- a/src/Scopes/CompanyScope.php +++ /dev/null @@ -1,116 +0,0 @@ -where(...)->get(); - * Model::withoutGlobalScope(CompanyScope::class)->where(...)->get(); - * - * The `withoutCompanyScope()` macro is the preferred, readable form. - */ -class CompanyScope implements Scope -{ - /** - * Per-process cache of which table names have a `company_uuid` column. - * Avoids repeated Schema::hasColumn() calls on the same table. - * - * @var array - */ - protected static array $columnCache = []; - - /** - * Apply the scope to a given Eloquent query builder. - * - * @return void - */ - public function apply(Builder $builder, Model $model) - { - // Never apply during CLI execution (artisan, queue workers, etc.) - if (app()->runningInConsole()) { - return; - } - - $companyUuid = Session::get('company'); - - // Only apply when there is an active company session. - if (empty($companyUuid)) { - return; - } - - // Only apply when the model's table actually has a company_uuid column. - // Cache the result per table to avoid repeated Schema introspection. - $table = $model->getTable(); - if (!isset(static::$columnCache[$table])) { - static::$columnCache[$table] = Schema::hasColumn($table, 'company_uuid'); - } - - if (!static::$columnCache[$table]) { - return; - } - - $builder->where($model->qualifyColumn('company_uuid'), $companyUuid); - } - - /** - * Extend the query builder with the withoutCompanyScope macro. - * - * @return void - */ - public function extend(Builder $builder) - { - $this->addWithoutCompanyScope($builder); - } - - /** - * Add the withoutCompanyScope macro to the builder. - * - * @return void - */ - protected function addWithoutCompanyScope(Builder $builder) - { - $builder->macro('withoutCompanyScope', function (Builder $builder) { - return $builder->withoutGlobalScope(CompanyScope::class); - }); - } - - /** - * Flush the column existence cache. - * Useful in tests where tables may be created/dropped between cases. - */ - public static function flushColumnCache(): void - { - static::$columnCache = []; - } -} diff --git a/src/Traits/HasApiControllerBehavior.php b/src/Traits/HasApiControllerBehavior.php index 79f80331..589f35aa 100644 --- a/src/Traits/HasApiControllerBehavior.php +++ b/src/Traits/HasApiControllerBehavior.php @@ -562,8 +562,9 @@ public function deleteRecord($id, Request $request) $builder = $this->model->wherePublicId($id); } - // Defence-in-depth: scope delete to the caller's company to prevent - // cross-tenant deletion (GHSA-3wj9-hh56-7fw7). + // Tenant constraint: scope the delete to the caller's company to prevent + // cross-tenant deletion (GHSA-3wj9-hh56-7fw7). No global scope backs this + // up — removing it reopens that vulnerability. $companyUuid = session('company'); if ($companyUuid && $this->model->isColumn($this->model->qualifyColumn('company_uuid'))) { $builder->where($this->model->qualifyColumn('company_uuid'), $companyUuid); diff --git a/src/Traits/HasApiModelBehavior.php b/src/Traits/HasApiModelBehavior.php index 2161f7cf..89c29cd3 100644 --- a/src/Traits/HasApiModelBehavior.php +++ b/src/Traits/HasApiModelBehavior.php @@ -17,6 +17,21 @@ /** * Adds API Model Behavior. + * + * Tenant isolation + * ---------------- + * There is deliberately no global tenant scope behind this trait. Every lookup + * that can reach a tenant-owned record carries its own explicit `company_uuid` + * clause, and that clause is the whole protection — not a backup for something + * else. Do not remove one on the assumption that a scope covers it. + * + * A global `CompanyScope` was tried and removed: it could not apply during + * console execution (queue workers, scheduler, artisan) or when no session + * company was set, so it failed open exactly where isolation mattered; any of + * the many `withoutGlobalScopes()` call sites silently dropped it as a side + * effect of dropping an unrelated scope; and because global scopes also apply to + * relations and eager loads, legitimate cross-company references resolved to + * null instead of raising, which destabilised callers far from the query. */ trait HasApiModelBehavior { @@ -377,8 +392,9 @@ public function updateRecordFromRequest(Request $request, $id, ?callable $onBefo } }); - // Defence-in-depth: scope update to the caller's company to prevent - // cross-tenant modification (GHSA-3wj9-hh56-7fw7). + // Tenant constraint: scope the update to the caller's company to prevent + // cross-tenant modification (GHSA-3wj9-hh56-7fw7). No global scope backs + // this up — removing it reopens that vulnerability. $companyUuid = session('company'); if ($companyUuid && $this->isColumn('company_uuid')) { $builder->where($this->qualifyColumn('company_uuid'), $companyUuid); @@ -501,8 +517,9 @@ public function bulkRemove($ids = []) } }); - // Defence-in-depth: scope bulk delete to the caller's company to prevent - // cross-tenant deletion (GHSA-3wj9-hh56-7fw7). + // Tenant constraint: scope the bulk delete to the caller's company to + // prevent cross-tenant deletion (GHSA-3wj9-hh56-7fw7). No global scope + // backs this up — removing it reopens that vulnerability. $companyUuid = session('company'); if ($companyUuid && $this->isColumn('company_uuid')) { $records->where($this->qualifyColumn('company_uuid'), $companyUuid); @@ -754,11 +771,9 @@ public function applySorts($request, $builder) /** * Retrieves a record based on primary key id. * - * The query is automatically scoped to the current company via the - * CompanyScope global scope registered on the base Model. This method - * adds an explicit defence-in-depth company_uuid check as well so that - * the constraint is visible at the call-site and survives any future - * withoutGlobalScope() calls higher up the stack. + * The company_uuid clause below is the tenant constraint for this path. + * Nothing scopes the query before it reaches here, so the clause is visible + * at the call site on purpose and must not be removed. * * @param string $id - The ID * @param Request $request - HTTP Request @@ -776,10 +791,10 @@ public function getById($id, ?callable $queryCallback, Request $request) } }); - // Defence-in-depth: explicitly scope to the caller's company when the - // model has a company_uuid column and a session company is available. - // The CompanyScope global scope provides the primary protection; this - // explicit clause ensures the constraint survives withoutGlobalScope(). + // Tenant constraint: scope to the caller's company when the model has a + // company_uuid column and a session company is available. This clause is + // the only thing keeping the lookup inside the caller's tenant + // (GHSA-3wj9-hh56-7fw7). $companyUuid = session('company'); if ($companyUuid && $this->isColumn('company_uuid')) { $builder->where($this->qualifyColumn('company_uuid'), $companyUuid); @@ -1350,7 +1365,7 @@ public static function findRecordOrFail($id, $with = [], $columns = ['*'], ?\Clo // has internal id? $hasInternalId = in_array('internal_id', $instance->getFillable()); - // create query — CompanyScope global scope is applied automatically + // create query $query = static::query() ->select($columns) ->with($with) @@ -1364,8 +1379,10 @@ function ($query) use ($id, $hasInternalId) { } ); - // Defence-in-depth: explicitly scope to the caller's company when the - // model's table has a company_uuid column and a session is active. + // Tenant constraint: scope to the caller's company when the model's table + // has a company_uuid column and a session is active. This clause is the + // only thing keeping the lookup inside the caller's tenant + // (GHSA-3wj9-hh56-7fw7). $companyUuid = session('company'); if ($companyUuid && Schema::hasColumn($instance->getTable(), 'company_uuid')) { $query->where($instance->qualifyColumn('company_uuid'), $companyUuid); diff --git a/tests/Unit/Scopes/CompanyScopeTest.php b/tests/Unit/Scopes/CompanyScopeTest.php deleted file mode 100644 index 1803e473..00000000 --- a/tests/Unit/Scopes/CompanyScopeTest.php +++ /dev/null @@ -1,150 +0,0 @@ -console; - } -} - -class CompanyScopeSessionFake -{ - public function __construct(private array $values = []) - { - } - - public function get(string $key, mixed $default = null): mixed - { - return $this->values[$key] ?? $default; - } -} - -class CompanyScopeRecord extends Model -{ - protected $connection = 'mysql'; - protected $table = 'company_scope_records'; - protected $guarded = []; - public $timestamps = false; -} - -class CompanyScopePlainRecord extends Model -{ - protected $connection = 'mysql'; - protected $table = 'company_scope_plain_records'; - protected $guarded = []; - public $timestamps = false; -} - -function company_scope_database(bool $console = false, ?string $company = 'company-1'): Capsule -{ - EloquentModel::clearBootedModels(); - CompanyScope::flushColumnCache(); - - Container::setInstance(new CompanyScopeTestContainer($console)); - $container = bind_test_container([ - 'database.default' => 'mysql', - 'database.connections.mysql' => [ - 'driver' => 'sqlite', - 'database' => ':memory:', - 'prefix' => '', - ], - 'fleetbase.connection.db' => 'mysql', - ]); - Facade::setFacadeApplication($container); - - $connection = $container->make('config')->get('database.connections.mysql'); - $capsule = new Capsule($container); - $capsule->addConnection($connection, 'mysql'); - $capsule->setEventDispatcher(new Dispatcher($container)); - $capsule->setAsGlobal(); - $capsule->bootEloquent(); - $capsule->getDatabaseManager()->setDefaultConnection('mysql'); - $container->instance('db', $capsule->getDatabaseManager()); - $container->instance('db.schema', $capsule->getConnection('mysql')->getSchemaBuilder()); - $container->instance('session', new CompanyScopeSessionFake(['company' => $company])); - Facade::clearResolvedInstance('db'); - Facade::clearResolvedInstance('db.schema'); - Facade::clearResolvedInstance('session'); - - $schema = $capsule->getConnection('mysql')->getSchemaBuilder(); - $schema->create('company_scope_records', function ($table) { - $table->string('uuid')->primary(); - $table->string('company_uuid')->nullable(); - $table->string('name')->nullable(); - }); - $schema->create('company_scope_plain_records', function ($table) { - $table->string('uuid')->primary(); - $table->string('name')->nullable(); - }); - - $capsule->getConnection('mysql')->table('company_scope_records')->insert([ - ['uuid' => 'record-1', 'company_uuid' => 'company-1', 'name' => 'Visible'], - ['uuid' => 'record-2', 'company_uuid' => 'company-2', 'name' => 'Hidden'], - ]); - $capsule->getConnection('mysql')->table('company_scope_plain_records')->insert([ - ['uuid' => 'plain-1', 'name' => 'Plain'], - ]); - - return $capsule; -} - -afterEach(function () { - CompanyScope::flushColumnCache(); - EloquentModel::clearBootedModels(); - Facade::clearResolvedInstances(); - Container::setInstance(new FleetbaseTestContainer()); -}); - -test('company scope constrains models with company uuid only during request context with company session', function () { - company_scope_database(); - - $builder = CompanyScopeRecord::query(); - $scope = new CompanyScope(); - $scope->apply($builder, new CompanyScopeRecord()); - - $plainBuilder = CompanyScopePlainRecord::query(); - $scope->apply($plainBuilder, new CompanyScopePlainRecord()); - - expect($builder->orderBy('uuid')->pluck('uuid')->all())->toBe(['record-1']) - ->and($plainBuilder->pluck('uuid')->all())->toBe(['plain-1']); -}); - -test('company scope skips console and missing session contexts and exposes removal macro', function () { - company_scope_database(console: true); - - $consoleBuilder = CompanyScopeRecord::query(); - $scope = new CompanyScope(); - $scope->apply($consoleBuilder, new CompanyScopeRecord()); - - expect($consoleBuilder->orderBy('uuid')->pluck('uuid')->all())->toBe(['record-1', 'record-2']); - - company_scope_database(console: false, company: null); - $missingSessionBuilder = CompanyScopeRecord::query(); - $scope->apply($missingSessionBuilder, new CompanyScopeRecord()); - - expect($missingSessionBuilder->orderBy('uuid')->pluck('uuid')->all())->toBe(['record-1', 'record-2']); - - company_scope_database(); - CompanyScopeRecord::addGlobalScope(new CompanyScope()); - - $scoped = CompanyScopeRecord::query()->orderBy('uuid')->pluck('uuid')->all(); - $unscoped = CompanyScopeRecord::query()->withoutCompanyScope()->orderBy('uuid')->pluck('uuid')->all(); - - expect($scoped)->toBe(['record-1']) - ->and($unscoped)->toBe(['record-1', 'record-2']) - ->and(CompanyScopeRecord::query())->toBeInstanceOf(Builder::class); -});