From 7f7feb8604e8bbd0b94b325524b29f50cef99b78 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Thu, 23 Jun 2022 13:57:08 +1200 Subject: [PATCH 1/6] ENH Refactor Backtrace to be a bit more readable. --- src/Dev/Backtrace.php | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/Dev/Backtrace.php b/src/Dev/Backtrace.php index 9139534f4..2bfbe73e6 100644 --- a/src/Dev/Backtrace.php +++ b/src/Dev/Backtrace.php @@ -13,11 +13,12 @@ class Backtrace use Configurable; /** - * @var array Replaces all arguments with a '' string, + * Replaces all arguments with a '' string, * mostly for security reasons. Use string values for global functions, * and array notation for class methods. * PHP's debug_backtrace() doesn't allow to inspect the argument names, * so all arguments of the provided functions will be filtered out. + * @var array */ private static $ignore_function_args = [ 'mysql_connect', @@ -107,22 +108,22 @@ class Backtrace // Filter out arguments foreach ($bt as $i => $frame) { $match = false; - if (!empty($bt[$i]['class'])) { + if (!empty($frame['class'])) { foreach ($ignoredArgs as $fnSpec) { if (is_array($fnSpec) && - ('*' == $fnSpec[0] || $bt[$i]['class'] == $fnSpec[0]) && - $bt[$i]['function'] == $fnSpec[1] + ('*' == $fnSpec[0] || $frame['class'] == $fnSpec[0]) && + $frame['function'] == $fnSpec[1] ) { $match = true; } } } else { - if (in_array($bt[$i]['function'], $ignoredArgs ?? [])) { + if (in_array($frame['function'], $ignoredArgs ?? [])) { $match = true; } } if ($match) { - foreach ($bt[$i]['args'] as $j => $arg) { + foreach ($frame['args'] as $j => $arg) { $bt[$i]['args'][$j] = ''; } } From 268a66418bffe032788b1fe608d081644e952675 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Thu, 23 Jun 2022 14:24:23 +1200 Subject: [PATCH 2/6] ENH Move backtrace ignored functions into yml config. Each module that adds its own methods will do it via yml. This keeps framework consistent with the others. --- _config/backtrace.yml | 33 +++++++++++++++++++++++++++++++++ src/Dev/Backtrace.php | 31 +------------------------------ 2 files changed, 34 insertions(+), 30 deletions(-) create mode 100644 _config/backtrace.yml diff --git a/_config/backtrace.yml b/_config/backtrace.yml new file mode 100644 index 000000000..13c83889e --- /dev/null +++ b/_config/backtrace.yml @@ -0,0 +1,33 @@ +--- +Name: framework-backtrace +--- +SilverStripe\Dev\Backtrace: + ignore_function_args: + - 'mysql_connect' + - 'mssql_connect' + - 'pg_connect' + - ['PDO', '__construct'] + - ['mysqli', 'mysqli'] + - ['mysqli', 'select_db'] + - ['mysqli', 'real_connect'] + - ['SilverStripe\\ORM\\DB', 'connect'] + - ['SilverStripe\\Security\\Security', 'check_default_admin'] + - ['SilverStripe\\Security\\Security', 'encrypt_password'] + - ['SilverStripe\\Security\\Security', 'setDefaultAdmin'] + - ['SilverStripe\\ORM\\DB', 'createDatabase'] + - ['SilverStripe\\Security\\Member', 'checkPassword'] + - ['SilverStripe\\Security\\Member', 'changePassword'] + - ['SilverStripe\\Security\\MemberAuthenticator\\MemberAuthenticator', 'checkPassword'] + - ['SilverStripe\\Security\\MemberPassword', 'checkPassword'] + - ['SilverStripe\\Security\\PasswordValidator', 'validate'] + - ['SilverStripe\\Security\\PasswordEncryptor_PHPHash', 'encrypt'] + - ['SilverStripe\\Security\\PasswordEncryptor_PHPHash', 'salt'] + - ['SilverStripe\\Security\\PasswordEncryptor_LegacyPHPHash', 'encrypt'] + - ['SilverStripe\\Security\\PasswordEncryptor_LegacyPHPHash', 'salt'] + - ['SilverStripe\\Security\\PasswordEncryptor_MySQLPassword', 'encrypt'] + - ['SilverStripe\\Security\\PasswordEncryptor_MySQLPassword', 'salt'] + - ['SilverStripe\\Security\\PasswordEncryptor_MySQLOldPassword', 'encrypt'] + - ['SilverStripe\\Security\\PasswordEncryptor_MySQLOldPassword', 'salt'] + - ['SilverStripe\\Security\\PasswordEncryptor_Blowfish', 'encrypt'] + - ['SilverStripe\\Security\\PasswordEncryptor_Blowfish', 'salt'] + - ['*', 'updateValidatePassword'], diff --git a/src/Dev/Backtrace.php b/src/Dev/Backtrace.php index 2bfbe73e6..efd962301 100644 --- a/src/Dev/Backtrace.php +++ b/src/Dev/Backtrace.php @@ -20,36 +20,7 @@ class Backtrace * so all arguments of the provided functions will be filtered out. * @var array */ - private static $ignore_function_args = [ - 'mysql_connect', - 'mssql_connect', - 'pg_connect', - ['PDO', '__construct'], - ['mysqli', 'mysqli'], - ['mysqli', 'select_db'], - ['mysqli', 'real_connect'], - ['SilverStripe\\ORM\\DB', 'connect'], - ['SilverStripe\\Security\\Security', 'check_default_admin'], - ['SilverStripe\\Security\\Security', 'encrypt_password'], - ['SilverStripe\\Security\\Security', 'setDefaultAdmin'], - ['SilverStripe\\ORM\\DB', 'createDatabase'], - ['SilverStripe\\Security\\Member', 'checkPassword'], - ['SilverStripe\\Security\\Member', 'changePassword'], - ['SilverStripe\\Security\\MemberAuthenticator\\MemberAuthenticator', 'checkPassword'], - ['SilverStripe\\Security\\MemberPassword', 'checkPassword'], - ['SilverStripe\\Security\\PasswordValidator', 'validate'], - ['SilverStripe\\Security\\PasswordEncryptor_PHPHash', 'encrypt'], - ['SilverStripe\\Security\\PasswordEncryptor_PHPHash', 'salt'], - ['SilverStripe\\Security\\PasswordEncryptor_LegacyPHPHash', 'encrypt'], - ['SilverStripe\\Security\\PasswordEncryptor_LegacyPHPHash', 'salt'], - ['SilverStripe\\Security\\PasswordEncryptor_MySQLPassword', 'encrypt'], - ['SilverStripe\\Security\\PasswordEncryptor_MySQLPassword', 'salt'], - ['SilverStripe\\Security\\PasswordEncryptor_MySQLOldPassword', 'encrypt'], - ['SilverStripe\\Security\\PasswordEncryptor_MySQLOldPassword', 'salt'], - ['SilverStripe\\Security\\PasswordEncryptor_Blowfish', 'encrypt'], - ['SilverStripe\\Security\\PasswordEncryptor_Blowfish', 'salt'], - ['*', 'updateValidatePassword'], - ]; + private static $ignore_function_args = []; /** * Return debug_backtrace() results with functions filtered From 2b0df58176d4a5a553996223f730bf30f4e996be Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Thu, 23 Jun 2022 15:16:46 +1200 Subject: [PATCH 3/6] ENH Minor performance enhancement for backtrace. Don't keep processing when we've found a match. --- src/Dev/Backtrace.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Dev/Backtrace.php b/src/Dev/Backtrace.php index efd962301..1aa6cde74 100644 --- a/src/Dev/Backtrace.php +++ b/src/Dev/Backtrace.php @@ -86,6 +86,7 @@ class Backtrace $frame['function'] == $fnSpec[1] ) { $match = true; + break; } } } else { From d448622ff4e47e136cbc20104a1f6a82f17e860d Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Thu, 23 Jun 2022 15:17:33 +1200 Subject: [PATCH 4/6] ENH Allow subclasses to be defined for backtrace filtered functions. --- src/Dev/Backtrace.php | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/Dev/Backtrace.php b/src/Dev/Backtrace.php index 1aa6cde74..deae15921 100644 --- a/src/Dev/Backtrace.php +++ b/src/Dev/Backtrace.php @@ -81,9 +81,9 @@ class Backtrace $match = false; if (!empty($frame['class'])) { foreach ($ignoredArgs as $fnSpec) { - if (is_array($fnSpec) && - ('*' == $fnSpec[0] || $frame['class'] == $fnSpec[0]) && - $frame['function'] == $fnSpec[1] + if (is_array($fnSpec) + && self::matchesFilterableClass($frame['class'], $fnSpec[0]) + && $frame['function'] == $fnSpec[1] ) { $match = true; break; @@ -202,4 +202,13 @@ class Backtrace } return $result; } + + /** + * Checks if the filterable class is wildcard, of if the class name is the filterable class, or a subclass of it, + * or implements it. + */ + private static function matchesFilterableClass(string $className, string $filterableClass): bool + { + return $filterableClass === '*' || $className === $filterableClass || is_subclass_of($className, $filterableClass); + } } From 74e5a94b32f2f48b037795374c637c572166d438 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Thu, 23 Jun 2022 15:18:28 +1200 Subject: [PATCH 5/6] ENH Update list of methods to have filtered args in backtrace. --- _config/backtrace.yml | 58 +++++++++++++++++++++++++------------------ 1 file changed, 34 insertions(+), 24 deletions(-) diff --git a/_config/backtrace.yml b/_config/backtrace.yml index 13c83889e..dd90eabbe 100644 --- a/_config/backtrace.yml +++ b/_config/backtrace.yml @@ -3,31 +3,41 @@ Name: framework-backtrace --- SilverStripe\Dev\Backtrace: ignore_function_args: - - 'mysql_connect' - 'mssql_connect' + - 'mysql_connect' - 'pg_connect' - - ['PDO', '__construct'] - ['mysqli', 'mysqli'] - - ['mysqli', 'select_db'] - ['mysqli', 'real_connect'] - - ['SilverStripe\\ORM\\DB', 'connect'] - - ['SilverStripe\\Security\\Security', 'check_default_admin'] - - ['SilverStripe\\Security\\Security', 'encrypt_password'] - - ['SilverStripe\\Security\\Security', 'setDefaultAdmin'] - - ['SilverStripe\\ORM\\DB', 'createDatabase'] - - ['SilverStripe\\Security\\Member', 'checkPassword'] - - ['SilverStripe\\Security\\Member', 'changePassword'] - - ['SilverStripe\\Security\\MemberAuthenticator\\MemberAuthenticator', 'checkPassword'] - - ['SilverStripe\\Security\\MemberPassword', 'checkPassword'] - - ['SilverStripe\\Security\\PasswordValidator', 'validate'] - - ['SilverStripe\\Security\\PasswordEncryptor_PHPHash', 'encrypt'] - - ['SilverStripe\\Security\\PasswordEncryptor_PHPHash', 'salt'] - - ['SilverStripe\\Security\\PasswordEncryptor_LegacyPHPHash', 'encrypt'] - - ['SilverStripe\\Security\\PasswordEncryptor_LegacyPHPHash', 'salt'] - - ['SilverStripe\\Security\\PasswordEncryptor_MySQLPassword', 'encrypt'] - - ['SilverStripe\\Security\\PasswordEncryptor_MySQLPassword', 'salt'] - - ['SilverStripe\\Security\\PasswordEncryptor_MySQLOldPassword', 'encrypt'] - - ['SilverStripe\\Security\\PasswordEncryptor_MySQLOldPassword', 'salt'] - - ['SilverStripe\\Security\\PasswordEncryptor_Blowfish', 'encrypt'] - - ['SilverStripe\\Security\\PasswordEncryptor_Blowfish', 'salt'] - - ['*', 'updateValidatePassword'], + - ['mysqli', 'select_db'] + - ['PDO', '__construct'] + - ['SilverStripe\Control\Middleware\ConfirmationMiddleware\GetParameter', buildConfirmationItem] + - ['SilverStripe\Control\Middleware\ConfirmationMiddleware\Url', buildConfirmationItem] + - ['SilverStripe\Control\Middleware\ConfirmationMiddleware\UrlPathStartswith', buildConfirmationItem] + - ['SilverStripe\Core\Startup\AbstractConfirmationToken', 'checkToken'] + - ['SilverStripe\Core\Startup\AbstractConfirmationToken', 'pathForToken'] + - ['SilverStripe\Core\Startup\AbstractConfirmationToken', 'prepare_tokens'] + - ['SilverStripe\ORM\DB', 'connect'] + - ['SilverStripe\ORM\DB', 'createDatabase'] + - ['SilverStripe\Security\Confirmation\Item', '__construct'] + - ['SilverStripe\Security\DefaultAdminService', 'isDefaultAdminCredentials'] + - ['SilverStripe\Security\DefaultAdminService', 'setDefaultAdmin'] + - ['SilverStripe\Security\Member', 'changePassword'] + - ['SilverStripe\Security\MemberAuthenticator\ChangePasswordHandler', 'setSessionToken'] + - ['SilverStripe\Security\MemberAuthenticator\CookieAuthenticationHandler', 'setTokenCookieName'] + - ['SilverStripe\Security\MemberAuthenticator\CookieAuthenticationHandler', 'setTokenCookieSecure'] + - ['SilverStripe\Security\MemberAuthenticator\LostPasswordHandler', 'sendEmail'] + - ['SilverStripe\Security\PasswordEncryptor', 'check'] + - ['SilverStripe\Security\PasswordEncryptor', 'encrypt'] + - ['SilverStripe\Security\PasswordEncryptor', 'salt'] + - ['SilverStripe\Security\PasswordEncryptor_Blowfish', 'encryptA'] + - ['SilverStripe\Security\PasswordEncryptor_Blowfish', 'encryptX'] + - ['SilverStripe\Security\PasswordEncryptor_Blowfish', 'encryptY'] + - ['SilverStripe\Security\PasswordValidator', 'validate'] + - ['SilverStripe\Security\RememberLoginHash', 'setToken'] + - ['SilverStripe\Security\Security', 'check_default_admin'] + - ['SilverStripe\Security\Security', 'encrypt_password'] + - ['SilverStripe\Security\Security', 'setDefaultAdmin'] + - ['*', 'checkPassword'] + - ['*', 'onAfterChangePassword'] + - ['*', 'onBeforeChangePassword'] + - ['*', 'updateValidatePassword'] From 86cf4049448357e2113eefc224fbf0b8b063944f Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Thu, 23 Jun 2022 15:54:36 +1200 Subject: [PATCH 6/6] MNT Add test for backtrace class checking method. --- tests/php/Dev/BacktraceTest.php | 57 +++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/tests/php/Dev/BacktraceTest.php b/tests/php/Dev/BacktraceTest.php index 89f3c3285..de44410d1 100644 --- a/tests/php/Dev/BacktraceTest.php +++ b/tests/php/Dev/BacktraceTest.php @@ -2,8 +2,13 @@ namespace SilverStripe\Dev\Tests; +use ReflectionMethod; +use SilverStripe\Core\BaseKernel; +use SilverStripe\Core\CoreKernel; +use SilverStripe\Core\Kernel; use SilverStripe\Dev\Backtrace; use SilverStripe\Dev\SapphireTest; +use SilverStripe\ORM\DataObject; class BacktraceTest extends SapphireTest { @@ -109,4 +114,56 @@ class BacktraceTest extends SapphireTest $this->assertEquals('', $filtered[1]['args']['password']); $this->assertEquals('myval', $filtered[2]['args']['myarg']); } + + public function matchesFilterableClassProvider(): array + { + return [ + [ + 'anything', + '*', + true, + 'Wildcard counts as a match', + ], + [ + DataObject::class, + BaseKernel::class, + false, + 'No match', + ], + [ + DataObject::class, + DataObject::class, + true, + 'Exact match', + ], + [ + CoreKernel::class, + BaseKernel::class, + true, + 'Subclass counts as a match', + ], + [ + BaseKernel::class, + CoreKernel::class, + false, + 'Superclass does not count as a match', + ], + [ + CoreKernel::class, + Kernel::class, + true, + 'Implements interface counts as a match', + ], + ]; + } + + /** + * @dataProvider matchesFilterableClassProvider + */ + public function testMatchesFilterableClass(string $className, string $filterableClass, bool $expected, string $message): void + { + $reflectionMethod = new ReflectionMethod(Backtrace::class . '::matchesFilterableClass'); + $reflectionMethod->setAccessible(true); + $this->assertSame($expected, $reflectionMethod->invoke(null, $className, $filterableClass), $message); + } }