From 04da9c896b4e948097cb414781673b720dca47f8 Mon Sep 17 00:00:00 2001 From: lukinovec Date: Fri, 26 Jun 2026 04:51:39 +0200 Subject: [PATCH 1/6] [MINOR BC] Fix pending tenant pull race conditions (#1463) > Minor breaking change: clearing pending_since no longer fires Eloquent events, PullingPendingTenant is now fired at a different point in the lifecycle and does not guarantee the tenant will actually be pulled. `pullPendingFromPool` had a race condition when user A attempted to pull a tenant at the same time as user B. Both could end up grabbing the same tenant, and the result was unexpected, e.g. one of them ending up with no pending tenant pulled at all even though there was a pending tenant in the pool. instead of selecting a pending tenant and updating the same model, we now run `update()` conditionally -- it clears `pending_since` _only_ if the tenant is still pending, and we check the affected row count. Only one process can get a row back, the other gets 0 and retries with the next pending candidate in the pool. The loop always terminates since every lost claim means the pool shrank by one. Eventually it's empty and we create a new tenant (or return null). The claim and the attribute update happen in a single transaction now, so if updating `$attributes` fails, the claim rolls back and the tenant stays in the pool. Added a regression test that simulates a concurrent "steal" synchronously via a PullingPendingTenant listener. Fails with the old code, passes with the HasPending changes. Very minor BC: - Clearing `pending_since` no longer fires model updating/updated events (since the update goes through query builder). `PendingTenantPulled` still fires the same as before and is the listener you'd want to use anyway. - `PullingPendingTenant` now fires before the claim (and outside the transaction), so it can fire more than once with concurrent pulls (e.g. when a tenant gets claimed by someone else). `PendingTenantPulled` is still the one that fires exactly once for the actually pulled tenant. --------- Co-authored-by: Samuel Stancl --- src/Database/Concerns/HasPending.php | 58 ++++++++++++++++++++-------- tests/PendingTenantsTest.php | 47 ++++++++++++++++++++++ 2 files changed, 88 insertions(+), 17 deletions(-) diff --git a/src/Database/Concerns/HasPending.php b/src/Database/Concerns/HasPending.php index 04fcccc1..e3d8a6fb 100644 --- a/src/Database/Concerns/HasPending.php +++ b/src/Database/Concerns/HasPending.php @@ -100,27 +100,51 @@ trait HasPending */ public static function pullPendingFromPool(bool $firstOrCreate = false, array $attributes = []): ?Tenant { - $tenant = DB::transaction(function () use ($attributes): ?Tenant { - /** @var (Model&Tenant)|null $tenant */ - $tenant = static::onlyPending()->first(); + // Attempt pulling a pending tenant. + // The loop handles the case where a single tenant is being pulled by multiple processes at the same time. + // If a tenant was pulled by a concurrent process, try pulling the next one in the pool. + while (true) { + /** @var (Model&Tenant)|null $pullCandidate */ + $pullCandidate = static::onlyPending()->first(); - if ($tenant !== null) { - event(new PullingPendingTenant($tenant)); - $tenant->update(array_merge($attributes, [ - 'pending_since' => null, - ])); + if ($pullCandidate === null) { + return $firstOrCreate ? static::create($attributes) : null; } + // Fired before the claim, so it can fire once per attempt, including for a candidate + // that ends up being claimed by a different process (in which case the loop retries). + // PendingTenantPulled (below) fires exactly once, for the actually pulled tenant. + event(new PullingPendingTenant($pullCandidate)); + + $tenant = DB::transaction(function () use ($pullCandidate, $attributes): ?Tenant { + $tenantWasPulled = static::onlyPending() + ->whereKey($pullCandidate->getKey()) + ->update([$pullCandidate->getColumnForQuery('pending_since') => null]) > 0; + + if (! $tenantWasPulled) { + return null; + } + + // The tenant's pending_since was just cleared, and a PullingPendingTenant listener + // may have made changes to the tenant, so re-fetch it to make sure it's up to date. + /** @var Model&Tenant $pulledTenant */ + $pulledTenant = static::findOrFail($pullCandidate->getKey()); + + if (! empty($attributes)) { + $pulledTenant->update($attributes); + } + + return $pulledTenant; + }); + + if ($tenant === null) { + // If another pull claimed this tenant first, try claiming the next one + continue; + } + + event(new PendingTenantPulled($tenant)); + return $tenant; - }); - - if ($tenant === null) { - return $firstOrCreate ? static::create($attributes) : null; } - - // Only triggered if a tenant that was pulled from the pool is returned - event(new PendingTenantPulled($tenant)); - - return $tenant; } } diff --git a/tests/PendingTenantsTest.php b/tests/PendingTenantsTest.php index b04f8bc4..c9960728 100644 --- a/tests/PendingTenantsTest.php +++ b/tests/PendingTenantsTest.php @@ -5,6 +5,7 @@ declare(strict_types=1); use Illuminate\Database\QueryException; use Illuminate\Database\Schema\Blueprint; use Illuminate\Support\Facades\Artisan; +use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Event; use Illuminate\Support\Facades\Schema; use Illuminate\Support\Str; @@ -126,6 +127,52 @@ test('a new tenant gets created while pulling a pending tenant if the pending po expect(Tenant::withPending()->get()->count())->toBe(1); // All tenants }); +test('pulling a pending tenant retries when the tenant is claimed concurrently', function () { + Tenant::createPending(); + Tenant::createPending(); + + $stolenId = null; + + Event::listen(PullingPendingTenant::class, function (PullingPendingTenant $event) use (&$stolenId) { + if ($stolenId !== null) { + return; + } + + $stolenId = $event->tenant->id; + + // Steal the tenant like a concurrent process would + Tenant::onlyPending() + ->whereKey($event->tenant->id) + ->update([$event->tenant->getColumnForQuery('pending_since') => null]); + }); + + $pulled = Tenant::pullPendingFromPool(); + + expect($pulled)->not()->toBeNull(); + expect($pulled->id)->not()->toBe($stolenId); // Stolen tenant was skipped, the next one was claimed by the pull + expect(Tenant::onlyPending()->count())->toBe(0); // Both tenants claimed +}); + +test('the pull is rolled back and the tenant stays in the pool if setting attributes fails', function () { + // Pulling a tenant and setting its attributes happen in one transaction, + // so if setting the attributes fails, the whole pull rolls back and the tenant stays in the pool. + Schema::table('tenants', function (Blueprint $table) { + $table->string('slug')->nullable()->unique(); + }); + + Tenant::$extraCustomColumns = ['slug']; + + Tenant::create(['slug' => 'taken']); + Tenant::createPending(); + + // During the pull, set slug to 'taken', which is already used by another tenant to make the attribute update throw + expect(fn () => Tenant::pullPendingFromPool(false, ['slug' => 'taken'])) + ->toThrow(QueryException::class); + + // The pull rolled back, so the tenant is still pending + expect(Tenant::onlyPending()->count())->toBe(1); +}); + test('withoutPending chained with where clauses returns correct results', function () { $tenant = Tenant::create(); $pendingTenant = Tenant::createPending(); From ecf031237d5eca044a54325f96cb346ba7e51bcb Mon Sep 17 00:00:00 2001 From: lukinovec Date: Sun, 28 Jun 2026 03:30:01 +0200 Subject: [PATCH 2/6] Make globalCache always use central conn with DB cache stores (#1462) `globalCache` should always use the central connection, but when using a `database`-driver cache store with `DatabaseTenancyBootstrapper`, it does not (with the exception of `DatabaseCacheBootstrapper`, explained below). `globalCache` creates a fresh `CacheManager` each time it's resolved (it's a `bind`, not a `singleton`). A freshly-created manager builds its database stores using the current default DB connection. When `DatabaseTenancyBootstrapper` is active, that default is `tenant`. So `globalCache` in tenant context points at the tenant DB. Specifically, `CachedTenantResolver` stores cached tenant lookups via `globalCache`. When a domain is deleted in tenant context, the invalidation logic calls `globalCache->forget(...)`, but that hits the tenant DB, while the resolver cache entry is in the central DB. `globalCache->forget(...)` doesn't actually do anything in that case. With `DatabaseCacheBootstrapper`, this is already handled. `globalCache` is always central because it sets `TenancyServiceProvider::$adjustCacheManagerUsing` to a callback that explicitly restores the central connection on `globalCache`'s stores. To fix this, after constructing the fresh CacheManager in the globalCache binding, explicitly set the connection of every database-driver store to its configured connection value, **falling back to central_connection** when the config value is null (null value = inherits whatever the current default DB connection is). This is sufficient for `CacheTenancyBootstrapper` and any other bootstrapper that doesn't explicitly set the store's DB connection. For `DatabaseCacheBootstrapper` specifically, this alone is not enough since it explicitly sets the store's config connection to 'tenant'. That's why DatabaseCacheBootstrapper's `$adjustCacheManagerUsing` callback runs after and overrides those stores back to the original (central) connection. > In short: `makeDatabaseCacheStoresCentral()` handles stores with a `null` connection config (falls back to central). `$adjustCacheManagerUsing` handles the `DatabaseCacheBootstrapper` case where the config is explicitly set to 'tenant'. Added datasets that use the database cache store + CacheTenancyBootstrapper to the relevant tests (globalCache and invalidation) to test regression (https://github.com/archtechx/tenancy/commit/0cf7043b733848b6d139de967df676e30247323e), and the changes mentioned above (https://github.com/archtechx/tenancy/pull/1462/commits/5e65c67ea0daf98f57f2a6a7b0e1937bbc397a56) make these tests pass. --------- Co-authored-by: Samuel Stancl --- .../CacheTenancyBootstrapper.php | 9 +++++++ .../DatabaseCacheBootstrapper.php | 8 ++++-- src/TenancyServiceProvider.php | 26 +++++++++++++++---- tests/CachedTenantResolverTest.php | 1 + tests/GlobalCacheTest.php | 1 + 5 files changed, 38 insertions(+), 7 deletions(-) diff --git a/src/Bootstrappers/CacheTenancyBootstrapper.php b/src/Bootstrappers/CacheTenancyBootstrapper.php index 97bd7d24..74fc8490 100644 --- a/src/Bootstrappers/CacheTenancyBootstrapper.php +++ b/src/Bootstrappers/CacheTenancyBootstrapper.php @@ -16,6 +16,15 @@ use Stancl\Tenancy\Contracts\Tenant; /** * Makes cache tenant-aware by applying a prefix. + * + * Using this bootstrapper together with DatabaseTenancyBootstrapper + * with a database cache store will result in "double scoping". The store will be scoped + * by the DB connection (entries will go into the tenant's database) *and* by the prefix. + * This is harmless in most cases, but is important to be aware of. + * + * If you only use database cache stores, consider using DatabaseCacheBootstrapper instead. + * + * @see Stancl\Tenancy\Bootstrappers\DatabaseCacheBootstrapper */ class CacheTenancyBootstrapper implements TenancyBootstrapper { diff --git a/src/Bootstrappers/DatabaseCacheBootstrapper.php b/src/Bootstrappers/DatabaseCacheBootstrapper.php index 0e41849f..81611d0a 100644 --- a/src/Bootstrappers/DatabaseCacheBootstrapper.php +++ b/src/Bootstrappers/DatabaseCacheBootstrapper.php @@ -21,11 +21,15 @@ use Stancl\Tenancy\TenancyServiceProvider; * * By default, this bootstrapper scopes ALL cache stores that use the database driver. If you only * want to scope SOME stores, set the static $stores property to an array of names of the stores - * you want to scope. These stores must use 'database' as their driver. + * you want to scope. Those stores must use 'database' as their driver. * * Notably, this bootstrapper sets TenancyServiceProvider::$adjustCacheManagerUsing to a callback * that ensures all affected stores still use the central connection when accessed via global cache - * (typicaly the GlobalCache facade or global_cache() helper). + * (typically the GlobalCache facade or global_cache() helper). The code in TenancyServiceProvider + * that uses `extend()` callbacks to make database stores on the global cache manager use the central + * connection only corrects stores scoped by the Database*Tenancy*Bootstrapper. This bootstrapper + * also changes the stores' connection in the *config* to 'tenant' which doesn't let that callback + * change the connection back to central on the global cache manager. */ class DatabaseCacheBootstrapper implements TenancyBootstrapper { diff --git a/src/TenancyServiceProvider.php b/src/TenancyServiceProvider.php index afd20fb6..23d1ffab 100644 --- a/src/TenancyServiceProvider.php +++ b/src/TenancyServiceProvider.php @@ -86,14 +86,30 @@ class TenancyServiceProvider extends ServiceProvider // This works great for cache stores that are *directly* scoped, like Redis or // any other tagged or prefixed stores, but it doesn't work for the database driver. // - // When we use the DatabaseTenancyBootstrapper, it changes the default connection, - // and therefore the connection of the database store that will be created when - // this new CacheManager is instantiated again. + // When DatabaseTenancyBootstrapper is used, it changes the default DB connection + // to 'tenant'. A freshly created CacheManager would therefore instantiate database + // stores with the tenant connection. // - // For that reason, we also adjust the relevant stores on this new CacheManager - // using the callback below. It is set by DatabaseCacheBootstrapper. + // For that reason, we override the 'database' driver creator on this manager so that + // database stores are built with the central connection, and we run the + // $adjustCacheManagerUsing callback below (set by DatabaseCacheBootstrapper). $manager = new CacheManager($app); + // When DatabaseTenancyBootstrapper is used, database stores whose 'connection' + // config is null fall back to the default DB connection ('tenant'). Reset each + // such store to its explicitly configured connection, or fall back to central. + $centralConnection = $app['config']['tenancy.database.central_connection']; + $manager->extend('database', function ($_app, array $config) use ($centralConnection) { + $config['connection'] ??= $centralConnection; + + /** @var CacheManager $this */ + return $this->createDatabaseDriver($config); // @phpstan-ignore method.protected + }); + + // DatabaseCacheBootstrapper explicitly writes 'tenant' into each store's 'connection' + // config. The extend() closure above would then read 'tenant' as the configured value + // (not null) and use it directly, so the central connection fallback wouldn't be used. + // This callback is used to correct those connections back to central for globalCache. if (static::$adjustCacheManagerUsing !== null) { (static::$adjustCacheManagerUsing)($manager); } diff --git a/tests/CachedTenantResolverTest.php b/tests/CachedTenantResolverTest.php index 920c95a1..fc6cfb79 100644 --- a/tests/CachedTenantResolverTest.php +++ b/tests/CachedTenantResolverTest.php @@ -165,6 +165,7 @@ test('cache is invalidated when tenant is updated from within the tenant context ['redis', [CacheTenancyBootstrapper::class]], ['redis', [CacheTagsBootstrapper::class]], ['database', [DatabaseTenancyBootstrapper::class, DatabaseCacheBootstrapper::class]], + ['database', [DatabaseTenancyBootstrapper::class, CacheTenancyBootstrapper::class]], ]); test('cache is invalidated when the tenant is deleted', function (string $resolver, bool $configureTenantModelColumn) { diff --git a/tests/GlobalCacheTest.php b/tests/GlobalCacheTest.php index 016ad2a4..4cda8b74 100644 --- a/tests/GlobalCacheTest.php +++ b/tests/GlobalCacheTest.php @@ -165,6 +165,7 @@ test('global cache is always central', function (string $store, array $bootstrap ['redis', [CacheTagsBootstrapper::class]], ['redis', [CacheTenancyBootstrapper::class]], ['database', [DatabaseTenancyBootstrapper::class, DatabaseCacheBootstrapper::class]], + ['database', [DatabaseTenancyBootstrapper::class, CacheTenancyBootstrapper::class]], ])->with([ 'helper', 'facade', From aa9d1d7fcf1d74ffe509163cbdec89db4cfbb1eb Mon Sep 17 00:00:00 2001 From: lukinovec Date: Sun, 28 Jun 2026 04:51:30 +0200 Subject: [PATCH 3/6] Parameter validation and other DB manager improvements (#1459) ### Parameter validation In `statement()` calls of `TenantDatabaseManager`s, use parameter binding when possible. When that's not possible, validate the parameters using `validateParameter()` or `validatePassword()`. Passwords use a less strict allowlist than other parameters (e.g. DB names), since passwords tend to use more special characters but we can afford to be more restrictive in the generic `validateParameter()`. In `SQLiteDatabaseManager`, names of file-based databases are validated (in `createDatabase`, `deleteDatabase` and `databaseExists`) using a similar allowlist to the (non-password) parameters in other DB managers, with an additional character: `.` (this addition is necessary since file-based SQLite databases end with `.sqlite`). ### DatabaseTenancyBootstrapper - harden() and the lost test file While checking for more places that could use validation, I realized that it's possible to update tenant's db_name to the central DB or the DB of another tenant. Added the `DatabaseTenancyBootstrapper::$harden` property -- setting it to true prevents tenants from connecting to the wrong databases (`RuntimeException` is thrown after connecting to the wrong database). Also, the DatabaseTenancyBootstrapper test file was ignored while running tests because it lacked the `Test` suffix. Added the suffix and fixed the broken `DATABASE_URL` test in the file. ### SQLiteDatabaseManager - respect static $path property in makeConnectionConfig() SQLiteDatabaseManager had a bug in `makeConnectionConfig`: the method didn't respect the static `$path` property, it used `database_path()` instead. Added a regression test for that. Also recognizing in-memory SQLite databases (using `isInMemory()`) is more strict now so that simply having a db_name with `_tenancy_inmemory_` somewhere in the name doesn't make a file-based database considered in-memory. ### MySQLDatabaseManager - charset and collation defaulting Creating databases with `null` charsets and collations resulted in a `QueryException`, since null isn't a valid charset/collation. To solve that, in the `CREATE DATABASE` statement in MySQLDatabaseManager, only add charset/collation to the statement if they are not null. MySQL defaults to the server's charset and collation, so it's safe to not pass any charset/collation in the `CREATE DATABASE` statement and let MySQL choose. Also, if e.g. collation is non-null and charset is null, MySQL will use a charset compatible with the used collation, and this works both ways. --------- Co-authored-by: Samuel Stancl Co-authored-by: github-actions[bot] Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- .../DatabaseTenancyBootstrapper.php | 57 +++++ .../Concerns/ManagesPostgresUsers.php | 7 +- .../Concerns/ValidatesDatabaseParameters.php | 96 +++++++ .../MicrosoftSQLDatabaseManager.php | 10 +- .../MySQLDatabaseManager.php | 28 ++- ...olledMicrosoftSQLServerDatabaseManager.php | 18 +- ...rmissionControlledMySQLDatabaseManager.php | 12 +- ...ionControlledPostgreSQLDatabaseManager.php | 8 +- ...ssionControlledPostgreSQLSchemaManager.php | 14 +- .../PostgreSQLDatabaseManager.php | 14 +- .../PostgreSQLSchemaManager.php | 14 +- .../SQLiteDatabaseManager.php | 42 +++- .../TenantDatabaseManager.php | 3 + .../DatabaseTenancyBootstrapper.php | 35 --- .../DatabaseTenancyBootstrapperTest.php | 162 ++++++++++++ tests/TenantDatabaseManagerTest.php | 236 ++++++++++++++++++ 16 files changed, 695 insertions(+), 61 deletions(-) create mode 100644 src/Database/Concerns/ValidatesDatabaseParameters.php delete mode 100644 tests/Bootstrappers/DatabaseTenancyBootstrapper.php create mode 100644 tests/Bootstrappers/DatabaseTenancyBootstrapperTest.php diff --git a/src/Bootstrappers/DatabaseTenancyBootstrapper.php b/src/Bootstrappers/DatabaseTenancyBootstrapper.php index 7f0bce0a..08b36f1d 100644 --- a/src/Bootstrappers/DatabaseTenancyBootstrapper.php +++ b/src/Bootstrappers/DatabaseTenancyBootstrapper.php @@ -5,14 +5,42 @@ declare(strict_types=1); namespace Stancl\Tenancy\Bootstrappers; use Exception; +use Illuminate\Support\Facades\Schema; +use RuntimeException; use Stancl\Tenancy\Contracts\TenancyBootstrapper; use Stancl\Tenancy\Contracts\Tenant; use Stancl\Tenancy\Database\Contracts\TenantWithDatabase; use Stancl\Tenancy\Database\DatabaseManager; use Stancl\Tenancy\Database\Exceptions\TenantDatabaseDoesNotExistException; +use Throwable; class DatabaseTenancyBootstrapper implements TenancyBootstrapper { + /** + * When true, throw an exception if a tenant gets connected to + * another tenant's database or to the central database. + * + * This case should never come up in well-configured apps where + * users cannot set or edit tenant IDs or database names, so this + * option is disabled by default. + * + * However, applications dealing with extremely sensitive data may + * choose to enable this runtime check to prevent a bug or misconfiguration + * from creating an exploit that would let an attacker access another + * tenant's data or data from the central database. + * + * One way such a scenario might come up is if an application allows + * broad tenant attribute updates on a page for updating some fields + * on the tenant, without restricting that action to only a limited + * set of fields that are safe to edit. An attacker might be able to add + * something like ['tenancy_db_name' => '...'] to the request which could + * lead to this internal attribute being updated on an existing tenant. + * + * It's possible that enabling this setting will negate the performance + * benefits of cached tenant lookup. + */ + public static bool $harden = false; + /** @var DatabaseManager */ protected $database; @@ -41,10 +69,39 @@ class DatabaseTenancyBootstrapper implements TenancyBootstrapper } $this->database->connectToTenant($tenant); + + if (static::$harden) { + try { + $this->verifyTenantCanUseDatabase($tenant); + } catch (Throwable $e) { + // Revert connection back to central + $this->revert(); + + throw $e; + } + } } public function revert(): void { $this->database->reconnectToCentral(); } + + protected function verifyTenantCanUseDatabase(Tenant $tenant): void + { + /** @var \Stancl\Tenancy\Database\Models\Tenant&TenantWithDatabase $tenant */ + $tenantDbName = $tenant->database()->getName(); + + // Check that no other tenant uses this tenant's database + if ($tenant::where($tenant->getTenantKeyName(), '!=', $tenant->getTenantKey()) + ->where($tenant::getDataColumn() . '->' . $tenant->internalPrefix() . 'db_name', $tenantDbName) + ->exists()) { + throw new RuntimeException('Tenant cannot use a database of another tenant.'); + } + + if (Schema::hasTable($tenant->getTable())) { + // Throw if the current database/schema has the tenants table (i.e. it's not central) + throw new RuntimeException('Tenant cannot use the central database.'); + } + } } diff --git a/src/Database/Concerns/ManagesPostgresUsers.php b/src/Database/Concerns/ManagesPostgresUsers.php index bec94f49..62b920b9 100644 --- a/src/Database/Concerns/ManagesPostgresUsers.php +++ b/src/Database/Concerns/ManagesPostgresUsers.php @@ -28,6 +28,9 @@ trait ManagesPostgresUsers $username = $databaseConfig->getUsername(); $password = $databaseConfig->getPassword(); + $this->validateParameter($username); + $this->validatePassword($password); + $createUser = ! $this->userExists($username); if ($createUser) { @@ -44,6 +47,8 @@ trait ManagesPostgresUsers // Tenant DB username $username = $databaseConfig->getUsername(); + $this->validateParameter($username); + // Tenant host connection config $connectionName = $this->connection()->getConfig('name'); $centralDatabase = $this->connection()->getConfig('database'); @@ -77,6 +82,6 @@ trait ManagesPostgresUsers public function userExists(string $username): bool { - return (bool) $this->connection()->selectOne("SELECT usename FROM pg_user WHERE usename = '{$username}'"); + return (bool) $this->connection()->select('SELECT usename FROM pg_user WHERE usename = ?', [$username]); } } diff --git a/src/Database/Concerns/ValidatesDatabaseParameters.php b/src/Database/Concerns/ValidatesDatabaseParameters.php new file mode 100644 index 00000000..d6d01a03 --- /dev/null +++ b/src/Database/Concerns/ValidatesDatabaseParameters.php @@ -0,0 +1,96 @@ +?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[]^_abcdefghijklmnopqrstuvwxyz{|}~'; + + /** + * Ensure that parameter (database name, username, etc.) + * only contains allowed characters before being used in SQL statements + * (or paths in the case of SQLiteDatabaseManager). + * + * By default, only the characters in $allowedParameterCharacters are allowed. + * + * @throws InvalidArgumentException + */ + protected function validateParameter(mixed $parameter, string|null $allowedCharacters = null): void + { + if (is_null($parameter)) { + throw new InvalidArgumentException('Parameter cannot be null.'); + } + + if (is_numeric($parameter)) { + $parameter = (string) $parameter; + } + + if (! is_string($parameter)) { + throw new InvalidArgumentException('Parameter has to be a string.'); + } + + if ($parameter === '') { + throw new InvalidArgumentException('Parameter cannot be an empty string.'); + } + + $allowedCharacters ??= static::$allowedParameterCharacters; + + foreach (str_split($parameter) as $character) { + if (! str_contains($allowedCharacters, $character)) { + throw new InvalidArgumentException("Forbidden character '{$character}' in parameter."); + } + } + } + + /** + * Ensure password only contains allowed characters ($allowedPasswordCharacters) + * before being used in SQL statements. + * + * Used in permission controlled managers as a shorthand for calling validateParameter() + * with the less strict allowlist to validate database user passwords. + * + * @throws InvalidArgumentException + */ + protected function validatePassword(string|null $password): void + { + if (is_null($password)) { + throw new InvalidArgumentException('Password cannot be null.'); + } + + if ($password === '') { + throw new InvalidArgumentException('Password cannot be an empty string.'); + } + + $this->validateParameter($password, allowedCharacters: static::$allowedPasswordCharacters); + } +} diff --git a/src/Database/TenantDatabaseManagers/MicrosoftSQLDatabaseManager.php b/src/Database/TenantDatabaseManagers/MicrosoftSQLDatabaseManager.php index da993956..f28ffd1e 100644 --- a/src/Database/TenantDatabaseManagers/MicrosoftSQLDatabaseManager.php +++ b/src/Database/TenantDatabaseManagers/MicrosoftSQLDatabaseManager.php @@ -12,16 +12,22 @@ class MicrosoftSQLDatabaseManager extends TenantDatabaseManager { $database = $tenant->database()->getName(); + $this->validateParameter($database); + return $this->connection()->statement("CREATE DATABASE [{$database}]"); } public function deleteDatabase(TenantWithDatabase $tenant): bool { - return $this->connection()->statement("DROP DATABASE [{$tenant->database()->getName()}]"); + $database = $tenant->database()->getName(); + + $this->validateParameter($database); + + return $this->connection()->statement("DROP DATABASE [{$database}]"); } public function databaseExists(string $name): bool { - return (bool) $this->connection()->select("SELECT name FROM master.sys.databases WHERE name = '$name'"); + return (bool) $this->connection()->select('SELECT name FROM master.sys.databases WHERE name = ?', [$name]); } } diff --git a/src/Database/TenantDatabaseManagers/MySQLDatabaseManager.php b/src/Database/TenantDatabaseManagers/MySQLDatabaseManager.php index b86faef2..10385208 100644 --- a/src/Database/TenantDatabaseManagers/MySQLDatabaseManager.php +++ b/src/Database/TenantDatabaseManagers/MySQLDatabaseManager.php @@ -14,16 +14,38 @@ class MySQLDatabaseManager extends TenantDatabaseManager $charset = $this->connection()->getConfig('charset'); $collation = $this->connection()->getConfig('collation'); - return $this->connection()->statement("CREATE DATABASE `{$database}` CHARACTER SET `$charset` COLLATE `$collation`"); + $this->validateParameter($database); + + // MySQL defaults to the server's charset and collation + // if charset and collation are not specified. + // If charset is specified but collation is null, MySQL + // will choose a default collation for the specified charset (and vice versa). + $statement = "CREATE DATABASE `{$database}`"; + + if ($charset !== null) { + $this->validateParameter($charset); + $statement .= " CHARACTER SET `{$charset}`"; + } + + if ($collation !== null) { + $this->validateParameter($collation); + $statement .= " COLLATE `{$collation}`"; + } + + return $this->connection()->statement($statement); } public function deleteDatabase(TenantWithDatabase $tenant): bool { - return $this->connection()->statement("DROP DATABASE `{$tenant->database()->getName()}`"); + $database = $tenant->database()->getName(); + + $this->validateParameter($database); + + return $this->connection()->statement("DROP DATABASE `{$database}`"); } public function databaseExists(string $name): bool { - return (bool) $this->connection()->select("SELECT SCHEMA_NAME FROM INFORMATION_SCHEMA.SCHEMATA WHERE SCHEMA_NAME = '$name'"); + return (bool) $this->connection()->select('SELECT SCHEMA_NAME FROM INFORMATION_SCHEMA.SCHEMATA WHERE SCHEMA_NAME = ?', [$name]); } } diff --git a/src/Database/TenantDatabaseManagers/PermissionControlledMicrosoftSQLServerDatabaseManager.php b/src/Database/TenantDatabaseManagers/PermissionControlledMicrosoftSQLServerDatabaseManager.php index b373f41e..b10b82bc 100644 --- a/src/Database/TenantDatabaseManagers/PermissionControlledMicrosoftSQLServerDatabaseManager.php +++ b/src/Database/TenantDatabaseManagers/PermissionControlledMicrosoftSQLServerDatabaseManager.php @@ -24,6 +24,10 @@ class PermissionControlledMicrosoftSQLServerDatabaseManager extends MicrosoftSQL $username = $databaseConfig->getUsername(); $password = $databaseConfig->getPassword(); + $this->validateParameter($database); + $this->validateParameter($username); + $this->validatePassword($password); + // Create login $this->connection()->statement("CREATE LOGIN [$username] WITH PASSWORD = '$password'"); @@ -37,12 +41,16 @@ class PermissionControlledMicrosoftSQLServerDatabaseManager extends MicrosoftSQL public function deleteUser(DatabaseConfig $databaseConfig): bool { - return $this->connection()->statement("DROP LOGIN [{$databaseConfig->getUsername()}]"); + $username = $databaseConfig->getUsername(); + + $this->validateParameter($username); + + return $this->connection()->statement("DROP LOGIN [{$username}]"); } public function userExists(string $username): bool { - return (bool) $this->connection()->select("SELECT sp.name as username FROM sys.server_principals sp WHERE sp.name = '{$username}'"); + return (bool) $this->connection()->select('SELECT sp.name as username FROM sys.server_principals sp WHERE sp.name = ?', [$username]); } public function makeConnectionConfig(array $baseConfig, string $databaseName): array @@ -54,11 +62,15 @@ class PermissionControlledMicrosoftSQLServerDatabaseManager extends MicrosoftSQL public function deleteDatabase(TenantWithDatabase $tenant): bool { + $name = $tenant->database()->getName(); + + $this->validateParameter($name); + // Close all connections to the database before deleting it // Set the database to SINGLE_USER mode to ensure that // No other connections are using the database while we're trying to delete it // Rollback all active transactions - $this->connection()->statement("ALTER DATABASE [{$tenant->database()->getName()}] SET SINGLE_USER WITH ROLLBACK IMMEDIATE;"); + $this->connection()->statement("ALTER DATABASE [{$name}] SET SINGLE_USER WITH ROLLBACK IMMEDIATE;"); return parent::deleteDatabase($tenant); } diff --git a/src/Database/TenantDatabaseManagers/PermissionControlledMySQLDatabaseManager.php b/src/Database/TenantDatabaseManagers/PermissionControlledMySQLDatabaseManager.php index 47ec11a2..421b3bc3 100644 --- a/src/Database/TenantDatabaseManagers/PermissionControlledMySQLDatabaseManager.php +++ b/src/Database/TenantDatabaseManagers/PermissionControlledMySQLDatabaseManager.php @@ -25,6 +25,10 @@ class PermissionControlledMySQLDatabaseManager extends MySQLDatabaseManager impl $username = $databaseConfig->getUsername(); $password = $databaseConfig->getPassword(); + $this->validateParameter($database); + $this->validateParameter($username); + $this->validatePassword($password); + $this->connection()->statement("CREATE USER `{$username}`@`%` IDENTIFIED BY '{$password}'"); $grants = implode(', ', static::$grants); @@ -48,11 +52,15 @@ class PermissionControlledMySQLDatabaseManager extends MySQLDatabaseManager impl public function deleteUser(DatabaseConfig $databaseConfig): bool { - return $this->connection()->statement("DROP USER IF EXISTS '{$databaseConfig->getUsername()}'"); + $username = $databaseConfig->getUsername(); + + $this->validateParameter($username); + + return $this->connection()->statement("DROP USER IF EXISTS '{$username}'"); } public function userExists(string $username): bool { - return (bool) $this->connection()->select("SELECT count(*) FROM mysql.user WHERE user = '$username'")[0]->{'count(*)'}; + return (bool) $this->connection()->select('SELECT count(*) FROM mysql.user WHERE user = ?', [$username])[0]->{'count(*)'}; } } diff --git a/src/Database/TenantDatabaseManagers/PermissionControlledPostgreSQLDatabaseManager.php b/src/Database/TenantDatabaseManagers/PermissionControlledPostgreSQLDatabaseManager.php index 1522234e..c846ace9 100644 --- a/src/Database/TenantDatabaseManagers/PermissionControlledPostgreSQLDatabaseManager.php +++ b/src/Database/TenantDatabaseManagers/PermissionControlledPostgreSQLDatabaseManager.php @@ -20,6 +20,10 @@ class PermissionControlledPostgreSQLDatabaseManager extends PostgreSQLDatabaseMa $username = $databaseConfig->getUsername(); $schema = $databaseConfig->connection()['search_path']; + $this->validateParameter($database); + $this->validateParameter($username); + $this->validateParameter($schema); + // Host config $connectionName = $this->connection()->getConfig('name'); $centralDatabase = $this->connection()->getConfig('database'); @@ -32,10 +36,10 @@ class PermissionControlledPostgreSQLDatabaseManager extends PostgreSQLDatabaseMa $this->connection()->reconnect(); // Grant permissions to create and use tables in the configured schema ("public" by default) to the user - $this->connection()->statement("GRANT USAGE, CREATE ON SCHEMA {$schema} TO \"{$username}\""); + $this->connection()->statement("GRANT USAGE, CREATE ON SCHEMA \"{$schema}\" TO \"{$username}\""); // Grant permissions to use sequences in the current schema to the user - $this->connection()->statement("GRANT USAGE ON ALL SEQUENCES IN SCHEMA {$schema} TO \"{$username}\""); + $this->connection()->statement("GRANT USAGE ON ALL SEQUENCES IN SCHEMA \"{$schema}\" TO \"{$username}\""); // Reconnect to central database config(["database.connections.{$connectionName}.database" => $centralDatabase]); diff --git a/src/Database/TenantDatabaseManagers/PermissionControlledPostgreSQLSchemaManager.php b/src/Database/TenantDatabaseManagers/PermissionControlledPostgreSQLSchemaManager.php index b528d4e3..b972ba0b 100644 --- a/src/Database/TenantDatabaseManagers/PermissionControlledPostgreSQLSchemaManager.php +++ b/src/Database/TenantDatabaseManagers/PermissionControlledPostgreSQLSchemaManager.php @@ -23,23 +23,27 @@ class PermissionControlledPostgreSQLSchemaManager extends PostgreSQLSchemaManage // Central database name $database = DB::connection(config('tenancy.database.central_connection'))->getDatabaseName(); - $this->connection()->statement("GRANT CONNECT ON DATABASE {$database} TO \"{$username}\""); + $this->validateParameter($username); + $this->validateParameter($schema); + $this->validateParameter($database); + + $this->connection()->statement("GRANT CONNECT ON DATABASE \"{$database}\" TO \"{$username}\""); $this->connection()->statement("GRANT USAGE, CREATE ON SCHEMA \"{$schema}\" TO \"{$username}\""); $this->connection()->statement("GRANT USAGE ON ALL SEQUENCES IN SCHEMA \"{$schema}\" TO \"{$username}\""); - $tables = $this->connection()->select("SELECT table_name FROM information_schema.tables WHERE table_schema = '{$schema}' AND table_type = 'BASE TABLE'"); + $tables = $this->connection()->select("SELECT table_name FROM information_schema.tables WHERE table_schema = ? AND table_type = 'BASE TABLE'", [$schema]); // Grant permissions to any existing tables. This is used with RLS foreach ($tables as $table) { $tableName = $table->table_name; /** @var string $primaryKey */ - $primaryKey = $this->connection()->selectOne(<<connection()->selectOne(<<<'SQL' SELECT column_name FROM information_schema.key_column_usage - WHERE table_name = '{$tableName}' + WHERE table_name = ? AND constraint_name LIKE '%_pkey' - SQL)->column_name; + SQL, [$tableName])->column_name; // Grant all permissions for all existing tables $this->connection()->statement("GRANT ALL ON \"{$schema}\".\"{$tableName}\" TO \"{$username}\""); diff --git a/src/Database/TenantDatabaseManagers/PostgreSQLDatabaseManager.php b/src/Database/TenantDatabaseManagers/PostgreSQLDatabaseManager.php index 4fff7202..fc293403 100644 --- a/src/Database/TenantDatabaseManagers/PostgreSQLDatabaseManager.php +++ b/src/Database/TenantDatabaseManagers/PostgreSQLDatabaseManager.php @@ -10,16 +10,24 @@ class PostgreSQLDatabaseManager extends TenantDatabaseManager { public function createDatabase(TenantWithDatabase $tenant): bool { - return $this->connection()->statement("CREATE DATABASE \"{$tenant->database()->getName()}\" WITH TEMPLATE=template0"); + $name = $tenant->database()->getName(); + + $this->validateParameter($name); + + return $this->connection()->statement("CREATE DATABASE \"{$name}\" WITH TEMPLATE=template0"); } public function deleteDatabase(TenantWithDatabase $tenant): bool { - return $this->connection()->statement("DROP DATABASE \"{$tenant->database()->getName()}\""); + $name = $tenant->database()->getName(); + + $this->validateParameter($name); + + return $this->connection()->statement("DROP DATABASE \"{$name}\""); } public function databaseExists(string $name): bool { - return (bool) $this->connection()->selectOne("SELECT datname FROM pg_database WHERE datname = '$name'"); + return (bool) $this->connection()->select('SELECT datname FROM pg_database WHERE datname = ?', [$name]); } } diff --git a/src/Database/TenantDatabaseManagers/PostgreSQLSchemaManager.php b/src/Database/TenantDatabaseManagers/PostgreSQLSchemaManager.php index d0fb0337..354eb768 100644 --- a/src/Database/TenantDatabaseManagers/PostgreSQLSchemaManager.php +++ b/src/Database/TenantDatabaseManagers/PostgreSQLSchemaManager.php @@ -10,17 +10,25 @@ class PostgreSQLSchemaManager extends TenantDatabaseManager { public function createDatabase(TenantWithDatabase $tenant): bool { - return $this->connection()->statement("CREATE SCHEMA \"{$tenant->database()->getName()}\""); + $name = $tenant->database()->getName(); + + $this->validateParameter($name); + + return $this->connection()->statement("CREATE SCHEMA \"{$name}\""); } public function deleteDatabase(TenantWithDatabase $tenant): bool { - return $this->connection()->statement("DROP SCHEMA \"{$tenant->database()->getName()}\" CASCADE"); + $name = $tenant->database()->getName(); + + $this->validateParameter($name); + + return $this->connection()->statement("DROP SCHEMA \"{$name}\" CASCADE"); } public function databaseExists(string $name): bool { - return (bool) $this->connection()->select("SELECT schema_name FROM information_schema.schemata WHERE schema_name = '$name'"); + return (bool) $this->connection()->select('SELECT schema_name FROM information_schema.schemata WHERE schema_name = ?', [$name]); } public function makeConnectionConfig(array $baseConfig, string $databaseName): array diff --git a/src/Database/TenantDatabaseManagers/SQLiteDatabaseManager.php b/src/Database/TenantDatabaseManagers/SQLiteDatabaseManager.php index 295cf304..ce3582c0 100644 --- a/src/Database/TenantDatabaseManagers/SQLiteDatabaseManager.php +++ b/src/Database/TenantDatabaseManagers/SQLiteDatabaseManager.php @@ -6,13 +6,17 @@ namespace Stancl\Tenancy\Database\TenantDatabaseManagers; use Closure; use Illuminate\Database\Eloquent\Model; +use InvalidArgumentException; use PDO; +use Stancl\Tenancy\Database\Concerns\ValidatesDatabaseParameters; use Stancl\Tenancy\Database\Contracts\TenantDatabaseManager; use Stancl\Tenancy\Database\Contracts\TenantWithDatabase; use Throwable; class SQLiteDatabaseManager implements TenantDatabaseManager { + use ValidatesDatabaseParameters; + /** * SQLite database directory path. * @@ -57,6 +61,13 @@ class SQLiteDatabaseManager implements TenantDatabaseManager */ public static Closure|null $closeInMemoryConnectionUsing = null; + /** + * Characters allowed in database names. + * + * Includes dots to support file extensions (e.g. '.sqlite'). + */ + public static string $allowedDatabaseNameCharacters = 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-.'; + public function createDatabase(TenantWithDatabase $tenant): bool { /** @var TenantWithDatabase&Model $tenant */ @@ -122,6 +133,9 @@ class SQLiteDatabaseManager implements TenantDatabaseManager public function makeConnectionConfig(array $baseConfig, string $databaseName): array { if ($this->isInMemory($databaseName)) { + // Named in-memory DBs are formatted like 'file:_tenancy_inmemory_tenant123?mode=memory&cache=shared' + $this->validateDatabaseName($databaseName, extraAllowedCharacters: ':?=&'); + $baseConfig['database'] = $databaseName; if (static::$persistInMemoryConnectionUsing !== null) { @@ -129,7 +143,7 @@ class SQLiteDatabaseManager implements TenantDatabaseManager (static::$persistInMemoryConnectionUsing)(new PDO($dsn), $dsn); } } else { - $baseConfig['database'] = database_path($databaseName); + $baseConfig['database'] = $this->getPath($databaseName); } return $baseConfig; @@ -137,6 +151,8 @@ class SQLiteDatabaseManager implements TenantDatabaseManager public function getPath(string $name): string { + $this->validateDatabaseName($name); + if (static::$path) { return rtrim(static::$path, DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR . $name; } @@ -146,6 +162,28 @@ class SQLiteDatabaseManager implements TenantDatabaseManager public static function isInMemory(string $name): bool { - return $name === ':memory:' || str_contains($name, '_tenancy_inmemory_'); + $isNamed = str_starts_with($name, 'file:_tenancy_inmemory_') && + str_ends_with($name, '?mode=memory&cache=shared'); + + return $name === ':memory:' || $isNamed; + } + + /** + * Ensure database name only contains allowed characters + * (allowedDatabaseNameCharacters() + $extraAllowedCharacters) and is not a directory name. + * + * @throws InvalidArgumentException + */ + protected function validateDatabaseName(string $name, string $extraAllowedCharacters = ''): void + { + $this->validateParameter($name, static::$allowedDatabaseNameCharacters . $extraAllowedCharacters); + + if ($name === '') { + throw new InvalidArgumentException('Database name cannot be empty.'); + } + + if (is_dir($name)) { + throw new InvalidArgumentException('Database name cannot be a directory.'); + } } } diff --git a/src/Database/TenantDatabaseManagers/TenantDatabaseManager.php b/src/Database/TenantDatabaseManagers/TenantDatabaseManager.php index 3d8d7610..a0822615 100644 --- a/src/Database/TenantDatabaseManagers/TenantDatabaseManager.php +++ b/src/Database/TenantDatabaseManagers/TenantDatabaseManager.php @@ -6,11 +6,14 @@ namespace Stancl\Tenancy\Database\TenantDatabaseManagers; use Illuminate\Database\Connection; use Illuminate\Support\Facades\DB; +use Stancl\Tenancy\Database\Concerns\ValidatesDatabaseParameters; use Stancl\Tenancy\Database\Contracts\StatefulTenantDatabaseManager; use Stancl\Tenancy\Database\Exceptions\NoConnectionSetException; abstract class TenantDatabaseManager implements StatefulTenantDatabaseManager { + use ValidatesDatabaseParameters; + /** The database connection to the server. */ protected string $connection; diff --git a/tests/Bootstrappers/DatabaseTenancyBootstrapper.php b/tests/Bootstrappers/DatabaseTenancyBootstrapper.php deleted file mode 100644 index 14109500..00000000 --- a/tests/Bootstrappers/DatabaseTenancyBootstrapper.php +++ /dev/null @@ -1,35 +0,0 @@ - $databaseUrl]); - - pest()->expectException(Exception::class); - } - - config(['tenancy.bootstrappers' => [DatabaseTenancyBootstrapper::class]]); - - $tenant1 = Tenant::create(); - - pest()->artisan('tenants:migrate'); - - tenancy()->initialize($tenant1); - - expect(true)->toBe(true); -})->with(['abc.us-east-1.rds.amazonaws.com', null]); - diff --git a/tests/Bootstrappers/DatabaseTenancyBootstrapperTest.php b/tests/Bootstrappers/DatabaseTenancyBootstrapperTest.php new file mode 100644 index 00000000..63762672 --- /dev/null +++ b/tests/Bootstrappers/DatabaseTenancyBootstrapperTest.php @@ -0,0 +1,162 @@ + [DatabaseTenancyBootstrapper::class], + "tenancy.database.managers.{$connection}" => $manager, + ]); + + // Point the central connection at the tested connection's config and migrate it + // (so that the central database/schema contains the tenants table). + $centralConnection = config('tenancy.database.central_connection'); + $centralConfig = config("database.connections.{$connection}"); + + if ($connection === 'sqlite') { + $centralConfig['database'] = database_path($sqliteCentralDb = 'central.sqlite'); + } + + DB::purge($centralConnection); + config(["database.connections.{$centralConnection}" => $centralConfig]); + + pest()->artisan('migrate:fresh', [ + '--force' => true, + '--path' => __DIR__ . '/../../assets/migrations', + '--realpath' => true, + ]); + + DatabaseTenancyBootstrapper::$harden = $harden; + + Event::listen(TenantCreated::class, JobPipeline::make([CreateDatabase::class])->send(function (TenantCreated $event) { + return $event->tenant; + })->toListener()); + + // Create the tenant with its own database, then repoint it at the central database/schema + // (which contains the tenants table that the hardening check looks for). + $tenant = Tenant::create(['tenancy_db_connection' => $connection]); + + $central = DB::connection($centralConnection); + $centralName = match (true) { + $manager === PostgreSQLSchemaManager::class => $central->selectOne('SELECT current_schema() AS schema')->schema, // Central schema name + $connection === 'sqlite' => $sqliteCentralDb, // Central SQLite DB name + default => $central->getDatabaseName(), // Central DB name + }; + + $tenant->update(['tenancy_db_name' => $centralName]); + + if ($harden) { + // Harden blocks initialization for tenants that use the central database + expect(fn () => tenancy()->initialize($tenant))->toThrow(RuntimeException::class); + + // Connection should be reverted back to central + expect(DB::connection()->getName())->toBe($centralConnection); + } else { + expect(fn () => tenancy()->initialize($tenant))->not()->toThrow(Throwable::class); + + // Connection not reverted to central + expect(DB::connection()->getName())->toBe('tenant'); + } +})->with([ + 'hardening enabled' => true, + 'hardening disabled' => false, +])->with('db_managers'); + +test('harden prevents tenants from using the database of another tenant', function (bool $harden, string $connection, string $manager) { + config([ + 'tenancy.bootstrappers' => [DatabaseTenancyBootstrapper::class], + "tenancy.database.managers.{$connection}" => $manager, + ]); + + DatabaseTenancyBootstrapper::$harden = $harden; + + Event::listen(TenantCreated::class, JobPipeline::make([CreateDatabase::class])->send(function (TenantCreated $event) { + return $event->tenant; + })->toListener()); + + $tenant = Tenant::create(['tenancy_db_connection' => $connection]); + + $dbName = Str::random(8) . ($connection === 'sqlite' ? '.sqlite' : ''); + + Tenant::create(['tenancy_db_name' => $dbName, 'tenancy_db_connection' => $connection]); + + $tenant->update(['tenancy_db_name' => $dbName]); + + if ($harden) { + // Harden blocks initialization for tenants that use the database of another tenant + expect(fn () => tenancy()->initialize($tenant))->toThrow(RuntimeException::class); + + // Connection should be reverted back to central + expect(DB::connection()->getName())->toBe('central'); + } else { + expect(fn() => tenancy()->initialize($tenant))->not()->toThrow(Throwable::class); + + // Connection not reverted to central + expect(DB::connection()->getName())->toBe('tenant'); + } +})->with([ + 'hardening enabled' => true, + 'hardening disabled' => false, +])->with('db_managers'); + +test('database tenancy bootstrapper throws an exception if DATABASE_URL is set', function (string|null $databaseUrl) { + config(['database.connections.central.url' => $databaseUrl]); + + config(['tenancy.bootstrappers' => [DatabaseTenancyBootstrapper::class]]); + + Event::listen(TenantCreated::class, JobPipeline::make([CreateDatabase::class])->send(function (TenantCreated $event) { + return $event->tenant; + })->toListener()); + + if ($databaseUrl) { + expect(fn() => Tenant::create())->toThrow(QueryException::class); + } else { + expect(function() { + $tenant1 = Tenant::create(); + + pest()->artisan('tenants:migrate'); + + tenancy()->initialize($tenant1); + })->not()->toThrow(Throwable::class); + } +})->with(['abc.us-east-1.rds.amazonaws.com', null]); + +// Database managers to test with hardening. +// Permission controlled managers omitted as they inherit the non-perm controlled managers (= they share the same code paths), +// each important code path is covered by testing the non-permission controlled manager, so adding permission controlled managers +// would add unnecessary complexity to the tests. +dataset('db_managers', [ + 'mysql' => ['mysql', MySQLDatabaseManager::class], + 'pgsql (database)' => ['pgsql', PostgreSQLDatabaseManager::class], + 'pgsql (schema)' => ['pgsql', PostgreSQLSchemaManager::class], + 'sqlite' => ['sqlite', SQLiteDatabaseManager::class], +]); diff --git a/tests/TenantDatabaseManagerTest.php b/tests/TenantDatabaseManagerTest.php index 0d83e70e..d95cb1f0 100644 --- a/tests/TenantDatabaseManagerTest.php +++ b/tests/TenantDatabaseManagerTest.php @@ -17,6 +17,7 @@ use Stancl\Tenancy\Events\TenancyInitialized; use Stancl\Tenancy\Listeners\BootstrapTenancy; use Stancl\Tenancy\Listeners\RevertToCentralContext; use Stancl\Tenancy\Bootstrappers\DatabaseTenancyBootstrapper; +use Stancl\Tenancy\Database\Contracts\ManagesDatabaseUsers; use Stancl\Tenancy\Database\Contracts\StatefulTenantDatabaseManager; use Stancl\Tenancy\Database\TenantDatabaseManagers\MySQLDatabaseManager; use Stancl\Tenancy\Database\TenantDatabaseManagers\SQLiteDatabaseManager; @@ -36,6 +37,10 @@ beforeEach(function () { SQLiteDatabaseManager::$path = null; }); +afterEach(function () { + SQLiteDatabaseManager::$path = null; +}); + test('databases can be created and deleted', function ($driver, $databaseManager) { Event::listen(TenantCreated::class, JobPipeline::make([CreateDatabase::class])->send(function (TenantCreated $event) { return $event->tenant; @@ -539,6 +544,237 @@ test('partial tenant connection templates get merged into the central connection expect($manager->connection()->getConfig('url'))->toBeNull(); }); +test('database managers validate parameters used in raw sql statements', function ($driver, $databaseManager) { + config()->set([ + "tenancy.database.template_tenant_connection" => $driver, + ]); + + $manager = app($databaseManager); + + if ($manager instanceof StatefulTenantDatabaseManager) { + $manager->setConnection($driver); + } + + $invalidDatabaseName = "\"database_with_quotes\""; + + if (! ($manager instanceof ManagesDatabaseUsers)) { + // Only test createDatabase() and deleteDatabase() with non-permission controlled managers here + // since permission controlled managers override these methods to e.g. delete users before + // calling parent::deleteDatabase(), and with invalid DB name, the user deletion will already + // fail before we even get to actual deleteDatabase() logic. + $tenant = Tenant::make([ + 'tenancy_db_name' => $invalidDatabaseName, + ]); + + expect(fn () => $manager->createDatabase($tenant)) + ->toThrow(InvalidArgumentException::class, 'Forbidden character'); + + expect(fn () => $manager->deleteDatabase($tenant)) + ->toThrow(InvalidArgumentException::class, 'Forbidden character'); + } else { + // Invalid username, createUser() and deleteUser() should + // throw an invalid argument exception. + $tenantWithInvalidUsername = Tenant::make([ + 'tenancy_db_name' => 'valid_database_name890', + 'tenancy_db_username' => "username with spaces", + ]); + + expect(fn () => $manager->createUser($tenantWithInvalidUsername->database())) + ->toThrow(InvalidArgumentException::class, 'Forbidden character'); + + expect(fn () => $manager->deleteUser($tenantWithInvalidUsername->database())) + ->toThrow(InvalidArgumentException::class, 'Forbidden character'); + + // Invalid database name, createUser() should throw + // an invalid argument exception. deleteUser() doesn't + // validate the DB name (it only validates the username). + $tenantWithInvalidDatabase = Tenant::make([ + 'tenancy_db_name' => $invalidDatabaseName, + 'tenancy_db_username' => 'valid_USERNAME', + 'tenancy_db_password' => 'valid_password', + ]); + + expect(fn () => $manager->createUser($tenantWithInvalidDatabase->database())) + ->toThrow(InvalidArgumentException::class, 'Forbidden character'); + + $tenantWithInvalidPassword = Tenant::make([ + 'tenancy_db_name' => 'valid_database_name890', + 'tenancy_db_username' => 'valid_USERNAME', + 'tenancy_db_password' => "p'ssword", + ]); + + expect(fn () => $manager->createUser($tenantWithInvalidPassword->database())) + ->toThrow(InvalidArgumentException::class, 'Forbidden character'); + + // Special characters are allowed in passwords + $tenantWithValidPassword = Tenant::make([ + 'tenancy_db_name' => 'valid_database_name890' . Str::random(8), + 'tenancy_db_username' => 'valid_USERNAME' . Str::random(8), + 'tenancy_db_password' => "]pa$$ ;word", + ]); + + expect(fn () => $manager->createUser($tenantWithValidPassword->database())) + ->not()->toThrow(InvalidArgumentException::class, 'Forbidden character'); + + $tenantWithNullCredentials = Tenant::make([ + 'tenancy_db_name' => 'valid_db_name', + 'tenancy_db_username' => null, + 'tenancy_db_password' => null, + ]); + + expect(fn () => $manager->createUser($tenantWithNullCredentials->database())) + ->toThrow(InvalidArgumentException::class, 'Parameter cannot be null.'); + } +})->with('database_managers'); + +test('mysql database manager validates charset and collation correctly', function (string $param) { + $manager = app(MySQLDatabaseManager::class); + $manager->setConnection('mysql'); + + // using a non-string value (empty array) which is invalid + config(["database.connections.mysql.$param" => []]); + DB::purge('mysql'); + + $tenant = Tenant::make([ + 'tenancy_db_name' => 'valid_db_name', + ]); + + expect(fn () => $manager->createDatabase($tenant)) + ->toThrow(InvalidArgumentException::class, 'Parameter has to be a string.'); +})->with(['charset', 'collation']); + +test('sqlite database manager validates database names correctly', function () { + $manager = app(SQLiteDatabaseManager::class); + + // Dots are allowed in database names + expect(fn () => $manager->databaseExists('valid-db_name.sqlite')) + ->not()->toThrow(InvalidArgumentException::class); + + // Directory names are considered invalid input for database names + expect(fn () => $manager->databaseExists('..')) + ->toThrow(InvalidArgumentException::class); + + // Empty strings are considered invalid input for database names + expect(fn () => $manager->databaseExists('')) + ->toThrow(InvalidArgumentException::class); +}); + +test('sqlite database manager recognizes inmemory databases correctly', function () { + $manager = app(SQLiteDatabaseManager::class); + + expect($manager->isInMemory('file:_tenancy_inmemory_123?mode=memory&cache=shared'))->toBeTrue(); + expect($manager->isInMemory(':memory:'))->toBeTrue(); + + // Missing the '?mode=memory&cache=shared' suffix + expect($manager->isInMemory('file:_tenancy_inmemory_456'))->toBeFalse(); + + // Doesn't start with 'file:_tenancy_inmemory_' + expect($manager->isInMemory('_tenancy_inmemory_123?mode=memory&cache=shared'))->toBeFalse(); + + // In-memory DB name is validated correctly in makeConnectionConfig() + expect(fn () => $manager->makeConnectionConfig([], 'file:_tenancy_inmemory_12"3?mode=memory&cache=shared')) + ->toThrow(InvalidArgumentException::class, 'Forbidden character'); + + expect(fn () => $manager->makeConnectionConfig([], 'file:_tenancy_inmemory_123?mode=memory&cache=shared')) + ->not()->toThrow(InvalidArgumentException::class); + + expect(fn () => $manager->makeConnectionConfig([], ':memory:')) + ->not()->toThrow(InvalidArgumentException::class); +}); + +test('sqlite database manager respects the configured path while making the database config', function () { + config()->set([ + 'tenancy.database.template_tenant_connection' => 'sqlite', + ]); + + $tenant = Tenant::make([ + 'tenancy_db_name' => 'tenant.sqlite', + ]); + + // SQLiteDatabaseManager::$path is null, the database path is built using database_path() + expect($tenant->database()->connection()['database'])->toBe(database_path('tenant.sqlite')); + + SQLiteDatabaseManager::$path = '/custom/path/'; + + expect($tenant->database()->connection()['database'])->toBe('/custom/path/tenant.sqlite'); +}); + +test('newly created tenant databases use the correct charset and collation with mysql', function () { + config([ + 'tenancy.bootstrappers' => [DatabaseTenancyBootstrapper::class], + 'database.connections.mysql.charset' => 'utf8mb4', + 'database.connections.mysql.collation' => 'utf8mb4_unicode_ci', + ]); + + Event::listen(TenantCreated::class, JobPipeline::make([CreateDatabase::class])->send(function (TenantCreated $event) { + return $event->tenant; + })->toListener()); + + withBootstrapping(); + + $serverDefaultCharset = DB::selectOne('SELECT @@character_set_server AS charset')->charset; + $serverDefaultCollation = DB::selectOne('SELECT @@collation_server AS collation')->collation; + + $databaseCharset = fn () => DB::selectOne('SELECT DEFAULT_CHARACTER_SET_NAME FROM information_schema.SCHEMATA WHERE SCHEMA_NAME = DATABASE()')->DEFAULT_CHARACTER_SET_NAME; + $databaseCollation = fn () => DB::selectOne('SELECT DEFAULT_COLLATION_NAME FROM information_schema.SCHEMATA WHERE SCHEMA_NAME = DATABASE()')->DEFAULT_COLLATION_NAME; + + $defaultTenant = Tenant::create(); + + tenancy()->initialize($defaultTenant); + + // No charset or collation specified, + // defaults from the MySQL config used. + expect($databaseCharset())->toBe('utf8mb4'); + expect($databaseCollation())->toBe('utf8mb4_unicode_ci'); + + $tenantWithCharsetAndCollation = Tenant::create([ + 'tenancy_db_charset' => 'latin1', + 'tenancy_db_collation' => 'latin1_swedish_ci', + ]); + + tenancy()->initialize($tenantWithCharsetAndCollation); + + // Custom charset and collation from tenant config + expect($databaseCharset())->toBe('latin1'); + expect($databaseCollation())->toBe('latin1_swedish_ci'); + + $tenantWithNullCharsetAndCollation = Tenant::create([ + 'tenancy_db_charset' => null, + 'tenancy_db_collation' => null, + ]); + + tenancy()->initialize($tenantWithNullCharsetAndCollation); + + // Default MySQL server charset and collation + // (e.g. charset = utf8mb4, collation = utf8mb4_0900_ai_ci) + expect($databaseCharset())->toBe($serverDefaultCharset); + expect($databaseCollation())->toBe($serverDefaultCollation); + + $tenantWithCharsetAndNullCollation = Tenant::create([ + 'tenancy_db_charset' => 'binary', + 'tenancy_db_collation' => null, + ]); + + tenancy()->initialize($tenantWithCharsetAndNullCollation); + + // Charset specified, collation is null, + // MySQL will choose a default collation for the specified charset. + expect($databaseCharset())->toBe('binary'); + expect($databaseCollation())->toBe('binary'); + + // Collation specified, charset is null, + // MySQL will choose a default charset for the specified collation. + $tenantWithCollationAndNullCharset = Tenant::create([ + 'tenancy_db_charset' => null, + 'tenancy_db_collation' => 'latin1_swedish_ci', + ]); + + tenancy()->initialize($tenantWithCollationAndNullCharset); + + expect($databaseCharset())->toBe('latin1'); + expect($databaseCollation())->toBe('latin1_swedish_ci'); +}); + // Datasets dataset('database_managers', [ ['mysql', MySQLDatabaseManager::class], From df4be2e060de9cd9c6c05e2dfbd803a6c86b027a Mon Sep 17 00:00:00 2001 From: lukinovec Date: Mon, 29 Jun 2026 04:04:02 +0200 Subject: [PATCH 4/6] migrate-fresh: show migration output when verbose (#1464) Resubmission of #1369 (by @lordofthebrain), changes adapted to v4. Also added a test (passes with the MigrateFresh changes, fails without them). --------- Co-authored-by: lordofthebrain Co-authored-by: Samuel Stancl --- src/Commands/MigrateFresh.php | 7 +++++-- tests/CommandsTest.php | 30 ++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/src/Commands/MigrateFresh.php b/src/Commands/MigrateFresh.php index d4733552..e53d6a89 100644 --- a/src/Commands/MigrateFresh.php +++ b/src/Commands/MigrateFresh.php @@ -14,6 +14,7 @@ use Stancl\Tenancy\Concerns\ParallelCommand; use Stancl\Tenancy\Database\Contracts\TenantWithDatabase; use Stancl\Tenancy\Database\Exceptions\TenantDatabaseDoesNotExistException; use Symfony\Component\Console\Input\InputOption; +use Symfony\Component\Console\Output\NullOutput; use Symfony\Component\Console\Output\OutputInterface as OI; class MigrateFresh extends BaseCommand @@ -72,11 +73,13 @@ class MigrateFresh extends BaseCommand protected function migrateTenant(TenantWithDatabase $tenant): bool { - return $this->callSilently('tenants:migrate', [ + $output = $this->getOutput()->isVerbose() ? $this->output : new NullOutput; + + return $this->runCommand('tenants:migrate', [ '--tenants' => [$tenant->getTenantKey()], '--step' => $this->option('step'), '--force' => true, - ]) === 0; + ], $output) === 0; } protected function childHandle(mixed ...$args): bool diff --git a/tests/CommandsTest.php b/tests/CommandsTest.php index bda3eea9..faa897bf 100644 --- a/tests/CommandsTest.php +++ b/tests/CommandsTest.php @@ -366,6 +366,36 @@ test('migrate fresh command works', function () { expect(DB::table('users')->exists())->toBeFalse(); }); +test('migrate fresh command only shows migration output when run with the verbose option', function () { + $tenant = Tenant::create(); + $migratingOutput = 'Migrating tenant ' . $tenant->getTenantKey(); + + // CI runs pest with --verbose which makes Artisan::call() inherit the verbosity + // so we cannot easily test commands without -v. To work around that, we temporarily + // override $_ENV['SHELL_VERBOSITY'] immediately before executing the command. If this + // ever stops working, try also overriding the value in $_SERVER and putenv(). + $emptySentinel = new \stdClass(); + $originalVerbosity = $_ENV['SHELL_VERBOSITY'] ?? $emptySentinel; + try { + $_ENV['SHELL_VERBOSITY'] = 0; + Artisan::call('tenants:migrate-fresh'); + $defaultOutput = Artisan::output(); + } finally { + if ($originalVerbosity === $emptySentinel) { + unset($_ENV['SHELL_VERBOSITY']); + } else { + $_ENV['SHELL_VERBOSITY'] = $originalVerbosity; + } + } + + Artisan::call('tenants:migrate-fresh -v'); + $verboseOutput = Artisan::output(); + + // The output is silent by default and only shown with the verbose option + expect($defaultOutput)->not()->toContain($migratingOutput); + expect($verboseOutput)->toContain($migratingOutput); +}); + test('migrate fresh command respects force option in production', function () { // Set environment to production app()->detectEnvironment(fn() => 'production'); From 17706f8e2c32859a359ba2a69c4f5c8fd2349df9 Mon Sep 17 00:00:00 2001 From: lukinovec Date: Wed, 1 Jul 2026 03:59:46 +0200 Subject: [PATCH 5/6] Correct DatabaseTenancyBootstrapperTest (#1466) ### Test for DatabaseTenancyBootstrapper throwing an exception when `DB_URL` is set The test meant to cover this set `central.url` before creating a tenant. That made the `CreateDatabase` job fail with a QueryException (it tried to use the URL host as a database name). The bootstrapper's check was never reached, and the exception that the test intended to cover wasn't ever thrown in the end. The test passed for the wrong reason. Fixed by creating the tenant first, setting the url only after that, and asserting that `tenancy()->initialize()` throws the bootstrapper's specific exception. ### Reference env variable that Laravel uses now (`DB_URL`) instead of `DATABASE_URL` Changed all `DATABASE_URL` references in Tenancy to `DB_URL`. [In Laravel 11](https://github.com/laravel/laravel/commit/96508d43ec#diff-e4382b565f69d02de16eef89c8d5ca2b60a679ffca90ab8b7d85fbd78f30075bR35), the `DATABASE_URL` env variable got renamed to `DB_URL` in `config/database.php`. The env var got renamed quite a while ago, so referencing `DATABASE_URL` in Tenancy's comments and tests was a bit confusing. ### Minor cleanup The `harden prevents tenants from using the database of another tenant` test now reads the central connection name from `config('tenancy.database.central_connection')` instead of hardcoding `'central'` for consistency with the other tests. --- .../DatabaseTenancyBootstrapper.php | 4 ++-- .../DatabaseTenancyBootstrapperTest.php | 24 +++++++++---------- tests/TestCase.php | 2 +- 3 files changed, 14 insertions(+), 16 deletions(-) diff --git a/src/Bootstrappers/DatabaseTenancyBootstrapper.php b/src/Bootstrappers/DatabaseTenancyBootstrapper.php index 08b36f1d..175d8b56 100644 --- a/src/Bootstrappers/DatabaseTenancyBootstrapper.php +++ b/src/Bootstrappers/DatabaseTenancyBootstrapper.php @@ -53,8 +53,8 @@ class DatabaseTenancyBootstrapper implements TenancyBootstrapper { /** @var TenantWithDatabase $tenant */ if (data_get($tenant->database()->getTemplateConnection(), 'url')) { - // The package works with individual parts of the database connection config, so DATABASE_URL is not supported. - // When DATABASE_URL is set, this bootstrapper can silently fail i.e. keep using the template connection's database URL + // The package works with individual parts of the database connection config, so DB_URL is not supported. + // When DB_URL is set, this bootstrapper can silently fail i.e. keep using the template connection's database URL // which takes precedence over individual segments of the connection config. This issue can be hard to debug as it can be // production-specific. Therefore, we throw an exception (that effectively blocks all tenant pages) to prevent incorrect DB use. throw new Exception('The template connection must NOT have URL defined. Specify the connection using individual parts instead of a database URL.'); diff --git a/tests/Bootstrappers/DatabaseTenancyBootstrapperTest.php b/tests/Bootstrappers/DatabaseTenancyBootstrapperTest.php index 63762672..4debea9e 100644 --- a/tests/Bootstrappers/DatabaseTenancyBootstrapperTest.php +++ b/tests/Bootstrappers/DatabaseTenancyBootstrapperTest.php @@ -12,7 +12,6 @@ use Stancl\Tenancy\Listeners\RevertToCentralContext; use Stancl\Tenancy\Tests\Etc\Tenant; use Illuminate\Support\Str; use Illuminate\Support\Facades\DB; -use Illuminate\Database\QueryException; use Stancl\Tenancy\Database\TenantDatabaseManagers\MySQLDatabaseManager; use Stancl\Tenancy\Database\TenantDatabaseManagers\SQLiteDatabaseManager; use Stancl\Tenancy\Database\TenantDatabaseManagers\PostgreSQLDatabaseManager; @@ -116,7 +115,9 @@ test('harden prevents tenants from using the database of another tenant', functi expect(fn () => tenancy()->initialize($tenant))->toThrow(RuntimeException::class); // Connection should be reverted back to central - expect(DB::connection()->getName())->toBe('central'); + $centralConnection = config('tenancy.database.central_connection'); + + expect(DB::connection()->getName())->toBe($centralConnection); } else { expect(fn() => tenancy()->initialize($tenant))->not()->toThrow(Throwable::class); @@ -128,25 +129,22 @@ test('harden prevents tenants from using the database of another tenant', functi 'hardening disabled' => false, ])->with('db_managers'); -test('database tenancy bootstrapper throws an exception if DATABASE_URL is set', function (string|null $databaseUrl) { - config(['database.connections.central.url' => $databaseUrl]); - +test('database tenancy bootstrapper throws an exception if DB_URL is set', function (string|null $databaseUrl) { config(['tenancy.bootstrappers' => [DatabaseTenancyBootstrapper::class]]); Event::listen(TenantCreated::class, JobPipeline::make([CreateDatabase::class])->send(function (TenantCreated $event) { return $event->tenant; })->toListener()); + $tenant = Tenant::create(); + + config(['database.connections.central.url' => $databaseUrl]); + if ($databaseUrl) { - expect(fn() => Tenant::create())->toThrow(QueryException::class); + expect(fn() => tenancy()->initialize($tenant)) + ->toThrow(Exception::class, 'The template connection must NOT have URL defined.'); } else { - expect(function() { - $tenant1 = Tenant::create(); - - pest()->artisan('tenants:migrate'); - - tenancy()->initialize($tenant1); - })->not()->toThrow(Throwable::class); + expect(fn() => tenancy()->initialize($tenant))->not()->toThrow(Throwable::class); } })->with(['abc.us-east-1.rds.amazonaws.com', null]); diff --git a/tests/TestCase.php b/tests/TestCase.php index cbc6f57e..eb58a0d9 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -131,7 +131,7 @@ abstract class TestCase extends \Orchestra\Testbench\TestCase 'cache.stores.apc' => ['driver' => 'apc'], 'database.connections.central' => [ 'driver' => 'mysql', - 'url' => env('DATABASE_URL'), + 'url' => env('DB_URL'), 'host' => 'mysql', 'port' => env('DB_PORT', '3306'), 'database' => 'main', From 3156c87ee3c3c26422db08dfc202182cdbb00a33 Mon Sep 17 00:00:00 2001 From: Samuel Stancl Date: Tue, 30 Jun 2026 19:30:50 -0700 Subject: [PATCH 6/6] phpstan fixes old ignore no longer necessary, two new ignores are needed (the added ignores are not necessary in PHP 8.5 but at this time our CI still runs on PHP 8.4) --- src/Features/DisallowSqliteAttach.php | 4 ++-- src/helpers.php | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/Features/DisallowSqliteAttach.php b/src/Features/DisallowSqliteAttach.php index 5cbfbf50..e5ae5c8f 100644 --- a/src/Features/DisallowSqliteAttach.php +++ b/src/Features/DisallowSqliteAttach.php @@ -74,8 +74,8 @@ class DisallowSqliteAttach implements Feature // @phpstan-ignore method.notFound $pdo->setAuthorizer(static function (int $action): int { return $action === 24 // SQLITE_ATTACH - ? PDO\Sqlite::DENY - : PDO\Sqlite::OK; + ? PDO\Sqlite::DENY // @phpstan-ignore classConstant.notFound + : PDO\Sqlite::OK; // @phpstan-ignore classConstant.notFound }); } } diff --git a/src/helpers.php b/src/helpers.php index 0b812e65..75514714 100644 --- a/src/helpers.php +++ b/src/helpers.php @@ -30,7 +30,6 @@ if (! function_exists('tenant')) { return app(Tenant::class); } - // @phpstan-ignore-next-line nullsafe.neverNull return app(Tenant::class)?->getAttribute($key); } }