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