[4.x] Parameter validation and other DB manager improvements#1459
Open
[4.x] Parameter validation and other DB manager improvements#1459
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.
Contributor
Author
|
@coderabbitai full review |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1459 +/- ##
============================================
+ Coverage 85.84% 86.27% +0.42%
- Complexity 1163 1184 +21
============================================
Files 185 186 +1
Lines 3399 3482 +83
============================================
+ Hits 2918 3004 +86
+ Misses 481 478 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a validation trait that enforces allowlisted characters for database identifiers and passwords; applies it across tenant DB managers and user-management concerns; replaces SQL interpolation with parameterized queries and safer heredocs; tightens SQLite name/in-memory checks and path handling; and adds an opt‑in bootstrapper hardening mode with tests. (34 words) ChangesValidation trait + manager updates
Bootstrapper hardening
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Bootstrapper
participant DBManager
participant Validator
participant TenantsStore
participant DB
Client->>Bootstrapper: initialize(tenant)
Bootstrapper->>DBManager: connectToTenant(tenant)
DBManager->>Validator: validateParameter(database, username, schema)
alt validation fails
Validator-->>DBManager: InvalidArgumentException
DBManager-->>Bootstrapper: propagate exception
Bootstrapper->>DB: revert()
Bootstrapper-->>Client: exception
else validation ok
Bootstrapper->>TenantsStore: find tenants by tenancy_db_name (if harden enabled)
TenantsStore-->>Bootstrapper: conflict / no conflict
alt conflict found
Bootstrapper->>DB: revert()
Bootstrapper-->>Client: RuntimeException
else no conflict
DBManager->>DB: execute parameterized SQL (CREATE/GRANT/DROP/SELECT)
DB-->>DBManager: result
DBManager-->>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 docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
Contributor
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
Contributor
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
Contributor
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
Contributor
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
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
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
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Bootstrappers/DatabaseTenancyBootstrapper.php (1)
54-62: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueException scope is narrow — consider catching broader exceptions.
The try-catch only handles
RuntimeException, so ifSchema::hasTable()or the tenant query throws aQueryException(e.g., database connection issue), the connection remains ontenantwithout reverting. Sinceharden()is a security feature, you may want to catch\Throwableto ensure the connection always reverts on any failure.♻️ Optional: broaden exception handling
if (static::$harden) { try { $this->harden($tenant); - } catch (RuntimeException $e) { + } catch (\Throwable $e) { // Revert connection back to central $this->revert(); throw $e; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Bootstrappers/DatabaseTenancyBootstrapper.php` around lines 54 - 62, The harden block currently only catches RuntimeException so failures like QueryException or other throwables (e.g., from Schema::hasTable() or the tenant query) will leave the connection switched to the tenant; update the catch to handle all throwables (catch \Throwable $e) around the harden($tenant) call and still call $this->revert() before re-throwing to guarantee the connection is reverted on any failure while keeping the existing revert() and re-throw behavior.
🤖 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/Bootstrappers/DatabaseTenancyBootstrapper.php`:
- Around line 54-62: The harden block currently only catches RuntimeException so
failures like QueryException or other throwables (e.g., from Schema::hasTable()
or the tenant query) will leave the connection switched to the tenant; update
the catch to handle all throwables (catch \Throwable $e) around the
harden($tenant) call and still call $this->revert() before re-throwing to
guarantee the connection is reverted on any failure while keeping the existing
revert() and re-throw behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3d8be12b-69c5-4198-a261-b4f8c65e2452
📒 Files selected for processing (2)
src/Bootstrappers/DatabaseTenancyBootstrapper.phptests/TenantDatabaseManagerTest.php
Cover non-string parameter validation and in-memory DB name validation
Contributor
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 `@tests/TenantDatabaseManagerTest.php`:
- Around line 709-712: The test currently hard-codes
'utf8mb4'/'utf8mb4_unicode_ci' for
expect(databaseCharset())/expect(databaseCollation()), which is brittle; instead
query the active DB connection for its defaults and assert against those values.
Replace the hard-coded expectations by retrieving the active connection's
default charset and collation (e.g. via a query for @@character_set_database and
@@collation_database on the current connection) and compare databaseCharset()
and databaseCollation() to those retrieved values so the test adapts to the
environment.
🪄 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: 11193c91-4672-489d-9add-df2260da5e7e
📒 Files selected for processing (1)
tests/TenantDatabaseManagerTest.php
Contributor
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 `@tests/TenantDatabaseManagerTest.php`:
- Line 556: The negated instanceof check is parsed incorrectly; change the
condition in TenantDatabaseManagerTest where it reads if (! $manager instanceof
ManagesDatabaseUsers) to explicitly negate the whole instanceof expression (e.g.
wrap the instanceof in parentheses) so the branch correctly executes only when
$manager is not an instance of ManagesDatabaseUsers; update the condition
referencing $manager and ManagesDatabaseUsers accordingly.
🪄 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: 44fa1cd6-f9ba-480e-8d6b-ecd2bd4f5526
📒 Files selected for processing (1)
tests/TenantDatabaseManagerTest.php
Contributor
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 `@tests/TenantDatabaseManagerTest.php`:
- Around line 632-646: Extend the test for SQLiteDatabaseManager to also
exercise createDatabase() and deleteDatabase() in addition to databaseExists():
call createDatabase('valid-db_name.sqlite') and
deleteDatabase('valid-db_name.sqlite') and assert they do not throw
InvalidArgumentException for the valid filename, and call createDatabase('..'),
deleteDatabase('..'), createDatabase(''), and deleteDatabase('') and assert they
throw InvalidArgumentException for the invalid inputs; reference the
SQLiteDatabaseManager class and its methods databaseExists, createDatabase, and
deleteDatabase when adding these assertions so the mutating paths are covered.
🪄 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: 3eb7f651-f899-4eee-9625-0ca636046907
📒 Files selected for processing (1)
tests/TenantDatabaseManagerTest.php
Contributor
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 `@tests/TenantDatabaseManagerTest.php`:
- Around line 543-584: The test mutates database.connections.mysql.charset to an
array for the MySQL sub-check and does not restore it, so capture the original
config value (config('database.connections.mysql.charset')) before setting it to
[] in the $driver === 'mysql' branch, run DB::purge('mysql') as you already do,
then restore the original value with config([...]) and call DB::purge('mysql')
again to ensure subsequent assertions or tests use the original charset;
reference the MySQL branch in the test (the anonymous test function, the $driver
=== 'mysql' block, and the DB::purge('mysql') call) to locate where to add the
save/restore.
🪄 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: 1a5b58c0-73cd-4645-bf75-070473e1a981
📒 Files selected for processing (1)
tests/TenantDatabaseManagerTest.php
Contributor
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 `@tests/TenantDatabaseManagerTest.php`:
- Around line 614-621: Add a positive test case that verifies the
password-specific allowlist path: create a Tenant via Tenant::make with a
tenancy_db_password containing a character that the identifier validator would
reject but validatePassword() permits (e.g., an at-sign '@' or colon ':'), then
call $manager->createUser($tenant->database()) and assert it does NOT throw
InvalidArgumentException; this ensures the relaxed password validation in
validatePassword() is exercised separately from the stricter DB/user name
validation.
🪄 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: 67c05f4f-9675-43ce-aa34-23bff7398a68
📒 Files selected for processing (1)
tests/TenantDatabaseManagerTest.php
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
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 `@tests/TenantDatabaseManagerTest.php`:
- Around line 624-631: The test reuses the literal username 'valid_USERNAME'
when calling $manager->createUser on the Tenant created via Tenant::make, which
can collide with existing users in concurrent/rerun environments; update the
test to use a unique per-test username (e.g., generate one with Str::random()
and assign it to 'tenancy_db_username' on the Tenant) or explicitly remove the
created DB user after the assertion, ensuring the call to
$manager->createUser($tenantWithValidPassword->database()) uses the unique
username to avoid pre-existing user errors.
🪄 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: d34b5777-7da1-4c59-b886-0598f5953a37
📒 Files selected for processing (1)
tests/TenantDatabaseManagerTest.php
lukinovec
commented
May 4, 2026
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tests/TenantDatabaseManagerTest.php`:
- Around line 624-631: The test creates a real DB role via
$manager->createUser($tenantWithValidPassword->database()) but never removes it;
update the test to remove the created user by calling the manager's deleteUser
(or equivalent) with the same database/username after the assertion—either
immediately after the expect(...) assertion or in a finally block / test
teardown (afterEach) to ensure cleanup even on failure; reference the createUser
call and the deleteUser method on the same manager instance to locate where to
add the cleanup.
🪄 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: f24c5619-434f-42a1-a39e-184e8fc00591
📒 Files selected for processing (1)
tests/TenantDatabaseManagerTest.php
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tests/TenantDatabaseManagerTest.php`:
- Around line 643-646: The comment in TenantDatabaseManagerTest referencing
validateParameter() and the
expect(...)->not()->toThrow(InvalidArgumentException::class) is misleading
because Pest will re-throw other exceptions (e.g., QueryException) causing the
test to fail; update the test to match intent: either change the assertion on
createUser($tenantWithNullDbParameters->database()) to expect the actual DB
exception (e.g., QueryException) or change the test to assert no exception by
preventing SQL execution (mocking/stubbing the DB call) or by making
createUser/validateParameter() explicitly throw InvalidArgumentException for
null params; reference validateParameter(), createUser(), and the
$tenantWithNullDbParameters setup when making the change.
🪄 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: 3108bf56-af2b-4e5b-949f-c0b3cd2480ce
📒 Files selected for processing (1)
tests/TenantDatabaseManagerTest.php
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/TenantDatabaseManagerTest.php (1)
639-642:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNull-parameter assertion is likely a latent test failure —
->not()->toThrow(InvalidArgumentException::class)re-throws any other exception.The inline comment explicitly says "an exception will be thrown, but not by
validateParameter()". However, Pest's->not()->toThrow(ExceptionClass)does not swallow non-matching exceptions — it re-throws them, causing the test to fail with that exception. If anyManagesDatabaseUsersbackend throws aQueryException(or any other non-InvalidArgumentException) whencreateUser()is called with anullusername, this test will fail with that propagated exception, not a clean assertion failure.Options to resolve:
- If the intent is to verify that
validateParameter()silently skipsnulland thatcreateUser()completes without any exception, use->not->toThrow()(no argument) to assert no exception at all; or- If
createUser()is expected to throw a different exception (e.g.,QueryException), expect it explicitly, or remove this assertion and rely on unit tests ofvalidateParameter()alone.🤖 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 `@tests/TenantDatabaseManagerTest.php` around lines 639 - 642, The current assertion expect(fn () => $manager->createUser($tenantWithNullDbParameters->database()))->not()->toThrow(InvalidArgumentException::class) is unsafe because Pest will re-throw any other exception (e.g., a backend QueryException) — fix by either 1) if you intended to assert that no exception at all is thrown, change the assertion to ->not()->toThrow() so any exception fails the test cleanly, or 2) if the backend is expected to throw a specific different exception (e.g., QueryException from a ManagesDatabaseUsers implementation), replace the assertion to expect(...)->toThrow(QueryException::class) to explicitly assert that outcome; locate this assertion near the validateParameter() / createUser() test and update it accordingly.
🤖 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.
Duplicate comments:
In `@tests/TenantDatabaseManagerTest.php`:
- Around line 639-642: The current assertion expect(fn () =>
$manager->createUser($tenantWithNullDbParameters->database()))->not()->toThrow(InvalidArgumentException::class)
is unsafe because Pest will re-throw any other exception (e.g., a backend
QueryException) — fix by either 1) if you intended to assert that no exception
at all is thrown, change the assertion to ->not()->toThrow() so any exception
fails the test cleanly, or 2) if the backend is expected to throw a specific
different exception (e.g., QueryException from a ManagesDatabaseUsers
implementation), replace the assertion to
expect(...)->toThrow(QueryException::class) to explicitly assert that outcome;
locate this assertion near the validateParameter() / createUser() test and
update it accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 38d1c8e5-503d-46b9-a622-b722e76408c0
📒 Files selected for processing (1)
tests/TenantDatabaseManagerTest.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.To-dos
createDatabase()(added in [4.x] Add charset support to PostgreSQLDatabaseManager #1429) - in the end, no, the PR will likely get closedSummary by CodeRabbit
New Features
Bug Fixes
Tests