[4.x] Validate DB parameters before using them in SQL statements or file paths#1459
Draft
[4.x] Validate DB parameters before using them in SQL statements or file paths#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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1459 +/- ##
============================================
+ Coverage 85.84% 86.05% +0.20%
- Complexity 1163 1172 +9
============================================
Files 185 186 +1
Lines 3399 3434 +35
============================================
+ Hits 2918 2955 +37
+ Misses 481 479 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
📝 WalkthroughWalkthroughThe PR introduces input validation across multiple database managers by adding a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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
Comment on lines
+61
to
+63
| foreach ((array) $parameters as $parameter) { | ||
| foreach (str_split($parameter) as $char) { | ||
| if (! str_contains($allowlist, $char)) { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify call sites that pass arrays to validateParameter and may include nullable values.
# Expected: identify array-based calls using getUsername()/getConfig() etc., which can be null.
set -euo pipefail
echo "Array-based validateParameter call sites:"
rg -nP 'validateParameter\(\s*\[' src
echo
echo "Potential nullable sources used in DB manager parameters:"
rg -nP 'getUsername\(\)|getPassword\(\)|getConfig\(' src/Database/TenantDatabaseManagers src/Database/ConcernsRepository: archtechx/tenancy
Length of output: 3645
🏁 Script executed:
# View the full ValidatesDatabaseParameters.php file to understand the method structure
fd -name "ValidatesDatabaseParameters.php" srcRepository: archtechx/tenancy
Length of output: 292
🏁 Script executed:
# Get the actual content of the file
cat src/Database/Concerns/ValidatesDatabaseParameters.phpRepository: archtechx/tenancy
Length of output: 2963
🏁 Script executed:
# Check return types of getUsername(), getPassword(), getConfig()
rg -A 5 "function (getUsername|getPassword|getConfig)" src/Database --type phpRepository: archtechx/tenancy
Length of output: 649
validateParameter() throws TypeError when array elements are null.
While top-level null is correctly handled on line 51-56, null values inside an input array are not checked before reaching str_split() on line 62. Since getUsername() and getPassword() both return ?string, calls like validateParameter([$database, $username]) will crash with a TypeError when those methods return null.
🐛 Proposed fix
foreach ((array) $parameters as $parameter) {
+ if (is_null($parameter)) {
+ continue;
+ }
+
foreach (str_split($parameter) as $char) {
if (! str_contains($allowlist, $char)) {
throw new InvalidArgumentException("Invalid character '{$char}' in parameter: {$parameter}");
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| foreach ((array) $parameters as $parameter) { | |
| foreach (str_split($parameter) as $char) { | |
| if (! str_contains($allowlist, $char)) { | |
| foreach ((array) $parameters as $parameter) { | |
| if (is_null($parameter)) { | |
| continue; | |
| } | |
| foreach (str_split($parameter) as $char) { | |
| if (! str_contains($allowlist, $char)) { | |
| throw new InvalidArgumentException("Invalid character '{$char}' in parameter: {$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 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.
Comment on lines
+13
to
23
| $database = $this->validateParameter($tenant->database()->getName()); | ||
|
|
||
| return $this->connection()->statement("CREATE DATABASE [{$database}]"); | ||
| } | ||
|
|
||
| public function deleteDatabase(TenantWithDatabase $tenant): bool | ||
| { | ||
| return $this->connection()->statement("DROP DATABASE [{$tenant->database()->getName()}]"); | ||
| $database = $this->validateParameter($tenant->database()->getName()); | ||
|
|
||
| return $this->connection()->statement("DROP DATABASE [{$database}]"); | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t assign validateParameter() return value to interpolated SQL identifier vars.
Line 13 and Line 20 capture a union-typed return (array|string|null), then Line 15 and Line 22 interpolate it as string. PHPStan already flags this. Validate in-place, but keep $database as the original string variable.
Suggested fix
public function createDatabase(TenantWithDatabase $tenant): bool
{
- $database = $this->validateParameter($tenant->database()->getName());
+ $database = $tenant->database()->getName();
+ $this->validateParameter($database);
return $this->connection()->statement("CREATE DATABASE [{$database}]");
}
public function deleteDatabase(TenantWithDatabase $tenant): bool
{
- $database = $this->validateParameter($tenant->database()->getName());
+ $database = $tenant->database()->getName();
+ $this->validateParameter($database);
return $this->connection()->statement("DROP DATABASE [{$database}]");
}Use this to verify all similar occurrences:
#!/bin/bash
set -euo pipefail
# 1) Find assignments from validateParameter()
rg -nP '\$\w+\s*=\s*\$this->validateParameter\(' src/Database -C2
# 2) Find interpolated statement() SQL strings
rg -nP '->statement\(".*\{\$\w+\}.*"\)' src/Database -C1🧰 Tools
🪛 GitHub Check: Static analysis (PHPStan)
[failure] 22-22:
Part $database (array|string|null) of encapsed string cannot be cast to string.
[failure] 15-15:
Part $database (array|string|null) of encapsed string cannot be cast to string.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Database/TenantDatabaseManagers/MicrosoftSQLDatabaseManager.php` around
lines 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.
Comment on lines
+39
to
45
| $primaryKey = $this->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; | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the PK metadata query and confirm schema predicate presence.
fd PermissionControlledPostgreSQLSchemaManager.php --exec sed -n '34,48p' {}Repository: archtechx/tenancy
Length of output: 726
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the full method containing the code snippet to confirm $schema is in scope
fd PermissionControlledPostgreSQLSchemaManager.php --exec sed -n '25,50p' {}Repository: archtechx/tenancy
Length of output: 1459
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for other information_schema queries in this file that might have similar issues
rg "information_schema" src/Database/TenantDatabaseManagers/PermissionControlledPostgreSQLSchemaManager.php -A 5 -B 2Repository: archtechx/tenancy
Length of output: 855
Primary-key lookup must include schema to avoid cross-schema collisions.
Line 42 filters only by table_name. In a multi-schema PostgreSQL environment, the information_schema.key_column_usage query can return the wrong primary key metadata if another schema contains a table with the same name. The method already correctly filters by table_schema when retrieving table names (line 31), but the primary key lookup is missing this critical filter. Add table_schema = ? and bind $schema.
Suggested fix
$primaryKey = $this->connection()->selectOne(<<<'SQL'
SELECT column_name
FROM information_schema.key_column_usage
- WHERE table_name = ?
+ WHERE table_schema = ?
+ AND table_name = ?
AND constraint_name LIKE '%_pkey'
-SQL, [$tableName])->column_name;
+SQL, [$schema, $tableName])->column_name;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/Database/TenantDatabaseManagers/PermissionControlledPostgreSQLSchemaManager.php`
around lines 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).
Comment on lines
+95
to
+96
| $this->validateParameter($name); | ||
|
|
Contributor
There was a problem hiding this comment.
Validation is incomplete: databaseExists() still uses unvalidated file-path input.
Lines 95 and 112 validate create/delete inputs, but Line 131 still passes raw $name to getPath(). That leaves path traversal-style existence probing unguarded. Apply the same validation in databaseExists().
Suggested fix
public function databaseExists(string $name): bool
{
- return $this->isInMemory($name) || file_exists($this->getPath($name));
+ if ($this->isInMemory($name)) {
+ return true;
+ }
+
+ $this->validateParameter($name);
+
+ return file_exists($this->getPath($name));
}Also applies to: 112-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Database/TenantDatabaseManagers/SQLiteDatabaseManager.php` around lines
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).
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.
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 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).To-dos
createDatabase()(added in [4.x] Add charset support to PostgreSQLDatabaseManager #1429)Summary by CodeRabbit
Bug Fixes
Tests