[4.x] Parameter validation and other DB manager improvements#1459
[4.x] Parameter validation and other DB manager improvements#1459lukinovec wants to merge 76 commits into
Conversation
This is just for consistency, since all the other DB managers use select().
DB manager methods validate the parameters they use in SQL statements using validateParameter() (excluding parameters passed via bindings in SELECT statements).
WIP: password validation, SQLite (check if validation is enough for valid FS paths), revisit the test
Also, use parameterAllowlist() instead of the static property (so that we can e.g. override it later in SQLiteDatabaseManager, since overriding the static property doesn't work).
Also stop skipping the validation test for sqlite.
Also, make the validateParameter method ignore null parameters, e.g. for cases when tenants are created using Tenant::make() without tenancy_db_username set -- $databaseConfig->getUsername() allows null, same should go for the validate method whose only concern is checking strings for invalid characters.
|
@coderabbitai full review |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1459 +/- ##
============================================
+ Coverage 85.98% 86.45% +0.46%
- Complexity 1175 1200 +25
============================================
Files 185 186 +1
Lines 3425 3513 +88
============================================
+ Hits 2945 3037 +92
+ Misses 480 476 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a validation trait enforcing allowed characters and password rules; applies it across tenant DB managers and user concerns; replaces SQL interpolation with parameterized queries and safer heredocs; tightens SQLite name/in-memory/path validation; and adds an opt‑in bootstrapper harden mode with tests. ChangesValidation trait + manager updates
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Manager
participant Validator
participant TenantsStore
participant DB
participant Bootstrapper
Client->>Bootstrapper: initialize(tenant)
Bootstrapper->>Manager: connectToTenant(tenant)
Manager->>Validator: validateParameter(database, username, schema)
alt validation fails
Validator-->>Manager: InvalidArgumentException
Manager-->>Bootstrapper: throw
Bootstrapper->>DB: revert()
Bootstrapper-->>Client: exception
else validation ok
alt harden enabled
Bootstrapper->>TenantsStore: find tenants by tenancy_db_name
TenantsStore-->>Bootstrapper: conflict / no conflict
alt conflict
Bootstrapper->>DB: revert()
Bootstrapper-->>Client: RuntimeException
else
Manager->>DB: execute parameterized SQL (CREATE/GRANT/DROP/SELECT)
DB-->>Manager: result
Manager-->>Client: success
end
else
Manager->>DB: execute parameterized SQL (CREATE/GRANT/DROP/SELECT)
DB-->>Manager: result
Manager-->>Client: success
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Database/Concerns/ValidatesDatabaseParameters.php`:
- Around line 61-63: The loop inside validateParameter currently calls
str_split($parameter) without guarding against null array elements, causing a
TypeError when callers pass arrays containing null (e.g., results of
getUsername()/getPassword()); update validateParameter to skip any null (or
non-string) elements before calling str_split—check each $parameter with
is_string($parameter) (or continue when $parameter === null) so only strings are
iterated, and keep existing handling for top-level nulls intact.
In `@src/Database/TenantDatabaseManagers/MicrosoftSQLDatabaseManager.php`:
- Around line 13-23: The code assigns the union-typed return of
validateParameter() to $database and then interpolates it into SQL, causing
PHPStan type errors; instead, call validateParameter(...) for
side-effect/validation but keep $database as the original string from the tenant
and use that when calling connection()->statement in createDatabase and
deleteDatabase (i.e., do not reassign $database to the validateParameter()
result—validate in-place and use $tenant->database()->getName() stored as the
string $database before interpolation). Ensure you update both createDatabase
and deleteDatabase (and any other methods using validateParameter() assignment)
so connection()->statement("CREATE DATABASE [{$database}]") / statement("DROP
DATABASE [{$database}]") interpolate a known string variable.
In
`@src/Database/TenantDatabaseManagers/PermissionControlledPostgreSQLSchemaManager.php`:
- Around line 39-45: The primary-key lookup query in
PermissionControlledPostgreSQLSchemaManager is missing a schema filter and can
collide across schemas; update the SELECT against
information_schema.key_column_usage to include "table_schema = ?" and bind the
existing $schema variable alongside $tableName so the query uses both parameters
when obtaining $primaryKey (keep the query against
information_schema.key_column_usage and the use of
$this->connection()->selectOne intact).
In `@src/Database/TenantDatabaseManagers/SQLiteDatabaseManager.php`:
- Around line 95-96: databaseExists currently calls getPath($name) with raw
input, leaving path traversal/existence probing unvalidated; add a call to
$this->validateParameter($name) at the start of databaseExists() so the name is
validated before passing to getPath(), and similarly ensure any other methods
that call getPath() (e.g., createDatabase(), deleteDatabase()) perform
validateParameter() first or reuse a validated helper; update databaseExists()
to return after validation and then compute $path = $this->getPath($name).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c3c813de-1e9a-4645-a9a9-dea7a7490e2e
📒 Files selected for processing (13)
src/Database/Concerns/ManagesPostgresUsers.phpsrc/Database/Concerns/ValidatesDatabaseParameters.phpsrc/Database/TenantDatabaseManagers/MicrosoftSQLDatabaseManager.phpsrc/Database/TenantDatabaseManagers/MySQLDatabaseManager.phpsrc/Database/TenantDatabaseManagers/PermissionControlledMicrosoftSQLServerDatabaseManager.phpsrc/Database/TenantDatabaseManagers/PermissionControlledMySQLDatabaseManager.phpsrc/Database/TenantDatabaseManagers/PermissionControlledPostgreSQLDatabaseManager.phpsrc/Database/TenantDatabaseManagers/PermissionControlledPostgreSQLSchemaManager.phpsrc/Database/TenantDatabaseManagers/PostgreSQLDatabaseManager.phpsrc/Database/TenantDatabaseManagers/PostgreSQLSchemaManager.phpsrc/Database/TenantDatabaseManagers/SQLiteDatabaseManager.phpsrc/Database/TenantDatabaseManagers/TenantDatabaseManager.phptests/TenantDatabaseManagerTest.php
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Add validation so that a malicious tenant DB name can't be used to detect if a file exists.
Also cover that in-memory db names aren't validated in databaseExists
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Database/Concerns/ValidatesDatabaseParameters.php`:
- Around line 67-70: The current exception thrown in the character-validation
loop inside ValidatesDatabaseParameters (the foreach over str_split and the
InvalidArgumentException) exposes the full rejected parameter; change the
exception so it does not include the raw $parameter (used by
validatePassword()), e.g. log or throw a message that references only the
invalid character and a non-sensitive identifier (parameter name or masked
length) instead of the plaintext value — update the throw in the loop to include
the offending $char and either the parameter key/type or a masked representation
(e.g. "<redacted>" or asterisks by length) rather than {$parameter}.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c919e0dc-5dd6-40e4-be94-13cc6164718d
📒 Files selected for processing (10)
src/Database/Concerns/ManagesPostgresUsers.phpsrc/Database/Concerns/ValidatesDatabaseParameters.phpsrc/Database/TenantDatabaseManagers/MicrosoftSQLDatabaseManager.phpsrc/Database/TenantDatabaseManagers/MySQLDatabaseManager.phpsrc/Database/TenantDatabaseManagers/PermissionControlledMicrosoftSQLServerDatabaseManager.phpsrc/Database/TenantDatabaseManagers/PermissionControlledMySQLDatabaseManager.phpsrc/Database/TenantDatabaseManagers/PostgreSQLDatabaseManager.phpsrc/Database/TenantDatabaseManagers/PostgreSQLSchemaManager.phpsrc/Database/TenantDatabaseManagers/SQLiteDatabaseManager.phptests/TenantDatabaseManagerTest.php
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Database/Concerns/ValidatesDatabaseParameters.php`:
- Line 67: The loop using foreach (str_split($parameter) as $char) in
ValidatesDatabaseParameters (method that iterates parameters) doesn't guard
against non-string, non-null values and will raise a TypeError; before calling
str_split in that loop check that $parameter is a string (e.g.,
is_string($parameter)) and if not throw the same InvalidArgumentException used
elsewhere (or coerce to string if intended), so validation stays
deterministic—update the parameter iteration in the ValidatesDatabaseParameters
class to explicitly handle non-string values and only call str_split when
$parameter is a string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dbfa5fdd-7921-4ad8-a166-0200519f5cb3
📒 Files selected for processing (2)
src/Database/Concerns/ValidatesDatabaseParameters.phptests/TenantDatabaseManagerTest.php
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Database/Concerns/ValidatesDatabaseParameters.php (1)
59-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject non-string array entries instead of skipping them.
This silently bypasses validation for mixed inputs, so values like
getConfig()results that are not strings can still flow into SQL/file-path construction unvalidated. That undermines the hardening this trait is meant to provide.Proposed fix
foreach ((array) $parameters as $parameter) { - if (! is_string($parameter)) { - // Skip if there's nothing to validate - // (e.g. when $tenant->database()->getUsername() of an - // improperly created tenant is null and it gets passed). - continue; - } + if (! is_string($parameter)) { + throw new InvalidArgumentException('Database parameters must be strings or null.'); + } foreach (str_split($parameter) as $char) { if (! str_contains($allowlist, $char)) { throw new InvalidArgumentException("Forbidden character '{$char}' in database parameter."); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/Concerns/ValidatesDatabaseParameters.php` around lines 59 - 65, In the ValidatesDatabaseParameters trait, stop silently skipping non-string entries in the foreach over (array) $parameters; instead validate each $parameter and throw an appropriate exception (e.g., InvalidArgumentException) when a non-string is encountered so mixed-type inputs are rejected; update the loop that currently does "if (! is_string($parameter)) { continue; }" to throw with a clear message referencing the offending parameter/value and that $parameters must contain only strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Database/Concerns/ValidatesDatabaseParameters.php`:
- Around line 59-65: In the ValidatesDatabaseParameters trait, stop silently
skipping non-string entries in the foreach over (array) $parameters; instead
validate each $parameter and throw an appropriate exception (e.g.,
InvalidArgumentException) when a non-string is encountered so mixed-type inputs
are rejected; update the loop that currently does "if (! is_string($parameter))
{ continue; }" to throw with a clear message referencing the offending
parameter/value and that $parameters must contain only strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d0ae3e29-793b-4ca6-baaf-4f0625b2a076
📒 Files selected for processing (1)
src/Database/Concerns/ValidatesDatabaseParameters.php
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/TenantDatabaseManagers/PermissionControlledPostgreSQLDatabaseManager.php (1)
37-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winQuote the schema name in the GRANT statements.
validateParameter()still allows schema names that require quoting in PostgreSQL, but these statements emit{$schema}unquoted. That will fail for valid mixed-case or hyphenated schemas and can target the wrong object.Suggested fix
- $this->connection()->statement("GRANT USAGE, CREATE ON SCHEMA {$schema} 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}\""); + $this->connection()->statement("GRANT USAGE ON ALL SEQUENCES IN SCHEMA \"{$schema}\" TO \"{$username}\"");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Database/TenantDatabaseManagers/PermissionControlledPostgreSQLDatabaseManager.php` around lines 37 - 40, The GRANT statements use unquoted {$schema} which breaks for mixed-case or special-character schemas; update the two calls in PermissionControlledPostgreSQLDatabaseManager that invoke $this->connection()->statement("GRANT ... IN SCHEMA {$schema} ...") to properly quote the schema identifier (using PostgreSQL identifier quoting, e.g. wrap the schema name in double quotes and escape any internal quotes) so the SQL targets the correct schema when $schema contains mixed case or hyphens; ensure both the "GRANT USAGE, CREATE ON SCHEMA" and the "GRANT USAGE ON ALL SEQUENCES IN SCHEMA" statements are fixed and that any escaping logic for $schema is centralized or reused.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/Database/TenantDatabaseManagers/PermissionControlledPostgreSQLSchemaManager.php`:
- Around line 23-28: The GRANT CONNECT SQL emits the central database name
unquoted which breaks for valid PostgreSQL names; update the statement in
PermissionControlledPostgreSQLSchemaManager so the database identifier is a
properly quoted PostgreSQL identifier (escape any internal double quotes by
replacing " with "" and wrap the result in double quotes) before passing it into
$this->connection()->statement("GRANT CONNECT ON DATABASE ... TO
\"{$username}\""); keep using validateParameter([$username, $schema, $database])
but substitute the safely quoted database identifier when building the GRANT
string.
In `@src/Database/TenantDatabaseManagers/SQLiteDatabaseManager.php`:
- Around line 63-66: parameterAllowlist() currently permits '.' so inputs
containing '.' or '..' segments can bypass validateParameter() and enable path
traversal later in database_path()/getPath() used by createDatabase(),
deleteDatabase(), and databaseExists(); update the validation to explicitly
reject single-dot and double-dot path segments before joining/building the path
(either in validateParameter() or immediately where segments are split), i.e.,
after splitting the input on path separators ensure no segment equals '.' or
'..' and return/throw on detection, and add a regression test that asserts '..'
is rejected (and that attempts to create/delete a database with '..' fail).
---
Outside diff comments:
In
`@src/Database/TenantDatabaseManagers/PermissionControlledPostgreSQLDatabaseManager.php`:
- Around line 37-40: The GRANT statements use unquoted {$schema} which breaks
for mixed-case or special-character schemas; update the two calls in
PermissionControlledPostgreSQLDatabaseManager that invoke
$this->connection()->statement("GRANT ... IN SCHEMA {$schema} ...") to properly
quote the schema identifier (using PostgreSQL identifier quoting, e.g. wrap the
schema name in double quotes and escape any internal quotes) so the SQL targets
the correct schema when $schema contains mixed case or hyphens; ensure both the
"GRANT USAGE, CREATE ON SCHEMA" and the "GRANT USAGE ON ALL SEQUENCES IN SCHEMA"
statements are fixed and that any escaping logic for $schema is centralized or
reused.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5e212025-2a13-46cb-bea4-1917be98c853
📒 Files selected for processing (13)
src/Database/Concerns/ManagesPostgresUsers.phpsrc/Database/Concerns/ValidatesDatabaseParameters.phpsrc/Database/TenantDatabaseManagers/MicrosoftSQLDatabaseManager.phpsrc/Database/TenantDatabaseManagers/MySQLDatabaseManager.phpsrc/Database/TenantDatabaseManagers/PermissionControlledMicrosoftSQLServerDatabaseManager.phpsrc/Database/TenantDatabaseManagers/PermissionControlledMySQLDatabaseManager.phpsrc/Database/TenantDatabaseManagers/PermissionControlledPostgreSQLDatabaseManager.phpsrc/Database/TenantDatabaseManagers/PermissionControlledPostgreSQLSchemaManager.phpsrc/Database/TenantDatabaseManagers/PostgreSQLDatabaseManager.phpsrc/Database/TenantDatabaseManagers/PostgreSQLSchemaManager.phpsrc/Database/TenantDatabaseManagers/SQLiteDatabaseManager.phpsrc/Database/TenantDatabaseManagers/TenantDatabaseManager.phptests/TenantDatabaseManagerTest.php
| } | ||
|
|
||
| if (! is_string($parameter)) { | ||
| // E.g. if a parameter is retrieved from the config, it isn't necessarily a string |
There was a problem hiding this comment.
Any examples where this comes up? I wonder if there are cases where a parameter could be numeric and then we might want to cast it to a string. But I'm not sure about where this would come up.
There was a problem hiding this comment.
E.g. charset set to an array like ['utf8'] -- basically invalid user config
I wonder if there are cases where a parameter could be numeric and then we might want to cast it to a string
Sounds valid. Though every parameter we pass to validateParameter() is either string|null (e.g $databaseConfig->getUsername(), has return type string|null), or mixed (config value like charset/collation, which could be anything)
Adding to this: #1459 (comment) -- if e.g. username is null, and there's any attempt to use it, it sounds like a good idea to throw an exception becuase it's just invalid. But there might be some cases where null could mean "default", which could be completely valid? I guess this is worth looking into
| { | ||
| $allowedCharacters ??= $this->allowedParameterCharacters(); | ||
|
|
||
| foreach ((array) $parameters as $parameter) { |
There was a problem hiding this comment.
Not sure if (array) always works well for strings or perhaps the null case I commented on in another review. (array) null returns [] I think so you'd never get the other case I reviewed in the first place.
It's standard to use Arr::wrap() in Laravel code.
There was a problem hiding this comment.
I think (array) and Arr:wrap would be equivalent in this case, but yeah not sure why I did not just use Arr::wrap() here as usual. I'll just use that
| } | ||
| } else { | ||
| $baseConfig['database'] = database_path($databaseName); | ||
| $baseConfig['database'] = $this->getPath($databaseName); |
There was a problem hiding this comment.
Seems like the old code using database_path() instead of getPath() was either overlooked or intentional. Could this change have any impact that'd change behavior?
There was a problem hiding this comment.
Yeah, this was an overlooked bug. See the test added here: 26c161a
Changing $this->getPath back to database_path makes it fail.
| { | ||
| if ($this->isInMemory($databaseName)) { | ||
| // Named in-memory DBs are formatted like 'file:_tenancy_inmemory_tenant123?mode=memory&cache=shared' | ||
| $this->validateDatabaseName($databaseName, ':?=&'); |
There was a problem hiding this comment.
I'd probably use a named param for the second arg here since it's the only place where the second param allowlist is extra characters.
There was a problem hiding this comment.
Makes sense, that'd be clearer
| 'tenancy.bootstrappers' => [DatabaseTenancyBootstrapper::class], | ||
| ]); | ||
|
|
||
| DatabaseTenancyBootstrapper::$harden = true; |
There was a problem hiding this comment.
I think I'd use a dataset for this setting so the test more clearly asserts it's actually $harden doing its job here. Same thing in the following test.
| if ($databaseUrl) { | ||
| pest()->expectException(Exception::class); | ||
| } |
There was a problem hiding this comment.
Can we make this expectation more targeted to a specific line here? As in expect x to throw.
There was a problem hiding this comment.
Made the assertions more targeted here: 49356a5
| expect($manager->connection()->getConfig('url'))->toBeNull(); | ||
| }); | ||
|
|
||
| test('database managers validate parameters that cannot be bound', function ($driver, $databaseManager) { |
There was a problem hiding this comment.
Agree, thought about it for quite a while but couldn't come up with anything better. Maybe something more generic, like "database managers validate parameters before using them in queries"? But that may be too broad/inaccurate..
| // 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/'; |
There was a problem hiding this comment.
This static prop is reset in beforeEach but not afterEach I think
There was a problem hiding this comment.
Right, added afterEach with the reset.
| // fail before we even get to actual deleteDatabase() logic. | ||
| $tenant = Tenant::make([ | ||
| 'tenancy_db_name' => $invalidDatabaseName, | ||
| 'tenancy_db_username' => 'valid-username', |
There was a problem hiding this comment.
Why set the username if this is guarded by ! ($manager instanceof ManagesDatabaseUsers)?
There was a problem hiding this comment.
Probably a leftover. Removing the username changes nothing, so it's redundant to set it. Fixed (d28abde).
| if ($driver === 'mysql') { | ||
| // MySQLDatabaseManager reads charset/collation from config. | ||
| // An exception is thrown during validation if the parameter is not a string. | ||
| config(['database.connections.mysql.charset' => []]); | ||
| DB::purge('mysql'); | ||
|
|
||
| $tenantWithNonStringCharset = Tenant::make([ | ||
| 'tenancy_db_name' => 'valid_db_name', | ||
| ]); | ||
|
|
||
| expect(fn () => $manager->createDatabase($tenantWithNonStringCharset)) | ||
| ->toThrow(InvalidArgumentException::class, 'Parameter has to be a string.'); | ||
|
|
||
| // Restore the default charset | ||
| config(['database.connections.mysql.charset' => 'utf8mb4']); | ||
| DB::purge('mysql'); | ||
| } |
There was a problem hiding this comment.
Wouldn't it make more sense to move mysql charset assertions to a separate test that just uses MySQL?
There was a problem hiding this comment.
Yeah that would make the test cleaner (36782eb)
| // validateParameter() doesn't throw InvalidArgumentException if a parameter is null | ||
| // (an exception will be thrown, but not by validateParameter()). | ||
| expect(fn () => $manager->createUser($tenantWithNullDbParameters->database())) | ||
| ->not()->toThrow(InvalidArgumentException::class); |
There was a problem hiding this comment.
Just adding a note to revisit this along with the other review about null values
| } | ||
|
|
||
| // Check if the current database doesn't have the tenants table (i.e. it's not the central database) | ||
| if (Schema::hasTable($tenant->getTable())) { |
There was a problem hiding this comment.
Wondering if this is the right way to assert this or if we could check that we're on the central connection in another way. Maybe if the config changes by connecting to a tenant that'd be a bit harder? Worth revisiting though
There was a problem hiding this comment.
Pull request overview
This PR hardens tenant database management by introducing centralized allowlist validation for database parameters, expanding parameter binding where feasible, and adding an opt-in “harden” mode in DatabaseTenancyBootstrapper to prevent tenants from connecting to the central DB or another tenant’s DB. It also fixes a SQLite connection config bug (respecting the configured path) and adds/renames tests to ensure coverage and correct execution.
Changes:
- Add
ValidatesDatabaseParametersand apply validation across database managers (plus parameter binding for existence checks where possible). - Add
DatabaseTenancyBootstrapper::$hardenwith tests, and fix the previously ignored bootstrapper test filename. - Fix SQLite path handling in
makeConnectionConfig()and tighten in-memory DB detection, with regression tests.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/TenantDatabaseManagerTest.php | Adds coverage for validation behavior, SQLite path/in-memory handling, and MySQL charset/collation cases. |
| tests/Bootstrappers/DatabaseTenancyBootstrapperTest.php | New/renamed test file to ensure bootstrapper tests run; adds harden behavior tests. |
| tests/Bootstrappers/DatabaseTenancyBootstrapper.php | Removes old bootstrapper test file that lacked the Test suffix. |
| src/Database/TenantDatabaseManagers/TenantDatabaseManager.php | Mixes in centralized DB parameter validation trait for managers inheriting from this base. |
| src/Database/TenantDatabaseManagers/SQLiteDatabaseManager.php | Adds SQLite DB-name validation, fixes path usage, and tightens in-memory detection. |
| src/Database/TenantDatabaseManagers/PostgreSQLSchemaManager.php | Adds validation for unbindable identifiers; parameter-binds schema existence check. |
| src/Database/TenantDatabaseManagers/PostgreSQLDatabaseManager.php | Adds validation for unbindable identifiers; parameter-binds database existence check. |
| src/Database/TenantDatabaseManagers/PermissionControlledPostgreSQLSchemaManager.php | Validates identifier parameters and binds table listing/PK lookup queries. |
| src/Database/TenantDatabaseManagers/PermissionControlledPostgreSQLDatabaseManager.php | Validates identifier parameters and quotes schema identifiers in GRANTs. |
| src/Database/TenantDatabaseManagers/PermissionControlledMySQLDatabaseManager.php | Validates username/db and password; parameter-binds user existence check. |
| src/Database/TenantDatabaseManagers/PermissionControlledMicrosoftSQLServerDatabaseManager.php | Validates username/db and password; parameter-binds user existence check; validates db name before altering. |
| src/Database/TenantDatabaseManagers/MySQLDatabaseManager.php | Validates inputs and conditionally appends charset/collation to CREATE DATABASE. |
| src/Database/TenantDatabaseManagers/MicrosoftSQLDatabaseManager.php | Validates identifiers and parameter-binds database existence check. |
| src/Database/Concerns/ValidatesDatabaseParameters.php | Introduces reusable allowlist-based validation for identifiers and passwords. |
| src/Database/Concerns/ManagesPostgresUsers.php | Adds validation and parameter-binds Postgres user existence query. |
| src/Bootstrappers/DatabaseTenancyBootstrapper.php | Adds opt-in harden mode and ensures reverting to central on harden failure. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $dbName = DB::getDatabaseName(); | ||
|
|
||
| // Check if any other tenant uses this tenant's database | ||
| if ($tenant::where($tenant->getTenantKeyName(), '!=', $tenant->getTenantKey()) | ||
| ->where('data->tenancy_db_name', $dbName) | ||
| ->exists()) { | ||
| throw new RuntimeException('Tenant cannot use a database of another tenant.'); | ||
| } |
There was a problem hiding this comment.
This is similar to what I suggested in my review about using internalPrefix() and getCustomColumns() but I'm not sure if this addresses the SQLite concern (which sounds reasonable?)
There was a problem hiding this comment.
I think the SQLite concern is worth looking into, will confirm if it's valid to use the suggested approach instead of the current one.
| // Check if the current database doesn't have the tenants table (i.e. it's not the central database) | ||
| if (Schema::hasTable($tenant->getTable())) { | ||
| throw new RuntimeException('Tenant cannot use the central database.'); |
There was a problem hiding this comment.
Definitely, but won't commit or resolve this until #1459 (comment) is resolved
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Bootstrappers/DatabaseTenancyBootstrapper.php (1)
77-85:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
db_name-only hardening will break schema-based PostgreSQL tenants.This method assumes one database per tenant, but this package also ships
PostgreSQLSchemaManagerandPermissionControlledPostgreSQLSchemaManager, where multiple tenants intentionally share the same database and are isolated by schema. With$harden = true, the second tenant on that shared database will always satisfy thisexists()query and get blocked even though the setup is valid. Make the verification manager-aware here: only enforcedb_nameuniqueness for database-per-tenant managers, and validate schema identity for schema-based managers instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bootstrappers/DatabaseTenancyBootstrapper.php` around lines 77 - 85, The verifyTenantCanUseDatabase method currently enforces db_name uniqueness for all tenancy managers; change it to be manager-aware: detect whether the active tenancy manager is a database-per-tenant manager or a schema-based manager (e.g. PostgreSQLSchemaManager / PermissionControlledPostgreSQLSchemaManager or a SchemaManagerInterface) and branch accordingly—if database-per-tenant, keep the existing db_name uniqueness query using $tenant::where(...)->where(... 'db_name' ...)->exists(); if schema-based, instead check uniqueness of the tenant's schema identifier (the schema column under $tenant::getDataColumn() . '->' . $tenant->internalPrefix() . 'schema' or the appropriate schema key) within the same DB, or otherwise use the manager's provided API to validate schema identity; ensure you reference the verifyTenantCanUseDatabase method and the tenancy manager classes/interfaces when implementing the conditional logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/Bootstrappers/DatabaseTenancyBootstrapper.php`:
- Around line 77-85: The verifyTenantCanUseDatabase method currently enforces
db_name uniqueness for all tenancy managers; change it to be manager-aware:
detect whether the active tenancy manager is a database-per-tenant manager or a
schema-based manager (e.g. PostgreSQLSchemaManager /
PermissionControlledPostgreSQLSchemaManager or a SchemaManagerInterface) and
branch accordingly—if database-per-tenant, keep the existing db_name uniqueness
query using $tenant::where(...)->where(... 'db_name' ...)->exists(); if
schema-based, instead check uniqueness of the tenant's schema identifier (the
schema column under $tenant::getDataColumn() . '->' . $tenant->internalPrefix()
. 'schema' or the appropriate schema key) within the same DB, or otherwise use
the manager's provided API to validate schema identity; ensure you reference the
verifyTenantCanUseDatabase method and the tenancy manager classes/interfaces
when implementing the conditional logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7727ca4e-e937-4fbb-b51f-c739fb0bd72a
📒 Files selected for processing (1)
src/Bootstrappers/DatabaseTenancyBootstrapper.php
Co-authored-by: Samuel Stancl <samuel@archte.ch>
Parameter validation
In
statement()calls ofTenantDatabaseManagers, use parameter binding when possible. If that's not possible, validate the parameters usingvalidateParameter()orvalidatePassword().Passwords use a less strict allowlist than other parameters (e.g. DB names), since passwords are always quoted in the SQL statements (and since passwords tend to use special characters).
In
SQLiteDatabaseManager, names of file-based databases are validated (increateDatabase,deleteDatabaseanddatabaseExists) 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::$hardenproperty -- setting it to true prevents tenants from connecting to the wrong databases (RuntimeExceptionis thrown after connecting to the wrong database).Also, the DatabaseTenancyBootstrapper test file was ignored while running tests because it lacked the
Testsuffix. Added the suffix and fixed the brokenDATABASE_URLtest in the file.SQLiteDatabaseManager - respect static $path property in makeConnectionConfig()
SQLiteDatabaseManager had a bug in
makeConnectionConfig: the method didn't respect the static$pathproperty, it useddatabase_path()instead. Added a regression test for that. Also recognizing in-memory SQLite databases (usingisInMemory()) is more strict now so that simply having a db_name with_tenancy_inmemory_anywhere doesn't e.g. make a file-based database considered in-memory.MySQLDatabaseManager - charset and collation defaulting
Creating databases with
nullcharsets and collations resulted in aQueryException, since null isn't a valid charset/collation. To solve that, in theCREATE DATABASEstatement 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 DATABASEstatement 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.Summary by CodeRabbit
New Features
Bug Fixes
Tests