-
-
Notifications
You must be signed in to change notification settings - Fork 487
[4.x] Parameter validation and other DB manager improvements #1459
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 11 commits
808f527
ad7d229
bdf592c
5adbc14
182f3a2
d5087d1
db03997
0fdb8b2
4a3e6ba
740d53e
8592949
f3f1ab9
75b74f2
322257f
46f73c4
4bdb877
50ea524
bacbf93
37a4c7d
2bd3a86
76c324d
d3607f8
e8168eb
9611a05
f3836cc
2bdda23
1a01164
665404e
fbd1e02
fc6a931
f5f5f1d
52f6857
0ce3d86
2ae1f79
b1f0d0a
7363318
48b4837
7683bef
e48d822
9a9adc0
7f93f44
7660ddd
26c161a
429e098
ea20eb1
405aaaf
2b3466f
338526d
fec170a
98a808b
6ed9975
de91348
bdbfbd4
e59195e
66ae88a
0331875
bbd8f6f
587f347
099a666
649c802
519c819
d9ae274
9055b61
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace Stancl\Tenancy\Database\Concerns; | ||
|
|
||
| use InvalidArgumentException; | ||
|
|
||
| /** | ||
| * Provides methods to validate database parameters (e.g. database names, usernames, passwords) | ||
| * before using them in SQL statements (or in file paths in the case of SQLiteDatabaseManager). | ||
| * | ||
| * Used where parameters can be provided by users, and where parameter binding isn't possible. | ||
| * | ||
| * @mixin \Stancl\Tenancy\Database\TenantDatabaseManagers\TenantDatabaseManager | ||
| * @mixin \Stancl\Tenancy\Database\TenantDatabaseManagers\SQLiteDatabaseManager | ||
| */ | ||
| trait ValidatesDatabaseParameters | ||
| { | ||
| /** | ||
| * Characters allowed in the parameters. | ||
| */ | ||
| protected static function parameterAllowlist(): string | ||
| { | ||
| return 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-'; | ||
| } | ||
|
|
||
| /** | ||
| * Characters allowed in database user passwords. | ||
| * | ||
| * Passwords are always quoted in the SQL statements, so it's safe | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are other parameters not always quoted? Does this refer to database names or which other parameters can we not quote? Besides the difference in quoting we might also want to mention just that we can be stricter with other params as they don't use that many characters so a stricter allowlist there is better. |
||
| * to allow a wider range of characters, as long as it doesn't include | ||
| * characters that can break out of the quoted SQL strings (so e.g. | ||
| * ', ", \, and ` aren't allowed). | ||
| */ | ||
| protected static function passwordAllowlist(): string | ||
| { | ||
| return ' !#$%&()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[]^_abcdefghijklmnopqrstuvwxyz{|}~'; | ||
| } | ||
|
|
||
| /** | ||
| * Ensure that parameters (database names, usernames, etc.) | ||
| * only contain allowed characters before used in SQL statements | ||
| * (or file names in the case of SQLiteDatabaseManager). | ||
| * | ||
| * By default, only the characters in static::parameterAllowlist() are allowed. | ||
| * | ||
| * @throws InvalidArgumentException | ||
| */ | ||
| protected function validateParameter(string|array|null $parameters, string|null $allowlist = null): string|array|null | ||
| { | ||
| if (is_null($parameters)) { | ||
| // Return null if there's nothing to validate | ||
| // (e.g. when $databaseConfig->getUsername() of an | ||
| // improperly created tenant is passed). | ||
| return null; | ||
| } | ||
|
|
||
| $allowlist = $allowlist ?? static::parameterAllowlist(); | ||
|
|
||
| foreach ((array) $parameters as $parameter) { | ||
| foreach (str_split($parameter) as $char) { | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| if (! str_contains($allowlist, $char)) { | ||
|
lukinovec marked this conversation as resolved.
Outdated
|
||
| throw new InvalidArgumentException("Invalid character '{$char}' in parameter: {$parameter}"); | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| } | ||
| } | ||
|
|
||
| return $parameters; | ||
| } | ||
|
|
||
| /** | ||
| * Ensure password only contains allowed characters before used in SQL statements. | ||
| * | ||
| * Used as a shorthand for calling validateParameter() with the less strict allowlist. | ||
| * | ||
| * @throws InvalidArgumentException | ||
| */ | ||
| protected function validatePassword(string|null $password): string|null | ||
| { | ||
| return $this->validateParameter($password, static::passwordAllowlist()); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the mixin annotations? We don't appear to be using methods from either of these classes in this trait?